Refactor decryption

Create the MatrixEvent wrapper before decryption, and then pass that into the
decryptors, which should update it.

Also remove the workaround that sends m.new_device messages when we get an
unknown session; it's just a bandaid which is obscuring more meaningful
problems.

lib/client.js

@@ -463,38 +463,31 @@ MatrixClient.prototype.isRoomEncrypted = function(roomId) {
  * Decrypt a received event according to the algorithm specified in the event.
  *
  * @param {MatrixClient} client
- * @param {object} raw event
+ * @param {MatrixEvent} event
- *
- * @return {MatrixEvent}
  */
 function _decryptEvent(client, event) {
     if (!client._crypto) {
-        return _badEncryptedMessage(event, "**Encryption not enabled**");
+        _badEncryptedMessage(event, "**Encryption not enabled**");
+        return;
     }
 
-    var decryptionResult;
     try {
-        decryptionResult = client._crypto.decryptEvent(event);
+        client._crypto.decryptEvent(event);
     } catch (e) {
         if (!(e instanceof Crypto.DecryptionError)) {
             throw e;
         }
-        return _badEncryptedMessage(event, "**" + e.message + "**");
+        _badEncryptedMessage(event, "**" + e.message + "**");
+        return;
     }
-    return new MatrixEvent(
-        event, decryptionResult.payload,
-        decryptionResult.keysProved,
-        decryptionResult.keysClaimed
-    );
 }
 
 function _badEncryptedMessage(event, reason) {
-    return new MatrixEvent(event, {
+    event.setClearData({
         type: "m.room.message",
         content: {
             msgtype: "m.bad.encrypted",
             body: reason,
-            content: event.content,
         },
     });
 }
@@ -2829,10 +2822,11 @@ function _resolve(callback, defer, res) {
 
 function _PojoToMatrixEventMapper(client) {
     function mapper(plainOldJsObject) {
-        if (plainOldJsObject.type === "m.room.encrypted") {
+        var event = new MatrixEvent(plainOldJsObject);
-            return _decryptEvent(client, plainOldJsObject);
+        if (event.isEncrypted()) {
+            _decryptEvent(client, event);
         }
-        return new MatrixEvent(plainOldJsObject);
+        return event;
     }
     return mapper;
 }

lib/crypto/algorithms/megolm.js

@@ -451,7 +451,7 @@ utils.inherits(MegolmDecryption, base.DecryptionAlgorithm);
 /**
  * @inheritdoc
  *
- * @param {object} event raw event
+ * @param {MatrixEvent} event
  *
  * @return {null} The event referred to an unknown megolm session
  * @return {module:crypto.DecryptionResult} decryption result
@@ -460,7 +460,7 @@ utils.inherits(MegolmDecryption, base.DecryptionAlgorithm);
  *   problem decrypting the event
  */
 MegolmDecryption.prototype.decryptEvent = function(event) {
-    var content = event.content;
+    var content = event.getWireContent();
 
     if (!content.sender_key || !content.session_id ||
         !content.ciphertext
@@ -471,14 +471,15 @@ MegolmDecryption.prototype.decryptEvent = function(event) {
     var res;
     try {
         res = this._olmDevice.decryptGroupMessage(
-            event.room_id, content.sender_key, content.session_id, content.ciphertext
+            event.getRoomId(), content.sender_key, content.session_id, content.ciphertext
         );
     } catch (e) {
         throw new base.DecryptionError(e);
     }
 
     if (res === null) {
-        return null;
+        // We've got a message for a session we don't have.
+        throw new base.DecryptionError("Unknown inbound session id");
     }
 
     var payload = JSON.parse(res.result);
@@ -486,17 +487,13 @@ MegolmDecryption.prototype.decryptEvent = function(event) {
     // belt-and-braces check that the room id matches that indicated by the HS
     // (this is somewhat redundant, since the megolm session is scoped to the
     // room, so neither the sender nor a MITM can lie about the room_id).
-    if (payload.room_id !== event.room_id) {
+    if (payload.room_id !== event.getRoomId()) {
         throw new base.DecryptionError(
             "Message intended for room " + payload.room_id
         );
     }
 
-    return {
+    event.setClearData(payload, res.keysClaimed, res.keysProved);
-        payload: payload,
-        keysClaimed: res.keysClaimed,
-        keysProved: res.keysProved,
-    };
 };
 
 /**

lib/crypto/algorithms/olm.js

@@ -151,15 +151,13 @@ utils.inherits(OlmDecryption, base.DecryptionAlgorithm);
 /**
  * @inheritdoc
  *
- * @param {object} event raw event
+ * @param {MatrixEvent} event
- *
- * @return {module:crypto.DecryptionResult} decryption result
  *
  * @throws {module:crypto/algorithms/base.DecryptionError} if there is a
  *   problem decrypting the event
  */
 OlmDecryption.prototype.decryptEvent = function(event) {
-    var content = event.content;
+    var content = event.getWireContent();
     var deviceKey = content.sender_key;
     var ciphertext = content.ciphertext;
 
@@ -178,7 +176,7 @@ OlmDecryption.prototype.decryptEvent = function(event) {
     } catch (e) {
         console.warn(
             "Failed to decrypt Olm event (id=" +
-                event.event_id + ") from " + deviceKey +
+                event.getId() + ") from " + deviceKey +
                 ": " + e.message
         );
         throw new base.DecryptionError("Bad Encrypted Message");
@@ -192,11 +190,11 @@ OlmDecryption.prototype.decryptEvent = function(event) {
         // older versions of riot did not set this field, so we cannot make
         // this check. TODO: kill this off once our users have updated
         console.warn(
-            "Olm event (id=" + event.event_id + ") contains no 'recipient' " +
+            "Olm event (id=" + event.getId() + ") contains no 'recipient' " +
              "property; cannot prevent unknown-key attack");
     } else if (payload.recipient != this._userId) {
         console.warn(
-            "Event " + event.event_id + ": Intended recipient " +
+            "Event " + event.getId() + ": Intended recipient " +
             payload.recipient + " does not match our id " + this._userId
         );
         throw new base.DecryptionError(
@@ -207,12 +205,12 @@ OlmDecryption.prototype.decryptEvent = function(event) {
     if (payload.recipient_keys === undefined) {
         // ditto
         console.warn(
-            "Olm event (id=" + event.event_id + ") contains no " +
+            "Olm event (id=" + event.getId() + ") contains no " +
             "'recipient_keys' property; cannot prevent unknown-key attack");
     } else if (payload.recipient_keys.ed25519 !=
                this._olmDevice.deviceEd25519Key) {
         console.warn(
-            "Event " + event.event_id + ": Intended recipient ed25519 key " +
+            "Event " + event.getId() + ": Intended recipient ed25519 key " +
             payload.recipient_keys.ed25519 + " did not match ours"
         );
         throw new base.DecryptionError("Message not intended for this device");
@@ -225,12 +223,12 @@ OlmDecryption.prototype.decryptEvent = function(event) {
     if (payload.sender === undefined) {
         // ditto
         console.warn(
-            "Olm event (id=" + event.event_id + ") contains no " +
+            "Olm event (id=" + event.getId() + ") contains no " +
             "'sender' property; cannot prevent unknown-key attack");
-    } else if (payload.sender != event.sender) {
+    } else if (payload.sender != event.getSender()) {
         console.warn(
-            "Event " + event.event_id + ": original sender " + payload.sender +
+            "Event " + event.getId() + ": original sender " + payload.sender +
-            " does not match reported sender " + event.sender
+            " does not match reported sender " + event.getSender()
         );
         throw new base.DecryptionError(
             "Message forwarded from " + payload.sender
@@ -238,9 +236,9 @@ OlmDecryption.prototype.decryptEvent = function(event) {
     }
 
     // Olm events intended for a room have a room_id.
-    if (payload.room_id !== event.room_id) {
+    if (payload.room_id !== event.getRoomId()) {
         console.warn(
-            "Event " + event.event_id + ": original room " + payload.room_id +
+            "Event " + event.getId() + ": original room " + payload.room_id +
             " does not match reported room " + event.room_id
         );
         throw new base.DecryptionError(
@@ -248,12 +246,7 @@ OlmDecryption.prototype.decryptEvent = function(event) {
         );
     }
 
-    return {
+    event.setClearData(payload, {curve25519: deviceKey}, payload.keys || {});
-        payload: payload,
-        sessionExists: true,
-        keysProved: {curve25519: deviceKey},
-        keysClaimed: payload.keys || {}
-    };
 };
 
 

lib/crypto/index.js

@@ -900,63 +900,17 @@ Crypto.prototype.encryptEventIfNeeded = function(event, room) {
     });
 };
 
-/**
- * @typedef {Object} module:crypto.DecryptionResult
- *
- * @property {Object} payload decrypted payload (with properties 'type',
- *     'content').
- *
- * @property {Object<string, string>} keysClaimed keys that the sender of the
- *     event claims ownership of: map from key type to base64-encoded key
- *
- * @property {Object<string, string>} keysProved keys that the sender of the
- *     event is known to have ownership of: map from key type to base64-encoded
- *     key
- */
-
 /**
  * Decrypt a received event
  *
- * @param {object} event raw event
+ * @param {MatrixEvent} event
- *
- * @return {module:crypto.DecryptionResult} decryption result
  *
  * @raises {algorithms.DecryptionError} if there is a problem decrypting the event
  */
 Crypto.prototype.decryptEvent = function(event) {
-    var content = event.content;
+    var content = event.getWireContent();
-    var alg = this._getRoomDecryptor(event.room_id, content.algorithm);
+    var alg = this._getRoomDecryptor(event.getRoomId(), content.algorithm);
-    var r = alg.decryptEvent(event);
+    alg.decryptEvent(event);
-
-    if (r !== null) {
-        return r;
-    } else {
-        // We've got a message for a session we don't have.  Maybe the sender
-        // forgot to tell us about the session.  Remind the sender that we
-        // exist so that they might tell us about the session on their next
-        // send.
-        //
-        // (Alternatively, it might be that we are just looking at
-        // scrollback... at least we rate-limit the m.new_device events :/)
-        //
-        // XXX: this is a band-aid which masks symptoms of other bugs. It would
-        // be nice to get rid of it.
-        if (event.room_id !== undefined && event.sender !== undefined) {
-            var device_id = event.content.device_id;
-            if (device_id === undefined) {
-                // if the sending device didn't tell us its device_id, fall
-                // back to all devices.
-                device_id = null;
-            }
-            console.log(
-                "Unknown session decrypting event id " + event.event_id +
-                    ": sending m.new_device event"
-            );
-            this._sendPingToDevice(event.sender, device_id, event.room_id);
-        }
-
-        throw new algorithms.DecryptionError("Unknown inbound session id");
-    }
 };
 
 /**

lib/models/event.js

@@ -51,15 +51,6 @@ module.exports.EventStatus = {
  *
  * @param {Object} event The raw event to be wrapped in this DAO
  *
- * @param {Object=} clearEvent For encrypted events, the plaintext payload for
- * the event (typically containing <tt>type</tt> and <tt>content</tt> fields).
- *
- * @param {Object=} keysProved Keys owned by the sender of this event.
- *    See {@link module:models/event.MatrixEvent#getKeysProved}.
- *
- * @param {Object=} keysClaimed Keys the sender of this event claims.
- *    See {@link module:models/event.MatrixEvent#getKeysClaimed}.
- *
  * @prop {Object} event The raw (possibly encrypted) event. <b>Do not access
  * this property</b> directly unless you absolutely have to. Prefer the getter
  * methods defined on this class. Using the getter methods shields your app
@@ -75,19 +66,18 @@ module.exports.EventStatus = {
  * Default: true. <strong>This property is experimental and may change.</strong>
  */
 module.exports.MatrixEvent = function MatrixEvent(
-    event, clearEvent, keysProved, keysClaimed
+    event
 ) {
     this.event = event || {};
     this.sender = null;
     this.target = null;
     this.status = null;
     this.forwardLooking = true;
-
-    this._clearEvent = clearEvent || {};
     this._pushActions = null;
 
-    this._keysProved = keysProved || {};
+    this._clearEvent = {};
-    this._keysClaimed = keysClaimed || {};
+    this._keysProved = {};
+    this._keysClaimed = {};
 };
 
 module.exports.MatrixEvent.prototype = {
@@ -239,12 +229,36 @@ module.exports.MatrixEvent.prototype = {
         this._keysClaimed = keys;
     },
 
+    /**
+     * Update the cleartext data on this event.
+     *
+     * (This is used after decrypting an event; it should not be used by applications).
+     *
+     * @internal
+     *
+     * @fires module:models/event.MatrixEvent#"Event.decrypted"
+     *
+     * @param {Object} clearEvent The plaintext payload for the event
+     *     (typically containing <tt>type</tt> and <tt>content</tt> fields).
+     *
+     * @param {Object=} keysProved Keys owned by the sender of this event.
+     *    See {@link module:models/event.MatrixEvent#getKeysProved}.
+     *
+     * @param {Object=} keysClaimed Keys the sender of this event claims.
+     *    See {@link module:models/event.MatrixEvent#getKeysClaimed}.
+     */
+    setClearData: function(clearEvent, keysProved, keysClaimed) {
+        this._clearEvent = clearEvent;
+        this._keysProved = keysProved || {};
+        this._keysClaimed = keysClaimed || {};
+    },
+
     /**
      * Check if the event is encrypted.
      * @return {boolean} True if this event is encrypted.
      */
     isEncrypted: function() {
-        return Boolean(this._clearEvent.type);
+        return this.event.type === "m.room.encrypted";
     },
 
     /**