[PATCH 3/3] REST: Allow a project to restrict submitter state changes

[Stephen Finucane]

On Tue, 2021-08-03 at 01:27 +1000, Daniel Axtens wrote:
> As with xmlrpc and the UI.
> 
> Signed-off-by: Daniel Axtens <dja at axtens.net>
> ---
>  patchwork/api/patch.py            | 10 +++++
>  patchwork/tests/api/test_patch.py | 70 +++++++++++++++++++++++++++++++
>  2 files changed, 80 insertions(+)
> 
> diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py
> index 9d222754412e..b8d0d5e17749 100644
> --- a/patchwork/api/patch.py
> +++ b/patchwork/api/patch.py
> @@ -122,6 +122,16 @@ class PatchListSerializer(BaseHyperlinkedModelSerializer):
>                                    "'%s'" % (value, self.instance.project))
>          return value
>  
> +    def validate_state(self, value):
> +        """Check that the users is authorised to set this state."""
> +        user = self.context.get('request').user
> +        if not self.instance.can_set_state(user):
> +            raise ValidationError(
> +                "User '%s' is not permitted to set state '%s' on this patch." %
> +                (user, value.name))
> +
> +        return value
> +

Oh, I think both this and the existing 'validate_delegate' are in the wrong
class. 'PatchListSerializer' is only used by the 'PatchList' view, which is
read-only. These should probably both be 'PatchDetailSerializer'? Assuming I've
groked things correctly, could you move 'validate_delegate' in a precursor
patch?

>      def to_representation(self, instance):
>          # NOTE(stephenfin): This is here to ensure our API looks the same even
>          # after we changed the series-patch relationship from M:N to 1:N. It
> diff --git a/patchwork/tests/api/test_patch.py b/patchwork/tests/api/test_patch.py
> index da2dd6e9084b..9de7b0d105f4 100644
> --- a/patchwork/tests/api/test_patch.py
> +++ b/patchwork/tests/api/test_patch.py
> @@ -11,6 +11,9 @@ from django.conf import settings
>  from django.urls import reverse
>  
>  from patchwork.models import Patch
> +from patchwork.models import Person
> +from patchwork.models import Project
> +from patchwork.models import State
>  from patchwork.tests.api import utils
>  from patchwork.tests.utils import create_maintainer
>  from patchwork.tests.utils import create_patch
> @@ -409,3 +412,70 @@ class TestPatchAPI(utils.APITestCase):
>          self.client.force_authenticate(user=user)
>          resp = self.client.delete(self.api_url(patch.id))
>          self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
> +
> +
> + at unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
> +class TestPatchStateChecks(utils.APITestCase):
> +    fixtures = ['default_tags']
> +
> +    @staticmethod
> +    def api_url(item=None):
> +        kwargs = {'pk': item}
> +        return reverse('api-patch-detail', kwargs=kwargs)
> +
> +    def setUp(self):
> +        self.projects = {}
> +        self.maintainers = {}
> +        self.delegates = {}
> +        self.submitters = {}
> +        self.patches = {}
> +
> +        for project_type in (Project.SUBMITTER_NO_STATE_CHANGES,
> +                             Project.SUBMITTER_ALL_STATE_CHANGES):
> +            project = create_project(
> +                submitter_state_change_rules=project_type)
> +            self.projects[project_type] = project
> +            self.maintainers[project_type] = create_maintainer(project)
> +            submitter = create_user(project)
> +            self.submitters[project_type] = submitter
> +            delegate = create_user(project)
> +            self.delegates[project_type] = delegate
> +
> +            patch = create_patch(project=project,
> +                                 submitter=Person.objects.get(
> +                                     user=submitter),
> +                                 delegate=delegate)
> +            self.patches[project_type] = patch
> +
> +        create_state(name="New")
> +        create_state(name="RFC")
> +
> +    def can_set_state(self, patch, user):
> +        new_state = State.objects.get(name="New")
> +        rfc_state = State.objects.get(name="RFC")
> +        patch.state = new_state
> +        patch.save()
> +
> +        self.client.force_authenticate(user=user)
> +        resp = self.client.patch(self.api_url(patch.id),
> +                                 {'state': rfc_state.slug})
> +
> +        if resp.status_code != status.HTTP_200_OK:
> +            return False
> +
> +        self.assertEqual(Patch.objects.get(id=patch.id).state, rfc_state)
> +        return True
> +
> +    def test_allset(self):
> +        project = Project.SUBMITTER_ALL_STATE_CHANGES
> +        patch = self.patches[project]
> +        self.assertTrue(self.can_set_state(patch, self.maintainers[project]))
> +        self.assertTrue(self.can_set_state(patch, self.delegates[project]))
> +        self.assertTrue(self.can_set_state(patch, self.submitters[project]))
> +
> +    def test_noset(self):
> +        project = Project.SUBMITTER_NO_STATE_CHANGES
> +        patch = self.patches[project]
> +        self.assertTrue(self.can_set_state(patch, self.maintainers[project]))
> +        self.assertTrue(self.can_set_state(patch, self.delegates[project]))
> +        self.assertFalse(self.can_set_state(patch, self.submitters[project]))

Same question as the XML-RPC changes. I think a simple positive-negative test
would be more than sufficient here. Let's leave the full matrix to a simple unit
test of the model method.

Stephen