diff --git a/cmd/auth/login/cmd.go b/cmd/auth/login/cmd.go index 156cde747..4bf1e4f15 100644 --- a/cmd/auth/login/cmd.go +++ b/cmd/auth/login/cmd.go @@ -17,6 +17,7 @@ package login import ( "context" "errors" + "os" "strings" "github.com/datarobot/cli/internal/auth" @@ -78,14 +79,31 @@ func RunE(cmd *cobra.Command, args []string) error { //nolint: cyclop noBrowser, _ := cmd.Flags().GetBool("no-browser") + timeout, _ := cmd.Flags().GetDuration("timeout") + if timeout < 0 { + log.Errorf("--timeout must be zero or positive, got %s", timeout) + + cmd.SilenceUsage = true + + return cli.ErrSilent + } + key, err := auth.RunBrowserLoginWith(cmd.Context(), datarobotHost, auth.LoginOptions{ NoBrowser: noBrowser, + Timeout: timeout, }) if err != nil { - log.Error(err) - cmd.SilenceUsage = true + // The bare timeout error is a Go string with no next step; the help block is. + if errors.Is(err, auth.ErrLoginTimedOut) { + auth.FprintLoginTimeoutHelp(os.Stderr) + + return cli.ErrSilent + } + + log.Error(err) + return err } @@ -128,6 +146,7 @@ If the browser cannot be opened, the CLI prints a link to open yourself. Pass // Read directly from cobra rather than binding to viper: this is a transient // per-invocation flag and must never be persisted to drconfig.yaml. cmd.Flags().Bool("no-browser", false, "print the login link instead of opening a browser") + cmd.Flags().Duration("timeout", 0, "how long to wait for the browser callback (default 5m)") return cmd } diff --git a/cmd/auth/login/cmd_test.go b/cmd/auth/login/cmd_test.go new file mode 100644 index 000000000..41709fc7c --- /dev/null +++ b/cmd/auth/login/cmd_test.go @@ -0,0 +1,105 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package login + +import ( + "context" + "io" + "os" + "testing" + "time" + + "github.com/datarobot/cli/internal/cli" + "github.com/datarobot/cli/internal/config" + "github.com/datarobot/cli/internal/config/viperx" + "github.com/datarobot/cli/internal/testutil" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCmd_HasTimeoutFlag(t *testing.T) { + cmd := Cmd() + + f := cmd.Flags().Lookup("timeout") + require.NotNil(t, f, "dr auth login must expose --timeout") + assert.Equal(t, "0s", f.DefValue, "zero default means DefaultLoginTimeout applies") + + require.NoError(t, cmd.Flags().Set("timeout", "30s")) + + got, err := cmd.Flags().GetDuration("timeout") + require.NoError(t, err) + assert.Equal(t, 30*time.Second, got) +} + +func TestCmd_HasNoBrowserFlag(t *testing.T) { + assert.NotNil(t, Cmd().Flags().Lookup("no-browser"), "dr auth login must keep --no-browser") +} + +// TestRunE_TimeoutPrintsHelpAndReturnsSilent drives the timeout branch end to end: +// a tiny --timeout with no browser and a dead endpoint reaches ErrLoginTimedOut fast. +func TestRunE_TimeoutPrintsHelpAndReturnsSilent(t *testing.T) { + testutil.SetTestHomeDir(t, t.TempDir()) + t.Setenv("DATAROBOT_ENDPOINT", "") + t.Setenv("DATAROBOT_API_TOKEN", "") + + viperx.Reset() + t.Cleanup(viperx.Reset) + viperx.Set(config.DataRobotURL, "https://nonexistent.invalid") + + cmd := Cmd() + cmd.SetContext(context.Background()) + require.NoError(t, cmd.Flags().Set("no-browser", "true")) + require.NoError(t, cmd.Flags().Set("timeout", "50ms")) + + oldOut, oldErr := os.Stdout, os.Stderr + rOut, wOut, err := os.Pipe() + require.NoError(t, err) + + rErr, wErr, err := os.Pipe() + require.NoError(t, err) + + os.Stdout, os.Stderr = wOut, wErr + + runErr := RunE(cmd, nil) + + require.NoError(t, wOut.Close()) + require.NoError(t, wErr.Close()) + + os.Stdout, os.Stderr = oldOut, oldErr + + stderr, _ := io.ReadAll(rErr) + _, _ = io.ReadAll(rOut) + + require.ErrorIs(t, runErr, cli.ErrSilent, "a timeout returns the silent sentinel, not the raw error") + assert.Contains(t, string(stderr), "authorization came back", "the recovery help must reach stderr") +} + +func TestRunE_RejectsNegativeTimeout(t *testing.T) { + testutil.SetTestHomeDir(t, t.TempDir()) + t.Setenv("DATAROBOT_ENDPOINT", "") + t.Setenv("DATAROBOT_API_TOKEN", "") + + viperx.Reset() + t.Cleanup(viperx.Reset) + viperx.Set(config.DataRobotURL, "https://nonexistent.invalid") + + cmd := Cmd() + cmd.SetContext(context.Background()) + require.NoError(t, cmd.Flags().Set("no-browser", "true")) + require.NoError(t, cmd.Flags().Set("timeout", "-1s")) + + err := RunE(cmd, nil) + assert.ErrorIs(t, err, cli.ErrSilent, "a negative --timeout is rejected before the browser flow starts") +} diff --git a/cmd/templates/setup/loginModel.go b/cmd/templates/setup/loginModel.go index 411c11be6..e52625e32 100644 --- a/cmd/templates/setup/loginModel.go +++ b/cmd/templates/setup/loginModel.go @@ -16,6 +16,7 @@ package setup import ( "context" + "errors" "fmt" "strings" @@ -36,6 +37,10 @@ type LoginModel struct { type errMsg struct{ error } //nolint: errname +// Unwrap lets errors.Is reach the wrapped error, so callers can match sentinels +// like auth.ErrLoginTimedOut through the tea.Msg envelope. +func (e errMsg) Unwrap() error { return e.error } + type startedMsg struct { flow *auth.BrowserFlow message string @@ -117,7 +122,10 @@ func (lm LoginModel) Update(msg tea.Msg) (LoginModel, tea.Cmd) { func (lm LoginModel) View() string { var sb strings.Builder - if lm.loginMessage != "" { + if errors.Is(lm.err, auth.ErrLoginTimedOut) { + sb.WriteString("Login timed out. Run 'dr auth login' to retry, or set ") + sb.WriteString("DATAROBOT_ENDPOINT and DATAROBOT_API_TOKEN to skip the browser.\n\n") + } else if lm.loginMessage != "" { sb.WriteString(lm.loginMessage) } else if lm.err != nil { fmt.Fprintf(&sb, "something went wrong: %s", lm.err) diff --git a/cmd/templates/setup/loginModel_test.go b/cmd/templates/setup/loginModel_test.go index e5b04eb46..f4f6e367e 100644 --- a/cmd/templates/setup/loginModel_test.go +++ b/cmd/templates/setup/loginModel_test.go @@ -16,6 +16,7 @@ package setup import ( "bytes" + "fmt" "os" "path/filepath" "testing" @@ -23,13 +24,27 @@ import ( tea "github.com/charmbracelet/bubbletea" "github.com/charmbracelet/x/exp/teatest" + "github.com/datarobot/cli/internal/auth" "github.com/datarobot/cli/internal/config" "github.com/datarobot/cli/internal/config/viperx" "github.com/datarobot/cli/internal/testutil" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/suite" "gopkg.in/yaml.v3" ) +// TestLoginModel_View_TimeoutShowsRecovery pins that a login timeout surfaces the +// friendly recovery line, which needs errMsg.Unwrap for the errors.Is match. +func TestLoginModel_View_TimeoutShowsRecovery(t *testing.T) { + timeoutErr := fmt.Errorf("no browser authorization within 5m0s: %w", auth.ErrLoginTimedOut) + lm, _ := LoginModel{}.Update(errMsg{timeoutErr}) + + view := lm.View() + assert.Contains(t, view, "Login timed out", "a timeout must surface the recovery line, not the raw error") + assert.Contains(t, view, "dr auth login") + assert.NotContains(t, view, "something went wrong") +} + func TestLoginModelSuite(t *testing.T) { suite.Run(t, new(LoginModelTestSuite)) } diff --git a/docs/commands/auth.md b/docs/commands/auth.md index 18936a1b6..d0ae792ad 100644 --- a/docs/commands/auth.md +++ b/docs/commands/auth.md @@ -44,6 +44,7 @@ dr auth login | Flag | Description | | -------------- | ---------------------------------------------------------- | | `--no-browser` | Print the login link instead of opening a browser (useful over SSH) | +| `--timeout` | How long to wait for the browser callback (default 5m); raise it behind a slow identity provider | **What happens:** @@ -97,7 +98,27 @@ $ dr auth login ``` If another `dr` process is already waiting on `localhost:51164`, the new one asks it to -release the port and takes over. The wait times out after 5 minutes. +release the port and takes over. + +If no callback arrives before the timeout (5 minutes by default), the CLI prints the next +steps instead of a bare error: retry, since a sign-in error often clears on the second +attempt, or set the `DATAROBOT_ENDPOINT` and `DATAROBOT_API_TOKEN` environment variables to +authenticate without the browser. + +```bash +$ dr auth login +❌ No authorization came back from the browser. + +If your browser showed a sign-in error, click through it and run login again. +The sign-in often completes on the second attempt: + dr auth login + +Or set the DATAROBOT_ENDPOINT and DATAROBOT_API_TOKEN environment variables +(from Developer Tools) to authenticate without the browser. +``` + +Behind a slow identity provider where a cold sign-in with MFA needs more than 5 minutes, +raise the deadline with `--timeout`, for example `dr auth login --timeout 10m`. ### `logout` diff --git a/docs/development/authentication.md b/docs/development/authentication.md index 52ebd86bf..386086e94 100644 --- a/docs/development/authentication.md +++ b/docs/development/authentication.md @@ -145,6 +145,10 @@ Three rules matter when changing this code: CLI-to-CLI port handover in `listenReclaimingPort` uses a Go `http.Client`, which sends no fetch metadata, so rejecting absent would deadlock two concurrent logins. Do not gate on `Sec-Fetch-Site`: the genuine callback is legitimately cross-site. +- **Surface the timeout, don't leak the raw error.** `Wait` returns `ErrLoginTimedOut` + after `DefaultLoginTimeout` (override with `LoginOptions.Timeout`, exposed as + `dr auth login --timeout`). Both callers print `FprintLoginTimeoutHelp` to stderr on it, + since the bare Go timeout string gives the user no next step. `auth.RunBrowserLoginWith` accepts `LoginOptions{NoBrowser: true}` for `--no-browser`, which renders the link prominently without reporting a failure. diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 0ed525fd2..d8447e551 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -407,7 +407,12 @@ func EnsureAuthenticated(ctx context.Context) bool { //nolint: cyclop key, err := APIKeyCallbackFunc(ctx, datarobotHost) if err != nil { - log.Error("Failed to retrieve API key.", "error", err) + if errors.Is(err, ErrLoginTimedOut) { + FprintLoginTimeoutHelp(os.Stderr) + } else { + log.Error("Failed to retrieve API key.", "error", err) + } + return false } diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index ffe3e7109..ea3345069 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -120,6 +120,27 @@ func TestEnsureAuthenticated_MissingCredentials(t *testing.T) { assert.Equal(t, server.URL, baseURL, "Expected base URL to be set from test server") } +func TestEnsureAuthenticated_LoginTimeoutPrintsHelp(t *testing.T) { + _, cleanup := setupTestEnvironment(t) + defer cleanup() + + viperx.Set(config.DataRobotAPIKey, "") + os.Unsetenv("DATAROBOT_API_TOKEN") + + APIKeyCallbackFunc = func(_ context.Context, _ string) (string, error) { + return "", ErrLoginTimedOut + } + + var result bool + + _, stderr := captureStdoutStderr(t, func() { + result = EnsureAuthenticated(context.Background()) + }) + + assert.False(t, result, "a login timeout must fail EnsureAuthenticated") + assert.Contains(t, stderr, "dr auth login", "the timeout help must reach the user on stderr") +} + func TestEnsureAuthenticated_ExpiredCredentials(t *testing.T) { _, cleanup := setupTestEnvironment(t) defer cleanup() diff --git a/internal/auth/browserflow.go b/internal/auth/browserflow.go index ca1b63358..186e1be45 100644 --- a/internal/auth/browserflow.go +++ b/internal/auth/browserflow.go @@ -18,6 +18,7 @@ import ( "context" "errors" "fmt" + "io" "net" "net/http" "os" @@ -45,6 +46,10 @@ const DefaultLoginTimeout = 5 * time.Minute // another CLI process takes over the callback port. var ErrLoginInterrupted = errors.New("login was interrupted") +// ErrLoginTimedOut is returned when no browser callback arrives before the +// deadline. Callers print FprintLoginTimeoutHelp instead of the raw error. +var ErrLoginTimedOut = errors.New("browser login timed out") + // BrowserFlow owns the local HTTP listener that receives the API key after the // user authorizes the CLI in their browser. // @@ -150,7 +155,7 @@ func (f *BrowserFlow) Wait(ctx context.Context) (string, error) { case <-ctx.Done(): if errors.Is(ctx.Err(), context.DeadlineExceeded) { - return "", fmt.Errorf("timed out after %s waiting for browser authorization: %w", f.timeout, ctx.Err()) + return "", fmt.Errorf("no browser authorization within %s: %w", f.timeout, ErrLoginTimedOut) } log.Debug("Login context cancelled, exiting auth wait") @@ -159,6 +164,21 @@ func (f *BrowserFlow) Wait(ctx context.Context) (string, error) { } } +// FprintLoginTimeoutHelp writes recovery steps after a browser login timed out: +// retry (a sign-in error often clears next try), or use the env-var credentials. +func FprintLoginTimeoutHelp(w io.Writer) { + base, info := writerStyles(w) + + fmt.Fprintln(w, base.Render("❌ No authorization came back from the browser.")) + fmt.Fprintln(w) + fmt.Fprintln(w, base.Render("If your browser showed a sign-in error, click through it and run login again.")) + fmt.Fprintln(w, base.Render("The sign-in often completes on the second attempt:")) + fmt.Fprintln(w, info.Render(" dr auth login")) + fmt.Fprintln(w) + fmt.Fprintln(w, base.Render("Or set the DATAROBOT_ENDPOINT and DATAROBOT_API_TOKEN environment variables")) + fmt.Fprintln(w, base.Render("(from Developer Tools) to authenticate without the browser.")) +} + // Close shuts the callback server down. It is safe to call more than once. func (f *BrowserFlow) Close() error { f.closeOnce.Do(func() { @@ -213,6 +233,9 @@ type LoginOptions struct { // NoBrowser skips launching a browser and shows the link instead. Useful over // SSH or anywhere the CLI cannot reach a usable browser. NoBrowser bool + + // Timeout overrides DefaultLoginTimeout for the callback wait. Zero uses the default. + Timeout time.Duration } // RunBrowserLogin opens the browser, tells the user what is happening, and blocks @@ -252,6 +275,10 @@ func RunBrowserLoginWith(ctx context.Context, datarobotHost string, opts LoginOp // Split out from RunBrowserLoginWith so tests can drive a flow on an ephemeral port // instead of competing for the fixed production one. func runLoginWithFlow(ctx context.Context, flow *BrowserFlow, opts LoginOptions) (string, error) { + if opts.Timeout > 0 { + flow.timeout = opts.Timeout + } + // The browser state drives the wording: when no browser opened, the link stops // being a footnote and becomes the primary instruction. state := BrowserSkipped diff --git a/internal/auth/browserflow_test.go b/internal/auth/browserflow_test.go index e4cbc89ec..c65c4c80b 100644 --- a/internal/auth/browserflow_test.go +++ b/internal/auth/browserflow_test.go @@ -15,6 +15,7 @@ package auth import ( + "bytes" "context" "io" "net/http" @@ -298,11 +299,41 @@ func TestBrowserFlow_WaitTimesOut(t *testing.T) { _, err := flow.Wait(context.Background()) require.Error(t, err) - require.ErrorIs(t, err, context.DeadlineExceeded) + require.ErrorIs(t, err, ErrLoginTimedOut) assert.NotErrorIs(t, err, ErrLoginInterrupted, "a timeout must be distinguishable from a user interrupt") } +func TestFprintLoginTimeoutHelp(t *testing.T) { + var buf bytes.Buffer + + FprintLoginTimeoutHelp(&buf) + + out := buf.String() + assert.Contains(t, out, "dr auth login", "the retry command is the primary recovery step") + assert.Contains(t, out, "DATAROBOT_ENDPOINT", "the browserless path names both env vars") + assert.Contains(t, out, "DATAROBOT_API_TOKEN") +} + +func TestRunLoginWithFlow_HonorsTimeoutOption(t *testing.T) { + // A short --timeout must shorten the wait, not sit on DefaultLoginTimeout. + flow := newTestFlow(t) + + start := time.Now() + + var err error + + captureStdoutStderr(t, func() { + _, err = runLoginWithFlow(context.Background(), flow, LoginOptions{ + NoBrowser: true, + Timeout: 50 * time.Millisecond, + }) + }) + + require.ErrorIs(t, err, ErrLoginTimedOut) + assert.Less(t, time.Since(start), 2*time.Second, "the option must override DefaultLoginTimeout") +} + func TestBrowserFlow_ExtraCallbacksDoNotBlockHandlers(t *testing.T) { // The key channel is buffered for exactly one key. Once Wait has returned and // nobody is draining it, further callbacks must still be answered and dropped