Slice 2: Login + role-aware dashboard happy path

[itlamb]

Summary

• Full email+password authentication: POST /auth/login (JWT HS256, 24h), GET /auth/me (loads patient/doctor profile)
• DB schema: users, patients, doctors, user_role enum via Drizzle ORM
• Seed script: bcrypt cost 12, idempotent upserts, 10 demo accounts
• Web: /login (useMutation form), /patient + /doctor dashboards with beforeLoad guards, logout
• Auth middlewares: requireAuth() (Bearer token), requireRole() (RBAC-ready)
• JWT payload validates role ∈ {doctor, patient} to block escalation
• Logger redaction extended to authorization, jwt, password_hash
• User-repository infrastructure layer separates use-cases from raw db/schema queries

Test plan

• pnpm db:push && pnpm db:seed — apply schema + seed 10 demo accounts
• pnpm test — 30 unit+integration tests green (jwt, env-guard, logger, auth routes, health, seed)
• pnpm typecheck — 3 packages clean
• Login with dr.kowalski@clinic.pl / Pass1234! → lands on /doctor with "Welcome, Marek"
• Login with p.zielinski@mail.pl / Pass1234! → lands on /patient with "Welcome, Jan"
• Wrong password → "Invalid credentials" message, stays on /login
• Refresh page → session persists (token in localStorage, /auth/me rehydrates)
• Logout → clears token, redirects to /login
• Hit /patient unauthenticated → redirected to /login
• pnpm test:e2e — full Playwright e2e suite (doctor+patient login/logout/refresh/guards)

Closes #2

🤖 Generated with Claude Code

[claude]

Claude finished @itlamb's task in 5m 29s —— View job

---

PR Review — Slice 2: Login + role-aware dashboard

• Read CLAUDE.md, ARCHITECTURE_RULES.md, GLOSSARY.md, PRD
• Read linked issue Slice 2: Login + role-aware dashboard happy path #2 acceptance criteria (via PRD)
• Review API: schema, auth routes, JWT, middleware, repository
• Review web: login page, dashboards, auth-token, queries
• Review tests (unit, integration, e2e)
• Security audit (auth, secrets, PII, injection)
• Architecture rule compliance check
• Post final verdict + structured review

---

Verdict: APPROVE ✅ (with non-blocking gaps)

The slice is genuinely vertical (schema → API → contracts → UI → tests), TDD red→green→refactor is visible in the commit history, and the auth surface is correct and reasonably hardened. A few PRD-listed acceptance criteria are not implemented and a couple of minor security/quality polish items are worth tracking, but nothing blocks merge.

---

Spec compliance

AC	Status
POST /auth/login (HS256, 24h)	✅ apps/api/src/routes/auth/login-route.ts
GET /auth/me joins patient/doctor profile	✅ apps/api/src/use-cases/auth/get-user-profile.ts
requireAuth() + requireRole([...]) middlewares	✅ apps/api/src/middleware/require-auth.ts
DB: users, patients, doctors, user_role enum	✅ apps/api/src/db/schema.ts
Seed script (bcrypt cost 12, idempotent, 10 accounts)	✅ apps/api/scripts/seed.ts + data/seed-accounts.json
JWT_SECRET min 32 chars, fail-fast at boot	✅ apps/api/src/lib/env-guard.ts + config.ts
Logger redacts password, password_hash, token, authorization, jwt	✅ apps/api/src/lib/logger.ts
JWT validates role ∈ {doctor, patient} and rejects missing exp	✅ apps/api/src/lib/jwt.ts:54-64
/login page calls typed contract, stores token, redirects by role	✅ apps/web/src/routes/login.tsx
/patient + /doctor beforeLoad route guards (incl. cross-role redirect)	✅ patient.tsx:10-21, doctor.tsx:10-21
Logout clears token + cache, navigates to /login	✅ patient.tsx:30-34, doctor.tsx:30-34
Session persists across refresh (localStorage + /auth/me rehydrate)	✅ covered by e2e
Auth events logged with named events	✅ auth.login.success/failed/db_error, auth.token.invalid/expired, auth.rbac.denied
Catch-all / redirects to /login (unauth) or dashboard (auth)	❌ apps/web/src/routes/index.tsx still renders the API-health smoke page from slice 1 — PRD §118 explicitly says "open /, get redirected to /login"
Hitting /login while authenticated redirects to dashboard	❌ login.tsx has no beforeLoad; an authenticated user lands on the form (PRD §108)
Vitest component tests for login form + route-guard wrappers	❌ Not present (PRD §129); e2e covers the surface but the spec called for both layers
Expired-token e2e (mutate localStorage to bad token, assert redirect)	❌ Not present (PRD §128)
Orphaned-subject path (token valid, user deleted) covered in tests	❌ me-route.ts:21-26 handles it but no test exercises it

---

Architecture findings

