Added token revocation functionality to Managed Identity #7422
Conversation
Robbie-Microsoft
requested review from
sameerag,
tnorling,
hectormmg,
peterzenz and
a team
as code owners
| @@ -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 () => { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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}
|
@Robbie-Microsoft MSI v1 token revocation is supported only on MSI endpoints versioned |
lib/msal-node/src/client/ManagedIdentitySources/BaseManagedIdentitySource.ts
Outdated
Show resolved
Hide resolved
| @@ -55,6 +55,7 @@ import { | |||
| } from "../../../src"; | |||
There was a problem hiding this comment.
The tests aren't related to IMDS? Is this the right location for them?
Yes. Fixed in this PR now. |
| @@ -133,6 +133,17 @@ export abstract class BaseManagedIdentitySource { | |||
| managedIdentityId | |||
| ); | |||
|
|
|||
| // if claims are present, the MSI will get a new token | |||
| if (managedIdentityRequest.claims) { | |||
There was a problem hiding this comment.
suggest using const here,
const BYPASS_CACHE = "bypass_cache";
const XMS_CC = "xms_cc";
| @@ -549,13 +550,82 @@ 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 the MSI 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 () => { | |||
There was a problem hiding this comment.
No tests in the PR to validate that the MSI V1 endpoint version is updated to 2024-09-30
| @@ -133,6 +133,17 @@ export abstract class BaseManagedIdentitySource { | |||
| managedIdentityId | |||
| ); | |||
|
|
|||
| // if claims are present, the MSI will get a new token | |||
| if (managedIdentityRequest.claims) { | |||
There was a problem hiding this comment.
can we add logs to these two conditions? i.e. when claims or capabilities are present?
We should bump the source's version to the one that supports MSI V1 CAE. But there are no Azure Resource that still supports this in GA, so we should bump the version for just this PR, mark it as
No, we should not.
MI Teams needs to guarantee that the version is updated across the board / azure resources, we will then test add e2e and then release/GA. For now, this is just an internal preview. So do not merge this PR. |
If claims are present, the cache will already be bypassed. Now, additionally,
bypass_cache=truewill be included in the token request, which indicates to ESTS that it should get a new token.Client Capabilities can now, optionally, be sent to ESTS as a non-empty string query parameter in the form of
xms_cc=client-capabilities-string-separated-by-commas.Client Capabilities can be set in the Managed Identity configuration object, which is consistent with non-Managed Identity configurations.