From 31338a7bdb87014844c87943113c7560810bc792 Mon Sep 17 00:00:00 2001
From: Rob Winch <rwinch@vmware.com>
Date: Tue, 26 Jun 2012 16:25:53 -0500
Subject: [PATCH] SEC-1875: ConcurrentSessionControlStrategy no longer
 adds/removes the session to the SessionRegistry twice

This fixes two issues introduced by SEC-1229

 * 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.
---
 .../ConcurrentSessionControlStrategy.java     |  7 ---
 ...ConcurrentSessionControlStrategyTests.java | 63 +++++++++++++++++++
 2 files changed, 63 insertions(+), 7 deletions(-)
 create mode 100644 web/src/test/java/org/springframework/security/web/authentication/session/ConcurrentSessionControlStrategyTests.java

diff --git a/web/src/main/java/org/springframework/security/web/authentication/session/ConcurrentSessionControlStrategy.java b/web/src/main/java/org/springframework/security/web/authentication/session/ConcurrentSessionControlStrategy.java
index b768e22f26..a893cf5b5e 100644
--- a/web/src/main/java/org/springframework/security/web/authentication/session/ConcurrentSessionControlStrategy.java
+++ b/web/src/main/java/org/springframework/security/web/authentication/session/ConcurrentSessionControlStrategy.java
@@ -141,13 +141,6 @@ public class ConcurrentSessionControlStrategy extends SessionFixationProtectionS
         leastRecentlyUsed.expireNow();
     }
 
-    @Override
-    protected void onSessionChange(String originalSessionId, HttpSession newSession, Authentication auth) {
-        // Update the session registry
-        sessionRegistry.removeSessionInformation(originalSessionId);
-        sessionRegistry.registerNewSession(newSession.getId(), auth.getPrincipal());
-    }
-
     /**
      * Sets the <tt>exceptionIfMaximumExceeded</tt> property, which determines whether the user should be prevented
      * from opening more sessions than allowed. If set to <tt>true</tt>, a <tt>SessionAuthenticationException</tt>
diff --git a/web/src/test/java/org/springframework/security/web/authentication/session/ConcurrentSessionControlStrategyTests.java b/web/src/test/java/org/springframework/security/web/authentication/session/ConcurrentSessionControlStrategyTests.java
new file mode 100644
index 0000000000..c997162fdb
--- /dev/null
+++ b/web/src/test/java/org/springframework/security/web/authentication/session/ConcurrentSessionControlStrategyTests.java
@@ -0,0 +1,63 @@
+package org.springframework.security.web.authentication.session;
+
+import static org.mockito.AdditionalMatchers.not;
+import static org.mockito.Matchers.anyObject;
+import static org.mockito.Matchers.anyString;
+import static org.mockito.Matchers.eq;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.runners.MockitoJUnitRunner;
+import org.springframework.mock.web.MockHttpServletRequest;
+import org.springframework.mock.web.MockHttpServletResponse;
+import org.springframework.security.core.Authentication;
+import org.springframework.security.core.session.SessionRegistry;
+
+/**
+ *
+ * @author Rob Winch
+ *
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class ConcurrentSessionControlStrategyTests {
+    @Mock
+    private SessionRegistry sessionRegistry;
+    @Mock
+    private Authentication authentication;
+
+    private MockHttpServletRequest request;
+    private MockHttpServletResponse response;
+
+    private ConcurrentSessionControlStrategy strategy;
+
+    @Before
+    public void setup() throws Exception {
+        request = new MockHttpServletRequest();
+        response = new MockHttpServletResponse();
+
+        strategy = new ConcurrentSessionControlStrategy(sessionRegistry);
+    }
+
+    @Test
+    public void onAuthenticationNewSession() {
+        strategy.onAuthentication(authentication, request, response);
+
+        verify(sessionRegistry,times(0)).removeSessionInformation(anyString());
+        verify(sessionRegistry).registerNewSession(anyString(), anyObject());
+    }
+
+    // SEC-1875
+    @Test
+    public void onAuthenticationChangeSession() {
+        String originalSessionId = request.getSession().getId();
+
+        strategy.onAuthentication(authentication, request, response);
+
+        verify(sessionRegistry,times(0)).removeSessionInformation(anyString());
+        verify(sessionRegistry).registerNewSession(not(eq(originalSessionId)), anyObject());
+    }
+}