-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Add DefaultRelyingPartyRegistrationResolver #8899
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.
@jzheaux I've left a few minor comments inline.
...ngframework/security/saml2/provider/service/web/DefaultRelyingPartyRegistrationResolver.java
Outdated
Show resolved
Hide resolved
...ngframework/security/saml2/provider/service/web/DefaultRelyingPartyRegistrationResolver.java
Outdated
Show resolved
Hide resolved
...ingframework/security/config/annotation/web/configurers/saml2/Saml2LoginConfigurerTests.java
Outdated
Show resolved
Hide resolved
...mework/security/saml2/provider/service/web/DefaultRelyingPartyRegistrationResolverTests.java
Outdated
Show resolved
Hide resolved
private Saml2AuthenticationRequestContextResolver getContextResolver(B http) { | ||
Saml2AuthenticationRequestContextResolver resolver = getBeanOrNull(http, Saml2AuthenticationRequestContextResolver.class); | ||
if (resolver == null) { | ||
return new DefaultSaml2AuthenticationRequestContextResolver( |
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.
I think it would be better to have a single return
statement.
new DefaultSaml2AuthenticationRequestContextResolver
can be moved into getDefaultContextResolver
method.
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.
I'm of a different opinion on this one. I think returning early is more readable.
However, since that's probably in the eye of the beholder, what I'd propose for now is aligning with how other similar methods in the class, like getResolver
. We can consider changing both methods at another time.
Thanks for the feedback, @evgeniycheban, it was very helpful. I incorporated it and committed the changes in 015281f to |
No description provided.