diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Authorization/AuthorizeFilter.cs b/src/Microsoft.AspNetCore.Mvc.Core/Authorization/AuthorizeFilter.cs index 727ac3d659..29cf7cc085 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Authorization/AuthorizeFilter.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Authorization/AuthorizeFilter.cs @@ -108,6 +108,24 @@ namespace Microsoft.AspNetCore.Mvc.Authorization bool IFilterFactory.IsReusable => true; + // Computes the actual policy for this filter using either Policy or PolicyProvider + AuthorizeData + private Task ComputePolicyAsync() + { + if (Policy != null) + { + return Task.FromResult(Policy); + } + if (PolicyProvider == null) + { + throw new InvalidOperationException( + Resources.FormatAuthorizeFilter_AuthorizationPolicyCannotBeCreated( + nameof(AuthorizationPolicy), + nameof(IAuthorizationPolicyProvider))); + } + + return AuthorizationPolicy.CombineAsync(PolicyProvider, AuthorizeData); + } + private async Task GetEffectivePolicyAsync(AuthorizationFilterContext context) { if (_effectivePolicy != null) @@ -115,7 +133,8 @@ namespace Microsoft.AspNetCore.Mvc.Authorization return _effectivePolicy; } - var effectivePolicy = Policy; + var effectivePolicy = await ComputePolicyAsync(); + var canCache = PolicyProvider == null; if (_mvcOptions == null) { @@ -124,13 +143,13 @@ namespace Microsoft.AspNetCore.Mvc.Authorization if (_mvcOptions.AllowCombiningAuthorizeFilters) { - if (!context.IsEffectivePolicy(this)) + if (!context.IsEffectivePolicy(this)) { return null; } // Combine all authorize filters into single effective policy that's only run on the closest filter - AuthorizationPolicyBuilder builder = null; + var builder = new AuthorizationPolicyBuilder(effectivePolicy); for (var i = 0; i < context.Filters.Count; i++) { if (ReferenceEquals(this, context.Filters[i])) @@ -140,29 +159,17 @@ namespace Microsoft.AspNetCore.Mvc.Authorization if (context.Filters[i] is AuthorizeFilter authorizeFilter) { - builder = builder ?? new AuthorizationPolicyBuilder(effectivePolicy); - builder.Combine(authorizeFilter.Policy); + // Combine using the explicit policy, or the dynamic policy provider + builder.Combine(await authorizeFilter.ComputePolicyAsync()); + canCache = canCache && authorizeFilter.PolicyProvider == null; } } effectivePolicy = builder?.Build() ?? effectivePolicy; } - if (effectivePolicy == null) - { - if (PolicyProvider == null) - { - throw new InvalidOperationException( - Resources.FormatAuthorizeFilter_AuthorizationPolicyCannotBeCreated( - nameof(AuthorizationPolicy), - nameof(IAuthorizationPolicyProvider))); - } - - effectivePolicy = await AuthorizationPolicy.CombineAsync(PolicyProvider, AuthorizeData); - } - - // We can cache the effective policy when there is no custom policy provider - if (PolicyProvider == null) + // We can cache the effective policy when there is no custom policy provider + if (canCache) { _effectivePolicy = effectivePolicy; } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Authorization/AuthorizeFilterTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Authorization/AuthorizeFilterTest.cs index 765fccd0e3..fdde1514de 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Authorization/AuthorizeFilterTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Authorization/AuthorizeFilterTest.cs @@ -221,6 +221,81 @@ namespace Microsoft.AspNetCore.Mvc.Authorization Assert.Null(authorizationContext.Result); } + private class TestPolicyProvider : IAuthorizationPolicyProvider + { + private AuthorizationPolicy _true = new AuthorizationPolicyBuilder().RequireAssertion(_ => true).Build(); + private AuthorizationPolicy _false = new AuthorizationPolicyBuilder().RequireAssertion(_ => false).Build(); + + public int GetPolicyCalls = 0; + + public Task GetDefaultPolicyAsync() + => Task.FromResult(_true); + + public Task GetPolicyAsync(string policyName) + { + GetPolicyCalls++; + return Task.FromResult(policyName == "true" ? _true : _false); + } + } + + [Fact] + public async Task AuthorizationFilterCombinesMultipleFiltersWithPolicyProvider() + { + // Arrange + var authorizeFilter = new AuthorizeFilter(new TestPolicyProvider(), new IAuthorizeData[] + { + new AuthorizeAttribute { Policy = "true"}, + new AuthorizeAttribute { Policy = "false"} + }); + var authorizationContext = GetAuthorizationContext(anonymous: false, registerServices: s => s.Configure(o => o.AllowCombiningAuthorizeFilters = true)); + // Effective policy should fail, if both are combined + authorizationContext.Filters.Add(authorizeFilter); + var secondFilter = new AuthorizeFilter(new AuthorizationPolicyBuilder().RequireAssertion(a => true).Build()); + authorizationContext.Filters.Add(secondFilter); + + // Act + await secondFilter.OnAuthorizationAsync(authorizationContext); + + // Assert + Assert.IsType(authorizationContext.Result); + } + + [Fact] + public async Task AuthorizationFilterCombinesMultipleFiltersWithDifferentPolicyProvider() + { + // Arrange + var testProvider1 = new TestPolicyProvider(); + var testProvider2 = new TestPolicyProvider(); + var authorizeFilter = new AuthorizeFilter(testProvider1, new IAuthorizeData[] + { + new AuthorizeAttribute { Policy = "true"}, + new AuthorizeAttribute { Policy = "false"} + }); + var authorizationContext = GetAuthorizationContext(anonymous: false, registerServices: s => s.Configure(o => o.AllowCombiningAuthorizeFilters = true)); + // Effective policy should fail, if both are combined + authorizationContext.Filters.Add(authorizeFilter); + var secondFilter = new AuthorizeFilter(new AuthorizationPolicyBuilder().RequireAssertion(a => true).Build()); + authorizationContext.Filters.Add(secondFilter); + var thirdFilter = new AuthorizeFilter(testProvider2, new IAuthorizeData[] { new AuthorizeAttribute(policy: "something") }); + authorizationContext.Filters.Add(thirdFilter); + + // Act + await thirdFilter.OnAuthorizationAsync(authorizationContext); + + // Assert + Assert.IsType(authorizationContext.Result); + Assert.Equal(2, testProvider1.GetPolicyCalls); + Assert.Equal(1, testProvider2.GetPolicyCalls); + + // Make sure the policy calls are not cached + await thirdFilter.OnAuthorizationAsync(authorizationContext); + + // Assert + Assert.IsType(authorizationContext.Result); + Assert.Equal(4, testProvider1.GetPolicyCalls); + Assert.Equal(2, testProvider2.GetPolicyCalls); + } + [Fact] public async Task AuthorizationFilterCombinesMultipleFilters() { diff --git a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/AuthorizeFilterIntegrationTest.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/AuthorizeFilterIntegrationTest.cs index 1134aa2f73..596d5b8ab3 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/AuthorizeFilterIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/AuthorizeFilterIntegrationTest.cs @@ -54,11 +54,47 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Equal(2, policyProvider.GetPolicyCount); } - private HttpContext GetHttpContext() + // 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_AlwaysCalledWithNonDefaultProvider() + { + // Arrange + var applicationModelProviderContext = GetProviderContext(typeof(AuthorizeController)); + + var policyProvider = new TestAuthorizationPolicyProvider(); + + var controller = Assert.Single(applicationModelProviderContext.Result.Controllers); + var action = Assert.Single(controller.Actions); + var authorizeData = action.Attributes.OfType(); + var authorizeFilter = new AuthorizeFilter(policyProvider, authorizeData); + + var actionContext = new ActionContext(GetHttpContext(combineAuthorize: true), 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, authorizeData); + authorizationFilterContext.Filters.Add(thirdFilter); + + // Act + await thirdFilter.OnAuthorizationAsync(authorizationFilterContext); + await thirdFilter.OnAuthorizationAsync(authorizationFilterContext); + + // Assert + Assert.Equal(4, policyProvider.GetPolicyCount); + } + + private HttpContext GetHttpContext(bool combineAuthorize = false) { var httpContext = new DefaultHttpContext(); - httpContext.RequestServices = GetServices(); + httpContext.RequestServices = GetServices(combineAuthorize); return httpContext; } @@ -73,11 +109,11 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests return context; } - private static IServiceProvider GetServices() + private static IServiceProvider GetServices(bool combineAuthorize) { var serviceCollection = new ServiceCollection(); serviceCollection.AddAuthorization(); - serviceCollection.AddMvc(); + serviceCollection.AddMvc(o => o.AllowCombiningAuthorizeFilters = combineAuthorize); serviceCollection .AddSingleton(NullLoggerFactory.Instance) .AddTransient, Logger>()