-
Notifications
You must be signed in to change notification settings - Fork 25
feat(world-builder): permissions and protected map restrictions for map editing (#4) #110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
ec22f9c
cf09ac9
e78070f
f4f523a
ac00742
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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 | ||
| }); | ||
|
|
||
| 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"); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,10 +96,12 @@ import { | |
| upsertGameBalance, | ||
| } from "./repositories/gameBalance"; | ||
| import { | ||
| canAccountEditMap, | ||
| clearTile, | ||
| discardDrafts, | ||
| getGraphicContent, | ||
| getMapStatus, | ||
| isMapProtected, | ||
| listGraphics, | ||
| listMapOverrides, | ||
| paintTiles, | ||
|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Quality: Per-account map permissions never wired at route layerEvery route calls Was this helpful? React with 👍 / 👎 |
||
|
|
||
| if (!permission.allowed) { | ||
| response.status(403).json({ error: permission.reason }); | ||
| return; | ||
| } | ||
|
Comment on lines
+862
to
+874
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Quality: Permission/override block duplicated across 5 map routesThe identical 13-line block that reads Centralize the override-header + permission check into one helper.:
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) { | ||
|
|
@@ -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 = | ||
|
|
@@ -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), | ||
| ); | ||
|
|
@@ -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 = | ||
|
|
@@ -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 = | ||
|
|
||
| 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, | ||
| ); | ||
| }); |
| 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); | ||
| }); |
There was a problem hiding this comment.
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
canAccountEditMapin isolation but nothing verifies the server routes actually return 403 for a protected map, honorx-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 👍 / 👎