Merge pull request #5667 from thaJeztah/login_clean

cli/command/registry: assorted refactor and test changes

cli/command/registry/login.go

@@ -11,6 +11,7 @@ import (
 	"github.com/docker/cli/cli"
 	"github.com/docker/cli/cli/command"
 	"github.com/docker/cli/cli/command/completion"
+	"github.com/docker/cli/cli/config/configfile"
 	configtypes "github.com/docker/cli/cli/config/types"
 	"github.com/docker/cli/cli/internal/oauth/manager"
 	registrytypes "github.com/docker/docker/api/types/registry"
@@ -88,7 +89,7 @@ func runLogin(ctx context.Context, dockerCli command.Cli, opts loginOptions) err
 	}
 	var (
 		serverAddress string
-		response      *registrytypes.AuthenticateOKBody
+		msg           string
 	)
 	if opts.serverAddress != "" && opts.serverAddress != registry.DefaultNamespace {
 		serverAddress = opts.serverAddress
@@ -100,25 +101,25 @@ func runLogin(ctx context.Context, dockerCli command.Cli, opts loginOptions) err
 	// attempt login with current (stored) credentials
 	authConfig, err := command.GetDefaultAuthConfig(dockerCli.ConfigFile(), opts.user == "" && opts.password == "", serverAddress, isDefaultRegistry)
 	if err == nil && authConfig.Username != "" && authConfig.Password != "" {
-		response, err = loginWithStoredCredentials(ctx, dockerCli, authConfig)
+		msg, err = loginWithStoredCredentials(ctx, dockerCli, authConfig)
 	}
 
 	// if we failed to authenticate with stored credentials (or didn't have stored credentials),
 	// prompt the user for new credentials
 	if err != nil || authConfig.Username == "" || authConfig.Password == "" {
-		response, err = loginUser(ctx, dockerCli, opts, authConfig.Username, authConfig.ServerAddress)
+		msg, err = loginUser(ctx, dockerCli, opts, authConfig.Username, authConfig.ServerAddress)
 		if err != nil {
 			return err
 		}
 	}
 
-	if response != nil && response.Status != "" {
+	if msg != "" {
-		_, _ = fmt.Fprintln(dockerCli.Out(), response.Status)
+		_, _ = fmt.Fprintln(dockerCli.Out(), msg)
 	}
 	return nil
 }
 
-func loginWithStoredCredentials(ctx context.Context, dockerCli command.Cli, authConfig registrytypes.AuthConfig) (*registrytypes.AuthenticateOKBody, error) {
+func loginWithStoredCredentials(ctx context.Context, dockerCli command.Cli, authConfig registrytypes.AuthConfig) (msg string, _ error) {
 	_, _ = fmt.Fprintf(dockerCli.Out(), "Authenticating with existing credentials...\n")
 	response, err := dockerCli.Client().RegistryLogin(ctx, authConfig)
 	if err != nil {
@@ -134,11 +135,11 @@ func loginWithStoredCredentials(ctx context.Context, dockerCli command.Cli, auth
 		authConfig.IdentityToken = response.IdentityToken
 	}
 
-	if err := storeCredentials(dockerCli, authConfig); err != nil {
+	if err := storeCredentials(dockerCli.ConfigFile(), authConfig); err != nil {
-		return nil, err
+		return "", err
 	}
 
-	return &response, err
+	return response.Status, err
 }
 
 const OauthLoginEscapeHatchEnvVar = "DOCKER_CLI_DISABLE_OAUTH_LOGIN"
@@ -154,7 +155,7 @@ func isOauthLoginDisabled() bool {
 	return false
 }
 
-func loginUser(ctx context.Context, dockerCli command.Cli, opts loginOptions, defaultUsername, serverAddress string) (*registrytypes.AuthenticateOKBody, error) {
+func loginUser(ctx context.Context, dockerCli command.Cli, opts loginOptions, defaultUsername, serverAddress string) (msg string, _ error) {
 	// Some links documenting this:
 	// - https://code.google.com/archive/p/mintty/issues/56
 	// - https://github.com/docker/docker/issues/15272
@@ -163,16 +164,17 @@ func loginUser(ctx context.Context, dockerCli command.Cli, opts loginOptions, de
 	// will hit this if you attempt docker login from mintty where stdin
 	// is a pipe, not a character based console.
 	if (opts.user == "" || opts.password == "") && !dockerCli.In().IsTerminal() {
-		return nil, errors.Errorf("Error: Cannot perform an interactive login from a non TTY device")
+		return "", errors.Errorf("Error: Cannot perform an interactive login from a non TTY device")
 	}
 
 	// If we're logging into the index server and the user didn't provide a username or password, use the device flow
 	if serverAddress == registry.IndexServer && opts.user == "" && opts.password == "" && !isOauthLoginDisabled() {
-		response, err := loginWithDeviceCodeFlow(ctx, dockerCli)
+		var err error
+		msg, err = loginWithDeviceCodeFlow(ctx, dockerCli)
 		// if the error represents a failure to initiate the device-code flow,
 		// then we fallback to regular cli credentials login
 		if !errors.Is(err, manager.ErrDeviceLoginStartFail) {
-			return response, err
+			return msg, err
 		}
 		_, _ = fmt.Fprint(dockerCli.Err(), "Failed to start web-based login - falling back to command line login...\n\n")
 	}
@@ -180,50 +182,50 @@ func loginUser(ctx context.Context, dockerCli command.Cli, opts loginOptions, de
 	return loginWithUsernameAndPassword(ctx, dockerCli, opts, defaultUsername, serverAddress)
 }
 
-func loginWithUsernameAndPassword(ctx context.Context, dockerCli command.Cli, opts loginOptions, defaultUsername, serverAddress string) (*registrytypes.AuthenticateOKBody, error) {
+func loginWithUsernameAndPassword(ctx context.Context, dockerCli command.Cli, opts loginOptions, defaultUsername, serverAddress string) (msg string, _ error) {
 	// Prompt user for credentials
 	authConfig, err := command.PromptUserForCredentials(ctx, dockerCli, opts.user, opts.password, defaultUsername, serverAddress)
 	if err != nil {
-		return nil, err
+		return "", err
 	}
 
-	response, err := loginWithRegistry(ctx, dockerCli, authConfig)
+	response, err := loginWithRegistry(ctx, dockerCli.Client(), authConfig)
 	if err != nil {
-		return nil, err
+		return "", err
 	}
 
 	if response.IdentityToken != "" {
 		authConfig.Password = ""
 		authConfig.IdentityToken = response.IdentityToken
 	}
-	if err = storeCredentials(dockerCli, authConfig); err != nil {
+	if err = storeCredentials(dockerCli.ConfigFile(), authConfig); err != nil {
-		return nil, err
+		return "", err
 	}
 
-	return &response, nil
+	return response.Status, nil
 }
 
-func loginWithDeviceCodeFlow(ctx context.Context, dockerCli command.Cli) (*registrytypes.AuthenticateOKBody, error) {
+func loginWithDeviceCodeFlow(ctx context.Context, dockerCli command.Cli) (msg string, _ error) {
 	store := dockerCli.ConfigFile().GetCredentialsStore(registry.IndexServer)
 	authConfig, err := manager.NewManager(store).LoginDevice(ctx, dockerCli.Err())
 	if err != nil {
-		return nil, err
+		return "", err
 	}
 
-	response, err := loginWithRegistry(ctx, dockerCli, registrytypes.AuthConfig(*authConfig))
+	response, err := loginWithRegistry(ctx, dockerCli.Client(), registrytypes.AuthConfig(*authConfig))
 	if err != nil {
-		return nil, err
+		return "", err
 	}
 
-	if err = storeCredentials(dockerCli, registrytypes.AuthConfig(*authConfig)); err != nil {
+	if err = storeCredentials(dockerCli.ConfigFile(), registrytypes.AuthConfig(*authConfig)); err != nil {
-		return nil, err
+		return "", err
 	}
 
-	return &response, nil
+	return response.Status, nil
 }
 
-func storeCredentials(dockerCli command.Cli, authConfig registrytypes.AuthConfig) error {
+func storeCredentials(cfg *configfile.ConfigFile, authConfig registrytypes.AuthConfig) error {
-	creds := dockerCli.ConfigFile().GetCredentialsStore(authConfig.ServerAddress)
+	creds := cfg.GetCredentialsStore(authConfig.ServerAddress)
 	if err := creds.Store(configtypes.AuthConfig(authConfig)); err != nil {
 		return errors.Errorf("Error saving credentials: %v", err)
 	}
@@ -231,30 +233,32 @@ func storeCredentials(dockerCli command.Cli, authConfig registrytypes.AuthConfig
 	return nil
 }
 
-func loginWithRegistry(ctx context.Context, dockerCli command.Cli, authConfig registrytypes.AuthConfig) (registrytypes.AuthenticateOKBody, error) {
+func loginWithRegistry(ctx context.Context, apiClient client.SystemAPIClient, authConfig registrytypes.AuthConfig) (*registrytypes.AuthenticateOKBody, error) {
-	response, err := dockerCli.Client().RegistryLogin(ctx, authConfig)
+	response, err := apiClient.RegistryLogin(ctx, authConfig)
-	if err != nil && client.IsErrConnectionFailed(err) {
-		// If the server isn't responding (yet) attempt to login purely client side
-		response, err = loginClientSide(ctx, authConfig)
-	}
-	// If we (still) have an error, give up
 	if err != nil {
-		return registrytypes.AuthenticateOKBody{}, err
+		if client.IsErrConnectionFailed(err) {
+			// daemon isn't responding; attempt to login client side.
+			return loginClientSide(ctx, authConfig)
+		}
+		return nil, err
 	}
 
-	return response, nil
+	return &response, nil
 }
 
-func loginClientSide(ctx context.Context, auth registrytypes.AuthConfig) (registrytypes.AuthenticateOKBody, error) {
+func loginClientSide(ctx context.Context, auth registrytypes.AuthConfig) (*registrytypes.AuthenticateOKBody, error) {
 	svc, err := registry.NewService(registry.ServiceOptions{})
 	if err != nil {
-		return registrytypes.AuthenticateOKBody{}, err
+		return nil, err
 	}
 
 	status, token, err := svc.Auth(ctx, &auth, command.UserAgent())
+	if err != nil {
+		return nil, err
+	}
 
-	return registrytypes.AuthenticateOKBody{
+	return &registrytypes.AuthenticateOKBody{
 		Status:        status,
 		IdentityToken: token,
-	}, err
+	}, nil
 }

cli/command/registry/login_test.go

@@ -1,10 +1,10 @@
 package registry
 
 import (
-	"bytes"
 	"context"
 	"errors"
 	"fmt"
+	"path/filepath"
 	"testing"
 	"time"
 
@@ -55,8 +55,9 @@ func (c *fakeClient) RegistryLogin(_ context.Context, auth registrytypes.AuthCon
 func TestLoginWithCredStoreCreds(t *testing.T) {
 	testCases := []struct {
 		inputAuthConfig registrytypes.AuthConfig
-		expectedMsg     string
 		expectedErr     string
+		expectedMsg     string
+		expectedErrMsg  string
 	}{
 		{
 			inputAuthConfig: registrytypes.AuthConfig{},
@@ -66,20 +67,25 @@ func TestLoginWithCredStoreCreds(t *testing.T) {
 			inputAuthConfig: registrytypes.AuthConfig{
 				Username: unknownUser,
 			},
-			expectedMsg: "Authenticating with existing credentials...\n",
+			expectedErr:    errUnknownUser,
-			expectedErr: fmt.Sprintf("Login did not succeed, error: %s\n", errUnknownUser),
+			expectedMsg:    "Authenticating with existing credentials...\n",
+			expectedErrMsg: fmt.Sprintf("Login did not succeed, error: %s\n", errUnknownUser),
 		},
 	}
 	ctx := context.Background()
+	cli := test.NewFakeCli(&fakeClient{})
+	cli.ConfigFile().Filename = filepath.Join(t.TempDir(), "config.json")
 	for _, tc := range testCases {
-		cli := test.NewFakeCli(&fakeClient{})
+		_, err := loginWithStoredCredentials(ctx, cli, tc.inputAuthConfig)
-		errBuf := new(bytes.Buffer)
+		if tc.expectedErrMsg != "" {
-		cli.SetErr(streams.NewOut(errBuf))
+			assert.Check(t, is.Error(err, tc.expectedErr))
-		loginWithStoredCredentials(ctx, cli, tc.inputAuthConfig)
+		} else {
-		outputString := cli.OutBuffer().String()
+			assert.NilError(t, err)
-		assert.Check(t, is.Equal(tc.expectedMsg, outputString))
+		}
-		errorString := errBuf.String()
+		assert.Check(t, is.Equal(tc.expectedMsg, cli.OutBuffer().String()))
-		assert.Check(t, is.Equal(tc.expectedErr, errorString))
+		assert.Check(t, is.Equal(tc.expectedErrMsg, cli.ErrBuffer().String()))
+		cli.ErrBuffer().Reset()
+		cli.OutBuffer().Reset()
 	}
 }
 
@@ -349,16 +355,16 @@ func TestLoginNonInteractive(t *testing.T) {
 		// "" meaning default registry
 		registries := []string{"", "my-registry.com"}
 
-		for _, registry := range registries {
+		for _, registryAddr := range registries {
 			for _, tc := range testCases {
 				t.Run(tc.doc, func(t *testing.T) {
 					tmpFile := fs.NewFile(t, "test-run-login")
 					defer tmpFile.Remove()
 					cli := test.NewFakeCli(&fakeClient{})
-					configfile := cli.ConfigFile()
+					cfg := cli.ConfigFile()
-					configfile.Filename = tmpFile.Path()
+					cfg.Filename = tmpFile.Path()
 					options := loginOptions{
-						serverAddress: registry,
+						serverAddress: registryAddr,
 					}
 					if tc.username {
 						options.user = "my-username"
@@ -412,26 +418,26 @@ func TestLoginNonInteractive(t *testing.T) {
 		// "" meaning default registry
 		registries := []string{"", "my-registry.com"}
 
-		for _, registry := range registries {
+		for _, registryAddr := range registries {
 			for _, tc := range testCases {
 				t.Run(tc.doc, func(t *testing.T) {
 					tmpFile := fs.NewFile(t, "test-run-login")
 					defer tmpFile.Remove()
 					cli := test.NewFakeCli(&fakeClient{})
-					configfile := cli.ConfigFile()
+					cfg := cli.ConfigFile()
-					configfile.Filename = tmpFile.Path()
+					cfg.Filename = tmpFile.Path()
-					serverAddress := registry
+					serverAddress := registryAddr
 					if serverAddress == "" {
 						serverAddress = "https://index.docker.io/v1/"
 					}
-					assert.NilError(t, configfile.GetCredentialsStore(serverAddress).Store(configtypes.AuthConfig{
+					assert.NilError(t, cfg.GetCredentialsStore(serverAddress).Store(configtypes.AuthConfig{
 						Username:      "my-username",
 						Password:      "my-password",
 						ServerAddress: serverAddress,
 					}))
 
 					options := loginOptions{
-						serverAddress: registry,
+						serverAddress: registryAddr,
 					}
 					if tc.username {
 						options.user = "my-username"