Expected Behavior
Provide a way to provide a redirect URL which depends on the request after OIDC logout.
Current Behavior
OidcClientInitiatedLogoutSuccessHandler.setPostLogoutRedirectUri(String postLogoutRedirectUri) can be used to statically set the redirect URL for all logout requests. It lacks a way to create the redirect URL dynamically depending on the HttpServletRequest.
Context
In my scenario I want to carry over query parameters to the redirect URL.
A user may be on /app1/page1 and wants to logout, so passes a query param to the /logout?clientId=47 endpoint (which is provided by Spring security) which then shall redirect afterwards to /app1/. Accordingly /logout?clientId=11 may lead to /app2/.
My initial WebSecurityConfig looked like the following
var handler = new OidcClientInitiatedLogoutSuccessHandler(oauthClientRepo);
handler.setPostLogoutRedirectUri("/app1/")
http.logout()
.logoutUrl("/logout")
.logoutSuccessHandler(handler)
This config always redirects to /app1/ regardless of the clientId query parameter.
As a workaround a wrote my own LogoutSuccessHandler which is basically a copy of OidcClientInitiatedLogoutSuccessHandler and changed only the part which builds the redirect URL to consider the query parameter that I envisioned. I dislike the code duplication as I want to benefit from updates made by others. The change that I did is very focussed (only the creation of the URL) and can be delegated to a plugin.
Comment From: jgrandja
Thanks for the report @sebsprenger. I left a comment and @jzheaux will be able to help with this further.
Comment From: jzheaux
@sebsprenger, thanks for the suggestion and the related PR.
A change may be reasonable, but I'd first like to understand your description better:
In my scenario I want to carry over query parameters to the redirect URL. A user may be on
/app1/page1and wants to logout, so passes a query param to the/logout?clientId=47endpoint (which is provided by Spring security) which then shall redirect afterwards to/app1/. Accordingly/logout?clientId=11may lead to/app2/.
I'm confused since you say that you want to carry over query parameters to the redirect URL, but your example redirect URLs (/app1/ and /app2/) don't have any query parameters. Am I misunderstanding your use case?
Comment From: sebsprenger
@jzheaux thanks for pointing that out. In my attempt to simplify I apparently eradicated some context as well. Let me try again with more details.
In my use case I have different apps redirecting to my app. In my app, I have to terminate the ADFS (Active Directory Federation Services) session if it is still active, and then have to do logic in ANY case. Some apps may redirect to my app with the ADFS session being already terminated. Then after doing my additional logic, I want to redirect back to the origin app. To do that I want to carry over some query parameters to let my app know where it needs to redirect the user to.
My current approach is a controller
@GetMapping(path = "/logout", params = {"clientId"})
String logout(@AuthenticationPrincipal OidcUser principal, @RequestParam ClientId clientId, HttpServletResponse response) {
if (principal != null) {
return "redirect:/terminateAdfsSession?clientId=" + clientId.getValue();
}
[more logic here that needs the clientId]
}
/terminateAdfsSession has been configured as
var handler = new OidcClientInitiatedLogoutSuccessHandler(oauthClientRepo);
handler.setPostLogoutRedirectUri("/logout")
http.
.logout()
.logoutUrl("/terminateAdfsSession")
.logoutSuccessHandler(handler)
Since OidcClientInitiatedLogoutSuccessHandler replaces the query in line 100 with null the clientId query parameter is lost.
For me it feels hard to tell whether not replacing the query parameters in line 100 would create ripple effects that I cannot foresee. Therefore I opted for plugin injection to replace the entire function. Making the class not final and the function protected as suggested by @jgrandja is fine by me. Much less code and I could extend OidcClientInitiatedLogoutSuccessHandler to override that one function as I would have done it with the interface implementation approach.
Does this help?
Comment From: jzheaux
That does help, @sebsprenger. Let me see if I've got things straight:
Apps redirect to /logout?clientId=11. That code conditionally redirects to /terminateAdfsSession?client=11. That in turn redirects to the authorization server. Once the authorization server completes its logout, it redirects back to the original endpoint /logout. You'd like the clientId=11 to still be in the URL, e.g. /logout?clientId=11, so that you can use that information to redirect back to the original app.
It can quickly get complicated relying on multiple parties to continue to pass around custom query parameters. I wonder if the state parameter from the OIDC spec isn't a better place for information like this:
state OPTIONAL. Opaque value used by the RP to maintain state between the logout request and the callback to the endpoint specified by the post_logout_redirect_uri parameter. If included in the logout request, the OP passes this value back to the RP using the state parameter when redirecting the User Agent back to the RP.
In my mind, the state parameter would be an opaque reference to the original request, including the clientId query parameter.
In addition to simplifying the contract between your app and the authorization server, which simplifies validation, the state parameter provides a CSRF defense mechanism for the post-logout redirect URI endpoint.
@jgrandja what are your thoughts about the logout success handler supporting the state parameter?
Comment From: sebsprenger
@jzheaux first things first: Reading your description of my scenario, I'd say we're on the same page ;-)
I must admit I was not aware of the state parameter at all. From what I read in the spec it would certainly do the trick.
I just gave it a go in my setup: Adding uriComponents.queryParam("state", "requestId-123"); lead ADFS to redirect back to /logout?state=requestId-123. I could change my controller to be able to deal with the state parameter in addition to the clientId parameter. So in the case where ADFS logout needs to happen I would afterwards have to rely on the state parameter and in cases where ADFS logout does not need to happen I can use clientId. I think I'd be slightly disappointed if I had two different parameters to do the same thing. I could have another endpoint /callback?state=requestId-123 which then redirects back to /logout?clientId=11.
How do you envision providing the value for the state parameter to OidcClientInitiatedLogoutSuccessHandler? Call /terminateAdfsSession?state=value?
Interestingly, the spec also says that the post_logout_redirect_uri may contain query parameters:
post_logout_redirect_uri OPTIONAL. URL to which the RP is requesting that the End-User's User Agent be redirected after a logout has been performed. This URL SHOULD use the https scheme and MAY contain port, path, and query parameter components; [...]
So apparently I was not trying to break the spec ;-) The way I see it is that either way could solve my problem.
I have an PR open, but I am also happy to rework it towards the protected way or think about a proposal to support the state parameter.
Comment From: jzheaux
So apparently I was not trying to break the spec ;-)
Agreed. :)
I think I'd be slightly disappointed if I had two different parameters to do the same thing
Also, agreed, though I'd note that the two things your application is doing are quite different. One is coordinating front-channel logout with a separate service and the other is local-only logout. It might be helpful for logout-start and logout-end to be separate endpoints that point to shared logic regardless.
Or, can your [more logic here that needs the clientId] happen before terminating the ADFS session? If so, you could try putting it into a LogoutHandler. This would save you one redirect and would make it possible to have a dynamic LogoutSuccessHandler like so:
private final Map<String, LogoutSuccessHandler> handlersByClientId = ...;
public void onLogoutSuccess(HttpServletRequest request, HttpServletResponse response, Authentication authentication) {
String clientId = request.getParameter("clientId");
validateClientId(authentication, clientId);
String appRedirectUri = computeAppRedirectUri(clientId);
this.handlersByClientId.computeIfAbsent(clientId, (k) -> {
var handler = new OidcClientInitiatedLogoutSuccessHandler(registrations);
handler.setPostLogoutRedirectUri(appRedirectUri);
handler.setDefaultTargetUrl(appRedirectUri);
}).onLogoutSuccess(request, response, authentication);
}
The idea here is that each single-tenant logout success handler would redirect to the original app or instruct ADFS to redirect to the original app for you. In this case, your configuration would change to:
http
.logoutUrl("/logout") // instead of `/terminateAdfsSession`
.addLogoutHandler(myAdditionalLogoutLogic())
.logoutSuccessHandler(myDynamicLogoutSuccessHandler());
and you'd remove the Spring MVC /logout endpoint.
Comment From: sebsprenger
One is coordinating front-channel logout with a separate service and the other is local-only logout. It might be helpful for logout-start and logout-end to be separate endpoints that point to shared logic regardless.
I agree, in this case they ought to be two different things, hence, I mentioned
I could have another endpoint /callback?state=requestId-123 which then redirects back to /logout?clientId=11.
earlier.
can your [more logic here that needs the clientId] happen before terminating the ADFS session?
It can. Your approach looks indeed interesting and promising. I went down a similar road before and got frustrated with the error handling as the response.sendError() didn't actually send out a body with the error message. Error handling is important to me here since the /login is basically the public API. The clientId being missing or incorrect needs to be properly handled.
Let me play around with that approach a bit tomorrow...
Comment From: sebsprenger
After pondering a bit I still tend to have an MVC controller handling the /logout?clientId=11 so I can use @ControllerAdvise to create a consistent error handling experience.
In your proposal you used a map as cache which in my scenario works just fine as I have do deal with a finite and manageable amount of client configurations, or in other words a limited amount of valid clientIds. A (maybe) less performant approach would be to create a new OidcClientInitiatedLogoutSuccessHandler on every request. I could further abstract to just pass query params along:
class MyDynamicLogoutSuccessHandler implements LogoutSuccessHandler {
private final static String REDIRECTION_PATH = "/logout";
@Override
public void onLogoutSuccess(HttpServletRequest request, HttpServletResponse response, Authentication authentication) throws IOException, ServletException {
var redirectUri = UriComponentsBuilder
.fromHttpUrl(UrlUtils.buildFullRequestUrl(request))
.replacePath(REDIRECTION_PATH)
.build().toUri();
var handler = new OidcClientInitiatedLogoutSuccessHandler(registrations);
handler.setPostLogoutRedirectUri(redirectUri.toASCIIString());
handler.onLogoutSuccess(request, response, authentication);
}
...
I'd still keep the /terminateAdfsSession as an "internal" (in the sense of private API) endpoint:
http
.logoutUrl("/terminateAdfsSession")
.logoutSuccessHandler(myDynamicLogoutSuccessHandler());
Although I find this approach to be an improvement over my current solution (copying lots of code), I am not too excited creating several (even if limited amount) OidcClientInitiatedLogoutSuccessHandlers. I'd still like this class to offer a tad more flexibility. Looking more closely at @jgrandja proposal to make OidcClientInitiatedLogoutSuccessHandler non-final and change OidcClientInitiatedLogoutSuccessHandler.postLogoutRedirectUri(HttpServletRequest request) from private to protected, I could write custom code which is very similar to the code above, but handles everything inside a single LogoutSuccessHandler:
public class DynamicOidcClientInitiatedLogoutSuccessHandler extends OidcClientInitiatedLogoutSuccessHandler {
private final static String REDIRECTION_PATH = "/logout";
@Override
protected URI postLogoutRedirectUri(HttpServletRequest request) {
return UriComponentsBuilder
.fromHttpUrl(UrlUtils.buildFullRequestUrl(request))
.replacePath(REDIRECTION_PATH)
.build().toUri();
}
...
}
What are your thoughts on Joe's proposal?
Comment From: sebsprenger
Hey @jzheaux , it's my first time trying to get something into Spring so I am kinda relying on your guidance. How do we proceed from here?
Comment From: jzheaux
Sorry for the delay. I'm still pondering this a bit myself.
After pondering a bit I still tend to have an MVC controller handling the
/logout?clientId=11
That's reasonable. In your controller, you can call the LogoutHandler and (set of) LogoutSuccessHandler(s) directly instead of coordinating through LogoutFilter.
In your proposal you used a map as cache which in my scenario works just fine
Glad to hear it!
By the way, I agree constructing a handler per request would not be ideal, but it also doesn't seem necessary. Were you just sharing that for illustration?
I'd still keep the /terminateAdfsSession as an "internal" (in the sense of private API) endpoint
Are you able to validate the clientId if the user is logged out? One concern with doing /logout -> /terminateAdfsSession -> AuthZ -> /logout -> /app is that the user is no longer logged in on the second /logout request. I'd be wary of accepting unvalidated input.
If that's an issue, you can call the logout APIs directly from your controller, which gives you /logout -> AuthZ -> /app. In addition to a simpler security model, it's also fewer browser hops and a more tightly defined per-tenant post_logout_redirect_uri value.
What are your thoughts on Joe's proposal?
I'm not opposed to making the class non-final if that's the best choice, and then you could invoke and modify the result of determineTargetUrl since that's already protected.
That said, I'd still prefer a composition solution if there is one. There are alternatives I'm still considering, so I'm not ready yet to accept a PR along those lines.
Comment From: sebsprenger
In your controller, you can call the LogoutHandler and (set of) LogoutSuccessHandler(s) directly instead of coordinating through LogoutFilter.
If I understand you correctly, you are suggesting to call logoutHandler.onLogoutSuccess(request, response, authentication) from within the controller. While the logout success handler operates directly on the response rather than returning the redirect URL, my current controller function returns a string with the "redirect:" prefix. To accommodate using the logout success handler from within the controller, I would need to change the controller function to void and operate on the response directly in every case (I'd be happy to be proven wrong). Just as a reminder I have two branches here "redirect:/terminateAdfsSession?clientId=" + clientId.getValue() and "redirect:" + client.getLogoutRedirectUrl(). While all of this can be done, I am much in favor of the straight-forwardness of returning a string with the redirect: prefix.
By the way, I agree constructing a handler per request would not be ideal, but it also doesn't seem necessary. Were you just sharing that for illustration?
Yes, my intent was/is to illustrate the example as plain as possible. The map reduces the amount of objects, my ideal here would be a reduction to one object. Hence, I think of that map as an optimization, but not an essential part of the discussion. Do you agree?
One concern with doing /logout -> /terminateAdfsSession -> AuthZ -> /logout -> /app is that the user is no longer logged in on the second /logout request. I'd be wary of accepting unvalidated input.
Thanks for pointing that out, really interesting question! I think it doesn't really matter in my case, but I want to check back with my team to think it through.
If that's an issue, you can call the logout APIs directly from your controller, which gives you /logout -> AuthZ -> /app. In addition to a simpler security model, it's also fewer browser hops and a more tightly defined per-tenant post_logout_redirect_uri value.
For that to work I need to register the callback URLs of my clients with the upstream ADFS. In our setup this cannot be done in a self-service fashion, so I would need to open a ticket with our IT, which is suboptimal for time2market. I'd like to avoid that.
I'm not opposed to making the class non-final if that's the best choice, and then you could invoke and modify the result of determineTargetUrl since that's already protected.
I understood Joe's proposal in a way, that we could change OidcClientInitiatedLogoutSuccessHandler.postLogoutRedirectUri() to protected. If I need to call or override determineTargetUrl(), then I don't really see the benefit over extending SimpleUrlLogoutSuccessHandler myself. That's actually what I did so far to accomplish my goal, but I want to benefit from OidcClientInitiatedLogoutSuccessHandler and future changes made to it. Overriding postLogoutRedirectUri() would allow me to change those few lines that I really want to behave differently and otherwise build upon OidcClientInitiatedLogoutSuccessHandler.
That said, I'd still prefer a composition solution if there is one.
I am a bit confused and hope you can help me out. Isn't the PR that I suggested a composition solution? It changes OidcClientInitiatedLogoutSuccessHandler to work with an interface and there are different implementations which can be provided to change the details in behavior. I'd like to understand what a "composition solution" would feel like to you - then I might be able to think in the same direction ;-)
Comment From: jzheaux
I appreciate the points you've shared, and I think we can continue exploring the tradeoffs as time permits.
In the meantime, this solution may be worth consideration:
public class CustomLogoutSuccessHandler implements LogoutSuccessHandler {
private final OidcClientInitiatedLogoutSuccessHandler delegate;
// ...
public void onLogoutSuccess(HttpServletRequest request,
HttpServletResponse response, Authentication authentication)
throws IOException, ServletException {
var wrapped = new HttpServletResponseWrapper(response) {
@Override
public void sendRedirect(String location) throws IOException {
String uri = postLogoutRedirectUri(request);
String withUri = // add parameter to location
super.sendRedirect(withUri);
}
};
this.delegate.onLogoutSuccess(request, wrapped, authentication);
}
private String postLogoutRedirectUri(HttpServletRequest request) {
// compute and return
}
}
It appears to be an improvement over what your app does today since it doesn't copy code from OidcClientInitiatedLogoutSuccessHandler.
Comment From: jzheaux
@sebsprenger, thanks for your patience while I've been finding time to come back to this issue.
While some of the following is a restatement, I'd like to post an update on my analysis:
-
Even though Spring Security favors delegation over sub-classing,
OidcClientInitiatedLogoutSuccessHandlersub-classesSimpleUrlLogoutSuccessHandler.LogoutSuccessHandler's contract makes delegation tricky since the result cannot be post-processed (the redirect has already been sent). So there's some evidence that makingOidcClientInitiatedLogoutSuccessHandlernon-final is best. -
When delegation is not simple, Spring Security favors setter methods, similar to the one you proposed. This isn't ideal here, though, since having two setters --
setPostLogoutRedirectUriandsetPostLogoutRedirectUriResolver-- could be confusing and there isn't a strong argument for replacingsetPostLogoutRedirectUriwithsetPostLogoutRedirectUriResolver. -
Given that your situation arises from multi-tenancy, there is some appeal to querying the
ClientRegistrationfor the tenant-specific data. For example, I've wondered if using theregistrationIdwould work for you:java handler.setPostLogoutRedirectUri("{baseUrl}/logout?clientId={registrationId}")or if you could pre-compute eachpost_logout_request_uriand store in each correspondingClientRegistrationlike so:java ClientRegistration registration(String issuer, String clientId) { String redirectUri = computRedirectUri(clientId); return ClientRegistrations.fromIssuerLocation(issuer) .registrationId(clientId) .configurationMetadata((metadata) -> metadata.put("post_logout_redirect_uri", redirectUri)) .build(); }In addition to less configuration, these have the advantage of being easier to secure since the URLs are not being computed at request-time and are thus better protected against open redirect.
Both of these assume that the dynamic aspect of the URI can be conceived as being static per-tenant. Does that sound reasonable for your use case?
- If the URI computation must be done at request time, I think we should move forward with making the class non-final as this is more intuitive than the workaround I posted a few months ago.
Comment From: spring-projects-issues
If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.
Comment From: schabch
For our use case the dynamic aspect of the URI is static per-tenant. We would appreciate either of the 2 proposed solutions.
Comment From: jzheaux
Awesome, we'll close this ticket then in favor of #7900 and #9669. Please feel free to volunteer for either of those if you are interested.