feat: scope GET /opportunity to an AGENT caller's own agents

[ivannissimrch]

Description

fe 934 asks that NGO users only see opportunities belonging to their own organisation. Today GET /opportunity applies no caller-based scoping at all, so an NGO user sees every organisation's.

The issue says both "their own operator (their email domain)" and "for their own shelter", which give different lists. Nadav settled it on fe 950: "anyone from the same NGO/agent can see the opportunities there of that NGO/agent". So this scopes by agent. agent.organization_id is NULL for every agent on local and dev so that operator scoping would return an empty list for everyone today anyway.

GET /opportunity now resolves the caller's agent memberships server-side and restricts the result to those agents when the caller's role is AGENT. Coordinators and admins are unaffected.

Only ACTIVE memberships count. A PENDING membership is one a coordinator has not approved yet, so it grants nothing.

fe 950 is the frontend half and needs this to be safe to ship.

Related Issues

fe 934
fe 950

Changes

• getCallerAgentIds in src/server/utils/data/: a person's ACTIVE agent memberships, deduped
• GET /opportunity: when the caller is an AGENT, constrain where.agent.id
  to those IDs
• An AGENT with no ACTIVE membership gets an empty list rather than falling
  through to an unfiltered query, so the failure mode is closed, not open
• 6 tests in src/test/server/routes/opportunity-agent-scope.routes.test.ts

Checklist

• WITHIN THE SCOPE OF AN ISSUE; No unnecessary files included
• Tests added/updated
• Documentation updated
• CI passes

[arturasmckwcz]

Code review findings

Correctness

1. src/server/utils/pii/visible-persons.ts:68 — resolveCallerVisibility resolves an AGENT caller's agentIds without filtering on ACTIVE membership status, while the new getCallerAgentIds does filter on AgentMembershipStatus.ACTIVE. Two disagreeing definitions of "the agents this caller belongs to" now exist. A person with only a PENDING membership at Agent X gets zero opportunities from the newly-scoped list endpoint, but resolveCallerVisibility still treats them as an Agent X member for PII-masking elsewhere (GET /opportunity/:id, comments, contact PII) — contradicting this PR's own stated principle that a PENDING membership grants nothing.

2. src/server/routes/opportunity/opportunity.routes.ts:614 (mirrored at :439 for POST / and :725 for the contact check) — the AGENT ownership check via agentPersonRepository.findOneBy has no status: ACTIVE filter, unlike getCallerAgentIds. A person with a PENDING membership at an agent can still create/update opportunities for that agent, even though the read path now excludes PENDING members — a more permissive write path sitting right next to the newly-tightened read path.

3. src/server/routes/opportunity/opportunity.routes.ts:153 — GET /opportunity/:id has no agent-ownership scoping for AGENT callers at all. An AGENT scoped out of another org's opportunities in the list endpoint can still fetch that opportunity directly by ID, undercutting the stated security goal (fe#934).

4. src/server/routes/opportunity/legacy.routes.ts:199 — the legacy GET / opportunity endpoint is public: true (bypasses fastify.authenticate entirely) and returns all NEW/ACTIVE/SEARCHING opportunities unmasked, including PII, to unauthenticated callers. Pre-existing, but it fully defeats the purpose of the scoping just added to the sibling list endpoint.

5. src/server/routes/opportunity/opportunity-volunteer.routes.ts:42 — GET /:id/volunteer-linked has no agent-ownership check on opportunityId. An AGENT scoped out of an opportunity by the new list restriction can still call this endpoint directly with that opportunity's ID and see matched-volunteer data.

Test coverage

6. src/test/server/routes/opportunity-agent-scope.routes.test.ts:91 — otherAgentId = Number(owned[1].agentId) indexes into a GROUP BY result with no check that ≥2 distinct agents exist in test data. If fewer, owned[1] is undefined and .agentId throws in beforeAll, failing the whole suite with a cryptic TypeError instead of a clear precondition error.

7. src/server/routes/opportunity/opportunity.routes.ts:286 — the where.agent spread is written to compose with a filter getOpportunityWhere might someday set, but getOpportunityWhere never sets where.agent today, so this branch is untested/unreachable and the "scope wins over filter" test passes trivially. If filter.agentId support is added later with an incompatible shape, the AGENT scope could be silently bypassed without this test catching it.

Simplification / reuse

8. src/server/utils/data/get-caller-agent-ids.ts:1 — duplicates agent-membership resolution already present in resolveCallerVisibility instead of reusing/extending it (see finding 1 — the two have already diverged on ACTIVE-status handling), contrary to the repo's reuse-before-create rule.

Minor

9. src/server/utils/data/get-caller-agent-ids.ts:8 — if (!personId) return []; treats personId === 0 as absent. Not exploitable today (Person.id starts at 1), but a latent trap if a sequence were ever reset to 0.

10. src/server/routes/opportunity/opportunity.routes.ts:271 — getCallerAgentIds re-queries agent_person for the caller's agentIds in the handler, duplicating the query resolveCallerVisibility already runs in the preSerialization PII-masking hook for the same request — a redundant DB round trip on every AGENT list request.

---

🤖 Posted by Claude Code

[arturasmckwcz]

