Mercurial > hg
changeset 36801:66de4555cefd
wireproto: formalize permissions checking as part of protocol interface
Per the inline comment desiring to formalize permissions checking
in the protocol interface, we do that.
I'm not convinced this is the best way to go about things. I would love
for there to e.g. be a better exception for denoting permissions
problems. But it does feel strictly better than snipping attributes
on the proto instance.
Differential Revision: https://phab.mercurial-scm.org/D2719
| author | Gregory Szorc <gregory.szorc@gmail.com> |
|---|---|
| date | Wed, 07 Mar 2018 16:18:52 -0800 |
| parents | 0b18604db95e |
| children | 7fc80c982656 |
| files | mercurial/hgweb/hgweb_mod.py mercurial/wireproto.py mercurial/wireprotoserver.py mercurial/wireprototypes.py tests/test-wireproto.py |
| diffstat | 5 files changed, 35 insertions(+), 20 deletions(-) [+] |
line wrap: on
line diff
--- a/mercurial/hgweb/hgweb_mod.py Wed Mar 07 16:02:24 2018 -0800 +++ b/mercurial/hgweb/hgweb_mod.py Wed Mar 07 16:18:52 2018 -0800 @@ -350,18 +350,15 @@ # Route it to a wire protocol handler if it looks like a wire protocol # request. - protohandler = wireprotoserver.parsehttprequest(rctx.repo, req, query) + protohandler = wireprotoserver.parsehttprequest(rctx, req, query, + self.check_perm) if protohandler: try: if query: raise ErrorResponse(HTTP_NOT_FOUND) - # TODO fold this into parsehttprequest - checkperm = lambda op: self.check_perm(rctx, req, op) - protohandler['proto'].checkperm = checkperm - - return protohandler['dispatch'](checkperm) + return protohandler['dispatch']() except ErrorResponse as inst: return protohandler['handleerror'](inst)
--- a/mercurial/wireproto.py Wed Mar 07 16:02:24 2018 -0800 +++ b/mercurial/wireproto.py Wed Mar 07 16:18:52 2018 -0800 @@ -731,13 +731,10 @@ vals[unescapearg(n)] = unescapearg(v) func, spec = commands[op] - # If the protocol supports permissions checking, perform that - # checking on each batched command. - # TODO formalize permission checking as part of protocol interface. - if util.safehasattr(proto, 'checkperm'): - perm = commands[op].permission - assert perm in ('push', 'pull') - proto.checkperm(perm) + # Validate that client has permissions to perform this command. + perm = commands[op].permission + assert perm in ('push', 'pull') + proto.checkperm(perm) if spec: keys = spec.split()
--- a/mercurial/wireprotoserver.py Wed Mar 07 16:02:24 2018 -0800 +++ b/mercurial/wireprotoserver.py Wed Mar 07 16:18:52 2018 -0800 @@ -54,9 +54,10 @@ return ''.join(chunks) class httpv1protocolhandler(wireprototypes.baseprotocolhandler): - def __init__(self, req, ui): + def __init__(self, req, ui, checkperm): self._req = req self._ui = ui + self._checkperm = checkperm @property def name(self): @@ -139,6 +140,9 @@ return caps + def checkperm(self, perm): + return self._checkperm(perm) + # This method exists mostly so that extensions like remotefilelog can # disable a kludgey legacy method only over http. As of early 2018, # there are no other known users, so with any luck we can discard this @@ -146,7 +150,7 @@ def iscmd(cmd): return cmd in wireproto.commands -def parsehttprequest(repo, req, query): +def parsehttprequest(rctx, req, query, checkperm): """Parse the HTTP request for a wire protocol request. If the current request appears to be a wire protocol request, this @@ -156,6 +160,8 @@ ``req`` is a ``wsgirequest`` instance. """ + repo = rctx.repo + # HTTP version 1 wire protocol requests are denoted by a "cmd" query # string parameter. If it isn't present, this isn't a wire protocol # request. @@ -174,13 +180,13 @@ if not iscmd(cmd): return None - proto = httpv1protocolhandler(req, repo.ui) + proto = httpv1protocolhandler(req, repo.ui, + lambda perm: checkperm(rctx, req, perm)) return { 'cmd': cmd, 'proto': proto, - 'dispatch': lambda checkperm: _callhttp(repo, req, proto, cmd, - checkperm), + 'dispatch': lambda: _callhttp(repo, req, proto, cmd), 'handleerror': lambda ex: _handlehttperror(ex, req, cmd), } @@ -224,7 +230,7 @@ opts = {'level': ui.configint('server', 'zliblevel')} return HGTYPE, util.compengines['zlib'], opts -def _callhttp(repo, req, proto, cmd, checkperm): +def _callhttp(repo, req, proto, cmd): def genversion2(gen, engine, engineopts): # application/mercurial-0.2 always sends a payload header # identifying the compression engine. @@ -242,7 +248,7 @@ 'over HTTP')) return [] - checkperm(wireproto.commands[cmd].permission) + proto.checkperm(wireproto.commands[cmd].permission) rsp = wireproto.dispatch(repo, proto, cmd) @@ -392,6 +398,9 @@ def addcapabilities(self, repo, caps): return caps + def checkperm(self, perm): + pass + class sshv2protocolhandler(sshv1protocolhandler): """Protocol handler for version 2 of the SSH protocol."""
--- a/mercurial/wireprototypes.py Wed Mar 07 16:02:24 2018 -0800 +++ b/mercurial/wireprototypes.py Wed Mar 07 16:18:52 2018 -0800 @@ -146,3 +146,12 @@ Returns a list of capabilities. The passed in argument can be returned. """ + + @abc.abstractmethod + def checkperm(self, perm): + """Validate that the client has permissions to perform a request. + + The argument is the permission required to proceed. If the client + doesn't have that permission, the exception should raise or abort + in a protocol specific manner. + """
--- a/tests/test-wireproto.py Wed Mar 07 16:02:24 2018 -0800 +++ b/tests/test-wireproto.py Wed Mar 07 16:18:52 2018 -0800 @@ -18,6 +18,9 @@ names = spec.split() return [args[n] for n in names] + def checkperm(self, perm): + pass + class clientpeer(wireproto.wirepeer): def __init__(self, serverrepo): self.serverrepo = serverrepo