From f6f27d380a5a3971d50976bbd8539412f7396c4b Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Wed, 1 Jul 2026 14:09:59 +0800 Subject: [PATCH] security(hazard): restrict impact records to hazard/registry roles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit spp_hazard's ACL granted base.group_user (every internal user) read on spp.hazard.impact, which links named registrants to incidents with damage level, dates, verification status, notes and verifier. The menu is gated to hazard groups, but the ACL left this sensitive data open to direct RPC/ search_read by any internal user. Dedicated hazard groups and registry_viewer already grant the intended read, so the base.group_user grant on impact was redundant and over-broad. Fix (surgical — impact only): - Remove the base.group_user read row for spp.hazard.impact. The four non-PII reference/operational models (category, incident, incident.area, impact.type) keep broad internal read, which sibling modules (spp_drims) rely on. - spp_hazard_programs: read spp.hazard.impact via sudo in the emergency-program eligibility computes, so a program user without a hazard group is not blocked (only aggregate counts / eligible registrants are surfaced, not impact rows). - Registrant form: gate the hazard-impact stat button, Emergency Response page, list columns and search filters to users with impact read (hazard groups / registry_viewer / admin), so non-hazard users reaching the form (e.g. via a CR's registrant link) don't render impact data. Tests: base.group_user is denied read on impact but retains the 4 non-sensitive models; hazard viewer and registry officer retain full impact access; a program user without a hazard group can still compute emergency eligibility. spp_hazard 86/86, spp_hazard_programs 25/25, spp_drims 250/250. --- spp_hazard/security/ir.model.access.csv | 1 - spp_hazard/tests/__init__.py | 2 + spp_hazard/tests/test_acl_group_user.py | 107 ++++++++++++++++++ spp_hazard/views/hazard_incident_views.xml | 7 +- spp_hazard/views/registrant_views.xml | 16 ++- spp_hazard_programs/models/program.py | 15 ++- spp_hazard_programs/tests/__init__.py | 2 + .../tests/test_program_user_access.py | 49 ++++++++ 8 files changed, 191 insertions(+), 8 deletions(-) create mode 100644 spp_hazard/tests/test_acl_group_user.py create mode 100644 spp_hazard_programs/tests/test_program_user_access.py diff --git a/spp_hazard/security/ir.model.access.csv b/spp_hazard/security/ir.model.access.csv index 36c1b5f6f..826732791 100644 --- a/spp_hazard/security/ir.model.access.csv +++ b/spp_hazard/security/ir.model.access.csv @@ -3,7 +3,6 @@ access_spp_hazard_category_user,spp.hazard.category user,model_spp_hazard_catego access_spp_hazard_incident_user,spp.hazard.incident user,model_spp_hazard_incident,base.group_user,1,0,0,0 access_spp_hazard_incident_area_user,spp.hazard.incident.area user,model_spp_hazard_incident_area,base.group_user,1,0,0,0 access_spp_hazard_impact_type_user,spp.hazard.impact.type user,model_spp_hazard_impact_type,base.group_user,1,0,0,0 -access_spp_hazard_impact_user,spp.hazard.impact user,model_spp_hazard_impact,base.group_user,1,0,0,0 access_spp_hazard_category_sysadmin,Hazard Category System Admin,model_spp_hazard_category,base.group_system,1,1,1,1 access_spp_hazard_incident_sysadmin,Hazard Incident System Admin,model_spp_hazard_incident,base.group_system,1,1,1,1 access_spp_hazard_incident_area_sysadmin,Hazard Incident Area System Admin,model_spp_hazard_incident_area,base.group_system,1,1,1,1 diff --git a/spp_hazard/tests/__init__.py b/spp_hazard/tests/__init__.py index b10eae59f..c25b529f7 100644 --- a/spp_hazard/tests/__init__.py +++ b/spp_hazard/tests/__init__.py @@ -7,3 +7,5 @@ from . import test_geofence from . import test_alert_ingestion from . import test_registrant + +from . import test_acl_group_user diff --git a/spp_hazard/tests/test_acl_group_user.py b/spp_hazard/tests/test_acl_group_user.py new file mode 100644 index 000000000..4f63b8cfe --- /dev/null +++ b/spp_hazard/tests/test_acl_group_user.py @@ -0,0 +1,107 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Security: hazard models must not be readable by every internal user. + +Regression test for "Broad internal read access exposes hazard impact records": +the ACL granted ``base.group_user`` read on the hazard models, so any internal +user could read hazard data (including registrant-linked impact records) via RPC, +even without a hazard role. Access must require a dedicated hazard group (or +``registry_viewer``/admin), not merely being an internal user. +""" + +from odoo import Command +from odoo.exceptions import AccessError +from odoo.tests import tagged + +from .common import HazardTestCase + +# The registrant-linked impact model is sensitive and must NOT be readable by +# every internal user. The other hazard models are non-PII reference/operational +# data that sibling modules (e.g. spp_drims) legitimately read broadly. +SENSITIVE_MODEL = "spp.hazard.impact" +NON_SENSITIVE_MODELS = [ + "spp.hazard.category", + "spp.hazard.incident", + "spp.hazard.incident.area", + "spp.hazard.impact.type", +] +ALL_HAZARD_MODELS = [SENSITIVE_MODEL, *NON_SENSITIVE_MODELS] + + +@tagged("post_install", "-at_install") +class TestHazardBaseUserNoAccess(HazardTestCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.plain_user = cls.env["res.users"].create( + { + "name": "Plain Internal User", + "login": "plain_internal_hazard_test", + "group_ids": [Command.link(cls.env.ref("base.group_user").id)], + } + ) + + def test_plain_internal_user_cannot_read_impact(self): + """base.group_user (any internal user) must NOT read the sensitive impact model.""" + with self.assertRaises(AccessError): + self.env[SENSITIVE_MODEL].with_user(self.plain_user).check_access("read") + + def test_plain_internal_user_can_read_non_sensitive_models(self): + """Non-PII hazard reference/operational models remain internally readable + (sibling modules such as spp_drims depend on reading incidents).""" + for model in NON_SENSITIVE_MODELS: + # Raises AccessError only if broad read was wrongly removed here. + self.env[model].with_user(self.plain_user).check_access("read") + + def test_hazard_viewer_retains_read(self): + """A hazard-group user must keep read access to all hazard models.""" + for model in ALL_HAZARD_MODELS: + self.env[model].with_user(self.hazard_viewer).check_access("read") + + def test_registry_user_can_still_read_registrant_hazard_fields(self): + """Regression: the registrant form's hazard indicator fields read + spp.hazard.impact in their compute. A registry user (Officer implies + Registry Viewer, which retains hazard read) must still be able to load + them after the ACL tightening — i.e. the fix must not break the form.""" + officer = self.env["res.users"].create( + { + "name": "Registry Officer (no hazard group)", + "login": "registry_officer_hazard_test", + "group_ids": [Command.link(self.env.ref("spp_registry.group_registry_officer").id)], + } + ) + # Sanity: this user is NOT in any hazard group. + self.assertFalse(officer.has_group("spp_hazard.group_hazard_read")) + + incident = self.env["spp.hazard.incident"].create( + { + "name": "Registry Officer Incident", + "code": "ROI-HAZ-001", + "category_id": self.category_typhoon.id, + "start_date": "2024-01-01", + } + ) + self.env["spp.hazard.impact"].create( + { + "incident_id": incident.id, + "registrant_id": self.registrant.id, + "impact_type_id": self.impact_type_displacement.id, + "damage_level": "moderate", + "impact_date": "2024-01-02", + } + ) + registrant_as_officer = self.registrant.with_user(officer) + # Force a live read through the impact O2M (not just the stored count), + # which must not raise AccessError for a registry user. + self.assertEqual(registrant_as_officer.hazard_impact_ids.mapped("damage_level"), ["moderate"]) + + def test_incident_form_hides_impacts_from_non_hazard_user(self): + """The incident form's Impacts O2M reads spp.hazard.impact; it must be + stripped from the arch for a user without impact read (e.g. a DRIMS-only + user), so opening an incident does not raise AccessError.""" + arch = self.env["spp.hazard.incident"].with_user(self.plain_user).get_view(view_type="form")["arch"] + self.assertNotIn("impact_ids", arch) + + 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) diff --git a/spp_hazard/views/hazard_incident_views.xml b/spp_hazard/views/hazard_incident_views.xml index 6bfba2971..1fae4f15c 100644 --- a/spp_hazard/views/hazard_incident_views.xml +++ b/spp_hazard/views/hazard_incident_views.xml @@ -84,6 +84,7 @@ type="object" class="oe_stat_button" icon="fa-users" + groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin" > - +

