PR feedback

Author: MackinnonBuck

src/Http/Authentication.Core/src/AuthenticationMetrics.cs

@@ -26,28 +26,28 @@ public AuthenticationMetrics(IMeterFactory meterFactory)
         _authenticatedRequestDuration = _meter.CreateHistogram<double>(
             "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<long>(
             "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<long>(
             "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<long>(
             "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<long>(
             "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)

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,50 +27,52 @@ public AuthorizationMetrics(IMeterFactory meterFactory)
         _authorizedRequestCount = _meter.CreateCounter<long>(
             "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);
 
         _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);

src/Security/Authorization/Core/src/DefaultAuthorizationServiceImpl.cs

@@ -30,11 +30,11 @@ public override async Task<AuthorizationResult> 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<AuthorizationResult> 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;
     }
 }

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