launchpad-reviewers team mailing list archive

[Jelmer Vernooij <jelmer@xxxxxxxxx>]

Jelmer Vernooij has proposed merging lp:~jelmer/launchpad/custom-ssh-commands into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jelmer/launchpad/custom-ssh-commands/+merge/85339

Refactor the code hosting SSH session to make it easier to support running multiple commands.

Rather than passing in a single allowed_command and a matching command template into RestrictedExecOnlySession, it is now possible to pass in a method that takes the command the user specified and either raises a matching template, or raises ForbiddenCommand.
-- 
https://code.launchpad.net/~jelmer/launchpad/custom-ssh-commands/+merge/85339
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jelmer/launchpad/custom-ssh-commands into lp:launchpad.

=== modified file 'lib/lp/codehosting/sshserver/session.py'
--- lib/lp/codehosting/sshserver/session.py	2011-02-18 16:16:58 +0000
+++ lib/lp/codehosting/sshserver/session.py	2011-12-12 14:17:29 +0000
@@ -22,8 +22,7 @@
     interfaces,
     process,
     )
-from twisted.python import log, failure
-from twisted.internet.main import CONNECTION_LOST, CONNECTION_DONE
+from twisted.python import log
 
 from canonical.config import config
 from lp.codehosting import get_bzr_path
@@ -381,29 +380,27 @@
 
 
 class RestrictedExecOnlySession(ExecOnlySession):
-    """Conch session that only allows a single command to be executed."""
+    """Conch session that only allows specific commands to be executed."""
 
-    def __init__(self, avatar, reactor, allowed_command,
-                 executed_command_template, environment=None):
+    def __init__(self, avatar, reactor, lookup_command_template,
+            environment=None):
         """Construct a RestrictedExecOnlySession.
 
         :param avatar: See `ExecOnlySession`.
         :param reactor: See `ExecOnlySession`.
-        :param allowed_command: The sole command that can be executed.
-        :param executed_command_template: A Python format string for the actual
-            command that will be run. '%(user_id)s' will be replaced with the
-            'user_id' attribute of the current avatar.
+        :param lookup_command_template: Lookup the template for a command.
+            A template is a Python format string for the actual command that
+            will be run.  '%(user_id)s' will be replaced with the 'user_id'
+            attribute of the current avatar. Should raise
+            ForbiddenCommand if the command is not allowed.
         """
         ExecOnlySession.__init__(self, avatar, reactor, environment)
-        self.allowed_command = allowed_command
-        self.executed_command_template = executed_command_template
+        self.lookup_command_template = lookup_command_template
 
     @classmethod
-    def getAvatarAdapter(klass, allowed_command, executed_command_template,
-                         environment=None):
+    def getAvatarAdapter(klass, lookup_command_template, environment=None):
         from twisted.internet import reactor
