[CFX-6319] Actionable recovery path and --timeout on a login timeout - #915
Open
chasdr wants to merge 9 commits into
Open
[CFX-6319] Actionable recovery path and --timeout on a login timeout#915chasdr wants to merge 9 commits into
chasdr wants to merge 9 commits into
Conversation
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.
|
🎫 Jira: |
…ound (testifylint)
- 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.
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).
This comment was marked as resolved.
This comment was marked as resolved.
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.
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.
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.
… 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.
Contributor
Author
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 1d8447d. Configure here.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved validation-order, shell-guidance, endpoint-documentation, and comment-completion findings remain.
Review details
Suppressed comments (4)
cmd/auth/login/cmd.go:89
- This validation runs only after
SetURLToConfig,GetBaseURLOrAsk, andconfig.GetAPIKey(context.Background()). With a stored token or an unreachable configured endpoint, a negative duration can trigger config/API work (or report that error) before the invalid flag is rejected. Read and validate--timeoutat the start ofRunE, before any URL or credential work.
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
}
cmd/auth/login/cmd.go:98
- This newly added comment ends with the incomplete fragment “the help block is.” Please complete the sentence so the rationale is readable.
// The bare timeout error is a Go string with no next step; the help block is.
docs/commands/auth.md:117
- These instructions omit the API endpoint format. Environment credential verification uses the value verbatim and appends
/version/, whiledr auth exportemits the canonical/api/v2endpoint; using the base URL shown in the PR's recovery example will probe the wrong path. Include/api/v2in the example or direct users to the canonical export output.
Or set the DATAROBOT_ENDPOINT and DATAROBOT_API_TOKEN environment variables
(from Developer Tools) to authenticate without the browser.
internal/auth/browserflow.go:179
- On the timeout path this only names the two variables; it does not provide the shell-specific copy/paste statements promised by the PR's recovery output (including PowerShell/Windows syntax). The existing auth export guidance already handles Bash, PowerShell, and cmd forms, so users—especially on Windows—still have to invent the recovery command here. Add shell-aware examples or a shared formatter.
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."))
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
Code OwnershipCli Maintainers
Review requested from the teams above. Labels will be removed automatically upon approval. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When
dr auth loginnever gets a browser callback, it sat out the 5-minute timeout and printed a bare Go error with no next step. The usual cause is a recoverable browser-side sign-in error that clears on a retry, so the user was left stuck. This adds an actionable recovery block on timeout, plus a--timeoutflag to shorten or extend the wait behind a slow identity provider.Notes for review
The wait now wraps a new
ErrLoginTimedOutsentinel instead ofcontext.DeadlineExceeded. Both callers,dr auth loginand the implicit login inEnsureAuthenticated, detect it and print the help to stderr, so--output-format jsonstill leaves stdout clean. Only a timeout prints the help.The 5-minute hang itself traces to a cross-team OIDC issue (PLT-23376); this is the CLI-side half, useful either way.
Output
Before, the whole message on timeout:
After (
--timeout 2sagainst a dead endpoint, help to stderr):Technical Changes
ErrLoginTimedOutsentinel,FprintLoginTimeoutHelp,LoginOptions.Timeoutapplied inrunLoginWithFlow.EnsureAuthenticatedprints the help on timeout.--timeoutflag; on timeout prints the help and exits quiet, not the raw error.--timeoutin the auth command and development docs.--timeouthonored, the implicit path prints help, the flag contract.Breakdown
Note
Low Risk
Changes are limited to login wait UX, error messaging, and an optional timeout flag; credential storage and callback security behavior are unchanged.
Overview
Browser login no longer ends with a raw Go deadline error when the callback never arrives.
BrowserFlow.Waitnow returnsErrLoginTimedOut, anddr auth login,EnsureAuthenticated, and the setup wizard show recovery guidance (retrydr auth loginor useDATAROBOT_ENDPOINT/DATAROBOT_API_TOKEN) viaFprintLoginTimeoutHelpor a dedicated wizard message.dr auth loginadds--timeout(default still 5m when unset) with rejection of negative values; timeouts exit withcli.ErrSilentafter printing help on stderr.LoginOptions.Timeoutapplies the override inrunLoginWithFlow. The setuperrMsgtype gainsUnwrapsoerrors.Iscan detect timeouts through bubbletea. Docs and tests cover the flag, help output, and timeout paths.Reviewed by Cursor Bugbot for commit 1d8447d. Configure here.