From b2799985f2f8eaaef1a80a03d571286a26cb12fd Mon Sep 17 00:00:00 2001 From: Luke Taylor Date: Fri, 31 Aug 2007 20:39:33 +0000 Subject: [PATCH] SEC-398: Added patch which uses response wrapper to set context in session on redirect or error. --- .../HttpSessionContextIntegrationFilter.java | 146 +++++++++++++++--- ...pSessionContextIntegrationFilterTests.java | 85 +++------- 2 files changed, 148 insertions(+), 83 deletions(-) diff --git a/core/src/main/java/org/acegisecurity/context/HttpSessionContextIntegrationFilter.java b/core/src/main/java/org/acegisecurity/context/HttpSessionContextIntegrationFilter.java index 8f8fba9375..b1594e3260 100644 --- a/core/src/main/java/org/acegisecurity/context/HttpSessionContextIntegrationFilter.java +++ b/core/src/main/java/org/acegisecurity/context/HttpSessionContextIntegrationFilter.java @@ -25,6 +25,8 @@ import javax.servlet.ServletException; import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import javax.servlet.http.HttpServletResponseWrapper; import javax.servlet.http.HttpSession; import org.apache.commons.logging.Log; @@ -50,8 +52,7 @@ import org.springframework.util.ReflectionUtils; * HttpSession for whatever reason, a fresh * SecurityContext will be created and used instead. The created * object will be of the instance defined by the {@link #setContext(Class)} - * method (which defaults to {@link - * org.acegisecurity.context.SecurityContextImpl}. + * method (which defaults to {@link org.acegisecurity.context.SecurityContextImpl}. *

*

* No HttpSession will be created by this filter if one does not @@ -93,6 +94,9 @@ import org.springframework.util.ReflectionUtils; * * @author Ben Alex * @author Patrick Burleson + * @author Luke Taylor + * @author Martin Algesten + * * @version $Id$ */ public class HttpSessionContextIntegrationFilter implements InitializingBean, Filter { @@ -180,12 +184,6 @@ public class HttpSessionContextIntegrationFilter implements InitializingBean, Fi } } - /** - * Does nothing. We use IoC container lifecycle services instead. - */ - public void destroy() { - } - public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { if (request.getAttribute(FILTER_APPLIED) != null) { @@ -227,13 +225,21 @@ public class HttpSessionContextIntegrationFilter implements InitializingBean, Fi int contextHashBeforeChainExecution = contextBeforeChainExecution.hashCode(); request.setAttribute(FILTER_APPLIED, Boolean.TRUE); + // Create a wrapper that will eagerly update the session with the security context + // if anything in the chain does a sendError() or sendRedirect(). + // See SEC-398 + + OnRedirectUpdateSessionResponseWrapper responseWrapper = + new OnRedirectUpdateSessionResponseWrapper( (HttpServletResponse)response, request, + httpSessionExistedAtStartOfRequest, contextHashBeforeChainExecution ); + // Proceed with chain try { // This is the only place in this class where SecurityContextHolder.setContext() is called SecurityContextHolder.setContext(contextBeforeChainExecution); - chain.doFilter(request, response); + chain.doFilter(request, responseWrapper); } finally { // This is the only place in this class where SecurityContextHolder.getContext() is called @@ -244,8 +250,13 @@ public class HttpSessionContextIntegrationFilter implements InitializingBean, Fi request.removeAttribute(FILTER_APPLIED); - storeSecurityContextInSession(contextAfterChainExecution, request, - httpSessionExistedAtStartOfRequest, contextHashBeforeChainExecution); + // storeSecurityContextInSession() might already be called by the response wrapper + // if something in the chain called sendError() or sendRedirect(). This ensures we only call it + // once per request. + if ( !responseWrapper.isSessionUpdateDone() ) { + storeSecurityContextInSession(contextAfterChainExecution, request, + httpSessionExistedAtStartOfRequest, contextHashBeforeChainExecution); + } if (logger.isDebugEnabled()) { logger.debug("SecurityContextHolder now cleared, as request processing completed"); @@ -331,7 +342,7 @@ public class HttpSessionContextIntegrationFilter implements InitializingBean, Fi * @param httpSessionExistedAtStartOfRequest indicates whether there was a session in place before the * filter chain executed. If this is true, and the session is found to be null, this indicates that it was * invalidated during the request and a new session will now be created. - * @param contextHashWhenChainProceeded the hashcode of the context before the filter chain executed. + * @param contextHashBeforeChainExecution the hashcode of the context before the filter chain executed. * The context will only be stored if it has a different hashcode, indicating that the context changed * during the request. * @@ -339,7 +350,7 @@ public class HttpSessionContextIntegrationFilter implements InitializingBean, Fi private void storeSecurityContextInSession(SecurityContext securityContext, ServletRequest request, boolean httpSessionExistedAtStartOfRequest, - int contextHashWhenChainProceeded) { + int contextHashBeforeChainExecution) { HttpSession httpSession = null; try { @@ -386,7 +397,7 @@ public class HttpSessionContextIntegrationFilter implements InitializingBean, Fi // If HttpSession exists, store current SecurityContextHolder contents but only if // the SecurityContext has actually changed (see JIRA SEC-37) - if (httpSession != null && securityContext.hashCode() != contextHashWhenChainProceeded) { + if (httpSession != null && securityContext.hashCode() != contextHashBeforeChainExecution) { httpSession.setAttribute(ACEGI_SECURITY_CONTEXT_KEY, securityContext); if (logger.isDebugEnabled()) { @@ -407,10 +418,6 @@ public class HttpSessionContextIntegrationFilter implements InitializingBean, Fi } } - public Class getContext() { - return context; - } - /** * Does nothing. We use IoC container lifecycle services instead. * @@ -420,23 +427,118 @@ public class HttpSessionContextIntegrationFilter implements InitializingBean, Fi public void init(FilterConfig filterConfig) throws ServletException { } - public boolean isAllowSessionCreation() { - return allowSessionCreation; + /** + * Does nothing. We use IoC container lifecycle services instead. + */ + public void destroy() { } - public boolean isForceEagerSessionCreation() { - return forceEagerSessionCreation; + public boolean isAllowSessionCreation() { + return allowSessionCreation; } public void setAllowSessionCreation(boolean allowSessionCreation) { this.allowSessionCreation = allowSessionCreation; } + public Class getContext() { + return context; + } + public void setContext(Class secureContext) { this.context = secureContext; } + public boolean isForceEagerSessionCreation() { + return forceEagerSessionCreation; + } + public void setForceEagerSessionCreation(boolean forceEagerSessionCreation) { this.forceEagerSessionCreation = forceEagerSessionCreation; } + + + //~ Inner Classes ================================================================================================== + + /** + * Wrapper that is applied to every request to update the HttpSession with + * the SecurityContext when a sendError() or sendRedirect + * happens. See SEC-398. The class contains the fields needed to call + * storeSecurityContextInSession() + */ + private class OnRedirectUpdateSessionResponseWrapper extends HttpServletResponseWrapper { + + ServletRequest request; + boolean httpSessionExistedAtStartOfRequest; + int contextHashBeforeChainExecution; + + // Used to ensure storeSecurityContextInSession() is only + // called once. + boolean sessionUpdateDone = false; + + /** + * Takes the parameters required to call storeSecurityContextInSession() in + * addition to the response object we are wrapping. + * @see HttpSessionContextIntegrationFilter#storeSecurityContextInSession(SecurityContext, ServletRequest, boolean, int) + */ + public OnRedirectUpdateSessionResponseWrapper(HttpServletResponse response, + ServletRequest request, + boolean httpSessionExistedAtStartOfRequest, + int contextHashBeforeChainExecution) { + super(response); + this.request = request; + this.httpSessionExistedAtStartOfRequest = httpSessionExistedAtStartOfRequest; + this.contextHashBeforeChainExecution = contextHashBeforeChainExecution; + } + + /** + * Makes sure the session is updated before calling the + * superclass sendError() + */ + public void sendError(int sc) throws IOException { + doSessionUpdate(); + super.sendError(sc); + } + + /** + * Makes sure the session is updated before calling the + * superclass sendError() + */ + public void sendError(int sc, String msg) throws IOException { + doSessionUpdate(); + super.sendError(sc, msg); + } + + /** + * Makes sure the session is updated before calling the + * superclass sendRedirect() + */ + public void sendRedirect(String location) throws IOException { + doSessionUpdate(); + super.sendRedirect(location); + } + + /** + * Calls storeSecurityContextInSession() + */ + private void doSessionUpdate() { + if (sessionUpdateDone) { + return; + } + SecurityContext securityContext = SecurityContextHolder.getContext(); + storeSecurityContextInSession(securityContext, request, + httpSessionExistedAtStartOfRequest, contextHashBeforeChainExecution); + sessionUpdateDone = true; + } + + /** + * Tells if the response wrapper has called + * storeSecurityContextInSession(). + */ + public boolean isSessionUpdateDone() { + return sessionUpdateDone; + } + + } + } diff --git a/core/src/test/java/org/acegisecurity/context/HttpSessionContextIntegrationFilterTests.java b/core/src/test/java/org/acegisecurity/context/HttpSessionContextIntegrationFilterTests.java index 355842a4a8..bc9f398c6e 100644 --- a/core/src/test/java/org/acegisecurity/context/HttpSessionContextIntegrationFilterTests.java +++ b/core/src/test/java/org/acegisecurity/context/HttpSessionContextIntegrationFilterTests.java @@ -39,25 +39,22 @@ import javax.servlet.ServletResponse; /** * Tests {@link HttpSessionContextIntegrationFilter}. - * + * * @author Ben Alex * @version $Id: HttpSessionContextIntegrationFilterTests.java 1858 2007-05-24 * 02:04:47Z benalex $ */ public class HttpSessionContextIntegrationFilterTests extends TestCase { - // ~ Constructors - // =================================================================================================== + //~ Constructors =================================================================================================== public HttpSessionContextIntegrationFilterTests() { - super(); } public HttpSessionContextIntegrationFilterTests(String arg0) { super(arg0); } - // ~ Methods - // ======================================================================================================== + //~ Methods ======================================================================================================== private static void executeFilterInContainerSimulator( FilterConfig filterConfig, Filter filter, ServletRequest request, @@ -68,11 +65,6 @@ public class HttpSessionContextIntegrationFilterTests extends TestCase { filter.destroy(); } - public static void main(String[] args) { - junit.textui.TestRunner - .run(HttpSessionContextIntegrationFilterTests.class); - } - public void testDetectsIncompatibleSessionProperties() throws Exception { HttpSessionContextIntegrationFilter filter = new HttpSessionContextIntegrationFilter(); @@ -111,8 +103,7 @@ public class HttpSessionContextIntegrationFilterTests extends TestCase { } } - public void testExceptionWithinFilterChainStillClearsSecurityContextHolder() - throws Exception { + public void testExceptionWithinFilterChainStillClearsSecurityContextHolder() throws Exception { // Build an Authentication object we simulate came from HttpSession PrincipalAcegiUserToken sessionPrincipal = new PrincipalAcegiUserToken( "key", @@ -151,12 +142,9 @@ public class HttpSessionContextIntegrationFilterTests extends TestCase { // Check the SecurityContextHolder is null, even though an exception was // thrown during chain - assertEquals(new SecurityContextImpl(), SecurityContextHolder - .getContext()); - assertNull( - "Should have cleared FILTER_APPLIED", - request - .getAttribute(HttpSessionContextIntegrationFilter.FILTER_APPLIED)); + assertEquals(new SecurityContextImpl(), SecurityContextHolder.getContext()); + assertNull("Should have cleared FILTER_APPLIED", + request.getAttribute(HttpSessionContextIntegrationFilter.FILTER_APPLIED)); } public void testExistingContextContentsCopiedIntoContextHolderFromSessionAndChangesToContextCopiedBackToSession() @@ -200,18 +188,13 @@ public class HttpSessionContextIntegrationFilterTests extends TestCase { request, response, chain); // Obtain new/update Authentication from HttpSession - SecurityContext context = (SecurityContext) request - .getSession() - .getAttribute( + SecurityContext context = (SecurityContext) request.getSession().getAttribute( HttpSessionContextIntegrationFilter.ACEGI_SECURITY_CONTEXT_KEY); - assertEquals(updatedPrincipal, ((SecurityContext) context) - .getAuthentication()); + assertEquals(updatedPrincipal, ((SecurityContext) context).getAuthentication()); } - public void testHttpSessionCreatedWhenContextHolderChanges() - throws Exception { - // Build an Authentication object we simulate our Authentication changed - // it to + public void testHttpSessionCreatedWhenContextHolderChanges() throws Exception { + // Build an Authentication object we simulate our Authentication changed it to PrincipalAcegiUserToken updatedPrincipal = new PrincipalAcegiUserToken( "key", "someone", "password", new GrantedAuthority[] { new GrantedAuthorityImpl( @@ -225,21 +208,15 @@ public class HttpSessionContextIntegrationFilterTests extends TestCase { // Prepare filter HttpSessionContextIntegrationFilter filter = new HttpSessionContextIntegrationFilter(); filter.setContext(SecurityContextImpl.class); - // don't call afterPropertiesSet to test case when not instantiated by - // Spring - // filter.afterPropertiesSet(); + // don't call afterPropertiesSet to test case when Spring filter.afterPropertiesSet(); isn't called // Execute filter - executeFilterInContainerSimulator(new MockFilterConfig(), filter, - request, response, chain); + executeFilterInContainerSimulator(new MockFilterConfig(), filter, request, response, chain); // Obtain new/updated Authentication from HttpSession - SecurityContext context = (SecurityContext) request - .getSession(false) - .getAttribute( + SecurityContext context = (SecurityContext) request.getSession(false).getAttribute( HttpSessionContextIntegrationFilter.ACEGI_SECURITY_CONTEXT_KEY); - assertEquals(updatedPrincipal, ((SecurityContext) context) - .getAuthentication()); + assertEquals(updatedPrincipal, ((SecurityContext) context).getAuthentication()); } public void testHttpSessionEagerlyCreatedWhenDirected() throws Exception { @@ -262,8 +239,7 @@ public class HttpSessionContextIntegrationFilterTests extends TestCase { assertNotNull(request.getSession(false)); } - public void testHttpSessionNotCreatedUnlessContextHolderChanges() - throws Exception { + public void testHttpSessionNotCreatedUnlessContextHolderChanges() throws Exception { // Build a mock request MockHttpServletRequest request = new MockHttpServletRequest(null, null); MockHttpServletResponse response = new MockHttpServletResponse(); @@ -282,8 +258,7 @@ public class HttpSessionContextIntegrationFilterTests extends TestCase { assertNull(request.getSession(false)); } - public void testHttpSessionWithNonContextInWellKnownLocationIsOverwritten() - throws Exception { + public void testHttpSessionWithNonContextInWellKnownLocationIsOverwritten() throws Exception { // Build an Authentication object we simulate our Authentication changed // it to PrincipalAcegiUserToken updatedPrincipal = new PrincipalAcegiUserToken( @@ -306,20 +281,15 @@ public class HttpSessionContextIntegrationFilterTests extends TestCase { filter.afterPropertiesSet(); // Execute filter - executeFilterInContainerSimulator(new MockFilterConfig(), filter, - request, response, chain); + executeFilterInContainerSimulator(new MockFilterConfig(), filter, request, response, chain); // Obtain new/update Authentication from HttpSession - SecurityContext context = (SecurityContext) request - .getSession() - .getAttribute( + SecurityContext context = (SecurityContext) request.getSession().getAttribute( HttpSessionContextIntegrationFilter.ACEGI_SECURITY_CONTEXT_KEY); - assertEquals(updatedPrincipal, ((SecurityContext) context) - .getAuthentication()); + assertEquals(updatedPrincipal, ((SecurityContext) context).getAuthentication()); } - public void testConcurrentThreadsLazilyChangeFilterAppliedValueToTrue() - throws Exception { + public void testConcurrentThreadsLazilyChangeFilterAppliedValueToTrue() throws Exception { PrincipalAcegiUserToken sessionPrincipal = new PrincipalAcegiUserToken( "key", "someone", @@ -366,15 +336,9 @@ public class HttpSessionContextIntegrationFilterTests extends TestCase { this.toThrowDuringChain = toThrowDuringChain; } - private MockFilterChain() { - } - - public void doFilter(ServletRequest arg0, ServletResponse arg1) - throws IOException, ServletException { - + public void doFilter(ServletRequest arg0, ServletResponse arg1) throws IOException, ServletException { if (expectedOnContextHolder != null) { - assertEquals(expectedOnContextHolder, SecurityContextHolder - .getContext().getAuthentication()); + assertEquals(expectedOnContextHolder, SecurityContextHolder.getContext().getAuthentication()); } if (changeContextHolder != null) { @@ -409,8 +373,7 @@ public class HttpSessionContextIntegrationFilterTests extends TestCase { public void run() { try { // Execute filter - executeFilterInContainerSimulator(new MockFilterConfig(), - filter, request, response, chain); + executeFilterInContainerSimulator(new MockFilterConfig(), filter, request, response, chain); // Check the session is not null assertNotNull(request.getSession(false));