Microsoft-identity-web: [Bug] DistributedSqlServerCache GetAccounts returns null despite having Accounts in TokenCache

Created on 9 Oct 2020 · 9Comments · Source: AzureAD/microsoft-identity-web

Which Version of MSAL are you using ?
Microsoft.Identity.Client 1.1.0

Platform
netcoreapp3.1

What authentication flow has the issue?
Other: not directly related to authentication flow but rather reading value from a populated TokenCache

Is this a new or existing app?
New test app to test update path for existing app

Repro

Prime the TokenCache with a successfully authenticated user flow (following standard Sample).
In my case I used .AddDistributedTokenCaches() with .AddDistributedSqlServerCache().
Database now shows one (non-expired) record in the TokenCache with let's say Id = 123.456 (userId and tenantId combo)

// _applicationOptions injected as IOptions<ConfidentialClientApplicationOptions>
// _httpClientFactory injected as IHttpClientFactory (and wrapped in MsalAspNetCoreHttpClientFactory)
// _tokenCacheProvider injected as IMsalTokenCacheProvider 
var confidentialClient = ConfidentialClientApplicationBuilder
                .CreateWithApplicationOptions(_applicationOptions)
                .WithHttpClientFactory(_httpClientFactory)
                .WithAuthority($"{_applicationOptions.Instance}{_applicationOptions.TenantId}/")
                .Build();

await _tokenCacheProvider.InitializeAsync(confidentialClient.AppTokenCache).ConfigureAwait(false);
await _tokenCacheProvider.InitializeAsync(confidentialClient.UserTokenCache).ConfigureAwait(false);

// this returns 0 accounts
var accounts= await confidentialClient.GetAccountsAsync();
// where this returns the account and proves there is an account in the TokenCache
var account = await confidentialClient.GetAccountAsync("123.456"); // replace with actual id from tokenCache

Expected behavior
GetAccountsAsync should return the accounts in the TokenCache.

Actual behavior
GetAccountsAsync returns an empty array

Possible Solution
SuggestedWebCacheKeyFactory should not return null for ApiId==ApiIds.GetAccounts

Additional context/ Logs / Screenshots
The problem occurs because GetAccounts, initiates a Read from the TokenCache but SuggestedWebCacheKeyFactory returns null for ApiIds.GetAccounts, which causes TokenCache not to read anything.

More verbose:

Starting in Microsoft.Identity.Client/ClientApplicationBase.cs
- GetAccountsAsync passes ApiIds.GetAccounts to
- GetAccountsInternalAsync passes it to
- GetAccountsFromCacheAsync to create the AuthenticationRequestParameters and set ApiId there.
- Instantiates CacheSessionManager with these params (at ClientApplicationBase.cs:205)
- then calls CacheSessionManager.GetAccountsAsync()
- which in turn calls RefreshCacheForReadOperationsAsync
- where the cache key is generated using:
- SuggestedWebCacheKeyFactory.GetKeyFromRequest(_requestParams); (_requestParams is the above AuthenticationRequestParameters) which returns null because there is no handling when ApiId==ApiIds.GetAccounts
- The MsalDistributedTokenCacheAdapter correctly checks for a null cacheKey and does not read from TokenCache.

Answered question

Source

aduggleby

All 9 comments

@aduggleby in confidential client scenarios, you need to use one cache per account (for performance and security), and therefore GetAccountsAsync() does not make sense when you are not in the context of an account.

What is your scenario?
why would you want to know the accounts?
What token cache provided did you use? one of the Microsoft.Identity.Web ones? https://github.com/AzureAD/microsoft-identity-web/tree/master/src/Microsoft.Identity.Web/TokenCacheProviders ?

In general, given you are on asp.net core, we strongly recommend that you use Microsoft.Identity.Web which GAed last week. This takes care of the dirty details for you, brings tons of features on top of MSAL.NET and will evolve to support more. See https://github.com/AzureAD/microsoft-identity-web/wiki/1.0.0
If you really need to use MSAL.NET directly, we want to know, as this might mean that we are missing features you need, and we are willing to provide them.

@bgavrilMS : as discussed, I propose that we remove GetAccountsAsync() from IConfidentialClientApplication in MSAL.NET 5.x (as this is a breaking change, but really is useless if customers follow the recommended guidance)

jmprieur on 9 Oct 2020

👍1

@jmprieur Understood and yes we implement one TokenCache per Account and yes this issue is part of our upgrade to GA of Microsoft.Identity.Web, but because we need to use the Token outside of ASP.NET Stack (Azure Functions to be specific) we must share the token and create our own ConfidentialClientApplication when we call the Graph there. All the AcquireToken* Methods required passing in IAccount which we need to get somehow.
I know by looking at the source code that the ID generated by MSAL is userId.tenantId which I could use, but that is strictly speaking internal knowledge to MSAL, may change in future and SuggestedWebCacheKeyFactory is not public, so the question really is: What is the correct way of getting the (only) IAccount from a TokenCache which we have loaded for a specific Account (note: not MSAL IAccount but our Apps Account, which are different entities)? In previous versions of MSAL and the samples it was always get the first account from GetAccounts().

And to your the last question: Yes we use the DistributedTokenCacheAdapter from Microsoft.Identity.Web.

aduggleby on 9 Oct 2020

@aduggleby : I recommend you use HttpContext.User.GetMaslAccountId() to get the account ID from the claims principal. (this is an extension method)

Note, however that this will work for a web API; but if you building a Web API, the cache key is a hash of the incoming token, so I don't think that this would retrieve the account this way. @bgavrilMS : am I right?

Yes, previous MSAL samples for confidential client applications were using GetAccounts(), but it was also returning empty collections when the cache got one cache per account, I'd think (which is an oversight on our side, and clearly shows that the MSAL cache [for all the MSAL libraries] was designed with public client applications in mind. Did you see any other behavior?

jmprieur on 9 Oct 2020

@jmprieur I saw that extension method, but we are running Graph Queries in Azure Functions with no access to the ClaimsPrincipal so that won't work there directly. In the Functions we simply have the job referencing the account and need load the TokenCache for that account and that's where we get stuck. Would the correct path be to just store the MsalAccountId on the Account record as a separate property and then load the IAccount that way?

An additional challenge will be that were need to migrate the existing MSALv2 TokenCaches over. Those are all referenced simply by our AccountID, so won't have the MsalAccountId on them when we migrate, but I guess I can generate the userId.tenantId format manually for now.

While Microsoft.Identity.Web definitely makes the initial user flow scenario very straight forward compare to MSALv2, as soon as you're calling Graph async somewhere the complexity creeps back in. Would love to see that addressed in future versions of Identity.

Regarding GetAccounts on previous versions: We have been running the app with MSAL v2 and GetAccounts does indeed return the Account there. It's always either 1 account returned (or none if the token is expired).

aduggleby on 9 Oct 2020

Thanks @aduggleby for this feedback
yes, I guess storing the account ID would work.

As soon as you're calling Graph async somewhere the complexity creeps back in.

Can you please elaborate? Do you mean for web hooks? (I assume you're familiar with AddMicrosoftGraph() and the injection of the GraphServiceClient in the controller/pages?

Regarding GetAccounts on previous versions: We have been running the app with MSAL v2 and GetAccounts does indeed return the Account there. It's always either 1 account returned (or none if the token is expired).

Do you feel that this would be the right behavior?

jmprieur on 9 Oct 2020

As soon as you're calling Graph async somewhere the complexity creeps back in.

Can you please elaborate? Do you mean for web hooks? (I assume you're familiar with AddMicrosoftGraph() and the injection of the GraphServiceClient in the controller/pages?

Sorry vy async here I meant: I have the front facing Web App where the user signs in and then we send Jobs via ServiceBus to be handled by Azure Functions asynchronously. All the Microsoft.Web.Identity helpers don't help us in Azure Functions (AFAIK) so we're left creating our own GraphClients and get the tokens via ConfidentialClientApplications.

On the webapp side this is all neatly abstracted away via the GraphClient injections as you mentioned.

Regarding GetAccounts on previous versions: We have been running the app with MSAL v2 and GetAccounts does indeed return the Account there. It's always either 1 account returned (or none if the token is expired).

Do you feel that this would be the right behavior?

If the TokenCache is only ever meant to have one account I would expect neither GetAccounts() nor GetAccount(id) really but simply GetAccount() (so not needing the id).

aduggleby on 9 Oct 2020

Thanks @aduggleby

Do I understand that we'd want to have a Microsoft.Identity.AzureFunction library ?
the suggestion of GetAccount() in confidential client applications is actually a great one. cc: @jennyf19 @bgavrilMS @henrik-me @pmaytak

jmprieur on 9 Oct 2020