Launchpad itself

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

  1. poppy-cleanup
  2. Merge into devel
Proposed by Jonathan Lange
Status: Merged
Approved by: Brad Crittenden
Approved revision: no longer in the source branch.
Merged at revision: 11071
Proposed branch: lp:~jml/launchpad/poppy-cleanup
Merge into: lp:launchpad
Diff against target: 270 lines (+155/-26)
4 files modified
daemons/poppy-sftp.tac (+4/-0)
lib/lp/codehosting/sshserver/session.py (+3/-25)
lib/lp/services/sshserver/session.py (+43/-1)
lib/lp/services/sshserver/tests/test_session.py (+105/-0)
To merge this branch: bzr merge lp:~jml/launchpad/poppy-cleanup
Reviewer Review Type Date Requested Status
Brad Crittenden (community) code Approve
Review via email: mp+28525@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Brad Crittenden (bac) wrote :

Nice branch Jono

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'daemons/poppy-sftp.tac'
2--- daemons/poppy-sftp.tac 2010-05-27 07:07:24 +0000
3+++ daemons/poppy-sftp.tac 2010-06-25 17:21:32 +0000
4@@ -8,6 +8,7 @@
5 import os
6
7 from twisted.application import service
8+from twisted.conch.interfaces import ISession
9 from twisted.conch.ssh import filetransfer
10 from twisted.cred.portal import IRealm, Portal
11 from twisted.python import components
12@@ -22,6 +23,7 @@
13 from lp.services.sshserver.auth import (
14 LaunchpadAvatar, PublicKeyFromLaunchpadChecker)
15 from lp.services.sshserver.service import SSHService
16+from lp.services.sshserver.session import DoNothingSession
17
18 # XXX: Rename this file to something that doesn't mention poppy. Talk to
19 # bigjools.
20@@ -79,6 +81,8 @@
21 components.registerAdapter(
22 poppy_sftp_adapter, LaunchpadAvatar, filetransfer.ISFTPServer)
23
24+components.registerAdapter(DoNothingSession, LaunchpadAvatar, ISession)
25+
26
27 # Construct an Application that has the Poppy SSH server.
28 application = service.Application('poppy-sftp')
29
30=== modified file 'lib/lp/codehosting/sshserver/session.py'
31--- lib/lp/codehosting/sshserver/session.py 2010-04-15 15:41:05 +0000
32+++ lib/lp/codehosting/sshserver/session.py 2010-06-25 17:21:32 +0000
33@@ -12,16 +12,14 @@
34 import urlparse
35
36 from zope.event import notify
37-from zope.interface import implements
38
39-from twisted.conch.interfaces import ISession
40-from twisted.conch.ssh import connection
41 from twisted.internet.process import ProcessExitedAlready
42 from twisted.python import log
43
44 from canonical.config import config
45 from lp.codehosting import get_bzr_path
46 from lp.services.sshserver.events import AvatarEvent
47+from lp.services.sshserver.session import DoNothingSession
48
49
50 class BazaarSSHStarted(AvatarEvent):
51@@ -38,13 +36,11 @@
52 """Raised when a session is asked to execute a forbidden command."""
53
54
55-class ExecOnlySession:
56+class ExecOnlySession(DoNothingSession):
57 """Conch session that only allows executing commands."""
58
59- implements(ISession)
60-
61 def __init__(self, avatar, reactor, environment=None):
62- self.avatar = avatar
63+ super(ExecOnlySession, self).__init__(avatar)
64 self.reactor = reactor
65 self.environment = environment
66 self._transport = None
67@@ -72,11 +68,6 @@
68 if self._transport is not None:
69 self._transport.closeStdin()
70
71- def errorWithMessage(self, protocol, msg):
72- protocol.session.writeExtended(
73- connection.EXTENDED_DATA_STDERR, msg)
74- protocol.loseConnection()
75-
76 def execCommand(self, protocol, command):
77 """Executes `command` using `protocol` as the ProcessProtocol.
78
79@@ -115,19 +106,6 @@
80 args = command.split()
81 return args[0], args
82
83- def getPty(self, term, windowSize, modes):
84- """See ISession."""
85- # Do nothing, as we don't provide shell access. openShell will get
86- # called and handle this error message and disconnect.
87-
88- def openShell(self, protocol):
89- """See ISession."""
90- self.errorWithMessage(protocol, "No shells on this server.\r\n")
91-
92- def windowChanged(self, newWindowSize):
93- """See ISession."""
94- raise NotImplementedError()
95-
96
97 class RestrictedExecOnlySession(ExecOnlySession):
98 """Conch session that only allows a single command to be executed."""
99
100=== modified file 'lib/lp/services/sshserver/session.py'
101--- lib/lp/services/sshserver/session.py 2010-04-15 15:32:16 +0000
102+++ lib/lp/services/sshserver/session.py 2010-06-25 17:21:32 +0000
103@@ -5,10 +5,14 @@
104
105 __metaclass__ = type
106 __all__ = [
107+ 'DoNothingSession',
108 'PatchedSSHSession',
109 ]
110
111-from twisted.conch.ssh import channel, session
112+from zope.interface import implements
113+
114+from twisted.conch.interfaces import ISession
115+from twisted.conch.ssh import channel, connection, session
116
117
118 class PatchedSSHSession(session.SSHSession, object):
119@@ -77,3 +81,41 @@
120 resumeProducing = getattr(transport, 'resumeProducing', None)
121 if resumeProducing is not None:
122 resumeProducing()
123+
124+
125+class DoNothingSession:
126+ """A Conch user session that allows nothing."""
127+
128+ implements(ISession)
129+
130+ def __init__(self, avatar):
131+ self.avatar = avatar
132+
133+ def closed(self):
134+ """See ISession."""
135+
136+ def eofReceived(self):
137+ """See ISession."""
138+
139+ def errorWithMessage(self, protocol, msg):
140+ protocol.session.writeExtended(
141+ connection.EXTENDED_DATA_STDERR, msg)
142+ protocol.loseConnection()
143+
144+ def execCommand(self, protocol, command):
145+ """See ISession."""
146+ self.errorWithMessage(
147+ protocol, "Not allowed to execute commands on this server.\r\n")
148+
149+ def getPty(self, term, windowSize, modes):
150+ """See ISession."""
151+ # Do nothing, as we don't provide shell access. openShell will get
152+ # called and handle this error message and disconnect.
153+
154+ def openShell(self, protocol):
155+ """See ISession."""
156+ self.errorWithMessage(protocol, "No shells on this server.\r\n")
157+
158+ def windowChanged(self, newWindowSize):
159+ """See ISession."""
160+ raise NotImplementedError(self.windowChanged)
161
162=== added file 'lib/lp/services/sshserver/tests/test_session.py'
163--- lib/lp/services/sshserver/tests/test_session.py 1970-01-01 00:00:00 +0000
164+++ lib/lp/services/sshserver/tests/test_session.py 2010-06-25 17:21:32 +0000
165@@ -0,0 +1,105 @@
166+# Copyright 2010 Canonical Ltd. This software is licensed under the
167+# GNU Affero General Public License version 3 (see the file LICENSE).
168+
169+"""Tests for generic SSH session support."""
170+
171+__metaclass__ = type
172+
173+import unittest
174+
175+from twisted.conch.interfaces import ISession
176+from twisted.conch.ssh import connection
177+
178+from lp.services.sshserver.session import DoNothingSession
179+from lp.testing import TestCase
180+
181+
182+class MockSSHSession:
183+ """Just enough of SSHSession to allow checking of reporting to stderr."""
184+
185+ def __init__(self, log):
186+ self.log = log
187+
188+ def writeExtended(self, channel, data):
189+ self.log.append(('writeExtended', channel, data))
190+
191+
192+class MockProcessTransport:
193+ """Mock transport used to fake speaking with child processes that are
194+ mocked out in tests.
195+ """
196+
197+ def __init__(self, executable):
198+ self._executable = executable
199+ self.log = []
200+ self.session = MockSSHSession(self.log)
201+
202+ def closeStdin(self):
203+ self.log.append(('closeStdin',))
204+
205+ def loseConnection(self):
206+ self.log.append(('loseConnection',))
207+
208+ def signalProcess(self, signal):
209+ self.log.append(('signalProcess', signal))
210+
211+ def write(self, data):
212+ self.log.append(('write', data))
213+
214+
215+class TestDoNothing(TestCase):
216+ """Tests for ExecOnlySession."""
217+
218+ def setUp(self):
219+ super(TestDoNothing, self).setUp()
220+ self.session = DoNothingSession(None)
221+
222+ def test_getPtyIsANoOp(self):
223+ # getPty is called on the way to establishing a shell. Since we don't
224+ # give out shells, it should be a no-op. Raising an exception would
225+ # log an OOPS, so we won't do that.
226+ self.assertEqual(None, self.session.getPty(None, None, None))
227+
228+ def test_openShellNotImplemented(self):
229+ # openShell closes the connection.
230+ protocol = MockProcessTransport('bash')
231+ self.session.openShell(protocol)
232+ self.assertEqual(
233+ [('writeExtended', connection.EXTENDED_DATA_STDERR,
234+ 'No shells on this server.\r\n'),
235+ ('loseConnection',)],
236+ protocol.log)
237+
238+ def test_windowChangedNotImplemented(self):
239+ # windowChanged raises a NotImplementedError. It doesn't matter what
240+ # we pass it.
241+ self.assertRaises(NotImplementedError,
242+ self.session.windowChanged, None)
243+
244+ def test_providesISession(self):
245+ # ExecOnlySession must provide ISession.
246+ self.failUnless(ISession.providedBy(self.session),
247+ "ExecOnlySession doesn't implement ISession")
248+
249+ def test_closedDoesNothing(self):
250+ # closed is a no-op.
251+ self.assertEqual(None, self.session.closed())
252+
253+ def test_execCommandNotImplemented(self):
254+ # ExecOnlySession.execCommand spawns the appropriate process.
255+ protocol = MockProcessTransport('bash')
256+ command = 'cat /etc/hostname'
257+ self.session.execCommand(protocol, command)
258+ self.assertEqual(
259+ [('writeExtended', connection.EXTENDED_DATA_STDERR,
260+ 'Not allowed to execute commands on this server.\r\n'),
261+ ('loseConnection',)],
262+ protocol.log)
263+
264+ def test_eofReceivedDoesNothingWhenNoCommand(self):
265+ # When no process has been created, 'eofReceived' is a no-op.
266+ self.assertEqual(None, self.session.eofReceived())
267+
268+
269+def test_suite():
270+ return unittest.TestLoader().loadTestsFromName(__name__)