diff --git a/src/Mvc/Mvc.Core/src/Authorization/AuthorizeFilter.cs b/src/Mvc/Mvc.Core/src/Authorization/AuthorizeFilter.cs index 0d2059b166..b407afc41c 100644 --- a/src/Mvc/Mvc.Core/src/Authorization/AuthorizeFilter.cs +++ b/src/Mvc/Mvc.Core/src/Authorization/AuthorizeFilter.cs @@ -23,8 +23,6 @@ namespace Microsoft.AspNetCore.Mvc.Authorization /// public class AuthorizeFilter : IAsyncAuthorizationFilter, IFilterFactory { - private AuthorizationPolicy _effectivePolicy; - /// /// Initializes a new instance. /// @@ -128,16 +126,8 @@ namespace Microsoft.AspNetCore.Mvc.Authorization internal async Task GetEffectivePolicyAsync(AuthorizationFilterContext context) { - if (_effectivePolicy != null) - { - return _effectivePolicy; - } - - var effectivePolicy = await ComputePolicyAsync(); - var canCache = PolicyProvider == null; - // Combine all authorize filters into single effective policy that's only run on the closest filter - var builder = new AuthorizationPolicyBuilder(effectivePolicy); + var builder = new AuthorizationPolicyBuilder(await ComputePolicyAsync()); for (var i = 0; i < context.Filters.Count; i++) { if (ReferenceEquals(this, context.Filters[i])) @@ -149,7 +139,6 @@ namespace Microsoft.AspNetCore.Mvc.Authorization { // Combine using the explicit policy, or the dynamic policy provider builder.Combine(await authorizeFilter.ComputePolicyAsync()); - canCache = canCache && authorizeFilter.PolicyProvider == null; } } @@ -169,20 +158,9 @@ namespace Microsoft.AspNetCore.Mvc.Authorization { builder.Combine(endpointPolicy); } - - // We cannot cache the policy since it varies by endpoint metadata. - canCache = false; } - effectivePolicy = builder?.Build() ?? effectivePolicy; - - // We can cache the effective policy when there is no custom policy provider - if (canCache) - { - _effectivePolicy = effectivePolicy; - } - - return effectivePolicy; + return builder.Build(); } /// diff --git a/src/Mvc/Mvc.Core/test/Authorization/AuthorizeFilterTest.cs b/src/Mvc/Mvc.Core/test/Authorization/AuthorizeFilterTest.cs index 645f784bf1..11d93b8907 100644 --- a/src/Mvc/Mvc.Core/test/Authorization/AuthorizeFilterTest.cs +++ b/src/Mvc/Mvc.Core/test/Authorization/AuthorizeFilterTest.cs @@ -513,26 +513,6 @@ namespace Microsoft.AspNetCore.Mvc.Authorization Assert.Same(policyProvider, actual.PolicyProvider); } - [Fact] - public async Task GetEffectivePolicyAsync_ReturnsCurrentPolicy_WhenNoEndpointMetadataIsAvailable() - { - // Arrange - var policy = new AuthorizationPolicyBuilder() - .RequireAssertion(_ => true) - .Build(); - var filter = new AuthorizeFilter(policy); - - var context = new AuthorizationFilterContext(ActionContext, new[] { filter }); - - // Act - var effectivePolicy = await filter.GetEffectivePolicyAsync(context); - - // Assert - // - // Verify the policy is cached - Assert.Same(effectivePolicy, await filter.GetEffectivePolicyAsync(context)); - } - [Fact] public async Task GetEffectivePolicyAsync_CombinesPoliciesFromAuthFilters() { diff --git a/src/Mvc/test/Mvc.IntegrationTests/AuthorizeFilterIntegrationTest.cs b/src/Mvc/test/Mvc.IntegrationTests/AuthorizeFilterIntegrationTest.cs index 09f6ef48c3..0729fbcd75 100644 --- a/src/Mvc/test/Mvc.IntegrationTests/AuthorizeFilterIntegrationTest.cs +++ b/src/Mvc/test/Mvc.IntegrationTests/AuthorizeFilterIntegrationTest.cs @@ -19,6 +19,7 @@ using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.ObjectPool; using Microsoft.Extensions.Options; +using Moq; using Xunit; namespace Microsoft.AspNetCore.Mvc.IntegrationTests @@ -53,6 +54,73 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Equal(2, policyProvider.GetPolicyCount); } + [Fact] + public async Task AuthorizeFilter_CalledTwiceWithDefaultProvider() + { + // Arrange + var applicationModelProviderContext = GetProviderContext(typeof(AuthorizeController)); + + var policy = new AuthorizationPolicyBuilder().RequireAssertion(_ => true).Build(); + var policyProvider = new Mock(Options.Create(new AuthorizationOptions())); + var getPolicyCalled = 0; + policyProvider.Setup(p => p.GetPolicyAsync(It.IsAny())).Callback(() => getPolicyCalled++).ReturnsAsync(policy); + + var controller = Assert.Single(applicationModelProviderContext.Result.Controllers); + var action = Assert.Single(controller.Actions); + var authorizeData = action.Attributes.OfType(); + var authorizeFilter = new AuthorizeFilter(policyProvider.Object, authorizeData); + + var actionContext = new ActionContext(GetHttpContext(), new RouteData(), new ControllerActionDescriptor()); + + var authorizationFilterContext = new AuthorizationFilterContext(actionContext, new[] { authorizeFilter }); + + // Act + await authorizeFilter.OnAuthorizationAsync(authorizationFilterContext); + await authorizeFilter.OnAuthorizationAsync(authorizationFilterContext); + + // Assert + Assert.Equal(2, getPolicyCalled); + } + + // This is a test for security, because we can't assume that any IAuthorizationPolicyProvider other than + // DefaultAuthorizationPolicyProvider will return the same result for the same input. So a cache could cause + // undesired access. + [Fact] + public async Task CombinedAuthorizeFilter_AlwaysCalledWithDefaultProvider() + { + // Arrange + var applicationModelProviderContext = GetProviderContext(typeof(AuthorizeController)); + + var policy = new AuthorizationPolicyBuilder().RequireAssertion(_ => true).Build(); + var policyProvider = new Mock(Options.Create(new AuthorizationOptions())); + var getPolicyCalled = 0; + policyProvider.Setup(p => p.GetPolicyAsync(It.IsAny())).Callback(() => getPolicyCalled++).ReturnsAsync(policy); + + var controller = Assert.Single(applicationModelProviderContext.Result.Controllers); + var action = Assert.Single(controller.Actions); + var authorizeData = action.Attributes.OfType(); + var authorizeFilter = new AuthorizeFilter(policyProvider.Object, authorizeData); + + var actionContext = new ActionContext(GetHttpContext(), new RouteData(), new ControllerActionDescriptor()); + + var authorizationFilterContext = new AuthorizationFilterContext(actionContext, action.Filters); + + authorizationFilterContext.Filters.Add(authorizeFilter); + + var secondFilter = new AuthorizeFilter(new AuthorizationPolicyBuilder().RequireAssertion(a => true).Build()); + authorizationFilterContext.Filters.Add(secondFilter); + + var thirdFilter = new AuthorizeFilter(policyProvider.Object, authorizeData); + authorizationFilterContext.Filters.Add(thirdFilter); + + // Act + await thirdFilter.OnAuthorizationAsync(authorizationFilterContext); + await thirdFilter.OnAuthorizationAsync(authorizationFilterContext); + + // Assert + Assert.Equal(4, getPolicyCalled); + } + // This is a test for security, because we can't assume that any IAuthorizationPolicyProvider other than // DefaultAuthorizationPolicyProvider will return the same result for the same input. So a cache could cause // undesired access.