diff --git a/spec/TestClient.js b/spec/TestClient.js index eb010ab6e..3f4348389 100644 --- a/spec/TestClient.js +++ b/spec/TestClient.js @@ -47,9 +47,10 @@ export default function TestClient( if (sessionStoreBackend === undefined) { sessionStoreBackend = new testUtils.MockStorageApi(); } - this.storage = new sdk.WebStorageSessionStore(sessionStoreBackend); + const sessionStore = new sdk.WebStorageSessionStore(sessionStoreBackend); - const cryptoStore = new LocalStorageCryptoStore(sessionStoreBackend); + // expose this so the tests can get to it + this.cryptoStore = new LocalStorageCryptoStore(sessionStoreBackend); this.httpBackend = new MockHttpBackend(); this.client = sdk.createClient({ @@ -57,8 +58,8 @@ export default function TestClient( userId: userId, accessToken: accessToken, deviceId: deviceId, - sessionStore: this.storage, - cryptoStore: cryptoStore, + sessionStore: sessionStore, + cryptoStore: this.cryptoStore, request: this.httpBackend.requestFn, }); diff --git a/spec/integ/devicelist-integ-spec.js b/spec/integ/devicelist-integ-spec.js index b0fde61b0..ead156fbf 100644 --- a/spec/integ/devicelist-integ-spec.js +++ b/spec/integ/devicelist-integ-spec.js @@ -151,9 +151,12 @@ describe("DeviceList management:", function() { aliceTestClient.httpBackend.flush('/keys/query', 1).then( () => aliceTestClient.httpBackend.flush('/send/', 1), ), + aliceTestClient.client._crypto._deviceList.saveIfDirty(), ]); }).then(() => { - expect(aliceTestClient.storage.getEndToEndDeviceSyncToken()).toEqual(1); + aliceTestClient.cryptoStore.getEndToEndDeviceData(null, (data) => { + expect(data.syncToken).toEqual(1); + }); // invalidate bob's and chris's device lists in separate syncs aliceTestClient.httpBackend.when('GET', '/sync').respond(200, { @@ -185,19 +188,20 @@ describe("DeviceList management:", function() { return aliceTestClient.httpBackend.flush('/keys/query', 1); }).then((flushed) => { expect(flushed).toEqual(0); - const bobStat = aliceTestClient.storage - .getEndToEndDeviceTrackingStatus()['@bob:xyz']; - if (bobStat != 1 && bobStat != 2) { - throw new Error('Unexpected status for bob: wanted 1 or 2, got ' + - bobStat); - } - - const chrisStat = aliceTestClient.storage - .getEndToEndDeviceTrackingStatus()['@chris:abc']; - if (chrisStat != 1 && chrisStat != 2) { - throw new Error('Unexpected status for chris: wanted 1 or 2, got ' + - chrisStat); - } + return aliceTestClient.client._crypto._deviceList.saveIfDirty(); + }).then(() => { + aliceTestClient.cryptoStore.getEndToEndDeviceData(null, (data) => { + const bobStat = data.trackingStatus['@bob:xyz']; + if (bobStat != 1 && bobStat != 2) { + throw new Error('Unexpected status for bob: wanted 1 or 2, got ' + + bobStat); + } + const chrisStat = data.trackingStatus['@chris:abc']; + if (chrisStat != 1 && chrisStat != 2) { + throw new Error('Unexpected status for chris: wanted 1 or 2, got ' + + chrisStat); + } + }); // now add an expectation for a query for bob's devices, and let // it complete. @@ -216,15 +220,17 @@ describe("DeviceList management:", function() { // wait for the client to stop processing the response return aliceTestClient.client.downloadKeys(['@bob:xyz']); }).then(() => { - const bobStat = aliceTestClient.storage - .getEndToEndDeviceTrackingStatus()['@bob:xyz']; - expect(bobStat).toEqual(3); - const chrisStat = aliceTestClient.storage - .getEndToEndDeviceTrackingStatus()['@chris:abc']; - if (chrisStat != 1 && chrisStat != 2) { - throw new Error('Unexpected status for chris: wanted 1 or 2, got ' + - bobStat); - } + return aliceTestClient.client._crypto._deviceList.saveIfDirty(); + }).then(() => { + aliceTestClient.cryptoStore.getEndToEndDeviceData(null, (data) => { + const bobStat = data.trackingStatus['@bob:xyz']; + expect(bobStat).toEqual(3); + const chrisStat = data.trackingStatus['@chris:abc']; + if (chrisStat != 1 && chrisStat != 2) { + throw new Error('Unexpected status for chris: wanted 1 or 2, got ' + + bobStat); + } + }); // now let the query for chris's devices complete. return aliceTestClient.httpBackend.flush('/keys/query', 1); @@ -234,16 +240,18 @@ describe("DeviceList management:", function() { // wait for the client to stop processing the response return aliceTestClient.client.downloadKeys(['@chris:abc']); }).then(() => { - const bobStat = aliceTestClient.storage - .getEndToEndDeviceTrackingStatus()['@bob:xyz']; - const chrisStat = aliceTestClient.storage - .getEndToEndDeviceTrackingStatus()['@chris:abc']; + return aliceTestClient.client._crypto._deviceList.saveIfDirty(); + }).then(() => { + aliceTestClient.cryptoStore.getEndToEndDeviceData(null, (data) => { + const bobStat = data.trackingStatus['@bob:xyz']; + const chrisStat = data.trackingStatus['@bob:xyz']; - expect(bobStat).toEqual(3); - expect(chrisStat).toEqual(3); - expect(aliceTestClient.storage.getEndToEndDeviceSyncToken()).toEqual(3); + expect(bobStat).toEqual(3); + expect(chrisStat).toEqual(3); + expect(data.syncToken).toEqual(3); + }); }); - }); + }).timeout(3000); // https://github.com/vector-im/riot-web/issues/4983 describe("Alice should know she has stale device lists", () => { @@ -262,13 +270,16 @@ describe("DeviceList management:", function() { }, ); await aliceTestClient.httpBackend.flush('/keys/query', 1); + await aliceTestClient.client._crypto._deviceList.saveIfDirty(); - const bobStat = aliceTestClient.storage - .getEndToEndDeviceTrackingStatus()['@bob:xyz']; + aliceTestClient.cryptoStore.getEndToEndDeviceData(null, (data) => { + const bobStat = data.trackingStatus['@bob:xyz']; + + expect(bobStat).toBeGreaterThan( + 0, "Alice should be tracking bob's device list", + ); + }); - expect(bobStat).toBeGreaterThan( - 0, "Alice should be tracking bob's device list", - ); }); it("when Bob leaves", async function() { @@ -297,12 +308,15 @@ describe("DeviceList management:", function() { await aliceTestClient.flushSync(); + await aliceTestClient.client._crypto._deviceList.saveIfDirty(); - const bobStat = aliceTestClient.storage - .getEndToEndDeviceTrackingStatus()['@bob:xyz']; - expect(bobStat).toEqual( - 0, "Alice should have marked bob's device list as untracked", - ); + aliceTestClient.cryptoStore.getEndToEndDeviceData(null, (data) => { + const bobStat = data.trackingStatus['@bob:xyz']; + + expect(bobStat).toEqual( + 0, "Alice should have marked bob's device list as untracked", + ); + }); }); it("when Alice leaves", async function() { @@ -330,12 +344,15 @@ describe("DeviceList management:", function() { ); await aliceTestClient.flushSync(); + await aliceTestClient.client._crypto._deviceList.saveIfDirty(); - const bobStat = aliceTestClient.storage - .getEndToEndDeviceTrackingStatus()['@bob:xyz']; - expect(bobStat).toEqual( - 0, "Alice should have marked bob's device list as untracked", - ); + aliceTestClient.cryptoStore.getEndToEndDeviceData(null, (data) => { + const bobStat = data.trackingStatus['@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() { @@ -344,23 +361,19 @@ describe("DeviceList management:", function() { 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(); + await aliceTestClient.client._crypto._deviceList.saveIfDirty(); - const bobStat = anotherTestClient.storage - .getEndToEndDeviceTrackingStatus()['@bob:xyz']; + aliceTestClient.cryptoStore.getEndToEndDeviceData(null, (data) => { + const bobStat = data.trackingStatus['@bob:xyz']; - expect(bobStat).toEqual( - 0, "Alice should have marked bob's device list as untracked", - ); + 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 ecb56d8cb..61661f3a5 100644 --- a/src/crypto/DeviceList.js +++ b/src/crypto/DeviceList.js @@ -89,8 +89,6 @@ export default class DeviceList { // Set whenever changes are made other than setting the sync token this._dirty = false; - // Timeout for a scheduled save - this._saveTimer = null; // Promise resolved when device data is saved this._savePromise = null; } @@ -150,11 +148,10 @@ export default class DeviceList { async saveIfDirty() { if (!this._dirty) return Promise.resolve(false); - if (this._saveTimer === null) { - this._saveTimer = setTimeout(() => { - console.log('Saving device tracking data...'); - this._saveTimer = null; - this._savePromise = this._cryptoStore.doTxn( + if (this._savePromise === null) { + this._savePromise = Promise.delay(500).then(() => { + console.log('Saving device tracking data at token ' + this._syncToken); + return this._cryptoStore.doTxn( 'readwrite', [IndexedDBCryptoStore.STORE_DEVICE_DATA], (txn) => { this._cryptoStore.storeEndToEndDeviceData({ devices: this._devices, @@ -162,11 +159,12 @@ export default class DeviceList { syncToken: this._syncToken, }, txn); }, - ).then(() => { - this._dirty = false; - return true; - }); - }, 500); + ); + }).then(() => { + this._dirty = false; + this._savePromise = null; + return true; + }); } else { return this._savePromise; } @@ -571,6 +569,7 @@ class DeviceListUpdateSerialiser { this._devices = null; // the complete device list this._updatedDevices = null; // device list updates we've fetched + this._syncToken = null; // The sync token we send with the requests } /** @@ -596,6 +595,11 @@ class DeviceListUpdateSerialiser { this._queuedQueryDeferred = Promise.defer(); } + // We always take the new sync token and just use the latest one we've + // been given, since it just needs to be at least as recent as the + // sync response the device invalidation message arrived in + this._syncToken = syncToken; + if (this._downloadInProgress) { // just queue up these users console.log('Queued key download for', users); @@ -605,10 +609,10 @@ class DeviceListUpdateSerialiser { this._devices = devices; this._updatedDevices = {}; // start a new download. - return this._doQueuedQueries(syncToken); + return this._doQueuedQueries(); } - _doQueuedQueries(syncToken) { + _doQueuedQueries() { if (this._downloadInProgress) { throw new Error( "DeviceListUpdateSerialiser._doQueuedQueries called with request active", @@ -624,8 +628,8 @@ class DeviceListUpdateSerialiser { this._downloadInProgress = true; const opts = {}; - if (syncToken) { - opts.token = syncToken; + if (this._syncToken) { + opts.token = this._syncToken; } this._baseApis.downloadKeysForUsers( @@ -654,7 +658,7 @@ class DeviceListUpdateSerialiser { // if we have queued users, fire off another request. if (this._queuedQueryDeferred) { - this._doQueuedQueries(syncToken); + this._doQueuedQueries(); } }, (e) => { console.warn('Error downloading keys for ' + downloadUsers + ':', e); diff --git a/src/crypto/store/localStorage-crypto-store.js b/src/crypto/store/localStorage-crypto-store.js index 9ec8394bf..a2e5d91f0 100644 --- a/src/crypto/store/localStorage-crypto-store.js +++ b/src/crypto/store/localStorage-crypto-store.js @@ -135,7 +135,9 @@ export default class LocalStorageCryptoStore extends MemoryCryptoStore { } storeEndToEndDeviceData(deviceData, txn) { - this.store.setItem(KEY_DEVICE_DATA, deviceData); + setJsonItem( + this.store, KEY_DEVICE_DATA, deviceData, + ); } /** @@ -151,12 +153,14 @@ export default class LocalStorageCryptoStore extends MemoryCryptoStore { // Olm account getAccount(txn, func) { - const account = this.store.getItem(KEY_END_TO_END_ACCOUNT); + const account = getJsonItem(this.store, KEY_END_TO_END_ACCOUNT); func(account); } storeAccount(txn, newData) { - this.store.setItem(KEY_END_TO_END_ACCOUNT, newData); + setJsonItem( + this.store, KEY_END_TO_END_ACCOUNT, newData, + ); } doTxn(mode, stores, func) {