-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
SAML 2.0 Single Logout Support #9483
Conversation
a88c1b1
to
ddf8e7a
Compare
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.
Josh, I decided to pause the review,, to grasp more around the configuration choice.
docs/manual/src/docs/asciidoc/_includes/servlet/saml2/saml2-login.adoc
Outdated
Show resolved
Hide resolved
docs/manual/src/docs/asciidoc/_includes/servlet/saml2/saml2-login.adoc
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.
Josh, I've given you some to work with.
The number 1 take away I have is that Spring Security shouldn't release 2nd class citizen implementations. That being, I have to jump through hoops like configuring logout handlers, filters, etc to just get something as basic as logout to work. This seems to contradict the reason why we have Spring Security, like logout()
in the DSL
I am happy to review the next, I need to get my environment setup for that
docs/manual/src/docs/asciidoc/_includes/servlet/saml2/saml2-login.adoc
Outdated
Show resolved
Hide resolved
docs/manual/src/docs/asciidoc/_includes/servlet/saml2/saml2-login.adoc
Outdated
Show resolved
Hide resolved
docs/manual/src/docs/asciidoc/_includes/servlet/saml2/saml2-login.adoc
Outdated
Show resolved
Hide resolved
docs/manual/src/docs/asciidoc/_includes/servlet/saml2/saml2-login.adoc
Outdated
Show resolved
Hide resolved
...g/springframework/security/saml2/provider/service/registration/RelyingPartyRegistration.java
Outdated
Show resolved
Hide resolved
...g/springframework/security/saml2/provider/service/registration/RelyingPartyRegistration.java
Outdated
Show resolved
Hide resolved
...g/springframework/security/saml2/provider/service/registration/RelyingPartyRegistration.java
Outdated
Show resolved
Hide resolved
...ngframework/security/saml2/provider/service/web/DefaultRelyingPartyRegistrationResolver.java
Show resolved
Hide resolved
...ngframework/security/saml2/provider/service/web/DefaultRelyingPartyRegistrationResolver.java
Show resolved
Hide resolved
2ee9b25
to
f16548f
Compare
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.
Big ticket items
- Metadata shows Single Logout Services even when single logout isn't enabled
- is there one
/logout
that is handles logout for any type of authentication or is there/<auth-type>/logout
and thus, if you support oidc,saml,oauth2,internal you have four different logout endpoints
Overall, this looks very promising and I think the above comments can be saved for future iterations as the community gets a chance to test drive it.
...main/java/org/springframework/security/config/annotation/web/configurers/CsrfConfigurer.java
Outdated
Show resolved
Hide resolved
.../springframework/security/config/annotation/web/configurers/saml2/Saml2LogoutConfigurer.java
Outdated
Show resolved
Hide resolved
.../springframework/security/config/annotation/web/configurers/saml2/Saml2LogoutConfigurer.java
Outdated
Show resolved
Hide resolved
.../springframework/security/config/annotation/web/configurers/saml2/Saml2LogoutConfigurer.java
Outdated
Show resolved
Hide resolved
docs/manual/src/docs/asciidoc/_includes/servlet/saml2/saml2-login.adoc
Outdated
Show resolved
Hide resolved
...a/org/springframework/security/saml2/provider/service/metadata/OpenSamlMetadataResolver.java
Outdated
Show resolved
Hide resolved
docs/manual/src/docs/asciidoc/_includes/servlet/saml2/saml2-login.adoc
Outdated
Show resolved
Hide resolved
...ity/saml2/provider/service/web/authentication/logout/HttpSessionLogoutRequestRepository.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.
I've provided some feedback inline
.../security/saml2/provider/service/web/authentication/logout/OpenSamlLogoutRequestHandler.java
Outdated
Show resolved
Hide resolved
...security/saml2/provider/service/web/authentication/logout/OpenSamlLogoutResponseHandler.java
Outdated
Show resolved
Hide resolved
...rg/springframework/security/saml2/provider/service/web/authentication/logout/Saml2Utils.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 for the hard work on this.
I've reopened the PR to take a closer look at this comment. |
ebf88c5
to
2c6c988
Compare
...pringframework/security/saml2/provider/service/authentication/logout/Saml2LogoutRequest.java
Outdated
Show resolved
Hide resolved
...security/saml2/provider/service/web/authentication/logout/OpenSamlLogoutRequestResolver.java
Outdated
Show resolved
Hide resolved
4d82f8c
to
6038bdb
Compare
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've provided feedback inline.
public interface RelyingPartyRegistrationResolver extends Converter<HttpServletRequest, RelyingPartyRegistration> { | ||
|
||
@Override | ||
default RelyingPartyRegistration convert(HttpServletRequest request) { |
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'd prefer to leave this method off of the new interface. Instead, I think it would be worth seeing if the following arrangement would work:
RelyingPartyRegistrationResolver
does not extendConverter
DefaultRelyingPartyRegistrationResolver
can implement bothConverer
andRelyingPartyRegistrationResolver
- Classes that use to use
Converter<HttpServletRequest, RelyingPartyRegistration>
can now also allowRelyingPartyRegistrationResolver
to be injected
In my mind this has the following advantages:
RelyingPartyRegistrationResolver
is kept cleaner because it does not need theconvert
method- Classes that use to use
Converter
no longer need useinstanceof
checks (i.e. SamlMetadataFilter) because there can be distinct constructors/methods for the types. - We can properly deprecate the old
Converter
methods/constructors - We can properly deprecate the old
DefaultRelyingPartyRegistrationResolver.convert
method which eventually allows us to remove it'sRequestMatcher
that may be out of sync with the way theFilter
is resolving theregistrationId
.
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 believe this raises a passivity concern. For example, a current construction of Saml2MetadataFilter
could look like:
new Saml2MetadataFilter(
new DefaultRelyingPartyRegistrationResolver(repository), new OpenSamlMetadataResolver())
If RelyingPartyRegistrationResolver
does not extend Converter
, then Saml2MetadataFilter
will need another constructor, one that takes RelyingPartyRegistrationResolver
. Since DefaultRelyingPartyRegistrationResolver
extends both, the constructor call is ambiguous, causing a compilation error.
One way to address this would be to add a static factory method Saml2MetadataFilter#construct(RelyingPartyRegistrationResolver, Saml2MetadataResolver)
. This method would be necessary in other classes like Saml2AuthenticationTokenConverter
.
Or, if we add the new constructors and deprecate the old ones, those who upgrade can change to the other constructor by adding a cast like so:
new Saml2MetadataFilter((RelyingPartyRegistrationResolver)
new DefaultRelyingPartyRegistrationResolver(repository), new OpenSamlMetadataResolver())
or by splitting up the line (more readable anyway):
RelyingPartyRegistrationResolver relyingPartyRegistrationResolver =
new DefaultRelyingPartyRegistrationResolver(repository);
new Saml2MetadataFilter(relyingPartyRegistrationResolver, new OpenSamlMetadataResolver())
The initial construction I listed may be uncommon and is certainly less readable -- what are your thoughts?
...security/saml2/provider/service/web/authentication/logout/OpenSamlLogoutRequestResolver.java
Outdated
Show resolved
Hide resolved
...security/saml2/provider/service/web/authentication/logout/OpenSamlLogoutRequestResolver.java
Outdated
Show resolved
Hide resolved
...urity/saml2/provider/service/web/authentication/logout/Saml2LogoutRequestSuccessHandler.java
Outdated
Show resolved
Hide resolved
} | ||
this.logoutHandler.logout(request, response, authentication); | ||
Saml2LogoutResponse logoutResponse = this.logoutResponseResolver.resolveLogoutResponse(request, authentication) | ||
.logoutResponse(); |
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 is worth separating the controller and the view so users can customize the UI without needing to do the more complex controller logic. We can allow this Filter to optionally forward to a URL that obtains the Saml2LogoutResponse
from a HttpServletRequest
attribute so it can render it's own UI. This could be logged as a separate ticket. The code below would become a default ui if no forward URL was provided.
.../org/springframework/security/saml2/provider/service/authentication/Saml2Authentication.java
Outdated
Show resolved
Hide resolved
...rk/security/saml2/provider/service/authentication/logout/Saml2LogoutAuthenticatorResult.java
Outdated
Show resolved
Hide resolved
...k/security/saml2/provider/service/authentication/logout/Saml2LogoutRequestAuthenticator.java
Outdated
Show resolved
Hide resolved
.../security/saml2/provider/service/authentication/logout/Saml2LogoutResponseAuthenticator.java
Outdated
Show resolved
Hide resolved
...k/security/saml2/provider/service/web/authentication/logout/Saml2LogoutResponseResolver.java
Outdated
Show resolved
Hide resolved
25d7d4b
to
a96844b
Compare
c29d465
to
30bb291
Compare
30bb291
to
4b29450
Compare
No description provided.