Stop caching policies in AuthorizeFilter (#9957)

src/Mvc/Mvc.Core/src/Authorization/AuthorizeFilter.cs

@@ -23,8 +23,6 @@ namespace Microsoft.AspNetCore.Mvc.Authorization
     /// </summary>
     public class AuthorizeFilter : IAsyncAuthorizationFilter, IFilterFactory
     {
-        private AuthorizationPolicy _effectivePolicy;
-
         /// <summary>
         /// Initializes a new <see cref="AuthorizeFilter"/> instance.
         /// </summary>
@@ -128,16 +126,8 @@ namespace Microsoft.AspNetCore.Mvc.Authorization
 
         internal async Task<AuthorizationPolicy> 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;
+            return builder.Build();
-
-            // We can cache the effective policy when there is no custom policy provider
-            if (canCache)
-            {
-                _effectivePolicy = effectivePolicy;
-            }
-
-            return effectivePolicy;
         }
 
         /// <inheritdoc />

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()
         {

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<DefaultAuthorizationPolicyProvider>(Options.Create<AuthorizationOptions>(new AuthorizationOptions()));
+            var getPolicyCalled = 0;
+            policyProvider.Setup(p => p.GetPolicyAsync(It.IsAny<string>())).Callback(() => getPolicyCalled++).ReturnsAsync(policy);
+
+            var controller = Assert.Single(applicationModelProviderContext.Result.Controllers);
+            var action = Assert.Single(controller.Actions);
+            var authorizeData = action.Attributes.OfType<AuthorizeAttribute>();
+            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<DefaultAuthorizationPolicyProvider>(Options.Create<AuthorizationOptions>(new AuthorizationOptions()));
+            var getPolicyCalled = 0;
+            policyProvider.Setup(p => p.GetPolicyAsync(It.IsAny<string>())).Callback(() => getPolicyCalled++).ReturnsAsync(policy);
+
+            var controller = Assert.Single(applicationModelProviderContext.Result.Controllers);
+            var action = Assert.Single(controller.Actions);
+            var authorizeData = action.Attributes.OfType<AuthorizeAttribute>();
+            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.