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

[identity] DeviceCodeCredential uses MSALClient #29405

Merged
merged 10 commits into from May 8, 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions sdk/identity/identity/CHANGELOG.md
Expand Up @@ -10,8 +10,12 @@

### Bug Fixes

- `ClientSecretCredential`, `ClientCertificateCredential`, and `ClientAssertionCredential` no longer try silent authentication unnecessarily as per the MSAL guidelines. For more information please refer to [the Entra documentation on token caching](https://learn.microsoft.com/entra/identity-platform/msal-acquire-cache-tokens#recommended-call-pattern-for-public-client-applications). [#29405](https://github.com/Azure/azure-sdk-for-js/pull/29405)

### Other Changes

- `DeviceCodeCredential` migrated to use MSALClient internally instead of MSALNode flow. This is an internal refactoring and should not result in any behavioral changes. [#29405](https://github.com/Azure/azure-sdk-for-js/pull/29405)

## 4.2.0 (2024-04-30)

### Features Added
Expand Down
2 changes: 1 addition & 1 deletion sdk/identity/identity/assets.json
Expand Up @@ -2,5 +2,5 @@
"AssetsRepo": "Azure/azure-sdk-assets",
"AssetsRepoPrefixPath": "js",
"TagPrefix": "js/identity/identity",
"Tag": "js/identity/identity_58e656fd32"
"Tag": "js/identity/identity_72abb85c88"
}
30 changes: 20 additions & 10 deletions sdk/identity/identity/src/credentials/deviceCodeCredential.ts
Expand Up @@ -5,14 +5,19 @@ import { AccessToken, GetTokenOptions, TokenCredential } from "@azure/core-auth"
import {
processMultiTenantRequest,
resolveAdditionallyAllowedTenantIds,
resolveTenantId,
} from "../util/tenantIdUtils";
import { DeviceCodeCredentialOptions, DeviceCodeInfo } from "./deviceCodeCredentialOptions";
import {
DeviceCodeCredentialOptions,
DeviceCodeInfo,
DeviceCodePromptCallback,
} from "./deviceCodeCredentialOptions";
import { AuthenticationRecord } from "../msal/types";
import { MsalDeviceCode } from "../msal/nodeFlows/msalDeviceCode";
import { MsalFlow } from "../msal/flows";
import { credentialLogger } from "../util/logging";
import { ensureScopes } from "../util/scopeUtils";
import { tracingClient } from "../util/tracing";
import { MsalClient, createMsalClient } from "../msal/nodeFlows/msalClient";
import { DeveloperSignOnClientId } from "../constants";

const logger = credentialLogger("DeviceCodeCredential");

Expand All @@ -31,8 +36,9 @@ export function defaultDeviceCodePromptCallback(deviceCodeInfo: DeviceCodeInfo):
export class DeviceCodeCredential implements TokenCredential {
private tenantId?: string;
private additionallyAllowedTenantIds: string[];
private msalFlow: MsalFlow;
private disableAutomaticAuthentication?: boolean;
private msalClient: MsalClient;
private userPromptCallback: DeviceCodePromptCallback;

/**
* Creates an instance of DeviceCodeCredential with the details needed
Expand All @@ -59,10 +65,11 @@ export class DeviceCodeCredential implements TokenCredential {
this.additionallyAllowedTenantIds = resolveAdditionallyAllowedTenantIds(
options?.additionallyAllowedTenants,
);
this.msalFlow = new MsalDeviceCode({
const clientId = options?.clientId ?? DeveloperSignOnClientId;
const tenantId = resolveTenantId(logger, options?.tenantId, clientId);
this.userPromptCallback = options?.userPromptCallback ?? defaultDeviceCodePromptCallback;
this.msalClient = createMsalClient(clientId, tenantId, {
...options,
logger,
userPromptCallback: options?.userPromptCallback || defaultDeviceCodePromptCallback,
tokenCredentialOptions: options || {},
});
this.disableAutomaticAuthentication = options?.disableAutomaticAuthentication;
Expand Down Expand Up @@ -93,7 +100,7 @@ export class DeviceCodeCredential implements TokenCredential {
);

const arrayScopes = ensureScopes(scopes);
return this.msalFlow.getToken(arrayScopes, {
return this.msalClient.getTokenByDeviceCode(arrayScopes, this.userPromptCallback, {
...newOptions,
disableAutomaticAuthentication: this.disableAutomaticAuthentication,
});
Expand All @@ -120,8 +127,11 @@ export class DeviceCodeCredential implements TokenCredential {
options,
async (newOptions) => {
const arrayScopes = Array.isArray(scopes) ? scopes : [scopes];
await this.msalFlow.getToken(arrayScopes, newOptions);
return this.msalFlow.getActiveAccount();
await this.msalClient.getTokenByDeviceCode(arrayScopes, this.userPromptCallback, {
...newOptions,
disableAutomaticAuthentication: false, // this method should always allow user interaction
Copy link
Member

Choose a reason for hiding this comment

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

I understand that we want to not attempt silent authentication, but the naming of this parameter confuses me since you'd think you'd want to disable automatic authentication rather than setting this to false which would enable it?

Copy link
Member Author

@maorleger maorleger May 3, 2024

Choose a reason for hiding this comment

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

Yeah, I agree that the naming is confusing. The reason I stuck with it is because it matches the same behavior and meaning as the public API's version of disableAutomaticAuthentication which can help when searching the codebase for this.

Personally I think a name like disableInteractiveAuthentication makes more sense, but I'm not sure if I want to stay consistent with the meaning of the public API field or do the name-mapping here

What this actually does, if I'm reading the source code correctly, is disable interactive auth - if an AuthenticationRequiredError is thrown from getTokenSilent and this option is true we will throw instead of continuing on to the interactive auth.

With that context in place - I don't have strong feelings and am happy to change it, but I have a slight preference to keeping the name and meaning consistent with the public API for better or worse. What do you think? @KarishmaGhiya would love your take as well!

Copy link
Contributor

Choose a reason for hiding this comment

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

@maorleger Is this param on L132 actually set by the public API param (L105) ? Or is this used for disabling interactive auth in another scenario (other than when passed in as a parameter by the user) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is used for enabling interactive auth fallback when the user passes is disableAutomaticAuthentication: true - so in the getToken call we will simply pass whatever the user passed in the public API param. In authenticate we ignore what the user passed in because authenticate is meant to be called by the user when getToken fails to silent auth, and disableAutomaticAuthentication is true.

Here's a few scenarios:

user sets disableAutomaticAuthentication deviceCodeCredential#getToken deviceCodeCredential#authenticate
undefined false false
false false false
true true false

Happy to discuss more in our sync!

});
return this.msalClient.getActiveAccount();
},
);
}
Expand Down
161 changes: 144 additions & 17 deletions sdk/identity/identity/src/msal/nodeFlows/msalClient.ts
Expand Up @@ -13,26 +13,44 @@ import {
getKnownAuthorities,
getMSALLogLevel,
handleMsalError,
msalToPublic,
publicToMsal,
} from "../utils";

import { AuthenticationRequiredError } from "../../errors";
import { CertificateParts } from "../types";
import { AuthenticationRecord, CertificateParts } from "../types";
import { IdentityClient } from "../../client/identityClient";
import { MsalNodeOptions } from "./msalNodeCommon";
import { calculateRegionalAuthority } from "../../regionalAuthority";
import { getLogLevel } from "@azure/logger";
import { resolveTenantId } from "../../util/tenantIdUtils";
import { DeviceCodePromptCallback } from "../../credentials/deviceCodeCredentialOptions";

/**
* The logger for all MsalClient instances.
*/
const msalLogger = credentialLogger("MsalClient");

export interface GetTokenWithSilentAuthOptions extends GetTokenOptions {
/**
* Disables automatic authentication. If set to true, the method will throw an error if the user needs to authenticate.
Copy link
Member

Choose a reason for hiding this comment

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

should this be called throwOnUserAuthenticationNeeded or something similar?

Copy link
Member Author

Choose a reason for hiding this comment

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

Heya @xirzec - Karishma and I chatted offline and we think that as backwards as this name is we will keep it throughout the codebase for consistency. At least it matches the meaning and behavior of the public API for better or worse. If you feel strongly, happy to make a follow-up PR with some renames! But merging for now

*
* @remarks
*
* This option will be set to `false` when the user calls `authenticate` directly on a credential that supports it.
*/
disableAutomaticAuthentication?: boolean;
}

/**
* Represents a client for interacting with the Microsoft Authentication Library (MSAL).
*/
export interface MsalClient {
getTokenByDeviceCode(
arrayScopes: string[],
userPromptCallback: DeviceCodePromptCallback,
options?: GetTokenWithSilentAuthOptions,
): Promise<AccessToken>;
/**
* Retrieves an access token by using a client certificate.
*
Expand Down Expand Up @@ -74,12 +92,21 @@ export interface MsalClient {
clientSecret: string,
options?: GetTokenOptions,
): Promise<AccessToken>;

/**
* Retrieves the last authenticated account. This method expects an authentication record to have been previously loaded.
*
* An authentication record could be loaded by calling the `getToken` method, or by providing an `authenticationRecord` when creating a credential.
*/
getActiveAccount(): AuthenticationRecord | undefined;
}

/**
* Options for creating an instance of the MsalClient.
*/
export type MsalClientOptions = Partial<Omit<MsalNodeOptions, "clientId" | "tenantId">>;
export type MsalClientOptions = Partial<
Copy link
Member Author

Choose a reason for hiding this comment

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

Slowly working on defining this type. It's probably fine for now, but I'll clean this up soonish

Omit<MsalNodeOptions, "clientId" | "tenantId" | "disableAutomaticAuthentication">
>;

/**
* Generates the configuration for MSAL (Microsoft Authentication Library).
Expand Down Expand Up @@ -173,6 +200,40 @@ export function createMsalClient(
pluginConfiguration: msalPlugins.generatePluginConfiguration(createMsalClientOptions),
};

const publicApps: Map<string, msal.PublicClientApplication> = new Map();
async function getPublicApp(
options: GetTokenOptions = {},
): Promise<msal.PublicClientApplication> {
const appKey = options.enableCae ? "CAE" : "default";

let publicClientApp = publicApps.get(appKey);
if (publicClientApp) {
msalLogger.getToken.info("Existing PublicClientApplication found in cache, returning it.");
return publicClientApp;
}

// Initialize a new app and cache it
msalLogger.getToken.info(
`Creating new PublicClientApplication with CAE ${options.enableCae ? "enabled" : "disabled"}.`,
);

const cachePlugin = options.enableCae
? state.pluginConfiguration.cache.cachePluginCae
: state.pluginConfiguration.cache.cachePlugin;

state.msalConfig.auth.clientCapabilities = options.enableCae ? ["cp1"] : undefined;

publicClientApp = new msal.PublicClientApplication({
...state.msalConfig,
broker: { nativeBrokerPlugin: state.pluginConfiguration.broker.nativeBrokerPlugin },
cache: { cachePlugin: await cachePlugin },
});

publicApps.set(appKey, publicClientApp);

return publicClientApp;
}

const confidentialApps: Map<string, msal.ConfidentialClientApplication> = new Map();
async function getConfidentialApp(
options: GetTokenOptions = {},
Expand Down Expand Up @@ -272,7 +333,7 @@ To work with multiple accounts for the same Client ID and Tenant ID, please prov
async function withSilentAuthentication(
msalApp: msal.ConfidentialClientApplication | msal.PublicClientApplication,
scopes: Array<string>,
options: GetTokenOptions,
options: GetTokenWithSilentAuthOptions,
onAuthenticationRequired: () => Promise<msal.AuthenticationResult | null>,
): Promise<AccessToken> {
let response: msal.AuthenticationResult | null = null;
Expand All @@ -282,7 +343,7 @@ To work with multiple accounts for the same Client ID and Tenant ID, please prov
if (e.name !== "AuthenticationRequiredError") {
throw e;
}
if (createMsalClientOptions.disableAutomaticAuthentication) {
if (options.disableAutomaticAuthentication) {
throw new AuthenticationRequiredError({
scopes,
getTokenOptions: options,
Expand Down Expand Up @@ -324,14 +385,24 @@ To work with multiple accounts for the same Client ID and Tenant ID, please prov

const msalApp = await getConfidentialApp(options);

return withSilentAuthentication(msalApp, scopes, options, () =>
msalApp.acquireTokenByClientCredential({
try {
const response = await msalApp.acquireTokenByClientCredential({
scopes,
authority: state.msalConfig.auth.authority,
azureRegion: calculateRegionalAuthority(),
claims: options?.claims,
}),
);
});
ensureValidMsalToken(scopes, response, options);

msalLogger.getToken.info(formatSuccess(scopes));

return {
token: response.accessToken,
expiresOnTimestamp: response.expiresOn.getTime(),
};
} catch (err: any) {
throw handleMsalError(scopes, err, options);
}
}

async function getTokenByClientAssertion(
Expand All @@ -345,15 +416,25 @@ To work with multiple accounts for the same Client ID and Tenant ID, please prov

const msalApp = await getConfidentialApp(options);

return withSilentAuthentication(msalApp, scopes, options, () =>
msalApp.acquireTokenByClientCredential({
try {
const response = await msalApp.acquireTokenByClientCredential({
scopes,
authority: state.msalConfig.auth.authority,
azureRegion: calculateRegionalAuthority(),
claims: options?.claims,
clientAssertion,
}),
);
});
ensureValidMsalToken(scopes, response, options);

msalLogger.getToken.info(formatSuccess(scopes));

return {
token: response.accessToken,
expiresOnTimestamp: response.expiresOn.getTime(),
};
} catch (err: any) {
throw handleMsalError(scopes, err, options);
}
}

async function getTokenByClientCertificate(
Expand All @@ -366,20 +447,66 @@ To work with multiple accounts for the same Client ID and Tenant ID, please prov
state.msalConfig.auth.clientCertificate = certificate;

const msalApp = await getConfidentialApp(options);

return withSilentAuthentication(msalApp, scopes, options, () =>
msalApp.acquireTokenByClientCredential({
try {
const response = await msalApp.acquireTokenByClientCredential({
scopes,
authority: state.msalConfig.auth.authority,
azureRegion: calculateRegionalAuthority(),
claims: options?.claims,
});
ensureValidMsalToken(scopes, response, options);

msalLogger.getToken.info(formatSuccess(scopes));

return {
token: response.accessToken,
expiresOnTimestamp: response.expiresOn.getTime(),
};
} catch (err: any) {
throw handleMsalError(scopes, err, options);
}
}

async function getTokenByDeviceCode(
scopes: string[],
deviceCodeCallback: DeviceCodePromptCallback,
options: GetTokenWithSilentAuthOptions = {},
): Promise<AccessToken> {
msalLogger.getToken.info(`Attempting to acquire token using device code`);

const msalApp = await getPublicApp(options);

return withSilentAuthentication(msalApp, scopes, options, () => {
const requestOptions: msal.DeviceCodeRequest = {
scopes,
cancel: options?.abortSignal?.aborted ?? false,
deviceCodeCallback,
authority: state.msalConfig.auth.authority,
claims: options?.claims,
}),
);
};
const deviceCodeRequest = msalApp.acquireTokenByDeviceCode(requestOptions);
if (options.abortSignal) {
options.abortSignal.addEventListener("abort", () => {
maorleger marked this conversation as resolved.
Show resolved Hide resolved
requestOptions.cancel = true;
});
}

return deviceCodeRequest;
});
}

function getActiveAccount(): AuthenticationRecord | undefined {
if (!state.cachedAccount) {
return undefined;
}
return msalToPublic(clientId, state.cachedAccount);
}

return {
getActiveAccount,
getTokenByClientSecret,
getTokenByClientAssertion,
getTokenByClientCertificate,
getTokenByDeviceCode,
};
}