Improve interaction_in_progress reliability (#4460)

* improve reliability of interaction_in_progress

* move preflight check

* fix conditional

* Update tests

* Change files

change/@azure-msal-browser-84b5d046-5b4d-4e29-9397-8243e566813f.json

@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Improve reliability of interaction_in_progress #4460",
+  "packageName": "@azure/msal-browser",
+  "email": "thomas.norling@microsoft.com",
+  "dependentChangeType": "patch"
+}

lib/msal-browser/src/app/ClientApplication.ts

@@ -199,8 +199,8 @@ export abstract class ClientApplication {
      */
     async acquireTokenRedirect(request: RedirectRequest): Promise<void> {
         // Preflight request
-        this.preflightBrowserEnvironmentCheck(InteractionType.Redirect);
         this.logger.verbose("acquireTokenRedirect called");
+        this.preflightBrowserEnvironmentCheck(InteractionType.Redirect);
 
         // If logged in, emit acquire token events
         const isLoggedIn = this.getAllAccounts().length > 0;
@@ -235,8 +235,8 @@ export abstract class ClientApplication {
      */
     acquireTokenPopup(request: PopupRequest): Promise<AuthenticationResult> {
         try {
-            this.preflightBrowserEnvironmentCheck(InteractionType.Popup);
             this.logger.verbose("acquireTokenPopup called", request.correlationId);
+            this.preflightBrowserEnvironmentCheck(InteractionType.Popup);
         } catch (e) {
             // Since this function is syncronous we need to reject
             return Promise.reject(e);
@@ -554,6 +554,24 @@ export abstract class ClientApplication {
             !this.config.cache.storeAuthStateInCookie) {
             throw BrowserConfigurationAuthError.createInMemoryRedirectUnavailableError();
         }
+
+        if (interactionType === InteractionType.Redirect || interactionType === InteractionType.Popup) {
+            this.preflightInteractiveRequest();
+        }
+    }
+
+    /**
+     * Helper to validate app environment before making a request.
+     * @param request
+     * @param interactionType
+     */
+    protected preflightInteractiveRequest(): void {
+        this.logger.verbose("preflightInteractiveRequest called, validating app environment");
+        // block the reload if it occurred inside a hidden iframe
+        BrowserUtils.blockReloadInHiddenIframes();
+
+        // Set interaction in progress temporary cache or throw if alread set.
+        this.browserStorage.setInteractionInProgress(true);
     }
 
     /**

lib/msal-browser/src/cache/BrowserCacheManager.ts

@@ -909,13 +909,16 @@ export class BrowserCacheManager extends CacheManager {
     }
 
     setInteractionInProgress(inProgress: boolean): void {
-        const clientId = this.getInteractionInProgress();
         // Ensure we don't overwrite interaction in progress for a different clientId
         const key = `${Constants.CACHE_PREFIX}.${TemporaryCacheKeys.INTERACTION_STATUS_KEY}`;
-        if (inProgress && !clientId) {
-            // No interaction is in progress
-            this.setTemporaryCache(key, this.clientId, false);
-        } else if (!inProgress && clientId === this.clientId) {
+        if (inProgress) {
+            if (this.getInteractionInProgress()) {
+                throw BrowserAuthError.createInteractionInProgressError();
+            } else {
+                // No interaction is in progress
+                this.setTemporaryCache(key, this.clientId, false);
+            }
+        } else if (!inProgress && this.getInteractionInProgress() === this.clientId) {
             this.removeItem(key);
         }
     }

lib/msal-browser/src/interaction_client/PopupClient.ts

@@ -3,8 +3,7 @@
  * Licensed under the MIT License.
  */
 
-import { AuthenticationResult, CommonAuthorizationCodeRequest, AuthorizationCodeClient, ThrottlingUtils, CommonEndSessionRequest, UrlString, AuthError } from "@azure/msal-common";
-import { AuthorizationUrlRequest } from "../request/AuthorizationUrlRequest";
+import { AuthenticationResult, CommonAuthorizationCodeRequest, AuthorizationCodeClient, ThrottlingUtils, CommonEndSessionRequest, UrlString, AuthError, OIDC_DEFAULT_SCOPES } from "@azure/msal-common";
 import { StandardInteractionClient } from "./StandardInteractionClient";
 import { PopupWindowAttributes, PopupUtils } from "../utils/PopupUtils";
 import { EventType } from "../event/EventType";
@@ -20,22 +19,21 @@ export class PopupClient extends StandardInteractionClient {
      * Acquires tokens by opening a popup window to the /authorize endpoint of the authority
      * @param request 
      */
-    async acquireToken(request: PopupRequest): Promise<AuthenticationResult> {
+    acquireToken(request: PopupRequest): Promise<AuthenticationResult> {
         try {
-            const validRequest = await this.preflightInteractiveRequest(request, InteractionType.Popup);
-            const popupName = PopupUtils.generatePopupName(this.config.auth.clientId, validRequest);
+            const popupName = PopupUtils.generatePopupName(this.config.auth.clientId, request.scopes || OIDC_DEFAULT_SCOPES, request.authority || this.config.auth.authority, this.correlationId);
             const popupWindowAttributes = request.popupWindowAttributes || {};
 
             // asyncPopups flag is true. Acquires token without first opening popup. Popup will be opened later asynchronously.
             if (this.config.system.asyncPopups) {
                 this.logger.verbose("asyncPopups set to true, acquiring token");
                 // Passes on popup position and dimensions if in request
-                return this.acquireTokenPopupAsync(validRequest, popupName, popupWindowAttributes);
+                return this.acquireTokenPopupAsync(request, popupName, popupWindowAttributes);
             } else {
                 // asyncPopups flag is set to false. Opens popup before acquiring token.
                 this.logger.verbose("asyncPopup set to false, opening popup before acquiring token");
                 const popup = PopupUtils.openSizedPopup("about:blank", popupName, popupWindowAttributes, this.logger);
-                return this.acquireTokenPopupAsync(validRequest, popupName, popupWindowAttributes, popup);
+                return this.acquireTokenPopupAsync(request, popupName, popupWindowAttributes, popup);
             }
         } catch (e) {
             return Promise.reject(e);
@@ -82,9 +80,10 @@ export class PopupClient extends StandardInteractionClient {
      *
      * @returns A promise that is fulfilled when this function has completed, or rejected if an error was raised.
      */
-    private async acquireTokenPopupAsync(validRequest: AuthorizationUrlRequest, popupName: string, popupWindowAttributes: PopupWindowAttributes, popup?: Window|null): Promise<AuthenticationResult> {
+    private async acquireTokenPopupAsync(request: PopupRequest, popupName: string, popupWindowAttributes: PopupWindowAttributes, popup?: Window|null): Promise<AuthenticationResult> {
         this.logger.verbose("acquireTokenPopupAsync called");
         const serverTelemetryManager = this.initializeServerTelemetryManager(ApiId.acquireTokenPopup);
+        const validRequest = await this.initializeAuthorizationRequest(request, InteractionType.Popup);
 
         try {
             // Create auth code request and generate PKCE params
@@ -155,7 +154,6 @@ export class PopupClient extends StandardInteractionClient {
             // Clear cache on logout
             await this.clearCacheOnLogout(validRequest.account);
 
-            this.browserStorage.setInteractionInProgress(true);
             // Initialize the client
             const authClient = await this.createAuthCodeClient(serverTelemetryManager, requestAuthority);
             this.logger.verbose("Auth code client created");

lib/msal-browser/src/interaction_client/RedirectClient.ts

@@ -5,7 +5,6 @@
 
 import { AuthenticationResult, CommonAuthorizationCodeRequest, AuthorizationCodeClient, UrlString, AuthError, ServerTelemetryManager } from "@azure/msal-common";
 import { StandardInteractionClient } from "./StandardInteractionClient";
-import { AuthorizationUrlRequest } from "../request/AuthorizationUrlRequest";
 import { ApiId, InteractionType, TemporaryCacheKeys } from "../utils/BrowserConstants";
 import { RedirectHandler } from "../interaction_handler/RedirectHandler";
 import { BrowserUtils } from "../utils/BrowserUtils";
@@ -21,7 +20,7 @@ export class RedirectClient extends StandardInteractionClient {
      * @param request 
      */
     async acquireToken(request: RedirectRequest): Promise<void> {
-        const validRequest: AuthorizationUrlRequest = await this.preflightInteractiveRequest(request, InteractionType.Redirect);
+        const validRequest = await this.initializeAuthorizationRequest(request, InteractionType.Redirect);
         const serverTelemetryManager = this.initializeServerTelemetryManager(ApiId.acquireTokenRedirect);
 
         try {

lib/msal-browser/src/interaction_client/StandardInteractionClient.ts

@@ -59,11 +59,6 @@ export abstract class StandardInteractionClient extends BaseInteractionClient {
     protected initializeLogoutRequest(logoutRequest?: EndSessionRequest): CommonEndSessionRequest {
         this.logger.verbose("initializeLogoutRequest called", logoutRequest?.correlationId);
 
-        // Check if interaction is in progress. Throw error if true.
-        if (this.browserStorage.isInteractionInProgress()) {
-            throw BrowserAuthError.createInteractionInProgressError();
-        }
-
         const validLogoutRequest: CommonEndSessionRequest = {
             correlationId: this.browserCrypto.createNewGuid(),
             ...logoutRequest
@@ -234,24 +229,6 @@ export abstract class StandardInteractionClient extends BaseInteractionClient {
         return await AuthorityFactory.createDiscoveredInstance(this.config.auth.authority, this.config.system.networkClient, this.browserStorage, authorityOptions);
     }
 
-    /**
-     * Helper to validate app environment before making a request.
-     * @param request
-     * @param interactionType
-     */
-    protected async preflightInteractiveRequest(request: RedirectRequest|PopupRequest, interactionType: InteractionType): Promise<AuthorizationUrlRequest> {
-        this.logger.verbose("preflightInteractiveRequest called, validating app environment", request?.correlationId);
-        // block the reload if it occurred inside a hidden iframe
-        BrowserUtils.blockReloadInHiddenIframes();
-
-        // Check if interaction is in progress. Throw error if true.
-        if (this.browserStorage.isInteractionInProgress(false)) {
-            throw BrowserAuthError.createInteractionInProgressError();
-        }
-
-        return await this.initializeAuthorizationRequest(request, interactionType);
-    }
-
     /**
      * Helper to initialize required request parameters for interactive APIs and ssoSilent()
      * @param request

lib/msal-browser/src/interaction_handler/PopupHandler.ts

@@ -37,8 +37,6 @@ export class PopupHandler extends InteractionHandler {
     initiateAuthRequest(requestUrl: string, params: PopupParams): Window {
         // Check that request url is not empty.
         if (!StringUtils.isEmpty(requestUrl)) {
-            // Set interaction status in the library.
-            this.browserStorage.setInteractionInProgress(true);
             this.browserRequestLogger.infoPii(`Navigate to: ${requestUrl}`);
             // Open the popup window to requestUrl.
             return this.popupUtils.openPopup(requestUrl, params);

lib/msal-browser/src/interaction_handler/RedirectHandler.ts

@@ -42,7 +42,6 @@ export class RedirectHandler extends InteractionHandler {
             }
 
             // Set interaction status in the library.
-            this.browserStorage.setInteractionInProgress(true);
             this.browserStorage.setTemporaryCache(TemporaryCacheKeys.CORRELATION_ID, this.authCodeRequest.correlationId, true);
             this.browserStorage.cacheCodeRequest(this.authCodeRequest, this.browserCrypto);
             this.browserRequestLogger.infoPii(`RedirectHandler.initiateAuthRequest: Navigate to: ${requestUrl}`);

lib/msal-browser/src/utils/PopupUtils.ts

@@ -7,7 +7,6 @@ import { CommonEndSessionRequest, Constants, Logger, StringUtils } from "@azure/
 import { BrowserCacheManager } from "../cache/BrowserCacheManager";
 import { BrowserAuthError } from "../error/BrowserAuthError";
 import { PopupParams } from "../interaction_handler/PopupHandler";
-import { AuthorizationUrlRequest } from "../request/AuthorizationUrlRequest";
 import { BrowserConstants, InteractionType } from "./BrowserConstants";
 
 /**
@@ -204,8 +203,8 @@ export class PopupUtils {
      * @param clientId
      * @param request
      */
-    static generatePopupName(clientId: string, request: AuthorizationUrlRequest): string {
-        return `${BrowserConstants.POPUP_NAME_PREFIX}.${clientId}.${request.scopes.join("-")}.${request.authority}.${request.correlationId}`;
+    static generatePopupName(clientId: string, scopes: Array<string>, authority: string, correlationId: string): string {
+        return `${BrowserConstants.POPUP_NAME_PREFIX}.${clientId}.${scopes.join("-")}.${authority}.${correlationId}`;
     }
 
     /**

lib/msal-browser/test/app/PublicClientApplication.spec.ts

@@ -28,6 +28,13 @@ import { SilentRefreshClient } from "../../src/interaction_client/SilentRefreshC
 import { BrowserConfigurationAuthError } from "../../src";
 import { RedirectHandler } from "../../src/interaction_handler/RedirectHandler";
 import { SilentAuthCodeClient } from "../../src/interaction_client/SilentAuthCodeClient";
+import { BrowserCacheManager } from "../../src/cache/BrowserCacheManager";
+
+const cacheConfig = {
+    cacheLocation: BrowserCacheLocation.SessionStorage,
+    storeAuthStateInCookie: false,
+    secureCookies: false
+};
 
 describe("PublicClientApplication.ts Class Unit Tests", () => {
     let pca: PublicClientApplication;
@@ -314,6 +321,28 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {
     });
 
     describe("acquireTokenRedirect", () => {
+        it("throws if interaction is currently in progress", async () => {
+            const browserCrypto = new CryptoOps(new Logger({}));
+            const logger = new Logger({});
+            const browserStorage = new BrowserCacheManager("client-id", cacheConfig, browserCrypto, logger);
+            browserStorage.setInteractionInProgress(true);
+            await expect(pca.acquireTokenRedirect({scopes: ["openid"]})).rejects.toMatchObject(BrowserAuthError.createInteractionInProgressError());
+        });
+
+        it("throws if interaction is currently in progress for a different clientId", async () => {
+            const browserCrypto = new CryptoOps(new Logger({}));
+            const logger = new Logger({});
+            const browserStorage = new BrowserCacheManager("client-id", cacheConfig, browserCrypto, logger);
+            const secondInstanceStorage = new BrowserCacheManager("different-client-id", cacheConfig, browserCrypto, logger);
+            secondInstanceStorage.setInteractionInProgress(true);
+
+            expect(browserStorage.isInteractionInProgress(true)).toBe(false);
+            expect(browserStorage.isInteractionInProgress(false)).toBe(true);
+            expect(secondInstanceStorage.isInteractionInProgress(true)).toBe(true);
+            expect(secondInstanceStorage.isInteractionInProgress(false)).toBe(true);
+            await expect(pca.acquireTokenRedirect({scopes: ["openid"]})).rejects.toMatchObject(BrowserAuthError.createInteractionInProgressError());
+        });
+
         it("throws error if called in a popup", (done) => {
             const oldWindowOpener = window.opener;
             const oldWindowName = window.name;
@@ -492,6 +521,15 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {
             sinon.restore();
         });
 
+        it("throws error if interaction is in progress", async () => {
+            const browserCrypto = new CryptoOps(new Logger({}));
+            const logger = new Logger({});
+            const browserStorage = new BrowserCacheManager("client-id", cacheConfig, browserCrypto, logger);
+            browserStorage.setInteractionInProgress(true);
+
+            await expect(pca.acquireTokenPopup({scopes:[]})).rejects.toMatchObject(BrowserAuthError.createInteractionInProgressError());
+        });
+
         it("Calls PopupClient.acquireToken and returns its response", async () => {
             const testAccount: AccountInfo = {
                 homeAccountId: TEST_DATA_CLIENT_INFO.TEST_HOME_ACCOUNT_ID,
@@ -1416,6 +1454,15 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {
             expect(response).toEqual(undefined);
             expect(popupClientSpy.calledOnce).toBe(true);
         });
+
+        it("throws error if interaction is in progress", async () => {
+            const browserCrypto = new CryptoOps(new Logger({}));
+            const logger = new Logger({});
+            const browserStorage = new BrowserCacheManager("client-id", cacheConfig, browserCrypto, logger);
+            browserStorage.setInteractionInProgress(true);
+
+            await expect(pca.logoutPopup()).rejects.toMatchObject(BrowserAuthError.createInteractionInProgressError());
+        });
     });
 
     describe("getAccount tests", () => {

lib/msal-browser/test/interaction_client/PopupClient.spec.ts

@@ -7,15 +7,13 @@ import sinon from "sinon";
 import { PublicClientApplication } from "../../src/app/PublicClientApplication";
 import { TEST_CONFIG, TEST_URIS, TEST_HASHES, TEST_TOKENS, TEST_DATA_CLIENT_INFO, TEST_TOKEN_LIFETIMES, RANDOM_TEST_GUID, testNavUrl, TEST_STATE_VALUES, TEST_SSH_VALUES } from "../utils/StringConstants";
 import { Constants, AccountInfo, TokenClaims, AuthenticationResult, CommonAuthorizationUrlRequest, AuthorizationCodeClient, ResponseMode, AuthenticationScheme, ServerTelemetryEntity, AccountEntity, CommonEndSessionRequest, PersistentCacheKeys, ClientConfigurationError } from "@azure/msal-common";
-import { BrowserConstants, TemporaryCacheKeys, ApiId } from "../../src/utils/BrowserConstants";
-import { BrowserAuthError } from "../../src/error/BrowserAuthError";
+import { TemporaryCacheKeys, ApiId } from "../../src/utils/BrowserConstants";
 import { PopupHandler } from "../../src/interaction_handler/PopupHandler";
 import { CryptoOps } from "../../src/crypto/CryptoOps";
 import { NavigationClient } from "../../src/navigation/NavigationClient";
 import { PopupUtils } from "../../src/utils/PopupUtils";
 import { EndSessionPopupRequest } from "../../src/request/EndSessionPopupRequest";
 import { PopupClient } from "../../src/interaction_client/PopupClient";
-import { PopupRequest } from "../../src/request/PopupRequest";
 
 describe("PopupClient", () => {
     let popupClient: PopupClient;
@@ -52,12 +50,6 @@ describe("PopupClient", () => {
             sinon.restore();
         });
 
-        it("throws error if interaction is in progress", async () => {
-            window.sessionStorage.setItem(`${Constants.CACHE_PREFIX}.${TemporaryCacheKeys.INTERACTION_STATUS_KEY}`, TEST_CONFIG.MSAL_CLIENT_ID);
-
-            await expect(popupClient.acquireToken({scopes:[]})).rejects.toMatchObject(BrowserAuthError.createInteractionInProgressError());
-        });
-
         it("throws error when AuthenticationScheme is set to SSH and SSH JWK is omitted from the request", async () => {
             const request: CommonAuthorizationUrlRequest = {
                 redirectUri: TEST_URIS.TEST_REDIR_URI,
@@ -263,12 +255,6 @@ describe("PopupClient", () => {
             sinon.restore();
         });
 
-        it("throws error if interaction is in progress", async () => {
-            window.sessionStorage.setItem(`${Constants.CACHE_PREFIX}.${TemporaryCacheKeys.INTERACTION_STATUS_KEY}`, TEST_CONFIG.MSAL_CLIENT_ID);
-
-            await expect(popupClient.logout()).rejects.toMatchObject(BrowserAuthError.createInteractionInProgressError());
-        });
-
         it("opens popup window before network request by default", async () => {
             const popupSpy = sinon.stub(PopupUtils, "openSizedPopup");