From cf929b646c44366bf5e149325d56f25caecdc83c Mon Sep 17 00:00:00 2001 From: David Fowler Date: Sun, 30 Jan 2022 21:00:00 -0800 Subject: [PATCH 01/11] Proposal for how we handle more metadata for auth - The user can specify additional requirements or policies on the endpoint and they will be combined with existing IAuthorizeData. --- .../Core/src/AuthorizationPolicy.cs | 32 +++++++++- .../Core/src/PublicAPI.Unshipped.txt | 1 + .../Policy/src/AuthorizationMiddleware.cs | 7 ++- .../test/AuthorizationMiddlewareTests.cs | 58 +++++++++++++++++++ .../test/AuthorizationPolicyFacts.cs | 23 ++++++++ 5 files changed, 118 insertions(+), 3 deletions(-) diff --git a/src/Security/Authorization/Core/src/AuthorizationPolicy.cs b/src/Security/Authorization/Core/src/AuthorizationPolicy.cs index afb62f810d16..294a0ba2a389 100644 --- a/src/Security/Authorization/Core/src/AuthorizationPolicy.cs +++ b/src/Security/Authorization/Core/src/AuthorizationPolicy.cs @@ -104,11 +104,16 @@ public static AuthorizationPolicy Combine(IEnumerable polic /// /// A which provides the policies to combine. /// A collection of authorization data used to apply authorization to a resource. + /// A collection of policies to combine. + /// A collection of s to add to the auth policy. /// /// A new which represents the combination of the /// authorization policies provided by the specified . /// - public static async Task CombineAsync(IAuthorizationPolicyProvider policyProvider, IEnumerable authorizeData) + public static async Task CombineAsync(IAuthorizationPolicyProvider policyProvider, + IEnumerable authorizeData, + IEnumerable? policies = null, + IEnumerable? requirements = null) { if (policyProvider == null) { @@ -120,6 +125,9 @@ public static AuthorizationPolicy Combine(IEnumerable polic throw new ArgumentNullException(nameof(authorizeData)); } + var anyPolicies = policies is not null && policies.Any(); + var anyRequirements = requirements is not null && requirements.Any(); + // Avoid allocating enumerator if the data is known to be empty var skipEnumeratingData = false; if (authorizeData is IList dataList) @@ -137,7 +145,7 @@ public static AuthorizationPolicy Combine(IEnumerable polic policyBuilder = new AuthorizationPolicyBuilder(); } - var useDefaultPolicy = true; + var useDefaultPolicy = !(anyPolicies || anyRequirements); if (!string.IsNullOrWhiteSpace(authorizeDatum.Policy)) { var policy = await policyProvider.GetPolicyAsync(authorizeDatum.Policy); @@ -176,6 +184,26 @@ public static AuthorizationPolicy Combine(IEnumerable polic } } + if (anyPolicies) + { + policyBuilder ??= new(); + + foreach (var policy in policies) + { + policyBuilder.Combine(policy); + } + } + + if (anyRequirements) + { + policyBuilder ??= new(); + + foreach (var requirement in requirements) + { + policyBuilder.Requirements.Add(requirement); + } + } + // If we have no policy by now, use the fallback policy if we have one if (policyBuilder == null) { diff --git a/src/Security/Authorization/Core/src/PublicAPI.Unshipped.txt b/src/Security/Authorization/Core/src/PublicAPI.Unshipped.txt index 7dc5c58110bf..c73b2f6624f9 100644 --- a/src/Security/Authorization/Core/src/PublicAPI.Unshipped.txt +++ b/src/Security/Authorization/Core/src/PublicAPI.Unshipped.txt @@ -1 +1,2 @@ #nullable enable +static Microsoft.AspNetCore.Authorization.AuthorizationPolicy.CombineAsync(Microsoft.AspNetCore.Authorization.IAuthorizationPolicyProvider! policyProvider, System.Collections.Generic.IEnumerable! authorizeData, System.Collections.Generic.IEnumerable? policies = null, System.Collections.Generic.IEnumerable? requirements = null) -> System.Threading.Tasks.Task! diff --git a/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs b/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs index 516f7f5d0357..ebd57a4570e2 100644 --- a/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs +++ b/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs @@ -57,7 +57,12 @@ public async Task Invoke(HttpContext context) // IMPORTANT: Changes to authorization logic should be mirrored in MVC's AuthorizeFilter var authorizeData = endpoint?.Metadata.GetOrderedMetadata() ?? Array.Empty(); - var policy = await AuthorizationPolicy.CombineAsync(_policyProvider, authorizeData); + + var policies = endpoint?.Metadata.GetOrderedMetadata() ?? Array.Empty(); + var reqirements = endpoint?.Metadata.GetOrderedMetadata() ?? Array.Empty(); + + var policy = await AuthorizationPolicy.CombineAsync(_policyProvider, authorizeData, policies, reqirements); + if (policy == null) { await _next(context); diff --git a/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs b/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs index d6548f532191..10ae54725eaf 100644 --- a/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs +++ b/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs @@ -207,6 +207,64 @@ public async Task OnAuthorizationAsync_WillCallPolicyProvider() Assert.Equal(3, next.CalledCount); } + [Fact] + public async Task CanApplyPolicyDirectlyToEndpoint() + { + // Arrange + var calledPolicy = false; + var policy = new AuthorizationPolicyBuilder().RequireAssertion(_ => + { + calledPolicy = true; + return true; + }).Build(); + + var policyProvider = new Mock(); + policyProvider.Setup(p => p.GetDefaultPolicyAsync()).ReturnsAsync(new AuthorizationPolicyBuilder().RequireAuthenticatedUser().Build()); + var next = new TestRequestDelegate(); + var middleware = CreateMiddleware(next.Invoke, policyProvider.Object); + var context = GetHttpContext(anonymous: false, endpoint: CreateEndpoint(new AuthorizeAttribute(), policy)); + + // Act & Assert + await middleware.Invoke(context); + Assert.True(calledPolicy); + } + + [Fact] + public async Task CanApplyAdditonalRequirementsToEndpoint() + { + // Arrange + var policyProvider = new Mock(); + policyProvider.Setup(p => p.GetDefaultPolicyAsync()).ReturnsAsync(new AuthorizationPolicyBuilder().RequireAuthenticatedUser().Build()); + var next = new TestRequestDelegate(); + var middleware = CreateMiddleware(next.Invoke, policyProvider.Object); + var context = GetHttpContext(anonymous: false, registerServices: services => + { + services.AddSingleton(); + }, + endpoint: CreateEndpoint(new AuthorizeAttribute(), new CustomRequirement("This"))); + + // Act & Assert + await middleware.Invoke(context); + } + + class CustomAuthHandler : AuthorizationHandler + { + protected override Task HandleRequirementAsync(AuthorizationHandlerContext context, CustomRequirement requirement) + { + return Task.CompletedTask; + } + } + + class CustomRequirement : IAuthorizationRequirement + { + public string Data { get; init; } + + public CustomRequirement(string data) + { + Data = data; + } + } + [Fact] public async Task Invoke_ValidClaimShouldNotFail() { diff --git a/src/Security/Authorization/test/AuthorizationPolicyFacts.cs b/src/Security/Authorization/test/AuthorizationPolicyFacts.cs index 58af80706b0a..9a3a4d78a89a 100644 --- a/src/Security/Authorization/test/AuthorizationPolicyFacts.cs +++ b/src/Security/Authorization/test/AuthorizationPolicyFacts.cs @@ -43,6 +43,29 @@ public async Task CanCombineAuthorizeAttributes() Assert.Single(combined.Requirements.OfType()); } + [Fact] + public async Task CanReplaceDefaultPolicyDirectly() + { + // Arrange + var attributes = new AuthorizeAttribute[] { + new AuthorizeAttribute(), + new AuthorizeAttribute(), + }; + + var policies = new[] { new AuthorizationPolicyBuilder().RequireAssertion(_ => true).Build() }; + + var options = new AuthorizationOptions(); + + var provider = new DefaultAuthorizationPolicyProvider(Options.Create(options)); + + // Act + var combined = await AuthorizationPolicy.CombineAsync(provider, attributes, policies); + + // Assert + Assert.Equal(1, combined.Requirements.Count); + Assert.Empty(combined.Requirements.OfType()); + } + [Fact] public async Task CanReplaceDefaultPolicy() { From 045f9f8b409c67b85f4e2bddfd7d6f665fb85e9a Mon Sep 17 00:00:00 2001 From: David Fowler Date: Mon, 31 Jan 2022 08:24:22 -0800 Subject: [PATCH 02/11] APIs... --- .../Core/src/AuthorizationPolicy.cs | 23 +++++++++++++++---- .../Core/src/PublicAPI.Unshipped.txt | 2 +- .../test/AuthorizationPolicyFacts.cs | 2 +- 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/src/Security/Authorization/Core/src/AuthorizationPolicy.cs b/src/Security/Authorization/Core/src/AuthorizationPolicy.cs index 294a0ba2a389..1942f7fca2eb 100644 --- a/src/Security/Authorization/Core/src/AuthorizationPolicy.cs +++ b/src/Security/Authorization/Core/src/AuthorizationPolicy.cs @@ -98,6 +98,21 @@ public static AuthorizationPolicy Combine(IEnumerable polic return builder.Build(); } + /// + /// Combines the provided by the specified + /// . + /// + /// A which provides the policies to combine. + /// A collection of authorization data used to apply authorization to a resource. + /// + /// A new which represents the combination of the + /// authorization policies provided by the specified . + /// + public static Task CombineAsync(IAuthorizationPolicyProvider policyProvider, + IEnumerable authorizeData) => CombineAsync(policyProvider, authorizeData, + Enumerable.Empty(), + Enumerable.Empty()); + /// /// Combines the provided by the specified /// . @@ -112,8 +127,8 @@ public static AuthorizationPolicy Combine(IEnumerable polic /// public static async Task CombineAsync(IAuthorizationPolicyProvider policyProvider, IEnumerable authorizeData, - IEnumerable? policies = null, - IEnumerable? requirements = null) + IEnumerable policies, + IEnumerable requirements) { if (policyProvider == null) { @@ -125,8 +140,8 @@ public static AuthorizationPolicy Combine(IEnumerable polic throw new ArgumentNullException(nameof(authorizeData)); } - var anyPolicies = policies is not null && policies.Any(); - var anyRequirements = requirements is not null && requirements.Any(); + var anyPolicies = policies.Any(); + var anyRequirements = requirements.Any(); // Avoid allocating enumerator if the data is known to be empty var skipEnumeratingData = false; diff --git a/src/Security/Authorization/Core/src/PublicAPI.Unshipped.txt b/src/Security/Authorization/Core/src/PublicAPI.Unshipped.txt index c73b2f6624f9..6fe5d6ca17f8 100644 --- a/src/Security/Authorization/Core/src/PublicAPI.Unshipped.txt +++ b/src/Security/Authorization/Core/src/PublicAPI.Unshipped.txt @@ -1,2 +1,2 @@ #nullable enable -static Microsoft.AspNetCore.Authorization.AuthorizationPolicy.CombineAsync(Microsoft.AspNetCore.Authorization.IAuthorizationPolicyProvider! policyProvider, System.Collections.Generic.IEnumerable! authorizeData, System.Collections.Generic.IEnumerable? policies = null, System.Collections.Generic.IEnumerable? requirements = null) -> System.Threading.Tasks.Task! +static Microsoft.AspNetCore.Authorization.AuthorizationPolicy.CombineAsync(Microsoft.AspNetCore.Authorization.IAuthorizationPolicyProvider! policyProvider, System.Collections.Generic.IEnumerable! authorizeData, System.Collections.Generic.IEnumerable! policies, System.Collections.Generic.IEnumerable! requirements) -> System.Threading.Tasks.Task! diff --git a/src/Security/Authorization/test/AuthorizationPolicyFacts.cs b/src/Security/Authorization/test/AuthorizationPolicyFacts.cs index 9a3a4d78a89a..3fb54f893f4d 100644 --- a/src/Security/Authorization/test/AuthorizationPolicyFacts.cs +++ b/src/Security/Authorization/test/AuthorizationPolicyFacts.cs @@ -59,7 +59,7 @@ public async Task CanReplaceDefaultPolicyDirectly() var provider = new DefaultAuthorizationPolicyProvider(Options.Create(options)); // Act - var combined = await AuthorizationPolicy.CombineAsync(provider, attributes, policies); + var combined = await AuthorizationPolicy.CombineAsync(provider, attributes, policies, Enumerable.Empty()); // Assert Assert.Equal(1, combined.Requirements.Count); From 585f30e30f0b52e5b962d77b21e547d42b91cfe2 Mon Sep 17 00:00:00 2001 From: Hao Kung Date: Tue, 12 Apr 2022 11:27:43 -0700 Subject: [PATCH 03/11] Remove ability to add requirements --- .../Core/src/AuthorizationPolicy.cs | 20 +++---------------- .../Core/src/PublicAPI.Unshipped.txt | 2 +- .../Policy/src/AuthorizationMiddleware.cs | 3 +-- .../test/AuthorizationMiddlewareTests.cs | 18 ----------------- .../test/AuthorizationPolicyFacts.cs | 2 +- 5 files changed, 6 insertions(+), 39 deletions(-) diff --git a/src/Security/Authorization/Core/src/AuthorizationPolicy.cs b/src/Security/Authorization/Core/src/AuthorizationPolicy.cs index beca9aac2082..044cd96bbad3 100644 --- a/src/Security/Authorization/Core/src/AuthorizationPolicy.cs +++ b/src/Security/Authorization/Core/src/AuthorizationPolicy.cs @@ -110,8 +110,7 @@ public static AuthorizationPolicy Combine(IEnumerable polic /// public static Task CombineAsync(IAuthorizationPolicyProvider policyProvider, IEnumerable authorizeData) => CombineAsync(policyProvider, authorizeData, - Enumerable.Empty(), - Enumerable.Empty()); + Enumerable.Empty()); /// /// Combines the provided by the specified @@ -120,15 +119,13 @@ public static AuthorizationPolicy Combine(IEnumerable polic /// A which provides the policies to combine. /// A collection of authorization data used to apply authorization to a resource. /// A collection of policies to combine. - /// A collection of s to add to the auth policy. /// /// A new which represents the combination of the /// authorization policies provided by the specified . /// public static async Task CombineAsync(IAuthorizationPolicyProvider policyProvider, IEnumerable authorizeData, - IEnumerable policies, - IEnumerable requirements) + IEnumerable policies) { if (policyProvider == null) { @@ -141,7 +138,6 @@ public static AuthorizationPolicy Combine(IEnumerable polic } var anyPolicies = policies.Any(); - var anyRequirements = requirements.Any(); // Avoid allocating enumerator if the data is known to be empty var skipEnumeratingData = false; @@ -160,7 +156,7 @@ public static AuthorizationPolicy Combine(IEnumerable polic policyBuilder = new AuthorizationPolicyBuilder(); } - var useDefaultPolicy = !(anyPolicies || anyRequirements); + var useDefaultPolicy = !(anyPolicies); if (!string.IsNullOrWhiteSpace(authorizeDatum.Policy)) { var policy = await policyProvider.GetPolicyAsync(authorizeDatum.Policy).ConfigureAwait(false); @@ -209,16 +205,6 @@ public static AuthorizationPolicy Combine(IEnumerable polic } } - if (anyRequirements) - { - policyBuilder ??= new(); - - foreach (var requirement in requirements) - { - policyBuilder.Requirements.Add(requirement); - } - } - // If we have no policy by now, use the fallback policy if we have one if (policyBuilder == null) { diff --git a/src/Security/Authorization/Core/src/PublicAPI.Unshipped.txt b/src/Security/Authorization/Core/src/PublicAPI.Unshipped.txt index b4e7ce93916e..3ae82ce3cead 100644 --- a/src/Security/Authorization/Core/src/PublicAPI.Unshipped.txt +++ b/src/Security/Authorization/Core/src/PublicAPI.Unshipped.txt @@ -3,4 +3,4 @@ *REMOVED*~Microsoft.AspNetCore.Authorization.DefaultAuthorizationService.DefaultAuthorizationService(Microsoft.AspNetCore.Authorization.IAuthorizationPolicyProvider! policyProvider, Microsoft.AspNetCore.Authorization.IAuthorizationHandlerProvider! handlers, Microsoft.Extensions.Logging.ILogger! logger, Microsoft.AspNetCore.Authorization.IAuthorizationHandlerContextFactory! contextFactory, Microsoft.AspNetCore.Authorization.IAuthorizationEvaluator! evaluator, Microsoft.Extensions.Options.IOptions! options) -> void Microsoft.AspNetCore.Authorization.DefaultAuthorizationPolicyProvider.DefaultAuthorizationPolicyProvider(Microsoft.Extensions.Options.IOptions! options) -> void Microsoft.AspNetCore.Authorization.DefaultAuthorizationService.DefaultAuthorizationService(Microsoft.AspNetCore.Authorization.IAuthorizationPolicyProvider! policyProvider, Microsoft.AspNetCore.Authorization.IAuthorizationHandlerProvider! handlers, Microsoft.Extensions.Logging.ILogger! logger, Microsoft.AspNetCore.Authorization.IAuthorizationHandlerContextFactory! contextFactory, Microsoft.AspNetCore.Authorization.IAuthorizationEvaluator! evaluator, Microsoft.Extensions.Options.IOptions! options) -> void -static Microsoft.AspNetCore.Authorization.AuthorizationPolicy.CombineAsync(Microsoft.AspNetCore.Authorization.IAuthorizationPolicyProvider! policyProvider, System.Collections.Generic.IEnumerable! authorizeData, System.Collections.Generic.IEnumerable! policies, System.Collections.Generic.IEnumerable! requirements) -> System.Threading.Tasks.Task! +static Microsoft.AspNetCore.Authorization.AuthorizationPolicy.CombineAsync(Microsoft.AspNetCore.Authorization.IAuthorizationPolicyProvider! policyProvider, System.Collections.Generic.IEnumerable! authorizeData, System.Collections.Generic.IEnumerable! policies) -> System.Threading.Tasks.Task! diff --git a/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs b/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs index ebd57a4570e2..0a58a00a37e0 100644 --- a/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs +++ b/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs @@ -59,9 +59,8 @@ public async Task Invoke(HttpContext context) var authorizeData = endpoint?.Metadata.GetOrderedMetadata() ?? Array.Empty(); var policies = endpoint?.Metadata.GetOrderedMetadata() ?? Array.Empty(); - var reqirements = endpoint?.Metadata.GetOrderedMetadata() ?? Array.Empty(); - var policy = await AuthorizationPolicy.CombineAsync(_policyProvider, authorizeData, policies, reqirements); + var policy = await AuthorizationPolicy.CombineAsync(_policyProvider, authorizeData, policies); if (policy == null) { diff --git a/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs b/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs index 10ae54725eaf..18de83166414 100644 --- a/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs +++ b/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs @@ -229,24 +229,6 @@ public async Task CanApplyPolicyDirectlyToEndpoint() Assert.True(calledPolicy); } - [Fact] - public async Task CanApplyAdditonalRequirementsToEndpoint() - { - // Arrange - var policyProvider = new Mock(); - policyProvider.Setup(p => p.GetDefaultPolicyAsync()).ReturnsAsync(new AuthorizationPolicyBuilder().RequireAuthenticatedUser().Build()); - var next = new TestRequestDelegate(); - var middleware = CreateMiddleware(next.Invoke, policyProvider.Object); - var context = GetHttpContext(anonymous: false, registerServices: services => - { - services.AddSingleton(); - }, - endpoint: CreateEndpoint(new AuthorizeAttribute(), new CustomRequirement("This"))); - - // Act & Assert - await middleware.Invoke(context); - } - class CustomAuthHandler : AuthorizationHandler { protected override Task HandleRequirementAsync(AuthorizationHandlerContext context, CustomRequirement requirement) diff --git a/src/Security/Authorization/test/AuthorizationPolicyFacts.cs b/src/Security/Authorization/test/AuthorizationPolicyFacts.cs index 3fb54f893f4d..9a3a4d78a89a 100644 --- a/src/Security/Authorization/test/AuthorizationPolicyFacts.cs +++ b/src/Security/Authorization/test/AuthorizationPolicyFacts.cs @@ -59,7 +59,7 @@ public async Task CanReplaceDefaultPolicyDirectly() var provider = new DefaultAuthorizationPolicyProvider(Options.Create(options)); // Act - var combined = await AuthorizationPolicy.CombineAsync(provider, attributes, policies, Enumerable.Empty()); + var combined = await AuthorizationPolicy.CombineAsync(provider, attributes, policies); // Assert Assert.Equal(1, combined.Requirements.Count); From 9c03db08f4d372f30a155a2326d7edbc410e6814 Mon Sep 17 00:00:00 2001 From: Hao Kung Date: Tue, 12 Apr 2022 11:30:54 -0700 Subject: [PATCH 04/11] Cleanup --- .../Authorization/test/AuthorizationMiddlewareTests.cs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs b/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs index 18de83166414..2543466726f0 100644 --- a/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs +++ b/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs @@ -229,14 +229,6 @@ public async Task CanApplyPolicyDirectlyToEndpoint() Assert.True(calledPolicy); } - class CustomAuthHandler : AuthorizationHandler - { - protected override Task HandleRequirementAsync(AuthorizationHandlerContext context, CustomRequirement requirement) - { - return Task.CompletedTask; - } - } - class CustomRequirement : IAuthorizationRequirement { public string Data { get; init; } From b512ca8ccb923119dcdbba6c8ac5fa68577b6581 Mon Sep 17 00:00:00 2001 From: Hao Kung Date: Tue, 12 Apr 2022 14:07:44 -0700 Subject: [PATCH 05/11] New RequireAuthorization overloads --- ...tionEndpointConventionBuilderExtensions.cs | 61 +++++++++++++++++++ .../Policy/src/PublicAPI.Unshipped.txt | 2 + ...ndpointConventionBuilderExtensionsTests.cs | 50 +++++++++++++++ 3 files changed, 113 insertions(+) diff --git a/src/Security/Authorization/Policy/src/AuthorizationEndpointConventionBuilderExtensions.cs b/src/Security/Authorization/Policy/src/AuthorizationEndpointConventionBuilderExtensions.cs index c3a957a78349..4f8b37f32976 100644 --- a/src/Security/Authorization/Policy/src/AuthorizationEndpointConventionBuilderExtensions.cs +++ b/src/Security/Authorization/Policy/src/AuthorizationEndpointConventionBuilderExtensions.cs @@ -79,6 +79,66 @@ public static TBuilder RequireAuthorization(this TBuilder builder, par return builder; } + /// + /// Adds an authorization policy to the endpoint(s). + /// + /// The endpoint convention builder. + /// The policy. + /// The original convention builder parameter. + public static TBuilder RequireAuthorization(this TBuilder builder, AuthorizationPolicy policy) + where TBuilder : IEndpointConventionBuilder + { + if (builder == null) + { + throw new ArgumentNullException(nameof(builder)); + } + + if (policy == null) + { + throw new ArgumentNullException(nameof(policy)); + } + + // REVIEW: should we check if there already is any authorize attributes first? + builder.Add(endpointBuilder => + { + endpointBuilder.Metadata.Add(new AuthorizeAttribute()); + endpointBuilder.Metadata.Add(policy); + }); + return builder; + } + + /// + /// Adds an new authorization policy configured by a callback to the endpoint(s). + /// + /// + /// The endpoint convention builder. + /// The callback used to configure the policy. + /// The original convention builder parameter. + public static TBuilder RequireAuthorization(this TBuilder builder, Action configurePolicy) + where TBuilder : IEndpointConventionBuilder + { + if (builder == null) + { + throw new ArgumentNullException(nameof(builder)); + } + + if (configurePolicy == null) + { + throw new ArgumentNullException(nameof(configurePolicy)); + } + + var policyBuilder = new AuthorizationPolicyBuilder(); + configurePolicy(policyBuilder); + + // REVIEW: should we check if there already is any authorize attributes first? + builder.Add(endpointBuilder => + { + endpointBuilder.Metadata.Add(new AuthorizeAttribute()); + endpointBuilder.Metadata.Add(policyBuilder.Build()); + }); + return builder; + } + /// /// Allows anonymous access to the endpoint by adding to the endpoint metadata. This will bypass /// all authorization checks for the endpoint including the default authorization policy and fallback authorization policy. @@ -105,4 +165,5 @@ private static void RequireAuthorizationCore(TBuilder builder, IEnumer } }); } + } diff --git a/src/Security/Authorization/Policy/src/PublicAPI.Unshipped.txt b/src/Security/Authorization/Policy/src/PublicAPI.Unshipped.txt index 7dc5c58110bf..9c11d1e7b2d6 100644 --- a/src/Security/Authorization/Policy/src/PublicAPI.Unshipped.txt +++ b/src/Security/Authorization/Policy/src/PublicAPI.Unshipped.txt @@ -1 +1,3 @@ #nullable enable +static Microsoft.AspNetCore.Builder.AuthorizationEndpointConventionBuilderExtensions.RequireAuthorization(this TBuilder builder, Microsoft.AspNetCore.Authorization.AuthorizationPolicy! policy) -> TBuilder +static Microsoft.AspNetCore.Builder.AuthorizationEndpointConventionBuilderExtensions.RequireAuthorization(this TBuilder builder, System.Action! configurePolicy) -> TBuilder diff --git a/src/Security/Authorization/test/AuthorizationEndpointConventionBuilderExtensionsTests.cs b/src/Security/Authorization/test/AuthorizationEndpointConventionBuilderExtensionsTests.cs index c2590efe913e..8ed5232f644e 100644 --- a/src/Security/Authorization/test/AuthorizationEndpointConventionBuilderExtensionsTests.cs +++ b/src/Security/Authorization/test/AuthorizationEndpointConventionBuilderExtensionsTests.cs @@ -117,6 +117,56 @@ public void RequireAuthorization_ChainedCall() Assert.True(chainedBuilder.TestProperty); } + [Fact] + public void RequireAuthorization_Policy() + { + // Arrange + var builder = new TestEndpointConventionBuilder(); + var policy = new AuthorizationPolicyBuilder().RequireAssertion(_ => true).Build(); + + // Act + builder.RequireAuthorization(policy); + + // Assert + var convention = Assert.Single(builder.Conventions); + + var endpointModel = new RouteEndpointBuilder((context) => Task.CompletedTask, RoutePatternFactory.Parse("/"), 0); + convention(endpointModel); + + Assert.Equal(2, endpointModel.Metadata.Count); + var authMetadata = Assert.IsAssignableFrom(endpointModel.Metadata[0]); + Assert.Null(authMetadata.Policy); + + Assert.Equal(policy, endpointModel.Metadata[1]); + } + + [Fact] + public void RequireAuthorization_PolicyCallback() + { + // Arrange + var builder = new TestEndpointConventionBuilder(); + var requirement = new TestRequirement(); + + // Act + builder.RequireAuthorization(policyBuilder => policyBuilder.Requirements.Add(requirement)); + + // Assert + var convention = Assert.Single(builder.Conventions); + + var endpointModel = new RouteEndpointBuilder((context) => Task.CompletedTask, RoutePatternFactory.Parse("/"), 0); + convention(endpointModel); + + Assert.Equal(2, endpointModel.Metadata.Count); + var authMetadata = Assert.IsAssignableFrom(endpointModel.Metadata[0]); + Assert.Null(authMetadata.Policy); + + var policy = Assert.IsAssignableFrom(endpointModel.Metadata[1]); + Assert.Equal(1, policy.Requirements.Count); + Assert.Equal(requirement, policy.Requirements[0]); + } + + class TestRequirement : IAuthorizationRequirement { } + [Fact] public void AllowAnonymous_Default() { From 3a65292d9354a2663a00ea5399127a5b74c86e20 Mon Sep 17 00:00:00 2001 From: Hao Kung Date: Wed, 13 Apr 2022 09:39:53 -0700 Subject: [PATCH 06/11] Only add authorize if needed --- ...tionEndpointConventionBuilderExtensions.cs | 29 ++++++----- ...ndpointConventionBuilderExtensionsTests.cs | 50 +++++++++++++++++++ 2 files changed, 66 insertions(+), 13 deletions(-) diff --git a/src/Security/Authorization/Policy/src/AuthorizationEndpointConventionBuilderExtensions.cs b/src/Security/Authorization/Policy/src/AuthorizationEndpointConventionBuilderExtensions.cs index 4f8b37f32976..6551f048fbba 100644 --- a/src/Security/Authorization/Policy/src/AuthorizationEndpointConventionBuilderExtensions.cs +++ b/src/Security/Authorization/Policy/src/AuthorizationEndpointConventionBuilderExtensions.cs @@ -98,12 +98,7 @@ public static TBuilder RequireAuthorization(this TBuilder builder, Aut throw new ArgumentNullException(nameof(policy)); } - // REVIEW: should we check if there already is any authorize attributes first? - builder.Add(endpointBuilder => - { - endpointBuilder.Metadata.Add(new AuthorizeAttribute()); - endpointBuilder.Metadata.Add(policy); - }); + RequirePolicyCore(builder, policy); return builder; } @@ -129,13 +124,7 @@ public static TBuilder RequireAuthorization(this TBuilder builder, Act var policyBuilder = new AuthorizationPolicyBuilder(); configurePolicy(policyBuilder); - - // REVIEW: should we check if there already is any authorize attributes first? - builder.Add(endpointBuilder => - { - endpointBuilder.Metadata.Add(new AuthorizeAttribute()); - endpointBuilder.Metadata.Add(policyBuilder.Build()); - }); + RequirePolicyCore(builder, policyBuilder.Build()); return builder; } @@ -154,6 +143,20 @@ public static TBuilder AllowAnonymous(this TBuilder builder) where TBu return builder; } + private static void RequirePolicyCore(TBuilder builder, AuthorizationPolicy policy) + where TBuilder : IEndpointConventionBuilder + { + builder.Add(endpointBuilder => + { + // Only add an authorize attribute if there isn't one + if (!endpointBuilder.Metadata.Any(meta => meta is IAuthorizeData)) + { + endpointBuilder.Metadata.Add(new AuthorizeAttribute()); + } + endpointBuilder.Metadata.Add(policy); + }); + } + private static void RequireAuthorizationCore(TBuilder builder, IEnumerable authorizeData) where TBuilder : IEndpointConventionBuilder { diff --git a/src/Security/Authorization/test/AuthorizationEndpointConventionBuilderExtensionsTests.cs b/src/Security/Authorization/test/AuthorizationEndpointConventionBuilderExtensionsTests.cs index 8ed5232f644e..8cc43a7d2f7f 100644 --- a/src/Security/Authorization/test/AuthorizationEndpointConventionBuilderExtensionsTests.cs +++ b/src/Security/Authorization/test/AuthorizationEndpointConventionBuilderExtensionsTests.cs @@ -165,6 +165,56 @@ public void RequireAuthorization_PolicyCallback() Assert.Equal(requirement, policy.Requirements[0]); } + [Fact] + public void RequireAuthorization_PolicyCallbackWithAuthorize() + { + // Arrange + var builder = new TestEndpointConventionBuilder(); + var authorize = new AuthorizeAttribute(); + var requirement = new TestRequirement(); + + // Act + builder.RequireAuthorization(policyBuilder => policyBuilder.Requirements.Add(requirement)); + + // Assert + var convention = Assert.Single(builder.Conventions); + + var endpointModel = new RouteEndpointBuilder((context) => Task.CompletedTask, RoutePatternFactory.Parse("/"), 0); + endpointModel.Metadata.Add(authorize); + convention(endpointModel); + + // Confirm that we don't add another authorize if one already exists + Assert.Equal(2, endpointModel.Metadata.Count); + Assert.Equal(authorize, endpointModel.Metadata[0]); + var policy = Assert.IsAssignableFrom(endpointModel.Metadata[1]); + Assert.Equal(1, policy.Requirements.Count); + Assert.Equal(requirement, policy.Requirements[0]); + } + + [Fact] + public void RequireAuthorization_PolicyWithAuthorize() + { + // Arrange + var builder = new TestEndpointConventionBuilder(); + var policy = new AuthorizationPolicyBuilder().RequireAssertion(_ => true).Build(); + var authorize = new AuthorizeAttribute(); + + // Act + builder.RequireAuthorization(policy); + + // Assert + var convention = Assert.Single(builder.Conventions); + + var endpointModel = new RouteEndpointBuilder((context) => Task.CompletedTask, RoutePatternFactory.Parse("/"), 0); + endpointModel.Metadata.Add(authorize); + convention(endpointModel); + + // Confirm that we don't add another authorize if one already exists + Assert.Equal(2, endpointModel.Metadata.Count); + Assert.Equal(authorize, endpointModel.Metadata[0]); + Assert.Equal(policy, endpointModel.Metadata[1]); + } + class TestRequirement : IAuthorizationRequirement { } [Fact] From a528db330bfa064ac3e89126718ebaac15b9be2e Mon Sep 17 00:00:00 2001 From: Hao Kung Date: Wed, 13 Apr 2022 11:12:07 -0700 Subject: [PATCH 07/11] Add signalR metadata tests --- .../test/AuthorizationMiddlewareTests.cs | 10 ---- .../test/MapConnectionHandlerTests.cs | 58 ++++++++++++++++--- .../server/SignalR/test/MapSignalRTests.cs | 47 +++++++++++++++ 3 files changed, 97 insertions(+), 18 deletions(-) diff --git a/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs b/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs index 2543466726f0..15ee3bc641c9 100644 --- a/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs +++ b/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs @@ -229,16 +229,6 @@ public async Task CanApplyPolicyDirectlyToEndpoint() Assert.True(calledPolicy); } - class CustomRequirement : IAuthorizationRequirement - { - public string Data { get; init; } - - public CustomRequirement(string data) - { - Data = data; - } - } - [Fact] public async Task Invoke_ValidClaimShouldNotFail() { diff --git a/src/SignalR/common/Http.Connections/test/MapConnectionHandlerTests.cs b/src/SignalR/common/Http.Connections/test/MapConnectionHandlerTests.cs index 8c21a7e23313..9f6aead906a3 100644 --- a/src/SignalR/common/Http.Connections/test/MapConnectionHandlerTests.cs +++ b/src/SignalR/common/Http.Connections/test/MapConnectionHandlerTests.cs @@ -1,12 +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.Linq; using System.Net.WebSockets; -using System.Reflection; -using System.Threading; -using System.Threading.Tasks; using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Connections; @@ -21,7 +16,6 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Logging; -using Xunit; using Xunit.Abstractions; namespace Microsoft.AspNetCore.Http.Connections.Tests; @@ -35,6 +29,46 @@ public MapConnectionHandlerTests(ITestOutputHelper output) _output = output; } + [Fact] + public void MapConnectionHandlerFindsMetadataPolicyOnEndPoint() + { + var authCount = 0; + var policy1 = new AuthorizationPolicyBuilder().RequireAssertion(_ => true).Build(); + var req = new TestRequirement(); + using (var host = BuildWebHost("/auth", + options => authCount += options.AuthorizationData.Count, + endpoints => endpoints.RequireAuthorization(policy1).RequireAuthorization(pb => pb.Requirements.Add(req)))) + { + host.Start(); + + var dataSource = host.Services.GetRequiredService(); + // We register 2 endpoints (/negotiate and /) + Assert.Collection(dataSource.Endpoints, + endpoint => + { + Assert.Equal("/auth/negotiate", endpoint.DisplayName); + Assert.Single(endpoint.Metadata.GetOrderedMetadata()); + var policies = endpoint.Metadata.GetOrderedMetadata(); + Assert.Equal(2, policies.Count); + Assert.Equal(policy1, policies[0]); + Assert.Equal(1, policies[1].Requirements.Count); + Assert.Equal(req, policies[1].Requirements.First()); + }, + endpoint => + { + Assert.Equal("/auth", endpoint.DisplayName); + Assert.Single(endpoint.Metadata.GetOrderedMetadata()); + var policies = endpoint.Metadata.GetOrderedMetadata(); + Assert.Equal(2, policies.Count); + Assert.Equal(policy1, policies[0]); + Assert.Equal(1, policies[1].Requirements.Count); + Assert.Equal(req, policies[1].Requirements.First()); + }); + } + + Assert.Equal(0, authCount); + } + [Fact] public void MapConnectionHandlerFindsAuthAttributeOnEndPoint() { @@ -421,6 +455,10 @@ public override Task OnConnectedAsync(ConnectionContext connection) } } + private class TestRequirement : IAuthorizationRequirement + { + } + private IHost BuildWebHost(Action configure) { return new HostBuilder() @@ -442,7 +480,7 @@ private IHost BuildWebHost(Action configure) .Build(); } - private IHost BuildWebHost(string path, Action configureOptions) where TConnectionHandler : ConnectionHandler + private IHost BuildWebHost(string path, Action configureOptions, Action configureEndpoints = null) where TConnectionHandler : ConnectionHandler { return new HostBuilder() .ConfigureWebHost(webHostBuilder => @@ -459,7 +497,11 @@ private IHost BuildWebHost(string path, Action { - routes.MapConnectionHandler(path, configureOptions); + var builder = routes.MapConnectionHandler(path, configureOptions); + if (configureEndpoints != null) + { + configureEndpoints(builder); + } }); }) .ConfigureLogging(factory => diff --git a/src/SignalR/server/SignalR/test/MapSignalRTests.cs b/src/SignalR/server/SignalR/test/MapSignalRTests.cs index 4eaaedc43f90..7eeadd7fe437 100644 --- a/src/SignalR/server/SignalR/test/MapSignalRTests.cs +++ b/src/SignalR/server/SignalR/test/MapSignalRTests.cs @@ -99,6 +99,49 @@ public void MapHubFindsAuthAttributeOnHub() Assert.Equal(0, authCount); } + [Fact] + public void MapHubFindsMetadataPolicyOnHub() + { + var authCount = 0; + var policy1 = new AuthorizationPolicyBuilder().RequireAssertion(_ => true).Build(); + var req = new TestRequirement(); + using (var host = BuildWebHost(routes => routes.MapHub("/path", options => + { + authCount += options.AuthorizationData.Count; + }) + .RequireAuthorization(policy1) + .RequireAuthorization(policy => policy.AddRequirements(req)))) + { + host.Start(); + + var dataSource = host.Services.GetRequiredService(); + // We register 2 endpoints (/negotiate and /) + Assert.Collection(dataSource.Endpoints, + endpoint => + { + Assert.Equal("/path/negotiate", endpoint.DisplayName); + Assert.Equal(1, endpoint.Metadata.GetOrderedMetadata().Count); + var policies = endpoint.Metadata.GetOrderedMetadata(); + Assert.Equal(2, policies.Count); + Assert.Equal(policy1, policies[0]); + Assert.Equal(1, policies[1].Requirements.Count); + Assert.Equal(req, policies[1].Requirements.First()); + }, + endpoint => + { + Assert.Equal("/path", endpoint.DisplayName); + Assert.Equal(1, endpoint.Metadata.GetOrderedMetadata().Count); + var policies = endpoint.Metadata.GetOrderedMetadata(); + Assert.Equal(2, policies.Count); + Assert.Equal(policy1, policies[0]); + Assert.Equal(1, policies[1].Requirements.Count); + Assert.Equal(req, policies[1].Requirements.First()); + }); + } + + Assert.Equal(0, authCount); + } + [Fact] public void MapHubFindsAuthAttributeOnInheritedHub() { @@ -345,6 +388,10 @@ private class AuthHub : Hub { } + private class TestRequirement : IAuthorizationRequirement + { + } + private IHost BuildWebHost(Action configure) { return new HostBuilder() From 56f38e13439726c6e2081dc19d6455dc4d7ff1b0 Mon Sep 17 00:00:00 2001 From: Hao Kung Date: Wed, 13 Apr 2022 14:18:23 -0700 Subject: [PATCH 08/11] Try to get endpoint metadata --- .../Core/src/Internal/DefaultHubDispatcher.cs | 15 +++++++++++---- .../Core/src/Internal/HubMethodDescriptor.cs | 6 ++++-- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs b/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs index b3f51f7fe4bb..5dd10a0d015b 100644 --- a/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs +++ b/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs @@ -7,7 +7,9 @@ using System.Security.Claims; using System.Threading.Channels; using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.Internal; +using Microsoft.AspNetCore.Routing; using Microsoft.AspNetCore.SignalR.Protocol; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Internal; @@ -549,21 +551,26 @@ private void InitializeHub(THub hub, HubConnectionContext connection) private static Task IsHubMethodAuthorized(IServiceProvider provider, HubConnectionContext hubConnectionContext, HubMethodDescriptor descriptor, object?[] hubMethodArguments, Hub hub) { + var endpoint = hubConnectionContext.Features.Get()?.Endpoint; + IReadOnlyList endpointPolicies = endpoint != null + ? endpoint.Metadata.GetOrderedMetadata() + : Array.Empty(); + // If there are no policies we don't need to run auth - if (descriptor.Policies.Count == 0) + if (descriptor.AuthorizeData.Count == 0 && endpointPolicies.Count == 0) { return TaskCache.True; } - return IsHubMethodAuthorizedSlow(provider, hubConnectionContext.User, descriptor.Policies, new HubInvocationContext(hubConnectionContext.HubCallerContext, provider, hub, descriptor.MethodExecutor.MethodInfo, hubMethodArguments)); + return IsHubMethodAuthorizedSlow(provider, hubConnectionContext.User, descriptor.AuthorizeData, endpointPolicies, new HubInvocationContext(hubConnectionContext.HubCallerContext, provider, hub, descriptor.MethodExecutor.MethodInfo, hubMethodArguments)); } - private static async Task IsHubMethodAuthorizedSlow(IServiceProvider provider, ClaimsPrincipal principal, IList policies, HubInvocationContext resource) + private static async Task IsHubMethodAuthorizedSlow(IServiceProvider provider, ClaimsPrincipal principal, IList authorizeData, IReadOnlyList endpointPolicies, HubInvocationContext resource) { var authService = provider.GetRequiredService(); var policyProvider = provider.GetRequiredService(); - var authorizePolicy = await AuthorizationPolicy.CombineAsync(policyProvider, policies); + var authorizePolicy = await AuthorizationPolicy.CombineAsync(policyProvider, authorizeData, endpointPolicies); // AuthorizationPolicy.CombineAsync only returns null if there are no policies and we check that above Debug.Assert(authorizePolicy != null); diff --git a/src/SignalR/server/Core/src/Internal/HubMethodDescriptor.cs b/src/SignalR/server/Core/src/Internal/HubMethodDescriptor.cs index 79d364d0ecab..106828105afc 100644 --- a/src/SignalR/server/Core/src/Internal/HubMethodDescriptor.cs +++ b/src/SignalR/server/Core/src/Internal/HubMethodDescriptor.cs @@ -99,7 +99,7 @@ public HubMethodDescriptor(ObjectMethodExecutor methodExecutor, IServiceProvider OriginalParameterTypes = methodExecutor.MethodParameters.Select(p => p.ParameterType).ToArray(); } - Policies = policies.ToArray(); + AuthorizeData = policies.ToArray(); } public List? StreamingParameters { get; private set; } @@ -116,7 +116,9 @@ public HubMethodDescriptor(ObjectMethodExecutor methodExecutor, IServiceProvider public Type? StreamReturnType { get; } - public IList Policies { get; } + public IList AuthorizeData { get; } + + public IReadOnlyList EndpointPolicies { get; } public bool HasSyntheticArguments { get; private set; } From e8602448e1142a91bff9283e3a963e4cc5467784 Mon Sep 17 00:00:00 2001 From: Hao Kung Date: Wed, 13 Apr 2022 14:56:17 -0700 Subject: [PATCH 09/11] Fix build --- src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs | 1 - src/SignalR/server/Core/src/Internal/HubMethodDescriptor.cs | 2 -- 2 files changed, 3 deletions(-) diff --git a/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs b/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs index 5dd10a0d015b..ea482735b1af 100644 --- a/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs +++ b/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs @@ -9,7 +9,6 @@ using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.Internal; -using Microsoft.AspNetCore.Routing; using Microsoft.AspNetCore.SignalR.Protocol; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Internal; diff --git a/src/SignalR/server/Core/src/Internal/HubMethodDescriptor.cs b/src/SignalR/server/Core/src/Internal/HubMethodDescriptor.cs index 106828105afc..e2d4f5022a13 100644 --- a/src/SignalR/server/Core/src/Internal/HubMethodDescriptor.cs +++ b/src/SignalR/server/Core/src/Internal/HubMethodDescriptor.cs @@ -118,8 +118,6 @@ public HubMethodDescriptor(ObjectMethodExecutor methodExecutor, IServiceProvider public IList AuthorizeData { get; } - public IReadOnlyList EndpointPolicies { get; } - public bool HasSyntheticArguments { get; private set; } public bool IsServiceArgument(int argumentIndex) From 98ac7b50038d2b8e6b2de67617aaee4ea58cadae Mon Sep 17 00:00:00 2001 From: Hao Kung Date: Fri, 15 Apr 2022 09:57:06 -0700 Subject: [PATCH 10/11] Revert "Fix build" This reverts commit e8602448e1142a91bff9283e3a963e4cc5467784. --- src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs | 1 + src/SignalR/server/Core/src/Internal/HubMethodDescriptor.cs | 2 ++ 2 files changed, 3 insertions(+) diff --git a/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs b/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs index ea482735b1af..5dd10a0d015b 100644 --- a/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs +++ b/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs @@ -9,6 +9,7 @@ using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.Internal; +using Microsoft.AspNetCore.Routing; using Microsoft.AspNetCore.SignalR.Protocol; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Internal; diff --git a/src/SignalR/server/Core/src/Internal/HubMethodDescriptor.cs b/src/SignalR/server/Core/src/Internal/HubMethodDescriptor.cs index e2d4f5022a13..106828105afc 100644 --- a/src/SignalR/server/Core/src/Internal/HubMethodDescriptor.cs +++ b/src/SignalR/server/Core/src/Internal/HubMethodDescriptor.cs @@ -118,6 +118,8 @@ public HubMethodDescriptor(ObjectMethodExecutor methodExecutor, IServiceProvider public IList AuthorizeData { get; } + public IReadOnlyList EndpointPolicies { get; } + public bool HasSyntheticArguments { get; private set; } public bool IsServiceArgument(int argumentIndex) From c212e8bd0ba873b1bd0fa5e9cc893c38e4663732 Mon Sep 17 00:00:00 2001 From: Hao Kung Date: Fri, 15 Apr 2022 09:57:12 -0700 Subject: [PATCH 11/11] Revert "Try to get endpoint metadata" This reverts commit 56f38e13439726c6e2081dc19d6455dc4d7ff1b0. --- .../Core/src/Internal/DefaultHubDispatcher.cs | 15 ++++----------- .../Core/src/Internal/HubMethodDescriptor.cs | 6 ++---- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs b/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs index 5dd10a0d015b..b3f51f7fe4bb 100644 --- a/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs +++ b/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs @@ -7,9 +7,7 @@ using System.Security.Claims; using System.Threading.Channels; using Microsoft.AspNetCore.Authorization; -using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.Internal; -using Microsoft.AspNetCore.Routing; using Microsoft.AspNetCore.SignalR.Protocol; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Internal; @@ -551,26 +549,21 @@ private void InitializeHub(THub hub, HubConnectionContext connection) private static Task IsHubMethodAuthorized(IServiceProvider provider, HubConnectionContext hubConnectionContext, HubMethodDescriptor descriptor, object?[] hubMethodArguments, Hub hub) { - var endpoint = hubConnectionContext.Features.Get()?.Endpoint; - IReadOnlyList endpointPolicies = endpoint != null - ? endpoint.Metadata.GetOrderedMetadata() - : Array.Empty(); - // If there are no policies we don't need to run auth - if (descriptor.AuthorizeData.Count == 0 && endpointPolicies.Count == 0) + if (descriptor.Policies.Count == 0) { return TaskCache.True; } - return IsHubMethodAuthorizedSlow(provider, hubConnectionContext.User, descriptor.AuthorizeData, endpointPolicies, new HubInvocationContext(hubConnectionContext.HubCallerContext, provider, hub, descriptor.MethodExecutor.MethodInfo, hubMethodArguments)); + return IsHubMethodAuthorizedSlow(provider, hubConnectionContext.User, descriptor.Policies, new HubInvocationContext(hubConnectionContext.HubCallerContext, provider, hub, descriptor.MethodExecutor.MethodInfo, hubMethodArguments)); } - private static async Task IsHubMethodAuthorizedSlow(IServiceProvider provider, ClaimsPrincipal principal, IList authorizeData, IReadOnlyList endpointPolicies, HubInvocationContext resource) + private static async Task IsHubMethodAuthorizedSlow(IServiceProvider provider, ClaimsPrincipal principal, IList policies, HubInvocationContext resource) { var authService = provider.GetRequiredService(); var policyProvider = provider.GetRequiredService(); - var authorizePolicy = await AuthorizationPolicy.CombineAsync(policyProvider, authorizeData, endpointPolicies); + var authorizePolicy = await AuthorizationPolicy.CombineAsync(policyProvider, policies); // AuthorizationPolicy.CombineAsync only returns null if there are no policies and we check that above Debug.Assert(authorizePolicy != null); diff --git a/src/SignalR/server/Core/src/Internal/HubMethodDescriptor.cs b/src/SignalR/server/Core/src/Internal/HubMethodDescriptor.cs index 106828105afc..79d364d0ecab 100644 --- a/src/SignalR/server/Core/src/Internal/HubMethodDescriptor.cs +++ b/src/SignalR/server/Core/src/Internal/HubMethodDescriptor.cs @@ -99,7 +99,7 @@ public HubMethodDescriptor(ObjectMethodExecutor methodExecutor, IServiceProvider OriginalParameterTypes = methodExecutor.MethodParameters.Select(p => p.ParameterType).ToArray(); } - AuthorizeData = policies.ToArray(); + Policies = policies.ToArray(); } public List? StreamingParameters { get; private set; } @@ -116,9 +116,7 @@ public HubMethodDescriptor(ObjectMethodExecutor methodExecutor, IServiceProvider public Type? StreamReturnType { get; } - public IList AuthorizeData { get; } - - public IReadOnlyList EndpointPolicies { get; } + public IList Policies { get; } public bool HasSyntheticArguments { get; private set; }