Merge lp:~wallyworld/launchpad/recursive-branch-resolution-failure into lp:launchpad

[Ian Booth]

When parsing short from branch urls of the for +branch/foo/bar, in the case of an error, handle the case where the referring url is None.

Bug 645544 describes a recursive failure when navigating to a url like +/branch/foo. This is a bug resulting from work done on lp:~wallyworld/lanchpad/tales-linkify-broken-links for bug 404131.

The problem is that when a url is entered directly, the http referer is None and the redirection logic tries to send the browser to an invalid url.

Proposed fix

Simple fix in the error handling logic to simply raise an exception if there is no referer in the http request header. This is how the system used to work before the fix for bug 404131 was done.

Implementation Details

The only code changes were to:
    - lib/canonical/launchpad/browser/launchpad.py
    - lib/canonical/launchpad/browser/tests/test_launchpad.py

Tests

    bin/test -vvm canonical.launchpad.browser.tests.test_launchpad TestBranchTraversal

    New tests were written to test for the cases where the referring url is None:
    - test_nonexistent_product_without_referer
    - test_private_without_referer

The TestBranchTraversal class (and friends) needed to be changed to pass a default referring url to the business logic being tested since previously the referring url was always None. The test cases never made the distinction between navigating from a valid url or hacking the url directly.

lint

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/browser/launchpad.py
  lib/canonical/launchpad/browser/tests/test_launchpad.py

[Michael Hudson-Doyle (mwhudson)]

As I said on IRC I'd like you to change the name of the 'url' variable redirect_branch to http_referer or referer. Otherwise it's all fine.

Cheers!

review: Approve

[Ian Booth (wallyworld)]

I noticed there was still a code path were a redirect to None could occur - if an attempt was made to navigate directly to a private branch. Code fixed and new unit tested added.

[Michael Hudson-Doyle (mwhudson)]

The changes look fine too.

=== modified file 'lib/canonical/launchpad/browser/launchpad.py'
--- lib/canonical/launchpad/browser/launchpad.py	2010-09-15 06:40:16 +0000
+++ lib/canonical/launchpad/browser/launchpad.py	2010-09-23 05:22:52 +0000
@@ -521,33 +521,47 @@
         with a suitable error message.
         """
 
-        # The default url to go to will be back to the referring page (in
-        # the case that there is an error resolving the branch url)
-        url = self.request.getHeader('referer')
+        # The default target url to go to will be back to the referring page
+        # (in the case that there is an error resolving the branch url).
+        # Note: the http referer may be None if someone has hacked a url
+        # directly rather than following a /+branch/<foo> link.
+        target_url = self.request.getHeader('referer')
         path = '/'.join(self.request.stepstogo)
         try:
             # first check for a valid branch url
             try:
                 branch_data = getUtility(IBranchLookup).getByLPPath(path)
                 branch, trailing = branch_data
-                url = canonical_url(branch)
+                target_url = canonical_url(branch)
                 if trailing is not None:
-                    url = urlappend(url, trailing)
+                    target_url = urlappend(target_url, trailing)
 
-            except (NoLinkedBranch):
+            except (NoLinkedBranch) as e:
                 # a valid ICanHasLinkedBranch target exists but there's no
                 # branch or it's not visible
+
+                # If are aren't arriving at this invalid branch URL from
+                # another page then we just raise an exception, otherwise we
+                # end up in a bad recursion loop. The target url will be None
+                # in that case.
+                if target_url is None:
+                    raise e
                 self.request.response.addNotification(
                     "The target %s does not have a linked branch." % path)
 
         except (CannotHaveLinkedBranch, InvalidNamespace,
                 InvalidProductName, NotFoundError) as e:
+            # If are aren't arriving at this invalid branch URL from another
+            # page then we just raise an exception, otherwise we end up in a
+            # bad recursion loop. The target url will be None in that case.
+            if target_url is None:
+                raise e
             error_msg = str(e)
             if error_msg == '':
                 error_msg = "Invalid branch lp:%s." % path
             self.request.response.addErrorNotification(error_msg)
 
-        return self.redirectSubTree(url)
+        return self.redirectSubTree(target_url)
 
     @stepto('+builds')
     def redirect_buildfarm(self):

=== modified file 'lib/canonical/launchpad/browser/tests/test_launchpad.py'
--- lib/canonical/launchpad/browser/tests/test_launchpad.py	2010-09-16 06:02:36 +0000
+++ lib/canonical/launchpad/browser/tests/test_launchpad.py	2010-09-23 05:22:52 +0000
@@ -20,6 +20,7 @@
 from canonical.launchpad.webapp.url import urlappend
 from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.app.errors import GoneError
+from lp.code.errors import NoLinkedBranch
 from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
 from lp.registry.interfaces.person import (
     IPersonSet,
@@ -33,6 +34,11 @@
 from lp.testing.views import create_view
 
 
+# We set the request header HTTP_REFERER  when we want to simulate navigation
+# from a valid page. This is used in the assertDisplaysNotification check.
+DEFAULT_REFERER = 'http://launchpad.dev'
+
+
 class TraversalMixin:
 
     def _validateNotificationContext(
@@ -73,28 +79,40 @@
         """
 
         redirection = self.traverse(path)
