From 78edd18524c099284559f8cc4a6bb6108ed07df9 Mon Sep 17 00:00:00 2001 From: Giuseppe Campanelli Date: Tue, 19 May 2020 11:59:29 -0400 Subject: [PATCH] Clean up logs for Negotiate Authentication and Authorization (#21927) * Clean up logs for Negotiate Authentication and Authorization * Add missing arg in NegotiateLoggingExtensions.cs * Adjust formatting * Remaining text changes * Update src/Security/Authentication/Negotiate/src/Internal/NegotiateLoggingExtensions.cs Co-authored-by: Chris Ross * Update src/Security/Authentication/Negotiate/src/Internal/NegotiateLoggingExtensions.cs Co-authored-by: Chris Ross * Update src/Security/Authentication/Negotiate/src/NegotiateHandler.cs Co-authored-by: Chris Ross * Update src/Security/Authentication/Negotiate/src/Internal/NegotiateLoggingExtensions.cs Co-authored-by: Chris Ross * Apply changes to feedback committed * Update logger call to refactored name Co-authored-by: Chris Ross Co-authored-by: Giuseppe Campanelli --- .../src/Internal/NegotiateLoggingExtensions.cs | 10 +++++++++- .../Authentication/Negotiate/src/NegotiateHandler.cs | 4 ++-- .../Core/src/DenyAnonymousAuthorizationRequirement.cs | 2 +- .../Authorization/Core/src/LoggingExtensions.cs | 2 +- .../test/DefaultAuthorizationServiceTests.cs | 6 +++--- .../test/DenyAnonymousAuthorizationRequirementTests.cs | 4 ++-- 6 files changed, 18 insertions(+), 10 deletions(-) diff --git a/src/Security/Authentication/Negotiate/src/Internal/NegotiateLoggingExtensions.cs b/src/Security/Authentication/Negotiate/src/Internal/NegotiateLoggingExtensions.cs index b39ec8cf7b..cc9aeeaa5d 100644 --- a/src/Security/Authentication/Negotiate/src/Internal/NegotiateLoggingExtensions.cs +++ b/src/Security/Authentication/Negotiate/src/Internal/NegotiateLoggingExtensions.cs @@ -17,6 +17,7 @@ namespace Microsoft.Extensions.Logging private static Action _challengeNegotiate; private static Action _reauthenticating; private static Action _deferring; + private static Action _negotiateError; static NegotiateLoggingExtensions() { @@ -43,7 +44,7 @@ namespace Microsoft.Extensions.Logging _challengeNegotiate = LoggerMessage.Define( eventId: new EventId(6, "ChallengeNegotiate"), logLevel: LogLevel.Debug, - formatString: "Challenged 401 Negotiate"); + formatString: "Challenged 401 Negotiate."); _reauthenticating = LoggerMessage.Define( eventId: new EventId(7, "Reauthenticating"), logLevel: LogLevel.Debug, @@ -60,6 +61,10 @@ namespace Microsoft.Extensions.Logging eventId: new EventId(10, "ClientError"), logLevel: LogLevel.Debug, formatString: "The users authentication request was invalid."); + _negotiateError = LoggerMessage.Define( + eventId: new EventId(11, "NegotiateError"), + logLevel: LogLevel.Debug, + formatString: "Negotiate error code: {error}."); } public static void IncompleteNegotiateChallenge(this ILogger logger) @@ -91,5 +96,8 @@ namespace Microsoft.Extensions.Logging public static void ClientError(this ILogger logger, Exception ex) => _clientError(logger, ex); + + public static void NegotiateError(this ILogger logger, string error) + => _negotiateError(logger, error, null); } } diff --git a/src/Security/Authentication/Negotiate/src/NegotiateHandler.cs b/src/Security/Authentication/Negotiate/src/NegotiateHandler.cs index 9392d1028a..835542a42d 100644 --- a/src/Security/Authentication/Negotiate/src/NegotiateHandler.cs +++ b/src/Security/Authentication/Negotiate/src/NegotiateHandler.cs @@ -42,7 +42,7 @@ namespace Microsoft.AspNetCore.Authentication.Negotiate { } /// - /// The handler calls methods on the events which give the application control at certain points where processing is occurring. + /// The handler calls methods on the events which give the application control at certain points where processing is occurring. /// If it is not provided a default instance is supplied which does nothing when the methods are called. /// protected new NegotiateEvents Events @@ -129,9 +129,9 @@ namespace Microsoft.AspNetCore.Authentication.Negotiate _negotiateState ??= Options.StateFactory.CreateInstance(); var outgoing = _negotiateState.GetOutgoingBlob(token, out var errorType, out var exception); - Logger.LogInformation(errorType.ToString()); if (errorType != BlobErrorType.None) { + Logger.NegotiateError(errorType.ToString()); _negotiateState.Dispose(); _negotiateState = null; if (persistence?.State != null) diff --git a/src/Security/Authorization/Core/src/DenyAnonymousAuthorizationRequirement.cs b/src/Security/Authorization/Core/src/DenyAnonymousAuthorizationRequirement.cs index 35828735a5..ccb896bbe1 100644 --- a/src/Security/Authorization/Core/src/DenyAnonymousAuthorizationRequirement.cs +++ b/src/Security/Authorization/Core/src/DenyAnonymousAuthorizationRequirement.cs @@ -32,7 +32,7 @@ namespace Microsoft.AspNetCore.Authorization.Infrastructure public override string ToString() { - return $"{nameof(DenyAnonymousAuthorizationRequirement)}:Requires an authenticated user."; + return $"{nameof(DenyAnonymousAuthorizationRequirement)}: Requires an authenticated user."; } } } diff --git a/src/Security/Authorization/Core/src/LoggingExtensions.cs b/src/Security/Authorization/Core/src/LoggingExtensions.cs index ef57f8c09f..fec8064a3d 100644 --- a/src/Security/Authorization/Core/src/LoggingExtensions.cs +++ b/src/Security/Authorization/Core/src/LoggingExtensions.cs @@ -20,7 +20,7 @@ namespace Microsoft.Extensions.Logging _userAuthorizationFailed = LoggerMessage.Define( eventId: new EventId(2, "UserAuthorizationFailed"), logLevel: LogLevel.Information, - formatString: "Authorization failed for {0}"); + formatString: "Authorization failed. {0}"); } public static void UserAuthorizationSucceeded(this ILogger logger) diff --git a/src/Security/Authorization/test/DefaultAuthorizationServiceTests.cs b/src/Security/Authorization/test/DefaultAuthorizationServiceTests.cs index ecd55a4acc..70d3045eda 100644 --- a/src/Security/Authorization/test/DefaultAuthorizationServiceTests.cs +++ b/src/Security/Authorization/test/DefaultAuthorizationServiceTests.cs @@ -1221,7 +1221,7 @@ namespace Microsoft.AspNetCore.Authorization.Test Assert.Equal("UserAuthorizationFailed", eventId.Name); var message = formatter(state, exception); - Assert.Equal("Authorization failed for These requirements were not met:" + Environment.NewLine + "LogRequirement" + Environment.NewLine + "LogRequirement", message); + Assert.Equal("Authorization failed. These requirements were not met:" + Environment.NewLine + "LogRequirement" + Environment.NewLine + "LogRequirement", message); } var authorizationService = BuildAuthorizationService(services => @@ -1241,7 +1241,7 @@ namespace Microsoft.AspNetCore.Authorization.Test // Assert } - + [Fact] public async Task Authorize_ShouldLogExplicitFailedWhenFailedCall() { @@ -1254,7 +1254,7 @@ namespace Microsoft.AspNetCore.Authorization.Test Assert.Equal("UserAuthorizationFailed", eventId.Name); var message = formatter(state, exception); - Assert.Equal("Authorization failed for Fail() was explicitly called.", message); + Assert.Equal("Authorization failed. Fail() was explicitly called.", message); } var authorizationService = BuildAuthorizationService(services => diff --git a/src/Security/Authorization/test/DenyAnonymousAuthorizationRequirementTests.cs b/src/Security/Authorization/test/DenyAnonymousAuthorizationRequirementTests.cs index 3d35634758..3b2011e2f3 100644 --- a/src/Security/Authorization/test/DenyAnonymousAuthorizationRequirementTests.cs +++ b/src/Security/Authorization/test/DenyAnonymousAuthorizationRequirementTests.cs @@ -15,7 +15,7 @@ namespace Microsoft.AspNetCore.Authorization.Test { return new DenyAnonymousAuthorizationRequirement(); } - + [Fact] public void ToString_ShouldReturnFormatValue() { @@ -26,7 +26,7 @@ namespace Microsoft.AspNetCore.Authorization.Test var formattedValue = requirement.ToString(); // Assert - Assert.Equal("DenyAnonymousAuthorizationRequirement:Requires an authenticated user.", formattedValue); + Assert.Equal("DenyAnonymousAuthorizationRequirement: Requires an authenticated user.", formattedValue); } } }