-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Allow multiple ServerLogoutHandler instances in ServerHttpSecurity #17381
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
base: main
Are you sure you want to change the base?
Allow multiple ServerLogoutHandler instances in ServerHttpSecurity #17381
Conversation
…gout handlers like with Servlet. Signed-off-by: Blake Bauman <[email protected]>
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, @blake-bauman, for the PR! In addition to my inline feedback, will you please add some tests to confirm that the new functionality works?
* @param logoutHandler | ||
* @return the {@link LogoutSpec} to configure | ||
*/ | ||
public LogoutSpec addLogoutHandler(ServerLogoutHandler logoutHandler) { |
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.
Instead of exposing addLogoutHandler
, would logout(Consumer<List<ServerLogoutHandler>> consumer)
also service your needs? The reason this is nice it because it also allows you to remove values. Please see OneTimeTokenLogoutSpec#authenticationSuccessHandler
for an example.
/** | ||
* Adds a logout handler in the last position. | ||
* @param logoutHandler | ||
* @return the {@link LogoutSpec} to configure |
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.
Will you please add @since 7.0
@blake-bauman, please also make sure to sign your commit and keep the commit title to about 50 characters, for example:
|
@jzheaux I didn't see any existing tests for the Logout handler. Could you point to where they might be so that I can add on? |
@jzheaux The reason I did it this way is because |
@blake-bauman, thanks for the updates. Here is the error message from the DCO bot:
It looks like it's complaining about "blake_bauman" being different from "Blake Bauman". |
I can see that, it makes sense. My guess is that the addLogoutHandler method is there bc it is an artifact from copy-pasting from LogoutHandlerConfigurer when LogoutSpec was first being written. That was before my time, though, so I'm not sure. Either way, yes, the consumer is prefererred. This allows complete control over teh default list without also needing to add something like |
I don't think there are any yet. You can add them in |
Spring Security for Spring MVC allows for specifying multiple LogoutHandler implementations which get wrapped in a DelegatingLogoutHandler. Spring Security for WebFlux currently only allows a single ServerLogoutHandler implementation.
This PR puts both Spring MVC and Spring WebFlux on equal functionality when it comes to logout handlers.