bugfix #1019 - wrong throttling on clientid. - #1050
bugfix #1019 - wrong throttling on clientid.#1050Maximilian Pfeffer (gterminator) wants to merge 1 commit into
Conversation
|
Thanks for making this fix Maximilian Pfeffer (@gterminator)! Overall it looks good and we'll likely be merging it in, however first we're going to do some investigation into the other MSALs (.NET, Python, Go, Node) to make sure 1) this behavior is consistent with our other SDKs and 2) the other SDKs don't have the same issue/need the same fix. I don't have an ETA, but this should get into the next MSAL Java release. |
| @@ -125,18 +125,19 @@ IHttpResponse executeHttpRequest(HttpRequest httpRequest) { | |||
|
|
|||
| private String getRequestThumbprint(RequestContext requestContext) { | |||
There was a problem hiding this comment.
Adding per-user throttling should help with the issues mentioned in #1019, and avoid a user's bad password or 500 errors from blocking other users.
However, I believe this getRequestThumbprint is used for all throttling scenarios in public client flows, not just user-specific ones. If an app is getting 429 errors then all users should be throttled, however with these changes each individual user will need to receive a 429 error to be added to the throttling cache.
In short, this will relieve user-specific throttling issues, but may exacerbate app-wide throttling issues.
There was a problem hiding this comment.
Adding per-user throttling should help with the issues mentioned in #1019, and avoid a user's bad password or 500 errors from blocking other users.
However, I believe this
getRequestThumbprintis used for all throttling scenarios in public client flows, not just user-specific ones. If an app is getting 429 errors then all users should be throttled, however with these changes each individual user will need to receive a 429 error to be added to the throttling cache.In short, this will relieve user-specific throttling issues, but may exacerbate app-wide throttling issues.
good point. I will rework this PR.
There was a problem hiding this comment.
I actually just went and created an updated version in MSAL Java, and two other MSALs that seemed to have a similar issue:
#1055
AzureAD/microsoft-authentication-library-for-dotnet#6159
AzureAD/microsoft-authentication-library-for-js#8756
I created the separate Java PR to fix that issue and add some extra tests as I do some more thorough checks in each of the SDKs, and allow it to run on our pipelines just to make sure there aren't any unexpected side effects.
There was a problem hiding this comment.
I actually just went and created an updated version in MSAL Java, and two other MSALs that seemed to have a similar issue: #1055 AzureAD/microsoft-authentication-library-for-dotnet#6159 AzureAD/microsoft-authentication-library-for-js#8756
I created the separate Java PR to fix that issue and add some extra tests as I do some more thorough checks in each of the SDKs, and allow it to run on our pipelines just to make sure there aren't any unexpected side effects.
perfect then i will close this pr.
2 participants
Hi,
here is my fix to #1019.
This fix is depending on the (existing) HttpHelper Class.
Which i refactored on another PR #935.
fixes #1019
open for feedback.