From 66766a8f6995f551a0dce87239dfa1ca33db7b5b Mon Sep 17 00:00:00 2001 From: Daniel Jiang Date: Wed, 19 May 2021 22:39:00 +0800 Subject: [PATCH] Remove the onboard and update funcs for OIDC user from common/dao Signed-off-by: Daniel Jiang --- src/common/dao/oidc_user.go | 109 ---------------- src/common/dao/oidc_user_test.go | 152 ---------------------- src/controller/user/controller.go | 36 +++++ src/core/controllers/oidc.go | 34 ++--- src/pkg/oidc/metamanager.go | 6 + src/testing/controller/user/controller.go | 35 +++++ src/testing/pkg/oidc/dao/meta_dao.go | 2 +- src/testing/pkg/oidc/meta_manager.go | 117 +++++++++++++++++ src/testing/pkg/pkg.go | 3 +- 9 files changed, 211 insertions(+), 283 deletions(-) delete mode 100644 src/common/dao/oidc_user.go delete mode 100644 src/common/dao/oidc_user_test.go create mode 100644 src/testing/pkg/oidc/meta_manager.go diff --git a/src/common/dao/oidc_user.go b/src/common/dao/oidc_user.go deleted file mode 100644 index fb8c0c2a9..000000000 --- a/src/common/dao/oidc_user.go +++ /dev/null @@ -1,109 +0,0 @@ -// Copyright Project Harbor Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package dao - -import ( - "fmt" - "strings" - "time" - - "github.com/astaxie/beego/orm" - "github.com/goharbor/harbor/src/common/models" - "github.com/goharbor/harbor/src/lib/errors" - "github.com/goharbor/harbor/src/lib/log" -) - -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") -) - -// UpdateOIDCUser updates the OIDCUser based on the input parm, only the column "secret" and "token" can be updated -func UpdateOIDCUser(oidcUser *models.OIDCUser) error { - cols := []string{"secret", "token"} - _, err := GetOrmer().Update(oidcUser, cols...) - return err -} - -// 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") - } - - o := orm.NewOrm() - err := o.Begin() - if err != nil { - return err - } - var errInsert error - - // insert user - now := time.Now() - u.CreationTime = now - userID, err := o.Insert(u) - if err != nil { - errInsert = err - log.Errorf("fail to insert user, %v", err) - if strings.Contains(err.Error(), "duplicate key value violates unique constraint") { - errInsert = errors.Wrap(errInsert, ErrDupUser.Error()) - } - err := o.Rollback() - if err != nil { - 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 - - } - u.UserID = int(userID) - u.OIDCUserMeta.UserID = int(userID) - - // insert oidc user - now = time.Now() - u.OIDCUserMeta.CreationTime = now - _, err = o.Insert(u.OIDCUserMeta) - if err != nil { - errInsert = err - log.Errorf("fail to insert oidc user, %v", err) - if strings.Contains(err.Error(), "duplicate key value violates unique constraint") { - errInsert = errors.Wrap(errInsert, ErrDupOIDCUser.Error()) - } - err := o.Rollback() - if err != nil { - 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 when to onboard oidc user, %v", err) - return fmt.Errorf("fail to commit when to onboard oidc user, %v", err) - } - - return nil -} diff --git a/src/common/dao/oidc_user_test.go b/src/common/dao/oidc_user_test.go deleted file mode 100644 index 48643d571..000000000 --- a/src/common/dao/oidc_user_test.go +++ /dev/null @@ -1,152 +0,0 @@ -// Copyright Project Harbor Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package dao - -import ( - "testing" - - "github.com/goharbor/harbor/src/common/models" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestOIDCUserMetaDaoMethods(t *testing.T) { - - user111 := models.User{ - Username: "user111", - Email: "user111@email.com", - } - user222 := models.User{ - Username: "user222", - Email: "user222@email.com", - } - userEmptyOuMeta := models.User{ - Username: "userEmptyOuMeta", - Email: "userEmptyOuMeta@email.com", - } - ou111 := models.OIDCUser{ - SubIss: "QWE123123RT1", - Secret: "QWEQWE1", - } - ou222 := models.OIDCUser{ - SubIss: "QWE123123RT2", - Secret: "QWEQWE2", - } - - // onboard OIDC ... - 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) - require.NotNil(t, err) - assert.Equal(t, "unable to onboard as empty oidc user", err.Error()) - - // test update - meta3 := &models.OIDCUser{ - ID: ou111.ID, - UserID: ou111.UserID, - SubIss: "newSub", - Secret: "newSecret", - } - require.Nil(t, UpdateOIDCUser(meta3)) - // clear data - defer func() { - _, err := GetOrmer().Raw(`delete from oidc_user`).Exec() - require.Nil(t, err) - }() -} - -func TestOIDCOnboard(t *testing.T) { - user333 := models.User{ - Username: "user333", - Email: "user333@email.com", - } - user555 := models.User{ - Username: "user555", - Email: "user555@email.com", - } - user666 := models.User{ - Username: "user666", - Email: "user666@email.com", - } - userDup := models.User{ - Username: "user333", - Email: "userDup@email.com", - } - - ou333 := &models.OIDCUser{ - SubIss: "QWE123123RT3", - Secret: "QWEQWE333", - } - ou555 := &models.OIDCUser{ - SubIss: "QWE123123RT5", - Secret: "QWEQWE555", - } - ouDup := &models.OIDCUser{ - SubIss: "QWE123123RT3", - Secret: "QWEQWE333", - } - ouDupSub := &models.OIDCUser{ - SubIss: "QWE123123RT3", - Secret: "ouDupSub", - } - - // data prepare ... - 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.Contains(t, err.Error(), ErrDupUser.Error()) - - // duplicate OIDC user -- ErrDupRows - // ouDup is duplicate with ou333 - user555.OIDCUserMeta = ouDup - err = OnBoardOIDCUser(&user555) - require.NotNil(t, err) - require.Contains(t, err.Error(), ErrDupOIDCUser.Error()) - - // success - user555.OIDCUserMeta = ou555 - err = OnBoardOIDCUser(&user555) - require.Nil(t, err) - 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.Contains(t, err.Error(), ErrDupOIDCUser.Error()) - - // clear data - defer func() { - _, err := GetOrmer().Raw(`delete from oidc_user`).Exec() - require.Nil(t, err) - }() - -} diff --git a/src/controller/user/controller.go b/src/controller/user/controller.go index 1b3576b76..cd688520b 100644 --- a/src/controller/user/controller.go +++ b/src/controller/user/controller.go @@ -18,6 +18,7 @@ import ( "context" "fmt" + commonmodels "github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/common/security" "github.com/goharbor/harbor/src/common/security/local" "github.com/goharbor/harbor/src/lib/errors" @@ -59,6 +60,11 @@ type Controller interface { UpdateProfile(ctx context.Context, u *models.User, cols ...string) error // SetCliSecret sets the OIDC CLI secret for a user SetCliSecret(ctx context.Context, id int, secret string) error + // UpdateOIDCMeta updates the OIDC metadata of a user, if the cols are not provided, by default the field of token and secret will be updated + UpdateOIDCMeta(ctx context.Context, ou *commonmodels.OIDCUser, cols ...string) error + // OnboardOIDCUser inserts the record for basic user info and the oidc metadata + // if the onboard process is successful the input parm of user model will be populated with user id + OnboardOIDCUser(ctx context.Context, u *models.User) error } // NewController ... @@ -79,6 +85,36 @@ type controller struct { oidcMetaMgr oidc.MetaManager } +func (c *controller) UpdateOIDCMeta(ctx context.Context, ou *commonmodels.OIDCUser, cols ...string) error { + defaultCols := []string{"secret", "token"} + if cols == nil || len(cols) == 0 { + cols = defaultCols + } + return c.oidcMetaMgr.Update(ctx, ou, cols...) +} + +func (c *controller) OnboardOIDCUser(ctx context.Context, u *models.User) error { + if u == nil { + return errors.BadRequestError(nil).WithMessage("user model is nil") + } + if u.OIDCUserMeta == nil { + return errors.BadRequestError(nil).WithMessage("OIDC meta of the user model is empty") + } + uid, err := c.mgr.Create(ctx, u) + if err != nil { + return errors.Wrap(err, "failed to create user record") + } + u.UserID = uid + u.OIDCUserMeta.UserID = uid + + mid, err2 := c.oidcMetaMgr.Create(ctx, u.OIDCUserMeta) + if err2 != nil { + return errors.Wrap(err2, "failed to create OIDC metadata record") + } + u.OIDCUserMeta.ID = int64(mid) + return nil +} + func (c *controller) GetBySubIss(ctx context.Context, sub, iss string) (*models.User, error) { oidcMeta, err := c.oidcMetaMgr.GetBySubIss(ctx, sub, iss) if err != nil { diff --git a/src/core/controllers/oidc.go b/src/core/controllers/oidc.go index 6e072900d..ac3f863f9 100644 --- a/src/core/controllers/oidc.go +++ b/src/core/controllers/oidc.go @@ -15,16 +15,16 @@ package controllers import ( + "context" "encoding/json" "fmt" "net/http" "strings" "github.com/goharbor/harbor/src/common" - "github.com/goharbor/harbor/src/common/dao" "github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/common/utils" - "github.com/goharbor/harbor/src/controller/user" + ctluser "github.com/goharbor/harbor/src/controller/user" "github.com/goharbor/harbor/src/core/api" "github.com/goharbor/harbor/src/lib/config" "github.com/goharbor/harbor/src/lib/errors" @@ -117,7 +117,7 @@ func (oc *OIDCController) Callback() { return } oc.SetSession(tokenKey, tokenBytes) - u, err := user.Ctl.GetBySubIss(ctx, info.Subject, info.Issuer) + u, err := ctluser.Ctl.GetBySubIss(ctx, info.Subject, info.Issuer) if errors.IsNotFoundErr(err) { // User is not onboarded, kickoff the onboard flow // Recover the username from d.Username by default username := info.Username @@ -136,7 +136,7 @@ func (oc *OIDCController) Callback() { oidcSettings.UserClaim)) return } - userRec, onboarded := userOnboard(oc, info, username, tokenBytes) + userRec, onboarded := userOnboard(ctx, oc, info, username, tokenBytes) if onboarded == false { log.Error("User not onboarded\n") return @@ -150,27 +150,27 @@ func (oc *OIDCController) Callback() { return } } else if err != nil { - oc.SendInternalServerError(err) + oc.SendError(err) return } oidc.InjectGroupsToUser(info, u) - um, err := user.Ctl.Get(ctx, u.UserID, &user.Option{WithOIDCInfo: true}) + um, err := ctluser.Ctl.Get(ctx, u.UserID, &ctluser.Option{WithOIDCInfo: true}) if err != nil { - oc.SendInternalServerError(err) + oc.SendError(err) return } _, t, err := secretAndToken(tokenBytes) oidcUser := um.OIDCUserMeta oidcUser.Token = t - if err := dao.UpdateOIDCUser(oidcUser); err != nil { - oc.SendInternalServerError(err) + if err := ctluser.Ctl.UpdateOIDCMeta(ctx, oidcUser); err != nil { + oc.SendError(err) return } oc.PopulateUserSession(*u) oc.Controller.Redirect("/", http.StatusFound) } -func userOnboard(oc *OIDCController, info *oidc.UserInfo, username string, tokenBytes []byte) (*models.User, bool) { +func userOnboard(ctx context.Context, oc *OIDCController, info *oidc.UserInfo, username string, tokenBytes []byte) (*models.User, bool) { s, t, err := secretAndToken(tokenBytes) if err != nil { oc.SendInternalServerError(err) @@ -193,17 +193,11 @@ func userOnboard(oc *OIDCController, info *oidc.UserInfo, username string, token log.Debugf("User created: %+v\n", *user) - err = dao.OnBoardOIDCUser(user) + err = ctluser.Ctl.OnboardOIDCUser(ctx, user) if err != nil { - if strings.Contains(err.Error(), dao.ErrDupUser.Error()) { - oc.RenderError(http.StatusConflict, "Conflict, the user with same username or email has been onboarded.") - return nil, false - } - - oc.SendInternalServerError(err) + oc.SendError(err) return nil, false } - return user, true } @@ -242,8 +236,8 @@ func (oc *OIDCController) Onboard() { oc.SendInternalServerError(err) return } - - if user, onboarded := userOnboard(oc, d, username, tb); onboarded { + ctx := oc.Ctx.Request.Context() + if user, onboarded := userOnboard(ctx, oc, d, username, tb); onboarded { user.OIDCUserMeta = nil oc.DelSession(userInfoKey) oc.PopulateUserSession(*user) diff --git a/src/pkg/oidc/metamanager.go b/src/pkg/oidc/metamanager.go index d6925a1cd..2109484d6 100644 --- a/src/pkg/oidc/metamanager.go +++ b/src/pkg/oidc/metamanager.go @@ -35,12 +35,18 @@ type MetaManager interface { GetBySubIss(ctx context.Context, sub, iss string) (*models.OIDCUser, error) // SetCliSecretByUserID updates the cli secret of a user based on the user ID SetCliSecretByUserID(ctx context.Context, uid int, secret string) error + // Update provides a general method for updating the data record for OIDC metadata + Update(ctx context.Context, oidcUser *models.OIDCUser, cols ...string) error } type metaManager struct { dao dao.MetaDAO } +func (m *metaManager) Update(ctx context.Context, oidcUser *models.OIDCUser, cols ...string) error { + return m.dao.Update(ctx, oidcUser, cols...) +} + func (m *metaManager) GetBySubIss(ctx context.Context, sub, iss string) (*models.OIDCUser, error) { logger := log.GetLogger(ctx) l, err := m.dao.List(ctx, q.New(q.KeyWords{"subiss": sub + iss})) diff --git a/src/testing/controller/user/controller.go b/src/testing/controller/user/controller.go index 510dd678d..2be1f9354 100644 --- a/src/testing/controller/user/controller.go +++ b/src/testing/controller/user/controller.go @@ -166,6 +166,20 @@ func (_m *Controller) List(ctx context.Context, query *q.Query) ([]*models.User, return r0, r1 } +// OnboardOIDCUser provides a mock function with given fields: ctx, u +func (_m *Controller) OnboardOIDCUser(ctx context.Context, u *models.User) error { + ret := _m.Called(ctx, u) + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, *models.User) error); ok { + r0 = rf(ctx, u) + } else { + r0 = ret.Error(0) + } + + return r0 +} + // SetCliSecret provides a mock function with given fields: ctx, id, secret func (_m *Controller) SetCliSecret(ctx context.Context, id int, secret string) error { ret := _m.Called(ctx, id, secret) @@ -194,6 +208,27 @@ func (_m *Controller) SetSysAdmin(ctx context.Context, id int, adminFlag bool) e return r0 } +// UpdateOIDCMeta provides a mock function with given fields: ctx, ou, cols +func (_m *Controller) UpdateOIDCMeta(ctx context.Context, ou *models.OIDCUser, cols ...string) error { + _va := make([]interface{}, len(cols)) + for _i := range cols { + _va[_i] = cols[_i] + } + var _ca []interface{} + _ca = append(_ca, ctx, ou) + _ca = append(_ca, _va...) + ret := _m.Called(_ca...) + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, *models.OIDCUser, ...string) error); ok { + r0 = rf(ctx, ou, cols...) + } else { + r0 = ret.Error(0) + } + + return r0 +} + // UpdatePassword provides a mock function with given fields: ctx, id, password func (_m *Controller) UpdatePassword(ctx context.Context, id int, password string) error { ret := _m.Called(ctx, id, password) diff --git a/src/testing/pkg/oidc/dao/meta_dao.go b/src/testing/pkg/oidc/dao/meta_dao.go index 6143b1bea..9bfd064ec 100644 --- a/src/testing/pkg/oidc/dao/meta_dao.go +++ b/src/testing/pkg/oidc/dao/meta_dao.go @@ -1,6 +1,6 @@ // Code generated by mockery v2.1.0. DO NOT EDIT. -package oidc +package dao import ( context "context" diff --git a/src/testing/pkg/oidc/meta_manager.go b/src/testing/pkg/oidc/meta_manager.go new file mode 100644 index 000000000..bbd8cfcea --- /dev/null +++ b/src/testing/pkg/oidc/meta_manager.go @@ -0,0 +1,117 @@ +// Code generated by mockery v2.1.0. DO NOT EDIT. + +package oidc + +import ( + context "context" + + models "github.com/goharbor/harbor/src/common/models" + mock "github.com/stretchr/testify/mock" +) + +// MetaManager is an autogenerated mock type for the MetaManager type +type MetaManager struct { + mock.Mock +} + +// Create provides a mock function with given fields: ctx, oidcUser +func (_m *MetaManager) Create(ctx context.Context, oidcUser *models.OIDCUser) (int, error) { + ret := _m.Called(ctx, oidcUser) + + var r0 int + if rf, ok := ret.Get(0).(func(context.Context, *models.OIDCUser) int); ok { + r0 = rf(ctx, oidcUser) + } else { + r0 = ret.Get(0).(int) + } + + var r1 error + if rf, ok := ret.Get(1).(func(context.Context, *models.OIDCUser) error); ok { + r1 = rf(ctx, oidcUser) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// GetBySubIss provides a mock function with given fields: ctx, sub, iss +func (_m *MetaManager) GetBySubIss(ctx context.Context, sub string, iss string) (*models.OIDCUser, error) { + ret := _m.Called(ctx, sub, iss) + + var r0 *models.OIDCUser + if rf, ok := ret.Get(0).(func(context.Context, string, string) *models.OIDCUser); ok { + r0 = rf(ctx, sub, iss) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*models.OIDCUser) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(context.Context, string, string) error); ok { + r1 = rf(ctx, sub, iss) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// GetByUserID provides a mock function with given fields: ctx, uid +func (_m *MetaManager) GetByUserID(ctx context.Context, uid int) (*models.OIDCUser, error) { + ret := _m.Called(ctx, uid) + + var r0 *models.OIDCUser + if rf, ok := ret.Get(0).(func(context.Context, int) *models.OIDCUser); ok { + r0 = rf(ctx, uid) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*models.OIDCUser) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(context.Context, int) error); ok { + r1 = rf(ctx, uid) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// SetCliSecretByUserID provides a mock function with given fields: ctx, uid, secret +func (_m *MetaManager) SetCliSecretByUserID(ctx context.Context, uid int, secret string) error { + ret := _m.Called(ctx, uid, secret) + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, int, string) error); ok { + r0 = rf(ctx, uid, secret) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// Update provides a mock function with given fields: ctx, oidcUser, cols +func (_m *MetaManager) Update(ctx context.Context, oidcUser *models.OIDCUser, cols ...string) error { + _va := make([]interface{}, len(cols)) + for _i := range cols { + _va[_i] = cols[_i] + } + var _ca []interface{} + _ca = append(_ca, ctx, oidcUser) + _ca = append(_ca, _va...) + ret := _m.Called(_ca...) + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, *models.OIDCUser, ...string) error); ok { + r0 = rf(ctx, oidcUser, cols...) + } else { + r0 = ret.Error(0) + } + + return r0 +} diff --git a/src/testing/pkg/pkg.go b/src/testing/pkg/pkg.go index 1c4d97149..2e32b4393 100644 --- a/src/testing/pkg/pkg.go +++ b/src/testing/pkg/pkg.go @@ -29,7 +29,8 @@ package pkg //go:generate mockery --case snake --dir ../../pkg/task --name ExecutionManager --output ./task --outpkg task //go:generate mockery --case snake --dir ../../pkg/user --name Manager --output ./user --outpkg user //go:generate mockery --case snake --dir ../../pkg/user/dao --name DAO --output ./user/dao --outpkg dao -//go:generate mockery --case snake --dir ../../pkg/oidc/dao --name MetaDAO --output ./oidc/dao --outpkg oidc +//go:generate mockery --case snake --dir ../../pkg/oidc --name MetaManager --output ./oidc --outpkg oidc +//go:generate mockery --case snake --dir ../../pkg/oidc/dao --name MetaDAO --output ./oidc/dao --outpkg dao //go:generate mockery --case snake --dir ../../pkg/rbac --name Manager --output ./rbac --outpkg rbac //go:generate mockery --case snake --dir ../../pkg/rbac/dao --name DAO --output ./rbac/dao --outpkg dao //go:generate mockery --case snake --dir ../../pkg/robot --name Manager --output ./robot --outpkg robot