Merge lp:~wgrant/launchpad/bug-750607 into lp:launchpad

[William Grant]

[r=henninge][bug=750607] launchpad.View on BranchMergeProposal now requires launchpad.View on prerequisite_branch, if one is set.

Users can't see BranchMergeProposal:+index unless they can see the prerequisite_branch, if any. This is enforced by an Unauthorized when accessing the prereq's attributes, not by BranchMergeProposal's security declarations.

This branch fixes the launchpad.View BranchMergeProposal adapter to require access to prerequisite_branch, if it is set.

[Henning Eggers (henninge)]

Ah, do you remember those days when lambdas were considered inappropriate in LP code? Good riddance! Welcome concise code using "map" and "all". Thanks for the reading pleasure. ;-)

I have one remark, though. Is there a reason for not using getUtility(ILaunchpadCelebrities).admin or are you simply not aware of it?

Cheers,
Henning

review: Approve

[William Grant (wgrant)]

I'm aware of it, but I find celebrities to be extremely distasteful.

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2011-05-03 04:39:43 +0000
+++ lib/canonical/launchpad/security.py	2011-05-16 07:27:38 +0000
@@ -1892,24 +1892,31 @@
     permission = 'launchpad.View'
     usedfor = IBranchMergeProposal
 
+    @property
+    def branches(self):
+        required = [self.obj.source_branch, self.obj.target_branch]
+        if self.obj.prerequisite_branch:
+            required.append(self.obj.prerequisite_branch)
+        return required
+
     def checkAuthenticated(self, user):
         """Is the user able to view the branch merge proposal?
 
-        The user can see a merge proposal between two branches
-        that the user can see.
+        The user can see a merge proposal if they can see the source, target
+        and prerequisite branches.
         """
-        return (AccessBranch(self.obj.source_branch).checkAuthenticated(user)
-                and
-                AccessBranch(self.obj.target_branch).checkAuthenticated(user))
+        return all(map(
+            lambda b: AccessBranch(b).checkAuthenticated(user),
+            self.branches))
 
     def checkUnauthenticated(self):
         """Is anyone able to view the branch merge proposal?
 
         Anyone can see a merge proposal between two public branches.
         """
-        return (AccessBranch(self.obj.source_branch).checkUnauthenticated()
-                and
-                AccessBranch(self.obj.target_branch).checkUnauthenticated())
+        return all(map(
+            lambda b: AccessBranch(b).checkUnauthenticated(),
+            self.branches))
 
 
 class PreviewDiffView(AuthorizationBase):

=== modified file 'lib/lp/code/tests/test_branch.py'
--- lib/lp/code/tests/test_branch.py	2011-01-25 19:47:56 +0000
+++ lib/lp/code/tests/test_branch.py	2011-05-16 07:27:38 +0000
@@ -58,6 +58,14 @@
         """
         self.assertAuthenticatedView(branch, None, can_access)
 
+    def assertCanView(self, person, secured_object):
+        """Assert 'person' can view 'secured_object'."""
+        self.assertPermission(True, person, secured_object, 'launchpad.View')
+
+    def assertCannotView(self, person, secured_object):
+        """Assert 'person' cannot view 'secured_object'."""
+        self.assertPermission(False, person, secured_object, 'launchpad.View')
+
     def assertCanEdit(self, person, secured_object):
         """Assert 'person' can edit 'secured_object'.
 

=== modified file 'lib/lp/code/tests/test_branchmergeproposal.py'
--- lib/lp/code/tests/test_branchmergeproposal.py	2010-10-04 19:50:45 +0000
+++ lib/lp/code/tests/test_branchmergeproposal.py	2011-05-16 07:27:38 +0000
@@ -11,9 +11,13 @@
 from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
 from lp.code.tests.test_branch import PermissionTest
+from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet
-from lp.testing import run_with_login
+from lp.testing import (
+    run_with_login,
+    with_celebrity_logged_in,
+    )
 
 
 class TestEditMergeProposal(PermissionTest):
@@ -73,3 +77,28 @@
         self.assertCanEdit(person, proposal.target_branch)
         # And that means they can edit the proposal too
         self.assertCanEdit(person, proposal)
+
+
+class TestViewMergeProposal(PermissionTest):
+
+    layer = DatabaseFunctionalLayer
+
+    @with_celebrity_logged_in('admin')
+    def assertBranchAccessRequired(self, attr):
+        person = self.factory.makePerson()
+        prereq = self.factory.makeBranch()
+        proposal = self.factory.makeBranchMergeProposal(
+            prerequisite_branch=prereq)
+        self.assertCanView(person, proposal)
+        getattr(proposal, attr).setPrivate(
+            True, getUtility(IPersonSet).getByName('admins'))
+        self.assertCannotView(person, proposal)
+
+    def test_source_branch_access_required(self):
+        self.assertBranchAccessRequired('source_branch')
+
+    def test_target_branch_access_required(self):
+        self.assertBranchAccessRequired('target_branch')
+
+    def test_prerequisite_branch_access_required(self):
+        self.assertBranchAccessRequired('prerequisite_branch')