-
Notifications
You must be signed in to change notification settings - Fork 35
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
Webview interaction and system webview #126
Changes from 56 commits
5f61177
e905d25
ba71b3d
fc1d8db
f29dcb3
c669afe
d8813b5
c8c826f
4dd6ec6
d987e60
8c99592
d99bc6f
10aae32
0e34d81
9e7fb0b
0bd3d7f
7b5255b
69e15a1
fbbb81e
c5675ec
f1207ee
d58eed6
6f737f0
b2c9929
0dc3a61
243fb01
9ea3817
a4d2286
459dc3a
a234566
a4eacd1
af44efc
36b3ef2
517a114
a5d230e
5ed28e3
2b8b756
1230844
dda26c7
375e1d4
f54800a
ec90fb5
b31e99f
8718d82
a9cff24
1ea816d
7b8979c
7d9ed31
58b7c6a
dad2296
9801600
9d505c8
97ddaa1
9ec3d45
804186a
17834c5
e5da20a
865eff9
185db0d
6183514
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
<?xml version="1.0" encoding="UTF-8"?> | ||
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd"> | ||
<plist version="1.0"> | ||
<dict> | ||
<key>IDEDidComputeMac32BitWarning</key> | ||
<true/> | ||
</dict> | ||
</plist> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,10 +69,51 @@ typedef NS_ENUM(NSInteger, MSIDErrorCode) | |
MSIDErrorInvalidClient = -51015, | ||
MSIDErrorInvalidGrant = -51016, | ||
MSIDErrorInvalidParameter = -51017, | ||
|
||
/*! | ||
The user or application failed to authenticate in the interactive flow. | ||
Inspect MSALOAuthErrorKey and MSALErrorDescriptionKey in the userInfo | ||
dictionary for more detailed information about the specific error. | ||
*/ | ||
MSIDErrorAuthorizationFailed = -52018, | ||
|
||
/*! | ||
The state returned by the server does not match the state that was sent to | ||
the server at the beginning of the authorization attempt. | ||
*/ | ||
MSIDErrorInvalidState = -52501, | ||
/*! | ||
Interaction required errors occur because of a wide variety of errors | ||
returned by the authentication service. | ||
*/ | ||
MSIDErrorMismatchedUser = -52101, | ||
MSIDErrorNoAuthorizationResponse = -52102, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. no, it's just url nil response. |
||
MSIDErrorBadAuthorizationResponse = -52103, | ||
|
||
|
||
MSIDErrorUserCancel = -51019, | ||
/*! | ||
The authentication request was cancelled programmatically. | ||
*/ | ||
MSIDErrorSessionCanceled = -51020, | ||
/*! | ||
An interactive authentication session is already running with the | ||
SafariViewController visible. Another authentication session can not be | ||
launched yet. | ||
*/ | ||
MSIDErrorInteractiveSessionAlreadyRunning = -51021, | ||
/*! | ||
An interactive authentication session failed to start. | ||
*/ | ||
MSIDErrorInteractiveSessionStartFailure = -51022, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. SFAuthenticationSession: when start is called on a cancelled session |
||
|
||
MSIDErrorUnsupportedFunctionality = -51018, | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Let's address errors in a separate PR |
||
MSIDErrorCodeFirst = MSIDErrorInternal, | ||
MSIDErrorCodeLast = MSIDErrorUnsupportedFunctionality | ||
}; | ||
|
||
extern NSError *MSIDCreateError(NSString *domain, NSInteger code, NSString *errorDescription, NSString *oauthError, NSString *subError, NSError *underlyingError, NSUUID *correlationId, NSDictionary *additionalUserInfo); | ||
|
||
extern MSIDErrorCode MSIDErrorCodeForOAuthError(NSString *oauthError, MSIDErrorCode defaultCode); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,3 +49,24 @@ | |
} | ||
|
||
|
||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. done, i've changed TokenResponse to call this. |
||
{ | ||
return MSIDErrorInvalidRequest; | ||
} | ||
if (oauthError && [oauthError caseInsensitiveCompare:@"invalid_client"] == NSOrderedSame) | ||
{ | ||
return MSIDErrorInvalidClient; | ||
} | ||
if (oauthError && [oauthError caseInsensitiveCompare:@"invalid_scope"] == NSOrderedSame) | ||
{ | ||
return MSIDErrorInvalidParameter; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: should we be more specific about which parameter is invalid? |
||
} | ||
if (oauthError && [oauthError caseInsensitiveCompare:@"invalid_grant"] == NSOrderedSame) | ||
{ | ||
return MSIDErrorInvalidGrant; | ||
} | ||
|
||
return defaultCode; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. nit: why is this necessary? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need pkce challenge and method to be passed along, it's easier to pass the whole object over. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: if MSIDClientInfo is not necessary, please remove this :) |
||
|
||
@interface MSIDConfiguration : NSObject <NSCopying> | ||
|
||
// Commonly used or needed properties | ||
@property (readwrite) NSURL *authority; | ||
@property (readwrite) NSString *redirectUri; | ||
@property (readwrite) NSString *clientId; | ||
@property (readwrite) NSString *target; | ||
|
||
@property (readwrite) NSUUID *correlationId; | ||
|
||
@property (readonly) NSString *resource; | ||
@property (readonly) NSOrderedSet<NSString *> *scopes; | ||
|
||
|
@@ -47,7 +53,6 @@ | |
|
||
// Optional configurations | ||
@property (readwrite) NSString *loginHint; | ||
@property (readwrite) NSUUID *correlationId; | ||
|
||
@property (readwrite) MSIDNetworkConfiguration *networkConfig; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ | |
|
||
#import "MSIDConfiguration.h" | ||
#import "NSOrderedSet+MSIDExtensions.h" | ||
#import "MSIDPkce.h" | ||
|
||
@implementation MSIDConfiguration | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should line 50 be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah it should've been exactly that |
||
|
@@ -46,7 +47,7 @@ - (instancetype)initWithAuthority:(NSURL *)authority | |
clientId:(NSString *)clientId | ||
target:(NSString *)target | ||
{ | ||
return [[MSIDConfiguration alloc] initWithAuthority:authority redirectUri:redirectUri clientId:clientId target:target correlationId:nil]; | ||
return [self initWithAuthority:authority redirectUri:redirectUri clientId:clientId target:target correlationId:nil]; | ||
} | ||
|
||
- (instancetype)initWithAuthority:(NSURL *)authority | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,25 +27,39 @@ | |
|
||
#import <Foundation/Foundation.h> | ||
#import "MSIDConfiguration.h" | ||
|
||
@class MSIDPkce; | ||
@class MSIDClientInfo; | ||
#import "MSIDPkce.h" | ||
|
||
@interface MSIDWebviewConfiguration : MSIDConfiguration | ||
|
||
// Common | ||
@property (readwrite) NSURL *authorizationEndpoint; | ||
|
||
@property (readwrite) NSDictionary<NSString *, NSString *> *extraQueryParameters; | ||
@property (readwrite) NSString *promptBehavior; | ||
@property (readwrite) NSString *claims; | ||
|
||
// Is this only for V2? | ||
@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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. sounds logical. will change accordingly. |
||
|
||
// PKCE Support | ||
@property (readwrite) MSIDPkce *pkce; | ||
@property (readwrite) MSIDClientInfo *clientInfo; | ||
|
||
// User information | ||
@property (readwrite) NSString *utid; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. it is used in the param settings for v2 |
||
@property (readwrite) NSString *uid; | ||
|
||
// Priority start URL | ||
@property (readwrite) NSURL *explicitStartURL; | ||
|
||
- (instancetype)initWithAuthority:(NSURL *)authority | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
authorizationEndpoint:(NSURL *)authorizationEndpoint | ||
redirectUri:(NSString *)redirectUri | ||
clientId:(NSString *)clientId | ||
target:(NSString *)target | ||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Will be removed - no longer a subclass. |
||
|
||
@end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ | |
// THE SOFTWARE. | ||
|
||
#import <Foundation/Foundation.h> | ||
#import "MSIDWebviewInteracting.h" | ||
|
||
@class MSIDTokenResponse; | ||
@class MSIDBaseToken; | ||
|
@@ -33,7 +34,8 @@ | |
@class MSIDLegacyRefreshToken; | ||
@class MSIDAccount; | ||
@class MSIDConfiguration; | ||
@class WKWebView; | ||
@class MSIDWebviewConfiguration; | ||
@class MSIDWebviewFactory; | ||
|
||
@protocol MSIDRequestContext; | ||
@protocol MSIDWebviewInteracting; | ||
|
@@ -59,9 +61,9 @@ | |
- (MSIDLegacySingleResourceToken *)legacyTokenFromResponse:(MSIDTokenResponse *)response configuration:(MSIDConfiguration *)configuration; | ||
- (MSIDAccount *)accountFromResponse:(MSIDTokenResponse *)response configuration:(MSIDConfiguration *)configuration; | ||
|
||
// Webviews | ||
- (id<MSIDWebviewInteracting>)embeddedWebviewControllerWithRequest:(MSIDConfiguration *)requestParams | ||
customWebview:(WKWebView *)webview; | ||
- (id<MSIDWebviewInteracting>)systemWebviewControllerWithRequest:(MSIDConfiguration *)requestParams; | ||
// Webview related | ||
- (MSIDWebviewFactory *)webviewFactory; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: this can be a property, I don't think we need to create a new one each time There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure, readonly property seems legit here. |
||
|
||
|
||
@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.
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)