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.

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

Process m.room.encryption events before emitting RoomMember events #2914

Merged
merged 8 commits into from
Nov 30, 2022
Merged
Show file tree
Hide file tree
Changes from 5 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
24 changes: 18 additions & 6 deletions spec/integ/megolm-integ.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,19 @@ import * as testUtils from "../test-utils/test-utils";
import { TestClient } from "../TestClient";
import { logger } from "../../src/logger";
import {
IClaimOTKsResult,
IContent,
IDownloadKeyResult,
IEvent,
IClaimOTKsResult,
IJoinedRoom,
IndexedDBCryptoStore,
ISyncResponse,
IDownloadKeyResult,
IUploadKeysRequest,
MatrixEvent,
MatrixEventEvent,
IndexedDBCryptoStore,
Room,
RoomMember,
RoomStateEvent,
} from "../../src/matrix";
import { IDeviceKeys } from "../../src/crypto/dehydration";
import { DeviceInfo } from "../../src/crypto/deviceinfo";
Expand Down Expand Up @@ -327,7 +330,9 @@ describe("megolm", () => {
const room = aliceTestClient.client.getRoom(ROOM_ID)!;
const event = room.getLiveTimeline().getEvents()[0];
expect(event.isEncrypted()).toBe(true);
const decryptedEvent = await testUtils.awaitDecryption(event);

// it probably won't be decrypted yet, because it takes a while to process the olm keys
const decryptedEvent = await testUtils.awaitDecryption(event, { waitOnDecryptionFailure: true });
Comment on lines +334 to +335
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 is needed due to timing differences in processing /sync responses: there are fewer awaits, which means that we finish processing the /sync while there are still to-device messages in the queue for processing.

expect(decryptedEvent.getContent().body).toEqual('42');
});

