login: improve text on already authenticated and on OAuth login #5774

Benehiko · 2025-01-27T11:18:46Z

Users have trouble understanding the different login paths on the CLI. The default login is performed through an OAuth flow with the option to fallback to a username and PAT login using the docker login -u <username> option.

This patch improves the text around docker login, indicating:

The username is shown when already authenticated
When not authenticated, the OAuth login flow explains the fallback clearly to the user and indicates a PAT can be used instead of a password.
The password prompt now explicitly states that it accepts a PAT.

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

codecov-commenter · 2025-01-27T11:22:26Z

Codecov Report

Attention: Patch coverage is 35.29412% with 11 lines in your changes missing coverage. Please review.

Project coverage is 59.41%. Comparing base (41c2786) to head (2198ccc).
Report is 10 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5774      +/-   ##
==========================================
- Coverage   59.42%   59.41%   -0.02%     
==========================================
  Files         347      347              
  Lines       29402    29414      +12     
==========================================
+ Hits        17472    17476       +4     
- Misses      10958    10966       +8     
  Partials      972      972

vvoland

LGTM

Benehiko · 2025-01-28T13:30:06Z

I'll update the tests quick so we can get this in. I think we might also want to adjust the prompt slightly when using docker login -u <username so that it says "Password/PAT:" instead of just "Password:"

Users have trouble understanding the different login paths on the CLI. The default login is performed through an OAuth flow with the option to fallback to a username and PAT login using the `docker login -u <username>` option. This patch improves the text around `docker login`, indicating: 1. The username is shown when already authenticated 2. When not authenticated, the OAuth login flow explains the fallback clearly to the user and indicates a PAT can be used instead of a password. 3. The password prompt now explicitly states that it accepts a PAT. Signed-off-by: Alano Terblanche <[email protected]>

Signed-off-by: Alano Terblanche <[email protected]>

Benehiko · 2025-01-28T15:33:38Z

@vvoland please take another look, added some additional changes and regenerated the documents

vvoland · 2025-01-29T13:22:25Z

cli/command/registry.go

@@ -178,7 +178,7 @@ func PromptUserForCredentials(ctx context.Context, cli Cli, argUser, argPassword
 			}
 		}()

-		argPassword, err = PromptForInput(ctx, cli.In(), cli.Out(), "Password: ")
+		argPassword, err = PromptForInput(ctx, cli.In(), cli.Out(), "Password/PAT: ")


The PAT feels a bit awkward as a prompt. @thaJeztah @laurazard any opinions on this?

Hmm, I'm a bit unsure. I think it's nice to be explicit (we could even say secret!) but I think in general, password is what's expected by users, and other CLIs that also accept other types of secrets usually default to password.

vvoland · 2025-01-29T13:23:38Z

cli/command/registry/login.go

+	flags.StringVarP(&opts.password, "password", "p", "", "Password/PAT")
+	flags.BoolVar(&opts.passwordStdin, "password-stdin", false, "Take the password/PAT from stdin")


WDYT about expanding the acronym in the description?

Suggested change

flags.StringVarP(&opts.password, "password", "p", "", "Password/PAT")

flags.BoolVar(&opts.passwordStdin, "password-stdin", false, "Take the password/PAT from stdin")

flags.StringVarP(&opts.password, "password", "p", "", "Password or Personal Access Token (PAT)")

flags.BoolVar(&opts.passwordStdin, "password-stdin", false, "Take the password/PAT from stdin")

Sure, I can expand it

laurazard · 2025-01-29T22:08:38Z

cli/command/registry.go

@@ -178,7 +178,7 @@ func PromptUserForCredentials(ctx context.Context, cli Cli, argUser, argPassword
 			}
 		}()

-		argPassword, err = PromptForInput(ctx, cli.In(), cli.Out(), "Password: ")
+		argPassword, err = PromptForInput(ctx, cli.In(), cli.Out(), "Password/PAT: ")


Hmm, I'm a bit unsure. I think it's nice to be explicit (we could even say secret!) but I think in general, password is what's expected by users, and other CLIs that also accept other types of secrets usually default to password.

laurazard · 2025-01-29T22:12:54Z

cli/command/registry/login.go

+		_, _ = fmt.Fprintf(dockerCli.Out(), " [Username: %s]", authConfig.Username)
+	}
+
+	_, _ = fmt.Fprintf(dockerCli.Out(), "\nTo login with a different account, run 'docker logout' followed by 'docker login'\n\n")


This is clearer for sure! I wish we could improve this eventually. Maybe in the future we can do something like prompt the user, if TTY – something like Found credentials for index.docker.io. Reuse credentials (Y/n)?

WDYT @thaJeztah @vvoland ?

laurazard · 2025-01-29T22:14:59Z

cli/internal/oauth/manager/manager.go

@@ -93,7 +93,8 @@ func (m *OAuthManager) LoginDevice(ctx context.Context, w io.Writer) (*types.Aut
 	}

 	_, _ = fmt.Fprintln(w, aec.Bold.Apply("\nUSING WEB-BASED LOGIN"))
-	_, _ = fmt.Fprintln(w, "To sign in with credentials on the command line, use 'docker login -u <username>'")
+	_, _ = fmt.Fprintln(w, "> To sign in with username and PAT credentials on the command line, use 'docker login -u <username>'")


...username and PAT credentials sounds weird (weirder than it already does).

Either keep it as is, which isn't great but works for password/PAT/whatever, or we should reword this.

laurazard · 2025-01-29T22:17:23Z

cli/internal/oauth/manager/manager.go

@@ -93,7 +93,8 @@ func (m *OAuthManager) LoginDevice(ctx context.Context, w io.Writer) (*types.Aut
 	}

 	_, _ = fmt.Fprintln(w, aec.Bold.Apply("\nUSING WEB-BASED LOGIN"))
-	_, _ = fmt.Fprintln(w, "To sign in with credentials on the command line, use 'docker login -u <username>'")
+	_, _ = fmt.Fprintln(w, "> To sign in with username and PAT credentials on the command line, use 'docker login -u <username>'")
+	_, _ = fmt.Fprintln(w, "> When prompted to enter a password, enter the PAT retrieved from: "+aec.Underline.Apply("https://app.docker.com/settings"))


??? This feels like the CLI going around the CLIs back and telling users to do something else 😅

Users can use PATs, but telling users explicitly to use PATs from the Web login prompt feels weird when they can also use passwords. And it turns an already long prompt that most users don't read all the way through even longer.

thoughts @dvdksn ?

Yeah, making this prompt even longer will have the opposite effect I think. The longer it is, the less likely a user is to read it at all. That's why we were trying to keep it as terse as possible in the first iteration.

dvdksn

What about Organizational Access Tokens (OAT)?

I think in general I would prefer if we used terminology like "Credential login" vs "Device login", where credential login encompasses username+pw/pat/oat. I don't like the idea of listing all the possible options in the CLI.

Signed-off-by: Alano Terblanche <[email protected]>

vvoland · 2025-01-31T11:40:05Z

cli/command/registry.go

@@ -178,7 +179,15 @@ func PromptUserForCredentials(ctx context.Context, cli Cli, argUser, argPassword
 			}
 		}()

