Webview integration #1262

unpluggedk · 2018-08-16T00:28:30Z

No description provided.

…y-library-for-objc into jak/webview-integration

Modify other codes to accomodate changes.

…azure-activedirectory-library-for-objc into jak/automation-update

oldalton · 2018-08-28T04:21:38Z

ADAL/automation/ios/AutoWebViewController/ADAutoWebViewController.m

@@ -32,8 +33,8 @@ @implementation ADAutoWebViewController
 - (void)viewDidLoad
 {
    [super viewDidLoad];
-
-    self.webView.accessibilityIdentifier = @"ADAL_SIGN_IN_WEBVIEW";
+    self.webView = [[WKWebView alloc] initWithFrame:[[UIScreen mainScreen] bounds]];


I think this is fixed in your other PR?

oldalton · 2018-08-28T04:23:12Z

ADAL/src/ADRequestParameters.h

+@property (retain, nonatomic) NSString *redirectUri;
+@property (retain, nonatomic) NSString *scopesString;
+@property (retain, nonatomic) ADUserIdentifier *identifier;
+@property (retain, nonatomic) NSString *claims;


nit: Should we add claims and extra query parameters also into the initializer?

We don't use the initializer in the code, I will actually remove the initializer.

oldalton · 2018-08-28T04:24:25Z

ADAL/src/broker/ios/ADBrokerKeyHelper.m

@@ -328,7 +328,7 @@ + (NSDictionary *)decryptBrokerResponse:(NSDictionary *)response correlationId:(
    }

    //now compute the hash on the unencrypted data
-    NSString *actualHash = [ADPkeyAuthHelper computeThumbprint:decrypted isSha2:YES];
+    NSString *actualHash = [MSIDPkeyAuthHelper computeThumbprint:decrypted isSha2:YES];


That's funny that we have Sha2 computation only inside PkeyAuthHelper :)

oldalton · 2018-08-28T04:26:21Z

ADAL/src/request/ADAuthenticationRequest+AcquireToken.m

@@ -301,7 +309,7 @@ - (void)requestToken:(ADAuthenticationCallback)completionBlock
        return;
    }

-    if (_silent && !_allowSilent)
+    if (_silent)


I assume allow Silent was removed because it's only used in broker?

that is correct

oldalton · 2018-08-28T04:27:23Z

ADAL/src/request/ADAuthenticationRequest+AcquireToken.m

+
+        if (error)
+        {
+            ADAuthenticationResult *result = (AD_ERROR_UI_USER_CANCEL == error.code) ? [ADAuthenticationResult resultFromCancellation:_requestParams.correlationId]


Shouldn't we automatically fallback to interactive flow if it's a silent request (AD_PROMPT_AUTO scenario)?

This is already in interactive flow. I may be misunderstanding your question, so let's sync offline

As settled in the offline discussion, this is strictly broker flow, thus we are fine with not having this logic.

oldalton · 2018-08-28T04:36:58Z

ADAL/src/ui/ADWebAuthController.m

-{
-    MSID_LOG_VERBOSE(_requestParams, @"-webAuthDidFinishLoad host: %@", [ADAuthorityUtils isKnownHost:url] ? url.host : @"unknown host");
-    MSID_LOG_VERBOSE_PII(_requestParams, @"-webAuthDidFinishLoad host: %@", url.host);
+    NSString *authorityWithOAuthSuffix = [NSString stringWithFormat:@"%@%@", context.authority, MSID_OAUTH2_AUTHORIZE_SUFFIX];


nit: shouldn't we have a common core or ADAL utility to compose this url?

It is being addressed in AzureAD/microsoft-authentication-library-common-for-objc#130 ,
I will at least add some comment on this to update when the above PR is merged.

oldalton · 2018-08-28T04:37:55Z

ADAL/src/ui/ADWebAuthController.m

 }

-// The user cancelled authentication
- (void)webAuthDidCancel
+ (NSDictionary *)dictFromQueryString:(NSString *)query


nit: if this is still not available in common core, let's move this utility there

This is a good point, common core's utility needs revision I think.
Query params can have key-only value and our utility does not account for this.

oldalton · 2018-08-28T04:38:35Z

ADAL/src/ui/ADWebAuthController.m


-+ (ADAuthenticationResult*)responseFromInterruptedBrokerSession
+- (void)cancelCurrentWebAuthSession


isn't this duplicate with line 121?

Yes and removed one of them.

oldalton · 2018-08-28T04:40:38Z

ADAL/tests/app/src/mac/ADTestAppAcquireTokenWindowController.m

@@ -289,7 +289,7 @@ - (IBAction)acquireTokenInteractive:(id)sender

             if ([_acquireSettingsView isHidden])
             {
-                 [_webView.mainFrame loadHTMLString:@"<html><head></head><body>done!</body></html>" baseURL:nil];
+                 [_webView loadHTMLString:@"<html><head></head><body>Done</body></html>" baseURL:nil];


This is a very important change :)

oldalton · 2018-08-28T04:41:51Z

ADAL/tests/app/src/mac/ADTestAppAcquireTokenWindowController.h

@@ -29,7 +29,7 @@
 @interface ADTestAppAcquireTokenWindowController : NSWindowController
 {
    IBOutlet NSView* _authView;
-    IBOutlet WebView* _webView;
+    IBOutlet WKWebView* _webView;


WKWebView is not supported on older OS-s as IBOutlet (in storyboard). I think you might still be using WebView in storyboard or how can it work?

Automation update

…y-library-for-objc into jak/webview-integration

string to be urn encoded.

comments

Feedback addressed or answered.

oldalton

Found a we minor telemetry issues and url opening issues. Please check them out before merging.

oldalton · 2018-09-05T03:39:40Z

ADAL/src/request/ADAuthenticationRequest+AcquireToken.m

+        ADAuthenticationResult *result = [ADAuthenticationResult resultFromError:error correlationId:_requestParams.correlationId];
+        [result setCloudAuthority:_cloudAuthority];
+        completionHandler(result);
+        return YES;


I don't think telemetry event gets stopped in this case

We stopped it in line 560 before coming in here.

ok, I missed that line :)

oldalton · 2018-09-05T03:39:45Z

ADAL/src/request/ADAuthenticationRequest+AcquireToken.m

+    [ADBrokerHelper promptBrokerInstall:[NSURL URLWithString:authResponse.appInstallLink]
+                          brokerRequest:brokerRequestURL
+                      completionHandler:completionHandler];
+    return YES;


I don't think telemetry event gets stopped in this case

We stopped it in line 560 before coming in here.

oldalton · 2018-09-05T03:40:26Z

ADAL/src/request/ADAuthenticationRequest+AcquireToken.m

+            [MSIDAppExtensionUtil sharedApplicationOpenURL:browserURL];
+        });
+
+        [MSIDAppExtensionUtil sharedApplicationOpenURL:browserURL];


we shouldn't call sharedApplicationOpenURL twice

Woops, fixed! Removed the first one as this method will check the main queue later anyways.

oldalton · 2018-09-05T03:42:07Z

ADAL/src/request/ADAuthenticationRequest+AcquireToken.m

+
+    MSIDWebOAuth2Response *oauthResponse = (MSIDWebOAuth2Response *)response;
+
+    if (oauthResponse.authorizationCode)


nit: should we check if it's empty too? (msidIsNilOrEmpty)

I think it would be better handled at common,
AzureAD/microsoft-authentication-library-common-for-objc#223

oldalton · 2018-09-05T03:49:25Z

ADAL/src/ui/ADWebAuthController.m

 #if TARGET_OS_IPHONE
-    [_authenticationViewController setParentController:parent];
-    [_authenticationViewController setFullScreen:fullScreen];


Did we remove the fullScreen functionality?

Replaced it with presentationStyle. Also marked it deprecated.

oldalton · 2018-09-05T03:51:31Z

ADAL/tests/app/src/mac/ADTestAppAcquireTokenWindowController.h


+    IBOutlet NSView *_contentWebView;


nit: can we call it contentView or webviewContentView?

It has 2 "views", and still sound ugly :p thus, I refuse to change this

…y-library-for-objc into jak/webview-integration

presentationStyle instead.

unpluggedk added 9 commits July 24, 2018 16:33

commiting initial changes before merging current dev

577a176

Merge branch 'dev' of https://github.com/AzureAD/azure-activedirector…

593a8fc

…y-library-for-objc into jak/webview-integration

integrate webview to ADAL

2bf7395

delete ADWebAuthController and ADWebAuthDelegate

ff0a7ec

remove url protocol related classes

73d2c2c

set notifications

d1e62e1

fix tests and create mock webauth controller

b44a5de

update submodule

7f09a72

remove wpj, fix unit tests and automation testing.

16d25b7

unpluggedk requested review from jasoncoolmax and oldalton August 16, 2018 00:28

unpluggedk added 9 commits August 16, 2018 12:36

remove WebViewType typedef from public - now it is only WkWebview

ddc2bd4

fix passed in webview for automation - iOS

bbf35d1

ADRequestParameters to contain claims and extra query parameters.

eac0d49

Modify other codes to accomodate changes.

fix automation testing, and revise iOS automation app.

4ee1eab

handle oauth error

22d6ba5

Merge branch 'jak/webview-integration' of https://github.com/AzureAD/…

7444f03

…azure-activedirectory-library-for-objc into jak/automation-update

fix mac automation

2bcd03d

update error handling in automation

c912d0f

update to latest common core

cbdd71a

unpluggedk requested review from antrix1989 and rohitnarula7176 August 24, 2018 00:35

unpluggedk added 3 commits August 24, 2018 13:28

fix applink passing for broker installation

566720f

Merge branch 'jak/webview-integration' of https://github.com/AzureAD/…

58bfdf0

…azure-activedirectory-library-for-objc into jak/automation-update

set ignoreInvalidState to YES, and update automation

0165ceb

oldalton previously requested changes Aug 28, 2018

View reviewed changes

unpluggedk added 4 commits August 28, 2018 01:54

update submodule and address feedback

640d14a

Merge pull request #1264 from AzureAD/jak/automation-update

a09275b

Automation update

Merge branch 'dev' of https://github.com/AzureAD/azure-activedirector…

831a169

…y-library-for-objc into jak/webview-integration

refactor requestCode, add specific comment on extra query parameter

e6c8ead

string to be urn encoded.

unpluggedk added 3 commits August 30, 2018 00:39

refactor requestCode completionBlock and address other minor feedback

be1e5cf

comments

update mac test app

136e1cb

error fix on processing oauth response

5abae08

oldalton approved these changes Sep 5, 2018

View reviewed changes

unpluggedk added 3 commits September 5, 2018 11:06

Merge branch 'dev' of https://github.com/AzureAD/azure-activedirector…

9f94759

…y-library-for-objc into jak/webview-integration

Merge branch 'dev' of https://github.com/AzureAD/azure-activedirector…

2a1941f

…y-library-for-objc into jak/webview-integration

fix issues addressed in code reviews.

8f99b58

unpluggedk force-pushed the jak/webview-integration branch from 15a2cc2 to 8f99b58 Compare September 7, 2018 07:55

unpluggedk added 3 commits September 7, 2018 01:02

mark fullScreen setting from ADAuthenticationSettings deprecated and add

427c5f4

presentationStyle instead.

mark enableFullScreen deprecated

a312074

handle deprecated property 'enableFullScreen'

a640aa4

unpluggedk merged commit 04319bd into dev Sep 7, 2018

unpluggedk deleted the jak/webview-integration branch September 7, 2018 22:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Webview integration #1262

Webview integration #1262

unpluggedk commented Aug 16, 2018

oldalton Aug 28, 2018

unpluggedk Aug 28, 2018

oldalton Aug 28, 2018 •

edited

unpluggedk Aug 28, 2018

oldalton Aug 28, 2018

unpluggedk Aug 28, 2018

oldalton Aug 28, 2018

unpluggedk Aug 28, 2018

oldalton Aug 28, 2018

unpluggedk Aug 28, 2018

unpluggedk Aug 28, 2018

oldalton Aug 28, 2018

unpluggedk Aug 29, 2018

oldalton Aug 28, 2018

unpluggedk Aug 29, 2018

oldalton Aug 28, 2018

unpluggedk Aug 29, 2018

oldalton Aug 28, 2018

oldalton Aug 28, 2018

unpluggedk Aug 30, 2018

oldalton left a comment

oldalton Sep 5, 2018

unpluggedk Sep 7, 2018

oldalton Sep 7, 2018

oldalton Sep 5, 2018

unpluggedk Sep 7, 2018

oldalton Sep 5, 2018

unpluggedk Sep 7, 2018

oldalton Sep 5, 2018

unpluggedk Sep 7, 2018

oldalton Sep 5, 2018

unpluggedk Sep 7, 2018

oldalton Sep 5, 2018

unpluggedk Sep 7, 2018


		+ (ADAuthenticationResult*)responseFromInterruptedBrokerSession
		- (void)cancelCurrentWebAuthSession


		MSIDWebOAuth2Response oauthResponse = (MSIDWebOAuth2Response )response;

		if (oauthResponse.authorizationCode)

Webview integration #1262

Webview integration #1262

Conversation

unpluggedk commented Aug 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oldalton Aug 28, 2018 • edited

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

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

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 Aug 28, 2018 •

edited