Expand Down Expand Up @@ -873,7 +878,12 @@ describe("megolm", () => {

const room = aliceTestClient.client.getRoom(ROOM_ID)!;
await room.decryptCriticalEvents();
expect(room.getLiveTimeline().getEvents()[0].getContent().body).toEqual('42');

// it probably won't be decrypted yet, because it takes a while to process the olm keys
const decryptedEvent = await testUtils.awaitDecryption(
room.getLiveTimeline().getEvents()[0], { waitOnDecryptionFailure: true },
);
expect(decryptedEvent.getContent().body).toEqual('42');

const exported = await aliceTestClient.client.exportRoomKeys();

Expand Down Expand Up @@ -1012,7 +1022,9 @@ describe("megolm", () => {
const room = aliceTestClient.client.getRoom(ROOM_ID)!;
const event = room.getLiveTimeline().getEvents()[0];
expect(event.isEncrypted()).toBe(true);
const decryptedEvent = await testUtils.awaitDecryption(event);

// it probably won't be decrypted yet, because it takes a while to process the olm keys
const decryptedEvent = await testUtils.awaitDecryption(event, { waitOnDecryptionFailure: true });
expect(decryptedEvent.getRoomId()).toEqual(ROOM_ID);
expect(decryptedEvent.getContent()).toEqual({});
expect(decryptedEvent.getClearContent()).toBeUndefined();
Expand Down
24 changes: 15 additions & 9 deletions spec/test-utils/test-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -362,22 +362,28 @@ export class MockStorageApi {
* @param {MatrixEvent} event
* @returns {Promise} promise which resolves (to `event`) when the event has been decrypted
*/
export async function awaitDecryption(event: MatrixEvent): Promise<MatrixEvent> {
export async function awaitDecryption(
event: MatrixEvent, { waitOnDecryptionFailure = false } = {},
): Promise<MatrixEvent> {
// An event is not always decrypted ahead of time
// getClearContent is a good signal to know whether an event has been decrypted
// already
if (event.getClearContent() !== null) {
return event;
if (waitOnDecryptionFailure && event.isDecryptionFailure()) {
logger.log(`${Date.now()} event ${event.getId()} got decryption error; waiting`);
} else {
return event;
}
} else {
logger.log(`${Date.now()} event ${event.getId()} is being decrypted; waiting`);
logger.log(`${Date.now()} event ${event.getId()} is not yet decrypted; waiting`);
}

return new Promise((resolve) => {
event.once(MatrixEventEvent.Decrypted, (ev) => {
logger.log(`${Date.now()} event ${event.getId()} now decrypted`);
resolve(ev);
});
return new Promise((resolve) => {
event.once(MatrixEventEvent.Decrypted, (ev) => {
logger.log(`${Date.now()} event ${event.getId()} now decrypted`);
resolve(ev);
});
}
});
}

export const emitPromise = (e: EventEmitter, k: string): Promise<any> => new Promise(r => e.once(k, r));
Expand Down
87 changes: 59 additions & 28 deletions src/crypto/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2552,12 +2552,45 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
* @param {boolean=} inhibitDeviceQuery true to suppress device list query for
* users in the room (for now). In case lazy loading is enabled,
* the device query is always inhibited as the members are not tracked.
*
* @deprecated It is normally incorrect to call this method directly. Encryption
* is enabled by receiving an `m.room.encryption` event (which we may have sent
* previously).
*/
public async setRoomEncryption(
roomId: string,
config: IRoomEncryption,
inhibitDeviceQuery?: boolean,
): Promise<void> {
const room = this.clientStore.getRoom(roomId);
if (!room) {
throw new Error(`Unable to enable encryption tracking devices in unknown room ${roomId}`);
}
await this.setRoomEncryptionImpl(room, config);
if (!this.lazyLoadMembers && !inhibitDeviceQuery) {
this.deviceList.refreshOutdatedDeviceLists();
}
}

/**
* Set up encryption for a room.
*
* This is called when an <tt>m.room.encryption</tt> event is received. It saves a flag
* for the room in the cryptoStore (if it wasn't already set), sets up an "encryptor" for
* the room, and enables device-list tracking for the room.
*
* It does <em>not</em> initiate a device list query for the room. That is normally
* done once we finish processing the sync, in onSyncCompleted.
*
* @param room The room to enable encryption in.
* @param config The encryption config for the room.
*/
private async setRoomEncryptionImpl(
room: Room,
config: IRoomEncryption,
): Promise<void> {
const roomId = room.roomId;

// ignore crypto events with no algorithm defined
// This will happen if a crypto event is redacted before we fetch the room state
// It would otherwise just throw later as an unknown algorithm would, but we may
Expand Down Expand Up @@ -2625,14 +2658,7 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
logger.log("Enabling encryption in " + roomId + "; " +
"starting to track device lists for all users therein");

await this.trackRoomDevices(roomId);
// TODO: this flag is only not used from MatrixClient::setRoomEncryption
// which is never used (inside Element at least)
// but didn't want to remove it as it technically would
// be a breaking change.
if (!inhibitDeviceQuery) {
this.deviceList.refreshOutdatedDeviceLists();
}
await this.trackRoomDevicesImpl(room);
} else {
logger.log("Enabling encryption in " + roomId);
}
Expand All @@ -2645,15 +2671,30 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
* @returns {Promise} when all devices for the room have been fetched and marked to track
*/
public trackRoomDevices(roomId: string): Promise<void> {
const room = this.clientStore.getRoom(roomId);
if (!room) {
throw new Error(`Unable to start tracking devices in unknown room ${roomId}`);
}
return this.trackRoomDevicesImpl(room);
}

/**
* Make sure we are tracking the device lists for all users in this room.
*
* This is normally called when we are about to send an encrypted event, to make sure
* we have all the devices in the room; but it is also called when processing an
* m.room.encryption state event (if lazy-loading is disabled), or when members are
* loaded (if lazy-loading is enabled), to prepare the device list.
*
* @param room Room to enable device-list tracking in
*/
private trackRoomDevicesImpl(room: Room): Promise<void> {
const roomId = room.roomId;
const trackMembers = async (): Promise<void> => {
// not an encrypted room
if (!this.roomEncryptors.has(roomId)) {
return;
}
const room = this.clientStore.getRoom(roomId);
if (!room) {
throw new Error(`Unable to start tracking devices in unknown room ${roomId}`);
}
logger.log(`Starting to track devices for room ${roomId} ...`);
const members = await room.getEncryptionTargetMembers();
members.forEach((m) => {
Expand Down Expand Up @@ -2815,17 +2856,14 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
// MatrixClient has already checked that this room should be encrypted,
// so this is an unexpected situation.
throw new Error(
"Room was previously configured to use encryption, but is " +
"Room " + roomId + " was previously configured to use encryption, but is " +
"no longer. Perhaps the homeserver is hiding the " +
"configuration event.",
);
}

if (!this.roomDeviceTrackingState[roomId]) {
this.trackRoomDevices(roomId);
}
// wait for all the room devices to be loaded
await this.roomDeviceTrackingState[roomId];
await this.trackRoomDevicesImpl(room);

let content = event.getContent();
// If event has an m.relates_to then we need
Expand Down Expand Up @@ -2972,19 +3010,12 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
/**
* handle an m.room.encryption event
*
* @param {module:models/event.MatrixEvent} event encryption event
* @param room in which the event was received
* @param event encryption event to be processed
*/
public async onCryptoEvent(event: MatrixEvent): Promise<void> {
const roomId = event.getRoomId()!;
public async onCryptoEvent(room: Room, event: MatrixEvent): Promise<void> {
Copy link
Member Author

Choose a reason for hiding this comment

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

strictly speaking, this is a breaking change, because this method is accessible outside the js-sdk. But it's really not intended to be called anywhere except internally.

If we really want I could add a new onCryptoEventWithARealRoom method, use that from sync.ts etc, and deprecate the existing method. But it feels to me like that would be adding a bunch of dead code for very little gain.

const content = event.getContent<IRoomEncryption>();

try {
// inhibit the device list refresh for now - it will happen once we've
// finished processing the sync, in onSyncCompleted.
await this.setRoomEncryption(roomId, content, true);
} catch (e) {
logger.error(`Error configuring encryption in room ${roomId}`, e);
}
await this.setRoomEncryptionImpl(room, content);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/sliding-sync-sdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -703,7 +703,7 @@ export class SlidingSyncSdk {
const processRoomEvent = async (e: MatrixEvent): Promise<void> => {
client.emit(ClientEvent.Event, e);
if (e.isState() && e.getType() == EventType.RoomEncryption && this.opts.crypto) {
await this.opts.crypto.onCryptoEvent(e);
await this.opts.crypto.onCryptoEvent(room, e);
}
};

Expand Down
32 changes: 17 additions & 15 deletions src/sync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1362,6 +1362,18 @@ export class SyncApi {
}
}

// process any crypto events *before* emitting the RoomStateEvent events. This
// avoids a race condition if the application tries to send a message after the
// state event is processed, but before crypto is enabled, which then causes the
// crypto layer to complain.
if (this.opts.crypto) {
for (const e of stateEvents.concat(events)) {
if (e.isState() && e.getType() === EventType.RoomEncryption && e.getStateKey() === "") {
await this.opts.crypto.onCryptoEvent(room, e);
}
}
}

try {
await this.injectRoomEvents(room, stateEvents, events, syncEventData.fromCache);
} catch (e) {
Expand Down Expand Up @@ -1389,21 +1401,11 @@ export class SyncApi {

this.processEventsForNotifs(room, events);

const processRoomEvent = async (e): Promise<void> => {
client.emit(ClientEvent.Event, e);
if (e.isState() && e.getType() == "m.room.encryption" && this.opts.crypto) {
await this.opts.crypto.onCryptoEvent(e);
}
};

await utils.promiseMapSeries(stateEvents, processRoomEvent);
await utils.promiseMapSeries(events, processRoomEvent);
ephemeralEvents.forEach(function(e) {
client.emit(ClientEvent.Event, e);
});
accountDataEvents.forEach(function(e) {
client.emit(ClientEvent.Event, e);
});
const emitEvent = (e: MatrixEvent): boolean => client.emit(ClientEvent.Event, e);
stateEvents.forEach(emitEvent);
events.forEach(emitEvent);
ephemeralEvents.forEach(emitEvent);
accountDataEvents.forEach(emitEvent);

// Decrypt only the last message in all rooms to make sure we can generate a preview
// And decrypt all events after the recorded read receipt to ensure an accurate
Expand Down