Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Instrument additional data points #7543

Merged
merged 7 commits into from
Jan 31, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "minor",
"comment": "Instrument additional data points #7543",
"packageName": "@azure/msal-browser",
"email": "[email protected]",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "minor",
"comment": "Instrument additional data points #7543",
"packageName": "@azure/msal-common",
"email": "[email protected]",
"dependentChangeType": "patch"
}
5 changes: 5 additions & 0 deletions lib/msal-browser/src/interaction_client/PopupClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ export class PopupClient extends StandardInteractionClient {
popupWindowParent: request.popupWindowParent ?? window,
};

this.performanceClient.addFields(
{ isAsyncPopup: this.config.system.asyncPopups },
this.correlationId
);

// asyncPopups flag is true. Acquires token without first opening popup. Popup will be opened later asynchronously.
if (this.config.system.asyncPopups) {
this.logger.verbose("asyncPopups set to true, acquiring token");
Expand Down
44 changes: 41 additions & 3 deletions lib/msal-browser/test/interaction_client/PopupClient.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ import { InteractionHandler } from "../../src/interaction_handler/InteractionHan
import { getDefaultPerformanceClient } from "../utils/TelemetryUtils.js";
import { AuthenticationResult } from "../../src/response/AuthenticationResult.js";
import { BrowserCacheManager } from "../../src/cache/BrowserCacheManager.js";
import { BrowserAuthErrorCodes } from "../../src/index.js";
import { BrowserAuthErrorCodes, BrowserUtils } from "../../src/index.js";
import { FetchClient } from "../../src/network/FetchClient.js";

const testPopupWondowDefaults = {
Expand Down Expand Up @@ -206,13 +206,22 @@ describe("PopupClient", () => {
});

it("opens popups asynchronously if configured", async () => {
const perfClient = getDefaultPerformanceClient();
let pca = new PublicClientApplication({
auth: {
clientId: TEST_CONFIG.MSAL_CLIENT_ID,
},
system: {
asyncPopups: true,
},
telemetry: {
client: perfClient,
},
});

let resEvents;
perfClient.addPerformanceCallback((events) => {
resEvents = events;
});

await pca.initialize();
Expand All @@ -237,7 +246,9 @@ describe("PopupClient", () => {
//@ts-ignore
pca.performanceClient,
//@ts-ignore
pca.nativeInternalStorage
pca.nativeInternalStorage,
undefined,
TEST_CONFIG.CORRELATION_ID
);

jest.spyOn(PkceGenerator, "generatePkceCodes").mockResolvedValue({
Expand All @@ -258,13 +269,18 @@ describe("PopupClient", () => {
TEST_CONFIG.TOKEN_TYPE_BEARER as AuthenticationScheme,
};

const rootMeasurement = perfClient.startMeasurement(
"root-measurement",
request.correlationId
);
const popupSpy = jest
.spyOn(PopupClient.prototype, "openSizedPopup")
.mockImplementation();

try {
await popupClient.acquireToken(request);
} catch (e) {}
rootMeasurement.end({ success: true });
expect(popupSpy).toHaveBeenCalled();
expect(popupSpy.mock.calls[0]).toHaveLength(2);
expect(
Expand All @@ -279,16 +295,29 @@ describe("PopupClient", () => {
expect(popupSpy.mock.calls[0][0]).toContain(
`login_hint=${encodeURIComponent(request.loginHint || "")}`
);

// @ts-ignore
const event = resEvents[0];
expect(event.isAsyncPopup).toBeTruthy();
});

it("calls native broker if server responds with accountId", async () => {
const perfClient = getDefaultPerformanceClient();
pca = new PublicClientApplication({
auth: {
clientId: TEST_CONFIG.MSAL_CLIENT_ID,
},
system: {
allowPlatformBroker: true,
},
telemetry: {
client: perfClient,
},
});

let resEvents;
perfClient.addPerformanceCallback((events) => {
resEvents = events;
});

await pca.initialize();
Expand Down Expand Up @@ -370,7 +399,7 @@ describe("PopupClient", () => {
//@ts-ignore
pca.logger,
2000,
getDefaultPerformanceClient()
perfClient
);
//@ts-ignore
popupClient = new PopupClient(
Expand All @@ -392,11 +421,20 @@ describe("PopupClient", () => {
pca.nativeInternalStorage,
nativeMessageHandler
);
const correlationId = BrowserUtils.createGuid();
const rootMeasurement = perfClient.startMeasurement(
"root-measurement",
correlationId
);
const tokenResp = await popupClient.acquireToken({
redirectUri: TEST_URIS.TEST_REDIR_URI,
scopes: TEST_CONFIG.DEFAULT_SCOPES,
correlationId,
});
rootMeasurement.end({ success: true });
expect(tokenResp).toEqual(testTokenResponse);
// @ts-ignore
expect(resEvents[0].isAsyncPopup).toBeFalsy();
});

it("throws if server responds with accountId but extension message handler is not instantiated", async () => {
Expand Down
19 changes: 14 additions & 5 deletions lib/msal-common/apiReview/msal-common.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -3062,6 +3062,15 @@ export type PerformanceEvent = {
retryError?: string;
embeddedClientId?: string;
embeddedRedirectUri?: string;
isAsyncPopup?: boolean;
rtExpiresOnMs?: number;
sidFromClaims?: boolean;
sidFromRequest?: boolean;
loginHintFromRequest?: boolean;
loginHintFromUpn?: boolean;
loginHintFromClaim?: boolean;
domainHintFromRequest?: boolean;
prompt?: string;
};

// Warning: (tsdoc-undefined-tag) The TSDoc tag "@export" is not defined in this configuration
Expand Down Expand Up @@ -4273,12 +4282,12 @@ const X_MS_LIB_CAPABILITY = "x-ms-lib-capability";
// src/client/AuthorizationCodeClient.ts:229:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/AuthorizationCodeClient.ts:307:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/AuthorizationCodeClient.ts:507:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/AuthorizationCodeClient.ts:730:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/AuthorizationCodeClient.ts:790:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/AuthorizationCodeClient.ts:763:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/AuthorizationCodeClient.ts:823:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/RefreshTokenClient.ts:193:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/RefreshTokenClient.ts:277:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/RefreshTokenClient.ts:278:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/RefreshTokenClient.ts:337:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/RefreshTokenClient.ts:286:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/RefreshTokenClient.ts:287:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/RefreshTokenClient.ts:346:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/SilentFlowClient.ts:172:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/config/ClientConfiguration.ts:50:5 - (ae-forgotten-export) The symbol "ClientCredentials" needs to be exported by the entry point index.d.ts
// src/config/ClientConfiguration.ts:51:5 - (ae-forgotten-export) The symbol "LibraryInfo" needs to be exported by the entry point index.d.ts
Expand Down
33 changes: 33 additions & 0 deletions lib/msal-common/src/client/AuthorizationCodeClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -571,8 +571,17 @@ export class AuthorizationCodeClient extends BaseClient {

if (request.domainHint) {
parameterBuilder.addDomainHint(request.domainHint);
this.performanceClient?.addFields(
{ domainHintFromRequest: true },
correlationId
);
}

this.performanceClient?.addFields(
{ prompt: request.prompt },
correlationId
);

// Add sid or loginHint with preference for login_hint claim (in request) -> sid -> loginHint (upn/email) -> username of AccountInfo object
if (request.prompt !== PromptValue.SELECT_ACCOUNT) {
// AAD will throw if prompt=select_account is passed with an account hint
Expand All @@ -582,6 +591,10 @@ export class AuthorizationCodeClient extends BaseClient {
"createAuthCodeUrlQueryString: Prompt is none, adding sid from request"
);
parameterBuilder.addSid(request.sid);
this.performanceClient?.addFields(
{ sidFromRequest: true },
correlationId
);
} else if (request.account) {
const accountSid = this.extractAccountSid(request.account);
let accountLoginHintClaim = this.extractLoginHint(
Expand All @@ -601,6 +614,10 @@ export class AuthorizationCodeClient extends BaseClient {
"createAuthCodeUrlQueryString: login_hint claim present on account"
);
parameterBuilder.addLoginHint(accountLoginHintClaim);
this.performanceClient?.addFields(
{ loginHintFromClaim: true },
correlationId
);
try {
const clientInfo = buildClientInfoFromHomeAccountId(
request.account.homeAccountId
Expand All @@ -620,6 +637,10 @@ export class AuthorizationCodeClient extends BaseClient {
"createAuthCodeUrlQueryString: Prompt is none, adding sid from account"
);
parameterBuilder.addSid(accountSid);
this.performanceClient?.addFields(
{ sidFromClaim: true },
correlationId
);
try {
const clientInfo = buildClientInfoFromHomeAccountId(
request.account.homeAccountId
Expand All @@ -636,12 +657,20 @@ export class AuthorizationCodeClient extends BaseClient {
);
parameterBuilder.addLoginHint(request.loginHint);
parameterBuilder.addCcsUpn(request.loginHint);
this.performanceClient?.addFields(
{ loginHintFromRequest: true },
correlationId
);
} else if (request.account.username) {
// Fallback to account username if provided
this.logger.verbose(
"createAuthCodeUrlQueryString: Adding login_hint from account"
);
parameterBuilder.addLoginHint(request.account.username);
this.performanceClient?.addFields(
{ loginHintFromUpn: true },
correlationId
);
try {
const clientInfo = buildClientInfoFromHomeAccountId(
request.account.homeAccountId
Expand All @@ -659,6 +688,10 @@ export class AuthorizationCodeClient extends BaseClient {
);
parameterBuilder.addLoginHint(request.loginHint);
parameterBuilder.addCcsUpn(request.loginHint);
this.performanceClient?.addFields(
{ loginHintFromRequest: true },
correlationId
);
}
} else {
this.logger.verbose(
Expand Down
27 changes: 18 additions & 9 deletions lib/msal-common/src/client/RefreshTokenClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,10 @@ export class RefreshTokenClient extends BaseClient {
DEFAULT_REFRESH_TOKEN_EXPIRATION_OFFSET_SECONDS
)
) {
this.performanceClient?.addFields(
{ rtExpiresOnMs: Number(refreshToken.expiresOn) },
request.correlationId
);
throw createInteractionRequiredAuthError(
InteractionRequiredAuthErrorCodes.refreshTokenExpired
);
Expand All @@ -256,16 +260,21 @@ export class RefreshTokenClient extends BaseClient {
request.correlationId
)(refreshTokenRequest);
} catch (e) {
if (
e instanceof InteractionRequiredAuthError &&
e.subError === InteractionRequiredAuthErrorCodes.badToken
) {
// Remove bad refresh token from cache
this.logger.verbose(
"acquireTokenWithRefreshToken: bad refresh token, removing from cache"
if (e instanceof InteractionRequiredAuthError) {
this.performanceClient?.addFields(
tnorling marked this conversation as resolved.
Show resolved Hide resolved
{ rtExpiresOnMs: Number(refreshToken.expiresOn) },
request.correlationId
);
const badRefreshTokenKey = generateCredentialKey(refreshToken);
this.cacheManager.removeRefreshToken(badRefreshTokenKey);

if (e.subError === InteractionRequiredAuthErrorCodes.badToken) {
// Remove bad refresh token from cache
this.logger.verbose(
"acquireTokenWithRefreshToken: bad refresh token, removing from cache"
);
const badRefreshTokenKey =
generateCredentialKey(refreshToken);
this.cacheManager.removeRefreshToken(badRefreshTokenKey);
}
}

throw e;
Expand Down
13 changes: 13 additions & 0 deletions lib/msal-common/src/telemetry/performance/PerformanceEvent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -868,6 +868,19 @@ export type PerformanceEvent = {

embeddedClientId?: string;
embeddedRedirectUri?: string;

isAsyncPopup?: boolean;

rtExpiresOnMs?: number;

sidFromClaims?: boolean;
sidFromRequest?: boolean;
loginHintFromRequest?: boolean;
loginHintFromUpn?: boolean;
loginHintFromClaim?: boolean;
domainHintFromRequest?: boolean;

prompt?: string;
};

export type PerformanceEventContext = {
Expand Down
Loading