Merge lp:~jelmer/brz/fix-open-policy into lp:brz/3.0

[Jelmer Vernooij]

Support redirects in URL policy opener.

Support redirects in URL policy opener.

This should fix the tests for lp:~cjwatson/launchpad/use-bzr-policy-open

[Colin Watson (cjwatson)]

This doesn't seem to fix anything for me:

======================================================================
ERROR: breezy.tests.test_url_policy_open.TestRedirects.test_redirect_forbidden
----------------------------------------------------------------------
Traceback (most recent call last):
testtools.testresult.real._StringException: log: {{{
4.551 Using Bazaar configuration directory (/home/cjwatson/.bazaar)
4.552 creating repository in file:///tmp/testbzr-0oBp8d.tmp/.bzr/.
4.553 Using Bazaar configuration directory (/home/cjwatson/.bazaar)
4.555 Using Bazaar configuration directory (/home/cjwatson/.bazaar)
4.555 Using Bazaar configuration directory (/home/cjwatson/.bazaar)
4.555 creating branch <breezy.bzr.branch.BzrBranchFormat7 object at 0x7fad7dcf6910> in file:///tmp/testbzr-0oBp8d.tmp/
4.556 Using Bazaar configuration directory (/home/cjwatson/.bazaar)
4.557 Using Bazaar configuration directory (/home/cjwatson/.bazaar)
4.558 Using Bazaar configuration directory (/home/cjwatson/.bazaar)
4.559 Using Bazaar configuration directory (/home/cjwatson/.bazaar)
4.561 Using Bazaar configuration directory (/home/cjwatson/.bazaar)
4.562 Using Bazaar configuration directory (/home/cjwatson/.bazaar)
4.563 Using Bazaar configuration directory (/home/cjwatson/.bazaar)
4.564 Using Bazaar configuration directory (/home/cjwatson/.bazaar)
4.564 trying to create missing lock u'/tmp/testbzr-0oBp8d.tmp/.bzr/checkout/dirstate'
4.581 opening working tree u'/tmp/testbzr-0oBp8d.tmp'
4.582 Using Bazaar configuration directory (/home/cjwatson/.bazaar)
4.582 Using Bazaar configuration directory (/home/cjwatson/.bazaar)
4.618 Using Bazaar configuration directory (/home/cjwatson/.bazaar)
4.618 Using Bazaar configuration directory (/home/cjwatson/.bazaar)
4.625 creating repository in file:///tmp/testbzr-0oBp8d.tmp/breezy.tests.test_url_policy_open.TestRedirects.test_redirect_forbidden/work/b/.bzr/.
4.627 creating branch <breezy.bzr.branch.BzrBranchFormat7 object at 0x7fad7f519190> in file:///tmp/testbzr-0oBp8d.tmp/breezy.tests.test_url_policy_open.TestRedirects.test_redirect_forbidden/work/b/
}}}

Traceback (most recent call last):
  File "/home/cjwatson/src/bzr/brz/breezy/tests/test_url_policy_open.py", line 355, in test_redirect_forbidden
    self.assertRaises(BadUrl, opener.open, 'redirecting:///')
  File "/home/cjwatson/src/bzr/brz/breezy/tests/__init__.py", line 1481, in assertRaises
    callableObj(*args, **kwargs)
  File "/home/cjwatson/src/bzr/brz/breezy/url_policy_open.py", line 297, in open
    url = self.check_and_follow_branch_reference(url)
  File "/home/cjwatson/src/bzr/brz/breezy/url_policy_open.py", line 241, in check_and_follow_branch_reference
    next_url = self.follow_reference(url)
  File "/home/cjwatson/src/bzr/brz/breezy/url_policy_open.py", line 285, in follow_reference
    controldir = ControlDir.open(url, probers=self.probers)
  File "/home/cjwatson/src/bzr/brz/breezy/controldir.py", line 707, in open
    _unsupported=_unsupported)
  File "/home/cjwatson/src/bzr/brz/breezy/controldir.py", line 718, in open_from_transport
    hook(transport)
  File "/home/cjwatson/src/bzr/brz/breezy/tests/__init__.py", line 1212, in...

review: Needs Information

[Colin Watson (cjwatson)]

This looks good to me now, and fixes the Launchpad test failures when I cherry-pick it into our version of bzr. Thanks.

review: Approve

[Jelmer Vernooij (jelmer)]

review: Approve

[The Breezy Bot (the-breezy-bot)]

Voting criteria not met
https://ci.breezy-vcs.org/job/brz-3.0/job/brz-3.0-land/42/

[Jelmer Vernooij (jelmer)]

review: Approve (code)

[Jelmer Vernooij (jelmer)]

review: Approve (code)

=== modified file 'breezy/tests/test_url_policy_open.py'
--- breezy/tests/test_url_policy_open.py	2018-11-12 01:41:38 +0000
+++ breezy/tests/test_url_policy_open.py	2019-06-09 18:14:12 +0000
@@ -30,7 +30,10 @@
     ControlDir,
     ControlDirFormat,
     )
