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..e7bae616 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 @@ -123,20 +123,43 @@ IHttpResponse executeHttpRequest(HttpRequest httpRequest) { return httpResponse; } + /* + * Two throttle fingerprints are derived from a request: + * + * - The app-wide thumbprint (includeUser == false) keys on clientId + authority + scope only. + * It is used for service-directed rate limiting (HTTP 429), which applies to the whole client + * regardless of which user made the request. + * + * - The user-aware thumbprint (includeUser == true) additionally folds in the request's user + * component (OID, else UPN). It is used for error-class throttling (HTTP 5xx), which can be + * specific to a single user (e.g. ADFS returns HTTP 500 for one user's bad password) and must + * not block other users of the same client. + * + * An explicit Retry-After header only affects how long an entry is throttled, not which of the + * two fingerprints is used (that is decided by the response status class). + */ 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(); + return getRequestThumbprint(requestContext, true); + } - if (apiParameters instanceof SilentParameters) { - IAccount account = ((SilentParameters) apiParameters).account(); - if (account != null) { - sb.append(account.homeAccountId() + POINT_DELIMITER); + private String getRequestThumbprint(RequestContext requestContext, boolean includeUser) { + StringBuilder sb = new StringBuilder(); + sb.append(requestContext.clientId()).append(POINT_DELIMITER); + sb.append(requestContext.authority()).append(POINT_DELIMITER); + + if (includeUser) { + UserIdentifier userIdentifier = requestContext.userIdentifier(); + if (userIdentifier != null) { + // Prefer OID: it is the stable, guaranteed-unique user identifier + if (!StringHelper.isBlank(userIdentifier.oid())) { + sb.append(userIdentifier.oid()).append(POINT_DELIMITER); + } else if (!StringHelper.isBlank(userIdentifier.upn())) { + sb.append(userIdentifier.upn()).append(POINT_DELIMITER); + } } } + IAcquireTokenParameters apiParameters = requestContext.apiParameters(); Set sortedScopes = new TreeSet<>(apiParameters.scopes()); sb.append(String.join(" ", sortedScopes)); @@ -168,9 +191,17 @@ IHttpResponse executeHttpRequestWithRetries(HttpRequest httpRequest, IHttpClient private void checkForThrottling(RequestContext requestContext) { if (requestContext.clientApplication() instanceof PublicClientApplication && requestContext.apiParameters() != null) { - String requestThumbprint = getRequestThumbprint(requestContext); - - long retryInMs = ThrottlingCache.retryInMs(requestThumbprint); + // Check the app-wide key first (429 / Retry-After entries), then the user-aware key + // (5xx entries) when it differs from the app-wide key. + String appWideThumbprint = getRequestThumbprint(requestContext, false); + long retryInMs = ThrottlingCache.retryInMs(appWideThumbprint); + + if (retryInMs <= 0) { + String userAwareThumbprint = getRequestThumbprint(requestContext); + if (!userAwareThumbprint.equals(appWideThumbprint)) { + retryInMs = ThrottlingCache.retryInMs(userAwareThumbprint); + } + } if (retryInMs > 0) { throw new MsalThrottlingException(retryInMs); @@ -181,17 +212,26 @@ private void checkForThrottling(RequestContext requestContext) { private void processThrottlingInstructions(IHttpResponse httpResponse, RequestContext requestContext) { if (requestContext.clientApplication() instanceof PublicClientApplication) { Long expirationTimestamp = null; + // Scope is determined by the status class, not by the presence of a Retry-After header: + // 5xx errors can be user-specific (e.g. ADFS returns HTTP 500 for one user's bad + // password), so they are throttled per-user; HTTP 429 is service-directed and is + // throttled app-wide. An explicit Retry-After header only overrides the throttle + // *duration*, leaving the scope decision to the status code. + boolean userScoped = false; Integer retryAfterHeaderVal = getRetryAfterHeader(httpResponse); - if (retryAfterHeaderVal != null) { - expirationTimestamp = System.currentTimeMillis() + retryAfterHeaderVal * 1000; - } else if (httpResponse.statusCode() == HttpStatus.HTTP_TOO_MANY_REQUESTS || - (httpResponse.statusCode() >= HttpStatus.HTTP_INTERNAL_ERROR)) { - - expirationTimestamp = System.currentTimeMillis() + ThrottlingCache.DEFAULT_THROTTLING_TIME_SEC * 1000; + boolean isServerError = httpResponse.statusCode() >= HttpStatus.HTTP_INTERNAL_ERROR; + boolean isTooManyRequests = httpResponse.statusCode() == HttpStatus.HTTP_TOO_MANY_REQUESTS; + + if (retryAfterHeaderVal != null || isTooManyRequests || isServerError) { + int throttleDurationSec = retryAfterHeaderVal != null + ? retryAfterHeaderVal + : ThrottlingCache.DEFAULT_THROTTLING_TIME_SEC; + expirationTimestamp = System.currentTimeMillis() + throttleDurationSec * 1000; + userScoped = isServerError; } if (expirationTimestamp != null) { - ThrottlingCache.set(getRequestThumbprint(requestContext), expirationTimestamp); + ThrottlingCache.set(getRequestThumbprint(requestContext, userScoped), expirationTimestamp); } } } 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..379ae412 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,7 +29,7 @@ class RequestThrottlingTest { public final Integer THROTTLE_IN_SEC = 1; public TokenEndpointResponseType responseType; IHttpClient httpClientMock = mock(IHttpClient.class); - + private boolean skipInvocationCountCheck = false; @BeforeEach void init() { @@ -36,7 +38,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 if (responseType == TokenEndpointResponseType.STATUS_CODE_500) { @@ -199,4 +203,229 @@ 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(); + // Large window so the second call is deterministically still within the throttle period, + // independent of CI timing. + ThrottlingCache.DEFAULT_THROTTLING_TIME_SEC = 1000; + + // 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)); + } + + // Direct reproduction of issue #1019 at the throttle-key level, isolated from the multi-hop + // ROPC HTTP flow. Two different users of the same public-client flow (username/password) sharing + // clientId/authority/scope must be throttled independently: user B must not be throttled by user + // A's 500. Uses a large throttle window so the result is deterministic and unaffected by + // retry/timing. + @Test + void UserNamePassword_DifferentUsersThrottledIndependently() throws Exception { + skipInvocationCountCheck = true; + ThrottlingCache.clear(); + ThrottlingCache.DEFAULT_THROTTLING_TIME_SEC = 1000; + + 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"); + + UserNamePasswordParameters paramsA = getUserNamePasswordApiParameters("userA@contoso.com", "scope1"); + RequestContext contextA = new RequestContext(app, PublicApi.ACQUIRE_TOKEN_BY_USERNAME_PASSWORD, paramsA, + UserIdentifier.fromUpn("userA@contoso.com")); + + // user A's request fails with a 500 -> gets cached as a throttled request + httpHelper.executeHttpRequest(httpRequest, contextA, serviceBundle); + // repeating user A's request should be throttled + assertThrows(MsalThrottlingException.class, + () -> httpHelper.executeHttpRequest(httpRequest, contextA, serviceBundle)); + + // user B, same clientId/authority/scope, must NOT be throttled by user A's failure + UserNamePasswordParameters paramsB = getUserNamePasswordApiParameters("userB@contoso.com", "scope1"); + RequestContext contextB = new RequestContext(app, PublicApi.ACQUIRE_TOKEN_BY_USERNAME_PASSWORD, paramsB, + UserIdentifier.fromUpn("userB@contoso.com")); + + assertDoesNotThrow(() -> httpHelper.executeHttpRequest(httpRequest, contextB, serviceBundle)); + } + + // Companion to the 500 test above: verifies the response-type-aware design. A 429 (service-directed + // rate limiting) must remain app-wide, so a different user of the same clientId/authority/scope IS + // throttled by another user's 429. This guards against over-narrowing throttling to per-user for + // triggers that are meant to back off the whole client. + @Test + void UserNamePassword_429ThrottlesDifferentUsersAppWide() throws Exception { + skipInvocationCountCheck = true; + ThrottlingCache.clear(); + ThrottlingCache.DEFAULT_THROTTLING_TIME_SEC = 1000; + + PublicClientApplication app = getPublicClientApp(); + + IHttpClient localHttpClientMock = mock(IHttpClient.class); + HttpResponse http429 = new HttpResponse(); + http429.statusCode(HttpStatus.HTTP_TOO_MANY_REQUESTS); + http429.body(TestConfiguration.TOKEN_ENDPOINT_INVALID_GRANT_ERROR_RESPONSE); + http429.addHeaders(Collections.singletonMap("Content-Type", Collections.singletonList("application/json"))); + doReturn(http429).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"); + + UserNamePasswordParameters paramsA = getUserNamePasswordApiParameters("userA@contoso.com", "scope1"); + RequestContext contextA = new RequestContext(app, PublicApi.ACQUIRE_TOKEN_BY_USERNAME_PASSWORD, paramsA, + UserIdentifier.fromUpn("userA@contoso.com")); + + // user A's request gets a 429 -> cached as an app-wide throttled request + httpHelper.executeHttpRequest(httpRequest, contextA, serviceBundle); + + // user B, same clientId/authority/scope, MUST also be throttled because 429 is app-wide + UserNamePasswordParameters paramsB = getUserNamePasswordApiParameters("userB@contoso.com", "scope1"); + RequestContext contextB = new RequestContext(app, PublicApi.ACQUIRE_TOKEN_BY_USERNAME_PASSWORD, paramsB, + UserIdentifier.fromUpn("userB@contoso.com")); + + assertThrows(MsalThrottlingException.class, + () -> httpHelper.executeHttpRequest(httpRequest, contextB, serviceBundle)); + } + + // A 5xx that carries a Retry-After header must still be scoped per-user: the status class (5xx) + // decides the scope, while the Retry-After header only sets the duration. This guards against a + // regression of issue #1019 for a federated STS that returns a user-specific 5xx together with a + // Retry-After header. + @Test + void UserNamePassword_500WithRetryAfterThrottlesUsersIndependently() throws Exception { + skipInvocationCountCheck = true; + ThrottlingCache.clear(); + ThrottlingCache.DEFAULT_THROTTLING_TIME_SEC = 1000; + + 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); + Map> headers = new HashMap<>(); + headers.put("Content-Type", Collections.singletonList("application/json")); + headers.put("Retry-After", Collections.singletonList("1000")); + http500.addHeaders(headers); + 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"); + + UserNamePasswordParameters paramsA = getUserNamePasswordApiParameters("userA@contoso.com", "scope1"); + RequestContext contextA = new RequestContext(app, PublicApi.ACQUIRE_TOKEN_BY_USERNAME_PASSWORD, paramsA, + UserIdentifier.fromUpn("userA@contoso.com")); + + // user A's request fails with a 500 + Retry-After -> gets cached as a throttled request + httpHelper.executeHttpRequest(httpRequest, contextA, serviceBundle); + // repeating user A's request should be throttled + assertThrows(MsalThrottlingException.class, + () -> httpHelper.executeHttpRequest(httpRequest, contextA, serviceBundle)); + + // user B, same clientId/authority/scope, must NOT be throttled by user A's 500 + Retry-After + UserNamePasswordParameters paramsB = getUserNamePasswordApiParameters("userB@contoso.com", "scope1"); + RequestContext contextB = new RequestContext(app, PublicApi.ACQUIRE_TOKEN_BY_USERNAME_PASSWORD, paramsB, + UserIdentifier.fromUpn("userB@contoso.com")); + + assertDoesNotThrow(() -> httpHelper.executeHttpRequest(httpRequest, contextB, serviceBundle)); + } }