Repository navigation
Split space-delimited group claims into separate groups - #100
Closed
afonsojanu wants to merge 1 commit into
Closed
afonsojanu wants to merge 1 commit into
afonsojanu wants to merge 1 commit into
Conversation
OIDC providers commonly return a multi-value claim such as groups or affiliation as a single space-delimited string, following the OAuth2 convention for scope-like claims, rather than a JSON array. See issue #88 and the related report in #77's comments. OIDCPlugin._create_update_groups and user_can_login both wrapped such a string in a one-element list instead of splitting it on whitespace. A claim of "staff member" ended up creating one bogus group literally named "staff member" instead of two separate groups, and an allowed_groups check against a single-word group name would silently reject users who were actually members, because the claim was never broken into individual group ids. Both call sites now split the claim on whitespace before treating it as a list of group ids, matching how a JSON array of the same values would already have been handled. Added coverage for group creation from a multi-value string claim and for the allowed_groups membership check.
Author
|
I'm closing out a lot of my older open PRs to bring the number down to something reasonable. This isn't about the fix being wrong, just trying to stop sitting on so many open at once. |
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
1 participant
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.
Closes #88.
_create_update_groupsanduser_can_loginboth handle a userinfo claim that can come back either as a list or as a single string, but the string branch in each just wraps the whole string in a one-element list rather than splitting it. Per the OAuth2/OIDC convention, a multi-value claim like a custom "affiliation" or "groups" claim that isn't covered by the default OpenIDSchema is returned by providers as a single space-delimited string, not JSON. That's exactly what #88 reports: a user with affiliations "staff" and "member" ends up in one group literally named "staff member" instead of two groups. The same pattern was independently spotted in a comment on #77, where it produced a group id like "member-reader-editor" after Plone's id normalization replaced the spaces with dashes.The more concerning side of this is
user_can_login: ifallowed_groupscontains a single-word group name and the provider returns the claim as "editor staff", the membership check compares "editor staff" against the allowed list and fails even though the user genuinely has the "staff" group, silently locking out users who should be allowed in.I split on whitespace in both places instead of wrapping the string as-is. This only changes behavior for the string branch; a claim that already arrives as a list is untouched. Single-word strings keep working exactly as before, since
"staff".split()is just["staff"].Tried reproducing this with a fresh user and
plugin._create_update_groupsdirectly rather than mocking out the OIDC flow, which surfaced an unrelated quirk worth noting: theuserobject passed into that method has a cachedgetGroups(), so even after adding group memberships, callinggetGroups()on the same object still returns stale data. The tests fetch the user again throughapi.user.get()to see the real membership instead of trusting the method's own return value.Ran the full test suite locally and it's green; the new tests fail on the old code and pass with this change.