Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix detection of encryption for all users in a room #10487

Merged
merged 1 commit into from
Mar 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 11 additions & 6 deletions src/createRoom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -398,16 +398,21 @@ export default async function createRoom(opts: IOpts): Promise<string | null> {
export async function canEncryptToAllUsers(client: MatrixClient, userIds: string[]): Promise<boolean> {
try {
const usersDeviceMap = await client.downloadKeys(userIds);
// { "@user:host": { "DEVICE": {...}, ... }, ... }
return Object.values(usersDeviceMap).every(
(userDevices) =>
// { "DEVICE": {...}, ... }
Object.keys(userDevices).length > 0,
);

// There are no devices at all.
Copy link
Member

Choose a reason for hiding this comment

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

s/devices/users/.

I think this is a change of behaviour from before 3.69.0. Are we sure it's what's expected?

It's also inconsistent with the documentation on the function. If there are no users in the room (or rather, in the userIds list) then "there is at least one device that we can encrypt to" is trivially true.

Copy link
Member

Choose a reason for hiding this comment

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

It's also inconsistent with the documentation on the function. If there are no users in the room (or rather, in the userIds list) then "there is at least one device that we can encrypt to" is trivially true.

sorry, I misquoted.

If there are no users, then "for every user in a room, <X>" is trivially true, for any <X>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is a change. Not sure if it is correct. If I download the keys for a list of users and I do get an empty response, can I not encrypt to them? 🤔

An empty userIds arg is a slightly separate case. If I do not specify a user, I cannot encrypt to them.

Copy link
Member

Choose a reason for hiding this comment

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

If a user has no devices, I don't think I want that to make me stop sending to a whole list of users. I can trivially do the right thing for that user, which is nothing.

Happy to be persuaded though.

Copy link
Member

Choose a reason for hiding this comment

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

An empty userIds arg is a slightly separate case. If I do not specify a user, I cannot encrypt to them.

worth noting that downloadKeys is not a direct map to /keys/query. As things currently stand, downloadKeys will return an entry in the map for each user in the input - even if they have no devices (in which case the device map for that user will be empty).

So no, it's not a separate case: the question is entirely about the behaviour of the function when the userIds arg is empty.

Honestly, I think the actual behaviour here is pretty unimportant, provided the documentation matches. Personally, I would have gone for preserving the previous behaviour, but if you want to change it, that's ok, but please update the docs.

Copy link
Contributor Author

@weeman1337 weeman1337 Mar 31, 2023

Choose a reason for hiding this comment

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

@andybalaam do you mean a misc case here where some users have devices?

To clarify about the discussion from above:

  • Before: 0 users or 0 devices returned by downloadKeystrue
  • Now: false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

If a user has no devices, I don't think I want that to make me stop sending to a whole list of users. I can trivially do the right thing for that user, which is nothing.

First of all, let's clarify. This code is relying on downloadKeys only returning devices which have encryption keys. So we're talking about encryption-capable devices here.

The whole point of this function is to check that all users in a given list have at least one encryption-capable device - which means we can safely encrypt a room which contains those users.

So, I disagree with you here @andybalaam: if any one of the users in the list has no (encryption-capable) devices, I do not want to start an encrypted room with the whole list.

Copy link
Member

Choose a reason for hiding this comment

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

What Rich said makes perfect sense to me.

if (usersDeviceMap.size === 0) return false;

for (const devices of usersDeviceMap.values()) {
if (devices.size === 0) {
return false;
Copy link
Member

Choose a reason for hiding this comment

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

A comment would be useful here:

Suggested change
return false;
// this user does not have any encryption-capable devices.
return false;

}
}
} catch (e) {
logger.error("Error determining if it's possible to encrypt to all users: ", e);
return false; // assume not
}

return true;
}

// Similar to ensureDMExists but also adds creation content
Expand Down
67 changes: 44 additions & 23 deletions test/createRoom-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,35 +147,56 @@ describe("createRoom", () => {
});

describe("canEncryptToAllUsers", () => {
const trueUser = new Map([
[
"@goodUser:localhost",
new Map([
["DEV1", {} as unknown as DeviceInfo],
["DEV2", {} as unknown as DeviceInfo],
]),
],
]);
const user1Id = "@user1:example.com";
const user2Id = "@user2:example.com";

const falseUser = {
"@badUser:localhost": {},
};
const devices = new Map([
["DEV1", {} as unknown as DeviceInfo],
["DEV2", {} as unknown as DeviceInfo],
]);

let client: Mocked<MatrixClient>;
beforeEach(() => {
stubClient();
client = mocked(MatrixClientPeg.get());

beforeAll(() => {
client = mocked(stubClient());
});

it("returns true if all devices have crypto", async () => {
client.downloadKeys.mockResolvedValue(trueUser);
const response = await canEncryptToAllUsers(client, ["@goodUser:localhost"]);
expect(response).toBe(true);
it("should return false if download keys does not return any user", async () => {
client.downloadKeys.mockResolvedValue(new Map());
const result = await canEncryptToAllUsers(client, [user1Id, user2Id]);
expect(result).toBe(false);
});

it("returns false if not all users have crypto", async () => {
client.downloadKeys.mockResolvedValue({ ...trueUser, ...falseUser });
const response = await canEncryptToAllUsers(client, ["@goodUser:localhost", "@badUser:localhost"]);
expect(response).toBe(false);
it("should return false if none of the users has a device", async () => {
client.downloadKeys.mockResolvedValue(
new Map([
[user1Id, new Map()],
[user2Id, new Map()],
]),
);
const result = await canEncryptToAllUsers(client, [user1Id, user2Id]);
expect(result).toBe(false);
});

it("should return false if some of the users don't have a device", async () => {
client.downloadKeys.mockResolvedValue(
new Map([
[user1Id, new Map()],
[user2Id, devices],
]),
);
const result = await canEncryptToAllUsers(client, [user1Id, user2Id]);
expect(result).toBe(false);
});

it("should return true if all users have a device", async () => {
client.downloadKeys.mockResolvedValue(
new Map([
[user1Id, devices],
[user2Id, devices],
]),
);
const result = await canEncryptToAllUsers(client, [user1Id, user2Id]);
expect(result).toBe(true);
});
});