Repository navigation
Reject an invalid host in auth login and auth token - #7195
maksim-zuev-db wants to merge 22 commits into
Conversation
Integration test reportCommit: 1ba63e7
7 interesting tests: 5 FAIL, 2 flaky
Top 3 slowest tests (at least 2 minutes):
|
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.
49c8f84 to
f80c59f
Compare
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.
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>
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>
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>
The glyph change affects every prompt that uses Validate, including the databricks configure host prompt, so it does not belong in this PR. The baseline tests added with it pass with the old glyph and stay. Co-authored-by: Isaac <no-reply@databricks.com>
ExtractHostQueryParams also handles ?w=, ?workspace_id= and ?account_id=, not just ?o= and ?a=. Co-authored-by: Isaac <no-reply@databricks.com>
A "/" host made the subtest name end in "//", which is awkward to target from the command line. TestLoginSkipWorkspaceHostEmptyAfterNormalization had landed between the two positional argument tests. Co-authored-by: Isaac <no-reply@databricks.com>
Only the host prompt trimmed spaces, so --host " ", a positional argument or a profile host of only spaces passed the empty host check and failed later with "host must start with 'https://'". A leading space also made url.Parse fail, so ?o= never ended up in WorkspaceID. promptForHost still trims its result because the profile picker passes the host to getProfileName before it is normalized. Co-authored-by: Isaac <no-reply@databricks.com>
The prompt only rejected blank input, so "/" or "?o=123" passed and then failed in setHost. In auth login, the "Enter a host" picker path first asked for a profile name and then failed, and in auth token the command exited instead of asking again. Running normalizeLoginHost in Validate keeps the prompt open with the error under it. Co-authored-by: Isaac <no-reply@databricks.com>
The invalid host error is user-visible output of both commands, and the auth token path was not exercised end to end. Co-authored-by: Isaac <no-reply@databricks.com>
50e1992 to
02bfdf4
Compare
|
Commit: Regression test reportSelected 2 changed tests (limit=20, 0 not selected) 2 tests: regression=2 unreleased=0 coverage=0 failing=0
TestAccept/cmd/auth/login/invalid-host: branch ✅ | main (3aa5c96) ❌ | latest (v1.23.0) ❌main (3aa5c96): latest (v1.23.0): TestAccept/cmd/auth/token/invalid-host: branch ✅ | main (3aa5c96) ❌ | latest (v1.23.0) ❌main (3aa5c96): latest (v1.23.0): same output as main (3aa5c96) |
TestPromptBaseline_ValidateEnterBeforeEdit and TestPromptBaseline_ValidateValidBeforeEdit were written for the blank prompt glyph, which was reverted. They only pin unchanged libs/cmdio behavior, and the cmd/auth goldens cover the host prompt. Co-authored-by: Isaac <no-reply@databricks.com>
normalizeLoginHost only rejected a host that was empty after removing the trailing slash and the query params, so "https://" became "https:/" and "https://?o=123" became "https:". The SDK reads those as a host named "https", so login tried to connect to it and auth token reported missing credentials. The check now parses the host, adding https:// to a host without an http(s) scheme as the SDK does, and fails if there is no hostname. A host that does not parse, e.g. with spaces, still fails later as before. Co-authored-by: Isaac <no-reply@databricks.com>
setHost normalizes the host after the keyring probe and the profile name prompt. With --host / and no --profile, login first asked for a profile name and only then failed with invalid host "/". Checking a host from --host or the positional argument right after the positional argument is resolved fails before both. Co-authored-by: Isaac <no-reply@databricks.com>
A host that url.Parse rejects, e.g. "my workspace.test", passed the hostname check and failed later with an SDK parse error. It now fails with invalid host, like a host without a hostname. TestSetHost used "val from --host" and "val from [HOST]" as hosts, which are not URLs, so they are now real test hosts. Co-authored-by: Isaac <no-reply@databricks.com>
hasHostname retried any host that failed to parse with "https://" in front, so "https://my workspace.test" or "https://x.test:abc" was read as a host named "https" and passed the check. A host with "://" is now not retried. Co-authored-by: Isaac <no-reply@databricks.com>
The early host check in auth login rejects the host before the --skip-workspace block normalizes it, so the test checks that an invalid host is rejected, not that it is empty after normalization. Co-authored-by: Isaac <no-reply@databricks.com>
Check for an http(s) scheme prefix instead of parsing the host twice. Co-authored-by: Isaac <no-reply@databricks.com>
Changes
databricks auth loginanddatabricks auth tokenreject a host that is not a URL with a hostname,e.g.
" ",/,?o=123,https://ormy workspace.test, withinvalid host "<input>".normalizeLoginHost, used bysetHostand the--skip-workspaceblock, so it covers--host, the positional argument, profile hosts and the prompt. A host without anhttp(s)scheme ischecked as an
https://host, like the SDK does. It replaceserrDeviceCodeNeedsHostfrom Prompt for the host instead of failing when device code login has none #7179.auth loginchecks--hostand the positional argument before the keyring probe and the profile name prompt.Behavior changes:
auth login --skip-workspace --host '?a=123'fails. On main this started discovery login.auth login --host https://fails. On main login tried to connect to a host namedhttps." https://x.test/?o=1 "works and keeps the workspace ID.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
TestSetHost,TestPromptForHostRejectsEmptyHost,TestPromptForHostRejectsHostEmptyAfterNormalization,TestLoginDeviceCodeHostEmptyAfterNormalization,TestLoginSkipWorkspaceHostEmptyAfterNormalization,TestLoginRejectsInvalidHostBeforeProfileNamePrompt.cmd/auth/login/invalid-hostandcmd/auth/token/invalid-host.This pull request and its description were written by Isaac.