twisted.conch.ssh.session has poor tests

[z3p]

See #2678, #2679, #2682, #2707, #2709.

[z3p]

Ready for review in branches/session-2710-2.

[therve]

First of all, there is a minor conflict on connection.py, you'll need to merge forward.

General:

• We don't yet stated on the blank lines, but 2 between classes/1 between method is a minimum. You can also apply the 3/2 rule.

unix.py:

• huge typo in the imports lines
• typo on subsystem in lookupSubsystem

session.py:

• use try/except/else return in the __init__ of SSHSession instead of try/return/except
• the test execCommand.func_code.co_argcount == 3 is a bit scary. There might be a better way, although I don't have one...
• most warnings should be set with stack_level=2, so that it appears in the good file when you use assertWarns.
• docstring of SSHSession.request_exec is cut somewhere
• docstring of SSHSession.getPty says @aram modes.
• I don't really like log.deferr, I'd prefer log.err. I don't know the official position on this :).
• the plain 'except:' are not great. In 2.6, {{{except Exception}}} will do the 'right' thing by not catching SystemExit/KeyboardInterrupt, but it'll change the semantic... Rha I don't know what's the good solution.

test_session.py:

• typo in the docstring twisted//conch/ssh/session.py. I would prefer twisted.conch.ssh.session though.
• StubClient must create the StubTransport in __init__, or you'll have to clean the buffer of transport (test_client fails with until-failure).

I guess this following ones are modified in other branches, so don't take my comments into account if it's the case:

test_userauth.py:

• unused import reactor

avatar.py:

• unused import log

That's it for now!

[z3p]

The last two are modified in other branches. The rest are ready for review in session-2710-3.

[therve]

• the change in StubClient miss a self, it broke a test
• test_requestExecGetsSession and test_requestShellGetsSession print warnings, they are missing a supress statement I guess
• speaking of it, suppress in general should be used with the message of the warning, so that it doesn't swallow all warnings
• while you're at it, if you do except Exception, you could do except Exception e and pass the exception instance to log.err.
• SSHSession.request_exec docstring is still broken
• there is bunch of docstrings missing in test_session: StubSessionForStubNewAvatar, MockProcessProtocol, MockProtocol in particular.

[radix]

log.err() seems preferable to log.err(e) here. log.err() will include traceback information, log.err(e) will not.

[z3p]

I think I've fixed these things (not including log.err) in seession-2710-3 so I'm putting this back up for review.

[therve]

Replying to radix:

log.err() seems preferable to log.err(e) here. log.err() will include traceback information, log.err(e) will not.

My bad, I didn't think there was a difference (that's kind of weird there is one in fact), but that was just for optimization so it doesn't really matter.

[therve]

• unused import util in test_session
• the conch script now prints some warnings, it should be fixed. I guess you should also open a ticket to add tests to it.

That's it!

[z3p]

I've fixed the two warnings I got when I ran the script and removed the unused import. As for the tests, once this is merged, I'm planning to merge forward free-assert-2112-3 to work on the script tests.

[therve]

I still have one warning:

twisted/conch/scripts/conch.py:346: DeprecationWarning: Using SSHSessionClient is deprecated.  Use SSHSession instead.
c = session.SSHSessionClient()

Otherwise it looks good to me, but I guess exarkun would like to have a look.

[z3p]

I've fixed this warning; bouncing to exarkun.

[exarkun]

simple stuff:

• avatar.py
  • no copyright statement or module docstring
  • twisted.python.log imported but unused
  • zope.interface import should come before twisted imports (imports should be in order of increasing dependencies - ie, if A uses B and C uses both and can reasonably know about A's use of B, C should import B before it imports A. This simplifies import-related debugging.)
  • ConchUser?.lookupChannel removes trailing whitespace but leaves whitespace around the = in the method call on the same lines, might as well fix that at the same time.
• interfaces.py
  • Lots of trailing whitespace
  • some methods (changed/added in this branch) smooshed right up against the method either before or after them - two blank lines between method definitions, please.
  • Could lookupSession's docstring use epytext?
• conch.py
  • is the change to eofReceived tested anywhere? (is eofReceived tested anywhere?)
  • what's up with the changes to the packRequest_pty_req calls? it looks like passing a list is deprecated in favor of passing a string, but the changes go the other way.
• session.py
  • some trailing whitespace
  • a module-level string above the list of supported signals - this should be a comment or something
  • SSHSession class docstring could use L{} in a few places, instead of C{}, to generate links to relevant API docs.
  • clobbering methods with instance attributes if there's no ISession is kinda ugly. Wouldn't it be cleaner to override requestReceived, do the check there, and then call the base implementation?
  • the log.err(e) call in request_session could probably use a 2nd argument explaining the context of the error.
  • doing argument count checks is pretty hairy - request_session will do nasty stuff to functions with varargs or optional args. not sure if there's much that can practically be done about this while preserving the method name openShell. It's a sticky point, though.
  • similar comment for request_exec. it might make sense to at least factor the counting code out into a helper function shared between these two.

Overall, I'm a bit more concerned with the backwards compatibility for the IConchUser/ISession change. Having # old style and # new style peppered throughout the code is certainly a recipe for weird bugs, broken compatibility, and other maintenance headaches. A less complex way to implement this kind of compatibility is to factor all of the old-style support code into one class, all of the new-style support code into another class, and then somehow arrange to have an instance of the right class at the right time (the most straightforward way being to have a completely separate object responsible for the area of the code with deprecations and have the main class, ie SSHSession, instantiate the right class based on what kind of object it gets). There are various subtleties involving interfaces (eg, if you create a new interface defining for the new API, then you can rely on adapters to pick the right implementation for you), but the basic idea remains the same.

I haven't looked over this code carefully enough yet to know if the current implementation is doing all of the right things wrt backwards compatibility. If you like, I can try to do that, and if it turns out to look correct, the branch can probably be merged (once above points are addressed, of course). However, I'd much prefer if the code were factored in the simpler way I just described, since it will result in better overall code and reviewing it will be much easier. :)

[z3p]

• avatar.py: fixed
• interfaces.py: fixed
• conch.py: the eofReceived change was because of a deprecation warning, but I'm not particular about keeping it. The packRequest_pty_req change is str -> list.
• session.py: I fixed whitespace, C{} -> L{}, and abstracted out the argument counting.

I disagree about the "clobbering methods." I think it's much cleaner than overriding requestReceived and special-casing the four requests there.

I understand what you're saying about backwards compatibility, but I don't think that moving all the backwards compatible code into a separate object will make the code much cleaner. It just replaces the # old style and # new style comments with method calls. I've written the code so that the tests that previously passed still pass with the new code, so as far as I'm concerned it's backwards compatible.

[glyph]

Replying to z3p:

I disagree about the "clobbering methods." I think it's much cleaner than overriding requestReceived and special-casing the four requests there.

Dispatching requests to different methods is a fairly standard "design pattern", especially within Twisted. Why is changing the interface of an object's attributes a preferable alternative?

I understand what you're saying about backwards compatibility, but I don't think that moving all the backwards compatible code into a separate object will make the code much cleaner. It just replaces the # old style and # new style comments with method calls.

... and moves all the backward compatibility code into a single unit which can be independently documented, and perhaps more importantly, independently removed when it is officially end-of-lifed. We have a lot more tools to work with method calls than with comments.

I've written the code so that the tests that previously passed still pass with the new code, so as far as I'm concerned it's backwards compatible.

Maaaybe this is okay for the code under discussion because it doesn't have too many users, but in general the issue is not the code you've written, it's the code that other people (launchpad et. al., in this case) have written that you don't have access to, but will break in some way you don't expect.

[z3p]

Okay, under the weight of both of your greater experience, I've moved the deprecated SSHSession code into a DeprecatedSSHSession class, which is instantiated to call the old methods. Currently I call it's methods with DeprecatedSSHSession(self).<method> each time because I couldn't decide on an appropriate place to make one instantiation. I also added a requestReceived method which does the ISession check. Ready for review.

[exarkun]

cftp tests are failing now, mostly like this:

  File "/home/exarkun/Projects/Twisted/trunk/twisted/conch/ssh/session.py", line 71, in requestReceived
    self.session = ISession(self.avatar)
exceptions.TypeError: ('Could not adapt', <twisted.conch.test.test_filetransfer.FileTransferTestAvatar instance at 0xb71de2cc>, <InterfaceClass twisted.conch.interfaces.ISession>)

I guess I didn't explain the backwards compatibility factoring idea very well, since the DeprecatedSSHSession(self).<method> thing isn't quite what I had in mind.

