Deeplinks/custom schema/SEP7 links for iOS, Android, Electron by cjb · Pull Request #17574 · keybase/client

cjb · 2019-05-16T06:27:37Z

Deeplinks! Actually, they're not called deeplinks. Deeplinks are when https://keybase.io/something redirects to a place inside our app. This PR is a "custom schema handler" providing a custom URL scheme that launches our app -- in this case, web+stellar:*, which is the prefix for SEP7 Stellar links.

This was pretty gnarly. I'll do another round of testing once it's in master and there are builds out. You need a build from the branch to test it since the integration that allows the browser to know what to do with keybase:// links is done at the OS-level through the build install.

There are so many ways to find out whether we're dealing with a launch from a URL. For each platform, we need a way to handle a hot link while the app is running, and a cold link where the app is started because of a URL. It looks like this:

iOS: Add the scheme in Info.plist, then Linking.addEventListener('url' ..), then your hot or cold callback runs any time there's a URL.
Android: Hot callback is the same as above, but cold requires a call to Linking.getInitialURL().
macOS: You call SafeElectron.getApp().on('open-url', ..) before ready (in will-finish-launching) to get callbacks to the main thread. But the main thread can't dispatch actions, so we need a way to communicate to Redux from there. In the hot case you can use sendToMainWindow('dispatchAction', ..) directly to dispatch an action from there with the link, but in the cold case the callback will run before Redux is live yet. In that case I'm storing away the launch URL in a new startupURL var, then waiting until startup's finished and firing `launchStartupURLIfPresent to check that var and dispatch an action.
Windows/Linux: Same as macOS for hot callbacks, but for cold the only way to get them is from process.argv in the main process. So launchStartupURLIfPresent checks argv for the case where the app wasn't running already, as does the second-instance-handler which fires when a URL request results in launching a second app.

cjb · 2019-05-23T15:53:27Z

@keybase/react-hackers This is ready for review!

cjb · 2019-05-24T14:10:54Z

Switched from keybase:// to web+stellar: (no slashes), which is the real URL scheme for SEP7.

…731-sep7-links

cjb · 2019-06-14T23:00:47Z

@keybase/react-hackers Bump for review, CC @heronhaye since there's a run_keybase change.

heronhaye · 2019-06-17T13:49:42Z

So is the idea that you can call /opt/keybase/Keybase keybase://whatever and instead of opening a new Keybase (if Keybase is already open) it'll tell the existing process the link? But if Keybase is not open, it'll start all keybase processes and then Keybase will know about the link? (where Keybase refers to the electron process).

Does there need to be a change to packaging/linux/keybase.desktop to register the URL?

Finally I'd prefer if we used an option to run_keybase instead, e.g. run_keybase --url .... I can make that change if you like as well. We'll need to work with the Arch maintainers to get this working in the official packages as well (I don't see an obvious unilateral solution on how to handle the case where Keybase isn't running yet).

cjb · 2019-06-17T18:03:33Z

@heronhaye Thanks for looking!

So is the idea that you can call /opt/keybase/Keybase keybase://whatever and instead of opening a new Keybase (if Keybase is already open) it'll tell the existing process the link? But if Keybase is not open, it'll start all keybase processes and then Keybase will know about the link? (where Keybase refers to the electron process).

Yup, exactly.

Does there need to be a change to packaging/linux/keybase.desktop to register the URL?

Oops, missed including that, just pushed it.

Finally I'd prefer if we used an option to run_keybase instead, e.g. run_keybase --url .... I can make that change if you like as well. We'll need to work with the Arch maintainers to get this working in the official packages as well (I don't see an obvious unilateral solution on how to handle the case where Keybase isn't running yet).

I'd like that too, but I don't see how to accomplish it. The keybase.desktop change passes the URL like this:

-Exec=run_keybase
+Exec=run_keybase %U

and so what we'd want to do is something like Exec=run_keybase {url && '--url %U'} and I just don't think the syntax is there. Perhaps the best option would be to install a second desktop entry just for link-handling, and that registers the schema handler and points to a shell script, and that shell script takes the url and calls run_keybase --url with it. Or, that new shell script could bypass run_keybase entirely and do the extra app launch itself.

I didn't do this because it seemed wrong to have an extra .desktop file, but I'm open to looking into it if you prefer it a lot.

heronhaye · 2019-06-17T18:15:15Z

packaging/linux/run_keybase

 KEYBASE_AUTOSTART="${KEYBASE_AUTOSTART:-0}"
 KEYBASE_KILL="${KEYBASE_KILL:-0}"
+
+if [[ -z "$1" ]]; then


Should be -n?

Actually better to check $# anyway. Though I think this being in this location messes with the arguments -h, etc. that are interpreted as the url here. Also, I think it should be "$@" instead, see https://github.com/koalaman/shellcheck/wiki/SC2068.

@heronhaye Does if [[ $# -eq 1 && -n "$1" && "$1" == web+stellar:* ]]; then work for you?

Oh, can we ensure there will always be a single web+stellar link (if a link is present)? i.e., %u instead of %U?

If so, that works for me. (Ignore my other comment). Can likely be if [[ $# -eq 1 && "$1" == web+stellar:* ]]; then , but lmk when pushed and I'll try it out. Thanks. Could use ~= for regex match instead of glob as well.

Oh, weird, I don't know how you could get multiple web+stellar URLs at once. We don't have to support that. Moved over to %u as you suggest.

heronhaye · 2019-06-17T19:18:43Z

@cjb Sorry I think I forgot to post my comment before. Good point, we can leave it as run_keybase %U then, as long as it doesn't mess with the other cmd options run_keybase has like -h and -a. To be honest, I'm not sure how these are templated in, but it may be easier to do something like run_keybase --url=dummy%u, although it's a bit messy for sure.

cjb · 2019-06-17T19:21:59Z

@heronhaye Ah, they're templated in outside of our control -- a web browser clicks on a web+stellar: link, the user's asked if they want to open it in Keybase since the OS knows about our registration of the custom scheme, then run_keybase web+stellar:.. is exec'd, and it's not us doing the exec.

heronhaye · 2019-06-17T19:54:27Z

So what happens if Keybase isn't running yet and you click a link? Seems the service wouldn't start in this case to support the GUI.

heronhaye · 2019-06-17T19:57:09Z

Unfortunately there's no foolproof way to check this across systemd, non-systemd, different setups, etc...but maybe you could check pgrep -u $USER Keybase? (Assuming no one is running GUI without the service). And if that check fails, do a normal startup but pass the link to Keybase.

heronhaye · 2019-06-17T20:20:31Z

Oh, not sure if you have an easy way to test different Linux systems but if you like I could try to do this as well. when passing urls to the Keybase binary works in master.

cjb · 2019-06-17T21:50:18Z

do a normal startup but pass the link to Keybase

Actually, that sounds tricky. How do we tell e.g. systemd to start keybase and also pass a new argument to the GUI?

The change to make it so that it does a normal startup if Keybase isn't already running sounds good, though. Even if it doesn't pass the link through. If we have to make it so that a first URL click when Keybase is stopped launches Keybase but doesn't pass through the link, and then the second click works to pass through the link, I think that'd be okay. The case where Keybase is running all the time is the one we really support fully.

cjb · 2019-06-17T22:03:12Z

@keybase/react-hackers CC @keybase/picnicsquad

Bump on a review for the JS changes, while @heronhaye and I work on the Linux run_keybase.

buoyad · 2019-06-18T13:14:53Z

shared/android/app/src/main/AndroidManifest.xml

          <data android:mimeType="audio/*" />
          <data android:mimeType="application/*" />
        </intent-filter>
+        <intent-filter android:label="filter_react_native">


It seems like the user sees this label before they switch into Keybase

The icon and label that are set in a component's <intent-filter> are shown to the user whenever that component is presented as an option to fulfill an intent.

Should this be something like "Sign with Keybase"?

Sounds good, switched to "Open with Keybase", since you can do payments as well as signatures.

buoyad · 2019-06-18T13:32:14Z

shared/desktop/app/node.desktop.tsx

    mainWindow && mainWindow.window.webContents.send('remoteWindowWantsProps', windowComponent, windowParam)
  })
+
+  SafeElectron.getIpcMain().on('launchStartupURLIfPresent', () => {


What do you think of making this something like reduxLaunched and stashing that in a module variable here? Then in on('open-url' you can look at that to decide whether to dispatch or stash the link.

Thanks, does 02c0446 look like what you're thinking?

heronhaye · 2019-06-18T13:53:44Z

Actually, that sounds tricky. How do we tell e.g. systemd to start keybase and also pass a new argument to the GUI?

We could for example set an environment variable in the systemd environment.

The change to make it so that it does a normal startup if Keybase isn't already running sounds good, though. Even if it doesn't pass the link through.

Ok, that sounds fine for now. As long as the user doesn't click a link and open up the GUI without the service or KBFS.

…731-sep7-links

cjb · 2019-06-18T20:31:33Z

@heronhaye Pushed an env var implementation that should do everything we talked about -- it's hard to test, would be good to start getting builds.

buoyad

Nice, tested desktop and iOS and got the link. JS/native LGTM 👍

heronhaye · 2019-06-18T21:12:01Z

Agree, if we can get a build in that respects KEYBASE_STARTUP_URL should be much easier to finish.

I made this diff with suggestions, let me know if you disagree with any of it

Don't override user stellar preference
Style, use -n instead of ! -z
When checking exit codes don't use [[ ]] (use this when using operators like = or -z, and also no need for $( ) if it's the only thing in the condition.
It's possible that systemctl is installed but user manager instance is not supported, so use same logic as other part of the code to detect if systemd user manager is supported
Move check to after arg parsing so KEYBASE_STARTUP_URL=whatever run_keybase -h prints help instead of opening Keybase.

diff --git a/packaging/linux/run_keybase b/packaging/linux/run_keybase
index f52131156a..bcf3402c61 100755
--- a/packaging/linux/run_keybase
+++ b/packaging/linux/run_keybase
@@ -182,14 +182,9 @@ check_for_url_scheme_launch() {
   # differentiate between a normal run_keybase invocation and a URL launch
   # by checking KEYBASE_STARTUP_URL.
 
-  # Check that we control the web+stellar scheme.
-  if command -v xdg-mime &> /dev/null; then
-    xdg-mime default keybase.desktop x-scheme-handler/web+stellar
-  fi
-
   # If Keybase is already running, then pass it the link and exit.
-  if [[ ! -z "${KEYBASE_STARTUP_URL:-}" ]]; then
-      if  [[ $(pgrep -u $USER 'Keybase$') ]]; then
+  if [[ -n "${KEYBASE_STARTUP_URL-}" ]]; then
+      if pgrep -u "$USER" 'Keybase$'; then
           if command -v Keybase &> /dev/null; then
               KEYBASE_STARTUP_URL="$KEYBASE_STARTUP_URL" Keybase &
           else
@@ -197,9 +192,11 @@ check_for_url_scheme_launch() {
           fi
           exit
       else
-          if command -v systemctl &> /dev/null; then
+          if keybase ctl wants-systemd &> /dev/null; then
               systemctl --user set-environment KEYBASE_STARTUP_URL="$KEYBASE_STARTUP_URL"
           fi
+          # NOTE: If the user is not using systemd, the injected environment
+          #       variable is passed through to the GUI automatically.
       fi
   fi
 
@@ -247,8 +244,6 @@ KEYBASE_NO_KBFS="${KEYBASE_NO_KBFS:-0}"
 KEYBASE_AUTOSTART="${KEYBASE_AUTOSTART:-0}"
 KEYBASE_KILL="${KEYBASE_KILL:-0}"
 
-check_for_url_scheme_launch
-
 # NOTE: Make sure to update the Linux User Guide doc if you change this!
 #   http://keybase.io/docs/linux-user-guide
 while getopts "afghk" flag; do
@@ -262,6 +257,8 @@ while getopts "afghk" flag; do
     esac
 done
 
+check_for_url_scheme_launch
+
 init
 # Always stop any running services. With systemd, we could've decided to just
 # `start` services and no-op if they're already running, however:

heronhaye · 2019-06-18T21:58:41Z

packaging/linux/systemd/keybase.gui.service

 # /usr/bin depending on the distro
-ExecStartPost=/usr/bin/env systemctl --user unset-environment KEYBASE_AUTOSTART
+# Also clear out KEYBASE_STARTUP_URL, for the same reasons.
+ExecStartPost=/usr/bin/env systemctl --user unset-environment KEYBASE_AUTOSTART KEYBASE_STARTUP_URL


cjb · 2019-06-19T03:57:04Z

@heronhaye Should be ready to go now, thanks!

I had to switch back from env var to argv, because of the behavior of Electron when a second instance of the app starts. When the second instance fails to get the single instance lock, it results in the second-instance handler on the first instance firing, which passes through the argv of the second instance back to the first instance.

If we want to communicate the startup URL via env vars, we need a way to send that second instance's env to the first instance, and Electron doesn't let you do that. Surya found a recent GitHub issue asking for it:

electron/electron#18434

So, if Electron makes this change to allow passing arbitrary data back, perhaps we'll end up using it then.

(I suppose contributing the upstream change ourselves would even be a nice thing to do, when we have some time.)

heronhaye · 2019-06-19T14:58:36Z

Ok, did a full build and looks OK. I fixed an off by one error in the JS though, throws a JS error when no url is provided. Small cleanup as well. Also, I forgot that pgrep needs the -f flag to do regex, fixed here too.

I gave a shot at doing a startup + url handle but found it non-trivial for systemd... can revisit later as you suggested. But I tested a dummy link in the browser and it works great.

diff --git a/packaging/linux/run_keybase b/packaging/linux/run_keybase
index 577273e650..a9bfab181f 100755
--- a/packaging/linux/run_keybase
+++ b/packaging/linux/run_keybase
@@ -121,11 +121,7 @@ start_background() {
       echo Launching Keybase GUI...
       gui_log="$logdir/Keybase.app.log"
       # Allow distributions to change the location of the gui as long as it's in PATH.
-      if command -v Keybase &> /dev/null; then
-          Keybase &>> "$gui_log" &
-      else
-          /opt/keybase/Keybase &>> "$gui_log" &
-      fi
+      "$KEYBASE" &>> "$gui_log" &
   fi
 }
 
@@ -174,6 +170,12 @@ init() {
 
   # Remove legacy envfiles; now stored in config directory by ctl init
   rm -f "$runtime_dir/keybase.env" "$runtime_dir/keybase.kbfs.env" "$runtime_dir/keybase.gui.env"
+
+  if command -v Keybase &> /dev/null; then
+    KEYBASE=Keybase
+  else
+    KEYBASE=/opt/keybase/Keybase
+  fi
 }
 
 check_for_url_scheme_launch() {
@@ -183,13 +185,9 @@ check_for_url_scheme_launch() {
   # by checking argv.
 
   # If Keybase is already running, then pass it the link and exit.
-  if [[ $# -eq 1 && "$1" == web+stellar:* ]]; then
-      if pgrep -u "$USER" 'Keybase$' &> /dev/null; then
-          if command -v Keybase &> /dev/null; then
-              Keybase $1 &
-          else
-              /opt/keybase/Keybase $1 &
-          fi
+  if [[ $# -eq 1 && "$1" =~ ^web+.* ]]; then
+      if pgrep -u "$USER" -f 'Keybase$' &> /dev/null; then
+          "$KEYBASE" "$1" &
           exit
       fi
   fi
@@ -251,9 +249,12 @@ while getopts "afghk" flag; do
     esac
 done
 
-check_for_url_scheme_launch $@
-
 init
+
+# Exit early if run caused by a URL scheme invocation and Keybase is alread
+# running; otherwise fall through to start Keybase.
+check_for_url_scheme_launch "$@"
+
 # Always stop any running services. With systemd, we could've decided to just
 # `start` services and no-op if they're already running, however:
 # 1) We still need to handle the case where services started outside systemd
diff --git a/shared/desktop/app/node.desktop.tsx b/shared/desktop/app/node.desktop.tsx
index f9831c789a..ff7531c983 100644
--- a/shared/desktop/app/node.desktop.tsx
+++ b/shared/desktop/app/node.desktop.tsx
@@ -73,7 +73,7 @@ const focusSelfOnAnotherInstanceLaunching = (_, commandLine) => {
 
   // The new instance might be due to a URL schema handler launch.
   logger.info('Launched with URL', commandLine)
-  if (commandLine.length > 0 && commandLine[1] && commandLine[1].startsWith('web+stellar:')) {
+  if (commandLine.length > 1 && commandLine[1] && commandLine[1].startsWith('web+stellar:')) {
     sendToMainWindow('dispatchAction', {payload: {link: commandLine[1]}, type: ConfigGen.link})
   }
 }
@@ -158,7 +158,7 @@ const createMainWindow = () => {
       // stash a startupURL to be dispatched when we're ready for it.
       sendToMainWindow('dispatchAction', {payload: {link: startupURL}, type: ConfigGen.link})
       startupURL = null
-    } else if (!isDarwin && process.argv.length > 0 && process.argv[1].startsWith('web+stellar:')) {
+    } else if (!isDarwin && process.argv.length > 1 && process.argv[1].startsWith('web+stellar:')) {
       // Windows and Linux instead store a launch URL in argv.
       sendToMainWindow('dispatchAction', {payload: {link: process.argv[1]}, type: ConfigGen.link})
     }

cjb · 2019-06-19T17:30:02Z

@heronhaye Thanks again! Pushed your diff and I'll merge in after CI.

heronhaye

LGTM! sorry for the environment-variable detour

cjb marked this pull request as ready for review May 23, 2019 15:53

cjb changed the title ~~WIP: Deeplinks for iOS, Android, Electron~~ Deeplinks for iOS, Android, Electron May 23, 2019

Rebase on master

32a2eeb

cjb force-pushed the cjb/DESKTOP-9731-sep7-links branch from d938441 to 32a2eeb Compare June 7, 2019 05:49

cjb added 4 commits June 14, 2019 15:47

fix up Linux startup

427ae13

remove debugging

d322938

Merge branch 'master' of github.com:keybase/client into cjb/DESKTOP-9…

086af84

…731-sep7-links

rename deeplink => schema handler launch

dab14fe

cjb changed the title ~~Deeplinks for iOS, Android, Electron~~ Deeplinks/custom schema/SEP7 links for iOS, Android, Electron Jun 14, 2019

add keybase.desktop change

95e96ef

heronhaye reviewed Jun 17, 2019

View reviewed changes

Feedback from Surya

294f56d

buoyad reviewed Jun 18, 2019

View reviewed changes

cjb added 4 commits June 18, 2019 12:25

Rename Android intent for SEP7

4bc33a7

reduxLaunched suggestion from Danny

02c0446

Try out Surya's KEYBASE_STARTUP_URL env var suggestion

5a50922

add to unset-environment too

1373c7e

Merge branch 'master' of github.com:keybase/client into cjb/DESKTOP-9…

8a7fd19

…731-sep7-links

buoyad approved these changes Jun 18, 2019

View reviewed changes

back to cmdline instead of env var

9985863

heronhaye reviewed Jun 18, 2019

View reviewed changes

cjb added 2 commits June 18, 2019 14:58

remove more KEYBASE_STARTUP_URL traces

f10c493

Surya's suggestions

3ccda6e

More Surya feedback

023499f

heronhaye approved these changes Jun 19, 2019

View reviewed changes

cjb merged commit 29941eb into master Jun 19, 2019

Conversation

cjb commented May 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjb commented May 23, 2019

Uh oh!

cjb commented May 24, 2019

Uh oh!

cjb commented Jun 14, 2019

Uh oh!

heronhaye commented Jun 17, 2019

Uh oh!

cjb commented Jun 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heronhaye Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heronhaye Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heronhaye Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heronhaye commented Jun 17, 2019

Uh oh!

cjb commented Jun 17, 2019

Uh oh!

heronhaye commented Jun 17, 2019

Uh oh!

heronhaye commented Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

heronhaye commented Jun 17, 2019

Uh oh!

cjb commented Jun 17, 2019

Uh oh!

cjb commented Jun 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heronhaye commented Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjb commented Jun 18, 2019

Uh oh!

buoyad left a comment

Choose a reason for hiding this comment

Uh oh!

heronhaye commented Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjb commented Jun 19, 2019

Uh oh!

heronhaye commented Jun 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjb commented Jun 19, 2019

Uh oh!

heronhaye left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

cjb commented May 16, 2019 •

edited

Loading

heronhaye Jun 17, 2019 •

edited

Loading

heronhaye Jun 17, 2019 •

edited

Loading

heronhaye Jun 17, 2019 •

edited

Loading

heronhaye commented Jun 17, 2019 •

edited

Loading

heronhaye commented Jun 18, 2019 •

edited

Loading

heronhaye commented Jun 18, 2019 •

edited

Loading

heronhaye commented Jun 19, 2019 •

edited

Loading