ClientCredentials: Fixed bug where user-supplied cache is loaded into memory only after network request

[Robbie-Microsoft]

While working on the PR Extend proactive token refresh to client_credentials, I discovered a bug while manually testing the fix.

A user-supplied cache (provided as cache: { cachePlugin } in the configuration) is not loaded into memory until after a network request, inside of handleServerTokenResponse, inside of the ResponseHandler.

We want to load the user-defined cache (if applicable) into memory BEFORE checking the in-memory-cache for existing tokens.

Without this fix, when a token is acquired in the client_credentials flow, fromCache will always be set to false, and a request will be sent over the network for a token.

Question to others and follow-up: Does this bug exist in other flows?

Edit: looks like this PR will solve the problem described in msal-node's performance doc here. Looking at the code at the bottom of that page, the line directly above the acquireToken call: await cca.getTokenCache().getAllAccounts(); // required for cache read

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 79.46%. Comparing base (81d34b4) to head (6e6f37d).
Report is 613 commits behind head on dev.

Files with missing lines	Patch %	Lines
lib/msal-node/src/client/ClientCredentialClient.ts	0.00%	3 Missing and 2 partials ⚠️

Additional details and impacted files

Flag	Coverage Δ
msal-angular	?
msal-browser	?
msal-common	?
msal-core	?
msal-node	79.46% <0.00%> (-3.93%)	⬇️
msal-node-extensions	?
msal-react	?
node-token-validation	?

Files with missing lines	Coverage Δ
lib/msal-node/src/client/ClientCredentialClient.ts	85.71% <0.00%> (ø)

... and 235 files with indirect coverage changes

[sameerag]

Added context for CacheManager class in comments, please keep that in mind for future changes.

[derisen]

@Robbie-Microsoft @bgavrilMS @sameerag this issue pertains to other flows as well (especially public client auth code flow in relation to msal-node-extensions). It was being tracked here: #5502

For CCA, there's a non-trivial cost to hydrating the in memory cache with the persistent cache i.e. if the persistence layer is hosted on cloud or similar, each check will involve a network call (couple 100ms worst case -consider latency differences between a redis cache vs a sql db). It might be fine for client credentials but it could be a bottleneck for a resource server using OBO.

Essentially the question is how often the in-memo cache should be hydrated before a token request, given that the in-memo cache is the single source of truth for msal. "Always" is probably the best wrt cache coherence in a horizontal scaling scenario, but worth discussing what other options might be viable for devs optimizing for perf. My 2 cents: perhaps it makes sense to have a cache policy configuration parameter for the cache field in msal-node configuration, with options like "check in-memo cache first, then persistence if needed", "check persistence first, then in-memo cache" and etc..

[pmaytak]

For CCA, there's a non-trivial cost to hydrating the in memory cache with the persistent cache i.e. if the persistence layer is hosted on cloud or similar, each check will involve a network call (couple 100ms worst case -consider latency differences between a redis cache vs a sql db). It might be fine for client credentials but it could be a bottleneck for a resource server using OBO.

Essentially the question is how often the in-memo cache should be checked before a token request, given that the in-memo cache is the single source of truth for msal. "Always" is probably the best wrt cache coherence in a horizontal scaling scenario, but worth discussing what other options might be viable for devs optimizing for perf. My 2 cents: perhaps it makes sense to have a cache policy configuration parameter for the cache field in msal-node configuration, with options like "check in-memo cache first, then persistence if needed", "check persistence first, then in-memo cache" and etc..

I would expect it to be something like - always check in-memory cache first, if not empty, return data, but if it's empty, check persistence cache. The issue now is if new CCA is created per request, the in-memory cache will always be empty (unless that workaround is used and getAccounts is called first),

[derisen]

I would expect it to be something like - always check in-memory cache first, if not empty, return data, but if it's empty, check persistence cache. The issue now is if new CCA is created per request, the in-memory cache will always be empty (unless that workaround is used and getAccounts is called first),

@pmaytak agreed, that would be the general approach (for PCA and CCA), but as you said, CCA will typically be a fresh instance and so, empty in-memory cache..