OIDC active state form by th3coop · Pull Request #15168 · pypi/warehouse

th3coop · 2024-01-08T22:58:03Z

This PR follows from #13980. It adds form management portion of the ActiveState Publisher in PyPI Account settings.

Without #13980 merged, this PR have about 1400 more changed lines than it should. It'll look more like this when 13980 is merged: ActiveState#3

di · 2024-01-10T18:47:37Z

I'll review this once #13980 is merged and conflicts are resolved here!

th3coop · 2024-01-12T20:22:16Z

@di, @woodruffw. Before you give this PR a super thorough review, I'll point out that I've included quite a bit of duplication tests/unit/accounts/test_views.py. I think I would be able to avoid this if I changed all of the validation tests in that file to call _add_pending_oidc_publisher and perhaps parameterize passing in the respective services values so I can check stuff like this has the right service name in the string?

That could result in quite large or complex parameterize lists though.

Any thoughts?

di · 2024-01-12T20:28:00Z

@th3coop We already have parametrized tests for these which were added recently, e.g. for that specific example:

warehouse/tests/unit/accounts/test_views.py

Lines 3442 to 3526 in e62049b

    
               @pytest.mark.parametrize( 
        
                   "view_name, flag, publisher_name", 
        
                   [ 
        
                       ( 
        
                           "add_pending_github_oidc_publisher", 
        
                           AdminFlagValue.DISALLOW_GITHUB_OIDC, 
        
                           "GitHub", 
        
                       ), 
        
                       ( 
        
                           "add_pending_google_oidc_publisher", 
        
                           AdminFlagValue.DISALLOW_GOOGLE_OIDC, 
        
                           "Google", 
        
                       ), 
        
                   ], 
        
               ) 
        
               def test_add_pending_oidc_publisher_admin_disabled( 
        
                   self, monkeypatch, pyramid_request, view_name, flag, publisher_name 
        
               ): 
        
                   pyramid_request.user = pretend.stub() 
        
                   pyramid_request.registry = pretend.stub( 
        
                       settings={ 
        
                           "github.token": "fake-api-token", 
        
                       } 
        
                   ) 
        
                   pyramid_request.flags = pretend.stub( 
        
                       disallow_oidc=pretend.call_recorder(lambda f=None: True), 
        
                   ) 
        
                   pyramid_request.session = pretend.stub( 
        
                       flash=pretend.call_recorder(lambda *a, **kw: None) 
        
                   ) 
        
                   project_factory = pretend.stub() 
        
                   project_factory_cls = pretend.call_recorder(lambda r: project_factory) 
        
                   monkeypatch.setattr(views, "ProjectFactory", project_factory_cls) 
        
                   pending_github_publisher_form_obj = pretend.stub() 
        
                   pending_github_publisher_form_cls = pretend.call_recorder( 
        
                       lambda *a, **kw: pending_github_publisher_form_obj 
        
                   ) 
        
                   monkeypatch.setattr( 
        
                       views, "PendingGitHubPublisherForm", pending_github_publisher_form_cls 
        
                   ) 
        
                   pending_google_publisher_form_obj = pretend.stub() 
        
                   pending_google_publisher_form_cls = pretend.call_recorder( 
        
                       lambda *a, **kw: pending_google_publisher_form_obj 
        
                   ) 
        
                   monkeypatch.setattr( 
        
                       views, "PendingGooglePublisherForm", pending_google_publisher_form_cls 
        
                   ) 
        
                   view = views.ManageAccountPublishingViews(pyramid_request) 
        
                   assert getattr(view, view_name)() == { 
        
                       "disabled": { 
        
                           "GitHub": True, 
        
                           "Google": True, 
        
                       }, 
        
                       "pending_github_publisher_form": pending_github_publisher_form_obj, 
        
                       "pending_google_publisher_form": pending_google_publisher_form_obj, 
        
                   } 
        
                   assert pyramid_request.flags.disallow_oidc.calls == [ 
        
                       pretend.call(AdminFlagValue.DISALLOW_GITHUB_OIDC), 
        
                       pretend.call(AdminFlagValue.DISALLOW_GOOGLE_OIDC), 
        
                       pretend.call(flag), 
        
                       pretend.call(AdminFlagValue.DISALLOW_GITHUB_OIDC), 
        
                       pretend.call(AdminFlagValue.DISALLOW_GOOGLE_OIDC), 
        
                   ] 
        
                   assert pyramid_request.session.flash.calls == [ 
        
                       pretend.call( 
        
                           ( 
        
                               f"{publisher_name}-based trusted publishing is temporarily " 
        
                               "disabled. See https://pypi.org/help#admin-intervention for " 
        
                               "details." 
        
                           ), 
        
                           queue="error", 
        
                       ) 
        
                   ] 
        
                   assert pending_github_publisher_form_cls.calls == [ 
        
                       pretend.call( 
        
                           pyramid_request.POST, 
        
                           api_token="fake-api-token", 
        
                           project_factory=project_factory, 
        
                       ) 
        
                   ]

I think if you get this branch up to date, you should just be able to add a new ActiveState parameter to these existing tests, rather than duplicating them.

th3coop · 2024-01-12T20:37:04Z

@di, i was expecting these merge conflicts to go away when #13980 was merged since I did a fresh rebase yesterday but I still see them now that that code is in main. I'll rebase again. I am now realizing that those are the params you already told me about a couple days ago.

di · 2024-01-12T20:39:27Z

@th3coop Is it possible you're rebasing on the out-of-date main from the Activestate fork rather than from PyPI's remote?

th3coop · 2024-01-12T20:49:45Z

@di, that is possible. I often pull in the upstream main to my local environment though. And I was certain I had done that before doing my last series of rebases but I could easily forgotten. Sorry for the delay on this one. I really thought it was ready for review already. I'm fixing these merge conflicts right now.

th3coop · 2024-01-12T23:07:07Z

OK. @di,I got things fixed up. Again, sorry about that mixup. Also, I'm working on getting Allowing changes to a pull request branch created from a fork enabled on this PR. I don't appear to have permissions to do that.

th3coop · 2024-01-13T00:50:50Z

Hey @di. It appears that we can't turn on **Allowing changes to a pull request branch created from a fork
**. The option to enable that doesn't show up for myself or any admins of our GitHub account and I can't find anything on the internet as to why that might be. I'm like 75% sure that it's because our fork is in the ActiveState org and not my personal GitHub account. That's my best guess based on step 4 from the docs where it says On user-owned forks.

If not having this turned on is a huge pain for y'all, let me know and I can move the remaining PRs to my personal account.

woodruffw · 2024-01-13T00:59:25Z

Yeah, that's an unfortunate known long-standing bug with GitHub orgs and forks. The hacky workaround is to manually give the person commit rights on your fork, but that's not always desirable 🙃

th3coop · 2024-01-13T01:04:40Z

I'm fine with that! Thanks for the workaround @woodruffw. Invites sent to yourself and DI. I'll remove you both when we get the PRs merged.

th3coop · 2024-01-22T22:29:54Z

Thanks so much for all your time @woodruffw !

th3coop · 2024-01-29T19:13:42Z

@miketheman will you have time to review this PR this week?

miketheman

Thanks to the heavy lifting the other folks did before, this was relatively easy to review.

I had a few notes, questions, comments inline - the only real blocker are with the linter disables which should be an easy fix.

There's other questions and comments inline, feel free to address them as you wish.
The branch should also be rebased/merged once ready.

miketheman · 2024-02-01T00:07:45Z

+        self.metrics.increment(
+            "warehouse.oidc.add_publisher.ok", tags=["publisher:ActiveState"]
+        )


