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

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
pull from: Elliu/out-of-your-element:feat/add-unlink-space-button
merge into: cadence:main
Conversation 2 Commits 6 Files changed 4 +30 -16
1 changed files with 30 additions and 16 deletions
Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 3597a3b5ce - Show all commits

46
src/web/routes/link.js
View file

@ -39,6 +39,33 @@ 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
}
const schema = {
linkSpace: z.object({
guild_id: z.string(),
@ -58,12 +85,11 @@ const schema = {
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"})
@ -112,18 +138,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"})
cadence commented 2025-11-02 09:43:07 +00:00
Copy link

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
Elliu commented 2025-11-04 15:22:39 +00:00
Copy link

About the quoted code, I'm not sure why it doesn't appear on the quote, but I replaced it with a

	const guild = await validateGuildAccess(event, guildID)

so the error was still being detected I think

Anyway, I reverted it back to inline checks so it should be ok now

About the quoted code, I'm not sure why it doesn't appear on the quote, but I replaced it with a ```js const guild = await validateGuildAccess(event, guildID) ``` so the error was still being detected I think Anyway, I reverted it back to inline checks so it should be ok now
// 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,15 +221,9 @@ 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)
// 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"})
await validateGuildAccess(event, guild_id)
// Check that the channel (if it exists) is part of this guild
/** @type {any} */