Merge lp:~jml/launchpad/poppy-cleanup into lp:launchpad

[Jonathan Lange]

This branch fixes bug 598532 and a related, unreported bug about poppy giving an ugly error when you try to execute a command on the server (e.g. "ssh ppa.launchpad.net ls")

It does so by providing a "session" adapter for poppy that provides no services itself. The session adapter is a user's view of the SSH server (minus any apps like sftp). For poppy, we don't want shells, terminals or executed processes, so a "do nothing" adapter is the right fit.

The code was written by stealing the relevant bits from the codehosting session adapter, which does pratically nothing.

[Brad Crittenden (bac)]

Nice branch Jono

review: Approve (code)

=== modified file 'daemons/poppy-sftp.tac'
--- daemons/poppy-sftp.tac	2010-05-27 07:07:24 +0000
+++ daemons/poppy-sftp.tac	2010-06-25 17:21:32 +0000
@@ -8,6 +8,7 @@
 import os
 
 from twisted.application import service
+from twisted.conch.interfaces import ISession
 from twisted.conch.ssh import filetransfer
 from twisted.cred.portal import IRealm, Portal
 from twisted.python import components
@@ -22,6 +23,7 @@
 from lp.services.sshserver.auth import (
     LaunchpadAvatar, PublicKeyFromLaunchpadChecker)
 from lp.services.sshserver.service import SSHService
+from lp.services.sshserver.session import DoNothingSession
 
 # XXX: Rename this file to something that doesn't mention poppy. Talk to
 # bigjools.
@@ -79,6 +81,8 @@
 components.registerAdapter(
     poppy_sftp_adapter, LaunchpadAvatar, filetransfer.ISFTPServer)
 
+components.registerAdapter(DoNothingSession, LaunchpadAvatar, ISession)
+
 
 # Construct an Application that has the Poppy SSH server.
 application = service.Application('poppy-sftp')

=== modified file 'lib/lp/codehosting/sshserver/session.py'
--- lib/lp/codehosting/sshserver/session.py	2010-04-15 15:41:05 +0000
+++ lib/lp/codehosting/sshserver/session.py	2010-06-25 17:21:32 +0000
@@ -12,16 +12,14 @@
 import urlparse
 
 from zope.event import notify
-from zope.interface import implements
 
-from twisted.conch.interfaces import ISession
-from twisted.conch.ssh import connection
 from twisted.internet.process import ProcessExitedAlready
 from twisted.python import log
 
 from canonical.config import config
 from lp.codehosting import get_bzr_path
 from lp.services.sshserver.events import AvatarEvent
+from lp.services.sshserver.session import DoNothingSession
 
 
 class BazaarSSHStarted(AvatarEvent):
@@ -38,13 +36,11 @@
     """Raised when a session is asked to execute a forbidden command."""
 
 
-class ExecOnlySession:
+class ExecOnlySession(DoNothingSession):
     """Conch session that only allows executing commands."""
 
-    implements(ISession)
-
     def __init__(self, avatar, reactor, environment=None):
-        self.avatar = avatar
+        super(ExecOnlySession, self).__init__(avatar)
         self.reactor = reactor
         self.environment = environment
         self._transport = None
@@ -72,11 +68,6 @@
         if self._transport is not None:
             self._transport.closeStdin()
 
