diff options
-rw-r--r-- | patchwork/api/check.py | 8 | ||||
-rw-r--r-- | patchwork/tests/api/test_check.py | 31 | ||||
-rw-r--r-- | releasenotes/notes/issue-226-27ea72266d3ee9ac.yaml | 7 |
3 files changed, 44 insertions, 2 deletions
diff --git a/patchwork/api/check.py b/patchwork/api/check.py index 4771455..0e35bd4 100644 --- a/patchwork/api/check.py +++ b/patchwork/api/check.py @@ -3,6 +3,8 @@ # # SPDX-License-Identifier: GPL-2.0-or-later +from django.http import Http404 +from django.shortcuts import get_object_or_404 from rest_framework.exceptions import PermissionDenied from rest_framework.generics import ListCreateAPIView from rest_framework.generics import RetrieveAPIView @@ -70,6 +72,10 @@ 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 + return Check.objects.prefetch_related('user').filter(patch=patch_id) @@ -86,7 +92,7 @@ class CheckListCreate(CheckMixin, ListCreateAPIView): ordering = 'id' def create(self, request, patch_id, *args, **kwargs): - p = Patch.objects.get(id=patch_id) + p = get_object_or_404(Patch, id=patch_id) if not p.is_editable(request.user): raise PermissionDenied() request.patch = p diff --git a/patchwork/tests/api/test_check.py b/patchwork/tests/api/test_check.py index 783a615..25a2a2c 100644 --- a/patchwork/tests/api/test_check.py +++ b/patchwork/tests/api/test_check.py @@ -77,6 +77,12 @@ class TestCheckAPI(APITestCase): resp = self.client.get(self.api_url(), {'user': 'otheruser'}) self.assertEqual(0, len(resp.data)) + def test_list_invalid_patch(self): + """Ensure we get a 404 for a non-existent patch.""" + resp = self.client.get( + reverse('api-check-list', kwargs={'patch_id': '99999'})) + self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code) + def test_detail(self): """Validate we can get a specific check.""" check = self._create_check() @@ -99,12 +105,21 @@ class TestCheckAPI(APITestCase): self.assertEqual(1, Check.objects.all().count()) self.assertSerialized(Check.objects.first(), resp.data) + def test_create_no_permissions(self): + """Ensure creations are rejected by standard users.""" + check = { + 'state': 'success', + 'target_url': 'http://t.co', + 'description': 'description', + 'context': 'context', + } + user = create_user() self.client.force_authenticate(user=user) resp = self.client.post(self.api_url(), check) self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code) - def test_create_invalid(self): + def test_create_invalid_state(self): """Ensure we handle invalid check states.""" check = { 'state': 'this-is-not-a-valid-state', @@ -118,6 +133,20 @@ class TestCheckAPI(APITestCase): self.assertEqual(status.HTTP_400_BAD_REQUEST, resp.status_code) self.assertEqual(0, Check.objects.all().count()) + def test_create_invalid_patch(self): + """Ensure we handle non-existent patches.""" + check = { + 'state': 'success', + 'target_url': 'http://t.co', + 'description': 'description', + 'context': 'context', + } + + self.client.force_authenticate(user=self.user) + resp = self.client.post( + reverse('api-check-list', kwargs={'patch_id': '99999'}), check) + self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code) + def test_update_delete(self): """Ensure updates and deletes aren't allowed""" check = self._create_check() diff --git a/releasenotes/notes/issue-226-27ea72266d3ee9ac.yaml b/releasenotes/notes/issue-226-27ea72266d3ee9ac.yaml new file mode 100644 index 0000000..8f891e0 --- /dev/null +++ b/releasenotes/notes/issue-226-27ea72266d3ee9ac.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + Showing checks for a non-existant patch was returning an empty response + instead of a HTTP 404. Similarly, attempting to create a new check against + this patch would result in a HTTP 5xx error instead of a HTTP 404. Both + issues are now resolved. |