-
Notifications
You must be signed in to change notification settings - Fork 6k
enhancement to support overriding SAMLRequest #9209
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
Conversation
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.
Thanks, @Adi-devops! I've left some feedback inline.
...mework/security/saml2/provider/service/authentication/Saml2AuthenticationRequestContext.java
Outdated
Show resolved
Hide resolved
...mework/security/saml2/provider/service/authentication/Saml2AuthenticationRequestContext.java
Outdated
Show resolved
Hide resolved
...mework/security/saml2/provider/service/authentication/Saml2AuthenticationRequestContext.java
Outdated
Show resolved
Hide resolved
...mework/security/saml2/provider/service/authentication/Saml2AuthenticationRequestContext.java
Outdated
Show resolved
Hide resolved
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.
Thanks, @Adi-devops, for the updates! I've left a bit more feedback inline.
Also, in preparation for merging, will you please squash your commits and in the resulting commit ensure that you have the phrase Closes gh-9199
at the end?
...mework/security/saml2/provider/service/authentication/Saml2AuthenticationRequestContext.java
Outdated
Show resolved
Hide resolved
...mework/security/saml2/provider/service/authentication/Saml2AuthenticationRequestContext.java
Outdated
Show resolved
Hide resolved
...mework/security/saml2/provider/service/authentication/Saml2AuthenticationRequestContext.java
Outdated
Show resolved
Hide resolved
...mework/security/saml2/provider/service/authentication/Saml2AuthenticationRequestContext.java
Show resolved
Hide resolved
...ork/security/saml2/provider/service/authentication/OpenSamlAuthenticationRequestFactory.java
Outdated
Show resolved
Hide resolved
@jzheaux Can you please review the PR. |
Blocked by #9277. Let's revisit once that PR is closed. |
Sure that would be great |
Now that #9277 is merged, an application can do the following to set each of these properties: First, @Bean
Saml2AuthenticationRequestResolver authenticationRequests(RelyingPartyRegistrationResolver registrations) {
Saml2AuthenticationRequestResolver resolver = new OpenSaml4AuthenticationRequestResolver(registrations);
resolver.setAuthnRequestCustomizer((params) -> params.getAuthnRequest().setIsPassive(Boolean.TRUE));
return resolver;
} Second, @Bean
Saml2AuthenticationRequestResolver authenticationRequests(RelyingPartyRegistrationResolver registrations) {
Saml2AuthenticationRequestResolver resolver = new OpenSaml4AuthenticationRequestResolver(registrations);
resolver.setAuthnRequestCustomizer((params) -> params.getAuthnRequest().setForceAuthn(Boolean.TRUE));
return resolver;
} And third, @Bean
Saml2AuthenticationRequestResolver authenticationRequests(RelyingPartyRegistrationResolver registrations) {
AuthnContextClassRef ref = // .. construct with OpenSAML
Saml2AuthenticationRequestResolver resolver = new OpenSaml4AuthenticationRequestResolver(registrations);
resolver.setAuthnRequestCustomizer((params) -> params.getAuthnRequest()
.getRequestedAuthnContext().getAuthnContextClassRefs().add(ref));
return resolver;
} Because of this, I think that we don't need to introduce additional fields into Thank you so much for your efforts. I'm going to close this; please feel free to reopen if you feel this doesn't address the issue. |
This PR is to enhance the SAMLRequest by providing support to override a few of the parameters mentioned in the issue GH-9199.
Support Added