cadence/out-of-your-element
cadence/out-of-your-element
3
7
Fork 3
Code Issues 11 Pull requests 2 Releases 22 Activity

Fix matrix api joinRoom() for remote rooms #60

Merged
cadence merged 9 commits from Elliu/out-of-your-element:fix-remote-join into main 2025-11-02 07:50:16 +00:00
Conversation 3 Commits 9 Files changed 8 +115 -18
Elliu commented 2025-08-22 08:17:38 +00:00
Contributor
Copy link

When using self-service mode and trying to link with a remote matrix
room (room not in the same HS as the bridge user), then we need to add
the "via" HSs to join the room with, or else it fails.

We get it from the "m.space.child" in the "children_state" of the space
hierarchy.

It seems like the "via" information can also be stored in the
"m.space.parent" in the states of the room, but hopefully this shouldn't
be needed in sane implementations

When using self-service mode and trying to link with a remote matrix room (room not in the same HS as the bridge user), then we need to add the "via" HSs to join the room with, or else it fails. We get it from the "m.space.child" in the "children_state" of the space hierarchy. It seems like the "via" information can also be stored in the "m.space.parent" in the states of the room, but hopefully this shouldn't be needed in sane implementations
Elliu added 1 commit 2025-08-22 08:17:38 +00:00 
When using self-service mode and trying to link with a remote matrix
room (room not in the same HS as the bridge user), then we need to add
the "via" HSs to join the room with, or else it fails.

We get it from the "m.space.child" in the "children_state" of the space
hierarchy.

It seems like the "via" information can also be stored in the
"m.space.parent" in the states of the room, but hopefully this shouldn't
be needed in sane implementations
Elliu reviewed 2025-08-22 08:21:07 +00:00
src/web/routes/link.js Outdated
@ -149,2 +160,3 @@
await api.joinRoom(parsedBody.matrix, null, via ?? {})
} catch (e) {
throw createError({status: 403, message: e.errcode, data: `${e.errcode} - ${e.message}`})
throw createError({status: 403, message: e.errcode, data: `${e.errcode} - ${e.message}${via === null ? " (hint: couln't find a \"via\" in the space children_state for this room in order to help joining this room)" : ""}`})
Elliu commented 2025-08-22 08:21:07 +00:00
Author
Contributor
Copy link

Not sure if adding this hint is a good idea, or if we should rather put it elsewhere (like in a "hint" member), or if we should not put it at all, or if we should add a "please consider manually inviting the bot user <> in the room"

Not sure if adding this hint is a good idea, or if we should rather put it elsewhere (like in a "hint" member), or if we should not put it at all, or if we should add a "please consider manually inviting the bot user <> in the room"
cadence commented 2025-08-22 09:29:05 +00:00
Owner
Copy link

I added a separate branch for it to say a different error message when the join fails. I felt the ternary was too complicated.

or if we should add a "please consider manually inviting

I decided to do it this way because it's better to see actionable feedback (do this) rather than merely identifying a problem (it tells you that the via data is wrong, but not what correct data would look like, or how to actually fix it) (I don't know any clients that allow you to edit the via data without devtools)

