Merge lp:~julian-edwards/launchpad/twisted-ftp-poppy into lp:launchpad

[Julian Edwards]

[testfix] [r=jml,leonardr][bug=251685,586695] An implementation of the Poppy FTP server using Twisted.

= Summary =
Extend poppy-sftp to also handle ftp.

== Proposed fix ==
Package uploads are currently handled either in Poppy (FTP) or Poppy-sftp
(SFTP). The former is written using Zope's FTP server, the latter using
Twisted.

The Zope FTP server has a number of bugs, so this branch adds FTP support via
the Twisted libraries.

== Pre-implementation notes ==
None, I suck.

== Implementation details ==
Some interesting points:

 * Poppy doesn't really care about authentication, it lets any credentials
upload files, whether it be anonymous or otherwise. Twisted has a special
mode where if anyone logs in as "anonymous" it will prevent any file uploads.
This behaviour is hard-coded in the base FTPShell class. To get around this,
the FTPFactory's "userAnonymous" is modified to a user called
"userthatwillneverhappen" so that the read-only mode is not encountered for
anonymous access.

 * The old port number was passed on the command line, it's now set in config.

 * The tests were easy to fix since they just needed the PoppyTac fixture
swapped in in favour of the old PoppyTestSetup.

 * The old Poppy had a file mode bug where it set group execute on uploaded
files, this is reflected in the test changes for the new ftp service.

 * The poppy server does post-processing of the upload. There's no way to
know when this is finished, so the test just sleeps for a few seconds. See
bug 586695. The old poppy code had some weird hook to print output to stdout
when it finished uploading, we might need to do that here too.

== Tests ==

bin/test -cvv test_poppy

== Demo and Q/A ==

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/poppy/twistedftp.py
  lib/canonical/config/schema-lazr.conf
  lib/lp/poppy/tests/test_poppy.py
  daemons/poppy-sftp.tac

./lib/canonical/config/schema-lazr.conf
     522: Line exceeds 78 characters.
    1034: Line exceeds 78 characters.

[Leonard Richardson (leonardr)]

The existing test coverage looks good, which helps compensate for my relative ignorance around this part of the code.

So if someone actually logs in as "userthatwillneverhappen", they won't be able to upload any files? I don't think this is a big deal, just trying to understand what's going on. What would happen if you set that to None?

If you made 0102674 a constant you wouldn't have to explain it twice.

review: Approve

[Jonathan Lange (jml)]

On Tue, Feb 22, 2011 at 10:22 AM, Julian Edwards
<email address hidden> wrote:
> Julian Edwards has proposed merging lp:~julian-edwards/launchpad/twisted-ftp-poppy into lp:launchpad.
>
> Requested reviews:
>  Launchpad code reviewers (launchpad-reviewers)
>
> For more details, see:
> https://code.launchpad.net/~julian-edwards/launchpad/twisted-ftp-poppy/+merge/50721
>
> = Summary =
> Extend poppy-sftp to also handle ftp.
>
> == Proposed fix ==
> Package uploads are currently handled either in Poppy (FTP) or Poppy-sftp
> (SFTP).  The former is written using Zope's FTP server, the latter using
> Twisted.
>
> The Zope FTP server has a number of bugs, so this branch adds FTP support via
> the Twisted libraries.
>
> == Pre-implementation notes ==
> None, I suck.
>
> == Implementation details ==
> Some interesting points:
>
>  * Poppy doesn't really care about authentication, it lets any credentials
> upload files, whether it be anonymous or otherwise.  Twisted has a special
> mode where if anyone logs in as "anonymous" it will prevent any file uploads.
> This behaviour is hard-coded in the base FTPShell class.  To get around this,
> the FTPFactory's "userAnonymous" is modified to a user called
> "userthatwillneverhappen" so that the read-only mode is not encountered for
> anonymous access.
>

