feat(http-utils): allow RO admin write ops on resources they own

[ravverma]

Problem

The readOnlyAdminWrapper previously blocked all write operations for read-only admin users. This was overly restrictive: a read-only admin whose IMS org owns a site or organization should be able to mutate that resource — they just should not be able to touch resources belonging to other orgs.

Solution

route-utils.js — new extractRouteParams

Added a focused, standalone extractRouteParams(context, routeMap) function that returns the named path parameters extracted from the matched route pattern (e.g. { siteId: 'abc-123' } from PATCH /sites/:siteId). The existing resolveRouteCapability is left completely unchanged so the s2s wrapper is unaffected.

read-only-admin-wrapper.js — ownership check before blocking writes

For write actions, before returning 403 the wrapper now calls isOwnerOfResource(context, authInfo, params):

• If siteId is in the path params: fetches context.dataAccess.Site.findById(siteId), resolves the org, and calls authInfo.hasOrganization(org.getImsOrgId())
• If organizationId is in the path params: fetches context.dataAccess.Organization.findById(organizationId) and checks the same
• Fail-closed in all error cases: dataAccess absent, entity not found, no recognisable ID in the path, or any DB exception — the exception is logged via log.error and the request is blocked

Read paths are completely untouched.

context.data fallback for ID-less routes

Some write routes carry no resource ID in the path but pass it in the request body instead (e.g. POST /preflight/jobs sends siteId in the JSON payload, resolved via context.data.siteId). For these cases isOwnerOfResource falls back to context.data after finding no match in the route params:

const siteId = params.siteId ?? context.data?.siteId;
const organizationId = params.organizationId ?? context.data?.organizationId;

Path params always take precedence over body data when both are present. Routes that carry neither (e.g. POST /sites creating a new resource) still return false and are blocked.

Test coverage

route-utils.test.js — 8 new cases for extractRouteParams: exact match (no params), single param, multiple nested params, org param, no match, missing method/suffix/pathInfo.

read-only-admin-wrapper.test.js — 15 new cases under "write on owned resource":

• Owned site write (path param) → allowed + audit log emitted
• Unowned site write → 403
• Site not found → 403
• Site with no org → 403
• Owned org write (path param) → allowed
• Organization not found → 403
• Unowned org write → 403
• No ID in path and no context.data (e.g. POST /sites) → 403
• No dataAccess on context → 403 (fail-closed)
• DB lookup throws → 403 + log.error called with the error (fail-closed)
• Owned site write via context.data.siteId (no path param) → allowed
• Unowned site via context.data.siteId → 403
• Owned org write via context.data.organizationId (no path param) → allowed
• Path param takes precedence over context.data when both present
• Audit log emitted on allowed owned write

All 309 tests pass. Coverage: 100% lines/statements, 98.18% branches (threshold: 97%).

Checklist

• resolveRouteCapability untouched — s2s wrapper unaffected
• Fail-closed for every error path in the ownership check
• Exceptions logged before returning false (no silent failures)
• Ownership check is lazy — only runs for write routes, never for reads
• context.data fallback handles routes like POST /preflight/jobs with no ID in path
• Path params take precedence over body data when both are present
• No new package dependencies

🤖 Generated with Claude Code

[github-actions]

This PR will trigger a minor release when merged.

[ravverma]

Self reviewed with review kit

All three agents completed. Here's the consolidated review:

---

PR #1595 Review Summary

Critical (fix before merge)

[security] Body fallback activates on param name mismatch, not just "no params in route"
read-only-admin-wrapper.js:52-53

params.siteId ?? context.data?.siteId fires whenever params.siteId is absent — including when the route has a path param but it's named :id, :site, or anything other than :siteId. An RO admin who owns site A could call PATCH /sites/B (a route using :id) with body { siteId: "A" } and the ownership check would pass for A while the handler writes to B.

Fix: only fall back to context.data when the route had zero path params — i.e. gate on Object.keys(params).length === 0:

const siteId = Object.keys(params).length === 0
  ? context.data?.siteId
  : params.siteId;
const organizationId = Object.keys(params).length === 0
  ? context.data?.organizationId
  : params.organizationId;

---

Important (should fix)

[observability] evaluateFeatureFlag catch is completely silent
read-only-admin-wrapper.js:108-110 — LD failures block all RO admin access with zero log output. Add log.error before return false.

[observability] Missing dataAccess returns false with no log
read-only-admin-wrapper.js:46-48 — A misconfigured wrapper chain produces a flood of unexplained 403s. Add log.error with a message pointing at wrapper ordering.

[observability] No-ID-found case is silent and indistinguishable from ownership failure
read-only-admin-wrapper.js:80 — When neither path nor body has a recognisable ID, return false fires with no log. Operators can't tell if a 403 came from "user doesn't own this" vs "no ID was found at all". Add a log.warn.

[audit] Audit log omits resolved resource ID and whether body fallback was used
read-only-admin-wrapper.js:177-183 — The ro-admin-audit log entry doesn't record which siteId/organizationId was used to authorise a write, making post-incident reconstruction impossible. Include the resolved IDs and source ('path' vs 'body').

[reliability] extractRouteParams called outside any try-catch
read-only-admin-wrapper.js:162 — If it throws, the exception escapes the wrapper as an unhandled rejection rather than a controlled 403. Wrap it.

---

Suggestions (nice to have)

[test] getOrganization() throwing is not covered — the catch block is exercised via findById throwing but not from a second async step inside the same try block.

[test] Organization.findById throwing is not covered — only the site lookup throw path is tested; the org path hits the same catch but isn't explicitly exercised.

[test] dataAccess present but Site/Organization property absent not tested — the optional chaining dataAccess.Site?.findById silently returns undefined; no test exercises this with context.dataAccess = {}.

[docs] JSDoc should document that context.data must be the parsed body — if readOnlyAdminWrapper runs before the body parser in the .with() chain, body-fallback routes will always 403 with no diagnostic.

---

Strengths

• Fail-closed is correctly implemented and well-tested for the main paths
• Path params take precedence over body data via ?? — correct priority
• Ownership check is lazy (only runs on write routes)
• 309 tests, 100% lines/statements, 98.18% branches
• isOwnerOfResource catch block correctly logs the error before returning false
• extractRouteParams returns {} (never null) — prevents null-dereference downstream

---

[ravverma]

Pushed. Here's a summary of everything addressed from the review:

Critical (security)

• Body fallback now gates on Object.keys(params).length === 0 — it only activates when the matched route has no path params at all, closing the bypass where an attacker on a :id-named route could supply a body.siteId for a site they own

Observability / logging (4 gaps fixed)

• evaluateFeatureFlag catch now calls log.error with the exception before returning false
• isOwnerOfResource logs log.error when dataAccess is absent (with a pointer to wrapper ordering)
• isOwnerOfResource logs log.warn when no ID can be resolved from path or body
• Allowed writes now emit a dedicated ro-admin-write log entry including resolved IDs and idSource: 'path' | 'body'

Reliability

• extractRouteParams call wrapped in try/catch — exceptions now produce a controlled 403 + log.error instead of an unhandled rejection

New tests (9 added)

• log.error assertions on LD SDK throw and isFlagEnabledForIMSOrg throw
• log.error on missing dataAccess; log.warn on no-ID-found
• getOrganization() throwing (fail-closed + log)
• Organization.findById throwing (fail-closed + log)
• dataAccess present but Site property absent
• Security test: body siteId ignored when route uses :id param name
• ro-admin-write log assertions for both path and body ID source
• extractRouteParams throwing via esmock