-
Notifications
You must be signed in to change notification settings - Fork 337
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
Disable ADAL cache #2309
Disable ADAL cache #2309
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left a few suggestions.
(jus so that we remember): We need to understand (and demonstrate) the impact on the https://github.com/Azure-Samples/active-directory-dotnet-v1-to-v2/tree/master/TokenCacheMigration sample
src/client/Microsoft.Identity.Client/AppConfig/AbstractApplicationBuilder.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/AppConfig/AbstractApplicationBuilder.cs
Outdated
Show resolved
Hide resolved
Looks good so far, I think you should look at the auto-detection separately, as it is out of scope for this work item. Unit tests + a perf tests would be a better next step. |
@pmaytak : could we please have an estimate of the performance gain (before the fix, and after the idea of the fix) ? |
@bgavrilMS @jmprieur Thanks for the suggestions; I'll work on a perf assessment. |
@henrik-me @jmprieur @bgavrilMS @jennyf19 |
2f9831e
to
7883d6b
Compare
@@ -530,57 +536,6 @@ public void DoNotWriteFRTs() | |||
AssertCacheEntryCount(0); | |||
} | |||
|
|||
private void PopulateLegacyCache(ILegacyCachePersistence legacyCachePersistence) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: Just moved these helper methods to LegacyTokenCacheHelper for ease of access.
Ready for review for the first two bullet points. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pmaytak
I've proposed an improvement for the XML comment of the new API.
src/client/Microsoft.Identity.Client/AppConfig/AbstractApplicationBuilder.cs
Show resolved
Hide resolved
Is there a reason to have this exposed only in the confidential client application options? As it's more of a confidential client issue. Also, i'm a little concerned about discoverability of this, and it can only be used w/MSAL only applications. Any applications that might share w/ADAL should not use this, would there be a risk of losing the cache? Do we have an internal partner or customer that we can have try this out? From my understanding, first party customers tend to try in MSAL first, and then fall back to ADAL if the call fails, so they might have to use the ADAL cache. Do we have a customer who has complained about the ADAL cache lookup? side note, not a fan of having "ADAL" in the MSAL public API, just like we were told not to have B2C, ADAL seems even worse as it's being deprecated. As a new customer, I wouldn't understand why i would need this or not (what is ADAL?). Maybe auto-detection is better? In reply to: 754546278 [](ancestors = 754546278) |
@henrik-me @bgavrilMS What's you opinion on Jenny's 3 points above? As far as the renaming, I could change it to |
@peter did you explore the impact on the cache compat tests JM pointed out? The concern is if we disable this functionality by default how many are going to be impacted, if it's opt in then none. I was considering we should auto detect to opt. out of this, however people should be able to opt. out of disabling. The only otherway to validate this would be to start adding telemetry and let us measure how many really still need this. I would think that there might be a few for public client applications in migration scenarios (thus though could have a way to opt out of the behavior change in case our auto detection doesn't work) but I don't see a need for this in confidential client applications. Typically the fallback to ADAL there is to a completely different cache as it's a completely different library. I think this change is safe on confidential client, and that is also where we need it the most. We can start by making this confidential client only. In reply to: 755112571 [](ancestors = 755112571) |
@pmaytak : having discussed the overall impact of implementing auto detection with @jmprieur, we suggest to not implement that at all. this feature should be opt. in and we will find other ways to encourage customers to opt.in. This means regardless of scenario it should only be disabled if a customer has opted in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…L cache operations.
b1fca1b
to
1381c11
Compare
Oh wow, I just saw the numbers. These are super impressive. We should really disable ADAL by default. |
My thought for automatic detection of ADAL usage is:
|
Issue #1770