Skip to content
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

AppAuth not working on certain versions of iOS #30

Closed
shankari opened this issue Aug 19, 2018 · 23 comments
Closed

AppAuth not working on certain versions of iOS #30

shankari opened this issue Aug 19, 2018 · 23 comments

Comments

@shankari
Copy link
Contributor

shankari commented Aug 19, 2018

Got a report from the TCAT folks that the AppAuth-based authentication is not working on certain versions of iOS.
Works on certain others.
Reported to not work on 10.2.1

@shankari
Copy link
Contributor Author

shankari commented Aug 19, 2018

Tried to reproduce on version 9.3
Everything worked fine until auth - the webview was launched with the open-to-all signin page.
When I signed in with the test account provided by Anat, though, the page load hung.
I waited for a long time, and then hit "Done" to close the WebView.
I then got this error on the screen.

@shankari
Copy link
Contributor Author

Errors in the console log. I am not sure if all of these are relevant.

1900-08-19 13:12:37.000000 -0800	emission	 DEBUG: [iOS Auth] 02:12:37: Initiating authorization request with scope: openid profile email offline_access
1900-08-19 13:13:26.000000 -0800	emission	 [iOS Auth] 02:13:26: OpenIDAuth Error: handleNotification should not be called!
1900-08-19 13:13:26.000000 -0800	emission	 DEBUG: [iOS Auth] 02:13:26: OpenIDAuth Error: handleNotification should not be called!
...
1900-08-19 13:13:26.000000 -0800	emission	 onLaunch method from external function called
1900-08-19 14:13:26.000000 -0800	emission	 GOT URL:emission.auth://oauth2redirect?state=n_XnEhZM6d3VNPvOQutFVve3zwtra6oQ6e6Gm-W-fAA&code=uss.cT8CftoN2_Ft527TPuR9qdh1W7sL8F9JbJxIdM0FZ1I.5f575a3f-13d3-4f82-a907-9a858a010b12.29327dfe-bbca-4681-887b-c8c40f3f2ae9

It was stuck here forever and in the sign in screen of the open-to-all page forever.
Finally, when I hit "Done", I got

1900-08-19 14:14:23.000000 -0800	emission	 [iOS Auth] 03:14:23: Authorization error: The operation couldn’t be completed. (org.openid.appauth.general error -4.)
1900-08-19 14:14:23.000000 -0800	emission	 DEBUG: [iOS Auth] 03:14:23: Authorization error: The operation couldn’t be completed. (org.openid.appauth.general error -4.)

@shankari
Copy link
Contributor Author

Going to focus on the

1900-08-19 14:13:26.000000 -0800	emission	 GOT URL:....

line since that is where I observed it hanging.

That line is actually from javascript.

angular.module('emission', ['ionic',
    'emission.controllers','emission.services', 'emission.plugin.logger',
    'emission.splash.customURLScheme', 'emission.splash.referral',
    'emission.splash.updatecheck',
    'emission.intro', 'emission.main'])

