Skip to content

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

Open
angelTomo9 wants to merge 1 commit into
Bitcoindefi:mainfrom
angelTomo9:feat-world-builder-permissions-1787657637722
Open

feat(world-builder): permissions and protected map restrictions for map editing (#4)#129
angelTomo9 wants to merge 1 commit into
Bitcoindefi:mainfrom
angelTomo9:feat-world-builder-permissions-1787657637722

Conversation

@angelTomo9

Copy link
Copy Markdown

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.

Comment thread api/src/server.ts
@@ -976,16 +1039,22 @@ app.post("/admin/game-data/maps/:mapNum/publish", async (request, response) => {
/** Descarta los borradores sin tocar lo ya publicado. */
app.post("/admin/game-data/maps/:mapNum/discard", async (request, response) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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 👍 / 👎

Comment thread api/schema.sql
Comment on lines +633 to +642
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)
);

CREATE INDEX IF NOT EXISTS idx_game_map_permissions_account_map
ON game_map_permissions(account_id, map_num);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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 👍 / 👎

Comment thread api/src/server.ts
Comment on lines +1097 to +1111
app.post(
"/admin/game-data/maps/:mapNum/permissions/:accountId",
async (request, response) => {
try {
const authorized = await requireAdminEmailSession(
request,
response,
);
if (!authorized) return;

const mapNum = Number.parseInt(request.params.mapNum ?? "", 10);
const accountId = request.params.accountId ?? "";

if (!Number.isInteger(mapNum) || mapNum < 0 || !accountId) {
response.status(400).json({ error: "Parametros invalidos." });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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 👍 / 👎

Comment on lines +25 to +34
it("rejects unauthorized accounts without permissions with 403", async () => {
const result = await checkMapEditPermission({
accountId: unauthorizedAccountId,
isSuperAdmin: false,
mapNum: 50,
});

assert.equal(result.allowed, false);
assert.match(result.reason ?? "", /no tiene permisos/i);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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

gitar-bot Bot commented Aug 25, 2026

Copy link
Copy Markdown
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Etapa 0: permisos y atribucion para edicion de mapas

1 participant