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.

[k-thornton]

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 within has_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.

[sparkyb]

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

I like this suggestion. You are right that NotImplemented is truthy so this is more backwards compatible than my previous solution of raising NotImplementedError. The conditionals in AND/OR/NOT get a teeny bit trickier, but the other advantage of this change is that no changes to APIView.check_permissions or APIView.check_object_permissions are necessary anymore. I updated my PR #7601 to change to returning NotImplemented by default and I also now do this for has_permission as well as has_object_permission (it turns out this change didn't break any existing tests so all I had to do was add an additional test to confirm the new behavior).

My PR was originally submitted back in October and still hasn't gotten a reviewer or any comments. There are several other outstanding PRs attempting to fix this issue as well as a handful of issues related to composing permissions in addition to this one, and it doesn't look like any of them have have much activity lately. How can I get my PR looked at/reviewed so we can move this forward? I've found a way to monkey-patch my PR into my current project for now (can share if anyone needs), but I'd like to see this resolved for real.

[iamahuman]

@sparkyb I just realized that NotImplemented shall not be evaluated in boolean context, and future versions of Python will throw TypeError on such attempts. My apologies if this would mean that your effort was (partly) wasted. Perhaps it's wise to revisit this apporach.

[iamahuman]

Some alternative approaches:

• Use something else that is truthy.
• Keep NotImplemented and simply don't evaluate it in boolean context.

[sparkyb]

Good catch. I had actually originally implemented as raising NotImplementedError and catching it, but changed to returning NotImplemented when someone suggested it. I think at this point I will probably keep it and just check for it explicitly. It will make the code slightly more verbose, but maybe more clear what it is doing. I'll update the PR with that fix and updating the docstrings when I get a chance, hopefully in a few days, although things are quite busy right now.

[brendenwcase]

This is an absolutely glaring security concern for otherwise great software. I would like to urge the developers to address this issue as soon as possible. Until then, anyone who uses this framework with a permissions class of the form (IsAdminUser | MyCustomPermission) may be unknowingly granting admin-level access to all users. Most concerning of all is the failure to address this issue in a timely manner: it has been open for over 1.5 years. Moreover, several other issues have called out the security concerns of this problem during that time. That is over 18 months' awareness of significant vulnerabilities (undoubtedly littering many DRF-fueled sites) without meaningful action.

The only legitimate reason for this issue not being prioritized as most important is if there are other issues pertaining to greater security concerns.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[CameronSima]

Is this still not fixed? In my project the simple permission composition of (ReadOnly | IsAdmin) fails open to allow any unauthenticated user have full access to the system. This basic functionality is what I would consider a prerequisite for any framework and I am shocked discussion has been ongoing for years without resolution. This is literally a day-one feature in any web framework.

[hamishfagg]

Spent hours bashing my head against this today, wondering why I could edit objects when I wasn't even logged in.

This seems to me like a pretty serious bug waiting to bite someone...

[tomchristie]

Agreed and agreed. This has gotten lost for an absurdly long time and needs to be dealt with. Not really acceptable. REST framework's gotten a bit lost with too much focus on other projects - needs dealing with.

Anyways.

PR #7522 (Based on #6605) looks like the right approach to me. Is anyone available to review that?

(A better approach might be for the base permission implementations to raise NotImplemented instead of defaulting to True, but I don't think that's a feasible change for this project.)

[Ou7law007]

It took me a while to know what was going on why my class inheriting from permissions.IsAdminUser keeps returning True when has_object_permission is called!

I do not understand why some permission classes like IsAdminUser and IsAuthenticated don't implement has_object_permission

[rau]

Because @Dog's workaround is quite buried above, I'll rephrase it here.

The easiest temporary workaround is to make a new, custom IsAdminUser permission class like such:

class IsAdmin(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)

This works because it includes both the required has_object_permission and has_permission classes.

[Ou7law007]

Yeah, I'm aware of how to check if a user is_staff, is_admin or is_superuser but that's not clean when a whole class called IsAdminUser exists and is supposed to do what its name suggests...

Another "workaround" is to call super().has_permission(request, view) because has_permission is implemented unlike has_object_permission

Again, I don't understand why we would need a workaround for something that's not defective but rather just implemented to not do what it's supposed to i.e. check if IsAdminUser