-    def errorWithMessage(self, protocol, msg):
-        protocol.session.writeExtended(
-            connection.EXTENDED_DATA_STDERR, msg)
-        protocol.loseConnection()
-
     def execCommand(self, protocol, command):
         """Executes `command` using `protocol` as the ProcessProtocol.
 
@@ -115,19 +106,6 @@
         args = command.split()
         return args[0], args
 
-    def getPty(self, term, windowSize, modes):
-        """See ISession."""
-        # Do nothing, as we don't provide shell access. openShell will get
-        # called and handle this error message and disconnect.
-
-    def openShell(self, protocol):
-        """See ISession."""
-        self.errorWithMessage(protocol, "No shells on this server.\r\n")
-
-    def windowChanged(self, newWindowSize):
-        """See ISession."""
-        raise NotImplementedError()
-
 
 class RestrictedExecOnlySession(ExecOnlySession):
     """Conch session that only allows a single command to be executed."""

=== modified file 'lib/lp/services/sshserver/session.py'
--- lib/lp/services/sshserver/session.py	2010-04-15 15:32:16 +0000
+++ lib/lp/services/sshserver/session.py	2010-06-25 17:21:32 +0000
@@ -5,10 +5,14 @@
 
 __metaclass__ = type
 __all__ = [
+    'DoNothingSession',
     'PatchedSSHSession',
     ]
 
-from twisted.conch.ssh import channel, session
+from zope.interface import implements
+
+from twisted.conch.interfaces import ISession
+from twisted.conch.ssh import channel, connection, session
 
 
 class PatchedSSHSession(session.SSHSession, object):
@@ -77,3 +81,41 @@
             resumeProducing = getattr(transport, 'resumeProducing', None)
             if resumeProducing is not None:
                 resumeProducing()
+
+
+class DoNothingSession:
+    """A Conch user session that allows nothing."""
+
+    implements(ISession)
+
+    def __init__(self, avatar):
+        self.avatar = avatar
+
+    def closed(self):
+        """See ISession."""
+
+    def eofReceived(self):
+        """See ISession."""
+
+    def errorWithMessage(self, protocol, msg):
+        protocol.session.writeExtended(
+            connection.EXTENDED_DATA_STDERR, msg)
+        protocol.loseConnection()
+
+    def execCommand(self, protocol, command):
+        """See ISession."""
+        self.errorWithMessage(
+            protocol, "Not allowed to execute commands on this server.\r\n")
+
+    def getPty(self, term, windowSize, modes):
+        """See ISession."""
+        # Do nothing, as we don't provide shell access. openShell will get
+        # called and handle this error message and disconnect.
+
+    def openShell(self, protocol):
+        """See ISession."""
+        self.errorWithMessage(protocol, "No shells on this server.\r\n")
+
+    def windowChanged(self, newWindowSize):
+        """See ISession."""
+        raise NotImplementedError(self.windowChanged)

=== added file 'lib/lp/services/sshserver/tests/test_session.py'
--- lib/lp/services/sshserver/tests/test_session.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/sshserver/tests/test_session.py	2010-06-25 17:21:32 +0000
@@ -0,0 +1,105 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for generic SSH session support."""
+
+__metaclass__ = type
+
+import unittest
+
+from twisted.conch.interfaces import ISession
+from twisted.conch.ssh import connection
+
+from lp.services.sshserver.session import DoNothingSession
+from lp.testing import TestCase
+
+
+class MockSSHSession:
+    """Just enough of SSHSession to allow checking of reporting to stderr."""
+
+    def __init__(self, log):
+        self.log = log
+
+    def writeExtended(self, channel, data):
+        self.log.append(('writeExtended', channel, data))
+
+
+class MockProcessTransport:
+    """Mock transport used to fake speaking with child processes that are
+    mocked out in tests.
+    """
+
+    def __init__(self, executable):
+        self._executable = executable
+        self.log = []
+        self.session = MockSSHSession(self.log)
+
+    def closeStdin(self):
+        self.log.append(('closeStdin',))
+
+    def loseConnection(self):
+        self.log.append(('loseConnection',))
+
+    def signalProcess(self, signal):
+        self.log.append(('signalProcess', signal))
+
+    def write(self, data):
+        self.log.append(('write', data))
+
+
+class TestDoNothing(TestCase):
+    """Tests for ExecOnlySession."""
+
+    def setUp(self):
+        super(TestDoNothing, self).setUp()
+        self.session = DoNothingSession(None)
+
+    def test_getPtyIsANoOp(self):
+        # getPty is called on the way to establishing a shell. Since we don't
+        # give out shells, it should be a no-op. Raising an exception would
+        # log an OOPS, so we won't do that.
+        self.assertEqual(None, self.session.getPty(None, None, None))
+
+    def test_openShellNotImplemented(self):
+        # openShell closes the connection.
+        protocol = MockProcessTransport('bash')
+        self.session.openShell(protocol)
+        self.assertEqual(
+            [('writeExtended', connection.EXTENDED_DATA_STDERR,
+              'No shells on this server.\r\n'),
+             ('loseConnection',)],
+            protocol.log)
+
+    def test_windowChangedNotImplemented(self):
+        # windowChanged raises a NotImplementedError. It doesn't matter what
+        # we pass it.
+        self.assertRaises(NotImplementedError,
+                          self.session.windowChanged, None)
+
+    def test_providesISession(self):
+        # ExecOnlySession must provide ISession.
+        self.failUnless(ISession.providedBy(self.session),
+                        "ExecOnlySession doesn't implement ISession")
+
+    def test_closedDoesNothing(self):
+        # closed is a no-op.
+        self.assertEqual(None, self.session.closed())
+
+    def test_execCommandNotImplemented(self):
+        # ExecOnlySession.execCommand spawns the appropriate process.
+        protocol = MockProcessTransport('bash')
+        command = 'cat /etc/hostname'
+        self.session.execCommand(protocol, command)
+        self.assertEqual(
+            [('writeExtended', connection.EXTENDED_DATA_STDERR,
+              'Not allowed to execute commands on this server.\r\n'),
+             ('loseConnection',)],
+            protocol.log)
+
+    def test_eofReceivedDoesNothingWhenNoCommand(self):
+        # When no process has been created, 'eofReceived' is a no-op.
+        self.assertEqual(None, self.session.eofReceived())
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)