Update retry for invalid_grant errors (#7249)

This PR: - Updates `RedirectClient` and `BrowserCacheManager` and tests to use the client ID instead of the correlation ID when caching retry value. - Cleans up error logic in `SilentIframeClient` and `PopupClient`.
AzureAD · Aug 12, 2024 · b588185 · b588185
1 parent 3c8bde6
commit b588185
Show file tree

Hide file tree

Showing 7 changed files with 96 additions and 123 deletions.
diff --git a/change/@azure-msal-browser-3a4cabb3-b4c2-46c4-a275-e6c3352004fd.json b/change/@azure-msal-browser-3a4cabb3-b4c2-46c4-a275-e6c3352004fd.json
@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Update request retry for invalid_grant #7249",
+  "packageName": "@azure/msal-browser",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}
diff --git a/lib/msal-browser/src/cache/BrowserCacheManager.ts b/lib/msal-browser/src/cache/BrowserCacheManager.ts
@@ -1615,18 +1615,16 @@ export class BrowserCacheManager extends CacheManager {
 
     /**
      * Create request retry key to cache retry status
-     * @param correlationId
      */
-    generateRequestRetriedKey(correlationId: string): string {
-        return `${Constants.CACHE_PREFIX}.${TemporaryCacheKeys.REQUEST_RETRY}.${correlationId}`;
+    generateRequestRetriedKey(): string {
+        return `${Constants.CACHE_PREFIX}.${TemporaryCacheKeys.REQUEST_RETRY}.${this.clientId}`;
     }
 
     /**
      * Gets the request retry value from the cache
-     * @param correlationId
      */
-    getRequestRetried(correlationId: string): number | null {
-        const requestRetriedKey = this.generateRequestRetriedKey(correlationId);
+    getRequestRetried(): number | null {
+        const requestRetriedKey = this.generateRequestRetriedKey();
         const cachedRetryNumber = this.getTemporaryCache(requestRetriedKey);
         if (!cachedRetryNumber) {
             return null;
@@ -1636,23 +1634,19 @@ export class BrowserCacheManager extends CacheManager {
 
     /**
      * Sets the request retry value to "retried" in the cache
-     * @param correlationId
      */
-    setRequestRetried(correlationId: string): void {
+    setRequestRetried(): void {
         this.logger.trace("BrowserCacheManager.setRequestRetried called");
-        const requestRetriedKey = this.generateRequestRetriedKey(correlationId);
+        const requestRetriedKey = this.generateRequestRetriedKey();
         this.setTemporaryCache(requestRetriedKey, "1", false);
     }
 
     /**
      * Removes all request retry values in the cache
      */
     removeRequestRetried(): void {
-        this.temporaryCacheStorage.getKeys().forEach((key) => {
-            if (key.indexOf(TemporaryCacheKeys.REQUEST_RETRY) !== -1) {
-                this.removeTemporaryItem(key);
-            }
-        });
+        const requestRetriedKey = this.generateRequestRetriedKey();
+        this.removeTemporaryItem(requestRetriedKey);
     }
 
     /**

diff --git a/lib/msal-browser/src/interaction_client/PopupClient.ts b/lib/msal-browser/src/interaction_client/PopupClient.ts
@@ -266,46 +266,43 @@ export class PopupClient extends StandardInteractionClient {
             }
 
             if (
-                e instanceof ServerError &&
-                e.errorCode === BrowserConstants.INVALID_GRANT_ERROR
+                !authClient ||
+                !(e instanceof ServerError) ||
+                e.errorCode !== BrowserConstants.INVALID_GRANT_ERROR
             ) {
-                if (!authClient) {
-                    throw e;
-                } else {
-                    this.performanceClient.addFields(
-                        {
-                            retryError: e.errorCode,
-                        },
-                        this.correlationId
-                    );
-
-                    const retryAuthCodeRequest = await invokeAsync(
-                        this.initializeAuthorizationCodeRequest.bind(this),
-                        PerformanceEvents.StandardInteractionClientInitializeAuthorizationCodeRequest,
-                        this.logger,
-                        this.performanceClient,
-                        this.correlationId
-                    )(validRequest);
-
-                    return await invokeAsync(
-                        this.acquireTokenPopupAsyncHelper.bind(this),
-                        PerformanceEvents.PopupClientTokenHelper,
-                        this.logger,
-                        this.performanceClient,
-                        this.correlationId
-                    )(
-                        authClient,
-                        retryAuthCodeRequest,
-                        validRequest,
-                        request,
-                        popupName,
-                        popupWindowAttributes,
-                        popup
-                    );
-                }
+                throw e;
             }
 
-            throw e;
+            this.performanceClient.addFields(
+                {
+                    retryError: e.errorCode,
+                },
+                this.correlationId
+            );
+
+            const retryAuthCodeRequest = await invokeAsync(
+                this.initializeAuthorizationCodeRequest.bind(this),
+                PerformanceEvents.StandardInteractionClientInitializeAuthorizationCodeRequest,
+                this.logger,
+                this.performanceClient,
+                this.correlationId
+            )(validRequest);
+
+            return await invokeAsync(
+                this.acquireTokenPopupAsyncHelper.bind(this),
+                PerformanceEvents.PopupClientTokenHelper,
+                this.logger,
+                this.performanceClient,
+                this.correlationId
+            )(
+                authClient,
+                retryAuthCodeRequest,
+                validRequest,
+                request,
+                popupName,
+                popupWindowAttributes,
+                popup
+            );
         }
     }
 

diff --git a/lib/msal-browser/src/interaction_client/RedirectClient.ts b/lib/msal-browser/src/interaction_client/RedirectClient.ts
@@ -356,9 +356,7 @@ export class RedirectClient extends StandardInteractionClient {
                     this.correlationId
                 );
 
-                const requestRetried = this.browserStorage.getRequestRetried(
-                    this.correlationId
-                );
+                const requestRetried = this.browserStorage.getRequestRetried();
 
                 if (requestRetried) {
                     this.logger.error(
@@ -372,15 +370,15 @@ export class RedirectClient extends StandardInteractionClient {
                     this.browserStorage.getCachedRedirectRequest();
                 if (!redirectRequest) {
                     this.logger.error(
-                        `Unable to retry. Please retry with redirect request and correlationId: ${this.correlationId}`
+                        "Unable to retry. Please retry with redirect request"
                     );
-                    this.browserStorage.setRequestRetried(this.correlationId);
+                    this.browserStorage.setRequestRetried();
                     throw createBrowserAuthError(
                         BrowserAuthErrorCodes.failedToRetry
                     );
                 }
 
-                this.browserStorage.setRequestRetried(this.correlationId);
+                this.browserStorage.setRequestRetried();
 
                 await this.acquireToken(redirectRequest);
                 return null;

diff --git a/lib/msal-browser/src/interaction_client/SilentIframeClient.ts b/lib/msal-browser/src/interaction_client/SilentIframeClient.ts
@@ -159,39 +159,36 @@ export class SilentIframeClient extends StandardInteractionClient {
             }
 
             if (
-                e instanceof ServerError &&
-                e.errorCode === BrowserConstants.INVALID_GRANT_ERROR
+                !authClient ||
+                !(e instanceof ServerError) ||
+                e.errorCode !== BrowserConstants.INVALID_GRANT_ERROR
             ) {
-                if (!authClient) {
-                    throw e;
-                } else {
-                    this.performanceClient.addFields(
-                        {
-                            retryError: e.errorCode,
-                        },
-                        this.correlationId
-                    );
+                throw e;
+            }
 
-                    const retrySilentRequest: AuthorizationUrlRequest =
-                        await invokeAsync(
-                            this.initializeAuthorizationRequest.bind(this),
-                            PerformanceEvents.StandardInteractionClientInitializeAuthorizationRequest,
-                            this.logger,
-                            this.performanceClient,
-                            request.correlationId
-                        )(inputRequest, InteractionType.Silent);
+            this.performanceClient.addFields(
+                {
+                    retryError: e.errorCode,
+                },
+                this.correlationId
+            );
 
-                    return await invokeAsync(
-                        this.silentTokenHelper.bind(this),
-                        PerformanceEvents.SilentIframeClientTokenHelper,
-                        this.logger,
-                        this.performanceClient,
-                        this.correlationId
-                    )(authClient, retrySilentRequest);
-                }
-            }
+            const retrySilentRequest: AuthorizationUrlRequest =
+                await invokeAsync(
+                    this.initializeAuthorizationRequest.bind(this),
+                    PerformanceEvents.StandardInteractionClientInitializeAuthorizationRequest,
+                    this.logger,
+                    this.performanceClient,
+                    request.correlationId
+                )(inputRequest, InteractionType.Silent);
 
-            throw e;
+            return await invokeAsync(
+                this.silentTokenHelper.bind(this),
+                PerformanceEvents.SilentIframeClientTokenHelper,
+                this.logger,
+                this.performanceClient,
+                this.correlationId
+            )(authClient, retrySilentRequest);
         }
     }
 

diff --git a/lib/msal-browser/test/cache/BrowserCacheManager.spec.ts b/lib/msal-browser/test/cache/BrowserCacheManager.spec.ts
@@ -2980,11 +2980,10 @@ describe("BrowserCacheManager tests", () => {
                 browserCrypto,
                 logger
             );
-            const requestRetriedKey = browserStorage.generateRequestRetriedKey(
-                TEST_CONFIG.CORRELATION_ID
-            );
+            const requestRetriedKey =
+                browserStorage.generateRequestRetriedKey();
             expect(requestRetriedKey).toBe(
-                `${Constants.CACHE_PREFIX}.${TemporaryCacheKeys.REQUEST_RETRY}.${TEST_CONFIG.CORRELATION_ID}`
+                `${Constants.CACHE_PREFIX}.${TemporaryCacheKeys.REQUEST_RETRY}.${TEST_CONFIG.MSAL_CLIENT_ID}`
             );
         });
 
@@ -2996,54 +2995,47 @@ describe("BrowserCacheManager tests", () => {
                 logger
             );
             browserStorage.setTemporaryCache(
-                `${Constants.CACHE_PREFIX}.${TemporaryCacheKeys.REQUEST_RETRY}.${TEST_CONFIG.CORRELATION_ID}`,
+                `${Constants.CACHE_PREFIX}.${TemporaryCacheKeys.REQUEST_RETRY}.${TEST_CONFIG.MSAL_CLIENT_ID}`,
                 "1"
             );
 
-            browserStorage.getRequestRetried(TEST_CONFIG.CORRELATION_ID);
+            browserStorage.getRequestRetried();
 
-            expect(
-                browserStorage.getRequestRetried(TEST_CONFIG.CORRELATION_ID)
-            ).toEqual(1);
+            expect(browserStorage.getRequestRetried()).toEqual(1);
         });
 
-        it("setRequestRetried() sets a request retry value for correlationId", () => {
+        it("setRequestRetried() sets a request retry value for client Id", () => {
             const browserStorage = new BrowserCacheManager(
                 TEST_CONFIG.MSAL_CLIENT_ID,
                 cacheConfig,
                 browserCrypto,
                 logger
             );
 
-            browserStorage.setRequestRetried(TEST_CONFIG.CORRELATION_ID);
+            browserStorage.setRequestRetried();
 
             expect(
                 window.sessionStorage[
-                    `${Constants.CACHE_PREFIX}.${TemporaryCacheKeys.REQUEST_RETRY}.${TEST_CONFIG.CORRELATION_ID}`
+                    `${Constants.CACHE_PREFIX}.${TemporaryCacheKeys.REQUEST_RETRY}.${TEST_CONFIG.MSAL_CLIENT_ID}`
                 ]
             ).toBeTruthy();
         });
 
-        it("removeRequestRetried() removes all temporary cache items with request retried value", () => {
+        it("removeRequestRetried() removes request retried value for clientId", () => {
             const browserStorage = new BrowserCacheManager(
                 TEST_CONFIG.MSAL_CLIENT_ID,
                 cacheConfig,
                 browserCrypto,
                 logger
             );
             browserStorage.setTemporaryCache(
-                `${Constants.CACHE_PREFIX}.${TemporaryCacheKeys.REQUEST_RETRY}.111`,
-                "1"
-            );
-            browserStorage.setTemporaryCache(
-                `${Constants.CACHE_PREFIX}.${TemporaryCacheKeys.REQUEST_RETRY}.222`,
+                `${Constants.CACHE_PREFIX}.${TemporaryCacheKeys.REQUEST_RETRY}.${TEST_CONFIG.MSAL_CLIENT_ID}`,
                 "1"
             );
 
             browserStorage.removeRequestRetried();
 
-            expect(browserStorage.getRequestRetried("111")).toEqual(null);
-            expect(browserStorage.getRequestRetried("222")).toEqual(null);
+            expect(browserStorage.getRequestRetried()).toEqual(null);
         });
 
         it("Successfully retrieves redirect request from cache", async () => {

diff --git a/lib/msal-browser/test/interaction_client/RedirectClient.spec.ts b/lib/msal-browser/test/interaction_client/RedirectClient.spec.ts
@@ -577,9 +577,7 @@ describe("RedirectClient", () => {
                     TemporaryCacheKeys.REDIRECT_REQUEST
                 )
             ).toEqual(null);
-            expect(
-                browserStorage.getRequestRetried(TEST_CONFIG.CORRELATION_ID)
-            ).toEqual(null);
+            expect(browserStorage.getRequestRetried()).toEqual(null);
         });
 
         it("gets hash from cache and calls native broker if hash contains accountId", async () => {
@@ -1985,9 +1983,7 @@ describe("RedirectClient", () => {
                     TemporaryCacheKeys.REDIRECT_REQUEST
                 )
             ).toEqual(null);
-            expect(
-                browserStorage.getRequestRetried(TEST_CONFIG.CORRELATION_ID)
-            ).toEqual(1);
+            expect(browserStorage.getRequestRetried()).toEqual(1);
         });
 
         it("throws invalid_grant error if already retried", (done) => {
@@ -2023,7 +2019,7 @@ describe("RedirectClient", () => {
                 "123523"
             );
             window.sessionStorage.setItem(
-                `${Constants.CACHE_PREFIX}.${TemporaryCacheKeys.REQUEST_RETRY}.${TEST_CONFIG.CORRELATION_ID}`,
+                `${Constants.CACHE_PREFIX}.${TemporaryCacheKeys.REQUEST_RETRY}.${TEST_CONFIG.MSAL_CLIENT_ID}`,
                 JSON.stringify(1)
             );
             const testRedirectRequest: RedirectRequest = {
@@ -2094,11 +2090,7 @@ describe("RedirectClient", () => {
                             TemporaryCacheKeys.REDIRECT_REQUEST
                         )
                     ).toEqual(null);
-                    expect(
-                        browserStorage.getRequestRetried(
-                            TEST_CONFIG.CORRELATION_ID
-                        )
-                    ).toEqual(null);
+                    expect(browserStorage.getRequestRetried()).toEqual(null);
 
                     done();
                 });
@@ -2190,11 +2182,7 @@ describe("RedirectClient", () => {
                             TemporaryCacheKeys.REDIRECT_REQUEST
                         )
                     ).toEqual(null);
-                    expect(
-                        browserStorage.getRequestRetried(
-                            TEST_CONFIG.CORRELATION_ID
-                        )
-                    ).toEqual(1);
+                    expect(browserStorage.getRequestRetried()).toEqual(1);
                     done();
                 });
         });