Skip to content

Commit f6f27d3

Browse files
security(hazard): restrict impact records to hazard/registry roles
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.
1 parent 9d8a2ba commit f6f27d3

8 files changed

Lines changed: 191 additions & 8 deletions

File tree

spp_hazard/security/ir.model.access.csv

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ access_spp_hazard_category_user,spp.hazard.category user,model_spp_hazard_catego
33
access_spp_hazard_incident_user,spp.hazard.incident user,model_spp_hazard_incident,base.group_user,1,0,0,0
44
access_spp_hazard_incident_area_user,spp.hazard.incident.area user,model_spp_hazard_incident_area,base.group_user,1,0,0,0
55
access_spp_hazard_impact_type_user,spp.hazard.impact.type user,model_spp_hazard_impact_type,base.group_user,1,0,0,0
6-
access_spp_hazard_impact_user,spp.hazard.impact user,model_spp_hazard_impact,base.group_user,1,0,0,0
76
access_spp_hazard_category_sysadmin,Hazard Category System Admin,model_spp_hazard_category,base.group_system,1,1,1,1
87
access_spp_hazard_incident_sysadmin,Hazard Incident System Admin,model_spp_hazard_incident,base.group_system,1,1,1,1
98
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/tests/__init__.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,3 +7,5 @@
77
from . import test_geofence
88
from . import test_alert_ingestion
99
from . import test_registrant
10+
11+
from . import test_acl_group_user
Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
# Part of OpenSPP. See LICENSE file for full copyright and licensing details.
2+
"""Security: hazard models must not be readable by every internal user.
3+
4+
Regression test for "Broad internal read access exposes hazard impact records":
5+
the ACL granted ``base.group_user`` read on the hazard models, so any internal
6+
user could read hazard data (including registrant-linked impact records) via RPC,
7+
even without a hazard role. Access must require a dedicated hazard group (or
8+
``registry_viewer``/admin), not merely being an internal user.
9+
"""
10+
11+
from odoo import Command
12+
from odoo.exceptions import AccessError
13+
from odoo.tests import tagged
14+
15+
from .common import HazardTestCase
16+
17+
# The registrant-linked impact model is sensitive and must NOT be readable by
18+
# every internal user. The other hazard models are non-PII reference/operational
19+
# data that sibling modules (e.g. spp_drims) legitimately read broadly.
20+
SENSITIVE_MODEL = "spp.hazard.impact"
21+
NON_SENSITIVE_MODELS = [
22+
"spp.hazard.category",
23+
"spp.hazard.incident",
24+
"spp.hazard.incident.area",
25+
"spp.hazard.impact.type",
26+
]
27+
ALL_HAZARD_MODELS = [SENSITIVE_MODEL, *NON_SENSITIVE_MODELS]
28+
29+
30+
@tagged("post_install", "-at_install")
31+
class TestHazardBaseUserNoAccess(HazardTestCase):
32+
@classmethod
33+
def setUpClass(cls):
34+
super().setUpClass()
35+
cls.plain_user = cls.env["res.users"].create(
36+
{
37+
"name": "Plain Internal User",
38+
"login": "plain_internal_hazard_test",
39+
"group_ids": [Command.link(cls.env.ref("base.group_user").id)],
40+
}
41+
)
42+
43+
def test_plain_internal_user_cannot_read_impact(self):
44+
"""base.group_user (any internal user) must NOT read the sensitive impact model."""
45+
with self.assertRaises(AccessError):
46+
self.env[SENSITIVE_MODEL].with_user(self.plain_user).check_access("read")
47+
48+
def test_plain_internal_user_can_read_non_sensitive_models(self):
49+
"""Non-PII hazard reference/operational models remain internally readable
50+
(sibling modules such as spp_drims depend on reading incidents)."""
51+
for model in NON_SENSITIVE_MODELS:
52+
# Raises AccessError only if broad read was wrongly removed here.
53+
self.env[model].with_user(self.plain_user).check_access("read")
54+
55+
def test_hazard_viewer_retains_read(self):
56+
"""A hazard-group user must keep read access to all hazard models."""
57+
for model in ALL_HAZARD_MODELS:
58+
self.env[model].with_user(self.hazard_viewer).check_access("read")
59+
60+
def test_registry_user_can_still_read_registrant_hazard_fields(self):
61+
"""Regression: the registrant form's hazard indicator fields read
62+
spp.hazard.impact in their compute. A registry user (Officer implies
63+
Registry Viewer, which retains hazard read) must still be able to load
64+
them after the ACL tightening — i.e. the fix must not break the form."""
65+
officer = self.env["res.users"].create(
66+
{
67+
"name": "Registry Officer (no hazard group)",
68+
"login": "registry_officer_hazard_test",
69+
"group_ids": [Command.link(self.env.ref("spp_registry.group_registry_officer").id)],
70+
}
71+
)
72+
# Sanity: this user is NOT in any hazard group.
73+
self.assertFalse(officer.has_group("spp_hazard.group_hazard_read"))
74+
75+
incident = self.env["spp.hazard.incident"].create(
76+
{
77+
"name": "Registry Officer Incident",
78+
"code": "ROI-HAZ-001",
79+
"category_id": self.category_typhoon.id,
80+
"start_date": "2024-01-01",
81+
}
82+
)
83+
self.env["spp.hazard.impact"].create(
84+
{
85+
"incident_id": incident.id,
86+
"registrant_id": self.registrant.id,
87+
"impact_type_id": self.impact_type_displacement.id,
88+
"damage_level": "moderate",
89+
"impact_date": "2024-01-02",
90+
}
91+
)
92+
registrant_as_officer = self.registrant.with_user(officer)
93+
# Force a live read through the impact O2M (not just the stored count),
94+
# which must not raise AccessError for a registry user.
95+
self.assertEqual(registrant_as_officer.hazard_impact_ids.mapped("damage_level"), ["moderate"])
96+
97+
def test_incident_form_hides_impacts_from_non_hazard_user(self):
98+
"""The incident form's Impacts O2M reads spp.hazard.impact; it must be
99+
stripped from the arch for a user without impact read (e.g. a DRIMS-only
100+
user), so opening an incident does not raise AccessError."""
101+
arch = self.env["spp.hazard.incident"].with_user(self.plain_user).get_view(view_type="form")["arch"]
102+
self.assertNotIn("impact_ids", arch)
103+
104+
def test_incident_form_shows_impacts_to_hazard_user(self):
105+
"""A hazard user still gets the Impacts O2M on the incident form."""
106+
arch = self.env["spp.hazard.incident"].with_user(self.hazard_viewer).get_view(view_type="form")["arch"]
107+
self.assertIn("impact_ids", arch)

