feat(auth): add requireRole RBAC preHandler decorator (Story #37)

[steilerDev]

Summary

• Add requireRole() factory function to the auth plugin that returns a Fastify preHandler for role-based access control
• Supports multiple roles via rest params: requireRole('admin') or requireRole('admin', 'member')
• Returns 401 for unauthenticated users, 403 FORBIDDEN for users lacking required role
• Role changes in database take effect on next request without re-login

Test Plan

• 7 integration tests covering:
  • Admin can access admin-protected routes (200)
  • Member gets 403 FORBIDDEN on admin-only routes
  • Unauthenticated gets 401 (auth before role check)
  • Multi-role routes work for both admin and member
  • Role change in DB takes effect on next request
• All 342 tests pass
• Lint, typecheck, format, build, audit all clean

Fixes #37

🤖 Generated with Claude Code

[steilerDev]

[security-engineer]

Security Review: APPROVED ✅

I have reviewed PR #60 for security vulnerabilities related to the role-based access control implementation. No security issues found.

Security Analysis

✅ Authentication Before Authorization

• The requireRole() preHandler explicitly checks if (!request.user) before checking roles, throwing UnauthorizedError (401) for unauthenticated users
• Fastify's lifecycle guarantees preValidation (auth check) runs before preHandler (role check)
• Tests confirm unauthenticated requests get 401, not 403 — preventing information leakage about route existence

✅ Fresh Role Lookup (No Stale Data)

• validateSession() performs a fresh database JOIN between sessions and users on EVERY request
• Roles are read directly from the users table (line 62 in sessionService.ts), NOT cached in the session token
• Role changes take effect immediately without requiring re-login
• Test coverage confirms this behavior (role changes take effect immediately test suite)

✅ Correct HTTP Status Codes

• Returns 403 FORBIDDEN with message "Insufficient permissions" for authenticated users lacking required roles
• Uses ForbiddenError class which maps to the FORBIDDEN ErrorCode
• Does not leak information about whether routes exist or authentication succeeded

✅ No Authorization Bypass Vectors

• Request object manipulation won't work — request.user is populated by the preValidation hook via cryptographically validated session lookup
• The preHandler cannot be skipped — it's registered in the route preHandler array, enforced by Fastify's routing engine
• No caching, no time-of-check-time-of-use (TOCTOU) issues
• Uses Fastify's type-safe hook system with proper error handling

✅ Multi-Role Support

• Variadic requireRole(...roles) design allows flexible RBAC without security compromise
• Uses roles.includes(request.user.role) for safe array membership check
• Tests confirm both admin and member can access multi-role routes

Test Coverage Review

264 lines of comprehensive integration tests covering:

• Admin access to admin-only routes (200 OK)
• Member denied access to admin-only routes (403 FORBIDDEN)
• Unauthenticated denied access (401 UNAUTHORIZED)
• Multi-role route access for both admin and member
• Hook ordering (auth before role check)
• Fresh role lookup (role change test confirms database-driven authorization)

All security-critical scenarios are tested.

Recommendation

APPROVED for merge. This implementation follows secure authorization patterns:

• Defense in depth (authentication + authorization layers)
• Fail-secure design (explicit checks, throws errors on failure)
• Fresh data (no cached roles)
• Clear separation of concerns (401 vs 403)

No remediation required.

[steilerDev]

[product-architect]

Architecture Review: Story #37 — Role-Based Access Control

Verdict: APPROVED

This is a clean, well-scoped implementation that aligns with the established architecture. The changes are minimal, focused, and consistent with our existing auth patterns.

---

Architecture Compliance

Fastify hook pattern — The requireRole() function returns a preHandlerHookHandler, which is the correct Fastify type for route-level preHandlers. This matches the layered hook design documented in ADR-010 (Decision #5): global preValidation for authentication, route-level preHandler for authorization. The separation of concerns is clean.

Plugin structure — The function is correctly exported as a named export from the auth plugin module rather than being registered as a Fastify decorator or plugin itself. This is the right call: requireRole is a factory function that produces hooks, not a plugin. It lives alongside the auth plugin it depends on, which makes the dependency relationship clear.

Error handling — Uses the established AppError subclasses (UnauthorizedError, ForbiddenError) from server/src/errors/AppError.ts. These integrate with the centralized error handler plugin to produce the standard { error: { code, message, details? } } response shape. The error codes UNAUTHORIZED (401) and FORBIDDEN (403) match the API Contract wiki page exactly.

Hook execution order — The comment explaining that preValidation runs before preHandler is accurate and important. For authenticated routes, the global preValidation hook sets request.user; requireRole then checks the role in preHandler. For unauthenticated requests to protected routes, the preValidation hook throws UnauthorizedError before requireRole ever executes. This means the 401-before-403 ordering is guaranteed by the Fastify lifecycle, not by the requireRole function itself. The defensive if (!request.user) check in requireRole is still good practice — it guards against misuse on public routes.

API Contract Consistency

The API Contract wiki page already documents requireRole('admin') as the mechanism for admin-only routes (see "Route Protection" section). This implementation matches that specification exactly. The FORBIDDEN error code with "Insufficient permissions" message is consistent with the error codes table.

Code Quality

TypeScript types — The preHandlerHookHandler import from fastify is the correct type. One observation (non-blocking): the roles parameter is typed as string[], but our schema defines roles as 'admin' | 'member' (see Drizzle schema text('role', { enum: ['admin', 'member'] }) and the shared UserRole type). Using ...roles: UserRole[] from @cornerstone/shared would provide compile-time safety against typos like requireRole('admim'). This is a minor refinement — the current string[] works correctly at runtime since request.user.role will always be a valid role from the database.

JSDoc — Thorough and accurate. Documents parameters, return type, throws, and includes a usage example. Well done.

Naming — requireRole follows our camelCase convention for functions. The inner function roleCheck is a nice touch for stack trace readability.

Test Quality

Coverage — 7 integration tests covering all acceptance criteria from Story #37:

• AC1: requireRole('admin') as preHandler decorator — tested
• AC2: 403 FORBIDDEN for non-admin — tested
• AC3: Auth before role check (401 before 403) — tested explicitly
• AC5: Role field in user context — verified in response assertions
• AC6: Role changes take effect immediately — tested with mid-test DB update

Test structure — Uses the established pattern: buildApp() with temp directory, app.inject() for HTTP-level testing, proper setup/teardown. The approach of registering test-specific routes (/api/test-admin, /api/test-multi-role) is sound — it isolates the RBAC behavior from any specific application route.

Edge case coverage — The "role changes take effect immediately" test (AC6) is particularly good. It verifies that the preValidation hook loads fresh user data from the database on every request, rather than caching the role in the session. This is a critical behavioral guarantee.

Non-Blocking Observations

1. Type narrowing for roles: As noted above, requireRole(...roles: UserRole[]) using the shared UserRole type would catch role string typos at compile time. This is a small improvement that could be picked up in refinement.

2. Empty roles array: requireRole() called with zero arguments would allow any authenticated user through (since roles.includes(user.role) on an empty array returns false, it would actually deny everyone). A runtime guard or TypeScript overload requiring at least one role would make this explicit. Not a real risk since it would be caught immediately in testing, but worth noting for defensive coding.

Summary

The implementation is architecturally sound, consistent with ADR-010, matches the API Contract, and has thorough test coverage against the acceptance criteria. CI passes. No blocking issues.

[steilerDev]

[product-owner] PR Review for Story #37: Role-Based Access Control -- Admin/Member

Acceptance Criteria Verification

AC	Criterion	Verdict	Evidence
1	requireRole('admin') preHandler decorator can be added to any route	PASS	requireRole() exported from auth.ts, returns preHandlerHookHandler, used via preHandler: [requireRole('admin')]
2	Non-admin users receive 403 with error code FORBIDDEN	PASS	ForbiddenError('Insufficient permissions') maps to { error: { code: 'FORBIDDEN', ... } } with HTTP 403. Test asserts response.statusCode === 403 and body.error.code === 'FORBIDDEN'
3	Role check occurs after authentication	PASS	requireRole is a preHandler hook; auth runs in preValidation (earlier lifecycle phase). If request.user is null, throws UnauthorizedError (401) before reaching the role check. Test confirms unauthenticated user gets 401, not 403
4	Admin-only routes for Sprint 1: user management endpoints	PASS	This story delivers the RBAC mechanism (decorator). Actual route policies are applied in Story #38. The decorator is ready to use
5	role field included in session/user context for all route handlers	PASS	request.user typed as typeof users.$inferSelect which includes role. validateSession() selects role from the database. Tests assert body.user.role is accessible and correct
6	Role changes take effect on next request (no re-login)	PASS	validateSession() performs a fresh DB join on every request, loading current role from the users table. Test confirms: member denied (403), role updated to admin in DB without new login, same session succeeds (200) on next request

Result: All 6 acceptance criteria PASS.

Test Coverage

7 integration tests present in server/src/plugins/auth.test.ts:

1. Admin can access admin-protected route (200)
2. Member gets 403 FORBIDDEN on admin-only route
3. Unauthenticated gets 401 UNAUTHORIZED (auth checked before role)
4. Admin can access multi-role route (200)
5. Member can access multi-role route (200)
6. Auth ordering -- missing session returns 401 before role check
7. Role change in DB takes effect on next request without re-login

All 342 tests pass per PR description. CI Quality Gates green.

UAT Scenario Coverage

Cross-referencing the 11 UAT scenarios from the uat-validator:

• Scenarios 1-5, 9-10: Directly covered by the 7 tests
• Scenario 6 (role downgrade): Not explicitly tested. The upgrade path is tested (member -> admin), but the symmetric downgrade (admin -> member) is not. This is a minor gap -- the mechanism is identical (fresh DB read), but an explicit downgrade test would be more thorough. Non-blocking.
• Scenarios 7, 11: Correctly deferred to Story #38 / future stories (endpoints do not exist yet)
• Scenario 8: Error format matches standard shape. Note: UAT suggested message "Admin role required" but implementation uses "Insufficient permissions". The AC specifies only the error code FORBIDDEN, not a specific message. The message "Insufficient permissions" is actually more appropriate since requireRole supports multiple roles, not just admin. Non-blocking.

Test Authorship

The commit is authored by "Claude frontend-developer (Opus 4.6)" with "Claude qa-integration-tester (Sonnet 4.5)" as a co-author. Per CLAUDE.md convention, the QA agent should be the primary author of test code. The QA agent is attributed as co-author, and the tests are well-structured integration tests. Non-blocking observation -- in future stories, the QA agent should ideally be the commit author for test-only changes, or tests should be in a separate commit authored by QA.

Non-Blocking Observations

1. Type safety on requireRole parameter: The function signature is requireRole(...roles: string[]) rather than requireRole(...roles: UserRole[]). Using the UserRole type from @cornerstone/shared would catch typos like requireRole('admn') at compile time. Minor improvement for a future refinement.

2. Missing role downgrade test: As noted above, adding a test for admin -> member downgrade would improve UAT scenario 6 coverage.

3. No scope creep detected: The PR touches only server/src/plugins/auth.ts (production) and server/src/plugins/auth.test.ts (tests). Clean, focused change.

Agent Reviews

Product-architect and security-engineer reviews are expected in parallel. This approval is conditional on those reviews completing without blocking issues.

Verdict

CONDITIONALLY APPROVED. All 6 acceptance criteria pass. 7 well-structured integration tests cover the core scenarios. CI is green. No scope creep. Pending product-architect and security-engineer reviews before merge.

[github-actions]

🎉 This PR is included in version 1.7.0-beta.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[github-actions]

🎉 This PR is included in version 1.7.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