Initial implementation of uploading with trusted publishing authentication #1194
Conversation
pyproject.toml
Outdated
| @@ -62,6 +62,7 @@ register = "twine.commands.register:main" | |||
|
|
|||
| [project.optional-dependencies] | |||
| keyring = ["keyring >= 15.1"] | |||
| oidc = ["id"] | |||
There was a problem hiding this comment.
| oidc = ["id"] | |
| trusted-publishing = ["id"] |
twine/auth.py
Outdated
|
|
||
| token_exchange_url = f"https://{repository_domain}/_/oidc/mint-token" | ||
|
|
||
| mint_token_resp = requests.post( |
There was a problem hiding this comment.
Using a session would be ideal. Better yet if we can share it with a Repository to get connection pooling and reuse for performance
There was a problem hiding this comment.
This is happening before we construct Repository. I could make it so a session is made beforehand and passed in, but the Repository constructor modifies the session object, so that's a bit messy.
Do you think that's worth dealing with, or are you happy to have two sessions? Putting the two requests here in a session should be easy enough.
There was a problem hiding this comment.
Yeah, I know the repository manages the session today. We could move that and the customization somewhere else. The repository manages it today because it was the only place to need a session. I'm not certain we need to share them, but it might be worth the effort.
|
I've started reworking it without the explicit flag, as discussed, but I'm aware I still need to do the fallback to prompting for a token. |
|
I've made the fallback relatively narrow - only if we're not on a supported platform for trusted publishing - but demoted trusted publishing so a token in keyring or in |
@takluyver @woodruffw do you know if this would impact |
I think it would indeed affect I have no strong opinions on whether it makes sense to block that on #194 or not 🙂 -- I figure |
|
One other note, please be sure to update our --version handling to include id and it's version string. Should be as easy as adding a string to a list |
|
So far I've not actually tested this at all. I'll do some manual testing before it's merged by setting up a project to upload to TestPyPI. How much of this is practical to test in twine's CI? Obviously I could mock out all the responses, but I'm never sure if a test like that is really worth much. |
|
Manual testing:
In between, I hit a separate error uploading (bad filetype), which #1198 should fix. |
|
One drawback I noticed of putting this below keyring in the priority order is that you get quite a long warning (including a traceback) to say that there's no keyring set up before it uses trusted publishing. I'm roughly imagining that someone has a CI set up where keyring retrieves a token for PyPI (or another index) from some sort of secret manager. So I want that to still have precedence over trusted publishing, as another way to manually provide a token to twine. But maybe NoKeyringError, when there's no backend set up, should produce a smaller, less error-ish message? |
There was a problem hiding this comment.
LGTM. Would like @woodruffw or @di to give it a once over if they have time too as they know the PyPI/action side better than me
I'll do a full review tomorrow! |
|
Definitely can review on Monday! |
|
Thanks. 🙂 I've done as I mentioned yesterday and made a much smaller message on NoKeyringError. I realised there's already a brief info-level message if keyring can't be imported, and I think the case where it's installed but has no backend is quite similar to that. |
|
Thanks @takluyver, this approach looks great to me! I'm going to try and do a TestPyPI deployment with this branch in a moment, to confirm that it works as well 🙂 |
|
Looks like the token exchange worked, although TestPyPI itself errored with a 400 for the actual upload (leaving the release created but empty): https://github.com/woodruffw-experiments/test-twine-tp/actions/runs/12242554831/job/34150133019 From the logs on TestPyPI: 
|
|
I think that's the issue same issue I ran into & fixed in #1198. There's a branch in my fork called |
🤦 I totally neglected to check whether this tip was behind |
|
I think that's three approvals and a successful test run, so I'm meeting this |
|
Thanks all! |
Fixes #999
This is quite rough - I suspect it needs some better error messages at least. But I figured I'd open it straight away to get comments on the overall structure. Does it make sense to have this logic in the
auth.Resolverclass, or should it be somewhere else?