diff --git a/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs b/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs index d3d0ad4f40..b21b4ba89c 100644 --- a/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs +++ b/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs @@ -31,6 +31,12 @@ public partial class ConfigurationManager : BaseConfigurationManager, IConfig private readonly IConfigurationValidator _configValidator; private T _currentConfiguration; + // Tracks the most recent fetch failure for the blocking path. Promoted from a local in + // GetConfigurationWithBlockingAsync so the original exception (e.g. an IOException carrying + // HttpDocumentRetriever.StatusCode/ResponseContent in its Data dictionary) is preserved across + // calls that arrive within the backoff window (_syncAfter > now) and skip the fetch. + private Exception _fetchMetadataFailure; + // task states are used to ensure the call to 'update config' (UpdateCurrentConfiguration) is a singleton. Uses Interlocked.CompareExchange. // metadata is not being obtained private const int ConfigurationRetrieverIdle = 0; diff --git a/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager_Blocking.cs b/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager_Blocking.cs index 28492c627b..67db24f04c 100644 --- a/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager_Blocking.cs +++ b/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager_Blocking.cs @@ -23,7 +23,6 @@ public partial class ConfigurationManager where T : class private async Task GetConfigurationWithBlockingAsync(CancellationToken cancel) { - Exception _fetchMetadataFailure = null; await _refreshLock.WaitAsync(cancel).ConfigureAwait(false); long startTimestamp = TimeProvider.GetTimestamp(); @@ -52,6 +51,8 @@ private async Task GetConfigurationWithBlockingAsync(CancellationToken cancel UpdateConfiguration(configurationRetrieved.Configuration, configurationRetrieved.RetrievalTime, retrievalContext); + _fetchMetadataFailure = null; + return _currentConfiguration; } } @@ -81,6 +82,8 @@ private async Task GetConfigurationWithBlockingAsync(CancellationToken cancel _refreshRequested = false; UpdateConfiguration(configuration, TimeProvider.GetUtcNow(), retrievalContext); + + _fetchMetadataFailure = null; } catch (Exception ex) { diff --git a/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs b/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs index 9cd114f139..cb827355fd 100644 --- a/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs +++ b/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs @@ -187,6 +187,98 @@ public async Task FetchMetadataFailureTest_Blocking() await FetchMetadataFailureTestBody(); } + // Reproduces a bug in the blocking path (Switch.Microsoft.IdentityModel.UpdateConfigAsBlocking). + // When a fetch fails and a subsequent call arrives within the backoff window (i.e. _syncAfter > now), + // GetConfigurationWithBlockingAsync skips the fetch entirely and throws IDX20803 with a null + // InnerException because _fetchMetadataFailure is declared as a local variable (reset to null on + // every invocation) rather than persisted between calls. This loses the original IOException + // (and the HttpDocumentRetriever.StatusCode / ResponseContent it carries in Data), preventing + // callers from distinguishing client errors (4xx -> misconfigured tenant) from server errors + // (5xx -> transient). + [Fact] + public async Task FetchMetadataFailure_Blocking_PreservesInnerExceptionDuringBackoffWindow() + { + AppContext.SetSwitch(AppContextSwitches.UpdateConfigAsBlockingSwitch, true); + + var context = new CompareContext($"{this}.{nameof(FetchMetadataFailure_Blocking_PreservesInnerExceptionDuringBackoffWindow)}"); + + var documentRetriever = new HttpDocumentRetriever( + HttpResponseMessageUtils.SetupHttpClientThatReturns("OpenIdConnectMetadata.json", HttpStatusCode.NotFound)); + var configManager = new ConfigurationManager( + "https://example.invalid/.well-known/openid-configuration", + new OpenIdConnectConfigurationRetriever(), + documentRetriever); + + // First call: fetch is attempted and fails. The thrown InvalidOperationException should + // wrap the original IOException carrying the HTTP status code in its Data dictionary. + Exception firstException = null; + try + { + _ = await configManager.GetConfigurationAsync(CancellationToken.None); + } + catch (Exception ex) + { + firstException = ex; + } + + if (firstException == null) + context.AddDiff("Expected first GetConfigurationAsync call to throw."); + else + { + if (firstException.InnerException == null) + context.AddDiff("Expected first call's InvalidOperationException to wrap the underlying IOException."); + else if (!ExceptionChainContainsStatusCode(firstException)) + context.AddDiff("Expected first call's exception chain to contain HttpDocumentRetriever.StatusCode in Data."); + } + + // Force the backoff window: ensure _syncAfter is in the future so the next call skips the fetch + // and goes through the "stale metadata is better than no metadata" path. _currentConfiguration + // is still null (bootstrap never succeeded), so the manager re-throws IDX20803. + TestUtilities.SetField(configManager, "_syncAfter", DateTimeOffset.UtcNow.AddHours(1)); + + Exception secondException = null; + try + { + _ = await configManager.GetConfigurationAsync(CancellationToken.None); + } + catch (Exception ex) + { + secondException = ex; + } + + if (secondException == null) + context.AddDiff("Expected second GetConfigurationAsync call (within backoff window) to throw."); + else + { + // The bug: second call throws IDX20803 with InnerException == null, losing the HTTP + // status code that callers use to classify the error (401 vs 503). + if (secondException.InnerException == null) + { + context.AddDiff( + "BUG: Second call within backoff window threw IDX20803 with a null InnerException. " + + "The original IOException (with HttpDocumentRetriever.StatusCode in Data) was lost " + + "because _fetchMetadataFailure is a local variable in GetConfigurationWithBlockingAsync."); + } + else if (!ExceptionChainContainsStatusCode(secondException)) + { + context.AddDiff("Expected second call's exception chain to contain HttpDocumentRetriever.StatusCode in Data."); + } + } + + TestUtilities.AssertFailIfErrors(context); + } + + private static bool ExceptionChainContainsStatusCode(Exception exception) + { + for (Exception current = exception; current != null; current = current.InnerException) + { + if (current.Data.Contains(HttpDocumentRetriever.StatusCode)) + return true; + } + + return false; + } + private async ValueTask FetchMetadataFailureTestBody() { var context = new CompareContext($"{this}.FetchMetadataFailureTest");