Change semantic of OR of two permission classes by smithdc1 · Pull Request #7522 · encode/django-rest-framework (original) (raw)
Follow on from #6605
- Rebase
- Fixed test
- Added additional regression test (fail on
masterbranch, pass with this change).
Closes #6605
Closes #7601
Closes #7117
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.
I'm going to merge this one for 3.14.0 unless there are any objections.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also LGTM
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM too
lunika added a commit to openfun/marsha that referenced this pull request
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
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
- 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 markyu98@outlook.com Co-authored-by: Tom Christie tom@tomchristie.com
I agree with this fix, but I'm surprised that it didn't get a mention in the release notes