Support overriding AuthorizeFilter.OnAuthorizationAsync in more caseshalter73/30025

3 files changed, 115 insertions, 28 deletions

diff --git a/src/Mvc/Mvc.Core/src/ApplicationModels/AuthorizationApplicationModelProvider.cs b/src/Mvc/Mvc.Core/src/ApplicationModels/AuthorizationApplicationModelProvider.cs
index f44c2c7722..5bf139efee 100644
--- a/src/Mvc/Mvc.Core/src/ApplicationModels/AuthorizationApplicationModelProvider.cs
+++ b/src/Mvc/Mvc.Core/src/ApplicationModels/AuthorizationApplicationModelProvider.cs
@@ -74,11 +74,8 @@ internal class AuthorizationApplicationModelProvider : IApplicationModelProvider
 
     public static AuthorizeFilter GetFilter(IAuthorizationPolicyProvider policyProvider, IEnumerable<IAuthorizeData> authData)
     {
-        // The default policy provider will make the same policy for given input, so make it only once.
-        // This will always execute synchronously.
-        if (policyProvider.GetType() == typeof(DefaultAuthorizationPolicyProvider))
+        if (GetPolicyIfDefaultProvider(policyProvider, authData) is AuthorizationPolicy policy)
         {
-            var policy = AuthorizationPolicy.CombineAsync(policyProvider, authData).GetAwaiter().GetResult()!;
             return new AuthorizeFilter(policy);
         }
         else
@@ -86,4 +83,16 @@ internal class AuthorizationApplicationModelProvider : IApplicationModelProvider
             return new AuthorizeFilter(policyProvider, authData);
         }
     }
+
+    // The default policy provider will make the same policy for given input, so make it only once.
+    // This will always execute synchronously.
+    public static AuthorizationPolicy? GetPolicyIfDefaultProvider(IAuthorizationPolicyProvider policyProvider, IEnumerable<IAuthorizeData> authData)
+    {
+        if (policyProvider.GetType() != typeof(DefaultAuthorizationPolicyProvider))
+        {
+            return null;
+        }
+
+        return AuthorizationPolicy.CombineAsync(policyProvider, authData).GetAwaiter().GetResult();
+    }
 }
diff --git a/src/Mvc/Mvc.Core/src/Authorization/AuthorizeFilter.cs b/src/Mvc/Mvc.Core/src/Authorization/AuthorizeFilter.cs
index fc24a8a75f..3fc19460f1 100644
--- a/src/Mvc/Mvc.Core/src/Authorization/AuthorizeFilter.cs
+++ b/src/Mvc/Mvc.Core/src/Authorization/AuthorizeFilter.cs
@@ -1,11 +1,8 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-using System;
-using System.Collections.Generic;
 using System.Diagnostics;
 using System.Linq;
-using System.Threading.Tasks;
 using Microsoft.AspNetCore.Authorization;
 using Microsoft.AspNetCore.Authorization.Policy;
 using Microsoft.AspNetCore.Http;
@@ -87,7 +84,7 @@ public class AuthorizeFilter : IAsyncAuthorizationFilter, IFilterFactory
     /// <summary>
     /// The <see cref="IAuthorizationPolicyProvider"/> to use to resolve policy names.
     /// </summary>
-    public IAuthorizationPolicyProvider? PolicyProvider { get; }
+    public IAuthorizationPolicyProvider? PolicyProvider { get; private set; }
 
     /// <summary>
     /// The <see cref="IAuthorizeData"/> to combine into an <see cref="IAuthorizeData"/>.
@@ -101,7 +98,7 @@ public class AuthorizeFilter : IAsyncAuthorizationFilter, IFilterFactory
     /// If<c>null</c>, the policy will be constructed using
     /// <see cref="AuthorizationPolicy.CombineAsync(IAuthorizationPolicyProvider, IEnumerable{IAuthorizeData})"/>.
     /// </remarks>
-    public AuthorizationPolicy? Policy { get; }
+    public AuthorizationPolicy? Policy { get; private set; }
 
     bool IFilterFactory.IsReusable => true;
 
@@ -207,15 +204,24 @@ public class AuthorizeFilter : IAsyncAuthorizationFilter, IFilterFactory
 
     IFilterMetadata IFilterFactory.CreateInstance(IServiceProvider serviceProvider)
     {
-        if (Policy != null || PolicyProvider != null)
+        if (Policy is null && PolicyProvider is null)
         {
-            // The filter is fully constructed. Use the current instance to authorize.
-            return this;
+            // REVIEW: Can serviceProvider ever change here? Can this ever be called in parallel?
+            // We mutate the existing object instead of returning a new one so OnAuthorizationAsync can still be overridden.
+            // See https://github.com/dotnet/aspnetcore/issues/30025
+            Debug.Assert(AuthorizeData != null);
+            var policyProvider = serviceProvider.GetRequiredService<IAuthorizationPolicyProvider>();
+
+            Policy = AuthorizationApplicationModelProvider.GetPolicyIfDefaultProvider(policyProvider, AuthorizeData);
+
+            if (Policy is null)
+            {
+                PolicyProvider = policyProvider;
+            }
         }
 
-        Debug.Assert(AuthorizeData != null);
-        var policyProvider = serviceProvider.GetRequiredService<IAuthorizationPolicyProvider>();
-        return AuthorizationApplicationModelProvider.GetFilter(policyProvider, AuthorizeData);
+        // The filter is fully constructed. Always use the current instance to call overridden OnAuthorizationAsync methods.
+        return this;
     }
 
     private static bool HasAllowAnonymous(AuthorizationFilterContext context)
diff --git a/src/Mvc/Mvc.Core/test/Authorization/AuthorizeFilterTest.cs b/src/Mvc/Mvc.Core/test/Authorization/AuthorizeFilterTest.cs
index 12f0b4ec62..8c6d4dd090 100644
--- a/src/Mvc/Mvc.Core/test/Authorization/AuthorizeFilterTest.cs
+++ b/src/Mvc/Mvc.Core/test/Authorization/AuthorizeFilterTest.cs
@@ -1,11 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-using System;
-using System.Collections.Generic;
-using System.Linq;
 using System.Security.Claims;
-using System.Threading.Tasks;
 using Microsoft.AspNetCore.Authentication;
 using Microsoft.AspNetCore.Authorization;
 using Microsoft.AspNetCore.Authorization.Infrastructure;
@@ -17,7 +13,6 @@ using Microsoft.AspNetCore.Routing;
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Options;
 using Moq;
-using Xunit;
 
 namespace Microsoft.AspNetCore.Mvc.Authorization;
 
@@ -467,7 +462,7 @@ public class AuthorizeFilterTest
 
     [Theory]
     [MemberData(nameof(AuthorizeFiltersCreatedWithoutPolicyOrPolicyProvider))]
-    public void CreateInstance_ReturnsNewFilterIfPolicyAndPolicyProviderAreNotSet(AuthorizeFilter authorizeFilter)
+    public void CreateInstance_ReturnsSelfFilterIfPolicyAndPolicyProviderAreNotSet(AuthorizeFilter authorizeFilter)
     {
         // Arrange
         var factory = (IFilterFactory)authorizeFilter;
@@ -486,14 +481,13 @@ public class AuthorizeFilterTest
         var result = factory.CreateInstance(serviceProvider);
 
         // Assert
-        Assert.NotSame(authorizeFilter, result);
-        var actual = Assert.IsType<AuthorizeFilter>(result);
-        Assert.NotNull(actual.Policy);
+        Assert.Same(authorizeFilter, result);
+        Assert.NotNull(authorizeFilter.Policy);
     }
 
     [Theory]
     [MemberData(nameof(AuthorizeFiltersCreatedWithoutPolicyOrPolicyProvider))]
