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

Track MSAL node SKU for broker flows #7213

Merged
merged 2 commits into from
Jul 19, 2024
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": "Track MSAL node SKU for broker flows #7213",
"packageName": "@azure/msal-browser",
"email": "kshabelko@microsoft.com",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "minor",
"comment": "Track MSAL node SKU for broker flows #7213",
"packageName": "@azure/msal-common",
"email": "kshabelko@microsoft.com",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "minor",
"comment": "Track MSAL node SKU for broker flows #7213",
"packageName": "@azure/msal-node",
"email": "kshabelko@microsoft.com",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "minor",
"comment": "Track MSAL node SKU for broker flows #7213",
"packageName": "@azure/msal-node-extensions",
"email": "kshabelko@microsoft.com",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
*/

import {
AADServerParamKeys,
AccountInfo,
AuthenticationResult,
AuthenticationScheme,
Expand All @@ -21,6 +22,7 @@ import {
NativeSignOutRequest,
PromptValue,
ServerError,
ServerTelemetryManager,
} from "@azure/msal-common";
import {
msalNodeRuntime,
Expand Down Expand Up @@ -487,6 +489,23 @@ export class NativeBrokerPlugin implements INativeBrokerPlugin {
}
);
}

const skus =
request.extraParameters &&
request.extraParameters[AADServerParamKeys.X_CLIENT_EXTRA_SKU]
?.length
? request.extraParameters[
AADServerParamKeys.X_CLIENT_EXTRA_SKU
]
: "";
authParams.SetAdditionalParameter(
AADServerParamKeys.X_CLIENT_EXTRA_SKU,
ServerTelemetryManager.makeExtraSkuString({
skus,
extensionName: "msal.node.ext",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this a constant?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, we can't as skus is picked up from request.extraParameters.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was referring to msal.node.ext

extensionVersion: version,
})
);
} catch (e) {
const wrappedError = this.wrapError(e);
if (wrappedError) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,34 +81,6 @@ const BrokerServerParamKeys = {
BROKER_REDIRECT_URI: "brk_redirect_uri",
};

/**
* Provides MSAL and browser extension SKUs
* @param messageHandler {NativeMessageHandler}
*/
function getSKUs(messageHandler: NativeMessageHandler): string {
const groupSeparator = ",";
const valueSeparator = "|";
const skus = Array.from({ length: 4 }, () => valueSeparator);
// Report MSAL SKU
skus[0] = [BrowserConstants.MSAL_SKU, version].join(valueSeparator);

// Report extension SKU
const extensionName =
messageHandler.getExtensionId() ===
NativeConstants.PREFERRED_EXTENSION_ID
? "chrome"
: messageHandler.getExtensionId()?.length
? "unknown"
: undefined;

if (extensionName) {
skus[2] = [extensionName, messageHandler.getExtensionVersion()].join(
valueSeparator
);
}
return skus.join(groupSeparator);
}

export class NativeInteractionClient extends BaseInteractionClient {
protected apiId: ApiId;
protected accountId: string;
Expand Down Expand Up @@ -161,7 +133,20 @@ export class NativeInteractionClient extends BaseInteractionClient {
this.serverTelemetryManager = this.initializeServerTelemetryManager(
this.apiId
);
this.skus = getSKUs(this.nativeMessageHandler);

const extensionName =
this.nativeMessageHandler.getExtensionId() ===
NativeConstants.PREFERRED_EXTENSION_ID
? "chrome"
: this.nativeMessageHandler.getExtensionId()?.length
? "unknown"
: undefined;
this.skus = ServerTelemetryManager.makeExtraSkuString({
libraryName: BrowserConstants.MSAL_SKU,
libraryVersion: version,
extensionName: extensionName,
extensionVersion: this.nativeMessageHandler.getExtensionVersion(),
});
}

/**
Expand Down
4 changes: 4 additions & 0 deletions lib/msal-common/apiReview/msal-common.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -3664,6 +3664,10 @@ export class ServerTelemetryManager {
getNativeBrokerErrorCode(): string | undefined;
getRegionDiscoveryFields(): string;
incrementCacheHits(): number;
// Warning: (ae-forgotten-export) The symbol "SkuParams" needs to be exported by the entry point index.d.ts
//
// (undocumented)
static makeExtraSkuString(params: SkuParams): string;
// Warning: (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
static maxErrorsToSend(serverTelemetryEntity: ServerTelemetryEntity): number;
setCacheOutcome(cacheOutcome: CacheOutcome): void;
Expand Down
67 changes: 67 additions & 0 deletions lib/msal-common/src/telemetry/server/ServerTelemetryManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,69 @@ import { ServerTelemetryRequest } from "./ServerTelemetryRequest";
import { ServerTelemetryEntity } from "../../cache/entities/ServerTelemetryEntity";
import { RegionDiscoveryMetadata } from "../../authority/RegionDiscoveryMetadata";

const skuGroupSeparator = ",";
const skuValueSeparator = "|";

type SkuParams = {
libraryName?: string;
libraryVersion?: string;
extensionName?: string;
extensionVersion?: string;
skus?: string;
};

function makeExtraSkuString(params: SkuParams): string {
const {
skus,
libraryName,
libraryVersion,
extensionName,
extensionVersion,
} = params;
const skuMap: Map<number, (string | undefined)[]> = new Map([
[0, [libraryName, libraryVersion]],
[2, [extensionName, extensionVersion]],
]);
let skuArr: string[] = [];

if (skus?.length) {
skuArr = skus.split(skuGroupSeparator);

// Ignore invalid input sku param
if (skuArr.length < 4) {
return skus;
}
} else {
skuArr = Array.from({ length: 4 }, () => skuValueSeparator);
}

skuMap.forEach((value, key) => {
if (value.length === 2 && value[0]?.length && value[1]?.length) {
setSku({
skuArr,
index: key,
skuName: value[0],
skuVersion: value[1],
});
}
});

return skuArr.join(skuGroupSeparator);
}

function setSku(params: {
skuArr: string[];
index: number;
skuName: string;
skuVersion: string;
}): void {
const { skuArr, index, skuName, skuVersion } = params;
if (index >= skuArr.length) {
return;
}
skuArr[index] = [skuName, skuVersion].join(skuValueSeparator);
}

/** @internal */
export class ServerTelemetryManager {
private cacheManager: CacheManager;
Expand Down Expand Up @@ -304,4 +367,8 @@ export class ServerTelemetryManager {
lastRequests
);
}

static makeExtraSkuString(params: SkuParams): string {
return makeExtraSkuString(params);
}
}
52 changes: 52 additions & 0 deletions lib/msal-common/test/telemetry/ServerTelemetryManager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -402,4 +402,56 @@ describe("ServerTelemetryManager.ts", () => {
) as ServerTelemetryEntity;
expect(cacheValue.cacheHits).toBe(2);
});

describe("makeExtraSkuString", () => {
it("Creates empty string from scratch", () => {
const skus = ServerTelemetryManager.makeExtraSkuString({});
expect(skus).toEqual("|,|,|,|");
});

it("Does not modify input string", () => {
const skus = ServerTelemetryManager.makeExtraSkuString({
skus: "test_sku|1.0.0,|,|,|",
});
expect(skus).toEqual("test_sku|1.0.0,|,|,|");
});

it("Returns invalid input", () => {
const skus = ServerTelemetryManager.makeExtraSkuString({
skus: "test_sku|1.0.0,|,|",
libraryName: "test_lib_name",
libraryVersion: "1.2.3",
});
expect(skus).toEqual("test_sku|1.0.0,|,|");
});

it("Adds library and extension info", () => {
const skus = ServerTelemetryManager.makeExtraSkuString({
skus: "test_sku|1.0.0,|,test_ext_sku|2.0.0,|",
libraryName: "test_lib_name",
libraryVersion: "1.2.3",
extensionName: "test_ext_name",
extensionVersion: "5.6.7",
});
expect(skus).toEqual("test_lib_name|1.2.3,|,test_ext_name|5.6.7,|");
});

it("Updates input string with library info", () => {
const skus = ServerTelemetryManager.makeExtraSkuString({
skus: "test_sku|1.0.0,|,test_ext_sku|2.0.0,|",
libraryName: "test_lib_name",
libraryVersion: "1.2.3",
});
expect(skus).toEqual("test_lib_name|1.2.3,|,test_ext_sku|2.0.0,|");
});

it("Updates input string with extension info", () => {
const skus = ServerTelemetryManager.makeExtraSkuString({
skus: "test_sku|1.0.0,|,test_ext_sku|2.0.0,|",
extensionName: "test_ext_name",
extensionVersion: "5.6.7",
});
expect(skus).toEqual("test_sku|1.0.0,|,test_ext_name|5.6.7,|");
});
});
});
2 changes: 1 addition & 1 deletion lib/msal-node/apiReview/msal-node.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -758,7 +758,7 @@ export const version = "2.11.1";
// src/client/OnBehalfOfClient.ts:249:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/OnBehalfOfClient.ts:250:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/OnBehalfOfClient.ts:310:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/PublicClientApplication.ts:298:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/PublicClientApplication.ts:310:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/UsernamePasswordClient.ts:74:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/UsernamePasswordClient.ts:75:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/client/UsernamePasswordClient.ts:114:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
Expand Down
14 changes: 13 additions & 1 deletion lib/msal-node/src/client/PublicClientApplication.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import {
AccountInfo,
INativeBrokerPlugin,
ServerAuthorizationCodeResponse,
AADServerParamKeys,
ServerTelemetryManager,
} from "@azure/msal-common";
import { Configuration } from "../config/Configuration.js";
import { ClientApplication } from "./ClientApplication.js";
Expand All @@ -36,6 +38,7 @@ import { SilentFlowRequest } from "../request/SilentFlowRequest.js";
import { SignOutRequest } from "../request/SignOutRequest.js";
import { ILoopbackClient } from "../network/ILoopbackClient.js";
import { DeviceCodeClient } from "./DeviceCodeClient.js";
import { version } from "../packageMetadata";

