-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix delete button from showing up in collections edit view for users with insufficient permissions #11964
Fix delete button from showing up in collections edit view for users with insufficient permissions #11964
Conversation
…with insufficient permissions 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.
Manage this branch in SquashTest this branch here: https://laymonagefix-collections-delet-nh1ms.squash.io |
) | ||
.filter(pk=self.object.pk) | ||
.first() | ||
context["can_delete"] = self.permission_policy.user_has_permission_for_instance( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 {% 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
orDeleteAction
subclass if we had one...
- 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
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.
I think the correct fix here is to fix the generic EditView to set |
…l permissions. Fixes wagtail#10084, supersedes wagtail#11964
Regression in 45ac80b. The template no longer checks for
can_delete
. Instead, it generates thedelete_url
based oncan_delete
in the genericEditView
'sget_context_data()
. As a result, any modification to thecan_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.