-        self.assertIs(redirection.target, None)
+        self.assertIs(redirection.target, DEFAULT_REFERER)
         self._validateNotificationContext(
             redirection.request, notification, level)
 
-    def assertNotFound(self, path):
-        self.assertRaises(NotFound, self.traverse, path)
+    def assertNoLinkedBranch(self, path, use_default_referer=True):
+        self.assertRaises(
+            NoLinkedBranch, self.traverse, path,
+            use_default_referer=use_default_referer)
+
+    def assertNotFound(self, path, use_default_referer=True):
+        self.assertRaises(
+            NotFound, self.traverse, path,
+            use_default_referer=use_default_referer)
 
     def assertRedirects(self, segments, url):
         redirection = self.traverse(segments)
         self.assertEqual(url, redirection.target)
 
-    def traverse(self, path, first_segment):
+    def traverse(self, path, first_segment, use_default_referer=True):
         """Traverse to 'segments' using a 'LaunchpadRootNavigation' object.
 
         Using the Zope traversal machinery, traverse to the path given by
         'segments', starting at a `LaunchpadRootNavigation` object.
 
         :param segments: A list of path segments.
+        :param use_default_referer: If True, set the referer attribute in the
+            request header to DEFAULT_REFERER = "http://launchpad.dev"
+            (otherwise it remains as None)
         :return: The object found.
         """
-        request = LaunchpadTestRequest(
-            PATH_INFO=urlappend('/%s' % first_segment, path))
+        extra = {'PATH_INFO': urlappend('/%s' % first_segment, path)}
+        if use_default_referer:
+            extra['HTTP_REFERER'] = DEFAULT_REFERER
+        request = LaunchpadTestRequest(**extra)
         segments = reversed(path.split('/'))
         request.setTraversalStack(segments)
         traverser = LaunchpadRootNavigation(None, request=request)
@@ -110,8 +128,9 @@
 
     layer = DatabaseFunctionalLayer
 
-    def traverse(self, path):
-        return super(TestBranchTraversal, self).traverse(path, '+branch')
+    def traverse(self, path, **kwargs):
+        return super(TestBranchTraversal, self).traverse(
+            path, '+branch', **kwargs)
 
     def test_unique_name_traversal(self):
         # Traversing to /+branch/<unique_name> redirects to the page for that
@@ -178,6 +197,26 @@
             non_existent, requiredMessage,
             BrowserNotificationLevel.ERROR)
 
+    def test_nonexistent_product_without_referer(self):
+        # Traversing to /+branch/<no-such-product> without a referer results
+        # in a 404 error. This happens if the user hacks the URL rather than
+        # navigating via a link
+        self.assertNotFound('non-existent', use_default_referer=False)
+
+    def test_private_without_referer(self):
+        # If the development focus of a product is private and there is no
+        # referer, we will get a 404 error. This happens if the user hacks
+        # the URL rather than navigating via a link
+        branch = self.factory.makeProductBranch()
+        naked_product = removeSecurityProxy(branch.product)
+        ICanHasLinkedBranch(naked_product).setBranch(branch)
+        removeSecurityProxy(branch).private = True
+
+        any_user = self.factory.makePerson()
+        login_person(any_user)
+        self.assertNoLinkedBranch(
+            naked_product.name, use_default_referer=False)
+
     def test_product_without_dev_focus(self):
         # Traversing to a product without a development focus displays a
         # user message on the same page.