Fix for #98 - cache cannot be used with ADFS #130
Conversation
| @@ -276,8 +276,7 @@ private static async Task<MsalCacheHelper> CreateCacheHelperAsync() | |||
| { | |||
| storageProperties = new StorageCreationPropertiesBuilder( | |||
There was a problem hiding this comment.
Example usage is here - you can see that Client_ID is no longer a mandatory param. If people need CacheChanged event, they need to configure both "client_id" and "authority". If they don't, they'll get a good error message when subscribing to CacheChanged
| @@ -287,6 +286,9 @@ private static async Task<MsalCacheHelper> CreateCacheHelperAsync() | |||
| .WithMacKeyChain( | |||
| Config.KeyChainServiceName, | |||
| Config.KeyChainAccountName) | |||
| .WithCacheChangedEvent( // do NOT use unless really necessary, high perf penalty! | |||
There was a problem hiding this comment.
CacheChanged event is scoped to this (client_id, authority) pair. There isn't (currently) a way to get events about other pairs, except by creating different instances of the MsalCacheHelper object.
There was a problem hiding this comment.
Does it mean that you won't have changes in the FOCI ?
bgavrilMS
changed the title
| @@ -37,11 +38,25 @@ public class StorageCreationPropertiesBuilder | |||
| /// <param name="cacheFileName">The name of the cache file to use when creating or opening storage.</param> | |||
| /// <param name="cacheDirectory">The name of the directory containing the cache file.</param> | |||
| /// <param name="clientId">The client id for the calling application</param> | |||
| [Obsolete("Use StorageCreationPropertiesBuilder(string, string) instead. " + | |||
| "If you need to consume the CacheChanged event then also use WithCacheChangedEvent(string, string)", false)] | |||
| public StorageCreationPropertiesBuilder(string cacheFileName, string cacheDirectory, string clientId) | |||
There was a problem hiding this comment.
This constructor is now obsolete.
|
Also adding @jmprieur for the public API changes. |
| @@ -287,6 +286,9 @@ private static async Task<MsalCacheHelper> CreateCacheHelperAsync() | |||
| .WithMacKeyChain( | |||
| Config.KeyChainServiceName, | |||
| Config.KeyChainAccountName) | |||
| .WithCacheChangedEvent( // do NOT use unless really necessary, high perf penalty! | |||
There was a problem hiding this comment.
Does it mean that you won't have changes in the FOCI ?
|
@jmprieur - for FOCI apps, the client_id is not used in the internal logic of Related to this being a shared cache. Think of this object as a singleton, as it represents a file on disk. I can then register a bunch of StoregePropertiesBuilder().WithCacheChangedEvent(client_id, authority);and later subscribe to the event MsalCacheHelper helper = MsalCacheHelper.CreateAsync(storageProperties);
helper.CacheChangedEvent += <listner>The You cannot currently listen to events for different pairs of (client_id, authority). VS doesn't care about it and nobody else expressed an interest in this event. As such, I think this is the simplest solution. A better solution would be to have an API like helper.RegisterCacheChangedCallback(client_id, authority, Action<accounts_added, accounts_removed>)This is doable, but more work. Do you think it's worth doing now? |
|
Thanks for the explanations, @bgavrilMS |
tests/Microsoft.Identity.Client.Extensions.Msal.UnitTests/MsalCacheHelperTests.cs
Show resolved
Hide resolved
Before this fix, developers had to configure client_id.
This design does not take into account the following, which this PR fixes:
CacheChangedevent is only relevant for the given client_idl.m.o/common). This is the root cause of bug persistent cache resiliency #89Fixes #98 #123 #121 #102 (yay!)