40 lines
9.0 KiB
Markdown
40 lines
9.0 KiB
Markdown
# Implementation Plan: Landing Page and Login/Register Modal 1.01
|
||
|
||
## Status
|
||
|
||
This job is **not fully implemented**. The repository already contains the landing page, login/register modal, public registration endpoint, and a nominal ten-per-minute limiter, but the reported end-to-end behavior demonstrates an off-by-one/account-count defect that must be isolated and corrected.
|
||
|
||
## 1. Architectural Reconnaissance
|
||
- **Codebase style & conventions:** Node.js npm-workspace monorepo using TypeScript, NestJS 10, Angular 17 standalone components, async service methods, Angular signals, reactive forms, Zod DTO validation, and Jest. Controllers are thin and delegate business logic to services; the Angular landing component owns modal/form state while `AuthService` owns HTTP calls and session setup.
|
||
- **Data Layer:** TypeORM over `better-sqlite3`; the `user` table has a unique username and stores player/admin accounts. Multi-step registration/session creation runs in a `DataSource.transaction`. No schema migration is expected because rate-limit state is currently in-memory and no user schema change is needed.
|
||
- **Relevant existing flow:** `POST /api/v1/auth/register` in `backend/src/modules/auth/auth.controller.ts` extracts the request IP and calls `AuthService.registerPlayer`. `AuthService.registerPlayer` checks `RegistrationRateLimitService`, checks `registrationsEnabled`, checks username uniqueness, hashes with Argon2id, inserts a player, records a hit, and mints a session. `RegistrationRateLimitService` currently allows fewer than ten retained timestamps. The Angular `/login` route renders `LandingComponent`, whose registration modal calls `AuthService.register`, signs the user in on success, and maps `RATE_LIMITED` errors through `buildLoginFailureMessage`.
|
||
- **Test Framework & Structure:** Root `npm test` runs Jest projects defined in `tests/jest.config.js`; backend tests run in `tests/backend/`, frontend logic tests run in `tests/frontend/`, and tests are not colocated with source. Existing registration integration coverage is in `tests/backend/auth-register.spec.ts`; pure limiter coverage is in `tests/backend/registration-rate-limit.spec.ts`; frontend error-message coverage is in `tests/frontend/landing-modal.spec.ts`.
|
||
- **Required Tools & Dependencies:** No new dependency or system tool is indicated. Existing Node/npm, NestJS, Angular, Jest, TypeORM, SQLite, Supertest, cookie-parser, cookiejar, and Argon2 setup are sufficient. Do not add Redis or another external rate-limit store for this single-process requirement. `setup.sh` already installs dependencies and builds both workspaces; it should remain unchanged unless implementation introduces a dependency, in which case update the lockfile and setup installation path rather than adding runtime infrastructure unnecessarily. Persistent `/data` storage is not needed because limiter state is intentionally ephemeral and tests use `DATABASE_PATH=:memory:`.
|
||
|
||
## 2. Impacted Files
|
||
- **To Modify:**
|
||
- `backend/src/common/services/registration-rate-limit.service.ts` — make the fixed-window boundary and/or reservation semantics explicit so exactly ten successful registrations are admitted and the eleventh is rejected.
|
||
- `backend/src/modules/auth/auth.service.ts` — adjust the registration admission/recording sequence if investigation confirms concurrent requests can pass separate `isAllowed` checks before either records, or otherwise centralize an atomic consume operation.
|
||
- `tests/backend/registration-rate-limit.spec.ts` — add a focused regression test for the exact ten-success/eleventh-rejection/rollover contract, preferably at the service level and with deterministic timestamps.
|
||
- `tests/backend/auth-register.spec.ts` — strengthen the existing public registration integration assertion to verify that the rejected request does not create a user and that a request after the window succeeds, without creating a large test matrix.
|
||
- **To Create:** None expected. Keep tests in the existing dedicated `tests/backend/` folder.
|
||
|
||
## 3. Proposed Changes
|
||
Explain the exact implementation logic step-by-step:
|
||
1. **Database / Schema Migration:** No migration. Preserve the `user` table and existing unique username constraint. The fix must ensure a rejected registration fails before user insertion; successful registrations remain player rows with `role='player'` and `status='enabled'`.
|
||
2. **Backend Logic & APIs:**
|
||
1. Reproduce the reported sequence using deterministic limiter time and the same IP representation used by the controller (`req.ip`, commonly `::ffff:127.0.0.1`). Distinguish a true limiter boundary bug from test contamination or a race between `isAllowed()` and `record()`.
|
||
2. Replace the externally split check-then-record path with an atomic “consume/admit” operation, or add equivalent synchronization inside `RegistrationRateLimitService`. The operation must prune timestamps older than the one-minute window, admit exactly when the retained count is below ten, and reserve/record the timestamp as part of the same operation. A blocked call must not append a timestamp.
|
||
3. Define the rollover boundary consistently: timestamps with age at least 60,000 ms are expired, so the first request after the minute rolls over is admitted. Use an injectable/deterministic `now` argument in the pure service path to avoid sleeping in unit tests; production calls use `Date.now()`.
|
||
4. Call the atomic admission operation at the correct point in `AuthService.registerPlayer`. Preserve existing validation, registration-enabled, username-conflict, Argon2id, transaction, response, CSRF, and refresh-cookie behavior. Decide and document whether failed validation/disabled/duplicate attempts count based on the existing intended semantics; the job’s acceptance criterion concerns successful account creation, so avoid charging the window for requests that cannot create an account unless repository requirements explicitly require otherwise.
|
||
5. Check the legacy first-admin/setup callers because they share `RegistrationRateLimitService`. Preserve their protection and ensure the API error remains `429 RATE_LIMITED`; use the existing public-registration message exactly: `Too many registration attempts; please wait a minute before trying again.`
|
||
6. Do not change the landing-page route or modal unless backend regression analysis exposes a client contract issue. The existing `LandingComponent` already switches login/register modes, calls `AuthService.register`, establishes the session, and routes to `/`; the existing error helper handles the registration rate-limit response as a generic wait message because the server text contains no numeric seconds.
|
||
3. **Frontend UI Integration:** No frontend code change is expected for this server-side off-by-one defect. If implementation changes the API error payload to include a numeric retry duration, update `login-modal.service.ts` and its focused test only to preserve the documented user-facing message; otherwise leave the already implemented landing page and modal intact.
|
||
|
||
## 4. Test Strategy
|
||
- **Target Unit Test File:** `tests/backend/registration-rate-limit.spec.ts` for deterministic limiter behavior: admit exactly ten calls, reject the eleventh without increasing the retained count, isolate IPs, and admit after a timestamp reaches the one-minute expiry boundary. Keep the existing service tests minimal and extend rather than creating a new test hierarchy.
|
||
- **Target Integration Test File:** `tests/backend/auth-register.spec.ts` for one focused public API regression: enable registrations, issue ten unique valid registrations from one agent/IP, assert each returns `201`, submit the eleventh with a unique username and assert `429 RATE_LIMITED`, query the repository or count users to prove no eleventh account exists, then invoke the limiter with deterministic time or use a narrowly controlled clock/expiry strategy to verify the next request after rollover succeeds. Avoid real 31/60-second sleeps.
|
||
- **Frontend Test File:** No new frontend test unless the error contract changes. Existing `tests/frontend/landing-modal.spec.ts` already covers the registration rate-limit mapping and should remain green.
|
||
- **Mocking Strategy:** Unit-test the limiter without mocks. For backend integration, use the existing Nest `AppModule`, in-memory SQLite initialization via `initDb`, real TypeORM persistence, CSRF cookie priming, and Supertest as already established; mock only time if required by the implementation, using Jest fake timers or an injected clock rather than mocking repositories. Ensure app/database resources are closed and avoid shared limiter state between tests by using a fresh service/app or clearing the service state through its public lifecycle/design. All assertions should be CLI-verifiable with the existing root `npm test` command.
|
||
- **Acceptance assertions:** There are exactly ten successful `201` registrations in the one-minute window; the eleventh returns HTTP 429 with code `RATE_LIMITED` and the required message; the rejected username is absent from `user`; the first request after the one-minute window succeeds with `201`; and no unrelated landing/login behavior regresses.
|