Merge pull request #13312 from reasonerjt/oidc-admin-group

Add admin group support to OIDC auth mode

src/common/config/metadata/metadatalist.go

@@ -139,6 +139,7 @@ var (
 		{Name: common.OIDCCLientID, Scope: UserScope, Group: OIDCGroup, ItemType: &StringType{}},
 		{Name: common.OIDCClientSecret, Scope: UserScope, Group: OIDCGroup, ItemType: &PasswordType{}},
 		{Name: common.OIDCGroupsClaim, Scope: UserScope, Group: OIDCGroup, ItemType: &StringType{}},
+		{Name: common.OIDCAdminGroup, 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{}},

src/common/const.go

@@ -105,6 +105,7 @@ const (
 	OIDCCLientID                     = "oidc_client_id"
 	OIDCClientSecret                 = "oidc_client_secret"
 	OIDCVerifyCert                   = "oidc_verify_cert"
+	OIDCAdminGroup                   = "oidc_admin_group"
 	OIDCGroupsClaim                  = "oidc_groups_claim"
 	OIDCAutoOnboard                  = "oidc_auto_onboard"
 	OIDCScope                        = "oidc_scope"

src/common/models/config.go

@@ -85,6 +85,7 @@ type OIDCSetting struct {
 	ClientID     string   `json:"client_id"`
 	ClientSecret string   `json:"client_secret"`
 	GroupsClaim  string   `json:"groups_claim"`
+	AdminGroup   string   `json:"admin_group"`
 	RedirectURL  string   `json:"redirect_url"`
 	Scope        []string `json:"scope"`
 	UserClaim    string   `json:"user_claim"`

src/common/utils/oidc/helper.go

@@ -26,6 +26,8 @@ import (
 	"time"
 
 	gooidc "github.com/coreos/go-oidc"
+	"github.com/goharbor/harbor/src/common"
+	"github.com/goharbor/harbor/src/common/dao/group"
 	"github.com/goharbor/harbor/src/common/models"
 	"github.com/goharbor/harbor/src/core/config"
 	"github.com/goharbor/harbor/src/lib/log"
@@ -119,12 +121,13 @@ type Token struct {
 // 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
+	Issuer           string   `json:"iss"`
+	Subject          string   `json:"sub"`
+	Username         string   `json:"name"`
+	Email            string   `json:"email"`
+	Groups           []string `json:"groups"`
+	AdminGroupMember bool     `json:"admin_group_member"`
+	hasGroupClaim    bool
 }
 
 func getOauthConf() (*oauth2.Config, error) {
@@ -247,7 +250,7 @@ func UserInfoFromToken(ctx context.Context, token *Token) (*UserInfo, error) {
 		return nil, err
 	}
 	setting := provider.setting.Load().(models.OIDCSetting)
-	local, err := userInfoFromIDToken(ctx, token, setting)
+	local, err := UserInfoFromIDToken(ctx, token, setting)
 	if err != nil {
 		return nil, err
 	}
@@ -280,9 +283,11 @@ func mergeUserInfo(remote, local *UserInfo) *UserInfo {
 	}
 	if remote.hasGroupClaim {
 		res.Groups = remote.Groups
+		res.AdminGroupMember = remote.AdminGroupMember
 		res.hasGroupClaim = true
 	} else if local.hasGroupClaim {
 		res.Groups = local.Groups
+		res.AdminGroupMember = local.AdminGroupMember
 		res.hasGroupClaim = true
 	} else {
 		res.Groups = []string{}
@@ -300,10 +305,11 @@ func userInfoFromRemote(ctx context.Context, token *Token, setting models.OIDCSe
 	if err != nil {
 		return nil, err
 	}
-	return userInfoFromClaims(u, setting.GroupsClaim, setting.UserClaim)
+	return userInfoFromClaims(u, setting)
 }
 
-func userInfoFromIDToken(ctx context.Context, token *Token, setting models.OIDCSetting) (*UserInfo, error) {
+// UserInfoFromIDToken extract user info from ID token
+func UserInfoFromIDToken(ctx context.Context, token *Token, setting models.OIDCSetting) (*UserInfo, error) {
 	if token.RawIDToken == "" {
 		return nil, nil
 	}
@@ -312,34 +318,41 @@ func userInfoFromIDToken(ctx context.Context, token *Token, setting models.OIDCS
 		return nil, err
 	}
 
-	return userInfoFromClaims(idt, setting.GroupsClaim, setting.UserClaim)
+	return userInfoFromClaims(idt, setting)
 }
 
-func userInfoFromClaims(c claimsProvider, g, u string) (*UserInfo, error) {
+func userInfoFromClaims(c claimsProvider, setting models.OIDCSetting) (*UserInfo, error) {
 	res := &UserInfo{}
 	if err := c.Claims(res); err != nil {
 		return nil, err
 	}
-	if u != "" {
+	if setting.UserClaim != "" {
 		allClaims := make(map[string]interface{})
 		if err := c.Claims(&allClaims); err != nil {
 			return nil, err
 		}
 
-		username, ok := allClaims[u].(string)
+		username, ok := allClaims[setting.UserClaim].(string)
 		if !ok {
-			return nil, fmt.Errorf("OIDC. Failed to recover Username from claim. Claim '%s' is invalid or not a string", u)
+			return nil, fmt.Errorf("OIDC. Failed to recover Username from claim. Claim '%s' is invalid or not a string", setting.UserClaim)
 		}
 		res.Username = username
-
 	}
-	res.Groups, res.hasGroupClaim = GroupsFromClaims(c, g)
+	res.Groups, res.hasGroupClaim = groupsFromClaims(c, setting.GroupsClaim)
+	if len(setting.AdminGroup) > 0 {
+		for _, g := range res.Groups {
+			if g == setting.AdminGroup {
+				res.AdminGroupMember = true
+				break
+			}
+		}
+	}
 	return res, nil
 }
 
-// GroupsFromClaims fetches the group name list from claimprovider, such as decoded ID token.
+// 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) {
+func groupsFromClaims(gp claimsProvider, k string) ([]string, bool) {
 	res := make([]string, 0)
 	claimMap := make(map[string]interface{})
 	if err := gp.Claims(&claimMap); err != nil {
@@ -362,6 +375,33 @@ func GroupsFromClaims(gp claimsProvider, k string) ([]string, bool) {
 	return res, true
 }
 
+type populate func(groupNames []string) ([]int, error)
+
+func populateGroupsDB(groupNames []string) ([]int, error) {
+	return group.PopulateGroup(models.UserGroupsFromName(groupNames, common.OIDCGroupType))
+}
+
+// InjectGroupsToUser populates the group to DB and inject the group IDs to user model.
+// The third optional parm is for UT only.
+func InjectGroupsToUser(info *UserInfo, user *models.User, f ...populate) {
+	if info == nil || user == nil {
+		log.Warningf("user info or user model is nil, skip the func")
+		return
+	}
+	var populateGroups populate
+	if len(f) == 0 {
+		populateGroups = populateGroupsDB
+	} else {
+		populateGroups = f[0]
+	}
+	if gids, err := populateGroups(info.Groups); err != nil {
+		log.Warningf("failed to get group ID, error: %v, skip populating groups", err)
+	} else {
+		user.GroupIDs = gids
+	}
+	user.AdminRoleInAuth = info.AdminGroupMember
+}
+
 // Conn wraps connection info of an OIDC endpoint
 type Conn struct {
 	URL        string `json:"url"`

src/common/utils/oidc/helper_test.go

@@ -165,7 +165,7 @@ func TestGroupsFromClaim(t *testing.T) {
 
 	for _, tc := range m {
 
-		r, ok := GroupsFromClaims(&fakeClaims{tc.input}, tc.key)
+		r, ok := groupsFromClaims(&fakeClaims{tc.input}, tc.key)
 		assert.Equal(t, tc.expect, r)
 		assert.Equal(t, tc.ok, ok)
 	}
@@ -173,10 +173,9 @@ func TestGroupsFromClaim(t *testing.T) {
 
 func TestUserInfoFromClaims(t *testing.T) {
 	s := []struct {
-		input      map[string]interface{}
-		groupClaim string
-		userClaim  string
-		expect     *UserInfo
+		input   map[string]interface{}
+		setting models.OIDCSetting
+		expect  *UserInfo
 	}{
 		{
 			input: map[string]interface{}{
@@ -184,8 +183,12 @@ func TestUserInfoFromClaims(t *testing.T) {
 				"email":  "daniel@gmail.com",
 				"groups": []interface{}{"g1", "g2"},
 			},
-			groupClaim: "grouplist",
-			userClaim:  "",
+			setting: models.OIDCSetting{
+				Name:        "t1",
+				GroupsClaim: "grouplist",
+				UserClaim:   "",
+				AdminGroup:  "g1",
+			},
 			expect: &UserInfo{
 				Issuer:        "",
 				Subject:       "",
@@ -201,15 +204,20 @@ func TestUserInfoFromClaims(t *testing.T) {
 				"email":  "daniel@gmail.com",
 				"groups": []interface{}{"g1", "g2"},
 			},
-			groupClaim: "groups",
-			userClaim:  "",
+			setting: models.OIDCSetting{
+				Name:        "t2",
+				GroupsClaim: "groups",
+				UserClaim:   "",
+				AdminGroup:  "g1",
+			},
 			expect: &UserInfo{
-				Issuer:        "",
-				Subject:       "",
-				Username:      "Daniel",
-				Email:         "daniel@gmail.com",
-				Groups:        []string{"g1", "g2"},
-				hasGroupClaim: true,
+				Issuer:           "",
+				Subject:          "",
+				Username:         "Daniel",
+				Email:            "daniel@gmail.com",
+				Groups:           []string{"g1", "g2"},
+				AdminGroupMember: true,
+				hasGroupClaim:    true,
 			},
 		},
 		{
@@ -220,15 +228,20 @@ func TestUserInfoFromClaims(t *testing.T) {
 				"email":      "jack@gmail.com",
 				"groupclaim": []interface{}{},
 			},
-			groupClaim: "groupclaim",
-			userClaim:  "",
+			setting: models.OIDCSetting{
+				Name:        "t3",
+				GroupsClaim: "groupclaim",
+				UserClaim:   "",
+				AdminGroup:  "g1",
+			},
 			expect: &UserInfo{
-				Issuer:        "issuer",
-				Subject:       "subject000",
-				Username:      "jack",
-				Email:         "jack@gmail.com",
-				Groups:        []string{},
-				hasGroupClaim: true,
+				Issuer:           "issuer",
+				Subject:          "subject000",
+				Username:         "jack",
+				Email:            "jack@gmail.com",
+				Groups:           []string{},
+				hasGroupClaim:    true,
+				AdminGroupMember: false,
 			},
 		},
 		{
@@ -237,20 +250,25 @@ func TestUserInfoFromClaims(t *testing.T) {
 				"email":  "airadier@gmail.com",
 				"groups": []interface{}{"g1", "g2"},
 			},
-			groupClaim: "grouplist",
-			userClaim:  "email",
+			setting: models.OIDCSetting{
+				Name:        "t4",
+				GroupsClaim: "grouplist",
+				UserClaim:   "email",
+				AdminGroup:  "g1",
+			},
 			expect: &UserInfo{
-				Issuer:        "",
-				Subject:       "",
-				Username:      "airadier@gmail.com",
-				Email:         "airadier@gmail.com",
-				Groups:        []string{},
-				hasGroupClaim: false,
+				Issuer:           "",
+				Subject:          "",
+				Username:         "airadier@gmail.com",
+				Email:            "airadier@gmail.com",
+				Groups:           []string{},
+				hasGroupClaim:    false,
+				AdminGroupMember: false,
 			},
 		},
 	}
 	for _, tc := range s {
-		out, err := userInfoFromClaims(&fakeClaims{tc.input}, tc.groupClaim, tc.userClaim)
+		out, err := userInfoFromClaims(&fakeClaims{tc.input}, tc.setting)
 		assert.Nil(t, err)
 		assert.Equal(t, *tc.expect, *out)
 	}
@@ -347,3 +365,86 @@ func TestMergeUserInfo(t *testing.T) {
 		assert.Equal(t, *tc.expected, *m)
 	}
 }
+
+func TestInjectGroupsToUser(t *testing.T) {
+	cases := []struct {
+		userInfo *UserInfo
+		old      *models.User
+		new      *models.User
+	}{
+		{
+			userInfo: &UserInfo{
+				Issuer:           "issuer-yahoo",
+				Subject:          "subject-jim",
+				Username:         "jim",
+				Email:            "jim@gmail.com",
+				Groups:           []string{},
+				hasGroupClaim:    true,
+				AdminGroupMember: false,
+			},
+			old: &models.User{
+				Username:        "jim",
+				Email:           "jim@gmail.com",
+				GroupIDs:        []int{},
+				AdminRoleInAuth: false,
+			},
+			new: &models.User{
+				Username:        "jim",
+				Email:           "jim@gmail.com",
+				GroupIDs:        []int{},
+				AdminRoleInAuth: false,
+			},
+		},
+		{
+			userInfo: &UserInfo{
+				Issuer:           "issuer-yahoo",
+				Subject:          "subject-jim",
+				Username:         "jim",
+				Email:            "jim@gmail.com",
+				Groups:           []string{"1", "abc"},
+				hasGroupClaim:    true,
+				AdminGroupMember: true,
+			},
+			old: &models.User{
+				Username:        "jim",
+				Email:           "jim@gmail.com",
+				GroupIDs:        []int{},
+				AdminRoleInAuth: false,
+			},
+			new: &models.User{
+				Username:        "jim",
+				Email:           "jim@gmail.com",
+				GroupIDs:        []int{},
+				AdminRoleInAuth: true,
+			},
+		},
+		{
+			userInfo: &UserInfo{
+				Issuer:           "issuer-yahoo",
+				Subject:          "subject-jim",
+				Username:         "jim",
+				Email:            "jim@gmail.com",
+				Groups:           []string{"1", "2"},
+				hasGroupClaim:    true,
+				AdminGroupMember: true,
+			},
+			old: &models.User{
+				Username:        "jim",
+				Email:           "jim@gmail.com",
+				GroupIDs:        []int{},
+				AdminRoleInAuth: false,
+			},
+			new: &models.User{
+				Username:        "jim",
+				Email:           "jim@gmail.com",
+				GroupIDs:        []int{1, 2},
+				AdminRoleInAuth: true,
+			},
+		},
+	}
+	for _, c := range cases {
+		u := c.old
+		InjectGroupsToUser(c.userInfo, u, mockPopulateGroups)
+		assert.Equal(t, *c.new, *u)
+	}
+}

src/common/utils/oidc/secret.go

@@ -4,15 +4,14 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"github.com/goharbor/harbor/src/common"
+	"sync"
+
 	"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/core/config"
 	"github.com/goharbor/harbor/src/lib/errors"
 	"github.com/goharbor/harbor/src/lib/log"
-	"sync"
 )
 
 // SecretVerifyError wraps the different errors happened when verifying a secret for OIDC user.  When seeing this error,
@@ -117,12 +116,7 @@ func (dm *defaultManager) VerifySecret(ctx context.Context, username string, sec
 	if err != nil {
 		return nil, verifyError(err)
 	}
-	gids, err := group.PopulateGroup(models.UserGroupsFromName(info.Groups, common.OIDCGroupType))
-	if err != nil {
-		log.Warningf("failed to get group ID, error: %v, skip populating groups", err)
-	} else {
-		user.GroupIDs = gids
-	}
+	InjectGroupsToUser(info, user)
 	return user, nil
 }
 

src/common/utils/oidc/testutils.go

@@ -3,6 +3,8 @@ package oidc
 import (
 	"context"
 	"fmt"
+	"strconv"
+
 	"github.com/goharbor/harbor/src/common/models"
 )
 import "errors"
@@ -24,3 +26,14 @@ func (fv *fakeVerifier) VerifySecret(ctx context.Context, name string, secret st
 func SetHardcodeVerifierForTest(s string) {
 	m = &fakeVerifier{s}
 }
+func mockPopulateGroups(groupNames []string) ([]int, error) {
+	res := make([]int, 0)
+	for _, g := range groupNames {
+		id, err := strconv.Atoi(g)
+		if err != nil {
+			return res, err
+		}
+		res = append(res, id)
+	}
+	return res, nil
+}

src/core/config/config.go

@@ -440,6 +440,7 @@ func OIDCSetting() (*models.OIDCSetting, error) {
 		ClientID:     cfgMgr.Get(common.OIDCCLientID).GetString(),
 		ClientSecret: cfgMgr.Get(common.OIDCClientSecret).GetString(),
 		GroupsClaim:  cfgMgr.Get(common.OIDCGroupsClaim).GetString(),
+		AdminGroup:   cfgMgr.Get(common.OIDCAdminGroup).GetString(),
 		RedirectURL:  extEndpoint + common.OIDCCallbackPath,
 		Scope:        scope,
 		UserClaim:    cfgMgr.Get(common.OIDCUserClaim).GetString(),

src/core/controllers/oidc.go

@@ -20,8 +20,6 @@ import (
 	"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"
@@ -153,12 +151,7 @@ func (oc *OIDCController) Callback() {
 			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
+	oidc.InjectGroupsToUser(info, u)
 	oidcUser, err := dao.GetOIDCUserByUserID(u.UserID)
 	if err != nil {
 		oc.SendInternalServerError(err)
@@ -181,30 +174,24 @@ func userOnboard(oc *OIDCController, info *oidc.UserInfo, username string, token
 		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{
+	user := &models.User{
 		Username:     username,
 		Realname:     username,
 		Email:        info.Email,
-		GroupIDs:     gids,
 		OIDCUserMeta: &oidcUser,
 		Comment:      oidcUserComment,
 	}
+	oidc.InjectGroupsToUser(info, user)
 
-	log.Debugf("User created: %+v\n", user)
+	log.Debugf("User created: %+v\n", *user)
 
-	err = dao.OnBoardOIDCUser(&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.")
@@ -215,7 +202,7 @@ func userOnboard(oc *OIDCController, info *oidc.UserInfo, username string, token
 		return nil, false
 	}
 
-	return &user, true
+	return user, true
 }
 
 // Onboard handles the request to onboard a user authenticated via OIDC provider

src/server/middleware/security/idtoken.go

@@ -20,8 +20,6 @@ import (
 
 	"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/security"
 	"github.com/goharbor/harbor/src/common/security/local"
 	"github.com/goharbor/harbor/src/common/utils/oidc"
@@ -33,14 +31,16 @@ import (
 type idToken struct{}
 
 func (i *idToken) Generate(req *http.Request) security.Context {
-	log := log.G(req.Context())
-	if lib.GetAuthMode(req.Context()) != common.OIDCAuth {
+	ctx := req.Context()
+	log := log.G(ctx)
+	if lib.GetAuthMode(ctx) != common.OIDCAuth {
 		return nil
 	}
 	if !strings.HasPrefix(req.URL.Path, "/api") {
 		return nil
 	}
-	claims, err := oidc.VerifyToken(req.Context(), bearerToken(req))
+	token := bearerToken(req)
+	claims, err := oidc.VerifyToken(ctx, token)
 	if err != nil {
 		log.Warningf("failed to verify token: %v", err)
 		return nil
@@ -54,19 +54,17 @@ func (i *idToken) Generate(req *http.Request) security.Context {
 		log.Warning("user matches token's claims is not onboarded.")
 		return nil
 	}
-	settings, err := config.OIDCSetting()
+	setting, err := config.OIDCSetting()
 	if err != nil {
 		log.Errorf("failed to get OIDC settings: %v", err)
 		return nil
 	}
-	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: %v", u.Username, err)
-			return nil
-		}
+	info, err := oidc.UserInfoFromIDToken(ctx, &oidc.Token{RawIDToken: token}, *setting)
+	if err != nil {
+		log.Errorf("Failed to get user info from ID token: %v", err)
+		return nil
 	}
+	oidc.InjectGroupsToUser(info, u)
 	log.Debugf("an ID token security context generated for request %s %s", req.Method, req.URL.Path)
 	return local.NewSecurityContext(u, config.GlobalProjectMgr)
 }