refactor: enhancement of login function

Signed-off-by: NDCCooper <1866858@gmail.com>

cmd/cli/login.go

@@ -76,9 +76,16 @@ func NewLoginCmd() *cobra.Command {
 }
 
 func loginCommand(c *cobra.Command, args []string) error {
-	if err := newLoginOptions(c, args); err != nil {
+	if len(args) == 0 {
+		return errEmptyRegistry
+	}
+	if len(args) > 1 {
+		return errTooManyArgs
+	}
+	if err := getRegistry(args); err != nil {
 		return err
 	}
+
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
@@ -86,7 +93,7 @@ func loginCommand(c *cobra.Command, args []string) error {
 	if err != nil {
 		return err
 	}
-	msg, err := runLogin(ctx, cli)
+	msg, err := runLogin(ctx, cli, c)
 	fmt.Println(msg)
 	if err != nil {
 		return err
@@ -94,39 +101,38 @@ func loginCommand(c *cobra.Command, args []string) error {
 	return nil
 }
 
-func runLogin(ctx context.Context, cli Cli) (string, error) {
-	if err := encryptOpts(); err != nil {
+func runLogin(ctx context.Context, cli Cli, c *cobra.Command) (string, error) {
+	req, err := genLoginReq(c, false)
+	if err != nil {
 		return "", err
 	}
-	req := &pb.LoginRequest{
-		Server:   loginOpts.server,
-		Username: loginOpts.username,
-		Password: loginOpts.password,
-		Key:      loginOpts.key,
-	}
 	resp, err := cli.Client().Login(ctx, req)
 	if err != nil {
+		if strings.Contains(err.Error(), "Failed to authenticate existing credentials") {
+			fmt.Printf("Failed to authenticate existing credentials, please input auth info directly\n\n")
+			if err = getAuthInfo(c); err != nil {
+				return "", err
+			}
+			req, err = genLoginReq(c, true)
+			if err != nil {
+				return "", err
+			}
+			resp, err = cli.Client().Login(ctx, req)
+			if err != nil {
+				return loginFailed, err
+			}
+			return resp.Content, err
+		}
 		return loginFailed, err
 	}
+
 	return resp.Content, err
 }
 
-func newLoginOptions(c *cobra.Command, args []string) error {
-	if len(args) == 0 {
-		return errEmptyRegistry
-	}
-	if len(args) > 1 {
-		return errTooManyArgs
-	}
-
-	if err := getRegistry(args); err != nil {
-		return err
-	}
-
+func getAuthInfo(c *cobra.Command) error {
 	if err := getUsername(c); err != nil {
 		return err
 	}
-
 	if err := getPassword(c); err != nil {
 		return err
 	}
@@ -134,6 +140,38 @@ func newLoginOptions(c *cobra.Command, args []string) error {
 	return nil
 }
 
+func genLoginReq(c *cobra.Command, shouldGetAuthInfo bool) (*pb.LoginRequest, error) {
+	// first check auth info from auth.json, so no auth info
+	// should be send from client to server
+	if loginOpts.username == "" && loginOpts.password == "" {
+		fmt.Printf("try to login with existing credentials...\n\n")
+		return &pb.LoginRequest{
+			Server:   loginOpts.server,
+			Username: "",
+			Password: "",
+			Key:      "",
+		}, nil
+	}
+
+	// if shouldGetAuthInfo is false, we don't need to getAuthInfo again
+	// because this action will do in outer place
+	if shouldGetAuthInfo || loginOpts.username != "" {
+		if err := getAuthInfo(c); err != nil {
+			return nil, err
+		}
+	}
+	if err := encryptOpts(); err != nil {
+		return nil, err
+	}
+	return &pb.LoginRequest{
+		Server:   loginOpts.server,
+		Username: loginOpts.username,
+		Password: loginOpts.password,
+		Key:      loginOpts.key,
+	}, nil
+
+}
+
 func getRegistry(args []string) error {
 	server, err := util.ParseServer(args[0])
 	if err != nil {
@@ -144,6 +182,11 @@ func getRegistry(args []string) error {
 }
 
 func getUsername(c *cobra.Command) error {
+	// in this scenario, it is second time trying to get username
+	// if already got it, there is no need to get username again
+	if loginOpts.username != "" {
+		return nil
+	}
 	username, err := c.Flags().GetString("username")
 	if err != nil {
 		return err
@@ -167,6 +210,11 @@ func getUsername(c *cobra.Command) error {
 }
 
 func getPassword(c *cobra.Command) error {
+	// in this scenario, it is second time trying to get password
+	// if already got it, there is no need to get pass again
+	if loginOpts.password != "" {
+		return nil
+	}
 	if c.Flag("password-stdin").Changed && !c.Flag("username").Changed {
 		return errLackOfFlags
 	}

cmd/cli/login_test.go

@@ -17,12 +17,10 @@ import (
 	"bytes"
 	"context"
 	"errors"
-	"fmt"
 	"io"
 	"strings"
 	"testing"
 
-	"github.com/spf13/cobra"
 	"gotest.tools/assert"
 )
 
@@ -33,7 +31,7 @@ func TestNewLoginCmd(t *testing.T) {
 	args := []string{"test.org"}
 	err = loginCommand(loginCmd, args)
 	if err != nil {
-		assert.ErrorContains(t, err, "auth info can not be empty")
+		assert.ErrorContains(t, err, "isula_build.sock")
 	}
 }
 
@@ -120,14 +118,18 @@ func TestRunLogin(t *testing.T) {
 	type testcase struct {
 		name      string
 		server    string
-		errString string
+		username  string
+		password  string
 		wantErr   bool
+		errString string
 	}
 	var testcases = []testcase{
 		{
-			name:    "TC1 - normal case",
-			server:  "test.org",
-			wantErr: false,
+			name:     "TC1 - normal case",
+			server:   "test.org",
+			username: "testUser",
+			password: "testPass",
+			wantErr:  false,
 		},
 		{
 			name:      "TC2 - abnormal case with empty server",
@@ -135,111 +137,30 @@ func TestRunLogin(t *testing.T) {
 			wantErr:   true,
 			errString: "empty server address",
 		},
+		{
+			name:     "TC3 - abnormal case with empty password",
+			server:   "test.org",
+			username: "testUser",
+			password: "",
+			wantErr:  true,
+		},
 	}
 	for _, tc := range testcases {
 		ctx := context.Background()
 		mockD := newMockDaemon()
 		cli := newMockClient(&mockGrpcClient{loginFunc: mockD.login})
 
-		loginOpts.server = tc.server
-		_, err := runLogin(ctx, &cli)
+		c := NewLoginCmd()
+		loginOpts = loginOptions{
+			server:   tc.server,
+			username: tc.username,
+			password: tc.password,
+		}
+
+		_, err := runLogin(ctx, &cli, c)
 		assert.Equal(t, err != nil, tc.wantErr, "Failed at [%s], err: %v", tc.name, err)
 		if err != nil {
 			assert.ErrorContains(t, err, tc.errString)
 		}
 	}
 }
-
-func TestNewLoginOptions(t *testing.T) {
-	type args struct {
-		c    *cobra.Command
-		args []string
-	}
-	type flags struct {
-		username  string
-		passStdin bool
-	}
-	tests := []struct {
-		name      string
-		args      args
-		flags     flags
-		errString string
-	}{
-		{
-			name: "TC1 - normal case",
-			args: args{
-				c:    NewLoginCmd(),
-				args: []string{"test.org -u testuser"},
-			},
-			flags: flags{
-				username:  "aaa",
-				passStdin: true,
-			},
-			errString: "auth info can not be empty",
-		},
-		{
-			name: "TC2 - abnormal case with out username flag",
-			args: args{
-				c:    NewLoginCmd(),
-				args: []string{"test.org"},
-			},
-			flags: flags{
-				passStdin: true,
-			},
-			errString: "",
-		},
-		{
-			name: "TC3 - abnormal case with invalid args",
-			args: args{
-				c:    NewLoginCmd(),
-				args: []string{"a", "b"},
-			},
-			errString: "too many arguments, login only accepts 1 argument",
-		},
-		{
-			name: "TC4 - abnormal case with empty args",
-			args: args{
-				c:    NewLoginCmd(),
-				args: []string{},
-			},
-			errString: "empty registry found",
-		},
-		{
-			name: "TC5 - abnormal case with empty args",
-			args: args{
-				c:    NewLoginCmd(),
-				args: []string{"/aaaa"},
-			},
-			errString: "invalid registry address",
-		},
-		{
-			name: "TC6 - abnormal case with long username",
-			args: args{
-				c:    NewLoginCmd(),
-				args: []string{"test.org"},
-			},
-			flags: flags{
-				username:  strings.Repeat("a", 129),
-				passStdin: true,
-			},
-			errString: "length of input exceeded",
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			if tt.flags.passStdin {
-				tt.args.c.Flag("password-stdin").Changed = true
-				tt.args.c.Flag("password-stdin").Value.Set(fmt.Sprintf("%v", tt.flags.passStdin))
-			}
-			if tt.flags.username != "" {
-				tt.args.c.Flag("username").Changed = true
-				tt.args.c.Flag("username").Value.Set(tt.flags.username)
-			}
-			tt.args.c.ParseFlags(tt.args.args)
-			err := newLoginOptions(tt.args.c, tt.args.args)
-			if err != nil {
-				assert.ErrorContains(t, err, tt.errString)
-			}
-		})
-	}
-}

cmd/cli/mock.go

@@ -291,12 +291,20 @@ func (f *mockDaemon) save(_ context.Context, in *pb.SaveRequest, opts ...grpc.Ca
 
 func (f *mockDaemon) login(_ context.Context, in *pb.LoginRequest, opts ...grpc.CallOption) (*pb.LoginResponse, error) {
 	f.loginReq = in
-	serverLen := len(f.loginReq.Server)
+	username := f.loginReq.Username
+	password := f.loginReq.Password
+	server := f.loginReq.Server
+	serverLen := len(server)
 	if serverLen == 0 || serverLen > 128 {
 		return &pb.LoginResponse{
 			Content: "Login Failed",
 		}, errors.New("empty server address")
 	}
+	if username == "" && password == "" && server != "" {
+		return &pb.LoginResponse{
+			Content: "Failed to authenticate existing credentials",
+		}, errors.New("Failed to authenticate existing credentials")
+	}
 
 	return &pb.LoginResponse{Content: "Success"}, nil
 }

daemon/login.go

@@ -30,14 +30,15 @@ import (
 )
 
 const (
-	loginSuccess       = "Login Succeeded"
-	loginWithAuthFile  = "Login Succeed with AuthFile"
-	loginFailed        = "Login Failed"
-	loginUnauthorized  = "Unauthorized login attempt"
-	loginSetAuthFailed = "Set Auth Failed"
-	emptyKey           = "empty key found"
-	emptyServer        = "empty server address"
-	emptyAuth          = "empty auth info"
+	loginSuccess             = "Login Succeeded"
+	loginFailed              = "Login Failed"
+	loginUnauthorized        = "Unauthorized login attempt"
+	loginSetAuthFailed       = "Set Auth Failed"
+	emptyKey                 = "empty key found"
+	emptyServer              = "empty server address"
+	emptyAuth                = "empty auth info"
+	errTryToUseAuth          = "Failed to authenticate existing credentials, try to use auth directly"
+	loginSuccessWithAuthFile = "Login Succeed with AuthFile"
 )
 
 // Login returns login response
@@ -51,33 +52,35 @@ func (b *Backend) Login(ctx context.Context, req *pb.LoginRequest) (*pb.LoginRes
 		return &pb.LoginResponse{Content: loginFailed}, err
 	}
 
-	password, err := util.DecryptAES(req.Password, req.Key)
-	if err != nil {
-		return &pb.LoginResponse{Content: err.Error()}, err
-	}
-
 	sysCtx := image.GetSystemContext()
 	sysCtx.DockerCertPath = filepath.Join(constant.DefaultCertRoot, req.Server)
 
-	auth, err := config.GetCredentials(sysCtx, req.Server)
-	if err != nil {
-		auth = types.DockerAuthConfig{}
-		return &pb.LoginResponse{Content: err.Error()}, errors.Wrapf(err, "failed to read auth file %v", constant.AuthFilePath)
-	}
+	if loginWithAuthFile(req) {
+		auth, err := config.GetCredentials(sysCtx, req.Server)
+		if err != nil || auth.Password == "" {
+			auth = types.DockerAuthConfig{}
+			return &pb.LoginResponse{Content: errTryToUseAuth}, errors.Errorf("failed to read auth file: %v", errTryToUseAuth)
+		}
 
-	usernameFromAuth, passwordFromAuth := auth.Username, auth.Password
-	// use existing credentials from authFile if any
-	if usernameFromAuth != "" && passwordFromAuth != "" {
-		logrus.Infof("Authenticating with existing credentials")
-		err = docker.CheckAuth(ctx, sysCtx, usernameFromAuth, passwordFromAuth, req.Server)
-		if err == nil {
-			logrus.Infof("Success login server: %s by auth file with username: %s", req.Server, usernameFromAuth)
-			return &pb.LoginResponse{Content: loginWithAuthFile}, err
+		usernameFromAuth, passwordFromAuth := auth.Username, auth.Password
+		// use existing credentials from authFile if any
+		if usernameFromAuth != "" && passwordFromAuth != "" {
+			logrus.Infof("Authenticating with existing credentials")
+			err = docker.CheckAuth(ctx, sysCtx, usernameFromAuth, passwordFromAuth, req.Server)
+			if err == nil {
+				logrus.Infof("Success login server: %s by auth file with username: %s", req.Server, usernameFromAuth)
+				return &pb.LoginResponse{Content: loginSuccessWithAuthFile}, nil
+			}
+			return &pb.LoginResponse{Content: errTryToUseAuth}, errors.Wrap(err, errTryToUseAuth)
 		}
-		logrus.Infof("Failed to authenticate existing credentials, try to use auth directly")
 	}
 
 	// use username and password from client to access
+	password, err := util.DecryptAES(req.Password, req.Key)
+	if err != nil {
+		return &pb.LoginResponse{Content: err.Error()}, err
+	}
+
 	if err = docker.CheckAuth(ctx, sysCtx, req.Username, password, req.Server); err != nil {
 		// check if user is authorized
 		if _, ok := err.(docker.ErrUnauthorizedForCredentials); ok {
@@ -95,7 +98,20 @@ func (b *Backend) Login(ctx context.Context, req *pb.LoginRequest) (*pb.LoginRes
 	return &pb.LoginResponse{Content: loginSuccess}, nil
 }
 
+func loginWithAuthFile(req *pb.LoginRequest) bool {
+	if req.Password == "" && req.Username == "" && req.Server != "" {
+		return true
+	}
+	return false
+}
+
 func validLoginOpts(req *pb.LoginRequest) error {
+	// in this scenario, the client just send server address to backend,
+	// there is no pass and user name info sent to here.
+	// we just check if there is valid auth info in auth.json later.
+	if req.Password == "" && req.Username == "" && req.Server != "" {
+		return nil
+	}
 	if req.Key == "" {
 		return errors.New(emptyKey)
 	}