Fix regression with Authorize + IPolicyProvider (#8068)

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<AuthorizationPolicy> 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<AuthorizationPolicy> 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<AuthorizeFilter>(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);
+                        // Combine using the explicit policy, or the dynamic policy provider
-                        builder.Combine(authorizeFilter.Policy);
+                        builder.Combine(await authorizeFilter.ComputePolicyAsync());
+                        canCache = canCache && authorizeFilter.PolicyProvider == null;
                     }
                 }
 
                 effectivePolicy = builder?.Build() ?? effectivePolicy;
             }
 
-            if (effectivePolicy == null)
+            // We can cache the effective policy when there is no custom policy provider
-            {
+            if (canCache)
-                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)
             {
                 _effectivePolicy = effectivePolicy;
             }

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<AuthorizationPolicy> GetDefaultPolicyAsync()
+                => Task.FromResult(_true);
+
+            public Task<AuthorizationPolicy> 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<MvcOptions>(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<ForbidResult>(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<MvcOptions>(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<ForbidResult>(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<ForbidResult>(authorizationContext.Result);
+            Assert.Equal(4, testProvider1.GetPolicyCalls);
+            Assert.Equal(2, testProvider2.GetPolicyCalls);
+        }
+
         [Fact]
         public async Task AuthorizationFilterCombinesMultipleFilters()
         {

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<AuthorizeAttribute>();
+            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<ILoggerFactory>(NullLoggerFactory.Instance)
                 .AddTransient<ILogger<DefaultAuthorizationService>, Logger<DefaultAuthorizationService>>()