[v4] Remove access tokens synchronously (#7822)

Updates the remove access token cache logic to perform its steps
synchronously. This is a pre-requisite for additional work to handle
cache quota errors in the near future. This requires pushing the POP key
removal (which is asynchronous) to the background, we will track
failures around this in telemetry instead of throwing an error
developers can't resolve anyway.

Author: tnorling

change/@azure-msal-browser-f0b6da2e-a220-4654-b1aa-6b4a0f4a921b.json

@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "remove access tokens synchronously",
+  "packageName": "@azure/msal-browser",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}

change/@azure-msal-common-042c27fe-92dc-4b48-b5e0-3d114168f8d4.json

@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "remove access tokens synchronously",
+  "packageName": "@azure/msal-common",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}

change/@azure-msal-node-74ee63c8-51fb-422d-bbac-294ec6e9b0e5.json

@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "remove access tokens synchronously",
+  "packageName": "@azure/msal-node",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}

lib/msal-browser/src/cache/BrowserCacheManager.ts

@@ -86,8 +86,6 @@ export class BrowserCacheManager extends CacheManager {
     protected cookieStorage: CookieStorage;
     // Logger instance
     protected logger: Logger;
-    // Telemetry perf client
-    protected performanceClient: IPerformanceClient;
     // Event Handler
     private eventHandler: EventHandler;
 
@@ -100,7 +98,13 @@ export class BrowserCacheManager extends CacheManager {
         eventHandler: EventHandler,
         staticAuthorityOptions?: StaticAuthorityOptions
     ) {
-        super(clientId, cryptoImpl, logger, staticAuthorityOptions);
+        super(
+            clientId,
+            cryptoImpl,
+            logger,
+            performanceClient,
+            staticAuthorityOptions
+        );
         this.cacheConfig = cacheConfig;
         this.logger = logger;
         this.internalStorage = new MemoryStorage();
@@ -118,7 +122,6 @@ export class BrowserCacheManager extends CacheManager {
         );
         this.cookieStorage = new CookieStorage();
 
-        this.performanceClient = performanceClient;
         this.eventHandler = eventHandler;
     }
 
@@ -298,17 +301,17 @@ export class BrowserCacheManager extends CacheManager {
      * Extends inherited removeAccount function to include removal of the account key from the map
      * @param key
      */
-    async removeAccount(key: string): Promise<void> {
-        void super.removeAccount(key);
+    removeAccount(key: string, correlationId: string): void {
+        super.removeAccount(key, correlationId);
         this.removeAccountKeyFromMap(key);
     }
 
     /**
      * Removes credentials associated with the provided account
      * @param account
      */
-    async removeAccountContext(account: AccountEntity): Promise<void> {
-        await super.removeAccountContext(account);
+    removeAccountContext(account: AccountEntity, correlationId: string): void {
+        super.removeAccountContext(account, correlationId);
 
         /**
          * @deprecated - Remove this in next major version in favor of more consistent LOGOUT event
@@ -337,8 +340,8 @@ export class BrowserCacheManager extends CacheManager {
      * Removes given accessToken from the cache and from the key map
      * @param key
      */
-    async removeAccessToken(key: string): Promise<void> {
-        void super.removeAccessToken(key);
+    removeAccessToken(key: string, correlationId: string): void {
+        super.removeAccessToken(key, correlationId);
         this.removeTokenKey(key, CredentialType.ACCESS_TOKEN);
     }
 
@@ -1012,9 +1015,9 @@ export class BrowserCacheManager extends CacheManager {
     /**
      * Clears all cache entries created by MSAL.
      */
-    async clear(): Promise<void> {
+    clear(correlationId: string): void {
         // Removes all accounts and their credentials
-        await this.removeAllAccounts();
+        this.removeAllAccounts(correlationId);
         this.removeAppMetadata();
 
         // Remove temp storage first to make sure any cookies are cleared
@@ -1046,34 +1049,30 @@ export class BrowserCacheManager extends CacheManager {
      * @param correlationId {string} correlation id
      * @returns
      */
-    async clearTokensAndKeysWithClaims(
-        performanceClient: IPerformanceClient,
-        correlationId: string
-    ): Promise<void> {
-        performanceClient.addQueueMeasurement(
+    clearTokensAndKeysWithClaims(correlationId: string): void {
+        this.performanceClient.addQueueMeasurement(
             PerformanceEvents.ClearTokensAndKeysWithClaims,
             correlationId
         );
 
         const tokenKeys = this.getTokenKeys();
-
-        const removedAccessTokens: Array<Promise<void>> = [];
+        let removedAccessTokens = 0;
         tokenKeys.accessToken.forEach((key: string) => {
             // if the access token has claims in its key, remove the token key and the token
             const credential = this.getAccessTokenCredential(key);
             if (
                 credential?.requestedClaimsHash &&
                 key.includes(credential.requestedClaimsHash.toLowerCase())
             ) {
-                removedAccessTokens.push(this.removeAccessToken(key));
+                this.removeAccessToken(key, correlationId);
+                removedAccessTokens++;
             }
         });
-        await Promise.all(removedAccessTokens);
 
         // warn if any access tokens are removed
-        if (removedAccessTokens.length > 0) {
+        if (removedAccessTokens > 0) {
             this.logger.warning(
-                `${removedAccessTokens.length} access tokens with claims in the cache keys have been removed from the cache.`
+                `${removedAccessTokens} access tokens with claims in the cache keys have been removed from the cache.`
             );
         }
     }

lib/msal-browser/src/controllers/NestedAppAuthController.ts

@@ -505,9 +505,7 @@ export class NestedAppAuthController implements IController {
             currentAccount,
             authRequest,
             tokenKeys,
-            currentAccount.tenantId,
-            this.performanceClient,
-            authRequest.correlationId
+            currentAccount.tenantId
         );
 
         // If there is no access token, log it and return null

lib/msal-browser/src/controllers/StandardController.ts

@@ -23,6 +23,7 @@ import {
     getRequestThumbprint,
     AccountEntity,
     invokeAsync,
+    invoke,
     createClientAuthError,
     ClientAuthErrorCodes,
     AccountFilter,
@@ -380,15 +381,15 @@ export class StandardController implements IController {
                 "Claims-based caching is disabled. Clearing the previous cache with claims"
             );
 
-            await invokeAsync(
+            invoke(
                 this.browserStorage.clearTokensAndKeysWithClaims.bind(
                     this.browserStorage
                 ),
                 PerformanceEvents.ClearTokensAndKeysWithClaims,
                 this.logger,
                 this.performanceClient,
                 initCorrelationId
-            )(this.performanceClient, initCorrelationId);
+            )(initCorrelationId);
         }
 
         this.config.system.asyncPopups &&

lib/msal-browser/src/crypto/CryptoOps.ts

@@ -4,6 +4,8 @@
  */
 
 import {
+    ClientAuthErrorCodes,
+    createClientAuthError,
     ICrypto,
     IPerformanceClient,
     JoseHeader,
@@ -168,10 +170,14 @@ export class CryptoOps implements ICrypto {
      * Removes cryptographic keypair from key store matching the keyId passed in
      * @param kid
      */
-    async removeTokenBindingKey(kid: string): Promise<boolean> {
+    async removeTokenBindingKey(kid: string): Promise<void> {
         await this.cache.removeItem(kid);
         const keyFound = await this.cache.containsKey(kid);
-        return !keyFound;
+        if (keyFound) {
+            throw createClientAuthError(
+                ClientAuthErrorCodes.bindingKeyNotRemoved
+            );
+        }
     }
 
     /**

lib/msal-browser/src/crypto/SignedHttpRequest.ts

@@ -5,6 +5,8 @@
 
 import { CryptoOps } from "./CryptoOps.js";
 import {
+    ClientAuthError,
+    ClientAuthErrorCodes,
     Logger,
     LoggerOptions,
     PopTokenGenerator,
@@ -71,6 +73,22 @@ export class SignedHttpRequest {
      * @returns If keys are properly deleted
      */
     async removeKeys(publicKeyThumbprint: string): Promise<boolean> {
-        return this.cryptoOps.removeTokenBindingKey(publicKeyThumbprint);
+        return this.cryptoOps
+            .removeTokenBindingKey(publicKeyThumbprint)
+            .then(() => true)
+            .catch((error) => {
+                /*
+                 * @deprecated - To maintain public API signature, we return false if the error is due to the key still being present in indexedDB.
+                 */
+                if (
+                    error instanceof ClientAuthError &&
+                    error.errorCode ===
+                        ClientAuthErrorCodes.bindingKeyNotRemoved
+                ) {
+                    return false;
+                }
+
+                throw error;
+            });
     }
 }

lib/msal-browser/src/interaction_client/BaseInteractionClient.ts

@@ -102,8 +102,9 @@ export abstract class BaseInteractionClient {
             }
             // Clear given account.
             try {
-                await this.browserStorage.removeAccount(
-                    AccountEntity.generateAccountCacheKey(account)
+                this.browserStorage.removeAccount(
+                    AccountEntity.generateAccountCacheKey(account),
+                    this.correlationId
                 );
                 this.logger.verbose(
                     "Cleared cache items belonging to the account provided in the logout request."
@@ -120,7 +121,7 @@ export abstract class BaseInteractionClient {
                     this.correlationId
                 );
                 // Clear all accounts and tokens
-                await this.browserStorage.clear();
+                this.browserStorage.clear(this.correlationId);
                 // Clear any stray keys from IndexedDB
                 await this.browserCrypto.clearKeystore();
             } catch (e) {

lib/msal-browser/src/interaction_client/PlatformAuthInteractionClient.ts

@@ -714,11 +714,10 @@ export class PlatformAuthInteractionClient extends BaseInteractionClient {
         await this.browserStorage.setAccount(accountEntity, this.correlationId);
 
         // Remove any existing cached tokens for this account in browser storage
-        this.browserStorage.removeAccountContext(accountEntity).catch((e) => {
-            this.logger.error(
-                `Error occurred while removing account context from browser storage. ${e}`
-            );
-        });
+        this.browserStorage.removeAccountContext(
+            accountEntity,
+            this.correlationId
+        );
     }
 
     /**

lib/msal-browser/src/interaction_client/PopupClient.ts

@@ -503,8 +503,9 @@ export class PopupClient extends StandardInteractionClient {
                     validRequest.postLogoutRedirectUri &&
                     authClient.authority.protocolMode === ProtocolMode.OIDC
                 ) {
-                    void this.browserStorage.removeAccount(
-                        validRequest.account?.homeAccountId
+                    this.browserStorage.removeAccount(
+                        validRequest.account?.homeAccountId,
+                        this.correlationId
                     );
 
                     this.eventHandler.emitEvent(

lib/msal-browser/src/interaction_client/RedirectClient.ts

@@ -704,8 +704,9 @@ export class RedirectClient extends StandardInteractionClient {
                     authClient.authority.endSessionEndpoint;
                 } catch {
                     if (validLogoutRequest.account?.homeAccountId) {
-                        void this.browserStorage.removeAccount(
-                            validLogoutRequest.account?.homeAccountId
+                        this.browserStorage.removeAccount(
+                            validLogoutRequest.account?.homeAccountId,
+                            this.correlationId
                         );
 
                         this.eventHandler.emitEvent(

lib/msal-browser/test/app/PublicClientApplication.spec.ts

@@ -7779,7 +7779,10 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {
                     // Ensure account is present in the cache before removing it
                     const cacheKey =
                         AccountEntity.generateAccountCacheKey(accountInfo);
-                    secondBrowserStorageInstance.removeAccount(cacheKey);
+                    secondBrowserStorageInstance.removeAccount(
+                        cacheKey,
+                        RANDOM_TEST_GUID
+                    );
                 });
         });