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

[Aaron Bentley]

[Aaron Bentley (abentley)]

= 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)

[Michael Nelson (michael.nelson)]

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)

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2010-02-18 09:04:51 +0000
+++ lib/canonical/launchpad/security.py	2010-02-20 04:33:20 +0000
@@ -42,6 +42,7 @@
     ICodeReviewComment, ICodeReviewCommentDeletion)
 from lp.code.interfaces.codereviewvote import (
     ICodeReviewVoteReference)
+from lp.code.interfaces.diff import IPreviewDiff
 from lp.registry.interfaces.distribution import IDistribution
 from lp.registry.interfaces.distributionmirror import (
     IDistributionMirror)
@@ -1708,6 +1709,29 @@
                 AccessBranch(self.obj.target_branch).checkUnauthenticated())
 
 
+class PreviewDiffView(AuthorizationBase):
+    permission = 'launchpad.View'
+    usedfor = IPreviewDiff
+
+    @property
+    def bmp_view(self):
+        return BranchMergeProposalView(self.obj.branch_merge_proposal)
+
+    def checkAuthenticated(self, user):
+        """Is the user able to view the preview diff?
+
+        The user can see a preview diff if they can see the merge proposal.
+        """
+        return self.bmp_view.checkAuthenticated(user)
+
+    def checkUnauthenticated(self):
+        """Is anyone able to view the branch merge proposal?
+
+        The user can see a preview diff if they can see the merge proposal.
+        """
+        return self.bmp_view.checkUnauthenticated()
+
+
 class CodeReviewVoteReferenceEdit(AuthorizationBase):
     permission = 'launchpad.Edit'
     usedfor = ICodeReviewVoteReference

=== modified file 'lib/lp/code/browser/configure.zcml'
--- lib/lp/code/browser/configure.zcml	2010-02-17 11:19:42 +0000
+++ lib/lp/code/browser/configure.zcml	2010-02-20 04:33:20 +0000
@@ -946,6 +946,11 @@
         attribute_to_parent="branch_merge_proposal"
         rootsite="code" />
 
+    <browser:navigation
+        module="lp.code.browser.diff"
+        classes="
+            PreviewDiffNavigation"/>
+
     <browser:page
         name="+diff"
         for="lp.code.interfaces.diff.IPreviewDiff"

=== 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-20 04:33:20 +0000
@@ -10,10 +10,19 @@
 
 
 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
+
+
 class PreviewDiffFormatterAPI(ObjectFormatterAPI):
     """Formatter for preview diffs."""
 
@@ -24,7 +33,7 @@
         if librarian_alias is None:
             return None
         else:
