[rooms] fix owner and invite role decline

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<App>()
 				'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<App>()
 					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<App>()
 			if (!room) return roomEnvelope(c, null, 'This room does not exist!')
 
 			const body = (await c.req.parseBody().catch(() => ({}))) as Record<string, unknown>
-			const role = typeof body.role === 'string' ? Number.parseInt(body.role, 10) : Number.NaN
+			// A body naming NO role means "no role" — the owner-side removal below. A role that
-			if (Number.isNaN(role)) return roomEnvelope(c, null, 'You must provide a valid role!')
+			// 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.
+			// `role=0` on SOMEONE ELSE'S entry is the owner-side revoke: there is no "holds no
-			if (role === Role.None) return roomEnvelope(c, null, 'You must provide a valid role!')
+			// 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

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<Role[]> =>
+			(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')

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<Room | null> {
+	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