Fix ATS request deduplication for pairwise broker requests (#7612)

acquireTokenSilent deduplicates identical requests fired off in
parallel. This logic has 2 bugs that may result in broker_timeout errors
in PWB flows.

1. Request thumbprint used for deduplication does not take
embeddedClientId into account, resulting in 2 requests originating from
different child apps being incorrectly deduped.
2. CorrelationId is shared for deduped requests, preventing embedded
apps from resolving the promise upon receiving the response from the
broker.

This PR addresses both of these issues and enables telemetry collection
for deduped requests (will need to filter these out in our
dashboards/queries)

---------

Co-authored-by: Sameera Gajjarapu <[email protected]>

Author: tnorling

change/@azure-msal-browser-ff22c8e9-d56a-4cee-a010-bc0e2d00ce5a.json

@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Fix acquireTokenSilent deduplication for pairwise broker requests",
+  "packageName": "@azure/msal-browser",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}

change/@azure-msal-common-46041a7e-4da2-4a81-93d9-1b2be924772e.json

@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Add embeddedClientId to RequestThumbprint",
+  "packageName": "@azure/msal-common",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}

lib/msal-browser/apiReview/msal-browser.api.md

@@ -1341,7 +1341,6 @@ export type PopupPosition = {
 export type PopupRequest = Partial<Omit<CommonAuthorizationUrlRequest, "responseMode" | "scopes" | "codeChallenge" | "codeChallengeMethod" | "requestedClaimsHash" | "platformBroker">> & {
     scopes: Array<string>;
     popupWindowAttributes?: PopupWindowAttributes;
-    tokenBodyParameters?: StringDict;
     popupWindowParent?: Window;
 };
 
@@ -1600,7 +1599,6 @@ export type RedirectRequest = Partial<Omit<CommonAuthorizationUrlRequest, "respo
     scopes: Array<string>;
     redirectStartPage?: string;
     onRedirectNavigate?: (url: string) => boolean | void;
-    tokenBodyParameters?: StringDict;
 };
 
 // Warning: (ae-missing-release-tag) "replaceHash" is part of the package's API, but it is missing a release tag (@alpha, @beta, @public, or @internal)
@@ -1679,7 +1677,6 @@ export type SilentRequest = Omit<CommonSilentFlowRequest, "authority" | "correla
     cacheLookupPolicy?: CacheLookupPolicy;
     prompt?: string;
     state?: string;
-    tokenBodyParameters?: StringDict;
 };
 
 // Warning: (ae-missing-release-tag) "spaCodeAndNativeAccountIdPresent" is part of the package's API, but it is missing a release tag (@alpha, @beta, @public, or @internal)
@@ -1690,9 +1687,7 @@ const spaCodeAndNativeAccountIdPresent = "spa_code_and_nativeAccountId_present";
 // Warning: (ae-missing-release-tag) "SsoSilentRequest" is part of the package's API, but it is missing a release tag (@alpha, @beta, @public, or @internal)
 //
 // @public
-export type SsoSilentRequest = Partial<Omit<CommonAuthorizationUrlRequest, "responseMode" | "codeChallenge" | "codeChallengeMethod" | "requestedClaimsHash" | "platformBroker">> & {
-    tokenBodyParameters?: StringDict;
-};
+export type SsoSilentRequest = Partial<Omit<CommonAuthorizationUrlRequest, "responseMode" | "codeChallenge" | "codeChallengeMethod" | "requestedClaimsHash" | "platformBroker">>;
 
 // Warning: (ae-missing-release-tag) "stateInteractionTypeMismatch" is part of the package's API, but it is missing a release tag (@alpha, @beta, @public, or @internal)
 //

lib/msal-browser/src/controllers/StandardController.ts