-from ..errors import NotBranchError
+from ..errors import (
+    NotBranchError,
+    RedirectRequested,
+    )
 from ..url_policy_open import (
     BadUrl,
     _BlacklistPolicy,
@@ -44,7 +47,15 @@
     TestCase,
     TestCaseWithTransport,
     )
-from ..transport import chroot
+from ..transport import (
+    chroot,
+    get_transport,
+    register_transport_proto,
+    register_transport,
+    unregister_transport,
+    transport_list_registry,
+    Transport,
+    )
 
 
 class TestBranchOpenerCheckAndFollowBranchReference(TestCase):
@@ -305,6 +316,48 @@
             set(TrackingProber.seen_urls), {b.base, a.base})
 
 
+class TestRedirects(TestCaseWithTransport):
+
+    def setUp(self):
+        super(TestRedirects, self).setUp()
+        BranchOpener.install_hook()
+
+    def setup_redirect(self, target_url):
+        class RedirectingTransport(Transport):
+
+            def get(self, name):
+                raise RedirectRequested(self.base, target_url)
+
+            def _redirected_to(self, source, target):
+                return get_transport(target)
+
+        register_transport_proto(
+            'redirecting://', help="Test transport that redirects.")
+        register_transport('redirecting://', RedirectingTransport)
+        self.addCleanup(unregister_transport, 'redirecting://', RedirectingTransport)
+
+    def make_branch_opener(self, allowed_urls, probers=None):
+        policy = WhitelistPolicy(True, allowed_urls, True)
+        return BranchOpener(policy, probers)
+
+    def test_redirect_forbidden(self):
+        b = self.make_branch('b')
+        self.setup_redirect(b.base)
+        class TrackingProber(BzrProber):
+            seen_urls = []
+
+            @classmethod
+            def probe_transport(klass, transport):
+                klass.seen_urls.append(transport.base)
+                return BzrProber.probe_transport(transport)
+
+        opener = self.make_branch_opener(['redirecting:///'], probers=[TrackingProber])
+        self.assertRaises(BadUrl, opener.open, 'redirecting:///')
+
+        opener = self.make_branch_opener(['redirecting:///', b.base], probers=[TrackingProber])
+        opener.open('redirecting:///')
+
+
 class TestOpenOnlyScheme(TestCaseWithTransport):
     """Tests for `open_only_scheme`."""
 

=== modified file 'breezy/url_policy_open.py'
--- breezy/url_policy_open.py	2018-11-12 01:41:38 +0000
+++ breezy/url_policy_open.py	2019-06-09 18:14:12 +0000
@@ -22,11 +22,17 @@
 
 from . import (
     errors,
+    trace,
     urlutils,
     )
 from .branch import Branch
 from .controldir import (
     ControlDir,
+    ControlDirFormat,
+    )
+from .transport import (
+    do_catching_redirections,
+    get_transport,
     )
 
 
@@ -200,6 +206,8 @@
         """
         self.policy = policy
         self._seen_urls = set()
+        if probers is None:
+            probers = ControlDirFormat.all_probers()
         self.probers = probers
 
     @classmethod
@@ -276,15 +284,46 @@
             # spurious loop exceptions.
             self._seen_urls = set()
 
+    def _open_dir(self, url):
+        """Simple BzrDir.open clone that only uses specific probers.
+
+        :param url: URL to open
+        :return: ControlDir instance
+        """
+        def redirected(transport, e, redirection_notice):
+            self.policy.check_one_url(e.target)
+            redirected_transport = transport._redirected_to(
+                e.source, e.target)
+            if redirected_transport is None:
+                raise errors.NotBranchError(e.source)
+            trace.note('%s is%s redirected to %s',
+                 transport.base, e.permanently, redirected_transport.base)
+            return redirected_transport
+
+        def find_format(transport):
+            last_error = errors.NotBranchError(transport.base)
+            for prober_kls in self.probers:
+                prober = prober_kls()
+                try:
+                    return transport, prober.probe_transport(transport)
+                except errors.NotBranchError as e:
+                    last_error = e
+            else:
+                raise last_error
+        transport = get_transport(url)
+        transport, format = do_catching_redirections(find_format, transport,
+            redirected)
+        return format.open(transport)
+
     def follow_reference(self, url):
         """Get the branch-reference value at the specified url.
 
         This exists as a separate method only to be overriden in unit tests.
         """
-        controldir = ControlDir.open(url, probers=self.probers)
+        controldir = self._open_dir(url)
         return controldir.get_branch_reference()
 
-    def open(self, url):
+    def open(self, url, ignore_fallbacks=False):
         """Open the Bazaar branch at url, first checking it.
 
         What is acceptable means is defined by the policy's `follow_reference`
@@ -295,11 +334,11 @@
 
         url = self.check_and_follow_branch_reference(url)
 
-        def open_branch(url):
-            dir = ControlDir.open(url, probers=self.probers)
-            return dir.open_branch()
+        def open_branch(url, ignore_fallbacks):
+            dir = self._open_dir(url)
+            return dir.open_branch(ignore_fallbacks=ignore_fallbacks)
         return self.run_with_transform_fallback_location_hook_installed(
-            open_branch, url)
+            open_branch, url, ignore_fallbacks)
 
 
 def open_only_scheme(allowed_scheme, url):