AI Implementation feature(902): Admin Area Challenges: List, Import/Export and Add/Edit Modal 1.02 (#43)
This commit was merged in pull request #43.
This commit is contained in:
@@ -1,151 +0,0 @@
|
||||
# Implementation Plan: Job 901 — Admin Area Challenges: List, Import/Export and Add/Edit Modal 1.01
|
||||
|
||||
## 0. Status: Already-Implemented Audit (per Job spec)
|
||||
|
||||
The Job description explicitly identifies **which cases already pass** and which are bugs to fix. Per the README/architecture docs (`docs/guides/admin-challenges.md`), the **list, search, sort, import, export, delete, file staging, and discard-on-cancel** flows are fully implemented and verified by the existing test suites (`tests/frontend/admin-challenges-list.spec.ts`, `tests/frontend/admin-challenges-form.spec.ts`, `tests/backend/admin-challenges-*.spec.ts`). The cases the spec marks PASS (1, 2, 5, 7) are therefore considered already implemented.
|
||||
|
||||
**Cases that FAIL and need code changes:**
|
||||
- **Case 3** — Connection NC with blank/non-numeric Port: submit is silently rejected; `validateChallengeForm`'s `'Port is required for NC'` message is computed in `onOk()` but never surfaced as a `cf-error-port` (or general) inline error. Also `<input type=number>` strips non-digits before they reach the validator.
|
||||
- **Case 4** — Blank IP with NC: the form auto-fills `port` to `1337` and accepts the submission, but the requirement is that blank IP with NC should not silently succeed. (Re-read Job text: "no rejection — IP is optional and the form auto-fills port to 1337, so a blank IP with NC accepted submission with port 1337." — i.e. blank IP itself is OK; the issue is that auto-fill of `port` to `1337` masks the real "Port is required" error. The Job marks this as a separate failing case to be discussed.)
|
||||
- **Case 6** — Drag-and-drop oversized upload: the modal only wires `<input type=file>`. No `drop`/`dragover` handler exists.
|
||||
|
||||
The plan therefore covers only the **delta** required to fix cases 3, 4, 6, plus regression tests for those fixes.
|
||||
|
||||
## 1. Architectural Reconnaissance
|
||||
|
||||
- **Codebase style & conventions:**
|
||||
- TypeScript monorepo (`backend/` NestJS + `frontend/` Angular). `strict` TS across both.
|
||||
- Frontend uses **standalone components**, **OnPush**, Angular **signals** for component state, **Reactive Forms** with `FormBuilder.nonNullable.group(...)`.
|
||||
- Errors are surfaced via per-field signals `fieldErrors` (Record<field,string>) and a top-level `formError` signal; inline elements use `data-testid="cf-error-<field>"`.
|
||||
- Backend: NestJS modules; `admin-challenges.controller.ts` + `challenges.service.ts` + `challenge-files.service.ts`. Zod-validated DTOs. Already enforces size limit (`UPLOAD_SIZE_LIMIT` via `parseUploadSizeLimit`) and per-upload oversize rejection — verified by `tests/backend/uploads.spec.ts`.
|
||||
- Styling is component-scoped via `styles: [...]` literals (no global CSS edits required).
|
||||
- **Data Layer:** SQLite via better-sqlite3, file-based under `./data/` (persistent named volume `/data` per project policy).
|
||||
- **Test Framework & Structure:**
|
||||
- Jest with two projects (`tests/jest.config.js`): `backend` (node) and `frontend` (jsdom).
|
||||
- Single command from root: `npm test`.
|
||||
- Tests live in `tests/backend/*.spec.ts` and `tests/frontend/*.spec.ts` — **never** alongside source.
|
||||
- **Required Tools & Dependencies:** No new packages required. All tooling (Angular Forms, Reactive Forms, Jest jsdom, ts-jest) already present.
|
||||
|
||||
## 2. Impacted Files
|
||||
|
||||
- **To Modify:**
|
||||
- `frontend/src/app/features/admin/challenges/challenge-form-modal.component.ts`
|
||||
- Plumb `clientErrors` from `validateChallengeForm` into `fieldErrors` input (or a new local `clientFieldErrors` signal) so `cf-error-port` renders.
|
||||
- Replace the silent "abort" path in `onOk()` with: when `clientErrors` is non-empty, mark only the relevant controls touched/dirty (so untouched ones don't pop up), emit/show inline errors, switch to the tab that contains the first error, and short-circuit the submit. Clear these client errors when the user edits a field.
|
||||
- Change `<input id="cf-port" type="number" ...>` to `type="text" inputmode="numeric"` so non-numeric characters are not silently stripped; add `Validators.pattern(/^\d+$/)` and an integer validator (`Validators.min(1)`, `Validators.max(65535)`) on the `port` control so `<input type=number>` quirks don't hide the value. Validate numeric-ness in `validateChallengeForm` (it already checks `Number.isInteger`).
|
||||
- Stop the **protocol valueChanges side-effect** from auto-filling `port` to `1337` when switching to NC — instead leave it `null` so the pure validator can fire "Port is required for NC". (This addresses Case 4: blank IP should not cause silent success driven by auto-fill port; with NC, blank port must be rejected visibly.)
|
||||
- Add a `(drop)` / `(dragover)` / `(dragleave)` handler to the Files tab to support drag-and-drop uploads, reusing the existing `onFileChange` staging pipeline. Reject oversize files using the same `globalFileLimit` check.
|
||||
- Add `preventDefault` on `dragover` so the browser does not navigate to the file when dropped outside the input.
|
||||
- `frontend/src/app/features/admin/challenges/challenge-form-modal.component.ts` (template portion only):
|
||||
- Add a drag-and-drop zone (`<div class="drop-zone" (dragover)="onDragOver($event)" (drop)="onDrop($event)">`) wrapping the file input. Show visual state on dragover.
|
||||
- `frontend/src/app/features/admin/challenges/challenge-form.pure.ts`
|
||||
- Add a small helper `firstErrorTab(errors)` that maps a `ChallengeFormErrors` map to `'general' | 'connection' | 'files'` so the modal can auto-switch the active tab when a field is rejected.
|
||||
- Tighten `validateChallengeForm` for port: reject non-integer or out-of-range values for NC **even if** the user typed characters — already covered by `Number.isInteger`/`min`/`max`. (No logic change needed; just rely on it.)
|
||||
- `tests/frontend/admin-challenges-form.spec.ts`
|
||||
- Add regression specs for cases 3 and 4: validate that submitting NC with blank port surfaces `cf-error-port` text "Port is required for NC"; validate that the auto-fill-to-1337 behavior is removed so a blank NC port triggers the inline error.
|
||||
- Add a regression spec for case 6: simulate a `drop` event with a `DataTransfer` containing a small `File` and verify `AdminService.stageChallengeFile` is called and a new staged entry appears; also verify oversize drop is rejected via `cf-upload-error` without hitting the network.
|
||||
|
||||
- **To Create:**
|
||||
- **None.** All edits go into existing files. (No new files are required; we are extending existing modules per the Job "Add/Edit Modal 1.01" patch.)
|
||||
|
||||
## 3. Proposed Changes
|
||||
|
||||
### 3.1 Backend — no changes required
|
||||
- The server already returns `400 VALIDATION_FAILED` with `details[].path = "port"` for blank/non-numeric NC ports (`backend/src/modules/admin/dto/challenges.dto.ts` Zod schema enforces `port: z.number().int().min(1).max(65535).optional()` with conditional refinement for `NC`). The frontend simply must render the error, which is the responsibility of `validateChallengeForm` (client-side, before any network call).
|
||||
- The file-size limit is already enforced by `challenge-files.service.ts` (the staged upload endpoint returns 400 with a structured error envelope that the modal already maps to `cf-upload-error` in `onFileChange`).
|
||||
|
||||
### 3.2 Frontend — `challenge-form-modal.component.ts`
|
||||
|
||||
1. **Stop auto-filling `port` on protocol change.**
|
||||
- In the `protocol.valueChanges` subscriber (currently lines 373–381), remove the `else if (p === 'NC' && ...)` branch that sets `port` to `1337`. With this removed, switching to NC leaves `port === null`, so the next submit fires the `validateChallengeForm` pure-error `'Port is required for NC'` path, which we then surface (step 3).
|
||||
|
||||
2. **Surface `validateChallengeForm` errors inline.**
|
||||
- Add a private `clientFieldErrors = signal<ChallengeFormErrors | null>(null)`.
|
||||
- In `onOk()`:
|
||||
```ts
|
||||
const clientErrors = validateChallengeForm(values);
|
||||
if (Object.keys(clientErrors).length > 0) {
|
||||
this.clientFieldErrors.set(clientErrors);
|
||||
// mark only those controls touched so the existing `showError()` template renders
|
||||
for (const f of Object.keys(clientErrors) as (keyof ChallengeFormErrors)[]) {
|
||||
const ctrl = this.form.controls[f as keyof ChallengeFormControls];
|
||||
if (ctrl) { ctrl.markAsTouched(); ctrl.markAsDirty(); }
|
||||
}
|
||||
// auto-switch to the tab containing the first error
|
||||
const tab = firstErrorTab(clientErrors);
|
||||
if (tab) this.tab.set(tab);
|
||||
return;
|
||||
}
|
||||
this.clientFieldErrors.set(null);
|
||||
```
|
||||
- Extend `showError(field)` (currently lines 401–419) to **prefer** `clientFieldErrors()[field]` over server errors and over Angular built-in validator errors. The pure-helper message is the most specific; the existing fall-through to required/min/max remains for untouched-after-clearing flows.
|
||||
- Clear the per-field `clientFieldErrors` entry when the user edits the corresponding control: subscribe to `this.form.valueChanges` and on each change drop any key whose value now passes that field's pure check (cheap: re-run `validateChallengeForm` and diff). This keeps the error visible until the user actually fixes the input.
|
||||
|
||||
3. **Replace `<input type="number">` for port with `<input type="text" inputmode="numeric">`.**
|
||||
- Rationale: native `type=number` silently strips non-digits before Angular's FormControl sees them, which prevents `validateChallengeForm` from ever receiving a non-numeric value to reject. Using `type="text" inputmode="numeric"` keeps the on-screen numeric keypad on mobile but lets the validator surface a clear error.
|
||||
- Add to the `port` control: `Validators.pattern(/^\d+$/)` (in `buildChallengeFormGroup`) so the Angular built-in path also catches it, giving `showError('port')` an additional message via the existing `pattern` branch. (Add a `pattern` branch in `showError` returning `${humanLabel(field)} has an invalid value`.)
|
||||
|
||||
4. **Add drag-and-drop support to the Files tab.**
|
||||
- Template change (inside the `tab() === 'files'` block):
|
||||
```html
|
||||
<div class="drop-zone"
|
||||
[class.dragover]="dragOver()"
|
||||
(dragover)="onDragOver($event)"
|
||||
(dragleave)="onDragLeave($event)"
|
||||
(drop)="onDrop($event)"
|
||||
data-testid="cf-drop-zone">
|
||||
<p>Drop files here or use the picker below</p>
|
||||
<input id="cf-upload" type="file" multiple (change)="onFileChange($event)" data-testid="cf-upload" />
|
||||
</div>
|
||||
```
|
||||
- Component:
|
||||
```ts
|
||||
readonly dragOver = signal(false);
|
||||
onDragOver(ev: DragEvent): void { ev.preventDefault(); this.dragOver.set(true); }
|
||||
onDragLeave(ev: DragEvent): void { this.dragOver.set(false); }
|
||||
onDrop(ev: DragEvent): void {
|
||||
ev.preventDefault();
|
||||
this.dragOver.set(false);
|
||||
const files = ev.dataTransfer?.files ? Array.from(ev.dataTransfer.files) : [];
|
||||
if (files.length === 0) return;
|
||||
// Reuse the same staging pipeline as the picker:
|
||||
void this.stageFiles(files);
|
||||
}
|
||||
```
|
||||
Extract the existing `for (const file of list)` loop in `onFileChange` into a private `stageFiles(files: File[])` method; both `onFileChange` and `onDrop` call it. Oversize check, error message, and placeholder staging remain identical, so case 5 behavior is preserved and case 6 is now covered.
|
||||
|
||||
### 3.3 Frontend — `challenge-form.pure.ts`
|
||||
- Add `firstErrorTab(errors)` mapping `name|descriptionMd|categoryId|difficulty|initialPoints|minimumPoints|decaySolves|flag → 'general'`, `protocol|port|ipAddress → 'connection'`, empty otherwise.
|
||||
|
||||
### 3.4 Tests (regression only, in dedicated folder)
|
||||
Add cases to `tests/frontend/admin-challenges-form.spec.ts`:
|
||||
1. **Case 3 — blank NC port:**
|
||||
- Build the form, set values to a valid challenge but `protocol: 'NC'` and `port: null`.
|
||||
- Call the modal's `onOk()` indirectly: assert that after invoking submit, `clientFieldErrors` contains `port === 'Port is required for NC'` and the `cf-error-port` element renders that text (mount the component via `TestBed.createComponent`).
|
||||
- Assert that the port `<input>` no longer has `type="number"` (regression for non-numeric stripping).
|
||||
2. **Case 3 — non-numeric NC port:**
|
||||
- Simulate user typing `"12a3"` into the port field. Assert the input retains the raw string and `validateChallengeForm` returns `port: 'Port is required for NC'` (or new "must be an integer" message).
|
||||
3. **Case 4 — blank IP with NC, blank port:**
|
||||
- Verify the modal **does not** auto-fill port to 1337 when switching to NC, and that submitting with `ipAddress=''` and `port=null` shows the inline port error.
|
||||
4. **Case 6 — drag-and-drop:**
|
||||
- Stub `AdminService.stageChallengeFile` via TestBed providers; dispatch a synthetic `drop` event with a `File` in `dataTransfer.files`; assert the staged entry appears and `stageChallengeFile` was called.
|
||||
- Drop a file larger than `globalFileLimit`; assert `cf-upload-error` text appears and no network call is made.
|
||||
|
||||
(For new tests the existing `tests/frontend/admin-challenges-form.spec.ts` already imports from the component; we extend it in place rather than creating new files.)
|
||||
|
||||
## 4. Test Strategy
|
||||
|
||||
- **Target Unit Test File:**
|
||||
- `tests/frontend/admin-challenges-form.spec.ts` (existing; extend with the four cases above).
|
||||
- **Mocking Strategy:**
|
||||
- All external boundaries (the network) are mocked via Angular's `provideHttpClientTesting` plus a stub of `AdminService` for `stageChallengeFile` / `discardChallengeFile` / `createChallenge` / `updateChallenge`.
|
||||
- The drag-and-drop test stubs `AdminService.stageChallengeFile` with a Jasmine spy that resolves `{ token: 't-1', publicUrl: '/uploads/challenges/x', originalFilename, mimeType, sizeBytes }`.
|
||||
- For oversize, assert the spy is **not** called and `fileUploadError` is set, using a synthetic `File` built from a `Uint8Array` of the desired length.
|
||||
- Backend tests are not required because the server already rejects blank/non-numeric ports and oversize uploads (covered by `tests/backend/admin-challenges-api.spec.ts` and `tests/backend/uploads.spec.ts`).
|
||||
|
||||
## 5. Out of Scope / Non-changes
|
||||
|
||||
- No backend changes (server already returns the right error envelopes).
|
||||
- No new dependencies, no `setup.sh` edits, no migration scripts, no DB schema changes.
|
||||
- No changes to import/export, list, search, sort, delete flows (already passing per Job spec).
|
||||
- No drag-and-drop file picker behavior for import — that uses a `<input type=file>` chooser inside `AdminChallengesComponent.pickImportFile`, which the Job does not flag.
|
||||
@@ -0,0 +1,93 @@
|
||||
# Implementation Plan: Admin Add/Edit Challenge — Category Validation Bug Fix (Job 902)
|
||||
|
||||
## 1. Architectural Reconnaissance
|
||||
|
||||
- **Codebase style & conventions:**
|
||||
- Monorepo using npm workspaces (`backend`, `frontend`). TypeScript everywhere.
|
||||
- Backend: **NestJS 10** with TypeORM, SQLite (better-sqlite3), class-validator-style validation through custom `ZodValidationPipe`.
|
||||
- Frontend: **Angular 17** standalone components with `OnPush` change detection, Angular Signals + `effect()`, typed reactive forms (`FormBuilder.nonNullable`), RxJS for streams.
|
||||
- Stylistic choices: pure helper functions live in `*.pure.ts` next to the components; component classes only orchestrate state and templates.
|
||||
- Error envelopes use stable codes (`VALIDATION_FAILED`, `CHALLENGE_NAME_TAKEN`, `CHALLENGE_INVALID_CATEGORY`) returned from `ApiError.badRequest(...)` with a `details[]` of `{ path, message }`.
|
||||
|
||||
- **Data Layer:** SQLite via TypeORM (`better-sqlite3`). `challenge.category_id` is `TEXT NOT NULL` with `ON DELETE RESTRICT` FK to `category.id`. The `category.id` column is TEXT UUID v4 (see `database/challenges.md`).
|
||||
|
||||
- **Test Framework & Structure:**
|
||||
- **Jest 29** with `ts-jest`. Multi-project config in `tests/jest.config.js` with two projects: `backend` (node env) and `frontend` (jsdom env).
|
||||
- Frontend tests live in `tests/frontend/*.spec.ts`, backend tests in `tests/backend/*.spec.ts`. Tests NEVER live next to source.
|
||||
- Single command from the repo root: `npm test` (or `npm run test:backend` / `npm run test:frontend`).
|
||||
- Frontend tests focus on logic / pure functions and DOM-free component behavior (no visual confirmation). `tests/frontend/admin-challenges-form.spec.ts` already exercises `validateChallengeForm`, `buildChallengeFormGroup`, `syncChallengeForm`, etc.
|
||||
|
||||
- **Required Tools & Dependencies:** No new tools or dependencies are required for this fix. Zod 3 is already a backend dep; UUID validation can use the standard `z.string().uuid()`. No new npm packages needed. `setup.sh` requires no changes.
|
||||
|
||||
## 2. Impacted Files
|
||||
|
||||
- **To Modify:**
|
||||
- `frontend/src/app/features/admin/challenges/challenge-form-modal.component.ts` — add a dedicated early-return guard for empty `categoryId` and a UUID-format validator on the control; tighten `onOk()`.
|
||||
- `frontend/src/app/features/admin/challenges/challenge-form.pure.ts` — add a `categoryUuidPattern` constant and a `CATEGORY_ID_REQUIRED` helper constant; tighten `validateChallengeForm` to flag any non-UUID value as a category error (defense in depth).
|
||||
- `backend/src/modules/admin/dto/challenges.dto.ts` — change `categoryId` in `baseShape` from `z.string().min(1).max(64)` to `z.string().uuid()` so an empty string, whitespace, or non-UUID payload is rejected at the validation pipe with a `400 VALIDATION_FAILED` and a `categoryId` detail.
|
||||
|
||||
- **To Create:**
|
||||
- `tests/frontend/admin-challenges-form-job902.spec.ts` — targeted unit tests covering the new client-side guards.
|
||||
- `tests/backend/admin-challenges-create-category-job902.spec.ts` — targeted integration test covering the new server-side Zod rejection of empty/non-UUID `categoryId` on `POST /api/v1/admin/challenges`.
|
||||
|
||||
## 3. Proposed Changes
|
||||
|
||||
### 1. Database / Schema Migration
|
||||
No schema migration. The `category_id` column already requires a valid existing row (FK + service-level `assertCategoryExists`). The fix is purely validation tightening on the wire format. The `RepairCategorySchemaAndSystemCategories1700000000400` and `UpgradeChallengeAdminSchema1700000000500` migrations remain untouched.
|
||||
|
||||
### 2. Backend Logic & APIs (`backend/src/modules/admin/dto/challenges.dto.ts`)
|
||||
- Update `baseShape.categoryId` from `z.string().min(1).max(64)` to `z.string().uuid({ message: 'Category is required' })`.
|
||||
- This single change makes the backend reject:
|
||||
- Empty string (`""`) → caught at the Zod pipe → `400 VALIDATION_FAILED` with `details: [{ path: 'categoryId', message: 'Category is required' }]`.
|
||||
- Whitespace-only → still rejected by `z.string().uuid()` (UUIDs cannot contain whitespace).
|
||||
- Any non-UUID string (including stray placeholders) → rejected.
|
||||
- `CategoryIdParamSchema` for `GET/PUT/DELETE /:id` already uses `z.string().min(1).max(64)`; for consistency, also tighten it to `z.string().uuid()` so that a missing/invalid id route produces a uniform 400.
|
||||
- The `ImportChallengeSchema` (used by import payloads) already passes a `categorySystemKey`, not `categoryId`, so it is unaffected.
|
||||
- `mapServerFieldErrors` in `challenge-form.pure.ts` already maps `path === 'categoryId'` to the per-field error, so the inline `cf-error-category` will render automatically once the backend returns a `categoryId` detail.
|
||||
|
||||
### 3. Frontend UI Integration (`frontend/src/app/features/admin/challenges/challenge-form-modal.component.ts`)
|
||||
- In `buildChallengeFormGroup`, replace the bare `Validators.required` on `categoryId` with a stronger validator set that:
|
||||
1. Rejects empty strings (`Validators.required`).
|
||||
2. Enforces UUID v4 format using a small inline regex (e.g. `/^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i`) wrapped in a named validator error key (`uuid`) so `showError('categoryId')` can render a meaningful message.
|
||||
- In `onOk()`:
|
||||
- Read `categoryId = this.form.controls.categoryId.value` BEFORE calling `validateChallengeForm`.
|
||||
- Add an explicit defensive guard that returns immediately when the trimmed value is empty or does not match the UUID pattern, sets `clientFieldErrors = { categoryId: 'Category is required' }`, marks the control touched/dirty, switches the active tab to `'general'` via `firstErrorTab`, and does NOT call `submit.emit(...)`. This is in addition to `validateChallengeForm` (which already catches the empty case) — it exists as a belt-and-suspenders measure so the bug cannot recur even if the pure helper is bypassed.
|
||||
- Keep the existing template — `<option value="">Select a category…</option>` — and the existing `[data-testid="cf-category"]` selector untouched.
|
||||
|
||||
### 4. Frontend Pure Helper (`frontend/src/app/features/admin/challenges/challenge-form.pure.ts`)
|
||||
- Export a new constant `CATEGORY_ID_PATTERN = /^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i`.
|
||||
- In `validateChallengeForm`, expand the category branch:
|
||||
```ts
|
||||
if (!values.categoryId) errs.categoryId = 'Category is required';
|
||||
else if (!CATEGORY_ID_PATTERN.test(values.categoryId))
|
||||
errs.categoryId = 'Category is required';
|
||||
```
|
||||
(Same message on purpose: do not leak internal format expectations to the user; both states mean "you have not selected a valid category".)
|
||||
- Add `firstErrorTab` already returns `'general'` for `errors.categoryId` — no change needed there.
|
||||
|
||||
## 4. Test Strategy
|
||||
|
||||
- **Target Unit Test File (frontend):** `tests/frontend/admin-challenges-form-job902.spec.ts`
|
||||
- Reuse the existing `makeDefaults()` helper pattern from `tests/frontend/admin-challenges-form.spec.ts`.
|
||||
- Test 1 — `validateChallengeForm({ categoryId: '' })` produces `errs.categoryId === 'Category is required'`.
|
||||
- Test 2 — `validateChallengeForm({ categoryId: ' ' })` produces the same error (whitespace string).
|
||||
- Test 3 — `validateChallengeForm({ categoryId: 'not-a-uuid' })` produces the same error (defense-in-depth against non-UUID garbage).
|
||||
- Test 4 — `validateChallengeForm({ categoryId: '<valid uuid>' })` does NOT produce a `categoryId` error.
|
||||
- Test 5 — `buildChallengeFormGroup` applies a `uuid` validator that rejects `'abc'`, `'not-a-uuid'`, and `''` once the control is touched; accepts a canonical UUID v4.
|
||||
- Test 6 — `firstErrorTab({ categoryId: '...' })` returns `'general'` (so the modal switches tabs to surface the error).
|
||||
|
||||
- **Target Integration Test File (backend):** `tests/backend/admin-challenges-create-category-job902.spec.ts`
|
||||
- Follow the boot pattern from `tests/backend/admin-challenges-api.spec.ts` (register first admin, prime CSRF, login).
|
||||
- Test 1 — `POST /api/v1/admin/challenges` with `categoryId: ''` and otherwise-valid body returns `400 VALIDATION_FAILED` with `details` containing a `categoryId` entry; no row is persisted (re-list and assert empty array).
|
||||
- Test 2 — `POST /api/v1/admin/challenges` with `categoryId: 'not-a-uuid'` and otherwise-valid body returns `400 VALIDATION_FAILED` with a `categoryId` detail.
|
||||
- Test 3 — `POST /api/v1/admin/challenges` with a valid UUID for `categoryId` (matching a seeded category) returns `201` and the row appears in the list.
|
||||
|
||||
- **Mocking Strategy:**
|
||||
- **Frontend tests:** No HTTP/network mocking required — they exercise pure helpers and the in-memory `FormGroup` from `@angular/forms`. The existing `StubAdminService` pattern from `tests/frontend/admin-challenges-form.spec.ts` can be referenced if needed but is not required for the new tests above.
|
||||
- **Backend tests:** Use the real `AppModule` with `process.env.DATABASE_PATH = ':memory:'`, real `GlobalExceptionFilter`, real `CsrfMiddleware`, and real admin login flow (same pattern as `tests/backend/admin-challenges-api.spec.ts`). Seed at least one category row through the existing `/api/v1/admin/categories` POST so the "valid UUID" path can succeed; rely on `category-repair-migration` to materialize the canonical system rows on fresh DBs.
|
||||
|
||||
- **Run command (single, root-level):**
|
||||
```bash
|
||||
npm test
|
||||
```
|
||||
This runs both projects (`backend` and `frontend`) via `tests/jest.config.js`. For narrower loops during development: `npm run test:backend` or `npm run test:frontend`.
|
||||
@@ -15,12 +15,12 @@ export const ChallengeDifficultySchema = z.enum(DIFFICULTIES);
|
||||
export const ChallengeProtocolSchema = z.enum(PROTOCOLS);
|
||||
|
||||
export const ChallengeIdParamSchema = z.object({
|
||||
id: z.string().min(1).max(64),
|
||||
id: z.string().uuid({ message: 'Invalid challenge id' }),
|
||||
});
|
||||
|
||||
export const ListChallengesQuerySchema = z.object({
|
||||
q: z.string().trim().min(1).max(128).optional(),
|
||||
categoryId: z.string().min(1).max(64).optional(),
|
||||
categoryId: z.string().uuid({ message: 'Invalid category id' }).optional(),
|
||||
});
|
||||
export type ListChallengesQuery = z.infer<typeof ListChallengesQuerySchema>;
|
||||
|
||||
@@ -44,7 +44,7 @@ const portField = z
|
||||
|
||||
const baseShape = {
|
||||
descriptionMd: z.string().max(64_000),
|
||||
categoryId: z.string().min(1).max(64),
|
||||
categoryId: z.string().uuid({ message: 'Category is required' }),
|
||||
difficulty: ChallengeDifficultySchema,
|
||||
initialPoints: z.number().int().min(1).max(100000),
|
||||
minimumPoints: z.number().int().min(1).max(100000),
|
||||
|
||||
@@ -11,7 +11,7 @@ import {
|
||||
signal,
|
||||
} from '@angular/core';
|
||||
import { CommonModule } from '@angular/common';
|
||||
import { FormBuilder, FormControl, FormGroup, ReactiveFormsModule, Validators } from '@angular/forms';
|
||||
import { AbstractControl, FormBuilder, FormControl, FormGroup, ReactiveFormsModule, ValidationErrors, Validators } from '@angular/forms';
|
||||
import { takeUntilDestroyed } from '@angular/core/rxjs-interop';
|
||||
import {
|
||||
AdminCategory,
|
||||
@@ -25,6 +25,7 @@ import {
|
||||
ChallengeFormErrors,
|
||||
ChallengeFormFileItem,
|
||||
ChallengeStagedFileUi,
|
||||
CATEGORY_ID_PATTERN,
|
||||
buildInitialFiles,
|
||||
defaultMinimumPoints,
|
||||
deriveMinimumPoints,
|
||||
@@ -61,7 +62,7 @@ export function buildChallengeFormGroup(fb: FormBuilder): FormGroup<ChallengeFor
|
||||
return fb.nonNullable.group({
|
||||
name: fb.nonNullable.control('', [Validators.required, Validators.maxLength(128)]),
|
||||
descriptionMd: fb.nonNullable.control('', [Validators.required]),
|
||||
categoryId: fb.nonNullable.control('', [Validators.required]),
|
||||
categoryId: fb.nonNullable.control('', [Validators.required, categoryIdUuidValidator]),
|
||||
difficulty: fb.nonNullable.control<'LOW' | 'MEDIUM' | 'HIGH'>('MEDIUM'),
|
||||
initialPoints: fb.nonNullable.control(100, [Validators.required, Validators.min(1), Validators.max(100000)]),
|
||||
minimumPoints: fb.nonNullable.control(50, [Validators.required, Validators.min(1)]),
|
||||
@@ -439,6 +440,9 @@ export class ChallengeFormModalComponent {
|
||||
if (ctrl.errors?.['pattern']) {
|
||||
return `${humanLabel(field)} must be a whole number`;
|
||||
}
|
||||
if (ctrl.errors?.['uuid']) {
|
||||
return `${humanLabel(field)} is required`;
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
@@ -521,6 +525,15 @@ export class ChallengeFormModalComponent {
|
||||
this.fileUploadError.set('Wait for uploads to finish before saving.');
|
||||
return;
|
||||
}
|
||||
const categoryRaw = this.form.controls.categoryId.value;
|
||||
if (!categoryRaw || !CATEGORY_ID_PATTERN.test(categoryRaw)) {
|
||||
this.clientFieldErrors.set({ categoryId: 'Category is required' });
|
||||
this.form.controls.categoryId.markAsTouched();
|
||||
this.form.controls.categoryId.markAsDirty();
|
||||
this.form.controls.categoryId.setErrors({ ...(this.form.controls.categoryId.errors ?? {}), uuid: true });
|
||||
this.tab.set('general');
|
||||
return;
|
||||
}
|
||||
const portRaw = this.form.controls.port.value;
|
||||
const portAsNumber =
|
||||
portRaw === null || portRaw === undefined || portRaw === ''
|
||||
@@ -604,5 +617,13 @@ function humanLabel(field: keyof ChallengeFormErrors): string {
|
||||
}
|
||||
}
|
||||
|
||||
function categoryIdUuidValidator(control: AbstractControl): ValidationErrors | null {
|
||||
const raw = control.value;
|
||||
if (raw === null || raw === undefined || raw === '') {
|
||||
return null;
|
||||
}
|
||||
return CATEGORY_ID_PATTERN.test(String(raw)) ? null : { uuid: true };
|
||||
}
|
||||
|
||||
// Re-export so tests can find default helpers.
|
||||
export { defaultMinimumPoints };
|
||||
@@ -11,6 +11,8 @@ export type Protocol = 'NC' | 'WEB';
|
||||
export const DEFAULT_INITIAL_POINTS = 100;
|
||||
export const DEFAULT_DECAY_SOLVES = 10;
|
||||
|
||||
export const CATEGORY_ID_PATTERN = /^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i;
|
||||
|
||||
export function defaultMinimumPoints(initial: number): number {
|
||||
if (!Number.isFinite(initial) || initial < 1) return 1;
|
||||
return Math.max(1, Math.floor(initial / 2));
|
||||
@@ -83,7 +85,9 @@ export function validateChallengeForm(values: ChallengeFormDefaults): ChallengeF
|
||||
if (!values.name.trim()) errs.name = 'Name is required';
|
||||
else if (values.name.length > 128) errs.name = 'Name must be 128 characters or fewer';
|
||||
if (!values.descriptionMd.trim()) errs.descriptionMd = 'Description is required';
|
||||
if (!values.categoryId) errs.categoryId = 'Category is required';
|
||||
if (!values.categoryId || !CATEGORY_ID_PATTERN.test(values.categoryId)) {
|
||||
errs.categoryId = 'Category is required';
|
||||
}
|
||||
if (!Number.isInteger(values.initialPoints) || values.initialPoints < 1 || values.initialPoints > 100000) {
|
||||
errs.initialPoints = 'Initial points must be between 1 and 100000';
|
||||
}
|
||||
|
||||
@@ -0,0 +1,142 @@
|
||||
process.env.DATABASE_PATH = ':memory:';
|
||||
process.env.THEMES_DIR = './themes';
|
||||
process.env.FRONTEND_DIST = './frontend/dist';
|
||||
process.env.UPLOAD_DIR = '/tmp/hipctf-challenges-create-category-job902';
|
||||
|
||||
import * as fs from 'fs';
|
||||
import * as os from 'os';
|
||||
import * as path from 'path';
|
||||
import { Test } from '@nestjs/testing';
|
||||
import { INestApplication } 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 { getRepositoryToken } from '@nestjs/typeorm';
|
||||
import { Repository } from 'typeorm';
|
||||
import { CategoryEntity } from '../../backend/src/database/entities/category.entity';
|
||||
import { initDb } from './db-helper';
|
||||
|
||||
describe('Admin Challenges API - create with empty/invalid categoryId (Job 902)', () => {
|
||||
let app: INestApplication;
|
||||
let adminToken: string;
|
||||
const uploadDir = fs.mkdtempSync(path.join(os.tmpdir(), 'hipctf-challenges-create-cat-'));
|
||||
let validCategoryId: string;
|
||||
|
||||
async function primeCsrf(): Promise<{ agent: any; csrf: string }> {
|
||||
const server = app.getHttpServer();
|
||||
const agent = request.agent(server);
|
||||
await agent.get('/api/v1/auth/csrf');
|
||||
const cookies: any = agent.jar.getCookies(CookieAccessInfo.All);
|
||||
return { agent, csrf: cookies.find((c: any) => c.name === 'csrf').value };
|
||||
}
|
||||
|
||||
function wellFormedBody(categoryId: string): any {
|
||||
return {
|
||||
name: 'Missing Category Attempt',
|
||||
descriptionMd: 'desc',
|
||||
categoryId,
|
||||
difficulty: 'MEDIUM',
|
||||
initialPoints: 100,
|
||||
minimumPoints: 50,
|
||||
decaySolves: 10,
|
||||
flag: 'flag{X}',
|
||||
protocol: 'WEB',
|
||||
port: null,
|
||||
ipAddress: '',
|
||||
enabled: true,
|
||||
};
|
||||
}
|
||||
|
||||
beforeAll(async () => {
|
||||
process.env.UPLOAD_DIR = uploadDir;
|
||||
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 (require('@nestjs/common').ValidationPipe)({ whitelist: true, forbidNonWhitelisted: false, transform: true }));
|
||||
const httpAdapterHost = app.get(HttpAdapterHost);
|
||||
app.useGlobalFilters(new GlobalExceptionFilter(httpAdapterHost));
|
||||
await app.init();
|
||||
await initDb(app);
|
||||
|
||||
const catRepo: Repository<CategoryEntity> = app.get(getRepositoryToken(CategoryEntity));
|
||||
const cry = await catRepo.findOne({ where: { systemKey: 'CRY' } });
|
||||
if (!cry) throw new Error('expected CRY category seed');
|
||||
validCategoryId = cry.id;
|
||||
|
||||
const server = app.getHttpServer();
|
||||
await request(server).post('/api/v1/auth/register-first-admin')
|
||||
.send({ username: 'admin', password: 'Sup3rSecret!Pass' })
|
||||
.expect(201);
|
||||
|
||||
const csrfPrime = await request(server).get('/api/v1/auth/csrf');
|
||||
const csrfCookie = (csrfPrime.headers['set-cookie'] as unknown as string[] | undefined)?.find((c) => c.startsWith('csrf='));
|
||||
const csrfToken = csrfCookie ? decodeURIComponent(csrfCookie.split(';')[0].split('=')[1]) : '';
|
||||
|
||||
const login = await request(server).post('/api/v1/auth/login')
|
||||
.set('Cookie', `csrf=${csrfToken}`)
|
||||
.set('X-CSRF-Token', csrfToken)
|
||||
.send({ username: 'admin', password: 'Sup3rSecret!Pass' })
|
||||
.expect(201);
|
||||
adminToken = login.body.accessToken;
|
||||
});
|
||||
|
||||
afterAll(async () => {
|
||||
await app.close();
|
||||
if (fs.existsSync(uploadDir)) {
|
||||
try {
|
||||
fs.rmSync(uploadDir, { recursive: true, force: true });
|
||||
} catch {
|
||||
// ignore
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
it('rejects an empty categoryId with 400 VALIDATION_FAILED and a categoryId detail', async () => {
|
||||
const { agent, csrf } = await primeCsrf();
|
||||
const res = await agent
|
||||
.post('/api/v1/admin/challenges')
|
||||
.set('Authorization', `Bearer ${adminToken}`)
|
||||
.set('X-CSRF-Token', csrf)
|
||||
.send(wellFormedBody(''));
|
||||
expect(res.status).toBe(400);
|
||||
expect(res.body?.code).toBe('VALIDATION_FAILED');
|
||||
const details = Array.isArray(res.body?.details) ? res.body.details : [];
|
||||
const categoryDetail = details.find((d: any) => d.path === 'categoryId');
|
||||
expect(categoryDetail).toBeDefined();
|
||||
});
|
||||
|
||||
it('rejects a non-UUID categoryId with 400 VALIDATION_FAILED and a categoryId detail', async () => {
|
||||
const { agent, csrf } = await primeCsrf();
|
||||
const res = await agent
|
||||
.post('/api/v1/admin/challenges')
|
||||
.set('Authorization', `Bearer ${adminToken}`)
|
||||
.set('X-CSRF-Token', csrf)
|
||||
.send(wellFormedBody('not-a-uuid'));
|
||||
expect(res.status).toBe(400);
|
||||
expect(res.body?.code).toBe('VALIDATION_FAILED');
|
||||
const details = Array.isArray(res.body?.details) ? res.body.details : [];
|
||||
const categoryDetail = details.find((d: any) => d.path === 'categoryId');
|
||||
expect(categoryDetail).toBeDefined();
|
||||
});
|
||||
|
||||
it('accepts a valid UUID categoryId and returns 201', async () => {
|
||||
const { agent, csrf } = await primeCsrf();
|
||||
const res = await agent
|
||||
.post('/api/v1/admin/challenges')
|
||||
.set('Authorization', `Bearer ${adminToken}`)
|
||||
.set('X-CSRF-Token', csrf)
|
||||
.send(wellFormedBody(validCategoryId));
|
||||
expect(res.status).toBe(201);
|
||||
expect(res.body?.id).toBeDefined();
|
||||
expect(res.body?.categoryId).toBe(validCategoryId);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,83 @@
|
||||
import '@angular/compiler';
|
||||
import { FormBuilder } from '@angular/forms';
|
||||
import {
|
||||
buildChallengeFormGroup,
|
||||
} from '../../frontend/src/app/features/admin/challenges/challenge-form-modal.component';
|
||||
import {
|
||||
CATEGORY_ID_PATTERN,
|
||||
ChallengeFormDefaults,
|
||||
firstErrorTab,
|
||||
validateChallengeForm,
|
||||
} from '../../frontend/src/app/features/admin/challenges/challenge-form.pure';
|
||||
|
||||
function makeDefaults(overrides: Partial<ChallengeFormDefaults> = {}): ChallengeFormDefaults {
|
||||
return {
|
||||
name: 'Missing Category Attempt',
|
||||
descriptionMd: 'desc',
|
||||
categoryId: '11111111-1111-4111-8111-111111111111',
|
||||
difficulty: 'MEDIUM',
|
||||
initialPoints: 100,
|
||||
minimumPoints: 50,
|
||||
decaySolves: 10,
|
||||
flag: 'flag{X}',
|
||||
protocol: 'WEB',
|
||||
port: null,
|
||||
ipAddress: '',
|
||||
enabled: true,
|
||||
...overrides,
|
||||
};
|
||||
}
|
||||
|
||||
const VALID_CATEGORY_ID = '11111111-1111-4111-8111-111111111111';
|
||||
|
||||
describe('Job 902: challenge form Category validation', () => {
|
||||
it('CATEGORY_ID_PATTERN accepts a canonical UUID v4', () => {
|
||||
expect(CATEGORY_ID_PATTERN.test(VALID_CATEGORY_ID)).toBe(true);
|
||||
});
|
||||
|
||||
it('validateChallengeForm flags empty categoryId', () => {
|
||||
const errs = validateChallengeForm(makeDefaults({ categoryId: '' }));
|
||||
expect(errs.categoryId).toBe('Category is required');
|
||||
});
|
||||
|
||||
it('validateChallengeForm flags whitespace-only categoryId', () => {
|
||||
const errs = validateChallengeForm(makeDefaults({ categoryId: ' ' }));
|
||||
expect(errs.categoryId).toBe('Category is required');
|
||||
});
|
||||
|
||||
it('validateChallengeForm flags non-UUID categoryId (defense in depth)', () => {
|
||||
const errs = validateChallengeForm(makeDefaults({ categoryId: 'not-a-uuid' }));
|
||||
expect(errs.categoryId).toBe('Category is required');
|
||||
});
|
||||
|
||||
it('validateChallengeForm accepts a well-formed UUID categoryId', () => {
|
||||
const errs = validateChallengeForm(makeDefaults());
|
||||
expect(errs.categoryId).toBeUndefined();
|
||||
});
|
||||
|
||||
it('firstErrorTab routes a categoryId error to the general tab', () => {
|
||||
expect(firstErrorTab({ categoryId: 'Category is required' })).toBe('general');
|
||||
});
|
||||
|
||||
it('buildChallengeFormGroup applies a uuid validator on categoryId', () => {
|
||||
const form = buildChallengeFormGroup(new FormBuilder());
|
||||
form.controls.categoryId.setValue('not-a-uuid');
|
||||
form.controls.categoryId.markAsTouched();
|
||||
expect(form.controls.categoryId.errors?.['uuid']).toBe(true);
|
||||
expect(form.controls.categoryId.errors?.['required']).toBeUndefined();
|
||||
});
|
||||
|
||||
it('buildChallengeFormGroup uuid validator passes for a valid UUID v4', () => {
|
||||
const form = buildChallengeFormGroup(new FormBuilder());
|
||||
form.controls.categoryId.setValue(VALID_CATEGORY_ID);
|
||||
form.controls.categoryId.markAsTouched();
|
||||
expect(form.controls.categoryId.errors?.['uuid']).toBeUndefined();
|
||||
});
|
||||
|
||||
it('buildChallengeFormGroup categoryId control still flags required for empty value', () => {
|
||||
const form = buildChallengeFormGroup(new FormBuilder());
|
||||
form.controls.categoryId.setValue('');
|
||||
form.controls.categoryId.markAsTouched();
|
||||
expect(form.controls.categoryId.errors?.['required']).toBe(true);
|
||||
});
|
||||
});
|
||||
@@ -27,7 +27,7 @@ function makeDetail(overrides: Partial<AdminChallengeDetail> = {}): AdminChallen
|
||||
id: 'd1',
|
||||
name: 'Sample',
|
||||
descriptionMd: '# hi',
|
||||
categoryId: 'cat-1',
|
||||
categoryId: '11111111-1111-4111-8111-111111111111',
|
||||
categoryAbbreviation: 'CRY',
|
||||
categoryIconPath: '/uploads/icons/CRY.png',
|
||||
difficulty: 'MEDIUM',
|
||||
@@ -58,7 +58,7 @@ function makeDefaults(overrides: Partial<ChallengeFormDefaults> = {}): Challenge
|
||||
return {
|
||||
name: 'X',
|
||||
descriptionMd: 'd',
|
||||
categoryId: 'cat-1',
|
||||
categoryId: '11111111-1111-4111-8111-111111111111',
|
||||
difficulty: 'MEDIUM',
|
||||
initialPoints: 100,
|
||||
minimumPoints: 50,
|
||||
|
||||
Reference in New Issue
Block a user