From ddadbd56609a6b77b561888ec08e8f78e51d930a Mon Sep 17 00:00:00 2001 From: Chris Kehayias Date: Fri, 21 Aug 2026 08:10:20 -0400 Subject: [PATCH] fix(contact-logs): authorize writes by MP security role, sanitize numeric IDs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes four TODOs across two related defect sets in the contact-log write path. Both sets are in one commit because they 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. ## Authorization: authenticate AND authorize Closes .claude/TODO/contact-log-actions-authenticate-but-not-authorize.md The contact-log 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. Policy decision (the TODO required one, and it was made rather than assumed): 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. New src/services/authorizationService.ts owns the gate. It 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 carrying table, operation, userId and a reason of no_mp_user / no_security_role / role_not_permitted. Roles are re-read per write with no caching, so a revoked role takes effect immediately — one extra MP read on a rare operation is worth more than a stale authorization decision against a shared production database. MP_WRITE_SECURITY_ROLES 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. ## Refactor: 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 User_ID lookup inline, byte-for-byte — work resolveMpUserId already does and caches, and which SessionContextService.getActingUserIdForWrite() exists to serve. Both inline blocks are gone, along with the getUserGuid helper and the MPHelper / sanitizeGuid imports. The acting User_ID now comes from the session-baked value customSession resolved, so a write costs no dp_Users round-trip. 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. ## Tests: 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 covering 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 confirmation resolves" regression the TODO named — fails all four gate tests. The suite that preceded them failed none of it. ## Filter injection: numeric IDs Closes .claude/TODO/mp-filter-injection-numeric-ids.md This half is the work of concurrent session mpnext-d2, carried here rather than in a PR of its own because of the import-level coupling described above. New sanitizeNumericId in filter-sanitize.ts 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 that the TODO had missed), contactLogService, userService, and 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 575 tests passing, 32 files (was 474 / 32) npx tsc --noEmit clean npx eslint . clean npm run build clean coverage thresholds pass; contact-logs/actions.ts at 100% statements Docs: authorization policy and MP_WRITE_SECURITY_ROLES in references/auth.md; mandatory-sanitization section in references/ministryplatform.query-syntax.md; Radix-under-jsdom test patterns and inventory in references/testing.md; docs/TestCoverage.md sections 5.1, 5.4, 5.5 and 6 marked fixed. Co-Authored-By: Claude Opus 5 (1M context) --- ...-actions-authenticate-but-not-authorize.md | 49 --- ...-actions-bypass-session-context-service.md | 79 ---- .../TODO/contact-logs-component-untested.md | 44 -- .../TODO/mp-filter-injection-numeric-ids.md | 68 --- .claude/docs/TestCoverage.md | 119 ++++-- .claude/references/auth.md | 83 ++++ .../ministryplatform.query-syntax.md | 22 + .claude/references/testing.md | 72 +++- src/components/contact-logs/actions.test.ts | 401 +++++++++++++----- src/components/contact-logs/actions.ts | 167 ++++---- .../contact-logs/contact-logs.test.tsx | 343 +++++++++++++++ .../contact-lookup-details/actions.test.ts | 29 +- .../contact-lookup-details/actions.ts | 7 +- .../utils/filter-sanitize.test.ts | 126 +++++- .../utils/filter-sanitize.ts | 32 ++ src/services/authorizationService.test.ts | 255 +++++++++++ src/services/authorizationService.ts | 194 +++++++++ src/services/contactLogService.test.ts | 83 ++++ src/services/contactLogService.ts | 16 +- src/services/userService.test.ts | 21 + src/services/userService.ts | 10 +- 21 files changed, 1725 insertions(+), 495 deletions(-) delete mode 100644 .claude/TODO/contact-log-actions-authenticate-but-not-authorize.md delete mode 100644 .claude/TODO/contact-log-actions-bypass-session-context-service.md delete mode 100644 .claude/TODO/contact-logs-component-untested.md delete mode 100644 .claude/TODO/mp-filter-injection-numeric-ids.md create mode 100644 src/components/contact-logs/contact-logs.test.tsx create mode 100644 src/services/authorizationService.test.ts create mode 100644 src/services/authorizationService.ts diff --git a/.claude/TODO/contact-log-actions-authenticate-but-not-authorize.md b/.claude/TODO/contact-log-actions-authenticate-but-not-authorize.md deleted file mode 100644 index f7c41bba..00000000 --- a/.claude/TODO/contact-log-actions-authenticate-but-not-authorize.md +++ /dev/null @@ -1,49 +0,0 @@ -# TODO: Contact-log actions authenticate but never authorize - -**Created:** 2026-08-21 -**Severity:** Medium-High — needs a policy decision before it can be called a bug or a feature. -**Status:** Open. Documented during the test-coverage push. Requires a product decision, not just code. - -## Symptom - -In `src/components/contact-logs/actions.ts`, `updateContactLog`, `deleteContactLog`, -`getContactLogById`, and `getContactLogsByContactId` all confirm that *a* valid session exists, then -act on whatever ID they are handed. Nothing verifies that: - -- the contact log belongs to the caller, or -- the caller is permitted to touch that contact at all. - -`deleteContactLog` is the sharpest edge. Unlike create/update it does not even resolve `userGuid`: - -```ts -const session = await auth.api.getSession({ headers: await headers() }); -if (!session?.user?.id) throw new Error("Authentication required"); -if (!contactLogId || contactLogId <= 0) throw new Error("Valid Contact Log ID is required"); -await contactLogService.deleteContactLog(contactLogId); -``` - -Any authenticated session can delete **any** contact log in the domain by ID. Given CLAUDE.md's -stance on MP write safety ("Ministry Platform is a shared production database containing real church -member data"), this needs an explicit decision rather than an implicit one. - -## The decision to make - -Either: - -**(a)** "Any authenticated staff user may read, edit, and delete any contact log" is the intended -policy — MP itself is a staff-facing system and this may well match how the church operates. If so, -document it in `.claude/references/auth.md` and add a test that *encodes* the decision, so a future -reader knows it was chosen rather than overlooked. - -**(b)** Ownership or role gating is required. Then `deleteContactLog` and `updateContactLog` should -load the log first, compare `Made_By` against the acting `User_ID`, and reject mismatches unless the -caller holds a supervisory role. - -Option (a) is plausible and cheap. What is not acceptable is leaving it ambiguous — the current tests -mirror the code's assumptions exactly (`deleteContactLog(42)` asserts the service was called with -`42`), so they keep passing under either policy and encode nothing. - -## Related - -- `.claude/TODO/mp-filter-injection-numeric-ids.md` — the same entry points, different defect -- `.claude/docs/TestCoverage.md` §7.5 diff --git a/.claude/TODO/contact-log-actions-bypass-session-context-service.md b/.claude/TODO/contact-log-actions-bypass-session-context-service.md deleted file mode 100644 index 5c845263..00000000 --- a/.claude/TODO/contact-log-actions-bypass-session-context-service.md +++ /dev/null @@ -1,79 +0,0 @@ -# TODO: Contact-log actions re-implement User_ID resolution instead of using `SessionContextService` - -**Created:** 2026-08-21 -**Severity:** Medium — redundant MP round-trips per write, duplicated logic, and it contradicts the -project's own decided policy on unattributed writes. -**Status:** Open. Refactor, not a bug — behavior is currently correct-ish but the wrong shape. - -## Symptom - -`src/components/contact-logs/actions.ts` resolves the acting user's MP `User_ID` inline, and does it -**twice** — once in `createContactLog` and again, byte-for-byte, in `updateContactLog`: - -```ts -const { MPHelper } = await import("@/lib/providers/ministry-platform"); -const mp = new MPHelper(); -const users = await mp.getTableRecords<{ User_ID: number }>({ - table: "dp_Users", - filter: `User_GUID = '${sanitizeGuid(userGuid)}'`, - select: "User_ID", - top: 1 -}); -if (!users || users.length === 0 || !users[0].User_ID) { - throw new Error("Unable to determine user User_ID"); -} -``` - -Three separate things are wrong with this: - -### 1. The work is already done - -`src/lib/auth.ts` has `resolveMpUserId`, which performs exactly this `dp_Users` lookup, caches it -process-wide by `User_GUID`, and bakes the result into the session as `session.user.userId` via -`customSession`. The actions ignore that and pay an uncached MP round-trip on **every single write**. - -### 2. `SessionContextService` exists for precisely this call site - -`src/services/sessionContextService.ts` is documented as the canonical way to get the acting user for -a write: - -> "Use this — not `getCurrentUserId` — at every MP write boundary." - -`getActingUserIdForWrite({ table, operation })` reads `session.user.userId` (free, already resolved) -and emits a structured `mp.write.non_user` warning when it comes back null. It is fully tested at -100%. Nothing in `contact-logs/actions.ts` calls it. - -### 3. Throwing on an unresolved user contradicts the decided policy - -The actions throw `"Unable to determine user User_ID"` and abandon the write. `SessionContextService` -was built on the opposite premise — anonymous writes are legitimate and should be *logged*, not -*blocked*, so the unattributed write is visible in production logs and can be investigated. Right now -a user whose `dp_Users` row is missing or whose lookup transiently fails simply cannot save a contact -log at all. - -## Fix - -Replace both inline blocks with: - -```ts -import { sessionContextService } from "@/services/sessionContextService"; -... -const userId = await sessionContextService.getActingUserIdForWrite({ - table: "Contact_Log", - operation: "create", // or "update" -}); -const logDataWithUser = { ...contactLogData, Made_By: userId }; -``` - -Then delete the now-unused `getUserGuid` helper and the `MPHelper`/`sanitizeGuid` imports if nothing -else in the file needs them. - -Confirm before doing this that `Made_By` accepts null on the MP side. If it does not, the graceful -path is to omit the field rather than to fail the write — decide and document which. - -## Test impact - -`src/components/contact-logs/actions.test.ts` mocks `MPHelper.getTableRecords` to satisfy the inline -lookup; those mocks get replaced with a `sessionContextService` mock. Add a case asserting that a -null acting user still performs the write and emits `mp.write.non_user`, which is the behavior change -this refactor is really about. diff --git a/.claude/TODO/contact-logs-component-untested.md b/.claude/TODO/contact-logs-component-untested.md deleted file mode 100644 index 899c568e..00000000 --- a/.claude/TODO/contact-logs-component-untested.md +++ /dev/null @@ -1,44 +0,0 @@ -# TODO: `contact-logs.tsx` — 602 lines driving MP writes, 0% coverage - -**Created:** 2026-08-21 -**Severity:** Medium — the largest untested file in the app, on the app's only write path. -**Status:** Open. Explicitly **out of scope** for the >90% non-UI coverage target (this is a React -component), but it is the highest-value remaining test gap in the repo and should not get lost. - -## Symptom - -`src/components/contact-logs/contact-logs.tsx` is 602 lines at 0% coverage. It is the component that -drives contact-log **create / update / delete** against Ministry Platform. It owns: - -- form state and client-side validation -- the delete-confirmation gate -- optimistic updates and rollback -- error handling and user-facing error surfaces - -The server actions beneath it sit at 97.8% coverage — but the actions are the easy half. The form -logic, the confirmation gate, and the error handling are where a regression silently corrupts or -deletes real member data. - -## Why it matters more than the coverage number suggests - -CLAUDE.md is unambiguous about MP write safety. The only interactive path a user has to mutate MP -data in this app goes through code that no test has ever executed. A regression that, say, fires -delete before the confirmation resolves would not be caught by anything in the suite. - -## Suggested approach — targeted, not exhaustive - -`@testing-library/react` is already installed and `components/layout/auth-wrapper.test.tsx` proves -the harness works. Do not chase full render coverage. Three tests, in priority order: - -1. **The delete-confirmation gate.** Clicking delete does *not* call `deleteContactLog` until the - confirmation is accepted; cancelling calls nothing. -2. **Form validation before submit.** Missing `Contact_Date` or `Notes` does not reach - `createContactLog` (the action throws on these, but the component should never send them). -3. **Action failure surfaces to the user** and does not leave an optimistic row in place. - -Even these three beat zero by a wide margin. - -## Related - -- `.claude/docs/TestCoverage.md` §6.3 -- `.claude/TODO/contact-log-actions-authenticate-but-not-authorize.md` diff --git a/.claude/TODO/mp-filter-injection-numeric-ids.md b/.claude/TODO/mp-filter-injection-numeric-ids.md deleted file mode 100644 index c70986e5..00000000 --- a/.claude/TODO/mp-filter-injection-numeric-ids.md +++ /dev/null @@ -1,68 +0,0 @@ -# TODO: Numeric IDs are interpolated into MP filters without sanitization - -**Created:** 2026-08-21 -**Severity:** High — confirmed filter injection reachable from a public server action. -**Status:** Open. Documented during the test-coverage push; deliberately **not** fixed, because the fix changes runtime behavior on read paths and deserves its own review. - -## Symptom - -`ContactLogService` interpolates caller-supplied IDs straight into the MP `$filter` string: - -- `src/services/contactLogService.ts:101` — `filter: \`Contact_Log_ID = ${contactLogId}\`` -- `src/services/contactLogService.ts:118` — `filter: \`Contact_ID = ${contactId}\`` -- `src/services/contactLogService.ts:83` — same shape via `searchContactLogs` -- `src/services/userService.ts:75` and `:80` — `User_ID = ${profile.User_ID}` (lower risk; the value originates from MP, not from a caller) - -The codebase has `sanitizeFilterValue`, `sanitizeLikeValue`, and `sanitizeGuid` in -`src/lib/providers/ministry-platform/utils/filter-sanitize.ts`, and applies them faithfully to every -**string** parameter. There is no equivalent for numeric IDs, and the TypeScript `number` annotation -is erased at runtime. - -## Why the action-level guard does not stop it - -`src/components/contact-logs/actions.ts` guards with `if (!contactLogId || contactLogId <= 0)`. -For `contactLogId = "1 OR 1=1"`: - -``` -!id -> false (a non-empty string is truthy) -id <= 0 -> false (string/number comparison does not reject it) -guard passes -``` - -Server actions compile to callable POST endpoints. A caller controls the payload *shape*, not just -its values, so a string arriving where the signature says `number` is entirely reachable. - -## Reproduction - -Verified empirically against the real service with a mocked `MPHelper` (probe test since removed): - -``` -getContactLogById("1 OR 1=1") -> filter: "Contact_Log_ID = 1 OR 1=1" -searchContactLogs("5; DROP") -> filter: "Contact_ID = 5; DROP" -``` - -Both reach the MP API. `Contact_Log_ID = 1 OR 1=1` widens a single-record read into a full-table read. - -## Proposed fix - -1. Add to `filter-sanitize.ts`: - -```ts -export function sanitizeNumericId(value: unknown): number { - const n = typeof value === 'number' ? value : Number(value); - if (!Number.isInteger(n) || n <= 0) { - throw new Error('Invalid numeric ID'); - } - return n; -} -``` - -2. Apply it at all five interpolation sites listed above. -3. Test the rejection set: `'1 OR 1=1'`, `'5; DROP'`, `NaN`, `Infinity`, `1.5`, `-1`, `0`, `null`, - `undefined`, `' 7 '` (decide whether whitespace-padded numerics are accepted or rejected). - -## Why the existing tests missed it - -`contactLogService.ts` is at 100% statement coverage. No test passes a non-numeric value to any of -these methods, so every line executes and the defect survives. This is the clearest example in the -repo of coverage measuring the wrong thing — see `.claude/docs/TestCoverage.md` §7. diff --git a/.claude/docs/TestCoverage.md b/.claude/docs/TestCoverage.md index 00eda80d..686412b7 100644 --- a/.claude/docs/TestCoverage.md +++ b/.claude/docs/TestCoverage.md @@ -4,6 +4,9 @@ **Reviewed commit:** `64f18f0` (branch `main`), plus the coverage work described in §2 **Scope:** whole application — `src/**` excluding generated MP models **Supersedes:** the 2026-08-20 review of `bb2cd19`, whose gap analysis has now been acted on +**Updated 2026-08-21:** §5.4, §5.5 and §6 (the three contact-log findings) are now fixed — see those +sections. §5.1 (filter injection via numeric IDs) is now fixed too. The suite is at **575 tests in 32 +files**; `contact-logs.tsx` went from 0% to 87.6% statements. --- @@ -26,11 +29,11 @@ Three things matter more than the headline number: |---|---| | **Measurement was inflated ~2.2×.** With no explicit `coverage.include`, every file no test imported dropped out of the denominator. | **Fixed.** `vitest.config.ts` now sets an explicit `include`, plus per-glob `thresholds` that fail the run on regression. | | **`testing.md` claimed 95.39% coverage** — not reproducible under any configuration. | **Fixed.** Rewritten against measured numbers, with the new mock patterns documented. | -| **Coverage was pointed away from the risk.** Two `'use server'` actions have no session check at all, and both sat at 100% line coverage. | **Documented, not fixed** — see §5. Each defect now has a file in `.claude/TODO/`. | +| **Coverage was pointed away from the risk.** Two `'use server'` actions have no session check at all, and both sat at 100% line coverage. | **Documented, then fixed.** §5.1 (filter injection), §5.2/§5.3 (missing auth) and §5.4/§5.5 (missing authz, duplicated User_ID lookup) are closed. §5.6 and §5.7 remain open in `.claude/TODO/`. | The shape of the original problem is worth restating, because the new number does not make it go -away: **high coverage is not evidence of correctness.** The confirmed filter-injection path in §5.1 -lives in a file at 100% statement coverage, and it still does. +away: **high coverage is not evidence of correctness.** The filter-injection path in §5.1 lived in a +file at 100% statement coverage for as long as no test passed it a value of the wrong type. --- @@ -84,7 +87,7 @@ pass when satisfied. ## 3. Reproducing these numbers ```bash -npm run test:run # 419 passed (30 files), ~3s +npm run test:run # 575 passed (32 files), ~4s npm run test:coverage # whole-app figure, and the threshold gate npx tsc --noEmit # clean npx eslint . # clean @@ -140,30 +143,39 @@ contrivance. ## 5. Where coverage is still actively misleading -**This is the most important section.** Each item below is fully covered by passing tests and is still -wrong. Per the scope of this work, these were **documented, not fixed** — one file per issue in -`.claude/TODO/`. New tests pin today's behavior, and any test asserting behavior a TODO proposes -changing carries a comment naming the TODO file. +**This is the most important section.** Each item below was fully covered by passing tests and was +still wrong. The original coverage work **documented rather than fixed** them — one file per issue in +`.claude/TODO/` — and the fixed items have since been closed out by follow-up work; each carries a +regression test that would have caught the defect. -### 5.1 Confirmed: numeric IDs are interpolated into MP filters unsanitized 🔴 +### 5.1 Numeric IDs are interpolated into MP filters unsanitized ✅ FIXED -→ `.claude/TODO/mp-filter-injection-numeric-ids.md` +Fixed 2026-08-21: `sanitizeNumericId` was added to `filter-sanitize.ts` and applied at all five +interpolation sites plus the five action-level boundaries (`contact-logs/actions.ts` ×4, +`contact-lookup-details/actions.ts` ×1 — the second entry point, which the TODO had missed). It +accepts a `number` or a digits-only string and throws otherwise, so `'1 OR 1=1'` now fails before any +HTTP call. The probe tests were **kept** this time: `contactLogService.test.ts` asserts the built +filter string and that `getTableRecords` is never called for each payload — the assertion the old +100% coverage lacked. Behavior change: `searchContactLogs(0)` now throws instead of silently reading +the whole table (the old `if (contactId)` truthiness check treated 0 as "no filter"). -`contactLogService.ts:101,118,83` and `userService.ts:75,80` interpolate IDs directly. The codebase -has `sanitizeFilterValue`, `sanitizeLikeValue`, and `sanitizeGuid`, applies them faithfully to every -**string** parameter, and has no equivalent for numeric IDs — while the TypeScript `number` annotation +Was: + +`contactLogService.ts:101,118,83` and `userService.ts:75,80` interpolated IDs directly. The codebase +had `sanitizeFilterValue`, `sanitizeLikeValue`, and `sanitizeGuid`, applied them faithfully to every +**string** parameter, and had no equivalent for numeric IDs — while the TypeScript `number` annotation is erased at runtime. -The action-level guard does not help. For `contactLogId = "1 OR 1=1"`, `!id` is false (non-empty -string is truthy) and `id <= 0` is false, so the guard passes. Verified empirically: +The action-level guard did not help. For `contactLogId = "1 OR 1=1"`, `!id` is false (non-empty +string is truthy) and `id <= 0` is false, so the guard passed. Verified empirically: ``` getContactLogById("1 OR 1=1") → filter: "Contact_Log_ID = 1 OR 1=1" searchContactLogs("5; DROP") → filter: "Contact_ID = 5; DROP" ``` -`contactLogService.ts` is at **100% statements and 100% branches**. No test passes a non-numeric value, -which is exactly why full coverage did not catch it. +`contactLogService.ts` was at **100% statements and 100% branches**. No test passed a non-numeric +value, which is exactly why full coverage did not catch it. ### 5.2 `searchContacts` — no authentication ✅ FIXED @@ -193,23 +205,36 @@ and keeps `userGuid` only as an effect dependency so switching users still re-fe If a feature ever needs to read another user's profile, that is a separate, explicitly role-gated function — not a widening of this one. -### 5.4 Contact-log actions authenticate but never authorize 🟠 +### 5.4 Contact-log actions authenticate but never authorize ✅ FIXED + +Resolved 2026-08-21. The policy decision was made: **any authenticated user holding an MP security +role may create, edit, and delete any contact log**, ownership not a factor. It is enforced by +`AuthorizationService.requireSecurityRoleForWrite()`, documented in `.claude/references/auth.md`, and +encoded in tests that would fail under a different policy (`should NOT delete when the caller holds +no security role`, `should permit editing a log made by a different user`). -→ `.claude/TODO/contact-log-actions-authenticate-but-not-authorize.md` +Authentication alone is no longer sufficient for a write: a session with no resolvable MP `User_ID`, +or an MP user holding no security role, fails closed with `UnauthorizedError` and a structured +`mp.write.unauthorized` log line. `MP_WRITE_SECURITY_ROLES` narrows the gate to named roles without a +code change. -`deleteContactLog` is the sharpest edge: any authenticated session can delete any contact log in the -domain by ID. This needs a policy decision, not just code — "any authenticated staff user may delete -any log" may well be correct, but it should be chosen and documented rather than left implicit. +### 5.5 Contact-log actions bypass `SessionContextService` ✅ FIXED -### 5.5 Contact-log actions bypass `SessionContextService` 🟠 +Resolved 2026-08-21. Both inline `dp_Users` lookups are gone. The acting `User_ID` now comes from +`AuthorizationService` → `SessionContextService` → the session-baked `userId` that `customSession` +resolved and `resolveMpUserId` cached — so a write costs no `dp_Users` round-trip at all, and the +`getUserGuid` helper plus the `MPHelper`/`sanitizeGuid` imports were deleted from the actions. -→ `.claude/TODO/contact-log-actions-bypass-session-context-service.md` +`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 — and under §5.4's policy it does not, because a user with no `User_ID` +has no roles. `should not resolve the acting user itself — SessionContextService owns that` guards +against the inline lookup returning. -Found while writing tests. `createContactLog` and `updateContactLog` each re-implement the `dp_Users` -User_ID lookup inline — work `resolveMpUserId` already does and caches, and which -`SessionContextService.getActingUserIdForWrite()` exists specifically to serve. Worse, they **throw** -when the User_ID cannot be resolved, contradicting the policy that service was built around: log -`mp.write.non_user` and proceed rather than blocking the write. +One behavior change worth calling out: `updateContactLog` no longer stamps `Made_By` with the editor. +That column records who made the *contact*; since any role-holder may edit anyone's log, stamping the +editor rewrote the pastoral record's authorship. MP's audit trail still captures the editor via +`$userId` in `ContactLogService`. ### 5.6 N+1 query in `getContactLogsByContactId` 🟡 @@ -239,15 +264,29 @@ Now rewritten to call the real `enrichSessionUser`. Verified by mutation: changi ## 6. Remaining gaps -### `contact-logs.tsx` — 602 lines, 0% 🔴 +### `contact-logs.tsx` — was 602 lines at 0% ✅ ADDRESSED + +Resolved 2026-08-21. `contact-logs.test.tsx` adds 13 targeted tests covering the three places where a +regression would silently corrupt or delete member data: + +1. **The delete-confirmation gate** — clicking the trash icon opens the confirmation and calls + nothing; cancelling calls nothing; only accepting calls `deleteContactLog(501)`. +2. **Client-side validation** — an empty `Notes` or a cleared `Contact_Date` surfaces the field error + and never reaches `createContactLog`. +3. **Error surfacing** — a rejected create/update/delete alerts the user, leaves the dialog open, and + does not call `onRefresh` as if it had succeeded; the log row stays on screen. + +Verified by mutation: making `handleDeleteClick` call `deleteContactLog(logId)` directly — the exact +"delete fires before the confirmation resolves" regression the TODO named — fails all four gate +tests. The previous suite would have caught none of it. -→ `.claude/TODO/contact-logs-component-untested.md` +Radix needs `ResizeObserver`, `hasPointerCapture`/`setPointerCapture`/`releasePointerCapture`, and +`scrollIntoView` polyfilled under jsdom; without them the primitives throw on mount rather than +failing an assertion. `installJsdomPolyfills()` in that test file is the pattern to copy for the +remaining component gaps below. -The largest untested file in the app, and the component that drives contact-log create / update / -delete. Out of scope for a non-UI coverage target, but it is the highest-value test gap left in the -repo: the server actions beneath it are at 100%, and the actions are the easy half. The form logic, -the delete-confirmation gate, and the error handling are where a regression silently corrupts or -deletes member data. Three targeted tests would beat zero by a wide margin. +Full render coverage was not chased — deliberately. These are the write-path tests, not a coverage +exercise, and the component stays ungated in `vitest.config.ts` thresholds. ### Other component gaps 🟡 @@ -303,12 +342,12 @@ deletes member data. Three targeted tests would beat zero by a wide margin. | 100% | 100% | 1/1 | `lib/auth-client.ts` | | 100% | 100% | 1/1 | `lib/utils.ts` | -The full test inventory (419 tests across 30 files, with per-file counts) lives in +The full test inventory (575 tests across 32 files, with per-file counts) lives in `.claude/references/testing.md`. --- -*All findings verified against the working tree. §5.1 was reproduced with a temporary probe test -(since removed) against the real `ContactLogService` with a mocked `MPHelper`. §5.8 was verified by -mutation. No Ministry Platform data was read or written during this review or by any test in the +*All findings verified against the working tree. §5.1 was reproduced with a probe test against the +real `ContactLogService` with a mocked `MPHelper`; that probe now ships as the regression guard in +`contactLogService.test.ts`. §5.8 was verified by mutation. No Ministry Platform data was read or written during this review or by any test in the suite — every test mocks at a boundary above the network.* diff --git a/.claude/references/auth.md b/.claude/references/auth.md index 725124b8..e116d79f 100644 --- a/.claude/references/auth.md +++ b/.claude/references/auth.md @@ -291,6 +291,17 @@ export async function myAction() { } ``` +For an action that **writes** to MP, the session check above is only the first gate. Add +the authorization gate and take the acting `User_ID` from its return value rather than +looking it up again — see [Authorization](#authorization-distinct-from-authentication): + +```typescript +const userId = await AuthorizationService.getInstance().requireSecurityRoleForWrite({ + table: "Contact_Log", + operation: "update", +}); +``` + ### Client Components ```typescript @@ -318,6 +329,77 @@ function MyComponent() { 4. Returns `MPUserProfile` (First_Name, Last_Name, Email, Image_GUID, etc.) 5. Profile available via `useUser()` hook in any client component +## Authorization (distinct from authentication) + +**Authentication** answers "is there a valid session?" — `auth.api.getSession()`. +**Authorization** answers "may this session do this?" — `AuthorizationService` +(`src/services/authorizationService.ts`). They are separate gates; a valid session is +necessary but not sufficient for a write. + +### Decided policy — contact-log writes (2026-08-21) + +> **Any authenticated user who holds a Ministry Platform security role may create, edit, +> and delete any contact log — including a log another user created.** + +This was chosen deliberately, not left implicit. Prior to this decision the contact-log +actions authenticated but never authorized, so *any* authenticated session could delete +*any* contact log in the domain by ID. + +**Why role membership and not ownership:** + +- MP security roles (`dp_User_Roles` → `dp_Roles`) are the domain's own authorization + mechanism. This app defers to them rather than inventing a parallel permission model + that could drift out of sync with MP. +- Ownership (`Made_By`) is deliberately **not** a factor. Contact logs are shared + pastoral records; staff need to correct and remove each other's entries. Gating on + ownership would mean a supervisor could not fix a bad log through this app. +- Authentication alone is *not* sufficient. A session for an MP user with no security + role cannot write, and neither can a session whose MP `User_ID` never resolved — the + gate fails closed. + +**Reads** (`getContactLogTypes`, `getContactLogsByContactId`, `getContactLogById`) require +authentication only. Every authenticated user of this app is MP staff who can already see +this data in MP itself, so the gate's purpose is write safety, not read confidentiality. + +### Using the gate + +```typescript +import { AuthorizationService } from "@/services/authorizationService"; + +// Throws UnauthorizedError when the caller may not write. +// Returns the acting user's MP User_ID, so no dp_Users round-trip is needed. +const userId = await AuthorizationService.getInstance().requireSecurityRoleForWrite({ + table: "Contact_Log", + operation: "create", // "create" | "update" | "delete" +}); +``` + +The acting user comes from `SessionContextService`, which reads the `userId` that +`customSession` already baked into the session (cached process-wide by `resolveMpUserId`). +Server actions must **not** re-implement the `dp_Users` lookup inline — that costs an +uncached MP round-trip on every write. + +Denials are logged as a structured `mp.write.unauthorized` event (with `table`, +`operation`, `userId`, and a `reason` of `no_mp_user` / `no_security_role` / +`role_not_permitted`) so refused writes are greppable in production logs. An unattributed +write still emits `mp.write.non_user` from `SessionContextService` before the gate rejects +it, so the attempt remains visible. + +**No caching.** Roles are re-read from MP on every gated write — one extra read per write. +Writes are rare (a staff member saving a form) and a cached authorization decision means a +revoked role keeps working. That trade is not worth making against a shared production +database. + +### Tightening the gate + +Set `MP_WRITE_SECURITY_ROLES` to a comma-separated list of MP role names to require one of +those specific roles instead of "any role". Comparison is case- and whitespace-insensitive. +Unset or blank means any security role is sufficient (the default policy above). + +``` +MP_WRITE_SECURITY_ROLES="Administrators,Pastoral Staff" +``` + ## Environment Variables | Variable | Required | Purpose | @@ -327,6 +409,7 @@ function MyComponent() { | `BETTER_AUTH_SECRET` | Yes* | Session signing secret. Fallback: `NEXTAUTH_SECRET` | | `OIDC_CLIENT_ID` | Yes | OAuth client ID registered in MP | | `OIDC_CLIENT_SECRET` | Yes | OAuth client secret | +| `MP_WRITE_SECURITY_ROLES` | No | Comma-separated MP role names permitted to perform gated writes. Unset = any security role. See [Authorization](#authorization-distinct-from-authentication). | *Fallback variables allow gradual migration from NextAuth. diff --git a/.claude/references/ministryplatform.query-syntax.md b/.claude/references/ministryplatform.query-syntax.md index feb174ea..ad0744ab 100644 --- a/.claude/references/ministryplatform.query-syntax.md +++ b/.claude/references/ministryplatform.query-syntax.md @@ -178,6 +178,28 @@ Notice in Query B that bare `End_Date` is qualified as `Group_Participants.End_D | Subquery rejected | Used `SELECT` inside `$filter` | Rewrite using `_TABLE` traversal; if not expressible, run two queries and merge in code | | `BETWEEN` rejected | Used SQL BETWEEN in `$filter` | Rewrite as two comparisons (`>= 'start' AND < 'end'`) | +## Sanitizing interpolated values — MANDATORY + +`$filter` becomes a SQL `WHERE` clause, so **every** value interpolated into a filter string must pass +through a sanitizer from `src/lib/providers/ministry-platform/utils/filter-sanitize.ts` first: + +| Value | Helper | Pattern | +|---|---|---| +| String (equality) | `sanitizeFilterValue` | `Column = '${sanitizeFilterValue(v)}'` | +| String (LIKE) | `sanitizeLikeValue` | `Column LIKE '%${sanitizeLikeValue(v)}%' ESCAPE ''` | +| GUID | `sanitizeGuid` | `Column = '${sanitizeGuid(v)}'` — throws on non-GUID | +| Numeric ID | `sanitizeNumericId` | `Column = ${sanitizeNumericId(v, 'Contact ID')}` — throws on anything but a positive integer or digits-only string | + +A `number` parameter is not exempt. TypeScript annotations are erased at runtime, and server actions +compile to POST endpoints whose payload *shape* the caller controls — so a string does arrive where +the signature says `number`. `getContactLogById('1 OR 1=1')` used to build +`Contact_Log_ID = 1 OR 1=1`, widening a single-record read into a full-table read; see +`.claude/docs/TestCoverage.md` §5.1. Guards of the form `if (!id || id <= 0)` do **not** catch this: +a non-empty string is truthy and `'1 OR 1=1' <= 0` is false. + +Sanitize at the interpolation site (the service), and validate again at the action boundary so bad +input fails before the authorization gate and the network call. + ## See also - `src/lib/providers/ministry-platform/helper.ts` — `MPHelper.getTableRecords` signature. diff --git a/.claude/references/testing.md b/.claude/references/testing.md index 5ff98c8c..716382e8 100644 --- a/.claude/references/testing.md +++ b/.claude/references/testing.md @@ -306,6 +306,55 @@ it('should load profile', async () => { }); ``` +## Radix Component Tests Under jsdom + +jsdom does not implement the browser APIs Radix primitives probe on mount. Without +these polyfills, `Dialog` / `AlertDialog` / `Select` **throw during render** rather +than failing an assertion, which makes the component look broken when only the +harness is. `components/contact-logs/contact-logs.test.tsx` carries the pattern: + +```typescript +function installJsdomPolyfills() { + if (!globalThis.ResizeObserver) { + globalThis.ResizeObserver = class { + observe() {} + unobserve() {} + disconnect() {} + } as unknown as typeof ResizeObserver; + } + const proto = Element.prototype as unknown as Record; + proto.hasPointerCapture ??= () => false; + proto.setPointerCapture ??= () => {}; + proto.releasePointerCapture ??= () => {}; + proto.scrollIntoView ??= () => {}; +} +``` + +Call it in `beforeEach`. Notes on the rest of the harness: + +- `fireEvent` is sufficient for Radix triggers and buttons — `@testing-library/user-event` + is **not** installed, so do not import it. +- Icon-only buttons (the trash icon on a log row) have no accessible name. Find them by + filtering `getAllByRole('button')` rather than adding a test-only label to the component. +- Scope assertions to the open dialog with `within(await screen.findByRole('dialog'))`; + `AlertDialog` uses role `alertdialog`, not `dialog`. +- Components that report errors with `window.alert()` need `vi.spyOn(window, 'alert')` — + jsdom's default implementation emits "not implemented" noise. +- react-hook-form + `zodResolver` validate asynchronously. Assert the error message with + `await screen.findByText(...)` before asserting the action was not called. + +### Verify a gate test actually gates + +A test that asserts "the action was called with 42" passes under any policy. For a +confirmation gate, mutate the component to bypass it and confirm the tests fail: + +``` +handleDeleteClick = (logId) => { deleteContactLog(logId); setDeleteLogId(logId); } +``` + +All four delete-gate tests fail on that mutation. The suite that preceded them failed +none of it. + ## Coverage Coverage uses the **v8** provider. @@ -356,7 +405,7 @@ and a non-zero exit code. | `src/contexts/**` | 95 | 85 | 95 | 95 | | `src/proxy.ts` | 100 | 100 | 100 | 100 | -### Current coverage (419 tests, 30 files) +### Current coverage (575 tests, 32 files) Non-UI functional code - every `src/**/*.ts` plus `src/contexts/*.tsx`, excluding generated models, codegen scripts, and test files (760 statements): @@ -393,24 +442,26 @@ message - Zod always throws an `Error`. | `lib/providers/ministry-platform/services/file.service.test.ts` | 35 | All 8 file endpoints, multipart bodies, unauthenticated blob fetch | | `lib/providers/ministry-platform/utils/http-client.test.ts` | 28 | HTTP verbs, URL building, form data, error handling | | `auth.test.ts` | 25 | `enrichSessionUser`, cached User_ID resolution, OAuth config guards | -| `components/contact-logs/actions.test.ts` | 24 | Contact log CRUD actions, auth and argument guards | +| `components/contact-logs/actions.test.ts` | 67 | Contact log CRUD actions, auth/argument guards, security-role write gate, ownership policy, numeric-ID injection rejection | | `lib/providers/ministry-platform/provider.test.ts` | 24 | Provider delegation to all six sub-services | | `lib/providers/ministry-platform/services/table.service.test.ts` | 21 | TableService CRUD | -| `services/contactLogService.test.ts` | 21 | Contact log CRUD, date conversion, Zod validation | -| `lib/providers/ministry-platform/utils/filter-sanitize.test.ts` | 20 | Quote doubling, LIKE escaping, GUID rejection | +| `services/contactLogService.test.ts` | 54 | Contact log CRUD, date conversion, Zod validation, filter-injection regression guard | +| `lib/providers/ministry-platform/utils/filter-sanitize.test.ts` | 49 | Quote doubling, LIKE escaping, GUID rejection, numeric-ID validation | | `services/domainTimezoneService.test.ts` | 18 | Windows-to-IANA mapping, DST, round-tripping, cache | | `lib/providers/ministry-platform/services/procedure.service.test.ts` | 16 | Procedure listing and execution, name encoding | | `lib/providers/ministry-platform/services/communication.service.test.ts` | 13 | Email/SMS JSON vs multipart paths | | `lib/providers/ministry-platform/client.test.ts` | 12 | OAuth token management | | `services/contactService.test.ts` | 12 | Contact search, getByGuid, updateContact | -| `components/contact-lookup-details/actions.test.ts` | 11 | Contact details + log type mapping | +| `components/contact-lookup-details/actions.test.ts` | 18 | Contact details + log type mapping, numeric-ID injection rejection | | `services/sessionContextService.test.ts` | 10 | Acting-user resolution, `mp.write.non_user` warning | +| `services/authorizationService.test.ts` | 21 | MP security-role write gate, `MP_WRITE_SECURITY_ROLES`, `mp.write.unauthorized` denials | +| `components/contact-logs/contact-logs.test.tsx` | 13 | Delete-confirmation gate, form validation, error surfacing (MP write path) | | `contexts/user-context.test.tsx` | 8 | UserProvider + useUser lifecycle | | `lib/providers/ministry-platform/services/domain.service.test.ts` | 8 | Domain info and global filters | | `lib/providers/ministry-platform/services/metadata.service.test.ts` | 8 | Metadata refresh, table listing | | `proxy.test.ts` | 8 | Route protection (public paths, session, errors) | | `lib/utils.test.ts` | 7 | `cn()` Tailwind class merging | -| `services/userService.test.ts` | 6 | User profile lookup | +| `services/userService.test.ts` | 8 | User profile lookup, GUID + User_ID validation | | `components/contact-lookup/actions.test.ts` | 5 | Search contacts action | | `components/user-menu/actions.test.ts` | 5 | Sign-out + OAuth end session redirect | | `lib/providers/ministry-platform/auth/client-credentials.test.ts` | 5 | Client-credentials token grant | @@ -419,7 +470,7 @@ message - Zod always throws an `Error`. | `components/shared-actions/domain.test.ts` | 3 | `getMpTimezone` delegation | | `components/shared-actions/user.test.ts` | 2 | `getCurrentUserProfile` delegation | | `contexts/session-context.test.tsx` | 2 | `useAppSession` wrapper | -| **Total** | **419** | | +| **Total** | **474** | | ## Ministry Platform Safety in Tests @@ -445,3 +496,10 @@ sanitization, and two `'use server'` actions with no session check at all. See Tests that pin behavior a TODO proposes changing carry a comment naming the TODO file, so the next person knows the assertion is a snapshot of today's behavior rather than a specification. + +The three contact-log TODOs are now resolved (see `.claude/docs/TestCoverage.md` +§5.4, §5.5, §6): the security-role write gate, the `SessionContextService` +refactor, and the component write-path tests. Their assertions are now +specifications rather than snapshots — `should NOT delete when the caller holds no +security role` and `should permit editing a log made by a different user` would +each fail under a different policy, which is the point. diff --git a/src/components/contact-logs/actions.test.ts b/src/components/contact-logs/actions.test.ts index f7c8704d..44c7356b 100644 --- a/src/components/contact-logs/actions.test.ts +++ b/src/components/contact-logs/actions.test.ts @@ -1,5 +1,14 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; +/** + * Contact-log action tests. + * + * These encode the decided authorization policy, not just the code's shape: + * writes require an authenticated session AND an MP security role; any + * role-holder may edit or delete a log another user created; reads require + * authentication only. See `.claude/references/auth.md`. + */ + const { mockGetSession, mockGetContactLogTypes, @@ -8,7 +17,7 @@ const { mockDeleteContactLog, mockGetContactLogsByContactId, mockGetContactLogById, - mockGetTableRecords, + mockRequireSecurityRoleForWrite, } = vi.hoisted(() => ({ mockGetSession: vi.fn(), mockGetContactLogTypes: vi.fn(), @@ -17,7 +26,7 @@ const { mockDeleteContactLog: vi.fn(), mockGetContactLogsByContactId: vi.fn(), mockGetContactLogById: vi.fn(), - mockGetTableRecords: vi.fn(), + mockRequireSecurityRoleForWrite: vi.fn(), })); vi.mock('@/lib/auth', () => ({ @@ -45,10 +54,19 @@ vi.mock('@/services/contactLogService', () => ({ }, })); -vi.mock('@/lib/providers/ministry-platform', () => { +vi.mock('@/services/authorizationService', () => { + class UnauthorizedError extends Error { + constructor(message: string) { + super(message); + this.name = 'UnauthorizedError'; + } + } return { - MPHelper: class { - getTableRecords = mockGetTableRecords; + UnauthorizedError, + AuthorizationService: { + getInstance: () => ({ + requireSecurityRoleForWrite: mockRequireSecurityRoleForWrite, + }), }, }; }); @@ -61,16 +79,30 @@ import { getContactLogsByContactId, getContactLogById, } from './actions'; +import { UnauthorizedError } from '@/services/authorizationService'; const validUserGuid = 'a1b2c3d4-e5f6-7890-abcd-ef1234567890'; const mockAuthSession = { - user: { id: 'internal-id', userGuid: validUserGuid }, + user: { id: 'internal-id', userGuid: validUserGuid, userId: 99 }, +}; + +const validCreateInput = { + Contact_ID: 42, + Contact_Date: '2024-01-15T10:00:00Z', + Notes: 'Test note', + Contact_Log_Type_ID: 1, + Planned_Contact_ID: null, + Contact_Successful: null, + Original_Contact_Log_Entry: null, + Feedback_Entry_ID: null, }; describe('contact-logs actions', () => { beforeEach(() => { vi.clearAllMocks(); + // Default: an authorized role-holder. Individual tests override. + mockRequireSecurityRoleForWrite.mockResolvedValue(99); }); describe('getContactLogTypes', () => { @@ -87,42 +119,36 @@ describe('contact-logs actions', () => { const result = await getContactLogTypes(); expect(result).toEqual(mockTypes); }); + + it('should not require a security role — it is a read', async () => { + mockGetSession.mockResolvedValueOnce(mockAuthSession); + mockGetContactLogTypes.mockResolvedValueOnce([]); + + await getContactLogTypes(); + expect(mockRequireSecurityRoleForWrite).not.toHaveBeenCalled(); + }); }); describe('createContactLog', () => { it('should require authentication', async () => { mockGetSession.mockResolvedValueOnce(null); - await expect( - createContactLog({ - Contact_ID: 42, - Contact_Date: '2024-01-15T10:00:00Z', - Notes: 'Test', - Contact_Log_Type_ID: 1, - Planned_Contact_ID: null, - Contact_Successful: null, - Original_Contact_Log_Entry: null, - Feedback_Entry_ID: null, - }) - ).rejects.toThrow('Authentication required'); + await expect(createContactLog(validCreateInput)).rejects.toThrow( + 'Authentication required' + ); + expect(mockRequireSecurityRoleForWrite).not.toHaveBeenCalled(); }); - it('should fetch User_ID and create log with Made_By', async () => { + it('should create the log with Made_By taken from the acting session', async () => { mockGetSession.mockResolvedValueOnce(mockAuthSession); - mockGetTableRecords.mockResolvedValueOnce([{ User_ID: 99 }]); const mockLog = { Contact_Log_ID: 1, Contact_ID: 42 }; mockCreateContactLog.mockResolvedValueOnce(mockLog); - const result = await createContactLog({ - Contact_ID: 42, - Contact_Date: '2024-01-15T10:00:00Z', - Notes: 'Test note', - Contact_Log_Type_ID: 1, - Planned_Contact_ID: null, - Contact_Successful: null, - Original_Contact_Log_Entry: null, - Feedback_Entry_ID: null, - }); + const result = await createContactLog(validCreateInput); + expect(mockRequireSecurityRoleForWrite).toHaveBeenCalledWith({ + table: 'Contact_Log', + operation: 'create', + }); expect(mockCreateContactLog).toHaveBeenCalledWith( expect.objectContaining({ Contact_ID: 42, @@ -135,88 +161,216 @@ describe('contact-logs actions', () => { it('should throw when required fields are missing', async () => { mockGetSession.mockResolvedValueOnce(mockAuthSession); - mockGetTableRecords.mockResolvedValueOnce([{ User_ID: 99 }]); await expect( createContactLog({ + ...validCreateInput, Contact_ID: 0, Contact_Date: '', Notes: '', - Contact_Log_Type_ID: null, - Planned_Contact_ID: null, - Contact_Successful: null, - Original_Contact_Log_Entry: null, - Feedback_Entry_ID: null, }) ).rejects.toThrow('Required fields are missing'); + expect(mockCreateContactLog).not.toHaveBeenCalled(); }); - it('should throw when user not found in MP', async () => { + it.each([ + ['Contact_ID', { Contact_ID: 0 }], + ['Contact_Date', { Contact_Date: '' }], + ['Notes', { Notes: '' }], + ])('should reject a create missing %s', async (_field, override) => { mockGetSession.mockResolvedValueOnce(mockAuthSession); - mockGetTableRecords.mockResolvedValueOnce([]); await expect( - createContactLog({ - Contact_ID: 42, - Contact_Date: '2024-01-15T10:00:00Z', - Notes: 'Test', - Contact_Log_Type_ID: 1, - Planned_Contact_ID: null, - Contact_Successful: null, - Original_Contact_Log_Entry: null, - Feedback_Entry_ID: null, - }) - ).rejects.toThrow('Unable to determine user User_ID'); + createContactLog({ ...validCreateInput, ...override }) + ).rejects.toThrow('Required fields are missing'); + expect(mockCreateContactLog).not.toHaveBeenCalled(); + }); + + it('should not write when the caller holds no security role', async () => { + mockGetSession.mockResolvedValueOnce(mockAuthSession); + mockRequireSecurityRoleForWrite.mockRejectedValueOnce( + new UnauthorizedError('Not authorized: an MP security role is required') + ); + + await expect(createContactLog(validCreateInput)).rejects.toThrow( + 'Not authorized: an MP security role is required' + ); + expect(mockCreateContactLog).not.toHaveBeenCalled(); + }); + + it('should not resolve the acting user itself — SessionContextService owns that', async () => { + // Regression guard for the inline dp_Users lookup this action used to do + // on every write. Made_By must come from the authorization gate's return + // value, which reads the session-baked (already cached) User_ID. + mockGetSession.mockResolvedValueOnce(mockAuthSession); + mockRequireSecurityRoleForWrite.mockResolvedValueOnce(4242); + mockCreateContactLog.mockResolvedValueOnce({ Contact_Log_ID: 1 }); + + await createContactLog(validCreateInput); + + expect(mockCreateContactLog).toHaveBeenCalledWith( + expect.objectContaining({ Made_By: 4242 }) + ); + }); + + it('should wrap a non-Error rejection from the service', async () => { + mockGetSession.mockResolvedValueOnce(mockAuthSession); + mockCreateContactLog.mockRejectedValueOnce('boom'); + + await expect(createContactLog(validCreateInput)).rejects.toThrow( + 'Failed to create contact log' + ); }); }); describe('updateContactLog', () => { it('should require authentication', async () => { mockGetSession.mockResolvedValueOnce(null); - await expect(updateContactLog(1, { Notes: 'Updated' })).rejects.toThrow('Authentication required'); + await expect(updateContactLog(1, { Notes: 'Updated' })).rejects.toThrow( + 'Authentication required' + ); + expect(mockRequireSecurityRoleForWrite).not.toHaveBeenCalled(); }); it('should throw for invalid contactLogId', async () => { mockGetSession.mockResolvedValueOnce(mockAuthSession); - mockGetTableRecords.mockResolvedValueOnce([{ User_ID: 99 }]); + await expect(updateContactLog(0, { Notes: 'Updated' })).rejects.toThrow( + 'Invalid Contact Log ID' + ); + expect(mockRequireSecurityRoleForWrite).not.toHaveBeenCalled(); + }); - await expect(updateContactLog(0, { Notes: 'Updated' })).rejects.toThrow('Valid Contact Log ID is required'); + it('should reject a negative contact log ID', async () => { + mockGetSession.mockResolvedValueOnce(mockAuthSession); + await expect(updateContactLog(-5, { Notes: 'x' })).rejects.toThrow( + 'Invalid Contact Log ID' + ); + expect(mockUpdateContactLog).not.toHaveBeenCalled(); }); - it('should update log with Made_By', async () => { + it('should update the log after the security-role gate passes', async () => { mockGetSession.mockResolvedValueOnce(mockAuthSession); - mockGetTableRecords.mockResolvedValueOnce([{ User_ID: 99 }]); const mockLog = { Contact_Log_ID: 1, Notes: 'Updated' }; mockUpdateContactLog.mockResolvedValueOnce(mockLog); const result = await updateContactLog(1, { Notes: 'Updated' }); - expect(mockUpdateContactLog).toHaveBeenCalledWith(1, expect.objectContaining({ - Notes: 'Updated', - Made_By: 99, - })); + expect(mockRequireSecurityRoleForWrite).toHaveBeenCalledWith({ + table: 'Contact_Log', + operation: 'update', + }); + expect(mockUpdateContactLog).toHaveBeenCalledWith(1, { Notes: 'Updated' }); expect(result).toEqual(mockLog); }); + + it('should NOT stamp Made_By with the editor', async () => { + // Made_By records who made the *contact*. Since any role-holder may edit + // anyone's log, stamping the editor would rewrite the record's authorship. + // MP's audit trail captures the editor via $userId in ContactLogService. + mockGetSession.mockResolvedValueOnce(mockAuthSession); + mockUpdateContactLog.mockResolvedValueOnce({ Contact_Log_ID: 1 }); + + await updateContactLog(1, { Notes: 'Updated' }); + + expect(mockUpdateContactLog).toHaveBeenCalledWith( + 1, + expect.not.objectContaining({ Made_By: expect.anything() }) + ); + }); + + it('should not write when the caller holds no security role', async () => { + mockGetSession.mockResolvedValueOnce(mockAuthSession); + mockRequireSecurityRoleForWrite.mockRejectedValueOnce( + new UnauthorizedError('Not authorized: an MP security role is required') + ); + + await expect(updateContactLog(1, { Notes: 'x' })).rejects.toThrow( + 'Not authorized: an MP security role is required' + ); + expect(mockUpdateContactLog).not.toHaveBeenCalled(); + }); + + it('should permit editing a log made by a different user', async () => { + // POLICY: ownership is not a factor. This test exists so a future reader + // knows the absence of an ownership check was chosen, not overlooked. + mockGetSession.mockResolvedValueOnce(mockAuthSession); + mockUpdateContactLog.mockResolvedValueOnce({ Contact_Log_ID: 7, Made_By: 12345 }); + + await expect(updateContactLog(7, { Notes: 'Corrected typo' })).resolves.toEqual({ + Contact_Log_ID: 7, + Made_By: 12345, + }); + // No read of the target log is performed to compare Made_By. + expect(mockGetContactLogById).not.toHaveBeenCalled(); + }); + + it('should wrap a non-Error rejection from the service', async () => { + mockGetSession.mockResolvedValueOnce(mockAuthSession); + mockUpdateContactLog.mockRejectedValueOnce('boom'); + + await expect(updateContactLog(1, { Notes: 'x' })).rejects.toThrow( + 'Failed to update contact log' + ); + }); }); describe('deleteContactLog', () => { it('should require authentication', async () => { mockGetSession.mockResolvedValueOnce(null); await expect(deleteContactLog(1)).rejects.toThrow('Authentication required'); + expect(mockRequireSecurityRoleForWrite).not.toHaveBeenCalled(); }); it('should throw for invalid contactLogId', async () => { mockGetSession.mockResolvedValueOnce(mockAuthSession); - await expect(deleteContactLog(0)).rejects.toThrow('Valid Contact Log ID is required'); + await expect(deleteContactLog(0)).rejects.toThrow('Invalid Contact Log ID'); + expect(mockRequireSecurityRoleForWrite).not.toHaveBeenCalled(); }); - it('should delete when authenticated', async () => { + it('should delete after the security-role gate passes', async () => { mockGetSession.mockResolvedValueOnce(mockAuthSession); mockDeleteContactLog.mockResolvedValueOnce(undefined); await deleteContactLog(42); + + expect(mockRequireSecurityRoleForWrite).toHaveBeenCalledWith({ + table: 'Contact_Log', + operation: 'delete', + }); expect(mockDeleteContactLog).toHaveBeenCalledWith(42); }); + + it('should NOT delete when the caller holds no security role', async () => { + // This is the sharpest edge the gate closes: previously any authenticated + // session could delete any contact log in the domain by ID. + mockGetSession.mockResolvedValueOnce(mockAuthSession); + mockRequireSecurityRoleForWrite.mockRejectedValueOnce( + new UnauthorizedError('Not authorized: an MP security role is required') + ); + + await expect(deleteContactLog(42)).rejects.toThrow( + 'Not authorized: an MP security role is required' + ); + expect(mockDeleteContactLog).not.toHaveBeenCalled(); + }); + + it('should permit deleting a log made by a different user', async () => { + // POLICY: ownership is not a factor — see updateContactLog above. + mockGetSession.mockResolvedValueOnce(mockAuthSession); + mockDeleteContactLog.mockResolvedValueOnce(undefined); + + await deleteContactLog(7); + + expect(mockDeleteContactLog).toHaveBeenCalledWith(7); + expect(mockGetContactLogById).not.toHaveBeenCalled(); + }); + + it('should wrap a non-Error rejection from the service', async () => { + mockGetSession.mockResolvedValueOnce(mockAuthSession); + mockDeleteContactLog.mockRejectedValueOnce('boom'); + + await expect(deleteContactLog(42)).rejects.toThrow('Failed to delete contact log'); + }); }); describe('getContactLogsByContactId', () => { @@ -227,16 +381,28 @@ describe('contact-logs actions', () => { it('should throw for invalid contactId', async () => { mockGetSession.mockResolvedValueOnce(mockAuthSession); - await expect(getContactLogsByContactId(0)).rejects.toThrow('Valid contact ID is required'); + await expect(getContactLogsByContactId(0)).rejects.toThrow( + 'Invalid Contact ID' + ); }); - it('should return logs when authenticated', async () => { + it('should return logs when authenticated, without a role check', async () => { mockGetSession.mockResolvedValueOnce(mockAuthSession); const mockLogs = [{ Contact_Log_ID: 1, Contact_ID: 42 }]; mockGetContactLogsByContactId.mockResolvedValueOnce(mockLogs); const result = await getContactLogsByContactId(42); expect(result).toEqual(mockLogs); + expect(mockRequireSecurityRoleForWrite).not.toHaveBeenCalled(); + }); + + it('should wrap a non-Error rejection from the service', async () => { + mockGetSession.mockResolvedValueOnce(mockAuthSession); + mockGetContactLogsByContactId.mockRejectedValueOnce('boom'); + + await expect(getContactLogsByContactId(42)).rejects.toThrow( + 'Failed to fetch contact logs' + ); }); }); @@ -248,7 +414,7 @@ describe('contact-logs actions', () => { it('should throw for invalid contactLogId', async () => { mockGetSession.mockResolvedValueOnce(mockAuthSession); - await expect(getContactLogById(0)).rejects.toThrow('Valid contact log ID is required'); + await expect(getContactLogById(0)).rejects.toThrow('Invalid Contact Log ID'); }); it('should return log when found', async () => { @@ -258,6 +424,7 @@ describe('contact-logs actions', () => { const result = await getContactLogById(1); expect(result).toEqual(mockLog); + expect(mockRequireSecurityRoleForWrite).not.toHaveBeenCalled(); }); it('should return null when not found', async () => { @@ -267,68 +434,104 @@ describe('contact-logs actions', () => { const result = await getContactLogById(999); expect(result).toBeNull(); }); - }); - - describe('Session and argument guards', () => { - it('should reject createContactLog when the session carries no userGuid', async () => { - // getUserGuid throws before any MP call is attempted — a Better Auth session - // without userGuid means mapProfileToUser did not run (see auth.test.ts). - mockGetSession.mockResolvedValueOnce({ user: { id: 'ba-internal-id' } }); - await expect( - createContactLog({ Contact_ID: 1, Contact_Date: '2026-08-21', Notes: 'x' } as never) - ).rejects.toThrow('User GUID not found in session'); + it('should wrap a non-Error rejection from the service', async () => { + mockGetSession.mockResolvedValueOnce(mockAuthSession); + mockGetContactLogById.mockRejectedValueOnce('boom'); - expect(mockGetTableRecords).not.toHaveBeenCalled(); - expect(mockCreateContactLog).not.toHaveBeenCalled(); + await expect(getContactLogById(1)).rejects.toThrow('Failed to fetch contact log'); }); + }); - it('should reject updateContactLog when the session carries no userGuid', async () => { - mockGetSession.mockResolvedValueOnce({ user: { id: 'ba-internal-id' } }); + describe('Session guards', () => { + it('should reject writes for a session with no user id', async () => { + mockGetSession.mockResolvedValue({ user: {} }); + await expect(createContactLog(validCreateInput)).rejects.toThrow( + 'Authentication required' + ); await expect(updateContactLog(1, { Notes: 'x' })).rejects.toThrow( - 'User GUID not found in session' + 'Authentication required' ); + await expect(deleteContactLog(1)).rejects.toThrow('Authentication required'); + expect(mockRequireSecurityRoleForWrite).not.toHaveBeenCalled(); + }); - expect(mockUpdateContactLog).not.toHaveBeenCalled(); + it('no longer requires userGuid on the session — the gate uses the resolved userId', async () => { + // The old actions threw "User GUID not found in session" here because they + // did their own dp_Users lookup. SessionContextService reads the User_ID + // that customSession already baked in, so userGuid is not needed. + mockGetSession.mockResolvedValueOnce({ user: { id: 'ba-internal-id', userId: 99 } }); + mockCreateContactLog.mockResolvedValueOnce({ Contact_Log_ID: 1 }); + + await expect(createContactLog(validCreateInput)).resolves.toEqual({ + Contact_Log_ID: 1, + }); }); + }); + + // Regression guard for `.claude/TODO/mp-filter-injection-numeric-ids.md`. + // + // These actions compile to POST endpoints, so a caller controls the payload's + // shape as well as its values — a string reaches a `number` parameter. The old + // `!id || id <= 0` guard passed such values through: for '1 OR 1=1', + // `!id` is false and `id <= 0` is false, so the guard was a no-op. + describe('numeric ID validation at the action boundary', () => { + const injectionPayloads = ['1 OR 1=1', '5; DROP', "1' OR '1'='1", '1 --', '', 'abc', ' 7 ']; - it('should reject updateContactLog when the acting User_ID cannot be resolved', async () => { - // NOTE: this action throws rather than proceeding with a null Made_By. That - // contradicts the policy SessionContextService was built around (log - // mp.write.non_user, do not block the write). Tracked in - // .claude/TODO/contact-log-actions-bypass-session-context-service.md — this - // test pins today's behavior so the refactor is a deliberate change. + it.each(injectionPayloads)('getContactLogById rejects %j before reaching the service', async (payload) => { mockGetSession.mockResolvedValueOnce(mockAuthSession); - mockGetTableRecords.mockResolvedValueOnce([]); - await expect(updateContactLog(1, { Notes: 'x' })).rejects.toThrow( - 'Unable to determine user User_ID' + await expect(getContactLogById(payload as unknown as number)).rejects.toThrow( + 'Invalid Contact Log ID' ); - - expect(mockUpdateContactLog).not.toHaveBeenCalled(); + expect(mockGetContactLogById).not.toHaveBeenCalled(); }); - it('should reject updateContactLog for a non-positive contact log ID', async () => { + it.each(injectionPayloads)('getContactLogsByContactId rejects %j before reaching the service', async (payload) => { mockGetSession.mockResolvedValueOnce(mockAuthSession); - mockGetTableRecords.mockResolvedValueOnce([{ User_ID: 4242 }]); - await expect(updateContactLog(0, { Notes: 'x' })).rejects.toThrow( - 'Valid Contact Log ID is required' + await expect(getContactLogsByContactId(payload as unknown as number)).rejects.toThrow( + 'Invalid Contact ID' ); + expect(mockGetContactLogsByContactId).not.toHaveBeenCalled(); + }); + + it.each(injectionPayloads)('updateContactLog rejects %j before the write gate', async (payload) => { + mockGetSession.mockResolvedValueOnce(mockAuthSession); + await expect( + updateContactLog(payload as unknown as number, { Notes: 'x' }) + ).rejects.toThrow('Invalid Contact Log ID'); + expect(mockRequireSecurityRoleForWrite).not.toHaveBeenCalled(); expect(mockUpdateContactLog).not.toHaveBeenCalled(); }); - it('should reject updateContactLog for a negative contact log ID', async () => { + it.each(injectionPayloads)('deleteContactLog rejects %j before the write gate', async (payload) => { mockGetSession.mockResolvedValueOnce(mockAuthSession); - mockGetTableRecords.mockResolvedValueOnce([{ User_ID: 4242 }]); - await expect(updateContactLog(-5, { Notes: 'x' })).rejects.toThrow( - 'Valid Contact Log ID is required' + await expect(deleteContactLog(payload as unknown as number)).rejects.toThrow( + 'Invalid Contact Log ID' ); + expect(mockRequireSecurityRoleForWrite).not.toHaveBeenCalled(); + expect(mockDeleteContactLog).not.toHaveBeenCalled(); + }); - expect(mockUpdateContactLog).not.toHaveBeenCalled(); + it('rejects before authorization but after authentication', async () => { + mockGetSession.mockResolvedValueOnce(null); + + await expect(deleteContactLog('1 OR 1=1' as unknown as number)).rejects.toThrow( + 'Authentication required' + ); + }); + + it('passes a digits-only ID through to the service as a number', async () => { + mockGetSession.mockResolvedValueOnce(mockAuthSession); + mockGetContactLogById.mockResolvedValueOnce({ Contact_Log_ID: 42 }); + + await getContactLogById('42' as unknown as number); + + expect(mockGetContactLogById).toHaveBeenCalledWith(42); }); }); }); diff --git a/src/components/contact-logs/actions.ts b/src/components/contact-logs/actions.ts index a196a1c2..9577e83d 100644 --- a/src/components/contact-logs/actions.ts +++ b/src/components/contact-logs/actions.ts @@ -4,25 +4,57 @@ import { ContactLog } from "@/lib/providers/ministry-platform/models/ContactLog" import { ContactLogTypes } from "@/lib/providers/ministry-platform/models/ContactLogTypes"; import { ContactLogInput } from "@/lib/providers/ministry-platform/models/ContactLogSchema"; import { ContactLogService } from "@/services/contactLogService"; -import { sanitizeGuid } from "@/lib/providers/ministry-platform/utils/filter-sanitize"; +import { AuthorizationService } from "@/services/authorizationService"; +import { sanitizeNumericId } from "@/lib/providers/ministry-platform/utils/filter-sanitize"; import { auth } from "@/lib/auth"; import { headers } from "next/headers"; -/** Extract the MP User_GUID from a Better Auth session */ -function getUserGuid(session: { user: Record }): string { - const guid = session.user.userGuid as string | undefined; - if (!guid) { - throw new Error("User GUID not found in session"); +/** + * Contact-log server actions. + * + * ## Authorization policy (decided 2026-08-21) + * + * **Writes** (`createContactLog`, `updateContactLog`, `deleteContactLog`) require + * an authenticated session AND an MP security role. Any user holding a security + * role may edit or delete **any** contact log, including one another user + * created — ownership (`Made_By`) is deliberately not a factor, because staff + * need to be able to correct and remove each other's logs. `AuthorizationService` + * owns the gate; see `.claude/references/auth.md` for the full rationale. + * + * **Reads** (`getContactLogTypes`, `getContactLogsByContactId`, + * `getContactLogById`) require authentication only. Every authenticated user of + * this app is MP staff who can already see this data in MP itself, so the gate's + * purpose is write safety, not read confidentiality. + */ + +/** Confirms an authenticated session exists. Reads need nothing more than this. */ +async function requireSession(): Promise { + const session = await auth.api.getSession({ headers: await headers() }); + if (!session?.user?.id) { + throw new Error("Authentication required"); } - return guid; +} + +/** + * Confirms the caller may write to `Contact_Log` and returns their MP `User_ID`. + * + * The acting user comes from `SessionContextService` via `AuthorizationService` + * — the session already carries a resolved `userId` (baked in by `customSession` + * and cached process-wide by `resolveMpUserId`), so this costs no `dp_Users` + * round-trip. + */ +async function requireContactLogWriteAccess( + operation: "create" | "update" | "delete" +): Promise { + return AuthorizationService.getInstance().requireSecurityRoleForWrite({ + table: "Contact_Log", + operation, + }); } export async function getContactLogTypes(): Promise { try { - const session = await auth.api.getSession({ headers: await headers() }); - if (!session?.user?.id) { - throw new Error("Authentication required"); - } + await requireSession(); const contactLogService = await ContactLogService.getInstance(); const types = await contactLogService.getContactLogTypes(); @@ -38,35 +70,15 @@ export async function createContactLog( contactLogData: Omit ): Promise { try { - const session = await auth.api.getSession({ headers: await headers() }); - if (!session?.user?.id) { - throw new Error("Authentication required"); - } - - const userGuid = getUserGuid(session); - - // Fetch User_ID from MP using User_GUID - const { MPHelper } = await import("@/lib/providers/ministry-platform"); - const mp = new MPHelper(); - - const users = await mp.getTableRecords<{ User_ID: number }>({ - table: "dp_Users", - filter: `User_GUID = '${sanitizeGuid(userGuid)}'`, - select: "User_ID", - top: 1 - }); - - if (!users || users.length === 0 || !users[0].User_ID) { - throw new Error("Unable to determine user User_ID"); - } - - const userId = users[0].User_ID; + await requireSession(); if (!contactLogData.Contact_ID || !contactLogData.Contact_Date || !contactLogData.Notes) { throw new Error("Required fields are missing: Contact_ID, Contact_Date, and Notes are required"); } - // Add Made_By from session (User_ID of logged-in user) + const userId = await requireContactLogWriteAccess("create"); + + // Made_By records who made the contact, taken from the acting session. const logDataWithUser = { ...contactLogData, Made_By: userId, @@ -90,45 +102,25 @@ export async function updateContactLog( contactLogData: Partial> ): Promise { try { - const session = await auth.api.getSession({ headers: await headers() }); - if (!session?.user?.id) { - throw new Error("Authentication required"); - } - - const userGuid = getUserGuid(session); - - // Fetch User_ID from MP using User_GUID - const { MPHelper } = await import("@/lib/providers/ministry-platform"); - const mp = new MPHelper(); - - const users = await mp.getTableRecords<{ User_ID: number }>({ - table: "dp_Users", - filter: `User_GUID = '${sanitizeGuid(userGuid)}'`, - select: "User_ID", - top: 1 - }); + await requireSession(); - if (!users || users.length === 0 || !users[0].User_ID) { - throw new Error("Unable to determine user User_ID"); - } - - const userId = users[0].User_ID; - - if (!contactLogId || contactLogId <= 0) { - throw new Error("Valid Contact Log ID is required"); - } + // Validates at the boundary. TypeScript's `number` is erased at runtime and a + // caller controls this POST payload's shape, so the ID must be checked here + // rather than trusted downstream. + const logId = sanitizeNumericId(contactLogId, "Contact Log ID"); - // Add Made_By from session (User_ID of logged-in user) - const logDataWithUser = { - ...contactLogData, - Made_By: userId, - }; + // Gate the write. The returned User_ID is deliberately NOT written to + // Made_By: that column records who made the *contact*, not who last edited + // the row. Under this policy any role-holder may edit anyone's log, so + // stamping the editor would rewrite the pastoral record's authorship. MP's + // audit trail already captures the editor via `$userId` in ContactLogService. + await requireContactLogWriteAccess("update"); - console.log("updateContactLog action - Updating log:", contactLogId); - console.log("updateContactLog action - Update data:", JSON.stringify(logDataWithUser, null, 2)); + console.log("updateContactLog action - Updating log:", logId); + console.log("updateContactLog action - Update data:", JSON.stringify(contactLogData, null, 2)); const contactLogService = await ContactLogService.getInstance(); - const contactLog = await contactLogService.updateContactLog(contactLogId, logDataWithUser); + const contactLog = await contactLogService.updateContactLog(logId, contactLogData); console.log("updateContactLog action - Successfully updated"); return contactLog; @@ -140,19 +132,16 @@ export async function updateContactLog( export async function deleteContactLog(contactLogId: number): Promise { try { - const session = await auth.api.getSession({ headers: await headers() }); - if (!session?.user?.id) { - throw new Error("Authentication required"); - } + await requireSession(); - if (!contactLogId || contactLogId <= 0) { - throw new Error("Valid Contact Log ID is required"); - } + const logId = sanitizeNumericId(contactLogId, "Contact Log ID"); - console.log("deleteContactLog action - Deleting log:", contactLogId); + await requireContactLogWriteAccess("delete"); + + console.log("deleteContactLog action - Deleting log:", logId); const contactLogService = await ContactLogService.getInstance(); - await contactLogService.deleteContactLog(contactLogId); + await contactLogService.deleteContactLog(logId); console.log("deleteContactLog action - Successfully deleted"); } catch (error) { @@ -163,17 +152,12 @@ export async function deleteContactLog(contactLogId: number): Promise { export async function getContactLogsByContactId(contactId: number): Promise { try { - const session = await auth.api.getSession({ headers: await headers() }); - if (!session?.user?.id) { - throw new Error("Authentication required"); - } + await requireSession(); - if (!contactId || contactId <= 0) { - throw new Error("Valid contact ID is required"); - } + const id = sanitizeNumericId(contactId, "Contact ID"); const contactLogService = await ContactLogService.getInstance(); - const results = await contactLogService.getContactLogsByContactId(contactId); + const results = await contactLogService.getContactLogsByContactId(id); return results; } catch (error) { @@ -184,17 +168,12 @@ export async function getContactLogsByContactId(contactId: number): Promise { try { - const session = await auth.api.getSession({ headers: await headers() }); - if (!session?.user?.id) { - throw new Error("Authentication required"); - } + await requireSession(); - if (!contactLogId || contactLogId <= 0) { - throw new Error("Valid contact log ID is required"); - } + const logId = sanitizeNumericId(contactLogId, "Contact Log ID"); const contactLogService = await ContactLogService.getInstance(); - const result = await contactLogService.getContactLogById(contactLogId); + const result = await contactLogService.getContactLogById(logId); return result; } catch (error) { diff --git a/src/components/contact-logs/contact-logs.test.tsx b/src/components/contact-logs/contact-logs.test.tsx new file mode 100644 index 00000000..c7eb6482 --- /dev/null +++ b/src/components/contact-logs/contact-logs.test.tsx @@ -0,0 +1,343 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { render, screen, fireEvent, waitFor, within } from "@testing-library/react"; +import type { ContactLogDisplay } from "@/lib/dto"; + +/** + * ContactLogs component tests — targeted, not exhaustive. + * + * This component is the only interactive path in the app that mutates Ministry + * Platform data, so these tests cover the three places where a regression would + * silently corrupt or delete real member records: + * + * 1. the delete-confirmation gate — delete must not fire before confirmation + * 2. client-side validation — invalid forms must never reach the action + * 3. error surfacing — a failed action must be shown, and must not + * close the dialog or signal a refresh as if + * it had succeeded + * + * See `.claude/TODO/contact-logs-component-untested.md` (the original gap) and + * `.claude/references/testing.md`. + */ + +const { + mockGetContactLogTypes, + mockCreateContactLog, + mockUpdateContactLog, + mockDeleteContactLog, +} = vi.hoisted(() => ({ + mockGetContactLogTypes: vi.fn(), + mockCreateContactLog: vi.fn(), + mockUpdateContactLog: vi.fn(), + mockDeleteContactLog: vi.fn(), +})); + +vi.mock("./actions", () => ({ + getContactLogTypes: mockGetContactLogTypes, + createContactLog: mockCreateContactLog, + updateContactLog: mockUpdateContactLog, + deleteContactLog: mockDeleteContactLog, +})); + +import { ContactLogs } from "./contact-logs"; + +// Radix primitives need a few browser APIs jsdom does not implement. Without +// these, Dialog/AlertDialog/Select throw on mount rather than failing an +// assertion, which makes every test below look like a component bug. +function installJsdomPolyfills() { + if (!globalThis.ResizeObserver) { + globalThis.ResizeObserver = class { + observe() {} + unobserve() {} + disconnect() {} + } as unknown as typeof ResizeObserver; + } + const proto = Element.prototype as unknown as Record; + proto.hasPointerCapture ??= () => false; + proto.setPointerCapture ??= () => {}; + proto.releasePointerCapture ??= () => {}; + proto.scrollIntoView ??= () => {}; +} + +const MP_TZ = "America/New_York"; + +const logs: ContactLogDisplay[] = [ + { + Contact_Log_ID: 501, + Contact_ID: 42, + Contact_Date: "2026-08-20T14:30:00", + Notes: "Called about the new members class.", + Contact_Log_Type: "Phone Call", + Contact_Log_Type_ID: 1, + // Deliberately a DIFFERENT user than the acting one: the component offers + // edit/delete on other people's logs, matching the decided policy. + Made_By: 12345, + MadeByContact: [ + { + Contact_ID: 12345, + First_Name: "Dana", + Nickname: "Dana", + Last_Name: "Reyes", + Email_Address: "dana@example.com", + Mobile_Phone: null, + Image_GUID: null, + }, + ], + }, +]; + +function renderLogs(overrides: Partial[0]> = {}) { + return render( + + ); +} + +/** Renders, opens the "Add Log" dialog, and returns its form scope. */ +async function openCreateDialog( + overrides: Partial[0]> = {} +) { + renderLogs(overrides); + fireEvent.click(screen.getByRole("button", { name: /add log/i })); + return within(await screen.findByRole("dialog")); +} + +describe("ContactLogs", () => { + let alertSpy: ReturnType; + + beforeEach(() => { + installJsdomPolyfills(); + vi.clearAllMocks(); + mockGetContactLogTypes.mockResolvedValue([ + { Contact_Log_Type_ID: 1, Contact_Log_Type: "Phone Call", Description: null }, + ]); + // The component reports failures with window.alert(); jsdom's default + // implementation logs "not implemented" noise, so stub it. + alertSpy = vi.spyOn(window, "alert").mockImplementation(() => {}); + vi.spyOn(console, "error").mockImplementation(() => {}); + vi.spyOn(console, "log").mockImplementation(() => {}); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + describe("delete confirmation gate", () => { + it("opens the confirmation without calling deleteContactLog", async () => { + renderLogs(); + + clickDeleteIcon(); + + // The confirmation is now on screen and nothing has been deleted. + expect(await screen.findByRole("alertdialog")).toBeInTheDocument(); + expect(mockDeleteContactLog).not.toHaveBeenCalled(); + }); + + it("does not call deleteContactLog when the confirmation is cancelled", async () => { + renderLogs(); + + clickDeleteIcon(); + const dialog = await screen.findByRole("alertdialog"); + fireEvent.click(within(dialog).getByRole("button", { name: /cancel/i })); + + await waitFor(() => + expect(screen.queryByRole("alertdialog")).not.toBeInTheDocument() + ); + expect(mockDeleteContactLog).not.toHaveBeenCalled(); + }); + + it("calls deleteContactLog with the log ID only after the confirmation is accepted", async () => { + const onRefresh = vi.fn(); + mockDeleteContactLog.mockResolvedValueOnce(undefined); + renderLogs({ onRefresh }); + + clickDeleteIcon(); + const dialog = await screen.findByRole("alertdialog"); + expect(mockDeleteContactLog).not.toHaveBeenCalled(); + + fireEvent.click(within(dialog).getByRole("button", { name: /^delete$/i })); + + await waitFor(() => expect(mockDeleteContactLog).toHaveBeenCalledWith(501)); + expect(mockDeleteContactLog).toHaveBeenCalledTimes(1); + await waitFor(() => expect(onRefresh).toHaveBeenCalledTimes(1)); + }); + + it("surfaces a delete failure and does not signal a refresh", async () => { + const onRefresh = vi.fn(); + mockDeleteContactLog.mockRejectedValueOnce( + new Error("Not authorized: an MP security role is required") + ); + renderLogs({ onRefresh }); + + clickDeleteIcon(); + const dialog = await screen.findByRole("alertdialog"); + fireEvent.click(within(dialog).getByRole("button", { name: /^delete$/i })); + + await waitFor(() => + expect(alertSpy).toHaveBeenCalledWith( + "Error: Not authorized: an MP security role is required" + ) + ); + expect(onRefresh).not.toHaveBeenCalled(); + // The row is still on screen — nothing was optimistically removed. + expect( + screen.getByText("Called about the new members class.") + ).toBeInTheDocument(); + }); + }); + + describe("form validation before submit", () => { + it("does not call createContactLog when Notes is empty", async () => { + const form = await openCreateDialog(); + + fireEvent.click(form.getByRole("button", { name: /create log/i })); + + expect(await screen.findByText("Notes are required")).toBeInTheDocument(); + expect(mockCreateContactLog).not.toHaveBeenCalled(); + }); + + it("does not call createContactLog when the contact date is cleared", async () => { + const form = await openCreateDialog(); + + fireEvent.change(form.getByLabelText(/contact date/i), { target: { value: "" } }); + fireEvent.change(form.getByLabelText(/notes/i), { + target: { value: "Left a voicemail." }, + }); + fireEvent.click(form.getByRole("button", { name: /create log/i })); + + expect( + await screen.findByText("Contact date and time is required") + ).toBeInTheDocument(); + expect(mockCreateContactLog).not.toHaveBeenCalled(); + }); + + it("submits a valid form with the contact ID and notes", async () => { + const onRefresh = vi.fn(); + mockCreateContactLog.mockResolvedValueOnce({ Contact_Log_ID: 900 }); + const form = await openCreateDialog({ onRefresh }); + + fireEvent.change(form.getByLabelText(/notes/i), { + target: { value: "Left a voicemail." }, + }); + fireEvent.click(form.getByRole("button", { name: /create log/i })); + + await waitFor(() => expect(mockCreateContactLog).toHaveBeenCalledTimes(1)); + expect(mockCreateContactLog).toHaveBeenCalledWith( + expect.objectContaining({ + Contact_ID: 42, + Notes: "Left a voicemail.", + }) + ); + await waitFor(() => expect(onRefresh).toHaveBeenCalledTimes(1)); + }); + }); + + describe("action failure surfacing", () => { + it("alerts on a create failure, keeps the dialog open, and does not refresh", async () => { + const onRefresh = vi.fn(); + mockCreateContactLog.mockRejectedValueOnce(new Error("Required fields are missing")); + const form = await openCreateDialog({ onRefresh }); + + fireEvent.change(form.getByLabelText(/notes/i), { target: { value: "A note." } }); + fireEvent.click(form.getByRole("button", { name: /create log/i })); + + await waitFor(() => + expect(alertSpy).toHaveBeenCalledWith("Error: Required fields are missing") + ); + expect(onRefresh).not.toHaveBeenCalled(); + // Dialog stays open so the user can retry without retyping the note. + expect(screen.getByRole("dialog")).toBeInTheDocument(); + }); + + it("falls back to a generic message when the action rejects with a non-Error", async () => { + mockCreateContactLog.mockRejectedValueOnce("boom"); + const form = await openCreateDialog(); + + fireEvent.change(form.getByLabelText(/notes/i), { target: { value: "A note." } }); + fireEvent.click(form.getByRole("button", { name: /create log/i })); + + await waitFor(() => + expect(alertSpy).toHaveBeenCalledWith("Error: Failed to create contact log") + ); + }); + + it("renders the empty state without crashing when there are no logs", async () => { + renderLogs({ contactLogs: [] }); + + expect(screen.getByText("No contact logs found")).toBeInTheDocument(); + // Log types still load — the create form needs them. + await waitFor(() => expect(mockGetContactLogTypes).toHaveBeenCalled()); + }); + + it("keeps rendering when the log-types lookup fails", async () => { + mockGetContactLogTypes.mockRejectedValueOnce(new Error("MP unavailable")); + renderLogs(); + + await waitFor(() => expect(mockGetContactLogTypes).toHaveBeenCalled()); + expect(screen.getByText(/Contact Logs \(1\)/)).toBeInTheDocument(); + }); + }); + + describe("edit flow", () => { + it("opens the edit dialog prefilled and updates the log", async () => { + const onRefresh = vi.fn(); + mockUpdateContactLog.mockResolvedValueOnce({ Contact_Log_ID: 501 }); + renderLogs({ onRefresh }); + + fireEvent.click(screen.getByRole("button", { name: /^edit$/i })); + const dialog = await screen.findByRole("dialog"); + const form = within(dialog); + expect(form.getByLabelText(/notes/i)).toHaveValue( + "Called about the new members class." + ); + // MP wall-clock is passed through to the datetime-local input unchanged. + expect(form.getByLabelText(/contact date/i)).toHaveValue("2026-08-20T14:30"); + + fireEvent.change(form.getByLabelText(/notes/i), { target: { value: "Corrected." } }); + fireEvent.click(form.getByRole("button", { name: /save changes/i })); + + await waitFor(() => expect(mockUpdateContactLog).toHaveBeenCalledTimes(1)); + expect(mockUpdateContactLog).toHaveBeenCalledWith( + 501, + expect.objectContaining({ Notes: "Corrected." }) + ); + // Made_By is not sent by the component — the action does not stamp it + // either, so an edit never rewrites who made the contact. + expect(mockUpdateContactLog.mock.calls[0][1]).not.toHaveProperty("Made_By"); + await waitFor(() => expect(onRefresh).toHaveBeenCalledTimes(1)); + }); + + it("surfaces an update failure without refreshing", async () => { + const onRefresh = vi.fn(); + mockUpdateContactLog.mockRejectedValueOnce(new Error("Invalid Contact Log ID")); + renderLogs({ onRefresh }); + + fireEvent.click(screen.getByRole("button", { name: /^edit$/i })); + const form = within(await screen.findByRole("dialog")); + fireEvent.click(form.getByRole("button", { name: /save changes/i })); + + await waitFor(() => + expect(alertSpy).toHaveBeenCalledWith("Error: Invalid Contact Log ID") + ); + expect(onRefresh).not.toHaveBeenCalled(); + }); + }); +}); + +/** + * Clicks the icon-only delete button in the first log row. It has no accessible + * name, so it is identified as the button that is not "Edit". + */ +function clickDeleteIcon() { + const buttons = screen.getAllByRole("button"); + const deleteButton = buttons.find( + (b) => b.querySelector("svg") && !/edit|add log/i.test(b.textContent ?? "") + ); + if (!deleteButton) throw new Error("delete button not found"); + fireEvent.click(deleteButton); +} diff --git a/src/components/contact-lookup-details/actions.test.ts b/src/components/contact-lookup-details/actions.test.ts index 55ec8ebb..ca04d189 100644 --- a/src/components/contact-lookup-details/actions.test.ts +++ b/src/components/contact-lookup-details/actions.test.ts @@ -100,7 +100,7 @@ describe('contact-lookup-details actions', () => { it('should throw for invalid contact ID', async () => { mockGetSession.mockResolvedValueOnce(mockAuthSession); - await expect(getContactLogsByContactId(0)).rejects.toThrow('Valid contact ID is required'); + await expect(getContactLogsByContactId(0)).rejects.toThrow('Invalid Contact ID'); }); it('should return logs with type names mapped', async () => { @@ -160,4 +160,31 @@ describe('contact-lookup-details actions', () => { await expect(getContactLogsByContactId(42)).rejects.toThrow('Failed to fetch contact logs'); }); }); + + // Regression guard for `.claude/TODO/mp-filter-injection-numeric-ids.md`. This + // is the second reachable entry point into + // `ContactLogService.getContactLogsByContactId` and carried the same + // ineffective `!contactId || contactId <= 0` guard. + describe('numeric ID validation at the action boundary', () => { + it.each(['1 OR 1=1', '5; DROP', "1' OR '1'='1", '', 'abc', ' 7 '])( + 'rejects %j before reaching the service', + async (payload) => { + mockGetSession.mockResolvedValueOnce(mockAuthSession); + + await expect( + getContactLogsByContactId(payload as unknown as number) + ).rejects.toThrow('Invalid Contact ID'); + expect(mockGetContactLogsByContactId).not.toHaveBeenCalled(); + } + ); + + it('passes a digits-only ID through to the service as a number', async () => { + mockGetSession.mockResolvedValueOnce(mockAuthSession); + mockGetContactLogsByContactId.mockResolvedValueOnce([]); + + await getContactLogsByContactId('42' as unknown as number); + + expect(mockGetContactLogsByContactId).toHaveBeenCalledWith(42); + }); + }); }); diff --git a/src/components/contact-lookup-details/actions.ts b/src/components/contact-lookup-details/actions.ts index fa2a87b5..f4cce2d4 100644 --- a/src/components/contact-lookup-details/actions.ts +++ b/src/components/contact-lookup-details/actions.ts @@ -3,6 +3,7 @@ import { ContactLookupDetails, ContactLogDisplay } from '@/lib/dto'; import { ContactService } from '@/services/contactService'; import { ContactLogService } from '@/services/contactLogService'; +import { sanitizeNumericId } from '@/lib/providers/ministry-platform/utils/filter-sanitize'; import { auth } from '@/lib/auth'; import { headers } from 'next/headers'; @@ -38,12 +39,10 @@ export async function getContactLogsByContactId(contactId: number): Promise { it('should return plain strings unchanged', () => { @@ -89,3 +89,127 @@ describe('sanitizeGuid', () => { expect(() => sanitizeGuid('1234567g-1234-1234-1234-123456789abc')).toThrow('Invalid GUID format'); }); }); + +describe('sanitizeNumericId', () => { + it('should return a positive integer unchanged', () => { + expect(sanitizeNumericId(42)).toBe(42); + }); + + it('should accept 1 as the lowest valid ID', () => { + expect(sanitizeNumericId(1)).toBe(1); + }); + + it('should coerce a digits-only string', () => { + expect(sanitizeNumericId('42')).toBe(42); + }); + + it('should accept the largest safe integer', () => { + expect(sanitizeNumericId(Number.MAX_SAFE_INTEGER)).toBe(Number.MAX_SAFE_INTEGER); + }); + + // The reason this helper exists: a `number`-typed parameter can receive a + // string at runtime, and an unsanitized value widens or rewrites the filter. + it('should reject a boolean-OR injection payload', () => { + expect(() => sanitizeNumericId('1 OR 1=1')).toThrow('Invalid ID'); + }); + + it('should reject a statement-terminator injection payload', () => { + expect(() => sanitizeNumericId('5; DROP')).toThrow('Invalid ID'); + }); + + it('should reject a quoted injection payload', () => { + expect(() => sanitizeNumericId("1' OR '1'='1")).toThrow('Invalid ID'); + }); + + it('should reject a subquery injection payload', () => { + expect(() => sanitizeNumericId('1 UNION SELECT Password FROM dp_Users')).toThrow('Invalid ID'); + }); + + it('should reject a comment-terminated payload', () => { + expect(() => sanitizeNumericId('1 --')).toThrow('Invalid ID'); + }); + + it('should reject NaN', () => { + expect(() => sanitizeNumericId(NaN)).toThrow('Invalid ID'); + }); + + it('should reject Infinity', () => { + expect(() => sanitizeNumericId(Infinity)).toThrow('Invalid ID'); + }); + + it('should reject -Infinity', () => { + expect(() => sanitizeNumericId(-Infinity)).toThrow('Invalid ID'); + }); + + it('should reject non-integers', () => { + expect(() => sanitizeNumericId(1.5)).toThrow('Invalid ID'); + }); + + it('should reject negative numbers', () => { + expect(() => sanitizeNumericId(-1)).toThrow('Invalid ID'); + }); + + it('should reject zero', () => { + expect(() => sanitizeNumericId(0)).toThrow('Invalid ID'); + }); + + it('should reject null', () => { + expect(() => sanitizeNumericId(null)).toThrow('Invalid ID'); + }); + + it('should reject undefined', () => { + expect(() => sanitizeNumericId(undefined)).toThrow('Invalid ID'); + }); + + it('should reject whitespace-padded numerics', () => { + expect(() => sanitizeNumericId(' 7 ')).toThrow('Invalid ID'); + }); + + it('should reject the empty string', () => { + // Number('') is 0, so a bare Number() coercion would let this through the + // integer check; the digits-only test is what stops it. + expect(() => sanitizeNumericId('')).toThrow('Invalid ID'); + }); + + it('should reject hex notation', () => { + expect(() => sanitizeNumericId('0x10')).toThrow('Invalid ID'); + }); + + it('should reject exponent notation', () => { + expect(() => sanitizeNumericId('1e3')).toThrow('Invalid ID'); + }); + + it('should reject a signed numeric string', () => { + expect(() => sanitizeNumericId('+7')).toThrow('Invalid ID'); + }); + + it('should reject a decimal string', () => { + expect(() => sanitizeNumericId('7.0')).toThrow('Invalid ID'); + }); + + it('should reject values above the safe-integer range', () => { + // 1e21 stringifies to "1e+21", which would leak exponent notation into the filter. + expect(() => sanitizeNumericId(1e21)).toThrow('Invalid ID'); + }); + + it('should reject objects', () => { + expect(() => sanitizeNumericId({ valueOf: () => 5 })).toThrow('Invalid ID'); + }); + + it('should reject arrays', () => { + expect(() => sanitizeNumericId([5])).toThrow('Invalid ID'); + }); + + it('should reject booleans', () => { + expect(() => sanitizeNumericId(true)).toThrow('Invalid ID'); + }); + + it('should reject bigints', () => { + expect(() => sanitizeNumericId(BigInt(5))).toThrow('Invalid ID'); + }); + + it('should name the field in the error without echoing the value', () => { + expect(() => sanitizeNumericId('1 OR 1=1', 'Contact Log ID')).toThrow('Invalid Contact Log ID'); + expect(() => sanitizeNumericId('1 OR 1=1', 'Contact Log ID')).not.toThrow('1 OR 1=1'); + }); +}); diff --git a/src/lib/providers/ministry-platform/utils/filter-sanitize.ts b/src/lib/providers/ministry-platform/utils/filter-sanitize.ts index 077b86c6..0ed57a5f 100644 --- a/src/lib/providers/ministry-platform/utils/filter-sanitize.ts +++ b/src/lib/providers/ministry-platform/utils/filter-sanitize.ts @@ -44,3 +44,35 @@ export function sanitizeGuid(guid: string): string { } return guid; } + +/** + * Validates a numeric primary-key ID for safe interpolation into a filter string. + * Accepts a `number`, or a string of digits only (e.g. a route param or an + * un-coerced form field); everything else throws. The digits-only rule admits no + * character that could alter the surrounding filter, and the safe-integer bound + * keeps large values from stringifying into exponent notation (`1e+21`). + * + * Use for numeric comparisons: `Column = ${sanitizeNumericId(value, 'Contact ID')}`. + * TypeScript's `number` annotation is erased at runtime, and server actions compile + * to POST endpoints whose payload shape the caller controls, so a `number`-typed + * parameter must still be validated here. + * + * @param value - The candidate ID, from any source + * @param field - Field name used in the error message (never the offending value) + * @returns The validated ID as a positive safe integer + * @throws Error if the value is not a positive integer ID + */ +export function sanitizeNumericId(value: unknown, field = 'ID'): number { + const n = + typeof value === 'number' + ? value + : typeof value === 'string' && /^[0-9]+$/.test(value) + ? Number(value) + : NaN; + + if (!Number.isSafeInteger(n) || n <= 0) { + throw new Error(`Invalid ${field}`); + } + + return n; +} diff --git a/src/services/authorizationService.test.ts b/src/services/authorizationService.test.ts new file mode 100644 index 00000000..1dcb61bf --- /dev/null +++ b/src/services/authorizationService.test.ts @@ -0,0 +1,255 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; + +/** + * AuthorizationService tests. + * + * These encode the decided policy (see `.claude/references/auth.md`): any + * authenticated user holding an MP security role may write; ownership is not a + * factor; the gate fails closed when the acting MP user or the role list cannot + * be established. + */ + +const { mockGetTableRecords, mockGetActingUserIdForWrite } = vi.hoisted(() => ({ + mockGetTableRecords: vi.fn(), + mockGetActingUserIdForWrite: vi.fn(), +})); + +vi.mock("@/lib/providers/ministry-platform", () => ({ + MPHelper: class { + getTableRecords = mockGetTableRecords; + }, +})); + +vi.mock("@/services/sessionContextService", () => ({ + SessionContextService: { + getInstance: () => ({ + getActingUserIdForWrite: mockGetActingUserIdForWrite, + }), + }, +})); + +import { AuthorizationService, UnauthorizedError } from "./authorizationService"; + +const WRITE_CTX = { table: "Contact_Log", operation: "create" as const }; + +describe("AuthorizationService", () => { + let warnSpy: ReturnType; + + beforeEach(() => { + vi.clearAllMocks(); + // Reset the singleton so a stale MPHelper never leaks between tests. + (AuthorizationService as unknown as { instance: unknown }).instance = undefined; + delete process.env.MP_WRITE_SECURITY_ROLES; + warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + }); + + afterEach(() => { + warnSpy.mockRestore(); + delete process.env.MP_WRITE_SECURITY_ROLES; + }); + + describe("getInstance", () => { + it("returns the same instance on repeat calls", () => { + expect(AuthorizationService.getInstance()).toBe(AuthorizationService.getInstance()); + }); + }); + + describe("getSecurityRoles", () => { + it("queries dp_User_Roles for the given User_ID and returns role names", async () => { + mockGetTableRecords.mockResolvedValueOnce([ + { Role_Name: "Administrators" }, + { Role_Name: "Pastoral Staff" }, + ]); + + const roles = await AuthorizationService.getInstance().getSecurityRoles(99); + + expect(mockGetTableRecords).toHaveBeenCalledWith({ + table: "dp_User_Roles", + filter: "User_ID = 99", + select: "Role_ID_TABLE.Role_Name", + }); + expect(roles).toEqual(["Administrators", "Pastoral Staff"]); + }); + + it("returns an empty array when the user holds no roles", async () => { + mockGetTableRecords.mockResolvedValueOnce([]); + expect(await AuthorizationService.getInstance().getSecurityRoles(99)).toEqual([]); + }); + + it("drops null and blank role names rather than treating them as roles", async () => { + // A blank Role_Name must not satisfy the "holds any security role" check. + mockGetTableRecords.mockResolvedValueOnce([ + { Role_Name: null }, + { Role_Name: " " }, + { Role_Name: "Administrators" }, + ]); + + expect(await AuthorizationService.getInstance().getSecurityRoles(99)).toEqual([ + "Administrators", + ]); + }); + + it("tolerates a nullish response from MP", async () => { + mockGetTableRecords.mockResolvedValueOnce(undefined); + expect(await AuthorizationService.getInstance().getSecurityRoles(99)).toEqual([]); + }); + + it.each([0, -1, 1.5, NaN])( + "refuses to interpolate a non-positive-integer User_ID (%s)", + async (badId) => { + await expect( + AuthorizationService.getInstance().getSecurityRoles(badId) + ).rejects.toThrow(UnauthorizedError); + expect(mockGetTableRecords).not.toHaveBeenCalled(); + } + ); + }); + + describe("requireSecurityRoleForWrite", () => { + it("returns the acting User_ID when the user holds any security role", async () => { + mockGetActingUserIdForWrite.mockResolvedValueOnce(99); + mockGetTableRecords.mockResolvedValueOnce([{ Role_Name: "Pastoral Staff" }]); + + const userId = await AuthorizationService.getInstance().requireSecurityRoleForWrite( + WRITE_CTX + ); + + expect(userId).toBe(99); + expect(mockGetActingUserIdForWrite).toHaveBeenCalledWith(WRITE_CTX); + }); + + it("rejects when no MP user is attached to the session", async () => { + mockGetActingUserIdForWrite.mockResolvedValueOnce(null); + + await expect( + AuthorizationService.getInstance().requireSecurityRoleForWrite(WRITE_CTX) + ).rejects.toThrow(/no Ministry Platform user is attached/); + + // Fails closed — no role lookup is even attempted. + expect(mockGetTableRecords).not.toHaveBeenCalled(); + }); + + it("rejects an authenticated user who holds no security role", async () => { + mockGetActingUserIdForWrite.mockResolvedValueOnce(99); + mockGetTableRecords.mockResolvedValueOnce([]); + + await expect( + AuthorizationService.getInstance().requireSecurityRoleForWrite(WRITE_CTX) + ).rejects.toThrow(/an MP security role is required to create records in Contact_Log/); + }); + + it("emits a greppable mp.write.unauthorized warning on denial", async () => { + mockGetActingUserIdForWrite.mockResolvedValueOnce(99); + mockGetTableRecords.mockResolvedValueOnce([]); + + await expect( + AuthorizationService.getInstance().requireSecurityRoleForWrite({ + table: "Contact_Log", + operation: "delete", + }) + ).rejects.toThrow(UnauthorizedError); + + const payload = JSON.parse(warnSpy.mock.calls.at(-1)![0] as string); + expect(payload).toMatchObject({ + event: "mp.write.unauthorized", + table: "Contact_Log", + operation: "delete", + userId: 99, + reason: "no_security_role", + }); + }); + + it("distinguishes a missing MP user from a missing role in the denial log", async () => { + mockGetActingUserIdForWrite.mockResolvedValueOnce(null); + + await expect( + AuthorizationService.getInstance().requireSecurityRoleForWrite(WRITE_CTX) + ).rejects.toThrow(UnauthorizedError); + + const payload = JSON.parse(warnSpy.mock.calls.at(-1)![0] as string); + expect(payload).toMatchObject({ reason: "no_mp_user", userId: null }); + }); + + it("propagates a failed role lookup instead of allowing the write", async () => { + mockGetActingUserIdForWrite.mockResolvedValueOnce(99); + mockGetTableRecords.mockRejectedValueOnce(new Error("MP unavailable")); + + await expect( + AuthorizationService.getInstance().requireSecurityRoleForWrite(WRITE_CTX) + ).rejects.toThrow("MP unavailable"); + }); + + it("does not cache the authorization decision across writes", async () => { + // A revoked role must take effect immediately. + mockGetActingUserIdForWrite.mockResolvedValue(99); + mockGetTableRecords.mockResolvedValueOnce([{ Role_Name: "Administrators" }]); + const svc = AuthorizationService.getInstance(); + + await expect(svc.requireSecurityRoleForWrite(WRITE_CTX)).resolves.toBe(99); + + mockGetTableRecords.mockResolvedValueOnce([]); + await expect(svc.requireSecurityRoleForWrite(WRITE_CTX)).rejects.toThrow( + UnauthorizedError + ); + expect(mockGetTableRecords).toHaveBeenCalledTimes(2); + }); + + describe("MP_WRITE_SECURITY_ROLES", () => { + it("permits only the named roles when the env var is set", async () => { + process.env.MP_WRITE_SECURITY_ROLES = "Administrators,Pastoral Staff"; + mockGetActingUserIdForWrite.mockResolvedValueOnce(99); + mockGetTableRecords.mockResolvedValueOnce([{ Role_Name: "Pastoral Staff" }]); + + await expect( + AuthorizationService.getInstance().requireSecurityRoleForWrite(WRITE_CTX) + ).resolves.toBe(99); + }); + + it("rejects a role that is not on the list", async () => { + process.env.MP_WRITE_SECURITY_ROLES = "Administrators"; + mockGetActingUserIdForWrite.mockResolvedValueOnce(99); + mockGetTableRecords.mockResolvedValueOnce([{ Role_Name: "Volunteer" }]); + + await expect( + AuthorizationService.getInstance().requireSecurityRoleForWrite(WRITE_CTX) + ).rejects.toThrow(UnauthorizedError); + + const payload = JSON.parse(warnSpy.mock.calls.at(-1)![0] as string); + expect(payload).toMatchObject({ reason: "role_not_permitted" }); + }); + + it("compares role names case- and whitespace-insensitively", async () => { + process.env.MP_WRITE_SECURITY_ROLES = " administrators , Pastoral Staff "; + mockGetActingUserIdForWrite.mockResolvedValueOnce(99); + mockGetTableRecords.mockResolvedValueOnce([{ Role_Name: "ADMINISTRATORS" }]); + + await expect( + AuthorizationService.getInstance().requireSecurityRoleForWrite(WRITE_CTX) + ).resolves.toBe(99); + }); + + it("falls back to 'any security role' when the env var is blank or all separators", async () => { + process.env.MP_WRITE_SECURITY_ROLES = " , , "; + mockGetActingUserIdForWrite.mockResolvedValueOnce(99); + mockGetTableRecords.mockResolvedValueOnce([{ Role_Name: "Volunteer" }]); + + await expect( + AuthorizationService.getInstance().requireSecurityRoleForWrite(WRITE_CTX) + ).resolves.toBe(99); + }); + + it("is read per call, not captured at module load", async () => { + mockGetActingUserIdForWrite.mockResolvedValue(99); + mockGetTableRecords.mockResolvedValue([{ Role_Name: "Volunteer" }]); + const svc = AuthorizationService.getInstance(); + + await expect(svc.requireSecurityRoleForWrite(WRITE_CTX)).resolves.toBe(99); + + process.env.MP_WRITE_SECURITY_ROLES = "Administrators"; + await expect(svc.requireSecurityRoleForWrite(WRITE_CTX)).rejects.toThrow( + UnauthorizedError + ); + }); + }); + }); +}); diff --git a/src/services/authorizationService.ts b/src/services/authorizationService.ts new file mode 100644 index 00000000..2fe9fb3f --- /dev/null +++ b/src/services/authorizationService.ts @@ -0,0 +1,194 @@ +import { MPHelper } from "@/lib/providers/ministry-platform"; +import { sanitizeNumericId } from "@/lib/providers/ministry-platform/utils/filter-sanitize"; +import { SessionContextService } from "@/services/sessionContextService"; + +/** + * Thrown when the acting user is authenticated but not permitted to perform + * the requested Ministry Platform write. + * + * Distinct from the generic `Error` the actions throw for authentication and + * argument problems so callers (and tests) can tell "you are not signed in" + * apart from "you are signed in but may not do this". + */ +export class UnauthorizedError extends Error { + constructor(message: string) { + super(message); + this.name = "UnauthorizedError"; + } +} + +/** + * Env var naming the MP security roles permitted to perform gated writes, + * comma-separated (e.g. `MP_WRITE_SECURITY_ROLES="Administrators,Pastoral Staff"`). + * + * Unset or empty means "any MP security role" — the decided default policy. + * Set it to tighten the gate without a code change. + */ +const REQUIRED_ROLES_ENV = "MP_WRITE_SECURITY_ROLES"; + +function normalizeRoleName(role: string): string { + return role.trim().toLowerCase(); +} + +/** + * Parses `MP_WRITE_SECURITY_ROLES` into normalized role names, or returns null + * when unset/blank, which means "holding any MP security role is sufficient". + * Read per call rather than at module load so tests and redeploys see changes. + */ +function parseRequiredRoles(): string[] | null { + const raw = process.env[REQUIRED_ROLES_ENV]; + if (!raw) return null; + const names = raw + .split(",") + .map(normalizeRoleName) + .filter((r) => r.length > 0); + return names.length > 0 ? names : null; +} + +/** + * AuthorizationService — decides whether the acting user may perform a + * Ministry Platform write. + * + * Policy (decided 2026-08-21, see `.claude/references/auth.md`): any + * authenticated user who holds an MP security role may create, edit, and + * delete contact logs — including logs another user created. MP security roles + * are the domain's own authorization mechanism, so this app defers to them + * rather than inventing a parallel one. Ownership (`Made_By`) is deliberately + * NOT a factor: staff need to be able to correct and remove each other's logs. + * + * Authentication alone is not sufficient. A session for an MP user with no + * security role at all cannot write, and neither can a session whose MP + * `User_ID` never resolved — the gate fails closed. + * + * No caching. Roles are re-read from MP on every gated write, one extra read + * per write. Writes are rare (a staff member saving a form) and a cached + * authorization decision means a revoked role keeps working; that trade is not + * worth making for a shared production database. + */ +export class AuthorizationService { + private static instance: AuthorizationService | null = null; + private mp: MPHelper | null = null; + + private constructor() {} + + /** + * Lazily creates the MP helper. Kept out of the constructor so importing + * this module never touches the MP provider (or its env vars) — the + * module-level singleton below is created at import time. + */ + private helper(): MPHelper { + if (!this.mp) { + this.mp = new MPHelper(); + } + return this.mp; + } + + public static getInstance(): AuthorizationService { + if (!AuthorizationService.instance) { + AuthorizationService.instance = new AuthorizationService(); + } + return AuthorizationService.instance; + } + + /** + * Returns the MP security role names held by a user. + * + * @param userId - MP `User_ID` (must be a positive integer) + * @returns Role names from `dp_User_Roles`; empty when the user holds none + */ + public async getSecurityRoles(userId: number): Promise { + // `sanitizeNumericId` is the single source of truth for the numeric-ID rule + // (see filter-sanitize.ts) and guards the interpolation below. Its plain + // Error is re-thrown as UnauthorizedError: an unusable acting User_ID means + // we cannot establish permission, so it must fail closed as an authz denial + // rather than surface as a generic validation error. + let safeUserId: number; + try { + safeUserId = sanitizeNumericId(userId, "acting MP User_ID"); + } catch { + throw new UnauthorizedError( + "Not authorized: acting MP User_ID is not a valid identifier", + ); + } + + const records = await this.helper().getTableRecords<{ Role_Name: string | null }>({ + table: "dp_User_Roles", + filter: `User_ID = ${safeUserId}`, + select: "Role_ID_TABLE.Role_Name", + }); + + return (records ?? []) + .map((r) => r.Role_Name) + .filter((name): name is string => Boolean(name && name.trim())); + } + + /** + * Gates an MP write on security-role membership and returns the acting + * user's MP `User_ID` so callers can use it for attribution. + * + * Resolves the acting user through `SessionContextService`, so an + * unattributed write still emits the structured `mp.write.non_user` warning + * before this gate rejects it — the attempt stays visible in production logs. + * + * @throws UnauthorizedError when no MP user is attached to the session, or + * when the user holds no permitted security role + */ + public async requireSecurityRoleForWrite(ctx: { + table: string; + operation: "create" | "update" | "delete"; + }): Promise { + const userId = await SessionContextService.getInstance() + .getActingUserIdForWrite(ctx); + + if (userId === null) { + this.logDenied({ ...ctx, userId: null, reason: "no_mp_user" }); + throw new UnauthorizedError( + `Not authorized: no Ministry Platform user is attached to this session (${ctx.operation} on ${ctx.table})`, + ); + } + + const roles = await this.getSecurityRoles(userId); + const required = parseRequiredRoles(); + const permitted = + required === null + ? roles.length > 0 + : roles.some((r) => required.includes(normalizeRoleName(r))); + + if (!permitted) { + this.logDenied({ + ...ctx, + userId, + reason: roles.length === 0 ? "no_security_role" : "role_not_permitted", + }); + throw new UnauthorizedError( + `Not authorized: an MP security role is required to ${ctx.operation} records in ${ctx.table}`, + ); + } + + return userId; + } + + /** + * Emits a structured denial so refused writes are greppable in production + * logs. Same shape convention as `mp.write.non_user`. + */ + private logDenied(ctx: { + table: string; + operation: string; + userId: number | null; + reason: string; + }): void { + console.warn( + JSON.stringify({ + event: "mp.write.unauthorized", + message: "MP write refused — acting user lacks a permitted security role", + table: ctx.table, + operation: ctx.operation, + userId: ctx.userId, + reason: ctx.reason, + }), + ); + } +} + +export const authorizationService = AuthorizationService.getInstance(); diff --git a/src/services/contactLogService.test.ts b/src/services/contactLogService.test.ts index 504a8012..c8eecc4b 100644 --- a/src/services/contactLogService.test.ts +++ b/src/services/contactLogService.test.ts @@ -423,4 +423,87 @@ describe('ContactLogService', () => { await expect(service.deleteContactLog(999)).rejects.toThrow('Record not found'); }); }); + + // Regression guard for `.claude/TODO/mp-filter-injection-numeric-ids.md`. + // + // Every method here declares `number`, but that annotation is erased at + // runtime and server actions compile to POST endpoints whose payload shape the + // caller controls — so a string does reach these methods. Before the fix, + // `getContactLogById('1 OR 1=1')` built the filter `Contact_Log_ID = 1 OR 1=1` + // and widened a single-record read into a full-table read. Asserting the filter + // string and that no request is issued is what statement coverage could not see: + // this file was at 100% while the defect was live. + describe('filter injection via numeric IDs', () => { + const injectionPayloads = [ + '1 OR 1=1', + '5; DROP', + "1' OR '1'='1", + '1 UNION SELECT Password FROM dp_Users', + '1 --', + '1.5', + '-1', + ' 7 ', + '', + 'abc', + ]; + + it.each(injectionPayloads)('getContactLogById rejects %j without calling MP', async (payload) => { + const service = await ContactLogService.getInstance(); + + await expect( + service.getContactLogById(payload as unknown as number) + ).rejects.toThrow('Invalid Contact Log ID'); + expect(mockGetTableRecords).not.toHaveBeenCalled(); + }); + + it.each(injectionPayloads)('getContactLogsByContactId rejects %j without calling MP', async (payload) => { + const service = await ContactLogService.getInstance(); + + await expect( + service.getContactLogsByContactId(payload as unknown as number) + ).rejects.toThrow('Invalid Contact ID'); + expect(mockGetTableRecords).not.toHaveBeenCalled(); + }); + + it.each(injectionPayloads)('searchContactLogs rejects %j without calling MP', async (payload) => { + const service = await ContactLogService.getInstance(); + + await expect( + service.searchContactLogs(payload as unknown as number) + ).rejects.toThrow('Invalid Contact ID'); + expect(mockGetTableRecords).not.toHaveBeenCalled(); + }); + + it('interpolates a digits-only string as a bare number', async () => { + mockGetTableRecords.mockResolvedValueOnce([]); + + const service = await ContactLogService.getInstance(); + await service.getContactLogById('42' as unknown as number); + + expect(mockGetTableRecords).toHaveBeenCalledWith( + expect.objectContaining({ filter: 'Contact_Log_ID = 42' }) + ); + }); + + it('searchContactLogs still reads unfiltered when the ID is omitted or null', async () => { + mockGetTableRecords.mockResolvedValue([]); + + const service = await ContactLogService.getInstance(); + await service.searchContactLogs(); + await service.searchContactLogs(null as unknown as number); + + expect(mockGetTableRecords).toHaveBeenNthCalledWith(1, expect.objectContaining({ filter: '' })); + expect(mockGetTableRecords).toHaveBeenNthCalledWith(2, expect.objectContaining({ filter: '' })); + }); + + it('searchContactLogs now rejects 0 instead of silently reading the whole table', async () => { + // Behavior change: the old `if (contactId)` truthiness check treated 0 as + // "no filter" and returned an unfiltered page. An explicit 0 is a caller + // bug, so it is now an error. + const service = await ContactLogService.getInstance(); + + await expect(service.searchContactLogs(0)).rejects.toThrow('Invalid Contact ID'); + expect(mockGetTableRecords).not.toHaveBeenCalled(); + }); + }); }); diff --git a/src/services/contactLogService.ts b/src/services/contactLogService.ts index 583c9986..dac461c2 100644 --- a/src/services/contactLogService.ts +++ b/src/services/contactLogService.ts @@ -2,6 +2,7 @@ import { ContactLog } from "@/lib/providers/ministry-platform/models/ContactLog" import { ContactLogTypes } from "@/lib/providers/ministry-platform/models/ContactLogTypes"; import { ContactLogSchema, ContactLogInput } from "@/lib/providers/ministry-platform/models/ContactLogSchema"; import { MPHelper } from "@/lib/providers/ministry-platform"; +import { sanitizeNumericId } from "@/lib/providers/ministry-platform/utils/filter-sanitize"; import { DomainTimezoneService } from "@/services/domainTimezoneService"; import { SessionContextService } from "@/services/sessionContextService"; @@ -67,15 +68,16 @@ export class ContactLogService { /** * Searches for contact log records based on contact ID * - * @param contactId - The contact ID to search for logs + * @param contactId - The contact ID to search for logs; omit for an unfiltered read * @param limit - Maximum number of records to return (default: 50) * @returns Promise - Array of matching contact log records + * @throws Error if contactId is supplied but is not a positive integer ID */ public async searchContactLogs(contactId?: number, limit: number = 50): Promise { let filter = ""; - - if (contactId) { - filter = `Contact_ID = ${contactId}`; + + if (contactId !== undefined && contactId !== null) { + filter = `Contact_ID = ${sanitizeNumericId(contactId, "Contact ID")}`; } const records = await this.mp!.getTableRecords({ @@ -94,11 +96,12 @@ export class ContactLogService { * * @param contactLogId - The unique ID of the contact log record * @returns Promise - The matching contact log record or null if not found + * @throws Error if contactLogId is not a positive integer ID */ public async getContactLogById(contactLogId: number): Promise { const records = await this.mp!.getTableRecords({ table: "Contact_Log", - filter: `Contact_Log_ID = ${contactLogId}`, + filter: `Contact_Log_ID = ${sanitizeNumericId(contactLogId, "Contact Log ID")}`, select: "Contact_Log_ID,Contact_ID,Contact_Date,Made_By,Notes,Contact_Log_Type_ID,Planned_Contact_ID,Contact_Successful,Original_Contact_Log_Entry,Feedback_Entry_ID", top: 1 }); @@ -111,11 +114,12 @@ export class ContactLogService { * * @param contactId - The contact ID to get logs for * @returns Promise - Array of contact log records for the contact + * @throws Error if contactId is not a positive integer ID */ public async getContactLogsByContactId(contactId: number): Promise { const records = await this.mp!.getTableRecords({ table: "Contact_Log", - filter: `Contact_ID = ${contactId}`, + filter: `Contact_ID = ${sanitizeNumericId(contactId, "Contact ID")}`, select: "Contact_Log_ID,Contact_ID,Contact_Date,Made_By,Notes,Contact_Log_Type_ID,Planned_Contact_ID,Contact_Successful,Original_Contact_Log_Entry,Feedback_Entry_ID", orderBy: "Contact_Date DESC" }); diff --git a/src/services/userService.test.ts b/src/services/userService.test.ts index 1454b291..12151d3a 100644 --- a/src/services/userService.test.ts +++ b/src/services/userService.test.ts @@ -123,5 +123,26 @@ describe('UserService', () => { const service = await UserService.getInstance(); await expect(service.getUserProfile('not-a-guid')).rejects.toThrow('Invalid GUID format'); }); + + // The User_ID here comes back from MP rather than from a caller, so this is + // belt-and-braces: it keeps the "no filter string is built from an + // unvalidated value" rule true for this file. + it('should throw rather than filter on a malformed User_ID from MP', async () => { + mockGetTableRecords.mockResolvedValueOnce([ + { User_ID: '1 OR 1=1', User_GUID: validGuid }, + ]); + + const service = await UserService.getInstance(); + await expect(service.getUserProfile(validGuid)).rejects.toThrow('Invalid User ID'); + // Only the dp_Users lookup ran; neither role query was issued. + expect(mockGetTableRecords).toHaveBeenCalledTimes(1); + }); + + it('should throw when the MP row has no User_ID', async () => { + mockGetTableRecords.mockResolvedValueOnce([{ User_GUID: validGuid }]); + + const service = await UserService.getInstance(); + await expect(service.getUserProfile(validGuid)).rejects.toThrow('Invalid User ID'); + }); }); }); diff --git a/src/services/userService.ts b/src/services/userService.ts index 91801d8d..cab1164a 100644 --- a/src/services/userService.ts +++ b/src/services/userService.ts @@ -1,6 +1,6 @@ import { MPUserProfile } from "@/lib/providers/ministry-platform/types"; import { MPHelper } from "@/lib/providers/ministry-platform"; -import { sanitizeGuid } from "@/lib/providers/ministry-platform/utils/filter-sanitize"; +import { sanitizeGuid, sanitizeNumericId } from "@/lib/providers/ministry-platform/utils/filter-sanitize"; /** * UserService - Singleton service for managing user-related operations @@ -69,15 +69,19 @@ export class UserService { const profile = records[0]; if (!profile) return undefined; + // Sanitized even though the value originates from MP, so that no filter string + // in this file is built from an unvalidated value. + const userId = sanitizeNumericId(profile.User_ID, "User ID"); + const [roleRecords, groupRecords] = await Promise.all([ this.mp!.getTableRecords<{ Role_Name: string }>({ table: "dp_User_Roles", - filter: `User_ID = ${profile.User_ID}`, + filter: `User_ID = ${userId}`, select: "Role_ID_TABLE.Role_Name", }), this.mp!.getTableRecords<{ User_Group_Name: string }>({ table: "dp_User_User_Groups", - filter: `User_ID = ${profile.User_ID}`, + filter: `User_ID = ${userId}`, select: "User_Group_ID_TABLE.User_Group_Name", }), ]);