Django-rest-framework: bitwise permissions not working when combine has_object_permission and has_permission

[Arti3DPlayer]

Checklist

• [x] I have verified that that issue exists against the master branch of Django REST framework.
• [x] I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
• [x] This is not a usage question. (Those should be directed to the discussion group instead.)
• [x] This cannot be dealt with as a third party library. (We prefer new functionality to be in the form of third party libraries where possible.)
• [x] I have reduced the issue to the simplest possible case.
• [ ] I have included a failing test as a pull request. (If you are unable to do so we can still accept the issue.)

Steps to reproduce

Set

from rest_framework import viewsets
from rest_framework.permissions import IsAdminUser

class IsCompanyMemberPermission(IsAuthenticated):
    """
        Allows access only to company owner members.

    """

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

class MyViewSet(viewsets.ModelViewSet):
            def get_permissions(self):
                    if self.action in ['update', 'partial_update', 'destroy']:
                          self.permission_classes = (IsAdminUser | IsCompanyMemberPermission, )
                return super(BuilderOrganizationViewSet, self).get_permissions()

Do put request

I also found similar issue on https://stackoverflow.com/a/55773420/1786016

Expected behavior

has_object_permission must be called and return False in my case

Actual behavior

has_object_permission not called

[Dog]

I have been able to work around this issue. I found that the order of classes in permission_classes matters. To help debug this issue I rewrote IsAdminUser in my code.

class IsOwnOrder(permissions.BasePermission):
    def has_object_permission(self, request, view, obj):
        import pdb; pdb.set_trace()
        return obj.placed_by == request.user

class IsAdminUser(permissions.BasePermission):
    def has_permission(self, request, view):
        import pdb; pdb.set_trace()
        return bool(request.user and request.user.is_staff)

I first tried my view written like this:

class OrderDetail(BaseViewMixin, generics.RetrieveUpdateDestroyAPIView):
    serializer_class = OrderSerializer
    permission_classes = [IsOwnOrder | IsAdminUser]

In this case only the breakpoint in IsOwnOrder.has_object_permission is hit.

If I instead wrote the class like this:

class OrderDetail(BaseViewMixin, generics.RetrieveUpdateDestroyAPIView):
    serializer_class = OrderSerializer
    permission_classes = [IsAdminUser | IsOwnOrder]

Now only the breakpoint in IsAdminUser.has_permission is hit.

I worked around it by writing IsAdminUser like this and ensuring it was last in the operation.

class IsAdminUser(permissions.BasePermission):
    def has_object_permission(self, request, view, obj):
        return self.has_permission(request, view)

    def has_permission(self, request, view):
        return bool(request.user and request.user.is_staff)

The issue is BasePermission.has_object_permission defaults to True:
https://github.com/encode/django-rest-framework/blob/95d4843abeecea96754a147f4f2cca33e620ad09/rest_framework/permissions.py#L116

I'm opening up a pull request that attempts to fix this issue

[xordoquy]

I think at some point bitwise permissions should enforce to have both has_object_permission and has_permission explicitly defined.
~I can't see how this would work without it.~
I can't see how it can be clear without making the default explicit.

[marcosox]

The issue is BasePermission.has_object_permission defaults to True

I think that a better default would be to make has_object_permission() return the same as has_permission().

[Dog]

@marcosox That is what I have done in #7155

[Arti3DPlayer]

@Dog this will not work in case of complex permission logic like:

(IsAdminUserOrSuperUserPermission |
(IsAuthenticated & ~permissions.BuilderCompanyMemberPermission),)

We need to check both methods to make it work as expected

[DrJfrost]

why has this not been merged?, the only quick fix I could do for this type of use case where it would check is admin (has_permission) or is owner (has_object_permission) [IsStaff | IsOwner] was to declare the permission has_object_permission on the is_admin permission, basically declaring twice the same thing just to make sure drf does not cut the permission check for has_object_permission

class IsStaff(permissions.BasePermission):

    def has_permission(self, request, view):
        return request.user.groups.filter(name="staff").exists()

    def has_object_permission(self, request, view, obj):
        return request.user.groups.filter(name="staff").exists()

class IsOwner(permissions.BasePermission):

    """
    Object-level permission to only allow owners of an object to edit it.
    """

    message = "user making the request is not the owner of this object"

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

[Arti3DPlayer]

@DrJfrost Unfortunately, this solution still not working for complex bitwise operations. We need to do not stop checking permissions on first True and make sure that all methods has_object_permission and has_permission being called.

[tomchristie]

Happy to accept #6605, or anyone issuing a fresh PR based on that and updating the required test case.

[sri-vathsa]

I have been able to work around this issue. I found that the order of classes in permission_classes matters. To help debug this issue I rewrote IsAdminUser in my code.

class IsOwnOrder(permissions.BasePermission):
    def has_object_permission(self, request, view, obj):
        import pdb; pdb.set_trace()
        return obj.placed_by == request.user

