diff --git a/spec/unit/crypto/algorithms/megolm.spec.js b/spec/unit/crypto/algorithms/megolm.spec.js index 7f86f86ff..3285d2b19 100644 --- a/spec/unit/crypto/algorithms/megolm.spec.js +++ b/spec/unit/crypto/algorithms/megolm.spec.js @@ -71,9 +71,13 @@ describe("MegolmDecryption", function() { groupSession = new global.Olm.OutboundGroupSession(); groupSession.create(); - const event = new MatrixEvent({}); - event.setClearData( - { + // construct a fake decrypted key event via the use of a mocked + // 'crypto' implementation. + const event = new MatrixEvent({ + type: 'm.room.encrypted', + }); + const decryptedData = { + clearEvent: { type: 'm.room_key', content: { algorithm: 'm.megolm.v1.aes-sha2', @@ -82,11 +86,19 @@ describe("MegolmDecryption", function() { session_key: groupSession.session_key(), }, }, - "SENDER_CURVE25519", - "SENDER_ED25519", - ); + senderCurve25519Key: "SENDER_CURVE25519", + claimedEd25519Key: "SENDER_ED25519", + }; - megolmDecryption.onRoomKeyEvent(event); + const mockCrypto = { + decryptEvent: function() { + return Promise.resolve(decryptedData); + }, + }; + + return event.attemptDecryption(mockCrypto).then(() => { + megolmDecryption.onRoomKeyEvent(event); + }); }); it('can decrypt an event', function() { @@ -104,8 +116,8 @@ describe("MegolmDecryption", function() { }, }); - return megolmDecryption.decryptEvent(event).then(() => { - expect(event.getContent()).toEqual('testytest'); + return megolmDecryption.decryptEvent(event).then((res) => { + expect(res.clearEvent.content).toEqual('testytest'); }); }); diff --git a/spec/unit/event.spec.js b/spec/unit/event.spec.js new file mode 100644 index 000000000..0c8acdacf --- /dev/null +++ b/spec/unit/event.spec.js @@ -0,0 +1,74 @@ +/* +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 sdk from '../..'; +const MatrixEvent = sdk.MatrixEvent; + +import testUtils from '../test-utils'; + +import expect from 'expect'; +import Promise from 'bluebird'; + +describe("MatrixEvent", () => { + beforeEach(function() { + testUtils.beforeEach(this); // eslint-disable-line no-invalid-this + }); + + describe(".attemptDecryption", () => { + let encryptedEvent; + + beforeEach(() => { + encryptedEvent = new MatrixEvent({ + id: 'test_encrypted_event', + type: 'm.room.encrypted', + content: { + ciphertext: 'secrets', + }, + }); + }); + + it('should retry decryption if a retry is queued', () => { + let callCount = 0; + + const crypto = { + decryptEvent: function() { + ++callCount; + console.log(`decrypt: ${callCount}`); + if (callCount == 1) { + // schedule a second decryption attempt while + // the first one is still running. + encryptedEvent.attemptDecryption(crypto); + + const error = new Error("nope"); + error.name = 'DecryptionError'; + return Promise.reject(error); + } else { + return Promise.resolve({ + clearEvent: { + type: 'm.room.message', + }, + }); + } + }, + }; + + return encryptedEvent.attemptDecryption(crypto).then(() => { + expect(callCount).toEqual(2); + expect(encryptedEvent.getType()).toEqual('m.room.message'); + }); + }); + }); +}); diff --git a/src/crypto/algorithms/base.js b/src/crypto/algorithms/base.js index 15f8aed25..c74e81f00 100644 --- a/src/crypto/algorithms/base.js +++ b/src/crypto/algorithms/base.js @@ -117,7 +117,8 @@ class DecryptionAlgorithm { * * @param {MatrixEvent} event undecrypted event * - * @return {Promise} resolves once we have finished decrypting. Rejects with an + * @return {Promise} promise which + * resolves once we have finished decrypting. Rejects with an * `algorithms.DecryptionError` if there is a problem decrypting the event. */ diff --git a/src/crypto/algorithms/megolm.js b/src/crypto/algorithms/megolm.js index 6034e71d3..3b2eb0d9a 100644 --- a/src/crypto/algorithms/megolm.js +++ b/src/crypto/algorithms/megolm.js @@ -535,9 +535,10 @@ utils.inherits(MegolmDecryption, base.DecryptionAlgorithm); * * @param {MatrixEvent} event * - * returns a promise which resolves once we have finished decrypting, or - * rejects with an `algorithms.DecryptionError` if there is a problem - * decrypting the event. + * returns a promise which resolves to a + * {@link module:crypto~EventDecryptionResult} once we have finished + * decrypting, or rejects with an `algorithms.DecryptionError` if there is a + * problem decrypting the event. */ MegolmDecryption.prototype.decryptEvent = async function(event) { const content = event.getWireContent(); @@ -588,8 +589,12 @@ MegolmDecryption.prototype.decryptEvent = async function(event) { ); } - event.setClearData(payload, res.senderKey, res.keysClaimed.ed25519, - res.forwardingCurve25519KeyChain); + return { + clearEvent: payload, + senderCurve25519Key: res.senderKey, + claimedEd25519Key: res.keysClaimed.ed25519, + forwardingCurve25519KeyChain: res.forwardingCurve25519KeyChain, + }; }; MegolmDecryption.prototype._requestKeysForEvent = function(event) { diff --git a/src/crypto/algorithms/olm.js b/src/crypto/algorithms/olm.js index c27e519a7..483556e53 100644 --- a/src/crypto/algorithms/olm.js +++ b/src/crypto/algorithms/olm.js @@ -157,8 +157,10 @@ utils.inherits(OlmDecryption, base.DecryptionAlgorithm); * * @param {MatrixEvent} event * - * returns a promise which resolves once we have finished decrypting. Rejects with an - * `algorithms.DecryptionError` if there is a problem decrypting the event. + * returns a promise which resolves to a + * {@link module:crypto~EventDecryptionResult} once we have finished + * decrypting. Rejects with an `algorithms.DecryptionError` if there is a + * problem decrypting the event. */ OlmDecryption.prototype.decryptEvent = async function(event) { const content = event.getWireContent(); @@ -227,9 +229,13 @@ OlmDecryption.prototype.decryptEvent = async function(event) { } const claimedKeys = payload.keys || {}; - event.setClearData(payload, deviceKey, claimedKeys.ed25519 || null); -}; + return { + clearEvent: payload, + senderCurve25519Key: deviceKey, + claimedEd25519Key: claimedKeys.ed25519 || null, + }; +}; /** * Attempt to decrypt an Olm message diff --git a/src/crypto/index.js b/src/crypto/index.js index 6c28bce3f..37ffeb21e 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -771,8 +771,9 @@ Crypto.prototype.encryptEvent = function(event, room) { * * @param {MatrixEvent} event * - * @return {Promise} resolves once we have finished decrypting. Rejects with an - * `algorithms.DecryptionError` if there is a problem decrypting the event. + * @return {Promise} resolves once we have + * finished decrypting. Rejects with an `algorithms.DecryptionError` if there + * is a problem decrypting the event. */ Crypto.prototype.decryptEvent = function(event) { const content = event.getWireContent(); @@ -1294,6 +1295,25 @@ class IncomingRoomKeyRequestCancellation { } } +/** + * The result of a (successful) call to decryptEvent. + * + * @typedef {Object} EventDecryptionResult + * + * @property {Object} clearEvent The plaintext payload for the event + * (typically containing type and content fields). + * + * @property {string?} senderCurve25519Key Key owned by the sender of this event. + * See {@link module:models/event.MatrixEvent#getSenderKey}. + * + * @property {string?} claimedEd25519Key ed25519 key claimed by the sender of + * this event. See {@link module:models/event.MatrixEvent#getClaimedEd25519Key}. + * + * @property {Array?} forwardingCurve25519KeyChain list of curve25519 keys + * involved in telling us about the senderCurve25519Key and claimedEd25519Key. + * See {@link module:models/event.MatrixEvent#getForwardingCurve25519KeyChain}. + */ + /** * Fires when we receive a room key request * diff --git a/src/models/event.js b/src/models/event.js index 0e2f862ac..fa7fe280d 100644 --- a/src/models/event.js +++ b/src/models/event.js @@ -25,8 +25,6 @@ const EventEmitter = require("events").EventEmitter; const utils = require('../utils.js'); -import Promise from 'bluebird'; - /** * Enum for event statuses. * @readonly @@ -326,13 +324,12 @@ utils.extend(module.exports.MatrixEvent.prototype, { * @internal * * @param {module:crypto} crypto crypto module + * + * returns a promise which resolves (to undefined) when the decryption + * attempt is completed. */ - attemptDecryption: function(crypto) { - if (!crypto) { - this._badEncryptedMessage("Encryption not enabled"); - return; - } - + attemptDecryption: async function(crypto) { + // start with a couple of sanity checks. if (!this.isEncrypted()) { throw new Error("Attempt to decrypt event which isn't encrypted"); } @@ -347,6 +344,12 @@ utils.extend(module.exports.MatrixEvent.prototype, { ); } + // if we already have a decryption attempt in progress, then it may + // fail because it was using outdated info. We now have reason to + // succeed where it failed before, but we don't want to have multiple + // attempts going at the same time, so just set a flag that says we have + // new info. + // if (this._decrypting) { console.log( `Event ${this.getId()} already being decrypted; queueing a retry`, @@ -356,51 +359,86 @@ utils.extend(module.exports.MatrixEvent.prototype, { } this._decrypting = true; - - this._doDecryption(crypto).finally(() => { - this._decrypting = false; + while (true) { this._retryDecryption = false; - }); - }, - _doDecryption: function(crypto) { - return Promise.try(() => { - return crypto.decryptEvent(this); - }).catch((e) => { - if (e.name !== "DecryptionError") { - // not a decryption error: log the whole exception as an error. - console.error( - `Error decrypting event (id=${this.getId()}): ${e.stack || e}`, - ); - return null; - } else if (this._retryDecryption) { - // decryption error, but we have a retry queued. - console.log( - `Got error decrypting event (id=${this.getId()}), but retrying`, - ); - this._retryDecryption = false; - return this._doDecryption(crypto); - } else { + let res; + try { + if (!crypto) { + res = this._badEncryptedMessage("Encryption not enabled"); + } else { + res = await crypto.decryptEvent(this); + } + } catch (e) { + if (e.name !== "DecryptionError") { + // not a decryption error: log the whole exception as an error + // (and don't bother with a retry) + console.error( + `Error decrypting event (id=${this.getId()}): ${e.stack || e}`, + ); + this._decrypting = false; + this._retryDecryption = false; + return; + } + + // see if we have a retry queued. + // + // NB: make sure to keep this check in the same tick of the + // event loop as `_decrypting = false` below - otherwise we + // risk a race: + // + // * A: we check _retryDecryption here and see that it is + // false + // * B: we get a second call to attemptDecryption, which sees + // that _decrypting is true so sets _retryDecryption + // * A: we continue below, clear _decrypting, and never do + // the retry. + // + if (this._retryDecryption) { + // decryption error, but we have a retry queued. + console.log( + `Got error decrypting event (id=${this.getId()}: ` + + `${e.message}), but retrying`, + ); + continue; + } + // decryption error, no retries queued. Warn about the error and // set it to m.bad.encrypted. console.warn( `Error decrypting event (id=${this.getId()}): ${e}`, ); - this._badEncryptedMessage(e.message); - return null; + res = this._badEncryptedMessage(e.message); } - }); + + // at this point, we've either successfully decrypted the event, or have given up + // (and set res to a 'badEncryptedMessage'). Either way, we can now set the + // cleartext of the event and raise Event.decrypted. + // + // make sure we clear '_decrypting' before sending the 'Event.decrypted' event, + // otherwise the app will be confused to see `isBeingDecrypted` still set when + // there isn't an `Event.decrypted` on the way. + // + // see also notes on _retryDecryption above. + // + this._decrypting = false; + this._retryDecryption = false; + this._setClearData(res); + return; + } }, _badEncryptedMessage: function(reason) { - this.setClearData({ - type: "m.room.message", - content: { - msgtype: "m.bad.encrypted", - body: "** Unable to decrypt: " + reason + " **", + return { + clearEvent: { + type: "m.room.message", + content: { + msgtype: "m.bad.encrypted", + body: "** Unable to decrypt: " + reason + " **", + }, }, - }); + }; }, /** @@ -412,29 +450,17 @@ utils.extend(module.exports.MatrixEvent.prototype, { * * @fires module:models/event.MatrixEvent#"Event.decrypted" * - * @param {Object} clearEvent The plaintext payload for the event - * (typically containing type and content fields). - * - * @param {string=} senderCurve25519Key Key owned by the sender of this event. - * See {@link module:models/event.MatrixEvent#getSenderKey}. - * - * @param {string=} claimedEd25519Key ed25519 key claimed by the sender of - * this event. See {@link module:models/event.MatrixEvent#getClaimedEd25519Key}. - * - * @param {Array=} forwardingCurve25519KeyChain list of curve25519 keys - * involved in telling us about the senderCurve25519Key and claimedEd25519Key. - * See {@link module:models/event.MatrixEvent#getForwardingCurve25519KeyChain}. + * @param {module:crypto~EventDecryptionResult} decryptionResult + * the decryption result, including the plaintext and some key info */ - setClearData: function( - clearEvent, - senderCurve25519Key, - claimedEd25519Key, - forwardingCurve25519KeyChain, - ) { - this._clearEvent = clearEvent; - this._senderCurve25519Key = senderCurve25519Key || null; - this._claimedEd25519Key = claimedEd25519Key || null; - this._forwardingCurve25519KeyChain = forwardingCurve25519KeyChain || []; + _setClearData: function(decryptionResult) { + this._clearEvent = decryptionResult.clearEvent; + this._senderCurve25519Key = + decryptionResult.senderCurve25519Key || null; + this._claimedEd25519Key = + decryptionResult.claimedEd25519Key || null; + this._forwardingCurve25519KeyChain = + decryptionResult.forwardingCurve25519KeyChain || []; this.emit("Event.decrypted", this); },