This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3208

wireproto: disallow commands handlers for multiple transport versions
ClosedPublic

Authored by indygreg on Apr 9 2018, 2:58 PM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Commits
rHG3e5e37204b32: wireproto: disallow commands handlers for multiple transport versions
Summary

I think it will be more trouble than it is worth to code version 1
and version 2 command handlers to the same interface. It will feel
awkward to shoehorn functionality into e.g. the version 1 protocol
handler interface. This would likely constrain the ability for version
2 to evolve.

Previous commits introduced a clean separation between command handlers
for version 1 and version 2 transports. This commit reinforces that
separation by dropping support for having a single command handler
service both version 1 and version 2 transports.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

indygreg created this revision.Apr 9 2018, 2:58 PM
durin42 accepted this revision.Apr 9 2018, 6:29 PM
This revision is now accepted and ready to land.Apr 9 2018, 6:29 PM
Closed by commit rHG3e5e37204b32: wireproto: disallow commands handlers for multiple transport versions (authored by indygreg). · Explain WhyApr 11 2018, 12:35 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/wireproto.py (43 lines)

Diff 7987

mercurial/wireproto.py

if proto.name not in entry.transports: if proto.name not in entry.transports:
return False return False
return True return True
# Constants specifying which transports a wire protocol command should be # Constants specifying which transports a wire protocol command should be
# available on. For use with @wireprotocommand. # available on. For use with @wireprotocommand.
POLICY_ALL = 'all'
POLICY_V1_ONLY = 'v1-only' POLICY_V1_ONLY = 'v1-only'
POLICY_V2_ONLY = 'v2-only' POLICY_V2_ONLY = 'v2-only'
# For version 1 transports. # For version 1 transports.
commands = commanddict() commands = commanddict()
# For version 2 transports. # For version 2 transports.
commandsv2 = commanddict() commandsv2 = commanddict()
def wireprotocommand(name, args='', transportpolicy=POLICY_V1_ONLY, def wireprotocommand(name, args=None, transportpolicy=POLICY_V1_ONLY,
permission='push'): permission='push'):
"""Decorator to declare a wire protocol command. """Decorator to declare a wire protocol command.
``name`` is the name of the wire protocol command being provided. ``name`` is the name of the wire protocol command being provided.
``args`` defines the named arguments accepted by the command. It is ``args`` defines the named arguments accepted by the command. It is
ideally a dict mapping argument names to their types. For backwards ideally a dict mapping argument names to their types. For backwards
compatibility, it can be a space-delimited list of argument names. For compatibility, it can be a space-delimited list of argument names. For
version 1 transports, ``*`` denotes a special value that says to accept version 1 transports, ``*`` denotes a special value that says to accept
all named arguments. all named arguments.
``transportpolicy`` is a POLICY_* constant denoting which transports ``transportpolicy`` is a POLICY_* constant denoting which transports
this wire protocol command should be exposed to. By default, commands this wire protocol command should be exposed to. By default, commands
are exposed to all wire protocol transports. are exposed to all wire protocol transports.
``permission`` defines the permission type needed to run this command. ``permission`` defines the permission type needed to run this command.
Can be ``push`` or ``pull``. These roughly map to read-write and read-only, Can be ``push`` or ``pull``. These roughly map to read-write and read-only,
respectively. Default is to assume command requires ``push`` permissions respectively. Default is to assume command requires ``push`` permissions
because otherwise commands not declaring their permissions could modify because otherwise commands not declaring their permissions could modify
a repository that is supposed to be read-only. a repository that is supposed to be read-only.
""" """
if transportpolicy == POLICY_ALL: if transportpolicy == POLICY_V1_ONLY:
transports = set(wireprototypes.TRANSPORTS)
transportversions = {1, 2}
elif transportpolicy == POLICY_V1_ONLY:
transports = {k for k, v in wireprototypes.TRANSPORTS.items() transports = {k for k, v in wireprototypes.TRANSPORTS.items()
if v['version'] == 1} if v['version'] == 1}
transportversions = {1} transportversion = 1
elif transportpolicy == POLICY_V2_ONLY: elif transportpolicy == POLICY_V2_ONLY:
transports = {k for k, v in wireprototypes.TRANSPORTS.items() transports = {k for k, v in wireprototypes.TRANSPORTS.items()
if v['version'] == 2} if v['version'] == 2}
transportversions = {2} transportversion = 2
else: else:
raise error.ProgrammingError('invalid transport policy value: %s' % raise error.ProgrammingError('invalid transport policy value: %s' %
transportpolicy) transportpolicy)
# Because SSHv2 is a mirror of SSHv1, we allow "batch" commands through to # Because SSHv2 is a mirror of SSHv1, we allow "batch" commands through to
# SSHv2. # SSHv2.
# TODO undo this hack when SSH is using the unified frame protocol. # TODO undo this hack when SSH is using the unified frame protocol.
if name == b'batch': if name == b'batch':
transports.add(wireprototypes.SSHV2) transports.add(wireprototypes.SSHV2)
if permission not in ('push', 'pull'): if permission not in ('push', 'pull'):
raise error.ProgrammingError('invalid wire protocol permission; ' raise error.ProgrammingError('invalid wire protocol permission; '
'got %s; expected "push" or "pull"' % 'got %s; expected "push" or "pull"' %
permission) permission)
if 1 in transportversions and not isinstance(args, bytes): if transportversion == 1:
raise error.ProgrammingError('arguments for version 1 commands must ' if args is None:
'be declared as bytes') args = ''
if isinstance(args, bytes): if not isinstance(args, bytes):
dictargs = {arg: b'legacy' for arg in args.split()} raise error.ProgrammingError('arguments for version 1 commands '
elif isinstance(args, dict): 'must be declared as bytes')
dictargs = args elif transportversion == 2:
else: if args is None:
raise ValueError('args must be bytes or a dict') args = {}
if not isinstance(args, dict):
raise error.ProgrammingError('arguments for version 2 commands '
'must be declared as dicts')
def register(func): def register(func):
if 1 in transportversions: if transportversion == 1:
if name in commands: if name in commands:
raise error.ProgrammingError('%s command already registered ' raise error.ProgrammingError('%s command already registered '
'for version 1' % name) 'for version 1' % name)
commands[name] = commandentry(func, args=args, commands[name] = commandentry(func, args=args,
transports=transports, transports=transports,
permission=permission) permission=permission)
if 2 in transportversions: elif transportversion == 2:
if name in commandsv2: if name in commandsv2:
raise error.ProgrammingError('%s command already registered ' raise error.ProgrammingError('%s command already registered '
'for version 2' % name) 'for version 2' % name)
commandsv2[name] = commandentry(func, args=dictargs, commandsv2[name] = commandentry(func, args=args,
transports=transports, transports=transports,
permission=permission) permission=permission)
else:
raise error.ProgrammingError('unhandled transport version: %d' %
transportversion)
return func return func
return register return register
# TODO define a more appropriate permissions type to use for this. # TODO define a more appropriate permissions type to use for this.
@wireprotocommand('batch', 'cmds *', permission='pull', @wireprotocommand('batch', 'cmds *', permission='pull',
transportpolicy=POLICY_V1_ONLY) transportpolicy=POLICY_V1_ONLY)
def batch(repo, proto, cmds, others): def batch(repo, proto, cmds, others):