IndexedDBCryptoStore.getAllEndToEndSessions does not pass deviceKey and sessionId to callback

[cedricvanrompay]

Here is IndexedDBCryptoStore.getAllEndToEndSessions:

    getAllEndToEndSessions(txn, func) {
        const objectStore = txn.objectStore("sessions");
        const getReq = objectStore.openCursor();
        getReq.onsuccess = function() {
            const cursor = getReq.result;
            if (cursor) {
                func(cursor.value);
                cursor.continue();
            } else {
                try {
                    func(null);
                } catch (e) {
                    abortWithException(txn, e);
                }
            }
        };
    }

(source)

It seems that cursor.value does not include the fields that are used for the key path

This is both not practical and in contradiction with the documentation of IndexedDBCryptoStore.getAllEndToEndSessions

I can try to find a way to include the missing attributes. I was thinking about fixing this in the pull request I am working on that is blocked by the problem.

EDIT: Actually it is possible that the problem is only for the in-memory DB and that cursor.value returns what we expect. See https://github.com/matrix-org/matrix-js-sdk/issues/1152#issuecomment-575610259

[cedricvanrompay]

The following patch solves the problem for the in-memory crypto store:

diff --git a/src/crypto/store/memory-crypto-store.js b/src/crypto/store/memory-crypto-store.js
index 952af669..4012210a 100644
--- a/src/crypto/store/memory-crypto-store.js
+++ b/src/crypto/store/memory-crypto-store.js
@@ -259,11 +259,15 @@ export default class MemoryCryptoStore {
     }

     getAllEndToEndSessions(txn, func) {
-        for (const deviceSessions of Object.values(this._sessions)) {
-            for (const sess of Object.values(deviceSessions)) {
-                func(sess);
-            }
-        }
+        Object.entries(this._sessions).forEach(([deviceKey, deviceSessions]) => {
+            Object.entries(deviceSessions).forEach(([sessionId, session]) => {
+                func({
+                    ...session,
+                    deviceKey,
+                    sessionId,
+                });
+            });
+        });
     }

     storeEndToEndSession(deviceKey, sessionId, sessionInfo, txn) {

Here is an example of value received by the callback after this patch:

{
   session: '/ZBE/vr5zkTOTeAVLR6lME6GYlH8ICM8HUf+OsIk9Vr8ld7Edn0T539P9B5G6zUubFuyQmFZa6a50BIGaP/Kj50UYHUBsmtLEbJCKGkH6qPqluGW9W0aXBXN5SN3mTli3tum6V2lxp92SzP11KwWmQbzNVJBlSDZJlKc40Dxk8u9ROE0aig0tZr5vqV/7ngbE7Lg3O7UZfGBz5Rizl/bSXfWBLXhd6dw8P3IDfqTlfEPv3nO4uCKzoSB/jl0uQVScN7FDWY0xyrzb6ysZtG6HD/ZxRXVupo+VyHb43pm3HvjESGAOvRuDvOXzL2qMj0+wpWzKTs7Q+Pxqt78wtNuyJiRMtqrFjYe',
   lastReceivedMessageTs: 1579102833205,
   deviceKey: 'asbnVX5D5xE7Ez3EAvheJwW15rAdF83Qc5Y7tgdT+1E',
   sessionId: 'EenPfpAeniEOD5ju7vCjsAs/TbpfLkOfV1fscWiy9bg'
}

Now we have to find out how to do the same with the IndexedDB store.

[cedricvanrompay]

It seems that what we are looking for is cursor.primaryKey:

https://stackoverflow.com/a/47931995/3025740

[cedricvanrompay]

Problem: I don't know how to make tests with Jest use the indexed DB instead of the in-memory DB.

[cedricvanrompay]

Actually it's possible that cursor.value does returns all the fields:

var request = indexedDB.open("testDB");

request.onupgradeneeded = function() {
  // The database did not previously exist, so create object stores and indexes.
  var db = request.result;
  var store = db.createObjectStore("testObjectStore", {keyPath: ['keyPathPart1', 'keyPathPart2']});

  // Populate with initial data.
  store.put({keyPathPart1: 'foo', keyPathPart2: 'bar', value:'lol'});
  store.put({keyPathPart1: 'lorem', keyPathPart2: 'ipsum', value:'lipsum'});
};

request.onsuccess = function() {
  db = request.result;
  txn = db.transaction('testObjectStore', 'readonly');
  objectStore = txn.objectStore('testObjectStore');
  const getReq = objectStore.openCursor();
  getReq.onsuccess = function() {
      const cursor = getReq.result;
      if (cursor) {
          console.log('CURSOR VALUE', cursor.value);
          console.log('CURSOR PRIMARY KEY', cursor.primaryKey);
          cursor.continue();
      } else {
          try {
              console.log('END');
          } catch (e) {
              abortWithException(txn, e);
          }
      }
  };
};

CURSOR VALUE Object { keyPathPart1: "foo", keyPathPart2: "bar", value: "lol" }
CURSOR PRIMARY KEY Array [ "foo", "bar" ]
CURSOR VALUE Object { keyPathPart1: "lorem", keyPathPart2: "ipsum", value: "lipsum" }
CURSOR PRIMARY KEY Array [ "lorem", "ipsum" ]
END

Now I realize that when I observed missing fields and created the issue I was actually using the in-memory database (with Jest).

So it's possible that the indexed DB behaves correctly but the in-memory DB did not in which case the patch I give above should suffice to solve the issue.

Still, it would be nice to confirm this by running the test suite with the indexed DB. I was advised to use https://www.npmjs.com/package/fake-indexeddb, I will have a look.

[cedricvanrompay]

Unless you people think that simply applying the above patch on the in-memory database code should be enough and that there is no need making the test job more complex by testing both in-memory DB as well as indexed DB (or that it should be done in a separate contribution later)?

[cedricvanrompay]

First try at using package fake-indexeddb: no luck :frowning_face:

Here is my diff (omitting `yarn.lock)

diff --git a/package.json b/package.json
index 74ca0da6..cc868235 100644
--- a/package.json
+++ b/package.json
@@ -79,6 +79,7 @@
     "eslint-plugin-babel": "^5.3.0",
     "eslint-plugin-jest": "^23.0.4",
     "exorcist": "^1.0.1",
+    "fake-indexeddb": "^3.0.0",
     "jest": "^24.9.0",
     "jsdoc": "^3.5.5",
     "matrix-mock-request": "^1.2.3",
diff --git a/spec/unit/crypto/algorithms/olm.spec.js b/spec/unit/crypto/algorithms/olm.spec.js
index 4d5b8307..aafc6159 100644
--- a/spec/unit/crypto/algorithms/olm.spec.js
+++ b/spec/unit/crypto/algorithms/olm.spec.js
@@ -14,9 +14,11 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

+import "fake-indexeddb/auto";
+
 import '../../../olm-loader';

-import MemoryCryptoStore from '../../../../lib/crypto/store/memory-crypto-store.js';
+import IndexedDBCryptoStore from '../../../../lib/crypto/store/indexeddb-crypto-store';
 import MockStorageApi from '../../../MockStorageApi';
 import logger from '../../../../lib/logger';

@@ -26,7 +28,7 @@ import DeviceInfo from '../../../../lib/crypto/deviceinfo';

 function makeOlmDevice() {
     const mockStorage = new MockStorageApi();
-    const cryptoStore = new MemoryCryptoStore(mockStorage);
+    const cryptoStore = new IndexedDBCryptoStore(mockStorage);
     const olmDevice = new OlmDevice(cryptoStore);
     return olmDevice;
 }

(inspired by this)

Tests fail with these kind of warnings in the logs:

console.warn lib/crypto/store/indexeddb-crypto-store.js:125
unable to connect to indexeddb undefined: falling back to localStorage store: TypeError: _this._indexedDB.open is not a function

I may need some help.

[ara4n]

@turt2live can you reveal how this is meant to work?