login: handle non-tty scenario consistently

laurazard · laurazard · commit bbb6e7643d2e · 2024-09-03T14:26:11.000+01:00
Running `docker login` in a non-interactive environment sometimes errors out if no username/pwd is provided. This handling is somewhat inconsistent – this commit addresses that. Before: | `--username` | `--password` | Result | |:------------:|:------------:| ------------------------------------------------------------------ | | ✅ | ✅ | ✅ | | ❌ | ❌ | `Error: Cannot perform an interactive login from a non TTY device` | | ✅ | ❌ | `Error: Cannot perform an interactive login from a non TTY device` | | ❌ | ✅ | hangs | After: | `--username` | `--password` | Result | |:------------:|:------------:| ------------------------------------------------------------------ | | ✅ | ✅ | ✅ | | ❌ | ❌ | `Error: Cannot perform an interactive login from a non TTY device` | | ✅ | ❌ | `Error: Cannot perform an interactive login from a non TTY device` | | ❌ | ✅ | `Error: Cannot perform an interactive login from a non TTY device` | It's worth calling out a separate scenario – if there are previous, valid credentials, then running `docker login` with no username or password provided will use the previously stored credentials, and not error out. ```console cat ~/.docker/config.json { "auths": { "https://index.docker.io/v1/": { "auth": "xxxxxxxxxxx" } } } ⭑ docker login 0>/dev/null Authenticating with existing credentials... Login Succeeded ``` This commit also applies the same non-interactive handling logic to the new web-based login flow, which means that now, if there are no prior credentials stored and a user runs `docker login`, instead of initiating the new web-based login flow, an error is returned. Signed-off-by: Laura Brehm <laurabrehm@hey.com>
diff --git a/cli/command/registry.go b/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)
 
diff --git a/cli/command/registry/login.go b/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)
diff --git a/cli/command/registry/login_test.go b/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)
