From 79bc2ef25b56737ac96396c513ed807c0661b7a0 Mon Sep 17 00:00:00 2001 From: saqib mir Date: Mon, 7 Sep 2026 21:22:49 +0530 Subject: [PATCH] feat(013-auth-hardening): password reset, password strength policy, login rate-limiting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../checklists/requirements.md | 22 ++++ specs/013-auth-hardening/tasks.md | 40 +++---- src/config/auth.ts | 4 + src/config/env.ts | 9 ++ .../identity/agents/service/users.service.ts | 7 +- .../auth/controller/auth.controller.ts | 22 +++- src/modules/identity/auth/index.ts | 1 + src/modules/identity/auth/mapper/index.ts | 2 + .../identity/auth/mapper/password-policy.ts | 13 ++ .../identity/auth/mapper/reset-token.ts | 18 +++ .../auth/repository/auth.repository.ts | 5 + src/modules/identity/auth/repository/index.ts | 1 + .../auth/repository/reset-token.repository.ts | 30 +++++ .../identity/auth/routes/auth.routes.ts | 8 ++ .../identity/auth/schema/auth.schema.ts | 18 +++ .../identity/auth/service/auth.service.ts | 93 +++++++++++++- tests/helpers/auth.ts | 20 ++-- tests/integration/login-rate-limit.test.ts | 74 ++++++++++++ tests/integration/password-reset-flow.test.ts | 113 ++++++++++++++++++ .../identity/login-failure-parity.test.ts | 7 +- .../login-rate-limit-ordering.test.ts | 37 ++++++ tests/unit/identity/password-policy.test.ts | 17 +++ 22 files changed, 524 insertions(+), 37 deletions(-) create mode 100644 src/modules/identity/auth/mapper/password-policy.ts create mode 100644 src/modules/identity/auth/mapper/reset-token.ts create mode 100644 src/modules/identity/auth/repository/reset-token.repository.ts create mode 100644 tests/integration/login-rate-limit.test.ts create mode 100644 tests/integration/password-reset-flow.test.ts create mode 100644 tests/unit/identity/login-rate-limit-ordering.test.ts create mode 100644 tests/unit/identity/password-policy.test.ts diff --git a/specs/013-auth-hardening/checklists/requirements.md b/specs/013-auth-hardening/checklists/requirements.md index ffcb65d..80bba7c 100644 --- a/specs/013-auth-hardening/checklists/requirements.md +++ b/specs/013-auth-hardening/checklists/requirements.md @@ -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`. diff --git a/specs/013-auth-hardening/tasks.md b/specs/013-auth-hardening/tasks.md index 073b3b0..fe7e45f 100644 --- a/specs/013-auth-hardening/tasks.md +++ b/specs/013-auth-hardening/tasks.md @@ -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) diff --git a/src/config/auth.ts b/src/config/auth.ts index c48c512..d15b5f5 100644 --- a/src/config/auth.ts +++ b/src/config/auth.ts @@ -3,4 +3,8 @@ import { env } from './env'; export const authConfig = { jwtSecret: env.JWT_SECRET, tokenLifetimeHours: env.AUTH_TOKEN_LIFETIME_HOURS, + passwordMinLength: env.PASSWORD_MIN_LENGTH, + passwordResetTokenLifetimeMinutes: env.PASSWORD_RESET_TOKEN_LIFETIME_MINUTES, + loginRateLimitMaxAttempts: env.LOGIN_RATE_LIMIT_MAX_ATTEMPTS, + loginRateLimitWindowSeconds: env.LOGIN_RATE_LIMIT_WINDOW_SECONDS, }; diff --git a/src/config/env.ts b/src/config/env.ts index 386d03c..cdf638a 100644 --- a/src/config/env.ts +++ b/src/config/env.ts @@ -65,6 +65,15 @@ const envSchema = z.object({ // already-required JWT_SECRET above (defined since the original scaffold, never consumed // until now) — see specs/010-identity-auth/research.md. AUTH_TOKEN_LIFETIME_HOURS: z.coerce.number().default(4), + + // Authentication Hardening (013) — password-strength policy, reset-token lifetime, and + // login rate-limiting, all CONFIGURABLE per docs/10-implementation-roadmap.md's own + // "never hardcode a placeholder value and ship it as final" instruction — see + // specs/013-auth-hardening/research.md. + PASSWORD_MIN_LENGTH: z.coerce.number().default(10), + PASSWORD_RESET_TOKEN_LIFETIME_MINUTES: z.coerce.number().default(30), + LOGIN_RATE_LIMIT_MAX_ATTEMPTS: z.coerce.number().default(5), + LOGIN_RATE_LIMIT_WINDOW_SECONDS: z.coerce.number().default(300), }); export type EnvConfig = z.infer; diff --git a/src/modules/identity/agents/service/users.service.ts b/src/modules/identity/agents/service/users.service.ts index fdc6267..ebd2389 100644 --- a/src/modules/identity/agents/service/users.service.ts +++ b/src/modules/identity/agents/service/users.service.ts @@ -1,16 +1,19 @@ import { User } from '@prisma/client'; import { ConflictError } from '@/common/errors'; -import { hashPassword } from '@/modules/identity/auth'; +import { hashPassword, validatePasswordStrength } from '@/modules/identity/auth'; import { usersRepository, UsersRepository } from '../repository'; import { CreateUserBody } from '../schema'; export class UsersService { constructor(private readonly repo: UsersRepository = usersRepository) {} - /** FR-008: rejects a duplicate email — never a second account silently sharing one. */ + /** FR-008: rejects a duplicate email — never a second account silently sharing one. + * 013-auth-hardening FR-005: the same password-strength policy every password-setting call + * site enforces. */ async create(body: CreateUserBody): Promise> { const existing = await this.repo.findByEmail(body.email); if (existing) throw new ConflictError('An account with this email already exists.'); + validatePasswordStrength(body.password); const passwordHash = await hashPassword(body.password); const user = await this.repo.create({ diff --git a/src/modules/identity/auth/controller/auth.controller.ts b/src/modules/identity/auth/controller/auth.controller.ts index 3c25e52..a04a710 100644 --- a/src/modules/identity/auth/controller/auth.controller.ts +++ b/src/modules/identity/auth/controller/auth.controller.ts @@ -1,7 +1,7 @@ import { FastifyReply, FastifyRequest } from 'fastify'; import { AuthenticationError } from '@/common/errors'; import { authService, AuthService } from '../service'; -import { loginSchema } from '../schema'; +import { loginSchema, requestPasswordResetSchema, resetPasswordSchema } from '../schema'; function bearerToken(request: FastifyRequest): string { const header = request.headers.authorization; @@ -29,6 +29,26 @@ export class AuthController { await this.service.logout(bearerToken(request)); return reply.status(200).send({ success: true, data: { loggedOut: true }, meta: null }); } + + /** 013-auth-hardening FR-001/SC-001: identical response regardless of account existence — + * the service itself is what decides whether a real token gets issued. */ + async requestPasswordReset(request: FastifyRequest, reply: FastifyReply) { + const { email } = requestPasswordResetSchema.parse(request.body); + await this.service.requestPasswordReset(email); + return reply.status(200).send({ + success: true, + data: { message: 'If that account exists, a reset link has been sent.' }, + meta: null, + }); + } + + async resetPassword(request: FastifyRequest, reply: FastifyReply) { + const { token, newPassword } = resetPasswordSchema.parse(request.body); + await this.service.resetPassword(token, newPassword); + return reply + .status(200) + .send({ success: true, data: { message: 'Password updated.' }, meta: null }); + } } export const authController = new AuthController(); diff --git a/src/modules/identity/auth/index.ts b/src/modules/identity/auth/index.ts index 28dfdc1..62b3f78 100644 --- a/src/modules/identity/auth/index.ts +++ b/src/modules/identity/auth/index.ts @@ -4,4 +4,5 @@ export { requireRole } from './service'; export type { LoginBody } from './schema'; export type { LoginResult } from './service'; export { hashPassword, verifyPassword, signToken, verifyToken, toAuthUser } from './mapper'; +export { validatePasswordStrength } from './mapper'; export { AUTH_CONSTANTS } from './constants'; diff --git a/src/modules/identity/auth/mapper/index.ts b/src/modules/identity/auth/mapper/index.ts index 0a117f4..7d06a81 100644 --- a/src/modules/identity/auth/mapper/index.ts +++ b/src/modules/identity/auth/mapper/index.ts @@ -1 +1,3 @@ export * from './auth.mapper'; +export * from './password-policy'; +export * from './reset-token'; diff --git a/src/modules/identity/auth/mapper/password-policy.ts b/src/modules/identity/auth/mapper/password-policy.ts new file mode 100644 index 0000000..97adcac --- /dev/null +++ b/src/modules/identity/auth/mapper/password-policy.ts @@ -0,0 +1,13 @@ +import { ValidationError } from '@/common/errors'; +import { authConfig } from '@/config'; + +/** 013-auth-hardening FR-005: the one password-strength rule, enforced identically everywhere + * a password is ever set (010's own POST /admin/users and this feature's own password-reset + * consume endpoint) — never duplicated or allowed to drift between call sites. */ +export function validatePasswordStrength(password: string): void { + if (password.length < authConfig.passwordMinLength) { + throw new ValidationError( + `Password must be at least ${authConfig.passwordMinLength} characters.`, + ); + } +} diff --git a/src/modules/identity/auth/mapper/reset-token.ts b/src/modules/identity/auth/mapper/reset-token.ts new file mode 100644 index 0000000..01a703f --- /dev/null +++ b/src/modules/identity/auth/mapper/reset-token.ts @@ -0,0 +1,18 @@ +import { randomBytes, createHash } from 'crypto'; + +export interface GeneratedResetToken { + token: string; + tokenHash: string; +} + +/** 013-auth-hardening: the raw token is what gets "delivered" (logged, per the stub decision, + * research.md); only its SHA-256 hash is ever persisted (data-model.md) — mirrors this + * codebase's own password-hashing discipline, never storing a usable secret at rest. */ +export function generateResetToken(): GeneratedResetToken { + const token = randomBytes(32).toString('hex'); + return { token, tokenHash: hashResetToken(token) }; +} + +export function hashResetToken(token: string): string { + return createHash('sha256').update(token).digest('hex'); +} diff --git a/src/modules/identity/auth/repository/auth.repository.ts b/src/modules/identity/auth/repository/auth.repository.ts index 1f31e40..ef234b8 100644 --- a/src/modules/identity/auth/repository/auth.repository.ts +++ b/src/modules/identity/auth/repository/auth.repository.ts @@ -14,6 +14,11 @@ export class AuthRepository { if (!user || !user.active) return null; return user; } + + /** 013-auth-hardening: applies a password-reset's new hash. */ + async updatePassword(id: string, passwordHash: string): Promise { + await this.prisma.user.update({ where: { id }, data: { passwordHash } }); + } } export const authRepository = new AuthRepository(); diff --git a/src/modules/identity/auth/repository/index.ts b/src/modules/identity/auth/repository/index.ts index cda518a..9a62bc0 100644 --- a/src/modules/identity/auth/repository/index.ts +++ b/src/modules/identity/auth/repository/index.ts @@ -1 +1,2 @@ export * from './auth.repository'; +export * from './reset-token.repository'; diff --git a/src/modules/identity/auth/repository/reset-token.repository.ts b/src/modules/identity/auth/repository/reset-token.repository.ts new file mode 100644 index 0000000..191b218 --- /dev/null +++ b/src/modules/identity/auth/repository/reset-token.repository.ts @@ -0,0 +1,30 @@ +import { cacheService } from '@/infrastructure/cache'; + +const TOKEN_KEY_PREFIX = 'password-reset:token:'; +const USER_KEY_PREFIX = 'password-reset:user:'; + +/** 013-auth-hardening data-model.md: two paired Redis keys per active reset token — the same + * Redis-key-with-TTL shape as 010's own revocation denylist. Only one active token exists per + * user at any time (FR-002): issuing a new one deletes the prior token's own key. */ +export class ResetTokenRepository { + async issue(userId: string, tokenHash: string, ttlSeconds: number): Promise { + const priorHash = await cacheService.get(`${USER_KEY_PREFIX}${userId}`); + if (priorHash) { + await cacheService.del(`${TOKEN_KEY_PREFIX}${priorHash}`); + } + await cacheService.set(`${TOKEN_KEY_PREFIX}${tokenHash}`, userId, ttlSeconds); + await cacheService.set(`${USER_KEY_PREFIX}${userId}`, tokenHash, ttlSeconds); + } + + async resolve(tokenHash: string): Promise { + return cacheService.get(`${TOKEN_KEY_PREFIX}${tokenHash}`); + } + + /** Single-use (FR-002/SC-002): deletes both keys for this token/user pair. */ + async consume(tokenHash: string, userId: string): Promise { + await cacheService.del(`${TOKEN_KEY_PREFIX}${tokenHash}`); + await cacheService.del(`${USER_KEY_PREFIX}${userId}`); + } +} + +export const resetTokenRepository = new ResetTokenRepository(); diff --git a/src/modules/identity/auth/routes/auth.routes.ts b/src/modules/identity/auth/routes/auth.routes.ts index 4943aa2..1b135f8 100644 --- a/src/modules/identity/auth/routes/auth.routes.ts +++ b/src/modules/identity/auth/routes/auth.routes.ts @@ -11,4 +11,12 @@ export async function authRoutes(fastify: FastifyInstance): Promise { fastify.post('/auth/logout', { preHandler: fastify.authenticate }, (req, reply) => authController.handleLogout(req, reply), ); + + // 013-auth-hardening: ungated, like login itself — the caller has no session yet. + fastify.post('/auth/password-reset/request', (req, reply) => + authController.requestPasswordReset(req, reply), + ); + fastify.post('/auth/password-reset/consume', (req, reply) => + authController.resetPassword(req, reply), + ); } diff --git a/src/modules/identity/auth/schema/auth.schema.ts b/src/modules/identity/auth/schema/auth.schema.ts index 2b82ccb..3faebd7 100644 --- a/src/modules/identity/auth/schema/auth.schema.ts +++ b/src/modules/identity/auth/schema/auth.schema.ts @@ -8,3 +8,21 @@ export const loginSchema = z .strict(); export type LoginBody = z.infer; + +/** 013-auth-hardening */ +export const requestPasswordResetSchema = z + .object({ + email: z.string().email(), + }) + .strict(); + +export type RequestPasswordResetBody = z.infer; + +export const resetPasswordSchema = z + .object({ + token: z.string().min(1), + newPassword: z.string().min(1), + }) + .strict(); + +export type ResetPasswordBody = z.infer; diff --git a/src/modules/identity/auth/service/auth.service.ts b/src/modules/identity/auth/service/auth.service.ts index 8a12044..17f40ce 100644 --- a/src/modules/identity/auth/service/auth.service.ts +++ b/src/modules/identity/auth/service/auth.service.ts @@ -1,8 +1,23 @@ import { User } from '@prisma/client'; -import { AuthenticationError } from '@/common/errors'; -import { revokeToken } from '@/infrastructure/cache'; -import { authRepository, AuthRepository } from '../repository'; -import { verifyPassword, signToken, verifyToken } from '../mapper'; +import { AppError, AuthenticationError, RateLimitError } from '@/common/errors'; +import { checkRateLimit, revokeToken } from '@/infrastructure/cache'; +import { logger } from '@/infrastructure/observability'; +import { authConfig } from '@/config'; +import { + authRepository, + AuthRepository, + resetTokenRepository, + ResetTokenRepository, +} from '../repository'; +import { + verifyPassword, + signToken, + verifyToken, + hashPassword, + generateResetToken, + hashResetToken, + validatePasswordStrength, +} from '../mapper'; import { LoginBody } from '../schema'; export interface LoginResult { @@ -15,14 +30,29 @@ function toPublicUser(user: User): LoginResult['user'] { } export class AuthService { - constructor(private readonly repo: AuthRepository = authRepository) {} + constructor( + private readonly repo: AuthRepository = authRepository, + private readonly resetTokens: ResetTokenRepository = resetTokenRepository, + ) {} /** * FR-002/SC-003: every failure branch (no such email, inactive account, wrong password) * throws the identical AuthenticationError — bcrypt.compare always runs exactly once, * against a fixed dummy hash when no user is found, so timing never leaks which branch fired. + * 013-auth-hardening FR-006/FR-007: the rate-limit check runs first, before any credential + * work — a rate-limited attempt never reaches (and can't distinguish itself via timing from) + * the identical-failure-response path below. */ async login(body: LoginBody): Promise { + const rateLimit = await checkRateLimit( + `login:${body.email}`, + authConfig.loginRateLimitMaxAttempts, + authConfig.loginRateLimitWindowSeconds, + ); + if (!rateLimit.allowed) { + throw new RateLimitError('Too many login attempts. Try again later.'); + } + const user = await this.repo.findByEmail(body.email); const passwordMatches = await verifyPassword(body.password, user?.passwordHash ?? null); @@ -46,6 +76,59 @@ export class AuthService { const remainingSeconds = Math.max(1, (payload.exp ?? 0) - Math.floor(Date.now() / 1000)); await revokeToken(payload.jti, remainingSeconds); } + + /** + * 013-auth-hardening FR-001/SC-001: always resolves the same way regardless of whether the + * email corresponds to a real, active account — only issues a real token when it does. The + * "delivery" step is a stubbed structured log line (research.md), not a real email. + */ + async requestPasswordReset(email: string): Promise { + const user = await this.repo.findByEmail(email); + if (user && user.active) { + const { token, tokenHash } = generateResetToken(); + await this.resetTokens.issue( + user.id, + tokenHash, + authConfig.passwordResetTokenLifetimeMinutes * 60, + ); + logger.info( + { + event: 'password_reset_requested', + userId: user.id, + resetUrl: `/reset-password?token=${token}`, + }, + 'Password reset requested — stubbed delivery (013-auth-hardening research.md): no real ' + + 'email is sent yet, this log line is the only place the token is visible.', + ); + } + // Same outcome either way (FR-001) — no branch here reveals which case fired. + } + + /** + * 013-auth-hardening FR-004/FR-005: password strength is checked before the token is even + * looked up (data-model.md); the token itself is single-use (SC-002) — resolving and + * consuming it happen together so a second attempt with the same token always fails. + * Edge Cases: a token issued for an account later deactivated is rejected — reactivation is + * 010's own admin domain, not something this flow performs incidentally. + */ + async resetPassword(token: string, newPassword: string): Promise { + validatePasswordStrength(newPassword); + + const tokenHash = hashResetToken(token); + const userId = await this.resetTokens.resolve(tokenHash); + if (!userId) { + throw new AppError('Invalid or expired reset token.', 'INVALID_RESET_TOKEN', 400); + } + await this.resetTokens.consume(tokenHash, userId); + + const user = await this.repo.findActiveById(userId); + if (!user) { + throw new AppError('Invalid or expired reset token.', 'INVALID_RESET_TOKEN', 400); + } + + const passwordHash = await hashPassword(newPassword); + await this.repo.updatePassword(userId, passwordHash); + } } export const authService = new AuthService(); diff --git a/tests/helpers/auth.ts b/tests/helpers/auth.ts index a1d6a8c..de66d8a 100644 --- a/tests/helpers/auth.ts +++ b/tests/helpers/auth.ts @@ -1,3 +1,4 @@ +import { randomUUID } from 'crypto'; import { FastifyInstance } from 'fastify'; import bcrypt from 'bcryptjs'; import { prismaClient } from '@/infrastructure/database'; @@ -6,20 +7,23 @@ const TEST_PASSWORD = 'Test-Password-123!'; /** * 010-identity-auth made fastify.authenticate real — every test file calling a route already - * gated by it (across 002-009's own suites) needs a real session now. Rather than depend on + * gated by it (across 002-009's own suites) needs a real session now. This creates its own + * throwaway admin/agent account directly and logs in as it, so callers don't depend on * prisma/seed/roles.seed.ts having already been run against whatever database the suite - * connects to, this upserts its own throwaway admin/agent account directly (idempotent — safe - * to call from many test files' own beforeAll against the same database) and logs in as it. + * connects to. + * + * 013-auth-hardening: the email is unique per call (not a fixed `test-admin@...` shared across + * every integration test file) because login is now rate-limited per email — dozens of files + * each calling this once in their own beforeAll would otherwise share one rate-limit bucket and + * trip it well before any file's own tests get to run. */ export async function loginAs( app: FastifyInstance, role: 'ADMIN' | 'AGENT' = 'ADMIN', ): Promise { - const email = `test-${role.toLowerCase()}@supporthub.test`; - await prismaClient.user.upsert({ - where: { email }, - update: {}, - create: { + const email = `test-${role.toLowerCase()}-${randomUUID()}@supporthub.test`; + await prismaClient.user.create({ + data: { email, name: `Test ${role}`, role, diff --git a/tests/integration/login-rate-limit.test.ts b/tests/integration/login-rate-limit.test.ts new file mode 100644 index 0000000..2ed759f --- /dev/null +++ b/tests/integration/login-rate-limit.test.ts @@ -0,0 +1,74 @@ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { buildApp } from '@/app'; +import { prismaClient } from '@/infrastructure/database'; +import { FastifyInstance } from 'fastify'; +import bcrypt from 'bcryptjs'; +import { authConfig } from '@/config'; + +/** + * Covers specs/013-auth-hardening/quickstart.md Scenario 3 against a real Postgres/Redis. + */ +describe('Login rate limiting (User Story 3)', () => { + let app: FastifyInstance; + const suffix = Date.now(); + const email = `rate-limit-test-${suffix}@supporthub.test`; + const otherEmail = `rate-limit-other-${suffix}@supporthub.test`; + const correctPassword = 'Correct-Password-1!'; + let userId: string; + let otherUserId: string; + + beforeAll(async () => { + app = await buildApp(); + const user = await prismaClient.user.create({ + data: { + email, + name: 'Rate Limit Test User', + role: 'AGENT', + passwordHash: await bcrypt.hash(correctPassword, 10), + }, + }); + userId = user.id; + + const otherUser = await prismaClient.user.create({ + data: { + email: otherEmail, + name: 'Rate Limit Other User', + role: 'AGENT', + passwordHash: await bcrypt.hash(correctPassword, 10), + }, + }); + otherUserId = otherUser.id; + }); + + afterAll(async () => { + await prismaClient.user.deleteMany({ where: { id: { in: [userId, otherUserId] } } }); + await app.close(); + }); + + it('blocks the same email after its attempt budget is exhausted, without affecting other emails', async () => { + for (let i = 0; i < authConfig.loginRateLimitMaxAttempts; i++) { + const res = await app.inject({ + method: 'POST', + url: '/auth/login', + payload: { email, password: 'definitely-wrong' }, + }); + expect(res.statusCode).toBe(401); + } + + // One more attempt for the same email, this time with the CORRECT password — still 429. + const blockedRes = await app.inject({ + method: 'POST', + url: '/auth/login', + payload: { email, password: correctPassword }, + }); + expect(blockedRes.statusCode).toBe(429); + + // A different email in the same window is unaffected. + const otherRes = await app.inject({ + method: 'POST', + url: '/auth/login', + payload: { email: otherEmail, password: correctPassword }, + }); + expect(otherRes.statusCode).toBe(200); + }); +}); diff --git a/tests/integration/password-reset-flow.test.ts b/tests/integration/password-reset-flow.test.ts new file mode 100644 index 0000000..74d7d25 --- /dev/null +++ b/tests/integration/password-reset-flow.test.ts @@ -0,0 +1,113 @@ +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import { buildApp } from '@/app'; +import { prismaClient } from '@/infrastructure/database'; +import { FastifyInstance } from 'fastify'; +import bcrypt from 'bcryptjs'; +import { logger } from '@/infrastructure/observability'; + +/** + * Covers specs/013-auth-hardening/quickstart.md Scenario 1 against a real Postgres/Redis — the + * full request -> (read the token from the stub's own log line) -> consume -> login-with-new- + * password flow, and the identical-response-regardless-of-existing-account behavior. + */ +describe('Password reset flow (User Story 1)', () => { + let app: FastifyInstance; + const suffix = Date.now(); + const email = `reset-test-${suffix}@supporthub.test`; + const originalPassword = 'Original-Password-1!'; + const newPassword = 'Brand-New-Password-2!'; + let userId: string; + + beforeAll(async () => { + app = await buildApp(); + const user = await prismaClient.user.create({ + data: { + email, + name: 'Reset Test User', + role: 'AGENT', + passwordHash: await bcrypt.hash(originalPassword, 10), + }, + }); + userId = user.id; + }); + + afterAll(async () => { + await prismaClient.user.deleteMany({ where: { id: userId } }); + await app.close(); + }); + + function extractLoggedToken(): string { + const infoSpy = vi.mocked(logger.info); + const call = infoSpy.mock.calls.find( + ([data]) => (data as { event?: string }).event === 'password_reset_requested', + ); + if (!call) throw new Error('Expected a password_reset_requested log line, but none was found.'); + + const resetUrl = (call[0] as unknown as { resetUrl: string }).resetUrl; + const token = new URL(resetUrl, 'http://localhost').searchParams.get('token'); + if (!token) throw new Error('Expected the logged resetUrl to carry a token query param.'); + return token; + } + + it('Scenario 1: request -> stub-logged token -> consume -> login with the new password', async () => { + const infoSpy = vi.spyOn(logger, 'info'); + + const requestRes = await app.inject({ + method: 'POST', + url: '/auth/password-reset/request', + payload: { email }, + }); + expect(requestRes.statusCode).toBe(200); + expect(requestRes.json().data.message).not.toMatch(/token|[a-f0-9]{64}/i); + + const nonexistentRes = await app.inject({ + method: 'POST', + url: '/auth/password-reset/request', + payload: { email: `nobody-${suffix}@supporthub.test` }, + }); + expect(nonexistentRes.statusCode).toBe(200); + expect(nonexistentRes.json()).toEqual(requestRes.json()); + + const token = extractLoggedToken(); + + const consumeRes = await app.inject({ + method: 'POST', + url: '/auth/password-reset/consume', + payload: { token, newPassword }, + }); + expect(consumeRes.statusCode).toBe(200); + + // Single-use — the same token fails a second time. + const secondConsumeRes = await app.inject({ + method: 'POST', + url: '/auth/password-reset/consume', + payload: { token, newPassword: 'Another-Password-3!' }, + }); + expect(secondConsumeRes.statusCode).toBe(400); + + const loginWithNew = await app.inject({ + method: 'POST', + url: '/auth/login', + payload: { email, password: newPassword }, + }); + expect(loginWithNew.statusCode).toBe(200); + + const loginWithOld = await app.inject({ + method: 'POST', + url: '/auth/login', + payload: { email, password: originalPassword }, + }); + expect(loginWithOld.statusCode).toBe(401); + + infoSpy.mockRestore(); + }); + + it('rejects an invalid token outright', async () => { + const res = await app.inject({ + method: 'POST', + url: '/auth/password-reset/consume', + payload: { token: 'not-a-real-token', newPassword: 'Whatever-Password-1!' }, + }); + expect(res.statusCode).toBe(400); + }); +}); diff --git a/tests/unit/identity/login-failure-parity.test.ts b/tests/unit/identity/login-failure-parity.test.ts index c0e45c4..da510db 100644 --- a/tests/unit/identity/login-failure-parity.test.ts +++ b/tests/unit/identity/login-failure-parity.test.ts @@ -1,6 +1,7 @@ -import { describe, it, expect, vi } from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; import bcrypt from 'bcryptjs'; import { AuthService } from '@/modules/identity/auth/service/auth.service'; +import * as cache from '@/infrastructure/cache'; const REAL_PASSWORD_HASH = bcrypt.hashSync('the-real-password', 10); @@ -19,6 +20,10 @@ function fakeUser(overrides: Partial> = {}) { } describe('AuthService.login failure parity', () => { + beforeEach(() => { + vi.spyOn(cache, 'checkRateLimit').mockResolvedValue({ allowed: true, count: 1 }); + }); + it('throws the identical error for a nonexistent email and a wrong password', async () => { const repoFoundUser = { findByEmail: vi.fn().mockResolvedValue(fakeUser()) } as never; const repoNoUser = { findByEmail: vi.fn().mockResolvedValue(null) } as never; diff --git a/tests/unit/identity/login-rate-limit-ordering.test.ts b/tests/unit/identity/login-rate-limit-ordering.test.ts new file mode 100644 index 0000000..754d289 --- /dev/null +++ b/tests/unit/identity/login-rate-limit-ordering.test.ts @@ -0,0 +1,37 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { AuthService } from '@/modules/identity/auth/service/auth.service'; +import * as cache from '@/infrastructure/cache'; + +describe('AuthService.login rate-limit ordering (User Story 3)', () => { + beforeEach(() => { + vi.restoreAllMocks(); + }); + + it('checks the rate limit before ever looking up the account', async () => { + const findByEmail = vi.fn().mockResolvedValue(null); + const repo = { findByEmail } as never; + const service = new AuthService(repo); + + vi.spyOn(cache, 'checkRateLimit').mockResolvedValue({ allowed: false, count: 6 }); + + await expect( + service.login({ email: 'agent@example.com', password: 'anything' }), + ).rejects.toMatchObject({ statusCode: 429 }); + + expect(findByEmail).not.toHaveBeenCalled(); + }); + + it('proceeds to credential checks once the rate limit allows the attempt', async () => { + const findByEmail = vi.fn().mockResolvedValue(null); + const repo = { findByEmail } as never; + const service = new AuthService(repo); + + vi.spyOn(cache, 'checkRateLimit').mockResolvedValue({ allowed: true, count: 1 }); + + await expect( + service.login({ email: 'agent@example.com', password: 'anything' }), + ).rejects.toMatchObject({ statusCode: 401 }); + + expect(findByEmail).toHaveBeenCalledWith('agent@example.com'); + }); +}); diff --git a/tests/unit/identity/password-policy.test.ts b/tests/unit/identity/password-policy.test.ts new file mode 100644 index 0000000..611a0aa --- /dev/null +++ b/tests/unit/identity/password-policy.test.ts @@ -0,0 +1,17 @@ +import { describe, it, expect } from 'vitest'; +import { validatePasswordStrength } from '@/modules/identity/auth/mapper/password-policy'; +import { authConfig } from '@/config'; + +describe('validatePasswordStrength', () => { + it('rejects a password shorter than the configured minimum, naming the actual requirement', () => { + const tooShort = 'a'.repeat(authConfig.passwordMinLength - 1); + expect(() => validatePasswordStrength(tooShort)).toThrowError( + `Password must be at least ${authConfig.passwordMinLength} characters.`, + ); + }); + + it('accepts a password meeting the configured minimum', () => { + const meetsPolicy = 'a'.repeat(authConfig.passwordMinLength); + expect(() => validatePasswordStrength(meetsPolicy)).not.toThrow(); + }); +});