Skip to content

Commit 61a6c88

Browse files
Avery-DunnAvery-Dunn
committed
Fix caching bug
1 parent 2fb864a commit 61a6c88

2 files changed

Lines changed: 81 additions & 27 deletions

File tree

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

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -91,13 +91,6 @@ private IAccount findCachedAccount() {
9191
return account;
9292
}
9393
}
94-
95-
// If no exact match but there's only one account, use it.
96-
// This handles cases where the IdP returns a slightly different username format
97-
// (e.g., preferred_username vs UPN) but the token is still for the same user.
98-
if (accounts.size() == 1) {
99-
return accounts.iterator().next();
100-
}
10194
} catch (Exception ex) {
10295
LOG.debug("Error looking up cached accounts: {}", ex.getMessage());
10396
}

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

Lines changed: 81 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,6 @@ void userFic_SendsCorrectGrantType() throws Exception {
7070

7171
UserFederatedIdentityCredentialParameters parameters = UserFederatedIdentityCredentialParameters
7272
.builder(SCOPES, TEST_UPN, FAKE_ASSERTION)
73-
.forceRefresh(true)
7473
.build();
7574

7675
// Act
@@ -97,7 +96,6 @@ void userFic_SendsAssertionInBody() throws Exception {
9796

9897
UserFederatedIdentityCredentialParameters parameters = UserFederatedIdentityCredentialParameters
9998
.builder(SCOPES, TEST_UPN, FAKE_ASSERTION)
100-
.forceRefresh(true)
10199
.build();
102100

103101
// Act
@@ -124,7 +122,6 @@ void userFic_WithUpn_SendsUsernameNotUserId() throws Exception {
124122

125123
UserFederatedIdentityCredentialParameters parameters = UserFederatedIdentityCredentialParameters
126124
.builder(SCOPES, TEST_UPN, FAKE_ASSERTION)
127-
.forceRefresh(true)
128125
.build();
129126

130127
// Act
@@ -148,13 +145,12 @@ void userFic_WithOid_SendsUserIdNotUsername() throws Exception {
148145

149146
UserFederatedIdentityCredentialParameters parameters = UserFederatedIdentityCredentialParameters
150147
.builder(SCOPES, TEST_OID, FAKE_ASSERTION)
151-
.forceRefresh(true)
152148
.build();
153149

154150
// Act
155151
cca.acquireToken(parameters).get();
156152

157-
// Assert — user_id present, username absent (as grant param, may appear in common params)
153+
// Assert — user_id present, username absent(as grant param, may appear in common params)
158154
verify(httpClientMock).send(argThat(request -> {
159155
String body = request.body();
160156
return body.contains("user_id=" + TEST_OID.toString())
@@ -176,7 +172,6 @@ void userFic_SendsAllOAuthParametersTogether() throws Exception {
176172

177173
UserFederatedIdentityCredentialParameters parameters = UserFederatedIdentityCredentialParameters
178174
.builder(SCOPES, TEST_UPN, FAKE_ASSERTION)
179-
.forceRefresh(true)
180175
.build();
181176

182177
// Act
@@ -206,7 +201,6 @@ void userFic_ScopeIncludesOidcScopes() throws Exception {
206201

207202
UserFederatedIdentityCredentialParameters parameters = UserFederatedIdentityCredentialParameters
208203
.builder(SCOPES, TEST_UPN, FAKE_ASSERTION)
209-
.forceRefresh(true)
210204
.build();
211205

212206
// Act
@@ -235,7 +229,6 @@ void userFic_TokenStoredInUserCache() throws Exception {
235229

236230
UserFederatedIdentityCredentialParameters parameters = UserFederatedIdentityCredentialParameters
237231
.builder(SCOPES, TEST_UPN, FAKE_ASSERTION)
238-
.forceRefresh(true)
239232
.build();
240233

241234
// Act
@@ -255,25 +248,26 @@ void userFic_TokenStoredInUserCache() throws Exception {
255248
@Test
256249
void userFic_ForceRefresh_BypassesCache() throws Exception {
257250
// Arrange
251+
String oid = "oid-user-1234";
252+
String tid = "f645ad92-e38d-4d1a-b510-d1b09a74a8ca";
258253
AtomicInteger callCount = new AtomicInteger(0);
259254

260255
DefaultHttpClient httpClientMock = mock(DefaultHttpClient.class);
261256
when(httpClientMock.send(any(HttpRequest.class))).thenAnswer(invocation -> {
262257
callCount.incrementAndGet();
263-
return createSuccessResponseWithIdToken();
258+
return createUserResponse(oid, TEST_UPN, "access-token", tid);
264259
});
265260

266261
ConfidentialClientApplication cca = createCca(httpClientMock);
267262

268-
// First call — populates cache
263+
// First call — populates cache (empty cache, goes to IdP)
269264
UserFederatedIdentityCredentialParameters params1 = UserFederatedIdentityCredentialParameters
270265
.builder(SCOPES, TEST_UPN, FAKE_ASSERTION)
271-
.forceRefresh(true)
272266
.build();
273267
cca.acquireToken(params1).get();
274268
assertEquals(1, callCount.get(), "First call should hit IdP");
275269

276-
// Second call with forceRefresh — should bypass cache
270+
// Second call with forceRefresh — should bypass cache despite matching cached account
277271
UserFederatedIdentityCredentialParameters params2 = UserFederatedIdentityCredentialParameters
278272
.builder(SCOPES, TEST_UPN, FAKE_ASSERTION)
279273
.forceRefresh(true)
@@ -289,25 +283,26 @@ void userFic_ForceRefresh_BypassesCache() throws Exception {
289283
@Test
290284
void userFic_CacheHit_WhenNotForceRefreshing() throws Exception {
291285
// Arrange
286+
String oid = "oid-user-1234";
287+
String tid = "f645ad92-e38d-4d1a-b510-d1b09a74a8ca";
292288
AtomicInteger callCount = new AtomicInteger(0);
293289

294290
DefaultHttpClient httpClientMock = mock(DefaultHttpClient.class);
295291
when(httpClientMock.send(any(HttpRequest.class))).thenAnswer(invocation -> {
296292
callCount.incrementAndGet();
297-
return createSuccessResponseWithIdToken();
293+
return createUserResponse(oid, TEST_UPN, "access-token-cached", tid);
298294
});
299295

300296
ConfidentialClientApplication cca = createCca(httpClientMock);
301297

302-
// First call — populates cache
298+
// First call — populates cache (cache is empty, so goes to IdP without needing forceRefresh)
303299
UserFederatedIdentityCredentialParameters params1 = UserFederatedIdentityCredentialParameters
304300
.builder(SCOPES, TEST_UPN, FAKE_ASSERTION)
305-
.forceRefresh(true)
306301
.build();
307302
cca.acquireToken(params1).get();
308303
assertEquals(1, callCount.get(), "First call should hit IdP");
309304

310-
// Second call without forceRefresh — should use cache
305+
// Second call without forceRefresh — should use cache (same UPN matches cached account)
311306
UserFederatedIdentityCredentialParameters params2 = UserFederatedIdentityCredentialParameters
312307
.builder(SCOPES, TEST_UPN, FAKE_ASSERTION)
313308
.forceRefresh(false)
@@ -448,7 +443,6 @@ void userFic_TwoUpns_SilentReturnsCorrectToken() throws Exception {
448443

449444
UserFederatedIdentityCredentialParameters aliceParams = UserFederatedIdentityCredentialParameters
450445
.builder(SCOPES, alice_upn, FAKE_ASSERTION)
451-
.forceRefresh(true)
452446
.build();
453447
IAuthenticationResult aliceResult = cca.acquireToken(aliceParams).get();
454448
assertEquals(alice_token, aliceResult.accessToken());
@@ -459,7 +453,6 @@ void userFic_TwoUpns_SilentReturnsCorrectToken() throws Exception {
459453

460454
UserFederatedIdentityCredentialParameters bobParams = UserFederatedIdentityCredentialParameters
461455
.builder(SCOPES, bob_upn, FAKE_ASSERTION)
462-
.forceRefresh(true)
463456
.build();
464457
IAuthenticationResult bobResult = cca.acquireToken(bobParams).get();
465458
assertEquals(bob_token, bobResult.accessToken());
@@ -517,7 +510,6 @@ void userFic_TwoOids_SilentReturnsCorrectToken() throws Exception {
517510

518511
UserFederatedIdentityCredentialParameters carolParams = UserFederatedIdentityCredentialParameters
519512
.builder(SCOPES, carol_upn, FAKE_ASSERTION)
520-
.forceRefresh(true)
521513
.build();
522514
IAuthenticationResult carolResult = cca.acquireToken(carolParams).get();
523515
assertEquals(carol_token, carolResult.accessToken());
@@ -527,7 +519,6 @@ void userFic_TwoOids_SilentReturnsCorrectToken() throws Exception {
527519

528520
UserFederatedIdentityCredentialParameters daveParams = UserFederatedIdentityCredentialParameters
529521
.builder(SCOPES, dave_upn, FAKE_ASSERTION)
530-
.forceRefresh(true)
531522
.build();
532523
IAuthenticationResult daveResult = cca.acquireToken(daveParams).get();
533524
assertEquals(dave_token, daveResult.accessToken());
@@ -554,4 +545,74 @@ void userFic_TwoOids_SilentReturnsCorrectToken() throws Exception {
554545
SilentParameters.builder(SCOPES, daveAccount).build()).get();
555546
assertEquals(dave_token, silentDave.accessToken(), "OID-based lookup for Dave should return Dave's token");
556547
}
548+
549+
/**
550+
* Regression test: verifies that when only one user's token is cached, a request
551+
* for a different user does NOT silently return the first user's cached token.
552+
*
553+
* Before the fix, findCachedAccount() had a fallback: if accounts.size() == 1 it
554+
* returned that sole account regardless of UPN/OID match. This caused Bob's initial
555+
* acquireToken call to silently return Alice's cached token instead of going to the IdP.
556+
*/
557+
@Test
558+
void userFic_SingleAccountFallback_DoesNotReturnWrongUserToken() throws Exception {
559+
// Arrange
560+
String alice_oid = "oid-alice-1111";
561+
String alice_upn = "alice@contoso.com";
562+
String alice_token = "access-token-alice";
563+
564+
String bob_oid = "oid-bob-2222";
565+
String bob_upn = "bob@contoso.com";
566+
String bob_token = "access-token-bob";
567+
568+
String tid = "f645ad92-e38d-4d1a-b510-d1b09a74a8ca";
569+
570+
AtomicInteger networkCalls = new AtomicInteger(0);
571+
AtomicReference<HttpResponse> nextResponse = new AtomicReference<>();
572+
573+
DefaultHttpClient httpClientMock = mock(DefaultHttpClient.class);
574+
when(httpClientMock.send(any(HttpRequest.class))).thenAnswer(invocation -> {
575+
networkCalls.incrementAndGet();
576+
return nextResponse.get();
577+
});
578+
579+
ConfidentialClientApplication cca = createCca(httpClientMock);
580+
581+
// Act 1: Acquire token for Alice (goes to IdP, gets cached)
582+
nextResponse.set(createUserResponse(alice_oid, alice_upn, alice_token, tid));
583+
584+
UserFederatedIdentityCredentialParameters aliceParams = UserFederatedIdentityCredentialParameters
585+
.builder(SCOPES, alice_upn, FAKE_ASSERTION)
586+
.build();
587+
IAuthenticationResult aliceResult = cca.acquireToken(aliceParams).get();
588+
assertEquals(alice_token, aliceResult.accessToken());
589+
int callsAfterAlice = networkCalls.get();
590+
591+
// At this point, cache has exactly one account (Alice).
592+
assertEquals(1, cca.getAccounts().get().size(), "Only Alice should be in cache");
593+
594+
// Act 2: Acquire token for Bob WITHOUT forceRefresh.
595+
// findCachedAccount() runs; there is one account (Alice) but it doesn't match Bob.
596+
// The correct behavior is to return null → go to IdP → get Bob's token.
597+
// The buggy behavior was: accounts.size()==1 → return Alice → return Alice's cached token.
598+
nextResponse.set(createUserResponse(bob_oid, bob_upn, bob_token, tid));
599+
600+
UserFederatedIdentityCredentialParameters bobParams = UserFederatedIdentityCredentialParameters
601+
.builder(SCOPES, bob_upn, FAKE_ASSERTION)
602+
.build();
603+
IAuthenticationResult bobResult = cca.acquireToken(bobParams).get();
604+
605+
// Assert: Bob should have gotten his own token, not Alice's
606+
assertEquals(bob_token, bobResult.accessToken(),
607+
"Bob should receive his own token, not Alice's cached token");
608+
assertNotEquals(alice_token, bobResult.accessToken(),
609+
"Bob's token must differ from Alice's");
610+
611+
// Assert: A network call was made for Bob (not just a cache hit)
612+
assertTrue(networkCalls.get() > callsAfterAlice,
613+
"A network call should have been made for Bob since Alice is a different user");
614+
615+
// Assert: Both accounts now in cache
616+
assertEquals(2, cca.getAccounts().get().size(), "Both Alice and Bob should be in cache");
617+
}
557618
}

0 commit comments

Comments
 (0)