[PATCH 2/3] xmlrpc: Allow a project to restrict submitter state changes
Stephen Finucane
stephen at that.guru
Wed Aug 4 01:27:17 AEST 2021
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?
> 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?
Stephen
> for (k, v) in params.items():
> if k not in ok_params:
> continue
More information about the Patchwork
mailing list