encrypt for invited users if history visibility allows. #666
Conversation
src/crypto/index.js
Outdated
| } else if (member.membership == 'invite' && | ||
| this._clientStore.getRoom(roomId).shouldEncryptForInvitedUsers()) { | ||
| console.log('Invite event for ' + member.userId + ' in ' + roomId); | ||
| this._deviceList.startTrackingDeviceList(member.userId); |
There was a problem hiding this comment.
Hard to check from my phone, but I worry about this causing (more) device tracking bugs, since we won't be reliably receiving device list updates from this user, but maybe we will think we are up to date, and hence not resync once the user does join the room?
There was a problem hiding this comment.
Agreed that encrypting messages to invited user could work unreliably if the server doesn't have the corresponding change, but it should be limited to invited users, right? Messages to joined user should work no worse than before?
There was a problem hiding this comment.
Actually no, I see what you mean. If the server doesn't have matrix-org/synapse#3484 then it breaks when:
- Alice is in a room
- Bob is invited to the room. Alice starts tracking Bob's device list
- Bob adds a device. The server doesn't send an update poke.
- Bob joins. Alice's client will call
startTrackingDeviceList(bob)but nothing will happen because Bob is already tracked. - Alice sends a message, but she doesn't know about Bob's new device list.
In other words, I think this is perfectly safe in the situation where we can be sure the server will have matrix-org/synapse#3484 . In the real world, one workaround could be to always force a re-fetch of the device list for a user when their membership transitions to join in any room?
There was a problem hiding this comment.
this of course assumes the synapse patch works, and is going to send enough devicelist updates for invited users correctly. i will test by seeing what happens if you add a device to an invited user before they join the room. (and then for the dumb synapse scenario, explicitly refetch the list when they join, as dave says)
There was a problem hiding this comment.
actually, it's going to be really hard for me to investigate this today, and we ideally need it for tomorrow. @dbkr: would you be able to check the concern above (and mitigate it by resyncing on /join if needed)?
There was a problem hiding this comment.
Following up, there was much discussion and the conclusion is that there's a big edge case here in that federated servers do not know what other servers are in the room at the time they've been invited. They therefore will not send device updates if a user logs in whilst their state is invite.
There was a problem hiding this comment.
Also, I think you forgot to commit where you added shouldEncryptForInvitedUsers?
There was a problem hiding this comment.
Fixed. Having looks into forcing an update when a member joins the room, actually it's not necessary since synapse sends a device list update when the user joins the room anyway. Arguably this is synapse impl specific behaviour though.
|
Conclusion here is that this works, other than for the edge case of matrix-org/synapse#3504. |
|
...and doesn't cause any regressions; hence merging. |
fixes element-hq/element-web#2713