-    public void CreateInstance_ReturnsNewFilterIfPolicyAndPolicyProviderAreNotSetAndCustomProviderIsUsed(
+    public void CreateInstance_ReturnsSelfIfPolicyAndPolicyProviderAreNotSetAndCustomProviderIsUsed(
         AuthorizeFilter authorizeFilter)
     {
         // Arrange
@@ -507,9 +501,87 @@ public class AuthorizeFilterTest
         var result = factory.CreateInstance(serviceProvider);
 
         // Assert
-        Assert.NotSame(authorizeFilter, result);
-        var actual = Assert.IsType<AuthorizeFilter>(result);
-        Assert.Same(policyProvider, actual.PolicyProvider);
+        Assert.Same(authorizeFilter, result);
+        Assert.Same(policyProvider, authorizeFilter.PolicyProvider);
+    }
+
+    public class OverriddenAuthorizeFilter : AuthorizeFilter
+    {
+        public OverriddenAuthorizeFilter()
+            : base()
+        {
+        }
+
+        public OverriddenAuthorizeFilter(AuthorizationPolicy policy)
+            : base(policy)
+        {
+        }
+
+        public OverriddenAuthorizeFilter(IAuthorizationPolicyProvider policyProvider, IEnumerable<IAuthorizeData> authorizeData)
+            : base(policyProvider, authorizeData)
+        {
+        }
+
+        public Exception OnAuthorizeAsyncException { get; set; }
+
+        public override Task OnAuthorizationAsync(AuthorizationFilterContext context)
+        {
+            throw OnAuthorizeAsyncException;
+        }
+    }
+
+    [Fact]
+    public async Task CreateInstance_AllowsOverridingOnAuthorizeAsyncIfPolicyAndPolicyProviderAreNotSet()
+    {
+        var authorizeFilter = new OverriddenAuthorizeFilter
+        {
+            OnAuthorizeAsyncException = new Exception(),
+        };
+
+        // The serviceProvider still must contain a policy provider if no policy
+        // or policy provider was provided to the filter constructor.
+        var policyProvider = Mock.Of<IAuthorizationPolicyProvider>();
+        var serviceProvider = new ServiceCollection()
+            .AddSingleton(policyProvider)
+            .BuildServiceProvider();
+
+        var factory = (IFilterFactory)authorizeFilter;
+        var result = factory.CreateInstance(serviceProvider);
+        var filterFromFactory = Assert.IsAssignableFrom<AuthorizeFilter>(result);
+
+        var context = new AuthorizationFilterContext(ActionContext, new[] { authorizeFilter });
+        var ex = await Assert.ThrowsAsync<Exception>(() => filterFromFactory.OnAuthorizationAsync(context));
+
+        Assert.Same(authorizeFilter.OnAuthorizeAsyncException, ex);
+    }
+
+    public static TheoryData OverriddenAuthorizeFiltersCreatedWithPolicyOrPolicyProvider
+    {
+        get
+        {
+            return new TheoryData<OverriddenAuthorizeFilter>
+            {
+                new OverriddenAuthorizeFilter(new AuthorizationPolicyBuilder().RequireAssertion(_ => true).Build()),
+                new OverriddenAuthorizeFilter(Mock.Of<IAuthorizationPolicyProvider>(), Enumerable.Empty<IAuthorizeData>()),
+            };
+        }
+    }
+
+    [Theory]
+    [MemberData(nameof(OverriddenAuthorizeFiltersCreatedWithPolicyOrPolicyProvider))]
+    public async Task CreateInstance_AllowsOverridingOnAuthorizeAsyncIfPolicyOrPolicyProviderAreSet(OverriddenAuthorizeFilter authorizeFilter)
+    {
+        authorizeFilter.OnAuthorizeAsyncException = new Exception();
+        var factory = (IFilterFactory)authorizeFilter;
+
+        // The serviceProvider argument shouldn't be necessary when an AuthorizeFilter is constructed with a policy or policy provider.
+        var result = factory.CreateInstance(serviceProvider: null);
+        var filterFromFactory = Assert.IsAssignableFrom<AuthorizeFilter>(result);
+
+        var context = new AuthorizationFilterContext(ActionContext, new[] { authorizeFilter });
+        var ex = await Assert.ThrowsAsync<Exception>(() => filterFromFactory.OnAuthorizationAsync(context));
+
+        Assert.Same(authorizeFilter.OnAuthorizeAsyncException, ex);
     }
 
     [Fact]