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.

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

make api.user.has_permission work with permission ids too #401

Open
frisi opened this issue Mar 28, 2018 · 4 comments
Open

make api.user.has_permission work with permission ids too #401

frisi opened this issue Mar 28, 2018 · 4 comments
Assignees

Comments

@frisi
Copy link
Contributor

frisi commented Mar 28, 2018

currently api.user.has_permission('cmf.ModifyPortalContent') returns True if logged in as Manager and False for users having the Modify portal content permission.

problem: the method can only handle permission titles and silently fails when called with permission ids (https://github.com/zopefoundation/Products.CMFCore/blob/2.3.0/Products/CMFCore/permissions.zcml#L30)

i previously used the following code in my project:

from AccessControl.security import checkPermission

def allowed(context):
    return checkPermission('cmf.ModifyPortalContent', context)

when i started to replace this checks with plone.api.user.has_permission i did not notice there is a problem first because i was logged in as manager user.
now our customer found out that ui elements where missing for content editors because the updated permission checks did not work properly.

this is why i'd suggest to make plone.api.user.has_permission to work with permission titles as well as ids
if that is not possible it would also help when plone.api.user.has_permission('invalid permission string', context) raises an exception if the permission is not valid (i can file another ticket for this if you agree with that)

some pseudo-test-code:

setRoles(['Contributor'])
assertEqual(
   api.user.has_permission('cmf.ModifyPortalContent'),
   api.user.has_permission('Modify portal content'),
assertRaises(
  api.user.has_permission,
  'non existent permission',
  PermissionNotFoundError
) 

plone.api.user.has_permission('invalid permission string', context) should raise an exception if the permission id is not valid (i can file another ticket if you agree with that)

@frisi
Copy link
Contributor Author

frisi commented Mar 28, 2018

i also assign @jensens because he recently contributed a change for has_permission (c950845)

@jensens
Copy link
Member

jensens commented Mar 28, 2018

I agree we should support both writings. It is confusing enough to have those two different ways to do the same. Also, checking if the permission exists at all is a very good feature and would make problems with typos in permissions visible early.

@jaroel
Copy link
Member

jaroel commented Mar 30, 2018

Also see #372

@mauritsvanrees
Copy link
Member

mauritsvanrees commented Mar 30, 2018

👍 for supporting both, and for raising a ValueError or a more specific error in case no such permission is found.
I would say the same exception should be raised when this check is done for a Manager.
Is anyone up for making a PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants