Repository navigation
Reject an empty host at the login host prompt - #7195
maksim-zuev-db wants to merge 9 commits into
Conversation
Pressing Enter at the "Databricks host" prompt left the host empty, so login fell back to DATABRICKS_HOST or failed later with an unrelated error. The prompt now refuses an empty answer, which replaces the device code specific empty host checks. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
A host pasted with surrounding spaces failed later with an unrelated URL error, and a host of only spaces got past the empty host check. Co-authored-by: Isaac <no-reply@databricks.com>
Integration test reportCommit: 50e1992
Top 6 slowest tests (at least 2 minutes):
|
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
Left a few comments inline. The main one is in login.go: removing the post-setHost empty check lets device code login continue with an empty host when the input normalizes to "" (e.g. / or ?o=123).
One more outside the diff: promptForAccountID (cmd/auth/auth.go:67) has the same problem for the account ID. An empty answer leaves AccountID == "", ToOAuthArgument then picks up DATABRICKS_ACCOUNT_ID through ConfigAttributes (or OAuth fails later), and the profile can be saved without account_id. Probably fine as a follow-up.
| if !isDeviceCodeSupportedForHost(authArguments.Host, authArguments.DiscoveryURL) { | ||
| return errDeviceCodeWorkspaceOnly | ||
| } | ||
| if useDeviceCode && !isDeviceCodeSupportedForHost(authArguments.Host, authArguments.DiscoveryURL) { |
There was a problem hiding this comment.
This drops the Host == "" check that ran after setHost. The prompt validation doesn't fully cover it, because setHost can still turn a non-empty input into an empty host (TrimSuffix("/") and then ExtractHostQueryParams). So / or ?o=123 at the prompt, or --host /, ends up as Host == "" here.
With an empty host, isDeviceCodeSupportedForHost("", "") returns true, and ToOAuthArgument resolves the host from DATABRICKS_HOST via ConfigAttributes. If that's an accounts host, we get an account OAuth argument, skip the workspace-only check, and save a profile without a host key. On main this returned errDeviceCodeNeedsHost.
Repro:
DATABRICKS_CLI_DEVICE_CODE=true DATABRICKS_HOST=https://accounts.cloud.databricks.com \
databricks auth login --use-device-code --profile p --host /
I think we should keep an empty check after setHost (see my comment on promptForHost for where I'd put it).
There was a problem hiding this comment.
added a check inside setHost
| result, err := cmdio.RunPrompt(ctx, cmdio.PromptOptions{ | ||
| Label: "Databricks host", | ||
| Validate: func(input string) error { | ||
| if strings.TrimSpace(input) == "" { |
There was a problem hiding this comment.
The check runs on the raw input, and setHost still normalizes it afterwards. So / or ?o=123 passes here and becomes "". --host /, a positional /, or a profile with host = / never hit this prompt at all. Browser login then falls back to DATABRICKS_HOST (or fails with host must start with 'https://'), and auth token does the same through setHostAndAccountId (token.go:209, token.go:439).
I think a single check at the end of setHost, after the query param extraction, would cover every source:
if authArguments.Host == "" {
return errors.New("host is required")
}Side note: cmd/configure/host.go already has normalizeHost/validateHost for the configure prompt. It isn't a drop-in since login needs to keep ?o=/?a=, but it might be worth sharing some of it so the two host prompts don't drift.
There was a problem hiding this comment.
added a check at the end of setHost
sharing normalizeHost/validateHost require to many code changes for this PR, I'd postpone it
| return cmdio.RunPrompt(ctx, cmdio.PromptOptions{ | ||
| result, err := cmdio.RunPrompt(ctx, cmdio.PromptOptions{ | ||
| Label: "Databricks host", | ||
| Validate: func(input string) error { |
There was a problem hiding this comment.
This also changes the browser flow. On main, picking "Enter a host" in the profile picker and pressing Enter on an empty prompt left Host == "", so shouldUseDiscovery kicked in and login went through login.databricks.com. Now the prompt loops on "host is required" and Ctrl+C is the only way out (no way back to "Create a new profile").
Was that intended? If so, I think the PR description and changelog should mention it.
There was a problem hiding this comment.
Yes, I intended this change. I suppose discovery fallback is unexpected here, as it is a part of 'Enter host manually' flow
| Label: "Databricks host", | ||
| Validate: func(input string) error { | ||
| if strings.TrimSpace(input) == "" { | ||
| return errors.New("host is required") |
There was a problem hiding this comment.
Two small nits:
- The removed
errDeviceCodeNeedsHosttold users they could pass the host as an argument or with--host. Might be worth keeping that hint in this message. promptModel.glyph()runsValidateon every render, so the prompt now opens with a red ✗ before anything is typed (it used to be a green ✔). Minor, but the first frame reads like an error.
There was a problem hiding this comment.
- I'm not sure it is suitable here, as a user already entering the url in an interactive mode
- I changed it to show nothing until the first edit (unless input is valid)
setHost strips a trailing "/" and the query params, so "/" or "?o=123" became an empty host after the prompt check. Login and auth token then resolved the host from DATABRICKS_HOST or failed later. Checking in setHost covers the prompt, --host, the positional argument, and profile hosts, and keeps the hint from the removed errDeviceCodeNeedsHost. Co-authored-by: Isaac <no-reply@databricks.com>
WaitFor matched the error printed for the first empty answer, so the test passed even if a whitespace-only host was accepted. Golden steps compare the rendered screen after each Enter. Co-authored-by: Isaac <no-reply@databricks.com>
The prompt glyph ran Validate on every render, so a prompt that rejects empty input, such as the login host prompt, opened with a red cross before anything was typed. The glyph is now blank until the first edit or Enter, then shows a cross while the input is invalid and a checkmark once it is valid. Co-authored-by: Isaac <no-reply@databricks.com>
Mention auth token, and that an empty host after "Enter a host" no longer starts discovery login. Co-authored-by: Isaac <no-reply@databricks.com>
49c8f84 to
f80c59f
Compare
With --skip-workspace, login stripped the query params from the host before setHost ran, so a host like "?a=123" became empty and login went to discovery instead of failing. Both places now share normalizeHost, which does the empty host check. The error is now `invalid host "<input>"`. The previous hint to pass the host as an argument or with --host did not help, because the error only happens when a host was given. Co-authored-by: Isaac <no-reply@databricks.com>
cmd/configure and libs/databrickscfg already have a normalizeHost. Co-authored-by: Isaac <no-reply@databricks.com>
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
Thanks for the fixes, the first-round points look addressed. A few more comments inline, nothing blocking.
The glyph change was my suggestion, my bad. It changes every prompt that uses Validate, so I think it should be rolled back here (see the comment on prompt.go).
[nit] In the description, "auth login --skip-workspace --host '?a=123' fails instead of starting discovery login (already the case on main)" reads like a contradiction. On main the host is empty after the query params are extracted, so shouldUseDiscovery returns true and login goes on to discovery. "On main this started discovery login" would be clearer.
| return cmdio.RunPrompt(ctx, cmdio.PromptOptions{ | ||
| result, err := cmdio.RunPrompt(ctx, cmdio.PromptOptions{ | ||
| Label: "Databricks host", | ||
| Validate: func(input string) error { |
There was a problem hiding this comment.
[nice to have] Validate only rejects blank input, so / or ?o=123 passes here and then fails in setHost. In auth login, picking "Enter a host" and typing / first asks for a profile name (with [/] as the suggested default), then errors with invalid host "/" and you have to start over. In the auth token "Enter a host" path the command exits instead of asking again.
I think running the same normalization in Validate after the blank check would keep the prompt open with the error under it:
return normalizeLoginHost(&auth.AuthArguments{Host: strings.TrimSpace(input)})Checking for a missing hostname (like validateHost in cmd/configure/host.go) would also catch https:// and https://?o=123, which currently get through as https:/ and https:. An explicit --host / has the same problem: it's only rejected after the keyring probe and (without --profile) the profile name prompt.
| // host and moves ?o= and ?a= into WorkspaceID and AccountID unless they are set. | ||
| // It fails if nothing is left of the host, e.g. for "/" or "?o=123", because | ||
| // with an empty host login would use discovery or DATABRICKS_HOST instead. | ||
| func normalizeLoginHost(authArguments *auth.AuthArguments) error { |
There was a problem hiding this comment.
[nice to have] Only the prompt trims spaces. --host " ", a positional arg or a profile host with spaces gets past this check and still fails later with host must start with 'https://': , which is the error the PR description mentions. With --host " https://x.test/?o=1", url.Parse fails on the leading space, so ?o= never ends up in WorkspaceID.
I think trimming once at the top of normalizeLoginHost would cover every source, and then promptForHost wouldn't need to trim its return value.
| params := auth.ExtractHostQueryParams(authArguments.Host) | ||
| authArguments.Host = params.Host | ||
| if authArguments.Host == "" { | ||
| return fmt.Errorf("%w %q", errInvalidHost, rawHost) |
There was a problem hiding this comment.
[nice to have] invalid host "<input>" is user-visible output from both auth login and auth token, but it's only covered by unit tests, and nothing exercises the auth token path end to end. .agents/rules/testing.md asks for acceptance tests first for cmd/... output. This one doesn't need a terminal, so something like musterr $CLI auth login --host '?o=123' --profile test and musterr $CLI auth token --host '?o=123' under acceptance/cmd/auth/login/ and acceptance/cmd/auth/token/ should work.
| var errInvalidHost = errors.New("invalid host") | ||
|
|
||
| // normalizeLoginHost strips the trailing slash and the query params from the | ||
| // host and moves ?o= and ?a= into WorkspaceID and AccountID unless they are set. |
There was a problem hiding this comment.
[nit] The comment says it moves ?o= and ?a=, but ExtractHostQueryParams also handles ?w= (which the help text calls the preferred form), ?workspace_id= and ?account_id=. Something like "moves the workspace and account ID query params into ..." would stay accurate.
| if m.validate == nil || m.validate(m.value()) == nil { | ||
| return ansiBold + ansiGreen + glyphValid + ansiReset | ||
| } | ||
| if !m.edited && m.submitErr == nil { |
There was a problem hiding this comment.
Coming back on my earlier comment about the red ✗ on the first frame, my bad. This changes every prompt that uses Validate, including the databricks configure host prompt, so I think it's out of scope for this PR. Can we roll back the libs/cmdio/prompt.go change and the TestPromptBaseline_Validate/01-empty.golden update (and drop it from the description)? The two new baseline tests and the cmd/auth goldens should still pass with the old glyph. If we still want the blank glyph, it can go in its own PR.
|
|
||
| func TestLoginDeviceCodeHostEmptyAfterNormalization(t *testing.T) { | ||
| for _, host := range []string{"/", "?o=123"} { | ||
| t.Run(host, func(t *testing.T) { |
There was a problem hiding this comment.
[nit] With host == "/" the subtest name ends up as TestLoginDeviceCodeHostEmptyAfterNormalization//, which is awkward to target with -run. Named cases ({name: "slash", host: "/"}) would avoid that.
Small thing: TestLoginSkipWorkspaceHostEmptyAfterNormalization in login_test.go landed between the paired TestLoginRejectsPositionalArgWithHostFlag and ...WithProfileFlag tests.
Changes
databricks auth loginanddatabricks auth tokenno longer accept an empty host./,?o=123)fails with
invalid host "<input>". The check is innormalizeLoginHost, used bysetHostand the--skip-workspaceblock, so it covers--host, the positional argument, profile hosts and the prompt.It replaces the device code specific checks from Prompt for the host instead of failing when device code login has none #7179 (
errDeviceCodeNeedsHost).Validateshow a blank glyph until the first edit or Enter, then ✗ or ✔.Behavior changes:
auth login --skip-workspace --host '?a=123'fails instead of starting discovery login (already the case on main).Why
Follow-up to a review comment on #7179. An empty host made login and token resolve the host from
DATABRICKS_HOST, save a profile without ahostkey, or fail later withhost must start with 'https://':.Tests
TestPromptForHostRejectsEmptyHost,TestSetHost,TestLoginDeviceCodeHostEmptyAfterNormalization,TestLoginSkipWorkspaceHostEmptyAfterNormalization, and prompt baselines for the glyph states.This pull request and its description were written by Isaac.