security(hazard): restrict impact records to hazard/registry roles - #262
Draft
gonzalesedwin1123 wants to merge 9 commits into
Draft
security(hazard): restrict impact records to hazard/registry roles#262gonzalesedwin1123 wants to merge 9 commits into
gonzalesedwin1123 wants to merge 9 commits 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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #262 +/- ##
==========================================
- Coverage 76.18% 75.62% -0.57%
==========================================
Files 661 1087 +426
Lines 44110 64707 +20597
==========================================
+ Hits 33605 48934 +15329
- Misses 10505 15773 +5268
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Record the hazard impact ACL restriction (spp_hazard 19.0.2.0.3) and the sudo eligibility-compute fix (spp_hazard_programs 19.0.2.0.1) in versions and HISTORY changelogs (regenerated READMEs).
gonzalesedwin1123
force-pushed
the
security-hazard-impact-acl
branch
from
July 2, 2026 14:38
155f201 to
5e9de01
Compare
The earlier README regeneration was run in a local env whose RST renderer produced wider table columns than CI's pinned oca-gen environment, causing the 'Generate addons README files from fragments' pre-commit hook to fail on CI. Restore the column widths CI's generator produces.
Bring the branch up to date with 19.0 (30 commits, incl. the geofence partial-revert #271). Conflict in spp_hazard/views/hazard_incident_views.xml resolved by adopting 19.0's Impacts-page rework while preserving this PR's security `groups=` gate on the Impacts page and the 'Affected' stat button. Auto-merged manifest (kept our 2.0.3 security bump) and tests/__init__.py (kept our test_acl_group_user import, took 19.0's removal of test_alert_ingestion).
gonzalesedwin1123
marked this pull request as draft
July 20, 2026 01:19
… SQL
Replace search()+mapped('registrant_id') with _read_group so impact
rows are never loaded into memory (review feedback on the sudo'd
queries, which now scan all impacts rather than the ACL-visible
subset). The eligible-registrants recordset is returned in the
caller's env instead of the sudo one.
spp.hazard.incident stays broadly readable (sibling modules read incidents), but affected_registrant_count is derived from the sensitive spp.hazard.impact table via raw ACL-bypassing SQL. Add field-level groups= so a plain internal user cannot read the affected-registrant aggregate over RPC, and gate the incident list column. Adds regression tests.
Assign the field read to a throwaway so the assertRaises still exercises Field.__get__ without tripping pylint W0104. Regenerate spp_hazard README from the updated HISTORY fragment.
19.0's #416 took spp_hazard 19.0.2.1.0 (incident status lifecycle) past this PR's 2.0.3: re-bump to 19.0.2.1.1 and ladder the HISTORY. The code composed cleanly — #416's status-workflow additions reference no impact data, the affected_registrant_count field gate and both gated view references survive, and the Impacts page carries both #416's closed-incident readonly and this PR's groups=. Merged suites green: spp_hazard 80/0, spp_hazard_programs 25/0. README regenerated via the pinned oca-gen.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
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).spp_hazard_programs(perf): those twosudo()queries now aggregate in SQL via_read_group(registrant_id:count_distinctfor the count;groupby=["registrant_id"]for the eligible-registrants recordset) instead ofsearch()+mapped("registrant_id"), so no impact rows are loaded into memory — the sudo scope makes them scan all qualifying impacts, not just an ACL-visible subset (review feedback). The eligible-registrants recordset is returned in the caller's env, not the sudo one.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.Merge order (vs #274 hazard-cap reland)
Per the cross-PR interaction analysis (2026-07-24,
internal/plans/security-prs-interaction-analysis.md): this PR merges before #274 (which is held per the PR76 re-land plan anyway).spp_hazard19.0.2.0.3, feat(spp_hazard): CAP vocabulary severity and alert ingestion, with upgrade migration (re-land from #76) #274 then19.0.3.0.0(+migration). Reversed, this PR would be a version downgrade and must renumber to19.0.3.0.1.spp_hazard_programs: both PRs bump to the identical19.0.2.0.1— auto-merges with no conflict marker. feat(spp_hazard): CAP vocabulary severity and alert ingestion, with upgrade migration (re-land from #76) #274's post-merge rebase must re-bump it to19.0.2.0.2(hard requirement: feat(spp_hazard): CAP vocabulary severity and alert ingestion, with upgrade migration (re-land from #76) #274 changes a view that references a field its model change removes; a suppressed-ubreaks the program form).<page>tag conflict inhazard_incident_views.xmlkeeping BOTH this PR'sgroups=attribute and feat(spp_hazard): CAP vocabulary severity and alert ingestion, with upgrade migration (re-land from #76) #274's empty-state hint, and apply the samegroups=to feat(spp_hazard): CAP vocabulary severity and alert ingestion, with upgrade migration (re-land from #76) #274's new ungatedaffected_registrant_countlist column (its compute is ACL-bypassing SQL — otherwise the aggregate this PR hides in the form view resurfaces in the list view).