Skip to content
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.

Sign up for GitHub

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

encrypt for invited users if history visibility allows. #666

Merged
merged 3 commits into from
Jul 10, 2018
Merged

encrypt for invited users if history visibility allows. #666

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d8d35f4
encrypt for invited users if history visibility allows.
ara4n Jul 4, 2018
5ae2c26
lint
ara4n Jul 5, 2018
7283076
Fix Users / Members mixup
dbkr Jul 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/crypto/algorithms/megolm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,8 +536,7 @@ MegolmEncryption.prototype._checkForUnknownDevices = function(devicesInRoom) {
* from userId to deviceId to deviceInfo
*/
MegolmEncryption.prototype._getDevicesInRoom = function(room) {
// XXX what about rooms where invitees can see the content?
const roomMembers = utils.map(room.getJoinedMembers(), function(u) {
const roomMembers = utils.map(room.getEncryptionTargetMembers(), function(u) {
return u.userId;
});

Expand Down
2 changes: 1 addition & 1 deletion src/crypto/algorithms/olm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ OlmEncryption.prototype.encryptMessage = function(room, eventType, content) {
// TODO: there is a race condition here! What if a new user turns up
// just as you are sending a secret message?

const users = utils.map(room.getJoinedMembers(), function(u) {
const users = utils.map(room.getEncryptionTargetMembers(), function(u) {
return u.userId;
});

Expand Down
8 changes: 6 additions & 2 deletions src/crypto/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -652,7 +652,7 @@ Crypto.prototype.setRoomEncryption = async function(roomId, config, inhibitDevic
throw new Error(`Unable to enable encryption in unknown room ${roomId}`);
}

const members = room.getJoinedMembers();
const members = room.getEncryptionTargetMembers();
members.forEach((m) => {
this._deviceList.startTrackingDeviceList(m.userId);
});
Expand Down Expand Up @@ -986,7 +986,7 @@ Crypto.prototype._evalDeviceListChanges = async function(deviceLists) {
Crypto.prototype._getE2eUsers = function() {
const e2eUserIds = [];
for (const room of this._getE2eRooms()) {
const members = room.getJoinedMembers();
const members = room.getEncryptionTargetMembers();
for (const member of members) {
e2eUserIds.push(member.userId);
}
Expand Down Expand Up @@ -1085,6 +1085,10 @@ Crypto.prototype._onRoomMembership = function(event, member, oldMembership) {
console.log('Join event for ' + member.userId + ' in ' + roomId);
// make sure we are tracking the deviceList for this user
this._deviceList.startTrackingDeviceList(member.userId);
} else if (member.membership == 'invite' &&
this._clientStore.getRoom(roomId).shouldEncryptForInvitedUsers()) {
console.log('Invite event for ' + member.userId + ' in ' + roomId);
this._deviceList.startTrackingDeviceList(member.userId);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually no, I see what you mean. If the server doesn't have matrix-org/synapse#3484 then it breaks when:

  1. Alice is in a room
  2. Bob is invited to the room. Alice starts tracking Bob's device list
  3. Bob adds a device. The server doesn't send an update poke.
  4. Bob joins. Alice's client will call startTrackingDeviceList(bob) but nothing will happen because Bob is already tracked.
  5. 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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I think you forgot to commit where you added shouldEncryptForInvitedUsers?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

alg.onRoomMembership(event, member, oldMembership);
Expand Down
23 changes: 22 additions & 1 deletion src/models/room.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,6 +456,28 @@ Room.prototype.addEventsToTimeline = function(events, toStartOfTimeline,
});
};

/**
* Get a list of members we should be encrypting for in this room
* @return {RoomMember[]} A list of members who we should encrypt messages for
* in this room.
*/
Room.prototype.getEncryptionTargetMembers = function() {
let members = this.getMembersWithMembership("join");
if (this.shouldEncryptForInvitedMembers()) {
members = members.concat(this.getMembersWithMembership("invite"));
}
return members;
};

/**
* Determine whether we should encrypt messages for invited users in this room
* @return {boolean} if we should encrypt messages for invited users
*/
Room.prototype.shouldEncryptForInvitedMembers = function() {
const ev = this.currentState.getStateEvents("m.room.history_visibility", "");
return (ev && ev.getContent() && ev.getContent().history_visibility !== "joined");
};

/**
* Get the default room name (i.e. what a given user would see if the
* room had no m.room.name)
Expand Down Expand Up @@ -950,7 +972,6 @@ Room.prototype.recalculate = function(userId) {
}
};


/**
* Get a list of user IDs who have <b>read up to</b> the given event.
* @param {MatrixEvent} event the event to get read receipts for.
Expand Down