diff --git a/spp_hazard/README.rst b/spp_hazard/README.rst index 5d392fbd6..ea9bb06ea 100644 --- a/spp_hazard/README.rst +++ b/spp_hazard/README.rst @@ -1196,6 +1196,14 @@ Changelog Gate the impact UI on the registrant and incident forms (stat buttons, Emergency Response / Impacts pages, list columns, search filters) to users with impact read. +- fix(security): guard the stored registrant indicators + ``res.partner.hazard_impact_count`` / ``has_active_impact`` with the + same field-level ``groups=`` as the impact ACL. Gating only the + registrant list/search views left both columns readable over RPC + (``search_read``, ``read_group``, export, search domains) by any + internal user — a per-registrant victim list. Stored computes run as + superuser, so partner creation by users without impact read is + unaffected (pinned by test). - fix(security): guard ``spp.hazard.incident.affected_registrant_count`` with field-level ``groups=``. ``spp.hazard.incident`` stays broadly readable (sibling modules read incidents), but this aggregate is diff --git a/spp_hazard/models/registrant.py b/spp_hazard/models/registrant.py index 639120386..52328dd78 100644 --- a/spp_hazard/models/registrant.py +++ b/spp_hazard/models/registrant.py @@ -17,15 +17,26 @@ class ResPartner(models.Model): "spp.hazard.impact", "registrant_id", string="Hazard Impacts", + # Reading the O2M searches spp.hazard.impact in the user's env; gate it + # like the impact ACL so a bare read() of a partner by a user without + # impact read does not fail on this field. + groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin", ) + # Both indicators are derived from the sensitive impact table and stored on + # the partner, so they are readable through the ORM (search_read, read_group, + # export, search domains) independently of the view-level gating. Field-level + # groups= mirrors the impact model's read ACL so a plain internal user cannot + # enumerate which registrants are disaster victims over RPC. hazard_impact_count = fields.Integer( compute="_compute_hazard_impact_count", string="Impact Count", store=True, + groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin", ) has_active_impact = fields.Boolean( compute="_compute_has_active_impact", store=True, + groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin", help="Whether the registrant has an impact from an active incident", ) diff --git a/spp_hazard/readme/HISTORY.md b/spp_hazard/readme/HISTORY.md index ed8809821..e95d9a1c7 100644 --- a/spp_hazard/readme/HISTORY.md +++ b/spp_hazard/readme/HISTORY.md @@ -1,6 +1,7 @@ ### 19.0.2.1.1 - fix(security): remove the `base.group_user` read grant on `spp.hazard.impact` so registrant-linked impact records (name, damage level, verification, notes) are readable only by hazard roles, `registry_viewer`, and admins — not every internal user via RPC. Gate the impact UI on the registrant and incident forms (stat buttons, Emergency Response / Impacts pages, list columns, search filters) to users with impact read. +- fix(security): guard the stored registrant indicators `res.partner.hazard_impact_count` / `has_active_impact` with the same field-level `groups=` as the impact ACL. Gating only the registrant list/search views left both columns readable over RPC (`search_read`, `read_group`, export, search domains) by any internal user — a per-registrant victim list. Stored computes run as superuser, so partner creation by users without impact read is unaffected (pinned by test). - fix(security): guard `spp.hazard.incident.affected_registrant_count` with field-level `groups=`. `spp.hazard.incident` stays broadly readable (sibling modules read incidents), but this aggregate is derived from the sensitive impact table via raw ACL-bypassing SQL, so a plain internal user could read the affected-registrant count over RPC even without impact read. The field is now restricted to hazard read / `registry_viewer` / admin, which also strips it from the incident list column for other users. ### 19.0.2.1.0 diff --git a/spp_hazard/static/description/index.html b/spp_hazard/static/description/index.html index e095a0991..8db1d981d 100644 --- a/spp_hazard/static/description/index.html +++ b/spp_hazard/static/description/index.html @@ -2463,6 +2463,14 @@

