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

CB-14013: (android) Change the InAppBrowser to allow custom schemes for oAuth #263

Merged

Conversation

SailingSteve
Copy link
Contributor

@SailingSteve SailingSteve commented Apr 5, 2018

Platforms affected

Android

What does this PR do?

Allows oAuth redirects that return to Android with custom schemes like "twitter://code=nnn" to be
acted on by the InAppBrowser. This is needed for oAuth 2.0 flow.

This PR builds on /pull/99 and /pull/261 and also builds on a good suggestion from NGumby and comments about viability from infil00p

Here is my actual working JavaScript app code from https://github.com/WeVote/WebApp/blob/develop/src/js/components/Twitter/TwitterSignIn.jsx

          inAppBrowserRef.addEventListener('customscheme', function (event) {
            oAuthLog("customscheme: ", event.url);
            TwitterSignIn.handleTwitterOpenURL(event.url);

            inAppBrowserRef.close();
          });

You configure the allowable schemes in a comma separated list in a new preference. This implements whitelisting for the allowed schemes, but doesn't use the Cordova Whitelist plugin -- this approach seemed much simpler and safer.

See my actual config.xml at https://github.com/wevote/WeVoteCordova/blob/master/config.xml

   <preference name="AllowedSchemes" value="mycoolapp,wevotetwitterscheme" />

What testing has been done on this change?

It has been tested and is working in my app https://github.com/wevote/WeVoteCordova. I tested to make sure that single and multiple schemes in the "AllowedSchemes" preference worked as expected.

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.

…ing.

Newtest in shouldOverrideUrlLoading, to allow whitelisted custom schemes
like"mycoolapp://"

inappbrowser.js: Added "customscheme" channel.
…ing.

Newtest in shouldOverrideUrlLoading, to allow whitelisted custom schemes
like"mycoolapp://"

inappbrowser.js: Added "customscheme" channel.
…ing.

Newtest in shouldOverrideUrlLoading, to allow whitelisted custom schemes
like"mycoolapp://"

inappbrowser.js: Added "customscheme" channel.
…ing of

"AllowedSchemes" in a new preference configuration item.
There is a new check in shouldOverrideUrlLoading, to allow whitelisted
custom schemes like "mycoolapp://"

inappbrowser.js: Added "customscheme" channel.
…ing of

"AllowedSchemes" in a new preference configuration item.
There is a new check in shouldOverrideUrlLoading, to allow whitelisted
custom schemes like "mycoolapp://"

inappbrowser.js: Added "customscheme" channel.
check for whitelisting custom schemes via a new "AllowedSchemes"
preference configuration item.  Allows custom schemes like
"mycoolapp://" or "wevotetwitterscheme://"

In file inappbrowser.js: Added new "customscheme" channel.
check for whitelisting custom schemes via a new "AllowedSchemes"
preference configuration item.  Allows custom schemes like
"mycoolapp://" or "wevotetwitterscheme://"

In file inappbrowser.js: Added new "customscheme" channel.
SailingSteve added a commit to SailingSteve/WebApp that referenced this pull request Apr 5, 2018
Added InAppBrowser support for the oAuth redirect scheme, this relies on
a PR that I made to Cordova to allow schemes
apache/cordova-plugin-inappbrowser#263
which they are likely to approve, but if not we can work off of the fork.
Moved all the iOS twitter scheme processing out of Application.js and into
TwitterSignIn.jsx, and enhanced it for Android.  Some small styling changes
to remove the "ios7plus-spacer" which is not needed for Android, since
they don't make the top line menu space available for the application.
Made some more changes to use logging.js based on the new defines in
config.js.
Big changes to www/index.html Content Security Policy, debugged to find
out what we acutally need.
@SailingSteve
Copy link
Contributor Author

PR is here #263

SailingSteve added a commit to SailingSteve/WebApp that referenced this pull request Apr 6, 2018
Added InAppBrowser support for the oAuth redirect scheme, this relies on
a PR that I made to Cordova to allow schemes
apache/cordova-plugin-inappbrowser#263
which they are likely to approve, but if not we can work off of the fork.
Moved all the iOS twitter scheme processing out of Application.js and into
TwitterSignIn.jsx, and enhanced it for Android.  Some small styling changes
to remove the iOS
@infil00p infil00p merged commit 50db5c4 into apache:master Apr 12, 2018
@timbru31
Copy link
Member

timbru31 commented May 2, 2018

@SailingSteve possible to add support for staging+mycoolapp? Currently the plus sign is excluded from your regexp (^[a-z]*://.*?$). Numbers would be great, too. Our URL scheme is d2d...

edit PR is #269

@wvengen
Copy link
Contributor

wvengen commented Jun 27, 2018

Wow, this is huge as it (finally) allows 'events' from the inAppBrowser to the Cordova app. Its use goes way beyond OAuth 2. Thanks a lot!

As a sidenote: I do see a use-case for allowing http and https in AllowedSchemes: deciding in the Cordova app whether a URL needs to be loaded or not. This would allow for more easily deciding which URLs are opened in the app, and which URLs are opened in the system web browser.

@boynet
Copy link

boynet commented Nov 27, 2018

I started testing it and I ran into bug, the event only working on the first time so a page with this simple html loaded into inAppBrowser:

config:
<preference name="AllowedSchemes" value="whatsapp,twitter" />
....
html:
<html>
<body>
<a href="whatsapp://send?text=test">click to send</a>
</body>
</html>

event:
        inAppBrowser.addEventListener('customscheme', function (event) {
            console.log(event);
        });

the first click everything is fine, the second click no longer trigger the event

@janpio
Copy link
Member

janpio commented Nov 27, 2018

Can you please create a new issue @boynet? This will be forgotten here, just link to the PR in your description for context. Thanks for taking the time to report this!

@customautosys
Copy link

Why not just make it launch an intent to open the default system app for the custom scheme? Something like https://github.com/ljcljc/cordova-plugin-inappbrowser

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

Successfully merging this pull request may close these issues.

7 participants