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

Fix matrix api joinRoom() for remote rooms #60

Open
Elliu wants to merge 6 commits from Elliu/out-of-your-element:fix-remote-join into main
pull from: Elliu/out-of-your-element:fix-remote-join
merge into: cadence:main
Conversation 2 Commits 6 Files changed 6 +119 -16
Elliu commented 2025-08-22 08:17:38 +00:00
First-time 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
First-time 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
First-time 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
First-time 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
First-time 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
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u fix-remote-join:Elliu-fix-remote-join
git checkout Elliu-fix-remote-join

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git checkout main
git merge --no-ff Elliu-fix-remote-join
git checkout Elliu-fix-remote-join
git rebase main
git checkout main
git merge --ff-only Elliu-fix-remote-join
git checkout Elliu-fix-remote-join
git rebase main
git checkout main
git merge --no-ff Elliu-fix-remote-join
git checkout main
git merge --squash Elliu-fix-remote-join
git checkout main
git merge --ff-only Elliu-fix-remote-join
git checkout main
git merge Elliu-fix-remote-join
git push origin main
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?