From e50ed9c4ec59dc27074b7adde80ada0c46ac82b8 Mon Sep 17 00:00:00 2001 From: Mackinnon Buck Date: Wed, 18 Dec 2024 13:56:51 -0800 Subject: [PATCH 1/9] Add authentication metrics --- ...ticationCoreServiceCollectionExtensions.cs | 2 + .../src/AuthenticationMetrics.cs | 105 +++++++++ .../src/AuthenticationService.cs | 41 +++- ...soft.AspNetCore.Authentication.Core.csproj | 4 + .../src/PublicAPI.Unshipped.txt | 1 + .../test/AuthenticationMetricsTest.cs | 216 ++++++++++++++++++ ...soft.AspNetCore.Authentication.Test.csproj | 2 + 7 files changed, 369 insertions(+), 2 deletions(-) create mode 100644 src/Http/Authentication.Core/src/AuthenticationMetrics.cs create mode 100644 src/Security/Authentication/test/AuthenticationMetricsTest.cs diff --git a/src/Http/Authentication.Core/src/AuthenticationCoreServiceCollectionExtensions.cs b/src/Http/Authentication.Core/src/AuthenticationCoreServiceCollectionExtensions.cs index 9b73616527ba..363c41fe66e6 100644 --- a/src/Http/Authentication.Core/src/AuthenticationCoreServiceCollectionExtensions.cs +++ b/src/Http/Authentication.Core/src/AuthenticationCoreServiceCollectionExtensions.cs @@ -20,10 +20,12 @@ public static IServiceCollection AddAuthenticationCore(this IServiceCollection s { ArgumentNullException.ThrowIfNull(services); + services.AddMetrics(); services.TryAddScoped(); services.TryAddSingleton(); // Can be replaced with scoped ones that use DbContext services.TryAddScoped(); services.TryAddSingleton(); + services.TryAddSingleton(); return services; } diff --git a/src/Http/Authentication.Core/src/AuthenticationMetrics.cs b/src/Http/Authentication.Core/src/AuthenticationMetrics.cs new file mode 100644 index 000000000000..b6e17d7ecf00 --- /dev/null +++ b/src/Http/Authentication.Core/src/AuthenticationMetrics.cs @@ -0,0 +1,105 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Diagnostics; +using System.Diagnostics.Metrics; + +namespace Microsoft.AspNetCore.Authentication; + +internal sealed class AuthenticationMetrics : IDisposable +{ + public const string MeterName = "Microsoft.AspNetCore.Authentication"; + + private readonly Meter _meter; + private readonly Counter _authenticatedRequestCount; + private readonly Counter _challengeCount; + private readonly Counter _forbidCount; + private readonly Counter _signInCount; + private readonly Counter _signOutCount; + + public AuthenticationMetrics(IMeterFactory meterFactory) + { + _meter = meterFactory.Create(MeterName); + + _authenticatedRequestCount = _meter.CreateCounter( + "aspnetcore.authentication.authenticated_requests", + unit: "{request}", + description: "The total number of authenticated requests"); + + _challengeCount = _meter.CreateCounter( + "aspnetcore.authentication.challenges", + unit: "{request}", + description: "The total number of times a scheme is challenged"); + + _forbidCount = _meter.CreateCounter( + "aspnetcore.authentication.forbids", + unit: "{request}", + description: "The total number of times an authenticated user attempts to access a resource they are not permitted to access"); + + _signInCount = _meter.CreateCounter( + "aspnetcore.authentication.sign_ins", + unit: "{request}", + description: "The total number of times a principal is signed in"); + + _signOutCount = _meter.CreateCounter( + "aspnetcore.authentication.sign_outs", + unit: "{request}", + description: "The total number of times a scheme is signed out"); + } + + public void AuthenticatedRequest(string scheme, AuthenticateResult result) + { + if (_authenticatedRequestCount.Enabled) + { + var resultTagValue = result switch + { + { Succeeded: true } => "success", + { Failure: not null } => "failure", + { None: true } => "none", + _ => throw new UnreachableException($"Could not determine the result state of the {nameof(AuthenticateResult)}"), + }; + + _authenticatedRequestCount.Add(1, [ + new("aspnetcore.authentication.scheme", scheme), + new("aspnetcore.authentication.result", resultTagValue), + ]); + } + } + + public void Challenge(string scheme) + { + if (_challengeCount.Enabled) + { + _challengeCount.Add(1, [new("aspnetcore.authentication.scheme", scheme)]); + } + } + + public void Forbid(string scheme) + { + if (_forbidCount.Enabled) + { + _forbidCount.Add(1, [new("aspnetcore.authentication.scheme", scheme)]); + } + } + + public void SignIn(string scheme) + { + if (_signInCount.Enabled) + { + _signInCount.Add(1, [new("aspnetcore.authentication.scheme", scheme)]); + } + } + + public void SignOut(string scheme) + { + if (_signOutCount.Enabled) + { + _signOutCount.Add(1, [new("aspnetcore.authentication.scheme", scheme)]); + } + } + + public void Dispose() + { + _meter.Dispose(); + } +} diff --git a/src/Http/Authentication.Core/src/AuthenticationService.cs b/src/Http/Authentication.Core/src/AuthenticationService.cs index 027886286ef3..a825c85643a6 100644 --- a/src/Http/Authentication.Core/src/AuthenticationService.cs +++ b/src/Http/Authentication.Core/src/AuthenticationService.cs @@ -4,6 +4,7 @@ using System.Linq; using System.Security.Claims; using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; namespace Microsoft.AspNetCore.Authentication; @@ -13,6 +14,8 @@ namespace Microsoft.AspNetCore.Authentication; /// public class AuthenticationService : IAuthenticationService { + private readonly AuthenticationMetrics? _metrics; + private HashSet? _transformCache; /// @@ -22,12 +25,36 @@ public class AuthenticationService : IAuthenticationService /// The . /// The . /// The . - public AuthenticationService(IAuthenticationSchemeProvider schemes, IAuthenticationHandlerProvider handlers, IClaimsTransformation transform, IOptions options) + public AuthenticationService( + IAuthenticationSchemeProvider schemes, + IAuthenticationHandlerProvider handlers, + IClaimsTransformation transform, + IOptions options) + : this(schemes, handlers, transform, options, services: null) + { + } + + /// + /// Constructor. + /// + /// The . + /// The . + /// The . + /// The . + /// The . + public AuthenticationService( + IAuthenticationSchemeProvider schemes, + IAuthenticationHandlerProvider handlers, + IClaimsTransformation transform, + IOptions options, + IServiceProvider? services) { Schemes = schemes; Handlers = handlers; Transform = transform; Options = options.Value; + + _metrics = services?.GetService(); } /// @@ -77,11 +104,13 @@ public virtual async Task AuthenticateAsync(HttpContext cont // Handlers should not return null, but we'll be tolerant of null values for legacy reasons. var result = (await handler.AuthenticateAsync()) ?? AuthenticateResult.NoResult(); + _metrics?.AuthenticatedRequest(scheme, result); + if (result.Succeeded) { var principal = result.Principal!; var doTransform = true; - _transformCache ??= new HashSet(); + _transformCache ??= []; if (_transformCache.Contains(principal)) { doTransform = false; @@ -122,6 +151,8 @@ public virtual async Task ChallengeAsync(HttpContext context, string? scheme, Au throw await CreateMissingHandlerException(scheme); } + _metrics?.Challenge(scheme); + await handler.ChallengeAsync(properties); } @@ -150,6 +181,8 @@ public virtual async Task ForbidAsync(HttpContext context, string? scheme, Authe throw await CreateMissingHandlerException(scheme); } + _metrics?.Forbid(scheme); + await handler.ForbidAsync(properties); } @@ -199,6 +232,8 @@ public virtual async Task SignInAsync(HttpContext context, string? scheme, Claim throw await CreateMismatchedSignInHandlerException(scheme, handler); } + _metrics?.SignIn(scheme); + await signInHandler.SignInAsync(principal, properties); } @@ -233,6 +268,8 @@ public virtual async Task SignOutAsync(HttpContext context, string? scheme, Auth throw await CreateMismatchedSignOutHandlerException(scheme, handler); } + _metrics?.SignOut(scheme); + await signOutHandler.SignOutAsync(properties); } diff --git a/src/Http/Authentication.Core/src/Microsoft.AspNetCore.Authentication.Core.csproj b/src/Http/Authentication.Core/src/Microsoft.AspNetCore.Authentication.Core.csproj index 5786fd9a889a..b03dd65d0f62 100644 --- a/src/Http/Authentication.Core/src/Microsoft.AspNetCore.Authentication.Core.csproj +++ b/src/Http/Authentication.Core/src/Microsoft.AspNetCore.Authentication.Core.csproj @@ -16,4 +16,8 @@ + + + + diff --git a/src/Http/Authentication.Core/src/PublicAPI.Unshipped.txt b/src/Http/Authentication.Core/src/PublicAPI.Unshipped.txt index 7dc5c58110bf..a5e375235430 100644 --- a/src/Http/Authentication.Core/src/PublicAPI.Unshipped.txt +++ b/src/Http/Authentication.Core/src/PublicAPI.Unshipped.txt @@ -1 +1,2 @@ #nullable enable +Microsoft.AspNetCore.Authentication.AuthenticationService.AuthenticationService(Microsoft.AspNetCore.Authentication.IAuthenticationSchemeProvider! schemes, Microsoft.AspNetCore.Authentication.IAuthenticationHandlerProvider! handlers, Microsoft.AspNetCore.Authentication.IClaimsTransformation! transform, Microsoft.Extensions.Options.IOptions! options, System.IServiceProvider? services) -> void diff --git a/src/Security/Authentication/test/AuthenticationMetricsTest.cs b/src/Security/Authentication/test/AuthenticationMetricsTest.cs new file mode 100644 index 000000000000..9165f17b0c87 --- /dev/null +++ b/src/Security/Authentication/test/AuthenticationMetricsTest.cs @@ -0,0 +1,216 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Security.Claims; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.InternalTesting; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using Microsoft.Extensions.Diagnostics.Metrics.Testing; +using Moq; + +namespace Microsoft.AspNetCore.Authentication; + +public class AuthenticationMetricsTest +{ + [Fact] + public async Task Authenticate_Success() + { + // Arrange + var authenticationHandler = new Mock(); + authenticationHandler.Setup(h => h.AuthenticateAsync()).Returns(Task.FromResult(AuthenticateResult.Success(new AuthenticationTicket(new ClaimsPrincipal(), "custom")))); + + var meterFactory = new TestMeterFactory(); + var httpContext = new DefaultHttpContext(); + var authenticationService = CreateAuthenticationService(authenticationHandler.Object, meterFactory); + var meter = meterFactory.Meters.Single(); + + using var authenticationRequestsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.authenticated_requests"); + + // Act + await authenticationService.AuthenticateAsync(httpContext, scheme: "custom"); + + // Assert + Assert.Equal(AuthenticationMetrics.MeterName, meter.Name); + Assert.Null(meter.Version); + + var measurement = Assert.Single(authenticationRequestsCollector.GetMeasurementSnapshot()); + Assert.Equal(1, measurement.Value); + Assert.Equal("custom", (string)measurement.Tags["aspnetcore.authentication.scheme"]); + Assert.Equal("success", (string)measurement.Tags["aspnetcore.authentication.result"]); + } + + [Fact] + public async Task Authenticate_Failure() + { + // Arrange + var authenticationHandler = new Mock(); + authenticationHandler.Setup(h => h.AuthenticateAsync()).Returns(Task.FromResult(AuthenticateResult.Fail("Authentication failed"))); + + var meterFactory = new TestMeterFactory(); + var httpContext = new DefaultHttpContext(); + var authenticationService = CreateAuthenticationService(authenticationHandler.Object, meterFactory); + var meter = meterFactory.Meters.Single(); + + using var authenticationRequestsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.authenticated_requests"); + + // Act + await authenticationService.AuthenticateAsync(httpContext, scheme: "custom"); + + // Assert + Assert.Equal(AuthenticationMetrics.MeterName, meter.Name); + Assert.Null(meter.Version); + + var measurement = Assert.Single(authenticationRequestsCollector.GetMeasurementSnapshot()); + Assert.Equal(1, measurement.Value); + Assert.Equal("custom", (string)measurement.Tags["aspnetcore.authentication.scheme"]); + Assert.Equal("failure", (string)measurement.Tags["aspnetcore.authentication.result"]); + } + + [Fact] + public async Task Authenticate_NoResult() + { + // Arrange + var authenticationHandler = new Mock(); + authenticationHandler.Setup(h => h.AuthenticateAsync()).Returns(Task.FromResult(AuthenticateResult.NoResult())); + + var meterFactory = new TestMeterFactory(); + var httpContext = new DefaultHttpContext(); + var authenticationService = CreateAuthenticationService(authenticationHandler.Object, meterFactory); + var meter = meterFactory.Meters.Single(); + + using var authenticationRequestsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.authenticated_requests"); + + // Act + await authenticationService.AuthenticateAsync(httpContext, scheme: "custom"); + + // Assert + Assert.Equal(AuthenticationMetrics.MeterName, meter.Name); + Assert.Null(meter.Version); + + var measurement = Assert.Single(authenticationRequestsCollector.GetMeasurementSnapshot()); + Assert.Equal(1, measurement.Value); + Assert.Equal("custom", (string)measurement.Tags["aspnetcore.authentication.scheme"]); + Assert.Equal("none", (string)measurement.Tags["aspnetcore.authentication.result"]); + } + + [Fact] + public async Task Challenge() + { + // Arrange + var meterFactory = new TestMeterFactory(); + var httpContext = new DefaultHttpContext(); + var authenticationService = CreateAuthenticationService(Mock.Of(), meterFactory); + var meter = meterFactory.Meters.Single(); + + using var challengesCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.challenges"); + + // Act + await authenticationService.ChallengeAsync(httpContext, scheme: "custom", properties: null); + + // Assert + Assert.Equal(AuthenticationMetrics.MeterName, meter.Name); + Assert.Null(meter.Version); + + var measurement = Assert.Single(challengesCollector.GetMeasurementSnapshot()); + Assert.Equal(1, measurement.Value); + Assert.Equal("custom", (string)measurement.Tags["aspnetcore.authentication.scheme"]); + } + + [Fact] + public async Task Forbid() + { + // Arrange + var meterFactory = new TestMeterFactory(); + var httpContext = new DefaultHttpContext(); + var authenticationService = CreateAuthenticationService(Mock.Of(), meterFactory); + var meter = meterFactory.Meters.Single(); + + using var forbidsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.forbids"); + + // Act + await authenticationService.ForbidAsync(httpContext, scheme: "custom", properties: null); + + // Assert + Assert.Equal(AuthenticationMetrics.MeterName, meter.Name); + Assert.Null(meter.Version); + + var measurement = Assert.Single(forbidsCollector.GetMeasurementSnapshot()); + Assert.Equal(1, measurement.Value); + Assert.Equal("custom", (string)measurement.Tags["aspnetcore.authentication.scheme"]); + } + + [Fact] + public async Task SignIn() + { + // Arrange + var meterFactory = new TestMeterFactory(); + var httpContext = new DefaultHttpContext(); + var authenticationService = CreateAuthenticationService(Mock.Of(), meterFactory); + var meter = meterFactory.Meters.Single(); + + using var signInsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.sign_ins"); + + // Act + await authenticationService.SignInAsync(httpContext, scheme: "custom", new ClaimsPrincipal(), properties: null); + + // Assert + Assert.Equal(AuthenticationMetrics.MeterName, meter.Name); + Assert.Null(meter.Version); + + var measurement = Assert.Single(signInsCollector.GetMeasurementSnapshot()); + Assert.Equal(1, measurement.Value); + Assert.Equal("custom", (string)measurement.Tags["aspnetcore.authentication.scheme"]); + } + + [Fact] + public async Task SignOut() + { + // Arrange + var httpContext = new DefaultHttpContext(); + var meterFactory = new TestMeterFactory(); + var authenticationService = CreateAuthenticationService(Mock.Of(), meterFactory); + var meter = meterFactory.Meters.Single(); + + using var signOutsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.sign_outs"); + + // Act + await authenticationService.SignOutAsync(httpContext, scheme: "custom", properties: null); + + // Assert + Assert.Equal(AuthenticationMetrics.MeterName, meter.Name); + Assert.Null(meter.Version); + + var measurement = Assert.Single(signOutsCollector.GetMeasurementSnapshot()); + Assert.Equal(1, measurement.Value); + Assert.Equal("custom", (string)measurement.Tags["aspnetcore.authentication.scheme"]); + } + + private static AuthenticationService CreateAuthenticationService(IAuthenticationHandler authenticationHandler, TestMeterFactory meterFactory) + { + var authenticationHandlerProvider = new Mock(); + authenticationHandlerProvider.Setup(p => p.GetHandlerAsync(It.IsAny(), "custom")).Returns(Task.FromResult(authenticationHandler)); + + var claimsTransform = new Mock(); + claimsTransform.Setup(t => t.TransformAsync(It.IsAny())).Returns((ClaimsPrincipal p) => Task.FromResult(p)); + + var options = Options.Create(new AuthenticationOptions + { + DefaultSignInScheme = "custom", + RequireAuthenticatedSignIn = false, + }); + + var serviceCollection = new ServiceCollection(); + serviceCollection.AddSingleton(new AuthenticationMetrics(meterFactory)); + var services = serviceCollection.BuildServiceProvider(); + + var authenticationService = new AuthenticationService( + Mock.Of(), + authenticationHandlerProvider.Object, + claimsTransform.Object, + options, + services); + + return authenticationService; + } +} diff --git a/src/Security/Authentication/test/Microsoft.AspNetCore.Authentication.Test.csproj b/src/Security/Authentication/test/Microsoft.AspNetCore.Authentication.Test.csproj index d94b1c3cbc57..64bcf143aa82 100644 --- a/src/Security/Authentication/test/Microsoft.AspNetCore.Authentication.Test.csproj +++ b/src/Security/Authentication/test/Microsoft.AspNetCore.Authentication.Test.csproj @@ -14,6 +14,7 @@ + PreserveNewest @@ -49,6 +50,7 @@ + From d6309e2fdaf2c5441d038b53d77deb510561dc95 Mon Sep 17 00:00:00 2001 From: Mackinnon Buck Date: Wed, 18 Dec 2024 15:30:41 -0800 Subject: [PATCH 2/9] Add authorization metrics --- .../Core/src/AuthorizationMetrics.cs | 48 ++++++ ...uthorizationServiceCollectionExtensions.cs | 3 + .../Core/src/DefaultAuthorizationService.cs | 87 ++++++++--- .../Microsoft.AspNetCore.Authorization.csproj | 5 + .../PublicAPI/net10.0/PublicAPI.Unshipped.txt | 1 + .../PublicAPI/net462/PublicAPI.Unshipped.txt | 1 + .../netstandard2.0/PublicAPI.Unshipped.txt | 1 + .../test/AuthorizationMetricsTest.cs | 138 ++++++++++++++++++ ...osoft.AspNetCore.Authorization.Test.csproj | 5 + 9 files changed, 265 insertions(+), 24 deletions(-) create mode 100644 src/Security/Authorization/Core/src/AuthorizationMetrics.cs create mode 100644 src/Security/Authorization/test/AuthorizationMetricsTest.cs diff --git a/src/Security/Authorization/Core/src/AuthorizationMetrics.cs b/src/Security/Authorization/Core/src/AuthorizationMetrics.cs new file mode 100644 index 000000000000..58ab534f962a --- /dev/null +++ b/src/Security/Authorization/Core/src/AuthorizationMetrics.cs @@ -0,0 +1,48 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.Diagnostics.Metrics; +using System.Linq; +using System.Text; +using System.Threading.Tasks; + +namespace Microsoft.AspNetCore.Authorization; + +internal sealed class AuthorizationMetrics : IDisposable +{ + public const string MeterName = "Microsoft.AspNetCore.Authorization"; + + private readonly Meter _meter; + private readonly Counter _authorizeCount; + + public AuthorizationMetrics(IMeterFactory meterFactory) + { + _meter = meterFactory.Create(MeterName); + + _authorizeCount = _meter.CreateCounter( + "aspnetcore.authorization.authorized_requests", + unit: "{request}", + description: "The total number of requests requiring authorization"); + } + + public void AuthorizedRequest(string? policyName, AuthorizationResult result) + { + if (_authorizeCount.Enabled) + { + var resultTagValue = result.Succeeded ? "success" : "failure"; + + _authorizeCount.Add(1, [ + new("aspnetcore.authorization.policy", policyName), + new("aspnetcore.authorization.result", resultTagValue), + ]); + } + } + + public void Dispose() + { + _meter.Dispose(); + } +} diff --git a/src/Security/Authorization/Core/src/AuthorizationServiceCollectionExtensions.cs b/src/Security/Authorization/Core/src/AuthorizationServiceCollectionExtensions.cs index 273f3c5ff563..8a9df713c046 100644 --- a/src/Security/Authorization/Core/src/AuthorizationServiceCollectionExtensions.cs +++ b/src/Security/Authorization/Core/src/AuthorizationServiceCollectionExtensions.cs @@ -27,6 +27,9 @@ public static IServiceCollection AddAuthorizationCore(this IServiceCollection se // aren't included by default. services.AddOptions(); + services.AddMetrics(); + + services.TryAdd(ServiceDescriptor.Singleton()); services.TryAdd(ServiceDescriptor.Transient()); services.TryAdd(ServiceDescriptor.Transient()); services.TryAdd(ServiceDescriptor.Transient()); diff --git a/src/Security/Authorization/Core/src/DefaultAuthorizationService.cs b/src/Security/Authorization/Core/src/DefaultAuthorizationService.cs index 24073df62676..16f749cadd87 100644 --- a/src/Security/Authorization/Core/src/DefaultAuthorizationService.cs +++ b/src/Security/Authorization/Core/src/DefaultAuthorizationService.cs @@ -6,6 +6,7 @@ using System.Security.Claims; using System.Threading.Tasks; using Microsoft.AspNetCore.Shared; +using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; @@ -17,6 +18,7 @@ namespace Microsoft.AspNetCore.Authorization; public class DefaultAuthorizationService : IAuthorizationService { private readonly AuthorizationOptions _options; + private readonly AuthorizationMetrics? _metrics; private readonly IAuthorizationHandlerContextFactory _contextFactory; private readonly IAuthorizationHandlerProvider _handlers; private readonly IAuthorizationEvaluator _evaluator; @@ -32,7 +34,35 @@ public class DefaultAuthorizationService : IAuthorizationService /// The used to create the context to handle the authorization. /// The used to determine if authorization was successful. /// The used. - public DefaultAuthorizationService(IAuthorizationPolicyProvider policyProvider, IAuthorizationHandlerProvider handlers, ILogger logger, IAuthorizationHandlerContextFactory contextFactory, IAuthorizationEvaluator evaluator, IOptions options) + public DefaultAuthorizationService( + IAuthorizationPolicyProvider policyProvider, + IAuthorizationHandlerProvider handlers, + ILogger logger, + IAuthorizationHandlerContextFactory contextFactory, + IAuthorizationEvaluator evaluator, + IOptions options) + : this(policyProvider, handlers, logger, contextFactory, evaluator, options, services: null) + { + } + + /// + /// Creates a new instance of . + /// + /// The used to provide policies. + /// The handlers used to fulfill s. + /// The logger used to log messages, warnings and errors. + /// The used to create the context to handle the authorization. + /// The used to determine if authorization was successful. + /// The used. + /// The used to provide other services. + public DefaultAuthorizationService( + IAuthorizationPolicyProvider policyProvider, + IAuthorizationHandlerProvider handlers, + ILogger logger, + IAuthorizationHandlerContextFactory contextFactory, + IAuthorizationEvaluator evaluator, + IOptions options, + IServiceProvider? services) { ArgumentNullThrowHelper.ThrowIfNull(options); ArgumentNullThrowHelper.ThrowIfNull(policyProvider); @@ -47,6 +77,7 @@ public DefaultAuthorizationService(IAuthorizationPolicyProvider policyProvider, _logger = logger; _evaluator = evaluator; _contextFactory = contextFactory; + _metrics = services?.GetService(); } /// @@ -59,7 +90,33 @@ public DefaultAuthorizationService(IAuthorizationPolicyProvider policyProvider, /// A flag indicating whether authorization has succeeded. /// This value is true when the user fulfills the policy, otherwise false. /// - public virtual async Task AuthorizeAsync(ClaimsPrincipal user, object? resource, IEnumerable requirements) + public virtual Task AuthorizeAsync(ClaimsPrincipal user, object? resource, IEnumerable requirements) + => AuthorizeCoreAsync(user, resource, requirements, policyName: null); + + /// + /// Checks if a user meets a specific authorization policy. + /// + /// The user to check the policy against. + /// The resource the policy should be checked with. + /// The name of the policy to check against a specific context. + /// + /// A flag indicating whether authorization has succeeded. + /// This value is true when the user fulfills the policy otherwise false. + /// + public virtual async Task AuthorizeAsync(ClaimsPrincipal user, object? resource, string policyName) + { + ArgumentNullThrowHelper.ThrowIfNull(policyName); + + var policy = await _policyProvider.GetPolicyAsync(policyName).ConfigureAwait(false); + if (policy == null) + { + throw new InvalidOperationException($"No policy found: {policyName}."); + } + + return await AuthorizeCoreAsync(user, resource, policy.Requirements, policyName).ConfigureAwait(false); + } + + private async Task AuthorizeCoreAsync(ClaimsPrincipal user, object? resource, IEnumerable requirements, string? policyName) { ArgumentNullThrowHelper.ThrowIfNull(requirements); @@ -75,6 +132,9 @@ public virtual async Task AuthorizeAsync(ClaimsPrincipal us } var result = _evaluator.Evaluate(authContext); + + _metrics?.AuthorizedRequest(policyName, result); + if (result.Succeeded) { _logger.UserAuthorizationSucceeded(); @@ -83,28 +143,7 @@ public virtual async Task AuthorizeAsync(ClaimsPrincipal us { _logger.UserAuthorizationFailed(result.Failure); } - return result; - } - /// - /// Checks if a user meets a specific authorization policy. - /// - /// The user to check the policy against. - /// The resource the policy should be checked with. - /// The name of the policy to check against a specific context. - /// - /// A flag indicating whether authorization has succeeded. - /// This value is true when the user fulfills the policy otherwise false. - /// - public virtual async Task AuthorizeAsync(ClaimsPrincipal user, object? resource, string policyName) - { - ArgumentNullThrowHelper.ThrowIfNull(policyName); - - var policy = await _policyProvider.GetPolicyAsync(policyName).ConfigureAwait(false); - if (policy == null) - { - throw new InvalidOperationException($"No policy found: {policyName}."); - } - return await this.AuthorizeAsync(user, resource, policy).ConfigureAwait(false); + return result; } } diff --git a/src/Security/Authorization/Core/src/Microsoft.AspNetCore.Authorization.csproj b/src/Security/Authorization/Core/src/Microsoft.AspNetCore.Authorization.csproj index 44df5654138a..1d2f6d562069 100644 --- a/src/Security/Authorization/Core/src/Microsoft.AspNetCore.Authorization.csproj +++ b/src/Security/Authorization/Core/src/Microsoft.AspNetCore.Authorization.csproj @@ -17,6 +17,7 @@ Microsoft.AspNetCore.Authorization.AuthorizeAttribute + @@ -31,4 +32,8 @@ Microsoft.AspNetCore.Authorization.AuthorizeAttribute + + + + diff --git a/src/Security/Authorization/Core/src/PublicAPI/net10.0/PublicAPI.Unshipped.txt b/src/Security/Authorization/Core/src/PublicAPI/net10.0/PublicAPI.Unshipped.txt index 7dc5c58110bf..26a7cc3253bb 100644 --- a/src/Security/Authorization/Core/src/PublicAPI/net10.0/PublicAPI.Unshipped.txt +++ b/src/Security/Authorization/Core/src/PublicAPI/net10.0/PublicAPI.Unshipped.txt @@ -1 +1,2 @@ #nullable enable +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, System.IServiceProvider? services) -> void diff --git a/src/Security/Authorization/Core/src/PublicAPI/net462/PublicAPI.Unshipped.txt b/src/Security/Authorization/Core/src/PublicAPI/net462/PublicAPI.Unshipped.txt index 7dc5c58110bf..26a7cc3253bb 100644 --- a/src/Security/Authorization/Core/src/PublicAPI/net462/PublicAPI.Unshipped.txt +++ b/src/Security/Authorization/Core/src/PublicAPI/net462/PublicAPI.Unshipped.txt @@ -1 +1,2 @@ #nullable enable +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, System.IServiceProvider? services) -> void diff --git a/src/Security/Authorization/Core/src/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt b/src/Security/Authorization/Core/src/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt index 7dc5c58110bf..26a7cc3253bb 100644 --- a/src/Security/Authorization/Core/src/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt +++ b/src/Security/Authorization/Core/src/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt @@ -1 +1,2 @@ #nullable enable +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, System.IServiceProvider? services) -> void diff --git a/src/Security/Authorization/test/AuthorizationMetricsTest.cs b/src/Security/Authorization/test/AuthorizationMetricsTest.cs new file mode 100644 index 000000000000..debe06c445c2 --- /dev/null +++ b/src/Security/Authorization/test/AuthorizationMetricsTest.cs @@ -0,0 +1,138 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Security.Claims; +using Microsoft.AspNetCore.InternalTesting; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Diagnostics.Metrics.Testing; + +namespace Microsoft.AspNetCore.Authorization.Test; + +public class AuthorizationMetricsTest +{ + [Fact] + public async Task Authorize_WithPolicyName_Success() + { + // Arrange + var meterFactory = new TestMeterFactory(); + var authorizationService = BuildAuthorizationService(meterFactory); + var meter = meterFactory.Meters.Single(); + var user = new ClaimsPrincipal(new ClaimsIdentity([new Claim("Permission", "CanViewPage")])); + + using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.authorized_requests"); + + // Act + await authorizationService.AuthorizeAsync(user, "Basic"); + + // Assert + Assert.Equal(AuthorizationMetrics.MeterName, meter.Name); + Assert.Null(meter.Version); + + var measurement = Assert.Single(authorizedRequestsCollector.GetMeasurementSnapshot()); + Assert.Equal(1, measurement.Value); + Assert.Equal("Basic", (string)measurement.Tags["aspnetcore.authorization.policy"]); + Assert.Equal("success", (string)measurement.Tags["aspnetcore.authorization.result"]); + } + + [Fact] + public async Task Authorize_WithPolicyName_Failure() + { + // Arrange + var meterFactory = new TestMeterFactory(); + var authorizationService = BuildAuthorizationService(meterFactory); + var meter = meterFactory.Meters.Single(); + var user = new ClaimsPrincipal(new ClaimsIdentity([])); // Will fail due to missing required claim + + using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.authorized_requests"); + + // Act + await authorizationService.AuthorizeAsync(user, "Basic"); + + // Assert + Assert.Equal(AuthorizationMetrics.MeterName, meter.Name); + Assert.Null(meter.Version); + + var measurement = Assert.Single(authorizedRequestsCollector.GetMeasurementSnapshot()); + Assert.Equal(1, measurement.Value); + Assert.Equal("Basic", (string)measurement.Tags["aspnetcore.authorization.policy"]); + Assert.Equal("failure", (string)measurement.Tags["aspnetcore.authorization.result"]); + } + + [Fact] + public async Task Authorize_WithoutPolicyName_Success() + { + // Arrange + var meterFactory = new TestMeterFactory(); + var authorizationService = BuildAuthorizationService(meterFactory, services => + { + services.AddSingleton(new AlwaysHandler(succeed: true)); + }); + var meter = meterFactory.Meters.Single(); + var user = new ClaimsPrincipal(new ClaimsIdentity([])); + + using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.authorized_requests"); + + // Act + await authorizationService.AuthorizeAsync(user, resource: null, new TestRequirement()); + + // Assert + Assert.Equal(AuthorizationMetrics.MeterName, meter.Name); + Assert.Null(meter.Version); + + var measurement = Assert.Single(authorizedRequestsCollector.GetMeasurementSnapshot()); + Assert.Equal(1, measurement.Value); + Assert.Null(measurement.Tags["aspnetcore.authorization.policy"]); + Assert.Equal("success", (string)measurement.Tags["aspnetcore.authorization.result"]); + } + + [Fact] + public async Task Authorize_WithoutPolicyName_Failure() + { + // Arrange + var meterFactory = new TestMeterFactory(); + var authorizationService = BuildAuthorizationService(meterFactory); // Will fail because there is no handler registered + var meter = meterFactory.Meters.Single(); + var user = new ClaimsPrincipal(new ClaimsIdentity([])); + + using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.authorized_requests"); + + // Act + await authorizationService.AuthorizeAsync(user, resource: null, new TestRequirement()); + + // Assert + Assert.Equal(AuthorizationMetrics.MeterName, meter.Name); + Assert.Null(meter.Version); + + var measurement = Assert.Single(authorizedRequestsCollector.GetMeasurementSnapshot()); + Assert.Equal(1, measurement.Value); + Assert.Null(measurement.Tags["aspnetcore.authorization.policy"]); + Assert.Equal("failure", (string)measurement.Tags["aspnetcore.authorization.result"]); + } + + private static IAuthorizationService BuildAuthorizationService(TestMeterFactory meterFactory, Action setupServices = null) + { + var services = new ServiceCollection(); + services.AddSingleton(new AuthorizationMetrics(meterFactory)); + services.AddAuthorizationBuilder() + .AddPolicy("Basic", policy => policy.RequireClaim("Permission", "CanViewPage")); + services.AddLogging(); + services.AddOptions(); + setupServices?.Invoke(services); + return services.BuildServiceProvider().GetRequiredService(); + } + + private sealed class AlwaysHandler(bool succeed) : AuthorizationHandler + { + protected override Task HandleRequirementAsync(AuthorizationHandlerContext context, TestRequirement requirement) + { + if (succeed) + { + context.Succeed(requirement); + } + + return Task.CompletedTask; + } + } + + private sealed class TestRequirement : IAuthorizationRequirement; +} diff --git a/src/Security/Authorization/test/Microsoft.AspNetCore.Authorization.Test.csproj b/src/Security/Authorization/test/Microsoft.AspNetCore.Authorization.Test.csproj index 11fe6d5589af..8eeed805c2c9 100644 --- a/src/Security/Authorization/test/Microsoft.AspNetCore.Authorization.Test.csproj +++ b/src/Security/Authorization/test/Microsoft.AspNetCore.Authorization.Test.csproj @@ -10,7 +10,12 @@ + + + + + From b08e43692b3c9468e29b583e6644b770e0497c2c Mon Sep 17 00:00:00 2001 From: Mackinnon Buck Date: Thu, 19 Dec 2024 10:59:22 -0800 Subject: [PATCH 3/9] Add missing reference --- .../src/Microsoft.AspNetCore.Authentication.Core.csproj | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Http/Authentication.Core/src/Microsoft.AspNetCore.Authentication.Core.csproj b/src/Http/Authentication.Core/src/Microsoft.AspNetCore.Authentication.Core.csproj index b03dd65d0f62..49e2634ae0e5 100644 --- a/src/Http/Authentication.Core/src/Microsoft.AspNetCore.Authentication.Core.csproj +++ b/src/Http/Authentication.Core/src/Microsoft.AspNetCore.Authentication.Core.csproj @@ -14,6 +14,7 @@ + From 6c69508495f3951ed7c211661b9fd64be3fd1b18 Mon Sep 17 00:00:00 2001 From: Mackinnon Buck Date: Wed, 8 Jan 2025 15:02:33 -0800 Subject: [PATCH 4/9] PR feedback for authentication metrics --- ...ticationCoreServiceCollectionExtensions.cs | 2 +- .../src/AuthenticationMetrics.cs | 200 +++++++++++++++--- .../src/AuthenticationService.cs | 74 +------ .../src/AuthenticationServiceImpl.cs | 96 +++++++++ ...soft.AspNetCore.Authentication.Core.csproj | 4 + .../src/PublicAPI.Unshipped.txt | 1 - src/Http/startvscode.cmd | 3 + .../test/AuthenticationMetricsTest.cs | 163 ++++++++++++-- src/Security/startvscode.cmd | 3 + 9 files changed, 438 insertions(+), 108 deletions(-) create mode 100644 src/Http/Authentication.Core/src/AuthenticationServiceImpl.cs create mode 100644 src/Http/startvscode.cmd create mode 100644 src/Security/startvscode.cmd diff --git a/src/Http/Authentication.Core/src/AuthenticationCoreServiceCollectionExtensions.cs b/src/Http/Authentication.Core/src/AuthenticationCoreServiceCollectionExtensions.cs index 363c41fe66e6..be04d14e3717 100644 --- a/src/Http/Authentication.Core/src/AuthenticationCoreServiceCollectionExtensions.cs +++ b/src/Http/Authentication.Core/src/AuthenticationCoreServiceCollectionExtensions.cs @@ -21,7 +21,7 @@ public static IServiceCollection AddAuthenticationCore(this IServiceCollection s ArgumentNullException.ThrowIfNull(services); services.AddMetrics(); - services.TryAddScoped(); + services.TryAddScoped(); services.TryAddSingleton(); // Can be replaced with scoped ones that use DbContext services.TryAddScoped(); services.TryAddSingleton(); diff --git a/src/Http/Authentication.Core/src/AuthenticationMetrics.cs b/src/Http/Authentication.Core/src/AuthenticationMetrics.cs index b6e17d7ecf00..9e11fb23064c 100644 --- a/src/Http/Authentication.Core/src/AuthenticationMetrics.cs +++ b/src/Http/Authentication.Core/src/AuthenticationMetrics.cs @@ -3,6 +3,8 @@ using System.Diagnostics; using System.Diagnostics.Metrics; +using System.Runtime.CompilerServices; +using Microsoft.AspNetCore.Http; namespace Microsoft.AspNetCore.Authentication; @@ -11,7 +13,7 @@ internal sealed class AuthenticationMetrics : IDisposable public const string MeterName = "Microsoft.AspNetCore.Authentication"; private readonly Meter _meter; - private readonly Counter _authenticatedRequestCount; + private readonly Histogram _authenticatedRequestDuration; private readonly Counter _challengeCount; private readonly Counter _forbidCount; private readonly Counter _signInCount; @@ -21,10 +23,11 @@ public AuthenticationMetrics(IMeterFactory meterFactory) { _meter = meterFactory.Create(MeterName); - _authenticatedRequestCount = _meter.CreateCounter( - "aspnetcore.authentication.authenticated_requests", + _authenticatedRequestDuration = _meter.CreateHistogram( + "aspnetcore.authentication.request.duration", unit: "{request}", - description: "The total number of authenticated requests"); + description: "The total number of requests for which authentication was attempted", + advice: new() { HistogramBucketBoundaries = MetricsConstants.ShortSecondsBucketBoundaries }); _challengeCount = _meter.CreateCounter( "aspnetcore.authentication.challenges", @@ -47,57 +50,202 @@ public AuthenticationMetrics(IMeterFactory meterFactory) description: "The total number of times a scheme is signed out"); } - public void AuthenticatedRequest(string scheme, AuthenticateResult result) + public void AuthenticatedRequestSucceeded(string? scheme, AuthenticateResult result, long startTimestamp, long currentTimestamp) { - if (_authenticatedRequestCount.Enabled) + if (_authenticatedRequestDuration.Enabled) { - var resultTagValue = result switch - { - { Succeeded: true } => "success", - { Failure: not null } => "failure", - { None: true } => "none", - _ => throw new UnreachableException($"Could not determine the result state of the {nameof(AuthenticateResult)}"), - }; - - _authenticatedRequestCount.Add(1, [ - new("aspnetcore.authentication.scheme", scheme), - new("aspnetcore.authentication.result", resultTagValue), - ]); + AuthenticatedRequestSucceededCore(scheme, result, startTimestamp, currentTimestamp); } } - public void Challenge(string scheme) + [MethodImpl(MethodImplOptions.NoInlining)] + private void AuthenticatedRequestSucceededCore(string? scheme, AuthenticateResult result, long startTimestamp, long currentTimestamp) + { + var tags = new TagList(); + TryAddSchemeTag(ref tags, scheme); + + var resultTagValue = result switch + { + { None: true } => "none", + { Succeeded: true } => "success", + { Failure: not null } => "failure", + _ => "_OTHER", // _OTHER is commonly used fallback for an extra or unexpected value. Shouldn't reach here. + }; + tags.Add("aspnetcore.authentication.result", resultTagValue); + + var duration = Stopwatch.GetElapsedTime(startTimestamp, currentTimestamp); + _authenticatedRequestDuration.Record(duration.TotalSeconds, tags); + } + + public void AuthenticatedRequestFailed(string? scheme, Exception exception, long startTimestamp, long currentTimestamp) + { + if (_authenticatedRequestDuration.Enabled) + { + AuthenticatedRequestFailedCore(scheme, exception, startTimestamp, currentTimestamp); + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private void AuthenticatedRequestFailedCore(string? scheme, Exception exception, long startTimestamp, long currentTimestamp) + { + var tags = new TagList(); + TryAddSchemeTag(ref tags, scheme); + AddErrorTag(ref tags, exception); + + var duration = Stopwatch.GetElapsedTime(startTimestamp, currentTimestamp); + _authenticatedRequestDuration.Record(duration.TotalSeconds, tags); + } + + public void ChallengeSucceeded(string? scheme) + { + if (_challengeCount.Enabled) + { + ChallengeSucceededCore(scheme); + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private void ChallengeSucceededCore(string? scheme) + { + var tags = new TagList(); + TryAddSchemeTag(ref tags, scheme); + + _challengeCount.Add(1, tags); + } + + public void ChallengeFailed(string? scheme, Exception exception) { if (_challengeCount.Enabled) { - _challengeCount.Add(1, [new("aspnetcore.authentication.scheme", scheme)]); + ChallengeFailedCore(scheme, exception); } } - public void Forbid(string scheme) + [MethodImpl(MethodImplOptions.NoInlining)] + private void ChallengeFailedCore(string? scheme, Exception exception) + { + var tags = new TagList(); + TryAddSchemeTag(ref tags, scheme); + AddErrorTag(ref tags, exception); + + _challengeCount.Add(1, tags); + } + + public void ForbidSucceeded(string? scheme) { if (_forbidCount.Enabled) { - _forbidCount.Add(1, [new("aspnetcore.authentication.scheme", scheme)]); + ForbidSucceededCore(scheme); } } - public void SignIn(string scheme) + [MethodImpl(MethodImplOptions.NoInlining)] + private void ForbidSucceededCore(string? scheme) + { + var tags = new TagList(); + TryAddSchemeTag(ref tags, scheme); + _forbidCount.Add(1, tags); + } + + public void ForbidFailed(string? scheme, Exception exception) + { + if (_forbidCount.Enabled) + { + ForbidFailedCore(scheme, exception); + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private void ForbidFailedCore(string? scheme, Exception exception) + { + var tags = new TagList(); + TryAddSchemeTag(ref tags, scheme); + AddErrorTag(ref tags, exception); + + _forbidCount.Add(1, tags); + } + + public void SignInSucceeded(string? scheme) { if (_signInCount.Enabled) { - _signInCount.Add(1, [new("aspnetcore.authentication.scheme", scheme)]); + SignInSucceededCore(scheme); } } - public void SignOut(string scheme) + [MethodImpl(MethodImplOptions.NoInlining)] + private void SignInSucceededCore(string? scheme) + { + var tags = new TagList(); + TryAddSchemeTag(ref tags, scheme); + _signInCount.Add(1, tags); + } + + public void SignInFailed(string? scheme, Exception exception) + { + if (_signInCount.Enabled) + { + SignInFailedCore(scheme, exception); + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private void SignInFailedCore(string? scheme, Exception exception) + { + var tags = new TagList(); + TryAddSchemeTag(ref tags, scheme); + AddErrorTag(ref tags, exception); + + _signInCount.Add(1, tags); + } + + public void SignOutSucceeded(string? scheme) { if (_signOutCount.Enabled) { - _signOutCount.Add(1, [new("aspnetcore.authentication.scheme", scheme)]); + SignOutSucceededCore(scheme); } } + [MethodImpl(MethodImplOptions.NoInlining)] + private void SignOutSucceededCore(string? scheme) + { + var tags = new TagList(); + TryAddSchemeTag(ref tags, scheme); + _signOutCount.Add(1, tags); + } + + public void SignOutFailed(string? scheme, Exception exception) + { + if (_signOutCount.Enabled) + { + SignOutFailedCore(scheme, exception); + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private void SignOutFailedCore(string? scheme, Exception exception) + { + var tags = new TagList(); + TryAddSchemeTag(ref tags, scheme); + AddErrorTag(ref tags, exception); + + _signOutCount.Add(1, tags); + } + + private static void TryAddSchemeTag(ref TagList tags, string? scheme) + { + if (scheme is not null) + { + tags.Add("aspnetcore.authentication.scheme", scheme); + } + } + + private static void AddErrorTag(ref TagList tags, Exception exception) + { + tags.Add("error.type", exception.GetType().FullName); + } + public void Dispose() { _meter.Dispose(); diff --git a/src/Http/Authentication.Core/src/AuthenticationService.cs b/src/Http/Authentication.Core/src/AuthenticationService.cs index a825c85643a6..3b45ffc56dc6 100644 --- a/src/Http/Authentication.Core/src/AuthenticationService.cs +++ b/src/Http/Authentication.Core/src/AuthenticationService.cs @@ -4,7 +4,6 @@ using System.Linq; using System.Security.Claims; using Microsoft.AspNetCore.Http; -using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; namespace Microsoft.AspNetCore.Authentication; @@ -14,8 +13,6 @@ namespace Microsoft.AspNetCore.Authentication; /// public class AuthenticationService : IAuthenticationService { - private readonly AuthenticationMetrics? _metrics; - private HashSet? _transformCache; /// @@ -30,31 +27,11 @@ public AuthenticationService( IAuthenticationHandlerProvider handlers, IClaimsTransformation transform, IOptions options) - : this(schemes, handlers, transform, options, services: null) - { - } - - /// - /// Constructor. - /// - /// The . - /// The . - /// The . - /// The . - /// The . - public AuthenticationService( - IAuthenticationSchemeProvider schemes, - IAuthenticationHandlerProvider handlers, - IClaimsTransformation transform, - IOptions options, - IServiceProvider? services) { Schemes = schemes; Handlers = handlers; Transform = transform; Options = options.Value; - - _metrics = services?.GetService(); } /// @@ -95,17 +72,11 @@ public virtual async Task AuthenticateAsync(HttpContext cont } } - var handler = await Handlers.GetHandlerAsync(context, scheme); - if (handler == null) - { - throw await CreateMissingHandlerException(scheme); - } + var handler = await Handlers.GetHandlerAsync(context, scheme) ?? throw await CreateMissingHandlerException(scheme); // Handlers should not return null, but we'll be tolerant of null values for legacy reasons. var result = (await handler.AuthenticateAsync()) ?? AuthenticateResult.NoResult(); - _metrics?.AuthenticatedRequest(scheme, result); - if (result.Succeeded) { var principal = result.Principal!; @@ -123,6 +94,7 @@ public virtual async Task AuthenticateAsync(HttpContext cont } return AuthenticateResult.Success(new AuthenticationTicket(principal, result.Properties, result.Ticket!.AuthenticationScheme)); } + return result; } @@ -145,14 +117,7 @@ public virtual async Task ChallengeAsync(HttpContext context, string? scheme, Au } } - var handler = await Handlers.GetHandlerAsync(context, scheme); - if (handler == null) - { - throw await CreateMissingHandlerException(scheme); - } - - _metrics?.Challenge(scheme); - + var handler = await Handlers.GetHandlerAsync(context, scheme) ?? throw await CreateMissingHandlerException(scheme); await handler.ChallengeAsync(properties); } @@ -175,14 +140,7 @@ public virtual async Task ForbidAsync(HttpContext context, string? scheme, Authe } } - var handler = await Handlers.GetHandlerAsync(context, scheme); - if (handler == null) - { - throw await CreateMissingHandlerException(scheme); - } - - _metrics?.Forbid(scheme); - + var handler = await Handlers.GetHandlerAsync(context, scheme) ?? throw await CreateMissingHandlerException(scheme); await handler.ForbidAsync(properties); } @@ -220,20 +178,12 @@ public virtual async Task SignInAsync(HttpContext context, string? scheme, Claim } } - var handler = await Handlers.GetHandlerAsync(context, scheme); - if (handler == null) - { - throw await CreateMissingSignInHandlerException(scheme); - } - - var signInHandler = handler as IAuthenticationSignInHandler; - if (signInHandler == null) + var handler = await Handlers.GetHandlerAsync(context, scheme) ?? throw await CreateMissingSignInHandlerException(scheme); + if (handler is not IAuthenticationSignInHandler signInHandler) { throw await CreateMismatchedSignInHandlerException(scheme, handler); } - _metrics?.SignIn(scheme); - await signInHandler.SignInAsync(principal, properties); } @@ -256,20 +206,12 @@ public virtual async Task SignOutAsync(HttpContext context, string? scheme, Auth } } - var handler = await Handlers.GetHandlerAsync(context, scheme); - if (handler == null) - { - throw await CreateMissingSignOutHandlerException(scheme); - } - - var signOutHandler = handler as IAuthenticationSignOutHandler; - if (signOutHandler == null) + var handler = await Handlers.GetHandlerAsync(context, scheme) ?? throw await CreateMissingSignOutHandlerException(scheme); + if (handler is not IAuthenticationSignOutHandler signOutHandler) { throw await CreateMismatchedSignOutHandlerException(scheme, handler); } - _metrics?.SignOut(scheme); - await signOutHandler.SignOutAsync(properties); } diff --git a/src/Http/Authentication.Core/src/AuthenticationServiceImpl.cs b/src/Http/Authentication.Core/src/AuthenticationServiceImpl.cs new file mode 100644 index 000000000000..3856dad11317 --- /dev/null +++ b/src/Http/Authentication.Core/src/AuthenticationServiceImpl.cs @@ -0,0 +1,96 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Diagnostics; +using System.Security.Claims; +using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.Options; + +namespace Microsoft.AspNetCore.Authentication; + +internal sealed class AuthenticationServiceImpl( + IAuthenticationSchemeProvider schemes, + IAuthenticationHandlerProvider handlers, + IClaimsTransformation transform, + IOptions options, + AuthenticationMetrics metrics) + : AuthenticationService(schemes, handlers, transform, options) +{ + public override async Task AuthenticateAsync(HttpContext context, string? scheme) + { + AuthenticateResult result; + var startTimestamp = Stopwatch.GetTimestamp(); + try + { + result = await base.AuthenticateAsync(context, scheme); + } + catch (Exception ex) + { + metrics.AuthenticatedRequestFailed(scheme, ex, startTimestamp, currentTimestamp: Stopwatch.GetTimestamp()); + throw; + } + + metrics.AuthenticatedRequestSucceeded(scheme, result, startTimestamp, currentTimestamp: Stopwatch.GetTimestamp()); + return result; + } + + public override async Task ChallengeAsync(HttpContext context, string? scheme, AuthenticationProperties? properties) + { + try + { + await base.ChallengeAsync(context, scheme, properties); + } + catch (Exception ex) + { + metrics.ChallengeFailed(scheme, ex); + throw; + } + + metrics.ChallengeSucceeded(scheme); + } + + public override async Task ForbidAsync(HttpContext context, string? scheme, AuthenticationProperties? properties) + { + try + { + await base.ForbidAsync(context, scheme, properties); + } + catch (Exception ex) + { + metrics.ForbidFailed(scheme, ex); + throw; + } + + metrics.ForbidSucceeded(scheme); + } + + public override async Task SignInAsync(HttpContext context, string? scheme, ClaimsPrincipal principal, AuthenticationProperties? properties) + { + try + { + await base.SignInAsync(context, scheme, principal, properties); + } + catch (Exception ex) + { + metrics.SignInFailed(scheme, ex); + throw; + } + + metrics.SignInSucceeded(scheme); + } + + public override async Task SignOutAsync(HttpContext context, string? scheme, AuthenticationProperties? properties) + { + try + { + await base.SignOutAsync(context, scheme, properties); + } + catch (Exception ex) + { + metrics.SignOutFailed(scheme, ex); + throw; + } + + metrics.SignOutSucceeded(scheme); + } +} diff --git a/src/Http/Authentication.Core/src/Microsoft.AspNetCore.Authentication.Core.csproj b/src/Http/Authentication.Core/src/Microsoft.AspNetCore.Authentication.Core.csproj index 49e2634ae0e5..61b6d3a8f71e 100644 --- a/src/Http/Authentication.Core/src/Microsoft.AspNetCore.Authentication.Core.csproj +++ b/src/Http/Authentication.Core/src/Microsoft.AspNetCore.Authentication.Core.csproj @@ -10,6 +10,10 @@ true + + + + diff --git a/src/Http/Authentication.Core/src/PublicAPI.Unshipped.txt b/src/Http/Authentication.Core/src/PublicAPI.Unshipped.txt index a5e375235430..7dc5c58110bf 100644 --- a/src/Http/Authentication.Core/src/PublicAPI.Unshipped.txt +++ b/src/Http/Authentication.Core/src/PublicAPI.Unshipped.txt @@ -1,2 +1 @@ #nullable enable -Microsoft.AspNetCore.Authentication.AuthenticationService.AuthenticationService(Microsoft.AspNetCore.Authentication.IAuthenticationSchemeProvider! schemes, Microsoft.AspNetCore.Authentication.IAuthenticationHandlerProvider! handlers, Microsoft.AspNetCore.Authentication.IClaimsTransformation! transform, Microsoft.Extensions.Options.IOptions! options, System.IServiceProvider? services) -> void diff --git a/src/Http/startvscode.cmd b/src/Http/startvscode.cmd new file mode 100644 index 000000000000..d403f3028231 --- /dev/null +++ b/src/Http/startvscode.cmd @@ -0,0 +1,3 @@ +@ECHO OFF + +%~dp0..\..\startvscode.cmd %~dp0 diff --git a/src/Security/Authentication/test/AuthenticationMetricsTest.cs b/src/Security/Authentication/test/AuthenticationMetricsTest.cs index 9165f17b0c87..8c5ffe46be74 100644 --- a/src/Security/Authentication/test/AuthenticationMetricsTest.cs +++ b/src/Security/Authentication/test/AuthenticationMetricsTest.cs @@ -4,7 +4,6 @@ using System.Security.Claims; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.InternalTesting; -using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; using Microsoft.Extensions.Diagnostics.Metrics.Testing; using Moq; @@ -25,7 +24,7 @@ public async Task Authenticate_Success() var authenticationService = CreateAuthenticationService(authenticationHandler.Object, meterFactory); var meter = meterFactory.Meters.Single(); - using var authenticationRequestsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.authenticated_requests"); + using var authenticationRequestsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.request.duration"); // Act await authenticationService.AuthenticateAsync(httpContext, scheme: "custom"); @@ -35,9 +34,10 @@ public async Task Authenticate_Success() Assert.Null(meter.Version); var measurement = Assert.Single(authenticationRequestsCollector.GetMeasurementSnapshot()); - Assert.Equal(1, measurement.Value); + Assert.True(measurement.Value > 0); Assert.Equal("custom", (string)measurement.Tags["aspnetcore.authentication.scheme"]); Assert.Equal("success", (string)measurement.Tags["aspnetcore.authentication.result"]); + Assert.False(measurement.Tags.ContainsKey("error.type")); } [Fact] @@ -52,7 +52,7 @@ public async Task Authenticate_Failure() var authenticationService = CreateAuthenticationService(authenticationHandler.Object, meterFactory); var meter = meterFactory.Meters.Single(); - using var authenticationRequestsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.authenticated_requests"); + using var authenticationRequestsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.request.duration"); // Act await authenticationService.AuthenticateAsync(httpContext, scheme: "custom"); @@ -62,9 +62,10 @@ public async Task Authenticate_Failure() Assert.Null(meter.Version); var measurement = Assert.Single(authenticationRequestsCollector.GetMeasurementSnapshot()); - Assert.Equal(1, measurement.Value); + Assert.True(measurement.Value > 0); Assert.Equal("custom", (string)measurement.Tags["aspnetcore.authentication.scheme"]); Assert.Equal("failure", (string)measurement.Tags["aspnetcore.authentication.result"]); + Assert.False(measurement.Tags.ContainsKey("error.type")); } [Fact] @@ -79,7 +80,7 @@ public async Task Authenticate_NoResult() var authenticationService = CreateAuthenticationService(authenticationHandler.Object, meterFactory); var meter = meterFactory.Meters.Single(); - using var authenticationRequestsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.authenticated_requests"); + using var authenticationRequestsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.request.duration"); // Act await authenticationService.AuthenticateAsync(httpContext, scheme: "custom"); @@ -89,9 +90,38 @@ public async Task Authenticate_NoResult() Assert.Null(meter.Version); var measurement = Assert.Single(authenticationRequestsCollector.GetMeasurementSnapshot()); - Assert.Equal(1, measurement.Value); + Assert.True(measurement.Value > 0); Assert.Equal("custom", (string)measurement.Tags["aspnetcore.authentication.scheme"]); Assert.Equal("none", (string)measurement.Tags["aspnetcore.authentication.result"]); + Assert.False(measurement.Tags.ContainsKey("error.type")); + } + + [Fact] + public async Task Authenticate_ExceptionThrownInHandler() + { + // Arrange + var authenticationHandler = new Mock(); + authenticationHandler.Setup(h => h.AuthenticateAsync()).Throws(new InvalidOperationException("An error occurred during authentication")); + + var meterFactory = new TestMeterFactory(); + var httpContext = new DefaultHttpContext(); + var authenticationService = CreateAuthenticationService(authenticationHandler.Object, meterFactory); + var meter = meterFactory.Meters.Single(); + + using var authenticationRequestsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.request.duration"); + + // Act + await Assert.ThrowsAsync(() => authenticationService.AuthenticateAsync(httpContext, scheme: "custom")); + + // Assert + Assert.Equal(AuthenticationMetrics.MeterName, meter.Name); + Assert.Null(meter.Version); + + var measurement = Assert.Single(authenticationRequestsCollector.GetMeasurementSnapshot()); + Assert.True(measurement.Value > 0); + Assert.Equal("custom", (string)measurement.Tags["aspnetcore.authentication.scheme"]); + Assert.Equal("System.InvalidOperationException", (string)measurement.Tags["error.type"]); + Assert.False(measurement.Tags.ContainsKey("aspnetcore.authentication.result")); } [Fact] @@ -117,6 +147,33 @@ public async Task Challenge() Assert.Equal("custom", (string)measurement.Tags["aspnetcore.authentication.scheme"]); } + [Fact] + public async Task Challenge_ExceptionThrownInHandler() + { + // Arrange + var authenticationHandler = new Mock(); + authenticationHandler.Setup(h => h.ChallengeAsync(It.IsAny())).Throws(new InvalidOperationException("An error occurred during challenge")); + + var meterFactory = new TestMeterFactory(); + var httpContext = new DefaultHttpContext(); + var authenticationService = CreateAuthenticationService(authenticationHandler.Object, meterFactory); + var meter = meterFactory.Meters.Single(); + + using var challengesCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.challenges"); + + // Act + await Assert.ThrowsAsync(() => authenticationService.ChallengeAsync(httpContext, scheme: "custom", properties: null)); + + // Assert + Assert.Equal(AuthenticationMetrics.MeterName, meter.Name); + Assert.Null(meter.Version); + + var measurement = Assert.Single(challengesCollector.GetMeasurementSnapshot()); + Assert.Equal(1, measurement.Value); + Assert.Equal("custom", (string)measurement.Tags["aspnetcore.authentication.scheme"]); + Assert.Equal("System.InvalidOperationException", (string)measurement.Tags["error.type"]); + } + [Fact] public async Task Forbid() { @@ -140,6 +197,33 @@ public async Task Forbid() Assert.Equal("custom", (string)measurement.Tags["aspnetcore.authentication.scheme"]); } + [Fact] + public async Task Forbid_ExceptionThrownInHandler() + { + // Arrange + var authenticationHandler = new Mock(); + authenticationHandler.Setup(h => h.ForbidAsync(It.IsAny())).Throws(new InvalidOperationException("An error occurred during forbid")); + + var meterFactory = new TestMeterFactory(); + var httpContext = new DefaultHttpContext(); + var authenticationService = CreateAuthenticationService(authenticationHandler.Object, meterFactory); + var meter = meterFactory.Meters.Single(); + + using var forbidsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.forbids"); + + // Act + await Assert.ThrowsAsync(() => authenticationService.ForbidAsync(httpContext, scheme: "custom", properties: null)); + + // Assert + Assert.Equal(AuthenticationMetrics.MeterName, meter.Name); + Assert.Null(meter.Version); + + var measurement = Assert.Single(forbidsCollector.GetMeasurementSnapshot()); + Assert.Equal(1, measurement.Value); + Assert.Equal("custom", (string)measurement.Tags["aspnetcore.authentication.scheme"]); + Assert.Equal("System.InvalidOperationException", (string)measurement.Tags["error.type"]); + } + [Fact] public async Task SignIn() { @@ -163,6 +247,33 @@ public async Task SignIn() Assert.Equal("custom", (string)measurement.Tags["aspnetcore.authentication.scheme"]); } + [Fact] + public async Task SignIn_ExceptionThrownInHandler() + { + // Arrange + var authenticationHandler = new Mock(); + authenticationHandler.Setup(h => h.SignInAsync(It.IsAny(), It.IsAny())).Throws(new InvalidOperationException("An error occurred during sign in")); + + var meterFactory = new TestMeterFactory(); + var httpContext = new DefaultHttpContext(); + var authenticationService = CreateAuthenticationService(authenticationHandler.Object, meterFactory); + var meter = meterFactory.Meters.Single(); + + using var signInsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.sign_ins"); + + // Act + await Assert.ThrowsAsync(() => authenticationService.SignInAsync(httpContext, scheme: "custom", new ClaimsPrincipal(), properties: null)); + + // Assert + Assert.Equal(AuthenticationMetrics.MeterName, meter.Name); + Assert.Null(meter.Version); + + var measurement = Assert.Single(signInsCollector.GetMeasurementSnapshot()); + Assert.Equal(1, measurement.Value); + Assert.Equal("custom", (string)measurement.Tags["aspnetcore.authentication.scheme"]); + Assert.Equal("System.InvalidOperationException", (string)measurement.Tags["error.type"]); + } + [Fact] public async Task SignOut() { @@ -186,7 +297,34 @@ public async Task SignOut() Assert.Equal("custom", (string)measurement.Tags["aspnetcore.authentication.scheme"]); } - private static AuthenticationService CreateAuthenticationService(IAuthenticationHandler authenticationHandler, TestMeterFactory meterFactory) + [Fact] + public async Task SignOut_ExceptionThrownInHandler() + { + // Arrange + var authenticationHandler = new Mock(); + authenticationHandler.Setup(h => h.SignOutAsync(It.IsAny())).Throws(new InvalidOperationException("An error occurred during sign out")); + + var httpContext = new DefaultHttpContext(); + var meterFactory = new TestMeterFactory(); + var authenticationService = CreateAuthenticationService(authenticationHandler.Object, meterFactory); + var meter = meterFactory.Meters.Single(); + + using var signOutsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.sign_outs"); + + // Act + await Assert.ThrowsAsync(() => authenticationService.SignOutAsync(httpContext, scheme: "custom", properties: null)); + + // Assert + Assert.Equal(AuthenticationMetrics.MeterName, meter.Name); + Assert.Null(meter.Version); + + var measurement = Assert.Single(signOutsCollector.GetMeasurementSnapshot()); + Assert.Equal(1, measurement.Value); + Assert.Equal("custom", (string)measurement.Tags["aspnetcore.authentication.scheme"]); + Assert.Equal("System.InvalidOperationException", (string)measurement.Tags["error.type"]); + } + + private static AuthenticationServiceImpl CreateAuthenticationService(IAuthenticationHandler authenticationHandler, TestMeterFactory meterFactory) { var authenticationHandlerProvider = new Mock(); authenticationHandlerProvider.Setup(p => p.GetHandlerAsync(It.IsAny(), "custom")).Returns(Task.FromResult(authenticationHandler)); @@ -200,16 +338,13 @@ private static AuthenticationService CreateAuthenticationService(IAuthentication RequireAuthenticatedSignIn = false, }); - var serviceCollection = new ServiceCollection(); - serviceCollection.AddSingleton(new AuthenticationMetrics(meterFactory)); - var services = serviceCollection.BuildServiceProvider(); - - var authenticationService = new AuthenticationService( + var metrics = new AuthenticationMetrics(meterFactory); + var authenticationService = new AuthenticationServiceImpl( Mock.Of(), authenticationHandlerProvider.Object, claimsTransform.Object, options, - services); + metrics); return authenticationService; } diff --git a/src/Security/startvscode.cmd b/src/Security/startvscode.cmd new file mode 100644 index 000000000000..d403f3028231 --- /dev/null +++ b/src/Security/startvscode.cmd @@ -0,0 +1,3 @@ +@ECHO OFF + +%~dp0..\..\startvscode.cmd %~dp0 From 1cec21f924a39707365fa9eefeb1e796877f0946 Mon Sep 17 00:00:00 2001 From: Mackinnon Buck Date: Wed, 8 Jan 2025 16:25:03 -0800 Subject: [PATCH 5/9] PR feedback for authorization metrics --- .../src/AuthenticationMetrics.cs | 7 +- .../test/AuthenticationMetricsTest.cs | 15 ++-- .../Core/src/AuthorizationMetrics.cs | 58 +++++++++--- ...uthorizationServiceCollectionExtensions.cs | 2 +- .../Core/src/DefaultAuthorizationService.cs | 88 ++++++------------- .../src/DefaultAuthorizationServiceImpl.cs | 62 +++++++++++++ .../PublicAPI/net10.0/PublicAPI.Unshipped.txt | 1 - .../PublicAPI/net462/PublicAPI.Unshipped.txt | 1 - .../netstandard2.0/PublicAPI.Unshipped.txt | 1 - .../test/AuthorizationMetricsTest.cs | 74 ++++++++++++++-- 10 files changed, 211 insertions(+), 98 deletions(-) create mode 100644 src/Security/Authorization/Core/src/DefaultAuthorizationServiceImpl.cs diff --git a/src/Http/Authentication.Core/src/AuthenticationMetrics.cs b/src/Http/Authentication.Core/src/AuthenticationMetrics.cs index 9e11fb23064c..b543a442e942 100644 --- a/src/Http/Authentication.Core/src/AuthenticationMetrics.cs +++ b/src/Http/Authentication.Core/src/AuthenticationMetrics.cs @@ -8,7 +8,7 @@ namespace Microsoft.AspNetCore.Authentication; -internal sealed class AuthenticationMetrics : IDisposable +internal sealed class AuthenticationMetrics { public const string MeterName = "Microsoft.AspNetCore.Authentication"; @@ -245,9 +245,4 @@ private static void AddErrorTag(ref TagList tags, Exception exception) { tags.Add("error.type", exception.GetType().FullName); } - - public void Dispose() - { - _meter.Dispose(); - } } diff --git a/src/Security/Authentication/test/AuthenticationMetricsTest.cs b/src/Security/Authentication/test/AuthenticationMetricsTest.cs index 8c5ffe46be74..382fe3bbb0d5 100644 --- a/src/Security/Authentication/test/AuthenticationMetricsTest.cs +++ b/src/Security/Authentication/test/AuthenticationMetricsTest.cs @@ -111,9 +111,10 @@ public async Task Authenticate_ExceptionThrownInHandler() using var authenticationRequestsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.request.duration"); // Act - await Assert.ThrowsAsync(() => authenticationService.AuthenticateAsync(httpContext, scheme: "custom")); + var ex = await Assert.ThrowsAsync(() => authenticationService.AuthenticateAsync(httpContext, scheme: "custom")); // Assert + Assert.Equal("An error occurred during authentication", ex.Message); Assert.Equal(AuthenticationMetrics.MeterName, meter.Name); Assert.Null(meter.Version); @@ -162,9 +163,10 @@ public async Task Challenge_ExceptionThrownInHandler() using var challengesCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.challenges"); // Act - await Assert.ThrowsAsync(() => authenticationService.ChallengeAsync(httpContext, scheme: "custom", properties: null)); + var ex = await Assert.ThrowsAsync(() => authenticationService.ChallengeAsync(httpContext, scheme: "custom", properties: null)); // Assert + Assert.Equal("An error occurred during challenge", ex.Message); Assert.Equal(AuthenticationMetrics.MeterName, meter.Name); Assert.Null(meter.Version); @@ -212,9 +214,10 @@ public async Task Forbid_ExceptionThrownInHandler() using var forbidsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.forbids"); // Act - await Assert.ThrowsAsync(() => authenticationService.ForbidAsync(httpContext, scheme: "custom", properties: null)); + var ex = await Assert.ThrowsAsync(() => authenticationService.ForbidAsync(httpContext, scheme: "custom", properties: null)); // Assert + Assert.Equal("An error occurred during forbid", ex.Message); Assert.Equal(AuthenticationMetrics.MeterName, meter.Name); Assert.Null(meter.Version); @@ -262,9 +265,10 @@ public async Task SignIn_ExceptionThrownInHandler() using var signInsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.sign_ins"); // Act - await Assert.ThrowsAsync(() => authenticationService.SignInAsync(httpContext, scheme: "custom", new ClaimsPrincipal(), properties: null)); + var ex = await Assert.ThrowsAsync(() => authenticationService.SignInAsync(httpContext, scheme: "custom", new ClaimsPrincipal(), properties: null)); // Assert + Assert.Equal("An error occurred during sign in", ex.Message); Assert.Equal(AuthenticationMetrics.MeterName, meter.Name); Assert.Null(meter.Version); @@ -312,9 +316,10 @@ public async Task SignOut_ExceptionThrownInHandler() using var signOutsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.sign_outs"); // Act - await Assert.ThrowsAsync(() => authenticationService.SignOutAsync(httpContext, scheme: "custom", properties: null)); + var ex = await Assert.ThrowsAsync(() => authenticationService.SignOutAsync(httpContext, scheme: "custom", properties: null)); // Assert + Assert.Equal("An error occurred during sign out", ex.Message); Assert.Equal(AuthenticationMetrics.MeterName, meter.Name); Assert.Null(meter.Version); diff --git a/src/Security/Authorization/Core/src/AuthorizationMetrics.cs b/src/Security/Authorization/Core/src/AuthorizationMetrics.cs index 58ab534f962a..df1c39c30949 100644 --- a/src/Security/Authorization/Core/src/AuthorizationMetrics.cs +++ b/src/Security/Authorization/Core/src/AuthorizationMetrics.cs @@ -6,43 +6,73 @@ using System.Diagnostics; using System.Diagnostics.Metrics; using System.Linq; +using System.Runtime.CompilerServices; using System.Text; using System.Threading.Tasks; namespace Microsoft.AspNetCore.Authorization; -internal sealed class AuthorizationMetrics : IDisposable +internal sealed class AuthorizationMetrics { public const string MeterName = "Microsoft.AspNetCore.Authorization"; private readonly Meter _meter; - private readonly Counter _authorizeCount; + private readonly Counter _authorizedRequestCount; public AuthorizationMetrics(IMeterFactory meterFactory) { _meter = meterFactory.Create(MeterName); - _authorizeCount = _meter.CreateCounter( - "aspnetcore.authorization.authorized_requests", + _authorizedRequestCount = _meter.CreateCounter( + "aspnetcore.authorization.requests", unit: "{request}", - description: "The total number of requests requiring authorization"); + description: "The total number of requests for which authorization was attempted"); } - public void AuthorizedRequest(string? policyName, AuthorizationResult result) + public void AuthorizedRequestSucceeded(string? policyName, AuthorizationResult result) { - if (_authorizeCount.Enabled) + if (_authorizedRequestCount.Enabled) { - var resultTagValue = result.Succeeded ? "success" : "failure"; + AuthorizedRequestSucceededCore(policyName, result); + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private void AuthorizedRequestSucceededCore(string? policyName, AuthorizationResult result) + { + var tags = new TagList(); + TryAddPolicyTag(ref tags, policyName); - _authorizeCount.Add(1, [ - new("aspnetcore.authorization.policy", policyName), - new("aspnetcore.authorization.result", resultTagValue), - ]); + var resultTagValue = result.Succeeded ? "success" : "failure"; + tags.Add("aspnetcore.authorization.result", resultTagValue); + + _authorizedRequestCount.Add(1, tags); + } + + public void AuthorizedRequestFailed(string? policyName, Exception exception) + { + if (_authorizedRequestCount.Enabled) + { + AuthorizedRequestFailedCore(policyName, exception); } } - public void Dispose() + [MethodImpl(MethodImplOptions.NoInlining)] + private void AuthorizedRequestFailedCore(string? policyName, Exception exception) + { + var tags = new TagList(); + TryAddPolicyTag(ref tags, policyName); + + tags.Add("error.type", exception.GetType().FullName); + + _authorizedRequestCount.Add(1, tags); + } + + private static void TryAddPolicyTag(ref TagList tags, string? policyName) { - _meter.Dispose(); + if (policyName is not null) + { + tags.Add("aspnetcore.authorization.policy", policyName); + } } } diff --git a/src/Security/Authorization/Core/src/AuthorizationServiceCollectionExtensions.cs b/src/Security/Authorization/Core/src/AuthorizationServiceCollectionExtensions.cs index 8a9df713c046..77ad7ff6752b 100644 --- a/src/Security/Authorization/Core/src/AuthorizationServiceCollectionExtensions.cs +++ b/src/Security/Authorization/Core/src/AuthorizationServiceCollectionExtensions.cs @@ -30,7 +30,7 @@ public static IServiceCollection AddAuthorizationCore(this IServiceCollection se services.AddMetrics(); services.TryAdd(ServiceDescriptor.Singleton()); - services.TryAdd(ServiceDescriptor.Transient()); + services.TryAdd(ServiceDescriptor.Transient()); services.TryAdd(ServiceDescriptor.Transient()); services.TryAdd(ServiceDescriptor.Transient()); services.TryAdd(ServiceDescriptor.Transient()); diff --git a/src/Security/Authorization/Core/src/DefaultAuthorizationService.cs b/src/Security/Authorization/Core/src/DefaultAuthorizationService.cs index 16f749cadd87..1a9474634485 100644 --- a/src/Security/Authorization/Core/src/DefaultAuthorizationService.cs +++ b/src/Security/Authorization/Core/src/DefaultAuthorizationService.cs @@ -6,7 +6,6 @@ using System.Security.Claims; using System.Threading.Tasks; using Microsoft.AspNetCore.Shared; -using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; @@ -18,7 +17,6 @@ namespace Microsoft.AspNetCore.Authorization; public class DefaultAuthorizationService : IAuthorizationService { private readonly AuthorizationOptions _options; - private readonly AuthorizationMetrics? _metrics; private readonly IAuthorizationHandlerContextFactory _contextFactory; private readonly IAuthorizationHandlerProvider _handlers; private readonly IAuthorizationEvaluator _evaluator; @@ -34,35 +32,7 @@ public class DefaultAuthorizationService : IAuthorizationService /// The used to create the context to handle the authorization. /// The used to determine if authorization was successful. /// The used. - public DefaultAuthorizationService( - IAuthorizationPolicyProvider policyProvider, - IAuthorizationHandlerProvider handlers, - ILogger logger, - IAuthorizationHandlerContextFactory contextFactory, - IAuthorizationEvaluator evaluator, - IOptions options) - : this(policyProvider, handlers, logger, contextFactory, evaluator, options, services: null) - { - } - - /// - /// Creates a new instance of . - /// - /// The used to provide policies. - /// The handlers used to fulfill s. - /// The logger used to log messages, warnings and errors. - /// The used to create the context to handle the authorization. - /// The used to determine if authorization was successful. - /// The used. - /// The used to provide other services. - public DefaultAuthorizationService( - IAuthorizationPolicyProvider policyProvider, - IAuthorizationHandlerProvider handlers, - ILogger logger, - IAuthorizationHandlerContextFactory contextFactory, - IAuthorizationEvaluator evaluator, - IOptions options, - IServiceProvider? services) + public DefaultAuthorizationService(IAuthorizationPolicyProvider policyProvider, IAuthorizationHandlerProvider handlers, ILogger logger, IAuthorizationHandlerContextFactory contextFactory, IAuthorizationEvaluator evaluator, IOptions options) { ArgumentNullThrowHelper.ThrowIfNull(options); ArgumentNullThrowHelper.ThrowIfNull(policyProvider); @@ -77,7 +47,6 @@ public DefaultAuthorizationService( _logger = logger; _evaluator = evaluator; _contextFactory = contextFactory; - _metrics = services?.GetService(); } /// @@ -90,33 +59,7 @@ public DefaultAuthorizationService( /// A flag indicating whether authorization has succeeded. /// This value is true when the user fulfills the policy, otherwise false. /// - public virtual Task AuthorizeAsync(ClaimsPrincipal user, object? resource, IEnumerable requirements) - => AuthorizeCoreAsync(user, resource, requirements, policyName: null); - - /// - /// Checks if a user meets a specific authorization policy. - /// - /// The user to check the policy against. - /// The resource the policy should be checked with. - /// The name of the policy to check against a specific context. - /// - /// A flag indicating whether authorization has succeeded. - /// This value is true when the user fulfills the policy otherwise false. - /// - public virtual async Task AuthorizeAsync(ClaimsPrincipal user, object? resource, string policyName) - { - ArgumentNullThrowHelper.ThrowIfNull(policyName); - - var policy = await _policyProvider.GetPolicyAsync(policyName).ConfigureAwait(false); - if (policy == null) - { - throw new InvalidOperationException($"No policy found: {policyName}."); - } - - return await AuthorizeCoreAsync(user, resource, policy.Requirements, policyName).ConfigureAwait(false); - } - - private async Task AuthorizeCoreAsync(ClaimsPrincipal user, object? resource, IEnumerable requirements, string? policyName) + public virtual async Task AuthorizeAsync(ClaimsPrincipal user, object? resource, IEnumerable requirements) { ArgumentNullThrowHelper.ThrowIfNull(requirements); @@ -132,9 +75,6 @@ private async Task AuthorizeCoreAsync(ClaimsPrincipal user, } var result = _evaluator.Evaluate(authContext); - - _metrics?.AuthorizedRequest(policyName, result); - if (result.Succeeded) { _logger.UserAuthorizationSucceeded(); @@ -143,7 +83,29 @@ private async Task AuthorizeCoreAsync(ClaimsPrincipal user, { _logger.UserAuthorizationFailed(result.Failure); } - return result; } + + /// + /// Checks if a user meets a specific authorization policy. + /// + /// The user to check the policy against. + /// The resource the policy should be checked with. + /// The name of the policy to check against a specific context. + /// + /// A flag indicating whether authorization has succeeded. + /// This value is true when the user fulfills the policy otherwise false. + /// + public virtual async Task AuthorizeAsync(ClaimsPrincipal user, object? resource, string policyName) + { + var policy = await GetPolicyAsync(policyName).ConfigureAwait(false); + return await this.AuthorizeAsync(user, resource, policy).ConfigureAwait(false); + } + + // For use in DefaultAuthorizationServiceImpl. + private protected async Task GetPolicyAsync(string policyName) + { + ArgumentNullThrowHelper.ThrowIfNull(policyName); + return await _policyProvider.GetPolicyAsync(policyName).ConfigureAwait(false) ?? throw new InvalidOperationException($"No policy found: {policyName}."); + } } diff --git a/src/Security/Authorization/Core/src/DefaultAuthorizationServiceImpl.cs b/src/Security/Authorization/Core/src/DefaultAuthorizationServiceImpl.cs new file mode 100644 index 000000000000..867d140bf537 --- /dev/null +++ b/src/Security/Authorization/Core/src/DefaultAuthorizationServiceImpl.cs @@ -0,0 +1,62 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Collections.Generic; +using System.Security.Claims; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Shared; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; + +namespace Microsoft.AspNetCore.Authorization; + +internal sealed class DefaultAuthorizationServiceImpl( + IAuthorizationPolicyProvider policyProvider, + IAuthorizationHandlerProvider handlers, + ILogger logger, + IAuthorizationHandlerContextFactory contextFactory, + IAuthorizationEvaluator evaluator, + IOptions options, + AuthorizationMetrics metrics) + : DefaultAuthorizationService(policyProvider, handlers, logger, contextFactory, evaluator, options) +{ + public override async Task AuthorizeAsync(ClaimsPrincipal user, object? resource, IEnumerable requirements) + { + AuthorizationResult result; + try + { + result = await base.AuthorizeAsync(user, resource, requirements).ConfigureAwait(false); + } + catch (Exception ex) + { + metrics.AuthorizedRequestFailed(policyName: null, ex); + throw; + } + + metrics.AuthorizedRequestSucceeded(policyName: null, result); + return result; + } + + public override async Task AuthorizeAsync(ClaimsPrincipal user, object? resource, string policyName) + { + AuthorizationResult result; + try + { + var policy = await GetPolicyAsync(policyName).ConfigureAwait(false); + + // Note that we deliberately call the base method of the other overload here. + // This is because the base implementation for this overload dispatches to the other overload, + // which would cause metrics to be recorded twice. + result = await base.AuthorizeAsync(user, resource, policy.Requirements).ConfigureAwait(false); + } + catch (Exception ex) + { + metrics.AuthorizedRequestFailed(policyName, ex); + throw; + } + + metrics.AuthorizedRequestSucceeded(policyName, result); + return result; + } +} diff --git a/src/Security/Authorization/Core/src/PublicAPI/net10.0/PublicAPI.Unshipped.txt b/src/Security/Authorization/Core/src/PublicAPI/net10.0/PublicAPI.Unshipped.txt index 26a7cc3253bb..7dc5c58110bf 100644 --- a/src/Security/Authorization/Core/src/PublicAPI/net10.0/PublicAPI.Unshipped.txt +++ b/src/Security/Authorization/Core/src/PublicAPI/net10.0/PublicAPI.Unshipped.txt @@ -1,2 +1 @@ #nullable enable -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, System.IServiceProvider? services) -> void diff --git a/src/Security/Authorization/Core/src/PublicAPI/net462/PublicAPI.Unshipped.txt b/src/Security/Authorization/Core/src/PublicAPI/net462/PublicAPI.Unshipped.txt index 26a7cc3253bb..7dc5c58110bf 100644 --- a/src/Security/Authorization/Core/src/PublicAPI/net462/PublicAPI.Unshipped.txt +++ b/src/Security/Authorization/Core/src/PublicAPI/net462/PublicAPI.Unshipped.txt @@ -1,2 +1 @@ #nullable enable -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, System.IServiceProvider? services) -> void diff --git a/src/Security/Authorization/Core/src/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt b/src/Security/Authorization/Core/src/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt index 26a7cc3253bb..7dc5c58110bf 100644 --- a/src/Security/Authorization/Core/src/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt +++ b/src/Security/Authorization/Core/src/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt @@ -1,2 +1 @@ #nullable enable -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, System.IServiceProvider? services) -> void diff --git a/src/Security/Authorization/test/AuthorizationMetricsTest.cs b/src/Security/Authorization/test/AuthorizationMetricsTest.cs index debe06c445c2..4aab636230be 100644 --- a/src/Security/Authorization/test/AuthorizationMetricsTest.cs +++ b/src/Security/Authorization/test/AuthorizationMetricsTest.cs @@ -19,7 +19,7 @@ public async Task Authorize_WithPolicyName_Success() var meter = meterFactory.Meters.Single(); var user = new ClaimsPrincipal(new ClaimsIdentity([new Claim("Permission", "CanViewPage")])); - using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.authorized_requests"); + using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.requests"); // Act await authorizationService.AuthorizeAsync(user, "Basic"); @@ -43,7 +43,7 @@ public async Task Authorize_WithPolicyName_Failure() var meter = meterFactory.Meters.Single(); var user = new ClaimsPrincipal(new ClaimsIdentity([])); // Will fail due to missing required claim - using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.authorized_requests"); + using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.requests"); // Act await authorizationService.AuthorizeAsync(user, "Basic"); @@ -58,6 +58,31 @@ public async Task Authorize_WithPolicyName_Failure() Assert.Equal("failure", (string)measurement.Tags["aspnetcore.authorization.result"]); } + [Fact] + public async Task Authorize_WithPolicyName_PolicyNotFound() + { + // Arrange + var meterFactory = new TestMeterFactory(); + var authorizationService = BuildAuthorizationService(meterFactory); + var meter = meterFactory.Meters.Single(); + var user = new ClaimsPrincipal(new ClaimsIdentity([])); // Will fail due to missing required claim + + using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.requests"); + + // Act + await Assert.ThrowsAsync(() => authorizationService.AuthorizeAsync(user, "UnknownPolicy")); + + // Assert + Assert.Equal(AuthorizationMetrics.MeterName, meter.Name); + Assert.Null(meter.Version); + + var measurement = Assert.Single(authorizedRequestsCollector.GetMeasurementSnapshot()); + Assert.Equal(1, measurement.Value); + Assert.Equal("UnknownPolicy", (string)measurement.Tags["aspnetcore.authorization.policy"]); + Assert.Equal("System.InvalidOperationException", (string)measurement.Tags["error.type"]); + Assert.False(measurement.Tags.ContainsKey("aspnetcore.authorization.result")); + } + [Fact] public async Task Authorize_WithoutPolicyName_Success() { @@ -70,7 +95,7 @@ public async Task Authorize_WithoutPolicyName_Success() var meter = meterFactory.Meters.Single(); var user = new ClaimsPrincipal(new ClaimsIdentity([])); - using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.authorized_requests"); + using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.requests"); // Act await authorizationService.AuthorizeAsync(user, resource: null, new TestRequirement()); @@ -81,8 +106,8 @@ public async Task Authorize_WithoutPolicyName_Success() var measurement = Assert.Single(authorizedRequestsCollector.GetMeasurementSnapshot()); Assert.Equal(1, measurement.Value); - Assert.Null(measurement.Tags["aspnetcore.authorization.policy"]); Assert.Equal("success", (string)measurement.Tags["aspnetcore.authorization.result"]); + Assert.False(measurement.Tags.ContainsKey("aspnetcore.authorization.policy")); } [Fact] @@ -94,7 +119,7 @@ public async Task Authorize_WithoutPolicyName_Failure() var meter = meterFactory.Meters.Single(); var user = new ClaimsPrincipal(new ClaimsIdentity([])); - using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.authorized_requests"); + using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.requests"); // Act await authorizationService.AuthorizeAsync(user, resource: null, new TestRequirement()); @@ -105,8 +130,37 @@ public async Task Authorize_WithoutPolicyName_Failure() var measurement = Assert.Single(authorizedRequestsCollector.GetMeasurementSnapshot()); Assert.Equal(1, measurement.Value); - Assert.Null(measurement.Tags["aspnetcore.authorization.policy"]); Assert.Equal("failure", (string)measurement.Tags["aspnetcore.authorization.result"]); + Assert.False(measurement.Tags.ContainsKey("aspnetcore.authorization.policy")); + } + + [Fact] + public async Task Authorize_WithoutPolicyName_ExceptionThrownInHandler() + { + // Arrange + var meterFactory = new TestMeterFactory(); + var authorizationService = BuildAuthorizationService(meterFactory, services => + { + services.AddSingleton(new AlwaysThrowHandler()); + }); + var meter = meterFactory.Meters.Single(); + var user = new ClaimsPrincipal(new ClaimsIdentity([])); + + using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.requests"); + + // Act + var ex = await Assert.ThrowsAsync(() => authorizationService.AuthorizeAsync(user, resource: null, new TestRequirement())); + + // Assert + Assert.Equal("An error occurred in the authorization handler", ex.Message); + Assert.Equal(AuthorizationMetrics.MeterName, meter.Name); + Assert.Null(meter.Version); + + var measurement = Assert.Single(authorizedRequestsCollector.GetMeasurementSnapshot()); + Assert.Equal(1, measurement.Value); + Assert.Equal("System.InvalidOperationException", (string)measurement.Tags["error.type"]); + Assert.False(measurement.Tags.ContainsKey("aspnetcore.authorization.policy")); + Assert.False(measurement.Tags.ContainsKey("aspnetcore.authorization.result")); } private static IAuthorizationService BuildAuthorizationService(TestMeterFactory meterFactory, Action setupServices = null) @@ -134,5 +188,13 @@ protected override Task HandleRequirementAsync(AuthorizationHandlerContext conte } } + private sealed class AlwaysThrowHandler : AuthorizationHandler + { + protected override Task HandleRequirementAsync(AuthorizationHandlerContext context, TestRequirement requirement) + { + throw new InvalidOperationException("An error occurred in the authorization handler"); + } + } + private sealed class TestRequirement : IAuthorizationRequirement; } From f5350c3fad59e0b766a5a91045104acfe335be83 Mon Sep 17 00:00:00 2001 From: Mackinnon Buck Date: Thu, 9 Jan 2025 09:43:08 -0800 Subject: [PATCH 6/9] PR feedback --- .../src/AuthenticationMetrics.cs | 10 ++++---- .../Core/src/AuthorizationMetrics.cs | 23 +++++++++++-------- .../src/DefaultAuthorizationServiceImpl.cs | 8 +++---- .../test/AuthorizationMetricsTest.cs | 8 ++++++- 4 files changed, 29 insertions(+), 20 deletions(-) diff --git a/src/Http/Authentication.Core/src/AuthenticationMetrics.cs b/src/Http/Authentication.Core/src/AuthenticationMetrics.cs index b543a442e942..73fac44827ce 100644 --- a/src/Http/Authentication.Core/src/AuthenticationMetrics.cs +++ b/src/Http/Authentication.Core/src/AuthenticationMetrics.cs @@ -26,28 +26,28 @@ public AuthenticationMetrics(IMeterFactory meterFactory) _authenticatedRequestDuration = _meter.CreateHistogram( "aspnetcore.authentication.request.duration", unit: "{request}", - description: "The total number of requests for which authentication was attempted", + description: "The total number of requests for which authentication was attempted.", advice: new() { HistogramBucketBoundaries = MetricsConstants.ShortSecondsBucketBoundaries }); _challengeCount = _meter.CreateCounter( "aspnetcore.authentication.challenges", unit: "{request}", - description: "The total number of times a scheme is challenged"); + description: "The total number of times a scheme is challenged."); _forbidCount = _meter.CreateCounter( "aspnetcore.authentication.forbids", unit: "{request}", - description: "The total number of times an authenticated user attempts to access a resource they are not permitted to access"); + description: "The total number of times an authenticated user attempts to access a resource they are not permitted to access."); _signInCount = _meter.CreateCounter( "aspnetcore.authentication.sign_ins", unit: "{request}", - description: "The total number of times a principal is signed in"); + description: "The total number of times a principal is signed in."); _signOutCount = _meter.CreateCounter( "aspnetcore.authentication.sign_outs", unit: "{request}", - description: "The total number of times a scheme is signed out"); + description: "The total number of times a scheme is signed out."); } public void AuthenticatedRequestSucceeded(string? scheme, AuthenticateResult result, long startTimestamp, long currentTimestamp) diff --git a/src/Security/Authorization/Core/src/AuthorizationMetrics.cs b/src/Security/Authorization/Core/src/AuthorizationMetrics.cs index df1c39c30949..a0d730d2e898 100644 --- a/src/Security/Authorization/Core/src/AuthorizationMetrics.cs +++ b/src/Security/Authorization/Core/src/AuthorizationMetrics.cs @@ -7,6 +7,7 @@ using System.Diagnostics.Metrics; using System.Linq; using System.Runtime.CompilerServices; +using System.Security.Claims; using System.Text; using System.Threading.Tasks; @@ -26,22 +27,22 @@ public AuthorizationMetrics(IMeterFactory meterFactory) _authorizedRequestCount = _meter.CreateCounter( "aspnetcore.authorization.requests", unit: "{request}", - description: "The total number of requests for which authorization was attempted"); + description: "The total number of requests for which authorization was attempted."); } - public void AuthorizedRequestSucceeded(string? policyName, AuthorizationResult result) + public void AuthorizedRequestSucceeded(ClaimsPrincipal user, string? policyName, AuthorizationResult result) { if (_authorizedRequestCount.Enabled) { - AuthorizedRequestSucceededCore(policyName, result); + AuthorizedRequestSucceededCore(user, policyName, result); } } [MethodImpl(MethodImplOptions.NoInlining)] - private void AuthorizedRequestSucceededCore(string? policyName, AuthorizationResult result) + private void AuthorizedRequestSucceededCore(ClaimsPrincipal user, string? policyName, AuthorizationResult result) { var tags = new TagList(); - TryAddPolicyTag(ref tags, policyName); + AddAuthorizationTags(ref tags, user, policyName); var resultTagValue = result.Succeeded ? "success" : "failure"; tags.Add("aspnetcore.authorization.result", resultTagValue); @@ -49,27 +50,29 @@ private void AuthorizedRequestSucceededCore(string? policyName, AuthorizationRes _authorizedRequestCount.Add(1, tags); } - public void AuthorizedRequestFailed(string? policyName, Exception exception) + public void AuthorizedRequestFailed(ClaimsPrincipal user, string? policyName, Exception exception) { if (_authorizedRequestCount.Enabled) { - AuthorizedRequestFailedCore(policyName, exception); + AuthorizedRequestFailedCore(user, policyName, exception); } } [MethodImpl(MethodImplOptions.NoInlining)] - private void AuthorizedRequestFailedCore(string? policyName, Exception exception) + private void AuthorizedRequestFailedCore(ClaimsPrincipal user, string? policyName, Exception exception) { var tags = new TagList(); - TryAddPolicyTag(ref tags, policyName); + AddAuthorizationTags(ref tags, user, policyName); tags.Add("error.type", exception.GetType().FullName); _authorizedRequestCount.Add(1, tags); } - private static void TryAddPolicyTag(ref TagList tags, string? policyName) + private static void AddAuthorizationTags(ref TagList tags, ClaimsPrincipal user, string? policyName) { + tags.Add("user.is_authenticated", user.Identity?.IsAuthenticated ?? false); + if (policyName is not null) { tags.Add("aspnetcore.authorization.policy", policyName); diff --git a/src/Security/Authorization/Core/src/DefaultAuthorizationServiceImpl.cs b/src/Security/Authorization/Core/src/DefaultAuthorizationServiceImpl.cs index 867d140bf537..d07ace5aff51 100644 --- a/src/Security/Authorization/Core/src/DefaultAuthorizationServiceImpl.cs +++ b/src/Security/Authorization/Core/src/DefaultAuthorizationServiceImpl.cs @@ -30,11 +30,11 @@ public override async Task AuthorizeAsync(ClaimsPrincipal u } catch (Exception ex) { - metrics.AuthorizedRequestFailed(policyName: null, ex); + metrics.AuthorizedRequestFailed(user, policyName: null, ex); throw; } - metrics.AuthorizedRequestSucceeded(policyName: null, result); + metrics.AuthorizedRequestSucceeded(user, policyName: null, result); return result; } @@ -52,11 +52,11 @@ public override async Task AuthorizeAsync(ClaimsPrincipal u } catch (Exception ex) { - metrics.AuthorizedRequestFailed(policyName, ex); + metrics.AuthorizedRequestFailed(user, policyName, ex); throw; } - metrics.AuthorizedRequestSucceeded(policyName, result); + metrics.AuthorizedRequestSucceeded(user, policyName, result); return result; } } diff --git a/src/Security/Authorization/test/AuthorizationMetricsTest.cs b/src/Security/Authorization/test/AuthorizationMetricsTest.cs index 4aab636230be..3c141808c3eb 100644 --- a/src/Security/Authorization/test/AuthorizationMetricsTest.cs +++ b/src/Security/Authorization/test/AuthorizationMetricsTest.cs @@ -17,7 +17,7 @@ public async Task Authorize_WithPolicyName_Success() var meterFactory = new TestMeterFactory(); var authorizationService = BuildAuthorizationService(meterFactory); var meter = meterFactory.Meters.Single(); - var user = new ClaimsPrincipal(new ClaimsIdentity([new Claim("Permission", "CanViewPage")])); + var user = new ClaimsPrincipal(new ClaimsIdentity([new Claim("Permission", "CanViewPage")], authenticationType: "someAuthentication")); using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.requests"); @@ -32,6 +32,7 @@ public async Task Authorize_WithPolicyName_Success() Assert.Equal(1, measurement.Value); Assert.Equal("Basic", (string)measurement.Tags["aspnetcore.authorization.policy"]); Assert.Equal("success", (string)measurement.Tags["aspnetcore.authorization.result"]); + Assert.True((bool)measurement.Tags["user.is_authenticated"]); } [Fact] @@ -56,6 +57,7 @@ public async Task Authorize_WithPolicyName_Failure() Assert.Equal(1, measurement.Value); Assert.Equal("Basic", (string)measurement.Tags["aspnetcore.authorization.policy"]); Assert.Equal("failure", (string)measurement.Tags["aspnetcore.authorization.result"]); + Assert.False((bool)measurement.Tags["user.is_authenticated"]); } [Fact] @@ -80,6 +82,7 @@ public async Task Authorize_WithPolicyName_PolicyNotFound() Assert.Equal(1, measurement.Value); Assert.Equal("UnknownPolicy", (string)measurement.Tags["aspnetcore.authorization.policy"]); Assert.Equal("System.InvalidOperationException", (string)measurement.Tags["error.type"]); + Assert.False((bool)measurement.Tags["user.is_authenticated"]); Assert.False(measurement.Tags.ContainsKey("aspnetcore.authorization.result")); } @@ -107,6 +110,7 @@ public async Task Authorize_WithoutPolicyName_Success() var measurement = Assert.Single(authorizedRequestsCollector.GetMeasurementSnapshot()); Assert.Equal(1, measurement.Value); Assert.Equal("success", (string)measurement.Tags["aspnetcore.authorization.result"]); + Assert.False((bool)measurement.Tags["user.is_authenticated"]); Assert.False(measurement.Tags.ContainsKey("aspnetcore.authorization.policy")); } @@ -131,6 +135,7 @@ public async Task Authorize_WithoutPolicyName_Failure() var measurement = Assert.Single(authorizedRequestsCollector.GetMeasurementSnapshot()); Assert.Equal(1, measurement.Value); Assert.Equal("failure", (string)measurement.Tags["aspnetcore.authorization.result"]); + Assert.False((bool)measurement.Tags["user.is_authenticated"]); Assert.False(measurement.Tags.ContainsKey("aspnetcore.authorization.policy")); } @@ -159,6 +164,7 @@ public async Task Authorize_WithoutPolicyName_ExceptionThrownInHandler() var measurement = Assert.Single(authorizedRequestsCollector.GetMeasurementSnapshot()); Assert.Equal(1, measurement.Value); Assert.Equal("System.InvalidOperationException", (string)measurement.Tags["error.type"]); + Assert.False((bool)measurement.Tags["user.is_authenticated"]); Assert.False(measurement.Tags.ContainsKey("aspnetcore.authorization.policy")); Assert.False(measurement.Tags.ContainsKey("aspnetcore.authorization.result")); } From 56791426686cd026acc15af2e38f2d264439620e Mon Sep 17 00:00:00 2001 From: Mackinnon Buck Date: Fri, 24 Jan 2025 09:26:28 -0800 Subject: [PATCH 7/9] API Review feedback --- src/Http/Authentication.Core/src/AuthenticationMetrics.cs | 6 +++--- src/Security/Authorization/Core/src/AuthorizationMetrics.cs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Http/Authentication.Core/src/AuthenticationMetrics.cs b/src/Http/Authentication.Core/src/AuthenticationMetrics.cs index 73fac44827ce..87a83676cc92 100644 --- a/src/Http/Authentication.Core/src/AuthenticationMetrics.cs +++ b/src/Http/Authentication.Core/src/AuthenticationMetrics.cs @@ -24,9 +24,9 @@ public AuthenticationMetrics(IMeterFactory meterFactory) _meter = meterFactory.Create(MeterName); _authenticatedRequestDuration = _meter.CreateHistogram( - "aspnetcore.authentication.request.duration", - unit: "{request}", - description: "The total number of requests for which authentication was attempted.", + "aspnetcore.authentication.authenticate.duration", + unit: "s", + description: "The authentication duration for a request.", advice: new() { HistogramBucketBoundaries = MetricsConstants.ShortSecondsBucketBoundaries }); _challengeCount = _meter.CreateCounter( diff --git a/src/Security/Authorization/Core/src/AuthorizationMetrics.cs b/src/Security/Authorization/Core/src/AuthorizationMetrics.cs index a0d730d2e898..8c4de9554ad6 100644 --- a/src/Security/Authorization/Core/src/AuthorizationMetrics.cs +++ b/src/Security/Authorization/Core/src/AuthorizationMetrics.cs @@ -25,7 +25,7 @@ public AuthorizationMetrics(IMeterFactory meterFactory) _meter = meterFactory.Create(MeterName); _authorizedRequestCount = _meter.CreateCounter( - "aspnetcore.authorization.requests", + "aspnetcore.authorization.attempts", unit: "{request}", description: "The total number of requests for which authorization was attempted."); } From 4b0e925f4214e3abee0447eb45064e3e559f4a63 Mon Sep 17 00:00:00 2001 From: Mackinnon Buck Date: Mon, 27 Jan 2025 09:07:19 -0800 Subject: [PATCH 8/9] Update tests --- .../Authentication/test/AuthenticationMetricsTest.cs | 8 ++++---- .../Authorization/test/AuthorizationMetricsTest.cs | 12 ++++++------ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/Security/Authentication/test/AuthenticationMetricsTest.cs b/src/Security/Authentication/test/AuthenticationMetricsTest.cs index 382fe3bbb0d5..d73bc52045e7 100644 --- a/src/Security/Authentication/test/AuthenticationMetricsTest.cs +++ b/src/Security/Authentication/test/AuthenticationMetricsTest.cs @@ -24,7 +24,7 @@ public async Task Authenticate_Success() var authenticationService = CreateAuthenticationService(authenticationHandler.Object, meterFactory); var meter = meterFactory.Meters.Single(); - using var authenticationRequestsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.request.duration"); + using var authenticationRequestsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.authenticate.duration"); // Act await authenticationService.AuthenticateAsync(httpContext, scheme: "custom"); @@ -52,7 +52,7 @@ public async Task Authenticate_Failure() var authenticationService = CreateAuthenticationService(authenticationHandler.Object, meterFactory); var meter = meterFactory.Meters.Single(); - using var authenticationRequestsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.request.duration"); + using var authenticationRequestsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.authenticate.duration"); // Act await authenticationService.AuthenticateAsync(httpContext, scheme: "custom"); @@ -80,7 +80,7 @@ public async Task Authenticate_NoResult() var authenticationService = CreateAuthenticationService(authenticationHandler.Object, meterFactory); var meter = meterFactory.Meters.Single(); - using var authenticationRequestsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.request.duration"); + using var authenticationRequestsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.authenticate.duration"); // Act await authenticationService.AuthenticateAsync(httpContext, scheme: "custom"); @@ -108,7 +108,7 @@ public async Task Authenticate_ExceptionThrownInHandler() var authenticationService = CreateAuthenticationService(authenticationHandler.Object, meterFactory); var meter = meterFactory.Meters.Single(); - using var authenticationRequestsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.request.duration"); + using var authenticationRequestsCollector = new MetricCollector(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.authenticate.duration"); // Act var ex = await Assert.ThrowsAsync(() => authenticationService.AuthenticateAsync(httpContext, scheme: "custom")); diff --git a/src/Security/Authorization/test/AuthorizationMetricsTest.cs b/src/Security/Authorization/test/AuthorizationMetricsTest.cs index 3c141808c3eb..affbc91de9da 100644 --- a/src/Security/Authorization/test/AuthorizationMetricsTest.cs +++ b/src/Security/Authorization/test/AuthorizationMetricsTest.cs @@ -19,7 +19,7 @@ public async Task Authorize_WithPolicyName_Success() var meter = meterFactory.Meters.Single(); var user = new ClaimsPrincipal(new ClaimsIdentity([new Claim("Permission", "CanViewPage")], authenticationType: "someAuthentication")); - using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.requests"); + using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.attempts"); // Act await authorizationService.AuthorizeAsync(user, "Basic"); @@ -44,7 +44,7 @@ public async Task Authorize_WithPolicyName_Failure() var meter = meterFactory.Meters.Single(); var user = new ClaimsPrincipal(new ClaimsIdentity([])); // Will fail due to missing required claim - using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.requests"); + using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.attempts"); // Act await authorizationService.AuthorizeAsync(user, "Basic"); @@ -69,7 +69,7 @@ public async Task Authorize_WithPolicyName_PolicyNotFound() var meter = meterFactory.Meters.Single(); var user = new ClaimsPrincipal(new ClaimsIdentity([])); // Will fail due to missing required claim - using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.requests"); + using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.attempts"); // Act await Assert.ThrowsAsync(() => authorizationService.AuthorizeAsync(user, "UnknownPolicy")); @@ -98,7 +98,7 @@ public async Task Authorize_WithoutPolicyName_Success() var meter = meterFactory.Meters.Single(); var user = new ClaimsPrincipal(new ClaimsIdentity([])); - using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.requests"); + using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.attempts"); // Act await authorizationService.AuthorizeAsync(user, resource: null, new TestRequirement()); @@ -123,7 +123,7 @@ public async Task Authorize_WithoutPolicyName_Failure() var meter = meterFactory.Meters.Single(); var user = new ClaimsPrincipal(new ClaimsIdentity([])); - using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.requests"); + using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.attempts"); // Act await authorizationService.AuthorizeAsync(user, resource: null, new TestRequirement()); @@ -151,7 +151,7 @@ public async Task Authorize_WithoutPolicyName_ExceptionThrownInHandler() var meter = meterFactory.Meters.Single(); var user = new ClaimsPrincipal(new ClaimsIdentity([])); - using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.requests"); + using var authorizedRequestsCollector = new MetricCollector(meterFactory, AuthorizationMetrics.MeterName, "aspnetcore.authorization.attempts"); // Act var ex = await Assert.ThrowsAsync(() => authorizationService.AuthorizeAsync(user, resource: null, new TestRequirement())); From e3ed7b52077f6ad26208ea8caff47c7a954a6783 Mon Sep 17 00:00:00 2001 From: Mackinnon Buck Date: Tue, 28 Jan 2025 10:35:55 -0800 Subject: [PATCH 9/9] PR feedback --- .../src/AuthenticationMetrics.cs | 156 +++++++----------- .../src/AuthenticationServiceImpl.cs | 20 +-- .../test/AuthenticationMetricsTest.cs | 2 +- .../Core/src/AuthorizationMetrics.cs | 49 ++---- .../src/DefaultAuthorizationServiceImpl.cs | 8 +- 5 files changed, 88 insertions(+), 147 deletions(-) diff --git a/src/Http/Authentication.Core/src/AuthenticationMetrics.cs b/src/Http/Authentication.Core/src/AuthenticationMetrics.cs index 87a83676cc92..12607938a10d 100644 --- a/src/Http/Authentication.Core/src/AuthenticationMetrics.cs +++ b/src/Http/Authentication.Core/src/AuthenticationMetrics.cs @@ -50,195 +50,151 @@ public AuthenticationMetrics(IMeterFactory meterFactory) description: "The total number of times a scheme is signed out."); } - public void AuthenticatedRequestSucceeded(string? scheme, AuthenticateResult result, long startTimestamp, long currentTimestamp) + public void AuthenticatedRequestCompleted(string? scheme, AuthenticateResult? result, Exception? exception, long startTimestamp, long currentTimestamp) { if (_authenticatedRequestDuration.Enabled) { - AuthenticatedRequestSucceededCore(scheme, result, startTimestamp, currentTimestamp); + AuthenticatedRequestCompletedCore(scheme, result, exception, startTimestamp, currentTimestamp); } } [MethodImpl(MethodImplOptions.NoInlining)] - private void AuthenticatedRequestSucceededCore(string? scheme, AuthenticateResult result, long startTimestamp, long currentTimestamp) + private void AuthenticatedRequestCompletedCore(string? scheme, AuthenticateResult? result, Exception? exception, long startTimestamp, long currentTimestamp) { var tags = new TagList(); - TryAddSchemeTag(ref tags, scheme); - var resultTagValue = result switch + if (scheme is not null) { - { None: true } => "none", - { Succeeded: true } => "success", - { Failure: not null } => "failure", - _ => "_OTHER", // _OTHER is commonly used fallback for an extra or unexpected value. Shouldn't reach here. - }; - tags.Add("aspnetcore.authentication.result", resultTagValue); - - var duration = Stopwatch.GetElapsedTime(startTimestamp, currentTimestamp); - _authenticatedRequestDuration.Record(duration.TotalSeconds, tags); - } + AddSchemeTag(ref tags, scheme); + } - public void AuthenticatedRequestFailed(string? scheme, Exception exception, long startTimestamp, long currentTimestamp) - { - if (_authenticatedRequestDuration.Enabled) + if (result is not null) { - AuthenticatedRequestFailedCore(scheme, exception, startTimestamp, currentTimestamp); + tags.Add("aspnetcore.authentication.result", result switch + { + { None: true } => "none", + { Succeeded: true } => "success", + { Failure: not null } => "failure", + _ => "_OTHER", // _OTHER is commonly used fallback for an extra or unexpected value. Shouldn't reach here. + }); } - } - [MethodImpl(MethodImplOptions.NoInlining)] - private void AuthenticatedRequestFailedCore(string? scheme, Exception exception, long startTimestamp, long currentTimestamp) - { - var tags = new TagList(); - TryAddSchemeTag(ref tags, scheme); - AddErrorTag(ref tags, exception); + if (exception is not null) + { + AddErrorTag(ref tags, exception); + } var duration = Stopwatch.GetElapsedTime(startTimestamp, currentTimestamp); _authenticatedRequestDuration.Record(duration.TotalSeconds, tags); } - public void ChallengeSucceeded(string? scheme) + public void ChallengeCompleted(string? scheme, Exception? exception) { if (_challengeCount.Enabled) { - ChallengeSucceededCore(scheme); + ChallengeCompletedCore(scheme, exception); } } [MethodImpl(MethodImplOptions.NoInlining)] - private void ChallengeSucceededCore(string? scheme) + private void ChallengeCompletedCore(string? scheme, Exception? exception) { var tags = new TagList(); - TryAddSchemeTag(ref tags, scheme); - _challengeCount.Add(1, tags); - } - - public void ChallengeFailed(string? scheme, Exception exception) - { - if (_challengeCount.Enabled) + if (scheme is not null) { - ChallengeFailedCore(scheme, exception); + AddSchemeTag(ref tags, scheme); } - } - [MethodImpl(MethodImplOptions.NoInlining)] - private void ChallengeFailedCore(string? scheme, Exception exception) - { - var tags = new TagList(); - TryAddSchemeTag(ref tags, scheme); - AddErrorTag(ref tags, exception); + if (exception is not null) + { + AddErrorTag(ref tags, exception); + } _challengeCount.Add(1, tags); } - public void ForbidSucceeded(string? scheme) + public void ForbidCompleted(string? scheme, Exception? exception) { if (_forbidCount.Enabled) { - ForbidSucceededCore(scheme); + ForbidCompletedCore(scheme, exception); } } [MethodImpl(MethodImplOptions.NoInlining)] - private void ForbidSucceededCore(string? scheme) + private void ForbidCompletedCore(string? scheme, Exception? exception) { var tags = new TagList(); - TryAddSchemeTag(ref tags, scheme); - _forbidCount.Add(1, tags); - } - public void ForbidFailed(string? scheme, Exception exception) - { - if (_forbidCount.Enabled) + if (scheme is not null) { - ForbidFailedCore(scheme, exception); + AddSchemeTag(ref tags, scheme); } - } - [MethodImpl(MethodImplOptions.NoInlining)] - private void ForbidFailedCore(string? scheme, Exception exception) - { - var tags = new TagList(); - TryAddSchemeTag(ref tags, scheme); - AddErrorTag(ref tags, exception); + if (exception is not null) + { + AddErrorTag(ref tags, exception); + } _forbidCount.Add(1, tags); } - public void SignInSucceeded(string? scheme) + public void SignInCompleted(string? scheme, Exception? exception) { if (_signInCount.Enabled) { - SignInSucceededCore(scheme); + SignInCompletedCore(scheme, exception); } } [MethodImpl(MethodImplOptions.NoInlining)] - private void SignInSucceededCore(string? scheme) + private void SignInCompletedCore(string? scheme, Exception? exception) { var tags = new TagList(); - TryAddSchemeTag(ref tags, scheme); - _signInCount.Add(1, tags); - } - public void SignInFailed(string? scheme, Exception exception) - { - if (_signInCount.Enabled) + if (scheme is not null) { - SignInFailedCore(scheme, exception); + AddSchemeTag(ref tags, scheme); } - } - [MethodImpl(MethodImplOptions.NoInlining)] - private void SignInFailedCore(string? scheme, Exception exception) - { - var tags = new TagList(); - TryAddSchemeTag(ref tags, scheme); - AddErrorTag(ref tags, exception); + if (exception is not null) + { + AddErrorTag(ref tags, exception); + } _signInCount.Add(1, tags); } - public void SignOutSucceeded(string? scheme) + public void SignOutCompleted(string? scheme, Exception? exception) { if (_signOutCount.Enabled) { - SignOutSucceededCore(scheme); + SignOutCompletedCore(scheme, exception); } } [MethodImpl(MethodImplOptions.NoInlining)] - private void SignOutSucceededCore(string? scheme) + private void SignOutCompletedCore(string? scheme, Exception? exception) { var tags = new TagList(); - TryAddSchemeTag(ref tags, scheme); - _signOutCount.Add(1, tags); - } - public void SignOutFailed(string? scheme, Exception exception) - { - if (_signOutCount.Enabled) + if (scheme is not null) { - SignOutFailedCore(scheme, exception); + AddSchemeTag(ref tags, scheme); } - } - [MethodImpl(MethodImplOptions.NoInlining)] - private void SignOutFailedCore(string? scheme, Exception exception) - { - var tags = new TagList(); - TryAddSchemeTag(ref tags, scheme); - AddErrorTag(ref tags, exception); + if (exception is not null) + { + AddErrorTag(ref tags, exception); + } _signOutCount.Add(1, tags); } - private static void TryAddSchemeTag(ref TagList tags, string? scheme) + private static void AddSchemeTag(ref TagList tags, string scheme) { - if (scheme is not null) - { - tags.Add("aspnetcore.authentication.scheme", scheme); - } + tags.Add("aspnetcore.authentication.scheme", scheme); } private static void AddErrorTag(ref TagList tags, Exception exception) diff --git a/src/Http/Authentication.Core/src/AuthenticationServiceImpl.cs b/src/Http/Authentication.Core/src/AuthenticationServiceImpl.cs index 3856dad11317..8aca027a1af6 100644 --- a/src/Http/Authentication.Core/src/AuthenticationServiceImpl.cs +++ b/src/Http/Authentication.Core/src/AuthenticationServiceImpl.cs @@ -26,11 +26,11 @@ public override async Task AuthenticateAsync(HttpContext con } catch (Exception ex) { - metrics.AuthenticatedRequestFailed(scheme, ex, startTimestamp, currentTimestamp: Stopwatch.GetTimestamp()); + metrics.AuthenticatedRequestCompleted(scheme, result: null, ex, startTimestamp, currentTimestamp: Stopwatch.GetTimestamp()); throw; } - metrics.AuthenticatedRequestSucceeded(scheme, result, startTimestamp, currentTimestamp: Stopwatch.GetTimestamp()); + metrics.AuthenticatedRequestCompleted(scheme, result, exception: result.Failure, startTimestamp, currentTimestamp: Stopwatch.GetTimestamp()); return result; } @@ -42,11 +42,11 @@ public override async Task ChallengeAsync(HttpContext context, string? scheme, A } catch (Exception ex) { - metrics.ChallengeFailed(scheme, ex); + metrics.ChallengeCompleted(scheme, ex); throw; } - metrics.ChallengeSucceeded(scheme); + metrics.ChallengeCompleted(scheme, exception: null); } public override async Task ForbidAsync(HttpContext context, string? scheme, AuthenticationProperties? properties) @@ -57,11 +57,11 @@ public override async Task ForbidAsync(HttpContext context, string? scheme, Auth } catch (Exception ex) { - metrics.ForbidFailed(scheme, ex); + metrics.ForbidCompleted(scheme, ex); throw; } - metrics.ForbidSucceeded(scheme); + metrics.ForbidCompleted(scheme, exception: null); } public override async Task SignInAsync(HttpContext context, string? scheme, ClaimsPrincipal principal, AuthenticationProperties? properties) @@ -72,11 +72,11 @@ public override async Task SignInAsync(HttpContext context, string? scheme, Clai } catch (Exception ex) { - metrics.SignInFailed(scheme, ex); + metrics.SignInCompleted(scheme, ex); throw; } - metrics.SignInSucceeded(scheme); + metrics.SignInCompleted(scheme, exception: null); } public override async Task SignOutAsync(HttpContext context, string? scheme, AuthenticationProperties? properties) @@ -87,10 +87,10 @@ public override async Task SignOutAsync(HttpContext context, string? scheme, Aut } catch (Exception ex) { - metrics.SignOutFailed(scheme, ex); + metrics.SignOutCompleted(scheme, ex); throw; } - metrics.SignOutSucceeded(scheme); + metrics.SignOutCompleted(scheme, exception: null); } } diff --git a/src/Security/Authentication/test/AuthenticationMetricsTest.cs b/src/Security/Authentication/test/AuthenticationMetricsTest.cs index d73bc52045e7..fec3667d4ee6 100644 --- a/src/Security/Authentication/test/AuthenticationMetricsTest.cs +++ b/src/Security/Authentication/test/AuthenticationMetricsTest.cs @@ -65,7 +65,7 @@ public async Task Authenticate_Failure() Assert.True(measurement.Value > 0); Assert.Equal("custom", (string)measurement.Tags["aspnetcore.authentication.scheme"]); Assert.Equal("failure", (string)measurement.Tags["aspnetcore.authentication.result"]); - Assert.False(measurement.Tags.ContainsKey("error.type")); + Assert.Equal("Microsoft.AspNetCore.Authentication.AuthenticationFailureException", (string)measurement.Tags["error.type"]); } [Fact] diff --git a/src/Security/Authorization/Core/src/AuthorizationMetrics.cs b/src/Security/Authorization/Core/src/AuthorizationMetrics.cs index 8c4de9554ad6..eb1d4060c593 100644 --- a/src/Security/Authorization/Core/src/AuthorizationMetrics.cs +++ b/src/Security/Authorization/Core/src/AuthorizationMetrics.cs @@ -30,52 +30,37 @@ public AuthorizationMetrics(IMeterFactory meterFactory) description: "The total number of requests for which authorization was attempted."); } - public void AuthorizedRequestSucceeded(ClaimsPrincipal user, string? policyName, AuthorizationResult result) + public void AuthorizedRequestCompleted(ClaimsPrincipal user, string? policyName, AuthorizationResult? result, Exception? exception) { if (_authorizedRequestCount.Enabled) { - AuthorizedRequestSucceededCore(user, policyName, result); + AuthorizedRequestCompletedCore(user, policyName, result, exception); } } [MethodImpl(MethodImplOptions.NoInlining)] - private void AuthorizedRequestSucceededCore(ClaimsPrincipal user, string? policyName, AuthorizationResult result) + private void AuthorizedRequestCompletedCore(ClaimsPrincipal user, string? policyName, AuthorizationResult? result, Exception? exception) { - var tags = new TagList(); - AddAuthorizationTags(ref tags, user, policyName); + var tags = new TagList([ + new("user.is_authenticated", user.Identity?.IsAuthenticated ?? false) + ]); - var resultTagValue = result.Succeeded ? "success" : "failure"; - tags.Add("aspnetcore.authorization.result", resultTagValue); - - _authorizedRequestCount.Add(1, tags); - } - - public void AuthorizedRequestFailed(ClaimsPrincipal user, string? policyName, Exception exception) - { - if (_authorizedRequestCount.Enabled) + if (policyName is not null) { - AuthorizedRequestFailedCore(user, policyName, exception); + tags.Add("aspnetcore.authorization.policy", policyName); } - } - - [MethodImpl(MethodImplOptions.NoInlining)] - private void AuthorizedRequestFailedCore(ClaimsPrincipal user, string? policyName, Exception exception) - { - var tags = new TagList(); - AddAuthorizationTags(ref tags, user, policyName); - tags.Add("error.type", exception.GetType().FullName); - - _authorizedRequestCount.Add(1, tags); - } - - private static void AddAuthorizationTags(ref TagList tags, ClaimsPrincipal user, string? policyName) - { - tags.Add("user.is_authenticated", user.Identity?.IsAuthenticated ?? false); + if (result is not null) + { + var resultTagValue = result.Succeeded ? "success" : "failure"; + tags.Add("aspnetcore.authorization.result", resultTagValue); + } - if (policyName is not null) + if (exception is not null) { - tags.Add("aspnetcore.authorization.policy", policyName); + tags.Add("error.type", exception.GetType().FullName); } + + _authorizedRequestCount.Add(1, tags); } } diff --git a/src/Security/Authorization/Core/src/DefaultAuthorizationServiceImpl.cs b/src/Security/Authorization/Core/src/DefaultAuthorizationServiceImpl.cs index d07ace5aff51..0912acb47792 100644 --- a/src/Security/Authorization/Core/src/DefaultAuthorizationServiceImpl.cs +++ b/src/Security/Authorization/Core/src/DefaultAuthorizationServiceImpl.cs @@ -30,11 +30,11 @@ public override async Task AuthorizeAsync(ClaimsPrincipal u } catch (Exception ex) { - metrics.AuthorizedRequestFailed(user, policyName: null, ex); + metrics.AuthorizedRequestCompleted(user, policyName: null, result: null, ex); throw; } - metrics.AuthorizedRequestSucceeded(user, policyName: null, result); + metrics.AuthorizedRequestCompleted(user, policyName: null, result, exception: null); return result; } @@ -52,11 +52,11 @@ public override async Task AuthorizeAsync(ClaimsPrincipal u } catch (Exception ex) { - metrics.AuthorizedRequestFailed(user, policyName, ex); + metrics.AuthorizedRequestCompleted(user, policyName, result: null, ex); throw; } - metrics.AuthorizedRequestSucceeded(user, policyName, result); + metrics.AuthorizedRequestCompleted(user, policyName, result, exception: null); return result; } }