Fix for 5108 - avoid concurrent writes to extra QP dictionary

* Fix for 5108 - avoid concurrent writes to extra QP dictionary

* test fix

* 2

* Address PR comments

* Fix for 5108 - avoid concurrent writes to extra QP dictionary

* 2

* Delete

* Update comments in ParallelRequestMockHandler.cs

Author: bgavrilMS

src/client/Microsoft.Identity.Client/AppConfig/AbstractApplicationBuilder.cs

@@ -294,7 +294,7 @@ protected T WithOptions(ApplicationOptions applicationOptions)
         /// <returns>The builder to chain the .With methods</returns>
         public T WithExtraQueryParameters(IDictionary<string, string> extraQueryParameters)
         {
-            Config.ExtraQueryParameters = extraQueryParameters ?? new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
+            Config.ExtraQueryParameters = extraQueryParameters;
             return this as T;
         }
 

src/client/Microsoft.Identity.Client/AppConfig/ApplicationConfiguration.cs

@@ -103,7 +103,7 @@ public string ClientVersion
         public LogCallback LoggingCallback { get; internal set; }
         public IIdentityLogger IdentityLogger { get; internal set; }
         public string Component { get; internal set; }
-        public IDictionary<string, string> ExtraQueryParameters { get; internal set; } = new Dictionary<string, string>();
+        public IDictionary<string, string> ExtraQueryParameters { get; internal set; }
         public bool UseRecommendedDefaultRedirectUri { get; internal set; }
 
         public bool ExperimentalFeaturesEnabled { get; set; } = false;

src/client/Microsoft.Identity.Client/Internal/Requests/AuthenticationRequestParameters.cs

@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Security.Cryptography.X509Certificates;
 using System.Text;
 using System.Threading.Tasks;
