Skip to content

Commit

Permalink
Fix handleRedirectPromise Memoization (#6998)
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@tnorling @jo-arroyo
tnorling and jo-arroyo authored Apr 9, 2024
1 parent a196470 commit d156946
Show file tree
Hide file tree
Showing 3 changed files with 164 additions and 161 deletions.
7 changes: 7 additions & 0 deletions change/@azure-msal-browser-b7c89de9-6914-47ba-9e31-0cd60c0fb29f.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "fix handleRedirectPromise memoization #6998",
"packageName": "@azure/msal-browser",
"email": "[email protected]",
"dependentChangeType": "patch"
}
299 changes: 152 additions & 147 deletions lib/msal-browser/src/controllers/StandardController.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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<AuthenticationResult | null>;
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"
Expand All @@ -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<AuthenticationResult | null> {
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<AuthenticationResult | null>;
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.
Expand Down
19 changes: 5 additions & 14 deletions lib/msal-browser/test/app/PublicClientApplication.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit d156946

Please sign in to comment.