support of STS throttling instructions + caching of InteractionRequiredException… #201

SomkaPe · 2020-03-28T05:06:58Z

support of STS throttling instructions - 429 Retry-After header + 429, 5xx response status codes
caching of InteractionRequiredException responses for refresh token grant requests
refactoring

issue #176

…edException responses

sangonzal

Looks good. Just a couple of questions.

src/main/java/com/microsoft/aad/msal4j/HttpHelper.java

sangonzal · 2020-04-02T17:28:47Z

src/main/java/com/microsoft/aad/msal4j/MsalServiceExceptionFactory.java

@@ -30,25 +30,26 @@ static MsalServiceException fromHttpResponse(HTTPResponse httpResponse){
        errorResponse.statusCode(httpResponse.getStatusCode());
        errorResponse.statusMessage(httpResponse.getStatusMessage());

-        if(errorResponse.error() != null &&
+        if (isRefreshTokenGrantRequest &&


I don't know if it makes sense to just limit throwing InteractionRequiredError for just RefreshTokenGrant. What about in the case of username password and integrated windows authentication? interaction required is just as relevant in those cases as it is for refresh flow.

both cases are "interactive", if it fails It means that provided credentials are wrong, there is no plan B - plan B exists only for silent flow

Not sure I would consider IWA and U/P interactive. To me it seems that an interactive request is one where the IDP renders UI, which is not the case for either of those.

Also IWA and U/P can fail for more than just providing wrong credentials - one example is MFA. In this case, someone doing U/P could theoretically fall back to doing an interactive flow to meet the MFA requirement. I agree that it probably does not make sense to do this in an application, but anyways the InteractionRequired error is still 100% valid and the most transparent error to expose to an user. In the case that we made the change you are recommending, and someone is using one of this flows with MFA enabled on the tenant, what kind of error would they see?

interactive vs non-interactive/silent distinguishing does make sense only for flows where user perform authorization "interactively" - like auth code flow - user redirected to identity provider authorization endpoint and he interact with it - so identity provider can give him additional challenges and he can satisfy it. In case of U/P or WIA - if credentials are wrong or missing there is not much we can do, we do not have plan B (different api) to handle it, like we do with auth code flow

This is a great point @sangonzal,
I have seen many client applications implementing flow as:

try AcquireTokenSilent

catch -- try IWA

catch -- use Interactive

In fact, the spec states

`... for silent flow some errors of WS-Trust flow should go in UI required cache as well as errors from token endpoint. Applications have tendency to loop on WS-Trust flow, which would result in DDoS of UserRealm endpoint. There is no way such cases for the application to proceed without showing UI. Here is the list of these errors:

account is managed/unknown,
integration is not WS-Trust (SAML2.0)
missing MEX endpoint,
invalid MEX document,
MEX doesn’t have Windows Transport Endpoint,
failed to get SAML token from ADFS,
invalid_grant,
interaction_required.`

throttling design also explicitly highlight that scope of responses caching is silent requests.

"it is about silent mode, only one type of requests can be sent in silent mode - refresh token grant" I don't agree with you here. The most meaningful difference IMO between "silent mode" and "non-silent mode" is showing UI. From that perspective the other flows are silent as well.

InteractionRequired is a valid error to expose for the other flows. What would we expose to users in the case of MFA for example (which is relevant in all flows)? InteractionRequired (pulling up UI and having someone click on it) is the way to resolve that issue, and the most transparent exception to expose to users, whether it's actionable or not for them.

From the perspective of throttling, wouldn't we also want to throttle applications stuck in a loop in ROPC or IWA flows if they are getting interaction required returned from the server?

One more thing to consider is that changing this behavior (only throwing InteractionRequired for refresh token grant) is a breaking change, and would require bumping up the major version.

"InteractionRequired is a valid error to expose for the other flows." - even for non interactive flows ?

removing redefinition of InteractionRequired Exception from the scope of this PR

src/main/java/com/microsoft/aad/msal4j/RefreshTokenRequest.java

src/main/java/com/microsoft/aad/msal4j/SilentRequest.java

src/main/java/com/microsoft/aad/msal4j/StringHelper.java

henrik-me · 2020-04-07T21:36:40Z

@bgavrilMS @rayluo : please add comments and discuss offline as needed.

src/main/java/com/microsoft/aad/msal4j/HttpHeaders.java

src/main/java/com/microsoft/aad/msal4j/HttpHelper.java

bgavrilMS · 2020-04-10T10:05:10Z

src/main/java/com/microsoft/aad/msal4j/HttpHelper.java

+        return StringHelper.createSha256Hash(sb.toString());
+    }
+
+    static private boolean isRetriable(IHttpResponse httpResponse) {


nit: typo retryable

ok, will rename it, was not sure what is correct spelling

bgavrilMS · 2020-04-10T10:09:12Z

src/main/java/com/microsoft/aad/msal4j/HttpHelper.java

+            long retryInMs = ThrottlingCache.retryInMs(requestThumbprint);
+
+            if (retryInMs > 0) {
+                throw new MsalThrottlingException(retryInMs);


I was under the impression that throttling concept is not exposed to client. We should just throw the old exception, as if the client had made the HTTP call and the server returned the same error.

I think this makes sense because it would behave well with retry policies that customers have added on their own.

Not sure about it, if we get instructions from STS to throttle some requests, we do it and do it transparently for app developers.

I sent an email to Sasha for clarifications as I am also not sure about this.

src/main/java/com/microsoft/aad/msal4j/HttpHelper.java

bgavrilMS · 2020-04-10T10:21:16Z

src/main/java/com/microsoft/aad/msal4j/MsalThrottlingException.java

+ */
+@Accessors(fluent = true)
+@Getter
+public class MsalThrottlingException extends MsalServiceException {


As stated above, ideally we wouldn't expose a new type of exception, but just rethrow an old MsalServiceExcpetion.

not sure that covering up what really happened is a good idea

src/test/java/com/microsoft/aad/msal4j/UIRequiredCacheTest.java

bgavrilMS

Generally looks good, clean implementation. Things to discuss:

not exposing throttling concept to end users (i.e. no new exception)
UI required exception caching should cover IWA and ROPC as well

bgavrilMS · 2020-04-17T09:08:01Z

src/main/java/com/microsoft/aad/msal4j/HttpHelper.java

+            headers.putAll(httpResponse.headers());
+
+            if (headers.containsKey(RETRY_AFTER_HEADER) && headers.get(RETRY_AFTER_HEADER).size() == 1) {
+                int headerValue = Integer.parseInt(headers.get(RETRY_AFTER_HEADER).get(0));


I was looking at the spec for Retry-After and it looks like it can also be a timestamp https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After

Consider using a library call that can parse this.

good point, but according to design doc we server side are going to use only delta-seconds format

src/main/java/com/microsoft/aad/msal4j/HttpHelper.java

bgavrilMS · 2020-04-17T17:24:27Z

src/main/java/com/microsoft/aad/msal4j/RefreshTokenRequest.java


        RefreshTokenGrant refreshTokenGrant = new RefreshTokenGrant(new RefreshToken(parameters.refreshToken()));
        return new OAuthAuthorizationGrant(refreshTokenGrant, parameters.scopes());
    }
+
+    private String getRefreshTokenRequestFullThumbprint() {
+        StringBuilder sb = new StringBuilder();


As per spec, this should include all possible inputs except the refresh token. Might be better to do it based on the form body params...

under request parameters I mean set of parameters passed to corresponding api - silent api or refresh token grant api

bgavrilMS · 2020-04-20T09:06:54Z

src/main/java/com/microsoft/aad/msal4j/HttpHelper.java

+
+        IApiParameters apiParameters = requestContext.apiParameters();
+
+        if (apiParameters instanceof SilentParameters) {


AcquireTokenSilent exists both on Public and Confidential clients, but we should only throttle Public client as per spec. Not sure if it's ok to throttle Confiential clients so aggressively. Maybe it is ...

I don't have all the context on how MSAL.java is organized, so maybe this comment isn't relevant.

check for public client happens in checkForThrottling and processThrottlingInstructions functions

bgavrilMS · 2020-04-21T15:17:01Z

src/test/java/com/microsoft/aad/msal4j/RequestThrottlingTest.java

+    }
+
+    @Test
+    public void STSResponseContainsStatusCode500_repeatedRequestsThrottled() throws Exception  {


Sasha wrote 21 acceptance tests. I was thinking it is too much, but there are subtle differences in each one. It's also a guarantee that libraries behave the same.

Please ensure the relevant acceptance tests are included here as that is the best way we have to ensure the code behaves as expected.

In reply to: 412276923 [](ancestors = 412276923)

Most of mentioned tests duplicated, and some not applicable for java.
Taking to account differences in api model of Msal and WAM we consider only refresh tokens grant requests as subject to UIRequired cache

bgavrilMS · 2020-04-21T16:32:26Z

src/main/java/com/microsoft/aad/msal4j/HttpHelper.java

+    }
+
+    static private boolean isRetriable(IHttpResponse httpResponse) {
+        return httpResponse.statusCode() >= 500 && httpResponse.statusCode() < 600;


I think we should also not retry if a Retry-After header is present. I just realised this implementing some of Sasha's tests.

good point - going to change it.

bgavrilMS

Looks good, I think we are on the same page after offline discussion

…edException… (#201) support of server side throttling instructions + caching of InteractionRequiredException responses

support of STS throttling instructions + caching of InteractionRequir…

730a7c4

…edException responses

SomkaPe requested review from sangonzal, henrik-me and Avery-Dunn March 28, 2020 05:07

sangonzal reviewed Apr 2, 2020

View reviewed changes

refactoring

d6273ba

henrik-me requested review from bgavrilMS and rayluo April 7, 2020 21:35

bgavrilMS reviewed Apr 10, 2020

View reviewed changes

src/main/java/com/microsoft/aad/msal4j/HttpHeaders.java Show resolved Hide resolved

bgavrilMS reviewed Apr 10, 2020

View reviewed changes

src/main/java/com/microsoft/aad/msal4j/HttpHelper.java Show resolved Hide resolved

bgavrilMS reviewed Apr 10, 2020

View reviewed changes

src/main/java/com/microsoft/aad/msal4j/HttpHelper.java Outdated Show resolved Hide resolved

bgavrilMS reviewed Apr 10, 2020

View reviewed changes

src/test/java/com/microsoft/aad/msal4j/UIRequiredCacheTest.java Outdated Show resolved Hide resolved

bgavrilMS requested changes Apr 10, 2020

View reviewed changes

bgavrilMS reviewed Apr 17, 2020

View reviewed changes

src/main/java/com/microsoft/aad/msal4j/HttpHelper.java Show resolved Hide resolved

bgavrilMS reviewed Apr 17, 2020

View reviewed changes

bgavrilMS reviewed Apr 20, 2020

View reviewed changes

bgavrilMS reviewed Apr 21, 2020

View reviewed changes

addressing pr comments

7cdde3c

bgavrilMS reviewed Apr 29, 2020

View reviewed changes

SomkaPe added 2 commits April 30, 2020 02:11

adding more tests

ef50844

Merge branch 'dev' into pesomka/throttling

c939a46

SomkaPe merged commit 8a79bc8 into dev May 5, 2020

SomkaPe added a commit that referenced this pull request May 5, 2020

support of STS throttling instructions + caching of InteractionRequir…

ecca2da

…edException… (#201) support of server side throttling instructions + caching of InteractionRequiredException responses

siddhijain deleted the pesomka/throttling branch January 19, 2022 18:27


		IApiParameters apiParameters = requestContext.apiParameters();

		if (apiParameters instanceof SilentParameters) {

support of STS throttling instructions + caching of InteractionRequiredException… #201

support of STS throttling instructions + caching of InteractionRequiredException… #201

Uh oh!

Conversation

SomkaPe commented Mar 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sangonzal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgavrilMS Apr 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sangonzal Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

henrik-me commented Apr 7, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SomkaPe Apr 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SomkaPe Apr 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SomkaPe Apr 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bgavrilMS left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SomkaPe commented Mar 28, 2020 •

edited

Loading

bgavrilMS Apr 10, 2020 •

edited

Loading

sangonzal Apr 13, 2020 •

edited

Loading

SomkaPe Apr 29, 2020 •

edited

Loading

SomkaPe Apr 10, 2020 •

edited

Loading

SomkaPe Apr 10, 2020 •

edited

Loading

bgavrilMS Apr 21, 2020 •

edited

Loading