@@ -45,9 +46,18 @@ public AuthenticationRequestParameters(
             RedirectUri = new Uri(serviceBundle.Config.RedirectUri);
             AuthorityManager = new AuthorityManager(RequestContext, initialAuthority);
 
-            // Set application wide query parameters.
-            ExtraQueryParameters = serviceBundle.Config.ExtraQueryParameters ??
-                new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
+            // it is important to copy the values from the config to the request, so that the request can be modified
+            // https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/issues/5108 
+            if (serviceBundle.Config.ExtraQueryParameters is null)
+            {
+                ExtraQueryParameters = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
+            }
+            else
+            {
+                ExtraQueryParameters = new Dictionary<string, string>(
+                    serviceBundle.Config.ExtraQueryParameters, 
+                    StringComparer.OrdinalIgnoreCase);
+            }
 
             // Copy in call-specific query parameters.
             if (commonParameters.ExtraQueryParameters != null)

src/client/Microsoft.Identity.Client/Internal/Requests/Interactive/InteractiveRequest.cs

@@ -123,12 +123,12 @@ private async Task<MsalTokenResponse> GetTokenResponseAsync(CancellationToken ca
             if (_requestParams.AppConfig.MultiCloudSupportEnabled)
             {
                 _logger.Info("Instance Aware was configured.");
-                _requestParams.AppConfig.ExtraQueryParameters[InstanceAwareParam] = "true";
+                _requestParams.ExtraQueryParameters[InstanceAwareParam] = "true";
             }
 
             // Aligh with MSAL CPP: https://github.com/AzureAD/microsoft-authentication-library-for-cpp/blob/4fa774db2b38cdce9c9b94e3e686ab53fc24b948/source/xplat/requests/WebRequestManager.cpp#L566
             // Always add 'haschrome=1' in the protocol to avoid unexpected back buttons on the first page.
-            _requestParams.AppConfig.ExtraQueryParameters["haschrome"] = "1";
+            _requestParams.ExtraQueryParameters["haschrome"] = "1";
 
             IAuthCodeRequestComponent authorizationFetcher =
                 _authCodeRequestComponentOverride ??

tests/Microsoft.Identity.Test.Unit/Helpers/ParallelRequestMockHandler.cs

@@ -0,0 +1,108 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Net.Http;
+using System.Security.Cryptography.X509Certificates;
+using System.Threading;
+using System.Threading.Tasks;
+using Microsoft.Identity.Client.Core;
+using Microsoft.Identity.Client.Http;
+using Microsoft.Identity.Client.OAuth2;
+using Microsoft.Identity.Test.Common.Core.Mocks;
+using Microsoft.Identity.Test.Unit.RequestsTests;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+namespace Microsoft.Identity.Test.Unit.Helpers
+{
+    /// <summary>
+    /// This custom HttpManager does the following: 
+    /// - responds to instance discovery calls
+    /// - responds with valid token response based on a naming convention (uid = "uid" + rtSecret, upn = "user_" + rtSecret) for "refresh_token" flow
+    /// - responds with valid app token response for client_credentials flow.    
+    /// </summary>
+    internal class ParallelRequestMockHandler : IHttpManager
+    {
+        public long LastRequestDurationInMs => 50;
+
+        public async Task<HttpResponse> SendRequestAsync(
+            Uri endpoint,
+            IDictionary<string, string> headers,
+            HttpContent body,
+            HttpMethod method,
+            ILoggerAdapter logger,
+            bool doNotThrow,
+            X509Certificate2 mtlsCertificate,
+            HttpClient customHttpClient,
+            CancellationToken cancellationToken,
+            int retryCount = 0)
+        {
+            // simulate delay and also add complexity due to thread context switch
+            await Task.Delay(ParallelRequestsTests.NetworkAccessPenaltyMs).ConfigureAwait(false);
+
+            if (HttpMethod.Get == method &&
+                endpoint.AbsoluteUri.StartsWith("https://login.microsoftonline.com/common/discovery/instance?api-version=1.1"))
+            {
+                return new HttpResponse()
+                {
+                    Body = TestConstants.DiscoveryJsonResponse,
+                    StatusCode = System.Net.HttpStatusCode.OK
+                };
+            }
+
+            if (HttpMethod.Post == method &&
+                UriWithoutQuery(endpoint).AbsoluteUri.Equals("https://login.microsoftonline.com/my-utid/oauth2/v2.0/token"))
+            {
+                var bodyString = await (body as FormUrlEncodedContent).ReadAsStringAsync().ConfigureAwait(false);
+                var bodyDict = bodyString.Replace("?", "").Split('&').ToDictionary(x => x.Split('=')[0], x => x.Split('=')[1]);
+
+                if (bodyDict["grant_type"] == "refresh_token")
+                {
+                    bodyDict.TryGetValue(OAuth2Parameter.RefreshToken, out string rtSecret);
+
+                    return new HttpResponse()
+                    {
+                        Body = GetTokenResponseForRt(rtSecret),
+                        StatusCode = System.Net.HttpStatusCode.OK
+                    };
+                }
+
+                if (bodyDict["grant_type"] == "client_credentials")
+                {
+                    HttpResponseMessage response = MockHelpers.CreateSuccessfulClientCredentialTokenResponseMessage();
+                    string payload = await response.Content.ReadAsStringAsync().ConfigureAwait(false);
+
+                    return new HttpResponse()
+                    {
+                        Body = payload,
+                        StatusCode = System.Net.HttpStatusCode.OK
+                    };
+                }
+            }
+
+            Assert.Fail("Test issue - this HttpRequest is not mocked");
+            return null;
+        }
+
+        private Uri UriWithoutQuery(Uri uri)
+        {
+            return new UriBuilder(uri) { Query = string.Empty }.Uri;
+        }
+
+        private string GetTokenResponseForRt(string rtSecret)
+        {
+            if (int.TryParse(rtSecret, out int i))
+            {
+                var upn = ParallelRequestsTests.GetUpn(i);
+                var uid = ParallelRequestsTests.GetUid(i);
+                HttpResponseMessage response = MockHelpers.CreateSuccessTokenResponseMessageWithUid(uid, TestConstants.Utid, upn);
+                return response.Content.ReadAsStringAsync().Result;
+            }
+
+            Assert.Fail("Expecting the rt secret to be a number, to be able to craft a response");
+            return null;
+        }
+    }
+}

tests/Microsoft.Identity.Test.Unit/ParallelRequestsTests.cs

@@ -17,6 +17,7 @@
 using Microsoft.Identity.Client.OAuth2;
 using Microsoft.Identity.Test.Common.Core.Helpers;
 using Microsoft.Identity.Test.Common.Core.Mocks;
+using Microsoft.Identity.Test.Unit.Helpers;
 using Microsoft.VisualStudio.TestTools.UnitTesting;
 using NSubstitute.Core;
 
@@ -41,6 +42,49 @@ public override void TestInitialize()
             _inMemoryCache = "{}";
         }
 
+        // regression test for  https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/issues/5108
+        [TestMethod]
+        public async Task ExtraQP()
+        {
+            Dictionary<string, string> extraQp = new()
+              {
+                  { "key1", "1" },
+                  { "key2", "2" }
+              };
+
+            // Arrange
+            const int NumberOfRequests = 20;
+
+            ParallelRequestMockHandler httpManager = new ParallelRequestMockHandler();
+
+            var cca = ConfidentialClientApplicationBuilder
+                .Create(TestConstants.ClientId)
+                .WithAuthority(TestConstants.AuthorityUtidTenant, true)
+                .WithClientSecret(TestConstants.ClientSecret)
+                .WithHttpManager(httpManager)
+                .Build();
+
+            var tasks = new List<Task<AuthenticationResult>>();
+
+            for (int i = 0; i < NumberOfRequests; i++)
+            {
+                tasks.Add(Task.Run(async () =>
+                {
+                    return await cca.AcquireTokenForClient(TestConstants.s_scope)
+                        .WithExtraQueryParameters(extraQp)
+                        .ExecuteAsync()
+                        .ConfigureAwait(false);
+
+                }));
+            }
+
+            // Wait for all tasks to complete
+            AuthenticationResult[] results = await Task.WhenAll(tasks).ConfigureAwait(false);
+
+            // Assert
+            Assert.AreEqual(NumberOfRequests, results.Length);
+        }
+
         [TestMethod]
         public async Task AcquireTokenSilent_ValidATs_ParallelRequests_Async()
         {
@@ -116,7 +160,7 @@ public async Task AcquireTokenSilent_ExpiredATs_ParallelRequests_Async()
             const int NumberOfRequests = 10;
 
             // The typical HttpMockHandler used by other tests can't deal with parallel request
-            ParallelRequestMockHanler httpManager = new ParallelRequestMockHanler();
+            ParallelRequestMockHandler httpManager = new ParallelRequestMockHandler();
 
             PublicClientApplication pca = PublicClientApplicationBuilder
                 .Create(TestConstants.ClientId)
@@ -228,97 +272,4 @@ private void ConfigureCacheSerialization(IPublicClientApplication pca)
             });
         }
     }
