From 882d3a765d2d98b4357d10103ca76dfdb521a7a1 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Tue, 10 Dec 2019 11:04:47 +0100 Subject: [PATCH 01/43] support .ready event in VerificationRequest --- .../verification/request/InRoomChannel.js | 8 ++- .../verification/request/ToDeviceChannel.js | 16 ++++-- .../request/VerificationRequest.js | 52 +++++++++++++++---- 3 files changed, 60 insertions(+), 16 deletions(-) diff --git a/src/crypto/verification/request/InRoomChannel.js b/src/crypto/verification/request/InRoomChannel.js index f8db518de..94882f200 100644 --- a/src/crypto/verification/request/InRoomChannel.js +++ b/src/crypto/verification/request/InRoomChannel.js @@ -15,7 +15,11 @@ See the License for the specific language governing permissions and limitations under the License. */ -import VerificationRequest, {REQUEST_TYPE, START_TYPE} from "./VerificationRequest"; +import VerificationRequest, { + REQUEST_TYPE, + READY_TYPE, + START_TYPE, +} from "./VerificationRequest"; const MESSAGE_TYPE = "m.room.message"; const M_REFERENCE = "m.reference"; const M_RELATES_TO = "m.relates_to"; @@ -179,7 +183,7 @@ export default class InRoomChannel { */ completeContent(type, content) { content = Object.assign({}, content); - if (type === REQUEST_TYPE || type === START_TYPE) { + if (type === REQUEST_TYPE || type === READY_TYPE || type === START_TYPE) { content.from_device = this._client.getDeviceId(); } if (type === REQUEST_TYPE) { diff --git a/src/crypto/verification/request/ToDeviceChannel.js b/src/crypto/verification/request/ToDeviceChannel.js index 6b280182c..2866cebaa 100644 --- a/src/crypto/verification/request/ToDeviceChannel.js +++ b/src/crypto/verification/request/ToDeviceChannel.js @@ -19,7 +19,9 @@ import { randomString } from '../../../randomstring'; import logger from '../../../logger'; import VerificationRequest, { PHASE_STARTED, + PHASE_READY, REQUEST_TYPE, + READY_TYPE, START_TYPE, CANCEL_TYPE, } from "./VerificationRequest"; @@ -145,14 +147,18 @@ export default class ToDeviceChannel { return this._sendToDevices(CANCEL_TYPE, cancelContent, [deviceId]); } } + const wasStarted = request.phase === PHASE_STARTED || + request.phase === PHASE_READY; - const wasStarted = request.phase === PHASE_STARTED; await request.handleEvent( event.getType(), event, ToDeviceChannel.getTimestamp(event)); - const isStarted = request.phase === PHASE_STARTED; - // the request has picked a start event, tell the other devices about it - if (type === START_TYPE && !wasStarted && isStarted && this._deviceId) { + const isStarted = request.phase === PHASE_STARTED || + request.phase === PHASE_READY; + + const isAcceptingEvent = type === START_TYPE || type === READY_TYPE; + // the request has picked a ready or start event, tell the other devices about it + if (isAcceptingEvent && !wasStarted && isStarted && this._deviceId) { const nonChosenDevices = this._devices.filter(d => d !== this._deviceId); if (nonChosenDevices.length) { const message = this.completeContent({ @@ -188,7 +194,7 @@ export default class ToDeviceChannel { if (this.transactionId) { content.transaction_id = this.transactionId; } - if (type === REQUEST_TYPE || type === START_TYPE) { + if (type === REQUEST_TYPE || type === READY_TYPE || type === START_TYPE) { content.from_device = this._client.getDeviceId(); } if (type === REQUEST_TYPE) { diff --git a/src/crypto/verification/request/VerificationRequest.js b/src/crypto/verification/request/VerificationRequest.js index ba63a479f..a3896dbb6 100644 --- a/src/crypto/verification/request/VerificationRequest.js +++ b/src/crypto/verification/request/VerificationRequest.js @@ -41,11 +41,11 @@ export const REQUEST_TYPE = EVENT_PREFIX + "request"; export const START_TYPE = EVENT_PREFIX + "start"; export const CANCEL_TYPE = EVENT_PREFIX + "cancel"; export const DONE_TYPE = EVENT_PREFIX + "done"; -// export const READY_TYPE = EVENT_PREFIX + "ready"; +export const READY_TYPE = EVENT_PREFIX + "ready"; export const PHASE_UNSENT = 1; export const PHASE_REQUESTED = 2; -// const PHASE_READY = 3; +export const PHASE_READY = 3; export const PHASE_STARTED = 4; export const PHASE_CANCELLED = 5; export const PHASE_DONE = 6; @@ -87,12 +87,13 @@ export default class VerificationRequest extends EventEmitter { return false; } - if (type === REQUEST_TYPE) { + if (type === REQUEST_TYPE || type === READY_TYPE) { if (!Array.isArray(content.methods)) { return false; } } - if (type === REQUEST_TYPE || type === START_TYPE) { + + if (type === REQUEST_TYPE || type === READY_TYPE || type === START_TYPE) { if (typeof content.from_device !== "string" || content.from_device.length === 0 ) { @@ -114,7 +115,7 @@ export default class VerificationRequest extends EventEmitter { return true; } - /** once the phase is PHASE_STARTED, common methods supported by both sides */ + /** once the phase is PHASE_STARTED (and !initiatedByMe) or PHASE_READY: common methods supported by both sides */ get methods() { return this._commonMethods; } @@ -230,7 +231,23 @@ export default class VerificationRequest extends EventEmitter { } } - /** @returns {Promise} with the verifier once it becomes available. Can be used after calling `sendRequest`. */ + /** + * Accepts the request, sending a .ready event to the other party + * @returns {Promise} resolves when the event has been sent. + */ + async accept() { + if (this.phase === PHASE_REQUESTED && !this.initiatedByMe) { + const methods = [...this._verificationMethods.keys()]; + this._setPhase(PHASE_READY, false); + await this.channel.send(READY_TYPE, {methods}); + this.emit("change"); + } + } + + /** + * @returns {Promise} with the verifier once it becomes available. + * Can be used after calling `accept` to wait for the other party to start verification. + */ waitForVerifier() { if (this.verifier) { return Promise.resolve(this.verifier); @@ -270,6 +287,8 @@ export default class VerificationRequest extends EventEmitter { } if (type === REQUEST_TYPE) { await this._handleRequest(content, event); + } else if (type === READY_TYPE) { + await this._handleReady(content); } else if (type === START_TYPE) { await this._handleStart(content, event); } @@ -291,8 +310,7 @@ export default class VerificationRequest extends EventEmitter { async _handleRequest(content, event) { if (this._phase === PHASE_UNSENT) { const otherMethods = content.methods; - this._commonMethods = otherMethods. - filter(m => this._verificationMethods.has(m)); + this._commonMethods = this._filterMethods(otherMethods); this._requestEvent = event; this._initiatedByMe = this._wasSentByMe(event); this._setPhase(PHASE_REQUESTED); @@ -303,8 +321,20 @@ export default class VerificationRequest extends EventEmitter { } } + async _handleReady(content) { + if (this._phase === PHASE_REQUESTED) { + const otherMethods = content.methods; + this._commonMethods = this._filterMethods(otherMethods); + this._setPhase(PHASE_READY); + } else { + logger.warn("Ignoring flagged verification ready event from " + + event.getSender()); + await this.cancel(errorFromEvent(newUnexpectedMessageError())); + } + } + _hasValidPreStartPhase() { - return this._phase === PHASE_REQUESTED || + return this._phase === PHASE_REQUESTED || this._phase === PHASE_READY || ( this.channel.constructor.canCreateRequest(START_TYPE) && this._phase === PHASE_UNSENT @@ -400,6 +430,10 @@ export default class VerificationRequest extends EventEmitter { } } + _filterMethods(methodNames) { + return methodNames.filter(m => this._verificationMethods.has(m)); + } + // only for .request and .start _wasSentByMe(event) { if (event.getSender() !== this._client.getUserId()) { From 2da725340c3950d2e00598db99991131bb2405d7 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Tue, 10 Dec 2019 11:04:55 +0100 Subject: [PATCH 02/43] return request instead of verifier from verification methods as MSC2366 adds an extra interactive step to the verification process, we can't wait for the verifier after sending the request. This is a breaking change in the js-sdk as it changes the return type of an existing method. --- src/client.js | 4 ++-- src/crypto/index.js | 29 ++++------------------------- 2 files changed, 6 insertions(+), 27 deletions(-) diff --git a/src/client.js b/src/client.js index 3503530b7..df74f8a50 100644 --- a/src/client.js +++ b/src/client.js @@ -894,8 +894,8 @@ MatrixClient.prototype.acceptVerificationDM = function(event, method) { * @param {Array} devices array of device IDs to send requests to. Defaults to * all devices owned by the user * - * @returns {Promise} resolves to a verifier - * when the request is accepted by the other user + * @returns {Promise} resolves to a VerificationRequest + * when the request has been sent to the other party. */ MatrixClient.prototype.requestVerification = function(userId, methods, devices) { if (this._crypto === null) { diff --git a/src/crypto/index.js b/src/crypto/index.js index fa547c395..9c12a54f7 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -1515,48 +1515,27 @@ Crypto.prototype.setDeviceVerification = async function( return deviceObj; }; -Crypto.prototype.requestVerificationDM = async function(userId, roomId, methods) { +Crypto.prototype.requestVerificationDM = function(userId, roomId, methods) { const channel = new InRoomChannel(this._baseApis, roomId, userId); - const request = await this._requestVerificationWithChannel( + return this._requestVerificationWithChannel( userId, methods, channel, this._inRoomVerificationRequests, ); - return await request.waitForVerifier(); }; -Crypto.prototype.acceptVerificationDM = function(event, method) { - if(!InRoomChannel.validateEvent(event, this._baseApis)) { - return; - } - - const sender = event.getSender(); - const requestsByTxnId = this._inRoomVerificationRequests.get(sender); - if (!requestsByTxnId) { - return; - } - const transactionId = InRoomChannel.getTransactionId(event); - const request = requestsByTxnId.get(transactionId); - if (!request) { - return; - } - - return request.beginKeyVerification(method); -}; - -Crypto.prototype.requestVerification = async function(userId, methods, devices) { +Crypto.prototype.requestVerification = function(userId, methods, devices) { if (!devices) { devices = Object.keys(this._deviceList.getRawStoredDevicesForUser(userId)); } const channel = new ToDeviceChannel(this._baseApis, userId, devices); - const request = await this._requestVerificationWithChannel( + return this._requestVerificationWithChannel( userId, methods, channel, this._toDeviceVerificationRequests, ); - return await request.waitForVerifier(); }; Crypto.prototype._requestVerificationWithChannel = async function( From 10e294784e25eb1711bae43617dc9e4301b1f64f Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Tue, 10 Dec 2019 12:10:49 +0100 Subject: [PATCH 03/43] waitForVerifier is unused now, make it more broadly useful with callback --- .../request/VerificationRequest.js | 41 +++++++++++-------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/src/crypto/verification/request/VerificationRequest.js b/src/crypto/verification/request/VerificationRequest.js index a3896dbb6..92f7ce9b7 100644 --- a/src/crypto/verification/request/VerificationRequest.js +++ b/src/crypto/verification/request/VerificationRequest.js @@ -245,23 +245,32 @@ export default class VerificationRequest extends EventEmitter { } /** - * @returns {Promise} with the verifier once it becomes available. - * Can be used after calling `accept` to wait for the other party to start verification. + * Can be used to listen for state changes until the callback returns true. + * @param {Function} fn callback to evaluate whether the request is in the desired state. + * Takes the request as an argument. + * @returns {Promise} that resolves once the callback returns true + * @throws {Error} when the request is cancelled */ - waitForVerifier() { - if (this.verifier) { - return Promise.resolve(this.verifier); - } else { - return new Promise(resolve => { - const checkVerifier = () => { - if (this.verifier) { - this.off("change", checkVerifier); - resolve(this.verifier); - } - }; - this.on("change", checkVerifier); - }); - } + waitFor(fn) { + return new Promise((resolve, reject) => { + const check = () => { + let handled = false; + if (fn(this)) { + resolve(this); + handled = true; + } else if (this.cancelled) { + reject(new Error("cancelled")); + handled = true; + } + if (handled) { + this.off("change", check); + } + return handled; + }; + if (!check()) { + this.on("change", check); + } + }); } _setPhase(phase, notify = true) { From 28e46a82ea74b16cbf9ba2e8ebb7008c896b5f1a Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Tue, 10 Dec 2019 12:11:19 +0100 Subject: [PATCH 04/43] expose common phases as properties so we don't need to import the PHASE_ constants where we need to check --- .../request/VerificationRequest.js | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/crypto/verification/request/VerificationRequest.js b/src/crypto/verification/request/VerificationRequest.js index 92f7ce9b7..1cd5aec87 100644 --- a/src/crypto/verification/request/VerificationRequest.js +++ b/src/crypto/verification/request/VerificationRequest.js @@ -115,6 +115,31 @@ export default class VerificationRequest extends EventEmitter { return true; } + /** returns whether the phase is PHASE_REQUESTED */ + get requested() { + return this.phase === PHASE_REQUESTED; + } + + /** returns whether the phase is PHASE_CANCELLED */ + get cancelled() { + return this.phase === PHASE_CANCELLED; + } + + /** returns whether the phase is PHASE_READY */ + get ready() { + return this.phase === PHASE_READY; + } + + /** returns whether the phase is PHASE_STARTED */ + get started() { + return this.phase === PHASE_STARTED; + } + + /** returns whether the phase is PHASE_DONE */ + get done() { + return this.phase === PHASE_DONE; + } + /** once the phase is PHASE_STARTED (and !initiatedByMe) or PHASE_READY: common methods supported by both sides */ get methods() { return this._commonMethods; From 4c6dd564a496c48d14fba4ba6dc8be95f49f2db1 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Tue, 10 Dec 2019 12:11:55 +0100 Subject: [PATCH 05/43] filter verification methods from argument --- src/crypto/index.js | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/crypto/index.js b/src/crypto/index.js index 9c12a54f7..d43c27d1d 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -1541,13 +1541,19 @@ Crypto.prototype.requestVerification = function(userId, methods, devices) { Crypto.prototype._requestVerificationWithChannel = async function( userId, methods, channel, requestsMap, ) { - if (!methods) { - // .keys() returns an iterator, so we need to explicitly turn it into an array - methods = [...this._verificationMethods.keys()]; + let verificationMethods = this._verificationMethods; + if (methods) { + verificationMethods = methods.reduce((map, name) => { + const method = this._verificationMethods.get(name); + if (!method) { + throw new Error(`Verification method ${name} is not supported.`); + } else { + map.set(name, method); + } + }, new Map()); } - // TODO: filter by given methods const request = new VerificationRequest( - channel, this._verificationMethods, userId, this._baseApis); + channel, verificationMethods, userId, this._baseApis); await request.sendRequest(); let requestsByTxnId = requestsMap.get(userId); From cd7cc1b71f59dfe0acb9bee63bf2748d52fc201c Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Tue, 10 Dec 2019 12:13:26 +0100 Subject: [PATCH 06/43] set verification request on event --- src/crypto/index.js | 1 + src/models/event.js | 10 ++++++++++ 2 files changed, 11 insertions(+) diff --git a/src/crypto/index.js b/src/crypto/index.js index d43c27d1d..d94ba0aa2 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -2503,6 +2503,7 @@ Crypto.prototype._handleVerificationEvent = async function( if (!request) { return; } + event.setVerificationRequest(request); isNewRequest = true; if (!requestsByTxnId) { requestsByTxnId = new Map(); diff --git a/src/models/event.js b/src/models/event.js index 0d1b7048b..34af1ecfe 100644 --- a/src/models/event.js +++ b/src/models/event.js @@ -155,6 +155,12 @@ module.exports.MatrixEvent = function MatrixEvent( * attempt may succeed) */ this._retryDecryption = false; + + /* If the event is a `m.key.verification.request` (or to_device `m.key.verification.start`) event, + * `Crypto` will set this the `VerificationRequest` for the event + * so it can be easily accessed from the timeline. + */ + this.verificationRequest = null; }; utils.inherits(module.exports.MatrixEvent, EventEmitter); @@ -1055,6 +1061,10 @@ utils.extend(module.exports.MatrixEvent.prototype, { encrypted: this.event, }; }, + + setVerificationRequest: function(request) { + this.verificationRequest = request; + }, }); From b34a2c7ee239cd956508508d2bfb36507becbe25 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Tue, 10 Dec 2019 17:51:34 +0100 Subject: [PATCH 07/43] WIP --- src/crypto/index.js | 5 +- .../verification/request/InRoomChannel.js | 9 +++- .../request/VerificationRequest.js | 51 +++++++++++++++---- 3 files changed, 52 insertions(+), 13 deletions(-) diff --git a/src/crypto/index.js b/src/crypto/index.js index d94ba0aa2..aa46f8159 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -1550,6 +1550,7 @@ Crypto.prototype._requestVerificationWithChannel = async function( } else { map.set(name, method); } + return map; }, new Map()); } const request = new VerificationRequest( @@ -2469,6 +2470,8 @@ Crypto.prototype._onKeyVerificationMessage = function(event) { * @param {module:models/event.MatrixEvent} event the timeline event */ Crypto.prototype._onTimelineEvent = function(event) { + // TODO: we still need a request object for past requests, so we can show it in the timeline + // validation now excludes old requests if (!InRoomChannel.validateEvent(event, this._baseApis)) { return; } @@ -2503,7 +2506,6 @@ Crypto.prototype._handleVerificationEvent = async function( if (!request) { return; } - event.setVerificationRequest(request); isNewRequest = true; if (!requestsByTxnId) { requestsByTxnId = new Map(); @@ -2511,6 +2513,7 @@ Crypto.prototype._handleVerificationEvent = async function( } requestsByTxnId.set(transactionId, request); } + event.setVerificationRequest(request); try { const hadVerifier = !!request.verifier; await request.channel.handleEvent(event, request); diff --git a/src/crypto/verification/request/InRoomChannel.js b/src/crypto/verification/request/InRoomChannel.js index 94882f200..b476fbb31 100644 --- a/src/crypto/verification/request/InRoomChannel.js +++ b/src/crypto/verification/request/InRoomChannel.js @@ -46,6 +46,10 @@ export default class InRoomChannel { return true; } + get roomId() { + return this._roomId; + } + /** The transaction id generated/used by this verification channel */ get transactionId() { return this._requestEventId; @@ -96,17 +100,20 @@ export default class InRoomChannel { static validateEvent(event, client) { const txnId = InRoomChannel.getTransactionId(event); if (typeof txnId !== "string" || txnId.length === 0) { + console.log("InRoomChannel: validateEvent: no valid txnId", type, txnId); return false; } const type = InRoomChannel.getEventType(event); const content = event.getContent(); if (type === REQUEST_TYPE) { - if (typeof content.to !== "string" || !content.to.length) { + if (!content || typeof content.to !== "string" || !content.to.length) { + console.log("InRoomChannel: validateEvent: no valid to", type, content.to); return false; } const ownUserId = client.getUserId(); // ignore requests that are not direct to or sent by the syncing user if (event.getSender() !== ownUserId && content.to !== ownUserId) { + console.log("InRoomChannel: validateEvent: not directed or sent my me", type, event.getSender(), content.to); return false; } } diff --git a/src/crypto/verification/request/VerificationRequest.js b/src/crypto/verification/request/VerificationRequest.js index 1cd5aec87..7b786735b 100644 --- a/src/crypto/verification/request/VerificationRequest.js +++ b/src/crypto/verification/request/VerificationRequest.js @@ -66,9 +66,12 @@ export default class VerificationRequest extends EventEmitter { this._commonMethods = []; this._setPhase(PHASE_UNSENT, false); this._requestEvent = null; + // TODO: this can also be ourselves, we need to fish .to out of content on a .request this._otherUserId = userId; this._initiatedByMe = null; this._startTimestamp = null; + this._cancellingUserId = null; + this._readyEvent = null; } /** @@ -83,12 +86,18 @@ export default class VerificationRequest extends EventEmitter { static validateEvent(type, event, timestamp, client) { const content = event.getContent(); + if (!content) { + console.log("VerificationRequest: validateEvent: no content", type); + } + if (!type.startsWith(EVENT_PREFIX)) { + console.log("VerificationRequest: validateEvent: fail because type doesnt start with " + EVENT_PREFIX, type); return false; } if (type === REQUEST_TYPE || type === READY_TYPE) { if (!Array.isArray(content.methods)) { + console.log("VerificationRequest: validateEvent: fail because methods", type); return false; } } @@ -97,6 +106,7 @@ export default class VerificationRequest extends EventEmitter { if (typeof content.from_device !== "string" || content.from_device.length === 0 ) { + console.log("VerificationRequest: validateEvent: fail because from_device", type); return false; } } @@ -106,7 +116,9 @@ export default class VerificationRequest extends EventEmitter { const elapsed = Date.now() - timestamp; // ignore if event is too far in the past or too far in the future if (elapsed > (VERIFICATION_REQUEST_TIMEOUT - VERIFICATION_REQUEST_MARGIN) || - elapsed < -(VERIFICATION_REQUEST_TIMEOUT / 2)) { + elapsed < -(VERIFICATION_REQUEST_TIMEOUT / 2) + ) { + console.log("VerificationRequest: validateEvent: verification event to far in future or past", type, elapsed); logger.log("received verification that is too old or from the future"); return false; } @@ -199,6 +211,14 @@ export default class VerificationRequest extends EventEmitter { } } + /** + * the id of the user that cancelled the request, + * only defined when phase is PHASE_CANCELLED + */ + get cancellingUserId() { + return this._cancellingUserId; + } + /* Start the key verification, creating a verifier and sending a .start event. * If no previous events have been sent, pass in `targetDevice` to set who to direct this request to. * @param {string} method the name of the verification method to use. @@ -249,6 +269,7 @@ export default class VerificationRequest extends EventEmitter { if (this._verifier) { return this._verifier.cancel(errorFactory(code, reason)); } else { + this._cancellingUserId = this._client.getUserId(); this._setPhase(PHASE_CANCELLED, false); await this.channel.send(CANCEL_TYPE, {code, reason}); } @@ -262,8 +283,8 @@ export default class VerificationRequest extends EventEmitter { */ async accept() { if (this.phase === PHASE_REQUESTED && !this.initiatedByMe) { - const methods = [...this._verificationMethods.keys()]; this._setPhase(PHASE_READY, false); + const methods = [...this._verificationMethods.keys()]; await this.channel.send(READY_TYPE, {methods}); this.emit("change"); } @@ -322,7 +343,7 @@ export default class VerificationRequest extends EventEmitter { if (type === REQUEST_TYPE) { await this._handleRequest(content, event); } else if (type === READY_TYPE) { - await this._handleReady(content); + await this._handleReady(event); } else if (type === START_TYPE) { await this._handleStart(content, event); } @@ -335,7 +356,7 @@ export default class VerificationRequest extends EventEmitter { } if (type === CANCEL_TYPE) { - this._handleCancel(); + this._handleCancel(event); } else if (type === DONE_TYPE) { this._handleDone(); } @@ -349,19 +370,22 @@ export default class VerificationRequest extends EventEmitter { this._initiatedByMe = this._wasSentByMe(event); this._setPhase(PHASE_REQUESTED); } else if (this._phase !== PHASE_REQUESTED) { - logger.warn("Ignoring flagged verification request from " + + logger.warn("Cancelling, unexpected .request verification event from " + event.getSender()); await this.cancel(errorFromEvent(newUnexpectedMessageError())); } } - async _handleReady(content) { - if (this._phase === PHASE_REQUESTED) { + async _handleReady(event) { + const content = event.getContent(); + console.log("handling ready event", content, this._phase); + if (this.phase === PHASE_REQUESTED || this.phase === PHASE_READY) { + this._readyEvent = event; const otherMethods = content.methods; this._commonMethods = this._filterMethods(otherMethods); this._setPhase(PHASE_READY); } else { - logger.warn("Ignoring flagged verification ready event from " + + logger.warn("Cancelling, unexpected .ready verification event from " + event.getSender()); await this.cancel(errorFromEvent(newUnexpectedMessageError())); } @@ -385,7 +409,9 @@ export default class VerificationRequest extends EventEmitter { if (this.phase === PHASE_UNSENT) { this._initiatedByMe = this._wasSentByMe(event); } - this._verifier = this._createVerifier(method, event); + if (!this._verifier) { + this._verifier = this._createVerifier(method, event); + } this._setPhase(PHASE_STARTED); } } @@ -409,8 +435,9 @@ export default class VerificationRequest extends EventEmitter { } } - _handleCancel() { + _handleCancel(event = null) { if (this._phase !== PHASE_CANCELLED) { + this._cancellingUserId = event ? event.getSender() : this._client.getUserId(); this._setPhase(PHASE_CANCELLED); } } @@ -450,6 +477,8 @@ export default class VerificationRequest extends EventEmitter { let targetEvent; if (startEvent && !this._wasSentByMe(startEvent)) { targetEvent = startEvent; + } else if (this._readyEvent && !this._wasSentByMe(this._readyEvent)) { + targetEvent = this._readyEvent; } else if (this._requestEvent && !this._wasSentByMe(this._requestEvent)) { targetEvent = this._requestEvent; } else { @@ -468,7 +497,7 @@ export default class VerificationRequest extends EventEmitter { return methodNames.filter(m => this._verificationMethods.has(m)); } - // only for .request and .start + // only for .request, .ready or .start _wasSentByMe(event) { if (event.getSender() !== this._client.getUserId()) { return false; From c4142d93c3971362c25037841991308a936616b9 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 18 Dec 2019 16:51:39 +0000 Subject: [PATCH 08/43] store in-room verification requests by roomId, txnId as it's harder to determine the other side of a request, given the in-room code also processes remote echos for own events. --- src/crypto/index.js | 68 ++++++++----------- .../verification/request/InRoomChannel.js | 48 ++++++++++++- .../verification/request/ToDeviceChannel.js | 59 +++++++++++++++- 3 files changed, 131 insertions(+), 44 deletions(-) diff --git a/src/crypto/index.js b/src/crypto/index.js index aa46f8159..a73890e62 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -52,8 +52,8 @@ import { keyFromPassphrase } from './key_passphrase'; import { encodeRecoveryKey } from './recoverykey'; import VerificationRequest from "./verification/request/VerificationRequest"; -import InRoomChannel from "./verification/request/InRoomChannel"; -import ToDeviceChannel from "./verification/request/ToDeviceChannel"; +import {InRoomChannel, InRoomRequests} from "./verification/request/InRoomChannel"; +import {ToDeviceChannel, ToDeviceRequests} from "./verification/request/ToDeviceChannel"; const defaultVerificationMethods = { [ScanQRCode.NAME]: ScanQRCode, @@ -206,8 +206,8 @@ export default function Crypto(baseApis, sessionStore, userId, deviceId, // } this._lastNewSessionForced = {}; - this._toDeviceVerificationRequests = new Map(); - this._inRoomVerificationRequests = new Map(); + this._toDeviceVerificationRequests = new ToDeviceRequests(); + this._inRoomVerificationRequests = new InRoomRequests(); const cryptoCallbacks = this._baseApis._cryptoCallbacks || {}; @@ -1557,16 +1557,11 @@ Crypto.prototype._requestVerificationWithChannel = async function( channel, verificationMethods, userId, this._baseApis); await request.sendRequest(); - let requestsByTxnId = requestsMap.get(userId); - if (!requestsByTxnId) { - requestsByTxnId = new Map(); - requestsMap.set(userId, requestsByTxnId); - } // TODO: we're only adding the request to the map once it has been sent // but if the other party is really fast they could potentially respond to the // request before the server tells us the event got sent, and we would probably // create a new request object - requestsByTxnId.set(channel.transactionId, request); + requestsMap.setRequestByChannel(channel, request); return request; }; @@ -1574,25 +1569,23 @@ Crypto.prototype._requestVerificationWithChannel = async function( Crypto.prototype.beginKeyVerification = function( method, userId, deviceId, transactionId = null, ) { - let requestsByTxnId = this._toDeviceVerificationRequests.get(userId); - if (!requestsByTxnId) { - requestsByTxnId = new Map(); - this._toDeviceVerificationRequests.set(userId, requestsByTxnId); - } let request; if (transactionId) { - request = requestsByTxnId.get(transactionId); + request = this._toDeviceVerificationRequests.getRequestBySenderAndTxnId( + userId, transactionId); + if (!request) { + throw new Error( + `No request found for user ${userId} with ` + + `transactionId ${transactionId}`); + } } else { transactionId = ToDeviceChannel.makeTransactionId(); const channel = new ToDeviceChannel( this._baseApis, userId, [deviceId], transactionId, deviceId); request = new VerificationRequest( channel, this._verificationMethods, userId, this._baseApis); - requestsByTxnId.set(transactionId, request); - } - if (!request) { - throw new Error( - `No request found for user ${userId} with transactionId ${transactionId}`); + this._toDeviceVerificationRequests.setRequestBySenderAndTxnId( + userId, transactionId, request); } return request.beginKeyVerification(method, {userId, deviceId}); }; @@ -2440,7 +2433,6 @@ Crypto.prototype._onKeyVerificationMessage = function(event) { if (!ToDeviceChannel.validateEvent(event, this._baseApis)) { return; } - const transactionId = ToDeviceChannel.getTransactionId(event); const createRequest = event => { if (!ToDeviceChannel.canCreateRequest(ToDeviceChannel.getEventType(event))) { return; @@ -2459,8 +2451,11 @@ Crypto.prototype._onKeyVerificationMessage = function(event) { return new VerificationRequest( channel, this._verificationMethods, userId, this._baseApis); }; - this._handleVerificationEvent(event, transactionId, - this._toDeviceVerificationRequests, createRequest); + this._handleVerificationEvent( + event, + this._toDeviceVerificationRequests, + createRequest, + ); }; /** @@ -2475,7 +2470,6 @@ Crypto.prototype._onTimelineEvent = function(event) { if (!InRoomChannel.validateEvent(event, this._baseApis)) { return; } - const transactionId = InRoomChannel.getTransactionId(event); const createRequest = event => { if (!InRoomChannel.canCreateRequest(InRoomChannel.getEventType(event))) { return; @@ -2489,17 +2483,18 @@ Crypto.prototype._onTimelineEvent = function(event) { return new VerificationRequest( channel, this._verificationMethods, userId, this._baseApis); }; - this._handleVerificationEvent(event, transactionId, - this._inRoomVerificationRequests, createRequest); + this._handleVerificationEvent( + event, + this._inRoomVerificationRequests, + createRequest, + ); }; Crypto.prototype._handleVerificationEvent = async function( - event, transactionId, requestsMap, createRequest, + event, requestsMap, createRequest, ) { - const sender = event.getSender(); - let requestsByTxnId = requestsMap.get(sender); + let request = requestsMap.getRequest(event); let isNewRequest = false; - let request = requestsByTxnId && requestsByTxnId.get(transactionId); if (!request) { request = createRequest(event); // a request could not be made from this event, so ignore event @@ -2507,11 +2502,7 @@ Crypto.prototype._handleVerificationEvent = async function( return; } isNewRequest = true; - if (!requestsByTxnId) { - requestsByTxnId = new Map(); - requestsMap.set(sender, requestsByTxnId); - } - requestsByTxnId.set(transactionId, request); + requestsMap.setRequest(event, request); } event.setVerificationRequest(request); try { @@ -2525,10 +2516,7 @@ Crypto.prototype._handleVerificationEvent = async function( console.error("error while handling verification event", event, err); } if (!request.pending) { - requestsByTxnId.delete(transactionId); - if (requestsByTxnId.size === 0) { - requestsMap.delete(sender); - } + requestsMap.removeRequest(event); } else if (isNewRequest && !request.initiatedByMe) { this._baseApis.emit("crypto.verification.request", request); } diff --git a/src/crypto/verification/request/InRoomChannel.js b/src/crypto/verification/request/InRoomChannel.js index b476fbb31..a086e0c78 100644 --- a/src/crypto/verification/request/InRoomChannel.js +++ b/src/crypto/verification/request/InRoomChannel.js @@ -28,7 +28,7 @@ const M_RELATES_TO = "m.relates_to"; * A key verification channel that sends verification events in the timeline of a room. * Uses the event id of the initial m.key.verification.request event as a transaction id. */ -export default class InRoomChannel { +export class InRoomChannel { /** * @param {MatrixClient} client the matrix client, to send messages with and get current user & device from. * @param {string} roomId id of the room where verification events should be posted in, should be a DM with the given user. @@ -242,3 +242,49 @@ export default class InRoomChannel { } } } + +export class InRoomRequests { + constructor() { + this._requestsByRoomId = new Map(); + } + + getRequest(event) { + const roomId = event.getRoomId(); + const requestsByTxnId = this._requestsByRoomId.get(roomId); + if (requestsByTxnId) { + return requestsByTxnId.get(InRoomChannel.getTransactionId(event)); + } + } + + setRequest(event, request) { + this._setRequest( + event.getRoomId(), + InRoomChannel.getTransactionId(event), + request, + ); + } + + setRequestByChannel(channel, request) { + this._setRequest(channel.roomId, channel.transactionId, request); + } + + _setRequest(roomId, txnId, request) { + let requestsByTxnId = this._requestsByRoomId.get(roomId); + if (!requestsByTxnId) { + requestsByTxnId = new Map(); + this._requestsByRoomId.set(roomId, requestsByTxnId); + } + requestsByTxnId.set(txnId, request); + } + + removeRequest(event) { + const roomId = event.getRoomId(); + const requestsByTxnId = this._requestsByRoomId.get(roomId); + if (requestsByTxnId) { + requestsByTxnId.delete(InRoomChannel.getTransactionId(event)); + if (requestsByTxnId.size === 0) { + this._requestsByRoomId.delete(roomId); + } + } + } +} diff --git a/src/crypto/verification/request/ToDeviceChannel.js b/src/crypto/verification/request/ToDeviceChannel.js index 2866cebaa..d80a5a5c0 100644 --- a/src/crypto/verification/request/ToDeviceChannel.js +++ b/src/crypto/verification/request/ToDeviceChannel.js @@ -34,11 +34,11 @@ import { * A key verification channel that sends verification events over to_device messages. * Generates its own transaction ids. */ -export default class ToDeviceChannel { +export class ToDeviceChannel { // userId and devices of user we're about to verify constructor(client, userId, devices, transactionId = null, deviceId = null) { this._client = client; - this._userId = userId; + this.userId = userId; this._devices = devices; this.transactionId = transactionId; this._deviceId = deviceId; @@ -239,7 +239,7 @@ export default class ToDeviceChannel { msgMap[deviceId] = content; } - return this._client.sendToDevice(type, {[this._userId]: msgMap}); + return this._client.sendToDevice(type, {[this.userId]: msgMap}); } else { return Promise.resolve(); } @@ -253,3 +253,56 @@ export default class ToDeviceChannel { return randomString(32); } } + + +export class ToDeviceRequests { + constructor() { + this._requestsByUserId = new Map(); + } + + getRequest(event) { + return this.getRequestBySenderAndTxnId( + event.getSender(), + ToDeviceChannel.getTransactionId(event), + ); + } + + getRequestBySenderAndTxnId(sender, txnId) { + const requestsByTxnId = this._requestsByUserId.get(sender); + if (requestsByTxnId) { + return requestsByTxnId.get(txnId); + } + } + + setRequest(event, request) { + this.setBySenderAndTxnId( + event.getSender(), + ToDeviceChannel.getTransactionId(event), + request, + ); + } + + setRequestByChannel(channel, request) { + this.setBySenderAndTxnId(channel.userId, channel.transactionId, request); + } + + setRequestBySenderAndTxnId(sender, txnId, request) { + let requestsByTxnId = this._requestsByUserId.get(sender); + if (!requestsByTxnId) { + requestsByTxnId = new Map(); + this._requestsByUserId.set(sender, requestsByTxnId); + } + requestsByTxnId.set(txnId, request); + } + + removeRequest(event) { + const userId = event.getSender(); + const requestsByTxnId = this._requestsByUserId.get(userId); + if (requestsByTxnId) { + requestsByTxnId.delete(ToDeviceChannel.getTransactionId(event)); + if (requestsByTxnId.size === 0) { + this._requestsByUserId.delete(userId); + } + } + } +} From 5f9e82204a2fb3857c46d8ce6719df457669834c Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 19 Dec 2019 16:07:39 +0000 Subject: [PATCH 09/43] more ready and remote echo support --- .../request/VerificationRequest.js | 37 +++++++++---------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/src/crypto/verification/request/VerificationRequest.js b/src/crypto/verification/request/VerificationRequest.js index 7b786735b..858ee81d8 100644 --- a/src/crypto/verification/request/VerificationRequest.js +++ b/src/crypto/verification/request/VerificationRequest.js @@ -401,19 +401,20 @@ export default class VerificationRequest extends EventEmitter { async _handleStart(content, event) { if (this._hasValidPreStartPhase()) { + const sentByMe = this._wasSentByMe(event); const {method} = content; - if (!this._verificationMethods.has(method)) { + if (!sentByMe && !this._verificationMethods.has(method)) { await this.cancel(errorFromEvent(newUnknownMethodError())); - } else { - // if not in requested phase - if (this.phase === PHASE_UNSENT) { - this._initiatedByMe = this._wasSentByMe(event); - } - if (!this._verifier) { - this._verifier = this._createVerifier(method, event); - } - this._setPhase(PHASE_STARTED); + return; } + // if not in requested phase + if (this.phase === PHASE_UNSENT) { + this._initiatedByMe = sentByMe; + } + if (!this._verifier) { + this._verifier = this._createVerifier(method, event); + } + this._setPhase(PHASE_STARTED); } } @@ -425,13 +426,11 @@ export default class VerificationRequest extends EventEmitter { handleVerifierSend(type, content) { if (type === CANCEL_TYPE) { this._handleCancel(); - } else if (type === START_TYPE) { - if (this._phase === PHASE_UNSENT || this._phase === PHASE_REQUESTED) { - // if unsent, we're sending a (first) .start event and hence requesting the verification. - // in any other situation, the request was initiated by the other party. - this._initiatedByMe = this.phase === PHASE_UNSENT; - this._setPhase(PHASE_STARTED); - } + } else if (type === START_TYPE && this._hasValidPreStartPhase()) { + // if unsent, we're sending a (first) .start event and hence requesting the verification. + // in any other situation, the request was initiated by the other party. + this._initiatedByMe = this.phase === PHASE_UNSENT; + this._setPhase(PHASE_STARTED); } } @@ -449,7 +448,7 @@ export default class VerificationRequest extends EventEmitter { } _createVerifier(method, startEvent = null, targetDevice = null) { - const startSentByMe = startEvent && this._wasSentByMe(startEvent); + const initiatedByMe = !startEvent || this._wasSentByMe(startEvent); const {userId, deviceId} = this._getVerifierTarget(startEvent, targetDevice); const VerifierCtor = this._verificationMethods.get(method); @@ -464,7 +463,7 @@ export default class VerificationRequest extends EventEmitter { this._client, userId, deviceId, - startSentByMe ? null : startEvent, + initiatedByMe ? null : startEvent, ); } From dac4a5452dd185d1a65650f1d42836d9870df876 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 20 Dec 2019 11:15:23 +0000 Subject: [PATCH 10/43] make this a public prop --- src/crypto/verification/request/VerificationRequest.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/crypto/verification/request/VerificationRequest.js b/src/crypto/verification/request/VerificationRequest.js index 858ee81d8..8a4533b6e 100644 --- a/src/crypto/verification/request/VerificationRequest.js +++ b/src/crypto/verification/request/VerificationRequest.js @@ -211,6 +211,11 @@ export default class VerificationRequest extends EventEmitter { } } + /** the user id of the other party in this request */ + get otherUserId() { + return this._otherUserId; + } + /** * the id of the user that cancelled the request, * only defined when phase is PHASE_CANCELLED From 984b6234d216943b46a28f71e47aab3ce3dcbc68 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 20 Dec 2019 11:16:34 +0000 Subject: [PATCH 11/43] don't block remote echos to VerificationRequests also put logic to block non-participating senders in VerificationRequest so it is shared between both channels. Remote echo's should not be passed to the verifier though. --- src/crypto/verification/request/InRoomChannel.js | 3 ++- .../verification/request/VerificationRequest.js | 11 ++++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/crypto/verification/request/InRoomChannel.js b/src/crypto/verification/request/InRoomChannel.js index a086e0c78..c3edbcaac 100644 --- a/src/crypto/verification/request/InRoomChannel.js +++ b/src/crypto/verification/request/InRoomChannel.js @@ -153,7 +153,8 @@ export class InRoomChannel { const type = InRoomChannel.getEventType(event); // do validations that need state (roomId, userId), // ignore if invalid - if (event.getRoomId() !== this._roomId || event.getSender() !== this._userId) { + + if (event.getRoomId() !== this._roomId) { return; } // set transactionId when receiving a .request diff --git a/src/crypto/verification/request/VerificationRequest.js b/src/crypto/verification/request/VerificationRequest.js index 8a4533b6e..8a7fb1b87 100644 --- a/src/crypto/verification/request/VerificationRequest.js +++ b/src/crypto/verification/request/VerificationRequest.js @@ -339,6 +339,13 @@ export default class VerificationRequest extends EventEmitter { * @returns {Promise} a promise that resolves when any requests as an anwser to the passed-in event are sent. */ async handleEvent(type, event, timestamp) { + const sender = event.getSender(); + + if (sender !== this._client.getUserId() && sender !== this._otherUserId) { + console.log(`VerificationRequest: ignoring verification event from non-participating sender ${sender}`); + return; + } + const content = event.getContent(); if (type === REQUEST_TYPE || type === START_TYPE) { if (this._startTimestamp === null) { @@ -353,7 +360,9 @@ export default class VerificationRequest extends EventEmitter { await this._handleStart(content, event); } - if (this._verifier) { + // only pass events from the other side to the verifier, + // no remote echos of our own events + if (this._verifier && sender === this._otherUserId) { if (type === CANCEL_TYPE || (this._verifier.events && this._verifier.events.includes(type))) { this._verifier.handleEvent(event); From 29c04b6f9cf3358fffb9a1003434e36d549bdcce Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 20 Dec 2019 11:17:56 +0000 Subject: [PATCH 12/43] only move to PHASE_DONE when both .done events are received as once in done, the request is removed from the request map and the second .done event that comes in will not find the request anymore, so the request wouldn't be attached to the event anymore, breaking rendering it in the timeline. --- .../request/VerificationRequest.js | 22 ++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/src/crypto/verification/request/VerificationRequest.js b/src/crypto/verification/request/VerificationRequest.js index 8a7fb1b87..aa981e3e7 100644 --- a/src/crypto/verification/request/VerificationRequest.js +++ b/src/crypto/verification/request/VerificationRequest.js @@ -72,6 +72,8 @@ export default class VerificationRequest extends EventEmitter { this._startTimestamp = null; this._cancellingUserId = null; this._readyEvent = null; + this._doneByThem = false; + this._doneByUs = false; } /** @@ -372,7 +374,7 @@ export default class VerificationRequest extends EventEmitter { if (type === CANCEL_TYPE) { this._handleCancel(event); } else if (type === DONE_TYPE) { - this._handleDone(); + this._handleDone(event); } } @@ -455,9 +457,23 @@ export default class VerificationRequest extends EventEmitter { } } - _handleDone() { + _handleDone(event) { if (this._phase === PHASE_STARTED) { - this._setPhase(PHASE_DONE); + const sender = event.getSender(); + if (sender === this._client.getUserId()) { + this._doneByUs = true; + console.log("VerificationRequest: received own .done event"); + } else if (sender === this._otherUserId) { + this._doneByThem = true; + console.log("VerificationRequest: received their .done event"); + } + // we need both .done events to consider the request done, + // as it will be deleted from the requests map once it's done, + // and we need to find the request on our own incoming .done + // to attach the request to the event to render it in the timeline. + if (this._doneByUs && this._doneByThem) { + this._setPhase(PHASE_DONE); + } } } From efe2488155b0d23cbc0941cc8a771865b4a34fe4 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 20 Dec 2019 13:43:49 +0000 Subject: [PATCH 13/43] get other user id from channel next up is inspecting the .request event to determine it reliably in InRoomChannel --- src/crypto/verification/request/InRoomChannel.js | 4 ++-- .../verification/request/VerificationRequest.js | 14 ++++++-------- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/crypto/verification/request/InRoomChannel.js b/src/crypto/verification/request/InRoomChannel.js index c3edbcaac..4faf4e9c0 100644 --- a/src/crypto/verification/request/InRoomChannel.js +++ b/src/crypto/verification/request/InRoomChannel.js @@ -37,7 +37,7 @@ export class InRoomChannel { constructor(client, roomId, userId) { this._client = client; this._roomId = roomId; - this._userId = userId; + this.userId = userId; this._requestEventId = null; } @@ -202,7 +202,7 @@ export class InRoomChannel { "verification. You will need to use legacy key " + "verification to verify keys.", msgtype: REQUEST_TYPE, - to: this._userId, + to: this.userId, from_device: content.from_device, methods: content.methods, }; diff --git a/src/crypto/verification/request/VerificationRequest.js b/src/crypto/verification/request/VerificationRequest.js index aa981e3e7..261751877 100644 --- a/src/crypto/verification/request/VerificationRequest.js +++ b/src/crypto/verification/request/VerificationRequest.js @@ -58,7 +58,7 @@ export const PHASE_DONE = 6; * @event "change" whenever the state of the request object has changed. */ export default class VerificationRequest extends EventEmitter { - constructor(channel, verificationMethods, userId, client) { + constructor(channel, verificationMethods, client) { super(); this.channel = channel; this._verificationMethods = verificationMethods; @@ -66,8 +66,6 @@ export default class VerificationRequest extends EventEmitter { this._commonMethods = []; this._setPhase(PHASE_UNSENT, false); this._requestEvent = null; - // TODO: this can also be ourselves, we need to fish .to out of content on a .request - this._otherUserId = userId; this._initiatedByMe = null; this._startTimestamp = null; this._cancellingUserId = null; @@ -200,14 +198,14 @@ export default class VerificationRequest extends EventEmitter { if (this.initiatedByMe) { return this._client.getUserId(); } else { - return this._otherUserId; + return this.otherUserId; } } /** the id of the user that (will) receive(d) the request */ get receivingUserId() { if (this.initiatedByMe) { - return this._otherUserId; + return this.otherUserId; } else { return this._client.getUserId(); } @@ -215,7 +213,7 @@ export default class VerificationRequest extends EventEmitter { /** the user id of the other party in this request */ get otherUserId() { - return this._otherUserId; + return this.channel.userId; } /** @@ -364,7 +362,7 @@ export default class VerificationRequest extends EventEmitter { // only pass events from the other side to the verifier, // no remote echos of our own events - if (this._verifier && sender === this._otherUserId) { + if (this._verifier && sender === this.otherUserId) { if (type === CANCEL_TYPE || (this._verifier.events && this._verifier.events.includes(type))) { this._verifier.handleEvent(event); @@ -463,7 +461,7 @@ export default class VerificationRequest extends EventEmitter { if (sender === this._client.getUserId()) { this._doneByUs = true; console.log("VerificationRequest: received own .done event"); - } else if (sender === this._otherUserId) { + } else if (sender === this.otherUserId) { this._doneByThem = true; console.log("VerificationRequest: received their .done event"); } From 48977e6eaa9cb3ca51aeb48af4ded37d94b1bc30 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 20 Dec 2019 13:45:48 +0000 Subject: [PATCH 14/43] get other party user id by inspecting initial event sender/to fields also fail validation with any event not sent by or directed to us --- src/crypto/index.js | 16 +++++++----- .../verification/request/InRoomChannel.js | 25 +++++++++++++++++-- 2 files changed, 33 insertions(+), 8 deletions(-) diff --git a/src/crypto/index.js b/src/crypto/index.js index a73890e62..63c7b3681 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -1554,7 +1554,7 @@ Crypto.prototype._requestVerificationWithChannel = async function( }, new Map()); } const request = new VerificationRequest( - channel, verificationMethods, userId, this._baseApis); + channel, verificationMethods, this._baseApis); await request.sendRequest(); // TODO: we're only adding the request to the map once it has been sent @@ -1583,7 +1583,7 @@ Crypto.prototype.beginKeyVerification = function( const channel = new ToDeviceChannel( this._baseApis, userId, [deviceId], transactionId, deviceId); request = new VerificationRequest( - channel, this._verificationMethods, userId, this._baseApis); + channel, this._verificationMethods, this._baseApis); this._toDeviceVerificationRequests.setRequestBySenderAndTxnId( userId, transactionId, request); } @@ -2449,7 +2449,7 @@ Crypto.prototype._onKeyVerificationMessage = function(event) { [deviceId], ); return new VerificationRequest( - channel, this._verificationMethods, userId, this._baseApis); + channel, this._verificationMethods, this._baseApis); }; this._handleVerificationEvent( event, @@ -2474,14 +2474,18 @@ Crypto.prototype._onTimelineEvent = function(event) { if (!InRoomChannel.canCreateRequest(InRoomChannel.getEventType(event))) { return; } - const userId = event.getSender(); + const otherPartyUserId = + InRoomChannel.getOtherPartyUserId(event, this._baseApis); + if (!otherPartyUserId) { + return; + } const channel = new InRoomChannel( this._baseApis, event.getRoomId(), - userId, + otherPartyUserId, ); return new VerificationRequest( - channel, this._verificationMethods, userId, this._baseApis); + channel, this._verificationMethods, this._baseApis); }; this._handleVerificationEvent( event, diff --git a/src/crypto/verification/request/InRoomChannel.js b/src/crypto/verification/request/InRoomChannel.js index 4faf4e9c0..ca6e66606 100644 --- a/src/crypto/verification/request/InRoomChannel.js +++ b/src/crypto/verification/request/InRoomChannel.js @@ -55,6 +55,27 @@ export class InRoomChannel { return this._requestEventId; } + static getOtherPartyUserId(event, client) { + const type = InRoomChannel.getEventType(event); + if (type !== REQUEST_TYPE) { + return; + } + const ownUserId = client.getUserId(); + const sender = event.getSender(); + const content = event.getContent(); + const receiver = content.to; + + // request is not sent by or directed at us + if (sender !== ownUserId && receiver !== ownUserId) { + return; + } + if (sender === ownUserId) { + return receiver; + } else { + return sender; + } + } + /** * @param {MatrixEvent} event the event to get the timestamp of * @return {number} the timestamp when the event was sent @@ -110,9 +131,9 @@ export class InRoomChannel { console.log("InRoomChannel: validateEvent: no valid to", type, content.to); return false; } - const ownUserId = client.getUserId(); + // ignore requests that are not direct to or sent by the syncing user - if (event.getSender() !== ownUserId && content.to !== ownUserId) { + if (!InRoomChannel.getOtherPartyUserId(event, client)) { console.log("InRoomChannel: validateEvent: not directed or sent my me", type, event.getSender(), content.to); return false; } From 883b83f1da8fa8a30cfd525fe09bac91ccd25dc9 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 20 Dec 2019 13:47:04 +0000 Subject: [PATCH 15/43] move blocking non-participating users back to InRoomChannel as it doesn't need to happen for ToDeviceChannel --- src/crypto/verification/request/InRoomChannel.js | 7 +++++++ src/crypto/verification/request/VerificationRequest.js | 6 ------ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/crypto/verification/request/InRoomChannel.js b/src/crypto/verification/request/InRoomChannel.js index ca6e66606..12aaeb843 100644 --- a/src/crypto/verification/request/InRoomChannel.js +++ b/src/crypto/verification/request/InRoomChannel.js @@ -178,6 +178,13 @@ export class InRoomChannel { if (event.getRoomId() !== this._roomId) { return; } + // ignore events not sent by us or the other party + const ownUserId = this._client.getUserId(); + const sender = event.getSender(); + if (sender !== ownUserId && sender !== this.userId) { + console.log(`InRoomChannel: ignoring verification event from non-participating sender ${sender}`); + return; + } // set transactionId when receiving a .request if (!this._requestEventId && type === REQUEST_TYPE) { this._requestEventId = event.getId(); diff --git a/src/crypto/verification/request/VerificationRequest.js b/src/crypto/verification/request/VerificationRequest.js index 261751877..b8db96d4f 100644 --- a/src/crypto/verification/request/VerificationRequest.js +++ b/src/crypto/verification/request/VerificationRequest.js @@ -340,12 +340,6 @@ export default class VerificationRequest extends EventEmitter { */ async handleEvent(type, event, timestamp) { const sender = event.getSender(); - - if (sender !== this._client.getUserId() && sender !== this._otherUserId) { - console.log(`VerificationRequest: ignoring verification event from non-participating sender ${sender}`); - return; - } - const content = event.getContent(); if (type === REQUEST_TYPE || type === START_TYPE) { if (this._startTimestamp === null) { From e7bcb61a3b634f13dbdf19a56e693354120709fb Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 20 Dec 2019 21:29:18 +0100 Subject: [PATCH 16/43] attempt at only creating verifier for live events but doesn't work yet? data where liveEvent is fished out is undefined --- src/crypto/index.js | 11 ++++++++--- src/crypto/verification/request/InRoomChannel.js | 5 +++-- src/crypto/verification/request/ToDeviceChannel.js | 5 +++-- .../verification/request/VerificationRequest.js | 8 ++++---- 4 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/crypto/index.js b/src/crypto/index.js index 63c7b3681..6c609b7dd 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -2463,8 +2463,12 @@ Crypto.prototype._onKeyVerificationMessage = function(event) { * * @private * @param {module:models/event.MatrixEvent} event the timeline event + * @param {module:models/Room} room not used + * @param {bool} atStart not used + * @param {bool} removed not used + * @param {bool} data.liveEvent whether this is a live event */ -Crypto.prototype._onTimelineEvent = function(event) { +Crypto.prototype._onTimelineEvent = function(event, room, atStart, removed, {liveEvent}) { // TODO: we still need a request object for past requests, so we can show it in the timeline // validation now excludes old requests if (!InRoomChannel.validateEvent(event, this._baseApis)) { @@ -2491,11 +2495,12 @@ Crypto.prototype._onTimelineEvent = function(event) { event, this._inRoomVerificationRequests, createRequest, + liveEvent, ); }; Crypto.prototype._handleVerificationEvent = async function( - event, requestsMap, createRequest, + event, requestsMap, createRequest, isLiveEvent = true, ) { let request = requestsMap.getRequest(event); let isNewRequest = false; @@ -2511,7 +2516,7 @@ Crypto.prototype._handleVerificationEvent = async function( event.setVerificationRequest(request); try { const hadVerifier = !!request.verifier; - await request.channel.handleEvent(event, request); + await request.channel.handleEvent(event, request, isLiveEvent); // emit start event when verifier got set if (!hadVerifier && request.verifier) { this._baseApis.emit("crypto.verification.start", request.verifier); diff --git a/src/crypto/verification/request/InRoomChannel.js b/src/crypto/verification/request/InRoomChannel.js index 12aaeb843..3ccb06270 100644 --- a/src/crypto/verification/request/InRoomChannel.js +++ b/src/crypto/verification/request/InRoomChannel.js @@ -168,9 +168,10 @@ export class InRoomChannel { * Changes the state of the channel, request, and verifier in response to a key verification event. * @param {MatrixEvent} event to handle * @param {VerificationRequest} request the request to forward handling to + * @param {bool} isLiveEvent whether this is an even received through sync or not * @returns {Promise} a promise that resolves when any requests as an anwser to the passed-in event are sent. */ - async handleEvent(event, request) { + async handleEvent(event, request, isLiveEvent) { const type = InRoomChannel.getEventType(event); // do validations that need state (roomId, userId), // ignore if invalid @@ -190,7 +191,7 @@ export class InRoomChannel { this._requestEventId = event.getId(); } - return await request.handleEvent(type, event, InRoomChannel.getTimestamp(event)); + return await request.handleEvent(type, event, InRoomChannel.getTimestamp(event), isLiveEvent); } /** diff --git a/src/crypto/verification/request/ToDeviceChannel.js b/src/crypto/verification/request/ToDeviceChannel.js index d80a5a5c0..3057d91e4 100644 --- a/src/crypto/verification/request/ToDeviceChannel.js +++ b/src/crypto/verification/request/ToDeviceChannel.js @@ -123,9 +123,10 @@ export class ToDeviceChannel { * Changes the state of the channel, request, and verifier in response to a key verification event. * @param {MatrixEvent} event to handle * @param {VerificationRequest} request the request to forward handling to + * @param {bool} isLiveEvent whether this is an even received through sync or not * @returns {Promise} a promise that resolves when any requests as an anwser to the passed-in event are sent. */ - async handleEvent(event, request) { + async handleEvent(event, request, isLiveEvent) { const type = event.getType(); const content = event.getContent(); if (type === REQUEST_TYPE || type === START_TYPE) { @@ -151,7 +152,7 @@ export class ToDeviceChannel { request.phase === PHASE_READY; await request.handleEvent( - event.getType(), event, ToDeviceChannel.getTimestamp(event)); + event.getType(), event, ToDeviceChannel.getTimestamp(event), isLiveEvent); const isStarted = request.phase === PHASE_STARTED || request.phase === PHASE_READY; diff --git a/src/crypto/verification/request/VerificationRequest.js b/src/crypto/verification/request/VerificationRequest.js index b8db96d4f..4daf49fbc 100644 --- a/src/crypto/verification/request/VerificationRequest.js +++ b/src/crypto/verification/request/VerificationRequest.js @@ -338,7 +338,7 @@ export default class VerificationRequest extends EventEmitter { * @param {number} timestamp the timestamp in milliseconds when this event was sent. * @returns {Promise} a promise that resolves when any requests as an anwser to the passed-in event are sent. */ - async handleEvent(type, event, timestamp) { + async handleEvent(type, event, timestamp, isLiveEvent) { const sender = event.getSender(); const content = event.getContent(); if (type === REQUEST_TYPE || type === START_TYPE) { @@ -351,7 +351,7 @@ export default class VerificationRequest extends EventEmitter { } else if (type === READY_TYPE) { await this._handleReady(event); } else if (type === START_TYPE) { - await this._handleStart(content, event); + await this._handleStart(content, event, isLiveEvent); } // only pass events from the other side to the verifier, @@ -407,7 +407,7 @@ export default class VerificationRequest extends EventEmitter { ); } - async _handleStart(content, event) { + async _handleStart(content, event, isLiveEvent) { if (this._hasValidPreStartPhase()) { const sentByMe = this._wasSentByMe(event); const {method} = content; @@ -419,7 +419,7 @@ export default class VerificationRequest extends EventEmitter { if (this.phase === PHASE_UNSENT) { this._initiatedByMe = sentByMe; } - if (!this._verifier) { + if (!this._verifier && isLiveEvent) { this._verifier = this._createVerifier(method, event); } this._setPhase(PHASE_STARTED); From cf42ad83da259722ce6a3628622e4f4763d589a5 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 27 Dec 2019 18:02:55 +0100 Subject: [PATCH 17/43] WIP historical --- src/crypto/index.js | 24 +- .../verification/request/InRoomChannel.js | 6 +- .../request/VerificationRequest.js | 282 +++++++++--------- 3 files changed, 167 insertions(+), 145 deletions(-) diff --git a/src/crypto/index.js b/src/crypto/index.js index 6c609b7dd..d5ad3da81 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -1128,17 +1128,13 @@ Crypto.prototype.registerEventHandlers = function(eventEmitter) { } }); - eventEmitter.on("toDeviceEvent", function(event) { - crypto._onToDeviceEvent(event); - }); + eventEmitter.on("toDeviceEvent", crypto._onToDeviceEvent.bind(crypto)); - eventEmitter.on("Room.timeline", function(event) { - crypto._onTimelineEvent(event); - }); + const timelineHandler = crypto._onTimelineEvent.bind(crypto); - eventEmitter.on("Event.decrypted", function(event) { - crypto._onTimelineEvent(event); - }); + eventEmitter.on("Room.timeline", timelineHandler); + + eventEmitter.on("Event.decrypted", timelineHandler); }; @@ -1561,6 +1557,7 @@ Crypto.prototype._requestVerificationWithChannel = async function( // but if the other party is really fast they could potentially respond to the // request before the server tells us the event got sent, and we would probably // create a new request object + console.log(`Crypto: adding new request to requestsByTxnId with id ${channel.transactionId}`); requestsMap.setRequestByChannel(channel, request); return request; @@ -2468,11 +2465,16 @@ Crypto.prototype._onKeyVerificationMessage = function(event) { * @param {bool} removed not used * @param {bool} data.liveEvent whether this is a live event */ -Crypto.prototype._onTimelineEvent = function(event, room, atStart, removed, {liveEvent}) { +Crypto.prototype._onTimelineEvent = function(event, room, atStart, removed, {liveEvent} = {}) { // TODO: we still need a request object for past requests, so we can show it in the timeline // validation now excludes old requests if (!InRoomChannel.validateEvent(event, this._baseApis)) { + if (InRoomChannel.getTransactionId(event)) { + console.log(`Crypto: _onTimelineEvent NONVALID ${InRoomChannel.getTransactionId(event)}, ${event.getType()}, ${event.getSender()} liveEvent=${liveEvent}`); + } return; + } else { + console.log(`Crypto: _onTimelineEvent YESVALID ${InRoomChannel.getTransactionId(event)}, ${event.getType()}, ${event.getSender()} liveEvent=${liveEvent}`); } const createRequest = event => { if (!InRoomChannel.canCreateRequest(InRoomChannel.getEventType(event))) { @@ -2508,6 +2510,7 @@ Crypto.prototype._handleVerificationEvent = async function( request = createRequest(event); // a request could not be made from this event, so ignore event if (!request) { + console.log(`Crypto: could not find VerificationRequest for ${event.getType()}, and could not create one, so ignoring.`); return; } isNewRequest = true; @@ -2524,6 +2527,7 @@ Crypto.prototype._handleVerificationEvent = async function( } catch (err) { console.error("error while handling verification event", event, err); } + console.log("Crypto: _handleVerificationEvent: done handling a request", isNewRequest, request.pending, request.phase, request.initiatedByMe); if (!request.pending) { requestsMap.removeRequest(event); } else if (isNewRequest && !request.initiatedByMe) { diff --git a/src/crypto/verification/request/InRoomChannel.js b/src/crypto/verification/request/InRoomChannel.js index 3ccb06270..1e5f66ac3 100644 --- a/src/crypto/verification/request/InRoomChannel.js +++ b/src/crypto/verification/request/InRoomChannel.js @@ -134,7 +134,7 @@ export class InRoomChannel { // ignore requests that are not direct to or sent by the syncing user if (!InRoomChannel.getOtherPartyUserId(event, client)) { - console.log("InRoomChannel: validateEvent: not directed or sent my me", type, event.getSender(), content.to); + console.log("InRoomChannel: validateEvent: not directed or sent by me", type, event.getSender(), content.to); return false; } } @@ -280,9 +280,11 @@ export class InRoomRequests { getRequest(event) { const roomId = event.getRoomId(); + const txnId = InRoomChannel.getTransactionId(event); + console.log(`looking for request in room ${roomId} with txnId ${txnId} for an ${event.getType()} from ${event.getSender()}...`); const requestsByTxnId = this._requestsByRoomId.get(roomId); if (requestsByTxnId) { - return requestsByTxnId.get(InRoomChannel.getTransactionId(event)); + return requestsByTxnId.get(txnId); } } diff --git a/src/crypto/verification/request/VerificationRequest.js b/src/crypto/verification/request/VerificationRequest.js index 4daf49fbc..12ccaa6c7 100644 --- a/src/crypto/verification/request/VerificationRequest.js +++ b/src/crypto/verification/request/VerificationRequest.js @@ -65,13 +65,9 @@ export default class VerificationRequest extends EventEmitter { this._client = client; this._commonMethods = []; this._setPhase(PHASE_UNSENT, false); - this._requestEvent = null; - this._initiatedByMe = null; - this._startTimestamp = null; - this._cancellingUserId = null; - this._readyEvent = null; - this._doneByThem = false; - this._doneByUs = false; + this._eventsByUs = new Map(); + this._eventsByThem = new Map(); + this._observeOnly = false; } /** @@ -180,6 +176,7 @@ export default class VerificationRequest extends EventEmitter { /** whether this request has sent it's initial event and needs more events to complete */ get pending() { + // TODO: if we remove local echo, PHASE_UNSENT should not be considered non-pending. return this._phase !== PHASE_UNSENT && this._phase !== PHASE_DONE && this._phase !== PHASE_CANCELLED; @@ -213,6 +210,7 @@ export default class VerificationRequest extends EventEmitter { /** the user id of the other party in this request */ get otherUserId() { + // TODO: make sure this can be read from the first event passed to handleEvent return this.channel.userId; } @@ -224,6 +222,10 @@ export default class VerificationRequest extends EventEmitter { return this._cancellingUserId; } + get observeOnly() { + return this._observeOnly; + } + /* Start the key verification, creating a verifier and sending a .start event. * If no previous events have been sent, pass in `targetDevice` to set who to direct this request to. * @param {string} method the name of the verification method to use. @@ -233,7 +235,8 @@ export default class VerificationRequest extends EventEmitter { */ beginKeyVerification(method, targetDevice = null) { // need to allow also when unsent in case of to_device - if (!this._verifier) { + if (!this._observeOnly && !this._verifier) { + // TODO if (this._hasValidPreStartPhase()) { // when called on a request that was initiated with .request event // check the method is supported by both sides @@ -254,12 +257,10 @@ export default class VerificationRequest extends EventEmitter { * @returns {Promise} resolves when the event has been sent. */ async sendRequest() { - if (this._phase === PHASE_UNSENT) { + if (!this._observeOnly && this._phase === PHASE_UNSENT) { this._initiatedByMe = true; - this._setPhase(PHASE_REQUESTED, false); const methods = [...this._verificationMethods.keys()]; await this.channel.send(REQUEST_TYPE, {methods}); - this.emit("change"); } } @@ -270,15 +271,13 @@ export default class VerificationRequest extends EventEmitter { * @returns {Promise} resolves when the event has been sent. */ async cancel({reason = "User declined", code = "m.user"} = {}) { - if (this._phase !== PHASE_CANCELLED) { + if (!this._observeOnly && this._phase !== PHASE_CANCELLED) { if (this._verifier) { return this._verifier.cancel(errorFactory(code, reason)); } else { this._cancellingUserId = this._client.getUserId(); - this._setPhase(PHASE_CANCELLED, false); await this.channel.send(CANCEL_TYPE, {code, reason}); } - this.emit("change"); } } @@ -287,11 +286,9 @@ export default class VerificationRequest extends EventEmitter { * @returns {Promise} resolves when the event has been sent. */ async accept() { - if (this.phase === PHASE_REQUESTED && !this.initiatedByMe) { - this._setPhase(PHASE_READY, false); + if (!this._observeOnly && this.phase === PHASE_REQUESTED && !this.initiatedByMe) { const methods = [...this._verificationMethods.keys()]; await this.channel.send(READY_TYPE, {methods}); - this.emit("change"); } } @@ -331,29 +328,129 @@ export default class VerificationRequest extends EventEmitter { } } + _getEventByEither(type) { + return this._eventsByThem.get(type) || this._eventsByUs.get(type); + } + + _getEventByOther(type, notSender) { + if (notSender === this._client.getUserId()) { + return this._eventsByThem.get(type); + } else { + return this._eventsByUs.get(type); + } + } + + _getEventBy(type, sender) { + if (sender === this._client.getUserId()) { + return this._eventsByUs.get(type); + } else { + return this._eventsByThem.get(type); + } + } + + _calculatePhaseTransitions() { + const transitions = [{phase: PHASE_UNSENT}]; + const phase = () => transitions[transitions.length - 1].phase; + + const cancelEvent = this._getEventByEither(CANCEL_TYPE); + if (cancelEvent) { + transitions.push({phase: PHASE_CANCELLED, event: cancelEvent}); + return transitions; + } + + const requestEvent = this._getEventByEither(REQUEST_TYPE); + if (requestEvent) { + transitions.push({phase: PHASE_REQUESTED, event: requestEvent}); + + const readyEvent = + this._getEventByOther(READY_TYPE, requestEvent.getSender()); + if (readyEvent) { + transitions.push({phase: PHASE_READY, event: readyEvent}); + } + } + + const startEvent = this._getEventByEither(START_TYPE); + if (startEvent) { + const fromRequestPhase = phase() === PHASE_REQUESTED && + requestEvent.getSender() !== startEvent.getSender(); + const fromUnsentPhase = phase() === PHASE_UNSENT && + this.channel.constructor.canCreateRequest(START_TYPE); + if (fromRequestPhase || phase() === PHASE_READY || fromUnsentPhase) { + transitions.push({phase: START_TYPE, event: startEvent}); + } + } + + const ourDoneEvent = this._eventsByUs[DONE_TYPE]; + const theirDoneEvent = this._eventsByThem[DONE_TYPE]; + if (ourDoneEvent && theirDoneEvent && phase() === START_TYPE) { + transitions.push({phase: PHASE_DONE}); + } + + return transitions; + } + + _transitionToPhase(transition) { + const {phase, event} = transition; + // get common methods + if (phase === PHASE_REQUESTED || phase === PHASE_READY) { + if (!this._wasSentByOwnDevice(event)) { + const content = event.getContent(); + this._commonMethods = + content.methods.filter(m => this._verificationMethods.has(m)); + } + } + // detect if we're not a party in the request, and we should just observe + if (!this._observeOnly) { + if (phase === PHASE_REQUESTED) { + // if requested by one of my other devices + if (this._wasSentByOwnUser(event) && !this._wasSentByOwnDevice(event)) { + this._observeOnly = true; + } + } else if (phase === PHASE_STARTED || phase === PHASE_READY) { + this._observeOnly = !this._wasSentByOwnDevice(event); + } + } + // create verifier + if (phase === PHASE_STARTED) { + const {method} = event.getContent(); + if (!this._verifier && !this._observeOnly) { + this._verifier = this._createVerifier(method, event); + } + this._handleStart(transition); + } + } + /** * Changes the state of the request and verifier in response to a key verification event. * @param {string} type the "symbolic" event type, as returned by the `getEventType` function on the channel. * @param {MatrixEvent} event the event to handle. Don't call getType() on it but use the `type` parameter instead. - * @param {number} timestamp the timestamp in milliseconds when this event was sent. + * @param {bool} isLiveEvent whether this is an even received through sync or not * @returns {Promise} a promise that resolves when any requests as an anwser to the passed-in event are sent. */ - async handleEvent(type, event, timestamp, isLiveEvent) { - const sender = event.getSender(); - const content = event.getContent(); - if (type === REQUEST_TYPE || type === START_TYPE) { - if (this._startTimestamp === null) { - this._startTimestamp = timestamp; - } - } - if (type === REQUEST_TYPE) { - await this._handleRequest(content, event); - } else if (type === READY_TYPE) { - await this._handleReady(event); - } else if (type === START_TYPE) { - await this._handleStart(content, event, isLiveEvent); + async handleEvent(type, event, isLiveEvent) { + // don't send out events for historical requests + if (!isLiveEvent) { + this._observeOnly = true; } + const sender = event.getSender(); + const isOurs = sender === this._client.getUserId(); + const isTheirs = sender === this.otherUserId; + + if (isOurs) { + this._eventsByUs.set(type, event); + } else if (isTheirs) { + this._eventsByThem.set(type, event); + } + + const transitions = this._calculatePhaseTransitions(); + const existingIdx = transitions.findIndex(t => t.phase === this._phase); + // trim off phases we already went through, if any + const newTransitions = transitions.slice(existingIdx + 1); + // transition to all new phases + for (const transition of newTransitions) { + this._transitionToPhase(transition); + } // only pass events from the other side to the verifier, // no remote echos of our own events if (this._verifier && sender === this.otherUserId) { @@ -363,114 +460,34 @@ export default class VerificationRequest extends EventEmitter { } } - if (type === CANCEL_TYPE) { - this._handleCancel(event); - } else if (type === DONE_TYPE) { - this._handleDone(event); + if (newTransitions.length) { + const lastTransition = newTransitions[newTransitions.length - 1]; + this._setPhase(lastTransition.phase); } - } - async _handleRequest(content, event) { - if (this._phase === PHASE_UNSENT) { - const otherMethods = content.methods; - this._commonMethods = this._filterMethods(otherMethods); - this._requestEvent = event; - this._initiatedByMe = this._wasSentByMe(event); - this._setPhase(PHASE_REQUESTED); - } else if (this._phase !== PHASE_REQUESTED) { + /* + // .request && .ready +if (!this._observeOnly && this._phase !== PHASE_REQUESTED) { logger.warn("Cancelling, unexpected .request verification event from " + event.getSender()); await this.cancel(errorFromEvent(newUnexpectedMessageError())); } - } - async _handleReady(event) { - const content = event.getContent(); - console.log("handling ready event", content, this._phase); - if (this.phase === PHASE_REQUESTED || this.phase === PHASE_READY) { - this._readyEvent = event; - const otherMethods = content.methods; - this._commonMethods = this._filterMethods(otherMethods); - this._setPhase(PHASE_READY); - } else { - logger.warn("Cancelling, unexpected .ready verification event from " + - event.getSender()); - await this.cancel(errorFromEvent(newUnexpectedMessageError())); - } - } - _hasValidPreStartPhase() { - return this._phase === PHASE_REQUESTED || this._phase === PHASE_READY || - ( - this.channel.constructor.canCreateRequest(START_TYPE) && - this._phase === PHASE_UNSENT - ); - } - - async _handleStart(content, event, isLiveEvent) { - if (this._hasValidPreStartPhase()) { - const sentByMe = this._wasSentByMe(event); - const {method} = content; + cancel on handle unexpected events (only if !this._observeOnly): + const sentByMe = this._wasSentByOwnDevice(event); if (!sentByMe && !this._verificationMethods.has(method)) { await this.cancel(errorFromEvent(newUnknownMethodError())); return; } - // if not in requested phase - if (this.phase === PHASE_UNSENT) { - this._initiatedByMe = sentByMe; - } - if (!this._verifier && isLiveEvent) { - this._verifier = this._createVerifier(method, event); - } - this._setPhase(PHASE_STARTED); - } - } + */ - /** - * Called by RequestCallbackChannel when the verifier sends an event - * @param {string} type the "symbolic" event type - * @param {object} content the completed or uncompleted content for the event to be sent - */ - handleVerifierSend(type, content) { - if (type === CANCEL_TYPE) { - this._handleCancel(); - } else if (type === START_TYPE && this._hasValidPreStartPhase()) { - // if unsent, we're sending a (first) .start event and hence requesting the verification. - // in any other situation, the request was initiated by the other party. - this._initiatedByMe = this.phase === PHASE_UNSENT; - this._setPhase(PHASE_STARTED); - } - } - - _handleCancel(event = null) { - if (this._phase !== PHASE_CANCELLED) { - this._cancellingUserId = event ? event.getSender() : this._client.getUserId(); - this._setPhase(PHASE_CANCELLED); - } - } - - _handleDone(event) { - if (this._phase === PHASE_STARTED) { - const sender = event.getSender(); - if (sender === this._client.getUserId()) { - this._doneByUs = true; - console.log("VerificationRequest: received own .done event"); - } else if (sender === this.otherUserId) { - this._doneByThem = true; - console.log("VerificationRequest: received their .done event"); - } - // we need both .done events to consider the request done, - // as it will be deleted from the requests map once it's done, - // and we need to find the request on our own incoming .done - // to attach the request to the event to render it in the timeline. - if (this._doneByUs && this._doneByThem) { - this._setPhase(PHASE_DONE); - } + console.log("VerificationRequest: handleEvent", event.getSender(), event.getType(), event, isLiveEvent); } } _createVerifier(method, startEvent = null, targetDevice = null) { - const initiatedByMe = !startEvent || this._wasSentByMe(startEvent); + const initiatedByMe = !startEvent || this._wasSentByOwnDevice(startEvent); const {userId, deviceId} = this._getVerifierTarget(startEvent, targetDevice); const VerifierCtor = this._verificationMethods.get(method); @@ -478,10 +495,8 @@ export default class VerificationRequest extends EventEmitter { console.warn("could not find verifier constructor for method", method); return; } - // invokes handleVerifierSend when verifier sends something - const callbackMedium = new RequestCallbackChannel(this, this.channel); return new VerifierCtor( - callbackMedium, + this.channel, this._client, userId, deviceId, @@ -496,17 +511,18 @@ export default class VerificationRequest extends EventEmitter { return targetDevice; } else { let targetEvent; - if (startEvent && !this._wasSentByMe(startEvent)) { + if (startEvent && !this._wasSentByOwnDevice(startEvent)) { targetEvent = startEvent; - } else if (this._readyEvent && !this._wasSentByMe(this._readyEvent)) { + } else if (this._readyEvent && !this._wasSentByOwnDevice(this._readyEvent)) { targetEvent = this._readyEvent; - } else if (this._requestEvent && !this._wasSentByMe(this._requestEvent)) { + } else if (this._requestEvent && !this._wasSentByOwnDevice(this._requestEvent)) { targetEvent = this._requestEvent; } else { throw new Error( "can't determine who the verifier should be targeted at. " + "No .request or .start event and no targetDevice"); } + // TODO: could be replaced by otherUserId const userId = targetEvent.getSender(); const content = targetEvent.getContent(); const deviceId = content && content.from_device; @@ -514,13 +530,13 @@ export default class VerificationRequest extends EventEmitter { } } - _filterMethods(methodNames) { - return methodNames.filter(m => this._verificationMethods.has(m)); + _wasSentByOwnUser(event) { + return event.getSender() === this._client.getUserId(); } // only for .request, .ready or .start - _wasSentByMe(event) { - if (event.getSender() !== this._client.getUserId()) { + _wasSentByOwnDevice(event) { + if (!this._wasSentByOwnUser(event)) { return false; } const content = event.getContent(); From 0d3d27a519e5113c5347b435da08057c69fb62b3 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 2 Jan 2020 16:49:38 +0100 Subject: [PATCH 18/43] fixes and cleanup for historical --- src/crypto/index.js | 23 ++- .../verification/request/InRoomChannel.js | 26 +-- .../request/RequestCallbackChannel.js | 59 ------- .../verification/request/ToDeviceChannel.js | 8 +- .../request/VerificationRequest.js | 163 ++++++++++++------ 5 files changed, 136 insertions(+), 143 deletions(-) delete mode 100644 src/crypto/verification/request/RequestCallbackChannel.js diff --git a/src/crypto/index.js b/src/crypto/index.js index d5ad3da81..6237cd852 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -2465,7 +2465,9 @@ Crypto.prototype._onKeyVerificationMessage = function(event) { * @param {bool} removed not used * @param {bool} data.liveEvent whether this is a live event */ -Crypto.prototype._onTimelineEvent = function(event, room, atStart, removed, {liveEvent} = {}) { +Crypto.prototype._onTimelineEvent = function( + event, room, atStart, removed, {liveEvent} = {}, +) { // TODO: we still need a request object for past requests, so we can show it in the timeline // validation now excludes old requests if (!InRoomChannel.validateEvent(event, this._baseApis)) { @@ -2477,18 +2479,9 @@ Crypto.prototype._onTimelineEvent = function(event, room, atStart, removed, {liv console.log(`Crypto: _onTimelineEvent YESVALID ${InRoomChannel.getTransactionId(event)}, ${event.getType()}, ${event.getSender()} liveEvent=${liveEvent}`); } const createRequest = event => { - if (!InRoomChannel.canCreateRequest(InRoomChannel.getEventType(event))) { - return; - } - const otherPartyUserId = - InRoomChannel.getOtherPartyUserId(event, this._baseApis); - if (!otherPartyUserId) { - return; - } const channel = new InRoomChannel( this._baseApis, event.getRoomId(), - otherPartyUserId, ); return new VerificationRequest( channel, this._verificationMethods, this._baseApis); @@ -2530,8 +2523,14 @@ Crypto.prototype._handleVerificationEvent = async function( console.log("Crypto: _handleVerificationEvent: done handling a request", isNewRequest, request.pending, request.phase, request.initiatedByMe); if (!request.pending) { requestsMap.removeRequest(event); - } else if (isNewRequest && !request.initiatedByMe) { - this._baseApis.emit("crypto.verification.request", request); + } else { + const shouldEmit = isNewRequest && + !request.initiatedByMe && + request.requested && + !request.observeOnly; + if (shouldEmit) { + this._baseApis.emit("crypto.verification.request", request); + } } }; diff --git a/src/crypto/verification/request/InRoomChannel.js b/src/crypto/verification/request/InRoomChannel.js index 1e5f66ac3..0fdb002a1 100644 --- a/src/crypto/verification/request/InRoomChannel.js +++ b/src/crypto/verification/request/InRoomChannel.js @@ -34,7 +34,7 @@ export class InRoomChannel { * @param {string} roomId id of the room where verification events should be posted in, should be a DM with the given user. * @param {string} userId id of user that the verification request is directed at, should be present in the room. */ - constructor(client, roomId, userId) { + constructor(client, roomId, userId = null) { this._client = client; this._roomId = roomId; this.userId = userId; @@ -58,7 +58,7 @@ export class InRoomChannel { static getOtherPartyUserId(event, client) { const type = InRoomChannel.getEventType(event); if (type !== REQUEST_TYPE) { - return; + return; } const ownUserId = client.getUserId(); const sender = event.getSender(); @@ -67,7 +67,7 @@ export class InRoomChannel { // request is not sent by or directed at us if (sender !== ownUserId && receiver !== ownUserId) { - return; + return sender; } if (sender === ownUserId) { return receiver; @@ -80,7 +80,7 @@ export class InRoomChannel { * @param {MatrixEvent} event the event to get the timestamp of * @return {number} the timestamp when the event was sent */ - static getTimestamp(event) { + getTimestamp(event) { return event.getTs(); } @@ -134,13 +134,12 @@ export class InRoomChannel { // ignore requests that are not direct to or sent by the syncing user if (!InRoomChannel.getOtherPartyUserId(event, client)) { - console.log("InRoomChannel: validateEvent: not directed or sent by me", type, event.getSender(), content.to); + console.log("InRoomChannel: validateEvent: not directed to or sent by me", type, event.getSender(), content.to); return false; } } - return VerificationRequest.validateEvent( - type, event, InRoomChannel.getTimestamp(event), client); + return VerificationRequest.validateEvent(type, event, client); } /** @@ -179,6 +178,13 @@ export class InRoomChannel { if (event.getRoomId() !== this._roomId) { return; } + // set userId if not set already + if (this.userId === null) { + const userId = InRoomChannel.getOtherPartyUserId(event, this._client); + if (userId) { + this.userId = userId; + } + } // ignore events not sent by us or the other party const ownUserId = this._client.getUserId(); const sender = event.getSender(); @@ -187,11 +193,11 @@ export class InRoomChannel { return; } // set transactionId when receiving a .request - if (!this._requestEventId && type === REQUEST_TYPE) { + if (this._requestEventId === null && type === REQUEST_TYPE) { this._requestEventId = event.getId(); } - return await request.handleEvent(type, event, InRoomChannel.getTimestamp(event), isLiveEvent); + return await request.handleEvent(type, event, isLiveEvent); } /** @@ -281,7 +287,7 @@ export class InRoomRequests { getRequest(event) { const roomId = event.getRoomId(); const txnId = InRoomChannel.getTransactionId(event); - console.log(`looking for request in room ${roomId} with txnId ${txnId} for an ${event.getType()} from ${event.getSender()}...`); +// console.log(`looking for request in room ${roomId} with txnId ${txnId} for an ${event.getType()} from ${event.getSender()}...`); const requestsByTxnId = this._requestsByRoomId.get(roomId); if (requestsByTxnId) { return requestsByTxnId.get(txnId); diff --git a/src/crypto/verification/request/RequestCallbackChannel.js b/src/crypto/verification/request/RequestCallbackChannel.js deleted file mode 100644 index f1faf79a9..000000000 --- a/src/crypto/verification/request/RequestCallbackChannel.js +++ /dev/null @@ -1,59 +0,0 @@ -/* -Copyright 2019 The Matrix.org Foundation C.I.C. - -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. -*/ - -/** a key verification channel that wraps over an actual channel to pass it to a verifier, - * to notify the VerificationRequest when the verifier tries to send anything over the channel. - * This way, the VerificationRequest can update its state based on events sent by the verifier. - * Anything that is not sending is just routing through to the wrapped channel. - */ -export default class RequestCallbackChannel { - constructor(request, channel) { - this._request = request; - this._channel = channel; - } - - get transactionId() { - return this._channel.transactionId; - } - - get needsDoneMessage() { - return this._channel.needsDoneMessage; - } - - handleEvent(event, request) { - return this._channel.handleEvent(event, request); - } - - completedContentFromEvent(event) { - return this._channel.completedContentFromEvent(event); - } - - completeContent(type, content) { - return this._channel.completeContent(type, content); - } - - async send(type, uncompletedContent) { - this._request.handleVerifierSend(type, uncompletedContent); - const result = await this._channel.send(type, uncompletedContent); - return result; - } - - async sendCompleted(type, content) { - this._request.handleVerifierSend(type, content); - const result = await this._channel.sendCompleted(type, content); - return result; - } -} diff --git a/src/crypto/verification/request/ToDeviceChannel.js b/src/crypto/verification/request/ToDeviceChannel.js index 3057d91e4..19caeefbf 100644 --- a/src/crypto/verification/request/ToDeviceChannel.js +++ b/src/crypto/verification/request/ToDeviceChannel.js @@ -106,15 +106,14 @@ export class ToDeviceChannel { } } - return VerificationRequest.validateEvent( - type, event, ToDeviceChannel.getTimestamp(event), client); + return VerificationRequest.validateEvent(type, event, client); } /** * @param {MatrixEvent} event the event to get the timestamp of * @return {number} the timestamp when the event was sent */ - static getTimestamp(event) { + getTimestamp(event) { const content = event.getContent(); return content && content.timestamp; } @@ -151,8 +150,7 @@ export class ToDeviceChannel { const wasStarted = request.phase === PHASE_STARTED || request.phase === PHASE_READY; - await request.handleEvent( - event.getType(), event, ToDeviceChannel.getTimestamp(event), isLiveEvent); + await request.handleEvent(event.getType(), event, isLiveEvent); const isStarted = request.phase === PHASE_STARTED || request.phase === PHASE_READY; diff --git a/src/crypto/verification/request/VerificationRequest.js b/src/crypto/verification/request/VerificationRequest.js index 12ccaa6c7..781397198 100644 --- a/src/crypto/verification/request/VerificationRequest.js +++ b/src/crypto/verification/request/VerificationRequest.js @@ -16,7 +16,6 @@ limitations under the License. */ import logger from '../../../logger'; -import RequestCallbackChannel from "./RequestCallbackChannel"; import {EventEmitter} from 'events'; import { newUnknownMethodError, @@ -75,11 +74,10 @@ export default class VerificationRequest extends EventEmitter { * Invoked by the same static method in either channel. * @param {string} type the "symbolic" event type, as returned by the `getEventType` function on the channel. * @param {MatrixEvent} event the event to validate. Don't call getType() on it but use the `type` parameter instead. - * @param {number} timestamp the timestamp in milliseconds when this event was sent. * @param {MatrixClient} client the client to get the current user and device id from * @returns {bool} whether the event is valid and should be passed to handleEvent */ - static validateEvent(type, event, timestamp, client) { + static validateEvent(type, event, client) { const content = event.getContent(); if (!content) { @@ -107,22 +105,26 @@ export default class VerificationRequest extends EventEmitter { } } - // a timestamp is not provided on all to_device events - if (Number.isFinite(timestamp)) { - const elapsed = Date.now() - timestamp; - // ignore if event is too far in the past or too far in the future - if (elapsed > (VERIFICATION_REQUEST_TIMEOUT - VERIFICATION_REQUEST_MARGIN) || - elapsed < -(VERIFICATION_REQUEST_TIMEOUT / 2) - ) { - console.log("VerificationRequest: validateEvent: verification event to far in future or past", type, elapsed); - logger.log("received verification that is too old or from the future"); - return false; - } - } + // // a timestamp is not provided on all to_device events + // if (Number.isFinite(timestamp)) { + // const elapsed = Date.now() - timestamp; + // // ignore if event is too far in the past or too far in the future + // if (elapsed > (VERIFICATION_REQUEST_TIMEOUT - VERIFICATION_REQUEST_MARGIN) || + // elapsed < -(VERIFICATION_REQUEST_TIMEOUT / 2) + // ) { + // console.log("VerificationRequest: validateEvent: verification event to far in future or past", type, elapsed); + // logger.log("received verification that is too old or from the future"); + // return false; + // } + // } return true; } + get invalid() { + return this.phase === PHASE_UNSENT; + } + /** returns whether the phase is PHASE_REQUESTED */ get requested() { return this.phase === PHASE_REQUESTED; @@ -161,7 +163,7 @@ export default class VerificationRequest extends EventEmitter { /** the m.key.verification.request event that started this request, provided for compatibility with previous verification code */ get event() { - return this._requestEvent; + return this._getEventByEither(REQUEST_TYPE) || this._getEventByEither(START_TYPE); } /** current phase of the request. Some properties might only be defined in a current phase. */ @@ -176,9 +178,7 @@ export default class VerificationRequest extends EventEmitter { /** whether this request has sent it's initial event and needs more events to complete */ get pending() { - // TODO: if we remove local echo, PHASE_UNSENT should not be considered non-pending. - return this._phase !== PHASE_UNSENT - && this._phase !== PHASE_DONE + return this._phase !== PHASE_DONE && this._phase !== PHASE_CANCELLED; } @@ -187,7 +187,25 @@ export default class VerificationRequest extends EventEmitter { * For ToDeviceChannel, this is who sent the .start event */ get initiatedByMe() { - return this._initiatedByMe; + // event created by us but no remote echo has been received yet + const noEventsYet = (this._eventsByUs.size + this._eventsByThem.size) === 0; + if (this._phase === PHASE_UNSENT && noEventsYet) { + return true; + } + const hasMyRequest = this._eventsByUs.has(REQUEST_TYPE); + const hasTheirRequest = this._eventsByThem.has(REQUEST_TYPE); + if (hasMyRequest && !hasTheirRequest) { + return true; + } + if (!hasMyRequest && hasTheirRequest) { + return false; + } + const hasMyStart = this._eventsByUs.has(START_TYPE); + const hasTheirStart = this._eventsByThem.has(START_TYPE); + if (hasMyStart && !hasTheirStart) { + return true; + } + return false; } /** the id of the user that initiated the request */ @@ -210,7 +228,6 @@ export default class VerificationRequest extends EventEmitter { /** the user id of the other party in this request */ get otherUserId() { - // TODO: make sure this can be read from the first event passed to handleEvent return this.channel.userId; } @@ -219,7 +236,16 @@ export default class VerificationRequest extends EventEmitter { * only defined when phase is PHASE_CANCELLED */ get cancellingUserId() { - return this._cancellingUserId; + const myCancel = this._eventsByUs.get(CANCEL_TYPE); + const theirCancel = this._eventsByThem.get(CANCEL_TYPE); + + if (myCancel && (!theirCancel || myCancel.getId() < theirCancel.getId())) { + return myCancel.getSender(); + } + if (theirCancel) { + return theirCancel.getSender(); + } + return undefined; } get observeOnly() { @@ -234,10 +260,15 @@ export default class VerificationRequest extends EventEmitter { * @returns {VerifierBase} the verifier of the given method */ beginKeyVerification(method, targetDevice = null) { + console.log("beginKeyVerification " + this._observeOnly); // need to allow also when unsent in case of to_device - if (!this._observeOnly && !this._verifier) { - // TODO - if (this._hasValidPreStartPhase()) { + if (!this.observeOnly && !this._verifier) { + const validStartPhase = + this.phase === PHASE_REQUESTED || + this.phase === PHASE_READY || + this.phase === (PHASE_UNSENT && + this.channel.constructor.canCreateRequest(START_TYPE)); + if (validStartPhase) { // when called on a request that was initiated with .request event // check the method is supported by both sides if (this._commonMethods.length && !this._commonMethods.includes(method)) { @@ -257,8 +288,7 @@ export default class VerificationRequest extends EventEmitter { * @returns {Promise} resolves when the event has been sent. */ async sendRequest() { - if (!this._observeOnly && this._phase === PHASE_UNSENT) { - this._initiatedByMe = true; + if (!this.observeOnly && this._phase === PHASE_UNSENT) { const methods = [...this._verificationMethods.keys()]; await this.channel.send(REQUEST_TYPE, {methods}); } @@ -271,7 +301,7 @@ export default class VerificationRequest extends EventEmitter { * @returns {Promise} resolves when the event has been sent. */ async cancel({reason = "User declined", code = "m.user"} = {}) { - if (!this._observeOnly && this._phase !== PHASE_CANCELLED) { + if (!this.observeOnly && this._phase !== PHASE_CANCELLED) { if (this._verifier) { return this._verifier.cancel(errorFactory(code, reason)); } else { @@ -286,7 +316,7 @@ export default class VerificationRequest extends EventEmitter { * @returns {Promise} resolves when the event has been sent. */ async accept() { - if (!this._observeOnly && this.phase === PHASE_REQUESTED && !this.initiatedByMe) { + if (!this.observeOnly && this.phase === PHASE_REQUESTED && !this.initiatedByMe) { const methods = [...this._verificationMethods.keys()]; await this.channel.send(READY_TYPE, {methods}); } @@ -352,37 +382,42 @@ export default class VerificationRequest extends EventEmitter { const transitions = [{phase: PHASE_UNSENT}]; const phase = () => transitions[transitions.length - 1].phase; + // always pass by .request first to be sure channel.userId has been set + const requestEvent = this._getEventByEither(REQUEST_TYPE); + if (requestEvent) { + transitions.push({phase: PHASE_REQUESTED, event: requestEvent}); + } else { + return transitions; + } + const cancelEvent = this._getEventByEither(CANCEL_TYPE); if (cancelEvent) { transitions.push({phase: PHASE_CANCELLED, event: cancelEvent}); return transitions; } - const requestEvent = this._getEventByEither(REQUEST_TYPE); - if (requestEvent) { - transitions.push({phase: PHASE_REQUESTED, event: requestEvent}); - - const readyEvent = - this._getEventByOther(READY_TYPE, requestEvent.getSender()); - if (readyEvent) { - transitions.push({phase: PHASE_READY, event: readyEvent}); - } + const readyEvent = + this._getEventByOther(READY_TYPE, requestEvent.getSender()); + if (readyEvent) { + transitions.push({phase: PHASE_READY, event: readyEvent}); } - const startEvent = this._getEventByEither(START_TYPE); + const startEvent = readyEvent ? + this._getEventByEither(START_TYPE) : // any party can send .start after a .ready + this._getEventByOther(START_TYPE, requestEvent.getSender()); if (startEvent) { const fromRequestPhase = phase() === PHASE_REQUESTED && requestEvent.getSender() !== startEvent.getSender(); const fromUnsentPhase = phase() === PHASE_UNSENT && this.channel.constructor.canCreateRequest(START_TYPE); if (fromRequestPhase || phase() === PHASE_READY || fromUnsentPhase) { - transitions.push({phase: START_TYPE, event: startEvent}); + transitions.push({phase: PHASE_STARTED, event: startEvent}); } } - const ourDoneEvent = this._eventsByUs[DONE_TYPE]; - const theirDoneEvent = this._eventsByThem[DONE_TYPE]; - if (ourDoneEvent && theirDoneEvent && phase() === START_TYPE) { + const ourDoneEvent = this._eventsByUs.get(DONE_TYPE); + const theirDoneEvent = this._eventsByThem.get(DONE_TYPE); + if (ourDoneEvent && theirDoneEvent && phase() === PHASE_STARTED) { transitions.push({phase: PHASE_DONE}); } @@ -400,23 +435,24 @@ export default class VerificationRequest extends EventEmitter { } } // detect if we're not a party in the request, and we should just observe - if (!this._observeOnly) { - if (phase === PHASE_REQUESTED) { + if (!this.observeOnly) { + // if requested or accepted by one of my other devices + if (phase === PHASE_REQUESTED || + phase === PHASE_STARTED || + phase === PHASE_READY + ) { // if requested by one of my other devices if (this._wasSentByOwnUser(event) && !this._wasSentByOwnDevice(event)) { this._observeOnly = true; } - } else if (phase === PHASE_STARTED || phase === PHASE_READY) { - this._observeOnly = !this._wasSentByOwnDevice(event); } } // create verifier if (phase === PHASE_STARTED) { const {method} = event.getContent(); - if (!this._verifier && !this._observeOnly) { + if (!this._verifier && !this.observeOnly) { this._verifier = this._createVerifier(method, event); } - this._handleStart(transition); } } @@ -432,6 +468,7 @@ export default class VerificationRequest extends EventEmitter { if (!isLiveEvent) { this._observeOnly = true; } + // TODO: check if event is too old/far into the future?, set this._observeOnly const sender = event.getSender(); const isOurs = sender === this._client.getUserId(); @@ -444,7 +481,7 @@ export default class VerificationRequest extends EventEmitter { } const transitions = this._calculatePhaseTransitions(); - const existingIdx = transitions.findIndex(t => t.phase === this._phase); + const existingIdx = transitions.findIndex(t => t.phase === this.phase); // trim off phases we already went through, if any const newTransitions = transitions.slice(existingIdx + 1); // transition to all new phases @@ -467,14 +504,14 @@ export default class VerificationRequest extends EventEmitter { /* // .request && .ready -if (!this._observeOnly && this._phase !== PHASE_REQUESTED) { +if (!this.observeOnly && this._phase !== PHASE_REQUESTED) { logger.warn("Cancelling, unexpected .request verification event from " + event.getSender()); await this.cancel(errorFromEvent(newUnexpectedMessageError())); } - cancel on handle unexpected events (only if !this._observeOnly): + cancel on handle unexpected events (only if !this.observeOnly): const sentByMe = this._wasSentByOwnDevice(event); if (!sentByMe && !this._verificationMethods.has(method)) { await this.cancel(errorFromEvent(newUnknownMethodError())); @@ -482,13 +519,25 @@ if (!this._observeOnly && this._phase !== PHASE_REQUESTED) { } */ - console.log("VerificationRequest: handleEvent", event.getSender(), event.getType(), event, isLiveEvent); - } + const url = `http://localhost:8080/#/room/${encodeURIComponent(event.getRoomId())}/${encodeURIComponent(event.getId())}`; + console.log("VerificationRequest: handleEvent", isLiveEvent, url, event); } _createVerifier(method, startEvent = null, targetDevice = null) { - const initiatedByMe = !startEvent || this._wasSentByOwnDevice(startEvent); - const {userId, deviceId} = this._getVerifierTarget(startEvent, targetDevice); + const startedByMe = !startEvent || this._wasSentByOwnDevice(startEvent); + if (!targetDevice) { + const theirFirstEvent = + this._eventsByThem.get(REQUEST_TYPE) || + this._eventsByThem.get(READY_TYPE) || + this._eventsByThem.get(START_TYPE); + const theirFirstContent = theirFirstEvent.getContent(); + const fromDevice = theirFirstContent.from_device; + targetDevice = { + userId: this.otherUserId, + deviceId: fromDevice, + }; + } + const {userId, deviceId} = targetDevice; const VerifierCtor = this._verificationMethods.get(method); if (!VerifierCtor) { @@ -500,7 +549,7 @@ if (!this._observeOnly && this._phase !== PHASE_REQUESTED) { this._client, userId, deviceId, - initiatedByMe ? null : startEvent, + startedByMe ? null : startEvent, ); } From 57135a898f2b0ee57359ba5ff754c713ccdc6587 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 3 Jan 2020 13:31:56 +0100 Subject: [PATCH 19/43] don't mark events loaded from cache as live events this makes the verifier want to interact with the other party when just reloading the session. --- src/models/event-timeline-set.js | 10 ++++++---- src/models/room.js | 10 ++++++---- src/sync.js | 8 +++++--- 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/models/event-timeline-set.js b/src/models/event-timeline-set.js index 36f546bc2..1f250600b 100644 --- a/src/models/event-timeline-set.js +++ b/src/models/event-timeline-set.js @@ -488,8 +488,9 @@ EventTimelineSet.prototype.addEventsToTimeline = function(events, toStartOfTimel * * @param {MatrixEvent} event Event to be added * @param {string?} duplicateStrategy 'ignore' or 'replace' + * @param {boolean} fromCache whether the sync response came from cache */ -EventTimelineSet.prototype.addLiveEvent = function(event, duplicateStrategy) { +EventTimelineSet.prototype.addLiveEvent = function(event, duplicateStrategy, fromCache) { if (this._filter) { const events = this._filter.filterRoomTimeline([event]); if (!events.length) { @@ -527,7 +528,7 @@ EventTimelineSet.prototype.addLiveEvent = function(event, duplicateStrategy) { return; } - this.addEventToTimeline(event, this._liveTimeline, false); + this.addEventToTimeline(event, this._liveTimeline, false, fromCache); }; /** @@ -539,11 +540,12 @@ EventTimelineSet.prototype.addLiveEvent = function(event, duplicateStrategy) { * @param {MatrixEvent} event * @param {EventTimeline} timeline * @param {boolean} toStartOfTimeline + * @param {boolean} fromCache whether the sync response came from cache * * @fires module:client~MatrixClient#event:"Room.timeline" */ EventTimelineSet.prototype.addEventToTimeline = function(event, timeline, - toStartOfTimeline) { + toStartOfTimeline, fromCache) { const eventId = event.getId(); timeline.addEvent(event, toStartOfTimeline); this._eventIdToTimeline[eventId] = timeline; @@ -553,7 +555,7 @@ EventTimelineSet.prototype.addEventToTimeline = function(event, timeline, const data = { timeline: timeline, - liveEvent: !toStartOfTimeline && timeline == this._liveTimeline, + liveEvent: !toStartOfTimeline && timeline == this._liveTimeline && !fromCache, }; this.emit("Room.timeline", event, this.room, Boolean(toStartOfTimeline), false, data); diff --git a/src/models/room.js b/src/models/room.js index 77f12e845..8ef3b27b1 100644 --- a/src/models/room.js +++ b/src/models/room.js @@ -1069,10 +1069,11 @@ Room.prototype.removeFilteredTimelineSet = function(filter) { * * @param {MatrixEvent} event Event to be added * @param {string?} duplicateStrategy 'ignore' or 'replace' + * @param {boolean} fromCache whether the sync response came from cache * @fires module:client~MatrixClient#event:"Room.timeline" * @private */ -Room.prototype._addLiveEvent = function(event, duplicateStrategy) { +Room.prototype._addLiveEvent = function(event, duplicateStrategy, fromCache) { if (event.isRedaction()) { const redactId = event.event.redacts; @@ -1119,7 +1120,7 @@ Room.prototype._addLiveEvent = function(event, duplicateStrategy) { // add to our timeline sets for (let i = 0; i < this._timelineSets.length; i++) { - this._timelineSets[i].addLiveEvent(event, duplicateStrategy); + this._timelineSets[i].addLiveEvent(event, duplicateStrategy, fromCache); } // synthesize and inject implicit read receipts @@ -1429,9 +1430,10 @@ Room.prototype._revertRedactionLocalEcho = function(redactionEvent) { * this function will be ignored entirely, preserving the existing event in the * timeline. Events are identical based on their event ID only. * + * @param {boolean} fromCache whether the sync response came from cache * @throws If duplicateStrategy is not falsey, 'replace' or 'ignore'. */ -Room.prototype.addLiveEvents = function(events, duplicateStrategy) { +Room.prototype.addLiveEvents = function(events, duplicateStrategy, fromCache) { let i; if (duplicateStrategy && ["replace", "ignore"].indexOf(duplicateStrategy) === -1) { throw new Error("duplicateStrategy MUST be either 'replace' or 'ignore'"); @@ -1457,7 +1459,7 @@ Room.prototype.addLiveEvents = function(events, duplicateStrategy) { for (i = 0; i < events.length; i++) { // TODO: We should have a filter to say "only add state event // types X Y Z to the timeline". - this._addLiveEvent(events[i], duplicateStrategy); + this._addLiveEvent(events[i], duplicateStrategy, fromCache); } }; diff --git a/src/sync.js b/src/sync.js index 80302ec1e..e2f7c6295 100644 --- a/src/sync.js +++ b/src/sync.js @@ -688,6 +688,7 @@ SyncApi.prototype._syncFromCache = async function(savedSync) { oldSyncToken: null, nextSyncToken, catchingUp: false, + fromCache: true, }; const data = { @@ -1237,7 +1238,7 @@ SyncApi.prototype._processSyncResponse = async function( } } - self._processRoomEvents(room, stateEvents, timelineEvents); + self._processRoomEvents(room, stateEvents, timelineEvents, syncEventData.fromCache); // set summary after processing events, // because it will trigger a name calculation @@ -1564,10 +1565,11 @@ SyncApi.prototype._resolveInvites = function(room) { * @param {MatrixEvent[]} stateEventList A list of state events. This is the state * at the *START* of the timeline list if it is supplied. * @param {MatrixEvent[]} [timelineEventList] A list of timeline events. Lower index + * @param {boolean} fromCache whether the sync response came from cache * is earlier in time. Higher index is later. */ SyncApi.prototype._processRoomEvents = function(room, stateEventList, - timelineEventList) { + timelineEventList, fromCache) { // If there are no events in the timeline yet, initialise it with // the given state events const liveTimeline = room.getLiveTimeline(); @@ -1621,7 +1623,7 @@ SyncApi.prototype._processRoomEvents = function(room, stateEventList, // if the timeline has any state events in it. // This also needs to be done before running push rules on the events as they need // to be decorated with sender etc. - room.addLiveEvents(timelineEventList || []); + room.addLiveEvents(timelineEventList || [], null, fromCache); }; /** From 8ed51c806e06f2213f306c03682dd60d0f5f2fa3 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 3 Jan 2020 13:32:47 +0100 Subject: [PATCH 20/43] don't cancel or timeout when verify isn't called --- src/crypto/verification/Base.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/crypto/verification/Base.js b/src/crypto/verification/Base.js index 9720f4dc9..ab720fe6c 100644 --- a/src/crypto/verification/Base.js +++ b/src/crypto/verification/Base.js @@ -67,9 +67,6 @@ export default class VerificationBase extends EventEmitter { this._done = false; this._promise = null; this._transactionTimeoutTimer = null; - - // At this point, the verification request was received so start the timeout timer. - this._resetTimer(); } _resetTimer() { @@ -123,7 +120,11 @@ export default class VerificationBase extends EventEmitter { const reject = this._reject; this._reject = undefined; reject(new Error("Other side cancelled verification")); - } else { + } else if (this._expectedEvent) { + // only cancel if there is an event expected. + // if there is no event expected, it means verify() wasn't called + // and we're just replaying the timeline events when syncing + // after a refresh when the events haven't been stored in the cache yet. const exception = new Error( "Unexpected message: expecting " + this._expectedEvent + " but got " + e.getType(), From 3ec8233a2ddcea305058a8b32a7d4abd10fdb576 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 3 Jan 2020 13:33:17 +0100 Subject: [PATCH 21/43] fixes & implement timeout --- src/crypto/index.js | 22 +- src/crypto/verification/Error.js | 6 +- .../verification/request/InRoomChannel.js | 13 +- .../request/VerificationRequest.js | 193 +++++++++++------- 4 files changed, 130 insertions(+), 104 deletions(-) diff --git a/src/crypto/index.js b/src/crypto/index.js index 6237cd852..5b2a83e00 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -2471,12 +2471,7 @@ Crypto.prototype._onTimelineEvent = function( // TODO: we still need a request object for past requests, so we can show it in the timeline // validation now excludes old requests if (!InRoomChannel.validateEvent(event, this._baseApis)) { - if (InRoomChannel.getTransactionId(event)) { - console.log(`Crypto: _onTimelineEvent NONVALID ${InRoomChannel.getTransactionId(event)}, ${event.getType()}, ${event.getSender()} liveEvent=${liveEvent}`); - } return; - } else { - console.log(`Crypto: _onTimelineEvent YESVALID ${InRoomChannel.getTransactionId(event)}, ${event.getType()}, ${event.getSender()} liveEvent=${liveEvent}`); } const createRequest = event => { const channel = new InRoomChannel( @@ -2520,17 +2515,12 @@ Crypto.prototype._handleVerificationEvent = async function( } catch (err) { console.error("error while handling verification event", event, err); } - console.log("Crypto: _handleVerificationEvent: done handling a request", isNewRequest, request.pending, request.phase, request.initiatedByMe); - if (!request.pending) { - requestsMap.removeRequest(event); - } else { - const shouldEmit = isNewRequest && - !request.initiatedByMe && - request.requested && - !request.observeOnly; - if (shouldEmit) { - this._baseApis.emit("crypto.verification.request", request); - } + const shouldEmit = isNewRequest && + !request.initiatedByMe && + request.requested && // check it is in requested phase + !request.observeOnly; + if (shouldEmit) { + this._baseApis.emit("crypto.verification.request", request); } }; diff --git a/src/crypto/verification/Error.js b/src/crypto/verification/Error.js index 3ce1a49d7..5fe9fde4c 100644 --- a/src/crypto/verification/Error.js +++ b/src/crypto/verification/Error.js @@ -23,12 +23,10 @@ limitations under the License. import {MatrixEvent} from "../../models/event"; export function newVerificationError(code, reason, extradata) { - extradata = extradata || {}; - extradata.code = code; - extradata.reason = reason; + const content = Object.assign({}, {code, reason}, extradata); return new MatrixEvent({ type: "m.key.verification.cancel", - content: extradata, + content, }); } diff --git a/src/crypto/verification/request/InRoomChannel.js b/src/crypto/verification/request/InRoomChannel.js index 0fdb002a1..9cf6d8b31 100644 --- a/src/crypto/verification/request/InRoomChannel.js +++ b/src/crypto/verification/request/InRoomChannel.js @@ -188,13 +188,14 @@ export class InRoomChannel { // ignore events not sent by us or the other party const ownUserId = this._client.getUserId(); const sender = event.getSender(); - if (sender !== ownUserId && sender !== this.userId) { - console.log(`InRoomChannel: ignoring verification event from non-participating sender ${sender}`); - return; + if (this.userId !== null) { + if (sender !== ownUserId && sender !== this.userId) { + console.log(`InRoomChannel: ignoring verification event from non-participating sender ${sender}`); + return; + } } - // set transactionId when receiving a .request - if (this._requestEventId === null && type === REQUEST_TYPE) { - this._requestEventId = event.getId(); + if (this._requestEventId === null) { + this._requestEventId = InRoomChannel.getTransactionId(event); } return await request.handleEvent(type, event, isLiveEvent); diff --git a/src/crypto/verification/request/VerificationRequest.js b/src/crypto/verification/request/VerificationRequest.js index 781397198..821608728 100644 --- a/src/crypto/verification/request/VerificationRequest.js +++ b/src/crypto/verification/request/VerificationRequest.js @@ -67,6 +67,7 @@ export default class VerificationRequest extends EventEmitter { this._eventsByUs = new Map(); this._eventsByThem = new Map(); this._observeOnly = false; + this._timeoutTimer = null; } /** @@ -105,19 +106,6 @@ export default class VerificationRequest extends EventEmitter { } } - // // a timestamp is not provided on all to_device events - // if (Number.isFinite(timestamp)) { - // const elapsed = Date.now() - timestamp; - // // ignore if event is too far in the past or too far in the future - // if (elapsed > (VERIFICATION_REQUEST_TIMEOUT - VERIFICATION_REQUEST_MARGIN) || - // elapsed < -(VERIFICATION_REQUEST_TIMEOUT / 2) - // ) { - // console.log("VerificationRequest: validateEvent: verification event to far in future or past", type, elapsed); - // logger.log("received verification that is too old or from the future"); - // return false; - // } - // } - return true; } @@ -155,10 +143,14 @@ export default class VerificationRequest extends EventEmitter { return this._commonMethods; } - /** the timeout of the request, provided for compatibility with previous verification code */ + /** the current remaining amount of ms before the request should be automatically cancelled */ get timeout() { - const elapsed = Date.now() - this._startTimestamp; - return Math.max(0, VERIFICATION_REQUEST_TIMEOUT - elapsed); + const requestEvent = this._getEventByEither(REQUEST_TYPE); + if (requestEvent) { + const elapsed = Date.now() - this.channel.getTimestamp(requestEvent); + return Math.max(0, VERIFICATION_REQUEST_TIMEOUT - elapsed); + } + return 0; } /** the m.key.verification.request event that started this request, provided for compatibility with previous verification code */ @@ -260,7 +252,6 @@ export default class VerificationRequest extends EventEmitter { * @returns {VerifierBase} the verifier of the given method */ beginKeyVerification(method, targetDevice = null) { - console.log("beginKeyVerification " + this._observeOnly); // need to allow also when unsent in case of to_device if (!this.observeOnly && !this._verifier) { const validStartPhase = @@ -390,12 +381,6 @@ export default class VerificationRequest extends EventEmitter { return transitions; } - const cancelEvent = this._getEventByEither(CANCEL_TYPE); - if (cancelEvent) { - transitions.push({phase: PHASE_CANCELLED, event: cancelEvent}); - return transitions; - } - const readyEvent = this._getEventByOther(READY_TYPE, requestEvent.getSender()); if (readyEvent) { @@ -421,6 +406,12 @@ export default class VerificationRequest extends EventEmitter { transitions.push({phase: PHASE_DONE}); } + const cancelEvent = this._getEventByEither(CANCEL_TYPE); + if (cancelEvent && phase() !== PHASE_DONE) { + transitions.push({phase: PHASE_CANCELLED, event: cancelEvent}); + return transitions; + } + return transitions; } @@ -464,22 +455,24 @@ export default class VerificationRequest extends EventEmitter { * @returns {Promise} a promise that resolves when any requests as an anwser to the passed-in event are sent. */ async handleEvent(type, event, isLiveEvent) { - // don't send out events for historical requests - if (!isLiveEvent) { - this._observeOnly = true; + // if reached phase cancelled or done, ignore anything else that comes + if (!this.pending) { + return; } - // TODO: check if event is too old/far into the future?, set this._observeOnly - const sender = event.getSender(); - const isOurs = sender === this._client.getUserId(); - const isTheirs = sender === this.otherUserId; + this._adjustObserveOnly(event, isLiveEvent); - if (isOurs) { - this._eventsByUs.set(type, event); - } else if (isTheirs) { - this._eventsByThem.set(type, event); + if (!this.observeOnly) { + const sentByMe = this._wasSentByOwnUser(event); + if (!sentByMe) { + if (await this._cancelOnError(type, event)) { + return; + } + } } + this._addEvent(type, event); + const transitions = this._calculatePhaseTransitions(); const existingIdx = transitions.findIndex(t => t.phase === this.phase); // trim off phases we already went through, if any @@ -490,7 +483,7 @@ export default class VerificationRequest extends EventEmitter { } // only pass events from the other side to the verifier, // no remote echos of our own events - if (this._verifier && sender === this.otherUserId) { + if (this._verifier && event.getSender() === this.otherUserId) { if (type === CANCEL_TYPE || (this._verifier.events && this._verifier.events.includes(type))) { this._verifier.handleEvent(event); @@ -499,28 +492,98 @@ export default class VerificationRequest extends EventEmitter { if (newTransitions.length) { const lastTransition = newTransitions[newTransitions.length - 1]; - this._setPhase(lastTransition.phase); + const {phase} = lastTransition; + + this._setupTimeout(phase); + // set phase as last thing as this emits the "change" event + this._setPhase(phase); } + } - /* - // .request && .ready -if (!this.observeOnly && this._phase !== PHASE_REQUESTED) { - logger.warn("Cancelling, unexpected .request verification event from " + - event.getSender()); - await this.cancel(errorFromEvent(newUnexpectedMessageError())); + _setupTimeout(phase) { + const shouldTimeout = !this._timeoutTimer && !this.observeOnly && + phase === PHASE_REQUESTED && this.initiatedByMe; + + if (shouldTimeout) { + this._timeoutTimer = setInterval(this._cancelOnTimeout, this.timeout); } - - - cancel on handle unexpected events (only if !this.observeOnly): - const sentByMe = this._wasSentByOwnDevice(event); - if (!sentByMe && !this._verificationMethods.has(method)) { - await this.cancel(errorFromEvent(newUnknownMethodError())); - return; + if (this._timeoutTimer) { + const shouldClear = phase === PHASE_STARTED || + phase === PHASE_READY || + phase === PHASE_DONE || + phase === PHASE_CANCELLED; + if (shouldClear) { + clearInterval(this._timeoutTimer); + this._timeoutTimer = null; } - */ + } + } - const url = `http://localhost:8080/#/room/${encodeURIComponent(event.getRoomId())}/${encodeURIComponent(event.getId())}`; - console.log("VerificationRequest: handleEvent", isLiveEvent, url, event); + _cancelOnTimeout = () => { + try { + this.cancel({reason: "Other party didn't accept in time", code: "m.timeout"}); + } catch (err) { + console.error("Error while cancelling verification request", err); + } + }; + + async _cancelOnError(type, event) { + if (type === START_TYPE) { + const method = event.getContent().method; + if (!this._verificationMethods.has(method)) { + await this.cancel(errorFromEvent(newUnknownMethodError())); + return true; + } + } + + const isUnexpectedRequest = type === REQUEST_TYPE && this.phase !== PHASE_UNSENT; + const isUnexpectedReady = type === READY_TYPE && this.phase !== PHASE_REQUESTED; + if (isUnexpectedRequest || isUnexpectedReady) { + logger.warn(`Cancelling, unexpected ${type} verification ` + + `event from ${event.getSender()}`); + const reason = `Unexpected ${type} event in phase ${this.phase}`; + await this.cancel(errorFromEvent(newUnexpectedMessageError({reason}))); + return true; + } + return false; + } + + _adjustObserveOnly(event, isLiveEvent) { + // don't send out events for historical requests + if (!isLiveEvent) { + this._observeOnly = true; + } + // a timestamp is not provided on all to_device events + const timestamp = this.channel.getTimestamp(event); + if (Number.isFinite(timestamp)) { + const elapsed = Date.now() - timestamp; + // don't allow interaction on old requests + if (elapsed > (VERIFICATION_REQUEST_TIMEOUT - VERIFICATION_REQUEST_MARGIN) || + elapsed < -(VERIFICATION_REQUEST_TIMEOUT / 2) + ) { + this._observeOnly = true; + } + } + } + + _addEvent(type, event) { + // TODO: this won't work when verifying our own devices + // and we can't use _wasSentByOwnDevice as not every event has from_device. + if (this._wasSentByOwnUser(event)) { + this._eventsByUs.set(type, event); + } else { + this._eventsByThem.set(type, event); + } + + // once we know the userId of the other party (from the .request event) + // see if any event by anyone else crept into this._eventsByThem + if (type === REQUEST_TYPE) { + for (const [type, event] of this._eventsByThem.entries()) { + if (event.getSender() !== this.otherUserId) { + this._eventsByThem.delete(type); + } + } + } } _createVerifier(method, startEvent = null, targetDevice = null) { @@ -553,32 +616,6 @@ if (!this.observeOnly && this._phase !== PHASE_REQUESTED) { ); } - _getVerifierTarget(startEvent, targetDevice) { - // targetDevice should be set when creating a verifier for to_device before the .start event has been sent, - // so the userId and deviceId are provided - if (targetDevice) { - return targetDevice; - } else { - let targetEvent; - if (startEvent && !this._wasSentByOwnDevice(startEvent)) { - targetEvent = startEvent; - } else if (this._readyEvent && !this._wasSentByOwnDevice(this._readyEvent)) { - targetEvent = this._readyEvent; - } else if (this._requestEvent && !this._wasSentByOwnDevice(this._requestEvent)) { - targetEvent = this._requestEvent; - } else { - throw new Error( - "can't determine who the verifier should be targeted at. " + - "No .request or .start event and no targetDevice"); - } - // TODO: could be replaced by otherUserId - const userId = targetEvent.getSender(); - const content = targetEvent.getContent(); - const deviceId = content && content.from_device; - return {userId, deviceId}; - } - } - _wasSentByOwnUser(event) { return event.getSender() === this._client.getUserId(); } From 423c8a886d2f7c66bad6d90a1613c6ea9316122a Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 3 Jan 2020 18:16:25 +0100 Subject: [PATCH 22/43] use isRemoteEcho to determine if the event is theirs or not rather than the sender and from_device (which is not always set) as this was one of the things breaking to_device verification of ones own devices. --- .../verification/request/InRoomChannel.js | 4 +++- .../verification/request/ToDeviceChannel.js | 2 +- .../request/VerificationRequest.js | 22 ++++++++----------- 3 files changed, 13 insertions(+), 15 deletions(-) diff --git a/src/crypto/verification/request/InRoomChannel.js b/src/crypto/verification/request/InRoomChannel.js index 9cf6d8b31..d54d78f47 100644 --- a/src/crypto/verification/request/InRoomChannel.js +++ b/src/crypto/verification/request/InRoomChannel.js @@ -198,7 +198,9 @@ export class InRoomChannel { this._requestEventId = InRoomChannel.getTransactionId(event); } - return await request.handleEvent(type, event, isLiveEvent); + const isRemoteEcho = !!event.getUnsigned().transaction_id; + + return await request.handleEvent(type, event, isLiveEvent, isRemoteEcho); } /** diff --git a/src/crypto/verification/request/ToDeviceChannel.js b/src/crypto/verification/request/ToDeviceChannel.js index 19caeefbf..48bfc4c7f 100644 --- a/src/crypto/verification/request/ToDeviceChannel.js +++ b/src/crypto/verification/request/ToDeviceChannel.js @@ -150,7 +150,7 @@ export class ToDeviceChannel { const wasStarted = request.phase === PHASE_STARTED || request.phase === PHASE_READY; - await request.handleEvent(event.getType(), event, isLiveEvent); + await request.handleEvent(event.getType(), event, isLiveEvent, false); const isStarted = request.phase === PHASE_STARTED || request.phase === PHASE_READY; diff --git a/src/crypto/verification/request/VerificationRequest.js b/src/crypto/verification/request/VerificationRequest.js index 821608728..9f4dd1fc0 100644 --- a/src/crypto/verification/request/VerificationRequest.js +++ b/src/crypto/verification/request/VerificationRequest.js @@ -452,9 +452,10 @@ export default class VerificationRequest extends EventEmitter { * @param {string} type the "symbolic" event type, as returned by the `getEventType` function on the channel. * @param {MatrixEvent} event the event to handle. Don't call getType() on it but use the `type` parameter instead. * @param {bool} isLiveEvent whether this is an even received through sync or not + * @param {bool} isRemoteEcho whether this is the remote echo of an event sent by the same device * @returns {Promise} a promise that resolves when any requests as an anwser to the passed-in event are sent. */ - async handleEvent(type, event, isLiveEvent) { + async handleEvent(type, event, isLiveEvent, isRemoteEcho) { // if reached phase cancelled or done, ignore anything else that comes if (!this.pending) { return; @@ -462,16 +463,13 @@ export default class VerificationRequest extends EventEmitter { this._adjustObserveOnly(event, isLiveEvent); - if (!this.observeOnly) { - const sentByMe = this._wasSentByOwnUser(event); - if (!sentByMe) { - if (await this._cancelOnError(type, event)) { - return; - } + if (!this.observeOnly && !isRemoteEcho) { + if (await this._cancelOnError(type, event)) { + return; } } - this._addEvent(type, event); + this._addEvent(type, event, isRemoteEcho); const transitions = this._calculatePhaseTransitions(); const existingIdx = transitions.findIndex(t => t.phase === this.phase); @@ -483,7 +481,7 @@ export default class VerificationRequest extends EventEmitter { } // only pass events from the other side to the verifier, // no remote echos of our own events - if (this._verifier && event.getSender() === this.otherUserId) { + if (this._verifier && !isRemoteEcho) { if (type === CANCEL_TYPE || (this._verifier.events && this._verifier.events.includes(type))) { this._verifier.handleEvent(event); @@ -566,10 +564,8 @@ export default class VerificationRequest extends EventEmitter { } } - _addEvent(type, event) { - // TODO: this won't work when verifying our own devices - // and we can't use _wasSentByOwnDevice as not every event has from_device. - if (this._wasSentByOwnUser(event)) { + _addEvent(type, event, isRemoteEcho) { + if (isRemoteEcho || this._wasSentByOwnDevice(event)) { this._eventsByUs.set(type, event); } else { this._eventsByThem.set(type, event); From 3a9dc37d02e315d3f94f3a279c8a1a1ab9428c77 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 3 Jan 2020 18:18:09 +0100 Subject: [PATCH 23/43] new state machine relies on having remote echos, so fake for to_device --- .../verification/request/ToDeviceChannel.js | 25 ++++++++++++++++--- .../request/VerificationRequest.js | 1 + 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/crypto/verification/request/ToDeviceChannel.js b/src/crypto/verification/request/ToDeviceChannel.js index 48bfc4c7f..1e021d4e0 100644 --- a/src/crypto/verification/request/ToDeviceChannel.js +++ b/src/crypto/verification/request/ToDeviceChannel.js @@ -30,6 +30,10 @@ import { newUnexpectedMessageError, errorFromEvent, } from "../Error"; + +const MatrixEvent = require("../../../models/event").MatrixEvent; + + /** * A key verification channel that sends verification events over to_device messages. * Generates its own transaction ids. @@ -223,12 +227,27 @@ export class ToDeviceChannel { * @param {object} content * @returns {Promise} the promise of the request */ - sendCompleted(type, content) { + async sendCompleted(type, content) { + let result; if (type === REQUEST_TYPE) { - return this._sendToDevices(type, content, this._devices); + result = await this._sendToDevices(type, content, this._devices); } else { - return this._sendToDevices(type, content, [this._deviceId]); + result = await this._sendToDevices(type, content, [this._deviceId]); } + // the VerificationRequest state machine requires remote echos of the event + // the client sends itself, so we fake this for to_device messages + const remoteEchoEvent = new MatrixEvent({ + sender: this._client.getUserId(), + content, + type, + }); + await this._request.handleEvent( + type, + remoteEchoEvent, + /*isLiveEvent=*/true, + /*isRemoteEcho=*/true, + ); + return result; } _sendToDevices(type, content, devices) { diff --git a/src/crypto/verification/request/VerificationRequest.js b/src/crypto/verification/request/VerificationRequest.js index 9f4dd1fc0..a36a8c8ba 100644 --- a/src/crypto/verification/request/VerificationRequest.js +++ b/src/crypto/verification/request/VerificationRequest.js @@ -60,6 +60,7 @@ export default class VerificationRequest extends EventEmitter { constructor(channel, verificationMethods, client) { super(); this.channel = channel; + this.channel._request = this; this._verificationMethods = verificationMethods; this._client = client; this._commonMethods = []; From 213bb9dba2914c9fe9981e32b365224890572bda Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 3 Jan 2020 18:19:49 +0100 Subject: [PATCH 24/43] allow to move straight from UNSENT to STARTED this was one of the things breaking to_device verification --- .../verification/request/InRoomChannel.js | 4 ++++ .../verification/request/VerificationRequest.js | 17 +++++++++-------- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/crypto/verification/request/InRoomChannel.js b/src/crypto/verification/request/InRoomChannel.js index d54d78f47..157e30380 100644 --- a/src/crypto/verification/request/InRoomChannel.js +++ b/src/crypto/verification/request/InRoomChannel.js @@ -46,6 +46,10 @@ export class InRoomChannel { return true; } + get receiveStartFromOtherDevices() { + return true; + } + get roomId() { return this._roomId; } diff --git a/src/crypto/verification/request/VerificationRequest.js b/src/crypto/verification/request/VerificationRequest.js index a36a8c8ba..49d576ce2 100644 --- a/src/crypto/verification/request/VerificationRequest.js +++ b/src/crypto/verification/request/VerificationRequest.js @@ -378,18 +378,16 @@ export default class VerificationRequest extends EventEmitter { const requestEvent = this._getEventByEither(REQUEST_TYPE); if (requestEvent) { transitions.push({phase: PHASE_REQUESTED, event: requestEvent}); - } else { - return transitions; } const readyEvent = - this._getEventByOther(READY_TYPE, requestEvent.getSender()); - if (readyEvent) { + requestEvent && this._getEventByOther(READY_TYPE, requestEvent.getSender()); + if (readyEvent && phase() === PHASE_REQUESTED) { transitions.push({phase: PHASE_READY, event: readyEvent}); } - const startEvent = readyEvent ? - this._getEventByEither(START_TYPE) : // any party can send .start after a .ready + const startEvent = readyEvent || !requestEvent ? + this._getEventByEither(START_TYPE) : // any party can send .start after a .ready or unsent this._getEventByOther(START_TYPE, requestEvent.getSender()); if (startEvent) { const fromRequestPhase = phase() === PHASE_REQUESTED && @@ -433,8 +431,11 @@ export default class VerificationRequest extends EventEmitter { phase === PHASE_STARTED || phase === PHASE_READY ) { - // if requested by one of my other devices - if (this._wasSentByOwnUser(event) && !this._wasSentByOwnDevice(event)) { + if ( + this.channel.receiveStartFromOtherDevices && + this._wasSentByOwnUser(event) && + !this._wasSentByOwnDevice(event) + ) { this._observeOnly = true; } } From 5919874f6f08256464dba100508473a52ea31c3f Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 3 Jan 2020 18:20:16 +0100 Subject: [PATCH 25/43] check !unsent instead of requested for emitting the crypto.request event --- src/crypto/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/crypto/index.js b/src/crypto/index.js index 5b2a83e00..749ea0726 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -2517,7 +2517,7 @@ Crypto.prototype._handleVerificationEvent = async function( } const shouldEmit = isNewRequest && !request.initiatedByMe && - request.requested && // check it is in requested phase + !request.invalid && // check it has enough events to pass the UNSENT stage !request.observeOnly; if (shouldEmit) { this._baseApis.emit("crypto.verification.request", request); From 75fc25feb58d47c18bcc64239dc822158b500df0 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 3 Jan 2020 18:20:50 +0100 Subject: [PATCH 26/43] fix method names --- src/crypto/verification/request/ToDeviceChannel.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/crypto/verification/request/ToDeviceChannel.js b/src/crypto/verification/request/ToDeviceChannel.js index 1e021d4e0..f011fc423 100644 --- a/src/crypto/verification/request/ToDeviceChannel.js +++ b/src/crypto/verification/request/ToDeviceChannel.js @@ -293,7 +293,7 @@ export class ToDeviceRequests { } setRequest(event, request) { - this.setBySenderAndTxnId( + this.setRequestBySenderAndTxnId( event.getSender(), ToDeviceChannel.getTransactionId(event), request, @@ -301,7 +301,7 @@ export class ToDeviceRequests { } setRequestByChannel(channel, request) { - this.setBySenderAndTxnId(channel.userId, channel.transactionId, request); + this.setRequestBySenderAndTxnId(channel.userId, channel.transactionId, request); } setRequestBySenderAndTxnId(sender, txnId, request) { From 9338d9c2a68aa952a9a2f404d5dbaf2e93f8938b Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 3 Jan 2020 18:20:59 +0100 Subject: [PATCH 27/43] commit logging --- src/crypto/verification/request/ToDeviceChannel.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/crypto/verification/request/ToDeviceChannel.js b/src/crypto/verification/request/ToDeviceChannel.js index f011fc423..063a70b94 100644 --- a/src/crypto/verification/request/ToDeviceChannel.js +++ b/src/crypto/verification/request/ToDeviceChannel.js @@ -88,10 +88,12 @@ export class ToDeviceChannel { } const content = event.getContent(); if (!content) { + logger.warn("ToDeviceChannel.validateEvent: invalid: no content"); return false; } if (!content.transaction_id) { + logger.warn("ToDeviceChannel.validateEvent: invalid: no transaction_id"); return false; } @@ -99,6 +101,7 @@ export class ToDeviceChannel { if (type === REQUEST_TYPE) { if (!Number.isFinite(content.timestamp)) { + logger.warn("ToDeviceChannel.validateEvent: invalid: no timestamp"); return false; } if (event.getSender() === client.getUserId() && @@ -106,6 +109,7 @@ export class ToDeviceChannel { ) { // ignore requests from ourselves, because it doesn't make sense for a // device to verify itself + logger.warn("ToDeviceChannel.validateEvent: invalid: from own device"); return false; } } From f44e0a8e121d8d6d9fab7d195d3b8cce69b44978 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 3 Jan 2020 18:21:18 +0100 Subject: [PATCH 28/43] parenthesis in wrong place broke logic --- src/crypto/verification/request/VerificationRequest.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/crypto/verification/request/VerificationRequest.js b/src/crypto/verification/request/VerificationRequest.js index 49d576ce2..45f250ccd 100644 --- a/src/crypto/verification/request/VerificationRequest.js +++ b/src/crypto/verification/request/VerificationRequest.js @@ -258,7 +258,7 @@ export default class VerificationRequest extends EventEmitter { const validStartPhase = this.phase === PHASE_REQUESTED || this.phase === PHASE_READY || - this.phase === (PHASE_UNSENT && + (this.phase === PHASE_UNSENT && this.channel.constructor.canCreateRequest(START_TYPE)); if (validStartPhase) { // when called on a request that was initiated with .request event From 72fd1e4e7cb65093d912c73b72161baa12e3d33b Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 3 Jan 2020 18:21:33 +0100 Subject: [PATCH 29/43] add note to fix bug later --- src/crypto/verification/request/VerificationRequest.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/crypto/verification/request/VerificationRequest.js b/src/crypto/verification/request/VerificationRequest.js index 45f250ccd..9d8b16c5f 100644 --- a/src/crypto/verification/request/VerificationRequest.js +++ b/src/crypto/verification/request/VerificationRequest.js @@ -536,6 +536,7 @@ export default class VerificationRequest extends EventEmitter { } } + /* FIXME: https://github.com/vector-im/riot-web/issues/11765 */ const isUnexpectedRequest = type === REQUEST_TYPE && this.phase !== PHASE_UNSENT; const isUnexpectedReady = type === READY_TYPE && this.phase !== PHASE_REQUESTED; if (isUnexpectedRequest || isUnexpectedReady) { From 07cc93cca2cc1dc47f138ffade56c00e8681ada9 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 17 Jan 2020 16:58:19 +0100 Subject: [PATCH 30/43] fix lint --- src/crypto/index.js | 8 +++++--- src/crypto/verification/request/InRoomChannel.js | 13 +++++++++---- .../verification/request/VerificationRequest.js | 11 +++++++---- src/sync.js | 3 ++- 4 files changed, 23 insertions(+), 12 deletions(-) diff --git a/src/crypto/index.js b/src/crypto/index.js index fec8251eb..65cb7da79 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -1600,7 +1600,8 @@ Crypto.prototype._requestVerificationWithChannel = async function( // but if the other party is really fast they could potentially respond to the // request before the server tells us the event got sent, and we would probably // create a new request object - console.log(`Crypto: adding new request to requestsByTxnId with id ${channel.transactionId}`); + logger.log(`Crypto: adding new request to ` + + `requestsByTxnId with id ${channel.transactionId}`); requestsMap.setRequestByChannel(channel, request); return request; @@ -2579,7 +2580,8 @@ Crypto.prototype._handleVerificationEvent = async function( request = createRequest(event); // a request could not be made from this event, so ignore event if (!request) { - console.log(`Crypto: could not find VerificationRequest for ${event.getType()}, and could not create one, so ignoring.`); + logger.log(`Crypto: could not find VerificationRequest for ` + + `${event.getType()}, and could not create one, so ignoring.`); return; } isNewRequest = true; @@ -2594,7 +2596,7 @@ Crypto.prototype._handleVerificationEvent = async function( this._baseApis.emit("crypto.verification.start", request.verifier); } } catch (err) { - console.error("error while handling verification event", event, err); + logger.error("error while handling verification event: " + err.message); } const shouldEmit = isNewRequest && !request.initiatedByMe && diff --git a/src/crypto/verification/request/InRoomChannel.js b/src/crypto/verification/request/InRoomChannel.js index c6dde65ae..57f2139f8 100644 --- a/src/crypto/verification/request/InRoomChannel.js +++ b/src/crypto/verification/request/InRoomChannel.js @@ -21,6 +21,7 @@ import { READY_TYPE, START_TYPE, } from "./VerificationRequest"; +import {logger} from '../../../logger'; const MESSAGE_TYPE = "m.room.message"; const M_REFERENCE = "m.reference"; @@ -127,20 +128,23 @@ export class InRoomChannel { static validateEvent(event, client) { const txnId = InRoomChannel.getTransactionId(event); if (typeof txnId !== "string" || txnId.length === 0) { - console.log("InRoomChannel: validateEvent: no valid txnId", type, txnId); + logger.log("InRoomChannel: validateEvent: no valid txnId " + txnId); return false; } const type = InRoomChannel.getEventType(event); const content = event.getContent(); if (type === REQUEST_TYPE) { if (!content || typeof content.to !== "string" || !content.to.length) { - console.log("InRoomChannel: validateEvent: no valid to", type, content.to); + logger.log("InRoomChannel: validateEvent: " + + "no valid to " + (content && content.to)); return false; } // ignore requests that are not direct to or sent by the syncing user if (!InRoomChannel.getOtherPartyUserId(event, client)) { - console.log("InRoomChannel: validateEvent: not directed to or sent by me", type, event.getSender(), content.to); + logger.log("InRoomChannel: validateEvent: " + + `not directed to or sent by me: ${event.getSender()}` + + `, ${content && content.to}`); return false; } } @@ -196,7 +200,8 @@ export class InRoomChannel { const sender = event.getSender(); if (this.userId !== null) { if (sender !== ownUserId && sender !== this.userId) { - console.log(`InRoomChannel: ignoring verification event from non-participating sender ${sender}`); + logger.log(`InRoomChannel: ignoring verification event from ` + + `non-participating sender ${sender}`); return; } } diff --git a/src/crypto/verification/request/VerificationRequest.js b/src/crypto/verification/request/VerificationRequest.js index f5456ddb8..c6d9117b1 100644 --- a/src/crypto/verification/request/VerificationRequest.js +++ b/src/crypto/verification/request/VerificationRequest.js @@ -83,17 +83,19 @@ export class VerificationRequest extends EventEmitter { const content = event.getContent(); if (!content) { - console.log("VerificationRequest: validateEvent: no content", type); + logger.log("VerificationRequest: validateEvent: no content"); } if (!type.startsWith(EVENT_PREFIX)) { - console.log("VerificationRequest: validateEvent: fail because type doesnt start with " + EVENT_PREFIX, type); + logger.log("VerificationRequest: validateEvent: " + + "fail because type doesnt start with " + EVENT_PREFIX); return false; } if (type === REQUEST_TYPE || type === READY_TYPE) { if (!Array.isArray(content.methods)) { - console.log("VerificationRequest: validateEvent: fail because methods", type); + logger.log("VerificationRequest: validateEvent: " + + "fail because methods"); return false; } } @@ -102,7 +104,8 @@ export class VerificationRequest extends EventEmitter { if (typeof content.from_device !== "string" || content.from_device.length === 0 ) { - console.log("VerificationRequest: validateEvent: fail because from_device", type); + logger.log("VerificationRequest: validateEvent: "+ + "fail because from_device"); return false; } } diff --git a/src/sync.js b/src/sync.js index a8946ef52..24d419317 100644 --- a/src/sync.js +++ b/src/sync.js @@ -1238,7 +1238,8 @@ SyncApi.prototype._processSyncResponse = async function( } } - self._processRoomEvents(room, stateEvents, timelineEvents, syncEventData.fromCache); + self._processRoomEvents(room, stateEvents, + timelineEvents, syncEventData.fromCache); // set summary after processing events, // because it will trigger a name calculation From 59bfc45856ef42c058d449cabf4d6602be0b17c7 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 17 Jan 2020 19:01:08 +0100 Subject: [PATCH 31/43] use setTimeout of setInterval --- src/crypto/verification/request/VerificationRequest.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/crypto/verification/request/VerificationRequest.js b/src/crypto/verification/request/VerificationRequest.js index c6d9117b1..d2139d10a 100644 --- a/src/crypto/verification/request/VerificationRequest.js +++ b/src/crypto/verification/request/VerificationRequest.js @@ -508,7 +508,7 @@ export class VerificationRequest extends EventEmitter { phase === PHASE_REQUESTED && this.initiatedByMe; if (shouldTimeout) { - this._timeoutTimer = setInterval(this._cancelOnTimeout, this.timeout); + this._timeoutTimer = setTimeout(this._cancelOnTimeout, this.timeout); } if (this._timeoutTimer) { const shouldClear = phase === PHASE_STARTED || @@ -516,7 +516,7 @@ export class VerificationRequest extends EventEmitter { phase === PHASE_DONE || phase === PHASE_CANCELLED; if (shouldClear) { - clearInterval(this._timeoutTimer); + clearTimeout(this._timeoutTimer); this._timeoutTimer = null; } } From cbe29658494ef1f5ad56f5121098a8c9bbc2068b Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 17 Jan 2020 19:01:30 +0100 Subject: [PATCH 32/43] mention reason in cancellation error --- src/crypto/verification/Base.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/crypto/verification/Base.js b/src/crypto/verification/Base.js index 8d1475b51..1bcba5ad6 100644 --- a/src/crypto/verification/Base.js +++ b/src/crypto/verification/Base.js @@ -119,7 +119,10 @@ export class VerificationBase extends EventEmitter { } else if (e.getType() === "m.key.verification.cancel") { const reject = this._reject; this._reject = undefined; - reject(new Error("Other side cancelled verification")); + const content = e.getContent(); + const {reason, code} = content; + reject(new Error(`Other side cancelled verification ` + + `because ${reason} (${code})`)); } else if (this._expectedEvent) { // only cancel if there is an event expected. // if there is no event expected, it means verify() wasn't called From e51ba795f3050f1fd175728c305e639525dce47f Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 20 Jan 2020 13:54:14 +0100 Subject: [PATCH 33/43] to make this work while using fake timers, don't use setTimeout but instead use Promise.resolved() as then always runs in the next tick. --- spec/unit/crypto/verification/util.js | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/spec/unit/crypto/verification/util.js b/spec/unit/crypto/verification/util.js index ea1cd81c9..c7d62f2a3 100644 --- a/spec/unit/crypto/verification/util.js +++ b/spec/unit/crypto/verification/util.js @@ -33,15 +33,13 @@ export async function makeTestClients(userInfos, options) { content: msg, }); const client = clientMap[userId][deviceId]; - if (event.isEncrypted()) { - event.attemptDecryption(client._crypto) - .then(() => client.emit("toDeviceEvent", event)); - } else { - setTimeout( - () => client.emit("toDeviceEvent", event), - 0, - ); - } + const decryptionPromise = event.isEncrypted() ? + event.attemptDecryption(client._crypto) : + Promise.resolve(); + + decryptionPromise.then( + () => client.emit("toDeviceEvent", event), + ); } } } From c34ccc9d5318ec8b96e9049ec92341de71968906 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 20 Jan 2020 14:03:43 +0100 Subject: [PATCH 34/43] adjust test: requestVerification returns the request instead of verifier --- spec/unit/crypto/verification/request.spec.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/spec/unit/crypto/verification/request.spec.js b/spec/unit/crypto/verification/request.spec.js index be039824c..bac62645f 100644 --- a/spec/unit/crypto/verification/request.spec.js +++ b/spec/unit/crypto/verification/request.spec.js @@ -64,7 +64,9 @@ describe("verification request", function() { // XXX: Private function access (but it's a test, so we're okay) bobVerifier._endTimer(); }); - const aliceVerifier = await alice.client.requestVerification("@bob:example.com"); + const aliceRequest = await alice.client.requestVerification("@bob:example.com"); + await aliceRequest.waitFor(r => r.started); + const aliceVerifier = aliceRequest.verifier; expect(aliceVerifier).toBeInstanceOf(SAS); // XXX: Private function access (but it's a test, so we're okay) From e89528315d010527ded419628f64cabad5e9be5f Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 20 Jan 2020 14:04:32 +0100 Subject: [PATCH 35/43] enable fake timers for consistency although it doesn't make or break the test --- spec/unit/crypto/verification/request.spec.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/unit/crypto/verification/request.spec.js b/spec/unit/crypto/verification/request.spec.js index bac62645f..437db0efd 100644 --- a/spec/unit/crypto/verification/request.spec.js +++ b/spec/unit/crypto/verification/request.spec.js @@ -22,6 +22,8 @@ import {makeTestClients} from './util'; const Olm = global.Olm; +jest.useFakeTimers(); + describe("verification request", function() { if (!global.Olm) { logger.warn('Not running device verification unit tests: libolm not present'); From 77d0a7618658d28e599b521441a4411da9162273 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 20 Jan 2020 14:52:34 +0100 Subject: [PATCH 36/43] fixup: another timeout --- spec/unit/crypto/verification/util.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/spec/unit/crypto/verification/util.js b/spec/unit/crypto/verification/util.js index c7d62f2a3..fb37ef381 100644 --- a/spec/unit/crypto/verification/util.js +++ b/spec/unit/crypto/verification/util.js @@ -56,9 +56,8 @@ export async function makeTestClients(userInfos, options) { event_id: eventId, }); for (const tc of clients) { - setTimeout( + Promise.resolve().then( () => tc.client.emit("Room.timeline", event), - 0, ); } From c12a3b6610852c2417225a896d49078ae072d3ca Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 20 Jan 2020 17:35:44 +0100 Subject: [PATCH 37/43] more fixup: make sure remote echo doesn't arrive earlier for TestClient --- spec/unit/crypto/verification/util.js | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/spec/unit/crypto/verification/util.js b/spec/unit/crypto/verification/util.js index fb37ef381..34f3c70e0 100644 --- a/spec/unit/crypto/verification/util.js +++ b/spec/unit/crypto/verification/util.js @@ -54,14 +54,17 @@ export async function makeTestClients(userInfos, options) { content: content, room_id: room, event_id: eventId, + origin_server_ts: Date.now(), }); - for (const tc of clients) { - Promise.resolve().then( - () => tc.client.emit("Room.timeline", event), - ); - } - return {event_id: eventId}; + + setImmediate(() => { + for (const tc of clients) { + tc.client.emit("Room.timeline", event); + } + }); + + return Promise.resolve({event_id: eventId}); }; for (const userInfo of userInfos) { From 121e9d0225ffd4993cfa4c191e6c380b966512a5 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 20 Jan 2020 17:36:55 +0100 Subject: [PATCH 38/43] don't overwrite a request when the remote echo arrives before event_id --- src/crypto/index.js | 20 +++++++++---------- .../verification/request/InRoomChannel.js | 8 ++++++++ .../verification/request/ToDeviceChannel.js | 4 ++++ 3 files changed, 22 insertions(+), 10 deletions(-) diff --git a/src/crypto/index.js b/src/crypto/index.js index 65cb7da79..c420a4952 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -1592,18 +1592,18 @@ Crypto.prototype._requestVerificationWithChannel = async function( return map; }, new Map()); } - const request = new VerificationRequest( + let request = new VerificationRequest( channel, verificationMethods, this._baseApis); await request.sendRequest(); - - // TODO: we're only adding the request to the map once it has been sent - // but if the other party is really fast they could potentially respond to the - // request before the server tells us the event got sent, and we would probably - // create a new request object - logger.log(`Crypto: adding new request to ` + - `requestsByTxnId with id ${channel.transactionId}`); - requestsMap.setRequestByChannel(channel, request); - + // don't replace the request created by a racing remote echo + const racingRequest = requestsMap.getRequestByChannel(channel); + if (racingRequest) { + request = racingRequest; + } else { + logger.log(`Crypto: adding new request to ` + + `requestsByTxnId with id ${channel.transactionId} ${channel.roomId}`); + requestsMap.setRequestByChannel(channel, request); + } return request; }; diff --git a/src/crypto/verification/request/InRoomChannel.js b/src/crypto/verification/request/InRoomChannel.js index 57f2139f8..ccd2b6758 100644 --- a/src/crypto/verification/request/InRoomChannel.js +++ b/src/crypto/verification/request/InRoomChannel.js @@ -302,6 +302,14 @@ export class InRoomRequests { const roomId = event.getRoomId(); const txnId = InRoomChannel.getTransactionId(event); // console.log(`looking for request in room ${roomId} with txnId ${txnId} for an ${event.getType()} from ${event.getSender()}...`); + return this._getRequestByTxnId(roomId, txnId); + } + + getRequestByChannel(channel) { + return this._getRequestByTxnId(channel.roomId, channel.transactionId); + } + + _getRequestByTxnId(roomId, txnId) { const requestsByTxnId = this._requestsByRoomId.get(roomId); if (requestsByTxnId) { return requestsByTxnId.get(txnId); diff --git a/src/crypto/verification/request/ToDeviceChannel.js b/src/crypto/verification/request/ToDeviceChannel.js index 4956b594c..9f4995cac 100644 --- a/src/crypto/verification/request/ToDeviceChannel.js +++ b/src/crypto/verification/request/ToDeviceChannel.js @@ -284,6 +284,10 @@ export class ToDeviceRequests { ); } + getRequestByChannel(channel) { + return this.getRequestBySenderAndTxnId(channel.userId, channel.transactionId); + } + getRequestBySenderAndTxnId(sender, txnId) { const requestsByTxnId = this._requestsByUserId.get(sender); if (requestsByTxnId) { From e5c65d53f84c1fe63c4b429a6dee2683aaa6eeae Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 20 Jan 2020 17:54:26 +0100 Subject: [PATCH 39/43] set transaction_id for remote echos in TestClient as InRoomChannel looks at this to decide whether an event is a remote echo (and to pass it to the verifier or not) --- spec/unit/crypto/verification/util.js | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/spec/unit/crypto/verification/util.js b/spec/unit/crypto/verification/util.js index 34f3c70e0..01749077e 100644 --- a/spec/unit/crypto/verification/util.js +++ b/spec/unit/crypto/verification/util.js @@ -48,19 +48,29 @@ export async function makeTestClients(userInfos, options) { const sendEvent = function(room, type, content) { // make up a unique ID as the event ID const eventId = "$" + this.makeTxnId(); // eslint-disable-line babel/no-invalid-this - const event = new MatrixEvent({ + const rawEvent = { sender: this.getUserId(), // eslint-disable-line babel/no-invalid-this type: type, content: content, room_id: room, event_id: eventId, origin_server_ts: Date.now(), - }); - + }; + const event = new MatrixEvent(rawEvent); + const remoteEcho = new MatrixEvent(Object.assign({}, rawEvent, { + unsigned: { + transaction_id: this.makeTxnId(), // eslint-disable-line babel/no-invalid-this + }, + })); setImmediate(() => { for (const tc of clients) { - tc.client.emit("Room.timeline", event); + if (tc.client === this) { // eslint-disable-line babel/no-invalid-this + console.log("sending remote echo!!"); + tc.client.emit("Room.timeline", remoteEcho); + } else { + tc.client.emit("Room.timeline", event); + } } }); From bd9a2c13eb9f8c7e871f477e7c150303525c8200 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 20 Jan 2020 17:55:48 +0100 Subject: [PATCH 40/43] implement API change in sas test for requestVerificationDM --- spec/unit/crypto/verification/sas.spec.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/spec/unit/crypto/verification/sas.spec.js b/spec/unit/crypto/verification/sas.spec.js index 388737dd3..7d0ced92b 100644 --- a/spec/unit/crypto/verification/sas.spec.js +++ b/spec/unit/crypto/verification/sas.spec.js @@ -442,9 +442,11 @@ describe("SAS verification", function() { }); }); - aliceVerifier = await alice.client.requestVerificationDM( + const aliceRequest = await alice.client.requestVerificationDM( bob.client.getUserId(), "!room_id", [verificationMethods.SAS], ); + await aliceRequest.waitFor(r => r.started); + aliceVerifier = aliceRequest.verifier; aliceVerifier.on("show_sas", (e) => { if (!e.sas.emoji || !e.sas.decimal) { e.cancel(); From aac68290acf44973b371219f47f8821508f2266d Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 20 Jan 2020 17:56:28 +0100 Subject: [PATCH 41/43] remove obsolete comment --- src/crypto/index.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/crypto/index.js b/src/crypto/index.js index c420a4952..1c947360f 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -2550,8 +2550,6 @@ Crypto.prototype._onKeyVerificationMessage = function(event) { Crypto.prototype._onTimelineEvent = function( event, room, atStart, removed, {liveEvent} = {}, ) { - // TODO: we still need a request object for past requests, so we can show it in the timeline - // validation now excludes old requests if (!InRoomChannel.validateEvent(event, this._baseApis)) { return; } From d526229a0fe13e52abf6716c0266ea6a4f56f25f Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 20 Jan 2020 18:12:52 +0100 Subject: [PATCH 42/43] update jsdoc of requestVerificationDM which now returns a Promise of VerificationRequest instead of verifier --- src/client.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/client.js b/src/client.js index 12671d27f..0e4a4b5b8 100644 --- a/src/client.js +++ b/src/client.js @@ -853,8 +853,8 @@ async function _setDeviceVerification( * @param {Array} methods array of verification methods to use. Defaults to * all known methods * - * @returns {Promise} resolves to a verifier - * when the request is accepted by the other user + * @returns {Promise} resolves to a VerificationRequest + * when the request has been sent to the other party. */ MatrixClient.prototype.requestVerificationDM = function(userId, roomId, methods) { if (this._crypto === null) { From 9d6f8730488661bb842363a0fa777b4a732ae421 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 20 Jan 2020 18:13:18 +0100 Subject: [PATCH 43/43] remove obsolete and now broken method a request should be accepted by calling accept() on the request. --- src/client.js | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/client.js b/src/client.js index 0e4a4b5b8..7b2f663f3 100644 --- a/src/client.js +++ b/src/client.js @@ -863,22 +863,6 @@ MatrixClient.prototype.requestVerificationDM = function(userId, roomId, methods) return this._crypto.requestVerificationDM(userId, roomId, methods); }; -/** - * Accept a key verification request from a DM. - * - * @param {module:models/event~MatrixEvent} event the verification request - * that is accepted - * @param {string} method the verification mmethod to use - * - * @returns {module:crypto/verification/Base} a verifier - */ -MatrixClient.prototype.acceptVerificationDM = function(event, method) { - if (this._crypto === null) { - throw new Error("End-to-end encryption disabled"); - } - return this._crypto.acceptVerificationDM(event, method); -}; - /** * Request a key verification from another user. *