Allow customizing LogoutHandler in OidcLogoutEndpointFilter

[finke-ba]

Expected Behavior
Option to override the default LogoutHandler(setter) in OidcLogoutEndpointFilter by OidcLogoutEndpointConfigurer.

Current Behavior
In the current version the LogoutHandler is hardcoded in the OidcLogoutEndpointFilter constructior.
It would be great to have it consistent with LogoutFilter, Saml2LogoutRequestFilter, LogoutWebFilter.
Also please consider usingCompositeLogoutHandler as in LogoutFilter and Saml2LogoutRequestFilter.

Context
I'm trying to add a couple custom actions for OIDC logout, but at the moment I have to override whole default AuthenticationSuccessHandler(performLogout function) in OidcLogoutEndpointFilter and copy quite a lot code from performLogout private function.

At the same time the implementation of performLogout function looks a little controversial in case of adding option for logoutHandler override:

// Check for active user session
if (oidcLogoutAuthentication.isPrincipalAuthenticated() &&
   	StringUtils.hasText(oidcLogoutAuthentication.getSessionId())) {
   // Perform logout
   this.logoutHandler.logout(request, response,
   		(Authentication) oidcLogoutAuthentication.getPrincipal());
}

logoutHandler.logout is called by condition which could cause problems in case of some custom logoutHandler or CompositeLogoutHandler.

Also I have a question about this part of performLogout function:

if (oidcLogoutAuthentication.isAuthenticated() &&
   	StringUtils.hasText(oidcLogoutAuthentication.getPostLogoutRedirectUri())) {
   // Perform post-logout redirect
   UriComponentsBuilder uriBuilder = UriComponentsBuilder
   		.fromUriString(oidcLogoutAuthentication.getPostLogoutRedirectUri());
   String redirectUri;
   if (StringUtils.hasText(oidcLogoutAuthentication.getState())) {
   	uriBuilder.queryParam(
   			OAuth2ParameterNames.STATE,
   			UriUtils.encode(oidcLogoutAuthentication.getState(), StandardCharsets.UTF_8));
   }
   redirectUri = uriBuilder.build(true).toUriString();		// build(true) -> Components are explicitly encoded
   this.redirectStrategy.sendRedirect(request, response, redirectUri);
} else {
   // Perform default redirect
   this.logoutSuccessHandler.onLogoutSuccess(request, response,
   		(Authentication) oidcLogoutAuthentication.getPrincipal());
}

In this code, I am confused by the fact that the logoutSuccessHandler will not always be called, but only by a condition.
It seems to me that the point of this logic is to redirect the user by postLogoutRedirectUri or default uri(which is "/").
So, could you please explain why it's not possible to use redirectStrategy or SimpleUrlLogoutSuccessHandler in both cases?

[jgrandja]

@finke-ba

Option to override the default LogoutHandler

The internal LogoutHandler is not intended to be exposed. The sole purpose is to reuse the logic in SecurityContextLogoutHandler to clear the SecurityContext and invalidate the session.

logoutHandler.logout is called by condition which could cause problems in case of some custom logoutHandler or CompositeLogoutHandler.

Can you provide details on the problem you forsee?

[finke-ba]

Hi @jgrandja and thank you for such a quick reply!
The problem I'm trying to solve is the following - before OidcLogoutEndpointFilter was implemented I used LogoutFilter with session cookie deletion(deleteCookies from LogoutConfigurer) and current OAuth2Authorization deletion by custom logout handler(addLogoutHandler from LogoutConfigurer).
Trying to achieve the same level of customisation of the logout process with OidcLogoutEndpointFilter I faced a problem that I can't expand existing functionality of logout process and if I want to add some additional steps to logout process I can only override AuthenticationSuccessHandler and I have to copy code from performLogout to my custom AuthenticationSuccessHandler to not lose functionality from the OIDC RP-Initiated Logout implementation(as far as I understand, the postLogoutRedirectUri and redirect itself from performLogout relates to it).
At the same time, an extension of the logout functionality exists in other similar filters(LogoutFilter, Saml2LogoutRequestFilter, LogoutWebFilter) by providing custom LogoutHandler(with option to provide CompositeLogoutHandler), which I find very handy to use.
This is why I would like to be able to customise the logout process as easily as with the other filters.

