Error code & other related tasks refactoring #153

unpluggedk · 2018-06-18T19:41:21Z

Removed not-being-used errors, feel free to re-introduce them when needed.

Categorized errors into

General errors
Cache Errors
Server errors
Interactive flow errors.

Other additions:

include additional checks to disable system webview from project
minor fixes

oldalton · 2018-06-18T23:27:38Z

IdentityCore/src/MSIDError.h

+    // Server returned a refresh token reject response
+    MSIDErrorServerRefreshTokenRejected = -51403,
+    // Other specific server response errors
+    MSIDErrorInvalidRequest             = -51404,


nit: should it also have "server" in it?

oldalton · 2018-06-18T23:28:23Z

IdentityCore/src/MSIDError.h

-     */
-    MSIDErrorSessionCanceled = -51020,
+    // The interactive flow was cancelled programmatically.
+    MSIDErrorSessionCanceled            = -51513,


nit: can the error name "hint" that it was canceled programmatically?

oldalton · 2018-06-18T23:29:38Z

IdentityCore/src/oauth2/MSIDTokenResponse.m

@@ -101,7 +101,7 @@ - (MSIDAccountType)accountType

 - (MSIDErrorCode)oauthErrorCode
 {
-    return MSIDErrorCodeForOAuthError(self.error, MSIDErrorInteractionRequired);
+    return MSIDErrorCodeForOAuthError(self.error, MSIDErrorServerOauth);


will the clients know to show UI on MSIDErrorServerOauth? I think the reason to have MSIDErrorInteractionRequired as default is so that client show UI in MSAL? Can we check what was MSAL and ADAL implementation before?

Top level : Interaction required
Underlying : ServerOAuth

and double check converter in MSAL and ADAL

oldalton · 2018-06-18T23:29:51Z

IdentityCore/src/webview/response/MSIDWebOAuth2Response.m

@@ -45,7 +45,10 @@ - (instancetype)initWithURL:(NSURL *)url
        {
            if (error)
            {
-                *error = MSIDCreateError(MSIDOAuthErrorDomain, MSIDErrorInvalidParameter, @"Unexpected error has occured. There is no auth code nor an error", nil, nil, nil, context.correlationId, nil);
+                *error = MSIDCreateError(MSIDOAuthErrorDomain,
+                                         MSIDErrorServerInvalidResponse,


…tion-library-common-for-objc into jak/error-coding

included.

…ft-authentication-library-common-for-objc into jak/additional-integration-fixes

ADAL integration.

unpluggedk · 2018-06-26T01:21:37Z

AzureAD/azure-activedirectory-library-for-objc#1213

AzureAD/microsoft-authentication-library-for-objc#283

updated in ADAL and MSAL respectively in their PRs.

oldalton

Just a couple of questions and nit fixes :)

oldalton · 2018-06-26T16:58:22Z

IdentityCore/IdentityCore.xcodeproj/project.pbxproj

@@ -134,7 +134,6 @@
 		6035CD8D207EA67300369E69 /* MSIDTelemetryIntegrationTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 6035CD8B207EA67300369E69 /* MSIDTelemetryIntegrationTests.m */; };
 		6057EE9020B5FDF8007976EB /* MSIDAADOAuthEmbeddedWebviewController.m in Sources */ = {isa = PBXBuildFile; fileRef = 6057EE8F20B5FDF8007976EB /* MSIDAADOAuthEmbeddedWebviewController.m */; };
 		6057EE9120B5FDF8007976EB /* MSIDAADOAuthEmbeddedWebviewController.m in Sources */ = {isa = PBXBuildFile; fileRef = 6057EE8F20B5FDF8007976EB /* MSIDAADOAuthEmbeddedWebviewController.m */; };
-		606830052098ACED00CCA6AB /* MSIDNegotiateHandler.m in Sources */ = {isa = PBXBuildFile; fileRef = 606830042098ACED00CCA6AB /* MSIDNegotiateHandler.m */; };


why this got removed?

It was removed for iOS. There is a separate issue for this in #157

oldalton · 2018-06-26T17:01:36Z

IdentityCore/src/MSIDError.m

+                      // Authority Validation Errors
+                      @(MSIDErrorAuthorityValidation),
+                      @(MSIDErrorAuthorityValidation),
+                      @(MSIDErrorAuthorityValidation),


MSIDErrorAuthorityValidation error is triple :)

woops, removed.

oldalton · 2018-06-26T17:04:13Z

IdentityCore/src/webview/MSIDWebviewAuthorization.m

@@ -83,7 +83,7 @@ + (void)startSession:(MSIDWebviewSession *)session
    // check session nil
    if (!session)
    {
-        NSError *error = MSIDCreateError(MSIDErrorDomain, MSIDErrorInvalidRequest, @"Interactive session failed to create.", nil, nil, nil, context.correlationId, nil);
+        NSError *error = MSIDCreateError(MSIDErrorDomain, MSIDErrorInvalidDeveloperParameter, @"Interactive session failed to create.", nil, nil, nil, context.correlationId, nil);


why is this developer parameter? This feels like it would be MSAL internal error or system error :)

It is an argument to the method itself. But I do understand where the argument is coming from - this method is only called by other methods that should create a session.
Thus, changing it to MSIDErrorInvalidInternalParameter.

oldalton · 2018-06-26T17:04:27Z

IdentityCore/src/webview/embeddedWebview/MSIDOAuth2EmbeddedWebviewController.m

@@ -114,7 +114,16 @@ - (void)cancel
    MSID_LOG_INFO(self.context, @"Cancel Web Auth...");

    // End web auth with error
-    NSError *error = MSIDCreateError(MSIDErrorDomain, MSIDErrorUserCancel, @"The user/application has cancelled the authorization.", nil, nil, nil, self.context.correlationId, nil);
+    NSError *error = MSIDCreateError(MSIDErrorDomain, MSIDErrorSessionCanceledProgramatically, @"Authorization session was cancelled programatically.", nil, nil, nil, self.context.correlationId, nil);


oldalton · 2018-06-26T17:07:42Z

IdentityCore/src/webview/embeddedWebview/ui/ios/MSIDWebviewUIController.m

    self.navigationItem.leftBarButtonItem = cancelButton;
 }

-// Authentication was cancelled by the user
- (IBAction)onCancel:(__unused id)sender


just confirming if this was changed also in the subclasses, because I didn't see onCancel changes in the subclasses :)

onCancel method is no longer needed. Button was set up programatically to simply call "userCancel" now. Some comments around methods were added for clarity nonetheless.

…tion-library-common-for-objc into jak/error-coding

revise error codes

59dedf2

unpluggedk requested review from antrix1989, jasoncoolmax and oldalton June 18, 2018 19:41

re-add codeFirst and last for boundary checks

5305f64

oldalton reviewed Jun 18, 2018

View reviewed changes

unpluggedk added 16 commits June 22, 2018 12:34

address feedback

1370486

Merge branch 'dev' of https://github.com/AzureAD/microsoft-authentica…

fb06c85

…tion-library-common-for-objc into jak/error-coding

update project file and error codes (MSAL tested)

1a4a89f

additional conditional checks for disabling system webview from

79f415f

included.

minor typo

26fcc80

additional conditionals to fix unit tests

d26da5a

add conditional checker for importing additional xcconfig

258ce69

separate authority validation failure from server error

f383235

Merge branch 'jak/error-coding' of https://github.com/AzureAD/microso…

abb22c7

…ft-authentication-library-common-for-objc into jak/additional-integration-fixes

separate embedded webview cancel to user cancel and app cancel

92ec263

report error at auth session cancellation programatically.

f14de0e

fix error to have right domain

d49dd2e

address more code refactoring

bbd6375

minor fix after MSAL integration

6b87210

rework numbers for easier addition in future, and minor adjustment after

3672f38

ADAL integration.

fix unit test

3117678

unpluggedk changed the title ~~Error code handling~~ Error code & other related tasks refactoring Jun 26, 2018

This was referenced Jun 26, 2018

Fix project - integrate fixed MSID AzureAD/microsoft-authentication-library-for-objc#283

Merged

Fix project - integrate fixed MSID AzureAD/azure-activedirectory-library-for-objc#1213

Merged

oldalton reviewed Jun 26, 2018

View reviewed changes

address comments

089ec9f

oldalton approved these changes Jun 26, 2018

View reviewed changes

Merge branch 'dev' of https://github.com/AzureAD/microsoft-authentica…

9acc5b8

…tion-library-common-for-objc into jak/error-coding

typo on MSIDErrorSessionCanceledProgramatically

330fcd8

unpluggedk merged commit a9c8443 into dev Jun 26, 2018

unpluggedk deleted the jak/error-coding branch June 26, 2018 23:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Error code & other related tasks refactoring #153

Error code & other related tasks refactoring #153

unpluggedk commented Jun 18, 2018 •

edited

Loading

oldalton Jun 18, 2018

unpluggedk Jun 22, 2018

oldalton Jun 18, 2018

unpluggedk Jun 22, 2018

oldalton Jun 18, 2018

unpluggedk Jun 18, 2018

oldalton Jun 18, 2018

unpluggedk commented Jun 26, 2018

oldalton left a comment

oldalton Jun 26, 2018

unpluggedk Jun 26, 2018

oldalton Jun 26, 2018

unpluggedk Jun 26, 2018

oldalton Jun 26, 2018

oldalton Jun 26, 2018

unpluggedk Jun 26, 2018

oldalton Jun 26, 2018

oldalton Jun 26, 2018 •

edited

Loading

unpluggedk Jun 26, 2018

oldalton Jun 26, 2018

Error code & other related tasks refactoring #153

Error code & other related tasks refactoring #153

Conversation

unpluggedk commented Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unpluggedk commented Jun 26, 2018

oldalton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oldalton Jun 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unpluggedk commented Jun 18, 2018 •

edited

Loading

oldalton Jun 26, 2018 •

edited

Loading