feat: First-Start Create Admin Modal 1.02

2026-07-21 16:11:04 +00:00
parent 8a573f9306
commit e05f2a5046
3 changed files with 192 additions and 295 deletions
@@ -1,294 +0,0 @@
 # Implementation Plan: Job 842 — First-Start Create Admin Modal Reappears on Refresh
 ## 0. Already-Implemented Check
 The bug described in Job 842 is **NOT** fully implemented/fixed. The current
 code reproduces the symptom:
 - `frontend/src/app/core/services/auth.service.ts:11-12` — `accessToken` and
  `user` are kept in in-memory Angular signals only. No persistence layer
  (`sessionStorage`, `localStorage`, or cookie-backed restoration).
 - `frontend/src/app/app.component.ts:13-15` — `AppComponent.ngOnInit` calls
  only `bootstrap.load()`; there is no `/api/v1/auth/session` (or refresh)
  call on startup to restore an in-flight session.
 - `frontend/src/app/core/guards/auth.guard.ts:11-13` — `authGuard` reads
  `bootstrap.initialized()` synchronously. It defaults to `false`, so on a
  cold load (before the `GET /api/v1/bootstrap` promise resolves) it
  redirects to `/bootstrap`.
 - `frontend/src/app/features/setup/setup-create-admin.component.ts:28-119` —
  The component does not check `bootstrap.initialized()` on entry; it
  unconditionally renders the create-admin modal even when the backend
  reports `initialized: true`.
 Therefore the fix described below is required.
 ---
 ## 1. Architectural Reconnaissance
 - **Codebase style & conventions:**
  - Frontend: Angular 17+ standalone components, signal-based state,
    `inject()` for DI, `provideHttpClient(withInterceptors([...]))`,
    `loadComponent` for lazy routes, functional `CanActivateFn` guards.
  - Backend: NestJS controllers/services, `@Public()` decorator, zod
    validation pipes, `setRefreshCookie` helper for the HttpOnly `rt`
    cookie, global `JwtAuthGuard` (`APP_GUARD`).
  - Monorepo with `npm` workspaces (`backend`, `frontend`); root-level
    `npm test` runs both via `tests/jest.config.js`.
 - **Data Layer:** SQLite via TypeORM (`backend/src/database`). For this
  Job, **no schema or migration changes are required**. The existing
  `refresh_token` table is sufficient: rotating the refresh token on
  app boot restores the session transparently.
 - **Test Framework & Structure:**
  - Jest with `ts-jest`, two projects (`backend`, `frontend`) under
    `tests/jest.config.js`.
  - Frontend uses `jsdom`; helpers in `tests/frontend/jest.setup.ts`
    (already stubs `matchMedia`).
  - Frontend unit tests live in `tests/frontend/*.spec.ts`, never inside
    `frontend/src/...`. Backend tests live in `tests/backend/*.spec.ts`.
  - Single command `npm test` from the repo root runs all suites.
 - **Required Tools & Dependencies:** **None new.** No new system tools,
  no new npm packages, no `setup.sh` changes are required. The fix
  uses built-in browser APIs (`sessionStorage`) and the existing
  `HttpClient` + `/api/v1/auth/refresh` endpoint already wired by the
  backend (see `backend/src/modules/auth/auth.controller.ts:50-69`).
  Note: the `angular-security` skill explicitly warns *against*
  `localStorage` for tokens, but session-scoped persistence of the
  short-lived access token (15m) plus a one-time silent refresh on
  app boot using the HttpOnly `rt` cookie is the standard pattern and
  matches how the SPA already protects the access token in memory
  (never written to `localStorage` today either).
 ## 2. Impacted Files
 ### To Modify
 - `frontend/src/app/core/services/auth.service.ts` — Add `restoreSession()`
  helper and a `loadFromStorage()`/persistence layer (write access
  token + user into `sessionStorage` on `setSession`, read them in
  `restoreSession`, clear on `clear()`).
 - `frontend/src/app/app.component.ts` — On startup, after
  `bootstrap.load()` resolves, call `auth.restoreSession()` (which
  performs a silent `POST /api/v1/auth/refresh` using the `rt` cookie
  via `withCredentials: true`, then `setSession(...)`).
 - `frontend/src/app/core/guards/auth.guard.ts` — Wait for the bootstrap
  load to resolve before deciding (return a `Promise` resolving to a
  `UrlTree` or `true`). This prevents the race where `initialized()`
  is read as `false` before the bootstrap HTTP call completes.
 - `frontend/src/app/core/guards/admin.guard.ts` — Same race fix:
  await `BootstrapService.loaded()` before reading `initialized()`.
 - `frontend/src/app/core/services/bootstrap.service.ts` — Expose a
  `loaded` signal/promise (or a `waitForLoad()` method) that resolves
  once `load()` has completed.
 - `frontend/src/app/features/setup/setup-create-admin.component.ts` —
  Guard the template so the create-admin modal is not rendered when
  `bootstrap.initialized() === true`. If the user lands on
  `/bootstrap` while initialized, redirect to `/login` (or `/`
  if already authenticated).
 ### To Create
 - `tests/frontend/auth-restore.spec.ts` — Unit tests for the new
  `AuthService.restoreSession()` happy path + failure (no refresh
  cookie) + persistence round-trip.
 - `tests/frontend/guard-bootstrap-race.spec.ts` — Verifies the
  `authGuard` waits for bootstrap before deciding (returns
  `/bootstrap` when truly uninitialized, allows when initialized, and
  does not redirect to `/bootstrap` once `load()` has resolved with
  `initialized: true`).
 ## 3. Proposed Changes
 The bug is the intersection of three problems on the SPA:
 1. **No session persistence / restoration.** Access token and user
   identity live only in in-memory signals. A browser refresh wipes
   them, so `isAuthenticated()` returns `false` and the user is
   treated as anonymous.
 2. **No silent refresh on app boot.** Even though the `rt` HttpOnly
   cookie is still set, the SPA never calls `/api/v1/auth/refresh`
   on startup, so the in-memory session is never rebuilt.
 3. **Bootstrap race in `authGuard`.** The guard reads
   `bootstrap.initialized()` synchronously. On a cold load the
   signal still defaults to `false` (the GET hasn't resolved yet),
   so the guard fires `UrlTree(['/bootstrap'])` *before* bootstrap
   completes. Then `SetupCreateAdminComponent` renders the modal
   unconditionally because it never checks `initialized()` itself.
 ### Fix A — Persistence + silent refresh in `AuthService`
 In `frontend/src/app/core/services/auth.service.ts`:
 - Add a `STORAGE_KEY = 'hipctf.auth.v1'` constant.
 - In `setSession(token, user)`: write `{ token, user }` to
  `sessionStorage` (in addition to setting the signals). If
  `sessionStorage` is unavailable (SSR / disabled), swallow the
  error and continue.
 - In `clear()`: also `sessionStorage.removeItem(STORAGE_KEY)`.
 - Add `restoreSession(): Promise<boolean>`:
  1. Read `sessionStorage[STORAGE_KEY]`. If present, restore the
     signals (so guards see `isAuthenticated() === true` during the
     in-flight refresh).
  2. `POST /api/v1/auth/refresh` with `{ withCredentials: true }`
     and an empty body — the backend reads the `rt` cookie
     (`backend/src/modules/auth/auth.controller.ts:59-69`) and
     returns `{ accessToken, expiresIn, user }` plus a rotated
     `rt` cookie. The endpoint is `@Public()`, so it works
     pre-auth.
  3. On success: call `setSession(accessToken, user)` and return
     `true`.
  4. On failure (network / 401 `TOKEN_REVOKED` / no cookie):
     `clear()` and return `false`.
 - Keep `isAuthenticated` as `computed(() => !!this.user())`. Add a
  separate `hydrated = signal(false)` that becomes `true` once
  `restoreSession()` has finished; guards may use this to wait.
 ### Fix B — Bootstrap-aware guards
 In `frontend/src/app/core/services/bootstrap.service.ts`:
 - Add `private loadPromise: Promise<BootstrapPayload | null> | null`.
 - Track the in-flight (or completed) load promise inside `load()`,
  e.g.:
  ```ts
  if (!this.loadPromise) this.loadPromise = this._load();
  return this.loadPromise;
  ```
  where `_load()` is the current body of `load()`.
 - Expose `ready(): Promise<BootstrapPayload | null>` that returns
  `this.loadPromise` (resolved to a settled promise even before
  `load()` has been called).
 In `frontend/src/app/core/guards/auth.guard.ts`:
 - Convert to an `async` `CanActivateFn`:
  ```ts
  export const authGuard: CanActivateFn = async () => {
    const auth = inject(AuthService);
    const bootstrap = inject(BootstrapService);
    const router = inject(Router);
    await bootstrap.ready();        // wait for GET /api/v1/bootstrap
    await auth.waitUntilHydrated(); // wait for restoreSession()
    if (!bootstrap.initialized()) return router.createUrlTree(['/bootstrap']);
    if (!auth.isAuthenticated())   return router.createUrlTree(['/login']);
    return true;
  };
  ```
 - Add `AuthService.waitUntilHydrated()` that resolves on the next
  microtask once `hydrated()` is `true` (or returns immediately if
  already hydrated). Implementation: store the resolve callback
  inside `restoreSession()` after the HTTP call settles.
 In `frontend/src/app/core/guards/admin.guard.ts`:
 - Same treatment: `await bootstrap.ready()` and
  `await auth.waitUntilHydrated()` before calling
  `decideAdminGuard(...)`.
 ### Fix C — AppComponent orchestrates startup
 In `frontend/src/app/app.component.ts`:
 ```ts
 async ngOnInit() {
  await this.bootstrap.load();      // populates initialized() + theme
  await this.auth.restoreSession(); // silent refresh via rt cookie
 }
 ```
 Order matters: bootstrap first (so guards know `initialized`), then
 the silent refresh (so `isAuthenticated` becomes true on the way
 back to `/admin` if that was the last route).
 ### Fix D — `SetupCreateAdminComponent` must not render when initialized
 In `frontend/src/app/features/setup/setup-create-admin.component.ts`:
 - In the constructor (or `ngOnInit`), if
  `bootstrap.initialized() === true`, redirect immediately:
  ```ts
  constructor() {
    if (this.bootstrap.initialized()) {
      this.router.navigateByUrl(this.auth.isAuthenticated() ? '/' : '/login');
    }
  }
  ```
  Because `bootstrap.ready()` is awaited in the guards *before*
  navigation to `/bootstrap` resolves, the constructor can rely on
  `initialized()` being settled by the time the component is
  created. (If for any reason it isn't, the `await` in the
  `constructor` flow uses `effect()`-friendly code — see below.)
 - Add a defensive `effect()` (or signal-bound `@if` in the template)
  that hides the `.overlay` when `bootstrap.initialized()` is true,
  so even if the navigation is reached mid-race the modal is never
  displayed.
 ### Fix E — Login + setup flows keep working
 - `LoginComponent` (`frontend/src/app/features/auth/login.component.ts:33-48`)
  and `SetupCreateAdminComponent.submit` already call
  `auth.setSession(...)`; they will now also persist to
  `sessionStorage` automatically.
 - `bootstrap.markInitialized()` (`frontend/src/app/core/services/bootstrap.service.ts:48-50`)
  is still called by `SetupCreateAdminComponent.submit` after success
  — preserved as-is.
 ## 4. Test Strategy
 - **Target Unit Test File:**
  `tests/frontend/auth-restore.spec.ts` (new) and
  `tests/frontend/guard-bootstrap-race.spec.ts` (new). Both live in
  the dedicated `tests/frontend/` folder, runnable via
  `npm run test:frontend` and `npm test`.
 - **Mocking Strategy:**
  - Use `jest.spyOn(global, 'fetch')` or the existing `HttpClient`
    via `provideHttpClientTesting()` + `HttpTestingController`
    (preferred per the `angular-testing` skill). The auth restore
    flow uses `HttpClient.post('/api/v1/auth/refresh', ...)`; mock
    that with `expectOne(...).flush(...)`.
  - Stub `sessionStorage` per test using a fresh `Map` (jsdom
    provides one; clear it in `beforeEach`).
  - For guard tests: instantiate the guard function via
    `TestBed.runInInjectionContext(authGuard)` after providing
    minimal mocks of `AuthService` and `BootstrapService` with
    `jasmine.createSpyObj`. Use `fakeAsync` + `tick()` only if
    awaiting a Promise is needed; otherwise rely on `async` +
    `await` (per the angular-testing skill: "Signals sync — no
    fakeAsync needed for most signal-driven tests").
 - **Tests to write (minimal, success-path focused):**
  1. `AuthService.restoreSession()` happy path:
     - pre-populate `sessionStorage` with a valid `{ token, user }`,
       flush a `200 { accessToken, user }` from
       `HttpTestingController`, expect `isAuthenticated() === true`,
       expect `sessionStorage` to now contain the *new* access
       token.
  2. `AuthService.restoreSession()` failure path (no refresh
     cookie / `401 TOKEN_REVOKED`):
     - flush a `401`, expect `isAuthenticated() === false` and
     `sessionStorage` cleared.
  3. `AuthService.setSession()` persists to `sessionStorage`;
     `clear()` removes it.
  4. `authGuard` redirects to `/bootstrap` while bootstrap is
     uninitialized (returns `UrlTree`).
  5. `authGuard` does **not** redirect to `/bootstrap` once
     bootstrap has resolved with `initialized: true` and a
     hydrated auth session — it allows navigation.
  6. `SetupCreateAdminComponent` constructor redirects away from
     `/bootstrap` when `bootstrap.initialized() === true` (smoke:
     instantiate via `TestBed`, spy on `Router.navigateByUrl`,
     expect call to `/login` or `/`).
 - **Out of scope for tests:** backend changes (none), visual
  confirmation, end-to-end browser refresh. No Playwright / no UI.
 - **No new dependencies** — all tests run with the existing
  `jest`, `ts-jest`, `jest-environment-jsdom`, and Angular's
  `provideHttpClientTesting` (already in `frontend/package.json`
  via `@angular/common/http/testing`).
@@ -0,0 +1,191 @@
 # Implementation Plan: First-Start Create Admin Modal — Empty-Field Validation Exercise
 ## Status: NOT ALREADY IMPLEMENTED
 The feature is **partially** in place. The `SetupCreateAdminComponent` (the
 modal loaded by the `/bootstrap` route) already contains reactive-form
 validators (required / minlength / maxlength / pattern on `username`,
 required on `password` + `passwordConfirm`, plus the group-level
 `passwordMatchValidator`), and `submit()` already calls
 `form.markAllAsTouched()` before bailing out on invalid forms. However, the
 **submit button is hard-disabled while the form is invalid**
 (`setup-create-admin.component.html:57`):
 `[disabled]="submitting() || form.invalid"`, so the user can never actually
 *click* submit while fields are empty/missing — the empty-field validation
 path is unreachable. The job requires that submitting an empty / partially
 filled form keeps the overlay open and surfaces field validation *without*
 creating an administrator; today that interaction cannot be exercised.
 The "Already Implemented Check" therefore does **not** pass and a code
 change is required.
 ---
 ## 1. Architectural Reconnaissance
 - **Codebase style & conventions:**
  - Monorepo: NestJS REST API in `backend/` + Angular 20 standalone SPA in
    `frontend/`. Workspace orchestration via root `package.json`
    (`workspaces: ["backend", "frontend"]`).
  - TypeScript strict mode (`tsconfig.base.json`), `OnPush` change detection
    on every component, `inject()`-based DI, Signals + `computed()` +
    `effect()`.
  - Reactive forms via `FormBuilder.nonNullable.group`, custom validators
    kept in standalone files (`setup-create-admin.validators.ts`).
  - `@if` / `@for` new control-flow syntax (no `*ngIf`/`*ngFor`).
 - **Data Layer:** SQLite via TypeORM (`backend/src/database/entities/user.entity.ts`,
  `setting.entity.ts`). No schema migration is required for this job — the
  bug is purely client-side UX gating.
 - **Test Framework & Structure:**
  - Jest 29 with two projects defined in `tests/jest.config.js`:
    `backend` (node) and `frontend` (jsdom).
  - Tests live under `tests/backend/` and `tests/frontend/`, never inside
    source folders.
  - Single root command: `npm test` (mapped to
    `jest --config tests/jest.config.js`). Sub-commands
    `npm run test:backend` / `npm run test:frontend` exist.
  - Setup script: `bash setup.sh` (no new system tools needed).
 - **Required Tools & Dependencies:** None new. Existing stack (Angular 20,
  `@angular/forms`, `@angular/common/http`, `jest`, `ts-jest`,
  `jest-environment-jsdom`) is sufficient.
 ## 2. Impacted Files
 - **To Modify:**
  - `frontend/src/app/features/setup/setup-create-admin.component.html`
    — relax the submit-button `disabled` binding so empty-field
    submission is exercisable; submit still must not double-fire.
  - `frontend/src/app/features/setup/setup-create-admin.component.ts`
    — make `submit()` an idempotent no-op when already in flight, and
    ensure that when `form.invalid` the call only marks touched (it
    already does) so the validation messages render under
    `data-testid="setup-username-error"` /
    `data-testid="setup-confirm-error"`.
  - `frontend/src/app/features/setup/setup-create-admin.component.ts`
    — guard the "already initialized → bounce" `effect()` so that the
    modal can still be opened when an integration test seeds the
    `BootstrapService.initialized` signal as `false` (the existing
    `alreadyInitialized` computed is already correct; no change
    required, but the plan documents the expected harness setup).
 - **To Create:**
  - `tests/frontend/setup-create-admin-submit.spec.ts` — focused spec
    asserting that (a) clicking submit with empty fields keeps the modal
    open, (b) the username required / minlength / pattern errors render,
    (c) the password-confirm mismatch error renders, and (d) the
    `SetupCreateAdminService.create()` call is **not** made.
 ## 3. Proposed Changes
 1. **HTML — relax submit-button gating**
   - In `setup-create-admin.component.html` change:
     ```html
     [disabled]="submitting() || form.invalid"
     ```
     to:
     ```html
     [disabled]="submitting()"
     ```
   - Keep `data-testid="setup-submit"` and the `[type="submit"]` so
     existing test selectors and the `(ngSubmit)="submit()"` binding
     continue to work. Add `attr.aria-disabled` only if needed for
     a11y — not required by the job.
   - Why: lets the user press the button with empty/missing fields. The
     component's `submit()` already short-circuits via
     `markAllAsTouched()` + early return when `form.invalid`, so no
     server call fires.
 2. **Component TS — harden submit() re-entry**
   - In `setup-create-admin.component.ts:99-129`, the existing
     `submit()` already guards with `if (this.submitting()) return;`
     and `markAllAsTouched()` then `if (this.form.invalid) return;`.
     No structural change is required; the only edit is to **document
     in a one-line code comment** that empty/invalid submissions are
     intentionally allowed through the UI so the validation branch is
     reachable. (Per project policy, no gratuitous comments — this is
     only added if the team prefers; the implementer can skip it.)
   - Concrete visible behavior unchanged: when form is invalid the
     `usernameErrors()` and `confirmErrors()` `computed()`s already
     surface `data-testid="setup-username-error"` /
     `data-testid="setup-confirm-error"` strings because
     `markAllAsTouched()` flips `c.touched` to true.
 3. **Component TS — already-initialized effect stays as-is**
   - The `effect()` at lines 39-43 still bounces to `/login` (or `/` when
     authenticated) when the installation is already initialized. This
     is the documented product behavior per
     `docs/guides/bootstrap.md` ("the setup route becomes a 404 and the
     modal can never appear again"). The job is explicitly scoped to
     **the uninitialized-installation path**, so no change here.
   - The test plan therefore seeds `BootstrapService.initialized` as
     `false` before instantiating the component (the standard pattern
     used by `guard-bootstrap-race.spec.ts` and the existing
     `setup-create-admin.spec.ts` validator-only spec).
 4. **No backend / no DB change.** `POST /api/v1/setup/create-admin`
   already returns `400 VALIDATION_FAILED` for empty bodies via the zod
   schema (`backend/src/modules/setup/dto/create-admin.dto.ts`), but the
   frontend never needs to hit that endpoint because the form is
   rejected client-side first.
 ## 4. Test Strategy
 - **Target Unit Test File:** `tests/frontend/setup-create-admin-submit.spec.ts`
  (new), kept under the existing `tests/frontend/` Jest project
  (`jsdom`, `@angular/forms` reactive form runtime, `TestBed`).
 - **Mocking Strategy:**
  - Use `TestBed.configureTestingModule({ imports: [SetupCreateAdminComponent] })`
    per the project's standalone-component pattern (see existing
    `tests/frontend/setup-create-admin.spec.ts` and the
    `angular-testing` skill rules).
  - Stub `BootstrapService` with a tiny `TestBed.runInInjectionContext`
    provider that exposes `initialized = signal(false)` and
    `passwordPolicy = signal({ description: '...', minLength: 12,
    requireMixed: true })`. No `HttpTestingController` needed because
    the success path is never reached; for the mismatch-error assertion
    we can populate two valid-password controls directly via
    `form.patchValue` to bypass `minLength`/`required`.
  - Stub `AuthService` and `Router` with `jest.fn()`-style test doubles
    so navigation / session mutations can be asserted (they should NOT
    fire in the empty-field cases).
  - Spy on `SetupCreateAdminService.create` via
    `provideHttpClientTesting()` (preferred per skill
    `angular-testing` §2) and assert `.expectNone(...)` /
    `.verify()` to prove no network call was made.
  - Drive the DOM via Angular's `ComponentFixture.debugElement.query`
    using the documented `data-testid` attributes
    (`setup-username`, `setup-password`, `setup-password-confirm`,
    `setup-submit`, `setup-username-error`, `setup-confirm-error`) —
    no visual confirmation required.
 - **Assertions:**
  1. With empty form, click `[data-testid="setup-submit"]` → modal
     remains mounted (component is not destroyed) and
     `[data-testid="setup-username-error"]` text equals
     `"Username is required"`.
  2. With `username = "ab"` and empty passwords → submit click does not
     invoke `SetupCreateAdminService.create` (no HTTP request flushed)
     and the username error reads
     `"Username must be at least 3 characters"`.
  3. With `password = "Sup3r!"` and `passwordConfirm = "Different!"` →
     after `markAllAsTouched()`,
     `[data-testid="setup-confirm-error"]` renders
     `"Passwords do not match"`.
  4. The button's `disabled` attribute reflects `submitting()` only
     (assert `disabled === false` when form is invalid and not
     submitting).
 - **Runnability:** Picked up automatically by
  `npm run test:frontend` and `npm test`. No new deps, no visual
  checks, no extra setup.
 ---
 ## 5. Out of Scope
 - Backend setup endpoint behavior (already correct: returns 400
  `VALIDATION_FAILED` for empty payloads).
 - `/api/v1/bootstrap` returning `initialized:false` — already
  implemented in `backend/src/modules/system/system.service.ts:36-49`.
 - Changing the `/bootstrap` route guard or the
  `decideAuthRedirect` function (already correct per
  `tests/frontend/guard-bootstrap-race.spec.ts`).
 - Any schema migration.
@@ -54,7 +54,7 @@
      <button
        type="submit"
        class="primary"
-        [disabled]="submitting() || form.invalid"
+        [disabled]="submitting()"
        data-testid="setup-submit"
      >
        @if (submitting()) {