Refactor OIDC mint token endpoint #14063

th3coop · 2023-07-05T20:36:11Z

Make it easier to mint tokens with additional publishers

This PR splits warehouse.oidc.view.mint_token_from_oidc into two functions. 1, mint_token, is OIDC provider specific. The other, mint_token_from_oidc_* that is OIDC provider agnostic. This should make it easier to add more OIDC provider token handlers.

ActiveState Internal work tracking ID: DS-1722

th3coop · 2023-07-07T20:53:26Z

tests/unit/oidc/test_views.py

-        publisher_name="fakepublishername",
-        publisher_url=lambda x=None: "https://fake/url",
+
+    publisher = github.GitHubPublisher(


I had to do this change because I had to add this change to stop linting errors from occurring here and anywhere else a prop was accessed on the OIDCPublisher object.

th3coop · 2023-07-08T00:05:00Z

warehouse/oidc/models/_core.py

-    def publisher_url(self, claims=None) -> str | None:  # pragma: no cover
+    def publisher_url(
+        self, claims: SignedClaims | None = None
+    ) -> str:  # pragma: no cover


I've been meaning to make this change for sometime so adding it to this PR so it's upstream sooner. This should never be None right? Or shouldn't, so types and tests should reflect that expectation? If that's not true, I'm happy to revert.

The original intention here is that not all publishers would necessarily have a URL to link to, so this could be None, although that's not currently the case.

Ahh ok. Then my change doesn't make sense. Will revert.

th3coop · 2023-07-08T00:06:25Z

I wanted to address the comment here in this PR but I ended up breaking things up into the known route and the publisher we know that route handles. So, no looking at the request to see which publisher to load. Does this make sense?

th3coop · 2023-07-10T18:47:18Z

Ok, this is ready for review. @di, is it appropriate to ping you in that case? I've had a triple check look at the submitting patches docs but want to make sure I'm not bypassing a step. The actions on this repo keep you pretty locked in to that pages processes which is great.

th3coop · 2023-07-12T22:49:10Z

I noticed that quite a few of the tests in warehouse/tests/unit/oidc/test_views.py are implicitly testing the GitHubPublisher and PendingGitHubPublisher classes functionality when it doesn't need to as that functionality is tested in oidc/models/test_github.py.

I went down a bit of a rabbithole to try to abstract things away a bit more in warehouse/tests/unit/oidc/test_views.py so that the new oidc.views::mint_token() can really be tested "publisher agnostically".

To achieve that I had to add two new factories to warehouse/tests/common/db/oidc.py, PendingOIDCPublisherFactory and OIDCPublisherFactory. It's all working quite nicely except for one hitch.

OIDCPublisher.publisher_name is a @property so I can't "stub" it's value. If I remove the @property decorator though, then I can stub it as a function.

Before I put in time to wrap up these changes, I wanted to run the idea of OIDCPublisher.publisher_name being a function, not a property of the class.

If I were able to make this change and introduce these new Factories, the only function in oidc/test_views.py that needs to know about a specific publisher is the test_mint_token_from_github_oidc_not_enabled method in this PR, which doesn't actually need to construct a Publisher.

If it made it easier to review and give feedback/answer this question, I could put up a PR that contains only the change to property_name, the new factories, and 1 example for each factory where they would be used in the existing tests.

th3coop · 2023-07-17T18:19:46Z

Morning @di, any thoughts on my question above?

th3coop · 2023-08-16T21:12:21Z

@di, if this PR overlaps with work you already have on the go, I don't mind dropping this PR. I should have asked before going this route in the first place. Let me know what your preference is.

di · 2023-08-16T21:13:23Z

@th3coop I think this pairs nicely actually, let me get my branch up and we can figure out next steps.

th3coop · 2023-08-16T21:25:10Z

Works for me, @di.

th3coop · 2023-08-31T23:44:48Z

#14063 (comment)
Any luck with that work @di? I've got the majority of implemenation work done for adding ActiveState but waiting on my PR here to wrap up to go through finishing/fixing tests and manually testing of my other PRs.

th3coop · 2023-11-21T20:26:24Z

Hi @di, We're hoping to move this stuff along over the next few weeks. Any chance the branch you mentioned above is ready?

Make it easier to mint tokens with additional publishers

th3coop · 2023-11-27T22:52:12Z

tests/unit/oidc/test_views.py

        id="fakeprojectid",
        record_event=pretend.call_recorder(lambda **kw: None),
    )
