From 368d1a83890eb8d0bf86656d34980817b0f737de Mon Sep 17 00:00:00 2001 From: chas Date: Fri, 11 Sep 2026 12:01:37 -0400 Subject: [PATCH 1/9] [CFX-6319] feat(auth): recovery help and --timeout on login timeout Wait wraps ErrLoginTimedOut on the deadline. Both callers (dr auth login and EnsureAuthenticated's implicit login) print FprintLoginTimeoutHelp to stderr instead of the bare Go timeout string: retry, since a sign-in error often clears on the second attempt, or set the DATAROBOT_ENDPOINT and DATAROBOT_API_TOKEN pair to skip the browser. --timeout overrides the 5m default for a slow identity provider where a cold SSO sign-in with MFA needs longer. --- cmd/auth/login/cmd.go | 15 +++++++++-- cmd/auth/login/cmd_test.go | 41 +++++++++++++++++++++++++++++++ internal/auth/auth.go | 7 +++++- internal/auth/auth_test.go | 21 ++++++++++++++++ internal/auth/browserflow.go | 30 +++++++++++++++++++++- internal/auth/browserflow_test.go | 33 ++++++++++++++++++++++++- 6 files changed, 142 insertions(+), 5 deletions(-) create mode 100644 cmd/auth/login/cmd_test.go diff --git a/cmd/auth/login/cmd.go b/cmd/auth/login/cmd.go index 156cde747..0523cd4fa 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" @@ -77,15 +78,24 @@ func RunE(cmd *cobra.Command, args []string) error { //nolint: cyclop viperx.Set(config.DataRobotAPIKey, "") noBrowser, _ := cmd.Flags().GetBool("no-browser") + timeout, _ := cmd.Flags().GetDuration("timeout") 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 +138,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..2f7d65e60 --- /dev/null +++ b/cmd/auth/login/cmd_test.go @@ -0,0 +1,41 @@ +// 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 ( + "testing" + "time" + + "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") +} 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..120741b46 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("timed out waiting for browser authorization") + // 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("timed out after %s waiting for browser authorization: %w", f.timeout, ErrLoginTimedOut) } log.Debug("Login context cancelled, exiting auth wait") @@ -159,6 +164,22 @@ 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 set the env pair to skip it. +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("To skip the browser, set both and try again:")) + fmt.Fprintln(w, info.Render(" export DATAROBOT_ENDPOINT=https://app.datarobot.com")) + fmt.Fprintln(w, info.Render(" export DATAROBOT_API_TOKEN=")) +} + // 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 +234,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 +276,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..2ea029424 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 needs 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.True(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 From accdc792bfdd7f3c85cca33d5ab504035720934f Mon Sep 17 00:00:00 2001 From: chas Date: Fri, 11 Sep 2026 12:01:37 -0400 Subject: [PATCH 2/9] [CFX-6319] docs(auth): login-timeout recovery path and --timeout --- docs/commands/auth.md | 25 ++++++++++++++++++++++++- docs/development/authentication.md | 4 ++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/docs/commands/auth.md b/docs/commands/auth.md index 18936a1b6..c300c4aca 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,29 @@ $ 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. A sign-in error in the browser often clears on a second +attempt, so the first step is to run `dr auth login` again; the alternative is to set the +`DATAROBOT_ENDPOINT` and `DATAROBOT_API_TOKEN` environment variables and skip the browser +entirely: + +```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 + +To skip the browser, set both and try again: + export DATAROBOT_ENDPOINT=https://app.datarobot.com + export DATAROBOT_API_TOKEN= +``` + +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. From 6afe063cf2d1881eca7e7dc0251cc3d9c7b3e4da Mon Sep 17 00:00:00 2001 From: chas Date: Fri, 11 Sep 2026 12:13:13 -0400 Subject: [PATCH 3/9] [CFX-6319] test(auth): assert.Less over assert.True for the timeout bound (testifylint) --- internal/auth/browserflow_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/auth/browserflow_test.go b/internal/auth/browserflow_test.go index 2ea029424..e60dbc7ac 100644 --- a/internal/auth/browserflow_test.go +++ b/internal/auth/browserflow_test.go @@ -331,7 +331,7 @@ func TestRunLoginWithFlow_HonorsTimeoutOption(t *testing.T) { }) require.ErrorIs(t, err, ErrLoginTimedOut) - assert.True(t, time.Since(start) < 2*time.Second, "the option must override DefaultLoginTimeout") + assert.Less(t, time.Since(start), 2*time.Second, "the option must override DefaultLoginTimeout") } func TestBrowserFlow_ExtraCallbacksDoNotBlockHandlers(t *testing.T) { From 6128bb00b6dead0bc9a5f469fee236892aa6c6cc Mon Sep 17 00:00:00 2001 From: chas Date: Fri, 11 Sep 2026 12:19:06 -0400 Subject: [PATCH 4/9] [CFX-6319] fix(auth): address bot review on the login-timeout help - FprintLoginTimeoutHelp renders the endpoint the user was logging into instead of a hardcoded app.datarobot.com, which pointed EU, JP, and on-prem users at the US instance. Reworded so the env-pair line no longer implies dr auth login itself skips the browser (it always opens it); the pair authenticates the CLI for the commands that run behind the gate. - Reject a negative --timeout instead of silently falling back to 5m, so a typo does not look like a hang. - Command-level tests for the timeout branch (help to stderr, cli.ErrSilent) and the negative-timeout rejection. --- cmd/auth/login/cmd.go | 10 ++++- cmd/auth/login/cmd_test.go | 64 +++++++++++++++++++++++++++++++ docs/commands/auth.md | 7 ++-- internal/auth/auth.go | 2 +- internal/auth/browserflow.go | 8 ++-- internal/auth/browserflow_test.go | 4 +- 6 files changed, 85 insertions(+), 10 deletions(-) diff --git a/cmd/auth/login/cmd.go b/cmd/auth/login/cmd.go index 0523cd4fa..63ea2e2cc 100644 --- a/cmd/auth/login/cmd.go +++ b/cmd/auth/login/cmd.go @@ -78,7 +78,15 @@ func RunE(cmd *cobra.Command, args []string) error { //nolint: cyclop viperx.Set(config.DataRobotAPIKey, "") 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, @@ -89,7 +97,7 @@ func RunE(cmd *cobra.Command, args []string) error { //nolint: cyclop // 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) + auth.FprintLoginTimeoutHelp(os.Stderr, datarobotHost) return cli.ErrSilent } diff --git a/cmd/auth/login/cmd_test.go b/cmd/auth/login/cmd_test.go index 2f7d65e60..cd0b1998e 100644 --- a/cmd/auth/login/cmd_test.go +++ b/cmd/auth/login/cmd_test.go @@ -15,9 +15,16 @@ 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" ) @@ -39,3 +46,60 @@ func TestCmd_HasTimeoutFlag(t *testing.T) { 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) + + assert.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/docs/commands/auth.md b/docs/commands/auth.md index c300c4aca..25c3df295 100644 --- a/docs/commands/auth.md +++ b/docs/commands/auth.md @@ -103,8 +103,9 @@ 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. A sign-in error in the browser often clears on a second attempt, so the first step is to run `dr auth login` again; the alternative is to set the -`DATAROBOT_ENDPOINT` and `DATAROBOT_API_TOKEN` environment variables and skip the browser -entirely: +`DATAROBOT_ENDPOINT` and `DATAROBOT_API_TOKEN` environment variables, which authenticate +the CLI without a browser. The endpoint printed is the host you were logging into, not a +fixed default: ```bash $ dr auth login @@ -114,7 +115,7 @@ 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 -To skip the browser, set both and try again: +Or authenticate without the browser by setting both: export DATAROBOT_ENDPOINT=https://app.datarobot.com export DATAROBOT_API_TOKEN= ``` diff --git a/internal/auth/auth.go b/internal/auth/auth.go index d8447e551..738cc2d0e 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -408,7 +408,7 @@ func EnsureAuthenticated(ctx context.Context) bool { //nolint: cyclop key, err := APIKeyCallbackFunc(ctx, datarobotHost) if err != nil { if errors.Is(err, ErrLoginTimedOut) { - FprintLoginTimeoutHelp(os.Stderr) + FprintLoginTimeoutHelp(os.Stderr, datarobotHost) } else { log.Error("Failed to retrieve API key.", "error", err) } diff --git a/internal/auth/browserflow.go b/internal/auth/browserflow.go index 120741b46..5b52fe754 100644 --- a/internal/auth/browserflow.go +++ b/internal/auth/browserflow.go @@ -165,8 +165,8 @@ 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 set the env pair to skip it. -func FprintLoginTimeoutHelp(w io.Writer) { +// retry (a sign-in error often clears next try), or authenticate via the env pair. +func FprintLoginTimeoutHelp(w io.Writer, datarobotHost string) { base, info := writerStyles(w) fmt.Fprintln(w, base.Render("❌ No authorization came back from the browser.")) @@ -175,8 +175,8 @@ func FprintLoginTimeoutHelp(w io.Writer) { 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("To skip the browser, set both and try again:")) - fmt.Fprintln(w, info.Render(" export DATAROBOT_ENDPOINT=https://app.datarobot.com")) + fmt.Fprintln(w, base.Render("Or authenticate without the browser by setting both:")) + fmt.Fprintln(w, info.Render(" export DATAROBOT_ENDPOINT="+datarobotHost)) fmt.Fprintln(w, info.Render(" export DATAROBOT_API_TOKEN=")) } diff --git a/internal/auth/browserflow_test.go b/internal/auth/browserflow_test.go index e60dbc7ac..3e7b54599 100644 --- a/internal/auth/browserflow_test.go +++ b/internal/auth/browserflow_test.go @@ -307,12 +307,14 @@ func TestBrowserFlow_WaitTimesOut(t *testing.T) { func TestFprintLoginTimeoutHelp(t *testing.T) { var buf bytes.Buffer - FprintLoginTimeoutHelp(&buf) + FprintLoginTimeoutHelp(&buf, "https://eu.datarobot.com") 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 needs both env vars") assert.Contains(t, out, "DATAROBOT_API_TOKEN") + assert.Contains(t, out, "https://eu.datarobot.com", + "the endpoint must be the host the user was logging into, not a hardcoded default") } func TestRunLoginWithFlow_HonorsTimeoutOption(t *testing.T) { From e63cbd466fd377ffcab214f8c51a02bb034c79b0 Mon Sep 17 00:00:00 2001 From: chas Date: Fri, 11 Sep 2026 12:31:26 -0400 Subject: [PATCH 5/9] [CFX-6319] fix(auth): PowerShell syntax in the timeout help on Windows The env-pair recovery lines printed bash `export`, which is invalid in PowerShell and cmd.exe. On Windows the help now prints the `$env:NAME="value"` form, matching how FprintUnsetTokenInstructions already handles the platform split. Also: require.ErrorIs before the follow-up assertion (testifylint). --- cmd/auth/login/cmd_test.go | 2 +- docs/commands/auth.md | 2 +- internal/auth/browserflow.go | 11 +++++++++-- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/cmd/auth/login/cmd_test.go b/cmd/auth/login/cmd_test.go index cd0b1998e..41709fc7c 100644 --- a/cmd/auth/login/cmd_test.go +++ b/cmd/auth/login/cmd_test.go @@ -82,7 +82,7 @@ func TestRunE_TimeoutPrintsHelpAndReturnsSilent(t *testing.T) { stderr, _ := io.ReadAll(rErr) _, _ = io.ReadAll(rOut) - assert.ErrorIs(t, runErr, cli.ErrSilent, "a timeout returns the silent sentinel, not the raw error") + 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") } diff --git a/docs/commands/auth.md b/docs/commands/auth.md index 25c3df295..6cf408f4c 100644 --- a/docs/commands/auth.md +++ b/docs/commands/auth.md @@ -105,7 +105,7 @@ steps instead of a bare error. A sign-in error in the browser often clears on a attempt, so the first step is to run `dr auth login` again; the alternative is to set the `DATAROBOT_ENDPOINT` and `DATAROBOT_API_TOKEN` environment variables, which authenticate the CLI without a browser. The endpoint printed is the host you were logging into, not a -fixed default: +fixed default, and on Windows the CLI prints the PowerShell `$env:` form instead of `export`: ```bash $ dr auth login diff --git a/internal/auth/browserflow.go b/internal/auth/browserflow.go index 5b52fe754..af0145f6c 100644 --- a/internal/auth/browserflow.go +++ b/internal/auth/browserflow.go @@ -22,6 +22,7 @@ import ( "net" "net/http" "os" + "runtime" "sync" "time" @@ -176,8 +177,14 @@ func FprintLoginTimeoutHelp(w io.Writer, datarobotHost string) { fmt.Fprintln(w, info.Render(" dr auth login")) fmt.Fprintln(w) fmt.Fprintln(w, base.Render("Or authenticate without the browser by setting both:")) - fmt.Fprintln(w, info.Render(" export DATAROBOT_ENDPOINT="+datarobotHost)) - fmt.Fprintln(w, info.Render(" export DATAROBOT_API_TOKEN=")) + + if runtime.GOOS == "windows" { + fmt.Fprintln(w, info.Render(` $env:DATAROBOT_ENDPOINT="`+datarobotHost+`"`)) + fmt.Fprintln(w, info.Render(` $env:DATAROBOT_API_TOKEN=""`)) + } else { + fmt.Fprintln(w, info.Render(" export DATAROBOT_ENDPOINT="+datarobotHost)) + fmt.Fprintln(w, info.Render(" export DATAROBOT_API_TOKEN=")) + } } // Close shuts the callback server down. It is safe to call more than once. From c64c24bb03308143fc30acecb3bdc617acc1c525 Mon Sep 17 00:00:00 2001 From: chas Date: Fri, 11 Sep 2026 12:42:36 -0400 Subject: [PATCH 6/9] [CFX-6319] fix(auth): friendly timeout in the templates setup login Wait has a third consumer, the template setup login model, which rendered the error verbatim. It now shows a retry/env-pair line on ErrLoginTimedOut, and that branch takes precedence so a timeout replaces the stale "browser opening" hint instead of hiding behind it. Also reworded the Wait error so it no longer repeats the sentinel text. --- cmd/templates/setup/loginModel.go | 6 +++++- internal/auth/browserflow.go | 4 ++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/cmd/templates/setup/loginModel.go b/cmd/templates/setup/loginModel.go index 411c11be6..9a7101b7c 100644 --- a/cmd/templates/setup/loginModel.go +++ b/cmd/templates/setup/loginModel.go @@ -16,6 +16,7 @@ package setup import ( "context" + "errors" "fmt" "strings" @@ -117,7 +118,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/internal/auth/browserflow.go b/internal/auth/browserflow.go index af0145f6c..1b80623aa 100644 --- a/internal/auth/browserflow.go +++ b/internal/auth/browserflow.go @@ -49,7 +49,7 @@ 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("timed out waiting for browser authorization") +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. @@ -156,7 +156,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, ErrLoginTimedOut) + return "", fmt.Errorf("no browser authorization within %s: %w", f.timeout, ErrLoginTimedOut) } log.Debug("Login context cancelled, exiting auth wait") From 0d53ed562f1f8d0b67bd5b08d7333b590216ad04 Mon Sep 17 00:00:00 2001 From: chas Date: Fri, 11 Sep 2026 13:08:16 -0400 Subject: [PATCH 7/9] [CFX-6319] fix(auth): errMsg.Unwrap so the setup timeout branch fires errMsg embedded error without an Unwrap method, so errors.Is against ErrLoginTimedOut was always false and the friendly timeout line added last round never showed. Add Unwrap plus a View test that pins the branch. --- cmd/templates/setup/loginModel.go | 4 ++++ cmd/templates/setup/loginModel_test.go | 15 +++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/cmd/templates/setup/loginModel.go b/cmd/templates/setup/loginModel.go index 9a7101b7c..e52625e32 100644 --- a/cmd/templates/setup/loginModel.go +++ b/cmd/templates/setup/loginModel.go @@ -37,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 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)) } From 18290aca0ccc87cccc68a6fe7c1202fd86788bcd Mon Sep 17 00:00:00 2001 From: chas Date: Fri, 11 Sep 2026 13:24:48 -0400 Subject: [PATCH 8/9] [CFX-6319] fix(auth): shell-quote the endpoint in the timeout help The env-pair recovery lines interpolated the user-configured endpoint into a copy-paste command unquoted, so a shell metacharacter in the value would execute when pasted. Single-quote it per shell, matching how dr auth export already quotes with posixQuote. --- docs/commands/auth.md | 2 +- internal/auth/browserflow.go | 17 +++++++++++++++-- internal/auth/browserflow_test.go | 4 ++-- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/docs/commands/auth.md b/docs/commands/auth.md index 6cf408f4c..e023a3ba3 100644 --- a/docs/commands/auth.md +++ b/docs/commands/auth.md @@ -116,7 +116,7 @@ The sign-in often completes on the second attempt: dr auth login Or authenticate without the browser by setting both: - export DATAROBOT_ENDPOINT=https://app.datarobot.com + export DATAROBOT_ENDPOINT='https://app.datarobot.com' export DATAROBOT_API_TOKEN= ``` diff --git a/internal/auth/browserflow.go b/internal/auth/browserflow.go index 1b80623aa..5689a644c 100644 --- a/internal/auth/browserflow.go +++ b/internal/auth/browserflow.go @@ -23,6 +23,7 @@ import ( "net/http" "os" "runtime" + "strings" "sync" "time" @@ -178,15 +179,27 @@ func FprintLoginTimeoutHelp(w io.Writer, datarobotHost string) { fmt.Fprintln(w) fmt.Fprintln(w, base.Render("Or authenticate without the browser by setting both:")) + // The endpoint is user-configured, so quote it into the copy-paste command the + // way dr auth export does, or a shell metacharacter in the value would execute. if runtime.GOOS == "windows" { - fmt.Fprintln(w, info.Render(` $env:DATAROBOT_ENDPOINT="`+datarobotHost+`"`)) + fmt.Fprintln(w, info.Render(" $env:DATAROBOT_ENDPOINT="+pwshQuote(datarobotHost))) fmt.Fprintln(w, info.Render(` $env:DATAROBOT_API_TOKEN=""`)) } else { - fmt.Fprintln(w, info.Render(" export DATAROBOT_ENDPOINT="+datarobotHost)) + fmt.Fprintln(w, info.Render(" export DATAROBOT_ENDPOINT="+posixQuote(datarobotHost))) fmt.Fprintln(w, info.Render(" export DATAROBOT_API_TOKEN=")) } } +// posixQuote and pwshQuote single-quote a value for the respective shell, so a +// metacharacter in a copy-paste command is a literal, not an instruction. +func posixQuote(value string) string { + return "'" + strings.ReplaceAll(value, "'", `'\''`) + "'" +} + +func pwshQuote(value string) string { + return "'" + strings.ReplaceAll(value, "'", "''") + "'" +} + // Close shuts the callback server down. It is safe to call more than once. func (f *BrowserFlow) Close() error { f.closeOnce.Do(func() { diff --git a/internal/auth/browserflow_test.go b/internal/auth/browserflow_test.go index 3e7b54599..4812d23b2 100644 --- a/internal/auth/browserflow_test.go +++ b/internal/auth/browserflow_test.go @@ -313,8 +313,8 @@ func TestFprintLoginTimeoutHelp(t *testing.T) { assert.Contains(t, out, "dr auth login", "the retry command is the primary recovery step") assert.Contains(t, out, "DATAROBOT_ENDPOINT", "the browserless path needs both env vars") assert.Contains(t, out, "DATAROBOT_API_TOKEN") - assert.Contains(t, out, "https://eu.datarobot.com", - "the endpoint must be the host the user was logging into, not a hardcoded default") + assert.Contains(t, out, "'https://eu.datarobot.com'", + "the real host is rendered, single-quoted so a shell metacharacter cannot execute") } func TestRunLoginWithFlow_HonorsTimeoutOption(t *testing.T) { From 1d8447d356c53dff2b5201922a5c1f2d417a8d4f Mon Sep 17 00:00:00 2001 From: chas Date: Fri, 11 Sep 2026 13:37:32 -0400 Subject: [PATCH 9/9] [CFX-6319] fix(auth): name the env vars in prose, drop the copy-paste command The env-pair recovery lines printed an exact export command that was wrong three ways: DATAROBOT_ENDPOINT dropped the /api/v2 path that GetBaseURL strips (so it would fail verification), the token placeholder's angle brackets broke a verbatim paste, and the value needed per-shell quoting. Naming the two variables in prose is correct on every shell and removes the GOOS branch, the quote helpers, and the endpoint argument. --- cmd/auth/login/cmd.go | 2 +- docs/commands/auth.md | 13 +++++-------- internal/auth/auth.go | 2 +- internal/auth/browserflow.go | 29 ++++------------------------- internal/auth/browserflow_test.go | 6 ++---- 5 files changed, 13 insertions(+), 39 deletions(-) diff --git a/cmd/auth/login/cmd.go b/cmd/auth/login/cmd.go index 63ea2e2cc..4bf1e4f15 100644 --- a/cmd/auth/login/cmd.go +++ b/cmd/auth/login/cmd.go @@ -97,7 +97,7 @@ func RunE(cmd *cobra.Command, args []string) error { //nolint: cyclop // 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, datarobotHost) + auth.FprintLoginTimeoutHelp(os.Stderr) return cli.ErrSilent } diff --git a/docs/commands/auth.md b/docs/commands/auth.md index e023a3ba3..d0ae792ad 100644 --- a/docs/commands/auth.md +++ b/docs/commands/auth.md @@ -101,11 +101,9 @@ If another `dr` process is already waiting on `localhost:51164`, the new one ask 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. A sign-in error in the browser often clears on a second -attempt, so the first step is to run `dr auth login` again; the alternative is to set the -`DATAROBOT_ENDPOINT` and `DATAROBOT_API_TOKEN` environment variables, which authenticate -the CLI without a browser. The endpoint printed is the host you were logging into, not a -fixed default, and on Windows the CLI prints the PowerShell `$env:` form instead of `export`: +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 @@ -115,9 +113,8 @@ 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 authenticate without the browser by setting both: - export DATAROBOT_ENDPOINT='https://app.datarobot.com' - export DATAROBOT_API_TOKEN= +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, diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 738cc2d0e..d8447e551 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -408,7 +408,7 @@ func EnsureAuthenticated(ctx context.Context) bool { //nolint: cyclop key, err := APIKeyCallbackFunc(ctx, datarobotHost) if err != nil { if errors.Is(err, ErrLoginTimedOut) { - FprintLoginTimeoutHelp(os.Stderr, datarobotHost) + FprintLoginTimeoutHelp(os.Stderr) } else { log.Error("Failed to retrieve API key.", "error", err) } diff --git a/internal/auth/browserflow.go b/internal/auth/browserflow.go index 5689a644c..186e1be45 100644 --- a/internal/auth/browserflow.go +++ b/internal/auth/browserflow.go @@ -22,8 +22,6 @@ import ( "net" "net/http" "os" - "runtime" - "strings" "sync" "time" @@ -167,8 +165,8 @@ 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 authenticate via the env pair. -func FprintLoginTimeoutHelp(w io.Writer, datarobotHost string) { +// 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.")) @@ -177,27 +175,8 @@ func FprintLoginTimeoutHelp(w io.Writer, datarobotHost string) { 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 authenticate without the browser by setting both:")) - - // The endpoint is user-configured, so quote it into the copy-paste command the - // way dr auth export does, or a shell metacharacter in the value would execute. - if runtime.GOOS == "windows" { - fmt.Fprintln(w, info.Render(" $env:DATAROBOT_ENDPOINT="+pwshQuote(datarobotHost))) - fmt.Fprintln(w, info.Render(` $env:DATAROBOT_API_TOKEN=""`)) - } else { - fmt.Fprintln(w, info.Render(" export DATAROBOT_ENDPOINT="+posixQuote(datarobotHost))) - fmt.Fprintln(w, info.Render(" export DATAROBOT_API_TOKEN=")) - } -} - -// posixQuote and pwshQuote single-quote a value for the respective shell, so a -// metacharacter in a copy-paste command is a literal, not an instruction. -func posixQuote(value string) string { - return "'" + strings.ReplaceAll(value, "'", `'\''`) + "'" -} - -func pwshQuote(value string) string { - return "'" + strings.ReplaceAll(value, "'", "''") + "'" + 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. diff --git a/internal/auth/browserflow_test.go b/internal/auth/browserflow_test.go index 4812d23b2..c65c4c80b 100644 --- a/internal/auth/browserflow_test.go +++ b/internal/auth/browserflow_test.go @@ -307,14 +307,12 @@ func TestBrowserFlow_WaitTimesOut(t *testing.T) { func TestFprintLoginTimeoutHelp(t *testing.T) { var buf bytes.Buffer - FprintLoginTimeoutHelp(&buf, "https://eu.datarobot.com") + 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 needs both env vars") + assert.Contains(t, out, "DATAROBOT_ENDPOINT", "the browserless path names both env vars") assert.Contains(t, out, "DATAROBOT_API_TOKEN") - assert.Contains(t, out, "'https://eu.datarobot.com'", - "the real host is rendered, single-quoted so a shell metacharacter cannot execute") } func TestRunLoginWithFlow_HonorsTimeoutOption(t *testing.T) {