Handle composition of unimplemented permission methods correctly

[sparkyb]

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).

[stale[bot]]

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.

[sparkyb]

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.

[lovelydinosaur]

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]

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.