spp_hazard/views/hazard_incident_views.xml

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@
8484
type="object"
8585
class="oe_stat_button"
8686
icon="fa-users"
87+
groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin"
8788
>
8889
<field
8990
name="affected_registrant_count"
@@ -241,7 +242,11 @@
241242
</list>
242243
</field>
243244
</page>
244-
<page string="Impacts" name="impacts">
245+
<page
246+
string="Impacts"
247+
name="impacts"
248+
groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin"
249+
>
245250
<p
246251
class="text-muted"
247252
invisible="impact_ids != []"

spp_hazard/views/registrant_views.xml

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
class="oe_stat_button"
1717
icon="fa-bolt"
1818
invisible="hazard_impact_count == 0"
19+
groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin"
1920
>
2021
<field
2122
name="hazard_impact_count"
@@ -30,6 +31,7 @@
3031
string="Emergency Response"
3132
name="emergency_response"
3233
invisible="not is_registrant"
34+
groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin"
3335
>
3436
<field name="has_active_impact" invisible="1" />
3537
<div
@@ -103,11 +105,17 @@
103105
<field name="priority">50</field>
104106
<field name="arch" type="xml">
105107
<xpath expr="//list" position="inside">
106-
<field name="hazard_impact_count" string="Impacts" optional="hide" />
108+
<field
109+
name="hazard_impact_count"
110+
string="Impacts"
111+
optional="hide"
112+
groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin"
113+
/>
107114
<field
108115
name="has_active_impact"
109116
string="Active Impact"
110117
optional="hide"
118+
groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin"
111119
/>
112120
</xpath>
113121
</field>
@@ -121,16 +129,20 @@
121129
<field name="priority">50</field>
122130
<field name="arch" type="xml">
123131
<xpath expr="//search" position="inside">
124-
<separator />
132+
<separator
133+
groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin"
134+
/>
125135
<filter
126136
name="has_active_impact"
127137
string="Has Active Impact"
128138
domain="[('has_active_impact', '=', True)]"
139+
groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin"
129140
/>
130141
<filter
131142
name="has_impact"
132143
string="Has Any Impact"
133144
domain="[('hazard_impact_count', '>', 0)]"
145+
groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin"
134146
/>
135147
</xpath>
136148
</field>

