Skip to content

Commit 0bce547

Browse files
committed
Keep credential confirmation responsive and refuse only proven revoked logins
1 parent 6315137 commit 0bce547

7 files changed

Lines changed: 157 additions & 24 deletions

File tree

‎internal/auth/auth.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -526,11 +526,9 @@ func (m *Manager) errNewerCredential(oauthType string) *output.Error {
526526
return e
527527
}
528528

529-
// ErrLoginRefused is the cause of a refresh the token endpoint refused with
530-
// invalid_grant: Basecamp no longer takes the login's refresh token, because
531-
// the session expired or was revoked, or, on Launchpad, because the
532-
// environment names a different OAuth client than the one it was issued to.
533-
// No retry changes that answer; only signing in again does.
529+
// ErrLoginRefused marks a BC5 refresh refused with invalid_grant: the
530+
// session expired or was revoked. Launchpad's same error can mean a
531+
// recoverable OAuth-client mismatch, so it does not carry this cause.
534532
var ErrLoginRefused = errors.New("the login's refresh token was refused")
535533

536534
// forgetRefusedGrant deletes the stored credential only while it still
@@ -756,7 +754,9 @@ func (m *Manager) refreshCredential(ctx context.Context, origin string, creds *C
756754
msg += ". " + containerLoginAdvice
757755
}
758756
refused := m.errAuth(msg)
759-
refused.Cause = ErrLoginRefused
757+
if creds.OAuthType == oauthTypeBC5 {
758+
refused.Cause = ErrLoginRefused
759+
}
760760
return refused
761761
}
762762

