Address feedback

AzureAD · Aug 2, 2024 · 5adf351 · 5adf351
1 parent 1235033
commit 5adf351
Show file tree

Hide file tree

Showing 5 changed files with 38 additions and 9 deletions.
diff --git a/lib/msal-browser/src/cache/BrowserCacheManager.ts b/lib/msal-browser/src/cache/BrowserCacheManager.ts
@@ -1655,7 +1655,8 @@ export class BrowserCacheManager extends CacheManager {
      */
     cacheRedirectRequest(redirectRequest: RedirectRequest): void {
         this.logger.trace("BrowserCacheManager.cacheRedirectRequest called");
-        const encodedValue = base64Encode(JSON.stringify(redirectRequest));
+        const { onRedirectNavigate, ...restParams } = redirectRequest;
+        const encodedValue = base64Encode(JSON.stringify(restParams));
 
         this.setTemporaryCache(
             TemporaryCacheKeys.REDIRECT_REQUEST,

diff --git a/lib/msal-browser/src/error/BrowserAuthError.ts b/lib/msal-browser/src/error/BrowserAuthError.ts
@@ -92,7 +92,7 @@ export const BrowserAuthErrorMessages = {
         "Invalid base64 encoded string.",
     [BrowserAuthErrorCodes.invalidPopTokenRequest]:
         "Invalid PoP token request. The request should not have both a popKid value and signPopToken set to true.",
-    [BrowserAuthErrorCodes.noAutoRetry]:
+    [BrowserAuthErrorCodes.failedToRetry]:
         "Unable to retry failed auth code redemption due to usage of the onRedirectNavigate request parameter. Please set onRedirectNavigate on the PublicClientApplication configuration instead or call loginRedirect again.",
 };
 

diff --git a/lib/msal-browser/src/interaction_client/RedirectClient.ts b/lib/msal-browser/src/interaction_client/RedirectClient.ts
@@ -373,13 +373,14 @@ export class RedirectClient extends StandardInteractionClient {
                     throw e;
                 }
 
-                if (request.onRedirectNavigate) {
+                const onRedirectNavigate = this.config.auth.onRedirectNavigate;
+                if (!onRedirectNavigate) {
                     this.logger.error(
-                        `Unable to retry redirect request due to presence of onRedirectNavigate request parameter. Please retry with redirect request and correlationId: ${this.correlationId}`
+                        `Unable to retry redirect request without onRedirectNavigate parameter. Please retry with redirect request and correlationId: ${this.correlationId}`
                     );
                     this.browserStorage.setRequestRetried(this.correlationId);
                     throw createBrowserAuthError(
-                        BrowserAuthErrorCodes.noAutoRetry
+                        BrowserAuthErrorCodes.failedToRetry
                     );
                 }
 

diff --git a/lib/msal-browser/src/interaction_handler/RedirectHandler.ts b/lib/msal-browser/src/interaction_handler/RedirectHandler.ts
@@ -225,6 +225,11 @@ export class RedirectHandler {
         this.browserStorage.removeRequestRetried(
             this.authCodeRequest.correlationId
         );
+        this.browserStorage.removeTemporaryItem(
+            this.browserStorage.generateCacheKey(
+                TemporaryCacheKeys.REDIRECT_REQUEST
+            )
+        );
         return tokenResponse;
     }
 

diff --git a/lib/msal-browser/test/interaction_client/RedirectClient.spec.ts b/lib/msal-browser/test/interaction_client/RedirectClient.spec.ts
@@ -474,6 +474,20 @@ describe("RedirectClient", () => {
                 `${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.NONCE_IDTOKEN}.${stateId}`,
                 "123523"
             );
+            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,
+            };
+            window.sessionStorage.setItem(
+                `${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.REDIRECT_REQUEST}`,
+                base64Encode(JSON.stringify(testRedirectRequest))
+            );
             const testTokenReq: CommonAuthorizationCodeRequest = {
                 redirectUri: `${TEST_URIS.DEFAULT_INSTANCE}/`,
                 code: "thisIsATestCode",
@@ -558,6 +572,14 @@ describe("RedirectClient", () => {
                 testTokenResponse.expiresOn.getMilliseconds() >=
                 tokenResponse.expiresOn.getMilliseconds()
             ).toBeTruthy();
+            expect(
+                browserStorage.getTemporaryCache(
+                    TemporaryCacheKeys.REDIRECT_REQUEST
+                )
+            ).toEqual(null);
+            expect(
+                browserStorage.getRequestRetried(TEST_CONFIG.CORRELATION_ID)
+            ).toEqual(null);
         });
 
         it("gets hash from cache and calls native broker if hash contains accountId", async () => {
@@ -1835,7 +1857,7 @@ describe("RedirectClient", () => {
             let pca = new PublicClientApplication({
                 auth: {
                     clientId: TEST_CONFIG.MSAL_CLIENT_ID,
-                    onRedirectNavigate: (url: string) => { },
+                    onRedirectNavigate: (url: string) => {},
                 },
             });
 
@@ -2177,7 +2199,7 @@ describe("RedirectClient", () => {
                 });
         });
 
-        it("throws no_redirect_request_config_error if invalid_grant is returned from server and onRedirectNavigate is not set in config", (done) => {
+        it("throws failed_to_retry if invalid_grant is returned from server and onRedirectNavigate is not set in config", (done) => {
             const stateString = TEST_STATE_VALUES.TEST_STATE_REDIRECT;
             const browserCrypto = new CryptoOps(new Logger({}));
             const stateId = ProtocolUtils.parseRequestState(
@@ -2269,7 +2291,7 @@ describe("RedirectClient", () => {
                 .handleRedirectPromise("", rootMeasurement)
                 .catch((err) => {
                     expect(err instanceof AuthError).toBeTruthy();
-                    expect(err.errorCode).toEqual("no_auto_retry_error");
+                    expect(err.errorCode).toEqual("failed_to_retry");
                     expect(acquireTokenSpy).toHaveBeenCalledTimes(0);
 
                     expect(
@@ -2687,7 +2709,7 @@ describe("RedirectClient", () => {
                 nonce: "",
                 authenticationScheme:
                     TEST_CONFIG.TOKEN_TYPE_BEARER as AuthenticationScheme,
-                onRedirectNavigate: (url: string) => { },
+                onRedirectNavigate: (url: string) => {},
             };
 
             const browserCrypto = new CryptoOps(new Logger({}));