Change semantic of OR of two permission classes #7522
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The original semantic of OR is defined as: the request pass either of the two has_permission() check, and pass either of the two has_object_permission() check, which could lead to situations that a request passes has_permission() but fails on has_object_permission() of Permission Class A, fails has_permission() but passes has_object_permission() of Permission Class B, passes the OR permission check. This should not be the desired permission check semantic in applications, because such a request should fail on either Permission Class (on Django object permission) alone, but passes the OR or the two. My code fix this by changing the semantic so that the request has to pass either class's has_permission() and has_object_permission() to get the Django object permission of the OR check.
|
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. |
tomchristie
reviewed
Jul 1, 2022
6 tasks
|
I'm going to merge this one for 3.14.0 unless there are any objections. |
tim-schilling
approved these changes
Aug 12, 2022
There was a problem hiding this comment.
I looked at #6402 and agree that this solves the bug. I was worried that there may be an issue with people migrating to v3.14.0 because of this, but there shouldn't be unless someone was making use of the generous permissions the bug was allowing.
Another idea would be to have a "evaluate_permission" method on BasePermission that makes it more explicit that both has_permission and has_object_permission are used when possible. Though that's a fair amount of rework and a change to the API which is likely out of scope for DRF.
|
@tomchristie can you merge this in? |
lunika
added a commit
to openfun/marsha
that referenced
this pull request
Sep 26, 2022
Since DRF 3.14.0 it is not possible to use view.get_object in the has_permission method. If we do it, we have a max recursion error. In this version, they fix a bug related to the usage of th OR and AND operator. To fix this issue, we created a dedicated mixin responsible to fetch the related object we are working on in the database. encode/django-rest-framework#7522
lunika
added a commit
to openfun/marsha
that referenced
this pull request
Sep 26, 2022
Since DRF 3.14.0 it is not possible to use view.get_object in the has_permission method. If we do it, we have a max recursion error. In this version, they fix a bug related to the usage of th OR and AND operator. To fix this issue, we created a dedicated mixin responsible to fetch the related object we are working on in the database. encode/django-rest-framework#7522
sigvef
pushed a commit
to sigvef/django-rest-framework
that referenced
this pull request
Dec 3, 2022
* Change semantic of OR of two permission classes The original semantic of OR is defined as: the request pass either of the two has_permission() check, and pass either of the two has_object_permission() check, which could lead to situations that a request passes has_permission() but fails on has_object_permission() of Permission Class A, fails has_permission() but passes has_object_permission() of Permission Class B, passes the OR permission check. This should not be the desired permission check semantic in applications, because such a request should fail on either Permission Class (on Django object permission) alone, but passes the OR or the two. My code fix this by changing the semantic so that the request has to pass either class's has_permission() and has_object_permission() to get the Django object permission of the OR check. * Update rest_framework/permissions.py * Update setup.cfg Co-authored-by: Mark Yu <[email protected]> Co-authored-by: Tom Christie <[email protected]>
|
I agree with this fix, but I'm surprised that it didn't get a mention in the release notes |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow on from #6605
masterbranch, pass with this change).Closes #6605
Closes #7601
Closes #7117