diff --git a/internal/appctx/context.go b/internal/appctx/context.go index 2004ce78d..4dfaa7d29 100644 --- a/internal/appctx/context.go +++ b/internal/appctx/context.go @@ -3,6 +3,7 @@ package appctx import ( "context" + "errors" "fmt" "net/http" "os" @@ -282,7 +283,7 @@ func (a *App) Err(err error) error { } // Print the error response - if outputErr := a.Output.Err(err, opts...); outputErr != nil { + if outputErr := a.Output.Err(a.withAuthRemedy(err), opts...); outputErr != nil { return outputErr } @@ -294,6 +295,39 @@ func (a *App) Err(err error) error { return nil } +// withAuthRemedy replaces the generic login hint on an API 401 with one that +// fits the credential the request actually sent. The SDK classifies a 401 +// far from the profile and the environment, so its conversion can only say +// "basecamp auth login": under an active profile that command would store +// the new credential somewhere the failing command never reads, and under +// BASECAMP_TOKEN — which every request sends ahead of any stored login — no +// login changes anything. Only errors carrying an SDK error are rewritten: +// the credential manager's own failures name the profile themselves, and +// come from stored-credential operations (auth refresh, auth token +// --stored) that ignore the environment token by design. +func (a *App) withAuthRemedy(err error) error { + var sdkErr *basecamp.Error + e := output.AsError(err) + if e.Code != output.CodeAuth || !errors.As(err, &sdkErr) || (e.Hint != "" && !strings.HasPrefix(e.Hint, output.DefaultAuthHint)) { + return err + } + var remedy string + switch { + case os.Getenv("BASECAMP_TOKEN") != "": + remedy = "BASECAMP_TOKEN is set and every request uses it instead of a stored login; unset it, or export a token the server accepts" + case a.Auth != nil: + remedy = a.Auth.LoginHint() + default: + return err + } + // A command may have appended guidance of its own to the default hint + // (a partial reorder's rerun note); the remedy replaces the default + // and keeps the rest. + hinted := *e + hinted.Hint = remedy + strings.TrimPrefix(e.Hint, output.DefaultAuthHint) + return &hinted +} + // shouldIncludeStatsInError returns true if stats should be included in the error envelope. func (a *App) shouldIncludeStatsInError() bool { if !a.Flags.Stats || a.Flags.NoStats || a.Collector == nil { diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 5c2f0fe2e..f8e72168b 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -4,6 +4,7 @@ package auth import ( "bufio" "context" + "errors" "fmt" "io" "net" @@ -128,6 +129,63 @@ func (m *Manager) credentialKey() string { return config.NormalizeBaseURL(m.cfg.BaseURL) } +// LoginCommand is the command that re-establishes the active credential: +// addressed to the active profile when there is one, since a bare login +// would store the new credential under the base URL instead. The command +// is meant to be pasted, so the profile name is shell-quoted. +func (m *Manager) LoginCommand() string { + if m.cfg.ActiveProfile != "" { + return "basecamp auth login -P " + shellQuote(m.cfg.ActiveProfile) + } + return "basecamp auth login" +} + +// shellQuote renders s safe to embed in an emitted shell command: a clearly +// inert name passes through bare, anything else is single-quoted — the one +// POSIX form in which nothing substitutes — with embedded single quotes +// spelled '\”. Profile names come from configuration files, which do not +// apply the create-time name check. +func shellQuote(s string) string { + if s != "" && strings.IndexFunc(s, shellActive) < 0 { + return s + } + return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" +} + +// shellActive reports whether r can mean anything to a POSIX shell outside +// quotes; letters, digits and a few inert punctuation marks cannot. +func shellActive(r rune) bool { + inert := r >= 'a' && r <= 'z' || r >= 'A' && r <= 'Z' || r >= '0' && r <= '9' || strings.ContainsRune("_./:@%+=-", r) + return !inert +} + +// LoginHint is LoginCommand as an error hint. +func (m *Manager) LoginHint() string { + return "Run: " + m.LoginCommand() +} + +// errAuth is an auth_required error whose remedy names the active profile. +func (m *Manager) errAuth(msg string) *output.Error { + e := output.ErrAuth(msg) + e.Hint = m.LoginHint() + return e +} + +// hintLogin gives an auth_required error that carries no remedy, or only +// the profile-less default, the login naming the active profile; every +// other error passes through unchanged. The error is copied rather than +// rewritten: a lane client's setup error is cached and returned to every +// later caller. +func (m *Manager) hintLogin(err error) error { + var e *output.Error + if !errors.As(err, &e) || e.Code != output.CodeAuth || (e.Hint != "" && e.Hint != output.DefaultAuthHint) { + return err + } + hinted := *e + hinted.Hint = m.LoginHint() + return &hinted +} + // AccessToken returns a valid access token, refreshing if needed. // If BASECAMP_TOKEN env var is set, it's used directly without OAuth. func (m *Manager) AccessToken(ctx context.Context) (string, error) { @@ -142,7 +200,7 @@ func (m *Manager) AccessToken(ctx context.Context) (string, error) { credKey := m.credentialKey() creds, err := m.store.Load(credKey) if err != nil { - return "", output.ErrAuth(fmt.Sprintf("Not authenticated for %s: %v", credKey, err)) + return "", m.errAuth(fmt.Sprintf("Not authenticated for %s: %v", credKey, err)) } // Check if token is expired (with 5 minute buffer). @@ -155,12 +213,12 @@ func (m *Manager) AccessToken(ctx context.Context) (string, error) { // Reload refreshed credentials creds, err = m.store.Load(credKey) if err != nil { - return "", output.ErrAuth(fmt.Sprintf("Failed to load refreshed credentials for %s: %v", credKey, err)) + return "", m.errAuth(fmt.Sprintf("Failed to load refreshed credentials for %s: %v", credKey, err)) } } if creds.AccessToken == "" { - return "", output.ErrAuth(fmt.Sprintf("Stored credentials for %s have empty access token", credKey)) + return "", m.errAuth(fmt.Sprintf("Stored credentials for %s have empty access token", credKey)) } return creds.AccessToken, nil @@ -176,7 +234,7 @@ func (m *Manager) StoredAccessToken(ctx context.Context) (string, error) { credKey := m.credentialKey() creds, err := m.store.Load(credKey) if err != nil { - return "", output.ErrAuth(fmt.Sprintf("No stored credentials for %s: %v", credKey, err)) + return "", m.errAuth(fmt.Sprintf("No stored credentials for %s: %v", credKey, err)) } // Check if token is expired (with the refresh-window buffer) @@ -188,12 +246,12 @@ func (m *Manager) StoredAccessToken(ctx context.Context) (string, error) { // Reload refreshed credentials creds, err = m.store.Load(credKey) if err != nil { - return "", output.ErrAuth(fmt.Sprintf("Failed to load refreshed credentials for %s: %v", credKey, err)) + return "", m.errAuth(fmt.Sprintf("Failed to load refreshed credentials for %s: %v", credKey, err)) } } if creds.AccessToken == "" { - return "", output.ErrAuth(fmt.Sprintf("Stored credentials for %s have empty access token", credKey)) + return "", m.errAuth(fmt.Sprintf("Stored credentials for %s have empty access token", credKey)) } return creds.AccessToken, nil @@ -223,15 +281,73 @@ func (m *Manager) Refresh(ctx context.Context) error { credKey := m.credentialKey() creds, err := m.store.Load(credKey) if err != nil { - return output.ErrAuth(fmt.Sprintf("Not authenticated for %s: %v", credKey, err)) + return m.errAuth(fmt.Sprintf("Not authenticated for %s: %v", credKey, err)) } return m.refreshLocked(ctx, credKey, creds) } +// invalidGrantPrefix is how the SDK's token exchanger renders an RFC 6749 +// token-endpoint error: it returns the response as an untyped error, so the +// OAuth error code is recoverable only from the message. Coupled to +// basecamp-sdk oauth.Exchanger; a re-pin that types the error can replace +// the string match. +const invalidGrantPrefix = "token error: invalid_grant" + +// invalidGrant reports whether a refresh was refused with invalid_grant — +// the refresh token is expired, revoked, or reused — and returns the +// server's error_description when it sent one. +func invalidGrant(err error) (string, bool) { + rest, ok := strings.CutPrefix(err.Error(), invalidGrantPrefix) + switch { + case !ok: + return "", false + case rest == "": + return "", true + default: + return strings.CutPrefix(rest, " - ") + } +} + +// forgetRefusedGrant deletes the stored credential only while it still +// carries the refresh token the server just refused. Each process has its +// own Manager lock, so two of them can enter the refresh window together: +// the first rotates and saves, the second is refused for reusing the old +// token, and an unconditional delete here would throw away the fresh +// credential the first one stored. The re-read closes that window down to +// the gap between this Load and Delete; a rotation landing inside it is +// lost, which costs one login, and a cross-process lock on a store that is +// usually the OS keyring is not a price worth paying for that. +// +// It reports whether the store holds a credential other than the refused +// one — another process's rotation, which is a live credential the caller +// can reload rather than a session that has ended. +func (m *Manager) forgetRefusedGrant(origin, refusedToken string) (rotated bool) { + current, err := m.store.Load(origin) + if err != nil { + return false + } + if current.RefreshToken != refusedToken { + return true + } + if err := m.store.Delete(origin); err != nil { + m.warnf("could not forget the refused credential for %s: %v", origin, err) + } + return false +} + +// refreshLocked rotates the stored credential under the manager lock. The +// credential is the active profile's, so whatever auth-class failure the +// refresh hits — an unusable stored endpoint, a half-configured OAuth +// client, a refused grant — the remedy is the login that writes that +// profile's credential, not the bare one. func (m *Manager) refreshLocked(ctx context.Context, origin string, creds *Credentials) error { + return m.hintLogin(m.refreshCredential(ctx, origin, creds)) +} + +func (m *Manager) refreshCredential(ctx context.Context, origin string, creds *Credentials) error { if creds.RefreshToken == "" { - return output.ErrAuth("No refresh token available") + return m.errAuth("No refresh token available") } // Migrate old credentials missing OAuthType @@ -242,7 +358,7 @@ func (m *Manager) refreshLocked(ctx context.Context, origin string, creds *Crede // Migrate old credentials missing TokenEndpoint if creds.TokenEndpoint == "" { if creds.OAuthType == "bc3" || creds.OAuthType == oauthTypeBC5 { - return output.ErrAuth("Stored credentials missing token endpoint — please re-authenticate: basecamp auth login") + return m.errAuth("Stored credentials are missing their token endpoint and cannot be refreshed") } lpURL, lpErr := m.launchpadURL() if lpErr != nil { @@ -268,7 +384,7 @@ func (m *Manager) refreshLocked(ctx context.Context, origin string, creds *Crede case "bc3": // DCR-era development flow, removed. Its per-install dynamic clients // can't be resolved anymore, so the refresh token is unusable. - return output.ErrAuth("Stored credentials are from a removed development flow — please re-authenticate: basecamp auth login") + return m.errAuth("Stored credentials are from a removed development flow and cannot be refreshed") case oauthTypeBC5: // Pre-registered public client: no secret. clientID = bc5ClientID @@ -311,7 +427,35 @@ func (m *Manager) refreshLocked(ctx context.Context, origin string, creds *Crede token, err := exchanger.Refresh(ctx, req) if err != nil { - return wrapOAuthError("token refresh failed", err) + desc, dead := invalidGrant(err) + if !dead { + return wrapOAuthError("token refresh failed", err) + } + // The grant is gone for good, so the credential is forgotten now + // rather than re-tried by every later command: Basecamp's abuse + // tracker bans the client and address after a handful of + // invalid_grant failures, which would turn one expired session + // into a lockout. Only a BC5 credential is forgotten: its client is + // the fixed public one, so the refusal can only be about the grant. + // A Launchpad refresh sends whatever client the environment names, + // and the server answers invalid_grant for a token issued to a + // different client too, which is not proof the grant is dead. The + // delete's own outcome cannot change the answer — the session is + // over either way. + if creds.OAuthType == oauthTypeBC5 && m.forgetRefusedGrant(origin, creds.RefreshToken) { + // Another process rotated the credential while this refresh + // was in flight: the store holds a live one, which the callers + // reload, so this refresh has succeeded by proxy. + return nil + } + msg := "Your session has expired or was revoked" + if creds.OAuthType != oauthTypeBC5 { + msg = "The refresh token was refused: the session has expired or was revoked, or BASECAMP_OAUTH_CLIENT_ID/SECRET name a different OAuth client than the one it was issued to" + } + if desc = strings.TrimSpace(richtext.SanitizeSingleLine(desc)); desc != "" { + msg += " (" + desc + ")" + } + return m.errAuth(msg) } creds.AccessToken = token.AccessToken diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 10c842123..8689e89fe 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -1884,3 +1884,281 @@ func TestLoginLaunchpadVerifyRunsBeforeStore(t *testing.T) { _, loadErr := m.store.Load(credKey) assert.Error(t, loadErr, "a rejected token is never stored") } + +// refreshRefusedBy is a token endpoint that answers every refresh with the +// given RFC 6749 error body, and a Manager whose active profile's credential +// refreshes against it. +func refreshRefusedBy(t *testing.T, status int, body string) (*Manager, string) { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + fmt.Fprint(w, body) + })) + t.Cleanup(srv.Close) + + cfg := config.Default() + cfg.ActiveProfile = "work" + m := &Manager{cfg: cfg, httpClient: srv.Client(), store: newTestStore(t, t.TempDir())} + key := m.credentialKey() + require.NoError(t, m.store.Save(key, &Credentials{ + AccessToken: "old-tok", + RefreshToken: "old-ref", + OAuthType: "bc5", + TokenEndpoint: srv.URL + "/oauth/tokens", + Scope: "full", + ExpiresAt: time.Now().Add(-time.Hour).Unix(), + })) + return m, key +} + +// TestRefresh_InvalidGrantForgetsTheCredential: a refresh token the server +// no longer honors is an auth failure with the login to run, and the dead +// credential is deleted so later commands do not keep re-trying it. +func TestRefresh_InvalidGrantForgetsTheCredential(t *testing.T) { + m, key := refreshRefusedBy(t, http.StatusBadRequest, + `{"error":"invalid_grant","error_description":"The refresh token was revoked\u001b[31m"}`) + + err := m.Refresh(context.Background()) + require.Error(t, err) + + var cliErr *output.Error + require.ErrorAs(t, err, &cliErr) + assert.Equal(t, output.CodeAuth, cliErr.Code) + assert.True(t, strings.HasPrefix(cliErr.Message, "Your session has expired or was revoked (The refresh token was revoked"), cliErr.Message) + assert.NotContains(t, cliErr.Message, "\x1b", "the server's description is sanitized for the terminal") + assert.Equal(t, "Run: basecamp auth login -P work", cliErr.Hint) + + _, loadErr := m.store.Load(key) + assert.Error(t, loadErr, "the dead credential must be forgotten") + assert.False(t, m.IsAuthenticated()) +} + +// TestRefresh_InvalidGrantWithoutDescription: the server may send the bare +// error code; that is still the session-over answer. +func TestRefresh_InvalidGrantWithoutDescription(t *testing.T) { + m, key := refreshRefusedBy(t, http.StatusBadRequest, `{"error":"invalid_grant"}`) + + err := m.Refresh(context.Background()) + var cliErr *output.Error + require.ErrorAs(t, err, &cliErr) + assert.Equal(t, output.CodeAuth, cliErr.Code) + assert.Equal(t, "Your session has expired or was revoked", cliErr.Message) + _, loadErr := m.store.Load(key) + assert.Error(t, loadErr) +} + +// TestRefresh_InvalidRequestKeepsTheCredential: any other refusal keeps its +// existing class and leaves the credential in place — a malformed request +// or a server fault says nothing about the grant. +func TestRefresh_InvalidRequestKeepsTheCredential(t *testing.T) { + m, key := refreshRefusedBy(t, http.StatusBadRequest, + `{"error":"invalid_request","error_description":"resource is required"}`) + + err := m.Refresh(context.Background()) + require.Error(t, err) + + var cliErr *output.Error + require.ErrorAs(t, err, &cliErr) + assert.Equal(t, output.CodeAPI, cliErr.Code) + assert.True(t, strings.HasPrefix(cliErr.Message, "token refresh failed: "), cliErr.Message) + + creds, loadErr := m.store.Load(key) + require.NoError(t, loadErr) + assert.Equal(t, "old-ref", creds.RefreshToken) +} + +// TestAccessToken_InvalidGrantHintsTheProfile: the automatic refresh on an +// ordinary command takes the same path as `auth refresh`. +func TestAccessToken_InvalidGrantHintsTheProfile(t *testing.T) { + m, key := refreshRefusedBy(t, http.StatusBadRequest, `{"error":"invalid_grant"}`) + t.Setenv("BASECAMP_TOKEN", "") + + _, err := m.AccessToken(context.Background()) + var cliErr *output.Error + require.ErrorAs(t, err, &cliErr) + assert.Equal(t, output.CodeAuth, cliErr.Code) + assert.Equal(t, "Run: basecamp auth login -P work", cliErr.Hint) + _, loadErr := m.store.Load(key) + assert.Error(t, loadErr) +} + +// TestRefresh_PreservesIdentityAndBinding: a rotation replaces the tokens +// and nothing else — the stored user, scope, and account binding survive a +// token response that does not repeat them. +func TestRefresh_PreservesIdentityAndBinding(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"access_token":"new-tok","refresh_token":"new-ref","expires_in":3600}`) + })) + defer srv.Close() + + m := &Manager{cfg: config.Default(), httpClient: srv.Client(), store: newTestStore(t, t.TempDir())} + key := m.credentialKey() + require.NoError(t, m.store.Save(key, &Credentials{ + AccessToken: "old-tok", + RefreshToken: "old-ref", + OAuthType: "bc5", + TokenEndpoint: srv.URL + "/oauth/tokens", + Scope: "full", + UserID: "51177542", + UserEmail: "bot@example.com", + Resource: "urn:bc:account:999", + ExpiresAt: time.Now().Add(-time.Hour).Unix(), + })) + + require.NoError(t, m.Refresh(context.Background())) + + creds, err := m.store.Load(key) + require.NoError(t, err) + assert.Equal(t, "new-tok", creds.AccessToken) + assert.Equal(t, "new-ref", creds.RefreshToken) + assert.Equal(t, "51177542", creds.UserID) + assert.Equal(t, "bot@example.com", creds.UserEmail) + assert.Equal(t, "full", creds.Scope) + assert.Equal(t, "urn:bc:account:999", creds.Resource) + assert.Equal(t, "bc5", creds.OAuthType) +} + +// TestRefresh_InvalidGrantKeepsAConcurrentlyRotatedCredential: two +// processes can refresh at once; when the other one has already saved the +// rotated token, the refusal this one gets for reusing the old token must +// not delete the fresh credential, and is not a failure: the store holds a +// live credential for the caller to reload. +func TestRefresh_InvalidGrantKeepsAConcurrentlyRotatedCredential(t *testing.T) { + var m *Manager + var key string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // The other process wins the race while this request is in flight. + require.NoError(t, m.store.Save(key, &Credentials{ + AccessToken: "rotated-tok", RefreshToken: "rotated-ref", OAuthType: "bc5", + TokenEndpoint: "http://" + r.Host + "/oauth/tokens", ExpiresAt: time.Now().Add(time.Hour).Unix(), + })) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusBadRequest) + fmt.Fprint(w, `{"error":"invalid_grant"}`) + })) + defer srv.Close() + + m = &Manager{cfg: config.Default(), httpClient: srv.Client(), store: newTestStore(t, t.TempDir())} + key = m.credentialKey() + require.NoError(t, m.store.Save(key, &Credentials{ + AccessToken: "old-tok", RefreshToken: "old-ref", OAuthType: "bc5", + TokenEndpoint: srv.URL + "/oauth/tokens", ExpiresAt: time.Now().Add(-time.Hour).Unix(), + })) + + require.NoError(t, m.Refresh(context.Background()), "the other process's rotation is this refresh's success") + + creds, loadErr := m.store.Load(key) + require.NoError(t, loadErr, "the rotated credential must survive") + assert.Equal(t, "rotated-ref", creds.RefreshToken) + + t.Setenv("BASECAMP_TOKEN", "") + token, err := m.AccessToken(context.Background()) + require.NoError(t, err) + assert.Equal(t, "rotated-tok", token, "the caller reloads the live credential") +} + +// TestRefresh_InvalidGrantOnLaunchpadKeepsTheCredential: a Launchpad +// refresh sends whichever client the environment names, and the server +// also answers invalid_grant for a token issued to another client, so the +// refusal is reported but the credential is not deleted. +func TestRefresh_InvalidGrantOnLaunchpadKeepsTheCredential(t *testing.T) { + t.Setenv("BASECAMP_OAUTH_CLIENT_ID", "custom-id") + t.Setenv("BASECAMP_OAUTH_CLIENT_SECRET", "custom-secret") + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusBadRequest) + fmt.Fprint(w, `{"error":"invalid_grant"}`) + })) + defer srv.Close() + + m := &Manager{cfg: config.Default(), httpClient: srv.Client(), store: newTestStore(t, t.TempDir())} + key := m.credentialKey() + require.NoError(t, m.store.Save(key, &Credentials{ + AccessToken: "old-tok", RefreshToken: "old-ref", OAuthType: oauthTypeLaunchpad, + TokenEndpoint: srv.URL + "/authorization/token", ExpiresAt: time.Now().Add(-time.Hour).Unix(), + })) + + err := m.Refresh(context.Background()) + var cliErr *output.Error + require.ErrorAs(t, err, &cliErr) + assert.Equal(t, output.CodeAuth, cliErr.Code) + assert.Contains(t, cliErr.Message, "BASECAMP_OAUTH_CLIENT_ID/SECRET name a different OAuth client", "the message names the client mismatch the refusal may mean") + assert.Equal(t, "Run: basecamp auth login", cliErr.Hint) + + creds, loadErr := m.store.Load(key) + require.NoError(t, loadErr, "a Launchpad refusal is not proof the grant is dead") + assert.Equal(t, "old-ref", creds.RefreshToken) +} + +// TestLoginCommand_QuotesTheProfile: the remedy is pasted into a shell, and +// profile names loaded from configuration are not checked at load time. +func TestLoginCommand_QuotesTheProfile(t *testing.T) { + for name, want := range map[string]string{ + "": "basecamp auth login", + "work": "basecamp auth login -P work", + "work profile": "basecamp auth login -P 'work profile'", + "it's": `basecamp auth login -P 'it'\''s'`, + "$(rm -rf x)": "basecamp auth login -P '$(rm -rf x)'", + } { + cfg := config.Default() + cfg.ActiveProfile = name + m := &Manager{cfg: cfg} + assert.Equal(t, want, m.LoginCommand(), "profile %q", name) + } +} + +// profiledRefresh is a Manager whose active profile holds the given +// credential, for refresh failures that never reach a token endpoint. +func profiledRefresh(t *testing.T, creds *Credentials) (*Manager, string) { + t.Helper() + cfg := config.Default() + cfg.ActiveProfile = "work" + m := &Manager{cfg: cfg, httpClient: &http.Client{}, store: newTestStore(t, t.TempDir())} + key := m.credentialKey() + creds.ExpiresAt = time.Now().Add(-time.Hour).Unix() + require.NoError(t, m.store.Save(key, creds)) + return m, key +} + +// TestRefresh_UnsafeTokenEndpointHintsTheProfile: a stored endpoint the +// refresh refuses to POST to is an auth failure of the profile's +// credential, and its remedy names the profile like the refused-grant path +// does; the credential is kept, since nothing was learned about the grant. +func TestRefresh_UnsafeTokenEndpointHintsTheProfile(t *testing.T) { + m, key := profiledRefresh(t, &Credentials{ + AccessToken: "old-tok", RefreshToken: "old-ref", OAuthType: "bc5", + TokenEndpoint: "https://user@evil.example/oauth/tokens", + }) + + err := m.Refresh(context.Background()) + var cliErr *output.Error + require.ErrorAs(t, err, &cliErr) + assert.Equal(t, output.CodeAuth, cliErr.Code) + assert.Contains(t, cliErr.Message, "invalid token endpoint") + assert.Equal(t, "Run: basecamp auth login -P work", cliErr.Hint) + + creds, loadErr := m.store.Load(key) + require.NoError(t, loadErr) + assert.Equal(t, "old-ref", creds.RefreshToken) +} + +// TestRefresh_HalfConfiguredClientHintsTheProfile: a Launchpad refresh +// with only one of the OAuth client variables set fails before any request, +// and that failure also names the profile. +func TestRefresh_HalfConfiguredClientHintsTheProfile(t *testing.T) { + t.Setenv("BASECAMP_OAUTH_CLIENT_ID", "custom-id") + t.Setenv("BASECAMP_OAUTH_CLIENT_SECRET", "") + m, _ := profiledRefresh(t, &Credentials{ + AccessToken: "old-tok", RefreshToken: "old-ref", OAuthType: oauthTypeLaunchpad, + TokenEndpoint: "https://launchpad.example/authorization/token", + }) + + err := m.Refresh(context.Background()) + var cliErr *output.Error + require.ErrorAs(t, err, &cliErr) + assert.Equal(t, output.CodeAuth, cliErr.Code) + assert.Contains(t, cliErr.Message, "BASECAMP_OAUTH_CLIENT_SECRET is required") + assert.Equal(t, "Run: basecamp auth login -P work", cliErr.Hint) +} diff --git a/internal/auth/device_test.go b/internal/auth/device_test.go index 0fbea4844..f48b81ae0 100644 --- a/internal/auth/device_test.go +++ b/internal/auth/device_test.go @@ -800,7 +800,10 @@ func TestRefreshLocked_LegacyBC3RequiresReauth(t *testing.T) { err := m.refreshLocked(context.Background(), "test", creds) require.Error(t, err) - assert.Contains(t, err.Error(), "re-authenticate") + var cliErr *output.Error + require.ErrorAs(t, err, &cliErr) + assert.Equal(t, output.CodeAuth, cliErr.Code) + assert.Equal(t, "Run: basecamp auth login", cliErr.Hint) assert.False(t, transport.attempted.Load(), "legacy bc3 refresh must fail without any network request") } diff --git a/internal/commands/auth_remedy_test.go b/internal/commands/auth_remedy_test.go new file mode 100644 index 000000000..6f18ffc55 --- /dev/null +++ b/internal/commands/auth_remedy_test.go @@ -0,0 +1,57 @@ +package commands + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + + "github.com/basecamp/basecamp-cli/internal/output" +) + +// TestAuthTokenStoredKeepsTheLoginHintUnderEnvToken: auth token --stored +// deliberately ignores BASECAMP_TOKEN, so its stored-credential failure +// must keep the login remedy rather than be rewritten to talk about the +// environment token. +func TestAuthTokenStoredKeepsTheLoginHintUnderEnvToken(t *testing.T) { + t.Setenv("BASECAMP_TOKEN", "bc_at_env") + app, out := setupProjectsMockApp(t, unauthorizedTransport{}) + + err := executeCommand(NewAuthCmd(), app, "token", "--stored") + require.Error(t, err) + assert.Equal(t, output.CodeAuth, output.AsError(err).Code) + require.NoError(t, app.Err(err)) + + var envelope struct { + Hint string `json:"hint"` + } + require.NoError(t, json.Unmarshal(out.Bytes(), &envelope), out.String()) + assert.Equal(t, "Run: basecamp auth login", envelope.Hint) +} + +// TestAuthRemedyKeepsGuidanceAppendedToTheDefaultHint: a command that +// appended its own note to the default hint (a partial reorder's rerun +// guidance) keeps that note; only the login prefix is replaced. +func TestAuthRemedyKeepsGuidanceAppendedToTheDefaultHint(t *testing.T) { + t.Setenv("BASECAMP_TOKEN", "") + app, out := setupProjectsMockApp(t, unauthorizedTransport{}) + app.Config.ActiveProfile = "work" + + sdkErr := &basecamp.Error{Code: basecamp.CodeAuth, Message: "authentication required", HTTPStatus: 401} + composed := &output.Error{ + Code: output.CodeAuth, + Message: "Reordered 1 of 3 todolists; failed at #2: authentication required", + Hint: output.DefaultAuthHint + " Rerun the whole command once the cause is fixed.", + Cause: sdkErr, + } + require.NoError(t, app.Err(composed)) + + var envelope struct { + Hint string `json:"hint"` + } + require.NoError(t, json.Unmarshal(out.Bytes(), &envelope), out.String()) + assert.Equal(t, "Run: basecamp auth login -P work Rerun the whole command once the cause is fixed.", envelope.Hint) +} diff --git a/internal/commands/projects.go b/internal/commands/projects.go index 70f4748bd..4cf270ddc 100644 --- a/internal/commands/projects.go +++ b/internal/commands/projects.go @@ -467,14 +467,14 @@ func convertSDKError(err error) error { // Handle structured SDK errors var sdkErr *basecamp.Error if errors.As(err, &sdkErr) { - return &output.Error{ + return output.WithAuthHint(&output.Error{ Code: sdkErr.Code, Message: sdkErr.Message, Hint: sdkErr.Hint, HTTPStatus: sdkErr.HTTPStatus, Retryable: sdkErr.Retryable, Cause: sdkErr, - } + }) } return err } diff --git a/internal/commands/projects_test.go b/internal/commands/projects_test.go index d3a812f1a..e88d85c03 100644 --- a/internal/commands/projects_test.go +++ b/internal/commands/projects_test.go @@ -217,3 +217,73 @@ func TestConvertSDKErrorCarriesTheGateMessageAndHint(t *testing.T) { assert.Equal(t, "Re-run, or lower parallelism.", outErr.Hint) assert.True(t, outErr.Retryable) } + +// unauthorizedTransport answers every request with a bare 401. +type unauthorizedTransport struct{} + +func (unauthorizedTransport) RoundTrip(*http.Request) (*http.Response, error) { + return jsonResponse(http.StatusUnauthorized, `{"error":"authentication required"}`, http.Header{"Content-Type": []string{"application/json"}}), nil +} + +// TestProjectsList401CarriesTheLoginHint: a 401 from the API arrives without +// a remedy, and the rendered error must still say what to run. +func TestProjectsList401CarriesTheLoginHint(t *testing.T) { + app, out := setupProjectsMockApp(t, unauthorizedTransport{}) + + err := executeCommand(NewProjectsCmd(), app, "list") + require.Error(t, err) + + converted := output.AsError(err) + assert.Equal(t, output.CodeAuth, converted.Code) + assert.Equal(t, "Run: basecamp auth login", converted.Hint) + + require.NoError(t, app.Err(err)) + var envelope struct { + OK bool `json:"ok"` + Code string `json:"code"` + Hint string `json:"hint"` + } + require.NoError(t, json.Unmarshal(out.Bytes(), &envelope), out.String()) + assert.False(t, envelope.OK) + assert.Equal(t, "auth_required", envelope.Code) + assert.Equal(t, "Run: basecamp auth login", envelope.Hint) +} + +// TestProjectsList401HintNamesTheProfile: the rendered remedy must be the +// login that repairs the credential the command used. +func TestProjectsList401HintNamesTheProfile(t *testing.T) { + t.Setenv("BASECAMP_TOKEN", "") + app, out := setupProjectsMockApp(t, unauthorizedTransport{}) + app.Config.ActiveProfile = "work" + + err := executeCommand(NewProjectsCmd(), app, "list") + require.Error(t, err) + require.NoError(t, app.Err(err)) + + var envelope struct { + Code string `json:"code"` + Hint string `json:"hint"` + } + require.NoError(t, json.Unmarshal(out.Bytes(), &envelope), out.String()) + assert.Equal(t, "auth_required", envelope.Code) + assert.Equal(t, "Run: basecamp auth login -P work", envelope.Hint) +} + +// TestProjectsList401HintUnderEnvToken: a login cannot help while +// BASECAMP_TOKEN shadows every stored credential, so the remedy names the +// variable instead. +func TestProjectsList401HintUnderEnvToken(t *testing.T) { + t.Setenv("BASECAMP_TOKEN", "bc_at_rejected") + app, out := setupProjectsMockApp(t, unauthorizedTransport{}) + + err := executeCommand(NewProjectsCmd(), app, "list") + require.Error(t, err) + require.NoError(t, app.Err(err)) + + var envelope struct { + Hint string `json:"hint"` + } + require.NoError(t, json.Unmarshal(out.Bytes(), &envelope), out.String()) + assert.Contains(t, envelope.Hint, "BASECAMP_TOKEN is set") + assert.NotContains(t, envelope.Hint, "auth login") +} diff --git a/internal/output/errors.go b/internal/output/errors.go index d9ec24c57..d48b0a0f6 100644 --- a/internal/output/errors.go +++ b/internal/output/errors.go @@ -33,10 +33,17 @@ func ErrAmbiguous(resource string, matches []string) *Error { return clioutput.ErrAmbiguous(resource, matches) } +// AsError converts err to the CLI's structured error. An *Error already in +// the chain is the CLI's own verdict and wins over the SDK error it may +// wrap as its cause; only a bare SDK error is converted from its taxonomy. func AsError(err error) *Error { if gateErr := AsGateError(err); gateErr != nil { return gateErr } + var cliErr *Error + if errors.As(err, &cliErr) { + return WithAuthHint(cliErr) + } var sdkErr *basecamp.Error if errors.As(err, &sdkErr) { message := err.Error() @@ -46,16 +53,37 @@ func AsError(err error) *Error { if message == "" { message = sdkErr.Message } - return &Error{ + return WithAuthHint(&Error{ Code: sdkErr.Code, Message: message, Hint: sdkErr.Hint, HTTPStatus: sdkErr.HTTPStatus, Retryable: sdkErr.Retryable, Cause: sdkErr, - } + }) + } + return WithAuthHint(clioutput.AsError(err)) +} + +// DefaultAuthHint is the remedy an auth_required error carries when nothing +// closer to the credential has named one. The app boundary (appctx.App.Err) +// replaces it with a remedy that knows the active profile and whether +// BASECAMP_TOKEN is in play; this is the floor beneath that. +const DefaultAuthHint = "Run: basecamp auth login" + +// WithAuthHint adds the login remedy to an auth_required error that has +// none. A 401 from the API and the SDK's own auth errors arrive without a +// hint, and "authentication required" alone leaves the reader to guess what +// to run. Errors that already carry a hint, or are not auth errors, pass +// through untouched; a hinted copy is returned so the caller's error is not +// rewritten under it. +func WithAuthHint(e *Error) *Error { + if e.Code != CodeAuth || e.Hint != "" { + return e } - return clioutput.AsError(err) + hinted := *e + hinted.Hint = DefaultAuthHint + return &hinted } // AsGateError converts a resilience gate rejection, which arrives through @@ -90,7 +118,7 @@ func ErrAuth(msg string) *Error { return &Error{ Code: CodeAuth, Message: msg, - Hint: "Run: basecamp auth login", + Hint: DefaultAuthHint, } }