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

Make APIv3 permission classes modular #5858

Closed
wants to merge 12 commits into from
4 changes: 2 additions & 2 deletions readthedocs/api/v3/mixins.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,12 @@ def detail_objects(self, queryset, user):

def listing_objects(self, queryset, user):
project = self._get_parent_project()
if self.has_admin_permission(user, project):
if self.is_project_maintainer(user, project):
return queryset

return queryset.none()

def has_admin_permission(self, user, project):
def is_project_maintainer(self, user, project):
# Use .only for small optimization
admin_projects = self.admin_projects(user).only('id')

Expand Down
45 changes: 25 additions & 20 deletions readthedocs/api/v3/permissions.py
Original file line number Diff line number Diff line change
@@ -1,42 +1,47 @@
from rest_framework.permissions import IsAuthenticated, BasePermission
from rest_framework.permissions import BasePermission


class PublicDetailPrivateListing(IsAuthenticated):
class PublicDetailPrivateListing(BasePermission):

"""
Permission class for our custom use case.

* Always give permission for a ``detail`` request
* Only give permission for ``listing`` request if user is admin of the project
* Allow access to ``/projects`` (user's projects listing)
"""

def has_permission(self, request, view):
is_authenticated = super().has_permission(request, view)
if is_authenticated:
if view.basename == 'projects' and any([
view.action == 'list',
view.action == 'create', # used to create Form in BrowsableAPIRenderer
view.action is None, # needed for BrowsableAPIRenderer
]):
# hitting ``/projects/``, allowing
return True
if view.detail:
return True

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


if view.detail:
return True
class ListCreateProject(BasePermission):

project = view._get_parent_project()
if view.has_admin_permission(request.user, project):
return True
"""
Permission class to grant projects listing and project creation.

* Allow access to ``/projects`` (user's projects listing)
"""

return False
def has_permission(self, request, view):
if view.basename == 'projects' and any([
view.action == 'list',
view.action == 'create', # used to create Form in BrowsableAPIRenderer
view.action is None, # needed for BrowsableAPIRenderer
]):
# hitting ``/projects/``, allowing
return True


class IsProjectAdmin(BasePermission):
class IsProjectMaintainer(BasePermission):

"""Grant permission if user has admin rights on the Project."""

def has_permission(self, request, view):
project = view._get_parent_project()
if view.has_admin_permission(request.user, project):
if view.is_project_maintainer(request.user, project):
return True
5 changes: 0 additions & 5 deletions readthedocs/api/v3/tests/test_projects.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

response_json,
self._get_response_dict('projects-list_POST'),
)

def test_import_project_with_extra_fields(self):
data = {
'name': 'Test Project',
Expand Down
8 changes: 4 additions & 4 deletions readthedocs/api/v3/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@

from .filters import BuildFilter, ProjectFilter, VersionFilter
from .mixins import ProjectQuerySetMixin
from .permissions import PublicDetailPrivateListing, IsProjectAdmin
from .permissions import PublicDetailPrivateListing, ListCreateProject, IsProjectMaintainer
from .renderers import AlphabeticalSortedJSONRenderer
from .serializers import (
BuildCreateSerializer,
Expand Down Expand Up @@ -60,7 +60,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),)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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!


pagination_class = LimitOffsetPagination
LimitOffsetPagination.default_limit = 10
Expand Down Expand Up @@ -363,7 +363,7 @@ class RedirectsViewSet(APIv3Settings, NestedViewSetMixin, ProjectQuerySetMixin,
lookup_field = 'pk'
lookup_url_kwarg = 'redirect_pk'
queryset = Redirect.objects.all()
permission_classes = (IsAuthenticated & IsProjectAdmin,)
permission_classes = (IsAuthenticated & IsProjectMaintainer,)

def get_queryset(self):
queryset = super().get_queryset()
Expand Down Expand Up @@ -391,7 +391,7 @@ class EnvironmentVariablesViewSet(APIv3Settings, NestedViewSetMixin,
lookup_url_kwarg = 'environmentvariable_pk'
queryset = EnvironmentVariable.objects.all()
serializer_class = EnvironmentVariableSerializer
permission_classes = (IsAuthenticated & IsProjectAdmin,)
permission_classes = (IsAuthenticated & IsProjectMaintainer,)

def get_queryset(self):
queryset = super().get_queryset()
Expand Down