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 <stephen@that.guru>
Cc: Daniel Axtens <dja@axtens.net>
[dja: s/TypeError/ValueError/; dropped an unrelated change]
Signed-off-by: Daniel Axtens <dja@axtens.net>

diff --git a/patchwork/api/check.py b/patchwork/api/check.py
index a6bf5f8cab89b97a7f9683917d662a4c1eea0063..5137c1b9d5c17cef69657e7d96733c1fde78df3a 100644 (file)
--- 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 799244861996595c4aa2246101b628a57059c6ef..1ada79c347be16463d6e1a1e53bda9f387bfd537 100644 (file)
--- 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 59450d8ab729d6041f3824ec296e137d1c089da6..22638d2f21ba01c0905fb7651262cd7f06a9c2e2 100644 (file)
--- 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 806ee6a4834d51ecc9d6f3d29fe1c01be8ee62f8..1c232ddb73b6cfa3d82ad9d430aa44dadb480cb1 100644 (file)
--- 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 b94ad2290dc4f6f427eff53439163ced1fcca1c0..1c61640921a926faccb7076cab656d8129e2b430 100644 (file)
--- 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 2139574b524a26a055c70bc48eec706401ea3cfc..e90700073b4ca283350eeafe725d5dc43df35579 100644 (file)
--- 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 5c2fbe12ef1af3572744aa072304f78bfb10f6ab..f2110af9cda85df4bc7af339c95a0a836a46ede9 100644 (file)
--- 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 491dd616e4229e695c190423dd08d8ad46ae9f83..ef6617730bd69f75228d54b3b75cd3aca26f1c30 100644 (file)
--- 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 af340dfe272e1be4a0d9a0e051724ba0dc25ee06..9e9008b8ef88c6449940fb937032a6790f353b30 100644 (file)
--- 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 1e6c12ab2232d623b8b6bd56d97eb05ee4033b07..34b07e2a10c5d5cc4fba4afa829c50f594f25839 100644 (file)
--- a/patchwork/urls.py
+++ b/patchwork/urls.py
@@ -249,7 +249,7 @@ if settings.ENABLE_REST_API:
             name='api-user-list',
         ),
         path(
-            'users/<pk>/',
+            'users/<int:pk>/',
             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/<pk>/',
+            'people/<int:pk>/',
             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/<pk>/',
+            'covers/<int:pk>/',
             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/<pk>/',
+            'patches/<int:pk>/',
             api_patch_views.PatchDetail.as_view(),
             name='api-patch-detail',
         ),
         path(
-            'patches/<patch_id>/checks/',
+            'patches/<int:patch_id>/checks/',
             api_check_views.CheckListCreate.as_view(),
             name='api-check-list',
         ),
         path(
-            'patches/<patch_id>/checks/<check_id>/',
+            'patches/<int:patch_id>/checks/<int:check_id>/',
             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/<pk>/',
+            'series/<int:pk>/',
             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/<pk>/',
+            'bundles/<int:pk>/',
             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/<patch_id>/comments/',
+            'patches/<int:patch_id>/comments/',
             api_comment_views.PatchCommentList.as_view(),
             name='api-patch-comment-list',
         ),
         path(
-            'covers/<pk>/comments/',
+            'covers/<int:pk>/comments/',
             api_comment_views.CoverCommentList.as_view(),
             name='api-cover-comment-list',
         ),