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

Added options to allow IDP to dynamically change values of Name ID and attributes when creating a SAML response. #88

Merged
merged 3 commits into from
Jul 26, 2021

Conversation

hamaron
Copy link
Contributor

@hamaron hamaron commented Jan 12, 2018

PR for an issue #87

values of Name ID and attributes when creating a SAML response.
@hamaron hamaron changed the title Added options to allow IPD to dynamically change values of Name ID and attributes when creating a SAML response. Added options to allow IDP to dynamically change values of Name ID and attributes when creating a SAML response. Jan 12, 2018
@hamaron
Copy link
Contributor Author

hamaron commented Mar 14, 2018

@mvastola Hi, MIke! Do you have any thoughts on this?

@hamaron
Copy link
Contributor Author

hamaron commented Jan 7, 2019

@jphenow Hi! Is this something you could take a look?
I'm preparing another pull request which enables multi IDP configurations, created from this branch.
That feature was actually requested in some places such as #54, so I'd be happy to create a PR for the feature after this pull request
get merged.

@Zogoo
Copy link
Collaborator

Zogoo commented Jan 9, 2019

I think good idea. SP initiated SAML request always have what NameID format required for response.

Copy link
Collaborator

@Zogoo Zogoo 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 for me. Actually i'm was also thinking about do same thing, but i found this pull request.

Copy link
Collaborator

@jphenow jphenow left a comment

Choose a reason for hiding this comment

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

Yea I think this makes sense. Can merge master in and add a note to the README? I believe we have a place where you could describe this option from the controller perspective at least.

@hamaron
Copy link
Contributor Author

hamaron commented Jan 9, 2019

@jphenow Sure thing. I'll add the description in README.

@Zogoo
Copy link
Collaborator

Zogoo commented Jan 20, 2020

Any update?

@shaneshort
Copy link

@jphenow is this still being blocked? I can see this being super useful for me down the line :)

@mjobin-mdsol
Copy link
Collaborator

@Zogoo did you include this PR in your fork ?

@Zogoo
Copy link
Collaborator

Zogoo commented Aug 30, 2020

@mjobin-mdsol I haven't merged yet, because this pull request removing that name id from SamlConfig. So, I was just using old way.

@Zogoo
Copy link
Collaborator

Zogoo commented Jan 12, 2021

Yea I think this makes sense. Can merge master in and add a note to the README? I believe we have a place where you could describe this option from the controller perspective at least.

@jphenow, I have added a new wiki page for configurations and how to override with options in the "controller".
https://github.com/saml-idp/saml_idp/wiki/IdP_Configuration
And I realized that we already have lots of options never mentioned in the README. So, I would suggest to merge this pull request for now and later we can contribute our documents including name id overriding.
What do you think about it?

@jphenow
Copy link
Collaborator

jphenow commented Jan 12, 2021

Yea that makes sense - the comment was more aspirational about not getting further off on the README, but I don't think this really needs to block on those README changes

@hamaron
Copy link
Contributor Author

hamaron commented Jan 13, 2021

Sorry, guys. It's been already 2 years since I got the homework to write the instruction in the Wiki to get this merged.
It would be nice if I could have a separate ticket to write the doc as you guys suggested.

I can resolve the conflicts of the files in a timely manner.

@Zogoo
Copy link
Collaborator

Zogoo commented Jan 18, 2021

@jphenow, @hamaron I have resolved the conflict for this pull request. Because the conflict came from my branch and it was easier to resolve it by my side. But, could you guys also double check that I didn't do any miss.

And let's merge this pull request as soon as possible.

lib/saml_idp/saml_response.rb Outdated Show resolved Hide resolved
@LukeGuanY
Copy link

LukeGuanY commented Jun 2, 2021

Do we have an update on this PR? We would like to set the name id dynamically. It would be very helpful.

@Zogoo Zogoo requested a review from jphenow July 7, 2021 05:45
@jphenow jphenow merged commit 4198dd2 into saml-idp:master Jul 26, 2021
@jphenow
Copy link
Collaborator

jphenow commented Jul 26, 2021

v0.14.0 released with this addition.

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