From e4ccf0fe13b95e51722603850807cecfd49a82b8 Mon Sep 17 00:00:00 2001 From: m0rph3us1987 Date: Thu, 23 Jul 2026 08:08:50 +0000 Subject: [PATCH] AI Implementation feature(867): Admin Area Players Management (#56) --- .kilo/plans/867.md | 50 ++ .kilo/plans/912.md | 129 ----- backend/src/common/utils/password-hashing.ts | 23 + backend/src/database/database.module.ts | 4 + .../database/entities/refresh-token.entity.ts | 7 +- backend/src/database/entities/solve.entity.ts | 5 + .../1700000000700-AddUserDeleteCascades.ts | 97 ++++ .../1700000000800-AddUserLastAdminTriggers.ts | 57 +++ backend/src/modules/admin/admin.controller.ts | 58 ++- backend/src/modules/admin/admin.service.ts | 160 +++++- backend/src/modules/admin/dto/admin.dto.ts | 26 +- backend/src/modules/auth/auth.service.ts | 22 +- backend/src/modules/setup/setup.service.ts | 9 +- backend/src/modules/users/users.service.ts | 184 ++++++- frontend/src/app/app.routes.ts | 5 + .../error-notification.interceptor.ts | 3 + .../src/app/core/services/admin.service.ts | 64 ++- .../app/features/admin/admin-players.pure.ts | 62 +++ .../features/admin/admin-shell.component.ts | 2 +- .../features/admin/admin-users.component.ts | 332 ++++++++++++- .../admin/player-delete-modal.component.ts | 75 +++ .../change-password-modal.component.ts | 22 +- tests/backend/admin-players.spec.ts | 456 ++++++++++++++++++ tests/backend/last-admin-invariant.spec.ts | 252 +++++++++- tests/backend/setup-create-admin.spec.ts | 42 +- tests/frontend/admin-players.pure.spec.ts | 99 ++++ tests/frontend/change-password-modal.spec.ts | 12 + 27 files changed, 2039 insertions(+), 218 deletions(-) create mode 100644 .kilo/plans/867.md delete mode 100644 .kilo/plans/912.md create mode 100644 backend/src/common/utils/password-hashing.ts create mode 100644 backend/src/database/migrations/1700000000700-AddUserDeleteCascades.ts create mode 100644 backend/src/database/migrations/1700000000800-AddUserLastAdminTriggers.ts create mode 100644 frontend/src/app/features/admin/admin-players.pure.ts create mode 100644 frontend/src/app/features/admin/player-delete-modal.component.ts create mode 100644 tests/backend/admin-players.spec.ts create mode 100644 tests/frontend/admin-players.pure.spec.ts diff --git a/.kilo/plans/867.md b/.kilo/plans/867.md new file mode 100644 index 0000000..fd5901b --- /dev/null +++ b/.kilo/plans/867.md @@ -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. diff --git a/.kilo/plans/912.md b/.kilo/plans/912.md deleted file mode 100644 index 2575867..0000000 --- a/.kilo/plans/912.md +++ /dev/null @@ -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 `` 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, - 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`. \ No newline at end of file diff --git a/backend/src/common/utils/password-hashing.ts b/backend/src/common/utils/password-hashing.ts new file mode 100644 index 0000000..a383bfe --- /dev/null +++ b/backend/src/common/utils/password-hashing.ts @@ -0,0 +1,23 @@ +import { ConfigService } from '@nestjs/config'; +import * as argon2 from 'argon2'; + +/** + * Centralized Argon2id hashing for every password-set flow in the system: + * + * - player self-registration + * - first-admin bootstrap + * - admin user creation + * - self change-password + * - admin-initiated password reset + * + * Using one helper keeps the configured cost parameters consistent across + * flows so the admin password policy behaves predictably. + */ +export async function hashPassword(password: string, config: ConfigService): Promise { + return argon2.hash(password, { + type: argon2.argon2id, + memoryCost: config.get('ARGON2_MEMORY_COST'), + timeCost: config.get('ARGON2_TIME_COST'), + parallelism: config.get('ARGON2_PARALLELISM'), + }); +} diff --git a/backend/src/database/database.module.ts b/backend/src/database/database.module.ts index 7b855f1..7e393b7 100644 --- a/backend/src/database/database.module.ts +++ b/backend/src/database/database.module.ts @@ -18,6 +18,8 @@ import { UpdateSystemCategoryKeys1700000000300 } from './migrations/170000000030 import { RepairCategorySchemaAndSystemCategories1700000000400 } from './migrations/1700000000400-RepairCategorySchemaAndSystemCategories'; import { UpgradeChallengeAdminSchema1700000000500 } from './migrations/1700000000500-UpgradeChallengeAdminSchema'; import { SeedSampleChallenges1700000000600 } from './migrations/1700000000600-SeedSampleChallenges'; +import { AddUserDeleteCascades1700000000700 } from './migrations/1700000000700-AddUserDeleteCascades'; +import { AddUserLastAdminTriggers1700000000800 } from './migrations/1700000000800-AddUserLastAdminTriggers'; import { DatabaseInitService } from './database-init.service'; const ENTITIES = [ @@ -39,6 +41,8 @@ const MIGRATIONS = [ RepairCategorySchemaAndSystemCategories1700000000400, UpgradeChallengeAdminSchema1700000000500, SeedSampleChallenges1700000000600, + AddUserDeleteCascades1700000000700, + AddUserLastAdminTriggers1700000000800, ]; @Global() diff --git a/backend/src/database/entities/refresh-token.entity.ts b/backend/src/database/entities/refresh-token.entity.ts index 6d65db9..fe585d5 100644 --- a/backend/src/database/entities/refresh-token.entity.ts +++ b/backend/src/database/entities/refresh-token.entity.ts @@ -1,4 +1,5 @@ -import { Entity, PrimaryColumn, Column, Index } from 'typeorm'; +import { Entity, PrimaryColumn, Column, Index, ManyToOne, JoinColumn } from 'typeorm'; +import { UserEntity } from './user.entity'; @Entity('refresh_token') export class RefreshTokenEntity { @@ -9,6 +10,10 @@ export class RefreshTokenEntity { @Column('text', { name: 'user_id' }) userId!: string; + @ManyToOne(() => UserEntity, { onDelete: 'CASCADE' }) + @JoinColumn({ name: 'user_id' }) + user?: UserEntity; + @Column('text', { name: 'token_hash', unique: true }) tokenHash!: string; diff --git a/backend/src/database/entities/solve.entity.ts b/backend/src/database/entities/solve.entity.ts index 394b7d4..2bb8fd3 100644 --- a/backend/src/database/entities/solve.entity.ts +++ b/backend/src/database/entities/solve.entity.ts @@ -8,6 +8,7 @@ import { JoinColumn, } from 'typeorm'; import { ChallengeEntity } from './challenge.entity'; +import { UserEntity } from './user.entity'; @Entity('solve') @Unique('uq_solve_challenge_user', ['challengeId', 'userId']) @@ -27,6 +28,10 @@ export class SolveEntity { @Column('text', { name: 'user_id' }) userId!: string; + @ManyToOne(() => UserEntity, { onDelete: 'CASCADE' }) + @JoinColumn({ name: 'user_id' }) + user?: UserEntity; + @Column('text', { name: 'solved_at' }) solvedAt!: string; diff --git a/backend/src/database/migrations/1700000000700-AddUserDeleteCascades.ts b/backend/src/database/migrations/1700000000700-AddUserDeleteCascades.ts new file mode 100644 index 0000000..b59eaee --- /dev/null +++ b/backend/src/database/migrations/1700000000700-AddUserDeleteCascades.ts @@ -0,0 +1,97 @@ +import { MigrationInterface, QueryRunner } from 'typeorm'; + +/** + * Updates foreign-key cascade behaviour so user deletion cleans up dependent + * rows without leaving orphaned solves or refresh tokens: + * + * - `solve.user_id` and `refresh_token.user_id` both gain + * `ON DELETE CASCADE`. + * + * Strategy mirrors UpgradeChallengeAdminSchema1700000000500: + * - SQLite cannot alter FK actions in place, so we rebuild the affected + * tables atomically with PRAGMA foreign_keys=OFF, preserving legacy + * data via INSERT...SELECT. + * - Forward-only. + */ +export class AddUserDeleteCascades1700000000700 implements MigrationInterface { + name = 'AddUserDeleteCascades1700000000700'; + + public async up(queryRunner: QueryRunner): Promise { + await queryRunner.query(`PRAGMA foreign_keys = OFF;`); + + await queryRunner.query(`DROP INDEX IF EXISTS "idx_solve_user";`); + await queryRunner.query(`DROP INDEX IF EXISTS "uq_solve_challenge_user";`); + await queryRunner.query(`DROP INDEX IF EXISTS "idx_solve_challenge";`); + await queryRunner.query(`DROP INDEX IF EXISTS "idx_refresh_token_user";`); + + await queryRunner.query(`ALTER TABLE "solve" RENAME TO "_solve_old";`); + await queryRunner.query(`ALTER TABLE "refresh_token" RENAME TO "_refresh_token_old";`); + + await queryRunner.query(` + CREATE TABLE "solve" ( + "id" TEXT PRIMARY KEY, + "challenge_id" TEXT NOT NULL, + "user_id" TEXT NOT NULL, + "solved_at" TEXT NOT NULL, + "points_awarded" INTEGER NOT NULL DEFAULT 0, + "base_points" INTEGER NOT NULL DEFAULT 0, + "rank_bonus" INTEGER NOT NULL DEFAULT 0, + "is_first" INTEGER NOT NULL DEFAULT 0, + "is_second" INTEGER NOT NULL DEFAULT 0, + "is_third" INTEGER NOT NULL DEFAULT 0, + CONSTRAINT "fk_solve_challenge" FOREIGN KEY ("challenge_id") REFERENCES "challenge"("id") ON DELETE CASCADE, + CONSTRAINT "fk_solve_user" FOREIGN KEY ("user_id") REFERENCES "user"("id") ON DELETE CASCADE + ); + `); + + await queryRunner.query(` + INSERT INTO "solve" ( + "id","challenge_id","user_id","solved_at","points_awarded","base_points","rank_bonus","is_first","is_second","is_third" + ) + SELECT + "id","challenge_id","user_id","solved_at", + COALESCE("points_awarded",0), + COALESCE("base_points",0), + COALESCE("rank_bonus",0), + COALESCE("is_first",0), + COALESCE("is_second",0), + COALESCE("is_third",0) + FROM "_solve_old"; + `); + await queryRunner.query(`DROP TABLE "_solve_old";`); + + await queryRunner.query(`CREATE UNIQUE INDEX IF NOT EXISTS "uq_solve_challenge_user" ON "solve"("challenge_id","user_id");`); + await queryRunner.query(`CREATE INDEX IF NOT EXISTS "idx_solve_user" ON "solve"("user_id");`); + await queryRunner.query(`CREATE INDEX IF NOT EXISTS "idx_solve_challenge" ON "solve"("challenge_id");`); + + await queryRunner.query(` + CREATE TABLE "refresh_token" ( + "id" TEXT PRIMARY KEY, + "user_id" TEXT NOT NULL, + "token_hash" TEXT NOT NULL UNIQUE, + "issued_at" TEXT NOT NULL, + "expires_at" TEXT NOT NULL, + "revoked_at" TEXT, + CONSTRAINT "fk_refresh_token_user" FOREIGN KEY ("user_id") REFERENCES "user"("id") ON DELETE CASCADE + ); + `); + + await queryRunner.query(` + INSERT INTO "refresh_token" ( + "id","user_id","token_hash","issued_at","expires_at","revoked_at" + ) + SELECT "id","user_id","token_hash","issued_at","expires_at","revoked_at" + FROM "_refresh_token_old"; + `); + await queryRunner.query(`DROP TABLE "_refresh_token_old";`); + + await queryRunner.query(`CREATE INDEX IF NOT EXISTS "idx_refresh_token_user" ON "refresh_token"("user_id");`); + + await queryRunner.query(`PRAGMA foreign_keys = ON;`); + await queryRunner.query(`PRAGMA foreign_key_check;`); + } + + public async down(): Promise { + // Forward-only. + } +} diff --git a/backend/src/database/migrations/1700000000800-AddUserLastAdminTriggers.ts b/backend/src/database/migrations/1700000000800-AddUserLastAdminTriggers.ts new file mode 100644 index 0000000..8e6b52e --- /dev/null +++ b/backend/src/database/migrations/1700000000800-AddUserLastAdminTriggers.ts @@ -0,0 +1,57 @@ +import { MigrationInterface, QueryRunner } from 'typeorm'; + +/** + * Deployment-wide safety net for the "always retain at least one admin" + * invariant. Even if a future code path bypasses + * `UsersService.applyLastAdminSafeMutation`, the database itself aborts + * the offending UPDATE/DELETE with a clear `LAST_ADMIN` error message. + * + * Triggers fire before the row mutation, so the offending statement never + * commits. The service-level wrapper still rolls back with 409 LAST_ADMIN + * in normal operation; the triggers are the belt-and-braces guard for + * cross-instance / out-of-band mistakes. + * + * Forward-only. + */ +export class AddUserLastAdminTriggers1700000000800 implements MigrationInterface { + name = 'AddUserLastAdminTriggers1700000000800'; + + public async up(queryRunner: QueryRunner): Promise { + // UPDATE trigger: blocks any role mutation that would leave the system + // with zero admins. We use WHEN to short-circuit so the trigger body + // only fires when (a) the old role was admin, (b) the new role is no + // longer admin, and (c) no other admin row exists. + await queryRunner.query(` + CREATE TRIGGER IF NOT EXISTS "trg_user_last_admin_update" + BEFORE UPDATE OF "role" ON "user" + FOR EACH ROW + WHEN OLD."role" = 'admin' AND NEW."role" <> 'admin' + AND NOT EXISTS ( + SELECT 1 FROM "user" AS "other" + WHERE "other"."role" = 'admin' AND "other"."id" <> OLD."id" + ) + BEGIN + SELECT RAISE(ABORT, 'LAST_ADMIN'); + END; + `); + + // DELETE trigger: blocks deletion of the only remaining admin row. + await queryRunner.query(` + CREATE TRIGGER IF NOT EXISTS "trg_user_last_admin_delete" + BEFORE DELETE ON "user" + FOR EACH ROW + WHEN OLD."role" = 'admin' + AND NOT EXISTS ( + SELECT 1 FROM "user" AS "other" + WHERE "other"."role" = 'admin' AND "other"."id" <> OLD."id" + ) + BEGIN + SELECT RAISE(ABORT, 'LAST_ADMIN'); + END; + `); + } + + public async down(): Promise { + // Forward-only. + } +} diff --git a/backend/src/modules/admin/admin.controller.ts b/backend/src/modules/admin/admin.controller.ts index d88b3dd..e3af9b0 100644 --- a/backend/src/modules/admin/admin.controller.ts +++ b/backend/src/modules/admin/admin.controller.ts @@ -3,6 +3,7 @@ import { Controller, Delete, Get, + HttpCode, Param, Patch, Post, @@ -15,8 +16,10 @@ import { AdminGuard } from '../../common/guards/admin.guard'; import { Roles } from '../../common/decorators/roles.decorator'; import { ZodValidationPipe } from '../../common/pipes/zod-validation.pipe'; import { + AdminResetPasswordDtoSchema, CreateUserDtoSchema, ListUsersQueryDtoSchema, + UpdatePlayerStatusDtoSchema, UpdateUserRoleDtoSchema, UserIdParamDtoSchema, } from './dto/admin.dto'; @@ -37,8 +40,55 @@ import { AdminService } from './admin.service'; export class AdminController { constructor(private readonly admin: AdminService) {} + @Get('players') + @ApiOperation({ summary: 'List all registered users (Players page)' }) + listPlayers( + @Query(new ZodValidationPipe(ListUsersQueryDtoSchema)) + query: { limit?: number; cursor?: string; role?: UserRole }, + ) { + return this.admin.listPlayers(query); + } + + @Patch('players/:id/role') + @ApiOperation({ summary: 'Toggle a user role between Admin and Player' }) + updatePlayerRole( + @Param(new ZodValidationPipe(UserIdParamDtoSchema)) params: { id: string }, + @Body(new ZodValidationPipe(UpdateUserRoleDtoSchema)) body: { role: UserRole }, + ) { + return this.admin.updatePlayerRole(params.id, body.role); + } + + @Patch('players/:id/status') + @ApiOperation({ summary: 'Toggle a user enabled/disabled status' }) + updatePlayerStatus( + @Param(new ZodValidationPipe(UserIdParamDtoSchema)) params: { id: string }, + @Body(new ZodValidationPipe(UpdatePlayerStatusDtoSchema)) body: { enabled: boolean }, + ) { + return this.admin.updatePlayerStatus(params.id, body.enabled); + } + + @Patch('players/:id/password') + @HttpCode(204) + @ApiOperation({ summary: 'Admin-initiated password reset for a user' }) + async resetPlayerPassword( + @Param(new ZodValidationPipe(UserIdParamDtoSchema)) params: { id: string }, + @Body(new ZodValidationPipe(AdminResetPasswordDtoSchema)) + body: { newPassword: string; confirmPassword: string }, + ): Promise { + await this.admin.resetPlayerPassword(params.id, body.newPassword); + } + + @Delete('players/:id') + @HttpCode(204) + @ApiOperation({ summary: 'Delete a user (cascades solves + refresh tokens)' }) + async deletePlayer( + @Param(new ZodValidationPipe(UserIdParamDtoSchema)) params: { id: string }, + ): Promise { + await this.admin.deletePlayer(params.id); + } + @Get('users') - @ApiOperation({ summary: 'List all users (admin only)' }) + @ApiOperation({ summary: 'List all users (admin only, legacy alias)' }) list( @Query(new ZodValidationPipe(ListUsersQueryDtoSchema)) query: { limit?: number; cursor?: string; role?: UserRole }, @@ -47,7 +97,7 @@ export class AdminController { } @Patch('users/:id') - @ApiOperation({ summary: 'Update a user role (admin only)' }) + @ApiOperation({ summary: 'Update a user role (admin only, legacy alias)' }) update( @Param(new ZodValidationPipe(UserIdParamDtoSchema)) params: { id: string }, @Body(new ZodValidationPipe(UpdateUserRoleDtoSchema)) body: { role: UserRole }, @@ -56,7 +106,7 @@ export class AdminController { } @Delete('users/:id') - @ApiOperation({ summary: 'Delete a user (admin only)' }) + @ApiOperation({ summary: 'Delete a user (admin only, legacy alias)' }) async remove( @Param(new ZodValidationPipe(UserIdParamDtoSchema)) params: { id: string }, ): Promise { @@ -70,4 +120,4 @@ export class AdminController { ) { return this.admin.createUser(body.username, body.password, body.role); } -} \ No newline at end of file +} diff --git a/backend/src/modules/admin/admin.service.ts b/backend/src/modules/admin/admin.service.ts index a9cebec..fdfbd75 100644 --- a/backend/src/modules/admin/admin.service.ts +++ b/backend/src/modules/admin/admin.service.ts @@ -1,29 +1,56 @@ -import { Injectable } from '@nestjs/common'; -import { DataSource } from 'typeorm'; +import { Injectable, Logger } from '@nestjs/common'; +import { DataSource, EntityManager } from 'typeorm'; import { v4 as uuid } from 'uuid'; -import * as argon2 from 'argon2'; -import { UserEntity, UserRole } from '../../database/entities/user.entity'; +import { UserEntity, UserRole, UserStatus } from '../../database/entities/user.entity'; +import { RefreshTokenEntity } from '../../database/entities/refresh-token.entity'; import { UsersService } from '../users/users.service'; import { ApiError } from '../../common/errors/api-error'; import { ERROR_CODES } from '../../common/errors/error-codes'; import { validatePassword } from '../../common/utils/password-policy'; +import { hashPassword } from '../../common/utils/password-hashing'; import { ConfigService } from '@nestjs/config'; +import { AdminPlayerView, ListUsersQueryDto } from './dto/admin.dto'; -export interface ListUsersQuery { - limit?: number; - cursor?: string; - role?: UserRole; +/** + * Every table that has a `userId` column and must be cleaned up when the + * parent `user` row is deleted. Iterated by `deleteUserOwnedRows` so future + * user-owned tables (e.g. comments, notifications) only need to be appended + * here. The ON DELETE CASCADE migration that backs this list is the + * deployment-wide safety net; the imperative delete below is what the + * admin delete path requires. + */ +const USER_OWNED_TABLES: { tableName: string; userIdColumn: string }[] = [ + { tableName: 'solve', userIdColumn: 'user_id' }, + { tableName: 'refresh_token', userIdColumn: 'user_id' }, +]; + +async function deleteUserOwnedRows(manager: EntityManager, userId: string): Promise> { + const counts: Record = {}; + for (const { tableName, userIdColumn } of USER_OWNED_TABLES) { + const res = await manager.query( + `DELETE FROM "${tableName}" WHERE "${userIdColumn}" = ?`, + [userId], + ); + counts[tableName] = Number(res?.changes ?? res?.affected ?? 0); + } + return counts; } @Injectable() export class AdminService { + private readonly logger = new Logger(AdminService.name); + constructor( private readonly usersService: UsersService, private readonly dataSource: DataSource, private readonly config: ConfigService, ) {} - async listUsers(query: ListUsersQuery = {}): Promise { + async listUsers(query: ListUsersQueryDto = {}): Promise { + return this.listPlayers(query); + } + + async listPlayers(query: ListUsersQueryDto = {}): Promise { const limit = Math.min(Math.max(query.limit ?? 50, 1), 200); const repo = this.dataSource.getRepository(UserEntity); const qb = repo @@ -34,37 +61,113 @@ export class AdminService { if (query.role) qb.andWhere('u.role = :role', { role: query.role }); if (query.cursor) qb.andWhere('u.id > :cursor', { cursor: query.cursor }); const rows = await qb.getMany(); - return rows.slice(0, limit); + return rows.slice(0, limit).map(toPlayerView); } - async updateUserRole(targetId: string, role: UserRole): Promise { + async updateUserRole(targetId: string, role: UserRole): Promise { + return this.updatePlayerRole(targetId, role); + } + + async updatePlayerRole(targetId: string, role: UserRole): Promise { if (role !== 'admin' && role !== 'player') { throw ApiError.validation('Invalid role'); } + const updated = await this.usersService.applyLastAdminSafeMutation(targetId, { + demotesOrDelete: true, + updatedRole: role, + mutate: async (manager, target) => { + target.role = role; + await manager.save(target); + return toPlayerView(target); + }, + }); + return updated; + } + + async updatePlayerStatus(targetId: string, enabled: boolean): Promise { + const target = await this.usersService.findById(targetId); + if (!target) throw ApiError.notFound('User not found'); + const newStatus: UserStatus = enabled ? 'enabled' : 'disabled'; return this.dataSource.transaction(async (manager) => { - await this.usersService.enforceLastAdminInvariant(manager, targetId, role); const user = await manager.findOne(UserEntity, { where: { id: targetId } }); if (!user) throw ApiError.notFound('User not found'); - user.role = role; + user.status = newStatus; await manager.save(user); - return user; + if (!enabled) { + await manager + .createQueryBuilder() + .update(RefreshTokenEntity) + .set({ revokedAt: new Date().toISOString() }) + .where('userId = :userId', { userId: user.id }) + .andWhere('revokedAt IS NULL') + .execute(); + } + return toPlayerView(user); + }); + } + + async resetPlayerPassword(targetId: string, newPassword: string): Promise { + if (!newPassword) { + throw ApiError.validation('New password is required'); + } + validatePassword(newPassword, this.config); + const target = await this.usersService.findById(targetId); + if (!target) throw ApiError.notFound('User not found'); + const hash = await hashPassword(newPassword, this.config); + await this.dataSource.transaction(async (manager) => { + const user = await manager.findOne(UserEntity, { where: { id: targetId } }); + if (!user) throw ApiError.notFound('User not found'); + user.passwordHash = hash; + await manager.save(user); + await manager + .createQueryBuilder() + .update(RefreshTokenEntity) + .set({ revokedAt: new Date().toISOString() }) + .where('userId = :userId', { userId: user.id }) + .andWhere('revokedAt IS NULL') + .execute(); }); } async deleteUser(targetId: string): Promise { - return this.dataSource.transaction(async (manager) => { - await this.usersService.enforceLastAdminInvariant(manager, targetId); - const res = await manager.delete(UserEntity, { id: targetId }); - if (!res.affected) throw ApiError.notFound('User not found'); + return this.deletePlayer(targetId); + } + + async deletePlayer(targetId: string): Promise { + await this.usersService.applyLastAdminSafeMutation(targetId, { + demotesOrDelete: true, + mutate: async (manager) => { + // Imperative dependent cleanup in the SAME transaction. The + // ON DELETE CASCADE migration is a safety net, but the explicit + // deletes (a) surface the affected row counts to the application, + // (b) keep deletes observable for future audit logging, and + // (c) ensure that the post-mutation admin count is the only + // invariant the caller still needs to verify. + const cleanedCounts = await deleteUserOwnedRows(manager, targetId); + + const res = await manager.delete(UserEntity, { id: targetId }); + if (!res.affected) { + throw ApiError.notFound('User not found'); + } + + // Note per-table cleanup counts; never log password hashes, token + // hashes, or the plaintext password. + this.logger.log( + `Deleted user ${targetId}; dependent rows: ${Object.entries(cleanedCounts) + .map(([t, n]) => `${t}=${n}`) + .join(', ') || 'none'}`, + ); + return null; + }, }); } - async createUser(username: string, password: string, role: UserRole): Promise { + async createUser(username: string, password: string, role: UserRole): Promise { validatePassword(password, this.config); - return this.dataSource.transaction(async (manager) => { + const created = await this.dataSource.transaction(async (manager) => { const existing = await manager.findOne(UserEntity, { where: { username } }); if (existing) throw ApiError.conflict(ERROR_CODES.CONFLICT, 'Username already taken'); - const hash = await argon2.hash(password, { type: argon2.argon2id }); + const hash = await hashPassword(password, this.config); const user = manager.create(UserEntity, { id: uuid(), username, @@ -73,7 +176,18 @@ export class AdminService { status: 'enabled', }); await manager.save(user); - return user; + return toPlayerView(user); }); + return created; } -} \ No newline at end of file +} + +export function toPlayerView(user: UserEntity): AdminPlayerView { + return { + id: user.id, + username: user.username, + role: user.role, + status: user.status, + createdAt: user.createdAt, + }; +} diff --git a/backend/src/modules/admin/dto/admin.dto.ts b/backend/src/modules/admin/dto/admin.dto.ts index 22c6dd6..6cd5799 100644 --- a/backend/src/modules/admin/dto/admin.dto.ts +++ b/backend/src/modules/admin/dto/admin.dto.ts @@ -12,6 +12,22 @@ export const UpdateUserRoleDtoSchema = z.object({ }); export type UpdateUserRoleDto = z.infer; +export const UpdatePlayerStatusDtoSchema = z.object({ + enabled: z.boolean(), +}); +export type UpdatePlayerStatusDto = z.infer; + +export const AdminResetPasswordDtoSchema = z + .object({ + newPassword: z.string().min(1).max(256), + confirmPassword: z.string().min(1).max(256), + }) + .refine((d) => d.newPassword === d.confirmPassword, { + path: ['confirmPassword'], + message: 'Passwords do not match', + }); +export type AdminResetPasswordDto = z.infer; + export const UserIdParamDtoSchema = z.object({ id: z.string().uuid(), }); @@ -22,4 +38,12 @@ export const ListUsersQueryDtoSchema = z.object({ cursor: z.string().uuid().optional(), role: z.enum(['admin', 'player']).optional(), }); -export type ListUsersQueryDto = z.infer; \ No newline at end of file +export type ListUsersQueryDto = z.infer; + +export interface AdminPlayerView { + id: string; + username: string; + role: 'admin' | 'player'; + status: 'enabled' | 'disabled'; + createdAt: string; +} diff --git a/backend/src/modules/auth/auth.service.ts b/backend/src/modules/auth/auth.service.ts index 249e2a1..a86ebee 100644 --- a/backend/src/modules/auth/auth.service.ts +++ b/backend/src/modules/auth/auth.service.ts @@ -20,6 +20,7 @@ import { LoginBackoffService } from '../../common/services/login-backoff.service import { RegistrationRateLimitService } from '../../common/services/registration-rate-limit.service'; import { ChangePasswordDto, LoginDto, LoginResponse, RegisterDto } from './dto/auth.dto'; import { validatePassword } from '../../common/utils/password-policy'; +import { hashPassword } from '../../common/utils/password-hashing'; import { SettingsService } from '../settings/settings.module'; import { SETTINGS_KEYS } from '../../config/env.schema'; import { UsersRankService } from '../users/users-rank.service'; @@ -117,12 +118,7 @@ export class AuthService implements OnModuleInit { const existing = await manager.findOne(UserEntity, { where: { username } }); if (existing) throw new ApiError(ERROR_CODES.CONFLICT, 'Username already taken', 409); - const passwordHash = await argon2.hash(password, { - type: argon2.argon2id, - memoryCost: this.config.get('ARGON2_MEMORY_COST'), - timeCost: this.config.get('ARGON2_TIME_COST'), - parallelism: this.config.get('ARGON2_PARALLELISM'), - }); + const passwordHash = await hashPassword(password, this.config); const user = manager.create(UserEntity, { id: uuid(), username, @@ -162,12 +158,7 @@ export class AuthService implements OnModuleInit { throw new ApiError(ERROR_CODES.USERNAME_TAKEN, 'Username already taken', 409); } - const passwordHash = await argon2.hash(dto.password, { - type: argon2.argon2id, - memoryCost: this.config.get('ARGON2_MEMORY_COST'), - timeCost: this.config.get('ARGON2_TIME_COST'), - parallelism: this.config.get('ARGON2_PARALLELISM'), - }); + const passwordHash = await hashPassword(dto.password, this.config); const user = manager.create(UserEntity, { id: uuid(), username: dto.username, @@ -220,12 +211,7 @@ export class AuthService implements OnModuleInit { throw e; } - user.passwordHash = await argon2.hash(dto.newPassword, { - type: argon2.argon2id, - memoryCost: this.config.get('ARGON2_MEMORY_COST'), - timeCost: this.config.get('ARGON2_TIME_COST'), - parallelism: this.config.get('ARGON2_PARALLELISM'), - }); + user.passwordHash = await hashPassword(dto.newPassword, this.config); await manager.save(user); // Invalidate all existing refresh tokens for this user. diff --git a/backend/src/modules/setup/setup.service.ts b/backend/src/modules/setup/setup.service.ts index 98c39b1..a3d84e6 100644 --- a/backend/src/modules/setup/setup.service.ts +++ b/backend/src/modules/setup/setup.service.ts @@ -3,13 +3,13 @@ import { ConfigService } from '@nestjs/config'; import { InjectRepository } from '@nestjs/typeorm'; import { JwtService } from '@nestjs/jwt'; import { DataSource, Repository } from 'typeorm'; -import * as argon2 from 'argon2'; import { v4 as uuid } from 'uuid'; import { UserEntity } from '../../database/entities/user.entity'; import { RefreshTokenEntity } from '../../database/entities/refresh-token.entity'; import { ApiError } from '../../common/errors/api-error'; import { ERROR_CODES } from '../../common/errors/error-codes'; import { validatePassword } from '../../common/utils/password-policy'; +import { hashPassword } from '../../common/utils/password-hashing'; import { RegistrationRateLimitService } from '../../common/services/registration-rate-limit.service'; import { AuthService } from '../auth/auth.service'; import type { LoginResponse } from '../auth/dto/auth.dto'; @@ -68,12 +68,7 @@ export class SetupService implements OnModuleInit { throw ApiError.conflict(ERROR_CODES.USERNAME_TAKEN, 'Username already exists'); } - const passwordHash = await argon2.hash(password, { - type: argon2.argon2id, - memoryCost: this.config.get('ARGON2_MEMORY_COST'), - timeCost: this.config.get('ARGON2_TIME_COST'), - parallelism: this.config.get('ARGON2_PARALLELISM'), - }); + const passwordHash = await hashPassword(password, this.config); const user = manager.create(UserEntity, { id: uuid(), diff --git a/backend/src/modules/users/users.service.ts b/backend/src/modules/users/users.service.ts index 97e4cc7..4816a58 100644 --- a/backend/src/modules/users/users.service.ts +++ b/backend/src/modules/users/users.service.ts @@ -1,15 +1,45 @@ -import { Injectable } from '@nestjs/common'; +import { Injectable, Logger } from '@nestjs/common'; import { InjectRepository } from '@nestjs/typeorm'; -import { EntityManager, Repository } from 'typeorm'; +import { DataSource, EntityManager, Repository } from 'typeorm'; import { UserEntity, UserRole } from '../../database/entities/user.entity'; import { ApiError } from '../../common/errors/api-error'; import { ERROR_CODES } from '../../common/errors/error-codes'; type UserRepoLike = Repository | EntityManager; +export interface LastAdminSafeMutation { + /** + * True when the mutation removes admin powers from a currently-admin + * user. When false, the invariant is bypassed (useful for non-destructive + * updates). + */ + demotesOrDelete: boolean; + /** + * Optional explicit role after the mutation. When `undefined`, the wrapper + * infers the new role from the post-mutation row (returning null after a + * delete). + */ + updatedRole?: UserRole; + mutate: (manager: EntityManager, target: UserEntity) => Promise; + onSuccess?: (manager: EntityManager, target: UserEntity, result: T) => Promise; +} + +/** Maximum number of times the wrapper retries on a transient lock conflict. */ +const LOCK_RETRY_MAX = 5; +/** Initial backoff in ms; doubled on every retry. */ +const LOCK_RETRY_BASE_MS = 10; + +const LAST_ADMIN_GUARD_MESSAGE = + 'Cannot delete or demote the last admin; the system must always retain at least one enabled user with role "admin".'; + @Injectable() export class UsersService { - constructor(@InjectRepository(UserEntity) private readonly repo: Repository) {} + private readonly logger = new Logger(UsersService.name); + + constructor( + @InjectRepository(UserEntity) private readonly repo: Repository, + private readonly dataSource: DataSource, + ) {} countAdmins(manager?: EntityManager): Promise { return this.getRepo(manager).count({ where: { role: 'admin' } }); @@ -24,9 +54,128 @@ export class UsersService { } /** - * Verify, inside an open transaction, that demoting/deleting the given user - * would not leave the system with zero admins. Throws ApiError(LAST_ADMIN) - * if so. Must be invoked from a `dataSource.transaction(...)` callback. + * Apply a user-role or user-deletion mutation atomically so the + * admin-count precondition, the mutation, and the postcondition all see + * a consistent database snapshot. + * + * Concurrency model: + * - Postgres / MySQL: `pessimistic_write` is a real + * `SELECT ... FOR UPDATE` on the target user and on every admin row, + * serializing the count → mutate → recount sequence. + * - SQLite (better-sqlite3): `pessimistic_write` is a no-op; the + * database-level write lock is acquired at the first write inside the + * transaction. The accompanying `trg_user_last_admin_*` triggers + * raise `SQLITE_CONSTRAINT_TRIGGER` / "LAST_ADMIN" if any path ever + * bypasses the service wrapper, so the system can never commit a + * state with zero admins. + * + * Transient `SQLITE_BUSY` / TypeORM serialization failures are retried + * with exponential backoff. If retries are exhausted, the wrapper raises + * the existing 409 LAST_ADMIN contract so callers always see a structured + * error. + */ + async applyLastAdminSafeMutation( + targetId: string, + spec: LastAdminSafeMutation, + ): Promise { + let lastError: unknown = undefined; + for (let attempt = 0; attempt < LOCK_RETRY_MAX; attempt += 1) { + if (attempt > 0) { + const delay = LOCK_RETRY_BASE_MS * 2 ** (attempt - 1); + await new Promise((resolve) => setTimeout(resolve, delay)); + } + try { + return await this.runSafeMutation(targetId, spec); + } catch (e) { + if (!isLockConflict(e)) { + throw e; + } + lastError = e; + this.logger.warn( + `applyLastAdminSafeMutation lock conflict on attempt ${attempt + 1}/${LOCK_RETRY_MAX}: ${(e as Error).message}`, + ); + } + } + this.logger.error( + `applyLastAdminSafeMutation exhausted ${LOCK_RETRY_MAX} lock retries; rejecting with LAST_ADMIN`, + ); + throw ApiError.conflict(ERROR_CODES.LAST_ADMIN, LAST_ADMIN_GUARD_MESSAGE); + } + + private async runSafeMutation( + targetId: string, + spec: LastAdminSafeMutation, + ): Promise { + return this.dataSource.transaction('SERIALIZABLE', async (manager) => { + const repo = manager.getRepository(UserEntity); + + const target = await this.lockTargetUser(repo, targetId); + if (!target) throw ApiError.notFound('User not found'); + + const destructive = spec.demotesOrDelete && target.role === 'admin'; + + if (destructive) { + await this.lockAdminRows(repo); + const beforeCount = await repo.count({ where: { role: 'admin' } }); + if (beforeCount <= 1) { + throw ApiError.conflict(ERROR_CODES.LAST_ADMIN, LAST_ADMIN_GUARD_MESSAGE); + } + } + + const result = await spec.mutate(manager, target); + + if (destructive) { + const newRole = spec.updatedRole + ?? (await repo.findOne({ where: { id: targetId } }))?.role; + if (newRole !== 'admin') { + const afterCount = await repo.count({ where: { role: 'admin' } }); + if (afterCount <= 0) { + throw ApiError.conflict(ERROR_CODES.LAST_ADMIN, LAST_ADMIN_GUARD_MESSAGE); + } + } + } + + if (spec.onSuccess) { + await spec.onSuccess(manager, target, result); + } + + return result; + }); + } + + private supportsRowLock(): boolean { + const driverType = (this.dataSource.options as { type?: string } | undefined)?.type; + return driverType !== 'better-sqlite3' && driverType !== 'sqlite' && driverType !== 'sqlite3'; + } + + private async lockTargetUser(repo: Repository, targetId: string): Promise { + if (!this.supportsRowLock()) { + return repo.findOne({ where: { id: targetId } }); + } + const row = await repo + .createQueryBuilder('u') + .setLock('pessimistic_write') + .where('u.id = :id', { id: targetId }) + .getOne(); + return row ?? null; + } + + private async lockAdminRows(repo: Repository): Promise { + if (!this.supportsRowLock()) { + return; + } + await repo + .createQueryBuilder('u') + .setLock('pessimistic_write') + .where('u.role = :role', { role: 'admin' }) + .getMany(); + } + + /** + * Backwards-compatible preflight helper retained for callers that only + * need a fast non-transactional check. The transactional safe-mutation + * path supersedes it for last-admin-sensitive flows; non-admin-sensitive + * callers can still rely on this to surface 404s early. */ async enforceLastAdminInvariant(manager: EntityManager, targetId: string, nextRole?: UserRole): Promise { const repo = manager.getRepository(UserEntity); @@ -39,11 +188,30 @@ export class UsersService { const adminCount = await repo.count({ where: { role: 'admin' } }); if (adminCount <= 1) { - throw ApiError.conflict(ERROR_CODES.LAST_ADMIN, 'Cannot delete or demote the last admin'); + throw ApiError.conflict(ERROR_CODES.LAST_ADMIN, LAST_ADMIN_GUARD_MESSAGE); } } private getRepo(manager?: EntityManager): Repository { return manager ? manager.getRepository(UserEntity) : this.repo; } -} \ No newline at end of file +} + +/** + * Identify transient lock conflicts so the wrapper can retry them. SQLite + * reports `SQLITE_BUSY` (errno 5) when another writer holds the database + * lock; TypeORM surfaces the same error code as `code` or wraps it in a + * `QueryFailedError` whose `driverError.code` is `'SQLITE_BUSY'`. Generic + * constraint failures and other 500-class errors are NOT treated as + * transient. + */ +function isLockConflict(e: unknown): boolean { + if (!e || typeof e !== 'object') return false; + const err = e as { code?: string; message?: string; driverError?: { code?: string } }; + if (err.code === 'SQLITE_BUSY') return true; + if (err.code === 'SQLITE_BUSY_SNAPSHOT') return true; + if (err.driverError?.code === 'SQLITE_BUSY') return true; + if (err.driverError?.code === 'SQLITE_BUSY_SNAPSHOT') return true; + if (typeof err.message === 'string' && err.message.includes('SQLITE_BUSY')) return true; + return false; +} diff --git a/frontend/src/app/app.routes.ts b/frontend/src/app/app.routes.ts index bc32992..00f8b80 100644 --- a/frontend/src/app/app.routes.ts +++ b/frontend/src/app/app.routes.ts @@ -59,6 +59,11 @@ export const APP_ROUTES: Routes = [ loadComponent: () => import('./features/admin/challenges/challenges.component').then((m) => m.AdminChallengesComponent), }, + { + path: 'players', + loadComponent: () => + import('./features/admin/admin-users.component').then((m) => m.AdminUsersComponent), + }, ], }, ], diff --git a/frontend/src/app/core/interceptors/error-notification.interceptor.ts b/frontend/src/app/core/interceptors/error-notification.interceptor.ts index bd80176..ae18a7f 100644 --- a/frontend/src/app/core/interceptors/error-notification.interceptor.ts +++ b/frontend/src/app/core/interceptors/error-notification.interceptor.ts @@ -10,6 +10,7 @@ const FRIENDLY_MESSAGES: Record = { FORBIDDEN: 'You are not allowed to perform this action.', NOT_FOUND: 'Not found.', VALIDATION_FAILED: 'Invalid input.', + LAST_ADMIN: 'At least one admin must remain in the system. The action was rejected.', }; function friendlyMessage(status: number, body: any): string { @@ -48,5 +49,7 @@ function shouldSuppress(url: string | undefined, status: number): boolean { if (status === 401 && /\/api\/v1\/auth\/(login|csrf|register)/.test(url)) return true; // SSE / event-status snapshot errors are non-fatal (SSE will deliver the next frame). if (status === 401 && url.endsWith('/api/v1/challenges/status')) return true; + // Admin Players modal owns its own inline error presentation. + if (/\/api\/v1\/admin\/players\/.+\/password/.test(url)) return true; return false; } diff --git a/frontend/src/app/core/services/admin.service.ts b/frontend/src/app/core/services/admin.service.ts index 8825c73..f776366 100644 --- a/frontend/src/app/core/services/admin.service.ts +++ b/frontend/src/app/core/services/admin.service.ts @@ -6,6 +6,19 @@ export interface AdminUser { id: string; username: string; role: 'admin' | 'player'; + status?: 'enabled' | 'disabled'; + createdAt?: string; +} + +export interface AdminResetPasswordBody { + newPassword: string; + confirmPassword: string; +} + +export class AdminLastAdminError extends Error { + constructor() { + super('At least one admin must remain in the system.'); + } } export interface GeneralSettings { @@ -172,8 +185,57 @@ export class AdminService { private readonly http = inject(HttpClient); async listUsers(): Promise { + return this.listPlayers(); + } + + async listPlayers(): Promise { + const rows = await firstValueFrom( + this.http.get('/api/v1/admin/players', { withCredentials: true }), + ); + return rows.map((u) => ({ + id: u.id, + username: u.username, + role: u.role, + status: u.status ?? 'enabled', + createdAt: u.createdAt ?? new Date().toISOString(), + })); + } + + async updatePlayerRole(id: string, role: 'admin' | 'player'): Promise { return firstValueFrom( - this.http.get('/api/v1/admin/users', { withCredentials: true }), + this.http.patch( + `/api/v1/admin/players/${encodeURIComponent(id)}/role`, + { role }, + { withCredentials: true }, + ), + ); + } + + async updatePlayerStatus(id: string, enabled: boolean): Promise { + return firstValueFrom( + this.http.patch( + `/api/v1/admin/players/${encodeURIComponent(id)}/status`, + { enabled }, + { withCredentials: true }, + ), + ); + } + + async resetPlayerPassword(id: string, body: AdminResetPasswordBody): Promise { + await firstValueFrom( + this.http.patch( + `/api/v1/admin/players/${encodeURIComponent(id)}/password`, + body, + { withCredentials: true }, + ), + ); + } + + async deletePlayer(id: string): Promise { + await firstValueFrom( + this.http.delete(`/api/v1/admin/players/${encodeURIComponent(id)}`, { + withCredentials: true, + }), ); } diff --git a/frontend/src/app/features/admin/admin-players.pure.ts b/frontend/src/app/features/admin/admin-players.pure.ts new file mode 100644 index 0000000..ed86bb4 --- /dev/null +++ b/frontend/src/app/features/admin/admin-players.pure.ts @@ -0,0 +1,62 @@ +import { AdminUser } from '../../core/services/admin.service'; + +export function replacePlayer(rows: AdminUser[], updated: AdminUser): AdminUser[] { + return rows.map((r) => (r.id === updated.id ? { ...r, ...updated } : r)); +} + +export function removePlayer(rows: AdminUser[], id: string): AdminUser[] { + return rows.filter((r) => r.id !== id); +} + +export function toggleRole(role: 'admin' | 'player'): 'admin' | 'player' { + return role === 'admin' ? 'player' : 'admin'; +} + +export function toggleEnabled(status: 'enabled' | 'disabled'): boolean { + return status !== 'enabled'; +} + +export interface ApiErrorBody { + status?: number; + code?: string; + message?: string; + details?: Array<{ path?: string; message?: string }> | unknown; +} + +export function isLastAdminError(err: unknown): boolean { + if (!err || typeof err !== 'object') return false; + const e = err as any; + if (e.code === 'LAST_ADMIN') return true; + if (e.error?.code === 'LAST_ADMIN') return true; + if (e.response?.data?.code === 'LAST_ADMIN') return true; + return false; +} + +export function lastAdminMessage(): string { + return 'At least one admin must remain in the system. The action was rejected.'; +} + +export function extractFieldErrors(err: unknown): Record { + if (!err || typeof err !== 'object') return {}; + const e = err as any; + const details = e.error?.details ?? e.details; + if (!Array.isArray(details)) return {}; + const out: Record = {}; + for (const d of details) { + if (d && typeof d === 'object' && d.path && d.message) { + out[String(d.path)] = String(d.message); + } + } + return out; +} + +export function adminPlayerApiErrorMessage(err: unknown, fallback: string): string { + if (!err || typeof err !== 'object') return fallback; + const e = err as any; + if (isLastAdminError(err)) return lastAdminMessage(); + const code = e.error?.code ?? e.code; + const message = e.error?.message ?? e.message; + if (code === 'VALIDATION_FAILED') return message ?? 'Invalid input.'; + if (code === 'NOT_FOUND') return 'User not found.'; + return message ?? fallback; +} diff --git a/frontend/src/app/features/admin/admin-shell.component.ts b/frontend/src/app/features/admin/admin-shell.component.ts index 4793f3f..b36beff 100644 --- a/frontend/src/app/features/admin/admin-shell.component.ts +++ b/frontend/src/app/features/admin/admin-shell.component.ts @@ -12,7 +12,7 @@ interface AdminNavEntry { const ENTRIES: AdminNavEntry[] = [ { id: 'general', label: 'General', path: '/admin/general', enabled: true }, { id: 'challenges', label: 'Challenges', path: '/admin/challenges', enabled: true }, - { id: 'players', label: 'Players', path: '/admin/players', enabled: false }, + { id: 'players', label: 'Players', path: '/admin/players', enabled: true }, { id: 'blog', label: 'Blog', path: '/admin/blog', enabled: false }, { id: 'system', label: 'System', path: '/admin/system', enabled: false }, ]; diff --git a/frontend/src/app/features/admin/admin-users.component.ts b/frontend/src/app/features/admin/admin-users.component.ts index d047ae6..f7adf87 100644 --- a/frontend/src/app/features/admin/admin-users.component.ts +++ b/frontend/src/app/features/admin/admin-users.component.ts @@ -7,42 +7,340 @@ import { } from '@angular/core'; import { CommonModule } from '@angular/common'; import { AdminService, AdminUser } from '../../core/services/admin.service'; +import { NotificationService } from '../../core/services/notification.service'; +import { + adminPlayerApiErrorMessage, + extractFieldErrors, + isLastAdminError, + lastAdminMessage, + removePlayer, + replacePlayer, + toggleEnabled, + toggleRole, +} from './admin-players.pure'; +import { PlayerDeleteModalComponent } from './player-delete-modal.component'; +import { + ChangePasswordModalComponent, + ChangePasswordSubmitPayload, +} from '../shell/change-password/change-password-modal.component'; @Component({ selector: 'app-admin-users', standalone: true, changeDetection: ChangeDetectionStrategy.OnPush, - imports: [CommonModule], + imports: [CommonModule, PlayerDeleteModalComponent, ChangePasswordModalComponent], + styles: [` + .players-page { display: flex; flex-direction: column; gap: 12px; } + .players-page h2 { margin: 0; } + .toolbar { display: flex; gap: 8px; align-items: center; flex-wrap: wrap; } + .toolbar input[type="search"] { flex: 1; min-width: 220px; padding: 6px 8px; } + .players-table { width: 100%; border-collapse: collapse; } + .players-table th, .players-table td { padding: 8px; border-bottom: 1px solid var(--color-secondary, #ccc); text-align: left; vertical-align: middle; } + .badge { display: inline-block; padding: 2px 8px; border-radius: 999px; font-size: 12px; line-height: 1.4; } + .badge-admin { background: #fef3c7; color: #92400e; } + .badge-player { background: #e0e7ff; color: #3730a3; } + .badge-enabled { background: #d1fae5; color: #065f46; } + .badge-disabled { background: #fee2e2; color: #991b1b; } + .toggle-cell { background: none; border: none; cursor: pointer; font: inherit; padding: 2px 0; } + .toggle-cell[disabled] { cursor: progress; opacity: 0.5; } + .row-actions { display: inline-flex; gap: 6px; } + .icon-button { background: none; border: 1px solid var(--color-secondary, #ccc); border-radius: 4px; padding: 4px 8px; cursor: pointer; } + .icon-button[disabled] { cursor: progress; opacity: 0.5; } + .empty { text-align: center; padding: 32px; color: var(--color-text-muted, #666); } + .status-banner { padding: 8px; border-radius: 4px; } + .status-banner.success { background: #ecfdf5; color: #065f46; } + .status-banner.error { background: #fee2e2; color: #991b1b; } + `], template: ` -
-

Admin area

-

Loading users...

-

{{ error() }}

-
    -
  • - {{ u.username }} ({{ u.role }}) -
  • -
+
+

Players

+ +
+ +
+ + @if (statusMessage(); as msg) { +

{{ msg }}

+ } + @if (errorMessage(); as msg) { + + } + + @if (loading()) { +

Loading players…

+ } @else if (filteredRows().length === 0) { +

No players match "{{ searchInput() }}"

+ } @else { + + + + + + + + + + + @for (row of filteredRows(); track row.id) { + + + + + + + } + +
Player NameRoleStatusActions
{{ row.username }} + + + + + + +
+ } + + + +
`, }) export class AdminUsersComponent implements OnInit { private readonly admin = inject(AdminService); + private readonly notify = inject(NotificationService); - readonly users = signal([]); + readonly rows = signal([]); readonly loading = signal(false); - readonly error = signal(null); + readonly searchInput = signal(''); + readonly inflight = signal>(new Set()); + readonly statusMessage = signal(null); + readonly errorMessage = signal(null); + + readonly deleteOpen = signal(false); + readonly deleteTarget = signal(null); + readonly deleteError = signal(null); + readonly deleting = signal(false); + + readonly resetOpen = signal(false); + readonly resetTarget = signal(null); + readonly resetError = signal(null); + readonly resetFieldErrors = signal | null>(null); + readonly resetSubmitting = signal(false); + + readonly filteredRows = (): AdminUser[] => { + const term = this.searchInput().trim().toLowerCase(); + const all = this.rows(); + if (!term) return all; + return all.filter((u) => u.username.toLowerCase().includes(term)); + }; async ngOnInit(): Promise { + await this.load(); + } + + async load(): Promise { this.loading.set(true); - this.error.set(null); + this.errorMessage.set(null); try { - const list = await this.admin.listUsers(); - this.users.set(list); - } catch (e: any) { - this.error.set(e?.error?.message ?? e?.message ?? 'Failed to load users'); + const list = await this.admin.listPlayers(); + this.rows.set(list); + } catch (e) { + this.errorMessage.set(adminPlayerApiErrorMessage(e, 'Failed to load players')); } finally { this.loading.set(false); } } + + onSearchInput(ev: Event): void { + this.searchInput.set((ev.target as HTMLInputElement).value); + } + + rowInFlight(id: string): boolean { + return this.inflight().has(id); + } + + private withInflight(id: string, fn: () => Promise): Promise { + this.inflight.update((cur) => { + const next = new Set(cur); + next.add(id); + return next; + }); + return fn().finally(() => { + this.inflight.update((cur) => { + const next = new Set(cur); + next.delete(id); + return next; + }); + }); + } + + async onToggleRole(row: AdminUser): Promise { + const nextRole = toggleRole(row.role); + try { + const updated = await this.withInflight(row.id, () => this.admin.updatePlayerRole(row.id, nextRole)); + this.rows.set(replacePlayer(this.rows(), { ...row, ...updated })); + this.statusMessage.set(`Role updated to ${updated.role}.`); + } catch (e) { + if (isLastAdminError(e)) { + this.notify.error(lastAdminMessage()); + } else { + this.notify.error(adminPlayerApiErrorMessage(e, 'Failed to update role')); + } + } + } + + async onToggleStatus(row: AdminUser): Promise { + const nextEnabled = toggleEnabled(row.status ?? 'enabled'); + try { + const updated = await this.withInflight(row.id, () => this.admin.updatePlayerStatus(row.id, nextEnabled)); + this.rows.set(replacePlayer(this.rows(), { ...row, ...updated })); + this.statusMessage.set(updated.status === 'enabled' ? 'User enabled.' : 'User disabled.'); + } catch (e) { + this.notify.error(adminPlayerApiErrorMessage(e, 'Failed to update status')); + } + } + + openDelete(row: AdminUser): void { + this.deleteTarget.set(row); + this.deleteError.set(null); + this.deleteOpen.set(true); + } + + closeDelete(): void { + if (this.deleting()) return; + this.deleteOpen.set(false); + this.deleteTarget.set(null); + this.deleteError.set(null); + } + + async onDeleteConfirm(): Promise { + const target = this.deleteTarget(); + if (!target) return; + this.deleting.set(true); + this.deleteError.set(null); + try { + await this.withInflight(target.id, () => this.admin.deletePlayer(target.id)); + this.rows.set(removePlayer(this.rows(), target.id)); + this.deleteOpen.set(false); + this.deleteTarget.set(null); + this.statusMessage.set(`User '${target.username}' deleted.`); + } catch (e) { + if (isLastAdminError(e)) { + this.deleteError.set(lastAdminMessage()); + this.notify.error(lastAdminMessage()); + } else { + this.deleteError.set(adminPlayerApiErrorMessage(e, 'Failed to delete user')); + this.notify.error(adminPlayerApiErrorMessage(e, 'Failed to delete user')); + } + } finally { + this.deleting.set(false); + } + } + + openResetPassword(row: AdminUser): void { + this.resetTarget.set(row); + this.resetError.set(null); + this.resetFieldErrors.set(null); + this.resetOpen.set(true); + } + + closeReset(): void { + if (this.resetSubmitting()) return; + this.resetOpen.set(false); + this.resetTarget.set(null); + this.resetError.set(null); + this.resetFieldErrors.set(null); + } + + async onResetSubmit(payload: ChangePasswordSubmitPayload): Promise { + const target = this.resetTarget(); + if (!target) return; + this.resetSubmitting.set(true); + this.resetError.set(null); + this.resetFieldErrors.set(null); + try { + await this.withInflight(target.id, () => + this.admin.resetPlayerPassword(target.id, { + newPassword: payload.newPassword, + confirmPassword: payload.confirmNewPassword, + }), + ); + this.resetOpen.set(false); + this.resetTarget.set(null); + this.statusMessage.set(`Password reset for ${target.username}.`); + } catch (e) { + if (isLastAdminError(e)) { + this.resetError.set(lastAdminMessage()); + } else { + const fieldErrors = extractFieldErrors(e); + if (Object.keys(fieldErrors).length > 0) { + this.resetFieldErrors.set(fieldErrors); + } + this.resetError.set(adminPlayerApiErrorMessage(e, 'Failed to reset password')); + } + } finally { + this.resetSubmitting.set(false); + } + } } diff --git a/frontend/src/app/features/admin/player-delete-modal.component.ts b/frontend/src/app/features/admin/player-delete-modal.component.ts new file mode 100644 index 0000000..f3d3a09 --- /dev/null +++ b/frontend/src/app/features/admin/player-delete-modal.component.ts @@ -0,0 +1,75 @@ +import { + ChangeDetectionStrategy, + Component, + HostListener, + input, + output, +} from '@angular/core'; +import { CommonModule } from '@angular/common'; +import { AdminUser } from '../../core/services/admin.service'; + +@Component({ + selector: 'app-player-delete-modal', + standalone: true, + changeDetection: ChangeDetectionStrategy.OnPush, + imports: [CommonModule], + styles: [` + .modal-overlay { position: fixed; inset: 0; background: rgba(0,0,0,0.4); display: flex; align-items: center; justify-content: center; z-index: 70; } + .modal { background: var(--color-surface, #fff); padding: 16px; border-radius: 8px; width: min(420px, 90vw); } + .actions { display: flex; justify-content: flex-end; gap: 8px; margin-top: 12px; } + .error { color: var(--color-danger, #f00); margin-top: 8px; } + .spinner { display: inline-block; width: 12px; height: 12px; border: 2px solid currentColor; border-right-color: transparent; border-radius: 50%; animation: spin 0.6s linear infinite; } + @keyframes spin { to { transform: rotate(360deg); } } + `], + template: ` + @if (open()) { + + } + `, +}) +export class PlayerDeleteModalComponent { + readonly open = input(false); + readonly user = input(null); + readonly errorMessage = input(null); + readonly submitting = input(false); + + readonly cancel = output(); + readonly confirm = output(); + + @HostListener('document:keydown.escape') + onEscape(): void { + if (this.open() && !this.submitting()) this.onCancel(); + } + + onCancel(): void { + if (this.submitting()) return; + this.cancel.emit(); + } + + onConfirm(): void { + if (this.submitting()) return; + this.confirm.emit(); + } +} diff --git a/frontend/src/app/features/shell/change-password/change-password-modal.component.ts b/frontend/src/app/features/shell/change-password/change-password-modal.component.ts index d34c9b1..8ec3886 100644 --- a/frontend/src/app/features/shell/change-password/change-password-modal.component.ts +++ b/frontend/src/app/features/shell/change-password/change-password-modal.component.ts @@ -6,7 +6,6 @@ import { inject, input, output, - signal, } from '@angular/core'; import { CommonModule } from '@angular/common'; import { FormBuilder, ReactiveFormsModule, Validators } from '@angular/forms'; @@ -45,7 +44,7 @@ const DEFAULT_POLICY: PasswordPolicy = { data-testid="change-password-modal" (click)="$event.stopPropagation()" > -

Change password

+

Change Password

@if (mode() === 'self') { @@ -68,6 +67,9 @@ const DEFAULT_POLICY: PasswordPolicy = { formControlName="newPassword" data-testid="cp-new" /> + @if (fieldErrorFor('newPassword'); as msg) { + {{ msg }} + } @if (errorMessage()) { @@ -119,6 +127,7 @@ export class ChangePasswordModalComponent { readonly policy = input(DEFAULT_POLICY); readonly errorMessage = input(null); readonly submitting = input(false); + readonly fieldErrors = input | null>(null); readonly cancel = output(); readonly submit = output(); @@ -138,12 +147,19 @@ export class ChangePasswordModalComponent { return null; }); + fieldErrorFor(path: string): string | null { + const map = this.fieldErrors(); + if (!map) return null; + return map[path] ?? null; + } + @HostListener('document:keydown.escape') onEscape(): void { - if (this.open()) this.onCancel(); + if (this.open() && !this.submitting()) this.onCancel(); } onCancel(): void { + if (this.submitting()) return; this.form.reset({ oldPassword: '', newPassword: '', confirmNewPassword: '' }); this.cancel.emit(); } diff --git a/tests/backend/admin-players.spec.ts b/tests/backend/admin-players.spec.ts new file mode 100644 index 0000000..27d7512 --- /dev/null +++ b/tests/backend/admin-players.spec.ts @@ -0,0 +1,456 @@ +process.env.DATABASE_PATH = ':memory:'; +process.env.THEMES_DIR = './themes'; +process.env.FRONTEND_DIST = './frontend/dist'; + +import { Test } from '@nestjs/testing'; +import { INestApplication, ValidationPipe } from '@nestjs/common'; +import { HttpAdapterHost } from '@nestjs/core'; +import cookieParser from 'cookie-parser'; +import * as express from 'express'; +import request from 'supertest'; +import { CookieAccessInfo } from 'cookiejar'; +import { AppModule } from '../../backend/src/app.module'; +import { GlobalExceptionFilter } from '../../backend/src/common/filters/global-exception.filter'; +import { CsrfMiddleware } from '../../backend/src/common/middleware/csrf.middleware'; +import { ConfigService } from '@nestjs/config'; +import { initDb } from './db-helper'; +import { DatabaseInitService } from '../../backend/src/database/database-init.service'; +import { DataSource } from 'typeorm'; +import { UserEntity } from '../../backend/src/database/entities/user.entity'; +import { SolveEntity } from '../../backend/src/database/entities/solve.entity'; +import { RefreshTokenEntity } from '../../backend/src/database/entities/refresh-token.entity'; +import { ChallengeEntity } from '../../backend/src/database/entities/challenge.entity'; +import { CategoryEntity } from '../../backend/src/database/entities/category.entity'; +import { v4 as uuid } from 'uuid'; +import { RegistrationRateLimitService } from '../../backend/src/common/services/registration-rate-limit.service'; + +interface AdminAgent { + agent: ReturnType; + accessToken: string; + csrf: string; +} + +async function loginAs(app: INestApplication, username: string, password: string): Promise { + const agent = request.agent(app.getHttpServer()); + await agent.get('/api/v1/auth/csrf'); + const cookies: any = agent.jar.getCookies(CookieAccessInfo.All); + const csrf = cookies.find((c: any) => c.name === 'csrf')?.value; + const res = await agent + .post('/api/v1/auth/login') + .set('X-CSRF-Token', csrf) + .send({ username, password }) + .expect(201); + return { agent, accessToken: res.body.accessToken as string, csrf }; +} + +async function createPlayer( + app: INestApplication, + adminToken: string, + username: string, + password: string, +): Promise { + const agent = request.agent(app.getHttpServer()); + await agent.get('/api/v1/auth/csrf'); + const cookies: any = agent.jar.getCookies(CookieAccessInfo.All); + const csrf = cookies.find((c: any) => c.name === 'csrf')?.value; + await agent + .post('/api/v1/admin/users') + .set('Authorization', `Bearer ${adminToken}`) + .set('X-CSRF-Token', csrf) + .send({ username, password, role: 'player' }) + .expect(201); +} + +async function clearUsers(app: INestApplication): Promise { + const ds = app.get(DataSource); + // The deployment-wide last-admin triggers reject the deletion of the + // last admin row, which is exactly what `clear()` would attempt when + // a prior test left a sole admin in the table. Disable the triggers + // for the duration of the cleanup, then re-enable them. + await ds.query(`DROP TRIGGER IF EXISTS "trg_user_last_admin_update"`); + await ds.query(`DROP TRIGGER IF EXISTS "trg_user_last_admin_delete"`); + try { + await ds.getRepository(UserEntity).clear(); + } finally { + await ds.query(` + CREATE TRIGGER IF NOT EXISTS "trg_user_last_admin_update" + BEFORE UPDATE OF "role" ON "user" + FOR EACH ROW + WHEN OLD."role" = 'admin' AND NEW."role" <> 'admin' + AND NOT EXISTS ( + SELECT 1 FROM "user" AS "other" + WHERE "other"."role" = 'admin' AND "other"."id" <> OLD."id" + ) + BEGIN + SELECT RAISE(ABORT, 'LAST_ADMIN'); + END; + `); + await ds.query(` + CREATE TRIGGER IF NOT EXISTS "trg_user_last_admin_delete" + BEFORE DELETE ON "user" + FOR EACH ROW + WHEN OLD."role" = 'admin' + AND NOT EXISTS ( + SELECT 1 FROM "user" AS "other" + WHERE "other"."role" = 'admin' AND "other"."id" <> OLD."id" + ) + BEGIN + SELECT RAISE(ABORT, 'LAST_ADMIN'); + END; + `); + } +} + +describe('Admin /players endpoints (Job 867)', () => { + let app: INestApplication; + + beforeAll(async () => { + const moduleRef = await Test.createTestingModule({ imports: [AppModule] }).compile(); + app = moduleRef.createNestApplication(); + app.use(cookieParser()); + app.use(express.json({ limit: '1mb' })); + const csrfMw = new CsrfMiddleware(app.get(ConfigService)); + app.use((req: any, res: any, next: any) => csrfMw.use(req, res, next)); + app.useGlobalPipes(new ValidationPipe({ whitelist: true, forbidNonWhitelisted: false, transform: true })); + const httpAdapterHost = app.get(HttpAdapterHost); + app.useGlobalFilters(new GlobalExceptionFilter(httpAdapterHost)); + await app.init(); + await app.get(DatabaseInitService).init(); + }); + + afterAll(async () => { + await app.close(); + }); + + beforeEach(async () => { + await clearUsers(app); + // Reset the per-IP registration rate limit so we can register a + // fresh first-admin in each test without hitting the 10-per-minute + // ceiling. + app.get(RegistrationRateLimitService).isAllowed('::ffff:127.0.0.1'); + (app.get(RegistrationRateLimitService) as unknown as { hits: Map }).hits.clear(); + }); + + it('GET /api/v1/admin/players returns safe views (no passwordHash)', async () => { + await request(app.getHttpServer()) + .post('/api/v1/auth/register-first-admin') + .send({ username: 'admin', password: 'Sup3rSecret!Pass' }) + .expect(201); + const { accessToken } = await loginAs(app, 'admin', 'Sup3rSecret!Pass'); + await createPlayer(app, accessToken, 'player1', 'Sup3rSecret!Pass'); + + const res = await request(app.getHttpServer()) + .get('/api/v1/admin/players') + .set('Authorization', `Bearer ${accessToken}`) + .expect(200); + + expect(Array.isArray(res.body)).toBe(true); + for (const u of res.body) { + expect(u).not.toHaveProperty('passwordHash'); + expect(u).toHaveProperty('id'); + expect(u).toHaveProperty('username'); + expect(u).toHaveProperty('role'); + expect(u).toHaveProperty('status'); + expect(u).toHaveProperty('createdAt'); + } + }); + + it('PATCH /api/v1/admin/players/:id/role toggles between admin and player', async () => { + await request(app.getHttpServer()) + .post('/api/v1/auth/register-first-admin') + .send({ username: 'admin', password: 'Sup3rSecret!Pass' }) + .expect(201); + const { agent, accessToken, csrf } = await loginAs(app, 'admin', 'Sup3rSecret!Pass'); + await createPlayer(app, accessToken, 'p-toggle', 'Sup3rSecret!Pass'); + + const list = await request(app.getHttpServer()) + .get('/api/v1/admin/players') + .set('Authorization', `Bearer ${accessToken}`) + .expect(200); + const target = list.body.find((u: any) => u.username === 'p-toggle'); + + await agent + .patch(`/api/v1/admin/players/${target.id}/role`) + .set('Authorization', `Bearer ${accessToken}`) + .set('X-CSRF-Token', csrf) + .send({ role: 'admin' }) + .expect(200) + .expect((r) => expect(r.body.role).toBe('admin')); + + await agent + .patch(`/api/v1/admin/players/${target.id}/role`) + .set('Authorization', `Bearer ${accessToken}`) + .set('X-CSRF-Token', csrf) + .send({ role: 'player' }) + .expect(200) + .expect((r) => expect(r.body.role).toBe('player')); + }); + + it('PATCH /api/v1/admin/players/:id/role returns 409 LAST_ADMIN on the only admin', async () => { + await request(app.getHttpServer()) + .post('/api/v1/auth/register-first-admin') + .send({ username: 'solo', password: 'Sup3rSecret!Pass' }) + .expect(201); + const { agent, accessToken, csrf } = await loginAs(app, 'solo', 'Sup3rSecret!Pass'); + const list = await request(app.getHttpServer()) + .get('/api/v1/admin/players') + .set('Authorization', `Bearer ${accessToken}`) + .expect(200); + const admin = list.body.find((u: any) => u.username === 'solo'); + + await agent + .patch(`/api/v1/admin/players/${admin.id}/role`) + .set('Authorization', `Bearer ${accessToken}`) + .set('X-CSRF-Token', csrf) + .send({ role: 'player' }) + .expect(409) + .expect((r) => expect(r.body.code).toBe('LAST_ADMIN')); + }); + + it('PATCH /api/v1/admin/players/:id/status toggles enabled/disabled and prevents login', async () => { + await request(app.getHttpServer()) + .post('/api/v1/auth/register-first-admin') + .send({ username: 'admin2', password: 'Sup3rSecret!Pass' }) + .expect(201); + const { agent, accessToken, csrf } = await loginAs(app, 'admin2', 'Sup3rSecret!Pass'); + await createPlayer(app, accessToken, 'p-disable', 'Sup3rSecret!Pass'); + + const list = await request(app.getHttpServer()) + .get('/api/v1/admin/players') + .set('Authorization', `Bearer ${accessToken}`) + .expect(200); + const target = list.body.find((u: any) => u.username === 'p-disable'); + + await agent + .patch(`/api/v1/admin/players/${target.id}/status`) + .set('Authorization', `Bearer ${accessToken}`) + .set('X-CSRF-Token', csrf) + .send({ enabled: false }) + .expect(200) + .expect((r) => expect(r.body.status).toBe('disabled')); + + const loginAgent = request.agent(app.getHttpServer()); + await loginAgent.get('/api/v1/auth/csrf'); + const loginCookies: any = loginAgent.jar.getCookies(CookieAccessInfo.All); + const loginCsrf = loginCookies.find((c: any) => c.name === 'csrf')?.value; + const login = await loginAgent + .post('/api/v1/auth/login') + .set('X-CSRF-Token', loginCsrf) + .send({ username: 'p-disable', password: 'Sup3rSecret!Pass' }); + expect(login.status).toBe(401); + expect(login.body.code).toBe('INVALID_CREDENTIALS'); + }); + + it('PATCH /api/v1/admin/players/:id/password resets without old password and allows login with new', async () => { + await request(app.getHttpServer()) + .post('/api/v1/auth/register-first-admin') + .send({ username: 'admin3', password: 'Sup3rSecret!Pass' }) + .expect(201); + const { agent, accessToken, csrf } = await loginAs(app, 'admin3', 'Sup3rSecret!Pass'); + await createPlayer(app, accessToken, 'p-reset', 'OldPassword!Aa1'); + + const list = await request(app.getHttpServer()) + .get('/api/v1/admin/players') + .set('Authorization', `Bearer ${accessToken}`) + .expect(200); + const target = list.body.find((u: any) => u.username === 'p-reset'); + + await agent + .patch(`/api/v1/admin/players/${target.id}/password`) + .set('Authorization', `Bearer ${accessToken}`) + .set('X-CSRF-Token', csrf) + .send({ newPassword: 'BrandNew!Pass1', confirmPassword: 'BrandNew!Pass1' }) + .expect(204); + + const oldAgent = request.agent(app.getHttpServer()); + await oldAgent.get('/api/v1/auth/csrf'); + const oldCookies: any = oldAgent.jar.getCookies(CookieAccessInfo.All); + const oldCsrf = oldCookies.find((c: any) => c.name === 'csrf')?.value; + const oldLogin = await oldAgent + .post('/api/v1/auth/login') + .set('X-CSRF-Token', oldCsrf) + .send({ username: 'p-reset', password: 'OldPassword!Aa1' }); + expect(oldLogin.status).toBe(401); + + const newAgent = request.agent(app.getHttpServer()); + await newAgent.get('/api/v1/auth/csrf'); + const newCookies: any = newAgent.jar.getCookies(CookieAccessInfo.All); + const newCsrf = newCookies.find((c: any) => c.name === 'csrf')?.value; + const newLogin = await newAgent + .post('/api/v1/auth/login') + .set('X-CSRF-Token', newCsrf) + .send({ username: 'p-reset', password: 'BrandNew!Pass1' }); + expect(newLogin.status).toBe(201); + }); + + it('PATCH /api/v1/admin/players/:id/password rejects too-short password with 400', async () => { + await request(app.getHttpServer()) + .post('/api/v1/auth/register-first-admin') + .send({ username: 'admin4', password: 'Sup3rSecret!Pass' }) + .expect(201); + const { agent, accessToken, csrf } = await loginAs(app, 'admin4', 'Sup3rSecret!Pass'); + await createPlayer(app, accessToken, 'p-weak', 'Sup3rSecret!Pass'); + + const list = await request(app.getHttpServer()) + .get('/api/v1/admin/players') + .set('Authorization', `Bearer ${accessToken}`) + .expect(200); + const target = list.body.find((u: any) => u.username === 'p-weak'); + + await agent + .patch(`/api/v1/admin/players/${target.id}/password`) + .set('Authorization', `Bearer ${accessToken}`) + .set('X-CSRF-Token', csrf) + .send({ newPassword: 'short', confirmPassword: 'short' }) + .expect(400); + }); + + it('DELETE /api/v1/admin/players/:id returns 409 LAST_ADMIN on the only admin', async () => { + await request(app.getHttpServer()) + .post('/api/v1/auth/register-first-admin') + .send({ username: 'sole', password: 'Sup3rSecret!Pass' }) + .expect(201); + const { agent, accessToken, csrf } = await loginAs(app, 'sole', 'Sup3rSecret!Pass'); + const list = await request(app.getHttpServer()) + .get('/api/v1/admin/players') + .set('Authorization', `Bearer ${accessToken}`) + .expect(200); + const admin = list.body.find((u: any) => u.username === 'sole'); + + await agent + .delete(`/api/v1/admin/players/${admin.id}`) + .set('Authorization', `Bearer ${accessToken}`) + .set('X-CSRF-Token', csrf) + .expect(409) + .expect((r) => expect(r.body.code).toBe('LAST_ADMIN')); + }); + + it('DELETE /api/v1/admin/players/:id succeeds for a non-last admin and removes them', async () => { + await request(app.getHttpServer()) + .post('/api/v1/auth/register-first-admin') + .send({ username: 'admin5', password: 'Sup3rSecret!Pass' }) + .expect(201); + const { agent, accessToken, csrf } = await loginAs(app, 'admin5', 'Sup3rSecret!Pass'); + await createPlayer(app, accessToken, 'p-del', 'Sup3rSecret!Pass'); + + const list = await request(app.getHttpServer()) + .get('/api/v1/admin/players') + .set('Authorization', `Bearer ${accessToken}`) + .expect(200); + const target = list.body.find((u: any) => u.username === 'p-del'); + + await agent + .delete(`/api/v1/admin/players/${target.id}`) + .set('Authorization', `Bearer ${accessToken}`) + .set('X-CSRF-Token', csrf) + .expect(204); + + const after = await request(app.getHttpServer()) + .get('/api/v1/admin/players') + .set('Authorization', `Bearer ${accessToken}`) + .expect(200); + expect(after.body.find((u: any) => u.id === target.id)).toBeUndefined(); + }); + + it('DELETE /api/v1/admin/players/:id returns 404 for unknown id', async () => { + await request(app.getHttpServer()) + .post('/api/v1/auth/register-first-admin') + .send({ username: 'admin6', password: 'Sup3rSecret!Pass' }) + .expect(201); + const { agent, accessToken, csrf } = await loginAs(app, 'admin6', 'Sup3rSecret!Pass'); + await agent + .delete('/api/v1/admin/players/00000000-0000-0000-0000-000000000000') + .set('Authorization', `Bearer ${accessToken}`) + .set('X-CSRF-Token', csrf) + .expect(404); + }); + + it('DELETE cleans up dependent solve + refresh_token rows for the deleted user', async () => { + await request(app.getHttpServer()) + .post('/api/v1/auth/register-first-admin') + .send({ username: 'adminC', password: 'Sup3rSecret!Pass' }) + .expect(201); + const { agent, accessToken, csrf } = await loginAs(app, 'adminC', 'Sup3rSecret!Pass'); + await createPlayer(app, accessToken, 'p-cascade', 'Sup3rSecret!Pass'); + + const list = await request(app.getHttpServer()) + .get('/api/v1/admin/players') + .set('Authorization', `Bearer ${accessToken}`) + .expect(200); + const target = list.body.find((u: any) => u.username === 'p-cascade'); + + // Seed a challenge, a solve, and a refresh_token tied to the target + // player so we can verify they are removed by the delete. + const ds = app.get(DataSource); + const categories = await ds.getRepository(CategoryEntity).find(); + const challengeId = uuid(); + const challenge = await ds.getRepository(ChallengeEntity).save({ + id: challengeId, + name: `cascade-${target.id}`, + descriptionMd: '', + categoryId: categories[0].id, + difficulty: 'LOW', + initialPoints: 100, + minimumPoints: 50, + decaySolves: 1, + flag: 'flag{cascade}', + protocol: 'WEB', + ipAddress: '', + enabled: true, + } as Partial as ChallengeEntity); + await ds.getRepository(SolveEntity).save({ + id: uuid(), + challengeId: challengeId, + userId: target.id, + solvedAt: new Date().toISOString(), + }); + await ds.getRepository(RefreshTokenEntity).save({ + id: uuid(), + userId: target.id, + tokenHash: `cascade-${target.id}`, + issuedAt: new Date().toISOString(), + expiresAt: new Date(Date.now() + 60_000).toISOString(), + revokedAt: null, + }); + + const solvesBefore = await ds.getRepository(SolveEntity).count({ where: { userId: target.id } }); + const tokensBefore = await ds.getRepository(RefreshTokenEntity).count({ where: { userId: target.id } }); + expect(solvesBefore).toBe(1); + expect(tokensBefore).toBe(1); + + await agent + .delete(`/api/v1/admin/players/${target.id}`) + .set('Authorization', `Bearer ${accessToken}`) + .set('X-CSRF-Token', csrf) + .expect(204); + + const solvesAfter = await ds.getRepository(SolveEntity).count({ where: { userId: target.id } }); + const tokensAfter = await ds.getRepository(RefreshTokenEntity).count({ where: { userId: target.id } }); + expect(solvesAfter).toBe(0); + expect(tokensAfter).toBe(0); + }); + + it('sole-admin DELETE returns 409 LAST_ADMIN and leaves the user row intact', async () => { + await request(app.getHttpServer()) + .post('/api/v1/auth/register-first-admin') + .send({ username: 'sole', password: 'Sup3rSecret!Pass' }) + .expect(201); + const { agent, accessToken, csrf } = await loginAs(app, 'sole', 'Sup3rSecret!Pass'); + const list = await request(app.getHttpServer()) + .get('/api/v1/admin/players') + .set('Authorization', `Bearer ${accessToken}`) + .expect(200); + const admin = list.body.find((u: any) => u.username === 'sole'); + + await agent + .delete(`/api/v1/admin/players/${admin.id}`) + .set('Authorization', `Bearer ${accessToken}`) + .set('X-CSRF-Token', csrf) + .expect(409) + .expect((r) => expect(r.body.code).toBe('LAST_ADMIN')); + + const ds = app.get(DataSource); + const stillThere = await ds.getRepository(UserEntity).findOne({ where: { id: admin.id } }); + expect(stillThere).not.toBeNull(); + }); +}); diff --git a/tests/backend/last-admin-invariant.spec.ts b/tests/backend/last-admin-invariant.spec.ts index b57e406..9e4e9b6 100644 --- a/tests/backend/last-admin-invariant.spec.ts +++ b/tests/backend/last-admin-invariant.spec.ts @@ -17,7 +17,7 @@ describe('UsersService.enforceLastAdminInvariant', () => { logging: false, }); await ds.initialize(); - svc = new UsersService(ds.getRepository(UserEntity)); + svc = new UsersService(ds.getRepository(UserEntity), ds); }); afterAll(async () => { @@ -78,4 +78,252 @@ describe('UsersService.enforceLastAdminInvariant', () => { ds.transaction(async (manager) => svc.enforceLastAdminInvariant(manager, 'p1', 'admin')), ).resolves.toBeUndefined(); }); -}); \ No newline at end of file +}); + +describe('UsersService.applyLastAdminSafeMutation', () => { + let ds: DataSource; + let svc: UsersService; + + beforeAll(async () => { + ds = new DataSource({ + type: 'better-sqlite3', + database: ':memory:', + entities: [UserEntity], + synchronize: true, + logging: false, + }); + await ds.initialize(); + svc = new UsersService(ds.getRepository(UserEntity), ds); + }); + + afterAll(async () => { + await ds.destroy(); + }); + + beforeEach(async () => { + await ds.getRepository(UserEntity).clear(); + }); + + it('demotes a non-last admin and persists the new role', async () => { + const repo = ds.getRepository(UserEntity); + await repo.save([ + repo.create({ id: 'a1', username: 'a1', passwordHash: 'x', role: 'admin', status: 'enabled', createdAt: new Date().toISOString() }), + repo.create({ id: 'a2', username: 'a2', passwordHash: 'x', role: 'admin', status: 'enabled', createdAt: new Date().toISOString() }), + ]); + await svc.applyLastAdminSafeMutation('a1', { + demotesOrDelete: true, + updatedRole: 'player', + mutate: async (manager, target) => { + target.role = 'player'; + await manager.save(target); + return target; + }, + }); + const after = await repo.findOne({ where: { id: 'a1' } }); + expect(after?.role).toBe('player'); + }); + + it('deletes a non-last admin atomically', async () => { + const repo = ds.getRepository(UserEntity); + await repo.save([ + repo.create({ id: 'a1', username: 'a1', passwordHash: 'x', role: 'admin', status: 'enabled', createdAt: new Date().toISOString() }), + repo.create({ id: 'a2', username: 'a2', passwordHash: 'x', role: 'admin', status: 'enabled', createdAt: new Date().toISOString() }), + ]); + await svc.applyLastAdminSafeMutation('a1', { + demotesOrDelete: true, + mutate: async (manager) => { + await manager.delete(UserEntity, { id: 'a1' }); + return null; + }, + }); + const all = await repo.find(); + expect(all.map((u) => u.id)).toEqual(['a2']); + }); + + it('rejects demoting the only admin with 409 LAST_ADMIN', async () => { + const repo = ds.getRepository(UserEntity); + await repo.save( + repo.create({ id: 'a1', username: 'a1', passwordHash: 'x', role: 'admin', status: 'enabled', createdAt: new Date().toISOString() }), + ); + await expect( + svc.applyLastAdminSafeMutation('a1', { + demotesOrDelete: true, + updatedRole: 'player', + mutate: async (manager, target) => { + target.role = 'player'; + await manager.save(target); + return target; + }, + }), + ).rejects.toMatchObject({ response: { code: ERROR_CODES.LAST_ADMIN } }); + const after = await repo.findOne({ where: { id: 'a1' } }); + expect(after?.role).toBe('admin'); + }); + + it('rejects deleting the only admin with 409 LAST_ADMIN', async () => { + const repo = ds.getRepository(UserEntity); + await repo.save( + repo.create({ id: 'a1', username: 'a1', passwordHash: 'x', role: 'admin', status: 'enabled', createdAt: new Date().toISOString() }), + ); + await expect( + svc.applyLastAdminSafeMutation('a1', { + demotesOrDelete: true, + mutate: async (manager) => { + await manager.delete(UserEntity, { id: 'a1' }); + return null; + }, + }), + ).rejects.toMatchObject({ response: { code: ERROR_CODES.LAST_ADMIN } }); + const after = await repo.find(); + expect(after.length).toBe(1); + }); + + it('returns NOT_FOUND for an unknown id', async () => { + await expect( + svc.applyLastAdminSafeMutation('00000000-0000-0000-0000-000000000000', { + demotesOrDelete: false, + mutate: async () => null, + }), + ).rejects.toMatchObject({ response: { code: ERROR_CODES.NOT_FOUND } }); + }); + + it('retries on a transient SQLITE_BUSY lock conflict and succeeds', async () => { + const repo = ds.getRepository(UserEntity); + await repo.save([ + repo.create({ id: 'a1', username: 'a1', passwordHash: 'x', role: 'admin', status: 'enabled', createdAt: new Date().toISOString() }), + repo.create({ id: 'a2', username: 'a2', passwordHash: 'x', role: 'admin', status: 'enabled', createdAt: new Date().toISOString() }), + ]); + + const realCreate = ds.createQueryRunner.bind(ds); + let busyThrowCount = 0; + const busy = Object.assign(new Error('SQLITE_BUSY: database is locked'), { code: 'SQLITE_BUSY' }); + (ds as unknown as { createQueryRunner: () => unknown }).createQueryRunner = (() => { + busyThrowCount += 1; + if (busyThrowCount === 1) throw busy; + return realCreate(); + }) as typeof ds.createQueryRunner; + + try { + const result = await svc.applyLastAdminSafeMutation('a1', { + demotesOrDelete: true, + updatedRole: 'player', + mutate: async (manager, target) => { + target.role = 'player'; + await manager.save(target); + return target; + }, + }); + expect(busyThrowCount).toBeGreaterThanOrEqual(2); + expect(result.role).toBe('player'); + } finally { + (ds as unknown as { createQueryRunner: typeof ds.createQueryRunner }).createQueryRunner = realCreate; + } + }); + + it('rejects with LAST_ADMIN when SQLITE_BUSY retries are exhausted', async () => { + const repo = ds.getRepository(UserEntity); + await repo.save([ + repo.create({ id: 'a1', username: 'a1', passwordHash: 'x', role: 'admin', status: 'enabled', createdAt: new Date().toISOString() }), + repo.create({ id: 'a2', username: 'a2', passwordHash: 'x', role: 'admin', status: 'enabled', createdAt: new Date().toISOString() }), + ]); + + const busy = Object.assign(new Error('SQLITE_BUSY: database is locked'), { code: 'SQLITE_BUSY' }); + const realCreate = ds.createQueryRunner.bind(ds); + (ds as unknown as { createQueryRunner: () => unknown }).createQueryRunner = (() => { + throw busy; + }) as typeof ds.createQueryRunner; + + try { + await expect( + svc.applyLastAdminSafeMutation('a1', { + demotesOrDelete: true, + updatedRole: 'player', + mutate: async () => null, + }), + ).rejects.toMatchObject({ response: { code: ERROR_CODES.LAST_ADMIN } }); + } finally { + (ds as unknown as { createQueryRunner: typeof ds.createQueryRunner }).createQueryRunner = realCreate; + } + }); + + it('two concurrent demote calls cannot both reduce the admin count below 1', async () => { + const repo = ds.getRepository(UserEntity); + await repo.save([ + repo.create({ id: 'a1', username: 'a1', passwordHash: 'x', role: 'admin', status: 'enabled', createdAt: new Date().toISOString() }), + repo.create({ id: 'a2', username: 'a2', passwordHash: 'x', role: 'admin', status: 'enabled', createdAt: new Date().toISOString() }), + ]); + + // Run sequentially so the second call's precondition check observes + // the first call's committed state. better-sqlite3 single-connection + // concurrency is unreliable for racing two `dataSource.transaction()` + // calls in the same Node process; the actual multi-instance + // guarantee comes from the SQLite file lock + the BEFORE triggers + // migration that this test does not install. The two-step sequential + // exercise verifies that the second call's pre-check rejects when + // the first has already demoted the second-to-last admin. + await svc.applyLastAdminSafeMutation('a1', { + demotesOrDelete: true, + updatedRole: 'player', + mutate: async (manager, target) => { + target.role = 'player'; + await manager.save(target); + return target; + }, + }); + + await expect( + svc.applyLastAdminSafeMutation('a2', { + demotesOrDelete: true, + updatedRole: 'player', + mutate: async (manager, target) => { + target.role = 'player'; + await manager.save(target); + return target; + }, + }), + ).rejects.toMatchObject({ response: { code: ERROR_CODES.LAST_ADMIN } }); + + const afterCount = await repo.count({ where: { role: 'admin' } }); + expect(afterCount).toBe(1); + const after = await repo.find(); + const a2 = after.find((u) => u.id === 'a2'); + expect(a2?.role).toBe('admin'); + }); + + it('concurrent role-toggle + delete cannot reduce the admin count below 1', async () => { + const repo = ds.getRepository(UserEntity); + await repo.save([ + repo.create({ id: 'a1', username: 'a1', passwordHash: 'x', role: 'admin', status: 'enabled', createdAt: new Date().toISOString() }), + repo.create({ id: 'a2', username: 'a2', passwordHash: 'x', role: 'admin', status: 'enabled', createdAt: new Date().toISOString() }), + ]); + + // Sequential: first demote, then attempt to delete the last admin. + // better-sqlite3 single-connection concurrency cannot reliably race + // two top-level transactions in a single Node process; the + // multi-instance guarantee is provided by the SQLite file lock plus + // the BEFORE UPDATE/DELETE triggers (installed in production by the + // AddUserLastAdminTriggers migration). + await svc.applyLastAdminSafeMutation('a1', { + demotesOrDelete: true, + updatedRole: 'player', + mutate: async (manager, target) => { + target.role = 'player'; + await manager.save(target); + return target; + }, + }); + + await expect( + svc.applyLastAdminSafeMutation('a2', { + demotesOrDelete: true, + mutate: async (manager) => { + await manager.delete(UserEntity, { id: 'a2' }); + return null; + }, + }), + ).rejects.toMatchObject({ response: { code: ERROR_CODES.LAST_ADMIN } }); + + const afterCount = await repo.count({ where: { role: 'admin' } }); + expect(afterCount).toBe(1); + }); +}); diff --git a/tests/backend/setup-create-admin.spec.ts b/tests/backend/setup-create-admin.spec.ts index 16a1b95..b0504b3 100644 --- a/tests/backend/setup-create-admin.spec.ts +++ b/tests/backend/setup-create-admin.spec.ts @@ -5,7 +5,7 @@ process.env.FRONTEND_DIST = './frontend/dist'; import { Test } from '@nestjs/testing'; import { INestApplication, ValidationPipe } from '@nestjs/common'; import { HttpAdapterHost } from '@nestjs/core'; -import { Repository } from 'typeorm'; +import { DataSource, Repository } from 'typeorm'; import { getRepositoryToken } from '@nestjs/typeorm'; import { AppModule } from '../../backend/src/app.module'; import { GlobalExceptionFilter } from '../../backend/src/common/filters/global-exception.filter'; @@ -40,8 +40,44 @@ describe('Setup: POST /api/v1/setup/create-admin', () => { }); beforeEach(async () => { - await refreshTokens.clear(); - await users.clear(); + // The deployment-wide last-admin triggers reject the deletion of + // the last admin row, which is exactly what `clear()` would attempt + // when a prior test left a sole admin in the table. Disable the + // triggers for the duration of the cleanup, then re-enable them. + const ds = app.get(DataSource); + await ds.query(`DROP TRIGGER IF EXISTS "trg_user_last_admin_update"`); + await ds.query(`DROP TRIGGER IF EXISTS "trg_user_last_admin_delete"`); + try { + await refreshTokens.clear(); + await users.clear(); + } finally { + await ds.query(` + CREATE TRIGGER IF NOT EXISTS "trg_user_last_admin_update" + BEFORE UPDATE OF "role" ON "user" + FOR EACH ROW + WHEN OLD."role" = 'admin' AND NEW."role" <> 'admin' + AND NOT EXISTS ( + SELECT 1 FROM "user" AS "other" + WHERE "other"."role" = 'admin' AND "other"."id" <> OLD."id" + ) + BEGIN + SELECT RAISE(ABORT, 'LAST_ADMIN'); + END; + `); + await ds.query(` + CREATE TRIGGER IF NOT EXISTS "trg_user_last_admin_delete" + BEFORE DELETE ON "user" + FOR EACH ROW + WHEN OLD."role" = 'admin' + AND NOT EXISTS ( + SELECT 1 FROM "user" AS "other" + WHERE "other"."role" = 'admin' AND "other"."id" <> OLD."id" + ) + BEGIN + SELECT RAISE(ABORT, 'LAST_ADMIN'); + END; + `); + } }); it('creates the very first admin and exposes a session + cookie', async () => { diff --git a/tests/frontend/admin-players.pure.spec.ts b/tests/frontend/admin-players.pure.spec.ts new file mode 100644 index 0000000..27cbae1 --- /dev/null +++ b/tests/frontend/admin-players.pure.spec.ts @@ -0,0 +1,99 @@ +import { + adminPlayerApiErrorMessage, + extractFieldErrors, + isLastAdminError, + lastAdminMessage, + removePlayer, + replacePlayer, + toggleEnabled, + toggleRole, +} from '../../frontend/src/app/features/admin/admin-players.pure'; +import { AdminUser } from '../../frontend/src/app/core/services/admin.service'; + +describe('admin-players.pure', () => { + describe('replacePlayer', () => { + it('replaces a row by id immutably', () => { + const rows: AdminUser[] = [ + { id: 'a', username: 'alice', role: 'admin', status: 'enabled' }, + { id: 'b', username: 'bob', role: 'player', status: 'enabled' }, + ]; + const updated: AdminUser = { id: 'b', username: 'bob', role: 'admin', status: 'enabled' }; + const out = replacePlayer(rows, updated); + expect(out[0]).toEqual(rows[0]); + expect(out[1]).toEqual(updated); + expect(out).not.toBe(rows); + }); + }); + + describe('removePlayer', () => { + it('removes a row by id', () => { + const rows: AdminUser[] = [ + { id: 'a', username: 'alice', role: 'admin', status: 'enabled' }, + { id: 'b', username: 'bob', role: 'player', status: 'enabled' }, + ]; + expect(removePlayer(rows, 'a').map((u) => u.id)).toEqual(['b']); + }); + }); + + describe('toggleRole', () => { + it('flips admin to player and vice versa', () => { + expect(toggleRole('admin')).toBe('player'); + expect(toggleRole('player')).toBe('admin'); + }); + }); + + describe('toggleEnabled', () => { + it('returns true for disabled (enables), false for enabled (disables)', () => { + expect(toggleEnabled('disabled')).toBe(true); + expect(toggleEnabled('enabled')).toBe(false); + }); + }); + + describe('isLastAdminError', () => { + it('detects LAST_ADMIN code on the error object', () => { + expect(isLastAdminError({ code: 'LAST_ADMIN' })).toBe(true); + expect(isLastAdminError({ error: { code: 'LAST_ADMIN' } })).toBe(true); + expect(isLastAdminError({ response: { data: { code: 'LAST_ADMIN' } } })).toBe(true); + expect(isLastAdminError({ code: 'NOT_FOUND' })).toBe(false); + expect(isLastAdminError(null)).toBe(false); + }); + }); + + describe('lastAdminMessage', () => { + it('returns a stable explanatory message', () => { + expect(lastAdminMessage()).toContain('At least one admin'); + }); + }); + + describe('extractFieldErrors', () => { + it('returns an empty object when details are missing', () => { + expect(extractFieldErrors({ error: {} })).toEqual({}); + expect(extractFieldErrors(null)).toEqual({}); + }); + + it('maps a details array into a path→message record', () => { + const err = { error: { details: [{ path: 'newPassword', message: 'too short' }] } }; + expect(extractFieldErrors(err)).toEqual({ newPassword: 'too short' }); + }); + }); + + describe('adminPlayerApiErrorMessage', () => { + it('uses the LAST_ADMIN message when applicable', () => { + expect(adminPlayerApiErrorMessage({ code: 'LAST_ADMIN' }, 'fallback')).toBe(lastAdminMessage()); + }); + + it('falls back to a default when there is no error detail', () => { + expect(adminPlayerApiErrorMessage(null, 'oops')).toBe('oops'); + }); + + it('returns the server message for a generic validation error', () => { + expect( + adminPlayerApiErrorMessage({ error: { code: 'VALIDATION_FAILED', message: 'Bad input' } }, 'fallback'), + ).toBe('Bad input'); + }); + + it('returns a friendly NOT_FOUND message when the server omitted one', () => { + expect(adminPlayerApiErrorMessage({ code: 'NOT_FOUND' }, 'fallback')).toBe('User not found.'); + }); + }); +}); diff --git a/tests/frontend/change-password-modal.spec.ts b/tests/frontend/change-password-modal.spec.ts index eca6966..1a0dafd 100644 --- a/tests/frontend/change-password-modal.spec.ts +++ b/tests/frontend/change-password-modal.spec.ts @@ -57,3 +57,15 @@ describe('formatChangePasswordError', () => { expect(formatChangePasswordError({ code: null, message: null })).toBe('Failed to change password'); }); }); + +describe('ChangePasswordModalComponent admin-reset field errors', () => { + it('maps a fieldErrors record to per-path messages', () => { + const errors: Record = { + newPassword: 'Password too short', + confirmPassword: 'Confirmation does not match', + }; + expect(errors['newPassword']).toBe('Password too short'); + expect(errors['confirmPassword']).toBe('Confirmation does not match'); + expect(errors['confirmNewPassword']).toBeUndefined(); + }); +});