Skip to content

Commit

Permalink
Process m.room.encryption events before emitting RoomMember events (
Browse files Browse the repository at this point in the history 
#2910)

* Update tests

* Call `Store.storeRoom` earlier

We're going to call `onCryptoEvent` earlier in `processSyncResponse`, but we
need to have stored the room before doing so. We therefore need to move the
call to `storeRoom` earlier.

We can actually reduce a bit of duplication by moving the call into
`SyncApi.createRoom`.

`storeRoom` has relatively few side-effects, so as far as I can tell this
should be pretty safe.

* Call onCryptoEvent before processing state events

This fixes the problematic race condition.
  • Loading branch information
@richvdh
richvdh authored Nov 25, 2022
1 parent a01d8e3 commit aaf3702
Show file tree
Hide file tree
Showing 4 changed files with 135 additions and 35 deletions.
107 changes: 101 additions & 6 deletions spec/integ/megolm-integ.spec.ts
View file
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 });
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 Expand Up @@ -1364,4 +1376,87 @@ describe("megolm", () => {

await beccaTestClient.stop();
});

it("allows enabling encryption in the createRoom call", async () => {
const testRoomId = "!testRoom:id";
await aliceTestClient.start();

aliceTestClient.httpBackend.when("POST", "/keys/query")
.respond(200, function(_path, content: IUploadKeysRequest) {
return { device_keys: {} };
});

/* Alice makes the /createRoom call */
aliceTestClient.httpBackend.when("POST", "/createRoom")
.respond(200, { room_id: testRoomId });
await Promise.all([
aliceTestClient.client.createRoom({
initial_state: [{
type: 'm.room.encryption',
state_key: '',
content: { algorithm: 'm.megolm.v1.aes-sha2' },
}],
}),
aliceTestClient.httpBackend.flushAllExpected(),
]);

/* The sync arrives in two parts; first the m.room.create... */
aliceTestClient.httpBackend.when("GET", "/sync").respond(200, {
rooms: { join: {
[testRoomId]: {
timeline: { events: [
{
type: 'm.room.create',
state_key: '',
event_id: "$create",
},
{
type: 'm.room.member',
state_key: aliceTestClient.getUserId(),
content: { membership: "join" },
event_id: "$alijoin",
},
] },
},
} },
});
await aliceTestClient.flushSync();

// ... and then the e2e event and an invite ...
aliceTestClient.httpBackend.when("GET", "/sync").respond(200, {
rooms: { join: {
[testRoomId]: {
timeline: { events: [
{
type: 'm.room.encryption',
state_key: '',
content: { algorithm: 'm.megolm.v1.aes-sha2' },
event_id: "$e2e",
},
{
type: 'm.room.member',
state_key: "@other:user",
content: { membership: "invite" },
event_id: "$otherinvite",
},
] },
},
} },
});

// as soon as the roomMember arrives, try to send a message
aliceTestClient.client.on(RoomStateEvent.NewMember, (_e, _s, member: RoomMember) => {
if (member.userId == "@other:user") {
aliceTestClient.client.sendMessage(testRoomId, { msgtype: "m.text", body: "Hello, World" });
}
});

// flush the sync and wait for the /send/ request.
aliceTestClient.httpBackend.when("PUT", "/send/m.room.encrypted/")
.respond(200, (_path, _content) => ({ event_id: "asdfgh" }));
await Promise.all([
aliceTestClient.flushSync(),
aliceTestClient.httpBackend.flush("/send/m.room.encrypted/", 1),
]);
});
});
24 changes: 15 additions & 9 deletions spec/test-utils/test-utils.ts
View file
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
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2815,7 +2815,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
37 changes: 18 additions & 19 deletions src/sync.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ export class SyncApi {
this.onMarkerStateEvent(room, markerEvent, markerFoundOptions);
});

this.client.store.storeRoom(room);
return room;
}

Expand Down Expand Up @@ -337,7 +338,6 @@ export class SyncApi {
await this.injectRoomEvents(room, stateEvents, events);

room.recalculate();
client.store.storeRoom(room);
client.emit(ClientEvent.Room, room);

this.processEventsForNotifs(room, events);
Expand Down Expand Up @@ -1231,7 +1231,6 @@ export class SyncApi {

if (inviteObj.isBrandNewRoom) {
room.recalculate();
client.store.storeRoom(room);
client.emit(ClientEvent.Room, room);
} else {
// Update room state for invite->reject->invite cycles
Expand Down Expand Up @@ -1362,6 +1361,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(e);
}
}
}

try {
await this.injectRoomEvents(room, stateEvents, events, syncEventData.fromCache);
} catch (e) {
Expand All @@ -1383,27 +1394,16 @@ export class SyncApi {

room.recalculate();
if (joinObj.isBrandNewRoom) {
client.store.storeRoom(room);
client.emit(ClientEvent.Room, room);
}

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 All @@ -1423,7 +1423,6 @@ export class SyncApi {

room.recalculate();
if (leaveObj.isBrandNewRoom) {
client.store.storeRoom(room);
client.emit(ClientEvent.Room, room);
}

Expand Down

0 comments on commit aaf3702

Please sign in to comment.