Present the device login as steps, say why no browser opened, count down the wait

e2e/auth.bats

@@ -77,15 +77,17 @@ 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"
 }
 
 @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"
 }

internal/auth/auth.go

@@ -533,24 +533,74 @@ 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
 }
 
 // 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.

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")
+	})
+}