[PATCH 2/3] xmlrpc: Allow a project to restrict submitter state changes
Stephen Finucane
stephen at that.guru
Thu Aug 12 20:38:16 AEST 2021
On Fri, 2021-08-06 at 11:44 +1000, Daniel Axtens wrote:
> 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.
Not quite yet. It's still a relatively small cost to maintain and pwclient is
not yet migrated (very soon though!). I think we have everything we need in the
REST API though. It's just a case of migrating the remaining tooling and
notifying people that the thing is going in the next major version release.
>
> > > 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?
Possibly. I know that Django has a permissions framework but I've never worked
with it and don't know how useful it would be here. What I would ultimately like
to avoid is duplicating the same permissions'y checks across three different
views: the REST API, the XML-RPC API and the web UI. Even if we drop the XML-RPC
API, two sets of checks is still one too many. We might not be able to do this
at the model layer, since we don't necessary have sufficient context there, but
we should be able to unify it somewhere, right?
Stephen
>
> Kind regards,
> Daniel
>
> > Stephen
> >
> > > for (k, v) in params.items():
> > > if k not in ok_params:
> > > continue
More information about the Patchwork
mailing list