From 127c563c1deee517024bf7668325a2afb236e157 Mon Sep 17 00:00:00 2001 From: OpenVelo Agent Date: Tue, 21 Jul 2026 17:38:20 +0000 Subject: [PATCH 1/2] docs: update documentation to OKF v0.1 format --- docs/api/setup.md | 2 +- docs/database/users.md | 24 ++++++++++++++++++++---- docs/index.md | 2 +- 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/docs/api/setup.md b/docs/api/setup.md index 0a8ad8b..2afaed7 100644 --- a/docs/api/setup.md +++ b/docs/api/setup.md @@ -3,7 +3,7 @@ type: api title: Setup Endpoint description: Dedicated POST /api/v1/setup/create-admin endpoint used only while no administrator exists. tags: [api, setup, bootstrap, first-admin] -timestamp: 2026-07-21T17:18:08Z +timestamp: 2026-07-21T17:37:18Z --- # Endpoint diff --git a/docs/database/users.md b/docs/database/users.md index c09c6c9..30b0e66 100644 --- a/docs/database/users.md +++ b/docs/database/users.md @@ -3,7 +3,7 @@ type: database title: User Table description: The `user` table — accounts, roles, and statuses. tags: [database, user, auth] -timestamp: 2026-07-21T14:43:00Z +timestamp: 2026-07-21T17:37:18Z --- # Schema @@ -37,14 +37,30 @@ timestamp: 2026-07-21T14:43:00Z When the `user` table is empty of admins, the dedicated `POST /api/v1/setup/create-admin` endpoint (see [Setup Endpoint](/api/setup.md)) creates the very first admin and -auto-issues a session. Once any admin row exists the endpoint becomes -inert and returns `404 NOT_FOUND`, so the route is effectively hidden -in production. +auto-issues a session. Once any admin row exists the endpoint rejects +further attempts with `409 SYSTEM_INITIALIZED`, so the route is +effectively hidden in production. The legacy `POST /api/v1/auth/register-first-admin` route is preserved for compatibility but the canonical first-admin flow now lives in the `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 - [Auth and Settings Tables](/database/auth-settings.md) diff --git a/docs/index.md b/docs/index.md index 52a3e5d..24c2b2a 100644 --- a/docs/index.md +++ b/docs/index.md @@ -11,7 +11,7 @@ scoreboard, an event window with a public countdown, theming, and admin controls. 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 -- 2.52.0 From 03f7a3357024f7778f3e846f10f09b2e32e0f34c Mon Sep 17 00:00:00 2001 From: OpenVelo Agent Date: Tue, 21 Jul 2026 17:38:21 +0000 Subject: [PATCH 2/2] feat: First-Start Create Admin Modal 1.06 --- .kilo/plans/846.md | 52 ---------------------- .kilo/plans/847.md | 107 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 52 deletions(-) delete mode 100644 .kilo/plans/846.md create mode 100644 .kilo/plans/847.md diff --git a/.kilo/plans/846.md b/.kilo/plans/846.md deleted file mode 100644 index e0abf18..0000000 --- a/.kilo/plans/846.md +++ /dev/null @@ -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. diff --git a/.kilo/plans/847.md b/.kilo/plans/847.md new file mode 100644 index 0000000..4bdb04e --- /dev/null +++ b/.kilo/plans/847.md @@ -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. -- 2.52.0