Fix bug affecting metadata resolution for tenanted authorities (#6814)

Our hardcoded endpoint metadata is stored in a map keyed on full authority urls including the common tenants (common, organizations, consumers). This results in authority urls with specific tenants to fail the hardcoded lookup and go to the network for metadata which is taking ~700ms at P95. This PR does the following: - Updates the hardcoded metadata map to key off domain only (consequently reducing the total number of entries) - Removes extraneous fields that are not being used - Adds telemetry data point indicating where we got the metadata from (hardcoded, config, network, cache) - Removes unnecessary `createInstance` function
AzureAD · Jan 17, 2024 · 7035dea · 7035dea
1 parent 2014e78
commit 7035dea
Show file tree

Hide file tree

Showing 27 changed files with 438 additions and 1,276 deletions.
diff --git a/change/@azure-msal-browser-8017ad17-73f7-4627-b31f-e8bc1f0bd541.json b/change/@azure-msal-browser-8017ad17-73f7-4627-b31f-e8bc1f0bd541.json
@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Fix bug affecting metadata resolution for tenanted authorities",
+  "packageName": "@azure/msal-browser",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}
diff --git a/change/@azure-msal-common-c8346546-a98e-47fe-83b2-fcb5ceb36845.json b/change/@azure-msal-common-c8346546-a98e-47fe-83b2-fcb5ceb36845.json
@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Fix bug affecting metadata resolution for tenanted authorities",
+  "packageName": "@azure/msal-common",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}
diff --git a/change/@azure-msal-node-8042a4bb-8629-4d03-9fc0-9ad9f3ffca48.json b/change/@azure-msal-node-8042a4bb-8629-4d03-9fc0-9ad9f3ffca48.json
@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Fix bug affecting metadata resolution for tenanted authorities",
+  "packageName": "@azure/msal-node",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}
diff --git a/lib/msal-browser/src/cache/TokenCache.ts b/lib/msal-browser/src/cache/TokenCache.ts
@@ -31,6 +31,7 @@ import {
 } from "../error/BrowserAuthError";
 import { AuthenticationResult } from "../response/AuthenticationResult";
 import { base64Decode } from "../encode/Base64Decode";
+import * as BrowserCrypto from "../crypto/BrowserCrypto";
 
 export type LoadTokenOptions = {
     clientInfo?: string;
@@ -142,7 +143,8 @@ export class TokenCache implements ITokenCache {
                 this.config.system.networkClient,
                 this.storage,
                 authorityOptions,
-                this.logger
+                this.logger,
+                request.correlationId || BrowserCrypto.createNewGuid()
             );
 
             // "clientInfo" from options takes precedence over "clientInfo" in response

diff --git a/lib/msal-browser/src/interaction_client/BaseInteractionClient.ts b/lib/msal-browser/src/interaction_client/BaseInteractionClient.ts
@@ -279,28 +279,16 @@ export abstract class BaseInteractionClient {
             authorityMetadata: this.config.auth.authorityMetadata,
         };
 
-        if (requestAuthority) {
-            this.logger.verbose(
-                "Creating discovered authority with request authority"
-            );
-            return AuthorityFactory.createDiscoveredInstance(
-                requestAuthority,
-                this.config.system.networkClient,
-                this.browserStorage,
-                authorityOptions,
-                this.logger
-            );
-        }
-
-        this.logger.verbose(
-            "Creating discovered authority with configured authority"
-        );
+        const authority = requestAuthority || this.config.auth.authority;
+        this.logger.verbose(`Creating discovered authority with ${authority}`);
         return AuthorityFactory.createDiscoveredInstance(
-            this.config.auth.authority,
+            authority,
             this.config.system.networkClient,
             this.browserStorage,
             authorityOptions,
-            this.logger
+            this.logger,
+            this.correlationId,
+            this.performanceClient
         );
     }
 }
diff --git a/lib/msal-browser/src/interaction_client/StandardInteractionClient.ts b/lib/msal-browser/src/interaction_client/StandardInteractionClient.ts
@@ -331,8 +331,8 @@ export abstract class StandardInteractionClient extends BaseInteractionClient {
             this.browserStorage,
             authorityOptions,
             this.logger,
-            this.performanceClient,
-            this.correlationId
+            this.correlationId,
+            this.performanceClient
         );
     }
 

diff --git a/lib/msal-browser/test/cache/BrowserCacheManager.spec.ts b/lib/msal-browser/test/cache/BrowserCacheManager.spec.ts
@@ -347,7 +347,8 @@ describe("BrowserCacheManager tests", () => {
                     cloudDiscoveryMetadata: "",
                     knownAuthorities: [],
                 },
-                logger
+                logger,
+                TEST_CONFIG.CORRELATION_ID
             );
             // Pre-populate localstorage with accounts
             const testAccount = AccountEntity.createAccount(
@@ -425,7 +426,8 @@ describe("BrowserCacheManager tests", () => {
                     cloudDiscoveryMetadata: "",
                     knownAuthorities: [],
                 },
-                logger
+                logger,
+                TEST_CONFIG.CORRELATION_ID
             );
             sinon
                 .stub(Authority.prototype, "getPreferredCache")
@@ -1615,7 +1617,8 @@ describe("BrowserCacheManager tests", () => {
                     cloudDiscoveryMetadata: "",
                     knownAuthorities: [],
                 },
-                logger
+                logger,
+                TEST_CONFIG.CORRELATION_ID
             );
             sinon
                 .stub(Authority.prototype, "getPreferredCache")

diff --git a/lib/msal-browser/test/interaction_handler/InteractionHandler.spec.ts b/lib/msal-browser/test/interaction_handler/InteractionHandler.spec.ts
@@ -11,7 +11,6 @@ import {
     LogLevel,
     LoggerOptions,
     AccountInfo,
-    AuthorityFactory,
     CommonAuthorizationCodeRequest,
     AuthenticationResult,
     AuthorizationCodeClient,
@@ -82,7 +81,7 @@ class TestInteractionHandler extends InteractionHandler {
 
 const testAuthCodeRequest: CommonAuthorizationCodeRequest = {
     authenticationScheme: AuthenticationScheme.BEARER,
-    authority: "",
+    authority: TEST_CONFIG.validAuthority,
     redirectUri: TEST_URIS.TEST_REDIR_URI,
     scopes: ["scope1", "scope2"],
     code: "",
@@ -171,11 +170,10 @@ let authConfig: ClientConfiguration;
 
 describe("InteractionHandler.ts Unit Tests", () => {
     let authCodeModule: AuthorizationCodeClient;
-    let browserRequestLogger: Logger;
     let browserStorage: BrowserCacheManager;
     const cryptoOpts = new CryptoOps(testBrowserRequestLogger);
 
-    beforeEach(() => {
+    beforeEach(async () => {
         const appConfig: Configuration = {
             auth: {
                 clientId: TEST_CONFIG.MSAL_CLIENT_ID,
@@ -193,13 +191,21 @@ describe("InteractionHandler.ts Unit Tests", () => {
             piiLoggingEnabled: true,
         };
         const logger: Logger = new Logger(loggerOptions);
-        authorityInstance = AuthorityFactory.createInstance(
+        browserStorage = new BrowserCacheManager(
+            TEST_CONFIG.MSAL_CLIENT_ID,
+            configObj.cache,
+            cryptoOpts,
+            logger
+        );
+        authorityInstance = new Authority(
             configObj.auth.authority,
             networkInterface,
             browserStorage,
             authorityOptions,
-            logger
+            logger,
+            TEST_CONFIG.CORRELATION_ID
         );
+        await authorityInstance.resolveEndpointsAsync();
         authConfig = {
             authOptions: {
                 ...configObj.auth,
@@ -232,13 +238,6 @@ describe("InteractionHandler.ts Unit Tests", () => {
             loggerOptions: loggerOptions,
         };
         authCodeModule = new AuthorizationCodeClient(authConfig);
-        browserRequestLogger = new Logger(authConfig.loggerOptions!);
-        browserStorage = new BrowserCacheManager(
-            TEST_CONFIG.MSAL_CLIENT_ID,
-            configObj.cache,
-            cryptoOpts,
-            logger
-        );
     });
 
     afterEach(() => {
@@ -272,7 +271,6 @@ describe("InteractionHandler.ts Unit Tests", () => {
                 code: "authcode",
                 nonce: idTokenClaims.nonce,
                 state: TEST_STATE_VALUES.TEST_STATE_REDIRECT,
-                cloud_instance_host_name: "contoso.com",
             };
             const testAccount: AccountInfo = {
                 homeAccountId: TEST_DATA_CLIENT_INFO.TEST_HOME_ACCOUNT_ID,
@@ -319,29 +317,6 @@ describe("InteractionHandler.ts Unit Tests", () => {
                 TemporaryCacheKeys.CCS_CREDENTIAL,
                 CcsCredentialType.UPN
             );
-
-            sinon.stub(Authority.prototype, "isAlias").returns(false);
-            const authorityOptions: AuthorityOptions = {
-                protocolMode: ProtocolMode.AAD,
-                knownAuthorities: ["www.contoso.com"],
-                cloudDiscoveryMetadata: "",
-                authorityMetadata: "",
-            };
-            const authority = new Authority(
-                "https://www.contoso.com/common/",
-                networkInterface,
-                browserStorage,
-                authorityOptions,
-                browserRequestLogger
-            );
-            sinon
-                .stub(AuthorityFactory, "createDiscoveredInstance")
-                .resolves(authority);
-            sinon.stub(Authority.prototype, "discoveryComplete").returns(true);
-            const updateAuthoritySpy = sinon.spy(
-                AuthorizationCodeClient.prototype,
-                "updateAuthority"
-            );
             const acquireTokenSpy = sinon
                 .stub(AuthorizationCodeClient.prototype, "acquireToken")
                 .resolves(testTokenResponse);
@@ -355,7 +330,7 @@ describe("InteractionHandler.ts Unit Tests", () => {
                 await interactionHandler.handleCodeResponseFromServer(
                     testCodeResponse,
                     {
-                        authority: "https://www.contoso.com/common/",
+                        authority: TEST_CONFIG.validAuthority,
                         scopes: ["User.Read"],
                         correlationId: TEST_CONFIG.CORRELATION_ID,
                         redirectUri: "/",
@@ -365,12 +340,6 @@ describe("InteractionHandler.ts Unit Tests", () => {
                     }
                 );
 
-            expect(
-                updateAuthoritySpy.calledWith(
-                    "contoso.com",
-                    TEST_CONFIG.CORRELATION_ID
-                )
-            ).toBe(true);
             expect(tokenResponse).toEqual(testTokenResponse);
             expect(
                 acquireTokenSpy.calledWith(
@@ -400,7 +369,7 @@ describe("InteractionHandler.ts Unit Tests", () => {
                 code: "authcode",
                 nonce: idTokenClaims.nonce,
                 state: TEST_STATE_VALUES.TEST_STATE_REDIRECT,
-                cloud_instance_host_name: "contoso.com",
+                cloud_instance_host_name: "login.windows.net",
             };
             const testAccount: AccountInfo = {
                 homeAccountId: TEST_DATA_CLIENT_INFO.TEST_HOME_ACCOUNT_ID,
@@ -444,24 +413,6 @@ describe("InteractionHandler.ts Unit Tests", () => {
                     "handleFragmentResponse"
                 )
                 .returns(testCodeResponse);
-            sinon.stub(Authority.prototype, "isAlias").returns(false);
-            const authorityOptions: AuthorityOptions = {
-                protocolMode: ProtocolMode.AAD,
-                knownAuthorities: ["www.contoso.com"],
-                cloudDiscoveryMetadata: "",
-                authorityMetadata: "",
-            };
-            const authority = new Authority(
-                "https://www.contoso.com/common/",
-                networkInterface,
-                browserStorage,
-                authorityOptions,
-                browserRequestLogger
-            );
-            sinon
-                .stub(AuthorityFactory, "createDiscoveredInstance")
-                .resolves(authority);
-            sinon.stub(Authority.prototype, "discoveryComplete").returns(true);
             const updateAuthoritySpy = sinon.spy(
                 AuthorizationCodeClient.prototype,
                 "updateAuthority"
@@ -480,7 +431,7 @@ describe("InteractionHandler.ts Unit Tests", () => {
                     state: TEST_STATE_VALUES.TEST_STATE_REDIRECT,
                 },
                 {
-                    authority: "https://www.contoso.com/common/",
+                    authority: TEST_CONFIG.validAuthority,
                     scopes: ["User.Read"],
                     correlationId: TEST_CONFIG.CORRELATION_ID,
                     redirectUri: "/",
@@ -491,7 +442,7 @@ describe("InteractionHandler.ts Unit Tests", () => {
             );
             expect(
                 updateAuthoritySpy.calledWith(
-                    "contoso.com",
+                    testCodeResponse.cloud_instance_host_name,
                     TEST_CONFIG.CORRELATION_ID
                 )
             ).toBe(true);
@@ -521,7 +472,6 @@ describe("InteractionHandler.ts Unit Tests", () => {
                 code: "authcode",
                 nonce: idTokenClaims.nonce,
                 state: TEST_STATE_VALUES.TEST_STATE_REDIRECT,
-                cloud_instance_host_name: "contoso.com",
             };
             const testAccount: AccountInfo = {
                 homeAccountId: TEST_DATA_CLIENT_INFO.TEST_HOME_ACCOUNT_ID,
@@ -574,28 +524,6 @@ describe("InteractionHandler.ts Unit Tests", () => {
                     "handleFragmentResponse"
                 )
                 .returns(testCodeResponse);
-            sinon.stub(Authority.prototype, "isAlias").returns(false);
-            const authorityOptions: AuthorityOptions = {
-                protocolMode: ProtocolMode.AAD,
-                knownAuthorities: ["www.contoso.com"],
-                cloudDiscoveryMetadata: "",
-                authorityMetadata: "",
-            };
-            const authority = new Authority(
-                "https://www.contoso.com/common/",
-                networkInterface,
-                browserStorage,
-                authorityOptions,
-                browserRequestLogger
-            );
-            sinon
-                .stub(AuthorityFactory, "createDiscoveredInstance")
-                .resolves(authority);
-            sinon.stub(Authority.prototype, "discoveryComplete").returns(true);
-            const updateAuthoritySpy = sinon.spy(
-                AuthorizationCodeClient.prototype,
-                "updateAuthority"
-            );
             const acquireTokenSpy = sinon
                 .stub(AuthorizationCodeClient.prototype, "acquireToken")
                 .resolves(testTokenResponse);
@@ -610,7 +538,7 @@ describe("InteractionHandler.ts Unit Tests", () => {
                     state: TEST_STATE_VALUES.TEST_STATE_REDIRECT,
                 },
                 {
-                    authority: "https://www.contoso.com/common/",
+                    authority: TEST_CONFIG.validAuthority,
                     scopes: ["User.Read"],
                     correlationId: TEST_CONFIG.CORRELATION_ID,
                     redirectUri: "/",
@@ -619,12 +547,6 @@ describe("InteractionHandler.ts Unit Tests", () => {
                     state: TEST_STATE_VALUES.TEST_STATE_REDIRECT,
                 }
             );
-            expect(
-                updateAuthoritySpy.calledWith(
-                    "contoso.com",
-                    TEST_CONFIG.CORRELATION_ID
-                )
-            ).toBe(true);
             expect(tokenResponse).toEqual(testTokenResponse);
             expect(
                 acquireTokenSpy.calledWith(

diff --git a/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts b/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts
@@ -112,12 +112,13 @@ describe("RedirectHandler.ts Unit Tests", () => {
             piiLoggingEnabled: true,
         };
         const logger: Logger = new Logger(loggerOptions);
-        authorityInstance = AuthorityFactory.createInstance(
+        authorityInstance = new Authority(
             configObj.auth.authority,
             networkInterface,
             browserStorage,
             authorityOptions,
-            logger
+            logger,
+            TEST_CONFIG.CORRELATION_ID
         );
         browserCrypto = new CryptoOps(logger);
         browserStorage = new BrowserCacheManager(