Now that we're at a third implementation, it might be smart for us to look at commonalities and refactoring opportunities. I'd invoke the Rule of Three, but I'm happy to let this become the Rule of Four, so we can see how these behave in reality.
It's also starting to feel like there's inheritance/service/Protocol opportunities here, but again, worth leaving for a real refactor.

miketheman · 2024-02-01T15:52:17Z

+def _no_leading_or_trailing_dashes(form, field):
+    if field.data.startswith("-") or field.data.endswith("-"):
+        raise wtforms.validators.ValidationError(
+            _("Leading or trailing dashes are not allowed in the name")
+        )


I hear you on the specificity - it's always great to have users get the feedback that will guide them to resolution.
These validation rules are from the ActiveState service, and subject to change. Would it be smarter to have the validation here be simpler, and for the GQL API to accept the input and return the correct response in the API call?
Also not going to block on this, but something for @th3coop and the platform folks to consider.

miketheman · 2024-02-01T16:02:02Z

+            _GRAPHQL_GET_ACTOR, {"username": actor}, process_actor_response
+        )
+
+    def validate_actor(self, field):


I was reading more about wtforms validators, and I think that if there's a named validator function, it fires before the validators on the field, so validate_actor() would perform API calls before the other validators like regex/dashes are in play.

It's definitely worth re-verifying - since if our validators operate in that order, then we're performing an API call regardless. I hope I'm wrong! If that is true, then it's not only this form - so I don't think it's immediately actionable, but if you can test it, that'd be great.

If you like the idea here #15168 (comment). I would do that change for this validator as well. We really just need a "you entered the wrong text" kind of feedback here.

th3coop · 2024-02-01T18:03:46Z

Thanks for the review, @miketheman! Great notes in there. I'll work through them either later today or tomorrow. The GQL API is available again if you wanted to, or have time to take it for a manual test drive. Again, sorry for that terrible timing.

miketheman

@th3coop we're almost there! I dropped a couple of notes inline, should be straightforward changes.

Then a rebase and make translations update, and should be good to merge!

Except that the type needs to match on __str__ which shouldn't return str | None

…action

th3coop · 2024-02-08T00:45:57Z

Alrighty @miketheman, this comment remains. I'm happy to remove the regexs for username and organization, or just leave them as is. I'm leaning towards removing them though.

Other than that though, I think we're good, assuming the checks all pa....oh they passed!

miketheman · 2024-02-08T15:16:20Z

I'm leaning towards removing them though.

@th3coop go ahead and remove them - less is more 😁

th3coop · 2024-02-08T18:47:15Z

go ahead and remove them - less is more 😁

Agreed and done. Fixed the tests and took it for a spin and it works as I'd expect it to

miketheman

Almost there! just a couple last bits, and rebase against main, and I think we're good to do.

miketheman

Yay! I'll approve now, and merge in the AM.

th3coop · 2024-02-09T16:19:26Z

Yay! I'll approve now, and merge in the AM.

Hurray! Thanks so much Mike, Will and Dustin!

miketheman · 2024-02-09T16:21:09Z

@th3coop ~~I've gone ahead and enabled the OIDC provider on TestPYI so you can test run it over there.~~ Missed the note at the top of #15204, disabled.

th3coop force-pushed the OIDC-ActiveState-form branch from ada3c70 to 1e9cb8b Compare January 10, 2024 00:25

di added the trusted-publishing label Jan 10, 2024

th3coop mentioned this pull request Jan 10, 2024

OIDC provider ActiveState #13980

Closed

th3coop marked this pull request as ready for review January 10, 2024 18:39

th3coop requested a review from a team as a code owner January 10, 2024 18:39

th3coop force-pushed the OIDC-ActiveState-form branch 7 times, most recently from 59af76f to d4916b4 Compare January 12, 2024 15:26

th3coop force-pushed the OIDC-ActiveState-form branch from d4916b4 to a66fad3 Compare January 12, 2024 22:57

