Set CacheSynchronization to true by default (#4310)

* Change EnableCacheSynchronization default. * Update tests. * Update method comment. * Update ConfidentialClientApplicationBuilder.cs * Update ConfidentialClientApplicationOptions.cs --------- Co-authored-by: Gladwin Johnson <[email protected]>
AzureAD · Aug 30, 2023 · d25f45f · d25f45f · github-actions · Aug 30, 2023
1 parent 0ea350f
commit d25f45f
Show file tree

Hide file tree

Showing 4 changed files with 41 additions and 45 deletions.
diff --git a/src/client/Microsoft.Identity.Client/AppConfig/ConfidentialClientApplicationBuilder.cs b/src/client/Microsoft.Identity.Client/AppConfig/ConfidentialClientApplicationBuilder.cs
@@ -82,8 +82,7 @@ public static ConfidentialClientApplicationBuilder Create(string clientId)
 
             var config = new ApplicationConfiguration(MsalClientType.ConfidentialClient);
             return new ConfidentialClientApplicationBuilder(config)
-                .WithClientId(clientId)
-                .WithCacheSynchronization(false);
+                .WithClientId(clientId);
         }
 
         /// <summary>
@@ -303,12 +302,12 @@ public ConfidentialClientApplicationBuilder WithAzureRegion(string azureRegion =
         /// <summary>
         /// When set to <c>true</c>, MSAL will lock cache access at the <see cref="ConfidentialClientApplication"/> level, i.e.
         /// the block of code between BeforeAccessAsync and AfterAccessAsync callbacks will be synchronized. 
-        /// Apps can set this flag to <c>false</c> to enable an optimistic cache locking strategy, which may result in better performance, especially 
-        /// when ConfidentialClientApplication objects are reused.
+        /// Apps can set this flag to <c>false</c> to enable an optimistic cache locking strategy, which may result in better performance
+        /// at the cost of cache consistency. 
+        /// Setting this flag to <c>false</c> is only recommended for apps which create a new <see cref="ConfidentialClientApplication"/> per request.
         /// </summary>
         /// <remarks>
-        /// False by default.
-        /// Not recommended for apps that call RemoveAsync
+        /// This flag is <c>true</c> by default. The default behavior is recommended.
         /// </remarks>
         public ConfidentialClientApplicationBuilder WithCacheSynchronization(bool enableCacheSynchronization)
         {

diff --git a/src/client/Microsoft.Identity.Client/AppConfig/ConfidentialClientApplicationOptions.cs b/src/client/Microsoft.Identity.Client/AppConfig/ConfidentialClientApplicationOptions.cs
@@ -36,13 +36,13 @@ public class ConfidentialClientApplicationOptions : ApplicationOptions
         /// <summary>
         /// When set to <c>true</c>, MSAL will lock cache access at the <see cref="ConfidentialClientApplication"/> level, i.e.
         /// the block of code between BeforeAccessAsync and AfterAccessAsync callbacks will be synchronized. 
-        /// Apps can set this flag to <c>false</c> to enable an optimistic cache locking strategy, which may result in better performance, especially 
-        /// when ConfidentialClientApplication objects are reused.
+        /// Apps can set this flag to <c>false</c> to enable an optimistic cache locking strategy, which may result in better performance
+        /// at the cost of cache consistency. 
+        /// Setting this flag to <c>false</c> is only recommended for apps which create a new <see cref="ConfidentialClientApplication"/> per request.
         /// </summary>
         /// <remarks>
-        /// False by default.
-        /// Not recommended for apps that call RemoveAsync
+        /// This flag is <c>true</c> by default. The default behavior is recommended.
         /// </remarks>
-        public bool EnableCacheSynchronization { get; set; } = false;
+        public bool EnableCacheSynchronization { get; set; } = true;
     }
 }
diff --git a/.../Microsoft.Identity.Test.Unit/AppConfigTests/ConfidentialClientApplicationBuilderTests.cs b/.../Microsoft.Identity.Test.Unit/AppConfigTests/ConfidentialClientApplicationBuilderTests.cs
@@ -97,58 +97,41 @@ public void TestBuildWithInstanceWithTrailingSlash()
         }
 
         [TestMethod]
