password option: avoid redundant prompting for same user@host

Recent commit ab4a84c added support for per-subdir interactive password
prompting, by way of having a "password" line with no =value in a subdir-level
.skeema file. The original logic memoized passwords per directory hierarchy
(avoiding redundant prompting for subdirs, if the parent dir has the bare
"password" configuration), but it contained no memoization for sibling subdirs
that have a bare "password" configuration and emit the same user and host.

This commit adjusts the logic to account for this, and also refactors the call
pattern to be more natural: individual Skeema subcommands no longer need to
check for directories which require password prompting. This allows many of
the changes from ab4a84c to be reverted while offering improved functionality.

cmd_format.go

@@ -93,21 +93,14 @@ func formatDir(dir *fs.Dir) error {
 		return NewExitValue(CodeBadConfig, err.Error())
 	}
 
-	wsType, _ := dir.Config.GetEnum("workspace", "temp-schema", "docker")
-	if wsType != "docker" {
-		if err := dir.PromptPasswordIfRequested(); err != nil {
-			log.Warn(err)
-		}
-	}
-
 	// Get workspace options for dir. This involves connecting to the first defined
 	// instance, so that any auto-detect-related settings work properly. However,
 	// with workspace=docker we can ignore connection errors; we'll get reasonable
 	// defaults from workspace.OptionsForDir if inst is nil as long as flavor is set.
 	var wsOpts workspace.Options
 	if len(dir.LogicalSchemas) > 0 {
 		inst, err := dir.FirstInstance()
-		if wsType != "docker" || !dir.Config.Changed("flavor") {
+		if wsType, _ := dir.Config.GetEnum("workspace", "temp-schema", "docker"); wsType != "docker" || !dir.Config.Changed("flavor") {
 			if err != nil {
 				return NewExitValue(CodeBadConfig, err.Error())
 			} else if inst == nil {

cmd_lint.go

@@ -130,21 +130,14 @@ func lintDir(dir *fs.Dir) *linter.Result {
 		return linter.BadConfigResult(dir, err)
 	}
 
-	wsType, _ := dir.Config.GetEnum("workspace", "temp-schema", "docker")
-	if wsType != "docker" {
-		if err := dir.PromptPasswordIfRequested(); err != nil {
-			log.Warn(err)
-		}
-	}
-
 	// Get workspace options for dir. This involves connecting to the first defined
 	// instance, so that any auto-detect-related settings work properly. However,
 	// with workspace=docker we can ignore connection errors; we'll get reasonable
 	// defaults from workspace.OptionsForDir if inst is nil as long as flavor is set.
 	var wsOpts workspace.Options
 	if len(dir.LogicalSchemas) > 0 {
 		inst, err := dir.FirstInstance()
-		if wsType != "docker" || !dir.Config.Changed("flavor") {
+		if wsType, _ := dir.Config.GetEnum("workspace", "temp-schema", "docker"); wsType != "docker" || !dir.Config.Changed("flavor") {
 			if err != nil {
 				return linter.BadConfigResult(dir, err)
 			} else if inst == nil {

cmd_pull.go

@@ -60,9 +60,6 @@ func pullWalker(dir *fs.Dir, maxDepth int) error {
 		log.Warnf("Skipping %s: %s", dir, dir.ParseError)
 		return NewExitValue(CodeBadConfig, "")
 	}
-	if err := dir.PromptPasswordIfRequested(); err != nil {
-		log.Warn(err)
-	}
 
 	var instance *tengo.Instance
 	var err error

go.mod

@@ -12,7 +12,7 @@ require (
 	github.com/mitchellh/go-wordwrap v1.0.0
 	github.com/pmezard/go-difflib v1.0.0
 	github.com/sirupsen/logrus v1.8.1
-	github.com/skeema/mybase v1.0.16-0.20221012044356-502626dd5a4c
+	github.com/skeema/mybase v1.0.16-0.20221017222940-9300bbbd5258
 	golang.org/x/lint v0.0.0-20210508222113-6edffad5e616
 	golang.org/x/sync v0.0.0-20220601150217-0de741cfad7f
 	golang.org/x/term v0.0.0-20210927222741-03fcf44c2211

go.sum

@@ -763,8 +763,8 @@ github.com/sirupsen/logrus v1.6.0/go.mod h1:7uNnSEd1DgxDLC74fIahvMZmmYsHGZGEOFrf
 github.com/sirupsen/logrus v1.7.0/go.mod h1:yWOB1SBYBC5VeMP7gHvWumXLIWorT60ONWic61uBYv0=
 github.com/sirupsen/logrus v1.8.1 h1:dJKuHgqk1NNQlqoA6BTlM1Wf9DOH3NBjQyu0h9+AZZE=
 github.com/sirupsen/logrus v1.8.1/go.mod h1:yWOB1SBYBC5VeMP7gHvWumXLIWorT60ONWic61uBYv0=
-github.com/skeema/mybase v1.0.16-0.20221012044356-502626dd5a4c h1:1dmwio+6BKTdQdUeBvlXYbWM0DpJDpcJWbHrIwWh80g=
-github.com/skeema/mybase v1.0.16-0.20221012044356-502626dd5a4c/go.mod h1:RJIQU667zLYletaM3hMf8jtmrCaLgJm0OaOFb6DfXe0=
+github.com/skeema/mybase v1.0.16-0.20221017222940-9300bbbd5258 h1:ClimlU+dTbRXO6XjASTpbnUaglNHLcdrpiBRQYXi7a0=
+github.com/skeema/mybase v1.0.16-0.20221017222940-9300bbbd5258/go.mod h1:RJIQU667zLYletaM3hMf8jtmrCaLgJm0OaOFb6DfXe0=
 github.com/smartystreets/assertions v0.0.0-20180927180507-b2de0cb4f26d/go.mod h1:OnSkiWE9lh6wB0YB77sQom3nweQdgAjqCqsofrRNTgc=
 github.com/smartystreets/goconvey v0.0.0-20190330032615-68dc04aab96a/go.mod h1:syvi0/a8iFYH4r/RixwvyeAJjdLS9QV7WQ/tjFTllLA=
 github.com/smartystreets/goconvey v1.6.4/go.mod h1:syvi0/a8iFYH4r/RixwvyeAJjdLS9QV7WQ/tjFTllLA=

internal/applier/target.go

@@ -108,9 +108,6 @@ func TargetsForDir(dir *fs.Dir, maxDepth int) (targets []*Target, skipCount int)
 		log.Errorf("Skipping %s: %s\n", dir.Path, dir.ParseError)
 		return nil, 1
 	}
-	if err := dir.PromptPasswordIfRequested(); err != nil {
-		log.Warn(err)
-	}
 	if dir.Config.Changed("host") && dir.HasSchema() {
 		var instances []*tengo.Instance
 		instances, skipCount = instancesForDir(dir)

internal/fs/dir.go

@@ -338,7 +338,10 @@ func (dir *Dir) Instances() ([]*tengo.Instance, error) {
 	// Before looping over hostnames, do a single lookup of user, password,
 	// connect-options, port, socket.
 	user := dir.Config.GetAllowEnvVar("user")
-	password := dir.Config.GetAllowEnvVar("password")
+	password, err := dir.Password(hosts...)
+	if err != nil {
+		return nil, err // for example, need interactive password but STDIN isn't a TTY
+	}
 	var userAndPass string
 	if password == "" {
 		userAndPass = user
@@ -686,43 +689,74 @@ func (dir *Dir) Generator() (major, minor, patch int, edition string) {
 	return int(version.Major()), int(version.Minor()), int(version.Patch()), edition
 }
 
-// PromptPasswordIfRequested checks if this dir's configuration indicates a
-// password should be read from STDIN, due to a .skeema file having a bare
-// "password" line with no equals sign or value. If successful, the password
-// will be stored in the directory's configuration as a runtime override. An
-// error is returned if a password should be prompted but cannot, for example
+// Package-level user@host interactive password cache, used by Dir.Password()
+var cachedInteractivePasswords = make(map[string]string)
+
+// Password returns the configured password in this dir, a cached password
+// from a previous interactive password check, or an interactively-prompted
+// password from STDIN if one should be obtained based on the directory's
+// configuration. If interactive input is requested and successful, the password
+// will be returned and also cached, so that subsequent identical requests
+// return the password without prompting.
+//
+// Optionally supply one or more hostnames to affect the behavior of interactive
+// password prompts and caching: with no hosts, the prompt text will mention the
+// directory and be cached in the directory's configuration; with one or more
+// hosts, the prompt text will mention the first host and will cache values in
+// a package-level map independent of this dir.
+//
+// An error is returned if a password should be prompted but cannot, for example
 // due to STDIN not being a TTY.
-func (dir *Dir) PromptPasswordIfRequested() error {
+func (dir *Dir) Password(hosts ...string) (string, error) {
 	// Only prompt if password option was supplied with no equals sign or value.
 	// If it was supplied with an equals sign but set to a blank value, mybase
 	// will expose this as "''" from GetRaw, since GetRaw doesn't remove the quotes
 	// like other Config getters. This allows us to differentiate between "prompt
 	// on STDIN" and "intentionally no/blank password" situations.
 	if dir.Config.GetRaw("password") != "" {
-		return nil
+		return dir.Config.GetAllowEnvVar("password"), nil
 	}
 
-	// Since different dirs/hosts may have different passwords, indicate in the
-	// prompt text which one is being requested
+	cacheKeys := make([]string, len(hosts))
 	var promptArg string
-	if !dir.Config.Changed("host-wrapper") && dir.Config.Changed("host") && !strings.ContainsAny(dir.Config.Get("host"), ",$") {
-		promptArg = dir.Config.Get("host")
-		if port, _ := dir.Port(); port != 3306 {
-			promptArg = fmt.Sprintf("%s:%d", promptArg, port)
-		}
-	} else {
+	if len(hosts) == 0 {
+		// No need to check a cache for dir-level prompting, since the previous Config
+		// check will already have managed a previously-prompted password
 		promptArg = "directory " + dir.RelPath()
+	} else {
+		user := dir.Config.GetAllowEnvVar("user")
+		for n, host := range hosts {
+			cacheKeys[n] = user + "@" + host
+			if cachedPassword, ok := cachedInteractivePasswords[cacheKeys[n]]; ok {
+				return cachedPassword, nil
+			}
+		}
+		promptArg = cacheKeys[0]
+		if len(hosts) == 2 {
+			promptArg += " and " + cacheKeys[1]
+		} else if len(hosts) > 2 {
+			promptArg = fmt.Sprintf("%s and %d other servers", promptArg, len(hosts))
+		}
 	}
+
 	val, err := util.PromptPassword("Enter password for %s: ", promptArg)
 	if err != nil {
-		return fmt.Errorf("Unable to prompt password for %s: %w", promptArg, err)
+		return "", fmt.Errorf("Unable to prompt password for %s: %w", promptArg, err)
 	}
-	// We single-quote-wrap the value (escaping any internal single-quotes) to
-	// prevent a redundant pw prompt on an empty string, and also to prevent
-	// input of the form $SOME_ENV_VAR from performing env var substitution.
-	val = fmt.Sprintf("'%s'", strings.ReplaceAll(val, "'", "\\'"))
-	dir.Config.SetRuntimeOverride("password", val)
-	return nil
+
+	if len(hosts) == 0 {
+		// We single-quote-wrap the value (escaping any internal single-quotes) to
+		// prevent a redundant pw prompt on an empty string, and also to prevent
+		// input of the form $SOME_ENV_VAR from performing env var substitution.
+		cacheVal := fmt.Sprintf("'%s'", strings.ReplaceAll(val, "'", "\\'"))
+		dir.Config.SetRuntimeOverride("password", cacheVal)
+	}
+	for _, cacheKey := range cacheKeys {
+		// For caching per host, we use the value as-is since this does not go
+		// through mybase Config getters.
+		cachedInteractivePasswords[cacheKey] = val
+	}
+	return val, nil
 }
 
 // parseContents reads the .skeema and *.sql files in the dir, populating
@@ -814,6 +848,18 @@ func (dir *Dir) parseContents() {
 			dir.LogicalSchemas = append(dir.LogicalSchemas, ls)
 		}
 	}
+
+	// If the dir's configuration includes "password" with no =value, and the dir
+	// does not configure any hosts, prompt for password now. This way, any subdirs
+	// will inherit the password without having to each prompt individually.
+	if !dir.Config.Changed("host") {
+		// This has no side-effects if the dir isn't configured to prompt for pw
+		// interactively. It will only return an error if an interactive prompt
+		// is attempted but fails due to STDIN not being a TTY.
+		if _, err := dir.Password(); err != nil {
+			log.Warn(err)
+		}
+	}
 }
 
 // ParentOptionFiles returns a slice of *mybase.File, corresponding to the

internal/fs/dir_test.go

@@ -601,118 +601,118 @@ func TestAncestorPaths(t *testing.T) {
 	}
 }
 
-func TestPromptPasswordIfRequested(t *testing.T) {
+func TestDirPassword(t *testing.T) {
 	defer func() {
 		util.PasswordPromptInput = util.PasswordInputSource(util.NoInteractiveInput)
+		cachedInteractivePasswords = make(map[string]string)
 	}()
 
 	// If a parent dir .skeema file has a bare "password" line, pw should be
-	// prompted there but not redundantly for its subdirs
-	dir := getDir(t, "testdata/pwprompt/basedir")
+	// prompted there, but not redundantly for its subdirs, since it gets cached
+	// in the dir's config as a runtime override, at dir parsing time (e.g. getDir)
 	util.PasswordPromptInput = util.NewMockPasswordInput("basedir")
-	if err := dir.PromptPasswordIfRequested(); err != nil {
-		t.Fatalf("Unexpected error from mock password input: %v", err)
-	} else if actual := dir.Config.GetAllowEnvVar("password"); actual != "basedir" {
-		t.Errorf("Unexpected configuration for password: %q (expected %q)", actual, "basedir")
+	dir := getDir(t, "testdata/pwprompt/basedir")
+	util.PasswordPromptInput = util.PasswordInputSource(util.NoInteractiveInput)
+	if pw, err := dir.Password(); pw != "basedir" || err != nil {
+		t.Errorf("Unexpected return values from dir.Password(): %q, %v", pw, err)
 	}
 	util.PasswordPromptInput = util.NewMockPasswordInput("different value to ensure not re-prompted")
 	subdirs, err := dir.Subdirs()
 	if err != nil {
 		t.Fatalf("Unexpected error from Subdirs: %v", err)
 	}
 	for _, subdir := range subdirs {
-		if err := subdir.PromptPasswordIfRequested(); err != nil {
-			t.Fatalf("Unexpected error from mock password input: %v", err)
-		} else if actual := subdir.Config.GetAllowEnvVar("password"); actual != "basedir" {
-			t.Errorf("Unexpected configuration for password: %q (expected %q)", actual, "basedir")
+		if pw, err := subdir.Password(); pw != "basedir" || err != nil {
+			t.Errorf("Unexpected return values from subdir.Password(): %q, %v", pw, err)
 		}
 	}
 
 	// Same situation as above, but verify that a blank interactive password won't
 	// re-prompt redundantly for subdirs
-	dir = getDir(t, "testdata/pwprompt/basedir")
 	util.PasswordPromptInput = util.NewMockPasswordInput("")
-	if err := dir.PromptPasswordIfRequested(); err != nil {
-		t.Fatalf("Unexpected error from mock password input: %v", err)
-	} else if actual := dir.Config.GetAllowEnvVar("password"); actual != "" {
-		t.Errorf("Unexpected configuration for password: %q (expected %q)", actual, "")
-	}
+	dir = getDir(t, "testdata/pwprompt/basedir")
 	util.PasswordPromptInput = util.NewMockPasswordInput("different value to ensure not re-prompted")
+	if pw, err := dir.Password(); pw != "" || err != nil {
+		t.Errorf("Unexpected return values from dir.Password(): %q, %v", pw, err)
+	}
 	subdirs, err = dir.Subdirs()
 	if err != nil {
 		t.Fatalf("Unexpected error from Subdirs: %v", err)
 	}
 	for _, subdir := range subdirs {
-		if err := subdir.PromptPasswordIfRequested(); err != nil {
-			t.Fatalf("Unexpected error from mock password input: %v", err)
-		} else if actual := subdir.Config.GetAllowEnvVar("password"); actual != "" {
-			t.Errorf("Unexpected configuration for password: %q (expected %q)", actual, "")
+		if pw, err := subdir.Password(); pw != "" || err != nil {
+			t.Errorf("Unexpected return values from subdir.Password(): %q, %v", pw, err)
 		}
 	}
 
 	// If parent dir doesn't have bare "password" but both subdirs do, they should
 	// each prompt password separately
-	dir = getDir(t, "testdata/pwprompt/leafdir")
 	util.PasswordPromptInput = util.NewMockPasswordInput("basedir")
-	if err := dir.PromptPasswordIfRequested(); err != nil {
-		t.Fatalf("Unexpected error from mock password input: %v", err)
-	} else if dir.Config.Changed("password") {
-		t.Errorf("password value unexpectedly changed for base dir %s", dir)
+	dir = getDir(t, "testdata/pwprompt/leafdir")
+	if pw, err := dir.Password(); pw != "" || err != nil {
+		t.Errorf("Unexpected return values from dir.Password(): %q, %v", pw, err)
+	}
+	var counter int
+	util.PasswordPromptInput = func() (string, error) {
+		val := fmt.Sprintf("leaf-%d", counter)
+		counter++
+		return val, nil
 	}
 	subdirs, err = dir.Subdirs()
 	if err != nil {
 		t.Fatalf("Unexpected error from Subdirs: %v", err)
 	}
 	for n, subdir := range subdirs {
-		leafPassword := fmt.Sprintf("leaf-%d", n)
-		util.PasswordPromptInput = util.NewMockPasswordInput(leafPassword)
-		if err := subdir.PromptPasswordIfRequested(); err != nil {
-			t.Fatalf("Unexpected error from mock password input: %v", err)
-		} else if actual := subdir.Config.GetAllowEnvVar("password"); actual != leafPassword {
-			t.Errorf("Unexpected configuration for password: %q (expected %q)", actual, leafPassword)
+		if pw, err := subdir.Password(); pw != fmt.Sprintf("leaf-%d", n) || err != nil {
+			t.Errorf("Unexpected return values from subdir.Password(): %q, %v", pw, err)
 		}
 	}
 
 	// If an equals sign is present, but no value or an empty string, this means
 	// empty password (not prompt) for compat with MySQL client behavior
 	util.PasswordPromptInput = util.NewMockPasswordInput("this should not show up")
 	dir = getDir(t, "testdata/pwprompt/noprompt/a")
-	if err := dir.PromptPasswordIfRequested(); err != nil {
-		t.Fatalf("Unexpected error from mock password input: %v", err)
-	} else if dir.Config.GetAllowEnvVar("password") != "" {
-		t.Errorf("password value unexpectedly non-blank for dir %s", dir)
+	if pw, err := dir.Password(); pw != "" || err != nil {
+		t.Errorf("Unexpected return values from dir.Password(): %q, %v", pw, err)
 	}
 	dir = getDir(t, "testdata/pwprompt/noprompt/b")
-	if err := dir.PromptPasswordIfRequested(); err != nil {
-		t.Fatalf("Unexpected error from mock password input: %v", err)
-	} else if dir.Config.GetAllowEnvVar("password") != "" {
-		t.Errorf("password value unexpectedly non-blank for dir %s", dir)
+	if pw, err := dir.Password(); pw != "" || err != nil {
+		t.Errorf("Unexpected return values from dir.Password(): %q, %v", pw, err)
 	}
 
-	// The prompt string and error strings should normally contain the host, or
-	// host:port if port is nonstandard, or directory name if no host or multiple
-	// hosts. We cannot directly test the prompt string, but we can confirm it via
-	// error strings.
-	util.PasswordPromptInput = util.PasswordInputSource(util.NoInteractiveInput)
-	dir = getDir(t, "testdata/pwprompt/basedir") // no host
-	if err := dir.PromptPasswordIfRequested(); err == nil || !strings.Contains(err.Error(), "testdata/pwprompt/basedir") {
-		t.Errorf("Error value did not contain expected string: %v", err)
+	// Now test prompting with hostnames:
+	// The first dir here contains 3 hosts, but a prompt should only occur once.
+	// Set the mock input to return a value once, followed by errors on subsequent
+	// calls.
+	util.PasswordPromptInput = func() (string, error) {
+		util.PasswordPromptInput = util.NoInteractiveInput
+		return "success", nil
 	}
-	dir = getDir(t, "testdata/pwprompt/leafdir/a") // host, and port, but port is set to 3306 which shouldn't display
-	if err := dir.PromptPasswordIfRequested(); err == nil || !strings.Contains(err.Error(), "1.2.3.4") || strings.Contains(err.Error(), ":3306") {
-		t.Errorf("Error value did not contain expected string: %v", err)
+	dir = getDir(t, "testdata/pwprompt/hosts/a")
+	if pw, err := dir.Password(dir.Config.GetSlice("host", ',', true)...); pw != "success" || err != nil {
+		t.Errorf("Unexpected return values from dir.Password(): %q, %v", pw, err)
 	}
-	dir = getDir(t, "testdata/pwprompt/leafdir/b") // host, and non-3306 port
-	if err := dir.PromptPasswordIfRequested(); err == nil || !strings.Contains(err.Error(), "1.2.3.4:3307") {
-		t.Errorf("Error value did not contain expected string: %v", err)
+
+	// The second dir has only one host, but it's one that also existed in first
+	// dir, so its pw should be cached since it also has same username.
+	dir = getDir(t, "testdata/pwprompt/hosts/b")
+	if pw, err := dir.Password(dir.Config.GetSlice("host", ',', true)...); pw != "success" || err != nil {
+		t.Errorf("Unexpected return values from dir.Password(): %q, %v", pw, err)
 	}
-	dir = getDir(t, "testdata/pwprompt/multihost/a") // multiple hosts comma-separated
-	if err := dir.PromptPasswordIfRequested(); err == nil || !strings.Contains(err.Error(), "testdata/pwprompt/multihost/a") {
-		t.Errorf("Error value did not contain expected string: %v", err)
+
+	// The third dir has a single host that also appeared in first dir, but a
+	// different user name, so pw prompt should error instead of using cache!
+	dir = getDir(t, "testdata/pwprompt/hosts/c")
+	if pw, err := dir.Password(dir.Config.GetSlice("host", ',', true)...); pw != "" || err == nil {
+		t.Errorf("Unexpected return values from dir.Password(): %q, %v", pw, err)
 	}
-	dir = getDir(t, "testdata/pwprompt/multihost/b") // using host-wrapper
-	if err := dir.PromptPasswordIfRequested(); err == nil || !strings.Contains(err.Error(), "testdata/pwprompt/multihost/b") {
-		t.Errorf("Error value did not contain expected string: %v", err)
+
+	// The fourth dir has a single host that did not appear previously, altho
+	// same user name as first two dirs. PW prompt should error instead of using
+	// cache.
+	dir = getDir(t, "testdata/pwprompt/hosts/d")
+	if pw, err := dir.Password(dir.Config.GetSlice("host", ',', true)...); pw != "" || err == nil {
+		t.Errorf("Unexpected return values from dir.Password(): %q, %v", pw, err)
 	}
 }
 

internal/fs/testdata/pwprompt/hosts/a/.skeema

@@ -0,0 +1,3 @@
+host=1.2.3.4,5.6.7.8,9.8.7.6
+user=foo
+password