Skip to content
This repository has been archived by the owner on Sep 7, 2023. It is now read-only.

Fix for #98 - cache cannot be used with ADFS #130

Merged
merged 3 commits into from
Feb 17, 2021
Merged

Fix for #98 - cache cannot be used with ADFS #130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fd0e2c9
Fix for #98 - cache extension does not work with ADFS
bgavrilMS Feb 11, 2021
50a2e94
more tests
bgavrilMS Feb 11, 2021
a5cce03
Comments
bgavrilMS Feb 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions sample/ManualTestApp/ExampleUsage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,7 @@ private static StorageCreationProperties ConfigureSecureStorage(bool usePlaintex
{
return new StorageCreationPropertiesBuilder(
Config.CacheFileName,
Config.CacheDir,
Config.ClientId)
Config.CacheDir)
.WithLinuxKeyring(
Config.LinuxKeyRingSchema,
Config.LinuxKeyRingCollection,
Expand All @@ -96,8 +95,7 @@ private static StorageCreationProperties ConfigureSecureStorage(bool usePlaintex

return new StorageCreationPropertiesBuilder(
Config.CacheFileName,
Config.CacheDir,
Config.ClientId)
Config.CacheDir)
.WithLinuxUnprotectedFile()
.WithMacKeyChain(
Config.KeyChainServiceName,
Expand Down
4 changes: 2 additions & 2 deletions sample/ManualTestApp/ManualTestApp.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
<OutputType>Exe</OutputType>
<IsPackable>false</IsPackable>

<TargetFrameworks Condition="$([MSBuild]::IsOsPlatform('Windows'))">netcoreapp3.0;net472</TargetFrameworks>
<TargetFrameworks Condition="!$([MSBuild]::IsOsPlatform('Windows'))">netcoreapp3.0</TargetFrameworks>
<TargetFrameworks Condition="$([MSBuild]::IsOsPlatform('Windows'))">netcoreapp3.1;net472</TargetFrameworks>
<TargetFrameworks Condition="!$([MSBuild]::IsOsPlatform('Windows'))">netcoreapp3.1</TargetFrameworks>

</PropertyGroup>

Expand Down
14 changes: 7 additions & 7 deletions sample/ManualTestApp/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ static async Task Main(string[] args)
var cacheHelper = await CreateCacheHelperAsync().ConfigureAwait(false);
cacheHelper.RegisterCache(pca.UserTokenCache);

// The token cache helper provides a high level event that informs apps about added / removed accounts.
cacheHelper.CacheChanged += (s, e) =>
// Advanced scenario for when 2 or more apps share the same cache
cacheHelper.CacheChanged += (s, e) => // this event is very expensive perf wise
{
Console.BackgroundColor = ConsoleColor.DarkCyan;
Console.WriteLine($"Cache Changed, Added: {e.AccountsAdded.Count()} Removed: {e.AccountsRemoved.Count()}");
Expand Down Expand Up @@ -276,8 +276,7 @@ private static async Task<MsalCacheHelper> CreateCacheHelperAsync()
{
storageProperties = new StorageCreationPropertiesBuilder(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Config.CacheFileName,
Config.CacheDir,
Config.ClientId)
Config.CacheDir)
.WithLinuxKeyring(
Config.LinuxKeyRingSchema,
Config.LinuxKeyRingCollection,
Expand All @@ -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!
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it mean that you won't have changes in the FOCI ?

Config.ClientId,
Config.Authority)
.Build();

var cacheHelper = await MsalCacheHelper.CreateAsync(
Expand All @@ -305,8 +307,7 @@ private static async Task<MsalCacheHelper> CreateCacheHelperAsync()
storageProperties =
new StorageCreationPropertiesBuilder(
Config.CacheFileName,
Config.CacheDir,
Config.ClientId)
Config.CacheDir)
.WithLinuxUnprotectedFile()
.WithMacKeyChain(
Config.KeyChainServiceName,
Expand All @@ -318,7 +319,6 @@ private static async Task<MsalCacheHelper> CreateCacheHelperAsync()

return cacheHelper;
}

}

private static IPublicClientApplication CreatePublicClient(string authority)
Expand Down
104 changes: 69 additions & 35 deletions src/Microsoft.Identity.Client.Extensions.Msal/MsalCacheHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,15 +68,32 @@ public class MsalCacheHelper
/// <summary>
/// Watches a filesystem location in order to fire events when the cache on disk is changed. Internal for testing.
/// </summary>
internal readonly FileSystemWatcher _cacheWatcher;
private readonly FileSystemWatcher _cacheWatcher;

private EventHandler<CacheChangedEventArgs> _cacheChangedEventHandler;
/// <summary>
/// Allows clients to listen for cache updates originating from disk.
/// </summary>
/// <remarks>
/// This event does not fire when the application is built against Mone framework (e.g. Xamarin.Mac), but it does fire on .Net Core on all 3 operating systems.
/// </remarks>
public event EventHandler<CacheChangedEventArgs> CacheChanged;
public event EventHandler<CacheChangedEventArgs> CacheChanged
{
add
{
if (!_storageCreationProperties.IsCacheEventConfigured)
{
throw new InvalidOperationException(
"To use this event, please configure the clientId and the authority " +
"using StorageCreationPropertiesBuilder.WithCacheChangedEvent");
}
_cacheChangedEventHandler += value;
}
remove
{
_cacheChangedEventHandler -= value;
}
}

/// <summary>
/// Contains a reference to all caches currently registered to synchronize with this MsalCacheHelper, along with
Expand All @@ -93,9 +110,14 @@ public class MsalCacheHelper
TraceSourceLogger logger)
{
var accountIdentifiers = new HashSet<string>();
if (File.Exists(storageCreationProperties.CacheFilePath))

if (storageCreationProperties.IsCacheEventConfigured &&
File.Exists(storageCreationProperties.CacheFilePath))
{
var pca = PublicClientApplicationBuilder.Create(storageCreationProperties.ClientId).Build();
var pca = PublicClientApplicationBuilder
.Create(storageCreationProperties.ClientId)
.WithAuthority(storageCreationProperties.Authority)
.Build();

pca.UserTokenCache.SetBeforeAccess((args) =>
{
Expand Down Expand Up @@ -136,7 +158,7 @@ public class MsalCacheHelper
private MsalCacheHelper(
StorageCreationProperties storageCreationProperties,
TraceSource logger,
HashSet<string> knownAccountIds,
HashSet<string> knownAccountIds, // only used for CacheChangedEvent
FileSystemWatcher cacheWatcher)
{
_logger = logger == null ? s_staticLogger.Value : new TraceSourceLogger(logger);
Expand All @@ -145,44 +167,46 @@ private MsalCacheHelper(
_knownAccountIds = knownAccountIds;

_cacheWatcher = cacheWatcher;
_cacheWatcher.Changed += OnCacheFileChangedAsync;
_cacheWatcher.Deleted += OnCacheFileChangedAsync;
if (_cacheWatcher != null)
{
_cacheWatcher.Changed += OnCacheFileChangedAsync;
_cacheWatcher.Deleted += OnCacheFileChangedAsync;
}
}

private async void OnCacheFileChangedAsync(object sender, FileSystemEventArgs args)
{
// avoid the high cost of computing the added / removed accounts if nobody listens to this
if (CacheChanged == null)
if (_cacheChangedEventHandler?.GetInvocationList() != null &&
_cacheChangedEventHandler?.GetInvocationList().Length > 0)
{
return;
}
try
{
IEnumerable<string> added;
IEnumerable<string> removed;

try
{
IEnumerable<string> added;
IEnumerable<string> removed;
using (CreateCrossPlatLock(_storageCreationProperties))
{
var currentAccountIds = await GetAccountIdentifiersNoLockAsync(_storageCreationProperties, _logger).ConfigureAwait(false);

using (CreateCrossPlatLock(_storageCreationProperties))
{
var currentAccountIds = await GetAccountIdentifiersNoLockAsync(_storageCreationProperties, _logger).ConfigureAwait(false);
var intersect = currentAccountIds.Intersect(_knownAccountIds);
removed = _knownAccountIds.Except(intersect);
added = currentAccountIds.Except(intersect);

var intersect = currentAccountIds.Intersect(_knownAccountIds);
removed = _knownAccountIds.Except(intersect);
added = currentAccountIds.Except(intersect);
_knownAccountIds = currentAccountIds;
}

_knownAccountIds = currentAccountIds;
if (added.Any() || removed.Any())
{
_cacheChangedEventHandler.Invoke(sender, new CacheChangedEventArgs(added, removed));
}
}

if (added.Any() || removed.Any())
catch (Exception e)
{
CacheChanged.Invoke(sender, new CacheChangedEventArgs(added, removed));
// Never let this throw, just log errors
_logger.LogWarning($"Exception within File Watcher : {e}");
}
}
catch (Exception e)
{
// Never let this throw, just log errors
_logger.LogWarning($"Exception within File Watcher : {e}");
}
}

/// <summary>
Expand All @@ -206,7 +230,7 @@ internal MsalCacheHelper(ITokenCache userTokenCache, MsalCacheStorage store, Tra
/// Creates a new instance of <see cref="MsalCacheHelper"/>. To configure MSAL to use this cache persistence, call <see cref="RegisterCache(ITokenCache)"/>
/// </summary>
/// <param name="storageCreationProperties">Properties to use when creating storage on disk.</param>
/// <param name="logger">Passing null uses a default logger</param>
/// <param name="logger">Passing null uses the default TraceSource logger. See https://github.com/AzureAD/microsoft-authentication-extensions-for-dotnet/wiki/Logging for details.</param>
/// <returns>A new instance of <see cref="MsalCacheHelper"/>.</returns>
public static async Task<MsalCacheHelper> CreateAsync(StorageCreationProperties storageCreationProperties, TraceSource logger = null)
{
Expand All @@ -219,17 +243,27 @@ public static async Task<MsalCacheHelper> CreateAsync(StorageCreationProperties
using (CreateCrossPlatLock(storageCreationProperties))
{
// Cache the list of accounts

var ts = logger == null ? s_staticLogger.Value : new TraceSourceLogger(logger);
var accountIdentifiers = await GetAccountIdentifiersNoLockAsync(storageCreationProperties, ts).ConfigureAwait(false);
var cacheWatcher = new FileSystemWatcher(storageCreationProperties.CacheDirectory, storageCreationProperties.CacheFileName);

HashSet<string> accountIdentifiers = null;
FileSystemWatcher cacheWatcher = null;

if (storageCreationProperties.IsCacheEventConfigured)
{
accountIdentifiers = await GetAccountIdentifiersNoLockAsync(storageCreationProperties, ts)
.ConfigureAwait(false);
cacheWatcher = new FileSystemWatcher(
storageCreationProperties.CacheDirectory,
storageCreationProperties.CacheFileName);
}

var helper = new MsalCacheHelper(storageCreationProperties, logger, accountIdentifiers, cacheWatcher);

try
{
if (!SharedUtilities.IsMonoPlatform())
if (!SharedUtilities.IsMonoPlatform() && storageCreationProperties.IsCacheEventConfigured)
{
cacheWatcher.EnableRaisingEvents = true;
cacheWatcher.EnableRaisingEvents = true;
}
}
catch (PlatformNotSupportedException)
Expand Down
4 changes: 3 additions & 1 deletion src/Shared/EnvUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@ namespace Microsoft.Identity.Client.Extensions.Msal
internal static class EnvUtils
{
internal const string TraceLevelEnvVarName = "IDENTITYEXTENSIONTRACELEVEL";
private const string DefaultTraceSource = "Microsoft.Identity.Client.Extensions.TraceSource";

internal static TraceSource GetNewTraceSource(string sourceName)
{
sourceName = sourceName ?? DefaultTraceSource;
#if DEBUG
var level = SourceLevels.Verbose;
#else
Expand All @@ -29,7 +31,7 @@ internal static TraceSource GetNewTraceSource(string sourceName)
level = result;
}

return new TraceSource("Microsoft.Identity.Client.Extensions.TraceSource", level);
return new TraceSource(sourceName, level);
}
}
}
32 changes: 25 additions & 7 deletions src/Shared/StorageCreationProperties.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ namespace Microsoft.Identity.Client.Extensions.Msal
namespace Microsoft.Identity.Client.Extensions.Web
#endif
{
/// <summary>
/// An immutable class containing information required to instantiate storage objects for MSAL caches in various platforms.
/// </summary>
public class StorageCreationProperties
/// <summary>
/// An immutable class containing information required to instantiate storage objects for MSAL caches in various platforms.
/// </summary>
public class StorageCreationProperties
{
/// <summary>
/// This constructor is intentionally internal. To get one of these objects use <see cref="StorageCreationPropertiesBuilder.Build"/>.
Expand All @@ -33,9 +33,10 @@ internal StorageCreationProperties(
string keyringSecretLabel,
KeyValuePair<string, string> keyringAttribute1,
KeyValuePair<string, string> keyringAttribute2,
string clientId,
int lockRetryDelay,
int lockRetryCount)
int lockRetryCount,
string clientId,
string authority)
{
CacheFileName = cacheFileName;
CacheDirectory = cacheDirectory;
Expand All @@ -53,6 +54,7 @@ internal StorageCreationProperties(
KeyringAttribute2 = keyringAttribute2;

ClientId = clientId;
Authority = authority;
LockRetryDelay = lockRetryDelay;
LockRetryCount = lockRetryCount;
}
Expand Down Expand Up @@ -123,8 +125,24 @@ internal StorageCreationProperties(
public readonly int LockRetryCount;

/// <summary>
/// The client id
/// The client id.
/// </summary>
/// <remarks> Only required for the MsalCacheHelper.CacheChanged event</remarks>
public string ClientId { get; }

/// <summary>
/// The authority
/// </summary>
/// <remarks> Only required for the MsalCacheHelper.CacheChanged event</remarks>
public string Authority { get; }

internal bool IsCacheEventConfigured
{
get
{
return !string.IsNullOrEmpty(ClientId) &&
!string.IsNullOrEmpty(Authority);
}
}
}
}
Loading