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
Fix MSAL cache cleaning method #675
Conversation
few questions / comments but looks good to me |
// Cache account and authority | ||
this.setAccountCache(account, serverAuthenticationRequest.state); | ||
this.setAuthorityCache(serverAuthenticationRequest.state, acquireTokenAuthority.CanonicalAuthority); | ||
this.updateAcquireTokenCacheEntries(serverAuthenticationRequest, account); |
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.
@pkanher617 As discussed you may want to combine the login calls caching code for "authority" and "nonce" also into this API. Please factor that login calls do not have "acquireTokenAccountKey".
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.
added to updateCacheEntries. Please check new code.
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.
Looks good except the one comment.
|
||
// Cache account and authority | ||
this.setAccountCache(account, serverAuthenticationRequest.state); | ||
this.setAuthorityCache(serverAuthenticationRequest.state, acquireTokenAuthority.CanonicalAuthority); |
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.
it's canonicalAuthority here, however in the updateCacheEntries is using authority
|
||
// Cache account and authority | ||
this.setAccountCache(account, serverAuthenticationRequest.state); | ||
this.setAuthorityCache(serverAuthenticationRequest.state, acquireTokenAuthority.CanonicalAuthority); |
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.
CanonicalAuthority - same as the other comment
lgtm approved |
This PR is a fix for issue #527 where cache keys are not being cleared out.
A previous PR (#544) was submitted for this, but was failing some use cases. This fix works for manually tested use cases and passes all unit tests.