Skip to content

Deprecate and remove /oidc/github/mint-token? #15181

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

Closed
woodruffw opened this issue Jan 10, 2024 · 4 comments · Fixed by #15183
Closed

Deprecate and remove /oidc/github/mint-token? #15181

woodruffw opened this issue Jan 10, 2024 · 4 comments · Fixed by #15183

Comments

@woodruffw
Copy link
Member

Following #15148, /_/oidc/github/mint-token is now an obsolete alias for /_/oidc/mint-token. As such, it can be removed.

From a quick search of GitHub, there are a decent number of projects that have manually reimplemented the Trusted Publishing flow, including PDM and Sphinx's CI/CD. Additionally, there is probably a decent "tail" of gh-action-pypi-publish users who won't immediately upgrade to the latest version, once we update the endpoint there.

Given that, this endpoint probably needs a deprecation period before outright removal (during which time I'll help all of the projects I can find migrate). If that seems reasonable, I can go ahead and create a d.p.o thread announcing the deprecation.

Alternatively, we could keep the alias around indefinitely, since it isn't really hurting anything. If that's preferred, please close this 🙂

@di
Copy link
Member

di commented Jan 10, 2024

I think we can just keep it around indefinitely. That said, we don't need multiple views for it, we can just mount the same view at multiple routes:

config.add_route("oidc.mint_token", "/_/oidc/mint-token", domain=auth)
config.add_route("oidc.github.mint_token", "/_/oidc/github/mint-token", domain=auth)

can just become:

config.add_route("oidc.mint_token", "/_/oidc/mint-token", domain=auth) 
config.add_route("oidc.mint_token", "/_/oidc/github/mint-token", domain=auth)

@woodruffw
Copy link
Member Author

woodruffw commented Jan 10, 2024

I might be doing something wrong, but it looks like add_route requires the route names to be unique:

warehouse-web-1            |   File "/opt/warehouse/lib/python3.11/site-packages/pyramid/config/actions.py", line 483, in resolveConflicts
warehouse-web-1            |     raise ConfigurationConflictError(conflicts)
warehouse-web-1            | pyramid.exceptions.ConfigurationConflictError: Conflicting configuration actions
warehouse-web-1            |   For: ('route', 'oidc.mint_token')
warehouse-web-1            |     Line 50 of file /opt/warehouse/src/warehouse/oidc/__init__.py:
warehouse-web-1            |         config.add_route("oidc.mint_token", "/_/oidc/mint-token", domain=auth)
warehouse-web-1            |     Line 51 of file /opt/warehouse/src/warehouse/oidc/__init__.py:
warehouse-web-1            |         config.add_route("oidc.mint_token", "/_/oidc/github/mint
-token", domain=auth)

That's with the following diff:

diff --git a/warehouse/oidc/__init__.py b/warehouse/oidc/__init__.py
index 8fddb01d1..739b68274 100644
--- a/warehouse/oidc/__init__.py
+++ b/warehouse/oidc/__init__.py
@@ -48,7 +48,7 @@ def includeme(config):
 
     config.add_route("oidc.audience", "/_/oidc/audience", domain=auth)
     config.add_route("oidc.mint_token", "/_/oidc/mint-token", domain=auth)
-    config.add_route("oidc.github.mint_token", "/_/oidc/github/mint-token", domain=auth)
+    config.add_route("oidc.mint_token", "/_/oidc/github/mint-token", domain=auth)
 
     # Compute OIDC metrics periodically
     config.add_periodic_task(crontab(minute=0, hour="*"), compute_oidc_metrics)
diff --git a/warehouse/oidc/views.py b/warehouse/oidc/views.py
index f047868b5..2c0961023 100644
--- a/warehouse/oidc/views.py
+++ b/warehouse/oidc/views.py
@@ -11,13 +11,11 @@
 # limitations under the License.
 
 import time
-
 from datetime import datetime
 from typing import TypedDict
 
 import jwt
 import sentry_sdk
-
 from pydantic import BaseModel, StrictStr, ValidationError
 from pyramid.request import Request
 from pyramid.response import Response
@@ -91,12 +89,6 @@ def oidc_audience(request: Request):
     return {"audience": audience}
 
 
-@view_config(
-    route_name="oidc.github.mint_token",
-    require_methods=["POST"],
-    renderer="json",
-    require_csrf=False,
-)
 @view_config(
     route_name="oidc.mint_token",
     require_methods=["POST"],

(Configurator.add_route also implies that name needs to be unique on a per-app basis: https://docs.pylonsproject.org/projects/pyramid/en/latest/api/config.html#pyramid.config.Configurator.add_route)

@di
Copy link
Member

di commented Jan 10, 2024

Ah, I guess I'm wrong! Let's just add a comment describing why the extra route exists and call this resolved.

@woodruffw
Copy link
Member Author

Sounds good! I'll close with that comment PR 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants