From dc38f78da25b1d238ee058f7e725caa5cfe1db27 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 22 Jan 2020 17:36:48 +0100 Subject: [PATCH 1/6] add unit tests for verification request --- spec/unit/crypto/verification/request.spec.js | 2 +- .../verification/verification_request.spec.js | 177 ++++++++++++++++++ 2 files changed, 178 insertions(+), 1 deletion(-) create mode 100644 spec/unit/crypto/verification/verification_request.spec.js diff --git a/spec/unit/crypto/verification/request.spec.js b/spec/unit/crypto/verification/request.spec.js index 437db0efd..81d29077a 100644 --- a/spec/unit/crypto/verification/request.spec.js +++ b/spec/unit/crypto/verification/request.spec.js @@ -24,7 +24,7 @@ const Olm = global.Olm; jest.useFakeTimers(); -describe("verification request", function() { +describe("verification request integration tests with crypto layer", function() { if (!global.Olm) { logger.warn('Not running device verification unit tests: libolm not present'); return; diff --git a/spec/unit/crypto/verification/verification_request.spec.js b/spec/unit/crypto/verification/verification_request.spec.js new file mode 100644 index 000000000..a2b0b3570 --- /dev/null +++ b/spec/unit/crypto/verification/verification_request.spec.js @@ -0,0 +1,177 @@ +/* +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. +*/ +import {VerificationRequest, READY_TYPE, START_TYPE, DONE_TYPE} from + "../../../../src/crypto/verification/request/VerificationRequest"; +import {InRoomChannel} from "../../../../src/crypto/verification/request/InRoomChannel"; +import {MatrixEvent} from "../../../../src/models/event"; + +function makeMockClient(userId, deviceId) { + let counter = 1; + let events = []; + return { + getUserId() { return userId; }, + getDeviceId() { return deviceId; }, + sendEvent(roomId, type, content) { + counter = counter + 1; + const eventId = `$${userId}-${deviceId}-${counter}`; + events.push(new MatrixEvent({ + sender: userId, + event_id: eventId, + room_id: roomId, + type, + content, + origin_server_ts: Date.now(), + })); + return Promise.resolve({event_id: eventId}); + }, + popEvents() { + const e = events; + events = []; + return e; + }, + }; +} + +const MOCK_METHOD = "mock-verify"; +class MockVerifier { + constructor(channel, client, userId, deviceId, startEvent) { + this._channel = channel; + this._startEvent = startEvent; + } + + get events() { + return [DONE_TYPE]; + } + + async start() { + if (this._startEvent) { + await this._channel.send(DONE_TYPE, {}); + } else { + await this._channel.send(START_TYPE, {method: MOCK_METHOD}); + } + } + + async handleEvent(event) { + if (event.getType() === DONE_TYPE && !this._startEvent) { + await this._channel.send(DONE_TYPE, {}); + } + } +} + +function makeRemoteEcho(event) { + return new MatrixEvent(Object.assign({}, event.event, { + unsigned: { + transaction_id: "abc", + }, + })); +} + +async function distributeEvent(ownRequest, theirRequest, event) { + await ownRequest.channel.handleEvent( + makeRemoteEcho(event), ownRequest, true); + await theirRequest.channel.handleEvent(event, theirRequest, true); +} + +describe("verification request unit tests", function() { + it("transition from UNSENT to DONE through happy path", async function() { + const alice = makeMockClient("@alice:matrix.tld", "device1"); + const bob = makeMockClient("@bob:matrix.tld", "device1"); + const aliceRequest = new VerificationRequest( + new InRoomChannel(alice, "!room", bob.getUserId()), + new Map([[MOCK_METHOD, MockVerifier]]), alice); + const bobRequest = new VerificationRequest( + new InRoomChannel(bob, "!room"), + new Map([[MOCK_METHOD, MockVerifier]]), bob); + expect(aliceRequest.invalid).toBe(true); + expect(bobRequest.invalid).toBe(true); + + await aliceRequest.sendRequest(); + const [requestEvent] = alice.popEvents(); + expect(requestEvent.getType()).toBe("m.room.message"); + await distributeEvent(aliceRequest, bobRequest, requestEvent); + expect(aliceRequest.requested).toBe(true); + expect(bobRequest.requested).toBe(true); + + await bobRequest.accept(); + const [readyEvent] = bob.popEvents(); + expect(readyEvent.getType()).toBe(READY_TYPE); + await distributeEvent(bobRequest, aliceRequest, readyEvent); + expect(bobRequest.ready).toBe(true); + expect(aliceRequest.ready).toBe(true); + + const verifier = aliceRequest.beginKeyVerification(MOCK_METHOD); + await verifier.start(); + const [startEvent] = alice.popEvents(); + expect(startEvent.getType()).toBe(START_TYPE); + await distributeEvent(aliceRequest, bobRequest, startEvent); + expect(aliceRequest.started).toBe(true); + expect(aliceRequest.verifier).toBeInstanceOf(MockVerifier); + expect(bobRequest.started).toBe(true); + expect(bobRequest.verifier).toBeInstanceOf(MockVerifier); + + await bobRequest.verifier.start(); + const [bobDoneEvent] = bob.popEvents(); + expect(bobDoneEvent.getType()).toBe(DONE_TYPE); + await distributeEvent(bobRequest, aliceRequest, bobDoneEvent); + const [aliceDoneEvent] = alice.popEvents(); + expect(aliceDoneEvent.getType()).toBe(DONE_TYPE); + await distributeEvent(aliceRequest, bobRequest, aliceDoneEvent); + expect(aliceRequest.done).toBe(true); + expect(bobRequest.done).toBe(true); + }); + + it("methods only contains common methods", async function() { + const alice = makeMockClient("@alice:matrix.tld", "device1"); + const bob = makeMockClient("@bob:matrix.tld", "device1"); + const aliceRequest = new VerificationRequest( + new InRoomChannel(alice, "!room", bob.getUserId()), + new Map([["c", function() {}], ["a", function() {}]]), alice); + const bobRequest = new VerificationRequest( + new InRoomChannel(bob, "!room"), + new Map([["c", function() {}], ["b", function() {}]]), bob); + await aliceRequest.sendRequest(); + const [requestEvent] = alice.popEvents(); + await distributeEvent(aliceRequest, bobRequest, requestEvent); + await bobRequest.accept(); + const [readyEvent] = bob.popEvents(); + await distributeEvent(bobRequest, aliceRequest, readyEvent); + expect(aliceRequest.methods).toStrictEqual(["c"]); + expect(bobRequest.methods).toStrictEqual(["c"]); + }); + + it("other client accepting request puts it in observeOnly mode", async function() { + const alice = makeMockClient("@alice:matrix.tld", "device1"); + const bob1 = makeMockClient("@bob:matrix.tld", "device1"); + const bob2 = makeMockClient("@bob:matrix.tld", "device2"); + const aliceRequest = new VerificationRequest( + new InRoomChannel(alice, "!room", bob1.getUserId()), new Map(), alice); + await aliceRequest.sendRequest(); + const [requestEvent] = alice.popEvents(); + const bob1Request = new VerificationRequest( + new InRoomChannel(bob1, "!room"), new Map(), bob1); + const bob2Request = new VerificationRequest( + new InRoomChannel(bob2, "!room"), new Map(), bob2); + + await bob1Request.channel.handleEvent(requestEvent, bob1Request, true); + await bob2Request.channel.handleEvent(requestEvent, bob2Request, true); + + await bob1Request.accept(); + const [readyEvent] = bob1.popEvents(); + expect(bob2Request.observeOnly).toBe(false); + await bob2Request.channel.handleEvent(readyEvent, bob2Request, true); + expect(bob2Request.observeOnly).toBe(true); + }); +}); From 45ec3e0bb96d144b497c48e383388e30c97374f9 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 22 Jan 2020 17:33:08 +0100 Subject: [PATCH 2/6] also emit if the phase didn't change but observeOnly did --- src/crypto/verification/request/VerificationRequest.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/crypto/verification/request/VerificationRequest.js b/src/crypto/verification/request/VerificationRequest.js index d2139d10a..e0e3b672f 100644 --- a/src/crypto/verification/request/VerificationRequest.js +++ b/src/crypto/verification/request/VerificationRequest.js @@ -465,6 +465,7 @@ export class VerificationRequest extends EventEmitter { if (!this.pending) { return; } + const wasObserveOnly = this._observeOnly; this._adjustObserveOnly(event, isLiveEvent); @@ -500,6 +501,8 @@ export class VerificationRequest extends EventEmitter { this._setupTimeout(phase); // set phase as last thing as this emits the "change" event this._setPhase(phase); + } else if (this._observeOnly !== wasObserveOnly) { + this.emit("change"); } } From 1367e285c8e0e44db291fce4f53b9d5503f435ef Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 22 Jan 2020 17:34:50 +0100 Subject: [PATCH 3/6] have channel decide what is considered "sent by us" for in room verification, if another client accepts the request, we still want to observe so those events should still be considered ours, so looking at from_device doesn't work there. --- src/crypto/verification/request/InRoomChannel.js | 4 +++- src/crypto/verification/request/ToDeviceChannel.js | 3 ++- src/crypto/verification/request/VerificationRequest.js | 10 ++++++---- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/crypto/verification/request/InRoomChannel.js b/src/crypto/verification/request/InRoomChannel.js index ccd2b6758..ed0613a82 100644 --- a/src/crypto/verification/request/InRoomChannel.js +++ b/src/crypto/verification/request/InRoomChannel.js @@ -210,8 +210,10 @@ export class InRoomChannel { } const isRemoteEcho = !!event.getUnsigned().transaction_id; + const isSentByUs = event.getSender() === this._client.getUserId(); - return await request.handleEvent(type, event, isLiveEvent, isRemoteEcho); + return await request.handleEvent( + type, event, isLiveEvent, isRemoteEcho, isSentByUs); } /** diff --git a/src/crypto/verification/request/ToDeviceChannel.js b/src/crypto/verification/request/ToDeviceChannel.js index 9f4995cac..68cb96b7a 100644 --- a/src/crypto/verification/request/ToDeviceChannel.js +++ b/src/crypto/verification/request/ToDeviceChannel.js @@ -153,7 +153,7 @@ export class ToDeviceChannel { const wasStarted = request.phase === PHASE_STARTED || request.phase === PHASE_READY; - await request.handleEvent(event.getType(), event, isLiveEvent, false); + await request.handleEvent(event.getType(), event, isLiveEvent, false, false); const isStarted = request.phase === PHASE_STARTED || request.phase === PHASE_READY; @@ -245,6 +245,7 @@ export class ToDeviceChannel { remoteEchoEvent, /*isLiveEvent=*/true, /*isRemoteEcho=*/true, + /*isSentByUs=*/true, ); return result; } diff --git a/src/crypto/verification/request/VerificationRequest.js b/src/crypto/verification/request/VerificationRequest.js index e0e3b672f..79f7d81bb 100644 --- a/src/crypto/verification/request/VerificationRequest.js +++ b/src/crypto/verification/request/VerificationRequest.js @@ -458,9 +458,11 @@ export class VerificationRequest extends EventEmitter { * @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 + * @param {bool} isSentByUs whether this event is sent by a party that can accept and/or observe the request like one of our peers. + * For InRoomChannel this means any device for the syncing user. For ToDeviceChannel, just the syncing 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, isRemoteEcho) { + async handleEvent(type, event, isLiveEvent, isRemoteEcho, isSentByUs) { // if reached phase cancelled or done, ignore anything else that comes if (!this.pending) { return; @@ -475,7 +477,7 @@ export class VerificationRequest extends EventEmitter { } } - this._addEvent(type, event, isRemoteEcho); + this._addEvent(type, event, isSentByUs); const transitions = this._calculatePhaseTransitions(); const existingIdx = transitions.findIndex(t => t.phase === this.phase); @@ -573,8 +575,8 @@ export class VerificationRequest extends EventEmitter { } } - _addEvent(type, event, isRemoteEcho) { - if (isRemoteEcho || this._wasSentByOwnDevice(event)) { + _addEvent(type, event, isSentByUs) { + if (isSentByUs) { this._eventsByUs.set(type, event); } else { this._eventsByThem.set(type, event); From 05f05c889ab859852269a6dc93092f0a8b48d917 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 22 Jan 2020 17:35:59 +0100 Subject: [PATCH 4/6] don't verify in observeOnly mode --- 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 79f7d81bb..f6d28c8b5 100644 --- a/src/crypto/verification/request/VerificationRequest.js +++ b/src/crypto/verification/request/VerificationRequest.js @@ -489,7 +489,7 @@ export class VerificationRequest extends EventEmitter { } // only pass events from the other side to the verifier, // no remote echos of our own events - if (this._verifier && !isRemoteEcho) { + if (this._verifier && !isRemoteEcho && !this.observeOnly) { if (type === CANCEL_TYPE || (this._verifier.events && this._verifier.events.includes(type))) { this._verifier.handleEvent(event); From 1b242e636b6c765f3483d8fb757b6454e9330e7e Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 22 Jan 2020 17:36:16 +0100 Subject: [PATCH 5/6] remove obsolete comment --- src/crypto/verification/request/VerificationRequest.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/crypto/verification/request/VerificationRequest.js b/src/crypto/verification/request/VerificationRequest.js index f6d28c8b5..6dbce5dbb 100644 --- a/src/crypto/verification/request/VerificationRequest.js +++ b/src/crypto/verification/request/VerificationRequest.js @@ -544,7 +544,6 @@ export 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 0c528876889dd44edebc4609b0a756aa3b5ab406 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 23 Jan 2020 10:51:19 +0000 Subject: [PATCH 6/6] copyright year Co-Authored-By: J. Ryan Stinnett --- spec/unit/crypto/verification/verification_request.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/unit/crypto/verification/verification_request.spec.js b/spec/unit/crypto/verification/verification_request.spec.js index a2b0b3570..4b36107d2 100644 --- a/spec/unit/crypto/verification/verification_request.spec.js +++ b/spec/unit/crypto/verification/verification_request.spec.js @@ -1,5 +1,5 @@ /* -Copyright 2019 The Matrix.org Foundation C.I.C. +Copyright 2020 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.