From c4a1684ae5346209b3787574361a25c81e4a5316 Mon Sep 17 00:00:00 2001 From: Tanner Stirrat Date: Thu, 19 Sep 2024 15:10:59 -0600 Subject: [PATCH 01/12] Separate existence concern from fetching concern --- internal/storage/config.go | 14 +++++++++++--- internal/storage/secrets.go | 23 ++++++++++++++++++----- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/internal/storage/config.go b/internal/storage/config.go index 739079e..741597e 100644 --- a/internal/storage/config.go +++ b/internal/storage/config.go @@ -15,6 +15,9 @@ const configFileName = "config.json" // ErrConfigNotFound is returned if there is no Config in a ConfigStore. var ErrConfigNotFound = errors.New("config did not exist") +// ErrTokenNotFound is returned if there is no Token in a ConfigStore. +var ErrTokenNotFound = errors.New("token does not exist") + // Config represents the contents of a zed configuration file. type Config struct { Version string @@ -69,23 +72,28 @@ func TokenWithOverride(overrideToken Token, referenceToken Token) (Token, error) // CurrentToken is a convenient way to obtain the CurrentToken field from the // current Config. -func CurrentToken(cs ConfigStore, ss SecretStore) (Token, error) { +func CurrentToken(cs ConfigStore, ss SecretStore) (token Token, err error) { cfg, err := cs.Get() if err != nil { return Token{}, err } - return GetToken(cfg.CurrentToken, ss) + return GetTokenIfExists(cfg.CurrentToken, ss) } // SetCurrentToken is a convenient way to set the CurrentToken field in a // the current config. func SetCurrentToken(name string, cs ConfigStore, ss SecretStore) error { // Ensure the token exists - if _, err := GetToken(name, ss); err != nil { + exists, err := TokenExists(name, ss) + if err != nil { return err } + if !exists { + return ErrTokenNotFound + } + cfg, err := cs.Get() if err != nil { if errors.Is(err, ErrConfigNotFound) { diff --git a/internal/storage/secrets.go b/internal/storage/secrets.go index 90d1ebb..c76620d 100644 --- a/internal/storage/secrets.go +++ b/internal/storage/secrets.go @@ -14,9 +14,6 @@ import ( "github.com/authzed/zed/internal/console" ) -// ErrTokenNotFound is returned if there is no Token in a ConfigStore. -var ErrTokenNotFound = errors.New("token does not exist") - type Token struct { Name string Endpoint string @@ -72,7 +69,8 @@ type SecretStore interface { Put(s Secrets) error } -func GetToken(name string, ss SecretStore) (Token, error) { +// Returns an empty token if no token exists. +func GetTokenIfExists(name string, ss SecretStore) (Token, error) { secrets, err := ss.Get() if err != nil { return Token{}, err @@ -84,7 +82,22 @@ func GetToken(name string, ss SecretStore) (Token, error) { } } - return Token{}, ErrTokenNotFound + return Token{}, nil +} + +func TokenExists(name string, ss SecretStore) (bool, error) { + secrets, err := ss.Get() + if err != nil { + return false, err + } + + for _, token := range secrets.Tokens { + if name == token.Name { + return true, nil + } + } + + return false, nil } func PutToken(t Token, ss SecretStore) error { From 6953f5772bfec949559e938d8f1f9b3a805c2815 Mon Sep 17 00:00:00 2001 From: Tanner Stirrat Date: Thu, 19 Sep 2024 15:12:27 -0600 Subject: [PATCH 02/12] Add tests that captured behavior --- internal/client/client_test.go | 73 ++++++++++++++++++++++++++-------- 1 file changed, 57 insertions(+), 16 deletions(-) diff --git a/internal/client/client_test.go b/internal/client/client_test.go index e1a0d92..60f3675 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -12,10 +12,11 @@ import ( ) func TestGetTokenWithCLIOverride(t *testing.T) { + require := require.New(t) testCert, err := os.CreateTemp("", "") - require.NoError(t, err) + require.NoError(err) _, err = testCert.Write([]byte("hi")) - require.NoError(t, err) + require.NoError(err) cmd := zedtesting.CreateTestCobraCommandWithFlagValue(t, zedtesting.StringFlag{FlagName: "token", FlagValue: "t1", Changed: true}, zedtesting.StringFlag{FlagName: "certificate-path", FlagValue: testCert.Name(), Changed: true}, @@ -29,13 +30,13 @@ func TestGetTokenWithCLIOverride(t *testing.T) { // cli args take precedence when defined to, err := client.GetTokenWithCLIOverride(cmd, storage.Token{}) - require.NoError(t, err) - require.True(t, to.AnyValue()) - require.Equal(t, "t1", to.APIToken) - require.Equal(t, "e1", to.Endpoint) - require.Equal(t, []byte("hi"), to.CACert) - require.Equal(t, &bTrue, to.Insecure) - require.Equal(t, &bTrue, to.NoVerifyCA) + require.NoError(err) + require.True(to.AnyValue()) + require.Equal("t1", to.APIToken) + require.Equal("e1", to.Endpoint) + require.Equal([]byte("hi"), to.CACert) + require.Equal(&bTrue, to.Insecure) + require.Equal(&bTrue, to.NoVerifyCA) // storage token takes precedence when defined cmd = zedtesting.CreateTestCobraCommandWithFlagValue(t, @@ -52,11 +53,51 @@ func TestGetTokenWithCLIOverride(t *testing.T) { Insecure: &bFalse, NoVerifyCA: &bFalse, }) - require.NoError(t, err) - require.True(t, to.AnyValue()) - require.Equal(t, "t2", to.APIToken) - require.Equal(t, "e2", to.Endpoint) - require.Equal(t, []byte("bye"), to.CACert) - require.Equal(t, &bFalse, to.Insecure) - require.Equal(t, &bFalse, to.NoVerifyCA) + require.NoError(err) + require.True(to.AnyValue()) + require.Equal("t2", to.APIToken) + require.Equal("e2", to.Endpoint) + require.Equal([]byte("bye"), to.CACert) + require.Equal(&bFalse, to.Insecure) + require.Equal(&bFalse, to.NoVerifyCA) +} + +func TestGetCurrentTokenWithCLIOverrideWithoutStoredContext(t *testing.T) { + // When we refactored the token setting logic, we broke the workflow where zed is used without a saved + // context. This asserts that that workflow works. + require := require.New(t) + cmd := zedtesting.CreateTestCobraCommandWithFlagValue(t, + zedtesting.StringFlag{FlagName: "token", FlagValue: "t1", Changed: true}, + zedtesting.StringFlag{FlagName: "endpoint", FlagValue: "e1", Changed: true}, + zedtesting.StringFlag{FlagName: "certificate-path", FlagValue: "", Changed: false}, + zedtesting.BoolFlag{FlagName: "insecure", FlagValue: true, Changed: true}, + ) + + bTrue := true + + configStore, secretStore := client.DefaultStorage() + token, err := client.GetCurrentTokenWithCLIOverride(cmd, configStore, secretStore) + + // cli args take precedence when defined + require.NoError(err) + require.True(token.AnyValue()) + require.Equal("t1", token.APIToken) + require.Equal("e1", token.Endpoint) + require.Equal(&bTrue, token.Insecure) +} + +func TestGetCurrentTokenWithCLIOverrideWithInsufficientArgs (t *testing.T) { + // This is to ensure that insufficient args don't unintentionally validate. + require := require.New(t) + cmd := zedtesting.CreateTestCobraCommandWithFlagValue(t, + zedtesting.StringFlag{FlagName: "token", FlagValue: "", Changed: false}, + zedtesting.StringFlag{FlagName: "endpoint", FlagValue: "e1", Changed: true}, + zedtesting.StringFlag{FlagName: "certificate-path", FlagValue: "", Changed: false}, + ) + + configStore, secretStore := client.DefaultStorage() + _, err := client.GetCurrentTokenWithCLIOverride(cmd, configStore, secretStore) + + // cli args take precedence when defined + require.NoError(err) } From 3f13864399b3203563d67d1b9098f7ce549384a0 Mon Sep 17 00:00:00 2001 From: Tanner Stirrat Date: Thu, 19 Sep 2024 15:12:38 -0600 Subject: [PATCH 03/12] Use IfExists form --- internal/client/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/client/client.go b/internal/client/client.go index b236267..e3d3330 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -54,7 +54,7 @@ func newClientForCurrentContext(cmd *cobra.Command) (Client, error) { } func newClientForContext(cmd *cobra.Command, contextName string, secretStore storage.SecretStore) (*authzed.Client, error) { - currentToken, err := storage.GetToken(contextName, secretStore) + currentToken, err := storage.GetTokenIfExists(contextName, secretStore) if err != nil { return nil, err } From c57f1cbf2c4a1f28b5d7a23387d4c49885b453cc Mon Sep 17 00:00:00 2001 From: Tanner Stirrat Date: Thu, 19 Sep 2024 15:46:32 -0600 Subject: [PATCH 04/12] Add handling for no config case --- internal/client/client.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/internal/client/client.go b/internal/client/client.go index e3d3330..5cdc72d 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -74,6 +74,14 @@ func newClientForContext(cmd *cobra.Command, contextName string, secretStore sto // GetCurrentTokenWithCLIOverride returns the current token, but overridden by any parameter specified via CLI args func GetCurrentTokenWithCLIOverride(cmd *cobra.Command, configStore storage.ConfigStore, secretStore storage.SecretStore) (storage.Token, error) { + // Handle the no-config case separately + configExists, err := configStore.Exists() + if err != nil { + return storage.Token{}, err + } + if !configExists { + return GetTokenWithCLIOverride(cmd, storage.Token{}) + } token, err := storage.CurrentToken( configStore, secretStore, From 3dfd4680785f934498b7e268103a03cb16a21eac Mon Sep 17 00:00:00 2001 From: Tanner Stirrat Date: Thu, 19 Sep 2024 15:46:57 -0600 Subject: [PATCH 05/12] Add existence function for config --- internal/storage/config.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/internal/storage/config.go b/internal/storage/config.go index 741597e..927a207 100644 --- a/internal/storage/config.go +++ b/internal/storage/config.go @@ -28,6 +28,7 @@ type Config struct { type ConfigStore interface { Get() (Config, error) Put(Config) error + Exists() (bool, error) } // TokenWithOverride returns a Token that retrieves its values from the reference Token, and has its values overridden @@ -147,6 +148,17 @@ func (s JSONConfigStore) Put(cfg Config) error { return atomicWriteFile(filepath.Join(s.ConfigPath, configFileName), cfgBytes, 0o774) } +func (s JSONConfigStore) Exists() (bool, error) { + _, err := os.Stat(filepath.Join(s.ConfigPath, configFileName)) + if err != nil { + if os.IsNotExist(err) { + return false, nil + } + return false, err + } + return true, nil +} + // atomicWriteFile writes data to filename+some suffix, then renames it into // filename. // From 5b857c31ff019528d3be85de1cfd7cedf8e6aa9b Mon Sep 17 00:00:00 2001 From: Tanner Stirrat Date: Thu, 19 Sep 2024 15:47:08 -0600 Subject: [PATCH 06/12] Finish writing tests --- internal/client/client_test.go | 37 ++++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/internal/client/client_test.go b/internal/client/client_test.go index 60f3675..4ca6737 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -2,6 +2,7 @@ package client_test import ( "os" + "path" "testing" "github.com/authzed/zed/internal/client" @@ -62,7 +63,7 @@ func TestGetTokenWithCLIOverride(t *testing.T) { require.Equal(&bFalse, to.NoVerifyCA) } -func TestGetCurrentTokenWithCLIOverrideWithoutStoredContext(t *testing.T) { +func TestGetCurrentTokenWithCLIOverrideWithoutConfigFile(t *testing.T) { // When we refactored the token setting logic, we broke the workflow where zed is used without a saved // context. This asserts that that workflow works. require := require.New(t) @@ -75,7 +76,39 @@ func TestGetCurrentTokenWithCLIOverrideWithoutStoredContext(t *testing.T) { bTrue := true - configStore, secretStore := client.DefaultStorage() + configStore := &storage.JSONConfigStore{ConfigPath: "/not/a/valid/path"} + secretStore := &storage.KeychainSecretStore{ConfigPath: "/not/a/valid/path"} + token, err := client.GetCurrentTokenWithCLIOverride(cmd, configStore, secretStore) + + // cli args take precedence when defined + require.NoError(err) + require.True(token.AnyValue()) + require.Equal("t1", token.APIToken) + require.Equal("e1", token.Endpoint) + require.Equal(&bTrue, token.Insecure) +} + +func TestGetCurrentTokenWithCLIOverrideWithoutSecretFile(t *testing.T) { + // When we refactored the token setting logic, we broke the workflow where zed is used without a saved + // context. This asserts that that workflow works. + require := require.New(t) + cmd := zedtesting.CreateTestCobraCommandWithFlagValue(t, + zedtesting.StringFlag{FlagName: "token", FlagValue: "t1", Changed: true}, + zedtesting.StringFlag{FlagName: "endpoint", FlagValue: "e1", Changed: true}, + zedtesting.StringFlag{FlagName: "certificate-path", FlagValue: "", Changed: false}, + zedtesting.BoolFlag{FlagName: "insecure", FlagValue: true, Changed: true}, + ) + + bTrue := true + + tmpDir, err := os.MkdirTemp("", "") + require.NoError(err) + configPath := path.Join(tmpDir, "config.json") + err = os.WriteFile(configPath, []byte("{}"), 0600) + require.NoError(err) + + configStore := &storage.JSONConfigStore{ConfigPath: tmpDir} + secretStore := &storage.KeychainSecretStore{ConfigPath: "/not/a/valid/path"} token, err := client.GetCurrentTokenWithCLIOverride(cmd, configStore, secretStore) // cli args take precedence when defined From ddf0f6253661810bc7adf90215aff33c7e7c7f6d Mon Sep 17 00:00:00 2001 From: Tanner Stirrat Date: Thu, 19 Sep 2024 15:49:16 -0600 Subject: [PATCH 07/12] Remove test that's hard to isolate --- internal/client/client_test.go | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/internal/client/client_test.go b/internal/client/client_test.go index 4ca6737..5538b42 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -118,19 +118,3 @@ func TestGetCurrentTokenWithCLIOverrideWithoutSecretFile(t *testing.T) { require.Equal("e1", token.Endpoint) require.Equal(&bTrue, token.Insecure) } - -func TestGetCurrentTokenWithCLIOverrideWithInsufficientArgs (t *testing.T) { - // This is to ensure that insufficient args don't unintentionally validate. - require := require.New(t) - cmd := zedtesting.CreateTestCobraCommandWithFlagValue(t, - zedtesting.StringFlag{FlagName: "token", FlagValue: "", Changed: false}, - zedtesting.StringFlag{FlagName: "endpoint", FlagValue: "e1", Changed: true}, - zedtesting.StringFlag{FlagName: "certificate-path", FlagValue: "", Changed: false}, - ) - - configStore, secretStore := client.DefaultStorage() - _, err := client.GetCurrentTokenWithCLIOverride(cmd, configStore, secretStore) - - // cli args take precedence when defined - require.NoError(err) -} From ca7c55ded3da11dd43a2ef981cdf52d5d67fb900 Mon Sep 17 00:00:00 2001 From: Tanner Stirrat Date: Thu, 19 Sep 2024 15:49:51 -0600 Subject: [PATCH 08/12] Gofumpt --- internal/client/client_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/client/client_test.go b/internal/client/client_test.go index 5538b42..f68bf5d 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -104,7 +104,7 @@ func TestGetCurrentTokenWithCLIOverrideWithoutSecretFile(t *testing.T) { tmpDir, err := os.MkdirTemp("", "") require.NoError(err) configPath := path.Join(tmpDir, "config.json") - err = os.WriteFile(configPath, []byte("{}"), 0600) + err = os.WriteFile(configPath, []byte("{}"), 0o600) require.NoError(err) configStore := &storage.JSONConfigStore{ConfigPath: tmpDir} From 194829136b47f39fd91e63ba32de753435dd1364 Mon Sep 17 00:00:00 2001 From: Tanner Stirrat Date: Thu, 19 Sep 2024 15:50:37 -0600 Subject: [PATCH 09/12] Update comment --- internal/client/client_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/client/client_test.go b/internal/client/client_test.go index f68bf5d..9f186ad 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -65,7 +65,7 @@ func TestGetTokenWithCLIOverride(t *testing.T) { func TestGetCurrentTokenWithCLIOverrideWithoutConfigFile(t *testing.T) { // When we refactored the token setting logic, we broke the workflow where zed is used without a saved - // context. This asserts that that workflow works. + // configuration. This asserts that that workflow works. require := require.New(t) cmd := zedtesting.CreateTestCobraCommandWithFlagValue(t, zedtesting.StringFlag{FlagName: "token", FlagValue: "t1", Changed: true}, From 75a4c78de5e3127ae3668f5cf66e538310007a94 Mon Sep 17 00:00:00 2001 From: Tanner Stirrat Date: Thu, 19 Sep 2024 15:24:22 -0700 Subject: [PATCH 10/12] Update internal/client/client_test.go Co-authored-by: Evan Cordell --- internal/client/client_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/client/client_test.go b/internal/client/client_test.go index 9f186ad..006bc4b 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -85,7 +85,8 @@ func TestGetCurrentTokenWithCLIOverrideWithoutConfigFile(t *testing.T) { require.True(token.AnyValue()) require.Equal("t1", token.APIToken) require.Equal("e1", token.Endpoint) - require.Equal(&bTrue, token.Insecure) + require.NotNil(token.Insecure) + require.True(*token.Insecure) } func TestGetCurrentTokenWithCLIOverrideWithoutSecretFile(t *testing.T) { From 05907d896339f93218abd708db8371b9e7fc79c2 Mon Sep 17 00:00:00 2001 From: Tanner Stirrat Date: Thu, 19 Sep 2024 16:31:26 -0600 Subject: [PATCH 11/12] Use errors.is, flatten chain --- internal/storage/config.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/internal/storage/config.go b/internal/storage/config.go index 927a207..1fca679 100644 --- a/internal/storage/config.go +++ b/internal/storage/config.go @@ -3,6 +3,7 @@ package storage import ( "encoding/json" "errors" + "io/fs" "os" "path/filepath" "runtime" @@ -119,10 +120,9 @@ var _ ConfigStore = JSONConfigStore{} // Get parses a Config from the filesystem. func (s JSONConfigStore) Get() (Config, error) { cfgBytes, err := os.ReadFile(filepath.Join(s.ConfigPath, configFileName)) - if err != nil { - if os.IsNotExist(err) { - return Config{}, ErrConfigNotFound - } + if errors.Is(err, fs.ErrNotExist) { + return Config{}, ErrConfigNotFound + } else if err != nil { return Config{}, err } @@ -149,11 +149,9 @@ func (s JSONConfigStore) Put(cfg Config) error { } func (s JSONConfigStore) Exists() (bool, error) { - _, err := os.Stat(filepath.Join(s.ConfigPath, configFileName)) - if err != nil { - if os.IsNotExist(err) { - return false, nil - } + if _, err := os.Stat(filepath.Join(s.ConfigPath, configFileName)); errors.Is(err, fs.ErrNotExist) { + return false, nil + } else if err != nil { return false, err } return true, nil From d7a9a00ada5c1c17b56d9de3f7040489224563ce Mon Sep 17 00:00:00 2001 From: Tanner Stirrat Date: Thu, 19 Sep 2024 16:41:19 -0600 Subject: [PATCH 12/12] Remove unused value --- internal/client/client_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/internal/client/client_test.go b/internal/client/client_test.go index 006bc4b..8770647 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -74,8 +74,6 @@ func TestGetCurrentTokenWithCLIOverrideWithoutConfigFile(t *testing.T) { zedtesting.BoolFlag{FlagName: "insecure", FlagValue: true, Changed: true}, ) - bTrue := true - configStore := &storage.JSONConfigStore{ConfigPath: "/not/a/valid/path"} secretStore := &storage.KeychainSecretStore{ConfigPath: "/not/a/valid/path"} token, err := client.GetCurrentTokenWithCLIOverride(cmd, configStore, secretStore)