From 655be2fa2e858bfb1a25a23fb7da2efa9bead892 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 5 May 2017 12:34:00 +0100 Subject: [PATCH] Fix race in device list updates Don't consider device lists up-to-date when we have another request for the relevant user in the queue. Fixes https://github.com/vector-im/riot-web/issues/3796. --- spec/unit/crypto/DeviceList.spec.js | 128 ++++++++++++++++++++++++++++ src/crypto/DeviceList.js | 13 ++- 2 files changed, 140 insertions(+), 1 deletion(-) create mode 100644 spec/unit/crypto/DeviceList.spec.js diff --git a/spec/unit/crypto/DeviceList.spec.js b/spec/unit/crypto/DeviceList.spec.js new file mode 100644 index 00000000000..b2129c2e08c --- /dev/null +++ b/spec/unit/crypto/DeviceList.spec.js @@ -0,0 +1,128 @@ +import DeviceList from '../../../lib/crypto/DeviceList'; +import MockStorageApi from '../../MockStorageApi'; +import WebStorageSessionStore from '../../../lib/store/session/webstorage'; +import testUtils from '../../test-utils'; +import utils from '../../../lib/utils'; + +import expect from 'expect'; +import q from 'q'; + +const signedDeviceList = { + "failures": {}, + "device_keys": { + "@test1:sw1v.org": { + "HGKAWHRVJQ": { + "signatures": { + "@test1:sw1v.org": { + "ed25519:HGKAWHRVJQ": + "8PB450fxKDn5s8IiRZ2N2t6MiueQYVRLHFEzqIi1eLdxx1w" + + "XEPC1/1Uz9T4gwnKlMVAKkhB5hXQA/3kjaeLABw", + }, + }, + "user_id": "@test1:sw1v.org", + "keys": { + "ed25519:HGKAWHRVJQ": + "0gI/T6C+mn1pjtvnnW2yB2l1IIBb/5ULlBXi/LXFSEQ", + "curve25519:HGKAWHRVJQ": + "mbIZED1dBsgIgkgzxDpxKkJmsr4hiWlGzQTvUnQe3RY", + }, + "algorithms": [ + "m.olm.v1.curve25519-aes-sha2", + "m.megolm.v1.aes-sha2", + ], + "device_id": "HGKAWHRVJQ", + "unsigned": {}, + }, + }, + }, +}; + +describe('DeviceList', function() { + let downloadSpy; + let sessionStore; + + beforeEach(function() { + testUtils.beforeEach(this); // eslint-disable-line no-invalid-this + + downloadSpy = expect.createSpy(); + const mockStorage = new MockStorageApi(); + sessionStore = new WebStorageSessionStore(mockStorage); + }); + + function createTestDeviceList() { + const baseApis = { + downloadKeysForUsers: downloadSpy, + }; + const mockOlm = { + verifySignature: function(key, message, signature) {}, + }; + return new DeviceList(baseApis, sessionStore, mockOlm); + } + + it("should successfully download and store device keys", function() { + const dl = createTestDeviceList(); + + dl.startTrackingDeviceList('@test1:sw1v.org'); + + const queryDefer1 = q.defer(); + downloadSpy.andReturn(queryDefer1.promise); + + const prom1 = dl.refreshOutdatedDeviceLists(); + expect(downloadSpy).toHaveBeenCalledWith(['@test1:sw1v.org'], {}); + queryDefer1.resolve(utils.deepCopy(signedDeviceList)); + + return prom1.then(() => { + const storedKeys = sessionStore.getEndToEndDevicesForUser('@test1:sw1v.org'); + expect(Object.keys(storedKeys)).toEqual(['HGKAWHRVJQ']); + }); + }); + + it("should have an outdated devicelist on an invalidation while an " + + "update is in progress", function() { + const dl = createTestDeviceList(); + + dl.startTrackingDeviceList('@test1:sw1v.org'); + + const queryDefer1 = q.defer(); + downloadSpy.andReturn(queryDefer1.promise); + + const prom1 = dl.refreshOutdatedDeviceLists(); + expect(downloadSpy).toHaveBeenCalledWith(['@test1:sw1v.org'], {}); + downloadSpy.reset(); + + // outdated notif arrives while the request is in flight. + const queryDefer2 = q.defer(); + downloadSpy.andReturn(queryDefer2.promise); + + dl.invalidateUserDeviceList('@test1:sw1v.org'); + dl.refreshOutdatedDeviceLists(); + + // the first request completes + queryDefer1.resolve({ + device_keys: { + '@test1:sw1v.org': {}, + }, + }); + + return prom1.then(() => { + // uh-oh; user restarts before second request completes. The new instance + // should know we never got a complete device list. + console.log("Creating new devicelist to simulate app reload"); + downloadSpy.reset(); + const dl2 = createTestDeviceList(); + const queryDefer3 = q.defer(); + downloadSpy.andReturn(queryDefer3.promise); + + const prom3 = dl2.refreshOutdatedDeviceLists(); + expect(downloadSpy).toHaveBeenCalledWith(['@test1:sw1v.org'], {}); + + queryDefer3.resolve(utils.deepCopy(signedDeviceList)); + + // allow promise chain to complete + return prom3; + }).then(() => { + const storedKeys = sessionStore.getEndToEndDevicesForUser('@test1:sw1v.org'); + expect(Object.keys(storedKeys)).toEqual(['HGKAWHRVJQ']); + }); + }); +}); diff --git a/src/crypto/DeviceList.js b/src/crypto/DeviceList.js index c1f8787f59b..e3d55e31849 100644 --- a/src/crypto/DeviceList.js +++ b/src/crypto/DeviceList.js @@ -263,6 +263,9 @@ export default class DeviceList { /** * If we have users who have outdated device lists, start key downloads for them + * + * @returns {Promise} which completes when the download completes; normally there + * is no need to wait for this (it's mostly for the unit tests). */ refreshOutdatedDeviceLists() { const usersToDownload = []; @@ -280,7 +283,7 @@ export default class DeviceList { // invalidateUserDeviceList, so do it now. this._persistDeviceTrackingStatus(); - this._doKeyDownload(usersToDownload); + return this._doKeyDownload(usersToDownload); } @@ -323,6 +326,14 @@ export default class DeviceList { const finished = (success) => { users.forEach((u) => { + // we may have queued up another download request for this user + // since we started this request. If that happens, we should + // ignore the completion of the first one. + if (this._keyDownloadsInProgressByUser[u] !== prom) { + console.log('Another update in the queue for', u, + '- not marking up-to-date'); + return; + } delete this._keyDownloadsInProgressByUser[u]; const stat = this._deviceTrackingStatus[u]; if (stat == TRACKING_STATUS_DOWNLOAD_IN_PROGRESS) {