Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature Request] Provide an option to disable the L1 cache: Token without MFA in L1 Token Cache after MFA was requested #1388

Closed
tjkb opened this issue Aug 17, 2021 · 9 comments

Comments

@tjkb
Copy link

tjkb commented Aug 17, 2021

One of our customers has Conditional Access policies in place. This means that some Graph calls work, but for others we get a MicrosoftIdentityWebChallengeUserException. We handle it by letting the user login again and providing the claims from the exception.

For Token Caching we use a distributed Redis cache.

So far, so good. But we run our Application on an Azure Web App with multiple instances. We're state-less, we have ARR Afinity disabled. Since there's a L1 InMemory cache in place by Identity.Web when using distributed cache, it means that when we get a new token for the user that complies to the Conditional Access policies, on some instances of our Web App, the L1 cache still contains the old token. Depending on the Web App instance that serves a particular API request of the user, we still get an exception. It's then basically a game of chance.

Did you think about this use case when implementing the L1 cache? How should be solve this issue? Is it possible to disable the L1 cache?

@tjkb tjkb changed the title Outdated token in L1 Token Cache after MFA was triggered Token without MFA in L1 Token Cache after MFA was requested Aug 17, 2021
@jennyf19
Copy link
Collaborator

@tjkb agree on having the option to disable the L1 cache, that's something the @jmprieur and I have discussed recently. However, the behavior you see should not be happening. Do you have a link to your code?

@tjkb
Copy link
Author

tjkb commented Aug 17, 2021

No, I don't have a link to the code. But what we have is a custom Token Cache Adapter that inherits from MsalDistributedTokenCacheAdapter. There in ReadCacheBytesAsync we read from cache like this:

var cached = await base.ReadCacheBytesAsync(cacheKey);

When I check the base implementation it's clear that first the L1 InMemory cache is checked. And if our app runs on multiple instances, I think that right after satisfying a Conditional Access policy, only one instance (the one that handled the re-login of the user), and of course the L2 Redis cache, contain the newly acquired token with MFA, but the rest of the L1 InMemory caches not.

Why you think it should not happen? From my point of view it is bound to happen, as described above.

@jennyf19
Copy link
Collaborator

can you share the code in the token cache adapter?

@tjkb
Copy link
Author

tjkb commented Aug 17, 2021

here it is:

public class ProtectedTokenCacheAdapter : MsalDistributedTokenCacheAdapter
	{
		private readonly IDataProtector protector;

		public ProtectedTokenCacheAdapter(IDistributedCache memoryCache,
			IOptions<MsalDistributedTokenCacheAdapterOptions> cacheOptions,
			IDataProtectionProvider protectionProvider,
			ILogger<MsalDistributedTokenCacheAdapter> logger) : base(memoryCache, cacheOptions, logger)
		{
			protector = (protectionProvider ?? throw new ArgumentNullException(nameof(protectionProvider))).CreateProtector(typeof(ProtectedTokenCacheAdapter).FullName);
		}

		protected override async Task<byte[]> ReadCacheBytesAsync(string cacheKey)
		{
			var cached = await base.ReadCacheBytesAsync(cacheKey);

			try
			{
				// ReSharper disable once ConditionIsAlwaysTrueOrFalse
				return cached == null ? cached : protector.Unprotect(cached);
			}
			catch (CryptographicException)
			{
				// cannot unprotect this key so it should be treated as cache miss
				await base.RemoveKeyAsync(cacheKey);
			}

			return null;
		}

		protected override Task WriteCacheBytesAsync(string cacheKey, byte[] bytes)
		{
			return base.WriteCacheBytesAsync(cacheKey, protector.Protect(bytes));
		}
	}

@jennyf19
Copy link
Collaborator

@tjkb thank you. i think this is a scenario we didn't think about for the L1/L2 cache, but it makes sense. will probably add a way to disable the L1 cache.

@jennyf19 jennyf19 added the enhancement New feature or request label Aug 17, 2021
@jennyf19 jennyf19 changed the title Token without MFA in L1 Token Cache after MFA was requested [Feature Request] Provide an option to disable the L1 cache: Token without MFA in L1 Token Cache after MFA was requested Aug 17, 2021
@jennyf19 jennyf19 added the fixed label Aug 18, 2021
@tjkb
Copy link
Author

tjkb commented Aug 18, 2021

Cool, as I see it's already fixed. Any estimation when 1.16 will be available?

@jennyf19
Copy link
Collaborator

jennyf19 commented Aug 18, 2021

@tjkb Friday (8.20) or sooner

@jennyf19
Copy link
Collaborator

Included in 1.16 release

@jennyf19
Copy link
Collaborator

cc @tjkb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants