diff --git a/core/src/main/java/org/springframework/security/ui/FilterChainOrder.java b/core/src/main/java/org/springframework/security/ui/FilterChainOrder.java index 46b3a521be..960a56d724 100644 --- a/core/src/main/java/org/springframework/security/ui/FilterChainOrder.java +++ b/core/src/main/java/org/springframework/security/ui/FilterChainOrder.java @@ -22,8 +22,7 @@ public abstract class FilterChainOrder { public static final int CHANNEL_FILTER = FILTER_CHAIN_FIRST; public static final int CONCURRENT_SESSION_FILTER = FILTER_CHAIN_FIRST + INTERVAL * i++; - public static final int HTTP_SESSION_CONTEXT_FILTER = FILTER_CHAIN_FIRST + INTERVAL * i++; - public static final int SESSION_FIXATION_FILTER = FILTER_CHAIN_FIRST + INTERVAL * i++; + public static final int HTTP_SESSION_CONTEXT_FILTER = FILTER_CHAIN_FIRST + INTERVAL * i++; public static final int LOGOUT_FILTER = FILTER_CHAIN_FIRST + INTERVAL * i++; public static final int X509_FILTER = FILTER_CHAIN_FIRST + INTERVAL * i++; public static final int PRE_AUTH_FILTER = FILTER_CHAIN_FIRST + INTERVAL * i++; @@ -37,6 +36,7 @@ public abstract class FilterChainOrder { public static final int ANONYMOUS_FILTER = FILTER_CHAIN_FIRST + INTERVAL * i++; public static final int EXCEPTION_TRANSLATION_FILTER = FILTER_CHAIN_FIRST + INTERVAL * i++; public static final int NTLM_FILTER = FILTER_CHAIN_FIRST + INTERVAL * i++; + public static final int SESSION_FIXATION_FILTER = FILTER_CHAIN_FIRST + INTERVAL * i++; public static final int FILTER_SECURITY_INTERCEPTOR = FILTER_CHAIN_FIRST + INTERVAL * i++; public static final int SWITCH_USER_FILTER = FILTER_CHAIN_FIRST + INTERVAL * i++; @@ -46,7 +46,6 @@ public abstract class FilterChainOrder { filterNameToOrder.put("FIRST", new Integer(Integer.MIN_VALUE)); filterNameToOrder.put("CHANNEL_FILTER", new Integer(CHANNEL_FILTER)); filterNameToOrder.put("CONCURRENT_SESSION_FILTER", new Integer(CONCURRENT_SESSION_FILTER)); - filterNameToOrder.put("SESSION_CONTEXT_INTEGRATION_FILTER", new Integer(HTTP_SESSION_CONTEXT_FILTER)); filterNameToOrder.put("LOGOUT_FILTER", new Integer(LOGOUT_FILTER)); filterNameToOrder.put("X509_FILTER", new Integer(X509_FILTER)); filterNameToOrder.put("PRE_AUTH_FILTER", new Integer(PRE_AUTH_FILTER)); @@ -59,6 +58,7 @@ public abstract class FilterChainOrder { filterNameToOrder.put("ANONYMOUS_FILTER", new Integer(ANONYMOUS_FILTER)); filterNameToOrder.put("EXCEPTION_TRANSLATION_FILTER", new Integer(EXCEPTION_TRANSLATION_FILTER)); filterNameToOrder.put("NTLM_FILTER", new Integer(NTLM_FILTER)); + filterNameToOrder.put("SESSION_CONTEXT_INTEGRATION_FILTER", new Integer(HTTP_SESSION_CONTEXT_FILTER)); filterNameToOrder.put("FILTER_SECURITY_INTERCEPTOR", new Integer(FILTER_SECURITY_INTERCEPTOR)); filterNameToOrder.put("SWITCH_USER_FILTER", new Integer(SWITCH_USER_FILTER)); filterNameToOrder.put("LAST", new Integer(Integer.MAX_VALUE)); diff --git a/core/src/main/java/org/springframework/security/ui/SessionFixationProtectionFilter.java b/core/src/main/java/org/springframework/security/ui/SessionFixationProtectionFilter.java index 90e1e05f9c..792e6cc33c 100644 --- a/core/src/main/java/org/springframework/security/ui/SessionFixationProtectionFilter.java +++ b/core/src/main/java/org/springframework/security/ui/SessionFixationProtectionFilter.java @@ -6,24 +6,23 @@ import javax.servlet.FilterChain; import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; -import javax.servlet.http.HttpServletResponseWrapper; +import javax.servlet.http.HttpSession; import org.springframework.security.Authentication; import org.springframework.security.AuthenticationTrustResolver; import org.springframework.security.AuthenticationTrustResolverImpl; import org.springframework.security.concurrent.SessionRegistry; +import org.springframework.security.context.HttpSessionContextIntegrationFilter; +import org.springframework.security.context.SecurityContext; import org.springframework.security.context.SecurityContextHolder; import org.springframework.security.util.SessionUtils; /** * Detects that a user has been authenticated since the start of the request and starts a new session. *

- * This is essentially a generalization of the functionality that was implemented for SEC-399. Additionally, it will - * update the configured SessionRegistry if one is in use, thus preventing problems when used with Spring Security's - * concurrent session control. - *

- * If the response has already been committed when the filter checks the authentication state, then it isn't possible - * to create a new session and the filter will print a warning to that effect. + * This is essentially a generalization of the functionality that was implemented for SEC-399. + * Additionally, it will update the configured SessionRegistry if one is in use, thus preventing problems when used + * with Spring Security's concurrent session control. * * @author Martin Algesten * @author Luke Taylor @@ -55,22 +54,17 @@ public class SessionFixationProtectionFilter extends SpringSecurityFilter { } request.setAttribute(FILTER_APPLIED, Boolean.TRUE); - - if (isAuthenticated()) { - // We don't have to worry about session fixation attack if already authenticated - chain.doFilter(request, response); - return; - } + + HttpSession session = request.getSession(); + SecurityContext sessionSecurityContext = + (SecurityContext) session.getAttribute(HttpSessionContextIntegrationFilter.SPRING_SECURITY_CONTEXT_KEY); - SessionFixationProtectionResponseWrapper wrapper = - new SessionFixationProtectionResponseWrapper(response, request); - try { - chain.doFilter(request, wrapper); - } finally { - if (!wrapper.isNewSessionStarted()) { - startNewSessionIfRequired(request, response); - } + if (sessionSecurityContext == null && isAuthenticated()) { + // The user has been authenticated during the current request, so do the session migration + startNewSessionIfRequired(request, response); } + + chain.doFilter(request, response); } private boolean isAuthenticated() { @@ -92,83 +86,12 @@ public class SessionFixationProtectionFilter extends SpringSecurityFilter { } /** - * Called when the an initially unauthenticated request completes or a redirect or sendError occurs. + * Called when the a user wasn't authenticated at the start of the request but has been during it *

- * If the user is now authenticated, a new session will be created, the session attributes copied to it (if - * migrateSessionAttributes is set and the sessionRegistry updated with the new session information. + * A new session will be created, the session attributes copied to it (if + * migrateSessionAttributes is set) and the sessionRegistry updated with the new session information. */ - protected void startNewSessionIfRequired(HttpServletRequest request, HttpServletResponse response) { - if (isAuthenticated()) { - if (request.getSession(false) != null && response.isCommitted()) { - logger.warn("Response is already committed. Unable to create new session."); - } - - SessionUtils.startNewSessionIfRequired(request, migrateSessionAttributes, sessionRegistry); - } - } - - /** - * Response wrapper to handle the situation where we need to migrate the session after a redirect or sendError. - * Similar in function to Martin Algesten's OnRedirectUpdateSessionResponseWrapper used in - * HttpSessionContextIntegrationFilter. - *

- * Only used to wrap the response if the conditions are right at the start of the request to potentially - * require starting a new session, i.e. that the user isn't authenticated and a session existed to begin with. - */ - class SessionFixationProtectionResponseWrapper extends HttpServletResponseWrapper { - private HttpServletRequest request; - private boolean newSessionStarted; - - SessionFixationProtectionResponseWrapper(HttpServletResponse response, HttpServletRequest request) { - super(response); - this.request = request; - } - - /** - * Makes sure a new session is created before calling the - * superclass sendError() - */ - public void sendError(int sc) throws IOException { - startNewSession(); - super.sendError(sc); - } - - /** - * Makes sure a new session is created before calling the - * superclass sendError() - */ - public void sendError(int sc, String msg) throws IOException { - startNewSession(); - super.sendError(sc, msg); - } - - /** - * Makes sure a new session is created before calling the - * superclass sendRedirect() - */ - public void sendRedirect(String location) throws IOException { - startNewSession(); - super.sendRedirect(location); - } - - public void flushBuffer() throws IOException { - startNewSession(); - super.flushBuffer(); - } - - /** - * Calls startNewSessionIfRequired() - */ - private void startNewSession() { - if (newSessionStarted) { - return; - } - startNewSessionIfRequired(request, this); - newSessionStarted = true; - } - - boolean isNewSessionStarted() { - return newSessionStarted; - } + protected void startNewSessionIfRequired(HttpServletRequest request, HttpServletResponse response) { + SessionUtils.startNewSessionIfRequired(request, migrateSessionAttributes, sessionRegistry); } } diff --git a/core/src/test/java/org/springframework/security/config/HttpSecurityBeanDefinitionParserTests.java b/core/src/test/java/org/springframework/security/config/HttpSecurityBeanDefinitionParserTests.java index 6f726837f1..c92627e4b5 100644 --- a/core/src/test/java/org/springframework/security/config/HttpSecurityBeanDefinitionParserTests.java +++ b/core/src/test/java/org/springframework/security/config/HttpSecurityBeanDefinitionParserTests.java @@ -96,8 +96,7 @@ public class HttpSecurityBeanDefinitionParserTests { Iterator filters = filterList.iterator(); - assertTrue(filters.next() instanceof HttpSessionContextIntegrationFilter); - assertTrue(filters.next() instanceof SessionFixationProtectionFilter); + assertTrue(filters.next() instanceof HttpSessionContextIntegrationFilter); assertTrue(filters.next() instanceof LogoutFilter); Object authProcFilter = filters.next(); assertTrue(authProcFilter instanceof AuthenticationProcessingFilter); @@ -112,6 +111,7 @@ public class HttpSecurityBeanDefinitionParserTests { assertTrue(filters.next() instanceof RememberMeProcessingFilter); assertTrue(filters.next() instanceof AnonymousProcessingFilter); assertTrue(filters.next() instanceof ExceptionTranslationFilter); + assertTrue(filters.next() instanceof SessionFixationProtectionFilter); Object fsiObj = filters.next(); assertTrue(fsiObj instanceof FilterSecurityInterceptor); FilterSecurityInterceptor fsi = (FilterSecurityInterceptor) fsiObj; @@ -173,7 +173,7 @@ public class HttpSecurityBeanDefinitionParserTests { " " + "" + AUTH_PROVIDER_XML); // These will be matched by the default pattern "/**" - AuthenticationProcessingFilter filter = (AuthenticationProcessingFilter) getFilters("/anything").get(2); + AuthenticationProcessingFilter filter = (AuthenticationProcessingFilter) getFilters("/anything").get(1); assertEquals("/default", filter.getDefaultTargetUrl()); assertEquals(Boolean.TRUE, FieldUtils.getFieldValue(filter, "alwaysUseDefaultTargetUrl")); } @@ -264,7 +264,7 @@ public class HttpSecurityBeanDefinitionParserTests { setContext("" + AUTH_PROVIDER_XML); List filters = getFilters("/someurl"); - ExceptionTranslationFilter etf = (ExceptionTranslationFilter) filters.get(filters.size() - 2); + ExceptionTranslationFilter etf = (ExceptionTranslationFilter) filters.get(filters.size() - 3); assertEquals("/access-denied", FieldUtils.getFieldValue(etf, "accessDeniedHandler.errorPage")); } @@ -324,7 +324,7 @@ public class HttpSecurityBeanDefinitionParserTests { assertEquals(14, filters.size()); assertTrue(filters.get(0) instanceof MockFilter); assertTrue(filters.get(1) instanceof SecurityContextHolderAwareRequestFilter); - assertTrue(filters.get(5) instanceof SecurityContextHolderAwareRequestFilter); + assertTrue(filters.get(4) instanceof SecurityContextHolderAwareRequestFilter); } @Test(expected=BeanCreationException.class) @@ -368,7 +368,7 @@ public class HttpSecurityBeanDefinitionParserTests { "" + AUTH_PROVIDER_XML); List filters = getFilters("/someurl"); - assertTrue(filters.get(3) instanceof X509PreAuthenticatedProcessingFilter); + assertTrue(filters.get(2) instanceof X509PreAuthenticatedProcessingFilter); } @Test @@ -422,7 +422,7 @@ public class HttpSecurityBeanDefinitionParserTests { "" + " " + "" + AUTH_PROVIDER_XML); - ExceptionTranslationFilter etf = (ExceptionTranslationFilter) getFilters("/someurl").get(9); + ExceptionTranslationFilter etf = (ExceptionTranslationFilter) getFilters("/someurl").get(8); assertTrue("ExceptionTranslationFilter should be configured with custom entry point", etf.getAuthenticationEntryPoint() instanceof MockAuthenticationEntryPoint); } diff --git a/core/src/test/java/org/springframework/security/ui/SessionFixationProtectionFilterTests.java b/core/src/test/java/org/springframework/security/ui/SessionFixationProtectionFilterTests.java index cf34f39b23..3adab7834b 100644 --- a/core/src/test/java/org/springframework/security/ui/SessionFixationProtectionFilterTests.java +++ b/core/src/test/java/org/springframework/security/ui/SessionFixationProtectionFilterTests.java @@ -2,19 +2,14 @@ package org.springframework.security.ui; import static org.junit.Assert.*; -import java.io.IOException; - -import javax.servlet.FilterChain; -import javax.servlet.ServletRequest; -import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; import org.junit.After; import org.junit.Test; import org.springframework.mock.web.MockFilterChain; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; +import org.springframework.security.context.HttpSessionContextIntegrationFilter; import org.springframework.security.context.SecurityContextHolder; import org.springframework.security.providers.TestingAuthenticationToken; @@ -34,6 +29,7 @@ public class SessionFixationProtectionFilterTests { public void newSessionShouldNotBeCreatedIfNoSessionExists() throws Exception { SessionFixationProtectionFilter filter = new SessionFixationProtectionFilter(); HttpServletRequest request = new MockHttpServletRequest(); + authenticateUser(); filter.doFilter(request, new MockHttpServletResponse(), new MockFilterChain()); @@ -41,14 +37,15 @@ public class SessionFixationProtectionFilterTests { } @Test - public void newSessionShouldNotBeCreatedIfUserIsAuthenticated() throws Exception { + public void newSessionBeCreatedIfAuthenticatedOccurredDuringRequest() throws Exception { SessionFixationProtectionFilter filter = new SessionFixationProtectionFilter(); HttpServletRequest request = new MockHttpServletRequest(); String sessionId = request.getSession().getId(); + authenticateUser(); filter.doFilter(request, new MockHttpServletResponse(), new MockFilterChain()); - assertEquals(sessionId, request.getSession().getId()); + assertFalse(sessionId.equals(request.getSession().getId())); } @Test @@ -68,91 +65,15 @@ public class SessionFixationProtectionFilterTests { HttpServletRequest request = new MockHttpServletRequest(); String sessionId = request.getSession().getId(); authenticateUser(); - + request.getSession().setAttribute(HttpSessionContextIntegrationFilter.SPRING_SECURITY_CONTEXT_KEY, + SecurityContextHolder.getContext()); + filter.doFilter(request, new MockHttpServletResponse(), new MockFilterChain()); assertEquals(sessionId, request.getSession().getId()); } - - @Test - public void newSessionShouldBeCreatedIfAuthenticationOccursDuringRequest() throws Exception { - SessionFixationProtectionFilter filter = new SessionFixationProtectionFilter(); - HttpServletRequest request = new MockHttpServletRequest(); - String sessionId = request.getSession().getId(); - - filter.doFilter(request, new MockHttpServletResponse(), new UserAuthenticatingFilterChain()); - - assertFalse(sessionId.equals(request.getSession().getId())); - } - - @Test - public void newSessionShouldBeCreatedIfAuthenticationAndRedirectOccursDuringRequest() throws Exception { - SessionFixationProtectionFilter filter = new SessionFixationProtectionFilter(); - HttpServletRequest request = new MockHttpServletRequest(); - String sessionId = request.getSession().getId(); - - AuthenticateAndRedirectFilterChain chain = new AuthenticateAndRedirectFilterChain(); - filter.doFilter(request, new MockHttpServletResponse(), chain); - - assertTrue(chain.getResponse() instanceof - SessionFixationProtectionFilter.SessionFixationProtectionResponseWrapper); - assertTrue("New session should have been created by session wrapper", - ((SessionFixationProtectionFilter.SessionFixationProtectionResponseWrapper)chain.getResponse()).isNewSessionStarted()); - assertFalse(sessionId.equals(request.getSession().getId())); - } - - @Test - public void wrapperSendErrorCreatesNewSession() throws Exception { - authenticateUser(); - SessionFixationProtectionFilter filter = new SessionFixationProtectionFilter(); - HttpServletRequest request = new MockHttpServletRequest(); - String sessionId = request.getSession().getId(); - SessionFixationProtectionFilter.SessionFixationProtectionResponseWrapper wrapper = - filter.new SessionFixationProtectionResponseWrapper(new MockHttpServletResponse(), request); - wrapper.sendError(HttpServletResponse.SC_FORBIDDEN); - assertFalse(sessionId.equals(request.getSession().getId())); - - // Message version - request = new MockHttpServletRequest(); - sessionId = request.getSession().getId(); - wrapper = filter.new SessionFixationProtectionResponseWrapper(new MockHttpServletResponse(), request); - wrapper.sendError(HttpServletResponse.SC_FORBIDDEN, "Hi. I'm your friendly forbidden message."); - assertFalse(sessionId.equals(request.getSession().getId())); - } - @Test - public void wrapperRedirectCreatesNewSession() throws Exception { - authenticateUser(); - SessionFixationProtectionFilter filter = new SessionFixationProtectionFilter(); - HttpServletRequest request = new MockHttpServletRequest(); - String sessionId = request.getSession().getId(); - SessionFixationProtectionFilter.SessionFixationProtectionResponseWrapper wrapper = - filter.new SessionFixationProtectionResponseWrapper(new MockHttpServletResponse(), request); - wrapper.sendRedirect("/somelocation"); - assertFalse(sessionId.equals(request.getSession().getId())); - } - private void authenticateUser() { SecurityContextHolder.getContext().setAuthentication(new TestingAuthenticationToken("user", "pass", null)); } - - private class UserAuthenticatingFilterChain implements FilterChain { - public void doFilter(ServletRequest request, ServletResponse response) throws IOException { - authenticateUser(); - } - } - - private class AuthenticateAndRedirectFilterChain extends UserAuthenticatingFilterChain{ - HttpServletResponse response; - - public void doFilter(ServletRequest request, ServletResponse response) throws IOException { - super.doFilter(request, response); - this.response = (HttpServletResponse)response; - this.response.sendRedirect("/someUrl"); - } - - public HttpServletResponse getResponse() { - return response; - } - } }