diff --git a/e2e/auth.bats b/e2e/auth.bats index 986508c65..bdc400dea 100644 --- a/e2e/auth.bats +++ b/e2e/auth.bats @@ -77,7 +77,8 @@ load test_helper @test "basecamp auth login --help describes flags provider-neutrally" { run basecamp auth login --help assert_success - assert_output_contains "Headless authentication with manual browser instructions" + assert_output_contains "one-time code to approve from any device" + assert_output_contains "Launchpad has no device flow" assert_output_contains "ignored by Launchpad" assert_output_contains "default full" } @@ -85,7 +86,8 @@ load test_helper @test "basecamp profile create --help describes flags provider-neutrally" { run basecamp profile create --help assert_success - assert_output_contains "Headless authentication with manual browser instructions" + assert_output_contains "one-time code to approve from any device" + assert_output_contains "Launchpad has no device flow" assert_output_contains "ignored by Launchpad" assert_output_contains "default full" } diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 96964ce82..b3ec7acfb 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -533,6 +533,18 @@ type LoginOptions struct { // If nil, messages are suppressed for headless/SDK use. Logger func(msg string) + // Progress, when it is a terminal, carries the live wait line drawn + // while the device flow polls for approval (spinner and expiry + // countdown, redrawn in place). Any other writer, or nil, gets a static + // "Waiting for approval" line through Logger instead. + Progress io.Writer + + // headlessReason is why defaults() turned the browser launch off on its + // own ("SSH session", "no display"), so the flow can say so instead of + // silently printing a link. Empty when the caller asked (NoBrowser) or + // a launch is going to be tried. + headlessReason string + // deviceOptions are appended last to the SDK device-flow options. // Test seam: lets tests inject WithDeviceSleep/WithDeviceClock. deviceOptions []oauth.DeviceOption @@ -540,17 +552,55 @@ type LoginOptions struct { // defaults fills in default values for LoginOptions. func (o *LoginOptions) defaults() { - if !o.Remote && !o.Local && hostutil.IsRemoteSession() { + // A host that cannot show a browser (SSH, CI, no display) is a remote + // one whatever else was asked: the link is going to be opened on some + // other device, so a Launchpad login must take the pasted callback + // rather than listen on this host's loopback, which that device could + // never reach. --local is the person's word that the browser is right + // here and wins over the host heuristics; --no-browser only silences + // the launch and must not silence this. + hostReason := "" + if !o.Local { + hostReason = hostutil.HeadlessReason() + } + autoRemote := !o.Remote && hostReason != "" + if autoRemote { o.Remote = true } - if o.Remote || config.NonInteractiveEnv() { + // The launch is turned off when nobody could see the browser, and the + // reason is kept for the transcript when the CLI decided that on its + // own: the environment says no one is at this terminal, or the host has + // nowhere to open one. A caller who asked (--no-browser, --remote, + // --device-code) gets the link without commentary. + switch { + case o.NoBrowser, o.Remote && !autoRemote: o.NoBrowser = true + case config.NonInteractiveEnv(): + o.NoBrowser, o.headlessReason = true, "BASECAMP_NONINTERACTIVE is set" + case autoRemote: + o.NoBrowser, o.headlessReason = true, hostReason } if o.BrowserLauncher == nil && !o.NoBrowser { o.BrowserLauncher = openBrowser } } +// announceBrowser tries the launch and says what happened in one line. A +// failed launch is not a failed login — the link is already on screen — so +// the line points back at it. Explicit --no-browser prints nothing: the +// person asked for the link alone. +func (o *LoginOptions) announceBrowser(target string) { + switch { + case o.headlessReason != "": + o.log(fmt.Sprintf("Not opening a browser here (%s). Open the link on any device.", o.headlessReason)) + case o.NoBrowser || o.BrowserLauncher == nil: + case o.BrowserLauncher(target) != nil: + o.log("Couldn't open a browser. Open the link above.") + default: + o.log("Opening your browser… If nothing appears, open the link above.") + } +} + // log outputs a message if a logger is configured. func (o *LoginOptions) log(msg string) { if o.Logger != nil { @@ -698,6 +748,9 @@ func (m *Manager) loginLaunchpad(ctx context.Context, credKey string, oauthCfg * opts.log(" 4. Copy the full URL from your browser's address bar and") opts.log(" paste it below.") opts.log("") + // Remote implies NoBrowser, so this never launches: it says why the + // CLI chose this flow when the host, not a flag, chose it. + opts.announceBrowser(authURL) reader := opts.InputReader if reader == nil { @@ -721,17 +774,12 @@ func (m *Manager) loginLaunchpad(ctx context.Context, credKey string, oauthCfg * } defer func() { _ = listener.Close() }() - // Open browser for authentication - if opts.BrowserLauncher != nil { - if launchErr := opts.BrowserLauncher(authURL); launchErr != nil { - opts.log("\nCouldn't open browser automatically.\nOpen this URL in your browser:\n" + authURL + "\n\nWaiting for authentication...") - } else { - opts.log("\nOpening browser for authentication...") - opts.log("If the browser doesn't open, visit: " + authURL + "\n\nWaiting for authentication...") - } - } else { - opts.log("\nOpen this URL in your browser:\n" + authURL + "\n\nWaiting for authentication...") - } + opts.log("\nSign in to Basecamp\n") + opts.log(" Open this link in your browser") + opts.log(" " + authURL) + opts.log("") + opts.announceBrowser(authURL) + opts.log("Waiting for you to finish signing in… (times out in 5 minutes)") // Wait for OAuth callback with a hard timeout to avoid hanging indefinitely waitCtx, cancel := context.WithTimeout(ctx, 5*time.Minute) @@ -754,11 +802,8 @@ func (m *Manager) loginLaunchpad(ctx context.Context, credKey string, oauthCfg * creds.TokenEndpoint = oauthCfg.TokenEndpoint creds.Scope = "" - if opts.Verify != nil { - if err := opts.Verify(ctx, creds.AccessToken, oauthTypeLaunchpad); err != nil { - m.discardGrant(ctx, creds, opts.log) - return nil, err - } + if err := m.verifyBeforeStore(ctx, opts, creds, oauthTypeLaunchpad); err != nil { + return nil, err } if err := m.store.Save(credKey, creds); err != nil { return nil, err @@ -821,6 +866,7 @@ func (m *Manager) loginDevice(ctx context.Context, credKey string, oauthCfg *oau defer cancelDev() var displayErr error + var wait *approvalWait display := func(devAuth oauth.DeviceAuthorization) { // Validate the raw server-supplied URIs before printing or launching // anything: browser target is the code-embedding URI when valid, @@ -846,28 +892,43 @@ func (m *Manager) loginDevice(ctx context.Context, credKey string, oauthCfg *oau return } - opts.log("\nTo authenticate, open this URL in a browser on any device:") - opts.log(" " + shownURI) - opts.log("") - opts.log("and enter the code: " + userCode) - if devAuth.ExpiresIn > 0 { - opts.log(fmt.Sprintf("The code expires in %v.", time.Duration(devAuth.ExpiresIn)*time.Second)) + // Link first, code second, each on its own line so a double-click + // or a triple-click copies exactly one of them; the lifetime is + // stated where the code is. The warning is RFC 8628 §5.4's remote + // phishing defense in one sentence: a code someone else handed over + // approves their device, not this one. + lifetime := time.Duration(devAuth.ExpiresIn) * time.Second + codeStep := " 2. Enter this one-time code when asked" + if lifetime > 0 { + codeStep += " (expires in " + expiresIn(lifetime) + ")" } + opts.log("\nSign in to Basecamp\n") + opts.log(" 1. Open this link on any device") + opts.log(" " + shownURI) + opts.log(codeStep) + opts.log(" " + userCode) + opts.log("") + opts.log("Only continue if you started this login yourself. If a website or another") + opts.log("person gave you this code, press Ctrl-C now.") + opts.log("") // Flag matrix: default/--local launch the browser; --remote, // --device-code, and --no-browser (Remote implies NoBrowser) print // only. defaults() leaves BrowserLauncher nil in headless modes, but - // honor NoBrowser too so an injected launcher can't override it. - if !opts.NoBrowser && opts.BrowserLauncher != nil { - if launchErr := opts.BrowserLauncher(target); launchErr != nil { - opts.log("\nCouldn't open browser automatically — use the URL above.") - } else { - opts.log("\nOpening browser for authentication...") + // announceBrowser honors NoBrowser too so an injected launcher can't + // override it. + opts.announceBrowser(target) + if lifetime > 0 { + if wait = startApprovalWait(opts.Progress, time.Now().Add(lifetime)); wait != nil { + return } + opts.log("Waiting for approval… (the code expires in " + expiresIn(lifetime) + ")") + return } - opts.log("\nWaiting for approval...") + opts.log("Waiting for approval…") } token, err := oauth.PerformDeviceLogin(devCtx, oauthCfg, bc5ClientID, display, devOpts...) + wait.Stop() if displayErr != nil { // The malformed display data — not the cancellation it triggered — // is the real cause. @@ -901,11 +962,8 @@ func (m *Manager) loginDevice(ctx context.Context, credKey string, oauthCfg *oau creds.ExpiresAt = token.ExpiresAt.Unix() } - if opts.Verify != nil { - if err := opts.Verify(ctx, creds.AccessToken, oauthTypeBC5); err != nil { - m.discardGrant(ctx, creds, opts.log) - return nil, err - } + if err := m.verifyBeforeStore(ctx, opts, creds, oauthTypeBC5); err != nil { + return nil, err } if err := m.store.Save(credKey, creds); err != nil { return nil, err @@ -914,6 +972,30 @@ func (m *Manager) loginDevice(ctx context.Context, credKey string, oauthCfg *oau return &LoginResult{OAuthType: oauthTypeBC5, Scope: effectiveScope}, nil } +// verifyBeforeStore runs the caller's Verify hook and refuses to let a +// canceled login reach the store. The token arrives from the flow after +// the person may already have pressed Ctrl-C — the poll or exchange can +// complete in the same instant — and a non-strict verifier answers a +// canceled request with nil, so without this check a login the person +// stopped would still be saved and announced as a success. +func (m *Manager) verifyBeforeStore(ctx context.Context, opts *LoginOptions, creds *Credentials, oauthType string) error { + if err := ctx.Err(); err != nil { + m.discardGrant(ctx, creds, opts.log) + return err + } + if opts.Verify != nil { + if err := opts.Verify(ctx, creds.AccessToken, oauthType); err != nil { + m.discardGrant(ctx, creds, opts.log) + return err + } + } + if err := ctx.Err(); err != nil { + m.discardGrant(ctx, creds, opts.log) + return err + } + return nil +} + // validVerificationURL validates a server-supplied verification URI with the // same policy as other OAuth browser URLs (https, or http on loopback, no // userinfo). Returns the raw URL when valid, "" otherwise. diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 805451f12..99077d3fb 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -883,6 +883,8 @@ func TestLoginDefaultsNeverOpenABrowserUnderNonInteractiveEnv(t *testing.T) { t.Setenv("SSH_CONNECTION", "") t.Setenv("SSH_CLIENT", "") t.Setenv("SSH_TTY", "") + t.Setenv("CI", "") + t.Setenv("DISPLAY", ":0") opts := LoginOptions{Local: true} opts.defaults() assert.True(t, opts.NoBrowser) @@ -2210,3 +2212,76 @@ func TestSetUserIdentity_WritesUnderEnvToken(t *testing.T) { assert.Equal(t, "2", creds.UserID) assert.Equal(t, "who@example.com", creds.UserEmail) } + +// TestLoginLaunchpad_HeadlessHostTakesThePastedCallback: a host that cannot +// show a browser (here a CI runner) is also one whose loopback the browser +// on another device could never reach, so the Launchpad flow must ask for +// the pasted callback URL rather than listen. +func TestLoginLaunchpad_HeadlessHostTakesThePastedCallback(t *testing.T) { + t.Setenv("BASECAMP_NONINTERACTIVE", "") + t.Setenv("SSH_CONNECTION", "") + t.Setenv("SSH_CLIENT", "") + t.Setenv("SSH_TTY", "") + t.Setenv("DISPLAY", ":0") + t.Setenv("CI", "true") + + opts := LoginOptions{} + opts.defaults() + assert.True(t, opts.Remote, "a headless host pastes the callback") + assert.True(t, opts.NoBrowser) + assert.Equal(t, "CI environment", opts.headlessReason) + + local := LoginOptions{Local: true} + local.defaults() + assert.False(t, local.Remote, "--local keeps the loopback listener") + assert.False(t, local.NoBrowser) + + quiet := LoginOptions{NoBrowser: true} + quiet.defaults() + assert.True(t, quiet.Remote, "--no-browser silences the launch, not the headless routing") + assert.True(t, quiet.NoBrowser) + assert.Empty(t, quiet.headlessReason, "the person asked for the link alone; no commentary") +} + +// TestLoginLaunchpad_RemoteTranscriptSaysWhyWhenTheHostChose: the pasted +// callback flow announces the headless reason when the CLI selected it, +// and stays silent when --remote asked for it. +func TestLoginLaunchpad_RemoteTranscriptSaysWhyWhenTheHostChose(t *testing.T) { + t.Setenv("BASECAMP_NONINTERACTIVE", "") + t.Setenv("SSH_CONNECTION", "") + t.Setenv("SSH_CLIENT", "") + t.Setenv("SSH_TTY", "") + t.Setenv("DISPLAY", ":0") + t.Setenv("CI", "") + + // newDeviceTestManager pins the interactive host, so the CI variable + // is set after it; both cases select remote mode, so a loopback + // listener (and its five-minute wait) never starts. + run := func(t *testing.T, ci string, opts LoginOptions) string { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { http.NotFound(w, r) })) + t.Cleanup(srv.Close) + t.Setenv("BASECAMP_LAUNCHPAD_URL", srv.URL) + m := newDeviceTestManager(t, srv.URL) + t.Setenv("CI", ci) + cl := &collectLogger{} + opts.Logger = cl.log + opts.InputReader = strings.NewReader("") + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + _, err := m.Login(ctx, opts) + require.Error(t, err, "EOF on the paste prompt ends the login") + return cl.joined() + } + + t.Run("host chose", func(t *testing.T) { + out := run(t, "true", LoginOptions{}) + assert.Contains(t, out, "Paste the callback URL") + assert.Contains(t, out, "Not opening a browser here (CI environment). Open the link on any device.") + }) + t.Run("--remote asked", func(t *testing.T) { + out := run(t, "", LoginOptions{Remote: true}) + assert.Contains(t, out, "Paste the callback URL") + assert.NotContains(t, out, "Not opening a browser") + }) +} diff --git a/internal/auth/device_test.go b/internal/auth/device_test.go index b5b16c767..bb0f6dfc9 100644 --- a/internal/auth/device_test.go +++ b/internal/auth/device_test.go @@ -174,6 +174,9 @@ func newDeviceTestManager(t *testing.T, baseURL string) *Manager { t.Setenv("SSH_CONNECTION", "") t.Setenv("SSH_CLIENT", "") t.Setenv("SSH_TTY", "") + t.Setenv("CI", "") + t.Setenv("DISPLAY", ":0") + t.Setenv("BASECAMP_NONINTERACTIVE", "") // A pinned issuer in the developer's environment would bypass the // discovery every test here exercises. t.Setenv("BASECAMP_OAUTH_ISSUER", "") @@ -410,7 +413,98 @@ func TestLoginDevice_BrowserLaunchFailureContinues(t *testing.T) { }) require.NoError(t, err, "launch failure must not abort the flow") assert.Equal(t, "bc5", result.OAuthType) - assert.Contains(t, cl.joined(), "Couldn't open browser") + assert.Contains(t, cl.joined(), "Couldn't open a browser. Open the link above.") +} + +// TestLoginDevice_Transcript pins the device-flow copy: link first, code +// second, each on its own line, the lifetime beside the code, the phishing +// warning, and one browser line that says what happened. +func TestLoginDevice_Transcript(t *testing.T) { + as := startDeviceAS(t) + resource := startResourceServer(t, as.srv.URL) + + t.Run("browser opens", func(t *testing.T) { + m := newDeviceTestManager(t, resource.URL) + cl := &collectLogger{} + _, err := m.Login(context.Background(), LoginOptions{ + Logger: cl.log, + BrowserLauncher: func(string) error { return nil }, + deviceOptions: []oauth.DeviceOption{instantSleep()}, + }) + require.NoError(t, err) + assert.Contains(t, cl.joined(), strings.Join([]string{ + "Sign in to Basecamp", + "", + " 1. Open this link on any device", + " " + as.srv.URL + "/verify?user_code=ABCD-EFGH", + " 2. Enter this one-time code when asked (expires in 10 minutes)", + " ABCD-EFGH", + "", + "Only continue if you started this login yourself. If a website or another", + "person gave you this code, press Ctrl-C now.", + "", + "Opening your browser… If nothing appears, open the link above.", + "Waiting for approval… (the code expires in 10 minutes)", + }, "\n")) + }) + + t.Run("headless host says why", func(t *testing.T) { + m := newDeviceTestManager(t, resource.URL) + t.Setenv("SSH_TTY", "/dev/pts/3") + cl := &collectLogger{} + launched := 0 + _, err := m.Login(context.Background(), LoginOptions{ + Logger: cl.log, + BrowserLauncher: func(string) error { launched++; return nil }, + deviceOptions: []oauth.DeviceOption{instantSleep()}, + }) + require.NoError(t, err) + assert.Zero(t, launched) + assert.Contains(t, cl.joined(), "Not opening a browser here (SSH session). Open the link on any device.") + }) + + t.Run("--local overrides the host heuristics", func(t *testing.T) { + m := newDeviceTestManager(t, resource.URL) + t.Setenv("CI", "true") + launched := 0 + _, err := m.Login(context.Background(), LoginOptions{ + Local: true, + BrowserLauncher: func(string) error { launched++; return nil }, + deviceOptions: []oauth.DeviceOption{instantSleep()}, + }) + require.NoError(t, err) + assert.Equal(t, 1, launched) + }) + + t.Run("--no-browser prints the link alone", func(t *testing.T) { + m := newDeviceTestManager(t, resource.URL) + cl := &collectLogger{} + _, err := m.Login(context.Background(), LoginOptions{ + NoBrowser: true, + Logger: cl.log, + BrowserLauncher: func(string) error { t.Fatal("must not launch"); return nil }, + deviceOptions: []oauth.DeviceOption{instantSleep()}, + }) + require.NoError(t, err) + logs := cl.joined() + assert.NotContains(t, logs, "browser") + assert.Contains(t, logs, "Waiting for approval… (the code expires in 10 minutes)") + }) + + t.Run("a non-terminal progress writer gets the static line", func(t *testing.T) { + m := newDeviceTestManager(t, resource.URL) + cl := &collectLogger{} + var progress strings.Builder + _, err := m.Login(context.Background(), LoginOptions{ + NoBrowser: true, + Logger: cl.log, + Progress: &progress, + deviceOptions: []oauth.DeviceOption{instantSleep()}, + }) + require.NoError(t, err) + assert.Empty(t, progress.String(), "nothing is drawn on a non-terminal") + assert.Contains(t, cl.joined(), "Waiting for approval… (the code expires in 10 minutes)") + }) } func TestLoginDevice_ScopeWiring(t *testing.T) { @@ -1225,3 +1319,27 @@ func TestDiscoverOAuth_PinnedIssuerIsSanitizedForTheTerminal(t *testing.T) { assert.NotContains(t, err.Error(), "\u0085") assert.NotContains(t, cl.joined(), "\u0085") } + +// TestLoginDevice_CancelDuringVerifyStoresNothing: a cancel that lands after +// the token was issued but before it is stored — Ctrl-C in the same instant +// the approval completes — must not save the credential, even though a +// non-strict verifier answers a canceled request with nil. +func TestLoginDevice_CancelDuringVerifyStoresNothing(t *testing.T) { + as := startDeviceAS(t) + resource := startResourceServer(t, as.srv.URL) + m := newDeviceTestManager(t, resource.URL) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + _, err := m.Login(ctx, LoginOptions{ + NoBrowser: true, + deviceOptions: []oauth.DeviceOption{instantSleep()}, + Verify: func(context.Context, string, string) error { + cancel() + return nil + }, + }) + require.ErrorIs(t, err, context.Canceled) + _, loadErr := m.store.Load(config.NormalizeBaseURL(resource.URL)) + require.Error(t, loadErr, "a canceled login stores nothing") +} diff --git a/internal/auth/progress.go b/internal/auth/progress.go new file mode 100644 index 000000000..4be635da0 --- /dev/null +++ b/internal/auth/progress.go @@ -0,0 +1,149 @@ +package auth + +import ( + "fmt" + "io" + "os" + "sync" + "time" + + "github.com/charmbracelet/x/term" +) + +// approvalWait is the live line a terminal shows while the device flow polls +// for approval: a spinner, the prompt, and the time left before the code +// expires, redrawn in place and cleared when the flow ends. It exists so a +// person staring at the terminal can tell the CLI is still working and how +// long the code is good for — a silent poll and a "waiting" line printed +// once are indistinguishable from a hang. Non-terminal writers get nothing +// drawn; the caller logs a static line for those. +type approvalWait struct { + w io.Writer + deadline time.Time + now func() time.Time + interval time.Duration + // width is the terminal's column count, which picks the line's form: + // the full sentence, a short one, or nothing at all when even that + // would wrap — a wrapped line is redrawn from its continuation row and + // leaves the previous row behind on every tick. + width int + + stopOnce sync.Once + stop chan struct{} + done chan struct{} +} + +var approvalFrames = [...]string{"⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"} + +const approvalInterval = 100 * time.Millisecond + +// Column budgets for the two forms of the live line, counted on the +// widest countdown they render ("99:59"); a terminal narrower than the +// short form draws nothing and the caller logs the static line instead. +const ( + approvalLineFullWidth = len("⠋ Waiting for approval… code expires in 99:59") - 4 // multibyte glyphs count once + approvalLineShortWidth = len("⠋ Waiting… 99:59") - 4 +) + +// isTerminal is term.IsTerminal, swappable so a test can stand a pipe in +// for a terminal. +var isTerminal = term.IsTerminal + +// startApprovalWait begins drawing on w when it is a terminal that can take +// the redraw and returns nil otherwise, so callers can treat "no live line" +// uniformly: Stop on a nil *approvalWait is a no-op. A dumb terminal (an +// editor's shell buffer, a screen reader's session) is a real TTY that +// cannot erase a line, so it gets the static line like a pipe would. +func startApprovalWait(w io.Writer, deadline time.Time) *approvalWait { + f, ok := w.(*os.File) + if !ok || !isTerminal(f.Fd()) || os.Getenv("TERM") == "dumb" { + return nil + } + width, _, err := term.GetSize(f.Fd()) + if err != nil { + width = 80 + } + return runApprovalWait(w, deadline, time.Now, approvalInterval, width) +} + +// runApprovalWait is the injectable core: tests drive it with a buffer, a +// fixed clock, a short interval, and a chosen width. A width too narrow +// for even the short form yields nil, like a non-terminal. +func runApprovalWait(w io.Writer, deadline time.Time, now func() time.Time, interval time.Duration, width int) *approvalWait { + if width < approvalLineShortWidth { + return nil + } + a := &approvalWait{ + w: w, + deadline: deadline, + now: now, + interval: interval, + width: width, + stop: make(chan struct{}), + done: make(chan struct{}), + } + go a.run() + return a +} + +// line renders one tick of the live line in the form the width allows. +func (a *approvalWait) line(frame int) string { + left := remaining(a.deadline.Sub(a.now())) + if a.width < approvalLineFullWidth { + return fmt.Sprintf("%s Waiting… %s", approvalFrames[frame], left) + } + return fmt.Sprintf("%s Waiting for approval… code expires in %s", approvalFrames[frame], left) +} + +func (a *approvalWait) run() { + defer close(a.done) + ticker := time.NewTicker(a.interval) + defer ticker.Stop() + frame := 0 + for { + fmt.Fprint(a.w, "\r\033[2K"+a.line(frame)) + select { + case <-a.stop: + fmt.Fprint(a.w, "\r\033[2K") + return + case <-ticker.C: + frame = (frame + 1) % len(approvalFrames) + } + } +} + +// Stop clears the line and waits for the drawing goroutine to finish, so +// nothing the caller prints next can interleave with a redraw. +func (a *approvalWait) Stop() { + if a == nil { + return + } + a.stopOnce.Do(func() { + close(a.stop) + <-a.done + }) +} + +// remaining renders a duration as m:ss, floored at 0:00 — the shape a person +// reads as a countdown, unlike time.Duration's "9m41.3s". +func remaining(d time.Duration) string { + if d < 0 { + d = 0 + } + d = d.Round(time.Second) + return fmt.Sprintf("%d:%02d", int(d/time.Minute), int(d%time.Minute/time.Second)) +} + +// expiresIn renders a code lifetime for a static line: whole minutes when it +// is one, seconds otherwise ("10 minutes", "90 seconds"). +func expiresIn(d time.Duration) string { + switch { + case d >= time.Minute && d%time.Minute == 0: + if d == time.Minute { + return "1 minute" + } + return fmt.Sprintf("%d minutes", int(d/time.Minute)) + default: + return fmt.Sprintf("%d seconds", int(d/time.Second)) + } +} diff --git a/internal/auth/progress_test.go b/internal/auth/progress_test.go new file mode 100644 index 000000000..59d278a27 --- /dev/null +++ b/internal/auth/progress_test.go @@ -0,0 +1,108 @@ +package auth + +import ( + "os" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/charmbracelet/x/term" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// syncBuffer is a strings.Builder the drawing goroutine and the test can +// share. +type syncBuffer struct { + cl collectLogger +} + +func (b *syncBuffer) Write(p []byte) (int, error) { + b.cl.log(string(p)) + return len(p), nil +} + +func (b *syncBuffer) String() string { + b.cl.mu.Lock() + defer b.cl.mu.Unlock() + return strings.Join(b.cl.logs, "") +} + +func TestApprovalWaitDrawsCountdownAndClears(t *testing.T) { + base := time.Date(2026, 9, 12, 12, 0, 0, 0, time.UTC) + // The drawing goroutine reads the clock while the test advances it, so + // the elapsed time lives in an atomic. + var elapsed atomic.Int64 + now := func() time.Time { return base.Add(time.Duration(elapsed.Load())) } + buf := &syncBuffer{} + wait := runApprovalWait(buf, base.Add(10*time.Minute), now, time.Millisecond, 80) + + require.Eventually(t, func() bool { return strings.Contains(buf.String(), "code expires in 10:00") }, time.Second, time.Millisecond) + elapsed.Store(int64(19 * time.Second)) + require.Eventually(t, func() bool { return strings.Contains(buf.String(), "code expires in 9:41") }, time.Second, time.Millisecond) + + wait.Stop() + wait.Stop() // idempotent + + out := buf.String() + assert.True(t, strings.HasSuffix(out, "\r\033[2K"), "the line is cleared when the wait ends") + assert.Contains(t, out, "Waiting for approval…") + assert.NotContains(t, out, "\n", "the live line never scrolls") + + var none *approvalWait + none.Stop() +} + +func TestStartApprovalWaitNeedsATerminal(t *testing.T) { + assert.Nil(t, startApprovalWait(nil, time.Now().Add(time.Minute))) + assert.Nil(t, startApprovalWait(&strings.Builder{}, time.Now().Add(time.Minute))) +} + +// TestStartApprovalWaitSkipsADumbTerminal: a TTY that reports TERM=dumb +// cannot erase the line, so the redraw stays off there and the static wait +// line is what the caller logs. +func TestStartApprovalWaitSkipsADumbTerminal(t *testing.T) { + r, w, err := os.Pipe() + require.NoError(t, err) + defer r.Close() + defer w.Close() + isTerminal = func(uintptr) bool { return true } + defer func() { isTerminal = term.IsTerminal }() + + t.Setenv("TERM", "dumb") + assert.Nil(t, startApprovalWait(w, time.Now().Add(time.Minute))) + + t.Setenv("TERM", "xterm-256color") + wait := startApprovalWait(w, time.Now().Add(time.Minute)) + require.NotNil(t, wait, "a capable terminal gets the live line") + wait.Stop() +} + +// TestApprovalWaitFitsTheTerminalWidth: a narrow pane gets the short form +// and a pane too narrow for that gets no live line, so the redraw never +// wraps onto a second row it cannot clear. +func TestApprovalWaitFitsTheTerminalWidth(t *testing.T) { + base := time.Date(2026, 9, 12, 12, 0, 0, 0, time.UTC) + now := func() time.Time { return base } + deadline := base.Add(9*time.Minute + 41*time.Second) + + buf := &syncBuffer{} + wait := runApprovalWait(buf, deadline, now, time.Millisecond, 30) + require.NotNil(t, wait) + require.Eventually(t, func() bool { return strings.Contains(buf.String(), "Waiting… 9:41") }, time.Second, time.Millisecond) + wait.Stop() + assert.NotContains(t, buf.String(), "Waiting for approval", "the full form does not fit in 30 columns") + + assert.Nil(t, runApprovalWait(&syncBuffer{}, deadline, now, time.Millisecond, 10), "narrower than the short form draws nothing") +} + +func TestRemainingAndExpiresIn(t *testing.T) { + assert.Equal(t, "9:41", remaining(9*time.Minute+41*time.Second)) + assert.Equal(t, "0:05", remaining(4900*time.Millisecond)) + assert.Equal(t, "0:00", remaining(-time.Second)) + assert.Equal(t, "10 minutes", expiresIn(10*time.Minute)) + assert.Equal(t, "1 minute", expiresIn(time.Minute)) + assert.Equal(t, "90 seconds", expiresIn(90*time.Second)) + assert.Equal(t, "45 seconds", expiresIn(45*time.Second)) +} diff --git a/internal/cli/root.go b/internal/cli/root.go index 16301d5f9..d1b03d187 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -421,6 +421,13 @@ func Execute() { // Convert error to structured output apiErr := output.AsError(err) + // An interrupted command has already told the person what stopped; + // an error envelope on top would dress a Ctrl-C or a SIGTERM up as + // a failure. + if apiErr.Code == output.CodeInterrupted || apiErr.Code == output.CodeTerminated { + os.Exit(output.ExitCodeFor(apiErr.Code)) + } + // Commands whose stdout speaks a wire protocol (basecamp mcp: // JSON-RPC) keep errors off stdout entirely — an error envelope // there is a malformed protocol message that hides the real failure diff --git a/internal/commands/auth.go b/internal/commands/auth.go index 28fca10e8..65a0955e9 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -7,9 +7,11 @@ import ( "fmt" "io" "os" + "os/signal" "sort" "strconv" "strings" + "syscall" "time" "unicode" @@ -263,9 +265,16 @@ func buildLoginCmd(use string) *cobra.Command { Short: "Authenticate with Basecamp", Long: `Start the OAuth flow to authenticate with Basecamp, or import a personal access token. +Against Basecamp's own authorization server the flow prints a link and a +one-time code, opens the link in your browser, and waits for you to approve it +(a Launchpad server signs you in through a browser callback instead). Over SSH, +in CI, or on a host with no display the browser is skipped and the link is yours +to open on any device — your phone included; --local forces a launch anyway, +--no-browser skips it. Ctrl-C cancels the wait. + Examples: basecamp auth login # Browser (or device) flow - basecamp auth login --device-code # Headless: approve the printed code elsewhere + basecamp auth login --device-code # Headless: approve the printed code from any device basecamp auth login --expect-identity 12345 # Refuse the login unless it is this identity Import a personal access token from stdin (never pass it as an argument): @@ -297,14 +306,8 @@ named profile, creating the profile when --account is given. return runLoginWithToken(cmd, app, scope, expect) } - if app.Flags.JQFilter != "" { - return output.ErrJQNotSupported("the login command") - } - if machineOutputFlagSet(app) { - return output.ErrUsageHint("Interactive login cannot run under a machine output mode", - "Browser and device logins print instructions and wait for approval, which no envelope can carry. "+ - "Check credentials with `basecamp auth status`, or import a token headlessly: "+ - "`... | basecamp auth login --with-token -P --account --json`.") + if err := refuseMachineOutputLogin(app, "the login command"); err != nil { + return err } if err := refuseNonInteractiveLogin(deviceCode); err != nil { return err @@ -340,15 +343,19 @@ named profile, creating the profile when --account is given. // checked before it is stored, and a mismatch stores nothing. // Without one the identity line stays informational. verifier := &loginVerifier{app: app, expectIdentity: expect, account: app.Config.AccountID, strict: expect != 0} - result, err := app.Auth.Login(cmd.Context(), auth.LoginOptions{ + ctx, stop := loginContext(cmd) + result, err := app.Auth.Login(ctx, auth.LoginOptions{ Scope: scope, NoBrowser: noBrowser, Remote: remote, Local: local, LoginHint: loginHint, Logger: func(msg string) { fmt.Fprintln(w, msg) }, + Progress: w, Verify: verifier.verify, }) + err = loginOutcome(ctx, err, w, r) + stop() if err != nil { return err } @@ -374,10 +381,7 @@ named profile, creating the profile when --account is given. } cmd.Flags().StringVar(&scope, "scope", "", "OAuth scope: 'read' or 'full' (default full; ignored by Launchpad)") - cmd.Flags().BoolVar(&noBrowser, "no-browser", false, "Don't open browser automatically") - cmd.Flags().BoolVar(&remote, "remote", false, "Force remote/headless mode (paste callback URL instead of local listener)") - cmd.Flags().BoolVar(&local, "local", false, "Force local mode (override SSH auto-detection)") - cmd.Flags().BoolVar(&deviceCode, "device-code", false, "Headless authentication with manual browser instructions") + registerLoginFlowFlags(cmd, &noBrowser, &remote, &local, &deviceCode) cmd.Flags().BoolVar(&withToken, "with-token", false, "Read a personal access token from stdin instead of running OAuth (requires --profile)") cmd.Flags().StringVar(&expectIdentity, "expect-identity", "", "Identity ID the login must authenticate as; otherwise store nothing") cmd.Flags().StringVar(&loginHint, "login-hint", "", "Email address to sign in as on the device-flow approval page (ignored by Launchpad)") @@ -390,6 +394,71 @@ named profile, creating the profile when --account is given. return cmd } +// registerLoginFlowFlags declares the flags that choose how the OAuth flow +// reaches a browser, once, for every command that runs a login (auth login, +// profile create): their help text is the one place the headless behavior +// is described, so the two commands must not drift. +func registerLoginFlowFlags(cmd *cobra.Command, noBrowser, remote, local, deviceCode *bool) { + cmd.Flags().BoolVar(noBrowser, "no-browser", false, "Print the link instead of opening a browser") + cmd.Flags().BoolVar(remote, "remote", false, "Treat this as a remote session: print the link without opening a browser, and paste the callback URL back when the server has no device flow (auto-detected over SSH, in CI, and without a display)") + cmd.Flags().BoolVar(local, "local", false, "Treat this as a local session: open the browser here even over SSH, in CI, or without a display") + cmd.Flags().BoolVar(deviceCode, "device-code", false, "Print a link and one-time code to approve from any device, never opening a browser here (Launchpad has no device flow: paste the callback URL back instead)") +} + +// loginContext derives the context an interactive login waits under: Ctrl-C +// (and a SIGTERM) cancels it instead of killing the process mid-line, so +// the flow can put the terminal back — clear the live wait line, close the +// loopback listener, stop polling, discard a grant the server already +// issued — and say it was canceled. The signal that fired is the context's +// cause, so the exit status still tells an interrupt from a termination. +// The handler is released by the first signal, so a second one while the +// cleanup is still under way ends the process the default way instead of +// being swallowed. The stop function must run as soon as Login returns, +// after loginOutcome has read the context: stopping cancels the context +// too, and while the handler is registered a signal is swallowed instead +// of ending whatever the command does next. +func loginContext(cmd *cobra.Command) (context.Context, context.CancelFunc) { + ctx, cancel := context.WithCancelCause(cmd.Context()) + signals := make(chan os.Signal, 1) + signal.Notify(signals, os.Interrupt, syscall.SIGTERM) + go func() { + select { + case sig := <-signals: + signal.Stop(signals) + cancel(loginSignalError{sig}) + case <-ctx.Done(): + } + }() + return ctx, func() { + signal.Stop(signals) + cancel(nil) + } +} + +// loginSignalError is the cause a login context is canceled with when a signal, +// rather than the parent context, ended the wait. +type loginSignalError struct{ os.Signal } + +func (s loginSignalError) Error() string { return "login stopped by " + s.String() } + +// loginOutcome turns the result of Login into the error the root will +// render. A login the person canceled is not a failure: the human line goes +// to the terminal here and the error carries the interrupted code — or the +// terminated code when a SIGTERM ended the wait — which the root exits with +// silently. Everything else, nil included, is returned as it came. +func loginOutcome(ctx context.Context, err error, w io.Writer, r *output.Renderer) error { + if err == nil || !errors.Is(ctx.Err(), context.Canceled) { + return err + } + fmt.Fprintln(w) + fmt.Fprintln(w, r.Muted.Render("Login canceled. Nothing was stored.")) + var sig loginSignalError + if errors.As(context.Cause(ctx), &sig) && sig.Signal == syscall.SIGTERM { + return output.ErrTerminated("login terminated") + } + return output.ErrInterrupted("login canceled") +} + // runLoginWithToken imports a personal access token from stdin as the // active profile's credential. Everything that can be checked without the // token is checked before stdin is read; the token is then verified through @@ -644,6 +713,24 @@ func refuseNonInteractiveLogin(deviceCode bool) error { "or check credentials with `basecamp auth status`.") } +// refuseMachineOutputLogin is the output half of the login gate, shared by +// every command that runs an OAuth flow: the transcript and the live wait +// line go to stdout, which a machine-output envelope also owns, so a +// login under --json would write prose and control sequences ahead of the +// envelope. +func refuseMachineOutputLogin(app *appctx.App, command string) error { + if app.Flags.JQFilter != "" { + return output.ErrJQNotSupported(command) + } + if !machineOutputFlagSet(app) { + return nil + } + return output.ErrUsageHint("Interactive login cannot run under a machine output mode", + "Browser and device logins print instructions and wait for approval, which no envelope can carry. "+ + "Check credentials with `basecamp auth status`, or import a token headlessly: "+ + "`... | basecamp auth login --with-token -P --account --json`.") +} + // machineOutputFlagSet reports whether an explicit output flag asked for a // machine format. The config-driven formats are deliberately excluded: a // configured format=json must not lock a person out of an interactive login. diff --git a/internal/commands/auth_login_signal_unix_test.go b/internal/commands/auth_login_signal_unix_test.go new file mode 100644 index 000000000..6e0aad59f --- /dev/null +++ b/internal/commands/auth_login_signal_unix_test.go @@ -0,0 +1,67 @@ +//go:build unix + +package commands + +import ( + "bytes" + "context" + "os" + "syscall" + "testing" + "time" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/output" +) + +// The signal is sent to this process while loginContext's handler is +// registered, so it cancels the login instead of ending the test binary. +func TestLoginContextExitsWithTheStatusOfTheSignalThatStoppedIt(t *testing.T) { + for _, tc := range []struct { + signal syscall.Signal + code string + exit int + }{ + {syscall.SIGINT, output.CodeInterrupted, 130}, + {syscall.SIGTERM, output.CodeTerminated, 143}, + } { + t.Run(tc.signal.String(), func(t *testing.T) { + cmd := &cobra.Command{} + cmd.SetContext(context.Background()) + ctx, stop := loginContext(cmd) + defer stop() + + require.NoError(t, syscall.Kill(os.Getpid(), tc.signal)) + select { + case <-ctx.Done(): + case <-time.After(5 * time.Second): + t.Fatal("the signal did not cancel the login context") + } + + var out bytes.Buffer + err := loginOutcome(ctx, context.Canceled, &out, output.NewRenderer(&out, false)) + var outErr *output.Error + require.ErrorAs(t, err, &outErr) + assert.Equal(t, tc.code, outErr.Code) + assert.Equal(t, tc.exit, output.ExitCodeFor(outErr.Code)) + assert.Contains(t, out.String(), "Login canceled. Nothing was stored.") + }) + } +} + +func TestLoginContextStopReleasesTheSignalHandler(t *testing.T) { + cmd := &cobra.Command{} + cmd.SetContext(context.Background()) + ctx, stop := loginContext(cmd) + stop() + + select { + case <-ctx.Done(): + case <-time.After(5 * time.Second): + t.Fatal("stop did not cancel the login context") + } + assert.NotErrorAs(t, context.Cause(ctx), new(loginSignalError), "a stop is not a signal") +} diff --git a/internal/commands/auth_login_test.go b/internal/commands/auth_login_test.go index 6dcb6bf2c..247632e7c 100644 --- a/internal/commands/auth_login_test.go +++ b/internal/commands/auth_login_test.go @@ -40,6 +40,8 @@ func TestAuthLoginDeviceCodeForcesRemoteMode(t *testing.T) { t.Setenv("SSH_CONNECTION", "") t.Setenv("SSH_CLIENT", "") t.Setenv("SSH_TTY", "") + t.Setenv("CI", "") + t.Setenv("DISPLAY", ":0") // No protected-resource metadata (404) → Launchpad fallback, pointed at // this server. The token endpoint is never reached. @@ -741,6 +743,15 @@ func TestAuthLoginRefusesMachineOutputForInteractiveFlows(t *testing.T) { assert.Empty(t, srv.seenBearers()) }) } + t.Run("jq", func(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{}) + app.Flags.JQFilter = ".name" + _, err := runLogin(t, app, strings.NewReader(""), "--device-code") + require.Error(t, err) + assert.Contains(t, err.Error(), "--jq is not supported by the login command") + assert.Empty(t, srv.seenBearers()) + }) } // TestAuthLoginRefusesNonInteractiveEnvWithoutDeviceCode covers the env half @@ -780,7 +791,8 @@ func TestAuthLoginDeviceCodeRunsUnderNonInteractiveEnv(t *testing.T) { out, err := runLogin(t, app, strings.NewReader(""), "--device-code") require.NoError(t, err, out) - assert.Contains(t, out, "and enter the code: ABCD-EFGH") + assert.Contains(t, out, " 2. Enter this one-time code when asked (expires in 10 minutes)\n ABCD-EFGH") + assert.NotContains(t, out, "browser", "--device-code asked for the link alone; no launch, no commentary") assert.Contains(t, out, "Authentication successful") } @@ -820,6 +832,8 @@ func TestAuthLoginDeviceFlowExpectIdentityStoresNothingOnMismatch(t *testing.T) t.Setenv("SSH_CONNECTION", "") t.Setenv("SSH_CLIENT", "") t.Setenv("SSH_TTY", "") + t.Setenv("CI", "") + t.Setenv("DISPLAY", ":0") out, err := runLogin(t, app, strings.NewReader(""), "--device-code", "--expect-identity", "1") require.Error(t, err) @@ -1001,6 +1015,8 @@ func TestAuthLoginDeviceFlowWithoutExpectationKeepsBestEffortIdentity(t *testing t.Setenv("SSH_CONNECTION", "") t.Setenv("SSH_CLIENT", "") t.Setenv("SSH_TTY", "") + t.Setenv("CI", "") + t.Setenv("DISPLAY", ":0") out, err := runLogin(t, app, strings.NewReader(""), "--device-code") require.NoError(t, err, out) @@ -1190,6 +1206,8 @@ func TestAuthLoginDeviceFlowExpectIdentityKeepsAShortLivedAccessToken(t *testing t.Setenv("SSH_CONNECTION", "") t.Setenv("SSH_CLIENT", "") t.Setenv("SSH_TTY", "") + t.Setenv("CI", "") + t.Setenv("DISPLAY", ":0") out, err := runLogin(t, app, strings.NewReader(""), "--device-code", "--expect-identity", "28142355") require.NoError(t, err, out) @@ -1219,3 +1237,57 @@ func TestAuthLoginWithTokenRefusesToRewriteANonObjectProfilesValue(t *testing.T) _, loadErr := app.Auth.GetStore().Load("profile:bot") assert.Error(t, loadErr) } + +// TestAuthLoginCtrlCCancelsCleanly: an interrupt while the device flow is +// waiting for approval ends the login with the interrupted code (exit 130, +// rendered by nothing but the command's own line) and stores nothing. +func TestAuthLoginCtrlCCancelsCleanly(t *testing.T) { + srv := startLoginIdentityServer(t, "dev-tok") + pending := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/oauth/device_authorizations": + fmt.Fprintf(w, `{"device_code":"dc","user_code":"ABCD-EFGH","verification_uri":%q,"expires_in":600,"interval":1}`, "http://"+r.Host+"/verify") + case "/oauth/tokens": + w.WriteHeader(http.StatusBadRequest) + fmt.Fprint(w, `{"error":"authorization_pending"}`) + default: + srv.srv.Config.Handler.ServeHTTP(w, r) + } + }) + srv.srv.Config.Handler = pending + app, _ := loginTestApp(t, srv, &config.Config{}) + t.Setenv("BASECAMP_OAUTH_ISSUER", srv.srv.URL) + t.Setenv("SSH_CONNECTION", "") + t.Setenv("SSH_CLIENT", "") + t.Setenv("SSH_TTY", "") + t.Setenv("CI", "") + t.Setenv("DISPLAY", ":0") + + // The parent context stands in for the signal: loginContext derives + // from it, so canceling it is what Ctrl-C does to the flow. + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + time.AfterFunc(300*time.Millisecond, cancel) + + cmd := NewAuthCmd() + cmd.SetArgs([]string{"login", "--no-browser"}) + cmd.SetContext(appctx.WithApp(ctx, app)) + var out bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&out) + cmd.SilenceErrors = true + cmd.SilenceUsage = true + + err := cmd.Execute() + require.Error(t, err) + var outErr *output.Error + require.ErrorAs(t, err, &outErr) + assert.Equal(t, output.CodeInterrupted, outErr.Code) + assert.Equal(t, output.ExitInterrupted, output.ExitCodeFor(outErr.Code)) + + assert.Contains(t, out.String(), "ABCD-EFGH", "the code was shown before the cancel") + assert.Contains(t, out.String(), "Login canceled. Nothing was stored.") + assert.NotContains(t, out.String(), "Authentication successful") + assert.False(t, app.Auth.IsAuthenticated(), "a canceled login stores nothing") +} diff --git a/internal/commands/profile.go b/internal/commands/profile.go index 177abdecc..bbf74ef45 100644 --- a/internal/commands/profile.go +++ b/internal/commands/profile.go @@ -249,6 +249,9 @@ Examples: profileCfg.AccountID = accountID } + if err := refuseMachineOutputLogin(app, "profile create"); err != nil { + return err + } if err := refuseNonInteractiveLogin(deviceCode); err != nil { return err } @@ -280,15 +283,20 @@ Examples: // With an expectation the credential is checked before it is // stored and a mismatch stores nothing; without one the // identity lookup stays informational. + w := cmd.OutOrStdout() verifier := &loginVerifier{app: app, expectIdentity: expect, account: accountID, strict: expect != 0} - loginResult, err := app.Auth.Login(cmd.Context(), auth.LoginOptions{ + ctx, stop := loginContext(cmd) + loginResult, err := app.Auth.Login(ctx, auth.LoginOptions{ Scope: scope, NoBrowser: noBrowser, Remote: remote, Local: local, - Logger: func(msg string) { fmt.Println(msg) }, + Logger: func(msg string) { fmt.Fprintln(w, msg) }, + Progress: w, Verify: verifier.verify, }) + err = loginOutcome(ctx, err, w, output.NewRenderer(w, false)) + stop() if err != nil { // Restore in-memory state delete(app.Config.Profiles, name) @@ -333,10 +341,7 @@ Examples: cmd.Flags().StringVar(&baseURL, "base-url", "", "Basecamp API base URL (default: https://3.basecampapi.com)") cmd.Flags().StringVar(&scope, "scope", "", "OAuth scope: 'read' or 'full' (default full; ignored by Launchpad)") cmd.Flags().StringVar(&accountID, "account", "", "Account ID") - cmd.Flags().BoolVar(&noBrowser, "no-browser", false, "Don't open browser automatically") - cmd.Flags().BoolVar(&remote, "remote", false, "Force remote/headless mode (paste callback URL instead of local listener)") - cmd.Flags().BoolVar(&local, "local", false, "Force local mode (override SSH auto-detection)") - cmd.Flags().BoolVar(&deviceCode, "device-code", false, "Headless authentication with manual browser instructions") + registerLoginFlowFlags(cmd, &noBrowser, &remote, &local, &deviceCode) cmd.Flags().StringVar(&expectIdentity, "expect-identity", "", "Identity ID the login must authenticate as; otherwise create nothing") cmd.MarkFlagsMutuallyExclusive("remote", "local") cmd.MarkFlagsMutuallyExclusive("device-code", "local") diff --git a/internal/commands/profile_test.go b/internal/commands/profile_test.go index 386aa1102..26804a695 100644 --- a/internal/commands/profile_test.go +++ b/internal/commands/profile_test.go @@ -246,6 +246,8 @@ func TestProfileCreateDeviceCodeForcesRemoteMode(t *testing.T) { t.Setenv("SSH_CONNECTION", "") t.Setenv("SSH_CLIENT", "") t.Setenv("SSH_TTY", "") + t.Setenv("CI", "") + t.Setenv("DISPLAY", ":0") // No protected-resource metadata (404) → Launchpad fallback, pointed at // this server. The token endpoint is never reached. @@ -287,6 +289,51 @@ func TestProfileCreateDeviceCodeForcesRemoteMode(t *testing.T) { "--device-code must select the remote paste-callback flow") } +// TestProfileCreateRefusesMachineOutput: the login transcript and the live +// wait line share stdout with the envelope, so an explicit machine-output +// flag — --jq included — refuses the flow before discovery, as auth login +// does. +func TestProfileCreateRefusesMachineOutput(t *testing.T) { + for name, tc := range map[string]struct { + set func(*appctx.App) + message string + }{ + "json": {func(a *appctx.App) { a.Flags.JSON = true }, "machine output mode"}, + "jq": {func(a *appctx.App) { a.Flags.JQFilter = ".name" }, "--jq is not supported by profile create"}, + } { + t.Run(name, func(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Errorf("no request may be made under a machine output mode: %s", r.URL.Path) + http.NotFound(w, r) + })) + defer srv.Close() + + cfg := &config.Config{BaseURL: srv.URL, CacheDir: t.TempDir(), Sources: make(map[string]string)} + authMgr := auth.NewManager(cfg, srv.Client()) + app := &appctx.App{Config: cfg, Auth: authMgr} + tc.set(app) + + root := &cobra.Command{Use: "basecamp"} + root.AddCommand(NewProfileCmd()) + root.SetArgs([]string{"profile", "create", "test-profile", "--base-url", srv.URL}) + root.SetContext(appctx.WithApp(context.Background(), app)) + root.SetOut(&bytes.Buffer{}) + root.SetErr(&bytes.Buffer{}) + root.SilenceErrors = true + root.SilenceUsage = true + + err := root.Execute() + require.Error(t, err) + var outErr *output.Error + require.ErrorAs(t, err, &outErr) + assert.Equal(t, output.CodeUsage, outErr.Code) + assert.Contains(t, outErr.Message, tc.message) + }) + } +} + // TestProfileCreateRefusesNonInteractiveEnv: profile create runs the same // OAuth flows as login, so it carries the same gate. Without --device-code // it refuses before discovery; with it, a Launchpad-backed server — whose @@ -1331,6 +1378,8 @@ func TestProfileCreateExpectIdentityCreatesNothingOnMismatch(t *testing.T) { t.Setenv("SSH_CONNECTION", "") t.Setenv("SSH_CLIENT", "") t.Setenv("SSH_TTY", "") + t.Setenv("CI", "") + t.Setenv("DISPLAY", ":0") create := func(expect string) error { return executeProfileCommand(NewProfileCmd(), app, "create", "bot", "--base-url", srv.srv.URL, "--account", "999", "--device-code", "--expect-identity", expect) } diff --git a/internal/commands/wizard.go b/internal/commands/wizard.go index d7bdba1a8..4b0c8d072 100644 --- a/internal/commands/wizard.go +++ b/internal/commands/wizard.go @@ -1,6 +1,7 @@ package commands import ( + "context" "encoding/json" "errors" "fmt" @@ -274,13 +275,13 @@ func showAuthenticationStart(w io.Writer, styles *tui.Styles, stepByStep bool) s return " " } +// authenticationLogger indents the login transcript under the wizard's +// step and drops the Launchpad discovery line: it names an authorization +// URL nobody needs to see, and the flow's own "Sign in to Basecamp" block +// that follows says everything the person does. func authenticationLogger(w io.Writer, prefix string) func(string) { - launchpadOpeningShown := false return func(message string) { if strings.HasPrefix(message, "Authenticating via launchpad (") { - message = "Opening browser for Basecamp login..." - launchpadOpeningShown = true - } else if launchpadOpeningShown && strings.TrimSpace(message) == "Opening browser for authentication..." { return } fmt.Fprintln(w, prefix+message) @@ -318,10 +319,18 @@ func wizardAuth(cmd *cobra.Command, app *appctx.App, styles *tui.Styles, showRes } loggerPrefix := showAuthenticationStart(w, styles, showResult) - result, err := app.Auth.Login(cmd.Context(), auth.LoginOptions{ - Logger: authenticationLogger(w, loggerPrefix), + ctx, stop := loginContext(cmd) + result, err := app.Auth.Login(ctx, auth.LoginOptions{ + Logger: authenticationLogger(w, loggerPrefix), + Progress: w, }) + err = loginOutcome(ctx, err, w, output.NewRenderer(w, false)) + canceled := errors.Is(ctx.Err(), context.Canceled) + stop() if err != nil { + if canceled { + return "", err + } return "", fmt.Errorf("authentication failed: %w", err) } diff --git a/internal/commands/wizard_test.go b/internal/commands/wizard_test.go index c2ad8ca83..76e3fa19d 100644 --- a/internal/commands/wizard_test.go +++ b/internal/commands/wizard_test.go @@ -380,20 +380,20 @@ func TestShowFastAuthenticationStart(t *testing.T) { prefix := showAuthenticationStart(&buf, styles, false) log := authenticationLogger(&buf, prefix) log("Authenticating via launchpad (https://launchpad.37signals.com/authorization/new)") - log("\nOpening browser for authentication...") + log("Opening your browser… If nothing appears, open the link above.") assert.Empty(t, prefix) - assert.Equal(t, "Opening browser for Basecamp login...\n", buf.String()) + assert.Equal(t, "Opening your browser… If nothing appears, open the link above.\n", buf.String(), + "the discovery line is dropped; the flow's own browser line is the only one") assert.NotContains(t, buf.String(), "Step 1") assert.NotContains(t, buf.String(), "launchpad") - assert.NotContains(t, buf.String(), "Opening browser for authentication") var deviceFlow bytes.Buffer deviceLog := authenticationLogger(&deviceFlow, "") deviceLog("Authenticating via https://3.basecampapi.com (device flow)") - deviceLog("\nOpening browser for authentication...") + deviceLog("Opening your browser… If nothing appears, open the link above.") assert.Contains(t, deviceFlow.String(), "Authenticating via https://3.basecampapi.com (device flow)") - assert.Contains(t, deviceFlow.String(), "Opening browser for authentication") + assert.Contains(t, deviceFlow.String(), "Opening your browser") } func TestShowFastSuccess(t *testing.T) { diff --git a/internal/hostutil/hostutil.go b/internal/hostutil/hostutil.go index d93b85c1a..8837c3eb7 100644 --- a/internal/hostutil/hostutil.go +++ b/internal/hostutil/hostutil.go @@ -125,6 +125,55 @@ func IsRemoteSession() bool { os.Getenv("SSH_TTY") != "" } +// HeadlessReason reports why this host cannot show a browser to the person +// at the terminal, or "" when launching one is worth trying. It is the +// browser-launch half of session detection: IsRemoteSession decides whether a +// loopback callback can be reached at all, this decides whether `open` / +// `xdg-open` would land anywhere the user can see. The reason is written for +// a terminal line ("Not opening a browser here (SSH session)"). +// +// Only environment is consulted, so the answer is cheap and deterministic: +// an SSH session (any of the three variables sshd sets), a CI runner (the +// CI variable every major provider exports), or a Unix host with neither an +// X11 nor a Wayland display. macOS and Windows have no display variable to +// check; a GUI-less session there is rare enough to leave to the launch +// error path. +func HeadlessReason() string { + switch { + case IsRemoteSession(): + return "SSH session" + case envTruthy(os.Getenv("CI")): + return "CI environment" + case unixWithoutDisplay(): + return "no display" + default: + return "" + } +} + +// unixWithoutDisplay is true on the Unix platforms whose browsers need a +// display server when neither DISPLAY nor WAYLAND_DISPLAY is set. +func unixWithoutDisplay() bool { + switch runtime.GOOS { + case "linux", "freebsd", "openbsd", "netbsd", "dragonfly", "solaris", "illumos": + return os.Getenv("DISPLAY") == "" && os.Getenv("WAYLAND_DISPLAY") == "" + default: + return false + } +} + +// envTruthy reads a boolean-ish environment value the way CI providers set +// it: "true", "1", "yes" and "on" (any case) count; everything else, +// including the empty string, does not. +func envTruthy(v string) bool { + switch strings.ToLower(strings.TrimSpace(v)) { + case "1", "true", "yes", "on": + return true + default: + return false + } +} + // OpenBrowser opens the specified URL in the default browser. func OpenBrowser(url string) error { var cmd string diff --git a/internal/hostutil/hostutil_test.go b/internal/hostutil/hostutil_test.go index ab65fcf74..87427c721 100644 --- a/internal/hostutil/hostutil_test.go +++ b/internal/hostutil/hostutil_test.go @@ -203,3 +203,54 @@ func TestIsTrustedBasecampHost(t *testing.T) { }) } } + +// pinInteractiveHost clears every signal HeadlessReason reads so a test +// observes the interactive answer regardless of the machine it runs on (CI +// exports CI=true and Linux runners have no display). +func pinInteractiveHost(t *testing.T) { + t.Helper() + for _, key := range []string{"SSH_CONNECTION", "SSH_CLIENT", "SSH_TTY", "CI"} { + t.Setenv(key, "") + } + t.Setenv("DISPLAY", ":0") + t.Setenv("WAYLAND_DISPLAY", "") +} + +func TestHeadlessReason(t *testing.T) { + pinInteractiveHost(t) + assert.Empty(t, HeadlessReason(), "a display and no SSH or CI is interactive") + + t.Run("ssh", func(t *testing.T) { + pinInteractiveHost(t) + t.Setenv("SSH_TTY", "/dev/pts/0") + assert.Equal(t, "SSH session", HeadlessReason()) + }) + t.Run("ci", func(t *testing.T) { + for _, v := range []string{"true", "1", "YES"} { + pinInteractiveHost(t) + t.Setenv("CI", v) + assert.Equal(t, "CI environment", HeadlessReason(), "CI=%s", v) + } + pinInteractiveHost(t) + t.Setenv("CI", "false") + assert.Empty(t, HeadlessReason(), "CI=false is not a CI runner") + }) + t.Run("ssh outranks ci", func(t *testing.T) { + pinInteractiveHost(t) + t.Setenv("CI", "true") + t.Setenv("SSH_CLIENT", "10.0.0.1 12345 22") + assert.Equal(t, "SSH session", HeadlessReason()) + }) + t.Run("no display", func(t *testing.T) { + pinInteractiveHost(t) + t.Setenv("DISPLAY", "") + want := "" + if unixWithoutDisplay() { + want = "no display" + } + assert.Equal(t, want, HeadlessReason()) + + t.Setenv("WAYLAND_DISPLAY", "wayland-0") + assert.Empty(t, HeadlessReason(), "a Wayland display is a display") + }) +} diff --git a/internal/output/codes.go b/internal/output/codes.go index e6f70766d..a0b620e3d 100644 --- a/internal/output/codes.go +++ b/internal/output/codes.go @@ -40,6 +40,19 @@ const ( ExitLimit = 10 // Account limit reached (507) ) +// CodeInterrupted marks a command the person stopped with Ctrl-C after it +// had started waiting on them (a login waiting for approval); +// CodeTerminated the same stop by a SIGTERM from whatever supervises the +// process. The command has already said what happened on its own terms, so +// the root renders nothing more and exits with the shell's conventional +// status for a process the signal ended. +const ( + CodeInterrupted = "interrupted" + ExitInterrupted = 130 + CodeTerminated = "terminated" + ExitTerminated = 143 +) + // ExitCodeFor returns the exit code for a given error code. func ExitCodeFor(code string) int { switch code { @@ -47,6 +60,10 @@ func ExitCodeFor(code string) int { return ExitValidation case CodeLimitExceeded: return ExitLimit + case CodeInterrupted: + return ExitInterrupted + case CodeTerminated: + return ExitTerminated } return clioutput.ExitCodeFor(code) } diff --git a/internal/output/errors.go b/internal/output/errors.go index d48b0a0f6..2d365b01d 100644 --- a/internal/output/errors.go +++ b/internal/output/errors.go @@ -122,6 +122,18 @@ func ErrAuth(msg string) *Error { } } +// ErrInterrupted reports a command stopped by the person at the terminal. +// msg is for machine consumers; the command prints its own human line. +func ErrInterrupted(msg string) *Error { + return &Error{Code: CodeInterrupted, Message: msg} +} + +// ErrTerminated reports a command stopped by a SIGTERM while it waited. +// msg is for machine consumers; the command prints its own human line. +func ErrTerminated(msg string) *Error { + return &Error{Code: CodeTerminated, Message: msg} +} + func ErrForbiddenScope() *Error { return &Error{ Code: CodeForbidden, diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index bb708a396..1a3485ae2 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -1391,7 +1391,7 @@ basecamp auth status # Check auth basecamp auth login # Re-authenticate basecamp auth login --scope full # Full access (the default; ignored by Launchpad) basecamp auth login --scope read # Read-only access (ignored by Launchpad) -basecamp auth login --device-code # Headless authentication with manual browser instructions +basecamp auth login --device-code # Print a link and one-time code to approve from any device, never opening a browser here (Launchpad has no device flow: paste the callback URL back instead) BASECAMP_NONINTERACTIVE=1 basecamp auth login --device-code # The only OAuth login that runs under BASECAMP_NONINTERACTIVE, and only where the server offers the device flow (Launchpad does not); browser and pasted-callback flows refuse — prefer --with-token basecamp auth login --with-token -P bot --account # Import a personal access token from stdin (pipe it in) basecamp auth login --expect-identity # Discard the login unless it authenticated as this identity