Remove dependencies from pkg/oidc to common/dao (#14739)

Signed-off-by: Daniel Jiang <jiangd@vmware.com>

src/common/dao/oidc_user.go

@@ -39,21 +39,6 @@ var (
 	ErrRollBackOIDCUser = errors.New("sql: transaction roll back error in oicd_user")
 )
 
-// GetOIDCUserByID ...
-func GetOIDCUserByID(id int64) (*models.OIDCUser, error) {
-	oidcUser := &models.OIDCUser{
-		ID: id,
-	}
-	if err := GetOrmer().Read(oidcUser); err != nil {
-		if err == orm.ErrNoRows {
-			return nil, nil
-		}
-		return nil, err
-	}
-
-	return oidcUser, nil
-}
-
 // GetUserBySubIss ...
 func GetUserBySubIss(sub, issuer string) (*models.User, error) {
 	var oidcUsers []models.OIDCUser
@@ -99,19 +84,6 @@ func UpdateOIDCUser(oidcUser *models.OIDCUser) error {
 	return err
 }
 
-// UpdateOIDCUserSecret updates the secret of the OIDC User.  The secret in the input parm should be encrypted before
-// calling this func
-func UpdateOIDCUserSecret(oidcUser *models.OIDCUser) error {
-	_, err := GetOrmer().Update(oidcUser, "secret")
-	return err
-}
-
-// DeleteOIDCUser ...
-func DeleteOIDCUser(id int64) error {
-	_, err := GetOrmer().QueryTable(&models.OIDCUser{}).Filter("ID", id).Delete()
-	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 {

src/common/dao/oidc_user_test.go

@@ -60,11 +60,6 @@ func TestOIDCUserMetaDaoMethods(t *testing.T) {
 	require.NotNil(t, err)
 	assert.Equal(t, "unable to onboard as empty oidc user", err.Error())
 
-	// test get by ID
-	oidcUser1, err := GetOIDCUserByID(ou111.ID)
-	require.Nil(t, err)
-	assert.Equal(t, ou111.UserID, oidcUser1.UserID)
-
 	// test get by userID
 	oidcUser2, err := GetOIDCUserByUserID(user111.UserID)
 	require.Nil(t, err)
@@ -83,11 +78,6 @@ func TestOIDCUserMetaDaoMethods(t *testing.T) {
 		Secret: "newSecret",
 	}
 	require.Nil(t, UpdateOIDCUser(meta3))
-	oidcUser1Update, err := GetOIDCUserByID(ou111.ID)
-	require.Nil(t, err)
-	assert.Equal(t, "QWE123123RT1", oidcUser1Update.SubIss)
-	assert.Equal(t, "newSecret", oidcUser1Update.Secret)
-
 	// clear data
 	defer func() {
 		_, err := GetOrmer().Raw(`delete from oidc_user`).Exec()

src/controller/user/controller.go

@@ -48,6 +48,8 @@ type Controller interface {
 	Count(ctx context.Context, query *q.Query) (int64, error)
 	// Get ...
 	Get(ctx context.Context, id int, opt *Option) (*models.User, error)
+	// GetByName gets the user model by username, it only supports getting the basic and does not support opt
+	GetByName(ctx context.Context, username string) (*models.User, error)
 	// Delete ...
 	Delete(ctx context.Context, id int) error
 	// UpdateProfile update the profile based on the ID and data in the model in parm, only a subset of attributes in the model
@@ -75,6 +77,10 @@ type controller struct {
 	oidcMetaMgr oidc.MetaManager
 }
 
+func (c *controller) GetByName(ctx context.Context, username string) (*models.User, error) {
+	return c.mgr.GetByName(ctx, username)
+}
+
 func (c *controller) SetCliSecret(ctx context.Context, id int, secret string) error {
 	return c.oidcMetaMgr.SetCliSecretByUserID(ctx, id, secret)
 }

src/pkg/oidc/secret.go

@@ -4,11 +4,12 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+
 	"github.com/goharbor/harbor/src/lib/config"
+	"github.com/goharbor/harbor/src/pkg/oidc/dao"
+
 	"sync"
 
-	"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/lib/log"
 )
@@ -30,8 +31,8 @@ func verifyError(err error) error {
 // SecretManager is the interface for store and verify the secret
 type SecretManager interface {
 	// VerifySecret verifies the secret and the token associated with it, it refreshes the token in the DB if it's
-	// refreshed during the verification.  It returns a populated user model based on the ID token associated with the secret.
+	// refreshed during the verification.
-	VerifySecret(ctx context.Context, username string, secret string) (*models.User, error)
+	VerifySecret(ctx context.Context, username string, secret string) (*UserInfo, error)
 }
 
 type keyGetter struct {
@@ -61,22 +62,18 @@ func (kg *keyGetter) encryptKey() (string, error) {
 var keyLoader = &keyGetter{}
 
 type defaultManager struct {
+	metaDao dao.MetaDAO
 }
 
-var m SecretManager = &defaultManager{}
+var m SecretManager = &defaultManager{
+	metaDao: dao.NewMetaDao(),
+}
 
 // VerifySecret verifies the secret and the token associated with it, it refreshes the token in the DB if it's
 // refreshed during the verification.  It returns a populated user model based on the ID token associated with the secret.
-func (dm *defaultManager) VerifySecret(ctx context.Context, username string, secret string) (*models.User, error) {
+func (dm *defaultManager) VerifySecret(ctx context.Context, username string, secret string) (*UserInfo, error) {
 	log.Debugf("Verifying the secret for user: %s", username)
-	user, err := dao.GetUser(models.User{Username: username})
+	oidcUser, err := dm.metaDao.GetByUsername(ctx, username)
-	if err != nil {
-		return nil, err
-	}
-	if user == nil {
-		return nil, verifyError(fmt.Errorf("user does not exist, name: %s", username))
-	}
-	oidcUser, err := dao.GetOIDCUserByUserID(user.UserID)
 	if err != nil {
 		return nil, fmt.Errorf("failed to get oidc user info, error: %v", err)
 	}
@@ -115,7 +112,7 @@ func (dm *defaultManager) VerifySecret(ctx context.Context, username string, sec
 		}
 		encToken, _ := utils.ReversibleEncrypt(string(tb), key)
 		oidcUser.Token = encToken
-		err = dao.UpdateOIDCUser(oidcUser)
+		err = dm.metaDao.Update(ctx, oidcUser)
 		if err != nil {
 			log.Errorf("Failed to persist token, user id: %d, error: %v", oidcUser.UserID, err)
 		}
@@ -125,12 +122,10 @@ func (dm *defaultManager) VerifySecret(ctx context.Context, username string, sec
 	if err != nil {
 		return nil, verifyError(err)
 	}
-	InjectGroupsToUser(info, user)
+	return info, nil
-	log.Debugf("Secret verification succeed, username: %s", username)
-	return user, nil
 }
 
 // VerifySecret calls the manager to verify the secret.
-func VerifySecret(ctx context.Context, name string, secret string) (*models.User, error) {
+func VerifySecret(ctx context.Context, name string, secret string) (*UserInfo, error) {
 	return m.VerifySecret(ctx, name, secret)
 }

src/pkg/oidc/testutils.go

@@ -4,8 +4,6 @@ import (
 	"context"
 	"fmt"
 	"strconv"
-
-	"github.com/goharbor/harbor/src/common/models"
 )
 import "errors"
 
@@ -14,11 +12,17 @@ type fakeVerifier struct {
 	secret string
 }
 
-func (fv *fakeVerifier) VerifySecret(ctx context.Context, name string, secret string) (*models.User, error) {
+func (fv *fakeVerifier) VerifySecret(ctx context.Context, name string, secret string) (*UserInfo, error) {
 	if secret != fv.secret {
 		return nil, verifyError(errors.New("mismatch"))
 	}
-	return &models.User{UserID: 1, Username: name, Email: fmt.Sprintf("%s@test.local", name)}, nil
+	return &UserInfo{
+		Username: name,
+		Email:    fmt.Sprintf("%s@test.local", name),
+		Subject:  "subject",
+		Issuer:   "issuer",
+	}, nil
+
 }
 
 // SetHardcodeVerifierForTest overwrite the default secret manager for testing.

src/pkg/user/manager.go

@@ -35,7 +35,7 @@ var (
 type Manager interface {
 	// Get get user by user id
 	Get(ctx context.Context, id int) (*models.User, error)
-	// Get get user by username
+	// GetByName get user by username
 	GetByName(ctx context.Context, username string) (*models.User, error)
 	// List users according to the query
 	List(ctx context.Context, query *q.Query) (models.Users, error)

src/server/middleware/security/oidc_cli.go

@@ -24,6 +24,7 @@ import (
 	"github.com/goharbor/harbor/src/common/api"
 	"github.com/goharbor/harbor/src/common/security"
 	"github.com/goharbor/harbor/src/common/security/local"
+	"github.com/goharbor/harbor/src/controller/user"
 	"github.com/goharbor/harbor/src/lib"
 	"github.com/goharbor/harbor/src/lib/log"
 	"github.com/goharbor/harbor/src/pkg/oidc"
@@ -38,15 +39,17 @@ var (
 	reposAPIRe     = regexp.MustCompile(fmt.Sprintf(`^%s/projects/.*/repositories$`, regexp.QuoteMeta(base)))
 	artifactsAPIRe = regexp.MustCompile(fmt.Sprintf(`^%s/projects/.*/repositories/.*/artifacts$`, regexp.QuoteMeta(base)))
 	tagsAPIRe      = regexp.MustCompile(fmt.Sprintf(`^%s/projects/.*/repositories/.*/artifacts/.*/tags/.*$`, regexp.QuoteMeta(base)))
+	uctl           = user.Ctl
 )
 
 type oidcCli struct{}
 
 func (o *oidcCli) Generate(req *http.Request) security.Context {
-	if lib.GetAuthMode(req.Context()) != common.OIDCAuth {
+	ctx := req.Context()
+	if lib.GetAuthMode(ctx) != common.OIDCAuth {
 		return nil
 	}
-	logger := log.G(req.Context())
+	logger := log.G(ctx)
 	username, secret, ok := req.BasicAuth()
 	if !ok {
 		return nil
@@ -54,13 +57,19 @@ func (o *oidcCli) Generate(req *http.Request) security.Context {
 	if !o.valid(req) {
 		return nil
 	}
-	user, err := oidc.VerifySecret(req.Context(), username, secret)
+	info, err := oidc.VerifySecret(ctx, username, secret)
 	if err != nil {
-		logger.Errorf("failed to verify secret: %v", err)
+		logger.Errorf("failed to verify secret, username: %s, error: %v", username, err)
 		return nil
 	}
+	u, err := uctl.GetByName(ctx, username)
+	if err != nil {
+		logger.Errorf("failed to get user model, username: %s, error: %v", username, err)
+		return nil
+	}
+	oidc.InjectGroupsToUser(info, u)
 	logger.Debugf("an OIDC CLI security context generated for request %s %s", req.Method, req.URL.Path)
-	return local.NewSecurityContext(user)
+	return local.NewSecurityContext(u)
 }
 
 func (o *oidcCli) valid(req *http.Request) bool {

src/server/middleware/security/oidc_cli_test.go

@@ -20,9 +20,12 @@ import (
 	"testing"
 
 	"github.com/goharbor/harbor/src/common"
+	"github.com/goharbor/harbor/src/common/models"
 	"github.com/goharbor/harbor/src/lib"
 	"github.com/goharbor/harbor/src/pkg/oidc"
+	testingUser "github.com/goharbor/harbor/src/testing/controller/user"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 )
 
@@ -43,6 +46,13 @@ func TestOIDCCli(t *testing.T) {
 	// pass
 	username := "oidcModiferTester"
 	password := "oidcSecret"
+	testCtl := &testingUser.Controller{}
+	testCtl.On("GetByName", mock.Anything, username).Return(
+		&models.User{
+			Username: username,
+			Email:    fmt.Sprintf("%s@test.domain", username),
+		}, nil)
+	uctl = testCtl
 	oidc.SetHardcodeVerifierForTest(password)
 	req = req.WithContext(lib.WithAuthMode(req.Context(), common.OIDCAuth))
 	req.SetBasicAuth(username, password)

src/testing/controller/user/controller.go

@@ -97,6 +97,29 @@ func (_m *Controller) Get(ctx context.Context, id int, opt *user.Option) (*model
 	return r0, r1
 }
 
+// GetByName provides a mock function with given fields: ctx, username
+func (_m *Controller) GetByName(ctx context.Context, username string) (*models.User, error) {
+	ret := _m.Called(ctx, username)
+
+	var r0 *models.User
+	if rf, ok := ret.Get(0).(func(context.Context, string) *models.User); ok {
+		r0 = rf(ctx, username)
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(*models.User)
+		}
+	}
+
+	var r1 error
+	if rf, ok := ret.Get(1).(func(context.Context, string) error); ok {
+		r1 = rf(ctx, username)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}
+
 // List provides a mock function with given fields: ctx, query
 func (_m *Controller) List(ctx context.Context, query *q.Query) ([]*models.User, error) {
 	ret := _m.Called(ctx, query)