fix(contact-logs): authorize writes by MP security role, sanitize numeric IDs

[chriskehayias]

Closes four TODOs across two related defect sets in the contact-log write path.

Why one PR: the two sets are inseparable at the import level. The contact-log actions now call sanitizeNumericId, and that helper is also used by contactLogService, userService, contact-lookup-details, and authorizationService. Splitting them would leave either half non-compiling. The filter-injection half is the work of a concurrent session (mpnext-d2), carried here rather than in a PR of its own for that reason.

1. Authorization — authenticate and authorize

Closes .claude/TODO/contact-log-actions-authenticate-but-not-authorize.md

The actions confirmed a session existed, then acted on whatever ID they were handed. deleteContactLog was the sharpest edge: any authenticated session could delete any contact log in the domain by ID.

The TODO required a policy decision. It was made:

Any authenticated user who holds a Ministry Platform security role may create, edit, and delete any contact log — including one another user created.

Ownership (Made_By) is deliberately not a factor: contact logs are shared pastoral records and staff need to correct and remove each other's entries. MP security roles are the domain's own authorization mechanism, so the app defers to them rather than inventing a parallel permission model that could drift out of sync.

New src/services/authorizationService.ts owns the gate and fails closed — a session with no resolvable MP User_ID, or an MP user holding no security role, gets UnauthorizedError plus a greppable mp.write.unauthorized log line (table, operation, userId, reason of no_mp_user / no_security_role / role_not_permitted).

No caching, on purpose: one extra MP read per write, so a revoked role takes effect immediately. A stale authorization decision is not a trade worth making against a shared production database.

MP_WRITE_SECURITY_ROLES="Administrators,Pastoral Staff" narrows the gate to named roles without a code change; unset means any security role. Reads stay authentication-only, documented as a choice — every authenticated user is MP staff who can already see this data in MP itself.

The policy is encoded in tests that would fail under a different one (should NOT delete when the caller holds no security role, should permit editing a log made by a different user), so a future reader knows the absence of an ownership check was chosen rather than overlooked.

2. Stop re-implementing User_ID resolution

Closes .claude/TODO/contact-log-actions-bypass-session-context-service.md

createContactLog and updateContactLog each re-implemented the dp_Users lookup inline, byte-for-byte — work resolveMpUserId already does and caches, and which SessionContextService.getActingUserIdForWrite() exists to serve. Both blocks are gone, along with the getUserGuid helper and the MPHelper / sanitizeGuid imports. A write now costs zero dp_Users round-trips.

mp.write.non_user is still emitted for an unresolved acting user, so the attempt stays visible in logs. Whether the write then proceeds is now the authorization gate's decision rather than an accident of a failed lookup.

⚠️ Behavior change: updateContactLog no longer stamps Made_By with the editor. That column records who made the contact, not who last touched the row. Since any role-holder may edit anyone's log, stamping the editor rewrote the pastoral record's authorship — a supervisor fixing a typo became the person who made the phone call. MP's audit trail still captures the editor via $userId.

3. Tests for the component driving MP writes

Closes .claude/TODO/contact-logs-component-untested.md

contact-logs.tsx was 602 lines at 0% coverage on the app's only interactive MP write path. Now 87.6% statements via 13 targeted tests: the delete-confirmation gate, client-side validation, and error surfacing.

Verified by mutation, not by coverage percentage — making handleDeleteClick call deleteContactLog(logId) directly, the exact "delete fires before the confirmation resolves" regression the TODO named, fails all four gate tests. The suite that preceded them failed none of it.

4. Numeric-ID filter injection

Closes .claude/TODO/mp-filter-injection-numeric-ids.md — credit to mpnext-d2.

New sanitizeNumericId validates numeric primary-key IDs before interpolation. A TypeScript number annotation is erased at runtime and server actions compile to POST endpoints whose payload shape the caller controls, so a string reaches a number parameter. The old !id || id <= 0 guard was a no-op against that: for '1 OR 1=1', !id is false and id <= 0 is false.

Applied at every numeric filter boundary: contact-logs actions, contact-lookup-details actions (a second entry point into getContactLogsByContactId the TODO had missed), contactLogService, userService, authorizationService.

⚠️ Behavior change: searchContactLogs(0) now throws instead of silently performing an unfiltered top-50 read — the old if (contactId) treated 0 as "no filter". Nothing calls searchContactLogs outside tests.

Verification

Gate	Result
npx vitest run	575 passed, 32 files (was 474)
npx tsc --noEmit	clean
npx eslint .	clean
npm run build	clean
coverage thresholds	pass; contact-logs/actions.ts 100% statements

Docs

• references/auth.md — authorization section, policy rationale, MP_WRITE_SECURITY_ROLES
• references/ministryplatform.query-syntax.md — mandatory-sanitization section
• references/testing.md — Radix-under-jsdom patterns, inventory, "verify a gate test actually gates"
• docs/TestCoverage.md — §5.1, §5.4, §5.5, §6 marked fixed

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!