[v5] Changes to Configuration - CacheOptions (Config #2) #7697
Conversation
github-actions
bot
added
documentation
jo-arroyo
changed the title
jo-arroyo
requested review from
sameerag,
tnorling,
hectormmg,
peterzenz,
Robbie-Microsoft,
konstantin-msft,
lalimasharda and
shylasummers
as code owners
…tication-library-for-js into config-change-cache
…tication-library-for-js into config-change-cache
…tication-library-for-js into config-change-cache
…tication-library-for-js into config-change-cache
| @@ -724,7 +720,7 @@ describe("BrowserCacheManager tests", () => { | |||
| ).toBe(CredentialType.ACCESS_TOKEN_WITH_AUTH_SCHEME); | |||
| }); | |||
|
|
|||
| it("clearTokensWithClaimsInCache clears all access tokens with claims in tokenKeys", async () => { | |||
| it("clearTokensAndKeysWithClaims clears all access tokens with claims in tokenKeys", async () => { | |||
There was a problem hiding this comment.
Fixed naming, clearTokensWithClaimsInCache is not a function
| @@ -99,19 +99,6 @@ describe("In Memory Storage Tests", function () { | |||
| await page.close(); | |||
| }); | |||
|
|
|||
| it("Performs loginRedirect", async () => { | |||
There was a problem hiding this comment.
This e2e test has been removed as loginRedirect is not supported with memory storage now that storeAuthStateInCookies is removed.
| if ( | ||
| config.cache.claimsBasedCachingEnabled && |
There was a problem hiding this comment.
Doesn't this change the behavior here? We are effectively defaulting this option to false so this block would never be hit. Do I have that right?
There was a problem hiding this comment.
We're removing claimsBasedCachingEnabled so this block would be hit as long as request.claims is set. In my discussion with @sameerag, the consensus was that claims still need to be able to be set on request for CAE purposes but we never want to cache claims.
There was a problem hiding this comment.
I guess I'm confused how this works today then when claimsBasedCachingEnabled is false? Do we have a bug today?
There was a problem hiding this comment.
It works the way @jo-arroyo described above, claims are added to the request but never cached. The bug is if this is true and user calls claims per request with a high frequency, the cache starts hitting its limit and requests fail post that point.
There was a problem hiding this comment.
Let's chat offline, I think we're conflating 2 related but different things and I think this specific change is incorrect.
| */ | ||
| claimsBasedCachingEnabled?: boolean; | ||
| }; | ||
| export type CacheOptions = {}; |
There was a problem hiding this comment.
Do we want to remove this altogether since it's empty?
There was a problem hiding this comment.
Good point, done.
Co-authored-by: Thomas Norling <[email protected]>
This PR updates the CacheOptions for MSAL Browser v5, including the removal of implementation, tests, and doc references for the following options:
temporaryCacheLocationclaimsBasedCachingEnabledstoreAuthStateInCookiesecureCookiescacheMigrationEnabledDeprecation notices for the above options were added in #7707