Merge pull request #920 from vector-im/bwindels/fetch-sender-keys-without-tracking-room

Fix keys not being fetched to verify senders when the room isn't tracked yet.

src/matrix/Sync.js

@@ -158,7 +158,7 @@ export class Sync {
         const isCatchupSync = this._status.get() === SyncStatus.CatchupSync;
         const sessionPromise = (async () => {
             try {
-                await log.wrap("session", log => this._session.afterSyncCompleted(sessionChanges, isCatchupSync, log), log.level.Detail);
+                await log.wrap("session", log => this._session.afterSyncCompleted(sessionChanges, isCatchupSync, log));
             } catch (err) {} // error is logged, but don't fail sessionPromise
         })();
         const roomsPromises = roomStates.map(async rs => {

src/matrix/e2ee/DeviceTracker.js

@@ -21,13 +21,17 @@ import {RoomMember} from "../room/members/RoomMember.js";
 const TRACKING_STATUS_OUTDATED = 0;
 const TRACKING_STATUS_UPTODATE = 1;
 
-function addRoomToIdentity(identity, userId, roomId) {
+function createUserIdentity(userId, initialRoomId = undefined) {
-    if (!identity) {
+    return {
-        identity = {
         userId: userId,
-            roomIds: [roomId],
+        roomIds: initialRoomId ? [initialRoomId] : [],
         deviceTrackingStatus: TRACKING_STATUS_OUTDATED,
     };
+}
+
+function addRoomToIdentity(identity, userId, roomId) {
+    if (!identity) {
+        identity = createUserIdentity(userId, roomId);
         return identity;
     } else {
         if (!identity.roomIds.includes(roomId)) {
@@ -120,6 +124,7 @@ export class DeviceTracker {
         const txn = await this._storage.readWriteTxn([
             this._storage.storeNames.roomSummary,
             this._storage.storeNames.userIdentities,
+            this._storage.storeNames.deviceIdentities, // to remove all devices in _removeRoomFromUserIdentity
         ]);
         try {
             let isTrackingChanges;
@@ -127,9 +132,13 @@ export class DeviceTracker {
                 isTrackingChanges = room.writeIsTrackingMembers(true, txn);
                 const members = Array.from(memberList.members.values());
                 log.set("members", members.length);
+                // TODO: should we remove any userIdentities we should not share the key with??
+                // e.g. as an extra security measure if we had a mistake in other code?
                 await Promise.all(members.map(async member => {
                     if (shouldShareKey(member.membership, historyVisibility)) {
                         await this._addRoomToUserIdentity(member.roomId, member.userId, txn);
+                    } else {
+                        await this._removeRoomFromUserIdentity(member.roomId, member.userId, txn);
                     }
                 }));
             } catch (err) {
@@ -202,6 +211,9 @@ export class DeviceTracker {
     async _queryKeys(userIds, hsApi, log) {
         // TODO: we need to handle the race here between /sync and /keys/query just like we need to do for the member list ...
         // there are multiple requests going out for /keys/query though and only one for /members
+        // So, while doing /keys/query, writeDeviceChanges should add userIds marked as outdated to a list
+        // when /keys/query returns, we should check that list and requery if we queried for a given user.
+        // and then remove the list.
 
         const deviceKeyResponse = await hsApi.queryKeys({
             "timeout": 10000,
@@ -264,7 +276,15 @@ export class DeviceTracker {
             txn.deviceIdentities.set(deviceIdentity);
         }
         // mark user identities as up to date
-        const identity = await txn.userIdentities.get(userId);
+        let identity = await txn.userIdentities.get(userId);
+        if (!identity) {
+            // create the identity if it doesn't exist, which can happen if
+            // we request devices before tracking the room.
+            // IMPORTANT here that the identity gets created without any roomId!
+            // if we claim that we share and e2ee room with the user without having
+            // checked, we could share keys with that user without them being in the room
+            identity = createUserIdentity(userId);
+        }
         identity.deviceTrackingStatus = TRACKING_STATUS_UPTODATE;
         txn.userIdentities.set(identity);
 
@@ -323,6 +343,7 @@ export class DeviceTracker {
 
     /**
      * Gives all the device identities for a room that is already tracked.
+     * Can be used to decide which users to share keys with.
      * Assumes room is already tracked. Call `trackRoom` first if unsure.
      * @param  {String} roomId [description]
      * @return {[type]}        [description]
@@ -339,41 +360,90 @@ export class DeviceTracker {
         
         // So, this will also contain non-joined memberships
         const userIds = await txn.roomMembers.getAllUserIds(roomId);
-
+        // TODO: check here if userIds is safe? yes it is
-        return await this._devicesForUserIds(roomId, userIds, txn, hsApi, log);
+        return await this._devicesForUserIdsInTrackedRoom(roomId, userIds, txn, hsApi, log);
     }
 
+    /** 
+     * Can be used to decide which users to share keys with.
+     * Assumes room is already tracked. Call `trackRoom` first if unsure.
+     */
     async devicesForRoomMembers(roomId, userIds, hsApi, log) {
         const txn = await this._storage.readTxn([
             this._storage.storeNames.userIdentities,
         ]);
-        return await this._devicesForUserIds(roomId, userIds, txn, hsApi, log);
+        return await this._devicesForUserIdsInTrackedRoom(roomId, userIds, txn, hsApi, log);
     }
 
     /** 
-     * @param  {string} roomId  [description]
+     * Cannot be used to decide which users to share keys with.
-     * @param  {Array<string>} userIds a set of user ids to try and find the identity for. Will be check to belong to roomId.
+     * Does not assume membership to any room or whether any room is tracked.
+     */
+    async devicesForUsers(userIds, hsApi, log) {
+        const txn = await this._storage.readTxn([
+            this._storage.storeNames.userIdentities,
+        ]);
+
+        const upToDateIdentities = [];
+        const outdatedUserIds = [];
+        await Promise.all(userIds.map(async userId => {
+            const i = await txn.userIdentities.get(userId);
+            if (i && i.deviceTrackingStatus === TRACKING_STATUS_UPTODATE) {
+                upToDateIdentities.push(i);
+            } else if (!i || i.deviceTrackingStatus === TRACKING_STATUS_OUTDATED) {
+                // allow fetching for userIdentities we don't know about yet,
+                // as we don't assume the room is tracked here.
+                outdatedUserIds.push(userId);
+            }
+        }));
+        return this._devicesForUserIdentities(upToDateIdentities, outdatedUserIds, hsApi, log);
+    }
+
+    /**
+     * Gets all the device identities with which keys should be shared for a set of users in a tracked room.
+     * If any userIdentities are outdated, it will fetch them from the homeserver.
+     * @param  {string} roomId the id of the tracked room to filter users by.
+     * @param  {Array<string>} userIds a set of user ids to try and find the identity for.
      * @param  {Transaction} userIdentityTxn to read the user identities
      * @param  {HomeServerApi} hsApi
-     * @return {Array<DeviceIdentity>}
+     * @return {Array<DeviceIdentity>} all devices identities for the given users we should share keys with.
      */
-    async _devicesForUserIds(roomId, userIds, userIdentityTxn, hsApi, log) {
+    async _devicesForUserIdsInTrackedRoom(roomId, userIds, userIdentityTxn, hsApi, log) {
         const allMemberIdentities = await Promise.all(userIds.map(userId => userIdentityTxn.userIdentities.get(userId)));
         const identities = allMemberIdentities.filter(identity => {
-            // identity will be missing for any userIds that don't have 
+            // we use roomIds to decide with whom we should share keys for a given room,
-            // membership join in any of your encrypted rooms
+            // taking into account the membership and room history visibility.
+            // so filter out anyone who we shouldn't share keys with.
+            // Given we assume the room is tracked,
+            // also exclude any userId which doesn't have a userIdentity yet.
             return identity && identity.roomIds.includes(roomId);
         });
         const upToDateIdentities = identities.filter(i => i.deviceTrackingStatus === TRACKING_STATUS_UPTODATE);
-        const outdatedIdentities = identities.filter(i => i.deviceTrackingStatus === TRACKING_STATUS_OUTDATED);
+        const outdatedUserIds = identities
+            .filter(i => i.deviceTrackingStatus === TRACKING_STATUS_OUTDATED)
+            .map(i => i.userId);
+        let devices = await this._devicesForUserIdentities(upToDateIdentities, outdatedUserIds, hsApi, log);
+        // filter out our own device as we should never share keys with it.
+        devices = devices.filter(device => {
+            const isOwnDevice = device.userId === this._ownUserId && device.deviceId === this._ownDeviceId;
+            return !isOwnDevice;
+        });
+        return devices;
+    }
+
+    /** Gets the device identites for a set of user identities that
+     * are known to be up to date, and a set of userIds that are known
+     * to be absent from our store our outdated. The outdated user ids
+     * will have their keys fetched from the homeserver. */
+    async _devicesForUserIdentities(upToDateIdentities, outdatedUserIds, hsApi, log) {
         log.set("uptodate", upToDateIdentities.length);
-        log.set("outdated", outdatedIdentities.length);
+        log.set("outdated", outdatedUserIds.length);
         let queriedDevices;
-        if (outdatedIdentities.length) {
+        if (outdatedUserIds.length) {
             // TODO: ignore the race between /sync and /keys/query for now,
             // where users could get marked as outdated or added/removed from the room while
             // querying keys
-            queriedDevices = await this._queryKeys(outdatedIdentities.map(i => i.userId), hsApi, log);
+            queriedDevices = await this._queryKeys(outdatedUserIds, hsApi, log);
         }
 
         const deviceTxn = await this._storage.readTxn([
@@ -386,12 +456,7 @@ export class DeviceTracker {
         if (queriedDevices && queriedDevices.length) {
             flattenedDevices = flattenedDevices.concat(queriedDevices);
         }
-        // filter out our own device
+        return flattenedDevices;
-        const devices = flattenedDevices.filter(device => {
-            const isOwnDevice = device.userId === this._ownUserId && device.deviceId === this._ownDeviceId;
-            return !isOwnDevice;
-        });
-        return devices;
     }
 
     async getDeviceByCurve25519Key(curve25519Key, txn) {
@@ -731,5 +796,35 @@ export function tests() {
             const txn2 = await storage.readTxn([storage.storeNames.userIdentities]);
             assert.deepEqual((await txn2.userIdentities.get("@bob:hs.tld")).roomIds, ["!abc:hs.tld", "!def:hs.tld"]);
         },
+        "devicesForUsers fetches users even though they aren't in any tracked room": async assert => {
+            const storage = await createMockStorage();
+            const tracker = new DeviceTracker({
+                storage,
+                getSyncToken: () => "token",
+                olmUtil: {ed25519_verify: () => {}}, // valid if it does not throw
+                ownUserId: "@alice:hs.tld",
+                ownDeviceId: "ABCD",
+            });
+            const hsApi = createQueryKeysHSApiMock();
+            const devices = await tracker.devicesForUsers(["@bob:hs.tld"], hsApi, NullLoggerInstance.item);
+            assert.equal(devices.length, 1);
+            assert.equal(devices[0].curve25519Key, "curve25519:@bob:hs.tld:device1:key");
+            const txn1 = await storage.readTxn([storage.storeNames.userIdentities]);
+            assert.deepEqual((await txn1.userIdentities.get("@bob:hs.tld")).roomIds, []);
+        },
+        "devicesForUsers doesn't add any roomId when creating userIdentity": async assert => {
+            const storage = await createMockStorage();
+            const tracker = new DeviceTracker({
+                storage,
+                getSyncToken: () => "token",
+                olmUtil: {ed25519_verify: () => {}}, // valid if it does not throw
+                ownUserId: "@alice:hs.tld",
+                ownDeviceId: "ABCD",
+            });
+            const hsApi = createQueryKeysHSApiMock();
+            await tracker.devicesForUsers(["@bob:hs.tld"], hsApi, NullLoggerInstance.item);
+            const txn1 = await storage.readTxn([storage.storeNames.userIdentities]);
+            assert.deepEqual((await txn1.userIdentities.get("@bob:hs.tld")).roomIds, []);
+        }
     }
 }

src/matrix/e2ee/RoomEncryption.js

@@ -230,9 +230,10 @@ export class RoomEncryption {
                     return senders.add(r.encryptedEvent.sender);
                 }, new Set()));
                 log.set("senders", sendersWithoutDevice);
-                // fetch the devices, ignore return value,
+                // Fetch the devices, ignore return value, and just reuse
-                // and just reuse _verifyDecryptionResults method so we only have one impl how to verify
+                // _verifyDecryptionResults method so we only have one impl how to verify.
-                await this._deviceTracker.devicesForRoomMembers(this._room.id, sendersWithoutDevice, hsApi, log);
+                // Use devicesForUsers rather than devicesForRoomMembers as the room might not be tracked yet
+                await this._deviceTracker.devicesForUsers(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);