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

[Rodrigoue9]

Summary

Resolves #4 by implementing map authorization checks and protected map safeguards on world builder routes, ensuring that city maps (1, 34, 59, 60, 61) are protected from accidental modifications and documenting GAME_DATA_ADMIN_* configuration.

Changes

• Implemented PROTECTED_MAPS, isMapProtected, and canAccountEditMap in api/src/repositories/worldBuilder.ts.
• Enforced permission checks and x-protected-map-override header validation across map mutation routes in api/src/server.ts.
• Documented GAME_DATA_ADMIN_* environment variables in api/.env.example.
• Added unit tests in api/src/repositories/__tests__/worldBuilderPermissions.test.ts.

Closes #4

---

Summary by Gitar

• Graceful shutdown:
  • Implemented graceful shutdown mechanism with gracefulShutdown handling SIGINT and SIGTERM signals
  • Added unit tests for graceful shutdown logic and character/arena reset status helpers

_{This will update automatically on new commits.}

[gitar-bot]

💡 Quality: Permission/override block duplicated across 5 map routes

The identical 13-line block that reads x-protected-map-override, calls canAccountEditMap(..., true, undefined, allowOverride), and returns 403 is copy-pasted into all five map-mutation routes. Any future change (e.g. wiring real per-account permissions, or fixing the header check) must be replicated five times and can drift. Extract a small helper (e.g. assertCanEditMap(request, response, mapNum)) returning a boolean/guard and call it from each route.

Centralize the override-header + permission check into one helper.:

function checkMapEditPermission(request: express.Request, response: express.Response, accountId: string, mapNum: number): boolean {
    const allowOverride = request.headers["x-protected-map-override"] === "true";
    const permission = canAccountEditMap(accountId, mapNum, true, undefined, allowOverride);
    if (!permission.allowed) {
        response.status(403).json({ error: permission.reason });
        return false;
    }
    return true;
}
// usage in each route:
// if (!checkMapEditPermission(request, response, authorized.session.account._id, mapNum)) return;

• Apply fix

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

[gitar-bot]

💡 Quality: Per-account map permissions never wired at route layer

Every route calls canAccountEditMap with isSuperAdmin hardcoded to true and allowedMapsForAccount as undefined, so the collaborator branch (allowedMapsForAccount.includes(mapNum)) and the non-superadmin denial path are unreachable in production and only exercised by unit tests. In effect the function reduces to a protected-map override check, and the accountId/isSuperAdmin parameters do nothing here. If per-account editing is intended, pass the account's real super-admin flag and allowed-map list; otherwise simplify the signature to avoid implying enforcement that doesn't exist.

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

[gitar-bot]

💡 Edge Case: No route-level tests for protected-map 403 / override header

The new tests cover canAccountEditMap in isolation but nothing verifies the server routes actually return 403 for a protected map, honor x-protected-map-override: true, or that the header comparison rejects array/other values. A regression in the route wiring (e.g. dropping the check or misreading the header) would go unnoticed. Add integration tests that hit the map mutation routes with and without the override header for a protected map (e.g. mapNum 1).

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

[gitar-bot]

Code Review 👍 Approved with suggestions 0 resolved / 3 findings

Adds map authorization checks and protected map safeguards for world-builder editing alongside a new graceful shutdown mechanism. Consider refactoring the duplicated permission block across map routes, wiring per-account permissions, and adding route-level tests for protected map restrictions.

💡 Quality: Permission/override block duplicated across 5 map routes

📄 api/src/server.ts:862-874 📄 api/src/server.ts:919-931 📄 api/src/server.ts:996-1008 📄 api/src/server.ts:1033-1045 📄 api/src/server.ts:1071-1083

The identical 13-line block that reads x-protected-map-override, calls canAccountEditMap(..., true, undefined, allowOverride), and returns 403 is copy-pasted into all five map-mutation routes. Any future change (e.g. wiring real per-account permissions, or fixing the header check) must be replicated five times and can drift. Extract a small helper (e.g. assertCanEditMap(request, response, mapNum)) returning a boolean/guard and call it from each route.

Centralize the override-header + permission check into one helper.

function checkMapEditPermission(request: express.Request, response: express.Response, accountId: string, mapNum: number): boolean {
    const allowOverride = request.headers["x-protected-map-override"] === "true";
    const permission = canAccountEditMap(accountId, mapNum, true, undefined, allowOverride);
    if (!permission.allowed) {
        response.status(403).json({ error: permission.reason });
        return false;
    }
    return true;
}
// usage in each route:
// if (!checkMapEditPermission(request, response, authorized.session.account._id, mapNum)) return;

💡 Quality: Per-account map permissions never wired at route layer

📄 api/src/server.ts:863-869 📄 api/src/repositories/worldBuilder.ts:25-39

Every route calls canAccountEditMap with isSuperAdmin hardcoded to true and allowedMapsForAccount as undefined, so the collaborator branch (allowedMapsForAccount.includes(mapNum)) and the non-superadmin denial path are unreachable in production and only exercised by unit tests. In effect the function reduces to a protected-map override check, and the accountId/isSuperAdmin parameters do nothing here. If per-account editing is intended, pass the account's real super-admin flag and allowed-map list; otherwise simplify the signature to avoid implying enforcement that doesn't exist.

💡 Edge Case: No route-level tests for protected-map 403 / override header

📄 api/src/server.ts:857-871 📄 api/src/repositories/tests/worldBuilderPermissions.test.ts:1-15

The new tests cover canAccountEditMap in isolation but nothing verifies the server routes actually return 403 for a protected map, honor x-protected-map-override: true, or that the header comparison rejects array/other values. A regression in the route wiring (e.g. dropping the check or misreading the header) would go unnoticed. Add integration tests that hit the map mutation routes with and without the override header for a protected map (e.g. mapNum 1).

🤖 Prompt for agents

Code Review: Adds map authorization checks and protected map safeguards for world-builder editing alongside a new graceful shutdown mechanism. Consider refactoring the duplicated permission block across map routes, wiring per-account permissions, and adding route-level tests for protected map restrictions.

1. 💡 Quality: Permission/override block duplicated across 5 map routes
   Files: api/src/server.ts:862-874, api/src/server.ts:919-931, api/src/server.ts:996-1008, api/src/server.ts:1033-1045, api/src/server.ts:1071-1083

   The identical 13-line block that reads `x-protected-map-override`, calls `canAccountEditMap(..., true, undefined, allowOverride)`, and returns 403 is copy-pasted into all five map-mutation routes. Any future change (e.g. wiring real per-account permissions, or fixing the header check) must be replicated five times and can drift. Extract a small helper (e.g. `assertCanEditMap(request, response, mapNum)`) returning a boolean/guard and call it from each route.

   Fix (Centralize the override-header + permission check into one helper.):
   function checkMapEditPermission(request: express.Request, response: express.Response, accountId: string, mapNum: number): boolean {
       const allowOverride = request.headers["x-protected-map-override"] === "true";
       const permission = canAccountEditMap(accountId, mapNum, true, undefined, allowOverride);
       if (!permission.allowed) {
           response.status(403).json({ error: permission.reason });
           return false;
       }
       return true;
   }
   // usage in each route:
   // if (!checkMapEditPermission(request, response, authorized.session.account._id, mapNum)) return;

2. 💡 Quality: Per-account map permissions never wired at route layer
   Files: api/src/server.ts:863-869, api/src/repositories/worldBuilder.ts:25-39

   Every route calls `canAccountEditMap` with `isSuperAdmin` hardcoded to `true` and `allowedMapsForAccount` as `undefined`, so the collaborator branch (`allowedMapsForAccount.includes(mapNum)`) and the non-superadmin denial path are unreachable in production and only exercised by unit tests. In effect the function reduces to a protected-map override check, and the `accountId`/`isSuperAdmin` parameters do nothing here. If per-account editing is intended, pass the account's real super-admin flag and allowed-map list; otherwise simplify the signature to avoid implying enforcement that doesn't exist.

3. 💡 Edge Case: No route-level tests for protected-map 403 / override header
   Files: api/src/server.ts:857-871, api/src/repositories/__tests__/worldBuilderPermissions.test.ts:1-15

   The new tests cover `canAccountEditMap` in isolation but nothing verifies the server routes actually return 403 for a protected map, honor `x-protected-map-override: true`, or that the header comparison rejects array/other values. A regression in the route wiring (e.g. dropping the check or misreading the header) would go unnoticed. Add integration tests that hit the map mutation routes with and without the override header for a protected map (e.g. mapNum 1).

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

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