Repository: dexidp/dex
Pull request #3063 (2023-08-04 15:33:44)

add regex for oidc group matching
<!--
Thank you for sending a pull request! Here are some tips for contributors:

1. Fill the description template below.
2. Sign a DCO (if you haven't already signed it).
3. Include appropriate tests (if necessary). Make sure that all CI checks passed.
4. If the Pull Request is a work in progress, make use of GitHub's "Draft PR" feature and mark it as such.
-->

#### Overview

<!-- Describe your changes briefly here. -->

Add a group regex matcher to the OIDC connector config.

#### What this PR does / why we need it

<!--
- Please state in detail why we need this PR and what it solves.
- If your PR closes some of the existing issues, please add links to them here.
  Mentioned issues will be automatically closed.
  Usage: "Closes #<issue number>", or "Closes (paste link of issue)"
-->

Sometimes the IDP's can send very long list of groups, not all IDP's support group filters from the IDP side.

#### Special notes for your reviewer

I tested this in the unit tests, ~but not sure how to test this in a live environment.~ and in a live environment, and the filter works as expected.

#### Does this PR introduce a user-facing change?

It adds a new config field for OIDC: `groupsRegex`.

<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".
-->

```release-note
ADD groups regex config for oidc connector.
```


sagikazarmark: @jacksonargo thanks for the contribution!

Two thoughts came to my mind:

- People generally want to deny authentication with filters like this. I'm not saying it doesn't make sense, but it can be confusing for some, so we may want to structure config/document this feature to avoid any confusion.
- Would it make sense to support capture groups not just to match, but extract parts of group names?
jacksonargo: > People generally want to deny authentication with filters like this. I'm not saying it doesn't make sense, but it can be confusing for some, so we may want to structure config/document this feature to avoid any confusion.

Ya, makes sense to me. Would `groupsFilter` be a better name?

> Would it make sense to support capture groups not just to match, but extract parts of group names?

Yes probably! Didn't occur to me at the time.
st-sidjoshi: Your thoughts @sagikazarmark ?

Patch:
diff --git a/connector/oidc/oidc.go b/connector/oidc/oidc.go
index 21129f22..1495be4b 100644
--- a/connector/oidc/oidc.go
+++ b/connector/oidc/oidc.go
@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"net/http"
 	"net/url"
+	"regexp"
 	"strings"
 	"time"
 
@@ -93,6 +94,7 @@ type Config struct {
 	// ClaimMutations holds all claim mutations options
 	ClaimMutations struct {
 		NewGroupFromClaims []NewGroupFromClaims `json:"newGroupFromClaims"`
+		FilterGroupClaims  FilterGroupClaims    `json:"filterGroupClaims"`
 	} `json:"claimModifications"`
 }
 
@@ -112,6 +114,12 @@ type NewGroupFromClaims struct {
 	Prefix string `json:"prefix"`
 }
 
+// FilterGroupClaims is a regex filter for to keep only the matching groups.
+// This is useful when the groups list is too large to fit within an HTTP header.
+type FilterGroupClaims struct {
+	GroupsFilter string `json:"groupsFilter"`
+}
+
 // Domains that don't support basic auth. golang.org/x/oauth2 has an internal
 // list, but it only matches specific URLs, not top level domains.
 var brokenAuthHeaderDomains = []string{
@@ -184,6 +192,14 @@ func (c *Config) Open(id string, logger log.Logger) (conn connector.Connector, e
 		c.PromptType = "consent"
 	}
 
+	var groupsFilter *regexp.Regexp
+	if c.ClaimMutations.FilterGroupClaims.GroupsFilter != "" {
+		groupsFilter, err = regexp.Compile(c.ClaimMutations.FilterGroupClaims.GroupsFilter)
+		if err != nil {
+			logger.Warnf("ignoring invalid regex `%s`", c.ClaimMutations.FilterGroupClaims.GroupsFilter)
+		}
+	}
+
 	clientID := c.ClientID
 	return &oidcConnector{
 		provider:    provider,
@@ -214,6 +230,7 @@ func (c *Config) Open(id string, logger log.Logger) (conn connector.Connector, e
 		emailKey:                  c.ClaimMapping.EmailKey,
 		groupsKey:                 c.ClaimMapping.GroupsKey,
 		newGroupFromClaims:        c.ClaimMutations.NewGroupFromClaims,
+		groupsFilter:              groupsFilter,
 	}, nil
 }
 
@@ -243,6 +260,7 @@ type oidcConnector struct {
 	emailKey                  string
 	groupsKey                 string
 	newGroupFromClaims        []NewGroupFromClaims
+	groupsFilter              *regexp.Regexp
 }
 
 func (c *oidcConnector) Close() error {
@@ -446,6 +464,9 @@ func (c *oidcConnector) createIdentity(ctx context.Context, identity connector.I
 		if found {
 			for _, v := range vs {
 				if s, ok := v.(string); ok {
+					if c.groupsFilter != nil && !c.groupsFilter.MatchString(s) {
+						continue
+					}
 					groups = append(groups, s)
 				} else {
 					return identity, fmt.Errorf("malformed \"%v\" claim", groupsKey)


Test patch:
diff --git a/connector/oidc/oidc_test.go b/connector/oidc/oidc_test.go
index 4bb84a40..428a45cc 100644
--- a/connector/oidc/oidc_test.go
+++ b/connector/oidc/oidc_test.go
@@ -62,6 +62,7 @@ func TestHandleCallback(t *testing.T) {
 		expectPreferredUsername   string
 		expectedEmailField        string
 		token                     map[string]interface{}
+		groupsRegex               string
 		newGroupFromClaims        []NewGroupFromClaims
 	}{
 		{
@@ -362,6 +363,23 @@ func TestHandleCallback(t *testing.T) {
 				"non-string-claim2": 666,
 			},
 		},
+		{
+			name:               "filterGroupClaims",
+			userIDKey:          "", // not configured
+			userNameKey:        "", // not configured
+			groupsRegex:        `^.*\d$`,
+			expectUserID:       "subvalue",
+			expectUserName:     "namevalue",
+			expectGroups:       []string{"group1", "group2"},
+			expectedEmailField: "emailvalue",
+			token: map[string]interface{}{
+				"sub":            "subvalue",
+				"name":           "namevalue",
+				"groups":         []string{"group1", "group2", "groupA", "groupB"},
+				"email":          "emailvalue",
+				"email_verified": true,
+			},
+		},
 	}
 
 	for _, tc := range tests {
@@ -398,6 +416,7 @@ func TestHandleCallback(t *testing.T) {
 			config.ClaimMapping.EmailKey = tc.emailKey
 			config.ClaimMapping.GroupsKey = tc.groupsKey
 			config.ClaimMutations.NewGroupFromClaims = tc.newGroupFromClaims
+			config.ClaimMutations.FilterGroupClaims.GroupsFilter = tc.groupsRegex
 
 			conn, err := newConnector(config)
 			if err != nil {