• Code-based routes vs file-based. ARCHITECTURE_RULES.md:22 mandates "TanStack Router file-based route definitions"; this PR wires routes manually via createRoute(...) and rootRoute.addChildren([...]) in apps/web/src/main.tsx:21. Functionally equivalent, but worth either adopting the Vite plugin / generated routeTree.gen.ts, or amending the rule via an ADR so downstream modules don't inherit the inconsistency. Fix this →
• createApp(deps?) makes auth optional. apps/api/src/app.ts:15-22 only mounts the auth router when deps is supplied. This exists so health.test.ts can boot the app without a DB, but it's a foot-gun: a future caller forgetting to pass deps ships an API with auth silently disabled. Prefer always-required deps and pass a fake/in-memory wiring in the health test.
• Layering is clean. routes → use-cases → infrastructure (user-repository) → drizzle — the recent refactor (27e01fa) correctly extracted SQL out of the use-cases. Domain has zero infra imports. ✅
• Env-loading duplicated 3×. Identical .env parsers in vitest.config.ts:5-15, drizzle.config.ts:8-17, and seed.ts:71-81. Either depend on dotenv once or extract a single helper.
• env-guard overlaps config. Both validate JWT_SECRET ≥ 32 and process.exit(1). env-guard.ts is dead weight once config.ts:6 exists; index.ts calls both back-to-back. Drop one.

---

Security findings

• Non-constant-time signature compare. apps/api/src/lib/jwt.ts:48 uses expectedSig !== sig. Hard to exploit over a network for HMAC, but trivially fixable — base64-decode both sides and use crypto.timingSafeEqual. Fix this →
• Email logged in plaintext on failed login. use-cases/auth/login.ts:21,27 logs { email } for auth.login.failed. Email is the login identifier per the glossary, mild PII, and not redacted by the logger. Acceptable for the prototype, but if logs are ever shipped externally consider hashing or dropping the field.
• Token in localStorage is XSS-readable. Acknowledged trade-off in the PRD (line 81 explicitly accepts this) — flagging only for the record. Future hardening would move to HttpOnly cookies + CSRF.
• No alg-confusion vector. Verifier hardcodes HMAC-SHA256 and ignores the header alg field, so alg: none and RS↔HS swaps fail signature check. ✅
• Email lookup is case-insensitive at both write and read. seed.ts:27 lowercases on insert; user-repository.ts:13 lowercases on lookup with lower(users.email). Belt-and-braces; safe. The DB-side lower() defeats the unique-email index, but at 10 seeded rows it doesn't matter.
• Login endpoint is unrate-limited. Out-of-scope per PRD §145 — flag for future modules.

---

Test quality

• Strong unit coverage for jwt (happy path, expired, tampered sig, wrong secret, malformed, missing exp, junk role) and the redacting logger. Tests are behavior-focused, not implementation-focused. ✅
• Integration tests gate cleanly on DATABASE_URL (describe.skip when missing) — this means CI without a DB silently skips real auth coverage. Acceptable, but the skip should be loud somewhere (e.g. fail the verify if no DB in CI).
• Missing: orphaned-subject case in auth.test.ts (a token whose sub no longer exists) — me-route.ts:25 returns 401 Account not found, and no test exercises this branch.
• Missing: any frontend Vitest tests. PRD §129 specifically asks for component tests on the login form and the route-guard wrappers. The Playwright e2e covers the same surface but at much higher cost; component tests would catch validation-message regressions in <1s.
• Missing: expired-token e2e (localStorage.setItem('medbridge_token', '<expired>') → assert /login redirect). PRD §128.
• No tautological tests spotted. Each test exercises real behavior through the public surface (app.request(...) for HTTP, real DB for seed).

---

Suggestions (non-blocking)

1. De-dup the patient.tsx / doctor.tsx dashboards. They are byte-for-byte identical aside from role text/labels. Extract a <RoleDashboard role={...} /> once a third surface lands; not now.
2. Move 401-on-protected-request handling into apiClient. lib/api-client.ts:23 returns the body unchanged; the dashboard beforeLoad is currently the only place that clears the token. PRD §107 wants this centralized so any future protected query gets the same treatment. A small if (response.status === 401 && path !== '/auth/login') { clearToken(); } (plus the toast PRD §107 mentions) covers it.
3. createApp should require deps. Update health.test.ts to construct a no-op logger + null-shape config so deps is always present.
4. Consider dotenv rather than three hand-rolled env parsers.

---

What's solid

• Layered architecture with the post-refactor user-repository is exactly the pattern downstream modules should follow.
• TDD red→green discipline visible in commit history (every feat(green) is preceded by a test(red)).
• Logger redaction list now covers authorization / jwt / password_hash, with array+nested coverage tested.
• Cross-role redirect (/patient → /doctor for a doctor) is a nice-to-have that's both implemented and e2e-covered.
• Drizzle schema is minimal and correct: cascading FKs from profile tables, $onUpdate for updated_at, UUID PKs.

---

· branch: afk/issue-2