feat/add-unlink-space-button #63

Open
Elliu wants to merge 6 commits from Elliu/out-of-your-element:feat/add-unlink-space-button into main
4 changed files with 268 additions and 40 deletions

View file

@ -129,13 +129,13 @@ block body
h3.mt32.fs-category Linked channels h3.mt32.fs-category Linked channels
.s-card.bs-sm.p0 .s-card.bs-sm.p0
form.s-table-container(method="post" action=rel("/api/unlink") hx-confirm="Do you want to unlink these channels?\nIt may take a moment to clean up Matrix resources.") form.s-table-container(method="post" action=rel("/api/unlink"))
input(type="hidden" name="guild_id" value=guild_id) input(type="hidden" name="guild_id" value=guild_id)
table.s-table.s-table__bx-simple table.s-table.s-table__bx-simple
each row in linkedChannelsWithDetails each row in linkedChannelsWithDetails
tr tr
td.w40: +discord(row.channel) td.w40: +discord(row.channel)
td.p2: button.s-btn.s-btn__muted.s-btn__xs(name="channel_id" value=row.channel.id hx-post=rel("/api/unlink") hx-trigger="click" hx-disabled-elt="this")!= icons.Icons.IconLinkSm td.p2: button.s-btn.s-btn__muted.s-btn__xs(name="channel_id" cx-prevent-default hx-post=rel("/api/unlink") hx-confirm="Do you want to unlink these channels?\nIt may take a moment to clean up Matrix resources." value=row.channel.id hx-indicator="this" hx-disabled-elt="this")!= icons.Icons.IconLinkSm
td: +matrix(row) td: +matrix(row)
else else
tr tr
@ -176,6 +176,18 @@ block body
!= icons.Icons.IconMerge != icons.Icons.IconMerge
= ` Link` = ` Link`
h3.mt32.fs-category Unlink server
form.s-card.d-flex.fd-row-reverse.gx24.pl24.ai-center(method="post" action=rel("/api/unlink-space"))
input(type="hidden" name="guild_id" value=guild.id)
.fl-grow1.s-prose.s-prose__sm.lh-xl
p.
Sick of this bridge, or just made a mistake? You can unlink the whole server and all its channels.#[br]
This may take a minute to process. Please be patient and wait until the page refreshes.
div
button.s-btn.s-btn__icon.s-btn__danger.s-btn__outlined(cx-prevent-default hx-post=rel("/api/unlink-space") hx-confirm="Do you want to unlink this server and all its channels?\nIt may take a minute to clean up Matrix resources." hx-indicator="this" hx-disabled-elt="this")
!= icons.Icons.IconUnsync
span.ml4= ` Unlink`
details.mt48 details.mt48
summary Debug room list summary Debug room list
.d-grid.grid__2.gx24 .d-grid.grid__2.gx24
@ -196,7 +208,7 @@ block body
ul.my8.ml24 ul.my8.ml24
each row in removedWrongTypeChannels each row in removedWrongTypeChannels
li: a(href=`https://discord.com/channels/${guild_id}/${row.id}`) (#{row.type}) #{row.name} li: a(href=`https://discord.com/channels/${guild_id}/${row.id}`) (#{row.type}) #{row.name}
h3.mt24 Unavailable channels: Bridge can't access h3.mt24 Unavailable channels: Discord bot can't access
.s-card.p0 .s-card.p0
ul.my8.ml24 ul.my8.ml24
each row in removedPrivateChannels each row in removedPrivateChannels

View file

@ -129,6 +129,13 @@ html(lang="en")
document.styleSheets[0].insertRule(t, document.styleSheets[0].cssRules.length) document.styleSheets[0].insertRule(t, document.styleSheets[0].cssRules.length)
}) })
}) })
//- Prevent default
script.
document.querySelectorAll("[cx-prevent-default]").forEach(e => {
e.addEventListener("click", event => {
event.preventDefault()
})
})
script(src=rel("/static/htmx.js")) script(src=rel("/static/htmx.js"))
//- Error dialog //- Error dialog
aside.s-modal#server-error(aria-hidden="true") aside.s-modal#server-error(aria-hidden="true")

View file

@ -12,6 +12,8 @@ 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")
const me = `@${reg.sender_localpart}:${reg.ooye.server_name}`
/** /**
* @param {H3Event} event * @param {H3Event} event
* @returns {import("../../matrix/api")} * @returns {import("../../matrix/api")}
@ -39,6 +41,60 @@ function getCreateSpace(event) {
return event.context.createSpace || sync.require("../../d2m/actions/create-space") return event.context.createSpace || sync.require("../../d2m/actions/create-space")
} }
/**
* @param {H3Event} event
* @param {string} guild_id
*/
async function validateUserHaveRightsOnGuild(event, guild_id) {
const managed = await auth.getManagedGuilds(event)
if (!managed.has(guild_id))
throw createError({status: 403, message: "Forbidden", data: "Can't edit a guild you don't have Manage Server permissions in"})
}
/**
* @param {H3Event} event
* @param {string} guild_id
* @returns {Promise<DiscordTypes.APIGuild & {members: DiscordTypes.APIGuildMember[]}>}
*/
async function validateGuildAccess(event, guild_id) {
// Check guild ID or nonce
await validateUserHaveRightsOnGuild(event, guild_id)
// Check guild exists
const guild = discord.guilds.get(guild_id)
if (!guild)
throw createError({status: 400, message: "Bad Request", data: "Discord guild does not exist or bot has not joined it"})
return guild
}
/**
* @param {H3Event} event
* @param {string} channel_id
* @param {string} guild_id
*/
async function doRoomUnlink(event, channel_id, guild_id) {
const createRoom = getCreateRoom(event)
// Check that the channel (if it exists) is part of this guild
/** @type {any} */
let channel = discord.channels.get(channel_id)
if (channel) {
if (!("guild_id" in channel) || channel.guild_id !== guild_id) throw createError({status: 400, message: "Bad Request", data: `Channel ID ${channel_id} is not part of guild ${guild_id}`})
} else {
// Otherwise, if the channel isn't cached, it must have been deleted.
// There's no other authentication here - it's okay for anyone to unlink a deleted channel just by knowing its ID.
channel = {id: channel_id}
}
// Check channel is currently bridged
const row = select("channel_room", "channel_id", {channel_id: channel_id}).get()
if (!row) throw createError({status: 400, message: "Bad Request", data: `Channel ID ${channel_id} is not currently bridged`})
// Do it
await createRoom.unbridgeDeletedChannel(channel, guild_id)
}
const schema = { const schema = {
linkSpace: z.object({ linkSpace: z.object({
guild_id: z.string(), guild_id: z.string(),
@ -52,18 +108,20 @@ const schema = {
unlink: z.object({ unlink: z.object({
guild_id: z.string(), guild_id: z.string(),
channel_id: z.string() channel_id: z.string()
}) }),
unlinkSpace: z.object({
guild_id: z.string(),
}),
} }
as.router.post("/api/link-space", defineEventHandler(async event => { as.router.post("/api/link-space", defineEventHandler(async event => {
const parsedBody = await readValidatedBody(event, schema.linkSpace.parse) const parsedBody = await readValidatedBody(event, schema.linkSpace.parse)
const session = await auth.useSession(event) const session = await auth.useSession(event)
const managed = await auth.getManagedGuilds(event)
const api = getAPI(event) const api = getAPI(event)
// Check guild ID // Check guild ID
const guildID = parsedBody.guild_id const guildID = parsedBody.guild_id
if (!managed.has(guildID)) throw createError({status: 403, message: "Forbidden", data: "Can't edit a guild you don't have Manage Server permissions in"}) await validateUserHaveRightsOnGuild(event, guildID)
// Check space ID // Check space ID
if (!session.data.mxid) throw createError({status: 403, message: "Forbidden", data: "Can't link with your Matrix space if you aren't logged in to Matrix"}) if (!session.data.mxid) throw createError({status: 403, message: "Forbidden", data: "Can't link with your Matrix space if you aren't logged in to Matrix"})
@ -87,7 +145,6 @@ as.router.post("/api/link-space", defineEventHandler(async event => {
} }
// Check bridge has PL 100 // Check bridge has PL 100
const me = `@${reg.sender_localpart}:${reg.ooye.server_name}`
/** @type {Ty.Event.M_Power_Levels?} */ /** @type {Ty.Event.M_Power_Levels?} */
let powerLevelsStateContent = null let powerLevelsStateContent = null
try { try {
@ -112,18 +169,12 @@ as.router.post("/api/link-space", defineEventHandler(async event => {
as.router.post("/api/link", defineEventHandler(async event => { as.router.post("/api/link", defineEventHandler(async event => {
const parsedBody = await readValidatedBody(event, schema.link.parse) const parsedBody = await readValidatedBody(event, schema.link.parse)
const managed = await auth.getManagedGuilds(event)
const api = getAPI(event) const api = getAPI(event)
const createRoom = getCreateRoom(event) const createRoom = getCreateRoom(event)
const createSpace = getCreateSpace(event) const createSpace = getCreateSpace(event)
// Check guild ID or nonce
const guildID = parsedBody.guild_id const guildID = parsedBody.guild_id
if (!managed.has(guildID)) throw createError({status: 403, message: "Forbidden", data: "Can't edit a guild you don't have Manage Server permissions in"}) const guild = await validateGuildAccess(event, guildID)

You deleted the thing that checks that the user has Manage Server permissions and you didn't put it back.

Not sure how this wasn't caught by tests. Maybe I need to add more tests.

I think I preferred how this looked before, where the checks were done inline, rather than how it looks now, where you've extracted each check to a function. Sure, there was some repetition of code before, but I think it was easier to read because you can just read down the file and see everything, rather than having to jump around looking in to each function.

If you keep the checked as extracted functions, I would like to see more descriptive function names + descriptions of what happens in the /** */ comments on the function so they can be hovered to see their intention. The current names are not so good:

  • validateUserHaveRightsOnGuild - what kind of validation outcome + what kind of user + what kind of rights? try ensureDiscordUserHasManageServer
  • validateGuildAccess - what kind of validation outcome + what kind of access + whose access? try coming up with a better name
You deleted the thing that checks that the user has Manage Server permissions and you didn't put it back. Not sure how this wasn't caught by tests. Maybe I need to add more tests. I think I preferred how this looked before, where the checks were done inline, rather than how it looks now, where you've extracted each check to a function. Sure, there was some repetition of code before, but I think it was easier to read because you can just read down the file and see everything, rather than having to jump around looking in to each function. If you keep the checked as extracted functions, I would like to see more descriptive function names + descriptions of what happens in the `/** */` comments on the function so they can be hovered to see their intention. The current names are not so good: - `validateUserHaveRightsOnGuild` - what kind of validation outcome + what kind of user + what kind of rights? try `ensureDiscordUserHasManageServer` - `validateGuildAccess` - what kind of validation outcome + what kind of access + whose access? try coming up with a better name
// Check guild is bridged
const guild = discord.guilds.get(guildID)
if (!guild) throw createError({status: 400, message: "Bad Request", data: "Discord guild does not exist or bot has not joined it"})
const spaceID = await createSpace.ensureSpace(guild) const spaceID = await createSpace.ensureSpace(guild)
// Check channel exists // Check channel exists
@ -201,33 +252,44 @@ as.router.post("/api/link", defineEventHandler(async event => {
as.router.post("/api/unlink", defineEventHandler(async event => { as.router.post("/api/unlink", defineEventHandler(async event => {
const {channel_id, guild_id} = await readValidatedBody(event, schema.unlink.parse) const {channel_id, guild_id} = await readValidatedBody(event, schema.unlink.parse)
const managed = await auth.getManagedGuilds(event) await validateGuildAccess(event, guild_id)
const createRoom = getCreateRoom(event)
// Check guild ID or nonce await doRoomUnlink(event, channel_id, guild_id)
if (!managed.has(guild_id)) throw createError({status: 403, message: "Forbidden", data: "Can't edit a guild you don't have Manage Server permissions in"})
setResponseHeader(event, "HX-Refresh", "true")
// Check guild exists return null // 204
const guild = discord.guilds.get(guild_id) }))
if (!guild) throw createError({status: 400, message: "Bad Request", data: "Discord guild does not exist or bot has not joined it"})
as.router.post("/api/unlink-space", defineEventHandler(async event => {
// Check that the channel (if it exists) is part of this guild const {guild_id} = await readValidatedBody(event, schema.unlinkSpace.parse)
/** @type {any} */ const api = getAPI(event)
let channel = discord.channels.get(channel_id) await validateGuildAccess(event, guild_id)
if (channel) {
if (!("guild_id" in channel) || channel.guild_id !== guild_id) throw createError({status: 400, message: "Bad Request", data: `Channel ID ${channel_id} is not part of guild ${guild_id}`}) const spaceID = select("guild_space", "space_id", {guild_id: guild_id}).pluck().get()
} else { if (!spaceID)
// Otherwise, if the channel isn't cached, it must have been deleted. throw createError({status: 400, message: "Bad Request", data: "Matrix space does not exist or bot has not linked it"})
// There's no other authentication here - it's okay for anyone to unlink a deleted channel just by knowing its ID.
channel = {id: channel_id} const linkedChannels = select("channel_room", ["channel_id", "room_id", "name", "nick"], {guild_id: guild_id}).all()
}
for (const channel of linkedChannels) {
// Check channel is currently bridged await doRoomUnlink(event, channel.channel_id, guild_id)
const row = select("channel_room", "channel_id", {channel_id: channel_id}).get() }
if (!row) throw createError({status: 400, message: "Bad Request", data: `Channel ID ${channel_id} is not currently bridged`})
const remainingLinkedChannels = select("channel_room", ["channel_id", "room_id", "name", "nick"], {guild_id: guild_id}).all()
// Do it if (remainingLinkedChannels.length !== 0)
await createRoom.unbridgeDeletedChannel(channel, guild_id) throw createError({status: 500, message: "Internal Server Error", data: "Some linked room still exists after trying to unlink all of them. Aborting the space unlinking..."})
await api.setUserPower(spaceID, me, 0)
await api.leaveRoom(spaceID)
db.prepare("DELETE FROM guild_space WHERE guild_id=? AND space_id=?").run(guild_id, spaceID)
// NOTE: not deleting from guild_active as this can lead to inconsistent state:
// if we only delete from DB, the guild is still displayed on the top-right dropdown,
// but when selected we get the "Please add the bot to your server using the buttons on the home page." page
//
// So either keep as-is, or delete from guild_active, but also leave the discord guild? Not sure if we want that or not
// db.prepare("DELETE FROM guild_active WHERE guild_id=?").run(guild_id)
setResponseHeader(event, "HX-Refresh", "true") setResponseHeader(event, "HX-Refresh", "true")
return null // 204 return null // 204

View file

@ -666,7 +666,9 @@ test("web unlink room: successfully calls unbridgeDeletedChannel when the channe
}) })
test("web unlink room: checks that the channel is bridged", async t => { test("web unlink room: checks that the channel is bridged", async t => {
const row = db.prepare("SELECT * FROM channel_room WHERE channel_id = '665310973967597573'").get()
db.prepare("DELETE FROM channel_room WHERE channel_id = '665310973967597573'").run() db.prepare("DELETE FROM channel_room WHERE channel_id = '665310973967597573'").run()
const [error] = await tryToCatch(() => router.test("post", "/api/unlink", { const [error] = await tryToCatch(() => router.test("post", "/api/unlink", {
sessionData: { sessionData: {
managedGuilds: ["665289423482519565"] managedGuilds: ["665289423482519565"]
@ -677,4 +679,149 @@ test("web unlink room: checks that the channel is bridged", async t => {
} }
})) }))
t.equal(error.data, "Channel ID 665310973967597573 is not currently bridged") t.equal(error.data, "Channel ID 665310973967597573 is not currently bridged")
db.prepare("INSERT INTO channel_room (channel_id, room_id, name, nick, thread_parent, custom_avatar, last_bridged_pin_timestamp, speedbump_id, speedbump_checked, speedbump_webhook_id, guild_id, custom_topic) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)").run(row.channel_id, row.room_id, row.name, row.nick, row.thread_parent, row.custom_avatar, row.last_bridged_pin_timestamp, row.speedbump_id, row.speedbump_checked, row.speedbump_webhook_id, row.guild_id, row.custom_topic)

What is the purpose of this database operation?

What is the purpose of this database operation?
const new_row = db.prepare("SELECT * FROM channel_room WHERE channel_id = '665310973967597573'").get()
t.deepEqual(row, new_row)
})
// *****
test("web unlink space: access denied if not logged in to Discord", async t => {
const [error] = await tryToCatch(() => router.test("post", "/api/unlink-space", {
body: {
guild_id: "665289423482519565"
}
}))
t.equal(error.data, "Can't edit a guild you don't have Manage Server permissions in")
})
test("web unlink space: checks that guild exists", async t => {
const [error] = await tryToCatch(() => router.test("post", "/api/unlink-space", {
sessionData: {
managedGuilds: ["2"]
},
body: {
guild_id: "2"
}
}))
t.equal(error.data, "Discord guild does not exist or bot has not joined it")
})
test("web unlink space: checks that a space is linked to the guild before trying to unlink the space", async t => {
const row = db.prepare("SELECT * FROM guild_space WHERE guild_id = '665289423482519565'").get()
db.prepare("DELETE FROM guild_space WHERE guild_id = '665289423482519565'").run()
const [error] = await tryToCatch(() => router.test("post", "/api/unlink-space", {
sessionData: {
managedGuilds: ["665289423482519565"]
},
body: {
guild_id: "665289423482519565"
}
}))
t.equal(error.data, "Matrix space does not exist or bot has not linked it")
db.prepare("INSERT INTO guild_space (guild_id, space_id, privacy_level, presence, url_preview) VALUES (?, ?, ?, ?, ?)").run(row.guild_id, row.space_id, row.privacy_level, row.presence, row.url_preview)
const new_row = db.prepare("SELECT * FROM guild_space WHERE guild_id = '665289423482519565'").get()
t.deepEqual(row, new_row)
})
test("web unlink space: correctly abort unlinking if some linked channels remain after trying to unlink them all", async t => {
let unbridgedChannel = false
const [error] = await tryToCatch(() => router.test("post", "/api/unlink-space", {
sessionData: {
managedGuilds: ["665289423482519565"]
},
body: {
guild_id: "665289423482519565",
},
createRoom: {
async unbridgeDeletedChannel(channel, guildID) {
unbridgedChannel = true
t.equal(channel.id, "665310973967597573")
t.equal(guildID, "665289423482519565")
// Do not actually delete the link from DB, should trigger error later in check
}
},
api: {
async *generateFullHierarchy(spaceID) {
t.equal(spaceID, "!zTMspHVUBhFLLSdmnS:cadence.moe")
yield {
room_id: "!NDbIqNpJyPvfKRnNcr:cadence.moe",
children_state: [],
guest_can_join: false,
num_joined_members: 2
}
/* c8 ignore next */
},
}
}))
t.equal(error.data, "Some linked room still exists after trying to unlink all of them. Aborting the space unlinking...")
t.equal(unbridgedChannel, true)
})
test("web unlink space: successfully calls unbridgeDeletedChannel on linked channels in space, self-downgrade power level, leave space, and delete link from DB", async t => {
const {reg} = require("../../matrix/read-registration")
const me = `@${reg.sender_localpart}:${reg.ooye.server_name}`
const getLinkRowQuery = "SELECT * FROM guild_space WHERE guild_id = '665289423482519565'"
const row = db.prepare(getLinkRowQuery).get()
t.equal(row.space_id, "!zTMspHVUBhFLLSdmnS:cadence.moe")
let unbridgedChannel = false
let downgradedPowerLevel = false
let leftRoom = false
await router.test("post", "/api/unlink-space", {
sessionData: {
managedGuilds: ["665289423482519565"]
},
body: {
guild_id: "665289423482519565",
},
createRoom: {
async unbridgeDeletedChannel(channel, guildID) {
unbridgedChannel = true
t.equal(channel.id, "665310973967597573")
t.equal(guildID, "665289423482519565")
// In order to not simulate channel deletion and not trigger the post unlink channels, pre-unlink space check
db.prepare("DELETE FROM channel_room WHERE guild_id = '665289423482519565' AND channel_id = '665310973967597573'").run()
}
},
api: {
async *generateFullHierarchy(spaceID) {
t.equal(spaceID, "!zTMspHVUBhFLLSdmnS:cadence.moe")
yield {
room_id: "!NDbIqNpJyPvfKRnNcr:cadence.moe",
children_state: [],
guest_can_join: false,
num_joined_members: 2
}
/* c8 ignore next */
},
async setUserPower(spaceID, targetUser, powerLevel) {
downgradedPowerLevel = true
t.equal(spaceID, "!zTMspHVUBhFLLSdmnS:cadence.moe")
t.equal(targetUser, me)
t.equal(powerLevel, 0)
},
async leaveRoom(spaceID) {
leftRoom = true
t.equal(spaceID, "!zTMspHVUBhFLLSdmnS:cadence.moe")
},
}
})
t.equal(unbridgedChannel, true)
t.equal(downgradedPowerLevel, true)
t.equal(leftRoom, true)
const missed_row = db.prepare(getLinkRowQuery).get()
t.equal(missed_row, undefined)
}) })