Django-rest-framework: Permission OR and AND should check `has_permission` before checking `has_object_permission`

[Mateusz-Slisz]

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

Let's use the built-in permission IsAdmin and a custom permission IsAuthenticatedUserOwner with OR operator provided in the latest 3.9 version. IsAdminUser does not implement the has_object_permission method, while IsAuthenticatedUserOwner only defines the has_object_permission method, to check whether user has permission to edit their own User instance. Since both of these classes inherit from BasePermission, by default they return True. In this case, when object permissions are checked, the IsAdminUser class will return True even for unauthenticated user (!), and effectively anyone can edit/delete the User instance.

class IsAdminUser(BasePermission):
    """
    Allows access only to admin users.
    """

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

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

permission_classes = (IsAuthenticatedUserOwner | IsAdmin,)



md5-953fb0b0637cc7f6b386a82682b5f3b3



class OR:
    def __init__(self, op1, op2):
        self.op1 = op1
        self.op2 = op2

    def has_permission(self, request, view):
        return (
            self.op1.has_permission(request, view) |
            self.op2.has_permission(request, view)
        )

    def has_object_permission(self, request, view, obj):
        return (
            self.op1.has_object_permission(request, view, obj) if self.op1.has_permission(request, view) else False |
            self.op2.has_object_permission(request, view, obj) if self.op2.has_permission(request, view) else False
        )

With this scenario we would only access to our user object.

Actual behavior

We would have access to any users without being an admin.
I didn't mention it but we means logged user, and access means that we could update, delete etc. any users.

[xordoquy]

Hi. thanks for the report. Didn't see that coming at all.
I'd like to consider alternatives where the has_permission won't be called twice though.

[belkka]

b if a else False is equivalent to a and b

[FMCorz]

Somewhat related, I just realised that permissions are not lazily checked.

For instance IsReadonly | IsSomethingElse will check both permissions when in fact it does not need to when the first resolves to True. I suggest changing the class OR to use or instead of |.

The same can be said about class AND when the first expression is false the other permissions should not be executed.

This is particularly important when the order of permissions was set intentionally, for instance IsReadonly | IsAssumingNotReadonly where the second one always assumes that the request is not a GET, which would lead to errors if it's evaluated.

[carltongibson]

@FMCorz Could you open a separate ticket for lazy evaluation here? That seems like a very do-able addition. (A PR would be welcome! — Would need to check a multiple-composition example, something like, A&B|C&D.)

The base problem here is using the provided IsAdminUser for object level checks, which it's clearly not capable of doing.

BasePermission returns True for both checks so that subclasses automatically signal _"I'm not capable of making this check, so I do not fail it"_. It's the responsibility of subclasses to implement the correct behaviour if they want to be in play. (The permissions behaviour is as old as DRF itself.)

Thus the correct approach here is to subclass IsAdminUser to implement the missing method, if you want to use the class in the has_object_permission() role. This sort of thing would do:

from rest_framework import permissions


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

Then (IsAuthenticatedUserOwner | IsAdmin,) would work as expected.

Of the example permission classes, only DjangoObjectPermissions implements has_object_permission(). Making the examples more complex isn't something we want to do: better users implement permissions in their own projects. As such I think all we can realistically do here is remind people in the docs that, bar DjangoObjectPermissions obviously, the examples permissions are not suitable for checking object permissions.

[belkka]

I agree with @Mateusz-Slisz about what is expected behavior. The base problem here is that we expect has_permission() being already checked when running has_object_permission(). Consider following example

class WarriorPermission:
    def has_permission(self, request, view):
        return request.user.is_warrior

    def has_object_permission(self, request, view, obj):
        return (is_sword(obj) or is_armor(obj)) and (obj.weight <= request.user.strength) 


class WizardPermission:
    def has_permission(self, request, view):
        return request.user.is_wizard

    def has_object_permission(self, request, view, obj):
        return is_wand(obj) or is_scroll(obj) or (obj.is_magic and obj.level <= request.user.level)


class MightAndMagicShopViewSet(ListRetrieveViewSet):
    queryset = Item.objects.exclude(is_legendary=True)
    permission_classes = [IsReadOnlyPermission or WarriorPermission or WizardPermission, IsNotRoguePermission]

    @action(detail=True, methods=['post'])
    def buy(self, request, pk):
        item = self.get_object()
        request.user.pay(item.cost)
        request.user.take(item)
        return Response('Have a nice day!')

Expected behavior: warriors can buy weapons, but cannot buy scrolls and other magic items (until they are wizards at the same time); wizards can buy wands and scrolls but cannot buy what warriors can. Actual behavior: warriors can buy scrolls, wands and magic items if they have enough level; wizards can buy weapon if they have enough strength. With @Mateusz-Slisz example implementation everything would work as expected. As pointed out by @xordoquy, current implementation of views.APIView badly combines with it though. :(

I do not see easy way to fix it withough making significant changes to permissions mechanism. Possible solutions:

1. Save result of get_permissions() on view instance and save result of has_permission() on permission instance, so it can be retrieved later before calling has_object_permission(). This is incompatible with current approach because now single permission instance can be safely used for different requests.
2. Store result of get_permissions() on view instance and remove permissions that are not granted in check_permissions(). E. g. get_permissions() returns [PermA or PermB, PermC], only permissions A and C are granted in check_permissions() so we replace PermA or PermB with PermA and save [PermA, PermC] on view instance. check_object_permissions() uses this saved value, so PermB.has_object_permission() is not called at all.

What do you think?