Fix delete button from showing up in collections edit view for users with insufficient permissions

[laymonage]

Regression in 45ac80b. The template no longer checks for can_delete. Instead, it generates the delete_url based on can_delete in the generic EditView's get_context_data(). As a result, any modification to the can_delete context value by a subclass is ignored.

Also add a small optimisation by using user_has_any_permission_for_instance instead of making a query for objects that the user has delete permission for and fetching the object with the same ID.

Fixes #10084.

[squash-labs]

Manage this branch in Squash

Test this branch here: https://laymonagefix-collections-delet-nh1ms.squash.io

[laymonage]

A different way to solve this would be to pass can_delete as a kwarg to super().get_context_data(), so that they are added to the context before the rest of the generic EditView.get_context_data() runs. Then, in the generic EditView, we'll need to change this line to only set the can_delete value if it is not already in the context:

wagtail/wagtail/admin/views/generic/models.py

Lines 920 to 923 in bf3f87b

	context["can_delete"] = (
	self.permission_policy is None
	or self.permission_policy.user_has_permission(self.request.user, "delete")
	)

That way, we don't need to add the can_delete check to the template.

However, this means all subclasses that override can_delete need to be updated to use this pattern. It's not a big deal though, as within Wagtail itself, the only other EditView subclass that overrides can_delete I can find is the user edit view. Even so, it does not have this issue as it uses a custom template.

It overrides the can_delete here:

wagtail/wagtail/users/views/users.py

Lines 330 to 333 in bf3f87b

	def get_context_data(self, **kwargs):
	context = super().get_context_data(**kwargs)
	context["can_delete"] = self.can_delete
	return context

Using the following user_can_delete_user util (which by the way, could be implemented via a ModelPermissionTester or DeleteAction subclass if we had one...)

wagtail/wagtail/users/views/users.py

Lines 291 to 295 in bf3f87b

	def setup(self, request, *args, **kwargs):
	super().setup(request, *args, **kwargs)
	self.object = self.get_object()
	self.can_delete = user_can_delete_user(request.user, self.object)
	self.editing_self = request.user == self.object

wagtail/wagtail/users/utils.py

Lines 13 to 25 in bf3f87b

	def user_can_delete_user(current_user, user_to_delete):
	if not current_user.has_perm(delete_user_perm):
	return False
	if current_user == user_to_delete:
	# users may not delete themselves
	return False
	if user_to_delete.is_superuser and not current_user.is_superuser:
	# ordinary users may not delete superusers
	return False
	return True

But the view uses a custom template that checks can_delete, so it doesn't have the same issue:

wagtail/wagtail/users/templates/wagtailusers/users/edit.html

Lines 70 to 72 in bf3f87b

	{% if can_delete %}
	<a href="{% url 'wagtailusers_users:delete' user.pk %}" class="button no">{% trans "Delete user" %}</a>
	{% endif %}

Although, I suppose the template can use a bit more refactoring e.g. using delete_url instead of {% url %}... I considered doing this in 4d305d2, but ultimately decided not to, to avoid unnecessary risks and to make the PR more focused on just introducing the UserViewSet. I'd be happy to do it in a separate PR, though!

I also found a few other view subclasses that have can_{foo} context, but from what I've seen they seem to add these context values rather than modify existing ones. For example:

• can_delete in locale delete view
  • The generic delete view does not have this context variable. The locale delete view uses this to render a message when deleting the locale is not allowed

    wagtail/wagtail/locales/views.py

    Lines 68 to 86 in bf3f87b

    	def can_delete(self, locale):
    	if not self.queryset.exclude(pk=locale.pk).exists():
    	self.cannot_delete_message = gettext_lazy(
    	"This locale cannot be deleted because there are no other locales."
    	)
    	return False
    	if get_locale_usage(locale) != (0, 0):
    	self.cannot_delete_message = gettext_lazy(
    	"This locale cannot be deleted because there are pages and/or other objects using it."
    	)
    	return False
    	return True
    	def get_context_data(self, object=None):
    	context = super().get_context_data()
    	context["can_delete"] = self.can_delete(object)
    	return context

    wagtail/wagtail/locales/templates/wagtaillocales/confirm_delete.html

    Lines 5 to 13 in bf3f87b

    	{% if can_delete %}
    	<p>{{ view.confirmation_message }}</p>
    	<form action="{{ view.get_delete_url }}" method="POST">
    	{% csrf_token %}
    	<input type="submit" value="{% trans 'Yes, delete' %}" class="button serious" />
    	<a href="{% url 'wagtaillocales:edit' locale.id %}" class="button button-secondary">{% trans "Back" %}</a>
    	</form>
    	{% else %}

  • Note that the message is rendered in the template. This means the view will load normally (with 200 status code) instead of a redirect with that message displayed as an error banner at the top like we usually do when the user does not have enough permissions.
  • Also, again, this additional logic could be implemented via a ModelPermissionTester or DeleteAction subclass if we had one...
• can_enable/can_disable in workflows and tasks edit view
  • These are domain-specific, since you can't delete workflows and tasks. You can only disable them. The templates make use of these additional variables.

[gasman]

I think the correct fix here is to fix the generic EditView to set context["can_delete"] based on user_has_permission_for_instance instead of user_has_permission. I suspect that the developer who originally migrated the collection management views to generic views noticed that this wasn't accounting for instance-level permissions, and fixed it by patching the logic in the collection view rather than dealing with the underlying problem. Will open a new PR to that effect in a moment...