fix(api_v2): REST API v2 defects found by the OpenFn adaptor (#554)

spp_api_v2/README.rst

@@ -147,6 +147,144 @@ Dependencies
 Changelog
 =========
 
+19.0.2.2.0
+~~~~~~~~~~
+
+- fix: search filters fail closed (#554). A filter that names nothing
+  (an unknown ``group``, ``gender``, ``membership-role`` or ``member``)
+  now returns no results instead of silently dropping out and returning
+  the whole registry. Malformed filters (``identifier``, ``group``,
+  ``gender``, ``birthdate``, ``_lastUpdated`` without the expected
+  format; ``member`` not an ``Individual/{system}|{value}`` reference)
+  return ``400`` on ``GET /Individual`` and ``GET /Group``.
+
+- fix: multi-condition search filters apply to one related row (#554).
+  ``GET /Individual?group=`` no longer lists someone who left the group
+  but is active in another; ``membership-role=`` requires the role on an
+  active membership; ``identifier=`` requires system and value on the
+  same ID. Behaviour change: ``GET /Group?member=`` now lists only
+  groups the individual is an active member of.
+
+- fix: returned references can be followed (#554).
+  ``Group.member[].entity``, the ``group``/``entity`` references in
+  ``$add-member``, ``$remove-member`` and member-update responses,
+  membership-history ``member``, and
+  ``Individual.groupMembership[].group`` were built from the identifier
+  type's vocabulary namespace (``urn:openspp:vocab:id-type|…``) and
+  could not be resolved; they now use the full code URI
+  (``urn:openspp:vocab:id-type#<code>|…``), the same value as
+  ``identifier[].system``.
+
+- fix: ending a membership "now" takes effect immediately (#554).
+  ``$remove-member`` without ``endedDate``, and the member moves in
+  group merge and split, wrote the end time with microseconds, which the
+  ``is_ended``/``status`` computes (second precision) treated as a
+  moment in the future: the membership stayed active until the repair
+  cron ran. They now use the ORM's clock (``fields.Datetime.now()``).
+
+- fix: identifiers are resolved to exactly one registrant (#554).
+  ``POST /Individual``, ``POST /Group``, ``$split`` and create entries
+  in ``$batch`` bundles return ``409`` when an identifier (type + value)
+  is already live on another registrant, instead of creating a second
+  record no lookup could tell apart; an archived registrant still holds
+  its identifiers (mark the ID invalid to free it). Lookups no longer
+  pick one of several registrants holding an identifier: read, update,
+  patch, member operations, merge/split and ``$batch``/transaction
+  bundles return ``409``; a client whose legal basis requires consent
+  and may not read every match gets the same ``403`` as for a registrant
+  that does not exist. In ``$batch``/transaction bundles, such a client
+  gets the entry's "not found" answer for a read, update or delete, and
+  ``403`` for a create whose member reference is ambiguous. Search
+  filters naming an ambiguous identifier (``group=``, ``member=``)
+  return ``409``, or an empty page for such a client; bulk export
+  reports the identifier with item status ``error`` (or
+  ``access_denied``). Individual endpoints and the ``member=`` filter
+  resolve individuals only, and Group endpoints groups only, so a value
+  held by one individual and one group is not ambiguous, and
+  ``GET /Individual/{id}`` no longer returns a group. Soft-removed
+  (``invalid``) IDs no longer resolve, are no longer listed in
+  ``identifier[]`` and are not matched by ``identifier=``; references
+  are built from a live ID, and a registrant with no live ID is left out
+  of ``Group.member[]``, membership history and
+  ``/Individual/{id}/groups``. Searching by ``identifier=`` still lists
+  every registrant holding a live match.
+
+- fix: ``GET /Group`` applies ``_offset`` (#554). The group search
+  ignored it, so every page (and every ``next`` link) returned the first
+  page again, and when consent filtering skipped records the page was
+  refilled from the start of the results, returning no records or
+  repeating groups.
+
+- fix: consent-filtered searches (``GET /Individual``, ``GET /Group``)
+  no longer skip records or stop early (#554). When the page filled
+  partway through a fetched batch, the ``next`` link jumped past the
+  rows not yet examined. Pages over 50 records mistook the per-query cap
+  of 100 for the end of the data. A page cut short by the over-fetch
+  safety limit dropped its ``next`` link although rows remained; the
+  link is now given until the scan reaches the end. **Clients must
+  follow ``next`` until it is null: a page can be short, or empty, and
+  still have a ``next`` link.** A consent-filtered page that ends on the
+  last row no longer links to an empty page.
+
+- fix(security): the search ``total`` never counts records the client
+  may not see (#554). For a client whose legal basis requires consent,
+  ``meta.total`` is the number of records returned on every page. It was
+  hidden only when this page met a hidden record, so a page past the end
+  (``?identifier=…&_offset=1``) or one filled before any hidden row
+  returned the full count, including records the client may not see.
+  Clients with a legal basis that doesn't require consent still get the
+  exact total. The ``next`` offsets are still database positions, so
+  hidden rows can be counted from them; see the follow-up issue.
+
+- fix: ``_offset`` values beyond what the database accepts return
+  ``422`` instead of a server error (#554).
+
+- fix: ``GET /Individual?group=…&membership-role=…`` requires the role
+  on the membership of that group (#554). Someone who held the role in
+  another group was also returned.
+
+- fix: ``PATCH /Individual`` updates ``gender`` (#554). The vocabulary
+  lookup ran without the privileges create uses, so every gender change
+  returned ``422 "Failed to patch individual"``. An unknown gender code
+  now returns ``422`` naming the code, as create does, instead of being
+  ignored. On create and ``PATCH``, only codes of the
+  ``urn:iso:std:iso:5218`` vocabulary are accepted: a code from another
+  vocabulary (eg a membership role) returns ``422``.
+
+- fix: ``$add-member`` and ``PATCH /Group/{id}/member/{id}`` reject a
+  role code that doesn't exist with ``422`` naming it (#554), instead of
+  adding or keeping the member without the role. For someone who is
+  already a member, ``$add-member`` still answers ``409`` first. Other
+  validation errors on these two endpoints now return their message with
+  the ``422`` (eg "Duplication of Member is not allowed", "End Date
+  cannot be earlier than Start Date") instead of a generic "Failed to
+  add member" / "Failed to update member".
+
+- fix(security): one rule decides whether a client is subject to
+  consent: its legal basis (#554 review). The "not found" answers for
+  the addressed registrant (reads, bulk export, the group of member
+  operations, membership history and ``/Individual/{id}/groups``), and
+  ``ConsentService.check_access``, branched on the client's
+  ``is_require_consent`` box while consent filtering keys off
+  ``legal_basis``. A client with legal basis "consent" and the box
+  unticked got ``404`` for an unknown registrant and ``403`` (or a
+  filtered ``200``) for a real one, so the status told them apart.
+  Behaviour change for clients whose two settings disagree: legal basis
+  "consent" now always gets ``403`` for both; a legal basis that needs
+  no consent gets ``404`` for an unknown registrant and reads without
+  consent, whatever the box says.
+
+- fix(security): the "identifier in use" ``409`` names the ID type, not
+  the value (#554 review), and ``$batch``/transaction bundle entries
+  that fail on client input (including a malformed resource) are logged
+  by error type only, so their error text no longer carries identifier
+  values (often national IDs) into the server log.
+
+- fix: ``$add-member``, ``$remove-member`` and
+  ``PATCH /Group/{id}/member/{id}`` choose ``409``/``404`` by error type
+  instead of by matching the English message text (#554 review), so a
+  non-English user language no longer turns them into ``422``.
+
 19.0.2.1.1
 ~~~~~~~~~~
 

spp_api_v2/__manifest__.py

@@ -1,7 +1,7 @@
 {
     "name": "OpenSPP API V2",
     "category": "OpenSPP/Integration",
-    "version": "19.0.2.1.1",
+    "version": "19.0.2.2.0",
     "sequence": 1,
     "author": "OpenSPP.org",
     "website": "https://github.com/OpenSPP/OpenSPP2",

spp_api_v2/readme/HISTORY.md

@@ -1,3 +1,22 @@
+### 19.0.2.2.0
+
+- fix: search filters fail closed (#554). A filter that names nothing (an unknown `group`, `gender`, `membership-role` or `member`) now returns no results instead of silently dropping out and returning the whole registry. Malformed filters (`identifier`, `group`, `gender`, `birthdate`, `_lastUpdated` without the expected format; `member` not an `Individual/{system}|{value}` reference) return `400` on `GET /Individual` and `GET /Group`.
+- fix: multi-condition search filters apply to one related row (#554). `GET /Individual?group=` no longer lists someone who left the group but is active in another; `membership-role=` requires the role on an active membership; `identifier=` requires system and value on the same ID. Behaviour change: `GET /Group?member=` now lists only groups the individual is an active member of.
+- fix: returned references can be followed (#554). `Group.member[].entity`, the `group`/`entity` references in `$add-member`, `$remove-member` and member-update responses, membership-history `member`, and `Individual.groupMembership[].group` were built from the identifier type's vocabulary namespace (`urn:openspp:vocab:id-type|…`) and could not be resolved; they now use the full code URI (`urn:openspp:vocab:id-type#<code>|…`), the same value as `identifier[].system`.
+- fix: ending a membership "now" takes effect immediately (#554). `$remove-member` without `endedDate`, and the member moves in group merge and split, wrote the end time with microseconds, which the `is_ended`/`status` computes (second precision) treated as a moment in the future: the membership stayed active until the repair cron ran. They now use the ORM's clock (`fields.Datetime.now()`).
+- fix: identifiers are resolved to exactly one registrant (#554). `POST /Individual`, `POST /Group`, `$split` and create entries in `$batch` bundles return `409` when an identifier (type + value) is already live on another registrant, instead of creating a second record no lookup could tell apart; an archived registrant still holds its identifiers (mark the ID invalid to free it). Lookups no longer pick one of several registrants holding an identifier: read, update, patch, member operations, merge/split and `$batch`/transaction bundles return `409`; a client whose legal basis requires consent and may not read every match gets the same `403` as for a registrant that does not exist. In `$batch`/transaction bundles, such a client gets the entry's "not found" answer for a read, update or delete, and `403` for a create whose member reference is ambiguous. Search filters naming an ambiguous identifier (`group=`, `member=`) return `409`, or an empty page for such a client; bulk export reports the identifier with item status `error` (or `access_denied`). Individual endpoints and the `member=` filter resolve individuals only, and Group endpoints groups only, so a value held by one individual and one group is not ambiguous, and `GET /Individual/{id}` no longer returns a group. Soft-removed (`invalid`) IDs no longer resolve, are no longer listed in `identifier[]` and are not matched by `identifier=`; references are built from a live ID, and a registrant with no live ID is left out of `Group.member[]`, membership history and `/Individual/{id}/groups`. Searching by `identifier=` still lists every registrant holding a live match.
+- fix: `GET /Group` applies `_offset` (#554). The group search ignored it, so every page (and every `next` link) returned the first page again, and when consent filtering skipped records the page was refilled from the start of the results, returning no records or repeating groups.
+- fix: consent-filtered searches (`GET /Individual`, `GET /Group`) no longer skip records or stop early (#554). When the page filled partway through a fetched batch, the `next` link jumped past the rows not yet examined. Pages over 50 records mistook the per-query cap of 100 for the end of the data. A page cut short by the over-fetch safety limit dropped its `next` link although rows remained; the link is now given until the scan reaches the end. **Clients must follow `next` until it is null: a page can be short, or empty, and still have a `next` link.** A consent-filtered page that ends on the last row no longer links to an empty page.
+- fix(security): the search `total` never counts records the client may not see (#554). For a client whose legal basis requires consent, `meta.total` is the number of records returned on every page. It was hidden only when this page met a hidden record, so a page past the end (`?identifier=…&_offset=1`) or one filled before any hidden row returned the full count, including records the client may not see. Clients with a legal basis that doesn't require consent still get the exact total. The `next` offsets are still database positions, so hidden rows can be counted from them; see the follow-up issue.
+- fix: `_offset` values beyond what the database accepts return `422` instead of a server error (#554).
+- fix: `GET /Individual?group=…&membership-role=…` requires the role on the membership of that group (#554). Someone who held the role in another group was also returned.
+- fix: `PATCH /Individual` updates `gender` (#554). The vocabulary lookup ran without the privileges create uses, so every gender change returned `422 "Failed to patch individual"`. An unknown gender code now returns `422` naming the code, as create does, instead of being ignored. On create and `PATCH`, only codes of the `urn:iso:std:iso:5218` vocabulary are accepted: a code from another vocabulary (eg a membership role) returns `422`.
+- fix: `$add-member` and `PATCH /Group/{id}/member/{id}` reject a role code that doesn't exist with `422` naming it (#554), instead of adding or keeping the member without the role. For someone who is already a member, `$add-member` still answers `409` first. Other validation errors on these two endpoints now return their message with the `422` (eg "Duplication of Member is not allowed", "End Date cannot be earlier than Start Date") instead of a generic "Failed to add member" / "Failed to update member".
+
+- fix(security): one rule decides whether a client is subject to consent: its legal basis (#554 review). The "not found" answers for the addressed registrant (reads, bulk export, the group of member operations, membership history and `/Individual/{id}/groups`), and `ConsentService.check_access`, branched on the client's `is_require_consent` box while consent filtering keys off `legal_basis`. A client with legal basis "consent" and the box unticked got `404` for an unknown registrant and `403` (or a filtered `200`) for a real one, so the status told them apart. Behaviour change for clients whose two settings disagree: legal basis "consent" now always gets `403` for both; a legal basis that needs no consent gets `404` for an unknown registrant and reads without consent, whatever the box says.
+- fix(security): the "identifier in use" `409` names the ID type, not the value (#554 review), and `$batch`/transaction bundle entries that fail on client input (including a malformed resource) are logged by error type only, so their error text no longer carries identifier values (often national IDs) into the server log.
+- fix: `$add-member`, `$remove-member` and `PATCH /Group/{id}/member/{id}` choose `409`/`404` by error type instead of by matching the English message text (#554 review), so a non-English user language no longer turns them into `422`.
+
 ### 19.0.2.1.1
 
 - chore(api_v2): the API V2 configuration menu moved from Registry > Configuration to Settings > Registry, alongside the other superuser configuration (#1009)

spp_api_v2/routers/batch.py

@@ -14,6 +14,8 @@
 from ..middleware.auth import get_authenticated_client
 from ..schemas.bundle import RegistrantBundle
 from ..services.bundle_service import BundleProcessor
+from ..services.registrant_resolver import AmbiguousIdentifierError, IdentifierInUseError
+from ..utils.registrant_lookup import ambiguous_identifier_status
 
 _logger = logging.getLogger(__name__)
 
@@ -128,7 +130,36 @@ async def process_bundle(
             return result
 
     except ValidationError as e:
-        _logger.error(f"Bundle processing failed for client {api_client.client_id}: {str(e)}")
+        if (
+            isinstance(e.__cause__, AmbiguousIdentifierError)
+            and ambiguous_identifier_status(env, api_client, e.__cause__) == status.HTTP_403_FORBIDDEN
+        ):
+            # Only a client that may read every match learns the identifier is
+            # ambiguous; the others get the 403 POST /Group gives them
+            raise HTTPException(
+                status_code=status.HTTP_403_FORBIDDEN,
+                detail={
+                    "resourceType": "OperationOutcome",
+                    "issue": [{"severity": "error", "code": "forbidden", "diagnostics": "Access denied"}],
+                },
+            ) from e
+        if isinstance(e.__cause__, AmbiguousIdentifierError | IdentifierInUseError):
+            # A transaction entry's identifier matches, or would match, more
+            # than one registrant: a conflict, as for the single-resource endpoints
+            raise HTTPException(
+                status_code=status.HTTP_409_CONFLICT,
+                detail={
+                    "resourceType": "OperationOutcome",
+                    "issue": [{"severity": "error", "code": "conflict", "diagnostics": str(e)}],
+                },
+            ) from e
+        # The message can carry the identifiers the client sent (PII): log the
+        # failing entry's error type, not its text
+        _logger.warning(
+            "Bundle processing failed for client %s: %s",
+            api_client.client_id,
+            type(e.__cause__ or e).__name__,
+        )
         raise HTTPException(
             status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
             detail={

spp_api_v2/routers/bulk.py

@@ -18,6 +18,8 @@
 from ..services.field_filter import filter_fields
 from ..services.group_service import GroupService
 from ..services.individual_service import IndividualService
+from ..services.registrant_resolver import AmbiguousIdentifierError
+from ..utils.registrant_lookup import AMBIGUOUS_DETAIL, ambiguous_identifier_status
 
 _logger = logging.getLogger(__name__)
 
@@ -94,8 +96,19 @@ async def bulk_export(
     for identifier, _system, _value in parsed:
         record = records_map.get(identifier)
 
+        if isinstance(record, AmbiguousIdentifierError):
+            # Several registrants hold the identifier: refuse to pick one,
+            # without revealing it to a client that may not see them all
+            if ambiguous_identifier_status(env, api_client, record) == status.HTTP_403_FORBIDDEN:
+                await asyncio.sleep(0.05 + random.uniform(0, 0.02))  # same jitter as "not found"
+                items.append(BulkExportItem(identifier=identifier, status="access_denied", error="Access denied"))
+            else:
+                items.append(BulkExportItem(identifier=identifier, status="error", error=AMBIGUOUS_DETAIL))
+            failed += 1
+            continue
+
         if not record:
-            if api_client.is_require_consent:
+            if ConsentService.is_consent_filtered(api_client):
                 # SECURITY: Add timing jitter to prevent enumeration via timing
                 await asyncio.sleep(0.05 + random.uniform(0, 0.02))  # 50-70ms delay
                 # Don't reveal if resource exists for consent-required clients
@@ -129,7 +142,7 @@ async def bulk_export(
         )
 
         # Check if consent was denied
-        if api_client.is_require_consent:
+        if ConsentService.is_consent_filtered(api_client):
             consent_info = filtered_data.get("_consent", {})
             if consent_info.get("status") in ("no_consent", "scope_mismatch"):
                 items.append(