diff --git a/spp_api_v2/README.rst b/spp_api_v2/README.rst index 26a40cefc..4c981fcfe 100644 --- a/spp_api_v2/README.rst +++ b/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#|…``), 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 ~~~~~~~~~~ diff --git a/spp_api_v2/__manifest__.py b/spp_api_v2/__manifest__.py index 213b07c74..3d4ddb527 100644 --- a/spp_api_v2/__manifest__.py +++ b/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", diff --git a/spp_api_v2/readme/HISTORY.md b/spp_api_v2/readme/HISTORY.md index 6fdfa4de3..7724ca57a 100644 --- a/spp_api_v2/readme/HISTORY.md +++ b/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#|…`), 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) diff --git a/spp_api_v2/routers/batch.py b/spp_api_v2/routers/batch.py index 9a6de1a4b..76661a0e9 100644 --- a/spp_api_v2/routers/batch.py +++ b/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={ diff --git a/spp_api_v2/routers/bulk.py b/spp_api_v2/routers/bulk.py index e701480da..b10fb679b 100644 --- a/spp_api_v2/routers/bulk.py +++ b/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( diff --git a/spp_api_v2/routers/dependencies.py b/spp_api_v2/routers/dependencies.py index 486f08665..9bf43bdf7 100644 --- a/spp_api_v2/routers/dependencies.py +++ b/spp_api_v2/routers/dependencies.py @@ -94,7 +94,7 @@ async def check_group_access( # SECURITY: Prevent user enumeration # For clients requiring consent, return same error for "not found" and "no consent" if not group: - if api_client.is_require_consent: + if ConsentService.is_consent_filtered(api_client): # SECURITY: Add timing jitter to prevent timing-based enumeration await asyncio.sleep(0.05 + random.uniform(0, 0.02)) # 50-70ms delay raise HTTPException( @@ -107,7 +107,7 @@ async def check_group_access( ) # SECURITY: Check consent before allowing operation (for clients requiring consent) - if api_client.is_require_consent: + if ConsentService.is_consent_filtered(api_client): consent_service = ConsentService(env) if not consent_service.check_access(group.id, api_client, "group", operation): raise HTTPException( @@ -140,7 +140,7 @@ async def check_individual_access( """ # SECURITY: Prevent user enumeration if not individual: - if api_client.is_require_consent: + if ConsentService.is_consent_filtered(api_client): # SECURITY: Add timing jitter to prevent timing-based enumeration await asyncio.sleep(0.05 + random.uniform(0, 0.02)) # 50-70ms delay raise HTTPException( @@ -153,7 +153,7 @@ async def check_individual_access( ) # SECURITY: Check consent before allowing operation (for clients requiring consent) - if api_client.is_require_consent: + if ConsentService.is_consent_filtered(api_client): consent_service = ConsentService(env) if not consent_service.check_access(individual.id, api_client, "individual", operation): raise HTTPException( diff --git a/spp_api_v2/routers/group.py b/spp_api_v2/routers/group.py index 926159141..87adffdb3 100644 --- a/spp_api_v2/routers/group.py +++ b/spp_api_v2/routers/group.py @@ -40,9 +40,15 @@ from ..services.api_audit_service import ApiAuditService from ..services.consent_service import ConsentService from ..services.field_filter import filter_fields, filter_list -from ..services.group_service import GroupService -from ..services.search_service import SearchService -from ..utils.pagination import fetch_with_consent +from ..services.group_service import AlreadyMemberError, GroupService, NotMemberError +from ..services.registrant_resolver import AmbiguousIdentifierError, IdentifierInUseError +from ..services.search_service import InvalidSearchParam, SearchService +from ..utils.pagination import MAX_OFFSET, fetch_with_consent, page_total_and_next +from ..utils.registrant_lookup import ( + ambiguous_filter_result, + lookup_registrant, + raise_ambiguous_identifier, +) from .dependencies import check_group_access, parse_identifier, parse_resource_reference _logger = logging.getLogger(__name__) @@ -82,12 +88,12 @@ async def read_group( # Find group service = GroupService(env) - group = service.find_by_identifier(system, value) + group = await lookup_registrant(env, api_client, service.find_by_identifier, system, value) # SECURITY: Prevent user enumeration # For clients requiring consent, return same error for "not found" and "no consent" if not group: - if api_client.is_require_consent: + if ConsentService.is_consent_filtered(api_client): # SECURITY: Add timing jitter to prevent timing-based enumeration await asyncio.sleep(0.05 + random.uniform(0, 0.02)) # 50-70ms delay raise HTTPException( @@ -113,7 +119,7 @@ async def read_group( ) # SECURITY: Check if consent was denied and return same error as "not found" - 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"): raise HTTPException( @@ -157,7 +163,7 @@ async def search_groups( type_: Annotated[str | None, Query(alias="type")] = None, member: Annotated[str | None, Query()] = None, count: Annotated[int, Query(alias="_count", ge=1, le=100)] = 20, - offset: Annotated[int, Query(alias="_offset", ge=0)] = 0, + offset: Annotated[int, Query(alias="_offset", ge=0, le=MAX_OFFSET)] = 0, elements: Annotated[str | None, Query(alias="_elements")] = None, extensions: Annotated[str | None, Query(alias="_extensions")] = None, ): @@ -199,7 +205,13 @@ async def search_groups( def search_function(offset, limit): search_params = {**params, "_count": limit, "_offset": offset} - return search_service.search_groups(search_params) + try: + return search_service.search_groups(search_params) + except InvalidSearchParam as e: + raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) from e + except AmbiguousIdentifierError as e: + # A filter reference (member=) matches more than one registrant + return ambiguous_filter_result(env, api_client, e, env["res.partner"]) def consent_filter_function(group): group_data = group_service.to_api_schema(group, extensions=extension_list) @@ -220,11 +232,16 @@ def consent_filter_function(group): if elements: data = filter_list(data, elements) - # Suppress total when consent filtering is active - if consent_was_applied: - total = len(data) - else: - total = raw_total + # For a consent-filtered client the total is the page size and the next + # link continues while rows remain (see page_total_and_next) + total, next_offset = page_total_and_next( + returned=len(data), + count=count, + offset=offset, + db_offset_consumed=db_offset_consumed, + raw_total=raw_total, + consent_filtered=consent_was_applied or consent_service.is_consent_filtered(api_client), + ) # Build pagination links with proper URL encoding base_url = "/api/v2/spp/Group" @@ -235,11 +252,8 @@ def build_url(offset_val: int) -> str: url_params = {**base_params, "_count": count, "_offset": offset_val} return f"{base_url}?{urlencode(url_params)}" - next_offset = db_offset_consumed if consent_was_applied else offset + count - has_more = len(data) >= count - self_url = build_url(offset) - next_url = build_url(next_offset) if has_more else None + next_url = build_url(next_offset) if next_offset is not None else None prev_url = build_url(max(0, offset - count)) if offset > 0 else None return create_search_result( @@ -282,6 +296,20 @@ async def create_group( # api_authorized=True tells service to skip user group check since # API client scope (verified above) is the authorization for API calls group_record = service.create(group, source=source_system, api_authorized=True) + except IdentifierInUseError as e: + # Another registrant holds one of the identifiers: creating a second + # would leave neither addressable unambiguously + audit_service.log_create( + "group", + group.identifier[0].system + "|" + group.identifier[0].value if group.identifier else "unknown", + None, + status="validation_error", + error_detail="Identifier in use", + ) + raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=e.args[0]) from e + except AmbiguousIdentifierError as e: + # A member reference matches more than one individual + await raise_ambiguous_identifier(env, api_client, e) except ValidationError as ve: _logger.warning("Validation error creating group: %s", ve) raise HTTPException( @@ -340,11 +368,11 @@ async def update_group( # Find group service = GroupService(env) - group_record = service.find_by_identifier(system, value) + group_record = await lookup_registrant(env, api_client, service.find_by_identifier, system, value) # SECURITY: Prevent user enumeration if not group_record: - if api_client.is_require_consent: + if ConsentService.is_consent_filtered(api_client): raise HTTPException( status_code=status.HTTP_403_FORBIDDEN, detail="Access denied", @@ -355,7 +383,7 @@ async def update_group( ) # SECURITY: Check consent before allowing update (for clients requiring consent) - if api_client.is_require_consent: + if ConsentService.is_consent_filtered(api_client): consent_service = ConsentService(env) if not consent_service.check_access(group_record.id, api_client, "group", "update"): raise HTTPException( @@ -435,7 +463,7 @@ async def patch_group( # Parse identifier and find group system, value = parse_identifier(identifier) service = GroupService(env) - group_record = service.find_by_identifier(system, value) + group_record = await lookup_registrant(env, api_client, service.find_by_identifier, system, value) # Security checks (enumeration prevention + consent) await check_group_access(group_record, api_client, env, "update") @@ -506,7 +534,7 @@ async def add_member( # Parse identifier and find group system, value = parse_identifier(identifier) service = GroupService(env) - group = service.find_by_identifier(system, value) + group = await lookup_registrant(env, api_client, service.find_by_identifier, system, value) # Security checks (enumeration prevention + consent) await check_group_access(group, api_client, env, "update") @@ -515,7 +543,7 @@ async def add_member( ind_system, ind_value = parse_resource_reference(request.entity.reference, "Individual") # Find individual - individual = service._find_individual(ind_system, ind_value) + individual = await lookup_registrant(env, api_client, service._find_individual, ind_system, ind_value) if not individual: raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, @@ -539,14 +567,17 @@ async def add_member( role_coding=role_coding, start_date=request.start_date, ) + except AlreadyMemberError as e: + raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=str(e)) from e + except ValidationError as ve: + # Other client errors (eg unknown role): keep the message + _logger.warning("Validation error adding member to group: %s", ve) + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=str(ve), + ) from ve except Exception as e: _logger.exception("Error adding member to group") - # Check for specific errors - if "already a member" in str(e).lower(): - raise HTTPException( - status_code=status.HTTP_409_CONFLICT, - detail=str(e), - ) from e raise HTTPException( status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail="Failed to add member", @@ -577,7 +608,7 @@ async def remove_member( # Parse identifier and find group system, value = parse_identifier(identifier) service = GroupService(env) - group = service.find_by_identifier(system, value) + group = await lookup_registrant(env, api_client, service.find_by_identifier, system, value) # Security checks (enumeration prevention + consent) await check_group_access(group, api_client, env, "update") @@ -586,7 +617,7 @@ async def remove_member( ind_system, ind_value = parse_resource_reference(request.entity.reference, "Individual") # Find individual - individual = service._find_individual(ind_system, ind_value) + individual = await lookup_registrant(env, api_client, service._find_individual, ind_system, ind_value) if not individual: raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, @@ -601,14 +632,10 @@ async def remove_member( ended_date=request.ended_date, reason=request.reason, ) + except NotMemberError as e: + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=str(e)) from e except Exception as e: _logger.exception("Error removing member from group") - # Check for specific errors - if "not a member" in str(e).lower(): - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail=str(e), - ) from e raise HTTPException( status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail="Failed to remove member", @@ -643,13 +670,13 @@ async def update_member( # Find group service = GroupService(env) - group = service.find_by_identifier(system, value) + group = await lookup_registrant(env, api_client, service.find_by_identifier, system, value) # Security checks (enumeration prevention + consent) await check_group_access(group, api_client, env, "update") # Find individual - individual = service._find_individual(ind_system, ind_value) + individual = await lookup_registrant(env, api_client, service._find_individual, ind_system, ind_value) if not individual: raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, @@ -674,14 +701,17 @@ async def update_member( start_date=request.start_date, ended_date=request.ended_date, ) + except NotMemberError as e: + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=str(e)) from e + except ValidationError as ve: + # Other client errors (eg unknown role): keep the message + _logger.warning("Validation error updating member in group: %s", ve) + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=str(ve), + ) from ve except Exception as e: _logger.exception("Error updating member in group") - # Check for specific errors - if "not a member" in str(e).lower(): - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail=str(e), - ) from e raise HTTPException( status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail="Failed to update member", @@ -719,24 +749,24 @@ async def merge_groups( # Find groups service = GroupService(env) - source_group = service.find_by_identifier(source_system, source_value) - target_group = service.find_by_identifier(target_system, target_value) + source_group = await lookup_registrant(env, api_client, service.find_by_identifier, source_system, source_value) + target_group = await lookup_registrant(env, api_client, service.find_by_identifier, target_system, target_value) # Security checks for both groups (with specific error messages) if not source_group: - if api_client.is_require_consent: + if ConsentService.is_consent_filtered(api_client): await asyncio.sleep(0.05 + random.uniform(0, 0.02)) raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="Access denied") raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Source group not found") if not target_group: - if api_client.is_require_consent: + if ConsentService.is_consent_filtered(api_client): await asyncio.sleep(0.05 + random.uniform(0, 0.02)) raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="Access denied") raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Target group not found") # SECURITY: Check consent for both groups (for clients requiring consent) - if api_client.is_require_consent: + if ConsentService.is_consent_filtered(api_client): consent_service = ConsentService(env) if not consent_service.check_access(source_group.id, api_client, "group", "update"): raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="Access denied") @@ -804,7 +834,7 @@ async def split_group( # Parse identifier and find source group system, value = parse_identifier(identifier) service = GroupService(env) - source_group = service.find_by_identifier(system, value) + source_group = await lookup_registrant(env, api_client, service.find_by_identifier, system, value) # Security checks (enumeration prevention + consent) await check_group_access(source_group, api_client, env, "update") @@ -822,7 +852,7 @@ async def split_group( ind_system, ind_value = parse_resource_reference(member_ref.reference, "Individual") # Find individual - individual = service._find_individual(ind_system, ind_value) + individual = await lookup_registrant(env, api_client, service._find_individual, ind_system, ind_value) if not individual: raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, @@ -850,7 +880,7 @@ async def split_group( head_system, head_value = head_ident_str.split("|", 1) # Find new head individual - new_head = service._find_individual(head_system, head_value) + new_head = await lookup_registrant(env, api_client, service._find_individual, head_system, head_value) if not new_head: raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, @@ -870,6 +900,9 @@ async def split_group( new_head=new_head, source=source_system, ) + except IdentifierInUseError as e: + # The new group's identifier is already held by another registrant + raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=e.args[0]) from e except Exception as e: _logger.exception("Error splitting group") # Check for specific validation errors @@ -936,7 +969,7 @@ async def get_membership_history( # Parse identifier and find group system, value = parse_identifier(identifier) service = GroupService(env) - group = service.find_by_identifier(system, value) + group = await lookup_registrant(env, api_client, service.find_by_identifier, system, value) # Security checks (enumeration prevention + consent) await check_group_access(group, api_client, env, "read") diff --git a/spp_api_v2/routers/individual.py b/spp_api_v2/routers/individual.py index 9916edfef..b690b3080 100644 --- a/spp_api_v2/routers/individual.py +++ b/spp_api_v2/routers/individual.py @@ -33,8 +33,10 @@ from ..services.consent_service import ConsentService from ..services.field_filter import filter_fields, filter_list from ..services.individual_service import IndividualService -from ..services.search_service import SearchService -from ..utils.pagination import fetch_with_consent +from ..services.registrant_resolver import AmbiguousIdentifierError, IdentifierInUseError +from ..services.search_service import InvalidSearchParam, SearchService +from ..utils.pagination import MAX_OFFSET, fetch_with_consent, page_total_and_next +from ..utils.registrant_lookup import ambiguous_filter_result, lookup_registrant from .dependencies import check_individual_access, parse_identifier _logger = logging.getLogger(__name__) @@ -76,13 +78,13 @@ async def read_individual( # Find individual by namespace_uri + value service = IndividualService(env) - partner = service.find_by_identifier(system, value) + partner = await lookup_registrant(env, api_client, service.find_by_identifier, system, value) # SECURITY: Prevent user enumeration # For clients requiring consent, return same error for "not found" and "no consent" # to prevent attackers from determining which individuals exist in the system. if not partner: - if api_client.is_require_consent: + if ConsentService.is_consent_filtered(api_client): # SECURITY: Add timing jitter to prevent timing-based enumeration # Simulate the time it would take to process a found record await asyncio.sleep(0.05 + random.uniform(0, 0.02)) # 50-70ms delay @@ -109,7 +111,7 @@ async def read_individual( ) # SECURITY: Check if consent was denied and return same error as "not found" - 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"): raise HTTPException( @@ -158,7 +160,7 @@ async def search_individuals( membership_role: Annotated[str | None, Query(alias="membership-role")] = None, last_updated: Annotated[str | None, Query(alias="_lastUpdated")] = None, count: Annotated[int, Query(alias="_count", ge=1, le=100)] = 20, - offset: Annotated[int, Query(alias="_offset", ge=0)] = 0, + offset: Annotated[int, Query(alias="_offset", ge=0, le=MAX_OFFSET)] = 0, sort: Annotated[str | None, Query(alias="_sort")] = None, elements: Annotated[str | None, Query(alias="_elements")] = None, extensions: Annotated[str | None, Query(alias="_extensions")] = None, @@ -214,7 +216,13 @@ async def search_individuals( def search_function(offset, limit): search_params = {**params, "_count": limit, "_offset": offset} - return search_service.search_individuals(search_params) + try: + return search_service.search_individuals(search_params) + except InvalidSearchParam as e: + raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) from e + except AmbiguousIdentifierError as e: + # A filter reference (group=) matches more than one registrant + return ambiguous_filter_result(env, api_client, e, env["res.partner"]) def consent_filter_function(partner): data = individual_service.to_api_schema(partner, extensions=extension_list) @@ -235,13 +243,17 @@ def consent_filter_function(partner): if elements: individuals_data = filter_list(individuals_data, elements) - # When consent filtering is active, suppress the exact total to - # avoid leaking the count of denied records. Clients detect - # end-of-results when they receive fewer than `count` records. - if consent_was_applied: - total = len(individuals_data) - else: - total = raw_total + # For a consent-filtered client the total is the page size and the next + # link continues while rows remain: clients follow `next` until it is + # null, a short or empty page is not the end of the results + total, next_offset = page_total_and_next( + returned=len(individuals_data), + count=count, + offset=offset, + db_offset_consumed=db_offset_consumed, + raw_total=raw_total, + consent_filtered=consent_was_applied or consent_service.is_consent_filtered(api_client), + ) # Build pagination links with proper URL encoding base_url = "/api/v2/spp/Individual" @@ -252,12 +264,8 @@ def build_url(offset_val: int) -> str: url_params = {**base_params, "_count": count, "_offset": offset_val} return f"{base_url}?{urlencode(url_params)}" - # Use the consumed DB offset for next page link when consent filtering - next_offset = db_offset_consumed if consent_was_applied else offset + count - has_more = len(individuals_data) >= count - self_url = build_url(offset) - next_url = build_url(next_offset) if has_more else None + next_url = build_url(next_offset) if next_offset is not None else None prev_url = build_url(max(0, offset - count)) if offset > 0 else None return create_search_result( @@ -306,6 +314,19 @@ async def create_individual( # api_authorized=True tells service to skip user group check since # API client scope (verified above) is the authorization for API calls partner = service.create(individual, source=source_system, api_authorized=True) + except IdentifierInUseError as e: + # Another registrant holds one of the identifiers: creating a second + # would leave neither addressable unambiguously + audit_service.log_create( + "individual", + individual.identifier[0].system + "|" + individual.identifier[0].value + if individual.identifier + else "unknown", + None, + status="validation_error", + error_detail="Identifier in use", + ) + raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=e.args[0]) from e except ValidationError as ve: _logger.warning("Validation error creating individual: %s", ve) # Log failed create attempt @@ -385,7 +406,7 @@ async def update_individual( ip_address=request.client.host if request.client else None, user_agent=request.headers.get("user-agent"), ) - partner = service.find_by_identifier(system, value) + partner = await lookup_registrant(env, api_client, service.find_by_identifier, system, value) # Security checks (enumeration prevention + consent) await check_individual_access(partner, api_client, env, "update") @@ -461,7 +482,7 @@ async def patch_individual( ip_address=request.client.host if request.client else None, user_agent=request.headers.get("user-agent"), ) - partner = service.find_by_identifier(system, value) + partner = await lookup_registrant(env, api_client, service.find_by_identifier, system, value) # Security checks (enumeration prevention + consent) await check_individual_access(partner, api_client, env, "update") @@ -544,7 +565,7 @@ async def get_individual_groups( # Parse identifier and find individual system, value = parse_identifier(identifier) service = IndividualService(env) - partner = service.find_by_identifier(system, value) + partner = await lookup_registrant(env, api_client, service.find_by_identifier, system, value) # Security checks (enumeration prevention + consent) await check_individual_access(partner, api_client, env, "read") diff --git a/spp_api_v2/services/auth_service.py b/spp_api_v2/services/auth_service.py index 3f2e3fa75..d1e641e13 100644 --- a/spp_api_v2/services/auth_service.py +++ b/spp_api_v2/services/auth_service.py @@ -10,6 +10,8 @@ import odoo +from .consent_service import NON_CONSENT_BASES + _logger = logging.getLogger(__name__) @@ -117,24 +119,15 @@ def get_allowed_fields(self, resource_type: str) -> set | None: def has_legal_basis_bypass(self) -> bool: """Check if client has legal basis that bypasses individual consent. - Per GDPR Article 6, these bases allow processing without consent: - - legal_obligation: Required by law - - vital_interest: Life-threatening emergencies - - public_interest: Public interest tasks - - public_task: Official authority + Per GDPR Article 6, the bases in ``NON_CONSENT_BASES`` allow processing + without consent (legal obligation, vital interest, public interest, + public task, contract, legitimate interest). The list lives in + consent_service, next to ``ConsentService.is_consent_filtered``. Returns: True if consent check can be bypassed """ - non_consent_bases = ( - "legal_obligation", - "vital_interest", - "public_interest", - "public_task", - "contract", - "legitimate_interest", - ) - return self.legal_basis in non_consent_bases + return self.legal_basis in NON_CONSENT_BASES def __repr__(self) -> str: identifier = self.client_id or self.sender_id or str(self.id) diff --git a/spp_api_v2/services/bundle_service.py b/spp_api_v2/services/bundle_service.py index a3e5ed06b..e8a844165 100644 --- a/spp_api_v2/services/bundle_service.py +++ b/spp_api_v2/services/bundle_service.py @@ -5,17 +5,36 @@ import re from typing import Any +from pydantic import ValidationError as PydanticValidationError + from odoo.api import Environment from odoo.exceptions import UserError, ValidationError +from fastapi import status + from ..schemas.bundle import Bundle, BundleEntry from ..schemas.group import Group from ..schemas.individual import Individual +from ..utils.registrant_lookup import ambiguous_identifier_status from .group_service import GroupService from .individual_service import IndividualService +from .registrant_resolver import AmbiguousIdentifierError, IdentifierInUseError _logger = logging.getLogger(__name__) +# Errors a bundle entry raises for bad input. Their messages can carry the +# identifiers the client sent (often national IDs), and a schema error quotes +# the input (input_value=...), so they are logged by type +CLIENT_ERRORS = (ValidationError, UserError, AmbiguousIdentifierError, PydanticValidationError) + + +def _log_entry_failure(bundle_kind, idx, error): + """Log a failed entry: a client error by type only, anything else with its traceback.""" + if isinstance(error, CLIENT_ERRORS): + _logger.warning("%s entry %s failed: %s", bundle_kind, idx + 1, type(error).__name__) + else: + _logger.error("%s entry %s failed", bundle_kind, idx + 1, exc_info=error) + class BundleProcessor: """Process transaction and batch bundles atomically""" @@ -87,7 +106,7 @@ def process_transaction(self, bundle: Bundle, api_client, source: str) -> dict[s except Exception as e: # Transaction failed - savepoint will rollback automatically - _logger.error(f"Transaction entry {idx + 1} failed: {str(e)}", exc_info=True) + _log_entry_failure("Transaction", idx, e) raise ValidationError( f"Transaction failed at entry {idx + 1} ({entry.full_url or 'unknown'}): {str(e)}" ) from e @@ -154,10 +173,10 @@ def process_batch(self, bundle: Bundle, api_client, source: str) -> dict[str, An except Exception as e: # For batch, continue processing - just record the error - _logger.warning(f"Batch entry {idx + 1} failed: {str(e)}", exc_info=True) + _log_entry_failure("Batch", idx, e) # Create error response - error_response = self._create_error_response(e) + error_response = self._create_error_response(e, api_client) results.append( { @@ -371,9 +390,7 @@ def _handle_update(self, url: str, resource_data: dict, api_client, source: str) raise ValidationError("Client does not have permission to update individuals") # Find individual - partner = self.individual_service.find_by_identifier(system, value) - if not partner: - raise ValidationError(f"Individual not found: {system}|{value}") + partner = self._find_registrant(self.individual_service, "Individual", system, value, api_client) # Parse schema individual = Individual(**resource_data) @@ -397,9 +414,7 @@ def _handle_update(self, url: str, resource_data: dict, api_client, source: str) raise ValidationError("Client does not have permission to update groups") # Find group - group_record = self.group_service.find_by_identifier(system, value) - if not group_record: - raise ValidationError(f"Group not found: {system}|{value}") + group_record = self._find_registrant(self.group_service, "Group", system, value, api_client) # Parse schema group = Group(**resource_data) @@ -441,9 +456,7 @@ def _handle_read(self, url: str, api_client) -> dict[str, Any]: raise ValidationError("Client does not have permission to read individuals") # Find individual - partner = self.individual_service.find_by_identifier(system, value) - if not partner: - raise ValidationError(f"Individual not found: {system}|{value}") + partner = self._find_registrant(self.individual_service, "Individual", system, value, api_client) # Convert to API schema result = self.individual_service.to_api_schema(partner) @@ -461,9 +474,7 @@ def _handle_read(self, url: str, api_client) -> dict[str, Any]: raise ValidationError("Client does not have permission to read groups") # Find group - group_record = self.group_service.find_by_identifier(system, value) - if not group_record: - raise ValidationError(f"Group not found: {system}|{value}") + group_record = self._find_registrant(self.group_service, "Group", system, value, api_client) # Convert to API schema result = self.group_service.to_api_schema(group_record) @@ -499,9 +510,7 @@ def _handle_delete(self, url: str, api_client, source: str) -> dict[str, Any]: raise ValidationError("Client does not have permission to delete individuals") # Find individual - partner = self.individual_service.find_by_identifier(system, value) - if not partner: - raise ValidationError(f"Individual not found: {system}|{value}") + partner = self._find_registrant(self.individual_service, "Individual", system, value, api_client) # Soft delete (set active=False) partner.with_context(source_system=source).write({"active": False}) @@ -521,9 +530,7 @@ def _handle_delete(self, url: str, api_client, source: str) -> dict[str, Any]: raise ValidationError("Client does not have permission to delete groups") # Find group - group_record = self.group_service.find_by_identifier(system, value) - if not group_record: - raise ValidationError(f"Group not found: {system}|{value}") + group_record = self._find_registrant(self.group_service, "Group", system, value, api_client) # Soft delete (set active=False) group_record.with_context(source_system=source).write({"active": False}) @@ -540,6 +547,29 @@ def _handle_delete(self, url: str, api_client, source: str) -> dict[str, Any]: else: raise ValidationError(f"Unsupported resource type: {resource_type}") + def _find_registrant(self, service, resource_type: str, system: str, value: str, api_client): + """ + Find the one registrant an entry addresses, or raise "not found". + + An identifier held by several registrants is a 409 only for a client + that may read every match; anyone else gets the "not found" answer, + so it doesn't learn that the identifier exists. + + Raises: + AmbiguousIdentifierError: several registrants hold it and the + client may read them all + ValidationError: no registrant (as far as the client may know) + """ + try: + record = service.find_by_identifier(system, value) + except AmbiguousIdentifierError as e: + if ambiguous_identifier_status(self.env, api_client, e) != status.HTTP_403_FORBIDDEN: + raise + record = None + if not record: + raise ValidationError(f"{resource_type} not found: {system}|{value}") + return record + def _extract_identifier_from_result(self, result: dict[str, Any]) -> str | None: """ Extract the identifier from a result for placeholder mapping. @@ -569,18 +599,37 @@ def _extract_identifier_from_result(self, result: dict[str, Any]) -> str | None: return None - def _create_error_response(self, exception: Exception) -> dict[str, Any]: + def _create_error_response(self, exception: Exception, api_client) -> dict[str, Any]: """ Create error response from exception. Args: exception: Exception that occurred + api_client: client the bundle is answered to Returns: Dict with status and OperationOutcome """ # Determine status code based on exception type - if isinstance(exception, ValidationError): + if ( + isinstance(exception, AmbiguousIdentifierError) + and ambiguous_identifier_status(self.env, api_client, exception) == 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 + return { + "status": "403 Forbidden", + "outcome": { + "resourceType": "OperationOutcome", + "issue": [{"severity": "error", "code": "forbidden", "diagnostics": "Access denied"}], + }, + } + if isinstance(exception, AmbiguousIdentifierError | IdentifierInUseError): + # The identifier matches, or would match, more than one registrant + status_code = "409 Conflict" + severity = "error" + code = "conflict" + elif isinstance(exception, ValidationError): status_code = "422 Unprocessable Entity" severity = "error" code = "invalid" diff --git a/spp_api_v2/services/consent_service.py b/spp_api_v2/services/consent_service.py index 493ce52dc..80c29a99d 100644 --- a/spp_api_v2/services/consent_service.py +++ b/spp_api_v2/services/consent_service.py @@ -9,6 +9,17 @@ _logger = logging.getLogger(__name__) +# Legal bases that allow processing without the registrant's consent +# (GDPR Article 6); clients with any other basis are consent-filtered +NON_CONSENT_BASES = ( + "legal_obligation", + "vital_interest", + "public_interest", + "public_task", + "contract", + "legitimate_interest", +) + class ConsentService: """Service for applying consent-based filtering to API responses""" @@ -19,6 +30,11 @@ def __init__(self, env: Environment): self._ip_address = None self._user_agent = None + @staticmethod + def is_consent_filtered(api_client) -> bool: + """Whether responses to this client are filtered by registrant consent""" + return api_client.legal_basis not in NON_CONSENT_BASES + def set_request_context( self, request_id: str | None = None, @@ -107,15 +123,7 @@ def filter_response( # - public_task: Official authority (e.g., government agencies) # - contract: Contractual necessity (still needs a contract, not individual consent) # - legitimate_interest: Legitimate business interest (requires balancing test) - non_consent_bases = ( - "legal_obligation", - "vital_interest", - "public_interest", - "public_task", - "contract", - "legitimate_interest", - ) - if api_client.legal_basis in non_consent_bases: + if not self.is_consent_filtered(api_client): # Still apply client scope filtering for field-level access control filtered = self._apply_client_scope_filter(api_client, resource_type, data) return filtered @@ -448,8 +456,9 @@ def check_access(self, registrant_id: int, api_client, resource_type: str, actio # For read operations, check consent if action in ("read", "search"): - # Check if client requires consent - if api_client.is_require_consent: + # Same rule as filter_response: the legal basis decides whether + # the client is subject to consent + if self.is_consent_filtered(api_client): # Fast path: Use consent_summary cache for O(1) check if self._check_consent_fast(registrant_id, api_client): return True diff --git a/spp_api_v2/services/group_service.py b/spp_api_v2/services/group_service.py index d9982c5a1..84fac924c 100644 --- a/spp_api_v2/services/group_service.py +++ b/spp_api_v2/services/group_service.py @@ -5,7 +5,7 @@ from datetime import datetime, time from typing import Any -from odoo import Command, _ +from odoo import Command, _, fields from odoo.api import Environment from odoo.exceptions import AccessError, ValidationError @@ -14,10 +14,25 @@ from ..schemas.membership import MembershipHistoryEntry from ..schemas.patch import GroupPatch from .membership_utils import membership_to_response +from .registrant_resolver import ( + assert_new_identifiers_free, + live_registry_ids, + primary_registry_id, + resolve_registrant, + resolve_registrants, +) _logger = logging.getLogger(__name__) +class AlreadyMemberError(ValidationError): + """The individual is already a member of the group.""" + + +class NotMemberError(ValidationError): + """The individual is not a member of the group.""" + + class GroupService: """Service for Group resource CRUD and mapping""" @@ -33,25 +48,13 @@ def find_by_identifier(self, system_uri: str, value: str): value: Identifier value Returns: - res.partner record (group) or empty recordset (in sudo context) + res.partner record (group) or empty recordset (in sudo context - + API handlers run as Public user) + + Raises: + AmbiguousIdentifierError: several groups hold the identifier """ - reg_id = ( - self.env["spp.registry.id"] # nosemgrep: odoo-sudo-without-context - .sudo() - .search( - [ - ("id_type_id.uri", "=", system_uri), - ("value", "=", value), - ("partner_id.is_group", "=", True), - ], - limit=1, - ) - ) - if reg_id and reg_id.partner_id: - # Return sudo partner - API handlers run as Public user - # nosemgrep: odoo-sudo-without-context, odoo-sudo-on-sensitive-models - return self.env["res.partner"].sudo().browse(reg_id.partner_id.id) - return self.env["res.partner"].sudo() # nosemgrep: odoo-sudo-on-sensitive-models, odoo-sudo-without-context + return resolve_registrant(self.env, system_uri, value, is_group=True) def find_by_identifiers(self, identifiers: list[tuple[str, str]]): """ @@ -61,38 +64,11 @@ def find_by_identifiers(self, identifiers: list[tuple[str, str]]): identifiers: List of (system_uri, value) tuples Returns: - Dict mapping "system_uri|value" to res.partner record (or None) + Dict mapping "system_uri|value" to the res.partner record (sudo), + or to an AmbiguousIdentifierError when several groups hold it; + identifiers with no match are absent """ - if not identifiers: - return {} - - # Build OR domain for batch search - or_domains = [] - for system_uri, value in identifiers: - or_domains.append("&") - or_domains.append(("id_type_id.uri", "=", system_uri)) - or_domains.append(("value", "=", value)) - - # Combine with OR - if len(identifiers) > 1: - domain = ["|"] * (len(identifiers) - 1) + or_domains - else: - domain = or_domains - - # Add is_group filter - domain = ["&", ("partner_id.is_group", "=", True)] + domain - - reg_ids = self.env["spp.registry.id"].sudo().search(domain) # nosemgrep: odoo-sudo-without-context - - # Build result map - result = {} - for reg_id in reg_ids: - key = f"{reg_id.id_type_id.uri}|{reg_id.value}" - if reg_id.partner_id: - # nosemgrep: odoo-sudo-without-context, odoo-sudo-on-sensitive-models - result[key] = self.env["res.partner"].sudo().browse(reg_id.partner_id.id) - - return result + return resolve_registrants(self.env, identifiers, is_group=True) def to_api_schema(self, group, extensions=None) -> dict[str, Any]: """ @@ -110,7 +86,8 @@ def to_api_schema(self, group, extensions=None) -> dict[str, Any]: # Build identifier list identifiers = [] - for reg_id in group.reg_ids: + # Live IDs only: a soft-removed ID no longer resolves, so it is not offered as a key + for reg_id in live_registry_ids(group): # Use id_type_id.uri for full code URI (e.g., urn:openspp:vocab:id-type#household_id) # NOT namespace_uri which only returns vocabulary namespace if reg_id.id_type_id and reg_id.id_type_id.uri and reg_id.value: @@ -139,7 +116,8 @@ def to_api_schema(self, group, extensions=None) -> dict[str, Any]: # Members - filter by ended_date directly for reliability # (is_ended computed field may have timing issues in tests) - now = datetime.now() + # fields.Datetime.now(): the same second-precision clock as the is_ended compute + now = fields.Datetime.now() members = [] for membership in group.group_membership_ids.filtered(lambda m: not m.ended_date or m.ended_date > now): if membership.individual: @@ -200,13 +178,15 @@ def to_api_schema(self, group, extensions=None) -> dict[str, Any]: def _build_member(self, membership) -> dict[str, Any]: """Build GroupMember from spp.group.membership record""" individual = membership.individual - if not individual or not individual.reg_ids: + # A live ID: removed IDs don't resolve + primary_id = primary_registry_id(individual) if individual else None + if not primary_id: return None # Build reference to individual - primary_id = individual.reg_ids[0] entity_ref = { - "reference": f"Individual/{primary_id.namespace_uri}|{primary_id.value}", + # id_type_id.uri (full code URI), NOT namespace_uri, so the reference resolves + "reference": f"Individual/{primary_id.id_type_id.uri}|{primary_id.value}", "display": individual.name, } @@ -366,6 +346,7 @@ def create(self, schema: Group, source: str, api_authorized: bool = False) -> An ) vals = self.from_api_schema(schema) + assert_new_identifiers_free(self.env, vals.get("reg_ids", [])) # Add source tracking (if field exists) if hasattr(self.env["res.partner"], "_fields") and "source_system" in self.env["res.partner"]._fields: @@ -373,19 +354,24 @@ def create(self, schema: Group, source: str, api_authorized: bool = False) -> An if hasattr(self.env["res.partner"], "_fields") and "collection_method" in self.env["res.partner"]._fields: vals["collection_method"] = "api" - # Use sudo() for cross-program group creation while enforcing authorization above - group = ( - self.env["res.partner"] # nosemgrep: odoo-sudo-on-sensitive-models, odoo-sudo-without-context - # nosemgrep: odoo-sudo-without-context, odoo-sudo-on-sensitive-models - # Group creation is restricted by API scope verification - .sudo() - .with_context(source_system=source) - .create(vals) - ) + # The group and its members are created together or not at all: a + # member reference that fails to resolve (e.g. an ambiguous identifier) + # must not leave the group behind for callers that keep the + # transaction, such as batch bundles + with self.env.cr.savepoint(): + # Use sudo() for cross-program group creation while enforcing authorization above + group = ( + self.env["res.partner"] # nosemgrep: odoo-sudo-on-sensitive-models, odoo-sudo-without-context + # nosemgrep: odoo-sudo-without-context, odoo-sudo-on-sensitive-models + # Group creation is restricted by API scope verification + .sudo() + .with_context(source_system=source) + .create(vals) + ) - # Handle members separately - if schema.member: - self._create_members(group, schema.member, source) + # Handle members separately + if schema.member: + self._create_members(group, schema.member, source) # Log using external identifier, not database ID primary_id = group.reg_ids[0] if group.reg_ids else None @@ -479,35 +465,16 @@ def _find_individual(self, system_uri: str, value: str): system_uri: Full URI (e.g., urn:openspp:vocab:id-type#national_id) or namespace URI (e.g., urn:openspp:vocab:id-type) value: Identifier value + + Raises: + AmbiguousIdentifierError: several individuals hold the identifier """ # Try full URI first (id_type_id.uri = namespace#code) - reg_id = ( - self.env["spp.registry.id"] # nosemgrep: odoo-sudo-without-context - .sudo() - .search( - [ - ("id_type_id.uri", "=", system_uri), - ("value", "=", value), - ("partner_id.is_group", "=", False), - ], - limit=1, - ) - ) - if not reg_id and "#" not in system_uri: + partner = resolve_registrant(self.env, system_uri, value, is_group=False) + if not partner and "#" not in system_uri: # Fallback: try namespace_uri (without #code suffix) - reg_id = ( - self.env["spp.registry.id"] # nosemgrep: odoo-sudo-without-context - .sudo() - .search( - [ - ("namespace_uri", "=", system_uri), - ("value", "=", value), - ("partner_id.is_group", "=", False), - ], - limit=1, - ) - ) - return reg_id.partner_id if reg_id else self.env["res.partner"] + partner = resolve_registrant(self.env, system_uri, value, is_group=False, system_field="namespace_uri") + return partner def update(self, group, schema: Group, source: str) -> Any: """ @@ -646,6 +613,15 @@ def get_membership(self, group, individual): ) ) + def _unknown_role_message(self, role_coding): + """Error for a role code that doesn't exist, so a typo isn't silently dropped""" + return self.env._( + "Unknown membership role: %(system)s|%(code)s. Roles are vocabulary codes, " + "eg urn:openspp:vocab:group-membership-type|head", + system=role_coding.get("system"), + code=role_coding.get("code"), + ) + def add_member(self, group, individual, role_coding=None, start_date=None) -> dict[str, Any]: """ Add an individual to a group. @@ -660,12 +636,12 @@ def add_member(self, group, individual, role_coding=None, start_date=None) -> di Dictionary matching MembershipResponse schema Raises: - ValidationError: If individual is already a member + AlreadyMemberError: If individual is already a member """ # Check if individual is already a member existing = self.get_membership(group, individual) if existing: - raise ValidationError(_("Individual is already a member of this group")) + raise AlreadyMemberError(_("Individual is already a member of this group")) # Prepare membership values vals = { @@ -692,14 +668,9 @@ def add_member(self, group, individual, role_coding=None, start_date=None) -> di limit=1, ) ) - if vocab_code: - vals["membership_type_ids"] = [Command.link(vocab_code.id)] - else: - _logger.warning( - "Vocabulary code not found for role: %s|%s", - role_coding.get("system"), - role_coding.get("code"), - ) + if not vocab_code: + raise ValidationError(self._unknown_role_message(role_coding)) + vals["membership_type_ids"] = [Command.link(vocab_code.id)] # Create membership membership = ( @@ -732,12 +703,12 @@ def remove_member(self, group, individual, ended_date=None, reason=None) -> dict Dictionary matching MembershipResponse schema Raises: - ValidationError: If individual is not a member or already ended + NotMemberError: If individual is not a member or already ended """ # Find membership membership = self.get_membership(group, individual) if not membership: - raise ValidationError(_("Individual is not a member of this group")) + raise NotMemberError(_("Individual is not a member of this group")) # Set ended date if ended_date: @@ -745,7 +716,10 @@ def remove_member(self, group, individual, ended_date=None, reason=None) -> dict ended_date = datetime.fromisoformat(ended_date.replace("Z", "+00:00")).date() ended_datetime = datetime.combine(ended_date, time.min) else: - ended_datetime = datetime.now() + # fields.Datetime.now(), not datetime.now(): the is_ended/status + # computes compare against whole seconds, so a microsecond end + # time would be "in the future" and the row stored as active + ended_datetime = fields.Datetime.now() membership.sudo().write({"ended_date": ended_datetime}) # nosemgrep: odoo-sudo-without-context @@ -778,12 +752,12 @@ def update_member(self, group, individual, role_coding=None, start_date=None, en Dictionary matching MembershipResponse schema Raises: - ValidationError: If individual is not a member + NotMemberError: If individual is not a member """ # Find membership membership = self.get_membership(group, individual) if not membership: - raise ValidationError(_("Individual is not a member of this group")) + raise NotMemberError(_("Individual is not a member of this group")) # Prepare update values vals = {} @@ -813,15 +787,10 @@ def update_member(self, group, individual, role_coding=None, start_date=None, en limit=1, ) ) - if vocab_code: - # Replace existing roles with new role - vals["membership_type_ids"] = [Command.set([vocab_code.id])] - else: - _logger.warning( - "Vocabulary code not found for role: %s|%s", - role_coding.get("system"), - role_coding.get("code"), - ) + if not vocab_code: + raise ValidationError(self._unknown_role_message(role_coding)) + # Replace existing roles with new role + vals["membership_type_ids"] = [Command.set([vocab_code.id])] if vals: membership.sudo().write(vals) # nosemgrep: odoo-sudo-without-context @@ -883,7 +852,8 @@ def merge_groups(self, source_group, target_group, role_mapping=None) -> dict: ) # Process each source member - now = datetime.now() + # fields.Datetime.now(): second precision, so ended memberships are ended at once + now = fields.Datetime.now() for source_membership in source_members: individual = source_membership.individual @@ -1085,6 +1055,8 @@ def split_group(self, source_group, new_identifiers, members_to_move, new_head=N # Use existing create method's vals generation new_group_vals = self.from_api_schema(new_group_schema) + # Same rule as create(): no second registrant with someone else's identifier + assert_new_identifiers_free(self.env, new_group_vals.get("reg_ids", [])) # Add source tracking source_system = source or "urn:openspp:api-v2:split-operation" @@ -1097,7 +1069,8 @@ def split_group(self, source_group, new_identifiers, members_to_move, new_head=N new_group = self.env["res.partner"].sudo().create(new_group_vals) # Move members to new group - now = datetime.now() + # fields.Datetime.now(): second precision, so ended memberships are ended at once + now = fields.Datetime.now() for individual in members_to_move: membership = membership_by_individual[individual.id] @@ -1201,13 +1174,15 @@ def get_membership_history(self, group, limit=100, offset=0, since=None) -> list # Build history entries for membership in memberships: individual = membership.individual - if not individual or not individual.reg_ids: + # A live ID: removed IDs don't resolve + primary_id = primary_registry_id(individual) if individual else None + if not primary_id: continue # Build individual reference - primary_id = individual.reg_ids[0] member_ref = Reference( - reference=f"Individual/{primary_id.namespace_uri}|{primary_id.value}", + # id_type_id.uri (full code URI), NOT namespace_uri, so the reference resolves + reference=f"Individual/{primary_id.id_type_id.uri}|{primary_id.value}", display=individual.name, ) diff --git a/spp_api_v2/services/individual_service.py b/spp_api_v2/services/individual_service.py index b50479c35..83a63bd2b 100644 --- a/spp_api_v2/services/individual_service.py +++ b/spp_api_v2/services/individual_service.py @@ -14,9 +14,19 @@ from ..schemas.individual import Individual from ..schemas.patch import IndividualPatch from .membership_utils import membership_to_response +from .registrant_resolver import ( + assert_new_identifiers_free, + live_registry_ids, + primary_registry_id, + resolve_registrant, + resolve_registrants, +) _logger = logging.getLogger(__name__) +# The vocabulary gender codes come from (gender_id's domain) +GENDER_SYSTEM = "urn:iso:std:iso:5218" + class IndividualService: """Service for Individual resource CRUD and mapping""" @@ -24,6 +34,30 @@ class IndividualService: def __init__(self, env: Environment): self.env = env + def _resolve_gender_code(self, system, code): + """ + Resolve a gender coding to its vocabulary code. + + Only codes of the ISO 5218 vocabulary are genders: an unknown code, or + a code from another vocabulary (eg a membership role), is a + ValidationError. + """ + gender_code = self.env["spp.vocabulary.code"] + if system == GENDER_SYSTEM: + # The API runs as the public user; vocabularies are public reference data + gender_code = ( + self.env["spp.vocabulary.code"] # nosemgrep: odoo-sudo-without-context + .sudo() + .search([("namespace_uri", "=", GENDER_SYSTEM), ("code", "=", code)], limit=1) + ) + if not gender_code: + raise ValidationError( + f"Invalid gender code: system='{system}', code='{code}'. " + f"Expected system '{GENDER_SYSTEM}' with codes: " + f"0 (Not Known), 1 (Male), 2 (Female), 9 (Not Applicable)." + ) + return gender_code + def find_by_identifier(self, system_uri: str, value: str): """ Lookup partner by external identifier. @@ -35,25 +69,13 @@ def find_by_identifier(self, system_uri: str, value: str): value: Identifier value Returns: - res.partner record or empty recordset (in sudo context) + res.partner record or empty recordset (in sudo context - API + handlers run as Public user) + + Raises: + AmbiguousIdentifierError: several registrants hold the identifier """ - reg_id = ( - self.env["spp.registry.id"] # nosemgrep: odoo-sudo-without-context — API auth - .sudo() - .search( - [ - ("id_type_id.uri", "=", system_uri), - ("value", "=", value), - ], - limit=1, - ) - ) - if reg_id and reg_id.partner_id: - # Return sudo partner - API handlers run as Public user - # nosemgrep: odoo-sudo-on-sensitive-models, odoo-sudo-without-context — API auth - return self.env["res.partner"].sudo().browse(reg_id.partner_id.id) - # nosemgrep: odoo-sudo-on-sensitive-models, odoo-sudo-without-context - return self.env["res.partner"].sudo() + return resolve_registrant(self.env, system_uri, value, is_group=False) def find_by_identifiers(self, identifiers: list[tuple[str, str]]): """ @@ -63,36 +85,11 @@ def find_by_identifiers(self, identifiers: list[tuple[str, str]]): identifiers: List of (system_uri, value) tuples Returns: - Dict mapping "system_uri|value" to res.partner record (or None) + Dict mapping "system_uri|value" to the res.partner record (sudo), + or to an AmbiguousIdentifierError when several registrants hold + it; identifiers with no match are absent """ - if not identifiers: - return {} - - # Build OR domain for batch search - or_domains = [] - for system_uri, value in identifiers: - or_domains.append("&") - or_domains.append(("id_type_id.uri", "=", system_uri)) - or_domains.append(("value", "=", value)) - - # Combine with OR: need (n-1) | operators for n terms - if len(identifiers) > 1: - domain = ["|"] * (len(identifiers) - 1) + or_domains - else: - domain = or_domains - - # nosemgrep: odoo-sudo-without-context — API auth; batch identifier lookup - reg_ids = self.env["spp.registry.id"].sudo().search(domain) - - # Build result map - result = {} - for reg_id in reg_ids: - key = f"{reg_id.id_type_id.uri}|{reg_id.value}" - if reg_id.partner_id: - # nosemgrep: odoo-sudo-on-sensitive-models, odoo-sudo-without-context — API auth - result[key] = self.env["res.partner"].sudo().browse(reg_id.partner_id.id) - - return result + return resolve_registrants(self.env, identifiers, is_group=False) def to_api_schema(self, partner, extensions=None) -> dict[str, Any]: """ @@ -116,7 +113,8 @@ def to_api_schema(self, partner, extensions=None) -> dict[str, Any]: # Build identifier list (REQUIRED, at least one) identifiers = [] - for reg_id in partner.reg_ids: + # Live IDs only: a soft-removed ID no longer resolves, so it is not offered as a key + for reg_id in live_registry_ids(partner): # Use id_type_id.uri for full code URI (e.g., urn:openspp:vocab:id-type#national_id) # NOT namespace_uri which only returns vocabulary namespace if reg_id.id_type_id and reg_id.id_type_id.uri and reg_id.value: @@ -280,10 +278,11 @@ def to_api_schema(self, partner, extensions=None) -> dict[str, Any]: def _build_group_reference(self, group) -> dict: """Build Reference to a Group""" - # Get primary identifier for group - if group.reg_ids: - primary_id = group.reg_ids[0] - ref = f"Group/{primary_id.namespace_uri}|{primary_id.value}" + # Get primary identifier for group (a live one: removed IDs don't resolve) + primary_id = primary_registry_id(group) + if primary_id: + # id_type_id.uri (full code URI), NOT namespace_uri, so the reference resolves + ref = f"Group/{primary_id.id_type_id.uri}|{primary_id.value}" else: # No identifier - this should not happen in a properly configured system _logger.error( @@ -352,26 +351,7 @@ def from_api_schema(self, schema: Individual) -> dict[str, Any]: # Gender - CRITICAL: Find gender_id by namespace_uri + code if schema.gender and schema.gender.coding: gender_coding = schema.gender.coding[0] - gender_code = ( - self.env["spp.vocabulary.code"] # nosemgrep: odoo-sudo-without-context - .sudo() - .search( - [ - ("namespace_uri", "=", gender_coding.system), - ("code", "=", gender_coding.code), - ], - limit=1, - ) - ) - if gender_code: - vals["gender_id"] = gender_code.id - else: - # Gender code not found - raise validation error with helpful message - raise ValidationError( - f"Invalid gender code: system='{gender_coding.system}', code='{gender_coding.code}'. " - f"Expected system 'urn:iso:std:iso:5218' with codes: " - f"0 (Not Known), 1 (Male), 2 (Female), 9 (Not Applicable)." - ) + vals["gender_id"] = self._resolve_gender_code(gender_coding.system, gender_coding.code).id # Telecom for contact in schema.telecom or []: @@ -497,6 +477,7 @@ def create(self, schema: Individual, source: str, api_authorized: bool = False) ) vals = self.from_api_schema(schema) + assert_new_identifiers_free(self.env, vals.get("reg_ids", [])) # Add source tracking (ADR-008) - if fields exist if hasattr(self.env["res.partner"], "_fields") and "source_system" in self.env["res.partner"]._fields: @@ -608,15 +589,10 @@ def partial_update(self, partner, patch: IndividualPatch, source: str) -> Any: if gender: if gender.get("coding"): gender_coding = gender["coding"][0] - gender_code = self.env["spp.vocabulary.code"].search( - [ - ("namespace_uri", "=", gender_coding.get("system")), - ("code", "=", gender_coding.get("code")), - ], - limit=1, - ) - if gender_code: - vals["gender_id"] = gender_code.id + # As on create: an unknown code is an error, not a no-op + vals["gender_id"] = self._resolve_gender_code( + gender_coding.get("system"), gender_coding.get("code") + ).id else: # RFC 7396: null clears the field vals["gender_id"] = False diff --git a/spp_api_v2/services/membership_utils.py b/spp_api_v2/services/membership_utils.py index fba7ff8ad..b9cdb88d5 100644 --- a/spp_api_v2/services/membership_utils.py +++ b/spp_api_v2/services/membership_utils.py @@ -5,6 +5,8 @@ from odoo.exceptions import ValidationError +from .registrant_resolver import primary_registry_id + def membership_to_response(membership) -> dict[str, Any]: """ @@ -24,23 +26,25 @@ def membership_to_response(membership) -> dict[str, Any]: """ # Build group reference group = membership.group - group_id = group.reg_ids[0] if group.reg_ids else None + group_id = primary_registry_id(group) if not group_id: raise ValidationError(f"Group {group.name} has no valid external identifiers") + # id_type_id.uri (full code URI), NOT namespace_uri: the reference must + # resolve through the identifier lookups, which match on the code URI group_ref = { - "reference": f"Group/{group_id.namespace_uri}|{group_id.value}", + "reference": f"Group/{group_id.id_type_id.uri}|{group_id.value}", "display": group.name, } # Build individual reference individual = membership.individual - individual_id = individual.reg_ids[0] if individual.reg_ids else None + individual_id = primary_registry_id(individual) if not individual_id: raise ValidationError(f"Individual {individual.name} has no valid external identifiers") individual_ref = { - "reference": f"Individual/{individual_id.namespace_uri}|{individual_id.value}", + "reference": f"Individual/{individual_id.id_type_id.uri}|{individual_id.value}", "display": individual.name, } diff --git a/spp_api_v2/services/registrant_resolver.py b/spp_api_v2/services/registrant_resolver.py new file mode 100644 index 000000000..61a02d762 --- /dev/null +++ b/spp_api_v2/services/registrant_resolver.py @@ -0,0 +1,141 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Resolve an external identifier to exactly one registrant. + +The registry allows two registrants to hold the same ID type and value +(imported data awaiting deduplication; see the ID-document deduplication +manager in spp_programs), but an API lookup addresses one record. These +helpers never pick one of several matches: they raise, and the API refuses. + +Only *live* IDs count. A soft-removed ID (``status = 'invalid'``) neither +resolves nor blocks reuse; a NULL status is an ID added straight through +the registry and is live, the same rule as ``spp.registry.id``'s own +uniqueness index. +""" + +from odoo.exceptions import ValidationError + +LIVE_ID = ("status", "!=", "invalid") + + +class AmbiguousIdentifierError(Exception): + """The identifier is live on more than one registrant.""" + + def __init__(self, partners): + super().__init__("Identifier matches more than one registrant") + self.partners = partners + + +class IdentifierInUseError(ValidationError): + """The identifier is already live on another registrant.""" + + +def live_registry_ids(partner): + """The registrant's IDs that resolve (not soft-removed), in ``reg_ids`` order.""" + return partner.reg_ids.filtered(lambda reg_id: reg_id.status != "invalid") + + +def primary_registry_id(partner): + """The ID references to this registrant are built from: its first live one (or empty).""" + return live_registry_ids(partner)[:1] + + +def _registry_ids(env): + # sudo: resolution is about the registry's data, not the API user's + # visibility; consent and scope are enforced by the caller + return env["spp.registry.id"].sudo() # nosemgrep: odoo-sudo-without-context + + +def resolve_registrant(env, system_uri, value, is_group=None, system_field="id_type_id.uri"): + """ + Return the one registrant holding a live ID ``system_uri|value``. + + Args: + env: Odoo environment + system_uri: Full code URI of the ID type (e.g. urn:openspp:vocab:id-type#national_id) + value: Identifier value + is_group: True/False to restrict to groups/individuals, None for either + system_field: registry ID field ``system_uri`` is matched on + + Returns: + res.partner record (sudo), or an empty recordset + + Raises: + AmbiguousIdentifierError: several registrants hold the identifier + """ + domain = [(system_field, "=", system_uri), ("value", "=", value), LIVE_ID] + if is_group is not None: + domain.append(("partner_id.is_group", "=", is_group)) + partners = _registry_ids(env).search(domain).partner_id + if len(partners) > 1: + raise AmbiguousIdentifierError(partners) + return partners + + +def resolve_registrants(env, identifiers, is_group=None): + """ + Batch form of :func:`resolve_registrant`. + + Args: + identifiers: list of (system_uri, value) tuples + + Returns: + dict mapping "system_uri|value" to the res.partner record, or to an + AmbiguousIdentifierError when several registrants hold it; keys with + no match are absent + """ + if not identifiers: + return {} + or_domains = [] + for system_uri, value in identifiers: + or_domains += ["&", ("id_type_id.uri", "=", system_uri), ("value", "=", value)] + domain = ["|"] * (len(identifiers) - 1) + or_domains + domain = ["&", LIVE_ID] + domain + if is_group is not None: + domain = ["&", ("partner_id.is_group", "=", is_group)] + domain + + matches = {} + for reg_id in _registry_ids(env).search(domain): + key = f"{reg_id.id_type_id.uri}|{reg_id.value}" + # reg_id is sudo, so its partner_id is too: the union stays in sudo + matches[key] = matches[key] | reg_id.partner_id if key in matches else reg_id.partner_id + return { + key: partners if len(partners) == 1 else AmbiguousIdentifierError(partners) for key, partners in matches.items() + } + + +def assert_identifier_free(env, id_type, value): + """ + Raise if any registrant holds a live ID of ``id_type`` with ``value``. + + Only creates call this: the API's updates drop ``reg_ids`` from the + values they write, so an update never adds an identifier. + + Args: + id_type: spp.vocabulary.code record of the ID type + value: Identifier value + + Raises: + IdentifierInUseError: another registrant holds the identifier + """ + domain = [("id_type_id", "=", id_type.id), ("value", "=", value), LIVE_ID] + if _registry_ids(env).search_count(domain, limit=1): + # The ID type is enough for the client to act on. The value (often a + # national ID) stays out: this message reaches the server log + raise IdentifierInUseError(f"Identifier of type {id_type.uri} is already in use by another registrant") + + +def assert_new_identifiers_free(env, reg_id_commands): + """ + Check the ``reg_ids`` create commands of a new registrant. + + A second registrant with an identifier someone else holds could never be + addressed unambiguously afterwards, so creation is refused instead. + + Raises: + IdentifierInUseError: another registrant holds one of the identifiers + """ + # nosemgrep: odoo-sudo-without-context + id_types = env["spp.vocabulary.code"].sudo() + for command in reg_id_commands: + reg_vals = command[2] + assert_identifier_free(env, id_types.browse(reg_vals["id_type_id"]), reg_vals["value"]) diff --git a/spp_api_v2/services/search_service.py b/spp_api_v2/services/search_service.py index 371c8e8d6..6c430e030 100644 --- a/spp_api_v2/services/search_service.py +++ b/spp_api_v2/services/search_service.py @@ -8,8 +8,19 @@ from odoo.api import Environment from odoo.osv import expression +from ..utils.pagination import MAX_ROWS_PER_QUERY +from .registrant_resolver import LIVE_ID, resolve_registrant + _logger = logging.getLogger(__name__) +# A filter that names nothing (unknown group, role, gender, member) must +# narrow the result to nothing, never drop out and widen it. +MATCH_NOTHING = [("id", "=", 0)] + + +class InvalidSearchParam(ValueError): + """A search parameter is malformed; the API answers 400.""" + class SearchService: """Service for search parameter parsing and execution""" @@ -63,11 +74,16 @@ def search_individuals(self, params: dict[str, Any]) -> tuple[list, int]: ] domain.extend(addr_domain) - if params.get("group"): - domain.extend(self._parse_group_param(params["group"])) - - if params.get("membership-role"): - domain.extend(self._parse_membership_role_param(params["membership-role"])) + group = params.get("group") + role = params.get("membership-role") + if group and role and group.lower() != "none": + # Together they must hold on the same membership + domain.extend(self._parse_group_and_role_params(group, role)) + else: + if group: + domain.extend(self._parse_group_param(group)) + if role: + domain.extend(self._parse_membership_role_param(role)) if params.get("_lastUpdated"): domain.extend(self._parse_date_param("write_date", params["_lastUpdated"])) @@ -88,7 +104,7 @@ def search_individuals(self, params: dict[str, Any]) -> tuple[list, int]: _logger.warning("Invalid _lastId parameter: %s, skipping cursor pagination", last_id) # Apply pagination - limit = min(int(params.get("_count", 20)), 100) # Max 100 + limit = min(int(params.get("_count", 20)), MAX_ROWS_PER_QUERY) offset = int(params.get("_offset", 0)) # Apply sorting - always include id for consistent cursor pagination @@ -111,6 +127,7 @@ def search_groups(self, params: dict[str, Any]) -> tuple[list, int]: - type: household/family/organization/other - member: Individual reference - _count: int + - _offset: int (skip records) - _lastId: int (cursor-based pagination) Returns: @@ -125,35 +142,7 @@ def search_groups(self, params: dict[str, Any]) -> tuple[list, int]: domain.append(("name", "ilike", params["name"])) if params.get("member"): - # Search for groups with specific member - member_ref = params["member"] - if member_ref.startswith("Individual/"): - ident_str = member_ref.replace("Individual/", "") - if "|" in ident_str: - system, value = ident_str.split("|", 1) - # Find individual first - # Use id_type_id.uri (full URI with code) instead of namespace_uri - # (which only contains the vocabulary namespace) - reg_id = ( - self.env["spp.registry.id"] # nosemgrep: odoo-sudo-without-context - .sudo() - .search( - [ - ("id_type_id.uri", "=", system), - ("value", "=", value), - ], - limit=1, - ) - ) - if reg_id: - # Search via group_membership_ids relationship - domain.append( - ( - "group_membership_ids.individual", - "=", - reg_id.partner_id.id, - ) - ) + domain.extend(self._parse_member_param(params["member"])) # Execute search with sudo() to access registry.id via domain Partner = self.env["res.partner"] @@ -168,12 +157,14 @@ def search_groups(self, params: dict[str, Any]) -> tuple[list, int]: except (ValueError, TypeError): _logger.warning("Invalid _lastId parameter: %s, skipping cursor pagination", last_id) - limit = min(int(params.get("_count", 20)), 100) + limit = min(int(params.get("_count", 20)), MAX_ROWS_PER_QUERY) + offset = int(params.get("_offset", 0)) order = self._parse_sort_param(params.get("_sort")) if "id" not in order.lower(): order = f"{order}, id" - records = Partner.sudo().search(domain, limit=limit, order=order) # nosemgrep: odoo-sudo-without-context + # nosemgrep: odoo-sudo-without-context + records = Partner.sudo().search(domain, limit=limit, offset=offset, order=order) return records, total @@ -184,16 +175,24 @@ def _parse_identifier_param(self, identifier: str) -> list: Returns Odoo domain for identifier search. """ if "|" not in identifier: - _logger.warning("Invalid identifier format: %s", identifier) - return [] + raise InvalidSearchParam("Invalid identifier format. Expected: {system}|{value}") system, value = identifier.split("|", 1) # Search via registry.id with id_type_id.uri (full code URI) # Format: urn:openspp:vocab:id-type#national_id|VALUE + # "any" keeps both conditions on the same registry ID; two dotted + # leaves would match a system from one ID and a value from another. return [ - ("reg_ids.id_type_id.uri", "=", system), - ("reg_ids.value", "=", value), + ( + "reg_ids", + "any", + [ + ("id_type_id.uri", "=", system), + ("value", "=", value), + LIVE_ID, + ], + ) ] def _parse_date_param(self, field: str, value: str) -> list: @@ -229,9 +228,10 @@ def _parse_date_param(self, field: str, value: str) -> list: # Try to parse as date parsed_date = datetime.strptime(date_str, "%Y-%m-%d").date() return [(field, operator, parsed_date)] - except ValueError: - _logger.warning("Invalid date format: %s", date_str) - return [] + except ValueError as e: + raise InvalidSearchParam( + f"Invalid date for {field}: expected an optional prefix (eq, ne, gt, ge, lt, le) and YYYY-MM-DD" + ) from e def _parse_gender_param(self, gender: str) -> list: """ @@ -240,7 +240,7 @@ def _parse_gender_param(self, gender: str) -> list: Returns domain for gender_id search. """ if "|" not in gender: - return [] + raise InvalidSearchParam("Invalid gender format. Expected: {system}|{code}") system, code = gender.split("|", 1) @@ -260,7 +260,7 @@ def _parse_gender_param(self, gender: str) -> list: if gender_code: return [("gender_id", "=", gender_code.id)] - return [] + return MATCH_NOTHING def _parse_group_param(self, group: str) -> list: """ @@ -280,36 +280,21 @@ def _parse_group_param(self, group: str) -> list: return [("id", "not in", membered_individual_ids)] # Standard case: filter by specific group - if "|" not in group: - _logger.warning("Invalid group identifier format: %s", group) - return [] + group_partner = self._resolve_group_filter(group) + if not group_partner: + return MATCH_NOTHING - system, value = group.split("|", 1) - - # Find group by identifier - # Use id_type_id.uri (full URI with code) instead of namespace_uri - # (which only contains the vocabulary namespace) - reg_id = ( - self.env["spp.registry.id"] # nosemgrep: odoo-sudo-without-context - .sudo() - .search( + # Find individuals with an active membership in this group; "any" + # keeps both conditions on the same membership row + return [ + ( + "individual_membership_ids", + "any", [ - ("id_type_id.uri", "=", system), - ("value", "=", value), + ("group", "=", group_partner.id), + ("is_ended", "=", False), ], - limit=1, ) - ) - - if not reg_id or not reg_id.partner_id.is_group: - _logger.warning("Group not found for identifier: %s", group) - return [] - - # Find individuals who are members of this group - # Use individual_membership_ids relationship - return [ - ("individual_membership_ids.group", "=", reg_id.partner_id.id), - ("individual_membership_ids.is_ended", "=", False), ] def _parse_membership_role_param(self, role: str) -> list: @@ -321,8 +306,64 @@ def _parse_membership_role_param(self, role: str) -> list: if not role: return [] - # Find role code in vocabulary - role_code = ( + role_code = self._resolve_role_filter(role) + if not role_code: + return MATCH_NOTHING + + # Find individuals holding this role on an active membership; "any" + # keeps both conditions on the same membership row + return [ + ( + "individual_membership_ids", + "any", + [ + ("membership_type_ids", "in", role_code.id), + ("is_ended", "=", False), + ], + ) + ] + + def _parse_group_and_role_params(self, group: str, role: str) -> list: + """ + Parse group and membership-role given together. + + Returns domain for individuals holding the role on an active + membership of that group: one "any" keeps the group, the role and + the active check on the same membership row. + """ + group_partner = self._resolve_group_filter(group) + role_code = self._resolve_role_filter(role) + if not group_partner or not role_code: + return MATCH_NOTHING + + return [ + ( + "individual_membership_ids", + "any", + [ + ("group", "=", group_partner.id), + ("membership_type_ids", "in", role_code.id), + ("is_ended", "=", False), + ], + ) + ] + + def _resolve_group_filter(self, group: str): + """ + Resolve a group filter (format: system|value) to the group, or an + empty recordset when no group holds it. + """ + if "|" not in group: + raise InvalidSearchParam('Invalid group format. Expected: {system}|{value} or "none"') + + system, value = group.split("|", 1) + + # Raises AmbiguousIdentifierError when several groups hold it + return resolve_registrant(self.env, system, value, is_group=True) + + def _resolve_role_filter(self, role: str): + """Resolve a membership-role code to its vocabulary code, or an empty recordset""" + return ( self.env["spp.vocabulary.code"] # nosemgrep: odoo-sudo-without-context .sudo() .search( @@ -334,14 +375,32 @@ def _parse_membership_role_param(self, role: str) -> list: ) ) - if not role_code: - _logger.warning("Membership role not found: %s", role) - return [] + def _parse_member_param(self, member: str) -> list: + """ + Parse the groups ``member`` parameter (format: Individual/{system}|{value}). + + Returns domain for groups with an active membership of that individual. + """ + ident_str = member.removeprefix("Individual/") + if ident_str == member or "|" not in ident_str: + raise InvalidSearchParam("Invalid member format. Expected: Individual/{system}|{value}") + + system, value = ident_str.split("|", 1) + + # Raises AmbiguousIdentifierError when several individuals hold it + member_partner = resolve_registrant(self.env, system, value, is_group=False) + if not member_partner: + return MATCH_NOTHING - # Find individuals with this membership role in any active group return [ - ("individual_membership_ids.membership_type_ids", "in", role_code.id), - ("individual_membership_ids.is_ended", "=", False), + ( + "group_membership_ids", + "any", + [ + ("individual", "=", member_partner.id), + ("is_ended", "=", False), + ], + ) ] def _parse_sort_param(self, sort: str | None) -> str: diff --git a/spp_api_v2/static/description/index.html b/spp_api_v2/static/description/index.html index 433b44e60..5a193f8a6 100644 --- a/spp_api_v2/static/description/index.html +++ b/spp_api_v2/static/description/index.html @@ -517,28 +517,154 @@

