From 8ad6d1d47aab7259e9bcad6b0c194fa20bf5c207 Mon Sep 17 00:00:00 2001 From: Jo Arroyo Date: Tue, 13 Aug 2024 16:20:21 -0700 Subject: [PATCH] onRedirectNavigate deprecation fix (#7251) This PR has two fixes: 1. Deprecating onRedirectNavigate on RedirectRequest requires additional changes on StandardController to ensure telemetry measurements are also being taken when onRedirectNavigate is set on the configuration. Tests are added in PublicClientApplication. 2. Temporary redirect request cache must also be cleared in the event of a back button being clicked during the redirect flow. Assertion added to existing test in RedirectClient. --- ...-92feeccb-6366-4bec-9fdd-4467a5cf21e9.json | 7 +++ .../src/cache/BrowserCacheManager.ts | 3 ++ .../src/controllers/StandardController.ts | 42 ++++++++++++----- .../interaction_handler/RedirectHandler.ts | 5 -- .../test/app/PublicClientApplication.spec.ts | 46 +++++++++++++++++++ .../interaction_client/RedirectClient.spec.ts | 15 ++++-- .../RedirectHandler.spec.ts | 27 +++++++++++ 7 files changed, 124 insertions(+), 21 deletions(-) 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" +} 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); } 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/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/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; diff --git a/lib/msal-browser/test/interaction_client/RedirectClient.spec.ts b/lib/msal-browser/test/interaction_client/RedirectClient.spec.ts index b9c333df43..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}/`, @@ -2420,6 +2420,13 @@ describe("RedirectClient", () => { ) ) ).toEqual(null); + expect( + browserStorage.getTemporaryCache( + browserStorage.generateCacheKey( + TemporaryCacheKeys.REDIRECT_REQUEST + ) + ) + ).toEqual(null); done(); return Promise.resolve(true); } @@ -2740,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}` ); @@ -3325,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}` ); diff --git a/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts b/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts index 1eadc8b645..2288fc1d91 100644 --- a/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts +++ b/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts @@ -54,6 +54,7 @@ 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"; const testPkceCodes = { challenge: "TestChallenge", @@ -444,6 +445,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}`, + JSON.stringify(testRedirectRequest) + ); sinon .stub( AuthorizationCodeClient.prototype, @@ -481,6 +500,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 () => {