From 9d40596e3565ae1f3c1e1175139aee017b02c1bd Mon Sep 17 00:00:00 2001 From: BrennanConroy Date: Wed, 9 Jun 2021 10:42:19 -0700 Subject: [PATCH 1/7] Add a feature for accessing the AuthenticateResult --- .../src/IAuthenticateResultFeature.cs | 16 ++++++++ .../src/PublicAPI.Unshipped.txt | 3 ++ .../Policy/src/AuthorizationMiddleware.cs | 18 ++++++-- .../src/Internal/AuthenticationFeatures.cs | 41 +++++++++++++++++++ .../Policy/src/PolicyEvaluator.cs | 10 ++++- 5 files changed, 83 insertions(+), 5 deletions(-) create mode 100644 src/Http/Authentication.Abstractions/src/IAuthenticateResultFeature.cs create mode 100644 src/Security/Authorization/Policy/src/Internal/AuthenticationFeatures.cs diff --git a/src/Http/Authentication.Abstractions/src/IAuthenticateResultFeature.cs b/src/Http/Authentication.Abstractions/src/IAuthenticateResultFeature.cs new file mode 100644 index 000000000000..a2b1a8c7a1db --- /dev/null +++ b/src/Http/Authentication.Abstractions/src/IAuthenticateResultFeature.cs @@ -0,0 +1,16 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +namespace Microsoft.AspNetCore.Authentication +{ + /// + /// TODO + /// + public interface IAuthenticateResultFeature + { + /// + /// TODO + /// + AuthenticateResult Result { get; set; } + } +} diff --git a/src/Http/Authentication.Abstractions/src/PublicAPI.Unshipped.txt b/src/Http/Authentication.Abstractions/src/PublicAPI.Unshipped.txt index 7dc5c58110bf..bb1a1ea62a7a 100644 --- a/src/Http/Authentication.Abstractions/src/PublicAPI.Unshipped.txt +++ b/src/Http/Authentication.Abstractions/src/PublicAPI.Unshipped.txt @@ -1 +1,4 @@ #nullable enable +Microsoft.AspNetCore.Authentication.IAuthenticateResultFeature +Microsoft.AspNetCore.Authentication.IAuthenticateResultFeature.Result.get -> Microsoft.AspNetCore.Authentication.AuthenticateResult! +Microsoft.AspNetCore.Authentication.IAuthenticateResultFeature.Result.set -> void diff --git a/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs b/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs index 0fddd96878f7..07043311a46b 100644 --- a/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs +++ b/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs @@ -3,8 +3,11 @@ using System; using System.Threading.Tasks; +using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Authorization.Policy; +using Microsoft.AspNetCore.Authorization.Policy.Internal; using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.Features.Authentication; using Microsoft.Extensions.DependencyInjection; namespace Microsoft.AspNetCore.Authorization @@ -29,7 +32,7 @@ public class AuthorizationMiddleware /// /// The next middleware in the application middleware pipeline. /// The . - public AuthorizationMiddleware(RequestDelegate next, IAuthorizationPolicyProvider policyProvider) + public AuthorizationMiddleware(RequestDelegate next, IAuthorizationPolicyProvider policyProvider) { _next = next ?? throw new ArgumentNullException(nameof(next)); _policyProvider = policyProvider ?? throw new ArgumentNullException(nameof(policyProvider)); @@ -64,12 +67,19 @@ public async Task Invoke(HttpContext context) return; } - // Policy evaluator has transient lifetime so it fetched from request services instead of injecting in constructor + // Policy evaluator has transient lifetime so it's fetched from request services instead of injecting in constructor var policyEvaluator = context.RequestServices.GetRequiredService(); var authenticateResult = await policyEvaluator.AuthenticateAsync(policy, context); - // Allow Anonymous skips all authorization + if (authenticateResult.Succeeded) + { + var authFeatures = new AuthenticationFeatures(authenticateResult); + context.Features.Set(authFeatures); + context.Features.Set(authFeatures); + } + + // Allow Anonymous still wants to run authorization to populate the User but skips any failure/challenge handling if (endpoint?.Metadata.GetMetadata() != null) { await _next(context); @@ -85,7 +95,7 @@ public async Task Invoke(HttpContext context) { resource = context; } - + var authorizeResult = await policyEvaluator.AuthorizeAsync(policy, authenticateResult, context, resource); var authorizationMiddlewareResultHandler = context.RequestServices.GetRequiredService(); await authorizationMiddlewareResultHandler.HandleAsync(_next, context, policy, authorizeResult); diff --git a/src/Security/Authorization/Policy/src/Internal/AuthenticationFeatures.cs b/src/Security/Authorization/Policy/src/Internal/AuthenticationFeatures.cs new file mode 100644 index 000000000000..de2c3f76a256 --- /dev/null +++ b/src/Security/Authorization/Policy/src/Internal/AuthenticationFeatures.cs @@ -0,0 +1,41 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Security.Claims; +using Microsoft.AspNetCore.Authentication; +using Microsoft.AspNetCore.Http.Features.Authentication; + +namespace Microsoft.AspNetCore.Authorization.Policy.Internal +{ + /// + /// Keeps the User and AuthenticationResult consistent with each other + /// + internal class AuthenticationFeatures : IAuthenticateResultFeature, IHttpAuthenticationFeature + { + public AuthenticationFeatures(AuthenticateResult result) + { + Result = result; + } + + public AuthenticateResult Result { get; set; } + + public ClaimsPrincipal? User + { + get => Result.Principal; + set + { + if (value is not null) + { + Result = AuthenticateResult.Success( + new AuthenticationTicket(value, Result.Ticket!.Properties, Result.Ticket.AuthenticationScheme)); + } + else + { + // REVIEW: Make Result nullable and null it out here? + throw new ArgumentNullException(nameof(User)); + } + } + } + } +} diff --git a/src/Security/Authorization/Policy/src/PolicyEvaluator.cs b/src/Security/Authorization/Policy/src/PolicyEvaluator.cs index 06da4e969cb0..10bfdafaa260 100644 --- a/src/Security/Authorization/Policy/src/PolicyEvaluator.cs +++ b/src/Security/Authorization/Policy/src/PolicyEvaluator.cs @@ -38,19 +38,27 @@ public virtual async Task AuthenticateAsync(AuthorizationPol if (policy.AuthenticationSchemes != null && policy.AuthenticationSchemes.Count > 0) { ClaimsPrincipal? newPrincipal = null; + DateTimeOffset? minExpiresUtc = null; foreach (var scheme in policy.AuthenticationSchemes) { var result = await context.AuthenticateAsync(scheme); if (result != null && result.Succeeded) { newPrincipal = SecurityHelper.MergeUserPrincipal(newPrincipal, result.Principal); + + if (minExpiresUtc is null || result.Properties?.ExpiresUtc < minExpiresUtc) + { + minExpiresUtc = result.Properties?.ExpiresUtc; + } } } if (newPrincipal != null) { context.User = newPrincipal; - return AuthenticateResult.Success(new AuthenticationTicket(newPrincipal, string.Join(";", policy.AuthenticationSchemes))); + var ticket = new AuthenticationTicket(newPrincipal, string.Join(";", policy.AuthenticationSchemes)); + ticket.Properties.ExpiresUtc = minExpiresUtc; + return AuthenticateResult.Success(ticket); } else { From 1881469118ba64bc0ddf28b5b466620dda93d298 Mon Sep 17 00:00:00 2001 From: BrennanConroy Date: Thu, 10 Jun 2021 13:57:35 -0700 Subject: [PATCH 2/7] update --- .../src/IAuthenticateResultFeature.cs | 2 +- .../src/PublicAPI.Unshipped.txt | 2 +- .../src/Internal/AuthenticationFeatures.cs | 27 ++++++++++--------- 3 files changed, 17 insertions(+), 14 deletions(-) diff --git a/src/Http/Authentication.Abstractions/src/IAuthenticateResultFeature.cs b/src/Http/Authentication.Abstractions/src/IAuthenticateResultFeature.cs index a2b1a8c7a1db..d031ecbaedaa 100644 --- a/src/Http/Authentication.Abstractions/src/IAuthenticateResultFeature.cs +++ b/src/Http/Authentication.Abstractions/src/IAuthenticateResultFeature.cs @@ -11,6 +11,6 @@ public interface IAuthenticateResultFeature /// /// TODO /// - AuthenticateResult Result { get; set; } + AuthenticateResult? Result { get; set; } } } diff --git a/src/Http/Authentication.Abstractions/src/PublicAPI.Unshipped.txt b/src/Http/Authentication.Abstractions/src/PublicAPI.Unshipped.txt index bb1a1ea62a7a..3b4c06f4ed67 100644 --- a/src/Http/Authentication.Abstractions/src/PublicAPI.Unshipped.txt +++ b/src/Http/Authentication.Abstractions/src/PublicAPI.Unshipped.txt @@ -1,4 +1,4 @@ #nullable enable Microsoft.AspNetCore.Authentication.IAuthenticateResultFeature -Microsoft.AspNetCore.Authentication.IAuthenticateResultFeature.Result.get -> Microsoft.AspNetCore.Authentication.AuthenticateResult! +Microsoft.AspNetCore.Authentication.IAuthenticateResultFeature.Result.get -> Microsoft.AspNetCore.Authentication.AuthenticateResult? Microsoft.AspNetCore.Authentication.IAuthenticateResultFeature.Result.set -> void diff --git a/src/Security/Authorization/Policy/src/Internal/AuthenticationFeatures.cs b/src/Security/Authorization/Policy/src/Internal/AuthenticationFeatures.cs index de2c3f76a256..fb8ee1168c76 100644 --- a/src/Security/Authorization/Policy/src/Internal/AuthenticationFeatures.cs +++ b/src/Security/Authorization/Policy/src/Internal/AuthenticationFeatures.cs @@ -13,28 +13,31 @@ namespace Microsoft.AspNetCore.Authorization.Policy.Internal /// internal class AuthenticationFeatures : IAuthenticateResultFeature, IHttpAuthenticationFeature { + private ClaimsPrincipal? _user; + private AuthenticateResult? _result; + public AuthenticationFeatures(AuthenticateResult result) { Result = result; } - public AuthenticateResult Result { get; set; } + public AuthenticateResult? Result + { + get => _result; + set + { + _result = value; + _user = _result?.Principal; + } + } public ClaimsPrincipal? User { - get => Result.Principal; + get => _user; set { - if (value is not null) - { - Result = AuthenticateResult.Success( - new AuthenticationTicket(value, Result.Ticket!.Properties, Result.Ticket.AuthenticationScheme)); - } - else - { - // REVIEW: Make Result nullable and null it out here? - throw new ArgumentNullException(nameof(User)); - } + _user = value; + Result = null; } } } From fbe94051ec295778d2c9f5c59c7a2458085c4d9f Mon Sep 17 00:00:00 2001 From: BrennanConroy Date: Thu, 10 Jun 2021 14:34:15 -0700 Subject: [PATCH 3/7] fixups --- .../Policy/src/{Internal => }/AuthenticationFeatures.cs | 5 ++--- .../Authorization/Policy/src/AuthorizationMiddleware.cs | 3 +-- src/Security/Authorization/Policy/src/PolicyEvaluator.cs | 2 ++ 3 files changed, 5 insertions(+), 5 deletions(-) rename src/Security/Authorization/Policy/src/{Internal => }/AuthenticationFeatures.cs (91%) diff --git a/src/Security/Authorization/Policy/src/Internal/AuthenticationFeatures.cs b/src/Security/Authorization/Policy/src/AuthenticationFeatures.cs similarity index 91% rename from src/Security/Authorization/Policy/src/Internal/AuthenticationFeatures.cs rename to src/Security/Authorization/Policy/src/AuthenticationFeatures.cs index fb8ee1168c76..860f208dbe1d 100644 --- a/src/Security/Authorization/Policy/src/Internal/AuthenticationFeatures.cs +++ b/src/Security/Authorization/Policy/src/AuthenticationFeatures.cs @@ -1,12 +1,11 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. -using System; using System.Security.Claims; using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Http.Features.Authentication; -namespace Microsoft.AspNetCore.Authorization.Policy.Internal +namespace Microsoft.AspNetCore.Authorization.Policy { /// /// Keeps the User and AuthenticationResult consistent with each other @@ -37,7 +36,7 @@ public ClaimsPrincipal? User set { _user = value; - Result = null; + _result = null; } } } diff --git a/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs b/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs index 07043311a46b..40fe09103638 100644 --- a/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs +++ b/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs @@ -5,7 +5,6 @@ using System.Threading.Tasks; using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Authorization.Policy; -using Microsoft.AspNetCore.Authorization.Policy.Internal; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Features.Authentication; using Microsoft.Extensions.DependencyInjection; @@ -72,7 +71,7 @@ public async Task Invoke(HttpContext context) var authenticateResult = await policyEvaluator.AuthenticateAsync(policy, context); - if (authenticateResult.Succeeded) + if (authenticateResult?.Succeeded ?? false) { var authFeatures = new AuthenticationFeatures(authenticateResult); context.Features.Set(authFeatures); diff --git a/src/Security/Authorization/Policy/src/PolicyEvaluator.cs b/src/Security/Authorization/Policy/src/PolicyEvaluator.cs index 10bfdafaa260..35b0b1181f64 100644 --- a/src/Security/Authorization/Policy/src/PolicyEvaluator.cs +++ b/src/Security/Authorization/Policy/src/PolicyEvaluator.cs @@ -57,6 +57,8 @@ public virtual async Task AuthenticateAsync(AuthorizationPol { context.User = newPrincipal; var ticket = new AuthenticationTicket(newPrincipal, string.Join(";", policy.AuthenticationSchemes)); + // ExpiresUtc is the easiest property to reason about when dealing with multiple schemese + // SignalR will use this property to evaluate auth expiration for long running connections ticket.Properties.ExpiresUtc = minExpiresUtc; return AuthenticateResult.Success(ticket); } From e313d178caa3dd057e83cb34d2c83cf3f72fa1a0 Mon Sep 17 00:00:00 2001 From: BrennanConroy Date: Fri, 11 Jun 2021 13:58:51 -0700 Subject: [PATCH 4/7] some tests --- .../Policy/src/AuthorizationMiddleware.cs | 2 +- .../test/AuthorizationMiddlewareTests.cs | 148 +++++++++++++++++- 2 files changed, 145 insertions(+), 5 deletions(-) diff --git a/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs b/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs index 40fe09103638..367d0624a2f4 100644 --- a/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs +++ b/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs @@ -95,7 +95,7 @@ public async Task Invoke(HttpContext context) resource = context; } - var authorizeResult = await policyEvaluator.AuthorizeAsync(policy, authenticateResult, context, resource); + var authorizeResult = await policyEvaluator.AuthorizeAsync(policy, authenticateResult!, context, resource); var authorizationMiddlewareResultHandler = context.RequestServices.GetRequiredService(); await authorizationMiddlewareResultHandler.HandleAsync(_next, context, policy, authorizeResult); } diff --git a/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs b/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs index 89de50e3d4ce..cb8e382232d4 100644 --- a/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs +++ b/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Linq; using System.Security.Claims; using System.Threading.Tasks; using Microsoft.AspNetCore.Authentication; @@ -9,7 +10,6 @@ using Microsoft.AspNetCore.Authorization.Test.TestObjects; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.DependencyInjection; -using Microsoft.Extensions.Options; using Moq; using Xunit; @@ -54,7 +54,7 @@ public async Task NoEndpointWithFallback_AnonymousUser_Challenges() // Assert Assert.False(next.Called); } - + [Fact] public async Task HasEndpointWithoutAuth_AnonymousUser_Allows() { @@ -156,7 +156,7 @@ public async Task HasEndpointWithAuth_ChallengesAuthenticationSchemes() Assert.False(next.Called); Assert.True(authenticationService.ChallengeCalled); } - + [Fact] public async Task HasEndpointWithAuth_AnonymousUser_ChallengePerScheme() { @@ -367,7 +367,7 @@ public async Task AuthZResourceShouldBeEndpointByDefaultWithCompatSwitch() // Assert Assert.Equal(endpoint, resource); } - + [Fact] public async Task Invoke_RequireUnknownRoleShouldForbid() { @@ -435,6 +435,146 @@ public async Task Invoke_InvalidClaimShouldForbid() Assert.True(authenticationService.ForbidCalled); } + [Fact] + public async Task IAuthenticateResultFeature_SetOnSuccessfulAuthorize() + { + // Arrange + var policy = new AuthorizationPolicyBuilder().RequireClaim("Permission", "CanViewPage").Build(); + var policyProvider = new Mock(); + policyProvider.Setup(p => p.GetDefaultPolicyAsync()).ReturnsAsync(policy); + var next = new TestRequestDelegate(); + + var middleware = CreateMiddleware(next.Invoke, policyProvider.Object); + var context = GetHttpContext(endpoint: CreateEndpoint(new AuthorizeAttribute())); + + // Act + await middleware.Invoke(context); + + // Assert + Assert.True(next.Called); + var authenticateResultFeature = context.Features.Get(); + Assert.NotNull(authenticateResultFeature); + Assert.NotNull(authenticateResultFeature.Result); + Assert.Same(context.User, authenticateResultFeature.Result.Principal); + } + + [Fact] + public async Task IAuthenticateResultFeature_NotSetOnUnsuccessfulAuthorize() + { + // Arrange + var policy = new AuthorizationPolicyBuilder().RequireRole("Wut").AddAuthenticationSchemes("NotImplemented").Build(); + var policyProvider = new Mock(); + policyProvider.Setup(p => p.GetDefaultPolicyAsync()).ReturnsAsync(policy); + var next = new TestRequestDelegate(); + var authenticationService = new TestAuthenticationService(); + + var middleware = CreateMiddleware(next.Invoke, policyProvider.Object); + var context = GetHttpContext(endpoint: CreateEndpoint(new AuthorizeAttribute(), new AllowAnonymousAttribute()), authenticationService: authenticationService); + + // Act + await middleware.Invoke(context); + + // Assert + Assert.True(next.Called); + var authenticateResultFeature = context.Features.Get(); + Assert.Null(authenticateResultFeature); + Assert.True(authenticationService.AuthenticateCalled); + } + + [Fact] + public async Task IAuthenticateResultFeature_ContainsLowestExpiration() + { + // Arrange + var policy = new AuthorizationPolicyBuilder().RequireRole("Wut").AddAuthenticationSchemes("Basic", "Bearer").Build(); + var policyProvider = new Mock(); + policyProvider.Setup(p => p.GetDefaultPolicyAsync()).ReturnsAsync(policy); + var next = new TestRequestDelegate(); + + var firstExpiration = new DateTimeOffset(2021, 5, 12, 2, 3, 4, TimeSpan.Zero); + var secondExpiration = new DateTimeOffset(2021, 5, 11, 2, 3, 4, TimeSpan.Zero); + var authenticationService = new Mock(); + authenticationService.Setup(s => s.AuthenticateAsync(It.IsAny(), "Basic")) + .ReturnsAsync((HttpContext c, string scheme) => + { + var res = AuthenticateResult.Success(new AuthenticationTicket(new ClaimsPrincipal(c.User.Identities.FirstOrDefault(i => i.AuthenticationType == scheme)), scheme)); + res.Properties.ExpiresUtc = firstExpiration; + return res; + }); + authenticationService.Setup(s => s.AuthenticateAsync(It.IsAny(), "Bearer")) + .ReturnsAsync((HttpContext c, string scheme) => + { + var res = AuthenticateResult.Success(new AuthenticationTicket(new ClaimsPrincipal(c.User.Identities.FirstOrDefault(i => i.AuthenticationType == scheme)), scheme)); + res.Properties.ExpiresUtc = secondExpiration; + return res; + }); + + var middleware = CreateMiddleware(next.Invoke, policyProvider.Object); + var context = GetHttpContext(endpoint: CreateEndpoint(new AuthorizeAttribute()), authenticationService: authenticationService.Object); + + // Act + await middleware.Invoke(context); + + // Assert + var authenticateResultFeature = context.Features.Get(); + Assert.NotNull(authenticateResultFeature); + Assert.NotNull(authenticateResultFeature.Result); + Assert.Same(context.User, authenticateResultFeature.Result.Principal); + Assert.Equal(secondExpiration, authenticateResultFeature.Result?.Properties?.ExpiresUtc); + } + + [Fact] + public async Task IAuthenticateResultFeature_NullResultWhenUserSetAfter() + { + // Arrange + var policy = new AuthorizationPolicyBuilder().RequireClaim("Permission", "CanViewPage").Build(); + var policyProvider = new Mock(); + policyProvider.Setup(p => p.GetDefaultPolicyAsync()).ReturnsAsync(policy); + var next = new TestRequestDelegate(); + + var middleware = CreateMiddleware(next.Invoke, policyProvider.Object); + var context = GetHttpContext(endpoint: CreateEndpoint(new AuthorizeAttribute())); + + // Act + await middleware.Invoke(context); + + // Assert + Assert.True(next.Called); + var authenticateResultFeature = context.Features.Get(); + Assert.NotNull(authenticateResultFeature); + Assert.NotNull(authenticateResultFeature.Result); + Assert.Same(context.User, authenticateResultFeature.Result.Principal); + + context.User = new ClaimsPrincipal(); + Assert.Null(authenticateResultFeature.Result); + } + + [Fact] + public async Task IAuthenticateResultFeature_SettingResultSetsUser() + { + // Arrange + var policy = new AuthorizationPolicyBuilder().RequireClaim("Permission", "CanViewPage").Build(); + var policyProvider = new Mock(); + policyProvider.Setup(p => p.GetDefaultPolicyAsync()).ReturnsAsync(policy); + var next = new TestRequestDelegate(); + + var middleware = CreateMiddleware(next.Invoke, policyProvider.Object); + var context = GetHttpContext(endpoint: CreateEndpoint(new AuthorizeAttribute())); + + // Act + await middleware.Invoke(context); + + // Assert + Assert.True(next.Called); + var authenticateResultFeature = context.Features.Get(); + Assert.NotNull(authenticateResultFeature); + Assert.NotNull(authenticateResultFeature.Result); + Assert.Same(context.User, authenticateResultFeature.Result.Principal); + + var newTicket = new AuthenticationTicket(new ClaimsPrincipal(), ""); + authenticateResultFeature.Result = AuthenticateResult.Success(newTicket); + Assert.Same(context.User, newTicket.Principal); + } + private AuthorizationMiddleware CreateMiddleware(RequestDelegate requestDelegate = null, IAuthorizationPolicyProvider policyProvider = null) { requestDelegate = requestDelegate ?? ((context) => Task.CompletedTask); From baeb4424e00f389ee5d1b11979e8be296e62dce0 Mon Sep 17 00:00:00 2001 From: BrennanConroy Date: Mon, 14 Jun 2021 19:27:11 -0700 Subject: [PATCH 5/7] update --- .../src/IAuthenticateResultFeature.cs | 7 +++++-- .../Authorization/Policy/src/AuthenticationFeatures.cs | 2 +- src/Security/Authorization/Policy/src/PolicyEvaluator.cs | 2 +- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/Http/Authentication.Abstractions/src/IAuthenticateResultFeature.cs b/src/Http/Authentication.Abstractions/src/IAuthenticateResultFeature.cs index d031ecbaedaa..1408002b1537 100644 --- a/src/Http/Authentication.Abstractions/src/IAuthenticateResultFeature.cs +++ b/src/Http/Authentication.Abstractions/src/IAuthenticateResultFeature.cs @@ -1,15 +1,18 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using Microsoft.AspNetCore.Http.Features.Authentication; + namespace Microsoft.AspNetCore.Authentication { /// - /// TODO + /// Used to capture the from the authorization middleware. /// public interface IAuthenticateResultFeature { /// - /// TODO + /// The from the authorization middleware. + /// Set to null if the property is set after the authorization middleware. /// AuthenticateResult? Result { get; set; } } diff --git a/src/Security/Authorization/Policy/src/AuthenticationFeatures.cs b/src/Security/Authorization/Policy/src/AuthenticationFeatures.cs index 860f208dbe1d..fe19a6a9700d 100644 --- a/src/Security/Authorization/Policy/src/AuthenticationFeatures.cs +++ b/src/Security/Authorization/Policy/src/AuthenticationFeatures.cs @@ -10,7 +10,7 @@ namespace Microsoft.AspNetCore.Authorization.Policy /// /// Keeps the User and AuthenticationResult consistent with each other /// - internal class AuthenticationFeatures : IAuthenticateResultFeature, IHttpAuthenticationFeature + internal sealed class AuthenticationFeatures : IAuthenticateResultFeature, IHttpAuthenticationFeature { private ClaimsPrincipal? _user; private AuthenticateResult? _result; diff --git a/src/Security/Authorization/Policy/src/PolicyEvaluator.cs b/src/Security/Authorization/Policy/src/PolicyEvaluator.cs index 35b0b1181f64..79e39e358e1d 100644 --- a/src/Security/Authorization/Policy/src/PolicyEvaluator.cs +++ b/src/Security/Authorization/Policy/src/PolicyEvaluator.cs @@ -57,7 +57,7 @@ public virtual async Task AuthenticateAsync(AuthorizationPol { context.User = newPrincipal; var ticket = new AuthenticationTicket(newPrincipal, string.Join(";", policy.AuthenticationSchemes)); - // ExpiresUtc is the easiest property to reason about when dealing with multiple schemese + // ExpiresUtc is the easiest property to reason about when dealing with multiple schemes // SignalR will use this property to evaluate auth expiration for long running connections ticket.Properties.ExpiresUtc = minExpiresUtc; return AuthenticateResult.Success(ticket); From bf539f55dc32b4fd66effd5c24ee9ecf57289dbd Mon Sep 17 00:00:00 2001 From: BrennanConroy Date: Wed, 23 Jun 2021 10:37:51 -0700 Subject: [PATCH 6/7] update --- .../src/IAuthenticateResultFeature.cs | 2 +- .../src/PublicAPI.Unshipped.txt | 4 +- .../Core/src/AuthenticationFeatures.cs | 42 ++++++ .../Core/src/AuthenticationMiddleware.cs | 7 + .../test/AuthenticationMiddlewareTests.cs | 125 +++++++++++++++++- .../Policy/src/AuthenticationFeatures.cs | 4 +- .../test/AuthorizationMiddlewareTests.cs | 55 ++++++-- 7 files changed, 222 insertions(+), 17 deletions(-) create mode 100644 src/Security/Authentication/Core/src/AuthenticationFeatures.cs diff --git a/src/Http/Authentication.Abstractions/src/IAuthenticateResultFeature.cs b/src/Http/Authentication.Abstractions/src/IAuthenticateResultFeature.cs index 1408002b1537..1d9ad0034446 100644 --- a/src/Http/Authentication.Abstractions/src/IAuthenticateResultFeature.cs +++ b/src/Http/Authentication.Abstractions/src/IAuthenticateResultFeature.cs @@ -14,6 +14,6 @@ public interface IAuthenticateResultFeature /// The from the authorization middleware. /// Set to null if the property is set after the authorization middleware. /// - AuthenticateResult? Result { get; set; } + AuthenticateResult? AuthenticateResult { get; set; } } } diff --git a/src/Http/Authentication.Abstractions/src/PublicAPI.Unshipped.txt b/src/Http/Authentication.Abstractions/src/PublicAPI.Unshipped.txt index 3b4c06f4ed67..1e3de58fe613 100644 --- a/src/Http/Authentication.Abstractions/src/PublicAPI.Unshipped.txt +++ b/src/Http/Authentication.Abstractions/src/PublicAPI.Unshipped.txt @@ -1,4 +1,4 @@ #nullable enable Microsoft.AspNetCore.Authentication.IAuthenticateResultFeature -Microsoft.AspNetCore.Authentication.IAuthenticateResultFeature.Result.get -> Microsoft.AspNetCore.Authentication.AuthenticateResult? -Microsoft.AspNetCore.Authentication.IAuthenticateResultFeature.Result.set -> void +Microsoft.AspNetCore.Authentication.IAuthenticateResultFeature.AuthenticateResult.get -> Microsoft.AspNetCore.Authentication.AuthenticateResult? +Microsoft.AspNetCore.Authentication.IAuthenticateResultFeature.AuthenticateResult.set -> void diff --git a/src/Security/Authentication/Core/src/AuthenticationFeatures.cs b/src/Security/Authentication/Core/src/AuthenticationFeatures.cs new file mode 100644 index 000000000000..bbe75a8261b8 --- /dev/null +++ b/src/Security/Authentication/Core/src/AuthenticationFeatures.cs @@ -0,0 +1,42 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Security.Claims; +using Microsoft.AspNetCore.Http.Features.Authentication; + +namespace Microsoft.AspNetCore.Authentication +{ + /// + /// Keeps the User and AuthenticationResult consistent with each other + /// + internal sealed class AuthenticationFeatures : IAuthenticateResultFeature, IHttpAuthenticationFeature + { + private ClaimsPrincipal? _user; + private AuthenticateResult? _result; + + public AuthenticationFeatures(AuthenticateResult result) + { + AuthenticateResult = result; + } + + public AuthenticateResult? AuthenticateResult + { + get => _result; + set + { + _result = value; + _user = _result?.Principal; + } + } + + public ClaimsPrincipal? User + { + get => _user; + set + { + _user = value; + _result = null; + } + } + } +} diff --git a/src/Security/Authentication/Core/src/AuthenticationMiddleware.cs b/src/Security/Authentication/Core/src/AuthenticationMiddleware.cs index 8a1fa97b0a82..b2b810cf4ec2 100644 --- a/src/Security/Authentication/Core/src/AuthenticationMiddleware.cs +++ b/src/Security/Authentication/Core/src/AuthenticationMiddleware.cs @@ -4,6 +4,7 @@ using System; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.Features.Authentication; using Microsoft.Extensions.DependencyInjection; namespace Microsoft.AspNetCore.Authentication @@ -71,6 +72,12 @@ public async Task Invoke(HttpContext context) { context.User = result.Principal; } + if (result?.Succeeded ?? false) + { + var authFeatures = new AuthenticationFeatures(result); + context.Features.Set(authFeatures); + context.Features.Set(authFeatures); + } } await _next(context); diff --git a/src/Security/Authentication/test/AuthenticationMiddlewareTests.cs b/src/Security/Authentication/test/AuthenticationMiddlewareTests.cs index 232b9fed0e88..cf48902da024 100644 --- a/src/Security/Authentication/test/AuthenticationMiddlewareTests.cs +++ b/src/Security/Authentication/test/AuthenticationMiddlewareTests.cs @@ -3,12 +3,15 @@ using System; using System.Security.Claims; using System.Threading.Tasks; +using Microsoft.AspNetCore.Authentication.JwtBearer; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Hosting; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.TestHost; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Logging; +using Moq; using Xunit; namespace Microsoft.AspNetCore.Authentication @@ -54,6 +57,126 @@ public async Task OnlyInvokesCanHandleRequestHandlers() Assert.Equal(607, (int)response.StatusCode); } + [Fact] + public async Task IAuthenticateResultFeature_SetOnSuccessfulAuthenticate() + { + var authenticationService = new Mock(); + authenticationService.Setup(s => s.AuthenticateAsync(It.IsAny(), It.IsAny())) + .Returns(Task.FromResult(AuthenticateResult.Success(new AuthenticationTicket(new ClaimsPrincipal(), "custom")))); + var schemeProvider = new Mock(); + schemeProvider.Setup(p => p.GetDefaultAuthenticateSchemeAsync()) + .Returns(Task.FromResult(new AuthenticationScheme("custom", "custom", typeof(JwtBearerHandler)))); + var middleware = new AuthenticationMiddleware(c => Task.CompletedTask, schemeProvider.Object); + var context = GetHttpContext(authenticationService: authenticationService.Object); + + // Act + await middleware.Invoke(context); + + // Assert + var authenticateResultFeature = context.Features.Get(); + Assert.NotNull(authenticateResultFeature); + Assert.NotNull(authenticateResultFeature.AuthenticateResult); + Assert.True(authenticateResultFeature.AuthenticateResult.Succeeded); + Assert.Same(context.User, authenticateResultFeature.AuthenticateResult.Principal); + } + + [Fact] + public async Task IAuthenticateResultFeature_NotSetOnUnsuccessfulAuthenticate() + { + var authenticationService = new Mock(); + authenticationService.Setup(s => s.AuthenticateAsync(It.IsAny(), It.IsAny())) + .Returns(Task.FromResult(AuthenticateResult.Fail("not authenticated"))); + var schemeProvider = new Mock(); + schemeProvider.Setup(p => p.GetDefaultAuthenticateSchemeAsync()) + .Returns(Task.FromResult(new AuthenticationScheme("custom", "custom", typeof(JwtBearerHandler)))); + var middleware = new AuthenticationMiddleware(c => Task.CompletedTask, schemeProvider.Object); + var context = GetHttpContext(authenticationService: authenticationService.Object); + + // Act + await middleware.Invoke(context); + + // Assert + var authenticateResultFeature = context.Features.Get(); + Assert.Null(authenticateResultFeature); + } + + [Fact] + public async Task IAuthenticateResultFeature_NullResultWhenUserSetAfter() + { + var authenticationService = new Mock(); + authenticationService.Setup(s => s.AuthenticateAsync(It.IsAny(), It.IsAny())) + .Returns(Task.FromResult(AuthenticateResult.Success(new AuthenticationTicket(new ClaimsPrincipal(), "custom")))); + var schemeProvider = new Mock(); + schemeProvider.Setup(p => p.GetDefaultAuthenticateSchemeAsync()) + .Returns(Task.FromResult(new AuthenticationScheme("custom", "custom", typeof(JwtBearerHandler)))); + var middleware = new AuthenticationMiddleware(c => Task.CompletedTask, schemeProvider.Object); + var context = GetHttpContext(authenticationService: authenticationService.Object); + + // Act + await middleware.Invoke(context); + + // Assert + var authenticateResultFeature = context.Features.Get(); + Assert.NotNull(authenticateResultFeature); + Assert.NotNull(authenticateResultFeature.AuthenticateResult); + Assert.True(authenticateResultFeature.AuthenticateResult.Succeeded); + Assert.Same(context.User, authenticateResultFeature.AuthenticateResult.Principal); + + context.User = new ClaimsPrincipal(); + Assert.Null(authenticateResultFeature.AuthenticateResult); + } + + [Fact] + public async Task IAuthenticateResultFeature_SettingResultSetsUser() + { + var authenticationService = new Mock(); + authenticationService.Setup(s => s.AuthenticateAsync(It.IsAny(), It.IsAny())) + .Returns(Task.FromResult(AuthenticateResult.Success(new AuthenticationTicket(new ClaimsPrincipal(), "custom")))); + var schemeProvider = new Mock(); + schemeProvider.Setup(p => p.GetDefaultAuthenticateSchemeAsync()) + .Returns(Task.FromResult(new AuthenticationScheme("custom", "custom", typeof(JwtBearerHandler)))); + var middleware = new AuthenticationMiddleware(c => Task.CompletedTask, schemeProvider.Object); + var context = GetHttpContext(authenticationService: authenticationService.Object); + + // Act + await middleware.Invoke(context); + + // Assert + var authenticateResultFeature = context.Features.Get(); + Assert.NotNull(authenticateResultFeature); + Assert.NotNull(authenticateResultFeature.AuthenticateResult); + Assert.True(authenticateResultFeature.AuthenticateResult.Succeeded); + Assert.Same(context.User, authenticateResultFeature.AuthenticateResult.Principal); + + var newTicket = new AuthenticationTicket(new ClaimsPrincipal(), ""); + authenticateResultFeature.AuthenticateResult = AuthenticateResult.Success(newTicket); + Assert.Same(context.User, newTicket.Principal); + } + + private HttpContext GetHttpContext( + Action registerServices = null, + IAuthenticationService authenticationService = null) + { + // ServiceProvider + var serviceCollection = new ServiceCollection(); + + authenticationService = authenticationService ?? Mock.Of(); + + serviceCollection.AddSingleton(authenticationService); + serviceCollection.AddOptions(); + serviceCollection.AddLogging(); + serviceCollection.AddAuthentication(); + registerServices?.Invoke(serviceCollection); + + var serviceProvider = serviceCollection.BuildServiceProvider(); + + //// HttpContext + var httpContext = new DefaultHttpContext(); + httpContext.RequestServices = serviceProvider; + + return httpContext; + } + private class ThreeOhFiveHandler : StatusCodeHandler { public ThreeOhFiveHandler() : base(305) { } } @@ -77,7 +200,7 @@ public StatusCodeHandler(int code) { _code = code; } - + public Task AuthenticateAsync() { throw new NotImplementedException(); diff --git a/src/Security/Authorization/Policy/src/AuthenticationFeatures.cs b/src/Security/Authorization/Policy/src/AuthenticationFeatures.cs index fe19a6a9700d..c8660f679930 100644 --- a/src/Security/Authorization/Policy/src/AuthenticationFeatures.cs +++ b/src/Security/Authorization/Policy/src/AuthenticationFeatures.cs @@ -17,10 +17,10 @@ internal sealed class AuthenticationFeatures : IAuthenticateResultFeature, IHttp public AuthenticationFeatures(AuthenticateResult result) { - Result = result; + AuthenticateResult = result; } - public AuthenticateResult? Result + public AuthenticateResult? AuthenticateResult { get => _result; set diff --git a/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs b/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs index cb8e382232d4..f01060e01950 100644 --- a/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs +++ b/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs @@ -454,8 +454,8 @@ public async Task IAuthenticateResultFeature_SetOnSuccessfulAuthorize() Assert.True(next.Called); var authenticateResultFeature = context.Features.Get(); Assert.NotNull(authenticateResultFeature); - Assert.NotNull(authenticateResultFeature.Result); - Assert.Same(context.User, authenticateResultFeature.Result.Principal); + Assert.NotNull(authenticateResultFeature.AuthenticateResult); + Assert.Same(context.User, authenticateResultFeature.AuthenticateResult.Principal); } [Fact] @@ -517,9 +517,9 @@ public async Task IAuthenticateResultFeature_ContainsLowestExpiration() // Assert var authenticateResultFeature = context.Features.Get(); Assert.NotNull(authenticateResultFeature); - Assert.NotNull(authenticateResultFeature.Result); - Assert.Same(context.User, authenticateResultFeature.Result.Principal); - Assert.Equal(secondExpiration, authenticateResultFeature.Result?.Properties?.ExpiresUtc); + Assert.NotNull(authenticateResultFeature.AuthenticateResult); + Assert.Same(context.User, authenticateResultFeature.AuthenticateResult.Principal); + Assert.Equal(secondExpiration, authenticateResultFeature.AuthenticateResult?.Properties?.ExpiresUtc); } [Fact] @@ -541,11 +541,11 @@ public async Task IAuthenticateResultFeature_NullResultWhenUserSetAfter() Assert.True(next.Called); var authenticateResultFeature = context.Features.Get(); Assert.NotNull(authenticateResultFeature); - Assert.NotNull(authenticateResultFeature.Result); - Assert.Same(context.User, authenticateResultFeature.Result.Principal); + Assert.NotNull(authenticateResultFeature.AuthenticateResult); + Assert.Same(context.User, authenticateResultFeature.AuthenticateResult.Principal); context.User = new ClaimsPrincipal(); - Assert.Null(authenticateResultFeature.Result); + Assert.Null(authenticateResultFeature.AuthenticateResult); } [Fact] @@ -567,14 +567,47 @@ public async Task IAuthenticateResultFeature_SettingResultSetsUser() Assert.True(next.Called); var authenticateResultFeature = context.Features.Get(); Assert.NotNull(authenticateResultFeature); - Assert.NotNull(authenticateResultFeature.Result); - Assert.Same(context.User, authenticateResultFeature.Result.Principal); + Assert.NotNull(authenticateResultFeature.AuthenticateResult); + Assert.Same(context.User, authenticateResultFeature.AuthenticateResult.Principal); var newTicket = new AuthenticationTicket(new ClaimsPrincipal(), ""); - authenticateResultFeature.Result = AuthenticateResult.Success(newTicket); + authenticateResultFeature.AuthenticateResult = AuthenticateResult.Success(newTicket); Assert.Same(context.User, newTicket.Principal); } + class TestAuthResultFeature : IAuthenticateResultFeature + { + public AuthenticateResult AuthenticateResult { get; set; } + } + + [Fact] + public async Task IAuthenticateResultFeature_ReplacesExistingFeature() + { + // Arrange + var policy = new AuthorizationPolicyBuilder().RequireClaim("Permission", "CanViewPage").Build(); + var policyProvider = new Mock(); + policyProvider.Setup(p => p.GetDefaultPolicyAsync()).ReturnsAsync(policy); + var next = new TestRequestDelegate(); + + var middleware = CreateMiddleware(next.Invoke, policyProvider.Object); + var context = GetHttpContext(endpoint: CreateEndpoint(new AuthorizeAttribute())); + var testAuthenticateResultFeature = new TestAuthResultFeature(); + var authenticateResult = AuthenticateResult.Success(new AuthenticationTicket(new ClaimsPrincipal(), "")); + testAuthenticateResultFeature.AuthenticateResult = authenticateResult; + context.Features.Set(testAuthenticateResultFeature); + + // Act + await middleware.Invoke(context); + + // Assert + Assert.True(next.Called); + var authenticateResultFeature = context.Features.Get(); + Assert.NotNull(authenticateResultFeature); + Assert.NotNull(authenticateResultFeature.AuthenticateResult); + Assert.NotSame(testAuthenticateResultFeature, authenticateResultFeature); + Assert.NotSame(authenticateResult, authenticateResultFeature.AuthenticateResult); + } + private AuthorizationMiddleware CreateMiddleware(RequestDelegate requestDelegate = null, IAuthorizationPolicyProvider policyProvider = null) { requestDelegate = requestDelegate ?? ((context) => Task.CompletedTask); From 46493893e4cd83886a722fdf1c2fce154f9a58cb Mon Sep 17 00:00:00 2001 From: BrennanConroy Date: Fri, 25 Jun 2021 14:23:12 -0700 Subject: [PATCH 7/7] reuse --- .../Policy/src/AuthorizationMiddleware.cs | 13 ++++++++++--- .../test/AuthorizationMiddlewareTests.cs | 4 ++-- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs b/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs index 367d0624a2f4..a7bfa785adc0 100644 --- a/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs +++ b/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs @@ -73,9 +73,16 @@ public async Task Invoke(HttpContext context) if (authenticateResult?.Succeeded ?? false) { - var authFeatures = new AuthenticationFeatures(authenticateResult); - context.Features.Set(authFeatures); - context.Features.Set(authFeatures); + if (context.Features.Get() is IAuthenticateResultFeature authenticateResultFeature) + { + authenticateResultFeature.AuthenticateResult = authenticateResult; + } + else + { + var authFeatures = new AuthenticationFeatures(authenticateResult); + context.Features.Set(authFeatures); + context.Features.Set(authFeatures); + } } // Allow Anonymous still wants to run authorization to populate the User but skips any failure/challenge handling diff --git a/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs b/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs index f01060e01950..2ba4f6d4e453 100644 --- a/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs +++ b/src/Security/Authorization/test/AuthorizationMiddlewareTests.cs @@ -581,7 +581,7 @@ class TestAuthResultFeature : IAuthenticateResultFeature } [Fact] - public async Task IAuthenticateResultFeature_ReplacesExistingFeature() + public async Task IAuthenticateResultFeature_UsesExistingFeature() { // Arrange var policy = new AuthorizationPolicyBuilder().RequireClaim("Permission", "CanViewPage").Build(); @@ -604,7 +604,7 @@ public async Task IAuthenticateResultFeature_ReplacesExistingFeature() var authenticateResultFeature = context.Features.Get(); Assert.NotNull(authenticateResultFeature); Assert.NotNull(authenticateResultFeature.AuthenticateResult); - Assert.NotSame(testAuthenticateResultFeature, authenticateResultFeature); + Assert.Same(testAuthenticateResultFeature, authenticateResultFeature); Assert.NotSame(authenticateResult, authenticateResultFeature.AuthenticateResult); }