diff --git a/internal/auth/auth.go b/internal/auth/auth.go index a2a1d59a..afa6fcff 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -48,9 +48,10 @@ type Manager struct { // 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 runs after the manager has deleted a credential, or + // replaced it with a signed-out record, on its own verdict. The owner of the + // response cache can then drop what that credential fetched. Logout is not + // that: its callers already clear the cache themselves. credentialCleared func() } @@ -312,9 +313,9 @@ func (m *Manager) LoginWithCookie(cookie string) error { } // 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. +// on its own — today, when the server has refused the refresh token. Replacing a +// credential with a signed-out record after deletion fails counts as clearing it. +// Logout does not: its callers clear the cache themselves. func (m *Manager) OnCredentialCleared(fn func()) { m.mu.Lock() defer m.mu.Unlock() @@ -401,6 +402,9 @@ func (m *Manager) refreshLocked(ctx context.Context, creds *Credentials) error { return nil } creds = cloneCredentials(stored) + if creds.AccessToken == "" && creds.RefreshToken == "" && creds.SessionCookie == "" { + return errNoCredential("stored credentials are signed out", nil) + } // Cookie-based auth has nothing to refresh. The fresh read above still lets // a 401 adopt a cookie another process stored before the SDK retries. @@ -486,10 +490,22 @@ func (m *Manager) accountForRefreshFailure(err error, sentRefreshToken string) e // 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. + // Keep the process-local guard even if the durable fallback also fails. m.refusedRefreshToken = sentRefreshToken - return errRefusedGrant(delErr) + + // The keyring can occasionally refuse a delete while still allowing a + // write. Replace the dead grant with a signed-out record so another + // process cannot load and submit it again. A later login overwrites this + // through the ordinary save path. + tombstone := &Credentials{} + if saveErr := m.store.save(m.baseURL, tombstone); saveErr != nil { + return errRefusedGrant(errors.Join(delErr, fmt.Errorf("replace refused credential: %w", saveErr))) + } + m.cachedCredentials = cloneCredentials(tombstone) + if m.credentialCleared != nil { + m.credentialCleared() + } + return errRefusedGrant(nil) } m.cachedCredentials = nil // Cached mail must not outlive the credential that fetched it, here as much diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 2550dd52..b2dac97b 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -982,51 +982,69 @@ func TestRefreshForgetsAGrantTheServerRefused(t *testing.T) { } // 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) { +// When the store will not let go of it, an empty credential takes its place so a +// later process cannot load and resend the same dead token. +func TestARefusedGrantIsNotResentByANewManagerWhenItCannotBeDeleted(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. + configDir := 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) { return stored, nil }, - delete: func(_, _ string) error { return errors.New("keyring is locked") }, + newManager := func() *Manager { + mgr := NewManager(server.URL, server.Client(), configDir) + 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") }, + } + return mgr } - saveExpiredCredential(t, mgr) - - for range 4 { - if _, err := mgr.AccessToken(t.Context()); err == nil { - t.Fatal("AccessToken succeeded with a refused grant") - } + first := newManager() + saveExpiredCredential(t, first) + second := newManager() + if authenticated, err := second.AuthenticationStatus(); err != nil || !authenticated { + t.Fatalf("second manager did not cache the original credential: authenticated = %t, error = %v", authenticated, err) } - if calls != 1 { - t.Errorf("refresh requests = %d, want 1 — the refusal has to outlive a delete that failed", calls) + if _, err := first.AccessToken(t.Context()); err == nil { + t.Fatal("first manager accepted a refused grant") } - _, err := mgr.AccessToken(t.Context()) + _, err := second.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) + t.Fatalf("second manager error = %v, want one coded %q", err, apierr.CodeAuth) + } + if calls != 1 { + t.Errorf("refresh requests = %d, want one across both managers", calls) + } + authenticated, statusErr := second.AuthenticationStatus() + if statusErr != nil { + t.Fatalf("AuthenticationStatus: %v", statusErr) + } + if authenticated { + t.Error("the refused credential still reports authenticated") + } + + if err := second.LoginWithToken("fresh-access"); err != nil { + t.Fatalf("LoginWithToken: %v", err) + } + if token, err := second.AccessToken(t.Context()); err != nil || token != "fresh-access" { + t.Fatalf("AccessToken after login = %q, %v", token, err) } } // 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. +// that fetched it. The hook runs when the credential is deleted or replaced by a +// signed-out record. 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 @@ -1036,7 +1054,7 @@ func TestTheClearedCredentialHookRunsOnlyWhenTheCredentialWent(t *testing.T) { 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: "refused grant replaced after delete fails", status: http.StatusBadRequest, body: `{"error":"invalid_grant"}`, refuseDelete: true, wantRuns: 1}, {name: "rate limited", status: http.StatusTooManyRequests, body: `{"error":"rate_limit_exceeded"}`, wantRuns: 0}, {name: "origin failure", status: http.StatusBadGateway, body: "upstream unavailable", wantRuns: 0}, } @@ -1083,8 +1101,15 @@ func TestTheClearedCredentialHookRunsOnlyWhenTheCredentialWent(t *testing.T) { 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) + var remaining Credentials + if stored != "" { + if err := json.Unmarshal([]byte(stored), &remaining); err != nil { + t.Fatalf("stored credential: %v", err) + } + } + kept := remaining.AccessToken != "" || remaining.SessionCookie != "" + if kept != (tt.wantRuns == 0) { + t.Errorf("usable credential kept = %v; the hook has to run exactly when it is gone", kept) } }) }