Support for warning users when unknown devices show up #335
Conversation
…n a room. hopefully a step towards fixing element-hq/element-web#2143
|
(yes, i know the tests are broken; i've left them as I'm out of time and I'm genuinely not sure what the right way is to handle the fact that i'm deliberately breaking them by legitimately firing the UnknownDeviceException) |
src/crypto/algorithms/megolm.js
Outdated
| @@ -433,6 +472,9 @@ MegolmEncryption.prototype._getDevicesInRoom = function(room) { | |||
| // have a list of the user's devices, then we already share an e2e room | |||
| // with them, which means that they will have announced any new devices via | |||
| // an m.new_device. | |||
| // | |||
| // XXX: what if the cache is stale, and the user left the room we had in common | |||
| // and then added new devices before joining this one? --Matthew | |||
There was a problem hiding this comment.
yup, this is exactly what element-hq/element-web#2305 (comment) is on about
|
Looks pretty sane to me. I can try and pick this up and make the tests work once I (finally) finish session import/export.
Yeah, it certainly is. I feel like it's very odd for |
src/crypto/index.js
Outdated
| @@ -706,10 +710,16 @@ Crypto.prototype.setDeviceVerification = function(userId, deviceId, verified, bl | |||
| verificationStatus = DeviceVerification.UNVERIFIED; | |||
| } | |||
|
|
|||
| if (dev.verified === verificationStatus) { | |||
| let knownStatus = dev.known; | |||
| if (known !== null) { | |||
There was a problem hiding this comment.
should probably also check for undefined here
|
My logic was that: just because known-ness and verification-ness (and blockedness) happen to be orthogonal currently doesn't mean that they should all be put in the same enum (especially one called |
|
also, thanks for picking this up; sorry for leaving it 95% done. |
…atrix-org/matrix-js-sdk into matthew/warn-unknown-devices
| app will have to search for the devices which are pure unverified to warn about them - have to do this from MembersList anyway? | ||
| - or megolm could warn which devices are causing the problems. | ||
|
|
||
| Why do we wait to establish outbound sessions? It just makes a horrible pause when we first try to send a message... but could otherwise unnecessarily consume resources? |
There was a problem hiding this comment.
We've discussed this before. Because each megolm session consumes 1K of localstorage on every other device in the room. I don't want to add a megolm session to my storage every time someone in #megolm logs in on a new device, and it feels O(N^2)y to start to do so.
We should be more proactive in fetching the device list (this ties into Erik's work there) - and a lot of the work we do (and consequent pause) is in verifying the content of that device list. See element-hq/element-web#2157 (comment) for further thoughts on reducing the pause.
|
I think we can land this and matrix-org/matrix-react-sdk#635. |
|
... or rather, we can land this once we can land matrix-org/matrix-react-sdk#635, which is waiting for a fix to element-hq/element-web#3018 |
This adds a new crypto.DeviceInfo field called
knownwhich tracks whether the user has ever been made aware of a device's existence.If the user tries to send a message into a room with unknown devices present, it throws a custom exception to the app with the details of the devices so the app can warn the user. It also marks the devices as
knowngiven the user now knows they exist.knownis implemented as a flag on DeviceInfo rather than part of the verification enum as it's slightly semantically distinct from the concept of whether a device is verified or blocked (although it's questionable as to how a user would be able to verify or block a device without knowing it existed!).The first draft of this tried to track
m.new_deviceevents as they came in, and warn the user if they referred to devices which the user hadn't seen before - however, this felt prone to races between correlating new-devices and room membership, downloading the actual device info, and the user speaking. Hopefully this simpler approach doesn't fall foul of the racing concerns mentioned on the original bug.Hopefully goes some way towards fixing element-hq/element-web#2143
Twinned with matrix-org/matrix-react-sdk#635.