Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Collections that a user cannot delete incorrectly show the Delete Collection button in the edit form #10084

Closed
coredumperror opened this issue Feb 14, 2023 · 6 comments
Closed

Collections that a user cannot delete incorrectly show the Delete Collection button in the edit form #10084

coredumperror opened this issue Feb 14, 2023 · 6 comments
Assignees
@laymonage
Labels
component:Collections component:Permissions type:Bug

Comments

@coredumperror
Copy link
Contributor

coredumperror commented Feb 14, 2023 •
edited

Issue Summary

When a user without permission to delete a Collection views the edit form for said Collection, the "Delete Collection" button erroneously appears in the form.

Steps to Reproduce

  1. Give a user Edit but not Delete permission on Collection X.
  2. User attempts to edit the collection from /admin/collections/X/.
  3. User sees "Delete Collection" button, even though they're not allowed to delete it.
  4. Clicking that button actually triggers a different bug, which I reported in Attempting to delete a Collection you aren't permitted to delete confusingly throws a "not found" error #10085.

Proposed Solution

I've looked into why this happens, and I see the exact problem in the code:
The wagtailadmin/collections/edit.html template doesn't use the can_delete context var that collections.Edit.get_context_data() adds. It just uses the generic edit form template code, which hides the Delete button only if the delete_url var is falsey, ignoring can_delete entirely.

Fixing this should be as simple as overriding the actions block in wagtailadmin/collections/edit.html to make it take can_delete into account.
@coredumperror coredumperror added status:Unconfirmed Issue, usually a bug, that has not yet been validated as a confirmed problem. type:Bug labels Feb 14, 2023
@coredumperror coredumperror mentioned this issue Feb 14, 2023
Closed
@HimanshuGarg47
Copy link
Contributor

HimanshuGarg47 commented Feb 20, 2023 •
edited

I tried to reproduce issue in bakerydemo project in chrome
I created new user and give permissions of add, edit for collections.
In my case User does not sees "Delete Collection" button.
image

@coredumperror
Copy link
Contributor Author

I created new user and give permissions of add, edit for collections.

Did you give the user those permissions through the Group system, or give it to them directly on the User? I failed to mention that I'm talking about giving the user this permission through their Group, via granting Permission for a given Collection which then applies to all that Collection's descendants.

@HimanshuGarg47
Copy link
Contributor

Yes, I give user permissions through Group (in my case Editors group)

@laymonage
Copy link
Member

I can't replicate this on Wagtail 6.1. Since this issue was created, we did some work on the permission policies in Wagtail 5.1 so that might've fixed it. Could you confirm whether this is still an issue @coredumperror? Thanks!

@laymonage laymonage added status:Needs Info and removed status:Needs Info status:Unconfirmed Issue, usually a bug, that has not yet been validated as a confirmed problem. labels May 8, 2024
@laymonage
Copy link
Member

laymonage commented May 8, 2024 •
edited

Oh, actually, never mind. I was able to replicate this after assigning the delete permission to a different collection:

image image

The issue is because the can_delete check only happens in the generic EditView's get_context_data, and by the time the collections Edit view overrides it with instance-specific check, the delete_url is already generated.

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

Lines 914 to 920 in 6fa3985

context["can_delete"] = (
self.permission_policy is None
or self.permission_policy.user_has_permission(self.request.user, "delete")
)
if context["can_delete"]:
context["delete_url"] = self.get_delete_url()
context["delete_item_label"] = self.delete_item_label

wagtail/wagtail/admin/views/collections.py

Lines 143 to 149 in 6fa3985

context["can_delete"] = (
self.permission_policy.instances_user_has_permission_for(
self.request.user, "delete"
)
.filter(pk=self.object.pk)
.first()
)

@laymonage laymonage self-assigned this May 8, 2024
@coredumperror
Copy link
Contributor Author

Great! Glad you managed to track down the ultimate source. I never realized it required a second Collection to have delete perms.

@laymonage laymonage mentioned this issue May 20, 2024
Closed
gasman added a commit to gasman/wagtail that referenced this issue Jun 6, 2024
@gasman
Fix can_delete flag in generic edit view to account for instance-leve…
7d5a550 
…l permissions.

Fixes wagtail#10084, supersedes wagtail#11964
@gasman gasman mentioned this issue Jun 6, 2024
Closed
@gasman gasman closed this as completed in 88311ce Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@laymonage laymonage

Labels
component:Collections component:Permissions type:Bug
Projects
Wagtail 6.2* release planning
Status: ✅ Done
Milestone
No milestone
3 participants
@coredumperror @laymonage @HimanshuGarg47