From 714f989759f3231873d974d4a5d618c63fda3043 Mon Sep 17 00:00:00 2001 From: Alvaro Iradier Date: Tue, 5 May 2020 23:13:51 +0200 Subject: [PATCH 1/3] Add options for automatic onboarding and username claim - Add an option in the UI to enable or disable the automatic user onboarding - Add an option to specify the claim name where the username is retrieved from. Signed-off-by: Alvaro Iradier --- src/common/config/metadata/metadatalist.go | 2 + src/common/const.go | 2 + src/common/models/config.go | 2 + src/common/utils/oidc/helper.go | 31 +++- src/core/config/config.go | 2 + src/core/config/config_test.go | 4 + src/core/controllers/oidc.go | 161 +++++++++++------- .../config/auth/config-auth.component.html | 63 +++++-- src/portal/src/i18n/lang/en-us-lang.json | 4 + .../src/lib/components/config/config.ts | 4 + 10 files changed, 189 insertions(+), 86 deletions(-) diff --git a/src/common/config/metadata/metadatalist.go b/src/common/config/metadata/metadatalist.go index dfc635b43..cf3ccc62b 100644 --- a/src/common/config/metadata/metadatalist.go +++ b/src/common/config/metadata/metadatalist.go @@ -140,7 +140,9 @@ var ( {Name: common.OIDCClientSecret, Scope: UserScope, Group: OIDCGroup, ItemType: &PasswordType{}}, {Name: common.OIDCGroupsClaim, Scope: UserScope, Group: OIDCGroup, ItemType: &StringType{}}, {Name: common.OIDCScope, Scope: UserScope, Group: OIDCGroup, ItemType: &StringType{}}, + {Name: common.OIDCUserClaim, Scope: UserScope, Group: OIDCGroup, ItemType: &StringType{}}, {Name: common.OIDCVerifyCert, Scope: UserScope, Group: OIDCGroup, DefaultValue: "true", ItemType: &BoolType{}}, + {Name: common.OIDCAutoOnboard, Scope: UserScope, Group: OIDCGroup, DefaultValue: "false", ItemType: &BoolType{}}, {Name: common.WithChartMuseum, Scope: SystemScope, Group: BasicGroup, EnvKey: "WITH_CHARTMUSEUM", DefaultValue: "false", ItemType: &BoolType{}, Editable: true}, {Name: common.WithClair, Scope: SystemScope, Group: BasicGroup, EnvKey: "WITH_CLAIR", DefaultValue: "false", ItemType: &BoolType{}, Editable: true}, diff --git a/src/common/const.go b/src/common/const.go index 229cd7136..6fb7c68b2 100755 --- a/src/common/const.go +++ b/src/common/const.go @@ -106,7 +106,9 @@ const ( OIDCClientSecret = "oidc_client_secret" OIDCVerifyCert = "oidc_verify_cert" OIDCGroupsClaim = "oidc_groups_claim" + OIDCAutoOnboard = "oidc_auto_onboard" OIDCScope = "oidc_scope" + OIDCUserClaim = "oidc_user_claim" CfgDriverDB = "db" NewHarborAdminName = "admin@harbor.local" diff --git a/src/common/models/config.go b/src/common/models/config.go index e208bb4fd..806969fd6 100644 --- a/src/common/models/config.go +++ b/src/common/models/config.go @@ -81,11 +81,13 @@ type OIDCSetting struct { Name string `json:"name"` Endpoint string `json:"endpoint"` VerifyCert bool `json:"verify_cert"` + AutoOnboard bool `json:"auto_onboard"` ClientID string `json:"client_id"` ClientSecret string `json:"client_secret"` GroupsClaim string `json:"groups_claim"` RedirectURL string `json:"redirect_url"` Scope []string `json:"scope"` + UserClaim string `json:"user_claim"` } // QuotaSetting wraps the settings for Quota diff --git a/src/common/utils/oidc/helper.go b/src/common/utils/oidc/helper.go index 5a9a1f416..0ba24b8f1 100644 --- a/src/common/utils/oidc/helper.go +++ b/src/common/utils/oidc/helper.go @@ -19,16 +19,17 @@ import ( "crypto/tls" "errors" "fmt" - gooidc "github.com/coreos/go-oidc" - "github.com/goharbor/harbor/src/common/models" - "github.com/goharbor/harbor/src/core/config" - "github.com/goharbor/harbor/src/lib/log" - "golang.org/x/oauth2" "net/http" "strings" "sync" "sync/atomic" "time" + + gooidc "github.com/coreos/go-oidc" + "github.com/goharbor/harbor/src/common/models" + "github.com/goharbor/harbor/src/core/config" + "github.com/goharbor/harbor/src/lib/log" + "golang.org/x/oauth2" ) const ( @@ -294,7 +295,7 @@ func userInfoFromRemote(ctx context.Context, token *Token, setting models.OIDCSe if err != nil { return nil, err } - return userInfoFromClaims(u, setting.GroupsClaim) + return userInfoFromClaims(u, setting.GroupsClaim, setting.UserClaim) } func userInfoFromIDToken(ctx context.Context, token *Token, setting models.OIDCSetting) (*UserInfo, error) { @@ -305,14 +306,28 @@ func userInfoFromIDToken(ctx context.Context, token *Token, setting models.OIDCS if err != nil { return nil, err } - return userInfoFromClaims(idt, setting.GroupsClaim) + + return userInfoFromClaims(idt, setting.GroupsClaim, setting.UserClaim) } -func userInfoFromClaims(c claimsProvider, g string) (*UserInfo, error) { +func userInfoFromClaims(c claimsProvider, g, u string) (*UserInfo, error) { res := &UserInfo{} if err := c.Claims(res); err != nil { return nil, err } + if u != "" { + allClaims := make(map[string]interface{}) + if err := c.Claims(&allClaims); err != nil { + return nil, err + } + + if username, ok := allClaims[u].(string); !ok { + log.Warningf("OIDC. Failed to recover Username from claim. Claim '%s' is empty", u) + } else { + res.Username = username + } + + } res.Groups, res.hasGroupClaim = GroupsFromClaims(c, g) return res, nil } diff --git a/src/core/config/config.go b/src/core/config/config.go index 4a24659eb..a67c91c52 100755 --- a/src/core/config/config.go +++ b/src/core/config/config.go @@ -440,11 +440,13 @@ func OIDCSetting() (*models.OIDCSetting, error) { Name: cfgMgr.Get(common.OIDCName).GetString(), Endpoint: cfgMgr.Get(common.OIDCEndpoint).GetString(), VerifyCert: cfgMgr.Get(common.OIDCVerifyCert).GetBool(), + AutoOnboard: cfgMgr.Get(common.OIDCAutoOnboard).GetBool(), ClientID: cfgMgr.Get(common.OIDCCLientID).GetString(), ClientSecret: cfgMgr.Get(common.OIDCClientSecret).GetString(), GroupsClaim: cfgMgr.Get(common.OIDCGroupsClaim).GetString(), RedirectURL: extEndpoint + common.OIDCCallbackPath, Scope: scope, + UserClaim: cfgMgr.Get(common.OIDCUserClaim).GetString(), }, nil } diff --git a/src/core/config/config_test.go b/src/core/config/config_test.go index 0993453e7..5a78f9baf 100644 --- a/src/core/config/config_test.go +++ b/src/core/config/config_test.go @@ -253,8 +253,10 @@ func TestOIDCSetting(t *testing.T) { common.OIDCName: "test", common.OIDCEndpoint: "https://oidc.test", common.OIDCVerifyCert: "true", + common.OIDCAutoOnboard: "false", common.OIDCScope: "openid, profile", common.OIDCGroupsClaim: "my_group", + common.OIDCUserClaim: "username", common.OIDCCLientID: "client", common.OIDCClientSecret: "secret", common.ExtEndpoint: "https://harbor.test", @@ -266,8 +268,10 @@ func TestOIDCSetting(t *testing.T) { assert.Equal(t, "https://oidc.test", v.Endpoint) assert.True(t, v.VerifyCert) assert.Equal(t, "my_group", v.GroupsClaim) + assert.False(t, v.AutoOnboard) assert.Equal(t, "client", v.ClientID) assert.Equal(t, "secret", v.ClientSecret) assert.Equal(t, "https://harbor.test/c/oidc/callback", v.RedirectURL) assert.ElementsMatch(t, []string{"openid", "profile"}, v.Scope) + assert.Equal(t, "username", v.UserClaim) } diff --git a/src/core/controllers/oidc.go b/src/core/controllers/oidc.go index 170c572d8..525686d1e 100644 --- a/src/core/controllers/oidc.go +++ b/src/core/controllers/oidc.go @@ -17,10 +17,11 @@ package controllers import ( "encoding/json" "fmt" - "github.com/goharbor/harbor/src/common/dao/group" "net/http" "strings" + "github.com/goharbor/harbor/src/common/dao/group" + "github.com/goharbor/harbor/src/common" "github.com/goharbor/harbor/src/common/dao" "github.com/goharbor/harbor/src/common/models" @@ -122,30 +123,100 @@ func (oc *OIDCController) Callback() { } oc.SetSession(tokenKey, tokenBytes) - if u == nil { - oc.SetSession(userInfoKey, string(ouDataStr)) - oc.Controller.Redirect(fmt.Sprintf("/oidc-onboard?username=%s", strings.Replace(info.Username, " ", "_", -1)), - http.StatusFound) - } else { - 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) - return - } - _, t, err := secretAndToken(tokenBytes) - oidcUser.Token = t - if err := dao.UpdateOIDCUser(oidcUser); err != nil { - oc.SendInternalServerError(err) - return - } - oc.PopulateUserSession(*u) - oc.Controller.Redirect("/", http.StatusFound) + oidcSettings, err := config.OIDCSetting() + if err != nil { + oc.SendInternalServerError(err) + return } + + if u == nil { + // Recover the username from d.Username by default + username := info.Username + + // Fix blanks in username + username = strings.Replace(username, " ", "_", -1) + + // If automatic onboard is enabled, skip the onboard page + if oidcSettings.AutoOnboard { + log.Debug("Doing automatic onboarding\n") + user, onboarded := userOnboard(oc, info, username, tokenBytes) + if onboarded == false { + log.Error("User not onboarded\n") + return + } + log.Debug("User automatically onboarded\n") + u = user + } else { + oc.SetSession(userInfoKey, string(ouDataStr)) + oc.Controller.Redirect(fmt.Sprintf("/oidc-onboard?username=%s", username), http.StatusFound) + // Once redirected, no further actions are done + return + } + } + + 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) + return + } + _, t, err := secretAndToken(tokenBytes) + oidcUser.Token = t + if err := dao.UpdateOIDCUser(oidcUser); err != nil { + oc.SendInternalServerError(err) + return + } + oc.PopulateUserSession(*u) + oc.Controller.Redirect("/", http.StatusFound) + +} + +func userOnboard(oc *OIDCController, info *oidc.UserInfo, username string, tokenBytes []byte) (*models.User, bool) { + s, t, err := secretAndToken(tokenBytes) + if err != nil { + oc.SendInternalServerError(err) + return nil, false + } + + gids, err := group.PopulateGroup(models.UserGroupsFromName(info.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: info.Subject + info.Issuer, + Secret: s, + Token: t, + } + + user := models.User{ + Username: username, + Realname: username, + Email: info.Email, + GroupIDs: gids, + OIDCUserMeta: &oidcUser, + Comment: oidcUserComment, + } + + log.Debugf("User created: %+v\n", user) + + err = dao.OnBoardOIDCUser(&user) + if err != nil { + if strings.Contains(err.Error(), dao.ErrDupUser.Error()) { + oc.RenderError(http.StatusConflict, "Conflict, the user with same username or email has been onboarded.") + return nil, false + } + + oc.SendInternalServerError(err) + oc.DelSession(userInfoKey) + return nil, false + } + + return &user, true } // Onboard handles the request to onboard a user authenticated via OIDC provider @@ -176,51 +247,19 @@ func (oc *OIDCController) Onboard() { oc.SendBadRequestError(errors.New("Failed to get OIDC token from session")) return } - s, t, err := secretAndToken(tb) - if err != nil { - oc.SendInternalServerError(err) - return - } + d := &oidc.UserInfo{} - err = json.Unmarshal([]byte(userInfoStr), &d) + 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, - Token: t, - } - email := d.Email - user := models.User{ - Username: username, - Realname: d.Username, - Email: email, - GroupIDs: gids, - OIDCUserMeta: &oidcUser, - Comment: oidcUserComment, - } - - err = dao.OnBoardOIDCUser(&user) - if err != nil { - if strings.Contains(err.Error(), dao.ErrDupUser.Error()) { - oc.RenderError(http.StatusConflict, "Conflict, the user with same username or email has been onboarded.") - return - } - oc.SendInternalServerError(err) + if user, onboarded := userOnboard(oc, d, username, tb); onboarded { + user.OIDCUserMeta = nil oc.DelSession(userInfoKey) - return + oc.PopulateUserSession(*user) } - - user.OIDCUserMeta = nil - oc.DelSession(userInfoKey) - oc.PopulateUserSession(user) } func secretAndToken(tokenBytes []byte) (string, string, error) { diff --git a/src/portal/src/app/config/auth/config-auth.component.html b/src/portal/src/app/config/auth/config-auth.component.html index 794249a8c..a5d39bcee 100644 --- a/src/portal/src/app/config/auth/config-auth.component.html +++ b/src/portal/src/app/config/auth/config-auth.component.html @@ -374,24 +374,53 @@ [(ngModel)]="currentConfig.oidc_scope.value" id="oidcScope" size="40" required [disabled]="disabled(currentConfig.oidc_scope)" pattern="^(\w+,){0,}openid(,\w+){0,}$" /> {{'TOOLTIP.SCOPE_REQUIRED' | translate}} - - - - - - - + + + + + + + + + + + + + + + + +
{{ 'CONFIG.OIDC.OIDC_REDIREC_URL' | translate}} - {{redirectUrl}}/c/oidc/callback
+ {{redirectUrl}}/c/oidc/callback +
diff --git a/src/portal/src/i18n/lang/en-us-lang.json b/src/portal/src/i18n/lang/en-us-lang.json index 00284b26c..41ca7e579 100644 --- a/src/portal/src/i18n/lang/en-us-lang.json +++ b/src/portal/src/i18n/lang/en-us-lang.json @@ -102,6 +102,8 @@ "OIDC_VERIFYCERT": "Uncheck this box if your OIDC server is hosted via self-signed certificate.", "OIDC_GROUP_CLAIM": "The name of Claim in the ID token whose value is the list of group names.", "OIDC_GROUP_CLAIM_WARNING": "It can only contain letters, numbers, underscores, and the input length is no more than 256 characters.", + "OIDC_AUTOONBOARD": "Skip the onboarding screen, so user cannot change its username. Username is provided from ID Token", + "OIDC_USER_CLAIM": "The name of the claim in the ID Token where the username is retrieved from. If not specified, it will default to 'name'", "NEW_SECRET": "The secret must longer than 8 chars with at least 1 uppercase letter, 1 lowercase letter and 1 number" }, "PLACEHOLDER": { @@ -911,6 +913,8 @@ "CLIENTSECRET": "OIDC Client Secret", "SCOPE": "OIDC Scope", "OIDC_VERIFYCERT": "Verify Certificate", + "OIDC_AUTOONBOARD": "Automatic onboarding", + "USER_CLAIM": "OIDC Username Claim", "OIDC_SETNAME": "Set OIDC Username", "OIDC_SETNAMECONTENT": "You must create a Harbor username the first time when authenticating via a third party(OIDC).This will be used within Harbor to be associated with projects, roles, etc.", "OIDC_USERNAME": "Username", diff --git a/src/portal/src/lib/components/config/config.ts b/src/portal/src/lib/components/config/config.ts index 6149104d0..b32047b5b 100644 --- a/src/portal/src/lib/components/config/config.ts +++ b/src/portal/src/lib/components/config/config.ts @@ -98,7 +98,9 @@ export class Configuration { oidc_client_id?: StringValueItem; oidc_client_secret?: StringValueItem; oidc_verify_cert?: BoolValueItem; + oidc_auto_onboard?: BoolValueItem; oidc_scope?: StringValueItem; + oidc_user_claim?: StringValueItem; count_per_project: NumberValueItem; storage_per_project: NumberValueItem; cfg_expiration: NumberValueItem; @@ -155,8 +157,10 @@ export class Configuration { this.oidc_client_id = new StringValueItem('', true); this.oidc_client_secret = new StringValueItem('', true); this.oidc_verify_cert = new BoolValueItem(false, true); + this.oidc_auto_onboard = new BoolValueItem(false, true); this.oidc_scope = new StringValueItem('', true); this.oidc_groups_claim = new StringValueItem('', true); + this.oidc_user_claim = new StringValueItem('', true); this.count_per_project = new NumberValueItem(-1, true); this.storage_per_project = new NumberValueItem(-1, true); } From 6f88ff7429479d5c36c43f92269b11b6dcb644af Mon Sep 17 00:00:00 2001 From: Alvaro Iradier Date: Wed, 6 May 2020 02:02:06 +0200 Subject: [PATCH 2/3] Fix test suite and add test for userClaim Signed-off-by: Alvaro Iradier --- src/common/utils/oidc/helper_test.go | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/common/utils/oidc/helper_test.go b/src/common/utils/oidc/helper_test.go index 06dbac0de..96b2970ff 100644 --- a/src/common/utils/oidc/helper_test.go +++ b/src/common/utils/oidc/helper_test.go @@ -175,6 +175,7 @@ func TestUserInfoFromClaims(t *testing.T) { s := []struct { input map[string]interface{} groupClaim string + userClaim string expect *UserInfo }{ { @@ -184,6 +185,7 @@ func TestUserInfoFromClaims(t *testing.T) { "groups": []interface{}{"g1", "g2"}, }, groupClaim: "grouplist", + userClaim: "", expect: &UserInfo{ Issuer: "", Subject: "", @@ -200,6 +202,7 @@ func TestUserInfoFromClaims(t *testing.T) { "groups": []interface{}{"g1", "g2"}, }, groupClaim: "groups", + userClaim: "", expect: &UserInfo{ Issuer: "", Subject: "", @@ -218,6 +221,7 @@ func TestUserInfoFromClaims(t *testing.T) { "groupclaim": []interface{}{}, }, groupClaim: "groupclaim", + userClaim: "", expect: &UserInfo{ Issuer: "issuer", Subject: "subject000", @@ -227,9 +231,26 @@ func TestUserInfoFromClaims(t *testing.T) { hasGroupClaim: true, }, }, + { + input: map[string]interface{}{ + "name": "Alvaro", + "email": "airadier@gmail.com", + "groups": []interface{}{"g1", "g2"}, + }, + groupClaim: "grouplist", + userClaim: "email", + expect: &UserInfo{ + Issuer: "", + Subject: "", + Username: "airadier@gmail.com", + Email: "airadier@gmail.com", + Groups: []string{}, + hasGroupClaim: false, + }, + }, } for _, tc := range s { - out, err := userInfoFromClaims(&fakeClaims{tc.input}, tc.groupClaim) + out, err := userInfoFromClaims(&fakeClaims{tc.input}, tc.groupClaim, tc.userClaim) assert.Nil(t, err) assert.Equal(t, *tc.expect, *out) } From 81a7239c66a63aca2ec8e9746ede9957fd759754 Mon Sep 17 00:00:00 2001 From: Alvaro Iradier Date: Sun, 24 May 2020 23:43:29 +0200 Subject: [PATCH 3/3] Better error handling * Raise an internal error if username claim is not found, instead of just logging a warning * Don't remove userInfoKey for session on error when it is not required * Rename "OIDC Username Claim" to just "Username claim" Signed-off-by: Alvaro Iradier --- src/common/utils/oidc/helper.go | 8 ++++---- src/core/controllers/oidc.go | 2 +- src/portal/src/i18n/lang/en-us-lang.json | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/common/utils/oidc/helper.go b/src/common/utils/oidc/helper.go index 0ba24b8f1..1b4981156 100644 --- a/src/common/utils/oidc/helper.go +++ b/src/common/utils/oidc/helper.go @@ -321,11 +321,11 @@ func userInfoFromClaims(c claimsProvider, g, u string) (*UserInfo, error) { return nil, err } - if username, ok := allClaims[u].(string); !ok { - log.Warningf("OIDC. Failed to recover Username from claim. Claim '%s' is empty", u) - } else { - res.Username = username + username, ok := allClaims[u].(string) + if !ok { + return nil, fmt.Errorf("OIDC. Failed to recover Username from claim. Claim '%s' is invalid or not a string", u) } + res.Username = username } res.Groups, res.hasGroupClaim = GroupsFromClaims(c, g) diff --git a/src/core/controllers/oidc.go b/src/core/controllers/oidc.go index 525686d1e..20f3a971e 100644 --- a/src/core/controllers/oidc.go +++ b/src/core/controllers/oidc.go @@ -212,7 +212,6 @@ func userOnboard(oc *OIDCController, info *oidc.UserInfo, username string, token } oc.SendInternalServerError(err) - oc.DelSession(userInfoKey) return nil, false } @@ -260,6 +259,7 @@ func (oc *OIDCController) Onboard() { oc.DelSession(userInfoKey) oc.PopulateUserSession(*user) } + } func secretAndToken(tokenBytes []byte) (string, string, error) { diff --git a/src/portal/src/i18n/lang/en-us-lang.json b/src/portal/src/i18n/lang/en-us-lang.json index 41ca7e579..47c538879 100644 --- a/src/portal/src/i18n/lang/en-us-lang.json +++ b/src/portal/src/i18n/lang/en-us-lang.json @@ -914,7 +914,7 @@ "SCOPE": "OIDC Scope", "OIDC_VERIFYCERT": "Verify Certificate", "OIDC_AUTOONBOARD": "Automatic onboarding", - "USER_CLAIM": "OIDC Username Claim", + "USER_CLAIM": "Username Claim", "OIDC_SETNAME": "Set OIDC Username", "OIDC_SETNAMECONTENT": "You must create a Harbor username the first time when authenticating via a third party(OIDC).This will be used within Harbor to be associated with projects, roles, etc.", "OIDC_USERNAME": "Username",