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 ed3d4b7e67..beec3992c2 100644 --- a/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs +++ b/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs @@ -35,6 +35,15 @@ 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. 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; + /// /// Instantiates a new that manages automatic and controls refreshing on configuration data. /// @@ -147,7 +156,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; @@ -214,7 +223,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); } } @@ -285,7 +300,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))); } @@ -309,16 +324,13 @@ 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; - if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverRunning, ConfigurationRetrieverIdle) == ConfigurationRetrieverIdle) - { - UpdateCurrentConfiguration(); - _lastRequestRefresh = now; - } + _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 2fba523106..caa7e0b6a8 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 { /// @@ -209,48 +207,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 +276,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 +297,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,10 +564,10 @@ public static TheoryData("OpenIdConnectMetadata.json", new OpenIdConnectConfigurationRetriever(), docRetriever); @@ -634,9 +593,18 @@ public async Task CheckSyncAfter() // make same check for RequestRefresh // force a refresh by setting internal field TestUtilities.SetField(configManager, "_lastRequestRefresh", DateTimeOffset.UtcNow - TimeSpan.FromHours(1)); + configManager.RequestRefresh(); - // wait 1000ms here because update of config is run as a new task. - Thread.Sleep(1000); + + bool refreshRequested = (bool)TestUtilities.GetField(configManager, "_refreshRequested"); + if (!refreshRequested) + context.Diffs.Add("Refresh is expected to be requested after RequestRefresh is called"); + + await configManager.GetConfigurationAsync(); + + refreshRequested = (bool)TestUtilities.GetField(configManager, "_refreshRequested"); + if (refreshRequested) + context.Diffs.Add("Refresh is not expected to be requested after GetConfigurationAsync is called"); // check that _syncAfter is greater than DateTimeOffset.UtcNow + AutomaticRefreshInterval syncAfter = (DateTimeOffset)TestUtilities.GetField(configManager, "_syncAfter"); @@ -750,6 +718,114 @@ 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 configAfterNoTimePassed = await configManager.GetConfigurationAsync(CancellationToken.None); + + // Second RequestRefresh should not trigger a refresh because the refresh interval has not passed. + 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 configAfterRefreshInterval = await configManager.GetConfigurationAsync(CancellationToken.None); + + // Third RequestRefresh should trigger a refresh because the refresh interval has passed. + 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); + } + + [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 @@ +