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

[Daniel Axtens]

Stephen Finucane <stephen at that.guru> writes:

> On Tue, 2021-08-03 at 01:27 +1000, Daniel Axtens wrote:
>> As with the UI.
>> 
>> Signed-off-by: Daniel Axtens <dja at axtens.net>
>> ---
>>  patchwork/tests/test_xmlrpc.py | 90 ++++++++++++++++++++++++++++++++++
>>  patchwork/views/xmlrpc.py      |  4 ++
>>  2 files changed, 94 insertions(+)
>> 
>> diff --git a/patchwork/tests/test_xmlrpc.py b/patchwork/tests/test_xmlrpc.py
>> index 4726fdffa5d5..eea0b4eaf560 100644
>> --- a/patchwork/tests/test_xmlrpc.py
>> +++ b/patchwork/tests/test_xmlrpc.py
>> @@ -10,6 +10,9 @@ from django.conf import settings
>>  from django.test import LiveServerTestCase
>>  from django.urls import reverse
>>  
>> +from patchwork.models import Person
>> +from patchwork.models import Project
>> +from patchwork.models import State
>>  from patchwork.tests import utils
>>  
>>  
>> @@ -81,6 +84,93 @@ class XMLRPCAuthenticatedTest(LiveServerTestCase):
>>          self.assertTrue(result['archived'])
>>  
>>  
>> + at unittest.skipUnless(settings.ENABLE_XMLRPC,
>> +                     'requires xmlrpc interface (use the ENABLE_XMLRPC '
>> +                     'setting)')
>> +class XMLRPCStateSettingTest(LiveServerTestCase):
>> +
>> +    def url_for_user(self, user):
>> +        return ('http://%s:%s@' + self.url[7:]) % \
>> +               (user.username, user.username)
>> +
>> +    def setUp(self):
>> +        self.url = self.live_server_url + reverse('xmlrpc')
>> +        # url is of the form http://localhost:PORT/PATH
>> +        # strip the http and replace it with the username/passwd of a user.
>> +        self.projects = {}
>> +        self.maintainers = {}
>> +        self.delegates = {}
>> +        self.submitters = {}
>> +        self.patches = {}
>> +        self.rpcs = {}
>> +
>> +        for project_type in (Project.SUBMITTER_NO_STATE_CHANGES,
>> +                             Project.SUBMITTER_ALL_STATE_CHANGES):
>> +            project = utils.create_project(
>> +                submitter_state_change_rules=project_type)
>> +            self.projects[project_type] = project
>> +            self.maintainers[project_type] = utils.create_maintainer(project)
>> +            submitter = utils.create_user(project)
>> +            self.submitters[project_type] = submitter
>> +            delegate = utils.create_user(project)
>> +            self.delegates[project_type] = delegate
>> +
>> +            self.rpcs[project_type] = {
>> +                'maintainer': ServerProxy(self.url_for_user(
>> +                    self.maintainers[project_type])),
>> +                'delegate': ServerProxy(self.url_for_user(delegate)),
>> +                'submitter': ServerProxy(self.url_for_user(submitter)),
>> +            }
>> +
>> +            patch = utils.create_patch(project=project,
>> +                                       submitter=Person.objects.get(
>> +                                           user=submitter),
>> +                                       delegate=delegate)
>> +            self.patches[project_type] = patch
>> +
>> +        utils.create_state(name="New")
>> +        utils.create_state(name="RFC")
>> +
>> +    def tearDown(self):
>> +        for project_type in self.rpcs:
>> +            rpc_dict = self.rpcs[project_type]
>> +            for user in rpc_dict:
>> +                rpc_dict[user].close()
>> +
>> +    def can_set_state(self, patch, rpc):
>> +        new_state = State.objects.get(name="New")
>> +        rfc_state = State.objects.get(name="RFC")
>> +        patch.state = new_state
>> +        patch.save()
>> +
>> +        result = rpc.patch_get(patch.id)
>> +        self.assertEqual(result['state_id'], new_state.id)
>> +
>> +        try:
>> +            rpc.patch_set(patch.id, {'state': rfc_state.id})
>> +        except xmlrpc_client.Fault:
>> +            return False
>> +
>> +        # reload the patch
>> +        result = rpc.patch_get(patch.id)
>> +        self.assertEqual(result['state_id'], rfc_state.id)
>> +        return True
>> +
>> +    def test_allset(self):
>> +        rpc_dict = self.rpcs[Project.SUBMITTER_ALL_STATE_CHANGES]
>> +        patch = self.patches[Project.SUBMITTER_ALL_STATE_CHANGES]
>> +        self.assertTrue(self.can_set_state(patch, rpc_dict['maintainer']))
>> +        self.assertTrue(self.can_set_state(patch, rpc_dict['delegate']))
>> +        self.assertTrue(self.can_set_state(patch, rpc_dict['submitter']))
>> +
>> +    def test_noset(self):
>> +        rpc_dict = self.rpcs[Project.SUBMITTER_NO_STATE_CHANGES]
>> +        patch = self.patches[Project.SUBMITTER_NO_STATE_CHANGES]
>> +        self.assertTrue(self.can_set_state(patch, rpc_dict['maintainer']))
>> +        self.assertTrue(self.can_set_state(patch, rpc_dict['delegate']))
>> +        self.assertFalse(self.can_set_state(patch, rpc_dict['submitter']))
>> +
>> +
>
> Do we need this level of validation at this layer? LiveServerTests are expensive
> to setup. Rather than doing this, could we simply have a positive and negative
> test case (ideally folded into 'XMLRPCPatchTest', but that might be easier said
> than done) and then rely on unit tests for 'can_set_state' to ensure we have a
> full test matrix?
>