spp_hazard_programs/models/program.py

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -84,8 +84,12 @@ def _compute_affected_registrant_count(self):
8484
# Build domain for qualifying damage levels
8585
damage_domain = rec._get_damage_level_domain()
8686

87-
# Count unique registrants with qualifying impacts
88-
impacts = self.env["spp.hazard.impact"].search(
87+
# Count unique registrants with qualifying impacts.
88+
# sudo: emergency-program eligibility must consider all qualifying
89+
# impacts regardless of the viewing user's hazard access; impact rows
90+
# are not exposed, only the aggregate count.
91+
impact_sudo = self.env["spp.hazard.impact"].sudo() # nosemgrep: odoo-sudo-without-context
92+
impacts = impact_sudo.search(
8993
[
9094
("incident_id", "in", rec.target_incident_ids.ids),
9195
("verification_status", "=", "verified"),
@@ -123,8 +127,11 @@ def get_emergency_eligible_registrants(self):
123127

124128
damage_domain = self._get_damage_level_domain()
125129

126-
# Find qualifying impacts
127-
impacts = self.env["spp.hazard.impact"].search(
130+
# Find qualifying impacts.
131+
# sudo: eligibility must consider all qualifying impacts regardless of the
132+
# viewing user's hazard access; only the resulting registrants are returned.
133+
impact_sudo = self.env["spp.hazard.impact"].sudo() # nosemgrep: odoo-sudo-without-context
134+
impacts = impact_sudo.search(
128135
[
129136
("incident_id", "in", self.target_incident_ids.ids),
130137
("verification_status", "=", "verified"),
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
11
# Part of OpenSPP. See LICENSE file for full copyright and licensing details.
22

33
from . import test_hazard_programs
4+
5+
from . import test_program_user_access
Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
# Part of OpenSPP. See LICENSE file for full copyright and licensing details.
2+
"""Regression: emergency-eligibility logic must work for non-hazard program users.
3+
4+
After tightening the hazard-impact ACL (removing the broad ``base.group_user``
5+
read grant), the program eligibility computes read ``spp.hazard.impact`` via
6+
``sudo`` so a program user without any hazard group can still use them. Without
7+
that sudo, ``affected_registrant_count`` / ``get_emergency_eligible_registrants``
8+
would raise ``AccessError`` for such users.
9+
"""
10+
11+
from odoo import Command
12+
from odoo.tests import tagged
13+
14+
from .common import HazardProgramsTestCase
15+
16+
17+
@tagged("post_install", "-at_install")
18+
class TestProgramUserHazardAccess(HazardProgramsTestCase):
19+
@classmethod
20+
def setUpClass(cls):
21+
super().setUpClass()
22+
cls.program_user = cls.env["res.users"].create(
23+
{
24+
"name": "Program Manager (no hazard group)",
25+
"login": "program_mgr_no_hazard_test",
26+
"group_ids": [
27+
Command.link(cls.env.ref("base.group_user").id),
28+
Command.link(cls.env.ref("spp_programs.group_programs_manager").id),
29+
],
30+
}
31+
)
32+
cls.program.write(
33+
{
34+
"target_incident_ids": [Command.link(cls.incident_active.id)],
35+
"qualifying_damage_levels": "any",
36+
}
37+
)
38+
39+
def test_program_user_without_hazard_group_can_compute_eligibility(self):
40+
"""A program user with no hazard group must still compute emergency
41+
eligibility (the impact reads are sudo'd)."""
42+
self.assertFalse(self.program_user.has_group("spp_hazard.group_hazard_read"))
43+
program = self.program.with_user(self.program_user)
44+
# Non-stored compute -> runs live as this user; reads impact via sudo.
45+
self.assertEqual(program.affected_registrant_count, 2)
46+
# Method -> runs live as this user; reads impact via sudo.
47+
eligible = program.get_emergency_eligible_registrants()
48+
self.assertIn(self.registrant_1, eligible)
49+
self.assertIn(self.registrant_2, eligible)

0 commit comments

Comments
 (0)