Skip to content

Support single and multi value attributes#159

Open
SxDx wants to merge 2 commits intoapokalipto:masterfrom
SxDx:master
Open

Support single and multi value attributes#159
SxDx wants to merge 2 commits intoapokalipto:masterfrom
SxDx:master

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