-
-    /// <summary>
-    /// This custom HttpManager does the following: 
-    /// - provides a standard response for discovery calls
-    /// - responds with valid tokens based on a naming convention (uid = "uid" + rtSecret, upn = "user_" + rtSecret)
-    /// </summary>
-    internal class ParallelRequestMockHanler : IHttpManager
-    {
-        public long LastRequestDurationInMs => 50;
-
-        public Task<HttpResponse> SendGetAsync(Uri endpoint, IDictionary<string, string> headers, ILoggerAdapter logger, bool retry = true, CancellationToken cancellationToken = default)
-        {
-            Assert.Fail("Only instance discovery is supported");
-            return Task.FromResult<HttpResponse>(null);
-        }
-
-        public async Task<HttpResponse> SendPostAsync(Uri endpoint, IDictionary<string, string> headers, IDictionary<string, string> bodyParameters, ILoggerAdapter logger, CancellationToken cancellationToken = default)
-        {
-            await Task.Delay(ParallelRequestsTests.NetworkAccessPenaltyMs).ConfigureAwait(false);
-
-            if (endpoint.AbsoluteUri.Equals("https://login.microsoftonline.com/my-utid/oauth2/v2.0/token"))
-            {
-                bodyParameters.TryGetValue(OAuth2Parameter.RefreshToken, out string rtSecret);
-
-                return new HttpResponse()
-                {
-                    Body = GetTokenResponseForRt(rtSecret),
-                    StatusCode = System.Net.HttpStatusCode.OK
-                };
-            }
-
-            Assert.Fail("Only refresh flow is supported");
-            return null;
-        }
-
-        public async Task<HttpResponse> SendRequestAsync(
-            Uri endpoint,
-            IDictionary<string, string> headers,
-            HttpContent body,
-            HttpMethod method,
-            ILoggerAdapter logger,
-            bool doNotThrow,
-            X509Certificate2 mtlsCertificate,
-            HttpClient customHttpClient,
-            CancellationToken cancellationToken, 
-            int retryCount = 0)
-        {
-            // simulate delay and also add complexity due to thread context switch
-            await Task.Delay(ParallelRequestsTests.NetworkAccessPenaltyMs).ConfigureAwait(false);
-
-            if (HttpMethod.Get == method &&
-                endpoint.AbsoluteUri.StartsWith("https://login.microsoftonline.com/common/discovery/instance?api-version=1.1"))
-            {
-                return new HttpResponse()
-                {
-                    Body = TestConstants.DiscoveryJsonResponse,
-                    StatusCode = System.Net.HttpStatusCode.OK
-                };
-            }
-
-            if (HttpMethod.Post == method &&
-                endpoint.AbsoluteUri.Equals("https://login.microsoftonline.com/my-utid/oauth2/v2.0/token"))
-            {
-                var bodyString = (body as FormUrlEncodedContent).ReadAsStringAsync().GetAwaiter().GetResult();
-                var bodyDict = bodyString.Replace("?", "").Split('&').ToDictionary(x => x.Split('=')[0], x => x.Split('=')[1]);
-
-                bodyDict.TryGetValue(OAuth2Parameter.RefreshToken, out string rtSecret);
-
-                return new HttpResponse()
-                {
-                    Body = GetTokenResponseForRt(rtSecret),
-                    StatusCode = System.Net.HttpStatusCode.OK
-                };
-            }
-
-            Assert.Fail("Test issue - this HttpRequest is not mocked");
-            return null;
-        }
-
-        private string GetTokenResponseForRt(string rtSecret)
-        {
-            if (int.TryParse(rtSecret, out int i))
-            {
-                var upn = ParallelRequestsTests.GetUpn(i);
-                var uid = ParallelRequestsTests.GetUid(i);
-                HttpResponseMessage response = MockHelpers.CreateSuccessTokenResponseMessageWithUid(uid, TestConstants.Utid, upn);
-                return response.Content.ReadAsStringAsync().Result;
-            }
-
-            Assert.Fail("Expecting the rt secret to be a number, to be able to craft a response");
-            return null;
-        }
-    }
 }