diff --git a/.claude/playbooks/port-better-auth-1.6-userguid.md b/.claude/playbooks/port-better-auth-1.6-userguid.md index a7c8703..f3747db 100644 --- a/.claude/playbooks/port-better-auth-1.6-userguid.md +++ b/.claude/playbooks/port-better-auth-1.6-userguid.md @@ -81,9 +81,15 @@ plugin. Better Auth generates its own internal `user.id`; the MP `User_GUID` (th OAuth `sub`) is carried on the session as a **custom user `additionalField`** named `userGuid`, populated server-side from the OAuth profile via `mapProfileToUser`. **Everything MP-related keys off `userGuid`** — the client -`UserProvider` calls `getCurrentUserProfile(userGuid)` to load the profile +`UserProvider` calls `getCurrentUserProfile()` to load the profile (avatar, name). No `userGuid` → no profile → dead avatar/menu. +> If your fork still declares that action as `getCurrentUserProfile(userGuid)`, +> fix it separately: a server action is a caller-shaped POST endpoint, so the +> parameter is an IDOR — any authenticated MP user can read another user's +> profile. Derive the GUID from the session inside the action. Unrelated to the +> 1.6 upgrade, but you will be looking right at the code. + **The breaking change:** As of Better Auth **1.6**, the function that pulls additional fields off an OAuth provider profile stopped letting a field declared `input: false` through when a value is supplied. Two things changed versus the @@ -643,7 +649,7 @@ unusual route-group layout — stop and ask the user before improvising. because `genericOAuth`'s `additionalFields` aren't inferred — e.g. `(session?.user as { userGuid?: string })?.userGuid`. - **The avatar/menu chain end-to-end:** `useSession()` → `session.user.userGuid` - → `UserProvider` → `getCurrentUserProfile(userGuid)` → `MPUserProfile` + → `UserProvider` → `getCurrentUserProfile()` → `MPUserProfile` (`Image_GUID`, names) → `Header` renders the photo + `UserMenu`. Any break in `userGuid` collapses the whole chain to a non-interactive fallback (a generic `UserCircleIcon`, not text initials). diff --git a/.claude/references/DECISIONS.md b/.claude/references/DECISIONS.md index 72cd75c..e98eb01 100644 --- a/.claude/references/DECISIONS.md +++ b/.claude/references/DECISIONS.md @@ -80,7 +80,7 @@ Architectural decisions captured by the context-engineering review at SHA `971c4 **Date:** 2026-04-17 **Status:** Accepted **Context:** A tempting design is to enrich the session object inside `customSession` with the user's full MP profile (roles, user groups, Contact_ID, Image_GUID). Every consumer would then read a single object. The downside is that `customSession` runs on every cache miss, and MP profile lookups require extra MP API calls (`dp_Users` + `dp_User_Roles` + `dp_User_User_Groups`). -**Decision:** `customSession` in `src/lib/auth.ts:97-112` does only `firstName` / `lastName` splitting from `user.name` — no API calls. MP profile loading moves to the client, behind `UserProvider` (`src/contexts/user-context.tsx`), which calls the `getCurrentUserProfile(userGuid)` server action on mount and exposes `useUser()`. +**Decision:** `customSession` in `src/lib/auth.ts:398-413` does only `firstName` / `lastName` splitting from `user.name` — no API calls. MP profile loading moves to the client, behind `UserProvider` (`src/contexts/user-context.tsx`), which calls the parameterless `getCurrentUserProfile()` server action on mount (the action re-derives the GUID from the session — a GUID parameter would be an IDOR) and exposes `useUser()`. **Consequences:** `getSession()` stays cheap. Sign-in does not break when MP is down. Client components that need roles/groups must mount under `UserProvider`; every page load incurs one extra round-trip. `UserService.getUserProfile()` issues three queries (profile + roles + groups). **Alternatives considered:** - **Enrich in `customSession`** — would hit MP API on every JWT refresh and couple sign-in availability to MP uptime. diff --git a/.claude/references/auth/oauth-flow.md b/.claude/references/auth/oauth-flow.md index 7bff4a6..e7af464 100644 --- a/.claude/references/auth/oauth-flow.md +++ b/.claude/references/auth/oauth-flow.md @@ -119,7 +119,8 @@ mapProfileToUser: (profile) => { e. Creates account (accountId=sub, tokens) — storeAccountCookie: true f. Creates session → sets JWT cookie (cookieCache) 8. Browser lands on callbackURL (app page) -9. Client-side UserProvider reads session.user.userGuid → getCurrentUserProfile(userGuid) +9. Client-side UserProvider reads session.user.userGuid (to decide whether to load) + → getCurrentUserProfile() [server action re-derives the GUID from the session] ``` ## Sign-in entry (verbatim from `src/app/signin/page.tsx`) diff --git a/.claude/references/auth/sessions.md b/.claude/references/auth/sessions.md index 633d39c..d1364d5 100644 --- a/.claude/references/auth/sessions.md +++ b/.claude/references/auth/sessions.md @@ -50,7 +50,7 @@ user: { ## `customSession` (verbatim) ```typescript -// src/lib/auth.ts:97-112 +// src/lib/auth.ts:398-413 customSession( async ({ user, session }) => { // No API calls here — profile loading is handled by UserProvider diff --git a/.claude/references/auth/user-identity.md b/.claude/references/auth/user-identity.md index bb69320..32469d4 100644 --- a/.claude/references/auth/user-identity.md +++ b/.claude/references/auth/user-identity.md @@ -123,12 +123,13 @@ const userGuid = (session?.user as { userGuid?: string } | undefined)?.userGuid; ```typescript // src/contexts/user-context.tsx:29-49 (excerpt) +// userGuid gates whether the load fires; it is NOT passed to the action. const userGuid = (session?.user as { userGuid?: string } | undefined)?.userGuid; const loadUserProfile = useCallback(async () => { if (!userGuid) { /* ... */ return; } // ... - const profile = await getCurrentUserProfile(userGuid); + const profile = await getCurrentUserProfile(); // no argument: the action re-derives the GUID server-side setUserProfile(profile ?? null); }, [userGuid]); ``` diff --git a/.claude/references/components/layout.md b/.claude/references/components/layout.md index e19d458..8618984 100644 --- a/.claude/references/components/layout.md +++ b/.claude/references/components/layout.md @@ -15,14 +15,14 @@ related: - ../routing/README.md - ../services/README.md - tool-framework.md -last_verified: 2026-04-17 +last_verified: 2026-09-13 --- ## Purpose `AuthWrapper` is the server-side session gate used at the app-shell level; it redirects unauthenticated requests to `/signin` while preserving the original path+query as `callbackUrl`. `shared-actions/` holds server actions used across multiple features (currently just `getCurrentUserProfile`). ## Files -- `src/components/layout/auth-wrapper.tsx` — server component, 20 lines +- `src/components/layout/auth-wrapper.tsx` — server component, 31 lines - `src/components/layout/auth-wrapper.test.tsx` — redirect + callback preservation tests - `src/components/layout/index.ts` — barrel: `AuthWrapper` - `src/components/shared-actions/user.ts` — `getCurrentUserProfile` server action @@ -36,6 +36,8 @@ last_verified: 2026-04-17 - The `x-pathname` header is set upstream by the proxy (`src/proxy.ts`) so the server component can see the original requested URL; it falls back to `/` when absent. - `shared-actions/user.ts` is marked `'use server'` at the top of the file — all exports are server actions. - Shared actions re-validate auth inside each action (`auth.api.getSession(...)`) — they do not trust the caller. +- `getCurrentUserProfile` takes **no parameters**. The MP `User_GUID` is derived from the session inside the action. A caller-supplied GUID would be a live IDOR: server actions are caller-shaped POST endpoints, so any authenticated MP user could have read another user's contact details, roles, and user groups. +- The guard keys on a non-empty-string `session.user.userGuid` (declared `required: true` in `src/lib/auth.ts`), not `session.user.id` — the latter is Better Auth's internal ID and its presence does not prove an MP identity exists. ## API / Interface @@ -65,19 +67,28 @@ export async function AuthWrapper({ children }: { children: React.ReactNode }) { redirect(`${signinUrl.pathname}${signinUrl.search}`); } + // A session without a userGuid is unusable: every MP lookup keys off userGuid, + // and without it the header avatar/menu never renders — which leaves the user + // with no way to even sign out (the trap behind the better-auth 1.6 regression). + // Route these broken sessions to a recovery page that CAN sign them out, + // rather than rendering a dead app. /session-error lives outside the (web) + // route group, so it is not wrapped by AuthWrapper and cannot redirect-loop. + const userGuid = (session.user as { userGuid?: string | null }).userGuid; + if (!userGuid) { + redirect("/session-error"); + } + return <>{children}; } ``` ### `getCurrentUserProfile` -Source: `src/components/shared-actions/user.ts:8` +Source: `src/components/shared-actions/user.ts:25` ```typescript -export async function getCurrentUserProfile( - id: string -): Promise +export async function getCurrentUserProfile(): Promise ``` -Implementation: +Implementation (docstring elided — see source for the IDOR rationale): ```typescript 'use server'; @@ -86,13 +97,13 @@ import { MPUserProfile } from "@/lib/providers/ministry-platform/types"; import { UserService } from '@/services/userService'; import { headers } from 'next/headers'; -export async function getCurrentUserProfile(id: string): Promise { +export async function getCurrentUserProfile(): Promise { const session = await auth.api.getSession({ headers: await headers() }); - if (!session?.user?.id) throw new Error('Unauthorized'); + const userGuid = (session?.user as Record | undefined)?.userGuid; + if (typeof userGuid !== 'string' || userGuid.length === 0) throw new Error('Unauthorized'); const userService = await UserService.getInstance(); - const userProfile = await userService.getUserProfile(id); - return userProfile; + return userService.getUserProfile(userGuid); } ``` @@ -101,30 +112,36 @@ export async function getCurrentUserProfile(id: string): Promise`, call `next/navigation` `redirect()` (which throws internally to abort rendering) - 4. On valid session, render `<>{children}` + 4. On a session with no `userGuid`, `redirect("/session-error")` — that route sits outside the `(web)` group, so it is not itself wrapped and cannot loop + 5. On valid session, render `<>{children}` - **`getCurrentUserProfile` flow** - 1. Re-validate session via `auth.api.getSession()` — if no `session.user.id`, throw `Unauthorized` + 1. Re-validate session via `auth.api.getSession()` — if `session.user.userGuid` is not a non-empty string, throw `Unauthorized` 2. Await `UserService.getInstance()` (async singleton) - 3. Delegate to `userService.getUserProfile(id)` and return the `MPUserProfile` (or `undefined`) + 3. Delegate to `userService.getUserProfile(userGuid)` and return the `MPUserProfile` (or `undefined`) ## Shared Actions catalog | Export | File | Purpose | |---|---|---| -| `getCurrentUserProfile(id)` | `src/components/shared-actions/user.ts:8` | Fetch the current user's MP profile (`MPUserProfile`) by `User_GUID`; throws `Unauthorized` if no session. Backed by `UserService.getUserProfile`. | +| `getCurrentUserProfile()` | `src/components/shared-actions/user.ts:25` | Fetch the **calling** user's MP profile (`MPUserProfile`); `User_GUID` comes from the session, never from a parameter. Throws `Unauthorized` if the session has no `userGuid`. Backed by `UserService.getUserProfile`. | Guidelines (verbatim from `src/components/shared-actions/README.md`): - Place actions here when they are **used by multiple components across different features**, provide **shared utility**, or handle **cross-cutting concerns**. - Keep actions **co-located** when they are feature-specific or tightly coupled to a single feature's logic. ## Tests -- `src/components/layout/auth-wrapper.test.tsx` — 4 cases: +- `src/components/layout/auth-wrapper.test.tsx` — 6 cases: - redirects with `callbackUrl` from `x-pathname` - falls back to `/` when `x-pathname` is missing - preserves URL-encoded query params through the redirect + - redirects to `/session-error` when `userGuid` is absent + - redirects to `/session-error` when `userGuid` is `null` - returns children when authenticated -- `src/components/shared-actions/user.test.ts` — 3 cases: - - passes `id` through to `UserService.getUserProfile` and returns the profile +- `src/components/shared-actions/user.test.ts` — 6 cases: + - looks the profile up with the session's `userGuid` and returns it + - ignores a caller-forged argument (cast through `unknown`) and still uses the session GUID + - throws `Unauthorized` when the session has no `userGuid` + - throws `Unauthorized` when `userGuid` is an empty string - throws `Unauthorized` when `auth.api.getSession()` returns `null` - propagates service-layer errors @@ -135,6 +152,7 @@ Both test files use `vi.hoisted()` to share mock references (required pattern - **`redirect()` throws.** `next/navigation` `redirect()` aborts rendering by throwing a magic error. Do not wrap in try/catch; do not add code after the redirect call expecting it to run on the unauthenticated branch. - **`callbackUrl` relies on `x-pathname`.** If a route bypasses the proxy (or a future proxy matcher excludes it), `x-pathname` will be missing and unauthenticated users land on `/` after sign-in. Verify proxy matcher coverage in `src/proxy.ts` when adding new protected routes. - **Shared actions must re-validate auth.** `getCurrentUserProfile` calls `auth.api.getSession()` itself rather than trusting caller context. Any new action added here must do the same (see `../auth/README.md` for session access patterns). +- **Never re-add an identity parameter.** `getCurrentUserProfile` is a CLAUDE.md rule-12 carve-out from the `AuthorizationService` gate on the grounds that it returns only the caller's own profile. That justification holds only because there is no GUID argument to forge; adding one re-opens the IDOR. ## Related docs - `../auth/README.md` — Better Auth session shape, `session.user.userGuid` vs `session.user.id` diff --git a/.claude/references/contexts/user-provider.md b/.claude/references/contexts/user-provider.md index 6e00444..204692d 100644 --- a/.claude/references/contexts/user-provider.md +++ b/.claude/references/contexts/user-provider.md @@ -5,26 +5,26 @@ type: reference applies_to: [src/contexts/user-context.tsx, src/contexts/user-context.test.tsx, src/app/providers.tsx] symbols: [UserProvider, useUser, MPUserProfile] related: [session.md, ../auth/README.md, ../services/README.md] -last_verified: 2026-04-17 +last_verified: 2026-09-13 --- ## Purpose -Client context that watches the Better Auth session, extracts `userGuid` (OIDC `sub` → MP `User_GUID`), and loads the enriched `MPUserProfile` (roles + groups) via the `getCurrentUserProfile` server action. +Client context that watches the Better Auth session, extracts `userGuid` (OIDC `sub` → MP `User_GUID`), and loads the enriched `MPUserProfile` (roles + groups) via the `getCurrentUserProfile` server action. `userGuid` gates *whether* the load fires; the action re-derives it server-side and takes no argument. ## Files - `src/contexts/user-context.tsx` — provider + hook + error handling - `src/contexts/user-context.test.tsx` — 6 test cases covering load/error/refresh/missing-guid - `src/app/providers.tsx` — mounts `` at the app shell -- `src/components/shared-actions/user.ts` — `getCurrentUserProfile(id)` server action -- `src/services/userService.ts:81` — `UserService.getUserProfile(id)` downstream lookup +- `src/components/shared-actions/user.ts` — `getCurrentUserProfile()` server action (no parameters; derives the GUID from the session) +- `src/services/userService.ts:81` — `UserService.getUserProfile(guid)` downstream lookup - `src/lib/providers/ministry-platform/types/user-profile.types.ts` — `MPUserProfile` shape ## Key concepts - **Client-side profile load** — MP profile is fetched after mount, not injected by the server. Page renders before `userProfile` is available (`isLoading` starts `true`). - **`userGuid` is the key** — `session.user.id` is Better Auth's internal ID; `session.user.userGuid` is the MP `User_GUID`. Only `userGuid` is used for MP lookups (`user-context.tsx:27-29`). -- **Effect gating** — the effect only fires load when `!isPending && userGuid`; the else-branch (`!isPending && !session`) clears state. If the session exists but has no `userGuid`, **neither branch runs** (`user-context.tsx:51-58`; test at `user-context.test.tsx:88-99`). +- **Effect gating** — the effect only fires load when `!isPending && userGuid`; the else-branch (`!isPending && !session`) clears state. If the session exists but has no `userGuid`, **neither branch runs** (`user-context.tsx:51-59`; test at `user-context.test.tsx:88-99`). - **Error state is local** — `getCurrentUserProfile` rejections are caught; `error` is exposed on context, `userProfile` is reset to `null`, `isLoading` ends `false` (`user-context.tsx:43-48`). -- **`useUser` throws outside a provider** — guard at `user-context.tsx:78-80`. +- **`useUser` throws outside a provider** — guard at `user-context.tsx:79-81`. ## API / Interface @@ -64,7 +64,7 @@ export interface MPUserProfile { - `useEffect` fires `loadUserProfile()` when `!isPending && userGuid` truthy. - `loadUserProfile`: - Sets `isLoading=true`, `error=null`. - - Calls `getCurrentUserProfile(userGuid)`. + - Calls `getCurrentUserProfile()` — no argument; the action re-derives the GUID from the server-side session. - On success: `setUserProfile(profile ?? null)`. - On failure: `setError(Error)`, `setUserProfile(null)`. - `finally` block sets `isLoading=false`. @@ -92,7 +92,7 @@ export function Providers({ children }: ProvidersProps) { } ``` -Consumer hook (from `src/contexts/user-context.tsx:76-82`): +Consumer hook (from `src/contexts/user-context.tsx:77-83`): ```typescript export function useUser() { @@ -112,10 +112,10 @@ UserProvider mount └─ userGuid = session.user.userGuid └─ useEffect [!isPending && userGuid] └─ loadUserProfile() - └─ getCurrentUserProfile(userGuid) (server action: src/components/shared-actions/user.ts) - └─ auth.api.getSession({ headers }) (re-verifies session server-side) + └─ getCurrentUserProfile() (server action: src/components/shared-actions/user.ts:25) + └─ auth.api.getSession({ headers }) (re-verifies session; throws Unauthorized unless user.userGuid is a non-empty string) └─ UserService.getInstance() - └─ UserService.getUserProfile(id) (src/services/userService.ts:81) + └─ UserService.getUserProfile(userGuid) (src/services/userService.ts:81 — GUID from the session, never the caller) ├─ MP getTableRecords dp_Users (filter: User_GUID = '...') ├─ MP getTableRecords dp_User_Roles (filter: User_ID = ...) └─ MP getTableRecords dp_User_User_Groups (filter: User_ID = ...) @@ -128,16 +128,16 @@ UserProvider mount | Test | Line | Asserts | |---|---|---| | throws outside provider | 34 | `useUser()` without `` throws | -| loads profile when session has userGuid | 47 | `getCurrentUserProfile` called with guid, profile set, `error` null | +| loads profile when session has userGuid | 47 | `getCurrentUserProfile` called with **no arguments**, profile set, `error` null | | null profile when no session | 72 | `data: null` → `userProfile=null`, action not called | | no fetch when session lacks userGuid | 88 | `data: { user: { id } }` (no `userGuid`) → action not called | | handles profile load error | 101 | rejected promise → `error` set, `userProfile=null` | | `refreshUserProfile` re-fetches | 119 | second call returns updated profile; action called twice | ## Gotchas -- Reading `useUser().userProfile` before load returns `null` — gate UI on `isLoading` (inline; `user-context.tsx:24,51-58`). -- Consumer must be a client component and wrapped by `` — the hook throws otherwise (`user-context.tsx:78-80`). -- Session without `userGuid` leaves `isLoading` at its previous value — if the session switches to "missing guid" mid-lifecycle, neither branch of the effect clears it (`user-context.tsx:51-58`). Prefer `useAppSession` + explicit checks in components that need to distinguish "loading" vs "guid missing". +- Reading `useUser().userProfile` before load returns `null` — gate UI on `isLoading` (inline; `user-context.tsx:24,51-59`). +- Consumer must be a client component and wrapped by `` — the hook throws otherwise (`user-context.tsx:79-81`). +- Session without `userGuid` leaves `isLoading` at its previous value — if the session switches to "missing guid" mid-lifecycle, neither branch of the effect clears it (`user-context.tsx:51-59`). Prefer `useAppSession` + explicit checks in components that need to distinguish "loading" vs "guid missing". - Direct import from `@/lib/auth-client` bypasses the `useAppSession` wrapper — `UserProvider` itself does this (`user-context.tsx:22`) because it needs `isPending` which `useAppSession` drops. ## Related docs diff --git a/.claude/references/data-flow/call-graphs.md b/.claude/references/data-flow/call-graphs.md index a64c5de..630f3ea 100644 --- a/.claude/references/data-flow/call-graphs.md +++ b/.claude/references/data-flow/call-graphs.md @@ -160,12 +160,12 @@ last_verified: 2026-04-17 2. `src/contexts/user-context.tsx:22` — `authClient.useSession()` (reactive subscription to JWT cookie cache). 3. `src/contexts/user-context.tsx:29` — derive `userGuid = (session?.user as { userGuid?: string } | undefined)?.userGuid`. 4. `src/contexts/user-context.tsx:51-58` — `useEffect` fires when `!isPending && userGuid`: calls `loadUserProfile()` (line 53). -5. `src/contexts/user-context.tsx:31-49` — `loadUserProfile` sets `isLoading=true`, calls `getCurrentUserProfile(userGuid)` (line 41). -6. `src/components/shared-actions/user.ts:8` — server action `getCurrentUserProfile(id)`. -7. `src/components/shared-actions/user.ts:9-10` — `auth.api.getSession({ headers: await headers() })`; throws `'Unauthorized'` if no `session.user.id`. -8. `src/components/shared-actions/user.ts:12-13` — `UserService.getInstance()` → `userService.getUserProfile(id)`. +5. `src/contexts/user-context.tsx:31-49` — `loadUserProfile` sets `isLoading=true`, calls `getCurrentUserProfile()` (line 41) with no arguments. +6. `src/components/shared-actions/user.ts:25` — server action `getCurrentUserProfile()`; takes no parameters, so a caller cannot name another user (IDOR). +7. `src/components/shared-actions/user.ts:26-28` — `auth.api.getSession({ headers: await headers() })`, then derive `userGuid` from `session.user`; throws `'Unauthorized'` unless it is a non-empty string. +8. `src/components/shared-actions/user.ts:30-31` — `UserService.getInstance()` → `userService.getUserProfile(userGuid)`. 9. `src/services/userService.ts:81-110` — runs 3 MP queries: - - `mp.getTableRecords('dp_Users', { filter: "User_GUID = ''", select: "User_ID, User_GUID, Contact_ID_TABLE.First_Name, ..., Contact_ID_TABLE.dp_fileUniqueId AS Image_GUID", top: 1 })` at lines 82-87. + - `mp.getTableRecords('dp_Users', { filter: "User_GUID = ''", select: "User_ID, User_GUID, Contact_ID_TABLE.First_Name, ..., Contact_ID_TABLE.dp_fileUniqueId AS Image_GUID", top: 1 })` at lines 86-91. - `Promise.all([dp_User_Roles fetch, dp_User_User_Groups fetch])` at lines 92-103, keyed by the numeric `User_ID` from the first query. 10. `src/services/userService.ts:105-109` — returns `{ ...profile, roles: string[], userGroups: string[] }`. 11. `src/contexts/user-context.tsx:42` — `setUserProfile(profile ?? null)`. @@ -176,7 +176,7 @@ last_verified: 2026-04-17 **Error paths:** - Server action throws → caught at `src/contexts/user-context.tsx:43-46` → sets `error` state, `userProfile = null`. -- No `session.user.id` → `Error('Unauthorized')` at `src/components/shared-actions/user.ts:10`. +- Session `userGuid` missing or empty → `Error('Unauthorized')` at `src/components/shared-actions/user.ts:28`. (Not keyed on `session.user.id` — that is Better Auth's internal ID and does not prove an MP identity.) - Profile row missing → `userProfile = undefined` returned at `src/services/userService.ts:90`; normalized to `null` in context (line 42). **Return shape:** `MPUserProfile | null` in context state (with `roles: string[]`, `userGroups: string[]`). diff --git a/.claude/references/data-flow/error-catalog.md b/.claude/references/data-flow/error-catalog.md index 9f55dfb..04719fe 100644 --- a/.claude/references/data-flow/error-catalog.md +++ b/.claude/references/data-flow/error-catalog.md @@ -53,10 +53,10 @@ All MP REST errors originate in `src/lib/providers/ministry-platform/utils/http- | `Error('Invalid GUID format: ${value}')` | `src/lib/validation.ts:6` (`validateGuid`) | server-action catch (e.g., `userService` caller) | component catches → `setError` | no | | `Error('Expected positive integer, got: ${value}')` | `src/lib/validation.ts:13` (`validatePositiveInt`) | service callsite try/catch propagates to action | component catches → `setError` | no | | `Error('Invalid column name: ${value}')` | `src/lib/validation.ts:20` (`validateColumnName`) | `toolService.resolveContactIds` callsite → action | component catches → `setError` | no | -| `Error('Unauthorized')` | `src/components/address-labels/actions.ts:30`, `src/components/dev-panel/panels/selection-actions.ts:18`, `src/components/dev-panel/panels/contact-records-actions.ts:15`, `src/components/dev-panel/panels/user-tools-actions.ts:12`, `src/components/field-management/actions.ts:10`, `src/components/group-wizard/actions.ts:19`, `src/components/shared-actions/user.ts:10`, `src/components/template-editor/actions.ts:9` | component `try/catch` → `setError` (or `ActionError` shape for group-wizard) | toast / inline error | no | -| `Error('Unauthorized - Missing user session data')` (deploy-tool, user-tools variants) | `src/components/dev-panel/panels/deploy-tool-actions.ts:19`, `src/components/dev-panel/panels/user-tools-actions.ts:12` | component catch → `setError` | as above | no | -| `Error('User GUID not found in session')` | `src/components/address-labels/actions.ts:36`, `src/components/dev-panel/panels/selection-actions.ts:21`, `src/components/dev-panel/panels/user-tools-actions.ts:17`, `src/components/group-wizard/actions.ts:25` | component catch → `setError` | toast / inline | no | -| `Error('Deploy Tool is not available in production.')` | `src/components/dev-panel/panels/deploy-tool-actions.ts:15` | deploy-tool panel catch → `setError` | "production" warning to user | no | +| `Error('Unauthorized')` | `src/components/shared-actions/user.ts:28` — session has no non-empty `userGuid` | `src/contexts/user-context.tsx:43-46` try/catch → `error` state | silent (consumers of `useUser()` inspect `error`) | no | +| `UnauthorizedError('Not authorized')` (`code: "UNAUTHORIZED"`) | `src/services/authorizationService.ts:186` (`requireSecurityRole`) — the gate every feature action and service method calls | component `try/catch` → `setError` (or `ActionError` shape for group-wizard) | toast / inline error | yes — `console.warn` of `{table, operation, reason, userId}` only (identifiers/shape, never record content) | +| `Error('Unauthorized - Missing user session data')` | `src/components/dev-panel/panels/require-dev-session.ts:34` — single guard shared by every dev-panel action | component catch → `setError` | as above | no | +| ``Error(`${featureLabel} is not available in production.`)`` | `src/components/dev-panel/panels/require-dev-session.ts:30` (`featureLabel` defaults to `"Dev panel"`; deploy-tool passes `"Deploy Tool"`) | dev-panel / deploy-tool panel catch → `setError` | "production" warning to user | no | | `Error('MJML source must be between 1 and 512000 characters')` | `src/components/template-editor/actions.ts:19` | editor dialogs (`editor-code-dialog.tsx`, `editor-export-dialog.tsx`) catch | inline error state | no | | Zod `ZodError` via `zodResolver` (group wizard) | `src/components/group-wizard/schema.ts` (`groupWizardSchema`) | RHF `form.trigger(...)` in `src/app/(web)/tools/groupwizard/group-wizard.tsx:104` | in-form field error messages | no | | `Error('Input must contain only digits')` (IMb) | `src/lib/imb-encoder.ts:323` | `src/lib/barcode-helpers.ts:48` **silent catch — falls through to POSTNET** | no user-facing error; label prints without IMb | no (silent) | @@ -70,7 +70,7 @@ All MP REST errors originate in `src/lib/providers/ministry-platform/utils/http- | Type | Thrown at | Caught at | User-facing | Logged | |---|---|---|---|---| -| `Error('User not found')` | `src/services/userService.ts:76` (`getUserIdByGuid`) | server action (e.g., `user-tools-actions.ts`, `address-labels/actions.ts:36`) propagates → component catch | inline error / toast | no | +| `Error('User not found')` | `src/services/userService.ts:76` (`getUserIdByGuid`) | caller propagates → component catch | inline error / toast | no | | `Error('Tool Name is required')` | `src/services/toolService.ts:248` (`deployTool` guard) | deploy-tool UI catch → `setError` | inline error | no | | `Error('Launch Page is required')` | `src/services/toolService.ts:249` | deploy-tool UI | inline | no | | `Error('Tool Name must be 30 characters or fewer')` | `src/services/toolService.ts:250` | deploy-tool UI | inline | no | @@ -92,7 +92,7 @@ These do NOT throw to the component; server action returns `{ success: false, er | PDF render failure (`@react-pdf/renderer` `toBlob`) | throws inside try at `src/components/address-labels/actions.ts:157` | `:162-168` wraps as `{success:false, error: message}` | inline error | yes, `console.error('generateLabelPdf error:', error)` | | Docx render failure (`Packer.toBuffer`) | throws inside try at `src/components/address-labels/actions.ts:190` | `:194-200` wraps as envelope | inline error | yes, `console.error('generateLabelDocx error:', error)` | | Docxtemplater render error (tag mismatch) | throws inside `doc.render` at `src/components/address-labels/actions.ts:273` | `:279-286` wraps; if message includes `'tag'` returns prettier error | inline error | yes, `console.error('mergeTemplate error:', error)` | -| Profile fetch failure in `UserProvider` | `src/components/shared-actions/user.ts:10` (`Unauthorized`) or downstream MP error from `UserService.getUserProfile` | `src/contexts/user-context.tsx:43-46` try/catch sets `error` state | silent (consumers of `useUser()` inspect `error`) | no | +| Profile fetch failure in `UserProvider` | `src/components/shared-actions/user.ts:28` (`Unauthorized` — session has no `userGuid`) or downstream MP error from `UserService.getUserProfile` | `src/contexts/user-context.tsx:43-46` try/catch sets `error` state | silent (consumers of `useUser()` inspect `error`) | no | | `Error("useUser must be used within a UserProvider")` | `src/contexts/user-context.tsx:79` | uncaught — surfaces to nearest React error boundary | white-screen error unless a boundary is present | no | | `Error("useFormField should be used within ")` | `src/components/ui/form.tsx:53` | uncaught — React error boundary | same as above | no | | "Invalid JSON data…" (template-editor import) | `JSON.parse` throws at `src/components/template-editor/editor-import-dialog.tsx:34` | `:36` silent catch → `setError('Invalid JSON data…')` | inline error | no | diff --git a/.claude/references/security/README.md b/.claude/references/security/README.md index 9dbea02..0b4e1bf 100644 --- a/.claude/references/security/README.md +++ b/.claude/references/security/README.md @@ -82,7 +82,8 @@ data. Each documents why **in-file**. Adding a fifth needs the same justification, in the file, in writing. - `components/layout/auth-wrapper.tsx` — it *is* the session gate -- `components/shared-actions/user.ts` — the user's own profile +- `components/shared-actions/user.ts` — the user's own profile; enforced by the + signature (no parameter to forge), not just asserted - `components/shared-actions/domain.ts` — the domain-wide time zone (one string) - `components/dev-panel/panels/require-dev-session.ts` — dev-only (`NODE_ENV !== "production"`), and the services it calls gate anyway diff --git a/.claude/references/testing/inventory.md b/.claude/references/testing/inventory.md index c90c613..148caf5 100644 --- a/.claude/references/testing/inventory.md +++ b/.claude/references/testing/inventory.md @@ -51,7 +51,7 @@ All 37 test files grouped by area. Totals from facts snapshot: **37 files / 507 | `src/components/dev-panel/panels/user-tools-actions.test.ts` | Authorization checks, session validation | | `src/components/layout/auth-wrapper.test.tsx` | `AuthWrapper` render gating based on session | | `src/components/user-menu/actions.test.ts` | Sign-out action, OIDC logout redirect | -| `src/components/shared-actions/user.test.ts` | `getCurrentUserProfile` delegation | +| `src/components/shared-actions/user.test.ts` | `getCurrentUserProfile` session-derived GUID lookup, `Unauthorized` on missing/empty `userGuid`, IDOR regression (caller-forged argument ignored), error propagation | ## Core lib (8 files) diff --git a/README.md b/README.md index e705e2f..04f079f 100644 --- a/README.md +++ b/README.md @@ -613,7 +613,7 @@ Alert, Alert Dialog, Avatar, Badge, Breadcrumb, Button, Card, Checkbox, Command, - **template-editor/** — 12 components for visual template editing with GrapesJS - **user-menu/** — User dropdown with profile display and OIDC sign-out action - **dev-panel/** — Unified developer overlay (localhost-only) showing parsed URL params, MP selection data, contact records, and authorized tools -- **shared-actions/** — Cross-feature server actions (`getCurrentUserProfile`) +- **shared-actions/** — Cross-feature server actions (`getCurrentUserProfile()` — returns the *calling* user's MP profile; the `User_GUID` comes from the session, never from a parameter) All components use kebab-case file naming, PascalCase component names, and named exports with barrel index files. @@ -670,7 +670,7 @@ npm run test:coverage # With coverage report | User Service | `userService.test.ts` | Profile with roles/groups, parallel queries | | User Tools Panel | `user-tools-actions.test.ts` | Authorization checks, session validation | | User Menu | `actions.test.ts` | Sign-out action, OIDC logout redirect | -| Shared Actions | `user.test.ts` | getCurrentUserProfile delegation | +| Shared Actions | `user.test.ts` | getCurrentUserProfile session-derived GUID, unauthorized paths, IDOR regression | | Session Context | `session-context.test.tsx` | useAppSession hook wrapper | | IMb Encoder | `imb-encoder.test.ts` | USPS Intelligent Mail barcode encoding | | POSTNET Encoder | `postnet-encoder.test.ts` | POSTNET barcode encoding | diff --git a/src/components/address-labels/actions.test.ts b/src/components/address-labels/actions.test.ts index f29276d..c88ea10 100644 --- a/src/components/address-labels/actions.test.ts +++ b/src/components/address-labels/actions.test.ts @@ -5,7 +5,6 @@ const mockGetSelectionRecordIds = vi.hoisted(() => vi.fn()); const mockGetAddressesForContacts = vi.hoisted(() => vi.fn()); const mockGetAddressForContact = vi.hoisted(() => vi.fn()); const mockToBlob = vi.hoisted(() => vi.fn()); -const mockGetUserIdByGuid = vi.hoisted(() => vi.fn()); const mockDocxtemplaterRender = vi.hoisted(() => vi.fn()); const mockDocxtemplaterGetZip = vi.hoisted(() => vi.fn()); @@ -56,14 +55,6 @@ vi.mock('@/services/toolService', () => ({ }, })); -vi.mock('@/services/userService', () => ({ - UserService: { - getInstance: vi.fn().mockResolvedValue({ - getUserIdByGuid: mockGetUserIdByGuid, - }), - }, -})); - vi.mock('@/services/addressLabelService', () => ({ AddressLabelService: { getInstance: vi.fn().mockResolvedValue({ diff --git a/src/components/address-labels/sample-template.ts b/src/components/address-labels/sample-template.ts index dfa1e2d..d888371 100644 --- a/src/components/address-labels/sample-template.ts +++ b/src/components/address-labels/sample-template.ts @@ -1,6 +1,7 @@ 'use server'; import { Document, Paragraph, TextRun, Packer, PageBreak } from 'docx'; +import { AuthorizationService } from '@/services/authorizationService'; /** * Generates a sample .docx template with merge tokens pre-placed @@ -12,6 +13,17 @@ import { Document, Paragraph, TextRun, Packer, PageBreak } from 'docx'; * Returns base64-encoded .docx content. */ export async function generateSampleTemplate(): Promise { + // Authorize, don't just authenticate (CLAUDE.md rule 12). This action reads no + // MP data — it builds a static .docx from constants — so it leaks nothing by + // itself. It is gated anyway rather than documented as a carve-out: it is a + // capability of the address-label tool, so it takes that tool's gate, and the + // carve-out list stays at four entries instead of growing a fifth that a + // reader would have to re-reason about later. + await AuthorizationService.getInstance().requireSecurityRole({ + table: 'Contacts', + operation: 'read', + }); + const doc = new Document({ sections: [{ properties: { diff --git a/src/components/dev-panel/panels/deploy-tool-actions.test.ts b/src/components/dev-panel/panels/deploy-tool-actions.test.ts index 9511d60..1be75e6 100644 --- a/src/components/dev-panel/panels/deploy-tool-actions.test.ts +++ b/src/components/dev-panel/panels/deploy-tool-actions.test.ts @@ -5,13 +5,11 @@ const { mockListPages, mockListRoles, mockDeployTool, - mockGetUserIdByGuid, } = vi.hoisted(() => ({ mockGetSession: vi.fn(), mockListPages: vi.fn(), mockListRoles: vi.fn(), mockDeployTool: vi.fn(), - mockGetUserIdByGuid: vi.fn(), })); vi.mock('@/lib/auth', () => ({ @@ -36,14 +34,6 @@ vi.mock('@/services/toolService', () => ({ }, })); -vi.mock('@/services/userService', () => ({ - UserService: { - getInstance: vi.fn().mockResolvedValue({ - getUserIdByGuid: mockGetUserIdByGuid, - }), - }, -})); - import { deployToolAction, getDeployToolEnvStatusAction, @@ -92,8 +82,6 @@ describe('deploy-tool-actions', () => { mockListPages.mockReset(); mockListRoles.mockReset(); mockDeployTool.mockReset(); - mockGetUserIdByGuid.mockReset(); - mockGetUserIdByGuid.mockResolvedValue(42); vi.stubEnv('NODE_ENV', 'development'); }); diff --git a/src/components/field-management/actions.test.ts b/src/components/field-management/actions.test.ts index 30dbf7b..f345f9a 100644 --- a/src/components/field-management/actions.test.ts +++ b/src/components/field-management/actions.test.ts @@ -7,8 +7,6 @@ const { mockGetTableMetadata, mockUpdatePageFieldOrder, mockGetInstance, - mockGetUserIdByGuid, - mockUserGetInstance, } = vi.hoisted(() => ({ mockGetSession: vi.fn(), mockGetPages: vi.fn(), @@ -16,8 +14,6 @@ const { mockGetTableMetadata: vi.fn(), mockUpdatePageFieldOrder: vi.fn(), mockGetInstance: vi.fn(), - mockGetUserIdByGuid: vi.fn(), - mockUserGetInstance: vi.fn(), })); vi.mock('@/lib/auth', () => ({ @@ -70,10 +66,6 @@ vi.mock('@/services/fieldManagementService', () => ({ }, })); -vi.mock('@/services/userService', () => ({ - UserService: { getInstance: mockUserGetInstance }, -})); - import { fetchPages, fetchPageFieldData, savePageFieldOrder } from './actions'; const authedSession = { @@ -94,10 +86,6 @@ describe('field-management actions', () => { getTableMetadata: mockGetTableMetadata, updatePageFieldOrder: mockUpdatePageFieldOrder, }); - mockUserGetInstance.mockResolvedValue({ - getUserIdByGuid: mockGetUserIdByGuid, - }); - mockGetUserIdByGuid.mockResolvedValue(42); }); describe('fetchPages', () => { diff --git a/src/components/group-wizard/actions.test.ts b/src/components/group-wizard/actions.test.ts index 2efcc9a..5e3b46c 100644 --- a/src/components/group-wizard/actions.test.ts +++ b/src/components/group-wizard/actions.test.ts @@ -8,9 +8,7 @@ const { mockGetGroup, mockCreateGroup, mockUpdateGroup, - mockGetUserIdByGuid, mockGroupGetInstance, - mockUserGetInstance, } = vi.hoisted(() => ({ mockGetSession: vi.fn(), mockFetchAllLookups: vi.fn(), @@ -19,9 +17,7 @@ const { mockGetGroup: vi.fn(), mockCreateGroup: vi.fn(), mockUpdateGroup: vi.fn(), - mockGetUserIdByGuid: vi.fn(), mockGroupGetInstance: vi.fn(), - mockUserGetInstance: vi.fn(), })); vi.mock('@/lib/auth', () => ({ @@ -72,10 +68,6 @@ vi.mock('@/services/groupService', () => ({ GroupService: { getInstance: mockGroupGetInstance }, })); -vi.mock('@/services/userService', () => ({ - UserService: { getInstance: mockUserGetInstance }, -})); - import { fetchGroupWizardLookups, searchContacts, @@ -144,9 +136,6 @@ mockRequireSecurityRole.mockImplementation(async () => { createGroup: mockCreateGroup, updateGroup: mockUpdateGroup, }); - mockUserGetInstance.mockResolvedValue({ - getUserIdByGuid: mockGetUserIdByGuid, - }); }); describe('fetchGroupWizardLookups', () => { @@ -318,7 +307,6 @@ describe('createGroup', () => { mockGetSession.mockResolvedValueOnce({ user: { id: 'user-1', userGuid: '550e8400-e29b-41d4-a716-446655440000' }, }); - mockGetUserIdByGuid.mockResolvedValueOnce(42); mockCreateGroup.mockResolvedValueOnce({ Group_ID: 200, Group_Name: 'Test Group' }); const result = await createGroup(BASE_FORM); @@ -331,7 +319,6 @@ describe('createGroup', () => { mockGetSession.mockResolvedValueOnce({ user: { id: 'user-1', userGuid: '550e8400-e29b-41d4-a716-446655440000' }, }); - mockGetUserIdByGuid.mockResolvedValueOnce(42); mockCreateGroup.mockRejectedValueOnce(new Error('MP create failed')); const result = await createGroup(BASE_FORM); @@ -343,7 +330,6 @@ describe('createGroup', () => { mockGetSession.mockResolvedValueOnce({ user: { id: 'user-1', userGuid: '550e8400-e29b-41d4-a716-446655440000' }, }); - mockGetUserIdByGuid.mockResolvedValueOnce(42); mockCreateGroup.mockRejectedValueOnce('boom'); const result = await createGroup(BASE_FORM); @@ -381,7 +367,6 @@ describe('updateGroup', () => { mockGetSession.mockResolvedValueOnce({ user: { id: 'user-1', userGuid: '550e8400-e29b-41d4-a716-446655440000' }, }); - mockGetUserIdByGuid.mockResolvedValueOnce(42); mockUpdateGroup.mockResolvedValueOnce({ Group_ID: 100, Group_Name: 'Updated' }); const result = await updateGroup(100, BASE_FORM); @@ -394,7 +379,6 @@ describe('updateGroup', () => { mockGetSession.mockResolvedValueOnce({ user: { id: 'user-1', userGuid: '550e8400-e29b-41d4-a716-446655440000' }, }); - mockGetUserIdByGuid.mockResolvedValueOnce(42); mockUpdateGroup.mockRejectedValueOnce(new Error('MP update failed')); const result = await updateGroup(100, BASE_FORM); @@ -406,7 +390,6 @@ describe('updateGroup', () => { mockGetSession.mockResolvedValueOnce({ user: { id: 'user-1', userGuid: '550e8400-e29b-41d4-a716-446655440000' }, }); - mockGetUserIdByGuid.mockResolvedValueOnce(42); mockUpdateGroup.mockRejectedValueOnce('boom'); const result = await updateGroup(100, BASE_FORM); diff --git a/src/components/shared-actions/README.md b/src/components/shared-actions/README.md index e9c7c58..d0ba1d1 100644 --- a/src/components/shared-actions/README.md +++ b/src/components/shared-actions/README.md @@ -18,7 +18,7 @@ Keep actions with their component folder when: ## Examples **Shared Actions (place here):** -- `user.ts` - User profile operations used by contexts and components +- `user.ts` - Current-user profile lookup used by contexts and components - `auth.ts` - Authentication actions used across the app - `analytics.ts` - Analytics tracking used by multiple features - `notifications.ts` - Notification system used app-wide @@ -32,6 +32,11 @@ Keep actions with their component folder when: // Importing shared actions import { getCurrentUserProfile } from '@/components/shared-actions/user'; +// Returns the CALLING user's profile. It takes no arguments by design: a server +// action is a caller-shaped POST endpoint, so a user-id/GUID parameter would let +// any authenticated MP user read anyone else's profile (IDOR). Do not re-add one. +const profile = await getCurrentUserProfile(); + // Importing feature-specific actions import { myAction } from './actions'; // Within same folder ``` diff --git a/src/components/shared-actions/user.test.ts b/src/components/shared-actions/user.test.ts index 259d026..28ccd74 100644 --- a/src/components/shared-actions/user.test.ts +++ b/src/components/shared-actions/user.test.ts @@ -1,16 +1,14 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -const { mockGetUserProfile, mockGetSession, mockGetUserIdByGuid } = vi.hoisted(() => ({ +const { mockGetUserProfile, mockGetSession } = vi.hoisted(() => ({ mockGetUserProfile: vi.fn(), mockGetSession: vi.fn(), - mockGetUserIdByGuid: vi.fn(), })); vi.mock('@/services/userService', () => ({ UserService: { getInstance: vi.fn().mockResolvedValue({ getUserProfile: mockGetUserProfile, - getUserIdByGuid: mockGetUserIdByGuid, }), }, })); @@ -23,20 +21,22 @@ vi.mock('next/headers', () => ({ headers: vi.fn().mockResolvedValue(new Headers()), })); -import { getCurrentUserProfile, getCurrentUserIdFromSession } from './user'; +import { getCurrentUserProfile } from './user'; + +const SESSION_GUID = '550e8400-e29b-41d4-a716-446655440000'; describe('getCurrentUserProfile', () => { beforeEach(() => { vi.clearAllMocks(); mockGetSession.mockResolvedValue({ - user: { id: 'internal-id', userGuid: '550e8400-e29b-41d4-a716-446655440000' }, + user: { id: 'internal-id', userGuid: SESSION_GUID }, }); }); - it('should call UserService with correct ID', async () => { + it("should look up the profile using the session's userGuid", async () => { const mockProfile = { User_ID: 1, - User_GUID: '550e8400-e29b-41d4-a716-446655440000', + User_GUID: SESSION_GUID, Contact_ID: 100, First_Name: 'John', Nickname: 'Johnny', @@ -49,67 +49,48 @@ describe('getCurrentUserProfile', () => { }; mockGetUserProfile.mockResolvedValueOnce(mockProfile); - const result = await getCurrentUserProfile('550e8400-e29b-41d4-a716-446655440000'); + const result = await getCurrentUserProfile(); - expect(mockGetUserProfile).toHaveBeenCalledWith('550e8400-e29b-41d4-a716-446655440000'); + expect(mockGetUserProfile).toHaveBeenCalledWith(SESSION_GUID); expect(result).toEqual(mockProfile); }); - it('should throw when not authenticated', async () => { - mockGetSession.mockResolvedValueOnce(null); + it('should ignore any caller-supplied argument and use the session GUID', async () => { + mockGetUserProfile.mockResolvedValueOnce(undefined); - await expect(getCurrentUserProfile('550e8400-e29b-41d4-a716-446655440000')) - .rejects.toThrow('Unauthorized'); - }); + // The action takes no parameters; a caller forging one must not influence the lookup. + await (getCurrentUserProfile as unknown as (id: string) => Promise)( + 'attacker-supplied-guid' + ); - it('should propagate errors', async () => { - mockGetUserProfile.mockRejectedValueOnce(new Error('Service error')); - - await expect(getCurrentUserProfile('550e8400-e29b-41d4-a716-446655440000')) - .rejects.toThrow('Service error'); + expect(mockGetUserProfile).toHaveBeenCalledWith(SESSION_GUID); + expect(mockGetUserProfile).not.toHaveBeenCalledWith('attacker-supplied-guid'); }); -}); -describe('getCurrentUserIdFromSession', () => { - beforeEach(() => { - vi.clearAllMocks(); - }); + it('should throw when the session has no userGuid', async () => { + mockGetSession.mockResolvedValueOnce({ user: { id: 'internal-id' } }); - it('resolves the MP User_ID via getUserIdByGuid', async () => { - mockGetUserIdByGuid.mockResolvedValueOnce(42); + await expect(getCurrentUserProfile()).rejects.toThrow('Unauthorized'); + expect(mockGetUserProfile).not.toHaveBeenCalled(); + }); - const result = await getCurrentUserIdFromSession({ - user: { id: 'internal-id', userGuid: '550e8400-e29b-41d4-a716-446655440000' }, - } as unknown as Awaited>); + it('should throw when the session userGuid is an empty string', async () => { + mockGetSession.mockResolvedValueOnce({ user: { id: 'internal-id', userGuid: '' } }); - expect(mockGetUserIdByGuid).toHaveBeenCalledWith('550e8400-e29b-41d4-a716-446655440000'); - expect(result).toBe(42); + await expect(getCurrentUserProfile()).rejects.toThrow('Unauthorized'); + expect(mockGetUserProfile).not.toHaveBeenCalled(); }); - it('throws when session is null', async () => { - await expect( - getCurrentUserIdFromSession( - null as unknown as Awaited> - ) - ).rejects.toThrow('User GUID not found in session'); - }); + it('should throw when not authenticated', async () => { + mockGetSession.mockResolvedValueOnce(null); - it('throws when session.user has no userGuid', async () => { - await expect( - getCurrentUserIdFromSession({ - user: { id: 'internal-id' }, - } as unknown as Awaited>) - ).rejects.toThrow('User GUID not found in session'); - expect(mockGetUserIdByGuid).not.toHaveBeenCalled(); + await expect(getCurrentUserProfile()).rejects.toThrow('Unauthorized'); + expect(mockGetUserProfile).not.toHaveBeenCalled(); }); - it('propagates errors from getUserIdByGuid', async () => { - mockGetUserIdByGuid.mockRejectedValueOnce(new Error('User not found')); + it('should propagate errors', async () => { + mockGetUserProfile.mockRejectedValueOnce(new Error('Service error')); - await expect( - getCurrentUserIdFromSession({ - user: { id: 'internal-id', userGuid: 'bogus' }, - } as unknown as Awaited>) - ).rejects.toThrow('User not found'); + await expect(getCurrentUserProfile()).rejects.toThrow('Service error'); }); }); diff --git a/src/components/shared-actions/user.ts b/src/components/shared-actions/user.ts index 6e3c822..7875442 100644 --- a/src/components/shared-actions/user.ts +++ b/src/components/shared-actions/user.ts @@ -5,32 +5,28 @@ import { MPUserProfile } from "@/lib/providers/ministry-platform/types"; import { UserService } from '@/services/userService'; import { headers } from 'next/headers'; -type BetterAuthSession = Awaited>; - -export async function getCurrentUserProfile(id: string): Promise { - const session = await auth.api.getSession({ headers: await headers() }); - if (!session?.user?.id) throw new Error('Unauthorized'); - - const userService = await UserService.getInstance(); - const userProfile = await userService.getUserProfile(id); - return userProfile; -} - /** - * Resolves the numeric MP `User_ID` for the given Better Auth session by looking up - * `session.user.userGuid` in `dp_Users`. Throws if the session has no user GUID. + * Returns the profile of the *calling* user only. + * + * Carve-out from the `AuthorizationService` gate (CLAUDE.md rule 12): this action + * exposes no MP data other than the caller's own profile, so there is no role to + * check beyond "is this a real MP identity". * - * This is the canonical way for server actions to obtain the `$userId` value that - * should be threaded through to MP write operations for audit attribution. + * The GUID is derived from the session and MUST NOT become a parameter again. + * Server actions are caller-shaped POST endpoints, so a `id`/`userGuid` argument + * here was a live IDOR: any authenticated MP user could read another user's + * contact details, roles, and user groups. Keeping it session-only makes the + * carve-out's "own profile" justification enforceable by the type signature. + * + * The guard keys on `userGuid` (declared `required: true` in `src/lib/auth.ts`), + * not `session.user.id` — the latter is Better Auth's internal ID and its presence + * does not prove an MP identity exists. */ -export async function getCurrentUserIdFromSession( - session: BetterAuthSession -): Promise { - const userGuid = (session?.user as Record | undefined)?.userGuid as - | string - | undefined; - if (!userGuid) throw new Error('User GUID not found in session'); +export async function getCurrentUserProfile(): Promise { + const session = await auth.api.getSession({ headers: await headers() }); + const userGuid = (session?.user as Record | undefined)?.userGuid; + if (typeof userGuid !== 'string' || userGuid.length === 0) throw new Error('Unauthorized'); const userService = await UserService.getInstance(); - return userService.getUserIdByGuid(userGuid); + return userService.getUserProfile(userGuid); } diff --git a/src/contexts/user-context.test.tsx b/src/contexts/user-context.test.tsx index c19785a..e799b11 100644 --- a/src/contexts/user-context.test.tsx +++ b/src/contexts/user-context.test.tsx @@ -66,7 +66,7 @@ describe('UserContext', () => { expect(result.current.userProfile).toEqual(mockProfile); expect(result.current.error).toBeNull(); - expect(mockGetCurrentUserProfile).toHaveBeenCalledWith('guid-123'); + expect(mockGetCurrentUserProfile).toHaveBeenCalledWith(); }); it('should set null profile when no session', async () => { diff --git a/src/contexts/user-context.tsx b/src/contexts/user-context.tsx index cd88cc7..967cae2 100644 --- a/src/contexts/user-context.tsx +++ b/src/contexts/user-context.tsx @@ -38,7 +38,7 @@ export function UserProvider({ children }: UserProviderProps) { try { setIsLoading(true); setError(null); - const profile = await getCurrentUserProfile(userGuid); + const profile = await getCurrentUserProfile(); setUserProfile(profile ?? null); } catch (err) { setError(err instanceof Error ? err : new Error("Failed to load user profile")); diff --git a/src/lib/auth.ts b/src/lib/auth.ts index 58f9483..cbaeb4b 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -126,7 +126,7 @@ export const userAdditionalFields = { * Extracts a usable MP `User_GUID` from an OIDC userinfo payload, or `null`. * * Exported for testing. Validates the shape because `sub` is the value every - * downstream MP lookup keys on — `UserService.getUserIdByGuid` already runs it + * downstream MP lookup keys on — `UserService.getUserProfile` already runs it * through `validateGuid`, so an unparseable `sub` would otherwise surface as a * mystery failure on the first MP call instead of a clean refusal at sign-in. */ diff --git a/src/services/userService.test.ts b/src/services/userService.test.ts index e140cf6..205b1e6 100644 --- a/src/services/userService.test.ts +++ b/src/services/userService.test.ts @@ -116,44 +116,4 @@ describe('UserService', () => { await expect(service.getUserProfile('b0000000-0000-0000-0000-000000000000')).rejects.toThrow('API error'); }); }); - - describe('getUserIdByGuid', () => { - it('should return User_ID for valid GUID', async () => { - mockGetTableRecords.mockResolvedValueOnce([{ User_ID: 42 }]); - - const service = await UserService.getInstance(); - const result = await service.getUserIdByGuid('550e8400-e29b-41d4-a716-446655440000'); - - expect(result).toBe(42); - expect(mockGetTableRecords).toHaveBeenCalledWith({ - table: 'dp_Users', - select: 'User_ID', - filter: "User_GUID = '550e8400-e29b-41d4-a716-446655440000'", - top: 1, - }); - }); - - it('should throw when user not found (empty array)', async () => { - mockGetTableRecords.mockResolvedValueOnce([]); - - const service = await UserService.getInstance(); - await expect( - service.getUserIdByGuid('550e8400-e29b-41d4-a716-446655440000') - ).rejects.toThrow('User not found'); - }); - - it('should throw when result is null', async () => { - mockGetTableRecords.mockResolvedValueOnce(null as unknown as []); - - const service = await UserService.getInstance(); - await expect( - service.getUserIdByGuid('550e8400-e29b-41d4-a716-446655440000') - ).rejects.toThrow('User not found'); - }); - - it('should throw for malformed GUID (via validateGuid)', async () => { - const service = await UserService.getInstance(); - await expect(service.getUserIdByGuid('not-a-guid')).rejects.toThrow('Invalid GUID format'); - }); - }); }); diff --git a/src/services/userService.ts b/src/services/userService.ts index dd5bd33..3c3e974 100644 --- a/src/services/userService.ts +++ b/src/services/userService.ts @@ -58,26 +58,6 @@ export class UserService { * @returns Promise - The user profile data from Ministry Platform * @throws Will throw an error if the Ministry Platform query fails */ - /** - * Looks up the Ministry Platform User_ID for a given User GUID - * - * @param guid - The User GUID to resolve - * @returns Promise - The numeric User_ID - * @throws Will throw an error if the user is not found - */ - public async getUserIdByGuid(guid: string): Promise { - const records = await this.mp!.getTableRecords<{ User_ID: number }>({ - table: 'dp_Users', - select: 'User_ID', - filter: `User_GUID = '${validateGuid(guid)}'`, - top: 1, - }); - if (!records || records.length === 0) { - throw new Error('User not found'); - } - return records[0].User_ID; - } - public async getUserProfile(id: string): Promise { const records = await this.mp!.getTableRecords({ table: "dp_Users",