-        return lambda avatar: klass(
-            avatar, reactor, allowed_command, executed_command_template,
+        return lambda avatar: klass(avatar, reactor, lookup_command_template,
             environment)
 
     def getCommandToRun(self, command):
@@ -411,10 +408,9 @@
 
         :raise ForbiddenCommand: when `command` is not the allowed command.
         """
-        if command != self.allowed_command:
-            raise ForbiddenCommand("Not allowed to execute %r." % (command,))
+        executed_command_template = self.lookup_command_template(command)
         return ExecOnlySession.getCommandToRun(
-            self, self.executed_command_template
+            self, executed_command_template
             % {'user_id': self.avatar.user_id})
 
 
@@ -448,23 +444,34 @@
                                       arguments, env, protocol)
 
 
-def launch_smart_server(avatar):
-    from twisted.internet import reactor
+def lookup_command_template(command):
+    """Map a command to a command template.
 
+    :param command: Command requested by the user
+    :return: Command template
+    :raise ForbiddenCommand: Raised when command isn't allowed
+    """
     python_command = "%(root)s/bin/py %(bzr)s" % {
             'root': config.root,
             'bzr': get_bzr_path(),
             }
     args = " lp-serve --inet %(user_id)s"
-    command = python_command + args
-    forking_command = "bzr" + args
+    command_template = python_command + args
+
+    if command == 'bzr serve --inet --directory=/ --allow-writes':
+        return command_template
+    # At the moment, only bzr branch serving is allowed.
+    raise ForbiddenCommand("Not allowed to execute %r." % (command,))
+
+
+def launch_smart_server(avatar):
+    from twisted.internet import reactor
 
     environment = dict(os.environ)
 
     # Extract the hostname from the supermirror root config.
     hostname = urlparse.urlparse(config.codehosting.supermirror_root)[1]
     environment['BZR_EMAIL'] = '%s@%s' % (avatar.username, hostname)
-    klass = RestrictedExecOnlySession
     # TODO: Use a FeatureFlag to enable this in a more fine-grained approach.
     #       If the forking daemon has been spawned, then we can use it if the
     #       feature is set to true for the given user, etc.
@@ -474,9 +481,8 @@
     #       forking daemon.
     if config.codehosting.use_forking_daemon:
         klass = ForkingRestrictedExecOnlySession
-    return klass(
-        avatar,
-        reactor,
-        'bzr serve --inet --directory=/ --allow-writes',
-        command,
+    else:
+        klass = RestrictedExecOnlySession
+
+    return klass(avatar, reactor, lookup_command_template,
         environment=environment)

=== modified file 'lib/lp/codehosting/sshserver/tests/test_session.py'
--- lib/lp/codehosting/sshserver/tests/test_session.py	2010-10-30 23:27:50 +0000
+++ lib/lp/codehosting/sshserver/tests/test_session.py	2011-12-12 14:17:29 +0000
@@ -24,6 +24,7 @@
     ForkingRestrictedExecOnlySession,
     RestrictedExecOnlySession,
     _WaitForExit,
+    lookup_command_template,
     )
 from lp.codehosting.tests.helpers import AvatarTestCase
 from lp.testing import TestCase
@@ -290,8 +291,12 @@
         AvatarTestCase.setUp(self)
         self.avatar = CodehostingAvatar(self.aliceUserDict, None)
         self.reactor = MockReactor()
+        def lookup_template(command):
+            if command == 'foo':
+                return 'bar baz %(user_id)s'
+            raise ForbiddenCommand("Not allowed to execute %r." % command)
         self.session = RestrictedExecOnlySession(
-            self.avatar, self.reactor, 'foo', 'bar baz %(user_id)s')
+            self.avatar, self.reactor, lookup_template)
 
     def test_makeRestrictedExecOnlySession(self):
         # A RestrictedExecOnlySession is constructed with an avatar, a reactor
@@ -302,9 +307,10 @@
             % (self.session,))
         self.assertEqual(self.avatar, self.session.avatar)
         self.assertEqual(self.reactor, self.session.reactor)
-        self.assertEqual('foo', self.session.allowed_command)
         self.assertEqual('bar baz %(user_id)s',
-                         self.session.executed_command_template)
+                         self.session.lookup_command_template('foo'))
+        self.assertRaises(ForbiddenCommand,
+            self.session.lookup_command_template, 'notfoo')
 
     def test_execCommandRejectsUnauthorizedCommands(self):
         # execCommand rejects all commands except for the command specified in
@@ -336,8 +342,12 @@
         # RestrictedExecOnlySession can be easily registered as an adapter for
         # Conch avatars.
         from twisted.internet import reactor
+        def lookup_template(command):
+            if command == 'foo':
+                return 'bar baz'
+            raise ForbiddenCommand(command)
         adapter = RestrictedExecOnlySession.getAvatarAdapter(
-            allowed_command='foo', executed_command_template='bar baz')
+            lookup_template)
         session = adapter(self.avatar)
         self.failUnless(
             isinstance(session, RestrictedExecOnlySession),
@@ -345,8 +355,10 @@
             "Got %r instead." % (session,))
         self.assertIs(self.avatar, session.avatar)
         self.assertIs(reactor, session.reactor)
-        self.assertEqual('foo', session.allowed_command)
-        self.assertEqual('bar baz', session.executed_command_template)
+        self.assertEqual('bar baz',
+                         session.lookup_command_template('foo'))
+        self.assertRaises(ForbiddenCommand,
+            session.lookup_command_template, 'notfoo')
 
 
 class TestSessionIntegration(AvatarTestCase):
@@ -411,3 +423,16 @@
              ['bzr', 'lp-serve',
               '--inet', str(self.avatar.user_id)],
              list(arguments))
+
+
+class TestLookupCommand(TestCase):
+
+    def test_other(self):
+        self.assertRaises(ForbiddenCommand, lookup_command_template, 'foo')
+
+    def test_bzr(self):
+        self.assertEquals(
+            config.root + '/bin/py ' + get_bzr_path() +
+            ' lp-serve --inet %(user_id)s',
+            lookup_command_template(
+                'bzr serve --inet --directory=/ --allow-writes'))