Launchpad itself

Merge lp:~abentley/launchpad/restricted-diffs into lp:launchpad

  1. restricted-diffs
  2. Merge into devel
Proposed by Aaron Bentley
Status: Merged
Merged at revision: not available
Proposed branch: lp:~abentley/launchpad/restricted-diffs
Merge into: lp:launchpad
Diff against target: 337 lines (+101/-21)
10 files modified
lib/canonical/launchpad/security.py (+24/-0)
lib/lp/code/browser/configure.zcml (+5/-0)
lib/lp/code/browser/diff.py (+10/-1)
lib/lp/code/browser/tests/test_tales.py (+16/-15)
lib/lp/code/configure.zcml (+3/-1)
lib/lp/code/interfaces/diff.py (+3/-0)
lib/lp/code/model/diff.py (+10/-2)
lib/lp/code/model/tests/test_diff.py (+13/-0)
lib/lp/code/stories/branches/xx-branchmergeproposals.txt (+15/-0)
lib/lp/code/templates/branchmergeproposal-diff.pt (+2/-2)
To merge this branch: bzr merge lp:~abentley/launchpad/restricted-diffs
Reviewer Review Type Date Requested Status
Michael Nelson (community) code Approve
Review via email: mp+19607@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Aaron Bentley (abentley) wrote :

= Summary =
Fix bug #328271, "Use the restricted librarian for diffs for private branches".

== Proposed fix ==
Use the restricted librarian for diffs of all branches, and let the standard
security mechanisms decide who can view the diffs.

== Pre-implementation notes ==
None

== Implementation details ==
PreviewDiffs are now accessible via the +files path segment, with the same mechanism as Launchpad components.

IPreviewDiff access was allowed to anyone. Now it is only allowed to those who
can access the merge proposal.

== Tests ==
bin/test -vt xx-branchmergeproposals.txt -t test_getFileByName

== Demo and Q/A ==
Create a private branch and propose it for merging. Wait until the diff is
generated. Go to the URL for the diff. Log out. Go to the URL for the diff.
You should get an access denied message.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/code/browser/configure.zcml
  lib/lp/code/configure.zcml
  lib/canonical/launchpad/security.py
  lib/lp/code/browser/diff.py
  lib/lp/code/model/diff.py
  lib/lp/code/stories/branches/xx-branchmergeproposals.txt
  lib/lp/code/model/tests/test_diff.py
  lib/lp/code/templates/branchmergeproposal-diff.pt
  lib/lp/code/interfaces/diff.py

== Pylint notices ==

lib/lp/code/model/diff.py
    18: [F0401] Unable to import 'lazr.delegates' (No module named delegates)
    210: [W0703, Diff.fromFile] Catch "Exception"

lib/lp/code/interfaces/diff.py
    20: [F0401] Unable to import 'lazr.restful.fields' (No module named restful)
    21: [F0401] Unable to import 'lazr.restful.declarations' (No module named restful)

Revision history for this message
Michael Nelson (michael.nelson) wrote :

Approve, just one missing blank line below, in addition to our IRC exchange.

> === modified file 'lib/lp/code/browser/diff.py'
> --- lib/lp/code/browser/diff.py 2010-02-02 17:32:23 +0000
> +++ lib/lp/code/browser/diff.py 2010-02-18 16:06:41 +0000
> @@ -10,10 +10,18 @@
>
>
> from canonical.launchpad import _
> +from canonical.launchpad.browser.librarian import FileNavigationMixin
> +from canonical.launchpad.webapp import Navigation
> +from canonical.launchpad.webapp.publisher import canonical_url
> from canonical.launchpad.webapp.tales import ObjectFormatterAPI
> +from lp.code.interfaces.diff import IPreviewDiff
> from lp.services.browser_helpers import get_plural_text
>
>
> +class PreviewDiffNavigation(Navigation, FileNavigationMixin):
> +
> + usedfor = IPreviewDiff
> +

Just need an extra line here.

> class PreviewDiffFormatterAPI(ObjectFormatterAPI):
> """Formatter for preview diffs."""
>

