Merge pull request #9820 from reasonerjt/oidc-cli-secret-group

Populate user groups during OIDC CLI secret verification

src/common/utils/oidc/helper.go

@@ -31,7 +31,13 @@ import (
 	"time"
 )
 
-const googleEndpoint = "https://accounts.google.com"
+const (
+	googleEndpoint = "https://accounts.google.com"
+)
+
+type claimsProvider interface {
+	Claims(v interface{}) error
+}
 
 type providerHelper struct {
 	sync.Mutex
@@ -106,7 +112,18 @@ var insecureTransport = &http.Transport{
 // Token wraps the attributes of a oauth2 token plus the attribute of ID token
 type Token struct {
 	oauth2.Token
-	IDToken string `json:"id_token,omitempty"`
+	RawIDToken string `json:"id_token,omitempty"`
+}
+
+// UserInfo wraps the information that is extracted via token.  It will be transformed to data object that is persisted
+// in the DB
+type UserInfo struct {
+	Issuer        string   `json:"iss"`
+	Subject       string   `json:"sub"`
+	Username      string   `json:"name"`
+	Email         string   `json:"email"`
+	Groups        []string `json:"groups"`
+	hasGroupClaim bool
 }
 
 func getOauthConf() (*oauth2.Config, error) {
@@ -115,7 +132,7 @@ func getOauthConf() (*oauth2.Config, error) {
 		return nil, err
 	}
 	setting := provider.setting.Load().(models.OIDCSetting)
-	scopes := []string{}
+	scopes := make([]string, 0)
 	for _, sc := range setting.Scope {
 		if strings.HasPrefix(p.Endpoint().AuthURL, googleEndpoint) && sc == gooidc.ScopeOfflineAccess {
 			log.Warningf("Dropped unsupported scope: %s ", sc)
@@ -159,18 +176,31 @@ func ExchangeToken(ctx context.Context, code string) (*Token, error) {
 	if err != nil {
 		return nil, err
 	}
-	return &Token{Token: *oauthToken, IDToken: oauthToken.Extra("id_token").(string)}, nil
+	return &Token{Token: *oauthToken, RawIDToken: oauthToken.Extra("id_token").(string)}, nil
+}
+
+func parseIDToken(ctx context.Context, rawIDToken string) (*gooidc.IDToken, error) {
+	conf := &gooidc.Config{SkipClientIDCheck: true, SkipExpiryCheck: true}
+	return verifyTokenWithConfig(ctx, rawIDToken, conf)
 }
 
 // VerifyToken verifies the ID token based on the OIDC settings
 func VerifyToken(ctx context.Context, rawIDToken string) (*gooidc.IDToken, error) {
+	return verifyTokenWithConfig(ctx, rawIDToken, nil)
+}
+
+func verifyTokenWithConfig(ctx context.Context, rawIDToken string, conf *gooidc.Config) (*gooidc.IDToken, error) {
+	log.Debugf("Raw ID token for verification: %s", rawIDToken)
 	p, err := provider.get()
 	if err != nil {
 		return nil, err
 	}
-	verifier := p.Verifier(&gooidc.Config{ClientID: provider.setting.Load().(models.OIDCSetting).ClientID})
-	setting := provider.setting.Load().(models.OIDCSetting)
-	ctx = clientCtx(ctx, setting.VerifyCert)
+	settings := provider.setting.Load().(models.OIDCSetting)
+	if conf == nil {
+		conf = &gooidc.Config{ClientID: settings.ClientID}
+	}
+	verifier := p.Verifier(conf)
+	ctx = clientCtx(ctx, settings.VerifyCert)
 	return verifier.Verify(ctx, rawIDToken)
 }
 
@@ -186,55 +216,120 @@ func clientCtx(ctx context.Context, verifyCert bool) context.Context {
 	return gooidc.ClientContext(ctx, client)
 }
 
-// RefreshToken refreshes the token passed in parameter, and return the new token.
-func RefreshToken(ctx context.Context, token *Token) (*Token, error) {
-	oauth, err := getOauthConf()
+// refreshToken tries to refresh the token if it's expired, if it doesn't the
+// original one will be returned.
+func refreshToken(ctx context.Context, token *Token) (*Token, error) {
+	oauthCfg, err := getOauthConf()
 	if err != nil {
-		log.Errorf("Failed to get OAuth configuration, error: %v", err)
 		return nil, err
 	}
 	setting := provider.setting.Load().(models.OIDCSetting)
-	ctx = clientCtx(ctx, setting.VerifyCert)
-	ts := oauth.TokenSource(ctx, &token.Token)
-	t, err := ts.Token()
+	cctx := clientCtx(ctx, setting.VerifyCert)
+	ts := oauthCfg.TokenSource(cctx, &token.Token)
+	nt, err := ts.Token()
 	if err != nil {
 		return nil, err
 	}
-
-	it, ok := t.Extra("id_token").(string)
+	it, ok := nt.Extra("id_token").(string)
 	if !ok {
-		log.Debugf("id_token not exist in refresh response")
+		log.Debug("id_token not exist in refresh response")
 	}
-	return &Token{Token: *t, IDToken: it}, nil
+	return &Token{Token: *nt, RawIDToken: it}, nil
 }
 
-// GroupsFromToken returns the list of group name in the token, the claims of the group list is set in OIDCSetting.
-// It's designed not to return errors, in case of unexpected situation it will log and return empty list.
-func GroupsFromToken(token *gooidc.IDToken) []string {
-	if token == nil {
-		log.Warning("Return empty list for nil token")
-		return []string{}
-	}
+// UserInfoFromToken tries to call the UserInfo endpoint of the OIDC provider, and consolidate with ID token
+// to generate a UserInfo object, if the ID token is not in the input token struct, some attributes will be empty
+func UserInfoFromToken(ctx context.Context, token *Token) (*UserInfo, error) {
 	setting := provider.setting.Load().(models.OIDCSetting)
-	if len(setting.GroupsClaim) == 0 {
-		log.Warning("Group claims is not set in OIDC setting returning empty group list.")
-		return []string{}
-	}
-	var c map[string]interface{}
-	err := token.Claims(&c)
+	local, err := userInfoFromIDToken(ctx, token, setting)
 	if err != nil {
-		log.Warningf("Failed to get claims map, error: %v", err)
-		return []string{}
+		return nil, err
 	}
-	return groupsFromClaim(c, setting.GroupsClaim)
+	remote, err := userInfoFromRemote(ctx, token, setting)
+	if err != nil {
+		log.Warningf("Failed to get userInfo by calling remote userinfo endpoint, error: %v ", err)
+	}
+	if remote != nil && local != nil {
+		if remote.Subject != local.Subject {
+			return nil, fmt.Errorf("the subject from userinfo: %s does not match the subject from ID token: %s, probably a security attack happened", remote.Subject, local.Subject)
+		}
+		return mergeUserInfo(remote, local), nil
+	} else if remote != nil && local == nil {
+		return remote, nil
+	} else if local != nil && remote == nil {
+		log.Debugf("Fall back to user data from ID token.")
+		return local, nil
+	}
+	return nil, fmt.Errorf("failed to get userinfo from both remote and ID token")
 }
 
-func groupsFromClaim(claimMap map[string]interface{}, k string) []string {
-	var res []string
+func mergeUserInfo(remote, local *UserInfo) *UserInfo {
+	res := &UserInfo{
+		// data only contained in ID token
+		Subject: local.Subject,
+		Issuer:  local.Issuer,
+		// Used data from userinfo
+		Username: remote.Username,
+		Email:    remote.Email,
+	}
+	if remote.hasGroupClaim {
+		res.Groups = remote.Groups
+		res.hasGroupClaim = true
+	} else if local.hasGroupClaim {
+		res.Groups = local.Groups
+		res.hasGroupClaim = true
+	} else {
+		res.Groups = []string{}
+	}
+	return res
+}
+
+func userInfoFromRemote(ctx context.Context, token *Token, setting models.OIDCSetting) (*UserInfo, error) {
+	p, err := provider.get()
+	if err != nil {
+		return nil, err
+	}
+	cctx := clientCtx(ctx, setting.VerifyCert)
+	u, err := p.UserInfo(cctx, oauth2.StaticTokenSource(&token.Token))
+	if err != nil {
+		return nil, err
+	}
+	return userInfoFromClaims(u, setting.GroupsClaim)
+}
+
+func userInfoFromIDToken(ctx context.Context, token *Token, setting models.OIDCSetting) (*UserInfo, error) {
+	if token.RawIDToken == "" {
+		return nil, nil
+	}
+	idt, err := parseIDToken(ctx, token.RawIDToken)
+	if err != nil {
+		return nil, err
+	}
+	return userInfoFromClaims(idt, setting.GroupsClaim)
+}
+
+func userInfoFromClaims(c claimsProvider, g string) (*UserInfo, error) {
+	res := &UserInfo{}
+	if err := c.Claims(res); err != nil {
+		return nil, err
+	}
+	res.Groups, res.hasGroupClaim = GroupsFromClaims(c, g)
+	return res, nil
+}
+
+// GroupsFromClaims fetches the group name list from claimprovider, such as decoded ID token.
+// If the claims does not have the claim defined as k, the second return value will be false, otherwise true
+func GroupsFromClaims(gp claimsProvider, k string) ([]string, bool) {
+	res := make([]string, 0)
+	claimMap := make(map[string]interface{})
+	if err := gp.Claims(&claimMap); err != nil {
+		log.Errorf("failed to fetch claims, error: %v", err)
+		return res, false
+	}
 	g, ok := claimMap[k].([]interface{})
 	if !ok {
 		log.Warningf("Unable to get groups from claims, claims: %+v, groups claims key: %s", claimMap, k)
-		return res
+		return res, false
 	}
 	for _, e := range g {
 		s, ok := e.(string)
@@ -244,7 +339,7 @@ func groupsFromClaim(claimMap map[string]interface{}, k string) []string {
 		}
 		res = append(res, s)
 	}
-	return res
+	return res, true
 }
 
 // Conn wraps connection info of an OIDC endpoint

src/common/utils/oidc/helper_test.go

@@ -15,7 +15,7 @@
 package oidc
 
 import (
-	gooidc "github.com/coreos/go-oidc"
+	"encoding/json"
 	"github.com/goharbor/harbor/src/common"
 	config2 "github.com/goharbor/harbor/src/common/config"
 	"github.com/goharbor/harbor/src/common/models"
@@ -112,11 +112,16 @@ func TestTestEndpoint(t *testing.T) {
 	assert.NotNil(t, TestEndpoint(c2))
 }
 
-func TestGroupsFromToken(t *testing.T) {
-	res := GroupsFromToken(nil)
-	assert.Equal(t, []string{}, res)
-	res = GroupsFromToken(&gooidc.IDToken{})
-	assert.Equal(t, []string{}, res)
+type fakeClaims struct {
+	claims map[string]interface{}
+}
+
+func (fc *fakeClaims) Claims(n interface{}) error {
+	b, err := json.Marshal(fc.claims)
+	if err != nil {
+		return err
+	}
+	return json.Unmarshal(b, n)
 }
 
 func TestGroupsFromClaim(t *testing.T) {
@@ -130,31 +135,194 @@ func TestGroupsFromClaim(t *testing.T) {
 		input  map[string]interface{}
 		key    string
 		expect []string
+		ok     bool
 	}{
 		{
 			in,
 			"user",
-			nil,
+			[]string{},
+			false,
 		},
 		{
 			in,
 			"prg",
-			nil,
+			[]string{},
+			false,
 		},
 		{
 			in,
 			"groups",
 			[]string{"group1", "group2"},
+			true,
 		},
 		{
 			in,
 			"groups_2",
 			[]string{"group1", "group2"},
+			true,
 		},
 	}
 
 	for _, tc := range m {
-		r := groupsFromClaim(tc.input, tc.key)
+
+		r, ok := GroupsFromClaims(&fakeClaims{tc.input}, tc.key)
 		assert.Equal(t, tc.expect, r)
+		assert.Equal(t, tc.ok, ok)
+	}
+}
+
+func TestUserInfoFromClaims(t *testing.T) {
+	s := []struct {
+		input      map[string]interface{}
+		groupClaim string
+		expect     *UserInfo
+	}{
+		{
+			input: map[string]interface{}{
+				"name":   "Daniel",
+				"email":  "daniel@gmail.com",
+				"groups": []interface{}{"g1", "g2"},
+			},
+			groupClaim: "grouplist",
+			expect: &UserInfo{
+				Issuer:        "",
+				Subject:       "",
+				Username:      "Daniel",
+				Email:         "daniel@gmail.com",
+				Groups:        []string{},
+				hasGroupClaim: false,
+			},
+		},
+		{
+			input: map[string]interface{}{
+				"name":   "Daniel",
+				"email":  "daniel@gmail.com",
+				"groups": []interface{}{"g1", "g2"},
+			},
+			groupClaim: "groups",
+			expect: &UserInfo{
+				Issuer:        "",
+				Subject:       "",
+				Username:      "Daniel",
+				Email:         "daniel@gmail.com",
+				Groups:        []string{"g1", "g2"},
+				hasGroupClaim: true,
+			},
+		},
+		{
+			input: map[string]interface{}{
+				"iss":        "issuer",
+				"sub":        "subject000",
+				"name":       "jack",
+				"email":      "jack@gmail.com",
+				"groupclaim": []interface{}{},
+			},
+			groupClaim: "groupclaim",
+			expect: &UserInfo{
+				Issuer:        "issuer",
+				Subject:       "subject000",
+				Username:      "jack",
+				Email:         "jack@gmail.com",
+				Groups:        []string{},
+				hasGroupClaim: true,
+			},
+		},
+	}
+	for _, tc := range s {
+		out, err := userInfoFromClaims(&fakeClaims{tc.input}, tc.groupClaim)
+		assert.Nil(t, err)
+		assert.Equal(t, *tc.expect, *out)
+	}
+}
+
+func TestMergeUserInfo(t *testing.T) {
+	s := []struct {
+		fromInfo    *UserInfo
+		fromIDToken *UserInfo
+		expected    *UserInfo
+	}{
+		{
+			fromInfo: &UserInfo{
+				Issuer:        "",
+				Subject:       "",
+				Username:      "daniel",
+				Email:         "daniel@gmail.com",
+				Groups:        []string{},
+				hasGroupClaim: false,
+			},
+			fromIDToken: &UserInfo{
+				Issuer:        "issuer-google",
+				Subject:       "subject-daniel",
+				Username:      "daniel",
+				Email:         "daniel@yahoo.com",
+				Groups:        []string{"developers", "everyone"},
+				hasGroupClaim: true,
+			},
+			expected: &UserInfo{
+				Issuer:        "issuer-google",
+				Subject:       "subject-daniel",
+				Username:      "daniel",
+				Email:         "daniel@gmail.com",
+				Groups:        []string{"developers", "everyone"},
+				hasGroupClaim: true,
+			},
+		},
+		{
+			fromInfo: &UserInfo{
+				Issuer:        "",
+				Subject:       "",
+				Username:      "tom",
+				Email:         "tom@gmail.com",
+				Groups:        nil,
+				hasGroupClaim: false,
+			},
+			fromIDToken: &UserInfo{
+				Issuer:        "issuer-okta",
+				Subject:       "subject-jiangtan",
+				Username:      "tom",
+				Email:         "tom@okta.com",
+				Groups:        []string{"nouse"},
+				hasGroupClaim: false,
+			},
+			expected: &UserInfo{
+				Issuer:        "issuer-okta",
+				Subject:       "subject-jiangtan",
+				Username:      "tom",
+				Email:         "tom@gmail.com",
+				Groups:        []string{},
+				hasGroupClaim: false,
+			},
+		},
+		{
+			fromInfo: &UserInfo{
+				Issuer:        "",
+				Subject:       "",
+				Username:      "jim",
+				Email:         "jim@gmail.com",
+				Groups:        []string{},
+				hasGroupClaim: true,
+			},
+			fromIDToken: &UserInfo{
+				Issuer:        "issuer-yahoo",
+				Subject:       "subject-jim",
+				Username:      "jim",
+				Email:         "jim@yaoo.com",
+				Groups:        []string{"g1", "g2"},
+				hasGroupClaim: true,
+			},
+			expected: &UserInfo{
+				Issuer:        "issuer-yahoo",
+				Subject:       "subject-jim",
+				Username:      "jim",
+				Email:         "jim@gmail.com",
+				Groups:        []string{},
+				hasGroupClaim: true,
+			},
+		},
+	}
+
+	for _, tc := range s {
+		m := mergeUserInfo(tc.fromInfo, tc.fromIDToken)
+		assert.Equal(t, *tc.expected, *m)
 	}
 }

src/common/utils/oidc/secret.go

@@ -4,14 +4,15 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"github.com/goharbor/harbor/src/common"
 	"github.com/goharbor/harbor/src/common/dao"
+	"github.com/goharbor/harbor/src/common/dao/group"
 	"github.com/goharbor/harbor/src/common/models"
 	"github.com/goharbor/harbor/src/common/utils"
 	"github.com/goharbor/harbor/src/common/utils/log"
 	"github.com/goharbor/harbor/src/core/config"
 	"github.com/pkg/errors"
 	"sync"
-	"time"
 )
 
 // SecretVerifyError wraps the different errors happened when verifying a secret for OIDC user.  When seeing this error,
@@ -31,11 +32,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
-	VerifySecret(ctx context.Context, userID int, secret string) error
-	// VerifyToken verifies the token in the model from parm,
-	// and refreshes the token in the DB if it's refreshed during the verification.
-	VerifyToken(ctx context.Context, user *models.OIDCUser) error
+	// refreshed during the verification.  It returns a populated user model based on the ID token associated with the secret.
+	VerifySecret(ctx context.Context, username string, secret string) (*models.User, error)
 }
 
 type defaultManager struct {
@@ -60,80 +58,75 @@ func (dm *defaultManager) getEncryptKey() (string, error) {
 	return dm.key, nil
 }
 
-// VerifySecret verifies the secret and the token associated with it, it tries to update the token in the DB if it's
-// refreshed during the verification
-func (dm *defaultManager) VerifySecret(ctx context.Context, userID int, secret string) error {
-	oidcUser, err := dao.GetOIDCUserByUserID(userID)
+// 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) {
+	user, err := dao.GetUser(models.User{Username: username})
 	if err != nil {
-		return fmt.Errorf("failed to get oidc user info, error: %v", err)
+		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)
 	}
 	if oidcUser == nil {
-		return fmt.Errorf("user is not onboarded as OIDC user")
+		return nil, fmt.Errorf("user is not onboarded as OIDC user")
 	}
 	key, err := dm.getEncryptKey()
 	if err != nil {
-		return fmt.Errorf("failed to load the key for encryption/decryption： %v", err)
+		return nil, fmt.Errorf("failed to load the key for encryption/decryption： %v", err)
 	}
 	plainSecret, err := utils.ReversibleDecrypt(oidcUser.Secret, key)
 	if err != nil {
-		return fmt.Errorf("failed to decrypt secret from DB: %v", err)
+		return nil, fmt.Errorf("failed to decrypt secret from DB: %v", err)
 	}
 	if secret != plainSecret {
-		return verifyError(errors.New("secret mismatch"))
+		return nil, verifyError(errors.New("secret mismatch"))
 	}
-	return dm.VerifyToken(ctx, oidcUser)
-}
-
-// VerifyToken checks the expiration of the token in the model, (we'll only do expiration checks b/c according to spec,
-// the response may not have ID token:
-// https://openid.net/specs/openid-connect-core-1_0.html#RefreshTokenResponse
-// and it will try to refresh the token
-// if it's expired, if the refresh is successful it will persist the token and consider the verification successful.
-func (dm *defaultManager) VerifyToken(ctx context.Context, user *models.OIDCUser) error {
-	if user == nil {
-		return verifyError(fmt.Errorf("input user is nil"))
-	}
-	key, err := dm.getEncryptKey()
+	tokenStr, err := utils.ReversibleDecrypt(oidcUser.Token, key)
 	if err != nil {
-		return fmt.Errorf("failed to load the key for encryption/decryption： %v", err)
-	}
-	tokenStr, err := utils.ReversibleDecrypt(user.Token, key)
-	if err != nil {
-		return verifyError(err)
+		return nil, verifyError(err)
 	}
 	token := &Token{}
 	err = json.Unmarshal(([]byte)(tokenStr), token)
 	if err != nil {
-		return verifyError(err)
+		return nil, verifyError(err)
 	}
-	log.Debugf("Token string for verify: %s", tokenStr)
-	if token.Expiry.After(time.Now()) {
-		return nil
+	if !token.Valid() {
+		log.Debug("Refreshing token")
+		token, err = refreshToken(ctx, token)
+		if err != nil {
+			return nil, fmt.Errorf("failed to refresh token")
+		}
+		tb, err := json.Marshal(token)
+		if err != nil {
+			return nil, fmt.Errorf("failed to encode the refreshed token, error: %v", err)
+		}
+		encToken, _ := utils.ReversibleEncrypt(string(tb), key)
+		oidcUser.Token = encToken
+		err = dao.UpdateOIDCUser(oidcUser)
+		if err != nil {
+			log.Errorf("Failed to persist token, user id: %d, error: %v", oidcUser.UserID, err)
+		}
+		log.Debug("Token refreshed and persisted")
 	}
-	log.Info("Token string has expired, refreshing...")
-	t, err := RefreshToken(ctx, token)
+	info, err := UserInfoFromToken(ctx, token)
 	if err != nil {
-		return verifyError(err)
+		return nil, verifyError(err)
 	}
-	tb, err := json.Marshal(t)
+	gids, err := group.PopulateGroup(models.UserGroupsFromName(info.Groups, common.OIDCGroupType))
 	if err != nil {
-		log.Warningf("Failed to encode the refreshed token, error: %v", err)
+		log.Warningf("failed to get group ID, error: %v, skip populating groups", err)
+	} else {
+		user.GroupIDs = gids
 	}
-	encToken, _ := utils.ReversibleEncrypt(string(tb), key)
-	user.Token = encToken
-	err = dao.UpdateOIDCUser(user)
-	if err != nil {
-		log.Warningf("Failed to update the token in DB: %v, ignore this error.", err)
-	}
-	return nil
+	return user, nil
 }
 
 // VerifySecret calls the manager to verify the secret.
-func VerifySecret(ctx context.Context, userID int, secret string) error {
-	return m.VerifySecret(ctx, userID, secret)
-}
-
-// VerifyAndPersistToken calls the manager to verify token and persist it if it's refreshed.
-func VerifyAndPersistToken(ctx context.Context, user *models.OIDCUser) error {
-	return m.VerifyToken(ctx, user)
+func VerifySecret(ctx context.Context, name string, secret string) (*models.User, error) {
+	return m.VerifySecret(ctx, name, secret)
 }

src/common/utils/oidc/secret_test.go

@@ -27,6 +27,10 @@ func TestDefaultManagerGetEncryptKey(t *testing.T) {
 
 func TestPkgVerifySecret(t *testing.T) {
 	SetHardcodeVerifierForTest("secret")
-	assert.Nil(t, VerifySecret(context.Background(), 1, "secret"))
-	assert.NotNil(t, VerifySecret(context.Background(), 1, "not-the-secret"))
+	u, err := VerifySecret(context.Background(), "user", "secret")
+	assert.Nil(t, err)
+	assert.Equal(t, "user", u.Username)
+	u2, err2 := VerifySecret(context.Background(), "user2", "not-the-secret")
+	assert.NotNil(t, err2)
+	assert.Nil(t, u2)
 }

src/common/utils/oidc/testutils.go

@@ -2,6 +2,7 @@ package oidc
 
 import (
 	"context"
+	"fmt"
 	"github.com/goharbor/harbor/src/common/models"
 )
 import "errors"
@@ -11,15 +12,11 @@ type fakeVerifier struct {
 	secret string
 }
 
-func (fv *fakeVerifier) VerifySecret(ctx context.Context, userID int, secret string) error {
+func (fv *fakeVerifier) VerifySecret(ctx context.Context, name string, secret string) (*models.User, error) {
 	if secret != fv.secret {
-		return verifyError(errors.New("mismatch"))
+		return nil, verifyError(errors.New("mismatch"))
 	}
-	return nil
-}
-
-func (fv *fakeVerifier) VerifyToken(ctx context.Context, u *models.OIDCUser) error {
-	return nil
+	return &models.User{UserID: 1, Username: name, Email: fmt.Sprintf("%s@test.local", name)}, nil
 }
 
 // SetHardcodeVerifierForTest overwrite the default secret manager for testing.

src/core/controllers/oidc.go

@@ -46,14 +46,6 @@ type onboardReq struct {
 	Username string `json:"username"`
 }
 
-type oidcUserData struct {
-	Issuer   string `json:"iss"`
-	Subject  string `json:"sub"`
-	Username string `json:"name"`
-	Email    string `json:"email"`
-	GroupIDs []int  `json:"group_ids"`
-}
-
 // Prepare include public code path for call request handler of OIDCController
 func (oc *OIDCController) Prepare() {
 	if mode, _ := config.AuthMode(); mode != common.OIDCAuth {
@@ -103,36 +95,26 @@ func (oc *OIDCController) Callback() {
 		oc.SendBadRequestError(err)
 		return
 	}
-	log.Debugf("ID token from provider: %s", token.IDToken)
-
-	idToken, err := oidc.VerifyToken(ctx, token.IDToken)
+	_, err = oidc.VerifyToken(ctx, token.RawIDToken)
 	if err != nil {
 		oc.SendInternalServerError(err)
 		return
 	}
-	d := &oidcUserData{}
-	err = idToken.Claims(d)
+	info, err := oidc.UserInfoFromToken(ctx, token)
 	if err != nil {
 		oc.SendInternalServerError(err)
 		return
 	}
-	groupNames := oidc.GroupsFromToken(idToken)
-	oidcGroups := models.UserGroupsFromName(groupNames, common.OIDCGroupType)
-	d.GroupIDs, err = group.PopulateGroup(oidcGroups)
-	if err != nil {
-		log.Warningf("Failed to get group ID list, due to error: %v, setting empty list into user model.", err)
-	}
-	ouDataStr, err := json.Marshal(d)
+	ouDataStr, err := json.Marshal(info)
 	if err != nil {
 		oc.SendInternalServerError(err)
 		return
 	}
-	u, err := dao.GetUserBySubIss(d.Subject, d.Issuer)
+	u, err := dao.GetUserBySubIss(info.Subject, info.Issuer)
 	if err != nil {
 		oc.SendInternalServerError(err)
 		return
 	}
-
 	tokenBytes, err := json.Marshal(token)
 	if err != nil {
 		oc.SendInternalServerError(err)
@@ -142,10 +124,14 @@ func (oc *OIDCController) Callback() {
 
 	if u == nil {
 		oc.SetSession(userInfoKey, string(ouDataStr))
-		oc.Controller.Redirect(fmt.Sprintf("/oidc-onboard?username=%s", strings.Replace(d.Username, " ", "_", -1)),
+		oc.Controller.Redirect(fmt.Sprintf("/oidc-onboard?username=%s", strings.Replace(info.Username, " ", "_", -1)),
 			http.StatusFound)
 	} else {
-		u.GroupIDs = d.GroupIDs
+		gids, err := group.PopulateGroup(models.UserGroupsFromName(info.Groups, common.OIDCGroupType))
+		if err != nil {
+			log.Warningf("Failed to populate groups, error: %v, user will have empty group list, username: %s", err, info.Username)
+		}
+		u.GroupIDs = gids
 		oidcUser, err := dao.GetOIDCUserByUserID(u.UserID)
 		if err != nil {
 			oc.SendInternalServerError(err)
@@ -195,12 +181,16 @@ func (oc *OIDCController) Onboard() {
 		oc.SendInternalServerError(err)
 		return
 	}
-	d := &oidcUserData{}
+	d := &oidc.UserInfo{}
 	err = json.Unmarshal([]byte(userInfoStr), &d)
 	if err != nil {
 		oc.SendInternalServerError(err)
 		return
 	}
+	gids, err := group.PopulateGroup(models.UserGroupsFromName(d.Groups, common.OIDCGroupType))
+	if err != nil {
+		log.Warningf("Failed to populate group user will have empty group list. username: %s", username)
+	}
 	oidcUser := models.OIDCUser{
 		SubIss: d.Subject + d.Issuer,
 		Secret: s,
@@ -212,7 +202,7 @@ func (oc *OIDCController) Onboard() {
 		Username:     username,
 		Realname:     d.Username,
 		Email:        email,
-		GroupIDs:     d.GroupIDs,
+		GroupIDs:     gids,
 		OIDCUserMeta: &oidcUser,
 		Comment:      oidcUserComment,
 	}
@@ -220,7 +210,7 @@ func (oc *OIDCController) Onboard() {
 	err = dao.OnBoardOIDCUser(&user)
 	if err != nil {
 		if strings.Contains(err.Error(), dao.ErrDupUser.Error()) {
-			oc.RenderError(http.StatusConflict, "Conflict in username, the user with same username has been onboarded.")
+			oc.RenderError(http.StatusConflict, "Conflict, the user with same username or email has been onboarded.")
 			return
 		}
 		oc.SendInternalServerError(err)

src/core/filter/security.go

@@ -239,18 +239,8 @@ func (oc *oidcCliReqCtxModifier) Modify(ctx *beegoctx.Context) bool {
 	if !ok {
 		return false
 	}
-
-	user, err := dao.GetUser(models.User{
-		Username: username,
-	})
+	user, err := oidc.VerifySecret(ctx.Request.Context(), username, secret)
 	if err != nil {
-		log.Errorf("Failed to get user: %v", err)
-		return false
-	}
-	if user == nil {
-		return false
-	}
-	if err := oidc.VerifySecret(ctx.Request.Context(), user.UserID, secret); err != nil {
 		log.Errorf("Failed to verify secret: %v", err)
 		return false
 	}
@@ -289,11 +279,17 @@ func (it *idTokenReqCtxModifier) Modify(ctx *beegoctx.Context) bool {
 		log.Warning("User matches token's claims is not onboarded.")
 		return false
 	}
-	groupNames := oidc.GroupsFromToken(claims)
-	groups := models.UserGroupsFromName(groupNames, common.OIDCGroupType)
-	u.GroupIDs, err = group.PopulateGroup(groups)
+	settings, err := config.OIDCSetting()
 	if err != nil {
-		log.Errorf("Failed to get group ID list for OIDC user: %s, error: %v", u.Username, err)
+		log.Errorf("Failed to get OIDC settings, error: %v", err)
+	}
+	if groupNames, ok := oidc.GroupsFromClaims(claims, settings.GroupsClaim); ok {
+		groups := models.UserGroupsFromName(groupNames, common.OIDCGroupType)
+		u.GroupIDs, err = group.PopulateGroup(groups)
+		if err != nil {
+			log.Errorf("Failed to get group ID list for OIDC user: %s, error: %v", u.Username, err)
+			return false
+		}
 	}
 	pm := config.GlobalProjectMgr
 	sc := local.NewSecurityContext(u, pm)