Skip to content

Commit 37be26d

Browse files
authored
API V2: remove write access to superusers (#10498)
We have access to some write operations using a build API token, we no longer need to grant access to superusers. - All usage of super/staff users have been removed from the API, they have read-only access as any other user now. - APIRestrictedPermission has been renamed to ReadOnlyPermission, since that's what it checks now.
1 parent 9b968ab commit 37be26d

File tree

8 files changed

+100
-161
lines changed

8 files changed

+100
-161
lines changed

docs/dev/settings.rst

-16
Original file line numberDiff line numberDiff line change
@@ -19,22 +19,6 @@ memory
1919
Examples: '200m' for 200MB of total memory, or '2g' for 2GB of
2020
total memory.
2121

22-
SLUMBER_USERNAME
23-
----------------
24-
25-
.. Don't set this automatically, lest we leak something. We are using the dev
26-
settings in the conf.py, but it's probably a good idea to be safe.
27-
28-
The username to use when connecting to the Read the Docs API. Used for hitting the API while building the docs.
29-
30-
SLUMBER_PASSWORD
31-
----------------
32-
33-
.. Don't set this automatically, lest we leak something. We are using the dev
34-
settings in the conf.py, but it's probably a good idea to be safe.
35-
36-
The password to use when connecting to the Read the Docs API. Used for hitting the API while building the docs.
37-
3822
USE_SUBDOMAIN
3923
---------------
4024

readthedocs/api/v2/permissions.py

+5-19
Original file line numberDiff line numberDiff line change
@@ -16,28 +16,12 @@ def has_object_permission(self, request, view, obj):
1616
return request.user in obj.users.all()
1717

1818

19-
class APIRestrictedPermission(permissions.BasePermission):
19+
class ReadOnlyPermission(permissions.BasePermission):
2020

21-
"""
22-
Allow admin write, authenticated and anonymous read only.
23-
24-
This differs from :py:class:`APIPermission` by not allowing for
25-
authenticated POSTs. This permission is endpoints like ``/api/v2/build/``,
26-
which are used by admin users to coordinate build instance creation, but
27-
only should be readable by end users.
28-
"""
21+
"""Allow read-only access to authenticated and anonymous users."""
2922

3023
def has_permission(self, request, view):
31-
return (
32-
request.method in permissions.SAFE_METHODS or
33-
(request.user and request.user.is_staff)
34-
)
35-
36-
def has_object_permission(self, request, view, obj):
37-
return (
38-
request.method in permissions.SAFE_METHODS or
39-
(request.user and request.user.is_staff)
40-
)
24+
return request.method in permissions.SAFE_METHODS
4125

4226

4327
class IsAuthorizedToViewVersion(permissions.BasePermission):
@@ -89,6 +73,8 @@ class HasBuildAPIKey(BaseHasAPIKey):
8973
to avoid having to parse and validate the key again on each view.
9074
The key is injected in the ``request.build_api_key`` attribute
9175
only if it's valid, otherwise it's set to ``None``.
76+
77+
This grants read and write access to the API.
9278
"""
9379

9480
model = BuildAPIKey

readthedocs/api/v2/views/model_views.py

+23-34
Original file line numberDiff line numberDiff line change
@@ -7,19 +7,14 @@
77
from django.core.exceptions import PermissionDenied
88
from django.db.models import BooleanField, Case, Value, When
99
from django.http import Http404
10-
from django.shortcuts import get_object_or_404
1110
from django.template.loader import render_to_string
12-
from rest_framework import decorators, permissions, status, viewsets
11+
from rest_framework import decorators, status, viewsets
1312
from rest_framework.mixins import CreateModelMixin, UpdateModelMixin
1413
from rest_framework.parsers import JSONParser, MultiPartParser
1514
from rest_framework.renderers import BaseRenderer, JSONRenderer
1615
from rest_framework.response import Response
1716

18-
from readthedocs.api.v2.permissions import (
19-
APIRestrictedPermission,
20-
HasBuildAPIKey,
21-
IsOwner,
22-
)
17+
from readthedocs.api.v2.permissions import HasBuildAPIKey, IsOwner, ReadOnlyPermission
2318
from readthedocs.api.v2.utils import normalize_build_command
2419
from readthedocs.builds.constants import INTERNAL
2520
from readthedocs.builds.models import Build, BuildCommandResult, Version
@@ -129,18 +124,16 @@ class UserSelectViewSet(viewsets.ReadOnlyModelViewSet):
129124
View set that varies serializer class based on request user credentials.
130125
131126
Viewsets using this class should have an attribute `admin_serializer_class`,
132-
which is a serializer that might have more fields that only admin/staff
133-
users require. If the user is staff, this class will be returned instead.
127+
which is a serializer that might have more fields that only the builders
128+
require. If the request is using a Build API key, this class will be returned instead.
134129
135130
By default read-only endpoints will be allowed,
136131
to allow write endpoints, inherit from the proper ``rest_framework.mixins.*`` classes.
137132
"""
138133

139134
def get_serializer_class(self):
140135
try:
141-
if (
142-
self.request.user.is_staff or self.request.build_api_key
143-
) and self.admin_serializer_class is not None:
136+
if self.request.build_api_key and self.admin_serializer_class is not None:
144137
return self.admin_serializer_class
145138
except AttributeError:
146139
pass
@@ -169,7 +162,7 @@ class ProjectViewSet(DisableListEndpoint, UpdateModelMixin, UserSelectViewSet):
169162

170163
"""List, filter, etc, Projects."""
171164

172-
permission_classes = [HasBuildAPIKey | APIRestrictedPermission]
165+
permission_classes = [HasBuildAPIKey | ReadOnlyPermission]
173166
renderer_classes = (JSONRenderer,)
174167
serializer_class = ProjectSerializer
175168
admin_serializer_class = ProjectAdminSerializer
@@ -214,7 +207,7 @@ def get_queryset_for_api_key(self, api_key):
214207

215208
class VersionViewSet(DisableListEndpoint, UpdateModelMixin, UserSelectViewSet):
216209

217-
permission_classes = [HasBuildAPIKey | APIRestrictedPermission]
210+
permission_classes = [HasBuildAPIKey | ReadOnlyPermission]
218211
renderer_classes = (JSONRenderer,)
219212
serializer_class = VersionSerializer
220213
admin_serializer_class = VersionAdminSerializer
@@ -232,7 +225,7 @@ def get_queryset(self):
232225

233226

234227
class BuildViewSet(DisableListEndpoint, UpdateModelMixin, UserSelectViewSet):
235-
permission_classes = [HasBuildAPIKey | APIRestrictedPermission]
228+
permission_classes = [HasBuildAPIKey | ReadOnlyPermission]
236229
renderer_classes = (JSONRenderer, PlainTextBuildRenderer)
237230
model = Build
238231
filterset_fields = ('project__slug', 'commit')
@@ -245,7 +238,7 @@ def get_serializer_class(self):
245238
pre-process the `command` field before returning it to the user, and we
246239
also want to have a specific serializer for admins.
247240
"""
248-
if self.request.user.is_staff or self.request.build_api_key:
241+
if self.request.build_api_key:
249242
# Logic copied from `UserSelectViewSet.get_serializer_class`
250243
# and extended to choose serializer from self.action
251244
if self.action not in ["list", "retrieve"]:
@@ -255,22 +248,20 @@ def get_serializer_class(self):
255248

256249
@decorators.action(
257250
detail=False,
258-
permission_classes=[HasBuildAPIKey | permissions.IsAdminUser],
251+
permission_classes=[HasBuildAPIKey],
259252
methods=['get'],
260253
)
261254
def concurrent(self, request, **kwargs):
262255
project_slug = request.GET.get('project__slug')
263256
build_api_key = request.build_api_key
264-
if build_api_key:
265-
if project_slug != build_api_key.project.slug:
266-
log.info(
267-
"Project slug doesn't match the one attached to the API key.",
268-
project_slug=project_slug,
269-
)
270-
raise Http404()
271-
project = build_api_key.project
272-
else:
273-
project = get_object_or_404(Project, slug=project_slug)
257+
if project_slug != build_api_key.project.slug:
258+
log.warning(
259+
"Project slug doesn't match the one attached to the API key.",
260+
api_key_id=build_api_key.id,
261+
project_slug=project_slug,
262+
)
263+
raise Http404()
264+
project = build_api_key.project
274265
limit_reached, concurrent, max_concurrent = Build.objects.concurrent(project)
275266
data = {
276267
'limit_reached': limit_reached,
@@ -319,7 +310,7 @@ def retrieve(self, *args, **kwargs):
319310

320311
@decorators.action(
321312
detail=True,
322-
permission_classes=[HasBuildAPIKey | permissions.IsAdminUser],
313+
permission_classes=[HasBuildAPIKey],
323314
methods=['post'],
324315
)
325316
def reset(self, request, **kwargs):
@@ -334,27 +325,25 @@ def get_queryset_for_api_key(self, api_key):
334325

335326
class BuildCommandViewSet(DisableListEndpoint, CreateModelMixin, UserSelectViewSet):
336327
parser_classes = [JSONParser, MultiPartParser]
337-
permission_classes = [HasBuildAPIKey | APIRestrictedPermission]
328+
permission_classes = [HasBuildAPIKey | ReadOnlyPermission]
338329
renderer_classes = (JSONRenderer,)
339330
serializer_class = BuildCommandSerializer
340331
model = BuildCommandResult
341332

342333
def perform_create(self, serializer):
343334
"""Restrict creation to builds attached to the project from the api key."""
344335
build_pk = serializer.validated_data["build"].pk
345-
api_key = self.request.build_api_key
346-
if api_key and not api_key.project.builds.filter(pk=build_pk).exists():
336+
build_api_key = self.request.build_api_key
337+
if not build_api_key.project.builds.filter(pk=build_pk).exists():
347338
raise PermissionDenied()
348-
# If the request isn't attached to a build api key,
349-
# the user doing the request is a superuser, so it has access to all projects.
350339
return super().perform_create(serializer)
351340

352341
def get_queryset_for_api_key(self, api_key):
353342
return self.model.objects.filter(build__project=api_key.project)
354343

355344

356345
class DomainViewSet(DisableListEndpoint, UserSelectViewSet):
357-
permission_classes = [APIRestrictedPermission]
346+
permission_classes = [ReadOnlyPermission]
358347
renderer_classes = (JSONRenderer,)
359348
serializer_class = DomainSerializer
360349
model = Domain

0 commit comments

Comments
 (0)