From 33cbb70270b6d930579fa47c1a069f643dab52a5 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 21 Jun 2017 16:47:06 -0400 Subject: [PATCH] Cleanup config/credentials, remove dependency on config file. Signed-off-by: Daniel Nephin --- cli/config/config.go | 2 +- cli/config/configfile/file.go | 5 + cli/config/credentials/default_store.go | 21 ++- cli/config/credentials/file_store.go | 24 +-- cli/config/credentials/file_store_test.go | 60 ++++--- cli/config/credentials/native_store.go | 3 +- cli/config/credentials/native_store_test.go | 168 ++++++-------------- 7 files changed, 101 insertions(+), 182 deletions(-) diff --git a/cli/config/config.go b/cli/config/config.go index 74862f1ab2..ce296a6b13 100644 --- a/cli/config/config.go +++ b/cli/config/config.go @@ -120,7 +120,7 @@ func LoadDefaultConfigFile(err io.Writer) *configfile.ConfigFile { fmt.Fprintf(err, "WARNING: Error loading config file:%v\n", e) } if !configFile.ContainsAuth() { - credentials.DetectDefaultStore(configFile) + configFile.CredentialsStore = credentials.DetectDefaultStore(configFile.CredentialsStore) } return configFile } diff --git a/cli/config/configfile/file.go b/cli/config/configfile/file.go index e6dd13934c..0be6e70b63 100644 --- a/cli/config/configfile/file.go +++ b/cli/config/configfile/file.go @@ -127,6 +127,11 @@ func (configFile *ConfigFile) ContainsAuth() bool { len(configFile.AuthConfigs) > 0 } +// GetAuthConfigs returns the mapping of repo to auth configuration +func (configFile *ConfigFile) GetAuthConfigs() map[string]types.AuthConfig { + return configFile.AuthConfigs +} + // SaveToWriter encodes and writes out all the authorization information to // the given writer func (configFile *ConfigFile) SaveToWriter(writer io.Writer) error { diff --git a/cli/config/credentials/default_store.go b/cli/config/credentials/default_store.go index dc080dbd40..b2cc4df8bc 100644 --- a/cli/config/credentials/default_store.go +++ b/cli/config/credentials/default_store.go @@ -2,21 +2,18 @@ package credentials import ( "os/exec" - - "github.com/docker/cli/cli/config/configfile" ) -// DetectDefaultStore sets the default credentials store -// if the host includes the default store helper program. -func DetectDefaultStore(c *configfile.ConfigFile) { - if c.CredentialsStore != "" { - // user defined - return +// DetectDefaultStore return the default credentials store for the platform if +// the store executable is available. +func DetectDefaultStore(store string) string { + // user defined or no default for platform + if store != "" || defaultCredentialsStore == "" { + return store } - if defaultCredentialsStore != "" { - if _, err := exec.LookPath(remoteCredentialsPrefix + defaultCredentialsStore); err == nil { - c.CredentialsStore = defaultCredentialsStore - } + if _, err := exec.LookPath(remoteCredentialsPrefix + defaultCredentialsStore); err == nil { + return defaultCredentialsStore } + return "" } diff --git a/cli/config/credentials/file_store.go b/cli/config/credentials/file_store.go index 4e3325c79b..b186dbbed6 100644 --- a/cli/config/credentials/file_store.go +++ b/cli/config/credentials/file_store.go @@ -1,37 +1,39 @@ package credentials import ( - "github.com/docker/cli/cli/config/configfile" "github.com/docker/docker/api/types" "github.com/docker/docker/registry" ) +type store interface { + Save() error + GetAuthConfigs() map[string]types.AuthConfig +} + // fileStore implements a credentials store using // the docker configuration file to keep the credentials in plain text. type fileStore struct { - file *configfile.ConfigFile + file store } // NewFileStore creates a new file credentials store. -func NewFileStore(file *configfile.ConfigFile) Store { - return &fileStore{ - file: file, - } +func NewFileStore(file store) Store { + return &fileStore{file: file} } // Erase removes the given credentials from the file store. func (c *fileStore) Erase(serverAddress string) error { - delete(c.file.AuthConfigs, serverAddress) + delete(c.file.GetAuthConfigs(), serverAddress) return c.file.Save() } // Get retrieves credentials for a specific server from the file store. func (c *fileStore) Get(serverAddress string) (types.AuthConfig, error) { - authConfig, ok := c.file.AuthConfigs[serverAddress] + authConfig, ok := c.file.GetAuthConfigs()[serverAddress] if !ok { // Maybe they have a legacy config file, we will iterate the keys converting // them to the new format and testing - for r, ac := range c.file.AuthConfigs { + for r, ac := range c.file.GetAuthConfigs() { if serverAddress == registry.ConvertToHostname(r) { return ac, nil } @@ -43,11 +45,11 @@ func (c *fileStore) Get(serverAddress string) (types.AuthConfig, error) { } func (c *fileStore) GetAll() (map[string]types.AuthConfig, error) { - return c.file.AuthConfigs, nil + return c.file.GetAuthConfigs(), nil } // Store saves the given credentials in the file store. func (c *fileStore) Store(authConfig types.AuthConfig) error { - c.file.AuthConfigs[authConfig.ServerAddress] = authConfig + c.file.GetAuthConfigs()[authConfig.ServerAddress] = authConfig return c.file.Save() } diff --git a/cli/config/credentials/file_store_test.go b/cli/config/credentials/file_store_test.go index eadbcc23ef..0d4fbd6bdc 100644 --- a/cli/config/credentials/file_store_test.go +++ b/cli/config/credentials/file_store_test.go @@ -1,55 +1,49 @@ package credentials import ( - "io/ioutil" "testing" - "github.com/docker/cli/cli/config/configfile" "github.com/docker/docker/api/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -func newConfigFile(auths map[string]types.AuthConfig) *configfile.ConfigFile { - tmp, _ := ioutil.TempFile("", "docker-test") - name := tmp.Name() - tmp.Close() +type fakeStore struct { + configs map[string]types.AuthConfig +} - c := configfile.NewConfigFile(name) - c.AuthConfigs = auths - return c +func (f *fakeStore) Save() error { + return nil +} + +func (f *fakeStore) GetAuthConfigs() map[string]types.AuthConfig { + return f.configs +} + +func newStore(auths map[string]types.AuthConfig) store { + return &fakeStore{configs: auths} } func TestFileStoreAddCredentials(t *testing.T) { - f := newConfigFile(make(map[string]types.AuthConfig)) + f := newStore(make(map[string]types.AuthConfig)) s := NewFileStore(f) - err := s.Store(types.AuthConfig{ + auth := types.AuthConfig{ Auth: "super_secret_token", Email: "foo@example.com", ServerAddress: "https://example.com", - }) + } + err := s.Store(auth) + require.NoError(t, err) + assert.Len(t, f.GetAuthConfigs(), 1) - if err != nil { - t.Fatal(err) - } - - if len(f.AuthConfigs) != 1 { - t.Fatalf("expected 1 auth config, got %d", len(f.AuthConfigs)) - } - - a, ok := f.AuthConfigs["https://example.com"] - if !ok { - t.Fatalf("expected auth for https://example.com, got %v", f.AuthConfigs) - } - if a.Auth != "super_secret_token" { - t.Fatalf("expected auth `super_secret_token`, got %s", a.Auth) - } - if a.Email != "foo@example.com" { - t.Fatalf("expected email `foo@example.com`, got %s", a.Email) - } + actual, ok := f.GetAuthConfigs()["https://example.com"] + assert.True(t, ok) + assert.Equal(t, auth, actual) } func TestFileStoreGet(t *testing.T) { - f := newConfigFile(map[string]types.AuthConfig{ + f := newStore(map[string]types.AuthConfig{ "https://example.com": { Auth: "super_secret_token", Email: "foo@example.com", @@ -73,7 +67,7 @@ func TestFileStoreGet(t *testing.T) { func TestFileStoreGetAll(t *testing.T) { s1 := "https://example.com" s2 := "https://example2.com" - f := newConfigFile(map[string]types.AuthConfig{ + f := newStore(map[string]types.AuthConfig{ s1: { Auth: "super_secret_token", Email: "foo@example.com", @@ -109,7 +103,7 @@ func TestFileStoreGetAll(t *testing.T) { } func TestFileStoreErase(t *testing.T) { - f := newConfigFile(map[string]types.AuthConfig{ + f := newStore(map[string]types.AuthConfig{ "https://example.com": { Auth: "super_secret_token", Email: "foo@example.com", diff --git a/cli/config/credentials/native_store.go b/cli/config/credentials/native_store.go index cef34db92f..ef3aab4ad3 100644 --- a/cli/config/credentials/native_store.go +++ b/cli/config/credentials/native_store.go @@ -1,7 +1,6 @@ package credentials import ( - "github.com/docker/cli/cli/config/configfile" "github.com/docker/docker-credential-helpers/client" "github.com/docker/docker-credential-helpers/credentials" "github.com/docker/docker/api/types" @@ -22,7 +21,7 @@ type nativeStore struct { // NewNativeStore creates a new native store that // uses a remote helper program to manage credentials. -func NewNativeStore(file *configfile.ConfigFile, helperSuffix string) Store { +func NewNativeStore(file store, helperSuffix string) Store { name := remoteCredentialsPrefix + helperSuffix return &nativeStore{ programFunc: client.NewShellProgramFunc(name), diff --git a/cli/config/credentials/native_store_test.go b/cli/config/credentials/native_store_test.go index 360cc20efc..582c2bd598 100644 --- a/cli/config/credentials/native_store_test.go +++ b/cli/config/credentials/native_store_test.go @@ -11,7 +11,10 @@ import ( "github.com/docker/docker-credential-helpers/client" "github.com/docker/docker-credential-helpers/credentials" "github.com/docker/docker/api/types" + "github.com/docker/docker/pkg/testutil" "github.com/pkg/errors" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) const ( @@ -90,53 +93,32 @@ func mockCommandFn(args ...string) client.Program { } func TestNativeStoreAddCredentials(t *testing.T) { - f := newConfigFile(make(map[string]types.AuthConfig)) - f.CredentialsStore = "mock" - + f := newStore(make(map[string]types.AuthConfig)) s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), } - err := s.Store(types.AuthConfig{ + auth := types.AuthConfig{ Username: "foo", Password: "bar", Email: "foo@example.com", ServerAddress: validServerAddress, - }) + } + err := s.Store(auth) + require.NoError(t, err) + assert.Len(t, f.GetAuthConfigs(), 1) - if err != nil { - t.Fatal(err) - } - - if len(f.AuthConfigs) != 1 { - t.Fatalf("expected 1 auth config, got %d", len(f.AuthConfigs)) - } - - a, ok := f.AuthConfigs[validServerAddress] - if !ok { - t.Fatalf("expected auth for %s, got %v", validServerAddress, f.AuthConfigs) - } - if a.Auth != "" { - t.Fatalf("expected auth to be empty, got %s", a.Auth) - } - if a.Username != "" { - t.Fatalf("expected username to be empty, got %s", a.Username) - } - if a.Password != "" { - t.Fatalf("expected password to be empty, got %s", a.Password) - } - if a.IdentityToken != "" { - t.Fatalf("expected identity token to be empty, got %s", a.IdentityToken) - } - if a.Email != "foo@example.com" { - t.Fatalf("expected email `foo@example.com`, got %s", a.Email) + actual, ok := f.GetAuthConfigs()[validServerAddress] + assert.True(t, ok) + expected := types.AuthConfig{ + Email: auth.Email, + ServerAddress: auth.ServerAddress, } + assert.Equal(t, expected, actual) } func TestNativeStoreAddInvalidCredentials(t *testing.T) { - f := newConfigFile(make(map[string]types.AuthConfig)) - f.CredentialsStore = "mock" - + f := newStore(make(map[string]types.AuthConfig)) s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), @@ -147,102 +129,66 @@ func TestNativeStoreAddInvalidCredentials(t *testing.T) { Email: "foo@example.com", ServerAddress: invalidServerAddress, }) - - if err == nil { - t.Fatal("expected error, got nil") - } - - if !strings.Contains(err.Error(), "program failed") { - t.Fatalf("expected `program failed`, got %v", err) - } - - if len(f.AuthConfigs) != 0 { - t.Fatalf("expected 0 auth config, got %d", len(f.AuthConfigs)) - } + testutil.ErrorContains(t, err, "program failed") + assert.Len(t, f.GetAuthConfigs(), 0) } func TestNativeStoreGet(t *testing.T) { - f := newConfigFile(map[string]types.AuthConfig{ + f := newStore(map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, }) - f.CredentialsStore = "mock" - s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), } - a, err := s.Get(validServerAddress) - if err != nil { - t.Fatal(err) - } + actual, err := s.Get(validServerAddress) + require.NoError(t, err) - if a.Username != "foo" { - t.Fatalf("expected username `foo`, got %s", a.Username) - } - if a.Password != "bar" { - t.Fatalf("expected password `bar`, got %s", a.Password) - } - if a.IdentityToken != "" { - t.Fatalf("expected identity token to be empty, got %s", a.IdentityToken) - } - if a.Email != "foo@example.com" { - t.Fatalf("expected email `foo@example.com`, got %s", a.Email) + expected := types.AuthConfig{ + Username: "foo", + Password: "bar", + Email: "foo@example.com", } + assert.Equal(t, expected, actual) } func TestNativeStoreGetIdentityToken(t *testing.T) { - f := newConfigFile(map[string]types.AuthConfig{ + f := newStore(map[string]types.AuthConfig{ validServerAddress2: { Email: "foo@example2.com", }, }) - f.CredentialsStore = "mock" s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), } - a, err := s.Get(validServerAddress2) - if err != nil { - t.Fatal(err) - } + actual, err := s.Get(validServerAddress2) + require.NoError(t, err) - if a.Username != "" { - t.Fatalf("expected username to be empty, got %s", a.Username) - } - if a.Password != "" { - t.Fatalf("expected password to be empty, got %s", a.Password) - } - if a.IdentityToken != "abcd1234" { - t.Fatalf("expected identity token `abcd1234`, got %s", a.IdentityToken) - } - if a.Email != "foo@example2.com" { - t.Fatalf("expected email `foo@example2.com`, got %s", a.Email) + expected := types.AuthConfig{ + IdentityToken: "abcd1234", + Email: "foo@example2.com", } + assert.Equal(t, expected, actual) } func TestNativeStoreGetAll(t *testing.T) { - f := newConfigFile(map[string]types.AuthConfig{ + f := newStore(map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, }) - f.CredentialsStore = "mock" s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), } as, err := s.GetAll() - if err != nil { - t.Fatal(err) - } - - if len(as) != 2 { - t.Fatalf("wanted 2, got %d", len(as)) - } + require.NoError(t, err) + assert.Len(t, as, 2) if as[validServerAddress].Username != "foo" { t.Fatalf("expected username `foo` for %s, got %s", validServerAddress, as[validServerAddress].Username) @@ -271,86 +217,62 @@ func TestNativeStoreGetAll(t *testing.T) { } func TestNativeStoreGetMissingCredentials(t *testing.T) { - f := newConfigFile(map[string]types.AuthConfig{ + f := newStore(map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, }) - f.CredentialsStore = "mock" s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), } _, err := s.Get(missingCredsAddress) - if err != nil { - // missing credentials do not produce an error - t.Fatal(err) - } + assert.NoError(t, err) } func TestNativeStoreGetInvalidAddress(t *testing.T) { - f := newConfigFile(map[string]types.AuthConfig{ + f := newStore(map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, }) - f.CredentialsStore = "mock" s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), } _, err := s.Get(invalidServerAddress) - if err == nil { - t.Fatal("expected error, got nil") - } - - if !strings.Contains(err.Error(), "program failed") { - t.Fatalf("expected `program failed`, got %v", err) - } + testutil.ErrorContains(t, err, "program failed") } func TestNativeStoreErase(t *testing.T) { - f := newConfigFile(map[string]types.AuthConfig{ + f := newStore(map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, }) - f.CredentialsStore = "mock" s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), } err := s.Erase(validServerAddress) - if err != nil { - t.Fatal(err) - } - - if len(f.AuthConfigs) != 0 { - t.Fatalf("expected 0 auth configs, got %d", len(f.AuthConfigs)) - } + require.NoError(t, err) + assert.Len(t, f.GetAuthConfigs(), 0) } func TestNativeStoreEraseInvalidAddress(t *testing.T) { - f := newConfigFile(map[string]types.AuthConfig{ + f := newStore(map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, }) - f.CredentialsStore = "mock" s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), } err := s.Erase(invalidServerAddress) - if err == nil { - t.Fatal("expected error, got nil") - } - - if !strings.Contains(err.Error(), "program failed") { - t.Fatalf("expected `program failed`, got %v", err) - } + testutil.ErrorContains(t, err, "program failed") }