From patchwork Mon Apr 29 17:07:53 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Axtens X-Patchwork-Id: 1092714 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.ozlabs.org (lists.ozlabs.org [203.11.71.2]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 44tB2K2QQSz9s6w for ; Tue, 30 Apr 2019 03:08:41 +1000 (AEST) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=axtens.net Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=axtens.net header.i=@axtens.net header.b="CUDJHmY2"; dkim-atps=neutral Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 44tB2K1MhpzDqSM for ; Tue, 30 Apr 2019 03:08:41 +1000 (AEST) X-Original-To: patchwork@lists.ozlabs.org Delivered-To: patchwork@lists.ozlabs.org Authentication-Results: lists.ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=axtens.net (client-ip=2607:f8b0:4864:20::530; helo=mail-pg1-x530.google.com; envelope-from=dja@axtens.net; receiver=) Authentication-Results: lists.ozlabs.org; dmarc=none (p=none dis=none) header.from=axtens.net Authentication-Results: lists.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=axtens.net header.i=@axtens.net header.b="CUDJHmY2"; dkim-atps=neutral Received: from mail-pg1-x530.google.com (mail-pg1-x530.google.com [IPv6:2607:f8b0:4864:20::530]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 44tB1k6cLYzDqSr for ; Tue, 30 Apr 2019 03:08:10 +1000 (AEST) Received: by mail-pg1-x530.google.com with SMTP id h1so5460710pgs.2 for ; Mon, 29 Apr 2019 10:08:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=axtens.net; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=iGRdCV2/g9gzq1gABBaXZP89fqY3Q1/YfEkNzscuNws=; b=CUDJHmY2yK5Lgl1GgTIga4m/W2hZuLaNHyGQzRiaWWOL/DGVOKCvqQaftotkCH9sgZ 8GCcc2yQ5x9alImP65sNOqTOlVrPtzNq57DilvFp/qS+deHgf61YMC8eW28D7AKM8pc3 Qy4wP9+/BqzwBltEPNQhDEDMQi2WaTOW2O2QI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=iGRdCV2/g9gzq1gABBaXZP89fqY3Q1/YfEkNzscuNws=; b=mOLDbpOiTtDUJrc7gIlCQlYx7QDvqbHNuEOMRqLc8++WzJgbR69PFDameGQYOjcu2T xRNeVkZphIzcbpsNHkn98Y1ISHhGlkUDYb/xJgzNsO1Ta+mnPST/XnSoazAPQUPEW5oj TKEQ4bGf8WTjFJYexWYtKbjp6TgVlpRlVe0UzWsbDGn9d2jIXE8N4cS7HobUAbDFAZKz 0V3cnfroCs1mXxyqhaBJrFXSLzYX5MRI6OXF9oFuAxKbSTZQG9ayKSQSunznZ3+NCd+r VmCyqe1tajD278G9y0wRU57KMfAGyR8+JqfN+8jlkg96kcUrvETGeNJOJjvv/+x1esw6 CGeA== X-Gm-Message-State: APjAAAW+Mo0JTyoUfVC1655vZ7yki+zFuHFYx9mv8aj9u0uE+VrAlNOs ebpfptLltC0vkvQETSx5GPW+V5eB6XQ= X-Google-Smtp-Source: APXvYqybPTxlk0w605yiIaAuOI0MV8h4iXP4OjLhbupRlDVlVihFEKPHoajuuXwOWQ1FNzmoM8EwBQ== X-Received: by 2002:aa7:918b:: with SMTP id x11mr30549311pfa.181.1556557688310; Mon, 29 Apr 2019 10:08:08 -0700 (PDT) Received: from localhost (203-217-53-131.dyn.iinet.net.au. [203.217.53.131]) by smtp.gmail.com with ESMTPSA id h189sm71836397pfc.125.2019.04.29.10.08.06 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 29 Apr 2019 10:08:07 -0700 (PDT) From: Daniel Axtens To: patchwork@lists.ozlabs.org Subject: [PATCH 1/2] REST: Handle regular form data requests for checks Date: Tue, 30 Apr 2019 03:07:53 +1000 Message-Id: <20190429170754.32644-2-dja@axtens.net> X-Mailer: git-send-email 2.19.1 In-Reply-To: <20190429170754.32644-1-dja@axtens.net> References: <20190429170754.32644-1-dja@axtens.net> MIME-Version: 1.0 X-BeenThere: patchwork@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Patchwork development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: patchwork-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org Sender: "Patchwork" 08d1459a4a40 ("Add REST API validation using OpenAPI schema") moved all API requests to JSON blobs rather than form data. dc48fbce99ef ("REST: Handle JSON requests") attempted to change the check serialiser to handle this. However, because both a JSON dict and a QueryDict satisfy isinstance(data, dict), everything was handled as JSON and the old style requests were broken. Found in the process of debugging issues from the OzLabs PW & Snowpatch crew - I'm not sure if they actually hit this one, but kudos to them anyway as we wouldn't have found it without them. Fixes: 08d1459a4a40 ("Add REST API validation using OpenAPI schema") Fixes: dc48fbce99ef ("REST: Handle JSON requests") Signed-off-by: Daniel Axtens --- This will need to go back to stable. --- patchwork/api/check.py | 7 ++-- patchwork/tests/api/test_check.py | 67 +++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 3 deletions(-) diff --git a/patchwork/api/check.py b/patchwork/api/check.py index 1f9fe06866a2..4d2181d0a04b 100644 --- a/patchwork/api/check.py +++ b/patchwork/api/check.py @@ -4,6 +4,7 @@ # 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 from rest_framework.exceptions import PermissionDenied from rest_framework.generics import ListCreateAPIView @@ -39,9 +40,7 @@ class CheckSerializer(HyperlinkedModelSerializer): if label != data['state']: continue - if isinstance(data, dict): # json request - data['state'] = val - else: # form-data request + if isinstance(data, QueryDict): # form-data request # NOTE(stephenfin): 'data' is essentially 'request.POST', which # is immutable by default. However, there's no good reason for # this to be this way [1], so temporarily unset that mutability @@ -52,6 +51,8 @@ class CheckSerializer(HyperlinkedModelSerializer): data._mutable = True # noqa data['state'] = val data._mutable = mutable # noqa + else: # json request + data['state'] = val break return super(CheckSerializer, self).run_validation(data) diff --git a/patchwork/tests/api/test_check.py b/patchwork/tests/api/test_check.py index 0c10b94553d3..1cfdff6e757b 100644 --- a/patchwork/tests/api/test_check.py +++ b/patchwork/tests/api/test_check.py @@ -18,6 +18,10 @@ from patchwork.tests.utils import create_user if settings.ENABLE_REST_API: from rest_framework import status + from rest_framework.test import APITestCase as BaseAPITestCase +else: + # stub out APITestCase + from django.test import TestCase as BaseAPITestCase @unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API') @@ -174,3 +178,66 @@ class TestCheckAPI(utils.APITestCase): resp = self.client.delete(self.api_url(check)) self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code) + + +@unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API') +class TestCheckAPIMultipart(BaseAPITestCase): + """Test a minimal subset of functionality where the data is passed as + multipart form data rather than as a JSON blob. + + We focus on the POST path exclusively and only on state validation: + everything else should be handled in the JSON tests. + + This is required due to the difference in handling JSON vs form-data in + CheckSerializer's run_validation(). + """ + fixtures = ['default_tags'] + + def setUp(self): + super(TestCheckAPIMultipart, self).setUp() + project = create_project() + self.user = create_maintainer(project) + self.patch = create_patch(project=project) + + def assertSerialized(self, check_obj, check_json): + self.assertEqual(check_obj.id, check_json['id']) + self.assertEqual(check_obj.get_state_display(), check_json['state']) + self.assertEqual(check_obj.target_url, check_json['target_url']) + self.assertEqual(check_obj.context, check_json['context']) + self.assertEqual(check_obj.description, check_json['description']) + self.assertEqual(check_obj.user.id, check_json['user']['id']) + + def _test_create(self, user, state='success'): + check = { + 'target_url': 'http://t.co', + 'description': 'description', + 'context': 'context', + } + if state is not None: + check['state'] = state + + self.client.force_authenticate(user=user) + return self.client.post( + reverse('api-check-list', args=[self.patch.id]), + check) + + def test_creates(self): + """Create a set of checks. + """ + resp = self._test_create(user=self.user) + self.assertEqual(status.HTTP_201_CREATED, resp.status_code) + self.assertEqual(1, Check.objects.all().count()) + self.assertSerialized(Check.objects.last(), resp.data) + + resp = self._test_create(user=self.user, state='pending') + self.assertEqual(status.HTTP_201_CREATED, resp.status_code) + self.assertEqual(2, Check.objects.all().count()) + self.assertSerialized(Check.objects.last(), resp.data) + + # you can also use the numeric ID of the state, the API explorer does + resp = self._test_create(user=self.user, state=2) + self.assertEqual(status.HTTP_201_CREATED, resp.status_code) + self.assertEqual(3, Check.objects.all().count()) + # we check against the string version + resp.data['state'] = 'warning' + self.assertSerialized(Check.objects.last(), resp.data) From patchwork Mon Apr 29 17:07:54 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Axtens X-Patchwork-Id: 1092716 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 44tB2b335Gz9sCJ for ; Tue, 30 Apr 2019 03:08:55 +1000 (AEST) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=axtens.net Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=axtens.net header.i=@axtens.net header.b="IrMPFUzI"; dkim-atps=neutral Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 44tB2b24dBzDqTw for ; Tue, 30 Apr 2019 03:08:55 +1000 (AEST) X-Original-To: patchwork@lists.ozlabs.org Delivered-To: patchwork@lists.ozlabs.org Authentication-Results: lists.ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=axtens.net (client-ip=2607:f8b0:4864:20::444; helo=mail-pf1-x444.google.com; envelope-from=dja@axtens.net; receiver=) Authentication-Results: lists.ozlabs.org; dmarc=none (p=none dis=none) header.from=axtens.net Authentication-Results: lists.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=axtens.net header.i=@axtens.net header.b="IrMPFUzI"; dkim-atps=neutral Received: from mail-pf1-x444.google.com (mail-pf1-x444.google.com [IPv6:2607:f8b0:4864:20::444]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 44tB1s0g9KzDqSg for ; Tue, 30 Apr 2019 03:08:16 +1000 (AEST) Received: by mail-pf1-x444.google.com with SMTP id g3so5629288pfi.4 for ; Mon, 29 Apr 2019 10:08:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=axtens.net; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=sPTZcI+rqBJiWNw0xcUZy6PojdlT2vnBC80xuZLq6bE=; b=IrMPFUzIokNEOH0uNY6/FJd9MihOkEdZO1b7DeVVv4rrp+gtFbZBtMCo5dLyloqZ8I LbZCuH22y9o+34gn4LgJcOxvqeE/YdLIfWnCbT6yjAYscA/vOO3K+9/yrmiPkBc/RMKh kT6A4mCpGZ7p34MTQLmrHI8T/mzBmkJdculqg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=sPTZcI+rqBJiWNw0xcUZy6PojdlT2vnBC80xuZLq6bE=; b=emurRmVsBdIy+2cnamMfjQvB1vBs8IML071yZVxkfWtNRZo1Gg/OiNYn8wHAFZZNtz XxiWg1PpnB9FFOmny3x3RAqIdusAAKf8zxRGBF21rOVPLT+iJh4BdRJHUsdbV0lUtVFK VrKSjJQnxxycV8xWaw4JuoA/i5TSy6jII8F7+/gs1c9UtK7sxkVrJ5jHcMjdp4yr5tv/ H5k38GFKkKB8Jdu+fRoYv5+mOaZ8JE8etbLLHn7IPZdZ2jHrLLFE+ROW2sCU2qRRCbfc 8QU1TlCYOtTXY+yKPPPIv+KnvPvbitBaL3aYF7Cs4aMqpUtqNO13YhizYA0BvskOFDgM kjyQ== X-Gm-Message-State: APjAAAXsjjqJzw7Efl8YYCr+3XCWjeGu9DMEkgl3j5y+8+LJzxWSRGeL xMNTUkoIrIaOGTpnSrH2KrE2ozkNs7M= X-Google-Smtp-Source: APXvYqxLQ1+W1KoKUegMIWPyneOqxHq9R3epvYXQunVkRpBdH2DoAEBvFlR+H+6i2FNuVQROYWvM+g== X-Received: by 2002:aa7:9194:: with SMTP id x20mr33956600pfa.29.1556557694130; Mon, 29 Apr 2019 10:08:14 -0700 (PDT) Received: from localhost (203-217-53-131.dyn.iinet.net.au. [203.217.53.131]) by smtp.gmail.com with ESMTPSA id f6sm38205834pgq.11.2019.04.29.10.08.12 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 29 Apr 2019 10:08:13 -0700 (PDT) From: Daniel Axtens To: patchwork@lists.ozlabs.org Subject: [PATCH 2/2] REST: A check must specify a state Date: Tue, 30 Apr 2019 03:07:54 +1000 Message-Id: <20190429170754.32644-3-dja@axtens.net> X-Mailer: git-send-email 2.19.1 In-Reply-To: <20190429170754.32644-1-dja@axtens.net> References: <20190429170754.32644-1-dja@axtens.net> MIME-Version: 1.0 X-BeenThere: patchwork@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Patchwork development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: patchwork-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org Sender: "Patchwork" The Ozlabs crew noticed that a check without a state caused a KeyError in data['state']. Mark state as mandatory, check for it, and add a test. Reported-by: Russell Currey Reported-by: Jeremy Kerr Signed-off-by: Daniel Axtens --- Also needs to go to stable. --- docs/api/schemas/latest/patchwork.yaml | 2 ++ docs/api/schemas/patchwork.j2 | 2 ++ docs/api/schemas/v1.0/patchwork.yaml | 2 ++ docs/api/schemas/v1.1/patchwork.yaml | 2 ++ patchwork/api/check.py | 4 ++++ patchwork/tests/api/test_check.py | 17 +++++++++++++++++ 6 files changed, 29 insertions(+) diff --git a/docs/api/schemas/latest/patchwork.yaml b/docs/api/schemas/latest/patchwork.yaml index e3ba69c5c64f..724b05ebf1b3 100644 --- a/docs/api/schemas/latest/patchwork.yaml +++ b/docs/api/schemas/latest/patchwork.yaml @@ -1316,6 +1316,8 @@ components: nullable: true CheckCreate: type: object + required: + - state properties: state: title: State diff --git a/docs/api/schemas/patchwork.j2 b/docs/api/schemas/patchwork.j2 index 7d3486387ede..5e2f5e4ddc74 100644 --- a/docs/api/schemas/patchwork.j2 +++ b/docs/api/schemas/patchwork.j2 @@ -1319,6 +1319,8 @@ components: nullable: true CheckCreate: type: object + required: + - state properties: state: title: State diff --git a/docs/api/schemas/v1.0/patchwork.yaml b/docs/api/schemas/v1.0/patchwork.yaml index 11e3ae30adc0..02f3a1561b7b 100644 --- a/docs/api/schemas/v1.0/patchwork.yaml +++ b/docs/api/schemas/v1.0/patchwork.yaml @@ -1311,6 +1311,8 @@ components: nullable: true CheckCreate: type: object + required: + - state properties: state: title: State diff --git a/docs/api/schemas/v1.1/patchwork.yaml b/docs/api/schemas/v1.1/patchwork.yaml index 4e81ac33d9b2..0c086edaa776 100644 --- a/docs/api/schemas/v1.1/patchwork.yaml +++ b/docs/api/schemas/v1.1/patchwork.yaml @@ -1316,6 +1316,8 @@ components: nullable: true CheckCreate: type: object + required: + - state properties: state: title: State diff --git a/patchwork/api/check.py b/patchwork/api/check.py index 4d2181d0a04b..2c3fe445872e 100644 --- a/patchwork/api/check.py +++ b/patchwork/api/check.py @@ -7,6 +7,7 @@ from django.http import Http404 from django.http.request import QueryDict from django.shortcuts import get_object_or_404 from rest_framework.exceptions import PermissionDenied +from rest_framework.exceptions import ValidationError from rest_framework.generics import ListCreateAPIView from rest_framework.generics import RetrieveAPIView from rest_framework.serializers import CurrentUserDefault @@ -36,6 +37,9 @@ class CheckSerializer(HyperlinkedModelSerializer): user = UserSerializer(default=CurrentUserDefault()) def run_validation(self, data): + if 'state' not in data: + raise ValidationError({'state': "A check must have a state."}) + for val, label in Check.STATE_CHOICES: if label != data['state']: continue diff --git a/patchwork/tests/api/test_check.py b/patchwork/tests/api/test_check.py index 1cfdff6e757b..24451aba09ad 100644 --- a/patchwork/tests/api/test_check.py +++ b/patchwork/tests/api/test_check.py @@ -151,6 +151,23 @@ class TestCheckAPI(utils.APITestCase): self.assertEqual(status.HTTP_400_BAD_REQUEST, resp.status_code) self.assertEqual(0, Check.objects.all().count()) + @utils.store_samples('check-create-error-missing-state') + def test_create_missing_state(self): + """Create a check using invalid values. + + Ensure we handle the state being absent. + """ + check = { + 'target_url': 'http://t.co', + 'description': 'description', + 'context': 'context', + } + + self.client.force_authenticate(user=self.user) + resp = self.client.post(self.api_url(), check) + self.assertEqual(status.HTTP_400_BAD_REQUEST, resp.status_code) + self.assertEqual(0, Check.objects.all().count()) + @utils.store_samples('check-create-error-not-found') def test_create_invalid_patch(self): """Ensure we handle non-existent patches."""