Support customizing the redirect URL in OidcClientInitiatedServerLogoutSuccessHandler

[stipx]

Expected Behavior

In order to be able to work with some restrictive SSO implementations sometimes additional parameters are needed (like "state") in order that the logout request is succeeding.

Current Behavior

In order to achieve this it was necessary to implement a custom logout handler which gets the logout/end_session URL from the client registration and sets the ID-Token hint, the redirect uri and the state and does a redirect then.

So a simple resolver/converter/customizer for the redirect URL would be much easier than implementing a whole new logout handler.

[programista-zacny]

I agree.

We needed to redirect to dynamic url when OIDC logout. location query param was used. E.g. /logout?location=http://somewhere.com

@RequiredArgsConstructor
public class RedirectOidcServerLogoutSuccessHandler implements ServerLogoutSuccessHandler {

    private final ReactiveClientRegistrationRepository clientRegistrationRepository;

    @Override
    public Mono<Void> onLogoutSuccess(WebFilterExchange exchange, Authentication authentication) {
        OidcClientInitiatedServerLogoutSuccessHandler delegate =
                new OidcClientInitiatedServerLogoutSuccessHandler(clientRegistrationRepository);
        getLocation(exchange).ifPresent(delegate::setPostLogoutRedirectUri);
        return delegate.onLogoutSuccess(exchange, authentication);
    }

    private Optional<String> getLocation(WebFilterExchange exchange) {
        return Optional.ofNullable(exchange.getExchange().getRequest().getQueryParams().getFirst("location"));
    }
}

[franticticktick]

I can suggest a simple solution - add setServerLogoutSuccessHandler. This will allow you to implement your RedirectServerLogoutSuccessHandler and pass it to OidcClientInitiatedServerLogoutSuccessHandler, for example:

    public class RedirectOidcServerLogoutSuccessHandler extends RedirectServerLogoutSuccessHandler {

        private final ReactiveClientRegistrationRepository clientRegistrationRepository;

        public RedirectOidcServerLogoutSuccessHandler(ReactiveClientRegistrationRepository clientRegistrationRepository) {
            this.clientRegistrationRepository = clientRegistrationRepository;
        }

        @Override
        public Mono<Void> onLogoutSuccess(WebFilterExchange exchange, Authentication authentication) {
            OidcClientInitiatedServerLogoutSuccessHandler delegate =
                    new OidcClientInitiatedServerLogoutSuccessHandler(clientRegistrationRepository);
            getLocation(exchange).ifPresent(delegate::setPostLogoutRedirectUri);
            return delegate.onLogoutSuccess(exchange, authentication);
        }

        private Optional<String> getLocation(WebFilterExchange exchange) {
            return Optional.ofNullable(exchange.getExchange().getRequest().getQueryParams().getFirst("location"));
        }
    }

Then:

OidcClientInitiatedServerLogoutSuccessHandler handler = new OidcClientInitiatedServerLogoutSuccessHandler(this.repository);
handler.setServerLogoutSuccessHandler(new RedirectOidcServerLogoutSuccessHandler(repository));

[jzheaux]

While a clever solution, @CrazyParanoid, I think it would be preferable to align with the imperative component in this case and have a protected method. While it's rare in Spring Security to open a method up for overriding, delegation in this case requires a bit of gymnastics since it is not possible to operate on the return object.

Instead of setServerLogoutSuccessHandler, what if we did this:

protected Mono<String> determineLogoutUri(WebFilterExchange exchange, Authentication authentication) {
    return this.clientRegistrationRepository::findByRegistrationId).flatMap((clientRegistration) -> {
		URI endSessionEndpoint = endSessionEndpoint(clientRegistration);
		if (endSessionEndpoint == null) {
			return Mono.empty();
		}
		String idToken = idToken(authentication);
		String postLogoutRedirectUri = postLogoutRedirectUri(exchange.getExchange().getRequest(), clientRegistration);
		return Mono.just(endpointUri(endSessionEndpoint, idToken, postLogoutRedirectUri));
    });
}

Then, an application can override this method to change the URI dynamically.

[jzheaux]

