-
Notifications
You must be signed in to change notification settings - Fork 223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Feature Request] Improved support for inheriting / customizing MicrosoftIdentity*AuthenticationHandler #1667
Comments
I created a branch with an example of the type of hooks I'm thinking of here: https://github.com/MattKotsenas/microsoft-identity-web/commits/feature/di-auth-message-handlers Each commit showcases a specific idea, the second shows adding an With these two changes together, my service would override the behavior with something like this: public class Startup
{
public void ConfigureServices(IServiceCollection services)
{
// register other services
services.Replace(ServiceDescriptor.Singleton<IDelegatingHandlerFactory, MyDelegatingHandlerFactory>());
}
}
internal class MyDelegatingHandlerFactory : IDelegatingHandlerFactory
{
public DelegatingHandler CreateAppHandler(IServiceProvider serviceProvider, string? name)
{
return new AuditingMicrosoftIdentityAppAuthenticationMessageHandler(
serviceProvider.GetRequiredService<ITokenAcquisition>(),
serviceProvider.GetRequiredService<IOptionsMonitor<MicrosoftIdentityAuthenticationMessageHandlerOptions>>(),
serviceProvider.GetRequiredService<ILogger<AuditingMicrosoftIdentityAppAuthenticationMessageHandler>>(),
name);
}
public DelegatingHandler CreateUserHandler(IServiceProvider serviceProvider, string? name)
{
// ...snip for brevity...
}
}
internal class MyAppAuthenticationMessageHandler : MicrosoftIdentityAppAuthenticationMessageHandler
{
private readonly ILogger<MyAppAuthenticationMessageHandler > _logger;
public MyAppAuthenticationMessageHandler (
ITokenAcquisition tokenAcquisition,
IOptionsMonitor<MicrosoftIdentityAuthenticationMessageHandlerOptions> namedMessageHandlerOptions,
ILogger<MyAppAuthenticationMessageHandler > logger,
string? serviceName = null)
: base(tokenAcquisition, namedMessageHandlerOptions, serviceName)
{
_logger = logger;
}
protected override async Task<AuthenticationResult> GetTokenAsync(MicrosoftIdentityAuthenticationMessageHandlerOptions options)
{
try
{
return await base.GetTokenAsync(options).ConfigureAwait(false);
}
catch (Exception ex)
{
_logger.LogWarning(ex, "Can't get token");
}
// add other logic; empty token here as an example
return new AuthenticationResult(string.Empty, false, string.Empty, DateTimeOffset.UtcNow, DateTimeOffset.UtcNow, string.Empty, null, string.Empty, new string[] { }, Guid.NewGuid());
}
} Please let me know if this example helps illustrate my situation, and if changes along these lines seem reasonable. If there's also a different way to be approaching this problem, please let me know! Thanks! |
@MattKotsenas do we want to try that you submit a PR? |
Released in 1.24.0. |
Is your feature request related to a problem?
My specific scenario is likely niche, but I think the capability would be broadly applicable. For context, in my case I have a web API that makes protected downstream web API calls using the
AddMicrosoftIdentityAppAuthenticationHandler
route. I chose this method overIDownstreamApi
because I have a set of HttpClient delegating handlers (like retry and backoff policies) that vary per HttpClient type.My specific scenario is that my downstream API has a mix of authenticated and anonymous endpoints, and the
AzureAd
config provided to each instance of my web API may be different and may have been misconfigured by the user. That means that if the user provides a bad configuration, all downstream API calls will fail, even the anonymous ones, because there's notry...catch
around token acquisition inMicrosoftIdentityAppAuthenticationMessageHandler.cs
(seemicrosoft-identity-web/src/Microsoft.Identity.Web/DownstreamWebApiSupport/MicrosoftIdentityAppAuthenticationMessageHandler.cs
Lines 44 to 49 in 2f46572
Describe the workaround you have today
Today I work around this issue by copying the entirety of
MicrosoftIdentityAppAuthenticationMessageHandler.cs
into my library to add thetry...catch
, and then additionally I need to create a new override ofAddMicrosoftIdentityAppAuthenticationHandler
with the implementation copied so that I can add my "safe" handler as the HTTP message handler (seemicrosoft-identity-web/src/Microsoft.Identity.Web/DownstreamWebApiSupport/MicrosoftIdentityAuthenticationMessageHandlerHttpClientBuilderExtensions.cs
Line 97 in 2f46572
Describe the solution you'd like
Depending on the level of support that makes sense for a scenario like this, I propose a tiered approach:
Tier 1: Register and retrieve the MicrosoftIdentity*AuthenticationHandler classes from DI
If instead of calling
new
directly on the handler classes in the extension methods, if the handlers were registered as dependencies and resolved from theServiceCollection
, I could inherit from the handler to do my custom work, which would eliminate the need to also duplicate the extension method. Bonus points if the authentication handlers gain an interface.Tier 2: Move token acquisition into a virtual method in the handlers
In this tier, if we moved the token acquisition, i.e.:
into a
virtual
method, then I could override just this method and add atry...catch
, which would avoid needing to re-implement the rest of the class.Describe alternatives you've considered
I added a section above with the workaround I have today. Alternatively, I could avoid the situation all together by splitting my downstream API into two different HttpClients, one for the authenticated endpoints and another for the anonymous endpoints, however in addition to having some negative performance characteristics (e.g. double TCP connections open to the same host), it also would lead to a suboptimal developer experience for my users, as the two sets of APIs aren't logically distinct, so it wouldn't be clear why some APIs use one client and some APIs use another.
Let me know if this request makes sense or not, and if you have any questions. Thanks!
The text was updated successfully, but these errors were encountered: