Skip to content

fix: stop extending API key access if OIDC refresh is available #17878

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

Merged
merged 4 commits into from
May 19, 2025

Conversation

spikecurtis
Copy link
Contributor

@spikecurtis spikecurtis commented May 16, 2025

fixes #17070

Cleans up our handling of APIKey expiration and OIDC to keep them separate concepts. For an OIDC-login APIKey, both the APIKey and OIDC link must be valid to login. If the OIDC link is expired and we have a refresh token, we will attempt to refresh.

OIDC refreshes do not have any effect on APIKey expiry.

#17070 (comment) explains why this is the correct behavior.

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@spikecurtis spikecurtis force-pushed the spike/17070-apikey-oidc branch from 473d5a4 to c430f42 Compare May 16, 2025 11:21
@spikecurtis spikecurtis requested review from Emyrk and mafredri May 16, 2025 11:24
@spikecurtis spikecurtis marked this pull request as ready for review May 16, 2025 11:25
// Checking if the key is expired.
// NOTE: The `RequireAuth` React component depends on this `Detail` to detect when
// the users token has expired. If you change the text here, make sure to update it
// in site/src/components/RequireAuth/RequireAuth.tsx as well.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that RequireAuth.tsx was modified to not have this string match dependency in #9442

Copy link
Contributor Author

Also includes some extra logging in oidctest which I figured would be useful to leave in.

Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

Two small suggestions but the change makes sense to me. I can approve if need be but I'd feel better if @Emyrk also took a look, so deferring approval for now.

Copy link
Member

@Emyrk Emyrk left a comment

Choose a reason for hiding this comment

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

This all makes sense to me 👍

@spikecurtis spikecurtis force-pushed the spike/17070-apikey-oidc branch from c430f42 to e6f157c Compare May 19, 2025 07:00
@spikecurtis spikecurtis force-pushed the spike/17070-apikey-oidc branch from 05131e2 to 45df24d Compare May 19, 2025 07:51
@spikecurtis spikecurtis merged commit 1a41608 into main May 19, 2025
35 checks passed
Copy link
Contributor Author

Merge activity

@spikecurtis spikecurtis deleted the spike/17070-apikey-oidc branch May 19, 2025 08:05
@github-actions github-actions bot locked and limited conversation to collaborators May 19, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: api key not refreshed when api key expired but oauth2 access token not
3 participants