diff --git a/internal/auth/agent.go b/internal/auth/agent.go index 5e9217e58..551b7f89f 100644 --- a/internal/auth/agent.go +++ b/internal/auth/agent.go @@ -192,7 +192,7 @@ func (m *Manager) mintAgentCredential(ctx context.Context, origin string, creds token, hold, err := m.mintAgentToken(ctx, mint) if err != nil { if hold != nil { - m.rememberRenewalHold(origin, hold) + m.rememberRenewalHold(origin, hold.Client, hold) } return err } diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 06199b6bd..7c06d0a19 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -505,6 +505,27 @@ func invalidGrant(err error) (string, bool) { return "", false } +// invalidClient reports whether a refresh was refused with invalid_client: +// the token endpoint does not accept the client the refresh was sent as. +// Read off the SDK's typed refusal, as invalidGrant is, and only from the +// statuses RFC 6749 §5.2 gives that refusal: a 429 or a server fault naming +// the client is not a verdict on it. +func invalidClient(err error) bool { + var refusal *basecamp.Error + return errors.As(err, &refusal) && refusal.OAuthError == "invalid_client" && + (refusal.HTTPStatus == http.StatusBadRequest || refusal.HTTPStatus == http.StatusUnauthorized) +} + +// errNewerCredential is the refusal for a stored credential whose OAuth type +// this version does not know. The type is quoted, so a damaged store cannot +// put terminal controls into the message. +func (m *Manager) errNewerCredential(oauthType string) *output.Error { + e := m.errAuth(fmt.Sprintf("These credentials were written by a newer basecamp CLI (OAuth type %q) and this version cannot refresh them", oauthType)) + command, _ := m.loginRemedy() + e.Hint = "Run: basecamp upgrade (or, to keep this version, log in again with it: " + command + ")" + return e +} + // forgetRefusedGrant deletes the stored credential only while it still // carries the refresh token the server just refused. Two processes used to // be able to enter the refresh window together — the first rotating and @@ -566,15 +587,28 @@ func (m *Manager) refreshLocked(ctx context.Context, origin string, creds *Crede // the credential's missing fields in place, checks what it holds, and // resolves the client and lane the request would go out through. func (m *Manager) prepareRefresh(creds *Credentials) (oauth.RefreshRequest, *oauth.Exchanger, error) { - if creds.RefreshToken == "" { - return oauth.RefreshRequest{}, nil, m.errAuth("No refresh token available") - } - // Migrate old credentials missing OAuthType if creds.OAuthType == "" { creds.OAuthType = oauthTypeLaunchpad } + // Only the types this version knows how to refresh go any further. A + // type it has never heard of was written by a newer CLI, whose client + // and token endpoint this one cannot know: refreshing it as Launchpad + // sends Launchpad's client to someone else's token endpoint, which + // answers invalid_client on every command, forever. Checked before + // anything the credential holds, since a newer type need not hold what + // this version's refresh asks for. + switch creds.OAuthType { + case "bc3", oauthTypeBC5, oauthTypeLaunchpad: + default: + return oauth.RefreshRequest{}, nil, m.errNewerCredential(creds.OAuthType) + } + + if creds.RefreshToken == "" { + return oauth.RefreshRequest{}, nil, m.errAuth("No refresh token available") + } + // Migrate old credentials missing TokenEndpoint if creds.TokenEndpoint == "" { if creds.OAuthType == "bc3" || creds.OAuthType == oauthTypeBC5 { @@ -608,7 +642,7 @@ func (m *Manager) prepareRefresh(creds *Credentials) (oauth.RefreshRequest, *oau case oauthTypeBC5: // Pre-registered public client: no secret. clientID = bc5ClientID - default: + case oauthTypeLaunchpad: // Launchpad (or old credentials defaulted to launchpad) if envCreds, err := resolveClientCredentials(func(string) {}); err != nil { return oauth.RefreshRequest{}, nil, err @@ -633,6 +667,12 @@ func (m *Manager) prepareRefresh(creds *Credentials) (oauth.RefreshRequest, *oau return oauth.RefreshRequest{}, nil, laneErr } + // A client the token endpoint already refused for this refresh token + // is refused again here, before anything is sent. + if err := m.refusedRefreshClient(creds, clientID, clientSecret); err != nil { + return oauth.RefreshRequest{}, nil, err + } + // A rate limit the token endpoint already put on this refresh token is // answered last, once everything local has resolved and only the // request is left — which is also what lets a report say the next @@ -669,6 +709,9 @@ func (m *Manager) refreshCredential(ctx context.Context, origin string, creds *C if held := m.holdRateLimitedRefresh(origin, creds, err); held != nil { return held } + if invalidClient(err) { + return m.holdRefusedRefreshClient(origin, creds, req.ClientID, req.ClientSecret, err) + } desc, dead := invalidGrant(err) if !dead { return wrapOAuthError("token refresh failed", err) diff --git a/internal/auth/refresh_client_test.go b/internal/auth/refresh_client_test.go new file mode 100644 index 000000000..fedbbf0c6 --- /dev/null +++ b/internal/auth/refresh_client_test.go @@ -0,0 +1,217 @@ +package auth + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + "time" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/config" + "github.com/basecamp/basecamp-cli/internal/output" +) + +// A credential of a type this version has never heard of was written by a +// newer CLI. It is refused before anything is sent — not refreshed as +// Launchpad, which sent Launchpad's client to the newer login's token +// endpoint and drew invalid_client on every command — and the refusal says +// to upgrade. +func TestRefresh_UnknownOAuthTypeRefusesWithoutSending(t *testing.T) { + for name, tc := range map[string]struct{ endpoint, refreshToken string }{ + "with its token endpoint": {"https://3.basecampapi.com/oauth/tokens", "old-ref"}, + // Launchpad's migration would have filled one in. + "without one": {"", "old-ref"}, + // A newer type need not renew by refresh token at all, and is + // still named for what it is rather than as a missing token. + "without a refresh token": {"https://3.basecampapi.com/oauth/tokens", ""}, + } { + endpoint := tc.endpoint + t.Run(name, func(t *testing.T) { + t.Setenv("BASECAMP_OAUTH_CLIENT_ID", "") + t.Setenv("BASECAMP_OAUTH_CLIENT_SECRET", "") + transport := &recordingTransport{} + cfg := config.Default() + cfg.ActiveProfile = "work" + m := &Manager{cfg: cfg, httpClient: &http.Client{Transport: transport}, store: newTestStore(t, t.TempDir())} + creds := &Credentials{ + AccessToken: "old-tok", + RefreshToken: tc.refreshToken, + OAuthType: "bc6\x1b[31m", + TokenEndpoint: endpoint, + ExpiresAt: time.Now().Add(-time.Hour).Unix(), + } + key := m.credentialKey() + require.NoError(t, m.store.Save(key, creds)) + + 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, "newer basecamp CLI") + assert.Contains(t, cliErr.Message, `"bc6\x1b[31m"`, "the type is quoted, never written raw") + assert.NotContains(t, cliErr.Message, "\x1b") + assert.Equal(t, "Run: basecamp upgrade (or, to keep this version, log in again with it: basecamp auth login -P work)", cliErr.Hint) + assert.False(t, transport.attempted.Load(), "an unknown type must be refused without any network request") + + stored, loadErr := m.store.Load(key) + require.NoError(t, loadErr, "the newer CLI's credential is left for the newer CLI") + assert.Equal(t, "bc6\x1b[31m", stored.OAuthType) + assert.Equal(t, endpoint, stored.TokenEndpoint, "nothing is migrated onto it") + + refusal := m.RefreshRefusal(stored) + require.Error(t, refusal) + assert.Contains(t, output.AsError(refusal).Message, "newer basecamp CLI", "the report says what the next command will do") + }) + } +} + +// invalidClientRefresh is a token endpoint that answers every refresh with +// status and an invalid_client body, counting what reaches it, and a Manager +// whose active profile's credential of oauthType refreshes against it. +func invalidClientRefresh(t *testing.T, status int, oauthType string) (*Manager, string, *atomic.Int32) { + t.Helper() + var hits atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hits.Add(1) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + fmt.Fprint(w, `{"error":"invalid_client","error_description":"Client authentication failed"}`) + })) + t.Cleanup(srv.Close) + + cfg := config.Default() + cfg.ActiveProfile = "work" + cfg.BaseURL = srv.URL + 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: oauthType, + TokenEndpoint: srv.URL + "/oauth/tokens", + ExpiresAt: time.Now().Add(-time.Hour).Unix(), + })) + return m, key, &hits +} + +// invalid_client on a refresh is terminal: the token endpoint does not know +// the client, and asking again as the same client cannot change that. The +// first refusal tells the person to log in again; every later command, in +// this process or another, answers from the store and sends nothing — each +// resend is charged to the abuse tracker that blocks the address. +func TestRefresh_InvalidClientIsTerminal(t *testing.T) { + for _, oauthType := range []string{oauthTypeBC5, oauthTypeLaunchpad} { + t.Run(oauthType, func(t *testing.T) { + t.Setenv("BASECAMP_OAUTH_CLIENT_ID", "") + t.Setenv("BASECAMP_OAUTH_CLIENT_SECRET", "") + t.Setenv("BASECAMP_TOKEN", "") + m, key, hits := invalidClientRefresh(t, http.StatusUnauthorized, oauthType) + + 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_client") + assert.Contains(t, cliErr.Message, "will not be sent again") + assert.Equal(t, "Run: basecamp auth login -P work", cliErr.Hint) + var refusal *basecamp.Error + require.ErrorAs(t, err, &refusal, "the SDK's refusal stays in the chain") + assert.Equal(t, int32(1), hits.Load()) + + stored, loadErr := m.store.Load(key) + require.NoError(t, loadErr, "the credential is kept: a hold is undone by a login, a delete is not undone") + require.NotNil(t, stored.RenewalHold) + assert.Equal(t, renewalHoldRefused, stored.RenewalHold.Kind) + assert.Zero(t, stored.RenewalHold.Until, "invalid_client is never rechecked") + + for range 3 { + _, err = m.AccessToken(context.Background()) + require.ErrorAs(t, err, &cliErr) + assert.Equal(t, output.CodeAuth, cliErr.Code) + assert.Contains(t, cliErr.Message, "remembered") + assert.Equal(t, "Run: basecamp auth login -P work", cliErr.Hint) + } + fresh := &Manager{cfg: m.cfg, httpClient: m.httpClient, store: m.store} + require.Error(t, fresh.Refresh(context.Background())) + assert.Equal(t, int32(1), hits.Load(), "a refused client was sent again") + + status := m.RenewalHoldStatus(stored, m.RefreshRefusal(stored)) + require.NotNil(t, status, "the report shows the hold") + assert.True(t, status.Permanent()) + + // A login writes a new refresh token, which the hold is not about. + stored.RefreshToken = "new-ref" + require.NoError(t, m.store.Save(key, stored)) + require.Error(t, m.Refresh(context.Background())) + assert.Equal(t, int32(2), hits.Load(), "a new login was held to the old one's refusal") + }) + } +} + +// A Launchpad refresh is sent as whatever client the environment names, so +// a refusal is held for that client only: fixing BASECAMP_OAUTH_CLIENT_ID +// sends the refresh again, without a login. +func TestRefresh_InvalidClientHoldIsForTheClientItRefused(t *testing.T) { + t.Setenv("BASECAMP_OAUTH_CLIENT_ID", "wrong-client") + t.Setenv("BASECAMP_OAUTH_CLIENT_SECRET", "wrong-secret") + m, _, hits := invalidClientRefresh(t, http.StatusUnauthorized, oauthTypeLaunchpad) + + err := m.Refresh(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "BASECAMP_OAUTH_CLIENT_ID/SECRET") + require.Error(t, m.Refresh(context.Background())) + assert.Equal(t, int32(1), hits.Load()) + + t.Setenv("BASECAMP_OAUTH_CLIENT_ID", "") + t.Setenv("BASECAMP_OAUTH_CLIENT_SECRET", "") + require.Error(t, m.Refresh(context.Background())) + assert.Equal(t, int32(2), hits.Load(), "a refresh as another client was held to this one's refusal") +} + +// The status decides, as it does for an agent's mint: a server fault or a +// rate limit that happens to name invalid_client is no verdict on the +// client, and holds nothing of the kind. +func TestRefresh_InvalidClientFromAServerFaultIsNotTerminal(t *testing.T) { + m, key, hits := invalidClientRefresh(t, http.StatusServiceUnavailable, oauthTypeBC5) + + err := m.Refresh(context.Background()) + require.Error(t, err) + assert.NotContains(t, err.Error(), "will not be sent again") + stored, loadErr := m.store.Load(key) + require.NoError(t, loadErr) + assert.Nil(t, stored.RenewalHold) + + _ = m.Refresh(context.Background()) + assert.Equal(t, int32(2), hits.Load(), "a server fault is tried again") +} + +// invalidClient is read off the typed refusal and its status, never the text. +func TestInvalidClient_ReadsTheTypedCodeAndStatus(t *testing.T) { + assert.True(t, invalidClient(fmt.Errorf("wrapped: %w", &basecamp.Error{HTTPStatus: 401, OAuthError: "invalid_client"}))) + assert.True(t, invalidClient(&basecamp.Error{HTTPStatus: 400, OAuthError: "invalid_client"})) + assert.False(t, invalidClient(&basecamp.Error{HTTPStatus: 429, OAuthError: "invalid_client"})) + assert.False(t, invalidClient(&basecamp.Error{HTTPStatus: 500, OAuthError: "invalid_client"})) + assert.False(t, invalidClient(&basecamp.Error{HTTPStatus: 401, OAuthError: "invalid_grant", Message: "invalid_client"})) + assert.False(t, invalidClient(fmt.Errorf("token error: invalid_client"))) +} + +// The refusal promises that the refresh will not be sent again only when the +// hold that keeps that promise was stored. +func TestRefresh_InvalidClientPromisesOnlyAStoredHold(t *testing.T) { + m := &Manager{cfg: config.Default(), store: newTestStore(t, t.TempDir())} + creds := &Credentials{RefreshToken: "old-ref", OAuthType: oauthTypeBC5} + refusal := &basecamp.Error{HTTPStatus: 401, OAuthError: "invalid_client"} + + err := m.holdRefusedRefreshClient("nowhere", creds, bc5ClientID, "", refusal) + var cliErr *output.Error + require.ErrorAs(t, err, &cliErr) + assert.Equal(t, output.CodeAuth, cliErr.Code) + assert.Contains(t, cliErr.Message, "invalid_client") + assert.NotContains(t, cliErr.Message, "will not be sent again", "nothing was stored to keep that promise") +} diff --git a/internal/auth/renewal_hold.go b/internal/auth/renewal_hold.go index b4e5b5d9c..f477818a4 100644 --- a/internal/auth/renewal_hold.go +++ b/internal/auth/renewal_hold.go @@ -110,8 +110,9 @@ const MaxServerWait = maxAgentConnectLifetime // RenewalHold is the token endpoint's last refusal of a credential's renewal // — an agent's mint, or anyone else's refresh — remembered so the next -// renewal can answer it without asking again. A refresh is only ever held -// for a rate limit (holdRateLimitedRefresh). +// renewal can answer it without asking again. A refresh is held for a rate +// limit (holdRateLimitedRefresh) or for an invalid_client refusal of the +// client it was sent as (holdRefusedRefreshClient). // // Nothing in it came from the server as free text: Detail is the same fixed // vocabulary agentMintRefusal renders (see oauthErrorCodes). @@ -184,7 +185,7 @@ func (m *Manager) holdRateLimitedRefresh(origin string, creds *Credentials, err Client: refreshTokenFingerprint(creds.RefreshToken), Until: ceilUnix(now.Add(min(wait, MaxServerWait))), } - m.rememberRenewalHold(origin, hold) + m.rememberRenewalHold(origin, hold.Client, hold) held := time.Unix(hold.Until, 0).UTC().Format(time.RFC3339) var msg string @@ -209,6 +210,72 @@ func (m *Manager) holdRateLimitedRefresh(origin string, creds *Credentials, err return e } +// refreshClientFingerprint names a refresh token together with the client it +// is presented as, for the one hold that is about the pair: an invalid_client +// refusal. A Launchpad refresh is sent as whatever client +// BASECAMP_OAUTH_CLIENT_ID/SECRET name, so the same refresh token can be +// refused as one client and accepted as another; a hold on the token alone +// would outlive the fix. +func refreshClientFingerprint(refreshToken, clientID, clientSecret string) string { + sum := sha256.Sum256([]byte("basecamp-cli refresh client hold\x00" + refreshToken + "\x00" + clientID + "\x00" + clientSecret)) + return hex.EncodeToString(sum[:16]) +} + +// holdRefusedRefreshClient remembers an invalid_client answer to a refresh +// and returns the auth error it is answered with. +// +// invalid_client is not a refusal that waits out: the token endpoint does not +// know the client the refresh was sent as, and asking again as the same +// client cannot change that. Every command that finds the access token near +// expiry would otherwise send the same refresh, and a scheduled job sends it +// on every run — each one charged to the abuse tracker that blocks the +// address after a handful. So it is held with no recheck, for exactly this +// refresh token and client (refreshClientFingerprint); a login writes a +// credential with no hold, and a changed client is not held. +func (m *Manager) holdRefusedRefreshClient(origin string, creds *Credentials, clientID, clientSecret string, err error) error { + stored := m.rememberRenewalHold(origin, refreshTokenFingerprint(creds.RefreshToken), &RenewalHold{ + Kind: renewalHoldRefused, + Detail: invalidClientDetail, + Client: refreshClientFingerprint(creds.RefreshToken, clientID, clientSecret), + }) + msg := refusedRefreshClientMessage(creds) + if stored { + msg += "; it will not be sent again" + } + e := m.errAuth(msg) + // The SDK's refusal stays in the chain, so its request id and OAuth + // error still reach the error envelope. + e.Cause = err + return e +} + +// refusedRefreshClient is the error a remembered invalid_client answers a +// refresh with, or nil when the refresh may be sent. A hold for another +// refresh token or another client holds nothing. +func (m *Manager) refusedRefreshClient(creds *Credentials, clientID, clientSecret string) error { + hold := creds.RenewalHold + if hold == nil || creds.OAuthType == oauthTypeAgent || hold.Kind != renewalHoldRefused || + hold.Until != 0 || hold.Detail != invalidClientDetail || + hold.Client != refreshClientFingerprint(creds.RefreshToken, clientID, clientSecret) { + return nil + } + e := m.errAuth(refusedRefreshClientMessage(creds) + "; the refusal is remembered, and this refresh will not be sent again") + e.Cause = errRenewalHeld + return e +} + +// refusedRefreshClientMessage says what an invalid_client refusal of creds' +// refresh means. A BC5 refresh is sent as the fixed public client, so the +// login itself is the remedy; a Launchpad one may have been sent as a client +// the environment names. +func refusedRefreshClientMessage(creds *Credentials) string { + msg := "The token endpoint refused the OAuth client this login refreshes as (" + invalidClientDetail + ")" + if creds.OAuthType == oauthTypeLaunchpad { + msg += ", either because the login is not one this client can refresh or because BASECAMP_OAUTH_CLIENT_ID/SECRET name a different client" + } + return msg +} + // refreshTokenFingerprint names a refresh token the way // agentClientFingerprint names a client: a truncated SHA-256 under a label of // its own, so a hold can say which refresh token it is about without @@ -375,8 +442,10 @@ func (hold *RenewalHold) holds(creds *Credentials, now time.Time) bool { if hold == nil || hold.Client != renewalSubject(creds) { return false } - // A refresh is only ever held for a rate limit: a refused refresh token - // is forgotten outright rather than held (refreshCredential). + // A refresh is held here only for a rate limit. A refused grant is + // forgotten outright rather than held (refreshCredential), and a refused + // client is held for the client it was sent as, which only + // refusedRefreshClient can read. if creds.OAuthType != oauthTypeAgent && hold.Kind != renewalHoldRateLimited { return false } @@ -410,8 +479,9 @@ func (hold *RenewalHold) holds(creds *Credentials, now time.Time) bool { // instead would leave exactly those hosts minting on every poll. // // A failure to write is reported and otherwise changes nothing — the -// refusal is what the caller sees either way. -func (m *Manager) rememberRenewalHold(origin string, hold *RenewalHold) { +// refusal is what the caller sees either way. It reports whether the hold +// was stored, for a caller whose message promises what later commands do. +func (m *Manager) rememberRenewalHold(origin, about string, hold *RenewalHold) (stored bool) { current, err := m.store.Load(origin) if err != nil { // A credential removed since the mint (a logout) has nothing to @@ -419,15 +489,17 @@ func (m *Manager) rememberRenewalHold(origin string, hold *RenewalHold) { if !errors.Is(err, ErrNoCredential) { m.warnf("warning: could not read the credential for %s to remember the token endpoint's refusal, so the next command will ask again: %v", origin, err) } - return + return false } - if renewalSubject(current) != hold.Client { - return + if renewalSubject(current) != about { + return false } current.RenewalHold = hold if err := m.store.Save(origin, current); err != nil { m.warnf("warning: could not remember the token endpoint's refusal for %s, so the next command will ask again: %v", origin, err) + return false } + return true } // now is the Manager's clock: the clock field when a test set one. diff --git a/internal/auth/renewal_hold_test.go b/internal/auth/renewal_hold_test.go index c52736e19..355b22c3f 100644 --- a/internal/auth/renewal_hold_test.go +++ b/internal/auth/renewal_hold_test.go @@ -360,7 +360,7 @@ func TestAStaleRefusalNeverLandsOnANewSecret(t *testing.T) { relogged.ClientSecret = "a-new-secret" require.NoError(t, m.store.Save(key, relogged)) - m.rememberRenewalHold(key, stale) + m.rememberRenewalHold(key, stale.Client, stale) stored, err := m.store.Load(key) require.NoError(t, err) @@ -489,12 +489,12 @@ func TestAFailedReadIsReportedWhenARefusalCannotBeRemembered(t *testing.T) { var warned []string m.Warnf = func(format string, args ...any) { warned = append(warned, fmt.Sprintf(format, args...)) } - m.rememberRenewalHold("nowhere", &RenewalHold{Kind: renewalHoldRefused}) + m.rememberRenewalHold("nowhere", "", &RenewalHold{Kind: renewalHoldRefused}) assert.Empty(t, warned, "a credential that is gone has nothing to remember a refusal on") m.store = &Store{inner: unreadableStore{}} m.store.initOnce.Do(func() {}) - m.rememberRenewalHold(key, &RenewalHold{Kind: renewalHoldRefused}) + m.rememberRenewalHold(key, "", &RenewalHold{Kind: renewalHoldRefused}) require.Len(t, warned, 1) assert.Contains(t, warned[0], "could not read the credential") } diff --git a/internal/commands/auth.go b/internal/commands/auth.go index e2f85e288..c561fe0dc 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -458,7 +458,13 @@ func authStatusReport(ctx context.Context, app *appctx.App) (*authStatus, error) case !expired && hold != nil: // A stored hold that never ends: the next renewal will be // refused, and the line says so before the token runs out. - expiry = "expires in " + coarseDuration(expiresIn) + "; renewal would be refused: " + hold.Detail + "; log in with a new secret" + // An agent's refusal is of its secret; anyone else's is of the + // client a refresh is sent as, which a login replaces. + remedy := "log in again" + if creds.OAuthType == "agent" { + remedy = "log in with a new secret" + } + expiry = "expires in " + coarseDuration(expiresIn) + "; renewal would be refused: " + hold.Detail + "; " + remedy report.hint = remedyFor(app, refusal) case !expired: expiry = "expires in " + coarseDuration(expiresIn) diff --git a/internal/commands/auth_status_agent_test.go b/internal/commands/auth_status_agent_test.go index 2c9cac0aa..c8372a2f1 100644 --- a/internal/commands/auth_status_agent_test.go +++ b/internal/commands/auth_status_agent_test.go @@ -224,6 +224,52 @@ func TestAuthStatusNamesAPermanentHoldWhileTheTokenIsLive(t *testing.T) { assert.Contains(t, report.hint, "--with-client-credentials") } +// TestAuthStatusNamesARefusedRefreshClientsRemedy: a login's refresh held +// for invalid_client is a refusal of the client the refresh is sent as, not +// of an agent's secret, so the line names a login, not a new secret. +func TestAuthStatusNamesARefusedRefreshClientsRemedy(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + t.Setenv("BASECAMP_TOKEN", "") + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte(`{"error":"invalid_client"}`)) + })) + t.Cleanup(srv.Close) + + cfg := &config.Config{BaseURL: srv.URL, ActiveProfile: "work", Sources: map[string]string{}} + authMgr := auth.NewManager(cfg, srv.Client()) + store := auth.NewStore(config.GlobalConfigDir()) + authMgr.SetStore(store) + require.NoError(t, store.Save("profile:work", &auth.Credentials{ + AccessToken: "spent", + RefreshToken: "refused-ref", + OAuthType: "bc5", + Scope: "full", + TokenEndpoint: srv.URL + "/oauth/tokens", + ExpiresAt: time.Now().Add(-time.Minute).Unix(), + })) + _, err := authMgr.AccessToken(context.Background()) + require.Error(t, err) + + creds, err := store.Load("profile:work") + require.NoError(t, err) + require.NotNil(t, creds.RenewalHold) + creds.AccessToken = "still-good" + creds.ExpiresAt = time.Now().Add(time.Hour).Unix() + require.NoError(t, store.Save("profile:work", creds)) + + app := &appctx.App{Config: cfg, Auth: authMgr} + report, err := authStatusReport(context.Background(), app) + require.NoError(t, err) + joined := strings.Join(report.details, "\n") + assert.Contains(t, joined, "renewal would be refused: token error: invalid_client; log in again") + assert.NotContains(t, joined, "new secret") + assert.Contains(t, report.hint, "basecamp auth login -P work") +} + // TestAuthStatusReportsATimeLimitedHoldAsRefreshable: a hold that ends on // its own — a 429 until its Retry-After, a refusal rechecked hourly — is // what a renewal sent now would meet, and nothing more. The credential is