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)