From c2d2cd63cdb35bd3ab730bc705a6c771c11b03ec Mon Sep 17 00:00:00 2001 From: Alex Ong Date: Sun, 24 May 2026 14:30:55 +1000 Subject: [PATCH 1/5] Route auth status/list/revoke/logout through TokenForResource MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In v2 split-host deployments (ENTIRE_AUTH_BASE_URL ≠ ENTIRE_API_BASE_URL), the keyring stores an auth-host-issued token. Sending that token directly to the data API's /api/v1/auth/tokens endpoint produces 401s because the audience is wrong; the user sees a misleading "Token in keychain ... is no longer valid" message. Three follow-on fixes: cmd/entire/cli/auth.go, logout.go: defaultListTokens, defaultRevokeTokenByID, and defaultRevokeCurrentToken now resolve a data-API-scoped bearer internally via auth.TokenForResource(ctx, api.OriginOnly(api.BaseURL())) — RFC 8693 exchange in split-host setups, same-host shortcut in single-host setups (so v1 single-host behavior is unchanged). The bug-prone `token` / `callerToken` parameters are dropped from authTokenLister, authTokenRevoker, and revokeCurrentFunc so the audience-mismatch mistake is structurally impossible at the call sites. New helper resolveDataAPIToken centralises the resolution. requireSecureBaseURL now also calls auth.EnableInsecureHTTP() when --insecure-http-auth is set, matching what NewAuthenticatedAPIClient already does — otherwise the tokenmanager's HTTPS guard would reject non-loopback http:// resources even after the per-command TLS check was waived. cmd/entire/cli/api_client.go: Wrap the not-logged-in case as `fmt.Errorf("...: %w", auth.ErrNotLoggedIn)` so callers can still errors.Is past the boundary (was errors.New(...) which lost the sentinel). cmd/entire/cli/activity_cmd.go: Only print "Not logged in. Run 'entire login' to authenticate." when errors.Is(err, auth.ErrNotLoggedIn). Any other error from NewAuthenticatedAPIClient (STS rejections, network errors, malformed env config) used to be silently re-mapped to that misleading login hint; now real errors surface verbatim via main.go. End-to-end verified against us.auth.entire.io: entire login, entire auth status, entire auth list, entire search, entire activity all succeed. Pre-existing TestExplainCmd_* failures are environmental (.entire/settings.json enabled:false in this working tree) and unrelated. Co-Authored-By: Claude Opus 4.7 --- cmd/entire/cli/activity_cmd.go | 14 ++++++- cmd/entire/cli/api_client.go | 8 +++- cmd/entire/cli/auth.go | 69 ++++++++++++++++++++++++++-------- cmd/entire/cli/auth_test.go | 54 +++++++++++++------------- cmd/entire/cli/logout.go | 16 +++++--- cmd/entire/cli/logout_test.go | 20 +++++----- 6 files changed, 119 insertions(+), 62 deletions(-) diff --git a/cmd/entire/cli/activity_cmd.go b/cmd/entire/cli/activity_cmd.go index c43af88df0..c6f4276fe1 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,16 @@ 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) + // 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/api_client.go b/cmd/entire/cli/api_client.go index b74b6dc159..fee82336d5 100644 --- a/cmd/entire/cli/api_client.go +++ b/cmd/entire/cli/api_client.go @@ -44,7 +44,13 @@ 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 rather than replace so callers can still + // errors.Is(err, auth.ErrNotLoggedIn) to distinguish "no + // keyring entry" from other resolution failures (STS + // rejection, network error). Pre-wrap, every caller had to + // fall back to string-matching, and the activity command + // papered over real failures with a misleading login hint. + return nil, fmt.Errorf("not logged in (run 'entire login' first): %w", auth.ErrNotLoggedIn) } 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..1de46eb31c 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,34 @@ 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 +} + // 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 +144,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,7 +163,7 @@ 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) { fmt.Fprintf(w, "Token in keychain for %s is no longer valid.\n", baseURL) @@ -176,7 +209,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 +474,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 +506,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..99f80f4451 100644 --- a/cmd/entire/cli/auth_test.go +++ b/cmd/entire/cli/auth_test.go @@ -26,7 +26,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 +50,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 +76,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"} } @@ -99,7 +99,7 @@ func TestRunAuthStatus_ServerError(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, errors.New("connection refused") } @@ -122,7 +122,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 +139,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 +179,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 +203,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 +295,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 +322,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 +342,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 +372,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 +395,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 +413,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") 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") From f8c6261c27f1dcc2f486971d790af4cddadb28b0 Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Thu, 28 May 2026 13:32:44 +0200 Subject: [PATCH 2/5] Preserve tokenmanager err context when wrapping ErrNotLoggedIn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit NewAuthenticatedAPIClient's "not logged in" branch was throwing away the original tokenmanager error and wrapping the bare sentinel: return nil, fmt.Errorf("...: %w", auth.ErrNotLoggedIn) Any context the manager attached — keyring backend message, expired- token reason, store-load error chain — was dropped, with no behavioural gain: the original err already wraps auth.ErrNotLoggedIn, so errors.Is(err, auth.ErrNotLoggedIn) keeps working unchanged when we wrap err directly. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: 035b5f21e289 --- cmd/entire/cli/api_client.go | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/cmd/entire/cli/api_client.go b/cmd/entire/cli/api_client.go index fee82336d5..5981fc0498 100644 --- a/cmd/entire/cli/api_client.go +++ b/cmd/entire/cli/api_client.go @@ -44,13 +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) { - // Wrap rather than replace so callers can still - // errors.Is(err, auth.ErrNotLoggedIn) to distinguish "no - // keyring entry" from other resolution failures (STS - // rejection, network error). Pre-wrap, every caller had to - // fall back to string-matching, and the activity command - // papered over real failures with a misleading login hint. - return nil, fmt.Errorf("not logged in (run 'entire login' first): %w", auth.ErrNotLoggedIn) + // 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) } From cf9f1fb0f02abe04816af5a4c298f44681ccfabf Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Thu, 28 May 2026 13:33:49 +0200 Subject: [PATCH 3/5] Surface STS/preflight rejections as 'token no longer valid' in auth status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before this commit, runAuthStatus's friendly "Token in keychain for X is no longer valid. Run 'entire login' to re-authenticate." branch fired only on *api.HTTPError 401 from the data API. In split-host setups (v2), two other failure modes leave the data API never being called at all: - tokenmanager preflight: a stored core JWT whose `exp` claim is in the past surfaces as auth.ErrNotLoggedIn from TokenForResource — not a 401 — and the keyring read at the top of runAuthStatus still found a non-empty entry, so the "Not logged in" branch didn't catch it either. User saw a raw "validate token: ..." error. - STS rejection: when the auth host rejects the core token during RFC 8693 exchange (revoked, malformed, audience mismatch), auth- go's sts package wraps the response as `token exchange: status 4xx: [: ]`. No typed sentinel is exposed upstream, so detection has to be by prefix. The "status 4" anchor catches the entire 4xx range — every 4xx from STS is a credential problem the user has to fix with a re-login; 5xx and transport errors flow through unchanged. New helper isKeychainTokenRejected collapses all three shapes (data- API 401, auth.ErrNotLoggedIn chain, sts 4xx prefix) into the single re-login branch. Other shapes — network failures, malformed STS response, manager construction errors — deliberately don't match so the user still sees the real diagnostic. Tested at two layers: - TestIsKeychainTokenRejected_AllShapes covers each error shape individually, including a deliberately-defeated string-only "wrapped ErrNotLoggedIn" case that confirms the substring fallback isn't accidentally catching sentinel cases. - TestRunAuthStatus_STSRejectionRendersInvalidMessage and TestRunAuthStatus_ExpiredCoreTokenRendersInvalidMessage pin the command-level UX — the friendly message actually fires on the new shapes, not just the helper in isolation. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: e5ce5b8770df --- cmd/entire/cli/auth.go | 31 +++++++++- cmd/entire/cli/auth_test.go | 119 ++++++++++++++++++++++++++++++++++++ 2 files changed, 149 insertions(+), 1 deletion(-) diff --git a/cmd/entire/cli/auth.go b/cmd/entire/cli/auth.go index 1de46eb31c..b158378578 100644 --- a/cmd/entire/cli/auth.go +++ b/cmd/entire/cli/auth.go @@ -98,6 +98,35 @@ func resolveDataAPIToken(ctx context.Context) (string, error) { 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) { @@ -165,7 +194,7 @@ func runAuthStatus(ctx context.Context, w io.Writer, store tokenStore, list auth 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 diff --git a/cmd/entire/cli/auth_test.go b/cmd/entire/cli/auth_test.go index 99f80f4451..052a62599b 100644 --- a/cmd/entire/cli/auth_test.go +++ b/cmd/entire/cli/auth_test.go @@ -10,6 +10,7 @@ import ( "time" "github.com/entireio/cli/cmd/entire/cli/api" + "github.com/entireio/cli/cmd/entire/cli/auth" ) const ( @@ -93,6 +94,87 @@ 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() @@ -452,6 +534,43 @@ func TestAuthCmd_RegistersExpectedSubcommands(t *testing.T) { } } +// --- 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) + } + }) + } +} + func TestAuthCmd_TopLevelLoginAndLogoutStillRegistered(t *testing.T) { t.Parallel() From 031c9600a19b60fb847eccbeac95c95299e1ce9c Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Thu, 28 May 2026 13:34:47 +0200 Subject: [PATCH 4/5] Test resolveDataAPIToken via SetManagerForTest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The function-injection tests for runAuthStatus / runAuthList / runAuthRevoke replace the authTokenLister / authTokenRevoker contracts with fakes and never reach resolveDataAPIToken — which is the helper that actually performs the audience-matching token exchange this PR was written to fix. So the production path (defaultListTokens etc. → resolveDataAPIToken → auth.TokenForResource) had no direct coverage. Two new tests install a real tokenmanager.Manager via auth.SetManagerForTest and stub only the wire-level STS call via tokenmanager.SetExchangeForTest: - TestResolveDataAPIToken_ScopesExchangeToDataAPIOrigin captures the Resource the manager hands to the exchange and asserts it is the api.OriginOnly(api.BaseURL()) origin — the bug this PR fixes was forwarding the wrong-audience token to the data API, so pinning the resource at the call boundary is the right assertion. - TestResolveDataAPIToken_WrapsManagerError covers the error path, asserting the "resolve API token" prefix and that the underlying manager error is preserved through %w. Supporting machinery (authMemStore implementing tokenstore.Store, saveCoreToken helper, newResolveTestManager wrapping tokenmanager.New + SetExchangeForTest) lives at the bottom of the test file. The memStore duplicates auth-go's private tokenmanager_test.go memStore rather than pulling in an internal test package. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: 8421ebe8c196 --- cmd/entire/cli/auth_test.go | 126 ++++++++++++++++++++++++++++++++++++ 1 file changed, 126 insertions(+) diff --git a/cmd/entire/cli/auth_test.go b/cmd/entire/cli/auth_test.go index 052a62599b..8d0e902753 100644 --- a/cmd/entire/cli/auth_test.go +++ b/cmd/entire/cli/auth_test.go @@ -9,6 +9,10 @@ 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" ) @@ -534,6 +538,76 @@ 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) { @@ -571,6 +645,58 @@ func TestIsKeychainTokenRejected_AllShapes(t *testing.T) { } } +// --- 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() From ae19736074d823cbdee5adc4cd54d985455432ec Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Thu, 28 May 2026 14:47:43 +0200 Subject: [PATCH 5/5] Silence context.Canceled in activity auth resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-PR runActivity treated every NewAuthenticatedAPIClient error as "Not logged in" and silenced it via NewSilentError. The previous commit (Route auth status/list/revoke/logout through TokenForResource) correctly stopped doing that for real errors — STS rejection, network failure, malformed config now surface verbatim instead of hiding under a misleading login hint. But it also stopped silencing context.Canceled. A user hitting Ctrl+C during the keyring read or STS exchange now sees a noisy "Error: context canceled" instead of a clean exit. The rest of the codebase has a clear convention for this (clean.go, explain.go, explain_export.go): errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) → NewSilentError. The new branch sits before the ErrNotLoggedIn check so cancellation takes priority over the login hint — a context.Canceled wrapping ErrNotLoggedIn (unlikely but possible during a slow keyring read) should still exit silently rather than print a confusing "re-authenticate" message at a user who chose to stop. Tested via SetManagerForTest + a stub exchange that returns context.Canceled — verifies SilentError wrap, preserved error chain, and that errOut stays empty (no spurious login hint on cancellation). Companion test pins the unchanged ErrNotLoggedIn path so the new branch isn't accidentally consuming legitimate "not logged in" cases. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: 5c1b29dca799 --- cmd/entire/cli/activity_cmd.go | 8 +++ cmd/entire/cli/activity_cmd_test.go | 79 +++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+) diff --git a/cmd/entire/cli/activity_cmd.go b/cmd/entire/cli/activity_cmd.go index c6f4276fe1..b2a5883d93 100644 --- a/cmd/entire/cli/activity_cmd.go +++ b/cmd/entire/cli/activity_cmd.go @@ -60,6 +60,14 @@ func newActivityCmd() *cobra.Command { func runActivity(ctx context.Context, w, errW io.Writer) error { client, err := NewAuthenticatedAPIClient(ctx, false) if err != nil { + // 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 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 {