[Bug] Microsoft.Identity.Web should not use HttpContext.User in AddAccountToCacheFromAuthorizationCodeAsync

[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 and call web APIs
• Token cache serialization
  • [x ] In Memory caches
  • [x ] Session caches
  • [x ] Distributed caches

Repro
TokenAcquisition.AddAccountToCacheFromAuthorizationCodeAsync uses HttpContext.User whereas it’s not populated yet. Therefore we add some claims to the HttpContext.User.Identity coming from the ID Token:

microsoft-identity-web/src/Microsoft.Identity.Web/TokenAcquisition.cs

Lines 132 to 135 in 0b628e2

	if (!context.HttpContext.User.Claims.Any())
	{
	(context.HttpContext.User.Identity as ClaimsIdentity).AddClaims(context.Principal.Claims);
	}

We do that so that the token cache serialization can compute a cacheKey. See:

microsoft-identity-web/src/Microsoft.Identity.Web/TokenCacheProviders/MsalAbstractTokenCacheProvider.cs

Line 73 in b9457a8

: _httpContextAccessor.HttpContext.User.GetMsalAccountId();

Expected behavior
The default user is an anonymous identity (the HttpContext.User is replaced when the token is processed). As an Auth library, we should avoid accessing HttpContext.User anwyway. This field is only updated at specific times in the flow of the request, and with the default scheme of the request. It gets confusing when people are making cookies in your app, and that might not be the right identity in the scenario.

Prefered Solution

• Have this cache key computed and surfaced in the TokenCacheNotificationArgs (Expose a new string SuggestedCacheKey in TokenCacheNotificationArgs). See [Feature Request] Expose a new string SuggestedCacheKey in TokenCacheNotificationArgs  microsoft-authentication-library-for-dotnet#1902

This means that we would no longer need to compute the cache key.

Alternative solution

• otherwise add a new .WithTokenSerializationCacheKey(msalAccountId) method to the acquire token buidlers for confidentials client apps.

The code in TokenAcquisition.cs could be use a new modifier .WithTokenSerializationCacheKey(msalAccountId)

 public async Task AddAccountToCacheFromAuthorizationCodeAsync(
            AuthorizationCodeReceivedContext context,
            IEnumerable<string> scopes)
{
 string msalAccountId = context.Principal.GetMsalAccountId();

 var result = await _application
                    .AcquireTokenByAuthorizationCode(scopes.Except(_scopesRequestedByMsal), 
 context.ProtocolMessage.Code)
                    .WithSendX5C(_microsoftIdentityOptions.SendX5C)
                    .WithTokenSerializationCacheKey(msalAccountId)
                    .ExecuteAsync()
                    .ConfigureAwait(false);
}

Then this code could be simplified to take info from the TokenCacheNotificationArgs

microsoft-identity-web/src/Microsoft.Identity.Web/TokenCacheProviders/MsalAbstractTokenCacheProvider.cs

Lines 60 to 75 in b9457a8

	private string GetCacheKey(bool isAppTokenCache)
	{
	if (isAppTokenCache)
	{
	return $"{_microsoftIdentityOptions.Value.ClientId}_AppTokenCache";
	}
	else
	{
	// In the case of Web Apps, the cache key is the user account Id, and the expectation is that AcquireTokenSilent
	// should return a token otherwise this might require a challenge.
	// In the case Web APIs, the token cache key is a hash of the access token used to call the Web API
	JwtSecurityToken jwtSecurityToken = _httpContextAccessor.HttpContext.GetTokenUsedToCallWebAPI();
	return (jwtSecurityToken != null) ? jwtSecurityToken.RawSignature
	: _httpContextAccessor.HttpContext.User.GetMsalAccountId();
	}
	}

[jennyf19]

@jmprieur doing this in 3 parts.
[1] Waiting on a PR to merge in MSAL .NET
[2] Continue the cache key work w/new package, already have started w/this branch
[3] Address the claims issue

[jennyf19]

Included in 0.2.0-preview release