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 1 commit
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
2 changes: 1 addition & 1 deletion src/crypto/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2856,7 +2856,7 @@ 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.",
);
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(room, 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