diff --git a/cli/config/credentials/file_store.go b/cli/config/credentials/file_store.go index 3b8955994dc2..95406281501c 100644 --- a/cli/config/credentials/file_store.go +++ b/cli/config/credentials/file_store.go @@ -25,8 +25,13 @@ func NewFileStore(file store) Store { return &fileStore{file: file} } -// Erase removes the given credentials from the file store. +// Erase removes the given credentials from the file store.This function is +// idempotent and does not update the file if credentials did not change. func (c *fileStore) Erase(serverAddress string) error { + if _, exists := c.file.GetAuthConfigs()[serverAddress]; !exists { + // nothing to do; no credentials found for the given serverAddress + return nil + } delete(c.file.GetAuthConfigs(), serverAddress) return c.file.Save() } @@ -52,9 +57,14 @@ func (c *fileStore) GetAll() (map[string]types.AuthConfig, error) { return c.file.GetAuthConfigs(), nil } -// Store saves the given credentials in the file store. +// Store saves the given credentials in the file store. This function is +// idempotent and does not update the file if credentials did not change. func (c *fileStore) Store(authConfig types.AuthConfig) error { authConfigs := c.file.GetAuthConfigs() + if oldAuthConfig, ok := authConfigs[authConfig.ServerAddress]; ok && oldAuthConfig == authConfig { + // Credentials didn't change, so skip updating the configuration file. + return nil + } authConfigs[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 94e505e13c3d..466fa054a03e 100644 --- a/cli/config/credentials/file_store_test.go +++ b/cli/config/credentials/file_store_test.go @@ -10,9 +10,15 @@ import ( type fakeStore struct { configs map[string]types.AuthConfig + saveFn func(*fakeStore) error } func (f *fakeStore) Save() error { + if f.saveFn != nil { + // Pass a reference to the fakeStore itself in case saveFn + // wants to access it. + return f.saveFn(f) + } return nil } @@ -21,15 +27,82 @@ func (f *fakeStore) GetAuthConfigs() map[string]types.AuthConfig { } func (f *fakeStore) GetFilename() string { - return "/tmp/docker-fakestore" + return "no-config.json" } -func newStore(auths map[string]types.AuthConfig) store { - return &fakeStore{configs: auths} +// TestFileStoreIdempotent verifies that the config-file isn't updated +// if nothing changed. +func TestFileStoreIdempotent(t *testing.T) { + var saveCount, expectedSaveCount int + + s := NewFileStore(&fakeStore{ + configs: map[string]types.AuthConfig{}, + saveFn: func(*fakeStore) error { + saveCount++ + return nil + }, + }) + authOne := types.AuthConfig{ + Auth: "super_secret_token", + Email: "foo@example.com", + ServerAddress: "https://example.com", + } + authTwo := types.AuthConfig{ + Auth: "also_super_secret_token", + Email: "bar@example.com", + ServerAddress: "https://other.example.com", + } + + expectedSaveCount = 1 + t.Run("store new credentials", func(t *testing.T) { + assert.NilError(t, s.Store(authOne)) + retrievedAuth, err := s.Get(authOne.ServerAddress) + assert.NilError(t, err) + assert.Check(t, is.Equal(retrievedAuth, authOne)) + assert.Check(t, is.Equal(saveCount, expectedSaveCount)) + }) + t.Run("store same credentials is a no-op", func(t *testing.T) { + assert.NilError(t, s.Store(authOne)) + retrievedAuth, err := s.Get(authOne.ServerAddress) + assert.NilError(t, err) + assert.Check(t, is.Equal(retrievedAuth, authOne)) + assert.Check(t, is.Equal(saveCount, expectedSaveCount), "should not have saved if nothing changed") + }) + t.Run("store other credentials", func(t *testing.T) { + expectedSaveCount++ + assert.NilError(t, s.Store(authTwo)) + retrievedAuth, err := s.Get(authTwo.ServerAddress) + assert.NilError(t, err) + assert.Check(t, is.Equal(retrievedAuth, authTwo)) + assert.Check(t, is.Equal(saveCount, expectedSaveCount)) + }) + t.Run("erase credentials", func(t *testing.T) { + expectedSaveCount++ + assert.NilError(t, s.Erase(authOne.ServerAddress)) + retrievedAuth, err := s.Get(authOne.ServerAddress) + assert.NilError(t, err) + assert.Check(t, is.Equal(retrievedAuth, types.AuthConfig{})) + assert.Check(t, is.Equal(saveCount, expectedSaveCount)) + }) + t.Run("erase non-existing credentials is a no-op", func(t *testing.T) { + assert.NilError(t, s.Erase(authOne.ServerAddress)) + retrievedAuth, err := s.Get(authOne.ServerAddress) + assert.NilError(t, err) + assert.Check(t, is.Equal(retrievedAuth, types.AuthConfig{})) + assert.Check(t, is.Equal(saveCount, expectedSaveCount), "should not have saved if nothing changed") + }) + t.Run("erase other credentials", func(t *testing.T) { + expectedSaveCount++ + assert.NilError(t, s.Erase(authTwo.ServerAddress)) + retrievedAuth, err := s.Get(authTwo.ServerAddress) + assert.NilError(t, err) + assert.Check(t, is.Equal(retrievedAuth, types.AuthConfig{})) + assert.Check(t, is.Equal(saveCount, expectedSaveCount)) + }) } func TestFileStoreAddCredentials(t *testing.T) { - f := newStore(make(map[string]types.AuthConfig)) + f := &fakeStore{configs: map[string]types.AuthConfig{}} s := NewFileStore(f) auth := types.AuthConfig{ @@ -47,13 +120,13 @@ func TestFileStoreAddCredentials(t *testing.T) { } func TestFileStoreGet(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ "https://example.com": { Auth: "super_secret_token", Email: "foo@example.com", ServerAddress: "https://example.com", }, - }) + }} s := NewFileStore(f) a, err := s.Get("https://example.com") @@ -71,7 +144,7 @@ func TestFileStoreGet(t *testing.T) { func TestFileStoreGetAll(t *testing.T) { s1 := "https://example.com" s2 := "https://example2.example.com" - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ s1: { Auth: "super_secret_token", Email: "foo@example.com", @@ -82,7 +155,7 @@ func TestFileStoreGetAll(t *testing.T) { Email: "foo@example2.com", ServerAddress: "https://example2.example.com", }, - }) + }} s := NewFileStore(f) as, err := s.GetAll() @@ -107,13 +180,13 @@ func TestFileStoreGetAll(t *testing.T) { } func TestFileStoreErase(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ "https://example.com": { Auth: "super_secret_token", Email: "foo@example.com", ServerAddress: "https://example.com", }, - }) + }} s := NewFileStore(f) err := s.Erase("https://example.com") diff --git a/cli/config/credentials/native_store_test.go b/cli/config/credentials/native_store_test.go index 5abcca3587e0..f55d269ed59f 100644 --- a/cli/config/credentials/native_store_test.go +++ b/cli/config/credentials/native_store_test.go @@ -91,7 +91,7 @@ func mockCommandFn(args ...string) client.Program { } func TestNativeStoreAddCredentials(t *testing.T) { - f := newStore(make(map[string]types.AuthConfig)) + f := &fakeStore{configs: map[string]types.AuthConfig{}} s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), @@ -116,7 +116,7 @@ func TestNativeStoreAddCredentials(t *testing.T) { } func TestNativeStoreAddInvalidCredentials(t *testing.T) { - f := newStore(make(map[string]types.AuthConfig)) + f := &fakeStore{configs: map[string]types.AuthConfig{}} s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), @@ -132,11 +132,11 @@ func TestNativeStoreAddInvalidCredentials(t *testing.T) { } func TestNativeStoreGet(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, - }) + }} s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), @@ -154,11 +154,11 @@ func TestNativeStoreGet(t *testing.T) { } func TestNativeStoreGetIdentityToken(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ validServerAddress2: { Email: "foo@example2.com", }, - }) + }} s := &nativeStore{ programFunc: mockCommandFn, @@ -176,11 +176,11 @@ func TestNativeStoreGetIdentityToken(t *testing.T) { } func TestNativeStoreGetAll(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, - }) + }} s := &nativeStore{ programFunc: mockCommandFn, @@ -217,11 +217,11 @@ func TestNativeStoreGetAll(t *testing.T) { } func TestNativeStoreGetMissingCredentials(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, - }) + }} s := &nativeStore{ programFunc: mockCommandFn, @@ -232,11 +232,11 @@ func TestNativeStoreGetMissingCredentials(t *testing.T) { } func TestNativeStoreGetInvalidAddress(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, - }) + }} s := &nativeStore{ programFunc: mockCommandFn, @@ -247,11 +247,11 @@ func TestNativeStoreGetInvalidAddress(t *testing.T) { } func TestNativeStoreErase(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, - }) + }} s := &nativeStore{ programFunc: mockCommandFn, @@ -263,11 +263,11 @@ func TestNativeStoreErase(t *testing.T) { } func TestNativeStoreEraseInvalidAddress(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, - }) + }} s := &nativeStore{ programFunc: mockCommandFn,