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

Xiao/Enrich metadata failure message during metadata refresh interval #2010

Merged
merged 3 commits into from
Feb 14, 2023
Merged

Xiao/Enrich metadata failure message during metadata refresh interval #2010

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
9a4802a
Enrich exception during metadata refresh interval
ciaozhang Feb 1, 2023
afca850
rename property
ciaozhang Feb 1, 2023
9148352
address comment
ciaozhang Feb 14, 2023
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
4 changes: 3 additions & 1 deletion src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ public class ConfigurationManager<T> : BaseConfigurationManager, IConfigurationM
private readonly IConfigurationRetriever<T> _configRetriever;
private readonly IConfigurationValidator<T> _configValidator;
private T _currentConfiguration;
private Exception _fetchMetadataFailure;

/// <summary>
/// Static initializer for a new object. Static initializers run before the first instance of the type is created.
Expand Down Expand Up @@ -148,6 +149,7 @@ public async Task<T> GetConfigurationAsync(CancellationToken cancel)
}
catch (Exception ex)
{
_fetchMetadataFailure = ex;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What requires this to be a variable at the class level instead of just a local variable? Seems like there's an oppertunity here for race conditions and I'm not clear on the advantage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If ConfigurationManager.GetConfigurationAsync is called again before the syncAfter, then the IDX20803 exception at Line 167 gets thrown instead. We need the exception at the class level so that we can return it as the inner exception.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this seems reasonable to me. Makes sense we wouldn't just want to have the exception in logs the first time from a usibility/servicability standpoint.

_syncAfter = DateTimeUtil.Add(now.UtcDateTime, AutomaticRefreshInterval < RefreshInterval ? AutomaticRefreshInterval : RefreshInterval);
if (_currentConfiguration == null) // Throw an exception if there's no configuration to return.
throw LogHelper.LogExceptionMessage(
Expand All @@ -166,7 +168,7 @@ public async Task<T> GetConfigurationAsync(CancellationToken cancel)
else
throw LogHelper.LogExceptionMessage(
new InvalidOperationException(
LogHelper.FormatInvariant(LogMessages.IDX20803, LogHelper.MarkAsNonPII(MetadataAddress ?? "null"))));
LogHelper.FormatInvariant(LogMessages.IDX20803, LogHelper.MarkAsNonPII(MetadataAddress ?? "null"), LogHelper.MarkAsNonPII(_fetchMetadataFailure)), _fetchMetadataFailure));
}
finally
{
Expand Down
36 changes: 36 additions & 0 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 @@ -6,6 +6,7 @@
using System.Collections.Generic;
using System.Diagnostics.Tracing;
using System.IO;
using System.Net;
using System.Reflection;
using System.Threading;
using Microsoft.IdentityModel.Protocols.OpenIdConnect.Configuration;
Expand Down Expand Up @@ -100,6 +101,41 @@ public void Defaults()
Assert.Equal(ConfigurationManager<OpenIdConnectConfiguration>.MinimumRefreshInterval, new TimeSpan(0, 0, 0, 1));
}

[Fact]
public void FetchMetadataFailureTest()
{
var context = new CompareContext($"{this}.FetchMetadataFailureTest");

var documentRetriever = new HttpDocumentRetriever(HttpResponseMessageUtils.SetupHttpClientThatReturns("OpenIdConnectMetadata.json", HttpStatusCode.NotFound));
var configManager = new ConfigurationManager<OpenIdConnectConfiguration>("OpenIdConnectMetadata.json", new OpenIdConnectConfigurationRetriever(), documentRetriever);

//First time to fetch metadata
ciaozhang marked this conversation as resolved.
Show resolved Hide resolved
try
{
var configuration = configManager.GetConfigurationAsync().Result;
}
catch (Exception firstFetchMetadataFailure)
{
if (firstFetchMetadataFailure.InnerException == null)
context.AddDiff($"Expected exception to contain inner exception for fetch metadata failure.");

//Fetch metadata again during refresh interval, the exception should be same from above
try
{
var configuration = configManager.GetConfigurationAsync().Result;
}
catch (Exception secondFetchMetadataFailure)
{
if (secondFetchMetadataFailure.InnerException == null)
context.AddDiff($"Expected exception to contain inner exception for fetch metadata failure.");

IdentityComparer.AreEqual(firstFetchMetadataFailure, secondFetchMetadataFailure, context);
}
}

TestUtilities.AssertFailIfErrors(context);
}

[Fact]
public void GetSets()
{
Expand Down