From d38da836568420eb8f2b0b18b160f696aa73edaf Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 4 Jun 2019 23:39:31 -0600 Subject: [PATCH 01/41] Provide the discovered URLs when a liveliness error occurs See https://github.com/vector-im/riot-web/issues/9828 --- src/autodiscovery.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/autodiscovery.js b/src/autodiscovery.js index 10f714d24..e031173fe 100644 --- a/src/autodiscovery.js +++ b/src/autodiscovery.js @@ -256,6 +256,11 @@ export class AutoDiscovery { if (!hsVersions || !hsVersions.raw["versions"]) { logger.error("Invalid /versions response"); clientConfig["m.homeserver"].error = AutoDiscovery.ERROR_INVALID_HOMESERVER; + + // Supply the base_url to the caller because they may be ignoring liveliness + // errors, like this one. + clientConfig["m.homeserver"].base_url = hsUrl; + return Promise.resolve(clientConfig); } @@ -311,6 +316,11 @@ export class AutoDiscovery { logger.error("Invalid /api/v1 response"); failingClientConfig["m.identity_server"].error = AutoDiscovery.ERROR_INVALID_IDENTITY_SERVER; + + // Supply the base_url to the caller because they may be ignoring + // liveliness errors, like this one. + clientConfig["m.identity_server"].base_url = isUrl; + return Promise.resolve(failingClientConfig); } } From 26c1c6db3bac8a7f791f29217f58638377495158 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 4 Jun 2019 23:51:41 -0600 Subject: [PATCH 02/41] Fix tests and populate the right IS validation object --- spec/unit/autodiscovery.spec.js | 10 +++++----- src/autodiscovery.js | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/unit/autodiscovery.spec.js b/spec/unit/autodiscovery.spec.js index f6cb486a1..6d4e5a3e9 100644 --- a/spec/unit/autodiscovery.spec.js +++ b/spec/unit/autodiscovery.spec.js @@ -275,7 +275,7 @@ describe("AutoDiscovery", function() { "m.homeserver": { state: "FAIL_ERROR", error: AutoDiscovery.ERROR_INVALID_HOMESERVER, - base_url: null, + base_url: "https://example.org", }, "m.identity_server": { state: "PROMPT", @@ -304,7 +304,7 @@ describe("AutoDiscovery", function() { "m.homeserver": { state: "FAIL_ERROR", error: AutoDiscovery.ERROR_INVALID_HOMESERVER, - base_url: null, + base_url: "https://example.org", }, "m.identity_server": { state: "PROMPT", @@ -335,7 +335,7 @@ describe("AutoDiscovery", function() { "m.homeserver": { state: "FAIL_ERROR", error: AutoDiscovery.ERROR_INVALID_HOMESERVER, - base_url: null, + base_url: "https://example.org", }, "m.identity_server": { state: "PROMPT", @@ -528,7 +528,7 @@ describe("AutoDiscovery", function() { "m.identity_server": { state: "FAIL_ERROR", error: AutoDiscovery.ERROR_INVALID_IDENTITY_SERVER, - base_url: null, + base_url: "https://identity.example.org", }, }; @@ -569,7 +569,7 @@ describe("AutoDiscovery", function() { "m.identity_server": { state: "FAIL_ERROR", error: AutoDiscovery.ERROR_INVALID_IDENTITY_SERVER, - base_url: null, + base_url: "https://identity.example.org", }, }; diff --git a/src/autodiscovery.js b/src/autodiscovery.js index e031173fe..900c01663 100644 --- a/src/autodiscovery.js +++ b/src/autodiscovery.js @@ -319,7 +319,7 @@ export class AutoDiscovery { // Supply the base_url to the caller because they may be ignoring // liveliness errors, like this one. - clientConfig["m.identity_server"].base_url = isUrl; + failingClientConfig["m.identity_server"].base_url = isUrl; return Promise.resolve(failingClientConfig); } From a30ef7250bbb8b601925c8f3f3bcfcf0b22aaaac Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 5 Jun 2019 15:27:55 -0600 Subject: [PATCH 03/41] Encode event IDs when redacting events Because v3 rooms are a thing. --- src/client.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/client.js b/src/client.js index d7dada2e5..8f9014289 100644 --- a/src/client.js +++ b/src/client.js @@ -1887,8 +1887,10 @@ function _sendEventHttpRequest(client, event) { } path = utils.encodeUri(pathTemplate, pathParams); } else if (event.getType() === "m.room.redaction") { - const pathTemplate = `/rooms/$roomId/redact/${event.event.redacts}/$txnId`; - path = utils.encodeUri(pathTemplate, pathParams); + const pathTemplate = `/rooms/$roomId/redact/$redactsEventId/$txnId`; + path = utils.encodeUri(pathTemplate, Object.assign({ + $redactsEventId: event.event.redacts, + }, pathParams)); } else { path = utils.encodeUri( "/rooms/$roomId/send/$eventType/$txnId", pathParams, From ae9bcd6f6cde5b63ec437f67781204ae7e441acb Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 6 Jun 2019 18:31:54 +0100 Subject: [PATCH 04/41] Don't poll UI auth again until current poll finishes On slow networks/servers we were ending up with lots of requests in flight. --- src/interactive-auth.js | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/interactive-auth.js b/src/interactive-auth.js index 48ca538f0..1a3661c92 100644 --- a/src/interactive-auth.js +++ b/src/interactive-auth.js @@ -115,6 +115,8 @@ function InteractiveAuth(opts) { this._chosenFlow = null; this._currentStage = null; + + this._polling = false; } InteractiveAuth.prototype = { @@ -147,8 +149,9 @@ InteractiveAuth.prototype = { * completed out-of-band. If so, the attemptAuth promise will * be resolved. */ - poll: function() { + poll: async function() { if (!this._data.session) return; + if (this._polling) return; let authDict = {}; if (this._currentStage == EMAIL_STAGE_TYPE) { @@ -169,7 +172,12 @@ InteractiveAuth.prototype = { } } - this.submitAuthDict(authDict, true); + this._polling = true; + try { + await this.submitAuthDict(authDict, true); + } finally { + this._polling = false; + } }, /** @@ -221,7 +229,7 @@ InteractiveAuth.prototype = { * in the attemptAuth promise being rejected. This can be set to true * for requests that just poll to see if auth has been completed elsewhere. */ - submitAuthDict: function(authData, background) { + submitAuthDict: async function(authData, background) { if (!this._resolveFunc) { throw new Error("submitAuthDict() called before attemptAuth()"); } @@ -232,7 +240,7 @@ InteractiveAuth.prototype = { }; utils.extend(auth, authData); - this._doRequest(auth, background); + await this._doRequest(auth, background); }, /** From bac73150ca3d8be79a443983f2b4ed47d5bb8784 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 6 Jun 2019 19:03:29 +0100 Subject: [PATCH 05/41] Don't send another token request while one's in flight Otherwise we end up with more tokens than are strictly necessary --- src/interactive-auth.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/interactive-auth.js b/src/interactive-auth.js index 1a3661c92..200c5be58 100644 --- a/src/interactive-auth.js +++ b/src/interactive-auth.js @@ -112,6 +112,7 @@ function InteractiveAuth(opts) { this._clientSecret = opts.clientSecret || this._matrixClient.generateClientSecret(); this._emailSid = opts.emailSid; if (this._emailSid === undefined) this._emailSid = null; + this._requestingEmailToken = false; this._chosenFlow = null; this._currentStage = null; @@ -313,12 +314,14 @@ InteractiveAuth.prototype = { if ( !this._emailSid && + !this._requestingEmailToken && this._chosenFlow.stages.includes('m.login.email.identity') ) { // If we've picked a flow with email auth, we send the email // now because we want the request to fail as soon as possible // if the email address is not valid (ie. already taken or not // registered, depending on what the operation is). + this._requestingEmailToken = true; try { const requestTokenResult = await this._requestEmailTokenCallback( this._inputs.emailAddress, @@ -341,6 +344,8 @@ InteractiveAuth.prototype = { // the failure up as the user can't complete auth if we can't // send the email, foe whatever reason. this._rejectFunc(e); + } finally { + this._requestingEmailToken = false; } } } From 7879709f62910e54a598e932db734543743c5cdf Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 7 Jun 2019 11:05:44 +0100 Subject: [PATCH 06/41] Fix backup sig validation with multiple sigs verifySignature modifies the object so we need to clone if we're verifying more than one signature. Fixes https://github.com/vector-im/riot-web/issues/9357 --- src/crypto/index.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/crypto/index.js b/src/crypto/index.js index 4854ea194..4224f8aff 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -371,7 +371,9 @@ Crypto.prototype.isKeyBackupTrusted = async function(backupInfo) { try { await olmlib.verifySignature( this._olmDevice, - backupInfo.auth_data, + // verifySignature modifies the object so we need to copy + // if we verify more than one sig + Object.assign({}, backupInfo.auth_data), this._userId, device.deviceId, device.getFingerprint(), From 08d236f5ecc214e1b5f2091d33afa3a7b4aabf99 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Fri, 7 Jun 2019 11:46:56 +0100 Subject: [PATCH 07/41] Add funding details for GitHub sponsor button --- .github/FUNDING.yml | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .github/FUNDING.yml diff --git a/.github/FUNDING.yml b/.github/FUNDING.yml new file mode 100644 index 000000000..afc29f014 --- /dev/null +++ b/.github/FUNDING.yml @@ -0,0 +1,2 @@ +patreon: matrixdotorg +liberapay: matrixdotorg From 654e8b41fa789874bc9b2bb427fbfff7978a9eae Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 10 Jun 2019 15:18:46 +0100 Subject: [PATCH 08/41] Don't overlap auth submissions with polls Wait for polls to complete before submitting auth dicts, otherwise we risk the requests overlapping and both returning a 200. Also introduces a setBusy interface to interactive-auth to explicitly set the busy status, since otherwise it doesn't get set until the request actually starts. --- src/interactive-auth.js | 53 +++++++++++++++++++++++++++++++++-------- 1 file changed, 43 insertions(+), 10 deletions(-) diff --git a/src/interactive-auth.js b/src/interactive-auth.js index 200c5be58..90b17fc0d 100644 --- a/src/interactive-auth.js +++ b/src/interactive-auth.js @@ -55,6 +55,12 @@ const MSISDN_STAGE_TYPE = "m.login.msisdn"; * promise which resolves to the successful response or rejects with a * MatrixError. * + * @param {function(bool): module:client.Promise} opts.setBusy + * called whenever the interactive auth logic is busy submitting + * information provided by the user. After this has been called with + * true the UI should indicate that a request is in progress until it + * is called again with false. + * * @param {function(string, object?)} opts.stateUpdated * called when the status of the UI auth changes, ie. when the state of * an auth stage changes of when the auth flow moves to a new stage. @@ -101,6 +107,7 @@ function InteractiveAuth(opts) { this._matrixClient = opts.matrixClient; this._data = opts.authData || {}; this._requestCallback = opts.doRequest; + this._setBusyCallback = opts.setBusy; // startAuthStage included for backwards compat this._stateUpdatedCallback = opts.stateUpdated || opts.startAuthStage; this._resolveFunc = null; @@ -117,7 +124,9 @@ function InteractiveAuth(opts) { this._chosenFlow = null; this._currentStage = null; - this._polling = false; + // if we are currently trying to submit an auth dict (which includes polling) + // the promise the will resolve/reject when it completes + this._submitPromise = false; } InteractiveAuth.prototype = { @@ -138,7 +147,10 @@ InteractiveAuth.prototype = { // if we have no flows, try a request (we'll have // just a session ID in _data if resuming) if (!this._data.flows) { - this._doRequest(this._data); + this._setBusyCallback(true); + this._doRequest(this._data).finally(() => { + this._setBusyCallback(false); + }); } else { this._startNextAuthStage(); } @@ -152,7 +164,9 @@ InteractiveAuth.prototype = { */ poll: async function() { if (!this._data.session) return; - if (this._polling) return; + // if we currently have a request in flight, there's no point making + // another just to check what the status is + if (this._submitPromise) return; let authDict = {}; if (this._currentStage == EMAIL_STAGE_TYPE) { @@ -173,12 +187,7 @@ InteractiveAuth.prototype = { } } - this._polling = true; - try { - await this.submitAuthDict(authDict, true); - } finally { - this._polling = false; - } + this.submitAuthDict(authDict, true); }, /** @@ -235,13 +244,37 @@ InteractiveAuth.prototype = { throw new Error("submitAuthDict() called before attemptAuth()"); } + if (!background && this._setBusyCallback) { + this._setBusyCallback(true); + } + + // if we're currently trying a request, wait for it to finish + // as otherwise we can get multiple 200 responses which can mean + // things like multiple logins for register requests. + // (but discard any expections as we only care when its done, + // not whether it worked or not) + while (this._submitPromise) { + try { + await this._submitPromise; + } catch (e) { + } + } + // use the sessionid from the last request. const auth = { session: this._data.session, }; utils.extend(auth, authData); - await this._doRequest(auth, background); + try { + // NB. the 'background' flag is deprecated by the setBusy + // callback and is here for backwards compat + this._submitPromise = this._doRequest(auth, background); + await this._submitPromise; + } finally { + this._submitPromise = null; + if (!background) this._setBusyCallback(false); + } }, /** From 61ee6eb8af61d42e46fd7b4eb77cab0b2c0239a3 Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 10 Jun 2019 16:19:45 +0100 Subject: [PATCH 09/41] This should be null, not false Co-Authored-By: J. Ryan Stinnett --- src/interactive-auth.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/interactive-auth.js b/src/interactive-auth.js index 90b17fc0d..23f756db1 100644 --- a/src/interactive-auth.js +++ b/src/interactive-auth.js @@ -126,7 +126,7 @@ function InteractiveAuth(opts) { // if we are currently trying to submit an auth dict (which includes polling) // the promise the will resolve/reject when it completes - this._submitPromise = false; + this._submitPromise = null; } InteractiveAuth.prototype = { From c80518bf3ebd23ce869c42794bb1c0846b5d6711 Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 10 Jun 2019 16:23:06 +0100 Subject: [PATCH 10/41] s/setBusy/busyChanged/ --- src/interactive-auth.js | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/interactive-auth.js b/src/interactive-auth.js index 23f756db1..c5bd21c77 100644 --- a/src/interactive-auth.js +++ b/src/interactive-auth.js @@ -55,11 +55,11 @@ const MSISDN_STAGE_TYPE = "m.login.msisdn"; * promise which resolves to the successful response or rejects with a * MatrixError. * - * @param {function(bool): module:client.Promise} opts.setBusy - * called whenever the interactive auth logic is busy submitting - * information provided by the user. After this has been called with - * true the UI should indicate that a request is in progress until it - * is called again with false. + * @param {function(bool): module:client.Promise} opts.busyChanged + * called whenever the interactive auth logic becomes busy submitting + * information provided by the user or finsihes. After this has been + * called with true the UI should indicate that a request is in progress + * until it is called again with false. * * @param {function(string, object?)} opts.stateUpdated * called when the status of the UI auth changes, ie. when the state of @@ -107,7 +107,7 @@ function InteractiveAuth(opts) { this._matrixClient = opts.matrixClient; this._data = opts.authData || {}; this._requestCallback = opts.doRequest; - this._setBusyCallback = opts.setBusy; + this._busyChangedCallback = opts.busyChanged; // startAuthStage included for backwards compat this._stateUpdatedCallback = opts.stateUpdated || opts.startAuthStage; this._resolveFunc = null; @@ -147,9 +147,9 @@ InteractiveAuth.prototype = { // if we have no flows, try a request (we'll have // just a session ID in _data if resuming) if (!this._data.flows) { - this._setBusyCallback(true); + this._busyChangedCallback(true); this._doRequest(this._data).finally(() => { - this._setBusyCallback(false); + this._busyChangedCallback(false); }); } else { this._startNextAuthStage(); @@ -244,8 +244,8 @@ InteractiveAuth.prototype = { throw new Error("submitAuthDict() called before attemptAuth()"); } - if (!background && this._setBusyCallback) { - this._setBusyCallback(true); + if (!background && this._busyChangedCallback) { + this._busyChangedCallback(true); } // if we're currently trying a request, wait for it to finish @@ -267,13 +267,13 @@ InteractiveAuth.prototype = { utils.extend(auth, authData); try { - // NB. the 'background' flag is deprecated by the setBusy + // NB. the 'background' flag is deprecated by the busyChanged // callback and is here for backwards compat this._submitPromise = this._doRequest(auth, background); await this._submitPromise; } finally { this._submitPromise = null; - if (!background) this._setBusyCallback(false); + if (!background) this._busyChangedCallback(false); } }, From 0412ca5810ff4cc9b1fb26fb680751af0ab207c0 Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 10 Jun 2019 16:24:20 +0100 Subject: [PATCH 11/41] make busyChanged optional --- src/interactive-auth.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/interactive-auth.js b/src/interactive-auth.js index c5bd21c77..99e02afd5 100644 --- a/src/interactive-auth.js +++ b/src/interactive-auth.js @@ -147,9 +147,9 @@ InteractiveAuth.prototype = { // if we have no flows, try a request (we'll have // just a session ID in _data if resuming) if (!this._data.flows) { - this._busyChangedCallback(true); + if (this._busyChangedCallback) this._busyChangedCallback(true); this._doRequest(this._data).finally(() => { - this._busyChangedCallback(false); + if (this._busyChangedCallback) this._busyChangedCallback(false); }); } else { this._startNextAuthStage(); @@ -273,7 +273,7 @@ InteractiveAuth.prototype = { await this._submitPromise; } finally { this._submitPromise = null; - if (!background) this._busyChangedCallback(false); + if (!background && this._busyChangedCallback) this._busyChangedCallback(false); } }, From 3b345707492cd43900254e0a8a1b46622b83ce7b Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 10 Jun 2019 16:26:09 +0100 Subject: [PATCH 12/41] doc background flag deprecation --- src/interactive-auth.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/interactive-auth.js b/src/interactive-auth.js index 99e02afd5..1192f25cf 100644 --- a/src/interactive-auth.js +++ b/src/interactive-auth.js @@ -49,11 +49,11 @@ const MSISDN_STAGE_TYPE = "m.login.msisdn"; * @param {object?} opts.authData error response from the last request. If * null, a request will be made with no auth before starting. * - * @param {function(object?, bool?): module:client.Promise} opts.doRequest - * called with the new auth dict to submit the request and a flag set - * to true if this request is a background request. Should return a - * promise which resolves to the successful response or rejects with a - * MatrixError. + * @param {function(object?): module:client.Promise} opts.doRequest + * called with the new auth dict to submit the request. Also passes a + * second deprecated arg which is a flag set to true if this request + * is a background request. Should return a promise which resolves + * to the successful response or rejects with a MatrixError. * * @param {function(bool): module:client.Promise} opts.busyChanged * called whenever the interactive auth logic becomes busy submitting From 23c4f19cdad82ac3ce041771a37ad18dac804a2a Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 10 Jun 2019 16:29:55 +0100 Subject: [PATCH 13/41] lint --- src/interactive-auth.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/interactive-auth.js b/src/interactive-auth.js index 1192f25cf..b3f5f70ad 100644 --- a/src/interactive-auth.js +++ b/src/interactive-auth.js @@ -273,7 +273,9 @@ InteractiveAuth.prototype = { await this._submitPromise; } finally { this._submitPromise = null; - if (!background && this._busyChangedCallback) this._busyChangedCallback(false); + if (!background && this._busyChangedCallback) { + this._busyChangedCallback(false); + } } }, From 9fb6eea8b768a1b1e1ded7c2eb47a53d1d106ee6 Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 10 Jun 2019 18:04:30 +0100 Subject: [PATCH 14/41] Document what to use instead --- src/interactive-auth.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/interactive-auth.js b/src/interactive-auth.js index b3f5f70ad..57f164bd1 100644 --- a/src/interactive-auth.js +++ b/src/interactive-auth.js @@ -52,7 +52,8 @@ const MSISDN_STAGE_TYPE = "m.login.msisdn"; * @param {function(object?): module:client.Promise} opts.doRequest * called with the new auth dict to submit the request. Also passes a * second deprecated arg which is a flag set to true if this request - * is a background request. Should return a promise which resolves + * is a background request. The busyChanged callback should be used + * instead of the backfround flag. Should return a promise which resolves * to the successful response or rejects with a MatrixError. * * @param {function(bool): module:client.Promise} opts.busyChanged From ac26c91cba94554c4634ba8cd6db1830d8dd0787 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Tue, 11 Jun 2019 12:59:06 +0100 Subject: [PATCH 15/41] Fix content uploads for modern browsers Modern browsers now expose a `stream` function on the Blob and File interfaces. This conflicts with an older style of passing data to the `uploadContent` SDK method, which supported supplying the data to upload in the `stream` property of an object. Since this old style is still in active use in the Matrix JS ecosystem, we preserve the backwards compatibility for now by checking whether `stream` is a function. This fix has been tested in Firefox Nightly (69), Firefox Release (67), Chrome Canary (77), and Chrome Stable (75). Fixes https://github.com/vector-im/riot-web/issues/9913 Fixes https://github.com/matrix-org/matrix-js-sdk/issues/949 --- src/http-api.js | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/http-api.js b/src/http-api.js index 6ec936155..0c42f765e 100644 --- a/src/http-api.js +++ b/src/http-api.js @@ -165,9 +165,21 @@ module.exports.MatrixHttpApi.prototype = { const contentType = opts.type || file.type || 'application/octet-stream'; const fileName = opts.name || file.name; - // we used to recommend setting file.stream to the thing to upload on - // nodejs. - const body = file.stream ? file.stream : file; + // We used to recommend setting file.stream to the thing to upload on + // Node.js. As of 2019-06-11, this is still in widespread use in various + // clients, so we should preserve this for simple objects used in + // Node.js. File API objects (via either the File or Blob interfaces) in + // the browser now define a `stream` method, which leads to trouble + // here, so we also check the type of `stream`. + let body = file; + if (body.stream && typeof body.stream !== "function") { + logger.warn( + "Using `file.stream` as the content to upload. Future " + + "versions of the js-sdk will change this to expect `file` to " + + "be the content directly.", + ); + body = body.stream; + } // backwards-compatibility hacks where we used to do different things // between browser and node. From e222fb17833ec7e958e2d88cb6f89a78ba53130c Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 6 Jun 2019 16:53:24 +0200 Subject: [PATCH 16/41] enqueue relations and redactions as well as they might need to wait until their target has been sent --- src/scheduler.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/scheduler.js b/src/scheduler.js index c6e9e4e71..ceba188a8 100644 --- a/src/scheduler.js +++ b/src/scheduler.js @@ -177,7 +177,7 @@ MatrixScheduler.RETRY_BACKOFF_RATELIMIT = function(event, attempts, err) { * @see module:scheduler~queueAlgorithm */ MatrixScheduler.QUEUE_MESSAGES = function(event) { - if (event.getType() === "m.room.message") { + if (event.getType() === "m.room.message" || !!event.getTargetId()) { // put these events in the 'message' queue. return "message"; } From c58db665ddf4f426af0a9c3690a26149874ab155 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 6 Jun 2019 16:53:49 +0200 Subject: [PATCH 17/41] give the client a chance to run room.updatePendingEvent after sending before the next event is sent. This is needed to update the target id if it was the local id of the event that was just sent. --- src/scheduler.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/scheduler.js b/src/scheduler.js index ceba188a8..64e29d52a 100644 --- a/src/scheduler.js +++ b/src/scheduler.js @@ -220,7 +220,14 @@ function _processQueue(scheduler, queueName) { ); // fire the process function and if it resolves, resolve the deferred. Else // invoke the retry algorithm. - scheduler._procFn(obj.event).done(function(res) { + + // First wait for a resolved promise, so the resolve handlers for + // the deferred of the previously sent event can run. + // This way enqueued relations/redactions to enqueued events can receive + // the remove id of their target before being sent. + Promise.resolve().then(() => { + return scheduler._procFn(obj.event); + }).then(function(res) { // remove this from the queue _removeNextEvent(scheduler, queueName); debuglog("Queue '%s' sent event %s", queueName, obj.event.getId()); From 6eb229ac1eb70843432f8c6f783537bd486cbd56 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 7 Jun 2019 15:45:00 +0200 Subject: [PATCH 18/41] first look in pending event list for event being redacted in case the redacted event hasn't been sent yet --- src/models/room.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/models/room.js b/src/models/room.js index 1146f73c3..693ce9809 100644 --- a/src/models/room.js +++ b/src/models/room.js @@ -1143,7 +1143,10 @@ Room.prototype.addPendingEvent = function(event, txnId) { if (event.getType() === "m.room.redaction") { const redactId = event.event.redacts; - const redactedEvent = this.getUnfilteredTimelineSet().findEventById(redactId); + let redactedEvent = this._pendingEventList && this._pendingEventList.find(e => e.getId() === redactId); + if (!redactedEvent) { + redactedEvent = this.getUnfilteredTimelineSet().findEventById(redactId); + } if (redactedEvent) { redactedEvent.markLocallyRedacted(event); this.emit("Room.redaction", event, this); From 831aec6488dd4327721423011f9e3ca5fa4174e5 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 7 Jun 2019 16:10:47 +0200 Subject: [PATCH 19/41] emit remote id once received so enqueued relations have it when sent --- src/client.js | 15 +++++++++++---- src/models/event.js | 24 ++++++++++++++++++++++++ src/models/room.js | 2 +- 3 files changed, 36 insertions(+), 5 deletions(-) diff --git a/src/client.js b/src/client.js index 8f9014289..2e2411a8e 100644 --- a/src/client.js +++ b/src/client.js @@ -1719,6 +1719,9 @@ MatrixClient.prototype._sendCompleteEvent = function(roomId, eventObject, txnId, txnId = this.makeTxnId(); } + // we always construct a MatrixEvent when sending because the store and + // scheduler use them. We'll extract the params back out if it turns out + // the client has no scheduler or store. const localEvent = new MatrixEvent(Object.assign(eventObject, { event_id: "~" + roomId + ":" + txnId, user_id: this.credentials.userId, @@ -1726,13 +1729,17 @@ MatrixClient.prototype._sendCompleteEvent = function(roomId, eventObject, txnId, origin_server_ts: new Date().getTime(), })); + const room = this.getRoom(roomId); + const targetId = localEvent.getTargetId(); + if (targetId && targetId.startsWith("~")) { + const target = room.getPendingEvents().find(e => e.getId() === targetId); + target.once("Event.localEventIdReplaced", () => { + localEvent.updateTargetId(target.getId()); + }); + } const type = localEvent.getType(); logger.log(`sendEvent of type ${type} in ${roomId} with txnId ${txnId}`); - // we always construct a MatrixEvent when sending because the store and - // scheduler use them. We'll extract the params back out if it turns out - // the client has no scheduler or store. - const room = this.getRoom(roomId); localEvent._txnId = txnId; localEvent.setStatus(EventStatus.SENDING); diff --git a/src/models/event.js b/src/models/event.js index e16d49b4d..d0f67a3a3 100644 --- a/src/models/event.js +++ b/src/models/event.js @@ -801,6 +801,11 @@ utils.extend(module.exports.MatrixEvent.prototype, { this.emit("Event.status", this, status); }, + replaceLocalEventId(eventId) { + this.event.event_id = eventId; + this.emit("Event.localEventIdReplaced", this); + }, + /** * Get whether the event is a relation event, and of a given type if * `relType` is passed in. @@ -876,6 +881,25 @@ utils.extend(module.exports.MatrixEvent.prototype, { return this._replacingEvent; }, + + getTargetId() { + const relation = this.getRelation(); + if (relation) { + return relation.event_id; + } else if (this.getType() === "m.room.redaction") { + return this.event.redacts; + } + }, + + updateTargetId(eventId) { + const relation = this.getRelation(); + if (relation) { + relation.event_id = eventId; + } else if (this.getType() === "m.room.redaction") { + this.event.redacts = eventId; + } + }, + /** * Summarise the event as JSON for debugging. If encrypted, include both the * decrypted and encrypted view of the event. This is named `toJSON` for use diff --git a/src/models/room.js b/src/models/room.js index 693ce9809..3dd45f7df 100644 --- a/src/models/room.js +++ b/src/models/room.js @@ -1318,7 +1318,7 @@ Room.prototype.updatePendingEvent = function(event, newStatus, newEventId) { if (newStatus == EventStatus.SENT) { // update the event id - event.event.event_id = newEventId; + event.replaceLocalEventId(newEventId); // if the event was already in the timeline (which will be the case if // opts.pendingEventOrdering==chronological), we need to update the From 7a10d504b2ea4f889ee9e39840f33612057f679a Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 7 Jun 2019 16:15:54 +0200 Subject: [PATCH 20/41] emit Relations.redaction synchronously, timeout should not be needed listeners shouldn't care about the original event, as it's removed from the Relations collection already. --- src/models/relations.js | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/models/relations.js b/src/models/relations.js index aa5c17d3f..22ed41669 100644 --- a/src/models/relations.js +++ b/src/models/relations.js @@ -242,12 +242,7 @@ export default class Relations extends EventEmitter { redactedEvent.removeListener("Event.beforeRedaction", this._onBeforeRedaction); - // Dispatch a redaction event on this collection. `setTimeout` is used - // to wait until the next event loop iteration by which time the event - // has actually been marked as redacted. - setTimeout(() => { - this.emit("Relations.redaction"); - }, 0); + this.emit("Relations.redaction"); } /** From f1336a5ce71349c95706b7d96e2f3daa3a4a49b6 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 7 Jun 2019 16:56:50 +0200 Subject: [PATCH 21/41] rename target id to related id and add jsdoc comments --- src/client.js | 4 ++-- src/models/event.js | 18 +++++++++++++++--- src/scheduler.js | 2 +- 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/client.js b/src/client.js index 2e2411a8e..45a3cfded 100644 --- a/src/client.js +++ b/src/client.js @@ -1730,11 +1730,11 @@ MatrixClient.prototype._sendCompleteEvent = function(roomId, eventObject, txnId, })); const room = this.getRoom(roomId); - const targetId = localEvent.getTargetId(); + const targetId = localEvent.getRelatedId(); if (targetId && targetId.startsWith("~")) { const target = room.getPendingEvents().find(e => e.getId() === targetId); target.once("Event.localEventIdReplaced", () => { - localEvent.updateTargetId(target.getId()); + localEvent.updateRelatedId(target.getId()); }); } const type = localEvent.getType(); diff --git a/src/models/event.js b/src/models/event.js index d0f67a3a3..61198c8f6 100644 --- a/src/models/event.js +++ b/src/models/event.js @@ -881,8 +881,12 @@ utils.extend(module.exports.MatrixEvent.prototype, { return this._replacingEvent; }, - - getTargetId() { + /** + * For relations and redactions, returns the event_id this event is referring to. + * + * @return {string?} + */ + getRelatedId() { const relation = this.getRelation(); if (relation) { return relation.event_id; @@ -891,7 +895,15 @@ utils.extend(module.exports.MatrixEvent.prototype, { } }, - updateTargetId(eventId) { + /** + * Update the related id with a new one. + * + * Used to replace a local id with remote one before sending + * an event with a related id. + * + * @param {string} eventId the new event id + */ + updateRelatedId(eventId) { const relation = this.getRelation(); if (relation) { relation.event_id = eventId; diff --git a/src/scheduler.js b/src/scheduler.js index 64e29d52a..cda8c21ec 100644 --- a/src/scheduler.js +++ b/src/scheduler.js @@ -177,7 +177,7 @@ MatrixScheduler.RETRY_BACKOFF_RATELIMIT = function(event, attempts, err) { * @see module:scheduler~queueAlgorithm */ MatrixScheduler.QUEUE_MESSAGES = function(event) { - if (event.getType() === "m.room.message" || !!event.getTargetId()) { + if (event.getType() === "m.room.message" || !!event.getRelatedId()) { // put these events in the 'message' queue. return "message"; } From 3f917b39c9e30113100b6899451976505f2ea794 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 7 Jun 2019 17:05:02 +0200 Subject: [PATCH 22/41] fix lint --- src/models/room.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/models/room.js b/src/models/room.js index 3dd45f7df..f527d83cb 100644 --- a/src/models/room.js +++ b/src/models/room.js @@ -1143,7 +1143,8 @@ Room.prototype.addPendingEvent = function(event, txnId) { if (event.getType() === "m.room.redaction") { const redactId = event.event.redacts; - let redactedEvent = this._pendingEventList && this._pendingEventList.find(e => e.getId() === redactId); + let redactedEvent = this._pendingEventList && + this._pendingEventList.find(e => e.getId() === redactId); if (!redactedEvent) { redactedEvent = this.getUnfilteredTimelineSet().findEventById(redactId); } From 7d2f7fae45d244a09bd4da46bef22042ed0c59a9 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 7 Jun 2019 19:15:18 +0200 Subject: [PATCH 23/41] fix tests --- spec/unit/scheduler.spec.js | 69 ++++++++++++++++++++++--------------- 1 file changed, 42 insertions(+), 27 deletions(-) diff --git a/spec/unit/scheduler.spec.js b/spec/unit/scheduler.spec.js index 502edb3f3..500704ec3 100644 --- a/spec/unit/scheduler.spec.js +++ b/spec/unit/scheduler.spec.js @@ -48,7 +48,7 @@ describe("MatrixScheduler", function() { clock.uninstall(); }); - it("should process events in a queue in a FIFO manner", function(done) { + it("should process events in a queue in a FIFO manner", async function() { retryFn = function() { return 0; }; @@ -57,28 +57,30 @@ describe("MatrixScheduler", function() { }; const deferA = Promise.defer(); const deferB = Promise.defer(); - let resolvedA = false; + let yieldedA = false; scheduler.setProcessFunction(function(event) { - if (resolvedA) { + if (yieldedA) { expect(event).toEqual(eventB); return deferB.promise; } else { + yieldedA = true; expect(event).toEqual(eventA); return deferA.promise; } }); - scheduler.queueEvent(eventA); - scheduler.queueEvent(eventB).done(function() { - expect(resolvedA).toBe(true); - done(); - }); - deferA.resolve({}); - resolvedA = true; - deferB.resolve({}); + const abPromise = Promise.all([ + scheduler.queueEvent(eventA), + scheduler.queueEvent(eventB), + ]); + deferB.resolve({b: true}); + deferA.resolve({a: true}); + const [a, b] = await abPromise; + expect(a.a).toEqual(true); + expect(b.b).toEqual(true); }); it("should invoke the retryFn on failure and wait the amount of time specified", - function(done) { + async function() { const waitTimeMs = 1500; const retryDefer = Promise.defer(); retryFn = function() { @@ -97,24 +99,27 @@ describe("MatrixScheduler", function() { return defer.promise; } else if (procCount === 2) { // don't care about this defer - return Promise.defer().promise; + return new Promise(); } expect(procCount).toBeLessThan(3); }); scheduler.queueEvent(eventA); + // as queing doesn't start processing + // synchronously anymore (see commit bbdb5ac) + // wait just long enough before it does + await Promise.resolve(); expect(procCount).toEqual(1); defer.reject({}); - retryDefer.promise.done(function() { - expect(procCount).toEqual(1); - clock.tick(waitTimeMs); - expect(procCount).toEqual(2); - done(); - }); + await retryDefer.promise; + expect(procCount).toEqual(1); + clock.tick(waitTimeMs); + await Promise.resolve(); + expect(procCount).toEqual(2); }); it("should give up if the retryFn on failure returns -1 and try the next event", - function(done) { + async function() { // Queue A & B. // Reject A and return -1 on retry. // Expect B to be tried next and the promise for A to be rejected. @@ -122,8 +127,8 @@ describe("MatrixScheduler", function() { return -1; }; queueFn = function() { - return "yep"; -}; + return "yep"; + }; const deferA = Promise.defer(); const deferB = Promise.defer(); @@ -142,13 +147,18 @@ describe("MatrixScheduler", function() { const globalA = scheduler.queueEvent(eventA); scheduler.queueEvent(eventB); - + // as queing doesn't start processing + // synchronously anymore (see commit bbdb5ac) + // wait just long enough before it does + await Promise.resolve(); expect(procCount).toEqual(1); deferA.reject({}); - globalA.catch(function() { + try { + await globalA; + } catch(err) { + await Promise.resolve(); expect(procCount).toEqual(2); - done(); - }); + } }); it("should treat each queue separately", function(done) { @@ -300,7 +310,12 @@ describe("MatrixScheduler", function() { expect(ev).toEqual(eventA); return defer.promise; }); - expect(procCount).toEqual(1); + // as queing doesn't start processing + // synchronously anymore (see commit bbdb5ac) + // wait just long enough before it does + Promise.resolve().then(() => { + expect(procCount).toEqual(1); + }); }); it("should not call the processFn if there are no queued events", function() { From 624c6f0a6eebea771650c29032af9de67147e7fe Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Tue, 11 Jun 2019 18:24:48 +0200 Subject: [PATCH 24/41] get the txnId from the correct place to delete event after remote echo --- src/models/room.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/models/room.js b/src/models/room.js index f527d83cb..46ee612c0 100644 --- a/src/models/room.js +++ b/src/models/room.js @@ -1215,7 +1215,7 @@ Room.prototype._handleRemoteEcho = function(remoteEvent, localEvent) { const oldStatus = localEvent.status; // no longer pending - delete this._txnToEvent[remoteEvent.transaction_id]; + delete this._txnToEvent[remoteEvent.getUnsigned().transaction_id]; // if it's in the pending list, remove it if (this._pendingEventList) { From 6d9fba8191ae4282ff490ab77ab0d1dbb2dd50f8 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Tue, 11 Jun 2019 18:25:25 +0200 Subject: [PATCH 25/41] preserve (locally) redacted state after applying remote echo because the RedactionDimensions was trying to redact an event that was already redacted after it's remote echo had come in but it's redaction hadn't synced yet. --- src/models/event.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/models/event.js b/src/models/event.js index 61198c8f6..43036acc9 100644 --- a/src/models/event.js +++ b/src/models/event.js @@ -776,9 +776,23 @@ utils.extend(module.exports.MatrixEvent.prototype, { * @param {Object} event the object to assign to the `event` property */ handleRemoteEcho: function(event) { + const oldUnsigned = this.getUnsigned(); + const oldId = this.getId(); this.event = event; + // keep being redacted if + // the event was redacted as a local echo + if (oldUnsigned.redacted_because) { + if (!this.event.unsigned) { + this.event.unsigned = {}; + } + this.event.unsigned.redacted_because = oldUnsigned.redacted_because; + } // successfully sent. this.setStatus(null); + if (this.getId() !== oldId) { + // emit the event if it changed + this.emit("Event.localEventIdReplaced", this); + } }, /** From 930de640aca846ddd39b26de113e2ce808c3cecc Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 12 Jun 2019 10:01:52 +0200 Subject: [PATCH 26/41] don't add events from /sync that have been locally redacted it'll cause the reactions counter to go up and down while reactions and redactions come in. In case the local redaction gets cancelled, Room._revertRedactionLocalEcho will add the relation back to the relations collection. --- src/models/event-timeline-set.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/models/event-timeline-set.js b/src/models/event-timeline-set.js index af2d48759..5b852f5b0 100644 --- a/src/models/event-timeline-set.js +++ b/src/models/event-timeline-set.js @@ -749,6 +749,10 @@ EventTimelineSet.prototype.aggregateRelations = function(event) { return; } + if (event.isRedacted()) { + return; + } + // If the event is currently encrypted, wait until it has been decrypted. if (event.isBeingDecrypted()) { event.once("Event.decrypted", () => { From 5602b94dcbb5b73478b2f09900b4659598dea34c Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 12 Jun 2019 10:22:39 +0200 Subject: [PATCH 27/41] make sure where not re-adding cancelled events when undoing local red. --- src/models/room.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/models/room.js b/src/models/room.js index 46ee612c0..02d999cea 100644 --- a/src/models/room.js +++ b/src/models/room.js @@ -1356,7 +1356,10 @@ Room.prototype._revertRedactionLocalEcho = function(redactionEvent) { // re-render after undoing redaction this.emit("Room.redactionCancelled", redactionEvent, this); // reapply relation now redaction failed - if (redactedEvent.isRelation()) { + if ( + redactedEvent.isRelation() && + redactedEvent.status !== EventStatus.CANCELLED + ) { this._aggregateNonLiveRelation(redactedEvent); } } From a9f9e2cf352f2d87c1a552f527a70ede6f625bc8 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 12 Jun 2019 14:04:52 +0000 Subject: [PATCH 28/41] comment typo Co-Authored-By: J. Ryan Stinnett --- spec/unit/scheduler.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/unit/scheduler.spec.js b/spec/unit/scheduler.spec.js index 500704ec3..3d1fc76da 100644 --- a/spec/unit/scheduler.spec.js +++ b/spec/unit/scheduler.spec.js @@ -105,7 +105,7 @@ describe("MatrixScheduler", function() { }); scheduler.queueEvent(eventA); - // as queing doesn't start processing + // as queueing doesn't start processing // synchronously anymore (see commit bbdb5ac) // wait just long enough before it does await Promise.resolve(); From b005b753319a7aa397159ab5c14b05d83c60aa87 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 12 Jun 2019 14:06:43 +0000 Subject: [PATCH 29/41] comment typo Co-Authored-By: J. Ryan Stinnett --- spec/unit/scheduler.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/unit/scheduler.spec.js b/spec/unit/scheduler.spec.js index 3d1fc76da..a513f97b9 100644 --- a/spec/unit/scheduler.spec.js +++ b/spec/unit/scheduler.spec.js @@ -310,7 +310,7 @@ describe("MatrixScheduler", function() { expect(ev).toEqual(eventA); return defer.promise; }); - // as queing doesn't start processing + // as queueing doesn't start processing // synchronously anymore (see commit bbdb5ac) // wait just long enough before it does Promise.resolve().then(() => { From 3ed9b003989c6690fcd7b341a5cd8e9f04c05335 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 13 Jun 2019 11:47:01 +0200 Subject: [PATCH 30/41] clarify why we need to listen for remote echo of related event --- src/client.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/client.js b/src/client.js index 45a3cfded..20b29d13a 100644 --- a/src/client.js +++ b/src/client.js @@ -1730,6 +1730,11 @@ MatrixClient.prototype._sendCompleteEvent = function(roomId, eventObject, txnId, })); const room = this.getRoom(roomId); + + // if this is a relation or redaction of an event + // that hasn't been sent yet (e.g. with a local id starting with a ~) + // then listen for the remote echo of that event so that by the time + // this event does get sent, we have the correct event_id const targetId = localEvent.getRelatedId(); if (targetId && targetId.startsWith("~")) { const target = room.getPendingEvents().find(e => e.getId() === targetId); From 4143a79f7bb68f2a7ab92d8bc0831db148359630 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 13 Jun 2019 11:50:29 +0200 Subject: [PATCH 31/41] rename related id to associated id --- src/client.js | 4 ++-- src/models/event.js | 4 ++-- src/scheduler.js | 3 ++- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/client.js b/src/client.js index 20b29d13a..1f86301d3 100644 --- a/src/client.js +++ b/src/client.js @@ -1735,11 +1735,11 @@ MatrixClient.prototype._sendCompleteEvent = function(roomId, eventObject, txnId, // that hasn't been sent yet (e.g. with a local id starting with a ~) // then listen for the remote echo of that event so that by the time // this event does get sent, we have the correct event_id - const targetId = localEvent.getRelatedId(); + const targetId = localEvent.getAssociatedId(); if (targetId && targetId.startsWith("~")) { const target = room.getPendingEvents().find(e => e.getId() === targetId); target.once("Event.localEventIdReplaced", () => { - localEvent.updateRelatedId(target.getId()); + localEvent.updateAssociatedId(target.getId()); }); } const type = localEvent.getType(); diff --git a/src/models/event.js b/src/models/event.js index 43036acc9..a1784d6b4 100644 --- a/src/models/event.js +++ b/src/models/event.js @@ -900,7 +900,7 @@ utils.extend(module.exports.MatrixEvent.prototype, { * * @return {string?} */ - getRelatedId() { + getAssociatedId() { const relation = this.getRelation(); if (relation) { return relation.event_id; @@ -917,7 +917,7 @@ utils.extend(module.exports.MatrixEvent.prototype, { * * @param {string} eventId the new event id */ - updateRelatedId(eventId) { + updateAssociatedId(eventId) { const relation = this.getRelation(); if (relation) { relation.event_id = eventId; diff --git a/src/scheduler.js b/src/scheduler.js index cda8c21ec..b3461bebd 100644 --- a/src/scheduler.js +++ b/src/scheduler.js @@ -177,7 +177,8 @@ MatrixScheduler.RETRY_BACKOFF_RATELIMIT = function(event, attempts, err) { * @see module:scheduler~queueAlgorithm */ MatrixScheduler.QUEUE_MESSAGES = function(event) { - if (event.getType() === "m.room.message" || !!event.getRelatedId()) { + // enqueue messages or events that associate with another event (redactions and relations) + if (event.getType() === "m.room.message" || !!event.getAssociatedId()) { // put these events in the 'message' queue. return "message"; } From 4462f4b90e3dfacb29395c0da12a227e9d5295f9 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 13 Jun 2019 11:57:39 +0200 Subject: [PATCH 32/41] add isRedaction helper on Event --- src/client.js | 2 +- src/models/event.js | 13 +++++++++++-- src/models/room.js | 8 ++++---- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/client.js b/src/client.js index 1f86301d3..239dc3b1a 100644 --- a/src/client.js +++ b/src/client.js @@ -1898,7 +1898,7 @@ function _sendEventHttpRequest(client, event) { pathTemplate = "/rooms/$roomId/state/$eventType/$stateKey"; } path = utils.encodeUri(pathTemplate, pathParams); - } else if (event.getType() === "m.room.redaction") { + } else if (event.isRedaction()) { const pathTemplate = `/rooms/$roomId/redact/$redactsEventId/$txnId`; path = utils.encodeUri(pathTemplate, Object.assign({ $redactsEventId: event.event.redacts, diff --git a/src/models/event.js b/src/models/event.js index a1784d6b4..abb6f8a24 100644 --- a/src/models/event.js +++ b/src/models/event.js @@ -753,6 +753,15 @@ utils.extend(module.exports.MatrixEvent.prototype, { return Boolean(this.getUnsigned().redacted_because); }, + /** + * Check if this event is a redaction of another event + * + * @return {boolean} True if this event is a redaction + */ + isRedaction: function() { + return this.getType() === "m.room.redaction"; + }, + /** * Get the push actions, if known, for this event * @@ -904,7 +913,7 @@ utils.extend(module.exports.MatrixEvent.prototype, { const relation = this.getRelation(); if (relation) { return relation.event_id; - } else if (this.getType() === "m.room.redaction") { + } else if (this.isRedaction()) { return this.event.redacts; } }, @@ -921,7 +930,7 @@ utils.extend(module.exports.MatrixEvent.prototype, { const relation = this.getRelation(); if (relation) { relation.event_id = eventId; - } else if (this.getType() === "m.room.redaction") { + } else if (this.isRedaction()) { this.event.redacts = eventId; } }, diff --git a/src/models/room.js b/src/models/room.js index 02d999cea..46eda57ec 100644 --- a/src/models/room.js +++ b/src/models/room.js @@ -1032,7 +1032,7 @@ Room.prototype.removeFilteredTimelineSet = function(filter) { * @private */ Room.prototype._addLiveEvent = function(event, duplicateStrategy) { - if (event.getType() === "m.room.redaction") { + if (event.isRedaction()) { const redactId = event.event.redacts; // if we know about this event, redact its contents now. @@ -1141,7 +1141,7 @@ Room.prototype.addPendingEvent = function(event, txnId) { this._aggregateNonLiveRelation(event); } - if (event.getType() === "m.room.redaction") { + if (event.isRedaction()) { const redactId = event.event.redacts; let redactedEvent = this._pendingEventList && this._pendingEventList.find(e => e.getId() === redactId); @@ -1333,7 +1333,7 @@ Room.prototype.updatePendingEvent = function(event, newStatus, newEventId) { const idx = this._pendingEventList.findIndex(ev => ev.getId() === oldEventId); if (idx !== -1) { const [removedEvent] = this._pendingEventList.splice(idx, 1); - if (removedEvent.getType() === "m.room.redaction") { + if (removedEvent.isRedaction()) { this._revertRedactionLocalEcho(removedEvent); } } @@ -1442,7 +1442,7 @@ Room.prototype.removeEvent = function(eventId) { for (let i = 0; i < this._timelineSets.length; i++) { const removed = this._timelineSets[i].removeEvent(eventId); if (removed) { - if (removed.getType() === "m.room.redaction") { + if (removed.isRedaction()) { this._revertRedactionLocalEcho(removed); } removedAny = true; From 811a98ad1953f1ffb6b8b00d5310bf4dc5782f21 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 13 Jun 2019 12:03:20 +0200 Subject: [PATCH 33/41] whitespace, newlines --- spec/unit/scheduler.spec.js | 9 +++------ src/client.js | 1 + 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/spec/unit/scheduler.spec.js b/spec/unit/scheduler.spec.js index a513f97b9..b518e8323 100644 --- a/spec/unit/scheduler.spec.js +++ b/spec/unit/scheduler.spec.js @@ -105,8 +105,7 @@ describe("MatrixScheduler", function() { }); scheduler.queueEvent(eventA); - // as queueing doesn't start processing - // synchronously anymore (see commit bbdb5ac) + // as queueing doesn't start processing synchronously anymore (see commit bbdb5ac) // wait just long enough before it does await Promise.resolve(); expect(procCount).toEqual(1); @@ -147,8 +146,7 @@ describe("MatrixScheduler", function() { const globalA = scheduler.queueEvent(eventA); scheduler.queueEvent(eventB); - // as queing doesn't start processing - // synchronously anymore (see commit bbdb5ac) + // as queueing doesn't start processing synchronously anymore (see commit bbdb5ac) // wait just long enough before it does await Promise.resolve(); expect(procCount).toEqual(1); @@ -310,8 +308,7 @@ describe("MatrixScheduler", function() { expect(ev).toEqual(eventA); return defer.promise; }); - // as queueing doesn't start processing - // synchronously anymore (see commit bbdb5ac) + // as queueing doesn't start processing synchronously anymore (see commit bbdb5ac) // wait just long enough before it does Promise.resolve().then(() => { expect(procCount).toEqual(1); diff --git a/src/client.js b/src/client.js index 239dc3b1a..4c146b9d0 100644 --- a/src/client.js +++ b/src/client.js @@ -1742,6 +1742,7 @@ MatrixClient.prototype._sendCompleteEvent = function(roomId, eventObject, txnId, localEvent.updateAssociatedId(target.getId()); }); } + const type = localEvent.getType(); logger.log(`sendEvent of type ${type} in ${roomId} with txnId ${txnId}`); From 3488fbe64cb3e66a1c536fa0ead7cf7ec86a34e9 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 13 Jun 2019 12:03:35 +0200 Subject: [PATCH 34/41] expand comment why need to preserve redaction local echo on remote echo --- src/models/event.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/models/event.js b/src/models/event.js index abb6f8a24..560ca1103 100644 --- a/src/models/event.js +++ b/src/models/event.js @@ -788,8 +788,11 @@ utils.extend(module.exports.MatrixEvent.prototype, { const oldUnsigned = this.getUnsigned(); const oldId = this.getId(); this.event = event; - // keep being redacted if - // the event was redacted as a local echo + // if this event was redacted before it was sent, it's locally marked as redacted. + // At this point, we've received the remote echo for the event, but not yet for + // the redaction that we are sending ourselves. Preserve the locally redacted + // state by copying over redacted_because so we don't get a flash of + // redacted, not-redacted, redacted as remote echos come in if (oldUnsigned.redacted_because) { if (!this.event.unsigned) { this.event.unsigned = {}; From 2a0c85c7723bd57f5ad8bc9b4ea0c8efaeb538f8 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 13 Jun 2019 12:06:54 +0200 Subject: [PATCH 35/41] add hasAssociation helper --- src/models/event.js | 9 +++++++++ src/scheduler.js | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/models/event.js b/src/models/event.js index 560ca1103..50adc34a8 100644 --- a/src/models/event.js +++ b/src/models/event.js @@ -921,6 +921,15 @@ utils.extend(module.exports.MatrixEvent.prototype, { } }, + /** + * Checks if this event is associated with another event. See `getAssociatedId`. + * + * @return {bool} + */ + hasAssocation() { + return !!this.getAssociatedId(); + }, + /** * Update the related id with a new one. * diff --git a/src/scheduler.js b/src/scheduler.js index b3461bebd..a799597b9 100644 --- a/src/scheduler.js +++ b/src/scheduler.js @@ -178,7 +178,7 @@ MatrixScheduler.RETRY_BACKOFF_RATELIMIT = function(event, attempts, err) { */ MatrixScheduler.QUEUE_MESSAGES = function(event) { // enqueue messages or events that associate with another event (redactions and relations) - if (event.getType() === "m.room.message" || !!event.getAssociatedId()) { + if (event.getType() === "m.room.message" || event.hasAssocation()) { // put these events in the 'message' queue. return "message"; } From 6059df1b678db623702d7b4f0dd6736f7e4c7c9a Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 13 Jun 2019 12:12:02 +0200 Subject: [PATCH 36/41] move CANCELLED check deeper into aggregation path --- src/models/event-timeline-set.js | 3 ++- src/models/room.js | 5 +---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/models/event-timeline-set.js b/src/models/event-timeline-set.js index 5b852f5b0..8f2fad30e 100644 --- a/src/models/event-timeline-set.js +++ b/src/models/event-timeline-set.js @@ -20,6 +20,7 @@ limitations under the License. const EventEmitter = require("events").EventEmitter; const utils = require("../utils"); const EventTimeline = require("./event-timeline"); +import {EventStatus} from "./event"; import logger from '../../src/logger'; import Relations from './relations'; @@ -749,7 +750,7 @@ EventTimelineSet.prototype.aggregateRelations = function(event) { return; } - if (event.isRedacted()) { + if (event.isRedacted() || event.status === EventStatus.CANCELLED) { return; } diff --git a/src/models/room.js b/src/models/room.js index 46eda57ec..a77aaa24f 100644 --- a/src/models/room.js +++ b/src/models/room.js @@ -1356,10 +1356,7 @@ Room.prototype._revertRedactionLocalEcho = function(redactionEvent) { // re-render after undoing redaction this.emit("Room.redactionCancelled", redactionEvent, this); // reapply relation now redaction failed - if ( - redactedEvent.isRelation() && - redactedEvent.status !== EventStatus.CANCELLED - ) { + if (redactedEvent.isRelation()) { this._aggregateNonLiveRelation(redactedEvent); } } From 64daa444dd2af1f0cc4d124fd1189db4b9d0f9d4 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Thu, 13 Jun 2019 10:55:06 -0400 Subject: [PATCH 37/41] Key verification request fixes - fix requestVerification in MatrixClient to match the function in crypto - reorder the arguments so that the arguments actually do what they say they do - pass through the third argument, which was accidentally omitted - ignore verification requests from ourselves - also fix a comment --- src/client.js | 8 ++++---- src/crypto/index.js | 7 ++++++- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/client.js b/src/client.js index d7dada2e5..7dc8b58dd 100644 --- a/src/client.js +++ b/src/client.js @@ -734,19 +734,19 @@ async function _setDeviceVerification( * Request a key verification from another user. * * @param {string} userId the user to request verification with - * @param {Array} devices array of device IDs to send requests to. Defaults to - * all devices owned by the user * @param {Array} methods array of verification methods to use. Defaults to * all known methods + * @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 */ -MatrixClient.prototype.requestVerification = function(userId, devices, methods) { +MatrixClient.prototype.requestVerification = function(userId, methods, devices) { if (this._crypto === null) { throw new Error("End-to-end encryption disabled"); } - return this._crypto.requestVerification(userId, devices); + return this._crypto.requestVerification(userId, methods, devices); }; /** diff --git a/src/crypto/index.js b/src/crypto/index.js index 4854ea194..91b7c339b 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -1665,6 +1665,11 @@ Crypto.prototype._onKeyVerificationRequest = function(event) { } const sender = event.getSender(); + if (sender === this._userId && content.from_device === this._deviceId) { + // ignore requests from ourselves, because it doesn't make sense for a + // device to verify itself + return; + } if (this._verificationTransactions.has(sender)) { if (this._verificationTransactions.get(sender).has(content.transaction_id)) { // transaction already exists: cancel it and drop the existing @@ -1717,7 +1722,7 @@ Crypto.prototype._onKeyVerificationRequest = function(event) { }, ); } else { - // notify the application that of the verification request, so it can + // notify the application of the verification request, so it can // decide what to do with it const request = { event: event, From 8aeb99483948b26f0136bbe1ed5b48b70d1f1853 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 13 Jun 2019 16:23:11 +0100 Subject: [PATCH 38/41] Expose the inhibit_login flag to register --- src/base-apis.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/base-apis.js b/src/base-apis.js index 6ef0b6ee9..9b56bcbad 100644 --- a/src/base-apis.js +++ b/src/base-apis.js @@ -151,13 +151,14 @@ MatrixBaseApis.prototype.isUsernameAvailable = function(username) { * threepid uses during registration in the ID server. Set 'msisdn' to * true to bind msisdn. * @param {string} guestAccessToken + * @param {string} inhibitLogin * @param {module:client.callback} callback Optional. * @return {module:client.Promise} Resolves: TODO * @return {module:http-api.MatrixError} Rejects: with an error response. */ MatrixBaseApis.prototype.register = function( username, password, - sessionId, auth, bindThreepids, guestAccessToken, + sessionId, auth, bindThreepids, guestAccessToken, inhibitLogin, callback, ) { // backwards compat @@ -166,6 +167,10 @@ MatrixBaseApis.prototype.register = function( } else if (bindThreepids === null || bindThreepids === undefined) { bindThreepids = {}; } + if (typeof inhibit_login === 'function') { + callback = inhibitLogin; + inhibitLogin = undefined; + } if (auth === undefined || auth === null) { auth = {}; @@ -192,6 +197,9 @@ MatrixBaseApis.prototype.register = function( if (guestAccessToken !== undefined && guestAccessToken !== null) { params.guest_access_token = guestAccessToken; } + if (inhibitLogin !== undefined && inhibitLogin !== null) { + params.inhibit_login = inhibitLogin; + } // Temporary parameter added to make the register endpoint advertise // msisdn flows. This exists because there are clients that break // when given stages they don't recognise. This parameter will cease From 65dd5cc6ad693b707baf4c1b7803d8f06f8f7fc2 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 13 Jun 2019 16:30:39 +0100 Subject: [PATCH 39/41] use right variable --- src/base-apis.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/base-apis.js b/src/base-apis.js index 9b56bcbad..2d1c95d2c 100644 --- a/src/base-apis.js +++ b/src/base-apis.js @@ -167,7 +167,7 @@ MatrixBaseApis.prototype.register = function( } else if (bindThreepids === null || bindThreepids === undefined) { bindThreepids = {}; } - if (typeof inhibit_login === 'function') { + if (typeof inhibitLogin === 'function') { callback = inhibitLogin; inhibitLogin = undefined; } From 0121bdbb75467f0a275d706d747f32be2e65055c Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Fri, 14 Jun 2019 08:23:27 -0600 Subject: [PATCH 40/41] welcome back, Olm --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index e5cd9051b..0504eabcc 100644 --- a/README.md +++ b/README.md @@ -296,7 +296,7 @@ Then visit ``http://localhost:8005`` to see the API docs. End-to-end encryption support ============================= -The SDK supports end-to-end encryption via the and Megolm protocols, using +The SDK supports end-to-end encryption via the Olm and Megolm protocols, using [libolm](https://gitlab.matrix.org/matrix-org/olm). It is left up to the application to make libolm available, via the ``Olm`` global. From 44bfc2e8469ff137140480e1b817864994dcfdf0 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 14 Jun 2019 15:57:49 +0100 Subject: [PATCH 41/41] Add flag to force saving sync store Add a 'force' flag to to the save method of the store to force the store to sace its data even if it wouldn't normally. --- src/store/indexeddb.js | 6 ++++-- src/store/memory.js | 4 +++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/store/indexeddb.js b/src/store/indexeddb.js index 8ca3f5244..dd8978b76 100644 --- a/src/store/indexeddb.js +++ b/src/store/indexeddb.js @@ -198,11 +198,13 @@ IndexedDBStore.prototype.wantsSave = function() { /** * Possibly write data to the database. + * + * @param {bool} force True to force a save to happen * @return {Promise} Promise resolves after the write completes * (or immediately if no write is performed) */ -IndexedDBStore.prototype.save = function() { - if (this.wantsSave()) { +IndexedDBStore.prototype.save = function(force) { + if (force || this.wantsSave()) { return this._reallySave(); } return Promise.resolve(); diff --git a/src/store/memory.js b/src/store/memory.js index df74f9860..7a666d071 100644 --- a/src/store/memory.js +++ b/src/store/memory.js @@ -335,8 +335,10 @@ module.exports.MemoryStore.prototype = { /** * Save does nothing as there is no backing data store. + * @param {bool} force True to force a save (but the memory + * store still can't save anything) */ - save: function() {}, + save: function(force) {}, /** * Startup does nothing as this store doesn't require starting up.