Spring Security
  1. Spring Security
  2. SEC-1875

SessionRegistry.registerNewSession invoked twice after successful authentication

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.7, 3.1.0
    • Fix Version/s: 3.0.8, 3.1.1
    • Component/s: Web
    • Labels:
      None
    • Environment:
      Spring Framework 3.1.0
      Spring Security 3.1.0

      Description

      In Spring Security 3.1.0, SessionRegistry.registerNewSession is now invoked twice, due to SessionFixationProtectionStrategy.onAuthentication calling onSessionChange (this call wasn't there in previous versions up to 3.0.7).

      I'm using a custom SessionRegistry implementation which uses a database table as the session registry, thus facing a constraint error if the same session is registered twice.

      Perhaps ConcurrentSessionControlStrategy.onAuthentication need not invoke sessionRegistry.registerNewSession now?

        Issue Links

          Activity

          Hide
          Grzegorz Rozniecki added a comment - - edited

          Temporary fix is to override onSessionChange like this:

          public class FixedConcurrentSessionControlStrategy extends ConcurrentSessionControlStrategy {
          
              public FixedConcurrentSessionControlStrategy( final SessionRegistry sessionRegistry ) {
                  super( sessionRegistry );
              }
          
              // Until https://jira.springsource.org/browse/SEC-1875 is resolved...
              @Override
              protected void onSessionChange(final String originalSessionId, final HttpSession newSession, final Authentication auth) {
              }
          }
          

          but I hope it'll be fixed in 3.1.1.

          Show
          Grzegorz Rozniecki added a comment - - edited Temporary fix is to override onSessionChange like this: public class FixedConcurrentSessionControlStrategy extends ConcurrentSessionControlStrategy { public FixedConcurrentSessionControlStrategy( final SessionRegistry sessionRegistry ) { super ( sessionRegistry ); } // Until https://jira.springsource.org/browse/SEC-1875 is resolved... @Override protected void onSessionChange( final String originalSessionId, final HttpSession newSession, final Authentication auth) { } } but I hope it'll be fixed in 3.1.1.
          Hide
          Rob Winch added a comment -

          The changes for SEC-1229 introduced two issues

          • SessionRegistry.registerNewSession is invoked twice
          • SessionRegistry.removeSession is invoked twice (once by the
            ConcurrentSessionControlStrategy#onSessionChange and once by
            SessionRegistryImpl#onApplicationEvent). This is not nearly
            as problematic since the interface states that implementations
            should be handle removing the session twice. However, as removing
            twice requires an unnecessary database hit we should only remove
            sessions once.

          The problem with the initially proposed solution is that if the session was new, it would not be inserted into the SessionRegistry. It also did not address the second issue. The solution to both of these issues was to remove the onSessionChange method from ConcurrentSessionControlStrategy and allow the the super class to process onSessionChange with a do nothing implementation to keep passivity (very similar to Grzegorz's solution).

          Show
          Rob Winch added a comment - The changes for SEC-1229 introduced two issues SessionRegistry.registerNewSession is invoked twice SessionRegistry.removeSession is invoked twice (once by the ConcurrentSessionControlStrategy#onSessionChange and once by SessionRegistryImpl#onApplicationEvent). This is not nearly as problematic since the interface states that implementations should be handle removing the session twice. However, as removing twice requires an unnecessary database hit we should only remove sessions once. The problem with the initially proposed solution is that if the session was new, it would not be inserted into the SessionRegistry. It also did not address the second issue. The solution to both of these issues was to remove the onSessionChange method from ConcurrentSessionControlStrategy and allow the the super class to process onSessionChange with a do nothing implementation to keep passivity (very similar to Grzegorz's solution).

            People

            • Assignee:
              Rob Winch
              Reporter:
              Alvin Chee
            • Votes:
              2 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 1d
                1d
                Remaining:
                Remaining Estimate - 1d
                1d
                Logged:
                Time Spent - Not Specified
                Not Specified