unitycontainer / microsoft-dependency-injection

Unity.Microsoft.DependencyInjection package
Apache License 2.0
63 stars 36 forks source link

Unity 5.8.13 appears to call AddJwtBearer extension method when it is not expected to #34

Closed bunceg closed 5 years ago

bunceg commented 5 years ago

Description

.NET Core 2.1.3 Web Api app with Unity Dependency Injection causes the AddJwtBearer extension method of the Microsoft.AspNetcore.Authentication.AuthenticationBuilder method "AddAuthentication" to be called multiple times.

When the "in-built" DI is used, this is not the case.

According to this comment https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/1078#issuecomment-448406475 this is not an ASP.NET Core issue but likely to be a DI issue.

This is configured using the Unity.Microsoft.DependencyInjection (v2.1.3) package.

To Reproduce

This is not a Unit Test issue, more an environmental one. Our Web Api uses JwtBearer tokens for authentication submitted via Identity Server.

Unity is configured like this in program.cs

public static IWebHostBuilder CreateWebHostBuilder(string[] args) =>
            WebHost.CreateDefaultBuilder(args)
                .UseUnityServiceProvider()
                .UseNLog()
                .UseStartup<Startup>();

and this in Startup.cs

public void ConfigureContainer(IUnityContainer services)
{
    try
    {
        services.RegisterType<IAsyncQueryHandler<GetPingQuery, GetPingResult>, GetPingHandler>();
        services.RegisterType<ILogger, NLogLogger>();
        services.RegisterType<IClock, SystemClock>();
        services.RegisterInstance<IConfigurationAdapter>(new ConfigurationAdapter(Configuration));
        services.RegisterType<ICorrelationRegistry, AsyncLocalCorrelationRegistry>(new SingletonLifetimeManager());
    }
    catch (Exception ex)
    {
        new NLogExceptionPublisher(new SystemClock()).HandleException(ex);
        throw;
    }
}

our authentication middleware hangs off the usual "ConfigureServices(IServiceCollection services) method of startup.cs

ENikS commented 5 years ago

I am pretty sure Unity does not call AddJwtBearer. Neither Unity nor me ever heard about it. :) Are you sure you register everything correctly?

ENikS commented 5 years ago

On second thought, I would check if it adds itself as a singleton or transient. That could be an issue with multiple calls.

ENikS commented 5 years ago

@davidfowl do you know anything about this one or perhaps could point to someone who does?

bunceg commented 5 years ago

@ENikS Thanks for the reply... AddJwtBearer isn't really a thing that you explicitly register with DI - it's just an extension method that is exposed when you include a microsoft package. The code you define is something like this:

services.AddAuthentication(JwtBearerDefaults.AuthenticationScheme)
    .AddJwtBearer(options =>
    {
        //Invoked once on start up
        options.IncludeErrorDetails = true;
        // etc.
        options.Events = new JwtBearerEvents
        {
            OnTokenValidated = context =>
            {
                //Invoked on each api call
                return Task.CompletedTask;
            }
        };
    });

What's happening with Unity is that the anonymous "options =>" method is invoked on every api call. What's meant to happen is that only the OnTokenValidated anonymous method is invoked on every api call.

This doesn't happen with the inbuilt DI - it works as expected.

Obviously this is a weird issue that I assume is a Unity one as it doesn't occur with the default DI... but it may be something odd going on with the way the AddJwtBearer extension method is written.... it's really hard to know where to start with this.

This comment is interesting : https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/1078#issuecomment-451985447. I've spot checked some objects that AddJwtBearer appears to register as singletons and I can see them in the list of Unity registrations correctly defined as singletons... so I'm not sure this is the case.

ENikS commented 5 years ago

If you could provide working example I could try to help.

Looking at the code above I am guessing you are providing a factory for the type. Factories are transient by the specification. You may want to run it by aspnet team to verify.

Instead of using Service Collection and provided extension method to configure Unity, why don't you try to configure it using InjectionFactory instead? Unfortunately I am not familiar with the API to give you better suggestion. Again, working example could help.

bunceg commented 5 years ago

