Repository navigation
Prompt for the host instead of failing when device code login has none - #7179
maksim-zuev-db wants to merge 5 commits into
Conversation
Integration test reportCommit: ffcd8b0
Top 13 slowest tests (at least 2 minutes):
|
simonfaltum
left a comment
There was a problem hiding this comment.
Thanks for picking this up! Left a few comments inline. The main ones:
- The picker still lists profiles that device code login always rejects (L241).
- The new test touches the real
~/.databricks/token-cache.json(L225). - I think skipping discovery for device code would fix this more generally and remove the need for the picker special case (L245).
The rest are small suggestions.
One more that can't go inline since the file isn't in this PR: step 1 in docs/device-code-login.md says the host comes from an argument, --host, or an existing profile. It doesn't mention the picker, so someone running databricks auth login --use-device-code with no arguments won't find out why "Create a new profile" is missing. Might be worth a line there.
| Label: label, | ||
| Default: currentDefault, |
There was a problem hiding this comment.
With --use-device-code the picker still lists every profile (MatchAllProfiles), including ones device code login always rejects:
- account hosts fail with
--use-device-code is only supported for workspace hosts(after host discovery, which can take up to 5s) - profiles without a host fail with
errDeviceCodeNeedsHost - profiles saved with
resourcesfail withcannot be used with profile ... scoped to resources
The default profile is also moved to the top with the cursor on it, so if the default is an account profile, pressing Enter goes straight into one of these errors.
I think we can improve the UX by marking those profiles, i.e. append ** to the name and add a line like "Profiles marked with ** are not supported with --use-device-code yet". Account hosts, missing hosts and resources can all be checked from the profile itself. Unified hosts are harder since that check uses the discovery URL, which isn't stored on the profile.
There was a problem hiding this comment.
I filtered out profiles which are guaranteed to fail now. It seems OK to me for now, as we expect all hosts to be supported before the feature becomes stable
f142176 to
7d59c27
Compare
Discovery login opens a browser on this machine and learns the workspace host only after sign-in. Device code login is meant for machines without a browser, but when no host was known it still went into the discovery branch and failed with "--use-device-code requires a workspace host". This broke "Create a new profile" in the login picker and "auth login --use-device-code --profile <new-profile>". Skip discovery for device code login so setHost resolves the host. It prompts for it when interactive and asks for --host otherwise. Co-authored-by: Isaac <no-reply@databricks.com>
7d59c27 to
12f1366
Compare
|
Commit: Regression test reportSelected 1 changed tests (limit=20, 0 not selected) 1 tests: regression=1 unreleased=0 coverage=0 failing=0
TestAccept/cmd/auth/login/device-code/unsupported-host: branch ✅ | main (c6bb214) ❌ | latest (v1.22.1) ❌main (c6bb214): latest (v1.22.1): |
On Windows, bubbletea cannot cancel its read from the test's stdin pipe, so after the profile picker quits its reader swallows the input typed for the profile name prompt, and the test timed out waiting for the host prompt. Passing --profile makes the host prompt the only prompt while still covering the skipped discovery branch. Co-authored-by: Isaac <no-reply@databricks.com>
With --use-device-code, the login picker leaves out profiles on a classic account host and profiles saved with resources, which device code login always rejects. Profiles on unified hosts are still listed, because the discovery URL that identifies them is not saved on the profile. isDeviceCodeSupportedForHost replaces validateDeviceCodeHost so the picker and the check after setHost share the host check. Co-authored-by: Isaac <no-reply@databricks.com>
Share the setup of the two termtest tests in startDeviceCodeLogin. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
simonfaltum
left a comment
There was a problem hiding this comment.
Thanks, the discovery change looks good and the earlier comments are all covered. One issue with an empty host answer below, plus an optional suggestion.
| } | ||
| // an account ID for account and unified hosts. A workspace without | ||
| // device code login enabled is rejected later by DeviceCodeChallenge. | ||
| if useDeviceCode && !isDeviceCodeSupportedForHost(authArguments.Host, authArguments.DiscoveryURL) { |
There was a problem hiding this comment.
With discovery skipped, setHost now prompts for the host, but promptForHost accepts an empty answer. If the user presses Enter at "Databricks host", Host stays "" and isDeviceCodeSupportedForHost("", "") returns true, so login continues:
ToOAuthArgumentresolves the config with theConfigAttributesloader, which reads env vars. WithDATABRICKS_HOSTset, login runs against that host (this workspace-only check never sees it), and the profile is saved without ahostkey.- Without
DATABRICKS_HOST, it fails later withhost must start with 'https://':.
Picker → "Enter a host URL manually" → empty input hits the same path, and the user gets the profile name prompt followed by a second host prompt. Before this PR, errDeviceCodeNeedsHost caught both cases.
I think the simplest fix is to reject empty input in promptForHost (cmdio.PromptOptions has Validate), or an empty host check here for device code.
| // (Create a new profile) and a manual host URL. Device code login skips | ||
| // discovery, so for it "Create a new profile" prompts for the host after | ||
| // the profile name. |
There was a problem hiding this comment.
Optional, and mostly unrelated to this PR: after "Create a new profile" no host is known yet, so the name prompt suggests DEFAULT, and pressing Enter loads the existing DEFAULT profile if there is one. If that's an account profile the picker just hid, the user gets --use-device-code is only supported for workspace hosts for a profile they didn't pick. If it's a workspace profile, login reuses its host instead of prompting as this comment describes. Browser login reuses DEFAULT the same way, so fine to leave for a follow-up.
Changes
databricks auth login --use-device-codenow prompts for the workspace host when none is known, instead of failing with--use-device-code requires a workspace host. Without a terminal it fails withplease specify a host using --host.Device code login skips the discovery branch, so
setHostresolves the host from the argument,--host, or the existing profile, and prompts for it otherwise.errDeviceCodeNeedsHostis removed because nothing returns it any more.With
--use-device-code, the login picker no longer lists profiles that device code login always rejects: profiles on a classic account host and profiles saved withresources. Profiles on unified hosts are still listed, because the discovery URL that identifies them is not saved on the profile.isDeviceCodeSupportedForHostreplacesvalidateDeviceCodeHost, so the picker and the check aftersetHostshare the host check.docs/device-code-login.mddescribes both changes.Why
Discovery login opens a browser on this machine through login.databricks.com and learns the workspace host only after sign-in. Device code login is meant for machines without a browser, but when no host was known it still went into the discovery branch and failed. This broke two paths:
databricks auth login --use-device-code --profile <new-profile>, which skips the pickerPicking a profile that device code login always rejects failed only after it was selected. If such a profile was the default, it was at the top of the picker with the cursor on it.
Tests
TestLoginDeviceCodeNewProfilePromptsForHostrunsauth login --use-device-code --profile newprofthrough thetermtestterminal emulator and checks that the CLI prompts for the host.TestLoginDeviceCodePickerHidesUnsupportedProfilesruns the picker throughtermtestwith a workspace, an account, and a resource-scoped profile, and checks that only the workspace profile is listed.TestMatchDeviceCodeProfilesandTestIsDeviceCodeSupportedForHostcover the profile filter and the host check.device-code/unsupported-hostacceptance test now expects the non-interactive host error for a new profile without a host.Acceptance tests can't cover the prompts and the picker, because they only appear in an interactive terminal.
No changelog fragment:
--use-device-codeis still hidden behindDATABRICKS_CLI_DEVICE_CODE.This pull request and its description were written by Isaac.