From de7d02e6b8c6d199355fd286ce4fb8a47f663758 Mon Sep 17 00:00:00 2001 From: Krish Suchak Date: Tue, 12 May 2026 15:28:06 -0400 Subject: [PATCH 1/7] refresh token and client creds positional args --- otdfctl/pkg/auth/errors.go | 2 + otdfctl/pkg/auth/refresh.go | 118 ++++++++++++++ otdfctl/pkg/auth/refresh_test.go | 265 +++++++++++++++++++++++++++++++ otdfctl/pkg/man/man.go | 14 +- otdfctl/pkg/man/man_test.go | 116 ++++++++++++++ 5 files changed, 514 insertions(+), 1 deletion(-) create mode 100644 otdfctl/pkg/auth/refresh.go create mode 100644 otdfctl/pkg/auth/refresh_test.go create mode 100644 otdfctl/pkg/man/man_test.go diff --git a/otdfctl/pkg/auth/errors.go b/otdfctl/pkg/auth/errors.go index c26dea08c2..b074779795 100644 --- a/otdfctl/pkg/auth/errors.go +++ b/otdfctl/pkg/auth/errors.go @@ -10,4 +10,6 @@ var ( ErrUnauthenticated = errors.New("not logged in") ErrParsingAccessToken = errors.New("failed to parse access token") ErrProfileCredentialsNotFound = errors.New("profile missing credentials") + ErrNoRefreshToken = errors.New("no refresh token available") + ErrRefreshFailed = errors.New("token refresh failed") ) diff --git a/otdfctl/pkg/auth/refresh.go b/otdfctl/pkg/auth/refresh.go new file mode 100644 index 0000000000..1041472364 --- /dev/null +++ b/otdfctl/pkg/auth/refresh.go @@ -0,0 +1,118 @@ +package auth + +import ( + "context" + "fmt" + "log/slog" + "time" + + "github.com/opentdf/platform/otdfctl/pkg/profiles" + "github.com/opentdf/platform/otdfctl/pkg/utils" + "golang.org/x/oauth2" +) + +const ( + DefaultPublicClientID = "cli-client" +) + +// Function variable for the token endpoint lookup — swappable in tests. +var getTokenEndpointFunc = getTokenEndpoint + +// RefreshAccessToken refreshes the access token using the stored refresh token +// and updates the profile with the new tokens. +func RefreshAccessToken(ctx context.Context, profile *profiles.OtdfctlProfileStore) error { + creds := profile.GetAuthCredentials() + + if creds.AuthType != profiles.AuthTypeAccessToken { + return fmt.Errorf("cannot refresh token: auth type is %s, not access-token", creds.AuthType) + } + + if creds.AccessToken.RefreshToken == "" { + return ErrNoRefreshToken + } + + endpoint := profile.GetEndpoint() + tlsNoVerify := profile.GetTLSNoVerify() + + normalized, err := utils.NormalizeEndpoint(endpoint) + if err != nil { + return fmt.Errorf("failed to normalize endpoint: %w", err) + } + + tokenEndpoint, err := getTokenEndpointFunc(normalized.String(), tlsNoVerify) + if err != nil { + return fmt.Errorf("failed to get token endpoint: %w", err) + } + + clientID := creds.AccessToken.ClientID + if clientID == "" { + clientID = DefaultPublicClientID + } + + oauth2Config := &oauth2.Config{ + ClientID: clientID, + Endpoint: oauth2.Endpoint{ + TokenURL: tokenEndpoint, + }, + } + + oldToken := &oauth2.Token{ + AccessToken: creds.AccessToken.AccessToken, + RefreshToken: creds.AccessToken.RefreshToken, + Expiry: time.Unix(creds.AccessToken.Expiration, 0), + } + + if tlsNoVerify { + httpClient := utils.NewHTTPClient(tlsNoVerify) + ctx = context.WithValue(ctx, oauth2.HTTPClient, httpClient) + } + + tokenSource := oauth2Config.TokenSource(ctx, oldToken) + newToken, err := tokenSource.Token() + if err != nil { + return fmt.Errorf("%w: %w", ErrRefreshFailed, err) + } + + slog.Debug("successfully refreshed access token") + + newCreds := profiles.AuthCredentials{ + AuthType: profiles.AuthTypeAccessToken, + AccessToken: profiles.AuthCredentialsAccessToken{ + ClientID: clientID, + AccessToken: newToken.AccessToken, + RefreshToken: newToken.RefreshToken, + Expiration: newToken.Expiry.Unix(), + }, + } + + if err := profile.SetAuthCredentials(newCreds); err != nil { + return fmt.Errorf("failed to save refreshed credentials: %w", err) + } + + slog.Info("access token refreshed and saved") + return nil +} + +// IsTokenExpired checks if the access token in the profile is expired. +func IsTokenExpired(profile *profiles.OtdfctlProfileStore) bool { + creds := profile.GetAuthCredentials() + if creds.AuthType != profiles.AuthTypeAccessToken { + return false + } + expiry := time.Unix(creds.AccessToken.Expiration, 0) + return time.Now().After(expiry) +} + +// HasRefreshToken checks if the profile has a refresh token. +func HasRefreshToken(profile *profiles.OtdfctlProfileStore) bool { + creds := profile.GetAuthCredentials() + return creds.AuthType == profiles.AuthTypeAccessToken && creds.AccessToken.RefreshToken != "" +} + +func getTokenEndpoint(endpoint string, tlsNoVerify bool) (string, error) { + pc, err := getPlatformConfiguration(endpoint, tlsNoVerify) + if err != nil { + return "", fmt.Errorf("failed to get platform configuration: %w", err) + } + return pc.tokenEndpoint, nil +} diff --git a/otdfctl/pkg/auth/refresh_test.go b/otdfctl/pkg/auth/refresh_test.go new file mode 100644 index 0000000000..b406b6aa9d --- /dev/null +++ b/otdfctl/pkg/auth/refresh_test.go @@ -0,0 +1,265 @@ +package auth + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/opentdf/platform/otdfctl/pkg/profiles" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func newTestProfile(t *testing.T, authType string, accessToken, refreshToken string, expiration int64) *profiles.OtdfctlProfileStore { + t.Helper() + cfg := &profiles.ProfileConfig{ + Name: "test", + Endpoint: "https://example.com", + TLSNoVerify: false, + } + store, err := profiles.NewOtdfctlProfileStore(profiles.ProfileDriverMemory, cfg, false) + require.NoError(t, err) + err = store.SetAuthCredentials(profiles.AuthCredentials{ + AuthType: authType, + AccessToken: profiles.AuthCredentialsAccessToken{ + ClientID: "cli-client", + AccessToken: accessToken, + RefreshToken: refreshToken, + Expiration: expiration, + }, + }) + require.NoError(t, err) + return store +} + +func TestIsTokenExpired(t *testing.T) { + tests := []struct { + name string + authType string + exp int64 + want bool + }{ + { + name: "expired token", + authType: profiles.AuthTypeAccessToken, + exp: time.Now().Add(-time.Hour).Unix(), + want: true, + }, + { + name: "valid token", + authType: profiles.AuthTypeAccessToken, + exp: time.Now().Add(time.Hour).Unix(), + want: false, + }, + { + name: "non-access-token auth type", + authType: profiles.AuthTypeClientCredentials, + exp: time.Now().Add(-time.Hour).Unix(), + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + profile := newTestProfile(t, tt.authType, "tok", "refresh", tt.exp) + got := IsTokenExpired(profile) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestHasRefreshToken(t *testing.T) { + tests := []struct { + name string + authType string + refreshToken string + want bool + }{ + { + name: "has refresh token", + authType: profiles.AuthTypeAccessToken, + refreshToken: "refresh-tok", + want: true, + }, + { + name: "no refresh token", + authType: profiles.AuthTypeAccessToken, + refreshToken: "", + want: false, + }, + { + name: "wrong auth type", + authType: profiles.AuthTypeClientCredentials, + refreshToken: "refresh-tok", + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + profile := newTestProfile(t, tt.authType, "tok", tt.refreshToken, time.Now().Add(time.Hour).Unix()) + got := HasRefreshToken(profile) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestRefreshAccessTokenWrongAuthType(t *testing.T) { + profile := newTestProfile(t, profiles.AuthTypeClientCredentials, "tok", "refresh", time.Now().Add(time.Hour).Unix()) + err := RefreshAccessToken(context.Background(), profile) + require.Error(t, err) +} + +func TestRefreshAccessTokenNoRefreshToken(t *testing.T) { + profile := newTestProfile(t, profiles.AuthTypeAccessToken, "tok", "", time.Now().Add(-time.Hour).Unix()) + err := RefreshAccessToken(context.Background(), profile) + require.ErrorIs(t, err, ErrNoRefreshToken) +} + +func TestRefreshAccessTokenSuccess(t *testing.T) { + tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(map[string]any{ + "access_token": "new-access-token", + "refresh_token": "new-refresh-token", + "token_type": "Bearer", + "expires_in": 3600, + }) + })) + defer tokenServer.Close() + + origFunc := getTokenEndpointFunc + getTokenEndpointFunc = func(string, bool) (string, error) { + return tokenServer.URL, nil + } + defer func() { getTokenEndpointFunc = origFunc }() + + profile := newTestProfile(t, profiles.AuthTypeAccessToken, "old-token", "old-refresh", time.Now().Add(-time.Hour).Unix()) + + err := RefreshAccessToken(context.Background(), profile) + require.NoError(t, err) + + creds := profile.GetAuthCredentials() + assert.Equal(t, "new-access-token", creds.AccessToken.AccessToken) + assert.Equal(t, "new-refresh-token", creds.AccessToken.RefreshToken) +} + +func TestRefreshAccessTokenEndpointError(t *testing.T) { + origFunc := getTokenEndpointFunc + getTokenEndpointFunc = func(string, bool) (string, error) { + return "", errors.New("gRPC connection failed") + } + defer func() { getTokenEndpointFunc = origFunc }() + + profile := newTestProfile(t, profiles.AuthTypeAccessToken, "tok", "refresh", time.Now().Add(-time.Hour).Unix()) + err := RefreshAccessToken(context.Background(), profile) + require.Error(t, err) +} + +func TestRefreshAccessTokenRefreshFails(t *testing.T) { + tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + json.NewEncoder(w).Encode(map[string]string{"error": "invalid_grant"}) + })) + defer tokenServer.Close() + + origFunc := getTokenEndpointFunc + getTokenEndpointFunc = func(string, bool) (string, error) { + return tokenServer.URL, nil + } + defer func() { getTokenEndpointFunc = origFunc }() + + profile := newTestProfile(t, profiles.AuthTypeAccessToken, "tok", "refresh", time.Now().Add(-time.Hour).Unix()) + err := RefreshAccessToken(context.Background(), profile) + require.Error(t, err) + require.ErrorIs(t, err, ErrRefreshFailed) +} + +func TestRefreshAccessTokenEmptyClientID(t *testing.T) { + tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(map[string]any{ + "access_token": "new-token", + "refresh_token": "new-refresh", + "token_type": "Bearer", + "expires_in": 3600, + }) + })) + defer tokenServer.Close() + + origFunc := getTokenEndpointFunc + getTokenEndpointFunc = func(string, bool) (string, error) { + return tokenServer.URL, nil + } + defer func() { getTokenEndpointFunc = origFunc }() + + cfg := &profiles.ProfileConfig{ + Name: "test", + Endpoint: "https://example.com", + } + profile, err := profiles.NewOtdfctlProfileStore(profiles.ProfileDriverMemory, cfg, false) + require.NoError(t, err) + err = profile.SetAuthCredentials(profiles.AuthCredentials{ + AuthType: profiles.AuthTypeAccessToken, + AccessToken: profiles.AuthCredentialsAccessToken{ + ClientID: "", + AccessToken: "old-token", + RefreshToken: "old-refresh", + Expiration: time.Now().Add(-time.Hour).Unix(), + }, + }) + require.NoError(t, err) + + err = RefreshAccessToken(context.Background(), profile) + require.NoError(t, err) + + creds := profile.GetAuthCredentials() + assert.Equal(t, DefaultPublicClientID, creds.AccessToken.ClientID) +} + +func TestRefreshAccessTokenTLSNoVerify(t *testing.T) { + tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(map[string]any{ + "access_token": "tls-token", + "refresh_token": "tls-refresh", + "token_type": "Bearer", + "expires_in": 3600, + }) + })) + defer tokenServer.Close() + + origFunc := getTokenEndpointFunc + getTokenEndpointFunc = func(string, bool) (string, error) { + return tokenServer.URL, nil + } + defer func() { getTokenEndpointFunc = origFunc }() + + cfg := &profiles.ProfileConfig{ + Name: "test", + Endpoint: "https://example.com", + TLSNoVerify: true, + } + profile, err := profiles.NewOtdfctlProfileStore(profiles.ProfileDriverMemory, cfg, false) + require.NoError(t, err) + err = profile.SetAuthCredentials(profiles.AuthCredentials{ + AuthType: profiles.AuthTypeAccessToken, + AccessToken: profiles.AuthCredentialsAccessToken{ + ClientID: "cli-client", + AccessToken: "old-token", + RefreshToken: "old-refresh", + Expiration: time.Now().Add(-time.Hour).Unix(), + }, + }) + require.NoError(t, err) + + err = RefreshAccessToken(context.Background(), profile) + require.NoError(t, err) + + creds := profile.GetAuthCredentials() + assert.Equal(t, "tls-token", creds.AccessToken.AccessToken) +} diff --git a/otdfctl/pkg/man/man.go b/otdfctl/pkg/man/man.go index c6d36c53b3..5b555c7059 100644 --- a/otdfctl/pkg/man/man.go +++ b/otdfctl/pkg/man/man.go @@ -245,7 +245,7 @@ func ProcessDoc(doc string) (*Doc, error) { d := Doc{ cobra.Command{ - Use: c.Name, + Use: buildUseString(c.Name, c.Args, c.ArbitraryArgs), Args: args, Hidden: c.Hidden, Aliases: c.Aliases, @@ -258,3 +258,15 @@ func ProcessDoc(doc string) (*Doc, error) { return &d, nil } + +func buildUseString(name string, args, arbitraryArgs []string) string { + parts := make([]string, 0, 1+len(args)+len(arbitraryArgs)) + parts = append(parts, name) + for _, a := range args { + parts = append(parts, "<"+a+">") + } + for _, a := range arbitraryArgs { + parts = append(parts, "["+a+"]") + } + return strings.Join(parts, " ") +} diff --git a/otdfctl/pkg/man/man_test.go b/otdfctl/pkg/man/man_test.go new file mode 100644 index 0000000000..cc8c28a531 --- /dev/null +++ b/otdfctl/pkg/man/man_test.go @@ -0,0 +1,116 @@ +package man + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestProcessDocNoArgs(t *testing.T) { + doc, err := ProcessDoc(`--- +title: List namespaces +command: + name: list +--- + +List all namespaces. +`) + require.NoError(t, err) + assert.Equal(t, "list", doc.Use) +} + +func TestProcessDocWithArgs(t *testing.T) { + doc, err := ProcessDoc(`--- +title: Get a resource +command: + name: get + arguments: + - resource-id +--- + +Get a resource by ID. +`) + require.NoError(t, err) + assert.Equal(t, "get ", doc.Use) +} + +func TestProcessDocWithArbitraryArgs(t *testing.T) { + doc, err := ProcessDoc(`--- +title: Do something +command: + name: do + arbitraryArgs: + - optional-arg +--- + +Do something optionally. +`) + require.NoError(t, err) + assert.Equal(t, "do [optional-arg]", doc.Use) +} + +func TestProcessDocWithBothArgTypes(t *testing.T) { + doc, err := ProcessDoc(`--- +title: Authenticate with client credentials +command: + name: client-credentials + arguments: + - client-id + arbitraryArgs: + - client-secret +--- + +Authenticate via client credentials flow. +`) + require.NoError(t, err) + assert.Equal(t, "client-credentials [client-secret]", doc.Use) +} + +func TestBuildUseString(t *testing.T) { + tests := []struct { + name string + cmdName string + args []string + arbitraryArgs []string + want string + }{ + { + name: "name only", + cmdName: "list", + want: "list", + }, + { + name: "with required args", + cmdName: "get", + args: []string{"id"}, + want: "get ", + }, + { + name: "with optional args", + cmdName: "run", + arbitraryArgs: []string{"extra"}, + want: "run [extra]", + }, + { + name: "with both", + cmdName: "auth", + args: []string{"client-id"}, + arbitraryArgs: []string{"client-secret"}, + want: "auth [client-secret]", + }, + { + name: "multiple required", + cmdName: "copy", + args: []string{"src", "dst"}, + want: "copy ", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := buildUseString(tt.cmdName, tt.args, tt.arbitraryArgs) + assert.Equal(t, tt.want, got) + }) + } +} From e15266e02eac60217493de40c93c2c34ef94e441 Mon Sep 17 00:00:00 2001 From: Krish Suchak Date: Tue, 12 May 2026 15:43:45 -0400 Subject: [PATCH 2/7] more tests --- otdfctl/pkg/auth/refresh_test.go | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/otdfctl/pkg/auth/refresh_test.go b/otdfctl/pkg/auth/refresh_test.go index b406b6aa9d..5f2c976753 100644 --- a/otdfctl/pkg/auth/refresh_test.go +++ b/otdfctl/pkg/auth/refresh_test.go @@ -263,3 +263,31 @@ func TestRefreshAccessTokenTLSNoVerify(t *testing.T) { creds := profile.GetAuthCredentials() assert.Equal(t, "tls-token", creds.AccessToken.AccessToken) } + +func TestGetTokenEndpointFuncSwappable(t *testing.T) { + origFunc := getTokenEndpointFunc + defer func() { getTokenEndpointFunc = origFunc }() + + called := false + getTokenEndpointFunc = func(endpoint string, tlsNoVerify bool) (string, error) { + called = true + assert.Equal(t, "https://example.com:443", endpoint) + assert.False(t, tlsNoVerify) + return "https://idp.example.com/token", nil + } + + profile := newTestProfile(t, profiles.AuthTypeAccessToken, "tok", "refresh", time.Now().Add(-time.Hour).Unix()) + _ = RefreshAccessToken(context.Background(), profile) + assert.True(t, called, "getTokenEndpointFunc should have been called") +} + +func TestGetTokenEndpointBadEndpoint(t *testing.T) { + _, err := getTokenEndpoint("https://localhost:1", false) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to get platform configuration") +} + +func TestGetTokenEndpointEmptyEndpoint(t *testing.T) { + _, err := getTokenEndpoint("", false) + require.Error(t, err) +} From 60b8fc85bf0b821754502807220577bac265eb1c Mon Sep 17 00:00:00 2001 From: Krish Suchak Date: Wed, 13 May 2026 11:12:59 -0400 Subject: [PATCH 3/7] copilot suggestions --- otdfctl/pkg/auth/refresh.go | 16 +++++++++++++--- otdfctl/pkg/auth/refresh_test.go | 31 +++++++++++++++++++++++++------ otdfctl/pkg/man/man.go | 8 +++++--- otdfctl/pkg/man/man_test.go | 19 +++++++++++++++++++ 4 files changed, 62 insertions(+), 12 deletions(-) diff --git a/otdfctl/pkg/auth/refresh.go b/otdfctl/pkg/auth/refresh.go index 1041472364..2fc4b4988f 100644 --- a/otdfctl/pkg/auth/refresh.go +++ b/otdfctl/pkg/auth/refresh.go @@ -2,6 +2,7 @@ package auth import ( "context" + "errors" "fmt" "log/slog" "time" @@ -21,10 +22,14 @@ var getTokenEndpointFunc = getTokenEndpoint // RefreshAccessToken refreshes the access token using the stored refresh token // and updates the profile with the new tokens. func RefreshAccessToken(ctx context.Context, profile *profiles.OtdfctlProfileStore) error { + if profile == nil { + return errors.New("profile is required") + } + creds := profile.GetAuthCredentials() if creds.AuthType != profiles.AuthTypeAccessToken { - return fmt.Errorf("cannot refresh token: auth type is %s, not access-token", creds.AuthType) + return fmt.Errorf("%w: auth type is %s, not access-token", ErrInvalidAuthType, creds.AuthType) } if creds.AccessToken.RefreshToken == "" { @@ -57,9 +62,7 @@ func RefreshAccessToken(ctx context.Context, profile *profiles.OtdfctlProfileSto } oldToken := &oauth2.Token{ - AccessToken: creds.AccessToken.AccessToken, RefreshToken: creds.AccessToken.RefreshToken, - Expiry: time.Unix(creds.AccessToken.Expiration, 0), } if tlsNoVerify { @@ -94,7 +97,11 @@ func RefreshAccessToken(ctx context.Context, profile *profiles.OtdfctlProfileSto } // IsTokenExpired checks if the access token in the profile is expired. +// Returns false for non-access-token auth types since refresh only applies there. func IsTokenExpired(profile *profiles.OtdfctlProfileStore) bool { + if profile == nil { + return true + } creds := profile.GetAuthCredentials() if creds.AuthType != profiles.AuthTypeAccessToken { return false @@ -105,6 +112,9 @@ func IsTokenExpired(profile *profiles.OtdfctlProfileStore) bool { // HasRefreshToken checks if the profile has a refresh token. func HasRefreshToken(profile *profiles.OtdfctlProfileStore) bool { + if profile == nil { + return false + } creds := profile.GetAuthCredentials() return creds.AuthType == profiles.AuthTypeAccessToken && creds.AccessToken.RefreshToken != "" } diff --git a/otdfctl/pkg/auth/refresh_test.go b/otdfctl/pkg/auth/refresh_test.go index 5f2c976753..8bd2d32b98 100644 --- a/otdfctl/pkg/auth/refresh_test.go +++ b/otdfctl/pkg/auth/refresh_test.go @@ -111,7 +111,7 @@ func TestHasRefreshToken(t *testing.T) { func TestRefreshAccessTokenWrongAuthType(t *testing.T) { profile := newTestProfile(t, profiles.AuthTypeClientCredentials, "tok", "refresh", time.Now().Add(time.Hour).Unix()) err := RefreshAccessToken(context.Background(), profile) - require.Error(t, err) + require.ErrorIs(t, err, ErrInvalidAuthType) } func TestRefreshAccessTokenNoRefreshToken(t *testing.T) { @@ -123,12 +123,13 @@ func TestRefreshAccessTokenNoRefreshToken(t *testing.T) { func TestRefreshAccessTokenSuccess(t *testing.T) { tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(map[string]any{ + err := json.NewEncoder(w).Encode(map[string]any{ "access_token": "new-access-token", "refresh_token": "new-refresh-token", "token_type": "Bearer", "expires_in": 3600, }) + assert.NoError(t, err) })) defer tokenServer.Close() @@ -146,6 +147,8 @@ func TestRefreshAccessTokenSuccess(t *testing.T) { creds := profile.GetAuthCredentials() assert.Equal(t, "new-access-token", creds.AccessToken.AccessToken) assert.Equal(t, "new-refresh-token", creds.AccessToken.RefreshToken) + assert.Greater(t, creds.AccessToken.Expiration, time.Now().Unix(), + "expiration should be updated to a future timestamp") } func TestRefreshAccessTokenEndpointError(t *testing.T) { @@ -157,13 +160,14 @@ func TestRefreshAccessTokenEndpointError(t *testing.T) { profile := newTestProfile(t, profiles.AuthTypeAccessToken, "tok", "refresh", time.Now().Add(-time.Hour).Unix()) err := RefreshAccessToken(context.Background(), profile) - require.Error(t, err) + require.ErrorContains(t, err, "failed to get token endpoint") } func TestRefreshAccessTokenRefreshFails(t *testing.T) { tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusUnauthorized) - json.NewEncoder(w).Encode(map[string]string{"error": "invalid_grant"}) + err := json.NewEncoder(w).Encode(map[string]string{"error": "invalid_grant"}) + assert.NoError(t, err) })) defer tokenServer.Close() @@ -182,12 +186,13 @@ func TestRefreshAccessTokenRefreshFails(t *testing.T) { func TestRefreshAccessTokenEmptyClientID(t *testing.T) { tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(map[string]any{ + err := json.NewEncoder(w).Encode(map[string]any{ "access_token": "new-token", "refresh_token": "new-refresh", "token_type": "Bearer", "expires_in": 3600, }) + assert.NoError(t, err) })) defer tokenServer.Close() @@ -224,12 +229,13 @@ func TestRefreshAccessTokenEmptyClientID(t *testing.T) { func TestRefreshAccessTokenTLSNoVerify(t *testing.T) { tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(map[string]any{ + err := json.NewEncoder(w).Encode(map[string]any{ "access_token": "tls-token", "refresh_token": "tls-refresh", "token_type": "Bearer", "expires_in": 3600, }) + assert.NoError(t, err) })) defer tokenServer.Close() @@ -291,3 +297,16 @@ func TestGetTokenEndpointEmptyEndpoint(t *testing.T) { _, err := getTokenEndpoint("", false) require.Error(t, err) } + +func TestRefreshAccessTokenNilProfile(t *testing.T) { + err := RefreshAccessToken(context.Background(), nil) + require.Error(t, err) +} + +func TestIsTokenExpiredNilProfile(t *testing.T) { + assert.True(t, IsTokenExpired(nil)) +} + +func TestHasRefreshTokenNilProfile(t *testing.T) { + assert.False(t, HasRefreshToken(nil)) +} diff --git a/otdfctl/pkg/man/man.go b/otdfctl/pkg/man/man.go index 5b555c7059..0039c4b216 100644 --- a/otdfctl/pkg/man/man.go +++ b/otdfctl/pkg/man/man.go @@ -236,10 +236,12 @@ func ProcessDoc(doc string) (*Doc, error) { long := "# " + matter.Title + "\n\n" + strings.TrimSpace(string(rest)) var args cobra.PositionalArgs - if len(c.Args) > 0 { + switch { + case len(c.Args) > 0 && len(c.ArbitraryArgs) > 0: + args = cobra.MinimumNArgs(len(c.Args)) + case len(c.Args) > 0: args = cobra.ExactArgs(len(c.Args)) - } - if len(c.ArbitraryArgs) > 0 { + case len(c.ArbitraryArgs) > 0: args = cobra.ArbitraryArgs } diff --git a/otdfctl/pkg/man/man_test.go b/otdfctl/pkg/man/man_test.go index cc8c28a531..023b04fbe0 100644 --- a/otdfctl/pkg/man/man_test.go +++ b/otdfctl/pkg/man/man_test.go @@ -67,6 +67,25 @@ Authenticate via client credentials flow. assert.Equal(t, "client-credentials [client-secret]", doc.Use) } +func TestProcessDocWithBothArgTypesValidator(t *testing.T) { + doc, err := ProcessDoc(`--- +title: Authenticate with client credentials +command: + name: client-credentials + arguments: + - client-id + arbitraryArgs: + - client-secret +--- + +Authenticate via client credentials flow. +`) + require.NoError(t, err) + require.NoError(t, doc.Args(&doc.Command, []string{"id"})) + require.NoError(t, doc.Args(&doc.Command, []string{"id", "secret"})) + require.Error(t, doc.Args(&doc.Command, []string{})) +} + func TestBuildUseString(t *testing.T) { tests := []struct { name string From ff76a60fac4a91b32b0f2e5964d75d88722f71ae Mon Sep 17 00:00:00 2001 From: Krish Suchak Date: Wed, 13 May 2026 11:58:33 -0400 Subject: [PATCH 4/7] add buffer and fix coderabbitai bug --- otdfctl/pkg/auth/refresh.go | 13 +++++++++++-- otdfctl/pkg/auth/refresh_test.go | 33 ++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/otdfctl/pkg/auth/refresh.go b/otdfctl/pkg/auth/refresh.go index 2fc4b4988f..33acc024d0 100644 --- a/otdfctl/pkg/auth/refresh.go +++ b/otdfctl/pkg/auth/refresh.go @@ -14,6 +14,9 @@ import ( const ( DefaultPublicClientID = "cli-client" + // expiryBuffer is subtracted from the token expiry to account for subprocess + // startup and network latency between the expiry check and the actual API call. + expiryBuffer = 30 * time.Second ) // Function variable for the token endpoint lookup — swappable in tests. @@ -78,13 +81,19 @@ func RefreshAccessToken(ctx context.Context, profile *profiles.OtdfctlProfileSto slog.Debug("successfully refreshed access token") + expiration := newToken.Expiry.Unix() + if newToken.Expiry.IsZero() { + expiration = time.Now().Add(time.Hour).Unix() + slog.Warn("token response missing expires_in, assuming 1 hour") + } + newCreds := profiles.AuthCredentials{ AuthType: profiles.AuthTypeAccessToken, AccessToken: profiles.AuthCredentialsAccessToken{ ClientID: clientID, AccessToken: newToken.AccessToken, RefreshToken: newToken.RefreshToken, - Expiration: newToken.Expiry.Unix(), + Expiration: expiration, }, } @@ -107,7 +116,7 @@ func IsTokenExpired(profile *profiles.OtdfctlProfileStore) bool { return false } expiry := time.Unix(creds.AccessToken.Expiration, 0) - return time.Now().After(expiry) + return time.Now().Add(expiryBuffer).After(expiry) } // HasRefreshToken checks if the profile has a refresh token. diff --git a/otdfctl/pkg/auth/refresh_test.go b/otdfctl/pkg/auth/refresh_test.go index 8bd2d32b98..2903c58229 100644 --- a/otdfctl/pkg/auth/refresh_test.go +++ b/otdfctl/pkg/auth/refresh_test.go @@ -55,6 +55,12 @@ func TestIsTokenExpired(t *testing.T) { exp: time.Now().Add(time.Hour).Unix(), want: false, }, + { + name: "token within expiry buffer", + authType: profiles.AuthTypeAccessToken, + exp: time.Now().Add(10 * time.Second).Unix(), + want: true, + }, { name: "non-access-token auth type", authType: profiles.AuthTypeClientCredentials, @@ -151,6 +157,33 @@ func TestRefreshAccessTokenSuccess(t *testing.T) { "expiration should be updated to a future timestamp") } +func TestRefreshAccessTokenZeroExpiry(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-token", + "refresh_token": "new-refresh", + "token_type": "Bearer", + }) + assert.NoError(t, err) + })) + defer tokenServer.Close() + + origFunc := getTokenEndpointFunc + getTokenEndpointFunc = func(string, bool) (string, error) { + return tokenServer.URL, nil + } + defer func() { getTokenEndpointFunc = origFunc }() + + profile := newTestProfile(t, profiles.AuthTypeAccessToken, "old", "refresh", time.Now().Add(-time.Hour).Unix()) + err := RefreshAccessToken(context.Background(), profile) + require.NoError(t, err) + + creds := profile.GetAuthCredentials() + assert.Greater(t, creds.AccessToken.Expiration, time.Now().Unix(), + "zero expiry should default to ~1 hour from now") +} + func TestRefreshAccessTokenEndpointError(t *testing.T) { origFunc := getTokenEndpointFunc getTokenEndpointFunc = func(string, bool) (string, error) { From bf5ca6dba06b99fda8fe7c706f4677bc690d0c89 Mon Sep 17 00:00:00 2001 From: Krish Suchak Date: Wed, 13 May 2026 17:04:39 -0400 Subject: [PATCH 5/7] update comments --- otdfctl/pkg/auth/refresh.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/otdfctl/pkg/auth/refresh.go b/otdfctl/pkg/auth/refresh.go index 33acc024d0..ecf5f44bac 100644 --- a/otdfctl/pkg/auth/refresh.go +++ b/otdfctl/pkg/auth/refresh.go @@ -14,8 +14,8 @@ import ( const ( DefaultPublicClientID = "cli-client" - // expiryBuffer is subtracted from the token expiry to account for subprocess - // startup and network latency between the expiry check and the actual API call. + // expiryBuffer is added to the current time to account for token expiry occurring during + // subprocess startup and network latency between the expiry check and the actual API call. expiryBuffer = 30 * time.Second ) @@ -116,6 +116,8 @@ func IsTokenExpired(profile *profiles.OtdfctlProfileStore) bool { return false } expiry := time.Unix(creds.AccessToken.Expiration, 0) + // We are checking if the current time plus the buffer is after the true token expiry time. + // If it is, we refresh the token. The purpose of the buffer is to avoid expiry between calls. return time.Now().Add(expiryBuffer).After(expiry) } From 3a9de575be97f673cff1bfc9cd40f3664edec98f Mon Sep 17 00:00:00 2001 From: Krish Suchak Date: Wed, 13 May 2026 17:23:42 -0400 Subject: [PATCH 6/7] suggestions --- otdfctl/pkg/auth/refresh.go | 12 +++++-- otdfctl/pkg/auth/refresh_test.go | 60 ++++++++------------------------ 2 files changed, 24 insertions(+), 48 deletions(-) diff --git a/otdfctl/pkg/auth/refresh.go b/otdfctl/pkg/auth/refresh.go index ecf5f44bac..8965dbcb4a 100644 --- a/otdfctl/pkg/auth/refresh.go +++ b/otdfctl/pkg/auth/refresh.go @@ -19,12 +19,18 @@ const ( expiryBuffer = 30 * time.Second ) -// Function variable for the token endpoint lookup — swappable in tests. -var getTokenEndpointFunc = getTokenEndpoint +// tokenEndpointResolver looks up the OAuth2 token endpoint for a given +// platform endpoint. Production code uses getTokenEndpoint; tests inject +// a stub to avoid real gRPC calls. +type tokenEndpointResolver func(endpoint string, tlsNoVerify bool) (string, error) // RefreshAccessToken refreshes the access token using the stored refresh token // and updates the profile with the new tokens. func RefreshAccessToken(ctx context.Context, profile *profiles.OtdfctlProfileStore) error { + return refreshAccessToken(ctx, profile, getTokenEndpoint) +} + +func refreshAccessToken(ctx context.Context, profile *profiles.OtdfctlProfileStore, resolveEndpoint tokenEndpointResolver) error { if profile == nil { return errors.New("profile is required") } @@ -47,7 +53,7 @@ func RefreshAccessToken(ctx context.Context, profile *profiles.OtdfctlProfileSto return fmt.Errorf("failed to normalize endpoint: %w", err) } - tokenEndpoint, err := getTokenEndpointFunc(normalized.String(), tlsNoVerify) + tokenEndpoint, err := resolveEndpoint(normalized.String(), tlsNoVerify) if err != nil { return fmt.Errorf("failed to get token endpoint: %w", err) } diff --git a/otdfctl/pkg/auth/refresh_test.go b/otdfctl/pkg/auth/refresh_test.go index 2903c58229..ed435c3905 100644 --- a/otdfctl/pkg/auth/refresh_test.go +++ b/otdfctl/pkg/auth/refresh_test.go @@ -1,7 +1,6 @@ package auth import ( - "context" "encoding/json" "errors" "net/http" @@ -116,13 +115,13 @@ func TestHasRefreshToken(t *testing.T) { func TestRefreshAccessTokenWrongAuthType(t *testing.T) { profile := newTestProfile(t, profiles.AuthTypeClientCredentials, "tok", "refresh", time.Now().Add(time.Hour).Unix()) - err := RefreshAccessToken(context.Background(), profile) + err := RefreshAccessToken(t.Context(), profile) require.ErrorIs(t, err, ErrInvalidAuthType) } func TestRefreshAccessTokenNoRefreshToken(t *testing.T) { profile := newTestProfile(t, profiles.AuthTypeAccessToken, "tok", "", time.Now().Add(-time.Hour).Unix()) - err := RefreshAccessToken(context.Background(), profile) + err := RefreshAccessToken(t.Context(), profile) require.ErrorIs(t, err, ErrNoRefreshToken) } @@ -139,15 +138,13 @@ func TestRefreshAccessTokenSuccess(t *testing.T) { })) defer tokenServer.Close() - origFunc := getTokenEndpointFunc - getTokenEndpointFunc = func(string, bool) (string, error) { + resolver := func(string, bool) (string, error) { return tokenServer.URL, nil } - defer func() { getTokenEndpointFunc = origFunc }() profile := newTestProfile(t, profiles.AuthTypeAccessToken, "old-token", "old-refresh", time.Now().Add(-time.Hour).Unix()) - err := RefreshAccessToken(context.Background(), profile) + err := refreshAccessToken(t.Context(), profile, resolver) require.NoError(t, err) creds := profile.GetAuthCredentials() @@ -169,14 +166,12 @@ func TestRefreshAccessTokenZeroExpiry(t *testing.T) { })) defer tokenServer.Close() - origFunc := getTokenEndpointFunc - getTokenEndpointFunc = func(string, bool) (string, error) { + resolver := func(string, bool) (string, error) { return tokenServer.URL, nil } - defer func() { getTokenEndpointFunc = origFunc }() profile := newTestProfile(t, profiles.AuthTypeAccessToken, "old", "refresh", time.Now().Add(-time.Hour).Unix()) - err := RefreshAccessToken(context.Background(), profile) + err := refreshAccessToken(t.Context(), profile, resolver) require.NoError(t, err) creds := profile.GetAuthCredentials() @@ -185,14 +180,12 @@ func TestRefreshAccessTokenZeroExpiry(t *testing.T) { } func TestRefreshAccessTokenEndpointError(t *testing.T) { - origFunc := getTokenEndpointFunc - getTokenEndpointFunc = func(string, bool) (string, error) { + resolver := func(string, bool) (string, error) { return "", errors.New("gRPC connection failed") } - defer func() { getTokenEndpointFunc = origFunc }() profile := newTestProfile(t, profiles.AuthTypeAccessToken, "tok", "refresh", time.Now().Add(-time.Hour).Unix()) - err := RefreshAccessToken(context.Background(), profile) + err := refreshAccessToken(t.Context(), profile, resolver) require.ErrorContains(t, err, "failed to get token endpoint") } @@ -204,14 +197,12 @@ func TestRefreshAccessTokenRefreshFails(t *testing.T) { })) defer tokenServer.Close() - origFunc := getTokenEndpointFunc - getTokenEndpointFunc = func(string, bool) (string, error) { + resolver := func(string, bool) (string, error) { return tokenServer.URL, nil } - defer func() { getTokenEndpointFunc = origFunc }() profile := newTestProfile(t, profiles.AuthTypeAccessToken, "tok", "refresh", time.Now().Add(-time.Hour).Unix()) - err := RefreshAccessToken(context.Background(), profile) + err := refreshAccessToken(t.Context(), profile, resolver) require.Error(t, err) require.ErrorIs(t, err, ErrRefreshFailed) } @@ -229,11 +220,9 @@ func TestRefreshAccessTokenEmptyClientID(t *testing.T) { })) defer tokenServer.Close() - origFunc := getTokenEndpointFunc - getTokenEndpointFunc = func(string, bool) (string, error) { + resolver := func(string, bool) (string, error) { return tokenServer.URL, nil } - defer func() { getTokenEndpointFunc = origFunc }() cfg := &profiles.ProfileConfig{ Name: "test", @@ -252,7 +241,7 @@ func TestRefreshAccessTokenEmptyClientID(t *testing.T) { }) require.NoError(t, err) - err = RefreshAccessToken(context.Background(), profile) + err = refreshAccessToken(t.Context(), profile, resolver) require.NoError(t, err) creds := profile.GetAuthCredentials() @@ -272,11 +261,9 @@ func TestRefreshAccessTokenTLSNoVerify(t *testing.T) { })) defer tokenServer.Close() - origFunc := getTokenEndpointFunc - getTokenEndpointFunc = func(string, bool) (string, error) { + resolver := func(string, bool) (string, error) { return tokenServer.URL, nil } - defer func() { getTokenEndpointFunc = origFunc }() cfg := &profiles.ProfileConfig{ Name: "test", @@ -296,30 +283,13 @@ func TestRefreshAccessTokenTLSNoVerify(t *testing.T) { }) require.NoError(t, err) - err = RefreshAccessToken(context.Background(), profile) + err = refreshAccessToken(t.Context(), profile, resolver) require.NoError(t, err) creds := profile.GetAuthCredentials() assert.Equal(t, "tls-token", creds.AccessToken.AccessToken) } -func TestGetTokenEndpointFuncSwappable(t *testing.T) { - origFunc := getTokenEndpointFunc - defer func() { getTokenEndpointFunc = origFunc }() - - called := false - getTokenEndpointFunc = func(endpoint string, tlsNoVerify bool) (string, error) { - called = true - assert.Equal(t, "https://example.com:443", endpoint) - assert.False(t, tlsNoVerify) - return "https://idp.example.com/token", nil - } - - profile := newTestProfile(t, profiles.AuthTypeAccessToken, "tok", "refresh", time.Now().Add(-time.Hour).Unix()) - _ = RefreshAccessToken(context.Background(), profile) - assert.True(t, called, "getTokenEndpointFunc should have been called") -} - func TestGetTokenEndpointBadEndpoint(t *testing.T) { _, err := getTokenEndpoint("https://localhost:1", false) require.Error(t, err) @@ -332,7 +302,7 @@ func TestGetTokenEndpointEmptyEndpoint(t *testing.T) { } func TestRefreshAccessTokenNilProfile(t *testing.T) { - err := RefreshAccessToken(context.Background(), nil) + err := RefreshAccessToken(t.Context(), nil) require.Error(t, err) } From f6e546ac82a17680414e6b33e44be1ebfd28ba82 Mon Sep 17 00:00:00 2001 From: Krish Suchak Date: Mon, 18 May 2026 11:45:35 -0400 Subject: [PATCH 7/7] test change --- otdfctl/pkg/auth/refresh_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/otdfctl/pkg/auth/refresh_test.go b/otdfctl/pkg/auth/refresh_test.go index ed435c3905..b7e4c9c0af 100644 --- a/otdfctl/pkg/auth/refresh_test.go +++ b/otdfctl/pkg/auth/refresh_test.go @@ -249,7 +249,7 @@ func TestRefreshAccessTokenEmptyClientID(t *testing.T) { } func TestRefreshAccessTokenTLSNoVerify(t *testing.T) { - tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + tokenServer := httptest.NewTLSServer(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": "tls-token",