Spring Security
  1. Spring Security
  2. SEC-1643

ConcurrentSessionFilter does not register session after tomcat restart

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Won't Fix
    • Affects Version/s: 3.0.5
    • Fix Version/s: 3.1.0.RC1
    • Component/s: Web
    • Labels:
      None

      Description

      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
            info.refreshLastRequest();
            }
            + } else {
            + // Check for de-persisted Session:
            + SecurityContext context = (SecurityContext)session.getAttribute("SPRING_SECURITY_CONTEXT");
            + if (context != null) {
            + Authentication auth = context.getAuthentication();
            + if (auth != null)
            Unknown macro: {+ Object princ = auth.getPrincipal();+ if (princ != null) { + // since Session survived restoration, re-register it: + sessionRegistry.registerNewSession(session.getId(), princ); + }+ }

            + }
            }
            }

        Activity

        Hide
        Luke Taylor added a comment -

        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.

        Show
        Luke Taylor added a comment - 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.
        Hide
        Burkhard Graves added a comment -

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

        Show
        Burkhard Graves added a comment - 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...
        Hide
        Sergey Klimenko added a comment -

        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.

        Show
        Sergey Klimenko added a comment - 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 .

          People

          • Assignee:
            Luke Taylor
            Reporter:
            Jack Punt
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: