Fix issue with delegation of patch via REST API

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 <stephen@that.guru>
Closes: #313
Reported-by: Bjorn Helgaas <helgaas@kernel.org>

2 files changed, 26 insertions, 2 deletions

diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py
index c936030..d1c9904 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 82ae018..edae985 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.