feat(world-builder): permissions and protected map restrictions for map editing (#4)

[angelTomo9]

Closes #4

Summary of Changes

Implements granular map editing permissions, protected capital/city map safeguards, account mutation attribution, and permission management endpoints.

Deliverables & Architectural Features

• Protected Maps Guard: Designated key capital/city maps (Ullathorpe 1, Nix 34, Banderbill 59, Lindos 150) as PROTECTED_MAPS. Protected maps reject modifications unless superadmins explicitly pass overrideProtected = true.
• Granular Map Permissions: Added game_map_permissions table in api/schema.sql enabling map-level assignment for builders/collaborators without granting global destructive permissions.
• Attribution: Every map tile paint, deletion, publish, discard, and revert operation enforces requireMapEditSession and records updated_by_account_id.
• Admin Endpoints: Added endpoints to grant (POST /admin/game-data/maps/:mapNum/permissions/:accountId), revoke (DELETE), and list (GET) map permissions.
• Documentation: Documented GAME_DATA_ADMIN_* configuration in api/.env.example.
• Integration Tests: Added unit & integration tests in api/src/tests/worldBuilder_permissions.integration.test.ts.

[gitar-bot]

⚠️ Security: Collaborators can run destructive revert/discard on maps

The publish, discard and revert endpoints now gate only on requireMapEditSession, which passes for any non-superadmin collaborator that holds a game_map_permissions row (including a global map_num = 0 grant). revertMap deletes ALL tiles (published + drafts — the described "panic button") and discardDrafts wipes drafts. This contradicts the stated intent of granting map-level access "without granting global destructive permissions": a builder given one map (or global) can now destroy published content. Consider requiring authorized.isSuperAdmin (or a distinct permission flag) for the revert and discard routes rather than treating them like a normal tile paint.

_{Was this helpful? React with 👍 / 👎}

[gitar-bot]

💡 Quality: Redundant index duplicates game_map_permissions primary key

idx_game_map_permissions_account_map indexes (account_id, map_num), which is identical (same columns, same order) to the PRIMARY KEY (account_id, map_num) that Postgres already backs with a unique btree index. The extra index adds write overhead and storage without benefiting any query. Drop the explicit index.

Remove the duplicate index; the primary key already covers these lookups.:

CREATE TABLE IF NOT EXISTS game_map_permissions (
    account_id UUID NOT NULL REFERENCES accounts(id) ON DELETE CASCADE,
    map_num INTEGER NOT NULL CHECK (map_num >= 0),
    granted_by UUID REFERENCES accounts(id) ON DELETE SET NULL,
    created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(),
    PRIMARY KEY (account_id, map_num)
);
-- (removed redundant idx_game_map_permissions_account_map)

• Apply fix

_{Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎}

[gitar-bot]

💡 Quality: Granting a protected map to a collaborator is silently useless

grantMapPermission accepts any mapNum >= 0, including protected maps (1, 34, 59, 150). But checkMapEditPermission rejects protected maps for non-superadmins before consulting game_map_permissions, so such a grant can never take effect and the admin gets a misleading { ok: true }. Reject grants for protected map numbers (or return a warning) so operators aren't misled.

_{Was this helpful? React with 👍 / 👎}

[gitar-bot]

💡 Quality: Permission integration tests lack DB setup and positive collaborator case

worldBuilder_permissions.integration.test.ts calls checkMapEditPermission for the unauthorized account (mapNum 50), which executes a live pool.query against game_map_permissions, yet the file has no DB setup/seeding/teardown — the test will throw on connection failure and there is no positive test seeding a collaborator grant to verify the allowed: true collaborator path. Add fixture setup (or mock the pool) and a granted-collaborator assertion.

_{Was this helpful? React with 👍 / 👎}

[gitar-bot]

Code Review ⚠️ Changes requested 0 resolved / 4 findings

Implements map editing permissions and protected map restrictions, but collaborators can bypass destructive action checks on revert and discard. Additionally, redundant indexing duplicates the primary key and permission tests lack database setup.

⚠️ Security: Collaborators can run destructive revert/discard on maps

📄 api/src/server.ts:1040 📄 api/src/server.ts:1056 📄 api/src/server.ts:1070 📄 api/src/server.ts:1086 📄 api/src/repositories/worldBuilder.ts:355 📄 api/src/repositories/worldBuilder.ts:370

The publish, discard and revert endpoints now gate only on requireMapEditSession, which passes for any non-superadmin collaborator that holds a game_map_permissions row (including a global map_num = 0 grant). revertMap deletes ALL tiles (published + drafts — the described "panic button") and discardDrafts wipes drafts. This contradicts the stated intent of granting map-level access "without granting global destructive permissions": a builder given one map (or global) can now destroy published content. Consider requiring authorized.isSuperAdmin (or a distinct permission flag) for the revert and discard routes rather than treating them like a normal tile paint.

💡 Quality: Redundant index duplicates game_map_permissions primary key

📄 api/schema.sql:633-642

idx_game_map_permissions_account_map indexes (account_id, map_num), which is identical (same columns, same order) to the PRIMARY KEY (account_id, map_num) that Postgres already backs with a unique btree index. The extra index adds write overhead and storage without benefiting any query. Drop the explicit index.

Remove the duplicate index; the primary key already covers these lookups.

CREATE TABLE IF NOT EXISTS game_map_permissions (
    account_id UUID NOT NULL REFERENCES accounts(id) ON DELETE CASCADE,
    map_num INTEGER NOT NULL CHECK (map_num >= 0),
    granted_by UUID REFERENCES accounts(id) ON DELETE SET NULL,
    created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(),
    PRIMARY KEY (account_id, map_num)
);
-- (removed redundant idx_game_map_permissions_account_map)

💡 Quality: Granting a protected map to a collaborator is silently useless

📄 api/src/server.ts:1097-1111 📄 api/src/repositories/worldBuilder.ts:459-465 📄 api/src/repositories/worldBuilder.ts:485-496

grantMapPermission accepts any mapNum >= 0, including protected maps (1, 34, 59, 150). But checkMapEditPermission rejects protected maps for non-superadmins before consulting game_map_permissions, so such a grant can never take effect and the admin gets a misleading { ok: true }. Reject grants for protected map numbers (or return a warning) so operators aren't misled.

💡 Quality: Permission integration tests lack DB setup and positive collaborator case

📄 api/src/tests/worldBuilder_permissions.integration.test.ts:25-34

worldBuilder_permissions.integration.test.ts calls checkMapEditPermission for the unauthorized account (mapNum 50), which executes a live pool.query against game_map_permissions, yet the file has no DB setup/seeding/teardown — the test will throw on connection failure and there is no positive test seeding a collaborator grant to verify the allowed: true collaborator path. Add fixture setup (or mock the pool) and a granted-collaborator assertion.

🤖 Prompt for agents

Code Review: Implements map editing permissions and protected map restrictions, but collaborators can bypass destructive action checks on revert and discard. Additionally, redundant indexing duplicates the primary key and permission tests lack database setup.

1. ⚠️ Security: Collaborators can run destructive revert/discard on maps
   Files: api/src/server.ts:1040, api/src/server.ts:1056, api/src/server.ts:1070, api/src/server.ts:1086, api/src/repositories/worldBuilder.ts:355, api/src/repositories/worldBuilder.ts:370

   The publish, discard and revert endpoints now gate only on `requireMapEditSession`, which passes for any non-superadmin collaborator that holds a `game_map_permissions` row (including a global `map_num = 0` grant). `revertMap` deletes ALL tiles (published + drafts — the described "panic button") and `discardDrafts` wipes drafts. This contradicts the stated intent of granting map-level access "without granting global destructive permissions": a builder given one map (or global) can now destroy published content. Consider requiring `authorized.isSuperAdmin` (or a distinct permission flag) for the revert and discard routes rather than treating them like a normal tile paint.

2. 💡 Quality: Redundant index duplicates game_map_permissions primary key
   Files: api/schema.sql:633-642

   `idx_game_map_permissions_account_map` indexes `(account_id, map_num)`, which is identical (same columns, same order) to the PRIMARY KEY `(account_id, map_num)` that Postgres already backs with a unique btree index. The extra index adds write overhead and storage without benefiting any query. Drop the explicit index.

   Fix (Remove the duplicate index; the primary key already covers these lookups.):
   CREATE TABLE IF NOT EXISTS game_map_permissions (
       account_id UUID NOT NULL REFERENCES accounts(id) ON DELETE CASCADE,
       map_num INTEGER NOT NULL CHECK (map_num >= 0),
       granted_by UUID REFERENCES accounts(id) ON DELETE SET NULL,
       created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(),
       PRIMARY KEY (account_id, map_num)
   );
   -- (removed redundant idx_game_map_permissions_account_map)

3. 💡 Quality: Granting a protected map to a collaborator is silently useless
   Files: api/src/server.ts:1097-1111, api/src/repositories/worldBuilder.ts:459-465, api/src/repositories/worldBuilder.ts:485-496

   `grantMapPermission` accepts any `mapNum >= 0`, including protected maps (1, 34, 59, 150). But `checkMapEditPermission` rejects protected maps for non-superadmins before consulting `game_map_permissions`, so such a grant can never take effect and the admin gets a misleading `{ ok: true }`. Reject grants for protected map numbers (or return a warning) so operators aren't misled.

4. 💡 Quality: Permission integration tests lack DB setup and positive collaborator case
   Files: api/src/tests/worldBuilder_permissions.integration.test.ts:25-34

   `worldBuilder_permissions.integration.test.ts` calls `checkMapEditPermission` for the unauthorized account (mapNum 50), which executes a live `pool.query` against `game_map_permissions`, yet the file has no DB setup/seeding/teardown — the test will throw on connection failure and there is no positive test seeding a collaborator grant to verify the `allowed: true` collaborator path. Add fixture setup (or mock the pool) and a granted-collaborator assertion.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

---

Important

Your trial ends in 7 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more.

_{Was this helpful? React with 👍 / 👎 | Gitar}