Consider request_subsystem:

    def request_subsystem(self, data):
        """
        The remote side has requested a subsystem.  Payload::
            string  subsystem name
        Try to get a subsystem object by calling our adapter's lookupSubsystem
        method.  If that method returns a subsystem, then connect to our
        client and return True.  Otherwise, return False.
        """
        subsystem, rest = common.getNS(data)
        log.msg('asking for subsystem "%s"' % subsystem)
        if hasattr(self.session, 'lookupSubsystem'): # new ISession
            client = self.session.lookupSubsystem(subsystem, rest)
            if client:
                self.makeConnection(client)
                return True
            else:
                return False
        else: # old ISession
            # we pass data instead of rest because that's what the old
            # code did
            return DeprecatedSSHSession(self).requestSubsystem(subsystem,
                    data)

The specific goals of the factoring change I tried to suggest are that:

• request_subsystem shouldn't need to know that the old ISession didn't have a lookupSubsystem method.
• request_subsystem shouldn't need to contain code for switching between old and new versions of the interface
• Implementations of the old ISession which happened to implement lookupSubsystem shouldn't be treated as implementations of the new ISession.

The ideal implementation of request_subsystem might look something like this:

    def request_subsystem(self, data):
        """
        The remote side has requested a subsystem.  Payload::
            string  subsystem name
        Try to get a subsystem object by calling our adapter's lookupSubsystem
        method.  If that method returns a subsystem, then connect to our
        client and return True.  Otherwise, return False.
        """
        subsystem, rest = common.getNS(data)
        log.msg('asking for subsystem "%s"' % subsystem)
        client = self.session.lookupSubsystem(subsystem, rest)
        if client:
            self.makeConnection(client)
            return True
        else:
            return False

If it can be made to work, this accomplishes all three of the above goals. I think it can be made to work, if self.session is initialized something like this (in either __init__ or requestReceived):

    session = getOldSession(avatar)
    if session is not None:
        session = AdaptOldSessionToNewSession(session)
    else:
        session = getNewSession(avatar)
    self.session = session

The specific details of getOldSession, getNewSession, and AdaptOldSessionToNewSession are important, of course, but in a lot of ways they're only secondary to the advantage this buys all the rest of the methods on SSHSession.

The most foolproof way to implement those three methods is to introduce a new interface which will ultimately supercede ISession, then replace getOldSession with ISession, getNewSession and AdaptOldSessionToNewSession with INewSession, and register an adapter from ISession to INewSession (which might have some overlap with DeprecatedSSHSession).

I hope this makes more sense. Sorry about the miscommunication earlier.

[glyph]

(In [22764]) Branching to 'session-2710-4'

[z3p]

Ready for review.

[therve]

• pyflakes errors:

  twisted/conch/scripts/conch.py:11: 'agent' imported but unused
  twisted/conch/scripts/conch.py:15: 'defer' imported but unused
  twisted/conch/scripts/conch.py:18: 'errno' imported but unused
  twisted/conch/scripts/conch.py:18: 'base64' imported but unused
  twisted/conch/scripts/conch.py:18: 'stat' imported but unused
  twisted/conch/scripts/conch.py:213: redefinition of unused 'errno' from line 18
  twisted/conch/scripts/conch.py:453: redefinition of unused 'errno' from line 18
  twisted/conch/scripts/conch.py:486: undefined name '_savedMode'
  

• trailing whitespaces
• test_filetransfer fails for me:

  [ERROR]: twisted.conch.test.test_filetransfer.TestFileTransferClose.test_stopConnectionServiceClosesChannel
  Traceback (most recent call last):
    File "/home/therve/Projects/Twisted/trunk/twisted/conch/test/test_filetransfer.py", line 503, in test_stopConnectionServiceClosesChannel
      self.interceptConnectionLost(sftpServer)
    File "/home/therve/Projects/Twisted/trunk/twisted/conch/test/test_filetransfer.py", line 435, in interceptConnectionLost
      origConnectionLost = sftpServer.connectionLost
  exceptions.AttributeError: _ProtocolToProcessProtocol instance has no attribute 'connectionLost'
  

• test_cftp is failing too
• ISessionApplication is missing documentation
• _SubsystemOnlyApplicationFactory.closed docstring is not finished. pass is unnecessary too.
• code is missing blank lines

That's it. I didn't really look deeply at the code yet, but it looked basically good. Thanks!

[z3p]

(In [23019]) Branching to 'session-2710-5'

[z3p]

I think I got all this stuff. I grepped around for trailing whitespace, but I didn't see any, so let me know if there's still some around.

[therve]

I have made the modifications for the whitespaces, and fixed a syntax problem with python 2.3.

• SSHSessionProcessProtocolTestCase.test_getSignalName is failing on several platforms. On windows 'module' object has no attribute 'SIGALRM'. On OS X exceptions.KeyError: 33. On some linux exceptions.KeyError: 66.

• StubApplicationFactoryForStubNewAvatar and StubSessionForStubOldAvatar use hasattr for several attributes: it should instead provide default value None for this attributes
• StubTransport has a undocumented close attribute
• tests should use assertIsInstance instead of assertTrue(isinstance(...))
• test_lookupSubsystem and test_lookupSubsystemProtocol both use a callLater(0). I don't know what's the good solution, but StubConnection could probably have a deferred that fire on some conditions (when a quantity of data sent reach a number for example).
• all calls to flushLoggedErrors should check their output. Otherwise, it could mask unexpected errors.
• test_processEndedWithExitSignalNoCoreDump and test_processEndedWithExitSignalCoreDump skip message is cut at the middle.
• in test_extendedDataReceived, if I comment the line pp.transport = object nothing happens

• in SSHSession, there are lots of 'if self.attribute', whereas we really want to check 'if self.attribute is not None'.

[therve]

(In [23184]) Address some points

Refs #2710

[jml]

Apparently addresses the issue in #2687.

[therve]

I think I've addressed all my points :). Can you have look?

[jml]

Hi Thomas & Paul,

This is a review of branches/session-2710-5.

This diff is over 3000 lines long. Although sometimes work can't be broken up, you should make an effort. The time it takes to review a branch scales non-linearly with the size of the diff.

Before you get into the review, I want to thank both of you for taking the time to do this work. Conch more than any other part of Twisted will benefit from better tests and being easier to test.

There's going to be another review round-trip and there *are* bits of the diff that I have serious issues with. Still, the patch as a whole shows clear thinking and a strong desire to separate concerns that were previously mixed together. You're attention to backwards compatibility is also greatly appreciated.

Because I'm coming to this branch cold, I'm going to be doing a full review. Personally, I hate it when an old branch of mine is subject to a fresh review by a fresh reviewer -- it means a lot of decisions get questioned again and feels like a lot of work little or no reason. It's also likely that the new reviewer will contradict the old reviewer.

I'm going to try as hard as I can to focus only on code cleanliness, and to assume that you and the previous reviewers knew what they were doing. What that means is I'll flag everything that's wrong and everything that I don't get, and I'll try to distinguish between the two.

I'd very much appreciate it if you could respond to each of my points, even if it is to say "no, I don't think I'll do that". The priority now is getting this branch landed. If you reply point-wise, my re-review can probably be done trivially.

Also, feel free to grab me on IRC or on skype ('blackjml') if you want to discuss any of these points.

Index: twisted/conch/test/test_filetransfer.py =================================================================== --- twisted/conch/test/test_filetransfer.py (revision 23210) +++ twisted/conch/test/test_filetransfer.py (working copy) @@ -56,11 +56,6 @@ return os.path.join(os.getcwd(), self.homeDir) -class ConchSessionForTestAvatar?: - - def init(self, avatar): - self.avatar = avatar - if unix: if not hasattr(unix, 'SFTPServerForUnixConchUser'): # unix should either be a fully working module, or None. I'm not sure @@ -463,7 +458,7 @@ self.interceptConnectionLost(sftpServer) # close session - testSession.closeReceived() + testSession.closed()

XXX I don't know what this is for. Let's see if I find out.

self.assertSFTPConnectionLost() Index: twisted/conch/scripts/conch.py =================================================================== --- twisted/conch/scripts/conch.py (revision 23210) +++ twisted/conch/scripts/conch.py (working copy) @@ -8,20 +8,21 @@ #""" Implementation module for the conch command. #"""

Please either delete this comment, or uncomment the docstring and make it comply with Twisted conventions.

