Skip to content

Commit deba5cb

Browse files
Avery-DunnAvery-Dunn
committed
PR feedback
1 parent d0cb584 commit deba5cb

3 files changed

Lines changed: 153 additions & 12 deletions

File tree

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

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -66,9 +66,13 @@ final class ClientAssertion implements IClientAssertion {
6666

6767
/**
6868
* Gets the JWT assertion for client authentication.
69-
* If this ClientAssertion was created with a Callable, the callable will be
70-
* invoked each time this method is called to generate a fresh assertion.
71-
* If created with a context-aware Function, it is invoked with a default (empty) context.
69+
*
70+
* <p>Dispatch logic:
71+
* <ul>
72+
* <li>Context-aware provider: delegates to {@link #assertion(AssertionRequestOptions)} with empty context</li>
73+
* <li>Callable provider: invokes the callable to generate a fresh assertion</li>
74+
* <li>Static string: returns the stored assertion directly</li>
75+
* </ul>
7276
*
7377
* @return A JWT assertion string
7478
* @throws MsalClientException if the assertion provider returns null/empty or throws an exception
@@ -87,10 +91,17 @@ public String assertion() {
8791

8892
/**
8993
* Gets the JWT assertion for client authentication with context information.
90-
* If this ClientAssertion was created with a context-aware Function, the function will receive
91-
* the provided context. If created with a Callable or static string, the context is ignored.
9294
*
93-
* @param options context information for the assertion request
95+
* <p>Dispatch logic:
96+
* <ul>
97+
* <li>Context-aware provider: invokes the Function with the provided options</li>
98+
* <li>Callable or static: context is ignored, falls back to {@link #assertion()}</li>
99+
* </ul>
100+
*
101+
* <p>This method is the primary entry point used by {@code TokenRequestExecutor} when
102+
* building token requests, as it can pass FMI path and token endpoint context.
103+
*
104+
* @param options context information for the assertion request (may contain nulls for non-FMI flows)
94105
* @return A JWT assertion string
95106
* @throws MsalClientException if the assertion provider returns null/empty or throws an exception
96107
*/

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

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
import java.util.Map;
77
import java.util.Set;
8+
import java.util.TreeMap;
89

910
import static com.microsoft.aad.msal4j.ParameterValidationUtils.validateNotNull;
1011

@@ -106,14 +107,22 @@ public String fmiPath() {
106107
* Returns an empty string if fmiPath is not set.
107108
* This is the single source of truth for the fmi_path cache key hash computation,
108109
* used by both cache writes (TokenCache) and cache reads (silent lookup).
110+
* The result is memoized since ClientCredentialParameters is immutable after construction.
109111
*/
112+
private String fmiCacheKeyHashCache;
113+
110114
String computeFmiCacheKeyHash() {
115+
if (fmiCacheKeyHashCache != null) {
116+
return fmiCacheKeyHashCache;
117+
}
111118
if (!StringHelper.isBlank(fmiPath)) {
112-
java.util.TreeMap<String, String> components = new java.util.TreeMap<>();
119+
TreeMap<String, String> components = new TreeMap<>();
113120
components.put("fmi_path", fmiPath);
114-
return StringHelper.computeExtCacheKeyHash(components);
121+
fmiCacheKeyHashCache = StringHelper.computeExtCacheKeyHash(components);
122+
} else {
123+
fmiCacheKeyHashCache = "";
115124
}
116-
return "";
125+
return fmiCacheKeyHashCache;
117126
}
118127

119128
public static class ClientCredentialParametersBuilder {
@@ -203,9 +212,7 @@ public ClientCredentialParametersBuilder clientCredential(IClientCredential clie
203212
* @return builder that can be used to construct ClientCredentialParameters
204213
*/
205214
public ClientCredentialParametersBuilder fmiPath(String fmiPath) {
206-
if (fmiPath != null && fmiPath.trim().isEmpty()) {
207-
throw new IllegalArgumentException("fmiPath cannot be empty or blank");
208-
}
215+
ParameterValidationUtils.validateNotBlank("fmiPath", fmiPath);
209216
this.fmiPath = fmiPath;
210217
return this;
211218
}

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

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,15 @@ void fmiPath_WhitespaceOnlyThrowsIllegalArgumentException() {
357357
.build());
358358
}
359359

360+
@Test
361+
void fmiPath_NullValueThrowsIllegalArgumentException() {
362+
assertThrows(IllegalArgumentException.class, () ->
363+
ClientCredentialParameters
364+
.builder(Collections.singleton("scope"))
365+
.fmiPath(null)
366+
.build());
367+
}
368+
360369
// ========================================================================
361370
// Exact cache key string validation
362371
// ========================================================================
@@ -451,4 +460,118 @@ void fmiPath_NoFmiPath_CacheKeyUsesAccessTokenCredentialType() throws Exception
451460
"Cache key without fmi_path should use 'accesstoken' credential type and no hash suffix");
452461
}
453462

463+
// ========================================================================
464+
// Cache filter isolation: FMI tokens not returned for non-FMI requests (and vice versa)
465+
// ========================================================================
466+
467+
@Test
468+
void fmiPath_CacheIsolation_FmiTokenNotReturnedForNonFmiRequest() throws Exception {
469+
// Seed cache with an FMI-tagged token, then verify a non-FMI request does NOT
470+
// return it from cache (goes to IdP instead).
471+
DefaultHttpClient httpClientMock = mock(DefaultHttpClient.class);
472+
473+
when(httpClientMock.send(any(HttpRequest.class))).thenReturn(
474+
TestHelper.expectedResponse(HttpStatus.HTTP_OK,
475+
TestHelper.getSuccessfulTokenResponse(new HashMap<>())));
476+
477+
ConfidentialClientApplication cca =
478+
ConfidentialClientApplication.builder("clientId",
479+
ClientCredentialFactory.createFromSecret("secret"))
480+
.authority("https://login.microsoftonline.com/tenant/")
481+
.aadInstanceDiscoveryResponse(TestHelper.getInstanceDiscoveryResponse())
482+
.httpClient(httpClientMock)
483+
.build();
484+
485+
// First request WITH fmi_path — seeds cache with an FMI-tagged token
486+
ClientCredentialParameters fmiParams = ClientCredentialParameters
487+
.builder(Collections.singleton("scope"))
488+
.fmiPath("agentApp1")
489+
.build();
490+
cca.acquireToken(fmiParams).get();
491+
assertEquals(1, cca.tokenCache.accessTokens.size());
492+
493+
// Second request WITHOUT fmi_path — should NOT get the FMI token from cache
494+
ClientCredentialParameters nonFmiParams = ClientCredentialParameters
495+
.builder(Collections.singleton("scope"))
496+
.build();
497+
cca.acquireToken(nonFmiParams).get();
498+
499+
// Both tokens should now be in cache (FMI miss → went to IdP → stored as non-FMI)
500+
assertEquals(2, cca.tokenCache.accessTokens.size(),
501+
"Non-FMI request should not match FMI-tagged cache entry; both tokens should exist");
502+
}
503+
504+
@Test
505+
void fmiPath_CacheIsolation_NonFmiTokenNotReturnedForFmiRequest() throws Exception {
506+
// Seed cache with a non-FMI token, then verify an FMI request does NOT
507+
// return it from cache (goes to IdP instead).
508+
DefaultHttpClient httpClientMock = mock(DefaultHttpClient.class);
509+
510+
when(httpClientMock.send(any(HttpRequest.class))).thenReturn(
511+
TestHelper.expectedResponse(HttpStatus.HTTP_OK,
512+
TestHelper.getSuccessfulTokenResponse(new HashMap<>())));
513+
514+
ConfidentialClientApplication cca =
515+
ConfidentialClientApplication.builder("clientId",
516+
ClientCredentialFactory.createFromSecret("secret"))
517+
.authority("https://login.microsoftonline.com/tenant/")
518+
.aadInstanceDiscoveryResponse(TestHelper.getInstanceDiscoveryResponse())
519+
.httpClient(httpClientMock)
520+
.build();
521+
522+
// First request WITHOUT fmi_path — seeds cache with a non-FMI token
523+
ClientCredentialParameters nonFmiParams = ClientCredentialParameters
524+
.builder(Collections.singleton("scope"))
525+
.build();
526+
cca.acquireToken(nonFmiParams).get();
527+
assertEquals(1, cca.tokenCache.accessTokens.size());
528+
529+
// Second request WITH fmi_path — should NOT get the non-FMI token from cache
530+
ClientCredentialParameters fmiParams = ClientCredentialParameters
531+
.builder(Collections.singleton("scope"))
532+
.fmiPath("agentApp1")
533+
.build();
534+
cca.acquireToken(fmiParams).get();
535+
536+
// Both tokens should now be in cache (FMI miss → went to IdP → stored as FMI)
537+
assertEquals(2, cca.tokenCache.accessTokens.size(),
538+
"FMI request should not match non-FMI cache entry; both tokens should exist");
539+
}
540+
541+
@Test
542+
void fmiPath_CacheIsolation_DifferentFmiPathsNotShared() throws Exception {
543+
// Two different fmi_path values should produce separate cache entries
544+
DefaultHttpClient httpClientMock = mock(DefaultHttpClient.class);
545+
546+
when(httpClientMock.send(any(HttpRequest.class))).thenReturn(
547+
TestHelper.expectedResponse(HttpStatus.HTTP_OK,
548+
TestHelper.getSuccessfulTokenResponse(new HashMap<>())));
549+
550+
ConfidentialClientApplication cca =
551+
ConfidentialClientApplication.builder("clientId",
552+
ClientCredentialFactory.createFromSecret("secret"))
553+
.authority("https://login.microsoftonline.com/tenant/")
554+
.aadInstanceDiscoveryResponse(TestHelper.getInstanceDiscoveryResponse())
555+
.httpClient(httpClientMock)
556+
.build();
557+
558+
// Request with fmi_path "agentA"
559+
ClientCredentialParameters paramsA = ClientCredentialParameters
560+
.builder(Collections.singleton("scope"))
561+
.fmiPath("agentA")
562+
.build();
563+
cca.acquireToken(paramsA).get();
564+
565+
// Request with fmi_path "agentB"
566+
ClientCredentialParameters paramsB = ClientCredentialParameters
567+
.builder(Collections.singleton("scope"))
568+
.fmiPath("agentB")
569+
.build();
570+
cca.acquireToken(paramsB).get();
571+
572+
// Each fmi_path produces a different hash → different cache key → 2 entries
573+
assertEquals(2, cca.tokenCache.accessTokens.size(),
574+
"Different fmi_path values should produce separate cache entries");
575+
}
576+
454577
}

0 commit comments

Comments
 (0)