D2718: wireproto: declare permissions requirements in @wireprotocommand (API)

[indygreg (Gregory Szorc)]

indygreg added inline comments.

INLINE COMMENTS

> yuja wrote in wireproto.py:711
> Perhaps ProgrammingError is preferred here because it shouldn't
> be a user-facing error.

I agree. I'll send a follow-up (unless we want to amend hg-committed).

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2718

To: indygreg, #hg-reviewers, durin42
Cc: yuja, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D2718: wireproto: declare permissions requirements in @wireprotocommand (API)

[yuja (Yuya Nishihara)]

yuja added inline comments.

INLINE COMMENTS

> wireproto.py:711
> +if permission not in ('push', 'pull'):
> +raise error.Abort(_('invalid wire protocol permission; got %s; '
> +'expected "push" or "pull"') % permission)

Perhaps ProgrammingError is preferred here because it shouldn't
be a user-facing error.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2718

To: indygreg, #hg-reviewers, durin42
Cc: yuja, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D2718: wireproto: declare permissions requirements in @wireprotocommand (API)

[indygreg (Gregory Szorc)]

This revision was automatically updated to reflect the committed changes.
Closed by commit rHG0b18604db95e: wireproto: declare permissions requirements 
in @wireprotocommand (API) (authored by indygreg, committed by ).

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D2718?vs=6712=6772

REVISION DETAIL
  https://phab.mercurial-scm.org/D2718

AFFECTED FILES
  hgext/largefiles/uisetup.py
  mercurial/hgweb/hgweb_mod.py
  mercurial/wireproto.py
  mercurial/wireprotoserver.py
  tests/test-http-permissions.t

CHANGE DETAILS

diff --git a/tests/test-http-permissions.t b/tests/test-http-permissions.t
--- a/tests/test-http-permissions.t
+++ b/tests/test-http-permissions.t
@@ -21,12 +21,10 @@
   > @wireproto.wireprotocommand('customwritenoperm')
   > def customwritenoperm(repo, proto):
   > return b'write command no defined permissions\n'
-  > wireproto.permissions['customreadwithperm'] = 'pull'
-  > @wireproto.wireprotocommand('customreadwithperm')
+  > @wireproto.wireprotocommand('customreadwithperm', permission='pull')
   > def customreadwithperm(repo, proto):
   > return b'read-only command w/ defined permissions\n'
-  > wireproto.permissions['customwritewithperm'] = 'push'
-  > @wireproto.wireprotocommand('customwritewithperm')
+  > @wireproto.wireprotocommand('customwritewithperm', permission='push')
   > def customwritewithperm(repo, proto):
   > return b'write command w/ defined permissions\n'
   > EOF
diff --git a/mercurial/wireprotoserver.py b/mercurial/wireprotoserver.py
--- a/mercurial/wireprotoserver.py
+++ b/mercurial/wireprotoserver.py
@@ -242,11 +242,7 @@
'over HTTP'))
 return []
 
-# Assume commands with no defined permissions are writes /
-# for pushes. This is the safest from a security perspective
-# because it doesn't allow commands with undefined semantics
-# from bypassing permissions checks.
-checkperm(wireproto.permissions.get(cmd, 'push'))
+checkperm(wireproto.commands[cmd].permission)
 
 rsp = wireproto.dispatch(repo, proto, cmd)
 
diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
--- a/mercurial/wireproto.py
+++ b/mercurial/wireproto.py
@@ -592,10 +592,12 @@
 
 class commandentry(object):
 """Represents a declared wire protocol command."""
-def __init__(self, func, args='', transports=None):
+def __init__(self, func, args='', transports=None,
+ permission='push'):
 self.func = func
 self.args = args
 self.transports = transports or set()
+self.permission = permission
 
 def _merge(self, func, args):
 """Merge this instance with an incoming 2-tuple.
@@ -605,7 +607,8 @@
 data not captured by the 2-tuple and a new instance containing
 the union of the two objects is returned.
 """
-return commandentry(func, args=args, transports=set(self.transports))
+return commandentry(func, args=args, transports=set(self.transports),
+permission=self.permission)
 
 # Old code treats instances as 2-tuples. So expose that interface.
 def __iter__(self):
@@ -643,7 +646,8 @@
 else:
 # Use default values from @wireprotocommand.
 v = commandentry(v[0], args=v[1],
- transports=set(wireprototypes.TRANSPORTS))
+ transports=set(wireprototypes.TRANSPORTS),
+ permission='push')
 else:
 raise ValueError('command entries must be commandentry instances '
  'or 2-tuples')
@@ -672,12 +676,8 @@
 
 commands = commanddict()
 
-# Maps wire protocol name to operation type. This is used for permissions
-# checking. All defined @wireiprotocommand should have an entry in this
-# dict.
-permissions = {}
-
-def wireprotocommand(name, args='', transportpolicy=POLICY_ALL):
+def wireprotocommand(name, args='', transportpolicy=POLICY_ALL,
+ permission='push'):
 """Decorator to declare a wire protocol command.
 
 ``name`` is the name of the wire protocol command being provided.
@@ -688,6 +688,12 @@
 ``transportpolicy`` is a POLICY_* constant denoting which transports
 this wire protocol command should be exposed to. By default, commands
 are exposed to all wire protocol transports.
+
+``permission`` defines the permission type needed to run this command.
+Can be ``push`` or ``pull``. These roughly map to read-write and read-only,
+respectively. Default is to assume command requires ``push`` permissions
+because otherwise commands not declaring their permissions could modify
+a repository that is supposed to be read-only.
 """
 if transportpolicy == POLICY_ALL:
 transports = set(wireprototypes.TRANSPORTS)