-        public void CacheSynchronizationWithDefaultCCA()
+        public void CacheSynchronization_Default_IsTrue()
         {
-            //Validate CacheSynchronizationEnabled when app is created from ApplicaitonOptions for CCA
-            var options = new ConfidentialClientApplicationOptions()
+            var ccaOptions = new ConfidentialClientApplicationOptions()
             {
                 ClientSecret = "secret",
                 ClientId = TestConstants.ClientId,
             };
-            var app = ConfidentialClientApplicationBuilder.CreateWithApplicationOptions(options).Build();
-            Assert.IsFalse((app.AppConfig as ApplicationConfiguration).CacheSynchronizationEnabled);
+            var cca = ConfidentialClientApplicationBuilder.CreateWithApplicationOptions(ccaOptions).Build();
+            Assert.IsTrue((cca.AppConfig as ApplicationConfiguration).CacheSynchronizationEnabled);
 
-            options = new ConfidentialClientApplicationOptions
-            {
-                ClientId = TestConstants.ClientId,
-                ClientSecret = "secret",
-                EnableCacheSynchronization = false
-            };
-            app = ConfidentialClientApplicationBuilder.CreateWithApplicationOptions(options).Build();
-            Assert.AreEqual(false, (app.AppConfig as ApplicationConfiguration).CacheSynchronizationEnabled);
+            cca = ConfidentialClientApplicationBuilder.Create(Guid.NewGuid().ToString()).WithClientSecret(TestConstants.ClientSecret).Build();
+            Assert.IsTrue((cca.AppConfig as ApplicationConfiguration).CacheSynchronizationEnabled);
+        }
 
-            options = new ConfidentialClientApplicationOptions
+        [DataTestMethod]
+        [DataRow(false)]
+        [DataRow(true)]
+        public void CacheSynchronization_WithOptions(bool enableCacheSynchronization)
+        {
+            var ccaOptions = new ConfidentialClientApplicationOptions
             {
                 ClientId = TestConstants.ClientId,
                 ClientSecret = "secret",
-                EnableCacheSynchronization = true
-            };
-            app = ConfidentialClientApplicationBuilder.CreateWithApplicationOptions(options).Build();
-            Assert.AreEqual(true, (app.AppConfig as ApplicationConfiguration).CacheSynchronizationEnabled);
-
-            //Validate CacheSynchronizationEnabled is false by default when app is created from ConfidentialClientApplicationBuilder
-            app = ConfidentialClientApplicationBuilder.Create(Guid.NewGuid().ToString()).WithClientSecret(TestConstants.ClientSecret).BuildConcrete();
-            Assert.IsFalse((app.AppConfig as ApplicationConfiguration).CacheSynchronizationEnabled);
-
-            //Validate CacheSynchronizationEnabled when app is created from ApplicaitonOptions for PCA
-            var options2 = new PublicClientApplicationOptions()
-            {
-                ClientId = TestConstants.ClientId
+                EnableCacheSynchronization = enableCacheSynchronization
             };
-            var app2 = PublicClientApplicationBuilder.CreateWithApplicationOptions(options2).Build();
-            Assert.IsTrue((app2.AppConfig as ApplicationConfiguration).CacheSynchronizationEnabled);
-
-            //Validate CacheSynchronizationEnabled is true by default when app is created from PublicClientApplicationBuilder
-            app2 = PublicClientApplicationBuilder.Create(Guid.NewGuid().ToString()).BuildConcrete();
-            Assert.IsTrue((app2.AppConfig as ApplicationConfiguration).CacheSynchronizationEnabled);
+            var cca = ConfidentialClientApplicationBuilder.CreateWithApplicationOptions(ccaOptions).Build();
+            Assert.AreEqual(enableCacheSynchronization, (cca.AppConfig as ApplicationConfiguration).CacheSynchronizationEnabled);
         }
 
         [DataTestMethod]
         [DataRow(false, false, false)]
         [DataRow(true, true, true)]
         [DataRow(true, false, false)]
         [DataRow(false, true, true)]
-        public void CacheSynchronizationNoDefault(bool optionFlag, bool builderFlag, bool result)
+        public void CacheSynchronization_WithCacheSynchronization_TakesPrecedence(bool optionFlag, bool builderFlag, bool result)
         {
             var options = new ConfidentialClientApplicationOptions
             {

diff --git a/tests/Microsoft.Identity.Test.Unit/AppConfigTests/PublicClientApplicationBuilderTests.cs b/tests/Microsoft.Identity.Test.Unit/AppConfigTests/PublicClientApplicationBuilderTests.cs
@@ -622,6 +622,20 @@ public void AuthorityWithTenant()
             Assert.AreEqual($"https://login.microsoftonline.com/{TestConstants.TenantId}/", app6.Authority);
         }
 
+        [TestMethod]
+        public void CacheSynchronization_Default_IsTrue()
+        {
+            var pcaOptions = new PublicClientApplicationOptions()
+            {
+                ClientId = TestConstants.ClientId
+            };
+            var pca = PublicClientApplicationBuilder.CreateWithApplicationOptions(pcaOptions).Build();
+            Assert.IsTrue((pca.AppConfig as ApplicationConfiguration).CacheSynchronizationEnabled);
+
+            pca = PublicClientApplicationBuilder.Create(Guid.NewGuid().ToString()).Build();
+            Assert.IsTrue((pca.AppConfig as ApplicationConfiguration).CacheSynchronizationEnabled);
+        }
+
 #if NET6_WIN
         [TestMethod]
         public void IsBrokerAvailable_net6()