-            return librarian_alias.getURL()
+            return canonical_url(self._context) + '/+files/preview.diff'
 
     def link(self, view_name):
         """The link to the diff should show the line count.

=== modified file 'lib/lp/code/browser/tests/test_tales.py'
--- lib/lp/code/browser/tests/test_tales.py	2009-11-18 01:53:19 +0000
+++ lib/lp/code/browser/tests/test_tales.py	2010-02-20 04:33:20 +0000
@@ -11,8 +11,9 @@
 from storm.store import Store
 from zope.security.proxy import removeSecurityProxy
 
+from canonical.launchpad.webapp.publisher import canonical_url
+from canonical.testing import LaunchpadFunctionalLayer
 from lp.testing import login, TestCaseWithFactory, test_tales
-from canonical.testing import LaunchpadFunctionalLayer
 
 
 class TestPreviewDiffFormatter(TestCaseWithFactory):
@@ -79,18 +80,18 @@
         # If the lines added and removed are None, they are now shown.
         preview = self._createPreviewDiff(10, added=None, removed=None)
         self.assertEqual(
-            '<a href="%s" class="diff-link">'
+            '<a href="%s/+files/preview.diff" class="diff-link">'
             '10 lines</a>'
-            % preview.diff_text.getURL(),
+            % canonical_url(preview),
             test_tales('preview/fmt:link', preview=preview))
 
     def test_fmt_lines_some_added_no_removed(self):
         # If the added and removed values are not None, they are shown.
         preview = self._createPreviewDiff(10, added=4, removed=0)
         self.assertEqual(
-            '<a href="%s" class="diff-link">'
+            '<a href="%s/+files/preview.diff" class="diff-link">'
             '10 lines (+4/-0)</a>'
-            % preview.diff_text.getURL(),
+            % canonical_url(preview),
             test_tales('preview/fmt:link', preview=preview))
 
     def test_fmt_lines_files_modified(self):
@@ -101,9 +102,9 @@
                 'file1': (1, 0),
                 'file2': (3, 0)})
         self.assertEqual(
-            '<a href="%s" class="diff-link">'
+            '<a href="%s/+files/preview.diff" class="diff-link">'
             '10 lines (+4/-0) 2 files modified</a>'
-            % preview.diff_text.getURL(),
+            % canonical_url(preview),
             test_tales('preview/fmt:link', preview=preview))
 
     def test_fmt_lines_one_file_modified(self):
@@ -112,18 +113,18 @@
             10, added=4, removed=0, diffstat={
                 'file': (3, 0)})
         self.assertEqual(
-            '<a href="%s" class="diff-link">'
+            '<a href="%s/+files/preview.diff" class="diff-link">'
             '10 lines (+4/-0) 1 file modified</a>'
-            % preview.diff_text.getURL(),
+            % canonical_url(preview),
             test_tales('preview/fmt:link', preview=preview))
 
     def test_fmt_simple_conflicts(self):
         # Conflicts are indicated using text in the link.
         preview = self._createPreviewDiff(10, 2, 3, u'conflicts')
         self.assertEqual(
-            '<a href="%s" class="diff-link">'
+            '<a href="%s/+files/preview.diff" class="diff-link">'
             '10 lines (+2/-3) (has conflicts)</a>'
-            % preview.diff_text.getURL(),
+            % canonical_url(preview),
             test_tales('preview/fmt:link', preview=preview))
 
     def test_fmt_stale_empty_diff(self):
@@ -140,9 +141,9 @@
         preview = self._createStalePreviewDiff(
             500, 89, 340, diffstat=diffstat)
         self.assertEqual(
-            '<a href="%s" class="diff-link">'
+            '<a href="%s/+files/preview.diff" class="diff-link">'
             '500 lines (+89/-340) 23 files modified</a>'
-            % preview.diff_text.getURL(),
+            % canonical_url(preview),
             test_tales('preview/fmt:link', preview=preview))
 
     def test_fmt_stale_non_empty_diff_with_conflicts(self):
@@ -152,9 +153,9 @@
         preview = self._createStalePreviewDiff(
             500, 89, 340, u'conflicts', diffstat=diffstat)
         self.assertEqual(
-            '<a href="%s" class="diff-link">'
+            '<a href="%s/+files/preview.diff" class="diff-link">'
             '500 lines (+89/-340) 23 files modified (has conflicts)</a>'
-            % preview.diff_text.getURL(),
+            % canonical_url(preview),
             test_tales('preview/fmt:link', preview=preview))
 
 

=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml	2010-02-18 16:00:24 +0000
+++ lib/lp/code/configure.zcml	2010-02-20 04:33:20 +0000
@@ -929,7 +929,9 @@
     <allow interface="lp.code.interfaces.diff.IStaticDiff" />
   </class>
   <class class="lp.code.model.diff.PreviewDiff">
-    <allow interface="lp.code.interfaces.diff.IPreviewDiff" />
+    <require
+        permission="launchpad.View"
+        interface="lp.code.interfaces.diff.IPreviewDiff"/>
   </class>
   <securedutility
       component="lp.code.model.diff.StaticDiff"

=== modified file 'lib/lp/code/interfaces/diff.py'
--- lib/lp/code/interfaces/diff.py	2010-02-02 17:19:05 +0000
+++ lib/lp/code/interfaces/diff.py	2010-02-20 04:33:20 +0000
@@ -136,3 +136,6 @@
                 'If the preview diff is stale, it is out of date when '
                 'compared to the tip revisions of the source, target, and '
                 'possibly prerequisite branches.')))
+
+    def getFileByName(filename):
+        """Return the file under +files with specified name."""

=== modified file 'lib/lp/code/model/diff.py'
--- lib/lp/code/model/diff.py	2010-02-02 17:19:05 +0000
+++ lib/lp/code/model/diff.py	2010-02-20 04:33:20 +0000
@@ -27,9 +27,10 @@
 from canonical.database.sqlbase import SQLBase
 from canonical.uuid import generate_uuid
 
+from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
+from canonical.launchpad.interfaces.launchpad import NotFoundError
 from lp.code.interfaces.diff import (
     IDiff, IPreviewDiff, IStaticDiff, IStaticDiffSource)
-from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
 
 
 class Diff(SQLBase):
@@ -195,7 +196,7 @@
             if filename is None:
                 filename = generate_uuid() + '.txt'
             diff_text = getUtility(ILibraryFileAliasSet).create(
-                filename, size, diff_content, 'text/x-diff')
+                filename, size, diff_content, 'text/x-diff', restricted=True)
             diff_content.seek(0)
             diff_content_bytes = diff_content.read(size)
             diff_lines_count = len(diff_content_bytes.strip().split('\n'))
@@ -383,3 +384,10 @@
             return True
         else:
             return False
+
+    def getFileByName(self, filename):
+        """See `IPreviewDiff`."""
+        if filename == 'preview.diff' and self.diff_text is not None:
+            return self.diff_text
+        else:
+            raise NotFoundError(filename)

=== modified file 'lib/lp/code/model/tests/test_diff.py'
--- lib/lp/code/model/tests/test_diff.py	2010-02-02 17:19:05 +0000
+++ lib/lp/code/model/tests/test_diff.py	2010-02-20 04:33:20 +0000
@@ -15,6 +15,7 @@
 from bzrlib import trace
 import transaction
 
+from canonical.launchpad.interfaces.launchpad import NotFoundError
 from canonical.launchpad.webapp import canonical_url, errorlog
 from canonical.launchpad.webapp.testing import verifyObject
 from canonical.testing import LaunchpadFunctionalLayer, LaunchpadZopelessLayer
@@ -123,6 +124,7 @@
         content = ''.join(unified_diff('', "1234567890" * 10))
         diff = self._create_diff(content)
         self.assertEqual(content, diff.text)
+        self.assertTrue(diff.diff_text.restricted)
 
     def test_oversized_normal(self):
         # A diff smaller than config.diff.max_read_size is not oversized.
@@ -478,6 +480,17 @@
         finally:
             logger.removeHandler(handler)
 
+    def test_getFileByName(self):
+        diff = self._createProposalWithPreviewDiff().preview_diff
+        self.assertEqual(diff.diff_text, diff.getFileByName('preview.diff'))
+        self.assertRaises(
+            NotFoundError, diff.getFileByName, 'different.name')
+
+    def test_getFileByName_with_no_diff(self):
+        diff = self._createProposalWithPreviewDiff(content='').preview_diff
+        self.assertRaises(
+            NotFoundError, diff.getFileByName, 'preview.diff')
+
 
 def test_suite():
     return TestLoader().loadTestsFromName(__name__)

=== 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-20 04:33:20 +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.../+preview-diff/+files/preview.diff
+    >>> nopriv_browser.open(str(link['href']))
+    >>> print nopriv_browser.contents
+    ---
+    +++
+    @@ -1,0 +1,1 @@
+    +Fake Diff...
+
 If no diff is present, nothing is shown.
+
     >>> login('[e-mail]')
     >>> removeSecurityProxy(bmp).preview_diff = None
     >>> logout()

=== modified file 'lib/lp/code/templates/branchmergeproposal-diff.pt'
--- lib/lp/code/templates/branchmergeproposal-diff.pt	2009-11-14 09:36:50 +0000
+++ lib/lp/code/templates/branchmergeproposal-diff.pt	2010-02-20 04:33:20 +0000
@@ -8,7 +8,7 @@
       <div class="boardCommentDetails filename">
         <ul class="horizontal">
           <li>
-            <a tal:attributes="href attachment/getURL">
+            <a tal:attributes="href view/preview_diff/fmt:url">
               <img src="/@@/download"/>
               Download diff
             </a>
@@ -30,4 +30,4 @@
     </tal:empty-diff>
   </div>
 
-</tal:root>
\ No newline at end of file
+</tal:root>