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

API V3: change permissions to allow anonymous access to public resources #11485

Merged
merged 5 commits into from
Aug 5, 2024

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Jul 18, 2024

  • No more "global" permission class, each view/action defines the permissions they need.
  • No more querysets based on the view name, we just call .api to filter resources by the current user, if anything extra is needed, each view can override the queryset.
  • Lots of tests

Ref readthedocs/ext-theme#154

@stsewd stsewd marked this pull request as ready for review July 23, 2024 02:18
@stsewd stsewd requested a review from a team as a code owner July 23, 2024 02:18
@stsewd stsewd requested a review from humitos July 23, 2024 02:18
Comment on lines +154 to +155
if self.name:
return self.name
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran into this while working on sync-versions, and glad to see it fixed :)

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a great refactor to me. I had a few questions, but overall it looks solid.

Comment on lines +154 to +155
if self.name:
return self.name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran into this while working on sync-versions, and glad to see it fixed :)

# Any other action is read-only.
else:
permission_classes = [ReadOnlyPermission]
return [permission() for permission in permission_classes]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting that you need to instantiate them.

@@ -238,7 +252,7 @@ class SubprojectRelationshipViewSet(
model = ProjectRelationship
lookup_field = "alias"
lookup_url_kwarg = "alias_slug"
queryset = ProjectRelationship.objects.all()
permission_classes = [ReadOnlyPermission | (IsAuthenticated & IsProjectAdmin)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this defined when we have the permissions setup in the APIv3Settings object?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APIv3Settings no longer has a default permission class, each view needs to explicitly declare the permissions. This is since each view requires different permissions.

get_permissions overrides all declarations of permissions, even in
actions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants