HttpSessionContextIntegrationFilter elegantly handles IOExceptions and ServletExceptions within filter chain (see http://opensource.atlassian.com/projects/spring/browse/SEC-20).

core/src/main/java/org/acegisecurity/context/HttpSessionContextIntegrationFilter.java

@@ -212,65 +212,74 @@ public class HttpSessionContextIntegrationFilter implements InitializingBean,
             httpSession = null;
 
             // Proceed with chain
-            chain.doFilter(request, response);
-
-            // Store context back to HttpSession
             try {
-                httpSession = ((HttpServletRequest) request).getSession(false);
-            } catch (IllegalStateException ignored) {}
-
-            if ((httpSession == null) && httpSessionExistedAtStartOfRequest) {
-                if (logger.isDebugEnabled()) {
-                    logger.debug(
-                        "HttpSession is now null, but was not null at start of request; session was invalidated, so do not create a new session");
-                }
-            }
-
-            // Generate a HttpSession only if we need to
-            if ((httpSession == null) && !httpSessionExistedAtStartOfRequest) {
-                if (!allowSessionCreation) {
+                chain.doFilter(request, response);
+            } catch (IOException ioe) {
+                throw ioe;
+            } catch (ServletException se) {
+                throw se;
+            } finally {
+                // do clean up, even if there was an exception
+                // Store context back to HttpSession
+                try {
+                    httpSession = ((HttpServletRequest) request).getSession(false);
+                } catch (IllegalStateException ignored) {}
+
+                if ((httpSession == null) && httpSessionExistedAtStartOfRequest) {
                     if (logger.isDebugEnabled()) {
                         logger.debug(
-                            "The HttpSession is currently null, and the HttpSessionContextIntegrationFilter is prohibited from creating a HttpSession (because the allowSessionCreation property is false) - SecurityContext thus not stored for next request");
+                            "HttpSession is now null, but was not null at start of request; session was invalidated, so do not create a new session");
                     }
-                } else if (!contextObject.equals(
-                        SecurityContextHolder.getContext())) {
-                    if (logger.isDebugEnabled()) {
-                        logger.debug(
-                            "HttpSession being created as SecurityContextHolder contents are non-default");
+                }
+
+                // Generate a HttpSession only if we need to
+                if ((httpSession == null)
+                    && !httpSessionExistedAtStartOfRequest) {
+                    if (!allowSessionCreation) {
+                        if (logger.isDebugEnabled()) {
+                            logger.debug(
+                                "The HttpSession is currently null, and the HttpSessionContextIntegrationFilter is prohibited from creating a HttpSession (because the allowSessionCreation property is false) - SecurityContext thus not stored for next request");
+                        }
+                    } else if (!contextObject.equals(
+                            SecurityContextHolder.getContext())) {
+                        if (logger.isDebugEnabled()) {
+                            logger.debug(
+                                "HttpSession being created as SecurityContextHolder contents are non-default");
+                        }
+
+                        try {
+                            httpSession = ((HttpServletRequest) request)
+                                .getSession(true);
+                        } catch (IllegalStateException ignored) {}
+                    } else {
+                        if (logger.isDebugEnabled()) {
+                            logger.debug(
+                                "HttpSession is null, but SecurityContextHolder has not changed from default: ' "
+                                + SecurityContextHolder.getContext()
+                                + "'; not creating HttpSession or storing SecurityContextHolder contents");
+                        }
                     }
+                }
+
+                // If HttpSession exists, store current SecurityContextHolder contents
+                if (httpSession != null) {
+                    httpSession.setAttribute(ACEGI_SECURITY_CONTEXT_KEY,
+                        SecurityContextHolder.getContext());
 
-                    try {
-                        httpSession = ((HttpServletRequest) request).getSession(true);
-                    } catch (IllegalStateException ignored) {}
-                } else {
                     if (logger.isDebugEnabled()) {
-                        logger.debug(
-                            "HttpSession is null, but SecurityContextHolder has not changed from default: ' "
-                            + SecurityContextHolder.getContext()
-                            + "'; not creating HttpSession or storing SecurityContextHolder contents");
+                        logger.debug("SecurityContext stored to HttpSession: '"
+                            + SecurityContextHolder.getContext() + "'");
                     }
                 }
-            }
 
-            // If HttpSession exists, store current SecurityContextHolder contents
-            if (httpSession != null) {
-                httpSession.setAttribute(ACEGI_SECURITY_CONTEXT_KEY,
-                    SecurityContextHolder.getContext());
+                // Remove SecurityContextHolder contents
+                SecurityContextHolder.setContext(generateNewContext());
 
                 if (logger.isDebugEnabled()) {
-                    logger.debug("SecurityContext stored to HttpSession: '"
-                        + SecurityContextHolder.getContext() + "'");
+                    logger.debug(
+                        "SecurityContextHolder set to new context, as request processing completed");
                 }
             }
-
-            // Remove SecurityContextHolder contents
-            SecurityContextHolder.setContext(generateNewContext());
-
-            if (logger.isDebugEnabled()) {
-                logger.debug(
-                    "SecurityContextHolder set to new context, as request processing completed");
-            }
         }
     }
 

core/src/test/java/org/acegisecurity/context/HttpSessionContextIntegrationFilterTests.java

@@ -83,6 +83,46 @@ public class HttpSessionContextIntegrationFilterTests extends TestCase {
         }
     }
 
