launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/git-mp-reviewer-fix into lp:launchpad.

Commit message:
Fix GitRepository.reviewer to satisfy Edit on BranchMergeProposal

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/git-mp-reviewer-fix/+merge/310867

Fix GitRepository.reviewer to satisfy Edit on BranchMergeProposal

The security adapter erroneously assumed that the target's
checkAuthenticated would return a bool, but GitRef uses a
DelegatedAuthorization that instead returns an iterator of checks to
delegate to. Since the iterator always evaluated non-zero, the reviewer
check was short-circuited, and the permission check returned negative
when the delegated check failed.

We probably want to move iter_authorizations down into BaseAuthorization at some point, but that entails passing the cache through, so I just added a minor safetynet for now.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/git-mp-reviewer-fix into lp:launchpad.

=== modified file 'lib/lp/app/security.py'
--- lib/lp/app/security.py	2015-07-08 16:05:11 +0000
+++ lib/lp/app/security.py	2016-11-15 11:53:41 +0000
@@ -119,6 +119,17 @@
         return True
 
 
+class non_boolean_izip(izip):
+
+    def __nonzero__(self):
+        # XXX wgrant 2016-11-15: Guard against security adapters
+        # accidentally using a delegation as a boolean authentication
+        # result.
+        raise Exception(
+            "DelegatedAuthorization results can't be used in boolean "
+            "expressions.")
+
+
 class DelegatedAuthorization(AuthorizationBase):
 
     def __init__(self, obj, forwarded_object=None, permission=None):
@@ -141,8 +152,8 @@
 
     def checkAuthenticated(self, user):
         """See `IAuthorization`."""
-        return izip(self.iter_objects(), repeat(self.permission))
+        return non_boolean_izip(self.iter_objects(), repeat(self.permission))
 
     def checkUnauthenticated(self):
         """See `IAuthorization`."""
-        return izip(self.iter_objects(), repeat(self.permission))
+        return non_boolean_izip(self.iter_objects(), repeat(self.permission))

=== modified file 'lib/lp/code/tests/test_branchmergeproposal.py'
--- lib/lp/code/tests/test_branchmergeproposal.py	2012-01-01 02:58:52 +0000
+++ lib/lp/code/tests/test_branchmergeproposal.py	2016-11-15 11:53:41 +0000
@@ -11,8 +11,14 @@
 from lp.code.tests.test_branch import PermissionTest
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.services.webapp.authorization import (
+    check_permission,
+    clear_cache as clear_permission_cache,
+    )
 from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet
 from lp.testing import (
+    admin_logged_in,
+    person_logged_in,
     run_with_login,
     with_celebrity_logged_in,
     )
@@ -76,6 +82,20 @@
         # And that means they can edit the proposal too
         self.assertCanEdit(person, proposal)
 
+    def test_reviewer_can_edit_git_merge_proposal(self):
+        person = self.factory.makePerson()
+        product = self.factory.makeProduct()
+        [target] = self.factory.makeGitRefs(target=product)
+        [source] = self.factory.makeGitRefs(target=product)
+        mp = self.factory.makeBranchMergeProposalForGit(
+            source_ref=source, target_ref=target)
+        with person_logged_in(person):
+            self.assertFalse(check_permission('launchpad.Edit', mp))
+        with admin_logged_in():
+            target.repository.reviewer = person
+        with person_logged_in(person):
+            self.assertTrue(check_permission('launchpad.Edit', mp))
+
 
 class TestViewMergeProposal(PermissionTest):
 

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2016-10-12 23:24:24 +0000
+++ lib/lp/security.py	2016-11-15 11:53:41 +0000
@@ -302,16 +302,11 @@
     usedfor = Interface
 
     def checkUnauthenticated(self):
-        return (
-            self.forwardCheckUnauthenticated(self.obj, 'launchpad.View') or
-            self.forwardCheckUnauthenticated(
-                self.obj, 'launchpad.SubscriberView'))
+        return self.forwardCheckUnauthenticated(permission='launchpad.View')
 
     def checkAuthenticated(self, user):
-        return (
-            self.forwardCheckAuthenticated(user, self.obj, 'launchpad.View') or
-            self.forwardCheckAuthenticated(
-                user, self.obj, 'launchpad.SubscriberView'))
+        return self.forwardCheckAuthenticated(
+            user, permission='launchpad.View')
 
 
 class AdminByAdminsTeam(AuthorizationBase):
@@ -2462,10 +2457,11 @@
           * the reviewer for the merge_target
           * an administrator
         """
-        return (user.inTeam(self.obj.registrant) or
+        if (user.inTeam(self.obj.registrant) or
                 user.inTeam(self.obj.merge_source.owner) or
-                self.forwardCheckAuthenticated(user, self.obj.merge_target) or
-                user.inTeam(self.obj.merge_target.reviewer))
+                user.inTeam(self.obj.merge_target.reviewer)):
+            return True
+        return self.forwardCheckAuthenticated(user, self.obj.merge_target)
 
 
 class AdminDistroSeriesLanguagePacks(
@@ -2618,6 +2614,23 @@
             Archive.id, And(Archive.id == self.obj.id, filter)).is_empty()
 
 
+class LimitedViewArchive(AuthorizationBase):
+    """Restricted existence knowledge of private archives.
+
+    Just delegate to SubscriberView, since that includes View.
+    """
+    permission = 'launchpad.LimitedView'
+    usedfor = IArchive
+
+    def checkUnauthenticated(self):
+        return self.forwardCheckUnauthenticated(
+            permission='launchpad.SubscriberView')
+
+    def checkAuthenticated(self, user):
+        return self.forwardCheckAuthenticated(
+            user, permission='launchpad.SubscriberView')
+
+
 class EditArchive(AuthorizationBase):
     """Restrict archive editing operations.
 
@@ -2810,18 +2823,12 @@
     usedfor = ISourcePackagePublishingHistory
 
     def checkUnauthenticated(self):
-        return (
-            self.forwardCheckUnauthenticated(
-                self.obj.archive, 'launchpad.View') or
-            self.forwardCheckUnauthenticated(
-                self.obj.archive, 'launchpad.SubscriberView'))
+        return self.forwardCheckUnauthenticated(
+            self.obj.archive, 'launchpad.SubscriberView')
 
     def checkAuthenticated(self, user):
-        return (
-            self.forwardCheckAuthenticated(
-                user, self.obj.archive, 'launchpad.View') or
-            self.forwardCheckAuthenticated(
-                user, self.obj.archive, 'launchpad.SubscriberView'))
+        return self.forwardCheckAuthenticated(
+            user, self.obj.archive, 'launchpad.SubscriberView')
 
 
 class EditPublishing(DelegatedAuthorization):