From 3e9810d25736272ee28ff2e9625d2bff50631022 Mon Sep 17 00:00:00 2001 From: Maximilian Pfeffer Date: Tue, 28 Jul 2026 01:11:38 +0200 Subject: [PATCH] bugfix #1019 - wrong throttling on clientid. --- .../com/microsoft/aad/msal4j/HttpHelper.java | 19 ++-- .../aad/msal4j/RequestThrottlingTest.java | 97 +++++++++++++++++++ 2 files changed, 107 insertions(+), 9 deletions(-) diff --git a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/HttpHelper.java b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/HttpHelper.java index 2f6da6ae..9e06d8bb 100644 --- a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/HttpHelper.java +++ b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/HttpHelper.java @@ -125,18 +125,19 @@ IHttpResponse executeHttpRequest(HttpRequest httpRequest) { private String getRequestThumbprint(RequestContext requestContext) { StringBuilder sb = new StringBuilder(); - sb.append(requestContext.clientId() + POINT_DELIMITER); - sb.append(requestContext.authority() + POINT_DELIMITER); - - IAcquireTokenParameters apiParameters = requestContext.apiParameters(); - - if (apiParameters instanceof SilentParameters) { - IAccount account = ((SilentParameters) apiParameters).account(); - if (account != null) { - sb.append(account.homeAccountId() + POINT_DELIMITER); + sb.append(requestContext.clientId()).append(POINT_DELIMITER); + sb.append(requestContext.authority()).append(POINT_DELIMITER); + + UserIdentifier userIdentifier = requestContext.userIdentifier(); + if (userIdentifier != null) { + if (!StringHelper.isBlank(userIdentifier.upn())) { + sb.append(userIdentifier.upn()).append(POINT_DELIMITER); + } else if (!StringHelper.isBlank(userIdentifier.oid())) { + sb.append(userIdentifier.oid()).append(POINT_DELIMITER); } } + IAcquireTokenParameters apiParameters = requestContext.apiParameters(); Set sortedScopes = new TreeSet<>(apiParameters.scopes()); sb.append(String.join(" ", sortedScopes)); diff --git a/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/RequestThrottlingTest.java b/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/RequestThrottlingTest.java index 7544f6a4..05e8ab31 100644 --- a/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/RequestThrottlingTest.java +++ b/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/RequestThrottlingTest.java @@ -7,6 +7,8 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInstance; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doReturn; @@ -27,6 +29,7 @@ class RequestThrottlingTest { public final Integer THROTTLE_IN_SEC = 1; public TokenEndpointResponseType responseType; IHttpClient httpClientMock = mock(IHttpClient.class); + private boolean skipInvocationCountCheck = false; @BeforeEach @@ -36,6 +39,9 @@ void init() { @AfterEach void check() throws Exception { + if (skipInvocationCountCheck) { + return; + } //throttlingTest() makes three non-throttled calls, so for a test without a retry there should be // 3 invocations of httpClientMock.send(), and 6 invocations if the calls are set to retry @@ -199,4 +205,95 @@ void STSResponseContains_StatusCode500_RetryAfterHeader() throws Exception { ThrottlingCache.DEFAULT_THROTTLING_TIME_SEC = 1000; throttlingTest(TokenEndpointResponseType.STATUS_CODE_500_RETRY_AFTER_HEADER); } + + private UserNamePasswordParameters getUserNamePasswordApiParameters(String username, String scope) { + return UserNamePasswordParameters + .builder(Collections.singleton(scope), username, "password".toCharArray()) + .build(); + } + + // Regression test for issue #1019: a failed request for one user must not throttle a different + // user under the same clientId/authority/scope. + @Test + void STSResponseContains_StatusCode500_DifferentUsersNotThrottledForEachOther() throws Exception { + skipInvocationCountCheck = true; + ThrottlingCache.clear(); + + // user A's request fails with a 500 -> gets cached as a throttled request + PublicClientApplication app = + getClientApplicationMockedWithOneTokenEndpointResponse(TokenEndpointResponseType.STATUS_CODE_500); + try { + app.acquireToken(getUserNamePasswordApiParameters("userA@contoso.com", "scope1")).join(); + } catch (Exception ex) { + if (!(ex.getCause() instanceof MsalServiceException)) { + fail("Unexpected exception"); + } + } + + // repeating user A's request should be throttled + try { + app = getPublicClientApp(); + app.acquireToken(getUserNamePasswordApiParameters("userA@contoso.com", "scope1")).join(); + fail("Expected MsalThrottlingException"); + } catch (Exception ex) { + if (!(ex.getCause() instanceof MsalThrottlingException)) { + fail("Unexpected exception"); + } + } + + // user B, same clientId/authority/scope, must NOT be throttled by user A's failure + app = getClientApplicationMockedWithOneTokenEndpointResponse(TokenEndpointResponseType.STATUS_CODE_500); + try { + app.acquireToken(getUserNamePasswordApiParameters("userB@contoso.com", "scope1")).join(); + } catch (Exception ex) { + if (!(ex.getCause() instanceof MsalServiceException)) { + fail("User B should not be throttled by user A's failed request"); + } + } + } + + // Confirms that per-account throttling isolation for the silent flow (previously driven by the + // `instanceof SilentParameters` special case) is preserved now that the fingerprint is derived + // from RequestContext.userIdentifier() instead. + @Test + void SilentFlow_DifferentAccountsThrottledIndependently() throws Exception { + skipInvocationCountCheck = true; + ThrottlingCache.clear(); + + PublicClientApplication app = getPublicClientApp(); + + IHttpClient localHttpClientMock = mock(IHttpClient.class); + HttpResponse http500 = new HttpResponse(); + http500.statusCode(HttpStatus.HTTP_INTERNAL_ERROR); + http500.body(TestConfiguration.TOKEN_ENDPOINT_INVALID_GRANT_ERROR_RESPONSE); + http500.addHeaders(Collections.singletonMap("Content-Type", Collections.singletonList("application/json"))); + doReturn(http500).when(localHttpClientMock).send(any()); + + HttpHelper httpHelper = new HttpHelper(localHttpClientMock, new DefaultRetryPolicy()); + ServiceBundle serviceBundle = new ServiceBundle(null, new TelemetryManager(null, false), httpHelper); + + HttpRequest httpRequest = new HttpRequest(HttpMethod.POST, + "https://login.microsoftonline.com/common/oauth2/v2.0/token"); + + IAccount accountA = mock(IAccount.class); + doReturn("oidA.tidA").when(accountA).homeAccountId(); + SilentParameters paramsA = SilentParameters.builder(Collections.singleton("scope1"), accountA).build(); + RequestContext contextA = new RequestContext(app, PublicApi.ACQUIRE_TOKEN_SILENTLY, paramsA, + UserIdentifier.fromHomeAccountId(accountA.homeAccountId())); + + // account A's request fails with a 500 -> gets cached as a throttled request + httpHelper.executeHttpRequest(httpRequest, contextA, serviceBundle); + // repeating account A's request should be throttled + assertThrows(MsalThrottlingException.class, + () -> httpHelper.executeHttpRequest(httpRequest, contextA, serviceBundle)); + + // account B, same clientId/authority/scope, must NOT be throttled by account A's failure + IAccount accountB = mock(IAccount.class); + doReturn("oidB.tidB").when(accountB).homeAccountId(); + SilentParameters paramsB = SilentParameters.builder(Collections.singleton("scope1"), accountB).build(); + RequestContext contextB = new RequestContext(app, PublicApi.ACQUIRE_TOKEN_SILENTLY, paramsB, + UserIdentifier.fromHomeAccountId(accountB.homeAccountId())); + + assertDoesNotThrow(() -> httpHelper.executeHttpRequest(httpRequest, contextB, serviceBundle)); + } }