SEC-1967: Restore original SecurityContext in finally when RunAsManager is used

Previously subclasses of AbstractSecurityInterceptor did not restore the original
Authentication when RunAsManager was used and an Exception was thrown in the
original method.

AbstractSecurityInterceptor has added a new method finallyInvocation which
should be invoked in a finally block immediately after the original invocation
which will restore the original Authentication. All existing sub classes have
been updated to use this new method.

core/src/main/java/org/springframework/security/access/intercept/AbstractSecurityInterceptor.java

@@ -66,7 +66,7 @@ import org.springframework.util.Assert;
  * <li>Pass control back to the concrete subclass, which will actually proceed with executing the object.
  * A {@link InterceptorStatusToken} is returned so that after the subclass has finished proceeding with
  * execution of the object, its finally clause can ensure the <code>AbstractSecurityInterceptor</code>
- * is re-called and tidies up correctly.</li>
+ * is re-called and tidies up correctly using {@link #finallyInvocation(InterceptorStatusToken)}.</li>
  * <li>The concrete subclass will re-call the <code>AbstractSecurityInterceptor</code> via the
  * {@link #afterInvocation(InterceptorStatusToken, Object)} method.</li>
  * <li>If the <code>RunAsManager</code> replaced the <code>Authentication</code> object, return the
@@ -91,6 +91,7 @@ import org.springframework.util.Assert;
  * </ol>
  *
  * @author Ben Alex
+ * @author Rob Winch
  */
 public abstract class AbstractSecurityInterceptor implements InitializingBean, ApplicationEventPublisherAware,
         MessageSourceAware {
@@ -242,6 +243,23 @@ public abstract class AbstractSecurityInterceptor implements InitializingBean, A
         }
     }
 
+    /**
+     * Cleans up the work of the <tt>AbstractSecurityInterceptor</tt> after the secure object invocation has been
+     * completed. This method should be invoked after the secure object invocation and before afterInvocation regardless
+     * of the secure object invocation returning successfully (i.e. it should be done in a finally block).
+     *
+     * @param token as returned by the {@link #beforeInvocation(Object)} method
+     */
+    protected void finallyInvocation(InterceptorStatusToken token) {
+        if (token != null && token.isContextHolderRefreshRequired()) {
+            if (logger.isDebugEnabled()) {
+                logger.debug("Reverting to original Authentication: " + token.getSecurityContext().getAuthentication());
+            }
+
+            SecurityContextHolder.setContext(token.getSecurityContext());
+        }
+    }
+
     /**
      * Completes the work of the <tt>AbstractSecurityInterceptor</tt> after the secure object invocation has been
      * completed.
@@ -256,13 +274,7 @@ public abstract class AbstractSecurityInterceptor implements InitializingBean, A
             return returnedObject;
         }
 
-        if (token.isContextHolderRefreshRequired()) {
-            if (logger.isDebugEnabled()) {
-                logger.debug("Reverting to original Authentication: " + token.getSecurityContext().getAuthentication());
-            }
-
-            SecurityContextHolder.setContext(token.getSecurityContext());
-        }
+        finallyInvocation(token); // continue to clean in this method for passivity
 
         if (afterInvocationManager != null) {
             // Attempt after invocation handling

core/src/main/java/org/springframework/security/access/intercept/aopalliance/MethodSecurityInterceptor.java

@@ -34,6 +34,7 @@ import org.aopalliance.intercept.MethodInvocation;
  * Refer to {@link AbstractSecurityInterceptor} for details on the workflow.
  *
  * @author Ben Alex
+ * @author Rob Winch
  */
 public class MethodSecurityInterceptor extends AbstractSecurityInterceptor implements MethodInterceptor {
     //~ Instance fields ================================================================================================
@@ -58,8 +59,12 @@ public class MethodSecurityInterceptor extends AbstractSecurityInterceptor imple
     public Object invoke(MethodInvocation mi) throws Throwable {
         InterceptorStatusToken token = super.beforeInvocation(mi);
 
-        Object result = mi.proceed();
-
+        Object result;
+        try {
+            result = mi.proceed();
+        } finally {
+            super.finallyInvocation(token);
+        }
         return super.afterInvocation(token, result);
     }
 

core/src/main/java/org/springframework/security/access/intercept/aspectj/AspectJMethodSecurityInterceptor.java

@@ -12,6 +12,7 @@ import org.springframework.security.access.intercept.aopalliance.MethodSecurityI
  * Alternatively you can use one of the pre-defined aspects from the aspects module.
  *
  * @author Luke Taylor
+ * @author Rob Winch
  * @since 3.0.3
  */
 public final class AspectJMethodSecurityInterceptor extends MethodSecurityInterceptor {
@@ -39,7 +40,12 @@ public final class AspectJMethodSecurityInterceptor extends MethodSecurityInterc
     public Object invoke(JoinPoint jp, AspectJCallback advisorProceed) {
         InterceptorStatusToken token = super.beforeInvocation(new MethodInvocationAdapter(jp));
 
-        Object result = advisorProceed.proceedWithObject();
+        Object result;
+        try {
+            result = advisorProceed.proceedWithObject();
+        }finally {
+            super.finallyInvocation(token);
+        }
 
         return super.afterInvocation(token, result);
     }

core/src/test/java/org/springframework/security/access/intercept/aopalliance/MethodSecurityInterceptorTests.java

@@ -45,11 +45,11 @@ import org.springframework.security.core.context.SecurityContext;
 import org.springframework.security.core.context.SecurityContextHolder;
 
 import java.util.*;
-
 /**
  * Tests {@link MethodSecurityInterceptor}.
  *
  * @author Ben Alex
+ * @author Rob Winch
  */
 @SuppressWarnings("unchecked")
 public class MethodSecurityInterceptorTests {
@@ -267,6 +267,31 @@ public class MethodSecurityInterceptorTests {
         assertSame(token, SecurityContextHolder.getContext().getAuthentication());
     }
 
+    // SEC-1967
+    @Test
+    public void runAsReplacementCleansAfterException() throws Exception {
+        createTarget(true);
+        when(realTarget.makeUpperCase(anyString())).thenThrow(new RuntimeException());
+        SecurityContext ctx = SecurityContextHolder.getContext();
+        ctx.setAuthentication(token);
+        token.setAuthenticated(true);
+        final RunAsManager runAs = mock(RunAsManager.class);
+        final RunAsUserToken runAsToken =
+            new RunAsUserToken("key", "someone", "creds", token.getAuthorities(), TestingAuthenticationToken.class);
+        interceptor.setRunAsManager(runAs);
+        mdsReturnsUserRole();
+        when(runAs.buildRunAs(eq(token), any(MethodInvocation.class), any(List.class))).thenReturn(runAsToken);
+
+        try {
+            advisedTarget.makeUpperCase("hello");
+            fail("Expected Exception");
+        }catch(RuntimeException success) {}
+
+        // Check we've changed back
+        assertSame(ctx, SecurityContextHolder.getContext());
+        assertSame(token, SecurityContextHolder.getContext().getAuthentication());
+    }
+
     @Test(expected=AuthenticationCredentialsNotFoundException.class)
     public void emptySecurityContextIsRejected() throws Exception {
         mdsReturnsUserRole();

core/src/test/java/org/springframework/security/access/intercept/aspectj/AspectJMethodSecurityInterceptorTests.java

@@ -34,15 +34,19 @@ import org.springframework.security.access.AccessDecisionManager;
 import org.springframework.security.access.AccessDeniedException;
 import org.springframework.security.access.SecurityConfig;
 import org.springframework.security.access.intercept.AfterInvocationManager;
+import org.springframework.security.access.intercept.RunAsManager;
+import org.springframework.security.access.intercept.RunAsUserToken;
 import org.springframework.security.access.method.MethodSecurityMetadataSource;
 import org.springframework.security.authentication.AuthenticationManager;
 import org.springframework.security.authentication.TestingAuthenticationToken;
 import org.springframework.security.core.Authentication;
+import org.springframework.security.core.context.SecurityContext;
 import org.springframework.security.core.context.SecurityContextHolder;
 import org.springframework.util.ClassUtils;
 
 import java.lang.reflect.Method;
 import java.util.Collection;
+import java.util.List;
 
 
 /**
@@ -50,6 +54,7 @@ import java.util.Collection;
  *
  * @author Ben Alex
  * @author Luke Taylor
+ * @author Rob Winch
  */
 @SuppressWarnings("deprecation")
 public class AspectJMethodSecurityInterceptorTests {
@@ -151,4 +156,51 @@ public class AspectJMethodSecurityInterceptorTests {
         verifyZeroInteractions(aim);
     }
 
+    // SEC-1967
+    @Test
+    @SuppressWarnings("unchecked")
+    public void invokeWithAspectJCallbackRunAsReplacementCleansAfterException() throws Exception {
+        SecurityContext ctx = SecurityContextHolder.getContext();
+        ctx.setAuthentication(token);
+        token.setAuthenticated(true);
+        final RunAsManager runAs = mock(RunAsManager.class);
+        final RunAsUserToken runAsToken =
+            new RunAsUserToken("key", "someone", "creds", token.getAuthorities(), TestingAuthenticationToken.class);
+        interceptor.setRunAsManager(runAs);
+        when(runAs.buildRunAs(eq(token), any(MethodInvocation.class), any(List.class))).thenReturn(runAsToken);
+        when(aspectJCallback.proceedWithObject()).thenThrow(new RuntimeException());
+
+        try {
+            interceptor.invoke(joinPoint, aspectJCallback);
+            fail("Expected Exception");
+        }catch(RuntimeException success) {}
+
+        // Check we've changed back
+        assertSame(ctx, SecurityContextHolder.getContext());
+        assertSame(token, SecurityContextHolder.getContext().getAuthentication());
+    }
+
+    // SEC-1967
+    @Test
+    @SuppressWarnings("unchecked")
+    public void invokeRunAsReplacementCleansAfterException() throws Throwable {
+        SecurityContext ctx = SecurityContextHolder.getContext();
+        ctx.setAuthentication(token);
+        token.setAuthenticated(true);
+        final RunAsManager runAs = mock(RunAsManager.class);
+        final RunAsUserToken runAsToken =
+            new RunAsUserToken("key", "someone", "creds", token.getAuthorities(), TestingAuthenticationToken.class);
+        interceptor.setRunAsManager(runAs);
+        when(runAs.buildRunAs(eq(token), any(MethodInvocation.class), any(List.class))).thenReturn(runAsToken);
+        when(joinPoint.proceed()).thenThrow(new RuntimeException());
+
+        try {
+            interceptor.invoke(joinPoint);
+            fail("Expected Exception");
+        }catch(RuntimeException success) {}
+
+        // Check we've changed back
+        assertSame(ctx, SecurityContextHolder.getContext());
+        assertSame(token, SecurityContextHolder.getContext().getAuthentication());
+    }
 }

web/src/main/java/org/springframework/security/web/access/intercept/FilterSecurityInterceptor.java

@@ -39,6 +39,7 @@ import org.springframework.security.web.FilterInvocation;
  * Refer to {@link AbstractSecurityInterceptor} for details on the workflow.</p>
  *
  * @author Ben Alex
+ * @author Rob Winch
  */
 public class FilterSecurityInterceptor extends AbstractSecurityInterceptor implements Filter {
     //~ Static fields/initializers =====================================================================================
@@ -113,7 +114,11 @@ public class FilterSecurityInterceptor extends AbstractSecurityInterceptor imple
 
             InterceptorStatusToken token = super.beforeInvocation(fi);
 
-            fi.getChain().doFilter(fi.getRequest(), fi.getResponse());
+            try {
+                fi.getChain().doFilter(fi.getRequest(), fi.getResponse());
+            } finally {
+                super.finallyInvocation(token);
+            }
 
             super.afterInvocation(token, null);
         }

web/src/test/java/org/springframework/security/web/access/intercept/FilterSecurityInterceptorTests.java

@@ -15,7 +15,7 @@
 
 package org.springframework.security.web.access.intercept;
 
-import static org.junit.Assert.fail;
+import static org.junit.Assert.*;
 import static org.mockito.Matchers.any;
 import static org.mockito.Mockito.*;
 
@@ -28,9 +28,11 @@ import org.springframework.security.access.SecurityConfig;
 import org.springframework.security.access.event.AuthorizedEvent;
 import org.springframework.security.access.intercept.AfterInvocationManager;
 import org.springframework.security.access.intercept.RunAsManager;
+import org.springframework.security.access.intercept.RunAsUserToken;
 import org.springframework.security.authentication.AuthenticationManager;
 import org.springframework.security.authentication.TestingAuthenticationToken;
 import org.springframework.security.core.Authentication;
+import org.springframework.security.core.context.SecurityContext;
 import org.springframework.security.core.context.SecurityContextHolder;
 import org.springframework.security.web.FilterInvocation;
 
@@ -44,6 +46,7 @@ import javax.servlet.http.HttpServletResponse;
  *
  * @author Ben Alex
  * @author Luke Taylor
+ * @author Rob Winch
  */
 public class FilterSecurityInterceptorTests {
     private AuthenticationManager am;
@@ -132,6 +135,39 @@ public class FilterSecurityInterceptorTests {
         verifyZeroInteractions(aim);
     }
 
+    // SEC-1967
+    @Test
+    @SuppressWarnings("unchecked")
+    public void finallyInvocationIsInvokedIfExceptionThrown() throws Exception {
+        SecurityContext ctx = SecurityContextHolder.getContext();
+        Authentication token = new TestingAuthenticationToken("Test", "Password", "NOT_USED");
+        token.setAuthenticated(true);
+        ctx.setAuthentication(token);
+
+        RunAsManager runAsManager = mock(RunAsManager.class);
+        when(runAsManager.buildRunAs(eq(token), any(), anyCollection())).thenReturn(new RunAsUserToken("key", "someone", "creds", token.getAuthorities(), token.getClass()));
+        interceptor.setRunAsManager(runAsManager);
+
+        FilterInvocation fi = createinvocation();
+        FilterChain chain = fi.getChain();
+
+        doThrow(new RuntimeException()).when(chain).doFilter(any(HttpServletRequest.class), any(HttpServletResponse.class));
+        when(ods.getAttributes(fi)).thenReturn(SecurityConfig.createList("MOCK_OK"));
+
+        AfterInvocationManager aim = mock(AfterInvocationManager.class);
+        interceptor.setAfterInvocationManager(aim);
+
+        try {
+            interceptor.invoke(fi);
+            fail("Expected exception");
+        } catch (RuntimeException expected) {
+        }
+
+        // Check we've changed back
+        assertSame(ctx, SecurityContextHolder.getContext());
+        assertSame(token, SecurityContextHolder.getContext().getAuthentication());
+    }
+
     private FilterInvocation createinvocation() {
         MockHttpServletResponse response = new MockHttpServletResponse();
         MockHttpServletRequest request = new MockHttpServletRequest();