.run(function($ionicPlatform, $rootScope, $http, Logger,
    CustomURLScheme, ReferralHandler, UpdateCheck) {
  // TODO: Although the onLaunch call doesn't need to wait for the platform the
  // handlers do. Can we rely on the fact that the event is generated from
  // native code, so will only be launched after the platform is ready?
  CustomURLScheme.onLaunch(function(event, url, urlComponents){
    console.log("GOT URL:"+url);
    // alert("GOT URL:"+url);

So we are getting called back with the URL.
Looking at the native code, though, we do see the message from doAuthWithAutoCodeExchange

Initiating authorization request with scope: openid profile email offline_access

but we don't see the callback

[self logMessage:@"Got authorization tokens. Access token: %@", authState.lastTokenResponse.accessToken];

@shankari
Copy link
Contributor Author

The native plugin interface BEMJWTAuth.m calls handleNotification on applicationLaunchedWithUrl. Could this error message be the key?

1900-08-19 13:13:26.000000 -0800	emission	 [iOS Auth] 02:13:26: OpenIDAuth Error: handleNotification should not be called!

@shankari
Copy link
Contributor Author

It certainly looks like it. It looks like the OpenIDAuth code does not expect the custom URL callback to be called through applicationLaunchedWithUrl

-(void)handleNotification:(NSNotification *)notification
{
    [self logMessage:@"OpenIDAuth Error: handleNotification should not be called!"];
}

Dunno how it is supposed to be called.
Going to experiment with the emulator in different versions...

@shankari
Copy link
Contributor Author

Ok, so this is 100% reproducible in the emulator as well. I used a 9.2 emulator and tried to log in.

Messages:

2018-08-19 20:59:20.921 emission[98384:8434274] [iOS Auth] 03:59:20: Initiating authorization request with scope: openid profile email offline_access
2018-08-19 20:59:20.921 emission[98384:8434274] DEBUG: [iOS Auth] 03:59:20: Initiating authorization request with scope: openid profile email offline_access
2018-08-19 21:01:27.670 emission[98384:8434274] [iOS Auth] 04:01:27: OpenIDAuth Error: handleNotification should not be called!
2018-08-19 21:01:27.670 emission[98384:8434274] DEBUG: [iOS Auth] 04:01:27: OpenIDAuth Error: handleNotification should not be called!
2018-08-19 21:01:34.865 emission[98384:8434274] onLaunch method from external function called
2018-08-19 21:01:34.865 emission[98384:8434274] GOT URL:emission.auth://oauth2redirect?

@Andrew-Tan, did you try on an emulator running an older version of iOS?
e.g. https://stackoverflow.com/questions/4262018/xcode-simulator-how-to-run-older-ios-version#8477254

@shankari
Copy link
Contributor Author

shankari commented Aug 20, 2018

If I run the exact same code against an emulator running 11.4, everything works and the log messages are:

2018-08-19 21:07:54.720150-0700 emission[99691:8478085] [iOS Auth] 09:07:54: Initiating authorization request with scope: openid profile email offline_access
2018-08-19 21:07:54.808918-0700 emission[99691:8480902] [AXRun-PID] Client requesting unsuspension of PID:-1 Name:<redacted>
2018-08-19 21:08:01.886652-0700 emission[99691:8478085] active
2018-08-19 21:08:01.886903-0700 emission[99691:8478085] PushPlugin skip clear badge
2018-08-19 21:08:02.160178-0700 emission[99691:8478085] [MC] Reading from private effective user settings.
2018-08-19 21:08:17.405790-0700 emission[99691:8478085] [App] if we're in the real pre-commit handler we can't actually add any new fences due to CA restriction
2018-08-19 21:08:28.479491-0700 emission[99691:8478085] [iOS Auth] 09:08:28: Got authorization tokens. Access token: ....

Note that we do get the Got authorization tokens message that we were expecting earlier.

Note also that we don't get the handleNotification and GOT URL messages. So in iOS 11+, the AppAuth code intercepts the callback in some way that it doesn't in previous versions of iOS. Or maybe in previous versions of iOS, cordova intercepts the callback in some way that doesn't allow AppAuth to reach it.

@Andrew-Tan
Copy link
Contributor

Hi @shankari, I didn't run it in the older version because I think I'll get the same error as yours.

I'm searching for this issue on the web and found this interesting article: https://medium.com/@jlchereau/stop-using-inappbrowser-for-your-cordova-phonegap-oauth-flow-a806b61a2dc5

Could you take a look?

@shankari
Copy link
Contributor Author

@Andrew-Tan do you know the mechanism by which the AppAuth library gets back the accessToken? I assumed it was that the redirectURL would be called, and since emission.auth is registered as one of the URLs supported by the emission application, it would open the app, and get the value.

But I just added a breakpoint to CDVAppDelegate.m

- (BOOL)application:(UIApplication*)application openURL:(NSURL*)url sourceApplication:(NSString*)sourceApplication annotation:(id)annotation
{

and it didn't trigger when running on a 11.4 emulator.

@shankari
Copy link
Contributor Author

Ah, in iOS10, the signature for openURL changed.
https://stackoverflow.com/a/39447154

// Options are specified in the section below for openURL options. An empty options dictionary will result in the same
// behavior as the older openURL call, aside from the fact that this is asynchronous and calls the completion handler rather
// than returning a result.
// The completion handler is called on the main queue.
- (void)openURL:(NSURL*)url options:(NSDictionary<NSString *, id> *)options completionHandler:(void (^ __nullable)(BOOL success))completion NS_AVAILABLE_IOS(10_0) NS_EXTENSION_UNAVAILABLE_IOS("");

@Andrew-Tan
Copy link
Contributor

In general, for example, the url emission.auth://oauth2redirect?state=n_XnEhZM6d3VNPvOQutFVve3zwtra6oQ6e6Gm-W-fAA&code=uss.cT8CftoN2_Ft527TPuR9qdh1W7sL8F9JbJxIdM0FZ1I.5f575a3f-13d3-4f82-a907-9a858a010b12.29327dfe-bbca-4681-887b-c8c40f3f2ae9 should be passed down to the AppAuth library, and it'll use the code to negotiate access token with the auth server.

@shankari
Copy link
Contributor Author

@Andrew-Tan yeah but it should be passed down by calling openURL (because emission has registered for the emission.auth protocol). The problem is that openURL in the cordova app delegate is called for iOS 9.2 but not for iOS 11.4.

@shankari
Copy link
Contributor Author

@Andrew-Tan ok I know what is going on.
The key is here: (note that we are on version 0.91.0)

    <framework src="AppAuth" type="podspec" spec="~> 0.91.0"/>

https://github.com/openid/AppAuth-iOS/blob/0.91.0/Source/iOS/OIDAuthorizationUICoordinatorIOS.m#L91

Note that for iOS version 11+, the library does not use the URL redirection for the callback. Instead, it specifies a completionHandler that is called directly, and in turn, calls resumeAuthorizationFlowWithURL.

This is the flow that you tested, and it works.

For iOS 9 -11, though, it opens safari directly with the request URL - there is no completionHandler defined.

      SFSafariViewController *safariVC =
[[[self class] safariViewControllerFactory] safariViewControllerWithURL:requestURL];

In this case, once the authentication is complete, Safari navigates to the redirect URL, with protocol emission.auth in this case. This opens up the app with the URL, and somebody needs to handle it.

I don't see the AppAuth plugin handling it. e-mission/Cordova do handle it, and call handleNotification in your code, but you ignore it.
https://github.com/e-mission/cordova-jwt-auth/blob/master/src/ios/OpenIDAuth.m#L92

I am not sure what AppAuth recommends for handling the callback URL in iOS 9 - 11. However, from an e-mission perspective, if you implement handleNotification in OpenIDAuth instead of ignoring it, it should work.

@shankari
Copy link
Contributor Author

@Andrew-Tan looking at the README, it certainly seems like you need to handle URL based redirects as well (see Handling the Redirect under https://github.com/openid/AppAuth-iOS/blob/619bb7c7d5f83cc2ed19380d425ca8afa279644c/README.md#authorizing-ios)

You should be able to test your changes by using an emulator running an older version.

@shankari
Copy link
Contributor Author

shankari commented Aug 20, 2018

wrt #30 (comment), I read the article and it looks like what AppAuth supports is consistent with their recommendation, even for iOS9. They are using SafariViewController, with a customURL ('emission.auth` in our case) as the redirectURL.

Note that at the end of that post, they say that you need to handleOpenURL? That is the part that you are missing. In the case of e-mission, handleOpenURL gets redirected to handleNotification. You need to handle that instead of ignoring it.

Note that while testing on iOS 9.2, I got the following message (#30 (comment), #30 (comment)).

So handleNotification is getting called correctly, you just need to handle it correctly.

2018-08-19 21:01:27.670 emission[98384:8434274] [iOS Auth] 04:01:27: OpenIDAuth Error: handleNotification should not be called!

@uwtcat
Copy link

uwtcat commented Aug 22, 2018

@Andrew-Tan
Have you been able to modify the handleNotification to fix the issue?

Sorry for the push, we have participants in the study who are ready to go!

@shankari
Copy link
Contributor Author

@Andrew-Tan @uwtcat note that after Andrew makes the fix, because it is a native code change on iOS, it also requires passing through app store review, which can take upto a week.

Current app store review time average is 3 days.
http://appreviewtimes.com/

@Andrew-Tan
Copy link
Contributor

@shankari How can I get the redirect URL from NSNotification?

@Andrew-Tan
Copy link
Contributor

@shankari Nevermind... I've figured it out. Just made the change and created a pull request.

@shankari
Copy link
Contributor Author

While testing this in a build that also included a fix for https://github.com/e-mission/e-mission-phone/issues/443, I ran into a several issues with ionic deploy - the feature that allows us to "skin" the UI efficiently on older versions of iOS (iOS 9, iOS 10).

When I filed an issue with ionic, the response was to point me to https://github.com/e-mission/e-mission-phone/issues/443, which says that

iOS Note: In order to allow for new features, live deploys on iOS 10.2 and lower are no longer supported. Devices running iOS 10.2 will operate normally but will be restricted to the currently installed native version. The total distribution of iOS devices running iOS 10.2 or lower is approximately 7.3%.

I am going to revert to the old version of ionic while the community debates what to do about versions. @uwtcat I will publish a new build on TestFlight today.

@shankari
Copy link
Contributor Author

Ok, so I reverted to the old version of ionic deploy and tested on iOS 9.2, iOS 10.2 and iOS 11.4. In all cases, the app was installed correctly, the UI-only update was deployed properly, and I got to the screen to log in to the AppAuth server.

Unfortunately, https://accounts.open-to-all.com/ currently has a gateway error, so I can't test further.

However, I have uploaded a new version (1.0.4) to TestFlight. @uwtcat and @Andrew-Tan you can test this version over the weekend. Please update this issue with your findings...

@shankari
Copy link
Contributor Author

shankari commented Sep 4, 2018

Fixed in #31

@shankari shankari closed this as completed Sep 4, 2018
@shankari
Copy link
Contributor Author

shankari commented Sep 4, 2018

@uwtcat @Andrew-Tan Very surprisingly, the update went through app store review in one day. Maybe they are catching up with their backlog after labor day weekend?

You can resume beta testing...

App Store Connect | 1:15 PM (38 minutes ago)

The following app has been approved for the App Store:

App Name: emTripLog

App Version Number: 1.0.6
App Type: iOS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants