Make BasePermission.has_object_permission return False

[Anton-Shutik]

Hi

I'm using rest_condition app and have permissions like below:

permission_classes = (Or(IsCreationOrIsAuthenticated, IsAdminUser),)

And it always allows access at the object level because:

class BasePermission(object):

    def has_object_permission(self, request, view, obj):
        return True

class IsAdminUser(BasePermission):

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

has_object_permission is not overridden in IsAdminUser, so it will always give True.

My idea here is to return False in BasePermission.has_object_permission(request, obj) and override this logic in IsAdminUser and IsAuthenticated permissons.
For now it happens that object level permissions a given by default.

I can create PR for this if you agree with the idea

[tomchristie]

Object permissions are only tested if the general permissions have already passed. From my POV it makes sense for them to default to True, because that's equivalent to "We don't test for object-level permissions unless your class defines it explicitly".

[ignus2]

Hi,

I just bumped into this same issue and was baffled at first about what was happening. I too am using rest_condition, and was having a permission something like:

Or(IsAdminUser, IsObjectCreator, IsObjectEditor)

having the intuition to give permission if "the user is admin or the object creator or an object editor", but this is not what is happening, because IsAdminUser returns True (the default) for has_object_permission().

I now understand that the two paths of (general and object-level) permissions have to be examined separately when I design my permissions.

I propose to either explicitly place a warning in the docs, that general permissions do not check at the object-level, or another solution might be to implement has_object_permission() return the same as has_permission() by default or otherwise in select cases, like IsAdminUser.

I feel the source of the problem is that the general permission classes (like IsAdminUser) assume, that their own general permission check (has_permission) already passed (as the permission classes are in AND relation by default), so object-level permissions are automatically granted.
However for rest_condition, this is no longer the case, as their general check might have failed.

Greets,
Balazs

[ignus2]

Thinking a bit further, I think a workable solution for the rest_condition case would be to clearly separate the general-path and the object-level path. So a class something like this:

class CombinedCondition(BasePermission):
    """
    Extension for rest_condition to be able to separate the general and object-level paths of permission checks.
    """

    def __init__(self, gen_perm_class, obj_perm_class):
        self.gen_perm = gen_perm_class()
        self.obj_perm = obj_perm_class()
        super().__init__()

    def __call__(self):
        return self

    def has_permission(self, request, view):
        return self.gen_perm.has_permission(request, view)

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


CC = CombinedCondition

This would allow to clearly define a general permission check path with whatever conditions, and a separate object-level permission check path with different conditions.
So later you can do:

permission_classes=[CC(
    IsAuthenticated,  # General-permissions
    Or(IsAdminUserObjectLevel, IsObjectCreator, IsObjectEditor) # Object-level permissions
)]

with

class IsAdminUserObjectLevel(IsAdminUser):
    """
    A fix for the IsAdminUser base permission to check at the object-level too.
    """

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

I don't know if this makes sense, hope it helps someone in the future.

[felix-last]

For anyone finding this on google, this issue is continued at #7117