launchpad-reviewers team mailing list archive

["j.c.sackett" <jcsackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/filediff-permission-denied into lp:launchpad.

Commit message:
Fixes escaping so filediff (and possibly other views) work with +branch shortnames in loggerhead.

Requested reviews:
  Curtis Hovey (sinzui)
Related bugs:
  Bug #1055751 in Launchpad itself: "Permission denied: "Cannot create '+filediff' viewing diffs on +branch urls"
  https://bugs.launchpad.net/launchpad/+bug/1055751

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/filediff-permission-denied/+merge/133924

Summary
=======
+filediff is broken on codebrowse; a permission denied error comes up when
trying to expand the diffs on the view for a given revision, because of
traversal issues.

We do a lot of escaping and serializing of branch data as we pass it around;
how this is escaped is assumed and matters for calculation of certain
attributes, such as the branch name. The +branch shortnames do not end up
escaped the same as regular branch names in lookup, leading to traversal
issues.

Preimp
======
Spoke with Curtis Hovey. A lot.

Implementation
==============
Change the lookup of short names to escape in the same manner as regular
names.

Tests
=====
bin/test -vvct PerformLookupTestCase

QA
==
QA is listed in a bug comment from Curtis Hovey (sinzui) in the linked bug.

LoC
===
I have a substantial LoC credit.

Lint
====

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/model/tests/test_branchlookup.py
  lib/lp/code/model/branchlookup.py
-- 
https://code.launchpad.net/~jcsackett/launchpad/filediff-permission-denied/+merge/133924
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/code/model/branchlookup.py'
--- lib/lp/code/model/branchlookup.py	2012-09-28 06:25:44 +0000
+++ lib/lp/code/model/branchlookup.py	2012-11-12 13:46:27 +0000
@@ -274,7 +274,8 @@
             return (self.get(lookup['branch_id']), lookup['trailing'])
         elif lookup['type'] == 'alias':
             try:
-                return self.getByLPPath(lookup['lp_path'])
+                branch, trail = self.getByLPPath(lookup['lp_path'])
+                return branch, escape(trail)
             except (InvalidProductName, NoLinkedBranch,
                     CannotHaveLinkedBranch, NameLookupFailed,
                     InvalidNamespace):

=== modified file 'lib/lp/code/model/tests/test_branchlookup.py'
--- lib/lp/code/model/tests/test_branchlookup.py	2012-10-31 14:29:13 +0000
+++ lib/lp/code/model/tests/test_branchlookup.py	2012-11-12 13:46:27 +0000
@@ -753,3 +753,37 @@
         result = self.branch_lookup.getByLPPath(
             '%s/other/bits' % package.path)
         self.assertEqual((branch, u'other/bits'), result)
+
+
+class PerformLookupTestCase(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def makeProductBranch(self):
+        """Create a branch with aa/b/c as its unique name."""
+        owner = self.factory.makePerson(name='aa')
+        product = self.factory.makeProduct('b')
+        return self.factory.makeProductBranch(
+            owner=owner, product=product, name='c')
+
+    def test_performLookup_with_branch_name(self):
+        # performLookup works with branch unique_name and
+        # escapes the trailing path.
+        product = make_product_with_branch(self.factory)
+        branch = product.development_focus.branch
+        lookup = dict(
+            type='branch_name', unique_name=branch.unique_name,
+            trailing='foo/+filediff')
+        branch_lookup = getUtility(IBranchLookup)
+        found_branch, trailing = branch_lookup.performLookup(lookup)
+        self.assertEqual(branch, found_branch)
+        self.assertEqual('foo/%2Bfilediff', trailing)
+
+    def test_performLookup_with_alias_and_extra_path(self):
+        # PerformLookup works with aliases and escapes the trailing path.
+        product = make_product_with_branch(self.factory)
+        lookup = dict(type='alias', lp_path='%s/foo/+filediff' % product.name)
+        branch_lookup = getUtility(IBranchLookup)
+        found_branch, trailing = branch_lookup.performLookup(lookup)
+        self.assertEqual(product.development_focus.branch, found_branch)
+        self.assertEqual('foo/%2Bfilediff', trailing)