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

[Vincent Ladeuil]

>>>>> "Andrew" == Andrew Bennetts <andrew.bennetts at canonical.com> writes:

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

Worth adding to the docstring module or as comment there.

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

What I still don't get is why you use a per thread data instead
of just a global one ? You have a use case where different jails
are needed inside the same process ? Outside the test suite ?

The other disturbing thing is having a *permanent* hook, why not
just make it part of the appropriate class then ?

Aren't we abusing the hook mechanisms here ?

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

Touche.

But you shouldn't be proud about making unrelated code help you
debug your code :-)

      Vincent