if a user attempts to add a second keyserver via qr code auth, we should prompt them to either replace their existing keyserver or cancel the auth attempt.
Depends on D12809
Details
Details
created new authoritative keyserver with username jackson, user ID D753543A-F5C9-4058-B6D8-DF9398C5AB51, device ID 25k0GJ+aTm1NQGZOo+5MkhRUBqhWfi66s2I3tIB38cQ
registered new user jacks with user ID 528DBB32-D52C-4B1F-8604-06F36B8AAFBB, device ID /fAt0mtfRH8mlRL9gjFVZmSfwTG7kSQ0hZ9kwuKxaiQ
spun up docker keyserver on port 3001
added keyserver as secondary device
removed mariadb volume and spun up another docker keyserver
REPLACED first keyserver with second in device list successfully
successfully added a web secondary device to ensure that i didn't break the existing workflow
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| native/profile/secondary-device-qr-code-scanner.react.js | ||
|---|---|---|
| 246 ↗ | (On Diff #42791) | i'll just make this missing keyserver device ID. the current error is too complicated |
| 256 ↗ | (On Diff #42791) | |
| 331 ↗ | (On Diff #42791) | I missed this in a previous diff. We should return here so that we don't call processDeviceListUpdate(). it doesn't make a huge difference since we check for aes256.current and secondaryDeviceID.current in processDeviceListUpdate() anyway, but this is more correct |
| 381 ↗ | (On Diff #42791) | same as above comment |
Comment Actions
Looks reasonable, but the copy should be reviewed by @ashoat
| native/profile/secondary-device-qr-code-scanner.react.js | ||
|---|---|---|
| 276–283 ↗ | (On Diff #42791) | I'm not sure, but for me it sounds that cancel and OK aren't good responses to a question Do you want...?. Maybe yes and no? Or replace and no? |
| 372–378 ↗ | (On Diff #42791) | Is it possible that secondaryDeviceType and secondaryDeviceID get out of sync? Maybe we should set secondaryDeviceType in the last statement in try clause? |
Comment Actions
Generally looks good, but requesting changes because of the volume of comments between me and @tomek, and because I want to see what the Alert looks like with the custom styles
| native/profile/secondary-device-qr-code-scanner.react.js | ||
|---|---|---|
| 266–270 ↗ | (On Diff #42791) | Can we reverse this condition and add an early exit? |
| 276–283 ↗ | (On Diff #42791) | Yeah, let's go with "Replace" with style: 'destructive' and "No" with style: 'cancel'. Can you share a screenshot? |
| 277–279 ↗ | (On Diff #42791) | Why do we define this new function? Can we just pass goBack, or is there some type issue? |
| 303 ↗ | (On Diff #42791) | We don't need to pass ownPeerDevices into this dep list. We're only using a part of it (keyserverDeviceID), so instead we should calculate that outside of the React.useCallback, and pass just that in. This will allow us to avoid regenerating the callback when irrelevant parts of ownPeerDevices change |
