Skip to content

OIDC ActiveState token handler #15204

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.

Sign up for GitHub

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

Jump to bottom
Merged
di merged 6 commits into from
Feb 12, 2024
Merged

OIDC ActiveState token handler #15204

di merged 6 commits into from
Feb 12, 2024

Conversation

@th3coop
Copy link
Contributor

@th3coop th3coop commented Jan 12, 2024 •
edited
Loading

This is the last PR from ActiveState adding ActiveState as an OIDC Publisher. Does not enable ActiveState as a publisher yet.

This PR follows from #15168.

More realistic Diff can be found below:
ActiveState#1
image

@th3coop th3coop force-pushed the OIDC-ActiveState-token-handler branch 2 times, most recently from eebaa7a to 0bae2c6 Compare January 13, 2024 00:16
@th3coop th3coop marked this pull request as ready for review January 13, 2024 00:50
@th3coop th3coop requested a review from a team as a code owner January 13, 2024 00:50
@th3coop th3coop mentioned this pull request Jan 16, 2024
Merged
woodruffw
woodruffw reviewed Jan 17, 2024
View reviewed changes
warehouse/templates/manage/account/publishing.html
Copy link
Member

@woodruffw woodruffw Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB: This LGTM (and I confirmed that it works), but you'll need a corresponding "non-pending" form in manage/project/publishing.html as well 🙂

Copy link
Contributor Author

@th3coop th3coop Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how I missed that :( I'll get that added right away.

Copy link
Member

@woodruffw woodruffw Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an annoying bit of duplication, so missing it is understandable! Thanks!

Copy link
Contributor Author

@th3coop th3coop Jan 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Easy peasy.
image

Good catch @woodruffw

Copy link
Contributor Author

@th3coop th3coop Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I didn't notice which PR we were commenting in. I've added the missing form in #15168

I'll need to rebase this branch off of #15168 for those changes to show up in this branch/PR. I plan to do that first thing tomorrow.

Copy link
Member

@woodruffw woodruffw Jan 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, sorry for the confusion there. I'll keep the review on #15168 going forwards (maybe we can mark this as draft, to signal that it'll follow that PR and isn't ready for review yet?)

Copy link
Contributor Author

@th3coop th3coop Jan 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can mark this as draft,

Can do and done.

@woodruffw
Copy link
Member

woodruffw commented Jan 17, 2024

I stood this up locally, and confirmed that I was able to register a pending ActiveState publisher (after toggling the admin flag):

Screenshot 2024-01-17 at 4 08 04 PM

However, it looks like the "normal" publisher tab is missing for ActiveState:

Screenshot 2024-01-17 at 4 07 23 PM

I believe #15204 (comment) is the proximate cause.

@th3coop th3coop marked this pull request as draft January 18, 2024 01:38
@th3coop th3coop changed the title OIDC active state token handler OIDC ActiveState token handler Feb 9, 2024
@th3coop th3coop force-pushed the OIDC-ActiveState-token-handler branch from 753c7e0 to c63726a Compare February 9, 2024 18:42
@th3coop th3coop marked this pull request as ready for review February 9, 2024 18:49
@th3coop th3coop requested a review from woodruffw February 9, 2024 22:08
woodruffw
woodruffw reviewed Feb 9, 2024
View reviewed changes
woodruffw
woodruffw reviewed Feb 9, 2024
View reviewed changes
woodruffw
woodruffw reviewed Feb 9, 2024
View reviewed changes
th3coop and others added 2 commits February 9, 2024 14:26
@th3coop @woodruffw
Remove print statement
15714ec 
Co-authored-by: William Woodruff <[email protected]>
@th3coop
Remove redundant token handler route. All token mint requests, for al…
6cd0ecc 
…l publishers, goes through 1 endpoint
@th3coop th3coop force-pushed the OIDC-ActiveState-token-handler branch from 9739cf8 to 6cd0ecc Compare February 9, 2024 22:27
woodruffw
woodruffw approved these changes Feb 12, 2024
View reviewed changes
Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @th3coop!

@woodruffw woodruffw requested a review from miketheman February 12, 2024 17:36
@woodruffw
Copy link
Member

woodruffw commented Feb 12, 2024

(Needs rebase/merge sync, FYI.)

@di
Copy link
Member

di commented Feb 12, 2024

Did something get lost in a rebase? I'm attempting to review here but I'm not seeing any substantial changes.

@woodruffw
Copy link
Member

woodruffw commented Feb 12, 2024

Did something get lost in a rebase? I'm attempting to review here but I'm not seeing any substantial changes.

Most of it got merged here: #15168

I believe this PR is meant to just be the "cap off" PR -- the core form, etc. logic is all merged 🙂

@th3coop
Copy link
Contributor Author

th3coop commented Feb 12, 2024

@di , you might have been confused by the comment from @woodruffw that looks like it's reviewing the ActiveState configuration form. That was my fault as I presented both PRs as ready to go at the time. Woodruffw is correct that this is the cap-off PR that finalizes the remaining ActiveState integration work. This PR adds the the token handling which piggy-backs on the previous PRs.

di
di reviewed Feb 12, 2024
View reviewed changes
@di
Copy link
Member

di commented Feb 12, 2024

This PR adds the the token handling which piggy-backs on the previous PRs.

Sorry, I don't see anything that's adding token handling? Was this already done in a previous PR as well?

@th3coop
Copy link
Contributor Author

th3coop commented Feb 12, 2024

Sorry, I don't see anything that's adding token handling? Was this already done in a previous PR as well?

Ahhh I see what's going on. I even confused myself.

@di, when Woodruff asked for this change most of what this PR added for implementation went away. Now, this PR is adding is some test refactors and additional testing of the ActiveState token endpoint (which technically isn't a thing, as all tokens now go through 1 endpoint).

@di
Copy link
Member

di commented Feb 12, 2024

Got it, makes sense now. This looks good aside from my one outstanding comment.

di
di approved these changes Feb 12, 2024
View reviewed changes
Copy link
Member

@di di left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! @th3coop If you can get this up-to-date we can merge it.

@th3coop
Copy link
Contributor Author

th3coop commented Feb 12, 2024

@di updated. Also, welcome back!

@di di merged commit 6e276c8 into pypi:main Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@di di di approved these changes

@miketheman miketheman Awaiting requested review from miketheman

+1 more reviewer

@woodruffw woodruffw woodruffw approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

trusted-publishing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@th3coop @woodruffw @di