class IsAdminUser(permissions.BasePermission):
    def has_permission(self, request, view):
        import pdb; pdb.set_trace()
        return bool(request.user and request.user.is_staff)

I first tried my view written like this:

class OrderDetail(BaseViewMixin, generics.RetrieveUpdateDestroyAPIView):
    serializer_class = OrderSerializer
    permission_classes = [IsOwnOrder | IsAdminUser]

In this case only the breakpoint in IsOwnOrder.has_object_permission is hit.

If I instead wrote the class like this:

class OrderDetail(BaseViewMixin, generics.RetrieveUpdateDestroyAPIView):
    serializer_class = OrderSerializer
    permission_classes = [IsAdminUser | IsOwnOrder]

Now only the breakpoint in IsAdminUser.has_permission is hit.

I worked around it by writing IsAdminUser like this and ensuring it was last in the operation.

class IsAdminUser(permissions.BasePermission):
    def has_object_permission(self, request, view, obj):
        return self.has_permission(request, view)

    def has_permission(self, request, view):
        return bool(request.user and request.user.is_staff)

The issue is BasePermission.has_object_permission defaults to True:

https://github.com/encode/django-rest-framework/blob/95d4843abeecea96754a147f4f2cca33e620ad09/rest_framework/permissions.py#L116

I'm opening up a pull request that attempts to fix this issue

This workaround is not working for me. I have a similar issue. I am using 2 permission classes using "|" operator with has_object_permission implementation. The second has_object_permission was not getting called(which would return False) so I changed the order in which case False is being returned but the other permission class returns True so essentially it's not working as intended

[Dog]

@tomchristie https://github.com/encode/django-rest-framework/pull/7155 is still waiting for review.

[Ishma59]

https://stackoverflow.com/a/35361292/10034447

[DrJfrost]

@Ishma59 is not really related with the problem

[Hamza5]

How is this bug still not fixed?! It is so obvious and everyone doing a serious project will fall into it!

[DrJfrost]

there has been several PR but none seems to review them, I think the resolution of this problem is crucial for a 3.12 version to be released

[sparkyb]

I appreciate #7155 and that is how I was intending to fix this issue when I came here to submit an issue/PR. However, when I searched to find this issue, I also found #6598 which points out that the behavior of NOT is also affected by the fact than a permission that doesn't implement has_object_permission just returns True right now. You could make a similar fix to NOT where its has_object_permission returns not (self.op1.has_permission(request, view) and self.op1.has_object_permission(request, view, obj). But now has_permission may be called redundantly many times if you use complex compositions. Instead I came up with an alternate solution that is more robust and avoids calling has_permission redundantly. The idea is for has_object_permission to be able to raise NotImplementedError when a particular permission doesn't care about it, and then compositions can treat it as a no-op, the effect of which can depend on the type of composition (AND vs OR vs NOT). See PR #7601 . I'd love to get feedback what people think of this approach. Could potentially do this for has_permission as well so that permissions that only handle object permission don't fail early when inverted with NOT, but that would break a lot of tests that I wasn't looking forward to rewriting.

[iamahuman]

How about we let the methods return NotImplemented and let operators skip over it?

In case every permission classes return NotImplemented, we can keep the current policy of allowing everything by default _or_ let it be specified by the user.

As per backwards compatibility, NotImplemented is a truth value and I suppose few users will count on the predefined permission classes returning exactly True or False. At minimum, this appears less disruptive than the alternative that changes how the authorization results are carried around.

Side note: this appears to be a duplicate of #6402.

[Thor--]

I ran into this same issue today. Ended up following the above advice, and implemented my own IsAdminUser, since the built-in one doesn't implement has_object_permissions()

My new IsAdminUser simply has has_object_permissions() return has_permission()

[titovanton]

```

permissions.py

class IsFlag(BasePermission):
def has_permission(self, request, view):
# NOTE: here is the problem without a chance to be solved within
# the permissions execution order
return True

def has_object_permission(self, request, view, obj):
    return bool(obj.flag)

views.py

class MyViewSet(ModelViewSet):
def get_permission_classes(self):
# ...
elif self.action in ['update', 'partial_update']:
return self.permission_classes + [~IsFlag & IsAdmin]
# ...
`` As you can see, I require an object must not have the flag to be True. But without access to an object withinhas_permission` context, I have no other options either then return True, False - does not solve it, but shift the issue to the opposite case.

A possible solution would be to split permissions by types: object level, view level, mixed. So the object level permission will only be called in get_object method, not ever earlier, view level permission - never to be called within get_object and mixed - simply the same structure and behavior as we have now implemented.

Unfortunately, permissions in DRF have not required flexibility, another example: will not be superfluous if view level permission gets filtered queryset as a parameter. As it's lazy, that change would not impact on performance, but gives more freedom to an end developer. Another solution for that example, would be if we split view layer has_permission on to pre_fetch and post_fetch hooks, so the second one gets queryset as a parameter, but first one behaves as it does now.

Within the current architecture of the DRF permissions, I have to write a custom permission for every complicated case, instead of building a logical formula with already written permissions.