SEC-1077: Added DefaultAuthenticatedSessionStrategy test to check that saved request attribute is retained when migrateAttributes is false.

config/src/test/java/org/springframework/security/config/http/HttpSecurityBeanDefinitionParserTests.java

@@ -578,17 +578,6 @@ public class HttpSecurityBeanDefinitionParserTests {
         assertEquals("uid=(.*),", p.pattern());
     }
 
-    @Test
-    public void x() throws Exception {
-        setContext(
-                "<http auto-config='true'>" +
-                "    <x509 />" +
-                "</http>"  + AUTH_PROVIDER_XML);
-        List<Filter> filters = getFilters("/someurl");
-
-        assertTrue(filters.get(2) instanceof X509PreAuthenticatedProcessingFilter);
-    }
-
     @Test
     public void concurrentSessionSupportAddsFilterAndExpectedBeans() throws Exception {
         setContext(
@@ -754,8 +743,8 @@ public class HttpSecurityBeanDefinitionParserTests {
         setContext(
                 "<http auto-config='true' session-fixation-protection='none'/>" + AUTH_PROVIDER_XML);
         List<Filter> filters = getFilters("/someurl");
-
+        assertTrue(filters.get(8) instanceof ExceptionTranslationFilter);
-        assertFalse(filters.get(1) instanceof SessionFixationProtectionFilter);
+        assertFalse(filters.get(9) instanceof SessionFixationProtectionFilter);
     }
 
     /**

web/src/main/java/org/springframework/security/web/session/DefaultAuthenticatedSessionStrategy.java

@@ -14,16 +14,19 @@ import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.springframework.security.authentication.concurrent.SessionRegistry;
 import org.springframework.security.core.Authentication;
-import org.springframework.security.core.context.SecurityContextHolder;
 import org.springframework.security.web.savedrequest.SavedRequest;
 
 /**
  * The default implementation of {@link AuthenticatedSessionStrategy}.
  * <p>
- * Creates a new session for the newly authenticated user if they already have a session, and copies their
+ * Creates a new session for the newly authenticated user if they already have a session (as a defence against
+ * session-fixation protection attacks), and copies their
  * session attributes across to the new session (can be disabled by setting <tt>migrateSessionAttributes</tt> to
  * <tt>false</tt>).
  * <p>
+ * This approach will only be effective if your servlet container always assigns a new session Id when a session is
+ * invalidated and a new session created by calling {@link HttpServletRequest#getSession()}.
+ * <p>
  * If concurrent session control is in use, then a <tt>SessionRegistry</tt> must be injected.
  *
  * @author Luke Taylor
@@ -91,6 +94,11 @@ public class DefaultAuthenticatedSessionStrategy implements AuthenticatedSession
             logger.debug("Started new session: " + session.getId());
         }
 
+        if (originalSessionId.equals(session.getId())) {
+            logger.warn("Your servlet container did not change the session ID when a new session was created. You will" +
+                    " not be adequately protected against session-fixation attacks");
+        }
+
         // Copy attributes to new session
         if (attributesToMigrate != null) {
             for (Map.Entry<String, Object> entry : attributesToMigrate.entrySet()) {
@@ -101,8 +109,7 @@ public class DefaultAuthenticatedSessionStrategy implements AuthenticatedSession
         // Update the session registry
         if (sessionRegistry != null) {
             sessionRegistry.removeSessionInformation(originalSessionId);
-            sessionRegistry.registerNewSession(session.getId(),
+            sessionRegistry.registerNewSession(session.getId(), authentication.getPrincipal());
-                    SecurityContextHolder.getContext().getAuthentication().getPrincipal());
         }
     }
 
@@ -152,4 +159,8 @@ public class DefaultAuthenticatedSessionStrategy implements AuthenticatedSession
     public void setRetainedAttributes(List<String> retainedAttributes) {
         this.retainedAttributes = retainedAttributes;
     }
+
+    public void setAlwaysCreateSession(boolean alwaysCreateSession) {
+        this.alwaysCreateSession = alwaysCreateSession;
+    }
 }

web/src/test/java/org/springframework/security/web/session/DefaultAuthenticatedSessionStrategyTests.java

@@ -4,11 +4,14 @@ import static org.junit.Assert.*;
 import static org.mockito.Mockito.mock;
 
 import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpSession;
 
 import org.junit.Test;
 import org.springframework.mock.web.MockHttpServletRequest;
 import org.springframework.mock.web.MockHttpServletResponse;
+import org.springframework.security.authentication.concurrent.SessionRegistry;
 import org.springframework.security.core.Authentication;
+import org.springframework.security.web.savedrequest.SavedRequest;
 
 /**
  *
@@ -30,6 +33,7 @@ public class DefaultAuthenticatedSessionStrategyTests {
     @Test
     public void newSessionIsCreatedIfSessionAlreadyExists() throws Exception {
         DefaultAuthenticatedSessionStrategy strategy = new DefaultAuthenticatedSessionStrategy();
+        strategy.setSessionRegistry(mock(SessionRegistry.class));
         HttpServletRequest request = new MockHttpServletRequest();
         String sessionId = request.getSession().getId();
 
@@ -38,4 +42,27 @@ public class DefaultAuthenticatedSessionStrategyTests {
         assertFalse(sessionId.equals(request.getSession().getId()));
     }
 
+    // See SEC-1077
+    @Test
+    public void onlySavedRequestAttributeIsMigratedIfMigrateAttributesIsFalse() throws Exception {
+        DefaultAuthenticatedSessionStrategy strategy = new DefaultAuthenticatedSessionStrategy();
+        strategy.setMigrateSessionAttributes(false);
+        HttpServletRequest request = new MockHttpServletRequest();
+        HttpSession session = request.getSession();
+        session.setAttribute("blah", "blah");
+        session.setAttribute(SavedRequest.SPRING_SECURITY_SAVED_REQUEST_KEY, "SavedRequest");
+
+        strategy.onAuthenticationSuccess(mock(Authentication.class), request, new MockHttpServletResponse());
+
+        assertNull(request.getSession().getAttribute("blah"));
+        assertNotNull(request.getSession().getAttribute(SavedRequest.SPRING_SECURITY_SAVED_REQUEST_KEY));
+    }
+
+    @Test
+    public void sessionIsCreatedIfAlwaysCreateTrue() throws Exception {
+        DefaultAuthenticatedSessionStrategy strategy = new DefaultAuthenticatedSessionStrategy();
+        strategy.setAlwaysCreateSession(true);
+
+    }
+
 }