From 98fdcabc00dfa2387bf44c09a6e173dc5c5ffc17 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Mon, 4 Mar 2019 16:59:54 -0500 Subject: [PATCH 1/3] stop client after each test --- spec/unit/crypto.spec.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/spec/unit/crypto.spec.js b/spec/unit/crypto.spec.js index de2893090d5..1dbbe865392 100644 --- a/spec/unit/crypto.spec.js +++ b/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,9 +271,6 @@ 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(); }, ); }); From 5480e8e1d572140e01dd52d1a214ef38d675f5f0 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Mon, 4 Mar 2019 17:09:56 -0500 Subject: [PATCH 2/3] refactor key sharing requests use sendRoomKeyRequest with a new resend flag, instead of cancelRoomKeyRequest, when requesting keys, so that we make sure that we send a new request if there is no previous request fixes https://github.com/vector-im/riot-web/issues/6838 --- spec/unit/crypto.spec.js | 24 ++++ src/client.js | 3 +- src/crypto/OutgoingRoomKeyRequestManager.js | 121 +++++++++++++++----- src/crypto/algorithms/megolm.js | 12 +- src/crypto/index.js | 18 +-- src/models/event.js | 30 ++++- 6 files changed, 158 insertions(+), 50 deletions(-) diff --git a/spec/unit/crypto.spec.js b/spec/unit/crypto.spec.js index 1dbbe865392..0440456d51f 100644 --- a/spec/unit/crypto.spec.js +++ b/spec/unit/crypto.spec.js @@ -273,5 +273,29 @@ describe("Crypto", function() { .toNotExist(); }, ); + + 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(); + }); }); }); diff --git a/src/client.js b/src/client.js index a472d17b7a1..fd4820be9b4 100644 --- a/src/client.js +++ b/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()); }; /** diff --git a/src/crypto/OutgoingRoomKeyRequestManager.js b/src/crypto/OutgoingRoomKeyRequestManager.js index bfde6019ba0..1b51ab230a2 100644 --- a/src/crypto/OutgoingRoomKeyRequestManager.js +++ b/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(); }); }); } diff --git a/src/crypto/algorithms/megolm.js b/src/crypto/algorithms/megolm.js index a123fe91661..5a5327fa5ea 100644 --- a/src/crypto/algorithms/megolm.js +++ b/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(), diff --git a/src/crypto/index.js b/src/crypto/index.js index 4674cdd6b6d..dfb67c41599 100644 --- a/src/crypto/index.js +++ b/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); } }; diff --git a/src/models/event.js b/src/models/event.js index 823ced97fdc..1d90439eafd 100644 --- a/src/models/event.js +++ b/src/models/event.js @@ -382,15 +382,39 @@ 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 */ - 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) { From 055ce673cd0af07db050f6458eada3f8bfb864c5 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Tue, 5 Mar 2019 10:54:02 -0500 Subject: [PATCH 3/3] fix jsdoc --- src/crypto/index.js | 2 +- src/models/event.js | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/crypto/index.js b/src/crypto/index.js index dfb67c41599..4b39141e3f0 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -1429,7 +1429,7 @@ Crypto.prototype.handleDeviceListChanges = async function(syncData, syncDeviceLi * @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 + * @return {Promise} a promise that resolves when the key request is queued */ Crypto.prototype.requestRoomKey = function(requestBody, recipients, resend=false) { return this._outgoingRoomKeyRequestManager.sendRoomKeyRequest( diff --git a/src/models/event.js b/src/models/event.js index 1d90439eafd..2cd90cf632a 100644 --- a/src/models/event.js +++ b/src/models/event.js @@ -383,6 +383,8 @@ utils.extend(module.exports.MatrixEvent.prototype, { * * @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, userId) { const wireContent = this.getWireContent();