From 3da07d42a58712d10d511b2410a0e8089abd7926 Mon Sep 17 00:00:00 2001 From: sameerag Date: Thu, 27 Feb 2025 06:29:11 -0800 Subject: [PATCH 1/6] Add check for cache when calling native APIs --- .../src/controllers/StandardController.ts | 11 +++++++++++ .../test/app/PublicClientApplication.spec.ts | 8 ++++++++ 2 files changed, 19 insertions(+) diff --git a/lib/msal-browser/src/controllers/StandardController.ts b/lib/msal-browser/src/controllers/StandardController.ts index 9378993af0..6c9a1a6999 100644 --- a/lib/msal-browser/src/controllers/StandardController.ts +++ b/lib/msal-browser/src/controllers/StandardController.ts @@ -2223,7 +2223,9 @@ export class StandardController implements IController { silentRequest: CommonSilentFlowRequest, cacheLookupPolicy: CacheLookupPolicy ): Promise { + // if the cache policy is set to access_token only, we should not be hitting the native layer yet if ( + cacheLookupPolicy !== CacheLookupPolicy.AccessToken && NativeMessageHandler.isPlatformBrokerAvailable( this.config, this.logger, @@ -2257,6 +2259,12 @@ export class StandardController implements IController { this.logger.verbose( "acquireTokenSilent - attempting to acquire token from web flow" ); + // add logs to identify embedded cache retrieval + if (cacheLookupPolicy === CacheLookupPolicy.AccessToken) { + this.logger.verbose( + "acquireTokenSilent - cache lookup policy set to AccessToken, attempting to acquire token from local cache" + ); + } return invokeAsync( this.acquireTokenFromCache.bind(this), PerformanceEvents.AcquireTokenFromCache, @@ -2266,6 +2274,9 @@ export class StandardController implements IController { )(silentRequest, cacheLookupPolicy).catch( (cacheError: AuthError) => { if (cacheLookupPolicy === CacheLookupPolicy.AccessToken) { + this.logger.verbose( + "Failed to retrieve token from cache" + ); throw cacheError; } diff --git a/lib/msal-browser/test/app/PublicClientApplication.spec.ts b/lib/msal-browser/test/app/PublicClientApplication.spec.ts index be58623622..dfe430f671 100644 --- a/lib/msal-browser/test/app/PublicClientApplication.spec.ts +++ b/lib/msal-browser/test/app/PublicClientApplication.spec.ts @@ -5760,6 +5760,13 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { const silentIframeSpy = jest .spyOn(SilentIframeClient.prototype, "acquireToken") .mockImplementation(); + const nativeRequestSpy = jest + .spyOn(NativeInteractionClient.prototype, "acquireToken") + .mockImplementation(); + const isPlatformBrokerAvailableSpy = jest + .spyOn(NativeMessageHandler, "isPlatformBrokerAvailable") + .mockReturnValue(true); + testAccount.nativeAccountId = "nativeAccountId"; await expect( pca.acquireTokenSilent({ @@ -5769,6 +5776,7 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { }) ).rejects.toThrow(refreshRequiredCacheError); expect(silentCacheSpy).toHaveBeenCalledTimes(1); + expect(nativeRequestSpy).toHaveBeenCalledTimes(0); expect(silentRefreshSpy).toHaveBeenCalledTimes(0); expect(silentIframeSpy).toHaveBeenCalledTimes(0); }); From 7153ca271495860944ed2482ae498d6bb6b398c3 Mon Sep 17 00:00:00 2001 From: sameerag Date: Thu, 27 Feb 2025 11:41:23 -0800 Subject: [PATCH 2/6] Add tests --- .../test/app/PublicClientApplication.spec.ts | 85 +++++++++++++++++-- 1 file changed, 77 insertions(+), 8 deletions(-) diff --git a/lib/msal-browser/test/app/PublicClientApplication.spec.ts b/lib/msal-browser/test/app/PublicClientApplication.spec.ts index dfe430f671..adce411ceb 100644 --- a/lib/msal-browser/test/app/PublicClientApplication.spec.ts +++ b/lib/msal-browser/test/app/PublicClientApplication.spec.ts @@ -117,6 +117,7 @@ import { Configuration, } from "../../src/config/Configuration.js"; import { buildAccountFromIdTokenClaims, buildIdToken } from "msal-test-utils"; +import { nativeConnectionNotEstablished } from "../../src/error/BrowserAuthErrorCodes.js"; const cacheConfig = { temporaryCacheLocation: BrowserCacheLocation.SessionStorage, @@ -5760,28 +5761,79 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { const silentIframeSpy = jest .spyOn(SilentIframeClient.prototype, "acquireToken") .mockImplementation(); + + await expect( + pca.acquireTokenSilent({ + scopes: ["openid"], + account: testAccount, + cacheLookupPolicy: CacheLookupPolicy.AccessToken, + }) + ).rejects.toThrow(refreshRequiredCacheError); + expect(silentCacheSpy).toHaveBeenCalledTimes(1); + expect(silentRefreshSpy).toHaveBeenCalledTimes(0); + expect(silentIframeSpy).toHaveBeenCalledTimes(0); + }); + + it("Calls SilentCacheClient.acquireToken, and does not call NativeInteractionClient.acquireToken when CacheLookupPolicy is set to AccessToken", async () => { + const silentCacheSpy: jest.SpyInstance = jest + .spyOn(SilentCacheClient.prototype, "acquireToken") + .mockRejectedValue(refreshRequiredCacheError); + const silentRefreshSpy = jest + .spyOn(SilentRefreshClient.prototype, "acquireToken") + .mockImplementation(); + const silentIframeSpy = jest + .spyOn(SilentIframeClient.prototype, "acquireToken") + .mockImplementation(); + const nativeRequestSpy = jest .spyOn(NativeInteractionClient.prototype, "acquireToken") .mockImplementation(); const isPlatformBrokerAvailableSpy = jest .spyOn(NativeMessageHandler, "isPlatformBrokerAvailable") .mockReturnValue(true); - testAccount.nativeAccountId = "nativeAccountId"; + const nativeAcquireTokenSpy: jest.SpyInstance = jest + .spyOn(NativeInteractionClient.prototype, "acquireToken") + .mockImplementation(); + const account = testAccount; + account.nativeAccountId = "nativeAccountId"; await expect( pca.acquireTokenSilent({ scopes: ["openid"], - account: testAccount, + account, cacheLookupPolicy: CacheLookupPolicy.AccessToken, }) ).rejects.toThrow(refreshRequiredCacheError); expect(silentCacheSpy).toHaveBeenCalledTimes(1); - expect(nativeRequestSpy).toHaveBeenCalledTimes(0); expect(silentRefreshSpy).toHaveBeenCalledTimes(0); expect(silentIframeSpy).toHaveBeenCalledTimes(0); + expect(nativeAcquireTokenSpy).toHaveBeenCalledTimes(0); + }); + + it("Calls SilentRefreshClient.acquireToken, and does not call SilentCacheClient.acquireToken or SilentIframeClient.acquireToken if refresh token is expired when CacheLookupPolicy is set to RefreshToken", async () => { + const silentCacheSpy = jest + .spyOn(SilentCacheClient.prototype, "acquireToken") + .mockImplementation(); + const silentRefreshSpy: jest.SpyInstance = jest + .spyOn(SilentRefreshClient.prototype, "acquireToken") + .mockRejectedValue(refreshRequiredServerError); + const silentIframeSpy = jest + .spyOn(SilentIframeClient.prototype, "acquireToken") + .mockImplementation(); + + await expect( + pca.acquireTokenSilent({ + scopes: ["openid"], + account: testAccount, + cacheLookupPolicy: CacheLookupPolicy.RefreshToken, + }) + ).rejects.toThrow(refreshRequiredServerError); + expect(silentCacheSpy).toHaveBeenCalledTimes(0); + expect(silentRefreshSpy).toHaveBeenCalledTimes(1); + expect(silentIframeSpy).toHaveBeenCalledTimes(0); }); - it("Calls SilentCacheClient.acquireToken and SilentRefreshClient.acquireToken, and does not call SilentIframeClient.acquireToken if cache lookup throws and refresh token is expired when CacheLookupPolicy is set to AccessTokenAndRefreshToken", async () => { + it("Calls NativeInteractionClient.acquireToken when CacheLookupPolicy is set to AccessTokenAndRefreshToken", async () => { const silentCacheSpy: jest.SpyInstance = jest .spyOn(SilentCacheClient.prototype, "acquireToken") .mockRejectedValue(refreshRequiredCacheError); @@ -5792,17 +5844,34 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { .spyOn(SilentIframeClient.prototype, "acquireToken") .mockImplementation(); + const nativeAcquireTokenSpy: jest.SpyInstance = jest + .spyOn(NativeInteractionClient.prototype, "acquireToken") + .mockImplementation(); + const account = testAccount; + account.nativeAccountId = "nativeAccountId"; + const isPlatformBrokerAvailableSpy = jest + .spyOn(NativeMessageHandler, "isPlatformBrokerAvailable") + .mockReturnValue(true); + testAccount.nativeAccountId = "nativeAccountId"; + await expect( pca.acquireTokenSilent({ scopes: ["openid"], - account: testAccount, + account, cacheLookupPolicy: CacheLookupPolicy.AccessTokenAndRefreshToken, }) - ).rejects.toThrow(refreshRequiredServerError); - expect(silentCacheSpy).toHaveBeenCalledTimes(1); - expect(silentRefreshSpy).toHaveBeenCalledTimes(1); + ) + .rejects.toThrow(BrowserAuthError) + .catch((error) => { + expect(error.errorCode).toBe( + BrowserAuthErrorCodes.nativeConnectionNotEstablished + ); + }); + expect(silentCacheSpy).toHaveBeenCalledTimes(0); + expect(silentRefreshSpy).toHaveBeenCalledTimes(0); expect(silentIframeSpy).toHaveBeenCalledTimes(0); + expect(nativeAcquireTokenSpy).toHaveBeenCalledTimes(0); }); it("Calls SilentRefreshClient.acquireToken, and does not call SilentCacheClient.acquireToken or SilentIframeClient.acquireToken if refresh token is expired when CacheLookupPolicy is set to RefreshToken", async () => { From 442279ea31d23d70c3bd33bb457da165e4985b4e Mon Sep 17 00:00:00 2001 From: sameerag Date: Thu, 27 Feb 2025 12:04:07 -0800 Subject: [PATCH 3/6] Change files --- ...-msal-browser-a7e892b4-c765-4859-b48a-d360f4da38d1.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/@azure-msal-browser-a7e892b4-c765-4859-b48a-d360f4da38d1.json diff --git a/change/@azure-msal-browser-a7e892b4-c765-4859-b48a-d360f4da38d1.json b/change/@azure-msal-browser-a7e892b4-c765-4859-b48a-d360f4da38d1.json new file mode 100644 index 0000000000..db90ee94a3 --- /dev/null +++ b/change/@azure-msal-browser-a7e892b4-c765-4859-b48a-d360f4da38d1.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "Fix double brokering bug: Native flow should not be triggered for cache only look up", + "packageName": "@azure/msal-browser", + "email": "sameera.gajjarapu@microsoft.com", + "dependentChangeType": "patch" +} From caa4ae95051c6440ba5550474a5faae1fb72c707 Mon Sep 17 00:00:00 2001 From: sameerag Date: Tue, 4 Mar 2025 13:47:09 -0800 Subject: [PATCH 4/6] Update code to reflect native cache lookup --- .../src/controllers/StandardController.ts | 11 +++--- .../NativeInteractionClient.ts | 12 ++++++- .../test/app/PublicClientApplication.spec.ts | 34 ++++++++++++------- 3 files changed, 39 insertions(+), 18 deletions(-) diff --git a/lib/msal-browser/src/controllers/StandardController.ts b/lib/msal-browser/src/controllers/StandardController.ts index 6c9a1a6999..ad9e361411 100644 --- a/lib/msal-browser/src/controllers/StandardController.ts +++ b/lib/msal-browser/src/controllers/StandardController.ts @@ -1519,7 +1519,8 @@ export class StandardController implements IController { public async acquireTokenNative( request: PopupRequest | SilentRequest | SsoSilentRequest, apiId: ApiId, - accountId?: string + accountId?: string, + cacheLookupPolicy?: CacheLookupPolicy ): Promise { this.logger.trace("acquireTokenNative called"); if (!this.nativeExtensionProvider) { @@ -1543,7 +1544,7 @@ export class StandardController implements IController { request.correlationId ); - return nativeClient.acquireToken(request); + return nativeClient.acquireToken(request, cacheLookupPolicy); } /** @@ -2225,7 +2226,7 @@ export class StandardController implements IController { ): Promise { // if the cache policy is set to access_token only, we should not be hitting the native layer yet if ( - cacheLookupPolicy !== CacheLookupPolicy.AccessToken && + // cacheLookupPolicy !== CacheLookupPolicy.AccessToken && NativeMessageHandler.isPlatformBrokerAvailable( this.config, this.logger, @@ -2239,7 +2240,9 @@ export class StandardController implements IController { ); return this.acquireTokenNative( silentRequest, - ApiId.acquireTokenSilent_silentFlow + ApiId.acquireTokenSilent_silentFlow, + silentRequest.account.nativeAccountId, + cacheLookupPolicy ).catch(async (e: AuthError) => { // If native token acquisition fails for availability reasons fallback to web flow if (e instanceof NativeAuthError && isFatalNativeAuthError(e)) { diff --git a/lib/msal-browser/src/interaction_client/NativeInteractionClient.ts b/lib/msal-browser/src/interaction_client/NativeInteractionClient.ts index c60a48f1ce..928249a7f6 100644 --- a/lib/msal-browser/src/interaction_client/NativeInteractionClient.ts +++ b/lib/msal-browser/src/interaction_client/NativeInteractionClient.ts @@ -52,6 +52,7 @@ import { TemporaryCacheKeys, NativeConstants, BrowserConstants, + CacheLookupPolicy, } from "../utils/BrowserConstants.js"; import { NativeExtensionRequestBody, @@ -157,7 +158,8 @@ export class NativeInteractionClient extends BaseInteractionClient { * @param request */ async acquireToken( - request: PopupRequest | SilentRequest | SsoSilentRequest + request: PopupRequest | SilentRequest | SsoSilentRequest, + cacheLookupPolicy?: CacheLookupPolicy ): Promise { this.performanceClient.addQueueMeasurement( PerformanceEvents.NativeInteractionClientAcquireToken, @@ -192,6 +194,14 @@ export class NativeInteractionClient extends BaseInteractionClient { }); return result; } catch (e) { + if (cacheLookupPolicy === CacheLookupPolicy.AccessToken) { + this.logger.info( + "MSAL internal Cache does not contain tokens, return error as per cache policy" + ); + throw createClientAuthError( + ClientAuthErrorCodes.tokenRefreshRequired + ); + } // continue with a native call for any and all errors this.logger.info( "MSAL internal Cache does not contain tokens, proceed to make a native call" diff --git a/lib/msal-browser/test/app/PublicClientApplication.spec.ts b/lib/msal-browser/test/app/PublicClientApplication.spec.ts index adce411ceb..75eec2186c 100644 --- a/lib/msal-browser/test/app/PublicClientApplication.spec.ts +++ b/lib/msal-browser/test/app/PublicClientApplication.spec.ts @@ -5774,7 +5774,7 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { expect(silentIframeSpy).toHaveBeenCalledTimes(0); }); - it("Calls SilentCacheClient.acquireToken, and does not call NativeInteractionClient.acquireToken when CacheLookupPolicy is set to AccessToken", async () => { + it("Calls SilentCacheClient.acquireToken, and calls NativeInteractionClient.acquireToken when CacheLookupPolicy is set to AccessToken", async () => { const silentCacheSpy: jest.SpyInstance = jest .spyOn(SilentCacheClient.prototype, "acquireToken") .mockRejectedValue(refreshRequiredCacheError); @@ -5785,29 +5785,35 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { .spyOn(SilentIframeClient.prototype, "acquireToken") .mockImplementation(); - const nativeRequestSpy = jest - .spyOn(NativeInteractionClient.prototype, "acquireToken") - .mockImplementation(); const isPlatformBrokerAvailableSpy = jest .spyOn(NativeMessageHandler, "isPlatformBrokerAvailable") .mockReturnValue(true); const nativeAcquireTokenSpy: jest.SpyInstance = jest .spyOn(NativeInteractionClient.prototype, "acquireToken") .mockImplementation(); - const account = testAccount; - account.nativeAccountId = "nativeAccountId"; + const cacheAccount = testAccount; + cacheAccount.nativeAccountId = "nativeAccountId"; await expect( pca.acquireTokenSilent({ scopes: ["openid"], - account, + account: cacheAccount, cacheLookupPolicy: CacheLookupPolicy.AccessToken, }) - ).rejects.toThrow(refreshRequiredCacheError); - expect(silentCacheSpy).toHaveBeenCalledTimes(1); + ) + .rejects.toThrow(BrowserAuthError) + .catch((error) => { + expect(error.errorCode).toBe( + BrowserAuthErrorCodes.nativeConnectionNotEstablished + ); + }); + expect(silentCacheSpy).toHaveBeenCalledTimes(0); expect(silentRefreshSpy).toHaveBeenCalledTimes(0); expect(silentIframeSpy).toHaveBeenCalledTimes(0); expect(nativeAcquireTokenSpy).toHaveBeenCalledTimes(0); + + nativeAcquireTokenSpy.mockRestore(); + isPlatformBrokerAvailableSpy.mockRestore(); }); it("Calls SilentRefreshClient.acquireToken, and does not call SilentCacheClient.acquireToken or SilentIframeClient.acquireToken if refresh token is expired when CacheLookupPolicy is set to RefreshToken", async () => { @@ -5843,12 +5849,12 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { const silentIframeSpy = jest .spyOn(SilentIframeClient.prototype, "acquireToken") .mockImplementation(); - const nativeAcquireTokenSpy: jest.SpyInstance = jest .spyOn(NativeInteractionClient.prototype, "acquireToken") .mockImplementation(); - const account = testAccount; - account.nativeAccountId = "nativeAccountId"; + + const cacheAccount = testAccount; + cacheAccount.nativeAccountId = "nativeAccountId"; const isPlatformBrokerAvailableSpy = jest .spyOn(NativeMessageHandler, "isPlatformBrokerAvailable") .mockReturnValue(true); @@ -5857,7 +5863,7 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { await expect( pca.acquireTokenSilent({ scopes: ["openid"], - account, + account: cacheAccount, cacheLookupPolicy: CacheLookupPolicy.AccessTokenAndRefreshToken, }) @@ -5872,6 +5878,8 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { expect(silentRefreshSpy).toHaveBeenCalledTimes(0); expect(silentIframeSpy).toHaveBeenCalledTimes(0); expect(nativeAcquireTokenSpy).toHaveBeenCalledTimes(0); + nativeAcquireTokenSpy.mockRestore(); + isPlatformBrokerAvailableSpy.mockRestore(); }); it("Calls SilentRefreshClient.acquireToken, and does not call SilentCacheClient.acquireToken or SilentIframeClient.acquireToken if refresh token is expired when CacheLookupPolicy is set to RefreshToken", async () => { From 34a463415866ed105aa37e5a1cb3f7dcb2593f32 Mon Sep 17 00:00:00 2001 From: sameerag Date: Tue, 4 Mar 2025 14:06:24 -0800 Subject: [PATCH 5/6] Remove commented code --- lib/msal-browser/src/controllers/StandardController.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/msal-browser/src/controllers/StandardController.ts b/lib/msal-browser/src/controllers/StandardController.ts index ad9e361411..0e81131e5f 100644 --- a/lib/msal-browser/src/controllers/StandardController.ts +++ b/lib/msal-browser/src/controllers/StandardController.ts @@ -2226,7 +2226,6 @@ export class StandardController implements IController { ): Promise { // if the cache policy is set to access_token only, we should not be hitting the native layer yet if ( - // cacheLookupPolicy !== CacheLookupPolicy.AccessToken && NativeMessageHandler.isPlatformBrokerAvailable( this.config, this.logger, From c7340da087e1e467e4efbea9663123042c399f19 Mon Sep 17 00:00:00 2001 From: sameerag Date: Tue, 4 Mar 2025 14:11:09 -0800 Subject: [PATCH 6/6] Address feedback --- lib/msal-browser/src/controllers/StandardController.ts | 3 --- .../src/interaction_client/NativeInteractionClient.ts | 4 +--- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/lib/msal-browser/src/controllers/StandardController.ts b/lib/msal-browser/src/controllers/StandardController.ts index 0e81131e5f..ee00bac23e 100644 --- a/lib/msal-browser/src/controllers/StandardController.ts +++ b/lib/msal-browser/src/controllers/StandardController.ts @@ -2276,9 +2276,6 @@ export class StandardController implements IController { )(silentRequest, cacheLookupPolicy).catch( (cacheError: AuthError) => { if (cacheLookupPolicy === CacheLookupPolicy.AccessToken) { - this.logger.verbose( - "Failed to retrieve token from cache" - ); throw cacheError; } diff --git a/lib/msal-browser/src/interaction_client/NativeInteractionClient.ts b/lib/msal-browser/src/interaction_client/NativeInteractionClient.ts index 928249a7f6..e72281be0f 100644 --- a/lib/msal-browser/src/interaction_client/NativeInteractionClient.ts +++ b/lib/msal-browser/src/interaction_client/NativeInteractionClient.ts @@ -198,9 +198,7 @@ export class NativeInteractionClient extends BaseInteractionClient { this.logger.info( "MSAL internal Cache does not contain tokens, return error as per cache policy" ); - throw createClientAuthError( - ClientAuthErrorCodes.tokenRefreshRequired - ); + throw e; } // continue with a native call for any and all errors this.logger.info(