Fix BasePermission.has_object_permission() from short circuiting in permission composition.

[Dog]

Description

Attempts to fix #7117

If an object permission evaluates to False while being composed with an object only implementing .has_permission(), then it is short circuited due to the default of BasePermission.has_object_permission(). An example where this can be problematic is if you are calculating an object permission and an admin permission. This is replicated in the failing test.

I resolved the failing test by changing BasePermission.has_object_permission() to return the value of BasePermission.has_permission()

[xordoquy]

I believe this would be solved by #6605

[Dog]

One difference between my pull request and #6605 is that a call to permissions.IsAdminUser().has_object_permission() would still return True for #6605 . With this fix permissions.IsAdminUser().has_object_permission() would return False 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

[tomchristie]

Hrm. Okay, but this is problematic in that it'll potentially run multiple queries for simpler cases that don't require this.

Agreed with @xordoquy - #6605 looks right to me.

is that a call to permissions.IsAdminUser().has_object_permission() would still return True for #6605

That's okay, if has_permission() returns False.

[Dog]

@tomchristie Regarding has_permission please reconsider the test case I pushed where there is an object permission but no explicit overwrite of has_permission. In that case has_permission is returning True when it should be returning False. It is being short circuited. As an aside, having permissions default to True seems potentially dangerous.

I can update #6605 but have the same hesitation @tomchristie has.

[tolomea]

As an aside, having permissions default to True seems potentially dangerous.

Given the docs say "If you wish to use the provided permission classes in order to check object permissions, you must subclass them and implement the has_object_permission() method" you would expect that by default that function either returns False or raises NotImplemented.

[DrJfrost]

I also noticed this might affect the custom message behaviour, is there a way you make that pull request compatible with this one #6502 as well?

[Dog]

@DrJfrost I'll look at supporting that PR once I wrap up merging with #6605

[Dog]

Do not merge, still WIP. I've been having some tox errors locally I was trying to avoid fixing, but since I'll look at #6502 afterwards I'm going to stop now and fix it so I can stop polluting the PR.

[Dog]

I believe this is now ready for review. I've already started looking at #6502 if I need to add compatibility with it.

[Dog]

@tomchristie Awaiting review, resolves a milestone that was moved from 3.12 to 3.13 yesterday.

[tomchristie]

Indeed. We had so much stuff already pending that I figured we were better off pushing on with a 3.12 release. We'll aim to have a 3.13 release without such a big delay next time.

[ysliaw01]

A better and simpler solution is to change the BasePermission.has_object_permission() to return self.has_permission() instead of True i.e.

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

I have implemented a custom IsAdminUser as such to fix this bug:

class IsAdminUser(permissions.IsAdminUser):
    """ validate auth user is admin"""
    # Overwrite the default to fix a bug in Django framework
    def has_object_permission(self, request, view, obj):
        return self.has_permission(request, view)

[tolomea]

@ysliaw01 we did the same and have been running it in production for over a year, we do it in a base class that all our perm classes inherit off, of course we have to be careful to never use the built in perm classes

[sparkyb]

@ysliaw01 It is certainly a simpler solution, and would solve the problem for many people in many cases, but I disagree that it is a better solution. If your base check is expensive it will unnecessarily be called twice. And there are also some other cases I was able to come up with in testing that this solution still doesn't get right. I still believe that a 3-value solution (allow, deny, don't-care) is more technically correct and expressive. That's what my patch does, as well as moving all the complexity into the composites (and/or/not) to keep the Permission class simple and only call has_permission as a fallback to has_object_permission in the very specific OR case where it is necessary.

[ysliaw01]

@sparkyb Can you kindly elaborate the other cases where my mentioned above solution would fail? Thank.

[DrJfrost]

It's been almost 2 years now and still no solution for this :s

[sparkyb]

@ysliaw01 The biggest case is with NOT. Let's say I have an IsOwner permission that checks if the authenticated user is the owner of the object in question. This has to be a has_object_permission test. If you wanted to have an endpoint that only allows users who aren't the owner (I'm not sure why you'd want to do this, but I want correctness in all cases regardless), then you could use ~IsOwner. However this would always fail, even for users that are not the owner, because since only has_object_permission is implemented, has_permission returns the default True, causing the inversion of it to be false and the object permission will never even be checked. With my proposed 3-state system where either method could return a 3rd value to indicate they don't implement it, has_permission could return that special value which will always be considered true, even when inverted, so that has_object_permission will get checked.

Beyond just that case, my other goal is to not redundantly call has_permission when not necessary.

If just calling has_permission from has_object_permission works for you, that's great, then I recommend using that solution. It is pretty easy to monkey patch that into the Permission base class in your project. However, if we're going to add a permanent solution to the framework for everyone, I think something that is more correct is a better solution for everyone, even if it is a bit more complex.

[ysliaw01]

@sparkyb First thank for the explanation. But for your given example of ~IsOwner, my mentioned solution will work because BasePermission.has_object_permission() return self.has_permission() instead of True. This is the only change that is required in my solution.

I like your proposed solution, which avoids the calling has_permission() twice as you said. This bug is serious and has been for quite a while, and I would like to see this being fixed asap.

[tolomea]

@ysliaw01 I don't think it will cover it. In a two value system with object specific things like "IsOwner"(of the object) the has_permission check fundamentally has to be broader than the actual truth, including users who are not the owner so they can be filtered by has_object_permission, in this case probably including everyone. But then to @sparkyb's point, when you invert something that is overboard it becomes over narrow, in this case excluding people who might not be the owner before we can call has_object_permission, in this case probably excluding everyone.

[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.