156 lines
7.6 KiB
Markdown
156 lines
7.6 KiB
Markdown
# Implementation Plan: Job 916 — Admin System Restore Commit (stagingId stripped from request body)
|
|
|
|
## Root cause (read-only investigation)
|
|
|
|
The destructive restore never executes because the Zod schema used to validate
|
|
`POST /api/v1/admin/system/restore/commit` **drops the `stagingId` field** from
|
|
the request body before the controller runs.
|
|
|
|
`backend/src/modules/admin/system/dto/admin-system.dto.ts:23`:
|
|
|
|
```ts
|
|
export const AdminRestoreCommitBodySchema = AdminDangerConfirmBodySchema;
|
|
```
|
|
|
|
`AdminDangerConfirmBodySchema` (line 18) only allows `{ operation, token }`.
|
|
Zod's `safeParse` / the NestJS `ZodValidationPipe` therefore strip every
|
|
unknown field — including `stagingId` — from the parsed body. The controller
|
|
then receives `body.stagingId === undefined`, falls back to the empty string
|
|
(`body.stagingId ?? ''`), and `RestoreService.commitRestore('')` finds no
|
|
matching stage and throws `SYSTEM_RESTORE_STAGE_EXPIRED`
|
|
(`RestoreService.commitRestore` at `restore.service.ts:223-231`).
|
|
|
|
The frontend already sends the field correctly
|
|
(`frontend/src/app/features/admin/system/system.service.ts:42`):
|
|
|
|
```ts
|
|
async commitRestore(payload: { operation: 'restore-backup'; token: string; stagingId: string })
|
|
```
|
|
|
|
so the deviation is purely server-side. The negative path (random JSON) still
|
|
fails with `SYSTEM_RESTORE_VALIDATION_FAILED` because the malformed archive
|
|
rejects at `validate`, never reaching `commit`.
|
|
|
|
The fix is to give `AdminRestoreCommitBodySchema` its own declaration that
|
|
**includes** `stagingId` (mirroring `AdminReauthBodySchema` at line 12-16), and
|
|
to tighten the controller so a missing `stagingId` returns a precise
|
|
`SYSTEM_RESTORE_STAGE_EXPIRED` instead of relying on the empty-string fallback.
|
|
|
|
## 1. Architectural Reconnaissance
|
|
|
|
- **Codebase style & conventions:** TypeScript monorepo (`backend` + `frontend`).
|
|
Backend is NestJS with `class-validator`/`zod` mixed; the admin-system module
|
|
uses `ZodValidationPipe` per-route. Strict async/await, error handling via
|
|
`ApiError` + `ERROR_CODES` (`backend/src/common/errors/`).
|
|
- **Data Layer:** SQLite via `better-sqlite3` + TypeORM. Restore streams are
|
|
in-memory (`RestoreService.stages: Map<stagingId, StagedRestore>`) plus a
|
|
sibling directory tree under `<DATA_DIR>/.system-staging/restore-<id>/`.
|
|
- **Test Framework & Structure:** Jest with two projects in
|
|
`tests/jest.config.js`. Tests live in `tests/backend/*.spec.ts` and
|
|
`tests/frontend/*.spec.ts`. Single command from the root:
|
|
`npm test` (already wired in `package.json`).
|
|
- **Required Tools & Dependencies:** No new tools. Existing: Node 20, NestJS,
|
|
better-sqlite3, zod, Jest, ts-jest. `setup.sh` already builds the app and
|
|
rebuilds the native binding — no updates needed.
|
|
|
|
## 2. Impacted Files
|
|
|
|
- **To Modify:**
|
|
- `backend/src/modules/admin/system/dto/admin-system.dto.ts` — replace the
|
|
`AdminRestoreCommitBodySchema` alias with a proper schema that accepts
|
|
`stagingId` (required for `restore-backup`).
|
|
- `backend/src/modules/admin/system/admin-system.controller.ts` — validate
|
|
`stagingId` is present before consuming the token, and stop feeding an
|
|
empty string into `commitRestore`.
|
|
- **To Create:**
|
|
- `tests/backend/admin-system-restore-commit.spec.ts` — focused e2e test
|
|
that the `restore/commit` endpoint forwards `stagingId` to
|
|
`RestoreService.commitRestore` and atomically swaps the live DB.
|
|
|
|
## 3. Proposed Changes
|
|
|
|
1. **DTO fix** — `backend/src/modules/admin/system/dto/admin-system.dto.ts`
|
|
- Remove the `AdminRestoreCommitBodySchema = AdminDangerConfirmBodySchema`
|
|
alias (line 23).
|
|
- Add a dedicated schema that requires `stagingId` for `restore-backup`
|
|
while still letting the safe schema accept the other two operations in
|
|
shared code paths:
|
|
|
|
```ts
|
|
export const AdminRestoreCommitBodySchema = z.object({
|
|
operation: z.enum(ADMIN_OPERATION_KINDS),
|
|
token: z.string().min(1).max(512),
|
|
stagingId: z.string().min(1).max(128).optional(),
|
|
});
|
|
```
|
|
|
|
- Keep the `AdminOperationKind` re-export and the
|
|
`AdminRestoreCommitBody` `z.infer` type aligned so the controller
|
|
signature remains `body: { operation; token; stagingId? }`.
|
|
|
|
2. **Controller hardening** — `backend/src/modules/admin/system/admin-system.controller.ts:95-121`
|
|
- After the `body.operation === 'restore-backup'` check, throw
|
|
`SYSTEM_RESTORE_STAGE_EXPIRED` with HTTP 400 if `body.stagingId` is
|
|
falsy. This produces a precise error instead of the empty-string
|
|
fallback that currently slips through to `RestoreService`.
|
|
- Pass `body.stagingId` directly to `commitRestore` (no `?? ''`).
|
|
- Existing behavior for `reset-scores` / `wipe-challenges` (which use
|
|
`AdminDangerConfirmBodySchema`) is unchanged.
|
|
|
|
3. **No frontend changes required.** `system.service.ts` already POSTs
|
|
`{ operation, token, stagingId }`; the modal already calls it after
|
|
`confirmations`. Once the server schema preserves `stagingId` the happy
|
|
path works end-to-end.
|
|
|
|
4. **No DB migration needed.** `admin_operation_token` already exists;
|
|
`stagingId` is bound through the in-memory `RestoreService.stages` map.
|
|
|
|
## 4. Test Strategy
|
|
|
|
- **Target Unit Test File:** `tests/backend/admin-system-restore-commit.spec.ts` (new).
|
|
Companion lightweight spec additions to
|
|
`tests/backend/admin-system-restore-validation.spec.ts` are optional; the
|
|
new file is sufficient and keeps the diff small.
|
|
- **Mocking Strategy:**
|
|
- Build a minimal `Test.createTestingModule` with just
|
|
`AdminSystemController`, `RestoreService`, `ConfirmationTokenService`,
|
|
`AuthService`, `BackupService`, `DangerZoneService`, the
|
|
`AdminOperationTokenEntity` repository, the `RestoreArchiveSchema` DTO,
|
|
and a `Role`-aware stub for `AdminGuard` (re-use the approach from
|
|
`tests/backend/admin-system-authorization.spec.ts`).
|
|
- Stub `ConfirmationTokenService.consume` to record the
|
|
`{ userId, operation, stagingId }` argument and resolve `{ id: 'tok' }`.
|
|
- Stub `RestoreService.commitRestore` to record its argument and resolve
|
|
`{ restoresPerformed: true, revokeUserId: userId }`.
|
|
- Stub `AuthService.reauthenticateAdmin` to resolve an admin user, and
|
|
`AuthService.revokeAllRefreshSessions` to a no-op.
|
|
- Stub `RestoreService.stageArchive` to return a fixed
|
|
`{ stagingId: 'stage-1', expiresAt, summary }` so the validate→confirm
|
|
→commit pipeline can be exercised when needed.
|
|
|
|
- **Cases covered (minimal, focused):**
|
|
1. `commitRestore` forwards `stagingId` to `RestoreService.commitRestore`
|
|
and returns `{ ok: true, operation: 'restore-backup' }` when the body
|
|
contains `{ operation, token, stagingId }`.
|
|
2. `commitRestore` returns
|
|
`SYSTEM_RESTORE_STAGE_EXPIRED` (HTTP 400) when `stagingId` is omitted
|
|
**before** consuming the token (i.e. the fix returns the error early
|
|
rather than reaching `RestoreService`).
|
|
3. `commitRestore` returns `SYSTEM_TOKEN_MISMATCH` when `operation` is
|
|
`reset-scores` (regression guard for the existing controller branch).
|
|
|
|
- **Run command:** `npm test` from the repo root (already wired in
|
|
`package.json:test`).
|
|
|
|
## 5. Verification
|
|
|
|
- Manual smoke test against the dev stack (`npm run dev`):
|
|
1. POST `/api/v1/admin/system/restore/validate` with a valid backup → expect
|
|
`stagingId`.
|
|
2. POST `/api/v1/admin/system/confirmations` with `{ operation: 'restore-backup', password, stagingId }` → expect `token`.
|
|
3. POST `/api/v1/admin/system/restore/commit` with `{ operation: 'restore-backup', token, stagingId }`
|
|
→ expect `{ ok: true, operation: 'restore-backup' }`, the live DB swapped,
|
|
and the admin session revoked (redirect to `/login`).
|
|
- The error path must still surface
|
|
`SYSTEM_RESTORE_VALIDATION_FAILED` for malformed JSON bodies.
|