From fecf7c86c2c5b570f6bcec66cbc034b7d72f5320 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Fri, 20 Aug 2021 22:57:51 +0100 Subject: [PATCH] urls: Add missing path converters for REST APIs Almost all of the API endpoints expect numerical resource IDs, with '/projects' being the sole exception. However, we were not actually enforcing this anywhere. Instead, we were relying on the custom 'get_object_or_404' implementation used by 'GenericAPIView.retrieve' via 'GenericAPIView.get_object'. Unfortunately we weren't using this everywhere, most notably in our handler for 'GET /patches/{id}/checks'. The end result was a HTTP 500 due to a ValueError. Resolve this by adding the path converters for all REST API paths in 'patchwork.urls', along with tests to prevent regressions going forward. We also switch to the DRF variant of 'get_object_or_404' in some places to provide additional protection. Signed-off-by: Stephen Finucane Cc: Daniel Axtens [dja: s/TypeError/ValueError/; dropped an unrelated change] Signed-off-by: Daniel Axtens --- patchwork/api/check.py | 7 +++---- patchwork/tests/api/test_bundle.py | 11 +++++++++++ patchwork/tests/api/test_comment.py | 19 +++++++++++++++++-- patchwork/tests/api/test_cover.py | 11 +++++++++++ patchwork/tests/api/test_patch.py | 11 +++++++++++ patchwork/tests/api/test_person.py | 14 ++++++++++++++ patchwork/tests/api/test_project.py | 8 ++++++++ patchwork/tests/api/test_series.py | 11 +++++++++++ patchwork/tests/api/test_user.py | 14 ++++++++++++++ patchwork/urls.py | 20 ++++++++++---------- 10 files changed, 110 insertions(+), 16 deletions(-) diff --git a/patchwork/api/check.py b/patchwork/api/check.py index a6bf5f8c..5137c1b9 100644 --- a/patchwork/api/check.py +++ b/patchwork/api/check.py @@ -3,11 +3,10 @@ # # SPDX-License-Identifier: GPL-2.0-or-later -from django.http import Http404 from django.http.request import QueryDict -from django.shortcuts import get_object_or_404 import rest_framework from rest_framework.exceptions import PermissionDenied +from rest_framework.generics import get_object_or_404 from rest_framework.generics import ListCreateAPIView from rest_framework.generics import RetrieveAPIView from rest_framework.serializers import CurrentUserDefault @@ -95,8 +94,8 @@ class CheckMixin(object): def get_queryset(self): patch_id = self.kwargs['patch_id'] - if not Patch.objects.filter(pk=self.kwargs['patch_id']).exists(): - raise Http404 + # ensure the patch exists + get_object_or_404(Patch, id=self.kwargs['patch_id']) return Check.objects.prefetch_related('user').filter(patch=patch_id) diff --git a/patchwork/tests/api/test_bundle.py b/patchwork/tests/api/test_bundle.py index 79924486..1ada79c3 100644 --- a/patchwork/tests/api/test_bundle.py +++ b/patchwork/tests/api/test_bundle.py @@ -6,6 +6,7 @@ import unittest from django.conf import settings +from django.urls import NoReverseMatch from django.urls import reverse from patchwork.models import Bundle @@ -184,6 +185,16 @@ class TestBundleAPI(utils.APITestCase): self.assertIn('url', resp.data) self.assertNotIn('web_url', resp.data) + def test_detail_non_existent(self): + """Ensure we get a 404 for a non-existent bundle.""" + resp = self.client.get(self.api_url('999999')) + self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code) + + def test_detail_invalid(self): + """Ensure we get a 404 for an invalid bundle ID.""" + with self.assertRaises(NoReverseMatch): + self.client.get(self.api_url('foo')) + def _test_create_update(self, authenticate=True): user = create_user() project = create_project() diff --git a/patchwork/tests/api/test_comment.py b/patchwork/tests/api/test_comment.py index 59450d8a..22638d2f 100644 --- a/patchwork/tests/api/test_comment.py +++ b/patchwork/tests/api/test_comment.py @@ -22,6 +22,7 @@ if settings.ENABLE_REST_API: @unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API') class TestCoverComments(utils.APITestCase): + @staticmethod def api_url(cover, version=None): kwargs = {} @@ -76,12 +77,19 @@ class TestCoverComments(utils.APITestCase): with self.assertRaises(NoReverseMatch): self.client.get(self.api_url(cover, version='1.0')) - def test_list_invalid_cover(self): + def test_list_non_existent_cover(self): """Ensure we get a 404 for a non-existent cover letter.""" resp = self.client.get( reverse('api-cover-comment-list', kwargs={'pk': '99999'})) self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code) + def test_list_invalid_cover(self): + """Ensure we get a 404 for an invalid cover letter ID.""" + with self.assertRaises(NoReverseMatch): + self.client.get( + reverse('api-cover-comment-list', kwargs={'pk': 'foo'}), + ) + @unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API') class TestPatchComments(utils.APITestCase): @@ -139,8 +147,15 @@ class TestPatchComments(utils.APITestCase): with self.assertRaises(NoReverseMatch): self.client.get(self.api_url(patch, version='1.0')) - def test_list_invalid_patch(self): + def test_list_non_existent_patch(self): """Ensure we get a 404 for a non-existent patch.""" resp = self.client.get( reverse('api-patch-comment-list', kwargs={'patch_id': '99999'})) self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code) + + def test_list_invalid_patch(self): + """Ensure we get a 404 for an invalid patch ID.""" + with self.assertRaises(NoReverseMatch): + self.client.get( + reverse('api-patch-comment-list', kwargs={'patch_id': 'foo'}), + ) diff --git a/patchwork/tests/api/test_cover.py b/patchwork/tests/api/test_cover.py index 806ee6a4..1c232ddb 100644 --- a/patchwork/tests/api/test_cover.py +++ b/patchwork/tests/api/test_cover.py @@ -7,6 +7,7 @@ import email.parser import unittest from django.conf import settings +from django.urls import NoReverseMatch from django.urls import reverse from patchwork.tests.api import utils @@ -169,6 +170,16 @@ class TestCoverAPI(utils.APITestCase): self.assertNotIn('web_url', resp.data) self.assertNotIn('comments', resp.data) + def test_detail_non_existent(self): + """Ensure we get a 404 for a non-existent cover.""" + resp = self.client.get(self.api_url('999999')) + self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code) + + def test_detail_invalid(self): + """Ensure we get a 404 for an invalid cover ID.""" + with self.assertRaises(NoReverseMatch): + self.client.get(self.api_url('foo')) + def test_create_update_delete(self): user = create_maintainer() user.is_superuser = True diff --git a/patchwork/tests/api/test_patch.py b/patchwork/tests/api/test_patch.py index b94ad229..1c616409 100644 --- a/patchwork/tests/api/test_patch.py +++ b/patchwork/tests/api/test_patch.py @@ -8,6 +8,7 @@ from email.utils import make_msgid import unittest from django.conf import settings +from django.urls import NoReverseMatch from django.urls import reverse from patchwork.models import Patch @@ -261,6 +262,16 @@ class TestPatchAPI(utils.APITestCase): self.assertNotIn('web_url', resp.data) self.assertNotIn('comments', resp.data) + def test_detail_non_existent(self): + """Ensure we get a 404 for a non-existent patch.""" + resp = self.client.get(self.api_url('999999')) + self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code) + + def test_detail_invalid(self): + """Ensure we get a 404 for an invalid patch ID.""" + with self.assertRaises(NoReverseMatch): + self.client.get(self.api_url('foo')) + def test_create(self): """Ensure creations are rejected.""" project = create_project() diff --git a/patchwork/tests/api/test_person.py b/patchwork/tests/api/test_person.py index 2139574b..e9070007 100644 --- a/patchwork/tests/api/test_person.py +++ b/patchwork/tests/api/test_person.py @@ -6,6 +6,7 @@ import unittest from django.conf import settings +from django.urls import NoReverseMatch from django.urls import reverse from patchwork.tests.api import utils @@ -99,6 +100,19 @@ class TestPersonAPI(utils.APITestCase): self.assertEqual(status.HTTP_200_OK, resp.status_code) self.assertSerialized(person, resp.data, has_user=True) + def test_detail_non_existent(self): + """Ensure we get a 404 for a non-existent person.""" + user = create_user(link_person=True) + self.client.force_authenticate(user=user) + + resp = self.client.get(self.api_url('999999')) + self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code) + + def test_detail_invalid(self): + """Ensure we get a 404 for an invalid person ID.""" + with self.assertRaises(NoReverseMatch): + self.client.get(self.api_url('foo')) + def test_create_update_delete(self): """Ensure creates, updates and deletes aren't allowed""" user = create_maintainer() diff --git a/patchwork/tests/api/test_project.py b/patchwork/tests/api/test_project.py index 5c2fbe12..f2110af9 100644 --- a/patchwork/tests/api/test_project.py +++ b/patchwork/tests/api/test_project.py @@ -172,6 +172,14 @@ class TestProjectAPI(utils.APITestCase): self.assertIn('name', resp.data) self.assertNotIn('subject_match', resp.data) + def test_detail_non_existent(self): + """Ensure we get a 404 for a non-existent project.""" + resp = self.client.get(self.api_url('999999')) + self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code) + + resp = self.client.get(self.api_url('foo')) + self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code) + def test_create(self): """Ensure creations are rejected.""" project = create_project() diff --git a/patchwork/tests/api/test_series.py b/patchwork/tests/api/test_series.py index 491dd616..ef661773 100644 --- a/patchwork/tests/api/test_series.py +++ b/patchwork/tests/api/test_series.py @@ -6,6 +6,7 @@ import unittest from django.conf import settings +from django.urls import NoReverseMatch from django.urls import reverse from patchwork.tests.api import utils @@ -173,6 +174,16 @@ class TestSeriesAPI(utils.APITestCase): self.assertNotIn('mbox', resp.data['cover_letter']) self.assertNotIn('web_url', resp.data['patches'][0]) + def test_detail_non_existent(self): + """Ensure we get a 404 for a non-existent series.""" + resp = self.client.get(self.api_url('999999')) + self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code) + + def test_detail_invalid(self): + """Ensure we get a 404 for an invalid series ID.""" + with self.assertRaises(NoReverseMatch): + self.client.get(self.api_url('foo')) + def test_create_update_delete(self): """Ensure creates, updates and deletes aren't allowed""" user = create_maintainer() diff --git a/patchwork/tests/api/test_user.py b/patchwork/tests/api/test_user.py index af340dfe..9e9008b8 100644 --- a/patchwork/tests/api/test_user.py +++ b/patchwork/tests/api/test_user.py @@ -6,6 +6,7 @@ import unittest from django.conf import settings +from django.urls import NoReverseMatch from django.urls import reverse from patchwork.tests.api import utils @@ -98,6 +99,19 @@ class TestUserAPI(utils.APITestCase): self.assertEqual(status.HTTP_200_OK, resp.status_code) self.assertSerialized(user, resp.data, has_settings=True) + def test_detail_non_existent(self): + """Ensure we get a 404 for a non-existent user.""" + user = create_user() + + self.client.force_authenticate(user=user) + resp = self.client.get(self.api_url('999999')) + self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code) + + def test_detail_invalid(self): + """Ensure we get a 404 for an invalid user ID.""" + with self.assertRaises(NoReverseMatch): + self.client.get(self.api_url('foo')) + @utils.store_samples('users-update-error-forbidden') def test_update_anonymous(self): """Update user as anonymous user.""" diff --git a/patchwork/urls.py b/patchwork/urls.py index 1e6c12ab..34b07e2a 100644 --- a/patchwork/urls.py +++ b/patchwork/urls.py @@ -249,7 +249,7 @@ if settings.ENABLE_REST_API: name='api-user-list', ), path( - 'users//', + 'users//', api_user_views.UserDetail.as_view(), name='api-user-detail', ), @@ -259,7 +259,7 @@ if settings.ENABLE_REST_API: name='api-person-list', ), path( - 'people//', + 'people//', api_person_views.PersonDetail.as_view(), name='api-person-detail', ), @@ -269,7 +269,7 @@ if settings.ENABLE_REST_API: name='api-cover-list', ), path( - 'covers//', + 'covers//', api_cover_views.CoverDetail.as_view(), name='api-cover-detail', ), @@ -279,17 +279,17 @@ if settings.ENABLE_REST_API: name='api-patch-list', ), path( - 'patches//', + 'patches//', api_patch_views.PatchDetail.as_view(), name='api-patch-detail', ), path( - 'patches//checks/', + 'patches//checks/', api_check_views.CheckListCreate.as_view(), name='api-check-list', ), path( - 'patches//checks//', + 'patches//checks//', api_check_views.CheckDetail.as_view(), name='api-check-detail', ), @@ -299,7 +299,7 @@ if settings.ENABLE_REST_API: name='api-series-list', ), path( - 'series//', + 'series//', api_series_views.SeriesDetail.as_view(), name='api-series-detail', ), @@ -309,7 +309,7 @@ if settings.ENABLE_REST_API: name='api-bundle-list', ), path( - 'bundles//', + 'bundles//', api_bundle_views.BundleDetail.as_view(), name='api-bundle-detail', ), @@ -332,12 +332,12 @@ if settings.ENABLE_REST_API: api_1_1_patterns = [ path( - 'patches//comments/', + 'patches//comments/', api_comment_views.PatchCommentList.as_view(), name='api-patch-comment-list', ), path( - 'covers//comments/', + 'covers//comments/', api_comment_views.CoverCommentList.as_view(), name='api-cover-comment-list', ), -- 2.47.3