-
Notifications
You must be signed in to change notification settings - Fork 156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support single and multi value attributes #159
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making this change! This looks great, but I have two requests:
- like you said, backwards compatibility will mean I can merge this—we have no immediate plans for a breaking version
- unit tests for the new functionality
Thanks again!
def value_by_saml_attribute_key(key) | ||
@attributes[String(key)] | ||
def value_by_saml_attribute_key(key, config) | ||
@attributes.send(config["attribute_type"], String(key)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer something a little more explicit:
@attributes.send(config["attribute_type"], String(key)) | |
case config["attribute_type"] | |
when "single" | |
@attributes.single(String(key)) | |
when "multi" | |
@attributes.multi(String(key)) | |
end |
I fixed the tests on master, so you should also merge/rebase master again. |
Hi Guys, |
Bumping to see if there's any plans to include this as well. Having multiple attribute values for roles would be a big plus |
I think this would be quite a valuable addition, but perhaps the way it is done should be changed a bit to be compatible with current setups without having to change the existing yml files for single value attributes. May look into making a PR for that if I find a good way to do it. |
I would benefit greatly from this as a v2.0.0 and breaking change. |
#245 fixes tests, add coverage. |
I think it is essential to handle both single and multi value attributes as Identity Providers often send
responses containing both types, e.g. single for email and multi for roles.
I have provided an example of the new attribute map yaml. The type is configureable per attribute.
The change is not backwards compatible but it would be relatively easy to add if it is a
blocking issue.