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

Added token revocation functionality to Managed Identity #7422

Open
wants to merge 17 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 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": "Added token revocation functionality to managed identity (#7422)",
"packageName": "@azure/msal-node",
"email": "[email protected]",
"dependentChangeType": "patch"
}
1 change: 1 addition & 0 deletions lib/msal-node/apiReview/msal-node.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,7 @@ export class ManagedIdentityApplication {

// @public (undocumented)
export type ManagedIdentityConfiguration = {
clientCapabilities?: Array<string>;
managedIdentityIdParams?: ManagedIdentityIdParams;
system?: NodeSystemOptions;
};
Expand Down
4 changes: 4 additions & 0 deletions lib/msal-node/src/client/ManagedIdentityApplication.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,10 @@ export class ManagedIdentityApplication {
],
authority: this.fakeAuthority.canonicalAuthority,
correlationId: this.cryptoProvider.createNewGuid(),
claims: managedIdentityRequestParams.claims,
clientCapabilities:
Robbie-Microsoft marked this conversation as resolved.
Show resolved Hide resolved
managedIdentityRequestParams.clientCapabilities ||
this.config.clientCapabilities,
};

if (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,17 @@ export abstract class BaseManagedIdentitySource {
managedIdentityId
);

// if claims are present, ESTS will get a new token
Robbie-Microsoft marked this conversation as resolved.
Show resolved Hide resolved
if (managedIdentityRequest.claims) {
Robbie-Microsoft marked this conversation as resolved.
Show resolved Hide resolved
Robbie-Microsoft marked this conversation as resolved.
Show resolved Hide resolved
networkRequest.queryParameters.bypass_cache = "true";
}

// if client capabilities are present, send them to ESTS
if (managedIdentityRequest.clientCapabilities?.length) {
networkRequest.queryParameters.xms_cc =
managedIdentityRequest.clientCapabilities.toString();
}

const headers: Record<string, string> = networkRequest.headers;
headers[HeaderNames.CONTENT_TYPE] = Constants.URL_FORM_CONTENT_TYPE;

Expand Down
4 changes: 4 additions & 0 deletions lib/msal-node/src/config/Configuration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ export type ManagedIdentityIdParams = {

/** @public */
export type ManagedIdentityConfiguration = {
clientCapabilities?: Array<string>;
managedIdentityIdParams?: ManagedIdentityIdParams;
system?: NodeSystemOptions;
};
Expand Down Expand Up @@ -247,13 +248,15 @@ export function buildAppConfiguration({

/** @internal */
export type ManagedIdentityNodeConfiguration = {
clientCapabilities: Array<string>;
managedIdentityId: ManagedIdentityId;
system: Required<
Pick<NodeSystemOptions, "loggerOptions" | "networkClient">
>;
};

export function buildManagedIdentityConfiguration({
clientCapabilities,
managedIdentityIdParams,
system,
}: ManagedIdentityConfiguration): ManagedIdentityNodeConfiguration {
Expand Down Expand Up @@ -290,6 +293,7 @@ export function buildManagedIdentityConfiguration({
}

return {
clientCapabilities: clientCapabilities || [],
managedIdentityId: managedIdentityId,
system: {
loggerOptions,
Expand Down
8 changes: 5 additions & 3 deletions lib/msal-node/src/request/ManagedIdentityRequestParams.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,15 @@

/**
* ManagedIdentityRequest
* - claims - a stringified claims request which will be used to determine whether or not the cache should be skipped
* - forceRefresh - forces managed identity requests to skip the cache and make network calls if true
* - resource - resource requested to access the protected API. It should be of the form "ResourceIdUri" or "ResourceIdUri/.default". For instance https://management.azure.net or, for Microsoft Graph, https://graph.microsoft.com/.default
* - claims - a stringified claims request which will be used to determine whether or not the cache should be skipped
* - clientCapabilities - an array of client capabilities to be sent to ESTS
* - forceRefresh - forces managed identity requests to skip the cache and make network calls if true
* - resource - resource requested to access the protected API. It should be of the form "ResourceIdUri" or "ResourceIdUri/.default". For instance https://management.azure.net or, for Microsoft Graph, https://graph.microsoft.com/.default
* @public
*/
export type ManagedIdentityRequestParams = {
claims?: string;
clientCapabilities?: Array<string>;
forceRefresh?: boolean;
resource: string;
};
130 changes: 129 additions & 1 deletion lib/msal-node/test/client/ManagedIdentitySources/Imds.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ import {
} from "../../../src";
Copy link
Member

Choose a reason for hiding this comment

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

The tests aren't related to IMDS? Is this the right location for them?

// NodeJS 16+ provides a built-in version of setTimeout that is promise-based
import { setTimeout } from "timers/promises";
import { CAE_CONSTANTS } from "../../test_kit/StringConstants.js";

describe("Acquires a token successfully via an IMDS Managed Identity", () => {
// IMDS doesn't need environment variables because there is a default IMDS endpoint
Expand Down Expand Up @@ -549,13 +550,136 @@ describe("Acquires a token successfully via an IMDS Managed Identity", () => {
);
});

test("ignores a cached token when claims are provided", async () => {
test('checks if a token was cached or not and if was "bypass_cache=true" was sent to ESTS depending on whether or not claims are provided, and ensures "xms=client-capabilites-string" was sent to ESTS when client capabilities are provided via the Managed Identity Request Parameters', async () => {

Choose a reason for hiding this comment

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

I think you should also include tests when claims are passed, and client capabilities are not and when client capabilities are passed, and claims are not.

Copy link
Member

@bgavrilMS bgavrilMS Nov 26, 2024

Choose a reason for hiding this comment

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

I think some are covered, but the tests are somewhat difficult to read.

There are 3 dimensions for this test, each with 2 possible values. So a total of 2^3 = 8 combos.

CP1: {sent, not_sent}
Claims: {sent, not_sent}
Token_in_cache? { yes, no}

const sendGetRequestAsyncSpy: jest.SpyInstance = jest.spyOn(
networkClient,
<any>"sendGetRequestAsync"
);

Robbie-Microsoft marked this conversation as resolved.
Show resolved Hide resolved
let networkManagedIdentityResult: AuthenticationResult =
await systemAssignedManagedIdentityApplication.acquireToken({
clientCapabilities: CAE_CONSTANTS.CLIENT_CAPABILITIES,
Robbie-Microsoft marked this conversation as resolved.
Show resolved Hide resolved
resource: MANAGED_IDENTITY_RESOURCE,
});
expect(networkManagedIdentityResult.fromCache).toBe(false);
expect(networkManagedIdentityResult.accessToken).toEqual(
DEFAULT_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken
);
let tokenRequest = sendGetRequestAsyncSpy.mock.lastCall;
let url = tokenRequest[0];
expect(url.includes("bypass_cache=true")).toBe(false);
expect(
url.includes(
`xms_cc=${CAE_CONSTANTS.CLIENT_CAPABILITIES.toString()}`
)
).toBe(true);

Robbie-Microsoft marked this conversation as resolved.
Show resolved Hide resolved
const cachedManagedIdentityResult: AuthenticationResult =
await systemAssignedManagedIdentityApplication.acquireToken({
clientCapabilities: CAE_CONSTANTS.CLIENT_CAPABILITIES,
resource: MANAGED_IDENTITY_RESOURCE,
});
expect(cachedManagedIdentityResult.fromCache).toBe(true);
expect(cachedManagedIdentityResult.accessToken).toEqual(
DEFAULT_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken
);

Robbie-Microsoft marked this conversation as resolved.
Show resolved Hide resolved
networkManagedIdentityResult =
await systemAssignedManagedIdentityApplication.acquireToken({
claims: TEST_CONFIG.CLAIMS,
clientCapabilities: CAE_CONSTANTS.CLIENT_CAPABILITIES,
resource: MANAGED_IDENTITY_RESOURCE,
});
expect(networkManagedIdentityResult.fromCache).toBe(false);
expect(networkManagedIdentityResult.accessToken).toEqual(
DEFAULT_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken
);
tokenRequest = sendGetRequestAsyncSpy.mock.lastCall;
url = tokenRequest[0];
expect(url.includes("bypass_cache=true")).toBe(true);
expect(
url.includes(
`xms_cc=${CAE_CONSTANTS.CLIENT_CAPABILITIES.toString()}`
)
).toBe(true);
});

test('checks if a token was cached or not and if was "bypass_cache=true" was sent to ESTS depending on whether or not claims are provided, and ensures "xms=client-capabilites-string" was sent to ESTS when client capabilities are provided via the Managed Identity configuration object', async () => {
const sendGetRequestAsyncSpy: jest.SpyInstance = jest.spyOn(
networkClient,
<any>"sendGetRequestAsync"
);

const systemAssignedManagedIdentityApplicationWithClientCapabilities: ManagedIdentityApplication =
new ManagedIdentityApplication({
...systemAssignedConfig,
clientCapabilities: CAE_CONSTANTS.CLIENT_CAPABILITIES,
});

let networkManagedIdentityResult: AuthenticationResult =
await systemAssignedManagedIdentityApplicationWithClientCapabilities.acquireToken(
{
resource: MANAGED_IDENTITY_RESOURCE,
}
);
expect(networkManagedIdentityResult.fromCache).toBe(false);
expect(networkManagedIdentityResult.accessToken).toEqual(
DEFAULT_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken
);

let tokenRequest = sendGetRequestAsyncSpy.mock.lastCall;
let url = tokenRequest[0];
expect(url.includes("bypass_cache=true")).toBe(false);
expect(
url.includes(
`xms_cc=${CAE_CONSTANTS.CLIENT_CAPABILITIES.toString()}`
)
).toBe(true);

const cachedManagedIdentityResult: AuthenticationResult =
await systemAssignedManagedIdentityApplicationWithClientCapabilities.acquireToken(
{
resource: MANAGED_IDENTITY_RESOURCE,
}
);
expect(cachedManagedIdentityResult.fromCache).toBe(true);
expect(cachedManagedIdentityResult.accessToken).toEqual(
DEFAULT_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken
);

networkManagedIdentityResult =
await systemAssignedManagedIdentityApplicationWithClientCapabilities.acquireToken(
{
claims: TEST_CONFIG.CLAIMS,
resource: MANAGED_IDENTITY_RESOURCE,
}
);
expect(networkManagedIdentityResult.fromCache).toBe(false);
expect(networkManagedIdentityResult.accessToken).toEqual(
DEFAULT_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken
);

tokenRequest = sendGetRequestAsyncSpy.mock.lastCall;
url = tokenRequest[0];
expect(url.includes("bypass_cache=true")).toBe(true);
expect(
url.includes(
`xms_cc=${CAE_CONSTANTS.CLIENT_CAPABILITIES.toString()}`
)
).toBe(true);
});

test('ignores a cached token when claims are provided, and ensures "bypass_cache=true" was sent to ESTS', async () => {
const sendGetRequestAsyncSpy: jest.SpyInstance = jest.spyOn(
networkClient,
<any>"sendGetRequestAsync"
);

let networkManagedIdentityResult: AuthenticationResult =
await systemAssignedManagedIdentityApplication.acquireToken({
resource: MANAGED_IDENTITY_RESOURCE,
});
expect(networkManagedIdentityResult.fromCache).toBe(false);
expect(networkManagedIdentityResult.accessToken).toEqual(
DEFAULT_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken
);
Expand All @@ -578,6 +702,10 @@ describe("Acquires a token successfully via an IMDS Managed Identity", () => {
expect(networkManagedIdentityResult.accessToken).toEqual(
DEFAULT_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken
);

const tokenRequest = sendGetRequestAsyncSpy.mock.lastCall;
const url = tokenRequest[0];
expect(url.includes("bypass_cache=true")).toBe(true);
});

test("ignores a cached token when forceRefresh is set to true", async () => {
Expand Down
Loading