Repository navigation
Reject existing profile names after "Create a new profile" - #7196
maksim-zuev-db wants to merge 14 commits into
Conversation
After "Create a new profile", the profile name prompt suggests DEFAULT, and pressing Enter logged in to an existing DEFAULT profile and reused its host. For device code login that profile may be an account profile the picker hid, which then failed with an error about a profile the user did not pick. The name prompt now rejects the names of all existing profiles in this case. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
The name prompt suggested DEFAULT even when a DEFAULT profile already existed, and then rejected it on Enter. A taken suggestion is now left out, so the prompt asks for a name without a default and rejects an empty answer. Co-authored-by: Isaac <no-reply@databricks.com>
auth token offers the same "Create a new profile" picker entry, and its name prompt could still log in to an existing profile. It now rejects existing profile names too. The error no longer tells the user to select the profile from the list, because device code login hides some profiles from the picker. auth login now loads the profiles once and filters the picker list from them. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
Integration test reportCommit: 6f63417
Top 3 slowest tests (at least 2 minutes):
|
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
Nice fix. A few things below.
The main one is that Validate checks the raw input while RunPrompt returns it with trailing \r\n stripped. So a pasted name with a trailing newline gets past the taken-name check, and a pasted blank line gets past the "required" check. I think trimming at the top of Validate fixes both (details inline).
One coverage gap that doesn't fit on a single line: there's no test for "Create a new profile" in the regular browser auth login flow, which is where the original bug was. The promptForProfile unit tests and the device-code/auth token termtests wouldn't catch it if the takenProfileNames = allProfiles.Names() wiring broke for that path. A termtest similar to TestResolveNoArgsTokenCreateNewProfileRejectsTakenName would cover it.
| profileName = "DEFAULT" | ||
| } | ||
| profileName, err = promptForProfile(ctx, profileName) | ||
| profileName, err = promptForProfile(ctx, profileName, takenProfileNames) |
There was a problem hiding this comment.
Not sure if this is in scope, but: if the user picks "Enter a host URL" and leaves the host empty, takenProfileNames is still nil here. The prompt then suggests [DEFAULT], and pressing Enter loads the existing DEFAULT profile and reuses its host. The device-code path guards the empty host (errDeviceCodeNeedsHost), the browser path doesn't.
The prompt validated the raw buffer, but RunPrompt trims the trailing newline only after validation. A pasted "taken\n" passed the taken-name check, and a pasted blank line or spaces passed the required check. Co-authored-by: Isaac <no-reply@databricks.com>
SaveToProfile rejects it, but only after the user has gone through the OAuth flow. Co-authored-by: Isaac <no-reply@databricks.com>
LoadProfiles skips sections without a host, so a name like [foo] with only cluster_id was accepted as a new profile, and SaveToProfile merged the login into that section, keeping its old keys. Read the taken names from the config file sections instead. Co-authored-by: Isaac <no-reply@databricks.com>
The taken profile names now come from the config file sections, so login no longer needs all profiles to filter the picker list from. Co-authored-by: Isaac <no-reply@databricks.com>
The test only checked that an empty answer is rejected, so it would still pass if the hidden DEFAULT profile name were only dropped from the suggestion. Co-authored-by: Isaac <no-reply@databricks.com>
… picker test env.Get falls back to the OS environment, so with either one exported resolveNoArgsToken returned before the picker and the test timed out. Co-authored-by: Isaac <no-reply@databricks.com>
The profile name prompt rejects the names of all existing sections, so runInlineLogin always creates a new profile. Looking up the existing profile, preserving its scopes, resources and client ID, and clearing its keys could no longer do anything. Co-authored-by: Isaac <no-reply@databricks.com>
This is the flow of the original bug. The other tests cover the prompt itself, device code login and auth token, but not this wiring. Co-authored-by: Isaac <no-reply@databricks.com>
TestLoginCreateNewProfileRejectsTakenName copied the setup of startDeviceCodeLogin. Move that setup into startLogin, which startDeviceCodeLogin now calls. Co-authored-by: Isaac <no-reply@databricks.com>
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
Looks good to me, and all my earlier comments are addressed. A few non-blocking things inline. Most are bugs that already exist on main but get past the new check, so follow-ups are fine.
One more that has no changed line to anchor on: in runInlineLogin (token.go L461) the timeout ctx is created after NewPersistentAuth, and Challenge uses the ctx stored at construction, so defaultTimeout never applies. Closing the browser tab leaves auth token waiting until Ctrl+C. Moving those two lines above NewPersistentAuth, as login.go does, fixes it. Unrelated to this change, so a follow-up is fine.
| func SaveToProfile(ctx context.Context, cfg *config.Config, clearKeys ...string) error { | ||
| if cfg.Profile == databricksSettingsSection { | ||
| return fmt.Errorf("profile name %q is reserved for internal use", databricksSettingsSection) | ||
| if err := ValidateProfileName(cfg.Profile); err != nil { |
There was a problem hiding this comment.
[nice to have] Further down (L494), section.Key(attr.Name) falls back to the parent section's key when the section doesn't have it, because go-ini treats team.prod as a child of [team]. So with [team] holding host and token, typing team.prod after "Create a new profile" passes the taken-name check. After OAuth, the save rewrites [team]'s host and auth_type, keeps its token, and leaves an empty [team.prod].
Using section.NewKey(attr.Name, attr.GetString(cfg)) only looks at the section's own keys, which fixes it (L312 in SaveResourcesToProfile has the same pattern). Fine as a follow-up since it predates this PR.
| Label: "Databricks profile name [" + defaultValue + "]", | ||
| Label: label, | ||
| Validate: func(input string) error { | ||
| input = strings.TrimSpace(input) |
There was a problem hiding this comment.
[nice to have] TrimSpace only trims the ends, so a bracketed paste of foo\nbar passes Validate. go-ini writes the section header unescaped, and every later config load fails with unclosed section: [foo. This also happens on main, but since ValidateProfileName now exists, rejecting \r and \n there (maybe [ and ] too) would close it.
| return err | ||
| } | ||
| if slices.Contains(takenNames, input) { | ||
| return fmt.Errorf("profile %s already exists. Choose a different name", input) |
There was a problem hiding this comment.
[nit] A [foo] with only cluster_id is hidden from the picker and now rejected here, and the message doesn't say how to log in to it. Something like profile foo already exists. Choose a different name, or run 'databricks auth login --profile foo' to update it would help.
| Scopes: scopesList, | ||
| ClientID: u2mClientIDFromProfile(existingProfile), | ||
| }, clearKeys...) | ||
| }) |
There was a problem hiding this comment.
[nit] The taken names are read before an OAuth wait that can take minutes. If a section with that name appears in the meantime, this save merges into it and leaves token/client_secret next to auth_type = databricks-cli. Passing oauthLoginClearKeys() here costs nothing on a new section and matches both saves in login.go.
Changes
After "Create a new profile" in the
databricks auth loginanddatabricks auth tokenpickers, the profile name prompt rejects the name of any existing config section withprofile X already exists. Choose a different name. This includes the profiles the device code picker hides and sections without ahost. A takenDEFAULTis no longer suggested: the prompt then has no default and requires a name.All profile name prompts now also trim spaces and pasted newlines, and reject the reserved
__settings__name before the OAuth flow rather than after it.The
auth tokeninline login can no longer target an existing profile, so its code for keeping that profile's scopes, client ID and resources is removed.Why
Follow-up to a review comment on #7179. With no host known yet, the prompt suggested
DEFAULT, and pressing Enter logged in to the existingDEFAULTprofile and reused its host. With--use-device-code, if that was an account profile the picker hid, login failed with--use-device-code is only supported for workspace hosts.A generated name like
DEFAULT-1is not offered instead: the host is not known yet, so the name would not say which workspace the profile is for.Tests
TestPromptForProfile*cover the prompt: free and taken defaults, and empty, taken, reserved and pasted names.TestLoginCreateNewProfileRejectsTakenName,TestLoginDeviceCodeCreateNewProfileRejectsHiddenProfileNameandTestResolveNoArgsTokenCreateNewProfileRejectsTakenNamecover the browser login, device code login andauth tokenflows. Like the other picker tests, they are skipped on Windows.TestGetSectionNamescovers sections without a host and empty sections.cmd/authunit tests andTestAccept/cmd/authpass.This pull request and its description were written by Isaac.