-		argPassword, err = PromptForInput(ctx, cli.In(), cli.Out(), "Password: ")
+		var credentialPrompt strings.Builder
+		credentialPrompt.WriteString(


We could reuse the Info -> printing from image push, it also handles the non-tty vs tty output:

cli/cli/command/image/push.go

Lines 187 to 207 in bdd70c1

func printNote(dockerCli command.Cli, format string, args ...any) {

if dockerCli.Err().IsTerminal() {

format = strings.ReplaceAll(format, "--platform", aec.Bold.Apply("--platform"))

}

header := " Info -> "

padding := len(header)

if dockerCli.Err().IsTerminal() {

padding = len("i Info > ")

header = aec.Bold.Apply(aec.LightCyanB.Apply(aec.BlackF.Apply("i")) + " " + aec.LightCyanF.Apply("Info → "))

}

_, _ = fmt.Fprint(dockerCli.Err(), header)

s := fmt.Sprintf(format, args...)

for idx, line := range strings.Split(s, "\n") {

if idx > 0 {

_, _ = fmt.Fprint(dockerCli.Err(), strings.Repeat(" ", padding))

}

_, _ = fmt.Fprintln(dockerCli.Err(), aec.Italic.Apply(line))

}

}

hmm, maybe we could merge your PR https://github.com/docker/cli/pull/5744/files#diff-b4864574dd8732ca90419836bdba5372ed10f8107dc49bf811adbfc0a1506592R15-R36

vvoland approved these changes Jan 28, 2025

View reviewed changes

vvoland added status/2-code-review area/ux area/authentication labels Jan 28, 2025

vvoland added this to the 28.0.0 milestone Jan 28, 2025

Benehiko force-pushed the improve-login-text branch from 6594119 to 02f89d5 Compare January 28, 2025 14:29

mddocs: regenerate login flag descriptions

1812b69

Signed-off-by: Alano Terblanche <[email protected]>

Benehiko requested review from thaJeztah and a team as code owners January 28, 2025 15:14

Benehiko requested a review from vvoland January 28, 2025 15:33

vvoland reviewed Jan 29, 2025

View reviewed changes

Benehiko self-assigned this Jan 29, 2025

laurazard reviewed Jan 29, 2025

View reviewed changes

dvdksn reviewed Jan 30, 2025

View reviewed changes

login: improve text

2198ccc

Signed-off-by: Alano Terblanche <[email protected]>

vvoland reviewed Jan 31, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

login: improve text on already authenticated and on OAuth login #5774

login: improve text on already authenticated and on OAuth login #5774

Benehiko commented Jan 27, 2025 •

edited

Loading

codecov-commenter commented Jan 27, 2025 •

edited

Loading

vvoland left a comment

Benehiko commented Jan 28, 2025

Benehiko commented Jan 28, 2025

vvoland Jan 29, 2025

laurazard Jan 29, 2025

vvoland Jan 29, 2025

Benehiko Jan 29, 2025

laurazard Jan 29, 2025

laurazard Jan 29, 2025

laurazard Jan 29, 2025

laurazard Jan 29, 2025

laurazard Jan 29, 2025

dvdksn Jan 30, 2025

dvdksn left a comment

vvoland Jan 31, 2025

Benehiko Jan 31, 2025

		flags.StringVarP(&opts.password, "password", "p", "", "Password/PAT")
		flags.BoolVar(&opts.passwordStdin, "password-stdin", false, "Take the password/PAT from stdin")

	func printNote(dockerCli command.Cli, format string, args ...any) {
	if dockerCli.Err().IsTerminal() {
	format = strings.ReplaceAll(format, "--platform", aec.Bold.Apply("--platform"))
	}

	header := " Info -> "
	padding := len(header)
	if dockerCli.Err().IsTerminal() {
	padding = len("i Info > ")
	header = aec.Bold.Apply(aec.LightCyanB.Apply(aec.BlackF.Apply("i")) + " " + aec.LightCyanF.Apply("Info → "))
	}

	_, _ = fmt.Fprint(dockerCli.Err(), header)
	s := fmt.Sprintf(format, args...)
	for idx, line := range strings.Split(s, "\n") {
	if idx > 0 {
	_, _ = fmt.Fprint(dockerCli.Err(), strings.Repeat(" ", padding))
	}
	_, _ = fmt.Fprintln(dockerCli.Err(), aec.Italic.Apply(line))
	}
	}

login: improve text on already authenticated and on OAuth login #5774

Are you sure you want to change the base?

login: improve text on already authenticated and on OAuth login #5774

Conversation

Benehiko commented Jan 27, 2025 • edited Loading

codecov-commenter commented Jan 27, 2025 • edited Loading

Codecov Report

vvoland left a comment

Choose a reason for hiding this comment

Benehiko commented Jan 28, 2025

Benehiko commented Jan 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvdksn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benehiko commented Jan 27, 2025 •

edited

Loading

codecov-commenter commented Jan 27, 2025 •

edited

Loading