Skip to content

Handle composition of unimplemented permission methods correctly #7601

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

Closed
wants to merge 6 commits into from

Conversation

sparkyb
Copy link

@sparkyb sparkyb commented Oct 17, 2020

Description

This is a fix for issues #7117 and #6598.

Previously has_object_permission defaulted to returning True when unimplemented because it was assumed that has_permission had already returned true, but when composed with OR, this might not be the case. Take for example the case where you want an object to be accessible by the user that created it or any admin. If you have a permission IsOwner that implements has_object_permissions and IsAdminUser which doesn't, then if you OR them together and have a user that is neither the owner nor the admin they should get denied but instead IsOwner will pass has_permission and IsAdminUser will pass has_object_permission even though it wouldn't have passed has_permission.

One solution would be to default has_object_permission to calling has_permission. This will return the expected value in all cases, but would potentially cause redundant database lookups even when no composition is used. Alternatively this could be done only in the OR class as with PR #7155 , but the redundant calls will still happen and incorrect behavior can still occur using more complicated compositions including NOT (See issue #6598). For example, the IsOwner permission above only implemented has_object_permission and not has_permission, but ~IsOwner will always fail, even on objects that the authenticated user doesn't own, because the default True from BasePermission.has_permission() will also be inverted.

My solution is to be more explicit about when a permission subclass doesn't implement has_permission or has_object_permission by returning NotImplemented. NotImplemented is truthy in a boolean context, so by default everything will continue to work the same as long as code is not expecting the result to literally be True or False. I modified AND and OR so that if one side returns NotImplemented, it defers to the other. If both sides return NotImplemented, NotImplemented will be returned to propagate upwards. NOT will also propagate NotImplemented instead of inverting it. If NotImplemented propagates all the way up to APIView.check_permissions or APIView.check_object_permissions, it is considered a pass (truthy).

@sparkyb sparkyb force-pushed the permissions branch 3 times, most recently from 8d14813 to 319238c Compare May 6, 2021 19:03
@sparkyb sparkyb changed the title Handle unimplemented has_object_permission composition correctly Handle composition of unimplemented permission methods correctly May 6, 2021
sparkyb added 4 commits May 7, 2021 10:00
Previously has_object_permission defaulted to returning True when
unimplemented because it was assumed that has_permission had already returned
true, but when composed with OR, this might not be the case. Take for example
the case where you want an object to be accessible by the user that created it
or any admin. If you have a permission IsOwner that implements
has_object_permission and IsAdminUser which doesn't, then if you OR them
together an have a user that is neither the owner nor the admin they should
get denied but instead IsOwner will pass has_permission and IsAdminUser will
pass has_object_permission even though it didn't pass has_permission.

One solution would be to default has_object_permission to calling
has_permission but that would potentially cause redundant database lookups.
Alternatively this could be done only in the OR class, but the redundant
calls will still happen.

Also, incorrect behavior can still occur using more complicated compositions
including NOT. For example, the IsOwner permission above only implemented
has_object_permission and not has_permission, but ~IsOwner will always fail,
even on objects that the authenticated user doesn't own, because the default
True from BasePermission.has_permission() will also be inverted.

My solution is to be more explicit about when a permission subclass doesn't
implement has_permission or has_object_permission by returning NotImplemented.
NotImplemented is truthy in a boolean context, so by default everything will
continue to work the same as long as code is not expecting the result to
literally be True or False. I modified AND and OR so that if one side returns
NotImplemented, it defers to the other. If both sides return NotImplemented,
NotImplmented will be returned to propogate upwards. NOT will also propagate
NotImplmented instead of inverting it. If NotImplemented propagates
all the way up to APIView.check_permissions or
APIView.check_object_permissions, it is considered a pass (truthy).
The previous test ensured that a permission whose has_permission returned
False but didn't implement has_object_permission didn't cause an OR to
suceed regardless of other conditions. However, if a permission doesn't
implement has_object_permission but has_permission returns True, that should
count as having permission on all objects.
…rmission

