Merge pull request #850 from uhoreg/ensure_key_request

make sure key requests get sent

spec/unit/crypto.spec.js

@@ -139,6 +139,11 @@ describe("Crypto", function() {
             await bobClient.initCrypto();
         });
 
+        afterEach(async function() {
+            aliceClient.stopClient();
+            bobClient.stopClient();
+        });
+
         it(
             "does not cancel keyshare requests if some messages are not decrypted",
             async function() {
@@ -266,10 +271,31 @@ describe("Crypto", function() {
                 // the room key request should be gone since we've now decypted everything
                 expect(await cryptoStore.getOutgoingRoomKeyRequest(roomKeyRequestBody))
                     .toNotExist();
-
-                aliceClient.stopClient();
-                bobClient.stopClient();
             },
         );
+
+        it("creates a new keyshare request if we request a keyshare", async function() {
+            // make sure that cancelAndResend... creates a new keyshare request
+            // if there wasn't an already-existing one
+            const event = new MatrixEvent({
+                sender: "@bob:example.com",
+                room_id: "!someroom",
+                content: {
+                    algorithm: olmlib.MEGOLM_ALGORITHM,
+                    session_id: "sessionid",
+                    sender_key: "senderkey",
+                },
+            });
+            await aliceClient.cancelAndResendEventRoomKeyRequest(event);
+            const cryptoStore = aliceClient._cryptoStore;
+            const roomKeyRequestBody = {
+                algorithm: olmlib.MEGOLM_ALGORITHM,
+                room_id: "!someroom",
+                session_id: "sessionid",
+                sender_key: "senderkey",
+            };
+            expect(await cryptoStore.getOutgoingRoomKeyRequest(roomKeyRequestBody))
+                .toExist();
+        });
     });
 });

src/client.js

@@ -760,9 +760,10 @@ MatrixClient.prototype.isEventSenderVerified = async function(event) {
  * request.
  * @param  {MatrixEvent} event event of which to cancel and resend the room
  *                            key request.
+ * @return {Promise} A promise that will resolve when the key request is queued
  */
 MatrixClient.prototype.cancelAndResendEventRoomKeyRequest = function(event) {
-    event.cancelAndResendKeyRequest(this._crypto);
+    return event.cancelAndResendKeyRequest(this._crypto, this.getUserId());
 };
 
 /**

src/crypto/OutgoingRoomKeyRequestManager.js

@@ -124,35 +124,115 @@ export default class OutgoingRoomKeyRequestManager {
      *
      * @param {module:crypto~RoomKeyRequestBody} requestBody
      * @param {Array<{userId: string, deviceId: string}>} recipients
+     * @param {boolean} resend whether to resend the key request if there is
+     *    already one
      *
      * @returns {Promise} resolves when the request has been added to the
      *    pending list (or we have established that a similar request already
      *    exists)
      */
-    sendRoomKeyRequest(requestBody, recipients) {
-        return this._cryptoStore.getOrAddOutgoingRoomKeyRequest({
-            requestBody: requestBody,
-            recipients: recipients,
-            requestId: this._baseApis.makeTxnId(),
-            state: ROOM_KEY_REQUEST_STATES.UNSENT,
-        }).then((req) => {
-            if (req.state === ROOM_KEY_REQUEST_STATES.UNSENT) {
-                this._startTimer();
+    async sendRoomKeyRequest(requestBody, recipients, resend=false) {
+        const req = await this._cryptoStore.getOutgoingRoomKeyRequest(
+            requestBody,
+        );
+        if (!req) {
+            await this._cryptoStore.getOrAddOutgoingRoomKeyRequest({
+                requestBody: requestBody,
+                recipients: recipients,
+                requestId: this._baseApis.makeTxnId(),
+                state: ROOM_KEY_REQUEST_STATES.UNSENT,
+            });
+        } else {
+            switch (req.state) {
+            case ROOM_KEY_REQUEST_STATES.CANCELLATION_PENDING_AND_WILL_RESEND:
+            case ROOM_KEY_REQUEST_STATES.UNSENT:
+                // nothing to do here, since we're going to send a request anyways
+                return;
+
+            case ROOM_KEY_REQUEST_STATES.CANCELLATION_PENDING: {
+                // existing request is about to be cancelled.  If we want to
+                // resend, then change the state so that it resends after
+                // cancelling.  Otherwise, just cancel the cancellation.
+                const state = resend ?
+                    ROOM_KEY_REQUEST_STATES.CANCELLATION_PENDING_AND_WILL_RESEND :
+                    ROOM_KEY_REQUEST_STATES.SENT;
+                await this._cryptoStore.updateOutgoingRoomKeyRequest(
+                    req.requestId, ROOM_KEY_REQUEST_STATES.CANCELLATION_PENDING, {
+                        state,
+                        cancellationTxnId: this._baseApis.makeTxnId(),
+                    },
+                );
+                break;
             }
-        });
+            case ROOM_KEY_REQUEST_STATES.SENT: {
+                // a request has already been sent.  If we don't want to
+                // resend, then do nothing.  If we do want to, then cancel the
+                // existing request and send a new one.
+                if (resend) {
+                    const state =
+                          ROOM_KEY_REQUEST_STATES.CANCELLATION_PENDING_AND_WILLRESEND;
+                    const updatedReq =
+                          await this._cryptoStore.updateOutgoingRoomKeyRequest(
+                              req.requestId, ROOM_KEY_REQUEST_STATES.SENT, {
+                                  state,
+                                  cancellationTxnId: this._baseApis.makeTxnId(),
+                              },
+                          );
+                    if (!updatedReq) {
+                        // updateOutgoingRoomKeyRequest couldn't find the request
+                        // in state ROOM_KEY_REQUEST_STATES.SENT, so we must have
+                        // raced with another tab to mark the request cancelled.
+                        // Try again, to make sure the request is resent.
+                        return await this.sendRoomKeyRequest(
+                            requestBody, recipients, resend,
+                        );
+                    }
+
+                    // We don't want to wait for the timer, so we send it
+                    // immediately. (We might actually end up racing with the timer,
+                    // but that's ok: even if we make the request twice, we'll do it
+                    // with the same transaction_id, so only one message will get
+                    // sent).
+                    //
+                    // (We also don't want to wait for the response from the server
+                    // here, as it will slow down processing of received keys if we
+                    // do.)
+                    try {
+                        await this._sendOutgoingRoomKeyRequestCancellation(
+                            updatedReq,
+                            true,
+                        );
+                    } catch (e) {
+                        logger.error(
+                            "Error sending room key request cancellation;"
+                                + " will retry later.", e,
+                        );
+                    }
+                    // The request has transitioned from
+                    // CANCELLATION_PENDING_AND_WILL_RESEND to UNSENT. We
+                    // still need to resend the request which is now UNSENT, so
+                    // start the timer if it isn't already started.
+                }
+                break;
+            }
+            default:
+                throw new Error('unhandled state: ' + req.state);
+            }
+        }
+        // some of the branches require the timer to be started.  Just start it
+        // all the time, because it doesn't hurt to start it.
+        this._startTimer();
     }
 
     /**
      * Cancel room key requests, if any match the given requestBody
      *
      * @param {module:crypto~RoomKeyRequestBody} requestBody
-     * @param {boolean} andResend if true, transition to UNSENT instead of
-     *                            deleting after sending cancellation.
      *
      * @returns {Promise} resolves when the request has been updated in our
      *    pending list.
      */
-    cancelRoomKeyRequest(requestBody, andResend=false) {
+    cancelRoomKeyRequest(requestBody) {
         return this._cryptoStore.getOutgoingRoomKeyRequest(
             requestBody,
         ).then((req) => {
@@ -183,15 +263,10 @@ export default class OutgoingRoomKeyRequestManager {
                     );
 
                 case ROOM_KEY_REQUEST_STATES.SENT: {
-                    // If `andResend` is set, transition to UNSENT once the
-                    // cancellation has successfully been sent.
-                    const state = andResend ?
-                        ROOM_KEY_REQUEST_STATES.CANCELLATION_PENDING_AND_WILL_RESEND :
-                        ROOM_KEY_REQUEST_STATES.CANCELLATION_PENDING;
                     // send a cancellation.
                     return this._cryptoStore.updateOutgoingRoomKeyRequest(
                         req.requestId, ROOM_KEY_REQUEST_STATES.SENT, {
-                            state,
+                            state: ROOM_KEY_REQUEST_STATES.CANCELLATION_PENDING,
                             cancellationTxnId: this._baseApis.makeTxnId(),
                         },
                     ).then((updatedReq) => {
@@ -221,20 +296,12 @@ export default class OutgoingRoomKeyRequestManager {
                         // do.)
                         this._sendOutgoingRoomKeyRequestCancellation(
                             updatedReq,
-                            andResend,
                         ).catch((e) => {
                             logger.error(
                                 "Error sending room key request cancellation;"
                                 + " will retry later.", e,
                             );
                             this._startTimer();
-                        }).then(() => {
-                            if (!andResend) return;
-                            // The request has transitioned from
-                            // CANCELLATION_PENDING_AND_WILL_RESEND to UNSENT. We
-                            // still need to resend the request which is now UNSENT, so
-                            // start the timer if it isn't already started.
-                            this._startTimer();
                         });
                     });
                 }

src/crypto/algorithms/megolm.js

@@ -815,19 +815,9 @@ MegolmDecryption.prototype.decryptEvent = async function(event) {
 };
 
 MegolmDecryption.prototype._requestKeysForEvent = function(event) {
-    const sender = event.getSender();
     const wireContent = event.getWireContent();
 
-    // send the request to all of our own devices, and the
-    // original sending device if it wasn't us.
-    const recipients = [{
-        userId: this._userId, deviceId: '*',
-    }];
-    if (sender != this._userId) {
-        recipients.push({
-            userId: sender, deviceId: wireContent.device_id,
-        });
-    }
+    const recipients = event.getKeyRequestRecipients(this._userId);
 
     this._crypto.requestRoomKey({
         room_id: event.getRoomId(),

src/crypto/index.js

@@ -1426,10 +1426,14 @@ Crypto.prototype.handleDeviceListChanges = async function(syncData, syncDeviceLi
  *
  * @param {module:crypto~RoomKeyRequestBody} requestBody
  * @param {Array<{userId: string, deviceId: string}>} recipients
+ * @param {boolean} resend whether to resend the key request if there is
+ *    already one
+ *
+ * @return {Promise} a promise that resolves when the key request is queued
  */
-Crypto.prototype.requestRoomKey = function(requestBody, recipients) {
-    this._outgoingRoomKeyRequestManager.sendRoomKeyRequest(
-        requestBody, recipients,
+Crypto.prototype.requestRoomKey = function(requestBody, recipients, resend=false) {
+    return this._outgoingRoomKeyRequestManager.sendRoomKeyRequest(
+        requestBody, recipients, resend,
     ).catch((e) => {
         // this normally means we couldn't talk to the store
         logger.error(
@@ -1443,11 +1447,9 @@ Crypto.prototype.requestRoomKey = function(requestBody, recipients) {
  *
  * @param {module:crypto~RoomKeyRequestBody} requestBody
  *    parameters to match for cancellation
- * @param {boolean} andResend
- *    if true, resend the key request after cancelling.
  */
-Crypto.prototype.cancelRoomKeyRequest = function(requestBody, andResend) {
-    this._outgoingRoomKeyRequestManager.cancelRoomKeyRequest(requestBody, andResend)
+Crypto.prototype.cancelRoomKeyRequest = function(requestBody) {
+    this._outgoingRoomKeyRequestManager.cancelRoomKeyRequest(requestBody)
     .catch((e) => {
         logger.warn("Error clearing pending room key requests", e);
     }).done();
@@ -1984,7 +1986,7 @@ Crypto.prototype._onToDeviceBadEncrypted = async function(event) {
             sender, device.deviceId,
         );
     for (const keyReq of requestsToResend) {
-        this.cancelRoomKeyRequest(keyReq.requestBody, true);
+        this.requestRoomKey(keyReq.requestBody, keyReq.recipients, true);
     }
 };
 

src/models/event.js

@@ -382,15 +382,41 @@ utils.extend(module.exports.MatrixEvent.prototype, {
      * Cancel any room key request for this event and resend another.
      *
      * @param {module:crypto} crypto crypto module
+     * @param {string} userId the user who received this event
+     *
+     * @returns {Promise} a promise that resolves when the request is queued
      */
-    cancelAndResendKeyRequest: function(crypto) {
+    cancelAndResendKeyRequest: function(crypto, userId) {
         const wireContent = this.getWireContent();
-        crypto.cancelRoomKeyRequest({
+        return crypto.requestRoomKey({
             algorithm: wireContent.algorithm,
             room_id: this.getRoomId(),
             session_id: wireContent.session_id,
             sender_key: wireContent.sender_key,
-        }, true);
+        }, this.getKeyRequestRecipients(userId), true);
+    },
+
+    /**
+     * Calculate the recipients for keyshare requests.
+     *
+     * @param {string} userId the user who received this event.
+     *
+     * @returns {Array} array of recipients
+     */
+    getKeyRequestRecipients: function(userId) {
+        // send the request to all of our own devices, and the
+        // original sending device if it wasn't us.
+        const wireContent = this.getWireContent();
+        const recipients = [{
+            userId, deviceId: '*',
+        }];
+        const sender = this.getSender();
+        if (sender !== userId) {
+            recipients.push({
+                userId: sender, deviceId: wireContent.device_id,
+            });
+        }
+        return recipients;
     },
 
     _decryptionLoop: async function(crypto) {