Allow multiple ServerLogoutHandler instances in ServerHttpSecurity

[blake-bauman]

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.

[jzheaux]

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?

[jzheaux]

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.

[jzheaux]

@blake-bauman, please also make sure to sign your commit and keep the commit title to about 50 characters, for example:

Support Multiple ServerLogoutHandlers

This commit adds support to ServerHttpSecurity for registering
multiple ServerLogoutHandlers. This is handy so that an application
does not need to re-supply any handlers already configured by
the DSL

[blake-bauman]

I'm a little confused because I did sign it using the -s

[blake-bauman]

@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?

[blake-bauman]

Instead of exposing addLogoutHandler, would logout(Consumer<List> consumer) also service your needs?

@jzheaux The reason I did it this way is because LogoutConfigurer for Servlet has addLogoutHandler() as public, and was attempting to match the spec for consistency. However, if you'd prefer that I switch to using Consumer<List<...>>, I can do that instead.

[jzheaux]

@blake-bauman, thanks for the updates. Here is the error message from the DCO bot:

Commit sha: [8fea725](https://github.com/spring-projects/spring-security/pull/17381/commits/8fea725b8c527233224b71bb5d01e3329f1feb41), Author: blake_bauman, Committer: blake_bauman; Expected "blake_bauman [[email protected]](mailto:[email protected])", but got "Blake Bauman [[email protected]](mailto:[email protected])".

It looks like it's complaining about "blake_bauman" being different from "Blake Bauman".

[jzheaux]

The reason I did it this way is because LogoutConfigurer for Servlet has addLogoutHandler() as public, and was attempting to match the spec for consistency.

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 ObjectPostProcessor into the DSL.

[jzheaux]

@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?

I don't think there are any yet. You can add them in LogoutSpecTests.

[blake-bauman]

@jzheaux Sorry for the delay. Been on vacation. Made the changes you requested.

[jzheaux]

Thanks, @blake-bauman! I've left some additional feedback inline.

Also, I think we are close enough to squash your commits. Will you please ensure that there are two commits:

1. for this feature
2. for the cleanup regarding the default logout handler null check (please see my inline comment)

Also, please double-check your DCO signature. The check is failing with the following error:

Commit sha: 8fea725, Author: blake_bauman, Committer: blake_bauman; Expected "blake_bauman [email protected]", but got "Blake Bauman [email protected]".

[blake-bauman]

Also, please double-check your DCO signature. The check is failing with the following error:

Not sure what's going on with the DCO. The latest commit has Signed-off-by: blake_bauman <[email protected]>

Also, I think we are close enough to squash your commits. Will you please ensure that there are two commits

Not quite sure how to do that since there's a rebase in the middle and I haven't been able to figure out how to squash without also squashing that one. (My git skills are admittedly weak.) Given the DCO issue and reverting the null change, would it make more sense if I simply closed this and created a new PR with just the one change? Or is squashing that rebase ok?

[jzheaux]

No problem, @blake-bauman. DCO is a new thing for a lot of people and can be confusing.

The issue was that there were three commits, but only one of them was signed in a way that matched the commit author. The merge commit was unsigned and the initial commit used "Blake Bauman" instead of "blake_bauman".

To fix this, I rebased with main to remove the merge commit and then I squashed your two commits together, preserving the signature of the latest commit.

[blake-bauman]

Ok, I think I got it now and it's ready. I reverted the change to the returning null and it should be only 1 commit now, with a proper signoff.

[jzheaux]

Thanks again, @blake-bauman! This will merge into main once the build completes.

[blake-bauman]

NP. Thanks for merging this!