Skip to content

Commit 2d4858a

Browse files
MarkYu98smithdc1
authored andcommitted
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.
1 parent 04f39c4 commit 2d4858a

File tree

2 files changed

+16
-3
lines changed

2 files changed

+16
-3
lines changed

rest_framework/permissions.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,8 +78,8 @@ def has_permission(self, request, view):
7878

7979
def has_object_permission(self, request, view, obj):
8080
return (
81-
self.op1.has_object_permission(request, view, obj) or
82-
self.op2.has_object_permission(request, view, obj)
81+
(self.op1.has_permission(request, view) and self.op1.has_object_permission(request, view, obj)) or
82+
(self.op2.has_permission(request, view) and self.op2.has_object_permission(request, view, obj))
8383
)
8484

8585

tests/test_permissions.py

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -635,7 +635,7 @@ def test_object_or_lazyness(self):
635635
composed_perm = (permissions.IsAuthenticated | permissions.AllowAny)
636636
hasperm = composed_perm().has_object_permission(request, None, None)
637637
assert hasperm is True
638-
assert mock_deny.call_count == 1
638+
assert mock_deny.call_count == 0
639639
assert mock_allow.call_count == 1
640640

641641
def test_and_lazyness(self):
@@ -677,3 +677,16 @@ def test_object_and_lazyness(self):
677677
assert hasperm is False
678678
assert mock_deny.call_count == 1
679679
mock_allow.assert_not_called()
680+
681+
def test_unimplemented_has_object_permission(self):
682+
"test for issue 6402 https://github.com/encode/django-rest-framework/issues/6402"
683+
request = factory.get('/1', format='json')
684+
request.user = AnonymousUser()
685+
686+
class IsAuthenticatedUserOwner(permissions.IsAuthenticated):
687+
def has_object_permission(self, request, view, obj):
688+
return True
689+
690+
composed_perm = (IsAuthenticatedUserOwner | permissions.IsAdminUser)
691+
hasperm = composed_perm().has_object_permission(request, None, None)
692+
assert hasperm is False

0 commit comments

Comments
 (0)