Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RequestRefresh back to a signal for GetConfigurationAsync #3087

Merged
merged 3 commits into from
Jan 10, 2025
Merged

RequestRefresh back to a signal for GetConfigurationAsync #3087

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
931a3bf
RequestRefresh back to a signal
Jan 10, 2025
c44ef1a
Add time based testing to ConfigurationManagerTests
Jan 10, 2025
d3c4dbf
Review comments
Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions build/dependenciesTest.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
29 changes: 19 additions & 10 deletions src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,12 @@ 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.
keegan-caruso marked this conversation as resolved.
Show resolved Hide resolved
bool _refreshRequested;

/// <summary>
/// Instantiates a new <see cref="ConfigurationManager{T}"/> that manages automatic and controls refreshing on configuration data.
/// </summary>
Expand Down Expand Up @@ -147,7 +153,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;
Expand Down Expand Up @@ -214,7 +220,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);
}
}

Expand Down Expand Up @@ -285,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)));
}

Expand All @@ -309,16 +321,13 @@ public override async Task<BaseConfiguration> GetBaseConfigurationAsync(Cancella
/// </summary>
public override void RequestRefresh()
keegan-caruso marked this conversation as resolved.
Show resolved Hide resolved
{
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;
keegan-caruso marked this conversation as resolved.
Show resolved Hide resolved
}
_syncAfter = now;
_lastRequestRefresh = now;
_refreshRequested = true;
}
}

Expand Down
166 changes: 112 additions & 54 deletions test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -19,9 +20,6 @@

namespace Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests
{
/// <summary>
///
/// </summary>
public class ConfigurationManagerTests
{
/// <summary>
Expand Down Expand Up @@ -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()
{
Expand Down Expand Up @@ -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
{
Expand All @@ -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);
}
Expand Down Expand Up @@ -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);
Expand All @@ -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");
Expand Down Expand Up @@ -750,6 +718,96 @@ public void TestConfigurationComparer()
TestUtilities.AssertFailIfErrors(context);
}

[Fact]
public async Task RequestRefresh_RespectsRefreshInterval()
keegan-caruso marked this conversation as resolved.
Show resolved Hide resolved
{
// 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 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<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)
{
Expand Down
7 changes: 7 additions & 0 deletions ...rotocols.OpenIdConnect.Tests/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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>
Expand Down
Loading