security(cr): add record rules to CR detail models (ownership + area)#261
security(cr): add record rules to CR detail models (ownership + area)#261gonzalesedwin1123 wants to merge 8 commits into
Conversation
Detail models (spp.cr.detail.*) are separate tables that do not inherit the parent spp.change.request record rules, so a low-privileged group_cr_user could read/write detail rows of change requests they do not own via RPC, tampering with pending/approved requests (e.g. re-pointing assign_program's program_id). Add per-model ir.rules mirroring the parent CR scope (users limited to own/related CRs; validators and managers unrestricted) for all 11 core detail models and the assign_program detail. Includes a completeness test asserting every concrete detail model carries a group_cr_user rule.
Post-review follow-ups: - Add a global area-filter ir.rule to every CR detail model (11 core + assign_program), mirroring the parent CR's area rule. Detail models do not inherit the parent's global rule, so an area-scoped officer could otherwise reach (via RPC) detail rows of change requests whose registrant is outside their center area. Global rule ANDs with the ownership rules; users without center areas are unaffected. - Strengthen the completeness test to require, per concrete detail model, that group_cr_user is scoped on read/write/create (not just read), that each higher role retains a permissive rule, and that a global area rule exists. - Add a functional test proving the area filter scopes a detail by its registrant's area, isolating the area dimension from ownership.
![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 introduces critical security record rules and area filters for various change request (CR) detail models across the spp_change_request_v2 and spp_cr_type_assign_program modules. Since detail models are stored in separate tables and do not inherit parent CR rules, these additions prevent low-privileged users from bypassing ownership restrictions via RPC. Comprehensive regression tests are also added to validate these security scopes. The review feedback points out two important issues in the new tests: a bug where clear_cache() is called instead of the correct Odoo registry method clear_caches(), and a potential test fragility issue where the test should only assert rules on models where the target user group actually has ACL access to avoid false positives when other modules are installed.
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 #261 +/- ##
==========================================
+ Coverage 74.28% 75.66% +1.38%
==========================================
Files 372 1080 +708
Lines 25385 64851 +39466
==========================================
+ Hits 18857 49069 +30212
- Misses 6528 15782 +9254
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…s + changelog Record the CR detail-model record-rule hardening (spp_change_request_v2 19.0.2.0.8, spp_cr_type_assign_program 19.0.1.0.2) in versions and HISTORY changelogs (regenerated READMEs).
84b07d2 to
18b46b8
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.
…able models The post_install sweep asserted group_cr_user scoping on every spp.cr.detail.* model; on a full stack that includes other apps' detail models (farmer registry, studio) gated by their own group models, which cr_user cannot reach via ACL at all. Skip those — their ownership scoping is a tracked, separate follow-up.
…-flow redesign The completeness sweep caught 10 spp.cr.detail.* models shipped by the group/membership redesign (19.0.3.0.0) without record rules: the create_group phone/bank/id_doc/member_existing/member_new lines, change_hoh.member, split_household.member, and 3 wizard models. Add ownership + area rules for the 7 regular models, traversing every alternative parent path (phone/bank/id_doc rows can hang off create_group, add_member, split_household, or a member_new row). The transient wizard models need no ir.rule — the ORM restricts transient records to their creator — so the sweep now skips _transient models.
|
The out-of-scope follow-up noted in the description (farmer-registry & studio CR detail models, which use different group models) is now tracked as #320. |
1 participant
Summary
CR detail models (
spp.cr.detail.*) are separate tables inheriting an abstract base (spp.cr.detail.base), so the parentspp.change.requestrecord rules do not cascade to them. Each detail ships an ACL grantinggroup_cr_userread/write/create but noir.rule— so a low-privileged CR user can, via direct RPC (call_kw, bypassing the UI), read/write detail rows of change requests they do not own, and tamper with pending/approved requests (the reported case: re-pointingassign_program'sprogram_idto enroll a beneficiary into an unauthorized program).Reported against
spp.cr.detail.assign_program; validated as systemic across the CR framework — all 11 core detail models inspp_change_request_v2share the identical gap.Fix
Per-model
ir.rules mirroring the parent CR's row-level security, for all 18 core detail models +assign_program(11 original + 7 line/child models shipped by the group/membership CR redesign,19.0.3.0.0— see below):group_cr_userscoped tochange_request_id.create_uid = user.id OR change_request_id.registrant_id in user.partner_id.ids(r/w/c);group_cr_validator/_validator_hq/_managerget[(1,=,1)](manager keeps unlink). The four-rule mirror is required because the higher roles implygroup_cr_user, so a permissive rule is needed to avoid caging them (Odoo ORs group-rules across a user's groups).change_request_id.registrant_id.area_id child_of user.center_area_ids) so an area-scoped officer can't reach out-of-area details; global roles (no center areas) are unaffected.Detail creation/apply already run under
.sudo()in the framework, so the new rules don't block legitimate flows — only the un-sudo'd RPC attack path.Tests
spp_cr_type_assign_program: reproduces the exact reported attack — a secondcr_usercannot read or re-pointprogram_idon another user's detail (AccessError); owner + validator retain access.spp_change_request_v2: cross-user read/write isolation onedit_individual; a functional area-filter test (isolating the area dimension from ownership); and a completeness test asserting every concretespp.cr.detail.*model is scoped on read/write/create forcr_user, retains permissive rules for higher roles, and carries a global area rule — guarding against a future detail model shipping without rules.spp_change_request_v2340/340 ·spp_cr_type_assign_program23/23. Lint-clean.Redesign fallout (caught by the completeness test)
Merging the
19.0.3.0.0base brought in the group/membership CR redesign (#242), which shipped 10 newspp.cr.detail.*models without record rules — the completeness test failed in CI exactly as designed. Added ownership + area rules for the 7 regular models (change_hoh.member,split_household.member,create_group.phone/bank/id_doc/member_existing/member_new); thephone/bank/id_docrows can hang off several alternative parents (create_group,add_member,split_household, or amember_newrow), so their domains traverse every parent path. The 3 transient wizard models need noir.rule— the ORM restricts transient records to their creator — and the completeness test now skips_transientmodels. The sweep also now only asserts on modelsgroup_cr_usercan reach via ACL, so full-stack DBs containing the deferred farmer/studio detail models (different group models, nocr_*ACLs) don't fail on a role that cannot touch them.Reviewed
Independent adversarial staff review: APPROVE WITH NITS, no blockers. Confirmed the
mail.threadside-channel is closed (message/follower read gates on document read), coverage is 1:1 with no misses, and no legitimate flow is over-restricted. Both nits addressed here (strengthened completeness test; area-filter mirror folded in).Out of scope — tracked as a separate follow-up
Two modules with the same missing-rule class but different group models needing per-module ownership analysis (not a copy of the CR rule):
spp_farmer_registry_cr(4 farm detail models,registry_officer) andspp_studio_change_requests(spp.cr.detail.generic,studio_editor).