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

[Review] Prototype for new protocol detection method #591

Open
wants to merge 4 commits into
base: develop
from

Conversation

Projects
None yet
8 participants
@zone117x
Copy link
Member

zone117x commented Jan 6, 2019

I think we should prioritize the auth problems due to the protocol handler. I'm requesting several reviewers so we can validate or invalidate this new technique.

This technique uses a redirect "ping" of sorts to detect the protocol handler in a way not dependent on browser quirks and user-agent detection. Compared to the existing method, this implementation errs on the side of caution..

  • False-positive detections are treated as unacceptable. Web app completely broken when user cannot login because we incorrectly assumed a protocol handler.
  • False-negatives should be rare, but are acceptable. The web app may be redirect to https auth when the protocol handler worked. These do not break the auth flow but rather slightly compromise the UX.

How it works:

  1. Web app tab A performs a protocol redirect for the propose of a ping (rather than auth). Example: blockstack://echo?redirectURL=https://testapp.com/echoReply?requestID=12345
  2. Web app tab A starts a 1000ms timer waiting for the echo reply. Side note: a couple browser APIs related to DOM window focus & visibility are leveraged to detect probable protocol handler capability, and increase the timeout to 2-4 seconds.
  3. If the local auth app is installed, it is launched and immediately opens the ping-echo url for the web app that initiated it. As in, it opens web app tab B using the query param echoReply=[requestID].
  4. When tab B is loaded it immediately leverages cross-window communication to tell tab A that the protocol handler was launched and is working. In most cases, this all occurs within the 1-4 second timeout, and the originating tab knows not to redirect itself to the https auth.
  5. Tab B then redirects back to the localhost/native auth flow.
  6. Tab A either:
    1. Acknowledges tab B's echo reply which indicates the protocol handler is working, and does not redirect to https web auth.
    2. OR it does not get the echo reply within the timeout period (1000ms) and it redirects the page to the https web auth.

Testing out the prototype:

This approach requires a minor change to the auth browser (Blockstack Browser), see blockstack/blockstack-browser@3cfb204

Improvements and fixes:

  • Should work on all browsers and unlikely to be broken by any given browser.
  • Prevents the browser prompt from Safari and others about unknown protocol when auth-browser is not installed by using an iframe technique rather than window.location to launch the protocol uri.
  • Fixes the bug where the Blockstack Browser app is uninstalled but the browser / OS still has a protocol handler registered. See #571 (comment)
  • Lessens the severity of the bug where using your non-default web browser causes the default browser to be opened and signed in. With this approach the default browser will still be opened, but the original browser will notice the failure and redirect to https auth rather than do nothing and prevent auth.
  • In theory this should work on mobile as well. No more browser user-agent detection code.

Downsides:

  • Requires a minor wait/timeout before redirecting to the http/web auth.
  • The window redirection dance is a bit odd.

Good & bad:

If the native app is installed and opens, but something prevents the ping reply to the web app from occurring within the timeout (like a slow computer), then the original tab will end up with a false-negative (the protocol handler is not detected), and will redirect to the https auth.

I think this could also be considered good behavior that indicates the resilience of this approach, because in the situation of some unknown failure condition, the user auth flow will still be able to progress through the https auth, as opposed to the web app doing nothing upon login-click.

And in the case of this false-positive (where the redirect to https auth even though the protocol handler did actually launch), that original tab was useless anyway with the existing approach which leaves that web app open and unauthenticated.

Backwards compatibility / upgrade path

This approach does not require a blockstack.js major semver bump or any code usage changes from app devs. It should also not break auth flow when blockstack.js is updated and the auth-browser is outdated or vice versa. It does, however, require both an updated blockstack.js and auth-browser for users experience the improvements offered.

Reference issues:

@zone117x zone117x requested review from jcnelson , hstove , kantai and yknl Jan 6, 2019

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Jan 6, 2019

CLA assistant check
All committers have signed the CLA.

@yknl

This comment has been minimized.

Copy link
Collaborator

yknl commented Jan 8, 2019

This is a great solution to the protocol handler problems we've been experiencing, but I think there are a few UX issues that we should resolve before moving forward.

When I tested this out on Safari, the open protocol handler prompt appeared, but if I don't click on "Allow" immediately, the app begins to load browser.blockstack.org in the background. I think we need a longer timeout.

Regarding implementing Larry's idea #2, it doesn't improve UX for 90% of the use cases. Because once a user is logged into an app, they're unlikely to have to log in again for a while. We would only need to trigger the auth flow for new apps that the user hasn't logged into before and which we don't have a preference for.

I think one potential enhancement would be to show a pop up indicating that the user is being redirected, and if it didn't work, they can click a button to go to the web app browser. However this means that we have to bundle UI into blockstack.js.

@hstove

This comment has been minimized.

Copy link
Contributor

hstove commented Jan 8, 2019

I think one potential enhancement would be to show a pop up indicating that the user is being redirected, and if it didn't work, they can click a button to go to the web app browser. However this means that we have to bundle UI into blockstack.js.

Matt discussed this in the forum, but we could inject an iframe with the UI. It'd be very little javascript required. Then we could also update it without apps having to update blockstack.js.

@zone117x

This comment has been minimized.

Copy link
Member

zone117x commented Jan 8, 2019

EDIT: The UX issue discussed below has been solved in this PR.


@yknl thanks for looking this over.

I think there are a few UX issues that we should resolve before moving forward ... When I tested this out on Safari, the open protocol handler prompt appeared, but if I don't click on "Allow" immediately, the app begins to load browser.blockstack.org in the background.

I just discovered this issue but worse. Safari creates some background page noise (minor inconvenience but not a deal breaker), however, all chromium-based browsers will auto-close the prompt if the page is redirected, which is definitely a deal-breaker. I outlined two solutions to this in the forum post I just made https://forum.blockstack.org/t/proposed-solutions-for-desktop-safari-firefox-sign-in/6713/9

I think one potential enhancement would be to show a pop up indicating that the user is being redirected, and if it didn't work, they can click a button to go to the web app browser. However this means that we have to bundle UI into blockstack.js.

This is my favorite approach at the moment, which I described in the forum post - this new detection method combined the with the choice modal.

I think the iframe for the https login modal (that @hstove just mentioned) is even better UX, but it comes at the cost of some minor security issues like user url src validation and click-jacking. However, at the moment I don't think we are doing anything on our https auth endpoint to prevent these types of attacks anyway, like x-frame-options, Content-Security-Policy frame-ancestors or iframe-busting.

@yknl @hstove Do you think we can move forward with either of these fixes (choice modal or iframe)? If so I'll update the prototype for additional review. If we are not concerned about the potential iframe security issues then that would be quick and easy to implement.

zone117x added some commits Jan 10, 2019

* Use input blur & focus technique to detect browsers prompts that as…
…k the user about opening a the installed auth-browser.

* Use page visibility API to increase redirection timeout and reduce probability of false-negative https auth redirects.
@zone117x

This comment has been minimized.

Copy link
Member

zone117x commented Jan 10, 2019

Update: figured out a way to leverage a couple js dom APIs that behave consistently and reliably across browsers to fix the browser prompt auto-dismissal bug with this new protocol detection method. So far extensive testing on macOS with Chrome, Firefox, Safari, Opera, and Brave show it working really well. Opera shows an error prompt but works fine after dismissing. Working on testing Edge.

These methods are a continuation of the graceful-fallback / probability based approach - where all the techniques / APIs are currently working well in all tested browsers, but if something breaks from some future browser update or edge case, then the https auth redirect will always still be triggered.

@hstove

This comment has been minimized.

Copy link
Contributor

hstove commented Jan 10, 2019

One behavior on Brave right now is that when I use the demo site, it redirects to localhost:8888, but in the original tab, it redirects to browser.blockstack.org. Is that expected behavior? I have no prompt for the protocol handle because it's set to always redirect.

@zone117x

This comment has been minimized.

Copy link
Member

zone117x commented Jan 10, 2019

One behavior on Brave right now is that when I use the demo site, it redirects to localhost:8888, but in the original tab, it redirects to browser.blockstack.org. Is that expected behavior? I have no prompt for the protocol handle because it's set to always redirect.

Sounds like the auth-browser branch isn't being ran on your machine?

@hstove

This comment has been minimized.

Copy link
Contributor

hstove commented Jan 10, 2019

Sounds like the auth-browser branch isn't being ran on your machine?

Oh yeah, whoops!

@zone117x

This comment has been minimized.

Copy link
Member

zone117x commented Jan 15, 2019

