From 69abcdd32d2b2f69ac4b3905bd0c8ff1c537006b Mon Sep 17 00:00:00 2001 From: Jo Arroyo Date: Mon, 12 Aug 2024 19:43:31 -0700 Subject: [PATCH 01/10] Update onRedirectNavigate override, add test --- .../src/controllers/StandardController.ts | 42 ++++++++++++----- .../test/app/PublicClientApplication.spec.ts | 46 +++++++++++++++++++ 2 files changed, 76 insertions(+), 12 deletions(-) diff --git a/lib/msal-browser/src/controllers/StandardController.ts b/lib/msal-browser/src/controllers/StandardController.ts index 7ecb752980..5270c86056 100644 --- a/lib/msal-browser/src/controllers/StandardController.ts +++ b/lib/msal-browser/src/controllers/StandardController.ts @@ -584,19 +584,37 @@ export class StandardController implements IController { scenarioId: request.scenarioId, }); + // Override on request only if set, as onRedirectNavigate field is deprecated const onRedirectNavigateCb = request.onRedirectNavigate; - request.onRedirectNavigate = (url: string) => { - const navigate = - typeof onRedirectNavigateCb === "function" - ? onRedirectNavigateCb(url) - : undefined; - if (navigate !== false) { - atrMeasurement.end({ success: true }); - } else { - atrMeasurement.discard(); - } - return navigate; - }; + if (onRedirectNavigateCb) { + request.onRedirectNavigate = (url: string) => { + const navigate = + typeof onRedirectNavigateCb === "function" + ? onRedirectNavigateCb(url) + : undefined; + if (navigate !== false) { + atrMeasurement.end({ success: true }); + } else { + atrMeasurement.discard(); + } + return navigate; + }; + } else { + const configOnRedirectNavigateCb = + this.config.auth.onRedirectNavigate; + this.config.auth.onRedirectNavigate = (url: string) => { + const navigate = + typeof configOnRedirectNavigateCb === "function" + ? configOnRedirectNavigateCb(url) + : undefined; + if (navigate !== false) { + atrMeasurement.end({ success: true }); + } else { + atrMeasurement.discard(); + } + return navigate; + }; + } // If logged in, emit acquire token events const isLoggedIn = this.getAllAccounts().length > 0; diff --git a/lib/msal-browser/test/app/PublicClientApplication.spec.ts b/lib/msal-browser/test/app/PublicClientApplication.spec.ts index 2bf8b219f7..26b953a040 100644 --- a/lib/msal-browser/test/app/PublicClientApplication.spec.ts +++ b/lib/msal-browser/test/app/PublicClientApplication.spec.ts @@ -1900,6 +1900,52 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { pca.acquireTokenRedirect(loginRequest); }); + it("emits pre-redirect telemetry event when onRedirectNavigate callback is set in configuration", async () => { + const onRedirectNavigate = (url: string) => { + expect(url).toBeDefined(); + }; + + pca = new PublicClientApplication({ + auth: { + clientId: TEST_CONFIG.MSAL_CLIENT_ID, + onRedirectNavigate, + }, + telemetry: { + client: new BrowserPerformanceClient(testAppConfig), + application: { + appName: TEST_CONFIG.applicationName, + appVersion: TEST_CONFIG.applicationVersion, + }, + }, + }); + pca = (pca as any).controller; + await pca.initialize(); + + const callbackId = pca.addPerformanceCallback((events) => { + expect(events[0].success).toBe(true); + expect(events[0].name).toBe( + PerformanceEvents.AcquireTokenPreRedirect + ); + pca.removePerformanceCallback(callbackId); + }); + + jest.spyOn( + NavigationClient.prototype, + "navigateExternal" + ).mockImplementation(() => Promise.resolve(true)); + + jest.spyOn(PkceGenerator, "generatePkceCodes").mockResolvedValue({ + challenge: TEST_CONFIG.TEST_CHALLENGE, + verifier: TEST_CONFIG.TEST_VERIFIER, + }); + const loginRequest: RedirectRequest = { + redirectUri: TEST_URIS.TEST_REDIR_URI, + scopes: ["user.read", "openid", "profile"], + state: TEST_STATE_VALUES.USER_STATE, + }; + await pca.acquireTokenRedirect(loginRequest); + }); + it("discards pre-redirect telemetry event when onRedirectNavigate callback returns false", async () => { const onRedirectNavigate = (url: string) => { return false; From 6bed78fabc4d29ab563418b13f824e011884b7ea Mon Sep 17 00:00:00 2001 From: Jo Arroyo Date: Mon, 12 Aug 2024 19:44:55 -0700 Subject: [PATCH 02/10] Change files --- ...-msal-browser-92feeccb-6366-4bec-9fdd-4467a5cf21e9.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/@azure-msal-browser-92feeccb-6366-4bec-9fdd-4467a5cf21e9.json diff --git a/change/@azure-msal-browser-92feeccb-6366-4bec-9fdd-4467a5cf21e9.json b/change/@azure-msal-browser-92feeccb-6366-4bec-9fdd-4467a5cf21e9.json new file mode 100644 index 0000000000..833f22e2a2 --- /dev/null +++ b/change/@azure-msal-browser-92feeccb-6366-4bec-9fdd-4467a5cf21e9.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "onRedirectNavigate deprecation fix #7251", + "packageName": "@azure/msal-browser", + "email": "joarroyo@microsoft.com", + "dependentChangeType": "patch" +} From 45ee23f28306eada834b59491005a68084d910a7 Mon Sep 17 00:00:00 2001 From: Jo Arroyo Date: Tue, 13 Aug 2024 10:03:47 -0700 Subject: [PATCH 03/10] Fix back button cache clearing bug --- lib/msal-browser/src/interaction_client/RedirectClient.ts | 5 +++++ .../test/interaction_client/RedirectClient.spec.ts | 7 +++++++ 2 files changed, 12 insertions(+) diff --git a/lib/msal-browser/src/interaction_client/RedirectClient.ts b/lib/msal-browser/src/interaction_client/RedirectClient.ts index b723dcf056..77fae64061 100644 --- a/lib/msal-browser/src/interaction_client/RedirectClient.ts +++ b/lib/msal-browser/src/interaction_client/RedirectClient.ts @@ -119,6 +119,11 @@ export class RedirectClient extends StandardInteractionClient { "Page was restored from back/forward cache. Clearing temporary cache." ); this.browserStorage.cleanRequestByState(validRequest.state); + this.browserStorage.removeTemporaryItem( + this.browserStorage.generateCacheKey( + TemporaryCacheKeys.REDIRECT_REQUEST + ) + ); this.eventHandler.emitEvent( EventType.RESTORE_FROM_BFCACHE, InteractionType.Redirect diff --git a/lib/msal-browser/test/interaction_client/RedirectClient.spec.ts b/lib/msal-browser/test/interaction_client/RedirectClient.spec.ts index b9c333df43..748abf2b72 100644 --- a/lib/msal-browser/test/interaction_client/RedirectClient.spec.ts +++ b/lib/msal-browser/test/interaction_client/RedirectClient.spec.ts @@ -2420,6 +2420,13 @@ describe("RedirectClient", () => { ) ) ).toEqual(null); + expect( + browserStorage.getTemporaryCache( + browserStorage.generateCacheKey( + TemporaryCacheKeys.REDIRECT_REQUEST + ) + ) + ).toEqual(null); done(); return Promise.resolve(true); } From 28795ab833ae9a1e959214071d7a195221410769 Mon Sep 17 00:00:00 2001 From: Jo Arroyo Date: Tue, 13 Aug 2024 12:11:14 -0700 Subject: [PATCH 04/10] Adjust localStorage.spec --- .../app/customizable-e2e-test/test/localStorage.spec.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/samples/msal-browser-samples/VanillaJSTestApp2.0/app/customizable-e2e-test/test/localStorage.spec.ts b/samples/msal-browser-samples/VanillaJSTestApp2.0/app/customizable-e2e-test/test/localStorage.spec.ts index 5215a8503f..43d750f25c 100644 --- a/samples/msal-browser-samples/VanillaJSTestApp2.0/app/customizable-e2e-test/test/localStorage.spec.ts +++ b/samples/msal-browser-samples/VanillaJSTestApp2.0/app/customizable-e2e-test/test/localStorage.spec.ts @@ -125,6 +125,7 @@ describe("LocalStorage Tests", function () { await sessionBrowserStorage.getWindowStorage(); const localStorage = await BrowserCache.getWindowStorage(); expect(Object.keys(localStorage).length).toEqual(0); + console.log(`sessionStorage: ${JSON.stringify(sessionStorage)}`); expect(Object.keys(sessionStorage).length).toEqual(0); }, ONE_SECOND_IN_MS); }); From aa0cdde86b0455a8cdca83ea787f0e328da0c8ed Mon Sep 17 00:00:00 2001 From: Jo Arroyo Date: Tue, 13 Aug 2024 12:56:36 -0700 Subject: [PATCH 05/10] Add cache removal, fix unit tests --- lib/msal-browser/src/interaction_client/RedirectClient.ts | 5 +++++ .../test/interaction_client/RedirectClient.spec.ts | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/msal-browser/src/interaction_client/RedirectClient.ts b/lib/msal-browser/src/interaction_client/RedirectClient.ts index 77fae64061..dda2b47c7d 100644 --- a/lib/msal-browser/src/interaction_client/RedirectClient.ts +++ b/lib/msal-browser/src/interaction_client/RedirectClient.ts @@ -200,6 +200,11 @@ export class RedirectClient extends StandardInteractionClient { } window.removeEventListener("pageshow", handleBackButton); this.browserStorage.cleanRequestByState(validRequest.state); + this.browserStorage.removeTemporaryItem( + this.browserStorage.generateCacheKey( + TemporaryCacheKeys.REDIRECT_REQUEST + ) + ); throw e; } } diff --git a/lib/msal-browser/test/interaction_client/RedirectClient.spec.ts b/lib/msal-browser/test/interaction_client/RedirectClient.spec.ts index 748abf2b72..4bdd0cc365 100644 --- a/lib/msal-browser/test/interaction_client/RedirectClient.spec.ts +++ b/lib/msal-browser/test/interaction_client/RedirectClient.spec.ts @@ -2747,7 +2747,7 @@ describe("RedirectClient", () => { await redirectClient.acquireToken(emptyRequest); } catch (e) { // Test that error was cached for telemetry purposes and then thrown - expect(window.sessionStorage).toHaveLength(2); + expect(window.sessionStorage).toHaveLength(1); const failures = window.sessionStorage.getItem( `server-telemetry-${TEST_CONFIG.MSAL_CLIENT_ID}` ); @@ -3332,7 +3332,7 @@ describe("RedirectClient", () => { await redirectClient.acquireToken(emptyRequest); } catch (e) { // Test that error was cached for telemetry purposes and then thrown - expect(window.sessionStorage).toHaveLength(2); + expect(window.sessionStorage).toHaveLength(1); const failures = window.sessionStorage.getItem( `server-telemetry-${TEST_CONFIG.MSAL_CLIENT_ID}` ); From 2354d67528d902feba93b9ad278ae3fe961f69c5 Mon Sep 17 00:00:00 2001 From: Jo Arroyo Date: Tue, 13 Aug 2024 13:37:11 -0700 Subject: [PATCH 06/10] Add remove redirect request to resetRequestCache --- lib/msal-browser/src/cache/BrowserCacheManager.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/msal-browser/src/cache/BrowserCacheManager.ts b/lib/msal-browser/src/cache/BrowserCacheManager.ts index 99c34b319f..2906978945 100644 --- a/lib/msal-browser/src/cache/BrowserCacheManager.ts +++ b/lib/msal-browser/src/cache/BrowserCacheManager.ts @@ -1552,6 +1552,9 @@ export class BrowserCacheManager extends CacheManager { this.removeTemporaryItem( this.generateCacheKey(TemporaryCacheKeys.NATIVE_REQUEST) ); + this.removeTemporaryItem( + this.generateCacheKey(TemporaryCacheKeys.REDIRECT_REQUEST) + ); this.setInteractionInProgress(false); } From 19fa5e5bc2b16d826a40d5fc3b0241b9be7d2561 Mon Sep 17 00:00:00 2001 From: Jo Arroyo Date: Tue, 13 Aug 2024 15:01:14 -0700 Subject: [PATCH 07/10] Clean up and remove removeTemporaryCache for redirect request usage, add tests --- .../src/interaction_client/RedirectClient.ts | 15 ---------- .../interaction_handler/RedirectHandler.ts | 5 ---- .../RedirectHandler.spec.ts | 28 +++++++++++++++++++ .../test/localStorage.spec.ts | 1 - 4 files changed, 28 insertions(+), 21 deletions(-) diff --git a/lib/msal-browser/src/interaction_client/RedirectClient.ts b/lib/msal-browser/src/interaction_client/RedirectClient.ts index dda2b47c7d..21290731ae 100644 --- a/lib/msal-browser/src/interaction_client/RedirectClient.ts +++ b/lib/msal-browser/src/interaction_client/RedirectClient.ts @@ -119,11 +119,6 @@ export class RedirectClient extends StandardInteractionClient { "Page was restored from back/forward cache. Clearing temporary cache." ); this.browserStorage.cleanRequestByState(validRequest.state); - this.browserStorage.removeTemporaryItem( - this.browserStorage.generateCacheKey( - TemporaryCacheKeys.REDIRECT_REQUEST - ) - ); this.eventHandler.emitEvent( EventType.RESTORE_FROM_BFCACHE, InteractionType.Redirect @@ -200,11 +195,6 @@ export class RedirectClient extends StandardInteractionClient { } window.removeEventListener("pageshow", handleBackButton); this.browserStorage.cleanRequestByState(validRequest.state); - this.browserStorage.removeTemporaryItem( - this.browserStorage.generateCacheKey( - TemporaryCacheKeys.REDIRECT_REQUEST - ) - ); throw e; } } @@ -394,11 +384,6 @@ export class RedirectClient extends StandardInteractionClient { return null; } - this.browserStorage.removeTemporaryItem( - this.browserStorage.generateCacheKey( - TemporaryCacheKeys.REDIRECT_REQUEST - ) - ); this.browserStorage.removeRequestRetried(); throw e; } finally { diff --git a/lib/msal-browser/src/interaction_handler/RedirectHandler.ts b/lib/msal-browser/src/interaction_handler/RedirectHandler.ts index 0937d242fc..46f527ad00 100644 --- a/lib/msal-browser/src/interaction_handler/RedirectHandler.ts +++ b/lib/msal-browser/src/interaction_handler/RedirectHandler.ts @@ -223,11 +223,6 @@ export class RedirectHandler { this.browserStorage.cleanRequestByState(state); this.browserStorage.removeRequestRetried(); - this.browserStorage.removeTemporaryItem( - this.browserStorage.generateCacheKey( - TemporaryCacheKeys.REDIRECT_REQUEST - ) - ); return tokenResponse; } diff --git a/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts b/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts index 1eadc8b645..8f1e7db0a5 100644 --- a/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts +++ b/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts @@ -54,6 +54,8 @@ import { DatabaseStorage } from "../../src/cache/DatabaseStorage"; import { BrowserCacheManager } from "../../src/cache/BrowserCacheManager"; import { NavigationClient } from "../../src/navigation/NavigationClient"; import { NavigationOptions } from "../../src/navigation/NavigationOptions"; +import { RedirectRequest } from "../../src/request/RedirectRequest"; +import { base64Encode } from "../../src/encode/Base64Encode"; const testPkceCodes = { challenge: "TestChallenge", @@ -444,6 +446,24 @@ describe("RedirectHandler.ts Unit Tests", () => { browserStorage.generateCacheKey(TemporaryCacheKeys.URL_HASH), TEST_HASHES.TEST_SUCCESS_CODE_HASH_REDIRECT ); + browserStorage.setItem( + `${Constants.CACHE_PREFIX}.${TemporaryCacheKeys.REQUEST_RETRY}.${TEST_CONFIG.MSAL_CLIENT_ID}`, + JSON.stringify(1) + ); + const testRedirectRequest: RedirectRequest = { + redirectUri: TEST_URIS.TEST_REDIR_URI, + scopes: TEST_CONFIG.DEFAULT_SCOPES, + correlationId: TEST_CONFIG.CORRELATION_ID, + state: TEST_STATE_VALUES.USER_STATE, + authority: TEST_CONFIG.validAuthority, + nonce: "", + authenticationScheme: + TEST_CONFIG.TOKEN_TYPE_BEARER as AuthenticationScheme, + }; + browserStorage.setItem( + `${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.REDIRECT_REQUEST}`, + base64Encode(JSON.stringify(testRedirectRequest)) + ); sinon .stub( AuthorizationCodeClient.prototype, @@ -481,6 +501,14 @@ describe("RedirectHandler.ts Unit Tests", () => { browserStorage.generateCacheKey(TemporaryCacheKeys.URL_HASH) ) ).toBe(null); + expect( + browserStorage.getTemporaryCache( + browserStorage.generateCacheKey(TemporaryCacheKeys.REDIRECT_REQUEST) + ) + ).toBe(null); + expect( + browserStorage.getRequestRetried() + ).toBe(null); }); it("successfully handles response adds CCS credential to auth code request", async () => { diff --git a/samples/msal-browser-samples/VanillaJSTestApp2.0/app/customizable-e2e-test/test/localStorage.spec.ts b/samples/msal-browser-samples/VanillaJSTestApp2.0/app/customizable-e2e-test/test/localStorage.spec.ts index 43d750f25c..5215a8503f 100644 --- a/samples/msal-browser-samples/VanillaJSTestApp2.0/app/customizable-e2e-test/test/localStorage.spec.ts +++ b/samples/msal-browser-samples/VanillaJSTestApp2.0/app/customizable-e2e-test/test/localStorage.spec.ts @@ -125,7 +125,6 @@ describe("LocalStorage Tests", function () { await sessionBrowserStorage.getWindowStorage(); const localStorage = await BrowserCache.getWindowStorage(); expect(Object.keys(localStorage).length).toEqual(0); - console.log(`sessionStorage: ${JSON.stringify(sessionStorage)}`); expect(Object.keys(sessionStorage).length).toEqual(0); }, ONE_SECOND_IN_MS); }); From ead44ee9db2691a126b66b293480b7770bf1d7ce Mon Sep 17 00:00:00 2001 From: Jo Arroyo Date: Tue, 13 Aug 2024 15:06:01 -0700 Subject: [PATCH 08/10] Remove base64encoding for unit tests --- .../test/interaction_client/RedirectClient.spec.ts | 4 ++-- .../test/interaction_handler/RedirectHandler.spec.ts | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/msal-browser/test/interaction_client/RedirectClient.spec.ts b/lib/msal-browser/test/interaction_client/RedirectClient.spec.ts index 4bdd0cc365..34944d0976 100644 --- a/lib/msal-browser/test/interaction_client/RedirectClient.spec.ts +++ b/lib/msal-browser/test/interaction_client/RedirectClient.spec.ts @@ -486,7 +486,7 @@ describe("RedirectClient", () => { }; window.sessionStorage.setItem( `${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.REDIRECT_REQUEST}`, - base64Encode(JSON.stringify(testRedirectRequest)) + JSON.stringify(testRedirectRequest) ); const testTokenReq: CommonAuthorizationCodeRequest = { redirectUri: `${TEST_URIS.DEFAULT_INSTANCE}/`, @@ -2034,7 +2034,7 @@ describe("RedirectClient", () => { }; window.sessionStorage.setItem( `${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.REDIRECT_REQUEST}`, - base64Encode(JSON.stringify(testRedirectRequest)) + JSON.stringify(testRedirectRequest) ); const testTokenReq: CommonAuthorizationCodeRequest = { redirectUri: `${TEST_URIS.DEFAULT_INSTANCE}/`, diff --git a/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts b/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts index 8f1e7db0a5..918abb956b 100644 --- a/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts +++ b/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts @@ -55,7 +55,6 @@ import { BrowserCacheManager } from "../../src/cache/BrowserCacheManager"; import { NavigationClient } from "../../src/navigation/NavigationClient"; import { NavigationOptions } from "../../src/navigation/NavigationOptions"; import { RedirectRequest } from "../../src/request/RedirectRequest"; -import { base64Encode } from "../../src/encode/Base64Encode"; const testPkceCodes = { challenge: "TestChallenge", @@ -462,7 +461,7 @@ describe("RedirectHandler.ts Unit Tests", () => { }; browserStorage.setItem( `${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.REDIRECT_REQUEST}`, - base64Encode(JSON.stringify(testRedirectRequest)) + JSON.stringify(testRedirectRequest) ); sinon .stub( From 367a1981e7fa4aebc3c9c4d301b13d8cffe8f88e Mon Sep 17 00:00:00 2001 From: Jo Arroyo Date: Tue, 13 Aug 2024 15:19:05 -0700 Subject: [PATCH 09/10] Format fix --- .../test/interaction_handler/RedirectHandler.spec.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts b/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts index 918abb956b..2288fc1d91 100644 --- a/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts +++ b/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts @@ -502,12 +502,12 @@ describe("RedirectHandler.ts Unit Tests", () => { ).toBe(null); expect( browserStorage.getTemporaryCache( - browserStorage.generateCacheKey(TemporaryCacheKeys.REDIRECT_REQUEST) + browserStorage.generateCacheKey( + TemporaryCacheKeys.REDIRECT_REQUEST + ) ) ).toBe(null); - expect( - browserStorage.getRequestRetried() - ).toBe(null); + expect(browserStorage.getRequestRetried()).toBe(null); }); it("successfully handles response adds CCS credential to auth code request", async () => { From 51568a29f870b9ea8aed365e653cac302f81cbaa Mon Sep 17 00:00:00 2001 From: Jo Arroyo Date: Tue, 13 Aug 2024 15:53:56 -0700 Subject: [PATCH 10/10] Restore remove redirectrequest --- lib/msal-browser/src/interaction_client/RedirectClient.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/msal-browser/src/interaction_client/RedirectClient.ts b/lib/msal-browser/src/interaction_client/RedirectClient.ts index 21290731ae..b723dcf056 100644 --- a/lib/msal-browser/src/interaction_client/RedirectClient.ts +++ b/lib/msal-browser/src/interaction_client/RedirectClient.ts @@ -384,6 +384,11 @@ export class RedirectClient extends StandardInteractionClient { return null; } + this.browserStorage.removeTemporaryItem( + this.browserStorage.generateCacheKey( + TemporaryCacheKeys.REDIRECT_REQUEST + ) + ); this.browserStorage.removeRequestRetried(); throw e; } finally {