feat: First-Start Create Admin Modal 1.06
This commit is contained in:
OpenVelo Agent
@@ -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.
|
||||
@@ -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.
|
||||
Reference in New Issue
Block a user