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-1307: Investigate optimization of logic in HttpSessionSecurityContextRepository which checks for changes in the SecurityContext #1552

spring-issuemaster opened this Issue Nov 24, 2009 · 2 comments


None yet
1 participant

Luke Taylor (Migrated from SEC-1307) said:

The current implementation uses the hashcode of the context to check for a change, and only write the context to the session if it has altered. The reason for this logic is to prevent a thread handling a slow request from overwriting the context in the case where an authentication has occurred in another thread, which has already completed and written the authentication to the session (SEC-37). Tests have shown that the calculation of the hashcode on each request is actually one of the most expensive operations in an invocation of the filter chain.

For an authenticated user, the most common situation will be that neither the SecurityContext or the (essentially immutable) Authentication it contains will have changed. It should be possible to come up with a set of situations which don't require a hashcode calculation in order to determine whether the context needs to be changed. For example, if Cs and Ct are the context values in the session and thread, respectively, As and At are the Authentication values and Ai and Ci are the initial values when the current thread started to handle the request, then

Cs == Ct => No change needed, the current security context is already in the session
Cs != Ct => Either the current thread has changed the context or another one already did (the SEC-37 issue). In this case, if we check against the initial values we can determine which is the case:

Ct == Ci && At == Ai => Current thread has not authenticated (No change needed)
Ct != Ci || At != Ai => Current thread has authenticated and the context should be saved

This issue initially arose as a result of AnonymousAuthenticationTokens being stored in the session and overwriting an authentication which occurred in another thread. This should not now happen, as anonymous tokens are not saved to the session these days. If there is a situation where an application has an existing authenticated user and wishes to change the authentication (permanently), then it should generally establish the new authenticated session prior to sending multiple concurrent requests. Even if it chooses not to dot this, it seems that comparing references as described above should be sufficient to determine whether the context should be saved, without resorting to comparing hashcodes. This would also prevent the possibility of hashcode collisions (possibly due to bad implementations) preventing a new authentication from being saved.

Luke Taylor said:

For reference, the original SEC-37 thread was http://forum.springsource.org/showthread.php?t=16458

Luke Taylor said:

Made the changes.

Also removed the deprecated HttpSessionContextIntegrationFilter.

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

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