-
Notifications
You must be signed in to change notification settings - Fork 320
Automatically replace Trusted Publishing Tokens #1249
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds automatic renewal for short-lived PyPI tokens during long-running Trusted Publishing uploads to avoid token expiration mid-operation.
- Introduces
TOKEN_USERNAME
andTOKEN_RENEWAL_THRESHOLD
constants. - Implements caching, validation, and renewal logic in
Resolver
(_has_valid_cached_tp_token
,_make_trusted_publishing_token
,make_trusted_publishing_token
). - Adds
TrustedPublishingAuthenticator
to wrap HTTP Basic Auth with token renewal. - Updates the changelog with feature details.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
twine/auth.py | Added token renewal logic, caching, constants, and new authenticator class |
changelog/1246.feature.rst | Documented automatic token refresh behavior |
Comments suppressed due to low confidence (3)
twine/auth.py:34
- The comment says renewal starts after 10 minutes, but
TOKEN_RENEWAL_THRESHOLD
is set to 5 minutes—update the comment or the constant to keep them in sync.
#: Tokens expire after 15 minutes, let's start allowing renewal/replacement
twine/auth.py:148
- The new token renewal and caching logic lacks explicit unit tests. Consider adding tests for
_has_valid_cached_tp_token
,_make_trusted_publishing_token
, and fallback behaviors.
def _has_valid_cached_tp_token(self) -> bool:
changelog/1246.feature.rst:6
- Typo: "we weill" should be "we will".
and *and* we weill
9a32402
to
f4cd53d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach makes a lot of sense to me! I'll go ahead and put up a prospective PR for pypi/warehouse#18235 to get this moving on PyPI's side too 🙂
Edit: pypi/warehouse#18238 for the above.
454c708
to
3b9229a
Compare
Tokens obtained via the Trusted Publishing flow are only valid for a short period of time. As such, projects using Trusted Publishing and trying to upload a large number of artifacts (or potentially a large number of large artifacts) could run out of time before the token expires. This is a first pass at solving the problem by replacing the token when it's within 5 minutes of expiry. This also adds optional parsing from the Warehouse API which does not currently return the expiration timestamp. If that's unavailable, we guesstimate the remaining validity. Closes pypagh-1246
On Python 3.9 ``importlib_metadata`` returns Optional[PackageMetadata] instead of PackageMetadata, this leads to mypy complaining that Optional[...] is not indexable and does not have ``get_all``. Instead, let's just cast it for ourselves.
3b9229a
to
5ff3239
Compare
Curious - I'm not an expert on the logic here, but does this now allow for a runaway/lengthy process to basically continue swapping out new tokens indefinitely? |
In principle yes, up to the job timeout window (which IIRC is 4 or 6 hours). I think this was already true for someone who wanted to grief PyPI though, since they could do the same thing in a busy loop on a CI runner. (And this would be less noisy than that, since the rotation is every ~10 minutes versus as often as they would want with a custom script.) |
Closes gh-1246