fix(spp_area,spp_programs,spp_change_request_v2): apply area filtering for Programs and Change Requests (#989)#200
Open
emjay0921 wants to merge 7 commits into
Open
fix(spp_area,spp_programs,spp_change_request_v2): apply area filtering for Programs and Change Requests (#989)#200emjay0921 wants to merge 7 commits into
emjay0921 wants to merge 7 commits into
Conversation
…memberships and change requests Local roles (Local Registrar, CR Local Validator, etc.) carry an assigned set of areas via `res.users.role.line.local_area_ids`, which spp_area aggregates onto `res.users.center_area_ids`. spp_area's registrant override (`spp_area/models/registrant.py`) honours that on res.partner reads, so a local user only sees registrants in their assigned regions and descendants. But the same user could open Programs and bulk-modify memberships outside their region, or open Change Requests and act on CRs targeting registrants in other regions — neither model had any equivalent filter. Mirror the registrant override on: - `spp.program.membership` — filter by `partner_id.area_id` - `spp.change.request` — filter by `registrant_id.area_id` Both override `_prepare_domain` / `search_read` / `web_search_read` and use `getattr(user, 'center_area_ids', None)` so the override is a no-op when spp_area isn't loaded. Users without center areas (global roles) see everything as before; users with center areas see only records under their assigned areas (with `child_of` traversal so parent-region assignment matches all child provinces). Refs OP#989.
… full row-level area filtering (OP#989 round-2)
QA round 1 surfaced 4 gaps in the original Python _prepare_domain
overrides on spp.program.membership and spp.change.request:
1. Access error clicking Eligibility Criteria — related-field
traversal bypasses search_read.
2. Program counts wrong — search_count / read_group bypass search_read.
3. Cycle generated from program ignores area — spp.cycle.membership
had no override at all.
4. CR registrant picker shows out-of-area registrants — name_search
routes through _search, not search_read.
Root cause is identical in all four: the Python overrides only hook
search_read / web_search_read, so every other ORM read path slips
past the filter. The same gap exists on res.partner itself (spp_area's
override is structurally identical).
Fix: use ir.rule (record rules) which Odoo applies automatically to
every read path — search, search_count, read_group, name_search, read,
and related-field traversal.
- spp_area/security/rules.xml (new): rule on res.partner.
- spp_programs/security/area_filter_rules.xml (new): rules on
spp.program.membership AND spp.cycle.membership (the latter was
missing entirely).
- spp_change_request_v2/security/area_filter_rules.xml (new): rule
on spp.change.request.
All four rules are 'global' and use a conditional domain
'[(... 'child_of', user.center_area_ids.ids)] if getattr(user,
'center_area_ids', False) else []' so they no-op for users without
center_area_ids (global roles) and degrade safely if spp_area isn't
loaded.
The existing Python _prepare_domain / search_read / web_search_read
overrides are left in place as belt-and-suspenders — they're now
redundant but harmless.
…ist)
Initial round-2 push failed at module install with:
NameError: name 'getattr' is not defined
ir.rule.domain_force is evaluated by safe_eval with a restricted
builtins allowlist that does not include getattr/hasattr. Replace
the defensive 'getattr(user, "center_area_ids", False)' guard
with a plain truthiness check 'user.center_area_ids' — the field
exists in every install path now because:
- spp_area's own rule references the field on its own model.
- spp_programs already depends on spp_area.
- spp_change_request_v2 now declares spp_area in depends (added
here) — formally what was already true in any OpenSPP deployment
but previously undeclared.
…und-2 polish)
QA hit:
Failed to write field spp.program.message_partner_ids
Sorry, QA — Local Program Manager (Region IV-A) (id=23) doesn't
have 'read' access to: Contact (res.partner)
The previous rule filtered the entire res.partner model by area_id.
That breaks mail follower lookups (message_partner_ids, message_-
follower_ids), the user's own partner record, system bots, and
company partners — all of which have area_id = NULL and are
excluded by the child_of clause.
Narrow the rule to registrants only:
['|', ('is_registrant', '=', False), ('area_id', 'child_of',
user.center_area_ids.ids)]
Non-registrant contacts remain readable; the area filter only
restricts registrant rows, which is the actual OP#989 spec.
![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 implements area-based row-level filtering across the spp_area, spp_change_request_v2, and spp_programs modules by introducing global ir.rule records. These rules ensure that users with assigned center areas can only access registrants, change requests, and program or cycle memberships within their geographical scope. Feedback indicates that the Python method overrides for search_read and web_search_read in the change_request and program_membership models are now redundant and should be removed, as the new record rules provide a more robust and standard security enforcement mechanism across all ORM access paths.
Comment on lines
+1655
to
+1689
|
|
||
| # ─── Area-based filtering (OP#989) ────────────────────────────────── | ||
| # Mirrors the `_prepare_domain` pattern from | ||
| # spp_area/models/registrant.py so a user with a local role (i.e. | ||
| # `center_area_ids` set on res.users by spp_area) only sees change | ||
| # requests whose registrant is in one of their assigned areas (or | ||
| # any descendant). Users without center areas (global roles) get | ||
| # no extra filter — same as before. Without this override the CR | ||
| # list and review screens ignored center-area boundaries even | ||
| # though the registrant filter on res.partner respected them. | ||
|
|
||
| @api.model | ||
| def _prepare_domain(self, domain): | ||
| domain = domain or [] | ||
| user = self.env.user | ||
| # Guard for installs without spp_area — the field would not | ||
| # exist on res.users in that case. spp_change_request_v2 does | ||
| # not yet hard-depend on spp_area, so the guard keeps this | ||
| # override safe even when spp_area isn't loaded. | ||
| center_area_ids = getattr(user, "center_area_ids", None) | ||
| if center_area_ids: | ||
| domain = list(domain) + [ | ||
| ("registrant_id.area_id", "child_of", center_area_ids.ids), | ||
| ] | ||
| return domain | ||
|
|
||
| @api.model | ||
| def search_read(self, domain=None, fields=None, offset=0, limit=None, order=None): | ||
| domain = self._prepare_domain(domain) | ||
| return super().search_read(domain, fields, offset, limit, order) | ||
|
|
||
| @api.model | ||
| def web_search_read(self, domain, specification, offset=0, limit=None, order=None, count_limit=None): | ||
| domain = self._prepare_domain(domain) | ||
| return super().web_search_read(domain, specification, offset, limit, order, count_limit) |
There was a problem hiding this comment.
The Python overrides for search_read and web_search_read (and the _prepare_domain helper) are redundant because this PR also introduces an ir.rule in security/area_filter_rules.xml.
Record rules are the standard and more robust way to enforce row-level security in Odoo. They are applied automatically by the ORM to all access paths—including read_group, search_count, name_search, and related field traversal—which these Python overrides miss. The PR description even notes that these overrides were the root cause of filtering gaps in "Round-1". Keeping both implementations increases maintenance overhead and can lead to conflicting logic.
Comment on lines
+499
to
+533
|
|
||
| # ─── Area-based filtering (OP#989) ────────────────────────────────── | ||
| # Mirrors the `_prepare_domain` pattern from | ||
| # spp_area/models/registrant.py so a user with a local role (i.e. | ||
| # `center_area_ids` set on res.users by spp_area) only sees program | ||
| # memberships whose partner is in one of their assigned areas (or | ||
| # any descendant of those areas). Users without center areas | ||
| # (global roles) get no extra filter — they see everything as | ||
| # before. Without this override "Verify Eligibility" / "Enroll | ||
| # Eligible" / the membership list bypassed area boundaries entirely. | ||
|
|
||
| @api.model | ||
| def _prepare_domain(self, domain): | ||
| domain = domain or [] | ||
| user = self.env.user | ||
| # Guard against installs where spp_area isn't loaded — the | ||
| # field would not exist on res.users in that case. spp_programs | ||
| # already depends on spp_area today, but the guard keeps this | ||
| # override behaving sensibly if that ever changes. | ||
| center_area_ids = getattr(user, "center_area_ids", None) | ||
| if center_area_ids: | ||
| domain = list(domain) + [ | ||
| ("partner_id.area_id", "child_of", center_area_ids.ids), | ||
| ] | ||
| return domain | ||
|
|
||
| @api.model | ||
| def search_read(self, domain=None, fields=None, offset=0, limit=None, order=None): | ||
| domain = self._prepare_domain(domain) | ||
| return super().search_read(domain, fields, offset, limit, order) | ||
|
|
||
| @api.model | ||
| def web_search_read(self, domain, specification, offset=0, limit=None, order=None, count_limit=None): | ||
| domain = self._prepare_domain(domain) | ||
| return super().web_search_read(domain, specification, offset, limit, order, count_limit) |
There was a problem hiding this comment.
These Python overrides are redundant with the new ir.rule records defined in security/area_filter_rules.xml.
As noted in the PR description, record rules are preferred because they cover all ORM entry points (like dashboard counts and dropdown searches) that search_read overrides do not. To maintain code clarity and avoid duplication of security logic, these methods should be removed in favor of the record rules.
Resolves version + HISTORY conflicts in spp_programs: - bump version to 19.0.2.1.2 on top of upstream's 19.0.2.1.1 - keep both changelog entries (#943 from upstream + #989 from this branch)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #200 +/- ##
==========================================
+ Coverage 66.08% 73.41% +7.33%
==========================================
Files 86 380 +294
Lines 7501 23324 +15823
==========================================
+ Hits 4957 17124 +12167
- Misses 2544 6200 +3656
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
OCA README generator re-renders section numbering in README.rst and static/description/index.html when a new entry is added to HISTORY.md. Applies CI's regenerated bytes (Python 3.11 docutils output) directly so local toolchain mismatches don't drift the rendering.
…main overrides (OP#989) Round-2 replaced the Python search_read / web_search_read overrides with ir.rule records that filter every ORM read path (search, count, read_group, name_search, related-field traversal). The earlier Python hooks were left in place by mistake — they only caught two of those paths and are now strictly redundant given the rule does more. Drop the dead code. Behaviour is unchanged (ir.rule still scopes the same reads). Reduces the codecov/patch denominator since the removed lines were never executed under area-restricted users.
Contributor
Author
|
@gonzalesedwin1123 — ready for review. All CI checks are green and conflicts with OP#989: https://projects.acn.fr/wp/989 |
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.
Why is this change needed?
OP#989 — area-scoped operators (Local Program Manager, Local Registrar, etc.) should only see programs / cycles / change requests for registrants in their assigned area(s). Round-1 attempted this via Python
_prepare_domainoverrides onspp.program.membershipandspp.change.request, but QA found four gaps where the filter was bypassed:search_read.search_countandread_groupalso bypasssearch_read.spp.cycle.membershiphad no override.name_searchroutes through_search, notsearch_read.Root cause: the Python overrides only hook
search_read/web_search_read, so every other ORM read path slips past the filter. The same gap existed forres.partner(spp_area's override was structurally identical).How was the change implemented?
Replaced Python
_prepare_domainoverrides withir.rule(record rules), which Odoo applies automatically to every read path —search,search_count,read_group,name_search,read, and related-field traversal. New rule files:spp_area/security/rules.xml— scopesres.partnerreads to registrants whosearea_idischild_of user.center_area_ids. Bounded tois_registrant=Trueso mail-follower lookups, the user's own partner, system bots and company partners (allarea_id=NULL) remain readable.spp_programs/security/area_filter_rules.xml— applies the samearea_id child_of user.center_area_idsfilter tospp.program.membershipandspp.cycle.membership.spp_change_request_v2/security/area_filter_rules.xml— same filter onspp.change.request.Round-2 polish:
getattr()from rule domains becauseir.rule.domain_forceis evaluated bysafe_evalwith a restricted builtins allowlist (nogetattr/hasattr). Plainuser.center_area_idsworks becausespp_change_request_v2now formally declaresspp_areain itsdepends.res.partnerrule tois_registrant=Trueonly, after QA hit "Local Program Manager doesn't have 'read' access to Contact (res.partner)" — mail-follower partners (message_partner_ids), the user's own partner, system bots and company partners are allis_registrant=False, area_id=NULLand were getting locked out by the original rule.New unit tests
No new test files in this branch — area filtering is enforced by ir.rule and verified by manual QA across the four bypass paths above. (Adding ir.rule unit tests would mean per-user environment switching across multiple models; treated as a follow-up if QA wants coverage.)
Unit tests executed by the author
Manual verification on a local instance (
spp_mis_demo_v2loaded, demo Local Program Manager assigned to Region IV-A):[1=1]for admins viagroups). ✅How to test manually
See the QA round-2 comment on OP#989 for the full test guide:
https://projects.acn.fr/wp/989
Quick steps:
spp_mis_demo_v2, load demo data.demo_local_pm_iva).read_group-based counts match the filtered lists.Related links