SEC-1832: SessionRegistry doesn't work after restart

[spring-projects-issues]

Sergey Klimenko (Migrated from SEC-1832) said:

Hi team,

I've read SEC-1643 issue and agree with resolution but the problem still exist: SessionRegistry doesn't work after server bounce because SecurityContext is restored from serialized session and SessionRegistry knows nothing about that. I'm not sure that it's good idea to implement serializable SessionRegistry (anyway, it should be part of library but not my custom implementation) class. There is no points when the registry should be stored and loaded from disk and there is no way synchronize it with original server sessions serialization.

Could you please think about any other solutions?

Probably, it should be done in SecurityContextPersistenceFilter because it manages states of security context and I see only one appropriate point where it can be done: in finally section. If after the filter execution there is no appropriate information about session in SessionRegistry it should be created. So code should be changed to something like the following code:

    HttpRequestResponseHolder holder = new HttpRequestResponseHolder(request, response);
    SecurityContext contextBeforeChainExecution = repo.loadContext(holder);

    try {
        SecurityContextHolder.setContext(contextBeforeChainExecution);

        chain.doFilter(holder.getRequest(), holder.getResponse());

    } finally {
        SecurityContext contextAfterChainExecution = SecurityContextHolder.getContext();
        // Crucial removal of SecurityContextHolder contents - do this before anything else.
        SecurityContextHolder.clearContext();
        repo.saveContext(contextAfterChainExecution, holder.getRequest(), holder.getResponse());

        /////////////////////// NEW CODE BEGIN ///////////////////
         SessionInformation info = sessionRegistry.getSessionInformation(session.getId());
         if (info == null) { // not sure about contextAfterChainExecution.getAuthentication() != null
           sessionRegistry.registerNewSession(session.getId(), contextAfterChainExecution.getAuthentication());
         }

        /////////////////////// NEW CODE END ///////////////////

        request.removeAttribute(FILTER_APPLIED);

        if (debug) {
            logger.debug("SecurityContextHolder now cleared, as request processing completed");
        }
    }

[spring-projects-issues]

Luke Taylor said:

Again, this is not a bug. The session registry implementation is not intended to survive a server restart, and adding in these kind of patches is not a proper solution because it allows the user to exceed the number of allocated sessions. The proper solution is to implement a persistent SessionRegistry, which also solves the issue of load-balancing across multiple servers using sticky sessions.

[spring-projects-issues]

Sergey Klimenko said:

Hi Luke,

I'm agree with you but in this case persistent implementation has to be provided by the library. Otherwise we have a issue and, for example, at this moment it really affects my application because I can't tract users after bounce.

What about SessionManagementFilter or SessionAuthenticationStrategy? Actually, as work around, at this moment I have created new filter and inserted it after SESSION_MANAGEMENT_FILTER:

public class SessionRegistryFixFilter extends GenericFilterBean {
private SessionRegistry sessionRegistry;

@Override
public void doFilter(ServletRequest req, ServletResponse res, FilterChain chain) throws IOException, ServletException {
    final HttpServletRequest request = (HttpServletRequest) req;

    final HttpSession session = request.getSession(false);
    if (session != null) {
        final SessionInformation info = sessionRegistry.getSessionInformation(session.getId());
        if (info == null) {
            Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
            if (authentication != null && authentication.getPrincipal() != null) {
                sessionRegistry.registerNewSession(session.getId(), authentication.getPrincipal());
            }
        }
    }
    chain.doFilter(req, res);
}

public void setSessionRegistry(SessionRegistry sessionRegistry) {
    this.sessionRegistry = sessionRegistry;
}

}

It works for me very well.

[spring-projects-issues]

Luke Taylor said:

I don't know what you mean by "has to be provided by the library".

Your solution won't work with load-balanced sessions and won't stop the user from creating an additional session, but if it suits your requirements then go for it.

[spring-projects-issues]

Paul Khodchenkov said:

Hi all,
I am currently setuping spring with tomcat 7 cluster [1]
Using tomcat's DeltaManager for session replication causes onSessionCreated,onSessionDestoryed events to be fired on all tomcat's nodes. Also onSessionCreated is fired on startup for every session when tomcat's node joins the cluster. There is no need to store something in database in this case.
The only improvement is that SessionRegistryImpl should listen also for SessionCreatedEvent and check the existense of authentication context:
void onSessionCreated() {
//need to restore authentication mapping if exist
Authentication authentication = (Authentication) session.getAttribute(SECURITY_CONTEXT_KEY);
if (authentication != null) {
registerNewSession(session.getId(),authentication.getPrincipal());
}
}

[1]

[spring-projects-issues]

Burkhard Graves said:

@paul (and @luke), that's similar to the stuff I've proposed in SEC-1643 (except that I thought about listening to HttpSessionDidActivateEvent) - should be investigated. I also believe that there is no need for a persistent implementation of SessionRegistry (at least on a properly working servlet engine ;-)).

[spring-projects-issues]

Paul Khodchenkov said:

There is also a problem with persistent registry if we have a requirement that admin can force logout for any user (or on some criteria based on session attributes). Servlet specification does not provide a way to get HttpSession object by id. The only way to store HttpSession in memory in hashmap directly. So SessionRegistryImpl can store Map<String, HttpSession> instead of Map<String, SessionInformation> sessionIds. This solution also solves the problem with refreshLastRequest (by default it will not work in clustered environment because changes to lastAccessTime is not visible to another node):

HttpSession session = httpSessionMap.get(sessionId);
result.add(new SessionInformation(principal, sessionId,
new Date(session.getLastAccessedTime())));

This solution works fine with tomcat7 cluster (using DeltaManager for session clustering)

[spring-projects-issues]

Paul Khodchenkov said:

I am afraid that SessionRegistry implementation should be specific to each session manager strategy used in tomcat. StandardManager,PersistentManager,DeltaManager fire http session created/destroyed/passivate/activate events in differents ways.

1. StandardManager : will fire session activate event for every serialized session on startup (HttpSessionActivationListener object should be stored inside a session), but does not fire http session created event
2. DeltaManager : fire http session created event for every session on startup
3. PersistentManager : does NOT load session on startup. Sessions are restored lazily(and http session created event is fired) only when the required session is accessed by http.

[spring-projects-issues]

Luke Taylor said:

SessionRegistryImpl was never designed to survive a server restart. Other solutions attempting to re-register the session when it is used are incomplete and would allow users to potentially exceed the number of allowed sessions.

Also they won't work with sticky-session load-balancing which is probably the most common deployment scenario with multiple servers.