From da31464aecdbed9afe35d6c3f095447ce08de468 Mon Sep 17 00:00:00 2001 From: Elizabeth Healy Date: Mon, 3 Aug 2026 23:32:27 -0400 Subject: [PATCH 1/3] add refresh support --- otdfctl/cmd/auth/login.go | 9 ++ otdfctl/cmd/common/common.go | 5 +- otdfctl/pkg/auth/auth.go | 20 ++- otdfctl/pkg/auth/errors.go | 1 + otdfctl/pkg/auth/refresh.go | 13 ++ otdfctl/pkg/auth/refresh_test.go | 29 +++- otdfctl/pkg/auth/token_source.go | 148 ++++++++++++++++++ otdfctl/pkg/auth/token_source_test.go | 206 ++++++++++++++++++++++++++ 8 files changed, 425 insertions(+), 6 deletions(-) create mode 100644 otdfctl/pkg/auth/token_source.go create mode 100644 otdfctl/pkg/auth/token_source_test.go diff --git a/otdfctl/cmd/auth/login.go b/otdfctl/cmd/auth/login.go index d1618ebba8..8b07959cb9 100644 --- a/otdfctl/cmd/auth/login.go +++ b/otdfctl/cmd/auth/login.go @@ -2,6 +2,7 @@ package auth import ( "fmt" + "log/slog" "github.com/opentdf/platform/otdfctl/cmd/common" "github.com/opentdf/platform/otdfctl/pkg/auth" @@ -39,6 +40,14 @@ func codeLogin(cmd *cobra.Command, args []string) { }); err != nil { c.ExitWithError("failed to set auth credentials", err) } + + // The profile store is filesystem-backed (see common.InitProfile). Warn the + // user that a refresh token has been written to disk and recommend the OS + // keychain when higher assurance is required. + if tok.RefreshToken != "" { + slog.Warn("refresh token stored in filesystem-backed profile; OS keychain is recommended for higher assurance") + } + c.ExitWithMessage(fmt.Sprintf("Code login complete for profile: [%s]", cp.Name()), cli.ExitCodeSuccess) } diff --git a/otdfctl/cmd/common/common.go b/otdfctl/cmd/common/common.go index c6a94594cc..075d0155e9 100644 --- a/otdfctl/cmd/common/common.go +++ b/otdfctl/cmd/common/common.go @@ -207,8 +207,11 @@ func NewHandler(c *cli.Cli, hooks ...handlers.Hook) handlers.Handler { cli.ExitWithWarning("Profile missing credentials. Please login or add client credentials.") } + if errors.Is(err, auth.ErrRefreshTokenInvalid) { + cli.ExitWithWarning("Your session has expired. Please login again.") + } if errors.Is(err, auth.ErrAccessTokenExpired) { - cli.ExitWithWarning("Access token expired. Please login or add flag-provided credentials.") + cli.ExitWithWarning("Access token expired and could not be refreshed. Please login or add flag-provided credentials.") } if errors.Is(err, auth.ErrAccessTokenNotFound) { cli.ExitWithWarning("No access token found. Please login or add flag-provided credentials.") diff --git a/otdfctl/pkg/auth/auth.go b/otdfctl/pkg/auth/auth.go index bd505d88ab..bc24d120e7 100644 --- a/otdfctl/pkg/auth/auth.go +++ b/otdfctl/pkg/auth/auth.go @@ -170,8 +170,7 @@ func GetSDKAuthOptionFromProfile(profile *profiles.OtdfctlProfileStore) (sdk.Opt case profiles.AuthTypeClientCredentials: return sdk.WithClientCredentials(c.ClientID, c.ClientSecret, NormalizeScopes(c.Scopes)), nil case profiles.AuthTypeAccessToken: - tokenSource := oauth2.StaticTokenSource(buildToken(&c)) - return sdk.WithOAuthAccessTokenSource(tokenSource), nil + return sdk.WithOAuthAccessTokenSource(newProfileTokenSource(profile)), nil default: return nil, ErrInvalidAuthType } @@ -190,9 +189,18 @@ func ValidateProfileAuthCredentials(ctx context.Context, profile *profiles.Otdfc } return nil case profiles.AuthTypeAccessToken: - if !buildToken(&c).Valid() { + if buildToken(&c).Valid() { + return nil + } + if !HasRefreshToken(profile) { return ErrAccessTokenExpired } + if err := RefreshAccessToken(ctx, profile); err != nil { + if errors.Is(err, ErrRefreshTokenInvalid) { + return err + } + return errors.Join(ErrAccessTokenExpired, err) + } default: return ErrInvalidAuthType } @@ -206,6 +214,12 @@ func GetTokenWithProfile(ctx context.Context, profile *profiles.OtdfctlProfileSt case profiles.AuthTypeClientCredentials: return GetTokenWithClientCreds(ctx, profile.GetEndpoint(), c.ClientID, c.ClientSecret, profile.GetTLSNoVerify(), c.Scopes) case profiles.AuthTypeAccessToken: + if !buildToken(&c).Valid() && HasRefreshToken(profile) { + if err := RefreshAccessToken(ctx, profile); err != nil { + return nil, err + } + c = profile.GetAuthCredentials() + } return buildToken(&c), nil default: return nil, ErrInvalidAuthType diff --git a/otdfctl/pkg/auth/errors.go b/otdfctl/pkg/auth/errors.go index b074779795..760be84b87 100644 --- a/otdfctl/pkg/auth/errors.go +++ b/otdfctl/pkg/auth/errors.go @@ -12,4 +12,5 @@ var ( ErrProfileCredentialsNotFound = errors.New("profile missing credentials") ErrNoRefreshToken = errors.New("no refresh token available") ErrRefreshFailed = errors.New("token refresh failed") + ErrRefreshTokenInvalid = errors.New("refresh token is invalid or expired; please re-login") ) diff --git a/otdfctl/pkg/auth/refresh.go b/otdfctl/pkg/auth/refresh.go index 8965dbcb4a..3e446f2e28 100644 --- a/otdfctl/pkg/auth/refresh.go +++ b/otdfctl/pkg/auth/refresh.go @@ -82,6 +82,12 @@ func refreshAccessToken(ctx context.Context, profile *profiles.OtdfctlProfileSto tokenSource := oauth2Config.TokenSource(ctx, oldToken) newToken, err := tokenSource.Token() if err != nil { + if isInvalidGrant(err) { + // Refresh token is dead server-side; wipe stored creds so the next + // command hits the login prompt cleanly. + _ = profile.SetAuthCredentials(profiles.AuthCredentials{}) + return errors.Join(ErrRefreshTokenInvalid, err) + } return fmt.Errorf("%w: %w", ErrRefreshFailed, err) } @@ -143,3 +149,10 @@ func getTokenEndpoint(endpoint string, tlsNoVerify bool) (string, error) { } return pc.tokenEndpoint, nil } + +// isInvalidGrant reports whether err is an oauth2 token-endpoint error carrying +// RFC 6749 ยง5.2 code "invalid_grant" โ€” i.e. the refresh token is expired or revoked. +func isInvalidGrant(err error) bool { + var re *oauth2.RetrieveError + return errors.As(err, &re) && re.ErrorCode == "invalid_grant" +} diff --git a/otdfctl/pkg/auth/refresh_test.go b/otdfctl/pkg/auth/refresh_test.go index b7e4c9c0af..a08427e6cf 100644 --- a/otdfctl/pkg/auth/refresh_test.go +++ b/otdfctl/pkg/auth/refresh_test.go @@ -189,9 +189,10 @@ func TestRefreshAccessTokenEndpointError(t *testing.T) { require.ErrorContains(t, err, "failed to get token endpoint") } -func TestRefreshAccessTokenRefreshFails(t *testing.T) { +func TestRefreshAccessTokenInvalidGrant(t *testing.T) { tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusUnauthorized) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusBadRequest) err := json.NewEncoder(w).Encode(map[string]string{"error": "invalid_grant"}) assert.NoError(t, err) })) @@ -201,10 +202,34 @@ func TestRefreshAccessTokenRefreshFails(t *testing.T) { return tokenServer.URL, nil } + profile := newTestProfile(t, profiles.AuthTypeAccessToken, "tok", "refresh", time.Now().Add(-time.Hour).Unix()) + err := refreshAccessToken(t.Context(), profile, resolver) + require.Error(t, err) + require.ErrorIs(t, err, ErrRefreshTokenInvalid) + + // Creds should be wiped so the next command re-prompts login. + creds := profile.GetAuthCredentials() + assert.Empty(t, creds.AuthType) + assert.Empty(t, creds.AccessToken.RefreshToken) +} + +func TestRefreshAccessTokenGenericFailure(t *testing.T) { + tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer tokenServer.Close() + + resolver := func(string, bool) (string, error) { + return tokenServer.URL, nil + } + profile := newTestProfile(t, profiles.AuthTypeAccessToken, "tok", "refresh", time.Now().Add(-time.Hour).Unix()) err := refreshAccessToken(t.Context(), profile, resolver) require.Error(t, err) require.ErrorIs(t, err, ErrRefreshFailed) + // Creds should NOT be wiped on transient failures. + creds := profile.GetAuthCredentials() + assert.Equal(t, "refresh", creds.AccessToken.RefreshToken) } func TestRefreshAccessTokenEmptyClientID(t *testing.T) { diff --git a/otdfctl/pkg/auth/token_source.go b/otdfctl/pkg/auth/token_source.go new file mode 100644 index 0000000000..67a462890d --- /dev/null +++ b/otdfctl/pkg/auth/token_source.go @@ -0,0 +1,148 @@ +package auth + +import ( + "context" + "errors" + "log/slog" + "sync" + "time" + + "github.com/opentdf/platform/otdfctl/pkg/profiles" + "github.com/opentdf/platform/otdfctl/pkg/utils" + "golang.org/x/oauth2" +) + +// profileTokenSource is an oauth2.TokenSource that transparently refreshes +// expired access tokens via the stored refresh token and persists the rotated +// credentials back to the profile. It is safe for concurrent use. +type profileTokenSource struct { + profile *profiles.OtdfctlProfileStore + resolve tokenEndpointResolver + + mu sync.Mutex + inner oauth2.TokenSource + cachedAccess string +} + +func newProfileTokenSource(profile *profiles.OtdfctlProfileStore) *profileTokenSource { + return &profileTokenSource{ + profile: profile, + resolve: getTokenEndpoint, + } +} + +// Token returns a valid access token, refreshing via the stored refresh token +// when needed and persisting rotated credentials to the profile. +func (p *profileTokenSource) Token() (*oauth2.Token, error) { + p.mu.Lock() + defer p.mu.Unlock() + + creds := p.profile.GetAuthCredentials() + if creds.AuthType != profiles.AuthTypeAccessToken { + return nil, ErrInvalidAuthType + } + + if p.inner == nil { + if err := p.rebuild(creds); err != nil { + return nil, err + } + } + + tok, err := p.inner.Token() + if err != nil { + if isInvalidGrant(err) { + _ = p.profile.SetAuthCredentials(profiles.AuthCredentials{}) + p.inner = nil + return nil, errors.Join(ErrRefreshTokenInvalid, err) + } + slog.Warn("token source refresh failed", slog.Any("error", err)) + return nil, err + } + + if tok.AccessToken != p.cachedAccess { + p.persist(creds, tok) + p.cachedAccess = tok.AccessToken + } + return tok, nil +} + +// Invalidate drops the cached inner source so the next Token() call rebuilds +// from the current profile creds and forces a refresh. Used by callers that +// know the server has rejected the current access token (e.g. a 401 retry). +func (p *profileTokenSource) Invalidate() { + p.mu.Lock() + defer p.mu.Unlock() + p.inner = nil +} + +// rebuild constructs the inner oauth2.TokenSource from the profile's creds. +// Caller must hold p.mu. +func (p *profileTokenSource) rebuild(creds profiles.AuthCredentials) error { + endpoint := p.profile.GetEndpoint() + tlsNoVerify := p.profile.GetTLSNoVerify() + + normalized, err := utils.NormalizeEndpoint(endpoint) + if err != nil { + return err + } + tokenEndpoint, err := p.resolve(normalized.String(), tlsNoVerify) + if err != nil { + return err + } + + clientID := creds.AccessToken.ClientID + if clientID == "" { + clientID = DefaultPublicClientID + } + + cfg := &oauth2.Config{ + ClientID: clientID, + Endpoint: oauth2.Endpoint{TokenURL: tokenEndpoint}, + } + + ctx := context.Background() + if tlsNoVerify { + ctx = context.WithValue(ctx, oauth2.HTTPClient, utils.NewHTTPClient(tlsNoVerify)) + } + + oldToken := buildToken(&creds) + p.inner = cfg.TokenSource(ctx, oldToken) + p.cachedAccess = oldToken.AccessToken + return nil +} + +// persist writes the rotated token back to the profile. Some IdPs omit +// refresh_token on refresh (RFC-compliant no-rotation); keep the old one in +// that case. A write-back failure is logged but not returned โ€” the fresh +// token is still valid for the in-flight call. +func (p *profileTokenSource) persist(oldCreds profiles.AuthCredentials, tok *oauth2.Token) { + refresh := tok.RefreshToken + if refresh == "" { + refresh = oldCreds.AccessToken.RefreshToken + } + expiry := tok.Expiry.Unix() + if tok.Expiry.IsZero() { + expiry = time.Now().Add(time.Hour).Unix() + slog.Warn("token response missing expires_in, assuming 1 hour") + } + + clientID := oldCreds.AccessToken.ClientID + if clientID == "" { + clientID = DefaultPublicClientID + } + + newCreds := profiles.AuthCredentials{ + AuthType: profiles.AuthTypeAccessToken, + AccessToken: profiles.AuthCredentialsAccessToken{ + ClientID: clientID, + AccessToken: tok.AccessToken, + RefreshToken: refresh, + Expiration: expiry, + }, + } + if err := p.profile.SetAuthCredentials(newCreds); err != nil { + slog.Warn("failed to persist refreshed credentials", slog.Any("error", err)) + return + } + slog.Info("access token refreshed", slog.String("profile", p.profile.Name())) +} diff --git a/otdfctl/pkg/auth/token_source_test.go b/otdfctl/pkg/auth/token_source_test.go new file mode 100644 index 0000000000..59374fd282 --- /dev/null +++ b/otdfctl/pkg/auth/token_source_test.go @@ -0,0 +1,206 @@ +package auth + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/opentdf/platform/otdfctl/pkg/profiles" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// newTokenSourceForTest builds a profileTokenSource with the resolver pointed +// at the given httptest server URL. +func newTokenSourceForTest(profile *profiles.OtdfctlProfileStore, tokenURL string) *profileTokenSource { + ts := newProfileTokenSource(profile) + ts.resolve = func(string, bool) (string, error) { return tokenURL, nil } + return ts +} + +func TestProfileTokenSource_ValidTokenNoRefresh(t *testing.T) { + var hits atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + hits.Add(1) + w.WriteHeader(http.StatusInternalServerError) + })) + defer server.Close() + + profile := newTestProfile(t, profiles.AuthTypeAccessToken, "current-access", "refresh", time.Now().Add(time.Hour).Unix()) + ts := newTokenSourceForTest(profile, server.URL) + + tok, err := ts.Token() + require.NoError(t, err) + assert.Equal(t, "current-access", tok.AccessToken) + assert.Equal(t, int32(0), hits.Load(), "no network call expected for a valid token") +} + +func TestProfileTokenSource_ExpiredRefreshesAndPersists(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + assert.NoError(t, json.NewEncoder(w).Encode(map[string]any{ + "access_token": "fresh-access", + "refresh_token": "rotated-refresh", + "token_type": "Bearer", + "expires_in": 3600, + })) + })) + defer server.Close() + + profile := newTestProfile(t, profiles.AuthTypeAccessToken, "stale", "old-refresh", time.Now().Add(-time.Hour).Unix()) + ts := newTokenSourceForTest(profile, server.URL) + + tok, err := ts.Token() + require.NoError(t, err) + assert.Equal(t, "fresh-access", tok.AccessToken) + + creds := profile.GetAuthCredentials() + assert.Equal(t, "fresh-access", creds.AccessToken.AccessToken) + assert.Equal(t, "rotated-refresh", creds.AccessToken.RefreshToken) + assert.Greater(t, creds.AccessToken.Expiration, time.Now().Unix()) +} + +func TestProfileTokenSource_NoRotationKeepsOldRefresh(t *testing.T) { + // RFC-compliant IdP that does not rotate refresh tokens omits the field. + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + assert.NoError(t, json.NewEncoder(w).Encode(map[string]any{ + "access_token": "fresh-access", + "token_type": "Bearer", + "expires_in": 3600, + })) + })) + defer server.Close() + + profile := newTestProfile(t, profiles.AuthTypeAccessToken, "stale", "keep-me", time.Now().Add(-time.Hour).Unix()) + ts := newTokenSourceForTest(profile, server.URL) + + _, err := ts.Token() + require.NoError(t, err) + + creds := profile.GetAuthCredentials() + assert.Equal(t, "keep-me", creds.AccessToken.RefreshToken) +} + +func TestProfileTokenSource_ConcurrentRefreshSingleFlights(t *testing.T) { + var hits atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + hits.Add(1) + w.Header().Set("Content-Type", "application/json") + assert.NoError(t, json.NewEncoder(w).Encode(map[string]any{ + "access_token": "shared-fresh", + "refresh_token": "shared-refresh", + "token_type": "Bearer", + "expires_in": 3600, + })) + })) + defer server.Close() + + profile := newTestProfile(t, profiles.AuthTypeAccessToken, "stale", "refresh", time.Now().Add(-time.Hour).Unix()) + ts := newTokenSourceForTest(profile, server.URL) + + const workers = 20 + var wg sync.WaitGroup + wg.Add(workers) + for range workers { + go func() { + defer wg.Done() + tok, err := ts.Token() + assert.NoError(t, err) + assert.Equal(t, "shared-fresh", tok.AccessToken) + }() + } + wg.Wait() + + assert.Equal(t, int32(1), hits.Load(), "concurrent Token() calls must single-flight the refresh") +} + +func TestProfileTokenSource_InvalidGrantClearsCreds(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusBadRequest) + assert.NoError(t, json.NewEncoder(w).Encode(map[string]string{"error": "invalid_grant"})) + })) + defer server.Close() + + profile := newTestProfile(t, profiles.AuthTypeAccessToken, "stale", "refresh", time.Now().Add(-time.Hour).Unix()) + ts := newTokenSourceForTest(profile, server.URL) + + _, err := ts.Token() + require.Error(t, err) + require.ErrorIs(t, err, ErrRefreshTokenInvalid) + + creds := profile.GetAuthCredentials() + assert.Empty(t, creds.AuthType) + assert.Empty(t, creds.AccessToken.RefreshToken) +} + +func TestProfileTokenSource_InvalidateForcesRefresh(t *testing.T) { + var hits atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + hits.Add(1) + w.Header().Set("Content-Type", "application/json") + assert.NoError(t, json.NewEncoder(w).Encode(map[string]any{ + "access_token": "post-invalidate", + "refresh_token": "post-invalidate-refresh", + "token_type": "Bearer", + "expires_in": 3600, + })) + })) + defer server.Close() + + // Start with a valid, non-expired token so the first Token() does not refresh. + profile := newTestProfile(t, profiles.AuthTypeAccessToken, "current", "refresh", time.Now().Add(time.Hour).Unix()) + ts := newTokenSourceForTest(profile, server.URL) + + tok, err := ts.Token() + require.NoError(t, err) + assert.Equal(t, "current", tok.AccessToken) + require.Equal(t, int32(0), hits.Load()) + + // Server has revoked the token; Invalidate() forces the next call to refresh. + ts.Invalidate() + // Zero the stored expiry so the underlying oauth2 source sees the token as invalid. + creds := profile.GetAuthCredentials() + creds.AccessToken.Expiration = time.Now().Add(-time.Hour).Unix() + require.NoError(t, profile.SetAuthCredentials(creds)) + + tok, err = ts.Token() + require.NoError(t, err) + assert.Equal(t, "post-invalidate", tok.AccessToken) + assert.Equal(t, int32(1), hits.Load()) +} + +func TestProfileTokenSource_ZeroExpiryFallback(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + assert.NoError(t, json.NewEncoder(w).Encode(map[string]any{ + "access_token": "fresh", + "refresh_token": "fresh-refresh", + "token_type": "Bearer", + })) + })) + defer server.Close() + + profile := newTestProfile(t, profiles.AuthTypeAccessToken, "stale", "refresh", time.Now().Add(-time.Hour).Unix()) + ts := newTokenSourceForTest(profile, server.URL) + + _, err := ts.Token() + require.NoError(t, err) + + creds := profile.GetAuthCredentials() + assert.Greater(t, creds.AccessToken.Expiration, time.Now().Unix(), + "zero expiry in response should fall back to ~1 hour") +} + +func TestProfileTokenSource_WrongAuthType(t *testing.T) { + profile := newTestProfile(t, profiles.AuthTypeClientCredentials, "tok", "refresh", time.Now().Add(time.Hour).Unix()) + ts := newTokenSourceForTest(profile, "http://ignored") + + _, err := ts.Token() + require.ErrorIs(t, err, ErrInvalidAuthType) +} From 3bc86972956e6aed559beb475ac216df9422302b Mon Sep 17 00:00:00 2001 From: Elizabeth Healy Date: Tue, 4 Aug 2026 10:16:58 -0400 Subject: [PATCH 2/3] remove log --- otdfctl/cmd/auth/login.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/otdfctl/cmd/auth/login.go b/otdfctl/cmd/auth/login.go index 8b07959cb9..e37ba35b93 100644 --- a/otdfctl/cmd/auth/login.go +++ b/otdfctl/cmd/auth/login.go @@ -2,7 +2,6 @@ package auth import ( "fmt" - "log/slog" "github.com/opentdf/platform/otdfctl/cmd/common" "github.com/opentdf/platform/otdfctl/pkg/auth" @@ -41,13 +40,6 @@ func codeLogin(cmd *cobra.Command, args []string) { c.ExitWithError("failed to set auth credentials", err) } - // The profile store is filesystem-backed (see common.InitProfile). Warn the - // user that a refresh token has been written to disk and recommend the OS - // keychain when higher assurance is required. - if tok.RefreshToken != "" { - slog.Warn("refresh token stored in filesystem-backed profile; OS keychain is recommended for higher assurance") - } - c.ExitWithMessage(fmt.Sprintf("Code login complete for profile: [%s]", cp.Name()), cli.ExitCodeSuccess) } From 6906def58af02045dff84fb3d503a3540e6a2692 Mon Sep 17 00:00:00 2001 From: Elizabeth Healy Date: Tue, 4 Aug 2026 10:49:47 -0400 Subject: [PATCH 3/3] address pr comments --- otdfctl/pkg/auth/auth.go | 5 ++- otdfctl/pkg/auth/refresh.go | 8 ++++- otdfctl/pkg/auth/refresh_test.go | 52 +++++++++++++++++++++++++++ otdfctl/pkg/auth/token_source.go | 17 ++++++--- otdfctl/pkg/auth/token_source_test.go | 7 ++-- 5 files changed, 78 insertions(+), 11 deletions(-) diff --git a/otdfctl/pkg/auth/auth.go b/otdfctl/pkg/auth/auth.go index bc24d120e7..d055afe243 100644 --- a/otdfctl/pkg/auth/auth.go +++ b/otdfctl/pkg/auth/auth.go @@ -214,7 +214,10 @@ func GetTokenWithProfile(ctx context.Context, profile *profiles.OtdfctlProfileSt case profiles.AuthTypeClientCredentials: return GetTokenWithClientCreds(ctx, profile.GetEndpoint(), c.ClientID, c.ClientSecret, profile.GetTLSNoVerify(), c.Scopes) case profiles.AuthTypeAccessToken: - if !buildToken(&c).Valid() && HasRefreshToken(profile) { + if !buildToken(&c).Valid() { + if !HasRefreshToken(profile) { + return nil, ErrAccessTokenExpired + } if err := RefreshAccessToken(ctx, profile); err != nil { return nil, err } diff --git a/otdfctl/pkg/auth/refresh.go b/otdfctl/pkg/auth/refresh.go index 3e446f2e28..46df71600b 100644 --- a/otdfctl/pkg/auth/refresh.go +++ b/otdfctl/pkg/auth/refresh.go @@ -99,12 +99,18 @@ func refreshAccessToken(ctx context.Context, profile *profiles.OtdfctlProfileSto slog.Warn("token response missing expires_in, assuming 1 hour") } + // Some IdPs omit refresh_token on refresh (no-rotation); keep the old one. + refreshToken := newToken.RefreshToken + if refreshToken == "" { + refreshToken = creds.AccessToken.RefreshToken + } + newCreds := profiles.AuthCredentials{ AuthType: profiles.AuthTypeAccessToken, AccessToken: profiles.AuthCredentialsAccessToken{ ClientID: clientID, AccessToken: newToken.AccessToken, - RefreshToken: newToken.RefreshToken, + RefreshToken: refreshToken, Expiration: expiration, }, } diff --git a/otdfctl/pkg/auth/refresh_test.go b/otdfctl/pkg/auth/refresh_test.go index a08427e6cf..0666ed05ab 100644 --- a/otdfctl/pkg/auth/refresh_test.go +++ b/otdfctl/pkg/auth/refresh_test.go @@ -232,6 +232,58 @@ func TestRefreshAccessTokenGenericFailure(t *testing.T) { assert.Equal(t, "refresh", creds.AccessToken.RefreshToken) } +func TestRefreshAccessTokenNoRotationKeepsOldRefreshToken(t *testing.T) { + tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + err := json.NewEncoder(w).Encode(map[string]any{ + "access_token": "new-access", + "token_type": "Bearer", + "expires_in": 3600, + }) + assert.NoError(t, err) + })) + defer tokenServer.Close() + + resolver := func(string, bool) (string, error) { + return tokenServer.URL, nil + } + + profile := newTestProfile(t, profiles.AuthTypeAccessToken, "old-access", "original-refresh", time.Now().Add(-time.Hour).Unix()) + err := refreshAccessToken(t.Context(), profile, resolver) + require.NoError(t, err) + + creds := profile.GetAuthCredentials() + assert.Equal(t, "new-access", creds.AccessToken.AccessToken) + assert.Equal(t, "original-refresh", creds.AccessToken.RefreshToken, + "when IdP omits refresh_token, the existing one must be preserved") +} + +func TestRefreshAccessTokenRotationStoresNewRefreshToken(t *testing.T) { + tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + err := json.NewEncoder(w).Encode(map[string]any{ + "access_token": "new-access", + "refresh_token": "rotated-refresh", + "token_type": "Bearer", + "expires_in": 3600, + }) + assert.NoError(t, err) + })) + defer tokenServer.Close() + + resolver := func(string, bool) (string, error) { + return tokenServer.URL, nil + } + + profile := newTestProfile(t, profiles.AuthTypeAccessToken, "old-access", "original-refresh", time.Now().Add(-time.Hour).Unix()) + err := refreshAccessToken(t.Context(), profile, resolver) + require.NoError(t, err) + + creds := profile.GetAuthCredentials() + assert.Equal(t, "rotated-refresh", creds.AccessToken.RefreshToken, + "when IdP returns a new refresh_token, it must replace the old one") +} + func TestRefreshAccessTokenEmptyClientID(t *testing.T) { tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") diff --git a/otdfctl/pkg/auth/token_source.go b/otdfctl/pkg/auth/token_source.go index 67a462890d..1d76a1948a 100644 --- a/otdfctl/pkg/auth/token_source.go +++ b/otdfctl/pkg/auth/token_source.go @@ -22,6 +22,7 @@ type profileTokenSource struct { mu sync.Mutex inner oauth2.TokenSource cachedAccess string + forceRefresh bool } func newProfileTokenSource(profile *profiles.OtdfctlProfileStore) *profileTokenSource { @@ -60,8 +61,9 @@ func (p *profileTokenSource) Token() (*oauth2.Token, error) { } if tok.AccessToken != p.cachedAccess { - p.persist(creds, tok) - p.cachedAccess = tok.AccessToken + if p.persist(creds, tok) { + p.cachedAccess = tok.AccessToken + } } return tok, nil } @@ -73,6 +75,7 @@ func (p *profileTokenSource) Invalidate() { p.mu.Lock() defer p.mu.Unlock() p.inner = nil + p.forceRefresh = true } // rebuild constructs the inner oauth2.TokenSource from the profile's creds. @@ -106,6 +109,11 @@ func (p *profileTokenSource) rebuild(creds profiles.AuthCredentials) error { } oldToken := buildToken(&creds) + if p.forceRefresh { + oldToken.AccessToken = "" + oldToken.Expiry = time.Time{} + p.forceRefresh = false + } p.inner = cfg.TokenSource(ctx, oldToken) p.cachedAccess = oldToken.AccessToken return nil @@ -115,7 +123,7 @@ func (p *profileTokenSource) rebuild(creds profiles.AuthCredentials) error { // refresh_token on refresh (RFC-compliant no-rotation); keep the old one in // that case. A write-back failure is logged but not returned โ€” the fresh // token is still valid for the in-flight call. -func (p *profileTokenSource) persist(oldCreds profiles.AuthCredentials, tok *oauth2.Token) { +func (p *profileTokenSource) persist(oldCreds profiles.AuthCredentials, tok *oauth2.Token) bool { refresh := tok.RefreshToken if refresh == "" { refresh = oldCreds.AccessToken.RefreshToken @@ -142,7 +150,8 @@ func (p *profileTokenSource) persist(oldCreds profiles.AuthCredentials, tok *oau } if err := p.profile.SetAuthCredentials(newCreds); err != nil { slog.Warn("failed to persist refreshed credentials", slog.Any("error", err)) - return + return false } slog.Info("access token refreshed", slog.String("profile", p.profile.Name())) + return true } diff --git a/otdfctl/pkg/auth/token_source_test.go b/otdfctl/pkg/auth/token_source_test.go index 59374fd282..59bfa0f2a4 100644 --- a/otdfctl/pkg/auth/token_source_test.go +++ b/otdfctl/pkg/auth/token_source_test.go @@ -162,12 +162,9 @@ func TestProfileTokenSource_InvalidateForcesRefresh(t *testing.T) { assert.Equal(t, "current", tok.AccessToken) require.Equal(t, int32(0), hits.Load()) - // Server has revoked the token; Invalidate() forces the next call to refresh. + // Server has revoked the token; Invalidate() forces the next call to refresh + // even though the stored token hasn't expired yet. ts.Invalidate() - // Zero the stored expiry so the underlying oauth2 source sees the token as invalid. - creds := profile.GetAuthCredentials() - creds.AccessToken.Expiration = time.Now().Add(-time.Hour).Unix() - require.NoError(t, profile.SetAuthCredentials(creds)) tok, err = ts.Token() require.NoError(t, err)