Merge ~ines-almeida/launchpad:project-tokens/update-models into launchpad:master
- Git
- lp:~ines-almeida/launchpad
- project-tokens/update-models
- Merge into master
Status: | Merged |
---|---|
Approved by: | Ines Almeida |
Approved revision: | bd5a112e360f894e0e7d0bd03615ac0eb01f3d8f |
Merge reported by: | Otto Co-Pilot |
Merged at revision: | not available |
Proposed branch: | ~ines-almeida/launchpad:project-tokens/update-models |
Merge into: | launchpad:master |
Prerequisite: | ~ines-almeida/launchpad:project-tokens/refactor-access-token-tests |
Diff against target: |
915 lines (+542/-95) 7 files modified
lib/lp/code/xmlrpc/git.py (+4/-1) lib/lp/code/xmlrpc/tests/test_git.py (+365/-61) lib/lp/registry/model/person.py (+2/-5) lib/lp/registry/model/product.py (+8/-3) lib/lp/services/auth/interfaces.py (+9/-1) lib/lp/services/auth/model.py (+52/-21) lib/lp/services/auth/tests/test_model.py (+102/-3) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Colin Watson (community) | Approve | ||
Review via email: mp+451542@code.launchpad.net |
Commit message
Update Access Token model and logic to enable Project scoped Access Tokens
This change introduces Projects as possible targets for Access Tokens similarly to what already existed for Git Repository, with the difference that a Project Access Token can be used to authenticate requests against any Git Repository within that Project.
Description of the change
Changes in this MP:
- Added `project` and a field within the `AccessToken` model, enabling Access Tokens to be targeted at Projects (before only Git Repositories could be targets for Access Tokens)
- Enabled using Project tokens to authenticate against requests similarly as Git Repository Access Tokens worked. Project Access Tokens can be used to authenticate against any of the Project's Git Repositories
Changes not in this MP:
- No interfaces (UI and API) to add Project Access Tokens are exposed
- 9bae4b0... by Ines Almeida
-
Address MP comments
- bd5a112... by Ines Almeida
-
Refactor AccessToken.
findByTarget query and unit tests
Ines Almeida (ines-almeida) wrote : | # |
This MP is ready to merge once its DB dependency is deployed to production
Preview Diff
1 | diff --git a/lib/lp/code/xmlrpc/git.py b/lib/lp/code/xmlrpc/git.py |
2 | index 3452fcd..7b450c0 100644 |
3 | --- a/lib/lp/code/xmlrpc/git.py |
4 | +++ b/lib/lp/code/xmlrpc/git.py |
5 | @@ -195,7 +195,10 @@ class GitAPI(LaunchpadXMLRPCView): |
6 | or access_token.owner.account_status != AccountStatus.ACTIVE |
7 | ): |
8 | raise faults.Unauthorized() |
9 | - if repository is not None and access_token.target != repository: |
10 | + if repository is not None and ( |
11 | + access_token.target != repository |
12 | + and access_token.target != repository.target |
13 | + ): |
14 | raise faults.Unauthorized() |
15 | access_token.updateLastUsed() |
16 | return AccessTokenVerificationResult(access_token) |
17 | diff --git a/lib/lp/code/xmlrpc/tests/test_git.py b/lib/lp/code/xmlrpc/tests/test_git.py |
18 | index 15e4f09..add6348 100644 |
19 | --- a/lib/lp/code/xmlrpc/tests/test_git.py |
20 | +++ b/lib/lp/code/xmlrpc/tests/test_git.py |
21 | @@ -1407,6 +1407,27 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory): |
22 | access_token_id=removeSecurityProxy(token).id, |
23 | ) |
24 | |
25 | + def test_confirm_git_repository_with_project_access_token(self): |
26 | + # Similarly to git repository tokens, a project access token cannot be |
27 | + # used to authorize confirming repository creation. |
28 | + requester = self.factory.makePerson() |
29 | + project = self.factory.makeProduct(owner=requester) |
30 | + repository = self.factory.makeGitRepository( |
31 | + target=project, |
32 | + owner=requester, |
33 | + status=GitRepositoryStatus.CREATING, |
34 | + ) |
35 | + _, token = self.factory.makeAccessToken( |
36 | + owner=requester, |
37 | + target=project, |
38 | + scopes=[AccessTokenScope.REPOSITORY_PUSH], |
39 | + ) |
40 | + self.assertConfirmRepoCreationUnauthorized( |
41 | + requester, |
42 | + repository, |
43 | + access_token_id=removeSecurityProxy(token).id, |
44 | + ) |
45 | + |
46 | def test_abort_repo_creation(self): |
47 | requester = self.factory.makePerson() |
48 | repo = self.factory.makeGitRepository(owner=requester) |
49 | @@ -1636,6 +1657,27 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory): |
50 | access_token_id=removeSecurityProxy(token).id, |
51 | ) |
52 | |
53 | + def test_abort_git_repository_with_project_access_token(self): |
54 | + # Similarly to git repository tokens, a project access token cannot be |
55 | + # used to authorize aborting repository creation. |
56 | + requester = self.factory.makePerson() |
57 | + project = self.factory.makeProduct(owner=requester) |
58 | + repository = self.factory.makeGitRepository( |
59 | + target=project, |
60 | + owner=requester, |
61 | + status=GitRepositoryStatus.CREATING, |
62 | + ) |
63 | + _, token = self.factory.makeAccessToken( |
64 | + owner=requester, |
65 | + target=project, |
66 | + scopes=[AccessTokenScope.REPOSITORY_PUSH], |
67 | + ) |
68 | + self.assertAbortRepoCreationUnauthorized( |
69 | + requester, |
70 | + repository, |
71 | + access_token_id=removeSecurityProxy(token).id, |
72 | + ) |
73 | + |
74 | def test_abort_git_repository_creation_of_non_existing_repository(self): |
75 | owner = self.factory.makePerson() |
76 | repo = removeSecurityProxy(self.factory.makeGitRepository(owner=owner)) |
77 | @@ -2634,6 +2676,110 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory): |
78 | macaroon_raw=macaroon.serialize(), |
79 | ) |
80 | |
81 | + def _assert_translatePath_expected( |
82 | + self, requester, token, repository, expected_success=True, scope="read" |
83 | + ): |
84 | + with person_logged_in(requester): |
85 | + path = "/%s" % repository.unique_name |
86 | + private = ( |
87 | + repository.information_type == InformationType.PRIVATESECURITY |
88 | + ) |
89 | + |
90 | + login(ANONYMOUS) |
91 | + if expected_success: |
92 | + self.assertTranslates( |
93 | + requester, |
94 | + path, |
95 | + repository, |
96 | + permission=scope, |
97 | + readable=(scope == "read"), |
98 | + writable=(scope == "write"), |
99 | + private=private, |
100 | + access_token_id=removeSecurityProxy(token).id, |
101 | + ) |
102 | + else: |
103 | + self.assertUnauthorized( |
104 | + requester, |
105 | + path, |
106 | + permission=scope, |
107 | + access_token_id=removeSecurityProxy(token).id, |
108 | + ) |
109 | + |
110 | + def test_translatePath_user_project_access_token_pull(self): |
111 | + # A user with a suitable project access token can pull from a |
112 | + # repository that belongs to that project, but not others, even if they |
113 | + # own them. |
114 | + requester = self.factory.makePerson() |
115 | + project = self.factory.makeProduct(owner=requester) |
116 | + with person_logged_in(requester): |
117 | + _, token = self.factory.makeAccessToken( |
118 | + owner=requester, |
119 | + target=project, |
120 | + scopes=[AccessTokenScope.REPOSITORY_PULL], |
121 | + ) |
122 | + |
123 | + repositories_access = [ |
124 | + (self.factory.makeGitRepository(), False), |
125 | + (self.factory.makeGitRepository(owner=requester), False), |
126 | + (self.factory.makeGitRepository(target=project), True), |
127 | + ( |
128 | + self.factory.makeGitRepository( |
129 | + owner=requester, target=project |
130 | + ), |
131 | + True, |
132 | + ), |
133 | + ( |
134 | + self.factory.makeGitRepository( |
135 | + owner=requester, |
136 | + target=project, |
137 | + information_type=InformationType.PRIVATESECURITY, |
138 | + ), |
139 | + True, |
140 | + ), |
141 | + ] |
142 | + |
143 | + for repository, expected_success in repositories_access: |
144 | + self._assert_translatePath_expected( |
145 | + requester, token, repository, expected_success |
146 | + ) |
147 | + |
148 | + def test_translatePath_user_project_access_token_push(self): |
149 | + # A user with a suitable project access token can push from a |
150 | + # repository that belongs to that project, but not others, even if they |
151 | + # own them. |
152 | + requester = self.factory.makePerson() |
153 | + project = self.factory.makeProduct(owner=requester) |
154 | + with person_logged_in(requester): |
155 | + _, token = self.factory.makeAccessToken( |
156 | + owner=requester, |
157 | + target=project, |
158 | + scopes=[AccessTokenScope.REPOSITORY_PUSH], |
159 | + ) |
160 | + |
161 | + repositories_access = [ |
162 | + (self.factory.makeGitRepository(), False), |
163 | + (self.factory.makeGitRepository(owner=requester), False), |
164 | + ( |
165 | + self.factory.makeGitRepository( |
166 | + owner=requester, target=project |
167 | + ), |
168 | + True, |
169 | + ), |
170 | + ( |
171 | + self.factory.makeGitRepository( |
172 | + owner=requester, |
173 | + target=project, |
174 | + information_type=InformationType.PRIVATESECURITY, |
175 | + ), |
176 | + True, |
177 | + ), |
178 | + ] |
179 | + |
180 | + for repository, expected_success in repositories_access: |
181 | + self._assert_translatePath_expected( |
182 | + requester, token, repository, expected_success, "write" |
183 | + ) |
184 | + |
185 | def test_translatePath_user_access_token_pull(self): |
186 | # A user with a suitable access token can pull from the |
187 | # corresponding repository, but not others, even if they own them. |
188 | @@ -2679,23 +2825,6 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory): |
189 | access_token_id=removeSecurityProxy(token).id, |
190 | ) |
191 | |
192 | - def test_translatePath_user_access_token_pull_wrong_scope(self): |
193 | - # A user with an access token that does not have the repository:pull |
194 | - # scope cannot pull from the corresponding repository. |
195 | - requester = self.factory.makePerson() |
196 | - repository = self.factory.makeGitRepository(owner=requester) |
197 | - _, token = self.factory.makeAccessToken( |
198 | - owner=requester, |
199 | - target=repository, |
200 | - scopes=[AccessTokenScope.REPOSITORY_BUILD_STATUS], |
201 | - ) |
202 | - self.assertPermissionDenied( |
203 | - requester, |
204 | - "/%s" % repository.unique_name, |
205 | - permission="read", |
206 | - access_token_id=removeSecurityProxy(token).id, |
207 | - ) |
208 | - |
209 | def test_translatePath_user_access_token_push(self): |
210 | # A user with a suitable access token can push to the corresponding |
211 | # repository, but not others, even if they own them. |
212 | @@ -2743,23 +2872,66 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory): |
213 | access_token_id=removeSecurityProxy(token).id, |
214 | ) |
215 | |
216 | - def test_translatePath_user_access_token_push_wrong_scope(self): |
217 | - # A user with an access token that does not have the repository:push |
218 | - # scope cannot push to the corresponding repository. |
219 | - requester = self.factory.makePerson() |
220 | - repository = self.factory.makeGitRepository(owner=requester) |
221 | + def _assert_translatePath_permission_denied_wrong_scope( |
222 | + self, requester, repository, token_target, scope |
223 | + ): |
224 | + wrong_scope = ( |
225 | + AccessTokenScope.REPOSITORY_PUSH |
226 | + if scope == "read" |
227 | + else AccessTokenScope.REPOSITORY_PULL |
228 | + ) |
229 | _, token = self.factory.makeAccessToken( |
230 | owner=requester, |
231 | - target=repository, |
232 | - scopes=[AccessTokenScope.REPOSITORY_PULL], |
233 | + target=token_target, |
234 | + scopes=[AccessTokenScope.REPOSITORY_BUILD_STATUS, wrong_scope], |
235 | ) |
236 | self.assertPermissionDenied( |
237 | requester, |
238 | "/%s" % repository.unique_name, |
239 | - permission="write", |
240 | + permission=scope, |
241 | access_token_id=removeSecurityProxy(token).id, |
242 | ) |
243 | |
244 | + def test_translatePath_user_git_access_token_pull_wrong_scope(self): |
245 | + # A user with a git repository access token that does not have the |
246 | + # repository:pull scope cannot pull from the corresponding repository. |
247 | + requester = self.factory.makePerson() |
248 | + repository = self.factory.makeGitRepository(owner=requester) |
249 | + self._assert_translatePath_permission_denied_wrong_scope( |
250 | + requester, repository, token_target=repository, scope="read" |
251 | + ) |
252 | + |
253 | + def test_translatePath_user_project_access_token_pull_wrong_scope(self): |
254 | + # A user with a project access token that does not have the |
255 | + # repository:pull scope cannot pull from the corresponding repository. |
256 | + requester = self.factory.makePerson() |
257 | + project = self.factory.makeProduct(owner=requester) |
258 | + repository = self.factory.makeGitRepository( |
259 | + owner=requester, target=project |
260 | + ) |
261 | + self._assert_translatePath_permission_denied_wrong_scope( |
262 | + requester, repository, token_target=project, scope="read" |
263 | + ) |
264 | + |
265 | + def test_translatePath_user_git_access_token_push_wrong_scope(self): |
266 | + # A user with a git repository access token that does not have the |
267 | + # repository:push scope cannot push to the corresponding repository. |
268 | + requester = self.factory.makePerson() |
269 | + repository = self.factory.makeGitRepository(owner=requester) |
270 | + self._assert_translatePath_permission_denied_wrong_scope( |
271 | + requester, repository, token_target=repository, scope="write" |
272 | + ) |
273 | + |
274 | + def test_translatePath_user_project_access_token_push_wrong_scope(self): |
275 | + # A user with a project access token that does not have the |
276 | + # repository:push scope cannot push to the corresponding repository. |
277 | + requester = self.factory.makePerson() |
278 | + project = self.factory.makeProduct(owner=requester) |
279 | + repository = self.factory.makeGitRepository(target=project) |
280 | + self._assert_translatePath_permission_denied_wrong_scope( |
281 | + requester, repository, token_target=project, scope="write" |
282 | + ) |
283 | + |
284 | def test_translatePath_user_access_token_nonexistent(self): |
285 | # Attempting to pass a nonexistent access token ID returns |
286 | # Unauthorized. |
287 | @@ -2927,15 +3099,12 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory): |
288 | auth_params, |
289 | ) |
290 | |
291 | - def test_getMergeProposalURL_user_access_token(self): |
292 | - # The merge proposal URL is returned by LP for a non-default branch |
293 | - # pushed by a user with a suitable access token that has their |
294 | - # ordinary privileges on the corresponding repository. |
295 | - requester = self.factory.makePerson() |
296 | - repository = self._makeGitRepositoryWithRefs(owner=requester) |
297 | + def _assert_getMergeProposalURL_user_access_token( |
298 | + self, requester, repository, token_target |
299 | + ): |
300 | _, token = self.factory.makeAccessToken( |
301 | owner=requester, |
302 | - target=repository, |
303 | + target=token_target, |
304 | scopes=[AccessTokenScope.REPOSITORY_PUSH], |
305 | ) |
306 | auth_params = _make_auth_params( |
307 | @@ -2943,13 +3112,35 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory): |
308 | ) |
309 | self.assertHasMergeProposalURL(repository, "branch", auth_params) |
310 | |
311 | - def test_getMergeProposalURL_user_access_token_wrong_repository(self): |
312 | - # getMergeProposalURL refuses access tokens for a different |
313 | - # repository. |
314 | + def test_getMergeProposalURL_user_git_access_token(self): |
315 | + # The merge proposal URL is returned by LP for a non-default branch |
316 | + # pushed by a user with a suitable git repository access token that has |
317 | + # their ordinary privileges on a repository in that project. |
318 | requester = self.factory.makePerson() |
319 | repository = self._makeGitRepositoryWithRefs(owner=requester) |
320 | + self._assert_getMergeProposalURL_user_access_token( |
321 | + requester, repository, token_target=repository |
322 | + ) |
323 | + |
324 | + def test_getMergeProposalURL_user_project_access_token(self): |
325 | + # The merge proposal URL is returned by LP for a non-default branch |
326 | + # pushed by a user with a suitable project access token that has |
327 | + # their ordinary privileges on a repository in that project. |
328 | + requester = self.factory.makePerson() |
329 | + project = self.factory.makeProduct(owner=requester) |
330 | + repository = self._makeGitRepositoryWithRefs(target=project) |
331 | + self._assert_getMergeProposalURL_user_access_token( |
332 | + requester, repository, token_target=repository |
333 | + ) |
334 | + |
335 | + def _assert_getMergeProposalURL_user_access_token_wrong_repository( |
336 | + self, requester, token_target |
337 | + ): |
338 | + repository = self._makeGitRepositoryWithRefs(owner=requester) |
339 | _, token = self.factory.makeAccessToken( |
340 | - owner=requester, scopes=[AccessTokenScope.REPOSITORY_PUSH] |
341 | + target=token_target, |
342 | + owner=requester, |
343 | + scopes=[AccessTokenScope.REPOSITORY_PUSH], |
344 | ) |
345 | auth_params = _make_auth_params( |
346 | requester, access_token_id=removeSecurityProxy(token).id |
347 | @@ -2963,6 +3154,26 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory): |
348 | auth_params, |
349 | ) |
350 | |
351 | + def test_getMergeProposalURL_user_git_access_token_wrong_repository(self): |
352 | + # getMergeProposalURL refuses access tokens for a different |
353 | + # repository. |
354 | + requester = self.factory.makePerson() |
355 | + repository = self.factory.makeGitRepository(owner=requester) |
356 | + self._assert_getMergeProposalURL_user_access_token_wrong_repository( |
357 | + requester, token_target=repository |
358 | + ) |
359 | + |
360 | + def test_getMergeProposalURL_user_project_access_token_wrong_repository( |
361 | + self, |
362 | + ): |
363 | + # getMergeProposalURL refuses access tokens for repository from a |
364 | + # different project. |
365 | + requester = self.factory.makePerson() |
366 | + project = self.factory.makeProduct(owner=requester) |
367 | + self._assert_getMergeProposalURL_user_access_token_wrong_repository( |
368 | + requester, token_target=project |
369 | + ) |
370 | + |
371 | def test_getMergeProposalURL_code_import(self): |
372 | # A merge proposal URL from LP to Turnip is not returned for |
373 | # code import job as there is no User at the other end. |
374 | @@ -3464,12 +3675,15 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory): |
375 | macaroon.serialize(), |
376 | ) |
377 | |
378 | - def test_authenticateWithPassword_user_access_token(self): |
379 | + def _assert_authenticateWithPassword_user_access_token( |
380 | + self, |
381 | + requester, |
382 | + secret, |
383 | + token, |
384 | + ): |
385 | # A user with a suitable access token can authenticate using it, in |
386 | # which case we return both the access token and the uid for use by |
387 | # later calls. |
388 | - requester = self.factory.makePerson() |
389 | - secret, token = self.factory.makeAccessToken(owner=requester) |
390 | self.assertIsNone(removeSecurityProxy(token).date_last_used) |
391 | self.assertEqual( |
392 | { |
393 | @@ -3490,6 +3704,30 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory): |
394 | secret, |
395 | ) |
396 | |
397 | + def test_authenticateWithPassword_user_git_access_token(self): |
398 | + # A user with a suitable git repository access token can authenticate |
399 | + # using it |
400 | + requester = self.factory.makePerson() |
401 | + secret, token = self.factory.makeAccessToken(owner=requester) |
402 | + self._assert_authenticateWithPassword_user_access_token( |
403 | + requester, |
404 | + secret, |
405 | + token, |
406 | + ) |
407 | + |
408 | + def test_authenticateWithPassword_user_project_access_token(self): |
409 | + # A user with a suitable project access token can authenticate using it |
410 | + requester = self.factory.makePerson() |
411 | + project = self.factory.makeProduct(owner=requester) |
412 | + secret, token = self.factory.makeAccessToken( |
413 | + owner=requester, target=project |
414 | + ) |
415 | + self._assert_authenticateWithPassword_user_access_token( |
416 | + requester, |
417 | + secret, |
418 | + token, |
419 | + ) |
420 | + |
421 | def test_authenticateWithPassword_user_access_token_expired(self): |
422 | # An expired access token is rejected. |
423 | requester = self.factory.makePerson() |
424 | @@ -3855,21 +4093,76 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory): |
425 | macaroon_raw=macaroon.serialize(), |
426 | ) |
427 | |
428 | - def test_checkRefPermissions_user_access_token(self): |
429 | + def _assert_checkRefPermissions_permissions( |
430 | + self, requester, token, repository, expected_success |
431 | + ): |
432 | + ref_path = b"refs/heads/main" |
433 | + login(ANONYMOUS) |
434 | + if expected_success: |
435 | + # This expects that since we don't add any permission rules and the |
436 | + # requester is the owner of the repository, then the call returns |
437 | + # all 3 permissions |
438 | + expected_permissions = ["create", "push", "force_push"] |
439 | + else: |
440 | + expected_permissions = [] |
441 | + self.assertHasRefPermissions( |
442 | + requester, |
443 | + repository, |
444 | + [ref_path], |
445 | + {ref_path: expected_permissions}, |
446 | + access_token_id=removeSecurityProxy(token).id, |
447 | + ) |
448 | + |
449 | + def test_checkRefPermissions_user_project_access_token(self): |
450 | + # A user with a suitable access token targetted at a project, has their |
451 | + # ordinary privileges on repositories from the same project, but not |
452 | + # others, even if they own them. |
453 | + requester = self.factory.makePerson() |
454 | + project = self.factory.makeProduct(owner=requester) |
455 | + repositories = [ |
456 | + (self.factory.makeGitRepository(), False), |
457 | + (self.factory.makeGitRepository(owner=requester), False), |
458 | + (self.factory.makeGitRepository(target=project), False), |
459 | + ( |
460 | + self.factory.makeGitRepository( |
461 | + owner=requester, target=project |
462 | + ), |
463 | + True, |
464 | + ), |
465 | + ( |
466 | + self.factory.makeGitRepository( |
467 | + target=project, |
468 | + owner=requester, |
469 | + information_type=InformationType.PRIVATESECURITY, |
470 | + ), |
471 | + True, |
472 | + ), |
473 | + ] |
474 | + with person_logged_in(requester): |
475 | + _, token = self.factory.makeAccessToken( |
476 | + owner=requester, |
477 | + target=project, |
478 | + scopes=[AccessTokenScope.REPOSITORY_PUSH], |
479 | + ) |
480 | + |
481 | + for repository, expected_success in repositories: |
482 | + self._assert_checkRefPermissions_permissions( |
483 | + requester, token, repository, expected_success |
484 | + ) |
485 | + |
486 | + def test_checkRefPermissions_user_git_access_token(self): |
487 | # A user with a suitable access token has their ordinary privileges |
488 | # on the corresponding repository, but not others, even if they own |
489 | # them. |
490 | requester = self.factory.makePerson() |
491 | repositories = [ |
492 | - self.factory.makeGitRepository(owner=requester) for _ in range(2) |
493 | - ] |
494 | - repositories.append( |
495 | + self.factory.makeGitRepository(owner=requester), |
496 | + self.factory.makeGitRepository(owner=requester), |
497 | self.factory.makeGitRepository( |
498 | owner=requester, |
499 | information_type=InformationType.PRIVATESECURITY, |
500 | - ) |
501 | - ) |
502 | - ref_path = b"refs/heads/main" |
503 | + ), |
504 | + ] |
505 | tokens = [] |
506 | with person_logged_in(requester): |
507 | for repository in repositories: |
508 | @@ -3879,29 +4172,21 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory): |
509 | scopes=[AccessTokenScope.REPOSITORY_PUSH], |
510 | ) |
511 | tokens.append(token) |
512 | + |
513 | for i, repository in enumerate(repositories): |
514 | for j, token in enumerate(tokens): |
515 | - login(ANONYMOUS) |
516 | - if i == j: |
517 | - expected_permissions = ["create", "push", "force_push"] |
518 | - else: |
519 | - expected_permissions = [] |
520 | - self.assertHasRefPermissions( |
521 | - requester, |
522 | - repository, |
523 | - [ref_path], |
524 | - {ref_path: expected_permissions}, |
525 | - access_token_id=removeSecurityProxy(token).id, |
526 | + self._assert_checkRefPermissions_permissions( |
527 | + requester, token, repository, expected_success=(i == j) |
528 | ) |
529 | |
530 | - def test_checkRefPermissions_user_access_token_wrong_scope(self): |
531 | + def _assert_checkRefPermissions_user_access_token_wrong_scope( |
532 | + self, requester, repository, token_target |
533 | + ): |
534 | # A user with an access token that does not have the repository:push |
535 | # scope cannot push to any branch in the corresponding repository. |
536 | - requester = self.factory.makePerson() |
537 | - repository = self.factory.makeGitRepository(owner=requester) |
538 | _, token = self.factory.makeAccessToken( |
539 | owner=requester, |
540 | - target=repository, |
541 | + target=token_target, |
542 | scopes=[AccessTokenScope.REPOSITORY_PULL], |
543 | ) |
544 | ref_path = b"refs/heads/main" |
545 | @@ -3913,6 +4198,25 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory): |
546 | access_token_id=removeSecurityProxy(token).id, |
547 | ) |
548 | |
549 | + def test_checkRefPermissions_user_git_access_token_wrong_scope(self): |
550 | + # A user with an access token that does not have the repository:push |
551 | + # scope cannot push to any branch in the corresponding repository. |
552 | + requester = self.factory.makePerson() |
553 | + repository = self.factory.makeGitRepository(owner=requester) |
554 | + self._assert_checkRefPermissions_user_access_token_wrong_scope( |
555 | + requester, repository, token_target=repository |
556 | + ) |
557 | + |
558 | + def test_checkRefPermissions_user_project_access_token_wrong_scope(self): |
559 | + # A user with an access token that does not have the repository:push |
560 | + # scope cannot push to any branch in the corresponding repository. |
561 | + requester = self.factory.makePerson() |
562 | + project = self.factory.makeProduct(owner=requester) |
563 | + repository = self.factory.makeGitRepository(target=project) |
564 | + self._assert_checkRefPermissions_user_access_token_wrong_scope( |
565 | + requester, repository, token_target=project |
566 | + ) |
567 | + |
568 | def assertUpdatesRepackStats(self, repo): |
569 | start_time = datetime.now(timezone.utc) |
570 | self.assertIsNone( |
571 | diff --git a/lib/lp/registry/model/person.py b/lib/lp/registry/model/person.py |
572 | index 788333b..1bc9a03 100644 |
573 | --- a/lib/lp/registry/model/person.py |
574 | +++ b/lib/lp/registry/model/person.py |
575 | @@ -1206,13 +1206,10 @@ class Person( |
576 | ] |
577 | |
578 | if transitive: |
579 | - # getProductPrivacyFilter may also use TeamParticipation, so |
580 | - # ensure we use a different one. |
581 | - ownership_participation = ClassAlias(TeamParticipation) |
582 | clauses.extend( |
583 | [ |
584 | - Product._owner_id == ownership_participation.team_id, |
585 | - ownership_participation.person_id == self.id, |
586 | + Product._owner_id == TeamParticipation.team_id, |
587 | + TeamParticipation.person_id == self.id, |
588 | ] |
589 | ) |
590 | else: |
591 | diff --git a/lib/lp/registry/model/product.py b/lib/lp/registry/model/product.py |
592 | index f3bb669..4319feb 100644 |
593 | --- a/lib/lp/registry/model/product.py |
594 | +++ b/lib/lp/registry/model/product.py |
595 | @@ -31,6 +31,7 @@ from storm.expr import ( |
596 | Or, |
597 | Select, |
598 | ) |
599 | +from storm.info import ClassAlias |
600 | from storm.locals import ( |
601 | Bool, |
602 | DateTime, |
603 | @@ -1891,6 +1892,10 @@ class ProductSet: |
604 | if roles.in_admin or roles.in_commercial_admin: |
605 | return True |
606 | |
607 | + # In places where this method is used, they might want to use |
608 | + # TeamParticipation. This ensures that we use a different one. |
609 | + ownership_participation = ClassAlias(TeamParticipation) |
610 | + |
611 | # Normal users can see any project for which they can see either |
612 | # an entire policy or an artifact. |
613 | # XXX wgrant 2015-06-26: This is slower than ideal for people in |
614 | @@ -1905,12 +1910,12 @@ class ProductSet: |
615 | tables=( |
616 | AccessPolicyGrantFlat, |
617 | Join( |
618 | - TeamParticipation, |
619 | - TeamParticipation.team_id |
620 | + ownership_participation, |
621 | + ownership_participation.team_id |
622 | == AccessPolicyGrantFlat.grantee_id, |
623 | ), |
624 | ), |
625 | - where=(TeamParticipation.person == user), |
626 | + where=(ownership_participation.person_id == user.id), |
627 | ), |
628 | ), |
629 | False, |
630 | diff --git a/lib/lp/services/auth/interfaces.py b/lib/lp/services/auth/interfaces.py |
631 | index 6081830..10c3c58 100644 |
632 | --- a/lib/lp/services/auth/interfaces.py |
633 | +++ b/lib/lp/services/auth/interfaces.py |
634 | @@ -73,7 +73,15 @@ class IAccessToken(Interface): |
635 | description=_("The Git repository for which the token was issued."), |
636 | # Really IGitRepository, patched in lp.services.auth.webservice. |
637 | schema=Interface, |
638 | - required=True, |
639 | + required=False, |
640 | + readonly=True, |
641 | + ) |
642 | + |
643 | + project = Reference( |
644 | + title=_("Project"), |
645 | + description=_("The project for which the token was issued."), |
646 | + schema=Interface, |
647 | + required=False, |
648 | readonly=True, |
649 | ) |
650 | |
651 | diff --git a/lib/lp/services/auth/model.py b/lib/lp/services/auth/model.py |
652 | index f595637..2b4fe2b 100644 |
653 | --- a/lib/lp/services/auth/model.py |
654 | +++ b/lib/lp/services/auth/model.py |
655 | @@ -21,6 +21,7 @@ from zope.security.proxy import removeSecurityProxy |
656 | |
657 | from lp.code.interfaces.gitcollection import IAllGitRepositories |
658 | from lp.code.interfaces.gitrepository import IGitRepository |
659 | +from lp.registry.interfaces.product import IProduct |
660 | from lp.registry.model.teammembership import TeamParticipation |
661 | from lp.services.auth.enums import AccessTokenScope |
662 | from lp.services.auth.interfaces import IAccessToken, IAccessTokenSet |
663 | @@ -50,9 +51,12 @@ class AccessToken(StormBase): |
664 | |
665 | description = Unicode(name="description", allow_none=False) |
666 | |
667 | - git_repository_id = Int(name="git_repository", allow_none=False) |
668 | + git_repository_id = Int(name="git_repository", allow_none=True) |
669 | git_repository = Reference(git_repository_id, "GitRepository.id") |
670 | |
671 | + project_id = Int(name="project", allow_none=True) |
672 | + project = Reference(project_id, "Product.id") |
673 | + |
674 | _scopes = JSON(name="scopes", allow_none=False) |
675 | |
676 | date_last_used = DateTime( |
677 | @@ -76,6 +80,8 @@ class AccessToken(StormBase): |
678 | self.description = description |
679 | if IGitRepository.providedBy(target): |
680 | self.git_repository = target |
681 | + elif IProduct.providedBy(target): |
682 | + self.project = target |
683 | else: |
684 | raise TypeError("Unsupported target: {!r}".format(target)) |
685 | self.scopes = scopes |
686 | @@ -85,7 +91,7 @@ class AccessToken(StormBase): |
687 | @property |
688 | def target(self): |
689 | """See `IAccessToken`.""" |
690 | - return self.git_repository |
691 | + return self.git_repository or self.project |
692 | |
693 | @property |
694 | def scopes(self): |
695 | @@ -181,32 +187,57 @@ class AccessTokenSet: |
696 | self, target, visible_by_user=None, include_expired=False |
697 | ): |
698 | """See `IAccessTokenSet`.""" |
699 | + |
700 | clauses = [] |
701 | if IGitRepository.providedBy(target): |
702 | clauses.append(AccessToken.git_repository == target) |
703 | - if visible_by_user is not None: |
704 | - collection = ( |
705 | - getUtility(IAllGitRepositories) |
706 | - .visibleByUser(visible_by_user) |
707 | - .ownedByTeamMember(visible_by_user) |
708 | + elif IProduct.providedBy(target): |
709 | + clauses.append(AccessToken.project == target) |
710 | + else: |
711 | + raise TypeError("Unsupported target: {!r}".format(target)) |
712 | + |
713 | + if visible_by_user is not None: |
714 | + # Evaluate if user owns the target (directly or indirectly). |
715 | + # The queries below might look more complex than they should to |
716 | + # evaluate ownership, but they are more performant as this will run |
717 | + # in one single query in the end. |
718 | + if IGitRepository.providedBy(target): |
719 | + user_owns_target = AccessToken.git_repository_id.is_in( |
720 | + removeSecurityProxy( |
721 | + ( |
722 | + getUtility(IAllGitRepositories) |
723 | + .visibleByUser(visible_by_user) |
724 | + .ownedByTeamMember(visible_by_user) |
725 | + ).getRepositoryIds() |
726 | + )._get_select() |
727 | ) |
728 | - ids = collection.getRepositoryIds() |
729 | - clauses.append( |
730 | - Or( |
731 | - AccessToken.owner_id.is_in( |
732 | - Select( |
733 | - TeamParticipation.team_id, |
734 | - where=TeamParticipation.person |
735 | - == visible_by_user, |
736 | - ) |
737 | - ), |
738 | - AccessToken.git_repository_id.is_in( |
739 | - removeSecurityProxy(ids)._get_select() |
740 | + else: |
741 | + # Avoid circular import |
742 | + from lp.registry.model.product import Product, ProductSet |
743 | + |
744 | + clauses.extend([Product.active, Product.id == target.id]) |
745 | + user_owns_target = AccessToken.project_id.is_in( |
746 | + Select( |
747 | + Product.id, |
748 | + where=And( |
749 | + Product._owner_id == TeamParticipation.team_id, |
750 | + TeamParticipation.person_id == visible_by_user.id, |
751 | + ProductSet.getProductPrivacyFilter( |
752 | + visible_by_user |
753 | + ), |
754 | ), |
755 | ) |
756 | ) |
757 | - else: |
758 | - raise TypeError("Unsupported target: {!r}".format(target)) |
759 | + |
760 | + user_owns_token = AccessToken.owner_id.is_in( |
761 | + Select( |
762 | + TeamParticipation.team_id, |
763 | + where=TeamParticipation.person_id == visible_by_user.id, |
764 | + ) |
765 | + ) |
766 | + |
767 | + clauses.append(Or(user_owns_target, user_owns_token)) |
768 | + |
769 | if not include_expired: |
770 | clauses.append( |
771 | Or( |
772 | diff --git a/lib/lp/services/auth/tests/test_model.py b/lib/lp/services/auth/tests/test_model.py |
773 | index ca33eec..a02904a 100644 |
774 | --- a/lib/lp/services/auth/tests/test_model.py |
775 | +++ b/lib/lp/services/auth/tests/test_model.py |
776 | @@ -22,6 +22,7 @@ from zope.component import getUtility |
777 | from zope.security.proxy import removeSecurityProxy |
778 | |
779 | from lp.code.interfaces.gitrepository import IGitRepository |
780 | +from lp.registry.interfaces.person import TeamMembershipPolicy |
781 | from lp.services.auth.enums import AccessTokenScope |
782 | from lp.services.auth.interfaces import IAccessTokenSet |
783 | from lp.services.auth.utils import create_access_token_secret |
784 | @@ -197,6 +198,11 @@ class TestAccessTokenGitRepository(TestAccessTokenBase, TestCaseWithFactory): |
785 | return self.factory.makeGitRepository(owner=owner) |
786 | |
787 | |
788 | +class TestAccessTokenProduct(TestAccessTokenBase, TestCaseWithFactory): |
789 | + def makeTarget(self, owner=None): |
790 | + return self.factory.makeProduct(owner=owner) |
791 | + |
792 | + |
793 | class TestAccessTokenSetBase: |
794 | layer = DatabaseFunctionalLayer |
795 | |
796 | @@ -226,7 +232,7 @@ class TestAccessTokenSetBase: |
797 | ) |
798 | |
799 | def test_getByID(self): |
800 | - secret, token = self.factory.makeAccessToken(target=self.makeTarget()) |
801 | + _, token = self.factory.makeAccessToken(target=self.makeTarget()) |
802 | token_id = removeSecurityProxy(token).id |
803 | self.assertEqual(token, getUtility(IAccessTokenSet).getByID(token_id)) |
804 | self.assertIsNone(getUtility(IAccessTokenSet).getByID(token_id + 1)) |
805 | @@ -315,6 +321,94 @@ class TestAccessTokenSetBase: |
806 | ), |
807 | ) |
808 | |
809 | + def test_findByTarget_visible_by_owner(self): |
810 | + # If a user owns a target, they can see all the tokens for that target |
811 | + # regadless if they own the tokens. |
812 | + owner = self.factory.makePerson() |
813 | + owned_target = self.makeTarget(owner=owner) |
814 | + owned_target_tokens = [ |
815 | + self.factory.makeAccessToken(target=owned_target)[1], |
816 | + self.factory.makeAccessToken(target=owned_target)[1], |
817 | + ] |
818 | + self.factory.makeAccessToken(target=self.makeTarget())[1] |
819 | + self.factory.makeAccessToken(target=self.makeTarget())[1] |
820 | + |
821 | + self.assertContentEqual( |
822 | + owned_target_tokens, |
823 | + getUtility(IAccessTokenSet).findByTarget( |
824 | + owned_target, visible_by_user=owner |
825 | + ), |
826 | + ) |
827 | + |
828 | + def test_findByTarget_visible_by_team_member(self): |
829 | + # If a user belongs to a team that owns a target, they can see all the |
830 | + # tokens for that target regardless if they own the tokens. |
831 | + |
832 | + # Only restricted or moderated teams can own certain targets (Product) |
833 | + team = self.factory.makeTeam( |
834 | + membership_policy=TeamMembershipPolicy.RESTRICTED |
835 | + ) |
836 | + team_member = self.factory.makePerson() |
837 | + with person_logged_in(team.teamowner): |
838 | + team.addMember(team_member, team) |
839 | + |
840 | + owned_target = self.makeTarget(owner=team) |
841 | + owned_target_tokens = [ |
842 | + self.factory.makeAccessToken(target=owned_target)[1], |
843 | + self.factory.makeAccessToken(target=owned_target)[1], |
844 | + ] |
845 | + |
846 | + self.factory.makeAccessToken(target=self.makeTarget())[1] |
847 | + self.factory.makeAccessToken(target=self.makeTarget())[1] |
848 | + |
849 | + self.assertContentEqual( |
850 | + owned_target_tokens, |
851 | + getUtility(IAccessTokenSet).findByTarget( |
852 | + owned_target, visible_by_user=team_member |
853 | + ), |
854 | + ) |
855 | + |
856 | + def test_findByTarget_visible_by_user_inactive_target(self): |
857 | + # User cannot see tokens for inactive targets. |
858 | + |
859 | + owner = self.factory.makePerson() |
860 | + target = self.makeTarget(owner=owner) |
861 | + |
862 | + if IGitRepository.providedBy(target): |
863 | + self.skipTest( |
864 | + "Currently, Git repositories allow requests without scopes." |
865 | + ) |
866 | + |
867 | + removeSecurityProxy(target).active = False |
868 | + self.factory.makeAccessToken(target=target, owner=owner)[1] |
869 | + |
870 | + self.assertContentEqual( |
871 | + [], |
872 | + getUtility(IAccessTokenSet).findByTarget( |
873 | + target, visible_by_user=owner |
874 | + ), |
875 | + ) |
876 | + |
877 | + def test_findByTarget_visible_by_user_from_a_team_that_owns_token(self): |
878 | + # Method with a visible_by_user parameter, returns only tokens that |
879 | + # are owned by the `visible_by_user` Person or by a team that the |
880 | + # `visible_by_user` Person belongs to. |
881 | + target = self.makeTarget() |
882 | + team = self.factory.makeTeam() |
883 | + team_member = self.factory.makePerson() |
884 | + with person_logged_in(team.teamowner): |
885 | + team.addMember(team_member, team) |
886 | + _, token = self.factory.makeAccessToken(owner=team, target=target) |
887 | + self.factory.makeAccessToken(target=target) |
888 | + |
889 | + for user in [team.teamowner, team_member]: |
890 | + self.assertContentEqual( |
891 | + [token], |
892 | + getUtility(IAccessTokenSet).findByTarget( |
893 | + target, visible_by_user=user |
894 | + ), |
895 | + ) |
896 | + |
897 | def test_findByTarget_excludes_expired(self): |
898 | target = self.makeTarget() |
899 | _, current_token = self.factory.makeAccessToken(target=target) |
900 | @@ -432,8 +526,13 @@ class TestAccessTokenSetBase: |
901 | class TestGitRepositoryAccessTokenSet( |
902 | TestAccessTokenSetBase, TestCaseWithFactory |
903 | ): |
904 | - def makeTarget(self): |
905 | - return self.factory.makeGitRepository() |
906 | + def makeTarget(self, **kwargs): |
907 | + return self.factory.makeGitRepository(**kwargs) |
908 | + |
909 | + |
910 | +class TestProjectAccessTokenSet(TestAccessTokenSetBase, TestCaseWithFactory): |
911 | + def makeTarget(self, **kwargs): |
912 | + return self.factory.makeProduct(**kwargs) |
913 | |
914 | |
915 | class TestAccessTokenTargetBase: |
You might want to rebase this onto master so that the CI lint check can pass.
This can't land until https:/ /code.launchpad .net/~ines- almeida/ launchpad/ +git/launchpad/ +merge/ 451470 (or its successor) has landed on db-devel, has been deployed to production, and has been merged onto master.