Automated review findings (Claude Code /code-review). This PR tightens agent/cross-org scoping on opportunities, but the scoping is inconsistently applied — a few gaps re-open the exposure it's meant to close. Inline comments cover issues on lines this PR touches; the following pre-existing code paths are not touched by this diff but are directly relevant to the scoping goal and should be checked in a follow-up:

1. src/server/routes/comment.ts:86 — GET /comment has no ownership/agent-scoping check at all. Any authenticated user (including an AGENT from an unrelated organisation) can read another agent's opportunity comment thread by numeric entityId. An AGENT-role caller who this PR now correctly 404s on GET /opportunity/<otherAgentOppId> can still call GET /comment?entityType=opportunity&entityId=<otherAgentOppId> and get the full comment thread (internal notes, potentially PII) for that opportunity — defeating the isolation this PR establishes elsewhere.

2. src/server/routes/volunteer/volunteer-opportunity.routes.ts:54 — GET /volunteer/opportunity has no role restriction or agent scoping. It's registered under a parent hook that only requires fastify.authenticate(), and builds its where purely from client query filters, unlike the now-fixed GET /opportunity. An AGENT-role caller can hit this endpoint instead and get an unscoped, cross-organisation opportunity list.

3. src/server/routes/opportunity/opportunity.routes.ts:522 — contactPersonId freezing (agent.agentPerson?.some(ap => ap.personId === opportunity.submittedByPersonId)) uses a plain relation with no status filter, inconsistent with the ACTIVE-only authorization check added in this PR just above it (line 474). A COORDINATOR (who bypasses the AGENT-only membership gate) creating an opportunity with submitted_by_id set to a person who only has a PENDING membership in the target agent will permanently freeze that not-yet-approved person in as the opportunity's public-facing contact.

[arturasmckwcz]

correctness (auth bypass): good that this adds an ACTIVE-only status filter, but the personId feeding this check (a couple lines above, not shown in this hunk) is request.authUser?.personId || request.body.submitted_by_id — if the caller's own personId is null/undefined, it silently falls back to the client-supplied submitted_by_id.

Failure scenario: an AGENT-role account with no linked personId sends agent_id: <targetAgent> and submitted_by_id: <activeMemberPersonId> (a personId it knows is an ACTIVE member of the target agent); this ACTIVE-membership check then succeeds using the impersonated id, letting a non-member create an opportunity for an agent they don't belong to.

[arturasmckwcz]

correctness / regression: the contactLinkId check now requires status: AgentMembershipStatus.ACTIVE, newly rejecting a PENDING agent member who previously could be set as an opportunity's contact — no test in the diff covers this behavior change, and the resulting error message ("is not registered as a contact") is misleading since the person IS registered, just not yet ACTIVE.

Failure scenario: a coordinator or agent tries to set a freshly-invited, still-PENDING colleague as an opportunity's contact during onboarding; the PATCH now 404s implying no registration exists at all.

[arturasmckwcz]

efficiency: this new AGENT ownership check issues its own opportunityRepository.findOne({ id, select: agentId }) immediately before opportunityVolunteerRepository.find(...) below, which already joins opportunity.agent via relations — an unnecessary extra DB round trip on every AGENT-role call.

(Note: a naive fix of checking volunteers[0]?.opportunity?.agentId instead would be unsafe on its own — when zero volunteers are linked, volunteers[0] is undefined regardless of ownership, silently turning the intended 404 into a 200 with an empty array.)

[arturasmckwcz]

simplification: this ownership-check-and-404 block (resolve caller's agentIds, compare to opportunity.agentId, throw NotFoundError) is copy-pasted near-verbatim across three handlers in this PR (opportunity.routes.ts GET /:id, this file, and a list-specific variant), rather than factored into one shared helper. A future change to this security-critical rule would need to be applied by hand in three places — missing one silently reopens the cross-agent exposure this PR is meant to close.

[arturasmckwcz]

efficiency: for AGENT-role callers, getCallerAgentIds now runs twice per request on GET /opportunity/:id, GET /opportunity, and GET /opportunity/:id/volunteer-linked — once in the route handler for the new ownership/scoping check, and again here inside resolveCallerVisibility's PII masking hook wired on the same routes. Consider resolving the caller's agentIds once per request and threading it through.

Also: a few lines below (unchanged by this diff, ~line 77), members = await agentPersonRepository.find({ where: { agentId: In([...agentIds]) } }) — which determines whose PII becomes unmasked for the caller — still has no status filter, contradicting the comment on getCallerAgentIds that a PENDING membership "grants nothing." An ACTIVE caller at Agent X with a PENDING co-applicant Y will still get Y's personId added to the unmasked set.

[arturasmckwcz]

reuse: this re-implements a query that the existing getActiveAgentMemberships(personId) helper (src/server/utils/data/get-agent-memberships.ts) already performs verbatim (same table, same personId+ACTIVE where-clause) instead of composing on top of it — per the shared-rules "reuse before you create" convention. If the definition of "active membership" ever changes, this duplicate can silently drift out of sync.

[need4deed]

@ivannissimrch can we merge this?

[ivannissimrch]

@need4deed Most of the review is fixed and pushed. Two comments I haven't addressed yet: getCallerAgentIds resolving twice per request, and the legacy public route. I need help with those two; I'm not confident enough to finish them on my own.

[need4deed]

@arturasmckwcz can you help out in guiding here?