AI Implementation feature(846): First-Start Create Admin Modal 1.05 (#8)
This commit was merged in pull request #8.
This commit is contained in:
@@ -1,147 +0,0 @@
|
||||
# Implementation Plan: Job 845 — First-Start Create Admin Modal 1.04 (surface field validation)
|
||||
|
||||
## 1. Architectural Reconnaissance
|
||||
|
||||
- **Codebase style & conventions:**
|
||||
- NestJS API + Angular 17 SPA monorepo (workspaces in `package.json`).
|
||||
- Frontend: **Standalone components**, `ChangeDetectionStrategy.OnPush`, Angular **signals** (`signal`/`computed`), no NgModules. Style guide mandates `OnPush` and signals for state.
|
||||
- Reactive Forms via `ReactiveFormsModule` with `fb.nonNullable.group(...)` (non-nullable typed controls).
|
||||
- All services are `providedIn: 'root'`. Two HTTP interceptors wired in `frontend/src/main.ts` (`csrfInterceptor`, then `authInterceptor`).
|
||||
- Frontend tests are **pure-logic / no TestBed**: `tests/frontend/**/*.spec.ts` import pure functions (validators, session-storage helpers, shell decision helpers) and rely on `jest` (`testEnvironment: 'jsdom'`) runnable with the root command `npm test` (which uses `jest --config tests/jest.config.js --selectProjects frontend`).
|
||||
- Strictly-typed reactive forms per `angular-forms` skill (`FormGroup<T>`, `fb.nonNullable`).
|
||||
- Data attributes / `data-testid` selectors are the stable DOM hooks (see `docs/guides/bootstrap.md`).
|
||||
- **Data Layer:** N/A for this Job — purely a frontend UI/validation correctness fix. The backend already returns a structured `400 VALIDATION_FAILED` envelope; the bug is solely that the SPA short-circuits on `form.invalid` without ever surfacing per-field error messages.
|
||||
- **Test Framework & Structure:**
|
||||
- Jest with `ts-jest`, jsdom environment for frontend.
|
||||
- Tests live **only** in `tests/frontend/` (never co-located with source).
|
||||
- Root command: `npm test` (runs all projects via `jest --config tests/jest.config.js`).
|
||||
- Existing precedent at `tests/frontend/setup-create-admin.spec.ts` is a pure-logic suite (mirrors component logic in standalone helper functions). **We will follow that pattern** — no TestBed, no DOM rendering required, no visual confirmation.
|
||||
- **Required Tools & Dependencies:** No new dependencies, system tools, or `setup.sh` changes needed. The fix is a localized source-code change in the standalone component + its template + a pure-helper module that mirrors the per-field error messages (for parity testing). The npm workspace already has `@angular/forms ^17.3` and `jest`.
|
||||
|
||||
## 2. Impacted Files
|
||||
|
||||
### To Modify
|
||||
- **`frontend/src/app/features/setup/setup-create-admin.component.ts`** — fix the reactivity of the per-field error calculations and expose `passwordErrors()`. Convert the `computed(...)`s whose bodies read `FormControl` properties (which are NOT signals in Angular 17) into plain **methods** so the template re-evaluates them on every change-detection cycle (cheap, OnPush-friendly, no extra subscriptions). Alternatively keep them as computed but reflect `touched` into a local `signal()` driven by `statusChanges`/`valueChanges` + an effect; **plain methods are the minimal, idiomatic fix** and are used elsewhere in the same component style. Keep all existing public surface (`form`, `submitting`, `serverError`, `serverErrorCode`, `canRetry`, `policyDescription`, `submit`, `swallowBackdrop`, `swallowEscape`) intact.
|
||||
- **`frontend/src/app/features/setup/setup-create-admin.component.html`** — render the new Password field-error block, mirror `aria-invalid` on the Password and Confirm Password inputs (Username already has it), and keep the `data-testid` hooks the bootstrap guide documents (`setup-username-error`, add `setup-password-error`, keep `setup-confirm-error`).
|
||||
- **`frontend/src/app/features/setup/setup-create-admin.component.css`** — no functional change required; `.field-error` styling already exists.
|
||||
- **`tests/frontend/setup-create-admin.spec.ts`** — extend the existing pure-logic suite to cover the new password-error helper and to assert that, given an all-empty (or partially-filled) post-submit state, the per-field error strings the template renders are correct.
|
||||
|
||||
### To Create
|
||||
- **`frontend/src/app/features/setup/setup-create-admin.field-errors.ts`** — pure helper functions that return the per-field error string (or `null`) for a given `(value, touched)` snapshot. Keeping them as pure functions (no Angular DI, no signals) is deliberate: it lets the component call them from methods **and** lets the pure-logic test suite assert them without spinning up Angular. Mirrors the pattern already used by `setup-create-admin.validators.ts` and tested in `setup-create-admin.spec.ts`.
|
||||
|
||||
## 3. Proposed Changes
|
||||
|
||||
### Root cause
|
||||
Angular 17 `FormControl.touched`, `.valid`, and `.errors` are plain class properties, **not** signals. The existing `computed(() => … usernameErrors …)` (component file lines 70–78 and 80–87) reads those plain properties and so registers **zero signal dependencies**. Even after `submit()` calls `form.markAllAsTouched()`, the computed is not re-evaluated — its cached `null` is returned and the `@if (usernameErrors()) { … }` block stays hidden. The same applies to `confirmErrors()` for the `passwordConfirm` control. There is also no `passwordErrors()` at all and no `aria-invalid` binding on the Password/Confirm inputs, which matches the user's observation.
|
||||
|
||||
### 3.1 New pure helper module
|
||||
|
||||
`frontend/src/app/features/setup/setup-create-admin.field-errors.ts`:
|
||||
|
||||
```ts
|
||||
export type FieldErrorKey =
|
||||
| 'required'
|
||||
| 'minlength'
|
||||
| 'maxlength'
|
||||
| 'pattern'
|
||||
| 'passwordMismatch';
|
||||
|
||||
export interface FieldErrorContext {
|
||||
value: string;
|
||||
touched: boolean;
|
||||
errors: Partial<Record<FieldErrorKey, true>> | null;
|
||||
groupMismatch?: boolean;
|
||||
}
|
||||
|
||||
export function usernameError(ctx: FieldErrorContext): string | null { … }
|
||||
export function passwordError(ctx: FieldErrorContext): string | null { … } // currently required-only; password POLICY is server-enforced
|
||||
export function confirmError(ctx: FieldErrorContext): string | null { … }
|
||||
```
|
||||
|
||||
Rules encoded (single source of truth, identical to current component intent):
|
||||
|
||||
- **Username:**
|
||||
- not touched or no errors → `null`
|
||||
- `required` → `'Username is required'`
|
||||
- `minlength` → `'Username must be at least 3 characters'`
|
||||
- `maxlength` → `'Username must be at most 32 characters'`
|
||||
- `pattern` → `'Username may only contain letters, digits, dot, dash and underscore'`
|
||||
- else → `'Invalid username'`
|
||||
- **Password:**
|
||||
- not touched or no errors → `null`
|
||||
- `required` → `'Password is required'`
|
||||
- else → `null` (policy is intentionally server-side only; this component never surfaces a "weak password" field error pre-submit)
|
||||
- **Confirm:**
|
||||
- not touched and no `passwordMismatch` group error → `null`
|
||||
- `required` → `'Please confirm your password'`
|
||||
- `passwordMismatch` (group-level) → `'Passwords do not match'`
|
||||
- else → `'Invalid value'`
|
||||
|
||||
These signatures accept the small slice of state they need so the component can pass `c.value`, `c.touched`, and `c.errors` (cast through `Partial<Record<…, true>>`) and so the test suite can call them directly without touching Angular.
|
||||
|
||||
### 3.2 Component changes
|
||||
|
||||
`setup-create-admin.component.ts`:
|
||||
|
||||
1. **Replace the two `computed(...)`s with plain methods** so the template re-renders them on every CD pass:
|
||||
- `usernameError(): string | null { return usernameError({ value: this.form.controls.username.value, touched: this.form.controls.username.touched, errors: this.form.controls.username.errors }); }`
|
||||
- `passwordError(): string | null { return passwordError({ value: this.form.controls.password.value, touched: this.form.controls.password.touched, errors: this.form.controls.password.errors }); }` ← **new**
|
||||
- `confirmError(): string | null { return confirmError({ value: this.form.controls.passwordConfirm.value, touched: this.form.controls.passwordConfirm.touched, errors: this.form.controls.passwordConfirm.errors, groupMismatch: !!this.form.errors?.['passwordMismatch'] }); }`
|
||||
2. **Delete the orphaned `usernameErrors` and `confirmErrors` signal properties** (template rewires to methods).
|
||||
3. Leave `submit()` exactly as-is: `markAllAsTouched()` then early-return on invalid. The early return is now correct and observable (because `…Error()` is re-evaluated by the next CD cycle that runs after `markAllAsTouched` synchronously mutates `touched` on every control). No new `effect()` needed; this is the minimal fix.
|
||||
4. Keep OnPush (`ChangeDetectionStrategy.OnPush`), keep all existing imports/decorators/HostListener for `escape` and `swallowBackdrop`. The host binding keyboard handler stays so the modal remains non-dismissible.
|
||||
|
||||
> Optional, NOT recommended for this Job (kept out of scope to honor "minimal changes"): a reactive alternative would be to subscribe to `statusChanges` / `valueChanges` via `toSignal()` in the constructor and gate rendering on those signals. The plain-method approach above is the minimum-surface fix that fully resolves the bug and matches the component's current style.
|
||||
|
||||
### 3.3 Template changes
|
||||
|
||||
`setup-create-admin.component.html`:
|
||||
|
||||
1. Username `<input>` — keep existing `[attr.aria-invalid]="!!usernameError()"` and existing `<small class="field-error">` block, but rebind to the new method name.
|
||||
2. Password `<label class="field">` — add `aria-invalid` and add the missing error block below the policy hint:
|
||||
```html
|
||||
<input … formControlName="password"
|
||||
[attr.aria-invalid]="!!passwordError()"
|
||||
data-testid="setup-password" />
|
||||
<small class="hint">{{ policyDescription() }}</small>
|
||||
@if (passwordError()) {
|
||||
<small class="field-error" data-testid="setup-password-error">{{ passwordError() }}</small>
|
||||
}
|
||||
```
|
||||
3. Confirm `<input>` — add `aria-invalid`:
|
||||
```html
|
||||
<input … formControlName="passwordConfirm"
|
||||
[attr.aria-invalid]="!!confirmError()"
|
||||
data-testid="setup-password-confirm" />
|
||||
@if (confirmError()) {
|
||||
<small class="field-error" data-testid="setup-confirm-error">{{ confirmError() }}</small>
|
||||
}
|
||||
```
|
||||
4. Submit button — **unchanged**. The Job accepts either "visible per-field error messages" **or** "disabled submit button until valid" — we keep the click-to-trigger-feedback behavior (already used by `markAllAsTouched`) and now surface real errors, which is the original design intent. We deliberately do **not** disable the button on invalid (preserves parity with the existing guide which says the button is "disabled only while submitting").
|
||||
|
||||
### 3.4 CSS
|
||||
|
||||
No changes. `.field-error` color/size is already defined.
|
||||
|
||||
### 3.5 No backend changes
|
||||
|
||||
The backend's `SetupController.createAdmin` already responds with `400 VALIDATION_FAILED` and a `details` array; that contract is unchanged. The client never reached it before, and after the fix the client still won't for an empty form (which is correct — the per-field client-side errors are now visible; the server is reachable when the client actually submits valid input).
|
||||
|
||||
## 4. Test Strategy
|
||||
|
||||
- **Target Unit Test File:** `tests/frontend/setup-create-admin.spec.ts` (extend the existing pure-logic file; do **not** create a co-located spec under `frontend/`).
|
||||
- The existing file already exports nothing new — it lives entirely in `tests/frontend/` per project convention and runs with the root command `npm test` (Jest, jsdom).
|
||||
- **Mocking Strategy:**
|
||||
- **No TestBed, no HTTP, no DOM rendering.** The new helpers (`usernameError`, `passwordError`, `confirmError`) are pure functions in `setup-create-admin.field-errors.ts` — they are imported and called directly with hand-built `FieldErrorContext` literals, mirroring the existing `usernameErrors`/`isFormReadyForSubmit` helpers in the same file.
|
||||
- The existing `passwordMatchValidator` tests and username rules act as the de-facto model; we extend the "Composed submit gate" suite to also assert the rendered error strings.
|
||||
- **New test cases (≤ 8, focused on the bug paths):**
|
||||
1. `usernameError({ value: '', touched: true, errors: { required: true } })` returns `'Username is required'`.
|
||||
2. `usernameError({ value: 'ab', touched: true, errors: { minlength: true } })` returns the minlength message.
|
||||
3. `passwordError({ value: '', touched: true, errors: { required: true } })` returns `'Password is required'`.
|
||||
4. `passwordError({ value: 'x', touched: true, errors: null })` returns `null` (policy is server-side only; no pre-submit hint).
|
||||
5. `confirmError({ value: '', touched: true, errors: { required: true } })` returns `'Please confirm your password'`.
|
||||
6. `confirmError({ value: 'a', touched: true, errors: null, groupMismatch: true })` returns `'Passwords do not match'`.
|
||||
7. `confirmError({ value: 'a', touched: false, errors: null })` returns `null` (no spurious visibility pre-touch).
|
||||
8. `usernameError({ value: '', touched: false, errors: { required: true } })` returns `null` (no spurious visibility pre-touch).
|
||||
- **How to run:** `npm test` (root). Selectors: `npm run test:frontend` for fast feedback. No browser, no screenshot, no Visual regression.
|
||||
- **Out of scope (explicitly):** TestBed rendering, HTTP mock for `SetupCreateAdminService.create`, fixture-based aria assertion, and end-to-end visual confirmation — all forbidden by the test rules.
|
||||
@@ -0,0 +1,52 @@
|
||||
# Implementation Plan: First-Start Create Admin Modal 1.05
|
||||
|
||||
## 1. Architectural Reconnaissance
|
||||
- **Status:** The requested race-condition fix is **not fully implemented**. `backend/src/modules/setup/setup.service.ts:58-93` performs the admin count check inside a transaction, but two transactions can both observe zero admins before the Argon2 hash and subsequent insert. Existing tests cover serial creation and post-initialization rejection, but not simultaneous different-user submissions or refresh-token cardinality.
|
||||
- **Codebase style & conventions:** TypeScript monorepo using NestJS 10, Angular 17 standalone components, async/await, dependency injection, Zod request validation, TypeORM repositories/transactions, and centralized `ApiError`/global exception formatting. Controllers are thin; bootstrap business logic belongs in `SetupService`.
|
||||
- **Data Layer:** SQLite through `better-sqlite3`, accessed with TypeORM and explicit migrations (`synchronize: false`). The `user` table has a unique username but no invariant-enforcing uniqueness constraint for the `role` value. Database startup runs migrations and enforces WAL mode. The setup mutation and refresh-token creation already share one TypeORM transaction.
|
||||
- **Test Framework & Structure:** Root Jest multi-project configuration (`tests/jest.config.js`) runs backend Node tests from `tests/backend/**/*.spec.ts` and frontend jsdom tests from `tests/frontend/**/*.spec.ts`; tests are kept outside source directories and run with `npm test`. Backend integration tests use Nest `Test.createTestingModule`, the real SQLite TypeORM database, `supertest` via `csrfClient`, and cleanup in `beforeEach`/`afterAll`.
|
||||
- **Required Tools & Dependencies:** No new package or system dependency should be required. Continue using the existing Node/npm toolchain, NestJS, TypeORM, `better-sqlite3`, Argon2, Jest, and Supertest. `setup.sh` already installs dependencies and builds both workspaces; do not add infrastructure unless the chosen SQLite locking approach requires an existing package (the preferred plan does not). If a migration is added, it is TypeScript and is included by the existing database migration registration rather than requiring a new CLI.
|
||||
|
||||
## 2. Impacted Files
|
||||
- **To Modify:**
|
||||
- `backend/src/modules/setup/setup.service.ts` — serialize or otherwise atomically coordinate first-admin bootstrap attempts so only one request can pass the empty-admin gate and complete user/session creation.
|
||||
- `tests/backend/setup-create-admin.spec.ts` — add one focused concurrent-submission regression test asserting one success, one controlled conflict/business-rule failure, exactly one admin, and exactly one refresh-token row.
|
||||
- `docs/api/setup.md` — document the concurrency guarantee and the structured losing-request response if the public contract changes from the currently documented 404/USERNAME_TAKEN cases.
|
||||
- `docs/database/users.md` and/or `docs/guides/bootstrap.md` — update the first-admin invariant description to explicitly cover concurrent requests, if documentation is maintained as part of implementation.
|
||||
- `backend/src/common/errors/error-codes.ts` — only if implementation introduces a dedicated bootstrap-race error code rather than reusing the existing `SYSTEM_INITIALIZED` or `NOT_FOUND` code.
|
||||
- **To Create:**
|
||||
- No new application source file is required by the preferred design.
|
||||
- A new migration file is not expected unless the implementer chooses a database-level sentinel/lock-row design; if that design is selected, create a timestamped migration under `backend/src/database/migrations/` and register it in `backend/src/database/database.module.ts`.
|
||||
|
||||
## 3. Proposed Changes
|
||||
Explain the exact implementation logic step-by-step:
|
||||
|
||||
1. **Database / Schema Migration:**
|
||||
- Prefer no schema change: the existing transaction and SQLite WAL database are sufficient if the setup critical section is serialized before the admin count check.
|
||||
- Implement a process-local, promise-based mutex/critical-section guard owned by `SetupService` (or an equivalent injected singleton lock service) around the entire first-admin check-to-commit path. The lock must be acquired before `dataSource.transaction` performs the admin count query and released in a `finally` block on success, validation failure, database failure, or controlled conflict.
|
||||
- Keep the password equality, password-policy validation, rate-limit check, and Argon2 hashing semantics unchanged. The critical requirement is that the second request cannot execute the empty-admin check until the first transaction has either committed the admin and refresh token or rolled back.
|
||||
- Once the first transaction commits, the next request rechecks the admin count and exits without hashing, creating a user, or minting a refresh token. Return a controlled business-rule response. The cleanest contract is the existing `SYSTEM_INITIALIZED` 409 already used by the legacy first-admin endpoint; alternatively retain the setup endpoint's existing `NOT_FOUND` 404 contract if compatibility is more important. Whichever status/code is selected must be consistent and explicitly tested/documented.
|
||||
- Do not rely on the username unique constraint alone: concurrent requests intentionally use different usernames, so that constraint cannot enforce the first-admin invariant. Do not rely on the current unlocked count query or on Argon2's synchronous blocking behavior.
|
||||
- If deployment topology may include multiple Node processes/workers, document that an in-process mutex only protects one process. For a true multi-process guarantee, replace it with a database-backed lock/sentinel approach: add one immutable bootstrap lock row/table via migration, acquire it using SQLite's write serialization/transaction semantics before counting admins, then perform the check, hash, insert, and session mint in the same transaction. Validate the chosen approach against the repository's single-process topology before implementation; the minimum Job reproduction is within one Nest process.
|
||||
|
||||
2. **Backend Logic & APIs:**
|
||||
- Update `SetupService.createAdmin` so every path that can mutate bootstrap state is within the same serialized/atomic boundary. Preserve the existing transaction callback ordering: count admins, reject initialized systems, check username, hash password, save enabled admin, record rate limit, and call `AuthService.createSession` using the transaction manager.
|
||||
- Ensure lock release occurs after the transaction promise settles, not immediately after starting it. This prevents the second request from observing an uncommitted or partially completed first request.
|
||||
- Normalize the losing request through `ApiError` with a stable structured `code`, message, and HTTP status. Do not expose raw SQLite `SQLITE_BUSY`/constraint errors. If a database-level locking strategy is used, catch/map expected lock contention to the same business-rule error and ensure failed transactions cannot leave a user or refresh-token row behind.
|
||||
- Leave `SetupController` as a thin delegator. It should continue to derive the IP, call the service, set the HttpOnly refresh cookie only on a successful session, and return the existing 201 response shape for the winner.
|
||||
- Keep frontend behavior unchanged unless the selected loser code is not already handled. `SetupCreateAdminService` already preserves status/code from the API, while `SetupCreateAdminComponent` treats only `USERNAME_TAKEN` as non-retryable and presents a generic retryable error for other codes. If `SYSTEM_INITIALIZED` is chosen, update the component/service contract only if UX should stop retrying after another caller wins; otherwise document that a concurrent loser should be redirected/reloaded rather than repeatedly retrying a now-initialized setup route.
|
||||
- Update the setup API documentation to state: among simultaneous valid first-admin requests, exactly one may return 201; all others receive the chosen structured conflict/business-rule response; only one enabled admin and one refresh-token row are created.
|
||||
|
||||
3. **Frontend UI Integration:**
|
||||
- No visual/modal redesign is required. The modal already disables duplicate clicks within one component instance (`submitting` signal), but that does not protect independent browser requests; the invariant must remain server-side.
|
||||
- If backend returns `SYSTEM_INITIALIZED`/409 for the losing request, decide and implement the smallest compatible handling: mark the setup flow unavailable and route to login/bootstrap reload, or retain the existing generic failure presentation. Add a frontend logic test only if a changed error-code branch is introduced; do not add visual/browser tests for this backend race fix.
|
||||
|
||||
## 4. Test Strategy
|
||||
- **Target Unit Test File:** `tests/backend/setup-create-admin.spec.ts`.
|
||||
- Add one minimal regression test using the existing initialized Nest app and real in-memory SQLite database. Clear both users and refresh tokens before the test (the current `beforeEach` only clears users, so the new test must explicitly clean refresh tokens or use a safe test isolation arrangement).
|
||||
- Arrange two independent valid requests with different usernames and invoke them concurrently with `Promise.all` through separate `csrfClient` instances or equivalent Supertest agents. Avoid mocking Argon2, TypeORM, or the database: the bug is transaction/concurrency behavior and must be verified at the HTTP/service boundary.
|
||||
- Assert that the result statuses contain exactly one `201` and exactly one expected controlled loser status (normally `409`, or `404` if compatibility-preserving behavior is selected). Assert the loser body has the exact structured error `code`; do not assert a generated token value.
|
||||
- Assert database postconditions through repositories: exactly one row with `role: 'admin'`, exactly one enabled first-admin user overall, and exactly one row in `refresh_token`. Verify the winning username is one of the two submitted names and the losing username is absent. This directly guards against both duplicate admins and duplicate sessions.
|
||||
- Retain the existing focused tests for winner success, serial post-initialization rejection, weak passwords, mismatch, and invalid usernames. Update only assertions/comments that become stale if the selected concurrency error code changes.
|
||||
- Follow AAA structure, clear mocks if any are introduced, close the Nest app, and ensure no shared persistent data is written. Use the existing `DATABASE_PATH=':memory:'`; `/data` is unnecessary because the test does not create successor-job assets or persistent fixtures.
|
||||
- Run the repository's single root command `npm test` during implementation, plus the project's available build/type validation commands discovered from package scripts. There is no lint or typecheck script in the inspected root/backend package manifests; do not invent one. `setup.sh` needs no update for the planned implementation.
|
||||
Reference in New Issue
Block a user