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

Memoize handleRedirectPromise #3072

Merged
merged 9 commits into from
Feb 25, 2021
Merged

Memoize handleRedirectPromise #3072

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
85dd41c
memoize handleRedirectPromise
tnorling Feb 23, 2021
9b0bb19
Remove .only
tnorling Feb 23, 2021
6f15375
Change files
tnorling Feb 23, 2021
6f7a3c4
Merge branch 'dev' into memoize-handleRedirectPromise
tnorling Feb 24, 2021
ac611cf
Add log messages/comments
tnorling Feb 24, 2021
a5bd835
Merge branch 'memoize-handleRedirectPromise' of https://github.com/Az…
tnorling Feb 24, 2021
98fe518
Merge branch 'dev' into memoize-handleRedirectPromise
tnorling Feb 24, 2021
d703266
Segment stored response by hash provided
tnorling Feb 24, 2021
2ca0228
Merge branch 'memoize-handleRedirectPromise' of https://github.com/Az…
tnorling Feb 24, 2021
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
7 changes: 7 additions & 0 deletions change/@azure-msal-browser-38f0f734-e4bf-4dd5-9c4e-ef1379f478d6.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "Memoize multiple calls to handleRedirectPromise (#3072)",
"packageName": "@azure/msal-browser",
"email": "thomas.norling@microsoft.com",
"dependentChangeType": "patch"
}
67 changes: 41 additions & 26 deletions lib/msal-browser/src/app/ClientApplication.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ export abstract class ClientApplication {
// Callback for subscribing to events
private eventCallbacks: Map<string, EventCallbackFunction>;

// Redirect Response Object
private redirectResponse: Promise<AuthenticationResult | null> | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

If the page is re-rendered won't this variable be re-rendered also? Doesn't this only solve handleRedirectPromise being called multiple times in a single page load?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that's right. More specifically this solves handleRedirectPromise being called multiple times per pca instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

in the PR description it says that it may solve issues when frameworks re-render components, we should update this in case someone gets confused thinking this will solve re-rendered instances.

Copy link
Member

@sameerag sameerag Feb 24, 2021 •
edited

Choose a reason for hiding this comment

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

For a min, I thought the same. :-D. Also @tnorling I do not see page refresh mentioned (or re-rendering) in the note in the description.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Page refresh is not a broken scenario. This is only to guard against concurrent calls to handleRedirectPromise throwing errors


/**
* @constructor
* Constructor for the PublicClientApplication used to instantiate the PublicClientApplication object
Expand Down Expand Up @@ -123,34 +126,46 @@ export abstract class ClientApplication {
this.logger.verbose("handleRedirectPromise called");
const loggedInAccounts = this.getAllAccounts();
if (this.isBrowserEnvironment) {
return this.handleRedirectResponse(hash)
.then((result: AuthenticationResult | null) => {
if (result) {
// Emit login event if number of accounts change
const isLoggingIn = loggedInAccounts.length < this.getAllAccounts().length;
if (isLoggingIn) {
this.emitEvent(EventType.LOGIN_SUCCESS, InteractionType.Redirect, result);
this.logger.verbose("handleRedirectResponse returned result, login success");
/**
* Store the promise on the PublicClientApplication instance if this is the first invocation of handleRedirectPromise,
* otherwise return the promise from the first invocation. Prevents race conditions when handleRedirectPromise is called
* several times concurrently.
*/
if (typeof this.redirectResponse === "undefined") {
tnorling marked this conversation as resolved.
Show resolved Hide resolved
this.logger.verbose("handleRedirectPromise has been called for the first time, storing the promise");
this.redirectResponse = this.handleRedirectResponse(hash)
.then((result: AuthenticationResult | null) => {
if (result) {
// Emit login event if number of accounts change
const isLoggingIn = loggedInAccounts.length < this.getAllAccounts().length;
if (isLoggingIn) {
this.emitEvent(EventType.LOGIN_SUCCESS, InteractionType.Redirect, result);
this.logger.verbose("handleRedirectResponse returned result, login success");
} else {
this.emitEvent(EventType.ACQUIRE_TOKEN_SUCCESS, InteractionType.Redirect, result);
this.logger.verbose("handleRedirectResponse returned result, acquire token success");
}
}
this.emitEvent(EventType.HANDLE_REDIRECT_END, InteractionType.Redirect);

return result;
})
.catch((e) => {
// Emit login event if there is an account
if (loggedInAccounts.length > 0) {
this.emitEvent(EventType.ACQUIRE_TOKEN_FAILURE, InteractionType.Redirect, null, e);
} else {
this.emitEvent(EventType.ACQUIRE_TOKEN_SUCCESS, InteractionType.Redirect, result);
this.logger.verbose("handleRedirectResponse returned result, acquire token success");
this.emitEvent(EventType.LOGIN_FAILURE, InteractionType.Redirect, null, e);
}
}
this.emitEvent(EventType.HANDLE_REDIRECT_END, InteractionType.Redirect);

return result;
})
.catch((e) => {
// Emit login event if there is an account
if (loggedInAccounts.length > 0) {
this.emitEvent(EventType.ACQUIRE_TOKEN_FAILURE, InteractionType.Redirect, null, e);
} else {
this.emitEvent(EventType.LOGIN_FAILURE, InteractionType.Redirect, null, e);
}
this.emitEvent(EventType.HANDLE_REDIRECT_END, InteractionType.Redirect);

throw e;
});
this.emitEvent(EventType.HANDLE_REDIRECT_END, InteractionType.Redirect);

throw e;
});
} else {
this.logger.verbose("handleRedirectPromise has been called previously, returning the result from the first call");
}

