diff --git a/sdk/identity/identity/CHANGELOG.md b/sdk/identity/identity/CHANGELOG.md index ab93d291eb46..d732cb21a761 100644 --- a/sdk/identity/identity/CHANGELOG.md +++ b/sdk/identity/identity/CHANGELOG.md @@ -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 diff --git a/sdk/identity/identity/assets.json b/sdk/identity/identity/assets.json index f98670e6069b..27940d612b4c 100644 --- a/sdk/identity/identity/assets.json +++ b/sdk/identity/identity/assets.json @@ -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" } diff --git a/sdk/identity/identity/src/credentials/deviceCodeCredential.ts b/sdk/identity/identity/src/credentials/deviceCodeCredential.ts index 5210728073e0..7f9b0fa92b3c 100644 --- a/sdk/identity/identity/src/credentials/deviceCodeCredential.ts +++ b/sdk/identity/identity/src/credentials/deviceCodeCredential.ts @@ -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"); @@ -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 @@ -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; @@ -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, }); @@ -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 + }); + return this.msalClient.getActiveAccount(); }, ); } diff --git a/sdk/identity/identity/src/msal/nodeFlows/msalClient.ts b/sdk/identity/identity/src/msal/nodeFlows/msalClient.ts index dc1a103694c9..d0a70a4b906e 100644 --- a/sdk/identity/identity/src/msal/nodeFlows/msalClient.ts +++ b/sdk/identity/identity/src/msal/nodeFlows/msalClient.ts @@ -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. + * + * @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; /** * Retrieves an access token by using a client certificate. * @@ -74,12 +92,21 @@ export interface MsalClient { clientSecret: string, options?: GetTokenOptions, ): Promise; + + /** + * 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>; +export type MsalClientOptions = Partial< + Omit +>; /** * Generates the configuration for MSAL (Microsoft Authentication Library). @@ -173,6 +200,40 @@ export function createMsalClient( pluginConfiguration: msalPlugins.generatePluginConfiguration(createMsalClientOptions), }; + const publicApps: Map = new Map(); + async function getPublicApp( + options: GetTokenOptions = {}, + ): Promise { + 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 = new Map(); async function getConfidentialApp( options: GetTokenOptions = {}, @@ -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, - options: GetTokenOptions, + options: GetTokenWithSilentAuthOptions, onAuthenticationRequired: () => Promise, ): Promise { let response: msal.AuthenticationResult | null = null; @@ -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, @@ -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( @@ -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( @@ -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 { + 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", () => { + 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, }; } diff --git a/sdk/identity/identity/test/internal/node/deviceCodeCredential.spec.ts b/sdk/identity/identity/test/internal/node/deviceCodeCredential.spec.ts index 550b89147368..cfa7fa55bc48 100644 --- a/sdk/identity/identity/test/internal/node/deviceCodeCredential.spec.ts +++ b/sdk/identity/identity/test/internal/node/deviceCodeCredential.spec.ts @@ -7,8 +7,6 @@ import { MsalTestCleanup, msalNodeTestSetup } from "../../node/msalNodeTestSetup import { Recorder, env, isLiveMode } from "@azure-tools/test-recorder"; import { Context } from "mocha"; import { DeviceCodeCredential } from "../../../src"; -import { GetTokenOptions } from "@azure/core-auth"; -import { MsalNode } from "../../../src/msal/nodeFlows/msalNodeCommon"; import { PublicClientApplication } from "@azure/msal-node"; import Sinon from "sinon"; import { assert } from "chai"; @@ -24,9 +22,10 @@ describe("DeviceCodeCredential (internal)", function () { cleanup = setup.cleanup; recorder = setup.recorder; - getTokenSilentSpy = setup.sandbox.spy(MsalNode.prototype, "getTokenSilent"); + // MsalClient calls to this method underneath when silent authentication can be attempted. + getTokenSilentSpy = setup.sandbox.spy(PublicClientApplication.prototype, "acquireTokenSilent"); - // MsalClientSecret calls to this method underneath. + // MsalClient calls to this method underneath for interactive auth. doGetTokenSpy = setup.sandbox.spy( PublicClientApplication.prototype, "acquireTokenByDeviceCode", @@ -51,19 +50,18 @@ describe("DeviceCodeCredential (internal)", function () { ); await credential.getToken(scope); - assert.equal(getTokenSilentSpy.callCount, 1, "getTokenSilentSpy.callCount should have been 1"); - assert.equal(doGetTokenSpy.callCount, 1, "doGetTokenSpy.callCount should have been 1"); + assert.equal(doGetTokenSpy.callCount, 1, "doGetTokenSpy.callCount should have been 1."); await credential.getToken(scope); assert.equal( getTokenSilentSpy.callCount, - 2, - "getTokenSilentSpy.callCount should have been 2 (2nd time)", + 1, + "getTokenSilentSpy.callCount should have been 1 (Silent authentication after the initial request).", ); assert.equal( doGetTokenSpy.callCount, 1, - "doGetTokenSpy.callCount should have been 1 (2nd time)", + "Expected no additional calls to doGetTokenSpy after the initial request.", ); }); @@ -79,8 +77,7 @@ describe("DeviceCodeCredential (internal)", function () { }), ); - await credential.getToken(scope, { tenantId: env.AZURE_TENANT_ID } as GetTokenOptions); - assert.equal(getTokenSilentSpy.callCount, 1, "getTokenSilentSpy.callCount should have been 1"); + await credential.getToken(scope, { tenantId: env.AZURE_TENANT_ID }); assert.equal(doGetTokenSpy.callCount, 1, "doGetTokenSpy.callCount should have been 1"); }); }); diff --git a/sdk/identity/identity/test/internal/node/msalClient.spec.ts b/sdk/identity/identity/test/internal/node/msalClient.spec.ts index 6435d9c4f83f..ad28fd3b2913 100644 --- a/sdk/identity/identity/test/internal/node/msalClient.spec.ts +++ b/sdk/identity/identity/test/internal/node/msalClient.spec.ts @@ -3,9 +3,14 @@ import * as msalClient from "../../../src/msal/nodeFlows/msalClient"; -import { AuthenticationResult, ConfidentialClientApplication } from "@azure/msal-node"; +import { + AuthenticationResult, + ClientApplication, + ConfidentialClientApplication, + PublicClientApplication, +} from "@azure/msal-node"; import { MsalTestCleanup, msalNodeTestSetup } from "../../node/msalNodeTestSetup"; -import { Recorder, env } from "@azure-tools/test-recorder"; +import { Recorder, env, isLiveMode } from "@azure-tools/test-recorder"; import { AbortError } from "@azure/abort-controller"; import { AuthenticationRequiredError } from "../../../src/errors"; @@ -14,6 +19,8 @@ import { assert } from "@azure-tools/test-utils"; import { credentialLogger } from "../../../src/util/logging"; import { msalPlugins } from "../../../src/msal/nodeFlows/msalPlugins"; import sinon from "sinon"; +import { DeveloperSignOnClientId } from "../../../src/constants"; +import { Context } from "mocha"; describe("MsalClient", function () { describe("recorded tests", function () { @@ -43,6 +50,29 @@ describe("MsalClient", function () { assert.isNotEmpty(accessToken.token); assert.isNotNaN(accessToken.expiresOnTimestamp); }); + + it("supports getTokenByDeviceCode", async function (this: Context) { + if (isLiveMode()) { + // Skip in CI live tests since this credential requires user interaction. + this.skip(); + } + const scopes = ["https://vault.azure.net/.default"]; + const clientId = DeveloperSignOnClientId; + const tenantId = env.IDENTITY_SP_TENANT_ID || env.AZURE_TENANT_ID!; + + const clientOptions = recorder.configureClientOptions({}); + const client = msalClient.createMsalClient(clientId, tenantId, { + tokenCredentialOptions: { additionalPolicies: clientOptions.additionalPolicies }, + }); + + const accessToken = await client.getTokenByDeviceCode(scopes, (info) => { + console.log( + `To complete the test recording, please go to ${info.verificationUri} and use code ${info.userCode} to authenticate.`, + ); + }); + assert.isNotEmpty(accessToken.token); + assert.isNotNaN(accessToken.expiresOnTimestamp); + }); }); describe("#createMsalClient", function () { @@ -219,11 +249,14 @@ describe("MsalClient", function () { }); }); - describe("#getTokenByClientSecret", function () { + describe("#getTokenByDeviceCode", function () { let sandbox: sinon.SinonSandbox; const clientId = "client-id"; const tenantId = "tenant-id"; + const deviceCodeCallback: () => void = () => { + // no-op + }; afterEach(async function () { sandbox.restore(); @@ -248,16 +281,15 @@ describe("MsalClient", function () { }); const silentAuthSpy = sandbox - .stub(ConfidentialClientApplication.prototype, "acquireTokenSilent") + .stub(ClientApplication.prototype, "acquireTokenSilent") .resolves({ accessToken: "token", expiresOn: new Date(), } as AuthenticationResult); const scopes = ["https://vault.azure.net/.default"]; - const clientSecret = process.env.AZURE_CLIENT_SECRET!; - await client.getTokenByClientSecret(scopes, clientSecret); + await client.getTokenByDeviceCode(scopes, deviceCodeCallback); assert.equal(silentAuthSpy.callCount, 1); assert.deepEqual(silentAuthSpy.firstCall.firstArg.account, { @@ -269,14 +301,14 @@ describe("MsalClient", function () { it("attempts silent authentication without AuthenticationRecord", async function () { const silentAuthStub = sandbox - .stub(ConfidentialClientApplication.prototype, "acquireTokenSilent") + .stub(ClientApplication.prototype, "acquireTokenSilent") .resolves({ accessToken: "token", expiresOn: new Date(), } as AuthenticationResult); const clientCredentialAuthStub = sandbox - .stub(ConfidentialClientApplication.prototype, "acquireTokenByClientCredential") + .stub(PublicClientApplication.prototype, "acquireTokenByDeviceCode") .resolves({ accessToken: "token", expiresOn: new Date(Date.now() + 3600 * 1000), @@ -290,12 +322,11 @@ describe("MsalClient", function () { } as AuthenticationResult); const scopes = ["https://vault.azure.net/.default"]; - const clientSecret = process.env.AZURE_CLIENT_SECRET!; const client = msalClient.createMsalClient(clientId, tenantId); - await client.getTokenByClientSecret(scopes, clientSecret); - await client.getTokenByClientSecret(scopes, clientSecret); + await client.getTokenByDeviceCode(scopes, deviceCodeCallback); + await client.getTokenByDeviceCode(scopes, deviceCodeCallback); assert.equal( clientCredentialAuthStub.callCount, @@ -322,21 +353,24 @@ describe("MsalClient", function () { }); sandbox - .stub(ConfidentialClientApplication.prototype, "acquireTokenSilent") + .stub(ClientApplication.prototype, "acquireTokenSilent") .rejects(new AbortError("operation has been aborted")); // AbortErrors should get re-thrown const scopes = ["https://vault.azure.net/.default"]; - const clientSecret = process.env.AZURE_CLIENT_SECRET!; await assert.isRejected( - client.getTokenByClientSecret(scopes, clientSecret), + client.getTokenByDeviceCode(scopes, deviceCodeCallback), "operation has been aborted", ); }); it("throws when silentAuthentication fails and disableAutomaticAuthentication is true", async function () { + const scopes = ["https://vault.azure.net/.default"]; + sandbox + .stub(ClientApplication.prototype, "acquireTokenSilent") + .rejects(new AuthenticationRequiredError({ scopes })); + const client = msalClient.createMsalClient(clientId, tenantId, { - disableAutomaticAuthentication: true, // An authentication record will get us to try the silent flow authenticationRecord: { authority: "https://login.microsoftonline.com/tenant-id", @@ -347,18 +381,35 @@ describe("MsalClient", function () { }, }); - const scopes = ["https://vault.azure.net/.default"]; - sandbox - .stub(ConfidentialClientApplication.prototype, "acquireTokenSilent") - .rejects(new AuthenticationRequiredError({ scopes })); - - const clientSecret = process.env.AZURE_CLIENT_SECRET!; - await assert.isRejected( - client.getTokenByClientSecret(scopes, clientSecret), + client.getTokenByDeviceCode( + scopes, + () => { + // no-op + }, + { disableAutomaticAuthentication: true }, + ), /Automatic authentication has been disabled/, ); }); }); + + it("supports cancellation", async function (this: Context) { + const client = msalClient.createMsalClient(clientId, tenantId); + + const scopes = ["https://vault.azure.net/.default"]; + // we expect the request to be aborted immediately without trying to reach the network + const abortSignal = AbortSignal.abort(); + const request = client.getTokenByDeviceCode( + scopes, + () => { + // no-op + }, + { + abortSignal, + }, + ); + await assert.isRejected(request, AbortError); + }); }); });