fix dao UT issue and refine the error of onboard OIDC user

Signed-off-by: wang yan <wangyan@vmware.com>

src/common/dao/base.go

@@ -36,12 +36,6 @@ const (
 // ErrDupRows is returned by DAO when inserting failed with error "duplicate key value violates unique constraint"
 var ErrDupRows = errors.New("sql: duplicate row in DB")
 
-// ErrRollback is returned by DAO when transaction roll back failed
-var ErrRollback = errors.New("<Ormer.Rollback> transaction roll back error")
-
-// ErrCommit is returned by DAO when transaction commit failed
-var ErrCommit = errors.New("<Ormer.Commit> transaction roll back error")
-
 // Database is an interface of different databases
 type Database interface {
 	// Name returns the name of database

src/common/dao/oidc_user.go

@@ -18,11 +18,25 @@ import (
 	"fmt"
 	"strings"
 	"time"
-	"errors"
 
 	"github.com/astaxie/beego/orm"
 	"github.com/goharbor/harbor/src/common/models"
 	"github.com/goharbor/harbor/src/common/utils/log"
+	"github.com/pkg/errors"
+)
+
+var (
+	// ErrDupUser ...
+	ErrDupUser = errors.New("sql: duplicate user in harbor_user")
+
+	// ErrRollBackUser ...
+	ErrRollBackUser = errors.New("sql: transaction roll back error in harbor_user")
+
+	// ErrDupOIDCUser ...
+	ErrDupOIDCUser = errors.New("sql: duplicate user in oicd_user")
+
+	// ErrRollBackOIDCUser ...
+	ErrRollBackOIDCUser = errors.New("sql: transaction roll back error in oicd_user")
 )
 
 // GetOIDCUserByID ...
@@ -92,6 +106,7 @@ func DeleteOIDCUser(id int64) error {
 }
 
 // OnBoardOIDCUser onboard OIDC user
+// For the api caller, should only care about the ErrDupUser. It could lead to http.StatusConflict.
 func OnBoardOIDCUser(u *models.User) error {
 	if u.OIDCUserMeta == nil {
 		return errors.New("unable to onboard as empty oidc user")
@@ -112,12 +127,13 @@ func OnBoardOIDCUser(u *models.User) error {
 		errInsert = err
 		log.Errorf("fail to insert user, %v", err)
 		if strings.Contains(err.Error(), "duplicate key value violates unique constraint") {
-			errInsert = ErrDupRows
+			errInsert = errors.Wrap(errInsert, ErrDupUser.Error())
 		}
 		err := o.Rollback()
 		if err != nil {
-			log.Errorf("fail to rollback, %v", err)
+			log.Errorf("fail to rollback when to onboard oidc user, %v", err)
-			return ErrRollback
+			errInsert = errors.Wrap(errInsert, err.Error())
+			return errors.Wrap(errInsert, ErrRollBackUser.Error())
 		}
 		return errInsert
 
@@ -131,22 +147,22 @@ func OnBoardOIDCUser(u *models.User) error {
 	_, err = o.Insert(u.OIDCUserMeta)
 	if err != nil {
 		errInsert = err
-		log.Errorf("fail to insert user, %v", err)
+		log.Errorf("fail to insert oidc user, %v", err)
 		if strings.Contains(err.Error(), "duplicate key value violates unique constraint") {
-			errInsert = ErrDupRows
+			errInsert = errors.Wrap(errInsert, ErrDupOIDCUser.Error())
 		}
 		err := o.Rollback()
 		if err != nil {
-			log.Errorf("fail to rollback, %v", err)
+			errInsert = errors.Wrap(errInsert, err.Error())
-			return ErrRollback
+			return errors.Wrap(errInsert, ErrRollBackOIDCUser.Error())
 		}
 		return errInsert
 	}
 	err = o.Commit()
 	if err != nil {
-		log.Errorf("fail to commit, %v", err)
+		log.Errorf("fail to commit when to onboard oidc user, %v", err)
-		return ErrCommit
+		return fmt.Errorf("fail to commit when to onboard oidc user, %v", err)
 	}
 
 	return nil
-}
+}

src/common/dao/oidc_user_test.go

@@ -49,9 +49,11 @@ func TestOIDCUserMetaDaoMethods(t *testing.T) {
 	user111.OIDCUserMeta = &ou111
 	err := OnBoardOIDCUser(&user111)
 	require.Nil(t, err)
+	defer CleanUser(int64(user111.UserID))
 	user222.OIDCUserMeta = &ou222
 	err = OnBoardOIDCUser(&user222)
 	require.Nil(t, err)
+	defer CleanUser(int64(user222.UserID))
 
 	// empty OIDC user meta ...
 	err = OnBoardOIDCUser(&userEmptyOuMeta)
@@ -110,7 +112,7 @@ func TestOIDCOnboard(t *testing.T) {
 	}
 	userDup := models.User{
 		Username: "user333",
-		Email:    "user333@email.com",
+		Email:    "userDup@email.com",
 	}
 
 	ou333 := &models.OIDCUser{
@@ -134,32 +136,46 @@ func TestOIDCOnboard(t *testing.T) {
 	user333.OIDCUserMeta = ou333
 	err := OnBoardOIDCUser(&user333)
 	require.Nil(t, err)
+	defer CleanUser(int64(user333.UserID))
 
 	// duplicate user -- ErrDupRows
 	// userDup is duplicate with user333
 	userDup.OIDCUserMeta = ou555
 	err = OnBoardOIDCUser(&userDup)
 	require.NotNil(t, err)
-	require.Equal(t, err, ErrDupRows)
+	require.Contains(t, err.Error(), ErrDupUser.Error())
+	exist, err := UserExists(userDup, "email")
+	require.Nil(t, err)
+	require.False(t, exist)
 
 	// duplicate OIDC user -- ErrDupRows
 	// ouDup is duplicate with ou333
 	user555.OIDCUserMeta = ouDup
 	err = OnBoardOIDCUser(&user555)
 	require.NotNil(t, err)
-	require.Equal(t, err, ErrDupRows)
+	require.Contains(t, err.Error(), ErrDupOIDCUser.Error())
+	exist, err = UserExists(user555, "username")
+	require.Nil(t, err)
+	require.False(t, exist)
 
 	// success
 	user555.OIDCUserMeta = ou555
 	err = OnBoardOIDCUser(&user555)
 	require.Nil(t, err)
+	exist, err = UserExists(user555, "username")
+	require.Nil(t, err)
+	require.True(t, exist)
+	defer CleanUser(int64(user555.UserID))
 
 	// duplicate OIDC user's sub -- ErrDupRows
 	// ouDup is duplicate with ou333
 	user666.OIDCUserMeta = ouDupSub
 	err = OnBoardOIDCUser(&user666)
 	require.NotNil(t, err)
-	require.Equal(t, err, ErrDupRows)
+	require.Contains(t, err.Error(), ErrDupOIDCUser.Error())
+	exist, err = UserExists(user666, "username")
+	require.Nil(t, err)
+	require.False(t, exist)
 
 	// clear data
 	defer func() {

src/core/controllers/oidc.go

@@ -95,4 +95,4 @@ func (oc *OIDCController) Onboard() {
 	oc.RenderError(http.StatusNotImplemented, "")
 	return
 
-}
+}