Support routing matrix.to links to joinable rooms #2250

turt2live · 2018-10-25T00:16:41Z

Fixes element-hq/element-web#2925 (for now)
Fixes https://github.com/vector-im/riot-web/issues/7071 (indirectly)
Fixes element-hq/element-web#7094
Fixes element-hq/element-web#1230 (can join; may not fully work. See element-hq/element-web#1230 (comment))

Requires element-hq/element-web#7552
Requires matrix-org/matrix-js-sdk#764

Please review all 3 PRs

The series of PRs here replace browser-request with request under the hood for the js-sdk. This is because browser-request doesn't do any of the right things for generating a URL like /join?server_name=first.com&server_name=second.com (which is how the spec/synapse expects room joins to work when suggesting servers to join through). request on the other hand does support doing this, albeit in an awkward way - as shown by the js-sdk PR.

browser-request is not completely removed from any of the layers due to it still being used in places. Instead of converting all of the places to request, it seemed easier and less damaging to just leave them be. Replacing the request library in the js-sdk is already a fairly drastic change that might have unforeseen consequences. I've done some light testing ("oh good, riot still loads and I can send a message") but nothing extensive as testing every endpoint would be a nightmare.

Webpack is made of fail and needs memfs installed in all 3 layers. Normally we'd add this to the riot-web readme, however the react-sdk's build also fails because of it. Instead of making it angry in all the places, it's just been included in the package.json for all 3 layers. There's a comment in the webpack config for what is going on here.

Note about matrix.to and introducing the new parameter:

There's no MSC associated with this yet. Technically speaking, ?via= is not a supported option for matrix.to URIs in today's environment. There's no expectation that other clients actually support this yet as this is very much a trial of how permalinking could work for now. A future MSC might make permalinking easier in Matrix, ideally making this solution a stopgap one. Provided ?via meets the expectations of making links actually work, an MSC may be raised to officially include it in the spec.

This is written in a way where the lack of ?via is completely okay and should therefore be backwards compatible with non-via links. Equally, a quick survey of some clients shows that adding ?via should be safe and not break other clients.

This ends up being translated to ?server_name= in the matrix-js-sdk, although that has a bug at the time of writing. It converts `server_name: ['a', 'b']` to `?server_name=a,b` instead of `?server_name=a&server_name=b` For reference: the `viaServers` option is routed through the 'join_room' action to RoomViewStore#_joinRoom which is passed directly to the js-sdk http-api#joinRoom function. Next steps: * Fix the js-sdk parsing * Make the SDK generate matrix.to links with ?via=

…outing

turt2live · 2018-10-25T00:17:45Z

This is still a WIP while I fix tests and finish testing it - just wanted to get it out there to show progress and get any early comments.

Half-Shot · 2018-10-25T00:29:59Z

Yay, this is cool! I assume once this is in and solidified an MSC could be written for the additional parameter?

turt2live · 2018-10-25T00:35:29Z

Yup. I want to make sure it actually solves the problem we expect it to without causing problems for people (ie: are the servers we picked actually helpful? does supplying servers actually make joins easier? etc). Once refined, an MSC will be opened assuming the refinement isn't to ditch the idea.

In the event someone changes how the hostname parsing works.

Tests do wonders.

t3chguy · 2018-10-26T16:19:12Z

src/matrix-to.js

+    if (highestPlUser.powerLevel >= 50) candidates.push(highestPlUser.serverName);
+
+    const beforePopulation = candidates.length;
+    const maxCandidates = 3;


I feel like this const should be outside of the method to make its purpose even clearer
(along with a comment in case anyone is hunting through the code which thing to tweak)

turt2live · 2018-10-26T16:33:23Z

src/components/structures/UserSettings.js

@@ -1298,7 +1298,7 @@ module.exports = React.createClass({
        // If the olmVersion is not defined then either crypto is disabled, or
        // we are using a version old version of olm. We assume the former.
        let olmVersionString = "<not-enabled>";
-        if (olmVersion !== undefined) {
+        if (olmVersion) {


note: this is unrelated to the changeset here, but has a 50% chance of being the solution to the build failing in earlier commits. It seems worth keeping the change regardless of build status imo.

src/matrix-to.js

ara4n · 2018-10-26T17:56:34Z

src/matrix-to.js

+    //   room.
+    //
+    //   Server 2: The next most popular server in the room (in user
+    //   distribution). This will probably be matrix.org in most cases


I'd avoid mentioning matrix.org here; it's very much a misfeature that public rooms are dominated by matrix.org users today, plus this ignores usages in private federations.

This is untrue for quite a few real scenarios, including private federations and a ton of rooms.

ara4n

lgtm. (and thanks for providing tests)! my only concern is to just chuck a few lines into an MSC to doc how the vias param on matrix.to is meant to work.

turt2live · 2018-10-26T20:22:04Z

MSC: matrix-org/matrix-spec-proposals#1704

aaronraimist · 2018-10-27T01:14:28Z

This breaks room completion pills

Fixes element-hq/element-web#7614 Regression of #2250

This was introduced in matrix-org/matrix-react-sdk#2250 but can be pulled out due to matrix-org/matrix-js-sdk#770. See #7634 for more information about the future.

This was introduced in #2250 but can be pulled out due to matrix-org/matrix-js-sdk#770. See element-hq/element-web#7634 for more information about the future.

This was introduced in matrix-org/matrix-react-sdk#2250 but can be pulled out due to #770. See element-hq/element-web#7634 for more information about the future.

Spec for [MSC1704](#1704) Reference implementations: * Original: matrix-org/matrix-react-sdk#2250 * Modern recommendations: https://github.com/matrix-org/matrix-react-sdk/blob/2ca281f6b7b735bc96945370584c5cb1b5f7e1f1/src/matrix-to.js#L29-L70 The only deviation from the original MSC is the recommendation for which servers to pick. The original MSC failed to consider server ACLs and IP addresses correctly, and during implementation it was realized that both of these cases should be handled. The core principles of the original MSC are left unaltered.

turt2live added 5 commits October 19, 2018 13:31

Merge branch 'develop' into travis/permalink-routing

d7367a7

Merge remote-tracking branch 'origin/develop' into travis/permalink-r…

6ee495b

…outing

Install memfs because webpack is made of fail

a878212

Pick servers for ?via on matrix.to links based on some heuristics

e8cb636

This was referenced Oct 25, 2018

Mock fs, tls, and net to support request in the browser element-hq/element-web#7552

Merged

Support request in the browser and support supplying servers to try in joinRoom() matrix-org/matrix-js-sdk#764

Merged

turt2live changed the title ~~Travis/permalink routing~~ [WIP] Support routing matrix.to links to joinable rooms Oct 25, 2018

turt2live added 5 commits October 25, 2018 15:21

Fix Karma/Webpack so it can build the tests

54ff5d8

Fix the tests so they actually test something

b9bfbdc

Add hostname sanity tests

43980ad

In the event someone changes how the hostname parsing works.

Fix the server candidate picker to actually work

5209496

Tests do wonders.

Merge branch 'develop' into travis/permalink-routing

d58ff90

turt2live mentioned this pull request Oct 25, 2018

Context links give a 404 on federated rooms element-hq/element-web#1230

Closed

turt2live changed the title ~~[WIP] Support routing matrix.to links to joinable rooms~~ Support routing matrix.to links to joinable rooms Oct 25, 2018

turt2live requested a review from a team October 25, 2018 21:42

turt2live added 2 commits October 26, 2018 09:19

Merge branch 'develop' into travis/permalink-routing

ea8a37f

Maybe fix UserSettings?

ef8c924

t3chguy reviewed Oct 26, 2018

View reviewed changes

turt2live added 2 commits October 26, 2018 10:22

Appease the linter

c389540

Move the max candidates constant out of the function

d802ee0

turt2live commented Oct 26, 2018

View reviewed changes

turt2live requested a review from a team October 26, 2018 16:45

ara4n reviewed Oct 26, 2018

View reviewed changes

src/matrix-to.js Show resolved Hide resolved

ara4n reviewed Oct 26, 2018

View reviewed changes

turt2live added 2 commits October 26, 2018 12:21

Don't mention that matrix.org is likely to be popular

3734c8a

This is untrue for quite a few real scenarios, including private federations and a ton of rooms.

Clarify why we pick popular servers over the one that created the room

0857e2c

ara4n approved these changes Oct 26, 2018

View reviewed changes

turt2live merged commit 0bd1d6b into develop Oct 26, 2018

turt2live deleted the travis/permalink-routing branch October 26, 2018 20:23

turt2live mentioned this pull request Oct 27, 2018

matrix.to?via breaks autocomplete pills for rooms element-hq/element-web#7614

Closed

turt2live added a commit that referenced this pull request Oct 27, 2018

Fix and test matrix.to alias permalinks

3bc5e2b

Fixes element-hq/element-web#7614 Regression of #2250

turt2live mentioned this pull request Oct 27, 2018

Fix and test matrix.to alias permalinks #2254

Merged

turt2live added a commit that referenced this pull request Oct 31, 2018

Remove the request-only stuff we don't need anymore

8731334

This was introduced in #2250 but can be pulled out due to matrix-org/matrix-js-sdk#770. See element-hq/element-web#7634 for more information about the future.

This was referenced Oct 31, 2018

Remove the request-only stuff we don't need anymore element-hq/element-web#7637

Merged

Remove the request-only stuff we don't need anymore #2263

Merged

Remove the request-only stuff we don't need anymore matrix-org/matrix-js-sdk#775

Merged

matthijskooijman mentioned this pull request Feb 22, 2019

Opening room links with ?via does not work and shows no error element-hq/riot-android#2992

Open

turt2live mentioned this pull request Apr 5, 2019

Add permalink routing through ?via parameters on matrix.to URIs matrix-org/matrix-spec-proposals#1955

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support routing matrix.to links to joinable rooms #2250

Support routing matrix.to links to joinable rooms #2250

turt2live commented Oct 25, 2018 •

edited

Loading

turt2live commented Oct 25, 2018

Half-Shot commented Oct 25, 2018

turt2live commented Oct 25, 2018

t3chguy Oct 26, 2018 •

edited

Loading

t3chguy Oct 26, 2018

turt2live Oct 26, 2018

ara4n Oct 26, 2018

ara4n left a comment

turt2live commented Oct 26, 2018

aaronraimist commented Oct 27, 2018

Support routing matrix.to links to joinable rooms #2250

Support routing matrix.to links to joinable rooms #2250

Conversation

turt2live commented Oct 25, 2018 • edited Loading

turt2live commented Oct 25, 2018

Half-Shot commented Oct 25, 2018

turt2live commented Oct 25, 2018

t3chguy Oct 26, 2018 • edited Loading

Choose a reason for hiding this comment

t3chguy Oct 26, 2018

Choose a reason for hiding this comment

turt2live Oct 26, 2018

Choose a reason for hiding this comment

ara4n Oct 26, 2018

Choose a reason for hiding this comment

ara4n left a comment

Choose a reason for hiding this comment

turt2live commented Oct 26, 2018

aaronraimist commented Oct 27, 2018

turt2live commented Oct 25, 2018 •

edited

Loading

t3chguy Oct 26, 2018 •

edited

Loading