Fix for #1652 - Make Authorization Filters run first

This change moves authorization filters to be the first filter stage
(before exception filters).

src/Microsoft.AspNet.Mvc.Core/FilterActionInvoker.cs

@@ -48,16 +48,24 @@ namespace Microsoft.AspNet.Mvc
             _filters = GetFilters();
             _cursor = new FilterCursor(_filters);
 
-            // >> ExceptionFilters >> AuthorizationFilters >> ActionFilters >> Action
+            await InvokeAllAuthorizationFiltersAsync();
-            await InvokeActionExceptionFilters();
 
-            // If Exception Filters or Authorization Filters provide a result, it's a short-circuit, we don't execute
+            // If Authorization Filters return a result, it's a short circuit because
-            // result filters around it.
+            // authorization failed. We don't execute Result Filters around the result.
+            Debug.Assert(_authorizationContext != null);
             if (_authorizationContext.Result != null)
             {
                 await _authorizationContext.Result.ExecuteResultAsync(ActionContext);
+                return;
             }
-            else if (_exceptionContext.Result != null)
+
+            // >> ExceptionFilters >> ActionFilters >> Action
+            await InvokeAllExceptionFiltersAsync();
+
+            // If Exception Filters provide a result, it's a short-circuit due to an exception.
+            // We don't execute Result Filters around the result.
+            Debug.Assert(_exceptionContext != null);
+            if (_exceptionContext.Result != null)
             {
                 await _exceptionContext.Result.ExecuteResultAsync(ActionContext);
             }
@@ -75,10 +83,13 @@ namespace Microsoft.AspNet.Mvc
             }
             else
             {
+                // We have a successful 'result' from the action or an Action Filter, so run
+                // Result Filters.
+                Debug.Assert(_actionExecutedContext != null);
                 var result = _actionExecutedContext.Result;
 
                 // >> ResultFilters >> (Result)
-                await InvokeActionResultWithFilters(result);
+                await InvokeAllResultFiltersAsync(result);
             }
         }
 
@@ -93,21 +104,63 @@ namespace Microsoft.AspNet.Mvc
             return filterProviderContext.Results.Select(item => item.Filter).Where(filter => filter != null).ToArray();
         }
 
-        private async Task InvokeActionExceptionFilters()
+        private async Task InvokeAllAuthorizationFiltersAsync()
+        {
+            _cursor.SetStage(FilterStage.AuthorizationFilters);
+
+            _authorizationContext = new AuthorizationContext(ActionContext, _filters);
+            await InvokeAuthorizationFilterAsync();
+        }
+
+        private async Task InvokeAuthorizationFilterAsync()
+        {
+            // We should never get here if we already have a result.
+            Debug.Assert(_authorizationContext != null);
+            Debug.Assert(_authorizationContext.Result == null);
+
+            var current = _cursor.GetNextFilter<IAuthorizationFilter, IAsyncAuthorizationFilter>();
+            if (current.FilterAsync != null)
+            {
+                await current.FilterAsync.OnAuthorizationAsync(_authorizationContext);
+
+                if (_authorizationContext.Result == null)
+                {
+                    // Only keep going if we don't have a result
+                    await InvokeAuthorizationFilterAsync();
+                }
+            }
+            else if (current.Filter != null)
+            {
+                current.Filter.OnAuthorization(_authorizationContext);
+
+                if (_authorizationContext.Result == null)
+                {
+                    // Only keep going if we don't have a result
+                    await InvokeAuthorizationFilterAsync();
+                }
+            }
+            else
+            {
+                // We've run out of Authorization Filters - if we haven't short circuited by now then this
+                // request is authorized.
+            }
+        }
+
+        private async Task InvokeAllExceptionFiltersAsync()
         {
             _cursor.SetStage(FilterStage.ExceptionFilters);
 
-            await InvokeExceptionFilter();
+            await InvokeExceptionFilterAsync();
         }
 
-        private async Task InvokeExceptionFilter()
+        private async Task InvokeExceptionFilterAsync()
         {
             var current = _cursor.GetNextFilter<IExceptionFilter, IAsyncExceptionFilter>();
             if (current.FilterAsync != null)
             {
                 // Exception filters run "on the way out" - so the filter is run after the rest of the
                 // pipeline.
-                await InvokeExceptionFilter();
+                await InvokeExceptionFilterAsync();
 
                 Debug.Assert(_exceptionContext != null);
                 if (_exceptionContext.Exception != null)
@@ -121,7 +174,7 @@ namespace Microsoft.AspNet.Mvc
             {
                 // Exception filters run "on the way out" - so the filter is run after the rest of the
                 // pipeline.
-                await InvokeExceptionFilter();
+                await InvokeExceptionFilterAsync();
 
                 Debug.Assert(_exceptionContext != null);
                 if (_exceptionContext.Exception != null)
@@ -143,23 +196,16 @@ namespace Microsoft.AspNet.Mvc
 
                 try
                 {
-                    await InvokeActionAuthorizationFilters();
+                    await InvokeAllActionFiltersAsync();
 
-                    Debug.Assert(_authorizationContext != null);
+                    // Action filters might 'return' an unhandled exception instead of throwing
-                    if (_authorizationContext.Result == null)
+                    Debug.Assert(_actionExecutedContext != null);
+                    if (_actionExecutedContext.Exception != null && !_actionExecutedContext.ExceptionHandled)
                     {
-                        // Authorization passed, run authorization filters and the action
+                        _exceptionContext.Exception = _actionExecutedContext.Exception;
-                        await InvokeActionMethodWithFilters();
+                        if (_actionExecutedContext.ExceptionDispatchInfo != null)
-
-                        // Action filters might 'return' an unahndled exception instead of throwing
-                        Debug.Assert(_actionExecutedContext != null);
-                        if (_actionExecutedContext.Exception != null && !_actionExecutedContext.ExceptionHandled)
                         {
-                            _exceptionContext.Exception = _actionExecutedContext.Exception;
+                            _exceptionContext.ExceptionDispatchInfo = _actionExecutedContext.ExceptionDispatchInfo;
-                            if (_actionExecutedContext.ExceptionDispatchInfo != null)
-                            {
-                                _exceptionContext.ExceptionDispatchInfo = _actionExecutedContext.ExceptionDispatchInfo;
-                            }
                         }
                     }
                 }
@@ -170,57 +216,15 @@ namespace Microsoft.AspNet.Mvc
             }
         }
 
-        private async Task InvokeActionAuthorizationFilters()
+        private async Task InvokeAllActionFiltersAsync()
-        {
-            _cursor.SetStage(FilterStage.AuthorizationFilters);
-
-            _authorizationContext = new AuthorizationContext(ActionContext, _filters);
-            await InvokeAuthorizationFilter();
-        }
-
-        private async Task InvokeAuthorizationFilter()
-        {
-            // We should never get here if we already have a result.
-            Debug.Assert(_authorizationContext != null);
-            Debug.Assert(_authorizationContext.Result == null);
-
-            var current = _cursor.GetNextFilter<IAuthorizationFilter, IAsyncAuthorizationFilter>();
-            if (current.FilterAsync != null)
-            {
-                await current.FilterAsync.OnAuthorizationAsync(_authorizationContext);
-
-                if (_authorizationContext.Result == null)
-                {
-                    // Only keep going if we don't have a result
-                    await InvokeAuthorizationFilter();
-                }
-            }
-            else if (current.Filter != null)
-            {
-                current.Filter.OnAuthorization(_authorizationContext);
-
-                if (_authorizationContext.Result == null)
-                {
-                    // Only keep going if we don't have a result
-                    await InvokeAuthorizationFilter();
-                }
-            }
-            else
-            {
-                // We've run out of Authorization Filters - if we haven't short circuited by now then this
-                // request is authorized.
-            }
-        }
-
-        private async Task InvokeActionMethodWithFilters()
         {
             _cursor.SetStage(FilterStage.ActionFilters);
             var arguments = await GetActionArgumentsAsync(ActionContext);
             _actionExecutingContext = new ActionExecutingContext(ActionContext, _filters, arguments);
-            await InvokeActionMethodFilter();
+            await InvokeActionFilterAsync();
         }
 
-        private async Task<ActionExecutedContext> InvokeActionMethodFilter()
+        private async Task<ActionExecutedContext> InvokeActionFilterAsync()
         {
             Debug.Assert(_actionExecutingContext != null);
             if (_actionExecutingContext.Result != null)
@@ -240,7 +244,7 @@ namespace Microsoft.AspNet.Mvc
             {
                 if (item.FilterAsync != null)
                 {
-                    await item.FilterAsync.OnActionExecutionAsync(_actionExecutingContext, InvokeActionMethodFilter);
+                    await item.FilterAsync.OnActionExecutionAsync(_actionExecutingContext, InvokeActionFilterAsync);
 
                     if (_actionExecutedContext == null)
                     {
@@ -267,7 +271,7 @@ namespace Microsoft.AspNet.Mvc
                     }
                     else
                     {
-                        item.Filter.OnActionExecuted(await InvokeActionMethodFilter());
+                        item.Filter.OnActionExecuted(await InvokeActionFilterAsync());
                     }
                 }
                 else
@@ -290,12 +294,12 @@ namespace Microsoft.AspNet.Mvc
             return _actionExecutedContext;
         }
 
-        private async Task InvokeActionResultWithFilters(IActionResult result)
+        private async Task InvokeAllResultFiltersAsync(IActionResult result)
         {
             _cursor.SetStage(FilterStage.ResultFilters);
 
             _resultExecutingContext = new ResultExecutingContext(ActionContext, _filters, result);
-            await InvokeActionResultFilter();
+            await InvokeResultFilterAsync();
 
             Debug.Assert(_resultExecutingContext != null);
             if (_resultExecutedContext.Exception != null && !_resultExecutedContext.ExceptionHandled)
@@ -312,7 +316,7 @@ namespace Microsoft.AspNet.Mvc
             }
         }
 
-        private async Task<ResultExecutedContext> InvokeActionResultFilter()
+        private async Task<ResultExecutedContext> InvokeResultFilterAsync()
         {
             Debug.Assert(_resultExecutingContext != null);
             if (_resultExecutingContext.Cancel == true)
@@ -333,7 +337,7 @@ namespace Microsoft.AspNet.Mvc
                 var item = _cursor.GetNextFilter<IResultFilter, IAsyncResultFilter>();
                 if (item.FilterAsync != null)
                 {
-                    await item.FilterAsync.OnResultExecutionAsync(_resultExecutingContext, InvokeActionResultFilter);
+                    await item.FilterAsync.OnResultExecutionAsync(_resultExecutingContext, InvokeResultFilterAsync);
 
                     if (_resultExecutedContext == null)
                     {
@@ -375,12 +379,12 @@ namespace Microsoft.AspNet.Mvc
                     }
                     else
                     {
-                        item.Filter.OnResultExecuted(await InvokeActionResultFilter());
+                        item.Filter.OnResultExecuted(await InvokeResultFilterAsync());
                     }
                 }
                 else
                 {
-                    await InvokeActionResult();
+                    await InvokeResultAsync();
 
                     Debug.Assert(_resultExecutedContext == null);
                     _resultExecutedContext = new ResultExecutedContext(
@@ -403,7 +407,7 @@ namespace Microsoft.AspNet.Mvc
             return _resultExecutedContext;
         }
 
-        private async Task InvokeActionResult()
+        private async Task InvokeResultAsync()
         {
             _cursor.SetStage(FilterStage.ActionResult);
 

test/Microsoft.AspNet.Mvc.Core.Test/ControllerActionInvokerTest.cs

@@ -326,10 +326,9 @@ namespace Microsoft.AspNet.Mvc
         }
 
         [Fact]
-        public async Task InvokeAction_ExceptionInAuthorizationFilterHandledByExceptionFilters()
+        public async Task InvokeAction_ExceptionInAuthorizationFilterCannotBeHandledByExceptionFilters()
         {
             // Arrange
-            Exception exception = null;
             var expected = new InvalidCastException();
 
             var exceptionFilter = new Mock<IExceptionFilter>(MockBehavior.Strict);
@@ -337,8 +336,6 @@ namespace Microsoft.AspNet.Mvc
                 .Setup(f => f.OnException(It.IsAny<ExceptionContext>()))
                 .Callback<ExceptionContext>(context =>
                 {
-                    exception = context.Exception;
-
                     // Mark as handled
                     context.Result = new EmptyResult();
                 })
@@ -365,10 +362,11 @@ namespace Microsoft.AspNet.Mvc
             });
 
             // Act
-            await invoker.InvokeAsync();
+            var thrown = await Assert.ThrowsAsync<InvalidCastException>(invoker.InvokeAsync);
 
             // Assert
-            exceptionFilter.Verify(f => f.OnException(It.IsAny<ExceptionContext>()), Times.Once());
+            Assert.Same(expected, thrown);
+            exceptionFilter.Verify(f => f.OnException(It.IsAny<ExceptionContext>()), Times.Never());
             authorizationFilter1.Verify(f => f.OnAuthorization(It.IsAny<AuthorizationContext>()), Times.Once());
         }
 
@@ -1067,7 +1065,7 @@ namespace Microsoft.AspNet.Mvc
             await invoker.InvokeAsync();
 
             // Assert
-            Assert.Equal(exception, context.Exception);
+            Assert.Same(exception, context.Exception);
 
             result.Verify(r => r.ExecuteResultAsync(It.IsAny<ActionContext>()), Times.Once());
 
@@ -1109,7 +1107,7 @@ namespace Microsoft.AspNet.Mvc
             await invoker.InvokeAsync();
 
             // Assert
-            Assert.Equal(exception, context.Exception);
+            Assert.Same(exception, context.Exception);
 
             result.Verify(r => r.ExecuteResultAsync(It.IsAny<ActionContext>()), Times.Once());
 
@@ -1153,7 +1151,7 @@ namespace Microsoft.AspNet.Mvc
             await invoker.InvokeAsync();
 
             // Assert
-            Assert.Equal(exception, context.Exception);
+            Assert.Same(exception, context.Exception);
 
             resultFilter1.Verify(f => f.OnResultExecuting(It.IsAny<ResultExecutingContext>()), Times.Once());
             resultFilter1.Verify(f => f.OnResultExecuted(It.IsAny<ResultExecutedContext>()), Times.Once());
@@ -1195,7 +1193,7 @@ namespace Microsoft.AspNet.Mvc
             await invoker.InvokeAsync();
 
             // Assert
-            Assert.Equal(exception, context.Exception);
+            Assert.Same(exception, context.Exception);
 
             resultFilter1.Verify(
                 f => f.OnResultExecutionAsync(It.IsAny<ResultExecutingContext>(), It.IsAny<ResultExecutionDelegate>()),

test/Microsoft.AspNet.Mvc.FunctionalTests/FiltersTest.cs

@@ -457,9 +457,9 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests
         }
 
         // Result Filter throws.
-        // Verifies the Global Exception Filter does not handle it.
+        // Exception Filters don't get a chance to handle this.
         [Fact]
-        public async Task ThrowingResultFilter_NotHandledByGlobalExceptionFilter()
+        public async Task ThrowingFilters_ResultFilter_NotHandledByGlobalExceptionFilter()
         {
             // Arrange
             var server = TestServer.Create(_services, _app);
@@ -472,27 +472,39 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests
 
         // Action Filter throws.
         // Verifies the Global Exception Filter handles it.
-        [Theory]
+        [Fact]
-        [InlineData("http://localhost/Home/ThrowingActionFilter")]
+        public async Task ThrowingFilters_ActionFilter_HandledByGlobalExceptionFilter()
-        [InlineData("http://localhost/Home/ThrowingAuthorizationFilter")]
-        public async Task ThrowingFilters_HandledByGlobalExceptionFilter(string url)
         {
             // Arrange
             var server = TestServer.Create(_services, _app);
             var client = server.CreateClient();
 
             // Act
-            var response = await client.GetAsync(url);
+            var response = await client.GetAsync("http://localhost/Home/ThrowingActionFilter");
 
             // Assert
             Assert.Equal(HttpStatusCode.OK, response.StatusCode);
             Assert.Equal("GlobalExceptionFilter.OnException", await response.Content.ReadAsStringAsync());
         }
 
+        // Authorization Filter throws.
+        // Exception Filters don't get a chance to handle this.
+        [Fact]
+        public async Task ThrowingFilters_AuthFilter_NotHandledByGlobalExceptionFilter()
+        {
+            // Arrange
+            var server = TestServer.Create(_services, _app);
+            var client = server.CreateClient();
+
+            // Act & Assert
+            await Assert.ThrowsAsync<InvalidProgramException>(
+                () => client.GetAsync("http://localhost/Home/ThrowingAuthorizationFilter"));
+        }
+
         // Exception Filter throws.
         // Verifies the thrown exception is ignored.
         [Fact]
-        public async Task ThrowingExceptionFilter_NotHandledByGlobalExceptionFilter()
+        public async Task ThrowingExceptionFilter_ExceptionFilter_NotHandledByGlobalExceptionFilter()
         {
             // Arrange
             var server = TestServer.Create(_services, _app);