From 95f972793b4a54885dfa467b4c36cc97864b0476 Mon Sep 17 00:00:00 2001 From: Ben Buchwald Date: Sat, 17 Oct 2020 17:17:45 -0400 Subject: [PATCH] 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). --- rest_framework/permissions.py | 48 ++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/rest_framework/permissions.py b/rest_framework/permissions.py index 3a8c58064..91d8957d2 100644 --- a/rest_framework/permissions.py +++ b/rest_framework/permissions.py @@ -53,16 +53,18 @@ class AND: 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,16 +73,18 @@ class OR: 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: @@ -88,10 +92,12 @@ class NOT: 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 @@ class BasePermission(metaclass=BasePermissionMetaclass): """ 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 @@ class DjangoModelPermissions(BasePermission): # 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):