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 2 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.
| # ir.rule evaluates and caches its domain per (model, mode); the earlier | ||
| # unrestricted read cached an empty domain, so drop the cache to pick up | ||
| # the new center-area scope (a real role change invalidates this too). | ||
| self.env.registry.clear_cache() |
There was a problem hiding this comment.
The method clear_cache() does not exist on the Odoo registry object. The correct method name is clear_caches(). Using clear_cache() will raise an AttributeError and cause the test to fail.
| self.env.registry.clear_cache() | |
| self.env.registry.clear_caches() |
| for model in models: | ||
| if self.env[model.model]._abstract: | ||
| continue | ||
| rules = Rule.search([("model_id", "=", model.id)]) |
There was a problem hiding this comment.
The test searches for all concrete spp.cr.detail.* models and asserts that they have record rules for group_cr_user. However, if other modules (such as spp_farmer_registry_cr or spp_studio_change_requests) are installed in the same test database, their detail models will also be found but they won't have rules for group_cr_user (since they use different group models). This will cause the test to fail.
To make the test robust and avoid false positives, we should only check models where group_cr_user actually has ACL access.
for model in models:
if self.env[model.model]._abstract:
continue
# Only check models where group_cr_user has ACL access
if not self.env["ir.model.access"].search_count([
("model_id", "=", model.id),
("group_id", "=", self.user_group.id),
]):
continue
rules = Rule.search([("model_id", "=", model.id)])
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #261 +/- ##
==========================================
+ Coverage 75.29% 75.32% +0.03%
==========================================
Files 1094 1095 +1
Lines 64997 65433 +436
==========================================
+ Hits 48940 49288 +348
- Misses 16057 16145 +88
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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 11 core detail models +assign_program: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_v2304/304 ·spp_cr_type_assign_program23/23. Lint-clean.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).