From 6e557de44993e0d1ec9b731f4b2556c3b2100b71 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 20 Jun 2023 16:52:07 -0500 Subject: [PATCH 1/2] API V3: add IsAuthenticated to permissions This isn't a security vulnerability, we were filtering by the current user. This was just causing errors, since we were expecting a real user in some queries. These views are used in .com only, so I have added tests there. Ref https://read-the-docs.sentry.io/issues/4261898141/?alert_rule_id=242443&alert_timestamp=1687234764178&alert_type=email&environment=production&project=161479&referrer=alert_email --- readthedocs/api/v3/views.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/readthedocs/api/v3/views.py b/readthedocs/api/v3/views.py index 5b34c4f6fd3..cfb04497765 100644 --- a/readthedocs/api/v3/views.py +++ b/readthedocs/api/v3/views.py @@ -414,7 +414,9 @@ class OrganizationsViewSet( lookup_url_kwarg = 'organization_slug' queryset = Organization.objects.all() serializer_class = OrganizationSerializer - permission_classes = [UserOrganizationsListing | IsOrganizationAdminMember] + permission_classes = [ + IsAuthenticated & (UserOrganizationsListing | IsOrganizationAdminMember) + ] permit_list_expands = [ 'projects', 'teams', @@ -443,7 +445,7 @@ class OrganizationsProjectsViewSet( lookup_url_kwarg = 'project_slug' queryset = Project.objects.all() serializer_class = ProjectSerializer - permission_classes = [IsOrganizationAdminMember] + permission_classes = [IsAuthenticated & IsOrganizationAdminMember] permit_list_expands = [ 'organization', 'organization.teams', From cdab60d9d256c8459123796cb60190707cb45802 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 20 Jun 2023 17:36:17 -0500 Subject: [PATCH 2/2] Add tests for all other endpoints --- readthedocs/api/v3/tests/test_builds.py | 63 ++++++---- .../api/v3/tests/test_environmentvariables.py | 22 ++-- readthedocs/api/v3/tests/test_projects.py | 110 +++++++++++------- readthedocs/api/v3/tests/test_redirects.py | 46 ++++---- .../api/v3/tests/test_remoteorganizations.py | 12 +- .../api/v3/tests/test_remoterepositories.py | 19 ++- readthedocs/api/v3/tests/test_subprojects.py | 79 ++++++++----- readthedocs/api/v3/tests/test_versions.py | 64 +++++----- 8 files changed, 246 insertions(+), 169 deletions(-) diff --git a/readthedocs/api/v3/tests/test_builds.py b/readthedocs/api/v3/tests/test_builds.py index 3614e853488..7d23358c587 100644 --- a/readthedocs/api/v3/tests/test_builds.py +++ b/readthedocs/api/v3/tests/test_builds.py @@ -19,26 +19,36 @@ class BuildsEndpointTests(APIEndpointMixin): def test_projects_builds_list(self): - self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}') - response = self.client.get( - reverse( - 'projects-builds-list', - kwargs={ - 'parent_lookup_project__slug': self.project.slug, - }), + url = reverse( + "projects-builds-list", + kwargs={ + "parent_lookup_project__slug": self.project.slug, + }, ) + + self.client.logout() + response = self.client.get(url) + self.assertEqual(response.status_code, 401) + + self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") + response = self.client.get(url) self.assertEqual(response.status_code, 200) def test_projects_builds_detail(self): - self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}') - response = self.client.get( - reverse( - 'projects-builds-detail', - kwargs={ - 'parent_lookup_project__slug': self.project.slug, - 'build_pk': self.build.pk, - }), + url = reverse( + "projects-builds-detail", + kwargs={ + "parent_lookup_project__slug": self.project.slug, + "build_pk": self.build.pk, + }, ) + + self.client.logout() + response = self.client.get(url) + self.assertEqual(response.status_code, 401) + + self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") + response = self.client.get(url) self.assertEqual(response.status_code, 200) self.assertDictEqual( @@ -47,16 +57,21 @@ def test_projects_builds_detail(self): ) def test_projects_versions_builds_list_post(self): - self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}') - self.assertEqual(self.project.builds.count(), 1) - response = self.client.post( - reverse( - 'projects-versions-builds-list', - kwargs={ - 'parent_lookup_project__slug': self.project.slug, - 'parent_lookup_version__slug': self.version.slug, - }), + url = reverse( + "projects-versions-builds-list", + kwargs={ + "parent_lookup_project__slug": self.project.slug, + "parent_lookup_version__slug": self.version.slug, + }, ) + + self.client.logout() + response = self.client.post(url) + self.assertEqual(response.status_code, 401) + + self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") + self.assertEqual(self.project.builds.count(), 1) + response = self.client.post(url) self.assertEqual(response.status_code, 202) self.assertEqual(self.project.builds.count(), 2) diff --git a/readthedocs/api/v3/tests/test_environmentvariables.py b/readthedocs/api/v3/tests/test_environmentvariables.py index 5f44c4b8f9e..a76ce0fe623 100644 --- a/readthedocs/api/v3/tests/test_environmentvariables.py +++ b/readthedocs/api/v3/tests/test_environmentvariables.py @@ -132,15 +132,19 @@ def test_projects_environmentvariables_list_post(self): ) def test_projects_environmentvariables_detail_delete(self): - self.assertEqual(self.project.environmentvariable_set.count(), 1) - self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}') - response = self.client.delete( - reverse( - 'projects-environmentvariables-detail', - kwargs={ - 'parent_lookup_project__slug': self.project.slug, - 'environmentvariable_pk': self.environmentvariable.pk, - }), + url = reverse( + "projects-environmentvariables-detail", + kwargs={ + "parent_lookup_project__slug": self.project.slug, + "environmentvariable_pk": self.environmentvariable.pk, + }, ) + self.client.logout() + response = self.client.delete(url) + self.assertEqual(response.status_code, 401) + + self.assertEqual(self.project.environmentvariable_set.count(), 1) + self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") + response = self.client.delete(url) self.assertEqual(response.status_code, 204) self.assertEqual(self.project.environmentvariable_set.count(), 0) diff --git a/readthedocs/api/v3/tests/test_projects.py b/readthedocs/api/v3/tests/test_projects.py index 6e4299902f8..a3e9e537bd1 100644 --- a/readthedocs/api/v3/tests/test_projects.py +++ b/readthedocs/api/v3/tests/test_projects.py @@ -18,10 +18,14 @@ class ProjectsEndpointTests(APIEndpointMixin): def test_projects_list(self): + url = reverse("projects-list") + + self.client.logout() + response = self.client.get(url) + self.assertEqual(response.status_code, 401) + self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}') - response = self.client.get( - reverse('projects-list'), - ) + response = self.client.get(url) self.assertEqual(response.status_code, 200) self.assertDictEqual( response.json(), @@ -96,21 +100,26 @@ def test_projects_list_filter_miss(self): ) def test_own_projects_detail(self): - self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}') - response = self.client.get( - reverse( - 'projects-detail', - kwargs={ - 'project_slug': self.project.slug, - }), - { - 'expand': ( - 'active_versions,' - 'active_versions.last_build,' - 'active_versions.last_build.config' - ), + url = reverse( + "projects-detail", + kwargs={ + "project_slug": self.project.slug, }, ) + data = { + "expand": ( + "active_versions," + "active_versions.last_build," + "active_versions.last_build.config" + ), + } + + self.client.logout() + response = self.client.get(url, data) + self.assertEqual(response.status_code, 401) + + self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") + response = self.client.get(url, data) self.assertEqual(response.status_code, 200) self.assertDictEqual( @@ -173,15 +182,20 @@ def test_own_projects_detail_privacy_levels_enabled(self): def test_projects_superproject(self): self._create_subproject() - self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}') - response = self.client.get( - reverse( - 'projects-superproject', - kwargs={ - 'project_slug': self.subproject.slug, - }, - ), + + url = reverse( + "projects-superproject", + kwargs={ + "project_slug": self.subproject.slug, + }, ) + + self.client.logout() + response = self.client.get(url) + self.assertEqual(response.status_code, 401) + + self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") + response = self.client.get(url) self.assertEqual(response.status_code, 200) self.assertDictEqual( @@ -247,9 +261,14 @@ def test_import_project(self): "programming_language": "py", "tags": ["test tag", "template tag"], } + url = reverse("projects-list") + + self.client.logout() + response = self.client.post(url, data) + self.assertEqual(response.status_code, 401) self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}') - response = self.client.post(reverse('projects-list'), data) + response = self.client.post(url, data) self.assertEqual(response.status_code, 201) query = Project.objects.filter(slug='test-project') @@ -381,17 +400,19 @@ def test_update_project(self): "single_version": True, "external_builds_enabled": True, } + url = reverse( + "projects-detail", + kwargs={ + "project_slug": self.project.slug, + }, + ) + + self.client.logout() + response = self.client.put(url, data) + self.assertEqual(response.status_code, 401) self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}') - response = self.client.put( - reverse( - 'projects-detail', - kwargs={ - 'project_slug': self.project.slug, - }, - ), - data, - ) + response = self.client.put(url, data) self.assertEqual(response.status_code, 204) self.project.refresh_from_db() @@ -422,16 +443,19 @@ def test_partial_update_project(self): "tags": ["partial tags", "updated"], } - self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}') - response = self.client.patch( - reverse( - 'projects-detail', - kwargs={ - 'project_slug': self.project.slug, - }, - ), - data, + url = reverse( + "projects-detail", + kwargs={ + "project_slug": self.project.slug, + }, ) + + self.client.logout() + response = self.client.patch(url, data) + self.assertEqual(response.status_code, 401) + + self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") + response = self.client.patch(url, data) self.assertEqual(response.status_code, 204) self.project.refresh_from_db() diff --git a/readthedocs/api/v3/tests/test_redirects.py b/readthedocs/api/v3/tests/test_redirects.py index 02aa768fc83..c69e9a287fc 100644 --- a/readthedocs/api/v3/tests/test_redirects.py +++ b/readthedocs/api/v3/tests/test_redirects.py @@ -170,23 +170,25 @@ def test_projects_redirects_type_sphinx_html_list_post(self): def test_projects_redirects_detail_put(self): + url = reverse( + "projects-redirects-detail", + kwargs={ + "parent_lookup_project__slug": self.project.slug, + "redirect_pk": self.redirect.pk, + }, + ) data = { 'from_url': '/changed/', 'to_url': '/toanother/', 'type': 'page', } + self.client.logout() + response = self.client.put(url, data) + self.assertEqual(response.status_code, 401) + self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}') - response = self.client.put( - reverse( - 'projects-redirects-detail', - kwargs={ - 'parent_lookup_project__slug': self.project.slug, - 'redirect_pk': self.redirect.pk, - }, - ), - data, - ) + response = self.client.put(url, data) self.assertEqual(response.status_code, 200) response_json = response.json() @@ -197,16 +199,20 @@ def test_projects_redirects_detail_put(self): ) def test_projects_redirects_detail_delete(self): - self.assertEqual(self.project.redirects.count(), 1) - self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}') - response = self.client.delete( - reverse( - 'projects-redirects-detail', - kwargs={ - 'parent_lookup_project__slug': self.project.slug, - 'redirect_pk': self.redirect.pk, - }, - ), + url = reverse( + "projects-redirects-detail", + kwargs={ + "parent_lookup_project__slug": self.project.slug, + "redirect_pk": self.redirect.pk, + }, ) + + self.client.logout() + response = self.client.delete(url) + self.assertEqual(response.status_code, 401) + + self.assertEqual(self.project.redirects.count(), 1) + self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") + response = self.client.delete(url) self.assertEqual(response.status_code, 204) self.assertEqual(self.project.redirects.count(), 0) diff --git a/readthedocs/api/v3/tests/test_remoteorganizations.py b/readthedocs/api/v3/tests/test_remoteorganizations.py index 8475e0e1a79..8c2f81eba51 100644 --- a/readthedocs/api/v3/tests/test_remoteorganizations.py +++ b/readthedocs/api/v3/tests/test_remoteorganizations.py @@ -36,10 +36,14 @@ def setUp(self): ) def test_remote_organization_list(self): - self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}') - response = self.client.get( - reverse('remoteorganizations-list') - ) + url = reverse("remoteorganizations-list") + + self.client.logout() + response = self.client.get(url) + self.assertEqual(response.status_code, 401) + + self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") + response = self.client.get(url) self.assertEqual(response.status_code, 200) self.assertDictEqual( diff --git a/readthedocs/api/v3/tests/test_remoterepositories.py b/readthedocs/api/v3/tests/test_remoterepositories.py index 0b1c164d533..34526744356 100644 --- a/readthedocs/api/v3/tests/test_remoterepositories.py +++ b/readthedocs/api/v3/tests/test_remoterepositories.py @@ -66,16 +66,15 @@ def setUp(self): ) def test_remote_repository_list(self): - self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}') - response = self.client.get( - reverse('remoterepositories-list'), - { - 'expand': ( - 'projects,' - 'remote_organization' - ) - } - ) + url = reverse("remoterepositories-list") + data = {"expand": ("projects," "remote_organization")} + + self.client.logout() + response = self.client.get(url, data) + self.assertEqual(response.status_code, 401) + + self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") + response = self.client.get(url, data) self.assertEqual(response.status_code, 200) self.assertDictEqual( diff --git a/readthedocs/api/v3/tests/test_subprojects.py b/readthedocs/api/v3/tests/test_subprojects.py index 902e42ea602..2be6f46c66e 100644 --- a/readthedocs/api/v3/tests/test_subprojects.py +++ b/readthedocs/api/v3/tests/test_subprojects.py @@ -15,15 +15,19 @@ def setUp(self): self._create_subproject() def test_projects_subprojects_list(self): - self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}') - response = self.client.get( - reverse( + url = reverse( 'projects-subprojects-list', kwargs={ 'parent_lookup_parent__slug': self.project.slug, }, - ), ) + + self.client.logout() + response = self.client.get(url) + self.assertEqual(response.status_code, 401) + + self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") + response = self.client.get(url) self.assertEqual(response.status_code, 200) self.assertDictEqual( response.json(), @@ -31,15 +35,20 @@ def test_projects_subprojects_list(self): ) def test_projects_subprojects_detail(self): - self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}') - response = self.client.get( - reverse( - 'projects-subprojects-detail', - kwargs={ - 'parent_lookup_parent__slug': self.project.slug, - 'alias_slug': self.project_relationship.alias, - }), + url = reverse( + "projects-subprojects-detail", + kwargs={ + "parent_lookup_parent__slug": self.project.slug, + "alias_slug": self.project_relationship.alias, + }, ) + + self.client.logout() + response = self.client.get(url) + self.assertEqual(response.status_code, 401) + + self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") + response = self.client.get(url) self.assertEqual(response.status_code, 200) self.assertDictEqual( response.json(), @@ -49,20 +58,23 @@ def test_projects_subprojects_detail(self): def test_projects_subprojects_list_post(self): newproject = self._create_new_project() self.assertEqual(self.project.subprojects.count(), 1) - self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}') + url = reverse( + "projects-subprojects-list", + kwargs={ + "parent_lookup_parent__slug": self.project.slug, + }, + ) data = { 'child': newproject.slug, 'alias': 'subproject-alias', } - response = self.client.post( - reverse( - 'projects-subprojects-list', - kwargs={ - 'parent_lookup_parent__slug': self.project.slug, - }, - ), - data, - ) + + self.client.logout() + response = self.client.post(url, data) + self.assertEqual(response.status_code, 401) + + self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") + response = self.client.post(url, data) self.assertEqual(response.status_code, 201) self.assertEqual(self.project.subprojects.count(), 2) @@ -233,17 +245,20 @@ def test_projects_subprojects_list_post_with_others_as_parent(self): self.assertEqual(self.others_project.subprojects.count(), 0) def test_projects_subprojects_detail_delete(self): - self.assertEqual(self.project.subprojects.count(), 1) - self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}') - response = self.client.delete( - reverse( - 'projects-subprojects-detail', - kwargs={ - 'parent_lookup_parent__slug': self.project.slug, - 'alias_slug': self.project_relationship.alias, - }, - ), + url = reverse( + "projects-subprojects-detail", + kwargs={ + "parent_lookup_parent__slug": self.project.slug, + "alias_slug": self.project_relationship.alias, + }, ) + self.client.logout() + response = self.client.delete(url) + self.assertEqual(response.status_code, 401) + + self.assertEqual(self.project.subprojects.count(), 1) + self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") + response = self.client.delete(url) self.assertEqual(response.status_code, 204) self.assertEqual(self.project.subprojects.count(), 0) diff --git a/readthedocs/api/v3/tests/test_versions.py b/readthedocs/api/v3/tests/test_versions.py index 775cf8563b7..6d28b55df4e 100644 --- a/readthedocs/api/v3/tests/test_versions.py +++ b/readthedocs/api/v3/tests/test_versions.py @@ -15,15 +15,19 @@ class VersionsEndpointTests(APIEndpointMixin): def test_projects_versions_list(self): - self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}') - response = self.client.get( - reverse( - 'projects-versions-list', - kwargs={ - 'parent_lookup_project__slug': self.project.slug, - }, - ), + url = reverse( + "projects-versions-list", + kwargs={ + "parent_lookup_project__slug": self.project.slug, + }, ) + + self.client.logout() + response = self.client.get(url) + self.assertEqual(response.status_code, 401) + + self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") + response = self.client.get(url) self.assertEqual(response.status_code, 200) response = response.json() self.assertEqual(len(response["results"]), 2) @@ -43,16 +47,20 @@ def test_others_projects_versions_list(self): self.assertEqual(response.status_code, 403) def test_projects_versions_detail(self): - self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}') - response = self.client.get( - reverse( - 'projects-versions-detail', - kwargs={ - 'parent_lookup_project__slug': self.project.slug, - 'version_slug': 'v1.0', - }, - ), + url = reverse( + "projects-versions-detail", + kwargs={ + "parent_lookup_project__slug": self.project.slug, + "version_slug": "v1.0", + }, ) + + self.client.logout() + response = self.client.get(url) + self.assertEqual(response.status_code, 401) + + self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") + response = self.client.get(url) self.assertEqual(response.status_code, 200) self.assertDictEqual( response.json(), @@ -147,22 +155,24 @@ def test_projects_versions_detail_unique(self): def test_projects_versions_partial_update(self): self.assertTrue(self.version.active) self.assertFalse(self.version.hidden) + url = reverse( + "projects-versions-detail", + kwargs={ + "parent_lookup_project__slug": self.project.slug, + "version_slug": self.version.slug, + }, + ) data = { 'active': False, 'hidden': True, } + self.client.logout() + response = self.client.patch(url, data) + self.assertEqual(response.status_code, 401) + self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}') - response = self.client.patch( - reverse( - 'projects-versions-detail', - kwargs={ - 'parent_lookup_project__slug': self.project.slug, - 'version_slug': self.version.slug, - }, - ), - data, - ) + response = self.client.patch(url, data) self.assertEqual(response.status_code, 204) self.version.refresh_from_db()