From fb991503a977ca4d591fd83d9e9af94d93a89898 Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 21 Nov 2017 18:27:40 +0000 Subject: [PATCH 01/16] Move OLM account to IndexedDBd Wraps all access to the account in a transaction so any updates done to the account must be done in the same transaction, making the update atomic between tabs. Doesn't do any migration from localstorage yet. --- src/crypto/OlmDevice.js | 69 ++++++++++++------- src/crypto/index.js | 2 +- .../store/indexeddb-crypto-store-backend.js | 41 ++++++++++- src/crypto/store/indexeddb-crypto-store.js | 16 +++++ src/crypto/store/memory-crypto-store.js | 20 ++++++ 5 files changed, 120 insertions(+), 28 deletions(-) diff --git a/src/crypto/OlmDevice.js b/src/crypto/OlmDevice.js index 3b4d1f2ae27..1601c2ee415 100644 --- a/src/crypto/OlmDevice.js +++ b/src/crypto/OlmDevice.js @@ -79,8 +79,9 @@ function checkPayloadLength(payloadString) { * @property {string} deviceCurve25519Key Curve25519 key for the account * @property {string} deviceEd25519Key Ed25519 key for the account */ -function OlmDevice(sessionStore) { +function OlmDevice(sessionStore, cryptoStore) { this._sessionStore = sessionStore; + this._cryptoStore = cryptoStore; this._pickleKey = "DEFAULT_KEY"; // don't know these until we load the account from storage in init() @@ -124,7 +125,7 @@ OlmDevice.prototype.init = async function() { let e2eKeys; const account = new Olm.Account(); try { - _initialise_account(this._sessionStore, this._pickleKey, account); + await _initialise_account(this._cryptoStore, this._pickleKey, account); e2eKeys = JSON.parse(account.identity_keys()); this._maxOneTimeKeys = account.max_number_of_one_time_keys(); @@ -137,16 +138,16 @@ OlmDevice.prototype.init = async function() { }; -function _initialise_account(sessionStore, pickleKey, account) { - const e2eAccount = sessionStore.getEndToEndAccount(); - if (e2eAccount !== null) { - account.unpickle(pickleKey, e2eAccount); +async function _initialise_account(cryptoStore, pickleKey, account) { + const accountTxn = await cryptoStore.endToEndAccountTransaction(); + if (accountTxn.account !== null) { + account.unpickle(pickleKey, accountTxn.account); return; } account.create(); const pickled = account.pickle(pickleKey); - sessionStore.storeEndToEndAccount(pickled); + await accountTxn.save(pickled); } /** @@ -158,21 +159,36 @@ OlmDevice.getOlmVersion = function() { /** - * extract our OlmAccount from the session store and call the given function + * extract our OlmAccount from the crypto store and call the given function + * with the account object and a 'save' function which returns a promise. + * The function will not be awaited upon and the save function must be + * called before the function returns, or not at all. * * @param {function} func * @return {object} result of func * @private */ -OlmDevice.prototype._getAccount = function(func) { - const account = new Olm.Account(); +OlmDevice.prototype._getAccount = async function(func) { + let result; + + let account = null; try { - const pickledAccount = this._sessionStore.getEndToEndAccount(); - account.unpickle(this._pickleKey, pickledAccount); - return func(account); + const accountTxn = await this._cryptoStore.endToEndAccountTransaction(); + // Olm has a limited stack size so we must tightly control the number of + // Olm account objects in existence at any given time: once created, it + // must be destroyed again before we await. + account = new Olm.Account(); + account.unpickle(this._pickleKey, accountTxn.account); + + result = func(account, () => { + const pickledAccount = account.pickle(this._pickleKey); + return accountTxn.save(pickledAccount); + } + ); } finally { - account.free(); + if (account !== null) account.free(); } + return result; }; @@ -182,9 +198,9 @@ OlmDevice.prototype._getAccount = function(func) { * @param {OlmAccount} account * @private */ -OlmDevice.prototype._saveAccount = function(account) { +OlmDevice.prototype._saveAccount = async function(account) { const pickledAccount = account.pickle(this._pickleKey); - this._sessionStore.storeEndToEndAccount(pickledAccount); + await this._cryptoStore.storeEndToEndAccount(pickledAccount); }; @@ -250,7 +266,7 @@ OlmDevice.prototype._getUtility = function(func) { * @return {Promise} base64-encoded signature */ OlmDevice.prototype.sign = async function(message) { - return this._getAccount(function(account) { + return await this._getAccount(function(account) { return account.sign(message); }); }; @@ -263,7 +279,7 @@ OlmDevice.prototype.sign = async function(message) { * key. */ OlmDevice.prototype.getOneTimeKeys = async function() { - return this._getAccount(function(account) { + return await this._getAccount(function(account) { return JSON.parse(account.one_time_keys()); }); }; @@ -283,9 +299,9 @@ OlmDevice.prototype.maxNumberOfOneTimeKeys = function() { */ OlmDevice.prototype.markKeysAsPublished = async function() { const self = this; - this._getAccount(function(account) { + await this._getAccount(function(account, save) { account.mark_keys_as_published(); - self._saveAccount(account); + return save(); }); }; @@ -296,9 +312,9 @@ OlmDevice.prototype.markKeysAsPublished = async function() { */ OlmDevice.prototype.generateOneTimeKeys = async function(numKeys) { const self = this; - this._getAccount(function(account) { + return this._getAccount(function(account, save) { account.generate_one_time_keys(numKeys); - self._saveAccount(account); + return save(); }); }; @@ -315,11 +331,12 @@ OlmDevice.prototype.createOutboundSession = async function( theirIdentityKey, theirOneTimeKey, ) { const self = this; - return this._getAccount(function(account) { + return await this._getAccount(async function(account, save) { const session = new Olm.Session(); try { session.create_outbound(account, theirIdentityKey, theirOneTimeKey); - self._saveSession(theirIdentityKey, session); + await save(); + await self._saveSession(theirIdentityKey, session); return session.session_id(); } finally { session.free(); @@ -349,12 +366,12 @@ OlmDevice.prototype.createInboundSession = async function( } const self = this; - return this._getAccount(function(account) { + return await this._getAccount(async function(account, save) { const session = new Olm.Session(); try { session.create_inbound_from(account, theirDeviceIdentityKey, ciphertext); account.remove_one_time_keys(session); - self._saveAccount(account); + await save(); const payloadString = session.decrypt(message_type, ciphertext); diff --git a/src/crypto/index.js b/src/crypto/index.js index 596ef9a25c8..da14c90c5d6 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -67,7 +67,7 @@ function Crypto(baseApis, sessionStore, userId, deviceId, this._clientStore = clientStore; this._cryptoStore = cryptoStore; - this._olmDevice = new OlmDevice(sessionStore); + this._olmDevice = new OlmDevice(sessionStore, cryptoStore); this._deviceList = new DeviceList(baseApis, sessionStore, this._olmDevice); // the last time we did a check for the number of one-time-keys on the diff --git a/src/crypto/store/indexeddb-crypto-store-backend.js b/src/crypto/store/indexeddb-crypto-store-backend.js index 0900a1d894e..e4cc3bcbc61 100644 --- a/src/crypto/store/indexeddb-crypto-store-backend.js +++ b/src/crypto/store/indexeddb-crypto-store-backend.js @@ -1,7 +1,7 @@ import Promise from 'bluebird'; import utils from '../../utils'; -export const VERSION = 1; +export const VERSION = 2; /** * Implementation of a CryptoStore which is backed by an existing @@ -257,6 +257,38 @@ export class Backend { }; return promiseifyTxn(txn); } + + /** + * Load the end to end account for the logged-in user, giving an object + * that has the base64 encoded account string and a method for saving + * the account string back to the database. This allows the account + * to be read and writen atomically. + * @return {Promise} Object + * @return {Promise.account} Base64 encoded account. + * @return {Promise.save} Function to save account data back. + * Takes base64 encoded account data, returns a promise. + */ + endToEndAccountTransaction() { + const txn = this._db.transaction("account", "readwrite"); + const objectStore = txn.objectStore("account"); + + + return new Promise((resolve, reject) => { + const getReq = objectStore.get("-"); + getReq.onsuccess = function() { + resolve({ + account: getReq.result || null, + save: (newData) => { + const saveReq = objectStore.put(newData, "-"); + return promiseifyTxn(txn); + }, + }); + }; + getReq.onerror = reject; + }); + + return promiseifyTxn(txn).then(() => returnObj); + } } export function upgradeDatabase(db, oldVersion) { @@ -267,6 +299,9 @@ export function upgradeDatabase(db, oldVersion) { if (oldVersion < 1) { // The database did not previously exist. createDatabase(db); } + if (oldVersion < 2) { + createV2Tables(db); + } // Expand as needed. } @@ -283,6 +318,10 @@ function createDatabase(db) { outgoingRoomKeyRequestsStore.createIndex("state", "state"); } +function createV2Tables(db) { + db.createObjectStore("account"); +} + function promiseifyTxn(txn) { return new Promise((resolve, reject) => { txn.oncomplete = resolve; diff --git a/src/crypto/store/indexeddb-crypto-store.js b/src/crypto/store/indexeddb-crypto-store.js index c3bde5fcb92..de08e9f4a73 100644 --- a/src/crypto/store/indexeddb-crypto-store.js +++ b/src/crypto/store/indexeddb-crypto-store.js @@ -220,4 +220,20 @@ export default class IndexedDBCryptoStore { return backend.deleteOutgoingRoomKeyRequest(requestId, expectedState); }); } + + /** + * Load the end to end account for the logged-in user, giving an object + * that has the base64 encoded account string and a method for saving + * the account string back to the database. This allows the account + * to be read and writen atomically. + * @return {Promise} Object + * @return {Promise.account} Base64 encoded account. + * @return {Promise.save} Function to save account data back. + * Takes base64 encoded account data, returns a promise. + */ + endToEndAccountTransaction() { + return this._connect().then((backend) => { + return backend.endToEndAccountTransaction(); + }); + } } diff --git a/src/crypto/store/memory-crypto-store.js b/src/crypto/store/memory-crypto-store.js index 9109e77108a..17d67520dd7 100644 --- a/src/crypto/store/memory-crypto-store.js +++ b/src/crypto/store/memory-crypto-store.js @@ -30,6 +30,7 @@ import utils from '../../utils'; export default class MemoryCryptoStore { constructor() { this._outgoingRoomKeyRequests = []; + this._account = null; } /** @@ -196,4 +197,23 @@ export default class MemoryCryptoStore { return Promise.resolve(null); } + + /** + * Load the end to end account for the logged-in user, giving an object + * that has the base64 encoded account string and a method for saving + * the account string back to the database. This allows the account + * to be read and writen atomically. + * @return {Promise} Object + * @return {Promise.account} Base64 encoded account. + * @return {Promise.save} Function to save account data back. + * Takes base64 encoded account data, returns a promise. + */ + endToEndAccountTransaction() { + return Promise.resolve({ + account: this._account, + save: (newData) => { + this._account = newData; + }, + }); + } } From 313cfacfa1943826c67376373e107b15fc3233c4 Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 21 Nov 2017 18:40:25 +0000 Subject: [PATCH 02/16] Add comment --- src/crypto/store/indexeddb-crypto-store-backend.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/crypto/store/indexeddb-crypto-store-backend.js b/src/crypto/store/indexeddb-crypto-store-backend.js index e4cc3bcbc61..86ead687cf3 100644 --- a/src/crypto/store/indexeddb-crypto-store-backend.js +++ b/src/crypto/store/indexeddb-crypto-store-backend.js @@ -275,6 +275,10 @@ export class Backend { return new Promise((resolve, reject) => { const getReq = objectStore.get("-"); + // We resolve on success here rather than on complete: + // the caller may wish to save the account back, which needs + // to be done while the transaction is still open (ie. before + // oncomplete) getReq.onsuccess = function() { resolve({ account: getReq.result || null, From 7ecf31313286fb567ed5dab0ed46f95f24f2d8a4 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 22 Nov 2017 10:04:27 +0000 Subject: [PATCH 03/16] Use a callback function at the store layer Rather than a promise which relies on the caller's promise handler code being run in the same tick which is not guaranteed. --- src/crypto/OlmDevice.js | 41 ++++++++------- .../store/indexeddb-crypto-store-backend.js | 50 ++++++++----------- src/crypto/store/indexeddb-crypto-store.js | 20 ++++---- src/crypto/store/memory-crypto-store.js | 27 +++++----- 4 files changed, 64 insertions(+), 74 deletions(-) diff --git a/src/crypto/OlmDevice.js b/src/crypto/OlmDevice.js index 1601c2ee415..eec5d021566 100644 --- a/src/crypto/OlmDevice.js +++ b/src/crypto/OlmDevice.js @@ -139,15 +139,15 @@ OlmDevice.prototype.init = async function() { async function _initialise_account(cryptoStore, pickleKey, account) { - const accountTxn = await cryptoStore.endToEndAccountTransaction(); - if (accountTxn.account !== null) { - account.unpickle(pickleKey, accountTxn.account); - return; - } - - account.create(); - const pickled = account.pickle(pickleKey); - await accountTxn.save(pickled); + await cryptoStore.endToEndAccountTransaction((accountData, save) => { + if (accountData !== null) { + account.unpickle(pickleKey, accountData); + } else { + account.create(); + const pickled = account.pickle(pickleKey); + save(pickled); + } + }); } /** @@ -171,23 +171,22 @@ OlmDevice.getOlmVersion = function() { OlmDevice.prototype._getAccount = async function(func) { let result; - let account = null; - try { - const accountTxn = await this._cryptoStore.endToEndAccountTransaction(); + await this._cryptoStore.endToEndAccountTransaction((accountData, save) => { // Olm has a limited stack size so we must tightly control the number of // Olm account objects in existence at any given time: once created, it // must be destroyed again before we await. - account = new Olm.Account(); - account.unpickle(this._pickleKey, accountTxn.account); + const account = new Olm.Account(); + try { + account.unpickle(this._pickleKey, accountData); - result = func(account, () => { + result = func(account, () => { const pickledAccount = account.pickle(this._pickleKey); - return accountTxn.save(pickledAccount); - } - ); - } finally { - if (account !== null) account.free(); - } + return save(pickledAccount); + }); + } finally { + if (account !== null) account.free(); + } + }); return result; }; diff --git a/src/crypto/store/indexeddb-crypto-store-backend.js b/src/crypto/store/indexeddb-crypto-store-backend.js index 86ead687cf3..5eef3917e34 100644 --- a/src/crypto/store/indexeddb-crypto-store-backend.js +++ b/src/crypto/store/indexeddb-crypto-store-backend.js @@ -259,39 +259,33 @@ export class Backend { } /** - * Load the end to end account for the logged-in user, giving an object - * that has the base64 encoded account string and a method for saving - * the account string back to the database. This allows the account - * to be read and writen atomically. - * @return {Promise} Object - * @return {Promise.account} Base64 encoded account. - * @return {Promise.save} Function to save account data back. - * Takes base64 encoded account data, returns a promise. + * Load the end to end account for the logged-in user. Once the account + * is retrieved, the given function is executed and passed the base64 + * encoded account string and a method for saving the account string + * back to the database. This allows the account to be read and writen + * atomically. + * @return {Promise} * Resolves with the return value of the function once + * the transaction is complete (ie. once data is written back if the + * save function is called. */ - endToEndAccountTransaction() { + endToEndAccountTransaction(func) { const txn = this._db.transaction("account", "readwrite"); const objectStore = txn.objectStore("account"); + const txnPromise = promiseifyTxn(txn); - return new Promise((resolve, reject) => { - const getReq = objectStore.get("-"); - // We resolve on success here rather than on complete: - // the caller may wish to save the account back, which needs - // to be done while the transaction is still open (ie. before - // oncomplete) - getReq.onsuccess = function() { - resolve({ - account: getReq.result || null, - save: (newData) => { - const saveReq = objectStore.put(newData, "-"); - return promiseifyTxn(txn); - }, - }); - }; - getReq.onerror = reject; - }); - - return promiseifyTxn(txn).then(() => returnObj); + const getReq = objectStore.get("-"); + let result; + getReq.onsuccess = function() { + result = func( + getReq.result || null, + (newData) => { + const saveReq = objectStore.put(newData, "-"); + return txnPromise; + }, + ); + }; + return txnPromise; } } diff --git a/src/crypto/store/indexeddb-crypto-store.js b/src/crypto/store/indexeddb-crypto-store.js index de08e9f4a73..0360b080f5e 100644 --- a/src/crypto/store/indexeddb-crypto-store.js +++ b/src/crypto/store/indexeddb-crypto-store.js @@ -222,18 +222,18 @@ export default class IndexedDBCryptoStore { } /** - * Load the end to end account for the logged-in user, giving an object - * that has the base64 encoded account string and a method for saving - * the account string back to the database. This allows the account - * to be read and writen atomically. - * @return {Promise} Object - * @return {Promise.account} Base64 encoded account. - * @return {Promise.save} Function to save account data back. - * Takes base64 encoded account data, returns a promise. + * Load the end to end account for the logged-in user. Once the account + * is retrieved, the given function is executed and passed the base64 + * encoded account string and a method for saving the account string + * back to the database. This allows the account to be read and writen + * atomically. + * @return {Promise} * Resolves with the return value of the function once + * the transaction is complete (ie. once data is written back if the + * save function is called. */ - endToEndAccountTransaction() { + endToEndAccountTransaction(func) { return this._connect().then((backend) => { - return backend.endToEndAccountTransaction(); + return backend.endToEndAccountTransaction(func); }); } } diff --git a/src/crypto/store/memory-crypto-store.js b/src/crypto/store/memory-crypto-store.js index 17d67520dd7..af9efa720f5 100644 --- a/src/crypto/store/memory-crypto-store.js +++ b/src/crypto/store/memory-crypto-store.js @@ -199,21 +199,18 @@ export default class MemoryCryptoStore { } /** - * Load the end to end account for the logged-in user, giving an object - * that has the base64 encoded account string and a method for saving - * the account string back to the database. This allows the account - * to be read and writen atomically. - * @return {Promise} Object - * @return {Promise.account} Base64 encoded account. - * @return {Promise.save} Function to save account data back. - * Takes base64 encoded account data, returns a promise. + * Load the end to end account for the logged-in user. Once the account + * is retrieved, the given function is executed and passed the base64 + * encoded account string and a method for saving the account string + * back to the database. This allows the account to be read and writen + * atomically. + * @return {Promise} * Resolves with the return value of the function once + * the transaction is complete (ie. once data is written back if the + * save function is called. */ - endToEndAccountTransaction() { - return Promise.resolve({ - account: this._account, - save: (newData) => { - this._account = newData; - }, - }); + endToEndAccountTransaction(func) { + return Promise.resolve(func(this._account, (newData) => { + this._account = newData; + })); } } From a5c5da5b8a8a2e60892e3cdde454ed9065bca917 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 22 Nov 2017 10:18:53 +0000 Subject: [PATCH 04/16] Lint --- src/crypto/OlmDevice.js | 6 +++--- src/crypto/store/indexeddb-crypto-store-backend.js | 9 ++++++--- src/crypto/store/indexeddb-crypto-store.js | 3 ++- src/crypto/store/memory-crypto-store.js | 3 ++- 4 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/crypto/OlmDevice.js b/src/crypto/OlmDevice.js index eec5d021566..9bc36c2b489 100644 --- a/src/crypto/OlmDevice.js +++ b/src/crypto/OlmDevice.js @@ -74,7 +74,8 @@ function checkPayloadLength(payloadString) { * @alias module:crypto/OlmDevice * * @param {Object} sessionStore A store to be used for data in end-to-end - * crypto + * crypto. This is deprecated and being replaced by cryptoStore. + * @param {Object} cryptoStore A store for crypto data * * @property {string} deviceCurve25519Key Curve25519 key for the account * @property {string} deviceEd25519Key Ed25519 key for the account @@ -297,7 +298,6 @@ OlmDevice.prototype.maxNumberOfOneTimeKeys = function() { * Marks all of the one-time keys as published. */ OlmDevice.prototype.markKeysAsPublished = async function() { - const self = this; await this._getAccount(function(account, save) { account.mark_keys_as_published(); return save(); @@ -308,9 +308,9 @@ OlmDevice.prototype.markKeysAsPublished = async function() { * Generate some new one-time keys * * @param {number} numKeys number of keys to generate + * @return {Promise} Resolved once the account is saved back having generated the keys */ OlmDevice.prototype.generateOneTimeKeys = async function(numKeys) { - const self = this; return this._getAccount(function(account, save) { account.generate_one_time_keys(numKeys); return save(); diff --git a/src/crypto/store/indexeddb-crypto-store-backend.js b/src/crypto/store/indexeddb-crypto-store-backend.js index 5eef3917e34..63ec0e109a7 100644 --- a/src/crypto/store/indexeddb-crypto-store-backend.js +++ b/src/crypto/store/indexeddb-crypto-store-backend.js @@ -264,7 +264,8 @@ export class Backend { * encoded account string and a method for saving the account string * back to the database. This allows the account to be read and writen * atomically. - * @return {Promise} * Resolves with the return value of the function once + * @param {func} func Function called with the account data and a save function + * @return {Promise} Resolves with the return value of the function once * the transaction is complete (ie. once data is written back if the * save function is called. */ @@ -280,12 +281,14 @@ export class Backend { result = func( getReq.result || null, (newData) => { - const saveReq = objectStore.put(newData, "-"); + objectStore.put(newData, "-"); return txnPromise; }, ); }; - return txnPromise; + return txnPromise.then(() => { + return result; + }); } } diff --git a/src/crypto/store/indexeddb-crypto-store.js b/src/crypto/store/indexeddb-crypto-store.js index 0360b080f5e..b950e3a5ba0 100644 --- a/src/crypto/store/indexeddb-crypto-store.js +++ b/src/crypto/store/indexeddb-crypto-store.js @@ -227,7 +227,8 @@ export default class IndexedDBCryptoStore { * encoded account string and a method for saving the account string * back to the database. This allows the account to be read and writen * atomically. - * @return {Promise} * Resolves with the return value of the function once + * @param {func} func Function called with the account data and a save function + * @return {Promise} Resolves with the return value of the function once * the transaction is complete (ie. once data is written back if the * save function is called. */ diff --git a/src/crypto/store/memory-crypto-store.js b/src/crypto/store/memory-crypto-store.js index af9efa720f5..4c9558a8a7b 100644 --- a/src/crypto/store/memory-crypto-store.js +++ b/src/crypto/store/memory-crypto-store.js @@ -204,7 +204,8 @@ export default class MemoryCryptoStore { * encoded account string and a method for saving the account string * back to the database. This allows the account to be read and writen * atomically. - * @return {Promise} * Resolves with the return value of the function once + * @param {func} func Function called with the account data and a save function + * @return {Promise} Resolves with the return value of the function once * the transaction is complete (ie. once data is written back if the * save function is called. */ From bae3f5ceb7ddd3778488a3edc08fc19a473902cb Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 22 Nov 2017 10:19:27 +0000 Subject: [PATCH 05/16] It's a heap, not a stack --- src/crypto/OlmDevice.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/crypto/OlmDevice.js b/src/crypto/OlmDevice.js index 9bc36c2b489..984cbf5d060 100644 --- a/src/crypto/OlmDevice.js +++ b/src/crypto/OlmDevice.js @@ -173,7 +173,7 @@ OlmDevice.prototype._getAccount = async function(func) { let result; await this._cryptoStore.endToEndAccountTransaction((accountData, save) => { - // Olm has a limited stack size so we must tightly control the number of + // Olm has a limited heap size so we must tightly control the number of // Olm account objects in existence at any given time: once created, it // must be destroyed again before we await. const account = new Olm.Account(); From 59f228dab7fd1fbe9a363759f4c81f1d84ab0b8f Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 22 Nov 2017 14:07:19 +0000 Subject: [PATCH 06/16] Migrate account from session store --- src/crypto/OlmDevice.js | 23 ++++++++++++++++++----- src/store/session/webstorage.js | 8 +++----- 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/src/crypto/OlmDevice.js b/src/crypto/OlmDevice.js index 984cbf5d060..9ae4dd2e8b7 100644 --- a/src/crypto/OlmDevice.js +++ b/src/crypto/OlmDevice.js @@ -126,7 +126,7 @@ OlmDevice.prototype.init = async function() { let e2eKeys; const account = new Olm.Account(); try { - await _initialise_account(this._cryptoStore, this._pickleKey, account); + await _initialise_account(this._sessionStore, this._cryptoStore, this._pickleKey, account); e2eKeys = JSON.parse(account.identity_keys()); this._maxOneTimeKeys = account.max_number_of_one_time_keys(); @@ -139,16 +139,29 @@ OlmDevice.prototype.init = async function() { }; -async function _initialise_account(cryptoStore, pickleKey, account) { +async function _initialise_account(sessionStore, cryptoStore, pickleKey, account) { + let removeFromSessionStore = false; await cryptoStore.endToEndAccountTransaction((accountData, save) => { if (accountData !== null) { account.unpickle(pickleKey, accountData); } else { - account.create(); - const pickled = account.pickle(pickleKey); - save(pickled); + // Migrate from sessionStore + accountData = sessionStore.getEndToEndAccount(); + if (accountData !== null) { + removeFromSessionStore = true; + account.unpickle(pickleKey, accountData); + } else { + account.create(); + accountData = account.pickle(pickleKey); + } + save(accountData); } }); + + // only remove this once it's safely saved to the crypto store + if (removeFromSessionStore) { + sessionStore.removeEndToEndAccount(); + } } /** diff --git a/src/store/session/webstorage.js b/src/store/session/webstorage.js index 53220cb5e40..1365e5fca76 100644 --- a/src/store/session/webstorage.js +++ b/src/store/session/webstorage.js @@ -48,13 +48,11 @@ function WebStorageSessionStore(webStore) { } WebStorageSessionStore.prototype = { - /** - * Store the end to end account for the logged-in user. - * @param {string} account Base64 encoded account. + * Remove the stored end to end account for the logged-in user. */ - storeEndToEndAccount: function(account) { - this.store.setItem(KEY_END_TO_END_ACCOUNT, account); + removeEndToEndAccount: function() { + this.store.removeItem(KEY_END_TO_END_ACCOUNT); }, /** From a90f5922248a36b540495dede62bb9fc23920e29 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 22 Nov 2017 14:12:35 +0000 Subject: [PATCH 07/16] Add comment on deprecation --- src/store/session/webstorage.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/store/session/webstorage.js b/src/store/session/webstorage.js index 1365e5fca76..f9e75ee3f88 100644 --- a/src/store/session/webstorage.js +++ b/src/store/session/webstorage.js @@ -57,6 +57,9 @@ WebStorageSessionStore.prototype = { /** * Load the end to end account for the logged-in user. + * Note that the end-to-end account is now stored in the + * crypto store rather than here: this remains here so + * old sessions can be migrated out of the session store. * @return {?string} Base64 encoded account. */ getEndToEndAccount: function() { From 4b7157b9878969b9077e56962e6827bdddbb8358 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 22 Nov 2017 14:31:06 +0000 Subject: [PATCH 08/16] Remove unnecessary 'if' --- src/crypto/OlmDevice.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/crypto/OlmDevice.js b/src/crypto/OlmDevice.js index 9ae4dd2e8b7..f88d295f0a3 100644 --- a/src/crypto/OlmDevice.js +++ b/src/crypto/OlmDevice.js @@ -198,7 +198,7 @@ OlmDevice.prototype._getAccount = async function(func) { return save(pickledAccount); }); } finally { - if (account !== null) account.free(); + account.free(); } }); return result; From 9218e518f1748d58fd7a4d9705f41bf7ccb6333c Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 22 Nov 2017 16:41:52 +0000 Subject: [PATCH 09/16] Add LocalStorageCryptoStore To avoid throwing away all the data for anyone running firefox in one of the modes where indexedDB is broken. --- src/crypto/store/indexeddb-crypto-store.js | 8 ++- src/crypto/store/localStorage-crypto-store.js | 59 +++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) create mode 100644 src/crypto/store/localStorage-crypto-store.js diff --git a/src/crypto/store/indexeddb-crypto-store.js b/src/crypto/store/indexeddb-crypto-store.js index b950e3a5ba0..ff07dd42db7 100644 --- a/src/crypto/store/indexeddb-crypto-store.js +++ b/src/crypto/store/indexeddb-crypto-store.js @@ -16,6 +16,7 @@ limitations under the License. import Promise from 'bluebird'; +import LocalStorageCryptoStore from './localStorage-crypto-store'; import MemoryCryptoStore from './memory-crypto-store'; import * as IndexedDBCryptoStoreBackend from './indexeddb-crypto-store-backend'; @@ -93,7 +94,12 @@ export default class IndexedDBCryptoStore { }).catch((e) => { console.warn( `unable to connect to indexeddb ${this._dbName}` + - `: falling back to in-memory store: ${e}`, + `: falling back to localStorage store: ${e}`, + ); + return new LocalStorageCryptoStore(); + }).catch((e) => { + console.warn( + `unable to open localStorage: falling back to in-memory store: ${e}`, ); return new MemoryCryptoStore(); }); diff --git a/src/crypto/store/localStorage-crypto-store.js b/src/crypto/store/localStorage-crypto-store.js new file mode 100644 index 00000000000..94e7b9c72b6 --- /dev/null +++ b/src/crypto/store/localStorage-crypto-store.js @@ -0,0 +1,59 @@ +/* +Copyright 2017 New Vector Ltd + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import Promise from 'bluebird'; +import MemoryCryptoStore from './memory-crypto-store.js'; + +/** + * Internal module. Partial localStorage backed storage for e2e. + * This is not a full crypto store, just the in-memory store with + * some things backed by localStorage. It exists because indexedDB + * is broken in Firefox private mode or set to, "will not remember + * history". + * + * + * @module + */ + +const E2E_PREFIX = "crypto."; +const KEY_END_TO_END_ACCOUNT = E2E_PREFIX + "account"; + +/** + * @implements {module:crypto/store/base~CryptoStore} + */ +export default class LocalStorageCryptoStore extends MemoryCryptoStore { + constructor() { + super(); + this.store = global.localStorage; + } + + /** + * Delete all data from this store. + * + * @returns {Promise} Promise which resolves when the store has been cleared. + */ + deleteAllData() { + this.store.removeItem(KEY_END_TO_END_ACCOUNT); + return Promise.resolve(); + } + + endToEndAccountTransaction(func) { + const account = this.store.getItem(KEY_END_TO_END_ACCOUNT); + return Promise.resolve(func(account, (newData) => { + this.store.setItem(KEY_END_TO_END_ACCOUNT, newData); + })); + } +} From 44b35cdb3d10ca66d07e08bbaf7b191f476709c4 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 22 Nov 2017 16:53:21 +0000 Subject: [PATCH 10/16] Lint --- src/crypto/OlmDevice.js | 4 +++- src/crypto/store/localStorage-crypto-store.js | 1 - 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/crypto/OlmDevice.js b/src/crypto/OlmDevice.js index f88d295f0a3..70292acce0d 100644 --- a/src/crypto/OlmDevice.js +++ b/src/crypto/OlmDevice.js @@ -126,7 +126,9 @@ OlmDevice.prototype.init = async function() { let e2eKeys; const account = new Olm.Account(); try { - await _initialise_account(this._sessionStore, this._cryptoStore, this._pickleKey, account); + await _initialise_account( + this._sessionStore, this._cryptoStore, this._pickleKey, account, + ); e2eKeys = JSON.parse(account.identity_keys()); this._maxOneTimeKeys = account.max_number_of_one_time_keys(); diff --git a/src/crypto/store/localStorage-crypto-store.js b/src/crypto/store/localStorage-crypto-store.js index 94e7b9c72b6..a9c5b5845cd 100644 --- a/src/crypto/store/localStorage-crypto-store.js +++ b/src/crypto/store/localStorage-crypto-store.js @@ -23,7 +23,6 @@ import MemoryCryptoStore from './memory-crypto-store.js'; * some things backed by localStorage. It exists because indexedDB * is broken in Firefox private mode or set to, "will not remember * history". - * * * @module */ From 6024163af8f8ac3dfb8d512f64c9ea33bb8e2d69 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 22 Nov 2017 17:50:00 +0000 Subject: [PATCH 11/16] s/accountData/pickledAccount/ --- src/crypto/OlmDevice.js | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/crypto/OlmDevice.js b/src/crypto/OlmDevice.js index 70292acce0d..93ce2cc9561 100644 --- a/src/crypto/OlmDevice.js +++ b/src/crypto/OlmDevice.js @@ -143,20 +143,20 @@ OlmDevice.prototype.init = async function() { async function _initialise_account(sessionStore, cryptoStore, pickleKey, account) { let removeFromSessionStore = false; - await cryptoStore.endToEndAccountTransaction((accountData, save) => { - if (accountData !== null) { - account.unpickle(pickleKey, accountData); + await cryptoStore.endToEndAccountTransaction((pickledAccount, save) => { + if (pickledAccount !== null) { + account.unpickle(pickleKey, pickledAccount); } else { // Migrate from sessionStore - accountData = sessionStore.getEndToEndAccount(); - if (accountData !== null) { + pickledAccount = sessionStore.getEndToEndAccount(); + if (pickledAccount !== null) { removeFromSessionStore = true; - account.unpickle(pickleKey, accountData); + account.unpickle(pickleKey, pickledAccount); } else { account.create(); - accountData = account.pickle(pickleKey); + pickledAccount = account.pickle(pickleKey); } - save(accountData); + save(pickledAccount); } }); @@ -187,13 +187,13 @@ OlmDevice.getOlmVersion = function() { OlmDevice.prototype._getAccount = async function(func) { let result; - await this._cryptoStore.endToEndAccountTransaction((accountData, save) => { + await this._cryptoStore.endToEndAccountTransaction((pickledAccount, save) => { // Olm has a limited heap size so we must tightly control the number of // Olm account objects in existence at any given time: once created, it // must be destroyed again before we await. const account = new Olm.Account(); try { - account.unpickle(this._pickleKey, accountData); + account.unpickle(this._pickleKey, pickledAccount); result = func(account, () => { const pickledAccount = account.pickle(this._pickleKey); From 57d425fae614f8835c2332888906a2b8a2b29246 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 22 Nov 2017 18:05:08 +0000 Subject: [PATCH 12/16] Make the save function not return a promise This was entirely unnecessary and hopefully make things a bit simpler to understand and has fewer asyncs flying around. --- src/crypto/OlmDevice.js | 16 ++++++++-------- .../store/indexeddb-crypto-store-backend.js | 1 - 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/crypto/OlmDevice.js b/src/crypto/OlmDevice.js index 93ce2cc9561..b4f383019ac 100644 --- a/src/crypto/OlmDevice.js +++ b/src/crypto/OlmDevice.js @@ -197,7 +197,7 @@ OlmDevice.prototype._getAccount = async function(func) { result = func(account, () => { const pickledAccount = account.pickle(this._pickleKey); - return save(pickledAccount); + save(pickledAccount); }); } finally { account.free(); @@ -315,7 +315,7 @@ OlmDevice.prototype.maxNumberOfOneTimeKeys = function() { OlmDevice.prototype.markKeysAsPublished = async function() { await this._getAccount(function(account, save) { account.mark_keys_as_published(); - return save(); + save(); }); }; @@ -328,7 +328,7 @@ OlmDevice.prototype.markKeysAsPublished = async function() { OlmDevice.prototype.generateOneTimeKeys = async function(numKeys) { return this._getAccount(function(account, save) { account.generate_one_time_keys(numKeys); - return save(); + save(); }); }; @@ -345,12 +345,12 @@ OlmDevice.prototype.createOutboundSession = async function( theirIdentityKey, theirOneTimeKey, ) { const self = this; - return await this._getAccount(async function(account, save) { + return await this._getAccount(function(account, save) { const session = new Olm.Session(); try { session.create_outbound(account, theirIdentityKey, theirOneTimeKey); - await save(); - await self._saveSession(theirIdentityKey, session); + save(); + self._saveSession(theirIdentityKey, session); return session.session_id(); } finally { session.free(); @@ -380,12 +380,12 @@ OlmDevice.prototype.createInboundSession = async function( } const self = this; - return await this._getAccount(async function(account, save) { + return await this._getAccount(function(account, save) { const session = new Olm.Session(); try { session.create_inbound_from(account, theirDeviceIdentityKey, ciphertext); account.remove_one_time_keys(session); - await save(); + save(); const payloadString = session.decrypt(message_type, ciphertext); diff --git a/src/crypto/store/indexeddb-crypto-store-backend.js b/src/crypto/store/indexeddb-crypto-store-backend.js index 63ec0e109a7..969ab8e990a 100644 --- a/src/crypto/store/indexeddb-crypto-store-backend.js +++ b/src/crypto/store/indexeddb-crypto-store-backend.js @@ -282,7 +282,6 @@ export class Backend { getReq.result || null, (newData) => { objectStore.put(newData, "-"); - return txnPromise; }, ); }; From c4e70be0a5fe12dbf3f54b84737733fe5d017759 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 22 Nov 2017 18:25:25 +0000 Subject: [PATCH 13/16] Better comment wording --- src/crypto/OlmDevice.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/crypto/OlmDevice.js b/src/crypto/OlmDevice.js index b4f383019ac..dde90fbb697 100644 --- a/src/crypto/OlmDevice.js +++ b/src/crypto/OlmDevice.js @@ -177,8 +177,8 @@ OlmDevice.getOlmVersion = function() { /** * extract our OlmAccount from the crypto store and call the given function * with the account object and a 'save' function which returns a promise. - * The function will not be awaited upon and the save function must be - * called before the function returns, or not at all. + * The `account` will be freed as soon as `func` returns - even if func returns + * a promise * * @param {function} func * @return {object} result of func From defaa918a632d49f9a25fb37de4ae4e0213f6895 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 22 Nov 2017 18:26:26 +0000 Subject: [PATCH 14/16] Remove unused function --- src/crypto/OlmDevice.js | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/crypto/OlmDevice.js b/src/crypto/OlmDevice.js index dde90fbb697..684dfbb6752 100644 --- a/src/crypto/OlmDevice.js +++ b/src/crypto/OlmDevice.js @@ -207,18 +207,6 @@ OlmDevice.prototype._getAccount = async function(func) { }; -/** - * store our OlmAccount in the session store - * - * @param {OlmAccount} account - * @private - */ -OlmDevice.prototype._saveAccount = async function(account) { - const pickledAccount = account.pickle(this._pickleKey); - await this._cryptoStore.storeEndToEndAccount(pickledAccount); -}; - - /** * extract an OlmSession from the session store and call the given function * From 6ebfd175bc42a467518b333dbefa648a883bef66 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 22 Nov 2017 18:37:16 +0000 Subject: [PATCH 15/16] jsdoc clarifications --- src/crypto/store/indexeddb-crypto-store-backend.js | 11 ++++++----- src/crypto/store/indexeddb-crypto-store.js | 11 ++++++----- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/crypto/store/indexeddb-crypto-store-backend.js b/src/crypto/store/indexeddb-crypto-store-backend.js index 969ab8e990a..c3def65ba97 100644 --- a/src/crypto/store/indexeddb-crypto-store-backend.js +++ b/src/crypto/store/indexeddb-crypto-store-backend.js @@ -260,14 +260,15 @@ export class Backend { /** * Load the end to end account for the logged-in user. Once the account - * is retrieved, the given function is executed and passed the base64 - * encoded account string and a method for saving the account string + * is retrieved, the given function is executed and passed the pickled + * account string and a method for saving the pickle * back to the database. This allows the account to be read and writen * atomically. - * @param {func} func Function called with the account data and a save function - * @return {Promise} Resolves with the return value of the function once + * @param {function(string, function())} func Function called with the + * account data and a save function + * @return {Promise} Resolves with the return value of `func` once * the transaction is complete (ie. once data is written back if the - * save function is called. + * save function is called.) */ endToEndAccountTransaction(func) { const txn = this._db.transaction("account", "readwrite"); diff --git a/src/crypto/store/indexeddb-crypto-store.js b/src/crypto/store/indexeddb-crypto-store.js index ff07dd42db7..54b044f3d2b 100644 --- a/src/crypto/store/indexeddb-crypto-store.js +++ b/src/crypto/store/indexeddb-crypto-store.js @@ -229,14 +229,15 @@ export default class IndexedDBCryptoStore { /** * Load the end to end account for the logged-in user. Once the account - * is retrieved, the given function is executed and passed the base64 - * encoded account string and a method for saving the account string + * is retrieved, the given function is executed and passed the pickled + * account string and a method for saving the pickle * back to the database. This allows the account to be read and writen * atomically. - * @param {func} func Function called with the account data and a save function - * @return {Promise} Resolves with the return value of the function once + * @param {function(string, function())} func Function called with the + * account data and a save function + * @return {Promise} Resolves with the return value of `func` once * the transaction is complete (ie. once data is written back if the - * save function is called. + * save function is called.) */ endToEndAccountTransaction(func) { return this._connect().then((backend) => { From 7e2c236582219f80586f09fde1433cb95f5faf0e Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 27 Nov 2017 13:46:31 +0000 Subject: [PATCH 16/16] Missed a s/account data/picked account/ --- src/crypto/store/indexeddb-crypto-store-backend.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/crypto/store/indexeddb-crypto-store-backend.js b/src/crypto/store/indexeddb-crypto-store-backend.js index c3def65ba97..31bdfae0cb0 100644 --- a/src/crypto/store/indexeddb-crypto-store-backend.js +++ b/src/crypto/store/indexeddb-crypto-store-backend.js @@ -265,7 +265,7 @@ export class Backend { * back to the database. This allows the account to be read and writen * atomically. * @param {function(string, function())} func Function called with the - * account data and a save function + * picked account and a save function * @return {Promise} Resolves with the return value of `func` once * the transaction is complete (ie. once data is written back if the * save function is called.)