Page MenuHomePhabricator

[keyserver/lib/web/native] Support external cameras scanning the QR code
ClosedPublic

Authored by rohan on Aug 24 2023, 6:53 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 13, 8:26 AM
Unknown Object (File)
Fri, Apr 12, 7:49 PM
Unknown Object (File)
Fri, Apr 12, 12:41 PM
Unknown Object (File)
Fri, Apr 12, 12:41 PM
Unknown Object (File)
Fri, Apr 12, 12:41 PM
Unknown Object (File)
Fri, Apr 12, 12:41 PM
Unknown Object (File)
Thu, Apr 11, 8:19 AM
Unknown Object (File)
Tue, Apr 9, 11:44 PM
Subscribers

Details

Summary

These are the changes to allow Comm to be opened from the external camera scanning one of the QR codes on either web or native. This flow only supports Comm being installed on the phone, and if it is, the QR code will open up the app and navigate to the barcode scanner where we can read the keys encoded in the QR code.

In this diff, I've not actually set any actual keys, but this is all hidden behind development so this won't get released to production. The idea is that down the line, we'll want to store a pair of two keys, so stringifying and encoding a JSON object into the URL that can later be parsed by the barcode scanner made the most sense.

Going to add @tomek as blocking since I tried to use the invite link work as inspiration for the best way to achieve this (especially in regards to link handling).

Addresses ENG-4648

Depends on D8912

Test Plan
  1. Confirmed that the barcode scanner read the data encoded in the QR code correctly still
  2. Had Comm logged in on my phone, then scanned the QR code with the iOS camera and saw Comm open and navigate to the barcode scanner
  3. Had Comm logged out of my phone, then scanned the QR code with the iOS camera and saw Comm open. After logging in, it took me to the barcode scanner.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

rohan requested review of this revision.Aug 24 2023, 7:10 AM
tomek requested changes to this revision.Aug 25 2023, 2:10 AM

An additional thing worth testing is a case where Comm is closed when the code is scanned.

I think that a long-term solution might be to have one deep link handler/listener, that would decide which handler to call (either invite links or QR code). It might be a little challenging to do this because of InviteLinksContextProvider.

native/navigation/qr-code-link-handler.react.js
13–40 ↗(On Diff #30279)

It doesn't seem that we're checking if this is a QR code link. What will happen when an invite link is opened? Won't that result in SecondaryDeviceQRCodeScannerRouteName being opened?

41 ↗(On Diff #30279)

It is a good idea to explicitly return from a component

native/qr-code/qr-code-screen.react.js
21–24 ↗(On Diff #30279)

This value will change in the near future, so it probably doesn't matter, but overall it is a good idea to compute things that don't depend on props or state outside the component.

This revision now requires changes to proceed.Aug 25 2023, 2:10 AM
In D8934#263624, @tomek wrote:

An additional thing worth testing is a case where Comm is closed when the code is scanned.

I thought it did, but it seems like when Comm is closed and a QR code is scanned, Comm opens but it's stuck on the splash screen. I'll update this diff to address feedback and try to investigate why

I think that a long-term solution might be to have one deep link handler/listener, that would decide which handler to call (either invite links or QR code). It might be a little challenging to do this because of InviteLinksContextProvider.

Yeah I agree, I was thinking something similar would be good, especially to make it easier down the line if we want more handlers/listeners. I'll create a follow up task so we can keep track of this: https://linear.app/comm/issue/ENG-4810/combine-deep-link-handlers-listeners-into-one-component

  1. Use inviteLinkUrlPrefix and arCodeLinkUrlPrefix to handle respective deep links opening (so clicking an invite link doesn’t open the barcode scanner)
  2. Return null explicit from the component
  3. Compute qrCodeValue outside of the component

Need to figure out why Comm gets stuck on the splash screen if it was closed when a QR code was scanned

rohan requested review of this revision.Aug 25 2023, 7:24 AM

After reading through this: https://reactnative.dev/docs/linking#getinitialurl, I made a prod release of Comm and scanned the QR code while the app was closed. Comm opened and successfully navigated to the barcode scanner

tomek requested changes to this revision.Aug 28 2023, 1:31 AM

Sorry for requesting changes once again, but the logic in invite-links-context-provider seems to be a little brittle and I'd like to verify the changes before landing.

native/invite-links/invite-links-context-provider.react.js
83 ↗(On Diff #30318)

This is unfortunately more complicated. D8693 changes iOS link URL, because the https one doesn't work on Chrome. So we have to also check if the link starts with comm://invite/.

88–98 ↗(On Diff #30318)

We're actually already checking the condition by parsing a secret and checking if it's there. parseSecret would extract the secret from both https and comm schemes so we might be ok to leave it as it was, or refactor the function to make it more obvious what it does.

This revision now requires changes to proceed.Aug 28 2023, 1:31 AM

@tomek I think the best thing to do is to follow your suggestion of keeping invite links provider as-is, and actually follow a similar logic to parseSecret rather than checking for the URL existence in the QR code handler since it will be a little more resistant to changes

native/invite-links/invite-links-context-provider.react.js
83 ↗(On Diff #30318)

Oh right thanks for pointing that out, I didn't consider the different URL there

88–98 ↗(On Diff #30318)

That's a good point, I somehow skipped over urlRegex only matching links that contain the invite link pattern, and not all generic links.

  1. Leave invite-links-context-provider.react.js as is
  2. Revert the changes to links.js to export the urls
  3. Follow a similar logic to parseSecret with a parseKeys function in qr-code-link-handler.react.js since it's more resistant to changing the url schema
  4. Tested that clicking invite links doesn't open the scanenr and opening the qr-code schema in the app doesn't prompt the invite links modal

Looks great!

native/profile/secondary-device-qr-code-scanner.react.js
40–42 ↗(On Diff #30444)

We can consider merging this logic with parseKeys - is it possible to have the regex in just one place?

This revision is now accepted and ready to land.Aug 28 2023, 8:22 AM

Move key parsing logic to lib so it can be reused across components

Handle null match to prevent JSON.parse throwing an error

This revision was landed with ongoing or failed builds.Aug 28 2023, 8:46 AM
This revision was automatically updated to reflect the committed changes.
lib/facts/links.js
7

Nit: we appear to be a little inconsistent about Url vs URL in the naming patterns here

lib/facts/links.js
7