-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
CB-11136: Fix OAuth by preventing InAppBrowser from blocking WKWebView thread #162
Conversation
I don't think it's a good idea to switch from presenting a modal view to adding a subview, it might behave differently in some cases. |
I agree that it feels kind of wrong. The problem is that many users (myself included) have no control over the call site for window.open - this Cordova and it's meant to be cross-platform; adding arbitrary Cordova-specific options into window.open for public OAuth packages seems worse in my eyes. I guess we could wrap window.open in our own wrapper function but none of this seems ideal. Other than the fact it's not the normal iOS way of doing things, is there anything fundamentally problematic with the approach in this PR? |
@ephemer Man you are genius! Thank you SO~~~~~~~~~~~~~~ much! |
Thank you, my pleasure. I have thought about this a bit longer.. The main issue is that recent Cordova changes, namely introducing WKWebView, have broken this plugin for a significant use case, namely OAuth. The changes suggested here put the functionality back to how it was before the internal changes broke it. I'd prefer a different solution but until we have that I think it's important that the plugin does what it's supposed to. Making these changes depend on an option like @jcesarsh what do you think, looking at this in that light? |
Good job! So far only solution that fixed the problem for me. |
hi @shazron - any way your team could consider this? OAuth is a major reason folks use InAppBrowser, and for years now it's been feasible and "supported" in a sense with UIWebView. One alternative is to use the "toolbar=yes" option, which shows a "done" link that will close the browser. Developers would need a self hosted page that tells the user to tap the link, like "Successfully connected! Please tap Done to continue". Not the best usability but possible:
However, I once had Apple reject my app with this option in place - they didn't like that it obviously loading "something" outside of my app. So that's probably out :( Thanks for considering. The performance gains from WKWebView are immense, so I'd hate to miss out on using it! |
Starting in October 20th google won't allow to use WebViews (like inAppBrowser) for OAuth, so I think that's another reason to not change the behaviour of the plugin just to fix the OAuth https://developers.googleblog.com/2016/08/modernizing-oauth-interactions-in-native-apps.html |
Thanks for sharing. Really good to know - someone needs to start building a "Google Sign-In" plugin! ;) Naturally, I disagree on this, what with an infinite amount of 3rd party apps, sites, and APIs to connect accounts to, but I understand. Thanks! |
There is a google+ plugin https://github.com/EddyVerbruggen/cordova-plugin-googleplus BTW, I'm not saying that this shouldn't be merged, I'm saying that it shouldn't change the default modal behaviour, this should be added in a way that you can configure it to be modal or not, being modal the default as it is now. |
I like the implementation in #187 for the |
I have experienced the defect described here with oAuth flows but considering Google deprecation of webviews for oAuth flows, I have decided to pursue another route explained here: https://medium.com/@jlchereau/stop-using-inappbrowser-for-your-cordova-phonegap-oauth-flow-a806b61a2dc5. I am looking forward to your opinion on this alternative especially regarding any security flaws I might have overlooked. |
Let's merge #187! |
Presenting a UIViewController in front of the main Cordova UIViewController containing a WKWebView causes the WKWebView thread to block entirely. This means that the OAuth login flow pauses on a blank screen on success, which is bad for the user experience.
See https://issues.apache.org/jira/browse/CB-11136 for details.