-
-
Notifications
You must be signed in to change notification settings - Fork 202
[RECYCLE BIN] Implement permission for accessing recyclebin #4176
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
base: rohnsha0-plip-recyclebin
Are you sure you want to change the base?
Conversation
…dd tests for permission checks
…g items in the recycle bin
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.
Casing of recycle bin. Otherwise this looks good. Needs a technical review.
…anage recycle bin"
@@ -35,6 +35,10 @@ def __init__(self, context, request): | |||
super().__init__(context, request) | |||
self.recycle_bin = getUtility(IRecycleBin) | |||
|
|||
def _get_context(self): | |||
"""Get the context (Plone site)""" | |||
return self.context |
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.
This doesn't appear to be used.
Products/CMFPlone/recyclebin.py
Outdated
# Additional check for Manager or Site Administrator roles | ||
user = sm.getUser() | ||
user_roles = user.getRolesInContext(context) | ||
return "Manager" in user_roles or "Site Administrator" in user_roles |
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.
Remove the explicit check for roles. That's already included in the checkPermission call (it checks that the current user has one of the roles for which the permission has been granted)
Products/CMFPlone/recyclebin.py
Outdated
@@ -125,6 +125,9 @@ def get_items_sorted_by_date(self, reverse=True): | |||
class RecycleBin: | |||
"""Stores deleted content items""" | |||
|
|||
# Permission for managing recycle bin | |||
MANAGE_RECYCLEBIN = "Products.CMFPlone.ManageRecycleBin" |
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.
This should be "Manage recycle bin". checkPermission
uses the permission's title, not its id
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.
done
Products/CMFPlone/recyclebin.py
Outdated
1. They have the ManageRecycleBin permission, or | ||
2. They are a Manager or Site Administrator, or | ||
3. They are the owner of the item, or | ||
4. They deleted the item |
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.
3 and 4 are a nice idea, but currently the "Manage recycle bin" permission is only granted to Managers and Site Administrators, which means that the recycle bin can't be accessed by users who don't have one of those roles.
There are a lot of details to get right here. I strongly suggest we focus on making this work only for Managers and Site Administrators for now, and then come back to it later if we want to provide more filtered access to other users.
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.
done
will soon create an issue for tracking
edit: #4185
# Log in as the test user | ||
user = self.portal.acl_users.getUser("testuser") | ||
user = user.__of__(self.portal.acl_users) | ||
newSecurityManager(None, user) |
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.
Use the login function from plone.app.testing
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.
done
fixes #4173
includes:-