Or conditionals shortcutting prematurely

[ryanisnan]

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.

[ryanisnan]

After a bit more research, I discovered that when using this conditional, things break even more significantly:

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

In that scenario, the permission w/ object specific checks (and no general has_permission definition) is first, and in this scenario, NONE of the permissions classes are checked and the overall result is returned as False.

[DavidJFelix]

So the issue here seems to more succinctly be that rest_condition doesn't support blended conditions. You can't mix a has_permission with a has_object_permission. Does this issue continue if you create a has_object_permission function that just does the same thing as has_permission?

[glynjackson]

@ryanisnan @DavidJFelix I got this to work with both has_object_permission and has_permission unless I'm missing something here...

i.e. Don't do this...

    permission_classes = [And(Or(TokenHasReadWriteScope, SecretKeyToken, permissions.IsAdminUser), IsOwnerOrReadOnly)]

Where IsOwnerOrReadOnly uses has_object_permission

Do...

    permission_classes = [ConditionalPermission, IsOwnerOrReadOnly]
    permission_condition = (C(SecretKeyToken) | C(TokenHasReadWriteScope) | C(permissions.IsAdminUser))

[lucasdavid]

I'm having a similar problem: non-authorized users are being able to access protected resources, even when I defined the permissions as suggested by @glynjackson:

permission_classes = [ConditionalPermission, TokenHasReadWriteScope]
permission_condition = (C(UserRetrievingTheirOwnClient) | C(IsAdminUser))

I believe what is happening is that IsAdminUser does not implement has_object_permission, because it assumes that has_permission would already eliminate non-admin users, but as we're using an Or here with UserRetrievingTheirOwnClient, this step succeeds. Then, when the view actually calls has_object_permission, IsAdminUser always returns True (the return defined in BasePermission), even when the user is not an admin or if they are not authenticated.

Is the development of this repository stalled? I'd be willing to help to solve this problem, these conditions are awesome.

[divick]

@lucasdavid I too see that the issue is manifested when conditions are combined such that when doing Or on a class which returns True for has_object_permission by default like in IsAdminUser. I overrode the has_object_permission in a derived class from IsAdminUser as a workaround, given that this bug has been there since a long time and probably it is not going to get fixed anytime soon.

[lucasdavid]

Yep, overriding IsAdminUser was the only way that I could think of, too. Thank you, @divkis01.