> === modified file 'lib/lp/code/stories/branches/xx-branchmergeproposals.txt'
> --- lib/lp/code/stories/branches/xx-branchmergeproposals.txt 2010-01-12 17:34:12 +0000
> +++ lib/lp/code/stories/branches/xx-branchmergeproposals.txt 2010-02-18 16:06:41 +0000
> @@ -614,10 +614,25 @@
> ... return find_tag_by_id(nopriv_browser.contents, 'review-diff')
>
> The text of the review diff is in the page.
> +
> >>> print repr(extract_text(get_review_diff()))
> u'Preview Diff\nDownload diff\n1\n...Fake Diff\u1010'
>
> +There is also a link to the diff URL, which is the preview diff URL plus
> +"+files/preview.diff"
> +
> + >>> link = get_review_diff().find('a')
> + >>> print link['href']
> + http://code.launchpad.dev/~person-name.../product-name.../branch.../+merge/.../+preview-diff/+files/preview.diff

17:20 < noodles775> abentley: your branch looks great. The only question I
have is whether there's really a need to include each segment of the url in
the doctest:
http://code.launchpad.dev/~person-name.../product-name.../branch.../+merge/.../+preview-diff/+files/preview.diff
17:21 < abentley> noodles775, I would be fine with just showing "/+preview-diff/+files/preview.diff". Would you prefer that?
17:22 < noodles775> abentley: yeah (not that I care much either way, it'd just mean it was within 78chars :))

Thanks Aaron.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/security.py'
2--- lib/canonical/launchpad/security.py 2010-02-18 09:04:51 +0000
3+++ lib/canonical/launchpad/security.py 2010-02-20 04:33:20 +0000
4@@ -42,6 +42,7 @@
5 ICodeReviewComment, ICodeReviewCommentDeletion)
6 from lp.code.interfaces.codereviewvote import (
7 ICodeReviewVoteReference)
8+from lp.code.interfaces.diff import IPreviewDiff
9 from lp.registry.interfaces.distribution import IDistribution
10 from lp.registry.interfaces.distributionmirror import (
11 IDistributionMirror)
12@@ -1708,6 +1709,29 @@
13 AccessBranch(self.obj.target_branch).checkUnauthenticated())
14
15
16+class PreviewDiffView(AuthorizationBase):
17+ permission = 'launchpad.View'
18+ usedfor = IPreviewDiff
19+
20+ @property
21+ def bmp_view(self):
22+ return BranchMergeProposalView(self.obj.branch_merge_proposal)
23+
24+ def checkAuthenticated(self, user):
25+ """Is the user able to view the preview diff?
26+
27+ The user can see a preview diff if they can see the merge proposal.
28+ """
29+ return self.bmp_view.checkAuthenticated(user)
30+
31+ def checkUnauthenticated(self):
32+ """Is anyone able to view the branch merge proposal?
33+
34+ The user can see a preview diff if they can see the merge proposal.
35+ """
36+ return self.bmp_view.checkUnauthenticated()
37+
38+
39 class CodeReviewVoteReferenceEdit(AuthorizationBase):
40 permission = 'launchpad.Edit'
41 usedfor = ICodeReviewVoteReference
42
43=== modified file 'lib/lp/code/browser/configure.zcml'
44--- lib/lp/code/browser/configure.zcml 2010-02-17 11:19:42 +0000
45+++ lib/lp/code/browser/configure.zcml 2010-02-20 04:33:20 +0000
46@@ -946,6 +946,11 @@
47 attribute_to_parent="branch_merge_proposal"
48 rootsite="code" />
49
50+ <browser:navigation
51+ module="lp.code.browser.diff"
52+ classes="
53+ PreviewDiffNavigation"/>
54+
55 <browser:page
56 name="+diff"
57 for="lp.code.interfaces.diff.IPreviewDiff"
58
59=== modified file 'lib/lp/code/browser/diff.py'
60--- lib/lp/code/browser/diff.py 2010-02-02 17:32:23 +0000
61+++ lib/lp/code/browser/diff.py 2010-02-20 04:33:20 +0000
62@@ -10,10 +10,19 @@
63
64
65 from canonical.launchpad import _
66+from canonical.launchpad.browser.librarian import FileNavigationMixin
67+from canonical.launchpad.webapp import Navigation
68+from canonical.launchpad.webapp.publisher import canonical_url
69 from canonical.launchpad.webapp.tales import ObjectFormatterAPI
70+from lp.code.interfaces.diff import IPreviewDiff
71 from lp.services.browser_helpers import get_plural_text
72
73
74+class PreviewDiffNavigation(Navigation, FileNavigationMixin):
75+
76+ usedfor = IPreviewDiff
77+
78+
79 class PreviewDiffFormatterAPI(ObjectFormatterAPI):
80 """Formatter for preview diffs."""
81
82@@ -24,7 +33,7 @@
83 if librarian_alias is None:
84 return None
85 else:
86- return librarian_alias.getURL()
87+ return canonical_url(self._context) + '/+files/preview.diff'
88
89 def link(self, view_name):
90 """The link to the diff should show the line count.
91
92=== modified file 'lib/lp/code/browser/tests/test_tales.py'
93--- lib/lp/code/browser/tests/test_tales.py 2009-11-18 01:53:19 +0000
94+++ lib/lp/code/browser/tests/test_tales.py 2010-02-20 04:33:20 +0000
95@@ -11,8 +11,9 @@
96 from storm.store import Store
97 from zope.security.proxy import removeSecurityProxy
98
99+from canonical.launchpad.webapp.publisher import canonical_url
100+from canonical.testing import LaunchpadFunctionalLayer
101 from lp.testing import login, TestCaseWithFactory, test_tales
102-from canonical.testing import LaunchpadFunctionalLayer
103
104
105 class TestPreviewDiffFormatter(TestCaseWithFactory):
106@@ -79,18 +80,18 @@
107 # If the lines added and removed are None, they are now shown.
108 preview = self._createPreviewDiff(10, added=None, removed=None)
109 self.assertEqual(
110- '<a href="%s" class="diff-link">'
111+ '<a href="%s/+files/preview.diff" class="diff-link">'
112 '10 lines</a>'
113- % preview.diff_text.getURL(),
114+ % canonical_url(preview),
115 test_tales('preview/fmt:link', preview=preview))
116
117 def test_fmt_lines_some_added_no_removed(self):
118 # If the added and removed values are not None, they are shown.
119 preview = self._createPreviewDiff(10, added=4, removed=0)
120 self.assertEqual(
121- '<a href="%s" class="diff-link">'
122+ '<a href="%s/+files/preview.diff" class="diff-link">'
123 '10 lines (+4/-0)</a>'
124- % preview.diff_text.getURL(),
125+ % canonical_url(preview),
126 test_tales('preview/fmt:link', preview=preview))
127
128 def test_fmt_lines_files_modified(self):
129@@ -101,9 +102,9 @@
130 'file1': (1, 0),
131 'file2': (3, 0)})
132 self.assertEqual(
133- '<a href="%s" class="diff-link">'
134+ '<a href="%s/+files/preview.diff" class="diff-link">'
135 '10 lines (+4/-0) 2 files modified</a>'
136- % preview.diff_text.getURL(),
137+ % canonical_url(preview),
138 test_tales('preview/fmt:link', preview=preview))
139
140 def test_fmt_lines_one_file_modified(self):
141@@ -112,18 +113,18 @@
142 10, added=4, removed=0, diffstat={
143 'file': (3, 0)})
144 self.assertEqual(
145- '<a href="%s" class="diff-link">'
146+ '<a href="%s/+files/preview.diff" class="diff-link">'
147 '10 lines (+4/-0) 1 file modified</a>'
148- % preview.diff_text.getURL(),
149+ % canonical_url(preview),
150 test_tales('preview/fmt:link', preview=preview))
151
152 def test_fmt_simple_conflicts(self):
153 # Conflicts are indicated using text in the link.
154 preview = self._createPreviewDiff(10, 2, 3, u'conflicts')
155 self.assertEqual(
156- '<a href="%s" class="diff-link">'
157+ '<a href="%s/+files/preview.diff" class="diff-link">'
158 '10 lines (+2/-3) (has conflicts)</a>'
159- % preview.diff_text.getURL(),
160+ % canonical_url(preview),
161 test_tales('preview/fmt:link', preview=preview))
162
163 def test_fmt_stale_empty_diff(self):
164@@ -140,9 +141,9 @@
165 preview = self._createStalePreviewDiff(
166 500, 89, 340, diffstat=diffstat)
167 self.assertEqual(
168- '<a href="%s" class="diff-link">'
169+ '<a href="%s/+files/preview.diff" class="diff-link">'
170 '500 lines (+89/-340) 23 files modified</a>'
171- % preview.diff_text.getURL(),
172+ % canonical_url(preview),
173 test_tales('preview/fmt:link', preview=preview))
174
175 def test_fmt_stale_non_empty_diff_with_conflicts(self):
176@@ -152,9 +153,9 @@
177 preview = self._createStalePreviewDiff(
178 500, 89, 340, u'conflicts', diffstat=diffstat)
179 self.assertEqual(
180- '<a href="%s" class="diff-link">'
181+ '<a href="%s/+files/preview.diff" class="diff-link">'
182 '500 lines (+89/-340) 23 files modified (has conflicts)</a>'
183- % preview.diff_text.getURL(),
184+ % canonical_url(preview),
185 test_tales('preview/fmt:link', preview=preview))
186
187
188
189=== modified file 'lib/lp/code/configure.zcml'
190--- lib/lp/code/configure.zcml 2010-02-18 16:00:24 +0000
191+++ lib/lp/code/configure.zcml 2010-02-20 04:33:20 +0000
192@@ -929,7 +929,9 @@
193 <allow interface="lp.code.interfaces.diff.IStaticDiff" />
194 </class>
195 <class class="lp.code.model.diff.PreviewDiff">
196- <allow interface="lp.code.interfaces.diff.IPreviewDiff" />
197+ <require
198+ permission="launchpad.View"
199+ interface="lp.code.interfaces.diff.IPreviewDiff"/>
200 </class>
201 <securedutility
202 component="lp.code.model.diff.StaticDiff"
203
204=== modified file 'lib/lp/code/interfaces/diff.py'
205--- lib/lp/code/interfaces/diff.py 2010-02-02 17:19:05 +0000
206+++ lib/lp/code/interfaces/diff.py 2010-02-20 04:33:20 +0000
207@@ -136,3 +136,6 @@
208 'If the preview diff is stale, it is out of date when '
209 'compared to the tip revisions of the source, target, and '
210 'possibly prerequisite branches.')))
211+
212+ def getFileByName(filename):
213+ """Return the file under +files with specified name."""
214
215=== modified file 'lib/lp/code/model/diff.py'
216--- lib/lp/code/model/diff.py 2010-02-02 17:19:05 +0000
217+++ lib/lp/code/model/diff.py 2010-02-20 04:33:20 +0000
218@@ -27,9 +27,10 @@
219 from canonical.database.sqlbase import SQLBase
220 from canonical.uuid import generate_uuid
221
222+from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
223+from canonical.launchpad.interfaces.launchpad import NotFoundError
224 from lp.code.interfaces.diff import (
225 IDiff, IPreviewDiff, IStaticDiff, IStaticDiffSource)
226-from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
227
228
229 class Diff(SQLBase):
230@@ -195,7 +196,7 @@
231 if filename is None:
232 filename = generate_uuid() + '.txt'
233 diff_text = getUtility(ILibraryFileAliasSet).create(
234- filename, size, diff_content, 'text/x-diff')
235+ filename, size, diff_content, 'text/x-diff', restricted=True)
236 diff_content.seek(0)
237 diff_content_bytes = diff_content.read(size)
238 diff_lines_count = len(diff_content_bytes.strip().split('\n'))
239@@ -383,3 +384,10 @@
240 return True
241 else:
242 return False
243+
244+ def getFileByName(self, filename):
245+ """See `IPreviewDiff`."""
246+ if filename == 'preview.diff' and self.diff_text is not None:
247+ return self.diff_text
248+ else:
249+ raise NotFoundError(filename)
250
251=== modified file 'lib/lp/code/model/tests/test_diff.py'
252--- lib/lp/code/model/tests/test_diff.py 2010-02-02 17:19:05 +0000
253+++ lib/lp/code/model/tests/test_diff.py 2010-02-20 04:33:20 +0000
254@@ -15,6 +15,7 @@
255 from bzrlib import trace
256 import transaction
257
258+from canonical.launchpad.interfaces.launchpad import NotFoundError
259 from canonical.launchpad.webapp import canonical_url, errorlog
260 from canonical.launchpad.webapp.testing import verifyObject
261 from canonical.testing import LaunchpadFunctionalLayer, LaunchpadZopelessLayer
262@@ -123,6 +124,7 @@
263 content = ''.join(unified_diff('', "1234567890" * 10))
264 diff = self._create_diff(content)
265 self.assertEqual(content, diff.text)
266+ self.assertTrue(diff.diff_text.restricted)
267
268 def test_oversized_normal(self):
269 # A diff smaller than config.diff.max_read_size is not oversized.
270@@ -478,6 +480,17 @@
271 finally:
272 logger.removeHandler(handler)
273
274+ def test_getFileByName(self):
275+ diff = self._createProposalWithPreviewDiff().preview_diff
276+ self.assertEqual(diff.diff_text, diff.getFileByName('preview.diff'))
277+ self.assertRaises(
278+ NotFoundError, diff.getFileByName, 'different.name')
279+
280+ def test_getFileByName_with_no_diff(self):
281+ diff = self._createProposalWithPreviewDiff(content='').preview_diff
282+ self.assertRaises(
283+ NotFoundError, diff.getFileByName, 'preview.diff')
284+
285
286 def test_suite():
287 return TestLoader().loadTestsFromName(__name__)
288
289=== modified file 'lib/lp/code/stories/branches/xx-branchmergeproposals.txt'
290--- lib/lp/code/stories/branches/xx-branchmergeproposals.txt 2010-01-12 17:34:12 +0000
291+++ lib/lp/code/stories/branches/xx-branchmergeproposals.txt 2010-02-20 04:33:20 +0000
292@@ -614,10 +614,25 @@
293 ... return find_tag_by_id(nopriv_browser.contents, 'review-diff')
294
295 The text of the review diff is in the page.
296+
297 >>> print repr(extract_text(get_review_diff()))
298 u'Preview Diff\nDownload diff\n1\n...Fake Diff\u1010'
299
300+There is also a link to the diff URL, which is the preview diff URL plus
301+"+files/preview.diff"
302+
303+ >>> link = get_review_diff().find('a')
304+ >>> print link['href']
305+ http.../+preview-diff/+files/preview.diff
306+ >>> nopriv_browser.open(str(link['href']))
307+ >>> print nopriv_browser.contents
308+ ---
309+ +++
310+ @@ -1,0 +1,1 @@
311+ +Fake Diff...
312+
313 If no diff is present, nothing is shown.
314+
315 >>> login('admin@canonical.com')
316 >>> removeSecurityProxy(bmp).preview_diff = None
317 >>> logout()
318
319=== modified file 'lib/lp/code/templates/branchmergeproposal-diff.pt'
320--- lib/lp/code/templates/branchmergeproposal-diff.pt 2009-11-14 09:36:50 +0000
321+++ lib/lp/code/templates/branchmergeproposal-diff.pt 2010-02-20 04:33:20 +0000
322@@ -8,7 +8,7 @@
323 <div class="boardCommentDetails filename">
324 <ul class="horizontal">
325 <li>
326- <a tal:attributes="href attachment/getURL">
327+ <a tal:attributes="href view/preview_diff/fmt:url">
328 <img src="/@@/download"/>
329 Download diff
330 </a>
331@@ -30,4 +30,4 @@
332 </tal:empty-diff>
333 </div>
334
335-</tal:root>
336\ No newline at end of file
337+</tal:root>