Merge pull request #14926 from reasonerjt/rm-common-dao-oidc-user-onboard

Remove the onboard and update funcs for OIDC user from common/dao

src/common/dao/oidc_user.go

@@ -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
-}

src/common/dao/oidc_user_test.go

@@ -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)
-	}()
-
-}

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 {

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 {
+	if err := ctluser.Ctl.UpdateOIDCMeta(ctx, oidcUser); err != nil {
-		oc.SendInternalServerError(err)
+		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.SendError(err)
-			oc.RenderError(http.StatusConflict, "Conflict, the user with same username or email has been onboarded.")
-			return nil, false
-		}
-
-		oc.SendInternalServerError(err)
 		return nil, false
 	}
-
 	return user, true
 }
 
@@ -242,8 +236,8 @@ func (oc *OIDCController) Onboard() {
 		oc.SendInternalServerError(err)
 		return
 	}
-
+	ctx := oc.Ctx.Request.Context()
-	if user, onboarded := userOnboard(oc, d, username, tb); onboarded {
+	if user, onboarded := userOnboard(ctx, oc, d, username, tb); onboarded {
 		user.OIDCUserMeta = nil
 		oc.DelSession(userInfoKey)
 		oc.PopulateUserSession(*user)

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}))

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)

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"

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
+}

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