Merge lp:~cjwatson/launchpad/better-bzr-mp-diffs into lp:launchpad

[Colin Watson]

Proxy loggerhead branch diffs through the webapp, allowing AJAX MP revision diffs to work for private branches.

This is the same approach as we already use for Git, built on top of my recent work to expose a suitable loggerhead API on a private port.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2018-03-16 21:20:00 +0000
+++ lib/lp/code/browser/branch.py	2018-06-22 10:03:28 +0000
@@ -46,6 +46,7 @@
     providedBy,
     )
 from zope.publisher.interfaces import NotFound
+from zope.publisher.interfaces.browser import IBrowserPublisher
 from zope.schema import (
     Bool,
     Choice,
@@ -136,6 +137,7 @@
 from lp.services.webapp.breadcrumb import NameBreadcrumb
 from lp.services.webapp.escaping import structured
 from lp.services.webapp.interfaces import ICanonicalUrlData
+from lp.services.webapp.publisher import DataDownloadView
 from lp.services.webhooks.browser import WebhookTargetNavigationMixin
 from lp.snappy.browser.hassnaps import (
     HasSnapsMenuMixin,
@@ -203,6 +205,22 @@
             return None
         return self.context.getMergeProposalByID(id)
 
+    @stepto("+diff")
+    def traverse_diff(self):
+        segments = list(self.request.getTraversalStack())
+        if len(segments) == 1:
+            new = segments.pop()
+            old = None
+            self.request.stepstogo.consume()
+        elif len(segments) == 2:
+            new = segments.pop()
+            old = segments.pop()
+            self.request.stepstogo.consume()
+            self.request.stepstogo.consume()
+        else:
+            return None
+        return BranchDiffView(self.context, self.request, new, old=old)
+
     @stepto("+code-import")
     def traverse_code_import(self):
         """Traverses to the `ICodeImport` for the branch."""
@@ -1280,3 +1298,33 @@
                     'target_branch',
                     "This branch is not mergeable into %s." %
                     target_branch.bzr_identity)
+
+
+@implementer(IBrowserPublisher)
+class BranchDiffView(DataDownloadView):
+
+    content_type = "text/x-patch"
+
+    def __init__(self, context, request, new, old=None):
+        super(BranchDiffView, self).__init__(context, request)
+        self.new = new
+        self.old = old
+
+    def __call__(self):
+        if getFeatureFlag(u"code.bzr.diff.disable_proxy"):
+            self.request.response.setStatus(401)
+            return "Proxying of branch diffs is disabled.\n"
+        return super(BranchDiffView, self).__call__()
+
+    @property
+    def filename(self):
+        if self.old is None:
+            return "%s_%s.diff" % (self.context.name, self.new)
+        else:
+            return "%s_%s_%s.diff" % (self.context.name, self.old, self.new)
+
+    def getBody(self):
+        return self.context.getDiff(self.new, old=self.old)
+
+    def browserDefault(self, request):
+        return self, ()

=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py	2018-04-25 12:01:33 +0000
+++ lib/lp/code/browser/branchmergeproposal.py	2018-06-22 10:03:28 +0000
@@ -617,19 +617,16 @@
         super(BranchMergeProposalView, self).initialize()
         cache = IJSONRequestCache(self.request)
         cache.objects['branch_name'] = self.context.merge_source.name
-        if IBranch.providedBy(self.context.merge_source):
-            cache.objects.update({
-                'branch_diff_link':
-                    'https://%s/+loggerhead/%s/diff/' % (
-                        config.launchpad.code_domain,
-                        self.context.source_branch.unique_name),
-                })
+        if (IBranch.providedBy(self.context.merge_source) and
+                getFeatureFlag("code.bzr.diff.disable_proxy")):
+            # This fallback works for public branches, but not private ones.
+            cache.objects['branch_diff_link'] = (
+                'https://%s/+loggerhead/%s/diff/' % (
+                    config.launchpad.code_domain,
+                    self.context.source_branch.unique_name))
         else:
-            cache.objects.update({
-                'branch_diff_link':
-                    canonical_url(self.context.source_git_repository) +
-                    '/+diff/',
-                })
+            cache.objects['branch_diff_link'] = (
+                canonical_url(self.context.parent) + '/+diff/')
         if getFeatureFlag("longpoll.merge_proposals.enabled"):
             cache.objects['merge_proposal_event_key'] = subscribe(
                 self.context).event_key

=== modified file 'lib/lp/code/browser/configure.zcml'
--- lib/lp/code/browser/configure.zcml	2017-09-27 13:50:02 +0000
+++ lib/lp/code/browser/configure.zcml	2018-06-22 10:03:28 +0000
@@ -1,4 +1,4 @@
-<!-- Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
+<!-- Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
      GNU Affero General Public License version 3 (see the file LICENSE).
 -->
 
@@ -442,6 +442,10 @@
         class="lp.code.browser.codeimport.CodeImportEditView"
         permission="launchpad.Edit"
         template="../../app/templates/generic-edit.pt"/>
+    <class class="lp.code.browser.branch.BranchDiffView">
+        <allow interface="zope.publisher.interfaces.browser.IBrowserPublisher"
+               attributes="__call__"/>
+    </class>
     <browser:page
         name="+delete"
         for="lp.code.interfaces.branch.IBranch"

=== modified file 'lib/lp/code/browser/gitrepository.py'
--- lib/lp/code/browser/gitrepository.py	2016-10-15 01:14:32 +0000
+++ lib/lp/code/browser/gitrepository.py	2018-06-22 10:03:28 +0000
@@ -97,6 +97,7 @@
 from lp.services.webapp.breadcrumb import Breadcrumb
 from lp.services.webapp.escaping import structured
 from lp.services.webapp.interfaces import ICanonicalUrlData
+from lp.services.webapp.publisher import DataDownloadView
 from lp.services.webhooks.browser import WebhookTargetNavigationMixin
 from lp.snappy.browser.hassnaps import HasSnapsViewMixin
 
@@ -638,23 +639,24 @@
 
 
 @implementer(IBrowserPublisher)
-class GitRepositoryDiffView:
+class GitRepositoryDiffView(DataDownloadView):
+
+    content_type = "text/x-patch"
+    charset = "UTF-8"
 
     def __init__(self, context, request, old, new):
+        super(GitRepositoryDiffView, self).__init__(context, request)
         self.context = context
         self.request = request
         self.old = old
         self.new = new
 
-    def __call__(self):
-        content = self.context.getDiff(self.old, self.new)
-        filename = "%s_%s.diff" % (self.old, self.new)
-        self.request.response.setHeader(
-            "Content-Type", 'text/x-patch; charset="UTF-8"')
-        self.request.response.setHeader("Content-Length", str(len(content)))
-        self.request.response.setHeader(
-            "Content-Disposition", "attachment; filename=%s" % filename)
-        return content
+    @property
+    def filename(self):
+        return "%s_%s_%s.diff" % (self.context.name, self.old, self.new)
+
+    def getBody(self):
+        return self.context.getDiff(self.old, self.new)
 
     def browserDefault(self, request):
         return self, ()

=== modified file 'lib/lp/code/browser/tests/test_branch.py'
--- lib/lp/code/browser/tests/test_branch.py	2018-02-01 18:38:24 +0000
+++ lib/lp/code/browser/tests/test_branch.py	2018-06-22 10:03:28 +0000
@@ -12,10 +12,12 @@
 
 from fixtures import FakeLogger
 import pytz
+from six.moves.urllib_error import HTTPError
 from storm.store import Store
 from testtools.matchers import Equals
 from zope.component import getUtility
 from zope.publisher.interfaces import NotFound
+from zope.security.interfaces import Unauthorized
 from zope.security.proxy import removeSecurityProxy
 
 from lp.app.enums import InformationType
@@ -32,12 +34,14 @@
     RepositoryFormat,
     )
 from lp.code.enums import BranchType
+from lp.code.tests.helpers import BranchHostingFixture
 from lp.registry.enums import BranchSharingPolicy
 from lp.registry.interfaces.accesspolicy import IAccessPolicySource
 from lp.registry.interfaces.person import PersonVisibility
 from lp.services.beautifulsoup import BeautifulSoup
 from lp.services.config import config
 from lp.services.database.constants import UTC_NOW
+from lp.services.features.testing import FeatureFixture
 from lp.services.helpers import truncate_text
 from lp.services.webapp.publisher import canonical_url
 from lp.services.webapp.servers import LaunchpadTestRequest
@@ -1220,3 +1224,71 @@
             InformationType.USERDATA.description, description.renderContents())
         self.assertIsNotNone(
             soup.find('a', id='privacy-link', attrs={'href': edit_url}))
+
+
+class TestBranchDiffView(BrowserTestCase):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_feature_disabled(self):
+        self.useFixture(FeatureFixture({"code.bzr.diff.disable_proxy": "on"}))
+        hosting_fixture = self.useFixture(BranchHostingFixture())
+        person = self.factory.makePerson()
+        branch = self.factory.makeBranch(owner=person)
+        e = self.assertRaises(
+            HTTPError, self.getUserBrowser,
+            canonical_url(branch) + "/+diff/2/1")
+        self.assertEqual(401, e.code)
+        self.assertEqual("Proxying of branch diffs is disabled.\n", e.read())
+        self.assertEqual([], hosting_fixture.getDiff.calls)
+
+    def test_render(self):
+        diff = b"A fake diff\n"
+        hosting_fixture = self.useFixture(BranchHostingFixture(diff=diff))
+        person = self.factory.makePerson()
+        branch = self.factory.makeBranch(owner=person, name="some-branch")
+        browser = self.getUserBrowser(
+            canonical_url(branch) + "/+diff/2/1")
+        with person_logged_in(person):
+            self.assertEqual(
+                [((branch.id, "2"), {"old": "1"})],
+                hosting_fixture.getDiff.calls)
+        self.assertEqual("text/x-patch", browser.headers["Content-Type"])
+        self.assertEqual(str(len(diff)), browser.headers["Content-Length"])
+        self.assertEqual(
+            'attachment; filename="some-branch_1_2.diff"',
+            browser.headers["Content-Disposition"])
+        self.assertEqual(diff, browser.contents)
+
+    def test_security(self):
+        # A user who can see a private branch can fetch diffs from it, but
+        # other users cannot.
+        diff = b"A fake diff\n"
+        self.useFixture(BranchHostingFixture(diff=diff))
+        person = self.factory.makePerson()
+        project = self.factory.makeProduct(
+            owner=person, information_type=InformationType.PROPRIETARY)
+        with person_logged_in(person):
+            branch = self.factory.makeBranch(
+                owner=person, product=project,
+                information_type=InformationType.PROPRIETARY)
+            branch_url = canonical_url(branch)
+        browser = self.getUserBrowser(branch_url + "/+diff/2/1", user=person)
+        self.assertEqual(diff, browser.contents)
+        self.useFixture(FakeLogger())
+        self.assertRaises(
+            Unauthorized, self.getUserBrowser, branch_url + "/+diff/2/1")
+
+    def test_filename_quoting(self):
+        # If we construct revisions containing metacharacters and somehow
+        # manage to get that past the hosting service, the Content-Disposition
+        # header is quoted properly.
+        diff = b"A fake diff\n"
+        self.useFixture(BranchHostingFixture(diff=diff))
+        person = self.factory.makePerson()
+        branch = self.factory.makeBranch(owner=person, name="some-branch")
+        browser = self.getUserBrowser(
+            canonical_url(branch) + '/+diff/foo"/"bar')
+        self.assertEqual(
+            r'attachment; filename="some-branch_\"bar_foo\".diff"',
+            browser.headers["Content-Disposition"])

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py	2018-04-25 12:01:33 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2018-06-22 10:03:28 +0000
@@ -89,6 +89,7 @@
     PersonVisibility,
     TeamMembershipPolicy,
     )
