Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Performance] Disable ADAL cache by default in Microsoft.Identity.Web #961

Closed
jmprieur opened this issue Feb 12, 2021 · 4 comments
Closed
Assignees
Milestone

Comments

@jmprieur
Copy link
Collaborator

Is your feature request related to a problem? Please describe.
MSAL maintains by default a dual cache serialization ADAL, MSAL, but this has performance improvements. See benchmark in See AzureAD/microsoft-authentication-library-for-dotnet#2309. New applications don't need the ADAL cache. And customers who are using Microsoft.Identity.Web (as opposed to those who use MSAL.NET directly) probably don't need token cache migration.

Describe the solution you'd like
MSAL.NET has now the possibility of disabling ADAL cache by specifying WithLegacyCacheCompatibility(false) when building the confidential client application, or using LegacyCacheCompatibilityEnabled = false in the ApplicationConfiguration

The proposal is to disable the ADAL cache by default in Microsoft.Identity.Web applications.

  • Option1: in the template, provide "LegacyCacheCompatibilityEnabled": "false" in the appsettings.json files. This is the safest, but won't have an impact on applications that are already created
  • Option2: have a new LegacyCacheCompatibilityEnabled property in MIcrosoftIdentityOptions set to false by default, but overridable by configuration, which would feed the ConfidentialClientApplicationOptions (like the ClientID etc ... does). This way, by default any app has the ADAL cache disabled, but customers can enable it if they want.

Alternatives thought of

  • Option 3: Just use the ConfidentialClientApplicationOptions.LegacyCacheCompatibilityEnabled, but it's true by default, and therefore this requires proactive work from customers to disable it (which they might not find)
  • Option 4: Always disable the ADAL token cache programmatically when creating the ConfidentialClientApplication. This does not allow customers who'd want the cache migration though.

Additional context
See AzureAD/microsoft-authentication-library-for-dotnet#2309 (comment)

@henrik-me
Copy link
Contributor

Option 2 gets my vote, should be like this out of the box. Call it out in the release and make sure there is an entry in the logs saying legacy caching disabled (or similar).

@jmprieur jmprieur modified the milestones: 1.6.1, 1.7.0, 1.8.0 Feb 13, 2021
@jmprieur jmprieur modified the milestones: 1.8.0, 1.7.0 Feb 23, 2021
@jmprieur jmprieur added the P1 label Feb 23, 2021
@jennyf19
Copy link
Collaborator

non-issue as we only serialize the msal cache.

@jennyf19 jennyf19 added by design and removed P1 labels Feb 23, 2021
@jmprieur jmprieur reopened this Feb 23, 2021
@jmprieur
Copy link
Collaborator Author

Reopening for the performance impact of processing things even when the ADAL cache is not serialized.

@jmprieur jmprieur added the fixed label Feb 24, 2021
@jennyf19
Copy link
Collaborator

Included in 1.7.0 release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants