[PATCH] Fix issue with delegation of patch via REST API

[Bjorn Helgaas]

On Sat, Sep 21, 2019 at 07:30:46PM +0100, Stephen Finucane wrote:
> 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.

I tried the instance at https://patchwork.kernel.org/project/linux-pci/list/
to see if it was new enough to work without this fix.  But it also
fails, slightly differently:

  $ git config -l | grep "^pw"
  pw.server=https://patchwork.kernel.org/api/1.1
  pw.project=linux-pci
  pw.token=...

  $ git-pw patch update --delegate helgaas 11151519
  More than one delegate found: helgaas

Is this another manifestation of the same bug or something else?

> [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 at that.guru>
> Closes: #313
> Reported-by: Bjorn Helgaas <helgaas at 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
>