feat(013-auth-hardening): password reset, password strength policy, login rate-limiting
Closes the two gaps 010-identity-auth explicitly deferred (password reset, login rate-limiting), plus a shared password-strength validator both the reset-consume endpoint and admin account creation now depend on. - Password reset: single-use, paired-Redis-key tokens (never in Postgres), identical response regardless of account existence, stubbed delivery via a structured log line (no email infrastructure exists yet). - Password strength: one validatePasswordStrength() call site, wired into both POST /admin/users and the reset-consume flow. - Login rate-limiting: checkRateLimit keyed by submitted email, checked before any credential verification. Also fixes tests/helpers/auth.ts's shared loginAs() helper, which reused two fixed accounts across the whole integration suite via upsert — now rate-limited per email, that collided across ~30 files sharing one budget. Each call now gets a unique email; no call sites needed to change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
saqib mir
co-authored by
Claude Sonnet 5
Claude Sonnet 5
parent
3bdccc901f
commit
79bc2ef25b
@@ -43,3 +43,25 @@
|
||||
direct, unavoidable dependency of User Story 1 — a password-reset flow that accepts any
|
||||
password would be hardening one gap while leaving the other wide open at the same door.
|
||||
- All items pass; no revision iterations were needed.
|
||||
|
||||
## Implementation Notes (post-build)
|
||||
|
||||
- `tests/helpers/auth.ts`'s shared `loginAs()` helper previously reused two fixed accounts
|
||||
(`test-admin@supporthub.test` / `test-agent@supporthub.test`) across every integration test
|
||||
file via `upsert`. Once login became rate-limited per email (User Story 3), the ~30 files that
|
||||
each call it once in their own `beforeAll` collectively exceeded the attempt budget for those
|
||||
two shared addresses well before most files' own tests ran, turning their legitimate logins
|
||||
into `429`s. Fixed by giving each `loginAs()` call its own unique, randomly-suffixed email —
|
||||
nothing in the suite depended on the literal fixed addresses, so no call sites needed to
|
||||
change, only the helper itself.
|
||||
- While re-running the full suite for regression, `tests/integration/orchestration-strategies.test.ts`'s
|
||||
"SKILL_BASED prefers the eligible agent with the higher proficiency level" test was found
|
||||
failing (picks the lower-proficiency agent). Verified via `git stash` that this reproduces
|
||||
identically on the clean pre-013 `HEAD` with none of this feature's changes present — it is a
|
||||
pre-existing bug in 007-orchestration-assignment's `SKILL_BASED` strategy, unrelated to and out
|
||||
of scope for this feature. Left unfixed here; worth its own follow-up.
|
||||
- `tests/integration/ticket-attachments.test.ts`'s 2 known MinIO-dependent failures (accepted
|
||||
baseline, this project doesn't run MinIO) remain unchanged by this feature.
|
||||
- All other integration and unit tests pass, including 010-identity-auth's own login/admin-account
|
||||
tests, confirming no regression from `AuthService.login`'s new rate-limit check or the shared
|
||||
`validatePasswordStrength` call added to `UsersService.create`.
|
||||
|
||||
@@ -23,7 +23,7 @@ All file paths are relative to `supporthub-api/` (repo root).
|
||||
|
||||
## Phase 1: Foundational (Blocking Prerequisites)
|
||||
|
||||
- [ ] T001 Add `PASSWORD_MIN_LENGTH` (default `10`),
|
||||
- [x] T001 Add `PASSWORD_MIN_LENGTH` (default `10`),
|
||||
`PASSWORD_RESET_TOKEN_LIFETIME_MINUTES` (default `30`),
|
||||
`LOGIN_RATE_LIMIT_MAX_ATTEMPTS` (default `5`), and `LOGIN_RATE_LIMIT_WINDOW_SECONDS`
|
||||
(default `300`) to `src/config/env.ts`, exposed via `src/config/auth.ts`'s existing
|
||||
@@ -42,18 +42,18 @@ the existing admin account-creation endpoint.
|
||||
|
||||
### Tests for User Story 2
|
||||
|
||||
- [ ] T002 [P] [US2] Unit test for `validatePasswordStrength` (too-short rejected with the
|
||||
- [x] T002 [P] [US2] Unit test for `validatePasswordStrength` (too-short rejected with the
|
||||
actual minimum named; policy-meeting password passes) in
|
||||
`tests/unit/identity/password-policy.test.ts`
|
||||
|
||||
### Implementation for User Story 2
|
||||
|
||||
- [ ] T003 [US2] Add `identity/auth/mapper/password-policy.ts`'s
|
||||
- [x] T003 [US2] Add `identity/auth/mapper/password-policy.ts`'s
|
||||
`validatePasswordStrength(password): void`, throwing `ValidationError` (depends on T001)
|
||||
- [ ] T004 [US2] Export it from `identity/auth`'s public `index.ts` (depends on T003)
|
||||
- [ ] T005 [US2] Call it from `identity/agents/service/users.service.ts`'s `UsersService.create`,
|
||||
- [x] T004 [US2] Export it from `identity/auth`'s public `index.ts` (depends on T003)
|
||||
- [x] T005 [US2] Call it from `identity/agents/service/users.service.ts`'s `UsersService.create`,
|
||||
before hashing (depends on T004)
|
||||
- [ ] T006 [US2] Run Quickstart Scenario 2 step 1 locally and confirm it passes; re-run
|
||||
- [x] T006 [US2] Run Quickstart Scenario 2 step 1 locally and confirm it passes; re-run
|
||||
010-identity-auth's own existing `POST /admin/users` tests to confirm no regression
|
||||
|
||||
**Checkpoint**: No password shorter than the policy can ever be set via the admin endpoint.
|
||||
@@ -68,31 +68,31 @@ the existing admin account-creation endpoint.
|
||||
|
||||
### Tests for User Story 1
|
||||
|
||||
- [ ] T007 [US1] Integration test covering Quickstart Scenario 1 (request issues a token via
|
||||
- [x] T007 [US1] Integration test covering Quickstart Scenario 1 (request issues a token via
|
||||
the log stub; a nonexistent email gets an identical response; consume succeeds once and
|
||||
fails the second time; login works with the new password and fails with the old) in
|
||||
`tests/integration/password-reset-flow.test.ts` (depends on T006)
|
||||
|
||||
### Implementation for User Story 1
|
||||
|
||||
- [ ] T008 [US1] Add `identity/auth/mapper/reset-token.ts` — `generateResetToken()` (raw token +
|
||||
- [x] T008 [US1] Add `identity/auth/mapper/reset-token.ts` — `generateResetToken()` (raw token +
|
||||
its SHA-256 hash) (depends on T001)
|
||||
- [ ] T009 [US1] Add `identity/auth/repository/reset-token.repository.ts` — `issue(userId,
|
||||
- [x] T009 [US1] Add `identity/auth/repository/reset-token.repository.ts` — `issue(userId,
|
||||
tokenHash, ttlSeconds)` (deletes any prior token for this user first, per data-model.md's
|
||||
paired-key shape), `resolve(tokenHash)` (returns `userId` or null), `consume(tokenHash,
|
||||
userId)` (deletes both keys) (depends on T008)
|
||||
- [ ] T010 [US1] Add `AuthService.requestPasswordReset(email)`: always returns the same public
|
||||
- [x] T010 [US1] Add `AuthService.requestPasswordReset(email)`: always returns the same public
|
||||
result; internally, if the email resolves to an active account, issues a token and logs
|
||||
the stub delivery event (structured log, research.md) (depends on T009)
|
||||
- [ ] T011 [US1] Add `AuthService.resetPassword(token, newPassword)`: validates password
|
||||
- [x] T011 [US1] Add `AuthService.resetPassword(token, newPassword)`: validates password
|
||||
strength first (depends on T004), then resolves/consumes the token, 400s with a specific
|
||||
reason if the token is missing/expired/used, hashes and stores the new password (depends
|
||||
on T009, T004)
|
||||
- [ ] T012 [US1] Add `POST /auth/password-reset/request` and `POST /auth/password-reset/consume`
|
||||
- [x] T012 [US1] Add `POST /auth/password-reset/request` and `POST /auth/password-reset/consume`
|
||||
(both ungated — no session exists yet) in `identity/auth/controller/` + `routes/` +
|
||||
`schema/`, registered from `src/api/routes.ts` (already registers `authRoutes` as a
|
||||
whole, so no new registration call needed — depends on T010, T011)
|
||||
- [ ] T013 [US1] Run Quickstart Scenario 1 locally and confirm all 5 steps pass
|
||||
- [x] T013 [US1] Run Quickstart Scenario 1 locally and confirm all 5 steps pass
|
||||
|
||||
**Checkpoint**: A locked-out user has a real, working self-service fix.
|
||||
|
||||
@@ -107,21 +107,21 @@ credential verification.
|
||||
|
||||
### Tests for User Story 3
|
||||
|
||||
- [ ] T014 [P] [US3] Unit test confirming the rate-limit check is invoked before
|
||||
- [x] T014 [P] [US3] Unit test confirming the rate-limit check is invoked before
|
||||
`repo.findByEmail`/`verifyPassword` in `AuthService.login` (a fake repo/mapper that would
|
||||
throw if called after an already-exceeded limit) in
|
||||
`tests/unit/identity/login-rate-limit-ordering.test.ts`
|
||||
- [ ] T015 [US3] Integration test covering Quickstart Scenario 3 (N attempts get 401, the N+1th
|
||||
- [x] T015 [US3] Integration test covering Quickstart Scenario 3 (N attempts get 401, the N+1th
|
||||
— even with the correct password — gets 429, a different email is unaffected) in
|
||||
`tests/integration/login-rate-limit.test.ts` (depends on T001)
|
||||
|
||||
### Implementation for User Story 3
|
||||
|
||||
- [ ] T016 [US3] In `AuthService.login`, call the existing
|
||||
- [x] T016 [US3] In `AuthService.login`, call the existing
|
||||
`checkRateLimit(`login:${email}`, authConfig.loginRateLimitMaxAttempts,
|
||||
authConfig.loginRateLimitWindowSeconds)` (from `@/infrastructure/cache`) as the very first
|
||||
step, throwing `RateLimitError` if exceeded (depends on T001)
|
||||
- [ ] T017 [US3] Run Quickstart Scenario 3 locally and confirm all 3 steps pass
|
||||
- [x] T017 [US3] Run Quickstart Scenario 3 locally and confirm all 3 steps pass
|
||||
|
||||
**Checkpoint**: All three user stories work independently and together — this feature's full
|
||||
scope.
|
||||
@@ -130,10 +130,10 @@ scope.
|
||||
|
||||
## Phase 5: Polish & Cross-Cutting Concerns
|
||||
|
||||
- [ ] T018 [P] Update `specs/013-auth-hardening/checklists/requirements.md` Notes with any
|
||||
- [x] T018 [P] Update `specs/013-auth-hardening/checklists/requirements.md` Notes with any
|
||||
implementation-time findings
|
||||
- [ ] T019 Run `npx tsx scripts/check-architecture.ts` and `npm run lint`/`npm run typecheck`
|
||||
- [ ] T020 Full regression: `npm run test:unit` then the full integration suite against real
|
||||
- [x] T019 Run `npx tsx scripts/check-architecture.ts` and `npm run lint`/`npm run typecheck`
|
||||
- [x] T020 Full regression: `npm run test:unit` then the full integration suite against real
|
||||
Docker-provisioned Postgres/Redis, confirming nothing outside this feature regressed
|
||||
(particularly 010-identity-auth's own login/admin-account tests, now touched by this
|
||||
feature's changes)
|
||||
|
||||
Reference in New Issue
Block a user