From 2d4858af3bfd41972813722006a82ec3619522ca Mon Sep 17 00:00:00 2001 From: Mark Yu Date: Wed, 24 Apr 2019 22:25:37 +0800 Subject: [PATCH 1/3] 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. --- rest_framework/permissions.py | 4 ++-- tests/test_permissions.py | 15 ++++++++++++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/rest_framework/permissions.py b/rest_framework/permissions.py index 3a8c580646..db678c228b 100644 --- a/rest_framework/permissions.py +++ b/rest_framework/permissions.py @@ -78,8 +78,8 @@ def has_permission(self, request, view): def has_object_permission(self, request, view, obj): return ( - self.op1.has_object_permission(request, view, obj) or - self.op2.has_object_permission(request, view, obj) + (self.op1.has_permission(request, view) and self.op1.has_object_permission(request, view, obj)) or + (self.op2.has_permission(request, view) and self.op2.has_object_permission(request, view, obj)) ) diff --git a/tests/test_permissions.py b/tests/test_permissions.py index 4e6cae4b81..f00b57ec19 100644 --- a/tests/test_permissions.py +++ b/tests/test_permissions.py @@ -635,7 +635,7 @@ def test_object_or_lazyness(self): composed_perm = (permissions.IsAuthenticated | permissions.AllowAny) hasperm = composed_perm().has_object_permission(request, None, None) assert hasperm is True - assert mock_deny.call_count == 1 + assert mock_deny.call_count == 0 assert mock_allow.call_count == 1 def test_and_lazyness(self): @@ -677,3 +677,16 @@ def test_object_and_lazyness(self): assert hasperm is False assert mock_deny.call_count == 1 mock_allow.assert_not_called() + + def test_unimplemented_has_object_permission(self): + "test for issue 6402 https://github.com/encode/django-rest-framework/issues/6402" + request = factory.get('/1', format='json') + request.user = AnonymousUser() + + class IsAuthenticatedUserOwner(permissions.IsAuthenticated): + def has_object_permission(self, request, view, obj): + return True + + composed_perm = (IsAuthenticatedUserOwner | permissions.IsAdminUser) + hasperm = composed_perm().has_object_permission(request, None, None) + assert hasperm is False From 34d33c3dee2ff5e00e3d63c1bd057591fd68503d Mon Sep 17 00:00:00 2001 From: Tom Christie Date: Fri, 1 Jul 2022 10:33:49 +0100 Subject: [PATCH 2/3] Update rest_framework/permissions.py --- rest_framework/permissions.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/rest_framework/permissions.py b/rest_framework/permissions.py index db678c228b..16459b251f 100644 --- a/rest_framework/permissions.py +++ b/rest_framework/permissions.py @@ -78,8 +78,11 @@ def has_permission(self, request, view): def has_object_permission(self, request, view, obj): return ( - (self.op1.has_permission(request, view) and self.op1.has_object_permission(request, view, obj)) or - (self.op2.has_permission(request, view) and self.op2.has_object_permission(request, view, obj)) + self.op1.has_permission(request, view) + and self.op1.has_object_permission(request, view, obj) + ) or ( + self.op2.has_permission(request, view) + and self.op2.has_object_permission(request, view, obj) ) From 39b54efd083a05d586c9a81384c0a27020f3be5f Mon Sep 17 00:00:00 2001 From: Tom Christie Date: Fri, 1 Jul 2022 10:38:15 +0100 Subject: [PATCH 3/3] Update setup.cfg --- setup.cfg | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.cfg b/setup.cfg index abb7cca908..0b0fec5a7e 100644 --- a/setup.cfg +++ b/setup.cfg @@ -6,7 +6,7 @@ addopts=--tb=short --strict -ra testspath = tests [flake8] -ignore = E501,W504 +ignore = E501,W503,W504 banned-modules = json = use from rest_framework.utils import json! [isort]