...
>  * The poppy server does post-processing of the upload.  There's no way to
> know when this is finished, so the test just sleeps for a few seconds.  See
> bug 586695.  The old poppy code had some weird hook to print output to stdout
> when it finished uploading, we might need to do that here too.

> === modified file 'daemons/poppy-sftp.tac'
> --- daemons/poppy-sftp.tac      2011-02-02 01:24:33 +0000
> +++ daemons/poppy-sftp.tac      2011-02-22 10:21:46 +0000
> @@ -7,10 +7,11 @@
>
>  import os
>
> -from twisted.application import service
> +from twisted.application import service, strports
>  from twisted.conch.interfaces import ISession
>  from twisted.conch.ssh import filetransfer
>  from twisted.cred.portal import IRealm, Portal
> +from twisted.protocols import ftp
>  from twisted.protocols.policies import TimeoutFactory
>  from twisted.python import components
>  from twisted.web.xmlrpc import Proxy
> @@ -20,6 +21,10 @@
>  from canonical.config import config
>  from canonical.launchpad.daemons import readyservice
>
> +from lp.poppy.twistedftp import (
> +    FTPRealm,
> +    PoppyAccessCheck,
> +    )
>  from lp.poppy.twistedsftp import SFTPServer
>  from lp.services.sshserver.auth import (
>     LaunchpadAvatar, PublicKeyFromLaunchpadChecker)
> @@ -85,9 +90,41 @@
>  components.registerAdapter(DoNothingSession, LaunchpadAvatar, ISession)
>
>
> -# Construct an Application that has the Poppy SSH server.
> +class FTPServiceFactory(service.Service):

This class should go in lp/poppy, and be imported by the tac file.
That way it can be re-used and unit tested.

>
> === modified file 'lib/canonical/config/schema-lazr.conf'
> --- lib/canonical/config/schema-lazr.conf       2011-02-21 18:54:53 +0000
> +++ lib/canonical/config/schema-lazr.conf       2011-02-22 10:21:46 +0000
> @@ -1654,6 +1654,9 @@
>  # datatype: string
>  port: tcp:5022
>
> +# data...

[Jonathan Lange (jml)]

Here's what I'd do to get rid of the timeouts:
  * Add a line at the end of Hooks.client_done_hook that logs a magic string to the Twisted logger.
  * Have the fixtures watch the poppytacfixture.logger file, as per tachandler _hasDaemonStarted and _waitForDaemonStartup
  * If you are shy about adding things like that to the log, use an env var to guard it

Times in tests lead to tests that are either needlessly slow or that fail intermittently. To me this is a greater evil than a few more lines of code and tainting the largely theoretical purity of non-test code.

[Julian Edwards (julian-edwards)]

Right, thanks for the review both of you.

I've made a ton of changes, pretty much everything you mentioned, apart from the strports thing since I want to fix the sftp config to have a plain integer, but that needs production config changes in tandem and I don't need the hassle of that with this branch. I'll fix it later.

Let me know how it looks now.

Cheers.

[Jonathan Lange (jml)]

Looks good. Line 261 of the diff has a typo ("FT Pserve") and using Hooks._targetcount is preferable to using a global.

review: Approve

[Julian Edwards (julian-edwards)]

On Wednesday 23 February 2011 13:11:56 you wrote:
> Review: Approve
> Looks good. Line 261 of the diff has a typo ("FT Pserve") and using

Thanks, it was a stray paste in vim's command mode wot did it.

> Hooks._targetcount is preferable to using a global.

That makes it effectively non-static again...?

[Jonathan Lange (jml)]

On Wed, Feb 23, 2011 at 2:22 PM, Julian Edwards
<email address hidden> wrote:
> On Wednesday 23 February 2011 13:11:56 you wrote:
...
>> Hooks._targetcount is preferable to using a global.
>
> That makes it effectively non-static again...?
>

No, it makes it a class variable rather than an instance variable.

Hooks._targetcount is another way of saying self.__class__._targetcount.

jlm

=== modified file 'daemons/poppy-sftp.tac'
--- daemons/poppy-sftp.tac	2011-02-02 01:24:33 +0000
+++ daemons/poppy-sftp.tac	2011-02-24 10:45:01 +0000
@@ -5,7 +5,7 @@
 #     twistd -noy sftp.tac
 # or similar.  Refer to the twistd(1) man page for details.
 
-import os
+import logging
 
 from twisted.application import service
 from twisted.conch.interfaces import ISession
@@ -20,15 +20,16 @@
 from canonical.config import config
 from canonical.launchpad.daemons import readyservice
 
+from lp.poppy import get_poppy_root
+from lp.poppy.twistedftp import (
+    FTPServiceFactory,
+    )
 from lp.poppy.twistedsftp import SFTPServer
 from lp.services.sshserver.auth import (
     LaunchpadAvatar, PublicKeyFromLaunchpadChecker)
 from lp.services.sshserver.service import SSHService
 from lp.services.sshserver.session import DoNothingSession
 
-# XXX: Rename this file to something that doesn't mention poppy. Talk to
-# bigjools.
-
 
 def make_portal():
     """Create and return a `Portal` for the SSH service.
@@ -63,31 +64,30 @@
         return deferred.addCallback(got_user_dict)
 
 
-def get_poppy_root():
-    """Return the poppy root to use for this server.
-
-    If the POPPY_ROOT environment variable is set, use that. If not, use
-    config.poppy.fsroot.
-    """
-    poppy_root = os.environ.get('POPPY_ROOT', None)
-    if poppy_root:
-        return poppy_root
-    return config.poppy.fsroot
-
-
 def poppy_sftp_adapter(avatar):
     return SFTPServer(avatar, get_poppy_root())
 
 
+# Connect Python logging to Twisted's logging.
+from lp.services.twistedsupport.loggingsupport import set_up_tacfile_logging
+set_up_tacfile_logging("poppy-sftp", logging.INFO)
+
+
 components.registerAdapter(
     poppy_sftp_adapter, LaunchpadAvatar, filetransfer.ISFTPServer)
 
 components.registerAdapter(DoNothingSession, LaunchpadAvatar, ISession)
 
 
-# Construct an Application that has the Poppy SSH server.
+# ftpport defaults to 2121 in schema-lazr.conf
+ftpservice = FTPServiceFactory.makeFTPService(port=config.poppy.ftp_port)
+
+# Construct an Application that has the Poppy SSH server,
+# and the Poppy FTP server.
 application = service.Application('poppy-sftp')
 
+ftpservice.setServiceParent(application)
+
 def timeout_decorator(factory):
     """Add idle timeouts to a factory."""
     return TimeoutFactory(factory, timeoutPeriod=config.poppy.idle_timeout)

=== modified file 'lib/canonical/config/schema-lazr.conf'
--- lib/canonical/config/schema-lazr.conf	2011-02-21 18:54:53 +0000
+++ lib/canonical/config/schema-lazr.conf	2011-02-24 10:45:01 +0000
@@ -1654,6 +1654,9 @@
 # datatype: string
 port: tcp:5022
 
+# datatype: string
+ftp_port: 2121
+
 # See [error_reports].
 copy_to_zlog: False
 

=== modified file 'lib/lp/poppy/__init__.py'
--- lib/lp/poppy/__init__.py	2009-06-25 05:39:50 +0000
+++ lib/lp/poppy/__init__.py	2011-02-24 10:45:01 +0000
@@ -1,4 +1,20 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # Make this directory into a Python package.
+
+import os
+
+from canonical.config import config
+
+
+def get_poppy_root():
+    """Return the poppy root to use for this server.
+
+    If the POPPY_ROOT environment variable is set, use that. If not, use
+    config.poppy.fsroot.
+    """
+    poppy_root = os.environ.get('POPPY_ROOT', None)
+    if poppy_root:
+        return poppy_root
+    return config.poppy.fsroot

=== modified file 'lib/lp/poppy/hooks.py'
--- lib/lp/poppy/hooks.py	2010-08-20 20:31:18 +0000
+++ lib/lp/poppy/hooks.py	2011-02-24 10:45:01 +0000
@@ -3,6 +3,12 @@
 
 __metaclass__ = type
 
+__all__ = [
+    'Hooks',
+    'PoppyInterfaceFailure',
+    ]
+
+
 import logging
 import os
 import shutil
@@ -19,6 +25,8 @@
 class Hooks:
 
     clients = {}
+    LOG_MAGIC = "Post-processing finished"
+    _targetcount = 0
 
     def __init__(self, targetpath, logger, allow_user, cmd=None,
                  targetstart=0, perms=None, prefix=''):
@@ -26,10 +34,15 @@
         self.logger = logging.getLogger("%s.Hooks" % logger.name)
         self.cmd = cmd
         self.allow_user = allow_user
-        self.targetcount = targetstart
         self.perms = perms
         self.prefix = prefix
 
+    @property
+    def targetcount(self):
+        """A guaranteed unique integer for ensuring unique upload dirs."""
+        Hooks._targetcount += 1
+        return Hooks._targetcount
+
     def new_client_hook(self, fsroot, host, port):
         """Prepare a new client record indexed by fsroot..."""
         self.clients[fsroot] = {
@@ -80,7 +93,6 @@
             pass
 
         try:
-            self.targetcount += 1
             timestamp = time.strftime("%Y%m%d-%H%M%S")
             path = "upload%s-%s-%06d" % (
                 self.prefix, timestamp, self.targetcount)
@@ -122,6 +134,9 @@
             self.lock.release(skip_delete=True)
 
         self.clients.pop(fsroot)
+        # This is mainly done so that tests know when the
+        # post-processing hook has finished.
+        self.logger.info(self.LOG_MAGIC)
 
     def auth_verify_hook(self, fsroot, user, password):
         """Verify that the username matches a distribution we care about.
