There should be an option to have TokenAcquisition be a singleton

[jennyf19]

This issue is for a: (mark with an x)

- [ ] bug report -> please search issues before submitting
- [ x] feature request
- [ ] documentation issue or request
- [ ] regression (a behavior that used to work and stopped in a new release)

Why?

Today, TokenAcquisition is a scoped by request. See https://github.com/Azure-Samples/active-directory-aspnetcore-webapp-openidconnect-v2/blob/ab84785cf757c2d3244090a90d563528ee4be616/Microsoft.Identity.Web/ServiceCollectionExtensions.cs#L38

There are scenarios, like using the Graph SDK, which require it to be a singleton. This is a request from the Graph SDK team (Darrel Miller).
Given Bogdan's analysis (below), this is safe to have a singleton.

What: proposed developer experience?

Provide an option in the configuration, which sets the TokenAcquisition service to be a singleton.

{
  "AzureAd": {
    "Instance": "https://login.microsoftonline.com/",
  
    // missing lines here ...

    "SingletonTokenAcquisition" :  false
  },

  // missing lines here ...
}

Proposed design:

1. Add a boolean SingletonTokenAcquisition in the MicrosoftIdentityOptions, by default this would be false.
2. Add a bool parameter in AddTokenAcquisition to specify if the token acquisition service should be a singleton or not (and use .AddScoped if it's not a singleton, and AddSignleton if this is a singleton)
3. Pass this parameter in methods calling AddTokenAcquisition, the value coming from the MicrosoftIdentityOptions.SingletonTokenAcquisition. Probably:
   • AddProtectedWebApiCallsWebApis
   • AddWebAppCallsWebApis,

Issue copied from Azure-Samples/active-directory-aspnetcore-webapp-openidconnect-v2#287, that will need to be updated

[bgavrilMS]

@jmprieur - I had a look at this scenario in more detail.

AquireTokenSilent does:

1. Read the cache
2. Internal logic, possibly call AAD for new tokens
3. When new tokens arrive, re-read the cache and merge the new tokens

Steps 1 and 3 are guaranteed to be consistent because we use locks. The entire AcquireTokenSilent is not locked. So the whole thing might work from a consistency perspective, even

However, please note that using the same cache has performance implications:

a. Operations 1 and 3 above are probably executed in a critical section . This can result in high contention, especially under heavy load. This isn't a bug in MSAL per se, but we'll have to retest token cache web implementations. Some improvements can be made such as using separate read and write locks.
b. Token cache has not been perf tested for holding large number of entries. We've had bugs in the past around this, especially with around ADAL fallback mechanism.
c. Although not perf related, storing all tokens in the same cache is a security issue. If we ever introduce a bug in MSAL which would allow user A to fetch a token that does not belong to user A, today we are protected by the fact user A has its own cache. With a shared cache, this would be a major security incident.

All in all, this work item requires a lot of testing.

[jmprieur]

Thanks @bgavrilMS
about b and c, we don't store many accounts in a cache. We have one cache per account (the key is an account ID)

[bgavrilMS]

Then concern a. above is also not relevant. The locks are at the Token Cache level, so if you have a token cache for each account, contention is not an issue.

[jmprieur]

Thanks for confirming @bgavrilMS

[jmprieur]

This is illustrated by the following sample:
https://github.com/AzureAD/microsoft-identity-web/tree/master/tests/WebAppCallsMicrosoftGraph

[jennyf19]

In 0.1.4-preview release