‎internal/auth/auth_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2135,6 +2135,7 @@ func TestRefresh_InvalidGrantOnLaunchpadKeepsTheCredential(t *testing.T) {
21352135
require.ErrorAs(t, err, &cliErr)
21362136
assert.Equal(t, output.CodeAuth, cliErr.Code)
21372137
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")
2138+
assert.NotErrorIs(t, err, ErrLoginRefused, "a client mismatch must not stop the connector as a revoked login")
21382139
assert.Equal(t, "Run: basecamp auth login", cliErr.Hint)
21392140

21402141
creds, loadErr := m.store.Load(key)

‎internal/commands/connect_credential.go‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,14 +63,37 @@ func (w *credentialWatch) WrapTransport(inner http.RoundTripper) http.RoundTripp
6363
// ends, and returns nil. confirm asks Basecamp whether it still takes the
6464
// credential, and reports true when it does not.
6565
func (w *credentialWatch) Run(ctx context.Context, confirm func(context.Context) bool) error {
66+
var confirming <-chan bool
67+
var cancelConfirm context.CancelFunc
68+
defer func() {
69+
if cancelConfirm != nil {
70+
cancelConfirm()
71+
}
72+
}()
6673
for {
6774
select {
6875
case <-ctx.Done():
6976
return nil
7077
case err := <-w.refused:
7178
return err
7279
case <-w.suspect:
73-
if confirm(ctx) {
80+
if confirming != nil {
81+
continue
82+
}
83+
checkCtx, cancel := context.WithCancel(ctx)
84+
cancelConfirm = cancel
85+
done := make(chan bool, 1)
86+
confirming = done
87+
go func() { done <- confirm(checkCtx) }()
88+
case refused := <-confirming:
89+
cancelConfirm()
90+
confirming = nil
91+
// All suspicions that arrived during this check share its answer.
92+
select {
93+
case <-w.suspect:
94+
default:
95+
}
96+
if refused {
7497
return errCredentialNotTaken
7598
}
7699
}

‎internal/commands/connect_credential_test.go‎

Lines changed: 116 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,9 @@ import (
77
"net/http"
88
"net/http/httptest"
99
"sync"
10+
"sync/atomic"
1011
"testing"
12+
"testing/synctest"
1113
"time"
1214

1315
"github.com/stretchr/testify/assert"
@@ -66,26 +68,92 @@ func TestTheCredentialWatchAsksAboutAnUnauthorizedRead(t *testing.T) {
6668
// A 401 Basecamp does not confirm, from a proxy say, leaves the run going,
6769
// and the next one asks again.
6870
func TestTheCredentialWatchRunsOnWhenBasecampStillTakesTheCredential(t *testing.T) {
71+
synctest.Test(t, func(t *testing.T) {
72+
w := newCredentialWatch()
73+
transport := w.WrapTransport(statusTransport(http.StatusUnauthorized))
74+
ctx, cancel := context.WithCancel(t.Context())
75+
defer cancel()
76+
var asks atomic.Int32
77+
done := make(chan error, 1)
78+
go func() {
79+
done <- w.Run(ctx, func(context.Context) bool { asks.Add(1); return false })
80+
}()
81+
for attempt := range int32(2) {
82+
roundTrip(t, transport)
83+
synctest.Wait() // The previous confirmation has finished, not just started.
84+
assert.Equal(t, attempt+1, asks.Load())
85+
}
86+
cancel()
87+
require.NoError(t, <-done, "ended by its context, the watch stops nothing")
88+
})
89+
}
90+
91+
func TestARefusedRenewalInterruptsAPendingConfirmation(t *testing.T) {
6992
w := newCredentialWatch()
70-
transport := w.WrapTransport(statusTransport(http.StatusUnauthorized))
7193
ctx, cancel := context.WithCancel(t.Context())
7294
defer cancel()
73-
asks := make(chan struct{})
95+
started, stopped := make(chan struct{}), make(chan struct{})
7496
done := make(chan error, 1)
7597
go func() {
76-
done <- w.Run(ctx, func(context.Context) bool { asks <- struct{}{}; return false })
98+
done <- w.Run(ctx, func(ctx context.Context) bool {
99+
close(started)
100+
<-ctx.Done()
101+
close(stopped)
102+
return false
103+
})
77104
}()
105+
w.unauthorized()
106+
select {
107+
case <-started:
108+
case <-time.After(5 * time.Second):
109+
t.Fatal("confirmation never started")
110+
}
111+
w.refuse(auth.ErrLoginRefused)
112+
select {
113+
case err := <-done:
114+
require.ErrorIs(t, err, auth.ErrLoginRefused)
115+
case <-time.After(5 * time.Second):
116+
t.Fatal("a definitive refusal waited behind confirmation")
117+
}
118+
select {
119+
case <-stopped:
120+
case <-time.After(5 * time.Second):
121+
t.Fatal("the in-flight confirmation was not canceled")
122+
}
123+
}
78124

79-
for range 2 {
80-
roundTrip(t, transport)
81-
select {
82-
case <-asks:
83-
case <-time.After(10 * time.Second):
84-
t.Fatal("the watch did not ask about a 401")
125+
func TestUnauthorizedReadsShareOnePendingConfirmation(t *testing.T) {
126+
synctest.Test(t, func(t *testing.T) {
127+
w := newCredentialWatch()
128+
ctx, cancel := context.WithCancel(t.Context())
129+
defer cancel()
130+
release := make(chan struct{})
131+
var calls atomic.Int32
132+
done := make(chan error, 1)
133+
go func() {
134+
done <- w.Run(ctx, func(context.Context) bool {
135+
calls.Add(1)
136+
<-release
137+
return false
138+
})
139+
}()
140+
w.unauthorized()
141+
synctest.Wait()
142+
require.EqualValues(t, 1, calls.Load())
143+
for range 20 {
144+
w.unauthorized()
85145
}
86-
}
87-
cancel()
88-
require.NoError(t, <-done, "ended by its context, the watch stops nothing")
146+
synctest.Wait()
147+
assert.EqualValues(t, 1, calls.Load())
148+
close(release)
149+
synctest.Wait()
150+
assert.EqualValues(t, 1, calls.Load(), "pending suspicions share the completed answer")
151+
w.unauthorized()
152+
synctest.Wait()
153+
assert.EqualValues(t, 2, calls.Load(), "a later refusal is checked again")
154+
cancel()
155+
require.NoError(t, <-done)
156+
})
89157
}
90158

91159
// A bot user's refresh the token endpoint refuses is Basecamp's answer
@@ -167,6 +235,30 @@ func TestABotUsersRefreshThatCanPassDoesNotStopTheRun(t *testing.T) {
167235
}
168236
}
169237

238+
func TestRepeatedUnauthorizedReadsDuringARefreshHoldDoNotSignOut(t *testing.T) {
239+
bc := newBotBasecamp(t)
240+
bc.answerRefresh(http.StatusTooManyRequests, `{"error":"temporarily_unavailable"}`)
241+
bc.answerMe(func(string) int { return http.StatusUnauthorized })
242+
app := bc.app(t)
243+
for range 2 {
244+
assert.False(t, confirmCredentialRefused(t.Context(), app, setup.KindBotUser, "bot"))
245+
}
246+
assert.Equal(t, []string{"refresh"}, bc.asked(), "a held refresh must not probe the old token or refresh twice")
247+
}
248+
249+
func TestALocalRefreshPreflightFailureDoesNotProveRevocation(t *testing.T) {
250+
bc := newBotBasecamp(t)
251+
bc.answerMe(func(string) int { return http.StatusUnauthorized })
252+
app := bc.app(t)
253+
creds, err := app.Auth.GetStore().Load("profile:bot")
254+
require.NoError(t, err)
255+
creds.TokenEndpoint = "ftp://invalid.example/tokens"
256+
require.Error(t, app.Auth.RefreshRefusal(creds))
257+
require.NoError(t, app.Auth.GetStore().Save("profile:bot", creds))
258+
assert.False(t, confirmCredentialRefused(t.Context(), app, setup.KindBotUser, "bot"))
259+
assert.Empty(t, bc.asked(), "a configuration fault is not proof from the old token")
260+
}
261+
170262
// A bot login stored from a bare token has no refresh to try; the token it
171263
// has is the only one it will ever have, so a 401 for it is the answer.
172264
func TestABotLoginWithNoRefreshIsAskedAboutWithItsOwnToken(t *testing.T) {
@@ -182,6 +274,18 @@ func TestABotLoginWithNoRefreshIsAskedAboutWithItsOwnToken(t *testing.T) {
182274
assert.Equal(t, []string{"me old-tok"}, bc.asked())
183275
}
184276

277+
func TestABoostRetainsARefusedRenewalForTheCredentialWatch(t *testing.T) {
278+
w := newCredentialWatch()
279+
tokens := w.Tokens(tokenFunc(func() (string, error) { return "", refusedLogin() }))
280+
client := basecamp.NewClient(&basecamp.Config{BaseURL: "http://127.0.0.1:1"}, tokens).ForAccount("555")
281+
booster := connector.SDKBooster{Client: client}
282+
_, err := booster.Boost(t.Context(), 123, "ok")
283+
require.Error(t, err)
284+
// Even if acknowledgement only logs the failure, the definitive
285+
// refusal remains visible to the connector's credential watch.
286+
assert.ErrorIs(t, runWatch(t, w, mustNotConfirm(t)), auth.ErrLoginRefused)
287+
}
288+
185289
// Every other failure is the reads' own: neither a renewal that failed for
186290
// another reason nor another status asks Basecamp or stops the run.
187291
func TestTheCredentialWatchIgnoresOtherFailures(t *testing.T) {

‎internal/commands/connect_operator.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -310,7 +310,7 @@ func connectStatusSummary(r connectStatusReport) string {
310310
case connector.ConnectionDisconnected:
311311
parts = append(parts, "disconnected")
312312
case connector.ConnectionSignedOut:
313-
parts = append(parts, "signed out")
313+
parts = append(parts, "signed out; "+botSignInHint(r.name(), r.identity))
314314
}
315315
}
316316
if r.Status.Hold != nil {

‎internal/commands/connect_run.go‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -354,6 +354,7 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error {
354354
watch := newCredentialWatch()
355355
watchedOpts := append(connectSDKOptions(), basecamp.WithTransportWrapper(watch))
356356
watchedTokens := watch.Tokens(tokens)
357+
watchedClient := basecamp.NewClient(&basecamp.Config{BaseURL: app.Config.BaseURL}, watchedTokens, watchedOpts...).ForAccount(account)
357358

358359
queue, err := connector.NewQueue(connector.DefaultBacklogWarn, connector.DefaultBacklogPause)
359360
if err != nil {
@@ -373,7 +374,7 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error {
373374
intakeOpts.Lines = lines
374375
intakeOpts.Logger = logger
375376
intakeOpts.Membership = connector.SDKMembership{
376-
Client: basecamp.NewClient(&basecamp.Config{BaseURL: app.Config.BaseURL}, watchedTokens, watchedOpts...).ForAccount(account),
377+
Client: watchedClient,
377378
}
378379
intake, err := connector.New(intakeOpts)
379380
if err != nil {
@@ -476,7 +477,7 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error {
476477
return connector.RunHandoff(ctx, connector.HandoffOptions{
477478
Ledger: ledger, Served: served.Current, Buckets: buckets, Pings: policy.HearsPings, AgentID: agentID,
478479
Lines: lines, Logger: logger, Started: started,
479-
AckContent: file.Ack().Content, Booster: connector.SDKBooster{Client: accountClient},
480+
AckContent: file.Ack().Content, Booster: connector.SDKBooster{Client: watchedClient},
480481
})
481482
})
482483
}
@@ -531,7 +532,7 @@ func connectorStoppedBy(err error, who connectRunAs, refused func() bool) (state
531532
return state, e.Message, e
532533
}
533534

534-
// errCredentialRefused says Basecamp refused who's credential in the words
535+
// errCredentialRefused says Basecamp refused whose credential in the words
535536
// for its kind, with the connection state status keeps for it: an Agent was
536537
// disconnected and is reconnected; a bot user's login was refused and the
537538
// bot signs in again.
@@ -647,7 +648,10 @@ func botLoginRefused(ctx context.Context, app *appctx.App, name string) bool {
647648
if err != nil {
648649
return false
649650
}
650-
if app.Auth.RefreshRefusal(creds) == nil {
651+
if creds.RefreshToken != "" {
652+
if app.Auth.RefreshRefusal(creds) != nil {
653+
return false
654+
}
651655
if err := app.Auth.Refresh(ctx); err != nil {
652656
return errors.Is(err, auth.ErrLoginRefused)
653657
}

‎internal/commands/connect_run_disconnect_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,7 @@ func TestConnectStatusLeadsWithSignedOut(t *testing.T) {
177177
assert.Equal(t, " Signed out: "+detail+". Sign it in again: basecamp auth login -P bot --expect-identity 4242", lines[2])
178178
assert.NotContains(t, out.String(), "Disconnected")
179179
assert.True(t, strings.HasPrefix(connectStatusSummary(report), "signed out"))
180+
assert.Contains(t, connectStatusSummary(report), "basecamp auth login -P bot --expect-identity 4242", "JSON output also carries the recovery command")
180181
}
181182

182183
// Status says the agent was disconnected first, with the command that

0 commit comments

Comments
 (0)