From 1a55f550c04e470fd490685a54bba6ef96cb390b Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 8 Sep 2017 15:35:47 +0100 Subject: [PATCH] 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 | 12 ++- spec/integ/devicelist-integ-spec.js | 142 +++++++++++++++++++++++++++- src/crypto/DeviceList.js | 47 ++++++++- src/crypto/index.js | 38 +++++--- src/sync.js | 12 ++- 5 files changed, 221 insertions(+), 30 deletions(-) diff --git a/spec/TestClient.js b/spec/TestClient.js index 409e065da..6e6038a73 100644 --- a/spec/TestClient.js +++ b/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", diff --git a/spec/integ/devicelist-integ-spec.js b/spec/integ/devicelist-integ-spec.js index 441af879d..b0fde61b0 100644 --- a/spec/integ/devicelist-integ-spec.js +++ b/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( - "@alice:localhost", "xzcvb", "akjgkrgjs", - ); - await aliceTestClient.client.initCrypto(); + // we create our own sessionStoreBackend so that we can use it for + // another TestClient. + sessionStoreBackend = new testUtils.MockStorageApi(); + + 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(); + } + }); + }); }); diff --git a/src/crypto/DeviceList.js b/src/crypto/DeviceList.js index fc938c33f..1308c44ad 100644 --- a/src/crypto/DeviceList.js +++ b/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. diff --git a/src/crypto/index.js b/src/crypto/index.js index 026c0cc9a..57f99ac78 100644 --- a/src/crypto/index.js +++ b/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) { - this._deviceList.invalidateUserDeviceList(userId); +Crypto.prototype.handleDeviceListChanges = async function(deviceLists) { + 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)) { - r.changed.forEach((u) => { - this._deviceList.invalidateUserDeviceList(u); - }); - } - }); + console.log("got key changes since", oldSyncToken, ":", r); + + await this.handleDeviceListChanges(r); }; /** diff --git a/src/sync.js b/src/sync.js index cc2ce5bac..1adfa298e 100644 --- a/src/sync.js +++ b/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) { - data.device_lists.changed.forEach((u) => { - this.opts.crypto.userDeviceListChanged(u); - }); + if (data.device_lists) { + if (this.opts.crypto) { + 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