Dependencies

Changelog

-

19.0.2.1.1

+

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)
-
-

19.0.2.1.0

+
+

19.0.2.1.0

  • Add OpenAPI polymorphic schema utilities (utils/openapi_polymorphic.py): polymorphic_body() for @@ -563,8 +689,8 @@

    19.0.2.1.0

    polymorphic utilities, and the overall OpenAPI document contract
-
-

19.0.2.0.1

+
+

19.0.2.0.1

  • Fix SerializationFailure race when multiple Odoo workers rebuild their routing map simultaneously (e.g. after -u all) and all try @@ -579,15 +705,15 @@

    19.0.2.0.1

    regressions are diagnosable without raising the global log level
-
-

19.0.2.0.0

+
+

19.0.2.0.0

  • Initial migration to OpenSPP2
-

Bug Tracker

+

Bug Tracker

Bugs are tracked on GitHub Issues. In case of trouble, please check there if your issue has already been reported. If you spotted it first, help us to smash it by providing a detailed and welcomed @@ -595,7 +721,7 @@

Bug Tracker

Do not contact contributors directly about support or help with technical issues.

diff --git a/spp_api_v2/tests/__init__.py b/spp_api_v2/tests/__init__.py index 0a9e53c0d..3a0e9c4e2 100644 --- a/spp_api_v2/tests/__init__.py +++ b/spp_api_v2/tests/__init__.py @@ -37,3 +37,11 @@ from . import test_scope_enforcement from . import test_schema_builder from . import test_search_service +from . import test_search_filters_fail_closed +from . import test_references_resolvable +from . import test_membership_end_now +from . import test_identifier_ambiguity +from . import test_identifier_ambiguity_paths +from . import test_consent_paging +from . import test_consent_predicate +from . import test_group_member_errors diff --git a/spp_api_v2/tests/test_consent_paging.py b/spp_api_v2/tests/test_consent_paging.py new file mode 100644 index 000000000..ee40c2e4f --- /dev/null +++ b/spp_api_v2/tests/test_consent_paging.py @@ -0,0 +1,297 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Consent-filtered paging neither skips records nor stops early (#554 item M). + +When consent filtering hides records, a page is filled from the rows after +them. The next page must start right after the last row actually examined, +and a page cut short by the over-fetch safety limit must still link to the +next page while rows remain. +""" + +import json + +from odoo.tests.common import BaseCase + +from ..utils import pagination +from ..utils.pagination import fetch_with_consent +from .common import ApiV2HttpTestCase + +NATIONAL_ID = "urn:openspp:vocab:id-type#test_national_id" +HOUSEHOLD_ID = "urn:openspp:vocab:id-type#test_household_id" + +# The search services return at most this many rows per query +SERVICE_MAX_ROWS = 100 + + +def _search_over(rows): + """A search function over a list of rows that caps each query like the services do""" + + def search_function(offset, limit): + limit = min(limit, SERVICE_MAX_ROWS) + return rows[offset : offset + limit], len(rows) + + return search_function + + +class TestFetchWithConsent(BaseCase): + """utils.pagination.fetch_with_consent""" + + def test_consumed_offset_counts_only_examined_rows(self): + """The page fills partway through a batch: rows after the last one used are not consumed""" + rows = list(range(10)) + denied = {1} + + collected, consumed, _total, consent_applied = fetch_with_consent( + _search_over(rows), lambda row: None if row in denied else row, count=2, offset=0 + ) + + self.assertEqual(collected, [0, 2]) + self.assertTrue(consent_applied) + self.assertEqual(consumed, 3, "The next page must start at row 3, the first row not examined") + + def test_page_larger_than_half_the_service_cap_is_filled(self): + """count > 50: a batch capped at the service maximum is not mistaken for the end of the data""" + rows = list(range(500)) + + collected, _consumed, _total, _applied = fetch_with_consent( + _search_over(rows), lambda row: None if row % 2 else row, count=60, offset=0 + ) + + self.assertEqual(len(collected), 60) + self.assertEqual(collected, list(range(0, 120, 2))) + + def test_pages_cover_every_visible_row_exactly_once(self): + """Following the consumed offset visits every consented row once""" + rows = list(range(37)) + denied = {1, 2, 5, 11, 12, 13, 20, 36} + seen = [] + offset = 0 + + for _page in range(50): + collected, offset, total, _applied = fetch_with_consent( + _search_over(rows), lambda row: None if row in denied else row, count=4, offset=offset + ) + seen.extend(collected) + if offset >= total: + break + + self.assertEqual(seen, [row for row in rows if row not in denied]) + + +class TestPageTotalAndNext(BaseCase): + """utils.pagination.page_total_and_next""" + + def test_consent_client_total_is_the_page_size(self): + """Hidden rows are never counted, even when none were met in this window""" + total, next_offset = pagination.page_total_and_next( + returned=1, count=1, offset=0, db_offset_consumed=1, raw_total=6, consent_filtered=True + ) + self.assertEqual(total, 1) + self.assertEqual(next_offset, 1) + + def test_consent_client_past_the_end(self): + """Past the end: nothing returned, total 0, no next, whatever the raw count""" + total, next_offset = pagination.page_total_and_next( + returned=0, count=1, offset=1, db_offset_consumed=1, raw_total=1, consent_filtered=True + ) + self.assertEqual(total, 0) + self.assertIsNone(next_offset) + + def test_consent_client_next_until_the_scan_ends(self): + """A short page links on while rows remain, and not once the scan reached the end""" + self.assertEqual( + pagination.page_total_and_next( + returned=0, count=1, offset=0, db_offset_consumed=3, raw_total=5, consent_filtered=True + )[1], + 3, + ) + self.assertIsNone( + pagination.page_total_and_next( + returned=1, count=1, offset=3, db_offset_consumed=5, raw_total=5, consent_filtered=True + )[1] + ) + + def test_legal_basis_client_unchanged(self): + """Without consent filtering: the real total, next while pages are full""" + self.assertEqual( + pagination.page_total_and_next( + returned=2, count=2, offset=0, db_offset_consumed=2, raw_total=7, consent_filtered=False + ), + (7, 2), + ) + self.assertEqual( + pagination.page_total_and_next( + returned=1, count=2, offset=6, db_offset_consumed=7, raw_total=7, consent_filtered=False + ), + (7, None), + ) + + +class TestConsentTotalNotLeakedAPI(ApiV2HttpTestCase): + """HTTP: the total never counts records the client has no consent for""" + + def setUp(self): + super().setUp() + scopes = [ + {"resource": "individual", "action": "read"}, + {"resource": "group", "action": "read"}, + ] + self.consent_client = self.create_api_client(name="Total Consent Client", scopes=scopes) + self.consent_token = self.generate_jwt_token(self.consent_client) + self.legal_client = self.create_api_client( + name="Total Legal Client", scopes=scopes, require_consent=False, legal_basis="public_task" + ) + self.legal_token = self.generate_jwt_token(self.legal_client) + + def _body(self, url, token): + response = self.url_open(url, headers={"Authorization": f"Bearer {token}"}) + self.assertEqual(response.status_code, 200, response.content) + return json.loads(response.content) + + def _assert_nothing(self, body): + self.assertEqual(body["data"], []) + self.assertEqual(body["meta"]["total"], 0) + self.assertIsNone(body["links"].get("next")) + + def test_individual_hidden_match_past_the_end_is_not_counted(self): + """identifier= of a registrant without consent, _offset=1: same answer as an unknown identifier""" + self.create_test_individual(name="Hidden Total", identifier_value="HT-IND-1") + + hidden = self._body( + f"/api/v2/spp/Individual?identifier={NATIONAL_ID.replace('#', '%23')}|HT-IND-1&_count=1&_offset=1", + self.consent_token, + ) + unknown = self._body( + f"/api/v2/spp/Individual?identifier={NATIONAL_ID.replace('#', '%23')}|HT-NOBODY&_count=1&_offset=1", + self.consent_token, + ) + + self._assert_nothing(hidden) + self._assert_nothing(unknown) + + def test_group_hidden_match_past_the_end_is_not_counted(self): + """identifier= of a group without consent, _offset=1: same answer as an unknown identifier""" + self.create_test_group(name="Hidden Total Group", identifier_value="HT-GRP-1") + + hidden = self._body( + f"/api/v2/spp/Group?identifier={HOUSEHOLD_ID.replace('#', '%23')}|HT-GRP-1&_count=1&_offset=1", + self.consent_token, + ) + + self._assert_nothing(hidden) + + def test_page_filled_before_hidden_rows_does_not_count_them(self): + """A is consented and fills the page; the hidden B, C, D after it are not in the total""" + for letter in "ABCD": + person = self.create_test_individual(name=f"Filled First {letter}", identifier_value=f"FF-{letter}") + if letter == "A": + self.create_consent( + registrant=person, + grantee_partner=self.consent_client.partner_id, + resource_type="individual", + field_access="all", + ) + + body = self._body("/api/v2/spp/Individual?name=Filled+First&_count=1", self.consent_token) + + self.assertEqual([r["identifier"][0]["value"] for r in body["data"]], ["FF-A"]) + self.assertEqual(body["meta"]["total"], 1) + + def test_legal_basis_client_gets_the_real_total(self): + """A client that isn't consent-filtered still gets the exact total""" + for letter in "ABC": + self.create_test_individual(name=f"Legal Total {letter}", identifier_value=f"LT-{letter}") + + body = self._body("/api/v2/spp/Individual?name=Legal+Total&_count=1", self.legal_token) + + self.assertEqual(body["meta"]["total"], 3) + self.assertIsNotNone(body["links"].get("next")) + + def test_legal_basis_client_gets_the_real_group_total(self): + """GET /Group: a client that isn't consent-filtered still gets the exact total""" + for letter in "ABC": + self.create_test_group(name=f"Legal Group Total {letter}", identifier_value=f"LGT-{letter}") + + body = self._body("/api/v2/spp/Group?name=Legal+Group+Total&_count=1", self.legal_token) + + self.assertEqual(body["meta"]["total"], 3) + self.assertIsNotNone(body["links"].get("next")) + + +class TestConsentPagingAPI(ApiV2HttpTestCase): + """HTTP: next links of consent-filtered Individual and Group searches""" + + def setUp(self): + super().setUp() + self.client = self.create_api_client( + name="Consent Paging Client", + scopes=[ + {"resource": "individual", "action": "read"}, + {"resource": "group", "action": "read"}, + ], + ) + self.token = self.generate_jwt_token(self.client) + + def _consent(self, registrant, resource_type): + self.create_consent( + registrant=registrant, + grantee_partner=self.client.partner_id, + resource_type=resource_type, + field_access="all", + ) + + def _page(self, url): + response = self.url_open(url, headers={"Authorization": f"Bearer {self.token}"}) + self.assertEqual(response.status_code, 200, response.content) + body = json.loads(response.content) + return [record["identifier"][0]["value"] for record in body["data"]], body["links"].get("next") + + def test_individual_next_page_does_not_skip(self): + """B has no consent: page 1 is A, C and page 2 starts at D""" + for letter in "ABCDE": + person = self.create_test_individual(name=f"Paging Person {letter}", identifier_value=f"CP-{letter}") + if letter != "B": + self._consent(person, "individual") + + page1, next_url = self._page("/api/v2/spp/Individual?name=Paging+Person&_count=2") + page2, last_next = self._page(next_url) + + self.assertEqual(page1, ["CP-A", "CP-C"]) + self.assertEqual(page2, ["CP-D", "CP-E"]) + self.assertIsNone(last_next, "E is the last row: no link to an empty page") + + def test_group_next_page_does_not_skip(self): + """Group B has no consent: page 1 is A, C and page 2 starts at D""" + for letter in "ABCDE": + group = self.create_test_group(name=f"Paging Group {letter}", identifier_value=f"CPG-{letter}") + if letter != "B": + self._consent(group, "group") + + page1, next_url = self._page("/api/v2/spp/Group?name=Paging+Group&_count=2") + page2, last_next = self._page(next_url) + + self.assertEqual(page1, ["CPG-A", "CPG-C"]) + self.assertEqual(page2, ["CPG-D", "CPG-E"]) + self.assertIsNone(last_next, "E is the last row: no link to an empty page") + + def test_short_page_keeps_next_while_rows_remain(self): + """The safety limit cuts the page short: the next link is still given, and leads to E""" + for letter in "ABCDE": + person = self.create_test_individual(name=f"Sparse Person {letter}", identifier_value=f"SP-{letter}") + if letter == "E": + self._consent(person, "individual") + + page1, next_url = self._page("/api/v2/spp/Individual?name=Sparse+Person&_count=1") + self.assertEqual(page1, [], "A, B and C are hidden and the 3x safety limit stops the scan") + self.assertIsNotNone(next_url, "Rows remain, so the client must be told to continue") + + seen = [] + pages = 1 + for _page in range(5): + values, next_url = self._page(next_url) + pages += 1 + seen.extend(values) + if not next_url: + break + self.assertEqual(seen, ["SP-E"]) + self.assertIsNone(next_url) + self.assertEqual(pages, 2, "Page 2 scans D and E and ends the scan: no third, empty page") diff --git a/spp_api_v2/tests/test_consent_predicate.py b/spp_api_v2/tests/test_consent_predicate.py new file mode 100644 index 000000000..4c86d3bdd --- /dev/null +++ b/spp_api_v2/tests/test_consent_predicate.py @@ -0,0 +1,141 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""One rule decides whether a client is subject to consent: its legal basis. + +A client has two consent settings, edited independently on its form: +``legal_basis`` and ``is_require_consent``. Consent filtering +(``ConsentService.filter_response``) keys off the legal basis. The +"not found" answer must follow the same rule, or the status code tells +an unknown registrant apart from one the client may not read. +""" + +import json + +from ..services.auth_service import AuthenticatedClient +from ..services.consent_service import NON_CONSENT_BASES, ConsentService +from .common import ApiV2HttpTestCase, ApiV2TestCase + +NATIONAL_ID = "urn:openspp:vocab:id-type%23test_national_id" +HOUSEHOLD_ID = "urn:openspp:vocab:id-type%23test_household_id" + +SCOPES = [ + {"resource": "individual", "action": "read"}, + {"resource": "individual", "action": "update"}, + {"resource": "group", "action": "read"}, + {"resource": "group", "action": "update"}, +] + + +class TestConsentPredicate(ApiV2HttpTestCase): + def setUp(self): + super().setUp() + self.person = self.create_test_individual(name="Predicate Person", identifier_value="PRED-IND") + self.household = self.create_test_group(name="Predicate Household", identifier_value="PRED-HH") + + def _token(self, **kwargs): + client = self.create_api_client(name="Predicate Client", scopes=SCOPES, **kwargs) + return self.generate_jwt_token(client) + + def _consent_basis_token(self): + """Consent is its legal basis, but the "require consent" box is unticked""" + return self._token(require_consent=False, legal_basis="consent") + + def _public_task_token(self): + """A legal basis that needs no consent, with the "require consent" box ticked""" + return self._token(require_consent=True, legal_basis="public_task") + + def _get(self, path, token): + return self.url_open(path, headers={"Authorization": f"Bearer {token}"}) + + def _post(self, path, payload, token): + return self.url_open( + path, + data=json.dumps(payload), + headers={"Content-Type": "application/json", "Authorization": f"Bearer {token}"}, + ) + + def _assert_same_answer(self, unknown, existing, status): + self.assertEqual(unknown.status_code, status, unknown.text) + self.assertEqual(existing.status_code, status, existing.text) + + # --- consent basis, box unticked: unknown and unconsented look the same + + def test_individual_read(self): + token = self._consent_basis_token() + self._assert_same_answer( + self._get(f"/api/v2/spp/Individual/{NATIONAL_ID}|PRED-NONE", token), + self._get(f"/api/v2/spp/Individual/{NATIONAL_ID}|PRED-IND", token), + 403, + ) + + def test_group_read(self): + token = self._consent_basis_token() + self._assert_same_answer( + self._get(f"/api/v2/spp/Group/{HOUSEHOLD_ID}|PRED-NONE", token), + self._get(f"/api/v2/spp/Group/{HOUSEHOLD_ID}|PRED-HH", token), + 403, + ) + + def test_individual_groups(self): + token = self._consent_basis_token() + self._assert_same_answer( + self._get(f"/api/v2/spp/Individual/{NATIONAL_ID}|PRED-NONE/groups", token), + self._get(f"/api/v2/spp/Individual/{NATIONAL_ID}|PRED-IND/groups", token), + 403, + ) + + def test_group_membership_history(self): + token = self._consent_basis_token() + self._assert_same_answer( + self._get(f"/api/v2/spp/Group/{HOUSEHOLD_ID}|PRED-NONE/membership-history", token), + self._get(f"/api/v2/spp/Group/{HOUSEHOLD_ID}|PRED-HH/membership-history", token), + 403, + ) + + def test_bulk_export(self): + token = self._consent_basis_token() + response = self._post( + "/api/v2/spp/$bulk/export", + { + "type": "Individual", + "identifiers": [ + "urn:openspp:vocab:id-type#test_national_id|PRED-NONE", + "urn:openspp:vocab:id-type#test_national_id|PRED-IND", + ], + }, + token, + ) + + self.assertEqual(response.status_code, 200, response.text) + statuses = [item["status"] for item in response.json()["items"]] + self.assertEqual(statuses, ["access_denied", "access_denied"]) + + # --- legal basis, box ticked: reads without consent, so unknown is 404 + + def test_legal_basis_client_reads_without_consent(self): + token = self._public_task_token() + + self.assertEqual(self._get(f"/api/v2/spp/Individual/{NATIONAL_ID}|PRED-IND", token).status_code, 200) + self.assertEqual(self._get(f"/api/v2/spp/Individual/{NATIONAL_ID}|PRED-NONE", token).status_code, 404) + + def test_legal_basis_client_reads_group_history_without_consent(self): + token = self._public_task_token() + + existing = self._get(f"/api/v2/spp/Group/{HOUSEHOLD_ID}|PRED-HH/membership-history", token) + unknown = self._get(f"/api/v2/spp/Group/{HOUSEHOLD_ID}|PRED-NONE/membership-history", token) + + self.assertEqual(existing.status_code, 200, existing.text) + self.assertEqual(unknown.status_code, 404, unknown.text) + + +class TestOneListOfLegalBases(ApiV2TestCase): + """The consent checks read one list of the legal bases that need no consent""" + + def test_bypass_and_consent_filtering_agree_for_every_legal_basis(self): + client = self.create_api_client(name="Basis Client") + for basis, _label in client._fields["legal_basis"].selection: + with self.subTest(legal_basis=basis): + client.legal_basis = basis + authenticated = AuthenticatedClient(record=client, auth_type="oauth2") + + self.assertEqual(authenticated.has_legal_basis_bypass(), basis in NON_CONSENT_BASES) + self.assertEqual(ConsentService.is_consent_filtered(client), basis not in NON_CONSENT_BASES) diff --git a/spp_api_v2/tests/test_group_api.py b/spp_api_v2/tests/test_group_api.py index 2d05ef62a..4d38830d4 100644 --- a/spp_api_v2/tests/test_group_api.py +++ b/spp_api_v2/tests/test_group_api.py @@ -4,6 +4,7 @@ import json from odoo import fields +from odoo.tools import mute_logger from .common import ApiV2HttpTestCase @@ -227,6 +228,34 @@ def test_search_pagination(self): self.assertLessEqual(len(data.get("data", [])), 1) self.assertIn("links", data) + def _search_values(self, query): + """GET /Group?{query} and return the identifier values of the page""" + response = self.url_open(f"{self.api_base_url}?{query}", headers=self._get_headers()) + self.assertEqual(response.status_code, 200) + return [group["identifier"][0]["value"] for group in json.loads(response.content)["data"]] + + def test_search_offset_pages_through_results(self): + """_offset moves to the next page instead of repeating the first one""" + second_group = self.create_test_group(name="Smith Household Two", identifier_value="HH-002") + self.create_consent( + registrant=second_group, + grantee_partner=self.client.partner_id, + resource_type="group", + field_access="all", + ) + + self.assertEqual(self._search_values("name=Smith+Household&_count=1&_offset=0"), ["HH-001"]) + self.assertEqual(self._search_values("name=Smith+Household&_count=1&_offset=1"), ["HH-002"]) + self.assertEqual(self._search_values("name=Smith+Household&_count=1&_offset=2"), []) + + def test_search_page_filled_past_consent_denied_groups(self): + """Groups without consent are skipped and the page is filled from the records after them""" + # Sorted by name these come before "Smith Household", and the client has no consent for them + self.create_test_group(name="Smith Aaa", identifier_value="HH-NC-1") + self.create_test_group(name="Smith Bbb", identifier_value="HH-NC-2") + + self.assertEqual(self._search_values("name=Smith&_count=1"), ["HH-001"]) + def test_create_group_success(self): """POST /Group creates new group""" payload = { @@ -662,6 +691,47 @@ def test_update_member_role_success(self): self.assertIn("role", data) self.assertEqual(data["role"]["coding"][0]["code"], "head") + def test_update_member_unknown_role_returns_422(self): + """PATCH /Group/{id}/member/{individual_id} with an unknown role code returns 422 and changes nothing""" + url = ( + f"{self.api_base_url}/urn:openspp:vocab:id-type%23test_household_id|HH-001/member/" + "urn:openspp:vocab:id-type%23test_national_id|IND-001" + ) + payload = { + "role": {"coding": [{"system": "urn:openspp:vocab:group-membership-type", "code": "not_a_role"}]}, + } + + with mute_logger("odoo.addons.spp_api_v2.routers.group", "odoo.http"): + response = self.url_patch(url, data=json.dumps(payload), headers=self._get_headers()) + + self.assertEqual(response.status_code, 422) + self.assertIn("Unknown membership role", json.loads(response.content)["detail"]) + self.env.invalidate_all() + membership = self.group.group_membership_ids.filtered(lambda m: m.individual == self.individual1) + self.assertEqual(membership.membership_type_ids, self.relationship_head) + + def test_add_member_unknown_role_returns_422(self): + """POST /Group/{id}/$add-member with an unknown role code returns 422 and adds no member""" + self.create_test_individual(name="Role Typo", identifier_value="IND-ROLE-TYPO") + url = f"{self.api_base_url}/urn:openspp:vocab:id-type%23test_household_id|HH-001/$add-member" + payload = { + "entity": {"reference": "Individual/urn:openspp:vocab:id-type#test_national_id|IND-ROLE-TYPO"}, + "role": {"coding": [{"system": "urn:openspp:vocab:group-membership-type", "code": "not_a_role"}]}, + } + + with mute_logger("odoo.addons.spp_api_v2.routers.group", "odoo.http"): + response = self.url_open(url, data=json.dumps(payload), headers=self._get_headers()) + + self.assertEqual(response.status_code, 422) + detail = json.loads(response.content)["detail"] + self.assertIn("Unknown membership role", detail) + self.assertIn("not_a_role", detail) + self.env.invalidate_all() + self.assertFalse( + self.group.group_membership_ids.filtered(lambda m: m.individual.name == "Role Typo"), + "No membership may be created for an unknown role", + ) + def test_update_member_dates(self): """PATCH /Group/{id}/member/{individual_id} updates membership dates""" url = ( diff --git a/spp_api_v2/tests/test_group_member_errors.py b/spp_api_v2/tests/test_group_member_errors.py new file mode 100644 index 000000000..d573882c1 --- /dev/null +++ b/spp_api_v2/tests/test_group_member_errors.py @@ -0,0 +1,78 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Member operations map the service's errors to a status by type, not by text. + +The service raises its messages through ``_()``, so their text follows the +user's language; a status decided by matching English text changes with it. +""" + +import json +from unittest.mock import patch + +from ..services.group_service import AlreadyMemberError, GroupService, NotMemberError +from .common import ApiV2HttpTestCase + +GROUP_PATH = "/api/v2/spp/Group/urn:openspp:vocab:id-type%23test_household_id|HH-ERR" +MEMBER_REF = "Individual/urn:openspp:vocab:id-type#test_national_id|IND-ERR-MEMBER" +OUTSIDER_ID = "urn:openspp:vocab:id-type%23test_national_id|IND-ERR-OUTSIDER" + + +class TestGroupMemberErrors(ApiV2HttpTestCase): + def setUp(self): + super().setUp() + self.member = self.create_test_individual(name="Member", identifier_value="IND-ERR-MEMBER") + self.outsider = self.create_test_individual(name="Outsider", identifier_value="IND-ERR-OUTSIDER") + self.group = self.create_test_group( + name="Error Household", identifier_value="HH-ERR", members=[(self.member, None)] + ) + client = self.create_api_client( + name="Member Errors Client", + scopes=[{"resource": "group", "action": "read"}, {"resource": "group", "action": "update"}], + require_consent=False, + legal_basis="public_task", + ) + self.headers = { + "Content-Type": "application/json", + "Authorization": f"Bearer {self.generate_jwt_token(client)}", + } + + def _post(self, path, payload): + return self.url_open(path, data=json.dumps(payload), headers=self.headers) + + def _patch(self, path, payload): + return self.url_patch(path, data=json.dumps(payload), headers=self.headers) + + # --- service raises the dedicated errors ------------------------------ + + def test_service_raises_already_member(self): + with self.assertRaises(AlreadyMemberError): + GroupService(self.env).add_member(self.group, self.member) + + def test_service_raises_not_member_on_update_and_remove(self): + service = GroupService(self.env) + with self.assertRaises(NotMemberError): + service.update_member(self.group, self.outsider) + with self.assertRaises(NotMemberError): + service.remove_member(self.group, self.outsider) + + # --- the status does not depend on the message's language ------------- + + def test_add_existing_member_is_409_whatever_the_language(self): + with patch.object(GroupService, "add_member", side_effect=AlreadyMemberError("Déjà membre de ce groupe")): + response = self._post(f"{GROUP_PATH}/$add-member", {"entity": {"reference": MEMBER_REF}}) + + self.assertEqual(response.status_code, 409, response.text) + + def test_update_non_member_is_404_whatever_the_language(self): + with patch.object(GroupService, "update_member", side_effect=NotMemberError("Pas membre de ce groupe")): + response = self._patch(f"{GROUP_PATH}/member/{OUTSIDER_ID}", {"startDate": "2024-01-15"}) + + self.assertEqual(response.status_code, 404, response.text) + + def test_remove_non_member_is_404_whatever_the_language(self): + with patch.object(GroupService, "remove_member", side_effect=NotMemberError("Pas membre de ce groupe")): + response = self._post( + f"{GROUP_PATH}/$remove-member", + {"entity": {"reference": "Individual/urn:openspp:vocab:id-type#test_national_id|IND-ERR-OUTSIDER"}}, + ) + + self.assertEqual(response.status_code, 404, response.text) diff --git a/spp_api_v2/tests/test_identifier_ambiguity.py b/spp_api_v2/tests/test_identifier_ambiguity.py new file mode 100644 index 000000000..d44c4bdfe --- /dev/null +++ b/spp_api_v2/tests/test_identifier_ambiguity.py @@ -0,0 +1,293 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Identifiers the API cannot resolve to one registrant (#554 item I). + +The registry allows two registrants to share an ID type and value (the +ID-document deduplication manager exists to find them), but every API +lookup addresses one record. So the API: + +- refuses to create an identifier already live on another registrant (409); +- refuses to guess when a lookup matches several registrants: 409, or, + for a client that needs consent and lacks it for any of them, the same + 403 as "not found"; +- ignores soft-removed (``invalid``) IDs when resolving. +""" + +import json + +from ..schemas.base import HumanName, Identifier +from ..schemas.individual import Individual +from ..services import registrant_resolver +from ..services.group_service import GroupService +from ..services.individual_service import IndividualService +from .common import ApiV2HttpTestCase, ApiV2TestCase + +NATIONAL_ID = "urn:openspp:vocab:id-type#test_national_id" +HOUSEHOLD_ID = "urn:openspp:vocab:id-type#test_household_id" + + +class TestRegistrantResolver(ApiV2TestCase): + """Shared resolution of an identifier to one registrant""" + + def setUp(self): + super().setUp() + self.individual_service = IndividualService(self.env) + self.group_service = GroupService(self.env) + + def _invalidate_id(self, partner, value): + partner.reg_ids.filtered(lambda r: r.value == value).write({"status": "invalid"}) + + def test_single_match(self): + partner = self.create_test_individual(identifier_value="RES-ONE") + + found = registrant_resolver.resolve_registrant(self.env, NATIONAL_ID, "RES-ONE") + + self.assertEqual(found, partner) + + def test_shared_live_identifier_is_ambiguous(self): + first = self.create_test_individual(name="First", identifier_value="RES-DUP") + second = self.create_test_individual(name="Second", identifier_value="RES-DUP") + + with self.assertRaises(registrant_resolver.AmbiguousIdentifierError) as ctx: + registrant_resolver.resolve_registrant(self.env, NATIONAL_ID, "RES-DUP") + + self.assertEqual(ctx.exception.partners, first | second) + + def test_invalid_id_does_not_resolve(self): + partner = self.create_test_individual(identifier_value="RES-INVALID") + self._invalidate_id(partner, "RES-INVALID") + + self.assertFalse(registrant_resolver.resolve_registrant(self.env, NATIONAL_ID, "RES-INVALID")) + self.assertFalse(self.individual_service.find_by_identifier(NATIONAL_ID, "RES-INVALID")) + + def test_invalid_id_does_not_make_a_live_one_ambiguous(self): + removed = self.create_test_individual(name="Removed", identifier_value="RES-MIXED") + live = self.create_test_individual(name="Live", identifier_value="RES-MIXED") + self._invalidate_id(removed, "RES-MIXED") + + self.assertEqual(self.individual_service.find_by_identifier(NATIONAL_ID, "RES-MIXED"), live) + + def test_individual_lookup_raises_on_ambiguity(self): + self.create_test_individual(name="First", identifier_value="RES-IND-DUP") + self.create_test_individual(name="Second", identifier_value="RES-IND-DUP") + + with self.assertRaises(registrant_resolver.AmbiguousIdentifierError): + self.individual_service.find_by_identifier(NATIONAL_ID, "RES-IND-DUP") + + def test_group_lookup_raises_on_ambiguity(self): + self.create_test_group(name="First", identifier_value="RES-HH-DUP") + self.create_test_group(name="Second", identifier_value="RES-HH-DUP") + + with self.assertRaises(registrant_resolver.AmbiguousIdentifierError): + self.group_service.find_by_identifier(HOUSEHOLD_ID, "RES-HH-DUP") + + def _individual_schema(self, value, given="Copy"): + return Individual( + identifier=[Identifier(system=NATIONAL_ID, value=value)], + name=HumanName(given=given, family="Test"), + ) + + def test_create_individual_with_identifier_in_use_is_refused(self): + self.create_test_individual(identifier_value="RES-TAKEN") + + with self.assertRaises(registrant_resolver.IdentifierInUseError): + self.individual_service.create(self._individual_schema("RES-TAKEN"), source="test", api_authorized=True) + + self.assertEqual(self.env["spp.registry.id"].search_count([("value", "=", "RES-TAKEN")]), 1) + + def test_identifier_in_use_message_names_the_type_not_the_value(self): + """The message is logged by bundles, and an ID value is PII""" + self.create_test_individual(identifier_value="RES-PII-VALUE") + + with self.assertRaises(registrant_resolver.IdentifierInUseError) as ctx: + self.individual_service.create(self._individual_schema("RES-PII-VALUE"), source="test", api_authorized=True) + + self.assertNotIn("RES-PII-VALUE", str(ctx.exception)) + self.assertIn(NATIONAL_ID, str(ctx.exception)) + + def test_create_individual_may_reuse_an_invalid_identifier(self): + removed = self.create_test_individual(identifier_value="RES-FREED") + self._invalidate_id(removed, "RES-FREED") + + partner = self.individual_service.create( + self._individual_schema("RES-FREED"), source="test", api_authorized=True + ) + + self.assertEqual(self.individual_service.find_by_identifier(NATIONAL_ID, "RES-FREED"), partner) + + +class TestIdentifierAmbiguityAPI(ApiV2HttpTestCase): + """HTTP contract, for a client with a legal basis (no consent needed)""" + + SCOPES = [ + {"resource": "individual", "action": "read"}, + {"resource": "individual", "action": "create"}, + {"resource": "individual", "action": "update"}, + {"resource": "group", "action": "read"}, + {"resource": "group", "action": "create"}, + {"resource": "group", "action": "update"}, + ] + + def setUp(self): + super().setUp() + self.first = self.create_test_individual(name="Original", identifier_value="AMB-DUP") + self.second = self.create_test_individual(name="Copy", identifier_value="AMB-DUP") + self.group = self.create_test_group(name="Amb Household", identifier_value="AMB-HH") + self.client = self.create_api_client( + name="Legal Basis Client", + scopes=self.SCOPES, + require_consent=False, + legal_basis="public_task", + ) + self.token = self.generate_jwt_token(self.client) + + def _headers(self, token=None): + return {"Content-Type": "application/json", "Authorization": f"Bearer {token or self.token}"} + + def _get(self, path, token=None): + return self.url_open(path, headers=self._headers(token)) + + def _post(self, path, payload, token=None): + return self.url_open(path, data=json.dumps(payload), headers=self._headers(token)) + + def test_post_individual_with_identifier_in_use_is_409(self): + response = self._post( + "/api/v2/spp/Individual", + { + "type": "Individual", + "identifier": [{"system": NATIONAL_ID, "value": "AMB-TAKEN"}], + "name": {"given": "Original"}, + }, + ) + self.assertEqual(response.status_code, 201, response.text) + + response = self._post( + "/api/v2/spp/Individual", + { + "type": "Individual", + "identifier": [{"system": NATIONAL_ID, "value": "AMB-TAKEN"}], + "name": {"given": "Copy"}, + }, + ) + + self.assertEqual(response.status_code, 409, response.text) + self.env.invalidate_all() + self.assertEqual(self.env["spp.registry.id"].search_count([("value", "=", "AMB-TAKEN")]), 1) + + def test_post_group_with_identifier_in_use_is_409(self): + response = self._post( + "/api/v2/spp/Group", + { + "type": "Group", + "identifier": [{"system": HOUSEHOLD_ID, "value": "AMB-HH"}], + "name": "Copy Household", + }, + ) + + self.assertEqual(response.status_code, 409, response.text) + self.env.invalidate_all() + self.assertEqual(self.env["spp.registry.id"].search_count([("value", "=", "AMB-HH")]), 1) + + def test_get_ambiguous_individual_is_409(self): + response = self._get("/api/v2/spp/Individual/urn:openspp:vocab:id-type%23test_national_id|AMB-DUP") + + self.assertEqual(response.status_code, 409, response.text) + + def test_patch_ambiguous_individual_is_409_and_changes_nothing(self): + """The reported symptom: PATCH deactivated one of the two, arbitrarily""" + self.env.cr.flush() + response = self.url_patch( + "/api/v2/spp/Individual/urn:openspp:vocab:id-type%23test_national_id|AMB-DUP", + data=json.dumps({"active": False}), + headers=self._headers(), + ) + + self.assertEqual(response.status_code, 409, response.text) + self.env.invalidate_all() + self.assertTrue(self.first.active) + self.assertTrue(self.second.active) + + def test_search_by_ambiguous_identifier_lists_both(self): + """A search is not a lookup: it lists every match""" + response = self._get("/api/v2/spp/Individual?identifier=urn:openspp:vocab:id-type%23test_national_id%7CAMB-DUP") + + self.assertEqual(response.status_code, 200, response.text) + self.assertEqual(response.json()["meta"]["total"], 2) + + def test_invalid_identifier_is_not_found(self): + partner = self.create_test_individual(identifier_value="AMB-REMOVED") + partner.reg_ids.write({"status": "invalid"}) + + response = self._get("/api/v2/spp/Individual/urn:openspp:vocab:id-type%23test_national_id|AMB-REMOVED") + + self.assertEqual(response.status_code, 404, response.text) + + def test_add_ambiguous_member_is_409(self): + response = self._post( + "/api/v2/spp/Group/urn:openspp:vocab:id-type%23test_household_id|AMB-HH/$add-member", + {"entity": {"reference": f"Individual/{NATIONAL_ID}|AMB-DUP"}}, + ) + + self.assertEqual(response.status_code, 409, response.text) + self.env.invalidate_all() + self.assertFalse(self.group.group_membership_ids) + + def test_get_ambiguous_group_is_409(self): + self.create_test_group(name="Amb Copy Household", identifier_value="AMB-HH") + + response = self._get("/api/v2/spp/Group/urn:openspp:vocab:id-type%23test_household_id|AMB-HH") + + self.assertEqual(response.status_code, 409, response.text) + + def test_bulk_export_reports_ambiguous_identifier(self): + response = self._post( + "/api/v2/spp/$bulk/export", + {"type": "Individual", "identifiers": [f"{NATIONAL_ID}|AMB-DUP"]}, + ) + + self.assertEqual(response.status_code, 200, response.text) + item = response.json()["items"][0] + self.assertEqual(item["status"], "error") + self.assertIn("more than one", item["error"]) + + def test_batch_read_of_ambiguous_identifier_is_409(self): + response = self._post( + "/api/v2/spp/$batch", + { + "resourceType": "Bundle", + "type": "batch", + "entry": [{"request": {"method": "GET", "url": f"Individual/{NATIONAL_ID}|AMB-DUP"}}], + }, + ) + + self.assertEqual(response.status_code, 200, response.text) + self.assertEqual(response.json()["entry"][0]["response"]["status"], "409 Conflict") + + # --- clients that need consent ---------------------------------------- + + def _consent_client_token(self, consented): + client = self.create_api_client(name="Consent Client", scopes=self.SCOPES) + for registrant in consented: + self.create_consent( + registrant=registrant, + grantee_partner=client.partner_id, + resource_type="all", + field_access="all", + ) + return self.generate_jwt_token(client) + + def test_consent_client_without_consent_for_every_match_gets_403(self): + """Ambiguity is not revealed unless the client may see every match""" + # Consent to the newest match only: a lookup that picks one (newest + # first) would find it and answer 200, so this fails without the fix + token = self._consent_client_token(consented=[self.second]) + + response = self._get("/api/v2/spp/Individual/urn:openspp:vocab:id-type%23test_national_id|AMB-DUP", token) + + self.assertEqual(response.status_code, 403, response.text) + + def test_consent_client_with_consent_for_every_match_gets_409(self): + token = self._consent_client_token(consented=[self.first, self.second]) + + response = self._get("/api/v2/spp/Individual/urn:openspp:vocab:id-type%23test_national_id|AMB-DUP", token) + + self.assertEqual(response.status_code, 409, response.text) diff --git a/spp_api_v2/tests/test_identifier_ambiguity_paths.py b/spp_api_v2/tests/test_identifier_ambiguity_paths.py new file mode 100644 index 000000000..d92668df5 --- /dev/null +++ b/spp_api_v2/tests/test_identifier_ambiguity_paths.py @@ -0,0 +1,442 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Every API path that creates or resolves identifiers handles ambiguity (#554 item I). + +Complements test_identifier_ambiguity.py with the paths found in review: +batch/transaction bundles, group create/split, search filters, bulk export, +soft-removed IDs in responses, and individual/group kind filtering. +""" + +import json +import logging + +from .common import ApiV2HttpTestCase + +NATIONAL_ID = "urn:openspp:vocab:id-type#test_national_id" +HOUSEHOLD_ID = "urn:openspp:vocab:id-type#test_household_id" +GROUP_PATH = "/api/v2/spp/Group/urn:openspp:vocab:id-type%23test_household_id|PATH-HH" + +SCOPES = [ + {"resource": "individual", "action": "read"}, + {"resource": "individual", "action": "create"}, + {"resource": "individual", "action": "update"}, + {"resource": "group", "action": "read"}, + {"resource": "group", "action": "create"}, + {"resource": "group", "action": "update"}, +] + + +class TestIdentifierAmbiguityPaths(ApiV2HttpTestCase): + def setUp(self): + super().setUp() + self.first = self.create_test_individual(name="Original", identifier_value="PATH-DUP") + self.second = self.create_test_individual(name="Copy", identifier_value="PATH-DUP") + self.head = self.create_test_individual(name="Path Head", identifier_value="PATH-HEAD") + self.mover = self.create_test_individual(name="Path Mover", identifier_value="PATH-MOVER") + self.group = self.create_test_group( + name="Path Household", + identifier_value="PATH-HH", + members=[(self.head, self.relationship_head), (self.mover, None)], + ) + self.dup_group_a = self.create_test_group(name="Dup Household A", identifier_value="PATH-HH-DUP") + self.dup_group_b = self.create_test_group(name="Dup Household B", identifier_value="PATH-HH-DUP") + self.client = self.create_api_client( + name="Legal Basis Client", + scopes=SCOPES, + require_consent=False, + legal_basis="public_task", + ) + self.token = self.generate_jwt_token(self.client) + + def _headers(self, token=None): + return {"Content-Type": "application/json", "Authorization": f"Bearer {token or self.token}"} + + def _get(self, path, token=None): + return self.url_open(path, headers=self._headers(token)) + + def _post(self, path, payload, token=None): + return self.url_open(path, data=json.dumps(payload), headers=self._headers(token)) + + def _count_ids(self, value): + self.env.invalidate_all() + return self.env["spp.registry.id"].search_count([("value", "=", value)]) + + def _consent_client_token(self, consented=(), **kwargs): + client = self.create_api_client(name="Consent Client", scopes=SCOPES, **kwargs) + for registrant in consented: + self.create_consent( + registrant=registrant, + grantee_partner=client.partner_id, + resource_type="all", + field_access="all", + ) + return self.generate_jwt_token(client) + + # --- creates ----------------------------------------------------------- + + def test_http_post_group_with_ambiguous_member_is_409_and_creates_nothing(self): + response = self._post( + "/api/v2/spp/Group", + { + "type": "Group", + "identifier": [{"system": HOUSEHOLD_ID, "value": "PATH-NEW-HH"}], + "name": "New Household", + "member": [{"entity": {"reference": f"Individual/{NATIONAL_ID}|PATH-DUP"}}], + }, + ) + + self.assertEqual(response.status_code, 409, response.text) + self.assertEqual(self._count_ids("PATH-NEW-HH"), 0) + + def test_batch_post_group_with_ambiguous_member_creates_nothing(self): + """The batch entry must not leave the group behind while reporting 409""" + response = self._post( + "/api/v2/spp/$batch", + { + "resourceType": "Bundle", + "type": "batch", + "entry": [ + { + "request": {"method": "POST", "url": "Group"}, + "resource": { + "type": "Group", + "identifier": [{"system": HOUSEHOLD_ID, "value": "PATH-BATCH-HH"}], + "name": "Batch Household", + "member": [{"entity": {"reference": f"Individual/{NATIONAL_ID}|PATH-DUP"}}], + }, + } + ], + }, + ) + + self.assertEqual(response.status_code, 200, response.text) + self.assertEqual(response.json()["entry"][0]["response"]["status"], "409 Conflict") + self.assertEqual(self._count_ids("PATH-BATCH-HH"), 0) + + def test_batch_post_individual_with_identifier_in_use_is_409(self): + response = self._post( + "/api/v2/spp/$batch", + { + "resourceType": "Bundle", + "type": "batch", + "entry": [ + { + "request": {"method": "POST", "url": "Individual"}, + "resource": { + "type": "Individual", + "identifier": [{"system": NATIONAL_ID, "value": "PATH-HEAD"}], + "name": {"given": "Clash"}, + }, + } + ], + }, + ) + + self.assertEqual(response.status_code, 200, response.text) + self.assertEqual(response.json()["entry"][0]["response"]["status"], "409 Conflict") + self.assertEqual(self._count_ids("PATH-HEAD"), 1) + + def _logged_text(self, records): + formatter = logging.Formatter("%(message)s") + return "\n".join(formatter.format(record) for record in records) + + def test_bundles_do_not_log_the_identifier_value_in_use(self): + """A failed entry is logged; an ID value (often a national ID) must not be""" + entry = { + "request": {"method": "POST", "url": "Individual"}, + "resource": { + "type": "Individual", + "identifier": [{"system": NATIONAL_ID, "value": "PATH-HEAD"}], + "name": {"given": "Clash"}, + }, + } + for bundle_type in ("batch", "transaction"): + with self.subTest(bundle_type=bundle_type): + with self.assertLogs("odoo.addons.spp_api_v2", level="WARNING") as logs: + response = self._post( + "/api/v2/spp/$batch", + {"resourceType": "Bundle", "type": bundle_type, "entry": [entry]}, + ) + + self.assertIn(response.status_code, (200, 409), response.text) + self.assertNotIn("PATH-HEAD", self._logged_text(logs.records)) + + def test_bundles_do_not_log_a_malformed_resource(self): + """Schema validation errors quote the input (input_value=...), so they are logged by type too""" + entry = { + "request": {"method": "POST", "url": "Individual"}, + "resource": { + "type": "Individual", + "identifier": [{"system": NATIONAL_ID, "value": "PATH-MALFORMED"}], + "name": "PATH-PII-NAME", + }, + } + with self.assertLogs("odoo.addons.spp_api_v2", level="WARNING") as logs: + self._post("/api/v2/spp/$batch", {"resourceType": "Bundle", "type": "batch", "entry": [entry]}) + + self.assertNotIn("PATH-PII-NAME", self._logged_text(logs.records)) + + def test_bundles_do_not_log_the_identifier_value_not_found(self): + entry = {"request": {"method": "GET", "url": f"Individual/{NATIONAL_ID}|PATH-NOBODY-PII"}} + for bundle_type in ("batch", "transaction"): + with self.subTest(bundle_type=bundle_type): + with self.assertLogs("odoo.addons.spp_api_v2", level="WARNING") as logs: + response = self._post( + "/api/v2/spp/$batch", + {"resourceType": "Bundle", "type": bundle_type, "entry": [entry]}, + ) + + self.assertIn(response.status_code, (200, 422), response.text) + self.assertNotIn("PATH-NOBODY-PII", self._logged_text(logs.records)) + + def test_transaction_bundle_with_ambiguous_identifier_is_409(self): + response = self._post( + "/api/v2/spp/$batch", + { + "resourceType": "Bundle", + "type": "transaction", + "entry": [{"request": {"method": "GET", "url": f"Individual/{NATIONAL_ID}|PATH-DUP"}}], + }, + ) + + self.assertEqual(response.status_code, 409, response.text) + + def test_split_to_an_identifier_in_use_is_409_and_creates_nothing(self): + response = self._post( + f"{GROUP_PATH}/$split", + { + "newGroupIdentifier": [{"system": HOUSEHOLD_ID, "value": "PATH-HH-DUP"}], + "membersToMove": [{"reference": f"Individual/{NATIONAL_ID}|PATH-MOVER"}], + }, + ) + + self.assertEqual(response.status_code, 409, response.text) + self.assertEqual(self._count_ids("PATH-HH-DUP"), 2) + self.env.invalidate_all() + self.assertFalse(self.group.group_membership_ids.filtered(lambda m: m.individual == self.mover).is_ended) + + # --- search filters ---------------------------------------------------- + + def test_group_filter_with_ambiguous_group_is_409(self): + response = self._get("/api/v2/spp/Individual?group=urn:openspp:vocab:id-type%23test_household_id%7CPATH-HH-DUP") + + self.assertEqual(response.status_code, 409, response.text) + + def test_member_filter_with_ambiguous_member_is_409(self): + response = self._get( + "/api/v2/spp/Group?member=Individual/urn:openspp:vocab:id-type%23test_national_id%7CPATH-DUP" + ) + + self.assertEqual(response.status_code, 409, response.text) + + def test_ambiguous_filter_looks_like_not_found_to_a_client_without_consent(self): + """In a search, "not found" is an empty 200, so ambiguity must be too""" + token = self._consent_client_token(consented=[self.dup_group_a]) + + response = self._get( + "/api/v2/spp/Individual?group=urn:openspp:vocab:id-type%23test_household_id%7CPATH-HH-DUP", token + ) + + self.assertEqual(response.status_code, 200, response.text) + self.assertEqual(response.json()["meta"]["total"], 0) + + # --- consent decision -------------------------------------------------- + + def test_legal_basis_client_that_requires_consent_flag_gets_409(self): + """The 409/403 decision uses the read path's rule: a legal basis reads without consent""" + token = self._consent_client_token(require_consent=True, legal_basis="public_task") + + response = self._get("/api/v2/spp/Individual/urn:openspp:vocab:id-type%23test_national_id|PATH-DUP", token) + + self.assertEqual(response.status_code, 409, response.text) + + def test_add_ambiguous_member_as_client_without_consent_is_403(self): + token = self._consent_client_token(consented=[self.group, self.first]) + + response = self._post( + f"{GROUP_PATH}/$add-member", + {"entity": {"reference": f"Individual/{NATIONAL_ID}|PATH-DUP"}}, + token, + ) + + self.assertEqual(response.status_code, 403, response.text) + + def test_bulk_export_ambiguous_identifier_for_client_without_consent_is_access_denied(self): + token = self._consent_client_token(consented=[self.first]) + + response = self._post( + "/api/v2/spp/$bulk/export", + {"type": "Individual", "identifiers": [f"{NATIONAL_ID}|PATH-DUP"]}, + token, + ) + + self.assertEqual(response.status_code, 200, response.text) + self.assertEqual(response.json()["items"][0]["status"], "access_denied") + + # --- soft-removed IDs -------------------------------------------------- + + def test_removed_ids_are_not_listed_or_searchable(self): + self.env["spp.registry.id"].create( + { + "partner_id": self.head.id, + "id_type_id": self.id_type_household.id, + "value": "PATH-REMOVED", + "status": "invalid", + } + ) + + read = self._get("/api/v2/spp/Individual/urn:openspp:vocab:id-type%23test_national_id|PATH-HEAD") + self.assertEqual(read.status_code, 200, read.text) + self.assertEqual([ident["value"] for ident in read.json()["identifier"]], ["PATH-HEAD"]) + + search = self._get( + "/api/v2/spp/Individual?identifier=urn:openspp:vocab:id-type%23test_household_id%7CPATH-REMOVED" + ) + self.assertEqual(search.status_code, 200, search.text) + self.assertEqual(search.json()["meta"]["total"], 0) + + def test_removed_group_ids_are_not_listed(self): + self.env["spp.registry.id"].create( + { + "partner_id": self.group.id, + "id_type_id": self.id_type_national.id, + "value": "PATH-HH-REMOVED", + "status": "invalid", + } + ) + + read = self._get(GROUP_PATH) + + self.assertEqual(read.status_code, 200, read.text) + self.assertEqual([ident["value"] for ident in read.json()["identifier"]], ["PATH-HH"]) + + # --- individual vs group ------------------------------------------------ + + def test_individual_and_group_sharing_a_value_are_resolved_by_kind(self): + """A value held by one individual and one group is not ambiguous per kind""" + self.create_test_individual(name="Kind Person", identifier_value="PATH-KIND") + self.create_test_group(name="Kind Household", identifier_value="PATH-KIND", id_type=self.id_type_national) + + individual = self._get("/api/v2/spp/Individual/urn:openspp:vocab:id-type%23test_national_id|PATH-KIND") + group = self._get("/api/v2/spp/Group/urn:openspp:vocab:id-type%23test_national_id|PATH-KIND") + + self.assertEqual(individual.status_code, 200, individual.text) + self.assertEqual(individual.json()["type"], "Individual") + self.assertEqual(group.status_code, 200, group.text) + self.assertEqual(group.json()["type"], "Group") + + +class TestBundleAmbiguityForConsentClient(ApiV2HttpTestCase): + """A bundle only says "ambiguous" to a client that may read every match + + Anyone else gets what they would get for an identifier nobody holds, + as on the single-resource endpoints. + """ + + def setUp(self): + super().setUp() + self.first = self.create_test_individual(name="Original", identifier_value="BND-DUP") + self.second = self.create_test_individual(name="Copy", identifier_value="BND-DUP") + self.consent_client = self.create_api_client(name="Bundle Consent Client", scopes=SCOPES) + self.create_consent( + registrant=self.first, + grantee_partner=self.consent_client.partner_id, + resource_type="all", + field_access="all", + ) + self.token = self.generate_jwt_token(self.consent_client) + + def _bundle(self, bundle_type, entries): + return self.url_open( + "/api/v2/spp/$batch", + data=json.dumps({"resourceType": "Bundle", "type": bundle_type, "entry": entries}), + headers={"Content-Type": "application/json", "Authorization": f"Bearer {self.token}"}, + ) + + def _read(self, value): + return {"request": {"method": "GET", "url": f"Individual/{NATIONAL_ID}|{value}"}} + + def test_batch_read_of_an_ambiguous_identifier_looks_like_not_found(self): + response = self._bundle("batch", [self._read("BND-DUP"), self._read("BND-NOBODY")]) + + self.assertEqual(response.status_code, 200, response.text) + ambiguous, unknown = response.json()["entry"] + self.assertEqual(ambiguous["response"]["status"], unknown["response"]["status"]) + self.assertEqual( + ambiguous["resource"]["issue"][0]["diagnostics"].replace("BND-DUP", "X"), + unknown["resource"]["issue"][0]["diagnostics"].replace("BND-NOBODY", "X"), + ) + + def test_batch_update_and_delete_of_an_ambiguous_identifier_look_like_not_found(self): + update = { + "request": {"method": "PUT", "url": f"Individual/{NATIONAL_ID}|BND-DUP"}, + "resource": {"type": "Individual", "name": {"given": "Changed"}}, + } + delete = {"request": {"method": "DELETE", "url": f"Individual/{NATIONAL_ID}|BND-DUP"}} + + response = self._bundle("batch", [update, delete, self._read("BND-NOBODY")]) + + self.assertEqual(response.status_code, 200, response.text) + statuses = [entry["response"]["status"] for entry in response.json()["entry"]] + self.assertNotIn("409 Conflict", statuses) + self.assertEqual(statuses[0], statuses[2]) + self.assertEqual(statuses[1], statuses[2]) + + def test_batch_create_with_an_ambiguous_member_is_403(self): + """As POST /Group answers the same client: the jittered "not found" 403""" + response = self._bundle( + "batch", + [ + { + "request": {"method": "POST", "url": "Group"}, + "resource": { + "type": "Group", + "identifier": [{"system": HOUSEHOLD_ID, "value": "BND-NEW-HH"}], + "name": "Bundle Household", + "member": [{"entity": {"reference": f"Individual/{NATIONAL_ID}|BND-DUP"}}], + }, + } + ], + ) + + self.assertEqual(response.status_code, 200, response.text) + entry = response.json()["entry"][0] + self.assertEqual(entry["response"]["status"], "403 Forbidden") + self.assertNotIn("more than one", json.dumps(entry)) + + def test_transaction_create_with_an_ambiguous_member_is_403(self): + response = self._bundle( + "transaction", + [ + { + "request": {"method": "POST", "url": "Group"}, + "resource": { + "type": "Group", + "identifier": [{"system": HOUSEHOLD_ID, "value": "BND-TX-HH"}], + "name": "Bundle Household", + "member": [{"entity": {"reference": f"Individual/{NATIONAL_ID}|BND-DUP"}}], + }, + } + ], + ) + + self.assertEqual(response.status_code, 403, response.text) + self.assertNotIn("more than one", response.text) + + def test_transaction_read_of_an_ambiguous_identifier_looks_like_not_found(self): + ambiguous = self._bundle("transaction", [self._read("BND-DUP")]) + unknown = self._bundle("transaction", [self._read("BND-NOBODY")]) + + self.assertEqual(ambiguous.status_code, unknown.status_code, ambiguous.text) + self.assertNotEqual(ambiguous.status_code, 409) + + def test_client_that_may_read_every_match_still_gets_409(self): + self.create_consent( + registrant=self.second, + grantee_partner=self.consent_client.partner_id, + resource_type="all", + field_access="all", + ) + + response = self._bundle("batch", [self._read("BND-DUP")]) + + self.assertEqual(response.json()["entry"][0]["response"]["status"], "409 Conflict") diff --git a/spp_api_v2/tests/test_individual_api.py b/spp_api_v2/tests/test_individual_api.py index 8a513b064..617b55a0a 100644 --- a/spp_api_v2/tests/test_individual_api.py +++ b/spp_api_v2/tests/test_individual_api.py @@ -4,6 +4,8 @@ import json from datetime import date +from odoo.tools import mute_logger + from .common import ApiV2HttpTestCase @@ -259,6 +261,21 @@ def test_create_individual_success(self): self.assertIn("location", response.headers) self.assertIn("CREATE-001", response.headers["location"]) + def test_create_individual_gender_from_another_vocabulary_returns_422(self): + """POST /Individual with a gender code from another vocabulary (eg a membership role) returns 422""" + payload = { + "type": "Individual", + "identifier": [{"system": "urn:openspp:vocab:id-type#test_national_id", "value": "CREATE-GENDER-VOCAB"}], + "name": {"given": "Wrong", "family": "Vocabulary"}, + "gender": {"coding": [{"system": "urn:openspp:vocab:group-membership-type", "code": "head"}]}, + } + + with mute_logger("odoo.addons.spp_api_v2.routers.individual", "odoo.http"): + response = self.url_open(self.api_base_url, data=json.dumps(payload), headers=self._get_headers()) + + self.assertEqual(response.status_code, 422) + self.assertIn("Invalid gender code", json.loads(response.content)["detail"]) + def test_create_individual_source_tracking(self): """Created individual has source_system set""" payload = { diff --git a/spp_api_v2/tests/test_membership_end_now.py b/spp_api_v2/tests/test_membership_end_now.py new file mode 100644 index 000000000..5ab704dd0 --- /dev/null +++ b/spp_api_v2/tests/test_membership_end_now.py @@ -0,0 +1,150 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Ending a membership "now" takes effect immediately (#554 item G). + +The stored ``is_ended``/``status`` computes compare ``ended_date`` with +``fields.Datetime.now()``, which has whole-second precision. An end date +written with microseconds (``datetime.now()``) is a fraction of a second +in the future at compute time, so the row was stored as still active and +only corrected later by the repair cron. +""" + +import json + +from ..services.group_service import GroupService +from .common import ApiV2HttpTestCase, ApiV2TestCase + +HOUSEHOLD_ID = "urn:openspp:vocab:id-type#test_household_id" + + +def _stored_end_state(env, membership): + """ended_date, is_ended, status as stored in the database (not the cache)""" + env.flush_all() + env.cr.execute( + "SELECT ended_date, is_ended, status FROM spp_group_membership WHERE id = %s", + (membership.id,), + ) + return env.cr.fetchone() + + +class TestMembershipEndNow(ApiV2TestCase): + """GroupService operations that end memberships at the current time""" + + def setUp(self): + super().setUp() + self.service = GroupService(self.env) + self.head = self.create_test_individual(name="End Head", identifier_value="END-IND-HEAD") + self.member_a = self.create_test_individual(name="End A", identifier_value="END-IND-A") + self.member_b = self.create_test_individual(name="End B", identifier_value="END-IND-B") + + def _membership(self, group, individual): + return self.env["spp.group.membership"].search( + [("group", "=", group.id), ("individual", "=", individual.id)], limit=1 + ) + + def _assert_ended_now(self, membership): + ended_date, is_ended, status = _stored_end_state(self.env, membership) + self.assertTrue(ended_date) + self.assertEqual(ended_date.microsecond, 0, "ended_date must use the ORM's second precision") + self.assertTrue(is_ended, "stored is_ended must be true without waiting for the repair cron") + self.assertEqual(status, "inactive") + + def test_remove_member_ends_membership_immediately(self): + group = self.create_test_group( + name="End Remove", + identifier_value="END-HH-REMOVE", + members=[(self.head, self.relationship_head), (self.member_a, None)], + ) + + response = self.service.remove_member(group, self.member_a) + + self.assertEqual(response["status"], "inactive") + self._assert_ended_now(self._membership(group, self.member_a)) + + def test_merge_ends_source_memberships_immediately(self): + source = self.create_test_group( + name="End Merge Source", + identifier_value="END-HH-MERGE-SRC", + members=[(self.member_a, None), (self.member_b, None)], + ) + target = self.create_test_group( + name="End Merge Target", + identifier_value="END-HH-MERGE-TGT", + members=[(self.head, self.relationship_head)], + ) + source_memberships = source.group_membership_ids + + self.service.merge_groups(source, target) + + for membership in source_memberships: + self._assert_ended_now(membership) + + def test_split_ends_moved_memberships_immediately(self): + source = self.create_test_group( + name="End Split Source", + identifier_value="END-HH-SPLIT-SRC", + members=[(self.head, self.relationship_head), (self.member_a, None), (self.member_b, None)], + ) + moved = self._membership(source, self.member_a) + + self.service.split_group( + source, + new_identifiers=[{"system": HOUSEHOLD_ID, "value": "END-HH-SPLIT-NEW"}], + members_to_move=[self.member_a], + ) + + self._assert_ended_now(moved) + self.assertFalse(_stored_end_state(self.env, self._membership(source, self.member_b))[1]) + + +class TestRemoveMemberEndNowAPI(ApiV2HttpTestCase): + """$remove-member without endedDate: the response and searches agree at once""" + + def setUp(self): + super().setUp() + self.head = self.create_test_individual(name="API End Head", identifier_value="END-API-HEAD") + self.leaver = self.create_test_individual(name="API End Leaver", identifier_value="END-API-LEAVER") + self.group = self.create_test_group( + name="API End Household", + identifier_value="END-API-HH", + members=[(self.head, self.relationship_head), (self.leaver, None)], + ) + self.client = self.create_api_client( + name="End Now Client", + scopes=[ + {"resource": "individual", "action": "read"}, + {"resource": "group", "action": "read"}, + {"resource": "group", "action": "update"}, + ], + ) + for registrant in (self.head, self.leaver, self.group): + self.create_consent( + registrant=registrant, + grantee_partner=self.client.partner_id, + resource_type="all", + field_access="all", + ) + self.token = self.generate_jwt_token(self.client) + + def _headers(self): + return {"Content-Type": "application/json", "Authorization": f"Bearer {self.token}"} + + def test_remove_member_is_effective_immediately(self): + response = self.url_open( + "/api/v2/spp/Group/urn:openspp:vocab:id-type%23test_household_id|END-API-HH/$remove-member", + data=json.dumps( + {"entity": {"reference": "Individual/urn:openspp:vocab:id-type#test_national_id|END-API-LEAVER"}} + ), + headers=self._headers(), + ) + self.assertEqual(response.status_code, 200, response.text) + self.assertEqual(response.json()["status"], "inactive") + self.assertIn("endedDate", response.json()) + + search = self.url_open( + "/api/v2/spp/Individual?group=urn:openspp:vocab:id-type%23test_household_id%7CEND-API-HH", + headers=self._headers(), + ) + self.assertEqual(search.status_code, 200, search.text) + values = {resource["identifier"][0]["value"] for resource in search.json().get("data") or []} + self.assertIn("END-API-HEAD", values) + self.assertNotIn("END-API-LEAVER", values) diff --git a/spp_api_v2/tests/test_patch_api.py b/spp_api_v2/tests/test_patch_api.py index 7327cb0bc..bb92aed5a 100644 --- a/spp_api_v2/tests/test_patch_api.py +++ b/spp_api_v2/tests/test_patch_api.py @@ -4,6 +4,8 @@ import json from datetime import date +from odoo.tools import mute_logger + from .common import ApiV2HttpTestCase @@ -299,6 +301,56 @@ def test_patch_individual_updates_version(self): # Version should have changed self.assertNotEqual(old_version_id, new_version_id) + def test_patch_individual_gender(self): + """PATCH /Individual/{id} changes gender from a vocabulary coding""" + url = "/api/v2/spp/Individual/urn:openspp:vocab:id-type%23test_national_id|IND-PATCH-001" + payload = {"gender": {"coding": [{"system": "urn:iso:std:iso:5218", "code": "1"}]}} + + response = self.url_patch(url, data=json.dumps(payload), headers=self._get_headers()) + + self.assertEqual(response.status_code, 200, response.content) + data = json.loads(response.content) + self.assertEqual(data["gender"]["coding"][0]["code"], "1") + self.individual.invalidate_recordset() + self.assertEqual(self.individual.gender_id, self.gender_male) + + def test_patch_individual_unknown_gender_returns_422(self): + """PATCH /Individual/{id} with an unknown gender code returns 422 naming it, as create does""" + url = "/api/v2/spp/Individual/urn:openspp:vocab:id-type%23test_national_id|IND-PATCH-001" + payload = {"gender": {"coding": [{"system": "urn:iso:std:iso:5218", "code": "7"}]}} + + with mute_logger("odoo.addons.spp_api_v2.routers.individual", "odoo.http"): + response = self.url_patch(url, data=json.dumps(payload), headers=self._get_headers()) + + self.assertEqual(response.status_code, 422) + self.assertIn("Invalid gender code", json.loads(response.content)["detail"]) + self.individual.invalidate_recordset() + self.assertEqual(self.individual.gender_id, self.gender_female) + + def test_patch_individual_gender_from_another_vocabulary_returns_422(self): + """PATCH gender with a code from another vocabulary (eg a membership role) returns 422""" + url = "/api/v2/spp/Individual/urn:openspp:vocab:id-type%23test_national_id|IND-PATCH-001" + payload = {"gender": {"coding": [{"system": "urn:openspp:vocab:group-membership-type", "code": "head"}]}} + + with mute_logger("odoo.addons.spp_api_v2.routers.individual", "odoo.http"): + response = self.url_patch(url, data=json.dumps(payload), headers=self._get_headers()) + + self.assertEqual(response.status_code, 422) + self.assertIn("Invalid gender code", json.loads(response.content)["detail"]) + self.individual.invalidate_recordset() + self.assertEqual(self.individual.gender_id, self.gender_female) + + def test_patch_individual_gender_null_clears(self): + """PATCH /Individual/{id} with gender null clears it""" + url = "/api/v2/spp/Individual/urn:openspp:vocab:id-type%23test_national_id|IND-PATCH-001" + + response = self.url_patch(url, data=json.dumps({"gender": None}), headers=self._get_headers()) + + self.assertEqual(response.status_code, 200, response.content) + self.assertIsNone(json.loads(response.content)["gender"]) + self.individual.invalidate_recordset() + self.assertFalse(self.individual.gender_id) + # ============================================================================= # Group PATCH Tests # ============================================================================= diff --git a/spp_api_v2/tests/test_references_resolvable.py b/spp_api_v2/tests/test_references_resolvable.py new file mode 100644 index 000000000..76d2ba8fd --- /dev/null +++ b/spp_api_v2/tests/test_references_resolvable.py @@ -0,0 +1,153 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Every reference the API returns can be followed back (#554 item F). + +References must carry the identifier type's full code URI +(``id_type_id.uri``, e.g. ``urn:openspp:vocab:id-type#test_national_id``), +the value lookups match on — not the vocabulary namespace alone. +""" + +import json +from urllib.parse import quote + +from .common import ApiV2HttpTestCase + +GROUP_PATH = "/api/v2/spp/Group/urn:openspp:vocab:id-type%23test_household_id|REF-HH-001" + + +class TestReferencesResolvable(ApiV2HttpTestCase): + """Follow Individual/Group references from each response that returns them""" + + def setUp(self): + super().setUp() + self.member = self.create_test_individual(name="Ref Member", identifier_value="REF-IND-001") + self.newcomer = self.create_test_individual(name="Ref Newcomer", identifier_value="REF-IND-002") + self.group = self.create_test_group( + name="Ref Household", + identifier_value="REF-HH-001", + members=[(self.member, self.relationship_head)], + ) + self.client = self.create_api_client( + name="Reference Client", + scopes=[ + {"resource": "individual", "action": "read"}, + {"resource": "group", "action": "read"}, + {"resource": "group", "action": "update"}, + ], + ) + for registrant in (self.member, self.newcomer, self.group): + self.create_consent( + registrant=registrant, + grantee_partner=self.client.partner_id, + resource_type="all", + field_access="all", + ) + self.token = self.generate_jwt_token(self.client) + + def _headers(self): + return {"Content-Type": "application/json", "Authorization": f"Bearer {self.token}"} + + def _get(self, path): + return self.url_open(path, headers=self._headers()) + + def _post(self, path, payload): + return self.url_open(path, data=json.dumps(payload), headers=self._headers()) + + def _assert_followable(self, reference, expected_value): + """GET the referenced resource and check it is the one named""" + resource_type, identifier = reference.split("/", 1) + response = self._get(f"/api/v2/spp/{resource_type}/{quote(identifier, safe=':|')}") + self.assertEqual(response.status_code, 200, f"{reference} → {response.status_code} {response.text}") + values = {ident["value"] for ident in response.json()["identifier"]} + self.assertIn(expected_value, values) + + def test_group_member_entity_reference(self): + """GET /Group/{id}: member[].entity.reference""" + response = self._get(GROUP_PATH) + self.assertEqual(response.status_code, 200, response.text) + + members = response.json()["member"] + self.assertTrue(members) + for member in members: + self._assert_followable(member["entity"]["reference"], "REF-IND-001") + + def test_add_member_response_references(self): + """POST $add-member: group and entity references in the response""" + response = self._post( + f"{GROUP_PATH}/$add-member", + {"entity": {"reference": "Individual/urn:openspp:vocab:id-type#test_national_id|REF-IND-002"}}, + ) + self.assertIn(response.status_code, (200, 201), response.text) + data = response.json() + + self._assert_followable(data["group"]["reference"], "REF-HH-001") + self._assert_followable(data["entity"]["reference"], "REF-IND-002") + + def test_remove_member_response_references(self): + """POST $remove-member: group and entity references in the response""" + response = self._post( + f"{GROUP_PATH}/$remove-member", + {"entity": {"reference": "Individual/urn:openspp:vocab:id-type#test_national_id|REF-IND-001"}}, + ) + self.assertEqual(response.status_code, 200, response.text) + data = response.json() + + self._assert_followable(data["group"]["reference"], "REF-HH-001") + self._assert_followable(data["entity"]["reference"], "REF-IND-001") + + def test_membership_history_member_reference(self): + """GET membership-history: data[].member.reference""" + response = self._get(f"{GROUP_PATH}/membership-history") + self.assertEqual(response.status_code, 200, response.text) + + entries = response.json()["data"] + self.assertTrue(entries) + for entry in entries: + self._assert_followable(entry["member"]["reference"], "REF-IND-001") + + def test_individual_group_membership_reference(self): + """GET /Individual/{id}: groupMembership[].group.reference""" + response = self._get("/api/v2/spp/Individual/urn:openspp:vocab:id-type%23test_national_id|REF-IND-001") + self.assertEqual(response.status_code, 200, response.text) + + memberships = response.json()["groupMembership"] + self.assertTrue(memberships) + for membership in memberships: + self._assert_followable(membership["group"]["reference"], "REF-HH-001") + + def _add_removed_id(self, partner, value): + """Give the registrant a newer, soft-removed ID (it sorts first in reg_ids)""" + self.env["spp.registry.id"].create( + { + "partner_id": partner.id, + "id_type_id": self.id_type_household.id, + "value": value, + "status": "invalid", + } + ) + + def test_references_use_a_live_id_not_a_removed_one(self): + """A soft-removed ID no longer resolves, so references must not be built from it""" + self._add_removed_id(self.member, "REF-REMOVED-IND") + removed_group_type = self.env["spp.vocabulary.code"].search( + [("uri", "=", "urn:openspp:vocab:id-type#test_national_id")], limit=1 + ) + self.env["spp.registry.id"].create( + { + "partner_id": self.group.id, + "id_type_id": removed_group_type.id, + "value": "REF-REMOVED-HH", + "status": "invalid", + } + ) + + group_response = self._get(GROUP_PATH) + self.assertEqual(group_response.status_code, 200, group_response.text) + for member in group_response.json()["member"]: + self._assert_followable(member["entity"]["reference"], "REF-IND-001") + + individual_response = self._get( + "/api/v2/spp/Individual/urn:openspp:vocab:id-type%23test_national_id|REF-IND-001" + ) + self.assertEqual(individual_response.status_code, 200, individual_response.text) + for membership in individual_response.json()["groupMembership"]: + self._assert_followable(membership["group"]["reference"], "REF-HH-001") diff --git a/spp_api_v2/tests/test_search_filters_fail_closed.py b/spp_api_v2/tests/test_search_filters_fail_closed.py new file mode 100644 index 000000000..d3954a780 --- /dev/null +++ b/spp_api_v2/tests/test_search_filters_fail_closed.py @@ -0,0 +1,309 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Search filters fail closed (#554 item H). + +A filter the API cannot apply must never widen the result: malformed input +is a 400, and a well-formed value naming nothing (unknown group, role, +gender, member) matches nothing. Multi-condition filters on one2many +relations must hold on the *same* related row. +""" + +import json +from datetime import timedelta + +from odoo import fields +from odoo.tools import mute_logger + +from ..services import search_service +from ..services.search_service import SearchService +from .common import ApiV2HttpTestCase, ApiV2TestCase + +NATIONAL_ID = "urn:openspp:vocab:id-type#test_national_id" +HOUSEHOLD_ID = "urn:openspp:vocab:id-type#test_household_id" + + +class TestSearchFiltersFailClosed(ApiV2TestCase): + """SearchService: malformed filters raise, unresolvable filters match nothing""" + + def setUp(self): + super().setUp() + self.service = SearchService(self.env) + self.ind1 = self.create_test_individual(identifier_value="FC-IND-001", gender_id=self.gender_female.id) + self.ind2 = self.create_test_individual(identifier_value="FC-IND-002", gender_id=self.gender_male.id) + self.group_g = self.create_test_group(name="Group G", identifier_value="FC-HH-G") + self.group_h = self.create_test_group(name="Group H", identifier_value="FC-HH-H") + + def _add_member(self, group, individual, ended=False, role=None): + vals = {"group": group.id, "individual": individual.id} + if ended: + vals["start_date"] = fields.Datetime.now() - timedelta(days=30) + vals["ended_date"] = fields.Datetime.now() - timedelta(days=1) + if role: + vals["membership_type_ids"] = [(4, role.id)] + return self.env["spp.group.membership"].create(vals) + + def _assert_invalid(self, params, search=None): + search = search or self.service.search_individuals + with self.assertRaises(search_service.InvalidSearchParam): + search(params) + + # --- malformed input raises ------------------------------------------- + + def test_malformed_identifier_raises(self): + self._assert_invalid({"identifier": "NO-SEPARATOR"}) + self._assert_invalid({"identifier": "NO-SEPARATOR"}, self.service.search_groups) + + def test_malformed_birthdate_raises(self): + self._assert_invalid({"birthdate": "not-a-date"}) + self._assert_invalid({"birthdate": "ge"}) + + def test_malformed_last_updated_raises(self): + self._assert_invalid({"_lastUpdated": "yesterday"}) + + def test_malformed_gender_raises(self): + self._assert_invalid({"gender": "female"}) + + def test_malformed_group_raises(self): + self._assert_invalid({"group": "NO-SEPARATOR"}) + + def test_malformed_member_raises(self): + self._assert_invalid({"member": f"{NATIONAL_ID}|FC-IND-001"}, self.service.search_groups) + self._assert_invalid({"member": "Individual/NO-SEPARATOR"}, self.service.search_groups) + + # --- unresolvable input matches nothing ------------------------------- + + def test_unknown_group_matches_nothing(self): + """The reported bug: an unknown group returned every individual""" + records, total = self.service.search_individuals({"group": f"{HOUSEHOLD_ID}|DOES-NOT-EXIST"}) + + self.assertEqual(total, 0) + self.assertFalse(records) + + def test_group_identifier_of_an_individual_matches_nothing(self): + """An identifier that resolves to an individual, not a group, is not a group""" + records, total = self.service.search_individuals({"group": f"{NATIONAL_ID}|FC-IND-001"}) + + self.assertEqual(total, 0) + self.assertFalse(records) + + def test_unknown_gender_matches_nothing(self): + records, total = self.service.search_individuals({"gender": "urn:iso:std:iso:5218|no-such-code"}) + + self.assertEqual(total, 0) + self.assertFalse(records) + + def test_unknown_membership_role_matches_nothing(self): + self._add_member(self.group_g, self.ind1, role=self.relationship_head) + + records, total = self.service.search_individuals({"membership-role": "no-such-role"}) + + self.assertEqual(total, 0) + self.assertFalse(records) + + def test_unknown_member_matches_nothing(self): + """An unknown member returned every group""" + records, total = self.service.search_groups({"member": f"Individual/{NATIONAL_ID}|DOES-NOT-EXIST"}) + + self.assertEqual(total, 0) + self.assertFalse(records) + + # --- multi-condition filters hold on the same row --------------------- + + def test_group_filter_ignores_ended_membership_when_active_elsewhere(self): + """Removed from G but active in H: not a member of G""" + self._add_member(self.group_g, self.ind1, ended=True) + self._add_member(self.group_h, self.ind1) + self._add_member(self.group_g, self.ind2) # positive control: a current member of G + + records, _total = self.service.search_individuals({"group": f"{HOUSEHOLD_ID}|FC-HH-G"}) + + self.assertNotIn(self.ind1, records) + self.assertIn(self.ind2, records) + + def test_membership_role_must_be_on_an_active_membership(self): + """Head of G (ended) and plain member of H (active): not a current head""" + self._add_member(self.group_g, self.ind1, ended=True, role=self.relationship_head) + self._add_member(self.group_h, self.ind1) + self._add_member(self.group_h, self.ind2, role=self.relationship_head) # positive control: a current head + + records, _total = self.service.search_individuals({"membership-role": "head"}) + + self.assertNotIn(self.ind1, records) + self.assertIn(self.ind2, records) + + def test_identifier_system_and_value_must_be_on_the_same_id(self): + """System from one ID and value from another is not a match""" + self.env["spp.registry.id"].create( + { + "partner_id": self.ind1.id, + "id_type_id": self.id_type_household.id, + "value": "FC-OTHER-VALUE", + "status": "valid", + } + ) + + records, total = self.service.search_individuals({"identifier": f"{NATIONAL_ID}|FC-OTHER-VALUE"}) + + self.assertEqual(total, 0) + self.assertFalse(records) + + def test_group_and_membership_role_must_be_on_the_same_membership(self): + """Plain member of G and head of H: not a head of G""" + self._add_member(self.group_g, self.ind1) + self._add_member(self.group_h, self.ind1, role=self.relationship_head) + self._add_member(self.group_g, self.ind2, role=self.relationship_head) # positive control: head of G + + records, _total = self.service.search_individuals( + {"group": f"{HOUSEHOLD_ID}|FC-HH-G", "membership-role": "head"} + ) + + self.assertNotIn(self.ind1, records) + self.assertIn(self.ind2, records) + + def test_group_and_membership_role_ignore_ended_membership(self): + """Head of G, but the membership has ended: not a current head of G""" + self._add_member(self.group_g, self.ind1, ended=True, role=self.relationship_head) + + records, _total = self.service.search_individuals( + {"group": f"{HOUSEHOLD_ID}|FC-HH-G", "membership-role": "head"} + ) + + self.assertNotIn(self.ind1, records) + + def test_no_group_and_membership_role_matches_nothing(self): + """group=none (no active group) with a role held on an active membership is contradictory""" + self._add_member(self.group_g, self.ind1, role=self.relationship_head) + + records, _total = self.service.search_individuals({"group": "none", "membership-role": "head"}) + + self.assertNotIn(self.ind1, records) + + def test_member_filter_ignores_ended_memberships(self): + """A group the individual has left is not listed for ?member=""" + self._add_member(self.group_g, self.ind2, ended=True) + self._add_member(self.group_h, self.ind2) + + records, _total = self.service.search_groups({"member": f"Individual/{NATIONAL_ID}|FC-IND-002"}) + + self.assertNotIn(self.group_g, records) + self.assertIn(self.group_h, records) + + +class TestSearchFiltersFailClosedAPI(ApiV2HttpTestCase): + """HTTP: malformed filters are 400, unresolvable filters return nothing""" + + def setUp(self): + super().setUp() + self.individual = self.create_test_individual(identifier_value="FC-API-IND-001") + self.group = self.create_test_group(name="FC API Group", identifier_value="FC-API-HH-001") + self.client = self.create_api_client( + name="Fail Closed Client", + scopes=[ + {"resource": "individual", "action": "read"}, + {"resource": "group", "action": "read"}, + ], + ) + self.create_consent( + registrant=self.individual, + grantee_partner=self.client.partner_id, + resource_type="all", + field_access="all", + ) + self.create_consent( + registrant=self.group, + grantee_partner=self.client.partner_id, + resource_type="all", + field_access="all", + ) + self.token = self.generate_jwt_token(self.client) + + def _get(self, url): + return self.url_open( + url, + headers={"Content-Type": "application/json", "Authorization": f"Bearer {self.token}"}, + ) + + def _assert_empty(self, response): + self.assertEqual(response.status_code, 200, response.text) + data = json.loads(response.content) + self.assertEqual(data["meta"]["total"], 0) + self.assertFalse(data.get("data")) + + def test_individual_group_and_role_on_the_same_membership(self): + """?group=G1&membership-role=head: a plain member of G1 who heads G2 is not returned""" + plain = self.create_test_individual(name="FC Plain G1 Head G2", identifier_value="FC-API-PLAIN") + head = self.create_test_individual(name="FC Head G1", identifier_value="FC-API-HEAD") + self.create_test_group( + name="FC API G1", identifier_value="FC-API-G1", members=[(plain, None), (head, self.relationship_head)] + ) + self.create_test_group( + name="FC API G2", identifier_value="FC-API-G2", members=[(plain, self.relationship_head)] + ) + for person in (plain, head): + self.create_consent( + registrant=person, + grantee_partner=self.client.partner_id, + resource_type="all", + field_access="all", + ) + + response = self._get( + f"/api/v2/spp/Individual?group={HOUSEHOLD_ID.replace('#', '%23')}|FC-API-G1&membership-role=head" + ) + + self.assertEqual(response.status_code, 200, response.text) + values = [r["identifier"][0]["value"] for r in json.loads(response.content)["data"]] + self.assertEqual(values, ["FC-API-HEAD"]) + + def test_huge_offset_is_a_validation_error(self): + """An _offset beyond what the database accepts is a 422, not a server error""" + for resource in ("Individual", "Group"): + with mute_logger("odoo.http"): + response = self._get(f"/api/v2/spp/{resource}?_offset=99999999999999999999") + self.assertEqual(response.status_code, 422, f"{resource}: {response.text[:200]}") + + def test_individual_unknown_group_returns_nothing(self): + self._assert_empty( + self._get("/api/v2/spp/Individual?group=urn:openspp:vocab:id-type%23test_household_id%7CDOES-NOT-EXIST") + ) + + def test_individual_malformed_group_is_400(self): + response = self._get("/api/v2/spp/Individual?group=NO-SEPARATOR") + + self.assertEqual(response.status_code, 400, response.text) + + def test_individual_malformed_identifier_is_400(self): + response = self._get("/api/v2/spp/Individual?identifier=NO-SEPARATOR") + + self.assertEqual(response.status_code, 400, response.text) + + def test_individual_malformed_birthdate_is_400(self): + response = self._get("/api/v2/spp/Individual?birthdate=not-a-date") + + self.assertEqual(response.status_code, 400, response.text) + + def test_individual_malformed_gender_is_400(self): + response = self._get("/api/v2/spp/Individual?gender=female") + + self.assertEqual(response.status_code, 400, response.text) + + def test_individual_malformed_last_updated_is_400(self): + response = self._get("/api/v2/spp/Individual?_lastUpdated=yesterday") + + self.assertEqual(response.status_code, 400, response.text) + + def test_group_malformed_member_is_400(self): + response = self._get("/api/v2/spp/Group?member=NO-SEPARATOR") + + self.assertEqual(response.status_code, 400, response.text) + + def test_group_unknown_member_returns_nothing(self): + self._assert_empty( + self._get( + "/api/v2/spp/Group?member=Individual/urn:openspp:vocab:id-type%23test_national_id%7CDOES-NOT-EXIST" + ) + ) + + def test_group_malformed_identifier_is_400(self): + response = self._get("/api/v2/spp/Group?identifier=NO-SEPARATOR") + + self.assertEqual(response.status_code, 400, response.text) diff --git a/spp_api_v2/tests/test_search_service.py b/spp_api_v2/tests/test_search_service.py index e41e9ea89..9978a20c9 100644 --- a/spp_api_v2/tests/test_search_service.py +++ b/spp_api_v2/tests/test_search_service.py @@ -61,19 +61,23 @@ def test_search_by_name(self): self.assertIn("Alice Brown", names) def test_parse_identifier_param(self): - """identifier=system|value creates proper domain""" + """identifier=system|value creates proper domain, both conditions on the same ID""" domain = self.service._parse_identifier_param("urn:openspp:vocab:id-type#test_national_id|IND-001") - self.assertEqual(len(domain), 2) - self.assertIn( - ( - "reg_ids.id_type_id.uri", - "=", - "urn:openspp:vocab:id-type#test_national_id", - ), + self.assertEqual( domain, + [ + ( + "reg_ids", + "any", + [ + ("id_type_id.uri", "=", "urn:openspp:vocab:id-type#test_national_id"), + ("value", "=", "IND-001"), + ("status", "!=", "invalid"), + ], + ) + ], ) - self.assertIn(("reg_ids.value", "=", "IND-001"), domain) def test_search_by_identifier(self): """Search by identifier finds exact match""" @@ -282,3 +286,12 @@ def test_search_groups_pagination(self): self.assertGreaterEqual(total, 2) self.assertEqual(len(records), 1) + + def test_search_groups_offset(self): + """_offset skips records: page 2 is the second group, not page 1 again""" + all_records, _total = self.service.search_groups({"_count": 100}) + first_page, _total = self.service.search_groups({"_count": 1, "_offset": 0}) + second_page, _total = self.service.search_groups({"_count": 1, "_offset": 1}) + + self.assertEqual(first_page, all_records[0]) + self.assertEqual(second_page, all_records[1]) diff --git a/spp_api_v2/utils/pagination.py b/spp_api_v2/utils/pagination.py index ca3acabd5..106834d18 100644 --- a/spp_api_v2/utils/pagination.py +++ b/spp_api_v2/utils/pagination.py @@ -5,6 +5,34 @@ _logger = logging.getLogger(__name__) +# The search services return at most this many rows per query, so a batch +# must not ask for more: a short batch is read as the end of the data +MAX_ROWS_PER_QUERY = 100 + +# Largest _offset PostgreSQL accepts (bigint); larger values are rejected as +# request validation errors instead of reaching the database +MAX_OFFSET = 2**63 - 1 + + +def page_total_and_next(returned, count, offset, db_offset_consumed, raw_total, consent_filtered): + """ + Return the (total, next_offset) to publish for a search page. + + For a client subject to consent filtering the total is the number of + records returned, whether or not this page met a hidden record: the raw + total counts records the client may not see, and a page past the end + would otherwise reveal whether a hidden match exists. The next page + starts after the last row examined and is given while rows remain, even + when the page came back short. + + next_offset is None on the last page. + """ + if consent_filtered: + next_offset = db_offset_consumed if db_offset_consumed < raw_total else None + return returned, next_offset + next_offset = offset + count if returned >= count else None + return raw_total, next_offset + def fetch_with_consent( search_function, @@ -16,11 +44,10 @@ def fetch_with_consent( Fetch records with consent filtering, over-fetching to fill pages. When consent filtering removes records from a page, this function - fetches additional records to fill the requested page size. This - prevents clients from seeing short pages due to consent denials. - - To prevent leaking the count of denied records, the total is - suppressed when any consent filtering occurs. + fetches additional records to fill the requested page size, up to a + safety limit of 3x the page size. A page can therefore still come back + short, or empty, while rows remain: clients follow `next` until it is + null (see page_total_and_next for the total and next offset to publish). Args: search_function: callable(offset, limit) -> (records, total) @@ -32,6 +59,9 @@ def fetch_with_consent( Returns: tuple of (collected_records, database_offset_consumed, raw_total, consent_was_applied) + + database_offset_consumed is the offset just after the last row + examined, so the next page starts at the first row not yet seen. """ collected = [] current_offset = offset @@ -42,11 +72,13 @@ def fetch_with_consent( fetched_so_far = 0 while len(collected) < count and fetched_so_far < max_overfetch: - batch_size = min(count * 2, max_overfetch - fetched_so_far) + batch_size = min(count * 2, max_overfetch - fetched_so_far, MAX_ROWS_PER_QUERY) records, raw_total = search_function(offset=current_offset, limit=batch_size) if not records: break + examined = 0 for record in records: + examined += 1 filtered = consent_filter_function(record) if filtered is not None: collected.append(filtered) @@ -54,8 +86,10 @@ def fetch_with_consent( break else: consent_was_applied = True - fetched_so_far += len(records) - current_offset += len(records) + # Rows after the one that filled the page were not examined: the next + # page must start at them, not skip them + fetched_so_far += examined + current_offset += examined if len(records) < batch_size: break # No more records in DB diff --git a/spp_api_v2/utils/registrant_lookup.py b/spp_api_v2/utils/registrant_lookup.py new file mode 100644 index 000000000..94dcdda87 --- /dev/null +++ b/spp_api_v2/utils/registrant_lookup.py @@ -0,0 +1,85 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""HTTP answer for an identifier that matches more than one registrant. + +The API refuses to guess (409). But saying "this identifier is ambiguous" +also says "it exists", so a client only learns it when it may read every +matching registrant; otherwise it gets exactly what it would get for a +registrant that does not exist (the API's anti-enumeration rule): +the 403 of a read, or the empty page of a search. +""" + +import asyncio +import random + +from fastapi import HTTPException, status + +from ..services.consent_service import ConsentService +from ..services.registrant_resolver import AmbiguousIdentifierError + +AMBIGUOUS_DETAIL = "Identifier matches more than one registrant" + + +def consent_denied(env, api_client, partner, resource_type) -> bool: + """Whether the client may not read ``partner``'s ``resource_type`` data. + + The same rule as a successful read (legal basis, consent, consent scope), + evaluated without logging an access. + """ + filtered = ConsentService(env).filter_response(partner.id, api_client, resource_type, {}, log_access=False) + return filtered.get("_consent", {}).get("status") in ("no_consent", "scope_mismatch") + + +async def deny_access(): + """Raise the anti-enumeration 403, with the 50-70ms delay of the "not found" path.""" + # SECURITY: the same delay whether the record is missing, ambiguous or + # not consented, so timing does not tell them apart + await asyncio.sleep(0.05 + random.uniform(0, 0.02)) + raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="Access denied") + + +def ambiguous_identifier_status(env, api_client, error: AmbiguousIdentifierError, resource_type=None) -> int: + """ + 409 if the client may read every match, else 403. + + Args: + resource_type: consent resource to check; by default each match's own + kind ("group" or "individual") + """ + for partner in error.partners: + kind = resource_type or ("group" if partner.is_group else "individual") + if consent_denied(env, api_client, partner, kind): + return status.HTTP_403_FORBIDDEN + return status.HTTP_409_CONFLICT + + +async def raise_ambiguous_identifier(env, api_client, error, resource_type=None): + """Raise the 409, or the jittered "not found" 403, for an ambiguous identifier.""" + if ambiguous_identifier_status(env, api_client, error, resource_type) == status.HTTP_403_FORBIDDEN: + await deny_access() + raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=AMBIGUOUS_DETAIL) + + +def ambiguous_filter_result(env, api_client, error, empty_records, resource_type=None): + """ + Search-filter form: an empty page (what "not found" looks like in a search) + for a client that may not know, else 409. + + Returns: + (empty_records, 0), the (records, total) shape of a search function + """ + if ambiguous_identifier_status(env, api_client, error, resource_type) == status.HTTP_403_FORBIDDEN: + return empty_records, 0 + raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=AMBIGUOUS_DETAIL) + + +async def lookup_registrant(env, api_client, lookup, system, value, resource_type=None): + """ + Call ``lookup(system, value)``, answering an ambiguous identifier with 409/403. + + Returns: + whatever ``lookup`` returns (a registrant or an empty recordset) + """ + try: + return lookup(system, value) + except AmbiguousIdentifierError as e: + await raise_ambiguous_identifier(env, api_client, e, resource_type) diff --git a/spp_api_v2_programs/README.rst b/spp_api_v2_programs/README.rst index c0a4f9cae..e1589009c 100644 --- a/spp_api_v2_programs/README.rst +++ b/spp_api_v2_programs/README.rst @@ -66,6 +66,77 @@ Dependencies Changelog ========= +19.0.1.1.0 +~~~~~~~~~~ + +- fix: ``GET``/``PUT /ProgramMembership/{identifier}`` no longer resolve + to an arbitrary program's membership when the beneficiary is enrolled + in several programs (#554). A new optional ``program`` query parameter + (``Program/{system}|{value}``) selects the membership (a malformed + value returns ``400``, an unknown program ``404``); without it, a + beneficiary with more than one membership returns ``409``. When no + single membership resolves, a client that requires consent and may not + read the beneficiary gets ``403`` whatever the reason (unknown + beneficiary, not enrolled in that program, several memberships), so + neither the registry's contents nor enrollments are revealed; the + ``409`` message does not give the number of memberships. +- fix: ``PUT /ProgramMembership`` can no longer move a membership to + another program or beneficiary: a body whose ``program`` or + ``beneficiary`` differs from the addressed membership returns ``422`` + (#554). +- fix: the ``Location`` header of ``POST /ProgramMembership`` is + URL-encoded and carries ``?program=``, so following it returns the + created membership (#554). +- fix: ``PUT /ProgramMembership`` accepts its own + ``ETag``/``meta.versionId`` in ``If-Match``; it previously compared + against a different timestamp format and always returned ``409`` + (#554). +- fix: a beneficiary identifier held by more than one registrant is + refused on ``GET``/``PUT``/``POST /ProgramMembership`` (``409``, or + ``403`` for a client that requires consent and may not read every + match) and in the ``beneficiary`` search filter (``409``, or an empty + page for such a client), instead of acting on an arbitrary one; + ``Individual/`` and ``Group/`` beneficiary references resolve only + that kind. Membership identifiers, beneficiary references and + ``Location`` use only live (not soft-removed) IDs (#554). +- fix: ``GET /ProgramMembership`` filters fail closed (#554). A + ``beneficiary`` that isn't ``Individual/{system}|{value}`` or + ``Group/{system}|{value}``, or a ``program`` that isn't + ``Program/{system}|{value}``, returns ``400`` instead of being dropped + and returning every membership. A well-formed reference that matches + nothing still returns an empty page. +- fix: ``POST /ProgramMembership`` for a beneficiary already in the + program returns ``409`` "Beneficiary is already a member of this + program" (#554). It returned ``422`` with the database error text, + including internal record ids. Create and ``PUT`` errors that aren't + client errors now return a generic message; the details are only + logged. +- fix: ``GET /Program`` without the ``program:search`` or + ``program:read`` scope returns ``403`` (#554). The ``status`` query + parameter shadowed FastAPI's ``status`` module, so it returned + ``500``. +- fix: consent-filtered ``GET /ProgramMembership`` pages no longer skip + records or stop early, and ``meta.total`` never counts memberships the + client may not see (#554, see ``spp_api_v2``). +- fix: ``GET /ProgramMembership`` ``self``/``next``/``prev`` links are + URL-encoded (#554). The ``#`` in a beneficiary's identifier type cut + the link short, so following ``next`` lost the beneficiary filter. +- fix: an unexpected error in ``GET /ProgramMembership`` returns ``500`` + "Failed to search program memberships" instead of ``400`` with the + error text (#554); malformed filters are still ``400``. An ``_offset`` + beyond what the database accepts returns ``422``. +- fix(security): whether a client gets the jittered ``403`` for an + unknown beneficiary follows its legal basis, the rule consent + filtering uses, not its ``is_require_consent`` box (#554 review, see + ``spp_api_v2``). +- fix: memberships of an archived beneficiary can be read and updated + with ``GET``/``PUT /ProgramMembership`` (#554 review). The lookup + dropped them, while ``POST`` answered ``409`` "already a member" for + the same membership. +- fix: the ``beneficiary`` and ``program`` search filters parse + references as ``POST``/``PUT`` bodies do (#554 review), so a + percent-encoded system (``...%23national_id``) matches on ``GET`` too. + 19.0.1.0.0 ~~~~~~~~~~ diff --git a/spp_api_v2_programs/__manifest__.py b/spp_api_v2_programs/__manifest__.py index 5becc04d0..01a4a05a1 100644 --- a/spp_api_v2_programs/__manifest__.py +++ b/spp_api_v2_programs/__manifest__.py @@ -2,7 +2,7 @@ { # pylint: disable=pointless-statement "name": "OpenSPP API V2 - Programs", "category": "OpenSPP/Integration", - "version": "19.0.1.0.0", + "version": "19.0.1.1.0", "sequence": 1, "author": "OpenSPP.org", "website": "https://github.com/OpenSPP/OpenSPP2", diff --git a/spp_api_v2_programs/readme/HISTORY.md b/spp_api_v2_programs/readme/HISTORY.md index a87d1ffe4..6db4a70f1 100644 --- a/spp_api_v2_programs/readme/HISTORY.md +++ b/spp_api_v2_programs/readme/HISTORY.md @@ -1,3 +1,20 @@ +### 19.0.1.1.0 + +- fix: `GET`/`PUT /ProgramMembership/{identifier}` no longer resolve to an arbitrary program's membership when the beneficiary is enrolled in several programs (#554). A new optional `program` query parameter (`Program/{system}|{value}`) selects the membership (a malformed value returns `400`, an unknown program `404`); without it, a beneficiary with more than one membership returns `409`. When no single membership resolves, a client that requires consent and may not read the beneficiary gets `403` whatever the reason (unknown beneficiary, not enrolled in that program, several memberships), so neither the registry's contents nor enrollments are revealed; the `409` message does not give the number of memberships. +- fix: `PUT /ProgramMembership` can no longer move a membership to another program or beneficiary: a body whose `program` or `beneficiary` differs from the addressed membership returns `422` (#554). +- fix: the `Location` header of `POST /ProgramMembership` is URL-encoded and carries `?program=`, so following it returns the created membership (#554). +- fix: `PUT /ProgramMembership` accepts its own `ETag`/`meta.versionId` in `If-Match`; it previously compared against a different timestamp format and always returned `409` (#554). +- fix: a beneficiary identifier held by more than one registrant is refused on `GET`/`PUT`/`POST /ProgramMembership` (`409`, or `403` for a client that requires consent and may not read every match) and in the `beneficiary` search filter (`409`, or an empty page for such a client), instead of acting on an arbitrary one; `Individual/` and `Group/` beneficiary references resolve only that kind. Membership identifiers, beneficiary references and `Location` use only live (not soft-removed) IDs (#554). +- fix: `GET /ProgramMembership` filters fail closed (#554). A `beneficiary` that isn't `Individual/{system}|{value}` or `Group/{system}|{value}`, or a `program` that isn't `Program/{system}|{value}`, returns `400` instead of being dropped and returning every membership. A well-formed reference that matches nothing still returns an empty page. +- fix: `POST /ProgramMembership` for a beneficiary already in the program returns `409` "Beneficiary is already a member of this program" (#554). It returned `422` with the database error text, including internal record ids. Create and `PUT` errors that aren't client errors now return a generic message; the details are only logged. +- fix: `GET /Program` without the `program:search` or `program:read` scope returns `403` (#554). The `status` query parameter shadowed FastAPI's `status` module, so it returned `500`. +- fix: consent-filtered `GET /ProgramMembership` pages no longer skip records or stop early, and `meta.total` never counts memberships the client may not see (#554, see `spp_api_v2`). +- fix: `GET /ProgramMembership` `self`/`next`/`prev` links are URL-encoded (#554). The `#` in a beneficiary's identifier type cut the link short, so following `next` lost the beneficiary filter. +- fix: an unexpected error in `GET /ProgramMembership` returns `500` "Failed to search program memberships" instead of `400` with the error text (#554); malformed filters are still `400`. An `_offset` beyond what the database accepts returns `422`. +- fix(security): whether a client gets the jittered `403` for an unknown beneficiary follows its legal basis, the rule consent filtering uses, not its `is_require_consent` box (#554 review, see `spp_api_v2`). +- fix: memberships of an archived beneficiary can be read and updated with `GET`/`PUT /ProgramMembership` (#554 review). The lookup dropped them, while `POST` answered `409` "already a member" for the same membership. +- fix: the `beneficiary` and `program` search filters parse references as `POST`/`PUT` bodies do (#554 review), so a percent-encoded system (`...%23national_id`) matches on `GET` too. + ### 19.0.1.0.0 - Split program and program-membership REST endpoints out of `spp_api_v2` into this companion module (#1081), so `spp_api_v2` no longer depends on `spp_programs`. diff --git a/spp_api_v2_programs/routers/program.py b/spp_api_v2_programs/routers/program.py index 98d431fb5..a55055c18 100644 --- a/spp_api_v2_programs/routers/program.py +++ b/spp_api_v2_programs/routers/program.py @@ -85,7 +85,7 @@ async def search_programs( api_client: Annotated[dict, Depends(get_authenticated_client)], identifier: Annotated[str | None, Query()] = None, name: Annotated[str | None, Query()] = None, - status: Annotated[str | None, Query()] = None, + status_: Annotated[str | None, Query(alias="status")] = None, type_: Annotated[str | None, Query(alias="type")] = None, target_type: Annotated[str | None, Query(alias="targetType")] = None, count: Annotated[int, Query(alias="_count", ge=1, le=100)] = 20, @@ -121,10 +121,10 @@ async def search_programs( domain.append(("name", "ilike", name)) # Status search - if status: - if status == "active": + if status_: + if status_ == "active": domain.append(("state", "=", "active")) - elif status == "ended": + elif status_ == "ended": domain.append(("state", "=", "ended")) # Target type search @@ -160,8 +160,8 @@ async def search_programs( params = {} if name: params["name"] = name - if status: - params["status"] = status + if status_: + params["status"] = status_ if target_type: params["targetType"] = target_type params["_count"] = str(count) diff --git a/spp_api_v2_programs/routers/program_membership.py b/spp_api_v2_programs/routers/program_membership.py index 0a3b34e6c..713b1e1a9 100644 --- a/spp_api_v2_programs/routers/program_membership.py +++ b/spp_api_v2_programs/routers/program_membership.py @@ -3,14 +3,24 @@ import logging from typing import Annotated +from urllib.parse import quote, urlencode from odoo.api import Environment +from odoo.exceptions import ValidationError from odoo.addons.fastapi.dependencies import odoo_env from odoo.addons.spp_api_v2.middleware.auth import get_authenticated_client from odoo.addons.spp_api_v2.schemas.search_result import SearchResult, create_search_result from odoo.addons.spp_api_v2.services.consent_service import ConsentService -from odoo.addons.spp_api_v2.utils.pagination import fetch_with_consent +from odoo.addons.spp_api_v2.services.registrant_resolver import AmbiguousIdentifierError +from odoo.addons.spp_api_v2.utils.pagination import MAX_OFFSET, fetch_with_consent, page_total_and_next +from odoo.addons.spp_api_v2.utils.registrant_lookup import ( + ambiguous_filter_result, + consent_denied, + deny_access, + lookup_registrant, + raise_ambiguous_identifier, +) from fastapi import ( APIRouter, @@ -25,24 +35,105 @@ ) from ..schemas.program_membership import ProgramMembership -from ..services.program_membership_service import ProgramMembershipService +from ..services.program_membership_service import ( + AmbiguousMembershipError, + DuplicateMembershipError, + ProgramMembershipService, +) _logger = logging.getLogger(__name__) program_membership_router = APIRouter(tags=["ProgramMembership"], prefix="/ProgramMembership") +PROGRAM_PARAM_DESCRIPTION = ( + "Program of the membership (format: Program/{system}|{value}). " + "Required when the beneficiary is enrolled in more than one program." +) + + +def _resolve_program_param(service, program): + """Resolve the ``program`` query parameter: 400 if malformed, 404 if unknown.""" + if not program: + return None + try: + program_record = service._parse_program_reference(program) + except ValidationError as e: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=e.args[0], + ) from e + if not program_record: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail="Program not found", + ) + return program_record + + +async def _resolve_membership(env, api_client, service, system, value, program_record): + """Resolve the addressed membership, or raise what the client may be told. + + When no single membership resolves, a client that may not read the + beneficiary gets the same jittered 403 whatever the reason (unknown + beneficiary, not enrolled in this program, several memberships), so + neither the registry's contents nor enrollments are revealed. + """ + partner = await lookup_registrant( + env, api_client, service.find_beneficiary, system, value, resource_type="program_membership" + ) + if not partner: + if ConsentService.is_consent_filtered(api_client): + await deny_access() + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail="ProgramMembership not found", + ) + + ambiguous = None + try: + membership = service.find_for_beneficiary(partner, program_record) + except AmbiguousMembershipError as e: + membership = env["spp.program.membership"] + ambiguous = e + + if not membership: + if consent_denied(env, api_client, partner, "program_membership"): + await deny_access() + if ambiguous: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=str(ambiguous), + ) + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail="ProgramMembership not found", + ) + return membership + + +def _membership_location(data: dict) -> str | None: + """Location of a membership: beneficiary identifier plus program, URL-encoded.""" + if not data.get("identifier"): + return None + primary_id = data["identifier"][0] + path_id = f"{quote(primary_id['system'], safe=':')}|{quote(primary_id['value'], safe='')}" + program_ref = quote(data["program"]["reference"], safe=":/|") + return f"/api/v2/spp/ProgramMembership/{path_id}?program={program_ref}" + @program_membership_router.get("/{identifier}", response_model=ProgramMembership) async def read_program_membership( - identifier: Annotated[str, Path(description="Format: {system}|{value} (URL-encoded)")], + identifier: Annotated[str, Path(description="Beneficiary identifier. Format: {system}|{value} (URL-encoded)")], env: Annotated[Environment, Depends(odoo_env)], api_client: Annotated[dict, Depends(get_authenticated_client)], response: Response, + program: Annotated[str | None, Query(description=PROGRAM_PARAM_DESCRIPTION)] = None, ): """ - Read ProgramMembership by external identifier. + Read ProgramMembership by beneficiary identifier and, optionally, program. - Applies consent filtering for beneficiary data. + Returns 409 when the beneficiary has several memberships and no + ``program`` is given. Applies consent filtering for beneficiary data. """ # Check scope if not api_client.has_scope("program_membership", "read"): @@ -60,21 +151,16 @@ async def read_program_membership( system, value = identifier.split("|", 1) - # Find program membership by namespace_uri + value service = ProgramMembershipService(env) - membership = service.find_by_identifier(system, value) + consent_service = ConsentService(env) + program_record = _resolve_program_param(service, program) - if not membership: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail="ProgramMembership not found", - ) + membership = await _resolve_membership(env, api_client, service, system, value, program_record) # Convert to API schema data = service.to_api_schema(membership) # Apply consent filtering for the beneficiary - consent_service = ConsentService(env) filtered_data = consent_service.filter_response( membership.partner_id.id, api_client, @@ -116,7 +202,7 @@ async def search_program_memberships( program: Annotated[str | None, Query()] = None, status_: Annotated[str | None, Query(alias="status")] = None, count: Annotated[int, Query(alias="_count", ge=1, le=100)] = 20, - offset: Annotated[int, Query(alias="_offset", ge=0)] = 0, + offset: Annotated[int, Query(alias="_offset", ge=0, le=MAX_OFFSET)] = 0, ): """ Search for program memberships. @@ -154,11 +240,21 @@ def search_function(offset, limit): search_params = {**params, "_count": limit, "_offset": offset} try: return service.search(search_params) - except Exception as e: - _logger.warning("Error in program membership search: %s", e) + except AmbiguousIdentifierError as e: + # The beneficiary filter matches more than one registrant + return ambiguous_filter_result(env, api_client, e, env["spp.program.membership"], "program_membership") + except ValidationError as e: + # A filter the API can't apply (malformed reference): our own message raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, - detail=f"Invalid search parameters: {e}", + detail=f"Invalid search parameters: {e.args[0]}", + ) from e + except Exception as e: + # Unexpected errors can carry database internals: log them, don't return them + _logger.exception("Error in program membership search") + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail="Failed to search program memberships", ) from e def consent_filter_function(membership): @@ -186,43 +282,33 @@ def consent_filter_function(membership): search_function, consent_filter_function, count, offset ) - # Suppress total when consent filtering is active - if consent_was_applied: - total = len(resources) - else: - total = raw_total + # For a consent-filtered client the total is the page size and the next + # link continues while rows remain (see page_total_and_next) + total, next_offset = page_total_and_next( + returned=len(resources), + count=count, + offset=offset, + db_offset_consumed=db_offset_consumed, + raw_total=raw_total, + consent_filtered=consent_was_applied or consent_service.is_consent_filtered(api_client), + ) - # Build pagination links - url_params = {} + # Build pagination links; references carry '#' and '|', so they are + # URL-encoded: a raw '#' would cut the link short at the fragment + base_params = {} if beneficiary: - url_params["beneficiary"] = beneficiary + base_params["beneficiary"] = beneficiary if program: - url_params["program"] = program + base_params["program"] = program if status_: - url_params["status"] = status_ - url_params["_count"] = str(count) - url_params["_offset"] = str(offset) - - query_string = "&".join(f"{k}={v}" for k, v in url_params.items()) - self_url = f"{request.url.path}?{query_string}" - - next_offset = db_offset_consumed if consent_was_applied else offset + count - has_more = len(resources) >= count - - next_url = None - if has_more: - next_params = url_params.copy() - next_params["_offset"] = str(next_offset) - next_query_string = "&".join(f"{k}={v}" for k, v in next_params.items()) - next_url = f"{request.url.path}?{next_query_string}" - - prev_url = None - if offset > 0: - prev_offset = max(0, offset - count) - prev_params = url_params.copy() - prev_params["_offset"] = str(prev_offset) - prev_query_string = "&".join(f"{k}={v}" for k, v in prev_params.items()) - prev_url = f"{request.url.path}?{prev_query_string}" + base_params["status"] = status_ + + def build_url(offset_val: int) -> str: + return f"{request.url.path}?{urlencode({**base_params, '_count': count, '_offset': offset_val})}" + + self_url = build_url(offset) + next_url = build_url(next_offset) if next_offset is not None else None + prev_url = build_url(max(0, offset - count)) if offset > 0 else None return create_search_result( data=resources, @@ -262,20 +348,31 @@ async def create_program_membership( try: membership = service.create(program_membership, source=source_system) + except AmbiguousIdentifierError as e: + # The beneficiary reference matches more than one registrant + await raise_ambiguous_identifier(env, api_client, e, "program_membership") + except DuplicateMembershipError as e: + raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=str(e)) from e + except ValidationError as e: + # Client error (unknown program or beneficiary): our own message, no traceback + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=f"Failed to create program membership: {e.args[0]}", + ) from e except Exception as e: + # Unexpected errors can carry database internals: log them, don't return them _logger.exception("Error creating program membership") raise HTTPException( status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, - detail=f"Failed to create program membership: {str(e)}", + detail="Failed to create program membership", ) from e # Return created resource data = service.to_api_schema(membership) # Set Location header - if data.get("identifier"): - primary_id = data["identifier"][0] - location = f"/api/v2/spp/ProgramMembership/{primary_id['system']}|{primary_id['value']}" + location = _membership_location(data) + if location: response.headers["Location"] = location return data @@ -283,11 +380,12 @@ async def create_program_membership( @program_membership_router.put("/{identifier}", response_model=ProgramMembership) async def update_program_membership( - identifier: Annotated[str, Path()], + identifier: Annotated[str, Path(description="Beneficiary identifier. Format: {system}|{value} (URL-encoded)")], program_membership: ProgramMembership, env: Annotated[Environment, Depends(odoo_env)], api_client: Annotated[dict, Depends(get_authenticated_client)], if_match: Annotated[str | None, Header()] = None, + program: Annotated[str | None, Query(description=PROGRAM_PARAM_DESCRIPTION)] = None, ): """ Update ProgramMembership (full replacement). @@ -296,6 +394,10 @@ async def update_program_membership( - Requires If-Match header with version ID for optimistic locking - Updates with source tracking - Checks client scopes for update permission + - Returns 409 when the beneficiary has several memberships and no + ``program`` is given + - The body's program and beneficiary must match the addressed + membership (422 otherwise); a PUT never moves a membership """ # Check client has update scope if not api_client.has_scope("program_membership", "update"): @@ -315,19 +417,12 @@ async def update_program_membership( # Find program membership service = ProgramMembershipService(env) - membership = service.find_by_identifier(system, value) + program_record = _resolve_program_param(service, program) + membership = await _resolve_membership(env, api_client, service, system, value, program_record) - if not membership: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail="ProgramMembership not found", - ) - - # Check version for optimistic locking + # Check version for optimistic locking (the version meta.versionId / ETag carry) if if_match: - current_version = str(membership.write_date.timestamp() if membership.write_date else 1) - if_match_clean = if_match.strip('"') - if if_match_clean != current_version: + if if_match.strip('"') != service.version_id(membership): raise HTTPException( status_code=status.HTTP_409_CONFLICT, detail="Version conflict. Resource was modified by another request.", @@ -338,11 +433,21 @@ async def update_program_membership( try: membership = service.update(membership, program_membership, source=source_system) + except AmbiguousIdentifierError as e: + # The body's beneficiary reference matches more than one registrant + await raise_ambiguous_identifier(env, api_client, e, "program_membership") + except ValidationError as e: + # Client error (unknown reference, identity mismatch): no traceback + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=f"Failed to update program membership: {e.args[0]}", + ) from e except Exception as e: + # Unexpected errors can carry database internals: log them, don't return them _logger.exception("Error updating program membership") raise HTTPException( status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, - detail=f"Failed to update program membership: {str(e)}", + detail="Failed to update program membership", ) from e # Return updated resource diff --git a/spp_api_v2_programs/services/program_membership_service.py b/spp_api_v2_programs/services/program_membership_service.py index 5625d3ee1..25e37c57a 100644 --- a/spp_api_v2_programs/services/program_membership_service.py +++ b/spp_api_v2_programs/services/program_membership_service.py @@ -4,13 +4,46 @@ import logging from typing import Any +from psycopg2.errors import UniqueViolation + from odoo.api import Environment from odoo.exceptions import ValidationError +from odoo.addons.spp_api_v2.services.registrant_resolver import ( + live_registry_ids, + primary_registry_id, + resolve_registrant, +) +from odoo.addons.spp_api_v2.utils.pagination import MAX_ROWS_PER_QUERY + from ..schemas.program_membership import ProgramMembership _logger = logging.getLogger(__name__) +INVALID_BENEFICIARY_FILTER = ( + "Invalid beneficiary format. Expected: Individual/{system}|{value} or Group/{system}|{value}" +) +INVALID_PROGRAM_FILTER = "Invalid program format. Expected: Program/{system}|{value}" + +# PostgreSQL name of spp.program.membership's UNIQUE(partner_id, program_id) +# constraint (_unique_partner_program) +MEMBERSHIP_UNIQUE_CONSTRAINT = "spp_program_membership_unique_partner_program" + + +class DuplicateMembershipError(Exception): + """The beneficiary already has a membership in the program.""" + + def __init__(self): + super().__init__("Beneficiary is already a member of this program") + + +class AmbiguousMembershipError(Exception): + """The beneficiary has several program memberships and no program was given.""" + + def __init__(self): + # The message leaves the count out: it is returned to API clients + super().__init__("Beneficiary has several program memberships; specify the program") + class ProgramMembershipService: """Service for ProgramMembership resource CRUD and mapping""" @@ -35,50 +68,25 @@ def search(self, params: dict[str, Any]) -> tuple[list, int]: """ domain = [] - # Beneficiary search + # Filters parse references as bodies do. A filter that can't be + # applied must not widen the result to every membership: it raises beneficiary = params.get("beneficiary") if beneficiary: - if beneficiary.startswith("Individual/") or beneficiary.startswith("Group/"): - if beneficiary.startswith("Individual/"): - identifier_str = beneficiary.replace("Individual/", "") - else: - identifier_str = beneficiary.replace("Group/", "") - - if "|" in identifier_str: - system, value = identifier_str.split("|", 1) - reg_id = ( - self.env["spp.registry.id"] # nosemgrep: odoo-sudo-without-context - .sudo() - .search( - [ - ("id_type_id.uri", "=", system), - ("value", "=", value), - ], - limit=1, - ) - ) - if reg_id and reg_id.partner_id: - domain.append(("partner_id", "=", reg_id.partner_id.id)) - else: - # No matching partner found, return empty result - domain.append(("id", "=", -1)) + try: + # Raises AmbiguousIdentifierError when several registrants hold it + partner = self._parse_beneficiary_reference(beneficiary) + except ValidationError as e: + raise ValidationError(INVALID_BENEFICIARY_FILTER) from e + # An unknown beneficiary matches no membership + domain.append(("partner_id", "=", partner.id) if partner else ("id", "=", -1)) - # Program search program = params.get("program") if program: - if program.startswith("Program/"): - identifier_str = program.replace("Program/", "") - if "|" in identifier_str: - system, value = identifier_str.split("|", 1) - from .program_service import ProgramService - - program_service = ProgramService(self.env) - prog = program_service.find_by_identifier(system, value) - if prog: - domain.append(("program_id", "=", prog.id)) - else: - # No matching program found, return empty result - domain.append(("id", "=", -1)) + try: + prog = self._parse_program_reference(program) + except ValidationError as e: + raise ValidationError(INVALID_PROGRAM_FILTER) from e + domain.append(("program_id", "=", prog.id) if prog else ("id", "=", -1)) # Status search status = params.get("status") @@ -86,7 +94,7 @@ def search(self, params: dict[str, Any]) -> tuple[list, int]: domain.append(("state", "=", status)) # Execute search - count = min(int(params.get("_count", 20)), 100) + count = min(int(params.get("_count", 20)), MAX_ROWS_PER_QUERY) offset = int(params.get("_offset", 0)) Membership = self.env["spp.program.membership"] @@ -100,47 +108,56 @@ def search(self, params: dict[str, Any]) -> tuple[list, int]: return records, total - def find_by_identifier(self, system_uri: str, value: str): + def find_beneficiary(self, system_uri: str, value: str, is_group=None): """ - Lookup program membership by external identifier. + Lookup the beneficiary (Individual or Group) by external identifier. Args: - system_uri: Full URI of identifier type (e.g., urn:openspp:vocab:id-type#national_id) - value: Identifier value + is_group: True/False when the reference names the kind + (``Group/`` or ``Individual/``), None when it does not Returns: - spp.program.membership record or empty recordset + res.partner record or empty recordset + + Raises: + AmbiguousIdentifierError: several registrants hold the identifier """ - # Program memberships might have identifiers via their partner_id - # or via a custom identifier system. Check partner's registry IDs. + return resolve_registrant(self.env, system_uri, value, is_group=is_group) - reg_id = ( - self.env["spp.registry.id"] # nosemgrep: odoo-sudo-without-context - .sudo() - .search( - [ - ("id_type_id.uri", "=", system_uri), - ("value", "=", value), - ], - limit=1, - ) - ) + def find_for_beneficiary(self, partner, program=None): + """ + Find the beneficiary's membership, in ``program`` when given. - if reg_id and reg_id.partner_id: - # Find membership for this partner - # We might have multiple memberships, so we need program reference too - # For now, return the first membership found - membership = ( - self.env["spp.program.membership"] # nosemgrep: odoo-sudo-without-context - .sudo() - .search( - [("partner_id", "=", reg_id.partner_id.id)], - limit=1, - ) - ) - return membership + A membership has no identifier of its own: it is addressed by its + beneficiary's identifier (resolved with :meth:`find_beneficiary`) + plus, when the beneficiary is enrolled in more than one program, the + program. Never picks one of several memberships: without a program, a + beneficiary enrolled in more than one program is ambiguous. - return self.env["spp.program.membership"] + Returns: + spp.program.membership record or empty recordset + + Raises: + AmbiguousMembershipError: no program given and the beneficiary + has several memberships + """ + domain = [("partner_id", "=", partner.id)] + if program: + domain.append(("program_id", "=", program.id)) + # The membership _inherits res.partner's ``active``: without + # active_test=False an archived beneficiary's memberships vanish here + # while the duplicate check (_find_existing_membership) still sees them + Membership = ( + self.env["spp.program.membership"] # nosemgrep: odoo-sudo-without-context + .sudo() + .with_context(active_test=False) + ) + memberships = Membership.search(domain, limit=2) + if len(memberships) > 1: + raise AmbiguousMembershipError() + # Hand back a recordset without active_test=False, so reads through it + # (e.g. the beneficiary's IDs) keep the usual archived-record filtering + return self.env["spp.program.membership"].sudo().browse(memberships.ids) # nosemgrep: odoo-sudo-without-context def find_by_partner_and_program(self, partner_id: int, program_id: int): """ @@ -165,6 +182,12 @@ def find_by_partner_and_program(self, partner_id: int, program_id: int): ) ) + @staticmethod + def version_id(membership) -> str: + """The membership's version: meta.versionId, its ETag, and what If-Match is checked against""" + # Integer microseconds avoid float precision issues + return str(int(membership.write_date.timestamp() * 1000000)) if membership.write_date else "1" + def to_api_schema(self, membership, extensions=None) -> dict[str, Any]: """ Convert Odoo program membership to ProgramMembership API schema. @@ -182,9 +205,10 @@ def to_api_schema(self, membership, extensions=None) -> dict[str, Any]: # Build identifier list (optional for memberships) identifiers = [] - # Use partner's identifiers as the membership identifiers - if membership.partner_id and membership.partner_id.reg_ids: - for reg_id in membership.partner_id.reg_ids: + # Use partner's live identifiers as the membership identifiers: they + # address the membership, and a removed ID no longer resolves + if membership.partner_id: + for reg_id in live_registry_ids(membership.partner_id): # Use id_type_id.uri for full code URI # NOT namespace_uri which only returns vocabulary namespace if reg_id.id_type_id and reg_id.id_type_id.uri and reg_id.value: @@ -229,10 +253,8 @@ def to_api_schema(self, membership, extensions=None) -> dict[str, Any]: # For now, we don't have exit_reason_id in the base model # Metadata - # Use integer microseconds for versionId to avoid float precision issues - version_id = str(int(membership.write_date.timestamp() * 1000000)) if membership.write_date else "1" membership_data["meta"] = { - "versionId": version_id, + "versionId": self.version_id(membership), "lastUpdated": membership.write_date.isoformat() if membership.write_date else None, "source": None, # Memberships don't have source tracking in base module } @@ -266,9 +288,9 @@ def _build_beneficiary_reference(self, partner) -> dict: # Determine resource type resource_type = "Group" if partner.is_group else "Individual" - # Get primary identifier - if partner.reg_ids: - primary_id = partner.reg_ids[0] + # Get primary identifier (a live one: removed IDs don't resolve) + primary_id = primary_registry_id(partner) + if primary_id: ref = f"{resource_type}/{primary_id.id_type_id.uri}|{primary_id.value}" else: # No identifier - this should not happen in a properly configured system @@ -379,23 +401,21 @@ def _parse_beneficiary_reference(self, reference: str): system = unquote(system) - # Find partner by identifier - reg_id = ( - self.env["spp.registry.id"] # nosemgrep: odoo-sudo-without-context + return self.find_beneficiary(system, value, is_group=reference.startswith("Group/")) + + def _find_existing_membership(self, partner_id, program_id): + """Return the beneficiary's membership in the program, if any (the pair is unique)""" + if not partner_id or not program_id: + return None + # The API runs as the public user; the pre-check must see every row the + # UNIQUE constraint sees, including archived beneficiaries + membership = ( + self.env["spp.program.membership"] # nosemgrep: odoo-sudo-without-context .sudo() - .search( - [ - ("id_type_id.uri", "=", system), - ("value", "=", value), - ], - limit=1, - ) + .with_context(active_test=False) + .search([("partner_id", "=", partner_id), ("program_id", "=", program_id)], limit=1) ) - - if reg_id and reg_id.partner_id: - return reg_id.partner_id - - return self.env["res.partner"] + return membership or None def create(self, schema: ProgramMembership, source: str) -> Any: """ @@ -413,7 +433,23 @@ def create(self, schema: ProgramMembership, source: str) -> Any: # Add source tracking if the model supports it # Base model doesn't have source_system field, but extensions might add it - membership = self.env["spp.program.membership"].sudo().create(vals) # nosemgrep: odoo-sudo-without-context + if self._find_existing_membership(vals.get("partner_id"), vals.get("program_id")): + raise DuplicateMembershipError() + + # The UNIQUE(partner_id, program_id) constraint still catches a concurrent + # create; the savepoint keeps the request's transaction usable after it + try: + with self.env.cr.savepoint(): + # Writes as the public API user; access is checked by the API scopes + # nosemgrep: odoo-sudo-without-context + membership = self.env["spp.program.membership"].sudo().create(vals) + except UniqueViolation as e: + # Only the membership's own constraint means "already a member"; + # a violation elsewhere (another table written during create) is + # an unexpected error + if e.diag.constraint_name != MEMBERSHIP_UNIQUE_CONSTRAINT: + raise + raise DuplicateMembershipError() from e # Log using beneficiary identifier, not database ID partner_id = membership.partner_id.reg_ids[0] if membership.partner_id.reg_ids else None @@ -432,9 +468,27 @@ def update(self, membership, schema: ProgramMembership, source: str) -> Any: Returns: Updated spp.program.membership record + + Raises: + ValidationError: the body names a different program or + beneficiary than the membership being updated """ vals = self.from_api_schema(schema) + # A membership's program and beneficiary are its identity. A PUT + # naming others must not move the record to another program or + # registrant; create a new membership instead. + if vals.pop("program_id") != membership.program_id.id: + raise ValidationError( + f"Program {schema.program.reference} does not match the membership being updated; " + "a membership cannot be moved to another program" + ) + if vals.pop("partner_id") != membership.partner_id.id: + raise ValidationError( + "Beneficiary does not match the membership being updated; " + "a membership cannot be moved to another beneficiary" + ) + # Update membership membership.sudo().write(vals) # nosemgrep: odoo-sudo-without-context diff --git a/spp_api_v2_programs/static/description/index.html b/spp_api_v2_programs/static/description/index.html index 27b961dcd..04ce2229c 100644 --- a/spp_api_v2_programs/static/description/index.html +++ b/spp_api_v2_programs/static/description/index.html @@ -414,6 +414,78 @@

Changelog

+

19.0.1.1.0

+
    +
  • fix: GET/PUT /ProgramMembership/{identifier} no longer resolve +to an arbitrary program’s membership when the beneficiary is enrolled +in several programs (#554). A new optional program query parameter +(Program/{system}|{value}) selects the membership (a malformed +value returns 400, an unknown program 404); without it, a +beneficiary with more than one membership returns 409. When no +single membership resolves, a client that requires consent and may not +read the beneficiary gets 403 whatever the reason (unknown +beneficiary, not enrolled in that program, several memberships), so +neither the registry’s contents nor enrollments are revealed; the +409 message does not give the number of memberships.
    • +
  • fix: PUT /ProgramMembership can no longer move a membership to +another program or beneficiary: a body whose program or +beneficiary differs from the addressed membership returns 422 +(#554).
    • +
  • fix: the Location header of POST /ProgramMembership is +URL-encoded and carries ?program=, so following it returns the +created membership (#554).
    • +
  • fix: PUT /ProgramMembership accepts its own +ETag/meta.versionId in If-Match; it previously compared +against a different timestamp format and always returned 409 +(#554).
    • +
  • fix: a beneficiary identifier held by more than one registrant is +refused on GET/PUT/POST /ProgramMembership (409, or +403 for a client that requires consent and may not read every +match) and in the beneficiary search filter (409, or an empty +page for such a client), instead of acting on an arbitrary one; +Individual/ and Group/ beneficiary references resolve only +that kind. Membership identifiers, beneficiary references and +Location use only live (not soft-removed) IDs (#554).
    • +
  • fix: GET /ProgramMembership filters fail closed (#554). A +beneficiary that isn’t Individual/{system}|{value} or +Group/{system}|{value}, or a program that isn’t +Program/{system}|{value}, returns 400 instead of being dropped +and returning every membership. A well-formed reference that matches +nothing still returns an empty page.
    • +
  • fix: POST /ProgramMembership for a beneficiary already in the +program returns 409 “Beneficiary is already a member of this +program” (#554). It returned 422 with the database error text, +including internal record ids. Create and PUT errors that aren’t +client errors now return a generic message; the details are only +logged.
    • +
  • fix: GET /Program without the program:search or +program:read scope returns 403 (#554). The status query +parameter shadowed FastAPI’s status module, so it returned +500.
    • +
  • fix: consent-filtered GET /ProgramMembership pages no longer skip +records or stop early, and meta.total never counts memberships the +client may not see (#554, see spp_api_v2).
    • +
  • fix: GET /ProgramMembership self/next/prev links are +URL-encoded (#554). The # in a beneficiary’s identifier type cut +the link short, so following next lost the beneficiary filter.
    • +
  • fix: an unexpected error in GET /ProgramMembership returns 500 +“Failed to search program memberships” instead of 400 with the +error text (#554); malformed filters are still 400. An _offset +beyond what the database accepts returns 422.
    • +
  • fix(security): whether a client gets the jittered 403 for an +unknown beneficiary follows its legal basis, the rule consent +filtering uses, not its is_require_consent box (#554 review, see +spp_api_v2).
    • +
  • fix: memberships of an archived beneficiary can be read and updated +with GET/PUT /ProgramMembership (#554 review). The lookup +dropped them, while POST answered 409 “already a member” for +the same membership.
    • +
  • fix: the beneficiary and program search filters parse +references as POST/PUT bodies do (#554 review), so a +percent-encoded system (...%23national_id) matches on GET too.
    • +
+
+

19.0.1.0.0

  • Split program and program-membership REST endpoints out of diff --git a/spp_api_v2_programs/tests/__init__.py b/spp_api_v2_programs/tests/__init__.py index c8d2b6f67..5a7131b0a 100644 --- a/spp_api_v2_programs/tests/__init__.py +++ b/spp_api_v2_programs/tests/__init__.py @@ -4,4 +4,5 @@ from . import test_program_membership_api from . import test_program_service from . import test_program_membership_service +from . import test_program_membership_identity from . import test_scope_enforcement_program diff --git a/spp_api_v2_programs/tests/test_program_api.py b/spp_api_v2_programs/tests/test_program_api.py index 98300f7c6..51200a51e 100644 --- a/spp_api_v2_programs/tests/test_program_api.py +++ b/spp_api_v2_programs/tests/test_program_api.py @@ -150,6 +150,21 @@ def test_search_by_status_ended(self): data = json.loads(response.content) self.assertGreater(data["meta"]["total"], 0) + def test_search_status_filter_selects_exactly_one_state(self): + """?status=active and ?status=ended each return only the program in that state""" + self.create_test_program(name="Status Filter Probe Running", state="active") + self.create_test_program(name="Status Filter Probe Closed", state="ended", active=False) + + def names(status): + response = self.url_open( + f"{self.api_base_url}?name=Status+Filter+Probe&status={status}", headers=self._get_headers() + ) + self.assertEqual(response.status_code, 200, response.content) + return [program["name"] for program in json.loads(response.content)["data"]] + + self.assertEqual(names("active"), ["Status Filter Probe Running"]) + self.assertEqual(names("ended"), ["Status Filter Probe Closed"]) + def test_search_by_target_type(self): """Search by targetType filters results""" # Create individual-targeted program diff --git a/spp_api_v2_programs/tests/test_program_membership_api.py b/spp_api_v2_programs/tests/test_program_membership_api.py index f06c3231a..8e2469163 100644 --- a/spp_api_v2_programs/tests/test_program_membership_api.py +++ b/spp_api_v2_programs/tests/test_program_membership_api.py @@ -3,9 +3,16 @@ import json from datetime import date +from unittest.mock import patch + +from odoo.tools import mute_logger from odoo.addons.spp_api_v2.tests.common import ApiV2HttpTestCase +from ..services.program_membership_service import ProgramMembershipService + +NATIONAL_ID = "urn:openspp:vocab:id-type#test_national_id" + class TestProgramMembershipAPIEndpoints(ApiV2HttpTestCase): """Test ProgramMembership resource HTTP endpoints""" @@ -60,6 +67,11 @@ def _get_headers(self, token=None): "Authorization": f"Bearer {token or self.token}", } + def _get_rejected(self, url): + """GET a request the API answers with an error, without the error log it writes""" + with mute_logger("odoo.http", "odoo.addons.spp_api_v2_programs.routers.program_membership"): + return self.url_open(url, headers=self._get_headers()) + def test_read_program_membership_success(self): """GET /ProgramMembership/{id} returns membership""" url = f"{self.api_base_url}/urn:openspp:vocab:id-type%23test_national_id|ENROLL-001" @@ -74,11 +86,28 @@ def test_read_program_membership_success(self): self.assertIn("program", data) self.assertIn("beneficiary", data) + def _no_consent_required_token(self): + """Token for a client with a legal basis (no consent needed), to test 404 behaviour + + Consent-requiring clients get 403 for an unknown beneficiary to prevent + enumeration, as on the Individual endpoints. + """ + client = self.create_api_client( + name="No Consent Required Client", + scopes=[ + {"resource": "program_membership", "action": "read"}, + {"resource": "program_membership", "action": "update"}, + ], + require_consent=False, + legal_basis="public_interest", + ) + return self.generate_jwt_token(client) + def test_read_program_membership_not_found(self): - """GET with non-existent ID returns 404""" + """GET with non-existent ID returns 404 (for non-consent-requiring clients)""" url = f"{self.api_base_url}/urn:openspp:vocab:id-type%23test_national_id|NONEXISTENT" - response = self.url_open(url, headers=self._get_headers()) + response = self.url_open(url, headers=self._get_headers(token=self._no_consent_required_token())) self.assertEqual(response.status_code, 404) @@ -336,8 +365,9 @@ def test_update_program_membership_wrong_if_match_returns_409(self): self.assertEqual(response.status_code, 409) def test_update_program_membership_not_found_returns_404(self): - """PUT to a non-existent membership returns 404""" + """PUT to a non-existent membership returns 404 (for non-consent-requiring clients)""" url = f"{self.api_base_url}/urn:openspp:vocab:id-type%23test_national_id|NONEXISTENT-PUT" + token = self._no_consent_required_token() payload = { "type": "ProgramMembership", @@ -348,7 +378,7 @@ def test_update_program_membership_not_found_returns_404(self): "status": "paused", } - response = self.url_put(url, data=json.dumps(payload), headers=self._get_headers()) + response = self.url_put(url, data=json.dumps(payload), headers=self._get_headers(token=token)) self.assertEqual(response.status_code, 404) def test_update_program_membership_no_scope(self): @@ -476,22 +506,51 @@ def test_consent_filtering_applied(self): self.assertEqual(response.status_code, 403) def test_search_with_invalid_beneficiary_format(self): - """Search with unrecognized beneficiary format ignores the filter""" + """Search with unrecognized beneficiary format returns 400 instead of every membership""" url = f"{self.api_base_url}?beneficiary=InvalidFormat" - response = self.url_open(url, headers=self._get_headers()) + response = self._get_rejected(url) - # Unrecognized format (not starting with Individual/ or Group/) is ignored - self.assertEqual(response.status_code, 200) + # Unrecognized format (not starting with Individual/ or Group/) is rejected + self.assertEqual(response.status_code, 400) + self.assertIn("Invalid beneficiary", json.loads(response.content)["detail"]) def test_search_with_invalid_program_format(self): - """Search with unrecognized program format ignores the filter""" + """Search with unrecognized program format returns 400 instead of every membership""" url = f"{self.api_base_url}?program=InvalidFormat" + response = self._get_rejected(url) + + # Unrecognized format (not starting with Program/) is rejected + self.assertEqual(response.status_code, 400) + self.assertIn("Invalid program", json.loads(response.content)["detail"]) + + def test_search_with_beneficiary_missing_value_separator(self): + """A beneficiary reference without system|value returns 400""" + url = f"{self.api_base_url}?beneficiary=Individual/ENROLL-001" + + response = self._get_rejected(url) + + self.assertEqual(response.status_code, 400) + self.assertIn("Invalid beneficiary", json.loads(response.content)["detail"]) + + def test_search_with_program_missing_value_separator(self): + """A program reference without system|value returns 400""" + url = f"{self.api_base_url}?program=Program/test-enrollment-program" + + response = self._get_rejected(url) + + self.assertEqual(response.status_code, 400) + self.assertIn("Invalid program", json.loads(response.content)["detail"]) + + def test_search_with_unknown_beneficiary_is_empty(self): + """A well-formed reference that matches nobody still returns an empty page""" + url = f"{self.api_base_url}?beneficiary=Individual/urn:openspp:vocab:id-type%23test_national_id|NOBODY" + response = self.url_open(url, headers=self._get_headers()) - # Unrecognized format (not starting with Program/) is ignored self.assertEqual(response.status_code, 200) + self.assertEqual(json.loads(response.content)["data"], []) def test_search_combined_filters(self): """Search supports combining multiple filters""" @@ -564,3 +623,227 @@ def test_no_token_returns_401(self): response = self.url_open(self.api_base_url, headers={"Content-Type": "application/json"}) self.assertEqual(response.status_code, 401) + + def _duplicate_payload(self): + return { + "type": "ProgramMembership", + "program": {"reference": "Program/urn:openspp:program|test-enrollment-program"}, + "beneficiary": {"reference": "Individual/urn:openspp:vocab:id-type#test_national_id|ENROLL-001"}, + "status": "enrolled", + } + + def _assert_no_database_internals(self, detail): + for leaked in ("duplicate key", "Key (", "partner_id", "program_id", "constraint"): + self.assertNotIn(leaked, detail) + + def test_create_duplicate_membership_returns_409(self): + """POST for a beneficiary already in the program returns 409 without database internals""" + with mute_logger("odoo.http"): + response = self.url_open( + self.api_base_url, + data=json.dumps(self._duplicate_payload()), + headers=self._get_headers(), + ) + + self.assertEqual(response.status_code, 409) + detail = json.loads(response.content)["detail"] + self.assertIn("already a member", detail) + self._assert_no_database_internals(detail) + + def test_create_duplicate_membership_race_returns_409(self): + """When the pre-check misses (concurrent create), the database constraint still answers 409""" + with ( + patch.object(ProgramMembershipService, "_find_existing_membership", return_value=None) as pre_check, + mute_logger("odoo.sql_db", "odoo.http", "odoo.addons.spp_api_v2_programs.routers.program_membership"), + ): + response = self.url_open( + self.api_base_url, + data=json.dumps(self._duplicate_payload()), + headers=self._get_headers(), + ) + + pre_check.assert_called_once() + self.assertEqual(response.status_code, 409) + detail = json.loads(response.content)["detail"] + self.assertIn("already a member", detail) + self._assert_no_database_internals(detail) + # The transaction is still usable after the violation, and no second row was written + self.env.invalidate_all() + self.assertEqual( + self.env["spp.program.membership"].search_count( + [("partner_id", "=", self.individual.id), ("program_id", "=", self.program.id)] + ), + 1, + ) + + def test_create_other_unique_violation_is_not_reported_as_duplicate(self): + """A unique violation on another table during create is a generic error, not 'already a member'""" + new_individual = self.create_test_individual(identifier_value="OTHER-UNIQUE-001") + payload = self._duplicate_payload() + payload["beneficiary"]["reference"] = "Individual/urn:openspp:vocab:id-type#test_national_id|OTHER-UNIQUE-001" + membership_model = type(self.env["spp.program.membership"]) + + def violate_another_constraint(*args, **kwargs): + # Duplicate an existing system parameter key: a real UniqueViolation on ir_config_parameter + self.env.cr.execute( + "INSERT INTO ir_config_parameter (key, value) SELECT key, value FROM ir_config_parameter LIMIT 1" + ) + + with ( + patch.object(membership_model, "create", side_effect=violate_another_constraint), + mute_logger("odoo.sql_db", "odoo.http", "odoo.addons.spp_api_v2_programs.routers.program_membership"), + ): + response = self.url_open(self.api_base_url, data=json.dumps(payload), headers=self._get_headers()) + + self.assertEqual(response.status_code, 422) + self.assertEqual(json.loads(response.content)["detail"], "Failed to create program membership") + self.env.invalidate_all() + self.assertFalse(self.env["spp.program.membership"].search([("partner_id", "=", new_individual.id)])) + + def test_create_unexpected_error_hides_exception_text(self): + """An unexpected create error returns a generic 422 and is only logged""" + self.create_test_individual(identifier_value="HIDE-ERR-001") + payload = self._duplicate_payload() + payload["beneficiary"]["reference"] = "Individual/urn:openspp:vocab:id-type#test_national_id|HIDE-ERR-001" + + with ( + patch.object(ProgramMembershipService, "create", side_effect=RuntimeError("internal detail 4242")), + mute_logger("odoo.http"), + self.assertLogs("odoo.addons.spp_api_v2_programs.routers.program_membership", "ERROR") as logs, + ): + response = self.url_open(self.api_base_url, data=json.dumps(payload), headers=self._get_headers()) + + self.assertEqual(response.status_code, 422) + self.assertEqual(json.loads(response.content)["detail"], "Failed to create program membership") + self.assertIn("internal detail 4242", "\n".join(logs.output)) + + def test_update_unexpected_error_hides_exception_text(self): + """An unexpected PUT error returns a generic 422 and is only logged""" + url = f"{self.api_base_url}/urn:openspp:vocab:id-type%23test_national_id|ENROLL-001" + payload = json.loads(self.url_open(url, headers=self._get_headers()).content) + payload["status"] = "paused" + + with ( + patch.object(ProgramMembershipService, "update", side_effect=RuntimeError("internal detail 4343")), + mute_logger("odoo.http"), + self.assertLogs("odoo.addons.spp_api_v2_programs.routers.program_membership", "ERROR") as logs, + ): + response = self.url_put(url, data=json.dumps(payload), headers=self._get_headers()) + + self.assertEqual(response.status_code, 422) + self.assertEqual(json.loads(response.content)["detail"], "Failed to update program membership") + self.assertIn("internal detail 4343", "\n".join(logs.output)) + + def test_search_unexpected_error_hides_exception_text(self): + """An unexpected search error returns a generic error and is only logged""" + with ( + patch.object(ProgramMembershipService, "search", side_effect=RuntimeError("internal detail 4545")), + mute_logger("odoo.http"), + self.assertLogs("odoo.addons.spp_api_v2_programs.routers.program_membership", "ERROR") as logs, + ): + response = self.url_open(self.api_base_url, headers=self._get_headers()) + + self.assertEqual(response.status_code, 500) + self.assertEqual(json.loads(response.content)["detail"], "Failed to search program memberships") + self.assertIn("internal detail 4545", "\n".join(logs.output)) + + def test_search_huge_offset_is_rejected(self): + """An _offset beyond what the database accepts is a validation error, not a database error text""" + response = self._get_rejected(f"{self.api_base_url}?_offset=99999999999999999999") + + self.assertEqual(response.status_code, 422) + self.assertNotIn("bigint", response.text) + self.assertNotIn("LINE", response.text) + + +class TestProgramMembershipPagingAPI(ApiV2HttpTestCase): + """GET /ProgramMembership next links: followable, consent-aware, and no leaked total""" + + PROGRAM_REF = "Program/urn:openspp:program|paging-program" + + def setUp(self): + super().setUp() + self.program = self.create_test_program(name="Paging Program", target_type="individual") + self.other_program = self.create_test_program(name="Other Paging Program", target_type="individual") + self.client = self.create_api_client( + name="Membership Paging Client", + scopes=[ + {"resource": "program_membership", "action": "read"}, + {"resource": "program_membership", "action": "search"}, + ], + ) + self.token = self.generate_jwt_token(self.client) + + def _consent(self, partner): + self.create_consent( + registrant=partner, + grantee_partner=self.client.partner_id, + resource_type="all", + field_access="all", + ) + + def _enroll(self, value, program, consented=True): + partner = self.env["res.partner"].search([("reg_ids.value", "=", value)], limit=1) + if not partner: + partner = self.create_test_individual(identifier_value=value) + if consented: + self._consent(partner) + self.create_test_membership(partner=partner, program=program, state="enrolled") + return partner + + def _get(self, url): + response = self.url_open(url, headers={"Authorization": f"Bearer {self.token}"}) + self.assertEqual(response.status_code, 200, response.content) + return json.loads(response.content) + + def test_next_link_keeps_the_beneficiary_filter(self): + """The '#' in the beneficiary's identifier type is encoded, so following next stays on that beneficiary""" + self._enroll("PAGE-TWO-PROGRAMS", self.program) + self._enroll("PAGE-TWO-PROGRAMS", self.other_program) + self._enroll("PAGE-SOMEONE-ELSE", self.program) + + first = self._get( + f"/api/v2/spp/ProgramMembership?beneficiary=Individual/{NATIONAL_ID.replace('#', '%23')}|PAGE-TWO-PROGRAMS" + "&_count=1" + ) + self.assertIn("%23", first["links"]["self"]) + second = self._get(first["links"]["next"]) + + references = {m["beneficiary"]["reference"] for m in first["data"] + second["data"]} + programs = {m["program"]["reference"] for m in first["data"] + second["data"]} + self.assertEqual(references, {f"Individual/{NATIONAL_ID}|PAGE-TWO-PROGRAMS"}) + self.assertEqual(len(programs), 2) + + def test_consent_filtered_pages_cover_every_visible_membership_once(self): + """Following next visits each consented member exactly once and then stops""" + visible = set() + for number in range(1, 7): + value = f"PAGE-MEMBER-{number}" + self._enroll(value, self.program, consented=number not in (2, 5)) + if number not in (2, 5): + visible.add(f"Individual/{NATIONAL_ID}|{value}") + + seen = [] + url = f"/api/v2/spp/ProgramMembership?program={self.PROGRAM_REF.replace('|', '%7C')}&_count=2" + for _page in range(10): + body = self._get(url) + seen.extend(m["beneficiary"]["reference"] for m in body["data"]) + url = body["links"].get("next") + if not url: + break + + self.assertIsNone(url) + self.assertEqual(sorted(seen), sorted(visible)) + + def test_hidden_beneficiary_past_the_end_is_not_counted(self): + """beneficiary= of someone without consent, _offset=1: no data, total 0, no next""" + self._enroll("PAGE-HIDDEN", self.program, consented=False) + + body = self._get( + f"/api/v2/spp/ProgramMembership?beneficiary=Individual/{NATIONAL_ID.replace('#', '%23')}|PAGE-HIDDEN" + "&_count=1&_offset=1" + ) + + self.assertEqual(body["data"], []) + self.assertEqual(body["meta"]["total"], 0) + self.assertIsNone(body["links"].get("next")) diff --git a/spp_api_v2_programs/tests/test_program_membership_identity.py b/spp_api_v2_programs/tests/test_program_membership_identity.py new file mode 100644 index 000000000..ef2c64962 --- /dev/null +++ b/spp_api_v2_programs/tests/test_program_membership_identity.py @@ -0,0 +1,622 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Tests that a ProgramMembership is addressed unambiguously (#554 items A and B). + +A beneficiary can be enrolled in several programs, so the beneficiary's +identifier alone does not identify one membership. Reads and updates must +either be told the program (``?program=``) or refuse to guess. +""" + +import json +from unittest.mock import patch +from urllib.parse import quote + +from odoo.exceptions import ValidationError + +from odoo.addons.spp_api_v2.schemas.base import Reference +from odoo.addons.spp_api_v2.tests.common import ApiV2HttpTestCase, ApiV2TestCase + +from ..schemas.program_membership import ProgramMembership +from ..services import program_membership_service +from ..services.program_membership_service import ProgramMembershipService + +NATIONAL_ID = "urn:openspp:vocab:id-type#test_national_id" +PROGRAM_1_REF = "Program/urn:openspp:program|first-identity-program" +PROGRAM_2_REF = "Program/urn:openspp:program|second-identity-program" +PROGRAM_3_REF = "Program/urn:openspp:program|third-identity-program" + + +def _beneficiary_ref(value): + return f"Individual/{NATIONAL_ID}|{value}" + + +class TestProgramMembershipIdentityAPI(ApiV2HttpTestCase): + """HTTP contract for addressing one membership among several""" + + def setUp(self): + super().setUp() + self.api_base_url = "/api/v2/spp/ProgramMembership" + + self.program_1 = self.create_test_program(name="First Identity Program") + self.program_2 = self.create_test_program(name="Second Identity Program") + self.program_3 = self.create_test_program(name="Third Identity Program") + + # Enrolled in two programs. P1 is created first, so it is the *older* + # membership: a lookup that ignores the program picks P2 (newest first). + self.multi = self.create_test_individual(identifier_value="MULTI-001") + self.multi_p1 = self.create_test_membership(partner=self.multi, program=self.program_1, state="enrolled") + self.multi_p2 = self.create_test_membership(partner=self.multi, program=self.program_2, state="enrolled") + + # Enrolled in exactly one program. + self.single = self.create_test_individual(identifier_value="SINGLE-001") + self.single_p1 = self.create_test_membership(partner=self.single, program=self.program_1, state="enrolled") + + # Not enrolled anywhere. + self.other = self.create_test_individual(identifier_value="OTHER-001") + + self.client = self.create_api_client( + name="Membership Identity Client", + scopes=[ + {"resource": "program_membership", "action": "read"}, + {"resource": "program_membership", "action": "search"}, + {"resource": "program_membership", "action": "create"}, + {"resource": "program_membership", "action": "update"}, + ], + ) + for registrant in (self.multi, self.single, self.other): + self.create_consent( + registrant=registrant, + grantee_partner=self.client.partner_id, + resource_type="all", + field_access="all", + ) + self.token = self.generate_jwt_token(self.client) + + # ------------------------------------------------------------------ + # helpers + # ------------------------------------------------------------------ + + def _headers(self, **extra): + headers = { + "Content-Type": "application/json", + "Authorization": f"Bearer {self.token}", + } + headers.update(extra) + return headers + + def _url(self, value, program_ref=None): + url = f"{self.api_base_url}/{quote(NATIONAL_ID, safe=':')}|{value}" + if program_ref: + url += f"?program={quote(program_ref, safe=':/|')}" + return url + + def _put(self, url, payload, **extra_headers): + # url_put bypasses url_open's flush, so push setUp data to the + # database the HTTP worker reads, and drop our cache for the reads after. + self.env.cr.flush() + self.env.invalidate_all() + response = self.url_put(url, data=json.dumps(payload), headers=self._headers(**extra_headers)) + self.env.invalidate_all() + return response + + def _payload(self, program_ref, beneficiary_value, status): + return { + "type": "ProgramMembership", + "program": {"reference": program_ref}, + "beneficiary": {"reference": _beneficiary_ref(beneficiary_value)}, + "status": status, + } + + # ------------------------------------------------------------------ + # GET + # ------------------------------------------------------------------ + + def test_get_without_program_is_409_when_beneficiary_has_several_memberships(self): + """Refuses to pick one of several memberships instead of returning an arbitrary one""" + response = self.url_open(self._url("MULTI-001"), headers=self._headers()) + + self.assertEqual(response.status_code, 409, response.text) + self.assertIn("program", response.json()["detail"]) + # The refusal does not disclose how many programs the beneficiary is in + self.assertNotIn("2", response.json()["detail"]) + + def _no_consent_headers(self): + """A client that requires consent and has none for any beneficiary here""" + client = self.create_api_client( + name="No Consent Membership Client", + scopes=[ + {"resource": "program_membership", "action": "read"}, + {"resource": "program_membership", "action": "update"}, + ], + ) + return {"Content-Type": "application/json", "Authorization": f"Bearer {self.generate_jwt_token(client)}"} + + def test_unknown_beneficiary_is_403_for_a_consent_client(self): + """Same answer as "exists, no consent": the registry's contents are not revealed""" + response = self.url_open(self._url("NO-SUCH-BENEFICIARY"), headers=self._no_consent_headers()) + + self.assertEqual(response.status_code, 403, response.text) + + def test_consent_basis_client_with_require_consent_unticked_gets_403_for_both(self): + """Consent filtering keys off the legal basis, so the not-found answer does too""" + client = self.create_api_client( + name="Consent Basis Membership Client", + scopes=[{"resource": "program_membership", "action": "read"}], + require_consent=False, + legal_basis="consent", + ) + headers = {"Authorization": f"Bearer {self.generate_jwt_token(client)}"} + + unknown = self.url_open(self._url("NO-SUCH-BENEFICIARY"), headers=headers) + unconsented = self.url_open(self._url("SINGLE-001"), headers=headers) + + self.assertEqual(unknown.status_code, 403, unknown.text) + self.assertEqual(unconsented.status_code, 403, unconsented.text) + + def test_search_resolves_a_percent_encoded_system_like_post_does(self): + """``?beneficiary=`` and ``?program=`` parse references as POST/PUT bodies do""" + encoded_beneficiary = _beneficiary_ref("SINGLE-001").replace("#", "%23") + encoded_program = PROGRAM_1_REF.replace(":program", "%3Aprogram") + + response = self.url_open( + f"{self.api_base_url}?beneficiary={quote(encoded_beneficiary)}&program={quote(encoded_program)}", + headers=self._headers(), + ) + + self.assertEqual(response.status_code, 200, response.text) + self.assertEqual(response.json()["meta"]["total"], 1) + + def test_search_with_malformed_filters_is_400_with_the_expected_format(self): + for param, value, expected in ( + ("beneficiary", "Nobody/x|y", "Individual/{system}|{value}"), + ("beneficiary", "Individual/no-separator", "Individual/{system}|{value}"), + ("program", "Programme/x|y", "Program/{system}|{value}"), + ("program", "Program/no-separator", "Program/{system}|{value}"), + ): + with self.subTest(param=param, value=value): + response = self.url_open(f"{self.api_base_url}?{param}={quote(value)}", headers=self._headers()) + + self.assertEqual(response.status_code, 400, response.text) + self.assertIn(expected, response.json()["detail"]) + + def test_put_without_program_and_without_consent_is_403(self): + """PUT does not reveal that the beneficiary has several memberships""" + headers = self._no_consent_headers() + self.env.cr.flush() + response = self.url_put( + self._url("MULTI-001"), + data=json.dumps(self._payload(PROGRAM_1_REF, "MULTI-001", "exited")), + headers=headers, + ) + self.env.invalidate_all() + + self.assertEqual(response.status_code, 403, response.text) + self.assertEqual(self.multi_p1.state, "enrolled") + + def test_get_with_program_returns_that_programs_membership(self): + """?program= selects the membership in that program, not the newest one""" + response = self.url_open(self._url("MULTI-001", PROGRAM_1_REF), headers=self._headers()) + + self.assertEqual(response.status_code, 200, response.text) + self.assertEqual(response.json()["program"]["reference"], PROGRAM_1_REF) + + def test_get_without_program_still_works_for_single_membership(self): + """Backward compatible: one membership needs no ?program=""" + response = self.url_open(self._url("SINGLE-001"), headers=self._headers()) + + self.assertEqual(response.status_code, 200, response.text) + self.assertEqual(response.json()["program"]["reference"], PROGRAM_1_REF) + + def test_get_with_program_the_beneficiary_is_not_in_is_404(self): + """?program= naming a program without a membership for this beneficiary is 404""" + response = self.url_open(self._url("MULTI-001", PROGRAM_3_REF), headers=self._headers()) + + self.assertEqual(response.status_code, 404, response.text) + + def test_get_with_unknown_program_is_404(self): + """?program= naming no existing program is 404""" + response = self.url_open( + self._url("MULTI-001", "Program/urn:openspp:program|no-such-program"), + headers=self._headers(), + ) + + self.assertEqual(response.status_code, 404, response.text) + + def test_get_with_malformed_program_is_400(self): + """?program= that is not a Program/{system}|{value} reference is 400""" + response = self.url_open(self._url("MULTI-001", "not-a-reference"), headers=self._headers()) + + self.assertEqual(response.status_code, 400, response.text) + + def test_ambiguity_is_not_revealed_without_consent(self): + """Consent is checked before ambiguity: no 409 leaks membership counts""" + no_consent = self.create_test_individual(identifier_value="NOCONSENT-MULTI-001") + self.create_test_membership(partner=no_consent, program=self.program_1) + self.create_test_membership(partner=no_consent, program=self.program_2) + + response = self.url_open(self._url("NOCONSENT-MULTI-001"), headers=self._headers()) + + self.assertEqual(response.status_code, 403, response.text) + + def test_enrollment_is_not_revealed_without_consent(self): + """?program= for a program the beneficiary is not in is 403, not 404, without consent""" + no_consent = self.create_test_individual(identifier_value="NOCONSENT-SINGLE-001") + self.create_test_membership(partner=no_consent, program=self.program_1) + + response = self.url_open(self._url("NOCONSENT-SINGLE-001", PROGRAM_3_REF), headers=self._headers()) + + self.assertEqual(response.status_code, 403, response.text) + + # ------------------------------------------------------------------ + # PUT + # ------------------------------------------------------------------ + + def test_put_with_program_updates_only_that_programs_membership(self): + """PUT ?program=P1 exits P1 and leaves P2 alone (#554 A reproduction)""" + response = self._put( + self._url("MULTI-001", PROGRAM_1_REF), + self._payload(PROGRAM_1_REF, "MULTI-001", "exited"), + ) + + self.assertEqual(response.status_code, 200, response.text) + self.assertEqual(response.json()["program"]["reference"], PROGRAM_1_REF) + self.assertEqual(self.multi_p1.state, "exited") + self.assertEqual(self.multi_p2.state, "enrolled") + self.assertEqual(self.multi_p1.program_id, self.program_1) + self.assertEqual(self.multi_p2.program_id, self.program_2) + + def test_put_without_program_is_409_when_beneficiary_has_several_memberships(self): + """PUT refuses to guess, and changes nothing""" + response = self._put( + self._url("MULTI-001"), + self._payload(PROGRAM_1_REF, "MULTI-001", "exited"), + ) + + self.assertEqual(response.status_code, 409, response.text) + self.assertEqual(self.multi_p1.state, "enrolled") + self.assertEqual(self.multi_p2.state, "enrolled") + + def test_put_cannot_move_membership_to_another_program(self): + """Body program different from the addressed membership is 422, not a re-parent""" + response = self._put( + self._url("SINGLE-001"), + self._payload(PROGRAM_3_REF, "SINGLE-001", "enrolled"), + ) + + self.assertEqual(response.status_code, 422, response.text) + self.assertEqual(self.single_p1.program_id, self.program_1) + self.assertFalse( + self.env["spp.program.membership"].search( + [("partner_id", "=", self.single.id), ("program_id", "=", self.program_3.id)] + ) + ) + + def test_put_body_program_must_match_query_program(self): + """?program=P1 with a body naming P2 is 422 and changes nothing""" + response = self._put( + self._url("MULTI-001", PROGRAM_1_REF), + self._payload(PROGRAM_2_REF, "MULTI-001", "exited"), + ) + + self.assertEqual(response.status_code, 422, response.text) + self.assertEqual(self.multi_p1.state, "enrolled") + self.assertEqual(self.multi_p2.state, "enrolled") + + def test_put_cannot_move_membership_to_another_beneficiary(self): + """Body beneficiary different from the addressed one is 422, not a reassignment""" + response = self._put( + self._url("SINGLE-001"), + self._payload(PROGRAM_1_REF, "OTHER-001", "enrolled"), + ) + + self.assertEqual(response.status_code, 422, response.text) + self.assertEqual(self.single_p1.partner_id, self.single) + self.assertFalse(self.env["spp.program.membership"].search([("partner_id", "=", self.other.id)])) + + # ------------------------------------------------------------------ + # POST Location + # ------------------------------------------------------------------ + + def test_post_location_addresses_the_created_membership(self): + """Following Location returns the membership just created, even with others present""" + response = self.url_open( + self.api_base_url, + data=json.dumps(self._payload(PROGRAM_3_REF, "MULTI-001", "enrolled")), + headers=self._headers(), + ) + self.assertEqual(response.status_code, 201, response.text) + location = response.headers["location"] + + followed = self.url_open(location, headers=self._headers()) + + self.assertEqual(followed.status_code, 200, followed.text) + self.assertEqual(followed.json()["program"]["reference"], PROGRAM_3_REF) + + def test_post_location_skips_a_removed_beneficiary_id(self): + """A soft-removed ID no longer resolves, so Location must use a live one""" + self.env["spp.registry.id"].create( + { + "partner_id": self.other.id, + "id_type_id": self.id_type_household.id, + "value": "OTHER-REMOVED-ID", + "status": "invalid", + } + ) + response = self.url_open( + self.api_base_url, + data=json.dumps(self._payload(PROGRAM_1_REF, "OTHER-001", "enrolled")), + headers=self._headers(), + ) + self.assertEqual(response.status_code, 201, response.text) + + followed = self.url_open(response.headers["location"], headers=self._headers()) + + self.assertEqual(followed.status_code, 200, followed.text) + self.assertEqual(followed.json()["beneficiary"]["reference"], _beneficiary_ref("OTHER-001")) + + # ------------------------------------------------------------------ + # If-Match (#554 B) + # ------------------------------------------------------------------ + + def test_put_accepts_its_own_etag_as_if_match(self): + """The ETag from GET is accepted by PUT (versionId format matches)""" + url = self._url("SINGLE-001") + get_response = self.url_open(url, headers=self._headers()) + self.assertEqual(get_response.status_code, 200, get_response.text) + etag = get_response.headers["etag"] + self.assertEqual(etag, f'"{get_response.json()["meta"]["versionId"]}"') + + response = self._put(url, self._payload(PROGRAM_1_REF, "SINGLE-001", "paused"), **{"If-Match": etag}) + + self.assertEqual(response.status_code, 200, response.text) + self.assertEqual(self.single_p1.state, "paused") + + def test_put_rejects_a_stale_etag(self): + """An ETag from before a later write is refused (409), so the comparison is real""" + url = self._url("SINGLE-001") + stale_etag = self.url_open(url, headers=self._headers()).headers["etag"] + first = self._put(url, self._payload(PROGRAM_1_REF, "SINGLE-001", "paused"), **{"If-Match": stale_etag}) + self.assertEqual(first.status_code, 200, first.text) + + second = self._put(url, self._payload(PROGRAM_1_REF, "SINGLE-001", "exited"), **{"If-Match": stale_etag}) + + self.assertEqual(second.status_code, 409, second.text) + self.assertEqual(self.single_p1.state, "paused") + + +class TestProgramMembershipIdentityService(ApiV2TestCase): + """Service-level resolution and update guards""" + + def setUp(self): + super().setUp() + self.service = ProgramMembershipService(self.env) + self.program_1 = self.create_test_program(name="First Identity Program") + self.program_2 = self.create_test_program(name="Second Identity Program") + self.program_3 = self.create_test_program(name="Third Identity Program") + self.multi = self.create_test_individual(identifier_value="SVC-MULTI-001") + self.multi_p1 = self.create_test_membership(partner=self.multi, program=self.program_1) + self.multi_p2 = self.create_test_membership(partner=self.multi, program=self.program_2) + self.other = self.create_test_individual(identifier_value="SVC-OTHER-001") + + def _beneficiary(self): + return self.service.find_beneficiary(NATIONAL_ID, "SVC-MULTI-001") + + def test_find_for_beneficiary_with_program(self): + """The program narrows resolution to that program's membership""" + found = self.service.find_for_beneficiary(self._beneficiary(), program=self.program_1) + + self.assertEqual(found, self.multi_p1) + + def test_find_for_beneficiary_with_program_without_membership(self): + """A program the beneficiary is not in resolves to nothing""" + found = self.service.find_for_beneficiary(self._beneficiary(), program=self.program_3) + + self.assertFalse(found) + + def test_find_for_beneficiary_ambiguous_raises(self): + """Several memberships and no program: refuse""" + with self.assertRaises(program_membership_service.AmbiguousMembershipError): + self.service.find_for_beneficiary(self._beneficiary()) + + def test_ambiguity_is_refused_without_counting_memberships(self): + """Refusing needs the two rows the lookup already read, not a count of them all""" + Membership = type(self.env["spp.program.membership"]) + with patch.object(Membership, "search_count") as search_count: + with self.assertRaises(program_membership_service.AmbiguousMembershipError): + self.service.find_for_beneficiary(self.multi) + + search_count.assert_not_called() + + def test_version_id_is_the_schema_version(self): + """One formula for meta.versionId (the ETag) and the If-Match check""" + version = self.service.version_id(self.multi_p1) + + self.assertEqual(version, self.service.to_api_schema(self.multi_p1)["meta"]["versionId"]) + self.assertEqual(version, str(int(self.multi_p1.write_date.timestamp() * 1000000))) + + def _schema(self, program_ref, beneficiary_value, status="exited"): + return ProgramMembership( + program=Reference(reference=program_ref), + beneficiary=Reference(reference=_beneficiary_ref(beneficiary_value)), + status=status, + ) + + def test_update_refuses_program_change(self): + """update() never re-parents a membership onto another program""" + with self.assertRaises(ValidationError): + self.service.update(self.multi_p1, self._schema(PROGRAM_3_REF, "SVC-MULTI-001"), source="test") + + self.assertEqual(self.multi_p1.program_id, self.program_1) + self.assertEqual(self.multi_p1.state, "enrolled") + + def test_update_refuses_beneficiary_change(self): + """update() never reassigns a membership to another registrant""" + with self.assertRaises(ValidationError): + self.service.update(self.multi_p1, self._schema(PROGRAM_1_REF, "SVC-OTHER-001"), source="test") + + self.assertEqual(self.multi_p1.partner_id, self.multi) + self.assertEqual(self.multi_p1.state, "enrolled") + + def test_update_matching_identity_writes_status(self): + """A body matching the membership's program and beneficiary updates it""" + self.service.update(self.multi_p1, self._schema(PROGRAM_1_REF, "SVC-MULTI-001"), source="test") + + self.assertEqual(self.multi_p1.state, "exited") + self.assertEqual(self.multi_p2.state, "enrolled") + + +class TestProgramMembershipAmbiguousBeneficiary(ApiV2HttpTestCase): + """A beneficiary identifier shared by two registrants addresses nothing (#554 I)""" + + def setUp(self): + super().setUp() + self.api_base_url = "/api/v2/spp/ProgramMembership" + self.program = self.create_test_program(name="First Identity Program") + self.first = self.create_test_individual(name="Original", identifier_value="PM-DUP") + self.second = self.create_test_individual(name="Copy", identifier_value="PM-DUP") + self.membership = self.create_test_membership(partner=self.first, program=self.program) + self.client = self.create_api_client( + name="Legal Basis Membership Client", + scopes=[ + {"resource": "program_membership", "action": "read"}, + {"resource": "program_membership", "action": "create"}, + {"resource": "program_membership", "action": "update"}, + ], + require_consent=False, + legal_basis="public_task", + ) + self.token = self.generate_jwt_token(self.client) + + def _headers(self): + return {"Content-Type": "application/json", "Authorization": f"Bearer {self.token}"} + + def test_get_with_ambiguous_beneficiary_is_409(self): + response = self.url_open( + f"{self.api_base_url}/{quote(NATIONAL_ID, safe=':')}|PM-DUP", + headers=self._headers(), + ) + + self.assertEqual(response.status_code, 409, response.text) + + def test_post_with_ambiguous_beneficiary_is_409(self): + response = self.url_open( + self.api_base_url, + data=json.dumps( + { + "type": "ProgramMembership", + "program": {"reference": PROGRAM_1_REF}, + "beneficiary": {"reference": _beneficiary_ref("PM-DUP")}, + "status": "enrolled", + } + ), + headers=self._headers(), + ) + + self.assertEqual(response.status_code, 409, response.text) + self.env.invalidate_all() + self.assertFalse( + self.env["spp.program.membership"].search([("partner_id", "=", self.second.id)]), + ) + + def test_search_by_ambiguous_beneficiary_is_409(self): + response = self.url_open( + f"{self.api_base_url}?beneficiary={quote(_beneficiary_ref('PM-DUP'), safe=':/|')}", + headers=self._headers(), + ) + + self.assertEqual(response.status_code, 409, response.text) + + def test_put_with_ambiguous_body_beneficiary_is_409(self): + """The addressed membership resolves, but the body names an ambiguous beneficiary""" + single = self.create_test_individual(name="Single", identifier_value="PM-SINGLE") + membership = self.create_test_membership(partner=single, program=self.program) + self.env.cr.flush() + response = self.url_put( + f"{self.api_base_url}/{quote(NATIONAL_ID, safe=':')}|PM-SINGLE", + data=json.dumps( + { + "type": "ProgramMembership", + "program": {"reference": PROGRAM_1_REF}, + "beneficiary": {"reference": _beneficiary_ref("PM-DUP")}, + "status": "exited", + } + ), + headers=self._headers(), + ) + self.env.invalidate_all() + + self.assertEqual(response.status_code, 409, response.text) + self.assertEqual(membership.state, "enrolled") + + +class TestProgramMembershipArchivedBeneficiary(ApiV2HttpTestCase): + """An archived beneficiary keeps its memberships, and they stay addressable (D8)""" + + def setUp(self): + super().setUp() + self.api_base_url = "/api/v2/spp/ProgramMembership" + self.program = self.create_test_program(name="First Identity Program") + self.archived = self.create_test_individual(name="Archived", identifier_value="PM-ARCHIVED") + self.membership = self.create_test_membership(partner=self.archived, program=self.program, state="enrolled") + self.archived.active = False + self.client = self.create_api_client( + name="Legal Basis Membership Client", + scopes=[ + {"resource": "program_membership", "action": "read"}, + {"resource": "program_membership", "action": "create"}, + {"resource": "program_membership", "action": "update"}, + ], + require_consent=False, + legal_basis="public_task", + ) + self.token = self.generate_jwt_token(self.client) + + def _headers(self): + return {"Content-Type": "application/json", "Authorization": f"Bearer {self.token}"} + + def _url(self): + return ( + f"{self.api_base_url}/{quote(NATIONAL_ID, safe=':')}|PM-ARCHIVED?program={quote(PROGRAM_1_REF, safe=':/|')}" + ) + + def test_get_reads_the_membership(self): + response = self.url_open(self._url(), headers=self._headers()) + + self.assertEqual(response.status_code, 200, response.text) + self.assertEqual(response.json()["status"], "enrolled") + + def test_put_updates_the_membership(self): + self.env.cr.flush() + response = self.url_put( + self._url(), + data=json.dumps( + { + "type": "ProgramMembership", + "program": {"reference": PROGRAM_1_REF}, + "beneficiary": {"reference": _beneficiary_ref("PM-ARCHIVED")}, + "status": "exited", + } + ), + headers=self._headers(), + ) + self.env.invalidate_all() + + self.assertEqual(response.status_code, 200, response.text) + self.assertEqual(self.membership.state, "exited") + + def test_post_again_is_409_and_the_membership_is_still_readable(self): + """The duplicate check and the lookup agree: the membership exists""" + response = self.url_open( + self.api_base_url, + data=json.dumps( + { + "type": "ProgramMembership", + "program": {"reference": PROGRAM_1_REF}, + "beneficiary": {"reference": _beneficiary_ref("PM-ARCHIVED")}, + "status": "enrolled", + } + ), + headers=self._headers(), + ) + + self.assertEqual(response.status_code, 409, response.text) + self.assertEqual(self.url_open(self._url(), headers=self._headers()).status_code, 200) diff --git a/spp_api_v2_programs/tests/test_program_membership_service.py b/spp_api_v2_programs/tests/test_program_membership_service.py index a536b2f25..73f7edf80 100644 --- a/spp_api_v2_programs/tests/test_program_membership_service.py +++ b/spp_api_v2_programs/tests/test_program_membership_service.py @@ -24,18 +24,19 @@ def setUp(self): self.individual = self.create_test_individual(identifier_value="MEMBER-TEST-001") self.group = self.create_test_group(identifier_value="GROUP-TEST-001") - def test_find_by_identifier_uses_namespace_uri(self): + def test_find_beneficiary_then_membership_by_namespace_uri(self): """Lookup uses partner's namespace_uri""" membership = self.create_test_membership(partner=self.individual, program=self.program) # Find using partner's identifier - found = self.service.find_by_identifier("urn:openspp:vocab:id-type#test_national_id", "MEMBER-TEST-001") + partner = self.service.find_beneficiary("urn:openspp:vocab:id-type#test_national_id", "MEMBER-TEST-001") + found = self.service.find_for_beneficiary(partner) self.assertEqual(found, membership) - def test_find_by_identifier_not_found(self): + def test_find_beneficiary_not_found(self): """Returns empty recordset when not found""" - found = self.service.find_by_identifier("urn:openspp:vocab:id-type#test_national_id", "NONEXISTENT") + found = self.service.find_beneficiary("urn:openspp:vocab:id-type#test_national_id", "NONEXISTENT") self.assertFalse(found) diff --git a/spp_api_v2_programs/tests/test_scope_enforcement_program.py b/spp_api_v2_programs/tests/test_scope_enforcement_program.py index 0feced987..fb6b87847 100644 --- a/spp_api_v2_programs/tests/test_scope_enforcement_program.py +++ b/spp_api_v2_programs/tests/test_scope_enforcement_program.py @@ -6,6 +6,8 @@ import json +from odoo.tools import mute_logger + from odoo.addons.spp_api_v2.tests.common import ApiV2HttpTestCase @@ -138,3 +140,26 @@ def test_individual_scope_does_not_grant_program_membership_access(self): data = json.loads(response.content) self.assertIn("Missing required scope", data["detail"]) self.assertIn("program_membership:create", data["detail"]) + + +class TestScopeEnforcementProgram(ApiV2HttpTestCase): + """Test scope enforcement on the Program search endpoint""" + + def test_program_search_requires_scope(self): + """GET /Program returns 403, not 500, without program:search or program:read scope""" + client = self.create_api_client( + name="No program scope", + scopes=[{"resource": "individual", "action": "all"}], + require_consent=False, + ) + token = self.generate_jwt_token(client) + + with mute_logger("odoo.http"): + response = self.url_open( + "/api/v2/spp/Program", + headers={"Authorization": f"Bearer {token}"}, + ) + + self.assertEqual(response.status_code, 403) + data = json.loads(response.content) + self.assertIn("program:search", data["detail"])