TokenCache.Count triggers cache notifications [was AcquireTokenByAuthorizationCodeAsync looks up cache]

[savbace]

Version: Microsoft.Identity.Client.1.1.2-preview0008
Platform: .NET

Calling ConfidentialClientApplication.AcquireTokenByAuthorizationCodeAsync results into execution of beforeAccess notification in userCache implementation.

this.Cache = new TokenCache();
this.Cache.SetAfterAccess(this.AfterAccessNotification);
this.Cache.SetBeforeAccess(this.BeforeAccessNotification);

// ...

// executed when calling AcquireTokenByAuthorizationCodeAsync
private void BeforeAccessNotification(TokenCacheNotificationArgs args)
{
    //...
}    

From docs:

This method does not lookup token cache, but stores the result in it

Is it expected behavior or am I misunderstanding something?

Thanks!

[luismanez]

Hi @jmprieur
Any update here? having similar issue with ADAL 4.3.0 in a asp.net core webapp using the DistributedTokenCache sample from here: https://docs.microsoft.com/en-us/azure/architecture/multitenant-identity/token-cache

When calling AcquireTokenByAuthorizationCodeAsync it goes to an infinite loop calling the AfterAccessNotification again and again.

When debugging, in the AfterAccessNotification method, when doing:

if (Count > 0)

I´m getting a Timeout

As far as I see in TokenCache class, the count method is also calling OnBeforeAccess and OnAfterAccess (this is not the case in 3.19.8 version)

 public int Count
    {
      get
      {
        lock (this.cacheLock)
        {
          TokenCacheNotificationArgs args = new TokenCacheNotificationArgs()
          {
            TokenCache = this
          };
          this.OnBeforeAccess(args);
          int count = this.tokenCacheDictionary.Count;
          this.OnAfterAccess(args);
          return count;
        }
      }
    }

so in the DistributedTokenCache.AfterAccessNotification (see all code here: https://github.com/mspnp/multitenant-saas-guidance/blob/master/src/Tailspin.Surveys.TokenStorage/DistributedTokenCache.cs), we are using the Count property, and this is causing the infinite loop.

All was working fine with version 3.19.8, and works fine again when downgrading to that version. In 3.19.8 TokenCache version, is not launching the Before/After methods.

Any help would be much appreciate it.

Many thanks!

[jmprieur]

Thanks for the detailed repros, @luismanez
@henrik-me @SomkaPe : can we please have a look?

[henrik-me]

@savbace @jmprieur : We will take a look. Assigning to @bgavrilMS

[luismanez]

hey guys, is there any update here? this is kind of a stopper for us moving to latest version (also thinking to move to msal). Thanks!
/cc @bgavrilMS @jmprieur

[bgavrilMS]

Apologies for the delay, will look at this today.

[bgavrilMS]

@savbace - the docs says "This method does not lookup token cache, but stores the result in it". When storing the result, it means we store the access token, refresh token, id token and account in the cache, which implies accessing the cache.

What is your concern - are we accessing the cache too often?

[bgavrilMS]

@luismanez - I am having trouble getting the sample to run because it was intended for netcore 1.1 and the EF tools are different nowadays. I'm trying some more, if you have this project converted to netcore 2+, please let me know.

[luismanez]

@bgavrilMS my project is too big to send it through. I think is much easier if you try a new netcore 2+ project and just re-uses the DistributedTokenCache. Just note that the code in Line 92 AfterAccessNotification method is calling the Count property of the base TokenCache (note I'm in latest ADAL version, not MSAL), as Count property, internally is again calling AfterAccessNotification, we get the infinite loop. So, or the Count property should not call again AfterAccessNotification, or the DistributedTokenCache should be implemented in a different way, but not sure how.

[bgavrilMS]

Yes, I see the problem by looking at the code. I think Count should either not trigger AfterAccessNotification or HasStateChanged should be reset.

Would you have time for a quick call on this? I am on the same timezone as you.

[bgavrilMS]

Workaround: set this.HasStateChanged to false before calling Count - we don't actually read this flag anywhere, so this should be safe.

[bgavrilMS]

@luismanez - Here's what I'm thinking in terms of a fix, please feel free to comment:

Context:

• All TokenCache operations are single threaded, i.e. inside lock(obj) { … }
• The TokenCache can be used multiple across processes / machines not just threads

•  We never read HasStateChange, we just set it to true and instruct users to set it to false themselves. (we also set it to false in MSAL at the end of the operation). 
  

Here are some proposals for dealing with this:

1. Don’t fire events from inside events. Shouldn’t be difficult to implement since everything is single threaded. (least intrusive but a bit hacky)
2. The HasStateChanged flag makes no sense as far as I can think of. Why would developers be expected to set this to false themselves ? Is this a legacy construct from a time when TokenCache wasn’t single threaded? We can [Obsolete] it and expose a similar flag in the TokenCacheNotificationArgs instead. This way a “Count” operation would always have this flag set it false, instead of relying on a global flag. (breaking changes simplifies the API)
3. Save and restore state of HasStateChanged when doing read-only operations such as Count (like above, but with no breaking changes - feels even more hacky)

[luismanez]

Hi @bgavrilMS
thank you so much for your help. I had to leave yesterday, but happy to have a call next week if I can help, though I'm not that expert in this topic. Regarding your comments, option 2 seems more correct to me. What I can do easily is to test your workaround, so at least we know it works fine and we can update ADAL version in the meantime.
Thanks again!

[bgavrilMS]

No problem, thank you for your input. Let me know if the workaround works, we'll implement a more durable solution after we have a team discussion about the options above.

[bgavrilMS]

Added a fix for ADAL - no longer triggering the notifications when calling Count (this was the behaviour from ADALv3).

Fix will be avaiable on next release, probably ADAL 4.5.0. Otherwise you can keep the workaround, I don't think it has side effects.

[luismanez]

That's so great @bgavrilMS THANK you very much. Looking forward to have the new release. Haven't had the chance to test the workaround yet, and I think I will wait the new release. Thanks again!!