I added a separate branch for it to say a different error message when the join fails. I felt the ternary was too complicated. > or if we should add a "please consider manually inviting I decided to do it this way because it's better to see actionable feedback (do this) rather than merely identifying a problem (it tells you that the via data is wrong, but not what correct data would look like, or how to actually fix it) (I don't know any clients that allow you to edit the via data without devtools)
cadence marked this conversation as resolved
cadence added 1 commit 2025-08-22 09:25:15 +00:00
cadence force-pushed fix-remote-join from e8bb63dd3a to 743fec6e69 2025-08-22 09:26:57 +00:00 Compare
cadence reviewed 2025-08-22 09:32:09 +00:00
src/matrix/api.js Outdated
@ -23,3 +23,3 @@
if (mxid) u.searchParams.set("user_id", mxid)
for (const entry of Object.entries(otherParams)) {
if (entry[1] != undefined) {
if (Array.isArray(entry[1])) {
cadence commented 2025-08-22 09:30:50 +00:00
Owner
Copy link

I fixed path so that passing arrays (like via) to it should work correctly. :)

I fixed `path` so that passing arrays (like `via`) to it should work correctly. :)
src/matrix/api.js Outdated
@ -68,2 +74,3 @@
async function joinRoom(roomIDOrAlias, mxid, via) {
/** @type {Ty.R.RoomJoined} */
const root = await mreq.mreq("POST", path(`/client/v3/join/${roomIDOrAlias}`, mxid), {})
const root = await mreq.mreq("POST", path(`/client/v3/join/${roomIDOrAlias}`, mxid, {via}), {})
cadence commented 2025-08-22 09:30:17 +00:00
Owner
Copy link

Third parameter needs to be an object, so I fixed that for you. I also labelled the inbound parameters so that it highlights the call site if the wrong data is passed to joinRoom. :)

Third parameter needs to be an object, so I fixed that for you. I also labelled the inbound parameters so that it highlights the call site if the wrong data is passed to `joinRoom`. :)
src/web/routes/link.js Outdated
@ -78,3 +78,3 @@
// Check space exists and bridge is joined
try {
await api.joinRoom(parsedBody.space_id)
await api.joinRoom(parsedBody.space_id, null, via)
cadence commented 2025-08-22 09:29:28 +00:00
Owner
Copy link

Where are you going to find via here?

Where are you going to find `via` here?
Elliu commented 2025-08-22 14:40:36 +00:00
Author
Contributor
Copy link

Hmmm, I didn't change that because the bridge needed to be manually invited to the space so that it can be selected.

However indeed, if the bridge is invited to the space, then kicked, the button to the space remains and if clicked, we have the same issue than for joining rooms, didn't notice that.

From the Client-Server API doc there doesn't seem to be "via" or "server_name" information on the m.room.member event, so I'm not sure.
I guess it should be safe to take the HS part of the user from sender in the m.room.member: as this user is able to send an invite, the space must be reachable through the inviter's HS

(unless the invite is old, all users from this HS left the space, and the room was purged from the HS, but nothing we can do about that)

Hmmm, I didn't change that because the bridge needed to be manually invited to the space so that it can be selected. However indeed, if the bridge is invited to the space, then kicked, the button to the space remains and if clicked, we have the same issue than for joining rooms, didn't notice that. From the Client-Server API doc there doesn't seem to be "via" or "server_name" information on the `m.room.member` event, so I'm not sure. I guess it should be safe to take the HS part of the user from `sender` in the `m.room.member`: as this user is able to send an invite, the space must be reachable through the inviter's HS (unless the invite is old, all users from this HS left the space, and the room was purged from the HS, but nothing we can do about that)
Elliu commented 2025-08-22 15:12:50 +00:00
Author
Contributor
Copy link

Last commit uses that, should we add a check to enrich the error message too if for some reasons we cannot detect the via?

Last commit uses that, should we add a check to enrich the error message too if for some reasons we cannot detect the via?
Elliu commented 2025-08-31 11:13:51 +00:00
Author
Contributor
Copy link

Added the error message asking to manually invite in case if /api/link-space joinRoom fails, similarly to /api/link

Added the error message asking to manually invite in case if /api/link-space joinRoom fails, similarly to /api/link
cadence added 1 commit 2025-08-22 09:42:20 +00:00
Elliu added 1 commit 2025-08-22 15:02:24 +00:00
Elliu force-pushed fix-remote-join from b6c3e37478 to 6734c15ca2 2025-08-22 15:10:46 +00:00 Compare
cadence added 1 commit 2025-08-23 11:46:58 +00:00
Elliu force-pushed fix-remote-join from 48e2080b94 to f6c749acca 2025-08-31 11:12:11 +00:00 Compare
cadence reviewed 2025-10-07 19:24:01 +00:00
cadence left a comment
Owner
Copy link

Looks good. Please address the invididual comments (or ask me to do it for you). Then I can do a final coverage check and merge this. Thanks for your patience!

Looks good. Please address the invididual comments (or ask me to do it for you). Then I can do a final coverage check and merge this. Thanks for your patience!
src/web/routes/link.js Outdated
@ -15,0 +16,4 @@
* @param {string} UserID
* @returns {string} the HS of the user, or "" if the user ID is malformed
*/
function getHSOfUser(user) {
cadence commented 2025-10-07 19:16:24 +00:00
Owner
Copy link

Please use server = mxid.match(/:(.*)/)?.[1] (see utils.js line 168) which is short enough that it can be used inline without needing to be extracted to a function. So please delete the function getHSOfUser and just do the operation inline where needed.

Please use `server = mxid.match(/:(.*)/)?.[1]` (see utils.js line 168) which is short enough that it can be used inline without needing to be extracted to a function. So please delete the function `getHSOfUser` and just do the operation inline where needed.
Elliu commented 2025-10-09 12:55:43 +00:00
Author
Contributor
Copy link

Updated, had to add a null coalescing or else linter complains about array of string OR undefined a few lines below

Updated, had to add a null coalescing or else linter complains about array of string OR undefined a few lines below
src/web/routes/link.js Outdated
@ -76,2 +90,4 @@
if (existing) throw createError({status: 400, message: "Bad Request", data: `Guild ID ${guildID} or space ID ${spaceID} are already bridged and cannot be reused`})
const inviteSender = select("invite", "mxid", {mxid: session.data.mxid, room_id: spaceID}).pluck().get()
via = [ getHSOfUser(inviteSender) ]
cadence commented 2025-10-07 19:17:49 +00:00
Owner
Copy link

When creating a new variable, it ought to be declared with let or const. Please use const here.

When creating a new variable, it ought to be declared with `let` or `const`. Please use `const` here.
src/web/routes/link.js Outdated
@ -80,2 +97,3 @@
await api.joinRoom(parsedBody.space_id)
await api.joinRoom(parsedBody.space_id, null, via)
} catch (e) {
if (via.join("") == "") {
cadence commented 2025-10-07 19:19:29 +00:00
Owner
Copy link

This condition can't happen, because it's always possible to extract a via from the user who sent the invite. Remove the if statement and just always use the more detailed error message.

This condition can't happen, because it's always possible to extract a `via` from the user who sent the invite. Remove the if statement and just always use the more detailed error message.
src/web/routes/link.js Outdated
@ -81,1 +97,4 @@
await api.joinRoom(parsedBody.space_id, null, via)
} catch (e) {
if (via.join("") == "") {
throw createError({status: 403, message: "Unable To Join", data: `Unable to join the requested Matrix space. Please invite the bridge to the space and try again. (Server said: ${e.errcode} - ${e.message})`})
cadence commented 2025-10-07 19:21:08 +00:00
Owner
Copy link

Status code 400 please.

Note that this error message doesn't make much sense. The user has to have already invited the bridge to the space to reach this point.

I honestly don't know what advice would be good to give if this occurs, because accepting an invite should always work, even for remote invites. /shrug

Status code 400 please. Note that this error message doesn't make much sense. The user has to have already invited the bridge to the space to reach this point. I honestly don't know what advice would be good to give if this occurs, because accepting an invite should always work, even for remote invites. /shrug
Elliu commented 2025-10-09 12:42:12 +00:00
Author
Contributor
Copy link

Yeah, I don't know.
The idea at the back of my mind was a weird state that could happen if:

  1. Space is not on the same HS than bridge
  2. Bridge is invited / join the space
  3. User kicks / remove the bridge from the space
  4. User leave the space
  5. No remaining user from this HS on the space, space is purged from the user's HS
  6. /api/link-space gets called somehow(?)
  7. bridge tries to join using the "via" of origin invite, but space got purged from the via-ed HS in between, so fails, requiring a reinvite from a user that is currently in the space

Don't know if all of this even makes sense

Yeah, I don't know. The idea at the back of my mind was a weird state that could happen if: 1. Space is not on the same HS than bridge 1. Bridge is invited / join the space 2. User kicks / remove the bridge from the space 3. User leave the space 4. No remaining user from this HS on the space, space is purged from the user's HS 5. `/api/link-space` gets called somehow(?) 6. bridge tries to join using the "via" of origin invite, but space got purged from the via-ed HS in between, so fails, requiring a reinvite from a user that is currently in the space Don't know if all of this even makes sense
src/web/routes/link.js Outdated
@ -149,1 +179,4 @@
await api.joinRoom(parsedBody.matrix, null, foundVia)
} catch (e) {
if (!foundVia) {
throw createError({status: 403, message: "Unable To Join", data: `Unable to join the requested Matrix room. Please invite the bridge to the room and try again. (Server said: ${e.errcode} - ${e.message})`})
cadence commented 2025-10-07 19:22:56 +00:00
Owner
Copy link

This is fine and doesn't need changing (apart from making the status code 400, please)

This is fine and doesn't need changing (apart from making the status code 400, please)
Elliu added 1 commit 2025-10-09 12:54:34 +00:00
Elliu force-pushed fix-remote-join from 23d87fb9a4 to ab69eab8a4 2025-11-01 12:02:33 +00:00 Compare
cadence added 1 commit 2025-11-02 07:46:39 +00:00
cadence force-pushed fix-remote-join from 789c7b0334 to a55d7a1632 2025-11-02 07:48:52 +00:00 Compare
cadence merged commit d95a114377 into main 2025-11-02 07:50:16 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
blocking

critical issue that blocks release of milestone

  
web
No labels
blocking
web
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: cadence/out-of-your-element#60
No description provided.
Delete branch "Elliu/out-of-your-element:fix-remote-join"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?