IsAdminUser class allows users without authentication to access objects (security issue)

[LitvK]

Checklist

• I have verified that that issue exists against the master branch of Django REST framework.
• I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
• This is not a usage question. (Those should be directed to the discussion group instead.)
• 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.)
• 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

Create a simple view with such code:

from rest_framework_simplejwt.authentication import JWTAuthentication
from rest_framework import permissions, generics

class MyCustomPermission(permissions.BasePermission):
    def has_object_permission(self, request, view, obj):
        return False

class PartDetailView(generics.RetrieveAPIView):
    permission_classes = (permissions.IsAdminUser | MyCustomPermission,)
    authentication_classes = (JWTAuthentication,)
    queryset = Part.objects.all()
    serializer_class = PartSerializer

Try to open this view not using authentication at all (without authentication token).

Expected behavior

Framework should deny such request, because user is not authenticated and framework isn't able to check if he Admin or not.

Actual behavior

But it will return requested object to user without authentication because of this:

class BasePermission(metaclass=BasePermissionMetaclass):
    """
    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 True

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

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

IsAdminUser always returns True on has_object_permission.

Actually it works as intended after I added

    def has_permission(self, request, view):
        return

into MyCustomPermission. Maybe BasePermission shouldn't return True at every request?

[ramwin]

You should user rest_framework.permissions.IsAuthenticated in your case

permission_classes = IsAuthenticated and (MyCustomPermission or IsAdminUser)

[LitvK]

You should use rest_framework.permissions.IsAuthenticated in your case

permission_classes = IsAuthenticated and (MyCustomPermission or IsAdminUser)

Actually MyCustomPermission in my case shouldn't require authentication. :( And the main goal of my post is just let anybody know that framework may acts weird if you'll forget to define has_permission function in custom class. I believe None value in super class is much more safer that True.

[Vestemir]

Actually I agree that this implementation is counterintuitive. I think most people would expect permissions.IsAdminUser | MyCustomPermission to deny access to an object if the User is not an Admin or if CustomPermission does not allow it. Enforcing isAuthenticated on top of that shouldn't be required.

[ramwin]

I think it is not the fault of the IsAdminUser, the permission check didn't work because you use or between the IsAdminUser and your custom permission. But your custom permission class didn't block the authenticated request. You think maybe you should inherite the IsAuthenticated.

from rest_framework.permissions import IsAuthenticated

...
class MyCustomPermission(IsAuthenticated):
    def has_object_permission(self, request, view, obj):
        return False
...
class PartDetailView
    ...
    permission_classes = (IsAdminUser | MyCustomPermission)
...

I agree that the default BasePermission should return False, this is meaningfull and more safe. But this will create a huge break change for many people. Maybe some global settings like DEFAULT_PERMISSION_DENY or DEFAULT_PERMISSION_ALLOW should be added to the project.

[dannyshaw]

The question being asked is "Should permission be granted".
The default response in BasePermission is "Yes".
IsAdminUser does not override this for has_object_permission.
Therefore any permission that uses has_object_permission composed ORwise with IsAdminUser is going to grant permission.

If the job of a Permission class is to grant permission then the default should be deny.

[MattFisher]

I think the problem might be in the OR class.
The way permissions are checked, the views assume that has_permission() has already returned True for the permission currently being checked, before they check has_object_permission(), but introducing an OR construct means this assumption is no longer valid.

E.g. check_permissions for (permissions.IsAdminUser | MyCustomPermission,) will be true because MyCustomPermission.check_permissions doesn't discriminate purely by user. check_object_permissions for (permissions.IsAdminUser | MyCustomPermission,) will be true for any user and object because IsAdminUser doesn't discriminate by object.
Hence access will be granted to all objects for all users.

To make check_object_permissions valid for OR, the method needs to take the results of check_permissions into account as well, ie:

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) or
            self.op2.has_permission(request, view)
        )

    def has_object_permission(self, request, view, obj):
        # Previous
        # return (
        #     self.op1.has_object_permission(request, view, obj) or
        #     self.op2.has_object_permission(request, view, obj)
        # )
        # New
        return (
            (
                self.op1.has_permission(request, view) and            # True only for staff
                self.op1.has_object_permission(request, view, obj)    # True for all objects
            ) or (
                self.op2.has_permission(request, view) and            # True for all users
                self.op2.has_object_permission(request, view, obj)    # Only True for MyCustomPermission
            )
        )

[MattFisher]

This is actually a duplicate of #7117 and PR #6605 implements the code change above.

[tomchristie]

Closing as a duplicate. Thanks @MattFisher