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 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@that.guru>
Cc: Daniel Axtens <dja@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(-)

Stephen Finucane <stephen@that.guru> writes:

> 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.

A ValueError, not a TypeError:
 Exception Type: ValueError at /api/patches/abc@def/comments/
 Exception Value: invalid literal for int() with base 10: 'abc@def'

> 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.

LGTM, applied with a tweaked commit message and the change mentioned below:

>
> Signed-off-by: Stephen Finucane <stephen@that.guru>
> Cc: Daniel Axtens <dja@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,
> +        ):

AFAICT this is purely a cosmetic change in an otherwise-unmodified file,
so I have dropped it.

>              if self.kwargs[field]:
>                  filter_kwargs[field_name] = self.kwargs[field]
>

The rest of the patch looks good, and the full suite of tox tests pass.*

Kind regards,
Daniel

* no they don't, the docs fail, but that's not because of this series.


> 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):
> +

(I nearly dropped this hunk too, but you're also making other changes in
this file, so I relented and kept it.)

>      @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