Composing permissions with OR should fall back on has_permission when
has_object_permission isn't implemented. If has_object_permission is
not implemented, than its has_permission should apply to all objects.
Strictly speaking, BasePermission.has_object_permission could return
self.has_permission(request, view), but only in the OR case is it possible
that wasn't already true, so only doing that fallback in the OR case
saves some redundant calls to has_permission.
Handling permissions with OR is tricky. If a permission only implements
has_object_permission, we wanted has_permission to return NotImplemented
instead of True, so that inverting it does what you would expect
(invert the object-level logic, don't invert the default has_permission True
so the object-level logic is never checked). However, having OR ignore
NotImplemented means that if the thing it is ORed with returns False, an
object-level only permission will never get the chance to check its
object-level logic. Rather than being ignored, NotImplemented should be
treated as true logic-wise (it is in Python) but with lower precedence than
any other truthy value.
Comment on lines 84 to 85
if hasperm1 is NotImplemented:
hasperm1 = self.op1.has_permission(request, view)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would happen if these lines are removed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TL;DR If I wrote my tests right, if you remove these two lines and run the tests, you'll see exactly why they are needed.

Long version: If has_object_permission returns NotImplemented it should fall back on has_permission, but this is only necessary in the OR case. With a non-composed permission or an AND case, calling has_permission is redundant because you know it must have returned True in the original has_permission check to make it to check the object. But with OR, it's possible that has_permission is False and the original check passed only because of the other side of the OR. We don't want the has_object_permission test to pass for the wrong reason.

In Python 3.9, evaluating NotImplemented in a boolean context is deprecated.
So permissions must use a custom truthy value instead to indicate that
`has_permission` defers granting/denying access until object-level permissions
can be checked and vice-versa, `has_object_permission` defers permission to
the view-level permission.

Also, updated docstrings and documentation.
@stale
Copy link

stale bot commented Apr 24, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 24, 2022
@sparkyb
Copy link
Author

sparkyb commented Jun 21, 2022

Can someone please look at this PR? It hasn't been looked at in over a year and I'd really like to get permission composition working correctly. I know I'm not the only one.

@stale stale bot removed the stale label Jun 21, 2022
@tomchristie
Copy link
Member

From review, the approach in #6605 looks good to me.

Could we work through seeing if there are any objections to that? I think we could also consider placing some of this on a deprecation path. (Perhaps the "NOT" operand?)

@sparkyb
Copy link
Author

sparkyb commented Sep 21, 2022

From review, the approach in #6605 looks good to me.

Could we work through seeing if there are any objections to that? I think we could also consider placing some of this on a deprecation path. (Perhaps the "NOT" operand?)

For some reason GitHub seems to not notify me of all comments, so I didn't see this comment/question until now when you closed this PR. That is unfortunate because I disagree with the approach in #6605/#7522. It's definitely the simpler change, and it is better than what we had before, but it doesn't seem semantically correct to me. For starters, it still doesn't handle NOT correctly. Fundamentally, it feels wrong for a permission that only intends to implement one of the two methods to return True in the other. I feel strongly that having a way to mark one of them as not implemented makes a lot more sense. It also prevents some unnecessary redundant calling of has_permission.

This PR may be slightly harder to reason about, but the beauty of it is that once you convince yourself it is correct, this is hidden in base classes and any custom permission classes will be very easy to reason about and always do what you would expect, because only the methods you choose to implement would have any effect on the result (you won't have to reason about methods you didn't write unintentionally allowing access because they returned True by default).

I have been monkey-patching this behavior into my project for over a year now and will continue to do so because I want the correct behavior. I urge you to reconsider supporting correct semantics for all DRF users. If you have any questions or concerns that this could have any unintended drawbacks, I'd be happy to convince you that's not the case.

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

Successfully merging this pull request may close these issues.

3 participants