cli/command/registry: login: improve flag validation

Before this change, some errors could be ambiguous as they did not
distinguish a flag to be omitted, or set, but with an empty value.

For example, if a user would try to loging but with an empty username,
the error would suggest that the `--username` flag was not set (which
it was);

I don't have `MY_USERNAME` set in this shell;

    printenv MY_USERNAME || echo 'variable not set'
    variable not set

Now, attempting to do a non-interactive login would result in an
ambiguous error;

        echo "supersecret" | docker login --password-stdin --username "$MY_USERNAME"
        Must provide --username with --password-stdin

With this patch applied, the error indicates that the username was empty,
or not set;

        echo "supersecret" | docker login --password-stdin --username "$MY_USERNAME"
        username is empty
        echo "supersecret" | docker login --password-stdin
        the --password-stdin option requires --username to be set
        echo "supersecret" | docker login --password-stdin --password "supersecret"
        conflicting options: cannot specify both --password and --password-stdin

Signed-off-by: Sebastiaan van Stijn <[email protected]>

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

@@ -550,12 +550,17 @@ func TestLoginValidateFlags(t *testing.T) {
 		{
 			name:        "--password-stdin without --username",
 			args:        []string{"--password-stdin"},
-			expectedErr: `Must provide --username with --password-stdin`,
+			expectedErr: `the --password-stdin option requires --username to be set`,
 		},
 		{
 			name:        "--password-stdin with empty --username",
 			args:        []string{"--password-stdin", "--username", ""},
-			expectedErr: `Must provide --username with --password-stdin`,
+			expectedErr: `username is empty`,
+		},
+		{
+			name:        "empty --username",
+			args:        []string{"--username", ""},
+			expectedErr: `username is empty`,
 		},
 		{
 			name:        "--username without value",
@@ -565,7 +570,12 @@ func TestLoginValidateFlags(t *testing.T) {
 		{
 			name:        "conflicting options --password-stdin and --password",
 			args:        []string{"--password-stdin", "--password", ""},
-			expectedErr: `Must provide --username with --password-stdin`,
+			expectedErr: `conflicting options: cannot specify both --password and --password-stdin`,
+		},
+		{
+			name:        "empty --password",
+			args:        []string{"--password", ""},
+			expectedErr: `password is empty`,
 		},
 		{
 			name:        "--password without value",