Composable permissions issue (v3.9.2)

[jhnbkr]

I am receiving unintended results when implementing a composed permission set. I am using several simple custom permissions which I have included below for reference.

The following composition works as expected:
permission_classes = [IsAuthenticated & (IsPublished | IsOwner)]

However, when I add a third OR condition the result is not as expected:
permission_classes = [IsAuthenticated & (IsPublished | IsOwner | IsAdmin)]

In a case where a user is authenticated, not the owner, not an admin, and the object is not published the composition returns true. I would expect True & (False | False | False) == False. My apologies if I am not utilizing the compose operations correctly.

Any insight is appreciated.

Django v2.1.5
DRF: v3.9.2

#permissions.py
from rest_framework import permissions

class IsAuthenticated(permissions.BasePermission):
    def has_permission(self, request, view):
        return bool(request.user and request.user.is_authenticated)

class IsAdmin(permissions.BasePermission):
    def has_permission(self, request, view):
        return bool(request.user and request.user.is_superuser)

class IsOwner(permissions.BasePermission):
    def has_object_permission(self, request, view, obj):
        owner = getattr(obj, 'owner', None)
        return bool(request.user and request.user == owner)

class IsPublished(permissions.BasePermission):
    def has_object_permission(self, request, view, obj):
        status = getattr(obj, 'status', None)
        return bool(status == 'published')

[rpkilby]

Hi @jhnbkr. By default, .has_permission() and .has_object_permission() return true.

django-rest-framework/rest_framework/permissions.py

Lines 110 to 120 in 9bfb587

	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 True

Also, there is no direct relation between the two checks. e.g., .has_permission() -> False does not directly imply .has_object_permission() -> False. However, .has_permission() is typically checked before .has_object_permission().

Given that IsOwner and IsPublished don't override .has_permission() and IsAuthenticated and IsAdmin don't override .has_object_permission(), I assume this is is what's happening:

• permission_classes = [IsAuthenticated & (IsPublished | IsOwner | IsAdmin)]
• has_permission() -> True & (True | True | False)
• then has_object_permission() -> True & (False | False | True)

How are you testing the composed permissions? Are you testing them directly, or are you testing the entire view?

[carltongibson]

Hi @jhnbkr. What @rpkilby said is almost certainly right. If you can provide a minimal code example reproducing the issue having implemented both methods on your permission classes then we can have a look. Otherwise I think this can be closed. Thanks.

[jhnbkr]

Thank you @rpkilby for the clarification. I have resolved my issue.

[FloChehab]

Hi there ! (Before saying anything else: I love the new composable permission classes 💯 )

After stumbling on a similar problem, I looked at this issue and #6402.
I feel that the way the permissions are checked when composed is misleading @rpkilby / @carltongibson . Shouldn't the permission classes that return False in the has_permission be flagged as False for the has_object_permission function (whatever this one should have returned).

So the example above would give:

• permission_classes = [IsAuthenticated & (IsPublished | IsOwner | IsAdmin)]
• has_permission() -> True & (True | True | False)
• then has_object_permission() -> True & (False | False | FALSE)

I was used to the fact that if has_permission returns False then has_object_permission would be ignored. It's not the case anymore and:

class ReadOnly(permissions.BasePermission):
    def has_permission(self, request, view):
        return request.method in permissions.SAFE_METHODS

Is (sadly) anything but "read-only" and that could result in (many) unfortunate security issues when used in composition with other classes.

As a result, wouldn't a flagging behavior (such as the one described above) be welcomed ? Put in another way, is this a bug or a wanted feature ?

[rpkilby]

This is an issue I've raised internally, but you're correct. A failing has_permission check previously meant that has_object_permission wasn't ran. However, given composable OR permissions, you may have a situation where has_permission may have failed on a specific class, but the overall check has passed. Because has_object_permission defaults to True, this check doesn't fail in the second stage.

I haven't fully thought this through, but we may need to change the API for the base permission class. My suggestions were:

• By default, has_permission/has_object_permission need to raise NotImplementedError
• Due to above, all permission classes need to implement the both methods.
• Permissions need a ternary of True/False/None, signifying Valid/Invalid/I don't care, where None does not affect the truthy/falseyness of the boolean logic, but is itself falsey. (e.g., True & None would be True, False & None would be False, and None & None would be False).

Although, the above ternary logic might be a bit complicated. We should check Django's authentication backends and other permissions-related logic for prior examples.

At least as a minimum, we should document that composable permission classes should implement both methods.

[FloChehab]

Thanks @rpkilby for the prompt feedback !
I would be in favor of 1 (&2) to make sure people know what is going on and because the default of security should be to be secured.

However, I am not familiar with the ternary logic, but as you present it I already see a big source of errors: it feels like it would be sensitive to the order of operations:

• True & None & True & None => True
• None & None & True & True => False

...that would be a nightmare 😢

I feel like permission should be dead simple:

• has_permission: checks the permission at the "viewset/queryset" level,
• has_object_permission: further checks if some permission is given at the object level or not; it nuances the previous one.
• You either have or don't have permission to do something.

Having said that, it would be great if has_permission and has_object_permission didn't need to be redundant "like before"; for example, for performance reasons: why do the exact same thing twice ? (the current workaround to the issue detailed here). Also, if I understand the code correctly, has_object_permission is called on all objects returned, so it is not a matter of "twice" but a matter of doing some potential heavy lifting permission checks 100+ times more (depending on the queryset length).

Hence the suggestion of tweaking the composable permission implementation to prevent the "double" level OR, that is hard to understand and to use purposefully (using this as feature feels like black magic for me); and might lead to huge performance drawbacks. This means permission would be composed as blocks; which is easier to comprehend.

[FloChehab]

So I was wrong about all my talk about performance issues since has_object_permission is not called for all abject of a queryset; this is especially clear in the doc. I am really sorry for that.

As you will see in #6570 I have created a POC to fix what I called the "double level" or.

[carltongibson]

At least as a minimum, we should document that composable permission classes should implement both methods.

Well, there's 6de33ef already. (Happy to see follow-ups there.)

We could also explore a system (or runtime) check on permissions used in with composition, to raise a warning and/or error.

[skorokithakis]

I was bitten by this also. The docs do mention that if has_permission fails, has_object_permission cannot be run, so I was surprised to see the check passing for a permission check that had failed.

Something like the OR operator storing the result of op1.has_permission and op2.has_permission on its instance and then doing return self.stored_result and op1.has_object_permission() on has_object_permission would solve the issue.

[xordoquy]

Doesn't #6402 fix that already ?

[skorokithakis]

It doesn't seem to, no. #6605 seems to, but runs has_permission twice. The first run can be stored somewhere, though probably not the class instance, if it's not per-request.

[xordoquy]

Whether has_permission should be called only once is another issue. #6605 ensures DRF doesn't consider has_object_permission is the has_permission is false which seems to fix the initial issue.

[tolomea]

I also got burned by this, I'm glad I took the time to write rigorous tests around the perms rather than trusting DRF to behave in the obvious way. Going forward we're banning all use of the builtin permission classes and overriding the base as such:

class BasePermission(permissions.BasePermission):
    def has_object_permission(self, request, view, obj):
        # see the note here https://www.django-rest-framework.org/api-guide/permissions/#object-level-permissions
        return self.has_permission(request, view)