Make APIv3 permission classes modular #5858

humitos · 2019-06-27T12:06:16Z

This PR just split our own permission class that handles all the cases into tree where each of them are in charge of an specific thing:

IsAuthenticated (from DRF): checks that the user is authenticated
PublicDetailPrivateListing: do not grant permissions if the user doing a listing over an object he doesn't is part of
ListCreateProject: allow listing at /api/v3/projects/ to list user's projects and also to create a project at the same endpoint by POST

Depends on #5857

This is needed to render the Form for POST when using BrowsableAPIRenderer and hitting /projects/

davidfischer · 2019-07-03T16:00:25Z

readthedocs/api/v3/permissions.py

+            return True
+
+        project = view._get_parent_project()
+        if view.has_admin_permission(request.user, project):


has_admin_permission seems poorly named to me. It isn't immediately clear what it means. I realize it means that the user is a maintainer on the project but maybe it should be more explicit. How about is_project_maintainer or something like that. It does seem to mirror the method name on the ProjectQuerySet but it is much more clear what that method does in context (Project.objects.for_admin_user is fairly clear although could be better while view.has_admin_permission is not).

is_project_maintainer is a good name, but I'm not sure it does fit with corporate (or under an organizational structure) --where only read access is granted.

Even if it doesn't fit 100%, it's still a better name 😄

davidfischer · 2019-07-03T16:09:11Z

readthedocs/api/v3/views.py

@@ -126,6 +130,20 @@ class ProjectsViewSet(APIv3Settings, NestedViewSetMixin, ProjectQuerySetMixin,
        'active_versions.last_build.config',
    ]

+    def get_serializer_class(self):


I see what you're doing here, but stylistically, I think it would be better to get rid of this method and make create() explicitly use a different serializer on incoming POST data.

In all cases, you want the JSON output to use ProjectSerializer. However, when there's user input like a user creating a project they aren't going to send every last field as some of them are automatic or have reasonable defaults so a different serializer may make sense. Since you're already overriding the output serializer in create(), I'd rather you just instead overrode the input serializer and used the default output serializer.

Actually, it looks like this is necessary due to how CreateModelMixin works.

I changed this. Although, since this PR is based on the "Import Project" one, some things changed there and I will re-check this once that one gets merged.

davidfischer · 2019-07-03T16:09:34Z

readthedocs/api/v3/views.py

@@ -50,7 +55,7 @@ class APIv3Settings:
    # Using only ``TokenAuthentication`` for now, so we can give access to
    # specific carefully selected users only
    authentication_classes = (TokenAuthentication,)
-    permission_classes = (PublicDetailPrivateListing,)
+    permission_classes = (IsAuthenticated & (ListCreateProject | PublicDetailPrivateListing),)


This is starting to make me nervous and I don't really understand how ListCreateProject is that different from PublicDetailPrivateListing.

Ouch! I thought that splitting these permissions in different classes would be easier to understand them.

Before this PR, everything was under a black box called PublicDetailPrivateListing. I decided to split it because it did more than what it name says.

My idea is:

IsAuthenticated is the first required condition,

then, the user has access to

ListCreateProject: list OR create at the /projects/ endpoint (which is kind special)

NOTE: ListCreateProject may be moved to ProjectsViewSet since it's the only place where it's needed

OR

PublicDetailPrivateListing: detail on any object or list of "private" (personal) objects.

Let me know if this helps to clarify or if you have another idea how to make it better.

I think overall, I'd feel a lot better if these special cases were not on the base class that every API endpoint extends from. The project endpoint is special (although with all the extra complexity, I'm starting to doubt that it should be) and these changes should be localized to there!

stsewd · 2019-07-16T15:29:56Z

@humitos there are merge conflicts to be resolved before review

…/apiv3-modular-permissions

humitos · 2019-07-23T11:30:53Z

@stsewd I fixed the conflicts and change the base to be master. Let me know!

humitos · 2019-07-30T15:19:01Z

@davidfischer I'd appreciate another round of review here.

davidfischer · 2019-07-30T16:55:29Z

readthedocs/api/v3/tests/test_projects.py

@@ -153,11 +153,6 @@ def test_import_project(self):
        response_json['created'] = '2019-04-29T10:00:00Z'
        response_json['modified'] = '2019-04-29T12:00:00Z'