-from twisted.conch.client import agent, connect, default, options +from twisted.conch.client import connect, default, options from twisted.conch.error import ConchError? from twisted.conch.ssh import connection, common from twisted.conch.ssh import session, forwarding, channel -from twisted.internet import reactor, stdio, defer, task +from twisted.internet import reactor, stdio, task from twisted.python import log, usage -import os, sys, getpass, struct, tty, fcntl, base64, signal, stat, errno +import os, sys, getpass, struct, tty, fcntl, signal, errno + class ClientOptions(options.ConchOptions?): - + synopsis = """Usage: conch [options] host [command] """ - + optParameters = [['escape', 'e', '~'], ['localforward', 'L', None, 'listen-port:host:port Forward local port to remote address'], ['remoteforward', 'R', None, 'listen-port:host:port Forward remote port to local address'],

Please wrap these lines to fit in 79 columns.

@@ -81,7 +82,7 @@ exitStatus = 0 old = None _inRawMode = 0 -_savedRawMode = None +_savedMode = None def run(): global options, old @@ -161,7 +162,7 @@ options.identitys = ['~/.ssh/id_rsa', '~/.ssh/id_dsa'] host = optionshost? if not optionsuser?: - optionsuser? = getpass.getuser() + optionsuser? = getpass.getuser() if not optionsport?: optionsport? = 22 else: @@ -210,7 +211,6 @@ try: os.close(i) except OSError, e: - import errno if e.errno != errno.EBADF: raise @@ -273,7 +273,7 @@ def requestRemoteForwarding(self, remotePort, hostport): data = forwarding.packGlobal_tcpip_forward(('0.0.0.0', remotePort)) - d = self.sendGlobalRequest('tcpip-forward', data, + d = self.sendGlobalRequest('tcpip-forward', data, wantReply=1) log.msg('requesting remote forwarding %s:%s' %(remotePort, hostport)) d.addCallback(self._cbRemoteForwarding, remotePort, hostport) @@ -283,7 +283,7 @@ log.msg('accepted remote forwarding %s:%s' % (remotePort, hostport)) self.remoteForwards[remotePort] = hostport log.msg(repr(self.remoteForwards)) - + def _ebRemoteForwarding(self, f, remotePort, hostport): log.msg('remote forwarding %s:%s failed' % (remotePort, hostport)) log.msg(f) @@ -330,7 +330,7 @@ else: # because of the unix thing self.class.bases[0].channelClosed(self, channel) - + class SSHSession(channel.SSHChannel): name = 'session' @@ -360,7 +360,7 @@ term = os.environTERM? winsz = fcntl.ioctl(fd, tty.TIOCGWINSZ, '12345678') winSize = struct.unpack('4H', winsz) - ptyReqData = session.packRequest_pty_req(term, winSize, ) + ptyReqData = session.packRequest_pty_req(term, winSize, [])

XXX I wonder why this is different.

self.conn.sendRequest(self, 'pty-req', ptyReqData) signal.signal(signal.SIGWINCH, self._windowResized) self.conn.sendRequest(self, 'exec', \ @@ -370,7 +370,7 @@ term = os.environTERM? winsz = fcntl.ioctl(fd, tty.TIOCGWINSZ, '12345678') winSize = struct.unpack('4H', winsz) - ptyReqData = session.packRequest_pty_req(term, winSize, ) + ptyReqData = session.packRequest_pty_req(term, winSize, [])

XXX I wonder why this is different.

self.conn.sendRequest(self, 'pty-req', ptyReqData) signal.signal(signal.SIGWINCH, self._windowResized) self.conn.sendRequest(self, 'shell', ) @@ -408,7 +408,8 @@ channels = self.conn.channels.keys() channels.sort() for channelId in channels: - self.stdio.write(' #%i %s\r\n' % (channelId, str(self.conn.channels[channelId]))) + self.stdio.write(' #%i %s\r\n' % (channelId, + str(self.conn.channels[channelId]))) return self.write('~' + char) else: @@ -425,8 +426,8 @@ def eofReceived(self): log.msg('got eof') - self.stdio.closeStdin() - + self.stdio.loseWriteConnection() + def closeReceived(self): log.msg('remote side closed %s' % self) self.conn.sendClose(self) @@ -449,7 +450,6 @@ try: os.close(i) except OSError, e: - import errno if e.errno != errno.EBADF: raise @@ -472,8 +472,8 @@ winSize = struct.unpack('4H', winsz) newSize = winSize[1], winSize[0], winSize[2], winSize[3] self.conn.sendRequest(self, 'window-change', struct.pack('!4L', *newSize)) - + class SSHListenClientForwardingChannel(forwarding.SSHListenClientForwardingChannel): pass class SSHConnectForwardingChannel(forwarding.SSHConnectForwardingChannel): pass @@ -504,7 +504,7 @@ new[0] = new[0] & ~tty.IUCLC # lflag - new[3] = new[3] & ~(tty.ISIG | tty.ICANON | tty.ECHO | tty.ECHO | + new[3] = new[3] & ~(tty.ISIG | tty.ICANON | tty.ECHO | tty.ECHO | tty.ECHOE | tty.ECHOK | tty.ECHONL) if hasattr(tty, 'IEXTEN'): new[3] = new[3] & ~tty.IEXTEN Index: twisted/conch/ssh/session.py =================================================================== --- twisted/conch/ssh/session.py (revision 23210) +++ twisted/conch/ssh/session.py (working copy) @@ -1,9 +1,7 @@ -# -*- test-case-name: twisted.conch.test.test_conch -*- -# Copyright (c) 2001-2004 Twisted Matrix Laboratories. +# -*- test-case-name: twisted.conch.test.test_session -*- +# Copyright (c) 2001-2007 Twisted Matrix Laboratories.

It's 2008 now.

# See LICENSE for details. -# - """ This module contains the implementation of SSHSession, which (by default) allows access to a shell and a python interpreter over SSH. @@ -11,214 +9,890 @@ Maintainer: U{Paul Swartz<mailto:z3p@twistedmatrix.com>} """ -import struct +import struct, warnings, signal, os, sys +from zope.interface import implements + from twisted.internet import protocol -from twisted.python import log -from twisted.conch.interfaces import ISession -from twisted.conch.ssh import common, channel +from twisted.python import components, log +from twisted.conch.interfaces import ISessionApplication +from twisted.conch.interfaces import ISessionApplicationFactory, ISession +from twisted.conch.ssh import common, channel, connection + +# supportedSignals is a list of signals that every session channel is supposed +# to support. +supportedSignals = ["ABRT", "ALRM", "FPE", "HUP", "ILL", "INT", "KILL", "PIPE", + "QUIT", "SEGV", "TERM", "USR1", "USR2"] + + + class SSHSession(channel.SSHChannel): + """ + A channel implementing the server side of the 'session' channel. This is + the channel that a client requests when it wants a subsystem, shell, or + command. + @ivar earlyData: data sent to this channel before a client is present. + @type earlyData: C{str} + @ivar earlyExtended: extended data sent to this channel before a client is + present. + @type earlyExtended: C{list} + @ivar applicationFactory: an object to which requests for shells and such + are dispatched. It implements I{ISessionApplicationFactory}. + @ivar sessionApplication: an object which handles interacting with the + other side of the channel. It implements I{ISessionApplication}. + @ivar client: a C{ProcessProtocol?} to which data sent to this channel is + sent. This variable is deprecated. + @type client: L{protocol.ProcessProtocol?} + @ivar session: an object implementing L{ISession} to which requests for + shells or commands are dispatched. This variable is deprecated. + """ + + + # set so that this object can be used as a client channel. name = 'session' + + + def setattr(self, k, v): + """ + Trap the 'client' attribute, what used to be the old name (roughly) for + 'sessionApplication', and 'session', which triggers setting up + _DeprecatedSSHSession as our application factory. + """ + if k == 'client': + # Someone is trying to inform us of an old-style client. Clear the + # buffers (because this would not have previously delivered any + # data, only delivered subsequent data) and set the old-style + # "client" object up as a new-style processProtocol. + self.earlyData = + self.earlyExtended = [] + self.setupSession(_TransportToProcessProtocol(v.transport)) + if k == 'session' and v is not None: + # Someone is trying to inform us of an old-style session. Wrap it + # in a _DeprecatedSSHSession. + self.applicationFactory = _DeprecatedSSHSession(self, v) + self.dict[k] = v +

I saw the 'setattr' and had a moment of quiet terror. Now I see that it's there for backwards compatibility -- this code will make my life easier, so thanks :)

However, I think that you could and should implement this with properties.

+ + def applicationFactory(self): + """ + Produce an applicationFactory dynamically if one does not yet exist. + """ + if self.avatar is not None: + factoryCandidate = ISessionApplicationFactory(self.avatar, None) + if factoryCandidate is None: + # Maybe it implements the old version. + oldStyle = ISession(self.avatar, None) + if oldStyle is not None: + # See setattr above. + self.session = oldStyle + else: + # Maybe it doesn't implement either. The test + # SFTP server doesn't implement a session, because + # subsystems were just looked up in the avatar. + # Use a _SubsystemOnlyApplicationFactory. + self.applicationFactory = _SubsystemOnlyApplicationFactory( + self) + else: + self.applicationFactory = factoryCandidate + return self.applicationFactory + applicationFactory = property(applicationFactory) +

This method doesn't do what it says it does. The check at the beginning is for whether 'self.avatar' is None. Calling this method multiple times will create an applicationFactory multiple times. Is this deliberate? If so, please update the docstring. If not, then I *think* you need to change the guard at the top of the function.

Also, it's not clear to me that applicationFactory should be None if no avatar is set. The docstring should state that this is the case and explain why.

+ + def client(self): + """ + This used to be the C{ProcessProtocol?} that was connected to us. + Since we're not connected to a C{ProcessProtocol?} anymore, we + fake it. + """ + if isinstance(self.sessionApplication, + _ProcessProtocolToSessionApplication): + return self.sessionApplication.processProtocol + elif isinstance(self.sessionApplication, + SSHSessionProcessProtocolApplication): + return self.sessionApplication.processProtocol + client = property(client) +

You should extract the if k == 'client' bit out of setattr and make it the setter for this property.

Also, I think this should raise a DeprecationWarning?.

+ + # This used to be set in the older SSHSession implementation. + session = None +

And this needs to have a getter. I'm sorry, but there's code out there that uses this.

+ def init(self, *args, **kw): channel.SSHChannel.init(self, *args, **kw) - self.buf = - self.client = None - self.session = None + self.earlyData = + self.earlyExtended = [] + self.sessionApplication = None + + def setupSession(self, sessionApplication): + """ + Connect us to the application. We set ourselves as it's channel + instance variable, and the application becomes our sessionApplication. + If any data was sent early, send it now. + """

Is this supposed to be public? If not, make it _setUpSession.

fIs there already an API or compatibility constraint on the name? If not, make it setUpSession.

+ # make sure we have an ISessionApplication + sessionApplication = ISessionApplication(sessionApplication) + sessionApplication.channel = self + sessionApplication.connectionMade() + # Maybe change this name too...

Please say why you think this name should maybe change.

+ self.sessionApplication = sessionApplication + if self.earlyData: + b, self.earlyData = self.earlyData, None + self.dataReceived(b) + if self.earlyExtended: + b, self.earlyExtended = self.earlyExtended, None + for dataType, data in b: + self.extReceived(dataType, data) +

I guess 'b' stands for 'bytes'. Could you please rename it?

+ def request_subsystem(self, data): - subsystem, ignored= common.getNS(data) + """ + The remote side has requested a subsystem. Payload:: + string subsystem name + + Try to get a subsystem object by calling our adapter's lookupSubsystem + method. If that method returns a subsystem, then connect to our + client and return True. Otherwise, return False. + """

I'm surprised you don't return the subsystem. Why don't you?

+ subsystem, rest = common.getNS(data) log.msg('asking for subsystem "%s"' % subsystem) - client = self.avatar.lookupSubsystem(subsystem, data) + client = self.applicationFactory.lookupSubsystem(subsystem, rest) if client: - pp = SSHSessionProcessProtocol(self) - proto = wrapProcessProtocol(pp) - client.makeConnection(proto) - pp.makeConnection(wrapProtocol(client)) - self.client = pp - return 1 + self.setupSession(client) + return True else: - log.msg('failed to get subsystem') - return 0 + return False + def request_shell(self, data): + """ + The remote side has requested a shell. No payload. Call the + application factory's openShell() method; it returns something + implementing I{ISessionApplication} that will become our + application. If there's no exception, return True. + Otherwise, return False. + """ log.msg('getting shell') - if not self.session: - self.session = ISession(self.avatar) try: - pp = SSHSessionProcessProtocol(self) - self.session.openShell(pp) - except: - log.deferr() - return 0 + self.setupSession(self.applicationFactory.openShell()) + except Exception:

This looks wrong. Why are you catching all exceptions here and not in other 'request_' methods?

Also, a bare except is the way to go, as long as you catch and reraise KeyboardInterrupt?.

+ log.err() + return False else: - self.client = pp - return 1 + return True + def request_exec(self, data): - if not self.session: - self.session = ISession(self.avatar) - f,data = common.getNS(data) - log.msg('executing command "%s"' % f) + """ + The remote side has requested to execute a command. Payload:: + string command line + + Call the application factory's execCommand method with the + command line. It should return something implementing + I{ISessionApplication} that becomes our client. If there's no + exception, return True. Otherwise, return False. + """ + command, data = common.getNS(data) + log.msg('executing command "%s"' % command) try: - pp = SSHSessionProcessProtocol(self) - self.session.execCommand(pp, f) - except: - log.deferr() - return 0 + self.setupSession(self.applicationFactory.execCommand(command)) + except Exception: + log.err() + return False

Same comment as before.

else: - self.client = pp - return 1 + return True + def request_pty_req(self, data): - if not self.session: - self.session = ISession(self.avatar) - term, windowSize, modes = parseRequest_pty_req(data) - log.msg('pty request: %s %s' % (term, windowSize)) + """ + The remote side has requested a psuedoterminal (PTY). Payload:: + string terminal name + uint32 columns + uint32 rows + uint32 xpixels + uint32 ypixels + string modes + + Modes is:: + 0 or more of:: + byte mode number + uint32 mode value + + Call the application factory's getPTY method. If no exception + is raised, return True. Otherwise, return False. + """

Thanks for including these docstrings, btw. They are a real help.

try: - self.session.getPty(term, windowSize, modes) - except: + term, windowSize, modes = parseRequest_pty_req(data) + log.msg('pty request: %s %s' % (term, windowSize)) + self.applicationFactory.getPTY(term, windowSize, modes) + except Exception: log.err() - return 0 + return False else: - return 1 + return True + def request_window_change(self, data): - if not self.session: - self.session = ISession(self.avatar) - winSize = parseRequest_window_change(data) + """ + The remote side has changed the window size. Payload:: + uint32 columns + uint32 rows + uint32 xpixels + uint32 ypixels + + + Call the application factory's windowChanged method. If no + exception is raised, return True. Otherwise, return False. + """ try: - self.session.windowChanged(winSize) - except: + winSize = parseRequest_window_change(data) + self.applicationFactory.windowChanged(winSize) + except Exception: log.msg('error changing window size') log.err() - return 0 + return False else: - return 1 + return True

OK, I'm beginning to suspect you just missed out the except clause in request_subsytem. Please make sure you add it and switch to bare excepts.

+ def dataReceived(self, data): - if not self.client: - #self.conn.sendClose(self) - self.buf += data + """ + We got data from the remote side. If we have an application, + send the data to it. Otherwise, buffer the data. + + @type data: C{str} + """ + if self.sessionApplication is None: + self.earlyData += data

Perhaps StringIO would be more appropriate than a string for earlyData. If you think so, whack an XXX comment somewhere in this file saying so -- this branch is already too big. Be sure to put your name and the date on the comment.

return - self.client.transport.write(data) + self.sessionApplication.dataReceived(data) + def extReceived(self, dataType, data): - if dataType == connection.EXTENDED_DATA_STDERR: - if self.client and hasattr(self.client.transport, 'writeErr'): - self.client.transport.writeErr(data) + """ + We got extended data from the remote side. If we have an + application, send the data to it. Otherwise, buffer the data. + + @type dataType: C{int} + @type data: C{str} + """

You know, I'd always wondered what this method was for. Thanks for adding the docstring :)

+ if self.sessionApplication is not None: + self.sessionApplication.extendedDataReceived(dataType, data) else: - log.msg('weird extended data: %s'%dataType) + self.earlyExtended.append((dataType, data)) + def eofReceived(self): - if self.session: - self.session.eofReceived() - elif self.client: - self.conn.sendClose(self) + """ + The remote side has closed its write side. If we have an + application factory, notify it. If we have an application, + notify it. + """ + if self.applicationFactory is not None: + self.applicationFactory.eofReceived() + if self.sessionApplication is not None: + self.sessionApplication.eofReceived() + def closed(self): - if self.session: - self.session.closed() - elif self.client: - self.client.transport.loseConnection() + """ + The channel is closed. If we have an application factory, + notify it. If we have an application, tell it the connection + is lost. + """

It *might* be worth mentioning here that just because the channel is closed, that doesn't mean that the SSH connection to the client is closed.

Just a thought.

+ if self.applicationFactory is not None: + self.applicationFactory.closed() + if self.sessionApplication is not None: + self.sessionApplication.closed() - #def closeReceived(self): - # self.loseConnection() # don't know what to do with this - def loseConnection(self): - if self.client: - self.client.transport.loseConnection() - channel.SSHChannel.loseConnection(self) + def closeReceived(self): + """ + The remote side has requested that we no longer send data. If + we have an application factory, notify it. If we have an + application, notify it. + """

Good docstring.

-class _ProtocolWrapper(protocol.ProcessProtocol?): + if self.applicationFactory is not None: + self.applicationFactory.closeReceived() + if self.sessionApplication is not None: + self.sessionApplication.closeReceived() + + + # client methods +

'client methods' is ambiguous. Please say what they are used for, why they are different to the previous methods and, if you think it's appropriate, why they are in this class at all.

+ + def getPty(self, term, windowSize, modes, wantReply=False): + """ + Request a PTY from the other side. + + @param term: the type of terminal (e.g. xterm) + @type term: C{str} + @param windowSize: the size of the window: (rows, cols, xpixels, + ypixels) + @type windowSize: C{tuple} + @param modes: termanal modes; a list of (modeNumber, modeValue) pairs.

It's spelled 'terminal'. It might be worth putting in a 'see also', so people who want to know what modeNumber and modeValue mean can find out.

+ @type modes: C{list} + @param wantReply: True if we want a reply to this request. + @type wantReply: C{bool} + + @returns: if wantReply, a Deferred that will be called back when + the request has succeeded or failed; else, None. + @rtype: C{Deferred}/C{None} + """ + data = packRequest_pty_req(term, windowSize, modes) + return self.conn.sendRequest(self, 'pty-req', data, + wantReply=wantReply) + + + def openSubsystem(self, subsystem, data=, wantReply=False): + """ + Request that a subsystem be opened on the other side. + + @param subsystem: the name of the subsystem + @type subsystem: C{str} + @param data: any extra data to send with the request + @type data: C{str} + @param wantReply: True if we want a reply to this request. + @type wantReply: C{bool} + + @returns: if wantReply, a Deferred that will be called back when + the request has succeeded or failed; else, None. + @rtype: C{Deferred}/C{None} + """ + return self.conn.sendRequest(self, 'subsystem', common.NS(subsystem) + + data, wantReply=wantReply) + + + def openShell(self, wantReply=False): + """ + Request that a shell be opened on the other side. + + @param wantReply: True if we want a reply to this request. + @type wantReply: C{bool} + + @returns: if wantReply, a Deferred that will be called back when + the request has succeeded or failed; else, None. + @rtype: C{Deferred}/C{None} + """ + return self.conn.sendRequest(self, 'shell', , wantReply=wantReply) + + + def execCommand(self, command, wantReply=False): + """ + Request that a command be executed on the other side. + + @param command: the command to execute + @type command: C{str} + @param wantReply: True if we want a reply to this request. + @type wantReply: C{bool} + + @returns: if wantReply, a Deferred that will be called back when + the request has succeeded or failed; else, None. + @rtype: C{Deferred}/C{None} + """ + return self.conn.sendRequest(self, 'exec', common.NS(command), + wantReply=wantReply) + + + def changeWindowSize(self, windowSize, wantReply=False): + """ + Inform the other side that the local terminal size has changed. + + @param windowSize: the new size of the window: (rows, cols, xpixels, + ypixels) + @type windowSize: C{tuple} + @param wantReply: True if we want a reply to this request. + @type wantReply: C{bool} + + @returns: if wantReply, a Deferred that will be called back when + the request has succeeded or fails; else, None. + @rtype: C{Deferred}/C{None} + """ + data = packRequest_window_change(windowSize) + return self.conn.sendRequest(self, 'window-change', data, + wantReply=wantReply) +

I'm guessing that this wantReply business is for backwards compat? It seems a little strange otherwise.

+ + +class _SubsystemOnlyApplicationFactory: """ + An application factory which which is only good for looking up a + subsystem. It is used when there is not an ISession adapter + defined for the avatar. Its use is deprecated. + """

If it's deprecated then please make sure it issues a deprecation warning when used. See twisted.python.deprecate.

+ implements(ISessionApplicationFactory) + + + def init(self, sessionChannel): + self.sessionChannel = sessionChannel + + + def lookupSubsystem(self, subsystem, data): + """ + The previous ConchUser? avatar had subsystems looked up through + the avatar instead of through the ISession adapter. To be backwards + compatible, try to look up the subsystem through the avatar. + """ + self.sessionChannel.earlyData = + self.sessionChannel.earlyExtended = [] + client = self.sessionChannel.avatar.lookupSubsystem(subsystem, + common.NS(subsystem) + data) + return wrapProcessProtocol(client) + + + def eofReceived(self): + self.sessionChannel.loseConnection() + + + def closeReceived(self): + """ + The old closeReceived method did not exist, so we dispatch to our + parent's method. + """ + channel.SSHChannel.closeReceived(self.sessionChannel) + + + def closed(self): + """ + The old implementation of this method didn't do anything, so we + don't do anything either. + """ + + + +class _DeprecatedSSHSession(_SubsystemOnlyApplicationFactory): + """ + This class brings the deprecated functionality of the old SSHSession + into a single place. + """ + + + def init(self, sessionChannel, oldISessionProvider): + _SubsystemOnlyApplicationFactory.init(self, sessionChannel) + self.oldISessionProvider = oldISessionProvider + + + def getPTY(self, term, windowSize, modes): + """ + The name of this method used to be getPty. + """ + return self.oldISessionProvider.getPty(term, windowSize, modes) + + + def openShell(self): + """ + The old openShell interface passed in a ProcessProtocol? which would be + connected to the shell. + """ + pp = SSHSessionProcessProtocol(self.sessionChannel) + self.oldISessionProvider.openShell(pp) + return SSHSessionProcessProtocolApplication(pp) + + + def execCommand(self, command): + """ + The old execCommand interface passed in a ProcessProtocol? which would + be connected to the command. + """ + pp = SSHSessionProcessProtocol(self.sessionChannel) + self.oldISessionProvider.execCommand(pp, command) + return SSHSessionProcessProtocolApplication(pp) + + + def eofReceived(self): + """ + The old eofReceived method closed the session connection. + """ + self.oldISessionProvider.eofReceived() + self.sessionChannel.conn.sendClose(self.sessionChannel) + + + def closed(self): + """ + The old closed method closed the client's transport. + """ + if self.sessionChannel.sessionApplication is not None: + self.sessionChannel.sessionApplication.processProtocol.transport.loseConnection() + self.oldISessionProvider.closed() + + + def windowChanged(self, newWindowSize): + """ + Just pass this method through. + """ + self.oldISessionProvider.windowChanged(newWindowSize) + + + +class _ProtocolToProcessProtocol(protocol.ProcessProtocol?): + """ This class wraps a L{Protocol} instance in a L{ProcessProtocol?} instance. + + @ivar proto: the C{Protocol} we're wrapping. """

This belongs in twisted.internet, not twisted.conch. Please add a XXX comment saying so (with name and the date).

+ + def init(self, proto): self.proto = proto - def connectionMade(self): self.proto.connectionMade() - def outReceived(self, data): self.proto.dataReceived(data) + def getattr(self, attr): + """ + This class did not previously exist, so some uses expect this object + to implement the Protocol interface. To handle this case, we pass + requests through to the wrapped object. + """ + return getattr(self.proto, attr)

Is there a reason for not using proxyForInterfaces?

- def processEnded(self, reason): self.proto.connectionLost(reason) -class _DummyTransport: + def setattr(self, attr, value): + """ + See the documentation for getattr. + """ + if attr in ('proto', 'transport'): + self.dict[attr] = value + else: + setattr(self.proto, attr, value) - def init(self, proto): - self.proto = proto - def dataReceived(self, data): - self.proto.transport.write(data) + def connectionMade(self): + """ + Connect our C{Protocol} to our transport. + """ + self.proto.makeConnection(self.transport) + self.transport.proto = self - def write(self, data): + + def outReceived(self, data): + """ + Give the data to the C{Protocol}s dataReceived method. + """ self.proto.dataReceived(data) - def writeSequence(self, seq): - self.write(.join(seq)) - def loseConnection(self): - self.proto.connectionLost(protocol.connectionDone) + def processEnded(self, reason): + """ + Give the C{Failure} to the C{Protocol}s connectionLost method. + """ + self.proto.connectionLost(reason) + + def wrapProcessProtocol(inst): + """ + If we're passed a C{Protocol}, wrap it in a C{ProcessProtocol?}. + Otherwise, just return what we were passed. + """ if isinstance(inst, protocol.Protocol): - return _ProtocolWrapper(inst) + return _ProtocolToProcessProtocol(inst) else: return inst

Can you use adapters for this instead?

If not, add a XXX comment saying that you *should* use adapters, and ideally, file a bug about it.

-def wrapProtocol(proto): - return _DummyTransport(proto) + +class _TransportToProcessProtocol(protocol.ProcessProtocol?): + """ + This wraps something implementing I{ITransport} in a + C{ProcessProtocol?} because that's how the old interface worked. +

Please say specifically which object you mean by "the old interface".

Given that this is backwards compatibility code, it would be good to say in which version of Twisted this was added. That way, we'll know when we can remove it.

+ @ivar applicationTransport: the ITransport we're wrapping. + """ + + + def init(self, applicationTransport): + self.applicationTransport = applicationTransport + + + def outReceived(self, data): + """ + When we get data, write it to our transport. + """ + self.applicationTransport.write(data) + + + def errReceived(self, errData): + """ + When we get extended data, give it to the writeErr method of + our transport. + """ + # XXX need to test when writeErr isn't available + self.applicationTransport.writeErr(errData) + + + def processEnded(self, reason): + """ + When we're told the process ended, tell the transport to drop + the connection. Yes, this loses data. + """ + self.applicationTransport.loseConnection() + + + +class _ProcessProtocolToSessionApplication: + """ + This adapts a C{ProcessProtocol?} to an ISessionApplication. + + @ivar processProtocol: the C{ProcessProtocol?} we're adapting. + """ +

It would be good to explain in the docstring why you need this adapter.

Same comments on including the version apply.

+ + implements(ISessionApplication) + + + def init(self, processProtocol): + self.processProtocol = processProtocol + + + def connectionMade(self): + """ + Connect the C{ProcessProtocol?} to the channel. + """ + self.processProtocol.makeConnection(self.channel) + + + def dataReceived(self, data): + """ + Give the data to the C{ProcessProtocol?} + """ + self.processProtocol.outReceived(data) + + + def extendedDataReceived(self, type, data): + """ + If the extended data came from standard error, give it to the + C{ProcessProtocol?}. Otherwise drop it on the floor. + """ + if type == connection.EXTENDED_DATA_STDERR: + self.processProtocol.errReceived(data) + + + def eofReceived(self): + """ + Tell the C{ProcessProtocol?} that no more data will be sent to + standard input. + """ + self.processProtocol.inConnectionLost() + + + def closeReceived(self): + """ + Tell the C{ProcessProtocol?} that it shouldn't send any more data. + """ + self.processProtocol.outConnectionLost() + self.processProtocol.errConnectionLost() + + + def closed(self): + """ + Tell the C{ProcessProtocol?} that the process ended. + + TODO: catch request_exit_status to give a better error message. + """ + self.processProtocol.processEnded(protocol.connectionDone) + + + +components.registerAdapter(_ProcessProtocolToSessionApplication, + protocol.ProcessProtocol?, ISessionApplication) + + + class SSHSessionProcessProtocol(protocol.ProcessProtocol?): + """ + This is a C{ProcessProtocol?} that is used to connect to old-style + ISession implementations. An instance of this is passed to + openShell() and execCommand(), and then it's wrapped using + SSHSessionProcessProtocolApplication. + """ -# implements = I + + _signalNames = None +

Please add a comment explaining the structure of this variable.

+ def init(self, session): self.session = session - def connectionMade(self): - if self.session.buf: - self.transport.write(self.session.buf) - self.session.buf = None + def _getSignalName(self, signum): + """ + Get a signal name given a signal number. + """ + if self._signalNames is None: + self.class._signalNames = {} + # make sure that the POSIX ones are the defaults + for signame in supportedSignals: + signame = 'SIG' + signame + sigvalue = getattr(signal, signame, None) + if sigvalue is not None: + self._signalNames[sigvalue] = signame + for k, v in signal.dict.items(): + if k.startswith('SIG') and not k.startswith('SIG_'): + if v not in self._signalNames: + self._signalNames[v] = k + '@' + sys.platform + return self._signalNames[signum] +

So, figuring this out, _signalNames maps from signal numbers to signal names.

The names are either the regular names (e.g. 'SIGKILL') or they are names with platforms at the end (e.g. 'SIGCHLD@linux'). They are in the latter if they aren't in 'supportedSignals', which appears to be a global.

The 'SIG_' guard is there because the Python signal module has variables that start with 'SIG_' that aren't the names of signals.

OK, I think I've got it. Here are the recommendations:

• Make supportedSignals 'SUPPORTED_SIGNALS'.
• If 'supported' really means 'all platforms', then say so.
• The self.class thing smells like premature optimization. Just use self.

+ def outReceived(self, data): + """ + Sent data from standard out over the channel. + """ self.session.write(data) + def errReceived(self, err): + """ + Send data from standard error as extended data of type + EXTENDED_DATA_STDERR. + """ self.session.writeExtended(connection.EXTENDED_DATA_STDERR, err) + def inConnectionLost(self): + """ + If we're told the incoming connection has been lost, send an EOF + over the channel. + """ self.session.conn.sendEOF(self.session) - def connectionLost(self, reason = None): - self.session.loseConnection() - def processEnded(self, reason = None): - if reason and hasattr(reason.value, 'exitCode'): - log.msg('exitCode: %s' % repr(reason.value.exitCode)) - self.session.conn.sendRequest(self.session, 'exit-status', struct.pack('!L', reason.value.exitCode)) + def outConnectionLost(self): + """ + If we're running as an subsystem, close the connection. + """ + if self.session.session is None: # we're running as an old-style + # subsystem + self.session.loseConnection() + + + def processEnded(self, reason=None): + """ + When we are told the process ended, try to notify the other side about + how the process ended using the exit-signal or exit-status requests. + Also, close the channel. + """ + if reason is not None and hasattr(reason.value, 'exitCode'): + err = reason.value + if err.signal is not None: + signame = self._getSignalName(err.signal) + log.msg('exitSignal: %s' % (signame,)) + if hasattr(os, 'WCOREDUMP') and os.WCOREDUMP(err.status): + coreDumped = 1 + else: + coreDumped = 0

If core was dumped, the log should say so. This is very useful information for debugging.

+ self.session.conn.sendRequest(self.session, 'exit-signal', + common.NS(signame[3:]) + chr(coreDumped) + + common.NS() + common.NS()) + elif err.exitCode is not None: + log.msg('exitCode: %r' % (err.exitCode,)) + self.session.conn.sendRequest(self.session, 'exit-status', + struct.pack('>L', err.exitCode))

What if 'err' has no exitCode and no signal?

self.session.loseConnection() - # transport stuff (we are also a transport!) + # also a transport :(

I share your sadness, but not your understanding. Why must this be a transport? What can we do about it? Please explain these things in the comment.

def write(self, data): + """ + If we're used like a transport, just send the data to the channel. + """ self.session.write(data) - def writeSequence(self, seq): - self.session.write(.join(seq)) def loseConnection(self): + """ + If we're use like a transport, send the close message. + """

'used', not 'use'.

self.session.loseConnection() -class SSHSessionClient(protocol.Protocol): + +class SSHSessionProcessProtocolApplication: + """ + Another layer of wrapping to make the old-style ISession + implemention work. This adapts SSHSessionProcessProtocol to + ISessionApplication. + + @ivar processProtocol: the C{SSHSessionProcessProtocol} we're adapting. + """ +

Gosh there are a lot of these. I think it would be worth giving a summary in the module-level docstring. I'm starting to find it all a bit confusing.

+ + implements(ISessionApplication) + + + def init(self, processProtocol): + self.processProtocol = processProtocol + + + def connectionMade(self): + """ + Don't need to do anything because SSHSessionProcessProtocol's + transport doesn't do anything with this. + """ + + def dataReceived(self, data): - if self.transport: - self.transport.write(data) + """ + When we get data, write it to the SSHSessionProcessProtocol's + transport. + """ + self.processProtocol.transport.write(data) + + def extendedDataReceived(self, dataType, data): + """ + If we get extended data from standard error and the transport + has a writeErr method, pass the data along. + """ + if not hasattr(self.processProtocol.transport, 'writeErr'): + return + if dataType == connection.EXTENDED_DATA_STDERR: + self.processProtocol.transport.writeErr(data) + + + def eofReceived(self): + """ + Don't need to implement this because + SSHSessionProcessProtocol's transport doesn't do anything with + this. + """ + + + def closeReceived(self): + """ + Don't need to implement this because + SSHSessionProcessProtocol's transport doesn't do anything with + this. + """ + + + def closed(self): + """ + Don't need to implement this because + SSHSessionProcessProtocol's transport doesn't do anything with + this. + """ +

No adapter registration?

+ + +class SSHSessionClient(protocol.Protocol): + """ + A class the conch command-line client uses to connect the channel + to standard output. Deprecated. + """ + def dataReceived(self, data): + """ + Send data to the transport. + """ + self.transport.write(data) +

If it's deprecated, raise a warning on construction.

+ + # methods factored out to make live easier on server writers def parseRequest_pty_req(data): - """Parse the data from a pty-req request into usable data. + """ + Parse the data from a pty-req request into usable data. @returns: a tuple of (terminal type, (rows, cols, xpixel, ypixel), modes) """

Please say where 'terminal type' and 'modes' come from.

By the way, there's no harm at all in referring to the RFC number and section in a docstring, especially for a protocol as complex as SSH.

@@ -226,30 +900,48 @@ cols, rows, xpixel, ypixel = struct.unpack('>4L', rest[: 16]) modes, ignored= common.getNS(rest[16:]) winSize = (rows, cols, xpixel, ypixel) - modes = [(ord(modes[i]), struct.unpack('>L', modes[i+1: i+5])[0]) for i in range(0, len(modes)-1, 5)] + modes = [(ord(modes[i]), struct.unpack('>L', modes[i+1: i+5])[0]) for i in + range(0, len(modes)-1, 5)]

No space after the colon, spaces around the '-' in 'len(modes)-1'.

return term, winSize, modes + + def packRequest_pty_req(term, (rows, cols, xpixel, ypixel), modes): - """Pack a pty-req request so that it is suitable for sending. + """ + Pack a pty-req request so that it is suitable for sending. NOTE: modes must be packed before being sent here. """ termPacked = common.NS(term) winSizePacked = struct.pack('>4L', cols, rows, xpixel, ypixel) - modesPacked = common.NS(modes) # depend on the client packing modes + if not isinstance(modes, str): + modes = .join([chr(m[0]) + struct.pack('>L', m[1]) for m in modes]) + else: + warnings.warn("packRequest_pty_req should be packing the modes.", + DeprecationWarning?, stacklevel=2)

There's an API for this now: twisted.python.deprecate.

+ modesPacked = common.NS(modes) return termPacked + winSizePacked + modesPacked + + def parseRequest_window_change(data): - """Parse the data from a window-change request into usuable data. + """ + Parse the data from a window-change request into usuable data. @returns: a tuple of (rows, cols, xpixel, ypixel) """ cols, rows, xpixel, ypixel = struct.unpack('>4L', data) return rows, cols, xpixel, ypixel + + def packRequest_window_change((rows, cols, xpixel, ypixel)): - """Pack a window-change request so that it is suitable for sending. """ + Pack a window-change request so that it is suitable for sending. + """ return struct.pack('>4L', cols, rows, xpixel, ypixel) -import connection + + +all = ['SSHSession', 'packRequest_window_change', 'packRequest_pty_req', + 'parseRequest_window_change', 'parseRequest_pty_req']

I think the world would be a happier place if this were at the top of the file.

Index: twisted/conch/avatar.py =================================================================== --- twisted/conch/avatar.py (revision 23210) +++ twisted/conch/avatar.py (working copy) @@ -1,9 +1,11 @@ # -*- test-case-name: twisted.conch.test.test_conch -*- -from interfaces import IConchUser -from error import ConchError? -from ssh.connection import OPEN_UNKNOWN_CHANNEL_TYPE -from twisted.python import log +# Copyright (c) 2007 Twisted Matrix Laboratories. +# See LICENSE for details. +

2008 :)

from zope import interface +from twisted.conch.interfaces import IConchUser +from twisted.conch.error import ConchError? +from twisted.conch.ssh.connection import OPEN_UNKNOWN_CHANNEL_TYPE class ConchUser?: interface.implements(IConchUser) @@ -17,12 +19,16 @@ if not klass: raise ConchError(OPEN_UNKNOWN_CHANNEL_TYPE, "unknown channel") else: - return klass(remoteWindow = windowSize, - remoteMaxPacket = maxPacket, + return klass(remoteWindow=windowSize, + remoteMaxPacket=maxPacket, data=data, avatar=self) def lookupSubsystem(self, subsystem, data): - log.msg(repr(self.subsystemLookup)) + """ + This is deprecated in favor of ISession.lookupSubsystem. However, + because there is already a warning in SSHSession.request_subsystem, + we do not warn again here. + """ klass = self.subsystemLookup.get(subsystem, None) if not klass: return False Index: twisted/conch/interfaces.py =================================================================== --- twisted/conch/interfaces.py (revision 23210) +++ twisted/conch/interfaces.py (working copy) @@ -4,7 +4,7 @@ from zope.interface import Interface class IConchUser(Interface): - """A user who has been authenticated to Cred through Conch. This is + """A user who has been authenticated to Cred through Conch. This is the interface between the SSH connection and the user. @ivar conn: The SSHConnection object for this user. @@ -30,26 +30,24 @@ @rtype: subclass of L{SSHChannel}/C{tuple} """ - def lookupSubsystem(subsystem, data): - """ - The other side requested a subsystem. - subsystem is the name of the subsystem being requested. - data is any other packet data (often nothing). - - We return a L{Protocol}. - """ + def gotGlobalRequest(requestType, data): """ A global request was sent from the other side. - + By default, this dispatches to a method 'channel_channelType' with any - non-alphanumerics in the channelType replace with _'s. If it cannot - find a suitable method, it returns an OPEN_UNKNOWN_CHANNEL_TYPE error. + non-alphanumerics in the channelType replace with _'s. If it cannot + find a suitable method, it returns an OPEN_UNKNOWN_CHANNEL_TYPE error. The method is called with arguments of windowSize, maxPacket, data. """ + class ISession(Interface): + """ + The old, deprecated interface for implementing SSH session applications. + Please don't use this for anything. + """

Please say which version it was deprecated in.

def getPty(term, windowSize, modes): """ @@ -82,13 +80,143 @@ """ Called when the other side has indicated no more data will be sent. """ - + def closed(): """ Called when the session is closed. """ + + +class ISessionApplicationFactory(Interface): + """ + The new, good interface for implementing an SSH session. Use this to + implement the application side of an SSH server session. + + @ivar channel: the L{SSHChannel} object to which this application is + connected. Use this attribute to send data, etc. + """ + + def getPTY(term, windowSize, modes): + """ + Get a psuedo-terminal for use by a shell or command. + + If a psuedo-terminal is not available, or the request otherwise + fails, raise an exception. + """ +

"pseudo", not "psuedo".

+ + def lookupSubsystem(subsystem, data): + """ + The other side requested a subsystem.

Blank line.

+ @param subsystem: the name of the subsystem being requested. + @param data: any other packet data (often nothing). + + @return: something that implements I{IConchApplication} + @raises: If the subsystem is unavailable, raise an exception. + """ + + + def openShell(): + """ + The other side requested a shell. + + @return: something that implementes I{IConchApplication} + @raises: If the subsystem is unavailable, raise an exception. + """ + + + def execCommand(command): + """ + The other side requested a shell. + + @param command: the command the other side wants us to execute + @type command: C{str} + + @return: something that implementes I{IConchApplication} + @raises: If the subsystem is unavailable, raise an exception. + """ + + + def windowChanged(newWindowSize): + """ + Called when the size of the remote screen has changed. + """ + + + def eofReceived(): + """ + Called when the other side has indicated no more data will be sent. + """ + + + def closeReceived(): + """ + Called when the other side has indicated it wants no more data. + """ + + + def closed(): + """ + Called when the session channel is closed. + """ + +

One more blank line please.

+class ISessionApplication(Interface): + """ + Objects which are returned by an ISessionApplicationFactory implementor + should implement this class. It receives data from the channel and can + send it back. + + @ivar channel: the C{SSHSession} to which this application is connected. + """ + + + def connectionMade(): + """ + Called when this application is connected to the channel. + """ + + + def dataReceived(data): + """ + Called when data is received from the other side. + + @type data: C{str} + """ + + + def extendedDataReceived(extendedDataType, extendedData): + """ + Called when extended data is received from the other side. + + @param extendedDataType: the type of extended data. Typically, this + is connection.EXTENDED_DATA_STDERR. + @type extendedDataType: C{int} + @type extendedData: C{str} + """ + + + def eofReceived(): + """ + Called when the other side has said it will not send any more data. + """ + + + def closeReceived(): + """ + Called when the other side has said it will not accept any more data. + """ + + + def closed(): + """ + Called when the application is closed on both sides. + """ + + + class ISFTPServer(Interface): """ The only attribute of this class is "avatar". It is the avatar @@ -350,5 +478,3 @@ @param attrs: a dictionary in the same format as the attrs argument to L{openFile}. """ - - Index: twisted/conch/unix.py =================================================================== --- twisted/conch/unix.py (revision 23210) +++ twisted/conch/unix.py (working copy) @@ -5,19 +5,20 @@ from twisted.python import components, log from twisted.internet.error import ProcessExitedAlready? from zope import interface -from ssh import session, forwarding, filetransfer -from ssh.filetransfer import FXF_READ, FXF_WRITE, FXF_APPEND, FXF_CREAT, FXF_TRUNC, FXF_EXCL +from twisted.conch.ssh import session, forwarding, filetransfer +from twisted.conch.ssh.filetransfer import FXF_READ, FXF_WRITE, FXF_APPEND +from twisted.conch.ssh.filetransfer import FXF_CREAT, FXF_TRUNC, FXF_EXCL from twisted.conch.ls import lsLine -from avatar import ConchUser? -from error import ConchError? -from interfaces import ISession, ISFTPServer, ISFTPFile +from twisted.conch.avatar import ConchUser? +from twisted.conch.error import ConchError? +from twisted.conch.interfaces import ISession, ISFTPServer, ISFTPFile +from twisted.conch import ttymodes import struct, os, time, socket import fcntl, tty import pwd, grp import pty -import ttymodes try: import utmp @@ -48,9 +49,6 @@ {"session": session.SSHSession, "direct-tcpip": forwarding.openConnectForwardingClient}) - self.subsystemLookup.update( - {"sftp": filetransfer.FileTransferServer?}) - def getUserGroupId(self): return self.pwdData[2:4] @@ -67,10 +65,10 @@ hostToBind, portToBind = forwarding.unpackGlobal_tcpip_forward(data) from twisted.internet import reactor try: listener = self._runAsUser( - reactor.listenTCP, portToBind, + reactor.listenTCP, portToBind, forwarding.SSHListenForwardingFactory(self.conn, (hostToBind, portToBind), - forwarding.SSHListenServerForwardingChannel), + forwarding.SSHListenServerForwardingChannel), interface = hostToBind) except: return 0 @@ -95,7 +93,8 @@ # remove all listeners for listener in self.listeners.itervalues(): self._runAsUser(listener.stopListening) - log.msg('avatar %s logging out (%i)' % (self.username, len(self.listeners))) + log.msg('avatar %s logging out (%i)' % (self.username, + len(self.listeners))) def _runAsUser(self, f, *args, **kw): euid = os.geteuid() @@ -160,7 +159,6 @@ b = utmp.UtmpRecord(utmp.WTMP_FILE) b.pututline(entry) b.endutent() - def getPty(self, term, windowSize, modes): self.environTERM? = term @@ -168,9 +166,13 @@ self.modes = modes master, slave = pty.openpty() ttyname = os.ttyname(slave) - self.environSSH_TTY? = ttyname + self.environSSH_TTY? = ttyname self.ptyTuple = (master, slave, ttyname) + def lookupSubsystem(subsystem, data): + if subsystem == 'sftp': + return filetransfer.FileTransferServer(data) + def openShell(self, proto): from twisted.internet import reactor if not self.ptyTuple: # we didn't get a pty-req @@ -191,7 +193,7 @@ shell, ['-%s' % shellExec], self.environ, homeDir, uid, gid, usePTY = self.ptyTuple) self.addUTMPEntry() - fcntl.ioctl(self.pty.fileno(), tty.TIOCSWINSZ, + fcntl.ioctl(self.pty.fileno(), tty.TIOCSWINSZ, struct.pack('4H', *self.winSize)) if self.modes: self.setModes() @@ -232,7 +234,7 @@ finally: os.setegid(egid) os.seteuid(euid) - + def setModes(self): pty = self.pty attr = tty.tcgetattr(pty.fileno()) @@ -276,7 +278,7 @@ def windowChanged(self, winSize): self.winSize = winSize - fcntl.ioctl(self.pty.fileno(), tty.TIOCSWINSZ, + fcntl.ioctl(self.pty.fileno(), tty.TIOCSWINSZ, struct.pack('4H', *self.winSize)) def _writeHack(self, data):

Here the diff ends, what follows is the added test file.

=== twisted/conch/test/test_session.py === # Copyright (c) 2007-2008 Twisted Matrix Laboratories. # See LICENSE for details. """ Tests for the 'session' channel implementation in twisted.conch.ssh.session. """

This is a good place to refer to the RFC.

import os, signal, sys, struct from zope.interface import implements from twisted.conch.ssh import common, connection, session from twisted.internet import defer, protocol, error from twisted.python import components, failure from twisted.trial import unittest class SubsystemOnlyAvatar(object): """ A stub class representing an avatar that is only useful for getting a subsystem. """ def lookupSubsystem(self, name, data): """ If the other side requests the 'subsystem' subsystem, allow it by returning a MockProcessProtocol? to implement it. Otherwise, fail. """ if name == 'subsystem': return MockProcessProtocol()

When I read 'fail', I think 'raise some sort of error', not 'return None'. The docstring would be more helpful if it explained the impact of returning None in this case.

class StubOldAvatar(object): """ A stub class representing the avatar representing the authenticated user. """

Please explain why it's 'Old'.

def lookupSubsystem(self, name, data): """ If the user requests the TestSubsystem? subsystem, connect them to our MockProcessProtocol?. If they request the protocol subsystem, connect them to a MockProtocol?. """ if name == 'TestSubsystem?': self.subsystem = MockProcessProtocol() self.subsystem.packetData = data return self.subsystem elif name == 'protocol': self.subsystem = MockProtocol() self.subsystem.packetData = data return self.subsystem

And if they request neither, what happens?

class StubSessionForStubOldAvatar(object): """ A stub ISession implementation for our StubOldAvatar?. @ivar avatar: the C{StubOldAvatar?} we are adapting. @ivar ptyRequest: if present, the terminal, window size, and modes passed to the getPty method. @ivar windowChange: if present, the window size passed to the windowChangned method. @ivar shellProtocol: if present, the C{SSHSessionProcessProtocol} passed to the openShell method. @ivar shellTransport: if present, the C{EchoTransport?} connected to shellProtocol. @ivar execProtocol: if present, the C{SSHSessionProcessProtocol} passed to the execCommand method. @ivar execTransport: if present, the C{EchoTransport?} connected to execProtocol. @ivar execCommandLine: if present, the command line passed to the execCommand method. @ivar gotEOF: if present, an EOF message was received. @ivar gotClosed: if present, a closed message was received. """

I guess you have all of those here so you can manipulate them from tests.

Looking down, I guessed wrong. These are like a log of the things that have been done to this session object. It's worth making a general comment about this in the docstring.

You should also mention that certain calls to certain methods will trigger errors.

Note that a more usual way of doing this is to have a 'log' or 'calls' attribute that appends information about the calls to a list. No need to change it though.

implements(session.ISession) def init(self, avatar): """ Store the avatar we're adapting. """ self.avatar = avatar self.shellProtocol = None def getPty(self, terminal, window, modes): """ If the terminal is 'bad', fail. Otherwise, store the information in the ptyRequest variable. """ if terminal != 'bad': self.ptyRequest = (terminal, window, modes) else: raise RuntimeError('not getting a pty') def windowChanged(self, window): """ If all the window sizes are 0, fail. Otherwise, store the size in the windowChange variable. """ if window == (0, 0, 0, 0): raise RuntimeError('not changing the window size') else: self.windowChange = window def openShell(self, pp): """ If we have gotten a shell request before, fail. Otherwise, store the process protocol in the shellProtocol variable, connect it to the EchoTransport? and store that as shellTransport. """ if self.shellProtocol is not None: raise RuntimeError('not getting a shell this time') else: self.shellProtocol = pp self.shellTransport = EchoTransport(pp) def execCommand(self, pp, command): """ If the command is 'true', store the command, the process protocol, and the transport we connect to the process protocol. Otherwise, just store the command and raise an error. """ self.execCommandLine = command if command == 'true': self.execProtocol = pp elif command[:4] == 'echo': self.execProtocol = pp self.execTransport = EchoTransport(pp) pp.outReceived(command[5:]) else: raise RuntimeError('not getting a command') def eofReceived(self): """ Note that EOF has been received. """ self.gotEOF = True def closed(self): """ Note that close has been received. """ self.gotClosed = True components.registerAdapter(StubSessionForStubOldAvatar?, StubOldAvatar?, session.ISession) class StubNewAvatar(object): """ A stub avatar. It does not need any methods, as it is only used as the object StubSessionForStubNewAvatar? adapts. """ class StubApplicationFactoryForStubNewAvatar?: """ A stub ISessionApplicationFactory implementation for our StubNewAvatar?. @ivar avatar: the C{StubOldAvatar?} we are adapting. @ivar inConnectionOpen: C{True} if the input side is open. @ivar outConnectionOpen: C{True} if the output side is open. @ivar ended: C{True} if the session has ended. @ivar subsystem: if present, the C{MockApplication?} that is the current subsystem. It has a packetData ivar which is the C{str} of data passed in the subsystem request. @ivar term: if present, the terminal name passed to getPty @ivar windowSize: if present, the window size passed to getPty or windowChanged @ivar modes: if present, the terminal modes passed to getPty @ivar command: if present, the C{MockApplication?} that is the current command. It has a command ivar which is the C{str} giving the command's name. @ivar shell: if present, the C{MockApplication?} that is the current shell. """ implements(session.ISessionApplicationFactory) def init(self, avatar): self.avatar = avatar self.inConnectionOpen = True self.outConnectionOpen = True self.ended = False self.subsystem = None self.command = None self.shell = None def lookupSubsystem(self, subsystem, data): """ Request a subsystem. If one has been requested already, raise an exception. Otherwise, set self.subsystem to a MockApplication? and return it. """ if self.subsystem is not None: raise ValueError('already opened a subsystem') if subsystem == 'echo': self.subsystem = MockApplication() self.subsystem.packetData = data return self.subsystem def getPTY(self, term, windowSize, modes): """ Request a psuedoterminal. Store the data passed to us. """

"pseudo", not "psuedo".

self.term = term self.windowSize = windowSize self.modes = modes def windowChanged(self, newSize): """ The window size has changed. Store the data. """ self.windowSize = newSize def execCommand(self, command): """ Request a command. If one has been requested already, raise an exception. Otherwise, set self.command to