From 64af09d52bc814d3e1621d5114302406e9394a44 Mon Sep 17 00:00:00 2001 From: Daniel Jiang Date: Mon, 11 Nov 2019 00:25:00 +0800 Subject: [PATCH] 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 --- src/common/utils/oidc/helper.go | 171 ++++++++++++++++++------ src/common/utils/oidc/helper_test.go | 186 +++++++++++++++++++++++++-- src/common/utils/oidc/secret.go | 105 +++++++-------- src/common/utils/oidc/secret_test.go | 8 +- src/common/utils/oidc/testutils.go | 11 +- src/core/controllers/oidc.go | 44 +++---- src/core/filter/security.go | 26 ++-- 7 files changed, 397 insertions(+), 154 deletions(-) diff --git a/src/common/utils/oidc/helper.go b/src/common/utils/oidc/helper.go index 7cbec4aa9..9eefbf0c9 100644 --- a/src/common/utils/oidc/helper.go +++ b/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 diff --git a/src/common/utils/oidc/helper_test.go b/src/common/utils/oidc/helper_test.go index 8270b44f7..06dbac0de 100644 --- a/src/common/utils/oidc/helper_test.go +++ b/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) } } diff --git a/src/common/utils/oidc/secret.go b/src/common/utils/oidc/secret.go index 861dc0e8d..7c5d46cf3 100644 --- a/src/common/utils/oidc/secret.go +++ b/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) } diff --git a/src/common/utils/oidc/secret_test.go b/src/common/utils/oidc/secret_test.go index 1a376f54f..127d91837 100644 --- a/src/common/utils/oidc/secret_test.go +++ b/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) } diff --git a/src/common/utils/oidc/testutils.go b/src/common/utils/oidc/testutils.go index 0f37468fc..6ff429825 100644 --- a/src/common/utils/oidc/testutils.go +++ b/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. diff --git a/src/core/controllers/oidc.go b/src/core/controllers/oidc.go index 6594721b4..fd4388a4c 100644 --- a/src/core/controllers/oidc.go +++ b/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) diff --git a/src/core/filter/security.go b/src/core/filter/security.go index 3e71947f9..f6d6b887a 100644 --- a/src/core/filter/security.go +++ b/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)