Merge lp:~twom/launchpad/branch-permissions-empty-default into lp:launchpad

Proposed by Tom Wardill on 2018-10-17
Status: Merged
Merged at revision: 18799
Proposed branch: lp:~twom/launchpad/branch-permissions-empty-default
Merge into: lp:launchpad
Diff against target: 213 lines (+127/-14)
2 files modified
lib/lp/code/xmlrpc/git.py (+9/-11)
lib/lp/code/xmlrpc/tests/test_git.py (+118/-3)
To merge this branch: bzr merge lp:~twom/launchpad/branch-permissions-empty-default
Reviewer Review Type Date Requested Status
Colin Watson 2018-10-17 Approve on 2018-10-17
Review via email: mp+356914@code.launchpad.net

Commit message

Add default no permissions rule to non-repository-owners

Description of the change

Users that have access to a repository should have a default no permissions grant, unless overridden by a specific grant to them.

To post a comment you must log in.
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/code/xmlrpc/git.py'
2--- lib/lp/code/xmlrpc/git.py 2018-10-15 13:36:07 +0000
3+++ lib/lp/code/xmlrpc/git.py 2018-10-17 10:20:54 +0000
4@@ -373,10 +373,6 @@
5 'permissions': ['create', 'push'],
6 })
7 continue
8- # If we otherwise don't have any matching grants,
9- # we can ignore this rule
10- if not matching_grants:
11- continue
12
13 # Permissions are a union of all the applicable grants
14 union_permissions = set()
15@@ -397,14 +393,16 @@
16 'permissions': sorted_permissions,
17 })
18
19- # The last rule for a repository owner is a default permission
20- # for everything. This is overridden by any matching rules previously
21- # in the list
22+ # The last rule is a default wildcard. The repository owner gets
23+ # permissions to everything, whereas other people get no permissions.
24 if is_owner:
25- result.append(
26- {'ref_pattern': '*',
27- 'permissions': ['create', 'push', 'force_push'],
28- })
29+ default_permissions = ['create', 'push', 'force_push']
30+ else:
31+ default_permissions = []
32+ result.append(
33+ {'ref_pattern': '*',
34+ 'permissions': default_permissions,
35+ })
36
37 return result
38
39
40=== modified file 'lib/lp/code/xmlrpc/tests/test_git.py'
41--- lib/lp/code/xmlrpc/tests/test_git.py 2018-10-15 13:36:07 +0000
42+++ lib/lp/code/xmlrpc/tests/test_git.py 2018-10-17 10:20:54 +0000
43@@ -284,6 +284,10 @@
44 'ref_pattern': Equals('refs/heads/*'),
45 'permissions': Equals(['create', 'push']),
46 }),
47+ MatchesDict({
48+ 'ref_pattern': Equals('*'),
49+ 'permissions': Equals([]),
50+ }),
51 ]))
52
53 def test_listRefRules_with_other_grants(self):
54@@ -306,7 +310,11 @@
55 MatchesDict({
56 'ref_pattern': Equals('refs/heads/*'),
57 'permissions': Equals(['push']),
58- })
59+ }),
60+ MatchesDict({
61+ 'ref_pattern': Equals('*'),
62+ 'permissions': Equals([]),
63+ }),
64 ]))
65
66 def test_listRefRules_owner_has_default(self):
67@@ -419,7 +427,16 @@
68 results = self.git_api.listRefRules(
69 repository.getInternalPath(),
70 {'uid': requester.id})
71- self.assertEqual(0, len(results))
72+ self.assertThat(results, MatchesListwise([
73+ MatchesDict({
74+ 'ref_pattern': Equals('refs/heads/*'),
75+ 'permissions': Equals([]),
76+ }),
77+ MatchesDict({
78+ 'ref_pattern': Equals('*'),
79+ 'permissions': Equals([]),
80+ }),
81+ ]))
82
83 def test_listRefRules_owner_has_default_with_other_grant(self):
84 owner = self.factory.makePerson()
85@@ -434,7 +451,6 @@
86 results = self.git_api.listRefRules(
87 repository.getInternalPath(),
88 {'uid': owner.id})
89- self.assertEqual(len(results), 2)
90 # Default grant should be last in pattern
91 self.assertThat(results, MatchesListwise([
92 MatchesDict({
93@@ -770,6 +786,10 @@
94
95 self.assertThat(results, MatchesListwise([
96 MatchesDict(
97+ {'ref_pattern': Equals('refs/heads/stable/next'),
98+ 'permissions': Equals([]),
99+ }),
100+ MatchesDict(
101 {'ref_pattern': Equals('refs/heads/archived/*'),
102 'permissions': Equals(['create']),
103 }),
104@@ -785,6 +805,10 @@
105 {'ref_pattern': Equals('refs/tags/*'),
106 'permissions': Equals(['create']),
107 }),
108+ MatchesDict(
109+ {'ref_pattern': Equals('*'),
110+ 'permissions': Equals([]),
111+ }),
112 ]))
113
114 def test_listRefRules_grantee_example_three(self):
115@@ -832,9 +856,100 @@
116
117 self.assertThat(results, MatchesListwise([
118 MatchesDict(
119+ {'ref_pattern': Equals('refs/heads/stable/next'),
120+ 'permissions': Equals([]),
121+ }),
122+ MatchesDict(
123+ {'ref_pattern': Equals('refs/heads/archived/*'),
124+ 'permissions': Equals([]),
125+ }),
126+ MatchesDict(
127+ {'ref_pattern': Equals('refs/heads/stable/*'),
128+ 'permissions': Equals([]),
129+ }),
130+ MatchesDict(
131 {'ref_pattern': Equals('refs/heads/*/next'),
132 'permissions': Equals(['push', 'force_push']),
133 }),
134+ MatchesDict(
135+ {'ref_pattern': Equals('refs/tags/*'),
136+ 'permissions': Equals([]),
137+ }),
138+ MatchesDict(
139+ {'ref_pattern': Equals('*'),
140+ 'permissions': Equals([]),
141+ }),
142+ ]))
143+
144+ def test_listRefRules_grantee_example_four(self):
145+ user_a = self.factory.makePerson()
146+ user_b = self.factory.makePerson()
147+ user_c = self.factory.makePerson()
148+ user_d = self.factory.makePerson()
149+ stable_team = self.factory.makeTeam(members=[user_a, user_b])
150+ next_team = self.factory.makeTeam(members=[user_b, user_c])
151+
152+ repository = removeSecurityProxy(
153+ self.factory.makeGitRepository(owner=user_a))
154+
155+ rule = self.factory.makeGitRule(
156+ repository, ref_pattern=u'refs/heads/stable/next')
157+ self.factory.makeGitRuleGrant(
158+ rule=rule, grantee=user_a, can_force_push=True)
159+
160+ rule = self.factory.makeGitRule(
161+ repository, ref_pattern=u'refs/heads/archived/*')
162+ self.factory.makeGitRuleGrant(
163+ rule=rule, grantee=user_a)
164+ self.factory.makeGitRuleGrant(
165+ rule=rule, grantee=user_b, can_create=True)
166+
167+ rule = self.factory.makeGitRule(
168+ repository, ref_pattern=u'refs/heads/stable/*')
169+ self.factory.makeGitRuleGrant(
170+ rule=rule, grantee=stable_team, can_push=True)
171+
172+ rule = self.factory.makeGitRule(
173+ repository, ref_pattern=u'refs/heads/*/next')
174+ self.factory.makeGitRuleGrant(
175+ rule=rule, grantee=next_team, can_force_push=True)
176+
177+ rule = self.factory.makeGitRule(
178+ repository, ref_pattern=u'refs/tags/*')
179+ self.factory.makeGitRuleGrant(
180+ rule=rule, grantee=user_a, can_create=True)
181+ self.factory.makeGitRuleGrant(
182+ rule=rule, grantee=stable_team, can_create=True)
183+
184+ results = self.git_api.listRefRules(
185+ repository.getInternalPath(),
186+ {'uid': user_d.id})
187+
188+ self.assertThat(results, MatchesListwise([
189+ MatchesDict(
190+ {'ref_pattern': Equals('refs/heads/stable/next'),
191+ 'permissions': Equals([]),
192+ }),
193+ MatchesDict(
194+ {'ref_pattern': Equals('refs/heads/archived/*'),
195+ 'permissions': Equals([]),
196+ }),
197+ MatchesDict(
198+ {'ref_pattern': Equals('refs/heads/stable/*'),
199+ 'permissions': Equals([]),
200+ }),
201+ MatchesDict(
202+ {'ref_pattern': Equals('refs/heads/*/next'),
203+ 'permissions': Equals([]),
204+ }),
205+ MatchesDict(
206+ {'ref_pattern': Equals('refs/tags/*'),
207+ 'permissions': Equals([]),
208+ }),
209+ MatchesDict(
210+ {'ref_pattern': Equals('*'),
211+ 'permissions': Equals([]),
212+ }),
213 ]))
214
215