Răsfoiți Sursa

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.
Rob Winch 13 ani în urmă
părinte
comite
de3dfb5b3f

+ 0 - 7
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>

+ 63 - 0
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());
+    }
+}