From patchwork Sat Sep 21 18:30:46 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stephen Finucane X-Patchwork-Id: 1165622 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 46bK0f2V3Hz9s7T for ; Sun, 22 Sep 2019 04:31:14 +1000 (AEST) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=that.guru Authentication-Results: ozlabs.org; dkim=fail reason="key not found in DNS" (0-bit key; unprotected) header.d=that.guru header.i=@that.guru header.b="MrbT6dQ6"; dkim-atps=neutral Received: from bilbo.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 46bK0f1czHzDqMv for ; Sun, 22 Sep 2019 04:31:14 +1000 (AEST) X-Original-To: patchwork@lists.ozlabs.org Delivered-To: patchwork@lists.ozlabs.org Authentication-Results: lists.ozlabs.org; spf=none (mailfrom) smtp.mailfrom=that.guru (client-ip=172.82.139.249; helo=qrelay249.mxroute.com; envelope-from=stephen@that.guru; receiver=) Authentication-Results: lists.ozlabs.org; dmarc=none (p=none dis=none) header.from=that.guru Authentication-Results: lists.ozlabs.org; dkim=fail reason="key not found in DNS" (0-bit key; unprotected) header.d=that.guru header.i=@that.guru header.b="MrbT6dQ6"; dkim-atps=neutral Received: from qrelay249.mxroute.com (qrelay249.mxroute.com [172.82.139.249]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 46bK0X1msszDqBx for ; Sun, 22 Sep 2019 04:31:07 +1000 (AEST) Received: from filter002.mxroute.com (unknown [94.130.183.33]) by qrelay249.mxroute.com (Postfix) with ESMTP id BE94E32103E; Sat, 21 Sep 2019 14:31:04 -0400 (EDT) Received: from one.mxroute.com (one.mxroute.com [195.201.59.211]) by filter002.mxroute.com (Postfix) with ESMTPS id 18B223F042; Sat, 21 Sep 2019 18:30:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=that.guru; s=default; h=Content-Transfer-Encoding:MIME-Version:Message-Id:Date:Subject: Cc:To:From:Sender:Reply-To:Content-Type:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: In-Reply-To:References:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=Tvz956M5/SWO7EkWPzjymYpv+42wezhOYSzC/GtKd14=; b=MrbT6dQ6xzYHzcNvsusodNrwTb rZTWign0bArgsUgnsWQBVr2adyoXGeyRMFbBe1KHg0/DTBNvSLLIWcFQTkALpLHAE1Vr+YBDmlI3s PG3yNZiZbxjMQRF9QiZv/VinJy7FKROPWq0GuhtVt/ERHUDVwXdYg9qfu2L4N4RKIuAbpGnWOiZks CUrNueXUE5pRe5Q3LAJNCZo24yR0UrlClWqhW4SX7PFVBy4A3Wj5cfleHa77C2AJs3lzxvDREq5nu 7sLfNhq/+coiXzEd8I1elM4gcZmpzYBWWOmJqeZSs2MFULTosXdI/B9kzyennNRMVdd4wN+3jPilu y00Xra8A==; From: Stephen Finucane To: patchwork@lists.ozlabs.org Subject: [PATCH] Fix issue with delegation of patch via REST API Date: Sat, 21 Sep 2019 19:30:46 +0100 Message-Id: <20190921183046.27279-1-stephen@that.guru> X-Mailer: git-send-email 2.21.0 MIME-Version: 1.0 X-AuthUser: stephen@that.guru 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" There have been reports of people being unable to delegate patches to themselves, despite being a maintainer or the project to which the patch is associated. The issue is a result of how we do a check for whether the user is a maintainer of the patch's project [1]. This check is checking if a given 'User.id' is in the list of items referenced by 'Project.maintainer_project'. However, 'Project.maintainer_project' is a backref to 'UserProfile.maintainer_projects'. This means we're comparing 'User.id' and 'UserProfile.id'. Boo. This wasn't seen in testing since we've had a post-save callback [2] for some time that ensures we always create a 'UserProfile' object whenever we create a 'User' object. This also means we won't have an issue on deployments initially deployed after that post-save callback was added, a 'User' with id=N will always have a corresponding 'UserProfile' with id=N. However, that's not true for older deployments such as the ozlabs.org one. [1] https://github.com/getpatchwork/patchwork/blob/89c924f9bc/patchwork/api/patch.py#L108-L111 [2] https://github.com/getpatchwork/patchwork/blob/89c924f9bc/patchwork/models.py#L204-L210 Signed-off-by: Stephen Finucane Closes: #313 Reported-by: Bjorn Helgaas --- patchwork/api/patch.py | 4 ++-- patchwork/tests/api/test_patch.py | 24 ++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py index c9360308..d1c9904d 100644 --- a/patchwork/api/patch.py +++ b/patchwork/api/patch.py @@ -105,8 +105,8 @@ class PatchListSerializer(BaseHyperlinkedModelSerializer): if not value: return value - if not self.instance.project.maintainer_project.filter( - id=value.id).exists(): + if not value.profile.maintainer_projects.only('id').filter( + id=self.instance.project.id).exists(): raise ValidationError("User '%s' is not a maintainer for project " "'%s'" % (value, self.instance.project)) return value diff --git a/patchwork/tests/api/test_patch.py b/patchwork/tests/api/test_patch.py index 82ae0184..edae9851 100644 --- a/patchwork/tests/api/test_patch.py +++ b/patchwork/tests/api/test_patch.py @@ -284,6 +284,30 @@ class TestPatchAPI(utils.APITestCase): self.assertContains(resp, 'Expected one of: %s.' % state.name, status_code=status.HTTP_400_BAD_REQUEST) + def test_update_legacy_delegate(self): + """Regression test for bug #313.""" + project = create_project() + state = create_state() + patch = create_patch(project=project, state=state) + user_a = create_maintainer(project) + + # create a user (User), then delete the associated UserProfile and save + # the user to ensure a new profile is generated + user_b = create_user() + self.assertEqual(user_b.id, user_b.profile.id) + user_b.profile.delete() + user_b.save() + user_b.profile.maintainer_projects.add(project) + user_b.profile.save() + self.assertNotEqual(user_b.id, user_b.profile.id) + + self.client.force_authenticate(user=user_a) + resp = self.client.patch(self.api_url(patch.id), + {'delegate': user_b.id}) + self.assertEqual(status.HTTP_200_OK, resp.status_code, resp) + self.assertEqual(Patch.objects.get(id=patch.id).state, state) + self.assertEqual(Patch.objects.get(id=patch.id).delegate, user_b) + def test_update_invalid_delegate(self): """Update patch with invalid fields.