diff --git a/cmd/entire/cli/activity_cmd.go b/cmd/entire/cli/activity_cmd.go index c43af88df0..b2a5883d93 100644 --- a/cmd/entire/cli/activity_cmd.go +++ b/cmd/entire/cli/activity_cmd.go @@ -2,6 +2,7 @@ package cli import ( "context" + "errors" "fmt" "io" "net/url" @@ -12,6 +13,7 @@ import ( "time" "github.com/entireio/cli/cmd/entire/cli/api" + "github.com/entireio/cli/cmd/entire/cli/auth" "github.com/entireio/cli/cmd/entire/cli/interactive" "github.com/spf13/cobra" "golang.org/x/sync/errgroup" @@ -58,8 +60,24 @@ func newActivityCmd() *cobra.Command { func runActivity(ctx context.Context, w, errW io.Writer) error { client, err := NewAuthenticatedAPIClient(ctx, false) if err != nil { - fmt.Fprintln(errW, "Not logged in. Run 'entire login' to authenticate.") - return NewSilentError(err) + // Ctrl+C during the keyring read or STS exchange surfaces as + // context.Canceled / DeadlineExceeded. Silence it to match the + // codebase convention (clean.go, explain.go, explain_export.go) — + // printing "context canceled" at a user who just hit Ctrl+C is + // noise, not diagnostic. + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return NewSilentError(err) + } + // Only the "no core token in keyring" sentinel gets the friendly + // login hint. Other failures (STS exchange rejected, network + // error, malformed env config) used to be swallowed under the + // same hint, which sent users on wild goose chases trying to + // "re-login" their way out of unrelated server-side problems. + if errors.Is(err, auth.ErrNotLoggedIn) { + fmt.Fprintln(errW, "Not logged in. Run 'entire login' to authenticate.") + return NewSilentError(err) + } + return err } // Non-interactive fallback: piped output or accessibility mode diff --git a/cmd/entire/cli/activity_cmd_test.go b/cmd/entire/cli/activity_cmd_test.go index e9f741d480..32ccafbfcf 100644 --- a/cmd/entire/cli/activity_cmd_test.go +++ b/cmd/entire/cli/activity_cmd_test.go @@ -1,12 +1,91 @@ package cli import ( + "bytes" + "context" + "errors" + "strings" "testing" "time" + + "github.com/entireio/auth-go/sts" + "github.com/entireio/auth-go/tokens" + "github.com/entireio/cli/cmd/entire/cli/auth" ) func strPtr(v string) *string { return &v } +// TestRunActivity_SilencesContextCanceled pins the codebase convention +// (clean.go, explain.go, explain_export.go) for Ctrl+C during the auth +// resolution: NewSilentError wraps the cancellation so cobra doesn't +// print "context canceled" at a user who just chose to stop. +// +// Pre-PR runActivity silenced *every* auth-resolution error under the +// "Not logged in" hint; that was wrong because real STS / network +// failures got mis-labeled. This PR surfaces real errors but has to +// keep the cancellation case silent. +func TestRunActivity_SilencesContextCanceled(t *testing.T) { + // No t.Parallel: SetManagerForTest mutates package-level auth state. + store := newAuthMemStore() + saveCoreToken(t, store, authResolveTestIssuer, "opaque-core-token") + + mgr := newResolveTestManager(t, store, func(context.Context, sts.ExchangeRequest) (*tokens.TokenSet, error) { + // Simulate the user hitting Ctrl+C mid-exchange. The real + // transport would return ctx.Err() wrapped; both shapes flow + // through errors.Is(err, context.Canceled) identically. + return nil, context.Canceled + }) + t.Cleanup(auth.SetManagerForTest(t, mgr)) + + var out, errOut bytes.Buffer + err := runActivity(t.Context(), &out, &errOut) + if err == nil { + t.Fatal("expected error when STS exchange is cancelled") + } + if !errors.Is(err, context.Canceled) { + t.Errorf("error chain missing context.Canceled: %v", err) + } + var silent *SilentError + if !errors.As(err, &silent) { + t.Errorf("error = %v, want SilentError wrap so cobra suppresses output", err) + } + if errOut.Len() != 0 { + t.Errorf("errOut = %q, want empty (no 'Not logged in' hint on cancellation)", errOut.String()) + } +} + +// TestRunActivity_PrintsLoginHintOnNotLoggedIn pins the other half of +// the same branch: a missing keyring entry still produces the friendly +// hint and a SilentError so the raw "not logged in" string doesn't +// also print via cobra. +func TestRunActivity_PrintsLoginHintOnNotLoggedIn(t *testing.T) { + // No t.Parallel: SetManagerForTest mutates package-level auth state. + store := newAuthMemStore() // empty: LookupCoreToken returns "" → ErrNotLoggedIn + + mgr := newResolveTestManager(t, store, func(context.Context, sts.ExchangeRequest) (*tokens.TokenSet, error) { + t.Fatal("exchange should not run when no core token is stored") + return nil, errors.New("unreachable") + }) + t.Cleanup(auth.SetManagerForTest(t, mgr)) + + var out, errOut bytes.Buffer + err := runActivity(t.Context(), &out, &errOut) + if err == nil { + t.Fatal("expected error when not logged in") + } + if !errors.Is(err, auth.ErrNotLoggedIn) { + t.Errorf("error chain missing ErrNotLoggedIn: %v", err) + } + var silent *SilentError + if !errors.As(err, &silent) { + t.Errorf("error = %v, want SilentError wrap", err) + } + wantHint := "Not logged in. Run 'entire login' to authenticate." + if got := errOut.String(); !strings.Contains(got, wantHint) { + t.Errorf("errOut = %q, want hint %q", got, wantHint) + } +} + func TestNormalizeAgentString(t *testing.T) { t.Parallel() tests := []struct { diff --git a/cmd/entire/cli/api_client.go b/cmd/entire/cli/api_client.go index b74b6dc159..5981fc0498 100644 --- a/cmd/entire/cli/api_client.go +++ b/cmd/entire/cli/api_client.go @@ -44,7 +44,14 @@ func NewAuthenticatedAPIClient(ctx context.Context, insecureHTTP bool) (*api.Cli token, err := auth.TokenForResource(ctx, api.OriginOnly(dataURL)) if err != nil { if errors.Is(err, auth.ErrNotLoggedIn) { - return nil, errors.New("not logged in (run 'entire login' first)") + // Wrap the original err (not the sentinel) so any context + // the tokenmanager attached — keyring backend message, + // expired-token reason — survives to the caller. The + // errors.Is(err, auth.ErrNotLoggedIn) chain is preserved + // because err already wraps the sentinel; replacing it + // with the bare sentinel would drop that context for + // zero behavioural gain. + return nil, fmt.Errorf("not logged in (run 'entire login' first): %w", err) } return nil, fmt.Errorf("resolve API token: %w", err) } diff --git a/cmd/entire/cli/auth.go b/cmd/entire/cli/auth.go index b7531a9896..b158378578 100644 --- a/cmd/entire/cli/auth.go +++ b/cmd/entire/cli/auth.go @@ -17,11 +17,16 @@ import ( "github.com/spf13/cobra" ) -// authTokenLister lists API tokens for the authenticated user. -type authTokenLister func(ctx context.Context, token string) ([]api.Token, error) +// authTokenLister lists API tokens for the authenticated user. The +// implementation resolves its own data-API bearer via +// auth.TokenForResource (RFC 8693 exchange in split-host setups, same- +// host shortcut otherwise); callers don't pass a bearer through, which +// removes the temptation to forward the wrong-audience keyring token. +type authTokenLister func(ctx context.Context) ([]api.Token, error) -// authTokenRevoker revokes a single API token by id. -type authTokenRevoker func(ctx context.Context, callerToken, id string) error +// authTokenRevoker revokes a single API token by id. Same bearer- +// resolution contract as authTokenLister. +type authTokenRevoker func(ctx context.Context, id string) error // User-visible placeholder strings. Promoted to constants so tests and // production share a single source of truth. @@ -40,8 +45,14 @@ const ( // auth host for login + auth-token management, and to the data host for // search/activity/dispatch/etc. Single-host deployments (ENTIRE_AUTH_BASE_URL // unset) skip the redundant second parse. +// +// When the opt-in flag is set, the tokenmanager's matching HTTP guard is +// also relaxed via auth.EnableInsecureHTTP — otherwise an STS exchange +// against a private-network http:// auth host would fail at the +// tokenmanager layer even though the per-command TLS check was waived. func requireSecureBaseURL(insecureHTTPAuth bool) error { if insecureHTTPAuth { + auth.EnableInsecureHTTP() return nil } dataURL, authURL := api.BaseURL(), api.AuthBaseURL() @@ -59,16 +70,63 @@ func requireSecureBaseURL(insecureHTTPAuth bool) error { // newAPITokensClient builds an api.Client for the auth-token management // endpoints (list / revoke / current). API tokens live on the data API -// regardless of split-host config — the auth host (entire-core in v2) mints -// OAuth tokens but doesn't manage application API tokens — so this targets -// api.BaseURL(). The bearer is whatever the caller already extracted from -// the keyring (keyed by api.AuthBaseURL()); the data API validates it via -// ENTIRE_CORE_BEARER_ENABLED in split-host setups. +// regardless of split-host config — the auth host (entire-core in v2) +// mints OAuth tokens but doesn't host application API token management +// endpoints — so this targets api.BaseURL(). +// +// The supplied token must already be scoped for api.BaseURL(). Callers +// must obtain it via resolveDataAPIToken (or auth.TokenForResource +// directly) rather than handing through the raw keyring entry — the +// keyring stores the auth-host-issued core token, which the data API +// rejects in split-host setups. func newAPITokensClient(token string) *api.Client { return api.NewClientWithBaseURL(token, api.BaseURL()). WithAuthTokensPath(auth.CurrentProvider().AuthTokensPath) } +// resolveDataAPIToken returns a bearer scoped for the data API. In +// split-host setups this triggers an RFC 8693 exchange against the +// auth host's STS endpoint; in single-host setups the tokenmanager +// hits the same-host shortcut and returns the core token unchanged. +// Centralised so the audience-mismatch bug that motivated this fix +// can't be reintroduced piecemeal at individual call sites. +func resolveDataAPIToken(ctx context.Context) (string, error) { + token, err := auth.TokenForResource(ctx, api.OriginOnly(api.BaseURL())) + if err != nil { + return "", fmt.Errorf("resolve API token: %w", err) + } + return token, nil +} + +// isKeychainTokenRejected reports whether err indicates the stored +// keyring token can't authenticate against the data API. Three failure +// modes collapse into this single "the user must re-login" branch: +// +// - data API returned 401 (single-host, or after a successful STS +// exchange whose result the data API then rejected), +// - tokenmanager's preflight rejected an expired core token JWT +// (surfacing as auth.ErrNotLoggedIn even though the keyring entry +// is still present), +// - the STS endpoint rejected the core token during exchange in a +// split-host setup. auth-go's sts package returns the response as +// "token exchange: status 4xx: [: ]" with no typed +// sentinel exposed, so detection has to be by prefix. The "status +// 4" anchor catches the entire 4xx range — every 4xx from STS is +// a credential problem, none are retryable without user action. +// +// Other shapes (network errors, malformed STS response, manager +// construction failures) deliberately don't match — the user sees the +// real diagnostic instead of a misleading "re-login" hint. +func isKeychainTokenRejected(err error) bool { + if api.IsHTTPErrorStatus(err, http.StatusUnauthorized) { + return true + } + if errors.Is(err, auth.ErrNotLoggedIn) { + return true + } + return strings.Contains(err.Error(), "token exchange: status 4") +} + // addInsecureHTTPAuthFlag attaches the hidden --insecure-http-auth flag used // by every authenticated command for local development. func addInsecureHTTPAuthFlag(cmd *cobra.Command, target *bool) { @@ -115,7 +173,11 @@ func newAuthStatusCmd() *cobra.Command { return cmd } -func defaultListTokens(ctx context.Context, token string) ([]api.Token, error) { +func defaultListTokens(ctx context.Context) ([]api.Token, error) { + token, err := resolveDataAPIToken(ctx) + if err != nil { + return nil, err + } return newAPITokensClient(token).ListTokens(ctx) //nolint:wrapcheck // ListTokens already wraps with action context } @@ -130,9 +192,9 @@ func runAuthStatus(ctx context.Context, w io.Writer, store tokenStore, list auth return nil } - tokens, err := list(ctx, token) + tokens, err := list(ctx) if err != nil { - if api.IsHTTPErrorStatus(err, http.StatusUnauthorized) { + if isKeychainTokenRejected(err) { fmt.Fprintf(w, "Token in keychain for %s is no longer valid.\n", baseURL) fmt.Fprintln(w, "Run 'entire login' to re-authenticate.") return nil @@ -176,7 +238,7 @@ func runAuthList(ctx context.Context, w io.Writer, store tokenStore, list authTo return fmt.Errorf("not logged in to %s; run 'entire login' first", baseURL) } - tokens, err := list(ctx, token) + tokens, err := list(ctx) if err != nil { return err } @@ -441,8 +503,12 @@ func newAuthRevokeCmd() *cobra.Command { return cmd } -func defaultRevokeTokenByID(ctx context.Context, callerToken, id string) error { - return newAPITokensClient(callerToken).RevokeToken(ctx, id) //nolint:wrapcheck // RevokeToken already wraps with action context +func defaultRevokeTokenByID(ctx context.Context, id string) error { + token, err := resolveDataAPIToken(ctx) + if err != nil { + return err + } + return newAPITokensClient(token).RevokeToken(ctx, id) //nolint:wrapcheck // RevokeToken already wraps with action context } func runAuthRevoke( @@ -469,14 +535,14 @@ func runAuthRevoke( return runLogout(ctx, outW, errW, store, revokeCurrent, baseURL) } - if err := revokeByID(ctx, token, id); err != nil { + if err := revokeByID(ctx, id); err != nil { return err } // The list endpoint requires bearer auth, so a 401 here means the id we // just revoked was the same one this CLI is using — the keychain entry is // now stale and would otherwise produce confusing 401s on every command. - if _, listErr := list(ctx, token); listErr != nil && api.IsHTTPErrorStatus(listErr, http.StatusUnauthorized) { + if _, listErr := list(ctx); listErr != nil && api.IsHTTPErrorStatus(listErr, http.StatusUnauthorized) { if delErr := store.DeleteToken(baseURL); delErr != nil { return fmt.Errorf("revoked token %s but failed to remove local copy: %w", id, delErr) } diff --git a/cmd/entire/cli/auth_test.go b/cmd/entire/cli/auth_test.go index 9acd3fe848..8d0e902753 100644 --- a/cmd/entire/cli/auth_test.go +++ b/cmd/entire/cli/auth_test.go @@ -9,7 +9,12 @@ import ( "testing" "time" + "github.com/entireio/auth-go/sts" + "github.com/entireio/auth-go/tokenmanager" + "github.com/entireio/auth-go/tokens" + "github.com/entireio/auth-go/tokenstore" "github.com/entireio/cli/cmd/entire/cli/api" + "github.com/entireio/cli/cmd/entire/cli/auth" ) const ( @@ -26,7 +31,7 @@ func TestRunAuthStatus_NotLoggedIn(t *testing.T) { store := newMockTokenStore() listCalled := false - list := func(context.Context, string) ([]api.Token, error) { + list := func(context.Context) ([]api.Token, error) { listCalled = true return nil, nil } @@ -50,7 +55,7 @@ func TestRunAuthStatus_LoggedIn(t *testing.T) { store := newMockTokenStore() store.tokens[testBaseURL] = testAuthTok - list := func(context.Context, string) ([]api.Token, error) { + list := func(context.Context) ([]api.Token, error) { return []api.Token{ {ID: "a", Name: "laptop"}, {ID: "b", Name: "ci"}, @@ -76,7 +81,7 @@ func TestRunAuthStatus_TokenInvalid(t *testing.T) { store := newMockTokenStore() store.tokens[testBaseURL] = testAuthTok - list := func(context.Context, string) ([]api.Token, error) { + list := func(context.Context) ([]api.Token, error) { return nil, &api.HTTPError{StatusCode: http.StatusUnauthorized, Message: "Not authenticated"} } @@ -93,13 +98,94 @@ func TestRunAuthStatus_TokenInvalid(t *testing.T) { } } +// TestRunAuthStatus_STSRejectionRendersInvalidMessage pins fix #2: in +// split-host setups, STS rejection happens before any HTTP call to the +// data API, so the friendly "Token in keychain ... is no longer valid" +// message has to fire on the auth-go sts package's wrapped string +// (no typed sentinel) as well as the data-API 401 case above. +func TestRunAuthStatus_STSRejectionRendersInvalidMessage(t *testing.T) { + t.Parallel() + + store := newMockTokenStore() + store.tokens[testBaseURL] = testAuthTok + + list := func(context.Context) ([]api.Token, error) { + // Exact format auth-go's sts package emits for an invalid_grant + // 4xx (see internal/oauthhttp's readAPIError). Without the + // detection in isKeychainTokenRejected this would fall through + // to the generic "validate token: ..." error path and the user + // would see a raw STS string instead of the re-login hint. + return nil, errors.New("token exchange: status 400: invalid_grant: subject_token expired") + } + + var out bytes.Buffer + if err := runAuthStatus(context.Background(), &out, store, list, testBaseURL); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if !strings.Contains(out.String(), "no longer valid") { + t.Fatalf("output = %q, want invalid-token message", out.String()) + } + if !strings.Contains(out.String(), "entire login") { + t.Fatalf("output = %q, want re-auth hint", out.String()) + } +} + +// TestRunAuthStatus_ExpiredCoreTokenRendersInvalidMessage pins the +// other half of fix #2: the tokenmanager's preflight check returns +// auth.ErrNotLoggedIn when a stored core JWT's exp claim is in the +// past. The keyring read at the top of runAuthStatus still finds a +// non-empty entry, so the "Not logged in" branch doesn't fire — the +// helper has to route the wrapped sentinel to the same re-login hint. +func TestRunAuthStatus_ExpiredCoreTokenRendersInvalidMessage(t *testing.T) { + t.Parallel() + + store := newMockTokenStore() + store.tokens[testBaseURL] = testAuthTok + + list := func(context.Context) ([]api.Token, error) { + return nil, errors.New("resolve API token: " + auth.ErrNotLoggedIn.Error()) + } + // errors.New above is intentionally string-only to defeat the + // detection — confirm the substring fallback alone isn't what's + // catching this case. The real production path wraps with %w. + listWithChain := func(context.Context) ([]api.Token, error) { + return nil, &wrappedTestError{msg: "resolve API token", inner: auth.ErrNotLoggedIn} + } + + // Sanity: string-only does NOT match (no sentinel chain). + var out1 bytes.Buffer + if err := runAuthStatus(context.Background(), &out1, store, list, testBaseURL); err == nil { + t.Fatal("string-only ErrNotLoggedIn should not match — keep the test honest") + } + + // Real path: errors.Is sees the sentinel through the %w chain. + var out2 bytes.Buffer + if err := runAuthStatus(context.Background(), &out2, store, listWithChain, testBaseURL); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !strings.Contains(out2.String(), "no longer valid") { + t.Fatalf("output = %q, want invalid-token message", out2.String()) + } +} + +// wrappedTestError is a tiny stand-in for fmt.Errorf("...: %w", inner) — kept +// local so the test reads as "this is what production hands runAuthStatus". +type wrappedTestError struct { + msg string + inner error +} + +func (e *wrappedTestError) Error() string { return e.msg + ": " + e.inner.Error() } +func (e *wrappedTestError) Unwrap() error { return e.inner } + func TestRunAuthStatus_ServerError(t *testing.T) { t.Parallel() store := newMockTokenStore() store.tokens[testBaseURL] = testAuthTok - list := func(context.Context, string) ([]api.Token, error) { + list := func(context.Context) ([]api.Token, error) { return nil, errors.New("connection refused") } @@ -122,7 +208,7 @@ func TestRunAuthList_NotLoggedInErrors(t *testing.T) { var out bytes.Buffer err := runAuthList(context.Background(), &out, store, - func(context.Context, string) ([]api.Token, error) { return nil, nil }, + func(context.Context) ([]api.Token, error) { return nil, nil }, testBaseURL, false) if err == nil { t.Fatal("expected error when not logged in") @@ -139,7 +225,7 @@ func TestRunAuthList_TablePrintsRows(t *testing.T) { store.tokens[testBaseURL] = testAuthTok lastUsed := "2026-04-01T12:00:00Z" - list := func(context.Context, string) ([]api.Token, error) { + list := func(context.Context) ([]api.Token, error) { return []api.Token{ {ID: "tok-1", Name: "laptop", Scope: "cli", CreatedAt: "2026-01-01T00:00:00Z", @@ -179,7 +265,7 @@ func TestRunAuthList_JSONOutput(t *testing.T) { store := newMockTokenStore() store.tokens[testBaseURL] = testAuthTok - list := func(context.Context, string) ([]api.Token, error) { + list := func(context.Context) ([]api.Token, error) { return []api.Token{{ID: "tok-1", Name: "laptop"}}, nil } @@ -203,7 +289,7 @@ func TestRunAuthList_EmptyPrintsMessage(t *testing.T) { store := newMockTokenStore() store.tokens[testBaseURL] = testAuthTok - list := func(context.Context, string) ([]api.Token, error) { return nil, nil } + list := func(context.Context) ([]api.Token, error) { return nil, nil } var out bytes.Buffer if err := runAuthList(context.Background(), &out, store, list, testBaseURL, false); err != nil { @@ -295,21 +381,20 @@ func TestRunAuthRevoke_ByIDCallsRevoker(t *testing.T) { store := newMockTokenStore() store.tokens[testBaseURL] = testAuthTok - var gotCallerToken, gotID string - revokeByID := func(_ context.Context, callerToken, id string) error { - gotCallerToken = callerToken + var gotID string + revokeByID := func(_ context.Context, id string) error { gotID = id return nil } revokeCurrentCalled := false - revokeCurrent := func(context.Context, string) error { + revokeCurrent := func(context.Context) error { revokeCurrentCalled = true return nil } // list returns 200 → token id was someone else's, no local cleanup expected. - list := func(context.Context, string) ([]api.Token, error) { + list := func(context.Context) ([]api.Token, error) { return []api.Token{{ID: "other"}}, nil } @@ -323,9 +408,8 @@ func TestRunAuthRevoke_ByIDCallsRevoker(t *testing.T) { if revokeCurrentCalled { t.Fatal("revokeCurrent should not be called when revoking by id") } - if gotCallerToken != testAuthTok || gotID != testTokenID { - t.Errorf("revokeByID called with caller=%q id=%q, want caller=%q id=%q", - gotCallerToken, gotID, testAuthTok, testTokenID) + if gotID != testTokenID { + t.Errorf("revokeByID called with id=%q, want %q", gotID, testTokenID) } if store.deleted[testBaseURL] { t.Fatal("local token should NOT be deleted when revoking another token") @@ -344,11 +428,11 @@ func TestRunAuthRevoke_ByIDSelfRevokeCleansLocal(t *testing.T) { store := newMockTokenStore() store.tokens[testBaseURL] = testAuthTok - revokeByID := func(context.Context, string, string) error { return nil } - revokeCurrent := func(context.Context, string) error { return nil } + revokeByID := func(context.Context, string) error { return nil } + revokeCurrent := func(context.Context) error { return nil } // list returns 401 → the id we just revoked was our own bearer. - list := func(context.Context, string) ([]api.Token, error) { + list := func(context.Context) ([]api.Token, error) { return nil, &api.HTTPError{StatusCode: http.StatusUnauthorized, Message: "Not authenticated"} } @@ -374,18 +458,18 @@ func TestRunAuthRevoke_CurrentDelegatesToLogout(t *testing.T) { store.tokens[testBaseURL] = testAuthTok revokeByIDCalled := false - revokeByID := func(context.Context, string, string) error { + revokeByID := func(context.Context, string) error { revokeByIDCalled = true return nil } - revokedToken := "" - revokeCurrent := func(_ context.Context, token string) error { - revokedToken = token + revokeCurrentCalled := false + revokeCurrent := func(context.Context) error { + revokeCurrentCalled = true return nil } - list := func(context.Context, string) ([]api.Token, error) { return nil, nil } + list := func(context.Context) ([]api.Token, error) { return nil, nil } var out, errOut bytes.Buffer err := runAuthRevoke(context.Background(), &out, &errOut, store, @@ -397,8 +481,8 @@ func TestRunAuthRevoke_CurrentDelegatesToLogout(t *testing.T) { if revokeByIDCalled { t.Fatal("revokeByID should not be called when --current is set") } - if revokedToken != testAuthTok { - t.Errorf("revokeCurrent called with token %q, want %q", revokedToken, testAuthTok) + if !revokeCurrentCalled { + t.Fatal("revokeCurrent should be called when --current is set") } if !store.deleted[testBaseURL] { t.Fatal("local token should be deleted via logout path") @@ -415,9 +499,9 @@ func TestRunAuthRevoke_NotLoggedInErrors(t *testing.T) { var out, errOut bytes.Buffer err := runAuthRevoke(context.Background(), &out, &errOut, store, - func(context.Context, string) ([]api.Token, error) { return nil, nil }, - func(context.Context, string, string) error { return nil }, + func(context.Context) ([]api.Token, error) { return nil, nil }, func(context.Context, string) error { return nil }, + func(context.Context) error { return nil }, testBaseURL, "some-id", false) if err == nil { t.Fatal("expected error when not logged in") @@ -454,6 +538,165 @@ func TestAuthCmd_RegistersExpectedSubcommands(t *testing.T) { } } +// --- resolveDataAPIToken ---------------------------------------------------- +// +// These tests exercise the production path: they install a real +// tokenmanager.Manager via auth.SetManagerForTest and stub only the +// STS wire call via SetExchangeForTest. That covers the audience- +// matching logic the function-injection tests above can't reach +// (defaultListTokens / defaultRevokeTokenByID call resolveDataAPIToken +// directly, but unit tests for the surrounding flows inject fakes +// that bypass it). + +// authResolveTestIssuer is intentionally distinct from the default +// api.BaseURL() ("https://entire.io") so the manager's same-host +// shortcut is skipped and the STS-exchange path runs. +const authResolveTestIssuer = "https://auth.resolve-test.example.com" + +func TestResolveDataAPIToken_ScopesExchangeToDataAPIOrigin(t *testing.T) { + // No t.Parallel: SetManagerForTest mutates package-level state in the + // auth package. Concurrent tests in this package don't reach the real + // auth.TokenForResource path (they inject lister/revoker fakes), so + // serial execution here is purely defensive. + + store := newAuthMemStore() + saveCoreToken(t, store, authResolveTestIssuer, "opaque-core-token") + + var capturedResource string + mgr := newResolveTestManager(t, store, func(_ context.Context, req sts.ExchangeRequest) (*tokens.TokenSet, error) { + capturedResource = req.Resource + return &tokens.TokenSet{AccessToken: "exchanged-data-api-tok"}, nil + }) + t.Cleanup(auth.SetManagerForTest(t, mgr)) + + got, err := resolveDataAPIToken(t.Context()) + if err != nil { + t.Fatalf("resolveDataAPIToken: %v", err) + } + + if got != "exchanged-data-api-tok" { + t.Errorf("token = %q, want %q", got, "exchanged-data-api-tok") + } + // The whole point of the helper: the resource handed to the STS + // exchange must be the data API's origin, not the auth host's + // origin and not the raw env var value. The default api.BaseURL() + // is "https://entire.io" and api.OriginOnly leaves it unchanged. + if capturedResource != "https://entire.io" { + t.Errorf("STS exchange Resource = %q, want %q (api.OriginOnly(api.BaseURL()))", + capturedResource, "https://entire.io") + } +} + +func TestResolveDataAPIToken_WrapsManagerError(t *testing.T) { + store := newAuthMemStore() + saveCoreToken(t, store, authResolveTestIssuer, "opaque-core-token") + + mgr := newResolveTestManager(t, store, func(context.Context, sts.ExchangeRequest) (*tokens.TokenSet, error) { + return nil, errors.New("simulated transport failure") + }) + t.Cleanup(auth.SetManagerForTest(t, mgr)) + + _, err := resolveDataAPIToken(t.Context()) + if err == nil { + t.Fatal("expected error when exchange fails") + } + if !strings.Contains(err.Error(), "resolve API token") { + t.Errorf("error = %v, want 'resolve API token' wrap prefix", err) + } + if !strings.Contains(err.Error(), "simulated transport failure") { + t.Errorf("error = %v, want underlying message preserved", err) + } +} + +// --- isKeychainTokenRejected ----------------------------------------------- + +func TestIsKeychainTokenRejected_AllShapes(t *testing.T) { + t.Parallel() + + cases := map[string]struct { + err error + want bool + }{ + "data API 401": {&api.HTTPError{StatusCode: http.StatusUnauthorized}, true}, + "data API 500": {&api.HTTPError{StatusCode: http.StatusInternalServerError}, false}, + "ErrNotLoggedIn": {auth.ErrNotLoggedIn, true}, + "wrapped ErrNotLoggedIn": {errors.New("resolve API token: " + auth.ErrNotLoggedIn.Error()), false /* string-only, no chain — not detected */}, + "sts 401": {errors.New("token exchange: status 401: invalid_client"), true}, + "sts 400 invalid_grant": {errors.New("token exchange: status 400: invalid_grant: token expired"), true}, + "sts 500": {errors.New("token exchange: status 500: server_error"), false}, + "network error": {errors.New("dial tcp: i/o timeout"), false}, + } + + // Confirm wrapped chains do propagate (the "wrapped ErrNotLoggedIn" + // case above uses string substitution which intentionally doesn't + // preserve the sentinel; this case uses fmt.Errorf %w which does). + cases["fmt.Errorf %w ErrNotLoggedIn"] = struct { + err error + want bool + }{errors.Join(errors.New("resolve API token"), auth.ErrNotLoggedIn), true} + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + t.Parallel() + if got := isKeychainTokenRejected(tc.err); got != tc.want { + t.Errorf("isKeychainTokenRejected(%v) = %v, want %v", tc.err, got, tc.want) + } + }) + } +} + +// --- helpers for resolveDataAPIToken tests ---------------------------------- + +// authMemStore is an in-memory tokenstore.Store for tests that need a +// real tokenmanager.Manager. Mirrors the private memStore in auth-go's +// tokenmanager_test.go — that one isn't exported, so we duplicate the +// trivial implementation rather than pull in a fragile internal package. +type authMemStore struct { + data map[string]tokens.TokenSet +} + +func newAuthMemStore() *authMemStore { return &authMemStore{data: map[string]tokens.TokenSet{}} } + +func (s *authMemStore) SaveTokens(profile string, t tokens.TokenSet) error { + s.data[profile] = t + return nil +} + +func (s *authMemStore) LoadTokens(profile string) (tokens.TokenSet, error) { + t, ok := s.data[profile] + if !ok { + return tokens.TokenSet{}, tokenstore.ErrNotFound + } + return t, nil +} + +func (s *authMemStore) DeleteTokens(profile string) error { + delete(s.data, profile) + return nil +} + +func saveCoreToken(t *testing.T, store tokenstore.Store, profile, accessToken string) { + t.Helper() + if err := store.SaveTokens(profile, tokens.TokenSet{AccessToken: accessToken}); err != nil { + t.Fatalf("SaveTokens: %v", err) + } +} + +func newResolveTestManager(t *testing.T, store tokenstore.Store, exchange func(context.Context, sts.ExchangeRequest) (*tokens.TokenSet, error)) *tokenmanager.Manager { + t.Helper() + mgr, err := tokenmanager.New(tokenmanager.Config{ + Issuer: authResolveTestIssuer, + ClientID: "entire-cli-test", + STSPath: "/sts/token", + Store: store, + }) + if err != nil { + t.Fatalf("tokenmanager.New: %v", err) + } + tokenmanager.SetExchangeForTest(t, mgr, exchange) + return mgr +} + func TestAuthCmd_TopLevelLoginAndLogoutStillRegistered(t *testing.T) { t.Parallel() diff --git a/cmd/entire/cli/logout.go b/cmd/entire/cli/logout.go index 91647b5ea8..a865a53df6 100644 --- a/cmd/entire/cli/logout.go +++ b/cmd/entire/cli/logout.go @@ -19,9 +19,11 @@ type tokenStore interface { DeleteToken(baseURL string) error } -// revokeCurrentFunc revokes the supplied token server-side. Mirrors the -// openURL injection pattern in login.go so tests can replace the real HTTP call. -type revokeCurrentFunc func(ctx context.Context, token string) error +// revokeCurrentFunc revokes the CLI's current token server-side. The +// implementation resolves its own data-API bearer (same audience- +// matching rule as authTokenLister); callers don't pass the keyring +// entry through. +type revokeCurrentFunc func(ctx context.Context) error func newLogoutCmd() *cobra.Command { var insecureHTTPAuth bool @@ -40,7 +42,11 @@ func newLogoutCmd() *cobra.Command { return cmd } -func defaultRevokeCurrentToken(ctx context.Context, token string) error { +func defaultRevokeCurrentToken(ctx context.Context) error { + token, err := resolveDataAPIToken(ctx) + if err != nil { + return err + } return newAPITokensClient(token).RevokeCurrentToken(ctx) //nolint:wrapcheck // RevokeCurrentToken already wraps with action context } @@ -52,7 +58,7 @@ func runLogout(ctx context.Context, outW, errW io.Writer, store tokenStore, revo fmt.Fprintf(errW, "Warning: failed to read token before revocation: %v\n", err) } if token != "" { - if err := revoke(ctx, token); err != nil && !api.IsHTTPErrorStatus(err, http.StatusUnauthorized) { + if err := revoke(ctx); err != nil && !api.IsHTTPErrorStatus(err, http.StatusUnauthorized) { // Best-effort: a transient network error shouldn't block local // logout. A 401 means the token is already invalid server-side, // so the desired state is achieved — no warning needed. diff --git a/cmd/entire/cli/logout_test.go b/cmd/entire/cli/logout_test.go index 28b3b57466..01cb92c5a3 100644 --- a/cmd/entire/cli/logout_test.go +++ b/cmd/entire/cli/logout_test.go @@ -52,9 +52,9 @@ func TestRunLogout_RevokesServerSideThenDeletesLocally(t *testing.T) { store := newMockTokenStore() store.tokens["https://entire.io"] = testLogoutToken - var gotToken string - revoke := func(_ context.Context, token string) error { - gotToken = token + revokeCalled := false + revoke := func(context.Context) error { + revokeCalled = true return nil } @@ -64,8 +64,8 @@ func TestRunLogout_RevokesServerSideThenDeletesLocally(t *testing.T) { t.Fatalf("unexpected error: %v", err) } - if gotToken != testLogoutToken { - t.Errorf("revoke called with token %q, want %q", gotToken, testLogoutToken) + if !revokeCalled { + t.Error("revoke should be called when a local token exists") } if !store.deleted["https://entire.io"] { t.Fatal("expected token to be deleted for https://entire.io") @@ -84,7 +84,7 @@ func TestRunLogout_NoTokenSkipsRevoke(t *testing.T) { store := newMockTokenStore() // no token stored revokeCalled := false - revoke := func(context.Context, string) error { + revoke := func(context.Context) error { revokeCalled = true return nil } @@ -112,7 +112,7 @@ func TestRunLogout_RevokeFailureWarnsButSucceeds(t *testing.T) { store := newMockTokenStore() store.tokens["https://entire.io"] = testLogoutToken - revoke := func(context.Context, string) error { + revoke := func(context.Context) error { return errors.New("connection refused") } @@ -142,7 +142,7 @@ func TestRunLogout_RevokeUnauthorizedIsSilent(t *testing.T) { store := newMockTokenStore() store.tokens["https://entire.io"] = testLogoutToken - revoke := func(context.Context, string) error { + revoke := func(context.Context) error { return &api.HTTPError{StatusCode: http.StatusUnauthorized, Message: "Not authenticated"} } @@ -170,7 +170,7 @@ func TestRunLogout_GetTokenErrorWarnsAndFallsThrough(t *testing.T) { store.getErr = errors.New("keyring locked for read") revokeCalled := false - revoke := func(context.Context, string) error { + revoke := func(context.Context) error { revokeCalled = true return nil } @@ -199,7 +199,7 @@ func TestRunLogout_ReturnsErrorOnDeleteFailure(t *testing.T) { store.tokens["https://entire.io"] = testLogoutToken store.deleteErr = errors.New("keyring locked") - revoke := func(context.Context, string) error { return nil } + revoke := func(context.Context) error { return nil } var out, errOut bytes.Buffer err := runLogout(context.Background(), &out, &errOut, store, revoke, "https://entire.io")