Merge lp:~mwhudson/launchpad/ssh-errors-on-stderr-bug-335156 into lp:launchpad

[Michael Hudson-Doyle]

Send the error messages from the ssh server about starting shells etc to the ssh clients stderr, not stdout.

Hi there,

This branch fixes the linked bug by sending the messages our smart server sends over stderr. I totally cargo culted the way to do this, but it works.

To test, run_codehosting and then run "BZR_REMOTE_PATH=foo bzr info lp://dev/gnome-terminal". In trunk, you'll get an ugly message as in the bug report and an oops on the server side. In this branch it should all be neat.

Cheers,
mwh

[Jonathan Lange (jml)]

I haven't tested, but it looks good to me.

review: Approve

=== modified file 'lib/lp/codehosting/sshserver/session.py'
--- lib/lp/codehosting/sshserver/session.py	2009-06-25 04:06:00 +0000
+++ lib/lp/codehosting/sshserver/session.py	2010-03-15 07:00:56 +0000
@@ -16,7 +16,7 @@
 from zope.interface import implements
 
 from twisted.conch.interfaces import ISession
-from twisted.conch.ssh import channel, session
+from twisted.conch.ssh import channel, connection, session
 from twisted.internet.process import ProcessExitedAlready
 from twisted.python import log
 
@@ -128,6 +128,11 @@
         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.
 
@@ -140,8 +145,7 @@
         try:
             executable, arguments = self.getCommandToRun(command)
         except ForbiddenCommand, e:
-            protocol.write(str(e) + '\r\n')
-            protocol.loseConnection()
+            self.errorWithMessage(protocol, str(e) + '\r\n')
             return
         log.msg('Running: %r, %r, %r'
                 % (executable, arguments, self.environment))
@@ -174,8 +178,7 @@
 
     def openShell(self, protocol):
         """See ISession."""
-        protocol.write("No shells on this server.\r\n")
-        protocol.loseConnection()
+        self.errorWithMessage(protocol, "No shells on this server.\r\n")
 
     def windowChanged(self, newWindowSize):
         """See ISession."""
 
=== modified file 'lib/lp/codehosting/sshserver/tests/test_session.py'
--- lib/lp/codehosting/sshserver/tests/test_session.py	2010-02-19 01:05:19 +0000
+++ lib/lp/codehosting/sshserver/tests/test_session.py	2010-03-15 07:00:56 +0000
@@ -5,11 +5,10 @@
 
 __metaclass__ = type
 
-import os
-import sys
 import unittest
 
 from twisted.conch.interfaces import ISession
+from twisted.conch.ssh import connection
 from twisted.internet.process import ProcessExitedAlready
 from twisted.internet.protocol import ProcessProtocol
 
@@ -36,6 +35,16 @@
         return MockProcessTransport(executable)
 
 
+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.
@@ -44,6 +53,7 @@
     def __init__(self, executable):
         self._executable = executable
         self.log = []
+        self.session = MockSSHSession(self.log)
 
     def closeStdin(self):
         self.log.append(('closeStdin',))
@@ -90,7 +100,11 @@
         # openShell closes the connection.
         protocol = MockProcessTransport('bash')
         self.session.openShell(protocol)
-        self.assertEqual(protocol.log[-1], ('loseConnection',))
+        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
@@ -253,7 +267,11 @@
         protocol = MockProcessTransport('cat')
         self.assertEqual(
             None, self.session.execCommand(protocol, 'cat'))
-        self.assertEqual(protocol.log[-1], ('loseConnection',))
+        self.assertEqual(
+            [('writeExtended', connection.EXTENDED_DATA_STDERR,
+             "Not allowed to execute 'cat'.\r\n"),
+             ('loseConnection',)],
+            protocol.log)
 
     def test_getCommandToRunReturnsTemplateCommand(self):
         # When passed the allowed command, getCommandToRun always returns the