[Bug] CacheSynchronizationEnabled flag default is inconsistent

[bgavrilMS]

MSAL 4.54.1

Expected

CacheSynchronizationEnabled should be true by default. It should have negligible impact over when CCA is constructed on a request by request basis. But when CCA is singleton, cache correctness cannot be achieved without this flag set to true (i.e. without locking).

(It should also be hidden / soft deprecated API, as it is too low level)

Actual

If using ApplicationOptions when constructing the CCA object, the CacheSynchronizationEnabled is set to false if not explicitly declared.
If not using ApplicationsOptions, the CacheSynchronizationEnabled is set to true

[bgavrilMS]

CC @jennyf19 and @jmprieur as this affects Id.Web since afaik it constructs the CCA from options all the time.

To minimize churn, is it possible to set this flag to false when you create a CCA per request and to true when in singleton mode? That way MSAL can change its default as it sees fit.

[jmprieur]

Yes, we might be able to do that in Id.Web.
However what about the customers not using Id.Web? isn't that a problem

[pmaytak]

But when CCA is singleton, cache correctness cannot be achieved without this flag set to true (i.e. without locking).

Nit clarification here - CCA is singleton and token cache serialization is enabled.

So the effects of changing default from false to true:

#	Scenario	Effect
1	CCA is per request w/ serialized cache	Negligible
2	Singleton CCA, no serialized cache	Not recommended, should not be used in prod.
3	Singleton CCA w/ serialized cache	Fixes cache inconsistency

We don't have telemetry how often 2 is used in prod.

I also wouldn't obsolete this in case people want to set the flag to false in scenario 2 or 1. I would just make sure the method comment is clear that defaults should be used.

[pmaytak]

So the flag is consistent - it's false by default.

microsoft-authentication-library-for-dotnet/tests/Microsoft.Identity.Test.Unit/AppConfigTests/ConfidentialClientApplicationBuilderTests.cs

Lines 103 to 109 in 80c848b

	var options = new ConfidentialClientApplicationOptions()
	{
	ClientSecret = "secret",
	ClientId = TestConstants.ClientId,
	};
	var app = ConfidentialClientApplicationBuilder.CreateWithApplicationOptions(options).Build();
	Assert.IsFalse((app.AppConfig as ApplicationConfiguration).CacheSynchronizationEnabled);

microsoft-authentication-library-for-dotnet/tests/Microsoft.Identity.Test.Unit/AppConfigTests/ConfidentialClientApplicationBuilderTests.cs

Lines 130 to 131 in 80c848b

	app = ConfidentialClientApplicationBuilder.Create(Guid.NewGuid().ToString()).WithClientSecret(TestConstants.ClientSecret).BuildConcrete();
	Assert.IsFalse((app.AppConfig as ApplicationConfiguration).CacheSynchronizationEnabled);

Although it is set to true in app config; it's actually overridden in the CcaBuilder.Create:

microsoft-authentication-library-for-dotnet/src/client/Microsoft.Identity.Client/AppConfig/ApplicationConfiguration.cs

Line 116 in 80c848b

public bool CacheSynchronizationEnabled { get; internal set; } = true;

microsoft-authentication-library-for-dotnet/src/client/Microsoft.Identity.Client/AppConfig/ConfidentialClientApplicationBuilder.cs

Line 86 in 80c848b

.WithCacheSynchronization(false);