Move sessions to the crypto store #584

dbkr · 2017-12-04T14:34:03Z

Migrates the olm sessions to the crypto store. On startup, looks for any sessions still in the session store and moves them over to the crypto store.

Also moves the account migration into the same function to keep all the migration code together.

This doesn't migrate existing ones yet

...into a separate function

richvdh · 2017-12-04T19:01:56Z

src/crypto/OlmDevice.js

@@ -219,38 +250,61 @@ OlmDevice.prototype._storeAccount = function(txn, account) {

 /**
 * extract an OlmSession from the session store and call the given function
+ * The session is useable only within the callback passed to this


or not. my bad.

i discovered yesterday that i've been saying "unless if" my whole life rather than just "unless". very sad.

richvdh

Generally looking pretty good, modulo a few niggles.

Can we do something about the exception handling though?

richvdh · 2017-12-04T20:02:25Z

src/crypto/OlmDevice.js

 *
 * @param {string} deviceKey
 * @param {string} sessionId
+ * @param {*} txn


can haz doc plz?

richvdh · 2017-12-04T20:30:10Z

src/crypto/OlmDevice.js

+        (txn) => {
+            this._cryptoStore.getEndToEndSessions(deviceIdentityKey, txn, (sessions) => {
+                const sessionIds = Object.keys(sessions).sort();
+                for (let i = 0; i < sessionIds.length; i++) {


for (const sessionId of sessionIds) ?

yeah, probably left from the old code I guess

richvdh · 2017-12-04T20:30:35Z

src/crypto/OlmDevice.js

+OlmDevice.prototype._getSession = function(deviceKey, sessionId, txn, func) {
+    this._cryptoStore.getEndToEndSession(
+        deviceKey, sessionId, txn, (pickledSession) => {
+            const session = new Olm.Session();


can we use _unpickleSession here?

richvdh · 2017-12-04T20:32:20Z

src/crypto/OlmDevice.js

+                try {
+                    payloadString = session.decrypt(messageType, ciphertext);
+                } catch (e) {
+                    exception = e;


It's debatable whether we should save the session here.

I really feel like this is pretty grim. Is there no way to have this propagate properly? Why have we picked out decryptMessage to be worthy of exception handling?

The thing is that we'll be in an indxexeddb callback here, so we can't just let the exception propagate as it'll propagate up into the VM. But yes, we should probably be doing this everywhere else (it was just done here because this was throwing on bad message macs etc). I wish there were a prettier way.

richvdh · 2017-12-04T20:35:10Z

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

@@ -31,6 +31,7 @@ export default class MemoryCryptoStore {
    constructor() {
        this._outgoingRoomKeyRequests = [];
        this._account = null;
+        this._sessions = {};


could you document what this holds? I think it's a map from remote device key to session id to session?

richvdh · 2017-12-04T20:37:18Z

src/crypto/store/localStorage-crypto-store.js

+        // JSON.parse(null) === null, so this returns null.
+        return JSON.parse(store.getItem(key));
+    } catch (e) {
+        console.log("Failed to get key %s: %s", key, e);


I think we've ended up with

console.log("Error: out of wotsits: %s", e.stack || e)

elsewhere

ah, this was just copied from the session store. fixed.

richvdh · 2017-12-04T22:06:47Z

src/store/session/webstorage.js

+        return results;
+    },
+
+    removeAllEndToEndSessions: function() {


richvdh

lgtm

[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)

The code to migrate from the `sessionStore` to `cryptoStore` originally appeared in matrix-org#584 (2017-12-06). At this point, it seems safe to assume most sessions that need migrating have already done so. Removing this code simplifies store handling and removes the `sessionStore` from most places in JS SDK.

dbkr added 8 commits December 4, 2017 14:33

Move sessions to the crypto store

aa70da5

This doesn't migrate existing ones yet

lint

d995019

Implement session storage in memory & localstorage

5372575

Move sessionstore -> cryptostore migration

55bbc71

...into a separate function

Migrate sessions from sessionStore

4796721

Remove prefix from sessions store keys

6782d53

Catch & rethrow decryption exceptions

8bfa81d

Lint

859d462

dbkr assigned richvdh Dec 4, 2017

Remove unused function

5e4149a

richvdh reviewed Dec 4, 2017

View reviewed changes

richvdh assigned dbkr and unassigned richvdh Dec 5, 2017

dbkr changed the title ~~[WIP] Move sessions to the crypto store~~ Move sessions to the crypto store Dec 5, 2017

dbkr added 6 commits December 5, 2017 10:04

doc

fd083e1

Use _unpickleSession in _getSession

27030ae

More sensible loop & logging

ae27c55

More doc

d3bc525

Maybe slightly better exception handling

bd08ed8

Doc exception handling fun

fb37150

dbkr assigned richvdh and unassigned dbkr Dec 5, 2017

s/sessionStore/cryptoStore/

9d7d48b

richvdh approved these changes Dec 5, 2017

View reviewed changes

richvdh assigned dbkr and unassigned richvdh Dec 5, 2017

dbkr merged commit 61df41d into develop Dec 6, 2017

jryans mentioned this pull request Mar 20, 2019

Remove sessionStore to cryptoStore migration path #865

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move sessions to the crypto store #584

Move sessions to the crypto store #584

dbkr commented Dec 4, 2017 •

edited

richvdh Dec 4, 2017

richvdh Dec 4, 2017

ara4n Dec 4, 2017

richvdh left a comment

richvdh Dec 4, 2017

dbkr Dec 5, 2017

richvdh Dec 4, 2017

dbkr Dec 5, 2017

richvdh Dec 4, 2017

dbkr Dec 5, 2017

richvdh Dec 4, 2017

dbkr Dec 5, 2017 •

edited

richvdh Dec 4, 2017

dbkr Dec 5, 2017

richvdh Dec 4, 2017

dbkr Dec 5, 2017

richvdh Dec 4, 2017

dbkr Dec 5, 2017

richvdh left a comment

Move sessions to the crypto store #584

Move sessions to the crypto store #584

Conversation

dbkr commented Dec 4, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbkr Dec 5, 2017 • edited

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

dbkr commented Dec 4, 2017 •

edited

dbkr Dec 5, 2017 •

edited