Add lock when configuration is null (#2780)

Co-authored-by: id4s <user@contoso.com>

src/Microsoft.IdentityModel.Protocols.OpenIdConnect/Configuration/OpenIdConnectConfigurationValidator.cs

@@ -37,11 +37,20 @@ public ConfigurationValidationResult Validate(OpenIdConnectConfiguration openIdC
                     Succeeded = false
                 };
             }
-            var numberOfValidKeys = openIdConnectConfiguration.JsonWebKeySet.Keys.Where(key => key.ConvertedSecurityKey != null).Count();
+
+            int numberOfValidKeys = 0;
+            for (int i = 0; i < openIdConnectConfiguration.JsonWebKeySet.Keys.Count; i++)
+                if (openIdConnectConfiguration.JsonWebKeySet.Keys[i].ConvertedSecurityKey != null)
+                    numberOfValidKeys++;
 
             if (numberOfValidKeys < MinimumNumberOfKeys)
             {
-                var convertKeyInfos = string.Join("\n", openIdConnectConfiguration.JsonWebKeySet.Keys.Where(key => !string.IsNullOrEmpty(key.ConvertKeyInfo)).Select(key => key.Kid.ToString() + ": " + key.ConvertKeyInfo));
+                string convertKeyInfos = string.Join(
+                    "\n",
+                    openIdConnectConfiguration.JsonWebKeySet.Keys.Where(
+                        key => !string.IsNullOrEmpty(key.ConvertKeyInfo))
+                    .Select(key => key.Kid.ToString() + ": " + key.ConvertKeyInfo));
+
                 return new ConfigurationValidationResult
                 {
                     ErrorMessage = LogHelper.FormatInvariant(

src/Microsoft.IdentityModel.Protocols/Configuration/HttpDocumentRetriever.cs

@@ -84,17 +84,22 @@ public HttpDocumentRetriever(HttpClient httpClient)
         public async Task<string> GetDocumentAsync(string address, CancellationToken cancel)
         {
             if (string.IsNullOrWhiteSpace(address))
-                throw LogHelper.LogArgumentNullException("address");
+                throw LogHelper.LogArgumentNullException(nameof(address));
 
             if (!Utility.IsHttps(address) && RequireHttps)
-                throw LogHelper.LogExceptionMessage(new ArgumentException(LogHelper.FormatInvariant(LogMessages.IDX20108, address), nameof(address)));
+                throw LogHelper.LogExceptionMessage(
+                    new ArgumentException(
+                        LogHelper.FormatInvariant(
+                            LogMessages.IDX20108,
+                            LogHelper.MarkAsNonPII(address)),
+                        nameof(address)));
 
             Exception unsuccessfulHttpResponseException;
             HttpResponseMessage response;
             try
             {
                 if (LogHelper.IsEnabled(EventLogLevel.Verbose))
-                    LogHelper.LogVerbose(LogMessages.IDX20805, address);
+                    LogHelper.LogVerbose(LogMessages.IDX20805, LogHelper.MarkAsNonPII(address));
 
                 var httpClient = _httpClient ?? _defaultHttpClient;
                 var uri = new Uri(address, UriKind.RelativeOrAbsolute);
@@ -104,13 +109,24 @@ public async Task<string> GetDocumentAsync(string address, CancellationToken can
                 if (response.IsSuccessStatusCode)
                     return responseContent;
 
-                unsuccessfulHttpResponseException = new IOException(LogHelper.FormatInvariant(LogMessages.IDX20807, address, response, responseContent));
+                unsuccessfulHttpResponseException = new IOException(
+                    LogHelper.FormatInvariant(
+                        LogMessages.IDX20807,
+                        LogHelper.MarkAsNonPII(address),
+                        response,
+                        responseContent));
+
                 unsuccessfulHttpResponseException.Data.Add(StatusCode, response.StatusCode);
                 unsuccessfulHttpResponseException.Data.Add(ResponseContent, responseContent);
             }
             catch (Exception ex)
             {
-                throw LogHelper.LogExceptionMessage(new IOException(LogHelper.FormatInvariant(LogMessages.IDX20804, address), ex));
+                throw LogHelper.LogExceptionMessage(
+                    new IOException(
+                        LogHelper.FormatInvariant(
+                            LogMessages.IDX20804,
+                            LogHelper.MarkAsNonPII(address)),
+                        ex));
             }
 
             throw LogHelper.LogExceptionMessage(unsuccessfulHttpResponseException);

test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/OpenIdConnectSerializationTests.cs

@@ -2,6 +2,7 @@
 // Licensed under the MIT License.
 
 using System;
+using System.Collections.Generic;
 using Microsoft.IdentityModel.TestUtils;
 using Microsoft.IdentityModel.Tokens.Json.Tests;
 using Xunit;
@@ -23,6 +24,7 @@ public void Deserialize(OpenIdConnectTheoryData theoryData)
                 OpenIdConnectConfiguration configuration = new OpenIdConnectConfiguration(theoryData.Json);
                 OpenIdConnectConfiguration configurationUpperCase = new OpenIdConnectConfiguration(JsonUtilities.SetPropertiesToUpperCase(theoryData.Json));
                 theoryData.ExpectedException.ProcessNoException(context);
+                context.PropertiesToIgnoreWhenComparing.Add(typeof(OpenIdConnectConfiguration), new List<string> { "JsonWebKeySet" });
 
                 IdentityComparer.AreEqual(configuration, theoryData.CompareTo, context);
                 IdentityComparer.AreEqual(configurationUpperCase, theoryData.CompareTo, context);

test/Microsoft.IdentityModel.Protocols.Tests/ExtensibilityTests.cs

@@ -69,28 +69,34 @@ public void GetMetadataTest(DocumentRetrieverTheoryData theoryData)
         }
 
         [Fact]
-        public void ConfigurationManagerUsingCustomClass()
+        public async Task ConfigurationManagerUsingCustomClass()
         {
             var docRetriever = new FileDocumentRetriever();
             var configManager = new ConfigurationManager<IssuerMetadata>("IssuerMetadata.json", new IssuerConfigurationRetriever(), docRetriever);
-            var context = new CompareContext($"{this}.GetConfiguration");
+            var context = new CompareContext($"{this}.ConfigurationManagerUsingCustomClass");
 
             var configuration = configManager.GetConfigurationAsync().Result;
             configManager.MetadataAddress = "IssuerMetadata.json";
             var configuration2 = configManager.GetConfigurationAsync().Result;
-            if (!IdentityComparer.AreEqual(configuration.Issuer, configuration2.Issuer))
+            if (!IdentityComparer.AreEqual(configuration.Issuer, configuration2.Issuer, context))
                 context.Diffs.Add("!IdentityComparer.AreEqual(configuration, configuration2)");
 
             // AutomaticRefreshInterval should pick up new bits.
             configManager = new ConfigurationManager<IssuerMetadata>("IssuerMetadata.json", new IssuerConfigurationRetriever(), docRetriever);
             configManager.RequestRefresh();
             configuration = configManager.GetConfigurationAsync().Result;
-            TestUtilities.SetField(configManager, "_lastRefresh", DateTimeOffset.UtcNow - TimeSpan.FromHours(1));
+            TestUtilities.SetField(configManager, "_lastRequestRefresh", DateTimeOffset.UtcNow - TimeSpan.FromHours(1));
             configManager.MetadataAddress = "IssuerMetadata2.json";
             configManager.RequestRefresh();
-            configuration2 = configManager.GetConfigurationAsync().Result;
-            if (IdentityComparer.AreEqual(configuration.Issuer, configuration2.Issuer))
-                context.Diffs.Add("IdentityComparer.AreEqual(configuration, configuration2)");
+
+            // Wait for the refresh to complete.
+            await Task.Delay(250).ContinueWith(_ =>
+            {
+
+                configuration2 = configManager.GetConfigurationAsync().GetAwaiter().GetResult();
+                if (IdentityComparer.AreEqual(configuration.Issuer, configuration2.Issuer))
+                    context.Diffs.Add("IdentityComparer.AreEqual(configuration.Issuer, configuration2.Issuer)");
+            });
 
             TestUtilities.AssertFailIfErrors(context);
         }

test/Microsoft.IdentityModel.TestUtils/InMemoryDocumentRetriever.cs

@@ -0,0 +1,38 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+using System.Collections.Generic;
+using System.Threading;
+using System.Threading.Tasks;
+using Microsoft.IdentityModel.Protocols;
+
+namespace Microsoft.IdentityModel.TestUtils
+{
+    /// <summary>
+    /// Returns a string set in the constructor.
+    /// Simplifies testing.
+    /// </summary>
+    public class InMemoryDocumentRetriever : IDocumentRetriever
+    {
+        private readonly IDictionary<string, string> _configurations;
+
+        /// <summary>
+        /// Initializes a new instance of the <see cref="FileDocumentRetriever"/> class.
+        /// </summary>
+        public InMemoryDocumentRetriever(IDictionary<string, string> configuration)
+        {
+            _configurations = configuration;
+        }
+
+        /// <summary>
+        /// Returns the document passed in constructor in dictionary./>
+        /// </summary>
+        /// <param name="address">Fully qualified path to a file. Ignored for now.</param>
+        /// <param name="cancel"><see cref="CancellationToken"/> Ignored for now.</param>
+        /// <returns>UTF8 decoding of bytes in the file.</returns>
+        public async Task<string> GetDocumentAsync(string address, CancellationToken cancel)
+        {
+            return await Task.FromResult(_configurations[address]).ConfigureAwait(false);
+        }
+    }
+}

test/Microsoft.IdentityModel.TestUtils/SampleListener.cs

@@ -8,7 +8,7 @@ namespace Microsoft.IdentityModel.TestUtils
 {
     public class SampleListener : EventListener
     {
-        public string TraceBuffer { get; set; }
+        public string TraceBuffer { get; set; } = string.Empty;
 
         protected override void OnEventWritten(EventWrittenEventArgs eventData)
         {