From 813bda7f0529189645231ff394c4e1afdc68edb8 Mon Sep 17 00:00:00 2001 From: Thomas Norling Date: Thu, 25 Jan 2024 14:07:11 -0800 Subject: [PATCH] Reduce calls to resolveEndpointsAsync (#6838) We are building an `Authority` object and parsing/validating endpoint metadata at least twice in a single request - once to validate it matches the account object passed in and again to build the final request object. This PR refactors to do both these steps at the same time using a single `Authority` object. Additionally, telemetry is added to track the number of times an API is invoked over the course of a request to identify redundancies easier. --- ...-4c8e4b58-0813-4305-b07f-6cd8802c8e46.json | 7 ++ ...-690f75f8-bf40-4dca-9363-0914dfa05c1d.json | 7 ++ .../BaseInteractionClient.ts | 71 +++++++++++-------- .../NativeInteractionClient.ts | 7 +- .../src/interaction_client/PopupClient.ts | 10 ++- .../src/interaction_client/RedirectClient.ts | 10 ++- .../SilentAuthCodeClient.ts | 7 +- .../interaction_client/SilentCacheClient.ts | 10 +-- .../interaction_client/SilentIframeClient.ts | 3 +- .../interaction_client/SilentRefreshClient.ts | 11 +-- .../StandardInteractionClient.ts | 68 +++--------------- .../BaseInteractionClient.spec.ts | 12 +++- lib/msal-common/src/utils/FunctionWrappers.ts | 16 +++++ 13 files changed, 133 insertions(+), 106 deletions(-) create mode 100644 change/@azure-msal-browser-4c8e4b58-0813-4305-b07f-6cd8802c8e46.json create mode 100644 change/@azure-msal-common-690f75f8-bf40-4dca-9363-0914dfa05c1d.json diff --git a/change/@azure-msal-browser-4c8e4b58-0813-4305-b07f-6cd8802c8e46.json b/change/@azure-msal-browser-4c8e4b58-0813-4305-b07f-6cd8802c8e46.json new file mode 100644 index 0000000000..d90fcfcb4c --- /dev/null +++ b/change/@azure-msal-browser-4c8e4b58-0813-4305-b07f-6cd8802c8e46.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "reduce number of calls to resolveEndpoints", + "packageName": "@azure/msal-browser", + "email": "thomas.norling@microsoft.com", + "dependentChangeType": "patch" +} diff --git a/change/@azure-msal-common-690f75f8-bf40-4dca-9363-0914dfa05c1d.json b/change/@azure-msal-common-690f75f8-bf40-4dca-9363-0914dfa05c1d.json new file mode 100644 index 0000000000..a83830f72f --- /dev/null +++ b/change/@azure-msal-common-690f75f8-bf40-4dca-9363-0914dfa05c1d.json @@ -0,0 +1,7 @@ +{ + "type": "minor", + "comment": "Track number of times an API is invoked in a single request", + "packageName": "@azure/msal-common", + "email": "thomas.norling@microsoft.com", + "dependentChangeType": "patch" +} diff --git a/lib/msal-browser/src/interaction_client/BaseInteractionClient.ts b/lib/msal-browser/src/interaction_client/BaseInteractionClient.ts index bee98d77ba..f3f06a3ba3 100644 --- a/lib/msal-browser/src/interaction_client/BaseInteractionClient.ts +++ b/lib/msal-browser/src/interaction_client/BaseInteractionClient.ts @@ -22,6 +22,8 @@ import { IPerformanceClient, PerformanceEvents, StringUtils, + AzureCloudOptions, + invokeAsync, } from "@azure/msal-common"; import { BrowserConfiguration } from "../config/Configuration"; import { BrowserCacheManager } from "../cache/BrowserCacheManager"; @@ -136,8 +138,7 @@ export abstract class BaseInteractionClient { * @param request */ protected async initializeBaseRequest( - request: Partial, - account?: AccountInfo + request: Partial ): Promise { this.performanceClient.addQueueMeasurement( PerformanceEvents.InitializeBaseRequest, @@ -145,10 +146,6 @@ export abstract class BaseInteractionClient { ); const authority = request.authority || this.config.auth.authority; - if (account) { - await this.validateRequestAuthority(authority, account); - } - const scopes = [...((request && request.scopes) || [])]; const validatedRequest: BaseAuthRequest = { @@ -218,25 +215,6 @@ export abstract class BaseInteractionClient { ); } - /* - * If authority provided in the request does not match environment/authority specified - * in the account or MSAL config, we throw an error. - */ - async validateRequestAuthority( - authority: string, - account: AccountInfo - ): Promise { - const discoveredAuthority = await this.getDiscoveredAuthority( - authority - ); - - if (!discoveredAuthority.isAlias(account.environment)) { - throw createClientConfigurationError( - ClientConfigurationErrorCodes.authorityMismatch - ); - } - } - /** * * @param apiId @@ -266,23 +244,46 @@ export abstract class BaseInteractionClient { /** * Used to get a discovered version of the default authority. * @param requestAuthority + * @param requestAzureCloudOptions + * @param account */ protected async getDiscoveredAuthority( - requestAuthority?: string + requestAuthority?: string, + requestAzureCloudOptions?: AzureCloudOptions, + account?: AccountInfo ): Promise { - this.logger.verbose("getDiscoveredAuthority called"); + this.performanceClient.addQueueMeasurement( + PerformanceEvents.StandardInteractionClientGetDiscoveredAuthority, + this.correlationId + ); const authorityOptions: AuthorityOptions = { protocolMode: this.config.auth.protocolMode, OIDCOptions: this.config.auth.OIDCOptions, knownAuthorities: this.config.auth.knownAuthorities, cloudDiscoveryMetadata: this.config.auth.cloudDiscoveryMetadata, authorityMetadata: this.config.auth.authorityMetadata, + skipAuthorityMetadataCache: + this.config.auth.skipAuthorityMetadataCache, }; - const authority = requestAuthority || this.config.auth.authority; - this.logger.verbose(`Creating discovered authority with ${authority}`); - return AuthorityFactory.createDiscoveredInstance( - authority, + // build authority string based on auth params, precedence - azureCloudInstance + tenant >> authority + const userAuthority = requestAuthority + ? requestAuthority + : this.config.auth.authority; + + // fall back to the authority from config + const builtAuthority = Authority.generateAuthority( + userAuthority, + requestAzureCloudOptions || this.config.auth.azureCloudOptions + ); + const discoveredAuthority = await invokeAsync( + AuthorityFactory.createDiscoveredInstance, + PerformanceEvents.AuthorityFactoryCreateDiscoveredInstance, + this.logger, + this.performanceClient, + this.correlationId + )( + builtAuthority, this.config.system.networkClient, this.browserStorage, authorityOptions, @@ -290,5 +291,13 @@ export abstract class BaseInteractionClient { this.correlationId, this.performanceClient ); + + if (account && !discoveredAuthority.isAlias(account.environment)) { + throw createClientConfigurationError( + ClientConfigurationErrorCodes.authorityMismatch + ); + } + + return discoveredAuthority; } } diff --git a/lib/msal-browser/src/interaction_client/NativeInteractionClient.ts b/lib/msal-browser/src/interaction_client/NativeInteractionClient.ts index 3600765302..ad12a36b7d 100644 --- a/lib/msal-browser/src/interaction_client/NativeInteractionClient.ts +++ b/lib/msal-browser/src/interaction_client/NativeInteractionClient.ts @@ -809,7 +809,12 @@ export class NativeInteractionClient extends BaseInteractionClient { const authority = request.authority || this.config.auth.authority; if (request.account) { - await this.validateRequestAuthority(authority, request.account); + // validate authority + await this.getDiscoveredAuthority( + authority, + request.azureCloudOptions, + request.account + ); } const canonicalAuthority = new UrlString(authority); diff --git a/lib/msal-browser/src/interaction_client/PopupClient.ts b/lib/msal-browser/src/interaction_client/PopupClient.ts index d82f6a92a1..a46b88986e 100644 --- a/lib/msal-browser/src/interaction_client/PopupClient.ts +++ b/lib/msal-browser/src/interaction_client/PopupClient.ts @@ -232,7 +232,8 @@ export class PopupClient extends StandardInteractionClient { )( serverTelemetryManager, validRequest.authority, - validRequest.azureCloudOptions + validRequest.azureCloudOptions, + validRequest.account ); const isNativeBroker = NativeMessageHandler.isNativeAvailable( @@ -406,7 +407,12 @@ export class PopupClient extends StandardInteractionClient { this.logger, this.performanceClient, this.correlationId - )(serverTelemetryManager, requestAuthority); + )( + serverTelemetryManager, + requestAuthority, + undefined, // AzureCloudOptions + validRequest.account || undefined + ); try { authClient.authority.endSessionEndpoint; diff --git a/lib/msal-browser/src/interaction_client/RedirectClient.ts b/lib/msal-browser/src/interaction_client/RedirectClient.ts index c33fc88bea..9355836d05 100644 --- a/lib/msal-browser/src/interaction_client/RedirectClient.ts +++ b/lib/msal-browser/src/interaction_client/RedirectClient.ts @@ -136,7 +136,8 @@ export class RedirectClient extends StandardInteractionClient { )( serverTelemetryManager, validRequest.authority, - validRequest.azureCloudOptions + validRequest.azureCloudOptions, + validRequest.account ); // Create redirect interaction handler. @@ -520,7 +521,12 @@ export class RedirectClient extends StandardInteractionClient { this.logger, this.performanceClient, this.correlationId - )(serverTelemetryManager, logoutRequest && logoutRequest.authority); + )( + serverTelemetryManager, + logoutRequest && logoutRequest.authority, + undefined, // AzureCloudOptions + (logoutRequest && logoutRequest.account) || undefined + ); if (authClient.authority.protocolMode === ProtocolMode.OIDC) { try { diff --git a/lib/msal-browser/src/interaction_client/SilentAuthCodeClient.ts b/lib/msal-browser/src/interaction_client/SilentAuthCodeClient.ts index 48b57603d8..3d924da196 100644 --- a/lib/msal-browser/src/interaction_client/SilentAuthCodeClient.ts +++ b/lib/msal-browser/src/interaction_client/SilentAuthCodeClient.ts @@ -99,7 +99,12 @@ export class SilentAuthCodeClient extends StandardInteractionClient { this.logger, this.performanceClient, request.correlationId - )(serverTelemetryManager, silentRequest.authority); + )( + serverTelemetryManager, + silentRequest.authority, + silentRequest.azureCloudOptions, + silentRequest.account + ); const authClient: HybridSpaAuthorizationCodeClient = new HybridSpaAuthorizationCodeClient(clientConfig); this.logger.verbose("Auth code client created"); diff --git a/lib/msal-browser/src/interaction_client/SilentCacheClient.ts b/lib/msal-browser/src/interaction_client/SilentCacheClient.ts index dee233431f..3070c639d4 100644 --- a/lib/msal-browser/src/interaction_client/SilentCacheClient.ts +++ b/lib/msal-browser/src/interaction_client/SilentCacheClient.ts @@ -42,7 +42,8 @@ export class SilentCacheClient extends StandardInteractionClient { const silentAuthClient = await this.createSilentFlowClient( serverTelemetryManager, silentRequest.authority, - silentRequest.azureCloudOptions + silentRequest.azureCloudOptions, + silentRequest.account ); this.logger.verbose("Silent auth client created"); @@ -94,7 +95,8 @@ export class SilentCacheClient extends StandardInteractionClient { protected async createSilentFlowClient( serverTelemetryManager: ServerTelemetryManager, authorityUrl?: string, - azureCloudOptions?: AzureCloudOptions + azureCloudOptions?: AzureCloudOptions, + account?: AccountInfo ): Promise { // Create auth module. const clientConfig = await invokeAsync( @@ -103,7 +105,7 @@ export class SilentCacheClient extends StandardInteractionClient { this.logger, this.performanceClient, this.correlationId - )(serverTelemetryManager, authorityUrl, azureCloudOptions); + )(serverTelemetryManager, authorityUrl, azureCloudOptions, account); return new SilentFlowClient(clientConfig, this.performanceClient); } @@ -122,7 +124,7 @@ export class SilentCacheClient extends StandardInteractionClient { this.logger, this.performanceClient, this.correlationId - )(request, account); + )(request); return { ...request, ...baseRequest, diff --git a/lib/msal-browser/src/interaction_client/SilentIframeClient.ts b/lib/msal-browser/src/interaction_client/SilentIframeClient.ts index e185a60ddb..5d8e45f091 100644 --- a/lib/msal-browser/src/interaction_client/SilentIframeClient.ts +++ b/lib/msal-browser/src/interaction_client/SilentIframeClient.ts @@ -135,7 +135,8 @@ export class SilentIframeClient extends StandardInteractionClient { )( serverTelemetryManager, silentRequest.authority, - silentRequest.azureCloudOptions + silentRequest.azureCloudOptions, + silentRequest.account ); return await invokeAsync( diff --git a/lib/msal-browser/src/interaction_client/SilentRefreshClient.ts b/lib/msal-browser/src/interaction_client/SilentRefreshClient.ts index 5e094ee0eb..7d9989db0a 100644 --- a/lib/msal-browser/src/interaction_client/SilentRefreshClient.ts +++ b/lib/msal-browser/src/interaction_client/SilentRefreshClient.ts @@ -12,6 +12,7 @@ import { AzureCloudOptions, PerformanceEvents, invokeAsync, + AccountInfo, } from "@azure/msal-common"; import { ApiId } from "../utils/BrowserConstants"; import { @@ -39,7 +40,7 @@ export class SilentRefreshClient extends StandardInteractionClient { this.logger, this.performanceClient, request.correlationId - )(request, request.account); + )(request); const silentRequest: CommonSilentFlowRequest = { ...request, ...baseRequest, @@ -59,7 +60,8 @@ export class SilentRefreshClient extends StandardInteractionClient { const refreshTokenClient = await this.createRefreshTokenClient( serverTelemetryManager, silentRequest.authority, - silentRequest.azureCloudOptions + silentRequest.azureCloudOptions, + silentRequest.account ); // Send request to renew token. Auth module will throw errors if token cannot be renewed. return invokeAsync( @@ -97,7 +99,8 @@ export class SilentRefreshClient extends StandardInteractionClient { protected async createRefreshTokenClient( serverTelemetryManager: ServerTelemetryManager, authorityUrl?: string, - azureCloudOptions?: AzureCloudOptions + azureCloudOptions?: AzureCloudOptions, + account?: AccountInfo ): Promise { // Create auth module. const clientConfig = await invokeAsync( @@ -106,7 +109,7 @@ export class SilentRefreshClient extends StandardInteractionClient { this.logger, this.performanceClient, this.correlationId - )(serverTelemetryManager, authorityUrl, azureCloudOptions); + )(serverTelemetryManager, authorityUrl, azureCloudOptions, account); return new RefreshTokenClient(clientConfig, this.performanceClient); } } diff --git a/lib/msal-browser/src/interaction_client/StandardInteractionClient.ts b/lib/msal-browser/src/interaction_client/StandardInteractionClient.ts index acfec17800..3ca4e929e1 100644 --- a/lib/msal-browser/src/interaction_client/StandardInteractionClient.ts +++ b/lib/msal-browser/src/interaction_client/StandardInteractionClient.ts @@ -9,9 +9,6 @@ import { Constants, AuthorizationCodeClient, ClientConfiguration, - AuthorityOptions, - Authority, - AuthorityFactory, UrlString, CommonEndSessionRequest, ProtocolUtils, @@ -207,7 +204,8 @@ export abstract class StandardInteractionClient extends BaseInteractionClient { protected async createAuthCodeClient( serverTelemetryManager: ServerTelemetryManager, authorityUrl?: string, - requestAzureCloudOptions?: AzureCloudOptions + requestAzureCloudOptions?: AzureCloudOptions, + account?: AccountInfo ): Promise { this.performanceClient.addQueueMeasurement( PerformanceEvents.StandardInteractionClientCreateAuthCodeClient, @@ -220,7 +218,12 @@ export abstract class StandardInteractionClient extends BaseInteractionClient { this.logger, this.performanceClient, this.correlationId - )(serverTelemetryManager, authorityUrl, requestAzureCloudOptions); + )( + serverTelemetryManager, + authorityUrl, + requestAzureCloudOptions, + account + ); return new AuthorizationCodeClient( clientConfig, this.performanceClient @@ -236,7 +239,8 @@ export abstract class StandardInteractionClient extends BaseInteractionClient { protected async getClientConfiguration( serverTelemetryManager: ServerTelemetryManager, requestAuthority?: string, - requestAzureCloudOptions?: AzureCloudOptions + requestAzureCloudOptions?: AzureCloudOptions, + account?: AccountInfo ): Promise { this.performanceClient.addQueueMeasurement( PerformanceEvents.StandardInteractionClientGetClientConfiguration, @@ -248,7 +252,7 @@ export abstract class StandardInteractionClient extends BaseInteractionClient { this.logger, this.performanceClient, this.correlationId - )(requestAuthority, requestAzureCloudOptions); + )(requestAuthority, requestAzureCloudOptions, account); const logger = this.config.system.loggerOptions; return { @@ -286,56 +290,6 @@ export abstract class StandardInteractionClient extends BaseInteractionClient { }; } - /** - * Used to get a discovered version of the default authority. - * @param requestAuthority - * @param requestCorrelationId - */ - protected async getDiscoveredAuthority( - requestAuthority?: string, - requestAzureCloudOptions?: AzureCloudOptions - ): Promise { - this.performanceClient.addQueueMeasurement( - PerformanceEvents.StandardInteractionClientGetDiscoveredAuthority, - this.correlationId - ); - const authorityOptions: AuthorityOptions = { - protocolMode: this.config.auth.protocolMode, - OIDCOptions: this.config.auth.OIDCOptions, - knownAuthorities: this.config.auth.knownAuthorities, - cloudDiscoveryMetadata: this.config.auth.cloudDiscoveryMetadata, - authorityMetadata: this.config.auth.authorityMetadata, - skipAuthorityMetadataCache: - this.config.auth.skipAuthorityMetadataCache, - }; - - // build authority string based on auth params, precedence - azureCloudInstance + tenant >> authority - const userAuthority = requestAuthority - ? requestAuthority - : this.config.auth.authority; - - // fall back to the authority from config - const builtAuthority = Authority.generateAuthority( - userAuthority, - requestAzureCloudOptions || this.config.auth.azureCloudOptions - ); - return invokeAsync( - AuthorityFactory.createDiscoveredInstance.bind(AuthorityFactory), - PerformanceEvents.AuthorityFactoryCreateDiscoveredInstance, - this.logger, - this.performanceClient, - this.correlationId - )( - builtAuthority, - this.config.system.networkClient, - this.browserStorage, - authorityOptions, - this.logger, - this.correlationId, - this.performanceClient - ); - } - /** * Helper to initialize required request parameters for interactive APIs and ssoSilent() * @param request diff --git a/lib/msal-browser/test/interaction_client/BaseInteractionClient.spec.ts b/lib/msal-browser/test/interaction_client/BaseInteractionClient.spec.ts index 5424711385..ef06bbcd24 100644 --- a/lib/msal-browser/test/interaction_client/BaseInteractionClient.spec.ts +++ b/lib/msal-browser/test/interaction_client/BaseInteractionClient.spec.ts @@ -65,6 +65,8 @@ describe("BaseInteractionClient", () => { // @ts-ignore pca.eventHandler, // @ts-ignore + pca.navigationClient, + // @ts-ignore pca.performanceClient ); }); @@ -202,7 +204,7 @@ describe("BaseInteractionClient", () => { expect(pca.getActiveAccount()).toBe(null); }); }); - describe("validateRequestAuthority()", () => { + describe("getDiscoveredAuthority()", () => { afterEach(() => { window.sessionStorage.clear(); }); @@ -217,8 +219,10 @@ describe("BaseInteractionClient", () => { }; await testClient - .validateRequestAuthority( + // @ts-ignore + .getDiscoveredAuthority( "https://login.microsoftonline.com/common", + undefined, // AzureCloudOptions testAccount ) .then(() => { @@ -243,8 +247,10 @@ describe("BaseInteractionClient", () => { }; testClient - .validateRequestAuthority( + // @ts-ignore + .getDiscoveredAuthority( "https://login.microsoftonline.com/common", + undefined, // AzureCloudOptions testAccount ) .then(() => { diff --git a/lib/msal-common/src/utils/FunctionWrappers.ts b/lib/msal-common/src/utils/FunctionWrappers.ts index c780b3257a..cb33e0e4ff 100644 --- a/lib/msal-common/src/utils/FunctionWrappers.ts +++ b/lib/msal-common/src/utils/FunctionWrappers.ts @@ -31,6 +31,14 @@ export const invoke = , U>( eventName, correlationId ); + if (correlationId) { + // Track number of times this API is called in a single request + const eventCount = eventName + "CallCount"; + telemetryClient?.incrementFields( + { [eventCount]: 1 }, + correlationId + ); + } try { const result = callback(...args); inProgressEvent?.end({ @@ -79,6 +87,14 @@ export const invokeAsync = , U>( eventName, correlationId ); + if (correlationId) { + // Track number of times this API is called in a single request + const eventCount = eventName + "CallCount"; + telemetryClient?.incrementFields( + { [eventCount]: 1 }, + correlationId + ); + } telemetryClient?.setPreQueueTime(eventName, correlationId); return callback(...args) .then((response) => {