Stephen Finucane <step...@that.guru> writes: > On Tue, 2021-08-03 at 01:27 +1000, Daniel Axtens wrote: >> As with the UI. >> >> Signed-off-by: Daniel Axtens <d...@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']) >> >> >> +@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 _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork