BasePermission returning True for has_permission and has_object_permission is a huge gotcha

[jzwick-narmi]

When using the negation of a permission, for example an implementation of ~IsOAuthAuthentication, that BasePermission. has_object_permission returs a static True is a very tricky gotcha.

Where I would expect this permission to be False, the negation turning the condition to True, it winds up being False when checking object_permissions, so the negation that swaps it back to False, which ultimately fails the boolean evaluation.

BasePermission should look like this;

class BasePermission(metaclass=BasePermissionMetaclass):
    def has_permission(self, request, view):
        return True

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

This will behave as one would intuitively expect both in the negated permission scenario, and the unnegated. It still returns True by "default", and if has_permission is ever False, it's likely not undesirable for has_object_permission to also be False.

[bryan-brancotte]

Hi,

I would add that this is very tricky/bugged as even with built-in permission we get issue, using a simple ~permissions.IsAdminUser does not work.

from tests.test_permissions import basic_auth_header, BasicSerializer
from rest_framework import authentication, generics, permissions, status
# ...

class PermissionCompositionWithModelTests(TestCase):
    def test_negation_is_admin_short(self):
        User.objects.create_user('admin_user', 'a@a.aa', 'p', is_staff=False)
        admin_user_credentials = basic_auth_header('admin_user', 'p')
        BasicModel(text='foo').save()  # expected to be pk=1

        class NegationIsAdminInstanceView(generics.RetrieveUpdateDestroyAPIView):
            queryset = BasicModel.objects.all()
            serializer_class = BasicSerializer
            authentication_classes = [authentication.BasicAuthentication]
            permission_classes = [~permissions.IsAdminUser]

        request = factory.put(f'/1', {'text': 'foobar'}, format='json',
                              HTTP_AUTHORIZATION=admin_user_credentials)

        # User is not admin anymore, should work with permission negation
        response = NegationIsAdminInstanceView().as_view()(request, pk=1)
        self.assertEqual(response.status_code, status.HTTP_200_OK)  # Is failing but should not

Tested on main branch 8d4c2d0

Using a permission patch as proposed works

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

BUT this would not solve all problem, indeed permission based on has_object_permission where has_permission is kept to default implementation with return True will always return False when negated. Think of a ~IsItsAuthor to prevent voting for your own article.

Should there be warning when using the ~, documentation explaining its pitfall, deprecation of the operator, ... I don't know, but something should be done.

[bryan-brancotte]

related issue : #6598