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 <step...@that.guru> Closes: #313 Reported-by: Bjorn Helgaas <helg...@kernel.org> --- 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. -- 2.21.0 _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
