Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

SxDx
Copy link

@SxDx SxDx commented Feb 11, 2020

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.

Copy link
Collaborator

@adamstegman adamstegman left a 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))
Copy link
Collaborator

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:

Suggested change
@attributes.send(config["attribute_type"], String(key))
case config["attribute_type"]
when "single"
@attributes.single(String(key))
when "multi"
@attributes.multi(String(key))
end

@adamstegman
Copy link
Collaborator

I fixed the tests on master, so you should also merge/rebase master again.

@victorlcampos
Copy link

Hi Guys,
any update about this PR? It will help a lot here =)

@khuong-acorns
Copy link

Bumping to see if there's any plans to include this as well. Having multiple attribute values for roles would be a big plus

@Taeir
Copy link

Taeir commented Aug 13, 2023

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.

@doconnor-clintel
Copy link
Contributor

I would benefit greatly from this as a v2.0.0 and breaking change.

@doconnor-clintel
Copy link
Contributor

#245 fixes tests, add coverage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants