From 315cfd00a9dadc91fc58fdf05c866a143d463459 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 8 Sep 2025 13:45:43 +0200 Subject: [PATCH 1/3] login: touch-up error for non-TTY Signed-off-by: Sebastiaan van Stijn --- cli/command/registry/login.go | 2 +- cli/command/registry/login_test.go | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/cli/command/registry/login.go b/cli/command/registry/login.go index 894a85cdee72..cc8400ba26ce 100644 --- a/cli/command/registry/login.go +++ b/cli/command/registry/login.go @@ -210,7 +210,7 @@ func loginUser(ctx context.Context, dockerCLI command.Cli, opts loginOptions, de // 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 "", errors.New("error: cannot perform an interactive login from a non TTY device") + return "", errors.New("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 diff --git a/cli/command/registry/login_test.go b/cli/command/registry/login_test.go index 3fdceb638bae..91450a2c8682 100644 --- a/cli/command/registry/login_test.go +++ b/cli/command/registry/login_test.go @@ -128,7 +128,7 @@ func TestRunLogin(t *testing.T) { input: loginOptions{ serverAddress: "reg1", }, - expectedErr: "error: cannot perform an interactive login from a non TTY device", + expectedErr: "error: cannot perform an interactive login from a non-TTY device", }, { doc: "store valid username and password", @@ -335,19 +335,19 @@ func TestLoginNonInteractive(t *testing.T) { doc: "error - w/o user w/o pass ", username: false, password: false, - expectedErr: "error: cannot perform an interactive login from a non TTY device", + 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", + 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", + expectedErr: "error: cannot perform an interactive login from a non-TTY device", }, } @@ -404,13 +404,13 @@ func TestLoginNonInteractive(t *testing.T) { doc: "error - w/ user w/o pass", username: true, password: false, - expectedErr: "error: cannot perform an interactive login from a non TTY device", + 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", + expectedErr: "error: cannot perform an interactive login from a non-TTY device", }, } From 390447754c8f3f301665bc3a1d70f02fe24ac296 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 24 Mar 2025 18:40:31 +0100 Subject: [PATCH 2/3] cli/command/registry: remove all uses of response message The message returned by the API is a hardcoded message; the only real information currently returned by the API is whether or not the auth was successul; https://github.com/moby/moby/blob/v2.0.0-beta.0/daemon/server/router/system/system_routes.go#L408-L421 Signed-off-by: Sebastiaan van Stijn --- cli/command/registry/login.go | 57 ++++++++++-------------------- cli/command/registry/login_test.go | 2 +- 2 files changed, 20 insertions(+), 39 deletions(-) diff --git a/cli/command/registry/login.go b/cli/command/registry/login.go index cc8400ba26ce..2474613af8ae 100644 --- a/cli/command/registry/login.go +++ b/cli/command/registry/login.go @@ -118,10 +118,7 @@ func runLogin(ctx context.Context, dockerCLI command.Cli, opts loginOptions) err maybePrintEnvAuthWarning(dockerCLI) - var ( - serverAddress string - msg string - ) + var serverAddress string if opts.serverAddress != "" && opts.serverAddress != registry.DefaultNamespace { serverAddress = opts.serverAddress } else { @@ -132,25 +129,23 @@ func runLogin(ctx context.Context, dockerCLI command.Cli, opts loginOptions) err // attempt login with current (stored) credentials authConfig, err := command.GetDefaultAuthConfig(dockerCLI.ConfigFile(), opts.user == "" && opts.password == "", serverAddress, isDefaultRegistry) if err == nil && authConfig.Username != "" && authConfig.Password != "" { - msg, err = loginWithStoredCredentials(ctx, dockerCLI, authConfig) + err = loginWithStoredCredentials(ctx, dockerCLI, authConfig) } // if we failed to authenticate with stored credentials (or didn't have stored credentials), // prompt the user for new credentials if err != nil || authConfig.Username == "" || authConfig.Password == "" { - msg, err = loginUser(ctx, dockerCLI, opts, authConfig.Username, authConfig.ServerAddress) + err = loginUser(ctx, dockerCLI, opts, authConfig.Username, authConfig.ServerAddress) if err != nil { return err } } - if msg != "" { - _, _ = fmt.Fprintln(dockerCLI.Out(), msg) - } + _, _ = fmt.Fprintln(dockerCLI.Out(), "Login Succeeded") return nil } -func loginWithStoredCredentials(ctx context.Context, dockerCLI command.Cli, authConfig registrytypes.AuthConfig) (msg string, _ error) { +func loginWithStoredCredentials(ctx context.Context, dockerCLI command.Cli, authConfig registrytypes.AuthConfig) error { _, _ = fmt.Fprintf(dockerCLI.Err(), "Authenticating with existing credentials...") if authConfig.Username != "" { _, _ = fmt.Fprintf(dockerCLI.Err(), " [Username: %s]", authConfig.Username) @@ -176,11 +171,7 @@ func loginWithStoredCredentials(ctx context.Context, dockerCLI command.Cli, auth authConfig.IdentityToken = response.IdentityToken } - if err := storeCredentials(dockerCLI.ConfigFile(), authConfig); err != nil { - return "", err - } - - return response.Status, err + return storeCredentials(dockerCLI.ConfigFile(), authConfig) } // OauthLoginEscapeHatchEnvVar disables the browser-based OAuth login workflow. @@ -201,7 +192,7 @@ func isOauthLoginDisabled() bool { return false } -func loginUser(ctx context.Context, dockerCLI command.Cli, opts loginOptions, defaultUsername, serverAddress string) (msg string, _ error) { +func loginUser(ctx context.Context, dockerCLI command.Cli, opts loginOptions, defaultUsername, serverAddress string) error { // Some links documenting this: // - https://code.google.com/archive/p/mintty/issues/56 // - https://github.com/docker/docker/issues/15272 @@ -210,17 +201,16 @@ func loginUser(ctx context.Context, dockerCLI command.Cli, opts loginOptions, de // 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 "", errors.New("error: cannot perform an interactive login from a non-TTY device") + return errors.New("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() { - var err error - msg, err = loginWithDeviceCodeFlow(ctx, dockerCLI) + err := loginWithDeviceCodeFlow(ctx, dockerCLI) // if the error represents a failure to initiate the device-code flow, // then we fallback to regular cli credentials login if !errors.Is(err, manager.ErrDeviceLoginStartFail) { - return msg, err + return err } _, _ = fmt.Fprint(dockerCLI.Err(), "Failed to start web-based login - falling back to command line login...\n\n") } @@ -228,46 +218,38 @@ func loginUser(ctx context.Context, dockerCLI command.Cli, opts loginOptions, de return loginWithUsernameAndPassword(ctx, dockerCLI, opts, defaultUsername, serverAddress) } -func loginWithUsernameAndPassword(ctx context.Context, dockerCLI command.Cli, opts loginOptions, defaultUsername, serverAddress string) (msg string, _ error) { +func loginWithUsernameAndPassword(ctx context.Context, dockerCLI command.Cli, opts loginOptions, defaultUsername, serverAddress string) error { // Prompt user for credentials authConfig, err := command.PromptUserForCredentials(ctx, dockerCLI, opts.user, opts.password, defaultUsername, serverAddress) if err != nil { - return "", err + return err } response, err := loginWithRegistry(ctx, dockerCLI.Client(), authConfig) if err != nil { - return "", err + return err } if response.IdentityToken != "" { authConfig.Password = "" authConfig.IdentityToken = response.IdentityToken } - if err = storeCredentials(dockerCLI.ConfigFile(), authConfig); err != nil { - return "", err - } - - return response.Status, nil + return storeCredentials(dockerCLI.ConfigFile(), authConfig) } -func loginWithDeviceCodeFlow(ctx context.Context, dockerCLI command.Cli) (msg string, _ error) { +func loginWithDeviceCodeFlow(ctx context.Context, dockerCLI command.Cli) error { store := dockerCLI.ConfigFile().GetCredentialsStore(registry.IndexServer) authConfig, err := manager.NewManager(store).LoginDevice(ctx, dockerCLI.Err()) if err != nil { - return "", err + return err } - response, err := loginWithRegistry(ctx, dockerCLI.Client(), registrytypes.AuthConfig(*authConfig)) + _, err = loginWithRegistry(ctx, dockerCLI.Client(), registrytypes.AuthConfig(*authConfig)) if err != nil { - return "", err - } - - if err = storeCredentials(dockerCLI.ConfigFile(), registrytypes.AuthConfig(*authConfig)); err != nil { - return "", err + return err } - return response.Status, nil + return storeCredentials(dockerCLI.ConfigFile(), registrytypes.AuthConfig(*authConfig)) } func storeCredentials(cfg *configfile.ConfigFile, authConfig registrytypes.AuthConfig) error { @@ -304,7 +286,6 @@ func loginClientSide(ctx context.Context, auth registrytypes.AuthConfig) (*regis } return ®istrytypes.AuthenticateOKBody{ - Status: "Login Succeeded", IdentityToken: token, }, nil } diff --git a/cli/command/registry/login_test.go b/cli/command/registry/login_test.go index 91450a2c8682..1f1368bcbd2a 100644 --- a/cli/command/registry/login_test.go +++ b/cli/command/registry/login_test.go @@ -75,7 +75,7 @@ func TestLoginWithCredStoreCreds(t *testing.T) { cli := test.NewFakeCli(&fakeClient{}) cli.ConfigFile().Filename = filepath.Join(t.TempDir(), "config.json") for _, tc := range testCases { - _, err := loginWithStoredCredentials(ctx, cli, tc.inputAuthConfig) + err := loginWithStoredCredentials(ctx, cli, tc.inputAuthConfig) if tc.expectedErrMsg != "" { assert.Check(t, is.Error(err, tc.expectedErr)) } else { From 1e99b255f413c239b999ece41a85c1ae62c012a8 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 8 Sep 2025 15:23:23 +0200 Subject: [PATCH 3/3] WIP: return early on error, and don't save Signed-off-by: Sebastiaan van Stijn --- cli/command/registry/login.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cli/command/registry/login.go b/cli/command/registry/login.go index 2474613af8ae..a96e32a2b6e3 100644 --- a/cli/command/registry/login.go +++ b/cli/command/registry/login.go @@ -164,6 +164,7 @@ func loginWithStoredCredentials(ctx context.Context, dockerCLI command.Cli, auth } else { _, _ = fmt.Fprintln(dockerCLI.Err(), "Login did not succeed, error:", err) } + return err } if response.IdentityToken != "" {