Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[msal-common] Fix caching for multiple resources #1553

Merged
merged 6 commits into from
May 5, 2020
Merged

[msal-common] Fix caching for multiple resources #1553

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a4a29b9
Initial Commit
tnorling Apr 28, 2020
da8889b
Unit tests
tnorling Apr 28, 2020
2c6c4f0
Merge branch 'dev' into fix-multiple-resource-bug-2.0
tnorling Apr 29, 2020
15f7553
Address feedback
tnorling May 4, 2020
f471935
Merge branch 'dev' of https://github.com/AzureAD/microsoft-authentica…
tnorling May 5, 2020
3a9ee9a
Merge branch 'fix-multiple-resource-bug-2.0' of https://github.com/Az…
tnorling May 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 9 additions & 1 deletion lib/msal-common/src/auth/ScopeSet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,15 @@ export class ScopeSet {
if (!otherScopes) {
throw ClientAuthError.createEmptyInputScopeSetError(otherScopes);
}
return this.unionScopeSets(otherScopes).size < (this.scopes.size + otherScopes.getScopeCount());

const unionScopes = this.unionScopeSets(otherScopes);

// Do not allow offline_access to be the only intersecting scope
const sizeOtherScopes = otherScopes.containsScope(Constants.OFFLINE_ACCESS_SCOPE)? otherScopes.getScopeCount() - 1 : otherScopes.getScopeCount();
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);
Copy link
Member

@sameerag sameerag May 4, 2020 •
edited

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.

Copy link
Collaborator Author

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

Copy link
Contributor

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, got it.

}

/**
Expand Down
25 changes: 12 additions & 13 deletions lib/msal-common/src/response/ResponseHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,32 +167,31 @@ export class ResponseHandler {
// If no items in cache with these parameters, set new item.
if (accessTokenCacheItems.length < 1) {
this.logger.info("No tokens found, creating new item.");
const newTokenKey = new AccessTokenKey(
authority,
this.clientId,
serverTokenResponse.scope,
resource,
clientInfo && clientInfo.uid,
clientInfo && clientInfo.utid,
this.cryptoObj
);
this.cacheStorage.setItem(JSON.stringify(newTokenKey), JSON.stringify(newAccessTokenValue));
} else {
// Check if scopes are intersecting. If they are, combine scopes and replace cache item.
accessTokenCacheItems.forEach(accessTokenCacheItem => {
const cachedScopes = ScopeSet.fromString(accessTokenCacheItem.key.scopes, this.clientId, true);
if(cachedScopes.intersectingScopeSets(responseScopes)) {
this.cacheStorage.removeItem(JSON.stringify(accessTokenCacheItem.key));
cachedScopes.appendScopes(responseScopeArray);
accessTokenCacheItem.key.scopes = cachedScopes.printScopes();
responseScopes.appendScopes(cachedScopes.asArray());
if (StringUtils.isEmpty(newAccessTokenValue.idToken)) {
newAccessTokenValue.idToken = accessTokenCacheItem.value.idToken;
}
this.cacheStorage.setItem(JSON.stringify(accessTokenCacheItem.key), JSON.stringify(newAccessTokenValue));
}
});
}

const newTokenKey = new AccessTokenKey(
authority,
this.clientId,
responseScopes.printScopes(),
resource,
clientInfo && clientInfo.uid,
clientInfo && clientInfo.utid,
this.cryptoObj
);
this.cacheStorage.setItem(JSON.stringify(newTokenKey), JSON.stringify(newAccessTokenValue));

// Save tokens in response and return
return {
...originalTokenResponse,
Expand Down
10 changes: 10 additions & 0 deletions lib/msal-common/test/auth/ScopeSet.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { ScopeSet } from "../../src/auth/ScopeSet";
import { TEST_CONFIG } from "../utils/StringConstants";
import { ClientConfigurationError, ClientConfigurationErrorMessage, Constants, ClientAuthError, ClientAuthErrorMessage } from "../../src";
import sinon from "sinon";
import { IdToken } from "../../src/auth/IdToken";

describe("ScopeSet.ts", () => {

Expand Down Expand Up @@ -316,6 +317,15 @@ describe("ScopeSet.ts", () => {
expect(newScopeSet.intersectingScopeSets(requiredScopeSet)).to.be.false;
});

it("intersectingScopeSets() returns false if ScopeSets have no scopes in common other than offline_access", () => {
const testScope2 = "testScope2";
const newScopeSet = new ScopeSet([testScope2], TEST_CONFIG.MSAL_CLIENT_ID, true);

expect(newScopeSet.asArray()).to.contain("offline_access");
expect(requiredScopeSet.asArray()).to.contain("offline_access");
expect(newScopeSet.intersectingScopeSets(requiredScopeSet)).to.be.false;
});

it("getScopeCount() correctly returns the size of the ScopeSet", () => {
expect(requiredScopeSet.getScopeCount()).to.be.eq(2);
expect(nonRequiredScopeSet.getScopeCount()).to.be.eq(4);
Expand Down