@@ -146,3 +161,4 @@
         #except object, e:
         #    print e
         #return False
+

=== modified file 'lib/lp/poppy/tests/test_poppy.py'
--- lib/lp/poppy/tests/test_poppy.py	2010-11-07 12:10:38 +0000
+++ lib/lp/poppy/tests/test_poppy.py	2011-02-24 10:45:01 +0000
@@ -1,14 +1,13 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Functional tests for poppy FTP daemon."""
 
 __metaclass__ = type
 
-import ftplib
 import os
 import shutil
-import socket
+import stat
 import StringIO
 import tempfile
 import time
@@ -33,10 +32,10 @@
     ZopelessAppServerLayer,
     ZopelessDatabaseLayer,
     )
-from lp.poppy.tests.helpers import PoppyTestSetup
 from lp.registry.interfaces.ssh import (
     ISSHKeySet,
     )
+from lp.poppy.hooks import Hooks
 from lp.testing import TestCaseWithFactory
 
 
@@ -45,48 +44,35 @@
 
     def __init__(self, root_dir, factory):
         self.root_dir = root_dir
-        self.port = 3421
+        self.port = 2121
 
     def setUp(self):
         super(FTPServer, self).setUp()
-        self.poppy = PoppyTestSetup(
-            self.root_dir, port=self.port, cmd='echo CLOSED')
-        self.poppy.startPoppy()
-        self.addCleanup(self.poppy.killPoppy)
+        self.poppytac = self.useFixture(PoppyTac(self.root_dir))
+
+    def getAnonTransport(self):
+        return get_transport(
+            'ftp://anonymous:[e-mail]@localhost:%s/' % (self.port,))
 
     def getTransport(self):
         return get_transport('ftp://ubuntu:@localhost:%s/' % (self.port,))
 
     def disconnect(self, transport):
-        transport._get_connection().quit()
-
-    def _getFTPConnection(self):
-        # poppy usually takes sometime to come up, we need to wait, or insist.
-        conn = ftplib.FTP()
-        while True:
-            try:
-                conn.connect("localhost", self.port)
-            except socket.error:
-                if not self.poppy.alive:
-                    raise
-            else:
-                break
-        return conn
+        transport._get_connection().close()
 
     def waitForStartUp(self):
         """Wait for the FTP server to start up."""
-        conn = self._getFTPConnection()
-        conn.quit()
+        pass
 
-    def waitForClose(self):
+    def waitForClose(self, number=1):
         """Wait for an FTP connection to close.
 
