Microsoft-identity-web: [Bug] Microsoft.Identity.Web should not call BuildServiceProvider

[jmprieur]

Which Version of Microsoft Identity Web are you using ?
Microsoft Identity Web 0.1.5-preview

Where is the issue?

• Web App
  
  
  
  • [x ] Sign-in users
  
  
• Web API
  
  
  
  • [x ] Protected web APIs (Validating tokens)
  
  

Repro
See:

https://github.com/AzureAD/microsoft-identity-web/blob/5bc881bb6088f92db6e157f839bffb924627b215/src/Microsoft.Identity.Web/WebApiAuthenticationBuilderExtensions.cs#L145

https://github.com/AzureAD/microsoft-identity-web/blob/5bc881bb6088f92db6e157f839bffb924627b215/src/Microsoft.Identity.Web/WebAppAuthenticationBuilderExtensions.cs#L202

Expected behavior
NEVER call BuildServiceProvider.

Actual behavior
Microsoft.Identity.Web calls BuildServiceProvider

Discussion

• It's a MUST FIX
• In NET 5.0, there is an overload of AddJwtBearer / AddOpenIdConnect with a service that you want to inject
• We need to come-up with alternatives for 3.0. @Tratcher will help

• in addition raise an issue with ASP.NET Core to have these diagnostics available by default without wrapping the events.

Possible design
(but feel free to do differently)

1. Add a constant in the Microsoft.Identity.Web.csproj file depending on the TargetFramework (maybe something like:

XML <PropertyGroup Condition="'$(TargetFramework)' == 'netcoreapp3.1'"> <DefineConstants>$(DefineConstants);DOTNET_CORE_31</DefineConstants> </PropertyGroup>

1. Based on the constant use one form of the other of AddOpenIdConnect and AddJwtBearer. For instance for AddOpenIdConnect we could have something like the following:

   #if DOTNET_CORE_31
           builder.AddOpenIdConnect(openIdConnectScheme, options =>
           {
            // Todo: replace by the work around that @Tratcher will provider
            IServiceProvider serviceProvider = builder.Services.BuildServiceProvider();
   #else
           builder.AddOpenIdConnect<IServiceProvider>(openIdConnectScheme, (options, serviceProvider) =>
           {
   #endif
   

Note that the aspnetcoreapp3.1 case would still use the BuildServiceProvider until Chris provides a workaround

1. In the section garded by the subscribeToXXXMiddlewareDiagnosticsEvents boolean, just use the serviceProvider to call GetRequiredService<>
   For instance for the OIDC case, something like:

   var diags = serviceProvider.GetRequiredService<IOpenIdConnectMiddlewareDiagnostics>();
   

2. Consider doing #239 soon after this one, as it leverages similar mechanisms

3. Follow-up with @Tratcher for the NET 3.1 work around to populate the service provider in the case of netcore3.1

[Tratcher]

https://github.com/dotnet/aspnetcore/issues/18772#issuecomment-588302416
This should work in prior versions:

services.AddOptions<OpenIdConnectOptions>(authSchemeName)
  .Configure<IOpenIdConnectMiddlewareDiagnostics>((options, diagnostics) =>
  {
  });

In fact, you could do it in both versions and avoid the #ifs.

[jmprieur]

thanks @Tratcher

[jmprieur]

So I understand that the section in WebApiAuthenticationBuilderExtensions.cs could be:

#if DOTNET_CORE_31
            // Change the authentication configuration to accommodate the Microsoft identity platform endpoint (v2.0).
            builder.AddJwtBearer(jwtBearerScheme, options => { });
            builder.Services.AddOptions<JwtBearerOptions>(jwtBearerScheme)
                .Configure<IServiceProvider>((options, serviceProvider) =>
            {
#else
            builder.AddJwtBearer<IServiceProvider>(jwtBearerScheme, (options, serviceProvider) =>
            {
#endif

And in WebAppAuthenticationBuilderExtensions.cs, it could be:

#if DOTNET_CORE_31
            builder.AddOpenIdConnect(openIdConnectScheme, options => { });
            builder.Services.AddOptions<OpenIdConnectOptions>(openIdConnectScheme)
                .Configure<IServiceProvider>((options, serviceProvider) =>
                {
#else
            builder.AddOpenIdConnect<IServiceProvider>(openIdConnectScheme, (options, serviceProvider) =>
            {
#endif

@jennyf19, moving back to "in progress" now that Chris has shared the work around for .NET Core 3.1

[jmprieur]

Finalized (even better), part of https://github.com/AzureAD/microsoft-identity-web/pull/277

[jennyf19]

Included in 0.2.0-preview release