From ce61735cb80e7015bf6c63d71dc10f306f1cf586 Mon Sep 17 00:00:00 2001 From: Keegan Caruso Date: Wed, 15 Jan 2025 17:52:31 -0800 Subject: [PATCH 1/2] Remove explicit locking in OpenIdConnectCachingSecurityTokenProvider Fixes #3078 See #3124 and #3118 for more context. --- .../InternalAPI.Unshipped.txt | 1 + ...enIdConnectCachingSecurityTokenProvider.cs | 45 ++++--------------- 2 files changed, 9 insertions(+), 37 deletions(-) diff --git a/src/Microsoft.Identity.Web.OWIN/InternalAPI.Unshipped.txt b/src/Microsoft.Identity.Web.OWIN/InternalAPI.Unshipped.txt index e69de29bb..8dcbe6a18 100644 --- a/src/Microsoft.Identity.Web.OWIN/InternalAPI.Unshipped.txt +++ b/src/Microsoft.Identity.Web.OWIN/InternalAPI.Unshipped.txt @@ -0,0 +1 @@ +readonly Microsoft.Identity.Web.OpenIdConnectCachingSecurityTokenProvider._configManager -> Microsoft.IdentityModel.Protocols.ConfigurationManager! \ No newline at end of file diff --git a/src/Microsoft.Identity.Web.OWIN/OpenIdConnectCachingSecurityTokenProvider.cs b/src/Microsoft.Identity.Web.OWIN/OpenIdConnectCachingSecurityTokenProvider.cs index 10950971b..086c662ed 100644 --- a/src/Microsoft.Identity.Web.OWIN/OpenIdConnectCachingSecurityTokenProvider.cs +++ b/src/Microsoft.Identity.Web.OWIN/OpenIdConnectCachingSecurityTokenProvider.cs @@ -1,13 +1,11 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +using System.Collections.Generic; using Microsoft.IdentityModel.Protocols; using Microsoft.IdentityModel.Protocols.OpenIdConnect; using Microsoft.IdentityModel.Tokens; using Microsoft.Owin.Security.Jwt; -using System.Collections.Generic; -using System.Threading; -using System.Threading.Tasks; namespace Microsoft.Identity.Web { @@ -15,12 +13,9 @@ namespace Microsoft.Identity.Web // the OpenID Connect metadata endpoint exposed by the STS by default. internal class OpenIdConnectCachingSecurityTokenProvider : IIssuerSecurityKeyProvider { - public ConfigurationManager _configManager; - private string? _issuer; - private IEnumerable? _keys; + public readonly ConfigurationManager _configManager; private readonly string _metadataEndpoint; - - private readonly ReaderWriterLockSlim _synclock = new ReaderWriterLockSlim(); + OpenIdConnectConfiguration? _oidcConfig; public OpenIdConnectCachingSecurityTokenProvider(string metadataEndpoint) { @@ -41,15 +36,7 @@ public string? Issuer get { RetrieveMetadata(); - _synclock.EnterReadLock(); - try - { - return _issuer; - } - finally - { - _synclock.ExitReadLock(); - } + return _oidcConfig!.Issuer; } } @@ -64,33 +51,17 @@ public IEnumerable? SecurityKeys get { RetrieveMetadata(); - _synclock.EnterReadLock(); - try - { - return _keys; - } - finally - { - _synclock.ExitReadLock(); - } + return _oidcConfig!.SigningKeys; } } private void RetrieveMetadata() { - _synclock.EnterWriteLock(); - try - { + // ConfigurationManager will return the same cached config unless enough time has passed, + // then the return value will be a new object. #pragma warning disable VSTHRD002 // Avoid problematic synchronous waits - OpenIdConnectConfiguration config = Task.Run(_configManager.GetConfigurationAsync).Result; + _oidcConfig = _configManager.GetConfigurationAsync().Result; #pragma warning restore VSTHRD002 // Avoid problematic synchronous waits - _issuer = config.Issuer; - _keys = config.SigningKeys; - } - finally - { - _synclock.ExitWriteLock(); - } } } } From e69d0a2a50a3cc065282d2c4f260aec5187478e1 Mon Sep 17 00:00:00 2001 From: Keegan Caruso Date: Thu, 16 Jan 2025 09:57:15 -0800 Subject: [PATCH 2/2] Simplify OpenIdConnectCachingSecurityTokenProvider --- ...enIdConnectCachingSecurityTokenProvider.cs | 25 +++---------------- 1 file changed, 4 insertions(+), 21 deletions(-) diff --git a/src/Microsoft.Identity.Web.OWIN/OpenIdConnectCachingSecurityTokenProvider.cs b/src/Microsoft.Identity.Web.OWIN/OpenIdConnectCachingSecurityTokenProvider.cs index 086c662ed..107a0d45c 100644 --- a/src/Microsoft.Identity.Web.OWIN/OpenIdConnectCachingSecurityTokenProvider.cs +++ b/src/Microsoft.Identity.Web.OWIN/OpenIdConnectCachingSecurityTokenProvider.cs @@ -14,12 +14,9 @@ namespace Microsoft.Identity.Web internal class OpenIdConnectCachingSecurityTokenProvider : IIssuerSecurityKeyProvider { public readonly ConfigurationManager _configManager; - private readonly string _metadataEndpoint; - OpenIdConnectConfiguration? _oidcConfig; public OpenIdConnectCachingSecurityTokenProvider(string metadataEndpoint) { - _metadataEndpoint = metadataEndpoint; _configManager = new ConfigurationManager(metadataEndpoint, new OpenIdConnectConfigurationRetriever()); RetrieveMetadata(); @@ -31,14 +28,7 @@ public OpenIdConnectCachingSecurityTokenProvider(string metadataEndpoint) /// /// The issuer the credentials are for. /// - public string? Issuer - { - get - { - RetrieveMetadata(); - return _oidcConfig!.Issuer; - } - } + public string? Issuer => RetrieveMetadata().Issuer; /// /// Gets all known security keys. @@ -46,21 +36,14 @@ public string? Issuer /// /// All known security keys. /// - public IEnumerable? SecurityKeys - { - get - { - RetrieveMetadata(); - return _oidcConfig!.SigningKeys; - } - } + public IEnumerable? SecurityKeys => RetrieveMetadata().SigningKeys; - private void RetrieveMetadata() + private OpenIdConnectConfiguration RetrieveMetadata() { // ConfigurationManager will return the same cached config unless enough time has passed, // then the return value will be a new object. #pragma warning disable VSTHRD002 // Avoid problematic synchronous waits - _oidcConfig = _configManager.GetConfigurationAsync().Result; + return _configManager.GetConfigurationAsync().Result; #pragma warning restore VSTHRD002 // Avoid problematic synchronous waits } }