Merge pull request #913 from vector-im/bwindels/verify-unfetched-roomkey-senders

Verify unfetched roomkey senders

src/matrix/Sync.js

@@ -47,10 +47,8 @@ function timelineIsEmpty(roomResponse) {
  * const changes = await room.writeSync(roomResponse, isInitialSync, preparation, syncTxn);
  * // applies and emits changes once syncTxn is committed
  * room.afterSync(changes);
- * if (room.needsAfterSyncCompleted(changes)) {
+ * // can do network requests
- *     // can do network requests
+ * await room.afterSyncCompleted(changes);
- *     await room.afterSyncCompleted(changes);
- * }
  * ```
  */
 export class Sync {
@@ -163,13 +161,9 @@ export class Sync {
                 await log.wrap("session", log => this._session.afterSyncCompleted(sessionChanges, isCatchupSync, log), log.level.Detail);
             } catch (err) {} // error is logged, but don't fail sessionPromise
         })();
-
+        const roomsPromises = roomStates.map(async rs => {
-        const roomsNeedingAfterSyncCompleted = roomStates.filter(rs => {
-            return rs.room.needsAfterSyncCompleted(rs.changes);
-        });
-        const roomsPromises = roomsNeedingAfterSyncCompleted.map(async rs => {
             try {
-                await log.wrap("room", log => rs.room.afterSyncCompleted(rs.changes, log), log.level.Detail);
+                await rs.room.afterSyncCompleted(rs.changes, log);
             } catch (err) {} // error is logged, but don't fail roomsPromises
         });
         // run everything in parallel,

src/matrix/e2ee/DecryptionResult.ts

@@ -27,6 +27,7 @@ limitations under the License.
  */
 
 import type {DeviceIdentity} from "../storage/idb/stores/DeviceIdentityStore";
+import type {TimelineEvent} from "../storage/types";
 
 type DecryptedEvent = {
     type?: string,
@@ -35,22 +36,18 @@ type DecryptedEvent = {
 
 export class DecryptionResult {
     private device?: DeviceIdentity;
-    private roomTracked: boolean = true;
 
     constructor(
         public readonly event: DecryptedEvent,
         public readonly senderCurve25519Key: string,
-        public readonly claimedEd25519Key: string
+        public readonly claimedEd25519Key: string,
+        public readonly encryptedEvent?: TimelineEvent
     ) {}
 
     setDevice(device: DeviceIdentity): void {
         this.device = device;
     }
 
-    setRoomNotTrackedYet(): void {
-        this.roomTracked = false;
-    }
-
     get isVerified(): boolean {
         if (this.device) {
             const comesFromDevice = this.device.ed25519Key === this.claimedEd25519Key;
@@ -62,15 +59,12 @@ export class DecryptionResult {
     get isUnverified(): boolean {
         if (this.device) {
             return !this.isVerified;
-        } else if (this.isVerificationUnknown) {
-            return false;
         } else {
             return true;
         }
     }
 
     get isVerificationUnknown(): boolean {
-        // verification is unknown if we haven't yet fetched the devices for the room
+        return !this.device;
-        return !this.device && !this.roomTracked;
     }
 }

src/matrix/e2ee/RoomEncryption.js

@@ -208,17 +208,43 @@ export class RoomEncryption {
         });
     }
 
-    async _verifyDecryptionResult(result, txn) {
+    async _verifyDecryptionResults(results, txn) {
-        let device = this._senderDeviceCache.get(result.senderCurve25519Key);
+        await Promise.all(results.map(async result => {
-        if (!device) {
+            let device = this._senderDeviceCache.get(result.senderCurve25519Key);
-            device = await this._deviceTracker.getDeviceByCurve25519Key(result.senderCurve25519Key, txn);
+            if (!device) {
-            this._senderDeviceCache.set(result.senderCurve25519Key, device);
+                device = await this._deviceTracker.getDeviceByCurve25519Key(result.senderCurve25519Key, txn);
-        }
+                this._senderDeviceCache.set(result.senderCurve25519Key, device);
-        if (device) {
+            }
-            result.setDevice(device);
+            if (device) {
-        } else if (!this._room.isTrackingMembers) {
+                result.setDevice(device);
-            result.setRoomNotTrackedYet();
+            }
+        }));
+    }
+
+    /** fetches the devices that are not yet known locally from the homeserver to verify the sender of this message. */
+    async _fetchKeyAndVerifyDecryptionResults(results, hsApi, log) {
+        const resultsWithoutDevice = results.filter(r => r.isVerificationUnknown);
+        if (resultsWithoutDevice.length) {
+            return log.wrap("fetch unverified senders", async log => {
+                const sendersWithoutDevice = Array.from(resultsWithoutDevice.reduce((senders, r) => {
+                    return senders.add(r.encryptedEvent.sender);
+                }, new Set()));
+                log.set("senders", sendersWithoutDevice);
+                // fetch the devices, ignore return value,
+                // and just reuse _verifyDecryptionResults method so we only have one impl how to verify
+                await this._deviceTracker.devicesForRoomMembers(this._room.id, sendersWithoutDevice, hsApi, log);
+                // now that we've fetched the missing devices, try verifying the results again
+                const txn = await this._storage.readTxn([this._storage.storeNames.deviceIdentities]);
+                await this._verifyDecryptionResults(resultsWithoutDevice, txn);
+                const resultsWithFoundDevice = resultsWithoutDevice.filter(r => !r.isVerificationUnknown);
+                const resultsToEventIdMap = resultsWithFoundDevice.reduce((map, r) => {
+                    map.set(r.encryptedEvent.event_id, r);
+                    return map;
+                }, new Map());
+                return new BatchDecryptionResult(resultsToEventIdMap, new Map(), this);
+            });
         }
+        return new BatchDecryptionResult(new Map(), new Map(), this);
     }
 
     async _requestMissingSessionFromBackup(senderKey, sessionId, log) {
@@ -531,24 +557,42 @@ class BatchDecryptionResult {
         this._roomEncryption = roomEncryption;
     }
 
-    applyToEntries(entries) {
+    applyToEntries(entries, callback = undefined) {
         for (const entry of entries) {
             const result = this.results.get(entry.id);
             if (result) {
                 entry.setDecryptionResult(result);
+                callback?.(entry);
             } else {
                 const error = this.errors.get(entry.id);
                 if (error) {
                     entry.setDecryptionError(error);
+                    callback?.(entry);
                 }
             }
         }
     }
 
-    verifySenders(txn) {
+    /** Verify the decryption results by looking for the corresponding device in local persistance
-        return Promise.all(Array.from(this.results.values()).map(result => {
+     *  @returns {BatchDecryptionResult} a new batch result with the results for which we now found a device */
-            return this._roomEncryption._verifyDecryptionResult(result, txn);
+    verifyKnownSenders(txn) {
-        }));
+        return this._roomEncryption._verifyDecryptionResults(Array.from(this.results.values()), txn);
+    }
+
+    get hasUnverifiedSenders() {
+        for (const r of this.results.values()) {
+            if (r.isVerificationUnknown) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    /** Verify any decryption results for which we could not find a device when
+     *  calling `verifyKnownSenders` prior, by fetching them from the homeserver.
+     *  @returns {Promise<BatchDecryptionResult>} the results for which we found a device */
+    fetchAndVerifyRemainingSenders(hsApi, log) {
+        return this._roomEncryption._fetchKeyAndVerifyDecryptionResults(Array.from(this.results.values()), hsApi, log);
     }
 }
 
@@ -556,7 +600,6 @@ import {createMockStorage} from "../../mocks/Storage";
 import {Clock as MockClock} from "../../mocks/Clock";
 import {poll} from "../../mocks/poll";
 import {Instance as NullLoggerInstance} from "../../logging/NullLogger";
-import {ConsoleLogger} from "../../logging/ConsoleLogger";
 import {HomeServer as MockHomeServer} from "../../mocks/HomeServer.js";
 
 export function tests() {
@@ -665,7 +708,7 @@ export function tests() {
             const storage = await createMockStorage();
             let isMemberChangesCalled = false;
             const deviceTracker = {
-                async writeMemberChanges(room, memberChanges, historyVisibility, txn) {
+                async writeMemberChanges(room, memberChanges, historyVisibility) {
                     assert.equal(historyVisibility, "invited");
                     isMemberChangesCalled = true;
                     return {removed: [], added: []};

src/matrix/e2ee/megolm/decryption/SessionDecryption.ts

@@ -31,17 +31,14 @@ interface DecryptAllResult {
  * Does the actual decryption of all events for a given megolm session in a batch
  */
 export class SessionDecryption {
-    private key: RoomKey;
-    private events: TimelineEvent[];
-    private keyLoader: KeyLoader;
-    private olmWorker?: OlmWorker;
     private decryptionRequests?: any[];
 
-    constructor(key: RoomKey, events: TimelineEvent[], olmWorker: OlmWorker | undefined, keyLoader: KeyLoader) {
+    constructor(
-        this.key = key;
+        private readonly key: RoomKey,
-        this.events = events;
+        private readonly events: TimelineEvent[],
-        this.olmWorker = olmWorker;
+        private readonly olmWorker: OlmWorker | undefined,
-        this.keyLoader = keyLoader;
+        private readonly keyLoader: KeyLoader
+    ) {
         this.decryptionRequests = olmWorker ? [] : undefined;
     }
 
@@ -75,7 +72,7 @@ export class SessionDecryption {
                             {encryptedRoomId: payload.room_id, eventRoomId: this.key.roomId});
                     }
                     replayEntries.push(new ReplayDetectionEntry(this.key.sessionId, decryptionResult!.message_index, event));
-                    const result = new DecryptionResult(payload, this.key.senderKey, this.key.claimedEd25519Key);
+                    const result = new DecryptionResult(payload, this.key.senderKey, this.key.claimedEd25519Key, event);
                     results.set(event.event_id, result);
                 } catch (err) {
                     // ignore AbortError from cancelling decryption requests in dispose method

src/matrix/room/BaseRoom.js

@@ -154,7 +154,7 @@ export class BaseRoom extends EventEmitter {
             try {
                 decryption = await changes.write(writeTxn, log);
                 if (isTimelineOpen) {
-                    await decryption.verifySenders(writeTxn);
+                    await decryption.verifyKnownSenders(writeTxn);
                 }
             } catch (err) {
                 writeTxn.abort();
@@ -166,6 +166,16 @@ export class BaseRoom extends EventEmitter {
             if (this._observedEvents) {
                 this._observedEvents.updateEvents(entries);
             }
+            if (isTimelineOpen && decryption.hasUnverifiedSenders) {
+                // verify missing senders async and update timeline once done so we don't delay rendering with network requests
+                log.wrapDetached("fetch unknown senders keys", async log => {
+                    const newlyVerifiedDecryption = await decryption.fetchAndVerifyRemainingSenders(this._hsApi, log);
+                    const verifiedEntries = [];
+                    newlyVerifiedDecryption.applyToEntries(entries, entry => verifiedEntries.push(entry));
+                    this._timeline?.replaceEntries(verifiedEntries);
+                    this._observedEvents?.updateEvents(verifiedEntries);
+                });
+            }
         }, ensureLogItem(log));
         return request;
     }

src/matrix/room/Room.js

@@ -124,12 +124,13 @@ export class Room extends BaseRoom {
         const {entries: newEntries, updatedEntries, newLiveKey, memberChanges} =
             await log.wrap("syncWriter", log => this._syncWriter.writeSync(
                 roomResponse, isRejoin, summaryChanges.hasFetchedMembers, txn, log), log.level.Detail);
+        let decryption;
         if (decryptChanges) {
-            const decryption = await log.wrap("decryptChanges", log => decryptChanges.write(txn, log));
+            decryption = await log.wrap("decryptChanges", log => decryptChanges.write(txn, log));
             log.set("decryptionResults", decryption.results.size);
             log.set("decryptionErrors", decryption.errors.size);
             if (this._isTimelineOpen) {
-                await decryption.verifySenders(txn);
+                await decryption.verifyKnownSenders(txn);
             }
             decryption.applyToEntries(newEntries);
             if (retryEntries?.length) {
@@ -189,6 +190,7 @@ export class Room extends BaseRoom {
             heroChanges,
             powerLevelsEvent,
             encryptionChanges,
+            decryption
         };
     }
 
@@ -291,19 +293,36 @@ export class Room extends BaseRoom {
         }
     }
 
-    needsAfterSyncCompleted({encryptionChanges}) {
-        return encryptionChanges?.shouldFlush;
-    }
-
     /**
      * Only called if the result of writeSync had `needsAfterSyncCompleted` set.
      * Can be used to do longer running operations that resulted from the last sync,
      * like network operations.
      */
-    async afterSyncCompleted(changes, log) {
+    async afterSyncCompleted({encryptionChanges, decryption, newEntries, updatedEntries}, log) {
-        log.set("id", this.id);
+        const shouldFlushKeys = encryptionChanges?.shouldFlush;
-        if (this._roomEncryption) {
+        const shouldFetchUnverifiedSenders = this._isTimelineOpen && decryption?.hasUnverifiedSenders;
-            await this._roomEncryption.flushPendingRoomKeyShares(this._hsApi, null, log);
+        // only log rooms where we actually do something
+        if (shouldFlushKeys || shouldFetchUnverifiedSenders) {
+            await log.wrap({l: "room", id: this.id}, async log => {
+                const promises = [];
+                if (shouldFlushKeys) {
+                    promises.push(this._roomEncryption.flushPendingRoomKeyShares(this._hsApi, null, log));
+                }
+                if (shouldFetchUnverifiedSenders) {
+                    const promise = log.wrap("verify senders", (async log => {
+                        const newlyVerifiedDecryption = await decryption.fetchAndVerifyRemainingSenders(this._hsApi, log);
+                        const verifiedEntries = [];
+                        const updateCallback = entry => verifiedEntries.push(entry);
+                        newlyVerifiedDecryption.applyToEntries(newEntries, updateCallback);
+                        newlyVerifiedDecryption.applyToEntries(updatedEntries, updateCallback);
+                        log.set("verifiedEntries", verifiedEntries.length);
+                        this._timeline?.replaceEntries(verifiedEntries);
+                        this._observedEvents?.updateEvents(verifiedEntries);
+                    }));
+                    promises.push(promise);
+                }
+                await Promise.all(promises);
+            });
         }
     }
 

src/matrix/room/timeline/entries/EventEntry.js

@@ -33,10 +33,11 @@ export class EventEntry extends BaseEventEntry {
     }
 
     updateFrom(other) {
-        if (other._decryptionResult && !this._decryptionResult) {
+        // only update these when we attempted decryption, as some updates (like reactions) don't.
+        if (other._decryptionResult) {
             this._decryptionResult = other._decryptionResult;
         }
-        if (other._decryptionError && !this._decryptionError) {
+        if (other._decryptionError) {
             this._decryptionError = other._decryptionError;
         }
         this._contextForEntries = other.contextForEntries;

src/platform/web/ui/session/room/timeline/BaseMessageView.js

@@ -46,7 +46,7 @@ export class BaseMessageView extends TemplateView {
                 "Timeline_message": true,
                 own: vm.isOwn,
                 unsent: vm.isUnsent,
-                unverified: vm.isUnverified,
+                unverified: vm => vm.isUnverified,
                 disabled: !this._interactive,
                 continuation: vm => vm.isContinuation,
             },