Better logs in AuthorizationMiddleware #43862
Conversation
ghost
added
area-auth
|
Thanks for your PR, @luccawilli. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
|
|
|
|
||
| if (authenticateResult != null && !authenticateResult.Succeeded) | ||
| { | ||
| _logger.LogDebug("Policy authentication schemes {policyName} did not succeed", String.Join(", ", policy.AuthenticationSchemes)); |
There was a problem hiding this comment.
What about logging this in the policyEvaluator instead?
There was a problem hiding this comment.
That would be also an option.
I just wanted as few log entries as possible. For me, it would have been enough to know that the combination does not work instead of each individual policy.
|
@halter73 now is the time when you say I told you so 😄 |
|
I'm not sure I follow @davidfowl. This isn't changing the logging in the @Tratcher merged the PR that issue anyway. We didn't end up just not logging authentication failures for anonymous endpoints like I suggested. We did something else. Or are we talking about something else? |
| #nullable enable | ||
| Microsoft.AspNetCore.Authorization.AuthorizationMiddleware | ||
| Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.AuthorizationMiddleware(Microsoft.AspNetCore.Http.RequestDelegate! next, Microsoft.AspNetCore.Authorization.IAuthorizationPolicyProvider! policyProvider) -> void | ||
| Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.AuthorizationMiddleware(Microsoft.AspNetCore.Http.RequestDelegate! next, Microsoft.AspNetCore.Authorization.IAuthorizationPolicyProvider! policyProvider, Microsoft.Extensions.Logging.ILogger<Microsoft.AspNetCore.Authorization.AuthorizationMiddleware!>! logger) -> void |
There was a problem hiding this comment.
Previously shipped APIs can't be modified. You need to add new overloads instead.
There was a problem hiding this comment.
Okay, thank you for the tip, I wasn't sure how to handle that😅
I mean making this middleware internal and adding the impl so we don't need public APIs to add dependencies. |
| /// <param name="policyProvider">The <see cref="IAuthorizationPolicyProvider"/>.</param> | ||
| /// <param name="services">The <see cref="IServiceProvider"/>.</param> | ||
| public AuthorizationMiddleware(RequestDelegate next, IAuthorizationPolicyProvider policyProvider, IServiceProvider services) : this(next, policyProvider) | ||
| /// <param name="logger">The <see cref="ILogger"/>.</param> |
There was a problem hiding this comment.
How do we feel about just service locating ILogger<AuthorizationMiddleware> so we don't have to add any new ctors, that would simplify the change a lot
There was a problem hiding this comment.
Only the longest constructor should be called by DI, I don't think we need the other new combinations. One new constructor isn't a big deal.
There was a problem hiding this comment.
@luccawilli can you remove all the extra new constuctor overloads except the one we need
src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Stephen Halter <[email protected]>
|
Thanks @luccawilli |
6 participants
Better logs in AuthorizationMiddleware
Summary of the changes (Less than 80 chars)
Description
Added a logger to AuthorizationMiddleware and log not succeeded policies with Debug-Level
Fixes #43861