Perhaps. Can we just kill all of xmlrpc yet? I can't remember if there
are any features we haven't got in REST yet.

>>  class XMLRPCModelTestMixin(object):
>>  
>>      def create_multiple(self, count):
>> diff --git a/patchwork/views/xmlrpc.py b/patchwork/views/xmlrpc.py
>> index 6701bf20f386..d73cfa7a8441 100644
>> --- a/patchwork/views/xmlrpc.py
>> +++ b/patchwork/views/xmlrpc.py
>> @@ -713,6 +713,10 @@ def patch_set(user, patch_id, params):
>>      if not patch.is_editable(user):
>>          raise Exception('No permissions to edit this patch')
>>  
>> +    if 'state' in params:
>> +        if not patch.can_set_state(user):
>> +            raise Exception('No permissions to set patch state')
>> +
>
> [Thinking out loud] I wonder if the additional verbosity provided by having a
> separate check for this one validation is worth the additional complexity in
> code (~90 lines of code and tests in this patch alone). Perhaps rather than
> having a separate check, 'is_editable' could raise a (custom) exception with the
> additional details about what validation, if any, failed, rather than returning
> a simple boolean. 'is_editable' isn't a Django-provided thing fwict, so we don't
> have a particular contract we need to adhere to. wdyt?
>

So. Depends. is_editable() doesn't know _what_ you're trying to edit
yet. Also, as I alluded to earlier, I'm interested in the idea of
extending the state restrictions to be a bit smarter, so at some point
can_check_state might need to know what state you are trying to change to.

It is worth thinking some more about this though, because I also want to
do a similar thing with delegates: basically allowing a project to lock
down setting the delegate to the autodelegation rules and
maintainers. If we can figure out a way to do this with the existing
editing checks, great, or maybe we need to finally bite the bullet and
do proper django-y permission checks, or maybe we want to kill
is_editable in favour of more specific methods for specific fields?

Kind regards,
Daniel

> Stephen
>
>>      for (k, v) in params.items():
>>          if k not in ok_params:
>>              continue