caxap / rest_condition Goto Github PK

DRF has a built-in way of providing custom error messages.
However, these error messages are currently ignored by 'rest_condition' classes. Instead, the default error message is always used.

In DRF, you can specify a class level attribute called message, that gets used as the error message if the permission check fails.

http://www.django-rest-framework.org/api-guide/permissions/#custom-permissions

e.g.

from rest_framework import permissions

class CustomerAccessPermission(permissions.BasePermission):
   message = 'Adding customers not allowed.'

def has_permission(self, request, view):
     ...

To fix this, It seems evaluate_permissions(....) in permissions.py could be enhanced to read the message attribute from the condition instance and set its own message attribute to that.

e.g. permissions.py

def evaluate_permissions(....)

    if reduced_result is not _NONE:

        # read the error message from the custom permissions class and set it here so DRF
       # can pick it up.
       if hasattr(condition.__class__, 'message'):
           Condition.message = condition.__class__.message

         return not reduced_result if self.negated else reduced_result

In my viewset I have the following:
permission_classes = [ Or(And(IsReadOnlyRequest, IsAuthenticated), And(IsPutRequest, IsObjectUser)) ]

Where IsObjectUser has:
def has_object_permission(self, request, view, obj): return obj.user == request.user

The has_object_permission method is never invoked when wrapped inside the conditional statement. However, If I use the IsObjectUser as a plain permission class in permission_classes list, it works fine. This isn't suitable for me because I need them for each method, not for the whole viewset, always.

If this is bug, I would love to see it fixed. Any suggestions or workarounds are appreciated.

Permission tests like rest_framework.permissions.IsAdminUser may return None. This isn't covered well in rest_condition.permissions.Condition and thus the permission check leaves early with an unexpected exception.

  File "…/lib/python3.5/site-packages/rest_condition/permissions.py", line 113, in has_permission
    return self.evaluate_permissions('has_permission', request, view)
  File "…/lib/python3.5/site-packages/rest_condition/permissions.py", line 98, in evaluate_permissions
    reduced_result = self.reduce_op(reduced_result, result)
TypeError: unsupported operand type(s) for |: 'NoneType' and 'bool'

I've defined a fairly standard set of permissions, and am using rest_condition to generate a set of conditional permissions.

I'm also trying to use these w/ the IsAuthenticated permission class, but have omitted it here for brevity.

In a simple ViewSet that uses the rest_condition permission class defined below, it appears as though the only permission that is being run in many circumstances is the IsSuperuser class.

I've found that by reordering things, I can get the others to run, but I feel as though this is unintended behaviour. Is it because one of the permission classes is an object-specific permission, while the others aren't?

class IsListView(permissions.BasePermission):
    def has_permission(self, request, view):
        return bool(view.action == 'list')


class IsSuperuser(permissions.BasePermission):
    def has_permission(self, request, view):
        return request.user.is_superuser


class IsFilteringOwnResources(permissions.BasePermission):
    def has_permission(self, request, view):
        return bool(request.QUERY_PARAMS.get('user') == str(request.user.id))


class IsResourceOwner(permissions.BasePermission):
    def has_object_permission(self, request, view, obj):
        return bool(obj.user == request.user)


IsSuperuserOrResourceOwner = Or(Or(IsSuperuser, IsResourceOwner), And(IsListView, IsFilteringOwnResources))

In my tests, I have added debugging statements in each permission class.

I have an Or containing two permissions. When the first permission fails, the second permissions has_object_permission method does not get called.

From what i can see, it appears to only occur when the first permission does not explicitly define a 'has_object_permission' method. If the method is explicitly defined on the first permission class, then the second permissions has_object_permission method gets called.

Plz mention in docs that in order to achieve correct combining has_permission with has_object_permission
We need to both override both has_object_permission and has_permission

For example

class OperatorPermission(permissions.BasePermission):
    """
        Allowing access to Operator only
    """

    def has_permission(self, request, view):
        return request.user.is_operator

    def has_object_permission(self, request, view, obj):
        return request.user.is_operator

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.

If you use rest_framework.permissions.IsAuthenticated the reduce operator will crash here because Django now returns a CallableBool.

This could be fixed by adding a callable test just after getting the result.

        if hasattr(result, '__call__'):
            result = result()