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
6 changed files with 119 additions and 16 deletions

View file

@ -22,7 +22,11 @@ function path(p, mxid, otherParams = {}) {
const u = new URL(p, "http://localhost") const u = new URL(p, "http://localhost")
if (mxid) u.searchParams.set("user_id", mxid) if (mxid) u.searchParams.set("user_id", mxid)
for (const entry of Object.entries(otherParams)) { for (const entry of Object.entries(otherParams)) {
if (entry[1] != undefined) { if (Array.isArray(entry[1])) {

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. :)
for (const element of entry[1]) {
u.searchParams.append(entry[0], element)
}
} else if (entry[1] != undefined) {
u.searchParams.set(entry[0], entry[1]) u.searchParams.set(entry[0], entry[1])
} }
} }
@ -62,11 +66,14 @@ async function createRoom(content) {
} }
/** /**
* @param {string} roomIDOrAlias
* @param {string?} [mxid]
* @param {string[]?} [via]
* @returns {Promise<string>} room ID * @returns {Promise<string>} room ID
*/ */
async function joinRoom(roomIDOrAlias, mxid) { async function joinRoom(roomIDOrAlias, mxid, via) {
/** @type {Ty.R.RoomJoined} */ /** @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}), {})

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`. :)
return root.room_id return root.room_id
} }

View file

@ -24,3 +24,7 @@ test("api path: real world mxid", t => {
test("api path: extras number works", t => { test("api path: extras number works", t => {
t.equal(path(`/client/v3/rooms/!example/timestamp_to_event`, null, {ts: 1687324651120}), "/client/v3/rooms/!example/timestamp_to_event?ts=1687324651120") t.equal(path(`/client/v3/rooms/!example/timestamp_to_event`, null, {ts: 1687324651120}), "/client/v3/rooms/!example/timestamp_to_event?ts=1687324651120")
}) })
test("api path: multiple via params", t => {
t.equal(path(`/client/v3/rooms/!example/join`, null, {via: ["cadence.moe", "matrix.org"], ts: 1687324651120}), "/client/v3/rooms/!example/join?via=cadence.moe&via=matrix.org&ts=1687324651120")
})

10
src/types.d.ts vendored
View file

@ -148,6 +148,14 @@ export namespace Event {
prev_content?: any prev_content?: any
} }
export type Outer_StrippedChildStateEvent = {
type: string
state_key: string
sender: string
origin_server_ts: number
content: any
}
export type M_Room_Message = { export type M_Room_Message = {
msgtype: "m.text" | "m.emote" msgtype: "m.text" | "m.emote"
body: string body: string
@ -344,7 +352,7 @@ export namespace R {
export type Hierarchy = { export type Hierarchy = {
avatar_url?: string avatar_url?: string
canonical_alias?: string canonical_alias?: string
children_state: {} children_state: Event.Outer_StrippedChildStateEvent[]
guest_can_join: boolean guest_can_join: boolean
join_rule?: string join_rule?: string
name?: string name?: string

View file

@ -12,6 +12,20 @@ const auth = sync.require("../auth")
const mreq = sync.require("../../matrix/mreq") const mreq = sync.require("../../matrix/mreq")
const {reg} = require("../../matrix/read-registration") const {reg} = require("../../matrix/read-registration")
/**
* @param {string} UserID
* @returns {string} the HS of the user, or "" if the user ID is malformed
*/
function getHSOfUser(user) {
domainStartIndex = user.indexOf(":");
if (domainStartIndex >= 1) {
return user.slice(domainStartIndex + 1)
}
return ""
}
/** /**
* @param {H3Event} event * @param {H3Event} event
* @returns {import("../../matrix/api")} * @returns {import("../../matrix/api")}
@ -75,10 +89,16 @@ as.router.post("/api/link-space", defineEventHandler(async event => {
const existing = select("guild_space", "guild_id", {}, "WHERE guild_id = ? OR space_id = ?").get(guildID, spaceID) const existing = select("guild_space", "guild_id", {}, "WHERE guild_id = ? OR space_id = ?").get(guildID, spaceID)
if (existing) throw createError({status: 400, message: "Bad Request", data: `Guild ID ${guildID} or space ID ${spaceID} are already bridged and cannot be reused`}) 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) ]
// Check space exists and bridge is joined // Check space exists and bridge is joined
try { try {
await api.joinRoom(parsedBody.space_id) await api.joinRoom(parsedBody.space_id, null, via)
} catch (e) { } 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})`})
}
throw createError({status: 403, message: e.errcode, data: `${e.errcode} - ${e.message}`}) throw createError({status: 403, message: e.errcode, data: `${e.errcode} - ${e.message}`})
} }
@ -134,19 +154,33 @@ as.router.post("/api/link", defineEventHandler(async event => {
if (row) throw createError({status: 400, message: "Bad Request", data: `Channel ID ${row.channel_id} or room ID ${parsedBody.matrix} are already bridged and cannot be reused`}) if (row) throw createError({status: 400, message: "Bad Request", data: `Channel ID ${row.channel_id} or room ID ${parsedBody.matrix} are already bridged and cannot be reused`})
// Check room is part of the guild's space // Check room is part of the guild's space
let found = false let foundRoom = false
/** @type {string[]?} */
let foundVia = null
for await (const room of api.generateFullHierarchy(spaceID)) { for await (const room of api.generateFullHierarchy(spaceID)) {
if (room.room_id === parsedBody.matrix && !room.room_type) { // When finding a space during iteration, look at space's children state, because we need a `via` to join the room (when we find it later)
found = true for (const state of room.children_state) {
cadence marked this conversation as resolved Outdated

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"

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)
break if (state.type === "m.space.child" && state.state_key === parsedBody.matrix) {
foundVia = state.content.via
}
} }
// When finding a room during iteration, see if it was the requested room (to confirm that the room is in the space)
if (room.room_id === parsedBody.matrix && !room.room_type) {
foundRoom = true
}
if (foundRoom && foundVia) break
} }
if (!found) throw createError({status: 400, message: "Bad Request", data: "Matrix room needs to be part of the bridged space"}) if (!foundRoom) throw createError({status: 400, message: "Bad Request", data: "Matrix room needs to be part of the bridged space"})
// Check room exists and bridge is joined // Check room exists and bridge is joined
try { try {
await api.joinRoom(parsedBody.matrix) await api.joinRoom(parsedBody.matrix, null, foundVia)
} catch (e) { } 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})`})
}
throw createError({status: 403, message: e.errcode, data: `${e.errcode} - ${e.message}`}) throw createError({status: 403, message: e.errcode, data: `${e.errcode} - ${e.message}`})
} }

View file

@ -360,7 +360,7 @@ test("web link room: check that room is part of space (not in hierarchy)", async
t.equal(called, 1) t.equal(called, 1)
}) })
test("web link room: check that bridge can join room", async t => { test("web link room: check that bridge can join room (notices lack of via and asks for invite instead)", async t => {
let called = 0 let called = 0
const [error] = await tryToCatch(() => router.test("post", "/api/link", { const [error] = await tryToCatch(() => router.test("post", "/api/link", {
sessionData: { sessionData: {
@ -381,7 +381,55 @@ test("web link room: check that bridge can join room", async t => {
t.equal(spaceID, "!zTMspHVUBhFLLSdmnS:cadence.moe") t.equal(spaceID, "!zTMspHVUBhFLLSdmnS:cadence.moe")
yield { yield {
room_id: "!NDbIqNpJyPvfKRnNcr:cadence.moe", room_id: "!NDbIqNpJyPvfKRnNcr:cadence.moe",
children_state: {}, children_state: [],
guest_can_join: false,
num_joined_members: 2
}
/* c8 ignore next */
}
}
}))
t.equal(error.data, "Unable to join the requested Matrix room. Please invite the bridge to the room and try again. (Server said: M_FORBIDDEN - not allowed to join I guess)")
t.equal(called, 2)
})
test("web link room: check that bridge can join room (uses via for join attempt)", async t => {
let called = 0
const [error] = await tryToCatch(() => router.test("post", "/api/link", {
sessionData: {
managedGuilds: ["665289423482519565"]
},
body: {
discord: "665310973967597573",
matrix: "!NDbIqNpJyPvfKRnNcr:cadence.moe",
guild_id: "665289423482519565"
},
api: {
async joinRoom(roomID, _, via) {
called++
t.deepEqual(via, ["cadence.moe", "hashi.re"])
throw new MatrixServerError({errcode: "M_FORBIDDEN", error: "not allowed to join I guess"})
},
async *generateFullHierarchy(spaceID) {
called++
t.equal(spaceID, "!zTMspHVUBhFLLSdmnS:cadence.moe")
yield {
room_id: "!NDbIqNpJyPvfKRnNcr:cadence.moe",
children_state: [],
guest_can_join: false,
num_joined_members: 2
}
yield {
room_id: "!zTMspHVUBhFLLSdmnS:cadence.moe",
children_state: [{
type: "m.space.child",
state_key: "!NDbIqNpJyPvfKRnNcr:cadence.moe",
sender: "@elliu:hashi.re",
content: {
via: ["cadence.moe", "hashi.re"]
},
origin_server_ts: 0
}],
guest_can_join: false, guest_can_join: false,
num_joined_members: 2 num_joined_members: 2
} }
@ -414,7 +462,7 @@ test("web link room: check that bridge has PL 100 in target room (event missing)
t.equal(spaceID, "!zTMspHVUBhFLLSdmnS:cadence.moe") t.equal(spaceID, "!zTMspHVUBhFLLSdmnS:cadence.moe")
yield { yield {
room_id: "!NDbIqNpJyPvfKRnNcr:cadence.moe", room_id: "!NDbIqNpJyPvfKRnNcr:cadence.moe",
children_state: {}, children_state: [],
guest_can_join: false, guest_can_join: false,
num_joined_members: 2 num_joined_members: 2
} }
@ -454,7 +502,7 @@ test("web link room: check that bridge has PL 100 in target room (users default)
t.equal(spaceID, "!zTMspHVUBhFLLSdmnS:cadence.moe") t.equal(spaceID, "!zTMspHVUBhFLLSdmnS:cadence.moe")
yield { yield {
room_id: "!NDbIqNpJyPvfKRnNcr:cadence.moe", room_id: "!NDbIqNpJyPvfKRnNcr:cadence.moe",
children_state: {}, children_state: [],
guest_can_join: false, guest_can_join: false,
num_joined_members: 2 num_joined_members: 2
} }
@ -494,7 +542,7 @@ test("web link room: successfully calls createRoom", async t => {
t.equal(spaceID, "!zTMspHVUBhFLLSdmnS:cadence.moe") t.equal(spaceID, "!zTMspHVUBhFLLSdmnS:cadence.moe")
yield { yield {
room_id: "!NDbIqNpJyPvfKRnNcr:cadence.moe", room_id: "!NDbIqNpJyPvfKRnNcr:cadence.moe",
children_state: {}, children_state: [],
guest_can_join: false, guest_can_join: false,
num_joined_members: 2 num_joined_members: 2
} }

View file

@ -17,6 +17,8 @@ const {reg} = require("../src/matrix/read-registration")
reg.ooye.discord_token = "Njg0MjgwMTkyNTUzODQ0NzQ3.Xl3zlw.baby" reg.ooye.discord_token = "Njg0MjgwMTkyNTUzODQ0NzQ3.Xl3zlw.baby"
reg.ooye.server_origin = "https://matrix.cadence.moe" // so that tests will pass even when hard-coded reg.ooye.server_origin = "https://matrix.cadence.moe" // so that tests will pass even when hard-coded
reg.ooye.server_name = "cadence.moe" reg.ooye.server_name = "cadence.moe"
reg.ooye.namespace_prefix = "_ooye_"
reg.sender_localpart = "_ooye_bot"
reg.id = "baby" reg.id = "baby"
reg.as_token = "don't actually take authenticated actions on the server" reg.as_token = "don't actually take authenticated actions on the server"
reg.hs_token = "don't actually take authenticated actions on the server" reg.hs_token = "don't actually take authenticated actions on the server"