Skip to content

Fix for concurrent iframe calls #6962

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

Merged
merged 9 commits into from
Apr 3, 2024
Merged

Fix for concurrent iframe calls #6962

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
29a2e1a
move cache and RT to separate function
tnorling Mar 19, 2024
d53c2e3
block concurrent calls
tnorling Mar 19, 2024
bad83b6
retry after awaiting
tnorling Mar 20, 2024
3eecce6
add tests
tnorling Mar 27, 2024
31bb1f8
Merge branch 'dev' of https://github.com/AzureAD/microsoft-authentica…
tnorling Mar 27, 2024
a3149f2
Change files
tnorling Mar 27, 2024
02ffc78
Merge branch 'dev' of https://github.com/AzureAD/microsoft-authentica…
tnorling Mar 29, 2024
8d9cc90
add awaitIframeCorrelationId to PerformanceEvent
tnorling Mar 29, 2024
35c9a38
Merge branch 'dev' into fix-iframe-concurrency
tnorling Apr 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions change/@azure-msal-browser-1c51e91a-9f80-485d-a826-aa20c210cc26.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "Improve perf and reliability of parallel acquireTokenSilent calls that must fallback to iframes #6962",
"packageName": "@azure/msal-browser",
"email": "[email protected]",
"dependentChangeType": "patch"
}
7 changes: 7 additions & 0 deletions change/@azure-msal-common-dbac308f-ff86-497a-b282-8b0e5cbf64ee.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "New performance event for awaiting parallel iframe calls #6962",
"packageName": "@azure/msal-common",
"email": "[email protected]",
"dependentChangeType": "patch"
}
269 changes: 172 additions & 97 deletions lib/msal-browser/src/controllers/StandardController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,9 @@ export class StandardController implements IController {
Promise<AuthenticationResult>
>;

// Active Iframe request
private activeIframeRequest: [Promise<void>, string] | undefined;

private ssoSilentMeasurement?: InProgressPerformanceEvent;
private acquireTokenByCodeAsyncMeasurement?: InProgressPerformanceEvent;
/**
Expand Down Expand Up @@ -1966,24 +1969,180 @@ export class StandardController implements IController {
document.addEventListener("visibilitychange", () =>
this.trackPageVisibility(request.correlationId)
);
let result: Promise<AuthenticationResult>;

const silentRequest = await invokeAsync(
initializeSilentRequest,
PerformanceEvents.InitializeSilentRequest,
this.logger,
this.performanceClient,
request.correlationId
)(request, account, this.config, this.performanceClient, this.logger);
const cacheLookupPolicy =
request.cacheLookupPolicy || CacheLookupPolicy.Default;

const result = this.acquireTokenSilentNoIframe(
silentRequest,
cacheLookupPolicy
).catch(async (refreshTokenError: AuthError) => {
const shouldTryToResolveSilently =
checkIfRefreshTokenErrorCanBeResolvedSilently(
refreshTokenError,
cacheLookupPolicy
);

if (shouldTryToResolveSilently) {
if (!this.activeIframeRequest) {
let _resolve: () => void,
_reject: (reason?: AuthError | Error) => void;
// Always set the active request tracker immediately after checking it to prevent races
this.activeIframeRequest = [
new Promise((resolve, reject) => {
_resolve = resolve;
_reject = reject;
}),
silentRequest.correlationId,
];
this.logger.verbose(
"Refresh token expired/invalid or CacheLookupPolicy is set to Skip, attempting acquire token by iframe.",
silentRequest.correlationId
);
return invokeAsync(
this.acquireTokenBySilentIframe.bind(this),
PerformanceEvents.AcquireTokenBySilentIframe,
this.logger,
this.performanceClient,
silentRequest.correlationId
)(silentRequest)
.then((iframeResult) => {
_resolve();
return iframeResult;
})
.catch((e) => {
_reject(e);
throw e;
})
.finally(() => {
this.activeIframeRequest = undefined;
});
} else if (cacheLookupPolicy !== CacheLookupPolicy.Skip) {
const [activePromise, activeCorrelationId] =
this.activeIframeRequest;
this.logger.verbose(
`Iframe request is already in progress, awaiting resolution for request with correlationId: ${activeCorrelationId}`,
silentRequest.correlationId
);
const awaitConcurrentIframeMeasure =
this.performanceClient.startMeasurement(
PerformanceEvents.AwaitConcurrentIframe,
silentRequest.correlationId
);
awaitConcurrentIframeMeasure.add({
awaitIframeCorrelationId: activeCorrelationId,
});

// Await for errors first so we can distinguish errors thrown by activePromise versus errors thrown by .then below
Copy link
Member

Choose a reason for hiding this comment

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

Comment incomplete?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I dont think so - do I need to rephrase?

await activePromise.catch(() => {
awaitConcurrentIframeMeasure.end({
success: false,
});
this.logger.info(
`Iframe request with correlationId: ${activeCorrelationId} failed. Interaction is required.`
);
// If previous iframe request failed, it's unlikely to succeed this time. Throw original error.
throw refreshTokenError;
});

return activePromise.then(() => {
awaitConcurrentIframeMeasure.end({ success: true });
this.logger.verbose(
`Parallel iframe request with correlationId: ${activeCorrelationId} succeeded. Retrying cache and/or RT redemption`,
silentRequest.correlationId
);
// Retry cache lookup and/or RT exchange after iframe completes
return this.acquireTokenSilentNoIframe(
silentRequest,
cacheLookupPolicy
);
});
} else {
// Cache policy set to skip and another iframe request is already in progress
this.logger.warning(
"Another iframe request is currently in progress and CacheLookupPolicy is set to Skip. This may result in degraded performance and/or reliability for both calls. Please consider changing the CacheLookupPolicy to take advantage of request queuing and token cache.",
silentRequest.correlationId
);
return invokeAsync(
this.acquireTokenBySilentIframe.bind(this),
PerformanceEvents.AcquireTokenBySilentIframe,
this.logger,
this.performanceClient,
silentRequest.correlationId
)(silentRequest);
}
} else {
// Error cannot be silently resolved or iframe renewal is not allowed, interaction required
throw refreshTokenError;
}
});

return result
.then((response) => {
this.eventHandler.emitEvent(
EventType.ACQUIRE_TOKEN_SUCCESS,
InteractionType.Silent,
response
);
if (request.correlationId) {
this.performanceClient.addFields(
{
fromCache: response.fromCache,
isNativeBroker: response.fromNativeBroker,
requestId: response.requestId,
},
request.correlationId
);
}

return response;
})
.catch((tokenRenewalError: Error) => {
this.eventHandler.emitEvent(
EventType.ACQUIRE_TOKEN_FAILURE,
InteractionType.Silent,
null,
tokenRenewalError
);
throw tokenRenewalError;
})
.finally(() => {
document.removeEventListener("visibilitychange", () =>
this.trackPageVisibility(request.correlationId)
);
});
}

/**
* AcquireTokenSilent without the iframe fallback. This is used to enable the correct fallbacks in cases where there's a potential for multiple silent requests to be made in parallel and prevent those requests from making concurrent iframe requests.
* @param silentRequest
* @param cacheLookupPolicy
* @returns
*/
private async acquireTokenSilentNoIframe(
silentRequest: CommonSilentFlowRequest,
cacheLookupPolicy: CacheLookupPolicy
): Promise<AuthenticationResult> {
if (
NativeMessageHandler.isNativeAvailable(
this.config,
this.logger,
this.nativeExtensionProvider,
request.authenticationScheme
silentRequest.authenticationScheme
) &&
account.nativeAccountId
silentRequest.account.nativeAccountId
) {
this.logger.verbose(
"acquireTokenSilent - attempting to acquire token from native platform"
);
const silentRequest: SilentRequest = {
...request,
account,
};
result = this.acquireTokenNative(
return this.acquireTokenNative(
silentRequest,
ApiId.acquireTokenSilent_silentFlow
).catch(async (e: AuthError) => {
Expand All @@ -1995,47 +2154,25 @@ export class StandardController implements IController {
this.nativeExtensionProvider = undefined; // Prevent future requests from continuing to attempt

// Cache will not contain tokens, given that previous WAM requests succeeded. Skip cache and RT renewal and go straight to iframe renewal
const silentIframeClient = this.createSilentIframeClient(
request.correlationId
throw createClientAuthError(
ClientAuthErrorCodes.tokenRefreshRequired
);
return silentIframeClient.acquireToken(request);
}
throw e;
});
} else {
this.logger.verbose(
"acquireTokenSilent - attempting to acquire token from web flow"
);

const silentRequest = await invokeAsync(
initializeSilentRequest,
PerformanceEvents.InitializeSilentRequest,
this.logger,
this.performanceClient,
request.correlationId
)(
request,
account,
this.config,
this.performanceClient,
this.logger
);

const cacheLookupPolicy =
request.cacheLookupPolicy || CacheLookupPolicy.Default;

result = invokeAsync(
return invokeAsync(
this.acquireTokenFromCache.bind(this),
PerformanceEvents.AcquireTokenFromCache,
this.logger,
this.performanceClient,
silentRequest.correlationId
)(silentRequest, cacheLookupPolicy).catch(
(cacheError: AuthError) => {
if (
request.cacheLookupPolicy ===
CacheLookupPolicy.AccessToken
) {
if (cacheLookupPolicy === CacheLookupPolicy.AccessToken) {
throw cacheError;
}

Expand All @@ -2051,71 +2188,10 @@ export class StandardController implements IController {
this.logger,
this.performanceClient,
silentRequest.correlationId
)(silentRequest, cacheLookupPolicy).catch(
(refreshTokenError: AuthError) => {
const shouldTryToResolveSilently =
checkIfRefreshTokenErrorCanBeResolvedSilently(
refreshTokenError,
silentRequest,
cacheLookupPolicy
);

if (shouldTryToResolveSilently) {
this.logger.verbose(
"Refresh token expired/invalid or CacheLookupPolicy is set to Skip, attempting acquire token by iframe.",
silentRequest.correlationId
);
return invokeAsync(
this.acquireTokenBySilentIframe.bind(this),
PerformanceEvents.AcquireTokenBySilentIframe,
this.logger,
this.performanceClient,
silentRequest.correlationId
)(silentRequest);
} else {
// Error cannot be silently resolved or iframe renewal is not allowed, interaction required
throw refreshTokenError;
}
}
);
)(silentRequest, cacheLookupPolicy);
}
);
}

return result
.then((response) => {
this.eventHandler.emitEvent(
EventType.ACQUIRE_TOKEN_SUCCESS,
InteractionType.Silent,
response
);
if (request.correlationId) {
this.performanceClient.addFields(
{
fromCache: response.fromCache,
isNativeBroker: response.fromNativeBroker,
requestId: response.requestId,
},
request.correlationId
);
}

return response;
})
.catch((tokenRenewalError: Error) => {
this.eventHandler.emitEvent(
EventType.ACQUIRE_TOKEN_FAILURE,
InteractionType.Silent,
null,
tokenRenewalError
);
throw tokenRenewalError;
})
.finally(() => {
document.removeEventListener("visibilitychange", () =>
this.trackPageVisibility(request.correlationId)
);
});
}
}

Expand All @@ -2128,7 +2204,6 @@ export class StandardController implements IController {
*/
function checkIfRefreshTokenErrorCanBeResolvedSilently(
refreshTokenError: AuthError,
silentRequest: CommonSilentFlowRequest,
cacheLookupPolicy: CacheLookupPolicy
): boolean {
const noInteractionRequired = !(
Expand Down
Loading
Loading