Handle composition of unimplemented permission methods correctly

Previously has_object_permission defaulted to returning True when
unimplemented because it was assumed that has_permission had already returned
true, but when composed with OR, this might not be the case. Take for example
the case where you want an object to be accessible by the user that created it
or any admin. If you have a permission IsOwner that implements
has_object_permission and IsAdminUser which doesn't, then if you OR them
together an have a user that is neither the owner nor the admin they should
get denied but instead IsOwner will pass has_permission and IsAdminUser will
pass has_object_permission even though it didn't pass has_permission.

One solution would be to default has_object_permission to calling
has_permission but that would potentially cause redundant database lookups.
Alternatively this could be done only in the OR class, but the redundant
calls will still happen.

Also, incorrect behavior can still occur using more complicated compositions
including NOT. For example, the IsOwner permission above only implemented
has_object_permission and not has_permission, but ~IsOwner will always fail,
even on objects that the authenticated user doesn't own, because the default
True from BasePermission.has_permission() will also be inverted.

My solution is to be more explicit about when a permission subclass doesn't
implement has_permission or has_object_permission by returning NotImplemented.
NotImplemented is truthy in a boolean context, so by default everything will
continue to work the same as long as code is not expecting the result to
literally be True or False. I modified AND and OR so that if one side returns
NotImplemented, it defers to the other. If both sides return NotImplemented,
NotImplmented will be returned to propogate upwards. NOT will also propagate
NotImplmented instead of inverting it. If NotImplemented propagates
all the way up to APIView.check_permissions or
APIView.check_object_permissions, it is considered a pass (truthy).

Author: sparkyb

rest_framework/permissions.py

@@ -53,16 +53,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 NotImplemented 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 NotImplemented else hasperm2
 
 
 class OR:
@@ -71,27 +73,31 @@ 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 NotImplemented:
+            return hasperm1
+        hasperm2 = self.op2.has_permission(request, view)
+        return hasperm1 if hasperm2 is NotImplemented else hasperm2
 
     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 and hasperm1 is not NotImplemented:
+            return hasperm1
+        hasperm2 = self.op2.has_object_permission(request, view, obj)
+        return hasperm1 if hasperm2 is NotImplemented else hasperm2
 
 
 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 NotImplemented 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 NotImplemented else not hasperm
 
 
 class BasePermissionMetaclass(OperationHolderMixin, type):
@@ -107,13 +113,13 @@ def has_permission(self, request, view):
         """
         Return `True` if permission is granted, `False` otherwise.
         """
-        return True
+        return NotImplemented
 
     def has_object_permission(self, request, view, obj):
         """
         Return `True` if permission is granted, `False` otherwise.
         """
-        return True
+        return NotImplemented
 
 
 class AllowAny(BasePermission):
@@ -220,7 +226,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):