Spring Security
  1. Spring Security
  2. SEC-1307

Investigate optimization of logic in HttpSessionSecurityContextRepository which checks for changes in the SecurityContext

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.1.0.M1
    • Component/s: Web
    • Labels:
      None

      Description

      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.

        Issue Links

          Activity

          Hide
          Luke Taylor added a comment -

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

          Show
          Luke Taylor added a comment - For reference, the original SEC-37 thread was http://forum.springsource.org/showthread.php?t=16458
          Hide
          Luke Taylor added a comment -

          Made the changes.

          Also removed the deprecated HttpSessionContextIntegrationFilter.

          Show
          Luke Taylor added a comment - Made the changes. Also removed the deprecated HttpSessionContextIntegrationFilter.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: