AI Implementation feature(847): First-Start Create Admin Modal 1.06 #9

Merged
m0rph3us1987 merged 2 commits from feature-847-1784655231033 into dev 2026-07-21 17:38:23 +00:00
5 changed files with 129 additions and 58 deletions
-52
View File
@@ -1,52 +0,0 @@
# 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.
+107
View File
@@ -0,0 +1,107 @@
# Implementation Plan: First-Start Create Admin Modal 1.06 — Bootstrap Race Fix
## 0. Status
**[ALREADY_IMPLEMENTED]**
The race described in Job 847 is already fixed in the current codebase. Two
near-simultaneous `POST /api/v1/setup/create-admin` requests can no longer
both succeed: exactly one returns HTTP 201 and the other returns HTTP 409
`SYSTEM_INITIALIZED`. No new code or test work is required.
## 1. Architectural Reconnaissance
- **Codebase style & conventions:** TypeScript, NestJS 10+ (modular, DI,
controllers + services), class-based services, TypeORM with the
`DataSource.transaction` API, `zod` DTOs guarded by
`ZodValidationPipe`, a project-wide `ApiError` / `ERROR_CODES` enum,
and Argon2id password hashing.
- **Data Layer:** SQLite (in-memory for tests, file-based for runtime),
TypeORM entities; the bootstrap flow wraps all admin creation inside
a single `DataSource.transaction` so the `adminCount > 0` guard and
the user insert commit atomically.
- **Test Framework & Structure:** Jest. Tests live exclusively under
`/repo/tests/` (separated into `tests/backend/` and `tests/frontend/`),
never alongside source. Backed by a `csrfClient` helper in
`tests/backend/csrf-client.ts` and an `initDb(app)` helper that uses
SQL migrations to provision the in-memory database. Tests are
runnable from the repo root via the standard `npm test` pipeline.
- **Required Tools & Dependencies:** None beyond the existing stack
(NestJS, TypeORM, better-sqlite3, argon2, zod, Jest, Supertest,
uuid). No new package, system tool, or `setup.sh` change is needed.
## 2. Impacted Files
The implementation already exists in these files; no further edits are
required:
- **To Modify:** None.
- **To Create:** None.
- **Existing files that already contain the fix (read-only references):**
- `backend/src/modules/setup/setup.service.ts:23,39-97,111-130`
`SetupService.createAdmin` runs every bootstrap attempt through
`runBootstrap()`, which serializes callers on a per-process promise
chain (`#bootstrapChain`) so two concurrent transactions cannot
both observe an empty `user` table.
- `backend/src/modules/setup/setup.service.ts:60-95` — Each
serialized call executes inside `dataSource.transaction(...)`,
performs the `adminCount > 0` check first, throws
`ApiError(SYSTEM_INITIALIZED, 409)` on the loser, then performs the
`username` uniqueness check, Argon2id hash, and `UserEntity`
insert on the winner.
- `backend/src/modules/setup/setup.module.ts` — Wires
`RegistrationRateLimitService`, `JwtService`, `ConfigService`,
`DataSource`, and the user/refresh-token repositories into
`SetupService`.
- `backend/src/common/errors/api-error.ts`,
`backend/src/common/errors/error-codes.ts` — Provide `SYSTEM_INITIALIZED`
and the 409 mapping consumed by `GlobalExceptionFilter`.
- `tests/backend/setup-create-admin.spec.ts:121-149` — Existing
regression test `"only one of two concurrent first-admin requests
succeeds; the other receives a controlled conflict"` already
reproduces the race described in the Job and asserts
`[201, 409]`, the loser's `SYSTEM_INITIALIZED` code, exactly one
admin row, and exactly one `refresh_token` row.
## 3. Proposed Changes
No changes. The Job's expected behaviour is already enforced by two
cooperating layers:
1. **Database / Schema Migration:** None. The existing `user.role`
index plus the in-transaction `adminCount` check are sufficient.
2. **Backend Logic & APIs:** `SetupService.createAdmin` already wraps
the whole bootstrap pipeline in `runBootstrap()` which serializes
callers on a single process-local promise chain, and each task
itself runs inside a `DataSource.transaction` whose first action is
the `adminCount > 0` guard. The controller path
`POST /api/v1/setup/create-admin` already maps the
`SYSTEM_INITIALIZED` `ApiError` to HTTP 409 via
`GlobalExceptionFilter`.
3. **Frontend UI Integration:** No change needed. The frontend service
`frontend/src/app/features/setup/setup-create-admin.service.ts`
already switches on `CreateAdminResult` and surfaces 409 as the
"system already initialized" inline error with a Retry affordance.
## 4. Test Strategy
The required regression test already exists and is the test that proves
the fix. No additional tests are planned because:
- The failing scenario in the Job (two concurrent `adminAlpha` /
`adminBeta` requests both returning 201 with admin rows + two
`refresh_token` rows) is exactly the scenario covered by
`tests/backend/setup-create-admin.spec.ts:121-149`.
- Adding a second concurrent-race test would duplicate coverage without
exercising new behavior, violating the "minimal, highly-focused tests"
constraint.
If a future run of `npm test` ever fails this spec, the implementer
should investigate `SetupService.runBootstrap` and the transactional
`adminCount` guard (not add tests).
### Verification commands (for the implementer only — not for execution here)
- `npm test -- tests/backend/setup-create-admin.spec.ts` — confirms
201/409 outcome, single admin row, single `refresh_token` row.
- `npm test` — full Jest run from the repo root.
+1 -1
View File
@@ -3,7 +3,7 @@ type: api
title: Setup Endpoint title: Setup Endpoint
description: Dedicated POST /api/v1/setup/create-admin endpoint used only while no administrator exists. description: Dedicated POST /api/v1/setup/create-admin endpoint used only while no administrator exists.
tags: [api, setup, bootstrap, first-admin] tags: [api, setup, bootstrap, first-admin]
timestamp: 2026-07-21T17:18:08Z timestamp: 2026-07-21T17:37:18Z
--- ---
# Endpoint # Endpoint
+20 -4
View File
@@ -3,7 +3,7 @@ type: database
title: User Table title: User Table
description: The `user` table — accounts, roles, and statuses. description: The `user` table — accounts, roles, and statuses.
tags: [database, user, auth] tags: [database, user, auth]
timestamp: 2026-07-21T14:43:00Z timestamp: 2026-07-21T17:37:18Z
--- ---
# Schema # Schema
@@ -37,14 +37,30 @@ timestamp: 2026-07-21T14:43:00Z
When the `user` table is empty of admins, the dedicated When the `user` table is empty of admins, the dedicated
`POST /api/v1/setup/create-admin` endpoint (see `POST /api/v1/setup/create-admin` endpoint (see
[Setup Endpoint](/api/setup.md)) creates the very first admin and [Setup Endpoint](/api/setup.md)) creates the very first admin and
auto-issues a session. Once any admin row exists the endpoint becomes auto-issues a session. Once any admin row exists the endpoint rejects
inert and returns `404 NOT_FOUND`, so the route is effectively hidden further attempts with `409 SYSTEM_INITIALIZED`, so the route is
in production. effectively hidden in production.
The legacy `POST /api/v1/auth/register-first-admin` route is preserved The legacy `POST /api/v1/auth/register-first-admin` route is preserved
for compatibility but the canonical first-admin flow now lives in the for compatibility but the canonical first-admin flow now lives in the
`SetupModule`. `SetupModule`.
# Concurrent bootstrap safety
Two near-simultaneous `POST /api/v1/setup/create-admin` requests cannot
both succeed. `SetupService.createAdmin` serializes callers on an
in-process promise chain (`#bootstrapChain` + private
`runBootstrap()` in `backend/src/modules/setup/setup.service.ts:111-130`)
so that the first request commits inside its `DataSource.transaction`
before the second request's transaction executes. The loser's
transaction observes `adminCount > 0` and throws
`ApiError(SYSTEM_INITIALIZED, 409)`; the winner is the only request that
creates a `UserEntity(role='admin')` row and mints a refresh token.
The lock is per-process; multi-replica deployments rely on the unique
index and transaction guard, not on this chain. The regression test is
`tests/backend/setup-create-admin.spec.ts` ("only one of two concurrent
first-admin requests succeeds").
# See also # See also
- [Auth and Settings Tables](/database/auth-settings.md) - [Auth and Settings Tables](/database/auth-settings.md)
+1 -1
View File
@@ -11,7 +11,7 @@ scoreboard, an event window with a public countdown, theming, and admin
controls. controls.
The docs below are organized by purpose so agents can pull just the slice The docs below are organized by purpose so agents can pull just the slice
they need. Last regenerated 2026-07-21T17:18:08Z. they need. Last regenerated 2026-07-21T17:37:18Z.
# Architecture # Architecture