-    publisher = pretend.stub(


This was changed to use the factory due to the type check here and here which was added because of mypy errors in the code after those type checks.

th3coop · 2023-11-27T23:05:39Z

tests/unit/oidc/test_views.py

    def _find_publisher(claims, pending=False):
        if pending:
-            raise errors.InvalidPublisherError
+            return None


This change is wrong I'm pretty sure but I'm not sure what to do about it.

I made this change because the coverage report was telling me that this line was never getting hit when the tests run.

This is true and correct because if I call oidc_service.find_publisher(claims, pending=False) and then I will NEVER get back anything but an OIDCPublisher, OR it will raise an exception.

The static type checking doesn't know that though, it just knows that OIDCPublisher | PendingOIDCPublisher will be returned.

The first thing that comes to mind to me is that _find_publisher shouldn't return two different types based on one of the inputs to the method. There should be a method per Class.

There are obviously other options but that's the first one that comes to mind for me.

I agree, this seems wrong, I think the line you're having trouble with is probably unnecessary, see the comment there.

th3coop · 2023-11-27T23:10:44Z

tests/unit/oidc/test_views.py

    ]


+def test_mint_token_from_oidc_only_pending_publisher_fail(monkeypatch, db_request):


This test tests an impossible state to reach as laid out in this comment and I'm not sure what to do about it.

th3coop · 2023-11-27T23:14:23Z

Closing loops on this commend: I ended up following the existing pattern of stubbing away the code that shouldn't be tested, to focus on the code being test. So this new classes was removed from my changes.

th3coop · 2023-11-27T23:15:30Z

@di I saw your Google PR seems to have been merged which I believe is what I was waiting on for this PR so I've gone through this one again to get it ready for review. It's now ready for review.

…n handler tests

di

Hi @th3coop, thanks for your extreme patience here. I've given this a review and I think it's mostly good to go aside from some small comments. Thank you!

di · 2024-01-05T00:51:33Z

warehouse/oidc/views.py

-        request.response.status = 422
-        return {"message": "Token request failed", "errors": errors}
-
+def mint_token_from_oidc_github(request: Request):


I think instead we should probably make a generic non-GitHub-specific endpoint that accepts any OIDC token in the body and attempts to mint a token for it based on properties of the token (specifically, the iss claim should allow us to pick the right publisher type)

I don't see a great reason to maintain a separate endpoint for every OIDC provider we want to support here, that was probably not necessary in the GitHub implementation.

(cc @woodruffw for any thoughts as well)

Yes, agreed -- the main reason I did that originally was to avoid snooping the unverified JWT claims before verifying them, but that ship has sailed elsewhere in the implementation IIRC (and is not a security risk, so long as we perform proper key segregation and add both positive and negative tests).

We'll probably have to keep the GitHub-specific endpoint around for a while because of existing users (including some people who have reimplemented the token exchange flow), but it can be a thin shim over the "generic" endpoint 🙂

di · 2024-01-05T00:56:55Z

warehouse/oidc/views.py

+    if not isinstance(publisher, OIDCPublisher):
+        # This should be impossible, but we have to perform this type check to
+        # appease mypy otherwise we get type errors in the code after this
+        # point.
+        return _invalid(
+            errors=[
+                {
+                    "code": "invalid-publisher",
+                    "description": "valid token, but no corresponding publisher",
+                }
+            ],
+            request=request,
+        )


This seems weird, does typing the find_publisher call fix this? Can we assert isinstance(publisher, OIDCPublisher) in the try/except instead?

Yeah, this was icky. Good idea to use assert.

di · 2024-01-05T00:57:54Z

tests/unit/oidc/test_views.py

    def _find_publisher(claims, pending=False):
        if pending:
-            raise errors.InvalidPublisherError
+            return None


I agree, this seems wrong, I think the line you're having trouble with is probably unnecessary, see the comment there.

woodruffw · 2024-01-05T20:32:47Z

@th3coop JFYI: I'm going to make a new PR off of your branch here, to address the comments/suggestions above. I'll preserve your history though, so it'll be your authorship in the merge 🙂

th3coop · 2024-01-05T21:30:20Z

@di thanks for the review and no sweat. I appreciate any of your time! @woodruffw much appreciated! I was just about to get to this but thanks for taking it. Do you need anything from me in #15148? I really should have thought of using a tokens iss field to establish what publisher it was 🤦

woodruffw · 2024-01-05T21:33:52Z

No problem! And nope, nothing needed at the moment -- I'll give you a ping there if I have any questions. Thanks a ton for your hard work here!

di · 2024-01-05T22:11:40Z

Thanks @th3coop, I wasn't sure what your availability was and you've already put in a lot of work here so I asked @woodruffw to work on keeping this moving. We'd definitely appreciate you getting #13980 ready to merge once this/#15148 are merged if you're able!

woodruffw · 2024-01-05T22:38:00Z

#15148 is undrafted, so closing here!

th3coop · 2024-01-05T22:39:24Z

No problem, I'll resolve the merge conflict in #13980. Clarification: #13980 is 1 of 3 PRs that add the full OIDC treatment for ActiveState.
#13980 adds the DB modeling and most of the validation (as you've seen).

The other two are PRs in the ActiveState fork:

OIDC active state form ActiveState/warehouse#3 adds the form in the account settings to configure an ActiveState Publisher.
ActiveState OIDC Token Handler Endpoint ActiveState/warehouse#1 adds the handler endpoint

