Cache improvements #708
Cache improvements #708
Conversation
| /** | ||
| * List of possible reasons the tokens in an {@link IAuthenticationResult} were refreshed. | ||
| */ | ||
| public enum CacheRefreshReason { |
There was a problem hiding this comment.
You are missing a case here when "Claims" are specified. When this happens, MSAL has to bypass the token cache and to ESTS.
But not for client capabilities. Client capabilities are internally still claims. But we should not bypass the cache.
There was a problem hiding this comment.
| /** | ||
| * A default value, likely indicates tokens could not be retrieved | ||
| */ | ||
| UNKNOWN, |
There was a problem hiding this comment.
MSAL throws exception when token cannot be retried. I don't think you can get this value. Also, there is no unit test covering this path.
msal4j-sdk/src/integrationtest/java/com.microsoft.aad.msal4j/TokenCacheIT.java
Show resolved
Hide resolved
| assertEquals(TokenSource.IDENTITY_PROVIDER, result.metadata().tokenSource()); | ||
| assertEquals(CacheRefreshReason.NOT_APPLICABLE, result.metadata().cacheRefreshReason()); | ||
| assertEquals(TokenSource.CACHE, acquireSilentResult.metadata().tokenSource()); | ||
| assertEquals(CacheRefreshReason.NOT_APPLICABLE, acquireSilentResult.metadata().cacheRefreshReason()); |
There was a problem hiding this comment.
Please add more tests. Consider adding these assertions to all integrationt tests, not just public client ones. Client_credentials is of highest priority.
In particular:
-
client_credentials + all combos of CacheRefreshReason (especially PROACTIVE_REFRESH)
-
client_credentials + claims
-
obo and auth code
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AcquireTokenSilentSupplier.java
Show resolved
Hide resolved
| //Because the confidential client flow has an internal silent call: | ||
| // -result_notStatic1 and result_notStatic2 should be the same, because they both used the non-static cache from one ConfidentialClientApplication instance | ||
| // -result_sharedCache1 and result_sharedCache2 should be the same, because they both used the static cache shared between two ConfidentialClientApplication instances | ||
| assertEquals(result_notStatic1.accessToken(), result_notStatic2.accessToken()); |
There was a problem hiding this comment.
Since you added token source in the metadata, you can also check the token source as cache or IDP
| @@ -208,6 +215,12 @@ public ConfidentialClientApplication.Builder appTokenProvider(Function<AppTokenP | |||
| throw new NullPointerException("appTokenProvider is null") ; | |||
| } | |||
|
|
|||
| public ConfidentialClientApplication.Builder useSharedCache(boolean val) { | |||
There was a problem hiding this comment.
The user experience in MSAL.net is enable this through cache options. Are we not planning to be consistent with it?
| /** | ||
| * List of possible reasons the tokens in an {@link IAuthenticationResult} were refreshed. | ||
| */ | ||
| public enum CacheRefreshReason { |
There was a problem hiding this comment.
I could not find where this value is being sent to server side telemetry. The expected format for that is Expected format: 5|api_id,cache_info,region_used,region_autodetection,region_outcome|platform_config where this values is sent as cache info. Also when you send it to server side telemetry it should be a numeric value as 0,1, etc. See https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/ac1cb0522a011e0a6e0802a2535bf03cfd8d9fab/src/client/Microsoft.Identity.Client/Cache/CacheRefreshReason.cs#L9
There was a problem hiding this comment.
The token source and cache refresh reason is also sent to the server side telemetry. I did not see the code to send it to server side telemetry. See HttpTelemetryManager.cs for reference.
Adds an option to use a static/shared cache in ConfidentialClientApplication instead of the per-instance cache (#699), and adds a new field to AuthenticationResult to store helpful metadata about the result (#697)