From 4806cc03375f73f8e8a6bcac6069a496380031bf Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Fri, 2 Oct 2026 14:29:12 -0400 Subject: [PATCH 1/7] Stop the connector when Basecamp no longer takes its agent's credential A running connector learned that its agent was disconnected in Basecamp, or connected on another computer, only from the event feed. The feed asks Basecamp for something only when it connects, and while its live connection stays up it asks for nothing. Admission and intake ask all the time, but to them a refusal was one failed read among others: admission blocked the mention it was deciding as read_failed, intake kept the projects it last read, and the connector ran on. Every mention addressed to the agent was blocked, and nothing said why, for as long as the socket held. Reproduced end to end against the fake Basecamp: disconnect the agent, or connect it on another computer and let its token come due, while the connector streams; the operator's next mention is blocked read_failed and the connector does not exit. A credential watch now sees the reads admission and intake make. A token renewal the token endpoint refuses is Basecamp's answer already, and stops the run at once. A read answered 401 is only a reason to ask, since something other than Basecamp can answer 401: the watch asks Basecamp itself, as the feed's own stop does, and stops the run only when it confirms. Either way the connector exits 3 with the words the feed's stop uses, and status records it as disconnected. --- internal/commands/connect_credential.go | 114 ++++++++++++++++ internal/commands/connect_credential_test.go | 134 +++++++++++++++++++ internal/commands/connect_run.go | 24 +++- 3 files changed, 268 insertions(+), 4 deletions(-) create mode 100644 internal/commands/connect_credential.go create mode 100644 internal/commands/connect_credential_test.go diff --git a/internal/commands/connect_credential.go b/internal/commands/connect_credential.go new file mode 100644 index 000000000..8a31ce44c --- /dev/null +++ b/internal/commands/connect_credential.go @@ -0,0 +1,114 @@ +package commands + +import ( + "context" + "errors" + "net/http" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + + "github.com/basecamp/basecamp-cli/internal/auth" +) + +// errAgentCredentialNotTaken is the credential watch's stop when Basecamp, +// asked, confirms that it no longer takes the agent's credential. +var errAgentCredentialNotTaken = errors.New("the agent's credential is no longer taken by Basecamp") + +// credentialWatch stops a running connector once Basecamp no longer takes +// its agent's credential, whichever of its reads meets that first. +// +// The feed stops on a refused credential itself, but only when it asks +// Basecamp for something, and while its live connection stays up it asks +// for nothing. Admission and intake ask all the time, and to them a refusal +// is one failed read among others: admission blocks the record it was +// deciding, intake keeps the projects it last read, and both carry on. A +// connector whose agent was disconnected in Basecamp, or connected on +// another computer, would then run on for as long as its socket held, +// blocking every mention addressed to it. The watch sees what their reads +// meet, and ends the run instead, with the disconnect said in the words +// the feed's own stop uses. +// +// It hears two things. A token renewal the token endpoint refuses is +// Basecamp's answer already, and stops the run at once. A read answered 401 +// is only a reason to ask: something other than Basecamp can answer 401, so +// the watch asks Basecamp itself, and stops the run only when it confirms. +type credentialWatch struct { + // refused carries the first refused renewal. + refused chan error + // suspect is signaled by a read answered 401; signals that arrive + // while one is pending coalesce into it. + suspect chan struct{} +} + +func newCredentialWatch() *credentialWatch { + return &credentialWatch{refused: make(chan error, 1), suspect: make(chan struct{}, 1)} +} + +// Tokens is p, watched for a renewal the token endpoint refuses. +func (w *credentialWatch) Tokens(p basecamp.TokenProvider) basecamp.TokenProvider { + return watchedTokens{provider: p, watch: w} +} + +// WrapTransport implements basecamp.TransportWrapper: inner, watched for a +// response answered 401. +func (w *credentialWatch) WrapTransport(inner http.RoundTripper) http.RoundTripper { + return watchedTransport{inner: inner, watch: w} +} + +// Run waits until the credential is refused, and returns why, or until ctx +// ends, and returns nil. confirm asks Basecamp whether it still takes the +// credential, and reports true when it does not. +func (w *credentialWatch) Run(ctx context.Context, confirm func(context.Context) bool) error { + for { + select { + case <-ctx.Done(): + return nil + case err := <-w.refused: + return err + case <-w.suspect: + if confirm(ctx) { + return errAgentCredentialNotTaken + } + } + } +} + +func (w *credentialWatch) refuse(err error) { + select { + case w.refused <- err: + default: + } +} + +func (w *credentialWatch) unauthorized() { + select { + case w.suspect <- struct{}{}: + default: + } +} + +type watchedTokens struct { + provider basecamp.TokenProvider + watch *credentialWatch +} + +func (t watchedTokens) AccessToken(ctx context.Context) (string, error) { + token, err := t.provider.AccessToken(ctx) + if errors.Is(err, auth.ErrAgentCredentialRefused) { + t.watch.refuse(err) + } + return token, err +} + +type watchedTransport struct { + inner http.RoundTripper + watch *credentialWatch +} + +func (t watchedTransport) RoundTrip(req *http.Request) (*http.Response, error) { + resp, err := t.inner.RoundTrip(req) + if err == nil && resp.StatusCode == http.StatusUnauthorized { + t.watch.unauthorized() + } + return resp, err +} diff --git a/internal/commands/connect_credential_test.go b/internal/commands/connect_credential_test.go new file mode 100644 index 000000000..89b46628e --- /dev/null +++ b/internal/commands/connect_credential_test.go @@ -0,0 +1,134 @@ +package commands + +import ( + "context" + "net/http" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/auth" + "github.com/basecamp/basecamp-cli/internal/connector" + "github.com/basecamp/basecamp-cli/internal/output" +) + +// A token renewal Basecamp refuses, met by any read the watch sees, stops +// the run at once, and the run says it as the disconnect it is without +// asking Basecamp again. +func TestTheCredentialWatchStopsOnARefusedRenewal(t *testing.T) { + w := newCredentialWatch() + refused := output.ErrAuth("Minting an agent token was refused (invalid_client)") + refused.Cause = auth.ErrAgentCredentialRefused + tokens := w.Tokens(tokenFunc(func() (string, error) { return "", refused })) + + _, err := tokens.AccessToken(t.Context()) + require.ErrorIs(t, err, auth.ErrAgentCredentialRefused, "the read still sees its own failure") + stop := runWatch(t, w, mustNotConfirm(t)) + require.ErrorIs(t, stop, auth.ErrAgentCredentialRefused) + + state, _, exit := connectorStoppedBy(stop, "Ryan Singer (agent)", "agent", mustNotAsk(t)) + var e *output.Error + require.ErrorAs(t, exit, &e) + assert.Equal(t, "Ryan Singer (agent) was disconnected in Basecamp, or connected on another computer", e.Message) + assert.Equal(t, connector.ConnectionDisconnected, state) +} + +// A read answered 401 asks Basecamp, and stops the run only when Basecamp +// confirms it no longer takes the credential. +func TestTheCredentialWatchAsksAboutAnUnauthorizedRead(t *testing.T) { + w := newCredentialWatch() + transport := w.WrapTransport(statusTransport(http.StatusUnauthorized)) + roundTrip(t, transport) + + asked := 0 + stop := runWatch(t, w, func(context.Context) bool { asked++; return true }) + require.ErrorIs(t, stop, errAgentCredentialNotTaken) + assert.Equal(t, 1, asked) + + state, _, exit := connectorStoppedBy(stop, "Ryan Singer (agent)", "agent", mustNotAsk(t)) + var e *output.Error + require.ErrorAs(t, exit, &e) + assert.Equal(t, output.CodeAuth, e.Code) + assert.Equal(t, connector.ConnectionDisconnected, state) +} + +// A 401 Basecamp does not confirm, from a proxy say, leaves the run going, +// and the next one asks again. +func TestTheCredentialWatchRunsOnWhenBasecampStillTakesTheCredential(t *testing.T) { + w := newCredentialWatch() + transport := w.WrapTransport(statusTransport(http.StatusUnauthorized)) + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + asks := make(chan struct{}) + done := make(chan error, 1) + go func() { + done <- w.Run(ctx, func(context.Context) bool { asks <- struct{}{}; return false }) + }() + + for range 2 { + roundTrip(t, transport) + select { + case <-asks: + case <-time.After(10 * time.Second): + t.Fatal("the watch did not ask about a 401") + } + } + cancel() + require.NoError(t, <-done, "ended by its context, the watch stops nothing") +} + +// Every other failure is the reads' own: neither a renewal that failed for +// another reason nor another status asks Basecamp or stops the run. +func TestTheCredentialWatchIgnoresOtherFailures(t *testing.T) { + w := newCredentialWatch() + tokens := w.Tokens(tokenFunc(func() (string, error) { return "", output.ErrRateLimit(1) })) + _, err := tokens.AccessToken(t.Context()) + require.Error(t, err) + for _, status := range []int{http.StatusForbidden, http.StatusNotFound, http.StatusTooManyRequests, http.StatusInternalServerError} { + roundTrip(t, w.WrapTransport(statusTransport(status))) + } + + ctx, cancel := context.WithCancel(t.Context()) + cancel() + assert.NoError(t, w.Run(ctx, mustNotConfirm(t))) + assert.Empty(t, w.refused) + assert.Empty(t, w.suspect) +} + +type tokenFunc func() (string, error) + +func (f tokenFunc) AccessToken(context.Context) (string, error) { return f() } + +type statusTransport int + +func (s statusTransport) RoundTrip(req *http.Request) (*http.Response, error) { + return &http.Response{StatusCode: int(s), Body: http.NoBody, Request: req}, nil +} + +func roundTrip(t *testing.T, rt http.RoundTripper) { + t.Helper() + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "http://basecamp.test/999/my/profile.json", nil) + require.NoError(t, err) + resp, err := rt.RoundTrip(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) +} + +// runWatch runs w until it stops on its own, failing the test if it does +// not within a deadline that only bounds a failure. +func runWatch(t *testing.T, w *credentialWatch, confirm func(context.Context) bool) error { + t.Helper() + ctx, cancel := context.WithTimeout(t.Context(), 10*time.Second) + defer cancel() + err := w.Run(ctx, confirm) + if ctx.Err() != nil { + t.Fatal("the watch did not stop") + } + return err +} + +func mustNotConfirm(t *testing.T) func(context.Context) bool { + return func(context.Context) bool { t.Error("asked Basecamp to confirm what it had already said"); return true } +} diff --git a/internal/commands/connect_run.go b/internal/commands/connect_run.go index 30ce5f838..2e5749006 100644 --- a/internal/commands/connect_run.go +++ b/internal/commands/connect_run.go @@ -345,6 +345,14 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { } lines := ndjson.NewWriter(cmd.OutOrStdout()) + // The reads admission and intake make are watched for Basecamp refusing + // the agent's credential, which the feed alone would not see while its + // live connection stays up. The start's own reads are not: a refusal + // there ends the start as it is. + watch := newCredentialWatch() + watchedOpts := append(connectSDKOptions(), basecamp.WithTransportWrapper(watch)) + watchedTokens := watch.Tokens(tokens) + queue, err := connector.NewQueue(connector.DefaultBacklogWarn, connector.DefaultBacklogPause) if err != nil { return err @@ -362,7 +370,9 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { intakeOpts.Queue = queue intakeOpts.Lines = lines intakeOpts.Logger = logger - intakeOpts.Membership = connector.SDKMembership{Client: accountClient} + intakeOpts.Membership = connector.SDKMembership{ + Client: basecamp.NewClient(&basecamp.Config{BaseURL: app.Config.BaseURL}, watchedTokens, watchedOpts...).ForAccount(account), + } intake, err := connector.New(intakeOpts) if err != nil { return err @@ -374,7 +384,7 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { // is at most connectServedTTL old — see TestConnectServedTTLStaysSmall. served := newConnectServed(path, file, logger) - reads := admission.NewSDKReads(&basecamp.Config{BaseURL: app.Config.BaseURL}, tokens, account, connectSDKOptions()...) + reads := admission.NewSDKReads(&basecamp.Config{BaseURL: app.Config.BaseURL}, watchedTokens, account, watchedOpts...) admitter, err := admission.NewAdmitter(policy, reads, admission.WithServed(served.Current)) if err != nil { return output.ErrUsage(err.Error()) @@ -446,6 +456,9 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { if err := ledger.NoteConnection(ctx, connector.ConnectionRunning, ""); err != nil { logger.Warn("connector: could not record that it runs, for status", "error", err) } + runPart("credential", func(ctx context.Context) error { + return watch.Run(ctx, func(ctx context.Context) bool { return confirmAgentRefused(ctx, app, kind, name) }) + }) runPart("intake", intake.Run) runPart("admission", func(ctx context.Context) error { return connector.RunAdmission(ctx, connector.AdmissionOptions{Ledger: ledger, Queue: queue, Admitter: admitter, Lines: lines, Logger: logger}) @@ -493,9 +506,12 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { // by a newer approval — is said in their words; anything else is returned as it // is. The feed's authorization_failed counts forbidden answers too, so it is // called a disconnect only when refused, asking Basecamp, confirms it; a -// refused token renewal is already Basecamp's answer. +// refused token renewal is already Basecamp's answer, and so is a refusal +// the credential watch has confirmed. func connectorStoppedBy(err error, agent, profile string, refused func() bool) (state, detail string, exit error) { - disconnected := errors.Is(err, auth.ErrAgentCredentialRefused) || (feedAuthorizationFailed(err) && refused()) + disconnected := errors.Is(err, auth.ErrAgentCredentialRefused) || + errors.Is(err, errAgentCredentialNotTaken) || + (feedAuthorizationFailed(err) && refused()) if !disconnected { return connector.ConnectionStopped, "", err } From c3572f4f34b736c87193602962031b19e9bb9326 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Fri, 2 Oct 2026 15:20:45 -0400 Subject: [PATCH 2/7] Stop a bot user's connector when Basecamp refuses its login The credential watch only ever stopped an Agent: its confirm answered no for a bot user, so a bot whose login was revoked kept running and blocked every mention as read_failed, the failure the watch exists to end. A bot user's refresh the token endpoint refuses (invalid_grant) now carries auth.ErrLoginRefused, and the watch treats it as Basecamp's answer, as it does an Agent's refused mint. A 401 for a bot user is confirmed with a fresh token: the login is refreshed, a refused refresh is the answer, and so is a 401 for the fresh token. A rate-limited or failing refresh answers nothing. The exit says " is no longer signed in: Basecamp refused its login", exit 3, with the login that signs it in again under its pinned identity. Status records it as signed_out and leads with the same remedy. A start that meets a refused refresh says the same. --- internal/auth/auth.go | 11 +- internal/auth/auth_test.go | 18 ++ internal/commands/connect_credential.go | 39 ++-- internal/commands/connect_credential_test.go | 207 +++++++++++++++++- internal/commands/connect_operator.go | 23 +- internal/commands/connect_run.go | 147 ++++++++++--- .../commands/connect_run_disconnect_test.go | 107 ++++++++- internal/commands/connect_start_wait_test.go | 12 +- internal/connector/ledger_hold.go | 6 +- skills/basecamp-connect/SKILL.md | 4 + 10 files changed, 500 insertions(+), 74 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 7c06d0a19..8a3c2dcbe 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -526,6 +526,13 @@ func (m *Manager) errNewerCredential(oauthType string) *output.Error { return e } +// ErrLoginRefused is the cause of a refresh the token endpoint refused with +// invalid_grant: Basecamp no longer takes the login's refresh token, because +// the session expired or was revoked, or, on Launchpad, because the +// environment names a different OAuth client than the one it was issued to. +// No retry changes that answer; only signing in again does. +var ErrLoginRefused = errors.New("the login's refresh token was refused") + // 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 @@ -748,7 +755,9 @@ func (m *Manager) refreshCredential(ctx context.Context, origin string, creds *C if reused && inContainer() { msg += ". " + containerLoginAdvice } - return m.errAuth(msg) + refused := m.errAuth(msg) + refused.Cause = ErrLoginRefused + return refused } creds.AccessToken = token.AccessToken diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 1d9a18d84..49faae37d 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -1960,6 +1960,7 @@ func TestRefresh_InvalidGrantForgetsTheCredential(t *testing.T) { assert.True(t, strings.HasPrefix(cliErr.Message, "Your session has expired or was revoked (The refresh token was revoked"), cliErr.Message) assert.NotContains(t, cliErr.Message, "\x1b", "the server's description is sanitized for the terminal") assert.Equal(t, "Run: basecamp auth login -P work", cliErr.Hint) + assert.ErrorIs(t, err, ErrLoginRefused, "a connector tells this refusal from every other failure by its cause") _, loadErr := m.store.Load(key) assert.Error(t, loadErr, "the dead credential must be forgotten") @@ -1994,6 +1995,7 @@ func TestRefresh_InvalidRequestKeepsTheCredential(t *testing.T) { require.ErrorAs(t, err, &cliErr) assert.Equal(t, output.CodeAPI, cliErr.Code) assert.True(t, strings.HasPrefix(cliErr.Message, "token refresh failed: "), cliErr.Message) + assert.NotErrorIs(t, err, ErrLoginRefused) creds, loadErr := m.store.Load(key) require.NoError(t, loadErr) @@ -2011,10 +2013,26 @@ func TestAccessToken_InvalidGrantHintsTheProfile(t *testing.T) { require.ErrorAs(t, err, &cliErr) assert.Equal(t, output.CodeAuth, cliErr.Code) assert.Equal(t, "Run: basecamp auth login -P work", cliErr.Hint) + assert.ErrorIs(t, err, ErrLoginRefused) _, loadErr := m.store.Load(key) assert.Error(t, loadErr) } +// TestRefresh_ARefusalThatCanPassIsNotTheLoginRefused: a rate limit or a +// server fault at the token endpoint says nothing about the login, so it +// never carries the cause that says Basecamp refused it. +func TestRefresh_ARefusalThatCanPassIsNotTheLoginRefused(t *testing.T) { + for _, status := range []int{http.StatusTooManyRequests, http.StatusServiceUnavailable} { + m, key := refreshRefusedBy(t, status, `{"error":"temporarily_unavailable"}`) + + err := m.Refresh(context.Background()) + require.Error(t, err, status) + assert.NotErrorIs(t, err, ErrLoginRefused, status) + _, loadErr := m.store.Load(key) + assert.NoError(t, loadErr, status) + } +} + // TestRefresh_PreservesIdentityAndBinding: a rotation replaces the tokens // and nothing else — the stored user, scope, and account binding survive a // token response that does not repeat them. diff --git a/internal/commands/connect_credential.go b/internal/commands/connect_credential.go index 8a31ce44c..817f51584 100644 --- a/internal/commands/connect_credential.go +++ b/internal/commands/connect_credential.go @@ -10,9 +10,9 @@ import ( "github.com/basecamp/basecamp-cli/internal/auth" ) -// errAgentCredentialNotTaken is the credential watch's stop when Basecamp, -// asked, confirms that it no longer takes the agent's credential. -var errAgentCredentialNotTaken = errors.New("the agent's credential is no longer taken by Basecamp") +// errCredentialNotTaken is the credential watch's stop when Basecamp, asked, +// confirms that it no longer takes the agent's credential. +var errCredentialNotTaken = errors.New("the agent's credential is no longer taken by Basecamp") // credentialWatch stops a running connector once Basecamp no longer takes // its agent's credential, whichever of its reads meets that first. @@ -22,16 +22,20 @@ var errAgentCredentialNotTaken = errors.New("the agent's credential is no longer // for nothing. Admission and intake ask all the time, and to them a refusal // is one failed read among others: admission blocks the record it was // deciding, intake keeps the projects it last read, and both carry on. A -// connector whose agent was disconnected in Basecamp, or connected on -// another computer, would then run on for as long as its socket held, -// blocking every mention addressed to it. The watch sees what their reads -// meet, and ends the run instead, with the disconnect said in the words -// the feed's own stop uses. +// connector whose Agent was disconnected in Basecamp, or connected on +// another computer, or whose bot user's login was revoked, would then run +// on for as long as its socket held, blocking every mention addressed to +// it. The watch sees what their reads meet, and ends the run instead, with +// the refusal said in the words the feed's own stop uses. // -// It hears two things. A token renewal the token endpoint refuses is -// Basecamp's answer already, and stops the run at once. A read answered 401 -// is only a reason to ask: something other than Basecamp can answer 401, so -// the watch asks Basecamp itself, and stops the run only when it confirms. +// It hears two things. A token renewal the token endpoint refuses, an +// Agent's mint or a bot user's refresh, is Basecamp's answer already, and +// stops the run at once. A read answered 401 is only a reason to ask: +// something other than Basecamp can answer 401, so the watch asks Basecamp +// itself (confirmCredentialRefused), and stops the run only when it +// confirms. A proxy that answers 401 to every request answers the confirm's +// read too, so the connector stops, and its message then names the wrong +// cause; stopping is still right, since no read can get through it. type credentialWatch struct { // refused carries the first refused renewal. refused chan error @@ -67,7 +71,7 @@ func (w *credentialWatch) Run(ctx context.Context, confirm func(context.Context) return err case <-w.suspect: if confirm(ctx) { - return errAgentCredentialNotTaken + return errCredentialNotTaken } } } @@ -94,12 +98,19 @@ type watchedTokens struct { func (t watchedTokens) AccessToken(ctx context.Context) (string, error) { token, err := t.provider.AccessToken(ctx) - if errors.Is(err, auth.ErrAgentCredentialRefused) { + if renewalRefused(err) { t.watch.refuse(err) } return token, err } +// renewalRefused reports whether err is a token renewal the token endpoint +// refused for the credential itself: an Agent's mint, or a bot user's +// refresh. A rate limit, a server fault or the network is not a refusal. +func renewalRefused(err error) bool { + return errors.Is(err, auth.ErrAgentCredentialRefused) || errors.Is(err, auth.ErrLoginRefused) +} + type watchedTransport struct { inner http.RoundTripper watch *credentialWatch diff --git a/internal/commands/connect_credential_test.go b/internal/commands/connect_credential_test.go index 89b46628e..84e8f95af 100644 --- a/internal/commands/connect_credential_test.go +++ b/internal/commands/connect_credential_test.go @@ -2,15 +2,24 @@ package commands import ( "context" + "encoding/json" + "fmt" "net/http" + "net/http/httptest" + "sync" "testing" "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + + "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/auth" + "github.com/basecamp/basecamp-cli/internal/config" "github.com/basecamp/basecamp-cli/internal/connector" + "github.com/basecamp/basecamp-cli/internal/connector/setup" "github.com/basecamp/basecamp-cli/internal/output" ) @@ -28,7 +37,7 @@ func TestTheCredentialWatchStopsOnARefusedRenewal(t *testing.T) { stop := runWatch(t, w, mustNotConfirm(t)) require.ErrorIs(t, stop, auth.ErrAgentCredentialRefused) - state, _, exit := connectorStoppedBy(stop, "Ryan Singer (agent)", "agent", mustNotAsk(t)) + state, _, exit := connectorStoppedBy(stop, anAgent("Ryan Singer (agent)", "agent"), mustNotAsk(t)) var e *output.Error require.ErrorAs(t, exit, &e) assert.Equal(t, "Ryan Singer (agent) was disconnected in Basecamp, or connected on another computer", e.Message) @@ -44,10 +53,10 @@ func TestTheCredentialWatchAsksAboutAnUnauthorizedRead(t *testing.T) { asked := 0 stop := runWatch(t, w, func(context.Context) bool { asked++; return true }) - require.ErrorIs(t, stop, errAgentCredentialNotTaken) + require.ErrorIs(t, stop, errCredentialNotTaken) assert.Equal(t, 1, asked) - state, _, exit := connectorStoppedBy(stop, "Ryan Singer (agent)", "agent", mustNotAsk(t)) + state, _, exit := connectorStoppedBy(stop, anAgent("Ryan Singer (agent)", "agent"), mustNotAsk(t)) var e *output.Error require.ErrorAs(t, exit, &e) assert.Equal(t, output.CodeAuth, e.Code) @@ -79,6 +88,100 @@ func TestTheCredentialWatchRunsOnWhenBasecampStillTakesTheCredential(t *testing. require.NoError(t, <-done, "ended by its context, the watch stops nothing") } +// A bot user's refresh the token endpoint refuses is Basecamp's answer +// already, just as an Agent's refused mint is: the run stops at once, and +// says the bot is signed out. +func TestTheCredentialWatchStopsOnARefusedBotRefresh(t *testing.T) { + w := newCredentialWatch() + tokens := w.Tokens(tokenFunc(func() (string, error) { return "", refusedLogin() })) + + _, err := tokens.AccessToken(t.Context()) + require.ErrorIs(t, err, auth.ErrLoginRefused, "the read still sees its own failure") + stop := runWatch(t, w, mustNotConfirm(t)) + require.ErrorIs(t, stop, auth.ErrLoginRefused) + + state, _, exit := connectorStoppedBy(stop, aBot("Triage Bot", "bot", 4242), mustNotAsk(t)) + var e *output.Error + require.ErrorAs(t, exit, &e) + assert.Equal(t, "Triage Bot is no longer signed in: Basecamp refused its login", e.Message) + assert.Equal(t, connector.ConnectionSignedOut, state) +} + +// A 401 for a bot user is asked about with a fresh token: its login is +// refreshed, and only a 401 for the fresh token too is Basecamp's answer. +// The watch then stops the run, and says the bot is signed out. +func TestABotUsers401ConfirmedWithAFreshTokenStopsTheRun(t *testing.T) { + bc := newBotBasecamp(t) + bc.answerMe(func(string) int { return http.StatusUnauthorized }) + app := bc.app(t) + + w := newCredentialWatch() + roundTrip(t, w.WrapTransport(statusTransport(http.StatusUnauthorized))) + stop := runWatch(t, w, func(ctx context.Context) bool { return confirmCredentialRefused(ctx, app, setup.KindBotUser, "bot") }) + require.ErrorIs(t, stop, errCredentialNotTaken) + assert.Equal(t, []string{"refresh", "me fresh-1"}, bc.asked(), "asked who the fresh token is, not the old one") + + state, _, exit := connectorStoppedBy(stop, aBot("Triage Bot", "bot", 4242), mustNotAsk(t)) + var e *output.Error + require.ErrorAs(t, exit, &e) + assert.Equal(t, output.CodeAuth, e.Code) + assert.Equal(t, connector.ConnectionSignedOut, state) +} + +// A 401 that a fresh token clears, an access token turned away before its +// time say, leaves the run going: the bot's login stands. +func TestABotUsers401AFreshTokenClearsRunsOn(t *testing.T) { + bc := newBotBasecamp(t) + bc.answerMe(func(token string) int { + if token == "fresh-1" { + return http.StatusOK + } + return http.StatusUnauthorized + }) + + assert.False(t, confirmCredentialRefused(t.Context(), bc.app(t), setup.KindBotUser, "bot")) + assert.Equal(t, []string{"refresh", "me fresh-1"}, bc.asked()) +} + +// A refresh the token endpoint refuses, met by the confirm itself, is +// Basecamp's answer, and nothing more is asked. +func TestABotUsersRefreshRefusedWhileConfirmingIsTheAnswer(t *testing.T) { + bc := newBotBasecamp(t) + bc.answerRefresh(http.StatusBadRequest, `{"error":"invalid_grant"}`) + + assert.True(t, confirmCredentialRefused(t.Context(), bc.app(t), setup.KindBotUser, "bot")) + assert.Equal(t, []string{"refresh"}, bc.asked()) +} + +// A refresh that is rate-limited or meets a server fault says nothing about +// the login: the confirm answers no, and nothing is asked of a token it +// could not renew. +func TestABotUsersRefreshThatCanPassDoesNotStopTheRun(t *testing.T) { + for _, status := range []int{http.StatusTooManyRequests, http.StatusInternalServerError, http.StatusServiceUnavailable} { + bc := newBotBasecamp(t) + bc.answerRefresh(status, `{"error":"temporarily_unavailable"}`) + bc.answerMe(func(string) int { return http.StatusUnauthorized }) + + assert.False(t, confirmCredentialRefused(t.Context(), bc.app(t), setup.KindBotUser, "bot"), status) + assert.Equal(t, []string{"refresh"}, bc.asked(), status) + } +} + +// A bot login stored from a bare token has no refresh to try; the token it +// has is the only one it will ever have, so a 401 for it is the answer. +func TestABotLoginWithNoRefreshIsAskedAboutWithItsOwnToken(t *testing.T) { + bc := newBotBasecamp(t) + bc.answerMe(func(string) int { return http.StatusUnauthorized }) + app := bc.app(t) + creds, err := app.Auth.GetStore().Load("profile:bot") + require.NoError(t, err) + creds.RefreshToken, creds.ExpiresAt = "", 0 + require.NoError(t, app.Auth.GetStore().Save("profile:bot", creds)) + + assert.True(t, confirmCredentialRefused(t.Context(), app, setup.KindBotUser, "bot")) + assert.Equal(t, []string{"me old-tok"}, bc.asked()) +} + // Every other failure is the reads' own: neither a renewal that failed for // another reason nor another status asks Basecamp or stops the run. func TestTheCredentialWatchIgnoresOtherFailures(t *testing.T) { @@ -132,3 +235,101 @@ func runWatch(t *testing.T, w *credentialWatch, confirm func(context.Context) bo func mustNotConfirm(t *testing.T) func(context.Context) bool { return func(context.Context) bool { t.Error("asked Basecamp to confirm what it had already said"); return true } } + +// botBasecamp is a Basecamp that a bot user's profile refreshes against and +// reads who it is from, recording what it was asked. +type botBasecamp struct { + srv *httptest.Server + mu sync.Mutex + log []string + refreshStatus int + refreshBody string + refreshes int + me func(token string) int +} + +func newBotBasecamp(t *testing.T) *botBasecamp { + t.Helper() + bc := &botBasecamp{refreshStatus: http.StatusOK, me: func(string) int { return http.StatusOK }} + bc.srv = httptest.NewServer(http.HandlerFunc(bc.serve)) + t.Cleanup(bc.srv.Close) + return bc +} + +func (bc *botBasecamp) serve(w http.ResponseWriter, r *http.Request) { + bc.mu.Lock() + defer bc.mu.Unlock() + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/oauth/tokens": + bc.log = append(bc.log, "refresh") + if bc.refreshStatus != http.StatusOK { + w.WriteHeader(bc.refreshStatus) + fmt.Fprint(w, bc.refreshBody) + return + } + bc.refreshes++ + _ = json.NewEncoder(w).Encode(map[string]any{ + "access_token": fmt.Sprintf("fresh-%d", bc.refreshes), "refresh_token": "ref-next", "expires_in": 3600, + }) + case "/555/my/profile.json": + token := r.Header.Get("Authorization")[len("Bearer "):] + bc.log = append(bc.log, "me "+token) + status := bc.me(token) + w.WriteHeader(status) + if status == http.StatusOK { + _ = json.NewEncoder(w).Encode(map[string]any{"id": 777, "name": "Triage Bot"}) + } + default: + w.WriteHeader(http.StatusNotFound) + } +} + +func (bc *botBasecamp) answerRefresh(status int, body string) { + bc.mu.Lock() + defer bc.mu.Unlock() + bc.refreshStatus, bc.refreshBody = status, body +} + +func (bc *botBasecamp) answerMe(status func(token string) int) { + bc.mu.Lock() + defer bc.mu.Unlock() + bc.me = status +} + +func (bc *botBasecamp) asked() []string { + bc.mu.Lock() + defer bc.mu.Unlock() + return append([]string(nil), bc.log...) +} + +// app is an App whose profile "bot", bound to account 555, holds a bot +// user's BC5 login that refreshes against bc, with an access token still +// inside its lifetime. +func (bc *botBasecamp) app(t *testing.T) *appctx.App { + t.Helper() + t.Setenv("BASECAMP_NO_KEYRING", "1") + t.Setenv("BASECAMP_TOKEN", "") + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + + cfg := &config.Config{ + BaseURL: bc.srv.URL, ActiveProfile: "bot", Sources: map[string]string{}, + Profiles: map[string]*config.ProfileConfig{"bot": {BaseURL: bc.srv.URL, AccountID: "555"}}, + } + mgr := auth.NewManager(cfg, bc.srv.Client()) + store := auth.NewStore(config.GlobalConfigDir()) + mgr.SetStore(store) + require.NoError(t, store.Save("profile:bot", &auth.Credentials{ + AccessToken: "old-tok", + RefreshToken: "old-ref", + OAuthType: "bc5", + TokenEndpoint: bc.srv.URL + "/oauth/tokens", + Scope: "full", + ExpiresAt: time.Now().Add(time.Hour).Unix(), + })) + return &appctx.App{ + Config: cfg, + Auth: mgr, + SDK: basecamp.NewClient(&basecamp.Config{BaseURL: bc.srv.URL}, mgr), + } +} diff --git a/internal/commands/connect_operator.go b/internal/commands/connect_operator.go index 131604d88..c69240e83 100644 --- a/internal/commands/connect_operator.go +++ b/internal/commands/connect_operator.go @@ -200,6 +200,9 @@ type connectStatusReport struct { LockHolder *connectLockHolder `json:"lock_holder,omitempty"` Pings connectStatusPings `json:"pings"` Status connector.Status `json:"status"` + // identity is a bot user's pinned identity, which the command that + // signs it in again names; zero for an Agent. + identity int64 } // connectStatusPings is whether the profile's connector hears the agent's @@ -276,7 +279,7 @@ func runConnectStatus(cmd *cobra.Command, shadow bool) error { status.Tasks[i].Worker = recordedWorkerState(t) status.Tasks[i].Taker = recordedTakerState(t) } - report := connectStatusReport{Profile: p.name, Shadow: shadow, Status: status} + report := connectStatusReport{Profile: p.name, Shadow: shadow, Status: status, identity: p.file.Agent.IdentityID} if holder, ok := connector.InstanceHolder(dir, p.file.AccountID, p.file.Agent.PersonID); ok { report.LockHolder = &connectLockHolder{PID: holder.PID, StartedAt: holder.StartedAt, PIDStatus: processPresence(holder.PID), Pings: holder.Pings, PingsBecause: holder.PingsBecause} } @@ -302,8 +305,13 @@ func runConnectStatus(cmd *cobra.Command, shadow bool) error { func connectStatusSummary(r connectStatusReport) string { parts := []string{} - if c := r.Status.Connection; c != nil && c.State == connector.ConnectionDisconnected { - parts = append(parts, "disconnected") + if c := r.Status.Connection; c != nil { + switch c.State { + case connector.ConnectionDisconnected: + parts = append(parts, "disconnected") + case connector.ConnectionSignedOut: + parts = append(parts, "signed out") + } } if r.Status.Hold != nil { parts = append(parts, "held") @@ -324,8 +332,13 @@ func renderConnectStatus(w io.Writer, r connectStatusReport) { title += " (shadow)" } fmt.Fprintf(w, "%s\n\n", title) - if s.Connection != nil && s.Connection.State == connector.ConnectionDisconnected { - fmt.Fprintf(w, " Disconnected: %s. Reconnect it: basecamp connect setup -P %s\n\n", clean(s.Connection.Detail), richtext.ShellQuote(r.name())) + if c := s.Connection; c != nil { + switch c.State { + case connector.ConnectionDisconnected: + fmt.Fprintf(w, " Disconnected: %s. Reconnect it: basecamp connect setup -P %s\n\n", clean(c.Detail), richtext.ShellQuote(r.name())) + case connector.ConnectionSignedOut: + fmt.Fprintf(w, " Signed out: %s. %s\n\n", clean(c.Detail), botSignInHint(r.name(), r.identity)) + } } switch { diff --git a/internal/commands/connect_run.go b/internal/commands/connect_run.go index 2e5749006..7304d1e8f 100644 --- a/internal/commands/connect_run.go +++ b/internal/commands/connect_run.go @@ -264,15 +264,17 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { } default: } + who := connectRunAs{Kind: kind, Profile: name, IdentityID: file.Agent.IdentityID} if err != nil { - return connectStartFailure(ctx, kind, name, err) + return connectStartFailure(ctx, who, err) } client := connectSDKClient(app, tokens) accountClient := client.ForAccount(account) me, err := (setup.SDKReader{Client: accountClient}).Me(ctx) if err != nil { - return connectStartFailure(ctx, kind, name, err) + return connectStartFailure(ctx, who, err) } + who.Name = me.Name if _, err := checkConnectIdentity(ctx, app, client, kind, creds.OAuthType, me, file.Agent.IdentityID); err != nil { return err } @@ -346,9 +348,9 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { lines := ndjson.NewWriter(cmd.OutOrStdout()) // The reads admission and intake make are watched for Basecamp refusing - // the agent's credential, which the feed alone would not see while its - // live connection stays up. The start's own reads are not: a refusal - // there ends the start as it is. + // the agent's credential, an Agent's or a bot user's, which the feed + // alone would not see while its live connection stays up. The start's + // own reads are not: a refusal there ends the start as it is. watch := newCredentialWatch() watchedOpts := append(connectSDKOptions(), basecamp.WithTransportWrapper(watch)) watchedTokens := watch.Tokens(tokens) @@ -457,7 +459,7 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { logger.Warn("connector: could not record that it runs, for status", "error", err) } runPart("credential", func(ctx context.Context) error { - return watch.Run(ctx, func(ctx context.Context) bool { return confirmAgentRefused(ctx, app, kind, name) }) + return watch.Run(ctx, func(ctx context.Context) bool { return confirmCredentialRefused(ctx, app, kind, name) }) }) runPart("intake", intake.Run) runPart("admission", func(ctx context.Context) error { @@ -488,8 +490,8 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { return connectStoppedBySignal(sig) case firstErr != nil: var err error - refused := func() bool { return confirmAgentRefused(context.WithoutCancel(ctx), app, kind, name) } - stopState, stopDetail, err = connectorStoppedBy(firstErr, me.Name, name, refused) + refused := func() bool { return confirmCredentialRefused(context.WithoutCancel(ctx), app, kind, name) } + stopState, stopDetail, err = connectorStoppedBy(firstErr, who, refused) if err != firstErr { //nolint:errorlint // identity: was firstErr put in the person's words logger.Error("connector: Basecamp refused the agent's credential", "error", firstErr) } @@ -500,23 +502,44 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { return nil } +// connectRunAs is who a connector runs as, for the words its refusals use: +// the kind of credential, the name Basecamp gave (empty until it is read), +// the profile, and a bot user's pinned identity. +type connectRunAs struct { + Kind string + Name string + Profile string + IdentityID int64 +} + // connectorStoppedBy is what the connector exits with when one of its parts // stopped it with err, and the connection state status keeps for that. The -// stop people meet — the connection was disconnected in Basecamp, or replaced -// by a newer approval — is said in their words; anything else is returned as it -// is. The feed's authorization_failed counts forbidden answers too, so it is -// called a disconnect only when refused, asking Basecamp, confirms it; a +// stop people meet, Basecamp refusing the agent's credential, is said in +// their words (errCredentialRefused); anything else is returned as it is. +// The feed's authorization_failed counts forbidden answers too, so it is +// called a refusal only when refused, asking Basecamp, confirms it; a // refused token renewal is already Basecamp's answer, and so is a refusal // the credential watch has confirmed. -func connectorStoppedBy(err error, agent, profile string, refused func() bool) (state, detail string, exit error) { - disconnected := errors.Is(err, auth.ErrAgentCredentialRefused) || - errors.Is(err, errAgentCredentialNotTaken) || +func connectorStoppedBy(err error, who connectRunAs, refused func() bool) (state, detail string, exit error) { + credentialRefused := renewalRefused(err) || + errors.Is(err, errCredentialNotTaken) || (feedAuthorizationFailed(err) && refused()) - if !disconnected { + if !credentialRefused { return connector.ConnectionStopped, "", err } - e := errAgentDisconnected(agent, profile) - return connector.ConnectionDisconnected, e.Message, e + state, e := errCredentialRefused(who) + return state, e.Message, e +} + +// errCredentialRefused says Basecamp refused who's credential in the words +// for its kind, with the connection state status keeps for it: an Agent was +// disconnected and is reconnected; a bot user's login was refused and the +// bot signs in again. +func errCredentialRefused(who connectRunAs) (state string, e *output.Error) { + if who.Kind == setup.KindBotUser { + return connector.ConnectionSignedOut, errBotSignedOut(who.Name, who.Profile, who.IdentityID) + } + return connector.ConnectionDisconnected, errAgentDisconnected(who.Name, who.Profile) } // acquireConnectLock takes the connector's instance lock for the run, or @@ -559,26 +582,27 @@ func connectLockFree(stateDir, account string, agentID int64) error { // connectStartFailure is what a start that could not learn who it is exits // with. Stopped by a signal, or because ctx itself ended, it says so as it // is; a request's own timeout is not the connector stopping. -// Disconnected while the connector wasn't running, it is said the way a -// running connector says it, with no name, since reading it failed. +// Refused while the connector wasn't running, it is said the way a running +// connector says it, with no name, since reading it failed. // Anything else is a failure to read who the profile is. -func connectStartFailure(ctx context.Context, kind, profile string, err error) error { +func connectStartFailure(ctx context.Context, who connectRunAs, err error) error { var e *output.Error switch { case ctx.Err() != nil, errors.As(err, &e) && (e.Code == output.CodeTerminated || e.Code == output.CodeInterrupted): return err - case agentDisconnectedAtStart(kind, err): - return errAgentDisconnected("", profile) + case agentDisconnectedAtStart(who.Kind, err), botSignedOutAtStart(who.Kind, err): + _, refused := errCredentialRefused(who) + return refused } - return output.ErrAuth(fmt.Sprintf("Could not read who profile %q is: %s", profile, setup.ErrorText(err))) + return output.ErrAuth(fmt.Sprintf("Could not read who profile %q is: %s", who.Profile, setup.ErrorText(err))) } // agentDisconnectedAtStart reports whether the connector's first read of who // it is failed because Basecamp refused an Agent's credential: a token // renewal it refused, or a token minted before the disconnect that it no // longer takes (disconnecting doesn't revoke tokens, so one can still be -// cached). A bot user's login keeps its own error. +// cached). func agentDisconnectedAtStart(kind string, err error) bool { if kind != setup.KindAgent || err == nil { return false @@ -588,19 +612,60 @@ func agentDisconnectedAtStart(kind string, err error) bool { (errors.As(err, &apiErr) && apiErr.HTTPStatus == http.StatusUnauthorized) } -// confirmAgentRefused asks Basecamp whether it still takes the profile's -// Agent credential. Only an Agent is asked: a bot user whose login was -// revoked keeps its own error and remedy. -func confirmAgentRefused(ctx context.Context, app *appctx.App, kind, name string) bool { - if kind != setup.KindAgent { - return false - } +// botSignedOutAtStart reports whether the connector's first read of who it +// is failed because Basecamp refused a bot user's refresh. A 401 alone is +// not that answer for a bot user, whose access token can be turned away +// while its login stands; it keeps its own error. +func botSignedOutAtStart(kind string, err error) bool { + return kind == setup.KindBotUser && errors.Is(err, auth.ErrLoginRefused) +} + +// confirmCredentialRefused asks Basecamp whether it still takes the +// profile's credential, and reports true when Basecamp says it does not. +// Anything short of that answer, a rate limit, a server fault, the network, +// a timeout, reports false: the run goes on. +func confirmCredentialRefused(ctx context.Context, app *appctx.App, kind, name string) bool { ctx, cancel := context.WithTimeout(ctx, 15*time.Second) defer cancel() + if kind == setup.KindBotUser { + return botLoginRefused(ctx, app, name) + } refused, _ := agentCredentialRefused(ctx, app, name) return refused } +// botLoginRefused reports whether Basecamp refuses the profile's bot-user +// login. A 401 for the token in hand cannot say: it may only have been +// turned away before its time, which a refresh repairs. So the login is +// refreshed first, and a refresh the token endpoint refuses is Basecamp's +// answer; then who the fresh token is is read, and a 401 for it is the +// answer too. A login that cannot be refreshed at all, one stored from a +// bare token, is asked about with the token it has, the only one it will +// ever have. +func botLoginRefused(ctx context.Context, app *appctx.App, name string) bool { + creds, err := app.Auth.GetStore().LoadContext(ctx, app.Auth.CredentialKey()) + if err != nil { + return false + } + if app.Auth.RefreshRefusal(creds) == nil { + if err := app.Auth.Refresh(ctx); err != nil { + return errors.Is(err, auth.ErrLoginRefused) + } + } + token, err := app.Auth.AccessToken(ctx) + if err != nil { + return errors.Is(err, auth.ErrLoginRefused) + } + accountID, err := connectAccount(app, name) + if err != nil { + return false + } + client := connectSDKClient(app, &basecamp.StaticTokenProvider{Token: token}).ForAccount(accountID) + _, err = client.People().Me(ctx) + var apiErr *basecamp.Error + return errors.As(err, &apiErr) && apiErr.HTTPStatus == http.StatusUnauthorized +} + // feedAuthorizationFailed reports whether err is the event feed ending on // repeated authorization failures. func feedAuthorizationFailed(err error) bool { @@ -621,6 +686,24 @@ func errAgentDisconnected(agent, profile string) *output.Error { return e } +// errBotSignedOut says the connector stopped because Basecamp refused its +// bot user's login, and how to sign the bot in again: the login setup's own +// refusal names, pinned to the identity connect.json records. +func errBotSignedOut(bot, profile string, identity int64) *output.Error { + who := richtext.SanitizeSingleLine(bot) + if who == "" { + who = "Your bot user" + } + e := output.ErrAuth(who + " is no longer signed in: Basecamp refused its login") + e.Hint = botSignInHint(profile, identity) + return e +} + +// botSignInHint is the command that signs a connector's bot user in again. +func botSignInHint(profile string, identity int64) string { + return fmt.Sprintf("Sign it in again: basecamp auth login -P %s --expect-identity %d", richtext.ShellQuote(profile), identity) +} + // connectSinceOverride is the feed position --since asks for. Zero is the // default and means "resume from the ledger"; intake takes only a positive // value as an override (connector.Options.SinceEventID), so a negative one diff --git a/internal/commands/connect_run_disconnect_test.go b/internal/commands/connect_run_disconnect_test.go index 17839f7bc..a0ca14c8e 100644 --- a/internal/commands/connect_run_disconnect_test.go +++ b/internal/commands/connect_run_disconnect_test.go @@ -29,7 +29,7 @@ func TestTheConnectorSaysPlainlyWhenItsAgentWasDisconnected(t *testing.T) { Err: errors.New("event feed mint failed (unauthorized)"), }) - state, detail, err := connectorStoppedBy(refused, "Ryan Singer (agent)", "agent", confirmed) + state, detail, err := connectorStoppedBy(refused, anAgent("Ryan Singer (agent)", "agent"), confirmed) var e *output.Error require.ErrorAs(t, err, &e) assert.Equal(t, output.CodeAuth, e.Code) @@ -39,7 +39,7 @@ func TestTheConnectorSaysPlainlyWhenItsAgentWasDisconnected(t *testing.T) { assert.Equal(t, connector.ConnectionDisconnected, state) assert.Equal(t, e.Message, detail) - _, _, err = connectorStoppedBy(refused, "Ryan\nSinger (agent)", "my agent", confirmed) + _, _, err = connectorStoppedBy(refused, anAgent("Ryan\nSinger (agent)", "my agent"), confirmed) require.ErrorAs(t, err, &e) assert.NotContains(t, e.Message, "\n") assert.Equal(t, "Reconnect it: basecamp connect setup -P 'my agent'", e.Hint) @@ -52,7 +52,7 @@ func TestARefusedTokenRenewalIsTheSameDisconnect(t *testing.T) { refused.Cause = auth.ErrAgentCredentialRefused for _, reason := range []eventfeed.TerminalReason{eventfeed.ReasonMintFailed, eventfeed.ReasonPollFailed} { err := fmt.Errorf("intake: %w", &eventfeed.TerminalError{Reason: reason, Err: fmt.Errorf("renew the token: %w", refused)}) - state, _, got := connectorStoppedBy(err, "Ryan Singer (agent)", "agent", mustNotAsk(t)) + state, _, got := connectorStoppedBy(err, anAgent("Ryan Singer (agent)", "agent"), mustNotAsk(t)) var e *output.Error require.ErrorAs(t, got, &e, reason) assert.Equal(t, `Profile "agent"'s connection to Ryan Singer (agent) was disconnected in Basecamp, or replaced by a newer connection`, e.Message, reason) @@ -65,7 +65,7 @@ func TestARefusedTokenRenewalIsTheSameDisconnect(t *testing.T) { // (Copilot on #806). func TestAnAuthorizationFailureTheCredentialSurvivesIsNotCalledADisconnect(t *testing.T) { err := fmt.Errorf("intake: %w", &eventfeed.TerminalError{Reason: eventfeed.ReasonAuthorizationFailed, Msg: "3 consecutive connection-level authorization failures"}) - state, detail, got := connectorStoppedBy(err, "Ryan Singer (agent)", "agent", func() bool { return false }) + state, detail, got := connectorStoppedBy(err, anAgent("Ryan Singer (agent)", "agent"), func() bool { return false }) assert.Same(t, err, got) assert.Equal(t, connector.ConnectionStopped, state) assert.Empty(t, detail) @@ -73,8 +73,8 @@ func TestAnAuthorizationFailureTheCredentialSurvivesIsNotCalledADisconnect(t *te // A connector started after a disconnect meets it reading who it is: as a // refused token renewal, or as a 401 for a token minted before the -// disconnect, which is still cached (Copilot on #806). A bot user's login -// keeps its own error. +// disconnect, which is still cached (Copilot on #806). A 401 is not a +// disconnect for a bot user, which has none. func TestADisconnectMetAtStartIsRecognisedForAnAgentOnly(t *testing.T) { refused := output.ErrAuth("Minting an agent token was refused (invalid_client)") refused.Cause = auth.ErrAgentCredentialRefused @@ -85,13 +85,62 @@ func TestADisconnectMetAtStartIsRecognisedForAnAgentOnly(t *testing.T) { assert.True(t, agentDisconnectedAtStart(setup.KindAgent, fmt.Errorf("read me: %w", unauthorized))) assert.False(t, agentDisconnectedAtStart(setup.KindAgent, serverError)) assert.False(t, agentDisconnectedAtStart(setup.KindAgent, nil)) - assert.False(t, agentDisconnectedAtStart(setup.KindBotUser, unauthorized), "a bot user's login keeps its own error") + assert.False(t, agentDisconnectedAtStart(setup.KindBotUser, unauthorized), "a bot user is never disconnected") } -// Only an Agent's credential is checked after the feed's authorization -// failure: a bot user is never told it was disconnected (Copilot on #806). -func TestABotUserIsNeverAskedAboutADisconnect(t *testing.T) { - assert.False(t, confirmAgentRefused(t.Context(), nil, setup.KindBotUser, "bot")) +// A bot user started after its login was revoked meets it as a refused +// refresh, and is told the bot is signed out, in the words a running +// connector uses, with the login that signs it in again. A 401 alone, or a +// refresh that failed some other way, keeps its own error. +func TestARefusedBotLoginMetAtStartSaysTheBotIsSignedOut(t *testing.T) { + bot := aBot("", "bot", 4242) + got := connectStartFailure(t.Context(), bot, fmt.Errorf("renew the token: %w", refusedLogin())) + assert.Equal(t, errBotSignedOut("", "bot", 4242), got) + + for _, err := range []error{ + &basecamp.Error{Code: basecamp.CodeAuth, Message: "unauthorized", HTTPStatus: 401}, + output.ErrRateLimit(30), + output.ErrAPI(503, "token refresh failed: unavailable"), + } { + var e *output.Error + require.ErrorAs(t, connectStartFailure(t.Context(), bot, err), &e) + assert.True(t, strings.HasPrefix(e.Message, `Could not read who profile "bot" is: `), e.Message) + } + assert.False(t, botSignedOutAtStart(setup.KindAgent, refusedLogin()), "an Agent's credential has no login to refuse") +} + +// A bot user whose login Basecamp refused is not told it was disconnected, +// a thing that happens only to an Agent: it is told it is signed out, with +// the login that signs it in again under its pinned identity, and status +// records it as signed out. Every way of meeting the refusal says the same. +func TestABotUserWhoseLoginWasRefusedIsSignedOut(t *testing.T) { + feedRefused := fmt.Errorf("intake: %w", &eventfeed.TerminalError{Reason: eventfeed.ReasonAuthorizationFailed, Msg: "3 consecutive connection-level authorization failures"}) + renewalRefused := fmt.Errorf("intake: %w", &eventfeed.TerminalError{Reason: eventfeed.ReasonPollFailed, Err: fmt.Errorf("renew the token: %w", refusedLogin())}) + for name, stop := range map[string]struct { + err error + refused func() bool + }{ + "a refresh the feed met": {renewalRefused, mustNotAsk(t)}, + "a refresh the watch met": {fmt.Errorf("credential: %w", refusedLogin()), mustNotAsk(t)}, + "a 401 the watch confirmed": {fmt.Errorf("credential: %w", errCredentialNotTaken), mustNotAsk(t)}, + "the feed's authorization fails": {feedRefused, confirmed}, + } { + state, detail, got := connectorStoppedBy(stop.err, aBot("Triage\nBot", "my bot", 4242), stop.refused) + var e *output.Error + require.ErrorAs(t, got, &e, name) + assert.Equal(t, output.CodeAuth, e.Code, name) + assert.Equal(t, "Triage Bot is no longer signed in: Basecamp refused its login", e.Message, name) + assert.Equal(t, "Sign it in again: basecamp auth login -P 'my bot' --expect-identity 4242", e.Hint, name) + assert.NotContains(t, e.Message, "disconnected", name) + assert.Equal(t, connector.ConnectionSignedOut, state, name) + assert.Equal(t, e.Message, detail, name) + } + + state, _, got := connectorStoppedBy(feedRefused, aBot("Triage Bot", "bot", 4242), func() bool { return false }) + assert.Same(t, feedRefused, got, "an authorization failure Basecamp does not confirm keeps its own words") + assert.Equal(t, connector.ConnectionStopped, state) + + assert.Equal(t, "Your bot user is no longer signed in: Basecamp refused its login", errBotSignedOut("", "bot", 4242).Message) } // With no name to give, the message still reads as a sentence. @@ -107,13 +156,29 @@ func TestTheConnectorKeepsOtherStopsAsTheyAre(t *testing.T) { fmt.Errorf("intake: %w", &eventfeed.TerminalError{Reason: eventfeed.ReasonProtocolFatal, Msg: "bad frame"}), fmt.Errorf("dispatch: %w", errors.New("stopped on its own")), } { - state, detail, got := connectorStoppedBy(err, "Ryan Singer (agent)", "agent", confirmed) + state, detail, got := connectorStoppedBy(err, anAgent("Ryan Singer (agent)", "agent"), confirmed) assert.Same(t, err, got) assert.Equal(t, connector.ConnectionStopped, state) assert.Empty(t, detail) } } +// Status says the bot user is signed out first, with the login that signs +// it in again. +func TestConnectStatusLeadsWithSignedOut(t *testing.T) { + detail := "Triage Bot is no longer signed in: Basecamp refused its login" + report := connectStatusReport{Profile: "bot", identity: 4242, Status: connector.Status{Connection: &connector.ConnectionStatus{ + State: connector.ConnectionSignedOut, PID: 42, ChangedAt: time.Now(), Detail: detail, + }}} + var out bytes.Buffer + renderConnectStatus(&out, report) + lines := strings.Split(out.String(), "\n") + require.Greater(t, len(lines), 2) + assert.Equal(t, " Signed out: "+detail+". Sign it in again: basecamp auth login -P bot --expect-identity 4242", lines[2]) + assert.NotContains(t, out.String(), "Disconnected") + assert.True(t, strings.HasPrefix(connectStatusSummary(report), "signed out")) +} + // Status says the agent was disconnected first, with the command that // reconnects it. func TestConnectStatusLeadsWithDisconnected(t *testing.T) { @@ -137,6 +202,24 @@ func TestConnectStatusLeadsWithDisconnected(t *testing.T) { func confirmed() bool { return true } +// anAgent is a connector running as an Agent. +func anAgent(name, profile string) connectRunAs { + return connectRunAs{Kind: setup.KindAgent, Name: name, Profile: profile} +} + +// aBot is a connector running as a bot user pinned to identity. +func aBot(name, profile string, identity int64) connectRunAs { + return connectRunAs{Kind: setup.KindBotUser, Name: name, Profile: profile, IdentityID: identity} +} + +// refusedLogin is a bot user's refresh the token endpoint refused, as the +// auth package returns it. +func refusedLogin() error { + e := output.ErrAuth("Your session has expired or was revoked") + e.Cause = auth.ErrLoginRefused + return e +} + // mustNotAsk is a check that fails the test if it is made: a refused renewal // is Basecamp's answer already. func mustNotAsk(t *testing.T) func() bool { diff --git a/internal/commands/connect_start_wait_test.go b/internal/commands/connect_start_wait_test.go index 343594bbe..f501e464b 100644 --- a/internal/commands/connect_start_wait_test.go +++ b/internal/commands/connect_start_wait_test.go @@ -356,7 +356,7 @@ func TestATruncatedRefusalStillEndsTheStart(t *testing.T) { err := awaitConnectToken(t.Context(), tokens, w.options(tokens)) require.Error(t, err) assert.ErrorIs(t, err, auth.ErrAgentCredentialRefused) - assert.Equal(t, errAgentDisconnected("", "agent"), connectStartFailure(t.Context(), setup.KindAgent, "agent", err)) + assert.Equal(t, errAgentDisconnected("", "agent"), connectStartFailure(t.Context(), anAgent("", "agent"), err)) assert.Empty(t, w.waits) assert.Equal(t, 1, mints()) } @@ -393,20 +393,20 @@ func TestAStartFailureKeepsItsFraming(t *testing.T) { canceled, cancel := context.WithCancel(t.Context()) cancel() - assert.Same(t, terminated, connectStartFailure(t.Context(), setup.KindAgent, "agent", terminated)) - assert.ErrorIs(t, connectStartFailure(canceled, setup.KindAgent, "agent", context.Canceled), context.Canceled) - assert.Equal(t, errAgentDisconnected("", "agent"), connectStartFailure(t.Context(), setup.KindAgent, "agent", refused)) + assert.Same(t, terminated, connectStartFailure(t.Context(), anAgent("", "agent"), terminated)) + assert.ErrorIs(t, connectStartFailure(canceled, anAgent("", "agent"), context.Canceled), context.Canceled) + assert.Equal(t, errAgentDisconnected("", "agent"), connectStartFailure(t.Context(), anAgent("", "agent"), refused)) // A request's own timeout, while the connector is not stopping, is a // failure to read who the profile is like any other. timedOut := &basecamp.Error{Code: basecamp.CodeNetwork, Message: "request timed out", Retryable: true, Cause: context.DeadlineExceeded} var framed *output.Error - require.ErrorAs(t, connectStartFailure(t.Context(), setup.KindAgent, "agent", timedOut), &framed) + require.ErrorAs(t, connectStartFailure(t.Context(), anAgent("", "agent"), timedOut), &framed) assert.Equal(t, output.CodeAuth, framed.Code) assert.Contains(t, framed.Message, `Could not read who profile "agent" is`) var e *output.Error - require.ErrorAs(t, connectStartFailure(t.Context(), setup.KindAgent, "agent", output.ErrAPI(404, "minting an agent token: the server answered HTTP 404")), &e) + require.ErrorAs(t, connectStartFailure(t.Context(), anAgent("", "agent"), output.ErrAPI(404, "minting an agent token: the server answered HTTP 404")), &e) assert.Equal(t, output.CodeAuth, e.Code) assert.Equal(t, `Could not read who profile "agent" is: minting an agent token: the server answered HTTP 404`, e.Message) } diff --git a/internal/connector/ledger_hold.go b/internal/connector/ledger_hold.go index c31dbdfbe..480f7d81c 100644 --- a/internal/connector/ledger_hold.go +++ b/internal/connector/ledger_hold.go @@ -509,9 +509,13 @@ const ( // reads this beside the instance lock's holder rather than instead of it. ConnectionStopped = "stopped" // ConnectionDisconnected is a connector that stopped because Basecamp - // refused the agent's credential: the agent was disconnected in Basecamp, + // refused its Agent's credential: the agent was disconnected in Basecamp, // or connected on another computer. Its detail says so, for the person. ConnectionDisconnected = "disconnected" + // ConnectionSignedOut is a connector that stopped because Basecamp + // refused its bot user's login: the session expired or was revoked. Its + // detail says so, for the person. + ConnectionSignedOut = "signed_out" ) // NoteConnection records the running connector's connection state, for diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index 544337918..21825ee02 100644 --- a/skills/basecamp-connect/SKILL.md +++ b/skills/basecamp-connect/SKILL.md @@ -643,6 +643,10 @@ output and tell the person what it means: - **"was disconnected in Basecamp, or replaced by a newer connection"**: run `basecamp connect setup -P ''` (it reconnects with one approval), then start it again. +- **"is no longer signed in: Basecamp refused its login"** (a bot user): the + person signs the bot in again with the command in the hint, + `basecamp auth login -P '' --expect-identity `, + then you start it again. - **Already running**: another connector holds the profile's lock in this state directory. Don't start a second. An older build holding legacy state must be stopped and its existing profile restarted on this build first. From 6315137c0bbac1d7379835da7900e2936895ab61 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Fri, 23 Oct 2026 15:44:15 -0400 Subject: [PATCH 3/7] Keep the credential-watch regression aligned with profile-scoped disconnects --- internal/commands/connect_credential_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/commands/connect_credential_test.go b/internal/commands/connect_credential_test.go index 84e8f95af..b3cb63a9d 100644 --- a/internal/commands/connect_credential_test.go +++ b/internal/commands/connect_credential_test.go @@ -40,7 +40,7 @@ func TestTheCredentialWatchStopsOnARefusedRenewal(t *testing.T) { state, _, exit := connectorStoppedBy(stop, anAgent("Ryan Singer (agent)", "agent"), mustNotAsk(t)) var e *output.Error require.ErrorAs(t, exit, &e) - assert.Equal(t, "Ryan Singer (agent) was disconnected in Basecamp, or connected on another computer", e.Message) + assert.Equal(t, "Profile \"agent\"'s connection to Ryan Singer (agent) was disconnected in Basecamp, or replaced by a newer connection", e.Message) assert.Equal(t, connector.ConnectionDisconnected, state) } From 0bce547c9e1565c35f409f38ca8f1c76a8bbc4e2 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Fri, 23 Oct 2026 16:40:32 -0400 Subject: [PATCH 4/7] Keep credential confirmation responsive and refuse only proven revoked logins --- internal/auth/auth.go | 12 +- internal/auth/auth_test.go | 1 + internal/commands/connect_credential.go | 25 +++- internal/commands/connect_credential_test.go | 128 ++++++++++++++++-- internal/commands/connect_operator.go | 2 +- internal/commands/connect_run.go | 12 +- .../commands/connect_run_disconnect_test.go | 1 + 7 files changed, 157 insertions(+), 24 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 8a3c2dcbe..2270e3618 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -526,11 +526,9 @@ func (m *Manager) errNewerCredential(oauthType string) *output.Error { return e } -// ErrLoginRefused is the cause of a refresh the token endpoint refused with -// invalid_grant: Basecamp no longer takes the login's refresh token, because -// the session expired or was revoked, or, on Launchpad, because the -// environment names a different OAuth client than the one it was issued to. -// No retry changes that answer; only signing in again does. +// ErrLoginRefused marks a BC5 refresh refused with invalid_grant: the +// session expired or was revoked. Launchpad's same error can mean a +// recoverable OAuth-client mismatch, so it does not carry this cause. var ErrLoginRefused = errors.New("the login's refresh token was refused") // forgetRefusedGrant deletes the stored credential only while it still @@ -756,7 +754,9 @@ func (m *Manager) refreshCredential(ctx context.Context, origin string, creds *C msg += ". " + containerLoginAdvice } refused := m.errAuth(msg) - refused.Cause = ErrLoginRefused + if creds.OAuthType == oauthTypeBC5 { + refused.Cause = ErrLoginRefused + } return refused } diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 49faae37d..4d61afcd3 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -2135,6 +2135,7 @@ func TestRefresh_InvalidGrantOnLaunchpadKeepsTheCredential(t *testing.T) { require.ErrorAs(t, err, &cliErr) assert.Equal(t, output.CodeAuth, cliErr.Code) assert.Contains(t, cliErr.Message, "BASECAMP_OAUTH_CLIENT_ID/SECRET name a different OAuth client", "the message names the client mismatch the refusal may mean") + assert.NotErrorIs(t, err, ErrLoginRefused, "a client mismatch must not stop the connector as a revoked login") assert.Equal(t, "Run: basecamp auth login", cliErr.Hint) creds, loadErr := m.store.Load(key) diff --git a/internal/commands/connect_credential.go b/internal/commands/connect_credential.go index 817f51584..93f47fb50 100644 --- a/internal/commands/connect_credential.go +++ b/internal/commands/connect_credential.go @@ -63,6 +63,13 @@ func (w *credentialWatch) WrapTransport(inner http.RoundTripper) http.RoundTripp // ends, and returns nil. confirm asks Basecamp whether it still takes the // credential, and reports true when it does not. func (w *credentialWatch) Run(ctx context.Context, confirm func(context.Context) bool) error { + var confirming <-chan bool + var cancelConfirm context.CancelFunc + defer func() { + if cancelConfirm != nil { + cancelConfirm() + } + }() for { select { case <-ctx.Done(): @@ -70,7 +77,23 @@ func (w *credentialWatch) Run(ctx context.Context, confirm func(context.Context) case err := <-w.refused: return err case <-w.suspect: - if confirm(ctx) { + if confirming != nil { + continue + } + checkCtx, cancel := context.WithCancel(ctx) + cancelConfirm = cancel + done := make(chan bool, 1) + confirming = done + go func() { done <- confirm(checkCtx) }() + case refused := <-confirming: + cancelConfirm() + confirming = nil + // All suspicions that arrived during this check share its answer. + select { + case <-w.suspect: + default: + } + if refused { return errCredentialNotTaken } } diff --git a/internal/commands/connect_credential_test.go b/internal/commands/connect_credential_test.go index b3cb63a9d..134a2e8ad 100644 --- a/internal/commands/connect_credential_test.go +++ b/internal/commands/connect_credential_test.go @@ -7,7 +7,9 @@ import ( "net/http" "net/http/httptest" "sync" + "sync/atomic" "testing" + "testing/synctest" "time" "github.com/stretchr/testify/assert" @@ -66,26 +68,92 @@ func TestTheCredentialWatchAsksAboutAnUnauthorizedRead(t *testing.T) { // A 401 Basecamp does not confirm, from a proxy say, leaves the run going, // and the next one asks again. func TestTheCredentialWatchRunsOnWhenBasecampStillTakesTheCredential(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + w := newCredentialWatch() + transport := w.WrapTransport(statusTransport(http.StatusUnauthorized)) + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + var asks atomic.Int32 + done := make(chan error, 1) + go func() { + done <- w.Run(ctx, func(context.Context) bool { asks.Add(1); return false }) + }() + for attempt := range int32(2) { + roundTrip(t, transport) + synctest.Wait() // The previous confirmation has finished, not just started. + assert.Equal(t, attempt+1, asks.Load()) + } + cancel() + require.NoError(t, <-done, "ended by its context, the watch stops nothing") + }) +} + +func TestARefusedRenewalInterruptsAPendingConfirmation(t *testing.T) { w := newCredentialWatch() - transport := w.WrapTransport(statusTransport(http.StatusUnauthorized)) ctx, cancel := context.WithCancel(t.Context()) defer cancel() - asks := make(chan struct{}) + started, stopped := make(chan struct{}), make(chan struct{}) done := make(chan error, 1) go func() { - done <- w.Run(ctx, func(context.Context) bool { asks <- struct{}{}; return false }) + done <- w.Run(ctx, func(ctx context.Context) bool { + close(started) + <-ctx.Done() + close(stopped) + return false + }) }() + w.unauthorized() + select { + case <-started: + case <-time.After(5 * time.Second): + t.Fatal("confirmation never started") + } + w.refuse(auth.ErrLoginRefused) + select { + case err := <-done: + require.ErrorIs(t, err, auth.ErrLoginRefused) + case <-time.After(5 * time.Second): + t.Fatal("a definitive refusal waited behind confirmation") + } + select { + case <-stopped: + case <-time.After(5 * time.Second): + t.Fatal("the in-flight confirmation was not canceled") + } +} - for range 2 { - roundTrip(t, transport) - select { - case <-asks: - case <-time.After(10 * time.Second): - t.Fatal("the watch did not ask about a 401") +func TestUnauthorizedReadsShareOnePendingConfirmation(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + w := newCredentialWatch() + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + release := make(chan struct{}) + var calls atomic.Int32 + done := make(chan error, 1) + go func() { + done <- w.Run(ctx, func(context.Context) bool { + calls.Add(1) + <-release + return false + }) + }() + w.unauthorized() + synctest.Wait() + require.EqualValues(t, 1, calls.Load()) + for range 20 { + w.unauthorized() } - } - cancel() - require.NoError(t, <-done, "ended by its context, the watch stops nothing") + synctest.Wait() + assert.EqualValues(t, 1, calls.Load()) + close(release) + synctest.Wait() + assert.EqualValues(t, 1, calls.Load(), "pending suspicions share the completed answer") + w.unauthorized() + synctest.Wait() + assert.EqualValues(t, 2, calls.Load(), "a later refusal is checked again") + cancel() + require.NoError(t, <-done) + }) } // A bot user's refresh the token endpoint refuses is Basecamp's answer @@ -167,6 +235,30 @@ func TestABotUsersRefreshThatCanPassDoesNotStopTheRun(t *testing.T) { } } +func TestRepeatedUnauthorizedReadsDuringARefreshHoldDoNotSignOut(t *testing.T) { + bc := newBotBasecamp(t) + bc.answerRefresh(http.StatusTooManyRequests, `{"error":"temporarily_unavailable"}`) + bc.answerMe(func(string) int { return http.StatusUnauthorized }) + app := bc.app(t) + for range 2 { + assert.False(t, confirmCredentialRefused(t.Context(), app, setup.KindBotUser, "bot")) + } + assert.Equal(t, []string{"refresh"}, bc.asked(), "a held refresh must not probe the old token or refresh twice") +} + +func TestALocalRefreshPreflightFailureDoesNotProveRevocation(t *testing.T) { + bc := newBotBasecamp(t) + bc.answerMe(func(string) int { return http.StatusUnauthorized }) + app := bc.app(t) + creds, err := app.Auth.GetStore().Load("profile:bot") + require.NoError(t, err) + creds.TokenEndpoint = "ftp://invalid.example/tokens" + require.Error(t, app.Auth.RefreshRefusal(creds)) + require.NoError(t, app.Auth.GetStore().Save("profile:bot", creds)) + assert.False(t, confirmCredentialRefused(t.Context(), app, setup.KindBotUser, "bot")) + assert.Empty(t, bc.asked(), "a configuration fault is not proof from the old token") +} + // A bot login stored from a bare token has no refresh to try; the token it // has is the only one it will ever have, so a 401 for it is the answer. func TestABotLoginWithNoRefreshIsAskedAboutWithItsOwnToken(t *testing.T) { @@ -182,6 +274,18 @@ func TestABotLoginWithNoRefreshIsAskedAboutWithItsOwnToken(t *testing.T) { assert.Equal(t, []string{"me old-tok"}, bc.asked()) } +func TestABoostRetainsARefusedRenewalForTheCredentialWatch(t *testing.T) { + w := newCredentialWatch() + tokens := w.Tokens(tokenFunc(func() (string, error) { return "", refusedLogin() })) + client := basecamp.NewClient(&basecamp.Config{BaseURL: "http://127.0.0.1:1"}, tokens).ForAccount("555") + booster := connector.SDKBooster{Client: client} + _, err := booster.Boost(t.Context(), 123, "ok") + require.Error(t, err) + // Even if acknowledgement only logs the failure, the definitive + // refusal remains visible to the connector's credential watch. + assert.ErrorIs(t, runWatch(t, w, mustNotConfirm(t)), auth.ErrLoginRefused) +} + // Every other failure is the reads' own: neither a renewal that failed for // another reason nor another status asks Basecamp or stops the run. func TestTheCredentialWatchIgnoresOtherFailures(t *testing.T) { diff --git a/internal/commands/connect_operator.go b/internal/commands/connect_operator.go index c69240e83..c8c7b59fc 100644 --- a/internal/commands/connect_operator.go +++ b/internal/commands/connect_operator.go @@ -310,7 +310,7 @@ func connectStatusSummary(r connectStatusReport) string { case connector.ConnectionDisconnected: parts = append(parts, "disconnected") case connector.ConnectionSignedOut: - parts = append(parts, "signed out") + parts = append(parts, "signed out; "+botSignInHint(r.name(), r.identity)) } } if r.Status.Hold != nil { diff --git a/internal/commands/connect_run.go b/internal/commands/connect_run.go index 7304d1e8f..92326841d 100644 --- a/internal/commands/connect_run.go +++ b/internal/commands/connect_run.go @@ -354,6 +354,7 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { watch := newCredentialWatch() watchedOpts := append(connectSDKOptions(), basecamp.WithTransportWrapper(watch)) watchedTokens := watch.Tokens(tokens) + watchedClient := basecamp.NewClient(&basecamp.Config{BaseURL: app.Config.BaseURL}, watchedTokens, watchedOpts...).ForAccount(account) queue, err := connector.NewQueue(connector.DefaultBacklogWarn, connector.DefaultBacklogPause) if err != nil { @@ -373,7 +374,7 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { intakeOpts.Lines = lines intakeOpts.Logger = logger intakeOpts.Membership = connector.SDKMembership{ - Client: basecamp.NewClient(&basecamp.Config{BaseURL: app.Config.BaseURL}, watchedTokens, watchedOpts...).ForAccount(account), + Client: watchedClient, } intake, err := connector.New(intakeOpts) if err != nil { @@ -476,7 +477,7 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { return connector.RunHandoff(ctx, connector.HandoffOptions{ Ledger: ledger, Served: served.Current, Buckets: buckets, Pings: policy.HearsPings, AgentID: agentID, Lines: lines, Logger: logger, Started: started, - AckContent: file.Ack().Content, Booster: connector.SDKBooster{Client: accountClient}, + AckContent: file.Ack().Content, Booster: connector.SDKBooster{Client: watchedClient}, }) }) } @@ -531,7 +532,7 @@ func connectorStoppedBy(err error, who connectRunAs, refused func() bool) (state return state, e.Message, e } -// errCredentialRefused says Basecamp refused who's credential in the words +// errCredentialRefused says Basecamp refused whose credential in the words // for its kind, with the connection state status keeps for it: an Agent was // disconnected and is reconnected; a bot user's login was refused and the // bot signs in again. @@ -647,7 +648,10 @@ func botLoginRefused(ctx context.Context, app *appctx.App, name string) bool { if err != nil { return false } - if app.Auth.RefreshRefusal(creds) == nil { + if creds.RefreshToken != "" { + if app.Auth.RefreshRefusal(creds) != nil { + return false + } if err := app.Auth.Refresh(ctx); err != nil { return errors.Is(err, auth.ErrLoginRefused) } diff --git a/internal/commands/connect_run_disconnect_test.go b/internal/commands/connect_run_disconnect_test.go index a0ca14c8e..6f324fe25 100644 --- a/internal/commands/connect_run_disconnect_test.go +++ b/internal/commands/connect_run_disconnect_test.go @@ -177,6 +177,7 @@ func TestConnectStatusLeadsWithSignedOut(t *testing.T) { assert.Equal(t, " Signed out: "+detail+". Sign it in again: basecamp auth login -P bot --expect-identity 4242", lines[2]) assert.NotContains(t, out.String(), "Disconnected") assert.True(t, strings.HasPrefix(connectStatusSummary(report), "signed out")) + assert.Contains(t, connectStatusSummary(report), "basecamp auth login -P bot --expect-identity 4242", "JSON output also carries the recovery command") } // Status says the agent was disconnected first, with the command that From db85e1379d96f2f4d0e24436f2a7d97277b618ca Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Fri, 23 Oct 2026 17:05:53 -0400 Subject: [PATCH 5/7] Keep repair polls inside the credential watch --- internal/commands/connect_credential_test.go | 21 ++++++++++++++++++++ internal/commands/connect_run.go | 2 +- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/internal/commands/connect_credential_test.go b/internal/commands/connect_credential_test.go index 134a2e8ad..9f610af59 100644 --- a/internal/commands/connect_credential_test.go +++ b/internal/commands/connect_credential_test.go @@ -16,6 +16,7 @@ import ( "github.com/stretchr/testify/require" "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp/eventfeed" "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/auth" @@ -274,6 +275,26 @@ func TestABotLoginWithNoRefreshIsAskedAboutWithItsOwnToken(t *testing.T) { assert.Equal(t, []string{"me old-tok"}, bc.asked()) } +func TestARepairPollRetainsARefusedRenewalForTheCredentialWatch(t *testing.T) { + bc := newBotBasecamp(t) + bc.answerRefresh(http.StatusBadRequest, `{"error":"invalid_grant"}`) + app := bc.app(t) + creds, err := app.Auth.GetStore().Load("profile:bot") + require.NoError(t, err) + creds.ExpiresAt = time.Now().Add(-time.Hour).Unix() + require.NoError(t, app.Auth.GetStore().Save("profile:bot", creds)) + + watch := newCredentialWatch() + tokens := watch.Tokens(&feedTokens{managerTokens: managerTokens{mgr: app.Auth}}) + opts := append(connectSDKOptions(), basecamp.WithTransportWrapper(watch)) + live, err := eventfeed.NewLive(&basecamp.Config{BaseURL: bc.srv.URL}, tokens, "555", eventfeed.AccountLane, opts...) + require.NoError(t, err) + _, err = connector.LiveOptions(live).PollsFor().Poll(t.Context(), eventfeed.Cursor{Since: "0"}, eventfeed.Filters{}) + require.Error(t, err) + assert.Equal(t, []string{"refresh"}, bc.asked()) + assert.ErrorIs(t, runWatch(t, watch, mustNotConfirm(t)), auth.ErrLoginRefused) +} + func TestABoostRetainsARefusedRenewalForTheCredentialWatch(t *testing.T) { w := newCredentialWatch() tokens := w.Tokens(tokenFunc(func() (string, error) { return "", refusedLogin() })) diff --git a/internal/commands/connect_run.go b/internal/commands/connect_run.go index 92326841d..7e6d7cb09 100644 --- a/internal/commands/connect_run.go +++ b/internal/commands/connect_run.go @@ -360,7 +360,7 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { if err != nil { return err } - live, err := eventfeed.NewLive(&basecamp.Config{BaseURL: app.Config.BaseURL}, &feedTokens{managerTokens: *tokens, log: func(line string) { logger.Warn(line) }}, account, eventfeed.AccountLane, connectSDKOptions()...) + live, err := eventfeed.NewLive(&basecamp.Config{BaseURL: app.Config.BaseURL}, watch.Tokens(&feedTokens{managerTokens: *tokens, log: func(line string) { logger.Warn(line) }}), account, eventfeed.AccountLane, watchedOpts...) if err != nil { return err } From 386b968ca6cc3545b6e503cbabc04263976efa08 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Fri, 23 Oct 2026 17:20:58 -0400 Subject: [PATCH 6/7] Retain observed revocations across concurrent connector shutdown --- internal/commands/connect_credential.go | 42 ++++++++++++++++---- internal/commands/connect_credential_test.go | 27 +++++++++++++ internal/commands/connect_run.go | 1 + 3 files changed, 62 insertions(+), 8 deletions(-) diff --git a/internal/commands/connect_credential.go b/internal/commands/connect_credential.go index 93f47fb50..efbe7ba6a 100644 --- a/internal/commands/connect_credential.go +++ b/internal/commands/connect_credential.go @@ -4,6 +4,7 @@ import ( "context" "errors" "net/http" + "sync" "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" @@ -37,15 +38,18 @@ var errCredentialNotTaken = errors.New("the agent's credential is no longer take // read too, so the connector stops, and its message then names the wrong // cause; stopping is still right, since no read can get through it. type credentialWatch struct { - // refused carries the first refused renewal. - refused chan error + mu sync.Mutex + refusal error // retained even if cancellation wins a part's shutdown race + + // refused wakes the watch after a refusal has been retained. + refused chan struct{} // suspect is signaled by a read answered 401; signals that arrive // while one is pending coalesce into it. suspect chan struct{} } func newCredentialWatch() *credentialWatch { - return &credentialWatch{refused: make(chan error, 1), suspect: make(chan struct{}, 1)} + return &credentialWatch{refused: make(chan struct{}, 1), suspect: make(chan struct{}, 1)} } // Tokens is p, watched for a renewal the token endpoint refuses. @@ -73,9 +77,9 @@ func (w *credentialWatch) Run(ctx context.Context, confirm func(context.Context) for { select { case <-ctx.Done(): - return nil - case err := <-w.refused: - return err + return w.finalError(nil) + case <-w.refused: + return w.finalError(nil) case <-w.suspect: if confirming != nil { continue @@ -84,7 +88,13 @@ func (w *credentialWatch) Run(ctx context.Context, confirm func(context.Context) cancelConfirm = cancel done := make(chan bool, 1) confirming = done - go func() { done <- confirm(checkCtx) }() + go func() { + refused := confirm(checkCtx) + if refused { + w.refuse(errCredentialNotTaken) + } + done <- refused + }() case refused := <-confirming: cancelConfirm() confirming = nil @@ -100,9 +110,25 @@ func (w *credentialWatch) Run(ctx context.Context, confirm func(context.Context) } } +// finalError keeps a refusal observed by any part even if another part's +// missing-credential error canceled the run before the watch could return it. +func (w *credentialWatch) finalError(other error) error { + w.mu.Lock() + defer w.mu.Unlock() + if w.refusal != nil { + return w.refusal + } + return other +} + func (w *credentialWatch) refuse(err error) { + w.mu.Lock() + if w.refusal == nil { + w.refusal = err + } + w.mu.Unlock() select { - case w.refused <- err: + case w.refused <- struct{}{}: default: } } diff --git a/internal/commands/connect_credential_test.go b/internal/commands/connect_credential_test.go index 9f610af59..6944398f5 100644 --- a/internal/commands/connect_credential_test.go +++ b/internal/commands/connect_credential_test.go @@ -123,6 +123,33 @@ func TestARefusedRenewalInterruptsAPendingConfirmation(t *testing.T) { } } +func TestObservedRevocationSurvivesConcurrentShutdown(t *testing.T) { + w := newCredentialWatch() + tokens := w.Tokens(tokenFunc(func() (string, error) { return "", refusedLogin() })) + _, err := tokens.AccessToken(t.Context()) + require.ErrorIs(t, err, auth.ErrLoginRefused) + + // Another read can now find no credential (the refused refresh deleted + // it) and cancel the run before the credential-watch part is selected. + ctx, cancel := context.WithCancel(t.Context()) + cancel() + assert.ErrorIs(t, w.Run(ctx, mustNotConfirm(t)), auth.ErrLoginRefused) + terminal := w.finalError(output.ErrAuth("no stored credential")) + require.ErrorIs(t, terminal, auth.ErrLoginRefused) + state, _, exit := connectorStoppedBy(terminal, aBot("Triage Bot", "bot", 4242), mustNotAsk(t)) + assert.Equal(t, connector.ConnectionSignedOut, state) + var problem *output.Error + require.ErrorAs(t, exit, &problem) + assert.Contains(t, problem.Hint, "basecamp auth login -P bot --expect-identity 4242") +} + +func TestAnUnobservedRefusalDoesNotReplaceTheOriginalStop(t *testing.T) { + w := newCredentialWatch() + original := fmt.Errorf("feed stopped") + assert.Same(t, original, w.finalError(original)) + assert.NoError(t, w.finalError(nil)) +} + func TestUnauthorizedReadsShareOnePendingConfirmation(t *testing.T) { synctest.Test(t, func(t *testing.T) { w := newCredentialWatch() diff --git a/internal/commands/connect_run.go b/internal/commands/connect_run.go index 7e6d7cb09..2b94f61f3 100644 --- a/internal/commands/connect_run.go +++ b/internal/commands/connect_run.go @@ -482,6 +482,7 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { }) } wg.Wait() + firstErr = watch.finalError(firstErr) mu.Lock() sig := received From b6a0424e3f7c445eb7cda4e94f039455bf1a3e85 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Fri, 23 Oct 2026 17:35:57 -0400 Subject: [PATCH 7/7] Keep server faults inconclusive and finish canceled confirmation publication --- internal/auth/auth.go | 3 +- internal/auth/auth_test.go | 14 ++++++ internal/commands/connect_credential.go | 13 +++++- internal/commands/connect_credential_test.go | 47 ++++++++++++++++++++ 4 files changed, 74 insertions(+), 3 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 2270e3618..37673e35b 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -499,7 +499,8 @@ const localClockLayout = "Jan 2 3:04 PM MST" // how an error happens to be phrased. func invalidGrant(err error) (string, bool) { var refusal *basecamp.Error - if errors.As(err, &refusal) && refusal.OAuthError == "invalid_grant" { + if errors.As(err, &refusal) && refusal.OAuthError == "invalid_grant" && + (refusal.HTTPStatus == http.StatusBadRequest || refusal.HTTPStatus == http.StatusUnauthorized) { return refusal.OAuthErrorDescription, true } return "", false diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 4d61afcd3..a95a1f8fc 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -2033,6 +2033,20 @@ func TestRefresh_ARefusalThatCanPassIsNotTheLoginRefused(t *testing.T) { } } +func TestInvalidGrantOnAServerFaultDoesNotForgetTheLogin(t *testing.T) { + for _, status := range []int{http.StatusTooManyRequests, http.StatusInternalServerError, http.StatusServiceUnavailable} { + t.Run(fmt.Sprint(status), func(t *testing.T) { + m, key := refreshRefusedBy(t, status, `{"error":"invalid_grant"}`) + err := m.Refresh(t.Context()) + require.Error(t, err) + assert.NotErrorIs(t, err, ErrLoginRefused) + creds, loadErr := m.store.Load(key) + require.NoError(t, loadErr, "a server fault is not proof that the refresh token is dead") + assert.Equal(t, "old-ref", creds.RefreshToken) + }) + } +} + // TestRefresh_PreservesIdentityAndBinding: a rotation replaces the tokens // and nothing else — the stored user, scope, and account binding survive a // token response that does not repeat them. diff --git a/internal/commands/connect_credential.go b/internal/commands/connect_credential.go index efbe7ba6a..1b707dd3a 100644 --- a/internal/commands/connect_credential.go +++ b/internal/commands/connect_credential.go @@ -64,8 +64,10 @@ func (w *credentialWatch) WrapTransport(inner http.RoundTripper) http.RoundTripp } // Run waits until the credential is refused, and returns why, or until ctx -// ends, and returns nil. confirm asks Basecamp whether it still takes the -// credential, and reports true when it does not. +// ends, and returns any refusal already observed, otherwise nil. confirm +// asks Basecamp whether it still takes the credential and must honor its +// context. Cancellation joins an in-flight confirmation before taking the +// final snapshot: it may already have deleted a refused credential. func (w *credentialWatch) Run(ctx context.Context, confirm func(context.Context) bool) error { var confirming <-chan bool var cancelConfirm context.CancelFunc @@ -77,6 +79,13 @@ func (w *credentialWatch) Run(ctx context.Context, confirm func(context.Context) for { select { case <-ctx.Done(): + if refused := w.finalError(nil); refused != nil { + return refused + } + if confirming != nil { + cancelConfirm() + <-confirming + } return w.finalError(nil) case <-w.refused: return w.finalError(nil) diff --git a/internal/commands/connect_credential_test.go b/internal/commands/connect_credential_test.go index 6944398f5..b72260bfe 100644 --- a/internal/commands/connect_credential_test.go +++ b/internal/commands/connect_credential_test.go @@ -143,6 +143,53 @@ func TestObservedRevocationSurvivesConcurrentShutdown(t *testing.T) { assert.Contains(t, problem.Hint, "basecamp auth login -P bot --expect-identity 4242") } +func TestCancellationJoinsAConfirmationThatHasAlreadyRefusedTheCredential(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + w := newCredentialWatch() + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + release := make(chan struct{}) + started := make(chan struct{}) + done := make(chan error, 1) + go func() { + done <- w.Run(ctx, func(context.Context) bool { + // The refused refresh has removed the credential, but this + // goroutine is preempted before publishing its answer. + close(started) + <-release + return true + }) + }() + w.unauthorized() + <-started + synctest.Wait() + cancel() // A concurrent missing-credential read stops another part. + synctest.Wait() + assert.Empty(t, done, "the final snapshot must not precede confirmation publication") + close(release) + synctest.Wait() + require.ErrorIs(t, <-done, errCredentialNotTaken) + assert.ErrorIs(t, w.finalError(output.ErrAuth("credential missing")), errCredentialNotTaken) + }) +} + +func TestCancellationStopsACooperativeConfirmation(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + w := newCredentialWatch() + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + done := make(chan error, 1) + go func() { + done <- w.Run(ctx, func(ctx context.Context) bool { <-ctx.Done(); return false }) + }() + w.unauthorized() + synctest.Wait() + cancel() + synctest.Wait() + require.NoError(t, <-done) + }) +} + func TestAnUnobservedRefusalDoesNotReplaceTheOriginalStop(t *testing.T) { w := newCredentialWatch() original := fmt.Errorf("feed stopped")