security(hazard): restrict impact records to hazard/registry roles#262
security(hazard): restrict impact records to hazard/registry roles#262gonzalesedwin1123 wants to merge 1 commit into
Conversation
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.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request restricts broad read access to the sensitive spp.hazard.impact model by removing its ACL for base.group_user and adding explicit group restrictions to several views. To prevent access errors for non-hazard program users, the emergency eligibility computations now query the impact model using sudo(). The review feedback highlights performance bottlenecks in these computations, recommending the use of _read_group instead of search() to avoid loading large numbers of impact records into memory.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| impact_sudo = self.env["spp.hazard.impact"].sudo() # nosemgrep: odoo-sudo-without-context | ||
| impacts = impact_sudo.search( |
There was a problem hiding this comment.
Performance Bottleneck: Memory Overhead & N+1 Queries
Using search() followed by mapped('registrant_id') loads all matching spp.hazard.impact records into the ORM cache, and then fetches the related res.partner records. 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_group to perform the aggregation directly at the database level. This avoids loading any records into memory.
| impact_sudo = self.env["spp.hazard.impact"].sudo() # nosemgrep: odoo-sudo-without-context | |
| impacts = impact_sudo.search( | |
| impact_sudo = self.env["spp.hazard.impact"].sudo() # nosemgrep: odoo-sudo-without-context | |
| groups = impact_sudo._read_group( | |
| [ | |
| ("incident_id", "in", rec.target_incident_ids.ids), | |
| ("verification_status", "=", "verified"), | |
| ] | |
| + damage_domain, | |
| groupby=["registrant_id"], | |
| ) | |
| rec.affected_registrant_count = len(groups) | |
| continue |
| impact_sudo = self.env["spp.hazard.impact"].sudo() # nosemgrep: odoo-sudo-without-context | ||
| impacts = impact_sudo.search( |
There was a problem hiding this comment.
Performance Bottleneck: Memory Overhead
Using search() followed by mapped('registrant_id') loads all matching spp.hazard.impact records into memory just to extract the registrant IDs.
You can optimize this by using _read_group to fetch only the unique registrant_ids from the database, and then using browse() to return the partner recordset. This avoids loading any impact records.
| impact_sudo = self.env["spp.hazard.impact"].sudo() # nosemgrep: odoo-sudo-without-context | |
| impacts = impact_sudo.search( | |
| impact_sudo = self.env["spp.hazard.impact"].sudo() # nosemgrep: odoo-sudo-without-context | |
| groups = impact_sudo._read_group( | |
| [ | |
| ("incident_id", "in", self.target_incident_ids.ids), | |
| ("verification_status", "=", "verified"), | |
| ] | |
| + damage_domain, | |
| groupby=["registrant_id"], | |
| ) | |
| return self.env["res.partner"].browse([r.id for r, in groups if r]) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #262 +/- ##
==========================================
- Coverage 75.29% 75.29% -0.01%
==========================================
Files 1094 1095 +1
Lines 64997 65000 +3
==========================================
Hits 48940 48940
- Misses 16057 16060 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
1 participant
Summary
spp_hazard's ACL grantedbase.group_user(every internal user) read onspp.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_readby any internal user. Dedicated hazard groups (read/viewer/officer/manager) andregistry_vieweralready grant the intended read, so thebase.group_usergrant on impact was redundant and over-broad.Fix (surgical — impact only)
base.group_userread row forspp.hazard.impactonly. The four non-PII reference/operational models (category,incident,incident.area,impact.type) keep broad internal read — sibling modules (notablyspp_drims, which reads incidents heavily) rely on that, and none of them readimpact.spp_hazard_programs: readspp.hazard.impactviasudo()in the two emergency-eligibility computes, so a program user without a hazard group isn't blocked (only aggregate counts / eligible registrants surface, never impact rows).group_hazard_read/registry_viewer/group_spp_admin) — the registrant form's stat button, Emergency Response page (with the impact O2M), list columns and search filters; and the incident form's Impacts page + "Affected" stat button. This preventsAccessErrorfor non-hazard users who reach these forms (e.g. via a CR's registrant link, or DRIMS' incident dashboard).Why not remove all 5 / add a record rule?
Removing
base.group_userfrom all 5 models brokespp_drims(incident reads) andspp_hazard_programsin an earlier revision — caught in review. Impact is the only model with registrant PII; the rest are non-sensitive and depended upon. No record rule is needed: hazard viewers are meant to see all hazard rows (no per-record ownership).Tests
base.group_useris denied read onimpactbut retains the 4 non-sensitive models.impact_idsO2M is stripped from the arch for a non-hazard user and present for a hazard user.spp_hazard88/88 ·spp_hazard_programs25/25 ·spp_drims250/250. Lint/semgrep clean.Reviewed
Three adversarial staff-review passes. The first two each caught a real regression (all-5 removal breaking DRIMS/hazard_programs; then the ungated incident-form O2M) — both fixed. Final pass: APPROVE, no blockers; an exhaustive repo-wide sweep confirmed no remaining path where a non-hazard user can read
spp.hazard.impact.