I hope my explanation wasn't too confusing, but to summarise I can say that the main problem is having to copy code from a private performLogout function into my custom AuthenticationSuccessHandler - which I'd like to avoid.

[jgrandja]

@finke-ba Thanks for the explanation. I'll review the customization capabilities of the other Filter's you mentioned and will look at aligning the same in OidcLogoutEndpointFilter. I'll get to this soon as I have other priorities I'm currently working on.

[finke-ba]

@jgrandja Great to hear that.
May I offer my help with this issue and send you my pull request or would you like to do it yourself?

[jgrandja]

@finke-ba If you can submit a PR that would be great 👍

[finke-ba]

@jgrandja Perfect, I'll submit a PR and mention you to review it, hopefully this week!

[marcusdacoregio]

We found the need to customize the LogoutSuccessHandler used by OidcLogoutEndpointFilter to be able to terminate the sessions with the SAML 2.0 IdP. When we invoke /connect/logout the session is terminated in the authorization server, but, in our case, the SAML 2.0 IdP session is still valid. For that use case, I customized the OidcLogoutEndpointFilter to check how that implementation could look like.

I ended up making the performLogout method to always invoke the LogoutSuccessHandler, that by default could be a PostLogoutRedirectUriLogoutSuccessHandler or a custom one if needed, in our case a Saml2RelyingPartyInitiatedLogoutSuccessHandler.

Instead of passing the Authentication argument by doing (Authentication) oidcLogoutAuthentication.getPrincipal(), it could just pass oidcLogoutAuthentication and users could have an implementation that extracts the principal and passes it to their implementation, something like:

class OidcLogoutAuthenticationPrincipalLogoutSuccessHandler implements LogoutSuccessHandler {

    private final LogoutSuccessHandler delegate;

    // ...

    @Override
    public void onLogoutSuccess(HttpServletRequest request, HttpServletResponse response, Authentication authentication) throws IOException, ServletException {
        if (authentication instance of OidcLogoutAuthenticationToken oidc) {
            this.delegate.onLogoutSuccess(request, response, (Authentication) oidc.getPrincipal());
        }
    }

}

[jgrandja]

@finke-ba You can now customize the LogoutHandler like this:

OidcLogoutAuthenticationSuccessHandler oidcLogoutAuthenticationSuccessHandler =
		new OidcLogoutAuthenticationSuccessHandler();
oidcLogoutAuthenticationSuccessHandler.setLogoutHandler(customLogoutHandler);

authorizationServerConfigurer
	.oidc(oidc ->
		oidc
			.logoutEndpoint(logoutEndpoint ->
				logoutEndpoint.logoutResponseHandler(oidcLogoutAuthenticationSuccessHandler)));

[jgrandja]

@marcusdacoregio

We don't need to expose OidcLogoutEndpointFilter.setLogoutSuccessHandler(LogoutSuccessHandler logoutSuccessHandler as the existing OidcLogoutEndpointFilter.setAuthenticationSuccessHandler(AuthenticationSuccessHandler authenticationSuccessHandler) serves the same purpose - performing a redirect or a forward after successfully authenticating the OidcLogoutAuthenticationToken.

If you need to configure Saml2RelyingPartyInitiatedLogoutSuccessHandler, you can provide a custom AuthenticationSuccessHandler as follows:

public class CustomAuthenticationSuccessHandler implements AuthenticationSuccessHandler {
	private final LogoutHandler logoutHandler = new SecurityContextLogoutHandler();
	private LogoutSuccessHandler logoutSuccessHandler;		// TODO Init Saml2RelyingPartyInitiatedLogoutSuccessHandler

	@Override
	public void onAuthenticationSuccess(HttpServletRequest request, HttpServletResponse response, Authentication authentication) throws IOException, ServletException {
		this.logoutHandler.logout(request, response, authentication);
		this.logoutSuccessHandler.onLogoutSuccess(request, response, authentication);
	}

}

Makes sense?