diff --git a/.nextchanges/cli/login-new-profile-existing-name.md b/.nextchanges/cli/login-new-profile-existing-name.md new file mode 100644 index 00000000000..9575a716810 --- /dev/null +++ b/.nextchanges/cli/login-new-profile-existing-name.md @@ -0,0 +1 @@ +* `databricks auth login` and `databricks auth token` now reject the name of an existing profile after "Create a new profile" instead of logging in to that profile. ([#7196](https://github.com/databricks/cli/pull/7196)) diff --git a/cmd/auth/login.go b/cmd/auth/login.go index 5c9f322d53d..166eda36312 100644 --- a/cmd/auth/login.go +++ b/cmd/auth/login.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "runtime" + "slices" "strconv" "strings" "time" @@ -26,14 +27,40 @@ import ( "golang.org/x/oauth2" ) -func promptForProfile(ctx context.Context, defaultValue string) (string, error) { +// promptForProfile prompts for a profile name, rejecting the names in +// takenNames. A taken defaultValue is not suggested. +func promptForProfile(ctx context.Context, defaultValue string, takenNames []string) (string, error) { if !cmdio.IsPromptSupported(ctx) { return "", nil } + label := "Databricks profile name" + if slices.Contains(takenNames, defaultValue) { + defaultValue = "" + } else { + label += " [" + defaultValue + "]" + } result, err := cmdio.RunPrompt(ctx, cmdio.PromptOptions{ - Label: "Databricks profile name [" + defaultValue + "]", + Label: label, + Validate: func(input string) error { + input = strings.TrimSpace(input) + if input == "" { + input = defaultValue + } + if input == "" { + return errors.New("profile name is required") + } + // Fail here rather than after the OAuth flow, in SaveToProfile. + if err := databrickscfg.ValidateProfileName(input); err != nil { + return err + } + if slices.Contains(takenNames, input) { + return fmt.Errorf("profile %s already exists. Choose a different name, or rerun with --profile %s to use it", input, input) + } + return nil + }, }) + result = strings.TrimSpace(result) if result == "" { return defaultValue, err } @@ -227,7 +254,8 @@ a new profile is created. // two action entries so the user can choose between web-based discovery // (Create a new profile) and a manual host URL. Device code login skips // discovery, so its "Create a new profile" asks for the host after the - // profile name, unless a profile with that name already has a host. + // profile name. + var takenProfileNames []string if profileName == "" && authArguments.Host == "" && len(args) == 0 && cmdio.IsPromptSupported(ctx) { matchProfiles := profile.MatchAllProfiles if useDeviceCode { @@ -265,7 +293,14 @@ a new profile is created. } authArguments.Host = host case profilePickerCreateNew: - // Fall through to the profile name prompt below. + // Reject the names of existing profiles, including those the + // picker hid and sections without a host, so that the profile + // name prompt below cannot log in to an existing profile and + // reuse its host or keys. + takenProfileNames, err = databrickscfg.GetSectionNames(ctx, env.Get(ctx, "DATABRICKS_CONFIG_FILE")) + if err != nil { + return err + } } } @@ -293,7 +328,7 @@ a new profile is created. if profileName == "" { profileName = "DEFAULT" } - profileName, err = promptForProfile(ctx, profileName) + profileName, err = promptForProfile(ctx, profileName, takenProfileNames) if err != nil { return err } diff --git a/cmd/auth/login_device_code_test.go b/cmd/auth/login_device_code_test.go index d38d2543b7c..38cab796d9c 100644 --- a/cmd/auth/login_device_code_test.go +++ b/cmd/auth/login_device_code_test.go @@ -208,27 +208,34 @@ func TestIsDeviceCodeSupportedForHost(t *testing.T) { } } -// startDeviceCodeLogin runs "auth login --use-device-code" with args in a -// terminal emulator, using config as the contents of .databrickscfg. -func startDeviceCodeLogin(t *testing.T, config string, args ...string) *termtest.Term[struct{}] { +// startLogin runs "auth login" with args in a terminal emulator, using config +// as the contents of .databrickscfg. +func startLogin(t *testing.T, config string, args ...string) *termtest.Term[struct{}] { home := t.TempDir() t.Setenv("HOME", home) t.Setenv("USERPROFILE", home) t.Setenv("DATABRICKS_CONFIG_FILE", writeTempConfig(t, config)) t.Setenv("DATABRICKS_AUTH_STORAGE", "plaintext") - t.Setenv(deviceCodeEnvVar, "true") return termtest.Run(t, func(ctx context.Context) (struct{}, error) { cmd := newLoginCommand(&auth.AuthArguments{}) cmd.Flags().String("profile", "", "") cmd.SetContext(ctx) - cmd.SetArgs(append([]string{"--use-device-code"}, args...)) + // Cobra reads os.Args when the args are nil. + cmd.SetArgs(append([]string{}, args...)) cmd.SetOut(io.Discard) cmd.SetErr(io.Discard) return struct{}{}, cmd.Execute() }) } +// startDeviceCodeLogin runs "auth login --use-device-code" with args in a +// terminal emulator, using config as the contents of .databrickscfg. +func startDeviceCodeLogin(t *testing.T, config string, args ...string) *termtest.Term[struct{}] { + t.Setenv(deviceCodeEnvVar, "true") + return startLogin(t, config, append([]string{"--use-device-code"}, args...)...) +} + func TestLoginDeviceCodeNewProfilePromptsForHost(t *testing.T) { tm := startDeviceCodeLogin(t, "", "--profile", "newprof") tm.WaitFor("Databricks host") @@ -253,6 +260,29 @@ func TestLoginDeviceCodePickerEmptyHost(t *testing.T) { assert.ErrorIs(t, err, errDeviceCodeNeedsHost) } +func TestLoginDeviceCodeCreateNewProfileRejectsHiddenProfileName(t *testing.T) { + // See TestLoginDeviceCodePickerEmptyHost. + if runtime.GOOS == "windows" { + t.Skip("bubbletea cannot cancel reads on non-console input on Windows") + } + tm := startDeviceCodeLogin(t, `[DEFAULT] +host = https://accounts.cloud.databricks.test +account_id = acct-123 +`) + tm.WaitFor(profilePickerCreateNewLabel) + tm.Type(termtest.KeyEnter) + tm.WaitFor("Databricks profile name") + tm.Type(termtest.KeyEnter) + tm.WaitFor(">> profile name is required") + tm.Type("DEFAULT" + termtest.KeyEnter) + tm.WaitFor(">> profile DEFAULT already exists") + tm.Type(strings.Repeat(termtest.KeyBackspace, len("DEFAULT")) + "newprof" + termtest.KeyEnter) + tm.WaitFor("Databricks host") + tm.Type(termtest.KeyCtrlC) + _, err := tm.Result() + assert.EqualError(t, err, "^C") +} + func TestMatchDeviceCodeProfiles(t *testing.T) { tests := []struct { name string diff --git a/cmd/auth/login_test.go b/cmd/auth/login_test.go index 026a002a158..3bbf7249e45 100644 --- a/cmd/auth/login_test.go +++ b/cmd/auth/login_test.go @@ -10,6 +10,8 @@ import ( "net/http/httptest" "os" "path/filepath" + "runtime" + "strings" "sync" "testing" "time" @@ -18,6 +20,7 @@ import ( "github.com/databricks/cli/libs/auth/storage" "github.com/databricks/cli/libs/auth/u2m" "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/cli/libs/cmdio/cmdiotest/termtest" "github.com/databricks/cli/libs/databrickscfg/profile" "github.com/databricks/cli/libs/env" "github.com/databricks/cli/libs/log" @@ -1459,3 +1462,81 @@ func TestLoginRejectsPositionalArgWithProfileFlag(t *testing.T) { err := cmd.Execute() assert.ErrorContains(t, err, `argument "https://example.com" cannot be combined with --host or --profile`) } + +func TestPromptForProfileRejectsTakenName(t *testing.T) { + tm := termtest.Run(t, func(ctx context.Context) (string, error) { + return promptForProfile(ctx, "DEFAULT", []string{"DEFAULT", "taken"}) + }) + screen := tm.WaitFor("Databricks profile name") + assert.NotContains(t, screen, "[DEFAULT]") + tm.Type(termtest.KeyEnter) + tm.WaitFor(">> profile name is required") + tm.Type("taken" + termtest.KeyEnter) + tm.WaitFor(">> profile taken already exists. Choose a different name, or rerun with --profile taken to use it") + tm.Type(strings.Repeat(termtest.KeyBackspace, len("taken")) + "__settings__" + termtest.KeyEnter) + tm.WaitFor(`>> profile name "__settings__" is reserved for internal use`) + tm.Type(strings.Repeat(termtest.KeyBackspace, len("__settings__")) + "take" + termtest.KeyEnter) + name, err := tm.Result() + require.NoError(t, err) + assert.Equal(t, "take", name) +} + +func TestPromptForProfileSuggestsFreeDefault(t *testing.T) { + tm := termtest.Run(t, func(ctx context.Context) (string, error) { + return promptForProfile(ctx, "DEFAULT", []string{"taken"}) + }) + tm.WaitFor("Databricks profile name [DEFAULT]") + tm.Type(termtest.KeyEnter) + name, err := tm.Result() + require.NoError(t, err) + assert.Equal(t, "DEFAULT", name) +} + +func TestPromptForProfileTrimsPastedName(t *testing.T) { + tm := termtest.Run(t, func(ctx context.Context) (string, error) { + return promptForProfile(ctx, "DEFAULT", []string{"DEFAULT", "taken"}) + }) + tm.WaitFor("Databricks profile name") + // A bracketed paste arrives as a single input event, so the trailing + // newline lands in the buffer instead of submitting the prompt. + tm.Type("\x1b[200~taken\n\x1b[201~" + termtest.KeyEnter) + tm.WaitFor(">> profile taken already exists") + tm.Type(strings.Repeat(termtest.KeyBackspace, len("taken\n")) + " " + termtest.KeyEnter) + tm.WaitFor(">> profile name is required") + tm.Type(" new " + termtest.KeyEnter) + name, err := tm.Result() + require.NoError(t, err) + assert.Equal(t, "new", name) +} + +func TestPromptForProfileRejectsMultilinePaste(t *testing.T) { + tm := termtest.Run(t, func(ctx context.Context) (string, error) { + return promptForProfile(ctx, "DEFAULT", nil) + }) + tm.WaitFor("Databricks profile name [DEFAULT]") + tm.Type("\x1b[200~foo\nbar\x1b[201~" + termtest.KeyEnter) + tm.WaitFor(`>> profile name "foo\nbar" must not contain line breaks`) + tm.Type(strings.Repeat(termtest.KeyBackspace, len("foo\nbar")) + "[foo]" + termtest.KeyEnter) + name, err := tm.Result() + require.NoError(t, err) + assert.Equal(t, "[foo]", name) +} + +func TestLoginCreateNewProfileRejectsTakenName(t *testing.T) { + // See TestLoginDeviceCodePickerEmptyHost. + if runtime.GOOS == "windows" { + t.Skip("bubbletea cannot cancel reads on non-console input on Windows") + } + tm := startLogin(t, `[DEFAULT] +host = https://default.cloud.databricks.test +`) + tm.WaitFor(profilePickerCreateNewLabel) + tm.Type(termtest.KeyDown + termtest.KeyEnter) + screen := tm.WaitFor("Databricks profile name") + assert.NotContains(t, screen, "[DEFAULT]") + tm.Type("DEFAULT" + termtest.KeyEnter) + tm.WaitFor(">> profile DEFAULT already exists") + tm.Type(termtest.KeyCtrlC) + _, err := tm.Result() + assert.EqualError(t, err, "^C") +} diff --git a/cmd/auth/token.go b/cmd/auth/token.go index e28d71083a0..e7c004d7b4c 100644 --- a/cmd/auth/token.go +++ b/cmd/auth/token.go @@ -422,38 +422,28 @@ func resolveNoArgsToken(ctx context.Context, profiler profile.Profiler, authArgs // runInlineLogin runs a minimal interactive login flow: prompts for a profile // name and host, performs the OAuth challenge, saves the profile to -// .databrickscfg, and returns the new profile name and profile. +// .databrickscfg, and returns the new profile name and profile. It rejects +// the names of existing sections so that it cannot log in to an existing +// profile. func runInlineLogin(ctx context.Context, profiler profile.Profiler, tokenStore storage.Store, mode storage.StorageMode) (string, *profile.Profile, error) { - profileName, err := promptForProfile(ctx, "DEFAULT") + takenNames, err := databrickscfg.GetSectionNames(ctx, env.Get(ctx, "DATABRICKS_CONFIG_FILE")) if err != nil { return "", nil, err } - - existingProfile, err := loadProfileByName(ctx, profileName, profiler) + profileName, err := promptForProfile(ctx, "DEFAULT", takenNames) if err != nil { return "", nil, err } loginArgs := &auth.AuthArguments{} - err = setHostAndAccountId(ctx, existingProfile, loginArgs, nil) + err = setHostAndAccountId(ctx, nil, loginArgs, nil) if err != nil { return "", nil, err } loginArgs.Profile = profileName - // Preserve scopes from the existing profile so the inline login - // uses the same scopes the user previously configured. - var scopesList []string - if existingProfile != nil && existingProfile.Scopes != "" { - scopesList = splitScopes(existingProfile.Scopes) - } - - // Preserve RFC 8707 resource indicators from the existing profile so the - // inline login requests the same resources the user previously configured. - resourcesList := u2mResourcesFromProfile(existingProfile) - oauthArgument, err := loginArgs.ToOAuthArgument() if err != nil { return "", nil, err @@ -462,15 +452,6 @@ func runInlineLogin(ctx context.Context, profiler profile.Profiler, tokenStore s u2m.WithOAuthArgument(oauthArgument), u2m.WithBrowser(func(url string) error { return browser.Open(ctx, url) }), } - if clientID := u2mClientIDFromProfile(existingProfile); clientID != "" { - persistentAuthOpts = append(persistentAuthOpts, u2m.WithClientID(clientID)) - } - if len(scopesList) > 0 { - persistentAuthOpts = append(persistentAuthOpts, u2m.WithScopes(scopesList)) - } - if len(resourcesList) > 0 { - persistentAuthOpts = append(persistentAuthOpts, u2m.WithResources(resourcesList)) - } persistentAuth, err := u2m.NewPersistentAuth(ctx, persistentAuthOpts...) if err != nil { return "", nil, err @@ -485,6 +466,8 @@ func runInlineLogin(ctx context.Context, profiler profile.Profiler, tokenStore s return "", nil, err } + // The section may have been created during the OAuth wait, after the + // taken names were read. clearKeys := oauthLoginClearKeys() clearKeys = append(clearKeys, databrickscfg.ExperimentalIsUnifiedHostKey) @@ -495,18 +478,10 @@ func runInlineLogin(ctx context.Context, profiler profile.Profiler, tokenStore s AccountID: loginArgs.AccountID, WorkspaceID: loginArgs.WorkspaceID, ConfigFile: env.Get(ctx, "DATABRICKS_CONFIG_FILE"), - Scopes: scopesList, - ClientID: u2mClientIDFromProfile(existingProfile), }, clearKeys...) if err != nil { return "", nil, err } - // TODO: resources is not supported in the SDK yet. This function is - // a short-cut to add the property to the profile until it is supported - // in the SDK. - if err := databrickscfg.SaveResourcesToProfile(ctx, profileName, env.Get(ctx, "DATABRICKS_CONFIG_FILE"), resourcesList); err != nil { - return "", nil, err - } if err := storeLoginToken(ctx, tokenStore, mode, oauthArgument, token); err != nil { return "", nil, err } diff --git a/cmd/auth/token_test.go b/cmd/auth/token_test.go index e37e7cdc01b..001aa527034 100644 --- a/cmd/auth/token_test.go +++ b/cmd/auth/token_test.go @@ -7,6 +7,8 @@ import ( "errors" "net/http" "net/url" + "runtime" + "strings" "testing" "time" @@ -14,6 +16,7 @@ import ( "github.com/databricks/cli/libs/auth/storage" "github.com/databricks/cli/libs/auth/u2m" "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/cli/libs/cmdio/cmdiotest/termtest" "github.com/databricks/cli/libs/databrickscfg/profile" "github.com/databricks/cli/libs/env" "github.com/databricks/databricks-sdk-go/httpclient/fixtures" @@ -965,3 +968,33 @@ func TestWriteTokenErrorOutput(t *testing.T) { assert.Equal(t, unauthenticatedErrorCode, got.ErrorCode) assert.Equal(t, "refresh token is invalid", got.Message) } + +func TestResolveNoArgsTokenCreateNewProfileRejectsTakenName(t *testing.T) { + // See TestLoginDeviceCodePickerEmptyHost. + if runtime.GOOS == "windows" { + t.Skip("bubbletea cannot cancel reads on non-console input on Windows") + } + // Either one would resolve the profile before the picker is shown. + t.Setenv("DATABRICKS_HOST", "") + t.Setenv("DATABRICKS_CONFIG_PROFILE", "") + t.Setenv("DATABRICKS_CONFIG_FILE", writeTempConfig(t, `[prod] +host = https://prod.cloud.databricks.test + +[hostless] +cluster_id = abc +`)) + tm := termtest.Run(t, func(ctx context.Context) (string, error) { + name, _, err := resolveNoArgsToken(ctx, profile.DefaultProfiler, &auth.AuthArguments{}, nil, storage.StorageModePlaintext) + return name, err + }) + tm.WaitFor(profilePickerCreateNewLabel) + tm.Type(termtest.KeyDown + termtest.KeyEnter) + tm.WaitFor("Databricks profile name [DEFAULT]") + tm.Type("prod" + termtest.KeyEnter) + tm.WaitFor(">> profile prod already exists") + tm.Type(strings.Repeat(termtest.KeyBackspace, len("prod")) + "hostless" + termtest.KeyEnter) + tm.WaitFor(">> profile hostless already exists") + tm.Type(termtest.KeyCtrlC) + _, err := tm.Result() + assert.EqualError(t, err, "^C") +} diff --git a/libs/databrickscfg/ops.go b/libs/databrickscfg/ops.go index 6358c5ba49d..352cac235d7 100644 --- a/libs/databrickscfg/ops.go +++ b/libs/databrickscfg/ops.go @@ -123,6 +123,27 @@ func GetDefaultProfile(ctx context.Context, configFilePath string) (string, erro return GetDefaultProfileFrom(configFile), nil } +// GetSectionNames returns the names of the non-empty sections in the config +// file at configFilePath. Unlike LoadProfiles, it includes sections without a +// host. SaveToProfile merges into an existing section with the same name, so +// none of these names is free for a new profile. Returns nil if the file +// doesn't exist. +func GetSectionNames(ctx context.Context, configFilePath string) ([]string, error) { + configFile, err := loadConfigFile(ctx, configFilePath) + if err != nil || configFile == nil { + return nil, err + } + var names []string + for _, s := range configFile.Sections() { + // go-ini always has a DEFAULT section, even if the file does not. + if len(s.Keys()) == 0 { + continue + } + names = append(names, s.Name()) + } + return names, nil +} + // loadConfigFile loads a config file without creating it if it doesn't exist. // Returns (nil, nil) when the file is not found. func loadConfigFile(ctx context.Context, filename string) (*config.File, error) { @@ -210,11 +231,25 @@ func isFirstProfileInFile(configFile *config.File) bool { return true } -// SetDefaultProfile writes the default_profile key to the [__settings__] section. -func SetDefaultProfile(ctx context.Context, profileName, configFilePath string) error { +// ValidateProfileName returns an error if profileName is reserved for the +// [__settings__] section or cannot be written as a section header. +func ValidateProfileName(profileName string) error { if profileName == databricksSettingsSection { return fmt.Errorf("profile name %q is reserved for internal use", databricksSettingsSection) } + // go-ini writes section headers unescaped, so a line break would produce a + // config file that no longer parses. + if strings.ContainsAny(profileName, "\r\n") { + return fmt.Errorf("profile name %q must not contain line breaks", profileName) + } + return nil +} + +// SetDefaultProfile writes the default_profile key to the [__settings__] section. +func SetDefaultProfile(ctx context.Context, profileName, configFilePath string) error { + if err := ValidateProfileName(profileName); err != nil { + return err + } configFile, err := loadOrCreateConfigFile(ctx, configFilePath) if err != nil { @@ -428,8 +463,8 @@ func backupConfigFile(ctx context.Context, configFile *config.File) error { // removed (use this for mutually exclusive fields like cluster_id vs // serverless_compute_id, or to drop stale auth credentials on auth-type switch). 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 { + return err } configFile, err := loadOrCreateConfigFile(ctx, cfg.ConfigFile) diff --git a/libs/databrickscfg/ops_test.go b/libs/databrickscfg/ops_test.go index 8fe1956a81f..674723aafcc 100644 --- a/libs/databrickscfg/ops_test.go +++ b/libs/databrickscfg/ops_test.go @@ -294,6 +294,32 @@ func TestGetDefaultProfile_NoFile(t *testing.T) { assert.NoFileExists(t, path) } +func TestGetSectionNames(t *testing.T) { + path := filepath.Join(t.TempDir(), "databrickscfg") + err := os.WriteFile(path, []byte(`[__settings__] +default_profile = ws + +[ws] +host = https://ws.cloud.databricks.test + +[hostless] +cluster_id = abc + +[empty] +`), 0o600) + require.NoError(t, err) + + got, err := GetSectionNames(t.Context(), path) + require.NoError(t, err) + assert.Equal(t, []string{"__settings__", "ws", "hostless"}, got) +} + +func TestGetSectionNames_NoFile(t *testing.T) { + got, err := GetSectionNames(t.Context(), filepath.Join(t.TempDir(), "databrickscfg")) + require.NoError(t, err) + assert.Nil(t, got) +} + func TestGetConfiguredDefaultProfile(t *testing.T) { testCases := []struct { name string @@ -506,6 +532,49 @@ func TestSetDefaultProfile_RoundTrip(t *testing.T) { assert.Equal(t, "xyz", section.Key("token").String()) } +func TestValidateProfileName(t *testing.T) { + for _, tc := range []struct { + name string + profile string + wantErr string + }{ + {name: "plain", profile: "my-workspace"}, + {name: "dotted", profile: "team.prod"}, + {name: "reserved", profile: "__settings__", wantErr: "reserved for internal use"}, + {name: "newline", profile: "foo\nbar", wantErr: "must not contain line breaks"}, + {name: "carriage return", profile: "foo\rbar", wantErr: "must not contain line breaks"}, + {name: "opening bracket", profile: "[foo"}, + {name: "closing bracket", profile: "foo]"}, + } { + t.Run(tc.name, func(t *testing.T) { + err := ValidateProfileName(tc.profile) + if tc.wantErr == "" { + assert.NoError(t, err) + } else { + assert.ErrorContains(t, err, tc.wantErr) + } + }) + } +} + +func TestSaveToProfile_SquareBracketsRoundTrip(t *testing.T) { + ctx := t.Context() + path := filepath.Join(t.TempDir(), "databrickscfg") + + for _, name := range []string{"[foo]", "a[b", "a]b"} { + err := SaveToProfile(ctx, &config.Config{ + ConfigFile: path, + Profile: name, + Host: "https://ws.cloud.databricks.test", + }) + require.NoError(t, err) + } + + got, err := GetSectionNames(ctx, path) + require.NoError(t, err) + assert.Equal(t, []string{"[foo]", "__settings__", "a[b", "a]b"}, got) +} + func TestSaveToProfile_RejectsReservedProfileName(t *testing.T) { ctx := t.Context() path := filepath.Join(t.TempDir(), "databrickscfg")