EFADALTokenCache needs documentation

[ckarras]

I'm trying to understand why EFADALTokenCache works the way it does. All references I found say that I should implement my own TokenCache but leave that implementation as an exercise to the reader, and this sample implementations makes some counterintuitive decisions.

• Why does EFADALTokenCache create a new entry in the database every time BeforeAccessNotification is called? If EFADALTokenCache.Clear is never called (because the user never clicks "Sign Out" and just lets his session expire), won't this grow the table unnecessarily?

• Why does EFADALTokenCache pick the "FirstOrDefault" entry for a user instead of the latest one? There has been a fix suggested here 2 years ago, is there any reason why it was not merged to the master branch? https://github.com/Azure-Samples/active-directory-dotnet-webapp-webapi-multitenant-openidconnect/issues/20 It also seems to me that this fix is incomplete, there are other cases in EFADALTokenCache where the "FirstOfDefault" is used without an order by LastWrite.

[kalyankrishna1]

Toekn cache documentation is provided here

• Customizing Token cache serialization

[ckarras]

@kalyankrishna1 The documentation doesn't answer the questions I mentioned in the issue. I'm talking specifically about the EFAdalTokenCache sample implementation, not a token cache in general.

[kalyankrishna1]

Please refer to an updated sample for a more updated implementation.

[ckarras]

The updated sample cache (https://github.com/Azure-Samples/active-directory-dotnet-webapi-onbehalfof/blob/master/TodoListService/DAL/DbTokenCache.cs) still seems wrong to me. All queries just take the "First" or "FirstOrDefault" value, in the order returned by the database. Unless there's something I don't understand in this code, I would think all the queries need to have a "order by LastWrite desc", otherwise the user is likely to get a token other than most recent one available for that user.

[jmprieur]

@ckarras In Web Apps and Web APIs, the token cache contains tokens for only one identity / account / user (contrary to what happens in public client application). That is given an account, and its unique home iod.tid (home object id, and home tenant id), there should be only one record for that user in the database, which contains the serialized cache for that user. Isn't it what you are observing?

[ckarras]

@jmprieur I have often seen multiple entries for the same unique user id in the table, due to concurrent logins. The same user can login from multiple devices: home PC(s), work PC(s), phone, virtual machines, multiple browsers on the same machine,... It could be argued that this a rare edge case because logins are not likely to happen concurrently, but in fact it often happens, for various reasons, for example:

• User wants to login the application with multiple browsers for some reason, and opens both sessions simultaneously
• User was logged in on multiple devices and lost its Internet connection. Once the connection is re-established, the token may be expired and may need to be refreshed. This will result in multiple clients trying to refresh the token (and thus the token cache) for the same user id.
• User logs in the application, and there are delays communicating with the authentication server (due to the server being too busy or throttling requests, or due to network congestion, etc). While waiting, user opens another session on other device(s)/browser(s), which experiences the same problem. There are now multiple requests queued to authenticate the same user, then the authentication server quickly returns a response for all the queued requests. Now, the web application has multiple threads working with ADAL/MSAL to complete the login process and update the token cache.
• Web application tries to acquire tokens for multiple underlying service (web application that calls multiple independent services), resulting in multiple concurrent updates to the cache, on the same device and browser
• User signs out on one device/browser then quickly reopens a new session, without waiting for confirmation that the sign-out was completed (for example he may want to test an updated password, or may have been given more rights and want to re-login with these new rights)
• The easiest way to reproduce the issue is to write an automated load test that will login the same account simultaneously by many threads, but there are also real user scenarios when it can happen (see previous examples - these are just some examples I could think of, there could be several others if concurrency is not handled correctly)

This line in AfterAccessNotification decides if an Insert or Update should be done in the database:

db.Entry(Cache).State = Cache.EntryId == 0 ? EntityState.Added : EntityState.Modified;

This can lead to the incorrect decision to be taken, for example:

• Try to update a row which no longer exists, because the user signed out concurrently with the new login
• Insert a row when another one for the same user has already been inserted

This kind of problem can happen because there's no transaction or distributed locking (lock needs to be distributed because web servers are typically load balanced) between the point where the row is read, in BeforeAccessNotification, and the point where it is written, in AfterAccessNotification. These two methods are called by ADAL/MSAL so various opaque things can happen between these two methods, increasing the time span where concurrency issues may happen (opaque because, from a ADAL/MSAL user's point of view, these libraries are there to abstract/hide the complexity of interacting with an authentication provider)

Now, I see that the following method in the sample is apparently intended to solve these concurrency issues:

        void BeforeWriteNotification(TokenCacheNotificationArgs args)
        {
            // if you want to ensure that no concurrent write take place, use this notification to place a lock on the entry
        }

But first, it's really not clear what should be done to implement that lock and I have not found a single example with other cache implementations that would help. And second, I don't believe locking is the right solution for a SQL database. Database locks are expensive, and a .Net-based lock such as the "lock" keyword or ReaderWriterLockSlim doesn't help when we have multiple web servers behind a load balancer.

The following strategy may work, but I don't have enough knowledge of when ADAL/MSAL decide to call the token cache methods to be 100% confident, and the documentation was not really helpful to validate it is a correct solution:

• Use a stored procedure to do an "Upsert" operation. If multiple threads try to update simultaneously, the last update wins (there's no data loss in this case because it's just the same user logging in concurrently with the same information - except maybe in the scenario where a web apps acquires multiple tokens for multiple independent services?)
• Use a stored procedure to do a "Delete only if not updated since @LastWrite"

But I'm not convinced this solution is perfect. It appears mostly correct for applications that acquire tokens for a single back end service, but not for applications that may interact with multiple services that each have their own tokens. Since the whole cache, including potentially multiple tokens, is a big opaque blob, how can we make sure concurrent updates won't remove other versions of the cache that include other tokens for other services that are still valid?

[ckarras]

@kalyankrishna1 Please reopen this issue. As I explained in my detailed reply to @jmprieur, the issue is not resolved. Thanks.

[kalyankrishna1]

@ckarras Please have a look at the added an updated token cache for Sql server

[ckarras]

@kalyankrishna1 Thanks. I'm going to test this in our project in the next few weeks and let you know what I find. One documentation issue remains however: the new example doesn't show how to do locking. I think avoiding locks is the right thing to do for a cache based on a SQL database. But for documentation completeness, it would be helpful to update the documentation with more information about BeforeWriteNotification, for other implementations where locking would be appropriate.

[kalyankrishna1]

@ckarras I chose to use RowVersion to address concurrency and in my tests (which are not super exhaustive) I was able to reproduce and handle it successfully. You are right, table locks will kill apps that serve millions of users. Rowversion helps with concurrent threads writing for the same account and which in my opinion should be sufficient.
I tried to heavily document the class. I hope it helped, but feel free to point out places where we can document better. We'll wait for your results.

[ckarras]

I've been working on integrating Azure AD B2C and MSAL in our app in the past few weeks, and now I would be ready to test a Token Cache. But it seems the sample app (or any app based on ADAL) won't work with Azure AD B2C, according to this issue: https://github.com/adrianhall/develop-mobile-apps-with-csharp-and-azure/issues/30. Is this limitation still applicable?

I would like to test your updated EFADALPerUserTokenCache and try reproducing some concurrency issues I had in the past, but it seems it won't work with my current environment. Is there another sample, that would work with Azure B2C (and based on MSAL) I could use to test this Token Cache? I think MSAL and ADAL caches are incompatible (different signatures). In fact I see there is a considerable number of samples for MSAL, I'm really not sure which one would be considered the best, most up-to-date reference, any suggestions? This one seems fairly comprehensive and recently updated https://github.com/Azure-Samples/active-directory-aspnetcore-webapp-openidconnect-v2, but is there anything more recent or more recommended?

[kalyankrishna1]

ADAL and MSAL use different cache formats , see this. I'd suggest you stick to MSAL for now. The samples under https://github.com/Azure-Samples/active-directory-aspnetcore-webapp-openidconnect-v2 nowadays have the latest bits as its the one where work is very active.

[kalyankrishna1]

Closing this due to the lapsed time and no activity