Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions cmd/auth/login/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package login
import (
"context"
"errors"
"os"
"strings"

"github.com/datarobot/cli/internal/auth"
Expand Down Expand Up @@ -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
Comment thread
chasdr marked this conversation as resolved.
}

log.Error(err)

return err
}

Expand Down Expand Up @@ -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
}
105 changes: 105 additions & 0 deletions cmd/auth/login/cmd_test.go
Original file line number Diff line number Diff line change
@@ -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")
}
10 changes: 9 additions & 1 deletion cmd/templates/setup/loginModel.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package setup

import (
"context"
"errors"
"fmt"
"strings"

Expand All @@ -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
Expand Down Expand Up @@ -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) {
Comment thread
Copilot marked this conversation as resolved.
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")
Comment thread
cursor[bot] marked this conversation as resolved.
} else if lm.loginMessage != "" {
sb.WriteString(lm.loginMessage)
} else if lm.err != nil {
fmt.Fprintf(&sb, "something went wrong: %s", lm.err)
Expand Down
15 changes: 15 additions & 0 deletions cmd/templates/setup/loginModel_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,35 @@ package setup

import (
"bytes"
"fmt"
"os"
"path/filepath"
"testing"
"time"

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))
}
Expand Down
23 changes: 22 additions & 1 deletion docs/commands/auth.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:**

Expand Down Expand Up @@ -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`

Expand Down
4 changes: 4 additions & 0 deletions docs/development/authentication.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
7 changes: 6 additions & 1 deletion internal/auth/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
21 changes: 21 additions & 0 deletions internal/auth/auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
29 changes: 28 additions & 1 deletion internal/auth/browserflow.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"context"
"errors"
"fmt"
"io"
"net"
"net/http"
"os"
Expand Down Expand Up @@ -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.
//
Expand Down Expand Up @@ -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")
Expand All @@ -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() {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Comment thread
chasdr marked this conversation as resolved.

// The browser state drives the wording: when no browser opened, the link stops
// being a footnote and becomes the primary instruction.
state := BrowserSkipped
Expand Down
Loading
Loading