You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by ephemer <gi...@git.apache.org> on 2016/04/21 22:27:20 UTC
[GitHub] cordova-plugin-inappbrowser pull request: CB-11136: Fix OAuth by p...
GitHub user ephemer opened a pull request:
https://github.com/apache/cordova-plugin-inappbrowser/pull/162
CB-11136: Fix OAuth by preventing InAppBrowser from blocking WKWebView thread
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 that falsely looks like an error has on occurred.
See https://issues.apache.org/jira/browse/CB-11136 for details.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ephemer/cordova-plugin-inappbrowser master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-plugin-inappbrowser/pull/162.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #162
----
commit 04091fde737519c149e7ad6316971cb6b490c5b3
Author: Geordie J <ge...@gmail.com>
Date: 2016-04-21T20:22:38Z
CB-11136: Fix OAuth by preventing InAppBrowser from blocking WKWebView thread
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser pull request: CB-11136: Fix OAuth by p...
Posted by ephemer <gi...@git.apache.org>.
Github user ephemer commented on the pull request:
https://github.com/apache/cordova-plugin-inappbrowser/pull/162#issuecomment-213118009
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser issue #162: CB-11136: Fix OAuth by preventing In...
Posted by jlchereau <gi...@git.apache.org>.
Github user jlchereau commented on the issue:
https://github.com/apache/cordova-plugin-inappbrowser/pull/162
I have experienced the defect described here with oAuth flows but considering [Google deprecation of webviews for oAuth flows](https://developers.googleblog.com/2016/08/modernizing-oauth-interactions-in-native-apps.html), 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser pull request: CB-11136: Fix OAuth by p...
Posted by jcesarsh <gi...@git.apache.org>.
Github user jcesarsh commented on the pull request:
https://github.com/apache/cordova-plugin-inappbrowser/pull/162#issuecomment-213116268
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.
Maybe adding a new "modal" option being "yes" as default so users will have the same behavior and the ones with CB-11136 problem can use the "modal=no"
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser issue #162: CB-11136: Fix OAuth by preventing In...
Posted by dotNetkow <gi...@git.apache.org>.
Github user dotNetkow commented on the issue:
https://github.com/apache/cordova-plugin-inappbrowser/pull/162
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!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser issue #162: CB-11136: Fix OAuth by preventing In...
Posted by shazron <gi...@git.apache.org>.
Github user shazron commented on the issue:
https://github.com/apache/cordova-plugin-inappbrowser/pull/162
I like the implementation in #187 for the `show` method, what do you think?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser issue #162: CB-11136: Fix OAuth by preventing In...
Posted by jcesarmobile <gi...@git.apache.org>.
Github user jcesarmobile commented on the issue:
https://github.com/apache/cordova-plugin-inappbrowser/pull/162
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser issue #162: CB-11136: Fix OAuth by preventing In...
Posted by jcesarmobile <gi...@git.apache.org>.
Github user jcesarmobile commented on the issue:
https://github.com/apache/cordova-plugin-inappbrowser/pull/162
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser issue #162: CB-11136: Fix OAuth by preventing In...
Posted by dotNetkow <gi...@git.apache.org>.
Github user dotNetkow commented on the issue:
https://github.com/apache/cordova-plugin-inappbrowser/pull/162
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. Developer's would have to have 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:
`inAppBrowserRef = window.open("www.google.com", '_blank', 'toolbar=yes');`
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!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser pull request: CB-11136: Fix OAuth by p...
Posted by Chun-Yang <gi...@git.apache.org>.
Github user Chun-Yang commented on the pull request:
https://github.com/apache/cordova-plugin-inappbrowser/pull/162#issuecomment-214800395
@ephemer Man you are genius! Thank you SO~~~~~~~~~~~~~~ much!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser issue #162: CB-11136: Fix OAuth by preventing In...
Posted by ephemer <gi...@git.apache.org>.
Github user ephemer commented on the issue:
https://github.com/apache/cordova-plugin-inappbrowser/pull/162
Let's merge #187!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser pull request: CB-11136: Fix OAuth by p...
Posted by cordova-qa <gi...@git.apache.org>.
Github user cordova-qa commented on the pull request:
https://github.com/apache/cordova-plugin-inappbrowser/pull/162#issuecomment-219916544
Cordova CI Build has completed successfully.
**Commit** - [Link](https://github.com/apache/cordova-plugin-inappbrowser/pull/162/commits/04091fde737519c149e7ad6316971cb6b490c5b3)
**Dashboard** - [Link](http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/5/)
| Builder Name | Console Output | Test Report | Device Logs |
| :---: | :---: | :---: | :---: |
| [Windows 8.1 Store]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/5//label=windows-slave,platformName=windows-8.1-store/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/5//label=windows-slave,platformName=windows-8.1-store/console) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/5//label=windows-slave,platformName=windows-8.1-store/testReport/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/5//label=windows-slave,platformName=windows-8.1-store/artifact/) |
| [Windows 10 Store]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/5//label=windows-slave,platformName=windows-10-store/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/5//label=windows-slave,platformName=windows-10-store/console) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/5//label=windows-slave,platformName=windows-10-store/testReport/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/5//label=windows-slave,platformName=windows-10-store/artifact/) |
| [Windows 8.1 Phone]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/5//label=windows-slave,platformName=windows-8.1-phone/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/5//label=windows-slave,platformName=windows-8.1-phone/console) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/5//label=windows-slave,platformName=windows-8.1-phone/testReport/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/5//label=windows-slave,platformName=windows-8.1-phone/artifact/) |
| [iOS]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/5//label=mac-slave,platformName=ios/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/5//label=mac-slave,platformName=ios/console) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/5//label=mac-slave,platformName=ios/testReport/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/5//label=mac-slave,platformName=ios/artifact/) |
| [Android Mac]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/5//label=mac-slave,platformName=android/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/5//label=mac-slave,platformName=android/console) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/5//label=mac-slave,platformName=android/testReport/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/5//label=mac-slave,platformName=android/artifact/) |
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser issue #162: CB-11136: Fix OAuth by preventing In...
Posted by alesveselka <gi...@git.apache.org>.
Github user alesveselka commented on the issue:
https://github.com/apache/cordova-plugin-inappbrowser/pull/162
Good job! So far only solution that fixed the problem for me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser pull request: CB-11136: Fix OAuth by p...
Posted by ephemer <gi...@git.apache.org>.
Github user ephemer commented on the pull request:
https://github.com/apache/cordova-plugin-inappbrowser/pull/162#issuecomment-214827853
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 `modal: false` is a bit like providing the option `worksAsItIsSupposedTo: true`. If anything we could provide an option `useStrictlyModalBrowser: true` or similar to opt-in to the new WKWebView behaviour.
@jcesarsh what do you think, looking at this in that light?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser pull request #162: CB-11136: Fix OAuth by preven...
Posted by ephemer <gi...@git.apache.org>.
Github user ephemer closed the pull request at:
https://github.com/apache/cordova-plugin-inappbrowser/pull/162
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org