diff --git a/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs b/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs index beec3992c2..fe6d70ff31 100644 --- a/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs +++ b/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs @@ -3,6 +3,7 @@ using System; using System.Net.Http; +using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; using Microsoft.IdentityModel.Logging; @@ -18,8 +19,16 @@ namespace Microsoft.IdentityModel.Protocols [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1001:TypesThatOwnDisposableFieldsShouldBeDisposable")] public class ConfigurationManager : BaseConfigurationManager, IConfigurationManager where T : class { - private DateTimeOffset _syncAfter = DateTimeOffset.MinValue; - private DateTimeOffset _lastRequestRefresh = DateTimeOffset.MinValue; + // To prevent tearing, this needs to be only updated through AtomicUpdateSyncAfter. + // Reads should be done through the property SyncAfter. + private DateTime _syncAfter = DateTime.MinValue; + private DateTime SyncAfter => _syncAfter; + + // See comment above, this should only be updated through AtomicUpdateLastRequestRefresh, + // read through LastRequestRefresh. + private DateTime _lastRequestRefresh = DateTime.MinValue; + private DateTime LastRequestRefresh => _lastRequestRefresh; + private bool _isFirstRefreshRequest = true; private readonly SemaphoreSlim _configurationNullLock = new SemaphoreSlim(1); @@ -156,7 +165,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 > _timeProvider.GetUtcNow()) + if (_currentConfiguration != null && SyncAfter > _timeProvider.GetUtcNow()) return _currentConfiguration; Exception fetchMetadataFailure = null; @@ -242,7 +251,7 @@ public virtual async Task GetConfigurationAsync(CancellationToken cancel) LogHelper.FormatInvariant( LogMessages.IDX20803, LogHelper.MarkAsNonPII(MetadataAddress ?? "null"), - LogHelper.MarkAsNonPII(_syncAfter), + LogHelper.MarkAsNonPII(SyncAfter), LogHelper.MarkAsNonPII(fetchMetadataFailure)), fetchMetadataFailure)); } @@ -300,8 +309,27 @@ private void UpdateCurrentConfiguration() private void UpdateConfiguration(T configuration) { _currentConfiguration = configuration; - _syncAfter = DateTimeUtil.Add(_timeProvider.GetUtcNow().UtcDateTime, AutomaticRefreshInterval + + var newSyncTime = DateTimeUtil.Add(_timeProvider.GetUtcNow().UtcDateTime, AutomaticRefreshInterval + TimeSpan.FromSeconds(new Random().Next((int)AutomaticRefreshInterval.TotalSeconds / 20))); + AtomicUpdateSyncAfter(newSyncTime); + } + + private void AtomicUpdateSyncAfter(DateTime syncAfter) + { + // DateTime's backing data is safe to treat as a long if the Kind is not local. + // _syncAfter will always be updated to a UTC time. + // See the implementation of ToBinary on DateTime. + Interlocked.Exchange( + ref Unsafe.As(ref _syncAfter), + Unsafe.As(ref syncAfter)); + } + + private void AtomicUpdateLastRequestRefresh(DateTime lastRequestRefresh) + { + // See the comment in AtomicUpdateSyncAfter. + Interlocked.Exchange( + ref Unsafe.As(ref _lastRequestRefresh), + Unsafe.As(ref lastRequestRefresh)); } /// @@ -324,12 +352,12 @@ public override async Task GetBaseConfigurationAsync(Cancella /// public override void RequestRefresh() { - DateTimeOffset now = _timeProvider.GetUtcNow(); - if (now >= DateTimeUtil.Add(_lastRequestRefresh.UtcDateTime, RefreshInterval) || _isFirstRefreshRequest) + DateTime now = _timeProvider.GetUtcNow().UtcDateTime; + if (now >= DateTimeUtil.Add(LastRequestRefresh, RefreshInterval) || _isFirstRefreshRequest) { _isFirstRefreshRequest = false; - _syncAfter = now; - _lastRequestRefresh = now; + AtomicUpdateSyncAfter(now); + AtomicUpdateLastRequestRefresh(now); _refreshRequested = true; } } diff --git a/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs b/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs index caa7e0b6a8..1ee7645570 100644 --- a/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs +++ b/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs @@ -1,7 +1,7 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -// Ignore Spelling: Metadata Validator Retreiver +// Ignore Spelling: Metadata Validator using System; using System.Collections.Generic; @@ -37,7 +37,7 @@ public async Task GetPublicMetadata(ConfigurationManagerTheoryData( theoryData.MetadataAddress, - theoryData.ConfigurationRetreiver, + theoryData.ConfigurationRetriever, theoryData.DocumentRetriever, theoryData.ConfigurationValidator); @@ -60,7 +60,7 @@ public static TheoryData("AccountsGoogleCom") { - ConfigurationRetreiver = new OpenIdConnectConfigurationRetriever(), + ConfigurationRetriever = new OpenIdConnectConfigurationRetriever(), ConfigurationValidator = new OpenIdConnectConfigurationValidator(), DocumentRetriever = new HttpDocumentRetriever(), MetadataAddress = OpenIdConfigData.AccountsGoogle @@ -68,7 +68,7 @@ public static TheoryData("AADCommonUrl") { - ConfigurationRetreiver = new OpenIdConnectConfigurationRetriever(), + ConfigurationRetriever = new OpenIdConnectConfigurationRetriever(), ConfigurationValidator = new OpenIdConnectConfigurationValidator(), DocumentRetriever = new HttpDocumentRetriever(), MetadataAddress = OpenIdConfigData.AADCommonUrl @@ -76,7 +76,7 @@ public static TheoryData("AADCommonUrlV1") { - ConfigurationRetreiver = new OpenIdConnectConfigurationRetriever(), + ConfigurationRetriever = new OpenIdConnectConfigurationRetriever(), ConfigurationValidator = new OpenIdConnectConfigurationValidator(), DocumentRetriever = new HttpDocumentRetriever(), MetadataAddress = OpenIdConfigData.AADCommonUrlV1 @@ -84,7 +84,7 @@ public static TheoryData("AADCommonUrlV2") { - ConfigurationRetreiver = new OpenIdConnectConfigurationRetriever(), + ConfigurationRetriever = new OpenIdConnectConfigurationRetriever(), ConfigurationValidator = new OpenIdConnectConfigurationValidator(), DocumentRetriever = new HttpDocumentRetriever(), MetadataAddress = OpenIdConfigData.AADCommonUrlV2 @@ -99,7 +99,7 @@ public void OpenIdConnectConstructor(ConfigurationManagerTheoryData(theoryData.MetadataAddress, theoryData.ConfigurationRetreiver, theoryData.DocumentRetriever, theoryData.ConfigurationValidator); + var configurationManager = new ConfigurationManager(theoryData.MetadataAddress, theoryData.ConfigurationRetriever, theoryData.DocumentRetriever, theoryData.ConfigurationValidator); theoryData.ExpectedException.ProcessNoException(); } catch (Exception ex) @@ -118,7 +118,7 @@ public static TheoryData { - ConfigurationRetreiver = new OpenIdConnectConfigurationRetriever(), + ConfigurationRetriever = new OpenIdConnectConfigurationRetriever(), ConfigurationValidator = new OpenIdConnectConfigurationValidator(), DocumentRetriever = new HttpDocumentRetriever(), ExpectedException = ExpectedException.ArgumentNullException("IDX10000:"), @@ -129,7 +129,7 @@ public static TheoryData { - ConfigurationRetreiver = null, + ConfigurationRetriever = null, ConfigurationValidator = new OpenIdConnectConfigurationValidator(), DocumentRetriever = new HttpDocumentRetriever(), ExpectedException = ExpectedException.ArgumentNullException("IDX10000:"), @@ -139,7 +139,7 @@ public static TheoryData { - ConfigurationRetreiver = new OpenIdConnectConfigurationRetriever(), + ConfigurationRetriever = new OpenIdConnectConfigurationRetriever(), ConfigurationValidator = new OpenIdConnectConfigurationValidator(), DocumentRetriever = null, ExpectedException = ExpectedException.ArgumentNullException("IDX10000:"), @@ -149,7 +149,7 @@ public static TheoryData { - ConfigurationRetreiver = new OpenIdConnectConfigurationRetriever(), + ConfigurationRetriever = new OpenIdConnectConfigurationRetriever(), ConfigurationValidator = null, DocumentRetriever = new HttpDocumentRetriever(), ExpectedException = ExpectedException.ArgumentNullException("IDX10000:"), @@ -232,7 +232,7 @@ public async Task VerifyInterlockGuardForGetConfigurationAsync() waitEvent.Reset(); signalEvent.Reset(); - TestUtilities.SetField(configurationManager, "_syncAfter", DateTimeOffset.MinValue); + TestUtilities.SetField(configurationManager, "_syncAfter", DateTime.MinValue); await configurationManager.GetConfigurationAsync(CancellationToken.None); // InMemoryDocumentRetrieverWithEvents will signal when it is OK to change the MetadataAddress @@ -276,8 +276,8 @@ public async Task BootstrapRefreshIntervalTest() catch (Exception firstFetchMetadataFailure) { // _syncAfter should not have been changed, because the fetch failed. - DateTimeOffset syncAfter = (DateTimeOffset)TestUtilities.GetField(configManager, "_syncAfter"); - if (syncAfter != DateTimeOffset.MinValue) + DateTime syncAfter = (DateTime)TestUtilities.GetField(configManager, "_syncAfter"); + if (syncAfter != DateTime.MinValue) context.AddDiff($"ConfigurationManager._syncAfter: '{syncAfter}' should equal '{DateTimeOffset.MinValue}'."); if (firstFetchMetadataFailure.InnerException == null) @@ -297,10 +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 = (DateTimeOffset)TestUtilities.GetField(configManager, "_syncAfter"); + syncAfter = (DateTime)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}'."); + if (!IdentityComparer.AreDatesEqualWithEpsilon(requestTime, syncAfter, 1)) + context.AddDiff($"ConfigurationManager._syncAfter: '{syncAfter}' should equal be within 1 second of '{requestTime}'."); IdentityComparer.AreEqual(firstFetchMetadataFailure, secondFetchMetadataFailure, context); } @@ -355,10 +355,10 @@ public async Task AutomaticRefreshInterval(ConfigurationManagerTheoryData("OpenIdConnectMetadata.json", new OpenIdConnectConfigurationRetriever(), docRetriever); var configuration = await configManager.GetConfigurationAsync(CancellationToken.None); - TestUtilities.SetField(configManager, "_lastRequestRefresh", DateTimeOffset.UtcNow - TimeSpan.FromHours(1)); + TestUtilities.SetField(configManager, "_lastRequestRefresh", DateTime.UtcNow.Subtract(TimeSpan.FromHours(1))); configManager.MetadataAddress = "http://127.0.0.1"; configManager.RequestRefresh(); var configuration2 = await configManager.GetConfigurationAsync(CancellationToken.None); @@ -832,7 +832,7 @@ public async Task ValidateOpenIdConnectConfigurationTests(ConfigurationManagerTh TestUtilities.WriteHeader($"{this}.ValidateOpenIdConnectConfigurationTests"); var context = new CompareContext(); OpenIdConnectConfiguration configuration; - var configurationManager = new ConfigurationManager(theoryData.MetadataAddress, theoryData.ConfigurationRetreiver, theoryData.DocumentRetriever, theoryData.ConfigurationValidator); + var configurationManager = new ConfigurationManager(theoryData.MetadataAddress, theoryData.ConfigurationRetriever, theoryData.DocumentRetriever, theoryData.ConfigurationValidator); if (theoryData.PresetCurrentConfiguration) TestUtilities.SetField(configurationManager, "_currentConfiguration", new OpenIdConnectConfiguration() { Issuer = Default.Issuer }); @@ -872,7 +872,7 @@ public static TheoryData { - ConfigurationRetreiver = new OpenIdConnectConfigurationRetriever(), + ConfigurationRetriever = new OpenIdConnectConfigurationRetriever(), ConfigurationValidator = openIdConnectConfigurationValidator, DocumentRetriever = new FileDocumentRetriever(), First = true, @@ -882,7 +882,7 @@ public static TheoryData { - ConfigurationRetreiver = new OpenIdConnectConfigurationRetriever(), + ConfigurationRetriever = new OpenIdConnectConfigurationRetriever(), ConfigurationValidator = openIdConnectConfigurationValidator2, DocumentRetriever = new FileDocumentRetriever(), ExpectedException = new ExpectedException(typeof(InvalidOperationException), "IDX21818:", typeof(InvalidConfigurationException)), @@ -892,7 +892,7 @@ public static TheoryData { - ConfigurationRetreiver = new OpenIdConnectConfigurationRetriever(), + ConfigurationRetriever = new OpenIdConnectConfigurationRetriever(), ConfigurationValidator = openIdConnectConfigurationValidator2, DocumentRetriever = new FileDocumentRetriever(), PresetCurrentConfiguration = true, @@ -903,7 +903,7 @@ public static TheoryData { - ConfigurationRetreiver = new OpenIdConnectConfigurationRetriever(), + ConfigurationRetriever = new OpenIdConnectConfigurationRetriever(), ConfigurationValidator = openIdConnectConfigurationValidator2, DocumentRetriever = new FileDocumentRetriever(), ExpectedException = new ExpectedException(typeof(InvalidOperationException), "IDX10810:", typeof(InvalidConfigurationException)), @@ -913,7 +913,7 @@ public static TheoryData { - ConfigurationRetreiver = new OpenIdConnectConfigurationRetriever(), + ConfigurationRetriever = new OpenIdConnectConfigurationRetriever(), ConfigurationValidator = openIdConnectConfigurationValidator2, DocumentRetriever = new FileDocumentRetriever(), PresetCurrentConfiguration = true, @@ -924,7 +924,7 @@ public static TheoryData { - ConfigurationRetreiver = new OpenIdConnectConfigurationRetriever(), + ConfigurationRetriever = new OpenIdConnectConfigurationRetriever(), ConfigurationValidator = openIdConnectConfigurationValidator2, DocumentRetriever = new FileDocumentRetriever(), ExpectedException = new ExpectedException(typeof(InvalidOperationException), "IDX21817:", typeof(InvalidConfigurationException)), @@ -934,7 +934,7 @@ public static TheoryData { - ConfigurationRetreiver = new OpenIdConnectConfigurationRetriever(), + ConfigurationRetriever = new OpenIdConnectConfigurationRetriever(), ConfigurationValidator = openIdConnectConfigurationValidator2, DocumentRetriever = new FileDocumentRetriever(), PresetCurrentConfiguration = true, @@ -945,7 +945,7 @@ public static TheoryData { - ConfigurationRetreiver = new OpenIdConnectConfigurationRetriever(), + ConfigurationRetriever = new OpenIdConnectConfigurationRetriever(), ConfigurationValidator = openIdConnectConfigurationValidator2, DocumentRetriever = new FileDocumentRetriever(), ExpectedException = new ExpectedException(typeof(InvalidOperationException), "IDX10814:", typeof(InvalidConfigurationException)), @@ -955,7 +955,7 @@ public static TheoryData { - ConfigurationRetreiver = new OpenIdConnectConfigurationRetriever(), + ConfigurationRetriever = new OpenIdConnectConfigurationRetriever(), ConfigurationValidator = openIdConnectConfigurationValidator2, DocumentRetriever = new FileDocumentRetriever(), PresetCurrentConfiguration = true, @@ -1001,7 +1001,7 @@ public ConfigurationManagerTheoryData(string testId) : base(testId) { } public TimeSpan AutomaticRefreshInterval { get; set; } - public IConfigurationRetriever ConfigurationRetreiver { get; set; } + public IConfigurationRetriever ConfigurationRetriever { get; set; } public IConfigurationValidator ConfigurationValidator { get; set; } diff --git a/test/Microsoft.IdentityModel.Protocols.Tests/ExtensibilityTests.cs b/test/Microsoft.IdentityModel.Protocols.Tests/ExtensibilityTests.cs index c37f5d04ce..71cf3e66d8 100644 --- a/test/Microsoft.IdentityModel.Protocols.Tests/ExtensibilityTests.cs +++ b/test/Microsoft.IdentityModel.Protocols.Tests/ExtensibilityTests.cs @@ -81,7 +81,7 @@ public async Task ConfigurationManagerUsingCustomClass() configManager = new ConfigurationManager("IssuerMetadata.json", new IssuerConfigurationRetriever(), docRetriever); configManager.RequestRefresh(); configuration = await configManager.GetConfigurationAsync(); - TestUtilities.SetField(configManager, "_lastRequestRefresh", DateTimeOffset.UtcNow - TimeSpan.FromHours(1)); + TestUtilities.SetField(configManager, "_lastRequestRefresh", DateTime.UtcNow.Subtract(TimeSpan.FromHours(1))); configManager.MetadataAddress = "IssuerMetadata2.json"; // Wait for the refresh to complete.