From 81df863aa42690e22c583e75e4681a201ab26107 Mon Sep 17 00:00:00 2001 From: Devin Zuczek Date: Wed, 16 Sep 2026 13:51:21 -0400 Subject: [PATCH] [rooms] fix owner and invite role decline --- apps/rooms/src/rooms.app.ts | 48 ++++++- apps/rooms/src/test/integration/api.test.ts | 142 +++++++++++++++++++- packages/domain/src/rooms-db.ts | 57 ++++++++ 3 files changed, 235 insertions(+), 12 deletions(-) diff --git a/apps/rooms/src/rooms.app.ts b/apps/rooms/src/rooms.app.ts index 180fd5f..b9ddf73 100644 --- a/apps/rooms/src/rooms.app.ts +++ b/apps/rooms/src/rooms.app.ts @@ -50,6 +50,7 @@ import { publishSubRoomSave, removeCheer, removeFavorite, + removeRoomRole, Role, roomNameRejection, roomRoles, @@ -2108,6 +2109,15 @@ const app = new Hono() 'that needs an invite, so it cannot be the one grant that skips the ceremony.', 'Any pending invite on the entry is left standing.', '', + '**The same caller, REVOKING a role** — `role=0`, or a body naming no role at all.', + 'There is no "holds no role" tier, so the target’s entry is removed outright, taking', + 'any pending invite on it with it. Two entries are protected and refused: the room’s', + 'owner (its creator, or a Creator entry), whose ownership moves only by transfer; and', + 'a co-owner, unless the caller is the room’s owner — co-owners are peers and cannot', + 'revoke each other. Removing a role the player does not hold succeeds and changes', + 'nothing. A role that is present but not a number is still a rejection, never read as', + 'a removal.', + '', '**The invited player, answering their own co-owner invite.** Accepting promotes', 'their entry from its pending `InvitedRole` to the real `Role` and clears the', 'pending one — an accepted entry reads `Role: 30, InvitedRole: 0`, so an invite', @@ -2134,7 +2144,10 @@ const app = new Hono() schema: { type: 'string', pattern: '^[0-9]+$' }, }, ], - requestBody: form(RoleRequest, 'The role tier, or 0 to decline an invite'), + requestBody: form( + RoleRequest, + 'The role tier; 0 or absent to revoke, or 0 to decline your own invite' + ), responses: { 200: json(RoomEnvelope, 'The updated room, or a rejection with `success: false`'), 401: UNAUTHORIZED_RESPONSE, @@ -2151,24 +2164,45 @@ const app = new Hono() if (!room) return roomEnvelope(c, null, 'This room does not exist!') const body = (await c.req.parseBody().catch(() => ({}))) as Record - const role = typeof body.role === 'string' ? Number.parseInt(body.role, 10) : Number.NaN - if (Number.isNaN(role)) return roomEnvelope(c, null, 'You must provide a valid role!') + // A body naming NO role means "no role" — the owner-side removal below. A role that + // is present but not a number is still malformed, so the two are kept apart: the + // absent case must not swallow a typo'd tier and silently strip someone. + const rawRole = typeof body.role === 'string' ? body.role.trim() : '' + const namesRole = rawRole !== '' + const role = namesRole ? Number.parseInt(rawRole, 10) : Role.None + if (namesRole && Number.isNaN(role)) { + return roomEnvelope(c, null, 'You must provide a valid role!') + } // Acting on your OWN entry is answering an invite — the only thing a player with // no standing in the room may do here, and the only way a co-owner role is ever // taken. An owner targeting themselves lands here too, and has no invite to answer. if (targetAccountId === accountId) { + // Answering an invite still has to NAME the answer: accepting names the tier offered + // and declining is an explicit `role=0`. An empty body is a removal, and removing + // your own entry is not what this branch does. + if (!namesRole) return roomEnvelope(c, null, 'You must provide a valid role!') const answered = await answerRoomRoleInvite(c.env.DB, roomId, accountId, role, room) if (!answered) return roomEnvelope(c, null, 'You have no such invite to this room!') await pushRoomUpdateToRoom(c, roomId, answered) return roomEnvelope(c, answered) } - // Otherwise it is a grant, and the room's owner/co-owner gate applies. + // Otherwise it is a grant or a REVOKE, and the room's owner/co-owner gate applies. if (!canManageRoom(room, accountId)) return c.body(null, 403) - // `role=0` is the invited player's decline, not an owner-side revoke — there is no - // revoke yet, and letting one in here would let a co-owner strip the other. - if (role === Role.None) return roomEnvelope(c, null, 'You must provide a valid role!') + + // `role=0` on SOMEONE ELSE'S entry is the owner-side revoke: there is no "holds no + // role" tier, so removing the role means removing the record. (The same value on your + // own entry is a decline, handled above — whose entry it is, is what tells them + // apart.) `removeRoomRole` owns the two protections: ownership is never revoked this + // way, and only the room's owner may revoke a co-owner, so co-owners cannot strip + // each other. + if (role === Role.None) { + const removed = await removeRoomRole(c.env.DB, roomId, targetAccountId, accountId, room) + if (!removed) return roomEnvelope(c, null, 'You cannot remove that role!') + await pushRoomUpdateToRoom(c, roomId, removed, [targetAccountId]) + return roomEnvelope(c, removed) + } // Null means the tier has to be ACCEPTED rather than handed over: nobody force-adds // a co-owner. `setRoomRole` refuses it itself — the rule belongs to the room's diff --git a/apps/rooms/src/test/integration/api.test.ts b/apps/rooms/src/test/integration/api.test.ts index 269438f..4858d2d 100644 --- a/apps/rooms/src/test/integration/api.test.ts +++ b/apps/rooms/src/test/integration/api.test.ts @@ -1676,11 +1676,6 @@ describe('rooms endpoints', () => { error: 'Co-ownership must be invited, not granted!', }) } - // `role=0` is the invited player's decline, not an owner-side revoke. - expect(await envOf(await putForm('/rooms/2/roles/860', { role: '0' }, '1'))).toMatchObject({ - success: false, - }) - expect(await roleOf(860)).toMatchObject({ Role: 10 }) // A grant leaves a pending co-owner invite standing — the two are answered separately. expect((await putForm('/rooms/2/roles/861/invite', { role: '30' }, '1')).status).toBe(200) @@ -1688,6 +1683,143 @@ describe('rooms endpoints', () => { expect(await roleOf(861)).toMatchObject({ Role: 20, InvitedRole: 30 }) }) + it('PUT /rooms/:id/roles/:accountId with role=0 from an owner REVOKES the role', async () => { + type Role = { AccountId: number; Role: number; InvitedRole: number } + const rolesOf = async (): Promise => + (await (await SELF.fetch(`${ORIGIN}/rooms/2/roles`)).json()) as Role[] + const roleOf = async (accountId: number) => + (await rolesOf()).find((r) => r.AccountId === accountId) + + // RecCenter (room 2) is owned by account 1, with account 2 as co-owner. + expect((await putForm('/rooms/2/roles/880', { role: '10' }, '1')).status).toBe(200) + expect(await roleOf(880)).toMatchObject({ Role: 10 }) + + // The owner removes the role. The RECORD goes — there is no "holds no role" tier — and + // the envelope carries the whole updated room, which is what the client re-renders from. + const revoked = await putForm('/rooms/2/roles/880', { role: '0' }, '1') + expect(revoked.status).toBe(200) + const body = (await envOf(revoked)) as { + success: boolean + error: string + value: { Roles: Role[] } + } + expect(body).toMatchObject({ success: true, error: '' }) + expect(body.value.Roles.some((r) => r.AccountId === 880)).toBe(false) + expect(await roleOf(880)).toBeUndefined() + + // A body naming NO role is the same removal. + expect((await putForm('/rooms/2/roles/881', { role: '20' }, '1')).status).toBe(200) + expect(await envOf(await putForm('/rooms/2/roles/881', {}, '1'))).toMatchObject({ + success: true, + }) + expect(await roleOf(881)).toBeUndefined() + + // ...but a role that is PRESENT and unparseable is still malformed, and must not be read + // as a removal — a typo'd tier silently stripping someone would be the worst outcome. + expect((await putForm('/rooms/2/roles/881', { role: '20' }, '1')).status).toBe(200) + expect(await envOf(await putForm('/rooms/2/roles/881', { role: 'nope' }, '1'))).toMatchObject({ + success: false, + error: 'You must provide a valid role!', + }) + expect(await roleOf(881)).toMatchObject({ Role: 20 }) + + // A co-owner may revoke a helper tier. + expect(await envOf(await putForm('/rooms/2/roles/881', { role: '0' }, '2'))).toMatchObject({ + success: true, + }) + expect(await roleOf(881)).toBeUndefined() + + // Removing a role the player does not hold is not a failure: they asked for no role, and + // there is none. Nothing changes. + const before = await rolesOf() + expect(await envOf(await putForm('/rooms/2/roles/889', { role: '0' }, '1'))).toMatchObject({ + success: true, + }) + expect(await rolesOf()).toEqual(before) + + // The gate still applies: no token 401s, and no standing in the room 403s. + expect((await putForm('/rooms/2/roles/882', { role: '0' })).status).toBe(401) + expect((await putForm('/rooms/2/roles/882', { role: '0' }, '999')).status).toBe(403) + }) + + it('a role revoke cannot touch the owner, and co-owners cannot revoke each other', async () => { + type Role = { AccountId: number; Role: number; InvitedRole: number } + const roleOf = async (accountId: number) => + ((await (await SELF.fetch(`${ORIGIN}/rooms/2/roles`)).json()) as Role[]).find( + (r) => r.AccountId === accountId + ) + const refused = { success: false, error: 'You cannot remove that role!' } + + // Ownership is not a role anyone beside it can take away — it moves only by transfer. + // Account 1 is RecCenter's creator; its co-owner cannot remove it. + expect(await envOf(await putForm('/rooms/2/roles/1', { role: '0' }, '2'))).toMatchObject( + refused + ) + + // Make 883 a second co-owner the only legal way: invited, then accepted. + expect((await putForm('/rooms/2/roles/883/invite', { role: '30' }, '1')).status).toBe(200) + expect((await putForm('/rooms/2/roles/883', { role: '30' }, '883')).status).toBe(200) + expect(await roleOf(883)).toMatchObject({ Role: 30 }) + + // Co-owners are peers, so one cannot strip the other — whichever asked first would win. + expect(await envOf(await putForm('/rooms/2/roles/883', { role: '0' }, '2'))).toMatchObject( + refused + ) + expect(await envOf(await putForm('/rooms/2/roles/2', { role: '0' }, '883'))).toMatchObject( + refused + ) + expect(await roleOf(883)).toMatchObject({ Role: 30 }) + + // The room's owner is not their peer, and may. + expect(await envOf(await putForm('/rooms/2/roles/883', { role: '0' }, '1'))).toMatchObject({ + success: true, + }) + expect(await roleOf(883)).toBeUndefined() + }) + + it('a role revoke takes a standing invite with it, and never removes your own entry', async () => { + type Role = { AccountId: number; Role: number; InvitedRole: number } + const roleOf = async (accountId: number) => + ((await (await SELF.fetch(`${ORIGIN}/rooms/2/roles`)).json()) as Role[]).find( + (r) => r.AccountId === accountId + ) + + // The whole entry goes, so a pending co-owner invite on it is withdrawn too. + expect((await putForm('/rooms/2/roles/884/invite', { role: '30' }, '1')).status).toBe(200) + expect((await putForm('/rooms/2/roles/884', { role: '10' }, '1')).status).toBe(200) + expect(await roleOf(884)).toMatchObject({ Role: 10, InvitedRole: 30 }) + expect((await putForm('/rooms/2/roles/884', { role: '0' }, '1')).status).toBe(200) + expect(await roleOf(884)).toBeUndefined() + + // On your OWN entry the path means answering an invite, which has to name its answer. An + // empty body is a removal, and removing yourself is not what that branch does. + expect((await putForm('/rooms/2/roles/885/invite', { role: '30' }, '1')).status).toBe(200) + expect(await envOf(await putForm('/rooms/2/roles/885', {}, '885'))).toMatchObject({ + success: false, + error: 'You must provide a valid role!', + }) + expect(await roleOf(885)).toMatchObject({ InvitedRole: 30 }) + }) + + it('a revoke pushes a RoomUpdate to the room and to the player who lost the role', async () => { + type Sent = { playerId: number; notificationType: string | number; data: { RoomId: number } } + const hub = () => env.RECFLARE_NOTIFICATIONS_HUB.getByName('global') + + expect((await putForm('/rooms/4/roles/887', { role: '20' }, '1')).status).toBe(200) + // A bystander in the room, and the player losing the role standing somewhere else. + await putInRoom(886, 4) + await putInRoom(887, 2) + await hub().fetch('http://do/all', { method: 'DELETE' }) + + expect((await putForm('/rooms/4/roles/887', { role: '0' }, '1')).status).toBe(200) + + const pushed = (await (await hub().fetch('http://do/all')).json()) as Sent[] + expect(pushed.map((n) => n.playerId).sort((a, b) => a - b)).toEqual([886, 887]) + expect(pushed.every((n) => n.data.RoomId === 4)).toBe(true) + + for (const id of [886, 887]) await clearPresence(id) + }) + it('a grant pushes a RoomUpdate to the room and to the affected player', async () => { type Sent = { playerId: number; notificationType: string | number; data: { RoomId: number } } const hub = () => env.RECFLARE_NOTIFICATIONS_HUB.getByName('global') diff --git a/packages/domain/src/rooms-db.ts b/packages/domain/src/rooms-db.ts index 253fb49..23d8196 100644 --- a/packages/domain/src/rooms-db.ts +++ b/packages/domain/src/rooms-db.ts @@ -632,6 +632,63 @@ export async function setRoomRole( return updated } +/** + * REVOKE a room role — dropping the target's `Roles` entry outright. This is how a room's + * owner or a co-owner takes back a role they handed out, and it is the counterpart to + * {@link setRoomRole}: there is no "demote to nothing" tier, because {@link Role.None} is the + * resting value of a PENDING invite rather than a role somebody holds. An entry that confers + * nothing should not exist, so a revoke removes the record. + * + * Two entries are protected, and both refuse by returning null without writing: + * + * - The room's OWNER — `CreatorAccountId`, or a {@link Role.Creator} entry. Ownership is not + * a role to be taken away by anyone standing beside it; it moves through + * {@link transferRoomOwnership} and nowhere else. Without this a co-owner could remove the + * creator's entry, and on an older room whose creator has no entry at all they could not — + * so the check is on `CreatorAccountId` as well as on the tier. + * - A CO-OWNER entry, unless the caller is the room's owner. Co-owners are peers: letting one + * remove another means whichever of them asks first wins, and the room's owner is the only + * person who is not a peer of theirs. A co-owner may still revoke the helper tiers. + * + * Removing a role a player does not have is NOT a failure — the room comes back unchanged and + * nothing is written. The caller asked for this player to hold no role, and they hold none; + * answering an error would make a client retrying a removal look broken. + * + * Takes any pending `InvitedRole` with it, since the whole entry goes. That is the same + * trade {@link answerRoomRoleInvite} makes when declining, and it means revoking a role from + * someone who has a co-owner invite standing also withdraws the invite. + * + * `removedByAccountId` is not stamped anywhere — the entry it would be stamped on is the one + * being deleted — and is read only to decide which of the two protections apply. The caller + * supplies the already-loaded room (after its gate) to avoid a re-read; the whole room JSON + * is rewritten. + */ +export async function removeRoomRole( + db: D1Database, + roomId: number, + targetAccountId: number, + removedByAccountId: number, + room: Room +): Promise { + if (room.CreatorAccountId === targetAccountId) return null + + const roles = roomRoles(room) + const index = roles.findIndex((r) => r.AccountId === targetAccountId) + if (index === -1) return room + + const entry = roles[index] + if (entry.Role === Role.Creator) return null + if (entry.Role === Role.CoOwner && !isRoomOwner(room, removedByAccountId)) return null + + roles.splice(index, 1) + const updated: Room = { ...room, Roles: roles } + await db + .prepare('UPDATE room SET data = ?2 WHERE room_id = ?1') + .bind(roomId, serializeRoom(updated)) + .run() + return updated +} + /** * TRANSFER a room to a new owner: the recipient becomes {@link Role.Creator} and the * outgoing owner is left as {@link Role.CoOwner}, keeping their access to a room they built