security(hazard): restrict impact records to hazard/registry roles

spp_hazard/README.rst

@@ -1186,6 +1186,25 @@ encounter unexpected behavior, please report it as a new issue.
 Changelog
 =========
 
+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 ``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
 ~~~~~~~~~~
 

spp_hazard/__manifest__.py

@@ -8,7 +8,7 @@
     "for emergency response. Links registrants to disaster events with geographic scope "
     "and severity tracking to enable targeted humanitarian assistance.",
     "category": "OpenSPP/Targeting",
-    "version": "19.0.2.1.0",
+    "version": "19.0.2.1.1",
     "sequence": 1,
     "author": "OpenSPP.org",
     "website": "https://github.com/OpenSPP/OpenSPP2",

spp_hazard/models/hazard_incident.py

@@ -124,9 +124,16 @@ class HazardIncident(models.Model):
     )
 
     # Computed metrics
+    # This aggregate is derived from the sensitive spp.hazard.impact table
+    # (registrant-linked) and is computed via raw SQL that bypasses record
+    # ACLs. spp.hazard.incident itself stays broadly readable (sibling modules
+    # such as spp_drims read incidents), so this field must carry its own
+    # group guard: without it, any internal user could read the affected-
+    # registrant aggregate over RPC even though they cannot read impact rows.
     affected_registrant_count = fields.Integer(
         compute="_compute_affected_registrant_count",
         string="Affected Registrants",
+        groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin",
     )
 
     _code_unique = models.Constraint(

spp_hazard/readme/HISTORY.md

@@ -1,3 +1,8 @@
+### 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 `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
 
 - feat(hazard): incidents start as a **Draft** and reach Alert or Active deliberately, rather than being assumed active on entry. Lifecycle moves are now refused server-side as well as hidden in the form: a draft cannot be closed (delete it instead) and a closed incident cannot be reopened (#1157, #1158)

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

spp_hazard/static/description/index.html

@@ -2454,6 +2454,26 @@ <h2><a class="toc-backref" href="#toc-entry-2">Changelog</a></h2>
 </div>
 </div>
 <div class="section" id="section-1">
+<h1>19.0.2.1.1</h1>
+<ul class="simple">
+<li>fix(security): remove the <tt class="docutils literal">base.group_user</tt> read grant on
+<tt class="docutils literal">spp.hazard.impact</tt> so registrant-linked impact records (name,
+damage level, verification, notes) are readable only by hazard roles,
+<tt class="docutils literal">registry_viewer</tt>, 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.</li>
+<li>fix(security): guard <tt class="docutils literal">spp.hazard.incident.affected_registrant_count</tt>
+with field-level <tt class="docutils literal">groups=</tt>. <tt class="docutils literal">spp.hazard.incident</tt> 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 / <tt class="docutils literal">registry_viewer</tt> / admin, which also strips it from the
+incident list column for other users.</li>
+</ul>
+</div>
+<div class="section" id="section-2">
 <h1>19.0.2.1.0</h1>
 <ul class="simple">
 <li>feat(hazard): incidents start as a <strong>Draft</strong> and reach Alert or Active
@@ -2463,7 +2483,7 @@ <h1>19.0.2.1.0</h1>
 cannot be reopened (#1157, #1158)</li>
 </ul>
 </div>
-<div class="section" id="section-2">
+<div class="section" id="section-3">
 <h1>19.0.2.0.2</h1>
 <ul class="simple">
 <li>fix(security): grant <tt class="docutils literal">group_hazard_viewer</tt> to spp_user_roles roles
@@ -2479,15 +2499,15 @@ <h1>19.0.2.0.2</h1>
 Support).</li>
 </ul>
 </div>
-<div class="section" id="section-3">
+<div class="section" id="section-4">
 <h1>19.0.2.0.1</h1>
 <ul class="simple">
 <li>fix(views): apply <tt class="docutils literal">spp_registry.x2many_no_padding</tt> widget to the
 hazard impacts list on registrant forms, and hide the table when empty
 (showing a muted info line instead) (#943).</li>
 </ul>
 </div>
-<div class="section" id="section-4">
+<div class="section" id="section-5">
 <h1>19.0.2.0.0</h1>
 <ul class="simple">
 <li>Initial migration to OpenSPP2</li>

spp_hazard/tests/__init__.py

@@ -6,3 +6,5 @@
 from . import test_hazard_impact_type
 from . import test_geofence
 from . import test_registrant
+
+from . import test_acl_group_user

spp_hazard/tests/test_acl_group_user.py

@@ -0,0 +1,167 @@
+# 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_plain_internal_user_cannot_read_affected_registrant_count(self):
+        """spp.hazard.incident stays broadly readable, but its
+        ``affected_registrant_count`` aggregate is derived from the sensitive
+        impact table via raw ACL-bypassing SQL. A plain internal user must be
+        able to read the incident yet be denied that field over RPC."""
+        incident = self.env["spp.hazard.incident"].create(
+            {
+                "name": "Aggregate Leak Incident",
+                "code": "ALI-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",
+            }
+        )
+        incident_as_plain = incident.with_user(self.plain_user)
+        # The incident itself remains readable (non-sensitive model)...
+        incident_as_plain.read(["name"])
+        # ...but the impact-derived aggregate must be denied.
+        with self.assertRaises(AccessError):
+            incident_as_plain.read(["affected_registrant_count"])
+        with self.assertRaises(AccessError):
+            # Attribute access goes through Field.__get__, which enforces the
+            # field-level group guard independently of read().
+            _ = incident_as_plain.affected_registrant_count
+
+    def test_hazard_viewer_can_read_affected_registrant_count(self):
+        """A hazard-group user must still read the affected-registrant aggregate."""
+        incident = self.env["spp.hazard.incident"].create(
+            {
+                "name": "Aggregate Visible Incident",
+                "code": "AVI-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",
+            }
+        )
+        self.assertEqual(incident.with_user(self.hazard_viewer).affected_registrant_count, 1)
+
+    def test_affected_registrant_count_column_hidden_from_non_hazard_user(self):
+        """The incident list column reads the gated aggregate; it must be stripped
+        from the arch for a plain internal user."""
+        arch = self.env["spp.hazard.incident"].with_user(self.plain_user).get_view(view_type="list")["arch"]
+        self.assertNotIn("affected_registrant_count", arch)
+
+    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)

spp_hazard/views/hazard_incident_views.xml

@@ -32,7 +32,11 @@
                     decoration-danger="severity == '5'"
                 />
                 <field name="area_count" string="Areas" />
-                <field name="affected_registrant_count" string="Affected" />
+                <field
+                    name="affected_registrant_count"
+                    string="Affected"
+                    groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin"
+                />
             </list>
         </field>
     </record>
@@ -92,6 +96,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"
                         >
                             <field
                                 name="affected_registrant_count"
@@ -206,7 +211,11 @@
                                 </list>
                             </field>
                         </page>
-                        <page string="Impacts" name="impacts">
+                        <page
+                            string="Impacts"
+                            name="impacts"
+                            groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin"
+                        >
                             <field name="impact_ids" readonly="status == 'closed'">
                                 <list editable="bottom" no_open="true">
                                     <field name="registrant_id" />

spp_hazard/views/registrant_views.xml

@@ -16,6 +16,7 @@
                     class="oe_stat_button"
                     icon="fa-bolt"
                     invisible="hazard_impact_count == 0"
+                    groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin"
                 >
                     <field
                         name="hazard_impact_count"
@@ -30,6 +31,7 @@
                     string="Emergency Response"
                     name="emergency_response"
                     invisible="not is_registrant"
+                    groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin"
                 >
                     <field name="has_active_impact" invisible="1" />
                     <div
@@ -103,11 +105,17 @@
         <field name="priority">50</field>
         <field name="arch" type="xml">
             <xpath expr="//list" position="inside">
-                <field name="hazard_impact_count" string="Impacts" optional="hide" />
+                <field
+                    name="hazard_impact_count"
+                    string="Impacts"
+                    optional="hide"
+                    groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin"
+                />
                 <field
                     name="has_active_impact"
                     string="Active Impact"
                     optional="hide"
+                    groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin"
                 />
             </xpath>
         </field>
@@ -121,16 +129,20 @@
         <field name="priority">50</field>
         <field name="arch" type="xml">
             <xpath expr="//search" position="inside">
-                <separator />
+                <separator
+                    groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin"
+                />
                 <filter
                     name="has_active_impact"
                     string="Has Active Impact"
                     domain="[('has_active_impact', '=', True)]"
+                    groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin"
                 />
                 <filter
                     name="has_impact"
                     string="Has Any Impact"
                     domain="[('hazard_impact_count', '>', 0)]"
+                    groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin"
                 />
             </xpath>
         </field>