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 SKU for broker flows #7182

Merged
merged 12 commits into from
Jul 17, 2024
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "minor",
"comment": "Track MSAL SKU for broker flows #7182",
"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": "Track MSAL SKU for broker flows #7182",
"packageName": "@azure/msal-common",
"email": "[email protected]",
"dependentChangeType": "patch"
}
174 changes: 120 additions & 54 deletions lib/msal-browser/src/interaction_client/NativeInteractionClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import {
CacheHelpers,
buildAccountToCache,
InProgressPerformanceEvent,
ServerTelemetryManager,
} from "@azure/msal-common";
import { BaseInteractionClient } from "./BaseInteractionClient";
import { BrowserConfiguration } from "../config/Configuration";
Expand All @@ -50,6 +51,7 @@ import {
ApiId,
TemporaryCacheKeys,
NativeConstants,
BrowserConstants,
} from "../utils/BrowserConstants";
import {
NativeExtensionRequestBody,
Expand All @@ -72,18 +74,49 @@ import {
import { SilentCacheClient } from "./SilentCacheClient";
import { AuthenticationResult } from "../response/AuthenticationResult";
import { base64Decode } from "../encode/Base64Decode";
import { version } from "../packageMetadata";

const BrokerServerParamKeys = {
BROKER_CLIENT_ID: "brk_client_id",
BROKER_REDIRECT_URI: "brk_redirect_uri",
};

/**
* Provides MSAL and browser extension SKUs
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explicitly document what you expect to have in skus[0..4].

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we need to document it here in the code given that we have a shared spec?

* @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;
protected nativeMessageHandler: NativeMessageHandler;
protected silentCacheClient: SilentCacheClient;
protected nativeStorageManager: BrowserCacheManager;
protected skus: string;
protected serverTelemetryManager: ServerTelemetryManager;

constructor(
config: BrowserConfiguration,
Expand Down Expand Up @@ -125,6 +158,22 @@ export class NativeInteractionClient extends BaseInteractionClient {
provider,
correlationId
);
this.serverTelemetryManager = this.initializeServerTelemetryManager(
this.apiId
);
this.skus = getSKUs(this.nativeMessageHandler);
}

/**
* Adds SKUs to request extra query parameters
* @param request {NativeTokenRequest}
* @private
*/
private addRequestSKUs(request: NativeTokenRequest) {
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.

Also, we should not modify input params. You should return a value instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one modifies validatedRequest which is an extended copy of request. I do not think we need to add another spread operator here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We fixed a bug related to this pattern last week and I have a work item open to resolve remaining instances & turn on linting to catch in the future. We can discuss offline if you like.

...request.extraParameters,
[AADServerParamKeys.X_CLIENT_EXTRA_SKU]: this.skus,
};
}

/**
Expand All @@ -147,64 +196,73 @@ export class NativeInteractionClient extends BaseInteractionClient {
);
const reqTimestamp = TimeUtils.nowSeconds();

// initialize native request
const nativeRequest = await this.initializeNativeRequest(request);

// check if the tokens can be retrieved from internal cache
try {
const result = await this.acquireTokensFromCache(
this.accountId,
nativeRequest
);
nativeATMeasurement.end({
success: true,
isNativeBroker: false, // Should be true only when the result is coming directly from the broker
fromCache: true,
});
return result;
} catch (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"
);
}
// initialize native request
const nativeRequest = await this.initializeNativeRequest(request);

const { ...nativeTokenRequest } = nativeRequest;

// fall back to native calls
const messageBody: NativeExtensionRequestBody = {
method: NativeExtensionMethod.GetToken,
request: nativeTokenRequest,
};

const response: object = await this.nativeMessageHandler.sendMessage(
messageBody
);
const validatedResponse: NativeResponse =
this.validateNativeResponse(response);

return this.handleNativeResponse(
validatedResponse,
nativeRequest,
reqTimestamp
)
.then((result: AuthenticationResult) => {
// check if the tokens can be retrieved from internal cache
try {
const result = await this.acquireTokensFromCache(
this.accountId,
nativeRequest
);
nativeATMeasurement.end({
success: true,
isNativeBroker: true,
requestId: result.requestId,
isNativeBroker: false, // Should be true only when the result is coming directly from the broker
fromCache: true,
});
return result;
})
.catch((error: AuthError) => {
nativeATMeasurement.end({
success: false,
errorCode: error.errorCode,
subErrorCode: error.subError,
isNativeBroker: true,
} catch (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"
);
}

const { ...nativeTokenRequest } = nativeRequest;

// fall back to native calls
const messageBody: NativeExtensionRequestBody = {
method: NativeExtensionMethod.GetToken,
request: nativeTokenRequest,
};

const response: object =
await this.nativeMessageHandler.sendMessage(messageBody);
const validatedResponse: NativeResponse =
this.validateNativeResponse(response);

return await this.handleNativeResponse(
validatedResponse,
nativeRequest,
reqTimestamp
)
.then((result: AuthenticationResult) => {
nativeATMeasurement.end({
success: true,
isNativeBroker: true,
requestId: result.requestId,
});
this.serverTelemetryManager.clearNativeBrokerErrorCode();
return result;
})
.catch((error: AuthError) => {
nativeATMeasurement.end({
success: false,
errorCode: error.errorCode,
subErrorCode: error.subError,
isNativeBroker: true,
});
throw error;
});
throw error;
});
} catch (e) {
if (e instanceof NativeAuthError) {
this.serverTelemetryManager.setNativeBrokerErrorCode(
e.errorCode
);
}
throw e;
}
}

/**
Expand Down Expand Up @@ -307,8 +365,13 @@ export class NativeInteractionClient extends BaseInteractionClient {
this.validateNativeResponse(response);
} catch (e) {
// Only throw fatal errors here to allow application to fallback to regular redirect. Otherwise proceed and the error will be thrown in handleRedirectPromise
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know if we document that if an error here happens, the application needs to retry? It feels like something we should handle for them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We fall back to the web flow if the native flow throws. cc @tnorling

if (e instanceof NativeAuthError && isFatalNativeAuthError(e)) {
throw e;
if (e instanceof NativeAuthError) {
this.serverTelemetryManager.setNativeBrokerErrorCode(
e.errorCode
);
if (isFatalNativeAuthError(e)) {
throw e;
}
}
}
this.browserStorage.setTemporaryCache(
Expand Down Expand Up @@ -399,7 +462,9 @@ export class NativeInteractionClient extends BaseInteractionClient {
reqTimestamp
);
this.browserStorage.setInteractionInProgress(false);
return await result;
const res = await result;
this.serverTelemetryManager.clearNativeBrokerErrorCode();
return res;
} catch (e) {
this.browserStorage.setInteractionInProgress(false);
throw e;
Expand Down Expand Up @@ -980,6 +1045,7 @@ export class NativeInteractionClient extends BaseInteractionClient {
// SPAs require whole string to be passed to broker
validatedRequest.reqCnf = reqCnfData;
}
this.addRequestSKUs(validatedRequest);

return validatedRequest;
}
Expand Down
14 changes: 12 additions & 2 deletions lib/msal-browser/test/app/PublicClientApplication.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3855,14 +3855,24 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {
.callsFake(async () => {
return testTokenResponse;
});
const response = await pca.acquireTokenSilent({
const silentRequest = {
scopes: ["User.Read"],
account: testAccount,
});
};
const response = await pca.acquireTokenSilent(silentRequest);

expect(response).toEqual(testTokenResponse);
expect(nativeAcquireTokenSpy.calledOnce).toBeTruthy();
expect(silentSpy.calledOnce).toBeTruthy();
expect(
silentSpy.calledWith({
...silentRequest,
tokenQueryParameters: {
"x-client-current-telemetry":
"||broker_error=ContentError",
},
})
);
});

it("throws error if native broker call fails due to non-fatal error", async () => {
Expand Down
Loading
Loading