From 8999d2a6ce75b93fc8bee0d53089ca48e3773824 Mon Sep 17 00:00:00 2001 From: sruke <73967733+sruke@users.noreply.github.com> Date: Tue, 5 Sep 2023 09:27:44 -0700 Subject: [PATCH 1/6] Introduces a lock while loading credentials from Credential Source --- .../DefaultCredentialsLoader.cs | 42 ++++++++++++++++++- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.Identity.Web.Certificate/DefaultCredentialsLoader.cs b/src/Microsoft.Identity.Web.Certificate/DefaultCredentialsLoader.cs index e94b14ec4..9288948bc 100644 --- a/src/Microsoft.Identity.Web.Certificate/DefaultCredentialsLoader.cs +++ b/src/Microsoft.Identity.Web.Certificate/DefaultCredentialsLoader.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.Logging; using Microsoft.Identity.Abstractions; @@ -15,6 +16,9 @@ namespace Microsoft.Identity.Web public class DefaultCredentialsLoader : ICredentialsLoader { ILogger? _logger; + + private readonly Dictionary _loadingSemaphores = new Dictionary(); + private readonly object _semaphoreLock = new object(); /// /// Constructor with a logger @@ -56,13 +60,47 @@ public async Task LoadCredentialsIfNeededAsync(CredentialDescription credentialD if (credentialDescription.CachedValue == null) { - if (CredentialSourceLoaders.TryGetValue(credentialDescription.SourceType, out ICredentialSourceLoader? loader)) + // Generate a unique key for the credentialDescription + string key = GenerateKey(credentialDescription); + + // Get or create a semaphore for this key + SemaphoreSlim semaphore; + lock (_semaphoreLock) + { + if (!_loadingSemaphores.TryGetValue(key, out semaphore)) + { + semaphore = new SemaphoreSlim(1); + _loadingSemaphores[key] = semaphore; + } + } + + // Wait to acquire the semaphore + await semaphore.WaitAsync(); + + try { - await loader.LoadIfNeededAsync(credentialDescription, parameters); + if (credentialDescription.CachedValue == null) + { + if (CredentialSourceLoaders.TryGetValue(credentialDescription.SourceType, out ICredentialSourceLoader? loader)) + { + await loader.LoadIfNeededAsync(credentialDescription, parameters); + } + } + } + finally + { + // Release the semaphore + semaphore.Release(); } } } + private string GenerateKey(CredentialDescription credentialDescription) + { + // Generate a unique key for the credentialDescription + return $"{credentialDescription.SourceType}_{credentialDescription.ReferenceOrValue}"; + } + /// public async Task LoadFirstValidCredentialsAsync(IEnumerable credentialDescriptions, CredentialSourceLoaderParameters? parameters = null) { From 8d0294265f23938196f8e0094b4c3c2958de25de Mon Sep 17 00:00:00 2001 From: sruke <73967733+sruke@users.noreply.github.com> Date: Tue, 5 Sep 2023 12:55:21 -0700 Subject: [PATCH 2/6] Make type nullable --- .../DefaultCredentialsLoader.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.Identity.Web.Certificate/DefaultCredentialsLoader.cs b/src/Microsoft.Identity.Web.Certificate/DefaultCredentialsLoader.cs index 9288948bc..8bad7b7c7 100644 --- a/src/Microsoft.Identity.Web.Certificate/DefaultCredentialsLoader.cs +++ b/src/Microsoft.Identity.Web.Certificate/DefaultCredentialsLoader.cs @@ -64,7 +64,7 @@ public async Task LoadCredentialsIfNeededAsync(CredentialDescription credentialD string key = GenerateKey(credentialDescription); // Get or create a semaphore for this key - SemaphoreSlim semaphore; + SemaphoreSlim? semaphore; lock (_semaphoreLock) { if (!_loadingSemaphores.TryGetValue(key, out semaphore)) From 4c878b0a921881dd42156940b66bd43e00de6f65 Mon Sep 17 00:00:00 2001 From: sruke <73967733+sruke@users.noreply.github.com> Date: Thu, 7 Sep 2023 11:31:08 -0700 Subject: [PATCH 3/6] Updates Abstractions version and CredentialLoader implementation --- Directory.Build.props | 2 +- .../DefaultCredentialsLoader.cs | 22 ++++--------------- 2 files changed, 5 insertions(+), 19 deletions(-) diff --git a/Directory.Build.props b/Directory.Build.props index 152e03d2f..d5d5e5653 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -80,7 +80,7 @@ 4.34.0 4.50.0-preview 3.1.3 - 4.0.0 + 4.1.0 4.7.2 diff --git a/src/Microsoft.Identity.Web.Certificate/DefaultCredentialsLoader.cs b/src/Microsoft.Identity.Web.Certificate/DefaultCredentialsLoader.cs index 8bad7b7c7..5edc259e5 100644 --- a/src/Microsoft.Identity.Web.Certificate/DefaultCredentialsLoader.cs +++ b/src/Microsoft.Identity.Web.Certificate/DefaultCredentialsLoader.cs @@ -2,6 +2,7 @@ // Licensed under the MIT License. using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Threading; using System.Threading.Tasks; @@ -16,9 +17,7 @@ namespace Microsoft.Identity.Web public class DefaultCredentialsLoader : ICredentialsLoader { ILogger? _logger; - - private readonly Dictionary _loadingSemaphores = new Dictionary(); - private readonly object _semaphoreLock = new object(); + private readonly ConcurrentDictionary _loadingSemaphores = new ConcurrentDictionary(); /// /// Constructor with a logger @@ -60,19 +59,8 @@ public async Task LoadCredentialsIfNeededAsync(CredentialDescription credentialD if (credentialDescription.CachedValue == null) { - // Generate a unique key for the credentialDescription - string key = GenerateKey(credentialDescription); - - // Get or create a semaphore for this key - SemaphoreSlim? semaphore; - lock (_semaphoreLock) - { - if (!_loadingSemaphores.TryGetValue(key, out semaphore)) - { - semaphore = new SemaphoreSlim(1); - _loadingSemaphores[key] = semaphore; - } - } + // Get or create a semaphore for this credentialDescription + var semaphore = _loadingSemaphores.GetOrAdd(credentialDescription.Id, new SemaphoreSlim(1)); // Wait to acquire the semaphore await semaphore.WaitAsync(); @@ -82,9 +70,7 @@ public async Task LoadCredentialsIfNeededAsync(CredentialDescription credentialD if (credentialDescription.CachedValue == null) { if (CredentialSourceLoaders.TryGetValue(credentialDescription.SourceType, out ICredentialSourceLoader? loader)) - { await loader.LoadIfNeededAsync(credentialDescription, parameters); - } } } finally From 2a7a7dad14d9201f1151aed58df099e5dbc2088f Mon Sep 17 00:00:00 2001 From: sruke <73967733+sruke@users.noreply.github.com> Date: Thu, 7 Sep 2023 13:12:43 -0700 Subject: [PATCH 4/6] Add test --- .../DefaultCredentialsLoader.cs | 7 --- .../TokenAcquirerTests/TokenAcquirer.cs | 44 +++++++++++++++++++ 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/src/Microsoft.Identity.Web.Certificate/DefaultCredentialsLoader.cs b/src/Microsoft.Identity.Web.Certificate/DefaultCredentialsLoader.cs index 5edc259e5..e1284aa56 100644 --- a/src/Microsoft.Identity.Web.Certificate/DefaultCredentialsLoader.cs +++ b/src/Microsoft.Identity.Web.Certificate/DefaultCredentialsLoader.cs @@ -1,7 +1,6 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -using System; using System.Collections.Concurrent; using System.Collections.Generic; using System.Threading; @@ -81,12 +80,6 @@ public async Task LoadCredentialsIfNeededAsync(CredentialDescription credentialD } } - private string GenerateKey(CredentialDescription credentialDescription) - { - // Generate a unique key for the credentialDescription - return $"{credentialDescription.SourceType}_{credentialDescription.ReferenceOrValue}"; - } - /// public async Task LoadFirstValidCredentialsAsync(IEnumerable credentialDescriptions, CredentialSourceLoaderParameters? parameters = null) { diff --git a/tests/IntegrationTests/TokenAcquirerTests/TokenAcquirer.cs b/tests/IntegrationTests/TokenAcquirerTests/TokenAcquirer.cs index 0b9b29995..a60299669 100644 --- a/tests/IntegrationTests/TokenAcquirerTests/TokenAcquirer.cs +++ b/tests/IntegrationTests/TokenAcquirerTests/TokenAcquirer.cs @@ -2,9 +2,12 @@ // Licensed under the MIT License. using System; +using System.Collections; +using System.Collections.Generic; using System.Linq; using System.Security.Cryptography; using System.Security.Cryptography.X509Certificates; +using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; @@ -14,6 +17,7 @@ using Microsoft.Identity.Web.TokenCacheProviders.InMemory; using Microsoft.IdentityModel.Tokens; using Xunit; +using TaskStatus = System.Threading.Tasks.TaskStatus; namespace TokenAcquirerTests { @@ -126,6 +130,46 @@ public async Task AcquireToken_WithFactoryAndAuthorityClientIdCert_ClientCredent Assert.False(string.IsNullOrEmpty(result.AccessToken)); } + [IgnoreOnAzureDevopsFact] + //[Fact] + public async Task LoadCredentialsIfNeededAsync_MultipleThreads_WaitsForSemaphore() + { + TokenAcquirerFactory tokenAcquirerFactory = TokenAcquirerFactory.GetDefaultInstance(); + IServiceCollection services = tokenAcquirerFactory.Services; + + services.Configure(s_optionName, option => + { + option.Instance = "https://login.microsoftonline.com/"; + option.TenantId = "msidentitysamplestesting.onmicrosoft.com"; + option.ClientId = "6af093f3-b445-4b7a-beae-046864468ad6"; + option.ClientCredentials = s_clientCredentials; + }); + + services.AddInMemoryTokenCaches(); + var serviceProvider = tokenAcquirerFactory.Build(); + var options = serviceProvider.GetRequiredService>().Get(s_optionName); + var credentialsLoader = serviceProvider.GetRequiredService(); + + var task1 = Task.Run(async () => + { + await credentialsLoader.LoadCredentialsIfNeededAsync(options.ClientCredentials!.First()); + }); + + var task2 = Task.Run(async () => + { + await credentialsLoader.LoadCredentialsIfNeededAsync(options.ClientCredentials!.First()); + }); + + // Run task1 and task2 concurrently + await Task.WhenAll(task1, task2); + + var cert = options.ClientCredentials!.First().Certificate; + + Assert.NotNull(cert); + Assert.Equal(TaskStatus.RanToCompletion, task1.Status); + Assert.Equal(TaskStatus.RanToCompletion, task2.Status); + } + [IgnoreOnAzureDevopsFact] //[Fact] public async Task AcquireTokenWithPop_ClientCredentialsAsync() From a0ec1662da4bdea87752f81e919357c53c1cd8e3 Mon Sep 17 00:00:00 2001 From: sruke <73967733+sruke@users.noreply.github.com> Date: Thu, 7 Sep 2023 13:18:37 -0700 Subject: [PATCH 5/6] Remove unnecessary usings --- tests/IntegrationTests/TokenAcquirerTests/TokenAcquirer.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/IntegrationTests/TokenAcquirerTests/TokenAcquirer.cs b/tests/IntegrationTests/TokenAcquirerTests/TokenAcquirer.cs index a60299669..5ab4c4a2a 100644 --- a/tests/IntegrationTests/TokenAcquirerTests/TokenAcquirer.cs +++ b/tests/IntegrationTests/TokenAcquirerTests/TokenAcquirer.cs @@ -2,12 +2,9 @@ // Licensed under the MIT License. using System; -using System.Collections; -using System.Collections.Generic; using System.Linq; using System.Security.Cryptography; using System.Security.Cryptography.X509Certificates; -using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; From 6c374a550a8947d3ad654feae49fc6206656657d Mon Sep 17 00:00:00 2001 From: sruke <73967733+sruke@users.noreply.github.com> Date: Thu, 7 Sep 2023 14:21:56 -0700 Subject: [PATCH 6/6] Address feedback --- .../DefaultCredentialsLoader.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.Identity.Web.Certificate/DefaultCredentialsLoader.cs b/src/Microsoft.Identity.Web.Certificate/DefaultCredentialsLoader.cs index e1284aa56..118beb256 100644 --- a/src/Microsoft.Identity.Web.Certificate/DefaultCredentialsLoader.cs +++ b/src/Microsoft.Identity.Web.Certificate/DefaultCredentialsLoader.cs @@ -59,7 +59,7 @@ public async Task LoadCredentialsIfNeededAsync(CredentialDescription credentialD if (credentialDescription.CachedValue == null) { // Get or create a semaphore for this credentialDescription - var semaphore = _loadingSemaphores.GetOrAdd(credentialDescription.Id, new SemaphoreSlim(1)); + var semaphore = _loadingSemaphores.GetOrAdd(credentialDescription.Id, (v) => new SemaphoreSlim(1)); // Wait to acquire the semaphore await semaphore.WaitAsync();