security(hazard): gate stored registrant impact indicators with field-level groups (fix-up for #262) - #514
Merged
gonzalesedwin1123 merged 4 commits intoSep 10, 2026
Conversation
…level groups Merge-turn review of #262 on the batch-3 staging branch found that res.partner.hazard_impact_count and has_active_impact are stored columns derived from the sensitive impact table, and #262 gated only the list and search views. Any internal user could still search_read / read_group them over RPC and enumerate which registrants are disaster victims, per person. Apply the same field-level groups= as the impact model's read ACL to both fields. All form/list/search elements that reference them already sit inside #262's gated containers, and no other module reads them. The review also claimed that removing base.group_user read on impacts would break partner creation for users without impact read, because the computes query spp.hazard.impact. That does not hold: stored computes run as superuser (compute_sudo defaults to True for stored fields). A test creating a partner as a Contact Creation-only user, flushing in that user's env, pins the behaviour. Tests: spp_hazard 83/83 locally (3 new; the RPC-read test is red without the groups=). HISTORY amended under the unreleased 19.0.2.1.1; no extra bump.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0-staging-sec-batch3 #514 +/- ##
===========================================================
+ Coverage 74.52% 74.89% +0.36%
===========================================================
Files 624 639 +15
Lines 43954 44599 +645
===========================================================
+ Hits 32757 33401 +644
- Misses 11197 11198 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
… paths; regenerate README Delta review of the fix-up: res.partner.hazard_impact_ids searches spp.hazard.impact in the reading user's env, so a bare read() of a partner by a user without impact read would fail on that field. Give the O2M the same field-level groups= as the two stored indicators. Extend the RPC test to cover the headline attack surface directly: search domains and order= on the gated columns, read_group over them, the O2M in a field list, fields_get() hiding all three, and the list arch stripping the columns. README.rst / index.html regenerated from CI's pinned oca-gen output. spp_hazard ACL suite 12/12 locally.
…gistrant list #262 kept the emergency aggregate (affected_registrant_count) readable by every program user, which is fine: a number. But the 'Affected' stat button and the public get_emergency_eligible_registrants() returned the list of impacted registrants to any spp.program reader with no hazard role, over the UI and over RPC. That list is the identity linkage the impact ACL protects. - action_view_affected_registrants: check_access('read') on spp.hazard.impact before building the action (buttons are RPC-callable). - The stat button carries the same field-level groups= as the impact ACL. - get_emergency_eligible_registrants -> _get_emergency_eligible_registrants: private, so not reachable via call_kw; Python callers and overrides keep working (no overrides exist in this repo, customers or legacy modules). Tests: 3 added in test_program_user_access.py (button stripped + action refused for a program user without impact read; method not public; hazard viewer keeps both); spp_hazard_programs 28/28 locally. HISTORY 19.0.2.0.1 amended (unreleased); DESCRIPTION updated; README regen left to CI.
…the amended 19.0.2.0.1 entry
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.
Fix-up for #262 on the batch-3 staging branch, from the merge-turn adversarial review (record: internal plans
batch3-merge-turn-review.md). Targets19.0-staging-sec-batch3; lands as an additional reviewed squash commit before umbrella #513 merges.Finding (HIGH, confirmed)
res.partner.hazard_impact_countandhas_active_impactare stored columns derived fromspp.hazard.impact. #262 removed every internal user's read on the impact model and gated the registrant list/search views, but a stored field without field-levelgroups=is still readable through the ORM regardless of views. Anybase.group_usercould runand get a per-registrant list of disaster victims with impact counts — strictly worse than the incident-level aggregate #262 did gate.
Change
groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin"on both fields (same set as the impact read ACL and security(hazard): restrict impact records to hazard/registry roles #262's view gating).spp_hazard.19.0.2.1.1; no extra version bump. README/index.html regeneration is left to CI's pinnedoca-gen-addon-readme(its diff will be applied as a follow-up commit if pre-commit reports one).Second finding refuted
The review also flagged that the two computes query
spp.hazard.impactin the calling user's env and run on every partner create, so users without impact read would hitAccessErrorcreating contacts. Not so: stored computes run withcompute_sudo=Trueby default.test_contact_creator_without_impact_read_can_create_partnercreates a partner as a Contact-Creation-only user, flushes in that user's env, and passes on the unfixed staging code. Kept as a guard.Tests
spp_hazardlocally on the current image: 83 passed, 0 failed (80 + 3 new).test_plain_internal_user_cannot_read_registrant_impact_fieldsfails without thegroups=;test_hazard_viewer_can_read_registrant_impact_fieldspins the privileged path.Round 2 (delta review + Edwin's decision)
groups=is enforced on search domains,order=,read_group, export and relational traversal, so no presence oracle remains. Its should-fix taken:hazard_impact_ids(the O2M) now carries the samegroups=, so a bareread()of a partner by a user without impact read no longer fails on it. Tests extended to pin domain / order /read_group/ O2M /fields_get/ list-arch paths.affected_registrant_countstays visible to every program user, but the list of impacted registrants is the identity linkage the impact ACL protects.action_view_affected_registrantsnow runscheck_access("read")onspp.hazard.impact(buttons are RPC-callable), its stat button is gated with the samegroups=, andget_emergency_eligible_registrants()is renamed_get_emergency_eligible_registrants()so it is no longer reachable over RPC. No overrides exist in this repo, customer repos or legacy modules. 3 tests added;spp_hazard_programs28/28 locally. HISTORY 19.0.2.0.1 amended (unreleased in this batch); README regen via CI's diff.