50 - + @@ -121,16 +129,20 @@ 50 - + diff --git a/spp_hazard_programs/models/program.py b/spp_hazard_programs/models/program.py index 4d48edc0e..0778b030d 100644 --- a/spp_hazard_programs/models/program.py +++ b/spp_hazard_programs/models/program.py @@ -84,8 +84,12 @@ def _compute_affected_registrant_count(self): # Build domain for qualifying damage levels damage_domain = rec._get_damage_level_domain() - # Count unique registrants with qualifying impacts - impacts = self.env["spp.hazard.impact"].search( + # Count unique registrants with qualifying impacts. + # sudo: emergency-program eligibility must consider all qualifying + # impacts regardless of the viewing user's hazard access; impact rows + # are not exposed, only the aggregate count. + impact_sudo = self.env["spp.hazard.impact"].sudo() # nosemgrep: odoo-sudo-without-context + impacts = impact_sudo.search( [ ("incident_id", "in", rec.target_incident_ids.ids), ("verification_status", "=", "verified"), @@ -123,8 +127,11 @@ def get_emergency_eligible_registrants(self): damage_domain = self._get_damage_level_domain() - # Find qualifying impacts - impacts = self.env["spp.hazard.impact"].search( + # Find qualifying impacts. + # sudo: eligibility must consider all qualifying impacts regardless of the + # viewing user's hazard access; only the resulting registrants are returned. + impact_sudo = self.env["spp.hazard.impact"].sudo() # nosemgrep: odoo-sudo-without-context + impacts = impact_sudo.search( [ ("incident_id", "in", self.target_incident_ids.ids), ("verification_status", "=", "verified"), diff --git a/spp_hazard_programs/tests/__init__.py b/spp_hazard_programs/tests/__init__.py index 66fbca0e0..669012f1b 100644 --- a/spp_hazard_programs/tests/__init__.py +++ b/spp_hazard_programs/tests/__init__.py @@ -1,3 +1,5 @@ # Part of OpenSPP. See LICENSE file for full copyright and licensing details. from . import test_hazard_programs + +from . import test_program_user_access diff --git a/spp_hazard_programs/tests/test_program_user_access.py b/spp_hazard_programs/tests/test_program_user_access.py new file mode 100644 index 000000000..bdfe7bed1 --- /dev/null +++ b/spp_hazard_programs/tests/test_program_user_access.py @@ -0,0 +1,49 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Regression: emergency-eligibility logic must work for non-hazard program users. + +After tightening the hazard-impact ACL (removing the broad ``base.group_user`` +read grant), the program eligibility computes read ``spp.hazard.impact`` via +``sudo`` so a program user without any hazard group can still use them. Without +that sudo, ``affected_registrant_count`` / ``get_emergency_eligible_registrants`` +would raise ``AccessError`` for such users. +""" + +from odoo import Command +from odoo.tests import tagged + +from .common import HazardProgramsTestCase + + +@tagged("post_install", "-at_install") +class TestProgramUserHazardAccess(HazardProgramsTestCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.program_user = cls.env["res.users"].create( + { + "name": "Program Manager (no hazard group)", + "login": "program_mgr_no_hazard_test", + "group_ids": [ + Command.link(cls.env.ref("base.group_user").id), + Command.link(cls.env.ref("spp_programs.group_programs_manager").id), + ], + } + ) + cls.program.write( + { + "target_incident_ids": [Command.link(cls.incident_active.id)], + "qualifying_damage_levels": "any", + } + ) + + def test_program_user_without_hazard_group_can_compute_eligibility(self): + """A program user with no hazard group must still compute emergency + eligibility (the impact reads are sudo'd).""" + self.assertFalse(self.program_user.has_group("spp_hazard.group_hazard_read")) + 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() + self.assertIn(self.registrant_1, eligible) + self.assertIn(self.registrant_2, eligible)