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
.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)
table.s-table.s-table__bx-simple
each row in linkedChannelsWithDetails
tr
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)
else
tr
@ -176,6 +176,18 @@ block body
!= icons.Icons.IconMerge
= ` 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
summary Debug room list
.d-grid.grid__2.gx24
@ -196,7 +208,7 @@ block body
ul.my8.ml24
each row in removedWrongTypeChannels
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
ul.my8.ml24
each row in removedPrivateChannels

View file

@ -129,6 +129,13 @@ html(lang="en")
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"))
//- Error dialog
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 {reg} = require("../../matrix/read-registration")
const me = `@${reg.sender_localpart}:${reg.ooye.server_name}`
/**
* @param {H3Event} event
* @returns {import("../../matrix/api")}
@ -39,6 +41,60 @@ function getCreateSpace(event) {
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 = {
linkSpace: z.object({
guild_id: z.string(),
@ -52,18 +108,20 @@ const schema = {
unlink: z.object({
guild_id: z.string(),
channel_id: z.string()
})
}),
unlinkSpace: z.object({
guild_id: z.string(),
}),
}
as.router.post("/api/link-space", defineEventHandler(async event => {
const parsedBody = await readValidatedBody(event, schema.linkSpace.parse)
const session = await auth.useSession(event)
const managed = await auth.getManagedGuilds(event)
const api = getAPI(event)
// Check 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
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
const me = `@${reg.sender_localpart}:${reg.ooye.server_name}`
/** @type {Ty.Event.M_Power_Levels?} */
let powerLevelsStateContent = null
try {
@ -112,18 +169,12 @@ as.router.post("/api/link-space", defineEventHandler(async event => {
as.router.post("/api/link", defineEventHandler(async event => {
const parsedBody = await readValidatedBody(event, schema.link.parse)
const managed = await auth.getManagedGuilds(event)
const api = getAPI(event)
const createRoom = getCreateRoom(event)
const createSpace = getCreateSpace(event)
// Check guild ID or nonce
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"})

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 guild = await validateGuildAccess(event, guildID)
const spaceID = await createSpace.ensureSpace(guild)
// Check channel exists
@ -201,33 +252,44 @@ as.router.post("/api/link", defineEventHandler(async event => {
as.router.post("/api/unlink", defineEventHandler(async event => {
const {channel_id, guild_id} = await readValidatedBody(event, schema.unlink.parse)
const managed = await auth.getManagedGuilds(event)
const createRoom = getCreateRoom(event)
await validateGuildAccess(event, guild_id)
// Check guild ID or nonce
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"})
// 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"})
// 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)
await doRoomUnlink(event, channel_id, guild_id)
setResponseHeader(event, "HX-Refresh", "true")
return null // 204
}))
as.router.post("/api/unlink-space", defineEventHandler(async event => {
const {guild_id} = await readValidatedBody(event, schema.unlinkSpace.parse)
const api = getAPI(event)
await validateGuildAccess(event, guild_id)
const spaceID = select("guild_space", "space_id", {guild_id: guild_id}).pluck().get()
if (!spaceID)
throw createError({status: 400, message: "Bad Request", data: "Matrix space does not exist or bot has not linked it"})
const linkedChannels = select("channel_room", ["channel_id", "room_id", "name", "nick"], {guild_id: guild_id}).all()
for (const channel of linkedChannels) {
await doRoomUnlink(event, channel.channel_id, guild_id)
}
const remainingLinkedChannels = select("channel_room", ["channel_id", "room_id", "name", "nick"], {guild_id: guild_id}).all()
if (remainingLinkedChannels.length !== 0)
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")
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 => {
const row = db.prepare("SELECT * FROM channel_room WHERE channel_id = '665310973967597573'").get()
db.prepare("DELETE FROM channel_room WHERE channel_id = '665310973967597573'").run()
const [error] = await tryToCatch(() => router.test("post", "/api/unlink", {
sessionData: {
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")
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)
})