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

Add support for multivalued attributes #59

Merged
merged 3 commits into from
Nov 12, 2020
Merged

Add support for multivalued attributes #59

merged 3 commits into from
Nov 12, 2020

Conversation

jking916
Copy link
Contributor

@jking916 jking916 commented Nov 11, 2020

Currently we use CAS to return multi-valued attributes like entitlements/roles, which aren't natively supported by this omniauth provider and instead get set to the last value returned. Looking to add optional support for returning these multi-valued attributes as arrays while maintaining backwards compatibility by default.

Copy link
Collaborator

@vjt vjt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, however potentially breaking. Should be properly tested

Array(hash[node_name]).push(e.content)
else
e.content
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be better stubbed out in a separate method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vjt Thanks for the feedback, see the updated changes. And glad to see this project is still active. 🎉

Copy link
Collaborator

@vjt vjt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thank you!

@vjt
Copy link
Collaborator

vjt commented Nov 12, 2020

@tagliala could you help in updating omniauth-cas in one of our projects to this branch to ensure nothing is broken?

Thank you

@vjt
Copy link
Collaborator

vjt commented Nov 12, 2020

This worked well in our environment (thanks @tagliala!) and it is backwards compatible, thus merging.

Thanks!

@vjt vjt merged commit c485392 into dlindahl:master Nov 12, 2020
@jking916 jking916 deleted the multivalued_attributes branch November 12, 2020 16:14
@jking916
Copy link
Contributor Author

Thanks to you both! I noticed that there hasn't been a new release in a while (also brought up in #53). Is this waiting on an owner or a change in ownership?

@vjt
Copy link
Collaborator

vjt commented Nov 12, 2020

Derek @dlindahl has already granted gem ownership to me and other contributors, so we can definitely cut a new release once this is merged

@jgribonvald
Copy link

Please could you publish a new release quickly ?

Thanks a lot for this update !

@vjt
Copy link
Collaborator

vjt commented Nov 14, 2020 via email

@vjt
Copy link
Collaborator

vjt commented Nov 14, 2020

@jgribonvald 2.0.0 has been pushed to Ruby Gems.

Enjoy and please revert back if something is wrong.

Cheers!

@jgribonvald
Copy link

@vjt thanks a lot, I will let you know if something goes wrong.

tduehr pushed a commit to tduehr/omniauth-cas3 that referenced this pull request Aug 23, 2022
redconfetti pushed a commit to sis-berkeley-edu/omniauth-cas that referenced this pull request Sep 19, 2023
redconfetti pushed a commit to sis-berkeley-edu/omniauth-cas that referenced this pull request Nov 18, 2023
redconfetti pushed a commit to sis-berkeley-edu/omniauth-cas that referenced this pull request Nov 18, 2023
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.

3 participants