/**
* This class is to be used to acquire tokens for public client applications (desktop, mobile). Public client applications
Expand All @@ -47,6 +50,7 @@ export class PublicClientApplication
implements IPublicClientApplication
{
private nativeBrokerPlugin?: INativeBrokerPlugin;
private readonly skus: string;
/**
* Important attributes in the Configuration object for auth are:
* - clientID: the application ID of your application. You can obtain one by registering your application with our Application registration portal.
Expand Down Expand Up @@ -78,6 +82,10 @@ export class PublicClientApplication
);
}
}
this.skus = ServerTelemetryManager.makeExtraSkuString({
libraryName: Constants.MSAL_SKU,
libraryVersion: version,
});
}

/**
Expand Down Expand Up @@ -156,6 +164,7 @@ export class PublicClientApplication
extraParameters: {
...remainingProperties.extraQueryParameters,
...remainingProperties.tokenQueryParameters,
[AADServerParamKeys.X_CLIENT_EXTRA_SKU]: this.skus,
},
accountId: remainingProperties.account?.nativeAccountId,
};
Expand Down Expand Up @@ -247,7 +256,10 @@ export class PublicClientApplication
redirectUri: `${Constants.HTTP_PROTOCOL}${Constants.LOCALHOST}`,
authority: request.authority || this.config.auth.authority,
correlationId: correlationId,
extraParameters: request.tokenQueryParameters,
extraParameters: {
...request.tokenQueryParameters,
[AADServerParamKeys.X_CLIENT_EXTRA_SKU]: this.skus,
},
accountId: request.account.nativeAccountId,
forceRefresh: request.forceRefresh || false,
};
Expand Down
29 changes: 29 additions & 0 deletions lib/msal-node/test/client/PublicClientApplication.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
CacheHelpers,
AuthorityFactory,
ProtocolMode,
AADServerParamKeys,
} from "@azure/msal-common";
import {
Configuration,
Expand Down Expand Up @@ -53,6 +54,7 @@ import { ClientAuthErrorCodes } from "@azure/msal-common";
import { TEST_CONFIG } from "../test_kit/StringConstants";
import { HttpClient } from "../../src/network/HttpClient";
import { MockStorageClass } from "./ClientTestUtils";
import { Constants } from "../../src/utils/Constants";

const msalCommon: MSALCommonModule = jest.requireActual("@azure/msal-common");

Expand Down Expand Up @@ -256,6 +258,33 @@ describe("PublicClientApplication", () => {
);
});

test("acquireTokenSilent sends extra telemetry to NativeBrokerPlugin", async () => {
const authApp = new PublicClientApplication({
...appConfig,
broker: {
nativeBrokerPlugin: new MockNativeBrokerPlugin(),
},
});

const request: SilentFlowRequest = {
account: mockNativeAccountInfo,
scopes: TEST_CONSTANTS.DEFAULT_GRAPH_SCOPE,
};
const brokerSpy: jest.SpyInstance<unknown, [...unknown[]]> =
jest.spyOn(
MockNativeBrokerPlugin.prototype,
"acquireTokenSilent"
);
await authApp.acquireTokenSilent(request);
const nativeRequest = brokerSpy.mock.calls[0][0];
expect(nativeRequest).toHaveProperty("extraParameters");
// @ts-ignore
expect(nativeRequest.extraParameters).toHaveProperty(
AADServerParamKeys.X_CLIENT_EXTRA_SKU,
`${Constants.MSAL_SKU}|${version},|,|,|`
);
});

test("acquireTokenSilent - calls into NativeBrokerPlugin and throws", (done) => {
const authApp = new PublicClientApplication({
...appConfig,
Expand Down
Loading