-        Poppy is configured to echo 'CLOSED' to stdout when a
-        connection closes, so we wait for CLOSED to appear in its
+        Poppy is configured to echo 'Post-processing finished' to stdout
+        when a connection closes, so we wait for that to appear in its
         output as a way to tell that the server has finished with the
         connection.
         """
-        self.poppy.verify_output(['CLOSED'])
+        self.poppytac.waitForPostProcessing(number)
 
 
 class SFTPServer(Fixture):
@@ -126,7 +112,7 @@
     def setUp(self):
         super(SFTPServer, self).setUp()
         self.setUpUser('joe')
-        self.useFixture(PoppyTac(self.root_dir))
+        self.poppytac = self.useFixture(PoppyTac(self.root_dir))
 
     def disconnect(self, transport):
         transport._get_connection().close()
@@ -134,11 +120,8 @@
     def waitForStartUp(self):
         pass
 
-    def waitForClose(self):
-        # XXX: Steve Kowalik 2010-05-24 bug=586695 There has to be a
-        # better way to wait for the SFTP server to process our upload
-        # rather than sleeping for 10 seconds.
-        time.sleep(10)
+    def waitForClose(self, number=1):
+        self.poppytac.waitForPostProcessing(number)
 
     def getTransport(self):
         return get_transport('sftp://joe@localhost:%s/' % (self.port,))
@@ -184,6 +167,24 @@
     def pidfile(self):
         return os.path.join(self.root, 'poppy-sftp.pid')
 
+    def waitForPostProcessing(self, number=1):
+        now = time.time()
+        deadline = now + 20
+        while now < deadline and not self._hasPostProcessed(number):
+            time.sleep(0.1)
+            now = time.time()
+
+        if now >= deadline:
+            raise Exception("Poppy post-processing did not complete")
+
+    def _hasPostProcessed(self, number):
+        if os.path.exists(self.logfile):
+            with open(self.logfile, "r") as logfile:
+                occurrences = logfile.read().count(Hooks.LOG_MAGIC)
+                return occurrences >= number
+        else:
+            return False
+
 
 class TestPoppy(TestCaseWithFactory):
     """Test if poppy.py daemon works properly."""
@@ -204,14 +205,20 @@
         upload_dir = contents[1]
         return os.path.join(self.root_dir, upload_dir, path)
 
-    def test_change_directory(self):
+    def test_change_directory_anonymous(self):
+        # Check that FTP access with an anonymous user works.
+        transport = self.server.getAnonTransport()
+        self.test_change_directory(transport)
+
+    def test_change_directory(self, transport=None):
         """Check automatic creation of directories 'cwd'ed in.
 
         Also ensure they are created with proper permission (g+rwxs)
         """
         self.server.waitForStartUp()
 
-        transport = self.server.getTransport()
+        if transport is None:
+            transport = self.server.getTransport()
         transport.stat('foo/bar') # .stat will implicity chdir for us
 
         self.server.disconnect(transport)
@@ -269,7 +276,11 @@
         wanted_path = self._uploadPath('foo/bar/baz')
         fs_content = open(os.path.join(wanted_path)).read()
         self.assertEqual(fs_content, "fake contents")
-        self.assertEqual(os.stat(wanted_path).st_mode, 0102674)
+        # Expected mode is -rw-rwSr--.
+        self.assertEqual(
+            os.stat(wanted_path).st_mode,
+            stat.S_IROTH | stat.S_ISGID | stat.S_IRGRP | stat.S_IWGRP
+            | stat.S_IWUSR | stat.S_IRUSR | stat.S_IFREG)
 
     def test_full_source_upload(self):
         """Check that the connection will deal with multiple files being
