AI Implementation feature(842): First-Start Create Admin Modal 1.01 (#4)

2026-07-21 15:32:51 +00:00
parent 960516a7bc
commit 6feaf08bdb
17 changed files with 834 additions and 217 deletions
@@ -0,0 +1,294 @@
+# 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`).