From 8e78789cbe86109b66f0acee1856b8453450611b Mon Sep 17 00:00:00 2001 From: Robbie Ginsburg Date: Thu, 2 Jan 2025 13:04:19 -0500 Subject: [PATCH 1/3] fixed bug --- .../client/ManagedIdentitySources/BaseManagedIdentitySource.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/msal-node/src/client/ManagedIdentitySources/BaseManagedIdentitySource.ts b/lib/msal-node/src/client/ManagedIdentitySources/BaseManagedIdentitySource.ts index 39d7a3e968..fa7bdd7e85 100644 --- a/lib/msal-node/src/client/ManagedIdentitySources/BaseManagedIdentitySource.ts +++ b/lib/msal-node/src/client/ManagedIdentitySources/BaseManagedIdentitySource.ts @@ -38,7 +38,7 @@ import { export const ManagedIdentityUserAssignedIdQueryParameterNames = { MANAGED_IDENTITY_CLIENT_ID: "client_id", MANAGED_IDENTITY_OBJECT_ID: "object_id", - MANAGED_IDENTITY_RESOURCE_ID: "mi_res_id", + MANAGED_IDENTITY_RESOURCE_ID: "msi_res_id", } as const; export type ManagedIdentityUserAssignedIdQueryParameterNames = (typeof ManagedIdentityUserAssignedIdQueryParameterNames)[keyof typeof ManagedIdentityUserAssignedIdQueryParameterNames]; From 7a3055fab7edf0d6b8bd0e3fad3388cc8e0fb18f Mon Sep 17 00:00:00 2001 From: Robbie Ginsburg Date: Thu, 2 Jan 2025 13:07:49 -0500 Subject: [PATCH 2/3] Change files --- ...ure-msal-node-643086d5-3524-43f3-adf2-97b752020dda.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/@azure-msal-node-643086d5-3524-43f3-adf2-97b752020dda.json diff --git a/change/@azure-msal-node-643086d5-3524-43f3-adf2-97b752020dda.json b/change/@azure-msal-node-643086d5-3524-43f3-adf2-97b752020dda.json new file mode 100644 index 0000000000..e09741a668 --- /dev/null +++ b/change/@azure-msal-node-643086d5-3524-43f3-adf2-97b752020dda.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "Fixed incorrect IMDS resource ID query parameter", + "packageName": "@azure/msal-node", + "email": "rginsburg@microsoft.com", + "dependentChangeType": "patch" +} From b54a602d5473a91f8bec8771811c8c06c3fb2b3f Mon Sep 17 00:00:00 2001 From: Robbie Ginsburg Date: Tue, 14 Jan 2025 13:16:02 -0500 Subject: [PATCH 3/3] Implemented GitHub Feedback --- .../BaseManagedIdentitySource.ts | 10 ++- .../src/client/ManagedIdentitySources/Imds.ts | 3 +- .../ManagedIdentitySources/AppService.spec.ts | 67 +++++++++++++++---- .../ManagedIdentitySources/Imds.spec.ts | 35 +++++++--- .../ServiceFabric.spec.ts | 66 ++++++++++++++---- .../test/test_kit/ManagedIdentityTestUtils.ts | 9 +++ 6 files changed, 151 insertions(+), 39 deletions(-) diff --git a/lib/msal-node/src/client/ManagedIdentitySources/BaseManagedIdentitySource.ts b/lib/msal-node/src/client/ManagedIdentitySources/BaseManagedIdentitySource.ts index fa7bdd7e85..0af1111d44 100644 --- a/lib/msal-node/src/client/ManagedIdentitySources/BaseManagedIdentitySource.ts +++ b/lib/msal-node/src/client/ManagedIdentitySources/BaseManagedIdentitySource.ts @@ -38,7 +38,8 @@ import { export const ManagedIdentityUserAssignedIdQueryParameterNames = { MANAGED_IDENTITY_CLIENT_ID: "client_id", MANAGED_IDENTITY_OBJECT_ID: "object_id", - MANAGED_IDENTITY_RESOURCE_ID: "msi_res_id", + MANAGED_IDENTITY_RESOURCE_ID_IMDS: "msi_res_id", + MANAGED_IDENTITY_RESOURCE_ID_NON_IMDS: "mi_res_id", } as const; export type ManagedIdentityUserAssignedIdQueryParameterNames = (typeof ManagedIdentityUserAssignedIdQueryParameterNames)[keyof typeof ManagedIdentityUserAssignedIdQueryParameterNames]; @@ -201,7 +202,8 @@ export abstract class BaseManagedIdentitySource { } public getManagedIdentityUserAssignedIdQueryParameterKey( - managedIdentityIdType: ManagedIdentityIdType + managedIdentityIdType: ManagedIdentityIdType, + imds?: boolean ): string { switch (managedIdentityIdType) { case ManagedIdentityIdType.USER_ASSIGNED_CLIENT_ID: @@ -214,7 +216,9 @@ export abstract class BaseManagedIdentitySource { this.logger.info( "[Managed Identity] Adding user assigned resource id to the request." ); - return ManagedIdentityUserAssignedIdQueryParameterNames.MANAGED_IDENTITY_RESOURCE_ID; + return imds + ? ManagedIdentityUserAssignedIdQueryParameterNames.MANAGED_IDENTITY_RESOURCE_ID_IMDS + : ManagedIdentityUserAssignedIdQueryParameterNames.MANAGED_IDENTITY_RESOURCE_ID_NON_IMDS; case ManagedIdentityIdType.USER_ASSIGNED_OBJECT_ID: this.logger.info( diff --git a/lib/msal-node/src/client/ManagedIdentitySources/Imds.ts b/lib/msal-node/src/client/ManagedIdentitySources/Imds.ts index 0559ea7cc9..aaaf0b7c58 100644 --- a/lib/msal-node/src/client/ManagedIdentitySources/Imds.ts +++ b/lib/msal-node/src/client/ManagedIdentitySources/Imds.ts @@ -114,7 +114,8 @@ export class Imds extends BaseManagedIdentitySource { ) { request.queryParameters[ this.getManagedIdentityUserAssignedIdQueryParameterKey( - managedIdentityId.idType + managedIdentityId.idType, + true // indicates source is IMDS ) ] = managedIdentityId.id; } diff --git a/lib/msal-node/test/client/ManagedIdentitySources/AppService.spec.ts b/lib/msal-node/test/client/ManagedIdentitySources/AppService.spec.ts index 6fed804be6..dd40e607d0 100644 --- a/lib/msal-node/test/client/ManagedIdentitySources/AppService.spec.ts +++ b/lib/msal-node/test/client/ManagedIdentitySources/AppService.spec.ts @@ -9,6 +9,7 @@ import { DEFAULT_USER_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT, MANAGED_IDENTITY_APP_SERVICE_NETWORK_REQUEST_400_ERROR, MANAGED_IDENTITY_RESOURCE, + MANAGED_IDENTITY_RESOURCE_ID, } from "../../test_kit/StringConstants.js"; import { @@ -17,6 +18,7 @@ import { systemAssignedConfig, networkClient, ManagedIdentityNetworkErrorClient, + userAssignedResourceIdConfig, } from "../../test_kit/ManagedIdentityTestUtils.js"; import { AuthenticationResult, @@ -28,6 +30,7 @@ import { ManagedIdentityEnvironmentVariableNames, ManagedIdentitySourceNames, } from "../../../src/utils/Constants.js"; +import { ManagedIdentityUserAssignedIdQueryParameterNames } from "../../../src/client/ManagedIdentitySources/BaseManagedIdentitySource.js"; describe("Acquires a token successfully via an App Service Managed Identity", () => { beforeAll(() => { @@ -52,21 +55,61 @@ describe("Acquires a token successfully via an App Service Managed Identity", () delete ManagedIdentityApplication["nodeStorage"]; }); - test("acquires a User Assigned Client Id token", async () => { - const managedIdentityApplication: ManagedIdentityApplication = - new ManagedIdentityApplication(userAssignedClientIdConfig); - expect(managedIdentityApplication.getManagedIdentitySource()).toBe( - ManagedIdentitySourceNames.APP_SERVICE - ); + describe("User Assigned", () => { + test("acquires a User Assigned Client Id token", async () => { + const managedIdentityApplication: ManagedIdentityApplication = + new ManagedIdentityApplication(userAssignedClientIdConfig); + expect(managedIdentityApplication.getManagedIdentitySource()).toBe( + ManagedIdentitySourceNames.APP_SERVICE + ); + + const networkManagedIdentityResult: AuthenticationResult = + await managedIdentityApplication.acquireToken( + managedIdentityRequestParams + ); - const networkManagedIdentityResult: AuthenticationResult = - await managedIdentityApplication.acquireToken( - managedIdentityRequestParams + expect(networkManagedIdentityResult.accessToken).toEqual( + DEFAULT_USER_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken + ); + }); + + test("acquires a User Assigned Resource Id token", async () => { + const sendGetRequestAsyncSpy: jest.SpyInstance = jest.spyOn( + networkClient, + "sendGetRequestAsync" + ); + + const managedIdentityApplication: ManagedIdentityApplication = + new ManagedIdentityApplication(userAssignedResourceIdConfig); + expect(managedIdentityApplication.getManagedIdentitySource()).toBe( + ManagedIdentitySourceNames.APP_SERVICE ); - expect(networkManagedIdentityResult.accessToken).toEqual( - DEFAULT_USER_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken - ); + const networkManagedIdentityResult: AuthenticationResult = + await managedIdentityApplication.acquireToken( + managedIdentityRequestParams + ); + + expect(networkManagedIdentityResult.accessToken).toEqual( + DEFAULT_USER_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken + ); + + const url: URLSearchParams = new URLSearchParams( + sendGetRequestAsyncSpy.mock.lastCall[0] + ); + expect( + url.has( + ManagedIdentityUserAssignedIdQueryParameterNames.MANAGED_IDENTITY_RESOURCE_ID_NON_IMDS + ) + ).toBe(true); + expect( + url.get( + ManagedIdentityUserAssignedIdQueryParameterNames.MANAGED_IDENTITY_RESOURCE_ID_NON_IMDS + ) + ).toEqual(MANAGED_IDENTITY_RESOURCE_ID); + + jest.restoreAllMocks(); + }); }); describe("System Assigned", () => { diff --git a/lib/msal-node/test/client/ManagedIdentitySources/Imds.spec.ts b/lib/msal-node/test/client/ManagedIdentitySources/Imds.spec.ts index 5f26efc9cc..b309ff4b14 100644 --- a/lib/msal-node/test/client/ManagedIdentitySources/Imds.spec.ts +++ b/lib/msal-node/test/client/ManagedIdentitySources/Imds.spec.ts @@ -18,7 +18,6 @@ import { THREE_SECONDS_IN_MILLI, getCacheKey, } from "../../test_kit/StringConstants.js"; - import { ManagedIdentityNetworkClient, ManagedIdentityNetworkErrorClient, @@ -26,6 +25,7 @@ import { userAssignedClientIdConfig, managedIdentityRequestParams, systemAssignedConfig, + userAssignedResourceIdConfig, } from "../../test_kit/ManagedIdentityTestUtils.js"; import { DEFAULT_MANAGED_IDENTITY_ID, @@ -53,8 +53,8 @@ import { ClientCredentialClient, NodeStorage, } from "../../../src/index.js"; -// NodeJS 16+ provides a built-in version of setTimeout that is promise-based -import { setTimeout } from "timers/promises"; +import { setTimeout } from "timers/promises"; // NodeJS 16+ provides a built-in version of setTimeout that is promise-based +import { ManagedIdentityUserAssignedIdQueryParameterNames } from "../../../src/client/ManagedIdentitySources/BaseManagedIdentitySource.js"; describe("Acquires a token successfully via an IMDS Managed Identity", () => { // IMDS doesn't need environment variables because there is a default IMDS endpoint @@ -81,14 +81,6 @@ describe("Acquires a token successfully via an IMDS Managed Identity", () => { userAssignedObjectId: MANAGED_IDENTITY_RESOURCE_ID, }, }; - const userAssignedResourceIdConfig: ManagedIdentityConfiguration = { - system: { - networkClient, - }, - managedIdentityIdParams: { - userAssignedResourceId: MANAGED_IDENTITY_RESOURCE_ID, - }, - }; describe("User Assigned", () => { test("acquires a User Assigned Client Id token", async () => { @@ -126,6 +118,11 @@ describe("Acquires a token successfully via an IMDS Managed Identity", () => { }); test("acquires a User Assigned Resource Id token", async () => { + const sendGetRequestAsyncSpy: jest.SpyInstance = jest.spyOn( + networkClient, + "sendGetRequestAsync" + ); + const managedIdentityApplication: ManagedIdentityApplication = new ManagedIdentityApplication(userAssignedResourceIdConfig); expect(managedIdentityApplication.getManagedIdentitySource()).toBe( @@ -140,6 +137,22 @@ describe("Acquires a token successfully via an IMDS Managed Identity", () => { expect(networkManagedIdentityResult.accessToken).toEqual( DEFAULT_USER_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken ); + + const url: URLSearchParams = new URLSearchParams( + sendGetRequestAsyncSpy.mock.lastCall[0] + ); + expect( + url.has( + ManagedIdentityUserAssignedIdQueryParameterNames.MANAGED_IDENTITY_RESOURCE_ID_IMDS + ) + ).toBe(true); + expect( + url.get( + ManagedIdentityUserAssignedIdQueryParameterNames.MANAGED_IDENTITY_RESOURCE_ID_IMDS + ) + ).toEqual(MANAGED_IDENTITY_RESOURCE_ID); + + jest.restoreAllMocks(); }); }); diff --git a/lib/msal-node/test/client/ManagedIdentitySources/ServiceFabric.spec.ts b/lib/msal-node/test/client/ManagedIdentitySources/ServiceFabric.spec.ts index aa3fee5d71..9830434fe8 100644 --- a/lib/msal-node/test/client/ManagedIdentitySources/ServiceFabric.spec.ts +++ b/lib/msal-node/test/client/ManagedIdentitySources/ServiceFabric.spec.ts @@ -8,6 +8,7 @@ import { DEFAULT_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT, DEFAULT_USER_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT, MANAGED_IDENTITY_RESOURCE, + MANAGED_IDENTITY_RESOURCE_ID, MANAGED_IDENTITY_SERVICE_FABRIC_NETWORK_REQUEST_400_ERROR, } from "../../test_kit/StringConstants.js"; @@ -17,6 +18,7 @@ import { systemAssignedConfig, ManagedIdentityNetworkErrorClient, networkClient, + userAssignedResourceIdConfig, } from "../../test_kit/ManagedIdentityTestUtils.js"; import { AuthenticationResult, @@ -28,6 +30,7 @@ import { ManagedIdentityEnvironmentVariableNames, ManagedIdentitySourceNames, } from "../../../src/utils/Constants.js"; +import { ManagedIdentityUserAssignedIdQueryParameterNames } from "../../../src/client/ManagedIdentitySources/BaseManagedIdentitySource.js"; describe("Acquires a token successfully via an App Service Managed Identity", () => { beforeAll(() => { @@ -58,21 +61,60 @@ describe("Acquires a token successfully via an App Service Managed Identity", () delete ManagedIdentityApplication["nodeStorage"]; }); - test("acquires a User Assigned Client Id token", async () => { - const managedIdentityApplication: ManagedIdentityApplication = - new ManagedIdentityApplication(userAssignedClientIdConfig); - expect(managedIdentityApplication.getManagedIdentitySource()).toBe( - ManagedIdentitySourceNames.SERVICE_FABRIC - ); + describe("User Assigned", () => { + test("acquires a User Assigned Client Id token", async () => { + const managedIdentityApplication: ManagedIdentityApplication = + new ManagedIdentityApplication(userAssignedClientIdConfig); + expect(managedIdentityApplication.getManagedIdentitySource()).toBe( + ManagedIdentitySourceNames.SERVICE_FABRIC + ); + + const networkManagedIdentityResult: AuthenticationResult = + await managedIdentityApplication.acquireToken( + managedIdentityRequestParams + ); + + expect(networkManagedIdentityResult.accessToken).toEqual( + DEFAULT_USER_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken + ); + }); + test("acquires a User Assigned Resource Id token", async () => { + const sendGetRequestAsyncSpy: jest.SpyInstance = jest.spyOn( + networkClient, + "sendGetRequestAsync" + ); + + const managedIdentityApplication: ManagedIdentityApplication = + new ManagedIdentityApplication(userAssignedResourceIdConfig); + expect(managedIdentityApplication.getManagedIdentitySource()).toBe( + ManagedIdentitySourceNames.SERVICE_FABRIC + ); - const networkManagedIdentityResult: AuthenticationResult = - await managedIdentityApplication.acquireToken( - managedIdentityRequestParams + const networkManagedIdentityResult: AuthenticationResult = + await managedIdentityApplication.acquireToken( + managedIdentityRequestParams + ); + + expect(networkManagedIdentityResult.accessToken).toEqual( + DEFAULT_USER_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken ); - expect(networkManagedIdentityResult.accessToken).toEqual( - DEFAULT_USER_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken - ); + const url: URLSearchParams = new URLSearchParams( + sendGetRequestAsyncSpy.mock.lastCall[0] + ); + expect( + url.has( + ManagedIdentityUserAssignedIdQueryParameterNames.MANAGED_IDENTITY_RESOURCE_ID_NON_IMDS + ) + ).toBe(true); + expect( + url.get( + ManagedIdentityUserAssignedIdQueryParameterNames.MANAGED_IDENTITY_RESOURCE_ID_NON_IMDS + ) + ).toEqual(MANAGED_IDENTITY_RESOURCE_ID); + + jest.restoreAllMocks(); + }); }); describe("System Assigned", () => { diff --git a/lib/msal-node/test/test_kit/ManagedIdentityTestUtils.ts b/lib/msal-node/test/test_kit/ManagedIdentityTestUtils.ts index 4e181c0440..9e03032074 100644 --- a/lib/msal-node/test/test_kit/ManagedIdentityTestUtils.ts +++ b/lib/msal-node/test/test_kit/ManagedIdentityTestUtils.ts @@ -131,6 +131,15 @@ export const userAssignedClientIdConfig: ManagedIdentityConfiguration = { }, }; +export const userAssignedResourceIdConfig: ManagedIdentityConfiguration = { + system: { + networkClient, + }, + managedIdentityIdParams: { + userAssignedResourceId: MANAGED_IDENTITY_RESOURCE_ID, + }, +}; + export const systemAssignedConfig: ManagedIdentityConfiguration = { system: { networkClient,