Network stack Integration #296

jasoncoolmax · 2018-07-30T23:37:58Z

Integrate the network stack with MSAL (necessary changes on the network stack is here: Changes for Network Integration microsoft-authentication-library-common-for-objc#185)
Fix a bug in ADFS authority validation, where authority host should be compared rather than the whole authority

oldalton

Looking good, added a couple of nit comments :)

oldalton · 2018-07-31T22:51:57Z

MSAL/src/instance/MSALAdfsAuthorityResolver.m

        CHECK_COMPLETION(!error);

+        NSMutableDictionary *responseDic = (NSMutableDictionary *)response;


nit: should we check that response is actually a dictionary? or should the request maybe already return a dictionary?

oldalton · 2018-07-31T22:52:21Z

MSAL/src/instance/MSALAdfsAuthorityResolver.m

@@ -252,7 +259,7 @@ - (BOOL)isRealmTrustedFromWebFingerPayload:(NSArray<MSALWebFingerLink *> *)links
    for (MSALWebFingerLink *link in links)
    {
        if ([link.rel caseInsensitiveCompare:TRUSTED_REALM] == NSOrderedSame &&
-            [[NSURL URLWithString:link.href] msidIsEquivalentAuthority:authority])
+            [[NSURL URLWithString:link.href] msidIsEquivalentAuthorityHost:authority])


oldalton · 2018-07-31T22:56:56Z

MSAL/src/requests/MSALBaseRequest.m

+    return nil;
+}
+
+- (NSURL *)tokenEndpointWithSliceParameter


can we make this more generic? slice parameter is like any other EQP.

the main purpose of the function is to get the token endpoint, will it be better to rename it - (NSURL *)tokenEndpoint? adding the slice parameter is just some internal details

Yes, I like tokenEndpoint more

unpluggedk · 2018-08-03T22:38:05Z

MSAL/src/instance/MSALAadAuthorityResolver.m

         if (error)
         {
             completionBlock(nil, error);
             return;
         }
+
+        if(response && ![response isKindOfClass:[NSMutableDictionary class]])


Does the response need to be a mutable dict?

It doesn't. Now changed to NSDictionary :)

unpluggedk · 2018-08-03T22:38:22Z

MSAL/src/instance/MSALAdfsAuthorityResolver.m

        CHECK_COMPLETION(!error);

+        if(response && ![response isKindOfClass:[NSMutableDictionary class]])


same as before

unpluggedk · 2018-08-03T22:39:54Z

MSAL/src/instance/MSALAdfsAuthorityResolver.m

-                                    context:context];
-
-    [request sendGet:^(MSALHttpResponse *response, NSError *error) {
+    NSURL *url = [MSALAdfsAuthorityResolver urlForWebFinger:authenticationEndpoint absoluteAuthority:authority.absoluteString];


nit: NSURL *url to be NSURL *webfingerUrl?

unpluggedk · 2018-08-03T22:40:14Z

MSAL/src/instance/MSALAuthorityBaseResolver.m

        CHECK_COMPLETION(!error);
+
+        if(response && ![response isKindOfClass:[NSMutableDictionary class]])


same as before. nsmutabledict?

unpluggedk · 2018-08-03T22:40:31Z

MSAL/src/requests/MSALBaseRequest.m

+            return;
+        }
+
+        if(response && ![response isKindOfClass:[NSMutableDictionary class]])


unpluggedk · 2018-08-03T22:46:17Z

MSAL/src/requests/MSALBaseRequest.m

+
+        NSMutableDictionary *jsonDictionary = (NSMutableDictionary *)response;
+
+        MSIDAADV2TokenResponse *tokenResponse = (MSIDAADV2TokenResponse *)[self.oauth2Factory tokenResponseFromJSON:jsonDictionary context:nil error:&error];


Try to use a local error for the whole block in this sendWithBlock, instead of re-using the "error" you got in line 170

unpluggedk · 2018-08-03T22:48:16Z

MSAL/src/requests/MSALInteractiveRequest.m

+    return [[MSIDAADAuthorizationCodeGrantRequest alloc] initWithEndpoint:[self tokenEndpoint]
+                                                                 clientId:_parameters.clientId
+                                                                    scope:[[self requestScopes:nil] msalToString]
+                                                              redirectUri:[_parameters.redirectUri absoluteString]


nit: I'd rather see a .absoluteString than [ ]

oldalton

A couple of questions + JK's comments, otherwise looking good :)

oldalton · 2018-08-04T00:09:31Z

MSAL/src/instance/MSALAdfsAuthorityResolver.m

+        if(response && ![response isKindOfClass:[NSMutableDictionary class]])
+        {
+            NSError *localError = CREATE_MSID_LOG_ERROR(context, MSALErrorInternal, @"response is not of the expected type: NSMutableDictionary.");
+            completionBlock(nil, localError);


nit: can we check that completionBlock is not nil?

oldalton · 2018-08-04T00:11:16Z

MSAL/src/requests/MSALBaseRequest.m

+        MSIDAccessToken *accessToken = [self.oauth2Factory accessTokenFromResponse:tokenResponse configuration:configuration];
+        MSIDIdToken *idToken = [self.oauth2Factory idTokenFromResponse:tokenResponse configuration:configuration];
+
+        MSALResult *result = [MSALResult resultWithAccessToken:accessToken idToken:idToken];


nit: check that accessToken and idToken are not nil

oldalton · 2018-08-04T00:12:31Z

MSAL/src/requests/MSALWebAuthResponse.m

-
-            NSString* messagePII = [NSString stringWithFormat:@"Error raised: (Domain: \"%@\" Response Code: %ld \n%@", @"Domain", (long)response.statusCode, errorData];
-
-            NSMutableDictionary *userInfo = [@{MSALHTTPResponseCodeKey : [NSString stringWithFormat: @"%ld", (long)response.statusCode]} mutableCopy];


Just double checking: does the new network stack have the feature to return http response code and headers?

Currently nope. But I have added them, they will be in another PR :)

all comments addressed :)

# Conflicts: # MSAL/IdentityCore # MSAL/MSAL.xcodeproj/project.pbxproj # MSAL/src/requests/MSALBaseRequest.m # MSAL/src/requests/MSALInteractiveRequest.m # MSAL/test/unit/MSALB2CPolicyTests.m # MSAL/test/unit/MSALInteractiveRequestTests.m

oldalton · 2018-08-24T02:19:34Z

@jasoncoolmax, can we merge this one? It's approved.
I think #314 will need to be updated after this one is merged :)

# Conflicts: # MSAL/MSAL.xcodeproj/project.pbxproj # MSAL/src/requests/MSALBaseRequest.m

jasoncoolmax added 3 commits July 30, 2018 15:53

Integration common core network stack with MSAL

01d4a68

Removed unnecessary header

e2c47c5

update submodule

d51cecc

jasoncoolmax requested review from oldalton, antrix1989, unpluggedk and rohitnarula7176 July 30, 2018 23:38

jasoncoolmax changed the title ~~Network stack Integration~~ Network stack Integration [On Hold] Jul 31, 2018

oldalton reviewed Jul 31, 2018

View reviewed changes

jasoncoolmax added 2 commits July 31, 2018 17:26

use MSID network request for Aad authority validation, fix UTs

c0f23a4

Remove MSAL network requests which is no longer needed

209d37c

jasoncoolmax changed the title ~~Network stack Integration [On Hold]~~ Network stack Integration Aug 1, 2018

jasoncoolmax added 3 commits July 31, 2018 17:57

update submodule

517a10b

Check response type before continue

76af6ff

fix typo

038ad75

oldalton added the 0.1.5 label Aug 2, 2018

oldalton added this to the 0.1.5 milestone Aug 2, 2018

unpluggedk previously requested changes Aug 3, 2018

View reviewed changes

AzureAD deleted a comment from unpluggedk Aug 4, 2018

oldalton approved these changes Aug 4, 2018

View reviewed changes

Address comments

8880615

jasoncoolmax added 2 commits August 5, 2018 12:23

fix error code

036e760

Merge branch 'dev' into jason/extendedExpires

c75e059

# Conflicts: # MSAL/MSAL.xcodeproj/project.pbxproj # MSAL/src/requests/MSALBaseRequest.m

jasoncoolmax merged commit 6314562 into dev Aug 24, 2018

jasoncoolmax deleted the jason/networkIntegration branch January 31, 2019 20:09

		CHECK_COMPLETION(!error);

		NSMutableDictionary responseDic = (NSMutableDictionary )response;

		CHECK_COMPLETION(!error);

		if(response && ![response isKindOfClass:[NSMutableDictionary class]])


		NSMutableDictionary jsonDictionary = (NSMutableDictionary )response;

		MSIDAADV2TokenResponse tokenResponse = (MSIDAADV2TokenResponse )[self.oauth2Factory tokenResponseFromJSON:jsonDictionary context:nil error:&error];


		NSString* messagePII = [NSString stringWithFormat:@"Error raised: (Domain: \"%@\" Response Code: %ld \n%@", @"Domain", (long)response.statusCode, errorData];

		NSMutableDictionary *userInfo = [@{MSALHTTPResponseCodeKey : [NSString stringWithFormat: @"%ld", (long)response.statusCode]} mutableCopy];

Network stack Integration #296

Network stack Integration #296

Uh oh!

Conversation

jasoncoolmax commented Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oldalton 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!

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!

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!

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!

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!

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!

oldalton left a comment • 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!

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!

oldalton commented Aug 24, 2018

Uh oh!

Uh oh!

jasoncoolmax commented Jul 30, 2018 •

edited

Loading

oldalton left a comment •

edited

Loading