@@ -701,14 +707,18 @@
 raise error.Abort(_('invalid transport policy value: %s') %
   

D2718: wireproto: declare permissions requirements in @wireprotocommand (API)

[indygreg (Gregory Szorc)]

indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  With the security patches from 4.5.2 merged into default, we now
  have a per-command attribute defining what permissions are needed
  to run that command. We now have a richer @wireprotocommand that
  can be extended to record additional command metadata. So we
  port the permissions mechanism to be based on @wireprotocommand.
  
  .. api::
  
hgweb_mod.perms and wireproto.permissions have been removed. Wire
protocol commands should declare their required permissions in the
@wireprotocommand decorator.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2718

AFFECTED FILES
  hgext/largefiles/uisetup.py
  mercurial/hgweb/hgweb_mod.py
  mercurial/wireproto.py
  mercurial/wireprotoserver.py
  tests/test-http-permissions.t

CHANGE DETAILS

diff --git a/tests/test-http-permissions.t b/tests/test-http-permissions.t
--- a/tests/test-http-permissions.t
+++ b/tests/test-http-permissions.t
@@ -21,12 +21,10 @@
   > @wireproto.wireprotocommand('customwritenoperm')
   > def customwritenoperm(repo, proto):
   > return b'write command no defined permissions\n'
-  > wireproto.permissions['customreadwithperm'] = 'pull'
-  > @wireproto.wireprotocommand('customreadwithperm')
+  > @wireproto.wireprotocommand('customreadwithperm', permission='pull')
   > def customreadwithperm(repo, proto):
   > return b'read-only command w/ defined permissions\n'
-  > wireproto.permissions['customwritewithperm'] = 'push'
-  > @wireproto.wireprotocommand('customwritewithperm')
+  > @wireproto.wireprotocommand('customwritewithperm', permission='push')
   > def customwritewithperm(repo, proto):
   > return b'write command w/ defined permissions\n'
   > EOF
diff --git a/mercurial/wireprotoserver.py b/mercurial/wireprotoserver.py
--- a/mercurial/wireprotoserver.py
+++ b/mercurial/wireprotoserver.py
@@ -242,11 +242,7 @@
'over HTTP'))
 return []
 
-# Assume commands with no defined permissions are writes /
-# for pushes. This is the safest from a security perspective
-# because it doesn't allow commands with undefined semantics
-# from bypassing permissions checks.
-checkperm(wireproto.permissions.get(cmd, 'push'))
+checkperm(wireproto.commands[cmd].permission)
 
 rsp = wireproto.dispatch(repo, proto, cmd)
 
diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
--- a/mercurial/wireproto.py
+++ b/mercurial/wireproto.py
@@ -592,10 +592,12 @@
 
 class commandentry(object):
 """Represents a declared wire protocol command."""
-def __init__(self, func, args='', transports=None):
+def __init__(self, func, args='', transports=None,
+ permission='push'):
 self.func = func
 self.args = args
 self.transports = transports or set()
+self.permission = permission
 
 def _merge(self, func, args):
 """Merge this instance with an incoming 2-tuple.
@@ -605,7 +607,8 @@
 data not captured by the 2-tuple and a new instance containing
 the union of the two objects is returned.
 """
-return commandentry(func, args=args, transports=set(self.transports))
+return commandentry(func, args=args, transports=set(self.transports),
+permission=self.permission)
 
 # Old code treats instances as 2-tuples. So expose that interface.
 def __iter__(self):
@@ -643,7 +646,8 @@
 else:
 # Use default values from @wireprotocommand.
 v = commandentry(v[0], args=v[1],
- transports=set(wireprototypes.TRANSPORTS))
+ transports=set(wireprototypes.TRANSPORTS),
+ permission='push')
 else:
 raise ValueError('command entries must be commandentry instances '
  'or 2-tuples')
@@ -672,12 +676,8 @@
 
 commands = commanddict()
 
-# Maps wire protocol name to operation type. This is used for permissions
-# checking. All defined @wireiprotocommand should have an entry in this
-# dict.
-permissions = {}
-
-def wireprotocommand(name, args='', transportpolicy=POLICY_ALL):
+def wireprotocommand(name, args='', transportpolicy=POLICY_ALL,
+ permission='push'):
 """Decorator to declare a wire protocol command.
 
 ``name`` is the name of the wire protocol command being provided.
@@ -688,6 +688,12 @@
 ``transportpolicy`` is a POLICY_* constant denoting which transports
 this wire protocol command should be exposed to. By default, commands
 are exposed to all wire protocol transports.
+
+``permission`` defines the permission type needed to run this command.
+Can be ``push`` or ``pull``. These roughly map to read-write and read-only,
+respectively. Default is to assume command