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

[Stephen Finucane]

On Mon, 2019-09-23 at 15:39 -0500, Bjorn Helgaas wrote:
> 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?

This is a different issue and, unlike the other one, is more feature
than bug. This is happening because the search for a particular user is
returning multiple matches. We match on username, first name, last name
and email, so I imagine you have multiple user accounts on the instance
and there might be a conflict between an email address of one account
and a username of another? (Let me know if this isn't the case). The
easy solution is to use a more specific match. I'd suggest just using
the email address associated with your user account ([1] suggests this
is 'bhelgaas at google.com'). We could also support lookup by user ID
(which would guarantee a single match) but I haven't added that to git-
pw yet since it didn't seem that usable.

Stephen

[1] https://patchwork.kernel.org/project/linux-pci/

> > [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
> >