Handle composition of unimplemented permission methods correctly

docs/api-guide/permissions.md

@@ -1,4 +1,4 @@
----
+---
 source:
     - permissions.py
 ---
@@ -212,7 +212,8 @@ To implement a custom permission, override `BasePermission` and implement either
 * `.has_permission(self, request, view)`
 * `.has_object_permission(self, request, view, obj)`
 
-The methods should return `True` if the request should be granted access, and `False` otherwise.
+The methods should return `True` if the request should be granted access, and `False` if it should be denied. Most permission classes will only need to implement one of these methods. The base class implementations return a special truthy value called `Deferred` which is used to make and/or/not composition work correctly where `has_permission` should always
+succeed in order to let object permissions be checked and `has_object_permission` should defer to the view-level permission.
 
 If you need to test if a request is a read operation or a write operation, you should check the request method against the constant `SAFE_METHODS`, which is a tuple containing `'GET'`, `'OPTIONS'` and `'HEAD'`.  For example:
 

rest_framework/permissions.py

@@ -8,6 +8,9 @@
 SAFE_METHODS = ('GET', 'HEAD', 'OPTIONS')
 
 
+Deferred = object()
+
+
 class OperationHolderMixin:
     def __and__(self, other):
         return OperandHolder(AND, self, other)
@@ -53,16 +56,18 @@ def __init__(self, op1, op2):
         self.op2 = op2
 
     def has_permission(self, request, view):
-        return (
-            self.op1.has_permission(request, view) and
-            self.op2.has_permission(request, view)
-        )
+        hasperm1 = self.op1.has_permission(request, view)
+        if not hasperm1:
+            return hasperm1
+        hasperm2 = self.op2.has_permission(request, view)
+        return hasperm1 if hasperm2 is Deferred else hasperm2
 
     def has_object_permission(self, request, view, obj):
-        return (
-            self.op1.has_object_permission(request, view, obj) and
-            self.op2.has_object_permission(request, view, obj)
-        )
+        hasperm1 = self.op1.has_object_permission(request, view, obj)
+        if not hasperm1:
+            return hasperm1
+        hasperm2 = self.op2.has_object_permission(request, view, obj)
+        return hasperm1 if hasperm2 is Deferred else hasperm2
 
 
 class OR:
@@ -71,27 +76,35 @@ def __init__(self, op1, op2):
         self.op2 = op2
 
     def has_permission(self, request, view):
-        return (
-            self.op1.has_permission(request, view) or
-            self.op2.has_permission(request, view)
-        )
+        hasperm1 = self.op1.has_permission(request, view)
+        if hasperm1 and hasperm1 is not Deferred:
+            return hasperm1
+        hasperm2 = self.op2.has_permission(request, view)
+        return hasperm2 or hasperm1
 
     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)
-        )
+        hasperm1 = self.op1.has_object_permission(request, view, obj)
+        if hasperm1 is Deferred:
+            hasperm1 = self.op1.has_permission(request, view)
+        if hasperm1 and hasperm1 is not Deferred:
+            return hasperm1
+        hasperm2 = self.op2.has_object_permission(request, view, obj)
+        if hasperm2 is Deferred:
+            hasperm2 = self.op2.has_permission(request, view)
+        return hasperm2 or hasperm1
 
 
 class NOT:
     def __init__(self, op1):
         self.op1 = op1
 
     def has_permission(self, request, view):
-        return not self.op1.has_permission(request, view)
+        hasperm = self.op1.has_permission(request, view)
+        return hasperm if hasperm is Deferred else not hasperm
 
     def has_object_permission(self, request, view, obj):
-        return not self.op1.has_object_permission(request, view, obj)
+        hasperm = self.op1.has_object_permission(request, view, obj)
+        return hasperm if hasperm is Deferred else not hasperm
 
 
 class BasePermissionMetaclass(OperationHolderMixin, type):
@@ -105,15 +118,22 @@ class BasePermission(metaclass=BasePermissionMetaclass):
 
     def has_permission(self, request, view):
         """
-        Return `True` if permission is granted, `False` otherwise.
+        Return `True` if permission is granted, `False` if permission is denied,
+        and `Deferred` if object permissions must be checked.
         """
-        return True
+        return Deferred
 
     def has_object_permission(self, request, view, obj):
         """
-        Return `True` if permission is granted, `False` otherwise.
+        Return `True` if permission is granted, `False` if permission is denied,
+        and `Deferred` if object permissions aren't implemented and should be
+        granted or denied based on `has_permission` as necessary. Returning
+        `Deferred` is more efficient than simply calling `has_permission`
+        because it prevents calling `has_permission` redundantly in most cases
+        where `has_permission` was already checked before calling
+        `has_object_perimssion`.
         """
-        return True
+        return Deferred
 
 
 class AllowAny(BasePermission):
@@ -220,7 +240,7 @@ def has_permission(self, request, view):
         # Workaround to ensure DjangoModelPermissions are not applied
         # to the root view when using DefaultRouter.
         if getattr(view, '_ignore_model_permissions', False):
-            return True
+            return super().has_permission(request, view)
 
         if not request.user or (
            not request.user.is_authenticated and self.authenticated_users_only):

tests/test_permissions.py

@@ -12,6 +12,7 @@
     HTTP_HEADER_ENCODING, authentication, generics, permissions, serializers,
     status, views
 )
+from rest_framework.permissions import Deferred
 from rest_framework.routers import DefaultRouter
 from rest_framework.test import APIRequestFactory
 from tests.models import BasicModel
@@ -528,6 +529,12 @@ def setUp(self):
             self.password
         )
         self.client.login(username=self.username, password=self.password)
+        self.admin_user = User.objects.create_user(
+            'paul',
+            '[email protected]',
+            'password',
+            is_staff=True,
+        )
 
     def test_and_false(self):
         request = factory.get('/1', format='json')
@@ -677,3 +684,30 @@ def test_object_and_lazyness(self):
                 assert hasperm is False
                 assert mock_deny.call_count == 1
                 mock_allow.assert_not_called()
+
+    def test_has_permission_not_implemented(self):
+        request = factory.get('/1', format='json')
+        request.user = self.user
+        composed_perm = ~BasicObjectPerm
+        assert composed_perm().has_permission(request, None) is Deferred
+        assert composed_perm().has_object_permission(request, None, None) is True
+
+    def test_has_object_permission_not_implemented_false(self):
+        request = factory.get('/1', format='json')
+        request.user = self.user
+        composed_perm = (
+            permissions.IsAdminUser |
+            BasicObjectPerm
+        )
+        assert composed_perm().has_permission(request, None) is Deferred
+        assert composed_perm().has_object_permission(request, None, None) is False
+
+    def test_has_object_permission_not_implemented_true(self):
+        request = factory.get('/1', format='json')
+        request.user = self.admin_user
+        composed_perm = (
+            permissions.IsAdminUser |
+            BasicObjectPerm
+        )
+        assert composed_perm().has_permission(request, None) is True
+        assert composed_perm().has_object_permission(request, None, None) is True