djangorestframework-stubs icon indicating copy to clipboard operation
djangorestframework-stubs copied to clipboard

change get_permissions to return list

Open mschoettle opened this issue 2 years ago • 4 comments

I have made things!

Proposal to change the signature of get_permissions to list[BasePermission]. This allows one to customize the permissions of a view as suggested by the documentation: https://www.django-rest-framework.org/api-guide/permissions/#overview-of-access-restriction-methods

The source code also suggests that is is a list:

https://github.com/encode/django-rest-framework/blob/f56b85b7dd7e4f786e0769bba6b7609d4507da83/rest_framework/views.py#L278

It was originally changed in #320 to Sequence[_SupportsHasPermission]. ~I am not sure if the element type has to be _SupportsHasPermission instead of BasePermission. All the other method signatures use the base type.~

Update: In the end I reverted back for the same reason why the return type was changed to Sequence in #320 and added a few more test cases.

Related issues

Refs #319 (PR: #320)

mschoettle avatar Oct 23 '23 19:10 mschoettle

I found a case that supports _SupportsHasPermission instead of BasePermission:

def get_permissions(self) -> Sequence[_SupportsHasPermission]:
        if self.action == 'list':
            return [OR(IsAdminUser(), SomeOtherPermission())]

        return super().get_permissions()

mschoettle avatar Oct 23 '23 20:10 mschoettle

I had to revert back to Sequence. Given that in some scenarios (see added test cases) _SupportsHasPermission has to be exposed I think it would be best to make it part of the public API. Thoughts?

mschoettle avatar Oct 24 '23 13:10 mschoettle

Thanks. I'll have a look in the evening. (feel free to ping if I forget)

I'm not using the DRF permission system myself, so I'm not very familiar with it.

Is it worth investigating support for boolean operators as well, e.g. IsAuthenticated & IsAdminUser, or are they deprecated? I think the | operator is particularly problematic because it conflicts with PEP 604 type union operator.

intgr avatar Oct 24 '23 13:10 intgr

The test / stubtest (3.12) failure is unrelated to your PR, it's due to upstream PR https://github.com/typeddjango/django-stubs/pull/1771

intgr avatar Oct 24 '23 13:10 intgr