-
Notifications
You must be signed in to change notification settings - Fork 209
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
Added iOS SF/ASWeb AuthenticationSession support #187
Conversation
Replaces #158 |
webauth/agent.js
Outdated
Linking.removeEventListener('url', urlHandler); | ||
if (err) { | ||
reject(err); | ||
if (result && result.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.
This seems odd ... if there is a result
, it could be a success or an error? Is there maybe an error_code
property on there when it's an error? Just seems like a non-standard way to pass a result/response to a callback.
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.
So I thought about this at the time, I could change the result to have a status e.g. success or error. Then you're changing the response to the end user as some errors have just been passed straight out from the Native Platform iOS/Android into err. Although looking at certainly iOS errors I don't think the JS side is equipped to process them anyway.
Perhaps we should more strongly define the errors from the native platforms or we need to change the callback method.
Currently using RCTResponseSenderBlock
which only accept 1 argument which is an array.
https://facebook.github.io/react-native/docs/native-modules-ios
It might be better to use RCTPromiseResolveBlock
although this becomes more of a refactor in both JS/iOS/Android to achieve this.
webauth/index.js
Outdated
@@ -54,7 +54,7 @@ export default class WebAuth { | |||
}/${bundleIdentifier}/callback`; | |||
const expectedState = options.state || state; | |||
let query = { | |||
...defaults |
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.
Was this even working before? Seems like a syntax error:
https://github.com/auth0/react-native-auth0/blob/master/webauth/index.js#L57
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, was in a previous previous 3d party PR. This is a fix for that.
@cocojoe |
I need to make some changes as I'm not happy with the implementation, even though it works 😄 |
I am facing the same issue when I can get this updated version. |
I did pick this back up and got delayed, I am on it this week. |
iOS 11 SFAuthenticationSession iOS 12 ASWebAuthenticationSession
43045ce
to
af6fea7
Compare
@taboulot @MSSPL-PiyaleeMaiti If you are up for trying out this branch and feeding back I would appreciate it. |
@cocojoe We will probably test it next will 👍Thanks ! |
@cocojoe thanks, I will catch you soon. |
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.
Very minor
@@ -10,8 +13,12 @@ | |||
#import <React/RCTUtils.h> | |||
#endif | |||
|
|||
#define ERROR_CANCELLED @{@"error": @"a0.session.user_cancelled",@"error_description": @"User cancelled the Auth"} | |||
#define ERROR_FAILED_TO_LOAD @{@"error": @"a0.session.failed_load",@"error_description": @"Failed to load 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.
"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.
These errors are from moving existing error responses to constants. So there is no actual change of code, only location of 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.
☝️
@@ -10,8 +13,12 @@ | |||
#import <React/RCTUtils.h> | |||
#endif | |||
|
|||
#define ERROR_CANCELLED @{@"error": @"a0.session.user_cancelled",@"error_description": @"User cancelled the Auth"} |
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.
"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.
Same as before
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's a message, though, so no reason not to correct it.
ios/A0Auth0.m
Outdated
completionHandler:^(NSURL * _Nullable callbackURL, | ||
NSError * _Nullable error) { | ||
if ([[error domain] isEqualToString:ASWebAuthenticationSessionErrorDomain]) { | ||
switch ([error 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.
switch
is a bit overkill for a single option ...
ios/A0Auth0.m
Outdated
@@ -87,22 +150,22 @@ - (NSString *)randomValue { | |||
|
|||
- (NSString *)sign:(NSString*)value { | |||
CC_SHA256_CTX ctx; | |||
|
|||
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.
Unnecessary whitespace here and 👇
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.
Oddly enough this is after running Xcode's Re-Indent it adds the whitespace, have removed.
Whitespace
We tested it and it works perfectly 👍 |
Thanks, will get this released soon. |
Any word on the release date for this @cocojoe ? |
Sorry for delays, released 👍 |
This reverts commit 14efe5f.
…n-session Revert "Added iOS SF/ASWeb AuthenticationSession support (auth0#187)"
Adds support for iOS 11 SFAuthenticationSession & iOS 12 ASWebAuthenticationSession
This enables SSO in iOS 11+
No changes are required by the developer, all is handled internally.