Launchpad itself

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

  1. recursive-branch-resolution-failure
  2. Merge into devel
Proposed by Ian Booth
Status: Merged
Approved by: Michael Hudson-Doyle
Approved revision: no longer in the source branch.
Merged at revision: 11613
Proposed branch: lp:~wallyworld/launchpad/recursive-branch-resolution-failure
Merge into: lp:launchpad
Diff against target: 168 lines (+68/-15)
2 files modified
lib/canonical/launchpad/browser/launchpad.py (+21/-7)
lib/canonical/launchpad/browser/tests/test_launchpad.py (+47/-8)
To merge this branch: bzr merge lp:~wallyworld/launchpad/recursive-branch-resolution-failure
Reviewer Review Type Date Requested Status
Michael Hudson-Doyle Approve
Review via email: mp+36403@code.launchpad.net

Commit message

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.

Description of the change

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

To post a comment you must log in.
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

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
Revision history for this message
Ian Booth (wallyworld) wrote :

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.

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

The changes look fine too.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
521 with a suitable error message.521 with a suitable error message.
522 """522 """
523523
524 # The default url to go to will be back to the referring page (in524 # The default target url to go to will be back to the referring page
525 # the case that there is an error resolving the branch url)525 # (in the case that there is an error resolving the branch url).
526 url = self.request.getHeader('referer')526 # Note: the http referer may be None if someone has hacked a url
527 # directly rather than following a /+branch/<foo> link.
528 target_url = self.request.getHeader('referer')
527 path = '/'.join(self.request.stepstogo)529 path = '/'.join(self.request.stepstogo)
528 try:530 try:
529 # first check for a valid branch url531 # first check for a valid branch url
530 try:532 try:
531 branch_data = getUtility(IBranchLookup).getByLPPath(path)533 branch_data = getUtility(IBranchLookup).getByLPPath(path)
532 branch, trailing = branch_data534 branch, trailing = branch_data
533 url = canonical_url(branch)535 target_url = canonical_url(branch)
534 if trailing is not None:536 if trailing is not None:
535 url = urlappend(url, trailing)537 target_url = urlappend(target_url, trailing)
536538
537 except (NoLinkedBranch):539 except (NoLinkedBranch) as e:
538 # a valid ICanHasLinkedBranch target exists but there's no540 # a valid ICanHasLinkedBranch target exists but there's no
539 # branch or it's not visible541 # branch or it's not visible
542
543 # If are aren't arriving at this invalid branch URL from
544 # another page then we just raise an exception, otherwise we
545 # end up in a bad recursion loop. The target url will be None
546 # in that case.
547 if target_url is None:
548 raise e
540 self.request.response.addNotification(549 self.request.response.addNotification(
541 "The target %s does not have a linked branch." % path)550 "The target %s does not have a linked branch." % path)
542551
543 except (CannotHaveLinkedBranch, InvalidNamespace,552 except (CannotHaveLinkedBranch, InvalidNamespace,
544 InvalidProductName, NotFoundError) as e:553 InvalidProductName, NotFoundError) as e:
554 # If are aren't arriving at this invalid branch URL from another
555 # page then we just raise an exception, otherwise we end up in a
556 # bad recursion loop. The target url will be None in that case.
557 if target_url is None:
558 raise e
545 error_msg = str(e)559 error_msg = str(e)
546 if error_msg == '':560 if error_msg == '':
547 error_msg = "Invalid branch lp:%s." % path561 error_msg = "Invalid branch lp:%s." % path
548 self.request.response.addErrorNotification(error_msg)562 self.request.response.addErrorNotification(error_msg)
549563
550 return self.redirectSubTree(url)564 return self.redirectSubTree(target_url)
551565
552 @stepto('+builds')566 @stepto('+builds')
553 def redirect_buildfarm(self):567 def redirect_buildfarm(self):
554568
=== 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 @@
20from canonical.launchpad.webapp.url import urlappend20from canonical.launchpad.webapp.url import urlappend
21from canonical.testing.layers import DatabaseFunctionalLayer21from canonical.testing.layers import DatabaseFunctionalLayer
22from lp.app.errors import GoneError22from lp.app.errors import GoneError
23from lp.code.errors import NoLinkedBranch
23from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch24from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
24from lp.registry.interfaces.person import (25from lp.registry.interfaces.person import (
25 IPersonSet,26 IPersonSet,
@@ -33,6 +34,11 @@
33from lp.testing.views import create_view34from lp.testing.views import create_view
3435
3536
37# We set the request header HTTP_REFERER when we want to simulate navigation
38# from a valid page. This is used in the assertDisplaysNotification check.
39DEFAULT_REFERER = 'http://launchpad.dev'
40
41
36class TraversalMixin:42class TraversalMixin:
3743
38 def _validateNotificationContext(44 def _validateNotificationContext(
@@ -73,28 +79,40 @@
73 """79 """
7480
75 redirection = self.traverse(path)81 redirection = self.traverse(path)
76 self.assertIs(redirection.target, None)82 self.assertIs(redirection.target, DEFAULT_REFERER)
77 self._validateNotificationContext(83 self._validateNotificationContext(
78 redirection.request, notification, level)84 redirection.request, notification, level)
7985
80 def assertNotFound(self, path):86 def assertNoLinkedBranch(self, path, use_default_referer=True):
81 self.assertRaises(NotFound, self.traverse, path)87 self.assertRaises(
88 NoLinkedBranch, self.traverse, path,
89 use_default_referer=use_default_referer)
90
91 def assertNotFound(self, path, use_default_referer=True):
92 self.assertRaises(
93 NotFound, self.traverse, path,
94 use_default_referer=use_default_referer)
8295
83 def assertRedirects(self, segments, url):96 def assertRedirects(self, segments, url):
84 redirection = self.traverse(segments)97 redirection = self.traverse(segments)
85 self.assertEqual(url, redirection.target)98 self.assertEqual(url, redirection.target)
8699
87 def traverse(self, path, first_segment):100 def traverse(self, path, first_segment, use_default_referer=True):
88 """Traverse to 'segments' using a 'LaunchpadRootNavigation' object.101 """Traverse to 'segments' using a 'LaunchpadRootNavigation' object.
89102
90 Using the Zope traversal machinery, traverse to the path given by103 Using the Zope traversal machinery, traverse to the path given by
91 'segments', starting at a `LaunchpadRootNavigation` object.104 'segments', starting at a `LaunchpadRootNavigation` object.
92105
93 :param segments: A list of path segments.106 :param segments: A list of path segments.
107 :param use_default_referer: If True, set the referer attribute in the
108 request header to DEFAULT_REFERER = "http://launchpad.dev"
109 (otherwise it remains as None)
94 :return: The object found.110 :return: The object found.
95 """111 """
96 request = LaunchpadTestRequest(112 extra = {'PATH_INFO': urlappend('/%s' % first_segment, path)}
97 PATH_INFO=urlappend('/%s' % first_segment, path))113 if use_default_referer:
114 extra['HTTP_REFERER'] = DEFAULT_REFERER
115 request = LaunchpadTestRequest(**extra)
98 segments = reversed(path.split('/'))116 segments = reversed(path.split('/'))
99 request.setTraversalStack(segments)117 request.setTraversalStack(segments)
100 traverser = LaunchpadRootNavigation(None, request=request)118 traverser = LaunchpadRootNavigation(None, request=request)
@@ -110,8 +128,9 @@
110128
111 layer = DatabaseFunctionalLayer129 layer = DatabaseFunctionalLayer
112130
113 def traverse(self, path):131 def traverse(self, path, **kwargs):
114 return super(TestBranchTraversal, self).traverse(path, '+branch')132 return super(TestBranchTraversal, self).traverse(
133 path, '+branch', **kwargs)
115134
116 def test_unique_name_traversal(self):135 def test_unique_name_traversal(self):
117 # Traversing to /+branch/<unique_name> redirects to the page for that136 # Traversing to /+branch/<unique_name> redirects to the page for that
@@ -178,6 +197,26 @@
178 non_existent, requiredMessage,197 non_existent, requiredMessage,
179 BrowserNotificationLevel.ERROR)198 BrowserNotificationLevel.ERROR)
180199
200 def test_nonexistent_product_without_referer(self):
201 # Traversing to /+branch/<no-such-product> without a referer results
202 # in a 404 error. This happens if the user hacks the URL rather than
203 # navigating via a link
204 self.assertNotFound('non-existent', use_default_referer=False)
205
206 def test_private_without_referer(self):
207 # If the development focus of a product is private and there is no
208 # referer, we will get a 404 error. This happens if the user hacks
209 # the URL rather than navigating via a link
210 branch = self.factory.makeProductBranch()
211 naked_product = removeSecurityProxy(branch.product)
212 ICanHasLinkedBranch(naked_product).setBranch(branch)
213 removeSecurityProxy(branch).private = True
214
215 any_user = self.factory.makePerson()
216 login_person(any_user)
217 self.assertNoLinkedBranch(
218 naked_product.name, use_default_referer=False)
219
181 def test_product_without_dev_focus(self):220 def test_product_without_dev_focus(self):
182 # Traversing to a product without a development focus displays a221 # Traversing to a product without a development focus displays a
183 # user message on the same page.222 # user message on the same page.