security(roles): scope program/CR roles to Tier-3 registry read (drop registry-search menu)#353
Draft
gonzalesedwin1123 wants to merge 4 commits into
Draft
security(roles): scope program/CR roles to Tier-3 registry read (drop registry-search menu)#353gonzalesedwin1123 wants to merge 4 commits into
gonzalesedwin1123 wants to merge 4 commits into
Conversation
Add tests asserting Program Viewer and the CR Requestor/Local/HQ Validator roles do NOT confer spp_registry.group_registry_viewer (which gates the Registry Search portal menu) but DO keep registrant read (res.partner, spp.registry.id, spp.phone.number) via Tier-3 group_registry_read.
… viewer menu Program Viewer, CR Requestor, CR Local Validator and CR HQ Validator implied spp_registry.group_registry_viewer (Tier-2), which gates the standalone Registry Search portal menu and exposed a broad registrant-PII enumeration surface to these program/CR-scoped roles. Switch them to Tier-3 group_registry_read, which grants the same registrant read ACLs (defined in spp_base_common; also provided to CR roles via the group_cr_* chain) without the Registry app menu. user_roles.xml is noupdate=1 and the roles ship in the 2026.07 release, so add post-migrations (spp_programs 19.0.2.2.2, spp_change_request_v2 19.0.3.0.3) that unlink the viewer group, link the read group, and re-sync already-assigned users via action_update_users. Reduces the registry-search UI surface; underlying RPC read scope is unchanged (governed by the area filter rule_res_partner_area_filter).
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #353 +/- ##
==========================================
- Coverage 74.28% 72.52% -1.77%
==========================================
Files 372 253 -119
Lines 25385 16405 -8980
==========================================
- Hits 18857 11897 -6960
+ Misses 6528 4508 -2020 Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…y-viewer scoping README.rst / index.html regenerated from CI's pinned oca-gen output for spp_programs (19.0.2.2.2) and spp_change_request_v2 (19.0.3.0.3); test files reformatted to CI's ruff-format line length (local hook env differed).
Member
Author
|
Staff-review residual filed as follow-up: #355 — |
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
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.
Problem
Role links added in the OP#951 menu audit gave several program/CR-scoped roles the Tier-2
spp_registry.group_registry_viewergroup:spp_programs: Program Viewerspp_change_request_v2: CR Requestor, CR Local Validator, CR HQ Validatorgroup_registry_viewergates the standalone Registry Search portal menu(
spp_registry_search.menu_registry_search) — a broad registrant-PII enumeration surface (name, IDnumber, phone, email) — on top of the registrant read ACLs. So a read-only "Program Viewer" (and the
CR roles) got the full registry browse/search UI, not just the registrant data needed for the
program/CR records they work with.
Important nuance (report framing corrected): OpenSPP does not scope the registry per-program.
Row-level need-to-know is area-based (
spp_arearule_res_partner_area_filter, keyed onuser.center_area_ids, populated from local role assignments); it is a no-op for global roles, whichsee the whole registry by design.
group_registry_restrict_selfscopes to the user's own partner(self-service), not per-program. So this change does not (and cannot, without a new feature) make these
roles see "only their program's beneficiaries" — it removes the Registry Search UI surface while
leaving the underlying RPC read governed by the area filter. Severity: medium (over-broad PII
enumeration UI for program/CR roles).
Fix
Switch the four roles from Tier-2
group_registry_viewerto Tier-3group_registry_read:group_registry_readgrants the same registrant read ACLs onres.partner,spp.registry.id,spp.phone.number(defined inspp_base_common) — so all registrantcross-reference reads keep working — but it does not gate the Registry app menu.
group_cr_*chain, so read is doubly preserved.spp.cycle/spp.cycle.membership/spp.program.membershipread for Program Viewer comes fromgroup_programs_viewer, independent of the registry group — unaffected.this reason.
Migration:
user_roles.xmlisnoupdate="1"and the roles ship in the2026.07release, so aplain upgrade would keep the old link. Added
post-migration.pyfor each module(
spp_programs/migrations/19.0.2.2.2,spp_change_request_v2/migrations/19.0.3.0.3) that unlinks theviewer group, links the read group, and re-materializes already-assigned users' group membership via
res.users.role.action_update_users().Tests
Test-first (red → green). New tests in each module assert that each role, once assigned to a fresh user
and synced, does not carry
group_registry_viewer(would fail before the fix) but does keepgroup_registry_readand can functionally read a registrant'sres.partner/spp.registry.id/spp.phone.number(and Program Viewer keepsgroup_programs_viewer)../spp t spp_programs→ 0 failed of 680;./spp t spp_change_request_v2→ 0 failed of 336.No existing tests removed or modified.
Scope / notes
app), Cycle Approver (already Tier-3).
new cross-cutting feature, not a security patch — noted as a separate design question, not included.
spp_programs.2.2dodges security(programs): enforce system-admin authorization on Force Unlock #336's.2.1;spp_change_request_v2.0.3dodges security(cr): route and apply the same single field for dynamic approvals #264.0.1/ security(cr): add record rules to CR detail models (ownership + area) #261.0.2. Whoever merges among {security(cr): add record rules to CR detail models (ownership + area) #261, security(cr): route and apply the same single field for dynamic approvals #264,security(programs): enforce system-admin authorization on Force Unlock #336, this} later re-conflicts on manifest/HISTORY/README (trivial re-merge). No logic conflict.
README.rst/static/description/index.htmlfor both modules to be regenerated from CI's pinned generator.