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
[msal-common] Fix caching for multiple resources #1553
Conversation
In general looks good can we add a test to validate different scope scenarios ? |
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, one nitpick.
lib/msal-common/src/auth/ScopeSet.ts
Outdated
const sizeThisScopes = this.containsScope(Constants.OFFLINE_ACCESS_SCOPE)? this.getScopeCount() - 1: this.getScopeCount(); | ||
const sizeUnionScopes = unionScopes.has(Constants.OFFLINE_ACCESS_SCOPE)? unionScopes.size - 1: unionScopes.size; | ||
|
||
return sizeUnionScopes < (sizeThisScopes + sizeOtherScopes); |
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.
Dumb q: Why are we checking count than content? What if count matches and content differs for the set? I may be missing some logic and will check. This is mostly a question I immediately had.
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.
unionScopes
is a set of scope array 1 + scope array 2 containing only unique entries. If the size of this is less than the size of both arrays added together then you know there were shared scopes between the 2 individual arrays
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.
Size of both sets, not arrays. these are all unique values.
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.
okay, got it.
…tion-library-for-js into fix-multiple-resource-bug-2.0
…ureAD/microsoft-authentication-library-for-js into fix-multiple-resource-bug-2.0
offline_access
scope was causing all scope sets to intersect, meaning all access tokens were being cached under the same entry with new scopes appended in the cache key. This PR aims to address this by ignoringoffline_access
when attempting to determine if we are adding or updating an access token in the cache.This will fix: #1487