I'll create subsequent PRs against pypi/warehouse:main as the respective "parent" PR gets merged.

Hopefully this makes sense for chunking up the changes. I didn't want to throw a PR at y'all with that includes ALL of the changes.

* Refactor OIDC mint token endpoint Make it easier to mint tokens with additional publishers * Move default JWT validation to general OIDC handler function * publisher_name to method. Add more factories. Publisher agnostic token handler tests * oidc/views: typo: JsonRespone -> JsonResponse Signed-off-by: William Woodruff <[email protected]> * lintage, begin making OIDC endpoint generic Signed-off-by: William Woodruff <[email protected]> * tests, warehouse: generic OIDC minting endpoint Needs coverage, but the idea is done. Signed-off-by: William Woodruff <[email protected]> * tests: complete coverage Signed-off-by: William Woodruff <[email protected]> * tests, warehouse: eliminate impossible states Signed-off-by: William Woodruff <[email protected]> * tests, warehouse: feedback Signed-off-by: William Woodruff <[email protected]> * error specialization Signed-off-by: William Woodruff <[email protected]> --------- Signed-off-by: William Woodruff <[email protected]> Co-authored-by: Carey Hoffman <[email protected]>

th3coop force-pushed the OIDC-Publisher-refactor-mint-token-handling branch from 0d2b3a5 to 933453d Compare July 5, 2023 22:14

th3coop commented Jul 7, 2023

View reviewed changes

th3coop marked this pull request as ready for review July 7, 2023 20:54

th3coop requested a review from a team as a code owner July 7, 2023 20:54

th3coop commented Jul 8, 2023

View reviewed changes

th3coop mentioned this pull request Jul 13, 2023

ActiveState OIDC Token Handler Endpoint ActiveState/warehouse#1

Closed

th3coop force-pushed the OIDC-Publisher-refactor-mint-token-handling branch from 04fa637 to 626618f Compare August 16, 2023 23:07

th3coop force-pushed the OIDC-Publisher-refactor-mint-token-handling branch 2 times, most recently from 84cdb92 to 84851d2 Compare August 30, 2023 17:23

miketheman mentioned this pull request Oct 27, 2023

Buildkite OIDC support #14814

Draft

th3coop force-pushed the OIDC-Publisher-refactor-mint-token-handling branch from 84851d2 to 1700989 Compare November 21, 2023 21:27

th3coop added 2 commits November 22, 2023 13:05

Refactor OIDC mint token endpoint

05b3d06

Make it easier to mint tokens with additional publishers

Move default JWT validation to general OIDC handler function

40b1708

th3coop force-pushed the OIDC-Publisher-refactor-mint-token-handling branch 4 times, most recently from 5a6d423 to 373d305 Compare November 27, 2023 22:26

th3coop commented Nov 27, 2023

View reviewed changes

th3coop requested a review from di November 27, 2023 23:14

publisher_name to method. Add more factories. Publisher agnostic toke…

e750531

…n handler tests

th3coop force-pushed the OIDC-Publisher-refactor-mint-token-handling branch from 373d305 to e750531 Compare November 27, 2023 23:48

miketheman added the trusted-publishing label Jan 3, 2024

This was referenced Jan 5, 2024

Genericize OIDC templates, forms and views #15143

Merged

Add support for Google trusted publishing #15144

Merged

di reviewed Jan 5, 2024

View reviewed changes

woodruffw mentioned this pull request Jan 5, 2024

Refactor OIDC mint token endpoint (cont. #14063) #15148

Merged

woodruffw closed this Jan 5, 2024

		]


		def test_mint_token_from_oidc_only_pending_publisher_fail(monkeypatch, db_request):

Refactor OIDC mint token endpoint #14063

Refactor OIDC mint token endpoint #14063

Uh oh!

Conversation

th3coop commented Jul 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

th3coop commented Jul 8, 2023

Uh oh!

th3coop commented Jul 10, 2023

Uh oh!

th3coop commented Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

th3coop commented Jul 17, 2023

Uh oh!

th3coop commented Aug 16, 2023

Uh oh!

di commented Aug 16, 2023

Uh oh!

th3coop commented Aug 16, 2023

Uh oh!

th3coop commented Aug 31, 2023

Uh oh!

th3coop commented Nov 21, 2023

Uh oh!

th3coop Nov 27, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

th3coop commented Nov 27, 2023

Uh oh!

th3coop commented Nov 27, 2023

Uh oh!

di left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

woodruffw commented Jan 5, 2024

Uh oh!

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

Uh oh!

Uh oh!

woodruffw commented Jan 5, 2024

Uh oh!

di commented Jan 5, 2024

Uh oh!

woodruffw commented Jan 5, 2024

Uh oh!

th3coop commented Jan 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

th3coop commented Jul 5, 2023 •

edited

Loading

th3coop commented Jul 12, 2023 •

edited

Loading

th3coop Nov 27, 2023 •

edited

Loading

th3coop commented Jan 5, 2024 •

edited

Loading