Skip to content

Commit

Permalink
onRedirectNavigate deprecation fix (#7251)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@jo-arroyo
jo-arroyo authored Aug 13, 2024
1 parent b588185 commit 8ad6d1d
Show file tree
Hide file tree
Showing 7 changed files with 124 additions and 21 deletions.
7 changes: 7 additions & 0 deletions change/@azure-msal-browser-92feeccb-6366-4bec-9fdd-4467a5cf21e9.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "onRedirectNavigate deprecation fix #7251",
"packageName": "@azure/msal-browser",
"email": "[email protected]",
"dependentChangeType": "patch"
}
3 changes: 3 additions & 0 deletions lib/msal-browser/src/cache/BrowserCacheManager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
42 changes: 30 additions & 12 deletions lib/msal-browser/src/controllers/StandardController.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
5 changes: 0 additions & 5 deletions lib/msal-browser/src/interaction_handler/RedirectHandler.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
46 changes: 46 additions & 0 deletions lib/msal-browser/test/app/PublicClientApplication.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
15 changes: 11 additions & 4 deletions lib/msal-browser/test/interaction_client/RedirectClient.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}/`,
Expand Down Expand Up @@ -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}/`,
Expand Down Expand Up @@ -2420,6 +2420,13 @@ describe("RedirectClient", () => {
)
)
).toEqual(null);
expect(
browserStorage.getTemporaryCache(
browserStorage.generateCacheKey(
TemporaryCacheKeys.REDIRECT_REQUEST
)
)
).toEqual(null);
done();
return Promise.resolve(true);
}
Expand Down Expand Up @@ -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}`
);
Expand Down Expand Up @@ -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}`
);
Expand Down
27 changes: 27 additions & 0 deletions lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 () => {
Expand Down

0 comments on commit 8ad6d1d

Please sign in to comment.