diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 7c06d0a19..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 @@ -526,6 +527,11 @@ func (m *Manager) errNewerCredential(oauthType string) *output.Error { return e } +// 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 // 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 +754,11 @@ func (m *Manager) refreshCredential(ctx context.Context, origin string, creds *C if reused && inContainer() { msg += ". " + containerLoginAdvice } - return m.errAuth(msg) + refused := m.errAuth(msg) + if creds.OAuthType == oauthTypeBC5 { + 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..a95a1f8fc 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,40 @@ 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) + } +} + +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. @@ -2117,6 +2149,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 new file mode 100644 index 000000000..1b707dd3a --- /dev/null +++ b/internal/commands/connect_credential.go @@ -0,0 +1,183 @@ +package commands + +import ( + "context" + "errors" + "net/http" + "sync" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + + "github.com/basecamp/basecamp-cli/internal/auth" +) + +// 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. +// +// 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, 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, 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 { + 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 struct{}, 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 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 + defer func() { + if cancelConfirm != nil { + cancelConfirm() + } + }() + 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) + case <-w.suspect: + if confirming != nil { + continue + } + checkCtx, cancel := context.WithCancel(ctx) + cancelConfirm = cancel + done := make(chan bool, 1) + confirming = done + go func() { + refused := confirm(checkCtx) + if refused { + w.refuse(errCredentialNotTaken) + } + done <- refused + }() + 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 + } + } + } +} + +// 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 <- struct{}{}: + 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 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 +} + +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..b72260bfe --- /dev/null +++ b/internal/commands/connect_credential_test.go @@ -0,0 +1,534 @@ +package commands + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "sync" + "sync/atomic" + "testing" + "testing/synctest" + "time" + + "github.com/stretchr/testify/assert" + "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" + "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" +) + +// 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, anAgent("Ryan Singer (agent)", "agent"), mustNotAsk(t)) + var e *output.Error + require.ErrorAs(t, exit, &e) + 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) +} + +// 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, errCredentialNotTaken) + assert.Equal(t, 1, asked) + + 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) + 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) { + 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() + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + started, stopped := make(chan struct{}), make(chan struct{}) + done := make(chan error, 1) + go func() { + 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") + } +} + +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 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") + 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() + 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() + } + 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 +// 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) + } +} + +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) { + 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()) +} + +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() })) + 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) { + 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 } +} + +// 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..c8c7b59fc 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; "+botSignInHint(r.name(), r.identity)) + } } 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 30ce5f838..2b94f61f3 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 } @@ -345,11 +347,20 @@ 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, 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) + watchedClient := basecamp.NewClient(&basecamp.Config{BaseURL: app.Config.BaseURL}, watchedTokens, watchedOpts...).ForAccount(account) + queue, err := connector.NewQueue(connector.DefaultBacklogWarn, connector.DefaultBacklogPause) 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 } @@ -362,7 +373,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: watchedClient, + } intake, err := connector.New(intakeOpts) if err != nil { return err @@ -374,7 +387,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 +459,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 confirmCredentialRefused(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}) @@ -461,11 +477,12 @@ 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}, }) }) } wg.Wait() + firstErr = watch.finalError(firstErr) mu.Lock() sig := received @@ -475,8 +492,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) } @@ -487,20 +504,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 -// refused token renewal is already Basecamp's answer. -func connectorStoppedBy(err error, agent, profile string, refused func() bool) (state, detail string, exit error) { - disconnected := errors.Is(err, auth.ErrAgentCredentialRefused) || (feedAuthorizationFailed(err) && refused()) - if !disconnected { +// 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, who connectRunAs, refused func() bool) (state, detail string, exit error) { + credentialRefused := renewalRefused(err) || + errors.Is(err, errCredentialNotTaken) || + (feedAuthorizationFailed(err) && refused()) + 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 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. +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 @@ -543,26 +584,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 @@ -572,19 +614,63 @@ 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 creds.RefreshToken != "" { + if app.Auth.RefreshRefusal(creds) != nil { + return false + } + 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 { @@ -605,6 +691,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..6f324fe25 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,30 @@ 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")) + 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 // reconnects it. func TestConnectStatusLeadsWithDisconnected(t *testing.T) { @@ -137,6 +203,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.