@ENikS - I have an example web api application that I can switch between Unity DI and in-built DI. How would you like me to send it to you?

davidfowl commented 5 years ago

Make a GitHub repository. That would be best

bunceg commented 5 years ago

@davidfowl @ENikS

GitHub repository can be found here: https://github.com/grahambunce/unityinjectionjwtbearer

I've been looking at the IdentityServer4 client (https://github.com/IdentityServer/IdentityServer4.AccessTokenValidation/blob/master/src/IdentityServerAuthenticationExtensions.cs) as they have an explicit "builder.Services.AddSingleton<IConfigureOptions>(xxx)" step but when I dig into the AddJwtBearer code, this has a registration anyway so I'm not sure if this is an indicator or not of something I'm doing wrong.

ENikS commented 5 years ago

Could you verify if it still broken with latest 5.9.0.-RC1 I have fixed significant number of issues with DI and it might already be working.

bunceg commented 5 years ago

@ENikS I'm not too sure how I can. My test project references Unity.Microsoft.DependencyInjection v2.1.3 and there is no RC candidate on nuget for this. So instead, I update one this packages dependencies (Unity.Container) to 5.9.0-RC1. This then automatically pulls in and updates the reference to Unity.Abstractions to 4.0.0-RC1.

When I then run the program I get a "IUnityContainter" is defined in an assembly that is not referenced. You must add a reference to 'Unity.Abstractions' version 3.3.1.0.

I'll be honest, now Binding Redirects have gone in .net core, I have no idea how to work around this. Do I need an RC version of Unity.Microsoft.DependencyInjection

ENikS commented 5 years ago

5.9.0 is out with all the assemblies

bunceg commented 5 years ago

@ENikS Unfortunately upgrading to 5.9.0 hasn't fixed the problem :(

ENikS commented 5 years ago

I am running your app, how do I replicate the issue? This line is called on page load. This line is never executed. The response I get:

{
  "response": {
    "api ping": "api layer ok",
    "domain layer": "domain layer ok"
  }
}
bunceg commented 5 years ago

@ENikS Hi, yes that's correct behaviour for the error scenario. Line 58 (first one) is called on startup and on each subsequent call. Line 68 (second one) is never called and isn't needed to be called in this case, just to highlight what should occur in a normal scenario.

What should happen is that line 58 is only ever called once, on startup. Subsequent calls to the api will not invoke line 58 again. In an app that was using proper authenticated apis, then line 68 would be called on every api call (not line 58) but as we're running anonymous... line 68 is basically irrelevant to the error scenario.

If you then change the code so that you comment out lines 98 - 103 in startup.cs... and also comment out line 16 in program.cs... then uncomment line 83... then you will be running with the default DI container instead of Unity.

If you then re-run the program you'll see correct behaviour, i.e. line 58 is only ever called once and never called again no matter how many times you call the api.

Does that help?

ENikS commented 5 years ago

I can reproduce the error but I see no Unity code executed. I am assuming Unity is used to resolve it at some point but I have no clue when.