+from lp.services.features.testing import FeatureFixture
 from lp.services.librarian.interfaces.client import LibrarianServerError
 from lp.services.messages.model.message import MessageSet
 from lp.services.timeout import TimeoutError
@@ -1634,8 +1635,24 @@
             "on 2015-01-01" % sha1, extract_text(tag))
 
     def test_client_cache_bzr(self):
-        # For Bazaar, the client cache contains the branch name and a
-        # loggerhead-based diff link.
+        # For Bazaar, the client cache contains the branch name and a link
+        # to the Loggerhead diff via the webapp.
+        bmp = self.factory.makeBranchMergeProposal()
+        view = create_initialized_view(bmp, '+index')
+        cache = IJSONRequestCache(view.request)
+        self.assertThat(cache.objects, ContainsDict({
+            'branch_name': Equals(bmp.source_branch.name),
+            'branch_diff_link': Equals(
+                'http://code.launchpad.dev/%s/+diff/' %
+                bmp.source_branch.unique_name),
+            }))
+
+    def test_client_cache_bzr_proxy_feature_disabled(self):
+        # For Bazaar, a feature flag can be set to cause the client cache to
+        # contain a direct link to the Loggerhead diff rather than via the
+        # webapp.  (This only works for public branches.)
+        self.useFixture(
+            FeatureFixture({u'code.bzr.diff.disable_proxy': u'on'}))
         bmp = self.factory.makeBranchMergeProposal()
         view = create_initialized_view(bmp, '+index')
         cache = IJSONRequestCache(view.request)

