Refactor cli/command/registry

Signed-off-by: Laura Brehm <[email protected]>

cli/command/registry.go

@@ -29,8 +29,7 @@ func RegistryAuthenticationPrivilegedFunc(cli Cli, index *registrytypes.IndexInf
 	return func(ctx context.Context) (string, error) {
 		fmt.Fprintf(cli.Out(), "\nLogin prior to %s:\n", cmdName)
 		indexServer := registry.GetAuthConfigKey(index)
-		isDefaultRegistry := indexServer == registry.IndexServer
-		authConfig, err := GetDefaultAuthConfig(cli.ConfigFile(), true, indexServer, isDefaultRegistry)
+		authConfig, err := GetDefaultAuthConfig(cli.ConfigFile(), true, indexServer)
 		if err != nil {
 			fmt.Fprintf(cli.Err(), "Unable to retrieve stored credentials for %s, error: %s.\n", indexServer, err)
 		}
@@ -41,7 +40,7 @@ func RegistryAuthenticationPrivilegedFunc(cli Cli, index *registrytypes.IndexInf
 		default:
 		}
 
-		err = ConfigureAuth(ctx, cli, "", "", &authConfig, isDefaultRegistry)
+		authConfig, err = ConfigureAuth(ctx, cli, "", "", authConfig.Username, indexServer)
 		if err != nil {
 			return "", err
 		}
@@ -67,8 +66,8 @@ func ResolveAuthConfig(cfg *configfile.ConfigFile, index *registrytypes.IndexInf
 
 // GetDefaultAuthConfig gets the default auth config given a serverAddress
 // If credentials for given serverAddress exists in the credential store, the configuration will be populated with values in it
-func GetDefaultAuthConfig(cfg *configfile.ConfigFile, checkCredStore bool, serverAddress string, isDefaultRegistry bool) (registrytypes.AuthConfig, error) {
-	if !isDefaultRegistry {
+func GetDefaultAuthConfig(cfg *configfile.ConfigFile, checkCredStore bool, serverAddress string) (registrytypes.AuthConfig, error) {
+	if serverAddress != registry.IndexServer {
 		serverAddress = credentials.ConvertToHostname(serverAddress)
 	}
 	authconfig := configtypes.AuthConfig{}
@@ -87,7 +86,7 @@ func GetDefaultAuthConfig(cfg *configfile.ConfigFile, checkCredStore bool, serve
 }
 
 // ConfigureAuth handles prompting of user's username and password if needed
-func ConfigureAuth(ctx context.Context, cli Cli, flUser, flPassword string, authconfig *registrytypes.AuthConfig, isDefaultRegistry bool) error {
+func ConfigureAuth(ctx context.Context, cli Cli, flUser, flPassword, defaultUsername, serverAddress string) (authConfig registrytypes.AuthConfig, err error) {
 	// On Windows, force the use of the regular OS stdin stream.
 	//
 	// See:
@@ -108,10 +107,11 @@ func ConfigureAuth(ctx context.Context, cli Cli, flUser, flPassword string, auth
 	// will hit this if you attempt docker login from mintty where stdin
 	// is a pipe, not a character based console.
 	if flPassword == "" && !cli.In().IsTerminal() {
-		return errors.Errorf("Error: Cannot perform an interactive login from a non TTY device")
+		return authConfig, errors.Errorf("Error: Cannot perform an interactive login from a non TTY device")
 	}
 
-	authconfig.Username = strings.TrimSpace(authconfig.Username)
+	isDefaultRegistry := serverAddress == registry.IndexServer
+	defaultUsername = strings.TrimSpace(defaultUsername)
 
 	if flUser = strings.TrimSpace(flUser); flUser == "" {
 		if isDefaultRegistry {
@@ -124,44 +124,43 @@ func ConfigureAuth(ctx context.Context, cli Cli, flUser, flPassword string, auth
 		}
 
 		var prompt string
-		if authconfig.Username == "" {
+		if defaultUsername == "" {
 			prompt = "Username: "
 		} else {
-			prompt = fmt.Sprintf("Username (%s): ", authconfig.Username)
+			prompt = fmt.Sprintf("Username (%s): ", defaultUsername)
 		}
-		var err error
 		flUser, err = PromptForInput(ctx, cli.In(), cli.Out(), prompt)
 		if err != nil {
-			return err
+			return authConfig, err
 		}
 		if flUser == "" {
-			flUser = authconfig.Username
+			flUser = defaultUsername
 		}
 	}
 	if flUser == "" {
-		return errors.Errorf("Error: Non-null Username Required")
+		return authConfig, errors.Errorf("Error: Non-null Username Required")
 	}
 	if flPassword == "" {
 		restoreInput, err := DisableInputEcho(cli.In())
 		if err != nil {
-			return err
+			return authConfig, err
 		}
 		defer restoreInput()
 
 		flPassword, err = PromptForInput(ctx, cli.In(), cli.Out(), "Password: ")
 		if err != nil {
-			return err
+			return authConfig, err
 		}
 		fmt.Fprint(cli.Out(), "\n")
 		if flPassword == "" {
-			return errors.Errorf("Error: Password Required")
+			return authConfig, errors.Errorf("Error: Password Required")
 		}
 	}
 
-	authconfig.Username = flUser
-	authconfig.Password = flPassword
-
-	return nil
+	authConfig.Username = flUser
+	authConfig.Password = flPassword
+	authConfig.ServerAddress = serverAddress
+	return authConfig, nil
 }
 
 // RetrieveAuthTokenFromImage retrieves an encoded auth token given a complete

cli/command/registry/login.go

@@ -100,83 +100,137 @@ func verifyloginOptions(dockerCli command.Cli, opts *loginOptions) error {
 	return nil
 }
 
-func runLogin(ctx context.Context, dockerCli command.Cli, opts loginOptions) error { //nolint:gocyclo
-	clnt := dockerCli.Client()
+func runLogin(ctx context.Context, dockerCli command.Cli, opts loginOptions) error {
 	if err := verifyloginOptions(dockerCli, &opts); err != nil {
 		return err
 	}
 	var (
 		serverAddress string
-		response      registrytypes.AuthenticateOKBody
+		response      *registrytypes.AuthenticateOKBody
 	)
 	if opts.serverAddress != "" && opts.serverAddress != registry.DefaultNamespace {
 		serverAddress = opts.serverAddress
 	} else {
 		serverAddress = registry.IndexServer
 	}
 
-	isDefaultRegistry := serverAddress == registry.IndexServer
-	authConfig, err := command.GetDefaultAuthConfig(dockerCli.ConfigFile(), opts.user == "" && opts.password == "", serverAddress, isDefaultRegistry)
+	// attempt login with current (stored) credentials
+	authConfig, err := command.GetDefaultAuthConfig(dockerCli.ConfigFile(), opts.user == "" && opts.password == "", serverAddress)
 	if err == nil && authConfig.Username != "" && authConfig.Password != "" {
-		response, err = loginWithCredStoreCreds(ctx, dockerCli, &authConfig)
+		response, 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 == "" {
-		if isDefaultRegistry && opts.user == "" && opts.password == "" {
-			// todo(laurazard: clean this up
-			tokenRes, err := dockerCli.OAuthManager().LoginDevice(ctx, dockerCli.Err())
-			if err != nil {
-				return err
-			}
-			authConfig.Username = tokenRes.Claims.Domain.Username
-			authConfig.Password = tokenRes.AccessToken
-			authConfig.Email = tokenRes.Claims.Domain.Email
-			authConfig.ServerAddress = serverAddress
-
-			response, err = clnt.RegistryLogin(ctx, authConfig)
-			if err != nil && client.IsErrConnectionFailed(err) {
-				// If the server isn't responding (yet) attempt to login purely client side
-				response, err = loginClientSide(ctx, authConfig)
-			}
-			// If we (still) have an error, give up
-			if err != nil {
-				return err
-			}
+		response, err = loginUser(ctx, dockerCli, opts, authConfig.Username, serverAddress)
+		if err != nil {
+			return err
+		}
+	}
 
-			authConfig.Password = authConfig.Password + ".." + tokenRes.RefreshToken
+	if response.Status != "" {
+		_, _ = fmt.Fprintln(dockerCli.Out(), response.Status)
+	}
+	return nil
+}
 
-			creds := dockerCli.ConfigFile().GetCredentialsStore(serverAddress)
-			if err := creds.Store(configtypes.AuthConfig(authConfig)); err != nil {
-				return errors.Errorf("Error saving credentials: %v", err)
-			}
-			return nil
+func loginWithStoredCredentials(ctx context.Context, dockerCli command.Cli, authConfig registrytypes.AuthConfig) (*registrytypes.AuthenticateOKBody, error) {
+	_, _ = fmt.Fprintf(dockerCli.Out(), "Authenticating with existing credentials...\n")
+	response, err := dockerCli.Client().RegistryLogin(ctx, authConfig)
+	if err != nil {
+		if errdefs.IsUnauthorized(err) {
+			_, _ = fmt.Fprintf(dockerCli.Err(), "Stored credentials invalid or expired\n")
 		} else {
-			err = command.ConfigureAuth(ctx, dockerCli, opts.user, opts.password, &authConfig, isDefaultRegistry)
-			if err != nil {
-				return err
-			}
+			_, _ = fmt.Fprintf(dockerCli.Err(), "Login did not succeed, error: %s\n", err)
 		}
+	}
 
-		response, err = clnt.RegistryLogin(ctx, authConfig)
-		if err != nil && client.IsErrConnectionFailed(err) {
-			// If the server isn't responding (yet) attempt to login purely client side
-			response, err = loginClientSide(ctx, authConfig)
-		}
-		// If we (still) have an error, give up
-		if err != nil {
-			return err
-		}
+	if response.IdentityToken != "" {
+		authConfig.Password = ""
+		authConfig.IdentityToken = response.IdentityToken
+	}
+
+	if err := storeCredentials(dockerCli, authConfig); err != nil {
+		return nil, err
+	}
+
+	return &response, err
+}
+
+func loginUser(ctx context.Context, dockerCli command.Cli, opts loginOptions, defaultUsername, serverAddress string) (*registrytypes.AuthenticateOKBody, error) {
+	// 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 == "" {
+		return loginWithDeviceCodeFlow(ctx, dockerCli)
+	} else {
+		return loginWithUsernameAndPassword(ctx, dockerCli, opts, defaultUsername, serverAddress)
 	}
+}
+
+func loginWithUsernameAndPassword(ctx context.Context, dockerCli command.Cli, opts loginOptions, defaultUsername, serverAddress string) (*registrytypes.AuthenticateOKBody, error) {
+	// Prompt user for credentials
+	authConfig, err := command.ConfigureAuth(ctx, dockerCli, opts.user, opts.password, defaultUsername, serverAddress)
+	if err != nil {
+		return nil, err
+	}
+
+	response, err := loginWithRegistry(ctx, dockerCli, authConfig)
+	if err != nil {
+		return nil, err
+	}
+
 	if response.IdentityToken != "" {
 		authConfig.Password = ""
 		authConfig.IdentityToken = response.IdentityToken
 	}
+	if err = storeCredentials(dockerCli, authConfig); err != nil {
+		return nil, err
+	}
+
+	return &response, nil
+}
+
+func loginWithDeviceCodeFlow(ctx context.Context, dockerCli command.Cli) (*registrytypes.AuthenticateOKBody, error) {
+	authConfig, refreshToken, err := getOAuthCredentials(ctx, dockerCli)
+	if err != nil {
+		return nil, err
+	}
+
+	response, err := loginWithRegistry(ctx, dockerCli, authConfig)
+	if err != nil {
+		return nil, err
+	}
+
+	// todo(laurazard): move this somewhere else
+	authConfig.Password = authConfig.Password + ".." + refreshToken
+	if err = storeCredentials(dockerCli, authConfig); err != nil {
+		return nil, err
+	}
+
+	return &response, nil
+}
+
+func getOAuthCredentials(ctx context.Context, dockerCli command.Cli) (authConfig registrytypes.AuthConfig, refreshToken string, err error) {
+	tokenRes, err := dockerCli.OAuthManager().LoginDevice(ctx, dockerCli.Err())
+	if err != nil {
+		return authConfig, "", err
+	}
+
+	return registrytypes.AuthConfig{
+		Username:      tokenRes.Claims.Domain.Username,
+		Password:      tokenRes.AccessToken,
+		Email:         tokenRes.Claims.Domain.Email,
+		ServerAddress: registry.IndexServer,
+	}, tokenRes.RefreshToken, nil
+}
 
-	creds := dockerCli.ConfigFile().GetCredentialsStore(serverAddress)
+func storeCredentials(dockerCli command.Cli, authConfig registrytypes.AuthConfig) error {
+	creds := dockerCli.ConfigFile().GetCredentialsStore(authConfig.ServerAddress)
 	// todo(laurazard): this will no longer trigger even when the store is a file store
 	store, isDefault := creds.(isFileStore)
 	// Display a warning if we're storing the users password (not a token)
 	if isDefault && authConfig.Password != "" {
-		err = displayUnencryptedWarning(dockerCli, store.GetFilename())
+		err := displayUnencryptedWarning(dockerCli, store.GetFilename())
 		if err != nil {
 			return err
 		}
@@ -186,24 +240,21 @@ func runLogin(ctx context.Context, dockerCli command.Cli, opts loginOptions) err
 		return errors.Errorf("Error saving credentials: %v", err)
 	}
 
-	if response.Status != "" {
-		fmt.Fprintln(dockerCli.Out(), response.Status)
-	}
 	return nil
 }
 
-func loginWithCredStoreCreds(ctx context.Context, dockerCli command.Cli, authConfig *registrytypes.AuthConfig) (registrytypes.AuthenticateOKBody, error) {
-	fmt.Fprintf(dockerCli.Out(), "Authenticating with existing credentials...\n")
-	cliClient := dockerCli.Client()
-	response, err := cliClient.RegistryLogin(ctx, *authConfig)
+func loginWithRegistry(ctx context.Context, dockerCli command.Cli, authConfig registrytypes.AuthConfig) (registrytypes.AuthenticateOKBody, error) {
+	response, err := dockerCli.Client().RegistryLogin(ctx, authConfig)
+	if err != nil && client.IsErrConnectionFailed(err) {
+		// If the server isn't responding (yet) attempt to login purely client side
+		response, err = loginClientSide(ctx, authConfig)
+	}
+	// If we (still) have an error, give up
 	if err != nil {
-		if errdefs.IsUnauthorized(err) {
-			fmt.Fprintf(dockerCli.Err(), "Stored credentials invalid or expired\n")
-		} else {
-			fmt.Fprintf(dockerCli.Err(), "Login did not succeed, error: %s\n", err)
-		}
+		return registrytypes.AuthenticateOKBody{}, err
 	}
-	return response, err
+
+	return response, nil
 }
 
 func loginClientSide(ctx context.Context, auth registrytypes.AuthConfig) (registrytypes.AuthenticateOKBody, error) {

cli/command/registry/login_test.go

@@ -74,7 +74,7 @@ func TestLoginWithCredStoreCreds(t *testing.T) {
 		cli := test.NewFakeCli(&fakeClient{})
 		errBuf := new(bytes.Buffer)
 		cli.SetErr(streams.NewOut(errBuf))
-		loginWithCredStoreCreds(ctx, cli, &tc.inputAuthConfig)
+		loginWithStoredCredentials(ctx, cli, tc.inputAuthConfig)
 		outputString := cli.OutBuffer().String()
 		assert.Check(t, is.Equal(tc.expectedMsg, outputString))
 		errorString := errBuf.String()

cli/command/registry_test.go

@@ -61,7 +61,7 @@ func TestGetDefaultAuthConfig(t *testing.T) {
 	}
 	for _, tc := range testCases {
 		serverAddress := tc.inputServerAddress
-		authconfig, err := command.GetDefaultAuthConfig(cfg, tc.checkCredStore, serverAddress, serverAddress == "https://index.docker.io/v1/")
+		authconfig, err := command.GetDefaultAuthConfig(cfg, tc.checkCredStore, serverAddress)
 		assert.NilError(t, err)
 		assert.Check(t, is.DeepEqual(tc.expectedAuthConfig, authconfig))
 	}
@@ -75,8 +75,7 @@ func TestGetDefaultAuthConfig_HelperError(t *testing.T) {
 	expectedAuthConfig := registry.AuthConfig{
 		ServerAddress: serverAddress,
 	}
-	const isDefaultRegistry = false // registry is not "https://index.docker.io/v1/"
-	authconfig, err := command.GetDefaultAuthConfig(cfg, true, serverAddress, isDefaultRegistry)
+	authconfig, err := command.GetDefaultAuthConfig(cfg, true, serverAddress)
 	assert.Check(t, is.DeepEqual(expectedAuthConfig, authconfig))
 	assert.Check(t, is.ErrorContains(err, "docker-credential-fake-does-not-exist"))
 }

cli/config/credentials/oauth_store.go

@@ -49,6 +49,7 @@ func (c *oauthStore) Get(serverAddress string) (types.AuthConfig, error) {
 	tokenRes, err := c.parseToken(auth.Password)
 	// if the password is not a token, return the auth config as is
 	if err != nil {
+		//nolint:nilerr
 		return auth, nil
 	}
 

cli/config/credentials/oauth_store_test.go

@@ -337,6 +337,7 @@ func TestStore(t *testing.T) {
 			}
 			err := s.Store(auth)
 			assert.NilError(t, err)
+
 			assert.Check(t, is.Len(f.GetAuthConfigs(), 1))
 		})
 
@@ -353,7 +354,14 @@ func TestStore(t *testing.T) {
 			}
 			err := s.Store(auth)
 			assert.NilError(t, err)
-			assert.Check(t, is.Len(f.GetAuthConfigs(), 0))
+
+			assert.Check(t, is.Len(f.GetAuthConfigs(), 1))
+			assert.DeepEqual(t, f.GetAuthConfigs()[registry.IndexServer], types.AuthConfig{
+				Username:      "foo",
+				Password:      validNotExpiredToken + "..refresh-token",
+				Email:         "[email protected]",
+				ServerAddress: registry.IndexServer,
+			})
 		})
 	})