@@ -298,13 +309,17 @@
         if transport._user == 'joe':
             self.assertEqual(dir_name.startswith('upload-sftp-2'), True)
         elif transport._user == 'ubuntu':
-            self.assertEqual(dir_name.startswith('upload-2'), True)
+            self.assertEqual(dir_name.startswith('upload-ftp-2'), True)
         for upload in files:
             wanted_path = self._uploadPath(
                 "~ppa-user/ppa/ubuntu/%s" % upload)
             fs_content = open(os.path.join(wanted_path)).read()
             self.assertEqual(fs_content, upload)
-            self.assertEqual(os.stat(wanted_path).st_mode, 0102674)
+            # Expected mode is -rw-rwSr--.
+            self.assertEqual(
+                os.stat(wanted_path).st_mode,
+                stat.S_IROTH | stat.S_ISGID | stat.S_IRGRP | stat.S_IWGRP
+                | stat.S_IWUSR | stat.S_IRUSR | stat.S_IFREG)
 
     def test_upload_isolation(self):
         """Check if poppy isolates the uploads properly.
@@ -319,13 +334,11 @@
         fake_file = StringIO.StringIO("ONE")
         conn_one.put_file('test', fake_file, mode=None)
         self.server.disconnect(conn_one)
-        self.server.waitForClose()
 
         conn_two = self.server.getTransport()
         fake_file = StringIO.StringIO("TWO")
         conn_two.put_file('test', fake_file, mode=None)
         self.server.disconnect(conn_two)
-        self.server.waitForClose()
 
         # Perform a pair of sessions with simultaneous connections.
         conn_three = self.server.getTransport()
@@ -338,10 +351,11 @@
         conn_four.put_file('test', fake_file, mode=None)
 
         self.server.disconnect(conn_three)
-        self.server.waitForClose()
 
         self.server.disconnect(conn_four)
-        self.server.waitForClose()
+
+        # Wait for the 4 uploads to finish.
+        self.server.waitForClose(4)
 
         # Build a list of directories representing the 4 sessions.
         upload_dirs = [leaf for leaf in sorted(os.listdir(self.root_dir))
@@ -372,4 +386,4 @@
     # SFTP doesn't have the concept of the server changing directories, since
     # clients will only send absolute paths, so drop that test.
     return exclude_tests_by_condition(
-        suite, condition_id_re(r'test_change_directory\(sftp\)$'))
+        suite, condition_id_re(r'test_change_directory.*\(sftp\)$'))

=== modified file 'lib/lp/poppy/tests/test_twistedsftp.py'
--- lib/lp/poppy/tests/test_twistedsftp.py	2010-07-01 19:13:11 +0000
+++ lib/lp/poppy/tests/test_twistedsftp.py	2011-02-24 10:45:01 +0000
@@ -51,7 +51,7 @@
         test_file = open(file_name, 'r')
         self.assertEqual(test_file.read(), "This is a test")
         test_file.close()
-        self.assertEqual(os.stat(file_name).st_mode, 0100654)
+        self.assertEqual(os.stat(file_name).st_mode, 0100644)
         dir_name = os.path.join(self.sftp_server._current_upload, 'bar/foo')
         os.makedirs(dir_name)
         upload_file = self.sftp_server.openFile('bar/foo', None, None)

=== added file 'lib/lp/poppy/twistedftp.py'
--- lib/lp/poppy/twistedftp.py	1970-01-01 00:00:00 +0000
+++ lib/lp/poppy/twistedftp.py	2011-02-24 10:45:01 +0000
@@ -0,0 +1,152 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Twisted FTP implementation of the Poppy upload server."""
+
+__metaclass__ = type
+__all__ = [
+    'FTPRealm',
+    'PoppyAnonymousShell',
+    ]
+
+import logging
+import os
+import tempfile
+
+from twisted.application import service, strports
+from twisted.cred import checkers, credentials
+from twisted.cred.portal import IRealm, Portal
+from twisted.internet import defer
+from twisted.protocols import ftp
+from twisted.python import filepath
+
+from zope.interface import implements
+
+from canonical.config import config
+from lp.poppy import get_poppy_root
+from lp.poppy.filesystem import UploadFileSystem
+from lp.poppy.hooks import Hooks
+
+
+class PoppyAccessCheck:
+    """An `ICredentialsChecker` for Poppy FTP sessions."""
+    implements(checkers.ICredentialsChecker)
+    credentialInterfaces = (
+        credentials.IUsernamePassword, credentials.IAnonymous)
+
+    def requestAvatarId(self, credentials):
+        # Poppy allows any credentials.  People can use "anonymous" if
+        # they want but anything goes.  Thus, we don't actually *check* the
+        # credentials, and we return the standard avatarId for 'anonymous'.
+        return checkers.ANONYMOUS
+
+
+class PoppyAnonymousShell(ftp.FTPShell):
+    """The 'command' interface for sessions.
+
+    Roughly equivalent to the SFTPServer in the sftp side of things.
+    """
+
+    def __init__(self, fsroot):
+        self._fs_root = fsroot
+        self.uploadfilesystem = UploadFileSystem(tempfile.mkdtemp())
+        self._current_upload = self.uploadfilesystem.rootpath
+        os.chmod(self._current_upload, 0770)
+        self._log = logging.getLogger("poppy-sftp")
+        self.hook = Hooks(
+            self._fs_root, self._log, "ubuntu", perms='g+rws',
+            prefix='-ftp')
+        self.hook.new_client_hook(self._current_upload, 0, 0)
+        self.hook.auth_verify_hook(self._current_upload, None, None)
+        super(PoppyAnonymousShell, self).__init__(
+            filepath.FilePath(self._current_upload))
+
+    def openForWriting(self, file_segments):
+        """Write the uploaded file to disk, safely.
+
+        :param file_segments: A list containing string items, one for each
+            path component of the file being uploaded.  The file referenced
+            is relative to the temporary root for this session.
+
+        If the file path contains directories, we create them.
+        """
+        filename = os.sep.join(file_segments)
+        self._create_missing_directories(filename)
+        return super(PoppyAnonymousShell, self).openForWriting(file_segments)
+
+    def makeDirectory(self, path):
+        """Make a directory using the secure `UploadFileSystem`."""
+        path = os.sep.join(path)
+        return defer.maybeDeferred(self.uploadfilesystem.mkdir, path)
+
+    def access(self, segments):
+        """Permissive CWD that auto-creates target directories."""
+        if segments:
+            path = self._path(segments)
+            path.makedirs()
+        return super(PoppyAnonymousShell, self).access(segments)
+
+    def logout(self):
+        """Called when the client disconnects.
+
+        We need to post-process the upload.
+        """
+        self.hook.client_done_hook(self._current_upload, 0, 0)
+
+    def _create_missing_directories(self, filename):
+        # Same as SFTPServer
+        new_dir, new_file = os.path.split(
+            self.uploadfilesystem._sanitize(filename))
+        if new_dir != '':
+            if not os.path.exists(
+                os.path.join(self._current_upload, new_dir)):
+                self.uploadfilesystem.mkdir(new_dir)
+
+    def list(self, path_segments, attrs):
+        return defer.fail(ftp.CmdNotImplementedError("LIST"))
+
+
+class FTPRealm:
+    """FTP Realm that lets anyone in."""
+    implements(IRealm)
+
+    def __init__(self, root):
+        self.root = root
+
+    def requestAvatar(self, avatarId, mind, *interfaces):
+        """Return a Poppy avatar - that is, an "authorisation".
+
+        Poppy FTP avatars are totally fake, we don't care about credentials.
+        See `PoppyAccessCheck` above.
+        """
+        for iface in interfaces:
+            if iface is ftp.IFTPShell:
+                avatar = PoppyAnonymousShell(self.root)
+                return ftp.IFTPShell, avatar, getattr(
+                    avatar, 'logout', lambda: None)
+        raise NotImplementedError(
+            "Only IFTPShell interface is supported by this realm")
+
+
+class FTPServiceFactory(service.Service):
+    """A factory that makes an `FTPService`"""
+
+    def __init__(self, port):
+        realm = FTPRealm(get_poppy_root())
+        portal = Portal(realm)
+        portal.registerChecker(PoppyAccessCheck())
+        factory = ftp.FTPFactory(portal)
+
+        factory.tld = get_poppy_root()
+        factory.protocol = ftp.FTP
+        factory.welcomeMessage = "Launchpad upload server"
+        factory.timeOut = config.poppy.idle_timeout
+
+        self.ftpfactory = factory
+        self.portno = port
+
+    @staticmethod
+    def makeFTPService(port=2121):
+        strport = "tcp:%s" % port
+        factory = FTPServiceFactory(port)
+        return strports.service(strport, factory.ftpfactory)

=== modified file 'lib/lp/poppy/twistedsftp.py'
--- lib/lp/poppy/twistedsftp.py	2010-08-20 20:31:18 +0000
+++ lib/lp/poppy/twistedsftp.py	2011-02-24 10:45:01 +0000
@@ -22,7 +22,6 @@
     adapter,
     provideHandler,
     )
-import zope.component.event
 from zope.interface import implements
 
 from lp.poppy.filesystem import UploadFileSystem
@@ -129,7 +128,7 @@
     def writeChunk(self, offset, data):
         try:
             chunk_file = os.open(
-                self.filename, os.O_CREAT | os.O_WRONLY, 0674)
+                self.filename, os.O_CREAT | os.O_WRONLY, 0664)
         except OSError, e:
             if e.errno != errno.EISDIR:
                 raise
@@ -143,4 +142,3 @@
 
     def setAttrs(self, attr):
         pass
-

=== modified file 'lib/lp/services/twistedsupport/loggingsupport.py'
--- lib/lp/services/twistedsupport/loggingsupport.py	2010-08-20 20:31:18 +0000
+++ lib/lp/services/twistedsupport/loggingsupport.py	2011-02-24 10:45:01 +0000
@@ -12,6 +12,7 @@
     'log_oops_from_failure',
     'set_up_logging_for_script',
     'set_up_oops_reporting',
+    'set_up_tacfile_logging',
     ]
 
 
@@ -75,6 +76,25 @@
     return logger_object
 
 
+def set_up_tacfile_logging(name, level):
+    """Create a `Logger` object for use in tac files.
+
+    This is preferable to use over `set_up_logging_for_script` for .tac
+    files since there's no options to pass through.  The logger object
+    is connected to Twisted's log and returned.
+
+    :param name: The logger instance name.
+    :param level: The log level to use, eg. logging.INFO or logging.DEBUG
+    """
+    logger = logging.getLogger(name)
+    channel = logging.StreamHandler(log.StdioOnnaStick())
+    channel.setLevel(level)
+    channel.setFormatter(logging.Formatter('%(message)s'))
+    logger.addHandler(channel)
+    logger.setLevel(level)
+    return logger
+
+
 def set_up_oops_reporting(configuration, name, mangle_stdout=True):
     """Set up OOPS reporting by starting the Twisted logger with an observer.
 
@@ -188,6 +208,7 @@
         self.request_count += 1
         self.logger.log(
             self.level, 'Sending request [%d]: %s%s', request, method, args)
+
         def _logResult(result):
             self.logger.log(
                 self.level, 'Reply to request [%d]: %s', request, result)