microsoft-identity-web
microsoft-identity-web copied to clipboard
AzureAD
[Bug] Prevent app developers from using DistributedMemoryCache in production applications
Which version of Microsoft Identity Web are you using? 1.22.0
Expected behavior Apps should configure either L1 cache or L1+L2 cache where L2 is not DistributedMemoryCache.
Actual behavior Our documentation is not clear enough and many apps use L1 + L2 caching where L2 is DistributedMemoryCache.
The problem with this approach is that both L1 (MemoryCache) and L2 (DistributedMemoryCache) end up using MemoryCache object internally to store data. So this results in having 2 MemoryCache objects with the same data.
As per https://docs.microsoft.com/en-us/aspnet/core/performance/caching/distributed?view=aspnetcore-6.0#distributed-memory-cache apps should not use DistributedMemoryCache in production. It's just a simple default.
Possible solution
Log errors when L2 cache is set to DistributedMemoryCache
Mitigated by updated doc PR (from Bogdan): https://github.com/MicrosoftDocs/azure-docs/pull/87209
Would be better done by a static analyzer. We are not going to log an error, we would not have a clean way to detect the implementation as Microsoft.Identity.Web does not depend on the implementations, only on the IDistributedCache interface. Only apps depend on the implementations.
The whole point of MemoryDistributedCache is that it ships in the box. It's in the same assembly and namespace as IDistributedCache.
Today, the library does the following:
services.AddDistributedMemoryCache();
services.AddSingleton<IMsalTokenCacheProvider, MsalDistributedTokenCacheAdapter>();
So we're injecting the default, non-production friendly IDistributedCache alongside the L1 memory cache. We treat "bad" defaults as bugs generally, as they can cause production issues in the long run. If we didn't have to care about backwards compat, I'd modify this as:
services.AddSingleton<IMsalTokenCacheProvider, MsalDistributedTokenCacheAdapter>();
// and later
if (distributedCache is null)
throw new Nice_Exception();
Just remove the services.AddDistributedMemoryCache() then, if that;s the issue., That is the bug
Wouldn't that be a breaking change @jmprieur ? I agree it's the right thing to do.
