Sagonzal/exceptions by sangonzal · Pull Request #69 · AzureAD/microsoft-authentication-library-for-java

sangonzal · 2019-06-25T20:13:43Z

Split AuthenticationException into AuthenticationClientException and AuthenticationServiceException
Expose http headers in AuthenticationServiceException- AuthenticationException should return 429 headers information #59
Expose suberror code in AuthenticationServiceException (invalid_grant and interaction_required classification) - Improve CA Error Handling #61
Remove ClaimsChallengeException.
- developers should instead check if AuthenticationServiceException contains claims (aligned with .NET)

Open question:
Should we rename:

AuthenticationException -> MsalException
AuthenticationServiceException -> MsalServiceException
AuthenticationClientException -> MsalClientException

SomkaPe · 2019-07-01T20:32:04Z

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


    /**
-     * claims challenge value
+     * Initializes a new instance of the exception class with a specified error message


probably not "error message" but instance of Throwable #Closed

SomkaPe · 2019-07-01T20:38:11Z

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

+    private String statusMessage;
+
+    /**
+     * An ID that can used to piece up a single authentication flow.


"can be used" probably #Closed

+1 : Seems like a missing word as Petro indicates

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

SomkaPe · 2019-07-01T20:46:01Z

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

+        Map<String, List<String>> headers = conn.getHeaderFields();
+        for(Map.Entry<String, List<String>> header: headers.entrySet()){
+
+            if(header.getKey() == null || StringHelper.isBlank(header.getKey())){


check for null is included into StringHelper.isBlank #Closed

sangonzal · 2019-07-02T17:28:29Z

@SomkaPe @henrik-me any opinions on the naming of the exceptions?

henrik-me · 2019-07-04T04:46:07Z

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

+            Assert.fail("Expected AuthenticationServiceException was not thrown");
+        } catch (AuthenticationServiceException ex) {
+            Assert.assertEquals(claims.replace("\\", ""), ex.claims());
+            Assert.assertEquals(ex.subError(), "");


Assert.assertEquals(ex.subError(), ""); [](start = 12, length = 39)

I dont' see any tests really validating the sub error. Also the subError is not what should be exposed, the exposed property is Classification and it will only be a subset of the suberrors. Please work with Bogdan on these details. #Closed

henrik-me

🕐

henrik-me · 2019-07-04T04:48:39Z

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

+ * Exception type thrown when service returns an error response or other networking errors occur.
+ */
+@Accessors(fluent = true)
+@Getter


Getter [](start = 1, length = 6)

subError is supposed to stay private and a subset of errors should be exposed via the Classification property #Closed

henrik-me · 2019-07-04T04:49:23Z

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

+ */
+@Accessors(fluent = true)
+@Getter
+public class AuthenticationServiceException extends AuthenticationException{


AuthenticationServiceException [](start = 13, length = 30)

Some of the errors should be categorized as client side / UiRequiredExceptions and not ServiceExceptions. #Closed

Yes, namely "invalid_grant" service exceptions are exposed as UiRequiredServiceExceptions. Currently MSAL.NET treats all "invalid_grant" exceptions as UiRequiredExceptions, but I have a PR to filter some of these out (e.g. suberror "client_mismatch", which is a FOCI related suberror, is not a UiRequiredException). #Closed

Consider having a factory that decides which type of exception needs to be thrown - ServiceException or UiRequiredException. #Closed

@bgavrilMS I believe the service also returns "interaction_required". Are you treating these as UiRequiredExceptions as well? #Closed

@sangonzal - yes, sorry, let me clarify. There are 2 cases where MSAL gets an error from the server and throws UiRequiredException:

Whenever it interacts with Evo and Evo responds with error code invalid_grant (this is where I suggest to use a factory - i.e. an object is responsible for transforming Evo errors into Service or UiRequired)

In the authorization flow, when the auth code url contains the param error=login_required

The MSAL library also throws UiRequiredException based on client-side logic, as follows:

in AcquireTokenSilent if the account / login_hint is null, or if a login_hint is used but no matching Account / too many Accounts are in the cache for that hint.

If no cache is configured

Some web UIs also throw this (should not apply to you, the system browser ui doesn't throw this) #Closed

For the Authorization flow, that error will be sent when either:

Prompt.Never was passsed, but interaction is actually required

An error occurs during silent auth that prevented the authentication to complete in time. #Closed

bgavrilMS · 2019-07-04T10:44:12Z

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

-    public String toString(){
-        return errorCode;
-    }
+    public final static String AUTHORIZATION_PENDING = "authorization_pending";


Consider providing comments on these strings, in a format similar to "What Happens?" and "Mitigation". #Closed

bgavrilMS · 2019-07-04T10:48:43Z

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

+    /**
+     * More specific error
+     */
+    private String subError;


As Henrik mentioned, we don't expose this because Evo does not guarantee that these sub errors will remain unchanged. #Closed

bgavrilMS · 2019-07-04T10:55:01Z

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

-                            errorObject.getCode();
+                // Some invalid_grant or interaction_required subError codes returned by
+                // the service are not supposed to be exposed to customers
+                if(errorResponse.error() != null &&


The TokenRequest class does too much, consider moving this logic to another object, for example the ServiceException factory can take on this responsability. #Closed

bgavrilMS · 2019-07-04T10:56:43Z

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

        }
    }

+    private ErrorResponse filterSubErrorCode(ErrorResponse errorResponse){


The spec I've used to do this work mentioned having a "Classification" property that exposes some of the sub-errors. We don't expose sub_error in general.

Also, for discoverability, the Classification property will return an enum instead of a string constant. #Closed

bgavrilMS

Main issues are around:

separate UiRequiredException, which is a subclass of ServiceException
do not expose sub_error
expose a classification getter on UiRequiredException (enum or string, I'd prefer enum) that encapsulates that filtering logic you already wrote.
allow list instead of block list (i.e. if sub_error is X, Y or Z, classification is A, B or C instead of if sub_error is NOT X...). I've discussed this at length with Henrik and we'd rather take responsability to curate new suberrors as they appear (i.e. add new mappings).

sangonzal · 2019-07-10T00:13:29Z

@bgavrilMS Thanks for all of the detailed feedback. I have addressed all of the main issues you have raised, and have aligned this implementation with the one in .NET. I have a couple of questions which I will follow up with offline. #Closed

bgavrilMS

LGTM, ping me to discuss further

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

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

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

henrik-me

henrik-me · 2019-07-11T05:10:09Z

one naming question/suggestion for the enum which should be thought of before closing

henrik-me · 2019-07-11T05:12:29Z

Not sure which exceptions. IMO, I like calling it InteractionRequired and not UIInteractionRequired, though this may be misleading as some of them we should really show them the system browser... otherwise using names that are used in other libraries makes the most sense to me for the exception naming (seems like you are generally following that pattern)

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

sgonzalezMSFT added 3 commits June 25, 2019 12:37

Expose headers and subErrors in exceptions

8d8c094

Merge branch 'dev' into sagonzal/exposeHttp429

ce5708c

Add misssing license headers

9405bdc

sangonzal requested review from SomkaPe and henrik-me June 25, 2019 20:13

SomkaPe reviewed Jul 1, 2019

View reviewed changes

SomkaPe approved these changes Jul 1, 2019

View reviewed changes

henrik-me reviewed Jul 4, 2019

View reviewed changes

henrik-me requested a review from bgavrilMS July 4, 2019 04:46

henrik-me requested changes Jul 4, 2019

View reviewed changes

henrik-me reviewed Jul 4, 2019

View reviewed changes

bgavrilMS reviewed Jul 4, 2019

View reviewed changes

bgavrilMS requested changes Jul 4, 2019

View reviewed changes

sgonzalezMSFT added 2 commits July 9, 2019 17:03

Rename exceptions. Add MsalInteractionRequiredException.

40508fd

Add comments to MsalInteractionRequiredException

35c7468

bgavrilMS approved these changes Jul 10, 2019

View reviewed changes

Santiago Gonzalez added 2 commits July 10, 2019 14:01

Merge branch 'dev' into sagonzal/exposeHttp429

a414ef0

Fix minor bug

974f555

henrik-me reviewed Jul 11, 2019

View reviewed changes

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

henrik-me reviewed Jul 11, 2019

View reviewed changes

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

henrik-me reviewed Jul 11, 2019

View reviewed changes

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

henrik-me approved these changes Jul 11, 2019

View reviewed changes

Pr Feedback

e6d5ea5

sangonzal merged commit 01498d5 into dev Jul 11, 2019

sangonzal deleted the sagonzal/exposeHttp429 branch July 12, 2019 17:54

Conversation

sangonzal commented Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SomkaPe Jul 1, 2019 • edited by henrik-me Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SomkaPe Jul 1, 2019 • edited by henrik-me Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henrik-me Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

SomkaPe Jul 1, 2019 • edited by henrik-me Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sangonzal commented Jul 2, 2019

Uh oh!

henrik-me Jul 4, 2019 • 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!

henrik-me left a comment

Choose a reason for hiding this comment

Uh oh!

henrik-me Jul 4, 2019 • 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!

henrik-me Jul 4, 2019 • 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!

bgavrilMS Jul 4, 2019 • edited by henrik-me Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgavrilMS Jul 4, 2019 • edited by henrik-me Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sangonzal Jul 8, 2019 • edited by henrik-me Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgavrilMS Jul 9, 2019 • edited by henrik-me Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgavrilMS Jul 9, 2019 • edited by henrik-me Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgavrilMS Jul 4, 2019 • edited by henrik-me Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgavrilMS Jul 4, 2019 • edited by henrik-me Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgavrilMS Jul 4, 2019 • edited by henrik-me Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgavrilMS Jul 4, 2019 • edited by henrik-me Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgavrilMS left a comment

Choose a reason for hiding this comment

Uh oh!

sangonzal commented Jul 10, 2019 • edited by henrik-me Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bgavrilMS left a comment

Choose a reason for hiding this comment

sangonzal commented Jun 25, 2019 •

edited

Loading

SomkaPe Jul 1, 2019 •

edited by henrik-me

Loading

SomkaPe Jul 1, 2019 •

edited by henrik-me

Loading

SomkaPe Jul 1, 2019 •

edited by henrik-me

Loading

henrik-me Jul 4, 2019 •

edited

Loading

henrik-me Jul 4, 2019 •

edited

Loading

henrik-me Jul 4, 2019 •

edited

Loading

bgavrilMS Jul 4, 2019 •

edited by henrik-me

Loading

bgavrilMS Jul 4, 2019 •

edited by henrik-me

Loading

sangonzal Jul 8, 2019 •

edited by henrik-me

Loading

bgavrilMS Jul 9, 2019 •

edited by henrik-me

Loading

bgavrilMS Jul 9, 2019 •

edited by henrik-me

Loading

bgavrilMS Jul 4, 2019 •

edited by henrik-me

Loading

bgavrilMS Jul 4, 2019 •

edited by henrik-me

Loading

bgavrilMS Jul 4, 2019 •

edited by henrik-me

Loading

bgavrilMS Jul 4, 2019 •

edited by henrik-me

Loading

sangonzal commented Jul 10, 2019 •

edited by henrik-me

Loading

henrik-me commented Jul 11, 2019 •

edited

Loading