Handle 'left' users in the deviceList mananagement

When we no longer share any rooms with a given user, the server will stop
sending us updates on their device list, and will (once synapse is updated)
send us a notification of that fact via the 'left' field in the device_lists
field in /sync, or the response from /keys/changes.

spec/TestClient.js

@@ -33,12 +33,20 @@ import Promise from 'bluebird';
  * @param {string} userId
  * @param {string} deviceId
  * @param {string} accessToken
+ *
+ * @param {WebStorage=} sessionStoreBackend a web storage object to use for the
+ *     session store. If undefined, we will create a MockStorageApi.
  */
-export default function TestClient(userId, deviceId, accessToken) {
+export default function TestClient(
+    userId, deviceId, accessToken, sessionStoreBackend,
+) {
     this.userId = userId;
     this.deviceId = deviceId;
 
-    this.storage = new sdk.WebStorageSessionStore(new testUtils.MockStorageApi());
+    if (sessionStoreBackend === undefined) {
+        sessionStoreBackend = new testUtils.MockStorageApi();
+    }
+    this.storage = new sdk.WebStorageSessionStore(sessionStoreBackend);
     this.httpBackend = new MockHttpBackend();
     this.client = sdk.createClient({
         baseUrl: "http://" + userId + ".test.server",

spec/integ/devicelist-integ-spec.js

@@ -52,21 +52,31 @@ function getSyncResponse(roomMembers) {
 }
 
 
-describe("DeviceList management", function() {
+describe("DeviceList management:", function() {
     if (!global.Olm) {
         console.warn('not running deviceList tests: Olm not present');
         return;
     }
 
+    let sessionStoreBackend;
     let aliceTestClient;
 
+    async function createTestClient() {
+        const testClient = new TestClient(
+            "@alice:localhost", "xzcvb", "akjgkrgjs", sessionStoreBackend,
+        );
+        await testClient.client.initCrypto();
+        return testClient;
+    }
+
     beforeEach(async function() {
         testUtils.beforeEach(this); // eslint-disable-line no-invalid-this
 
-        aliceTestClient = new TestClient(
+        // we create our own sessionStoreBackend so that we can use it for
-            "@alice:localhost", "xzcvb", "akjgkrgjs",
+        // another TestClient.
-        );
+        sessionStoreBackend = new testUtils.MockStorageApi();
-        await aliceTestClient.client.initCrypto();
+
+        aliceTestClient = await createTestClient();
     });
 
     afterEach(function() {
@@ -234,4 +244,126 @@ describe("DeviceList management", function() {
                expect(aliceTestClient.storage.getEndToEndDeviceSyncToken()).toEqual(3);
            });
        });
+
+    // https://github.com/vector-im/riot-web/issues/4983
+    describe("Alice should know she has stale device lists", () => {
+        beforeEach(async function() {
+            await aliceTestClient.start();
+
+            aliceTestClient.httpBackend.when('GET', '/sync').respond(
+                200, getSyncResponse(['@bob:xyz']));
+            await aliceTestClient.flushSync();
+
+            aliceTestClient.httpBackend.when('POST', '/keys/query').respond(
+                200, {
+                    device_keys: {
+                        '@bob:xyz': {},
+                    },
+                },
+            );
+            await aliceTestClient.httpBackend.flush('/keys/query', 1);
+
+            const bobStat = aliceTestClient.storage
+                      .getEndToEndDeviceTrackingStatus()['@bob:xyz'];
+
+            expect(bobStat).toBeGreaterThan(
+                0, "Alice should be tracking bob's device list",
+            );
+        });
+
+        it("when Bob leaves", async function() {
+            aliceTestClient.httpBackend.when('GET', '/sync').respond(
+                200, {
+                    next_batch: 2,
+                    device_lists: {
+                        left: ['@bob:xyz'],
+                    },
+                    rooms: {
+                        join: {
+                            [ROOM_ID]: {
+                                timeline: {
+                                    events: [
+                                        testUtils.mkMembership({
+                                            mship: 'leave',
+                                            sender: '@bob:xyz',
+                                        }),
+                                    ],
+                                },
+                            },
+                        },
+                    },
+                },
+            );
+
+
+            await aliceTestClient.flushSync();
+
+            const bobStat = aliceTestClient.storage
+                      .getEndToEndDeviceTrackingStatus()['@bob:xyz'];
+            expect(bobStat).toEqual(
+                0, "Alice should have marked bob's device list as untracked",
+            );
+        });
+
+        it("when Alice leaves", async function() {
+            aliceTestClient.httpBackend.when('GET', '/sync').respond(
+                200, {
+                    next_batch: 2,
+                    device_lists: {
+                        left: ['@bob:xyz'],
+                    },
+                    rooms: {
+                        leave: {
+                            [ROOM_ID]: {
+                                timeline: {
+                                    events: [
+                                        testUtils.mkMembership({
+                                            mship: 'leave',
+                                            sender: '@bob:xyz',
+                                        }),
+                                    ],
+                                },
+                            },
+                        },
+                    },
+                },
+            );
+
+            await aliceTestClient.flushSync();
+
+            const bobStat = aliceTestClient.storage
+                      .getEndToEndDeviceTrackingStatus()['@bob:xyz'];
+            expect(bobStat).toEqual(
+                0, "Alice should have marked bob's device list as untracked",
+            );
+        });
+
+        it("when Bob leaves whilst Alice is offline", async function() {
+            aliceTestClient.stop();
+
+            const anotherTestClient = await createTestClient();
+
+            try {
+                anotherTestClient.httpBackend.when('GET', '/keys/changes').respond(
+                    200, {
+                        changed: [],
+                        left: ['@bob:xyz'],
+                    },
+                );
+                await anotherTestClient.start();
+                anotherTestClient.httpBackend.when('GET', '/sync').respond(
+                    200, getSyncResponse([]));
+                await anotherTestClient.flushSync();
+
+                const bobStat = anotherTestClient.storage
+                      .getEndToEndDeviceTrackingStatus()['@bob:xyz'];
+
+                expect(bobStat).toEqual(
+                    0, "Alice should have marked bob's device list as untracked",
+                );
+            } finally {
+                anotherTestClient.stop();
+            }
+        });
+    });
 });

src/crypto/DeviceList.js

@@ -26,8 +26,30 @@ import Promise from 'bluebird';
 import DeviceInfo from './deviceinfo';
 import olmlib from './olmlib';
 
+
+/* State transition diagram for DeviceList._deviceTrackingStatus
+ *
+ *                                |
+ *     stopTrackingDeviceList     V
+ *   +---------------------> NOT_TRACKED
+ *   |                            |
+ *   +<--------------------+      | startTrackingDeviceList
+ *   |                     |      V
+ *   |   +-------------> PENDING_DOWNLOAD <--------------------+-+
+ *   |   |                      ^ |                            | |
+ *   |   | restart     download | |  start download            | | invalidateUserDeviceList
+ *   |   | client        failed | |                            | |
+ *   |   |                      | V                            | |
+ *   |   +------------ DOWNLOAD_IN_PROGRESS -------------------+ |
+ *   |                    |       |                              |
+ *   +<-------------------+       |  download successful         |
+ *   ^                            V                              |
+ *   +----------------------- UP_TO_DATE ------------------------+
+ */
+
+
 // constants for DeviceList._deviceTrackingStatus
-// const TRACKING_STATUS_NOT_TRACKED = 0;
+const TRACKING_STATUS_NOT_TRACKED = 0;
 const TRACKING_STATUS_PENDING_DOWNLOAD = 1;
 const TRACKING_STATUS_DOWNLOAD_IN_PROGRESS = 2;
 const TRACKING_STATUS_UP_TO_DATE = 3;
@@ -236,6 +258,26 @@ export default class DeviceList {
         // refreshOutdatedDeviceLists.
     }
 
+    /**
+     * Mark the given user as no longer being tracked for device-list updates.
+     *
+     * This won't affect any in-progress downloads, which will still go on to
+     * complete; it will just mean that we don't think that we have an up-to-date
+     * list for future calls to downloadKeys.
+     *
+     * @param {String} userId
+     */
+    stopTrackingDeviceList(userId) {
+        if (this._deviceTrackingStatus[userId]) {
+            console.log('No longer tracking device list for ' + userId);
+            this._deviceTrackingStatus[userId] = TRACKING_STATUS_NOT_TRACKED;
+        }
+        // we don't yet persist the tracking status, since there may be a lot
+        // of calls; instead we wait for the forthcoming
+        // refreshOutdatedDeviceLists.
+    }
+
+
     /**
      * Mark the cached device list for the given user outdated.
      *
@@ -283,9 +325,6 @@ export default class DeviceList {
                 usersToDownload.push(userId);
             }
         }
-        if (usersToDownload.length == 0) {
-            return;
-        }
 
         // we didn't persist the tracking status during
         // invalidateUserDeviceList, so do it now.

src/crypto/index.js

@@ -775,12 +775,24 @@ Crypto.prototype.decryptEvent = function(event) {
 };
 
 /**
- * Handle the notification from /sync that a user has updated their device list.
+ * Handle the notification from /sync or /keys/changes that device lists have
+ * been changed.
  *
- * @param {String} userId
+ * @param {Object} deviceLists device_lists field from /sync, or response from
+ * /keys/changes
  */
-Crypto.prototype.userDeviceListChanged = function(userId) {
+Crypto.prototype.handleDeviceListChanges = async function(deviceLists) {
-    this._deviceList.invalidateUserDeviceList(userId);
+    if (deviceLists.changed && Array.isArray(deviceLists.changed)) {
+        deviceLists.changed.forEach((u) => {
+            this._deviceList.invalidateUserDeviceList(u);
+        });
+    }
+
+    if (deviceLists.left && Array.isArray(deviceLists.left)) {
+        deviceLists.left.forEach((u) => {
+            this._deviceList.stopTrackingDeviceList(u);
+        });
+    }
 
     // don't flush the outdated device list yet - we do it once we finish
     // processing the sync.
@@ -899,23 +911,19 @@ Crypto.prototype.onSyncCompleted = async function(syncData) {
  * @param {String} oldSyncToken
  * @param {String} lastKnownSyncToken
  *
- * @returns {Promise} resolves once the query is complete. Rejects if the
+ * Returns a Promise which resolves once the query is complete. Rejects if the
  *   keyChange query fails.
  */
-Crypto.prototype._invalidateDeviceListsSince = function(
+Crypto.prototype._invalidateDeviceListsSince = async function(
     oldSyncToken, lastKnownSyncToken,
 ) {
-    return this._baseApis.getKeyChanges(
+    const r = await this._baseApis.getKeyChanges(
         oldSyncToken, lastKnownSyncToken,
-    ).then((r) => {
+    );
-        console.log("got key changes since", oldSyncToken, ":", r.changed);
 
-        if (r.changed && Array.isArray(r.changed)) {
+    console.log("got key changes since", oldSyncToken, ":", r);
-            r.changed.forEach((u) => {
+
-                this._deviceList.invalidateUserDeviceList(u);
+    await this.handleDeviceListChanges(r);
-            });
-        }
-    });
 };
 
 /**

src/sync.js

@@ -1024,10 +1024,14 @@ SyncApi.prototype._processSyncResponse = async function(syncToken, data) {
     }
 
     // Handle device list updates
-    if (this.opts.crypto && data.device_lists && data.device_lists.changed) {
+    if (data.device_lists) {
-        data.device_lists.changed.forEach((u) => {
+        if (this.opts.crypto) {
-            this.opts.crypto.userDeviceListChanged(u);
+            await this.opts.crypto.handleDeviceListChanges(data.device_lists);
-        });
+        } else {
+            // FIXME if we *don't* have a crypto module, we still need to
+            // invalidate the device lists. But that would require a
+            // substantial bit of rework :/.
+        }
     }
 
     // Handle one_time_keys_count