Files
HIPCTF2/.kilo/plans/903.md
T

12 KiB
Raw Blame History

Implementation Plan: Job 903 — Admin Challenges Import must Replace, Modal must Auto-Close

1. Architectural Reconnaissance

  • Codebase style & conventions:

    • Backend: NestJS (TypeScript, strict, class-based Injectable services, TypeORM with better-sqlite3, async/await, controllers delegating to services).
    • Frontend: Angular 20 standalone components with ChangeDetectionStrategy.OnPush, signal input() / output(), @if/@for control flow, RxJS only for the search debounce, smart/dumb split. Pure helpers live in *.pure.ts and are tested directly without a TestBed.
    • Tests: Jest + ts-jest configured in tests/jest.config.js as two projects (backend, frontend). Backend specs use Test.createTestingModule with in-memory SQLite and a unique mkdtempSync upload dir per file (no shared FS races). Frontend specs in tests/frontend/ import pure helpers directly (no DOM) — the only TestBed specs are for components that must exercise @Input bindings.
    • npm test at the repo root runs all tests for all components via the Jest project selector.
  • Data Layer:

    • SQLite via TypeORM; relevant tables challenge, challenge_file, category, solve.
    • File storage: data/uploads/challenges/<uuid> for committed files, .staging/<uuid> for in-flight uploads.
    • Repository injection via @InjectRepository(Entity); transactions via this.dataSource.transaction(async (manager) => { ... }).
  • Test Framework & Structure:

    • Backend tests in /repo/tests/backend/*.spec.ts (Jest, testEnvironment: 'node').
    • Frontend tests in /repo/tests/frontend/*.spec.ts (Jest + jsdom); pure-helper specs do not need TestBed.
    • Single command: npm test (already wired in root package.json).
    • New specs to add: one backend spec for the replace-all-on-confirmed-import behavior, one frontend spec for the auto-close + list-refresh behavior of the import modal. Both must be runnable with npm test.
  • Required Tools & Dependencies: No new tools or packages. Existing better-sqlite3, TypeORM, NestJS, Angular, and Jest setup is sufficient.

2. Impacted Files

  • To Modify:

    • backend/src/modules/admin/challenges.service.ts — change importConfirmed() so it wipes every pre-existing challenge (and cascade-deletes their challenge_file + solve rows) before re-inserting from the document. Re-link the now-unreferenced disk files for unlink and unlink any orphaned file rows after the swap.
    • frontend/src/app/features/admin/challenges/challenges.component.ts — after a successful confirmed import (onImportConfirm), call closeImport() so the dialog disappears (it currently stays open showing the result message and a manual Close button).
    • docs/guides/admin-challenges.md — clarify that Import with ?confirm=overwrite replaces the entire challenge set (not just upserts by name). Update the "Importing the same document twice is idempotent" line to state the full-replace semantics.
    • docs/architecture/key-files.md — update the one-line responsibility for challenges.service.ts to mention the replace-on-confirmed-import contract.
  • To Create:

    • tests/backend/admin-challenges-import-replace.spec.ts — focused contract test asserting that an overwrite-confirmed import removes pre-existing challenges (including their files and solves) and leaves only the imported set; checks both DB and the on-disk challenges/ directory.
    • tests/frontend/admin-challenges-import-autoclose.spec.ts — pure/smart test asserting that after a successful onImportConfirm, the import modal closes (i.e. importOpen() flips to false) and the result is still surfaced via the page-level status banner.

3. Proposed Changes

3.1 Backend — AdminChallengesService.importConfirmed becomes a full replace

In backend/src/modules/admin/challenges.service.ts, refactor importConfirmed (currently lines 412522) so the confirmed path:

  1. Stage every file up front (unchanged logic, lines 421446). Keep the early rollbackStaged on validation failure.
  2. Inside one transaction (unchanged transaction wrapper, line 450): a. Load every existing challenge row with manager.getRepository(ChallengeEntity).find() (no filter). b. Capture each existing challenge's ChallengeFileEntity rows (find({ where: { challengeId: In(ids) } })) so we know which storedFilenames are still referenced after the transaction. c. Delete every existing challenge row with a single manager.delete(ChallengeEntity, {}) — TypeORM cascades to challenge_file and solve rows via the existing FK cascade. This is the replace semantics. d. Then iterate doc.challenges and (as today) skip entries whose category is unknown, otherwise create a new challenge row + new file rows pointing at the already-staged tokens. Result counts stay { imported, skipped, warnings }.
  3. After commit, clean up disk — for the captured pre-existing stored filenames, call fileService.removeFinal(storedFilename) best-effort (same helper used by remove()). Newly-inserted challenges' files are then committed via the existing commit(item.staged.token) loop (lines 506517).
  4. Do not break the preview path. previewImport keeps the existing conflict/unknown-category logic; the destructive replace only happens when ?confirm=overwrite (controller path admin-challenges.controller.ts:87-92).
  5. No changes to DTOs or to the export shape. exportAll() already produces the versioned envelope that the new behavior requires for round-tripping.

Edge cases to handle explicitly:

  • Empty doc.challenges ⇒ result { imported: 0, skipped: [], warnings: [] }; existing rows are wiped, files unlinked, no new rows created.
  • Unknown-category rows ⇒ counted in skipped; do not preserve them, do not abort the wipe. The wipe happens before the per-challenge inserts.
  • Disk-unlink failure for a removed file ⇒ swallow + log via ChallengeFilesService.removeFinal (matches existing delete behavior, never throws).

3.2 Frontend — AdminChallengesComponent.onImportConfirm closes the modal

In frontend/src/app/features/admin/challenges/challenges.component.ts:

  • In onImportConfirm() (lines 455470), after await this.load() succeeds and importResult is set, additionally call closeImport(). closeImport() already clears importDoc, importPreview, importResult, and importError (lines 447453) and resets importOpen to false.
  • The page-level status banner is already populated by this.statusMessage.set(summarizeImportResult(result)) immediately before closeImport(), so the user still sees the "Imported N challenges" message after the dialog closes.
  • The <app-challenge-import-modal> template already renders a "Close" button while result() is set; that branch becomes unreachable after the fix (the modal will close before the user can click it), so no template change is required. The result branch is kept as a defensive fallback for any future flow that intentionally keeps the modal open (e.g. partial-failure display).

3.3 Docs — clarify the replace semantics

  • docs/guides/admin-challenges.md — under the "Import" section (line 103) replace the third bullet ("Click Overwrite & Import … the server upserts challenges inside one transaction by case-insensitive name") with: "Click Overwrite & Import → the client posts the same document with ?confirm=overwrite. The server stages every file up front (rolling back any FS failure), then wipes every existing challenge (cascade-deleting their challenge_file and solve rows and best-effort unlinking their disk files) and inserts the archive's challenges in one transaction."
  • Update the last "Notes" line on idempotency (line 138) to: "Importing the same document twice is idempotent: every existing challenge (matching or not) is replaced by the archive contents."
  • docs/architecture/key-files.md — update the challenges.service.ts responsibility to "... CRUD, list aggregates, transactional deletion, export, full-replace import orchestration."

4. Test Strategy

4.1 Backend contract test — tests/backend/admin-challenges-import-replace.spec.ts

  • Mirror the wiring of tests/backend/admin-challenges-import-export.spec.ts: :memory: SQLite, unique mkdtempSync(os.tmpdir() + '/hipctf-challenges-import-replace-…') UPLOAD_DIR, all migrations including UpgradeChallengeAdminSchema1700000000500, TypeOrmModule.forFeature for CategoryEntity, ChallengeEntity, ChallengeFileEntity, SolveEntity.
  • AAA structure. Mocking strategy: no external boundaries — use real in-memory SQLite + real ChallengeFilesService writing to the temp dir. Isolation: per-file temp dir (no parallel FS races).
  • Tests (minimal, one logical assertion each):
    1. Replace wipes unrelated challenges — Seed Alpha (CRY, NC, port=1337, flag='flag{a}', with one staged file committed) and Bravo (CRY, WEB). Call importConfirmed with a single-challenge doc { name: 'Imported Charlie', … categorySystemKey: 'CRY', … }. Assert list({}).map(r => r.name) is exactly ['Imported Charlie'] and that chRepo.count() equals 1.
    2. Disk files of wiped challenges are unlinked — Capture the stored filename of Alpha's committed file via fileRepo.findOne({ where: { challengeId: alpha.id } }). After the import, assert fs.existsSync(path.join(finalDir, alphaStoredName)) is false, while the new challenge's stored file exists.
    3. Empty archive wipes everything — Re-seed Alpha, then importConfirmed with { challenges: [] }. Assert chRepo.count() === 0 and the previously-committed disk file is unlinked.
    4. Unknown-category rows are skipped, not preserved — Seed Keep (WEB). Import a doc containing one unknown-category challenge plus nothing else. Assert result.skipped has length 1 and list({}) is empty (the wipe still happened).

4.2 Frontend modal test — tests/frontend/admin-challenges-import-autoclose.spec.ts

  • Smart container test using TestBed.createComponent(AdminChallengesComponent) to exercise the live handler.
  • Mock AdminService (jest.fn() for previewChallengeImport, confirmChallengeImport, listChallenges, listCategories, exportChallenges) and stub the document createElement('input') file-picker flow by setting importDoc/importPreview directly via a small helper (or by calling the public openImport() + injecting the file via a fake <input> ref). Keep it minimal: directly drive the component signals is preferable to mocking the file picker.
  • AAA structure. Mocking strategy: only AdminService (network boundary). No DOM snapshotting; we assert on signal values (OnPush friendly) and DOM text via fixture.nativeElement.textContent if needed.
  • Tests:
    1. onImportConfirm closes the modal after success — arrange: component.importOpen.set(true), component.importDoc.set({ … }), component.importPreview.set({ total: 1, conflicts: [], unknownCategories: [], warnings: [] }). Stub admin.confirmChallengeImport to resolve { imported: 1, skipped: [], warnings: [] } and admin.listChallenges to resolve []. Act: await component.onImportConfirm(). Assert component.importOpen() is false, component.importResult() is null (cleared by closeImport), and component.statusMessage() contains "Imported 1".
    2. onImportConfirm keeps the modal open when the network call rejects — arrange same, stub confirmChallengeImport to reject with a generic error. Act + assert component.importOpen() stays true and component.importError() is non-null.

4.3 Run command

  • From the repo root: npm test (or npm run test:backend / npm run test:frontend for individual projects). Both new specs run inside the existing ts-jest configuration with no additional setup.