Skip to content
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

Removed logout confirmation page #106

Merged
merged 1 commit into from
Mar 1, 2024
Merged

Conversation

emmdurin
Copy link
Contributor

Spring by default shows a logout confirmation page and we do not want it.

@emmdurin emmdurin requested review from f-necas and pmauduit February 21, 2024 09:50
@@ -99,7 +95,8 @@ SecurityWebFilterChain securityWebFilterChain(ServerHttpSecurity http,
RedirectServerLogoutSuccessHandler defaultRedirect = new RedirectServerLogoutSuccessHandler();
defaultRedirect.setLogoutSuccessUrl(URI.create(georchestraLogoutUrl));

LogoutSpec logoutUrl = http.formLogin().loginPage("/login").and().logout().logoutUrl("/logout")
LogoutSpec logoutUrl = http.formLogin().loginPage("/login").and().logout()
.requiresLogout(ServerWebExchangeMatchers.pathMatchers(HttpMethod.GET, "/logout"))
Copy link
Collaborator

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 the possibility to choose whether or not we want the confirm page instead of hardcode it.

The confirm page should be the default behavior and direct logout should be a configuration in gateway.yaml. WDYT ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed for a configurable behavior, if it makes sense to you.
Default behavior should be as for the security proxy IMO : immediate logout.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if the default workflow from spring (GET /logout then POST /logout) is made on purpose e.g. for security's sake. I'm not an expert, what would be the risk / impact to directly logout the user after the GET request ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

@f-necas f-necas Feb 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed we will keep only one strategy and won't keep logout page at all.

We will stay with a GET first to keep header's compatibility with gateway & security-proxy.

So LGTM then

@emmdurin emmdurin force-pushed the remove_logout_confirmation branch from 7538e23 to c248595 Compare March 1, 2024 09:00
@emmdurin emmdurin merged commit 419e212 into main Mar 1, 2024
3 checks passed
@f-necas f-necas deleted the remove_logout_confirmation branch June 26, 2024 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants