-
Notifications
You must be signed in to change notification settings - Fork 37
Webview interaction and system webview #126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Interaction required errors occur because of a wide variety of errors | ||
returned by the authentication service. | ||
*/ | ||
MSIDErrorMismatchedUser = -52101, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curious, when is this error returned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When parameter sets a user, and we force webview to login as a different user.
We handled it as an error in MSAL, we can discuss about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we put errors in increasing order? (MSIDErrorInvalidState is in the middle)
returned by the authentication service. | ||
*/ | ||
MSIDErrorMismatchedUser = -52101, | ||
MSIDErrorNoAuthorizationResponse = -52102, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: no authorization code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, it's just url nil response.
/*! | ||
An interactive authentication session failed to start. | ||
*/ | ||
MSIDErrorInteractiveSessionStartFailure = -51022, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the case when this would happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SFAuthenticationSession: when start is called on a cancelled session
SafariViewController, when we can't find the current viewcontroller to present from.
IdentityCore/src/MSIDError.m
Outdated
@@ -49,3 +49,20 @@ | |||
} | |||
|
|||
|
|||
MSIDErrorCode MSIDErrorCodeForOAuthError(NSString *oauthError, MSIDErrorCode defaultCode) | |||
{ | |||
if (oauthError && [oauthError caseInsensitiveCompare:@"invalid_request"] == NSOrderedSame) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have this code in token response I think. Can we combine it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, i've changed TokenResponse to call this.
@@ -40,5 +40,7 @@ | |||
|
|||
@property (readwrite) NSString *loginHint; | |||
@property (readwrite) NSString *extraQueryParameters; | |||
@property (readwrite) NSString *promptBehavior; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think prompt behavior was something AAD V1 specific?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also exists for MSAL in a different value.
case MSALForceLogin : return @{ @"prompt" : @"login" };
case MSALForceConsent : return @{ @"prompt" : @"consent" };
case MSALSelectAccount : return @{ @"prompt" : @"select_account" };
{ | ||
return nil; | ||
} | ||
- (id)initWithURL:(NSURL *)url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: id -> instancetype
|
||
@interface MSIDEmbeddedWebviewRequest : MSIDWebviewRequest | ||
- (id)initWithURL:(NSURL *)url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: id -> instancetype
_url = url; | ||
_context = context; | ||
|
||
_safariViewController = [[SFSafariViewController alloc] initWithURL:url entersReaderIfAvailable:NO]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how do we ensure that we're on the main thread?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I think it is logical that we simply check if we are on main thread when start is called, and return NO if we aren't.
|
||
@property NSURL *endURL; | ||
@property NSDictionary *headers; | ||
@property id<MSIDSystemWebviewResponseDelegate> webviewDelegate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is webviewDelegate set? Also, if it will be set in a "wrapper" class, who has reference to this controller, this should be weak.
MSIDWebUICompletionHandler _completionHandler; | ||
|
||
#ifdef __IPHONE_11_0 | ||
MSIDSFAuthenticationSession *_authSession; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this is not set anywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a nit, huge mistake :p this should've been set 👍
error_description and a fallback logic on error_subcode.
5a5c632
to
69e15a1
Compare
{ | ||
if (!s_currentWebSession) | ||
{ | ||
@synchronized([MSIDWebviewAuthorization class]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to include line 104 into the synchronized block? seems it will suffer from race condition otherwise :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you are right, will change
context:(id<MSIDRequestContext>)context | ||
completionHandler:(MSIDWebUICompletionHandler)completionHandler | ||
{ | ||
if (![self setCurrentWebSession:webviewController]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems we are not able to setCurrentWebSession
successfully for the second time, as there is no code to release s_currentWebSession
when a webview call is completed? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've included the session cancelling as discussed before
NSError *error = MSIDCreateError(MSIDErrorDomain, MSIDErrorInteractiveSessionStartFailure, @"Interactive web session failed to start.", nil, nil, nil, context.correlationId, nil); | ||
|
||
completionHandler(nil, error); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have already passed completionHandler
to system/embedded webview, shall we let the webview class to call the completion block if it fails to start?
The start
action is kind of async because it involves UI/main thread, it may not be able to return a value immediately to indicate if the start action succeeds :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed with JK offline, embedded webview will return YES for the start method, to avoid the completion block being called twice :)
session as a completion block.
…tion-library-common-for-objc into jak/system_webview
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some additional questions and comments. Looking good otherwise, I'm fine merging it after those comments are addressed and commented out tests are either removed or fixed. We should also add an action item to add more logs to this, but we can do in in a separate PR :)
IdentityCore/src/MSIDError.m
Outdated
} | ||
if (oauthError && [oauthError caseInsensitiveCompare:@"invalid_scope"] == NSOrderedSame) | ||
{ | ||
return MSIDErrorInvalidParameter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should we be more specific about which parameter is invalid?
@@ -24,13 +24,19 @@ | |||
#import <Foundation/Foundation.h> | |||
#import "MSIDNetworkConfiguration.h" | |||
|
|||
@class MSIDPkce; | |||
@class MSIDClientInfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: if MSIDClientInfo is not necessary, please remove this :)
@property (readwrite) MSIDClientInfo *clientInfo; | ||
|
||
// User information | ||
@property (readwrite) NSString *utid; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this is necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is used in the param settings for v2
|
||
// Priority start URL | ||
@property (readwrite) NSURL *explicitStartURL; | ||
|
||
- (instancetype)initWithAuthority:(NSURL *)authority |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just for me to know, why do we need both authority and authorizationEndpoint, shouldn't authorizationEndpoint be enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed according to offline discussion : MSIDWebviewConfig is different enough to be its own class, not a subclass.
correlationId:(NSUUID *)correlationId; | ||
|
||
- (instancetype)initWithAuthority:(NSURL *)authority redirectUri:(NSString *)redirectUri clientId:(NSString *)clientId target:(NSString *)target correlationId:(NSUUID *)correlationId NS_UNAVAILABLE; | ||
- (instancetype)initWithAuthority:(NSURL *)authority redirectUri:(NSString *)redirectUri clientId:(NSString *)clientId target:(NSString *)target NS_UNAVAILABLE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feels a bit awkward that both of those are unavailable, yet it's a subclass of MSIDConfiguration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be removed - no longer a subclass.
if (!url) | ||
{ | ||
if (error){ | ||
*error = MSIDCreateError(MSIDOAuthErrorDomain, MSIDErrorInvalidParameter, @"Trying to create a response with nil", nil, nil, nil, context.correlationId, nil); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: with nil "url"
if (@available(iOS 11.0, *)) | ||
{ | ||
_authSession = [[SFAuthenticationSession alloc] initWithURL:_startURL | ||
callbackURLScheme:_callbackURLScheme |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we pass full redirectUri in here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scheme here should be enough
MSIDSFAuthenticationSession *_authSession; | ||
MSIDSafariViewController *_safariViewController; | ||
|
||
MSIDOauth2Factory *_factory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where's factory being set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nowhere, removed
- (void)cancel | ||
{ | ||
if (@available(iOS 11.0, *)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: both session and safariViewController conform to the same protocol, don't they?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I could change this to remove this unnecessary check.
return NO; | ||
} | ||
|
||
if (@available(iOS 11.0, *)) { return NO; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should this be yes as we don't have anything to handle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, it should be no if we don't handle it.
think about it from this angle,
it app delegate, it will do something like
return microsoft_url_handle || other_url_handle
- remove unnecessary properties from MSIDConfiguration - update testing - update how pkce is handled - update MSIDNetworkConfiguration to be only consist of class properties - address nits and comments
_retryCount = s_defaultRetryCount; | ||
} | ||
return self; | ||
s_timeout = timeout; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it is better to have default timeout & retryCount setter & getters and assign default values to them in initializer? In such case you will not need to override setters & getters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are class properties, which means they don't have default synthesizer.
thus, we'd need to write a setter/getter anyways. so, with that, I don't see a need for an initializer.
@@ -24,11 +24,9 @@ | |||
#import <Foundation/Foundation.h> | |||
|
|||
|
|||
@interface MSIDNetworkConfiguration : NSObject <NSCopying> | |||
@interface MSIDNetworkConfiguration : NSObject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should MSIDNetworkConfiguration be singleton? If no, how are we going to access it in the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both it's properties are class properties with setters and getters.
So, you'd just use MSIDNetworkConfiguration.timeout and MSIDNetworkConfiguration.retryCount
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://developer.apple.com/videos/play/wwdc2016/405/ at 5:07 for reference.
if (configuration.sliceParameters) | ||
{ | ||
[parameters addEntriesFromDictionary:configuration.sliceParameters]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already add slice parameters at line 38 :)
verifyState:(BOOL)verifyState | ||
context:(id<MSIDRequestContext>)context | ||
completionHandler:(MSIDWebviewAuthCompletionHandler)completionHandler | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parameter verifyState
is no longer used in this function anymore :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it is already in the configuration. let me remove.
@@ -2823,7 +3042,7 @@ | |||
GCC_WARN_UNUSED_FUNCTION = YES; | |||
GCC_WARN_UNUSED_VARIABLE = YES; | |||
INFOPLIST_FILE = MSIDTestsHostApp/Info.plist; | |||
IPHONEOS_DEPLOYMENT_TARGET = 9.0; | |||
IPHONEOS_DEPLOYMENT_TARGET = 10.3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we change deployment target?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😕
MSIDErrorUnsupportedFunctionality = -51018, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both MSIDErrorInvalidParameter = -51018, and MSIDErrorUnsupportedFunctionality = -51018 are -51018?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's address errors in a separate PR
} | ||
|
||
NSMutableOrderedSet<NSString *> *allScopes = parameters[MSID_OAUTH2_SCOPE].scopeSet.mutableCopy; | ||
[allScopes addObject:MSID_OAUTH2_SCOPE_OPENID_VALUE]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For V1, no scopes are passed in, thenallScopes
will be nil
? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, we should address it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -33,27 +34,15 @@ - (instancetype)copyWithZone:(NSZone*)zone | |||
configuration.redirectUri = [_redirectUri copyWithZone:zone]; | |||
configuration.target = [_target copyWithZone:zone]; | |||
configuration.clientId = [_clientId copyWithZone:zone]; | |||
configuration.correlationId = [_correlationId copyWithZone:zone]; | |||
configuration.loginHint = [_loginHint copyWithZone:zone]; | |||
configuration.sliceParameters = [_sliceParameters copyWithZone:zone]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need slice parameters for all requests, both interactive and silent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is to be determined whether it will use MSIDConfiguration class or not.
@property (readwrite) NSString *requestState; | ||
// State verifier: Recommended verifier for state value of the response. | ||
// Set to YES to stop if verifying state fails | ||
@property (readwrite) BOOL verifyState; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'd prefer verify state to be set on initialize, so nobody can change it in the middle of authentication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds logical. will change accordingly.
customWebview:(WKWebView *)webview; | ||
- (id<MSIDWebviewInteracting>)systemWebviewControllerWithRequest:(MSIDConfiguration *)requestParams; | ||
// Webview related | ||
@property(readonly) MSIDWebviewFactory *webviewFactory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: space
|
||
- (NSString *)generateStateValue | ||
{ | ||
return [[NSUUID UUID] UUIDString]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oki :)
{ | ||
if (!_webviewFactory) | ||
{ | ||
_webviewFactory = [[MSIDAADWebviewFactory alloc] init]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: do we need to do it in each subclass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they create different type of webview factory
} | ||
|
||
NSMutableOrderedSet<NSString *> *allScopes = parameters[MSID_OAUTH2_SCOPE].scopeSet.mutableCopy; | ||
[allScopes addObject:MSID_OAUTH2_SCOPE_OPENID_VALUE]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, we should address it :)
{ | ||
[parameters addEntriesFromDictionary: | ||
@{ | ||
MSID_OAUTH2_CORRELATION_ID_REQUEST : @"true", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just curious why is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was in x-plat libraries as well as our ADAL.
From MSDN, https://msdn.microsoft.com/en-us/library/mt739779.aspx
2.2.1.2 return-client-request-id
The return-client-request-id HTTP header is optional. This header is sent in the request and is used by the key provisioning server to determine whether to return the client-specified client-request-id in the server response. If present, the value of the return-client-request-id header MUST be "true".
The format of the return-client-request-id header, in ABNF, is as follows.
return-client-request-id = "true"
@@ -94,4 +94,22 @@ - (NSDictionary *)dictionaryByRemovingFields:(NSArray *)fieldsToRemove | |||
return mutableDict; | |||
} | |||
|
|||
- (NSArray<NSURLQueryItem *> *)urlQueryItemsArray; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curious, where is this being used?
and nit: there's a ";" in the end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we use this constructing startURL.
urlComponents.queryItems = [parameters urlQueryItemsArray];
// [toReturn appendFormat:@"%02x", hash[i]]; | ||
// } | ||
// return toReturn; | ||
return [NSString msidBase64UrlEncodeData:[self msalSHA256Data]]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine merging it after previous nit comments are addressed. This PR has grown way too much :)
urlComponents.queryItems = @{ | ||
MSID_OAUTH2_ERROR : errorString, | ||
MSID_OAUTH2_ERROR_DESCRIPTION : errorDescription, | ||
MSID_OAUTH2_SUB_ERROR : subError, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we also have a test for the "subcode" scenario?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. The only place where it made sense was when a developer clicks on a back button on the webview itself.
By passing haschrome=1, there no longer is the back button and thus, we are fine without.
@@ -417,5 +421,112 @@ - (void)testAccountFromTokenResponse_whenAADV2TokenResponse_shouldInitAccountAnd | |||
XCTAssertEqualObjects(account.authority.absoluteString, @"https://login.microsoftonline.com/contoso.com"); | |||
} | |||
|
|||
#pragma mark - Webview (StartURL) | |||
//- (void)testStartURL_whenValidParams_shouldContainQPs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this code is not necessary, let's remove it :)
No description provided.