plone / plone.api

The Plone API
https://6.docs.plone.org/plone.api
Other
86 stars 53 forks source link

Warn if the permission does not exist #515

Closed gforcada closed 4 months ago

gforcada commented 11 months ago

I was doing some tests using api.user.has_permission and the test was failing 💥 but of course I should be right 🤣

Turns out that I have a permission:

  <permission
    id="der.freitag.my-permission"
    title="der.freitag: Cool stuff"
    />

And my api call was:

api.user.has_permission(
    'der.freitag.my-permission',
    obj=self.portal.autoren[user1['USER_ID']])
)

But, actually this is indeed wrong, the right call is:

api.user.has_permission(
    'der.freitag: Cool stuff',
    obj=self.portal.autoren[user1['USER_ID']])
)

i.e. one has to use the title of the permission, not the id! 🤦🏾

It would be nice if either plone.api could use either of them, or otherwise warn the user if the permission does not exist at all.

I can hardly think of any valid/reasonable use case where you don't want the permission that you are checking to not exist.

As a first step, mentioning it on the documentation it would be helpful ✨

davisagli commented 9 months ago

It would be nice if either plone.api could use either of them

This doesn't sound nice to me; there could be some edge case where the id of one permission matches the title of a different one, and then you are checking the wrong one and have a security bug.

otherwise warn the user if the permission does not exist at all.

This does sound helpful

ale-rt commented 4 months ago

It would be nice if either plone.api could use either of them

This doesn't sound nice to me; there could be some edge case where the id of one permission matches the title of a different one, and then you are checking the wrong one and have a security bug.

I agree.

Maybe that could be achieved by having a function signature that would allow making that possible, something like:

@mutually_exclusive_parameters("username", "user")
@mutually_exclusive("permission", "permission_id")
def has_permission(permission=None, username=None, user=None, obj=None, permission_id=None):
     ...

I am slightly -1 on that because so far the need did not surface.

gforcada commented 4 months ago

I did not go that route, as you say, if it's asked later we can implement it.