@@ -20,7 +20,7 @@ import {
     BaseAuthRequest,
     PromptValue,
     InProgressPerformanceEvent,
-    RequestThumbprint,
+    getRequestThumbprint,
     AccountEntity,
     invokeAsync,
     createClientAuthError,
@@ -1960,30 +1960,71 @@ export class StandardController implements IController {
         }
         atsMeasurement.add({ accountType: getAccountType(account) });
 
-        const thumbprint: RequestThumbprint = {
-            clientId: this.config.auth.clientId,
-            authority: request.authority || Constants.EMPTY_STRING,
-            scopes: request.scopes,
-            homeAccountIdentifier: account.homeAccountId,
-            claims: request.claims,
-            authenticationScheme: request.authenticationScheme,
-            resourceRequestMethod: request.resourceRequestMethod,
-            resourceRequestUri: request.resourceRequestUri,
-            shrClaims: request.shrClaims,
-            sshKid: request.sshKid,
-            shrOptions: request.shrOptions,
-        };
+        return this.acquireTokenSilentDeduped(request, account, correlationId)
+            .then((result) => {
+                atsMeasurement.end({
+                    success: true,
+                    fromCache: result.fromCache,
+                    isNativeBroker: result.fromNativeBroker,
+                    accessTokenSize: result.accessToken.length,
+                    idTokenSize: result.idToken.length,
+                });
+                return {
+                    ...result,
+                    state: request.state,
+                    correlationId: correlationId, // Ensures PWB scenarios can correctly match request to response
+                };
+            })
+            .catch((error: Error) => {
+                if (error instanceof AuthError) {
+                    // Ensures PWB scenarios can correctly match request to response
+                    error.setCorrelationId(correlationId);
+                }
+
+                atsMeasurement.end(
+                    {
+                        success: false,
+                    },
+                    error
+                );
+                throw error;
+            });
+    }
+
+    /**
+     * Checks if identical request is already in flight and returns reference to the existing promise or fires off a new one if this is the first
+     * @param request
+     * @param account
+     * @param correlationId
+     * @returns
+     */
+    private async acquireTokenSilentDeduped(
+        request: SilentRequest,
+        account: AccountInfo,
+        correlationId: string
+    ): Promise<AuthenticationResult> {
+        const thumbprint = getRequestThumbprint(
+            this.config.auth.clientId,
+            {
+                ...request,
+                authority: request.authority || this.config.auth.authority,
+                correlationId: correlationId,
+            },
+            account.homeAccountId
+        );
         const silentRequestKey = JSON.stringify(thumbprint);
 
-        const cachedResponse =
+        const inProgressRequest =
             this.activeSilentTokenRequests.get(silentRequestKey);
-        if (typeof cachedResponse === "undefined") {
+
+        if (typeof inProgressRequest === "undefined") {
             this.logger.verbose(
                 "acquireTokenSilent called for the first time, storing active request",
                 correlationId
             );
+            this.performanceClient.addFields({ deduped: false }, correlationId);
 
-            const response = invokeAsync(
+            const activeRequest = invokeAsync(
                 this.acquireTokenSilentAsync.bind(this),
                 PerformanceEvents.AcquireTokenSilentAsync,
                 this.logger,
@@ -1995,45 +2036,19 @@ export class StandardController implements IController {
                     correlationId,
                 },
                 account
-            )
-                .then((result) => {
-                    this.activeSilentTokenRequests.delete(silentRequestKey);
-                    atsMeasurement.end({
-                        success: true,
-                        fromCache: result.fromCache,
-                        isNativeBroker: result.fromNativeBroker,
-                        cacheLookupPolicy: request.cacheLookupPolicy,
-                        accessTokenSize: result.accessToken.length,
-                        idTokenSize: result.idToken.length,
-                    });
-                    return result;
-                })
-                .catch((error: Error) => {
-                    this.activeSilentTokenRequests.delete(silentRequestKey);
-                    atsMeasurement.end(
-                        {
-                            success: false,
-                        },
-                        error
-                    );
-                    throw error;
-                });
-            this.activeSilentTokenRequests.set(silentRequestKey, response);
-            return {
-                ...(await response),
-                state: request.state,
-            };
+            );
+            this.activeSilentTokenRequests.set(silentRequestKey, activeRequest);
+
+            return activeRequest.finally(() => {
+                this.activeSilentTokenRequests.delete(silentRequestKey);
+            });
         } else {
             this.logger.verbose(
                 "acquireTokenSilent has been called previously, returning the result from the first call",
                 correlationId
             );
-            // Discard measurements for memoized calls, as they are usually only a couple of ms and will artificially deflate metrics
-            atsMeasurement.discard();
-            return {
-                ...(await cachedResponse),
-                state: request.state,
-            };
+            this.performanceClient.addFields({ deduped: true }, correlationId);
+            return inProgressRequest;
         }
     }
 

lib/msal-browser/src/request/PopupRequest.ts

@@ -3,10 +3,7 @@
  * Licensed under the MIT License.
  */
 
-import {
-    CommonAuthorizationUrlRequest,
-    StringDict,
-} from "@azure/msal-common/browser";
+import { CommonAuthorizationUrlRequest } from "@azure/msal-common/browser";
 import { PopupWindowAttributes } from "./PopupWindowAttributes.js";
 
 /**
@@ -51,6 +48,5 @@ export type PopupRequest = Partial<
 > & {
     scopes: Array<string>;
     popupWindowAttributes?: PopupWindowAttributes;
-    tokenBodyParameters?: StringDict;
     popupWindowParent?: Window;
 };

lib/msal-browser/src/request/RedirectRequest.ts

@@ -3,10 +3,7 @@
  * Licensed under the MIT License.
  */
 
-import {
-    CommonAuthorizationUrlRequest,
-    StringDict,
-} from "@azure/msal-common/browser";
+import { CommonAuthorizationUrlRequest } from "@azure/msal-common/browser";
 
 /**
  * RedirectRequest: Request object passed by user to retrieve a Code from the
@@ -55,5 +52,4 @@ export type RedirectRequest = Partial<
      * Set onRedirectNavigate in Configuration instead.
      */
     onRedirectNavigate?: (url: string) => boolean | void;
-    tokenBodyParameters?: StringDict;
 };

lib/msal-browser/src/request/SilentRequest.ts

@@ -46,5 +46,4 @@ export type SilentRequest = Omit<
     cacheLookupPolicy?: CacheLookupPolicy;
     prompt?: string;
     state?: string;
-    tokenBodyParameters?: StringDict;
 };

lib/msal-browser/src/request/SsoSilentRequest.ts

@@ -3,10 +3,7 @@
  * Licensed under the MIT License.
  */
 
-import {
-    CommonAuthorizationUrlRequest,
-    StringDict,
-} from "@azure/msal-common/browser";
+import { CommonAuthorizationUrlRequest } from "@azure/msal-common/browser";
 
 /**
  * Request object passed by user to ssoSilent to retrieve a Code from the server (first leg of authorization code grant flow)
@@ -42,6 +39,4 @@ export type SsoSilentRequest = Partial<
         | "requestedClaimsHash"
         | "platformBroker"
     >
-> & {
-    tokenBodyParameters?: StringDict;
-};
+>;