return this.redirectResponse;
}
this.logger.verbose("handleRedirectPromise returns null, not browser environment");
return null;
Expand Down
105 changes: 105 additions & 0 deletions lib/msal-browser/test/app/PublicClientApplication.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,111 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {
expect(window.sessionStorage.length).to.be.eq(4);
});

it("Multiple concurrent calls to handleRedirectPromise return the same promise", async () => {
const b64Encode = new Base64Encode();
const stateString = TEST_STATE_VALUES.TEST_STATE_REDIRECT;
const browserCrypto = new CryptoOps();
const stateId = ProtocolUtils.parseRequestState(browserCrypto, stateString).libraryState.id;

window.sessionStorage.setItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.ORIGIN_URI}`, TEST_URIS.TEST_REDIR_URI);
window.sessionStorage.setItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.AUTHORITY}.${stateId}`, TEST_CONFIG.validAuthority);
window.sessionStorage.setItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.REQUEST_STATE}.${stateId}`, TEST_STATE_VALUES.TEST_STATE_REDIRECT);
window.sessionStorage.setItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.URL_HASH}`, TEST_HASHES.TEST_SUCCESS_CODE_HASH_REDIRECT);
window.sessionStorage.setItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.INTERACTION_STATUS_KEY}`, BrowserConstants.INTERACTION_IN_PROGRESS_VALUE);
window.sessionStorage.setItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.NONCE_IDTOKEN}.${stateId}`, "123523");
const testTokenReq: AuthorizationCodeRequest = {
redirectUri: `${TEST_URIS.DEFAULT_INSTANCE}/`,
code: "thisIsATestCode",
scopes: TEST_CONFIG.DEFAULT_SCOPES,
codeVerifier: TEST_CONFIG.TEST_VERIFIER,
authority: `${Constants.DEFAULT_AUTHORITY}`,
correlationId: RANDOM_TEST_GUID,
authenticationScheme: TEST_CONFIG.TOKEN_TYPE_BEARER as AuthenticationScheme
};
window.sessionStorage.setItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.REQUEST_PARAMS}`, b64Encode.encode(JSON.stringify(testTokenReq)));
const testServerTokenResponse = {
headers: null,
status: 200,
body: {
token_type: TEST_CONFIG.TOKEN_TYPE_BEARER,
scope: TEST_CONFIG.DEFAULT_SCOPES.join(" "),
expires_in: TEST_TOKEN_LIFETIMES.DEFAULT_EXPIRES_IN,
ext_expires_in: TEST_TOKEN_LIFETIMES.DEFAULT_EXPIRES_IN,
access_token: TEST_TOKENS.ACCESS_TOKEN,
refresh_token: TEST_TOKENS.REFRESH_TOKEN,
id_token: TEST_TOKENS.IDTOKEN_V2,
client_info: TEST_DATA_CLIENT_INFO.TEST_RAW_CLIENT_INFO
}
};
const testIdTokenClaims: TokenClaims = {
"ver": "2.0",
"iss": "https://login.microsoftonline.com/9188040d-6c67-4c5b-b112-36a304b66dad/v2.0",
"sub": "AAAAAAAAAAAAAAAAAAAAAIkzqFVrSaSaFHy782bbtaQ",
"name": "Abe Lincoln",
"preferred_username": "AbeLi@microsoft.com",
"oid": "00000000-0000-0000-66f3-3332eca7ea81",
"tid": "3338040d-6c67-4c5b-b112-36a304b66dad",
"nonce": "123523",
};
const testAccount: AccountInfo = {
homeAccountId: TEST_DATA_CLIENT_INFO.TEST_HOME_ACCOUNT_ID,
localAccountId: TEST_DATA_CLIENT_INFO.TEST_UID,
environment: "login.windows.net",
tenantId: testIdTokenClaims.tid,
username: testIdTokenClaims.preferred_username
};
const testTokenResponse: AuthenticationResult = {
authority: TEST_CONFIG.validAuthority,
uniqueId: testIdTokenClaims.oid,
tenantId: testIdTokenClaims.tid,
scopes: TEST_CONFIG.DEFAULT_SCOPES,
idToken: testServerTokenResponse.body.id_token,
idTokenClaims: testIdTokenClaims,
accessToken: testServerTokenResponse.body.access_token,
fromCache: false,
expiresOn: new Date(Date.now() + (testServerTokenResponse.body.expires_in * 1000)),
account: testAccount,
tokenType: AuthenticationScheme.BEARER
};
sinon.stub(XhrClient.prototype, "sendGetRequestAsync").resolves(DEFAULT_OPENID_CONFIG_RESPONSE);
sinon.stub(XhrClient.prototype, "sendPostRequestAsync").resolves(testServerTokenResponse);
pca = new PublicClientApplication({
auth: {
clientId: TEST_CONFIG.MSAL_CLIENT_ID
}
});

const promise1 = pca.handleRedirectPromise();
const promise2 = pca.handleRedirectPromise();
const tokenResponse1 = await promise1;
const tokenResponse2 = await promise2;

if (!tokenResponse1 || !tokenResponse2) {
throw "This should not throw. Both responses should be non-null."
}

// Response from first promise
expect(tokenResponse1.uniqueId).to.be.eq(testTokenResponse.uniqueId);
expect(tokenResponse1.tenantId).to.be.eq(testTokenResponse.tenantId);
expect(tokenResponse1.scopes).to.be.deep.eq(testTokenResponse.scopes);
expect(tokenResponse1.idToken).to.be.eq(testTokenResponse.idToken);
expect(tokenResponse1.idTokenClaims).to.be.contain(testTokenResponse.idTokenClaims);
expect(tokenResponse1.accessToken).to.be.eq(testTokenResponse.accessToken);
expect(testTokenResponse.expiresOn.getMilliseconds() >= tokenResponse1.expiresOn.getMilliseconds()).to.be.true;

// Response from second promise
expect(tokenResponse2.uniqueId).to.be.eq(testTokenResponse.uniqueId);
expect(tokenResponse2.tenantId).to.be.eq(testTokenResponse.tenantId);
expect(tokenResponse2.scopes).to.be.deep.eq(testTokenResponse.scopes);
expect(tokenResponse2.idToken).to.be.eq(testTokenResponse.idToken);
expect(tokenResponse2.idTokenClaims).to.be.contain(testTokenResponse.idTokenClaims);
expect(tokenResponse2.accessToken).to.be.eq(testTokenResponse.accessToken);
expect(testTokenResponse.expiresOn.getMilliseconds() >= tokenResponse2.expiresOn.getMilliseconds()).to.be.true;

expect(tokenResponse1).to.deep.eq(tokenResponse2);
expect(window.sessionStorage.length).to.be.eq(4);
});

it("gets hash from cache and processes error", (done) => {
const testAuthCodeRequest: AuthorizationCodeRequest = {
redirectUri: TEST_URIS.TEST_REDIR_URI,
Expand Down