Completed testing with Edge browser. This new method works in Edge but has a minor UX imperfection - when the native app is not installed, the browser will ask the user if they want to "look for an app in Microsoft app store". The web app does detect that a protocol handler is not installed, and does redirect them to the https auth. The user simply has to dismiss the prompt.
When the native app is installed, it works without issue. This seems like an acceptable trade off.

function failCallback() {
Logger.warn('protocol handler not detected')
window.location = httpsURI
const detectionTimeout = 1000

This comment has been minimized.

@jcnelson

jcnelson Jan 16, 2019

Member

Is this long enough? What happens if I do this on an underpowered mobile phone requesting the desktop site?

This comment has been minimized.

@zone117x

zone117x Jan 16, 2019

Member

I've been calling these situations "false-negative" detections. On mobile a false redirect to https auth is largely inconsequential. The native mobile protocol handler app will have already launched.

Due to the nature of this new detection approach, these are unavoidable but we aim to make them unlikely and also not that bad when they do occur.

See the initial PR description for more info

False-positive detections are treated as unacceptable. Web app completely broken when user cannot login because we incorrectly assumed a protocol handler.
False-negatives should be rare, but are acceptable. The web app may be redirect to https auth when the protocol handler worked.

This comment has been minimized.

@kantai

kantai Jan 16, 2019

Member

I think this period of time is also weird if you don't click "Always open with this app" in your browser-- because it definitely takes my computer longer than 1 second to actually open the native app. Not sure about the trade off there.

This comment has been minimized.

@zone117x

zone117x Jan 16, 2019

Member

@kantai We detect when the "open blockstack app?" browser popup occurs, then pause the redirect timeout. When the popup is closed (user clicks either yes or no/cancel), the timer is restarted.

If the user clicks "yes open blockstack app", then the page visibility API detects that the protocol handler is probably working, and restarts the timer with 3000ms timeout. This is for the purpose of reducing probability of false-negative redirect, while still performing the redirect if something goes wrong with the protocol handler.

This comment has been minimized.

@zone117x

zone117x Jan 16, 2019

Member

@kantai For context on this see the comments on these two components:

This comment has been minimized.

@kantai

kantai Jan 16, 2019

Member

Cool, that works for me, @zone117x -- and yeah, it correctly detected when that "open blockstack app?" popup occurs -- it might have been an older version I tested with that had that issue.

// Create the storage entry to signal a protocol detection attempt for the
// next browser window to check.
window.localStorage.setItem(echoReplyKey, 'pending')
const cleanUpLocalStorage = () => {

This comment has been minimized.

@jcnelson

jcnelson Jan 16, 2019

Member

Shouldn't this clear out all echo reply keys? For example, what if the user closes their browser (or the browser crashes) while doing an authentication?

This comment has been minimized.

@zone117x

zone117x Jan 16, 2019

Member

To avoid cluttering the localStorage right? We can, but then run the risk of breaking an in-progress protocol-ping detection, for the same web app in another tab. Unlikely but possible.
Which error condition in either of these is worse?

We can also add a timestamp to the key's value (currently value is just set to "pending"), and clear out all keys older than 1 minute or so?

This comment has been minimized.

@jcnelson

jcnelson Jan 16, 2019

Member

To avoid re-using echo reply keys by mistake. Clearing them out if they're e.g. over an hour old should be fine.

This comment has been minimized.

@zone117x

zone117x Jan 16, 2019

Member

👍will get this implemented.

@kantai

This comment has been minimized.

Copy link
Member

kantai commented Jan 16, 2019

This worked for me pretty seamlessly in Chromium on Linux. I tested it by changing my protocol handler in Linux:

$ cat ~/.local/share/applications/blockstack-protocol-handler
#!/bin/bash
AUTH=$(echo "$1" | sed s#/##g | sed s/blockstack:// | sed 's/ //g')
xdg-open "https://deploy-preview-1774--reporter-beaver-73821.netlify.com/auth?authRequest=${AUTH}"

Which is the netlify preview of the browser built with echo response features. For other Linux testers, this could be the fastest way to test it out.

document.body.appendChild(iframe)
} else {
Logger.error('document.body is null when attempting iframe injection for protoocol URI launch')
}

This comment has been minimized.

@kantai

kantai Jan 16, 2019

Member

Can this be refactored to look somewhat like checkedBrowserLaunch(protocolURI, failCallback) ?

It'd make it a bit more clear later on if we want to do something other than just redirect to browser.blockstack.org.

This comment has been minimized.

@zone117x

zone117x Jan 16, 2019

Member

Good idea, I will do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment