From 814a37548b6adae2f846eae3144e8f37c1388520 Mon Sep 17 00:00:00 2001 From: Hao Kung Date: Tue, 29 Oct 2019 15:48:06 -0700 Subject: [PATCH] Only run claims transformation once per ClaimsPrincipal instance by default (#12028) --- .../src/AuthenticationService.cs | 19 ++++++++- .../test/AuthenticationPropertiesTests.cs | 4 +- .../test/AuthenticationSchemeProviderTests.cs | 2 +- .../test/AuthenticationServiceTests.cs | 41 ++++++++++++++++++- .../test/TokenExtensionTests.cs | 2 +- .../test/SharedAuthenticationTests.cs | 40 ++++++++++++++++++ 6 files changed, 100 insertions(+), 8 deletions(-) diff --git a/src/Http/Authentication.Core/src/AuthenticationService.cs b/src/Http/Authentication.Core/src/AuthenticationService.cs index 9cc0807539..de63980c4a 100644 --- a/src/Http/Authentication.Core/src/AuthenticationService.cs +++ b/src/Http/Authentication.Core/src/AuthenticationService.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.Collections.Generic; using System.Linq; using System.Security.Claims; using System.Threading.Tasks; @@ -15,6 +16,8 @@ namespace Microsoft.AspNetCore.Authentication /// public class AuthenticationService : IAuthenticationService { + private HashSet _transformCache; + /// /// Constructor. /// @@ -77,8 +80,20 @@ namespace Microsoft.AspNetCore.Authentication var result = await handler.AuthenticateAsync(); if (result != null && result.Succeeded) { - var transformed = await Transform.TransformAsync(result.Principal); - return AuthenticateResult.Success(new AuthenticationTicket(transformed, result.Properties, result.Ticket.AuthenticationScheme)); + var principal = result.Principal; + var doTransform = true; + _transformCache ??= new HashSet(); + if (_transformCache.Contains(principal)) + { + doTransform = false; + } + + if (doTransform) + { + principal = await Transform.TransformAsync(principal); + _transformCache.Add(principal); + } + return AuthenticateResult.Success(new AuthenticationTicket(principal, result.Properties, result.Ticket.AuthenticationScheme)); } return result; } diff --git a/src/Http/Authentication.Core/test/AuthenticationPropertiesTests.cs b/src/Http/Authentication.Core/test/AuthenticationPropertiesTests.cs index 84db381ce4..c8a8056077 100644 --- a/src/Http/Authentication.Core/test/AuthenticationPropertiesTests.cs +++ b/src/Http/Authentication.Core/test/AuthenticationPropertiesTests.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Globalization; using System.Linq; @@ -298,4 +298,4 @@ namespace Microsoft.AspNetCore.Authentication.Core.Test } } } -} +} \ No newline at end of file diff --git a/src/Http/Authentication.Core/test/AuthenticationSchemeProviderTests.cs b/src/Http/Authentication.Core/test/AuthenticationSchemeProviderTests.cs index 8ef2f9d347..f422ee885d 100644 --- a/src/Http/Authentication.Core/test/AuthenticationSchemeProviderTests.cs +++ b/src/Http/Authentication.Core/test/AuthenticationSchemeProviderTests.cs @@ -11,7 +11,7 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; using Xunit; -namespace Microsoft.AspNetCore.Authentication +namespace Microsoft.AspNetCore.Authentication.Core.Test { public class AuthenticationSchemeProviderTests { diff --git a/src/Http/Authentication.Core/test/AuthenticationServiceTests.cs b/src/Http/Authentication.Core/test/AuthenticationServiceTests.cs index b62db22077..6134ca9172 100644 --- a/src/Http/Authentication.Core/test/AuthenticationServiceTests.cs +++ b/src/Http/Authentication.Core/test/AuthenticationServiceTests.cs @@ -8,7 +8,7 @@ using Microsoft.AspNetCore.Http; using Microsoft.Extensions.DependencyInjection; using Xunit; -namespace Microsoft.AspNetCore.Authentication +namespace Microsoft.AspNetCore.Authentication.Core.Test { public class AuthenticationServiceTests { @@ -27,6 +27,30 @@ namespace Microsoft.AspNetCore.Authentication Assert.Contains("base", ex.Message); } + [Fact] + public async Task CustomHandlersAuthenticateRunsClaimsTransformationEveryTime() + { + var transform = new RunOnce(); + var services = new ServiceCollection().AddOptions().AddAuthenticationCore(o => + { + o.AddScheme("base", "whatever"); + }) + .AddSingleton(transform) + .BuildServiceProvider(); + var context = new DefaultHttpContext(); + context.RequestServices = services; + + // Because base handler returns a different principal per call, its run multiple times + await context.AuthenticateAsync("base"); + Assert.Equal(1, transform.Ran); + + await context.AuthenticateAsync("base"); + Assert.Equal(2, transform.Ran); + + await context.AuthenticateAsync("base"); + Assert.Equal(3, transform.Ran); + } + [Fact] public async Task ChallengeThrowsForSchemeMismatch() { @@ -219,12 +243,25 @@ namespace Microsoft.AspNetCore.Authentication await context.ForbidAsync(); } + private class RunOnce : IClaimsTransformation + { + public int Ran = 0; + public Task TransformAsync(ClaimsPrincipal principal) + { + Ran++; + return Task.FromResult(new ClaimsPrincipal()); + } + } private class BaseHandler : IAuthenticationHandler { public Task AuthenticateAsync() { - return Task.FromResult(AuthenticateResult.NoResult()); + return Task.FromResult(AuthenticateResult.Success( + new AuthenticationTicket( + new ClaimsPrincipal(new ClaimsIdentity("whatever")), + new AuthenticationProperties(), + "whatever"))); } public Task ChallengeAsync(AuthenticationProperties properties) diff --git a/src/Http/Authentication.Core/test/TokenExtensionTests.cs b/src/Http/Authentication.Core/test/TokenExtensionTests.cs index 7215d526e9..49f47cf45f 100644 --- a/src/Http/Authentication.Core/test/TokenExtensionTests.cs +++ b/src/Http/Authentication.Core/test/TokenExtensionTests.cs @@ -10,7 +10,7 @@ using Microsoft.AspNetCore.Http; using Microsoft.Extensions.DependencyInjection; using Xunit; -namespace Microsoft.AspNetCore.Authentication +namespace Microsoft.AspNetCore.Authentication.Core.Test { public class TokenExtensionTests { diff --git a/src/Security/Authentication/test/SharedAuthenticationTests.cs b/src/Security/Authentication/test/SharedAuthenticationTests.cs index 36956f8374..834efeaedd 100644 --- a/src/Security/Authentication/test/SharedAuthenticationTests.cs +++ b/src/Security/Authentication/test/SharedAuthenticationTests.cs @@ -203,6 +203,46 @@ namespace Microsoft.AspNetCore.Authentication Assert.Equal(0, forwardDefault.SignOutCount); } + private class RunOnce : IClaimsTransformation + { + public int Ran = 0; + public Task TransformAsync(ClaimsPrincipal principal) + { + Ran++; + return Task.FromResult(new ClaimsPrincipal()); + } + } + + [Fact] + public async Task ForwardAuthenticateOnlyRunsTransformOnceByDefault() + { + var services = new ServiceCollection().AddLogging(); + var transform = new RunOnce(); + var builder = services.AddSingleton(transform).AddAuthentication(o => + { + o.DefaultScheme = DefaultScheme; + o.AddScheme("auth1", "auth1"); + o.AddScheme("specific", "specific"); + }); + RegisterAuth(builder, o => + { + o.ForwardDefault = "auth1"; + o.ForwardAuthenticate = "specific"; + }); + + var specific = new TestHandler(); + services.AddSingleton(specific); + var forwardDefault = new TestHandler2(); + services.AddSingleton(forwardDefault); + + var sp = services.BuildServiceProvider(); + var context = new DefaultHttpContext(); + context.RequestServices = sp; + + await context.AuthenticateAsync(); + Assert.Equal(1, transform.Ran); + } + [Fact] public async Task ForwardAuthenticateWinsOverDefault() {