-
-
Notifications
You must be signed in to change notification settings - Fork 578
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
Support for warning users when unknown devices show up #335
Merged
Merged
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
fb820fa
experimental support for warning users when unknown devices show up i…
ara4n 34a0bd4
oops, unbreak it
ara4n e79926d
fix lint
ara4n 247deac
some incoherent jottings on the warning semantics
ara4n 4ccd649
Address my own review comments
richvdh 5245c7f
Merge remote-tracking branch 'origin/develop' into matthew/warn-unkno…
richvdh 085493d
Fix tests
richvdh dfae72e
Merge branch 'matthew/warn-unknown-devices' of git+ssh://github.com/m…
ara4n 5911c4d
don't automatically mark devices as known; require the app to do it
ara4n 34fde7d
Store device 'known' status in session store
richvdh 5d544c7
Merge branch 'develop' into matthew/warn-unknown-devices
ara4n File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
Random notes from Matthew on the two possible approaches for warning users about unexpected | ||
unverified devices popping up in their rooms.... | ||
|
||
Original idea... | ||
================ | ||
|
||
Warn when an existing user adds an unknown device to a room. | ||
|
||
Warn when a user joins the room with unverified or unknown devices. | ||
|
||
Warn when you initial sync if the room has any unverified devices in it. | ||
^ this is good enough if we're doing local storage. | ||
OR, better: | ||
Warn when you initial sync if the room has any new undefined devices since you were last there. | ||
=> This means persisting the rooms that devices are in, across initial syncs. | ||
|
||
|
||
Updated idea... | ||
=============== | ||
|
||
Warn when the user tries to send a message: | ||
- If the room has unverified devices which the user has not yet been told about in the context of this room | ||
...or in the context of this user? currently all verification is per-user, not per-room. | ||
...this should be good enough. | ||
|
||
- so track whether we have warned the user or not about unverified devices - blocked, unverified, verified, unverified_warned. | ||
throw an error when trying to encrypt if there are pure unverified devices there | ||
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? | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -388,6 +388,10 @@ MegolmEncryption.prototype._shareKeyWithDevices = function(session, devicesByUse | |
MegolmEncryption.prototype.encryptMessage = function(room, eventType, content) { | ||
const self = this; | ||
return this._getDevicesInRoom(room).then(function(devicesInRoom) { | ||
// check if any of these devices are not yet known to the user. | ||
// if so, warn the user so they can verify or ignore. | ||
self._checkForUnknownDevices(devicesInRoom); | ||
|
||
return self._ensureOutboundSession(devicesInRoom); | ||
}).then(function(session) { | ||
const payloadJson = { | ||
|
@@ -415,6 +419,41 @@ MegolmEncryption.prototype.encryptMessage = function(room, eventType, content) { | |
}); | ||
}; | ||
|
||
/** | ||
* Checks the devices we're about to send to and see if any are entirely | ||
* unknown to the user. If so, warn the user, and mark them as known to | ||
* give the user a chance to go verify them before re-sending this message. | ||
* | ||
* @param {Object} devicesInRoom userId -> {deviceId -> object} | ||
* devices we should shared the session with. | ||
*/ | ||
MegolmEncryption.prototype._checkForUnknownDevices = function(devicesInRoom) { | ||
const unknownDevices = {}; | ||
|
||
Object.keys(devicesInRoom).forEach((userId)=>{ | ||
Object.keys(devicesInRoom[userId]).forEach((deviceId)=>{ | ||
const device = devicesInRoom[userId][deviceId]; | ||
if (device.isUnverified() && !device.isKnown()) { | ||
// mark the devices as known to the user, given we're about to | ||
// yell at them. | ||
this._crypto.setDeviceVerification(userId, device.deviceId, | ||
undefined, undefined, true); | ||
if (!unknownDevices[userId]) { | ||
unknownDevices[userId] = {}; | ||
} | ||
unknownDevices[userId][deviceId] = device; | ||
} | ||
}); | ||
}); | ||
|
||
if (Object.keys(unknownDevices).length) { | ||
// it'd be kind to pass unknownDevices up to the user in this error | ||
throw new base.UnknownDeviceError( | ||
"This room contains unknown devices which have not been verified. " + | ||
"We strongly recommend you verify them before continuing.", unknownDevices); | ||
} | ||
}; | ||
|
||
/** | ||
* Get the list of unblocked devices for all users in the room | ||
* | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yup, this is exactly what element-hq/element-web#2305 (comment) is on about |
||
return this._crypto.downloadKeys(roomMembers, false).then(function(devices) { | ||
// remove any blocked devices | ||
for (const userId in devices) { | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -684,8 +684,12 @@ Crypto.prototype.getDeviceByIdentityKey = function(userId, algorithm, sender_key | |
* | ||
* @param {?boolean} blocked whether to mark the device as blocked. Null to | ||
* leave unchanged. | ||
* | ||
* @param {?boolean} known whether to mark that the user has been made aware of | ||
* the existence of this device. Null to leave unchanged | ||
*/ | ||
Crypto.prototype.setDeviceVerification = function(userId, deviceId, verified, blocked) { | ||
Crypto.prototype.setDeviceVerification = function(userId, deviceId, verified, | ||
blocked, known) { | ||
const devices = this._sessionStore.getEndToEndDevicesForUser(userId); | ||
if (!devices || !devices[deviceId]) { | ||
throw new Error("Unknown device " + userId + ":" + deviceId); | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should probably also check for |
||
knownStatus = known; | ||
} | ||
|
||
if (dev.verified === verificationStatus && dev.known === knownStatus) { | ||
return; | ||
} | ||
dev.verified = verificationStatus; | ||
dev.known = knownStatus; | ||
this._sessionStore.storeEndToEndDevicesForUser(userId, devices); | ||
}; | ||
|
||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.