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

Proposed by Colin Watson on 2018-05-16
Status: Merged
Merged at revision: 18707
Proposed branch: lp:~cjwatson/launchpad/better-bzr-mp-diffs
Merge into: lp:launchpad
Prerequisite: lp:~cjwatson/launchpad/branch-hosting-client
Diff against target: 506 lines (+230/-34)
11 files modified
lib/lp/code/browser/branch.py (+48/-0)
lib/lp/code/browser/branchmergeproposal.py (+9/-12)
lib/lp/code/browser/configure.zcml (+5/-1)
lib/lp/code/browser/gitrepository.py (+12/-10)
lib/lp/code/browser/tests/test_branch.py (+72/-0)
lib/lp/code/browser/tests/test_branchmergeproposal.py (+19/-2)
lib/lp/code/browser/tests/test_gitrepository.py (+18/-2)
lib/lp/code/interfaces/branch.py (+10/-1)
lib/lp/code/model/branch.py (+6/-0)
lib/lp/code/tests/helpers.py (+16/-1)
lib/lp/services/webapp/publisher.py (+15/-5)
To merge this branch: bzr merge lp:~cjwatson/launchpad/better-bzr-mp-diffs
Reviewer Review Type Date Requested Status
William Grant code 2018-05-16 Approve on 2018-06-21
Review via email: mp+345705@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
18665. By Colin Watson on 2018-05-16

Update copyright.

18666. By Colin Watson on 2018-05-16

Minor simplification.

18667. By Colin Watson on 2018-06-14

Merge branch-hosting-client.

William Grant (wgrant) :
review: Approve (code)
18668. By Colin Watson on 2018-06-21

Merge branch-hosting-client, adjusting Branch.getDiff.

18669. By Colin Watson on 2018-06-21

Add a feature flag to disable the Bazaar diff proxy view.

18670. By Colin Watson on 2018-06-21

Quote filename properly in Content-Disposition header.

18671. By Colin Watson on 2018-06-22

Remove unnecessary u"" in unicode_literals file.

18672. By Colin Watson on 2018-06-22

Add branch/repository name to filename in proxied diff views.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/browser/branch.py'
2--- lib/lp/code/browser/branch.py 2018-03-16 21:20:00 +0000
3+++ lib/lp/code/browser/branch.py 2018-06-22 10:03:28 +0000
4@@ -46,6 +46,7 @@
5 providedBy,
6 )
7 from zope.publisher.interfaces import NotFound
8+from zope.publisher.interfaces.browser import IBrowserPublisher
9 from zope.schema import (
10 Bool,
11 Choice,
12@@ -136,6 +137,7 @@
13 from lp.services.webapp.breadcrumb import NameBreadcrumb
14 from lp.services.webapp.escaping import structured
15 from lp.services.webapp.interfaces import ICanonicalUrlData
16+from lp.services.webapp.publisher import DataDownloadView
17 from lp.services.webhooks.browser import WebhookTargetNavigationMixin
18 from lp.snappy.browser.hassnaps import (
19 HasSnapsMenuMixin,
20@@ -203,6 +205,22 @@
21 return None
22 return self.context.getMergeProposalByID(id)
23
24+ @stepto("+diff")
25+ def traverse_diff(self):
26+ segments = list(self.request.getTraversalStack())
27+ if len(segments) == 1:
28+ new = segments.pop()
29+ old = None
30+ self.request.stepstogo.consume()
31+ elif len(segments) == 2:
32+ new = segments.pop()
33+ old = segments.pop()
34+ self.request.stepstogo.consume()
35+ self.request.stepstogo.consume()
36+ else:
37+ return None
38+ return BranchDiffView(self.context, self.request, new, old=old)
39+
40 @stepto("+code-import")
41 def traverse_code_import(self):
42 """Traverses to the `ICodeImport` for the branch."""
43@@ -1280,3 +1298,33 @@
44 'target_branch',
45 "This branch is not mergeable into %s." %
46 target_branch.bzr_identity)
47+
48+
49+@implementer(IBrowserPublisher)
50+class BranchDiffView(DataDownloadView):
51+
52+ content_type = "text/x-patch"
53+
54+ def __init__(self, context, request, new, old=None):
55+ super(BranchDiffView, self).__init__(context, request)
56+ self.new = new
57+ self.old = old
58+
59+ def __call__(self):
60+ if getFeatureFlag(u"code.bzr.diff.disable_proxy"):
61+ self.request.response.setStatus(401)
62+ return "Proxying of branch diffs is disabled.\n"
63+ return super(BranchDiffView, self).__call__()
64+
65+ @property
66+ def filename(self):
67+ if self.old is None:
68+ return "%s_%s.diff" % (self.context.name, self.new)
69+ else:
70+ return "%s_%s_%s.diff" % (self.context.name, self.old, self.new)
71+
72+ def getBody(self):
73+ return self.context.getDiff(self.new, old=self.old)
74+
75+ def browserDefault(self, request):
76+ return self, ()
77
78=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
79--- lib/lp/code/browser/branchmergeproposal.py 2018-04-25 12:01:33 +0000
80+++ lib/lp/code/browser/branchmergeproposal.py 2018-06-22 10:03:28 +0000
81@@ -617,19 +617,16 @@
82 super(BranchMergeProposalView, self).initialize()
83 cache = IJSONRequestCache(self.request)
84 cache.objects['branch_name'] = self.context.merge_source.name
85- if IBranch.providedBy(self.context.merge_source):
86- cache.objects.update({
87- 'branch_diff_link':
88- 'https://%s/+loggerhead/%s/diff/' % (
89- config.launchpad.code_domain,
90- self.context.source_branch.unique_name),
91- })
92+ if (IBranch.providedBy(self.context.merge_source) and
93+ getFeatureFlag("code.bzr.diff.disable_proxy")):
94+ # This fallback works for public branches, but not private ones.
95+ cache.objects['branch_diff_link'] = (
96+ 'https://%s/+loggerhead/%s/diff/' % (
97+ config.launchpad.code_domain,
98+ self.context.source_branch.unique_name))
99 else:
100- cache.objects.update({
101- 'branch_diff_link':
102- canonical_url(self.context.source_git_repository) +
103- '/+diff/',
104- })
105+ cache.objects['branch_diff_link'] = (
106+ canonical_url(self.context.parent) + '/+diff/')
107 if getFeatureFlag("longpoll.merge_proposals.enabled"):
108 cache.objects['merge_proposal_event_key'] = subscribe(
109 self.context).event_key
110
111=== modified file 'lib/lp/code/browser/configure.zcml'
112--- lib/lp/code/browser/configure.zcml 2017-09-27 13:50:02 +0000
113+++ lib/lp/code/browser/configure.zcml 2018-06-22 10:03:28 +0000
114@@ -1,4 +1,4 @@
115-<!-- Copyright 2009-2017 Canonical Ltd. This software is licensed under the
116+<!-- Copyright 2009-2018 Canonical Ltd. This software is licensed under the
117 GNU Affero General Public License version 3 (see the file LICENSE).
118 -->
119
120@@ -442,6 +442,10 @@
121 class="lp.code.browser.codeimport.CodeImportEditView"
122 permission="launchpad.Edit"
123 template="../../app/templates/generic-edit.pt"/>
124+ <class class="lp.code.browser.branch.BranchDiffView">
125+ <allow interface="zope.publisher.interfaces.browser.IBrowserPublisher"
126+ attributes="__call__"/>
127+ </class>
128 <browser:page
129 name="+delete"
130 for="lp.code.interfaces.branch.IBranch"
131
132=== modified file 'lib/lp/code/browser/gitrepository.py'
133--- lib/lp/code/browser/gitrepository.py 2016-10-15 01:14:32 +0000
134+++ lib/lp/code/browser/gitrepository.py 2018-06-22 10:03:28 +0000
135@@ -97,6 +97,7 @@
136 from lp.services.webapp.breadcrumb import Breadcrumb
137 from lp.services.webapp.escaping import structured
138 from lp.services.webapp.interfaces import ICanonicalUrlData
139+from lp.services.webapp.publisher import DataDownloadView
140 from lp.services.webhooks.browser import WebhookTargetNavigationMixin
141 from lp.snappy.browser.hassnaps import HasSnapsViewMixin
142
143@@ -638,23 +639,24 @@
144
145
146 @implementer(IBrowserPublisher)
147-class GitRepositoryDiffView:
148+class GitRepositoryDiffView(DataDownloadView):
149+
150+ content_type = "text/x-patch"
151+ charset = "UTF-8"
152
153 def __init__(self, context, request, old, new):
154+ super(GitRepositoryDiffView, self).__init__(context, request)
155 self.context = context
156 self.request = request
157 self.old = old
158 self.new = new
159
160- def __call__(self):
161- content = self.context.getDiff(self.old, self.new)
162- filename = "%s_%s.diff" % (self.old, self.new)
163- self.request.response.setHeader(
164- "Content-Type", 'text/x-patch; charset="UTF-8"')
165- self.request.response.setHeader("Content-Length", str(len(content)))
166- self.request.response.setHeader(
167- "Content-Disposition", "attachment; filename=%s" % filename)
168- return content
169+ @property
170+ def filename(self):
171+ return "%s_%s_%s.diff" % (self.context.name, self.old, self.new)
172+
173+ def getBody(self):
174+ return self.context.getDiff(self.old, self.new)
175
176 def browserDefault(self, request):
177 return self, ()
178
179=== modified file 'lib/lp/code/browser/tests/test_branch.py'
180--- lib/lp/code/browser/tests/test_branch.py 2018-02-01 18:38:24 +0000
181+++ lib/lp/code/browser/tests/test_branch.py 2018-06-22 10:03:28 +0000
182@@ -12,10 +12,12 @@
183
184 from fixtures import FakeLogger
185 import pytz
186+from six.moves.urllib_error import HTTPError
187 from storm.store import Store
188 from testtools.matchers import Equals
189 from zope.component import getUtility
190 from zope.publisher.interfaces import NotFound
191+from zope.security.interfaces import Unauthorized
192 from zope.security.proxy import removeSecurityProxy
193
194 from lp.app.enums import InformationType
195@@ -32,12 +34,14 @@
196 RepositoryFormat,
197 )
198 from lp.code.enums import BranchType
199+from lp.code.tests.helpers import BranchHostingFixture
200 from lp.registry.enums import BranchSharingPolicy
201 from lp.registry.interfaces.accesspolicy import IAccessPolicySource
202 from lp.registry.interfaces.person import PersonVisibility
203 from lp.services.beautifulsoup import BeautifulSoup
204 from lp.services.config import config
205 from lp.services.database.constants import UTC_NOW
206+from lp.services.features.testing import FeatureFixture
207 from lp.services.helpers import truncate_text
208 from lp.services.webapp.publisher import canonical_url
209 from lp.services.webapp.servers import LaunchpadTestRequest
210@@ -1220,3 +1224,71 @@
211 InformationType.USERDATA.description, description.renderContents())
212 self.assertIsNotNone(
213 soup.find('a', id='privacy-link', attrs={'href': edit_url}))
214+
215+
216+class TestBranchDiffView(BrowserTestCase):
217+
218+ layer = DatabaseFunctionalLayer
219+
220+ def test_feature_disabled(self):
221+ self.useFixture(FeatureFixture({"code.bzr.diff.disable_proxy": "on"}))
222+ hosting_fixture = self.useFixture(BranchHostingFixture())
223+ person = self.factory.makePerson()
224+ branch = self.factory.makeBranch(owner=person)
225+ e = self.assertRaises(
226+ HTTPError, self.getUserBrowser,
227+ canonical_url(branch) + "/+diff/2/1")
228+ self.assertEqual(401, e.code)
229+ self.assertEqual("Proxying of branch diffs is disabled.\n", e.read())
230+ self.assertEqual([], hosting_fixture.getDiff.calls)
231+
232+ def test_render(self):
233+ diff = b"A fake diff\n"
234+ hosting_fixture = self.useFixture(BranchHostingFixture(diff=diff))
235+ person = self.factory.makePerson()
236+ branch = self.factory.makeBranch(owner=person, name="some-branch")
237+ browser = self.getUserBrowser(
238+ canonical_url(branch) + "/+diff/2/1")
239+ with person_logged_in(person):
240+ self.assertEqual(
241+ [((branch.id, "2"), {"old": "1"})],
242+ hosting_fixture.getDiff.calls)
243+ self.assertEqual("text/x-patch", browser.headers["Content-Type"])
244+ self.assertEqual(str(len(diff)), browser.headers["Content-Length"])
245+ self.assertEqual(
246+ 'attachment; filename="some-branch_1_2.diff"',
247+ browser.headers["Content-Disposition"])
248+ self.assertEqual(diff, browser.contents)
249+
250+ def test_security(self):
251+ # A user who can see a private branch can fetch diffs from it, but
252+ # other users cannot.
253+ diff = b"A fake diff\n"
254+ self.useFixture(BranchHostingFixture(diff=diff))
255+ person = self.factory.makePerson()
256+ project = self.factory.makeProduct(
257+ owner=person, information_type=InformationType.PROPRIETARY)
258+ with person_logged_in(person):
259+ branch = self.factory.makeBranch(
260+ owner=person, product=project,
261+ information_type=InformationType.PROPRIETARY)
262+ branch_url = canonical_url(branch)
263+ browser = self.getUserBrowser(branch_url + "/+diff/2/1", user=person)
264+ self.assertEqual(diff, browser.contents)
265+ self.useFixture(FakeLogger())
266+ self.assertRaises(
267+ Unauthorized, self.getUserBrowser, branch_url + "/+diff/2/1")
268+
269+ def test_filename_quoting(self):
270+ # If we construct revisions containing metacharacters and somehow
271+ # manage to get that past the hosting service, the Content-Disposition
272+ # header is quoted properly.
273+ diff = b"A fake diff\n"
274+ self.useFixture(BranchHostingFixture(diff=diff))
275+ person = self.factory.makePerson()
276+ branch = self.factory.makeBranch(owner=person, name="some-branch")
277+ browser = self.getUserBrowser(
278+ canonical_url(branch) + '/+diff/foo"/"bar')
279+ self.assertEqual(
280+ r'attachment; filename="some-branch_\"bar_foo\".diff"',
281+ browser.headers["Content-Disposition"])
282
283=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
284--- lib/lp/code/browser/tests/test_branchmergeproposal.py 2018-04-25 12:01:33 +0000
285+++ lib/lp/code/browser/tests/test_branchmergeproposal.py 2018-06-22 10:03:28 +0000
286@@ -89,6 +89,7 @@
287 PersonVisibility,
288 TeamMembershipPolicy,
289 )
290+from lp.services.features.testing import FeatureFixture
291 from lp.services.librarian.interfaces.client import LibrarianServerError
292 from lp.services.messages.model.message import MessageSet
293 from lp.services.timeout import TimeoutError
294@@ -1634,8 +1635,24 @@
295 "on 2015-01-01" % sha1, extract_text(tag))
296
297 def test_client_cache_bzr(self):
298- # For Bazaar, the client cache contains the branch name and a
299- # loggerhead-based diff link.
300+ # For Bazaar, the client cache contains the branch name and a link
301+ # to the Loggerhead diff via the webapp.
302+ bmp = self.factory.makeBranchMergeProposal()
303+ view = create_initialized_view(bmp, '+index')
304+ cache = IJSONRequestCache(view.request)
305+ self.assertThat(cache.objects, ContainsDict({
306+ 'branch_name': Equals(bmp.source_branch.name),
307+ 'branch_diff_link': Equals(
308+ 'http://code.launchpad.dev/%s/+diff/' %
309+ bmp.source_branch.unique_name),
310+ }))
311+
312+ def test_client_cache_bzr_proxy_feature_disabled(self):
313+ # For Bazaar, a feature flag can be set to cause the client cache to
314+ # contain a direct link to the Loggerhead diff rather than via the
315+ # webapp. (This only works for public branches.)
316+ self.useFixture(
317+ FeatureFixture({u'code.bzr.diff.disable_proxy': u'on'}))
318 bmp = self.factory.makeBranchMergeProposal()
319 view = create_initialized_view(bmp, '+index')
320 cache = IJSONRequestCache(view.request)
321
322=== modified file 'lib/lp/code/browser/tests/test_gitrepository.py'
323--- lib/lp/code/browser/tests/test_gitrepository.py 2018-02-01 18:38:24 +0000
324+++ lib/lp/code/browser/tests/test_gitrepository.py 2018-06-22 10:03:28 +0000
325@@ -875,7 +875,8 @@
326 hosting_fixture = self.useFixture(GitHostingFixture(
327 diff={"patch": diff}))
328 person = self.factory.makePerson()
329- repository = self.factory.makeGitRepository(owner=person)
330+ repository = self.factory.makeGitRepository(
331+ owner=person, name="some-repository")
332 browser = self.getUserBrowser(
333 canonical_url(repository) + "/+diff/0123456/0123456^")
334 with person_logged_in(person):
335@@ -886,7 +887,7 @@
336 'text/x-patch;charset=UTF-8', browser.headers["Content-Type"])
337 self.assertEqual(str(len(diff)), browser.headers["Content-Length"])
338 self.assertEqual(
339- "attachment; filename=0123456^_0123456.diff",
340+ 'attachment; filename="some-repository_0123456^_0123456.diff"',
341 browser.headers["Content-Disposition"])
342 self.assertEqual(diff, browser.contents)
343
344@@ -911,6 +912,21 @@
345 Unauthorized, self.getUserBrowser,
346 repository_url + "/+diff/0123456/0123456^")
347
348+ def test_filename_quoting(self):
349+ # If we construct revisions containing metacharacters and somehow
350+ # manage to get that past the hosting service, the
351+ # Content-Disposition header is quoted properly.
352+ diff = "A fake diff\n"
353+ self.useFixture(GitHostingFixture(diff={"patch": diff}))
354+ person = self.factory.makePerson()
355+ repository = self.factory.makeGitRepository(
356+ owner=person, name="some-repository")
357+ browser = self.getUserBrowser(
358+ canonical_url(repository) + '/+diff/foo"/"bar')
359+ self.assertEqual(
360+ r'attachment; filename="some-repository_\"bar_foo\".diff"',
361+ browser.headers["Content-Disposition"])
362+
363
364 class TestGitRepositoryDeletionView(BrowserTestCase):
365
366
367=== modified file 'lib/lp/code/interfaces/branch.py'
368--- lib/lp/code/interfaces/branch.py 2018-03-06 00:59:06 +0000
369+++ lib/lp/code/interfaces/branch.py 2018-06-22 10:03:28 +0000
370@@ -1,4 +1,4 @@
371-# Copyright 2009-2016 Canonical Ltd. This software is licensed under the
372+# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
373 # GNU Affero General Public License version 3 (see the file LICENSE).
374
375 """Branch interfaces."""
376@@ -765,6 +765,15 @@
377 :param launchbag: `ILaunchBag`.
378 """
379
380+ def getDiff(new, old):
381+ """Get the diff between two revisions in this branch.
382+
383+ :param new: The new revno or revision ID.
384+ :param old: The old revno or revision ID. Defaults to the parent
385+ revision of `new`.
386+ :return: The diff as a byte string.
387+ """
388+
389 @export_read_operation()
390 @operation_for_version('beta')
391 def canBeDeleted():
392
393=== modified file 'lib/lp/code/model/branch.py'
394--- lib/lp/code/model/branch.py 2018-03-06 00:59:06 +0000
395+++ lib/lp/code/model/branch.py 2018-06-22 10:03:28 +0000
396@@ -105,6 +105,7 @@
397 WrongNumberOfReviewTypeArguments,
398 )
399 from lp.code.interfaces.branchcollection import IAllBranches
400+from lp.code.interfaces.branchhosting import IBranchHostingClient
401 from lp.code.interfaces.branchlookup import IBranchLookup
402 from lp.code.interfaces.branchmergeproposal import (
403 BRANCH_MERGE_PROPOSAL_FINAL_STATES,
404@@ -782,6 +783,11 @@
405 RevisionAuthor, revisions, ['revision_author_id'])
406 return DecoratedResultSet(result, pre_iter_hook=eager_load)
407
408+ def getDiff(self, new, old=None):
409+ """See `IBranch`."""
410+ hosting_client = getUtility(IBranchHostingClient)
411+ return hosting_client.getDiff(self.id, new, old=old)
412+
413 def canBeDeleted(self):
414 """See `IBranch`."""
415 if ((len(self.deletionRequirements()) != 0) or not
416
417=== modified file 'lib/lp/code/tests/helpers.py'
418--- lib/lp/code/tests/helpers.py 2017-12-19 17:16:38 +0000
419+++ lib/lp/code/tests/helpers.py 2018-06-22 10:03:28 +0000
420@@ -1,4 +1,4 @@
421-# Copyright 2009-2017 Canonical Ltd. This software is licensed under the
422+# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
423 # GNU Affero General Public License version 3 (see the file LICENSE).
424
425 """Helper functions for code testing live here."""
426@@ -8,6 +8,7 @@
427 __metaclass__ = type
428 __all__ = [
429 'add_revision_to_branch',
430+ 'BranchHostingFixture',
431 'get_non_existant_source_package_branch_unique_name',
432 'GitHostingFixture',
433 'make_erics_fooix_project',
434@@ -35,6 +36,7 @@
435 )
436
437 from lp.app.enums import InformationType
438+from lp.code.interfaces.branchhosting import IBranchHostingClient
439 from lp.code.interfaces.branchmergeproposal import (
440 IBranchMergeProposalJobSource,
441 )
442@@ -301,6 +303,19 @@
443 c.execute('delete from branch')
444
445
446+class BranchHostingFixture(fixtures.Fixture):
447+ """A fixture that temporarily registers a fake Bazaar hosting client."""
448+
449+ def __init__(self, diff=None, inventory=None, blob=None):
450+ self.create = FakeMethod()
451+ self.getDiff = FakeMethod(result=diff or {})
452+ self.getInventory = FakeMethod(result=inventory or {})
453+ self.getBlob = FakeMethod(result=blob)
454+
455+ def _setUp(self):
456+ self.useFixture(ZopeUtilityFixture(self, IBranchHostingClient))
457+
458+
459 class GitHostingFixture(fixtures.Fixture):
460 """A fixture that temporarily registers a fake Git hosting client."""
461
462
463=== modified file 'lib/lp/services/webapp/publisher.py'
464--- lib/lp/services/webapp/publisher.py 2017-11-22 13:05:11 +0000
465+++ lib/lp/services/webapp/publisher.py 2018-06-22 10:03:28 +0000
466@@ -29,6 +29,7 @@
467 from cgi import FieldStorage
468 import httplib
469 import re
470+from wsgiref.headers import Headers
471
472 from lazr.restful import (
473 EntryResource,
474@@ -233,9 +234,12 @@
475 class DataDownloadView:
476 """Download data without templating.
477
478- Subclasses must provide getBody, content_type and filename.
479+ Subclasses must provide getBody, content_type and filename, and may
480+ provide charset.
481 """
482
483+ charset = None
484+
485 def __init__(self, context, request):
486 self.context = context
487 self.request = request
488@@ -246,10 +250,16 @@
489 It is not necessary to supply Content-length, because this is added by
490 the caller.
491 """
492- self.request.response.setHeader('Content-Type', self.content_type)
493- self.request.response.setHeader(
494- 'Content-Disposition', 'attachment; filename="%s"' % (
495- self.filename))
496+ headers = Headers([])
497+ content_type_params = {}
498+ if self.charset is not None:
499+ content_type_params['charset'] = self.charset
500+ headers.add_header(
501+ 'Content-Type', self.content_type, **content_type_params)
502+ headers.add_header(
503+ 'Content-Disposition', 'attachment', filename=self.filename)
504+ for key, value in headers.items():
505+ self.request.response.setHeader(key, value)
506 return self.getBody()
507
508