rest_condition
rest_condition copied to clipboard
Permissions not treated as units
Because C.has_permission and C.has_object_permission both run self.evaluate_permissions()
in a disconnected manner, this leads to the following logical error when OR-ing permissions:
Given
A = hpA & hopA, and
B = hpB & hopB,
A || B should really get evaluated unitarily, as
(hpA & hopA) || (hpB & hopB),
but instead gets evaluated as
(hpA || hpB) & (hopA || hopB)
which is incorrect, and leads to the following problem:
Consider
class A(BasePermission):
def has_permission(self, request, view):
return True
def has_object_permission(self, request, view, obj):
return False
class B(BasePermission):
def has_permission(self, request, view):
return False
def has_object_permission(self, request, view, obj):
return True
C(A) | C(B)
should always return false for object-level access, but it returns true.
I saw that in #1 you dismissed maintaining state during the initial has_permission
run as something downstream developers should implement, but it really needs to be done in rest_condition for logical correctness. If maintaining state, in the case above "the whole of B" would get marked as False during the initial phase, and B.has_object_permission would never get to run.
On an unrelated note, this would also be the stepping stone for dealing sensibly with mixing permissions with different combinations of has_permissions / has_object_permissions (both #1 and #2). When the method is missing (presuming one didn't inherit from BasePermission), it should be a no-op -- basically behaving like it returned True when AND-ed, and False when OR-ed.
I worked around the issue mentioned here as well as in #2 by separating general and object-level permissions completely. By using a class something like this:
class CombinedCondition(BasePermission):
"""
Extension for rest_condition to be able to separate the general and object-level paths of permission checks.
"""
def __init__(self, gen_perm_class, obj_perm_class):
self.gen_perm = gen_perm_class()
self.obj_perm = obj_perm_class()
super().__init__()
def __call__(self):
return self
def has_permission(self, request, view):
return self.gen_perm.has_permission(request, view)
def has_object_permission(self, request, view, obj):
return self.obj_perm.has_object_permission(request, view, obj)
CC = CombinedCondition
you can do:
permission_classes=[CC(
IsAuthenticated, # General-permissions
Or(IsAdminUserObjectLevel, IsObjectCreator, IsObjectEditor) # Object-level permissions
)]
The first set of permissions only check has_permission(), while the second only check has_object_permission(). At least for me, this makes it perfectly clear what is happening.
Hope it helps someone.