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

Support customizing the redirect URL in OidcClientInitiatedServerLogoutSuccessHandler #14778

Open
stipx opened this issue Mar 19, 2024 · 9 comments · May be fixed by #14808
Open

Support customizing the redirect URL in OidcClientInitiatedServerLogoutSuccessHandler #14778

stipx opened this issue Mar 19, 2024 · 9 comments · May be fixed by #14808
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: feedback-provided Feedback has been provided type: enhancement A general enhancement

Comments

@stipx
Copy link

stipx commented Mar 19, 2024

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.

@stipx stipx added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Mar 19, 2024
@programista-zacny
Copy link

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 pushed a commit to franticticktick/spring-security that referenced this issue Mar 27, 2024
@franticticktick
Copy link
Contributor

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));

@sjohnr sjohnr added the in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) label Mar 28, 2024
@jzheaux
Copy link
Contributor

jzheaux commented Apr 1, 2024

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
Copy link
Contributor

jzheaux commented Apr 1, 2024

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

@jzheaux jzheaux added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 1, 2024
@franticticktick
Copy link
Contributor

franticticktick commented Apr 1, 2024

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?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Apr 1, 2024
@franticticktick
Copy link
Contributor

@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
Copy link

programista-zacny commented Apr 2, 2024

@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
Copy link
Contributor

jzheaux commented Apr 17, 2024

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.

@jzheaux jzheaux added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Apr 17, 2024
franticticktick pushed a commit to franticticktick/spring-security that referenced this issue Apr 17, 2024
@franticticktick
Copy link
Contributor

@jzheaux good solution! I have updated PR.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Apr 17, 2024
franticticktick pushed a commit to franticticktick/spring-security that referenced this issue Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: feedback-provided Feedback has been provided type: enhancement A general enhancement
Projects
None yet
6 participants