Token Cache remains after logout about active-directory-aspnetcore-webapp-openidconnect-v2 HOT 7 CLOSED

@HenningRoigaard it was actually a bug when using guest accounts. The fix got merged already.

Thanks

from active-directory-aspnetcore-webapp-openidconnect-v2.

I am aware of the Guest user issue , and this is something else. Fix issue 130 does not fix this.

This issue can be reproduced with fix 130, both for guest and regular accounts. The issue remains that UserTokenCacheAfterAccessNotification is invoked after cache is cleared, with a dirty (args.HasStateChanged) but empty cache. The code as is, persists that empty cache, and as a result, we will accumulate accountids.

I would like to reopen this issue, but I do not have the option!?

from active-directory-aspnetcore-webapp-openidconnect-v2.

@HenningRoigaard I used this sample, https://github.com/Azure-Samples/active-directory-aspnetcore-webapp-openidconnect-v2/tree/master/2-WebApp-graph-user/2-2-TokenCache, to test and after logging out, I had an empty table on the DB.

So, could you please tell me how did you get this empty cache persisted?

from active-directory-aspnetcore-webapp-openidconnect-v2.

I have tested using 2-2, and you are correct: there is no entry in the database after logout. However, this is due to some lucky but most likely unintentional programming :)
The flow from the point of view of MSALPerUserSqlTokenCacheProvider when signing out is:

1. Create instance of MSALPerUserSqlTokenCacheProvider
2. Invoke UserTokenCacheBeforeAccessNotification, which invokes ReadCacheForSignedInUser, and thus initializes InMemoryCache with my credentials
3. Invoke Clear, which deletes from database but leaves InMemoryCache initialized with my credentials
4. Invoke UserTokenCacheAfterAccessNotification with empty cache (as described previously).

This is where the luck part comes in: Because the same instance is used for both Clear and subsequent UserTokenCacheAfterAccessNotification (it’s the same “scope”), the InMemoryCache remains initialized, and we then try to persist the empty token as an update. As it does not exist, this result in and DbUpdateConcurrencyException, which we catch and ignore.

Clearing InMemoryCache as part of Clear will not solve the problem. It will simply result in us persisting an empty cache.

from active-directory-aspnetcore-webapp-openidconnect-v2.

After investigating it further more with the engineering team, we could reproduce what you pointed out but this behavior is actually by design. So, when you clear the cache, MSAL doesn't exclude the entry but saves an empty JSON of the cache entry.
The bytes that you are seeing when inspecting the memory cache is this JSON:

"{\"AccessToken\":{},\"RefreshToken\":{},\"IdToken\":{},\"Account\":{},\"AppMetadata\":{\"appmetadata-login.windows.net-{GUID}\":{\"environment\":\"login.windows.net\",\"client_id\":\"{GUID}\"}}}"

So, the access token, refresh token and id token are being cleared properly and there is no "random garbage" being saved. It is just the empty JSON of the token entry class.

I hope it answers your question.
Thanks for pointing that out though.

from active-directory-aspnetcore-webapp-openidconnect-v2.

@TiagoBrenck I can accept that MSAL callbacks with an empty cache by design.

However, I still believe it would be a good idea to do cleanup of empty caches, e.g., by not persisting them in the first place. From a storage perspective, it probably doesn't matter. However, I fear that accumulation might result in some GDPR compliance issue, and more generally, I would like to do it simply because of best practice housekeeping/cleanup.

If TokenAcquisition.RemoveAccount remains inchanged, it will be the responsibility of a custom cacheprovider to identify and avoid persisting empty caches if so desired.

1. What is a good criteria for identifying an empty cache in UserTokenCacheAfterAccessNotification? The ITokenCache s not null, and it doesn't have an "IsEmpty" property or anything like it...

Alternatively, it could be handled centrally in TokenAcquisition.RemoveAccount, by changing the order of Clear and RemoveAsync, so that we can freely persist the empty token only to delete it in Clear.

2. Would that have any unwanted sideeffects?

from active-directory-aspnetcore-webapp-openidconnect-v2.

@TiagoBrenck, please open an issue on MSAL.NET so that the persistance callback help this scenario. We would not want to pass null for public client applications, as Mark explained in the private thread. I like the IsEmpty property on the Token cache serialization argument.

cc: @MarkZuber

from active-directory-aspnetcore-webapp-openidconnect-v2.