Launchpad itself

Merge lp:~wgrant/launchpad/git-mp-reviewer-fix into lp:launchpad

  1. git-mp-reviewer-fix
  2. Merge into devel
Proposed by William Grant
Status: Merged
Merged at revision: 18267
Proposed branch: lp:~wgrant/launchpad/git-mp-reviewer-fix
Merge into: lp:launchpad
Diff against target: 155 lines (+58/-23)
3 files modified
lib/lp/app/security.py (+13/-2)
lib/lp/code/tests/test_branchmergeproposal.py (+17/-0)
lib/lp/security.py (+28/-21)
To merge this branch: bzr merge lp:~wgrant/launchpad/git-mp-reviewer-fix
Reviewer Review Type Date Requested Status
Colin Watson (community) Approve
Review via email: mp+310867@code.launchpad.net

Commit message

Fix GitRepository.reviewer to satisfy Edit on BranchMergeProposal

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Colin Watson (cjwatson) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/app/security.py'
2--- lib/lp/app/security.py 2015-07-08 16:05:11 +0000
3+++ lib/lp/app/security.py 2016-11-15 11:55:35 +0000
4@@ -119,6 +119,17 @@
5 return True
6
7
8+class non_boolean_izip(izip):
9+
10+ def __nonzero__(self):
11+ # XXX wgrant 2016-11-15: Guard against security adapters
12+ # accidentally using a delegation as a boolean authentication
13+ # result.
14+ raise Exception(
15+ "DelegatedAuthorization results can't be used in boolean "
16+ "expressions.")
17+
18+
19 class DelegatedAuthorization(AuthorizationBase):
20
21 def __init__(self, obj, forwarded_object=None, permission=None):
22@@ -141,8 +152,8 @@
23
24 def checkAuthenticated(self, user):
25 """See `IAuthorization`."""
26- return izip(self.iter_objects(), repeat(self.permission))
27+ return non_boolean_izip(self.iter_objects(), repeat(self.permission))
28
29 def checkUnauthenticated(self):
30 """See `IAuthorization`."""
31- return izip(self.iter_objects(), repeat(self.permission))
32+ return non_boolean_izip(self.iter_objects(), repeat(self.permission))
33
34=== modified file 'lib/lp/code/tests/test_branchmergeproposal.py'
35--- lib/lp/code/tests/test_branchmergeproposal.py 2012-01-01 02:58:52 +0000
36+++ lib/lp/code/tests/test_branchmergeproposal.py 2016-11-15 11:55:35 +0000
37@@ -11,8 +11,11 @@
38 from lp.code.tests.test_branch import PermissionTest
39 from lp.registry.interfaces.person import IPersonSet
40 from lp.registry.interfaces.pocket import PackagePublishingPocket
41+from lp.services.webapp.authorization import check_permission
42 from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet
43 from lp.testing import (
44+ admin_logged_in,
45+ person_logged_in,
46 run_with_login,
47 with_celebrity_logged_in,
48 )
49@@ -76,6 +79,20 @@
50 # And that means they can edit the proposal too
51 self.assertCanEdit(person, proposal)
52
53+ def test_reviewer_can_edit_git_merge_proposal(self):
54+ person = self.factory.makePerson()
55+ product = self.factory.makeProduct()
56+ [target] = self.factory.makeGitRefs(target=product)
57+ [source] = self.factory.makeGitRefs(target=product)
58+ mp = self.factory.makeBranchMergeProposalForGit(
59+ source_ref=source, target_ref=target)
60+ with person_logged_in(person):
61+ self.assertFalse(check_permission('launchpad.Edit', mp))
62+ with admin_logged_in():
63+ target.repository.reviewer = person
64+ with person_logged_in(person):
65+ self.assertTrue(check_permission('launchpad.Edit', mp))
66+
67
68 class TestViewMergeProposal(PermissionTest):
69
70
71=== modified file 'lib/lp/security.py'
72--- lib/lp/security.py 2016-10-12 23:24:24 +0000
73+++ lib/lp/security.py 2016-11-15 11:55:35 +0000
74@@ -302,16 +302,11 @@
75 usedfor = Interface
76
77 def checkUnauthenticated(self):
78- return (
79- self.forwardCheckUnauthenticated(self.obj, 'launchpad.View') or
80- self.forwardCheckUnauthenticated(
81- self.obj, 'launchpad.SubscriberView'))
82+ return self.forwardCheckUnauthenticated(permission='launchpad.View')
83
84 def checkAuthenticated(self, user):
85- return (
86- self.forwardCheckAuthenticated(user, self.obj, 'launchpad.View') or
87- self.forwardCheckAuthenticated(
88- user, self.obj, 'launchpad.SubscriberView'))
89+ return self.forwardCheckAuthenticated(
90+ user, permission='launchpad.View')
91
92
93 class AdminByAdminsTeam(AuthorizationBase):
94@@ -2462,10 +2457,11 @@
95 * the reviewer for the merge_target
96 * an administrator
97 """
98- return (user.inTeam(self.obj.registrant) or
99+ if (user.inTeam(self.obj.registrant) or
100 user.inTeam(self.obj.merge_source.owner) or
101- self.forwardCheckAuthenticated(user, self.obj.merge_target) or
102- user.inTeam(self.obj.merge_target.reviewer))
103+ user.inTeam(self.obj.merge_target.reviewer)):
104+ return True
105+ return self.forwardCheckAuthenticated(user, self.obj.merge_target)
106
107
108 class AdminDistroSeriesLanguagePacks(
109@@ -2618,6 +2614,23 @@
110 Archive.id, And(Archive.id == self.obj.id, filter)).is_empty()
111
112
113+class LimitedViewArchive(AuthorizationBase):
114+ """Restricted existence knowledge of private archives.
115+
116+ Just delegate to SubscriberView, since that includes View.
117+ """
118+ permission = 'launchpad.LimitedView'
119+ usedfor = IArchive
120+
121+ def checkUnauthenticated(self):
122+ return self.forwardCheckUnauthenticated(
123+ permission='launchpad.SubscriberView')
124+
125+ def checkAuthenticated(self, user):
126+ return self.forwardCheckAuthenticated(
127+ user, permission='launchpad.SubscriberView')
128+
129+
130 class EditArchive(AuthorizationBase):
131 """Restrict archive editing operations.
132
133@@ -2810,18 +2823,12 @@
134 usedfor = ISourcePackagePublishingHistory
135
136 def checkUnauthenticated(self):
137- return (
138- self.forwardCheckUnauthenticated(
139- self.obj.archive, 'launchpad.View') or
140- self.forwardCheckUnauthenticated(
141- self.obj.archive, 'launchpad.SubscriberView'))
142+ return self.forwardCheckUnauthenticated(
143+ self.obj.archive, 'launchpad.SubscriberView')
144
145 def checkAuthenticated(self, user):
146- return (
147- self.forwardCheckAuthenticated(
148- user, self.obj.archive, 'launchpad.View') or
149- self.forwardCheckAuthenticated(
150- user, self.obj.archive, 'launchpad.SubscriberView'))
151+ return self.forwardCheckAuthenticated(
152+ user, self.obj.archive, 'launchpad.SubscriberView')
153
154
155 class EditPublishing(DelegatedAuthorization):