Enable custom structure for group claim with default name group #839
Conversation
Update Fork
Merge Changes from Base Repo
|
@NickMeves You are right, there is an overlap. But the decisive difference is that in my PR, even the default group claim can have a completely different structure. |
If you don't mind, can you support getting the proper fixes in the other PR? We try to lean toward letting the first submitter of a PR the precedence to encourage the community to contribute and get the experience of getting a PR merged on an open source project. If it starts to go stale, we can pick up the work here. We won't wait the normal 60 days because we want this feature in v7 which we want to release in 4-6 weeks. |
There was a problem hiding this comment.
I see enough differences in the bug you are trying to solve vs the other PR to merge this separately in case you don't see movement on the other PR in a timely manner.
I left a couple of questions that aren't blocking, looking for opinions.
Please go ahead and add a CHANGELOG entry reflecting the new support for complex groups in OIDC IDTokens.
Update Repo from origin
…ture-for-group-claim-with-default-name-group
You are welcome! 😊 |
| func formatGroup(rawGroup interface{}) (string, error) { | ||
| group, ok := rawGroup.(string) | ||
| if !ok { | ||
| jsonGroup, err := json.Marshal(rawGroup) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| group = string(jsonGroup) | ||
| } | ||
| return group, nil |
There was a problem hiding this comment.
This function is a tad terse, it's not obvious what this is trying to achieve, maybe could have done with a comment on this 🤔
Is the point that groups is always flattened to some JSON string?
There was a problem hiding this comment.
Yeah - only if the IdP had a structure that wasn't []string . In the contributers case, this was what the claim looked like from there IdP raised in the linked issue:
"groups": [
{
"groupId": "CIDAAS_ADMINS",
"roles": [
"ADMIN"
]
}
],
|
When its planned to be merged? In which version? |
|
This is merged and in releases from 7.0.0 onwards |
…h2-proxy#839) * Allow complex structure for groups in group claim. * Remove unused constant * Update variable name * Fix linting * Use helper method * Log error if not possible to append group value * Add missing import * Use own logger * Fix imports * Remove Dockerfile for testing * Add Changelog entry * Use formatGroup helper method and update tests * Return string instead of string array * Remove groups variable * Return error in format method. * Reorder imports Co-authored-by: Nick Meves <nick.meves@greenhouse.io>
Description
I ignore the default group claim for unmarshalling json, because we can not be sure that it has the type
[]string.Instead I extract only the group claim defined via config. If the group value is not a string I use the json representation of the group value. With this change the application can handle the group values individually.
Maybe you have a better idea for handling the group value, but at least for me it would work.
Motivation and Context
Couldn't extract claims from id_token because group claim has invalid structure
How Has This Been Tested?
I created an additional test case.
Checklist: