Skip to content

Commit

Permalink
Optimize response parsing (#6646)
Browse files Browse the repository at this point in the history 
This PR optimizes response fragment/query parsing by:
- ensuring it only happens once (today /authorize responses are parsed
at least twice and up to 3 times in a single request)
- using APIs provided by the browser/node runtime rather than our own,
slower, implementation
- ensuring we correctly extract the fragment or query depending on what
was configured by OIDCOptions (there are bugs related to this today)
- renames references to "hash" to "response" to reflect the fact that it
can come from either the hash or query
- removes popup timeout logic which only started counting once the popup
returned to the redirectUri. We should consider implementing a timeout
for the entire operation but this may be a breaking change so left that
out of scope for this PR.
  • Loading branch information
@tnorling
tnorling authored Nov 6, 2023
1 parent e9dd933 commit 619e10f
Show file tree
Hide file tree
Showing 30 changed files with 747 additions and 810 deletions.
7 changes: 7 additions & 0 deletions change/@azure-msal-browser-30b7bab2-c13f-46ec-bc23-99211431d098.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "minor",
"comment": "Optimize response parsing & address bugs related to query response type #6646",
"packageName": "@azure/msal-browser",
"email": "[email protected]",
"dependentChangeType": "patch"
}
7 changes: 7 additions & 0 deletions change/@azure-msal-common-d245eb0b-38fb-4f30-a5f6-aaafa5295fba.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "minor",
"comment": "Update hash/query parsing to use runtime provided APIs #6646",
"packageName": "@azure/msal-common",
"email": "[email protected]",
"dependentChangeType": "patch"
}
7 changes: 7 additions & 0 deletions change/@azure-msal-node-54978b56-aca4-4e1c-bc18-a79298d7231a.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "Use new query string parser in LoopbackClient #6646",
"packageName": "@azure/msal-node",
"email": "[email protected]",
"dependentChangeType": "patch"
}
21 changes: 3 additions & 18 deletions lib/msal-browser/src/controllers/StandardController.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ import {
InProgressPerformanceEvent,
RequestThumbprint,
AccountEntity,
ServerResponseType,
UrlString,
invokeAsync,
createClientAuthError,
ClientAuthErrorCodes,
Expand Down Expand Up @@ -336,25 +334,14 @@ export class StandardController implements IController {
// Block token acquisition before initialize has been called
BrowserUtils.blockAPICallsBeforeInitialize(this.initialized);

let foundServerResponse = hash;

if (
this.config.auth.OIDCOptions?.serverResponseType ===
ServerResponseType.QUERY
) {
const url = window.location.href;
foundServerResponse = UrlString.parseQueryServerResponse(url);
}

const loggedInAccounts = this.getAllAccounts();
if (this.isBrowserEnvironment) {
/**
* 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.
*/
const redirectResponseKey =
foundServerResponse || Constants.EMPTY_STRING;
const redirectResponseKey = hash || "";
let response = this.redirectResponse.get(redirectResponseKey);
if (typeof response === "undefined") {
this.eventHandler.emitEvent(
Expand All @@ -376,7 +363,7 @@ export class StandardController implements IController {
this.nativeExtensionProvider
) &&
this.nativeExtensionProvider &&
!foundServerResponse
!hash
) {
this.logger.trace(
"handleRedirectPromise - acquiring token from native platform"
Expand Down Expand Up @@ -408,9 +395,7 @@ export class StandardController implements IController {
const redirectClient =
this.createRedirectClient(correlationId);
redirectResponse =
redirectClient.handleRedirectPromise(
foundServerResponse
);
redirectClient.handleRedirectPromise(hash);
}

response = redirectResponse
Expand Down
171 changes: 34 additions & 137 deletions lib/msal-browser/src/interaction_client/PopupClient.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,15 @@ import {
UrlString,
AuthError,
OIDC_DEFAULT_SCOPES,
Constants,
ProtocolUtils,
ServerAuthorizationCodeResponse,
PerformanceEvents,
IPerformanceClient,
Logger,
ICrypto,
ProtocolMode,
ServerResponseType,
invokeAsync,
invoke,
} from "@azure/msal-common";
import { StandardInteractionClient } from "./StandardInteractionClient";
import { EventType } from "../event/EventType";
Expand All @@ -47,6 +46,7 @@ import { InteractionHandler } from "../interaction_handler/InteractionHandler";
import { PopupWindowAttributes } from "../request/PopupWindowAttributes";
import { EventError } from "../event/EventMessage";
import { AuthenticationResult } from "../response/AuthenticationResult";
import * as ResponseHandler from "../response/ResponseHandler";

export type PopupParams = {
popup?: Window | null;
Expand Down Expand Up @@ -284,10 +284,19 @@ export class PopupClient extends StandardInteractionClient {
);

// Monitor the window for the hash. Return the string value and close the popup when the hash is received. Default timeout is 60 seconds.
const hash = await this.monitorPopupForHash(popupWindow);
// Deserialize hash fragment response parameters.
const serverParams: ServerAuthorizationCodeResponse =
UrlString.getDeserializedHash(hash);
const responseString = await this.monitorPopupForHash(popupWindow);

const serverParams = invoke(
ResponseHandler.deserializeResponse,
PerformanceEvents.DeserializeResponse,
this.logger,
this.performanceClient,
this.correlationId
)(
responseString,
this.config.auth.OIDCOptions.serverResponseType,
this.logger
);
// Remove throttle if it exists
ThrottlingUtils.removeThrottle(
this.browserStorage,
Expand Down Expand Up @@ -338,8 +347,8 @@ export class PopupClient extends StandardInteractionClient {
}

// Handle response from hash string.
const result = await interactionHandler.handleCodeResponseFromHash(
hash,
const result = await interactionHandler.handleCodeResponse(
serverParams,
validRequest
);

Expand Down Expand Up @@ -464,7 +473,9 @@ export class PopupClient extends StandardInteractionClient {
null
);

await this.waitForLogoutPopup(popupWindow);
await this.monitorPopupForHash(popupWindow).catch(() => {
// Swallow any errors related to monitoring the window. Server logout is best effort
});

if (mainWindowRedirectUri) {
const navigationOptions: NavigationOptions = {
Expand Down Expand Up @@ -545,16 +556,7 @@ export class PopupClient extends StandardInteractionClient {
* @param timeout - timeout for processing hash once popup is redirected back to application
*/
monitorPopupForHash(popupWindow: Window): Promise<string> {
return new Promise((resolve, reject) => {
/*
* Polling for popups needs to be tick-based,
* since a non-trivial amount of time can be spent on interaction (which should not count against the timeout).
*/
const maxTicks =
this.config.system.windowHashTimeout /
this.config.system.pollIntervalMilliseconds;
let ticks = 0;

return new Promise<string>((resolve, reject) => {
this.logger.verbose(
"PopupHandler.monitorPopupForHash - polling started"
);
Expand All @@ -565,7 +567,6 @@ export class PopupClient extends StandardInteractionClient {
this.logger.error(
"PopupHandler.monitorPopupForHash - window closed"
);
this.cleanPopup();
clearInterval(intervalId);
reject(
createBrowserAuthError(
Expand All @@ -575,126 +576,41 @@ export class PopupClient extends StandardInteractionClient {
return;
}

let href = Constants.EMPTY_STRING;
let serverResponseString = Constants.EMPTY_STRING;
let href = "";
try {
/*
* Will throw if cross origin,
* which should be caught and ignored
* since we need the interval to keep running while on STS UI.
*/
href = popupWindow.location.href;
serverResponseString =
this.extractServerResponseStringFromPopup(
popupWindow,
href
);
} catch (e) {}

// Don't process blank pages or cross domain
if (!href || href === "about:blank") {
return;
}
clearInterval(intervalId);

this.logger.verbose(
"PopupHandler.monitorPopupForHash - popup window is on same origin as caller"
);

/*
* Only run clock when we are on same domain for popups
* as popup operations can take a long time.
*/
ticks++;

if (serverResponseString) {
this.logger.verbose(
"PopupHandler.monitorPopupForHash - found hash in url"
);
clearInterval(intervalId);
this.cleanPopup(popupWindow);

if (
UrlString.hashContainsKnownProperties(
serverResponseString
)
) {
this.logger.verbose(
"PopupHandler.monitorPopupForHash - hash contains known properties, returning."
);
resolve(serverResponseString);
let responseString = "";
const responseType =
this.config.auth.OIDCOptions.serverResponseType;
if (popupWindow) {
if (responseType === ServerResponseType.QUERY) {
responseString = popupWindow.location.search;
} else {
this.logger.error(
"PopupHandler.monitorPopupForHash - found hash in url but it does not contain known properties. Check that your router is not changing the hash prematurely."
);
this.logger.errorPii(
`PopupHandler.monitorPopupForHash - hash found: ${serverResponseString}`
);
reject(
createBrowserAuthError(
BrowserAuthErrorCodes.hashDoesNotContainKnownProperties
)
);
responseString = popupWindow.location.hash;
}
} else if (ticks > maxTicks) {
this.logger.error(
"PopupHandler.monitorPopupForHash - unable to find hash in url, timing out"
);
clearInterval(intervalId);
reject(
createBrowserAuthError(
BrowserAuthErrorCodes.monitorPopupTimeout
)
);
}
}, this.config.system.pollIntervalMilliseconds);
});
}

/**
* Waits for user interaction in logout popup window
* @param popupWindow
* @returns
*/
waitForLogoutPopup(popupWindow: Window): Promise<void> {
return new Promise((resolve) => {
this.logger.verbose(
"PopupHandler.waitForLogoutPopup - polling started"
);

const intervalId = setInterval(() => {
// Window is closed
if (popupWindow.closed) {
this.logger.error(
"PopupHandler.waitForLogoutPopup - window closed"
);
this.cleanPopup();
clearInterval(intervalId);
resolve();
}

let href: string = Constants.EMPTY_STRING;
try {
/*
* Will throw if cross origin,
* which should be caught and ignored
* since we need the interval to keep running while on STS UI.
*/
href = popupWindow.location.href;
} catch (e) {}

// Don't process blank pages or cross domain
if (!href || href === "about:blank") {
return;
}

this.logger.verbose(
"PopupHandler.waitForLogoutPopup - popup window is on same origin as caller, closing."
"PopupHandler.monitorPopupForHash - popup window is on same origin as caller"
);

clearInterval(intervalId);
this.cleanPopup(popupWindow);
resolve();
resolve(responseString);
}, this.config.system.pollIntervalMilliseconds);
}).finally(() => {
this.cleanPopup(popupWindow);
});
}

Expand Down Expand Up @@ -884,23 +800,4 @@ export class PopupClient extends StandardInteractionClient {
const homeAccountId = request.account && request.account.homeAccountId;
return `${BrowserConstants.POPUP_NAME_PREFIX}.${this.config.auth.clientId}.${homeAccountId}.${this.correlationId}`;
}

/**
* Extracts the server response from the popup window
*/
extractServerResponseStringFromPopup(
popupWindow: Window,
href: string
): string {
let serverResponseString;
if (
this.config.auth.OIDCOptions?.serverResponseType ===
ServerResponseType.QUERY
) {
serverResponseString = UrlString.parseQueryServerResponse(href);
} else {
serverResponseString = popupWindow.location.hash;
}
return serverResponseString;
}
}
Loading

0 comments on commit 619e10f

Please sign in to comment.