>   UnityInjection.Api.dll!UnityInjection.Api.Startup.ConfigureServices.AnonymousMethod__7_2(Microsoft.AspNetCore.Authentication.JwtBearer.JwtBearerOptions options) Line 58    C#
    Microsoft.Extensions.Options.dll!Microsoft.Extensions.Options.ConfigureNamedOptions<System.__Canon>.Configure(string name, System.__Canon options) Line 52  C#
    Microsoft.Extensions.Options.dll!Microsoft.Extensions.Options.OptionsFactory<Microsoft.AspNetCore.Authentication.JwtBearer.JwtBearerOptions>.Create(string name) Line 35    C#
    Microsoft.Extensions.Options.dll!Microsoft.Extensions.Options.OptionsMonitor<Microsoft.AspNetCore.Authentication.JwtBearer.JwtBearerOptions>.Get.AnonymousMethod__0() Line 64   C#
    System.Private.CoreLib.dll!System.Lazy<Microsoft.AspNetCore.Authentication.JwtBearer.JwtBearerOptions>.ViaFactory(System.Threading.LazyThreadSafetyMode mode)   Unknown
    System.Private.CoreLib.dll!System.Lazy<System.__Canon>.ExecutionAndPublication(System.LazyHelper executionAndPublication, bool useDefaultConstructor)   Unknown
    System.Private.CoreLib.dll!System.Lazy<Microsoft.AspNetCore.Authentication.JwtBearer.JwtBearerOptions>.CreateValue()    Unknown
    Microsoft.Extensions.Options.dll!Microsoft.Extensions.Options.OptionsCache<Microsoft.AspNetCore.Authentication.JwtBearer.JwtBearerOptions>.GetOrAdd(string name, System.Func<Microsoft.AspNetCore.Authentication.JwtBearer.JwtBearerOptions> createOptions) Line 35 C#
    Microsoft.Extensions.Options.dll!Microsoft.Extensions.Options.OptionsMonitor<Microsoft.AspNetCore.Authentication.JwtBearer.JwtBearerOptions>.Get(string name) Line 64   C#
    Microsoft.AspNetCore.Authentication.dll!Microsoft.AspNetCore.Authentication.AuthenticationHandler<Microsoft.AspNetCore.Authentication.JwtBearer.JwtBearerOptions>.InitializeAsync(Microsoft.AspNetCore.Authentication.AuthenticationScheme scheme, Microsoft.AspNetCore.Http.HttpContext context)   Unknown
    System.Private.CoreLib.dll!System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start<Microsoft.AspNetCore.Authentication.AuthenticationHandler<Microsoft.AspNetCore.Authentication.JwtBearer.JwtBearerOptions>.<InitializeAsync>d__42>(ref Microsoft.AspNetCore.Authentication.AuthenticationHandler<Microsoft.AspNetCore.Authentication.JwtBearer.JwtBearerOptions>.<InitializeAsync>d__42 stateMachine)    Unknown
    Microsoft.AspNetCore.Authentication.dll!Microsoft.AspNetCore.Authentication.AuthenticationHandler<Microsoft.AspNetCore.Authentication.JwtBearer.JwtBearerOptions>.InitializeAsync(Microsoft.AspNetCore.Authentication.AuthenticationScheme scheme, Microsoft.AspNetCore.Http.HttpContext context)   Unknown
    Microsoft.AspNetCore.Authentication.Core.dll!Microsoft.AspNetCore.Authentication.AuthenticationHandlerProvider.GetHandlerAsync(Microsoft.AspNetCore.Http.HttpContext context, string authenticationScheme)  Unknown
    System.Private.CoreLib.dll!System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start<Microsoft.AspNetCore.Authentication.AuthenticationHandlerProvider.<GetHandlerAsync>d__5>(ref Microsoft.AspNetCore.Authentication.AuthenticationHandlerProvider.<GetHandlerAsync>d__5 stateMachine)  Unknown
    Microsoft.AspNetCore.Authentication.Core.dll!Microsoft.AspNetCore.Authentication.AuthenticationHandlerProvider.GetHandlerAsync(Microsoft.AspNetCore.Http.HttpContext context, string authenticationScheme)  Unknown
    Microsoft.AspNetCore.Authentication.Core.dll!Microsoft.AspNetCore.Authentication.AuthenticationService.AuthenticateAsync(Microsoft.AspNetCore.Http.HttpContext context, string scheme)  Unknown
    System.Private.CoreLib.dll!System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start<Microsoft.AspNetCore.Authentication.AuthenticationService.<AuthenticateAsync>d__10>(ref Microsoft.AspNetCore.Authentication.AuthenticationService.<AuthenticateAsync>d__10 stateMachine)    Unknown
    Microsoft.AspNetCore.Authentication.Core.dll!Microsoft.AspNetCore.Authentication.AuthenticationService.AuthenticateAsync(Microsoft.AspNetCore.Http.HttpContext context, string scheme)  Unknown
    Microsoft.AspNetCore.Authentication.Abstractions.dll!Microsoft.AspNetCore.Authentication.AuthenticationHttpContextExtensions.AuthenticateAsync(Microsoft.AspNetCore.Http.HttpContext context, string scheme)    Unknown
    Microsoft.AspNetCore.Authentication.dll!Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(Microsoft.AspNetCore.Http.HttpContext context)  Unknown
    System.Private.CoreLib.dll!System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start<Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.<Invoke>d__6>(ref Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.<Invoke>d__6 stateMachine)  Unknown
    Microsoft.AspNetCore.Authentication.dll!Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(Microsoft.AspNetCore.Http.HttpContext context)  Unknown
    Microsoft.AspNetCore.Diagnostics.dll!Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(Microsoft.AspNetCore.Http.HttpContext context)    Unknown
    System.Private.CoreLib.dll!System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start<Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.<Invoke>d__7>(ref Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.<Invoke>d__7 stateMachine)    Unknown
    Microsoft.AspNetCore.Diagnostics.dll!Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(Microsoft.AspNetCore.Http.HttpContext context)    Unknown
    Microsoft.AspNetCore.Server.IISIntegration.dll!Microsoft.AspNetCore.Server.IISIntegration.IISMiddleware.Invoke(Microsoft.AspNetCore.Http.HttpContext httpContext)   Unknown
    System.Private.CoreLib.dll!System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start<Microsoft.AspNetCore.Server.IISIntegration.IISMiddleware.<Invoke>d__13>(ref Microsoft.AspNetCore.Server.IISIntegration.IISMiddleware.<Invoke>d__13 stateMachine)    Unknown
    Microsoft.AspNetCore.Server.IISIntegration.dll!Microsoft.AspNetCore.Server.IISIntegration.IISMiddleware.Invoke(Microsoft.AspNetCore.Http.HttpContext httpContext)   Unknown
    Microsoft.AspNetCore.HttpOverrides.dll!Microsoft.AspNetCore.HttpOverrides.ForwardedHeadersMiddleware.Invoke(Microsoft.AspNetCore.Http.HttpContext context)  Unknown
    Microsoft.AspNetCore.HostFiltering.dll!Microsoft.AspNetCore.HostFiltering.HostFilteringMiddleware.Invoke(Microsoft.AspNetCore.Http.HttpContext context) Unknown
    Microsoft.AspNetCore.Hosting.dll!Microsoft.AspNetCore.Hosting.Internal.RequestServicesContainerMiddleware.Invoke(Microsoft.AspNetCore.Http.HttpContext httpContext) Unknown
    Microsoft.AspNetCore.Hosting.dll!Microsoft.AspNetCore.Hosting.Internal.HostingApplication.ProcessRequestAsync(Microsoft.AspNetCore.Hosting.Internal.HostingApplication.Context context) Unknown
    Microsoft.AspNetCore.Server.Kestrel.Core.dll!Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.ProcessRequests<Microsoft.AspNetCore.Hosting.Internal.HostingApplication.Context>(Microsoft.AspNetCore.Hosting.Server.IHttpApplication<Microsoft.AspNetCore.Hosting.Internal.HostingApplication.Context> application)  Unknown
    System.Private.CoreLib.dll!System.Threading.ExecutionContext.RunInternal(System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, object state)   Unknown
    System.Private.CoreLib.dll!System.Runtime.CompilerServices.AsyncTaskMethodBuilder<System.Threading.Tasks.VoidTaskResult>.AsyncStateMachineBox<Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.<ProcessRequests>d__188<Microsoft.AspNetCore.Hosting.Internal.HostingApplication.Context>>.MoveNext() Unknown
    System.Private.CoreLib.dll!System.Threading.ExecutionContext.RunInternal(System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, object state)   Unknown
    System.Private.CoreLib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch()  Unknown
bunceg commented 5 years ago

@ENikS Well.. at least you can reproduce so that's something. I don't know where to start either! Maybe ping the related thread for help? https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/1078#issuecomment-456458869

ENikS commented 5 years ago

To switch between DIs you need just this one line

ENikS commented 5 years ago

I just cloned entire Extensions Repo, built your code with project references instead of nuget packages and tried to reproduce the error.

I could not!

So, I assume that whatever issues making this sample fail in 2.1.0 were fixed in 2.2.0. It also means that Unity is not the cause of this behavior. I am closing this issue, feel free to reopen if more info is available.