Spring Security
  1. Spring Security
  2. SEC-1229

Redesign Concurrent Session Control implementation

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.5, 3.0.0 M2
    • Fix Version/s: 3.0.0 RC1
    • Component/s: Core, Namespace, Web
    • Labels:
      None

      Description

      There are several problems with the concurrent session control code, mostly stemming from the fact that it is largely to do with Http sessions (although in theory the session could be something else) but the checks are applied through the AuthenticationManager and not in the web layer. This requires that a session already be created eagerly, wasting resources, before the ConcurrentSessionController is invoked. The creation of a separate internal AuthenticationManager for use by the <http> namespace block is also incompatible with the current approach (SEC-1227).

      Ideally the sequence should be:

      1. the AuthenticationManager is called first to authenticate the request.
      2. If successful, the ConcurrentSessionController is invoked to determine whether the authentication will exceed the allowed sessions
      3. A new session is created to store the authentication and provide an Id to add to the SessionRegistry
      4. The ConcurrentSessionController is invoked again to register the authentication with the SessionRegistry

      This creates some issues:

      1. The control cannot be enacted from a single point (the AuthenticationManager). It could be integrated with the existing SessionManagementFilter and the AuthenticatedSessionStrategy interface.
      2. Event generation will not be as simple as before. the AuthenticationManager will generate a success event and subsequently, the AuthenticatedSessionStrategy (with the concurrent checks built in) will reject the authentication.

        Issue Links

          Activity

          Hide
          Luke Taylor added a comment - - edited

          Some more thoughts on an alternative implementation:

          A ConcurrentSessionControlAuthenticatedSessionStrategy could extend the DefaultAuthenticatedSessionStrategy. This would be injected into AbstractAuthenticatonProcessingFilter instances and the SessionManagementFilter - basically the only two locations where session-fixation protection is implemented. It would implement the call to the ConcurrentSessionController to check the authentication. It shouldn't even be necessary to create a session Id first - just assume that one will be created, if it doesn't already exist. ConcurrentSessionControllerImpl checks to see if the current request is already attached to a registered session, but we don't need to do that if the session Id is null, since the current request has no session. So change the checkAuthenticationAllowed method to take an Id argument and to only perform the registry check if the Id is not null.

          In fact, it would probably be better if the ConcurrentSessionController was dropped altogether. The checkAuthenticationAllowed behaviour can be added to the ConcurrentSessionControlAuthenticatedSessionStrategy and the existing registerSuccessfulAuthentication method is just a delegated call to the SessionRegistry. Configuration would be simpler without having the extra bean to inject. The addition of the session-controller-ref attribute in the namespace should also be rolled back.

          Show
          Luke Taylor added a comment - - edited Some more thoughts on an alternative implementation: A ConcurrentSessionControlAuthenticatedSessionStrategy could extend the DefaultAuthenticatedSessionStrategy. This would be injected into AbstractAuthenticatonProcessingFilter instances and the SessionManagementFilter - basically the only two locations where session-fixation protection is implemented. It would implement the call to the ConcurrentSessionController to check the authentication. It shouldn't even be necessary to create a session Id first - just assume that one will be created, if it doesn't already exist. ConcurrentSessionControllerImpl checks to see if the current request is already attached to a registered session, but we don't need to do that if the session Id is null, since the current request has no session. So change the checkAuthenticationAllowed method to take an Id argument and to only perform the registry check if the Id is not null. In fact, it would probably be better if the ConcurrentSessionController was dropped altogether. The checkAuthenticationAllowed behaviour can be added to the ConcurrentSessionControlAuthenticatedSessionStrategy and the existing registerSuccessfulAuthentication method is just a delegated call to the SessionRegistry. Configuration would be simpler without having the extra bean to inject. The addition of the session-controller-ref attribute in the namespace should also be rolled back.
          Hide
          Luke Taylor added a comment -

          Concurrent session control is now handled through a specialized AuthenticatedSessionStrategy implementation "ConcurrentSessionControlAuthenticatedSessionStrategy", injected into the AbstractAuthenticationProcessingFilter and the SessionManagementFilter (the latter handles non-interactive authentication situations). The legacy classes and namespace syntax have been removed.

          The namepspace config is now enabled through the new "session-management" element (which also now handles session-fixation-protection and other properties related to the SessionManagementFilter). The child element <concurrency-control> enables concurrency checking and has the attributes max-sessions, error-if-max-exceeded (NB "error", not "exception" as it was in the old element) and error-url. The latter allows a URL to be supplied to which the user will be sent if they exceed their allocated sessions (and error-if-max-exceeded is 'true').

          Show
          Luke Taylor added a comment - Concurrent session control is now handled through a specialized AuthenticatedSessionStrategy implementation "ConcurrentSessionControlAuthenticatedSessionStrategy", injected into the AbstractAuthenticationProcessingFilter and the SessionManagementFilter (the latter handles non-interactive authentication situations). The legacy classes and namespace syntax have been removed. The namepspace config is now enabled through the new "session-management" element (which also now handles session-fixation-protection and other properties related to the SessionManagementFilter). The child element <concurrency-control> enables concurrency checking and has the attributes max-sessions, error-if-max-exceeded (NB "error", not "exception" as it was in the old element) and error-url. The latter allows a URL to be supplied to which the user will be sent if they exceed their allocated sessions (and error-if-max-exceeded is 'true').
          Hide
          Luke Taylor added a comment -

          Docs have been updated to match the code changes.

          Show
          Luke Taylor added a comment - Docs have been updated to match the code changes.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: