From 15b5b79d0e6be458645521ebfc7819faf659a7c0 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Mon, 29 Jun 2026 22:37:18 +0800 Subject: [PATCH 1/3] security(dci): scope notification delivery per subscriber (consent + filter) The DCI notification pipeline built full DCI person/group records with no sender context and fanned the identical payload out to every subscription matched only on event/registry/active/expiry. Stored filter_expression was never applied, consent was never checked, and auto_approve removed the human gate -- so any active/auto-approved subscriber to a broad event received full PII (names, birthdate, address, phone, email, household, programmes) for every changed registrant, outside its requested filter, consent, or legal basis. Make delivery per-subscription and sender-scoped: - spp_dci_server: capture filter_type at subscribe time; add eligibility helpers on spp.dci.subscription -- _consent_allows_partner (correct primitive: has_legal_basis_bypass / can_access_registrant; NOT the dead status=="active" domain), _partner_matches_filter (base fail-closed), _eligible_partner_ids, _build_notification_records (hook), _delete_records_for. notify_event now takes (event_type, partner_ids, reg_type, delete_payloads) and, per matching subscription, delivers only consent-allowed + filter-matching records built with that subscription's sender context. - spp_dci_server_social: override _partner_matches_filter (compile the stored filter via the search service; drop on parse failure) and _build_notification_records (build with sender_id). Snapshot per-subscription eligibility at unlink so delete notifications go only to eligible subscribers; _execute_dci_notification hands off partner_ids / scoped delete payloads instead of a single global payload. Decisions: legal-basis-bypass senders pass consent (filter still applies), documented; delete scoped via unlink-time eligibility snapshot. Deferred to follow-ups: field minimization (response_fields / layer C), and the parallel dead consent filter on the search path (build_consented_domain status=="active"). Tests: spp_dci_server 320/320, spp_dci_server_social 82/82. --- spp_dci_server/models/subscription.py | 115 +++++++++++++++--- spp_dci_server/routers/async_router.py | 1 + spp_dci_server/tests/test_subscription.py | 18 ++- spp_dci_server_social/models/__init__.py | 1 + .../models/dci_subscription_social.py | 82 +++++++++++++ .../models/res_partner_dci_notify.py | 78 +++++------- .../tests/test_dci_notifications.py | 107 +++++++++++++++- 7 files changed, 327 insertions(+), 75 deletions(-) create mode 100644 spp_dci_server_social/models/dci_subscription_social.py diff --git a/spp_dci_server/models/subscription.py b/spp_dci_server/models/subscription.py index 57f128c56..e96f33e91 100644 --- a/spp_dci_server/models/subscription.py +++ b/spp_dci_server/models/subscription.py @@ -96,6 +96,14 @@ class DCISubscription(models.Model): string="Filter Expression", help="Optional DCI expression to filter which events trigger notifications", ) + filter_type = fields.Char( + string="Filter Type", + help=( + "How to interpret filter_expression (DCI query_type: idtype-value, " + "expression, predicate). Required to evaluate the filter at notify time; " + "an unparseable/unknown filter is treated as non-matching (fail closed)." + ), + ) # Subscription State active = fields.Boolean( @@ -195,28 +203,91 @@ def check_expired_subscriptions(self): _logger.info("Deactivated %d expired subscriptions", len(expired)) return True - def notify_event(self, event_type: str, records: list, reg_type: str): - """Queue notifications for matching subscriptions. + @api.model + def _matching_subscriptions(self, event_type: str, reg_type: str): + """Active, unexpired subscriptions for an event/registry type.""" + return self.search( + [ + ("event_type", "=", event_type), + ("reg_type", "=", reg_type), + ("active", "=", True), + ("state", "=", "active"), + "|", + ("expires", "=", False), + ("expires", ">", fields.Datetime.now()), + ] + ) - Called when a registry event occurs (registration, update, delete). + def _consent_allows_partner(self, partner_id: int) -> bool: + """Whether this subscription's sender may receive this registrant's data. - Args: - event_type: Type of event (registration, update, delete) - records: List of affected record data dicts - reg_type: Registry type (SOCIAL_REGISTRY, DR, etc.) + Uses the proper consent primitive (legal-basis bypass, or per-registrant + consent via the API consent service). Fail-closed when no sender. + """ + self.ensure_one() + from ..services.consent_adapter import DCIConsentAdapter + + adapter = DCIConsentAdapter(self.env, self.sender_id) + if adapter.has_legal_basis_bypass(): + return True + return adapter.can_access_registrant(partner_id) + + def _partner_matches_filter(self, partner_id: int) -> bool: + """Whether a partner matches this subscription's filter_expression. + + Base policy: no filter -> match; a filter that this (generic) layer + cannot evaluate -> NO match (fail closed). Registry modules override + this to actually evaluate their filter shape. + """ + self.ensure_one() + return not self.filter_expression + + def _eligible_partner_ids(self, partner_ids: list) -> list: + """Subset of partner_ids this subscription is allowed to be told about.""" + self.ensure_one() + return [ + pid + for pid in (partner_ids or []) + if self._consent_allows_partner(pid) and self._partner_matches_filter(pid) + ] + + def _build_notification_records(self, partner_ids: list) -> list: + """Build the DCI record payloads for partner_ids (registry-specific). + + Base returns nothing; registry server modules (e.g. spp_dci_server_social) + override this to materialise records with this subscription's sender scope. + """ + self.ensure_one() + return [] + + def _delete_records_for(self, delete_payloads: list) -> list: + """Identifier payloads this subscription is eligible to receive on delete. + + Eligibility is precomputed at unlink time (while the record still exists) + and carried per payload as ``eligible_subscription_ids``. """ - # Find matching active subscriptions - domain = [ - ("event_type", "=", event_type), - ("reg_type", "=", reg_type), - ("active", "=", True), - ("state", "=", "active"), - "|", - ("expires", "=", False), - ("expires", ">", fields.Datetime.now()), + self.ensure_one() + return [ + {"identifiers": p.get("identifiers", [])} + for p in (delete_payloads or []) + if self.id in (p.get("eligible_subscription_ids") or []) ] - subscriptions = self.search(domain) + def notify_event(self, event_type: str, partner_ids: list, reg_type: str, delete_payloads: list = None): + """Queue notifications for matching subscriptions, scoped per subscriber. + + Each matching subscription only receives records its sender is permitted + to see (consent/legal-basis) and that match its filter_expression; the + payload is built with that subscription's sender context. + + Args: + event_type: Event type (registration, update, delete) + partner_ids: Affected res.partner ids (create/update). Ignored for delete. + reg_type: Registry type (SOCIAL_REGISTRY, DR, ...) + delete_payloads: For delete, per-record identifier payloads carrying + precomputed ``eligible_subscription_ids`` (records are gone by now). + """ + subscriptions = self._matching_subscriptions(event_type, reg_type) _logger.info( "Found %d subscriptions for event %s/%s", len(subscriptions), @@ -225,7 +296,15 @@ def notify_event(self, event_type: str, records: list, reg_type: str): ) for sub in subscriptions: - # Queue notification job for each subscription + if event_type == "delete": + records = sub._delete_records_for(delete_payloads or []) + else: + eligible = sub._eligible_partner_ids(partner_ids or []) + records = sub._build_notification_records(eligible) if eligible else [] + + if not records: + continue + sub.with_delay( channel="dci", timeout=60, diff --git a/spp_dci_server/routers/async_router.py b/spp_dci_server/routers/async_router.py index 4de334a62..9484728a8 100644 --- a/spp_dci_server/routers/async_router.py +++ b/spp_dci_server/routers/async_router.py @@ -302,6 +302,7 @@ async def subscribe( "event_type": event_type, "reg_type": reg_type, "filter_expression": filter_expression, + "filter_type": req_item.subscribe_criteria.filter_type, "original_message_id": envelope.header.message_id, "original_transaction_id": sub_request.transaction_id, "state": "pending", diff --git a/spp_dci_server/tests/test_subscription.py b/spp_dci_server/tests/test_subscription.py index a63b038e3..a2f48df82 100644 --- a/spp_dci_server/tests/test_subscription.py +++ b/spp_dci_server/tests/test_subscription.py @@ -269,15 +269,21 @@ def test_notify_event_finds_matching_subscriptions(self): } ) - # Patch with_delay at the model class level to avoid read-only attribute error - with patch.object(type(self.Subscription), "with_delay") as mock_delay: - mock_job = MagicMock() - mock_delay.return_value = mock_job + # notify_event now scopes per subscription: it queues a job only when the + # subscription's sender is eligible AND records can be built. The generic + # layer builds no records (registry modules do), so stub those hooks to + # verify the matching/queueing orchestration. + with ( + patch.object(type(self.Subscription), "with_delay") as mock_delay, + patch.object(type(self.Subscription), "_eligible_partner_ids", return_value=[1]), + patch.object(type(self.Subscription), "_build_notification_records", return_value=[{"id": "rec-001"}]), + ): + mock_delay.return_value = MagicMock() # Trigger notification self.Subscription.notify_event( event_type="registration", - records=[{"id": "rec-001", "name": "Test"}], + partner_ids=[1], reg_type="SOCIAL_REGISTRY", ) @@ -302,7 +308,7 @@ def test_notify_event_ignores_inactive(self): # Trigger notification self.Subscription.notify_event( event_type="registration", - records=[{"id": "rec-001"}], + partner_ids=[1], reg_type="SOCIAL_REGISTRY", ) diff --git a/spp_dci_server_social/models/__init__.py b/spp_dci_server_social/models/__init__.py index 5104211d5..33b1d27aa 100644 --- a/spp_dci_server_social/models/__init__.py +++ b/spp_dci_server_social/models/__init__.py @@ -1,4 +1,5 @@ # Part of OpenSPP. See LICENSE file for full copyright and licensing details. from . import fastapi_endpoint_social +from . import dci_subscription_social from . import res_partner_dci_notify diff --git a/spp_dci_server_social/models/dci_subscription_social.py b/spp_dci_server_social/models/dci_subscription_social.py new file mode 100644 index 000000000..e6a96513a --- /dev/null +++ b/spp_dci_server_social/models/dci_subscription_social.py @@ -0,0 +1,82 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Social Registry scoping for DCI event subscriptions. + +Implements the registry-specific hooks the generic subscription model uses to +deliver notifications per subscriber: evaluating the subscription's stored +filter against a registrant, and building the DCI record payload with the +subscription's sender context (so consent/sender-scoped serialization applies). +""" + +import json +import logging +from types import SimpleNamespace + +from odoo import models + +_logger = logging.getLogger(__name__) + +_SOCIAL = "SOCIAL_REGISTRY" + + +class DCISubscriptionSocial(models.Model): + _inherit = "spp.dci.subscription" + + def _partner_matches_filter(self, partner_id): + """Evaluate the subscription's filter_expression against a registrant. + + No filter -> match. Otherwise the stored filter (with its filter_type + discriminator) is compiled to a domain via the Social Registry search + service and tested against the partner. Any parse/eval failure is + treated as NON-matching (fail closed) so an unparseable filter never + widens delivery. + """ + self.ensure_one() + if self.reg_type != _SOCIAL: + return super()._partner_matches_filter(partner_id) + if not self.filter_expression: + return True + try: + from ..services.search_service import DCISocialSearchService + + criteria = SimpleNamespace( + query_type=self.filter_type or "expression", + query=json.loads(self.filter_expression), + ) + service = DCISocialSearchService(self.env, self.sender_id) + domain = service._build_domain(criteria) + # sudo: matching the sender's declared filter against the registry; + # consent/authorization is enforced separately by _consent_allows_partner. + # nosemgrep: odoo-sudo-on-sensitive-models, odoo-sudo-without-context + Partner = self.env["res.partner"].sudo() + return bool(Partner.search_count([("id", "=", partner_id)] + domain)) + except Exception as e: + _logger.warning( + "Subscription %s: could not evaluate filter, dropping record (fail-closed): %s", + self.subscription_code, + e, + ) + return False + + def _build_notification_records(self, partner_ids): + """Build DCI records for partner_ids using this subscription's sender. + + Called only for already-eligible partners (consent + filter checked by + the generic layer). Builds with sender context so any sender-scoped + serialization applies. + """ + self.ensure_one() + if self.reg_type != _SOCIAL: + return super()._build_notification_records(partner_ids) + + from ..services.search_service import DCISocialSearchService + + service = DCISocialSearchService(self.env, self.sender_id) + partners = self.env["res.partner"].browse(partner_ids).exists() + records = [] + for partner in partners: + try: + dci_record = service._to_dci_group(partner) if partner.is_group else service._to_dci_person(partner) + records.append(dci_record.model_dump(mode="json", by_alias=True, exclude_none=True)) + except Exception as e: + _logger.warning("Failed to convert partner %d to DCI format: %s", partner.id, e) + return records diff --git a/spp_dci_server_social/models/res_partner_dci_notify.py b/spp_dci_server_social/models/res_partner_dci_notify.py index 702732912..e59ea946e 100644 --- a/spp_dci_server_social/models/res_partner_dci_notify.py +++ b/spp_dci_server_social/models/res_partner_dci_notify.py @@ -95,13 +95,19 @@ def unlink(self): return result def _dci_delete_payloads(self): - """Snapshot external identifiers for delete notifications. + """Snapshot external identifiers + per-subscription eligibility for delete. Returns one payload dict per registrant in ``self`` containing the registrant's external identifiers (namespace URI preferred, falling - back to the vocabulary code). Raw database ids are deliberately not - included (api-design principle: never expose DB IDs). + back to the vocabulary code) and the ids of the delete subscriptions + whose sender is allowed to be told about this registrant (consent + + filter), computed now while the record still exists. Raw database ids + are deliberately not included (api-design principle: never expose DB IDs). """ + delete_subs = self.env["spp.dci.subscription"] + if "spp.dci.subscription" in self.env: + delete_subs = delete_subs._matching_subscriptions("delete", "SOCIAL_REGISTRY") + payloads = [] for partner in self: identifiers = [ @@ -112,7 +118,17 @@ def _dci_delete_payloads(self): for reg_id in partner.reg_ids if reg_id.value and reg_id.id_type_id ] - payloads.append({"identifiers": identifiers}) + eligible_subscription_ids = [ + sub.id + for sub in delete_subs + if sub._consent_allows_partner(partner.id) and sub._partner_matches_filter(partner.id) + ] + payloads.append( + { + "identifiers": identifiers, + "eligible_subscription_ids": eligible_subscription_ids, + } + ) return payloads def _schedule_dci_notification(self, event_type, partner_ids, payloads=None): @@ -239,47 +255,19 @@ def _execute_dci_notification(self, event_type, partner_ids, payloads=None): return Subscription = self.env["spp.dci.subscription"] - # For delete events the records are gone - use the identifier - # payloads snapshotted in unlink(). Jobs queued before this field - # existed carry no payloads; emit empty identifier lists rather - # than leaking raw database ids. + # notify_event scopes delivery per subscription: each matching + # subscription only receives records its sender is permitted to see + # (consent/legal-basis) and that match its filter, with the payload + # built using that subscription's sender context. The record building + # and filter evaluation live in the per-subscription hooks + # (see dci_subscription_social.py), so this method only hands off the + # affected partner ids (create/update) or the eligibility-scoped + # identifier payloads snapshotted at unlink (delete). if event_type == "delete": - records = payloads if payloads is not None else [{"identifiers": []} for _ in partner_ids] - Subscription.notify_event(event_type, records, "SOCIAL_REGISTRY") + # Jobs queued before eligibility snapshotting carry no payloads; + # emit nothing rather than leaking to unscoped subscribers. + delete_payloads = payloads if payloads is not None else [] + Subscription.notify_event(event_type, partner_ids, "SOCIAL_REGISTRY", delete_payloads=delete_payloads) return - # For create/update, fetch current partner data and convert to DCI format - partners = self.env["res.partner"].browse(partner_ids).exists() - if not partners: - _logger.warning("No partners found for notification IDs: %s", partner_ids) - return - - # Import search service to convert to DCI format - try: - from ..services.search_service import DCISocialSearchService - - search_service = DCISocialSearchService(self.env) - - records = [] - for partner in partners: - try: - if partner.is_group: - dci_record = search_service._to_dci_group(partner) - else: - dci_record = search_service._to_dci_person(partner) - # Convert to dict - records.append(dci_record.model_dump(mode="json", by_alias=True, exclude_none=True)) - except Exception as e: - _logger.warning( - "Failed to convert partner %d to DCI format: %s", - partner.id, - str(e), - ) - - if records: - Subscription.notify_event(event_type, records, "SOCIAL_REGISTRY") - - except ImportError: - _logger.error("DCISocialSearchService not available for notification conversion") - except Exception as e: - _logger.exception("Error executing DCI notification: %s", str(e)) + Subscription.notify_event(event_type, partner_ids, "SOCIAL_REGISTRY") diff --git a/spp_dci_server_social/tests/test_dci_notifications.py b/spp_dci_server_social/tests/test_dci_notifications.py index 2507a58e6..de28a456d 100644 --- a/spp_dci_server_social/tests/test_dci_notifications.py +++ b/spp_dci_server_social/tests/test_dci_notifications.py @@ -1,6 +1,7 @@ # Part of OpenSPP. See LICENSE file for full copyright and licensing details. """Tests for DCI notification triggers on res.partner changes.""" +import json import logging from unittest.mock import MagicMock, patch @@ -152,9 +153,10 @@ def test_delete_notification_payload_has_no_db_ids(self): self.env.cr.postcommit.run() mock_notify.assert_called_once() - event_type, records, reg_type = mock_notify.call_args[0] + event_type = mock_notify.call_args[0][0] + delete_payloads = mock_notify.call_args.kwargs["delete_payloads"] self.assertEqual(event_type, "delete") - for record in records: + for record in delete_payloads: self.assertNotIn("id", record, f"delete payload leaks raw DB id: {record}") self.assertIn("identifiers", record) @@ -241,18 +243,19 @@ def test_execute_notification_calls_subscription(self): self.assertEqual(args[2], "SOCIAL_REGISTRY") # reg_type def test_execute_notification_delete_with_ids_only(self): - """A legacy delete job queued without identifier payloads must still - notify - with an empty identifier list, never the raw DB id.""" + """A legacy delete job queued without an eligibility snapshot must + deliver to no one (fail-closed): without per-subscription eligibility we + cannot know who is authorised, so we send empty delete_payloads.""" with patch.object(self.env["spp.dci.subscription"].__class__, "notify_event") as mock_notify: partner = self.Partner.browse(self.individual_1.id) # Simulate a legacy queued job (no payloads argument serialized) partner._execute_dci_notification("delete", [99999]) # Non-existent ID - # Verify notify_event was still called + # notify_event is called, but with no payloads -> notifies nobody. mock_notify.assert_called_once() args = mock_notify.call_args[0] self.assertEqual(args[0], "delete") - self.assertEqual(args[1], [{"identifiers": []}]) + self.assertEqual(mock_notify.call_args.kwargs["delete_payloads"], []) def test_multiple_writes_same_transaction(self): """Test that multiple writes in same transaction are handled.""" @@ -302,3 +305,95 @@ def test_group_notification(self): args = mock_queue.call_args[0] self.assertEqual(args[0], "registration") self.assertIn(group.id, args[1]) + + +@tagged("post_install", "-at_install") +class TestDCINotificationScoping(DCISocialServerCommon): + """Per-subscription consent + filter scoping of notification delivery.""" + + def setUp(self): + super().setUp() + self.Subscription = self.env["spp.dci.subscription"] + self.subject = self._create_test_individual( + {"family_name": "ScopeMatch", "given_name": "Subject"}, + identifier_value="NAT-SCOPE-001", + ) + + def _sub(self, **vals): + base = { + "sender_id": self.test_sender.id, + "event_type": "update", + "reg_type": "SOCIAL_REGISTRY", + "state": "active", + } + base.update(vals) + return self.Subscription.create(base) + + # --- consent (A) --------------------------------------------------------- + + def test_consent_blocks_without_basis(self): + self.test_sender.write({"legal_basis": "consent", "is_require_consent": True}) + sub = self._sub() + self.assertFalse(sub._consent_allows_partner(self.subject.id)) + self.assertEqual(sub._eligible_partner_ids([self.subject.id]), []) + + def test_legal_basis_bypass_allows(self): + self.test_sender.write({"legal_basis": "legal_obligation"}) + sub = self._sub() + self.assertTrue(sub._consent_allows_partner(self.subject.id)) + self.assertEqual(sub._eligible_partner_ids([self.subject.id]), [self.subject.id]) + + # --- filter (B) ---------------------------------------------------------- + + def test_filter_matching_and_nonmatching(self): + self.test_sender.write({"legal_basis": "legal_obligation"}) # isolate filter from consent + match = self._sub( + filter_type="expression", + filter_expression=json.dumps( + {"seq": [{"attribute": "family_name", "operator": "=", "value": "ScopeMatch"}]} + ), + ) + nomatch = self._sub( + filter_type="expression", + filter_expression=json.dumps({"seq": [{"attribute": "family_name", "operator": "=", "value": "Other"}]}), + ) + self.assertTrue(match._partner_matches_filter(self.subject.id)) + self.assertFalse(nomatch._partner_matches_filter(self.subject.id)) + + def test_unparseable_filter_fails_closed(self): + self.test_sender.write({"legal_basis": "legal_obligation"}) + sub = self._sub(filter_type="expression", filter_expression="{ not valid json") + self.assertFalse(sub._partner_matches_filter(self.subject.id)) + + def test_no_filter_matches(self): + self.test_sender.write({"legal_basis": "legal_obligation"}) + sub = self._sub() + self.assertTrue(sub._partner_matches_filter(self.subject.id)) + + # --- record building (sender context) ------------------------------------ + + def test_build_records_for_eligible_partner(self): + self.test_sender.write({"legal_basis": "legal_obligation"}) + sub = self._sub() + records = sub._build_notification_records([self.subject.id]) + self.assertEqual(len(records), 1) + self.assertIn("identifier", records[0]) + + # --- delete eligibility snapshot ----------------------------------------- + + def test_delete_payload_scopes_to_eligible_subscriptions(self): + # An active delete subscription for a bypass sender is eligible; the + # snapshot taken at unlink records its id so only it is notified. + self.test_sender.write({"legal_basis": "legal_obligation"}) + del_sub = self._sub(event_type="delete") + payloads = self.subject._dci_delete_payloads() + self.assertEqual(len(payloads), 1) + self.assertIn(del_sub.id, payloads[0]["eligible_subscription_ids"]) + + def test_delete_payload_excludes_unconsented_subscription(self): + self.test_sender.write({"legal_basis": "consent", "is_require_consent": True}) + del_sub = self._sub(event_type="delete") + payloads = self.subject._dci_delete_payloads() + self.assertNotIn(del_sub.id, payloads[0]["eligible_subscription_ids"]) + # _delete_records_for therefore yields nothing for that subscription. + self.assertEqual(del_sub._delete_records_for(payloads), []) From 21bb862121e9ab521c4b524b34ac977e8d22b851 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Mon, 29 Jun 2026 23:03:58 +0800 Subject: [PATCH 2/3] security(dci): batch filter eval + cover notification scoping helpers Address review on PR #258: - Gemini (perf): replace per-partner filter evaluation (O(partners x subscriptions) queries) with a batched `_filter_matching_partners(partner_ids)` that evaluates the filter in a single query per subscription. The social override now intersects partner_ids with the compiled filter domain in one search; `_partner_matches_filter` is a thin wrapper; `_eligible_partner_ids` applies consent per record then filters in one batch. - Codecov: add generic unit tests for the new subscription helpers (_filter_matching_partners base policy, _partner_matches_filter wrapper, _consent_allows_partner bypass, _eligible_partner_ids, _delete_records_for, notify_event delete scoping) that the generic spp_dci_server test DB exercises. Tests: spp_dci_server 326/326, spp_dci_server_social 82/82. --- spp_dci_server/models/subscription.py | 34 +++++++---- spp_dci_server/tests/test_subscription.py | 60 +++++++++++++++++++ .../models/dci_subscription_social.py | 24 ++++---- 3 files changed, 95 insertions(+), 23 deletions(-) diff --git a/spp_dci_server/models/subscription.py b/spp_dci_server/models/subscription.py index e96f33e91..e4adbac3f 100644 --- a/spp_dci_server/models/subscription.py +++ b/spp_dci_server/models/subscription.py @@ -232,24 +232,34 @@ def _consent_allows_partner(self, partner_id: int) -> bool: return True return adapter.can_access_registrant(partner_id) - def _partner_matches_filter(self, partner_id: int) -> bool: - """Whether a partner matches this subscription's filter_expression. + def _filter_matching_partners(self, partner_ids: list) -> list: + """Subset of partner_ids matching this subscription's filter_expression. - Base policy: no filter -> match; a filter that this (generic) layer - cannot evaluate -> NO match (fail closed). Registry modules override - this to actually evaluate their filter shape. + Base policy: no filter -> all; a filter that this (generic) layer cannot + evaluate -> none (fail closed). Registry modules override this to + evaluate their filter shape in a single batched query (avoiding an + O(partners x subscriptions) per-record query pattern). """ self.ensure_one() - return not self.filter_expression + if not self.filter_expression: + return list(partner_ids or []) + return [] + + def _partner_matches_filter(self, partner_id: int) -> bool: + """Whether a single partner matches this subscription's filter.""" + self.ensure_one() + return bool(self._filter_matching_partners([partner_id])) def _eligible_partner_ids(self, partner_ids: list) -> list: - """Subset of partner_ids this subscription is allowed to be told about.""" + """Subset of partner_ids this subscription is allowed to be told about. + + Applies consent per record, then the filter in a single batch. + """ self.ensure_one() - return [ - pid - for pid in (partner_ids or []) - if self._consent_allows_partner(pid) and self._partner_matches_filter(pid) - ] + if not partner_ids: + return [] + consented = [pid for pid in partner_ids if self._consent_allows_partner(pid)] + return self._filter_matching_partners(consented) def _build_notification_records(self, partner_ids: list) -> list: """Build the DCI record payloads for partner_ids (registry-specific). diff --git a/spp_dci_server/tests/test_subscription.py b/spp_dci_server/tests/test_subscription.py index a2f48df82..5cf12ddbf 100644 --- a/spp_dci_server/tests/test_subscription.py +++ b/spp_dci_server/tests/test_subscription.py @@ -315,6 +315,66 @@ def test_notify_event_ignores_inactive(self): # Should not have been called for inactive subscription mock_delay.assert_not_called() + def _make_sub(self, **vals): + base = { + "sender_id": self.test_sender.id, + "event_type": "update", + "reg_type": "SOCIAL_REGISTRY", + "state": "active", + } + base.update(vals) + return self.Subscription.create(base) + + def test_filter_matching_partners_base_policy(self): + """Generic layer: no filter -> all; an (unparseable here) filter -> none.""" + self.assertEqual(self._make_sub()._filter_matching_partners([1, 2]), [1, 2]) + # A present filter cannot be evaluated by the generic layer -> fail closed. + self.assertEqual(self._make_sub(filter_expression="{}")._filter_matching_partners([1, 2]), []) + + def test_partner_matches_filter_wrapper(self): + self.assertTrue(self._make_sub()._partner_matches_filter(1)) + self.assertFalse(self._make_sub(filter_expression="{}")._partner_matches_filter(1)) + + def test_consent_allows_partner_with_legal_basis_bypass(self): + self.test_sender.write({"legal_basis": "legal_obligation"}) + # Bypass short-circuits before any per-registrant consent lookup. + self.assertTrue(self._make_sub()._consent_allows_partner(999999)) + + def test_eligible_partner_ids_consent_then_filter(self): + self.test_sender.write({"legal_basis": "legal_obligation"}) + self.assertEqual(self._make_sub()._eligible_partner_ids([1, 2, 3]), [1, 2, 3]) + # Present-but-unevaluable filter drops everything even with consent. + self.assertEqual(self._make_sub(filter_expression="{}")._eligible_partner_ids([1, 2, 3]), []) + + def test_delete_records_for_scopes_by_eligibility(self): + sub = self._make_sub(event_type="delete") + payloads = [ + {"identifiers": [{"identifier_value": "A"}], "eligible_subscription_ids": [sub.id]}, + {"identifiers": [{"identifier_value": "B"}], "eligible_subscription_ids": [sub.id + 999]}, + ] + self.assertEqual(sub._delete_records_for(payloads), [{"identifiers": [{"identifier_value": "A"}]}]) + + def test_notify_event_delete_scoped_to_eligible(self): + sub = self._make_sub(event_type="delete") + with patch.object(type(self.Subscription), "with_delay") as mock_delay: + mock_delay.return_value = MagicMock() + self.Subscription.notify_event( + "delete", + [], + "SOCIAL_REGISTRY", + delete_payloads=[{"identifiers": [], "eligible_subscription_ids": [sub.id]}], + ) + mock_delay.assert_called() + + with patch.object(type(self.Subscription), "with_delay") as mock_delay_none: + self.Subscription.notify_event( + "delete", + [], + "SOCIAL_REGISTRY", + delete_payloads=[{"identifiers": [], "eligible_subscription_ids": [sub.id + 999]}], + ) + mock_delay_none.assert_not_called() + def test_build_notification_structure(self): """Test notification envelope structure.""" sub = self.Subscription.create( diff --git a/spp_dci_server_social/models/dci_subscription_social.py b/spp_dci_server_social/models/dci_subscription_social.py index e6a96513a..9047a73bf 100644 --- a/spp_dci_server_social/models/dci_subscription_social.py +++ b/spp_dci_server_social/models/dci_subscription_social.py @@ -21,20 +21,22 @@ class DCISubscriptionSocial(models.Model): _inherit = "spp.dci.subscription" - def _partner_matches_filter(self, partner_id): - """Evaluate the subscription's filter_expression against a registrant. + def _filter_matching_partners(self, partner_ids): + """Evaluate the subscription's filter_expression against registrants in batch. - No filter -> match. Otherwise the stored filter (with its filter_type + No filter -> all. Otherwise the stored filter (with its filter_type discriminator) is compiled to a domain via the Social Registry search - service and tested against the partner. Any parse/eval failure is - treated as NON-matching (fail closed) so an unparseable filter never - widens delivery. + service and intersected with partner_ids in a single query. Any + parse/eval failure is treated as matching NOTHING (fail closed) so an + unparseable filter never widens delivery. """ self.ensure_one() if self.reg_type != _SOCIAL: - return super()._partner_matches_filter(partner_id) + return super()._filter_matching_partners(partner_ids) if not self.filter_expression: - return True + return list(partner_ids or []) + if not partner_ids: + return [] try: from ..services.search_service import DCISocialSearchService @@ -48,14 +50,14 @@ def _partner_matches_filter(self, partner_id): # consent/authorization is enforced separately by _consent_allows_partner. # nosemgrep: odoo-sudo-on-sensitive-models, odoo-sudo-without-context Partner = self.env["res.partner"].sudo() - return bool(Partner.search_count([("id", "=", partner_id)] + domain)) + return Partner.search([("id", "in", list(partner_ids))] + domain).ids except Exception as e: _logger.warning( - "Subscription %s: could not evaluate filter, dropping record (fail-closed): %s", + "Subscription %s: could not evaluate filter, dropping records (fail-closed): %s", self.subscription_code, e, ) - return False + return [] def _build_notification_records(self, partner_ids): """Build DCI records for partner_ids using this subscription's sender. From 7465ae58c375b095a6e307f1613f14aa62570d82 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Mon, 29 Jun 2026 23:28:39 +0800 Subject: [PATCH 3/3] security(dci): fail closed on missing filter_type (staff-review fix) Staff review of the implemented branch caught a HIGH over-delivery regression: the social filter evaluator defaulted query_type to "expression" when filter_type was unset. The DCI client never sends filter_type, so a stored idtype-value filter (a specific person) was parsed as an expression with no seq/or keys -> empty domain -> matched EVERY registrant in the batch. For legal-basis-bypass senders (consent does not backstop) this re-introduced the full unscoped PII fan-out the PR set out to fix. Fix: in _filter_matching_partners, treat the SPDCI {"type":"*","value":"*"} wildcard as match-all (the client's "subscribe to all" default; consent still gates), but for any other filter require a known filter_type (idtype-value/expression/predicate) and fail closed (match nothing) when the discriminator is missing/unknown rather than guessing. Tests: add regressions for the two cases that would have caught this - real idtype-value filter without filter_type -> matches nothing; wildcard filter without filter_type -> matches all. spp_dci_server_social 84/84. --- .../models/dci_subscription_social.py | 39 +++++++++++++++++-- .../tests/test_dci_notifications.py | 19 +++++++++ 2 files changed, 54 insertions(+), 4 deletions(-) diff --git a/spp_dci_server_social/models/dci_subscription_social.py b/spp_dci_server_social/models/dci_subscription_social.py index 9047a73bf..3036a6504 100644 --- a/spp_dci_server_social/models/dci_subscription_social.py +++ b/spp_dci_server_social/models/dci_subscription_social.py @@ -37,13 +37,39 @@ def _filter_matching_partners(self, partner_ids): return list(partner_ids or []) if not partner_ids: return [] + try: - from ..services.search_service import DCISocialSearchService + raw_filter = json.loads(self.filter_expression) + except Exception as e: + _logger.warning( + "Subscription %s: unparseable filter, dropping records (fail-closed): %s", + self.subscription_code, + e, + ) + return [] + + # The SPDCI "match all" wildcard means no real filter -> deliver all + # (consent still gates non-bypass senders). + if self._is_match_all_filter(raw_filter): + return list(partner_ids) - criteria = SimpleNamespace( - query_type=self.filter_type or "expression", - query=json.loads(self.filter_expression), + # A real filter needs its discriminator to be interpreted correctly. + # Missing/unknown filter_type must NOT be guessed: defaulting to + # "expression" silently collapses an idtype-value filter to "all + # registrants" (over-delivery). Fail closed instead. + query_type = (self.filter_type or "").strip().lower() + if query_type not in ("idtype-value", "expression", "predicate"): + _logger.warning( + "Subscription %s: filter present but filter_type is %r; dropping records (fail-closed)", + self.subscription_code, + self.filter_type, ) + return [] + + try: + from ..services.search_service import DCISocialSearchService + + criteria = SimpleNamespace(query_type=query_type, query=raw_filter) service = DCISocialSearchService(self.env, self.sender_id) domain = service._build_domain(criteria) # sudo: matching the sender's declared filter against the registry; @@ -59,6 +85,11 @@ def _filter_matching_partners(self, partner_ids): ) return [] + @staticmethod + def _is_match_all_filter(raw_filter): + """SPDCI clients use {"type": "*", "value": "*"} to mean 'all events'.""" + return isinstance(raw_filter, dict) and raw_filter.get("type") == "*" and raw_filter.get("value") == "*" + def _build_notification_records(self, partner_ids): """Build DCI records for partner_ids using this subscription's sender. diff --git a/spp_dci_server_social/tests/test_dci_notifications.py b/spp_dci_server_social/tests/test_dci_notifications.py index de28a456d..5fa2ba032 100644 --- a/spp_dci_server_social/tests/test_dci_notifications.py +++ b/spp_dci_server_social/tests/test_dci_notifications.py @@ -365,6 +365,25 @@ def test_unparseable_filter_fails_closed(self): sub = self._sub(filter_type="expression", filter_expression="{ not valid json") self.assertFalse(sub._partner_matches_filter(self.subject.id)) + def test_real_filter_without_filter_type_fails_closed(self): + """A specific idtype-value filter stored WITHOUT filter_type must not be + guessed as 'expression' (which would collapse to 'all registrants' and + over-deliver). Missing discriminator on a real filter -> match nothing.""" + self.test_sender.write({"legal_basis": "legal_obligation"}) + sub = self._sub( + filter_type=False, # discriminator dropped, as the DCI client does + filter_expression=json.dumps({"type": "NATIONAL_ID", "value": "SOMEONE-ELSE"}), + ) + self.assertEqual(sub._filter_matching_partners([self.subject.id]), []) + self.assertFalse(sub._partner_matches_filter(self.subject.id)) + + def test_wildcard_filter_matches_all(self): + """The SPDCI {"type":"*","value":"*"} wildcard (sent without filter_type) + means 'subscribe to all' and must match (consent gates separately).""" + self.test_sender.write({"legal_basis": "legal_obligation"}) + sub = self._sub(filter_type=False, filter_expression=json.dumps({"type": "*", "value": "*"})) + self.assertEqual(sub._filter_matching_partners([self.subject.id]), [self.subject.id]) + def test_no_filter_matches(self): self.test_sender.write({"legal_basis": "legal_obligation"}) sub = self._sub()