From dcf1d704e684248e4cc624471193fa68694a90cd Mon Sep 17 00:00:00 2001 From: wang yan Date: Wed, 3 Apr 2019 09:38:41 +0800 Subject: [PATCH] fix dao UT issue and refine the error of onboard OIDC user Signed-off-by: wang yan --- src/common/dao/base.go | 6 ----- src/common/dao/oidc_user.go | 38 +++++++++++++++++++++++--------- src/common/dao/oidc_user_test.go | 24 ++++++++++++++++---- src/core/controllers/oidc.go | 2 +- 4 files changed, 48 insertions(+), 22 deletions(-) diff --git a/src/common/dao/base.go b/src/common/dao/base.go index dc5608334..3e04867da 100644 --- a/src/common/dao/base.go +++ b/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(" transaction roll back error") - -// ErrCommit is returned by DAO when transaction commit failed -var ErrCommit = errors.New(" transaction roll back error") - // Database is an interface of different databases type Database interface { // Name returns the name of database diff --git a/src/common/dao/oidc_user.go b/src/common/dao/oidc_user.go index aa26296f6..6045cff36 100644 --- a/src/common/dao/oidc_user.go +++ b/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) - return ErrRollback + log.Errorf("fail to rollback when to onboard oidc user, %v", err) + 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) - return ErrRollback + errInsert = errors.Wrap(errInsert, err.Error()) + return errors.Wrap(errInsert, ErrRollBackOIDCUser.Error()) } return errInsert } err = o.Commit() if err != nil { - log.Errorf("fail to commit, %v", err) - return ErrCommit + log.Errorf("fail to commit when to onboard oidc user, %v", err) + return fmt.Errorf("fail to commit when to onboard oidc user, %v", err) } return nil -} \ No newline at end of file +} diff --git a/src/common/dao/oidc_user_test.go b/src/common/dao/oidc_user_test.go index 0da3e1a8c..7ec303c9c 100644 --- a/src/common/dao/oidc_user_test.go +++ b/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() { diff --git a/src/core/controllers/oidc.go b/src/core/controllers/oidc.go index 653240703..7ba18580d 100644 --- a/src/core/controllers/oidc.go +++ b/src/core/controllers/oidc.go @@ -95,4 +95,4 @@ func (oc *OIDCController) Onboard() { oc.RenderError(http.StatusNotImplemented, "") return -} \ No newline at end of file +}