m0rph3us1987/HIPCTF2
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

AI Implementation feature(867): Admin Area Players Management (#56)

Browse Source
This commit was merged in pull request #56.
This commit is contained in:
m0rph3us1987
2026-07-23 08:08:50 +00:00
parent 705530e71f
commit e4ccf0fe13
27 changed files with 2039 additions and 218 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.kilo/plans/867.md
+50
View File
@@ -0,0 +1,50 @@
# Implementation Plan: Hardened User Deletion Cleanup + Concurrency-Safe Last-Admin Lock
## 1. Architectural Reconnaissance
- **Codebase style & conventions:** TypeScript NestJS 10 backend over `better-sqlite3` via TypeORM 0.3. All admin mutations live in `AdminService` (`backend/src/modules/admin/admin.service.ts:109-119`) and call a shared `UsersService.applyLastAdminSafeMutation` wrapper (`backend/src/modules/users/users.service.ts:63-95`). The wrapper currently uses `dataSource.transaction('SERIALIZABLE', ...)` which on `better-sqlite3` still emits a plain `BEGIN TRANSACTION` (deferred) and only escalates the SQLite write lock at the first write. Two concurrent transactions can therefore both pass the precondition `count > 1` before either commits, racing the invariant.
- **Data Layer:** `user`, `solve`, and `refresh_token` are the only tables with a `user_id` column. `blog_post` has no `author_id`, and awards are persisted as rows in `solve` itself (`points_awarded`, `is_first/second/third`). The new `AddUserDeleteCascades1700000000700` migration already adds `ON DELETE CASCADE` on `solve.user_id` and `refresh_token.user_id`, so a plain `manager.delete(UserEntity, ...)` would cascade in practice — but the user request asks us to perform the dependent cleanup explicitly inside the same transaction so the operation is observable to business code, logging, and the post-condition count.
- **Test Framework & Structure:** Jest 29 multi-project config (`tests/jest.config.js`); existing `tests/backend/last-admin-invariant.spec.ts` and `tests/backend/admin-players.spec.ts` already exercise the safe-mutation wrapper and the Players endpoints with an in-memory SQLite database. All new tests will be appended to those files (no new top-level test files required) and run via `npm run test:backend`.
- **Required Tools & Dependencies:** No new package or global CLI is required. The plan uses the existing TypeORM `QueryRunner`, the in-repo `ApiError` / `ERROR_CODES` envelope, and SQLite's native `BEGIN IMMEDIATE` semantics. If a future deployment moves to Postgres/MySQL, the locking strategy below degrades gracefully: `setLock('pessimistic_write')` is a no-op for read paths and a real row lock on databases that support it, while `BEGIN IMMEDIATE` is harmless outside SQLite.
## 2. Impacted Files
- **To Modify:**
- `backend/src/modules/admin/admin.service.ts` — perform explicit `solve`, `refresh_token`, and any other user-owned table deletion inside the same transaction as the `user` row delete; report per-table affected counts.
- `backend/src/modules/users/users.service.ts` — replace the read/count-only SERIALIZABLE wrapper with a `QueryRunner`-driven `BEGIN IMMEDIATE` transaction that performs row-level pessimistic locking where supported, retries on `SQLITE_BUSY`/serialization failures, and rolls back with `409 LAST_ADMIN` if the post-mutation admin count is zero.
- `backend/src/common/errors/error-codes.ts` — add a stable `CONFLICT_RETRY`/`CONFLICT_BUSY` code (only if the retry path needs to surface a distinct code; otherwise reuse `LAST_ADMIN` and rely on logs).
- `tests/backend/last-admin-invariant.spec.ts` — extend with explicit dependent-cleanup and `SQLITE_BUSY` retry coverage; add a concurrent `Promise.all` assertion that at most one writer can drive the admin count to zero.
- `tests/backend/admin-players.spec.ts` — add a delete-cascade assertion (no orphan `solve`/`refresh_token` rows after deletion) and a `LAST_ADMIN` rejection when the sole remaining admin is deleted.
- **To Create:** None. The dependent table set is already known (solve + refresh_token) and the existing migration already enforces ON DELETE CASCADE; the request explicitly asks us to delete the rows imperatively in the same transaction rather than rely solely on the cascade, so no new migration is required.
## 3. Proposed Changes
1. **Backend Logic & APIs — Dependent cleanup on delete (admin.service.ts:109-119):**
- Inside the existing `applyLastAdminSafeMutation` callback (which already supplies a TypeORM `EntityManager`), issue explicit deletions in this order:
1. `manager.delete(SolveEntity, { userId: targetId })` (capture `affected`).
2. `manager.delete(RefreshTokenEntity, { userId: targetId })` (capture `affected`).
3. `manager.delete(UserEntity, { id: targetId })` — this remains the actual user delete and is the only one that returns 404 when the row is missing; dependent tables short-circuit harmlessly when there are no rows.
- Sum the affected counts and return them from the mutate closure so the `applyLastAdminSafeMutation` can log them at `info` level (NEVER include `passwordHash`, `token_hash`, or the plaintext password). The cascade is retained as a safety net; the imperative delete is what the request mandates.
- Treat any user-owned table that is added in the future as opt-in: introduce a small registry (`USER_OWNED_TABLES = [SolveEntity, RefreshTokenEntity]`) iterated by the delete path so we don't have to add new call sites each time a new `userId` column appears. The plan keeps this registry co-located with the service; no new file.
2. **Backend Logic & APIs — Concurrency-safe last-admin wrapper (users.service.ts:63-95):**
- Replace the `dataSource.transaction('SERIALIZABLE', ...)` call with a `dataSource.createQueryRunner()`-driven flow:
1. `await queryRunner.connect()` then `await queryRunner.query('BEGIN IMMEDIATE')` so the very first statement acquires the SQLite RESERVED lock, preventing a second writer from racing through the precondition.
2. Acquire a row-level lock on the target user via `repo.createQueryBuilder('u').setLock('pessimistic_write').where('u.id = :id', { id: targetId }).getOne()`. This is a no-op for SQLite (the database-level lock is what matters) but a real `SELECT ... FOR UPDATE` on Postgres/MySQL if the deployment is ever migrated.
3. Acquire a second `pessimistic_write` lock on the full set of admin rows by selecting all `user.id` rows where `role = 'admin'` with `setLock('pessimistic_write')` (a no-op for SQLite; equivalent to a per-row `FOR UPDATE` on databases that support it). The aim is to serialize all concurrent admin mutations across the cluster.
4. Count admins under the lock, fail fast with `409 LAST_ADMIN` if `count <= 1` and the operation is destructive, otherwise apply the mutation, re-count under the lock, and roll back with `409 LAST_ADMIN` if the post-condition is `count <= 0`.
5. Commit; in the `catch`, attempt `ROLLBACK` and rethrow. Translate `SQLITE_BUSY` / serialization failures (error code `SQLITE_BUSY` from `better-sqlite3` and the TypeORM `LockNotSupportedError`/`TransactionAlreadyStartedError` chain) into a small retry loop (`maxRetries = 5`, exponential backoff 10ms / 20ms / 40ms / 80ms / 160ms). If retries are exhausted and the last failure still has the system in an indeterminate state, throw `ApiError.conflict(ERROR_CODES.LAST_ADMIN, ...)` so the caller always sees the existing 409 contract.
- Add a database-level safety net: a forward-only migration that adds a partial unique index on the `user` table. The current `user` schema has no `is_last_admin` column to enforce a CHECK on, so the simplest correct approach is to add a server-side trigger that raises on `UPDATE`/`DELETE` that would reduce the `admin` count to zero. SQL: `CREATE TRIGGER trg_user_last_admin BEFORE UPDATE OF role ON user WHEN NOT EXISTS (SELECT 1 FROM user WHERE role = 'admin' AND id <> NEW.id) AND OLD.role = 'admin' AND NEW.role <> 'admin' BEGIN SELECT RAISE(ABORT, 'LAST_ADMIN'); END;` plus a sibling `BEFORE DELETE` trigger with the same condition. The trigger is the deployment-wide safety net so even a buggy caller that bypasses the service wrapper cannot commit a zero-admin state. The service still rolls back to 409 in normal operation; the trigger ensures the database itself rejects the offending statement if it ever does slip through (e.g. raw SQL or a future parallel module that forgets the wrapper).
3. **Backend Logic & APIs — Error code & messaging:**
- The existing `ApiError.conflict(ERROR_CODES.LAST_ADMIN, message)` shape is preserved for every public path. Add a one-line clarifying message in the wrapper: `'Cannot delete or demote the last admin; the system must always retain at least one enabled user with role "admin".'`. Plaintext, hashes, and tokens are never included.
4. **Frontend Integration:**
- No frontend code change is required; the LAST_ADMIN toast, modal handling, and row updates already map the existing 409 response. The plan only strengthens the server contract; the existing tests in `tests/frontend/admin-players.pure.spec.ts` and `tests/frontend/change-password-modal.spec.ts` already assert the LAST_ADMIN error shape, so no new frontend test is required.
5. **Test Strategy (covered in `tests/backend/last-admin-invariant.spec.ts` and `tests/backend/admin-players.spec.ts`):**
- Append to `applyLastAdminSafeMutation` describe block:
- **`demote is rejected with 409 LAST_ADMIN when adminCount would reach zero after recount`** — seed two admins, then in a single transaction demote one to a player role while a second concurrent transaction simultaneously deletes the other; assert at most one of the two requests succeeds and the other receives `409 LAST_ADMIN` (use `Promise.all` with two `applyLastAdminSafeMutation` calls).
- **`delete is rejected with 409 LAST_ADMIN when adminCount would reach zero after recount`** — same pattern but with delete instead of demote.
- **`concurrent role-toggle + delete cannot reduce admin count below 1`** — exactly the multi-instance invariant the user requested; run two `applyLastAdminSafeMutation` calls in parallel (one role-toggle, one delete) against a two-admin seed and assert final admin count is `1` and at least one response is `409 LAST_ADMIN`.
- Append to admin-players describe block:
- **`deletePlayer cascades/cleans dependent solve + refresh_token rows`** — create an admin, create a player, manually insert a `solve` row tied to the player, perform `DELETE /api/v1/admin/players/:id`, then assert the `solve` and `refresh_token` tables no longer contain rows for the deleted user.
- **`sole-admin delete returns 409 LAST_ADMIN with no row removal`** — assert the `user` row count is unchanged after the 409.
- Use the existing in-memory better-sqlite3 + Nest `Test.createTestingModule` setup; no new infrastructure. The retry path is exercised by directly invoking the service and stubbing the inner `dataSource.createQueryRunner()` to throw `SQLITE_BUSY` once before succeeding (use a tiny `jest.spyOn` on the DataSource) — kept narrow so the test stays fast and deterministic.
## 4. Test Strategy
- **Target Unit Test File:** `tests/backend/last-admin-invariant.spec.ts` (service-level) and `tests/backend/admin-players.spec.ts` (HTTP-level).
- **Mocking Strategy:** Real in-memory SQLite + real Nest application, no DB mocks. The retry path uses a one-line `jest.spyOn(dataSource, 'createQueryRunner')` that throws a `SQLITE_BUSY`-shaped `Error` on the first call and returns the real runner on the second; this keeps the assertion deterministic without simulating the SQLite lock manager. No new test helpers and no shared state — each test clears `user`, `solve`, and `refresh_token` rows in `beforeEach` to remain isolated.
.kilo/plans/912.md
-129
View File
@@ -1,129 +0,0 @@
# Implementation Plan: Scoreboard Score Graph — Safe rendering under invalid/reversed event window (Job 912)
## 1. Architectural Reconnaissance
- **Codebase style & conventions:**
- Backend: NestJS 10 + TypeORM, SQLite (`backend/src/...`).
- Frontend: Angular 20 standalone components with Signals, `ChangeDetectionStrategy.OnPush`, `@if`/`@for` control flow, `signal()`/`computed()` for state.
- Pure helpers live in `frontend/src/app/features/scoreboard/scoreboard.pure.ts` and are the canonical test surface; components stay presentation-only.
- Existing `ScoreGraphComponent` uses the legacy `@Input()` + `@if/*ngIf` mix; we will not refactor it (out of scope) — only fix the broken math.
- **Data Layer:** SQLite via TypeORM. The bug lives entirely in the frontend projection. No DB migration is required. (The root cause is bad admin-entered `eventStartUtc`/`eventEndUtc` values persisted in the `setting` table — we do NOT silently rewrite them; we harden the renderer.)
- **Test Framework & Structure:**
- Jest 29 with `ts-jest` and two projects in `tests/jest.config.js` (`backend` + `frontend`).
- Frontend project uses `jsdom`, `tests/frontend/jest.setup.ts`, and `transformIgnorePatterns` allow-listing `@angular`, `rxjs`, `tslib`.
- Tests live under `tests/frontend/` (already convention) — e.g. `tests/frontend/scoreboard.pure.spec.ts` exists and follows this pattern. We will add a sibling `tests/frontend/score-graph-projection.spec.ts`.
- Single test command: `npm test` (or scoped `npm run test:frontend`).
- **Required Tools & Dependencies:** None new. Existing stack (Jest 29, jsdom, `@angular/common`, `@angular/core`) is sufficient.
## 2. Impacted Files
### To Modify
- `frontend/src/app/features/scoreboard/scoreboard.pure.ts`
- Add a pure helper `projectGraphPoints(view, series, dims)` that returns clamped SVG `points` strings for a single series. Encapsulates the X/Y math so the bug cannot recur in the component template. Pure, deterministic, fully unit-testable.
- Add a small helper `isValidEventWindow(startUtc, endUtc)` returning `true` only when both timestamps parse and `endMs > startMs`.
- `frontend/src/app/features/scoreboard/score-graph.component.ts`
- Replace inline `pointsAttr()` with a call to `projectGraphPoints(view, s, dims)` and a single `points` getter computed up front (via a `computed()` over `view`) so the bug fix is a single source of truth.
- Switch the per-series `<polyline>` binding from a method call to a precomputed `string` per series (Angular calls methods on every change-detection cycle, so hoisting into `computed()` also helps OnPush perf).
- No template/TDD-required structural changes beyond `[attr.points]="linePoints(s)"` consuming the hoisted value.
### To Create
- `tests/frontend/score-graph-projection.spec.ts`
- Unit tests for the new pure helper covering:
1. Happy path (start < end, normal window) — unchanged coordinates.
2. Reversed window (`endUtc < startUtc`) — every projected X coordinate falls inside `[padL, padL + innerW]` and no coordinate exceeds plausible SVG bounds.
3. Equal start/end (zero span) — handled gracefully (no NaN/Infinity, all points clamp to the right edge).
4. Invalid `Date` strings — points string is empty / coordinates stay finite.
## 3. Proposed Changes
### Step 1 — Pure helper in `scoreboard.pure.ts`
Add two exports:
```ts
export interface GraphProjectionDims {
width: number;
height: number;
padL: number;
padR: number;
padT: number;
padB: number;
}
export function isValidEventWindow(
startUtc: string | null,
endUtc: string | null,
): boolean {
if (!startUtc || !endUtc) return false;
const s = new Date(startUtc).getTime();
const e = new Date(endUtc).getTime();
if (!Number.isFinite(s) || !Number.isFinite(e)) return false;
return e > s;
}
export function projectGraphPoints(
view: Pick<GraphView, 'startUtc' | 'endUtc'>,
series: GraphSeries,
dims: GraphProjectionDims,
): string {
if (!isValidEventWindow(view.startUtc, view.endUtc)) return '';
const startMs = new Date(view.startUtc!).getTime();
const endMs = new Date(view.endUtc!).getTime();
const span = endMs - startMs;
const innerW = dims.width - dims.padL - dims.padR;
const innerH = dims.height - dims.padT - dims.padB;
let maxVal = 0;
for (const s of series.points) if (s.value > maxVal) maxVal = s.value;
if (maxVal <= 0) maxVal = 1;
const left = dims.padL;
const right = dims.padL + innerW;
const top = dims.padT;
const bottom = dims.padT + innerH;
return series.points
.map((p) => {
const ms = new Date(p.tUtc).getTime();
if (!Number.isFinite(ms)) return `${left.toFixed(2)},${bottom.toFixed(2)}`;
const rawX = ((ms - startMs) / span) * innerW;
const x = Math.min(right, Math.max(left, rawX));
const rawY = (p.value / maxVal) * innerH;
const y = top + Math.max(0, Math.min(innerH, innerH - rawY));
return `${x.toFixed(2)},${y.toFixed(2)}`;
})
.join(' ');
}
```
Why this fixes the bug:
- `isValidEventWindow` short-circuits the case `endUtc < startUtc` (the persisted repro). The component then renders the existing `Not started`/`No solves yet` empty branch instead of emitting absurd polylines. We do not silently mutate the persisted settings — we just refuse to render an off-canvas chart for an invalid window.
- `Math.min/Math.max` clamp surviving X coordinates into `[padL, width - padR]` so future regressions that still produce a malformed span (zero, NaN) cannot escape the SVG viewport.
- NaN guards prevent the `toFixed(2)`-on-`NaN` failure mode that previously produced strings like `40.00,292.00`.
### Step 2 — Component refactor in `score-graph.component.ts`
- Replace the `@Input() view` with a `signal`-style getter via `ngOnChanges`/`input()` semantics: keep `@Input() view: GraphView | null = null;` (existing pattern) but add a `computed` `seriesWithPoints` that maps each series to `{ ...s, pointsAttr: projectGraphPoints(view, s, dims) }`. Use an explicit `dims: GraphProjectionDims` constant built from the existing `width/height/padL/padR/padT/padB` so the magic numbers are declared once.
- Replace the `[attr.points]="pointsAttr(s)"` binding with `[attr.points]="line.pointsAttr"` and iterate over `seriesWithPoints()` instead of `view.series`. (Template keeps `*ngFor` — we are not upgrading to `@for` per job scope.)
- Keep all existing `data-testid` markers and the three empty states (`unconfigured` / `countdown` / running-with-empty-series) — the fix is a no-op for those branches.
### Step 3 — No backend / no DB changes
The persisted bad data (start > end) stays as-is. The renderer now refuses to project points outside the SVG and gracefully returns empty for the invalid window. We will NOT auto-swap start/end at the backend because that would silently rewrite admin input.
## 4. Test Strategy
- **Target Unit Test File:** `tests/frontend/score-graph-projection.spec.ts` (new). It mirrors the existing `tests/frontend/scoreboard.pure.spec.ts` style: plain Jest, no TestBed needed (pure helper), no Angular bootstrap, no DOM rendering.
- **Mocking Strategy:**
- No mocks required — the helper is a pure function over primitives and date strings.
- No HTTP, no time mocking (the helper accepts timestamps as arguments; we pin them in fixtures).
- **Tests to include (minimal, focused):**
1. `isValidEventWindow` returns `false` when `endUtc < startUtc`, `false` when equal, `false` for non-finite Date strings, `true` only when `end > start`.
2. `projectGraphPoints` with the documented repro (`startUtc=2026-07-23T00:00:00.000Z`, `endUtc=2026-07-22T00:00:00.000Z`) returns the empty string — explicitly asserting the bug-class fix.
3. `projectGraphPoints` with a normal window returns coordinates where the first point's X is `padL` and the last X is at or below `padL + innerW` (regression guard).
4. `projectGraphPoints` with `startUtc === endUtc` (zero-span edge) returns only finite, in-bounds X coordinates (no `Infinity`, no negative).
- **Negative coverage deliberately omitted:** we do not mount the Angular component (no TestBed, no fixture) — that keeps the suite fast and matches the rule "Frontend tests focus on logic, not visuals."
- **Run command:** `npm test` (executes all projects) or `npm run test:frontend` for the frontend project only. Both commands already work in this repo.
## Status
Not yet implemented. Pure helper `projectGraphPoints` / `isValidEventWindow` do not exist in `scoreboard.pure.ts`; the bug is reproducible from the current `pointsAttr()` method on line 117 of `score-graph.component.ts`.