Skip to content

Commit

Permalink
RequestRefresh back to a signal for GetConfigurationAsync (#3087)
Browse files Browse the repository at this point in the history
* 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

* Add time based testing to ConfigurationManagerTests
through FakeTimeProvider
  • Loading branch information
keegan-caruso authored Jan 10, 2025
1 parent 6146f1f commit 42b0c2c
Showing 4 changed files with 168 additions and 64 deletions.
9 changes: 9 additions & 0 deletions build/dependenciesTest.props
Original file line number Diff line number Diff line change
@@ -19,4 +19,13 @@
<PropertyGroup Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net9.0'))">
<XunitRunnerVisualStudioVersion>3.0.0-pre.49</XunitRunnerVisualStudioVersion>
</PropertyGroup>

<!-- MicrosoftExtensionsTimeProviderTesting 8.x has a 6.0.0 target, 9.x does not. -->
<PropertyGroup Condition="'$(TargetFramework)' != 'net6.0'">
<MicrosoftExtensionsTimeProviderTestingVersion>9.0.0</MicrosoftExtensionsTimeProviderTestingVersion>
</PropertyGroup>
<PropertyGroup Condition="'$(TargetFramework)' == 'net6.0'">
<MicrosoftExtensionsTimeProviderTestingVersion>8.10.0</MicrosoftExtensionsTimeProviderTestingVersion>
</PropertyGroup>

</Project>
Original file line number Diff line number Diff line change
@@ -35,6 +35,15 @@ public class ConfigurationManager<T> : 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;

/// <summary>
/// Instantiates a new <see cref="ConfigurationManager{T}"/> that manages automatic and controls refreshing on configuration data.
/// </summary>
@@ -147,7 +156,7 @@ public async Task<T> GetConfigurationAsync()
/// <remarks>If the time since the last call is less than <see cref="BaseConfigurationManager.AutomaticRefreshInterval"/> then <see cref="IConfigurationRetriever{T}.GetConfigurationAsync"/> is not called and the current Configuration is returned.</remarks>
public virtual async Task<T> 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<T> 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<BaseConfiguration> GetBaseConfigurationAsync(Cancella
/// </summary>
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;
}
}

Original file line number Diff line number Diff line change
@@ -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
{
/// <summary>
///
/// </summary>
public class ConfigurationManagerTests
{
/// <summary>
@@ -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<OpenIdConnectConfiguration>(
"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<ConfigurationManagerTheoryData<OpenIdConnectConfigurati
}

[Fact]
public async Task CheckSyncAfter()
public async Task CheckSyncAfterAndRefreshRequested()
{
// This test checks that the _syncAfter field is set correctly after a refresh.
var context = new CompareContext($"{this}.CheckSyncAfter");
var context = new CompareContext($"{this}.CheckSyncAfterAndRefreshRequested");

var docRetriever = new FileDocumentRetriever();
var configManager = new ConfigurationManager<OpenIdConnectConfiguration>("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<OpenIdConnectConfiguration>("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<OpenIdConnectConfiguration>("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<OpenIdConnectConfiguration> theoryData)
{
Original file line number Diff line number Diff line change
@@ -11,6 +11,12 @@
<DelaySign>true</DelaySign>
</PropertyGroup>

<PropertyGroup Condition="'$(TargetFramework)' == 'net462' or '$(TargetFramework)' == 'net472'">
<!-- This warning comes from Microsoft.Extensions.TimeProvider.Testing
see here for details: https://github.com/dotnet/extensions/issues/5162#issuecomment-2316266601-->
<SuppressTfmSupportBuildWarnings>true</SuppressTfmSupportBuildWarnings>
</PropertyGroup>

<ItemGroup>
<None Update="JsonWebKeySet.json;JsonWebKeySetBadBase64Data.json;JsonWebKeySetBadX509Data.json;JsonWebKeySetEnd2End.json;JsonWebKeySetSingleX509Data.json;OpenIdConnectMetadata.json;OpenIdConnectMetadata2.json;JsonWebKeySetUnrecognizedKty.json;JsonWebKeySetBadRsaDataMissingComponent.json;OpenIdConnectMetadataBadBase64Data.json;OpenIdConnectMetadataBadX509Data.json;OpenIdConnectMetadataEnd2End.json;OpenIdConnectMetadataJsonWebKeySetBadUri.json;PingLabsJWKS.json;PingLabs-openid-configuration.json;;JsonWebKeySetEnd2EndEC.json;OpenIdConnectMetadataEnd2EndEC.json;OpenIdConnectMetadataUnrecognizedKty.json;OpenIdConnectMetadataBadRsaDataMissingComponent.json;OpenIdConnectMetadataEnd2EndAcrValuesLast.json">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
@@ -27,6 +33,7 @@
<ItemGroup>
<PackageReference Include="System.Text.RegularExpressions" Version="$(SystemTextRegularExpressions)"/>
<PackageReference Include="System.Net.Http" Version="$(SystemNetHttp)"/>
<PackageReference Include="Microsoft.Extensions.TimeProvider.Testing" Version="$(MicrosoftExtensionsTimeProviderTestingVersion)" />
</ItemGroup>

<ItemGroup>

0 comments on commit 42b0c2c

Please sign in to comment.