Merge ~ines-almeida/launchpad:project-tokens/refactor-access-token-mixins into launchpad:master
- Git
- lp:~ines-almeida/launchpad
- project-tokens/refactor-access-token-mixins
- Merge into master
Status: | Merged |
---|---|
Approved by: | Ines Almeida |
Approved revision: | 3f598da50b719b5038e1264211e07685ebcf125f |
Merge reported by: | Otto Co-Pilot |
Merged at revision: | not available |
Proposed branch: | ~ines-almeida/launchpad:project-tokens/refactor-access-token-mixins |
Merge into: | launchpad:master |
Diff against target: |
677 lines (+306/-190) 6 files modified
lib/lp/code/interfaces/gitrepository.py (+14/-5) lib/lp/code/model/gitrepository.py (+7/-32) lib/lp/code/model/tests/test_gitrepository.py (+0/-152) lib/lp/services/auth/interfaces.py (+45/-0) lib/lp/services/auth/model.py (+26/-0) lib/lp/services/auth/tests/test_model.py (+214/-1) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Colin Watson (community) | Approve | ||
Review via email: mp+450898@code.launchpad.net |
Commit message
Move "issueAccessToken" logic to generic Access Token Target model and interface.
Logic was originally in Git Repository model/interface. Moving it to a generic place makes adding new access token targets much easier
Description of the change
This will make adding project-scoped access tokens much easier and without duplication
Ines Almeida (ines-almeida) wrote : | # |
I restructured the code according to Colin's suggestion.
I updated the code such that once we want to remove the Macaroon logic from the Git Repository, we only need to remove the methods that override the `(I)AccessToken
Note that I added a couple of unit tests that test the `issueAccessToken` endpoint that don't yet truly run (because the GitRepository tests skip them), but I think it makes sense to keep them to have coherence with the code (plus they will be used by other targets soon).
Colin Watson (cjwatson) wrote : | # |
I like this much better, thanks! Just a few tweaks.
- 930d35d... by Ines Almeida
-
Move "issueAccessToken" logic to generic Access Token Target model and interface.
Logic was originally in Git Repository model/interface. Moving it to a generic place makes adding new access token targets much easier
- 3f598da... by Ines Almeida
-
Move issueAccessToken unit tests to Access Token Target test file
Ines Almeida (ines-almeida) wrote : | # |
Thanks for the comments! I addressed them all and cleanup up (squashed) some commits.
Could you have a look at the first `# XXX ines-almeida 2023-09-08:` so check if the context is right and clear before I merge this?
Colin Watson (cjwatson) : | # |
Preview Diff
1 | diff --git a/lib/lp/code/interfaces/gitrepository.py b/lib/lp/code/interfaces/gitrepository.py |
2 | index 354d2df..f8b04fb 100644 |
3 | --- a/lib/lp/code/interfaces/gitrepository.py |
4 | +++ b/lib/lp/code/interfaces/gitrepository.py |
5 | @@ -83,7 +83,10 @@ from lp.registry.interfaces.personproduct import IPersonProductFactory |
6 | from lp.registry.interfaces.product import IProduct |
7 | from lp.registry.interfaces.role import IPersonRoles |
8 | from lp.services.auth.enums import AccessTokenScope |
9 | -from lp.services.auth.interfaces import IAccessTokenTarget |
10 | +from lp.services.auth.interfaces import ( |
11 | + IAccessTokenTarget, |
12 | + IAccessTokenTargetEdit, |
13 | +) |
14 | from lp.services.fields import InlineObject, PersonChoice, PublicPersonChoice |
15 | from lp.services.webhooks.interfaces import IWebhookTarget |
16 | |
17 | @@ -129,7 +132,7 @@ def git_repository_name_validator(name): |
18 | return True |
19 | |
20 | |
21 | -class IGitRepositoryView(IHasRecipes): |
22 | +class IGitRepositoryView(IHasRecipes, IAccessTokenTarget): |
23 | """IGitRepository attributes that require launchpad.View permission.""" |
24 | |
25 | id = exported(Int(title=_("ID"), readonly=True, required=True)) |
26 | @@ -877,8 +880,14 @@ class IGitRepositoryView(IHasRecipes): |
27 | :return: A `ResultSet` of `IGitActivity`. |
28 | """ |
29 | |
30 | - # XXX cjwatson 2021-10-13: This should move to IAccessTokenTarget, but |
31 | - # currently has rather too much backward-compatibility code for that. |
32 | + # XXX ines-almeida 2023-09-08: This overwrites the definition in |
33 | + # IAccessTokenTarget because we want to generate serialised macaroons in |
34 | + # certain cases for git repositories specifically. Once |
35 | + # `snapcraft remote-build` stops using the old workflow (see |
36 | + # https://github.com/snapcore/snapcraft/pull/4270), this can be removed in |
37 | + # favour of the general definition in `IAccessTokenTarget`. |
38 | + # Note that `snap info snapcraft` still lists a number of older versions |
39 | + # of snapcraft from before that change that are still supported. |
40 | @operation_parameters( |
41 | description=TextLine( |
42 | title=_("A short description of the token."), required=False |
43 | @@ -1054,7 +1063,7 @@ class IGitRepositoryExpensiveRequest(Interface): |
44 | that is not an admin or a registry expert.""" |
45 | |
46 | |
47 | -class IGitRepositoryEdit(IWebhookTarget, IAccessTokenTarget): |
48 | +class IGitRepositoryEdit(IWebhookTarget, IAccessTokenTargetEdit): |
49 | """IGitRepository methods that require launchpad.Edit permission.""" |
50 | |
51 | @mutator_for(IGitRepositoryView["name"]) |
52 | diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py |
53 | index 85240eb..738a181 100644 |
54 | --- a/lib/lp/code/model/gitrepository.py |
55 | +++ b/lib/lp/code/model/gitrepository.py |
56 | @@ -126,9 +126,7 @@ from lp.registry.model.accesspolicy import ( |
57 | ) |
58 | from lp.registry.model.person import Person |
59 | from lp.registry.model.teammembership import TeamParticipation |
60 | -from lp.services.auth.interfaces import IAccessTokenSet |
61 | from lp.services.auth.model import AccessTokenTargetMixin |
62 | -from lp.services.auth.utils import create_access_token_secret |
63 | from lp.services.config import config |
64 | from lp.services.database import bulk |
65 | from lp.services.database.constants import DEFAULT, UTC_NOW |
66 | @@ -1880,26 +1878,6 @@ class GitRepository( |
67 | results, pre_iter_hook=preloadDataForActivities |
68 | ) |
69 | |
70 | - def _issuePersonalAccessToken( |
71 | - self, user, description, scopes, date_expires=None |
72 | - ): |
73 | - """Issue a personal access token for this repository.""" |
74 | - if user is None: |
75 | - raise Unauthorized( |
76 | - "Personal access tokens may only be issued for a logged-in " |
77 | - "user." |
78 | - ) |
79 | - secret = create_access_token_secret() |
80 | - getUtility(IAccessTokenSet).new( |
81 | - secret, |
82 | - owner=user, |
83 | - description=description, |
84 | - target=self, |
85 | - scopes=scopes, |
86 | - date_expires=date_expires, |
87 | - ) |
88 | - return secret |
89 | - |
90 | # XXX cjwatson 2021-10-13: Remove this once lp.code.xmlrpc.git accepts |
91 | # pushes using personal access tokens. |
92 | def _issueMacaroon(self, user): |
93 | @@ -1913,22 +1891,19 @@ class GitRepository( |
94 | .serialize() |
95 | ) |
96 | |
97 | + # XXX ines-almeida 2023-09-08: This method can be removed in favour of the |
98 | + # inherited one from AccessTokenTarget once we don't need `_issueMacaroon` |
99 | + # (see `_issueMacaroon` above) |
100 | def issueAccessToken( |
101 | - self, owner=None, description=None, scopes=None, date_expires=None |
102 | + self, description=None, scopes=None, date_expires=None |
103 | ): |
104 | """See `IGitRepository`.""" |
105 | - # It's more usual in model code to pass the user as an argument, |
106 | - # e.g. using @call_with(user=REQUEST_USER) in the webservice |
107 | - # interface. However, in this case that would allow anyone who |
108 | - # constructs a way to call this method not via the webservice to |
109 | - # issue a token for any user, which seems like a bad idea. |
110 | - user = getUtility(ILaunchBag).user |
111 | if description is not None and scopes is not None: |
112 | - return self._issuePersonalAccessToken( |
113 | - user, description, scopes, date_expires=date_expires |
114 | + return super().issueAccessToken( |
115 | + description, scopes, date_expires=date_expires |
116 | ) |
117 | else: |
118 | - return self._issueMacaroon(user) |
119 | + return self._issueMacaroon(getUtility(ILaunchBag).user) |
120 | |
121 | def canBeDeleted(self): |
122 | """See `IGitRepository`.""" |
123 | diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py |
124 | index 29e8d59..9ee06d9 100644 |
125 | --- a/lib/lp/code/model/tests/test_gitrepository.py |
126 | +++ b/lib/lp/code/model/tests/test_gitrepository.py |
127 | @@ -30,7 +30,6 @@ from testtools.matchers import ( |
128 | MatchesListwise, |
129 | MatchesSetwise, |
130 | MatchesStructure, |
131 | - StartsWith, |
132 | ) |
133 | from testtools.testcase import ExpectedException |
134 | from zope.component import getUtility |
135 | @@ -150,7 +149,6 @@ from lp.registry.interfaces.personociproject import IPersonOCIProjectFactory |
136 | from lp.registry.interfaces.personproduct import IPersonProductFactory |
137 | from lp.registry.tests.test_accesspolicy import get_policies_for_artifact |
138 | from lp.services.auth.enums import AccessTokenScope |
139 | -from lp.services.auth.interfaces import IAccessTokenSet |
140 | from lp.services.authserver.xmlrpc import AuthServerAPIView |
141 | from lp.services.config import config |
142 | from lp.services.database.constants import UTC_NOW |
143 | @@ -6672,156 +6670,6 @@ class TestGitRepositoryWebservice(TestCaseWithFactory): |
144 | ), |
145 | ) |
146 | |
147 | - def test_issueAccessToken(self): |
148 | - # A user can request an access token via the webservice API. |
149 | - self.pushConfig("codehosting", git_macaroon_secret_key="some-secret") |
150 | - repository = self.factory.makeGitRepository() |
151 | - # Write access to the repository isn't checked at this stage |
152 | - # (although the access token will only be useful if the user has |
153 | - # some kind of write access). |
154 | - requester = self.factory.makePerson() |
155 | - with person_logged_in(requester): |
156 | - repository_url = api_url(repository) |
157 | - webservice = webservice_for_person( |
158 | - requester, |
159 | - permission=OAuthPermission.WRITE_PUBLIC, |
160 | - default_api_version="devel", |
161 | - ) |
162 | - response = webservice.named_post(repository_url, "issueAccessToken") |
163 | - self.assertEqual(200, response.status) |
164 | - macaroon = Macaroon.deserialize(response.jsonBody()) |
165 | - with person_logged_in(ANONYMOUS): |
166 | - self.assertThat( |
167 | - macaroon, |
168 | - MatchesStructure( |
169 | - location=Equals(config.vhost.mainsite.hostname), |
170 | - identifier=Equals("git-repository"), |
171 | - caveats=MatchesListwise( |
172 | - [ |
173 | - MatchesStructure.byEquality( |
174 | - caveat_id="lp.git-repository %s" |
175 | - % repository.id |
176 | - ), |
177 | - MatchesStructure( |
178 | - caveat_id=StartsWith( |
179 | - "lp.principal.openid-identifier " |
180 | - ) |
181 | - ), |
182 | - MatchesStructure( |
183 | - caveat_id=StartsWith("lp.expires ") |
184 | - ), |
185 | - ] |
186 | - ), |
187 | - ), |
188 | - ) |
189 | - |
190 | - def test_issueAccessToken_anonymous(self): |
191 | - # An anonymous user cannot request an access token via the |
192 | - # webservice API. |
193 | - repository = self.factory.makeGitRepository() |
194 | - with person_logged_in(repository.owner): |
195 | - repository_url = api_url(repository) |
196 | - webservice = webservice_for_person(None, default_api_version="devel") |
197 | - response = webservice.named_post(repository_url, "issueAccessToken") |
198 | - self.assertEqual(401, response.status) |
199 | - self.assertEqual( |
200 | - b"git-repository macaroons may only be issued for a logged-in " |
201 | - b"user.", |
202 | - response.body, |
203 | - ) |
204 | - |
205 | - def test_issueAccessToken_personal(self): |
206 | - # A user can request a personal access token via the webservice API. |
207 | - repository = self.factory.makeGitRepository() |
208 | - # Write access to the repository isn't checked at this stage |
209 | - # (although the access token will only be useful if the user has |
210 | - # some kind of write access). |
211 | - requester = self.factory.makePerson() |
212 | - with person_logged_in(requester): |
213 | - repository_url = api_url(repository) |
214 | - webservice = webservice_for_person( |
215 | - requester, |
216 | - permission=OAuthPermission.WRITE_PUBLIC, |
217 | - default_api_version="devel", |
218 | - ) |
219 | - response = webservice.named_post( |
220 | - repository_url, |
221 | - "issueAccessToken", |
222 | - description="Test token", |
223 | - scopes=["repository:build_status"], |
224 | - ) |
225 | - self.assertEqual(200, response.status) |
226 | - secret = response.jsonBody() |
227 | - with person_logged_in(requester): |
228 | - token = getUtility(IAccessTokenSet).getBySecret(secret) |
229 | - self.assertThat( |
230 | - token, |
231 | - MatchesStructure( |
232 | - owner=Equals(requester), |
233 | - description=Equals("Test token"), |
234 | - target=Equals(repository), |
235 | - scopes=Equals([AccessTokenScope.REPOSITORY_BUILD_STATUS]), |
236 | - date_expires=Is(None), |
237 | - ), |
238 | - ) |
239 | - |
240 | - def test_issueAccessToken_personal_with_expiry(self): |
241 | - # A user can set an expiry time when requesting a personal access |
242 | - # token via the webservice API. |
243 | - repository = self.factory.makeGitRepository() |
244 | - # Write access to the repository isn't checked at this stage |
245 | - # (although the access token will only be useful if the user has |
246 | - # some kind of write access). |
247 | - requester = self.factory.makePerson() |
248 | - with person_logged_in(requester): |
249 | - repository_url = api_url(repository) |
250 | - webservice = webservice_for_person( |
251 | - requester, |
252 | - permission=OAuthPermission.WRITE_PUBLIC, |
253 | - default_api_version="devel", |
254 | - ) |
255 | - date_expires = datetime.now(timezone.utc) + timedelta(days=30) |
256 | - response = webservice.named_post( |
257 | - repository_url, |
258 | - "issueAccessToken", |
259 | - description="Test token", |
260 | - scopes=["repository:build_status"], |
261 | - date_expires=date_expires.isoformat(), |
262 | - ) |
263 | - self.assertEqual(200, response.status) |
264 | - secret = response.jsonBody() |
265 | - with person_logged_in(requester): |
266 | - token = getUtility(IAccessTokenSet).getBySecret(secret) |
267 | - self.assertThat( |
268 | - token, |
269 | - MatchesStructure.byEquality( |
270 | - owner=requester, |
271 | - description="Test token", |
272 | - target=repository, |
273 | - scopes=[AccessTokenScope.REPOSITORY_BUILD_STATUS], |
274 | - date_expires=date_expires, |
275 | - ), |
276 | - ) |
277 | - |
278 | - def test_issueAccessToken_personal_anonymous(self): |
279 | - # An anonymous user cannot request a personal access token via the |
280 | - # webservice API. |
281 | - repository = self.factory.makeGitRepository() |
282 | - with person_logged_in(repository.owner): |
283 | - repository_url = api_url(repository) |
284 | - webservice = webservice_for_person(None, default_api_version="devel") |
285 | - response = webservice.named_post( |
286 | - repository_url, |
287 | - "issueAccessToken", |
288 | - description="Test token", |
289 | - scopes=["repository:build_status"], |
290 | - ) |
291 | - self.assertEqual(401, response.status) |
292 | - self.assertEqual( |
293 | - b"Personal access tokens may only be issued for a logged-in user.", |
294 | - response.body, |
295 | - ) |
296 | - |
297 | def test_builder_constraints_commercial_admin(self): |
298 | # A commercial admin can change a repository's builder constraints. |
299 | self.factory.makeBuilder(open_resources=["gpu", "large"]) |
300 | diff --git a/lib/lp/services/auth/interfaces.py b/lib/lp/services/auth/interfaces.py |
301 | index be90837..6081830 100644 |
302 | --- a/lib/lp/services/auth/interfaces.py |
303 | +++ b/lib/lp/services/auth/interfaces.py |
304 | @@ -7,6 +7,7 @@ __all__ = [ |
305 | "IAccessToken", |
306 | "IAccessTokenSet", |
307 | "IAccessTokenTarget", |
308 | + "IAccessTokenTargetEdit", |
309 | "IAccessTokenVerifiedRequest", |
310 | ] |
311 | |
312 | @@ -18,6 +19,7 @@ from lazr.restful.declarations import ( |
313 | exported, |
314 | exported_as_webservice_entry, |
315 | operation_for_version, |
316 | + operation_parameters, |
317 | operation_returns_collection_of, |
318 | ) |
319 | from lazr.restful.fields import Reference |
320 | @@ -207,6 +209,49 @@ class IAccessTokenVerifiedRequest(Interface): |
321 | class IAccessTokenTarget(Interface): |
322 | """An object that can be a target for access tokens.""" |
323 | |
324 | + # XXX ines-almeida 2023-09-08: We keep this class separated from |
325 | + # `IAccessTokenTargetEdit` because we need them to have different |
326 | + # permission settings. Once the `_issueMacaroon` logic is no longer needed, |
327 | + # we might want to reconsider requiring `launchpad.Edit` permissions for |
328 | + # the below endpoints. |
329 | + |
330 | + @operation_parameters( |
331 | + description=TextLine( |
332 | + title=_("A short description of the token."), required=True |
333 | + ), |
334 | + scopes=List( |
335 | + title=_("A list of scopes to be granted by this token."), |
336 | + value_type=Choice(vocabulary=AccessTokenScope), |
337 | + required=True, |
338 | + ), |
339 | + date_expires=Datetime( |
340 | + title=_("When the token should expire."), required=False |
341 | + ), |
342 | + ) |
343 | + @export_write_operation() |
344 | + @operation_for_version("devel") |
345 | + def issueAccessToken(description, scopes, date_expires=None): |
346 | + """Issue a personal access token for this target. |
347 | + |
348 | + Access tokens can be used to push to repositories over HTTPS. These may |
349 | + be used in webservice API requests for certain methods in the target's |
350 | + repositories. |
351 | + |
352 | + They are either non-expiring or with an expiry time given by |
353 | + `date_expires`. |
354 | + |
355 | + :return: The secret for a new personal access token (Launchpad only |
356 | + records the hash of this secret and not the secret itself, so the |
357 | + caller must be careful to save this). |
358 | + """ |
359 | + |
360 | + |
361 | +@exported_as_webservice_entry(as_of="beta") |
362 | +class IAccessTokenTargetEdit(Interface): |
363 | + """An object that can be a target for access tokens that requires |
364 | + launchpad.Edit permission. |
365 | + """ |
366 | + |
367 | @call_with(visible_by_user=REQUEST_USER) |
368 | @operation_returns_collection_of(IAccessToken) |
369 | @export_read_operation() |
370 | diff --git a/lib/lp/services/auth/model.py b/lib/lp/services/auth/model.py |
371 | index 436c382..f595637 100644 |
372 | --- a/lib/lp/services/auth/model.py |
373 | +++ b/lib/lp/services/auth/model.py |
374 | @@ -16,6 +16,7 @@ from storm.expr import SQL, And, Cast, Or, Select, Update |
375 | from storm.locals import DateTime, Int, Reference, Unicode |
376 | from zope.component import getUtility |
377 | from zope.interface import implementer |
378 | +from zope.security.interfaces import Unauthorized |
379 | from zope.security.proxy import removeSecurityProxy |
380 | |
381 | from lp.code.interfaces.gitcollection import IAllGitRepositories |
382 | @@ -23,9 +24,11 @@ from lp.code.interfaces.gitrepository import IGitRepository |
383 | from lp.registry.model.teammembership import TeamParticipation |
384 | from lp.services.auth.enums import AccessTokenScope |
385 | from lp.services.auth.interfaces import IAccessToken, IAccessTokenSet |
386 | +from lp.services.auth.utils import create_access_token_secret |
387 | from lp.services.database.constants import UTC_NOW |
388 | from lp.services.database.interfaces import IPrimaryStore, IStore |
389 | from lp.services.database.stormbase import StormBase |
390 | +from lp.services.webapp.interfaces import ILaunchBag |
391 | |
392 | |
393 | @implementer(IAccessToken) |
394 | @@ -236,3 +239,26 @@ class AccessTokenTargetMixin: |
395 | visible_by_user=visible_by_user, |
396 | include_expired=include_expired, |
397 | ) |
398 | + |
399 | + def issueAccessToken(self, description, scopes, date_expires=None): |
400 | + # It's more usual in model code to pass the user as an argument, |
401 | + # e.g. using @call_with(user=REQUEST_USER) in the webservice |
402 | + # interface. However, in this case that would allow anyone who |
403 | + # constructs a way to call this method not via the webservice to |
404 | + # issue a token for any user, which seems like a bad idea. |
405 | + user = getUtility(ILaunchBag).user |
406 | + if user is None: |
407 | + raise Unauthorized( |
408 | + "Personal access tokens may only be issued for a logged-in " |
409 | + "user." |
410 | + ) |
411 | + secret = create_access_token_secret() |
412 | + getUtility(IAccessTokenSet).new( |
413 | + secret, |
414 | + owner=user, |
415 | + description=description, |
416 | + target=self, |
417 | + scopes=scopes, |
418 | + date_expires=date_expires, |
419 | + ) |
420 | + return secret |
421 | diff --git a/lib/lp/services/auth/tests/test_model.py b/lib/lp/services/auth/tests/test_model.py |
422 | index 9449f0c..b33ccf7 100644 |
423 | --- a/lib/lp/services/auth/tests/test_model.py |
424 | +++ b/lib/lp/services/auth/tests/test_model.py |
425 | @@ -9,14 +9,23 @@ import signal |
426 | from datetime import datetime, timedelta, timezone |
427 | |
428 | import transaction |
429 | +from pymacaroons import Macaroon |
430 | from storm.store import Store |
431 | -from testtools.matchers import Is, MatchesStructure |
432 | +from testtools.matchers import ( |
433 | + Equals, |
434 | + Is, |
435 | + MatchesListwise, |
436 | + MatchesStructure, |
437 | + StartsWith, |
438 | +) |
439 | from zope.component import getUtility |
440 | from zope.security.proxy import removeSecurityProxy |
441 | |
442 | +from lp.code.interfaces.gitrepository import IGitRepository |
443 | from lp.services.auth.enums import AccessTokenScope |
444 | from lp.services.auth.interfaces import IAccessTokenSet |
445 | from lp.services.auth.utils import create_access_token_secret |
446 | +from lp.services.config import config |
447 | from lp.services.database.sqlbase import ( |
448 | disconnect_stores, |
449 | get_transaction_timestamp, |
450 | @@ -24,6 +33,7 @@ from lp.services.database.sqlbase import ( |
451 | from lp.services.webapp.authorization import check_permission |
452 | from lp.services.webapp.interfaces import OAuthPermission |
453 | from lp.testing import ( |
454 | + ANONYMOUS, |
455 | TestCaseWithFactory, |
456 | api_url, |
457 | login, |
458 | @@ -405,6 +415,10 @@ class TestAccessTokenTargetBase: |
459 | self.owner, permission=OAuthPermission.WRITE_PRIVATE |
460 | ) |
461 | |
462 | + def _makePerson(self): |
463 | + with person_logged_in(ANONYMOUS): |
464 | + return self.factory.makePerson() |
465 | + |
466 | def test_getAccessTokens(self): |
467 | with person_logged_in(self.owner): |
468 | for description in ("Test token 1", "Test token 2"): |
469 | @@ -468,9 +482,208 @@ class TestAccessTokenTargetBase: |
470 | recorder1, recorder2 = record_two_runs(get_tokens, create_token, 2) |
471 | self.assertThat(recorder2, HasQueryCount.byEquality(recorder1)) |
472 | |
473 | + def test_issueAccessToken(self): |
474 | + # A user can request a personal access token via the webservice API. |
475 | + # Write access to the repositories isn't checked at this stage |
476 | + # (although the access token will only be useful if the user has |
477 | + # some kind of write access). |
478 | + requester = self._makePerson() |
479 | + with person_logged_in(requester): |
480 | + target_url = api_url(self.target) |
481 | + webservice = webservice_for_person( |
482 | + requester, |
483 | + permission=OAuthPermission.WRITE_PUBLIC, |
484 | + default_api_version="devel", |
485 | + ) |
486 | + response = webservice.named_post( |
487 | + target_url, |
488 | + "issueAccessToken", |
489 | + description="Test token", |
490 | + scopes=["repository:build_status"], |
491 | + ) |
492 | + self.assertEqual(200, response.status) |
493 | + secret = response.jsonBody() |
494 | + with person_logged_in(requester): |
495 | + token = getUtility(IAccessTokenSet).getBySecret(secret) |
496 | + self.assertThat( |
497 | + token, |
498 | + MatchesStructure( |
499 | + owner=Equals(requester), |
500 | + description=Equals("Test token"), |
501 | + target=Equals(self.target), |
502 | + scopes=Equals([AccessTokenScope.REPOSITORY_BUILD_STATUS]), |
503 | + date_expires=Is(None), |
504 | + ), |
505 | + ) |
506 | + |
507 | + def test_issueAccessToken_with_expiry(self): |
508 | + # A user can set an expiry time when requesting a personal access |
509 | + # token via the webservice API. |
510 | + # Write access to the repositories isn't checked at this stage |
511 | + # (although the access token will only be useful if the user has |
512 | + # some kind of write access). |
513 | + requester = self._makePerson() |
514 | + with person_logged_in(requester): |
515 | + target_url = api_url(self.target) |
516 | + webservice = webservice_for_person( |
517 | + requester, |
518 | + permission=OAuthPermission.WRITE_PUBLIC, |
519 | + default_api_version="devel", |
520 | + ) |
521 | + date_expires = datetime.now(timezone.utc) + timedelta(days=30) |
522 | + response = webservice.named_post( |
523 | + target_url, |
524 | + "issueAccessToken", |
525 | + description="Test token", |
526 | + scopes=["repository:build_status"], |
527 | + date_expires=date_expires.isoformat(), |
528 | + ) |
529 | + self.assertEqual(200, response.status) |
530 | + secret = response.jsonBody() |
531 | + with person_logged_in(requester): |
532 | + token = getUtility(IAccessTokenSet).getBySecret(secret) |
533 | + self.assertThat( |
534 | + token, |
535 | + MatchesStructure.byEquality( |
536 | + owner=requester, |
537 | + description="Test token", |
538 | + target=self.target, |
539 | + scopes=[AccessTokenScope.REPOSITORY_BUILD_STATUS], |
540 | + date_expires=date_expires, |
541 | + ), |
542 | + ) |
543 | + |
544 | + def test_issueAccessToken_anonymous(self): |
545 | + # An anonymous user cannot request a personal access token via the |
546 | + # webservice API. |
547 | + with person_logged_in(self.owner): |
548 | + target_url = api_url(self.target) |
549 | + webservice = webservice_for_person(None, default_api_version="devel") |
550 | + response = webservice.named_post( |
551 | + target_url, |
552 | + "issueAccessToken", |
553 | + description="Test token", |
554 | + scopes=["repository:build_status"], |
555 | + ) |
556 | + self.assertEqual(401, response.status) |
557 | + self.assertEqual( |
558 | + b"Personal access tokens may only be issued for a logged-in user.", |
559 | + response.body, |
560 | + ) |
561 | + |
562 | + def test_issueAccessToken_no_scope(self): |
563 | + # Scopes are required to request a personal access token |
564 | + |
565 | + if IGitRepository.providedBy(self.target): |
566 | + self.skipTest( |
567 | + "Currently, Git repositories allow requests without scopes." |
568 | + ) |
569 | + |
570 | + with person_logged_in(self.owner): |
571 | + target_url = api_url(self.target) |
572 | + webservice = webservice_for_person( |
573 | + self.owner, |
574 | + permission=OAuthPermission.WRITE_PUBLIC, |
575 | + default_api_version="devel", |
576 | + ) |
577 | + response = webservice.named_post( |
578 | + target_url, |
579 | + "issueAccessToken", |
580 | + description="Test token", |
581 | + ) |
582 | + self.assertEqual(400, response.status) |
583 | + self.assertEqual( |
584 | + b"scopes: Required input is missing.", |
585 | + response.body, |
586 | + ) |
587 | + |
588 | + def test_issueAccessToken_no_description(self): |
589 | + # A description is required to request a personal access token |
590 | + |
591 | + if IGitRepository.providedBy(self.target): |
592 | + self.skipTest( |
593 | + "Currently, Git repositories allow requests without a " |
594 | + "description." |
595 | + ) |
596 | + |
597 | + with person_logged_in(self.owner): |
598 | + target_url = api_url(self.target) |
599 | + webservice = webservice_for_person( |
600 | + self.owner, |
601 | + permission=OAuthPermission.WRITE_PUBLIC, |
602 | + default_api_version="devel", |
603 | + ) |
604 | + response = webservice.named_post( |
605 | + target_url, |
606 | + "issueAccessToken", |
607 | + scopes=["repository:build_status"], |
608 | + ) |
609 | + self.assertEqual(400, response.status) |
610 | + self.assertEqual( |
611 | + b"description: Required input is missing.", |
612 | + response.body, |
613 | + ) |
614 | + |
615 | |
616 | class TestAccessTokenTargetGitRepository( |
617 | TestAccessTokenTargetBase, TestCaseWithFactory |
618 | ): |
619 | def makeTarget(self): |
620 | return self.factory.makeGitRepository() |
621 | + |
622 | + def test_issueAccessTokenMacaroon(self): |
623 | + # A user can request a git repository access token via the webservice |
624 | + # API without scopes and description (a Macaroon is returned) |
625 | + self.pushConfig("codehosting", git_macaroon_secret_key="some-secret") |
626 | + # Write access to the repositories isn't checked at this stage |
627 | + # (although the access token will only be useful if the user has |
628 | + # some kind of write access). |
629 | + requester = self._makePerson() |
630 | + with person_logged_in(requester): |
631 | + target_url = api_url(self.target) |
632 | + webservice = webservice_for_person( |
633 | + requester, |
634 | + permission=OAuthPermission.WRITE_PUBLIC, |
635 | + default_api_version="devel", |
636 | + ) |
637 | + response = webservice.named_post(target_url, "issueAccessToken") |
638 | + self.assertEqual(200, response.status) |
639 | + macaroon = Macaroon.deserialize(response.jsonBody()) |
640 | + with person_logged_in(ANONYMOUS): |
641 | + self.assertThat( |
642 | + macaroon, |
643 | + MatchesStructure( |
644 | + location=Equals(config.vhost.mainsite.hostname), |
645 | + identifier=Equals("git-repository"), |
646 | + caveats=MatchesListwise( |
647 | + [ |
648 | + MatchesStructure.byEquality( |
649 | + caveat_id="lp.git-repository %s" |
650 | + % self.target.id |
651 | + ), |
652 | + MatchesStructure( |
653 | + caveat_id=StartsWith( |
654 | + "lp.principal.openid-identifier " |
655 | + ) |
656 | + ), |
657 | + MatchesStructure( |
658 | + caveat_id=StartsWith("lp.expires ") |
659 | + ), |
660 | + ] |
661 | + ), |
662 | + ), |
663 | + ) |
664 | + |
665 | + def test_issueAccessTokenMacaroon_anonymous(self): |
666 | + # An anonymous user cannot request a git repository access token |
667 | + # without scopes and description (Macaroon) via the webservice |
668 | + with person_logged_in(self.owner): |
669 | + target_url = api_url(self.target) |
670 | + webservice = webservice_for_person(None, default_api_version="devel") |
671 | + response = webservice.named_post(target_url, "issueAccessToken") |
672 | + self.assertEqual(401, response.status) |
673 | + self.assertEqual( |
674 | + b"git-repository macaroons may only be issued for a logged-in " |
675 | + b"user.", |
676 | + response.body, |
677 | + ) |
As my XXX comment noted, the reason I hadn't done this yet was because of the backward- compatibility macaroon-issuing code, and I still think that shouldn't be added to other targets. I can see why you want to do something like this though.
Would it be possible to move just the personal access token code to a common place, and leave `_issueMacaroon` in `GitRepository`? The common interface definition should have `description` and `scopes` set to `required=True`, and we can keep the existing declaration in `IGitRepository` for now that overrides them to `required=False` and has some extra docstring text describing the old macaroon workflow.