search

typeddjango / djangorestframework-stubs

PEP-484 stubs for django-rest-framework
MIT License
439 stars 115 forks source link

change get_permissions to return list #500

Open mschoettle opened 11 months ago

mschoettle commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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?

intgr commented 11 months ago

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 commented 11 months ago

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