From 96cae63aeafc1c59f1c1c7a6303c9311d8c165ed Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 3 Jul 2024 21:37:36 -0500 Subject: [PATCH 1/4] API V3: Allow other users to see build notifications from public projects Fixes https://github.com/readthedocs/readthedocs.org/issues/11333 --- readthedocs/api/v3/permissions.py | 10 +++------- readthedocs/api/v3/views.py | 9 +++++++++ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/readthedocs/api/v3/permissions.py b/readthedocs/api/v3/permissions.py index 77d7b26afea..0c3765afe34 100644 --- a/readthedocs/api/v3/permissions.py +++ b/readthedocs/api/v3/permissions.py @@ -63,13 +63,9 @@ def has_permission(self, request, view): if view.detail and view.action in ("list", "retrieve", "superproject"): # detail view is only allowed on list/retrieve actions (not # ``update`` or ``partial_update``). - if view.basename not in ( - "projects-notifications", - "projects-builds-notifications", - ): - # We don't want to give detail access to resources' - # notifications to users that don't have access to those - # resources. + if view.basename != "projects-notifications": + # We don't want to give detail access to projects' + # notifications to users that don't have access to the project. return True if view.basename.startswith("projects"): diff --git a/readthedocs/api/v3/views.py b/readthedocs/api/v3/views.py index dbf6bee4554..83381574384 100644 --- a/readthedocs/api/v3/views.py +++ b/readthedocs/api/v3/views.py @@ -2,6 +2,7 @@ from django.contrib.auth.models import User from django.contrib.contenttypes.models import ContentType from django.db.models import Exists, OuterRef +from django.shortcuts import get_object_or_404 from rest_flex_fields import is_expanded from rest_flex_fields.views import FlexFieldsMixin from rest_framework import status @@ -477,6 +478,14 @@ class NotificationsBuildViewSet( serializer_class = NotificationSerializer queryset = Notification.objects.all() filterset_class = NotificationFilter + # Permissions are checked at the queryset level. + # We need to show build notifications to anonymous + # users on public builds. + permission_classes = () + + def _get_parent_build(self): + pk = self._get_parent_object_lookup(self.BUILD_LOOKUP_NAMES) + return get_object_or_404(Build.objects.api(user=self.request.user), pk=pk) def get_queryset(self): build = self._get_parent_build() From e0d9f9e2b402077c0fa60c40a3139999585e92b9 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 8 Jul 2024 16:25:56 -0500 Subject: [PATCH 2/4] Fix tests --- readthedocs/api/v3/tests/test_builds.py | 230 ++++++++++++++++++++++-- readthedocs/api/v3/views.py | 9 +- 2 files changed, 219 insertions(+), 20 deletions(-) diff --git a/readthedocs/api/v3/tests/test_builds.py b/readthedocs/api/v3/tests/test_builds.py index 2410d0c845e..8c392b9929c 100644 --- a/readthedocs/api/v3/tests/test_builds.py +++ b/readthedocs/api/v3/tests/test_builds.py @@ -4,6 +4,7 @@ from django.urls import reverse from readthedocs.builds.constants import EXTERNAL +from readthedocs.projects.constants import PRIVATE, PUBLIC from readthedocs.subscriptions.constants import TYPE_CONCURRENT_BUILDS from readthedocs.subscriptions.products import RTDProductFeature @@ -114,7 +115,7 @@ def test_external_version_projects_versions_builds_list_post(self): expected["version"]["urls"]["vcs"] = "https://github.com/rtfd/project/pull/v1.0" self.assertDictEqual(response_json, expected) - def test_projects_builds_notifications_list(self): + def test_projects_builds_notifications_list_anonymous_user(self): url = reverse( "projects-builds-notifications-list", kwargs={ @@ -122,19 +123,88 @@ def test_projects_builds_notifications_list(self): "parent_lookup_build__id": self.build.pk, }, ) + expected_response = self._get_response_dict( + "projects-builds-notifications-list" + ) self.client.logout() + + # Project and version are public. response = self.client.get(url) - self.assertEqual(response.status_code, 401) + self.assertEqual(response.status_code, 200) + self.assertDictEqual(response.json(), expected_response) + + # Project is private, version is public. + self.project.privacy_level = PRIVATE + self.project.save() + self.version.privacy_level = PUBLIC + self.version.save() + response = self.client.get(url) + self.assertEqual(response.status_code, 404) + + # Project and version are private. + self.project.privacy_level = PRIVATE + self.project.save() + self.version.privacy_level = PRIVATE + self.version.save() + response = self.client.get(url) + self.assertEqual(response.status_code, 404) + + # Project is public, but version is private. + self.project.privacy_level = PUBLIC + self.project.save() + self.version.privacy_level = PRIVATE + self.version.save() + response = self.client.get(url) + self.assertEqual(response.status_code, 404) + + def test_projects_builds_notifications_list(self): + url = reverse( + "projects-builds-notifications-list", + kwargs={ + "parent_lookup_project__slug": self.project.slug, + "parent_lookup_build__id": self.build.pk, + }, + ) + expected_response = self._get_response_dict( + "projects-builds-notifications-list" + ) + + self.client.logout() self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") response = self.client.get(url) + + # Project and version are public. self.assertEqual(response.status_code, 200) + self.assertDictEqual(response.json(), expected_response) - self.assertDictEqual( - response.json(), - self._get_response_dict("projects-builds-notifications-list"), - ) + # Project is private, version is public. + self.project.privacy_level = PRIVATE + self.project.save() + self.version.privacy_level = PUBLIC + self.version.save() + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + self.assertDictEqual(response.json(), expected_response) + + # Project and version are private. + self.project.privacy_level = PRIVATE + self.project.save() + self.version.privacy_level = PRIVATE + self.version.save() + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + self.assertDictEqual(response.json(), expected_response) + + # Project is public, but version is private. + self.project.privacy_level = PUBLIC + self.project.save() + self.version.privacy_level = PRIVATE + self.version.save() + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + self.assertDictEqual(response.json(), expected_response) def test_projects_builds_notifications_list_other_user(self): url = reverse( @@ -144,10 +214,40 @@ def test_projects_builds_notifications_list_other_user(self): "parent_lookup_build__id": self.build.pk, }, ) - + expected_response = self._get_response_dict( + "projects-builds-notifications-list" + ) + self.client.logout() self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.others_token.key}") + + # Project and version are public. + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + self.assertDictEqual(response.json(), expected_response) + + # Project is private, version is public. + self.project.privacy_level = PRIVATE + self.project.save() + self.version.privacy_level = PUBLIC + self.version.save() + response = self.client.get(url) + self.assertEqual(response.status_code, 404) + + # Project and version are private. + self.project.privacy_level = PRIVATE + self.project.save() + self.version.privacy_level = PRIVATE + self.version.save() + response = self.client.get(url) + self.assertEqual(response.status_code, 404) + + # Project is public, but version is private. + self.project.privacy_level = PUBLIC + self.project.save() + self.version.privacy_level = PRIVATE + self.version.save() response = self.client.get(url) - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 404) def test_projects_builds_notifications_list_post(self): url = reverse( @@ -169,6 +269,49 @@ def test_projects_builds_notifications_list_post(self): # We don't allow POST on this endpoint self.assertEqual(response.status_code, 405) + def test_projects_builds_notifitications_detail_anonymous_user(self): + url = reverse( + "projects-builds-notifications-detail", + kwargs={ + "parent_lookup_project__slug": self.project.slug, + "parent_lookup_build__id": self.build.pk, + "notification_pk": self.notification_build.pk, + }, + ) + expected_response = self._get_response_dict( + "projects-builds-notifications-detail" + ) + self.client.logout() + + # Project and version are public. + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + self.assertDictEqual(response.json(), expected_response) + + # Project is private, version is public. + self.project.privacy_level = PRIVATE + self.project.save() + self.version.privacy_level = PUBLIC + self.version.save() + response = self.client.get(url) + self.assertEqual(response.status_code, 404) + + # Project and version are private. + self.project.privacy_level = PRIVATE + self.project.save() + self.version.privacy_level = PRIVATE + self.version.save() + response = self.client.get(url) + self.assertEqual(response.status_code, 404) + + # Project is public, but version is private. + self.project.privacy_level = PUBLIC + self.project.save() + self.version.privacy_level = PRIVATE + self.version.save() + response = self.client.get(url) + self.assertEqual(response.status_code, 404) + def test_projects_builds_notifitications_detail(self): url = reverse( "projects-builds-notifications-detail", @@ -178,19 +321,44 @@ def test_projects_builds_notifitications_detail(self): "notification_pk": self.notification_build.pk, }, ) + expected_response = self._get_response_dict( + "projects-builds-notifications-detail" + ) self.client.logout() + self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") + + # Project and version are public. response = self.client.get(url) - self.assertEqual(response.status_code, 401) + self.assertEqual(response.status_code, 200) + self.assertDictEqual(response.json(), expected_response) - self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") + # Project is private, version is public. + self.project.privacy_level = PRIVATE + self.project.save() + self.version.privacy_level = PUBLIC + self.version.save() response = self.client.get(url) self.assertEqual(response.status_code, 200) + self.assertDictEqual(response.json(), expected_response) - self.assertDictEqual( - response.json(), - self._get_response_dict("projects-builds-notifications-detail"), - ) + # Project and version are private. + self.project.privacy_level = PRIVATE + self.project.save() + self.version.privacy_level = PRIVATE + self.version.save() + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + self.assertDictEqual(response.json(), expected_response) + + # Project is public, but version is private. + self.project.privacy_level = PUBLIC + self.project.save() + self.version.privacy_level = PRIVATE + self.version.save() + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + self.assertDictEqual(response.json(), expected_response) def test_projects_builds_notifitications_detail_other_user(self): url = reverse( @@ -201,10 +369,40 @@ def test_projects_builds_notifitications_detail_other_user(self): "notification_pk": self.notification_build.pk, }, ) - + expected_response = self._get_response_dict( + "projects-builds-notifications-detail" + ) + self.client.logout() self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.others_token.key}") + + # Project and version are public. + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + self.assertDictEqual(response.json(), expected_response) + + # Project is private, version is public. + self.project.privacy_level = PRIVATE + self.project.save() + self.version.privacy_level = PUBLIC + self.version.save() + response = self.client.get(url) + self.assertEqual(response.status_code, 404) + + # Project and version are private. + self.project.privacy_level = PRIVATE + self.project.save() + self.version.privacy_level = PRIVATE + self.version.save() + response = self.client.get(url) + self.assertEqual(response.status_code, 404) + + # Project is public, but version is private. + self.project.privacy_level = PUBLIC + self.project.save() + self.version.privacy_level = PRIVATE + self.version.save() response = self.client.get(url) - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 404) def test_projects_builds_notifitications_detail_post(self): url = reverse( diff --git a/readthedocs/api/v3/views.py b/readthedocs/api/v3/views.py index 83381574384..130e278a691 100644 --- a/readthedocs/api/v3/views.py +++ b/readthedocs/api/v3/views.py @@ -24,6 +24,7 @@ from rest_framework.viewsets import GenericViewSet, ModelViewSet, ReadOnlyModelViewSet from rest_framework_extensions.mixins import NestedViewSetMixin +from readthedocs.api.v2.permissions import ReadOnlyPermission from readthedocs.builds.models import Build, Version from readthedocs.core.utils import trigger_build from readthedocs.core.utils.extend import SettingsOverrideObject @@ -478,10 +479,10 @@ class NotificationsBuildViewSet( serializer_class = NotificationSerializer queryset = Notification.objects.all() filterset_class = NotificationFilter - # Permissions are checked at the queryset level. - # We need to show build notifications to anonymous - # users on public builds. - permission_classes = () + # We need to show build notifications to anonymous users + # on public builds (the queryset will filter them out). + # We allow project admins to edit notifications. + permission_classes = [ReadOnlyPermission | IsProjectAdmin] def _get_parent_build(self): pk = self._get_parent_object_lookup(self.BUILD_LOOKUP_NAMES) From 7b12604a813d4d9db49901809eb86344ec60ff22 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 9 Jul 2024 11:28:52 -0500 Subject: [PATCH 3/4] Fix --- readthedocs/api/v3/views.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/readthedocs/api/v3/views.py b/readthedocs/api/v3/views.py index 130e278a691..b89b0048dfd 100644 --- a/readthedocs/api/v3/views.py +++ b/readthedocs/api/v3/views.py @@ -485,8 +485,14 @@ class NotificationsBuildViewSet( permission_classes = [ReadOnlyPermission | IsProjectAdmin] def _get_parent_build(self): - pk = self._get_parent_object_lookup(self.BUILD_LOOKUP_NAMES) - return get_object_or_404(Build.objects.api(user=self.request.user), pk=pk) + """Overriden to filter by builds the current user has access to.""" + build_pk = self._get_parent_object_lookup(self.BUILD_LOOKUP_NAMES) + project_slug = self._get_parent_object_lookup(self.PROJECT_LOOKUP_NAMES) + return get_object_or_404( + Build.objects.api(user=self.request.user), + pk=build_pk, + project__slug=project_slug, + ) def get_queryset(self): build = self._get_parent_build() From d1534f2125e415ca1866977c6a0d62bcb1f2684e Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 9 Jul 2024 11:30:44 -0500 Subject: [PATCH 4/4] Update docstring --- readthedocs/api/v3/views.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/readthedocs/api/v3/views.py b/readthedocs/api/v3/views.py index b89b0048dfd..7a4201bd644 100644 --- a/readthedocs/api/v3/views.py +++ b/readthedocs/api/v3/views.py @@ -485,7 +485,11 @@ class NotificationsBuildViewSet( permission_classes = [ReadOnlyPermission | IsProjectAdmin] def _get_parent_build(self): - """Overriden to filter by builds the current user has access to.""" + """ + Overriden to filter by builds the current user has access to. + + This includes public builds from other projects. + """ build_pk = self._get_parent_object_lookup(self.BUILD_LOOKUP_NAMES) project_slug = self._get_parent_object_lookup(self.PROJECT_LOOKUP_NAMES) return get_object_or_404(