security(hazard): restrict impact records to hazard/registry roles #262
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performance Bottleneck: Memory Overhead & N+1 Queries
Using
search()followed bymapped('registrant_id')loads all matchingspp.hazard.impactrecords into the ORM cache, and then fetches the relatedres.partnerrecords. In production environments with a large number of impacts, this will cause significant memory overhead and slow down the compute method.Since you only need the count of unique registrants, you should use
_read_groupto perform the aggregation directly at the database level. This avoids loading any records into memory.