Should "has_object_permission" call "has_permission" as default instead of returning always "True"?

[Cloves23]

Hi, there.

Short description: The method BasePermission.has_object_permission is returning a inconsistent value.

I was trying to implement a more complex permission check using custom permissions and noticed an inconsistency in the final result. In my case, I have a permission that checks for specific action names and then verifies the user type. The problem occurs when I need to verify object-level permissions. While 'has_permission' returns 'False', 'has_object_permission' returns 'True'. For consistency, wouldn't it be correct to call has_object_permission by default?

My code:

from rest_framework import permissions

@perm_with_attrs("action")
class IsAction(permissions.BasePermission):
    code = "action_not_allowed"
    message = _("Action is not allowed.")
    action: str

    def has_permission(self, request, view):
        return view.action == self.action

high_privileges = ~IsAction("cancel_account") & (
    IsAdminUser | IsTrainerUser | HasDjangoPermission(Customer, ["view"])
)

How I solved it:

from rest_framework import permissions

class BasePermission(permissions.BasePermission):
    def has_permission(self, request, view):
        return True

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

@perm_with_attrs("action")
class IsAction(BasePermission):
    ...

How to reproduce

from collections import namedtuple
from rest_framework.permissions import IsAdminUser

User = namedtuple("User", "is_staff")
Request = namedtuple("Request", "user")

req_user = Request(User(False))
assert IsAdminUser().has_permission(req_user, None) is False
assert IsAdminUser().has_object_permission(req_user, None, None) is False  # Break
assert (~IsAdminUser)().has_permission(req_user, None) is True
assert (~IsAdminUser)().has_object_permission(req_user, None, None) is True  # Break

req_staff = Request(User(True))
assert IsAdminUser().has_permission(req_staff, None) is True
assert IsAdminUser().has_object_permission(req_staff, None, None) is True
assert (~IsAdminUser)().has_permission(req_staff, None) is False
assert (~IsAdminUser)().has_object_permission(req_staff, None, None) is False

[browniebroke]

DRF took another approach as described in the docs:

Note: With the exception of DjangoObjectPermissions, the provided permission classes in rest_framework.permissions do not implement the methods necessary to check object permissions.

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 described in the Custom permissions section (below).

The base APIView checks for permissions pretty early on:

django-rest-framework/rest_framework/views.py

Line 421 in 2ede857

self.check_permissions(request)

And the generic view checks for object permissions while trying to fetch the individual instance for details endpoints:

django-rest-framework/rest_framework/generics.py

Line 103 in 15c613a

self.check_object_permissions(self.request, obj)

Taking a step back, if you get has_permission() is False, then I think DRF won't even get to the point where has_object_permission() is called, so returning True or defaulting to has_permission doesn't matter unless you bypass the framework...

[browniebroke]

In my case, the issue arises when has_permission() returns True, and has_object_permission() is subsequently called.

How is calling a method that returns True different than returning True directly?

I don't follow your example with complex composite permission, can you distill it down to a more minimal one?

[Cloves23]

Using the IsAction permission as an example: its purpose is to verify if the action name matches the expected value.

Now, imagine I have a ViewSet with multiple routes, and I want the user to have access to all routes except one (my_admin_route), which should be restricted to admins only. The permissions would look like this:

@perm_with_attrs("action")
class IsAction(BasePermission):
    code = "action_not_allowed"
    message = _("Action is not allowed.")
    action: str

    def has_permission(self, request, view):
        return view.action == self.action

class MyViewSet(..., viewsets.GenericViewSet):
    ...
    permission_classes = (IsAuthenticated, ~IsAction("my_admin_route") | IsAdminUser)

    @action(methods=["GET"], detail=True, ...)
    def my_route(...):
        obj = self.get_object()
        ...

When a user accesses the detail route my_route, even if they're not an admin, they should have access because it's not the restricted my_admin_route. However, this is where the problem arises:

• While ~IsAction("my_admin_route") correctly negates has_permission() and returns True for non-admin users accessing my_route, the same negation fails for has_object_permission(). This happens because its default implementation always returns True and doesn't take the result of has_permission() into account when no specific logic is applied.

As a result, the overall permission check gives the wrong outcome.

To fix this, I created a new class inheriting from permissions.BasePermission, ensuring that has_object_permission defaults to calling has_permission unless explicitly overridden:

from rest_framework import permissions

class BasePermission(permissions.BasePermission):
    def has_permission(self, request, view):
        return True

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

With this approach, the behavior is consistent, and has_object_permission() respects the logic in has_permission by default, preventing such issues.

[Cloves23]

You can use this to test this behavior:

from collections import namedtuple

from rest_framework.permissions import BasePermission as DRFBasePermission


class MyBasePermission(DRFBasePermission):
    def has_permission(self, request, view):
        return True

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


class IsActionMixin:
    action: str = "my_admin_route"

    def has_permission(self, request, view):
        return view.action == self.action


class DRFIsAction(IsActionMixin, DRFBasePermission):
    pass


class MyIsAction(IsActionMixin, MyBasePermission):
    pass


fake_view = namedtuple("FakeView", "action")("my_route")
print(
    "\n".join((
        "DRF",
        "---",
        f"       ~has_permission(): {(~DRFIsAction)().has_permission(None, fake_view)}",
        f"~has_object_permission(): {(~DRFIsAction)().has_object_permission(None, fake_view, None)}",
        "",
        "My",
        "--",
        f"       ~has_permission(): {(~MyIsAction)().has_permission(None, fake_view)}",
        f"~has_object_permission(): {(~MyIsAction)().has_object_permission(None, fake_view, None)}",
    ))
)

Result:

DRF
---
       ~has_permission(): True
~has_object_permission(): False

My
--
       ~has_permission(): True
~has_object_permission(): True

[browniebroke]

Ok that last example makes it clearer, I missed the fact that you negate the permissions. This was introduced in #6361 and the problem was reported in #6598

While the fix you're suggesting sounds simple, I'm 99% sure doing it is going to break someone's workflow who relies on the current behaviour.

[Cloves23]

I agree with you that the impact would be very significant if it were immediate. If there is an intention for a future update, a smooth migration could be done. For example, you could use two versions of the same class for compatibility. Over time, people would migrate from the old version to the new one. Then, in another version, you could remove it.

Something like:

# rest_framework/permissions.py
from rest_framework.settings import api_settings

class BasePermission(metaclass=BasePermissionMetaclass):  # Fixed
    """
    A base class from which all permission classes should inherit.
    """

    def has_permission(self, request, view):
        """
        Return `True` if permission is granted, `False` otherwise.
        """
        return True

    def has_object_permission(self, request, view, obj):
        """
        Return `True` if permission is granted, `False` otherwise.
        """
        return self.has_permission(request, view)


class ClassicBasePermission(BasePermission):  # Original
    def has_object_permission(self, request, view, obj):
        return True

if api_settings.USE_CLASSIC_BASE_PERMISSION:
    BasePermission = ClassicBasePermission

I'm not sure if this could be a solution. This is just an idea, but could be a start to solve the problem.

[Cloves23]

Considering that there have been no interactions for a few weeks, I'm closing the question. I thank @browniebroke for his willingness and attention.

The answer that I've interpreted is: Create a custom base permission.