From: Stephen Finucane <stephen@that.guru>
Date: Sun, 28 Oct 2018 13:31:12 +0000 (+0000)
Subject: REST: Ensure patch exists for check creation
X-Git-Tag: v2.1.2~16
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=63165ec62db3e3e1a3c3bdc057e54a256579e7c5;p=thirdparty%2Fpatchwork.git

REST: Ensure patch exists for check creation

Signed-off-by: Stephen Finucane <stephen@that.guru>
Closes: #226
(cherry picked from commit 93ce847c359234f987285edbecedc534c7cde50e)
---

diff --git a/patchwork/api/check.py b/patchwork/api/check.py
index 1cfa5375..594ecd4b 100644
--- a/patchwork/api/check.py
+++ b/patchwork/api/check.py
@@ -17,6 +17,8 @@
 # along with Patchwork; if not, write to the Free Software
 # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 
+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
@@ -84,6 +86,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)
 
 
@@ -94,7 +100,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 57d5b775..bc06e86e 100644
--- a/patchwork/tests/api/test_check.py
+++ b/patchwork/tests/api/test_check.py
@@ -90,6 +90,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()
@@ -112,12 +118,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',
@@ -131,6 +146,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 00000000..8f891e04
--- /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.