launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-750607 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #750607 in Launchpad itself: "on bugs page can see a branch merge proposal which cannot be seen directly"
  https://bugs.launchpad.net/launchpad/+bug/750607

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-750607/+merge/61069

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.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-750607/+merge/61069
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-750607 into lp:launchpad.

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