[MERGE/RFC] Add transport jail to bzr smart server; add ignore_fallbacks option to BzrDir.open_branch

[Andrew Bennetts]

Vincent Ladeuil wrote:
[...]
> I lack time to investigate the why, but the following seems
> required or 
> 
>   bzr selftest -s bp.xmloutput.tests.test_service.TestXmlRpcServer
> 
[...]
> === modified file 'bzrlib/smart/request.py'
> --- bzrlib/smart/request.py	2009-03-24 06:40:26 +0000
> +++ bzrlib/smart/request.py	2009-03-25 11:22:38 +0000
> @@ -58,7 +58,10 @@
>  
>  
>  def _pre_open_hook(transport):
> -    allowed_transports = jail_info.transports
> +    try:
> +        allowed_transports = jail_info.transports
> +    except AttributeError:
> +        allowed_transports = None
>      if allowed_transports is None:
>          return
>      abspath = transport.base

Thanks, you're right.  Although I prefer 3-arg getattr here:

=== modified file 'bzrlib/smart/request.py'
--- bzrlib/smart/request.py	2009-03-24 06:40:26 +0000
+++ bzrlib/smart/request.py	2009-03-25 22:10:45 +0000
@@ -58,7 +58,7 @@
 
 
 def _pre_open_hook(transport):
-    allowed_transports = jail_info.transports
+    allowed_transports = getattr(jail_info, 'transports', None)
     if allowed_transports is None:
         return
     abspath = transport.base


I'll add a unit test too (for starting a thread and then opening a BzrDir in
it with this hook installed).

> Looking from 10.000 feet, I also find the bare '_install_hook()'
> call at the module init time in bzrlib.smart.request choking.
> 
> The comment in TestCase._clear_hooks() where I went (immediately
> of course :) 'this hook should always be installed' didn't
> explain *why* either.
> 
> Without going further, my feeling is that this hook shouldn't be
> installed unconditionally, the tests shouldn't have to reinstall
> it either.
> 
> It seems to break the test isolation *on purpose* !
> 
> It also smells a lot like global state and I thought we hated
> that...

The purpose of this hook is to enforce a global restriction that should
always be checked.  If jail_info.transports is set in a thread, then
BzrDir.open *must* fail, no matter what code path lead us there.  This
prevents a smart server, even when running plugins that might not be careful
about e.g. stacked branches, from accidentally opening bzrdirs outside the
backing_transport.  This closes a potential security hole, and also catches
accidental bugs (like the bug it found in our cloning_metadir RPC).

So, this hook has to always be installed, at least during the processing of
a smart server request.  Robert and I agreed that an always-installed[1]
hook was the simplest and most robust way to do this.  The obvious
alternative would be to continually add and remove the hook in setup_jail
and teardown_jail, while worrying about what other hooks might be installed
and in what order, and still needing to provide a threading.local() or
similar so that plugins can have the opportunity to adjust the jail if
appropriate.

And I'll mention that by making sure the hook was always installed in the
test suite, just like it is in the production code, it found the bug in the
hook that you just reported.  So far from breaking test isolation, without
having tests reinstalled the test environment would have been different from
the real environment in a way that would have hidden this bug!

The only test isolation issue I see here is if a test leaves
jail_info.transports set to not None.  I'm not sure if we should worry about
that or not... very few places change that value, and the failures that
would cause would be pretty obviously due to previous, broken test. 

It is global state, and I don't like adding it either.  But I think it's to
needed to fully enforce global state that we already have: the ChrootServer
that was set up — and registered in the global transport registry — to
provide the backing_transport for a smart server. 

> I have a note in my TODO list to look on how to test hooks
> easily, is it what is missing here ?

No, this hook function and hook point (not sure which you are referring to)
is easy enough to test.  What's missing is arguably a way to uninstall a
single hook, but I doubt it would make this code would be much simpler.  Or
even a way to install/uninstall a thread-local hook, but I think it'd be
over-engineering to build infrastructure for that.

-Andrew.


[1] The only time it isn't installed is if bzrlib.smart.request isn't
    imported, which is fine, because then there's clearly no smart server
    code running to worry about :)