th3coop force-pushed the OIDC-ActiveState-form branch 5 times, most recently from 40e20e5 to 07eb067 Compare January 12, 2024 23:38

th3coop mentioned this pull request Jan 12, 2024

OIDC ActiveState token handler #15204

Merged

di reviewed Jan 13, 2024

View reviewed changes

woodruffw approved these changes Jan 22, 2024

View reviewed changes

miketheman requested changes Feb 1, 2024

View reviewed changes

miketheman requested changes Feb 7, 2024

View reviewed changes

Comment thread warehouse/templates/manage/account/publishing.html Outdated

Comment thread warehouse/templates/manage/account/publishing.html Outdated

th3coop force-pushed the OIDC-ActiveState-form branch from d8fa465 to 8f064db Compare February 7, 2024 23:34

th3coop and others added 9 commits February 7, 2024 16:40

Add ActiveState OIDC Form

5b459f4

Fix return from mocked validation functions

871c28d

Catch more invalid inputs for fields. Add test cases

e92804d

Refactor ActiveState gql calls into 1 func

2e18182

Add form for non-pending publishers

0b1de90

Backout types that do not match other classes. Simplify message concat

93acdad

Have type declarations match interface class in OIDCPublisherMixin

5bd0b8d

Except that the type needs to match on __str__ which shouldn't return str | None

Update publisher email templates to support ActiveState

a791132

Make user facing error message indicate a more appropriate follow up …

52e8231

…action

th3coop force-pushed the OIDC-ActiveState-form branch from 21081d5 to 52e8231 Compare February 8, 2024 00:41

Rmv regex validation on AS org and actor fields. Not helpful to end user

9c806c9

th3coop force-pushed the OIDC-ActiveState-form branch from 4ff5ac6 to 9c806c9 Compare February 8, 2024 18:15

miketheman requested changes Feb 8, 2024

View reviewed changes

Comment thread warehouse/templates/manage/account/publishing.html Outdated

Comment thread warehouse/oidc/forms/activestate.py Outdated

Review touchups

933fefe

th3coop force-pushed the OIDC-ActiveState-form branch from aa679b4 to 933fefe Compare February 8, 2024 21:20

Merge branch 'main' into OIDC-ActiveState-form

a038784

miketheman approved these changes Feb 8, 2024

View reviewed changes

miketheman merged commit 5fca44a into pypi:main Feb 9, 2024

Conversation

th3coop commented Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

di commented Jan 10, 2024

Uh oh!

th3coop commented Jan 12, 2024

Uh oh!

di commented Jan 12, 2024

Uh oh!

th3coop commented Jan 12, 2024

Uh oh!

di commented Jan 12, 2024

Uh oh!

th3coop commented Jan 12, 2024

Uh oh!

th3coop commented Jan 12, 2024

Uh oh!

th3coop commented Jan 13, 2024

Uh oh!

woodruffw commented Jan 13, 2024

Uh oh!

th3coop commented Jan 13, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

th3coop commented Jan 22, 2024

Uh oh!

th3coop commented Jan 29, 2024

Uh oh!

miketheman left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miketheman Feb 1, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miketheman Feb 1, 2024

Choose a reason for hiding this comment

Uh oh!

miketheman Feb 1, 2024

Choose a reason for hiding this comment

Uh oh!

th3coop Feb 6, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

th3coop commented Feb 1, 2024

Uh oh!

miketheman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

th3coop commented Feb 8, 2024

Uh oh!

miketheman commented Feb 8, 2024

Uh oh!

th3coop commented Feb 8, 2024

Uh oh!

miketheman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

miketheman left a comment

Choose a reason for hiding this comment

Uh oh!

th3coop commented Feb 9, 2024

Uh oh!

miketheman commented Feb 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

th3coop commented Jan 8, 2024 •

edited

Loading

miketheman left a comment •

edited

Loading

miketheman commented Feb 9, 2024 •

edited

Loading