-
Notifications
You must be signed in to change notification settings - Fork 39
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
Resolves #349: Add Browser IDP & upgrade to saml2aws v2.32.0 #350
Conversation
@@ -1,4 +1,4 @@ | |||
[submodule "third_party/saml2aws"] | |||
path = third_party/saml2aws | |||
url = https://github.com/richardcase/saml2aws.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently we are using a forked version of saml2aws to support Ping with NTLM:
richardcase/saml2aws@e3668c0
We will need to either get this merged in with upstream repo or update the forked version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. Is there a PR against saml2aws to get your NTLM patch in the upstream repo? Is there a reason you are maintaining the fork?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@richardcase can you help with this query please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason for us to maintain the fork other than not having made the PR upstream. I will try and get the rebase/PR done Monday so that we can ultimately get rid of the fork.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @richardcase, any luck with submitting that PR to upstream?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, i completely forgot about this. You may want to fork into fidelity and then create the PR from there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no worries, ok will do. thanks for the update. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully this will be merged some day: Versent/saml2aws#794
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤞
Closing this PR since it is out dated, and we now have a fix already merged: #650 |
What this PR does / why we need it:
Allows for the use of Browser based SAML IDP that is available in 2.32.0 of saml2aws
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #349