From cc126e6d3b0c636dc48ec6519c219c15b62355c7 Mon Sep 17 00:00:00 2001 From: dog Date: Wed, 22 Jan 2020 03:24:03 -0800 Subject: [PATCH 1/3] Push a failing test for permission composition --- tests/test_permissions.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/test_permissions.py b/tests/test_permissions.py index 232c72dd26..a016089d74 100644 --- a/tests/test_permissions.py +++ b/tests/test_permissions.py @@ -311,6 +311,7 @@ class ObjectPermissionsIntegrationTests(TestCase): """ Integration tests for the object level permissions API. """ + def setUp(self): from guardian.shortcuts import assign_perm @@ -605,6 +606,16 @@ def test_several_levels_and_precedence(self): ) assert composed_perm().has_permission(request, None) is True + def test_has_object_permissions_not_short_circuited(self): + request = factory.get('/1', format='json') + request.user = self.user + composed_perm = ( + permissions.IsAdminUser | + BasicObjectPerm + ) + assert composed_perm().has_object_permission(request, None, None) is False + + @pytest.mark.skipif(not PY36, reason="assert_called_once() not available") def test_or_lazyness(self): request = factory.get('/1', format='json') request.user = AnonymousUser() From d88d06d43428a6d9ebd3c797b266d76ecb9fea29 Mon Sep 17 00:00:00 2001 From: Mark Yu Date: Wed, 24 Apr 2019 22:25:37 +0800 Subject: [PATCH 2/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 | 5 +++-- tests/test_permissions.py | 1 - 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/rest_framework/permissions.py b/rest_framework/permissions.py index 3a8c580646..a906fd089f 100644 --- a/rest_framework/permissions.py +++ b/rest_framework/permissions.py @@ -78,8 +78,9 @@ 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 a016089d74..19aba56a30 100644 --- a/tests/test_permissions.py +++ b/tests/test_permissions.py @@ -615,7 +615,6 @@ def test_has_object_permissions_not_short_circuited(self): ) assert composed_perm().has_object_permission(request, None, None) is False - @pytest.mark.skipif(not PY36, reason="assert_called_once() not available") def test_or_lazyness(self): request = factory.get('/1', format='json') request.user = AnonymousUser() From 5d463d444410b694266bc06e01096098ba63d2b2 Mon Sep 17 00:00:00 2001 From: Kevin Engle <5172592+Dog@users.noreply.github.com> Date: Mon, 18 May 2020 11:31:00 -0700 Subject: [PATCH 3/3] Fix test for new object permission resolution --- rest_framework/permissions.py | 9 ++++----- tests/test_permissions.py | 2 +- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/rest_framework/permissions.py b/rest_framework/permissions.py index a906fd089f..0d1b658cd7 100644 --- a/rest_framework/permissions.py +++ b/rest_framework/permissions.py @@ -77,11 +77,10 @@ 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)) - ) + op1 = self.op1.has_permission(request, view) and self.op1.has_object_permission(request, view, obj) + if op1: + return op1 + return self.op2.has_permission(request, view) and self.op2.has_object_permission(request, view, obj) class NOT: diff --git a/tests/test_permissions.py b/tests/test_permissions.py index 19aba56a30..6049e94f1e 100644 --- a/tests/test_permissions.py +++ b/tests/test_permissions.py @@ -648,7 +648,7 @@ def test_object_or_lazyness(self): mock_deny.assert_not_called() with mock.patch.object(permissions.AllowAny, 'has_object_permission', return_value=True) as mock_allow: - with mock.patch.object(permissions.IsAuthenticated, 'has_object_permission', return_value=False) as mock_deny: + with mock.patch.object(permissions.IsAuthenticated, 'has_permission', return_value=False) as mock_deny: composed_perm = (permissions.IsAuthenticated | permissions.AllowAny) hasperm = composed_perm().has_object_permission(request, None, None) assert hasperm is True