From 931a3bf0ecdcf83214461f5f7f7373028024a05a Mon Sep 17 00:00:00 2001 From: Keegan Caruso Date: Thu, 9 Jan 2025 16:57:03 -0800 Subject: [PATCH 1/3] RequestRefresh back to a signal Further reverting the change to RequestRefresh from #2780 This is still too much of a behavioral change. RequestRefresh goes back to its historical function, signalling that the next call to GetConfigurationAsync should request data. This is incorporated with the background thread change to instead do the request as a blocking operation if from a RequestRefresh Part of #3082 --- .../Configuration/ConfigurationManager.cs | 20 ++++-- .../ConfigurationManagerTests.cs | 70 ++++++------------- 2 files changed, 33 insertions(+), 57 deletions(-) diff --git a/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs b/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs index ed3d4b7e67..8e37d60ac2 100644 --- a/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs +++ b/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs @@ -35,6 +35,10 @@ public class ConfigurationManager : BaseConfigurationManager, IConfigurationM private const int ConfigurationRetrieverRunning = 1; private int _configurationRetrieverState = ConfigurationRetrieverIdle; + // If a refresh is requested, then do the refresh as a blocking operation + // not on a background thread. + bool _refreshRequested; + /// /// Instantiates a new that manages automatic and controls refreshing on configuration data. /// @@ -214,7 +218,13 @@ public virtual async Task GetConfigurationAsync(CancellationToken cancel) { if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverRunning, ConfigurationRetrieverIdle) == ConfigurationRetrieverIdle) { - _ = Task.Run(UpdateCurrentConfiguration, CancellationToken.None); + if (_refreshRequested) + { + UpdateCurrentConfiguration(); + _refreshRequested = false; + } + else + _ = Task.Run(UpdateCurrentConfiguration, CancellationToken.None); } } @@ -310,15 +320,11 @@ public override async Task GetBaseConfigurationAsync(Cancella public override void RequestRefresh() { DateTimeOffset now = DateTimeOffset.UtcNow; - if (now >= DateTimeUtil.Add(_lastRequestRefresh.UtcDateTime, RefreshInterval) || _isFirstRefreshRequest) { _isFirstRefreshRequest = false; - if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverRunning, ConfigurationRetrieverIdle) == ConfigurationRetrieverIdle) - { - UpdateCurrentConfiguration(); - _lastRequestRefresh = now; - } + _syncAfter = now; + _refreshRequested = true; } } diff --git a/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs b/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs index 2fba523106..f7bba705bc 100644 --- a/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs +++ b/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs @@ -209,48 +209,6 @@ public async Task FetchMetadataFailureTest() TestUtilities.AssertFailIfErrors(context); } - [Fact] - public async Task VerifyInterlockGuardForRequestRefresh() - { - ManualResetEvent waitEvent = new ManualResetEvent(false); - ManualResetEvent signalEvent = new ManualResetEvent(false); - InMemoryDocumentRetriever inMemoryDocumentRetriever = InMemoryDocumentRetrieverWithEvents(waitEvent, signalEvent); - - var configurationManager = new ConfigurationManager( - "AADCommonV1Json", - new OpenIdConnectConfigurationRetriever(), - inMemoryDocumentRetriever); - - // populate the configurationManager with AADCommonV1Config - TestUtilities.SetField(configurationManager, "_currentConfiguration", OpenIdConfigData.AADCommonV1Config); - - // InMemoryDocumentRetrieverWithEvents will block until waitEvent.Set() is called. - // The first RequestRefresh will not have finished before the next RequestRefresh() is called. - // The guard '_lastRequestRefresh' will not block as we set it to DateTimeOffset.MinValue. - // Interlocked guard will block. - // Configuration should be AADCommonV1Config - signalEvent.Reset(); - _ = Task.Run(() => configurationManager.RequestRefresh()); - - // InMemoryDocumentRetrieverWithEvents will signal when it is OK to change the MetadataAddress - // otherwise, it may be the case that the MetadataAddress is changed before the previous Task has finished. - signalEvent.WaitOne(); - - // AADCommonV1Json would have been passed to the the previous retriever, which is blocked on an event. - configurationManager.MetadataAddress = "AADCommonV2Json"; - TestUtilities.SetField(configurationManager, "_lastRequestRefresh", DateTimeOffset.MinValue); - _ = Task.Run(() => configurationManager.RequestRefresh()); - - // Set the event to release the lock and let the previous retriever finish. - waitEvent.Set(); - - // Configuration should be AADCommonV1Config - var configuration = await configurationManager.GetConfigurationAsync(); - Assert.True(configuration.Issuer.Equals(OpenIdConfigData.AADCommonV1Config.Issuer), - $"configuration.Issuer from configurationManager was not as expected," + - $"configuration.Issuer: '{configuration.Issuer}' != expected '{OpenIdConfigData.AADCommonV1Config.Issuer}'."); - } - [Fact] public async Task VerifyInterlockGuardForGetConfigurationAsync() { @@ -320,13 +278,15 @@ public async Task BootstrapRefreshIntervalTest() catch (Exception firstFetchMetadataFailure) { // _syncAfter should not have been changed, because the fetch failed. - var syncAfter = TestUtilities.GetField(configManager, "_syncAfter"); - if ((DateTimeOffset)syncAfter != DateTimeOffset.MinValue) + DateTimeOffset syncAfter = (DateTimeOffset)TestUtilities.GetField(configManager, "_syncAfter"); + if (syncAfter != DateTimeOffset.MinValue) context.AddDiff($"ConfigurationManager._syncAfter: '{syncAfter}' should equal '{DateTimeOffset.MinValue}'."); if (firstFetchMetadataFailure.InnerException == null) context.AddDiff($"Expected exception to contain inner exception for fetch metadata failure."); + DateTime requestTime = DateTime.UtcNow; + // Fetch metadata again during refresh interval, the exception should be same from above. try { @@ -339,9 +299,10 @@ public async Task BootstrapRefreshIntervalTest() context.AddDiff($"Expected exception to contain inner exception for fetch metadata failure."); // _syncAfter should not have been changed, because the fetch failed. - syncAfter = TestUtilities.GetField(configManager, "_syncAfter"); - if ((DateTimeOffset)syncAfter != DateTimeOffset.MinValue) - context.AddDiff($"ConfigurationManager._syncAfter: '{syncAfter}' should equal '{DateTimeOffset.MinValue}'."); + syncAfter = (DateTimeOffset)TestUtilities.GetField(configManager, "_syncAfter"); + + if (!IdentityComparer.AreDatesEqualWithEpsilon(requestTime, syncAfter.UtcDateTime, 1)) + context.AddDiff($"ConfigurationManager._syncAfter: '{syncAfter.UtcDateTime}' should equal be within 1 second of '{requestTime}'."); IdentityComparer.AreEqual(firstFetchMetadataFailure, secondFetchMetadataFailure, context); } @@ -605,7 +566,7 @@ public static TheoryData Date: Thu, 9 Jan 2025 19:22:46 -0800 Subject: [PATCH 2/3] Add time based testing to ConfigurationManagerTests through FakeTimeProvider --- build/dependenciesTest.props | 9 ++ .../Configuration/ConfigurationManager.cs | 9 +- .../ConfigurationManagerTests.cs | 96 ++++++++++++++++++- ...Model.Protocols.OpenIdConnect.Tests.csproj | 7 ++ 4 files changed, 114 insertions(+), 7 deletions(-) diff --git a/build/dependenciesTest.props b/build/dependenciesTest.props index 65daedcf38..7cc73f438b 100644 --- a/build/dependenciesTest.props +++ b/build/dependenciesTest.props @@ -19,4 +19,13 @@ 3.0.0-pre.49 + + + + 9.0.0 + + + 8.10.0 + + diff --git a/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs b/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs index 8e37d60ac2..b1cea9ebdc 100644 --- a/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs +++ b/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs @@ -35,6 +35,8 @@ public class ConfigurationManager : BaseConfigurationManager, IConfigurationM private const int ConfigurationRetrieverRunning = 1; private int _configurationRetrieverState = ConfigurationRetrieverIdle; + private readonly TimeProvider _timeProvider = TimeProvider.System; + // If a refresh is requested, then do the refresh as a blocking operation // not on a background thread. bool _refreshRequested; @@ -151,7 +153,7 @@ public async Task GetConfigurationAsync() /// If the time since the last call is less than then is not called and the current Configuration is returned. public virtual async Task GetConfigurationAsync(CancellationToken cancel) { - if (_currentConfiguration != null && _syncAfter > DateTimeOffset.UtcNow) + if (_currentConfiguration != null && _syncAfter > _timeProvider.GetUtcNow()) return _currentConfiguration; Exception fetchMetadataFailure = null; @@ -295,7 +297,7 @@ private void UpdateCurrentConfiguration() private void UpdateConfiguration(T configuration) { _currentConfiguration = configuration; - _syncAfter = DateTimeUtil.Add(DateTime.UtcNow, AutomaticRefreshInterval + + _syncAfter = DateTimeUtil.Add(_timeProvider.GetUtcNow().UtcDateTime, AutomaticRefreshInterval + TimeSpan.FromSeconds(new Random().Next((int)AutomaticRefreshInterval.TotalSeconds / 20))); } @@ -319,11 +321,12 @@ public override async Task GetBaseConfigurationAsync(Cancella /// public override void RequestRefresh() { - DateTimeOffset now = DateTimeOffset.UtcNow; + DateTimeOffset now = _timeProvider.GetUtcNow(); if (now >= DateTimeUtil.Add(_lastRequestRefresh.UtcDateTime, RefreshInterval) || _isFirstRefreshRequest) { _isFirstRefreshRequest = false; _syncAfter = now; + _lastRequestRefresh = now; _refreshRequested = true; } } diff --git a/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs b/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs index f7bba705bc..ebf33d0b68 100644 --- a/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs +++ b/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs @@ -11,6 +11,7 @@ using System.Reflection; using System.Threading; using System.Threading.Tasks; +using Microsoft.Extensions.Time.Testing; using Microsoft.IdentityModel.Protocols.Configuration; using Microsoft.IdentityModel.Protocols.OpenIdConnect.Configuration; using Microsoft.IdentityModel.TestUtils; @@ -19,9 +20,6 @@ namespace Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests { - /// - /// - /// public class ConfigurationManagerTests { /// @@ -569,7 +567,7 @@ public static TheoryData("OpenIdConnectMetadata.json", new OpenIdConnectConfigurationRetriever(), docRetriever); @@ -720,6 +718,96 @@ public void TestConfigurationComparer() TestUtilities.AssertFailIfErrors(context); } + [Fact] + public async Task RequestRefresh_RespectsRefreshInterval() + { + // This test checks that the _syncAfter field is set correctly after a refresh. + var context = new CompareContext($"{this}.RequestRefresh_RespectsRefreshInterval"); + + var timeProvider = new FakeTimeProvider(); + + var docRetriever = new FileDocumentRetriever(); + var configManager = new ConfigurationManager("OpenIdConnectMetadata.json", new OpenIdConnectConfigurationRetriever(), docRetriever); + TestUtilities.SetField(configManager, "_timeProvider", timeProvider); + + // Get the first configuration. + var configuration = await configManager.GetConfigurationAsync(CancellationToken.None); + + configManager.RequestRefresh(); + + var configAfterFirstRefresh = await configManager.GetConfigurationAsync(CancellationToken.None); + + // First RequestRefresh triggers a refresh. + if (object.ReferenceEquals(configuration, configAfterFirstRefresh)) + context.Diffs.Add("object.ReferenceEquals(configuration, configAfterFirstRefresh)"); + + configManager.RequestRefresh(); + + var configAfterSecondRefresh = await configManager.GetConfigurationAsync(CancellationToken.None); + + // Second RequestRefresh should not trigger a refresh because the refresh interval has not passed. + if (!object.ReferenceEquals(configAfterFirstRefresh, configAfterSecondRefresh)) + context.Diffs.Add("!object.ReferenceEquals(configAfterFirstRefresh, configAfterSecondRefresh)"); + + // Advance time to trigger a refresh. + timeProvider.Advance(configManager.RefreshInterval); + + configManager.RequestRefresh(); + + var configAfterThirdRefresh = await configManager.GetConfigurationAsync(CancellationToken.None); + + // Third RequestRefresh should trigger a refresh because the refresh interval has passed. + if (object.ReferenceEquals(configAfterSecondRefresh, configAfterThirdRefresh)) + context.Diffs.Add("object.ReferenceEquals(configAfterSecondRefresh, configAfterThirdRefresh)"); + + TestUtilities.AssertFailIfErrors(context); + } + + [Fact] + public async Task GetConfigurationAsync_RespectsRefreshInterval() + { + var context = new CompareContext($"{this}.GetConfigurationAsync_RespectsRefreshInterval"); + + var timeProvider = new FakeTimeProvider(); + + var docRetriever = new FileDocumentRetriever(); + var configManager = new ConfigurationManager("OpenIdConnectMetadata.json", new OpenIdConnectConfigurationRetriever(), docRetriever); + TestUtilities.SetField(configManager, "_timeProvider", timeProvider); + + TimeSpan advanceInterval = BaseConfigurationManager.DefaultAutomaticRefreshInterval.Add(TimeSpan.FromSeconds(configManager.AutomaticRefreshInterval.TotalSeconds / 20)); + + TestUtilities.SetField(configManager, "_timeProvider", timeProvider); + + // Get the first configuration. + var configuration = await configManager.GetConfigurationAsync(CancellationToken.None); + + var configNoAdvanceInTime = await configManager.GetConfigurationAsync(CancellationToken.None); + + // First GetConfigurationAsync should not trigger a refresh because the refresh interval has not passed. + if (!object.ReferenceEquals(configuration, configNoAdvanceInTime)) + context.Diffs.Add("!object.ReferenceEquals(configuration, configNoAdvanceInTime)"); + + // Advance time to trigger a refresh. + timeProvider.Advance(advanceInterval); + + var configAfterTimeIsAdvanced = await configManager.GetConfigurationAsync(CancellationToken.None); + + // Same config, but a task is queued to update the configuration. + if (!object.ReferenceEquals(configNoAdvanceInTime, configAfterTimeIsAdvanced)) + context.Diffs.Add("!object.ReferenceEquals(configuration, configAfterTimeIsAdvanced)"); + + // Need to wait for background task to finish. + Thread.Sleep(250); + + var configAfterBackgroundTask = await configManager.GetConfigurationAsync(CancellationToken.None); + + // Configuration should be updated after the background task finishes. + if (object.ReferenceEquals(configAfterTimeIsAdvanced, configAfterBackgroundTask)) + context.Diffs.Add("object.ReferenceEquals(configuration, configAfterBackgroundTask)"); + + TestUtilities.AssertFailIfErrors(context); + } + [Theory, MemberData(nameof(ValidateOpenIdConnectConfigurationTestCases), DisableDiscoveryEnumeration = true)] public async Task ValidateOpenIdConnectConfigurationTests(ConfigurationManagerTheoryData theoryData) { diff --git a/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests.csproj b/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests.csproj index 1cbfb9eed3..8d266dc7ce 100644 --- a/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests.csproj +++ b/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests.csproj @@ -11,6 +11,12 @@ true + + + true + + PreserveNewest @@ -27,6 +33,7 @@ + From d3c4dbff093c0e1eeb686e1b5124c265015e5b57 Mon Sep 17 00:00:00 2001 From: Keegan Caruso Date: Fri, 10 Jan 2025 10:03:15 -0800 Subject: [PATCH 3/3] Review comments --- .../Configuration/ConfigurationManager.cs | 5 +++- .../ConfigurationManagerTests.cs | 30 +++++++++++++++---- 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs b/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs index b1cea9ebdc..beec3992c2 100644 --- a/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs +++ b/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs @@ -38,7 +38,10 @@ public class ConfigurationManager : BaseConfigurationManager, IConfigurationM private readonly TimeProvider _timeProvider = TimeProvider.System; // If a refresh is requested, then do the refresh as a blocking operation - // not on a background thread. + // not on a background thread. RequestRefresh signals that the app is explicitly + // requesting a refresh, so it should be done immediately so the next + // call to GetConfiguration will return new configuration if the minimum + // refresh interval has passed. bool _refreshRequested; /// diff --git a/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs b/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs index ebf33d0b68..caa7e0b6a8 100644 --- a/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs +++ b/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs @@ -743,22 +743,40 @@ public async Task RequestRefresh_RespectsRefreshInterval() configManager.RequestRefresh(); - var configAfterSecondRefresh = await configManager.GetConfigurationAsync(CancellationToken.None); + var configAfterNoTimePassed = await configManager.GetConfigurationAsync(CancellationToken.None); // Second RequestRefresh should not trigger a refresh because the refresh interval has not passed. - if (!object.ReferenceEquals(configAfterFirstRefresh, configAfterSecondRefresh)) - context.Diffs.Add("!object.ReferenceEquals(configAfterFirstRefresh, configAfterSecondRefresh)"); + if (!object.ReferenceEquals(configAfterFirstRefresh, configAfterNoTimePassed)) + context.Diffs.Add("!object.ReferenceEquals(configAfterFirstRefresh, configAfterNoTimePassed)"); // Advance time to trigger a refresh. timeProvider.Advance(configManager.RefreshInterval); configManager.RequestRefresh(); - var configAfterThirdRefresh = await configManager.GetConfigurationAsync(CancellationToken.None); + var configAfterRefreshInterval = await configManager.GetConfigurationAsync(CancellationToken.None); // Third RequestRefresh should trigger a refresh because the refresh interval has passed. - if (object.ReferenceEquals(configAfterSecondRefresh, configAfterThirdRefresh)) - context.Diffs.Add("object.ReferenceEquals(configAfterSecondRefresh, configAfterThirdRefresh)"); + if (object.ReferenceEquals(configAfterNoTimePassed, configAfterRefreshInterval)) + context.Diffs.Add("object.ReferenceEquals(configAfterNoTimePassed, configAfterRefreshInterval)"); + + // Advance time just prior to a refresh. + timeProvider.Advance(configManager.RefreshInterval.Subtract(TimeSpan.FromSeconds(1))); + + var configAfterLessThanRefreshInterval = await configManager.GetConfigurationAsync(CancellationToken.None); + + // Fourth RequestRefresh should not trigger a refresh because the refresh interval has not passed. + if (!object.ReferenceEquals(configAfterRefreshInterval, configAfterLessThanRefreshInterval)) + context.Diffs.Add("object.ReferenceEquals(configAfterRefreshInterval, configAfterLessThanRefreshInterval)"); + + // Advance time 365 days. + timeProvider.Advance(TimeSpan.FromDays(365)); + + var configAfterOneYear = await configManager.GetConfigurationAsync(CancellationToken.None); + + // Fifth RequestRefresh should trigger a refresh because the refresh interval has passed. + if (!object.ReferenceEquals(configAfterLessThanRefreshInterval, configAfterOneYear)) + context.Diffs.Add("object.ReferenceEquals(configAfterLessThanRefreshInterval, configAfterOneYear)"); TestUtilities.AssertFailIfErrors(context); }