Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change get_permissions to return list #500

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mschoettle
Copy link

@mschoettle mschoettle commented Oct 23, 2023

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
Copy link
Author

mschoettle commented Oct 23, 2023

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 mschoettle marked this pull request as draft October 24, 2023 13:10
@mschoettle
Copy link
Author

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
Copy link
Contributor

intgr commented Oct 24, 2023

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
Copy link
Contributor

intgr commented Oct 24, 2023

The test / stubtest (3.12) failure is unrelated to your PR, it's due to upstream PR typeddjango/django-stubs#1771

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants