Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions api/.env.example
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,8 @@ PORT=3001
DATABASE_URL=postgresql://postgres:postgres@localhost:5432/aoweb
TOKEN_AUTH=changeme
CORS_ORIGIN=http://localhost:3000

# Game Data Administration & Map Builder
GAME_DATA_ADMIN_EMAIL=admin@aoweb.app
GAME_DATA_ADMIN_ACCOUNT_ID=
GAME_DATA_ADMIN_PROXY_TOKEN=
42 changes: 42 additions & 0 deletions api/src/repositories/__tests__/worldBuilderPermissions.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { describe, it, expect } from "vitest";
import {
PROTECTED_MAPS,
isMapProtected,
canAccountEditMap,
} from "../worldBuilder";

describe("World Builder Map Permissions and Protections (#4)", () => {
it("should identify city maps as protected by default", () => {
expect(isMapProtected(1)).toBe(true); // Ullathorpe
expect(isMapProtected(34)).toBe(true); // Nix
expect(isMapProtected(59)).toBe(true); // Banderbill
expect(isMapProtected(50)).toBe(false); // Regular map
});

Comment on lines +1 to +15

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

it("should reject edits to protected maps when override is false", () => {
const result = canAccountEditMap("admin_123", 1, true, undefined, false);
expect(result.allowed).toBe(false);
expect(result.reason).toContain("protegido contra modificaciones");
});

it("should allow edits to protected maps when override is true", () => {
const result = canAccountEditMap("admin_123", 1, true, undefined, true);
expect(result.allowed).toBe(true);
});

it("should allow superadmin to edit non-protected maps", () => {
const result = canAccountEditMap("admin_123", 50, true, undefined, false);
expect(result.allowed).toBe(true);
});

it("should allow collaborator to edit specifically assigned map", () => {
const result = canAccountEditMap("collab_456", 50, false, [50, 51], false);
expect(result.allowed).toBe(true);
});

it("should reject collaborator attempting to edit unassigned map", () => {
const result = canAccountEditMap("collab_456", 100, false, [50, 51], false);
expect(result.allowed).toBe(false);
expect(result.reason).toContain("no tiene permisos");
});
});
38 changes: 38 additions & 0 deletions api/src/repositories/worldBuilder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,44 @@ export const UPLOADED_GRAPHIC_INDEX_START = 1_000_000;
/** Los mapas del juego son de 100x100. */
export const MAP_SIZE = 100;

/**
* Mapas protegidos contra modificaciones destructivas (ciudades principales).
* Requieren autorizacion explicita con override para ser modificados.
*/
export const PROTECTED_MAPS = new Set<number>([1, 34, 59, 60, 61]);

export function isMapProtected(mapNum: number): boolean {
return PROTECTED_MAPS.has(mapNum);
}

export function canAccountEditMap(
accountId: string,
mapNum: number,
isSuperAdmin: boolean,
allowedMapsForAccount?: number[],
allowProtectedOverride = false,
): { allowed: boolean; reason?: string } {
if (isMapProtected(mapNum) && !allowProtectedOverride) {
return {
allowed: false,
reason: `El mapa ${mapNum} esta protegido contra modificaciones.`,
};
}

if (isSuperAdmin) {
return { allowed: true };
}

if (allowedMapsForAccount && allowedMapsForAccount.includes(mapNum)) {
return { allowed: true };
}

return {
allowed: false,
reason: `La cuenta ${accountId} no tiene permisos para editar el mapa ${mapNum}.`,
};
}

export type UploadedGraphic = {
grhIndex: number;
checksum: string;
Expand Down
72 changes: 72 additions & 0 deletions api/src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,12 @@ import {
upsertGameBalance,
} from "./repositories/gameBalance";
import {
canAccountEditMap,
clearTile,
discardDrafts,
getGraphicContent,
getMapStatus,
isMapProtected,
listGraphics,
listMapOverrides,
paintTiles,
Expand Down Expand Up @@ -857,6 +859,20 @@ app.put("/admin/game-data/maps/:mapNum/tiles", async (request, response) => {
return;
}

const allowOverride = request.headers["x-protected-map-override"] === "true";
const permission = canAccountEditMap(
authorized.session.account._id,
mapNum,
true,
undefined,
allowOverride,
);
Comment on lines +863 to +869

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


if (!permission.allowed) {
response.status(403).json({ error: permission.reason });
return;
}
Comment on lines +862 to +874

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


const parsed = paintTilesSchema.safeParse(request.body);

if (!parsed.success) {
Expand Down Expand Up @@ -900,6 +916,20 @@ app.delete(
return;
}

const allowOverride = request.headers["x-protected-map-override"] === "true";
const permission = canAccountEditMap(
authorized.session.account._id,
mapNum,
true,
undefined,
allowOverride,
);

if (!permission.allowed) {
response.status(403).json({ error: permission.reason });
return;
}

response.json({ removed: await clearTile(mapNum, x, y, layer) });
} catch (error) {
const message =
Expand Down Expand Up @@ -963,6 +993,20 @@ app.post("/admin/game-data/maps/:mapNum/publish", async (request, response) => {
return;
}

const allowOverride = request.headers["x-protected-map-override"] === "true";
const permission = canAccountEditMap(
authorized.session.account._id,
mapNum,
true,
undefined,
allowOverride,
);

if (!permission.allowed) {
response.status(403).json({ error: permission.reason });
return;
}

response.json(
await publishMap(mapNum, authorized.session.account._id),
);
Expand All @@ -986,6 +1030,20 @@ app.post("/admin/game-data/maps/:mapNum/discard", async (request, response) => {
return;
}

const allowOverride = request.headers["x-protected-map-override"] === "true";
const permission = canAccountEditMap(
authorized.session.account._id,
mapNum,
true,
undefined,
allowOverride,
);

if (!permission.allowed) {
response.status(403).json({ error: permission.reason });
return;
}

response.json(await discardDrafts(mapNum));
} catch (error) {
const message =
Expand All @@ -1010,6 +1068,20 @@ app.post("/admin/game-data/maps/:mapNum/revert", async (request, response) => {
return;
}

const allowOverride = request.headers["x-protected-map-override"] === "true";
const permission = canAccountEditMap(
authorized.session.account._id,
mapNum,
true,
undefined,
allowOverride,
);

if (!permission.allowed) {
response.status(403).json({ error: permission.reason });
return;
}

response.json(await revertMap(mapNum));
} catch (error) {
const message =
Expand Down
46 changes: 46 additions & 0 deletions api/src/tests/shutdownReset.unit.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import assert from "node:assert/strict";
import { test, vi } from "vitest";

const query = vi.hoisted(() => vi.fn());

vi.mock("../db", () => ({
default: { query },
}));

import { resetAllCharactersConnectedStatus } from "../repositories/characters";
import { resetAllArenaRoomMembersConnectedStatus } from "../repositories/arenas";

test("reset helpers clear connected characters and arena members", async () => {
query
.mockReset()
.mockResolvedValueOnce({ rowCount: 4 })
.mockResolvedValueOnce({ rowCount: 2 });

const [updatedCharacters, updatedArenaMembers] = await Promise.all([
resetAllCharactersConnectedStatus(),
resetAllArenaRoomMembersConnectedStatus(),
]);

assert.equal(updatedCharacters, 4);
assert.equal(updatedArenaMembers, 2);
assert.equal(query.mock.calls.length, 2);

const sqlStatements = query.mock.calls.map(([sql]) => String(sql));
assert.equal(
sqlStatements.some(
(sql) =>
sql.includes("UPDATE characters") &&
sql.includes("connected = FALSE") &&
sql.includes("deleted_at IS NULL"),
),
true,
);
assert.equal(
sqlStatements.some(
(sql) =>
sql.includes("UPDATE arena_room_members") &&
sql.includes("connected = FALSE"),
),
true,
);
});
66 changes: 66 additions & 0 deletions server/src/gracefulShutdown.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import assert from "node:assert/strict";
import { test } from "node:test";
import {
gracefulShutdown,
type GracefulShutdownDependencies,
type ShutdownClient,
} from "./gracefulShutdown";

test("gracefulShutdown closes clients and resets connected characters", async () => {
const closedClients: string[] = [];
const requests: Array<{ url: string; options: RequestInit }> = [];
const output: string[] = [];
const errors: string[] = [];
let exitCode: number | undefined;
let clearedTimer = false;

const clients: Record<string, ShutdownClient> = {
open: {
readyState: 1,
OPEN: 1,
close: () => closedClients.push("open"),
},
closed: {
readyState: 3,
OPEN: 1,
close: () => closedClients.push("closed"),
},
};

const dependencies: GracefulShutdownDependencies = {
clients,
tokenAuth: "test-token",
fetchUrl: async (url, options) => {
requests.push({ url, options });
return { updated: 4 };
},
exit: (code) => {
exitCode = code;
},
setTimeout: (callback) => {
void callback;
return setTimeout(() => undefined, 60_000);
},
clearTimeout: (timer) => {
clearedTimer = true;
clearTimeout(timer);
},
writeOut: (message) => output.push(message),
writeErr: (message) => errors.push(message),
};

await gracefulShutdown("SIGTERM", dependencies);

assert.deepEqual(closedClients, ["open"]);
assert.equal(requests.length, 1);
assert.equal(requests[0]?.url, "/internal/characters/reset-connected");
assert.equal(requests[0]?.options.method, "POST");
assert.equal(
(requests[0]?.options.headers as Record<string, string>).Authorization,
"test-token",
);
assert.equal(exitCode, 0);
assert.equal(clearedTimer, true);
assert.equal(errors.length, 0);
assert.equal(output.some((message) => message.includes("4 personajes")), true);
});
Loading