92 lines
4.2 KiB
Markdown
92 lines
4.2 KiB
Markdown
# 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).
|