From cbda6d64a7d6e4a8a11f8d58b8c00fc2ec19b850 Mon Sep 17 00:00:00 2001 From: MelekTaus Date: Wed, 30 Sep 2026 21:13:20 +0200 Subject: [PATCH] Let only the owner delete a project, not an administrator (#272) #179 decided that nobody deletes a project on someone else's behalf. The API still let an administrator do it, because the admin store treated every project as its own. Option A from the issue is now in place: opening, editing, snapshots and release stay open to the administrator, deleting stays with the owner. - Store::delete checks the owner under the save lock and throws NotOwnerException for anyone else. It follows the pattern of StaleRevisionException. - The route answers that with 403 "Only its owner may delete a project". A file without an owner is covered the same way. Non-admins still get 404 for a project that is not theirs. - The editor has no way to delete a foreign project, the project picker lists only one's own. Nothing changes there. - An account that owns projects can no longer be deleted by an administrator at all. Its owner has to delete them first. - e2e 49 deleted the throwaway projects as administrator, both in its sweep and in 49i. It now signs in as the owner for that. 49i checks both answers. Tests: - tests/php/accounts.test.php: 56 checks passed (was 52, one check turned around, five new) - e2e 49: 22 checks passed (was 21), deleteByAdmin 403, deleteByOwner 204 - Gate: 197 files, 2855 tests passed - Counter-checks: without the owner check 4 PHP checks and 49i fail - The e2e total of 1603 is computed (1602 + 1), not measured Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- api/index.php | 13 ++++++++++--- api/lib/Store.php | 30 ++++++++++++++++++++++++++---- tests/php/accounts.test.php | 21 ++++++++++++++++----- tools/e2e/49-users.mjs | 20 ++++++++++++++++---- 5 files changed, 69 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index 0d455975..f2af98d3 100644 --- a/README.md +++ b/README.md @@ -45,7 +45,7 @@ npm run build # writes dist/ and regenerates index.html; the folder is wh The browser suite and the three example games need a machine with a GPU: ```bash -node tools/seed-e2e.mjs && node tools/e2e.mjs # the editor, 1602 checks (GM_STEPS=20 for one step) +node tools/seed-e2e.mjs && node tools/e2e.mjs # the editor, 1603 checks (GM_STEPS=20 for one step) # every tool signs in first – see the Accounts page node tools/seed-jumprun.mjs && node tools/smoke-jumprun.mjs node tools/seed-tetris.mjs && node tools/smoke-tetris.mjs diff --git a/api/index.php b/api/index.php index a57830b2..4ca149c1 100644 --- a/api/index.php +++ b/api/index.php @@ -16,7 +16,8 @@ declare(strict_types=1); * * Everything below needs a signed-in user (401 otherwise), and projects belong to one: a project * of somebody else answers like one that is not there – unless the one asking is an administrator - * (#179), who reads, saves and deletes it like their own; a save keeps its owner. + * (#179), who reads and saves it like their own; a save keeps its owner. Deleting a project is + * its owner's alone (#272). * * Administrators only (403 for everybody else, #180): * @@ -31,7 +32,8 @@ declare(strict_types=1); * POST ?r=projects {name} → {id,name,revision,updatedAt,project} * GET ?r=projects/{id} → {id,name,revision,updatedAt,project} * PUT ?r=projects/{id} {revision, project} → {revision,updatedAt} | 409 {error,revision} - * DELETE ?r=projects/{id} → 204 (its snapshots and its release go with it) + * DELETE ?r=projects/{id} → 204 | 403 | 404 (its snapshots and its release go with it; + * 403 for an administrator who is not the owner) * * GET ?r=projects/{id}/snapshots → [{id,name,revision,createdAt}], newest first * POST ?r=projects/{id}/snapshots {name?} → 201 {id,name,revision,createdAt} @@ -230,7 +232,12 @@ function routeProjects(array $parts, string $method, string $owner, bool $seesAl } if ($method === 'DELETE') { - if (!$store->delete($id)) { + try { + $deleted = $store->delete($id); + } catch (NotOwnerException $e) { + Response::error($e->getMessage(), 403); + } + if (!$deleted) { Response::error('Project not found', 404); } $snapshots->deleteAll($id); diff --git a/api/lib/Store.php b/api/lib/Store.php index 52a505b0..f77bfda8 100644 --- a/api/lib/Store.php +++ b/api/lib/Store.php @@ -10,6 +10,15 @@ final class StaleRevisionException extends RuntimeException } } +/** Thrown by Store::delete when an administrator reaches a project that is not their own (#272). */ +final class NotOwnerException extends RuntimeException +{ + public function __construct() + { + parent::__construct('Only its owner may delete a project'); + } +} + /** * File-based project store: one JSON document per project in `data/projects/{id}.json`. * Each file wraps the project with metadata: { id, name, owner, revision, updatedAt, project }. @@ -20,9 +29,9 @@ final class StaleRevisionException extends RuntimeException * `owner` is that user. Someone else's project is answered as missing rather than as forbidden, * because "this id exists" is already more than a stranger should learn. * - * An administrator's store (`$seesAll`, #179) reads, writes and deletes every project as if it - * were their own, and a save keeps the owner it found. The list stays their own: the others are - * reached through the user management, one account at a time. + * An administrator's store (`$seesAll`, #179) reads and writes every project as if it were their + * own, and a save keeps the owner it found. Deleting stays with the owner (#272). The list stays + * their own: the others are reached through the user management, one account at a time. */ final class Store { @@ -121,11 +130,24 @@ final class Store }); } + /** + * Deletes a project of this store's user. False when there is none to reach; throws + * NotOwnerException when an administrator reaches one of somebody else's. + */ public function delete(string $id): bool { $path = $this->path($id); // Under the save lock, or a save running at the same time writes the project back. - $deleted = $this->locked($path, fn(): bool => $this->load($id) !== null && unlink($path)); + $deleted = $this->locked($path, function () use ($id, $path): bool { + $doc = $this->load($id); + if ($doc === null) { + return false; + } + if ($doc['owner'] !== $this->owner) { + throw new NotOwnerException(); + } + return unlink($path); + }); if ($deleted) { // Whoever still waits for the lock finds no project and writes nothing. @unlink($path . '.lock'); diff --git a/tests/php/accounts.test.php b/tests/php/accounts.test.php index 683f301e..7e2a9e94 100644 --- a/tests/php/accounts.test.php +++ b/tests/php/accounts.test.php @@ -27,12 +27,15 @@ function check(string $name, bool $ok, mixed $got = null): void echo "FAIL {$name}", $got === null ? '' : ' — got ' . json_encode($got), "\n"; } -function throws(callable $work): bool +function throws(callable $work, string $class = InvalidArgumentException::class): bool { try { $work(); - } catch (InvalidArgumentException) { - return true; + } catch (Throwable $e) { + if ($e instanceof $class) { + return true; + } + throw $e; } return false; } @@ -110,8 +113,16 @@ check('the new name owns it', $moved !== null && $moved['owner'] === 'annabel'); check('the old name does not', $mine->load($id) === null); check('transfer keeps the revision', $moved['revision'] === 2, $moved['revision'] ?? null); -$other = (new Store($projects, 'carl'))->create('Carl\'s game'); -check('an administrator deletes somebody else\'s project', $admin->delete($other['id']) && $admin->listOf('carl') === []); +$carl = new Store($projects, 'carl'); +$other = $carl->create('Carl\'s game'); +check('an administrator may not delete somebody else\'s project', throws(fn() => $admin->delete($other['id']), NotOwnerException::class)); +check('the refused delete leaves the project', $carl->load($other['id']) !== null); +check('its owner deletes it', $carl->delete($other['id']) && $admin->listOf('carl') === []); +$ownerless = "{$projects}/nobody01.json"; +file_put_contents($ownerless, json_encode(['id' => 'nobody01', 'revision' => 1, 'project' => []])); +check('an administrator may not delete a project without an owner', throws(fn() => $admin->delete('nobody01'), NotOwnerException::class) && is_file($ownerless)); +$own = $admin->create('Boss\'s game'); +check('an administrator deletes their own project', $admin->delete($own['id']) && $admin->list() === []); // The web server serves `data/` as static files, so a readable project would skip the sign-in. check('a project file is not readable for the web server', (fileperms("{$projects}/{$id}.json") & 0o007) === 0); diff --git a/tools/e2e/49-users.mjs b/tools/e2e/49-users.mjs index c67fbcd5..a5219f1b 100644 --- a/tools/e2e/49-users.mjs +++ b/tools/e2e/49-users.mjs @@ -49,11 +49,15 @@ export async function users(t) { signal: AbortSignal.timeout(20_000), }); const asAdmin = (method, route, body) => call(adminCookie, method, route, body); - // Projects first: an account that still owns one is not deleted (#179). + // Projects first: an account that still owns one is not deleted (#179), and only the owner may + // delete them (#272). The step never changes the throwaway password, so it still signs in. const sweep = async () => { const accounts = await (await asAdmin('GET', 'users')).json(); for (const account of accounts.filter((a) => a.username === TEMP || a.username === RENAMED)) { - for (const project of account.projects) await asAdmin('DELETE', `projects/${project.id}`); + if (account.projects.length > 0) { + const ownerCookie = await logIn(baseUrl, account.username, TEMP_PASSWORD); + for (const project of account.projects) await call(ownerCookie, 'DELETE', `projects/${project.id}`); + } await asAdmin('DELETE', `users/${account.username}`); } }; @@ -283,8 +287,16 @@ export async function users(t) { JSON.stringify({ name: stored.name, owner: stored.owner }), ); - // 49i. With the project gone, the account goes too. - await asAdmin('DELETE', `projects/${made.id}`); + // 49i. Opening and editing are not deleting: that stays with the owner (#272). With the + // project gone, the account goes too. + const deleteByAdmin = (await asAdmin('DELETE', `projects/${made.id}`)).status; + const ownerCookie = await logIn(baseUrl, RENAMED, TEMP_PASSWORD); + const deleteByOwner = (await call(ownerCookie, 'DELETE', `projects/${made.id}`)).status; + check( + 'an administrator may not delete somebody else’s project, its owner may', + deleteByAdmin === 403 && deleteByOwner === 204, + JSON.stringify({ deleteByAdmin, deleteByOwner }), + ); await openManagement(); await pressButton(RENAMED, 'delete'); await clickLast('Delete');