Make crypto.decryptMessage return decryption results

... instead of having it call event.setClearData.

The main advantage of this is that it fixes a race condition, wherein apps
could see `event.isDecrypting()` to be true, but in fact the event had been
decrypted (and there was no `Event.decrypted` event on its way).

We're also fixing another race, wherein if the first attempt to decrypt failed,
a call to `attemptDecryption` would race against the first call and a second
attempt to decrypt would never happen.

This also gives a cleaner interface to MatrixEvent, at the expense of making
the `megolm` unit test a bit more hoop-jumpy.

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({});
+            // construct a fake decrypted key event via the use of a mocked
-            event.setClearData(
+            // '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,12 +86,20 @@ describe("MegolmDecryption", function() {
                         session_key: groupSession.session_key(),
                     },
                 },
-                "SENDER_CURVE25519",
+                senderCurve25519Key: "SENDER_CURVE25519",
-                "SENDER_ED25519",
+                claimedEd25519Key: "SENDER_ED25519",
-            );
+            };
 
+            const mockCrypto = {
+                decryptEvent: function() {
+                    return Promise.resolve(decryptedData);
+                },
+            };
+
+            return event.attemptDecryption(mockCrypto).then(() => {
                 megolmDecryption.onRoomKeyEvent(event);
             });
+        });
 
         it('can decrypt an event', function() {
             const event = new MatrixEvent({
@@ -104,8 +116,8 @@ describe("MegolmDecryption", function() {
                 },
             });
 
-            return megolmDecryption.decryptEvent(event).then(() => {
+            return megolmDecryption.decryptEvent(event).then((res) => {
-                expect(event.getContent()).toEqual('testytest');
+                expect(res.clearEvent.content).toEqual('testytest');
             });
         });
 

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');
+            });
+        });
+    });
+});

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<module:crypto~EventDecryptionResult>} promise which
+     * resolves once we have finished decrypting. Rejects with an
      * `algorithms.DecryptionError` if there is a problem decrypting the event.
      */
 

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
+ * returns a promise which resolves to a
- * rejects with an `algorithms.DecryptionError` if there is a problem
+ * {@link module:crypto~EventDecryptionResult} once we have finished
- * decrypting the event.
+ * 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,
+    return {
-        res.forwardingCurve25519KeyChain);
+        clearEvent: payload,
+        senderCurve25519Key: res.senderKey,
+        claimedEd25519Key: res.keysClaimed.ed25519,
+        forwardingCurve25519KeyChain: res.forwardingCurve25519KeyChain,
+    };
 };
 
 MegolmDecryption.prototype._requestKeysForEvent = function(event) {

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
+ * returns a promise which resolves to a
- * `algorithms.DecryptionError` if there is a problem decrypting the event.
+ * {@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

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
+ * @return {Promise<module:crypto~EventDecryptionResult>} resolves once we have
- * `algorithms.DecryptionError` if there is a problem decrypting the event.
+ *  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 <tt>type</tt> and <tt>content</tt> 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<string>?} 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
  *

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) {
+    attemptDecryption: async function(crypto) {
-        if (!crypto) {
+        // start with a couple of sanity checks.
-            this._badEncryptedMessage("Encryption not enabled");
-            return;
-        }
-
         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;
-
+        while (true) {
-        this._doDecryption(crypto).finally(() => {
-            this._decrypting = false;
             this._retryDecryption = false;
-        });
-    },
 
-    _doDecryption: function(crypto) {
+            let res;
-        return Promise.try(() => {
+            try {
-            return crypto.decryptEvent(this);
+                if (!crypto) {
-        }).catch((e) => {
+                    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.
+                    // 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}`,
                     );
-                return null;
+                    this._decrypting = false;
-            } else if (this._retryDecryption) {
+                    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()}), but retrying`,
+                        `Got error decrypting event (id=${this.getId()}: ` +
+                        `${e.message}), but retrying`,
                     );
-                this._retryDecryption = false;
+                    continue;
-                return this._doDecryption(crypto);
+                }
-            } else {
+
                 // 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);
+                res = this._badEncryptedMessage(e.message);
-                return null;
+            }
+
+            // 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({
+        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
+     * @param {module:crypto~EventDecryptionResult} decryptionResult
-     *     (typically containing <tt>type</tt> and <tt>content</tt> fields).
+     *     the decryption result, including the plaintext and some key info
-     *
-     * @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<string>=} forwardingCurve25519KeyChain list of curve25519 keys
-     *     involved in telling us about the senderCurve25519Key and claimedEd25519Key.
-     *     See {@link module:models/event.MatrixEvent#getForwardingCurve25519KeyChain}.
      */
-    setClearData: function(
+    _setClearData: function(decryptionResult) {
-        clearEvent,
+        this._clearEvent = decryptionResult.clearEvent;
-        senderCurve25519Key,
+        this._senderCurve25519Key =
-        claimedEd25519Key,
+            decryptionResult.senderCurve25519Key || null;
-        forwardingCurve25519KeyChain,
+        this._claimedEd25519Key =
-    ) {
+            decryptionResult.claimedEd25519Key || null;
-        this._clearEvent = clearEvent;
+        this._forwardingCurve25519KeyChain =
-        this._senderCurve25519Key = senderCurve25519Key || null;
+            decryptionResult.forwardingCurve25519KeyChain || [];
-        this._claimedEd25519Key = claimedEd25519Key || null;
-        this._forwardingCurve25519KeyChain = forwardingCurve25519KeyChain || [];
         this.emit("Event.decrypted", this);
     },