Move Device Tracking Data to Crypto Store #594

dbkr · 2018-01-12T14:06:30Z

Moves the device tracking to the crypto store.

Most of the cross-tab safety here works by writing all the device data atomically, so it's all stored in one big object, much as it feels like an abuse of indexeddb.

This also changes the API a little (in particular there's an explicit save to avoid multiple writes when we mark a room full of devices as known)

* Message sending works again, but * Marking multiple devices known (ie. 'send anyway') is very slow because it writes all device info out each time * Support for non-indexedb stores not written yet * No migration

Don't end up with devices / device tracking status being null

Includes making saveIfDirty() return a promise in case you care about when changes got saved (which the test does).

Allow localstorage store to take a localstorage impl, make TestClient pass a cryptostore & fix True/true typo

Mostly making tests aware of new storage format or making them force it to be written. Also some bugfixes like we didn't json encode some things in the localstorage store and we didn't correctly check the promise when requesting device data saves.

* Check whether we share an e2e room with user IDs in the 'left' field of /keys/changes: there's no guarantee we no longer share any e2e rooms with these users * Reset everyone's tracking status on an initial sync - just re-fetching device lists for all users we're currently tracking isn't good enough since room memberships may have changed. * Fix typo in test

* Change test for new storage layer * Always store device keys we download, even if we weren't tracking the user.

It may change whilst processing the sync

This is more normal, and the code doesn't expect to get empty objects here which is reasonable since it never sets one.

richvdh · 2018-01-18T16:44:50Z

src/crypto/DeviceList.js

+    }
+
+    /**
+     * Gets the current sync token


the current sync token? I'm not sure I even know what it means for a sync token to be current.

dbkr · 2018-01-18T16:55:48Z

spec/unit/crypto/DeviceList.spec.js

@@ -72,7 +92,7 @@ describe('DeviceList', function() {
        queryDefer1.resolve(utils.deepCopy(signedDeviceList));

        return prom1.then(() => {
-            const storedKeys = sessionStore.getEndToEndDevicesForUser('@test1:sw1v.org');
+            const storedKeys = dl.getRawStoredDevicesForUser('@test1:sw1v.org');


We can pull the stored key straight out of the DeviceList now rather than inspecting the backing storage, which I think makes it more of a unit test.

dbkr · 2018-01-18T16:57:46Z

spec/integ/devicelist-integ-spec.js

-               expect(aliceTestClient.storage.getEndToEndDeviceSyncToken()).toEqual(1);
+               aliceTestClient.cryptoStore.getEndToEndDeviceData(null, (data) => {
+                   expect(data.syncToken).toEqual(1);
+               });


Most of the test changes are the same pattern: add the explicit save calls and wait on them, and change the fetch from the store to the new callback based API.

dbkr · 2018-01-18T17:07:52Z

src/crypto/DeviceList.js

+     * @return {Object} userId->deviceId->{object} devices, or null if
+     * there is no data for this user.
+     */
+    getRawStoredDevicesForUser(userId) {


This got added because crypto/index.js was rifling around in the session store itself to get data out, so rather than make it rifle around in indexeddb instead (which wouldn't necessarily be up to date anyway), I've given it a function to get the data it wants from the DeviceList class.

Why not just use getStoredDevice ?

It could do, but getStoredDevice returns a DeviceInfo so we'd have to adapt the code to manipulate the DeviceInfo class and save it back rather than just manipulating the data as stored.

dbkr · 2018-01-18T17:10:00Z

src/crypto/DeviceList.js

@@ -326,10 +471,6 @@ export default class DeviceList {
            }
        }

-        // we didn't persist the tracking status during
-        // invalidateUserDeviceList, so do it now.
-        this._persistDeviceTrackingStatus();


Changed to a saveIfDirty() (above, for no particular reason)

dbkr · 2018-01-18T17:12:31Z

src/crypto/DeviceList.js

            users.forEach((u) => {
+                this._devices[u] = newDevices[u];
+                this._dirty = true;
+


The serialiser used to save the device keys to localstorage as they arrived, but since it store now lives in the DeviceList class, it made more sense to pass all the changes back in the promise.

dbkr · 2018-01-18T17:27:48Z

src/crypto/store/indexeddb-crypto-store-backend.js

+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+


This is all just adding support to the crypto store, fairly obviously.

dbkr · 2018-01-18T17:28:39Z

src/store/session/webstorage.js

     */
-    storeEndToEndDevicesForUser: function(userId, devices) {


We never write to this store anymore :)

dbkr · 2018-01-18T17:28:59Z

src/store/session/webstorage.js

-     * @param {string} userId The user's ID.
-     * @return {object} A map from device ID to keys for the device.
-     */
-    getEndToEndDevicesForUser: function(userId) {


...or need individual devices from it, only ever all of them at once for the migration.

dbkr · 2018-01-18T17:30:36Z

src/sync.js

+    if (this.opts.crypto && !isCachedResponse) {
+        // tell the crypto module we're about to process a sync
+        // response
+        await this.opts.crypto.onSyncWillProcess(syncEventData);


Adds the callback for before the sync is processed - this is where we stop tracking all the devices on an initial sync, so we mark the appropriate ones as tracked again during the processing of the sync.

dbkr · 2018-01-18T17:32:35Z

src/sync.js

-        oldSyncToken: syncToken,
-        nextSyncToken: data.next_batch,
-        catchingUp: this._catchingUp,
-    };


This got declared earlier so we can pass the same data structure into the various different hooks (lots of them need to know if this is an initial sync or not, etc). The catchingUp flag therefore needs to be updated once the sync is actually processed.

richvdh · 2018-01-18T16:52:47Z

src/crypto/DeviceList.js

+        if (this._savePromise === null) {
+            this._savePromise = Promise.delay(500).then(() => {
+                console.log('Saving device tracking data at token ' + this._syncToken);
+                return this._cryptoStore.doTxn(


is there scope for this to fail and leave _savePromise non-null forever?

mmm, maybe. Changed to clear this before we start the write to fix the below, which should fix this too.

richvdh · 2018-01-18T16:56:29Z

src/crypto/DeviceList.js

+                );
+            }).then(() => {
+                this._dirty = false;
+                this._savePromise = null;


this looks racy: we are clearing _savePromise some time after reading the current state, and we might miss updates which happen in that window

Yeah, true. Fixed now I think by clearing the promise before we start the write.

richvdh · 2018-01-18T16:57:30Z

src/crypto/DeviceList.js

+        if (!this._dirty) return Promise.resolve(false);
+
+        if (this._savePromise === null) {
+            this._savePromise = Promise.delay(500).then(() => {


this code path doesn't return a value.

oops, good spot

richvdh · 2018-01-18T16:59:00Z

src/crypto/DeviceList.js

+     * Save the device tracking state to storage, if any changes are
+     * pending other than updating the sync token
+     *
+     * @return {Promise<bool>} true is the data was saved, false if


might be worth noting that it may take 500ms to resolve...?

richvdh · 2018-01-18T17:01:36Z

src/crypto/DeviceList.js

+        this._deviceTrackingStatus = {}; // loaded from storage in load()
+
+        // The 'next_batch' sync token at the point the data was writen,
+        // ie. a token represtenting the point immediately after the


represtenting

richvdh · 2018-01-18T17:45:48Z

src/crypto/DeviceList.js

+     *
+     * @param {string} userId the user to get data for
+     *
+     * @return {Object} userId->deviceId->{object} devices, or null if


surely just deviceId->{object} ?

richvdh · 2018-01-18T17:48:49Z

src/crypto/DeviceList.js

+     * @return {Object} userId->deviceId->{object} devices, or null if
+     * there is no data for this user.
+     */
+    getRawStoredDevicesForUser(userId) {


Why not just use getStoredDevice ?

richvdh · 2018-01-18T17:51:00Z

src/crypto/index.js

-            this._deviceList.stopTrackingDeviceList(u);
-        });
+Crypto.prototype.handleDeviceListChanges = async function(syncData, syncDeviceLists) {
+    // No point processing device list changes for initial syncs: they'd be meaningless


initial syncs have empty change lists iirc

ah, I suspected they probably did. updated comment to reflect

richvdh · 2018-01-18T17:57:17Z

src/crypto/index.js


-    await this.handleDeviceListChanges(r);
+        deviceLists.left.forEach((u) => {
+            if (!e2eUserIds.has(u)) {


this looks O(N^2) ish

The set membership test shouldn't be O(n) though?

er good point

richvdh · 2018-01-18T17:58:56Z

src/crypto/store/memory-crypto-store.js

+    }
+
+    storeEndToEndDeviceData(deviceData, txn) {
+        this._deviceData = deviceData;


wondering if there is any point in this at all, given DeviceList maintains its own copy

Yeah, it is a bit pointless but this is for the unit tests anyway so it basically just has to obey the interface.

richvdh · 2018-01-24T18:17:11Z

src/crypto/DeviceList.js

@@ -154,6 +159,11 @@ export default class DeviceList {
            // in quick succession (eg. when a whole room's devices are marked as known)
            this._savePromise = Promise.delay(500).then(() => {
                console.log('Saving device tracking data at token ' + this._syncToken);
+                // null out savePromise now (after the delay but before the write),
+                // otherwise we could return the existing promise when the save has
+                // actually already happened. Likewsie for the dirty flag.


oops, fixed

richvdh · 2018-01-24T18:36:25Z

src/crypto/DeviceList.js

@@ -510,7 +665,7 @@ class DeviceListUpdateSerialiser {
            console.log('Completed key download for ' + downloadUsers);

            this._downloadInProgress = false;
-            deferred.resolve();
+            deferred.resolve(this._updatedDevices);


ok so the problem with passing the updated device list back to the caller and having the caller actually persist the updates is twofold:

firstly, note that this function calls itself recursively just below here and when that happens, the result is discarded. Any results from queued queries will therefore be lost (I'm surprised that the tests don't pick this up, but shrug)

Secondly I'm concerned that there may be a race wherein the caller hasn't yet updated the device list before _processQueryResponseForUser reads it again and builds a response based on the old value rather than the new one. I'm not actually sure if that's a problem, but we should think about it.

Mmm, I see what you mean. I've made it manipulate the device list by passing the DeviceList object in and using the getter and a new setter, which shouldn't have that problem and doesn't seem too bad.

richvdh · 2018-01-24T19:08:06Z

src/crypto/index.js


-    await this.handleDeviceListChanges(r);
+        deviceLists.left.forEach((u) => {
+            if (!e2eUserIds.has(u)) {


er good point

richvdh

lgtm except for lying comment

richvdh · 2018-01-29T12:26:46Z

src/crypto/DeviceList.js

@@ -446,18 +605,22 @@ class DeviceListUpdateSerialiser {
     *
     * @return {module:client.Promise} resolves when all the users listed have
     *     been updated. rejects if there was a problem updating any of the
-     *     users.
+     *     users. Returns a fresh device list object for the users queried.


oops, fixed

richvdh · 2018-01-29T12:27:40Z

src/crypto/DeviceList.js

-        this._sessionStore.storeEndToEndDevicesForUser(
-            userId, storage,
-        );
+        this._deviceList._setRawStoredDevicesForUser(userId, storage);


I'm not sure this is any more elegant than just manipulating the device list directly, but fine

Probably not, but it seemed prudent to at least do it via setters & getters.

[Full Changelog](matrix-org/matrix-js-sdk@v0.9.2...v0.10.0-rc.1) * Fix duplicated state events in timeline from peek [\matrix-org#630](matrix-org#630) * Create indexeddb worker when starting the store [\matrix-org#627](matrix-org#627) * Fix indexeddb logging [\matrix-org#626](matrix-org#626) * Don't do /keys/changes on incremental sync [\matrix-org#625](matrix-org#625) * Don't mark devicelist dirty unnecessarily [\matrix-org#623](matrix-org#623) * Cache the joined member count for a room state [\matrix-org#619](matrix-org#619) * Fix JS doc [\matrix-org#618](matrix-org#618) * Precompute push actions for state events [\matrix-org#617](matrix-org#617) * Fix bug where global "Never send to unverified..." is ignored [\matrix-org#616](matrix-org#616) * Intern legacy top-level 'membership' field [\matrix-org#615](matrix-org#615) * Don't synthesize RR for m.room.redaction as causes the RR to go missing. [\matrix-org#598](matrix-org#598) * Make Events create Dates on demand [\matrix-org#613](matrix-org#613) * Stop cloning events when adding to state [\matrix-org#612](matrix-org#612) * De-dup code: use the initialiseState function [\matrix-org#611](matrix-org#611) * Create sentinel members on-demand [\matrix-org#610](matrix-org#610) * Some more doc on how sentinels work [\matrix-org#609](matrix-org#609) * Migrate room encryption store to crypto store [\matrix-org#597](matrix-org#597) * add parameter to getIdentityServerUrl to strip the protocol for invites [\matrix-org#600](matrix-org#600) * Move Device Tracking Data to Crypto Store [\matrix-org#594](matrix-org#594) * Optimise pushprocessor [\matrix-org#591](matrix-org#591) * Set event error before emitting [\matrix-org#592](matrix-org#592) * Add event type for stickers [WIP] [\matrix-org#590](matrix-org#590) * Migrate inbound sessions to cryptostore [\matrix-org#587](matrix-org#587) * Disambiguate names if they contain an mxid [\matrix-org#588](matrix-org#588) * Check for sessions in indexeddb before migrating [\matrix-org#585](matrix-org#585) * Emit an event for crypto store migration [\matrix-org#586](matrix-org#586) * Supporting fixes For making UnknownDeviceDialog not pop up automatically [\matrix-org#575](matrix-org#575) * Move sessions to the crypto store [\matrix-org#584](matrix-org#584) * Change crypto store transaction API [\matrix-org#582](matrix-org#582) * Add some missed copyright notices [\matrix-org#581](matrix-org#581) * Move Olm account to IndexedDB [\matrix-org#579](matrix-org#579) * Fix logging of DecryptionErrors to be more useful [\matrix-org#580](matrix-org#580) * [BREAKING] Change the behaviour of the unverfied devices blacklist flag [\matrix-org#568](matrix-org#568) * Support set_presence=offline for syncing [\matrix-org#557](matrix-org#557) * Consider cases where the sender may not redact their own event [\matrix-org#556](matrix-org#556)

dbkr added 16 commits January 11, 2018 18:10

Initial attempt at device tracking -> indexeddb

83e4aa2

* Message sending works again, but * Marking multiple devices known (ie. 'send anyway') is very slow because it writes all device info out each time * Support for non-indexedb stores not written yet * No migration

delay saving so we can batch multiple operations

29d44f8

Add localstorage & memory device data stores

60052f5

Fix lint

2cdb010

Migrate from session store

6c1087e

Fix migration

a94f3c7

Null fix & lint

36d7d33

Don't end up with devices / device tracking status being null

Make DeviceList test pass

110f43a

Includes making saveIfDirty() return a promise in case you care about when changes got saved (which the test does).

More test fixing

27d28b8

Allow localstorage store to take a localstorage impl, make TestClient pass a cryptostore & fix True/true typo

Make tests pass, finally.

0174c56

Mostly making tests aware of new storage format or making them force it to be written. Also some bugfixes like we didn't json encode some things in the localstorage store and we didn't correctly check the promise when requesting device data saves.

lint

5a0997d

more lint

585ea14

More test fixing

461acbc

* Change test for new storage layer * Always store device keys we download, even if we weren't tracking the user.

Update the catchingup flag

fb29da4

It may change whilst processing the sync

Return null if device data isn't set

ea6974f

This is more normal, and the code doesn't expect to get empty objects here which is reasonable since it never sets one.

dbkr changed the title ~~[WIP] Move Device Tracking Data to Crypto Store~~ Move Device Tracking Data to Crypto Store Jan 18, 2018

dbkr assigned richvdh Jan 18, 2018

All the copyrights

8b649ce

richvdh reviewed Jan 18, 2018

View reviewed changes

Add more comments

074cfb7

dbkr commented Jan 18, 2018

View reviewed changes

richvdh requested changes Jan 18, 2018

View reviewed changes

richvdh assigned dbkr and unassigned richvdh Jan 18, 2018

Hopefully address PR feedback

4fbf4f1

dbkr assigned richvdh and unassigned dbkr Jan 22, 2018

dbkr mentioned this pull request Jan 22, 2018

Migrate room encryption store to crypto store #597

Merged

richvdh reviewed Jan 24, 2018

View reviewed changes

richvdh assigned dbkr and unassigned richvdh Jan 24, 2018

dbkr added 2 commits January 24, 2018 21:39

Comment typo

8c2645c

Add new devices as we get them

470bd23

t3chguy assigned richvdh Jan 25, 2018

dbkr removed their assignment Jan 29, 2018

richvdh approved these changes Jan 29, 2018

View reviewed changes

richvdh assigned dbkr and unassigned richvdh Jan 29, 2018

Fux lying comment

c3bede5

dbkr merged commit f1e874c into develop Jan 29, 2018

turt2live mentioned this pull request Jan 29, 2018

Riot stalls when receiving messages element-hq/element-web#6060

Closed

t3chguy deleted the dbkr/device_tracking_indexeddb branch May 10, 2022 14:17

Move Device Tracking Data to Crypto Store #594

Move Device Tracking Data to Crypto Store #594

Conversation

dbkr commented Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbkr Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbkr Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbkr commented Jan 12, 2018 •

edited

Loading

dbkr Jan 22, 2018 •

edited

Loading

dbkr Jan 22, 2018 •

edited

Loading