Page MenuHomePhabricator

[lib] avoid listening to Tunnelbroker messages in `QRAuthProvider` when QR auth is finished
ClosedPublic

Authored by kamil on Mon, Jun 17, 4:07 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Jun 25, 8:40 PM
Unknown Object (File)
Tue, Jun 25, 1:49 PM
Unknown Object (File)
Tue, Jun 25, 9:21 AM
Unknown Object (File)
Mon, Jun 24, 11:18 PM
Unknown Object (File)
Mon, Jun 24, 9:22 PM
Unknown Object (File)
Mon, Jun 24, 12:38 PM
Unknown Object (File)
Mon, Jun 24, 10:45 AM
Unknown Object (File)
Sun, Jun 23, 5:59 PM
Subscribers

Details

Summary

ENG-8385.

QR code login is not working right now. As described here (Point 3) and here the issue is occuring because of the fact that useQRAuth is unmounting before it can send SECONDARY_DEVICE_REGISTRATION_SUCCESS. This stack is refactoring the code to move logic to context (see here).

Make sure we no longer track Tunnelbroker messages here and avoid sending SECONDARY_DEVICE_REGISTRATION_SUCCESS multiple times (BACKUP_DATA_KEY_MESSAGE is the last message between devices).

Note that without scanning code listener is not added, so in normal app usage, listener code shouldn't be executed.

Depends on D12453

Test Plan

Test QR login flow on native and web

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Mon, Jun 17, 4:37 AM
ashoat requested changes to this revision.Mon, Jun 17, 10:22 AM

What happens if the user logs in via QR code, then logs out, and then tries again? It seems like qrAuthFinished is never set to false after being set to true, which makes me suspect that things will be broken in this case.

This revision now requires changes to proceed.Mon, Jun 17, 10:22 AM

set qrAuthFinished to false when calling generateQRCode

What happens if the user logs in via QR code, then logs out, and then tries again? It seems like qrAuthFinished is never set to false after being set to true, which makes me suspect that things will be broken in this case.

Good catch, updated code to unset this each time when we generate new QR code and tested case you described.

Please remove the unnecessary effect before landing, or re-request review if you think I'm missing something

lib/components/qr-auth-provider.react.js
225–229 ↗(On Diff #41454)

This effect should not be necessary. If qrAuthFinished flips to true, the above effect will be re-run. Its cleanup function will run first, which will call removeListener. Then it will be processed again, but since qrAuthFinished it won't do anything

This revision is now accepted and ready to land.Tue, Jun 18, 10:04 AM
lib/components/qr-auth-provider.react.js
67 ↗(On Diff #41454)

Might be worth considering if qrAuthFinished should be merged into qrData, but probably not necessary