diff --git a/internal/apierr/sdk.go b/internal/apierr/sdk.go index 5b1b0081..fa209e90 100644 --- a/internal/apierr/sdk.go +++ b/internal/apierr/sdk.go @@ -1,6 +1,7 @@ package apierr import ( + "errors" "fmt" hey "github.com/basecamp/hey-sdk/go/pkg/hey" @@ -15,6 +16,16 @@ func FromSDK(err error) error { return nil } + // Not every error that comes back from an SDK call was made by the SDK. The + // auth strategy is ours, and the SDK returns what it hands back untouched, so + // a credential failure arrives here already classified. hey.AsError only + // recognizes the SDK's own type and would flatten it to "api" — losing the + // auth exit code and the hint that says how to fix it. + var cliErr *Error + if errors.As(err, &cliErr) { + return cliErr + } + sdkErr := hey.AsError(err) switch sdkErr.Code { case hey.CodeAuth: diff --git a/internal/apierr/sdk_test.go b/internal/apierr/sdk_test.go index f96f5bbe..b4b14f95 100644 --- a/internal/apierr/sdk_test.go +++ b/internal/apierr/sdk_test.go @@ -2,6 +2,7 @@ package apierr import ( "errors" + "fmt" "testing" hey "github.com/basecamp/hey-sdk/go/pkg/hey" @@ -132,3 +133,35 @@ func TestFromSDKKeepsTheCauseReachable(t *testing.T) { t.Error("the SDK error a validation failure came from must stay reachable through errors.Is") } } + +// The CLI's own auth strategy runs inside SDK calls, and the SDK hands back what it +// returns untouched. Such an error arrives here already classified; flattening it to +// "api" would cost the auth exit code and the hint that says how to fix it. +func TestFromSDKKeepsAnAlreadyClassifiedError(t *testing.T) { + tests := []struct { + name string + err error + }{ + {name: "bare", err: ErrAuth("not authenticated")}, + {name: "wrapped", err: fmt.Errorf("reading changes: %w", ErrAuth("not authenticated"))}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := AsError(FromSDK(tt.err)) + if got.Code != CodeAuth { + t.Errorf("code = %q, want %q", got.Code, CodeAuth) + } + if got.Hint == "" { + t.Error("the login hint was dropped") + } + }) + } +} + +func TestFromSDKStillMapsRateLimitedCLIErrors(t *testing.T) { + got := AsError(FromSDK(&Error{Code: CodeRateLimit, Message: "rate limited", HTTPStatus: 429})) + if got.Code != CodeRateLimit { + t.Errorf("code = %q, want %q", got.Code, CodeRateLimit) + } +} diff --git a/internal/auth/auth.go b/internal/auth/auth.go index b50e7e1a..73789402 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -12,6 +12,8 @@ import ( "strings" "sync" "time" + + "github.com/basecamp/hey-cli/internal/apierr" ) // Built-in OAuth client ID for the CLI app. @@ -32,8 +34,26 @@ type Manager struct { callbackWait callbackWaiter listen listenerFactory mu sync.Mutex + + // refreshHoldUntil parks refreshes after the token endpoint rate-limited one. + // Guarded by mu, which every path into refreshLocked already holds. + refreshHoldUntil time.Time + + // refusedRefreshToken is a grant the server refused that the store could not + // delete, so this process remembers not to send it again. Guarded by mu. + refusedRefreshToken string + + // credentialCleared runs after the manager has deleted a credential on its own + // verdict, so the owner of the response cache can drop what that credential + // fetched. Logout is not that: its callers already clear the cache themselves. + credentialCleared func() } +// defaultRefreshHold is how long to sit out a rate limit that came without a +// Retry-After. The token endpoint's window is an hour and it counts refusals, so a +// `hey watch` redialling every fifteen seconds would spend it in minutes. +const defaultRefreshHold = 15 * time.Minute + // NewManager creates a new auth manager. func NewManager(baseURL string, httpClient *http.Client, configDir string) *Manager { listenConfig := &net.ListenConfig{} @@ -62,7 +82,7 @@ func (m *Manager) AccessToken(ctx context.Context) (string, error) { creds, err := m.store.Load(m.baseURL) if err != nil { - return "", fmt.Errorf("not authenticated: %w", err) + return "", errNotAuthenticated(err) } // Check if token is expired (with 5-minute buffer) @@ -85,7 +105,7 @@ func (m *Manager) AccessToken(ctx context.Context) (string, error) { return creds.SessionCookie, nil } - return "", fmt.Errorf("no access token or session cookie available") + return "", errNoCredential("no access token or session cookie available", nil) } // AuthenticateRequest sets the appropriate auth header on an HTTP request. @@ -101,7 +121,7 @@ func (m *Manager) AuthenticateRequest(ctx context.Context, req *http.Request) er creds, err := m.store.Load(m.baseURL) if err != nil { - return fmt.Errorf("not authenticated: %w", err) + return errNotAuthenticated(err) } if creds.AccessToken != "" { @@ -124,7 +144,7 @@ func (m *Manager) AuthenticateRequest(ctx context.Context, req *http.Request) er return nil } - return fmt.Errorf("no access token or session cookie available") + return errNoCredential("no access token or session cookie available", nil) } // IsAuthenticated checks if there are valid credentials. @@ -246,6 +266,16 @@ func (m *Manager) LoginWithCookie(cookie string) error { return m.store.Save(m.baseURL, creds) } +// OnCredentialCleared registers what to run when the manager clears a credential +// on its own — today, when the server has refused the refresh token. It does not +// run for Logout, whose callers clear the cache themselves, and not when the store +// refused the deletion, because the credential is then still there to be used. +func (m *Manager) OnCredentialCleared(fn func()) { + m.mu.Lock() + defer m.mu.Unlock() + m.credentialCleared = fn +} + // Logout removes stored credentials. func (m *Manager) Logout() error { return m.store.Delete(m.baseURL) @@ -258,7 +288,7 @@ func (m *Manager) Refresh(ctx context.Context) error { creds, err := m.store.Load(m.baseURL) if err != nil { - return fmt.Errorf("not authenticated: %w", err) + return errNotAuthenticated(err) } // Cookie-based auth doesn't support refresh; treat as no-op. @@ -281,17 +311,36 @@ func (m *Manager) refreshLocked(ctx context.Context, creds *Credentials) error { } defer unlock() - if stored, loadErr := m.store.load(m.baseURL); loadErr == nil { - if stored.AccessToken != "" && stored.AccessToken != creds.AccessToken { - return nil - } - creds = stored + stored, loadErr := m.store.load(m.baseURL) + if loadErr != nil { + // Another process had this same grant refused and forgot it while we + // waited for the lock. The copy we came in with is the same dead token. + return errNotAuthenticated(loadErr) + } + if stored.AccessToken != "" && stored.AccessToken != creds.AccessToken { + return nil } + creds = stored if creds.RefreshToken == "" { return fmt.Errorf("no refresh token available") } + if m.refusedRefreshToken != "" && creds.RefreshToken == m.refusedRefreshToken { + // Refused once already, and the store would not let go of it. + return errRefusedGrant(nil) + } + + if held, until := m.refreshHeld(); held { + // Asking again inside the rate limit cannot get through. A token that has + // not actually expired is still good, so carry on with it: the SDK's own + // retry after a 401 costs one request at most. + if creds.ExpiresAt > time.Now().Unix() { + return nil + } + return errRefreshHeld(until) + } + tokenEndpoint := creds.TokenEndpoint if tokenEndpoint == "" { tokenEndpoint = m.baseURL + "/oauth/tokens" @@ -304,8 +353,9 @@ func (m *Manager) refreshLocked(ctx context.Context, creds *Credentials) error { token, err := refreshOAuthToken(ctx, m.httpClient, tokenEndpoint, creds.RefreshToken, oauthClientID, installID) if err != nil { - return fmt.Errorf("token refresh failed: %w", err) + return m.accountForRefreshFailure(err, creds.RefreshToken) } + m.refreshHoldUntil = time.Time{} // A 200 without an access token is not a refresh. Storing the empty string would // take the working token with it and leave nothing to authenticate with. if token.AccessToken == "" { @@ -325,6 +375,92 @@ func (m *Manager) refreshLocked(ctx context.Context, creds *Credentials) error { return m.store.save(m.baseURL, creds) } +// accountForRefreshFailure decides what a failed refresh costs the stored credential. +// Only invalid_grant clears it: that is the one answer that proves re-sending can never +// work. Anything softer keeps it, or a passing 502 would sign people out. +func (m *Manager) accountForRefreshFailure(err error, sentRefreshToken string) error { + var refusal *tokenEndpointError + if !errors.As(err, &refusal) { + // No answer from the server, so no verdict on the grant. + return fmt.Errorf("token refresh failed: %w", err) + } + + if refusal.rateLimited() { + m.holdRefreshes(refusal.RetryAfter) + return errRefreshHeld(m.refreshHoldUntil) + } + + if !refusal.grantRefused() { + return fmt.Errorf("token refresh failed: %w", err) + } + + // Forget it under the lock that already spans this load-refresh-save, so the + // next command asks for a login instead of re-sending it. + if delErr := m.store.delete(m.baseURL); delErr != nil { + // The credential is still on disk for the next command to load, so the + // refusal is remembered here instead. + m.refusedRefreshToken = sentRefreshToken + return errRefusedGrant(delErr) + } + // Cached mail must not outlive the credential that fetched it, here as much + // as on an explicit logout. + if m.credentialCleared != nil { + m.credentialCleared() + } + return errRefusedGrant(nil) +} + +// refreshHeld reports whether this process is sitting out a rate limit, and until when. +func (m *Manager) refreshHeld() (bool, time.Time) { + if m.refreshHoldUntil.IsZero() || !time.Now().Before(m.refreshHoldUntil) { + return false, time.Time{} + } + return true, m.refreshHoldUntil +} + +// holdRefreshes parks refreshes until the rate limit has had time to clear, honoring +// the server's Retry-After when it sends one. The hold only ever moves later. +func (m *Manager) holdRefreshes(retryAfter time.Duration) { + if retryAfter <= 0 { + retryAfter = defaultRefreshHold + } + if until := time.Now().Add(retryAfter); until.After(m.refreshHoldUntil) { + m.refreshHoldUntil = until + } +} + +// errNoCredential is an auth failure with the store's reason attached. It has to be +// classified here: the SDK hands our errors back untouched, so an unclassified one +// would reach the envelope as a generic API failure without the login hint. +func errNoCredential(msg string, cause error) *apierr.Error { + err := apierr.ErrAuth(msg) + err.Cause = cause + return err +} + +func errNotAuthenticated(cause error) *apierr.Error { + return errNoCredential(fmt.Sprintf("not authenticated: %v", cause), cause) +} + +// errRefusedGrant reports a refresh token the server answered invalid_grant for. +// cleanupErr is the failure to clear it from the store, when there was one. +func errRefusedGrant(cleanupErr error) *apierr.Error { + if cleanupErr != nil { + err := errNoCredential(fmt.Sprintf("HEY refused the stored refresh token: the session has expired or was revoked. The stored credentials could not be cleared: %v", cleanupErr), cleanupErr) + err.Hint = "Run: hey auth logout, then: hey auth login" + return err + } + return apierr.ErrAuth("HEY refused the stored refresh token: the session has expired or was revoked. Changing your HEY password ends every session, including this one. The stored credentials have been cleared") +} + +func errRefreshHeld(until time.Time) *apierr.Error { + wait := time.Until(until).Round(time.Second) + err := apierr.ErrRateLimit(int(wait / time.Second)) + err.Message = fmt.Sprintf("HEY is rate-limiting token requests — not asking again for %s", wait) + err.Hint = "Wait for the limit to clear, then run the command again" + return err +} + // GetStore returns the credential store. func (m *Manager) GetStore() *Store { return m.store diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 488d18c1..a142d8df 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -14,6 +14,8 @@ import ( "strings" "testing" "time" + + "github.com/basecamp/hey-cli/internal/apierr" ) func testManager(t *testing.T, server *httptest.Server) *Manager { @@ -439,6 +441,10 @@ func TestMissingCredentialsDoNotModifyRequest(t *testing.T) { if err == nil || !strings.Contains(err.Error(), tt.want) { t.Fatalf("error = %v, want substring %q", err, tt.want) } + var authErr *apierr.Error + if !errors.As(err, &authErr) || authErr.Code != apierr.CodeAuth { + t.Errorf("error = %v, want one coded %q so the exit code and login hint survive the SDK", err, apierr.CodeAuth) + } if got := req.Header.Get("Authorization"); got != "original" { t.Errorf("Authorization = %q, want original header preserved", got) } @@ -570,16 +576,29 @@ func TestRefreshUsesStoredEndpointAndRotatesToken(t *testing.T) { } func TestRefreshFailuresPreserveCredentials(t *testing.T) { + // Every refusal that is not the server's verdict on the grant itself. The + // credential has to come through each of them untouched: signing someone out + // over a blip is worse than the resend this fix exists to stop. + refusable := func() *Credentials { + return &Credentials{AccessToken: "access", RefreshToken: "refresh"} + } + tests := []struct { name string creds *Credentials status int + body string want string wantCalls int }{ {name: "not authenticated", want: "not authenticated", wantCalls: 0}, {name: "no refresh token", creds: &Credentials{AccessToken: "access"}, want: "no refresh token", wantCalls: 0}, - {name: "server failure", creds: &Credentials{AccessToken: "access", RefreshToken: "refresh"}, status: http.StatusUnauthorized, want: "token refresh failed", wantCalls: 1}, + {name: "server failure", creds: refusable(), status: http.StatusUnauthorized, body: "denied", want: "token refresh failed", wantCalls: 1}, + {name: "rate limited", creds: refusable(), status: http.StatusTooManyRequests, body: `{"error":"rate_limit_exceeded"}`, want: "rate-limiting", wantCalls: 1}, + {name: "origin failure", creds: refusable(), status: http.StatusBadGateway, body: "upstream unavailable", want: "token refresh failed", wantCalls: 1}, + {name: "unparseable refusal", creds: refusable(), status: http.StatusBadRequest, body: "nope", want: "token refresh failed", wantCalls: 1}, + {name: "a different oauth error", creds: refusable(), status: http.StatusBadRequest, body: `{"error":"invalid_request"}`, want: "token refresh failed", wantCalls: 1}, + {name: "invalid_grant from a broken origin", creds: refusable(), status: http.StatusInternalServerError, body: `{"error":"invalid_grant"}`, want: "token refresh failed", wantCalls: 1}, } for _, tt := range tests { @@ -588,7 +607,7 @@ func TestRefreshFailuresPreserveCredentials(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { calls++ w.WriteHeader(tt.status) - _, _ = fmt.Fprint(w, "denied") + _, _ = fmt.Fprint(w, tt.body) })) defer server.Close() @@ -829,3 +848,424 @@ func TestLoginOptionsLoggerReceivesProgress(t *testing.T) { t.Errorf("stderr should stay silent when a Logger is set, got %q", captured) } } + +// saveExpiredCredential stores a credential whose access token has run out, so the +// next use has to refresh. +func saveExpiredCredential(t *testing.T, mgr *Manager) { + t.Helper() + if err := mgr.GetStore().Save(mgr.CredentialKey(), &Credentials{ + AccessToken: "expired-access", + RefreshToken: "refresh", + ExpiresAt: time.Now().Add(-time.Hour).Unix(), + }); err != nil { + t.Fatalf("Save: %v", err) + } +} + +// The refusal HEY sends when a refresh token has been killed — by a password change, +// a revoked session, or a token already spent. +func invalidGrantHandler(calls *int) http.HandlerFunc { + return func(w http.ResponseWriter, _ *http.Request) { + *calls++ + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusBadRequest) + _, _ = fmt.Fprint(w, `{"error":"invalid_grant","error_description":"The refresh token is invalid"}`) + } +} + +func TestRefreshForgetsAGrantTheServerRefused(t *testing.T) { + // The verdict is the error code. What the server puts alongside it — a + // description, a malformed one, or nothing — does not change whether the grant + // is dead, so none of these may change whether the credential is cleared. + bodies := map[string]string{ + "with a description": `{"error":"invalid_grant","error_description":"The refresh token is invalid"}`, + "bare": `{"error":"invalid_grant"}`, + "malformed description": `{"error":"invalid_grant","error_description":123}`, + "description as an object": `{"error":"invalid_grant","error_description":{"detail":"gone"}}`, + } + + for name, body := range bodies { + t.Run(name, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusBadRequest) + _, _ = fmt.Fprint(w, body) + })) + defer server.Close() + + t.Setenv("HEY_TOKEN", "") + mgr := testManager(t, server) + saveExpiredCredential(t, mgr) + + _, err := mgr.AccessToken(t.Context()) + if err == nil { + t.Fatal("AccessToken succeeded against a refused grant") + } + + var authErr *apierr.Error + if !errors.As(err, &authErr) || authErr.Code != apierr.CodeAuth { + t.Errorf("error = %v, want one coded %q so the caller stops rather than retries", err, apierr.CodeAuth) + } + if _, loadErr := mgr.GetStore().Load(mgr.CredentialKey()); loadErr == nil { + t.Error("the refused credential is still stored; every later command will re-send it") + } + if mgr.IsAuthenticated() { + t.Error("IsAuthenticated still true after the session was refused") + } + }) + } +} + +// Deleting the credential is what normally stops a refused grant being sent again. +// When the store will not let go of it, the refusal has to be remembered instead, or +// the next command loads the same dead token and spends another attempt on it. +func TestARefusedGrantIsNotResentWhenItCannotBeDeleted(t *testing.T) { + calls := 0 + server := httptest.NewServer(invalidGrantHandler(&calls)) + defer server.Close() + + t.Setenv("HEY_TOKEN", "") + t.Setenv("HEY_NO_KEYRING", "") + mgr := NewManager(server.URL, server.Client(), t.TempDir()) + + // A keyring that stores and reads but refuses to delete. + stored := "" + mgr.GetStore().useKeyring = true + mgr.GetStore().initOnce.Do(func() {}) + mgr.GetStore().keyring = credentialKeyring{ + set: func(_, _, password string) error { stored = password; return nil }, + get: func(_, _ string) (string, error) { return stored, nil }, + delete: func(_, _ string) error { return errors.New("keyring is locked") }, + } + + saveExpiredCredential(t, mgr) + + for range 4 { + if _, err := mgr.AccessToken(t.Context()); err == nil { + t.Fatal("AccessToken succeeded with a refused grant") + } + } + + if calls != 1 { + t.Errorf("refresh requests = %d, want 1 — the refusal has to outlive a delete that failed", calls) + } + + _, err := mgr.AccessToken(t.Context()) + var authErr *apierr.Error + if !errors.As(err, &authErr) || authErr.Code != apierr.CodeAuth { + t.Errorf("error = %v, want one coded %q", err, apierr.CodeAuth) + } +} + +// Forgetting the credential is the manager's call, so whoever owns the response +// cache has to hear about it from here: cached mail must not outlive the credential +// that fetched it. The hook runs only when the credential actually went — a refusal +// the store would not delete, or a failure that is no verdict on the grant, keeps +// the credential and so keeps the cache. +func TestTheClearedCredentialHookRunsOnlyWhenTheCredentialWent(t *testing.T) { + tests := []struct { + name string + status int + body string + refuseDelete bool + wantRuns int + }{ + {name: "refused grant", status: http.StatusBadRequest, body: `{"error":"invalid_grant"}`, wantRuns: 1}, + {name: "refused grant the store keeps", status: http.StatusBadRequest, body: `{"error":"invalid_grant"}`, refuseDelete: true, wantRuns: 0}, + {name: "rate limited", status: http.StatusTooManyRequests, body: `{"error":"rate_limit_exceeded"}`, wantRuns: 0}, + {name: "origin failure", status: http.StatusBadGateway, body: "upstream unavailable", wantRuns: 0}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(tt.status) + _, _ = fmt.Fprint(w, tt.body) + })) + defer server.Close() + + t.Setenv("HEY_TOKEN", "") + t.Setenv("HEY_NO_KEYRING", "") + mgr := NewManager(server.URL, server.Client(), t.TempDir()) + stored := "" + mgr.GetStore().useKeyring = true + mgr.GetStore().initOnce.Do(func() {}) + mgr.GetStore().keyring = credentialKeyring{ + set: func(_, _, password string) error { stored = password; return nil }, + get: func(_, _ string) (string, error) { + if stored == "" { + return "", errors.New("no credential") + } + return stored, nil + }, + delete: func(_, _ string) error { + if tt.refuseDelete { + return errors.New("keyring is locked") + } + stored = "" + return nil + }, + } + runs := 0 + mgr.OnCredentialCleared(func() { runs++ }) + saveExpiredCredential(t, mgr) + + if _, err := mgr.AccessToken(t.Context()); err == nil { + t.Fatal("AccessToken succeeded against a failing token endpoint") + } + + if runs != tt.wantRuns { + t.Errorf("hook ran %d times, want %d", runs, tt.wantRuns) + } + if kept := stored != ""; kept != (tt.wantRuns == 0) { + t.Errorf("credential kept = %v; the hook has to run exactly when it is gone", kept) + } + }) + } +} + +// The bug itself. `hey watch` re-authenticates on every ActionCable dial, so a refresh +// token the server has already killed used to go back out every fifteen seconds for as +// long as the process lived — and the token endpoint's limit counts refusals, so the +// client spent the allowance it needed to log back in. +func TestARefusedGrantIsNeverSentTwice(t *testing.T) { + calls := 0 + server := httptest.NewServer(invalidGrantHandler(&calls)) + defer server.Close() + + t.Setenv("HEY_TOKEN", "") + mgr := testManager(t, server) + saveExpiredCredential(t, mgr) + + for range 5 { + req, reqErr := http.NewRequestWithContext(t.Context(), http.MethodGet, server.URL, nil) + if reqErr != nil { + t.Fatalf("NewRequest: %v", reqErr) + } + if err := mgr.AuthenticateRequest(t.Context(), req); err == nil { + t.Fatal("AuthenticateRequest succeeded with a refused grant") + } + } + + if calls != 1 { + t.Errorf("refresh requests = %d, want 1 — the grant is dead, so only the attempt that learned that should reach the server", calls) + } +} + +func TestRateLimitedRefreshStopsAskingUntilTheLimitCanHaveCleared(t *testing.T) { + calls := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + calls++ + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusTooManyRequests) + _, _ = fmt.Fprint(w, `{"error":"rate_limit_exceeded"}`) + })) + defer server.Close() + + t.Setenv("HEY_TOKEN", "") + mgr := testManager(t, server) + saveExpiredCredential(t, mgr) + + _, err := mgr.AccessToken(t.Context()) + var rateErr *apierr.Error + if !errors.As(err, &rateErr) || rateErr.Code != apierr.CodeRateLimit { + t.Fatalf("error = %v, want the first rate limit coded %q", err, apierr.CodeRateLimit) + } + + for range 4 { + _, err = mgr.AccessToken(t.Context()) + if !errors.As(err, &rateErr) || rateErr.Code != apierr.CodeRateLimit { + t.Fatalf("error = %v, want one coded %q while the hold is in force", err, apierr.CodeRateLimit) + } + } + + if calls != 1 { + t.Errorf("refresh requests = %d, want 1 — asking again inside the window cannot get through", calls) + } +} + +// Being rate-limited is not being logged out. The refresh window opens five minutes +// before expiry, so a token turned away there is usually still good, and the command +// should run on it rather than fail. +func TestRateLimitedRefreshFallsBackToTheTokenItStillHolds(t *testing.T) { + calls := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + calls++ + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusTooManyRequests) + _, _ = fmt.Fprint(w, `{"error":"rate_limit_exceeded"}`) + })) + defer server.Close() + + t.Setenv("HEY_TOKEN", "") + mgr := testManager(t, server) + if err := mgr.GetStore().Save(mgr.CredentialKey(), &Credentials{ + AccessToken: "still-good", + RefreshToken: "refresh", + ExpiresAt: time.Now().Add(2 * time.Minute).Unix(), + }); err != nil { + t.Fatalf("Save: %v", err) + } + + if _, err := mgr.AccessToken(t.Context()); err == nil { + t.Fatal("the first refresh should surface the rate limit") + } + + token, err := mgr.AccessToken(t.Context()) + if err != nil { + t.Fatalf("AccessToken: %v — a rate limit is not a logout", err) + } + if token != "still-good" { + t.Errorf("token = %q, want the unexpired one already stored", token) + } + if calls != 1 { + t.Errorf("refresh requests = %d, want 1", calls) + } +} + +func TestRefreshHoldHonorsRetryAfter(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Retry-After", "42") + w.WriteHeader(http.StatusTooManyRequests) + _, _ = fmt.Fprint(w, `{"error":"rate_limit_exceeded"}`) + })) + defer server.Close() + + t.Setenv("HEY_TOKEN", "") + mgr := testManager(t, server) + saveExpiredCredential(t, mgr) + + if _, err := mgr.AccessToken(t.Context()); err == nil { + t.Fatal("AccessToken succeeded while rate limited") + } + + held, until := mgr.refreshHeld() + if !held { + t.Fatal("no hold after a 429") + } + if wait := time.Until(until); wait > 42*time.Second || wait < 30*time.Second { + t.Errorf("hold = %s, want about the 42 seconds the server asked for", wait.Round(time.Second)) + } +} + +func TestParseRetryAfter(t *testing.T) { + tests := []struct { + name string + value string + want time.Duration + }{ + {name: "absent", value: "", want: 0}, + {name: "seconds", value: "90", want: 90 * time.Second}, + {name: "zero", value: "0", want: 0}, + {name: "negative", value: "-5", want: 0}, + {name: "nonsense", value: "soon", want: 0}, + {name: "capped", value: "86400", want: maxRetryAfter}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := parseRetryAfter(tt.value); got != tt.want { + t.Errorf("parseRetryAfter(%q) = %s, want %s", tt.value, got, tt.want) + } + }) + } +} + +func TestASuccessfulRefreshLiftsTheHold(t *testing.T) { + t.Setenv("HEY_TOKEN", "") + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = fmt.Fprint(w, `{"access_token":"fresh","refresh_token":"rotated","expires_in":3600}`) + })) + defer server.Close() + + mgr := testManager(t, server) + saveExpiredCredential(t, mgr) + mgr.holdRefreshes(time.Minute) + + // The hold is in force, so this one is skipped and the expired token reported. + if _, err := mgr.AccessToken(t.Context()); err == nil { + t.Fatal("AccessToken succeeded while held") + } + + mgr.refreshHoldUntil = time.Time{} + if _, err := mgr.AccessToken(t.Context()); err != nil { + t.Fatalf("AccessToken: %v", err) + } + if held, _ := mgr.refreshHeld(); held { + t.Error("a successful refresh left the hold in place") + } +} + +// Two processes can queue on the credential lock holding the same dead grant. The +// first is refused and forgets it; the second must not go on to send its own copy, +// or the allowance is spent twice over for one dead session. +func TestRefreshStopsWhenAnotherProcessForgotTheCredential(t *testing.T) { + calls := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + calls++ + w.WriteHeader(http.StatusBadRequest) + _, _ = fmt.Fprint(w, `{"error":"invalid_grant"}`) + })) + defer server.Close() + + t.Setenv("HEY_TOKEN", "") + mgr := testManager(t, server) + + // What the losing process is still holding after the winner cleared the store. + stale := &Credentials{ + AccessToken: "dead-access", + RefreshToken: "dead-refresh", + ExpiresAt: time.Now().Add(-time.Hour).Unix(), + } + + err := mgr.refreshLocked(t.Context(), stale) + if err == nil { + t.Fatal("refreshLocked succeeded with no credential in the store") + } + var authErr *apierr.Error + if !errors.As(err, &authErr) || authErr.Code != apierr.CodeAuth { + t.Errorf("error = %v, want one coded %q", err, apierr.CodeAuth) + } + if calls != 0 { + t.Errorf("refresh requests = %d, want none — the credential was already gone", calls) + } +} + +// A 429 whose body never arrives is still a 429: the hold has to come from the +// status, not from parsing what followed it. +func TestRateLimitHoldSurvivesABodyThatNeverArrives(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Length", "64") + w.Header().Set("Retry-After", "30") + w.WriteHeader(http.StatusTooManyRequests) + // Fewer bytes than promised, then hang up: ReadAll fails. + _, _ = fmt.Fprint(w, "trunc") + if flusher, ok := w.(http.Flusher); ok { + flusher.Flush() + } + if hijacker, ok := w.(http.Hijacker); ok { + conn, _, hijackErr := hijacker.Hijack() + if hijackErr == nil { + _ = conn.Close() + } + } + })) + defer server.Close() + + t.Setenv("HEY_TOKEN", "") + mgr := testManager(t, server) + saveExpiredCredential(t, mgr) + + if _, err := mgr.AccessToken(t.Context()); err == nil { + t.Fatal("AccessToken succeeded against a truncated 429") + } + + if held, _ := mgr.refreshHeld(); !held { + t.Error("no hold after a 429 whose body could not be read") + } + if _, err := mgr.GetStore().Load(mgr.CredentialKey()); err != nil { + t.Errorf("the credential was cleared on a rate limit: %v", err) + } +} diff --git a/internal/auth/oauth.go b/internal/auth/oauth.go index 199a3710..e307273c 100644 --- a/internal/auth/oauth.go +++ b/internal/auth/oauth.go @@ -10,12 +10,92 @@ import ( "io" "net/http" "net/url" + "strconv" "strings" "time" "github.com/basecamp/hey-cli/internal/version" ) +// tokenEndpointError is a non-200 answer from the OAuth token endpoint, typed so a +// caller can tell a dead grant from a server that would not answer. Code is the RFC +// 6749 §5.2 error code, and stays empty when the body carried none: an unrecognized +// failure is transient, never proof that the credential is dead. +type tokenEndpointError struct { + Op string // "token exchange" or "token refresh", for the message + StatusCode int + Code string + Body string // verbatim, as the message has always shown it + RetryAfter time.Duration +} + +func (e *tokenEndpointError) Error() string { + return fmt.Sprintf("%s failed (status %d): %s", e.Op, e.StatusCode, e.Body) +} + +// grantRefused reports whether the server refused the grant itself. invalid_grant is +// the one answer RFC 6749 gives for a refresh token that is expired, revoked or spent. +// The status is checked too: a 5xx that echoes the code is an origin failing. +func (e *tokenEndpointError) grantRefused() bool { + return e.Code == "invalid_grant" && e.StatusCode >= 400 && e.StatusCode < 500 +} + +// rateLimited reports whether the server declined to look at the grant at all. +func (e *tokenEndpointError) rateLimited() bool { + return e.StatusCode == http.StatusTooManyRequests +} + +func newTokenEndpointError(op string, resp *http.Response, body []byte) *tokenEndpointError { + err := &tokenEndpointError{ + Op: op, + StatusCode: resp.StatusCode, + Body: string(body), + RetryAfter: parseRetryAfter(resp.Header.Get("Retry-After")), + } + + var payload struct { + Error string `json:"error"` + } + if jsonErr := json.Unmarshal(body, &payload); jsonErr == nil { + err.Code = payload.Error + } + return err +} + +// parseRetryAfter reads the delay-seconds and HTTP-date forms of RFC 9110 §10.2.3, +// and returns 0 for anything else. A negative or absurd value is dropped rather than +// honored: the header is the server's suggestion, not a lever to pin a client with. +func parseRetryAfter(value string) time.Duration { + value = strings.TrimSpace(value) + if value == "" { + return 0 + } + + if seconds, err := strconv.Atoi(value); err == nil { + return clampRetryAfter(time.Duration(seconds) * time.Second) + } + if when, err := http.ParseTime(value); err == nil { + return clampRetryAfter(time.Until(when)) + } + return 0 +} + +// maxRetryAfter caps how long a server may park the client. An hour is the whole +// window the token endpoint's limit is measured over, so nothing beyond it can be +// about this limit. +const maxRetryAfter = time.Hour + +func clampRetryAfter(d time.Duration) time.Duration { + switch { + case d <= 0: + return 0 + case d > maxRetryAfter: + return maxRetryAfter + default: + return d + } +} + // OAuthToken represents the token response from the HEY OAuth server. type OAuthToken struct { AccessToken string `json:"access_token"` //nolint:gosec // G117: legitimate OAuth field @@ -49,25 +129,7 @@ func exchangeCode(ctx context.Context, httpClient *http.Client, tokenEndpoint, c } defer resp.Body.Close() - body, err := io.ReadAll(io.LimitReader(resp.Body, 64<<10)) - if err != nil { - return nil, fmt.Errorf("reading token response: %w", err) - } - - if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("token exchange failed (status %d): %s", resp.StatusCode, string(body)) - } - - var token OAuthToken - if err := json.Unmarshal(body, &token); err != nil { - return nil, fmt.Errorf("parsing token response: %w", err) - } - - if token.ExpiresIn > 0 { - token.ExpiresAt = time.Now().Add(time.Duration(token.ExpiresIn) * time.Second) - } - - return &token, nil + return readTokenResponse("token exchange", resp) } // refreshOAuthToken refreshes an access token using a refresh token. @@ -92,18 +154,25 @@ func refreshOAuthToken(ctx context.Context, httpClient *http.Client, tokenEndpoi } defer resp.Body.Close() - body, err := io.ReadAll(io.LimitReader(resp.Body, 64<<10)) - if err != nil { - return nil, fmt.Errorf("reading refresh response: %w", err) - } + return readTokenResponse("token refresh", resp) +} +// readTokenResponse reads the token endpoint's answer, typing a refusal so the caller +// can tell a dead grant from a server that would not answer. +func readTokenResponse(op string, resp *http.Response) (*OAuthToken, error) { + body, err := io.ReadAll(io.LimitReader(resp.Body, 64<<10)) if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("token refresh failed (status %d): %s", resp.StatusCode, string(body)) + // A body that truncates on the way in does not lose the verdict with it: + // the status and its Retry-After are already in hand. + return nil, newTokenEndpointError(op, resp, body) + } + if err != nil { + return nil, fmt.Errorf("reading %s response: %w", op, err) } var token OAuthToken if err := json.Unmarshal(body, &token); err != nil { - return nil, fmt.Errorf("parsing refresh response: %w", err) + return nil, fmt.Errorf("parsing %s response: %w", op, err) } if token.ExpiresIn > 0 { diff --git a/internal/auth/oauth_test.go b/internal/auth/oauth_test.go index 245313b6..c5ce7e89 100644 --- a/internal/auth/oauth_test.go +++ b/internal/auth/oauth_test.go @@ -100,9 +100,9 @@ func TestOAuthTokenResponseFailures(t *testing.T) { exchange bool }{ {name: "exchange status", status: http.StatusUnauthorized, body: "denied", want: "token exchange failed (status 401): denied", exchange: true}, - {name: "exchange invalid JSON", status: http.StatusOK, body: "not-json", want: "parsing token response", exchange: true}, + {name: "exchange invalid JSON", status: http.StatusOK, body: "not-json", want: "parsing token exchange response", exchange: true}, {name: "refresh status", status: http.StatusBadGateway, body: "upstream unavailable", want: "token refresh failed (status 502): upstream unavailable"}, - {name: "refresh invalid JSON", status: http.StatusOK, body: "not-json", want: "parsing refresh response"}, + {name: "refresh invalid JSON", status: http.StatusOK, body: "not-json", want: "parsing token refresh response"}, } for _, tt := range tests { diff --git a/internal/auth/store.go b/internal/auth/store.go index 3dc03265..3e71f31f 100644 --- a/internal/auth/store.go +++ b/internal/auth/store.go @@ -100,15 +100,11 @@ func (s *Store) Delete(origin string) error { } defer unlock() - s.ensureInit() - if s.useKeyring { - return s.keyring.delete(serviceName, key(origin)) - } - return s.deleteFile(origin) + return s.delete(origin) } -// load and save are the unlocked pair, for a caller already holding the lock over a -// whole read-modify-write. Everything else goes through Load and Save. +// load, save and delete are the unlocked set, for a caller already holding the lock over +// a whole read-modify-write. Everything else goes through Load, Save and Delete. func (s *Store) load(origin string) (*Credentials, error) { s.ensureInit() if s.useKeyring { @@ -125,6 +121,14 @@ func (s *Store) save(origin string, creds *Credentials) error { return s.saveToFile(origin, creds) } +func (s *Store) delete(origin string) error { + s.ensureInit() + if s.useKeyring { + return s.keyring.delete(serviceName, key(origin)) + } + return s.deleteFile(origin) +} + func (s *Store) loadFromKeyring(origin string) (*Credentials, error) { data, err := s.keyring.get(serviceName, key(origin)) if err != nil { diff --git a/internal/cmd/auth.go b/internal/cmd/auth.go index 50c2403d..1758ad08 100644 --- a/internal/cmd/auth.go +++ b/internal/cmd/auth.go @@ -2,6 +2,7 @@ package cmd import ( "context" + "errors" "fmt" "os" "strings" @@ -277,7 +278,7 @@ func newAuthRefreshCommand() *cobra.Command { Short: "Force token refresh", RunE: func(cmd *cobra.Command, args []string) error { if err := authMgr.Refresh(cmd.Context()); err != nil { - return apierr.ErrAuth(fmt.Sprintf("refresh failed: %v", err)) + return authFailure("refresh failed", err) } return writeMutation(cmd, "Token refreshed", nil) }, @@ -310,7 +311,7 @@ Cookie header, so it is not a bearer token and this command refuses to print it. token, err := authMgr.AccessToken(cmd.Context()) if err != nil { - return apierr.ErrAuth(fmt.Sprintf("could not get token: %v", err)) + return authFailure("could not get token", err) } fmt.Fprint(cmd.OutOrStdout(), token) return nil @@ -322,6 +323,21 @@ Cookie header, so it is not a bearer token and this command refuses to print it. return cmd } +// authFailure reports a manager failure with the command's context in front of it. +// The manager classifies its own refusals — a refused grant is auth, a throttled +// token endpoint is rate_limit with how long to wait — and wrapping every one as +// ErrAuth turned a 429 into exit 3 and "Run: hey auth login". A classified error +// keeps its code, hint and status; only its message gains the context. +func authFailure(context string, err error) error { + var classified *apierr.Error + if !errors.As(err, &classified) { + return apierr.ErrAuth(fmt.Sprintf("%s: %v", context, err)) + } + prefixed := *classified + prefixed.Message = fmt.Sprintf("%s: %s", context, classified.Message) + return &prefixed +} + // refuseSessionCookieAsToken stops `hey auth token` from printing a session cookie. // AccessToken falls back to one, and a cookie sent as a bearer token 401s with // nothing to explain it — besides leaving the cookie in the caller's shell history. diff --git a/internal/cmd/auth_commands_test.go b/internal/cmd/auth_commands_test.go index 1cd7819b..d5fc8028 100644 --- a/internal/cmd/auth_commands_test.go +++ b/internal/cmd/auth_commands_test.go @@ -3,6 +3,7 @@ package cmd import ( "bytes" "encoding/json" + "errors" "io" "net/http" "net/http/httptest" @@ -12,6 +13,7 @@ import ( "testing" "time" + "github.com/basecamp/hey-cli/internal/apierr" "github.com/basecamp/hey-cli/internal/auth" "github.com/basecamp/hey-cli/internal/output" ) @@ -206,6 +208,57 @@ func TestAuthRefreshFailure(t *testing.T) { } } +// A throttled token endpoint is not a missing login. Both commands used to wrap every +// manager failure as ErrAuth, so a 429 came out as exit 3 with "Run: hey auth login" — +// advice that spends another request on the same limit. +func TestAuthCommandsKeepARateLimitClassified(t *testing.T) { + commands := map[string][]string{ + "refresh": {"auth", "refresh"}, + "token": {"auth", "token", "--stored"}, + } + + for name, args := range commands { + t.Run(name, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Retry-After", "42") + w.WriteHeader(http.StatusTooManyRequests) + })) + defer server.Close() + configHome := t.TempDir() + t.Setenv("HEY_NO_KEYRING", "1") + manager := auth.NewManager(server.URL, server.Client(), filepath.Join(configHome, "hey-cli")) + // Expired, so `auth token` has to refresh rather than print what it holds. + expired := &auth.Credentials{AccessToken: "old-access", RefreshToken: "old-refresh", ExpiresAt: time.Now().Add(-time.Hour).Unix()} + if err := manager.GetStore().Save(manager.CredentialKey(), expired); err != nil { + t.Fatalf("seed credentials: %v", err) + } + + _, _, err := runAuthCommand(t, configHome, server.URL, "", true, args...) + + var classified *apierr.Error + if !errors.As(err, &classified) { + t.Fatalf("error = %v, want an *apierr.Error", err) + } + if classified.Code != apierr.CodeRateLimit { + t.Errorf("code = %q, want %q", classified.Code, apierr.CodeRateLimit) + } + if got := output.ExitCodeFor(err); got != output.ExitRateLimit { + t.Errorf("exit code = %d, want %d", got, output.ExitRateLimit) + } + if !strings.Contains(classified.Message, "rate-limiting") || !strings.Contains(classified.Message, "42s") { + t.Errorf("message = %q, want the rate limit and its wait", classified.Message) + } + if strings.Contains(classified.Hint, "hey auth login") { + t.Errorf("hint = %q; logging in again spends the same limit", classified.Hint) + } + creds, loadErr := manager.GetStore().Load(manager.CredentialKey()) + if loadErr != nil || creds.RefreshToken != "old-refresh" { + t.Errorf("credentials after a rate limit = %#v, %v; want them kept", creds, loadErr) + } + }) + } +} + func TestDoctorCommandReportsEnvironment(t *testing.T) { server := httptest.NewServer(http.NotFoundHandler()) defer server.Close() diff --git a/internal/cmd/root.go b/internal/cmd/root.go index 3c1206cf..9d543fdd 100644 --- a/internal/cmd/root.go +++ b/internal/cmd/root.go @@ -116,6 +116,10 @@ func newRootCmd() *cobra.Command { configDir := config.ConfigDir() httpClient := &http.Client{Timeout: 30 * time.Second} authMgr = auth.NewManager(cfg.BaseURL, httpClient, configDir) + // A refresh token HEY refuses is forgotten by the manager itself, on + // whatever command happened to send it. Cached mail must not outlive + // that credential any more than it outlives a logout. + authMgr.OnCredentialCleared(func() { clearHTTPCache(cmd.ErrOrStderr()) }) initSDK(authMgr, cfg.BaseURL) // The agent-local setup subcommands and skill commands never read diff --git a/internal/cmd/sdk_cache_test.go b/internal/cmd/sdk_cache_test.go index ef5255d1..c0209cf7 100644 --- a/internal/cmd/sdk_cache_test.go +++ b/internal/cmd/sdk_cache_test.go @@ -1,10 +1,16 @@ package cmd import ( + "errors" + "io" + "net/http" + "net/http/httptest" "os" "path/filepath" "testing" + "time" + "github.com/basecamp/hey-cli/internal/apierr" "github.com/basecamp/hey-cli/internal/auth" ) @@ -26,18 +32,7 @@ func TestInitSDKEnablesTheRevalidationCache(t *testing.T) { // ones cached: the replacement clears the cache without waiting for a logout. func TestLoginReplacingCredentialsClearsTheHTTPCache(t *testing.T) { configHome := t.TempDir() - responses := filepath.Join(configHome, "hey-cli", "http", "responses") - if err := os.MkdirAll(responses, 0o700); err != nil { - t.Fatal(err) - } - for name, content := range map[string]string{ - filepath.Join(responses, "abc123.body"): `{"cached":"mail"}`, - filepath.Join(configHome, "hey-cli", "http", "etags.json"): `{"abc123":"\"v1\""}`, - } { - if err := os.WriteFile(name, []byte(content), 0o600); err != nil { - t.Fatal(err) - } - } + responses, etags := seedHTTPCache(t, configHome) if _, _, err := runAuthCommand(t, configHome, "https://app.hey.com", "", true, "auth", "login", "--cookie", "replacement-cookie"); err != nil { t.Fatalf("auth login: %v", err) @@ -46,25 +41,14 @@ func TestLoginReplacingCredentialsClearsTheHTTPCache(t *testing.T) { if _, err := os.Stat(responses); !os.IsNotExist(err) { t.Error("expected login to drop the previous credentials' cached responses") } - if _, err := os.Stat(filepath.Join(configHome, "hey-cli", "http", "etags.json")); !os.IsNotExist(err) { + if _, err := os.Stat(etags); !os.IsNotExist(err) { t.Error("expected login to drop the previous credentials' cached ETags") } } func TestLogoutClearsTheHTTPCache(t *testing.T) { configHome := t.TempDir() - responses := filepath.Join(configHome, "hey-cli", "http", "responses") - if err := os.MkdirAll(responses, 0o700); err != nil { - t.Fatal(err) - } - for name, content := range map[string]string{ - filepath.Join(responses, "abc123.body"): `{"cached":"mail"}`, - filepath.Join(configHome, "hey-cli", "http", "etags.json"): `{"abc123":"\"v1\""}`, - } { - if err := os.WriteFile(name, []byte(content), 0o600); err != nil { - t.Fatal(err) - } - } + responses, etags := seedHTTPCache(t, configHome) if _, _, err := runAuthCommand(t, configHome, "https://app.hey.com", "", true, "auth", "login", "--cookie", "session-cookie"); err != nil { t.Fatalf("auth login: %v", err) @@ -76,7 +60,107 @@ func TestLogoutClearsTheHTTPCache(t *testing.T) { if _, err := os.Stat(responses); !os.IsNotExist(err) { t.Error("expected logout to drop the cached responses") } - if _, err := os.Stat(filepath.Join(configHome, "hey-cli", "http", "etags.json")); !os.IsNotExist(err) { + if _, err := os.Stat(etags); !os.IsNotExist(err) { t.Error("expected logout to drop the cached ETags") } } + +// A refresh token HEY has refused is forgotten by the manager itself, in the middle +// of whatever command sent it. Cached mail must not outlive that credential any more +// than it outlives an explicit logout. +func TestARefusedGrantClearsTheHTTPCache(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusBadRequest) + _, _ = io.WriteString(w, `{"error":"invalid_grant","error_description":"The refresh token is invalid"}`) + })) + defer server.Close() + configHome := t.TempDir() + responses, etags := seedHTTPCache(t, configHome) + manager := seedExpiredCredential(t, configHome, server) + + _, _, err := runAuthCommand(t, configHome, server.URL, "", true, "auth", "refresh") + var classified *apierr.Error + if !errors.As(err, &classified) || classified.Code != apierr.CodeAuth { + t.Fatalf("error = %v, want one coded %q", err, apierr.CodeAuth) + } + + if _, loadErr := manager.GetStore().Load(manager.CredentialKey()); loadErr == nil { + t.Error("the refused credential is still stored") + } + if _, statErr := os.Stat(responses); !os.IsNotExist(statErr) { + t.Error("expected the refused grant to drop the cached responses") + } + if _, statErr := os.Stat(etags); !os.IsNotExist(statErr) { + t.Error("expected the refused grant to drop the cached ETags") + } +} + +// Anything softer than a refused grant is no verdict on the credential, so it keeps +// both the credential and the mail it fetched. +func TestATransientRefreshFailureKeepsTheCredentialAndTheHTTPCache(t *testing.T) { + statuses := map[string]int{ + "rate limited": http.StatusTooManyRequests, + "origin failure": http.StatusBadGateway, + } + + for name, status := range statuses { + t.Run(name, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(status) + })) + defer server.Close() + configHome := t.TempDir() + responses, etags := seedHTTPCache(t, configHome) + manager := seedExpiredCredential(t, configHome, server) + + if _, _, err := runAuthCommand(t, configHome, server.URL, "", true, "auth", "refresh"); err == nil { + t.Fatal("auth refresh succeeded against a failing token endpoint") + } + + creds, loadErr := manager.GetStore().Load(manager.CredentialKey()) + if loadErr != nil || creds.RefreshToken != "old-refresh" { + t.Errorf("credentials = %#v, %v; want them kept", creds, loadErr) + } + if _, statErr := os.Stat(filepath.Join(responses, "abc123.body")); statErr != nil { + t.Errorf("cached response: %v; want it kept", statErr) + } + if _, statErr := os.Stat(etags); statErr != nil { + t.Errorf("cached ETags: %v; want them kept", statErr) + } + }) + } +} + +// seedHTTPCache plants one cached response and its ETag under configHome, the way +// the SDK's cache lays them out, and answers where each is. +func seedHTTPCache(t *testing.T, configHome string) (responses, etags string) { + t.Helper() + responses = filepath.Join(configHome, "hey-cli", "http", "responses") + etags = filepath.Join(configHome, "hey-cli", "http", "etags.json") + if err := os.MkdirAll(responses, 0o700); err != nil { + t.Fatal(err) + } + for name, content := range map[string]string{ + filepath.Join(responses, "abc123.body"): `{"cached":"mail"}`, + etags: `{"abc123":"\"v1\""}`, + } { + if err := os.WriteFile(name, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + } + return responses, etags +} + +// seedExpiredCredential stores a credential whose access token has expired, so the +// next command has to send the refresh token to server. +func seedExpiredCredential(t *testing.T, configHome string, server *httptest.Server) *auth.Manager { + t.Helper() + t.Setenv("HEY_NO_KEYRING", "1") + manager := auth.NewManager(server.URL, server.Client(), filepath.Join(configHome, "hey-cli")) + expired := &auth.Credentials{AccessToken: "old-access", RefreshToken: "old-refresh", ExpiresAt: time.Now().Add(-time.Hour).Unix()} + if err := manager.GetStore().Save(manager.CredentialKey(), expired); err != nil { + t.Fatalf("seed credentials: %v", err) + } + return manager +} diff --git a/internal/cmd/watch.go b/internal/cmd/watch.go index cc4f005c..37855d21 100644 --- a/internal/cmd/watch.go +++ b/internal/cmd/watch.go @@ -706,6 +706,15 @@ func (w *postingsWatch) classify(box *watchedBox, posting generated.Posting) *bo // cursor or credentials the server won't take doesn't get better by waiting two minutes, // and a watch that retried it silently would sit there for hours and still exit 0. func permanentReadError(err error) bool { + // A credential failure comes from our own auth strategy, which the SDK passes + // back untouched, so it is already classified and hey.AsError would read it as + // a generic API error. A watch that retried a session the server has ended + // would redial every fifteen seconds for as long as it was left running. + var cliErr *apierr.Error + if errors.As(err, &cliErr) { + return cliErr.Code == apierr.CodeUsage || cliErr.Code == apierr.CodeAuth + } + switch hey.AsError(err).Code { case hey.CodeUsage, hey.CodeAuth: return true diff --git a/internal/cmd/watch_test.go b/internal/cmd/watch_test.go index 8fab476b..5b4412f6 100644 --- a/internal/cmd/watch_test.go +++ b/internal/cmd/watch_test.go @@ -17,7 +17,9 @@ import ( actioncable "github.com/basecamp/actioncable-go" "github.com/basecamp/hey-sdk/go/pkg/generated" + hey "github.com/basecamp/hey-sdk/go/pkg/hey" + "github.com/basecamp/hey-cli/internal/apierr" "github.com/basecamp/hey-cli/internal/auth" ) @@ -1027,3 +1029,32 @@ func TestWatchLineDescribesTheWatchsOwnNews(t *testing.T) { t.Errorf("line = %q, want ready described without a box", line) } } + +// A watch whose credentials the server has ended must stop, not redial. The auth +// failure comes from the CLI's own auth strategy, which the SDK returns untouched, +// so it is not a *hey.Error and the SDK's classifier reads it as a generic API +// error — the kind this retries every two minutes, for as long as the shell service +// keeps restarting it. +func TestPermanentReadErrorRecognizesACLIAuthFailure(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + {name: "cli auth", err: apierr.ErrAuth("not authenticated"), want: true}, + {name: "cli auth wrapped", err: fmt.Errorf("reading changes: %w", apierr.ErrAuth("not authenticated")), want: true}, + {name: "cli usage", err: apierr.ErrUsage("bad cursor"), want: true}, + {name: "cli rate limit", err: apierr.ErrRateLimit(30), want: false}, + {name: "cli network", err: apierr.ErrNetwork(io.EOF), want: false}, + {name: "sdk auth", err: &hey.Error{Code: hey.CodeAuth, Message: "not authenticated"}, want: true}, + {name: "sdk server error", err: &hey.Error{Code: hey.CodeAPI, Message: "boom"}, want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := permanentReadError(tt.err); got != tt.want { + t.Errorf("permanentReadError(%v) = %t, want %t", tt.err, got, tt.want) + } + }) + } +}