2.15.1 must wait all OnTokenValidated delegates to completion

[recumbented]

Microsoft.Identity.Web Library

Microsoft.Identity.Web

Microsoft.Identity.Web version

2.15.1

Web app

Sign-in users

Web API

Protected web APIs (validating tokens)

Token cache serialization

In-memory caches

Description

Related to #2508

Since 2.15.1, OnTokenValidated is used as MulticastDelegate.
So await _onTokenValidated(context); will not wait it's invocations.
This causes unexpected behavior in users's OnTokenValidated invocation.

It's desired to wait all invocations like

await Task.WhenAll(((MulticastDelegate)_onTokenValidated)
	.GetInvocationList()
	.Cast<Func<TokenValidatedContext, Task>>()
	.Select(t=>t(context)));

Reproduction steps

If you use scoped services from DI in OnTokenValidated,
async method invocation may throw ObjectDisposedException.

Error message

No response

Id Web logs

No response

Relevant code snippets

options.Events.OnTokenValidated = async context =>
{
    var dep = context.HttpContext.RequestServices.GetRequiredService<Service>();
    await Task.Delay(1000);
    await dep.UseResourceAsync(); //Out of DI scope at the time. ObjectDisposedException may be thrown.
}

### Regression

_No response_

### Expected behavior

All OpenIdConnectEvents delegates must be awaited to completion.

[jmprieur]

@recumbented : do you think it would be better to use delegate chaining?

[recumbented]

@jmprieur Therefore OnTokenValidated exposes HttpContext, all invocations should be run in HttpContext scope.
And ensure backward compatibility, all invocations will be run sequentially and awaited is better I think.
Like

foreach (var t in (((MulticastDelegate)_onTokenValidated)
	.GetInvocationList()
	.Cast<Func<TokenValidatedContext, Task>>()))
{
	await t(_context).ConfigureAwait(false);
}

And invoke at first below resolves issue #2508.

microsoft-identity-web/src/Microsoft.Identity.Web/WebAppExtensions/MicrosoftIdentityWebAppAuthenticationBuilder.cs

Lines 168 to 183 in 0a212c4

	options.Events.OnTokenValidated += async context =>
	{
	string? clientInfo = context!.ProtocolMessage?.GetParameter(ClaimConstants.ClientInfo);
	if (!string.IsNullOrEmpty(clientInfo))
	{
	ClientInfo? clientInfoFromServer = ClientInfo.CreateFromJson(clientInfo);
	if (clientInfoFromServer != null && clientInfoFromServer.UniqueTenantIdentifier != null && clientInfoFromServer.UniqueObjectIdentifier != null)
	{
	context!.Principal!.Identities.FirstOrDefault()?.AddClaim(new Claim(ClaimConstants.UniqueTenantIdentifier, clientInfoFromServer.UniqueTenantIdentifier));
	context!.Principal!.Identities.FirstOrDefault()?.AddClaim(new Claim(ClaimConstants.UniqueObjectIdentifier, clientInfoFromServer.UniqueObjectIdentifier));
	}
	}
	await Task.CompletedTask;
	};

[jmprieur]

@westin-m : in 2.15.1, this PR introduced an undesirable behavioral change which I had not though of.

What to do:
In MicrosoftIdentityWebAppAuthenticationBuilder undo the changes lines 167-183:

// Handling the token validated to get the client_info for cases where tenantId is not present (example: B2C)
var onTokenValidatedHandler = options.Events.OnTokenValidated;
options.Events.OnTokenValidated = async context =>
{
   string? clientInfo = context!.ProtocolMessage?.GetParameter(ClaimConstants.ClientInfo);
   if (!string.IsNullOrEmpty(clientInfo))
      {
          ClientInfo? clientInfoFromServer = ClientInfo.CreateFromJson(clientInfo);
            if (clientInfoFromServer != null && clientInfoFromServer.UniqueTenantIdentifier != null && clientInfoFromServer.UniqueObjectIdentifier != null)
         {
            context!.Principal!.Identities.FirstOrDefault()?.AddClaim(new Claim(ClaimConstants.UniqueTenantIdentifier, clientInfoFromServer.UniqueTenantIdentifier));
            context!.Principal!.Identities.FirstOrDefault()?.AddClaim(new Claim(ClaimConstants.UniqueObjectIdentifier, clientInfoFromServer.UniqueObjectIdentifier));
         }
      }
       await onTokenValidatedHandler(context).ConfigureAwait(false);
 };

cc: @jennyf19 to prioritize

[westin-m]

@recumbented A fix for this is proposed in #2524. If you can, please try it for yourself and let us know the results.

[recumbented]

@westin-m Thanks. I tested and confirmed it works as expected.

[sruke]

Microsoft.Identity.Web v2.15.2 contains a fix for this issue.