Scope error-class throttling per-user for public-client flows - #1055
Scope error-class throttling per-user for public-client flows#1055Avery-Dunn wants to merge 1 commit into
Conversation
Avery-Dunn
changed the title
|
Avery-Dunn I'm not sure about this change. What happens when there is some gateway timeout? Or other 5xx messages? I mean we have to check for the user case that we have a 5xx AND a retry in the header, otherwise we could get some delays in that we dont want e.g. on 502 Bad Gateway for example. Btw. What happens when we retry? Do we consume the exception and automatically retry? |
| if (includeUser) { | ||
| UserIdentifier userIdentifier = requestContext.userIdentifier(); | ||
| if (userIdentifier != null) { | ||
| if (!StringHelper.isBlank(userIdentifier.upn())) { |
There was a problem hiding this comment.
nit: we should generally not use upn as discriminator. Despite the name, it's not unique. Consider using oid and then falling back to upn.
There was a problem hiding this comment.
Pull request overview
This PR adjusts MSAL4J’s public-client request throttling so that HTTP 5xx “error-class” throttling is keyed per user, while HTTP 429 / explicit Retry-After throttling remains app-wide, preventing one user’s failures (e.g., bad password causing ADFS 500) from throttling other users sharing the same clientId/authority/scope.
Changes:
- Introduces app-wide vs user-aware throttle thumbprints and updates throttling read/write logic accordingly.
- Updates throttling checks to consult the app-wide key first, then the user-aware key when different.
- Adds regression and isolation tests for per-user 5xx throttling and app-wide 429 throttling.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/HttpHelper.java |
Adds user-aware thumbprint support and routes throttling behavior based on response type (5xx vs 429/Retry-After). |
msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/RequestThrottlingTest.java |
Adds regression tests ensuring 5xx throttling is per-user while 429 remains app-wide. |
| // 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, true); | ||
| if (!userAwareThumbprint.equals(appWideThumbprint)) { | ||
| retryInMs = ThrottlingCache.retryInMs(userAwareThumbprint); | ||
| } | ||
| } |
| @Test | ||
| void STSResponseContains_StatusCode500_DifferentUsersNotThrottledForEachOther() throws Exception { | ||
| skipInvocationCountCheck = true; | ||
| ThrottlingCache.clear(); | ||
|
|
|
Avery-Dunn can we maybe pin this special behavior to a special status code? I want to avoid that you get DDOS-ed when some Auth Server is on bad gateway or has a error. |
Fixes #1019, using the core fix made by Maximilian Pfeffer (@gterminator) in #1050 but avoiding an issue in 429 throttling scenarios, adds a couple extra tests, and allowing it to be run in our CI pipelines.
See:
AzureAD/microsoft-authentication-library-for-js#8756
AzureAD/microsoft-authentication-library-for-dotnet#6159
Problem
When a public-client app acquires tokens for multiple users under the same
clientId/ authority / scope, a failed request for one user could throttle every other user. Reported symptoms in #1019:MsalThrottlingException.Root cause
HttpHelperthrottles public-client requests by a "request thumbprint". Before this change the thumbprint was derived from clientId + authority + scope only — it did not include any user component for flows like Username/Password (ROPC), where there is noAccountyet. As a result two different users produced a byte-identical throttle key, so a 500 cached for user A immediately throttled user B.Fix
Make the throttle key response-type-aware:
Retry-After(service-directed rate limiting for the whole client) → key remains app-wide (no user component), preserving correct global back-off.checkForThrottlingnow checks the app-wide key first, then the user-aware key when it differs;processThrottlingInstructionswrites under the app-wide key for 429/Retry-After and under the user-aware key for 5xx.Files changed
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/HttpHelper.javagetRequestThumbprint(RequestContext, boolean includeUser)overload.checkForThrottlingchecks both app-wide and user-aware keys.processThrottlingInstructionsroutes 5xx → user-aware, 429/Retry-After → app-wide.Behavior change
Retry-AfterTests
msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/RequestThrottlingTest.java:UserNamePassword_DifferentUsersThrottledIndependently— user A's 500 throttles A but not user B (regression test for [Bug] Throttling cache affects whole clientId when user provides wrong password in ADFS federation #1019).UserNamePassword_429ThrottlesDifferentUsersAppWide— a 429 still throttles a different user, proving 429 stays app-wide (guards against over-narrowing).STSResponseContains_StatusCode500_DifferentUsersNotThrottledForEachOtherandSilentFlow_DifferentAccountsThrottledIndependently— end-to-end and silent-flow isolation.RETRY_AFTER_HEADER,429,500, combinations) continue to pass unchanged.