Use Get rather than GetAll to retrieve credentials from credential helpers.

Signed-off-by: Jake Sanders <jsand@google.com>

cli/config/configfile/file.go

@@ -251,24 +251,29 @@ func decodeAuth(authStr string) (string, string, error) {
 
 // GetCredentialsStore returns a new credentials store from the settings in the
 // configuration file
-func (configFile *ConfigFile) GetCredentialsStore(serverAddress string) credentials.Store {
+func (configFile *ConfigFile) GetCredentialsStore(registryHostname string) credentials.Store {
-	if helper := getConfiguredCredentialStore(configFile, serverAddress); helper != "" {
+	if helper := getConfiguredCredentialStore(configFile, registryHostname); helper != "" {
-		return credentials.NewNativeStore(configFile, helper)
+		return newNativeStore(configFile, helper)
 	}
 	return credentials.NewFileStore(configFile)
 }
 
+// var for unit testing.
+var newNativeStore = func(configFile *ConfigFile, helperSuffix string) credentials.Store {
+	return credentials.NewNativeStore(configFile, helperSuffix)
+}
+
 // GetAuthConfig for a repository from the credential store
-func (configFile *ConfigFile) GetAuthConfig(serverAddress string) (types.AuthConfig, error) {
+func (configFile *ConfigFile) GetAuthConfig(registryHostname string) (types.AuthConfig, error) {
-	return configFile.GetCredentialsStore(serverAddress).Get(serverAddress)
+	return configFile.GetCredentialsStore(registryHostname).Get(registryHostname)
 }
 
 // getConfiguredCredentialStore returns the credential helper configured for the
 // given registry, the default credsStore, or the empty string if neither are
 // configured.
-func getConfiguredCredentialStore(c *ConfigFile, serverAddress string) string {
+func getConfiguredCredentialStore(c *ConfigFile, registryHostname string) string {
-	if c.CredentialHelpers != nil && serverAddress != "" {
+	if c.CredentialHelpers != nil && registryHostname != "" {
-		if helper, exists := c.CredentialHelpers[serverAddress]; exists {
+		if helper, exists := c.CredentialHelpers[registryHostname]; exists {
 			return helper
 		}
 	}
@@ -285,19 +290,20 @@ func (configFile *ConfigFile) GetAllCredentials() (map[string]types.AuthConfig,
 		}
 	}
 
-	for registry := range configFile.CredentialHelpers {
-		helper := configFile.GetCredentialsStore(registry)
-		newAuths, err := helper.GetAll()
-		if err != nil {
-			return nil, err
-		}
-		addAll(newAuths)
-	}
 	defaultStore := configFile.GetCredentialsStore("")
 	newAuths, err := defaultStore.GetAll()
 	if err != nil {
 		return nil, err
 	}
 	addAll(newAuths)
+
+	// Auth configs from a registry-specific helper should override those from the default store.
+	for registryHostname := range configFile.CredentialHelpers {
+		newAuth, err := configFile.GetAuthConfig(registryHostname)
+		if err != nil {
+			return nil, err
+		}
+		auths[registryHostname] = newAuth
+	}
 	return auths, nil
 }

cli/config/configfile/file_test.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/docker/cli/cli/config/credentials"
 	"github.com/docker/docker/api/types"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -142,7 +143,37 @@ func TestConfigFile(t *testing.T) {
 	assert.Equal(t, configFilename, configFile.Filename)
 }
 
-func TestGetAllCredentials(t *testing.T) {
+type mockNativeStore struct {
+	GetAllCallCount int
+	authConfigs     map[string]types.AuthConfig
+}
+
+func (c *mockNativeStore) Erase(registryHostname string) error {
+	delete(c.authConfigs, registryHostname)
+	return nil
+}
+
+func (c *mockNativeStore) Get(registryHostname string) (types.AuthConfig, error) {
+	return c.authConfigs[registryHostname], nil
+}
+
+func (c *mockNativeStore) GetAll() (map[string]types.AuthConfig, error) {
+	c.GetAllCallCount = c.GetAllCallCount + 1
+	return c.authConfigs, nil
+}
+
+func (c *mockNativeStore) Store(authConfig types.AuthConfig) error {
+	return nil
+}
+
+// make sure it satisfies the interface
+var _ credentials.Store = (*mockNativeStore)(nil)
+
+func NewMockNativeStore(authConfigs map[string]types.AuthConfig) credentials.Store {
+	return &mockNativeStore{authConfigs: authConfigs}
+}
+
+func TestGetAllCredentialsFileStoreOnly(t *testing.T) {
 	configFile := New("filename")
 	exampleAuth := types.AuthConfig{
 		Username: "user",
@@ -157,3 +188,186 @@ func TestGetAllCredentials(t *testing.T) {
 	expected["example.com/foo"] = exampleAuth
 	assert.Equal(t, expected, authConfigs)
 }
+
+func TestGetAllCredentialsCredsStore(t *testing.T) {
+	configFile := New("filename")
+	configFile.CredentialsStore = "test_creds_store"
+	testRegistryHostname := "example.com"
+	expectedAuth := types.AuthConfig{
+		Username: "user",
+		Password: "pass",
+	}
+
+	testCredsStore := NewMockNativeStore(map[string]types.AuthConfig{testRegistryHostname: expectedAuth})
+
+	tmpNewNativeStore := newNativeStore
+	defer func() { newNativeStore = tmpNewNativeStore }()
+	newNativeStore = func(configFile *ConfigFile, helperSuffix string) credentials.Store {
+		return testCredsStore
+	}
+
+	authConfigs, err := configFile.GetAllCredentials()
+	require.NoError(t, err)
+
+	expected := make(map[string]types.AuthConfig)
+	expected[testRegistryHostname] = expectedAuth
+	assert.Equal(t, expected, authConfigs)
+	assert.Equal(t, 1, testCredsStore.(*mockNativeStore).GetAllCallCount)
+}
+
+func TestGetAllCredentialsCredHelper(t *testing.T) {
+	testCredHelperSuffix := "test_cred_helper"
+	testCredHelperRegistryHostname := "credhelper.com"
+	testExtraCredHelperRegistryHostname := "somethingweird.com"
+
+	unexpectedCredHelperAuth := types.AuthConfig{
+		Username: "file_store_user",
+		Password: "file_store_pass",
+	}
+	expectedCredHelperAuth := types.AuthConfig{
+		Username: "cred_helper_user",
+		Password: "cred_helper_pass",
+	}
+
+	configFile := New("filename")
+	configFile.CredentialHelpers = map[string]string{testCredHelperRegistryHostname: testCredHelperSuffix}
+
+	testCredHelper := NewMockNativeStore(map[string]types.AuthConfig{
+		testCredHelperRegistryHostname: expectedCredHelperAuth,
+		// Add in an extra auth entry which doesn't appear in CredentialHelpers section of the configFile.
+		// This verifies that only explicitly configured registries are being requested from the cred helpers.
+		testExtraCredHelperRegistryHostname: unexpectedCredHelperAuth,
+	})
+
+	tmpNewNativeStore := newNativeStore
+	defer func() { newNativeStore = tmpNewNativeStore }()
+	newNativeStore = func(configFile *ConfigFile, helperSuffix string) credentials.Store {
+		return testCredHelper
+	}
+
+	authConfigs, err := configFile.GetAllCredentials()
+	require.NoError(t, err)
+
+	expected := make(map[string]types.AuthConfig)
+	expected[testCredHelperRegistryHostname] = expectedCredHelperAuth
+	assert.Equal(t, expected, authConfigs)
+	assert.Equal(t, 0, testCredHelper.(*mockNativeStore).GetAllCallCount)
+}
+
+func TestGetAllCredentialsFileStoreAndCredHelper(t *testing.T) {
+	testFileStoreRegistryHostname := "example.com"
+	testCredHelperSuffix := "test_cred_helper"
+	testCredHelperRegistryHostname := "credhelper.com"
+
+	expectedFileStoreAuth := types.AuthConfig{
+		Username: "file_store_user",
+		Password: "file_store_pass",
+	}
+	expectedCredHelperAuth := types.AuthConfig{
+		Username: "cred_helper_user",
+		Password: "cred_helper_pass",
+	}
+
+	configFile := New("filename")
+	configFile.CredentialHelpers = map[string]string{testCredHelperRegistryHostname: testCredHelperSuffix}
+	configFile.AuthConfigs[testFileStoreRegistryHostname] = expectedFileStoreAuth
+
+	testCredHelper := NewMockNativeStore(map[string]types.AuthConfig{testCredHelperRegistryHostname: expectedCredHelperAuth})
+
+	newNativeStore = func(configFile *ConfigFile, helperSuffix string) credentials.Store {
+		return testCredHelper
+	}
+
+	tmpNewNativeStore := newNativeStore
+	defer func() { newNativeStore = tmpNewNativeStore }()
+	authConfigs, err := configFile.GetAllCredentials()
+	require.NoError(t, err)
+
+	expected := make(map[string]types.AuthConfig)
+	expected[testFileStoreRegistryHostname] = expectedFileStoreAuth
+	expected[testCredHelperRegistryHostname] = expectedCredHelperAuth
+	assert.Equal(t, expected, authConfigs)
+	assert.Equal(t, 0, testCredHelper.(*mockNativeStore).GetAllCallCount)
+}
+
+func TestGetAllCredentialsCredStoreAndCredHelper(t *testing.T) {
+	testCredStoreSuffix := "test_creds_store"
+	testCredStoreRegistryHostname := "credstore.com"
+	testCredHelperSuffix := "test_cred_helper"
+	testCredHelperRegistryHostname := "credhelper.com"
+
+	configFile := New("filename")
+	configFile.CredentialsStore = testCredStoreSuffix
+	configFile.CredentialHelpers = map[string]string{testCredHelperRegistryHostname: testCredHelperSuffix}
+
+	expectedCredStoreAuth := types.AuthConfig{
+		Username: "cred_store_user",
+		Password: "cred_store_pass",
+	}
+	expectedCredHelperAuth := types.AuthConfig{
+		Username: "cred_helper_user",
+		Password: "cred_helper_pass",
+	}
+
+	testCredHelper := NewMockNativeStore(map[string]types.AuthConfig{testCredHelperRegistryHostname: expectedCredHelperAuth})
+	testCredsStore := NewMockNativeStore(map[string]types.AuthConfig{testCredStoreRegistryHostname: expectedCredStoreAuth})
+
+	tmpNewNativeStore := newNativeStore
+	defer func() { newNativeStore = tmpNewNativeStore }()
+	newNativeStore = func(configFile *ConfigFile, helperSuffix string) credentials.Store {
+		if helperSuffix == testCredHelperSuffix {
+			return testCredHelper
+		}
+		return testCredsStore
+	}
+
+	authConfigs, err := configFile.GetAllCredentials()
+	require.NoError(t, err)
+
+	expected := make(map[string]types.AuthConfig)
+	expected[testCredStoreRegistryHostname] = expectedCredStoreAuth
+	expected[testCredHelperRegistryHostname] = expectedCredHelperAuth
+	assert.Equal(t, expected, authConfigs)
+	assert.Equal(t, 1, testCredsStore.(*mockNativeStore).GetAllCallCount)
+	assert.Equal(t, 0, testCredHelper.(*mockNativeStore).GetAllCallCount)
+}
+
+func TestGetAllCredentialsCredHelperOverridesDefaultStore(t *testing.T) {
+	testCredStoreSuffix := "test_creds_store"
+	testCredHelperSuffix := "test_cred_helper"
+	testRegistryHostname := "example.com"
+
+	configFile := New("filename")
+	configFile.CredentialsStore = testCredStoreSuffix
+	configFile.CredentialHelpers = map[string]string{testRegistryHostname: testCredHelperSuffix}
+
+	unexpectedCredStoreAuth := types.AuthConfig{
+		Username: "cred_store_user",
+		Password: "cred_store_pass",
+	}
+	expectedCredHelperAuth := types.AuthConfig{
+		Username: "cred_helper_user",
+		Password: "cred_helper_pass",
+	}
+
+	testCredHelper := NewMockNativeStore(map[string]types.AuthConfig{testRegistryHostname: expectedCredHelperAuth})
+	testCredsStore := NewMockNativeStore(map[string]types.AuthConfig{testRegistryHostname: unexpectedCredStoreAuth})
+
+	tmpNewNativeStore := newNativeStore
+	defer func() { newNativeStore = tmpNewNativeStore }()
+	newNativeStore = func(configFile *ConfigFile, helperSuffix string) credentials.Store {
+		if helperSuffix == testCredHelperSuffix {
+			return testCredHelper
+		}
+		return testCredsStore
+	}
+
+	authConfigs, err := configFile.GetAllCredentials()
+	require.NoError(t, err)
+
+	expected := make(map[string]types.AuthConfig)
+	expected[testRegistryHostname] = expectedCredHelperAuth
+	assert.Equal(t, expected, authConfigs)
+	assert.Equal(t, 1, testCredsStore.(*mockNativeStore).GetAllCallCount)
+	assert.Equal(t, 0, testCredHelper.(*mockNativeStore).GetAllCallCount)
+}