Skip to content

Commit b0db0ee

Browse files
Avery-DunnAvery-Dunn
committed
Minor cleanup
1 parent 3a8ba3e commit b0db0ee

2 files changed

Lines changed: 24 additions & 176 deletions

File tree

‎msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AcquireTokenForAgentSupplier.java‎

Lines changed: 23 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -103,20 +103,8 @@ AuthenticationResult execute() throws Exception {
103103
// Leg 3: Exchange the assertion for a user-scoped token via UserFIC.
104104
// The result is written to the Agent CCA's user token cache for future silent retrieval.
105105
LOG.debug("Executing Leg 3 (user FIC token) for agent app ID: {}", agentAppId);
106-
UserFederatedIdentityCredentialParameters ficParams;
107-
if (agentIdentity.userObjectId() != null) {
108-
ficParams = propagateToUserFicParams(
109-
UserFederatedIdentityCredentialParameters
110-
.builder(callerScopes, agentIdentity.userObjectId(), assertion)
111-
.forceRefresh(true)) // always fetch from network (we already checked the cache above)
112-
.build();
113-
} else {
114-
ficParams = propagateToUserFicParams(
115-
UserFederatedIdentityCredentialParameters
116-
.builder(callerScopes, agentIdentity.username(), assertion)
117-
.forceRefresh(true))
118-
.build();
119-
}
106+
UserFederatedIdentityCredentialParameters ficParams = buildLeg3FicParams(
107+
callerScopes, agentIdentity, assertion);
120108

121109
return (AuthenticationResult) joinAndUnwrap(agentCca.acquireToken(ficParams));
122110
}
@@ -181,6 +169,27 @@ private static String extractOid(String homeAccountId) {
181169
return dotIndex >= 0 ? homeAccountId.substring(0, dotIndex) : homeAccountId;
182170
}
183171

172+
/**
173+
* Builds the Leg 3 (UserFIC) parameters, selecting the OID or UPN overload as
174+
* appropriate. ForceRefresh is always set because the caller-level silent check
175+
* (tryAcquireTokenSilent) has already run; if we reach Leg 3 the token must be
176+
* fetched from the network.
177+
*/
178+
private UserFederatedIdentityCredentialParameters buildLeg3FicParams(
179+
Set<String> callerScopes,
180+
AgentIdentity agentIdentity,
181+
String assertion) {
182+
UserFederatedIdentityCredentialParameters.UserFederatedIdentityCredentialParametersBuilder builder;
183+
if (agentIdentity.userObjectId() != null) {
184+
builder = UserFederatedIdentityCredentialParameters
185+
.builder(callerScopes, agentIdentity.userObjectId(), assertion);
186+
} else {
187+
builder = UserFederatedIdentityCredentialParameters
188+
.builder(callerScopes, agentIdentity.username(), assertion);
189+
}
190+
return propagateToUserFicParams(builder.forceRefresh(true)).build();
191+
}
192+
184193
// ========================================================================
185194
// Outer Request Parameter Propagation
186195
// ========================================================================

‎msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/AcquireTokenForAgentTest.java‎

Lines changed: 1 addition & 162 deletions
Original file line numberDiff line numberDiff line change
@@ -267,7 +267,7 @@ void acquireTokenForAgent_withOid_acquiresUserToken() throws Exception {
267267
// ========================================================================
268268

269269
@Test
270-
void acquireTokenForAgent_samAgentId_reusesCca() throws Exception {
270+
void acquireTokenForAgent_sameAgentId_reusesCca() throws Exception {
271271
// Arrange
272272
DefaultHttpClient httpClientMock = mock(DefaultHttpClient.class);
273273

@@ -827,165 +827,4 @@ void acquireTokenForAgent_leg2CacheIsolation_credentialFmiPathPreventsCollision(
827827
assertEquals(3, agentCca.tokenCache.accessTokens.size(),
828828
"Agent CCA should have 3 tokens: 1 Leg 2 assertion + 2 user tokens");
829829
}
830-
831-
// ========================================================================
832-
// Scope collision test: caller uses api://AzureADTokenExchange/.default
833-
// which is the same scope used internally for Leg 2 assertion tokens.
834-
// This probes whether the flat cache can distinguish between an app-level
835-
// assertion token and a user-level FIC token when both share the same scope.
836-
// ========================================================================
837-
838-
@Test
839-
void acquireTokenForAgent_callerScopeMatchesInternalAssertionScope_noCollision() throws Exception {
840-
// The internal Leg 2 uses api://AzureADTokenExchange/.default for the assertion token.
841-
// If an external caller also requests that scope for a user token, the agent CCA's
842-
// flat cache will contain both an app token and a user token with the same scope.
843-
Set<String> tokenExchangeScope = Collections.singleton("api://AzureADTokenExchange/.default");
844-
845-
String user3Upn = "charlie@contoso.com";
846-
String user3Oid = "33333333-3333-3333-3333-333333333333";
847-
848-
DefaultHttpClient httpClientMock = mock(DefaultHttpClient.class);
849-
850-
when(httpClientMock.send(any(HttpRequest.class)))
851-
.thenReturn(
852-
// --- Alice: normal scopes (Legs 1+2+3 = 3 HTTP calls) ---
853-
createAppTokenResponse("fmi-credential-token"),
854-
createAppTokenResponse("assertion-token-leg2"),
855-
createUserTokenResponse("alice-graph-token", USER1_UPN, USER1_OID),
856-
857-
// --- Bob: api://AzureADTokenExchange scope (Leg 3 only = 1 HTTP call) ---
858-
// Legs 1+2 are cached from Alice's flow.
859-
// Leg 3 creates a USER token with the same scope as the Leg 2 app token.
860-
createUserTokenResponse("bob-exchange-token", USER2_UPN, USER2_OID),
861-
862-
// --- Charlie: non-agent client_credentials with same exchange scope ---
863-
// This goes through the BLUEPRINT CCA (not the agent CCA).
864-
createAppTokenResponse("charlie-exchange-app-token"));
865-
866-
ConfidentialClientApplication blueprintCca = createBlueprintCca(httpClientMock);
867-
868-
AgentIdentity aliceAgent = AgentIdentity.withUsername(AGENT_APP_ID, USER1_UPN);
869-
AgentIdentity bobAgent = AgentIdentity.withUsername(AGENT_APP_ID, USER2_UPN);
870-
871-
// ---- Step 1: Alice with normal Graph scopes ----
872-
IAuthenticationResult aliceResult = blueprintCca.acquireTokenForAgent(
873-
AcquireTokenForAgentParameters.builder(CALLER_SCOPES, aliceAgent).build()
874-
).get();
875-
876-
assertEquals("alice-graph-token", aliceResult.accessToken());
877-
verify(httpClientMock, times(3)).send(any(HttpRequest.class));
878-
879-
// ---- Step 2: Bob with api://AzureADTokenExchange/.default ----
880-
// This is the dangerous scenario: the agent CCA already has an app token
881-
// (assertion-token-leg2) for this exact scope from Leg 2. Now we're asking
882-
// for a USER token with the same scope. If the cache lookup for Leg 2 on
883-
// future calls uses findAny() without filtering by homeAccountId, it could
884-
// return Bob's user token instead of the assertion token.
885-
IAuthenticationResult bobResult = blueprintCca.acquireTokenForAgent(
886-
AcquireTokenForAgentParameters.builder(tokenExchangeScope, bobAgent).build()
887-
).get();
888-
889-
assertEquals("bob-exchange-token", bobResult.accessToken());
890-
verify(httpClientMock, times(4)).send(any(HttpRequest.class)); // +1 for Bob's Leg 3
891-
892-
// ---- Step 3: Verify agent CCA cache state ----
893-
ConfidentialClientApplication agentCca =
894-
blueprintCca.agentCcaCache.get("agent_" + AGENT_APP_ID);
895-
assertNotNull(agentCca);
896-
897-
// Agent CCA should have:
898-
// - 1 Leg 2 assertion token (app token, scope=api://AzureADTokenExchange/.default, homeAccountId="")
899-
// - 1 Alice user token (scope=graph.microsoft.com/.default, homeAccountId=alice-oid.tenant)
900-
// - 1 Bob user token (scope=api://AzureADTokenExchange/.default, homeAccountId=bob-oid.tenant)
901-
assertEquals(3, agentCca.tokenCache.accessTokens.size(),
902-
"Agent CCA cache should have 3 tokens (1 assertion + 2 user)");
903-
904-
// Count how many tokens have api://AzureADTokenExchange scope in the agent CCA.
905-
// There should be 2: the Leg 2 assertion token (app) and Bob's user token.
906-
long exchangeScopeTokenCount = agentCca.tokenCache.accessTokens.values().stream()
907-
.filter(at -> at.target() != null &&
908-
at.target().toLowerCase().contains("azureadtokenexchange"))
909-
.count();
910-
assertEquals(2, exchangeScopeTokenCount,
911-
"Agent CCA should have 2 tokens with the exchange scope (1 app + 1 user)");
912-
913-
// ---- Step 4: Alice again — should still return from cache (no collision) ----
914-
IAuthenticationResult aliceAgain = blueprintCca.acquireTokenForAgent(
915-
AcquireTokenForAgentParameters.builder(CALLER_SCOPES, aliceAgent).build()
916-
).get();
917-
918-
assertEquals("alice-graph-token", aliceAgain.accessToken());
919-
verify(httpClientMock, times(4)).send(any(HttpRequest.class)); // still 4
920-
921-
// ---- Step 5: Bob again — this is the critical test ----
922-
// When we request Bob's token again, the silent lookup should find Bob's
923-
// USER token (not the Leg 2 assertion token) even though both share the same scope.
924-
IAuthenticationResult bobAgain = blueprintCca.acquireTokenForAgent(
925-
AcquireTokenForAgentParameters.builder(tokenExchangeScope, bobAgent).build()
926-
).get();
927-
928-
assertEquals("bob-exchange-token", bobAgain.accessToken(),
929-
"Bob's silent retrieval should return the user token, not the assertion token");
930-
verify(httpClientMock, times(4)).send(any(HttpRequest.class)); // still 4
931-
932-
// ---- Step 6: Now trigger a NEW agent flow for a different user ----
933-
// This is where the Leg 2 collision matters most. A new user triggers Leg 2
934-
// (acquireToken(ClientCredentialParameters)) which uses getApplicationAccessTokenCacheEntity.
935-
// If that lookup returns Bob's USER token instead of the app assertion token,
936-
// Leg 3 will use the wrong assertion and fail or return incorrect results.
937-
//
938-
// We queue a Leg 3 response for a hypothetical third user to test this.
939-
// If Leg 2 correctly returns the cached assertion token, only 1 HTTP call (Leg 3) fires.
940-
// If Leg 2 gets a collision and returns Bob's user token as the assertion, the behavior
941-
// will be unpredictable (wrong assertion value, possibly an error, or 2+ HTTP calls).
942-
943-
String user3_upn = "charlie@contoso.com";
944-
String user3_oid = "33333333-3333-3333-3333-333333333333";
945-
946-
// Clear invocation history so we can count only Charlie's HTTP calls
947-
clearInvocations(httpClientMock);
948-
949-
// Reset mock for the next sequence: only Leg 3 should fire (1 call).
950-
// If Leg 2 has a cache collision, it may re-fetch (2+ calls), or if
951-
// the wrong token is used as the assertion, it may error out.
952-
when(httpClientMock.send(any(HttpRequest.class)))
953-
.thenReturn(
954-
createUserTokenResponse("charlie-exchange-token", user3_upn, user3_oid));
955-
956-
AgentIdentity charlieAgent = AgentIdentity.withUsername(AGENT_APP_ID, user3_upn);
957-
IAuthenticationResult charlieResult = blueprintCca.acquireTokenForAgent(
958-
AcquireTokenForAgentParameters.builder(tokenExchangeScope, charlieAgent).build()
959-
).get();
960-
961-
// If this assertion fails, it means Leg 2 returned Bob's user token instead of the
962-
// cached assertion token, causing downstream problems.
963-
assertEquals("charlie-exchange-token", charlieResult.accessToken(),
964-
"Charlie should get a fresh Leg 3 token using the cached Leg 2 assertion");
965-
966-
// Verify only 1 new HTTP call was made (Leg 3 for Charlie).
967-
// If 2+ new calls were made, Leg 2 had to re-fetch because of a cache collision.
968-
verify(httpClientMock, times(1)).send(any(HttpRequest.class));
969-
970-
// ---- Step 7: Final cache state verification ----
971-
assertEquals(4, agentCca.tokenCache.accessTokens.size(),
972-
"Agent CCA should now have 4 tokens: 1 assertion + 3 user tokens");
973-
974-
// ---- Step 8: Non-agent client_credentials with exchange scope on BLUEPRINT ----
975-
// This tests that the blueprint's own cache doesn't collide with the FMI token
976-
// (which also targets api://AzureADTokenExchange/.default but has an extCacheKeyHash).
977-
clearInvocations(httpClientMock);
978-
when(httpClientMock.send(any(HttpRequest.class)))
979-
.thenReturn(createAppTokenResponse("blueprint-exchange-app-token"));
980-
981-
IAuthenticationResult blueprintExchangeResult = blueprintCca.acquireToken(
982-
ClientCredentialParameters.builder(tokenExchangeScope).build()
983-
).get();
984-
985-
// The FMI token has extCacheKeyHash set (from fmi_path), so a plain
986-
// client_credentials call without fmi_path should NOT match it.
987-
// It should trigger a new HTTP call and store a separate cache entry.
988-
assertEquals("blueprint-exchange-app-token", blueprintExchangeResult.accessToken(),
989-
"Blueprint's non-FMI exchange token should not collide with the FMI token");
990-
}
991830
}

0 commit comments

Comments
 (0)