PR feedback for authorization metrics

Author: MackinnonBuck

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();
-    }
 }

src/Security/Authentication/test/AuthenticationMetricsTest.cs

@@ -111,9 +111,10 @@ public async Task Authenticate_ExceptionThrownInHandler()
         using var authenticationRequestsCollector = new MetricCollector<double>(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.request.duration");
 
         // Act
-        await Assert.ThrowsAsync<InvalidOperationException>(() => authenticationService.AuthenticateAsync(httpContext, scheme: "custom"));
+        var ex = await Assert.ThrowsAsync<InvalidOperationException>(() => 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<long>(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.challenges");
 
         // Act
-        await Assert.ThrowsAsync<InvalidOperationException>(() => authenticationService.ChallengeAsync(httpContext, scheme: "custom", properties: null));
+        var ex = await Assert.ThrowsAsync<InvalidOperationException>(() => 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<long>(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.forbids");
 
         // Act
-        await Assert.ThrowsAsync<InvalidOperationException>(() => authenticationService.ForbidAsync(httpContext, scheme: "custom", properties: null));
+        var ex = await Assert.ThrowsAsync<InvalidOperationException>(() => 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<long>(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.sign_ins");
 
         // Act
-        await Assert.ThrowsAsync<InvalidOperationException>(() => authenticationService.SignInAsync(httpContext, scheme: "custom", new ClaimsPrincipal(), properties: null));
+        var ex = await Assert.ThrowsAsync<InvalidOperationException>(() => 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<long>(meterFactory, AuthenticationMetrics.MeterName, "aspnetcore.authentication.sign_outs");
 
         // Act
-        await Assert.ThrowsAsync<InvalidOperationException>(() => authenticationService.SignOutAsync(httpContext, scheme: "custom", properties: null));
+        var ex = await Assert.ThrowsAsync<InvalidOperationException>(() => 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);
 

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<long> _authorizeCount;
+    private readonly Counter<long> _authorizedRequestCount;
 
     public AuthorizationMetrics(IMeterFactory meterFactory)
     {
         _meter = meterFactory.Create(MeterName);
 
-        _authorizeCount = _meter.CreateCounter<long>(
-            "aspnetcore.authorization.authorized_requests",
+        _authorizedRequestCount = _meter.CreateCounter<long>(
+            "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);
+        }
     }
 }

src/Security/Authorization/Core/src/AuthorizationServiceCollectionExtensions.cs

@@ -30,7 +30,7 @@ public static IServiceCollection AddAuthorizationCore(this IServiceCollection se
         services.AddMetrics();
 
         services.TryAdd(ServiceDescriptor.Singleton<AuthorizationMetrics, AuthorizationMetrics>());
-        services.TryAdd(ServiceDescriptor.Transient<IAuthorizationService, DefaultAuthorizationService>());
+        services.TryAdd(ServiceDescriptor.Transient<IAuthorizationService, DefaultAuthorizationServiceImpl>());
         services.TryAdd(ServiceDescriptor.Transient<IAuthorizationPolicyProvider, DefaultAuthorizationPolicyProvider>());
         services.TryAdd(ServiceDescriptor.Transient<IAuthorizationHandlerProvider, DefaultAuthorizationHandlerProvider>());
         services.TryAdd(ServiceDescriptor.Transient<IAuthorizationEvaluator, DefaultAuthorizationEvaluator>());

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
     /// <param name="contextFactory">The <see cref="IAuthorizationHandlerContextFactory"/> used to create the context to handle the authorization.</param>
     /// <param name="evaluator">The <see cref="IAuthorizationEvaluator"/> used to determine if authorization was successful.</param>
     /// <param name="options">The <see cref="AuthorizationOptions"/> used.</param>
-    public DefaultAuthorizationService(
-        IAuthorizationPolicyProvider policyProvider,
-        IAuthorizationHandlerProvider handlers,
-        ILogger<DefaultAuthorizationService> logger,
-        IAuthorizationHandlerContextFactory contextFactory,
-        IAuthorizationEvaluator evaluator,
-        IOptions<AuthorizationOptions> options)
-        : this(policyProvider, handlers, logger, contextFactory, evaluator, options, services: null)
-    {
-    }
-
-    /// <summary>
-    /// Creates a new instance of <see cref="DefaultAuthorizationService"/>.
-    /// </summary>
-    /// <param name="policyProvider">The <see cref="IAuthorizationPolicyProvider"/> used to provide policies.</param>
-    /// <param name="handlers">The handlers used to fulfill <see cref="IAuthorizationRequirement"/>s.</param>
-    /// <param name="logger">The logger used to log messages, warnings and errors.</param>
-    /// <param name="contextFactory">The <see cref="IAuthorizationHandlerContextFactory"/> used to create the context to handle the authorization.</param>
-    /// <param name="evaluator">The <see cref="IAuthorizationEvaluator"/> used to determine if authorization was successful.</param>
-    /// <param name="options">The <see cref="AuthorizationOptions"/> used.</param>
-    /// <param name="services">The <see cref="IServiceProvider"/> used to provide other services.</param>
-    public DefaultAuthorizationService(
-        IAuthorizationPolicyProvider policyProvider,
-        IAuthorizationHandlerProvider handlers,
-        ILogger<DefaultAuthorizationService> logger,
-        IAuthorizationHandlerContextFactory contextFactory,
-        IAuthorizationEvaluator evaluator,
-        IOptions<AuthorizationOptions> options,
-        IServiceProvider? services)
+    public DefaultAuthorizationService(IAuthorizationPolicyProvider policyProvider, IAuthorizationHandlerProvider handlers, ILogger<DefaultAuthorizationService> logger, IAuthorizationHandlerContextFactory contextFactory, IAuthorizationEvaluator evaluator, IOptions<AuthorizationOptions> options)
     {
         ArgumentNullThrowHelper.ThrowIfNull(options);
         ArgumentNullThrowHelper.ThrowIfNull(policyProvider);
@@ -77,7 +47,6 @@ public DefaultAuthorizationService(
         _logger = logger;
         _evaluator = evaluator;
         _contextFactory = contextFactory;
-        _metrics = services?.GetService<AuthorizationMetrics>();
     }
 
     /// <summary>
@@ -90,33 +59,7 @@ public DefaultAuthorizationService(
     /// A flag indicating whether authorization has succeeded.
     /// This value is <c>true</c> when the user fulfills the policy, otherwise <c>false</c>.
     /// </returns>
-    public virtual Task<AuthorizationResult> AuthorizeAsync(ClaimsPrincipal user, object? resource, IEnumerable<IAuthorizationRequirement> requirements)
-        => AuthorizeCoreAsync(user, resource, requirements, policyName: null);
-
-    /// <summary>
-    /// Checks if a user meets a specific authorization policy.
-    /// </summary>
-    /// <param name="user">The user to check the policy against.</param>
-    /// <param name="resource">The resource the policy should be checked with.</param>
-    /// <param name="policyName">The name of the policy to check against a specific context.</param>
-    /// <returns>
-    /// A flag indicating whether authorization has succeeded.
-    /// This value is <c>true</c> when the user fulfills the policy otherwise <c>false</c>.
-    /// </returns>
-    public virtual async Task<AuthorizationResult> 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<AuthorizationResult> AuthorizeCoreAsync(ClaimsPrincipal user, object? resource, IEnumerable<IAuthorizationRequirement> requirements, string? policyName)
+    public virtual async Task<AuthorizationResult> AuthorizeAsync(ClaimsPrincipal user, object? resource, IEnumerable<IAuthorizationRequirement> requirements)
     {
         ArgumentNullThrowHelper.ThrowIfNull(requirements);
 
@@ -132,9 +75,6 @@ private async Task<AuthorizationResult> AuthorizeCoreAsync(ClaimsPrincipal user,
         }
 
         var result = _evaluator.Evaluate(authContext);
-
-        _metrics?.AuthorizedRequest(policyName, result);
-
         if (result.Succeeded)
         {
             _logger.UserAuthorizationSucceeded();
@@ -143,7 +83,29 @@ private async Task<AuthorizationResult> AuthorizeCoreAsync(ClaimsPrincipal user,
         {
             _logger.UserAuthorizationFailed(result.Failure);
         }
-
         return result;
     }
+
+    /// <summary>
+    /// Checks if a user meets a specific authorization policy.
+    /// </summary>
+    /// <param name="user">The user to check the policy against.</param>
+    /// <param name="resource">The resource the policy should be checked with.</param>
+    /// <param name="policyName">The name of the policy to check against a specific context.</param>
+    /// <returns>
+    /// A flag indicating whether authorization has succeeded.
+    /// This value is <c>true</c> when the user fulfills the policy otherwise <c>false</c>.
+    /// </returns>
+    public virtual async Task<AuthorizationResult> 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<AuthorizationPolicy> GetPolicyAsync(string policyName)
+    {
+        ArgumentNullThrowHelper.ThrowIfNull(policyName);
+        return await _policyProvider.GetPolicyAsync(policyName).ConfigureAwait(false) ?? throw new InvalidOperationException($"No policy found: {policyName}.");
+    }
 }

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<DefaultAuthorizationService> logger,
+    IAuthorizationHandlerContextFactory contextFactory,
+    IAuthorizationEvaluator evaluator,
+    IOptions<AuthorizationOptions> options,
+    AuthorizationMetrics metrics)
+    : DefaultAuthorizationService(policyProvider, handlers, logger, contextFactory, evaluator, options)
+{
+    public override async Task<AuthorizationResult> AuthorizeAsync(ClaimsPrincipal user, object? resource, IEnumerable<IAuthorizationRequirement> 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<AuthorizationResult> 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;
+    }
+}

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<Microsoft.AspNetCore.Authorization.DefaultAuthorizationService!>! logger, Microsoft.AspNetCore.Authorization.IAuthorizationHandlerContextFactory! contextFactory, Microsoft.AspNetCore.Authorization.IAuthorizationEvaluator! evaluator, Microsoft.Extensions.Options.IOptions<Microsoft.AspNetCore.Authorization.AuthorizationOptions!>! options, System.IServiceProvider? services) -> void

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<Microsoft.AspNetCore.Authorization.DefaultAuthorizationService!>! logger, Microsoft.AspNetCore.Authorization.IAuthorizationHandlerContextFactory! contextFactory, Microsoft.AspNetCore.Authorization.IAuthorizationEvaluator! evaluator, Microsoft.Extensions.Options.IOptions<Microsoft.AspNetCore.Authorization.AuthorizationOptions!>! options, System.IServiceProvider? services) -> void

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<Microsoft.AspNetCore.Authorization.DefaultAuthorizationService!>! logger, Microsoft.AspNetCore.Authorization.IAuthorizationHandlerContextFactory! contextFactory, Microsoft.AspNetCore.Authorization.IAuthorizationEvaluator! evaluator, Microsoft.Extensions.Options.IOptions<Microsoft.AspNetCore.Authorization.AuthorizationOptions!>! options, System.IServiceProvider? services) -> void