From d15694644f9cec046032a16267110bf4209dad9d Mon Sep 17 00:00:00 2001 From: Thomas Norling Date: Tue, 9 Apr 2024 13:40:17 -0700 Subject: [PATCH] Fix handleRedirectPromise Memoization (#6998) We're doing too many things after checking for an existing promise but before setting the current promise which can create a race condition if multiple instances of handleRedirectPromise are invoked. To address this PR sets the promise immediately after checking for one. Fixes #6893 --------- Co-authored-by: Jo Arroyo --- ...-b7c89de9-6914-47ba-9e31-0cd60c0fb29f.json | 7 + .../src/controllers/StandardController.ts | 299 +++++++++--------- .../test/app/PublicClientApplication.spec.ts | 19 +- 3 files changed, 164 insertions(+), 161 deletions(-) create mode 100644 change/@azure-msal-browser-b7c89de9-6914-47ba-9e31-0cd60c0fb29f.json diff --git a/change/@azure-msal-browser-b7c89de9-6914-47ba-9e31-0cd60c0fb29f.json b/change/@azure-msal-browser-b7c89de9-6914-47ba-9e31-0cd60c0fb29f.json new file mode 100644 index 0000000000..4c5d22c5b9 --- /dev/null +++ b/change/@azure-msal-browser-b7c89de9-6914-47ba-9e31-0cd60c0fb29f.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "fix handleRedirectPromise memoization #6998", + "packageName": "@azure/msal-browser", + "email": "thomas.norling@microsoft.com", + "dependentChangeType": "patch" +} diff --git a/lib/msal-browser/src/controllers/StandardController.ts b/lib/msal-browser/src/controllers/StandardController.ts index 9f4bd52fb9..9a9b53a049 100644 --- a/lib/msal-browser/src/controllers/StandardController.ts +++ b/lib/msal-browser/src/controllers/StandardController.ts @@ -335,7 +335,6 @@ export class StandardController implements IController { // Block token acquisition before initialize has been called BrowserUtils.blockAPICallsBeforeInitialize(this.initialized); - const loggedInAccounts = this.getAllAccounts(); if (this.isBrowserEnvironment) { /** * Store the promise on the PublicClientApplication instance if this is the first invocation of handleRedirectPromise, @@ -345,155 +344,11 @@ export class StandardController implements IController { const redirectResponseKey = hash || ""; let response = this.redirectResponse.get(redirectResponseKey); if (typeof response === "undefined") { - const request: NativeTokenRequest | null = - this.browserStorage.getCachedNativeRequest(); - const useNative = - request && - NativeMessageHandler.isNativeAvailable( - this.config, - this.logger, - this.nativeExtensionProvider - ) && - this.nativeExtensionProvider && - !hash; - const correlationId = useNative - ? request?.correlationId - : this.browserStorage.getTemporaryCache( - TemporaryCacheKeys.CORRELATION_ID, - true - ) || ""; - const rootMeasurement = this.performanceClient.startMeasurement( - "acquireTokenRedirect", - correlationId - ); - this.eventHandler.emitEvent( - EventType.HANDLE_REDIRECT_START, - InteractionType.Redirect - ); + response = this.handleRedirectPromiseInternal(hash); + this.redirectResponse.set(redirectResponseKey, response); this.logger.verbose( "handleRedirectPromise has been called for the first time, storing the promise" ); - let redirectResponse: Promise; - if (useNative && this.nativeExtensionProvider) { - this.logger.trace( - "handleRedirectPromise - acquiring token from native platform" - ); - const nativeClient = new NativeInteractionClient( - this.config, - this.browserStorage, - this.browserCrypto, - this.logger, - this.eventHandler, - this.navigationClient, - ApiId.handleRedirectPromise, - this.performanceClient, - this.nativeExtensionProvider, - request.accountId, - this.nativeInternalStorage, - request.correlationId - ); - - redirectResponse = invokeAsync( - nativeClient.handleRedirectPromise.bind(nativeClient), - PerformanceEvents.HandleNativeRedirectPromiseMeasurement, - this.logger, - this.performanceClient, - rootMeasurement.event.correlationId - )( - this.performanceClient, - rootMeasurement.event.correlationId - ); - } else { - this.logger.trace( - "handleRedirectPromise - acquiring token from web flow" - ); - const redirectClient = - this.createRedirectClient(correlationId); - redirectResponse = invokeAsync( - redirectClient.handleRedirectPromise.bind( - redirectClient - ), - PerformanceEvents.HandleRedirectPromiseMeasurement, - this.logger, - this.performanceClient, - rootMeasurement.event.correlationId - )( - hash, - this.performanceClient, - rootMeasurement.event.correlationId - ); - } - - response = redirectResponse - .then((result: AuthenticationResult | null) => { - if (result) { - // Emit login event if number of accounts change - - const isLoggingIn = - loggedInAccounts.length < - this.getAllAccounts().length; - if (isLoggingIn) { - this.eventHandler.emitEvent( - EventType.LOGIN_SUCCESS, - InteractionType.Redirect, - result - ); - this.logger.verbose( - "handleRedirectResponse returned result, login success" - ); - } else { - this.eventHandler.emitEvent( - EventType.ACQUIRE_TOKEN_SUCCESS, - InteractionType.Redirect, - result - ); - this.logger.verbose( - "handleRedirectResponse returned result, acquire token success" - ); - } - rootMeasurement.end({ success: true }); - } - this.eventHandler.emitEvent( - EventType.HANDLE_REDIRECT_END, - InteractionType.Redirect - ); - rootMeasurement.end({ success: false }); - - return result; - }) - .catch((e) => { - const eventError = e as EventError; - // Emit login event if there is an account - if (loggedInAccounts.length > 0) { - this.eventHandler.emitEvent( - EventType.ACQUIRE_TOKEN_FAILURE, - InteractionType.Redirect, - null, - eventError - ); - } else { - this.eventHandler.emitEvent( - EventType.LOGIN_FAILURE, - InteractionType.Redirect, - null, - eventError - ); - } - this.eventHandler.emitEvent( - EventType.HANDLE_REDIRECT_END, - InteractionType.Redirect - ); - - rootMeasurement.end( - { - success: false, - }, - eventError - ); - - throw e; - }); - this.redirectResponse.set(redirectResponseKey, response); } else { this.logger.verbose( "handleRedirectPromise has been called previously, returning the result from the first call" @@ -508,6 +363,156 @@ export class StandardController implements IController { return null; } + /** + * The internal details of handleRedirectPromise. This is separated out to a helper to allow handleRedirectPromise to memoize requests + * @param hash + * @returns + */ + private async handleRedirectPromiseInternal( + hash?: string + ): Promise { + const loggedInAccounts = this.getAllAccounts(); + const request: NativeTokenRequest | null = + this.browserStorage.getCachedNativeRequest(); + const useNative = + request && + NativeMessageHandler.isNativeAvailable( + this.config, + this.logger, + this.nativeExtensionProvider + ) && + this.nativeExtensionProvider && + !hash; + const correlationId = useNative + ? request?.correlationId + : this.browserStorage.getTemporaryCache( + TemporaryCacheKeys.CORRELATION_ID, + true + ) || ""; + const rootMeasurement = this.performanceClient.startMeasurement( + "acquireTokenRedirect", + correlationId + ); + this.eventHandler.emitEvent( + EventType.HANDLE_REDIRECT_START, + InteractionType.Redirect + ); + + let redirectResponse: Promise; + if (useNative && this.nativeExtensionProvider) { + this.logger.trace( + "handleRedirectPromise - acquiring token from native platform" + ); + const nativeClient = new NativeInteractionClient( + this.config, + this.browserStorage, + this.browserCrypto, + this.logger, + this.eventHandler, + this.navigationClient, + ApiId.handleRedirectPromise, + this.performanceClient, + this.nativeExtensionProvider, + request.accountId, + this.nativeInternalStorage, + request.correlationId + ); + + redirectResponse = invokeAsync( + nativeClient.handleRedirectPromise.bind(nativeClient), + PerformanceEvents.HandleNativeRedirectPromiseMeasurement, + this.logger, + this.performanceClient, + rootMeasurement.event.correlationId + )(this.performanceClient, rootMeasurement.event.correlationId); + } else { + this.logger.trace( + "handleRedirectPromise - acquiring token from web flow" + ); + const redirectClient = this.createRedirectClient(correlationId); + redirectResponse = invokeAsync( + redirectClient.handleRedirectPromise.bind(redirectClient), + PerformanceEvents.HandleRedirectPromiseMeasurement, + this.logger, + this.performanceClient, + rootMeasurement.event.correlationId + )( + hash, + this.performanceClient, + rootMeasurement.event.correlationId + ); + } + + return redirectResponse + .then((result: AuthenticationResult | null) => { + if (result) { + // Emit login event if number of accounts change + + const isLoggingIn = + loggedInAccounts.length < this.getAllAccounts().length; + if (isLoggingIn) { + this.eventHandler.emitEvent( + EventType.LOGIN_SUCCESS, + InteractionType.Redirect, + result + ); + this.logger.verbose( + "handleRedirectResponse returned result, login success" + ); + } else { + this.eventHandler.emitEvent( + EventType.ACQUIRE_TOKEN_SUCCESS, + InteractionType.Redirect, + result + ); + this.logger.verbose( + "handleRedirectResponse returned result, acquire token success" + ); + } + rootMeasurement.end({ success: true }); + } + this.eventHandler.emitEvent( + EventType.HANDLE_REDIRECT_END, + InteractionType.Redirect + ); + rootMeasurement.end({ success: false }); + + return result; + }) + .catch((e) => { + const eventError = e as EventError; + // Emit login event if there is an account + if (loggedInAccounts.length > 0) { + this.eventHandler.emitEvent( + EventType.ACQUIRE_TOKEN_FAILURE, + InteractionType.Redirect, + null, + eventError + ); + } else { + this.eventHandler.emitEvent( + EventType.LOGIN_FAILURE, + InteractionType.Redirect, + null, + eventError + ); + } + this.eventHandler.emitEvent( + EventType.HANDLE_REDIRECT_END, + InteractionType.Redirect + ); + + rootMeasurement.end( + { + success: false, + }, + eventError + ); + + throw e; + }); + } + /** * Use when you want to obtain an access_token for your API by redirecting the user's browser window to the authorization endpoint. This function redirects * the page, so any code that follows this function will not execute. diff --git a/lib/msal-browser/test/app/PublicClientApplication.spec.ts b/lib/msal-browser/test/app/PublicClientApplication.spec.ts index 591ad5047c..58a2998976 100644 --- a/lib/msal-browser/test/app/PublicClientApplication.spec.ts +++ b/lib/msal-browser/test/app/PublicClientApplication.spec.ts @@ -944,20 +944,9 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { account: testAccount, tokenType: AuthenticationScheme.BEARER, }; - sinon - .stub(FetchClient.prototype, "sendGetRequestAsync") - .callsFake((url): any => { - if (url.includes("discovery/instance")) { - return DEFAULT_TENANT_DISCOVERY_RESPONSE; - } else if ( - url.includes(".well-known/openid-configuration") - ) { - return DEFAULT_OPENID_CONFIG_RESPONSE; - } - }); - sinon - .stub(FetchClient.prototype, "sendPostRequestAsync") - .resolves(testServerTokenResponse); + const postMock = jest + .spyOn(FetchClient.prototype, "sendPostRequestAsync") + .mockResolvedValueOnce(testServerTokenResponse); pca = new PublicClientApplication({ auth: { clientId: TEST_CONFIG.MSAL_CLIENT_ID, @@ -985,6 +974,8 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { throw "This should not throw. Both responses should be non-null."; } + expect(postMock).toHaveBeenCalledTimes(1); + // Response from first promise expect(tokenResponse1.uniqueId).toEqual(testTokenResponse.uniqueId); expect(tokenResponse1.tenantId).toEqual(testTokenResponse.tenantId);