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" +} diff --git a/lib/msal-browser/src/controllers/StandardController.ts b/lib/msal-browser/src/controllers/StandardController.ts index 9378993af0..ee00bac23e 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); } /** @@ -2223,6 +2224,7 @@ 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 ( NativeMessageHandler.isPlatformBrokerAvailable( this.config, @@ -2237,7 +2239,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)) { @@ -2257,6 +2261,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, diff --git a/lib/msal-browser/src/interaction_client/NativeInteractionClient.ts b/lib/msal-browser/src/interaction_client/NativeInteractionClient.ts index c60a48f1ce..e72281be0f 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,12 @@ 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 e; + } // 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 be58623622..75eec2186c 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, @@ -5773,10 +5774,52 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { 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 SilentCacheClient.acquireToken, and calls 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 isPlatformBrokerAvailableSpy = jest + .spyOn(NativeMessageHandler, "isPlatformBrokerAvailable") + .mockReturnValue(true); + const nativeAcquireTokenSpy: jest.SpyInstance = jest + .spyOn(NativeInteractionClient.prototype, "acquireToken") + .mockImplementation(); + const cacheAccount = testAccount; + cacheAccount.nativeAccountId = "nativeAccountId"; + + await expect( + pca.acquireTokenSilent({ + scopes: ["openid"], + account: cacheAccount, + cacheLookupPolicy: CacheLookupPolicy.AccessToken, + }) + ) + .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 () => { + const silentCacheSpy = jest + .spyOn(SilentCacheClient.prototype, "acquireToken") + .mockImplementation(); const silentRefreshSpy: jest.SpyInstance = jest .spyOn(SilentRefreshClient.prototype, "acquireToken") .mockRejectedValue(refreshRequiredServerError); @@ -5788,15 +5831,57 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { pca.acquireTokenSilent({ scopes: ["openid"], account: testAccount, - cacheLookupPolicy: - CacheLookupPolicy.AccessTokenAndRefreshToken, + cacheLookupPolicy: CacheLookupPolicy.RefreshToken, }) ).rejects.toThrow(refreshRequiredServerError); - expect(silentCacheSpy).toHaveBeenCalledTimes(1); + expect(silentCacheSpy).toHaveBeenCalledTimes(0); expect(silentRefreshSpy).toHaveBeenCalledTimes(1); expect(silentIframeSpy).toHaveBeenCalledTimes(0); }); + it("Calls NativeInteractionClient.acquireToken when CacheLookupPolicy is set to AccessTokenAndRefreshToken", async () => { + const silentCacheSpy: jest.SpyInstance = jest + .spyOn(SilentCacheClient.prototype, "acquireToken") + .mockRejectedValue(refreshRequiredCacheError); + const silentRefreshSpy: jest.SpyInstance = jest + .spyOn(SilentRefreshClient.prototype, "acquireToken") + .mockRejectedValue(refreshRequiredServerError); + const silentIframeSpy = jest + .spyOn(SilentIframeClient.prototype, "acquireToken") + .mockImplementation(); + const nativeAcquireTokenSpy: jest.SpyInstance = jest + .spyOn(NativeInteractionClient.prototype, "acquireToken") + .mockImplementation(); + + const cacheAccount = testAccount; + cacheAccount.nativeAccountId = "nativeAccountId"; + const isPlatformBrokerAvailableSpy = jest + .spyOn(NativeMessageHandler, "isPlatformBrokerAvailable") + .mockReturnValue(true); + testAccount.nativeAccountId = "nativeAccountId"; + + await expect( + pca.acquireTokenSilent({ + scopes: ["openid"], + account: cacheAccount, + cacheLookupPolicy: + CacheLookupPolicy.AccessTokenAndRefreshToken, + }) + ) + .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 () => { const silentCacheSpy = jest .spyOn(SilentCacheClient.prototype, "acquireToken")