-        self.assertDictEqual(


I we remove this, the previous couple lines don't do anything, right?

davidfischer · 2019-07-30T17:00:21Z

readthedocs/api/v3/views.py

@@ -50,7 +55,7 @@ class APIv3Settings:
    # Using only ``TokenAuthentication`` for now, so we can give access to
    # specific carefully selected users only
    authentication_classes = (TokenAuthentication,)
-    permission_classes = (PublicDetailPrivateListing,)
+    permission_classes = (IsAuthenticated & (ListCreateProject | PublicDetailPrivateListing),)


I think overall, I'd feel a lot better if these special cases were not on the base class that every API endpoint extends from. The project endpoint is special (although with all the extra complexity, I'm starting to doubt that it should be) and these changes should be localized to there!

stale · 2019-09-13T17:27:00Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

dojutsu-user · 2019-09-20T18:25:22Z

I believe this is still valid.

humitos · 2020-07-22T13:27:02Z

I will need to re-visit this PR after #6489 got merged.

humitos requested a review from a team June 27, 2019 12:06

humitos changed the base branch from master to humitos/apiv3-import-project June 27, 2019 12:06

humitos force-pushed the humitos/apiv3-import-project branch from d9f08a9 to 6302256 Compare June 27, 2019 12:07

humitos changed the base branch from humitos/apiv3-import-project to master June 27, 2019 12:08

humitos changed the base branch from master to humitos/apiv3-import-project June 27, 2019 12:08

humitos added 7 commits June 27, 2019 14:09

Make Project.respository.type a ChoiceField

c80fd05

Grant permissions on 'create' action

6f0f9fa

This is needed to render the Form for POST when using BrowsableAPIRenderer and hitting /projects/

Use standard DRF status codes

5585e28

Refactor 'trigger_initial_build' to allow calling from other places

1077e02

View/Serializer to import a Project from APIv3

766bb8d

Endpoint to import a project

67b7137

Make permissions class more modular

064889a

humitos force-pushed the humitos/apiv3-modular-permissions branch from ed42f57 to 064889a Compare June 27, 2019 12:09

humitos mentioned this pull request Jul 2, 2019

APIv3 CRUD for Redirect objects #5879

Merged

davidfischer reviewed Jul 3, 2019

View reviewed changes

humitos added 2 commits July 3, 2019 18:52

Better method name

536a64a

Simplify selection of serializer

18c4f32

humitos force-pushed the humitos/apiv3-import-project branch from c44f039 to b29b3b5 Compare July 9, 2019 08:51

humitos requested a review from davidfischer July 9, 2019 08:56

humitos requested a review from a team July 16, 2019 09:50

humitos added 2 commits July 23, 2019 13:26

Merge branch 'master' of github.com:rtfd/readthedocs.org into humitos…

3e34658

…/apiv3-modular-permissions

Rename IsProjectAdmin to IsProjectMaintainer for consistency

23e17f3

humitos changed the base branch from humitos/apiv3-import-project to master July 23, 2019 11:28

Merge conflicts

5ea105c

davidfischer reviewed Jul 30, 2019

View reviewed changes

stale bot added the Status: stale Issue will be considered inactive soon label Sep 13, 2019

stale bot closed this Sep 20, 2019

dojutsu-user reopened this Sep 20, 2019

stale bot removed the Status: stale Issue will be considered inactive soon label Sep 20, 2019

agjohnson added the Accepted Accepted issue on our roadmap label Sep 25, 2019

humitos closed this Jan 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make APIv3 permission classes modular #5858

Make APIv3 permission classes modular #5858

humitos commented Jun 27, 2019

davidfischer Jul 3, 2019

humitos Jul 3, 2019

davidfischer Jul 3, 2019

davidfischer Jul 3, 2019

humitos Jul 3, 2019

davidfischer Jul 3, 2019

humitos Jul 3, 2019

davidfischer Jul 30, 2019

stsewd commented Jul 16, 2019

humitos commented Jul 23, 2019

humitos commented Jul 30, 2019

davidfischer Jul 30, 2019

davidfischer Jul 30, 2019

stale bot commented Sep 13, 2019

dojutsu-user commented Sep 20, 2019

humitos commented Jul 22, 2020

Make APIv3 permission classes modular #5858

Make APIv3 permission classes modular #5858

Conversation

humitos commented Jun 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsewd commented Jul 16, 2019

humitos commented Jul 23, 2019

humitos commented Jul 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Sep 13, 2019

dojutsu-user commented Sep 20, 2019

humitos commented Jul 22, 2020