fix(oidc): apply groupsFilter to single-string groups claim - #4920
Open
vidigoat wants to merge 1 commit into
Open
fix(oidc): apply groupsFilter to single-string groups claim#4920vidigoat wants to merge 1 commit into
vidigoat wants to merge 1 commit into
Conversation
When an upstream IdP returns the groups claim as a single string rather than an array, dex assigned it directly to the identity's groups and skipped the configured groupsFilter entirely. Operators use groupsFilter as an allow-list to restrict which upstream groups propagate into the ID token (and downstream RBAC), so for those IdPs the filter silently failed open and excluded groups leaked through. Fold the single-string fallback into the array path so groupsFilter (and the existing per-group handling) applies uniformly. Add a regression test covering a filtered single-string groups claim. Signed-off-by: Vidit Patankar <vidit.patankar16@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes OIDC group-claim mutation behavior so the configured groupsFilter is applied consistently whether the upstream IdP returns the groups claim as an array or a single string, preventing filtered-out groups from leaking into identity.Groups.
Changes:
- Fold the single-string groups-claim fallback into the existing array-processing path so the same filtering and per-group handling runs for both formats.
- Add a regression test covering the single-string groups claim with
groupsFilterconfigured (ensuring the group is filtered out as expected).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| connector/oidc/oidc.go | Routes single-string groups claim through the same per-group processing path so groupsFilter is applied uniformly. |
| connector/oidc/oidc_test.go | Adds a regression test to ensure a string-form groups claim is filtered by groupsFilter the same way as an array-form claim. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Apply the configured
groupsFilterto the groups claim even when the upstream IdP returns it as a single string instead of an array.What this PR does / why we need it
createIdentityin the OIDC connector appliesgroupsFilter(fromclaimMutations.filterGroupClaims.groupsFilter) inside the loop that handles the array form of the groups claim. The single-string fallback, however, assigned the group directly:Some IdPs deliver the groups claim as a single string (which is exactly why this fallback exists). For those providers
groupsFilterwas silently a no-op: a group the operator configured the filter to exclude still landed inidentity.Groups, and therefore in the ID token and every downstream authorization decision — failing open, with no signal to the operator.This folds the single-string case into the array path (
vs = []interface{}{g}; found = true) sogroupsFilter— and the existing per-group prefix/map handling below it — applies uniformly. Behaviour is unchanged when no filter is configured (the existingsingularGroupResponseAsStringtest still yields["group1"]).Special notes for your reviewer
Added a regression test (
filterGroupClaimsString) toTestHandleCallback: a single-string groups claim of"groupA"with filter^.*\d$must be filtered out. It fails onmaster(leaks["groupA"]) and passes with this change.go test ./connector/oidc/...,gofmt, andgo vetare clean.Disclosure: I used an LLM to help find and draft this fix; I reproduced the bug, wrote the test, and verified everything myself.