Merge pull request #5401 from laurazard/login-non-tty-standardize

login: handle non-tty scenario consistently

Author: thaJeztah

cli/command/registry.go

@@ -124,17 +124,6 @@ func PromptUserForCredentials(ctx context.Context, cli Cli, argUser, argPassword
 		cli.SetIn(streams.NewIn(os.Stdin))
 	}
 
-	// Some links documenting this:
-	// - https://code.google.com/archive/p/mintty/issues/56
-	// - https://github.com/docker/docker/issues/15272
-	// - https://mintty.github.io/ (compatibility)
-	// Linux will hit this if you attempt `cat | docker login`, and Windows
-	// will hit this if you attempt docker login from mintty where stdin
-	// is a pipe, not a character based console.
-	if argPassword == "" && !cli.In().IsTerminal() {
-		return authConfig, errors.Errorf("Error: Cannot perform an interactive login from a non TTY device")
-	}
-
 	isDefaultRegistry := serverAddress == registry.IndexServer
 	defaultUsername = strings.TrimSpace(defaultUsername)
 

cli/command/registry/login.go

@@ -155,6 +155,17 @@ func isOauthLoginDisabled() bool {
 }
 
 func loginUser(ctx context.Context, dockerCli command.Cli, opts loginOptions, defaultUsername, serverAddress string) (*registrytypes.AuthenticateOKBody, error) {
+	// Some links documenting this:
+	// - https://code.google.com/archive/p/mintty/issues/56
+	// - https://github.com/docker/docker/issues/15272
+	// - https://mintty.github.io/ (compatibility)
+	// Linux will hit this if you attempt `cat | docker login`, and Windows
+	// 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")
+	}
+
 	// 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)

cli/command/registry/login_test.go

@@ -313,6 +313,145 @@ func TestRunLogin(t *testing.T) {
 	}
 }
 
+func TestLoginNonInteractive(t *testing.T) {
+	t.Run("no prior credentials", func(t *testing.T) {
+		testCases := []struct {
+			doc         string
+			username    bool
+			password    bool
+			expectedErr string
+		}{
+			{
+				doc:      "success - w/ user w/ password",
+				username: true,
+				password: true,
+			},
+			{
+				doc:         "error - w/o user w/o pass ",
+				username:    false,
+				password:    false,
+				expectedErr: "Error: Cannot perform an interactive login from a non TTY device",
+			},
+			{
+				doc:         "error - w/ user w/o pass",
+				username:    true,
+				password:    false,
+				expectedErr: "Error: Cannot perform an interactive login from a non TTY device",
+			},
+			{
+				doc:         "error - w/o user w/ pass",
+				username:    false,
+				password:    true,
+				expectedErr: "Error: Cannot perform an interactive login from a non TTY device",
+			},
+		}
+
+		// "" meaning default registry
+		registries := []string{"", "my-registry.com"}
+
+		for _, registry := 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()
+					configfile.Filename = tmpFile.Path()
+					options := loginOptions{
+						serverAddress: registry,
+					}
+					if tc.username {
+						options.user = "my-username"
+					}
+					if tc.password {
+						options.password = "my-password"
+					}
+
+					loginErr := runLogin(context.Background(), cli, options)
+					if tc.expectedErr != "" {
+						assert.Error(t, loginErr, tc.expectedErr)
+						return
+					}
+					assert.NilError(t, loginErr)
+				})
+			}
+		}
+	})
+
+	t.Run("w/ prior credentials", func(t *testing.T) {
+		testCases := []struct {
+			doc         string
+			username    bool
+			password    bool
+			expectedErr string
+		}{
+			{
+				doc:      "success - w/ user w/ password",
+				username: true,
+				password: true,
+			},
+			{
+				doc:      "success - w/o user w/o pass ",
+				username: false,
+				password: false,
+			},
+			{
+				doc:         "error - w/ user w/o pass",
+				username:    true,
+				password:    false,
+				expectedErr: "Error: Cannot perform an interactive login from a non TTY device",
+			},
+			{
+				doc:         "error - w/o user w/ pass",
+				username:    false,
+				password:    true,
+				expectedErr: "Error: Cannot perform an interactive login from a non TTY device",
+			},
+		}
+
+		// "" meaning default registry
+		registries := []string{"", "my-registry.com"}
+
+		for _, registry := 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()
+					configfile.Filename = tmpFile.Path()
+					serverAddress := registry
+					if serverAddress == "" {
+						serverAddress = "https://index.docker.io/v1/"
+					}
+					assert.NilError(t, configfile.GetCredentialsStore(serverAddress).Store(configtypes.AuthConfig{
+						Username:      "my-username",
+						Password:      "my-password",
+						ServerAddress: serverAddress,
+					}))
+
+					options := loginOptions{
+						serverAddress: registry,
+					}
+					if tc.username {
+						options.user = "my-username"
+					}
+					if tc.password {
+						options.password = "my-password"
+					}
+
+					loginErr := runLogin(context.Background(), cli, options)
+					if tc.expectedErr != "" {
+						assert.Error(t, loginErr, tc.expectedErr)
+						return
+					}
+					assert.NilError(t, loginErr)
+				})
+			}
+		}
+	})
+}
+
 func TestLoginTermination(t *testing.T) {
 	p, tty, err := pty.Open()
 	assert.NilError(t, err)