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 f7c41bb..0000000 --- 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 5c84526..0000000 --- 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 899c568..0000000 --- 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 c70986e..0000000 --- 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 00eda80..686412b 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 725124b..e116d79 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 feb174e..ad0744a 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 5ff98c8..716382e 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 f7c8704..44c7356 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 a196a1c..9577e83 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 0000000..c7eb648 --- /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 55ec8eb..ca04d18 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 fa2a87b..f4cce2d 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 077b86c..0ed57a5 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 0000000..1dcb61b --- /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 0000000..2fe9fb3 --- /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 504a801..c8eecc4 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 583c998..dac461c 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 1454b29..12151d3 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 91801d8..cab1164 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", }), ]);