Merge pull request #6036 from thaJeztah/improve_username_handling

cli/command/registry: login: improve flag validation

Author: thaJeztah

cli/command/registry/login.go

@@ -21,6 +21,7 @@ import (
 	"github.com/docker/docker/registry"
 	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
+	"github.com/spf13/pflag"
 )
 
 type loginOptions struct {
@@ -43,6 +44,9 @@ func NewLoginCommand(dockerCLI command.Cli) *cobra.Command {
 			if len(args) > 0 {
 				opts.serverAddress = args[0]
 			}
+			if err := verifyLoginFlags(cmd.Flags(), opts); err != nil {
+				return err
+			}
 			return runLogin(cmd.Context(), dockerCLI, opts)
 		},
 		Annotations: map[string]string{
@@ -60,17 +64,35 @@ func NewLoginCommand(dockerCLI command.Cli) *cobra.Command {
 	return cmd
 }
 
-func verifyLoginOptions(dockerCLI command.Cli, opts *loginOptions) error {
+// verifyLoginFlags validates flags set on the command.
+//
+// TODO(thaJeztah); combine with verifyLoginOptions, but this requires rewrites of many tests.
+func verifyLoginFlags(flags *pflag.FlagSet, opts loginOptions) error {
+	if flags.Changed("password-stdin") {
+		if flags.Changed("password") {
+			return errors.New("conflicting options: cannot specify both --password and --password-stdin")
+		}
+		if !flags.Changed("username") {
+			return errors.New("the --password-stdin option requires --username to be set")
+		}
+	}
+	if flags.Changed("username") && opts.user == "" {
+		return errors.New("username is empty")
+	}
+	if flags.Changed("password") && opts.password == "" {
+		return errors.New("password is empty")
+	}
+	return nil
+}
+
+func verifyLoginOptions(dockerCLI command.Streams, opts *loginOptions) error {
 	if opts.password != "" {
 		_, _ = fmt.Fprintln(dockerCLI.Err(), "WARNING! Using --password via the CLI is insecure. Use --password-stdin.")
-		if opts.passwordStdin {
-			return errors.New("--password and --password-stdin are mutually exclusive")
-		}
 	}
 
 	if opts.passwordStdin {
 		if opts.user == "" {
-			return errors.New("Must provide --username with --password-stdin")
+			return errors.New("username is empty")
 		}
 
 		contents, err := io.ReadAll(dockerCLI.In())

cli/command/registry/login_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"io"
 	"path/filepath"
 	"testing"
 	"time"
@@ -539,3 +540,61 @@ func TestIsOauthLoginDisabled(t *testing.T) {
 		assert.Equal(t, disabled, tc.disabled)
 	}
 }
+
+func TestLoginValidateFlags(t *testing.T) {
+	for _, tc := range []struct {
+		name        string
+		args        []string
+		expectedErr string
+	}{
+		{
+			name:        "--password-stdin without --username",
+			args:        []string{"--password-stdin"},
+			expectedErr: `the --password-stdin option requires --username to be set`,
+		},
+		{
+			name:        "--password-stdin with empty --username",
+			args:        []string{"--password-stdin", "--username", ""},
+			expectedErr: `username is empty`,
+		},
+		{
+			name:        "empty --username",
+			args:        []string{"--username", ""},
+			expectedErr: `username is empty`,
+		},
+		{
+			name:        "--username without value",
+			args:        []string{"--username"},
+			expectedErr: `flag needs an argument: --username`,
+		},
+		{
+			name:        "conflicting options --password-stdin and --password",
+			args:        []string{"--password-stdin", "--password", ""},
+			expectedErr: `conflicting options: cannot specify both --password and --password-stdin`,
+		},
+		{
+			name:        "empty --password",
+			args:        []string{"--password", ""},
+			expectedErr: `password is empty`,
+		},
+		{
+			name:        "--password without value",
+			args:        []string{"--password"},
+			expectedErr: `flag needs an argument: --password`,
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			cmd := NewLoginCommand(test.NewFakeCli(&fakeClient{}))
+			cmd.SetOut(io.Discard)
+			cmd.SetErr(io.Discard)
+			cmd.SetArgs(tc.args)
+
+			err := cmd.Execute()
+			if tc.expectedErr != "" {
+				assert.Check(t, is.ErrorContains(err, tc.expectedErr))
+			} else {
+				assert.Check(t, is.Nil(err))
+			}
+		})
+	}
+}