Populate user groups during OIDC CLI secret verification

This commit refactors the flow to populate user info and verify CLI
secret in OIDC authentication.

It will call the `userinfo` backend of OIDC backend and fallback to
using the ID token if userinfo is not supported by the backend.

It also makes sure the token will be persisted if it's refreshed during
this procedure.

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

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)