From 2df588f95af0d109694fef63d7fb698c295624b2 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 16 Oct 2020 12:53:08 +0100 Subject: [PATCH] Support party_id Send party_id on events and check the party_id of incoming events matches Includes a basic test to assert that it actually does: we should build out a decent test suite for calls as there's a lot of edge-case functionality that can break and slip through the cracks (eg. glare). This is a start. Fixes https://github.com/matrix-org/matrix-js-sdk/issues/1511 --- spec/TestClient.js | 7 ++ spec/unit/webrtc/call.spec.js | 169 +++++++++++++++++++++++++++++++++ src/webrtc/call.ts | 109 +++++++++++++-------- src/webrtc/callEventHandler.ts | 22 ++--- 4 files changed, 257 insertions(+), 50 deletions(-) create mode 100644 spec/unit/webrtc/call.spec.js diff --git a/spec/TestClient.js b/spec/TestClient.js index a3d2dcb8a..52b4b3d70 100644 --- a/spec/TestClient.js +++ b/spec/TestClient.js @@ -69,6 +69,9 @@ export function TestClient( this.deviceKeys = null; this.oneTimeKeys = {}; + this._callEventHandler = { + calls: new Map(), + }; } TestClient.prototype.toString = function() { @@ -230,3 +233,7 @@ TestClient.prototype.flushSync = function() { logger.log(`${this}: flushSync completed`); }); }; + +TestClient.prototype.isFallbackICEServerAllowed = function() { + return true; +}; diff --git a/spec/unit/webrtc/call.spec.js b/spec/unit/webrtc/call.spec.js new file mode 100644 index 000000000..f0e68e468 --- /dev/null +++ b/spec/unit/webrtc/call.spec.js @@ -0,0 +1,169 @@ +/* +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. +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 {TestClient} from '../../TestClient'; +import {MatrixCall} from '../../../src/webrtc/call'; + +const DUMMY_SDP = ( + "v=0\r\n" + + "o=- 5022425983810148698 2 IN IP4 127.0.0.1\r\n" + + "s=-\r\nt=0 0\r\na=group:BUNDLE 0\r\n" + + "a=msid-semantic: WMS h3wAi7s8QpiQMH14WG3BnDbmlOqo9I5ezGZA\r\n" + + "m=audio 9 UDP/TLS/RTP/SAVPF 111 103 104 9 0 8 106 105 13 110 112 113 126\r\n" + + "c=IN IP4 0.0.0.0\r\na=rtcp:9 IN IP4 0.0.0.0\r\na=ice-ufrag:hLDR\r\n" + + "a=ice-pwd:bMGD9aOldHWiI+6nAq/IIlRw\r\n" + + "a=ice-options:trickle\r\n" + + "a=fingerprint:sha-256 E4:94:84:F9:4A:98:8A:56:F5:5F:FD:AF:72:B9:32:89:49:5C:4B:9A:4A:15:8E:41:8A:F3:69:E4:39:52:DC:D6\r\n" + + "a=setup:active\r\n" + + "a=mid:0\r\n" + + "a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level\r\n" + + "a=extmap:2 http://www.webrtc.org/experiments/rtp-hdrext/abs-send-time\r\n" + + "a=extmap:3 http://www.ietf.org/id/draft-holmer-rmcat-transport-wide-cc-extensions-01\r\n" + + "a=extmap:4 urn:ietf:params:rtp-hdrext:sdes:mid\r\n" + + "a=extmap:5 urn:ietf:params:rtp-hdrext:sdes:rtp-stream-id\r\n" + + "a=extmap:6 urn:ietf:params:rtp-hdrext:sdes:repaired-rtp-stream-id\r\n" + + "a=sendrecv\r\n" + + "a=msid:h3wAi7s8QpiQMH14WG3BnDbmlOqo9I5ezGZA 4357098f-3795-4131-bff4-9ba9c0348c49\r\n" + + "a=rtcp-mux\r\n" + + "a=rtpmap:111 opus/48000/2\r\n" + + "a=rtcp-fb:111 transport-cc\r\n" + + "a=fmtp:111 minptime=10;useinbandfec=1\r\n" + + "a=rtpmap:103 ISAC/16000\r\n" + + "a=rtpmap:104 ISAC/32000\r\n" + + "a=rtpmap:9 G722/8000\r\n" + + "a=rtpmap:0 PCMU/8000\r\n" + + "a=rtpmap:8 PCMA/8000\r\n" + + "a=rtpmap:106 CN/32000\r\n" + + "a=rtpmap:105 CN/16000\r\n" + + "a=rtpmap:13 CN/8000\r\n" + + "a=rtpmap:110 telephone-event/48000\r\n" + + "a=rtpmap:112 telephone-event/32000\r\n" + + "a=rtpmap:113 telephone-event/16000\r\n" + + "a=rtpmap:126 telephone-event/8000\r\n" + + "a=ssrc:3619738545 cname:2RWtmqhXLdoF4sOi\r\n" +); + +class MockRTCPeerConnection { + constructor() { + this.localDescription = { + sdp: DUMMY_SDP, + type: '', + } + } + + addEventListener() {} + createOffer() { + return Promise.resolve({}); + } + setRemoteDescription() { + return Promise.resolve(); + } + setLocalDescription() { + return Promise.resolve(); + } +} + +describe('Call', function() { + let client; + let call; + let prevNavigator; + let prevDocument; + let prevWindow; + + beforeEach(function() { + prevNavigator = global.navigator; + prevDocument = global.document; + prevWindow = global.window; + + global.navigator = { + mediaDevices: { + getUserMedia: () => { + return { + getTracks: () => [], + getAudioTracks: () => [], + getVideoTracks: () => [], + }; + } + } + }; + + global.window = { + RTCPeerConnection: MockRTCPeerConnection, + RTCSessionDescription: {}, + RTCIceCandidate: {}, + getUserMedia: {}, + }; + global.document = {}; + + client = new TestClient(); + call = new MatrixCall({ + client: client.client, + roomId: '!foo:bar', + }); + // call checks one of these is wired up + call.on('error', () => {}); + }); + + afterEach(function() { + client.stop(); + global.navigator = prevNavigator; + global.window = prevWindow; + global.document = prevDocument; + }); + + it('should ignore candidate events from non-matching party ID', async function() { + await call.placeVoiceCall(); + await call.receivedAnswer({ + version: 0, + call_id: call.callId, + party_id: 'the_correct_party_id', + answer: { + sdp: DUMMY_SDP, + } + }); + + call.peerConn.addIceCandidate = jest.fn(); + call.onRemoteIceCandidatesReceived({ + getContent: () => { return { + version: 0, + call_id: call.callId, + party_id: 'the_correct_party_id', + candidates: [ + { + candidate: '', + sdpMid: '', + } + ] + }}, + }); + expect(call.peerConn.addIceCandidate.mock.calls.length).toBe(1); + + call.onRemoteIceCandidatesReceived({ + getContent: () => { return { + version: 0, + call_id: call.callId, + party_id: 'some_other_party_id', + candidates: [ + { + candidate: '', + sdpMid: '', + } + ] + }}, + }); + expect(call.peerConn.addIceCandidate.mock.calls.length).toBe(1); + }); +}); \ No newline at end of file diff --git a/src/webrtc/call.ts b/src/webrtc/call.ts index 5c914cfb6..43278f1f9 100644 --- a/src/webrtc/call.ts +++ b/src/webrtc/call.ts @@ -231,6 +231,10 @@ export class MatrixCall extends EventEmitter { private config: MediaStreamConstraints; private successor: MatrixCall; private opponentVersion: number; + private ourPartyId: string; + // The party ID of the other side: undefined if we haven't chosen a partner + // yet, null if we have but they didn't send a party ID. + private opponentPartyId: string; constructor(opts: CallOpts) { super(); @@ -238,6 +242,7 @@ export class MatrixCall extends EventEmitter { this.client = opts.client; this.type = null; this.forceTURN = opts.forceTURN; + this.ourPartyId = this.client.deviceId; // Array of Objects with urls, username, credential keys this.turnServers = opts.turnServers || []; if (this.turnServers.length === 0 && this.client.isFallbackICEServerAllowed()) { @@ -449,6 +454,7 @@ export class MatrixCall extends EventEmitter { this.setState(CallState.Ringing); this.direction = CallDirection.Inbound; this.opponentVersion = this.msg.version; + this.opponentPartyId = this.msg.party_id || null; if (event.getLocalAge()) { setTimeout(() => { @@ -545,14 +551,11 @@ export class MatrixCall extends EventEmitter { logger.debug("Ending call " + this.callId); this.terminate(CallParty.Local, reason, !suppressEvent); - const content = { - version: VOIP_PROTO_VERSION, - call_id: this.callId, - }; + const content = {}; // Continue to send no reason for user hangups temporarily, until // clients understand the user_hangup reason (voip v1) if (reason !== CallErrorCode.UserHangup) content['reason'] = reason; - this.sendEvent('m.call.hangup', content); + this.sendVoipEvent('m.call.hangup', {}); } /** @@ -575,11 +578,7 @@ export class MatrixCall extends EventEmitter { logger.debug("Rejecting call: " + this.callId); this.terminate(CallParty.Local, CallErrorCode.UserHangup, true); - const content = { - version: VOIP_PROTO_VERSION, - call_id: this.callId, - }; - this.sendEvent('m.call.reject', content); + this.sendVoipEvent('m.call.reject', {}); } /** @@ -692,7 +691,7 @@ export class MatrixCall extends EventEmitter { private sendAnswer() { this.setState(CallState.Connecting); - this.sendEvent('m.call.answer', this.answerContent).then(() => { + this.sendVoipEvent('m.call.answer', this.answerContent).then(() => { // If this isn't the first time we've tried to send the answer, // we may have candidates queued up, so send them now. this.sendCandidateQueue(); @@ -750,8 +749,6 @@ export class MatrixCall extends EventEmitter { await this.peerConn.setLocalDescription(myAnswer); this.answerContent = { - version: VOIP_PROTO_VERSION, - call_id: this.callId, answer: { sdp: this.peerConn.localDescription.sdp, // type is now deprecated as of Matrix VoIP v1, but @@ -808,24 +805,37 @@ export class MatrixCall extends EventEmitter { } }; - /** - * Used by MatrixClient. - * @param {Object} cand - */ - gotRemoteIceCandidate(cand: RTCIceCandidate) { + onRemoteIceCandidatesReceived(ev: MatrixEvent) { if (this.state == CallState.Ended) { //debuglog("Ignoring remote ICE candidate because call has ended"); return; } - if ( - (cand.sdpMid === null || cand.sdpMid === undefined) && - (cand.sdpMLineIndex === null || cand.sdpMLineIndex === undefined) - ) { - logger.debug("Ignoring remote ICE candidate with no sdpMid or sdpMLineIndex"); + + if (!this.partyIdMatches(ev.getContent())) { + logger.info( + `Ignoring candidates from party ID ${ev.getContent().party_id}: ` + + `we have chosen party ID ${this.opponentPartyId}`, + ); return; } - logger.debug("Got remote ICE " + cand.sdpMid + " candidate: " + cand.candidate); - this.peerConn.addIceCandidate(cand); + + const cands = ev.getContent().candidates; + if (!cands) { + logger.info("Ignoring candidates event with no candidates!"); + return; + } + + for (const cand of cands) { + if ( + (cand.sdpMid === null || cand.sdpMid === undefined) && + (cand.sdpMLineIndex === null || cand.sdpMLineIndex === undefined) + ) { + logger.debug("Ignoring remote ICE candidate with no sdpMid or sdpMLineIndex"); + return; + } + logger.debug("Got remote ICE " + cand.sdpMid + " candidate: " + cand.candidate); + this.peerConn.addIceCandidate(cand); + } } /** @@ -837,10 +847,19 @@ export class MatrixCall extends EventEmitter { return; } + if (this.opponentPartyId !== undefined) { + logger.info( + `Ignoring answer from party ID ${msg.party_id}: ` + + `we already have an answer/reject from ${this.opponentPartyId}`, + ); + return; + } + this.opponentVersion = msg.version; + this.opponentPartyId = msg.party_id || null; try { - this.peerConn.setRemoteDescription(msg.answer); + await this.peerConn.setRemoteDescription(msg.answer); } catch (e) { logger.debug("Failed to set remote description", e); this.terminate(CallParty.Local, CallErrorCode.SetRemoteDescription, false); @@ -868,8 +887,6 @@ export class MatrixCall extends EventEmitter { } const content = { - version: VOIP_PROTO_VERSION, - call_id: this.callId, // OpenWebRTC appears to add extra stuff (like the DTLS fingerprint) // to the description when setting it on the peerconnection. // According to the spec it should only add ICE @@ -888,7 +905,7 @@ export class MatrixCall extends EventEmitter { lifetime: CALL_TIMEOUT_MS, }; try { - await this.sendEvent('m.call.invite', content); + await this.sendVoipEvent('m.call.invite', content); this.setState(CallState.InviteSent); setTimeout(() => { if (this.state === CallState.InviteSent) { @@ -1043,12 +1060,22 @@ export class MatrixCall extends EventEmitter { onHangupReceived = (msg) => { logger.debug("Hangup received"); + + if (!this.partyIdMatches(msg)) { + logger.info(`Ignoring message from party ID ${msg.party_id}: our partner is ${this.opponentPartyId}`); + return; + } + // default reason is user_hangup this.terminate(CallParty.Remote, msg.reason || CallErrorCode.UserHangup, true); }; onRejectReceived = (msg) => { logger.debug("Reject received"); + + // No need to check party_id for reject because if we'd received either + // an answer or reject, we wouldn't be in state InviteSent + if (this.state === CallState.InviteSent) { this.terminate(CallParty.Remote, CallErrorCode.UserHangup, true); } else { @@ -1073,8 +1100,12 @@ export class MatrixCall extends EventEmitter { * @param {Object} content * @return {Promise} */ - private sendEvent(eventType: string, content: object) { - return this.client.sendEvent(this.roomId, eventType, content); + private sendVoipEvent(eventType: string, content: object) { + return this.client.sendEvent(this.roomId, eventType, Object.assign({}, content, { + version: VOIP_PROTO_VERSION, + call_id: this.callId, + party_id: this.ourPartyId, + })); } sendCandidate(content: RTCIceCandidate) { @@ -1176,12 +1207,10 @@ export class MatrixCall extends EventEmitter { this.candidateSendQueue = []; ++this.candidateSendTries; const content = { - version: VOIP_PROTO_VERSION, - call_id: this.callId, candidates: cands, }; logger.debug("Attempting to send " + cands.length + " candidates"); - this.sendEvent('m.call.candidates', content).then(() => { + this.sendVoipEvent('m.call.candidates', content).then(() => { this.candidateSendTries = 0; this.sendCandidateQueue(); }, (error) => { @@ -1224,7 +1253,7 @@ export class MatrixCall extends EventEmitter { } private createPeerConnection(): RTCPeerConnection { - const pc = new RTCPeerConnection({ + const pc = new window.RTCPeerConnection({ iceTransportPolicy: this.forceTURN ? 'relay' : undefined, iceServers: this.turnServers, }); @@ -1238,6 +1267,12 @@ export class MatrixCall extends EventEmitter { return pc; } + + private partyIdMatches(msg): boolean { + // They must either match or both be absent (in which case opponentPartyId will be null) + const msgPartyId = msg.party_id || null; + return msgPartyId === this.opponentPartyId; + } } function setTracksEnabled(tracks: Array, enabled: boolean) { @@ -1256,7 +1291,7 @@ function isTracksEnabled(tracks: Array) { } function getUserMediaVideoContraints(callType: CallType) { - const isWebkit = !!window.navigator.webkitGetUserMedia; + const isWebkit = !!navigator.webkitGetUserMedia; switch (callType) { case CallType.Voice: @@ -1332,7 +1367,7 @@ export function createNewMatrixCall(client: any, roomId: string, options?: CallO try { const supported = Boolean( window.RTCPeerConnection || window.RTCSessionDescription || - window.RTCIceCandidate || navigator.getUserMedia, + window.RTCIceCandidate || navigator.mediaDevices, ); if (!supported) { logger.error("WebRTC is not supported in this browser / environment"); diff --git a/src/webrtc/callEventHandler.ts b/src/webrtc/callEventHandler.ts index 628f52c4f..17b35a817 100644 --- a/src/webrtc/callEventHandler.ts +++ b/src/webrtc/callEventHandler.ts @@ -28,7 +28,7 @@ export class CallEventHandler { client: MatrixClient; calls: Map; callEventBuffer: MatrixEvent[]; - candidatesByCall: Map>; + candidateEventsByCall: Map>; constructor(client: MatrixClient) { this.client = client; @@ -42,7 +42,7 @@ export class CallEventHandler { // This happens quite often, eg. replaying sync from storage, catchup sync // after loading and after we've been offline for a bit. this.callEventBuffer = []; - this.candidatesByCall = new Map>(); + this.candidateEventsByCall = new Map>(); this.client.on("sync", this.evaluateEventBuffer); this.client.on("event", this.onEvent); } @@ -157,9 +157,9 @@ export class CallEventHandler { this.calls.set(call.callId, call); // if we stashed candidate events for that call ID, play them back now - if (this.candidatesByCall.get(call.callId)) { - for (const cand of this.candidatesByCall.get(call.callId)) { - call.gotRemoteIceCandidate(cand); + if (this.candidateEventsByCall.get(call.callId)) { + for (const ev of this.candidateEventsByCall.get(call.callId)) { + call.onRemoteIceCandidatesReceived(ev); } } @@ -221,16 +221,12 @@ export class CallEventHandler { } if (!call) { // store the candidates; we may get a call eventually. - if (!this.candidatesByCall.has(content.call_id)) { - this.candidatesByCall.set(content.call_id, []); + if (!this.candidateEventsByCall.has(content.call_id)) { + this.candidateEventsByCall.set(content.call_id, []); } - this.candidatesByCall.set(content.call_id, this.candidatesByCall.get( - content.call_id, - ).concat(content.candidates)); + this.candidateEventsByCall.get(content.call_id).push(event); } else { - for (const cand of content.candidates) { - call.gotRemoteIceCandidate(cand); - } + call.onRemoteIceCandidatesReceived(event); } } else if ([EventType.CallHangup, EventType.CallReject].includes(event.getType())) { // Note that we also observe our own hangups here so we can see