From 2adc544dea5f7653b01e20bae1fb2da884f50e61 Mon Sep 17 00:00:00 2001 From: Dominik Schulz Date: Fri, 29 Mar 2024 15:28:26 +0100 Subject: [PATCH] [bugfix] Default to true for core.exportkeys even in substores (#2848) * [bugfix] Default to true for core.exportkeys even in substores This PR changes the default for core.exportkeys from false to true in mounted substores to match the default of the global root store. It also refactors and simplifies the config package a little bit by removing special typed lookup methods and replacing them with conversion helpers that can be applied to any string. Fixes #2830 Signed-off-by: Dominik Schulz * Fix config tests Signed-off-by: Dominik Schulz --------- Signed-off-by: Dominik Schulz --- internal/action/generate.go | 10 ++-- internal/action/otp.go | 2 +- internal/action/show.go | 2 +- internal/action/sync.go | 2 +- internal/action/version.go | 2 +- internal/config/config.go | 51 +++++------------- internal/config/config_test.go | 4 +- internal/config/utils.go | 49 +++++++++++++++-- internal/config/utils_test.go | 87 +++++++++++++++++++++++++++++++ internal/store/leaf/recipients.go | 6 +-- main.go | 4 +- 11 files changed, 163 insertions(+), 56 deletions(-) create mode 100644 internal/config/utils_test.go diff --git a/internal/action/generate.go b/internal/action/generate.go index 8eb2258f13..a31c2f2a25 100644 --- a/internal/action/generate.go +++ b/internal/action/generate.go @@ -118,13 +118,13 @@ func (s *Action) generateCopyOrPrint(ctx context.Context, c *cli.Context, name, // copy to clipboard if: // - explicitly requested with -c // - autoclip=true, but only if output is not being redirected. - if IsClip(ctx) || (s.cfg.GetBool("generate.autoclip") && ctxutil.IsTerminal(ctx)) { - if err := clipboard.CopyTo(ctx, name, []byte(password), s.cfg.GetInt("core.cliptimeout")); err != nil { + if IsClip(ctx) || (config.AsBool(s.cfg.Get("generate.autoclip")) && ctxutil.IsTerminal(ctx)) { + if err := clipboard.CopyTo(ctx, name, []byte(password), config.AsInt(s.cfg.Get("core.cliptimeout"))); err != nil { return exit.Error(exit.IO, err, "failed to copy to clipboard: %s", err) } // if autoclip is on and we're not printing the password to the terminal // at least leave a notice that we did indeed copy it. - if s.cfg.GetBool("generate.autoclip") && !c.Bool("print") { + if config.AsBool(s.cfg.Get("generate.autoclip")) && !c.Bool("print") { out.Print(ctx, "Copied to clipboard") return nil @@ -177,7 +177,7 @@ func (s *Action) generatePassword(ctx context.Context, c *cli.Context, length, n symbols = c.Bool("symbols") } else { if cfg.GetM(mp, "generate.symbols") != "" { - symbols = cfg.GetBoolM(mp, "generate.symbols") + symbols = config.AsBool(cfg.GetM(mp, "generate.symbols")) } } @@ -521,5 +521,5 @@ func isStrict(ctx context.Context, c *cli.Context) bool { } // if the config option is not set, GetBoolM will return false by default - return cfg.GetBoolM(mp, "generate.strict") + return config.AsBool(cfg.GetM(mp, "generate.strict")) } diff --git a/internal/action/otp.go b/internal/action/otp.go index 3c042ea75d..0bbc9207d1 100644 --- a/internal/action/otp.go +++ b/internal/action/otp.go @@ -181,7 +181,7 @@ func (s *Action) otp(ctx context.Context, name, qrf string, clip, pw, recurse bo debug.Log("OTP period: %ds", two.Period()) if clip { - if err := clipboard.CopyTo(ctx, fmt.Sprintf("token for %s", name), []byte(token), s.cfg.GetInt("core.cliptimeout")); err != nil { + if err := clipboard.CopyTo(ctx, fmt.Sprintf("token for %s", name), []byte(token), config.AsInt(s.cfg.Get("core.cliptimeout"))); err != nil { return exit.Error(exit.IO, err, "failed to copy to clipboard: %s", err) } diff --git a/internal/action/show.go b/internal/action/show.go index a35f01626e..722a897b82 100644 --- a/internal/action/show.go +++ b/internal/action/show.go @@ -204,7 +204,7 @@ func (s *Action) showHandleOutput(ctx context.Context, name string, sec gopass.S } if (IsClip(ctx) || config.Bool(ctx, "show.autoclip")) && pw != "" { - if err := clipboard.CopyTo(ctx, name, []byte(pw), s.cfg.GetInt("core.cliptimeout")); err != nil { + if err := clipboard.CopyTo(ctx, name, []byte(pw), config.AsInt(s.cfg.Get("core.cliptimeout"))); err != nil { return err } } diff --git a/internal/action/sync.go b/internal/action/sync.go index e50e78691b..ecac9dccbe 100644 --- a/internal/action/sync.go +++ b/internal/action/sync.go @@ -219,7 +219,7 @@ func (s *Action) syncMount(ctx context.Context, mp string) error { } syncPrintDiff(ctxno, l, ln) - exportKeys := s.cfg.GetBoolM(mp, "core.exportkeys") + exportKeys := config.AsBool(s.cfg.GetM(mp, "core.exportkeys")) debug.Log("Syncing Mount %s. Exportkeys: %t", mp, exportKeys) if err := syncImportKeys(ctxno, sub, name); err != nil { return err diff --git a/internal/action/version.go b/internal/action/version.go index 84a197c9dd..f958e00903 100644 --- a/internal/action/version.go +++ b/internal/action/version.go @@ -53,7 +53,7 @@ func (s *Action) checkVersion(ctx context.Context, u chan string) { } // NB: "updater.check" isn't supported as a local config option, hence no mount point here - if cfg, _ := config.FromContext(ctx); cfg.IsSet("updater.check") && !cfg.GetBool("updater.check") { + if cfg, _ := config.FromContext(ctx); cfg.IsSet("updater.check") && !config.AsBool(cfg.Get("updater.check")) { debug.Log("remote version check disabled by updater.check = false") return diff --git a/internal/config/config.go b/internal/config/config.go index b2d0605884..b256d5a4e7 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -6,7 +6,6 @@ import ( "fmt" "os" "strconv" - "strings" "github.com/gopasspw/gopass/pkg/debug" "github.com/gopasspw/gopass/pkg/gitconfig" @@ -146,6 +145,19 @@ func (c *Config) IsSet(key string) bool { return c.root.IsSet(key) } +// IsSetM returns true if the key is set in the mount or the root config if mount is empty. +func (c *Config) IsSetM(mount, key string) bool { + if mount == "" || mount == "" { + return c.root.IsSet(key) + } + + if cfg := c.cfgs[mount]; cfg != nil { + return cfg.IsSet(key) + } + + return false +} + // Get returns the given key from the root config. func (c *Config) Get(key string) string { return c.root.Get(key) @@ -175,41 +187,6 @@ func (c *Config) GetM(mount, key string) string { return "" } -// GetBool returns true if the value of the key evaluates to "true". -// Otherwise, it returns false. -func (c *Config) GetBool(key string) bool { - return c.GetBoolM("", key) -} - -// GetBoolM returns true if the value of the key evaluates to "true" for the provided mount, -// or the root config if mount is empty. -// Otherwise, it returns false. -func (c *Config) GetBoolM(mount, key string) bool { - if strings.ToLower(strings.TrimSpace(c.GetM(mount, key))) == "true" { - return true - } - - return false -} - -// GetInt returns the integer value of the key if it can be parsed. -// Otherwise, it returns 0. -func (c *Config) GetInt(key string) int { - return c.GetIntM("", key) -} - -// GetIntM returns the integer value of the key if it can be parsed for the provided mount, -// or the root config if mount is empty -// Otherwise, it returns 0. -func (c *Config) GetIntM(mount, key string) int { - iv, err := strconv.Atoi(c.GetM(mount, key)) - if err != nil { - return 0 - } - - return iv -} - // Set tries to set the key to the given value. // The mount option is necessary to discern between // the per-user (global) and possible per-directory (local) @@ -361,7 +338,7 @@ func DefaultPasswordLengthFromEnv(ctx context.Context) (int, bool) { def := DefaultPasswordLength cfg, mp := FromContext(ctx) - if l := cfg.GetIntM(mp, "generate.length"); l > 0 { + if l := AsInt(cfg.GetM(mp, "generate.length")); l > 0 { def = l } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 12e395d0c3..a3f4d8bc4a 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -25,8 +25,8 @@ func TestConfig(t *testing.T) { require.NoError(t, cfg.Set("", "core.int", "42")) assert.Equal(t, "foo", cfg.Get("core.string")) - assert.True(t, cfg.GetBool("core.bool")) - assert.Equal(t, 42, cfg.GetInt("core.int")) + assert.True(t, AsBool(cfg.Get("core.bool"))) + assert.Equal(t, 42, AsInt(cfg.Get("core.int"))) require.NoError(t, cfg.SetEnv("env.string", "foo")) assert.Equal(t, "foo", cfg.Get("env.string")) diff --git a/internal/config/utils.go b/internal/config/utils.go index 59b83e27f4..b777f54a37 100644 --- a/internal/config/utils.go +++ b/internal/config/utils.go @@ -1,12 +1,55 @@ package config -import "context" +import ( + "context" + "strconv" +) + +// AsBool converts a string to a bool value. +func AsBool(s string) bool { + return AsBoolWithDefault(s, false) +} + +// AsBoolWithDefault converts a string to a bool value with a default value. +func AsBoolWithDefault(s string, def bool) bool { + if s == "" { + return def + } + + switch s { + case "1", "true", "yes", "on": + return true + case "0", "false", "no", "off": + return false + default: + return def + } +} + +// AsInt converts a string to an integer value. +func AsInt(s string) int { + return AsIntWithDefault(s, 0) +} + +// AsIntWithDefault converts a string to an integer value with a default value. +func AsIntWithDefault(s string, def int) int { + if s == "" { + return def + } + + i, err := strconv.Atoi(s) + if err != nil { + return def + } + + return i +} // Bool returns a bool value from the config in the context. func Bool(ctx context.Context, key string) bool { cfg, mp := FromContext(ctx) - return cfg.GetBoolM(mp, key) + return AsBool(cfg.GetM(mp, key)) } // String returns a string value from the config in the context. @@ -20,5 +63,5 @@ func String(ctx context.Context, key string) string { func Int(ctx context.Context, key string) int { cfg, mp := FromContext(ctx) - return cfg.GetIntM(mp, key) + return AsInt(cfg.GetM(mp, key)) } diff --git a/internal/config/utils_test.go b/internal/config/utils_test.go new file mode 100644 index 0000000000..444692320a --- /dev/null +++ b/internal/config/utils_test.go @@ -0,0 +1,87 @@ +package config + +import ( + "testing" +) + +func TestAsBoolWithDefault(t *testing.T) { + tests := []struct { + name string + s string + def bool + expected bool + }{ + { + name: "Empty string with default true", + s: "", + def: true, + expected: true, + }, + { + name: "Empty string with default false", + s: "", + def: false, + expected: false, + }, + { + name: "Valid string '1' with default true", + s: "1", + def: true, + expected: true, + }, + { + name: "Valid string '0' with default true", + s: "0", + def: true, + expected: false, + }, + // Add more test cases here + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + result := AsBoolWithDefault(test.s, test.def) + if result != test.expected { + t.Errorf("Expected %v, but got %v", test.expected, result) + } + }) + } +} + +func TestAsIntWithDefault(t *testing.T) { + tests := []struct { + name string + s string + def int + expected int + }{ + { + name: "Empty string with default 0", + s: "", + def: 0, + expected: 0, + }, + { + name: "Valid string '123' with default 0", + s: "123", + def: 0, + expected: 123, + }, + { + name: "Invalid string 'abc' with default 0", + s: "abc", + def: 0, + expected: 0, + }, + // Add more test cases here + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + result := AsIntWithDefault(test.s, test.def) + if result != test.expected { + t.Errorf("Expected %v, but got %v", test.expected, result) + } + }) + } +} diff --git a/internal/store/leaf/recipients.go b/internal/store/leaf/recipients.go index 0a08bf2933..2c9a2226e5 100644 --- a/internal/store/leaf/recipients.go +++ b/internal/store/leaf/recipients.go @@ -302,7 +302,7 @@ func (s *Store) getRecipients(ctx context.Context, idf string) (*recipients.Reci cfg, _ := config.FromContext(ctx) // check recipients hash, global config takes precedence here for security reasons - if cfg.GetGlobal("recipients.check") != "true" && !cfg.GetBoolM(s.alias, "recipients.check") { + if cfg.GetGlobal("recipients.check") != "true" && !config.AsBool(cfg.GetM(s.alias, "recipients.check")) { return rs, nil } @@ -464,7 +464,7 @@ func (s *Store) saveRecipients(ctx context.Context, rs recipientMarshaler, msg s } // save all recipients public keys to the repo if wanted - if cfg.GetBoolM(s.alias, "core.exportkeys") { + if config.AsBoolWithDefault(cfg.GetM(s.alias, "core.exportkeys"), true) { debug.Log("updating exported keys") if _, err := s.UpdateExportedPublicKeys(ctx, rs.IDs()); err != nil { out.Errorf(ctx, "Failed to export missing public keys: %s", err) @@ -487,7 +487,7 @@ func (s *Store) saveRecipients(ctx context.Context, rs recipientMarshaler, msg s return fmt.Errorf("failed to commit changes to git: %w", err) } - if !cfg.GetBoolM(s.alias, "core.autopush") { + if !config.AsBool(cfg.GetM(s.alias, "core.autopush")) { debug.Log("not pushing to git remote, core.autopush is false") return nil diff --git a/main.go b/main.go index b47795a326..3a78c65c32 100644 --- a/main.go +++ b/main.go @@ -113,7 +113,7 @@ func setupApp(ctx context.Context, sv semver.Version) (context.Context, *cli.App } // set some action callbacks - if !cfg.GetBool("core.autoimport") { + if !config.AsBool(cfg.Get("core.autoimport")) { ctx = ctxutil.WithImportFunc(ctx, termio.AskForKeyImport) } @@ -326,7 +326,7 @@ func initContext(ctx context.Context, cfg *config.Config) context.Context { // on all other platforms we should be able to use color. Only set // this if it's in the config. if cfg.IsSet("core.nocolor") { - color.NoColor = cfg.GetBool("core.nocolor") + color.NoColor = config.AsBool(cfg.Get("core.nocolor")) } }