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
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)