+    public void testExceptionWithinFilterChainStillClearsSecurityContextHolder()
+        throws Exception {
+        // Build an Authentication object we simulate came from HttpSession
+        PrincipalAcegiUserToken sessionPrincipal = new PrincipalAcegiUserToken("key",
+                "someone", "password",
+                new GrantedAuthority[] {new GrantedAuthorityImpl("SOME_ROLE")});
+
+        // Build a Context to store in HttpSession (simulating prior request)
+        SecurityContext sc = new SecurityContextImpl();
+        sc.setAuthentication(sessionPrincipal);
+
+        // Build a mock request
+        MockHttpServletRequest request = new MockHttpServletRequest();
+        request.getSession().setAttribute(HttpSessionContextIntegrationFilter.ACEGI_SECURITY_CONTEXT_KEY,
+            sc);
+
+        MockHttpServletResponse response = new MockHttpServletResponse();
+        FilterChain chain = new MockFilterChain(sessionPrincipal, null,
+                new IOException());
+
+        // Prepare filter
+        HttpSessionContextIntegrationFilter filter = new HttpSessionContextIntegrationFilter();
+        filter.setContext(SecurityContextImpl.class);
+        filter.afterPropertiesSet();
+
+        // Execute filter
+        try {
+            executeFilterInContainerSimulator(new MockFilterConfig(), filter,
+                request, response, chain);
+            fail(
+                "We should have received the IOException thrown inside the filter chain here");
+        } catch (IOException ioe) {
+            assertTrue(true);
+        }
+
+        // Check the SecurityContextHolder is null, even though an exception was thrown during chain
+        assertEquals(new SecurityContextImpl(),
+            SecurityContextHolder.getContext());
+    }
+
     public void testExistingContextContentsCopiedIntoContextHolderFromSessionAndChangesToContextCopiedBackToSession()
         throws Exception {
         // Build an Authentication object we simulate came from HttpSession
@@ -106,7 +146,7 @@ public class HttpSessionContextIntegrationFilterTests extends TestCase {
 
         MockHttpServletResponse response = new MockHttpServletResponse();
         FilterChain chain = new MockFilterChain(sessionPrincipal,
-                updatedPrincipal);
+                updatedPrincipal, null);
 
         // Prepare filter
         HttpSessionContextIntegrationFilter filter = new HttpSessionContextIntegrationFilter();
@@ -134,7 +174,7 @@ public class HttpSessionContextIntegrationFilterTests extends TestCase {
         // Build a mock request
         MockHttpServletRequest request = new MockHttpServletRequest();
         MockHttpServletResponse response = new MockHttpServletResponse();
-        FilterChain chain = new MockFilterChain(null, updatedPrincipal);
+        FilterChain chain = new MockFilterChain(null, updatedPrincipal, null);
 
         // Prepare filter
         HttpSessionContextIntegrationFilter filter = new HttpSessionContextIntegrationFilter();
@@ -157,7 +197,7 @@ public class HttpSessionContextIntegrationFilterTests extends TestCase {
         // Build a mock request
         MockHttpServletRequest request = new MockHttpServletRequest(null, null);
         MockHttpServletResponse response = new MockHttpServletResponse();
-        FilterChain chain = new MockFilterChain(null, null);
+        FilterChain chain = new MockFilterChain(null, null, null);
 
         // Prepare filter
         HttpSessionContextIntegrationFilter filter = new HttpSessionContextIntegrationFilter();
@@ -185,7 +225,7 @@ public class HttpSessionContextIntegrationFilterTests extends TestCase {
             "NOT_A_CONTEXT_OBJECT");
 
         MockHttpServletResponse response = new MockHttpServletResponse();
-        FilterChain chain = new MockFilterChain(null, updatedPrincipal);
+        FilterChain chain = new MockFilterChain(null, updatedPrincipal, null);
 
         // Prepare filter
         HttpSessionContextIntegrationFilter filter = new HttpSessionContextIntegrationFilter();
@@ -216,11 +256,13 @@ public class HttpSessionContextIntegrationFilterTests extends TestCase {
     private class MockFilterChain extends TestCase implements FilterChain {
         private Authentication changeContextHolder;
         private Authentication expectedOnContextHolder;
+        private IOException toThrowDuringChain;
 
         public MockFilterChain(Authentication expectedOnContextHolder,
-            Authentication changeContextHolder) {
+            Authentication changeContextHolder, IOException toThrowDuringChain) {
             this.expectedOnContextHolder = expectedOnContextHolder;
             this.changeContextHolder = changeContextHolder;
+            this.toThrowDuringChain = toThrowDuringChain;
         }
 
         private MockFilterChain() {}
@@ -237,6 +279,10 @@ public class HttpSessionContextIntegrationFilterTests extends TestCase {
                 sc.setAuthentication(changeContextHolder);
                 SecurityContextHolder.setContext(sc);
             }
+
+            if (toThrowDuringChain != null) {
+                throw toThrowDuringChain;
+            }
         }
     }
 }

doc/xdocs/changes.xml

@@ -42,6 +42,7 @@
       <action dev="benalex" type="fix">Remove getters and setters from JdbcDaoImpl so IoC container cannot modify MappingSqlQuerys</action>
       <action dev="benalex" type="update">Refactor DAO authentication failure events under a consistent abstract superclass</action>
       <action dev="benalex" type="fix">JBoss container adapter to use getName() instead to toString() (see http://opensource.atlassian.com/projects/spring/browse/SEC-22)</action>
+      <action dev="benalex" type="fix">HttpSessionContextIntegrationFilter elegantly handles IOExceptions and ServletExceptions within filter chain (see http://opensource.atlassian.com/projects/spring/browse/SEC-20)</action>
     </release>
     <release version="0.8.2" date="2005-04-20">
       <action dev="benalex" type="fix">Correct location of AuthenticationSimpleHttpInvokerRequestExecutor in clientContext.xml</action>