Refuse credentials from a newer CLI, and stop refreshing a refused client

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
 	}

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)

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")
+}