Skip to content
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

Add support for Google trusted publishing #15144

Merged
merged 17 commits into from
Jan 10, 2024
Merged

Add support for Google trusted publishing #15144

merged 17 commits into from
Jan 10, 2024

Conversation

di
Copy link
Member

@di di commented Jan 5, 2024

Draft while we get #15143 and #14063 #15148 merged.

On top of #15143, 09812df is the relevant commit.

Still needs:

  • Tests for the PendingPublisher
  • Tests for the Publisher

Towards #13551.

The flag is disabled for now while we get docs added.

@di
Copy link
Member Author

di commented Jan 6, 2024

FYI @woodruffw, in case you need to take over this on Monday: I generalized the PendingPublisher tests in a656cf8 and added parameters for the Google PendingPublisher in 0c49d87.

The same needs to be done for the tests that would exercise the view here, which currently has no coverage:

Name                                 Stmts   Miss Branch BrPart  Cover   Missing
--------------------------------------------------------------------------------
warehouse/manage/views/__init__.py     797     29    336      0    97%   1312-1413
--------------------------------------------------------------------------------
TOTAL                                13543     29   4487      0    99%

We should also give some thought to how the docs will support multiple publishers, right now they assume GitHub. Maybe mkdocs has some sort of tabbed thing that would let us switch all the examples in the docs between different publishers? Haven't looked into it yet...

@di
Copy link
Member Author

di commented Jan 8, 2024

Looks like https://squidfunk.github.io/mkdocs-material/reference/content-tabs/#linked-content-tabs-feature-enabled might be the answer for the doc updates needed here.

@woodruffw
Copy link
Member

Looks like https://squidfunk.github.io/mkdocs-material/reference/content-tabs/#linked-content-tabs-feature-enabled might be the answer for the doc updates needed here.

I'll do a separate PR for this today!

@di di marked this pull request as ready for review January 9, 2024 23:28
@di di requested a review from a team as a code owner January 9, 2024 23:28
@di
Copy link
Member Author

di commented Jan 9, 2024

OK, I think this is good to go, we can queue up docs for this in #15173.

warehouse/templates/manage/account/publishing.html Outdated Show resolved Hide resolved
Comment on lines +1311 to +1317
def add_google_oidc_publisher(self):
if self.request.flags.disallow_oidc(AdminFlagValue.DISALLOW_GOOGLE_OIDC):
self.request.session.flash(
self.request._(
"Google-based trusted publishing is temporarily disabled. "
"See https://pypi.org/help#admin-intervention for details."
),
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity: If the DISALLOW_{GOOGLE,GITHUB}_OIDC flag already stops the publishing of packages through OIDC publishers (src), why do we also forbid adding a publisher configuration?
Is it just a matter of minimizing user confusion? (If OIDC is disabled, they might be confused if they successfully set up a publisher which then doesn't work)
Or can adding publishers also be problematic during high spam periods?

Copy link
Member Author

Choose a reason for hiding this comment

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

Partly to minimize user confusion, but also my plan is to use this as a feature flag until we're ready to enable the publisher.

Copy link
Member

Choose a reason for hiding this comment

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

Is it just a matter of minimizing user confusion? (If OIDC is disabled, they might be confused if they successfully set up a publisher which then doesn't work)
Or can adding publishers also be problematic during high spam periods?

At least for the GitHub provider, a little bit of both: allowing registration at a time when actually using the publisher is impossible is likely (IMO) to cause user confusion, and also registering the publisher (in GitHub's case) involves making some GitHub API calls that we probably want to disable during periods of spam. The latter is not true for the Google publisher, though 🙂

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!

Copy link
Member

@miketheman miketheman left a comment

Choose a reason for hiding this comment

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

I didn't go through with a fine-tooth comb, but a cursory skim looks generally okay.

I had one validator-based question inline, feel free to take it or leave it - it can always be reevaluated later.

warehouse/oidc/forms/google.py Outdated Show resolved Hide resolved
@di di merged commit 4023254 into pypi:main Jan 10, 2024
17 checks passed
@di di deleted the fix/13551 branch January 10, 2024 17:20
@di di mentioned this pull request Jan 10, 2024
@di di mentioned this pull request Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants