Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

SEC-1643: ConcurrentSessionFilter does not register session after tomcat restart #1884

spring-issuemaster opened this Issue Dec 22, 2010 · 3 comments


None yet
1 participant

Jack Punt (Migrated from SEC-1643) said:

Login to web app from Firefox.
Stop and restart server (from STS)
Login to web app from Chrome (creates a new SessionInformation in sessionRegistry)
Refresh page in Firefox: it works! (old/persisted session is still valid, but it is not in sessionRegistry)
[if max-sessions="1", we would expect it to fail...]

I'm using ConcurrentSessionFilter to track the list of current/logged-in user;
so this [Firefox] session/user becomes unfindable.

Turns out the restored Session and Authentication is available!
So we add the else clause (line 107) to inject the session being used into the sessionRegistry:
[works for me...]

:security>diff -c ConcurrentSessionFilter.java~ ConcurrentSessionFilter.java
*** ConcurrentSessionFilter.java~ Wed Dec 22 15:02:56 2010
--- ConcurrentSessionFilter.java Wed Dec 22 15:01:47 2010

*** 26,31 ****
--- 26,32 ----
import javax.servlet.http.HttpSession;

import org.springframework.security.core.Authentication;

  • import org.springframework.security.core.context.SecurityContext;
    import org.springframework.security.core.context.SecurityContextHolder;
    import org.springframework.security.core.session.SessionInformation;
    import org.springframework.security.core.session.SessionRegistry;
    *** 103,108 ****
    --- 104,122 ----
    // Non-expired - update last request date/time
  •         } else {
  •             // Check for de-persisted Session:
  •             SecurityContext context = (SecurityContext)session.getAttribute("SPRING_SECURITY_CONTEXT");
  •             if (context != null) {
  •                 Authentication auth = context.getAuthentication();
  •                 if (auth != null) {
  •                     Object princ = auth.getPrincipal();
  •                     if (princ != null) {
  •                         // since Session survived restoration, re-register it:
  •                         sessionRegistry.registerNewSession(session.getId(), princ);
  •                     }
  •                 }
  •             }

Luke Taylor said:

This won't address the problem you report. The default SessionRegistry is an in-memory cache so is not designed to survive a server restart (or span multiple servers, for example). In order for concurrent session control to work, the state of the session registry would need to be restored on restart - your patch will only restore the state if the user makes a request on the session in question, hence it will not guarantee that the state is accurate. The user could easily open another session before re-using the existing one. If you want this to work properly, you need to write a persistent implementation of SessionRegistry which restores the state accurately on startup.

Burkhard Graves said:

Luke, I understand your concerns, but after restarting the server as described by Jack the state of the default SessionRegistry is inaccurate anyway (because its caches are empty - by the way, the directive "max-sessions=x" is checked against the SessionRegistry, which means, that x+1 sessions are possible after a server restart). Insofar Jacks hack makes the SessionRegistry only "more accourate" - or am I wrong?

And, instead of a persistent implementation of the SessionRegistry, what about the following idea:

  • Extend the HttpSessionEventPublisher in the following way: if a session is created put a HttpSessionActivationListener into the newly created session which only holds a backreference to the HttpSessionEventPublisher. If a sessionWillPassivate or sessionDidActivate is fired, the HttpSessionActivationListener can inform the HttpSessionEventPublisher such that corresponding ApplicationEvents (HttpSessionWillPassivateEvent, HttpSessionDidActivateEvent) are fired.
  • Now the SessionRegistryImpl can react to this new application events: removeSessionInformation() can (but must not) be called if a session will passivate and if a session did activate, it can be checked in the way described by Jack, such that a new sessionInformation can be registered.

Absolutely untested... ;-)

Sergey Klimenko said:

Luke, I have the same issue and I think it's still not solved but ConcurrentSessionFilter is not correct place where it can be solved. I've raised new issue for the problem: SEC-1832.

@spring-issuemaster spring-issuemaster added this to the 3.1.0.RC1 milestone Feb 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment