-
Notifications
You must be signed in to change notification settings - Fork 68
Add force-refresh path for cached U2M tokens #1552
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
Changes from all commits
968933d
03ddc9b
d9b1a2c
b11d7c3
b73f4e9
08dab3c
a414d1e
b3d26c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,14 @@ | ||
| package u2m | ||
|
|
||
| // InvalidRefreshTokenError is returned from PersistentAuth's Load() method | ||
| // if the access token has expired and the refresh token in the token cache | ||
| // is invalid. | ||
| import "errors" | ||
|
|
||
| // ErrMissingRefreshToken is returned when a token refresh is requested but the | ||
| // cached OAuth token does not include a refresh token. | ||
| var ErrMissingRefreshToken = errors.New("cached token has no refresh token") | ||
|
|
||
| // InvalidRefreshTokenError is returned from PersistentAuth's Token() and | ||
| // ForceRefreshToken() methods when a token refresh is attempted and the cached | ||
| // refresh token is invalid. | ||
| type InvalidRefreshTokenError struct { | ||
| error | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -222,17 +222,17 @@ func NewPersistentAuth(ctx context.Context, opts ...PersistentAuthOption) (*Pers | |
| return p, nil | ||
| } | ||
|
|
||
| // Token loads the OAuth2 token for the given OAuthArgument from the cache. If | ||
| // the token is expired or close to expiry, it is refreshed using the refresh | ||
| // token. | ||
| // | ||
| // loadToken loads the cached OAuth2 token for the configured OAuthArgument. | ||
|
mihaimitrea-db marked this conversation as resolved.
|
||
| // When a profile is set, lookup is by profile key first. If the profile key is | ||
| // not found and the OAuthArgument implements HostCacheKeyProvider, a fallback | ||
| // lookup by host key is attempted. If found, the token is returned without | ||
| // being migrated to the profile key (see inline comment for rationale). | ||
| func (a *PersistentAuth) Token() (t *oauth2.Token, err error) { | ||
| // | ||
| // The returned token may be expired; callers are responsible for deciding | ||
| // whether and how to refresh it. | ||
| func (a *PersistentAuth) loadToken() (*oauth2.Token, error) { | ||
| key := a.oAuthArgument.GetCacheKey() | ||
| t, err = a.cache.Lookup(key) | ||
| t, err := a.cache.Lookup(key) | ||
| if errors.Is(err, cache.ErrNotFound) { | ||
| hostKey := a.hostCacheKey() | ||
| if hostKey != "" && hostKey != key { | ||
|
|
@@ -250,6 +250,18 @@ func (a *PersistentAuth) Token() (t *oauth2.Token, err error) { | |
| if err != nil { | ||
| return nil, fmt.Errorf("cache: %w", err) | ||
| } | ||
| return t, nil | ||
|
mihaimitrea-db marked this conversation as resolved.
|
||
| } | ||
|
|
||
| // Token loads the OAuth2 token for the given OAuthArgument from the cache. If | ||
| // the token is expired or close to expiry, it is refreshed using the refresh | ||
| // token. When a proactive refresh (token still valid but near expiry) fails, | ||
| // the existing token is returned so the caller is not blocked. | ||
| func (a *PersistentAuth) Token() (*oauth2.Token, error) { | ||
| t, err := a.loadToken() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if needsRefresh(t) { | ||
| if refreshedToken, err := a.refresh(t); err == nil { | ||
| t = refreshedToken | ||
|
|
@@ -259,9 +271,35 @@ func (a *PersistentAuth) Token() (t *oauth2.Token, err error) { | |
| logger.Debugf(a.ctx, "proactive token refresh failed, returning existing token: %v", err) | ||
| } | ||
| } | ||
| t.RefreshToken = "" | ||
| return t, nil | ||
|
mihaimitrea-db marked this conversation as resolved.
|
||
| } | ||
|
|
||
| // Do not include the refresh token for security reasons. Refresh tokens are | ||
| // long-lived credentials that we do not want to expose unnecessarily. | ||
| // ForceRefreshToken loads the OAuth2 token from the cache and always refreshes | ||
| // it, regardless of its expiry. Unlike Token(), if the refresh fails the error | ||
| // is always returned -- the caller explicitly asked for a fresh token, so | ||
| // silently falling back to a stale one would be incorrect. | ||
| // | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 This dual-write/migration behavior also applies to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Addressed in 08dab3c. Doc updated per your suggestion. |
||
| // When the token was loaded via host-key fallback (profile key not found), the | ||
|
mihaimitrea-db marked this conversation as resolved.
|
||
| // refreshed token is dual-written to both the profile key and host key, | ||
| // effectively migrating the token to the profile. Token() has the same | ||
| // migration behavior when it refreshes a host-fallback token (expired or | ||
| // nearly expired); ForceRefreshToken widens this to still-valid tokens because | ||
| // it always refreshes. | ||
| // | ||
| // TODO: Thread provenance out of loadToken() so callers know whether the token | ||
| // came from the primary key or host-key fallback. ForceRefreshToken could then | ||
| // refuse to migrate a fallback token or refresh without writing to the profile | ||
| // key, avoiding the risk of making a wrong-scoped token sticky. | ||
| func (a *PersistentAuth) ForceRefreshToken() (*oauth2.Token, error) { | ||
| t, err := a.loadToken() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| t, err = a.refresh(t) | ||
| if err != nil { | ||
|
mihaimitrea-db marked this conversation as resolved.
|
||
| return nil, fmt.Errorf("forced token refresh: %w", err) | ||
| } | ||
| t.RefreshToken = "" | ||
|
simonfaltum marked this conversation as resolved.
|
||
| return t, nil | ||
| } | ||
|
|
@@ -278,7 +316,20 @@ func needsRefresh(t *oauth2.Token) bool { | |
|
|
||
| // refresh refreshes the token for the given OAuthArgument, storing the new | ||
| // token in the cache. | ||
|
mihaimitrea-db marked this conversation as resolved.
|
||
| // | ||
| // TODO: This read-refresh-write sequence is not coordinated across processes. | ||
| // Because the CLI is stateless, two separate CLI invocations can load the same | ||
| // cached refresh token, both attempt a refresh, and race to update the cache. | ||
| // This should be fixed in a follow-up by adding cross-process coordination | ||
| // around refresh and cache writes. | ||
| func (a *PersistentAuth) refresh(oldToken *oauth2.Token) (*oauth2.Token, error) { | ||
| // Fail fast with ErrMissingRefreshToken instead of letting the oauth2 | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 This changes the error semantics of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Addressed in 08dab3c. Moved ErrMissingRefreshToken to API Changes; changelog now documents it as a general error returned when refresh is requested but the cached token has no refresh token. |
||
| // library attempt to refresh and return a misleading error (e.g. "token | ||
| // expired" when the real problem is that the cached token is not | ||
| // refresh-capable). | ||
| if oldToken.RefreshToken == "" { | ||
| return nil, ErrMissingRefreshToken | ||
| } | ||
| cfg, err := a.oauth2Config() | ||
| if err != nil { | ||
| return nil, err | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.