Do not wipe out current session when calling findSession with other session IDs

[michaelcameron]

The contract of o.a.c.Manager.findSession is to just return the active session that matches the passed in session ID and does not specify any side-effects with the current session. In the current implementation if you ask for a session that's not the current session it changes the currentSession ThreadLocal which will then cause the current session to not be persisted in afterRequest.

We ran into this problem with Spring Security's session fixation prevention:

• User loads login page in the browser and remains idle long enough for the session to expire
• User uses login page to log in
• The request will include an expired session ID. When tomcat can't find the session it will create a new session with a new ID.
• The session fixation prevention code will look to see if the requested session ID was valid (it doesn't need to create a new session if the one asked for isn't being used). The implementation of this will call Manager.findSession with the original requested session ID
• The tomcat redis session manager will then wipe out the currentSession ThreadLocal causing the current session to not be persisted

If I understand the reasoning behind the implementation, you were trying to make sure the session requested gets persisted in case the caller mutates the returned session. But this leads to the existing, valid, current session not getting persisted. To make sure both the current session and any sessions asked for by findSession get persisted there needs to be a mechanism to track all sessions returned by findSession that are not the currentSession and make sure those are persisted after the request. However, that's complicated by the fact that multiple concurrent threads could have called findSession for the same session ID. I feel like the implementation should at least honor the currentSession without wiping it out on calls to findSession. Best case you've asked for the currentSession, which just returns that existing object, worst case you're asking for other sessions and even the current implementation can't guarantee that the session object returned will get persisted, just the last one asked for.

[jcoleman]

While it'd be possible to implement the contract you mention, your PR as existing breaks the current implementation pretty fundamentally, since the currentSession cache is how the manager knows to persist the session after the request.

If you're interesting in fixing this, you'd have to create some kind of set that gets stored in a thread local--but even then you'd run into problems with multiple threads. The problem here is that the fundamental nature of a session in the traditional implementation is one that is "persisted" on change, rather than being persisted after the request. Since we're not saving until after the request, there's a fundamental incompatibility there with multiple threads since the manager is a singleton.

I definitely can't accept the patch as is. I can definitely see a possible implantation that makes multiple session requests succeed under a single-threaded model, but handling the multi-threaded model is going to be far more difficult.

[michaelcameron]

I'm not seeing how the PR breaks the current implementation. The only case where findSession sets a non-null currentSession is where the sessionId passed to findSession is found in Redis but is not the current session. The current implementation is broken because it will fail to persist the old non-null session that was assigned to currentSession prior to this call. In all of the other cases it either clears the possible non-null currentSession without replacing or does not affect the currentSession.

It does swap one case of not saving a session for another, but my argument is that it's better to preserve the currentSession set in other parts of the code (e.g. The session created in createSession) then to swap it out for an arbitrary session that was asked for from findSession that isn't necessarily being mutated (but the one created in createSession most certainly needs persisting).

If you don't agree, then I propose to eliminate the cases of clearing current session in the null ID case (lines 425-428) and a non-null session ID but not found in redis (lines 441-444), but revert the change in the PR that swaps one session for another (lines 436-439). That would still fix the current source of the defect that we found.

[michaelcameron]

I think I understand now. I've been focusing on new sessions, but the new problem would crop up with an existing session ID that is found in Redis. Would you be okay with my proposal from my last comment, to revert just lines 436-439 (I'll change the PR in anticipation so it's easier for you to just merge when you have time if you agree).