[PATCH] urls: Add missing path converters for REST APIs
Stephen Finucane
stephen at that.guru
Sat Aug 21 07:57:51 AEST 2021
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 TypeError.
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 at that.guru>
Cc: Daniel Axtens <dja at axtens.net>
---
patchwork/api/base.py | 3 ++-
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 ++++++++++----------
11 files changed, 112 insertions(+), 17 deletions(-)
diff --git patchwork/api/base.py patchwork/api/base.py
index 6cb703b1..5368c0cb 100644
--- patchwork/api/base.py
+++ patchwork/api/base.py
@@ -59,7 +59,8 @@ class MultipleFieldLookupMixin(object):
queryset = self.filter_queryset(self.get_queryset())
filter_kwargs = {}
for field_name, field in zip(
- self.lookup_fields, self.lookup_url_kwargs):
+ self.lookup_fields, self.lookup_url_kwargs,
+ ):
if self.kwargs[field]:
filter_kwargs[field_name] = self.kwargs[field]
diff --git patchwork/api/check.py patchwork/api/check.py
index a6bf5f8c..5137c1b9 100644
--- patchwork/api/check.py
+++ 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 patchwork/tests/api/test_bundle.py patchwork/tests/api/test_bundle.py
index 79924486..1ada79c3 100644
--- patchwork/tests/api/test_bundle.py
+++ 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 patchwork/tests/api/test_comment.py patchwork/tests/api/test_comment.py
index 59450d8a..22638d2f 100644
--- patchwork/tests/api/test_comment.py
+++ 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 patchwork/tests/api/test_cover.py patchwork/tests/api/test_cover.py
index 806ee6a4..1c232ddb 100644
--- patchwork/tests/api/test_cover.py
+++ 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 patchwork/tests/api/test_patch.py patchwork/tests/api/test_patch.py
index b94ad229..1c616409 100644
--- patchwork/tests/api/test_patch.py
+++ 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 patchwork/tests/api/test_person.py patchwork/tests/api/test_person.py
index 2139574b..e9070007 100644
--- patchwork/tests/api/test_person.py
+++ 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 patchwork/tests/api/test_project.py patchwork/tests/api/test_project.py
index 5c2fbe12..f2110af9 100644
--- patchwork/tests/api/test_project.py
+++ 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 patchwork/tests/api/test_series.py patchwork/tests/api/test_series.py
index 491dd616..ef661773 100644
--- patchwork/tests/api/test_series.py
+++ 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 patchwork/tests/api/test_user.py patchwork/tests/api/test_user.py
index af340dfe..9e9008b8 100644
--- patchwork/tests/api/test_user.py
+++ 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 patchwork/urls.py patchwork/urls.py
index 1e6c12ab..34b07e2a 100644
--- patchwork/urls.py
+++ 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',
),
--
2.31.1
More information about the Patchwork
mailing list