Merge pull request #5569 from thaJeztah/27.x_backport_login_idempotent

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()
 }

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")

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,