19.0.2.1.1

Gate the impact UI on the registrant and incident forms (stat buttons, Emergency Response / Impacts pages, list columns, search filters) to users with impact read. +
  • fix(security): guard the stored registrant indicators +res.partner.hazard_impact_count / has_active_impact with the +same field-level groups= as the impact ACL. Gating only the +registrant list/search views left both columns readable over RPC +(search_read, read_group, export, search domains) by any +internal user — a per-registrant victim list. Stored computes run as +superuser, so partner creation by users without impact read is +unaffected (pinned by test).
  • fix(security): guard spp.hazard.incident.affected_registrant_count with field-level groups=. spp.hazard.incident stays broadly readable (sibling modules read incidents), but this aggregate is diff --git a/spp_hazard/tests/test_acl_group_user.py b/spp_hazard/tests/test_acl_group_user.py index a06660d83..1fa2960d3 100644 --- a/spp_hazard/tests/test_acl_group_user.py +++ b/spp_hazard/tests/test_acl_group_user.py @@ -165,3 +165,77 @@ def test_incident_form_shows_impacts_to_hazard_user(self): """A hazard user still gets the Impacts O2M on the incident form.""" arch = self.env["spp.hazard.incident"].with_user(self.hazard_viewer).get_view(view_type="form")["arch"] self.assertIn("impact_ids", arch) + + def test_plain_internal_user_cannot_read_registrant_impact_fields(self): + """``res.partner.hazard_impact_count`` / ``has_active_impact`` are stored + columns derived from the sensitive impact table. Gating only the views is + not enough: a plain internal user could still ``search_read`` them over RPC + and enumerate which registrants are disaster victims. The fields must carry + field-level ``groups=`` so the ORM refuses the read.""" + partner_as_plain = self.env["res.partner"].with_user(self.plain_user) + with self.assertRaises(AccessError): + partner_as_plain.search_read( + [("id", "=", self.registrant.id)], + ["name", "hazard_impact_count", "has_active_impact"], + ) + # The headline attack is the domain, not the field list: filtering or + # ordering on the gated columns must be refused too (presence oracle). + with self.assertRaises(AccessError): + partner_as_plain.search([("has_active_impact", "=", True)]) + with self.assertRaises(AccessError): + partner_as_plain.search([("hazard_impact_count", ">", 0)]) + with self.assertRaises(AccessError): + partner_as_plain.search([("id", "=", self.registrant.id)], order="hazard_impact_count desc") + with self.assertRaises(AccessError): + partner_as_plain.read_group([], ["hazard_impact_count:sum"], ["has_active_impact"]) + # The O2M itself must not be reachable either, and all three must be + # hidden from fields_get(): that is what makes a bare read() with no + # field list (generic RPC clients) skip them instead of failing on them. + with self.assertRaises(AccessError): + partner_as_plain.search_read([("id", "=", self.registrant.id)], ["hazard_impact_ids"]) + visible = partner_as_plain.fields_get(["hazard_impact_ids", "hazard_impact_count", "has_active_impact"]) + self.assertEqual(visible, {}) + # And the gated columns/filters are stripped from the list/search arch. + arch = partner_as_plain.get_view(view_type="list")["arch"] + self.assertNotIn("hazard_impact_count", arch) + self.assertNotIn("has_active_impact", arch) + + def test_hazard_viewer_can_read_registrant_impact_fields(self): + """A hazard-group user keeps read on the registrant impact indicator fields.""" + rows = ( + self.env["res.partner"] + .with_user(self.hazard_viewer) + .search_read([("id", "=", self.registrant.id)], ["hazard_impact_count", "has_active_impact"]) + ) + self.assertEqual(len(rows), 1) + + def test_contact_creator_without_impact_read_can_create_partner(self): + """Guard: the two stored impact indicators are computed on every + ``res.partner`` create by querying ``spp.hazard.impact``. Stored computes + run as superuser (``compute_sudo`` defaults to True for stored fields), so + removing ``base.group_user`` read on impacts must NOT break partner + creation for an internal user who may create contacts but holds no + hazard/registry role (e.g. Contact Creation only). Pins that behaviour, + including the flush that actually runs the compute.""" + creator = self.env["res.users"].create( + { + "name": "Contact Creator (no hazard/registry group)", + "login": "contact_creator_hazard_test", + "group_ids": [ + Command.link(self.env.ref("base.group_user").id), + Command.link(self.env.ref("base.group_partner_manager").id), + ], + } + ) + self.assertFalse(creator.has_group("spp_hazard.group_hazard_read")) + self.assertFalse(creator.has_group("spp_registry.group_registry_viewer")) + with self.assertRaises(AccessError): + self.env[SENSITIVE_MODEL].with_user(creator).check_access("read") + + partner = self.env["res.partner"].with_user(creator).create({"name": "Created by contact creator"}) + # Stored computes are deferred to flush time; flush in the CREATOR's env + # (as a real request does at its end) so the compute runs as that user. + partner.env.flush_all() + self.assertTrue(partner.exists()) + self.assertEqual(partner.sudo().hazard_impact_count, 0) + self.assertFalse(partner.sudo().has_active_impact) diff --git a/spp_hazard_programs/README.rst b/spp_hazard_programs/README.rst index 2d9b09aac..384162d78 100644 --- a/spp_hazard_programs/README.rst +++ b/spp_hazard_programs/README.rst @@ -87,7 +87,7 @@ access from: Extension Points ~~~~~~~~~~~~~~~~ -- Override ``get_emergency_eligible_registrants()`` to customize +- Override ``_get_emergency_eligible_registrants()`` to customize eligibility logic beyond damage levels - Override ``_get_damage_level_domain()`` to add custom damage filtering rules @@ -331,8 +331,14 @@ Changelog emergency-eligibility computes (``affected_registrant_count``, ``get_emergency_eligible_registrants``), so they keep working for non-hazard program users after impact read access was restricted to - hazard/registry roles. Only aggregate counts / eligible registrants - are surfaced, not impact rows. + hazard/registry roles. Only aggregate counts are surfaced to program + users without impact read; the list of eligible (impacted) registrants + is the identity linkage the impact ACL protects, so + ``action_view_affected_registrants`` now checks impact read access + server-side, its stat button is gated in the form, and + ``get_emergency_eligible_registrants()`` is renamed + ``_get_emergency_eligible_registrants()`` so it is no longer callable + over RPC (Python callers and overrides are unaffected). 19.0.2.0.0 ~~~~~~~~~~ diff --git a/spp_hazard_programs/models/program.py b/spp_hazard_programs/models/program.py index 66efa57fa..fb1f2791e 100644 --- a/spp_hazard_programs/models/program.py +++ b/spp_hazard_programs/models/program.py @@ -113,10 +113,16 @@ def _get_damage_level_domain(self): return [("damage_level", "in", ("critical", "totally_damaged"))] return [] - def get_emergency_eligible_registrants(self): + def _get_emergency_eligible_registrants(self): """ Get registrants eligible for this emergency program based on hazard impacts. + Private on purpose: the result is the list of registrants affected by a + hazard, i.e. the identity linkage the impact ACL protects. It is meant + for Python callers (eligibility logic, overrides), not for RPC. The UI + entry point is ``action_view_affected_registrants``, which checks impact + read access before exposing the list. + Returns registrants who: - Have verified impact from one of the target incidents - Meet the qualifying damage level threshold @@ -159,9 +165,15 @@ def action_view_target_incidents(self): } def action_view_affected_registrants(self): - """Open a list view of potentially affected registrants.""" + """Open a list view of potentially affected registrants. + + The aggregate count stays visible to every program user, but the list + names the impacted registrants, so it requires impact read access. The + stat button is gated in the view; this check covers RPC callers. + """ self.ensure_one() - registrants = self.get_emergency_eligible_registrants() + self.env["spp.hazard.impact"].check_access("read") + registrants = self._get_emergency_eligible_registrants() return { "name": _("Affected Registrants - %s", self.name), "type": "ir.actions.act_window", diff --git a/spp_hazard_programs/readme/DESCRIPTION.md b/spp_hazard_programs/readme/DESCRIPTION.md index 29f7ab3bd..a2bff17c5 100644 --- a/spp_hazard_programs/readme/DESCRIPTION.md +++ b/spp_hazard_programs/readme/DESCRIPTION.md @@ -39,7 +39,7 @@ No new models or ACL entries. Fields added to existing models inherit access fro ### Extension Points -- Override `get_emergency_eligible_registrants()` to customize eligibility logic beyond damage levels +- Override `_get_emergency_eligible_registrants()` to customize eligibility logic beyond damage levels - Override `_get_damage_level_domain()` to add custom damage filtering rules - Inherit `spp.program` to add fields used in emergency calculations - Use `is_emergency_program` and `is_emergency_mode` flags in downstream program logic diff --git a/spp_hazard_programs/readme/HISTORY.md b/spp_hazard_programs/readme/HISTORY.md index 329ff0b7d..e96a173ff 100644 --- a/spp_hazard_programs/readme/HISTORY.md +++ b/spp_hazard_programs/readme/HISTORY.md @@ -1,6 +1,6 @@ ### 19.0.2.0.1 -- fix(security): read `spp.hazard.impact` via `sudo` in the emergency-eligibility computes (`affected_registrant_count`, `get_emergency_eligible_registrants`), so they keep working for non-hazard program users after impact read access was restricted to hazard/registry roles. Only aggregate counts / eligible registrants are surfaced, not impact rows. +- fix(security): read `spp.hazard.impact` via `sudo` in the emergency-eligibility computes (`affected_registrant_count`, `get_emergency_eligible_registrants`), so they keep working for non-hazard program users after impact read access was restricted to hazard/registry roles. Only aggregate counts are surfaced to program users without impact read; the list of eligible (impacted) registrants is the identity linkage the impact ACL protects, so `action_view_affected_registrants` now checks impact read access server-side, its stat button is gated in the form, and `get_emergency_eligible_registrants()` is renamed `_get_emergency_eligible_registrants()` so it is no longer callable over RPC (Python callers and overrides are unaffected). ### 19.0.2.0.0 diff --git a/spp_hazard_programs/static/description/index.html b/spp_hazard_programs/static/description/index.html index 68329ad5b..9cbb32734 100644 --- a/spp_hazard_programs/static/description/index.html +++ b/spp_hazard_programs/static/description/index.html @@ -447,7 +447,7 @@

    Security

    Extension Points

    diff --git a/spp_hazard_programs/tests/test_hazard_programs.py b/spp_hazard_programs/tests/test_hazard_programs.py index 42ccefcbe..bda984b5a 100644 --- a/spp_hazard_programs/tests/test_hazard_programs.py +++ b/spp_hazard_programs/tests/test_hazard_programs.py @@ -137,7 +137,7 @@ def test_get_emergency_eligible_registrants(self): "qualifying_damage_levels": "any", } ) - registrants = self.program.get_emergency_eligible_registrants() + registrants = self.program._get_emergency_eligible_registrants() self.assertEqual(len(registrants), 2) self.assertIn(self.registrant_1, registrants) self.assertIn(self.registrant_2, registrants) @@ -146,7 +146,7 @@ def test_get_emergency_eligible_registrants(self): def test_get_emergency_eligible_registrants_no_incidents(self): """Test eligible registrants returns empty when no incidents linked.""" - registrants = self.program.get_emergency_eligible_registrants() + registrants = self.program._get_emergency_eligible_registrants() self.assertEqual(len(registrants), 0) def test_get_emergency_eligible_registrants_with_filter(self): @@ -157,7 +157,7 @@ def test_get_emergency_eligible_registrants_with_filter(self): "qualifying_damage_levels": "critical_only", } ) - registrants = self.program.get_emergency_eligible_registrants() + registrants = self.program._get_emergency_eligible_registrants() self.assertEqual(len(registrants), 1) self.assertIn(self.registrant_1, registrants) diff --git a/spp_hazard_programs/tests/test_program_user_access.py b/spp_hazard_programs/tests/test_program_user_access.py index bdfe7bed1..3be50bcc3 100644 --- a/spp_hazard_programs/tests/test_program_user_access.py +++ b/spp_hazard_programs/tests/test_program_user_access.py @@ -43,7 +43,47 @@ def test_program_user_without_hazard_group_can_compute_eligibility(self): program = self.program.with_user(self.program_user) # Non-stored compute -> runs live as this user; reads impact via sudo. self.assertEqual(program.affected_registrant_count, 2) - # Method -> runs live as this user; reads impact via sudo. - eligible = program.get_emergency_eligible_registrants() + # Method -> runs live as this user; reads impact via sudo. Private so it + # is reachable from Python (eligibility, overrides) but not over RPC. + eligible = program._get_emergency_eligible_registrants() self.assertIn(self.registrant_1, eligible) self.assertIn(self.registrant_2, eligible) + + def test_eligible_registrants_method_is_not_rpc_callable(self): + """The eligible-registrant list is the identity linkage the impact ACL + protects. It must not be exposed as a public (call_kw-reachable) method.""" + self.assertFalse(hasattr(type(self.program), "get_emergency_eligible_registrants")) + self.assertTrue(hasattr(type(self.program), "_get_emergency_eligible_registrants")) + + def test_program_user_without_impact_read_cannot_open_affected_registrants(self): + """The 'Affected' stat button opens the list of impacted registrants. A + program user without impact read keeps the aggregate count but must be + refused the list, server-side (buttons are RPC-callable) and in the arch.""" + from odoo.exceptions import AccessError + + program = self.program.with_user(self.program_user) + self.assertEqual(program.affected_registrant_count, 2) + with self.assertRaises(AccessError): + program.action_view_affected_registrants() + arch = self.env["spp.program"].with_user(self.program_user).get_view(view_type="form")["arch"] + self.assertNotIn("action_view_affected_registrants", arch) + + def test_hazard_user_can_open_affected_registrants(self): + """A user with impact read (hazard viewer + program manager) keeps the list.""" + hazard_program_user = self.env["res.users"].create( + { + "name": "Program Manager with hazard read", + "login": "program_mgr_hazard_read_test", + "group_ids": [ + Command.link(self.env.ref("base.group_user").id), + Command.link(self.env.ref("spp_programs.group_programs_manager").id), + Command.link(self.env.ref("spp_hazard.group_hazard_viewer").id), + ], + } + ) + program = self.program.with_user(hazard_program_user) + action = program.action_view_affected_registrants() + self.assertEqual(action["res_model"], "res.partner") + self.assertEqual(set(action["domain"][0][2]), {self.registrant_1.id, self.registrant_2.id}) + arch = self.env["spp.program"].with_user(hazard_program_user).get_view(view_type="form")["arch"] + self.assertIn("action_view_affected_registrants", arch) diff --git a/spp_hazard_programs/views/program_views.xml b/spp_hazard_programs/views/program_views.xml index 910715cd6..5453049ed 100644 --- a/spp_hazard_programs/views/program_views.xml +++ b/spp_hazard_programs/views/program_views.xml @@ -28,6 +28,7 @@ class="oe_stat_button" icon="fa-users" invisible="affected_registrant_count == 0" + groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin" >