Launchpad itself

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

  1. ssh-errors-on-stderr-bug-335156
  2. Merge into devel
Proposed by Michael Hudson-Doyle
Status: Merged
Approved by: Jonathan Lange
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~mwhudson/launchpad/ssh-errors-on-stderr-bug-335156
Merge into: lp:launchpad
Diff against target: 112 lines (+30/-9)
2 files modified
lib/lp/codehosting/sshserver/session.py (+8/-5)
lib/lp/codehosting/sshserver/tests/test_session.py (+22/-4)
To merge this branch: bzr merge lp:~mwhudson/launchpad/ssh-errors-on-stderr-bug-335156
Reviewer Review Type Date Requested Status
Jonathan Lange (community) Approve
Review via email: mp+21353@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Revision history for this message
Jonathan Lange (jml) wrote :

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

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/codehosting/sshserver/session.py'
2--- lib/lp/codehosting/sshserver/session.py 2009-06-25 04:06:00 +0000
3+++ lib/lp/codehosting/sshserver/session.py 2010-03-15 07:00:56 +0000
4@@ -16,7 +16,7 @@
5 from zope.interface import implements
6
7 from twisted.conch.interfaces import ISession
8-from twisted.conch.ssh import channel, session
9+from twisted.conch.ssh import channel, connection, session
10 from twisted.internet.process import ProcessExitedAlready
11 from twisted.python import log
12
13@@ -128,6 +128,11 @@
14 if self._transport is not None:
15 self._transport.closeStdin()
16
17+ def errorWithMessage(self, protocol, msg):
18+ protocol.session.writeExtended(
19+ connection.EXTENDED_DATA_STDERR, msg)
20+ protocol.loseConnection()
21+
22 def execCommand(self, protocol, command):
23 """Executes `command` using `protocol` as the ProcessProtocol.
24
25@@ -140,8 +145,7 @@
26 try:
27 executable, arguments = self.getCommandToRun(command)
28 except ForbiddenCommand, e:
29- protocol.write(str(e) + '\r\n')
30- protocol.loseConnection()
31+ self.errorWithMessage(protocol, str(e) + '\r\n')
32 return
33 log.msg('Running: %r, %r, %r'
34 % (executable, arguments, self.environment))
35@@ -174,8 +178,7 @@
36
37 def openShell(self, protocol):
38 """See ISession."""
39- protocol.write("No shells on this server.\r\n")
40- protocol.loseConnection()
41+ self.errorWithMessage(protocol, "No shells on this server.\r\n")
42
43 def windowChanged(self, newWindowSize):
44 """See ISession."""
45
46=== modified file 'lib/lp/codehosting/sshserver/tests/test_session.py'
47--- lib/lp/codehosting/sshserver/tests/test_session.py 2010-02-19 01:05:19 +0000
48+++ lib/lp/codehosting/sshserver/tests/test_session.py 2010-03-15 07:00:56 +0000
49@@ -5,11 +5,10 @@
50
51 __metaclass__ = type
52
53-import os
54-import sys
55 import unittest
56
57 from twisted.conch.interfaces import ISession
58+from twisted.conch.ssh import connection
59 from twisted.internet.process import ProcessExitedAlready
60 from twisted.internet.protocol import ProcessProtocol
61
62@@ -36,6 +35,16 @@
63 return MockProcessTransport(executable)
64
65
66+class MockSSHSession:
67+ """Just enough of SSHSession to allow checking of reporting to stderr."""
68+
69+ def __init__(self, log):
70+ self.log = log
71+
72+ def writeExtended(self, channel, data):
73+ self.log.append(('writeExtended', channel, data))
74+
75+
76 class MockProcessTransport:
77 """Mock transport used to fake speaking with child processes that are
78 mocked out in tests.
79@@ -44,6 +53,7 @@
80 def __init__(self, executable):
81 self._executable = executable
82 self.log = []
83+ self.session = MockSSHSession(self.log)
84
85 def closeStdin(self):
86 self.log.append(('closeStdin',))
87@@ -90,7 +100,11 @@
88 # openShell closes the connection.
89 protocol = MockProcessTransport('bash')
90 self.session.openShell(protocol)
91- self.assertEqual(protocol.log[-1], ('loseConnection',))
92+ self.assertEqual(
93+ [('writeExtended', connection.EXTENDED_DATA_STDERR,
94+ 'No shells on this server.\r\n'),
95+ ('loseConnection',)],
96+ protocol.log)
97
98 def test_windowChangedNotImplemented(self):
99 # windowChanged raises a NotImplementedError. It doesn't matter what
100@@ -253,7 +267,11 @@
101 protocol = MockProcessTransport('cat')
102 self.assertEqual(
103 None, self.session.execCommand(protocol, 'cat'))
104- self.assertEqual(protocol.log[-1], ('loseConnection',))
105+ self.assertEqual(
106+ [('writeExtended', connection.EXTENDED_DATA_STDERR,
107+ "Not allowed to execute 'cat'.\r\n"),
108+ ('loseConnection',)],
109+ protocol.log)
110
111 def test_getCommandToRunReturnsTemplateCommand(self):
112 # When passed the allowed command, getCommandToRun always returns the