From 7dd88a01989c661a8112fce7b753c557d5d69eb4 Mon Sep 17 00:00:00 2001 From: Devin Zuczek Date: Tue, 6 Oct 2026 10:48:22 -0400 Subject: [PATCH] [rooms] fix get subroom save endpoints for co-owners --- CLAUDE.md | 13 +++---- apps/rooms/src/rooms.app.ts | 39 ++++++++++++--------- apps/rooms/src/test/integration/api.test.ts | 16 ++++----- 3 files changed, 37 insertions(+), 31 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index d777737..b249b32 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -118,12 +118,13 @@ inconsistency here without checking the client first. `GET …/saves/:saveId` is the detail behind a list row, under the same gate, but in the CAMELCASE projection the room save's response uses — not the PascalCase rows the list serves. Three shapes of one save; keep them straight. -- Both save reads (`rooms`: `…/saves` and `…/saves/:saveId`) are auth-gated and readable by - the room's CREATOR or by anyone whose live `presence` row puts them in that room — not by - co-owners as such (a co-owner passes only by standing there). They list unpublished - staged saves, so they aren't public; but a visitor resolves which version an instance is - running from this list, so creator-only locks them out of loading the room. The grant - expires with the presence row. +- The save reads (`rooms`: `…/saves`, `…/saves/no_unity_assets` and `…/saves/:saveId`) are + auth-gated and readable by the room's creator, a CO-OWNER, or anyone whose live `presence` + row puts them in that room. They list unpublished staged saves, so they aren't public; but + a co-owner opens the save history from the room's settings without being in the room + (creator-only showed them the buttons and a list that 403'd), and a visitor resolves which + version an instance is running from this list, so managers-only locks them out of loading + the room. The presence grant expires with the presence row. - A room save writes ONLY to the subroom and its save row — never to the room. Everything the body carries describes that one revision: `Description` is the save comment shown in `…/saves`, and `PersistenceVersion`/`InventionUsage` describe the scene just saved (the diff --git a/apps/rooms/src/rooms.app.ts b/apps/rooms/src/rooms.app.ts index 1108e4b..e856c8a 100644 --- a/apps/rooms/src/rooms.app.ts +++ b/apps/rooms/src/rooms.app.ts @@ -338,18 +338,22 @@ async function handlePhotonAccessToken(c: Context) { } /** - * May this caller read the room's saves? The room's creator always may. So may anyone - * whose live presence puts them IN the room: they are already loading its scene, and the - * client resolves which version to load — the published one or the creator's latest — from - * the save list, so refusing everyone but the creator leaves a visitor unable to load what - * the instance is actually running. + * May this caller read the room's saves? The room's creator and its co-owners always may: + * they are the ones who write and publish saves (`canManageRoom` gates both), and the + * client shows them the save history from the room's settings, wherever they are — a + * co-owner refused here sees the buttons and a list that never loads. * - * Presence is the shared `presence` table the `match` heartbeat maintains, so this grant + * So may anyone whose live presence puts them IN the room: they are already loading its + * scene, and the client resolves which version to load — the published one or the latest — + * from the save list, so refusing everyone but the room's managers leaves a visitor unable + * to load what the instance is actually running. + * + * Presence is the shared `presence` table the `match` heartbeat maintains, so that grant * lasts only as long as the player is actually there (rows carry an absolute expiry and - * expired ones don't read back). Co-owners get nothing extra from being co-owners — a - * co-owner standing in the room passes because of where they are, not what they hold. + * expired ones don't read back). * - * The presence read only happens for a non-creator, so the owner's own path stays one query. + * The presence read only happens for someone who doesn't manage the room, so the owner's + * and a co-owner's path stays one query. */ async function canReadSaves( c: Context, @@ -357,7 +361,7 @@ async function canReadSaves( roomId: number, accountId: number ): Promise { - if (room.CreatorAccountId === accountId) return true + if (canManageRoom(room, accountId)) return true const instance = (await getPresence(c.env.DB, accountId))?.roomInstance return instance?.roomId === roomId } @@ -3437,7 +3441,8 @@ const app = new Hono() // A subroom's saved-data versions — the room-history / "restore a save" list. Every // save is its own `subroom_save` row (nothing is overwritten), so this is real // history, newest first, paged by skip/take. Auth-gated (401), and readable by the - // room's creator or anyone whose presence puts them in the room (see `canReadSaves`). + // room's creator, a co-owner, or anyone whose presence puts them in the room (see + // `canReadSaves`). .get( '/rooms/:roomId{[0-9]+}/subrooms/:subRoomId{[0-9]+}/saves', describeRoute({ @@ -3451,8 +3456,8 @@ const app = new Hono() '(0 Windows, 2 Android/Quest). `unityAssetVersion` is accepted and ignored.', '', 'The list includes STAGED saves that were never published, so it is not public:', - 'the room’s creator may read it, and so may anyone standing IN the room (their live', - 'presence says so). Anyone else is a 403. It is what the client reads to resolve', + 'the room’s creator or a co-owner may read it, and so may anyone standing IN the room', + '(their live presence says so). Anyone else is a 403. It is what the client reads to resolve', '“load the latest or the published version?” on entering a private instance — a', 'visitor who cannot read it cannot load what the instance is running.', '', @@ -3525,8 +3530,8 @@ const app = new Hono() 'paged wrapper, with no `{ success, error, value }` envelope around it.', '', 'Gated exactly like `…/saves`, and for the same reason: the list includes STAGED', - 'saves that were never published, so it is the room’s creator or anyone whose live', - 'presence puts them in the room, and anyone else is a 403.', + 'saves that were never published, so it is the room’s creator, a co-owner, or anyone', + 'whose live presence puts them in the room, and anyone else is a 403.', '', '`TotalResults` and `TotalCount` carry the same number — the client’s paged DTO and', 'the reference disagree on the name, so both are emitted.', @@ -3588,8 +3593,8 @@ const app = new Hono() 'save by guessing an id: a save that belongs elsewhere is a 404, same as an unknown', 'one.', '', - 'Gated like the list it details — the room’s creator, or anyone whose presence puts', - 'them in the room. A save id resolves whether or not it was ever published, so this', + 'Gated like the list it details — the room’s creator, a co-owner, or anyone whose', + 'presence puts them in the room. A save id resolves whether or not it was ever published, so this', 'reads unpublished work.', ].join(' '), security: AUTHED, diff --git a/apps/rooms/src/test/integration/api.test.ts b/apps/rooms/src/test/integration/api.test.ts index 1cae149..c8ad8a8 100644 --- a/apps/rooms/src/test/integration/api.test.ts +++ b/apps/rooms/src/test/integration/api.test.ts @@ -5306,18 +5306,18 @@ describe('rooms endpoints', () => { expect(await empty.json()).toEqual({ Results: [], TotalResults: 0, TotalCount: 0 }) // The list exposes unpublished saves, so it isn't public: no token → 401, and a - // valid token from someone who is neither the creator nor in the room → 403. Account - // 2 is a co-owner (Role 30 on the seeded rooms) and is refused too — holding a role - // grants nothing here; being in the room does (see below). + // valid token from someone who neither manages the room nor is in it → 403. expect((await SELF.fetch(`${ORIGIN}/rooms/2/subrooms/2/saves`)).status).toBe(401) expect( (await SELF.fetch(`${ORIGIN}/rooms/2/subrooms/2/saves`, { headers: await bearer('999') })) .status ).toBe(403) + // Account 2 is a co-owner (Role 30 on the seeded rooms) and reads it from anywhere: + // the client opens the save history from the room's settings, not from inside it. expect( (await SELF.fetch(`${ORIGIN}/rooms/2/subrooms/2/saves`, { headers: await bearer('2') })) .status - ).toBe(403) + ).toBe(200) // …but a player standing IN the room reads it: the client resolves which version to // load from this list, so a visitor who can't read it can't load the instance. @@ -5409,7 +5409,7 @@ describe('rooms endpoints', () => { // Same gate as the list it mirrors — it exposes the same unpublished saves. expect((await get(light)).status).toBe(401) expect((await get(light, '999')).status).toBe(403) - expect((await get(light, '2')).status).toBe(403) + expect((await get(light, '2')).status).toBe(200) await putInRoom(999, 2) expect((await get(light, '999')).status).toBe(200) await clearPresence(999) @@ -5460,12 +5460,12 @@ describe('rooms endpoints', () => { expect((await get('/rooms/99999/subrooms/2/saves/1', '1')).status).toBe(404) expect((await get('/rooms/2/subrooms/99999/saves/1', '1')).status).toBe(404) - // Same gate as the list it details: 401 unauthed, 403 for someone who is neither the - // creator nor in the room (a co-owner included) — it reads unpublished saves. + // Same gate as the list it details: 401 unauthed, 403 for someone who neither manages + // the room nor is in it — it reads unpublished saves. A co-owner (account 2) passes. const detail = `/rooms/2/subrooms/2/saves/${row.SubRoomDataSaveId}` expect((await get(detail)).status).toBe(401) expect((await get(detail, '999')).status).toBe(403) - expect((await get(detail, '2')).status).toBe(403) + expect((await get(detail, '2')).status).toBe(200) // A player standing in the room reads it, for as long as they're there. await putInRoom(999, 2) expect((await get(detail, '999')).status).toBe(200)