diff --git a/src/Microsoft.AspNet.Mvc.Core/FilterActionInvoker.cs b/src/Microsoft.AspNet.Mvc.Core/FilterActionInvoker.cs index c039ca0304..bba19c8bc3 100644 --- a/src/Microsoft.AspNet.Mvc.Core/FilterActionInvoker.cs +++ b/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 InvokeActionExceptionFilters(); + await InvokeAllAuthorizationFiltersAsync(); - // If Exception Filters or Authorization Filters provide a result, it's a short-circuit, we don't execute - // result filters around it. + // If Authorization Filters return a result, it's a short circuit because + // 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(); + 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(); 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); - if (_authorizationContext.Result == null) + // Action filters might 'return' an unhandled exception instead of throwing + Debug.Assert(_actionExecutedContext != null); + if (_actionExecutedContext.Exception != null && !_actionExecutedContext.ExceptionHandled) { - // Authorization passed, run authorization filters and the action - await InvokeActionMethodWithFilters(); - - // Action filters might 'return' an unahndled exception instead of throwing - Debug.Assert(_actionExecutedContext != null); - if (_actionExecutedContext.Exception != null && !_actionExecutedContext.ExceptionHandled) + _exceptionContext.Exception = _actionExecutedContext.Exception; + if (_actionExecutedContext.ExceptionDispatchInfo != null) { - _exceptionContext.Exception = _actionExecutedContext.Exception; - if (_actionExecutedContext.ExceptionDispatchInfo != null) - { - _exceptionContext.ExceptionDispatchInfo = _actionExecutedContext.ExceptionDispatchInfo; - } + _exceptionContext.ExceptionDispatchInfo = _actionExecutedContext.ExceptionDispatchInfo; } } } @@ -170,57 +216,15 @@ namespace Microsoft.AspNet.Mvc } } - private async Task InvokeActionAuthorizationFilters() - { - _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(); - 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() + private async Task InvokeAllActionFiltersAsync() { _cursor.SetStage(FilterStage.ActionFilters); var arguments = await GetActionArgumentsAsync(ActionContext); _actionExecutingContext = new ActionExecutingContext(ActionContext, _filters, arguments); - await InvokeActionMethodFilter(); + await InvokeActionFilterAsync(); } - private async Task InvokeActionMethodFilter() + private async Task 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 InvokeActionResultFilter() + private async Task InvokeResultFilterAsync() { Debug.Assert(_resultExecutingContext != null); if (_resultExecutingContext.Cancel == true) @@ -333,7 +337,7 @@ namespace Microsoft.AspNet.Mvc var item = _cursor.GetNextFilter(); 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); diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ControllerActionInvokerTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ControllerActionInvokerTest.cs index 038b732ae7..6e3958de3b 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ControllerActionInvokerTest.cs +++ b/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(MockBehavior.Strict); @@ -337,8 +336,6 @@ namespace Microsoft.AspNet.Mvc .Setup(f => f.OnException(It.IsAny())) .Callback(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(invoker.InvokeAsync); // Assert - exceptionFilter.Verify(f => f.OnException(It.IsAny()), Times.Once()); + Assert.Same(expected, thrown); + exceptionFilter.Verify(f => f.OnException(It.IsAny()), Times.Never()); authorizationFilter1.Verify(f => f.OnAuthorization(It.IsAny()), 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()), 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()), 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()), Times.Once()); resultFilter1.Verify(f => f.OnResultExecuted(It.IsAny()), 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(), It.IsAny()), diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/FiltersTest.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/FiltersTest.cs index 0d6984876a..0c13710b0b 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/FiltersTest.cs +++ b/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] - [InlineData("http://localhost/Home/ThrowingActionFilter")] - [InlineData("http://localhost/Home/ThrowingAuthorizationFilter")] - public async Task ThrowingFilters_HandledByGlobalExceptionFilter(string url) + [Fact] + public async Task ThrowingFilters_ActionFilter_HandledByGlobalExceptionFilter() { // 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( + () => 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);