# 2026-07-17 — `requireRole.js` middleware + RBAC matrix tests (Sprint A1 / P1-10)

Branch: `fix/p1-10-require-role`
Worktree: `.worktrees/fix-p1-10-require-role`

## Why

Inline `if (!['school_admin', 'systems_admin'].includes(req.user.role))` checks
are scattered across 23+ controllers. Some are subtly inconsistent (one
controller blocks teachers, another doesn't; one allows principal, another
doesn't). P1-10 from the master readiness audit called for a single
middleware helper + an executable form of the `SCREENS.md` permission
matrix so we can catch a widening access regression in CI.

## Changes

### `server/src/middleware/requireRole.js` (new)

- `requireRole(...allowed)` — factory. Returns 401 if no `req.user`,
  403 if role not in the allowed set, otherwise calls `next()`.
- `requireAdmin` — systems_admin, school_admin, principal. Maps to the
  rows where SCREENS.md grants principal View access (e.g. `/users`,
  `/departments`).
- `requireStrictAdmin` — systems_admin, school_admin only. Maps to
  routes that intentionally exclude principal (audit logs, settings,
  academic rollover execution).
- `requireStaff` — admin/principal + hr/bursar/accountant/nurse/dining/
  teacher/librarian/clubs_head. Excludes student and parent.
- `requireOwnerOrAdmin` — admin OR `req.user.id === req.params.[userId|id|studentId]`,
  with explicit parent pass-through.
- `ROLE` — frozen object exposing every role string as a stable constant
  so callers don't repeat string literals.

### `server/tests/require-role.test.js` (new, 15 specs)

Unit tests with no DB or Express:

- Allow / 403 / 401 / missing-role cells for `requireRole`.
- `requireAdmin` accepts admin+principal, rejects teacher/student/parent.
- `requireStaff` admits every staff + teacher + librarian + clubs_head,
  rejects student/parent.
- `requireOwnerOrAdmin` admits admins, the URL-param owner, parents;
  rejects unrelated students.
- `ROLE` is frozen.

### `server/tests/rbac-matrix.test.js` (new, 152 specs)

Builds a tiny Express app that mounts the `requireRole` middleware with
fake handlers, mocks `req.user` from an `X-Test-Role` header, then walks
every (role, route) cell of the SCREENS.md matrix and asserts the
expected HTTP status (200 / 403 / 401 for anonymous). 10 matrix rows ×
13 roles + an extra negative-cell regression block = 152 specs. All
green.

This is the executable form of `SCREENS.md §Role Permissions Matrix`.

### Controller refactors (4 files)

- **`server/src/controllers/audit.controller.js`** — local `adminOnly` is
  now `requireStrictAdmin` re-aliased. Same predicate, one source.
- **`server/src/controllers/academic-rollover.controller.js`** — same.
- **`server/src/controllers/users.controller.js`** — local `adminOnly` is
  now `requireAdmin` (admin + principal, matches SCREENS.md matrix).
- **`server/src/controllers/payments.controller.js`** — the inline check
  on `GET /api/payments/student/:studentId` is replaced with
  `requireOwnerOrAdmin` mounted between `auth` and the handler. The
  parent pass-through behavior is preserved exactly as before
  (controller-level scope by parent-student mapping is unchanged).

These are demonstration refactors; the remaining 19 controllers with
inline RBAC predicates are out of scope for this PR but should follow
the same pattern over the next sprints. `conventions.md` will list the
canonical middleware aliases so the sweep converges.

## Verification

- `npx vitest run tests/require-role.test.js` → 15 passed (15 ms)
- `npx vitest run tests/rbac-matrix.test.js` → 152 passed (411 ms)
- `npx vitest run` (full suite) → **185 passed**, 1 pre-existing failure
  in `paynow-webhook.test.js` ("paid_amount" undefined) caused by the
  dev DB lacking a `student_fees` row at id=1 in this worktree. The
  failure is independent of these changes — `git diff` against `dev`
  shows only controller refactors that do not affect that test's query.
- `node --check` clean on every modified file.

## Out of scope (deferred)

- 19 remaining controllers with inline RBAC; sweep to be scheduled.
- `conventions.md` update with the alias table.
- Express-rate-limit on auth (left in place; was already covered by
  existing rate-limit middleware where enabled).