Include FilterChain on SessionInformationExpiredEvent to allow continuing the request

[arvyy]

I expect to always be able to invoke endpoints marked with permitAll. However, ConcurrentSessionFilter early aborts such requests at the edge of expiration.

spring-security/web/src/main/java/org/springframework/security/web/session/ConcurrentSessionFilter.java

Line 145 in ed6ff67

return;

and there doesn't seem a way to sidestep this early return through SessionInformationExpiredStrategy because it doesn't expose FilterChain to call doFilter on.

[marcusdacoregio]

Hi, @arvyy, thanks for the report.

permitAll means that it will allow requests with/without authentication to go through, but Spring Security filters will continue doing what they do. Therefore, since you have concurrent session control, if you perform a request with a session that has expired by concurrency, the ConcurrentSessionFilter won't allow that request to go through.

Can you elaborate more on your use case? How does the session expire? What are those requests that you always want to go through?

[arvyy]

I have a /currentUser endpoint that I want to work regardless if user is logged in or not (in latter case it'd return null). On frontend I create a timer until the end of the session (which I know from an auxiliary cookie). At the end of the timer, I call /currentUser, and if it weren't null (because user was browsing in different tab), I restart the timer. If it was null, I know user has been idle too long and change frontend view to the login screen. This is currently broken, because frontend instead of receiving a json response of an expected form, it instead receives a plain text message "This session has been expired ...".

I understand that permitAll still invokes security filters, and that's desired, because I still want to be able to fetch (just potentially empty) security context's user authentication value. My problem isn't with design as a whole, but with inflexibility of ConcurrentSessionFilter specifically.

[marcusdacoregio]

It seems to me that your /currentUser endpoint should require authentication and instead of returning null when the user is not authenticated, it should return a 401. This way you can provide a SessionInformationExpiredStrategy implementation that returns 401 and your frontend code can be adapted to expect it.

But, if you do not want to do that and instead return null, you can provide a SessionInformationExpiredStrategy implementation that writes nothing to the response. Does that make sense?

[arvyy]

Doesn't seem like a robust solution. SessionInformationExpiredStrategy shouldn't be tight coupled to specific endpoint's concerns, especially since there can be more than one permitAll endpoint in an application (albeit the one I shared is the one most likely to trigger this issue). As a current workaround I'll probably reimplement the concurrent session filter altogether.

[marcusdacoregio]

As I mentioned before, a "robust" solution would be to return 401 since the user has provided some credentials (session id) but that session is not valid anymore, the filter is not aware of specific endpoint's concerns as well.

[arvyy]

I see, I thought you were commenting about my specific example solution.

return 401 since the user has provided some credentials (session id) but that session is not valid anymore

As a default general case, a 401 isn't consistent with broader spring security behaviour. Usually calling into spring with a bad sessionid cookie will simply make spring treat you as an anonymous user: if a given endpoint is allowed to be accessed by anonymous users, the request will succeed with a 200, not killed with a 401 just because sessionid was bad.

[marcusdacoregio]

I have been thinking about this and it seems reasonable to include the FilterChain inside the SessionInformationExpiredEvent, you can do the following:

static class ContinueRequestSessionInformationExpiredStrategy implements SessionInformationExpiredStrategy {

	@Override
	public void onExpiredSessionDetected(SessionInformationExpiredEvent event) throws ServletException, IOException {
		event.getFilterChain().doFilter(event.getRequest(), event.getResponse());
	}

}

This way the return statement is not removed, keeping the behavior of some strategies where they might not want to continue the chain, and new strategies can choose to continue if they want to.

That being said, this would be an enhancement in the behavior, therefore I'll schedule this for 6.3.0-M1. Would you be interested in submitting a PR? Ideally, the SessionInformationExpiredEvent would have a new FilterChain field and a new constructor that accepts the FilterChain alongside other parameters.

[marcusdacoregio]

I wonder if, instead of saving the session with the status expired in the SessionRegistry to later on be checked by the ConcurrentSessionFilter, we could perform the actual logout with a POST /logout, similar to what was done to OIDC BackChannel Logout

spring-security/config/src/main/java/org/springframework/security/config/web/server/OidcBackChannelServerLogoutHandler.java

Lines 105 to 117 in 67d3e4c

	private Mono<ResponseEntity<Void>> eachLogout(WebFilterExchange exchange, OidcSessionInformation session) {
	HttpHeaders headers = new HttpHeaders();
	headers.add(HttpHeaders.COOKIE, this.sessionCookieName + "=" + session.getSessionId());
	for (Map.Entry<String, String> credential : session.getAuthorities().entrySet()) {
	headers.add(credential.getKey(), credential.getValue());
	}
	String url = exchange.getExchange().getRequest().getURI().toString();
	String logout = UriComponentsBuilder.fromHttpUrl(url)
	.replacePath(this.logoutEndpointName)
	.build()
	.toUriString();
	return this.web.post().uri(logout).headers((h) -> h.putAll(headers)).retrieve().toBodilessEntity();
	}

[Ilpyo-Yang]

Hello @marcusdacoregio may i handle this issue? please assign to me. i will make pr soon :)

[Ilpyo-Yang]

@marcusdacoregio
I've submitted a PR to create a separate strategy class for ContinueRequestSessionInformationExpiredStrategy.

1. Alternatively, would it be better to apply ContinueRequestSessionInformationExpiredStrategy as the default strategy and add it to the ConcurrentSessionFilter?

public ConcurrentSessionFilter(SessionRegistry sessionRegistry) {
      Assert.notNull(sessionRegistry, "SessionRegistry required");
      this.sessionRegistry = sessionRegistry;
      this.sessionInformationExpiredStrategyArray = new SessionInformationExpiredStrategy[]{
              new ResponseBodySessionInformationExpiredStrategy(),
              new ContinueRequestSessionInformationExpiredStrategy()};
}

2. How about creating a separate PR or issue for handling expired session performed by POST /logout?

[arvyy]

Sorry for dropping the ball; I was busy at the time, and managed to forget about it later on when I was free. Let me know if you need me