From a480378a44890bfa27bac865eda9e2804e23ba64 Mon Sep 17 00:00:00 2001 From: Pranav K Date: Wed, 14 Sep 2016 13:43:56 -0700 Subject: [PATCH] Make AuthorizeFilter constructable * Make AuthorizeFilter constructable Fixes #5253 --- .../Authorization/AuthorizeFilter.cs | 69 +++++++++- .../AuthorizationApplicationModelProvider.cs | 12 +- .../Properties/Resources.Designer.cs | 24 +++- .../Resources.resx | 3 + .../Authorization/AuthorizeFilterTest.cs | 123 +++++++++++++++++- 5 files changed, 213 insertions(+), 18 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Authorization/AuthorizeFilter.cs b/src/Microsoft.AspNetCore.Mvc.Core/Authorization/AuthorizeFilter.cs index 7afdd0d228..0d46ef88b0 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Authorization/AuthorizeFilter.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Authorization/AuthorizeFilter.cs @@ -3,11 +3,14 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Linq; using System.Security.Claims; using System.Threading.Tasks; using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Mvc.Core; using Microsoft.AspNetCore.Mvc.Filters; +using Microsoft.AspNetCore.Mvc.Internal; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Internal; @@ -18,7 +21,7 @@ namespace Microsoft.AspNetCore.Mvc.Authorization /// . MVC recognizes the and adds an instance of /// this filter to the associated action or controller. /// - public class AuthorizeFilter : IAsyncAuthorizationFilter + public class AuthorizeFilter : IAsyncAuthorizationFilter, IFilterFactory { /// /// Initialize a new instance. @@ -30,6 +33,7 @@ namespace Microsoft.AspNetCore.Mvc.Authorization { throw new ArgumentNullException(nameof(policy)); } + Policy = policy; } @@ -39,20 +43,39 @@ namespace Microsoft.AspNetCore.Mvc.Authorization /// The to use to resolve policy names. /// The to combine into an . public AuthorizeFilter(IAuthorizationPolicyProvider policyProvider, IEnumerable authorizeData) + : this(authorizeData) { if (policyProvider == null) { throw new ArgumentNullException(nameof(policyProvider)); } + + PolicyProvider = policyProvider; + } + + /// + /// Initializes a new instance of . + /// + /// The to combine into an . + public AuthorizeFilter(IEnumerable authorizeData) + { if (authorizeData == null) { throw new ArgumentNullException(nameof(authorizeData)); } - PolicyProvider = policyProvider; AuthorizeData = authorizeData; } + /// + /// Initializes a new instance of . + /// + /// The name of the policy to require for authorization. + public AuthorizeFilter(string policy) + : this(new[] { new AuthorizeAttribute(policy) }) + { + } + /// /// The to use to resolve policy names. /// @@ -64,11 +87,16 @@ namespace Microsoft.AspNetCore.Mvc.Authorization public IEnumerable AuthorizeData { get; } /// - /// Gets the authorization policy to be used. If null, the policy will be constructed via - /// AuthorizePolicy.CombineAsync(PolicyProvider, AuthorizeData) + /// Gets the authorization policy to be used. /// + /// + /// Ifnull, the policy will be constructed using + /// . + /// public AuthorizationPolicy Policy { get; private set; } + bool IFilterFactory.IsReusable => true; + /// public virtual async Task OnAuthorizationAsync(AuthorizationFilterContext context) { @@ -77,18 +105,32 @@ namespace Microsoft.AspNetCore.Mvc.Authorization throw new ArgumentNullException(nameof(context)); } - var effectivePolicy = Policy ?? await AuthorizationPolicy.CombineAsync(PolicyProvider, AuthorizeData); + var effectivePolicy = Policy; + if (effectivePolicy == null) + { + if (PolicyProvider == null) + { + throw new InvalidOperationException( + Resources.FormatAuthorizeFilter_AuthorizationPolicyCannotBeCreated( + nameof(AuthorizationPolicy), + nameof(IAuthorizationPolicyProvider))); + } + + effectivePolicy = await AuthorizationPolicy.CombineAsync(PolicyProvider, AuthorizeData); + } + if (effectivePolicy == null) { return; } // Build a ClaimsPrincipal with the Policy's required authentication types - if (effectivePolicy.AuthenticationSchemes != null && effectivePolicy.AuthenticationSchemes.Any()) + if (effectivePolicy.AuthenticationSchemes != null && effectivePolicy.AuthenticationSchemes.Count > 0) { ClaimsPrincipal newPrincipal = null; - foreach (var scheme in effectivePolicy.AuthenticationSchemes) + for (var i = 0; i < effectivePolicy.AuthenticationSchemes.Count; i++) { + var scheme = effectivePolicy.AuthenticationSchemes[i]; var result = await context.HttpContext.Authentication.AuthenticateAsync(scheme); if (result != null) { @@ -118,5 +160,18 @@ namespace Microsoft.AspNetCore.Mvc.Authorization context.Result = new ChallengeResult(effectivePolicy.AuthenticationSchemes.ToArray()); } } + + IFilterMetadata IFilterFactory.CreateInstance(IServiceProvider serviceProvider) + { + if (Policy != null || PolicyProvider != null) + { + // The filter is fully constructed. Use the current instance to authorize. + return this; + } + + Debug.Assert(AuthorizeData != null); + var policyProvider = serviceProvider.GetRequiredService(); + return AuthorizationApplicationModelProvider.GetFilter(policyProvider, AuthorizeData); + } } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/AuthorizationApplicationModelProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/AuthorizationApplicationModelProvider.cs index 2640f2fa62..31115f1320 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/AuthorizationApplicationModelProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/AuthorizationApplicationModelProvider.cs @@ -38,7 +38,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal var controllerModelAuthData = controllerModel.Attributes.OfType().ToArray(); if (controllerModelAuthData.Length > 0) { - controllerModel.Filters.Add(GetFilter(controllerModelAuthData)); + controllerModel.Filters.Add(GetFilter(_policyProvider, controllerModelAuthData)); } foreach (var attribute in controllerModel.Attributes.OfType()) { @@ -50,7 +50,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal var actionModelAuthData = actionModel.Attributes.OfType().ToArray(); if (actionModelAuthData.Length > 0) { - actionModel.Filters.Add(GetFilter(actionModelAuthData)); + actionModel.Filters.Add(GetFilter(_policyProvider, actionModelAuthData)); } foreach (var attribute in actionModel.Attributes.OfType()) @@ -61,18 +61,18 @@ namespace Microsoft.AspNetCore.Mvc.Internal } } - private AuthorizeFilter GetFilter(IEnumerable authData) + public static AuthorizeFilter GetFilter(IAuthorizationPolicyProvider policyProvider, IEnumerable authData) { // The default policy provider will make the same policy for given input, so make it only once. // This will always execute syncronously. - if (_policyProvider.GetType() == typeof(DefaultAuthorizationPolicyProvider)) + if (policyProvider.GetType() == typeof(DefaultAuthorizationPolicyProvider)) { - var policy = AuthorizationPolicy.CombineAsync(_policyProvider, authData).GetAwaiter().GetResult(); + var policy = AuthorizationPolicy.CombineAsync(policyProvider, authData).GetAwaiter().GetResult(); return new AuthorizeFilter(policy); } else { - return new AuthorizeFilter(_policyProvider, authData); + return new AuthorizeFilter(policyProvider, authData); } } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs b/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs index b5cd370b82..0669d0049d 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs @@ -1211,7 +1211,7 @@ namespace Microsoft.AspNetCore.Mvc.Core } /// - /// Having multiple overloads of method '{0}' is not supported. + /// Multiple overloads of method '{0}' are not supported. /// internal static string MiddewareFilter_ConfigureMethodOverload { @@ -1219,7 +1219,7 @@ namespace Microsoft.AspNetCore.Mvc.Core } /// - /// Having multiple overloads of method '{0}' is not supported. + /// Multiple overloads of method '{0}' are not supported. /// internal static string FormatMiddewareFilter_ConfigureMethodOverload(object p0) { @@ -1259,7 +1259,7 @@ namespace Microsoft.AspNetCore.Mvc.Core } /// - /// '{0}' property cannot be null. + /// The '{0}' property cannot be null. /// internal static string MiddlewareFilterBuilder_NullApplicationBuilder { @@ -1267,7 +1267,7 @@ namespace Microsoft.AspNetCore.Mvc.Core } /// - /// '{0}' property cannot be null. + /// The '{0}' property cannot be null. /// internal static string FormatMiddlewareFilterBuilder_NullApplicationBuilder(object p0) { @@ -1306,6 +1306,22 @@ namespace Microsoft.AspNetCore.Mvc.Core return string.Format(CultureInfo.CurrentCulture, GetString("MiddlewareFilter_ServiceResolutionFail"), p0, p1, p2, p3); } + /// + /// An {0} cannot be created without a valid instance of {1}. + /// + internal static string AuthorizeFilter_AuthorizationPolicyCannotBeCreated + { + get { return GetString("AuthorizeFilter_AuthorizationPolicyCannotBeCreated"); } + } + + /// + /// An {0} cannot be created without a valid instance of {1}. + /// + internal static string FormatAuthorizeFilter_AuthorizationPolicyCannotBeCreated(object p0, object p1) + { + return string.Format(CultureInfo.CurrentCulture, GetString("AuthorizeFilter_AuthorizationPolicyCannotBeCreated"), p0, p1); + } + private static string GetString(string name, params string[] formatterNames) { var value = _resourceManager.GetString(name); diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx b/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx index 8bb5bc4735..197ad6f2e2 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx +++ b/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx @@ -370,4 +370,7 @@ Could not resolve a service of type '{0}' for the parameter '{1}' of method '{2}' on type '{3}'. + + An {0} cannot be created without a valid instance of {1}. + \ No newline at end of file diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Authorization/AuthorizeFilterTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Authorization/AuthorizeFilterTest.cs index 51f3d69c64..c5b18795df 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Authorization/AuthorizeFilterTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Authorization/AuthorizeFilterTest.cs @@ -26,6 +26,36 @@ namespace Microsoft.AspNetCore.Mvc.Authorization Assert.True(authorizationContext.HttpContext.User.Identities.Any(i => i.IsAuthenticated)); } + [Fact] + public async Task AuthorizeFilter_CreatedWithAuthorizeData_ThrowsWhenOnAuthorizationAsyncIsCalled() + { + // Arrange + var authorizeFilter = new AuthorizeFilter(new[] { new AuthorizeAttribute() }); + var authorizationContext = GetAuthorizationContext(services => services.AddAuthorization()); + var expected = "An AuthorizationPolicy cannot be created without a valid instance of " + + "IAuthorizationPolicyProvider."; + + // Act & Assert + var ex = await Assert.ThrowsAsync( + () => authorizeFilter.OnAuthorizationAsync(authorizationContext)); + Assert.Equal(expected, ex.Message); + } + + [Fact] + public async Task AuthorizeFilter_CreatedWithPolicy_ThrowsWhenOnAuthorizationAsyncIsCalled() + { + // Arrange + var authorizeFilter = new AuthorizeFilter(new[] { new AuthorizeAttribute() }); + var authorizationContext = GetAuthorizationContext(services => services.AddAuthorization()); + var expected = "An AuthorizationPolicy cannot be created without a valid instance of " + + "IAuthorizationPolicyProvider."; + + // Act & Assert + var ex = await Assert.ThrowsAsync( + () => authorizeFilter.OnAuthorizationAsync(authorizationContext)); + Assert.Equal(expected, ex.Message); + } + [Fact] public async Task AuthorizeFilterCanAuthorizeNonAuthenticatedUser() { @@ -333,7 +363,98 @@ namespace Microsoft.AspNetCore.Mvc.Authorization Assert.NotNull(authorizationContext.Result); } - private Filters.AuthorizationFilterContext GetAuthorizationContext( + [Fact] + public void CreateInstance_ReturnsSelfIfPolicyIsSet() + { + // Arrange + var authorizeFilter = new AuthorizeFilter(new AuthorizationPolicyBuilder() + .RequireAssertion(_ => true) + .Build()); + var factory = (IFilterFactory)authorizeFilter; + + // Act + var result = factory.CreateInstance(new ServiceCollection().BuildServiceProvider()); + + // Assert + Assert.Same(authorizeFilter, result); + } + + [Fact] + public void CreateInstance_ReturnsSelfIfPolicyProviderIsSet() + { + // Arrange + var authorizeFilter = new AuthorizeFilter(new AuthorizationPolicyBuilder() + .RequireAssertion(_ => true) + .Build()); + var factory = (IFilterFactory)authorizeFilter; + + // Act + var result = factory.CreateInstance(new ServiceCollection().BuildServiceProvider()); + + // Assert + Assert.Same(authorizeFilter, result); + } + + public static TheoryData AuthorizeFiltersCreatedWithoutPolicyOrPolicyProvider + { + get + { + return new TheoryData + { + new AuthorizeFilter(new[] { new AuthorizeAttribute()}), + new AuthorizeFilter("some-policy"), + }; + } + } + + [Theory] + [MemberData(nameof(AuthorizeFiltersCreatedWithoutPolicyOrPolicyProvider))] + public void CreateInstance_ReturnsNewFilterIfPolicyAndPolicyProviderAreNotSet(AuthorizeFilter authorizeFilter) + { + // Arrange + var factory = (IFilterFactory)authorizeFilter; + var serviceProvider = new ServiceCollection() + .AddOptions() + .AddAuthorization(options => + { + var policy = new AuthorizationPolicyBuilder() + .RequireAssertion(_ => true) + .Build(); + options.AddPolicy("some-policy", policy); + }) + .BuildServiceProvider(); + + // Act + var result = factory.CreateInstance(serviceProvider); + + // Assert + Assert.NotSame(authorizeFilter, result); + var actual = Assert.IsType(result); + Assert.NotNull(actual.Policy); + } + + [Theory] + [MemberData(nameof(AuthorizeFiltersCreatedWithoutPolicyOrPolicyProvider))] + public void CreateInstance_ReturnsNewFilterIfPolicyAndPolicyProviderAreNotSetAndCustomProviderIsUsed( + AuthorizeFilter authorizeFilter) + { + // Arrange + var factory = (IFilterFactory)authorizeFilter; + var policyProvider = Mock.Of(); + var serviceProvider = new ServiceCollection() + .AddSingleton(policyProvider) + .BuildServiceProvider(); + + // Act + var result = factory.CreateInstance(serviceProvider); + + // Assert + Assert.NotSame(authorizeFilter, result); + var actual = Assert.IsType(result); + Assert.Same(policyProvider, actual.PolicyProvider); + } + + private AuthorizationFilterContext GetAuthorizationContext( Action registerServices, bool anonymous = false) {