-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
Conversation
8d14813
to
319238c
Compare
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.
rest_framework/permissions.py
Outdated
if hasperm1 is NotImplemented: | ||
hasperm1 = self.op1.has_permission(request, view) |
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.
What would happen if these lines are removed?
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.
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.
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. |
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. |
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 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. |
Description
This is a fix for issues #7117 and #6598.
Previously
has_object_permission
defaulted to returningTrue
when unimplemented because it was assumed thathas_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 permissionIsOwner
that implementshas_object_permissions
andIsAdminUser
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 insteadIsOwner
will passhas_permission
andIsAdminUser
will passhas_object_permission
even though it wouldn't have passedhas_permission
.One solution would be to default
has_object_permission
to callinghas_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, theIsOwner
permission above only implementedhas_object_permission
and nothas_permission
, but~IsOwner
will always fail, even on objects that the authenticated user doesn't own, because the defaultTrue
fromBasePermission.has_permission()
will also be inverted.My solution is to be more explicit about when a permission subclass doesn't implement
has_permission
orhas_object_permission
by returningNotImplemented
.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 beTrue
orFalse
. I modifiedAND
andOR
so that if one side returnsNotImplemented
, it defers to the other. If both sides returnNotImplemented
,NotImplemented
will be returned to propagate upwards. NOT will also propagateNotImplemented
instead of inverting it. IfNotImplemented
propagates all the way up toAPIView.check_permissions
orAPIView.check_object_permissions
, it is considered a pass (truthy).