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
Proposed by
Ines Almeida
| 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:
|
|||
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
To post a comment you must log in.
Revision history for this message
| 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).
Revision history for this message
| Colin Watson (cjwatson) wrote : | # |
I like this much better, thanks! Just a few tweaks.
review:
Approve
- 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
Revision history for this message
| 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?
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 | 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-
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.