=== modified file 'lib/lp/code/browser/tests/test_gitrepository.py'
--- lib/lp/code/browser/tests/test_gitrepository.py	2018-02-01 18:38:24 +0000
+++ lib/lp/code/browser/tests/test_gitrepository.py	2018-06-22 10:03:28 +0000
@@ -875,7 +875,8 @@
         hosting_fixture = self.useFixture(GitHostingFixture(
             diff={"patch": diff}))
         person = self.factory.makePerson()
-        repository = self.factory.makeGitRepository(owner=person)
+        repository = self.factory.makeGitRepository(
+            owner=person, name="some-repository")
         browser = self.getUserBrowser(
             canonical_url(repository) + "/+diff/0123456/0123456^")
         with person_logged_in(person):
@@ -886,7 +887,7 @@
             'text/x-patch;charset=UTF-8', browser.headers["Content-Type"])
         self.assertEqual(str(len(diff)), browser.headers["Content-Length"])
         self.assertEqual(
-            "attachment; filename=0123456^_0123456.diff",
+            'attachment; filename="some-repository_0123456^_0123456.diff"',
             browser.headers["Content-Disposition"])
         self.assertEqual(diff, browser.contents)
 
@@ -911,6 +912,21 @@
             Unauthorized, self.getUserBrowser,
             repository_url + "/+diff/0123456/0123456^")
 
+    def test_filename_quoting(self):
+        # If we construct revisions containing metacharacters and somehow
+        # manage to get that past the hosting service, the
+        # Content-Disposition header is quoted properly.
+        diff = "A fake diff\n"
+        self.useFixture(GitHostingFixture(diff={"patch": diff}))
+        person = self.factory.makePerson()
+        repository = self.factory.makeGitRepository(
+            owner=person, name="some-repository")
+        browser = self.getUserBrowser(
+            canonical_url(repository) + '/+diff/foo"/"bar')
+        self.assertEqual(
+            r'attachment; filename="some-repository_\"bar_foo\".diff"',
+            browser.headers["Content-Disposition"])
+
 
 class TestGitRepositoryDeletionView(BrowserTestCase):
 

=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2018-03-06 00:59:06 +0000
+++ lib/lp/code/interfaces/branch.py	2018-06-22 10:03:28 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Branch interfaces."""
@@ -765,6 +765,15 @@
         :param launchbag: `ILaunchBag`.
         """
 
+    def getDiff(new, old):
+        """Get the diff between two revisions in this branch.
+
+        :param new: The new revno or revision ID.
+        :param old: The old revno or revision ID.  Defaults to the parent
+            revision of `new`.
+        :return: The diff as a byte string.
+        """
+
     @export_read_operation()
     @operation_for_version('beta')
     def canBeDeleted():

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2018-03-06 00:59:06 +0000
+++ lib/lp/code/model/branch.py	2018-06-22 10:03:28 +0000
@@ -105,6 +105,7 @@
     WrongNumberOfReviewTypeArguments,
     )
 from lp.code.interfaces.branchcollection import IAllBranches
+from lp.code.interfaces.branchhosting import IBranchHostingClient
 from lp.code.interfaces.branchlookup import IBranchLookup
 from lp.code.interfaces.branchmergeproposal import (
     BRANCH_MERGE_PROPOSAL_FINAL_STATES,
@@ -782,6 +783,11 @@
                 RevisionAuthor, revisions, ['revision_author_id'])
         return DecoratedResultSet(result, pre_iter_hook=eager_load)
 
+    def getDiff(self, new, old=None):
+        """See `IBranch`."""
+        hosting_client = getUtility(IBranchHostingClient)
+        return hosting_client.getDiff(self.id, new, old=old)
+
     def canBeDeleted(self):
         """See `IBranch`."""
         if ((len(self.deletionRequirements()) != 0) or not

=== modified file 'lib/lp/code/tests/helpers.py'
--- lib/lp/code/tests/helpers.py	2017-12-19 17:16:38 +0000
+++ lib/lp/code/tests/helpers.py	2018-06-22 10:03:28 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Helper functions for code testing live here."""
@@ -8,6 +8,7 @@
 __metaclass__ = type
 __all__ = [
     'add_revision_to_branch',
+    'BranchHostingFixture',
     'get_non_existant_source_package_branch_unique_name',
     'GitHostingFixture',
     'make_erics_fooix_project',
@@ -35,6 +36,7 @@
     )
 
 from lp.app.enums import InformationType
+from lp.code.interfaces.branchhosting import IBranchHostingClient
 from lp.code.interfaces.branchmergeproposal import (
     IBranchMergeProposalJobSource,
     )
@@ -301,6 +303,19 @@
     c.execute('delete from branch')
 
 
+class BranchHostingFixture(fixtures.Fixture):
+    """A fixture that temporarily registers a fake Bazaar hosting client."""
+
+    def __init__(self, diff=None, inventory=None, blob=None):
+        self.create = FakeMethod()
+        self.getDiff = FakeMethod(result=diff or {})
+        self.getInventory = FakeMethod(result=inventory or {})
+        self.getBlob = FakeMethod(result=blob)
+
+    def _setUp(self):
+        self.useFixture(ZopeUtilityFixture(self, IBranchHostingClient))
+
+
 class GitHostingFixture(fixtures.Fixture):
     """A fixture that temporarily registers a fake Git hosting client."""
 

=== modified file 'lib/lp/services/webapp/publisher.py'
--- lib/lp/services/webapp/publisher.py	2017-11-22 13:05:11 +0000
+++ lib/lp/services/webapp/publisher.py	2018-06-22 10:03:28 +0000
@@ -29,6 +29,7 @@
 from cgi import FieldStorage
 import httplib
 import re
+from wsgiref.headers import Headers
 
 from lazr.restful import (
     EntryResource,
@@ -233,9 +234,12 @@
 class DataDownloadView:
     """Download data without templating.
 
-    Subclasses must provide getBody, content_type and filename.
+    Subclasses must provide getBody, content_type and filename, and may
+    provide charset.
     """
 
+    charset = None
+
     def __init__(self, context, request):
         self.context = context
         self.request = request
@@ -246,10 +250,16 @@
         It is not necessary to supply Content-length, because this is added by
         the caller.
         """
-        self.request.response.setHeader('Content-Type', self.content_type)
-        self.request.response.setHeader(
-            'Content-Disposition', 'attachment; filename="%s"' % (
-             self.filename))
+        headers = Headers([])
+        content_type_params = {}
+        if self.charset is not None:
+            content_type_params['charset'] = self.charset
+        headers.add_header(
+            'Content-Type', self.content_type, **content_type_params)
+        headers.add_header(
+            'Content-Disposition', 'attachment', filename=self.filename)
+        for key, value in headers.items():
+            self.request.response.setHeader(key, value)
         return self.getBody()