@CrazyParanoid and @programista-zacny, I'm sorry. I should have looked at your proposal more carefully. Redirecting to a location specified in a query parameter is not advised as this is vulnerable to open redirect. You should not redirect to a location that a client can arbitrarily specify, say through a request parameter like location.

Instead, the locations to which you can redirect should be something that the server controls. Before proceeding, then, I'd like to understand better what you are trying to do. This will ensure that we don't add a feature for an insecure reason.

[franticticktick]

Hi @jzheaux this is a very interesting solution. I considered this solution, but I was confused by the fact that such an implementation is rarely found in the spring security API and developers are mostly accustomed to work with lambda-based factories. Maybe we should try to find such a solution? We can extract the logic for determining the URL to lambda-factory

	private class DefaultLogoutUriFactory implements BiFunction<WebFilterExchange,Authentication, Mono<String>> {

		@Override
		public Mono<String> apply(WebFilterExchange exchange, Authentication authentication) {
			return Mono.just(authentication)
					.filter(OAuth2AuthenticationToken.class::isInstance)
					.filter((token) -> authentication.getPrincipal() instanceof OidcUser)
					.map(OAuth2AuthenticationToken.class::cast)
					.map(OAuth2AuthenticationToken::getAuthorizedClientRegistrationId)
					.flatMap(clientRegistrationRepository::findByRegistrationId)
					.flatMap((clientRegistration) -> {
						URI endSessionEndpoint = endSessionEndpoint(clientRegistration);
						if (endSessionEndpoint == null) {
							return Mono.empty();
						}
						String idToken = idToken(authentication);
						String postLogoutRedirectUri = postLogoutRedirectUri(exchange.getExchange().getRequest(), clientRegistration);
						return Mono.just(endpointUri(endSessionEndpoint, idToken, postLogoutRedirectUri));
					});
		}
	}

Then we define:

private BiFunction<WebFilterExchange,Authentication, Mono<String>> logoutUriFactory = new DefaultLogoutUriFactory();

@Override
public Mono<Void> onLogoutSuccess(WebFilterExchange exchange, Authentication authentication) {
	return logoutUriFactory.apply(exchange, authentication)
				.switchIfEmpty(
						this.serverLogoutSuccessHandler.onLogoutSuccess(exchange, authentication).then(Mono.empty())
				)
				.flatMap((endpointUri) -> this.redirectStrategy.sendRedirect(exchange.getExchange(), URI.create(endpointUri)));
}

public void setLogoutUriFactory(BiFunction<WebFilterExchange, Authentication, Mono<String>> logoutUriFactory) {
     Assert.notNull(serverLogoutSuccessHandler, "logoutUriFactory cannot be null");
     this.logoutUriFactory = logoutUriFactory;
}

Now you can easily determine the URL:

this.handler.setLogoutUriFactory((ex, auth) -> Mono.just(
				Objects.requireNonNull(
						ex.getExchange()
								.getRequest()
								.getQueryParams()
								.getFirst("location")
				)
		));

What do you think about this solution?

[franticticktick]

@jzheaux I agree it looks unsafe. @stipx said: "In order to be able to work with some restrictive SSO implementations sometimes additional parameters are needed (like "state") in order that the logout request is succeeding." It seems that such implementation resolves this case, but at the same time it can add potential vulnerabilities if the developer is careless.

[programista-zacny]

@jzheaux In my case, Keycloak still controls valid redirect urls :) It's not totally up to user of the logout, it can be somehow "dynamically" but in the end Keycloak will validate if the redirect url is safe (there is a list of valid redirect urls in Keycloak).

[jzheaux]

Nice idea, @CrazyParanoid, though, I'd prefer not to expose a BiFunction as it's not as self-documenting and, perhaps more importantly, the same outcome can be achieved with Function and a wrapper object. A wrapper object makes it easier to have more than two parameters and also get the servlet and webflux implementations aligned.

So instead, could we do setRedirectUriResolver(Converter<RedirectUriParameters, Mono<String>>)? RedirectUriParameters would be a static inner class that contains the ServerWebExchange, the Authentication, and the ClientRegistration.

[franticticktick]

@jzheaux good solution! I have updated PR.

[jzheaux]

Closing in favor of https://github.com/spring-projects/spring-security/pull/14808