Skip to content

Support single and multi value attributes#159

Open
SxDx wants to merge 2 commits into
apokalipto:masterfrom
SxDx:master
Open

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

Conversation

@SxDx

@SxDx SxDx commented Feb 11, 2020

Copy link
Copy Markdown

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.

@adamstegman adamstegman left a comment

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown
Collaborator

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

@victorlcampos

Copy link
Copy Markdown

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

@khuong-acorns

Copy link
Copy Markdown

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

Taeir commented Aug 13, 2023

Copy link
Copy Markdown

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
Copy Markdown
Contributor

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

@doconnor-clintel

Copy link
Copy Markdown
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