Merge lp:~spiv/bzr/bzr-ssh-homedir-take-3 into lp:bzr

[Andrew Bennetts]

[Andrew Bennetts (spiv)]

This fixes bug 109143. It implements expansion of /~/... and /~user/... paths in the smart server. With this patch, the expansion will Just Work.

The core of the implementation is to use a PathFilteringTransport decorator (see <lp:~spiv/bzr/path-filtering-chroot>) to invoke os.path.expanduser.

This patch takes care to make sure this interacts sanely with edge cases like "bzr serve --directory=/home" ("~andrew" should not cause access to /home/home/andrew) and "bzr serve --directory=/nothome" (a client using ~andrew should not break out of the chroot that doesn't include /home/andrew).

I did a large refactoring of bzr_serve to break the work in to multiple functions, and then made those functions methods on a class to share the relevant state between them. This made it possible to adequately unit test the key parts of the new code. We can probably do better here, but I think that would involve breaking the public interface of transport_server_registry. e.g. it would be nice to avoid contortions to extract the original --directory from the command-line from the transport; ideally cmd_serve would have passed that to server factory more directly. Similarly, bzr-svn probably should be able to reuse the logic to change the default lock timeout and ui... This change is a good step in the right direction though, and doesn't break any APIs used by e.g. bzr-svn.

I've added some unit tests to blackbox.test_serve, which feels a little bit weird. But they are unit tests for the "bzr serve" UI, so this seems like the best home we have for that sort of thing.

[Martin Pool (mbp)]

Thanks, that's great to have this finished off, and it's nice that this
covers all those cases and apparently makes the code clearer to boot.

I'm stopping for today but I'll try to read the rest of it tomorrow.

This needs a NEWS entry and possibly a small update to the user manual
just to say that it's possible.

=== modified file 'bzrlib/smart/server.py'
--- bzrlib/smart/server.py 2009-07-20 11:27:05 +0000
+++ bzrlib/smart/server.py 2009-09-16 04:42:12 +0000
@@ -17,6 +17,7 @@
 """Server for smart-server protocol."""

 import errno
+import os.path
 import socket
 import sys
 import threading
@@ -30,6 +31,14 @@
 from bzrlib.lazy_import import lazy_import
 lazy_import(globals(), """
 from bzrlib.smart import medium
+from bzrlib.transport import (
+ chroot,
+ get_transport,
+ pathfilter,
+ )
+from bzrlib import (
+ urlutils,
+ )
 """)

@@ -313,34 +322,125 @@
         return transport.get_transport(url)

-def serve_bzr(transport, host=None, port=None, inet=False):
- from bzrlib import lockdir, ui
- from bzrlib.transport import get_transport
- from bzrlib.transport.chroot import ChrootServer
- chroot_server = ChrootServer(transport)
- chroot_server.setUp()
- transport = get_transport(chroot_server.get_url())
- if inet:
- smart_server = medium.SmartServerPipeStreamMedium(
- sys.stdin, sys.stdout, transport)
     else:
- if host is None:
- host = medium.BZR_DEFAULT_INTERFACE
- if port is None:
- port = medium.BZR_DEFAULT_PORT
- smart_server = SmartTCPServer(transport, host=host, port=port)
- trace.note('listening on port: %s' % smart_server.port)
- # For the duration of this server, no UI output is permitted. note
- # that this may cause problems with blackbox tests. This should be
- # changed with care though, as we dont want to use bandwidth sending
- # progress over stderr to smart server clients!
- old_factory = ui.ui_factory
- old_lockdir_timeout = lockdir._DEFAULT_TIMEOUT_SECONDS
- try:
+def _local_path_for_transport(transport):
+ """Return a local path for transport, if reasonably possible.
+
+ This function works even if transport's url has a "readonly+" prefix,
+ unlike local_path_from_url.
+
+ This essentially recovers the --directory argument the user passed to "bzr
+ serve" from the transport passed to serve_bzr.
+ """
+ try:
+ base_url = transport.external_url()
+ except (errors.InProcessTransport, NotImplementedError):
+ return None
     else:
+ # Strip readonly prefix
+ if base_url.startswith('readonly+'):
+ base_url = base_url[len('readonly+'):]
+ try:
+ return urlutils.local_path_from_url(base_url)
+ except errors.InvalidURL:
+ return None

(Not brilliant diff alignment there so I tweaked it for clarity.)

I wonder if that method should be a method on Transport, or at least a
function in transport.py? It seems likely to be reinvented elsewhere
otherwise, if it is not in fact already somewhere in our code. (For
example how does a WorkingTree go from a transport to a local dir?)

+
+
+class B...

[Andrew Bennetts (spiv)]

Martin Pool wrote:
> Thanks, that's great to have this finished off, and it's nice that this
> covers all those cases and apparently makes the code clearer to boot.
>
> I'm stopping for today but I'll try to read the rest of it tomorrow.
>
> This needs a NEWS entry and possibly a small update to the user manual
> just to say that it's possible.

Good points. I've done so.

[...]
> +def _local_path_for_transport(transport):
[...]
> I wonder if that method should be a method on Transport, or at least a
> function in transport.py? It seems likely to be reinvented elsewhere
> otherwise, if it is not in fact already somewhere in our code. (For
> example how does a WorkingTree go from a transport to a local dir?)

Yes, it's currently reinvented (with less paranoia) in bzr-svn, I think. Many
places can use external_url or local_abspath, but the wrinkle here is the
readonly decorator.

I don't think the right fix is to provide an API to try unwrap arbitrarily
complex transport decorators back to a local path, the right fix is to just pass
the original directory specified by the user directly to this code. That will
involve an API break for the transport_server_registry that (at least) bzr-svn
is using, but would be worthwhile I think. There are other things we can (and
probably should) improve if we break that API, e.g. bzr-svn almost certainly
should be using the logic in _change_globals, but currently isn't. I think the
current design delegates too much to the function in the
transport_server_registry, I think probably just something that can replace just
the _make_smart_server portion of BzrServerFactory would be more appropriate.

> +class BzrServerMaker(object):
>
> We have many things called Factory but no Makers (yet). Is this a
> Factory perhaps?

Fair enough. Changed.

> Perhaps it could have a docstring?

Added; actually I think most of docs belong to bzr_serve, so I've added a longer
one to it. They are now:

class BzrServerFactory(object):
    """Helper class for serve_bzr."""

and:

def serve_bzr(transport, host=None, port=None, inet=False):
    """This is the default implementation of 'bzr serve'.

    It creates a TCP or pipe smart server on 'transport, and runs it. The
    transport will be decorated with a chroot and pathfilter (using
    os.path.expanduser).
    """

[...]
> + def _expand_userdirs(self, path):
> + """Translate /~/ or /~user/ to e.g. /home/foo, using
> + os.path.expanduser.
>
> Actually not necessarily os.path.expanduser.

Thanks, corrected.

[...]
> This seems to have a little tension that it's now almost a generic
> path-rewrite facility that just happens to be used for homedirs. So the
> name seems almost overly specific. But perhaps a generic method is
> yagni so this is not bad for now.

Yes, my thoughts exactly :)

[...]
> + """
> + if path.split('/', 1)[0] == '~user':
> + return '/home/user' + path[len('~user'):]
> + return path
>
> (Comment) this does seem to be calling out for a class where we can do
> these as url manipulations, not string manipulations. Maybe something a
> bit more abstract is already possible using urlutils?

Surprisi...

[Andrew Bennetts (spiv)]

I've updated the branch with all the review comments so far.

For convenience, here's the NEWS entry that it adds under New Features in 2.1:

"""
* ``bzr+ssh`` and ``bzr`` paths can now be relative to home directories
  specified in the URL. Paths starting with a path segment of ``~`` are
  relative to the home directory of the user running the server, and paths
  starting with ``~user`` are relative to the home directory of the named
  user. For example, for a user "bob" with a home directory of
  ``/home/bob``, these URLs are all equivalent:

  * ``bzr+ssh://bob@host/~/repo``
  * ``bzr+ssh://bob@host/~bob/repo``
  * ``bzr+ssh://bob@host/home/bob/repo``

  If ``bzr serve`` was invoked with a ``--directory`` argument, then no
  home directories outside that directory will be accessible via this
  method.

  This is a feature of ``bzr serve``, so pre-2.1 clients will
  automatically benefit from this feature when ``bzr`` on the server is
  upgraded. (Andrew Bennetts, #109143)
"""

It's long, but it does render to HTML pretty well.

In fact, it feels like a very long way to say "finally it Just Works the way you'd intuitively expect it to". I guess it's not so bad to make this improvement extra visible...

[Martin Pool (mbp)]

+class BzrServerFactory(object):
+ """Helper class for serve_bzr."""
+
+ def __init__(self, userdir_expander=None, get_base_path=None):
+ self.cleanups = []
+ self.base_path = None
+ self.backing_transport = None
+ if userdir_expander is None:
+ userdir_expander = os.path.expanduser
+ self.userdir_expander = userdir_expander
+ if get_base_path is None:
+ get_base_path = _local_path_for_transport
+ self.get_base_path = get_base_path
+
+ def _expand_userdirs(self, path):
+ """Translate /~/ or /~user/ to e.g. /home/foo, using
+ self.userdir_expander (os.path.expanduser by default).
+
+ If the translated path would fall outside base_path, or the path does
+ not start with ~, then no translation is applied.
+
+ If the path is inside, it is adjusted to be relative to the base path.
+
+ e.g. if base_path is /home, and the expanded path is /home/joe, then
+ the translated path is joe.
+ """
+ result = path
+ if path.startswith('~'):
+ expanded = self.userdir_expander(path)
+ if not expanded.endswith('/'):
+ expanded += '/'
+ if expanded.startswith(self.base_path):
+ result = expanded[len(self.base_path):]
+ return result
+
+ def _make_expand_userdirs_filter(self, transport):
+ return pathfilter.PathFilteringServer(transport, self._expand_userdirs)
+
+ def _make_backing_transport(self, transport):
+ """Chroot transport, and decorate with userdir expander."""
+ self.base_path = self.get_base_path(transport)
+ chroot_server = chroot.ChrootServer(transport)
+ chroot_server.setUp()
+ self.cleanups.append(chroot_server.tearDown)
+ transport = get_transport(chroot_server.get_url())
+ if self.base_path is not None:
+ # Decorate the server's backing transport with a filter that can
+ # expand homedirs.
+ expand_userdirs = self._make_expand_userdirs_filter(transport)
+ expand_userdirs.setUp()
+ self.cleanups.append(expand_userdirs.tearDown)
+ transport = get_transport(expand_userdirs.get_url())
+ self.transport = transport

The handling of get_base_path still seems just a bit convoluted to me,
though it's probably not worth polishing any more. Maybe it can wait
unless/until someone wants other more complicated filtering.

+
+ def _make_smart_server(self, host, port, inet):
+ if inet:
+ smart_server = medium.SmartServerPipeStreamMedium(
+ sys.stdin, sys.stdout, self.transport)
+ else:
+ if host is None:
+ host = medium.BZR_DEFAULT_INTERFACE
+ if port is None:
+ port = medium.BZR_DEFAULT_PORT
+ smart_server = SmartTCPServer(self.transport, host=host, port=port)
+ trace.note('listening on port: %s' % smart_server.port)
+ self.smart_server = smart_server
+
+ def _change_globals(self):
+ from bzrlib import lockdir, ui
+ # For the duration of this server, no UI output is permitt...

review: Approve

[Andrew Bennetts (spiv)]

Martin Pool wrote:
[...]
>
> The handling of get_base_path still seems just a bit convoluted to me,
> though it's probably not worth polishing any more. Maybe it can wait
> unless/until someone wants other more complicated filtering.

Yeah, to me too. It's another symptom of cmd_serve not just passing this value
to bzr_serve directly, I think.

[...]
>
> Unless this is also going to be used as a TestCase (really?) why not
> make them setup() and tear_down()? Or maybe that is the consistent style for
> transport servers?

Good point. It is consistent with transport servers, but I don't know that that
counts for much compared to being consistent with everything else in the
(non-test) code base. I've changed them to set_up and tear_down. ("set_up"
rather than "setup", because "set up" is a verb phrase whereas "setup" is a
noun, I think... and it's consistent with "setUp").

Sending to PQM now.

-Andrew.

[Martin Pool (mbp)]

2009/9/18 Andrew Bennetts <email address hidden>:
> Martin Pool wrote:
> [...]
>>
>> The handling of get_base_path still seems just a bit convoluted to me,
>> though it's probably not worth polishing any more.  Maybe it can wait
>> unless/until someone wants other more complicated filtering.
>
> Yeah, to me too.  It's another symptom of cmd_serve not just passing this value
> to bzr_serve directly, I think.

I think it is reasonable for that layer to work with transports. I
think the expansion mechanism should probably be given either URLs or
transports, and then if a particular filter wants to say "does this
may to a local path" it can do so.
--
Martin <http://launchpad.net/~mbp/>

=== modified file 'bzrlib/smart/server.py'
--- bzrlib/smart/server.py	2009-07-20 11:27:05 +0000
+++ bzrlib/smart/server.py	2009-09-16 04:42:12 +0000
@@ -17,6 +17,7 @@
 """Server for smart-server protocol."""
 
 import errno
+import os.path
 import socket
 import sys
 import threading
@@ -30,6 +31,14 @@
 from bzrlib.lazy_import import lazy_import
 lazy_import(globals(), """
 from bzrlib.smart import medium
+from bzrlib.transport import (
+    chroot,
+    get_transport,
+    pathfilter,
+    )
+from bzrlib import (
+    urlutils,
+    )
 """)
 
 
@@ -313,34 +322,125 @@
         return transport.get_transport(url)
 
 
-def serve_bzr(transport, host=None, port=None, inet=False):
-    from bzrlib import lockdir, ui
-    from bzrlib.transport import get_transport
-    from bzrlib.transport.chroot import ChrootServer
-    chroot_server = ChrootServer(transport)
-    chroot_server.setUp()
-    transport = get_transport(chroot_server.get_url())
-    if inet:
-        smart_server = medium.SmartServerPipeStreamMedium(
-            sys.stdin, sys.stdout, transport)
+def _local_path_for_transport(transport):
+    """Return a local path for transport, if reasonably possible.
+    
+    This function works even if transport's url has a "readonly+" prefix,
+    unlike local_path_from_url.
+    
+    This essentially recovers the --directory argument the user passed to "bzr
+    serve" from the transport passed to serve_bzr.
+    """
+    try:
+        base_url = transport.external_url()
+    except (errors.InProcessTransport, NotImplementedError):
+        return None
     else:
-        if host is None:
-            host = medium.BZR_DEFAULT_INTERFACE
-        if port is None:
-            port = medium.BZR_DEFAULT_PORT
-        smart_server = SmartTCPServer(transport, host=host, port=port)
-        trace.note('listening on port: %s' % smart_server.port)
-    # For the duration of this server, no UI output is permitted. note
-    # that this may cause problems with blackbox tests. This should be
-    # changed with care though, as we dont want to use bandwidth sending
-    # progress over stderr to smart server clients!
-    old_factory = ui.ui_factory
-    old_lockdir_timeout = lockdir._DEFAULT_TIMEOUT_SECONDS
-    try:
+        # Strip readonly prefix
+        if base_url.startswith('readonly+'):
+            base_url = base_url[len('readonly+'):]
+        try:
+            return urlutils.local_path_from_url(base_url)
+        except errors.InvalidURL:
+            return None
+
+
+class BzrServerMaker(object):
+
+    def __init__(self, userdir_expander=None, get_base_path=None):
+        self.cleanups = []
+        self.base_path = None
+        self.backing_transport = None
+        if userdir_expander is None:
+            userdir_expander = os.path.expanduser
+        self.userdir_expander = userdir_expander
+        if get_base_path is None:
+            get_base_path = _local_path_for_transport
+        self.get_base_path = get_base_path
+
+    def _expand_userdirs(self, path):
+        """Translate /~/ or /~user/ to e.g. /home/foo, using
+        os.path.expanduser.
+
+        If the translated path would fall outside base_path, or the path does
+        not start with ~, then no translation is applied.
+
+        If the path is inside, it is adjusted to be relative to the base path.
+
+        e.g. if base_path is /home, and the expanded path is /home/joe, then
+        the translated path is joe.
+        """
+        result = path
+        if path.startswith('~'):
+            expanded = self.userdir_expander(path)
+            if not expanded.endswith('/'):
+                expanded += '/'
+            if expanded.startswith(self.base_path):
+                result = expanded[len(self.base_path):]
+        return result
+
+    def _make_expand_userdirs_filter(self, transport):
+        return pathfilter.PathFilteringServer(transport, self._expand_userdirs)
+
+    def _make_backing_transport(self, transport):
+        """Chroot transport, and decorate with userdir expander."""
+        self.base_path = self.get_base_path(transport)
+        chroot_server = chroot.ChrootServer(transport)
+        chroot_server.setUp()
+        self.cleanups.append(chroot_server.tearDown)
+        transport = get_transport(chroot_server.get_url())
+        if self.base_path is not None:
+            # Decorate the server's backing transport with a filter that can
+            # expand homedirs.
+            expand_userdirs = self._make_expand_userdirs_filter(transport)
+            expand_userdirs.setUp()
+            self.cleanups.append(expand_userdirs.tearDown)
+            transport = get_transport(expand_userdirs.get_url())
+        self.transport = transport
+
+    def _make_smart_server(self, host, port, inet):
+        if inet:
+            smart_server = medium.SmartServerPipeStreamMedium(
+                sys.stdin, sys.stdout, self.transport)
+        else:
+            if host is None:
+                host = medium.BZR_DEFAULT_INTERFACE
+            if port is None:
+                port = medium.BZR_DEFAULT_PORT
+            smart_server = SmartTCPServer(self.transport, host=host, port=port)
+            trace.note('listening on port: %s' % smart_server.port)
+        self.smart_server = smart_server
+
+    def _change_globals(self):
+        from bzrlib import lockdir, ui
+        # For the duration of this server, no UI output is permitted. note
+        # that this may cause problems with blackbox tests. This should be
+        # changed with care though, as we dont want to use bandwidth sending
+        # progress over stderr to smart server clients!
+        old_factory = ui.ui_factory
+        old_lockdir_timeout = lockdir._DEFAULT_TIMEOUT_SECONDS
+        def restore_default_ui_factory_and_lockdir_timeout():
+            ui.ui_factory = old_factory
+            lockdir._DEFAULT_TIMEOUT_SECONDS = old_lockdir_timeout
+        self.cleanups.append(restore_default_ui_factory_and_lockdir_timeout)
         ui.ui_factory = ui.SilentUIFactory()
         lockdir._DEFAULT_TIMEOUT_SECONDS = 0
-        smart_server.serve()
+
+    def setUp(self, transport, host, port, inet):
+        self._make_backing_transport(transport)
+        self._make_smart_server(host, port, inet)
+        self._change_globals()
+
+    def tearDown(self):
+        for cleanup in reversed(self.cleanups):
+            cleanup()
+
+
+def serve_bzr(transport, host=None, port=None, inet=False):
+    bzr_server = BzrServerMaker()
+    try:
+        bzr_server.setUp(transport, host, port, inet)
+        bzr_server.smart_server.serve()
     finally:
-        ui.ui_factory = old_factory
-        lockdir._DEFAULT_TIMEOUT_SECONDS = old_lockdir_timeout
+        bzr_server.tearDown()
 

=== modified file 'bzrlib/tests/blackbox/test_serve.py'
--- bzrlib/tests/blackbox/test_serve.py	2009-07-20 11:27:05 +0000
+++ bzrlib/tests/blackbox/test_serve.py	2009-09-16 04:42:12 +0000
@@ -18,6 +18,7 @@
 """Tests of the bzr serve command."""
 
 import os
+import os.path
 import signal
 import subprocess
 import sys
@@ -25,17 +26,21 @@
 import threading
 
 from bzrlib import (
+    builtins,
     errors,
     osutils,
     revision as _mod_revision,
-    transport,
     )
 from bzrlib.branch import Branch
 from bzrlib.bzrdir import BzrDir
 from bzrlib.errors import ParamikoNotPresent
 from bzrlib.smart import client, medium
-from bzrlib.smart.server import SmartTCPServer
-from bzrlib.tests import TestCaseWithTransport, TestSkipped
+from bzrlib.smart.server import BzrServerMaker, SmartTCPServer
+from bzrlib.tests import (
+    TestCaseWithTransport,
+    TestCaseWithMemoryTransport,
+    TestSkipped,
+    )
 from bzrlib.trace import mutter
 from bzrlib.transport import get_transport, remote
 
@@ -316,4 +321,61 @@
         client_medium.disconnect()
 
 
+class TestUserdirExpansion(TestCaseWithMemoryTransport):
+
+    def fake_expanduser(self, path):
+        """A simple, environment-independent, function for the duration of this
+        test.
+
+        Paths starting with a path segment of '~user' will expand to start with
+        '/home/user/'.  Every other path will be unchanged.
+        """
+        if path.split('/', 1)[0] == '~user':
+            return '/home/user' + path[len('~user'):]
+        return path
+
+    def make_test_server(self, base_path='/'):
+        """Make and setUp a BzrServerMaker, backed by a memory transport, and
+        creat '/home/user' in that transport.
+        """
+        bzr_server = BzrServerMaker(
+            self.fake_expanduser, lambda t: base_path)
+        mem_transport = self.get_transport()
+        mem_transport.mkdir_multi(['home', 'home/user'])
+        bzr_server.setUp(mem_transport, None, None, inet=True)
+        self.addCleanup(bzr_server.tearDown)
+        return bzr_server
+
+    def test_bzr_serve_expands_userdir(self):
+        bzr_server = self.make_test_server()
+        self.assertTrue(bzr_server.smart_server.backing_transport.has('~user'))
+
+    def test_bzr_serve_does_not_expand_userdir_outside_base(self):
+        bzr_server = self.make_test_server('/foo')
+        self.assertFalse(bzr_server.smart_server.backing_transport.has('~user'))
+
+    def test_get_base_path(self):
+        """cmd_serve will turn the --directory option into a LocalTransport
+        (optionally decorated with 'readonly+').  BzrServerMaker can determine
+        the original --directory from that transport.
+        """
+        # Define a fake 'protocol' to capture the transport that cmd_serve
+        # passes to serve_bzr.
+        def capture_transport(transport, host, port, inet):
+            self.bzr_serve_transport = transport
+        cmd = builtins.cmd_serve()
+        # Read-only
+        cmd.run(directory='/a/b/c', protocol=capture_transport)
+        server_maker = BzrServerMaker()
+        self.assertEqual(
+            'readonly+file:///a/b/c/', self.bzr_serve_transport.base)
+        self.assertEqual(
+            u'/a/b/c/', server_maker.get_base_path(self.bzr_serve_transport))
+        # Read-write
+        cmd.run(directory='/a/b/c', protocol=capture_transport,
+            allow_writes=True)
+        server_maker = BzrServerMaker()
+        self.assertEqual('file:///a/b/c/', self.bzr_serve_transport.base)
+        self.assertEqual(
+            u'/a/b/c/', server_maker.get_base_path(self.bzr_serve_transport))
 

=== modified file 'bzrlib/tests/test_transport.py'
--- bzrlib/tests/test_transport.py	2009-03-23 14:59:43 +0000
+++ bzrlib/tests/test_transport.py	2009-09-14 07:22:43 +0000
@@ -48,6 +48,7 @@
 from bzrlib.transport.memory import MemoryTransport
 from bzrlib.transport.local import (LocalTransport,
                                     EmulatedWin32LocalTransport)
+from bzrlib.transport.pathfilter import PathFilteringServer
 
 
 # TODO: Should possibly split transport-specific tests into their own files.
@@ -445,6 +446,90 @@
         server.tearDown()
 
 
+class PathFilteringDecoratorTransportTest(TestCase):
+    """Pathfilter decoration specific tests."""
+
+    def test_abspath(self):
+        # The abspath is always relative to the base of the backing transport.
+        server = PathFilteringServer(get_transport('memory:///foo/bar/'),
+            lambda x: x)
+        server.setUp()
+        transport = get_transport(server.get_url())
+        self.assertEqual(server.get_url(), transport.abspath('/'))
+
+        subdir_transport = transport.clone('subdir')
+        self.assertEqual(server.get_url(), subdir_transport.abspath('/'))
+        server.tearDown()
+
+    def make_pf_transport(self, filter_func=None):
+        """Make a PathFilteringTransport backed by a MemoryTransport.
+        
+        :param filter_func: by default this will be a no-op function.  Use this
+            parameter to override it."""
+        if filter_func is None:
+            filter_func = lambda x: x
+        server = PathFilteringServer(
+            get_transport('memory:///foo/bar/'), filter_func)
+        server.setUp()
+        self.addCleanup(server.tearDown)
+        return get_transport(server.get_url())
+
+    def test__filter(self):
+        # _filter (with an identity func as filter_func) always returns
+        # paths relative to the base of the backing transport.
+        transport = self.make_pf_transport()
+        self.assertEqual('foo', transport._filter('foo'))
+        self.assertEqual('foo/bar', transport._filter('foo/bar'))
+        self.assertEqual('', transport._filter('..'))
+        self.assertEqual('', transport._filter('/'))
+        # The base of the pathfiltering transport is taken into account too.
+        transport = transport.clone('subdir1/subdir2')
+        self.assertEqual('subdir1/subdir2/foo', transport._filter('foo'))
+        self.assertEqual(
+            'subdir1/subdir2/foo/bar', transport._filter('foo/bar'))
+        self.assertEqual('subdir1', transport._filter('..'))
+        self.assertEqual('', transport._filter('/'))
+
+    def test_filter_invocation(self):
+        filter_log = []
+        def filter(path):
+            filter_log.append(path)
+            return path
+        transport = self.make_pf_transport(filter)
+        transport.has('abc')
+        self.assertEqual(['abc'], filter_log)
+        del filter_log[:]
+        transport.clone('abc').has('xyz')
+        self.assertEqual(['abc/xyz'], filter_log)
+        del filter_log[:]
+        transport.has('/abc')
+        self.assertEqual(['abc'], filter_log)
+
+    def test_clone(self):
+        transport = self.make_pf_transport()
+        # relpath from root and root path are the same
+        relpath_cloned = transport.clone('foo')
+        abspath_cloned = transport.clone('/foo')
+        self.assertEqual(transport.server, relpath_cloned.server)
+        self.assertEqual(transport.server, abspath_cloned.server)
+
+    def test_url_preserves_pathfiltering(self):
+        """Calling get_transport on a pathfiltered transport's base should
+        produce a transport with exactly the same behaviour as the original
+        pathfiltered transport.
+
+        This is so that it is not possible to escape (accidentally or
+        otherwise) the filtering by doing::
+            url = filtered_transport.base
+            parent_url = urlutils.join(url, '..')
+            new_transport = get_transport(parent_url)
+        """
+        transport = self.make_pf_transport()
+        new_transport = get_transport(transport.base)
+        self.assertEqual(transport.server, new_transport.server)
+        self.assertEqual(transport.base, new_transport.base)
+
+
 class ReadonlyDecoratorTransportTest(TestCase):
     """Readonly decoration specific tests."""
 

=== modified file 'bzrlib/transport/__init__.py'
--- bzrlib/transport/__init__.py	2009-08-04 11:40:59 +0000
+++ bzrlib/transport/__init__.py	2009-09-14 05:39:09 +0000
@@ -90,8 +90,10 @@
                 modules.add(factory._module_name)
             else:
                 modules.add(factory._obj.__module__)
-    # Add chroot directly, because there is no handler registered for it.
+    # Add chroot and pathfilter directly, because there is no handler
+    # registered for it.
     modules.add('bzrlib.transport.chroot')
+    modules.add('bzrlib.transport.pathfilter')
     result = list(modules)
     result.sort()
     return result

=== modified file 'bzrlib/transport/chroot.py'
--- bzrlib/transport/chroot.py	2009-03-23 14:59:43 +0000
+++ bzrlib/transport/chroot.py	2009-09-16 04:37:33 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006 Canonical Ltd
+# Copyright (C) 2006-2009 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -17,21 +17,18 @@
 """Implementation of Transport that prevents access to locations above a set
 root.
 """
-from urlparse import urlparse
 
-from bzrlib import errors, urlutils
 from bzrlib.transport import (
     get_transport,
+    pathfilter,
     register_transport,
     Server,
     Transport,
     unregister_transport,
     )
-from bzrlib.transport.decorator import TransportDecorator, DecoratorServer
-from bzrlib.transport.memory import MemoryTransport
-
-
-class ChrootServer(Server):
+
+
+class ChrootServer(pathfilter.PathFilteringServer):
     """User space 'chroot' facility.
 
     The server's get_url returns the url for a chroot transport mapped to the
@@ -39,126 +36,40 @@
     directories of the backing transport are not visible. The chroot url will
     not allow '..' sequences to result in requests to the chroot affecting
     directories outside the backing transport.
+
+    PathFilteringServer does all the path sanitation needed to enforce a
+    chroot, so this is a simple subclass of PathFilteringServer that ignores
+    filter_func.
     """
 
     def __init__(self, backing_transport):
-        self.backing_transport = backing_transport
+        pathfilter.PathFilteringServer.__init__(self, backing_transport, None)
 
     def _factory(self, url):
         return ChrootTransport(self, url)
 
-    def get_url(self):
-        return self.scheme
-
     def setUp(self):
         self.scheme = 'chroot-%d:///' % id(self)
         register_transport(self.scheme, self._factory)
 
-    def tearDown(self):
-        unregister_transport(self.scheme, self._factory)
-
-
-class ChrootTransport(Transport):
+
+class ChrootTransport(pathfilter.PathFilteringTransport):
     """A ChrootTransport.
 
     Please see ChrootServer for details.
     """
 
-    def __init__(self, server, base):
-        self.server = server
-        if not base.endswith('/'):
-            base += '/'
-        Transport.__init__(self, base)
-        self.base_path = self.base[len(self.server.scheme)-1:]
-        self.scheme = self.server.scheme
-
-    def _call(self, methodname, relpath, *args):
-        method = getattr(self.server.backing_transport, methodname)
-        return method(self._safe_relpath(relpath), *args)
-
-    def _safe_relpath(self, relpath):
-        safe_relpath = self._combine_paths(self.base_path, relpath)
-        if not safe_relpath.startswith('/'):
-            raise ValueError(safe_relpath)
-        return safe_relpath[1:]
-
-    # Transport methods
-    def abspath(self, relpath):
-        return self.scheme + self._safe_relpath(relpath)
-
-    def append_file(self, relpath, f, mode=None):
-        return self._call('append_file', relpath, f, mode)
-
-    def _can_roundtrip_unix_modebits(self):
-        return self.server.backing_transport._can_roundtrip_unix_modebits()
-
-    def clone(self, relpath):
-        return ChrootTransport(self.server, self.abspath(relpath))
-
-    def delete(self, relpath):
-        return self._call('delete', relpath)
-
-    def delete_tree(self, relpath):
-        return self._call('delete_tree', relpath)
-
-    def external_url(self):
-        """See bzrlib.transport.Transport.external_url."""
-        # Chroots, like MemoryTransport depend on in-process
-        # state and thus the base cannot simply be handed out.
-        # See the base class docstring for more details and
-        # possible directions. For now we return the chrooted
-        # url.
-        return self.server.backing_transport.external_url()
-
-    def get(self, relpath):
-        return self._call('get', relpath)
-
-    def has(self, relpath):
-        return self._call('has', relpath)
-
-    def is_readonly(self):
-        return self.server.backing_transport.is_readonly()
-
-    def iter_files_recursive(self):
-        backing_transport = self.server.backing_transport.clone(
-            self._safe_relpath('.'))
-        return backing_transport.iter_files_recursive()
-
-    def listable(self):
-        return self.server.backing_transport.listable()
-
-    def list_dir(self, relpath):
-        return self._call('list_dir', relpath)
-
-    def lock_read(self, relpath):
-        return self._call('lock_read', relpath)
-
-    def lock_write(self, relpath):
-        return self._call('lock_write', relpath)
-
-    def mkdir(self, relpath, mode=None):
-        return self._call('mkdir', relpath, mode)
-
-    def open_write_stream(self, relpath, mode=None):
-        return self._call('open_write_stream', relpath, mode)
-
-    def put_file(self, relpath, f, mode=None):
-        return self._call('put_file', relpath, f, mode)
-
-    def rename(self, rel_from, rel_to):
-        return self._call('rename', rel_from, self._safe_relpath(rel_to))
-
-    def rmdir(self, relpath):
-        return self._call('rmdir', relpath)
-
-    def stat(self, relpath):
-        return self._call('stat', relpath)
+    def _filter(self, relpath):
+        # A simplified version of PathFilteringTransport's _filter that omits
+        # the call to self.server.filter_func.
+        return self._relpath_from_server_root(relpath)
 
 
 class TestingChrootServer(ChrootServer):
 
     def __init__(self):
         """TestingChrootServer is not usable until setUp is called."""
+        ChrootServer.__init__(self, None)
 
     def setUp(self, backing_server=None):
         """Setup the Chroot on backing_server."""
@@ -171,5 +82,4 @@
 
 def get_test_permutations():
     """Return the permutations to be used in testing."""
-    return [(ChrootTransport, TestingChrootServer),
-            ]
+    return [(ChrootTransport, TestingChrootServer)]

=== added file 'bzrlib/transport/pathfilter.py'
--- bzrlib/transport/pathfilter.py	1970-01-01 00:00:00 +0000
+++ bzrlib/transport/pathfilter.py	2009-09-16 04:37:33 +0000
@@ -0,0 +1,195 @@
+# Copyright (C) 2006 Canonical Ltd
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+"""A transport decorator that filters all paths that are passed to it."""
+
+
+from bzrlib.transport import (
+    get_transport,
+    register_transport,
+    Server,
+    Transport,
+    unregister_transport,
+    )
+
+
+class PathFilteringServer(Server):
+    """Transport server for PathFilteringTransport.
+
+    It holds the backing_transport and filter_func for PathFilteringTransports.
+    All paths will be passed through filter_func before calling into the
+    backing_transport.
+
+    Note that paths returned from the backing transport are *not* altered in
+    anyway.  So, depending on the filter_func, PathFilteringTransports might
+    not conform to the usual expectations of Transport behaviour; e.g. 'name'
+    in t.list_dir('dir') might not imply t.has('dir/name') is True!  A filter
+    that merely prefixes a constant path segment will be essentially
+    transparent, whereas a filter that does rot13 to paths will break
+    expectations and probably cause confusing errors.  So choose your
+    filter_func with care.
+    """
+
+    def __init__(self, backing_transport, filter_func):
+        """Constructor.
+
+        :param backing_transport: a transport
+        :param filter_func: a callable that takes paths, and translates them
+            into paths for use with the backing transport.
+        """
+        self.backing_transport = backing_transport
+        self.filter_func = filter_func
+
+    def _factory(self, url):
+        return PathFilteringTransport(self, url)
+
+    def get_url(self):
+        return self.scheme
+
+    def setUp(self):
+        self.scheme = 'filtered-%d:///' % id(self)
+        register_transport(self.scheme, self._factory)
+
+    def tearDown(self):
+        unregister_transport(self.scheme, self._factory)
+
+
+class PathFilteringTransport(Transport):
+    """A PathFilteringTransport.
+
+    Please see PathFilteringServer for details.
+    """
+
+    def __init__(self, server, base):
+        self.server = server
+        if not base.endswith('/'):
+            base += '/'
+        Transport.__init__(self, base)
+        self.base_path = self.base[len(self.server.scheme)-1:]
+        self.scheme = self.server.scheme
+
+    def _relpath_from_server_root(self, relpath):
+        unfiltered_path = self._combine_paths(self.base_path, relpath)
+        if not unfiltered_path.startswith('/'):
+            raise ValueError(unfiltered_path)
+        return unfiltered_path[1:]
+
+    def _filter(self, relpath):
+        return self.server.filter_func(self._relpath_from_server_root(relpath))
+
+    def _call(self, methodname, relpath, *args):
+        """Helper for Transport methods of the form:
+            operation(path, [other args ...])
+        """
+        backing_method = getattr(self.server.backing_transport, methodname)
+        return backing_method(self._filter(relpath), *args)
+
+    # Transport methods
+    def abspath(self, relpath):
+        # We do *not* want to filter at this point; e.g if the filter is
+        # homedir expansion, self.base == 'this:///' and relpath == '~/foo',
+        # then the abspath should be this:///~/foo (not this:///home/user/foo).
+        # Instead filtering should happen when self's base is passed to the
+        # backing.
+        return self.scheme + self._relpath_from_server_root(relpath)
+
+    def append_file(self, relpath, f, mode=None):
+        return self._call('append_file', relpath, f, mode)
+
+    def _can_roundtrip_unix_modebits(self):
+        return self.server.backing_transport._can_roundtrip_unix_modebits()
+
+    def clone(self, relpath):
+        return self.__class__(self.server, self.abspath(relpath))
+
+    def delete(self, relpath):
+        return self._call('delete', relpath)
+
+    def delete_tree(self, relpath):
+        return self._call('delete_tree', relpath)
+
+    def external_url(self):
+        """See bzrlib.transport.Transport.external_url."""
+        # PathFilteringTransports, like MemoryTransport, depend on in-process
+        # state and thus the base cannot simply be handed out.  See the base
+        # class docstring for more details and possible directions. For now we
+        # return the path-filtered URL.
+        return self.server.backing_transport.external_url()
+
+    def get(self, relpath):
+        return self._call('get', relpath)
+
+    def has(self, relpath):
+        return self._call('has', relpath)
+
+    def is_readonly(self):
+        return self.server.backing_transport.is_readonly()
+
+    def iter_files_recursive(self):
+        backing_transport = self.server.backing_transport.clone(
+            self._filter('.'))
+        return backing_transport.iter_files_recursive()
+
+    def listable(self):
+        return self.server.backing_transport.listable()
+
+    def list_dir(self, relpath):
+        return self._call('list_dir', relpath)
+
+    def lock_read(self, relpath):
+        return self._call('lock_read', relpath)
+
+    def lock_write(self, relpath):
+        return self._call('lock_write', relpath)
+
+    def mkdir(self, relpath, mode=None):
+        return self._call('mkdir', relpath, mode)
+
+    def open_write_stream(self, relpath, mode=None):
+        return self._call('open_write_stream', relpath, mode)
+
+    def put_file(self, relpath, f, mode=None):
+        return self._call('put_file', relpath, f, mode)
+
+    def rename(self, rel_from, rel_to):
+        return self._call('rename', rel_from, self._filter(rel_to))
+
+    def rmdir(self, relpath):
+        return self._call('rmdir', relpath)
+
+    def stat(self, relpath):
+        return self._call('stat', relpath)
+
+
+class TestingPathFilteringServer(PathFilteringServer):
+
+    def __init__(self):
+        """TestingChrootServer is not usable until setUp is called."""
+
+    def setUp(self, backing_server=None):
+        """Setup the Chroot on backing_server."""
+        if backing_server is not None:
+            self.backing_transport = get_transport(backing_server.get_url())
+        else:
+            self.backing_transport = get_transport('.')
+        self.backing_transport.clone('added-by-filter').ensure_base()
+        self.filter_func = lambda x: 'added-by-filter/' + x
+        PathFilteringServer.setUp(self)
+
+
+def get_test_permutations():
+    """Return the permutations to be used in testing."""
+    return [(PathFilteringTransport, TestingPathFilteringServer)]