Skip to content

security(roles): scope program/CR roles to Tier-3 registry read (drop registry-search menu) - #353

Merged
gonzalesedwin1123 merged 2 commits into
19.0-staging-sec-batch2from
security-program-cr-registry-viewer
Aug 14, 2026
Merged

security(roles): scope program/CR roles to Tier-3 registry read (drop registry-search menu)#353
gonzalesedwin1123 merged 2 commits into
19.0-staging-sec-batch2from
security-program-cr-registry-viewer

Conversation

@gonzalesedwin1123

Copy link
Copy Markdown
Member

Problem

Role links added in the OP#951 menu audit gave several program/CR-scoped roles the Tier-2
spp_registry.group_registry_viewer group:

  • spp_programs: Program Viewer
  • spp_change_request_v2: CR Requestor, CR Local Validator, CR HQ Validator

group_registry_viewer gates the standalone Registry Search portal menu
(spp_registry_search.menu_registry_search) — a broad registrant-PII enumeration surface (name, ID
number, 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_area rule_res_partner_area_filter, keyed on
user.center_area_ids, populated from local role assignments); it is a no-op for global roles, which
see the whole registry by design. group_registry_restrict_self scopes 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_viewer to Tier-3 group_registry_read:

  • Tier-3 group_registry_read grants the same registrant read ACLs on res.partner,
    spp.registry.id, spp.phone.number (defined in spp_base_common) — so all registrant
    cross-reference reads keep working — but it does not gate the Registry app menu.
  • The CR roles also imply Tier-3 read/write via their group_cr_* chain, so read is doubly preserved.
  • spp.cycle / spp.cycle.membership / spp.program.membership read for Program Viewer comes from
    group_programs_viewer, independent of the registry group — unaffected.
  • This mirrors the existing Program Cycle Approver precedent, which already uses Tier-3 for exactly
    this reason.

Migration: user_roles.xml is noupdate="1" and the roles ship in the 2026.07 release, so a
plain upgrade would keep the old link. Added post-migration.py for each module
(spp_programs/migrations/19.0.2.2.2, spp_change_request_v2/migrations/19.0.3.0.3) that unlinks the
viewer 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 keep
group_registry_read and can functionally read a registrant's res.partner/spp.registry.id/
spp.phone.number (and Program Viewer keeps group_programs_viewer).

./spp t spp_programs0 failed of 680; ./spp t spp_change_request_v20 failed of 336.
No existing tests removed or modified.

Scope / notes

@gemini-code-assist

Copy link
Copy Markdown

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.99%. Comparing base (f82a94a) to head (acdba20).

Additional details and impacted files

Impacted file tree graph

@@                     Coverage Diff                     @@
##           19.0-staging-sec-batch2     #353      +/-   ##
===========================================================
+ Coverage                    72.49%   72.99%   +0.50%     
===========================================================
  Files                          450      469      +19     
  Lines                        31136    31983     +847     
===========================================================
+ Hits                         22571    23345     +774     
- Misses                        8565     8638      +73     
Flag Coverage Δ
spp_api_v2_change_request 66.53% <ø> (ø)
spp_api_v2_cycles 71.03% <ø> (ø)
spp_api_v2_entitlements 70.23% <ø> (ø)
spp_api_v2_programs 92.22% <ø> (ø)
spp_api_v2_simulation 71.19% <ø> (ø)
spp_base_common 91.07% <ø> (ø)
spp_case_entitlements 100.00% <ø> (ø)
spp_case_programs 100.00% <ø> (ø)
spp_change_request_v2 78.02% <ø> (ø)
spp_cr_type_assign_program 92.50% <ø> (ø)
spp_dci_compliance 93.01% <ø> (?)
spp_dci_demo 94.28% <ø> (ø)
spp_dci_server_social 89.57% <ø> (?)
spp_farmer_registry_cr 61.24% <ø> (ø)
spp_programs 65.47% <ø> (+0.02%) ⬆️
spp_registry 87.22% <ø> (+0.07%) ⬆️
spp_security 69.56% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.
see 21 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@gonzalesedwin1123

Copy link
Copy Markdown
Member Author

Staff-review residual filed as follow-up: #355global_role_program_validator still carries Tier-2 group_registry_viewer (Registry Search menu). Left out of this PR's scope (it holds registry write, so the menu is more defensible); tracked for consistency with Cycle Approver and the roles fixed here.

gonzalesedwin1123 added a commit that referenced this pull request Aug 10, 2026
The changed-field derivation reads apply_mapping_ids, which only the
field_mapping apply strategy populates. For a type with dynamic approval and
a custom strategy the mapping set is empty, so the derivation returned an
empty set rather than None — which makes the field-scoped conflict filter
match nothing and drives duplicate similarity to 0, disabling both checks.
That was strictly more permissive than the selected_field_name path it
replaces, which is set regardless of strategy.

No shipped CR type enables dynamic approval today, so this was latent rather
than live, but it is reachable by configuration. Now returns None (use the
full configured field set) unless the type is field_mapping with mappings
present, with a regression test covering the custom-strategy case.

Renumbered to 19.0.3.1.5: 19.0 carries 19.0.3.1.1, so the previous
19.0.3.0.3 would have been a downgrade (and collided with #353).
@gonzalesedwin1123
gonzalesedwin1123 force-pushed the security-program-cr-registry-viewer branch from 5f743ea to 2845af9 Compare August 10, 2026 06:13
@gonzalesedwin1123
gonzalesedwin1123 changed the base branch from 19.0 to 19.0-staging-sec-batch2 August 14, 2026 09:58
… registry-search menu (#353)

Squashed net change of the #353 branch for the batch-2 staging rebase.

Program Viewer and the CR requestor/validator/validator-HQ roles were granted
spp_registry.group_registry_viewer (Tier-2), which exposes the Registry Search
portal and app root. Narrow them to group_registry_read (Tier-3): registrant
read is preserved, the registry-search menu subtree is pruned. Migrations on
both spp_change_request_v2 (19.0.3.1.4) and spp_programs (19.0.2.2.2) revoke the
viewer group from already-assigned users on upgrade via action_update_users().
@gonzalesedwin1123
gonzalesedwin1123 force-pushed the security-program-cr-registry-viewer branch from f228b96 to e1ac2d4 Compare August 14, 2026 13:32
Applied verbatim from CI's pinned oca-gen output (run 31805166557).
@gonzalesedwin1123
gonzalesedwin1123 marked this pull request as ready for review August 14, 2026 13:51
@gonzalesedwin1123
gonzalesedwin1123 merged commit c9acdc3 into 19.0-staging-sec-batch2 Aug 14, 2026
35 checks passed
@gonzalesedwin1123
gonzalesedwin1123 deleted the security-program-cr-registry-viewer branch August 14, 2026 13:52
gonzalesedwin1123 added a commit that referenced this pull request Aug 20, 2026
Second drift back-merge (#392, #436, #438 landed on 19.0). Conflicts
confined to spp_programs metadata: 19.0's #436 took 19.0.2.2.1, which the
batch chain had assigned to #336 (with #353 at .2 carrying a migration).
Resolution renumbers the batch chain one notch up: manifest -> 19.0.2.2.3,
HISTORY ladder 2.2.3 = #353, 2.2.2 = #336, 2.2.1 = #436, and
migrations/19.0.2.2.2/ renamed to 19.0.2.2.3/ so the dir matches the
version claiming #353's change in the merged changelog (either name runs
for real upgrade paths; only test DBs ever saw it as 2.2.2).
README.rst/index.html regenerated via the pinned oca-gen hook.
program_manager.py auto-merged; batch 2 (lock helpers) and #436
(protected-state filtering) touch disjoint functions.
kneckinator added a commit that referenced this pull request Aug 25, 2026
Resolve the spp_change_request_v2 version collision: 19.0 shipped
19.0.3.1.2 (#437) after this branch claimed the same number for #264.
19.0's entry keeps 3.1.2; the batch-2 chain shifts up one to 3.1.3-3.1.7,
and the #353 migration directory moves to 19.0.3.1.5 to match.
kneckinator added a commit that referenced this pull request Aug 25, 2026
19.0 released spp_programs 19.0.2.3.0 (deduplication, #445), which lands
above the 2.2.2 / 2.2.3 this branch claimed. Left alone the manifest
would regress to 2.2.3 and — worse — the #353 role-repointing migration
in migrations/19.0.2.2.3 would never run on a database already upgraded
to 2.3.0, since Odoo only runs scripts where installed < script <=
target.

The batch's two spp_programs fixes move above 2.3.0: #336 to 19.0.2.3.1
and #353 to 19.0.2.3.2, with the migration directory renamed to match.
19.0's 2.3.0 entry keeps its number and its code is untouched.
kneckinator added a commit that referenced this pull request Aug 26, 2026
security: batch 2 — CR + programs chain (#264, #261, #338, #336, #353, #343, #365)
kneckinator added a commit that referenced this pull request Aug 27, 2026
…mlids

Two corrections from review.

The two consent wizard grants are dropped. Neither wizard is a
registrant-form dependency by this branch's own criterion — both Record
Consent entry points are gated to the officer and manager tiers, and
read without create cannot open a wizard anyway — so the grant buys a
Tier-3 user nothing. It is not inert elsewhere: the bulk wizard's action
binds to res.partner with no group_ids, and get_bindings filters only on
read access to the action's model, so the row would surface "Bulk Record
Consent" in the registrant list for read-only users who then fail on
click. Odoo also enforces no per-user isolation on transient models, so a
model-level read grant exposes other users' in-flight wizard rows.
Restoring pre-#353 parity here would have restored a bug.

The irrigation ACL identifiers now follow the documented
access_{model}_{group} convention: the misnamed pre-existing row becomes
..._registry_viewer, and the new Tier-3 row takes ..._registry_read. The
claim that renaming would orphan the old xmlid was wrong — CSV rows load
with noupdate false, and _process_end removes exactly those identifiers
when a reloaded module no longer declares them — so the rationale is
dropped from the changelog rather than restated.

Also from review: the spp.cycle.membership reason now names the real
dependency (latest_cycle_state, computed by searching that model as the
acting user) rather than cycle_id; the consent changelog records why the
wizards are excluded; Command.link() replaces (4, id) tuples per the
Odoo 19 compatibility guidance; spp_consent/tests/__init__.py is
alphabetical again; and ./e2e/reports takes ,z, the last bind mount
without it.
gonzalesedwin1123 pushed a commit that referenced this pull request Aug 27, 2026
…odels (#454)

* fix(security): let Tier-3 registry read reach the registrant form's models

spp_registry.group_registry_viewer (Tier-2) implies group_registry_read
(Tier-3), so any model granted only to the viewer tier becomes
unreadable for a role scoped to the read tier. Several modules that
extend the registrant form grant their models to the viewer tier alone,
so such a role gets an AccessError merely opening a registrant.

Verified per model against a live database rather than by reading ACL
files: spp_consent's nine models, spp_programs' spp.cycle and
spp.cycle.membership (the entitlement lists render cycle_id) and
spp_irrigation's spp.irrigation.asset are genuinely unreachable.
res.partner.bank is not, despite being viewer-only here — Odoo core
already grants it to base.group_user — so spp_banking needs no change.

spp_programs skips 19.0.2.2.2 and .2.2.3, which the security batch-2
branch claims; this assumes that branch merges first.

Also mounts docker/postgresql.conf with :ro,z. Without the SELinux
relabel flag the database container cannot read it from a fresh git
worktree, which breaks scripts/test_single_module.sh there.

* chore: move spp_programs to 19.0.2.3.4

The security batch-2 branch (#422) now also takes 19.0.2.3.3, for
guarding the operation lock on create as well as write, so this branch
moves up one to stay above it. Still assumes #422 merges first.

* chore: restore spp_change_request_v2 README to the generator's output

Regenerating the README fragments after merging 19.0 rewrote every
module, not just the one this branch touches, and the local renderer
lays RST table columns out one character wider than CI does. Only
spp_programs was corrected, so an unrelated spp_change_request_v2 diff
rode along. Reverted to 19.0's version — this branch does not touch that
module.

* fix(security): address review — drop the wizard grants, fix the ACL xmlids

Two corrections from review.

The two consent wizard grants are dropped. Neither wizard is a
registrant-form dependency by this branch's own criterion — both Record
Consent entry points are gated to the officer and manager tiers, and
read without create cannot open a wizard anyway — so the grant buys a
Tier-3 user nothing. It is not inert elsewhere: the bulk wizard's action
binds to res.partner with no group_ids, and get_bindings filters only on
read access to the action's model, so the row would surface "Bulk Record
Consent" in the registrant list for read-only users who then fail on
click. Odoo also enforces no per-user isolation on transient models, so a
model-level read grant exposes other users' in-flight wizard rows.
Restoring pre-#353 parity here would have restored a bug.

The irrigation ACL identifiers now follow the documented
access_{model}_{group} convention: the misnamed pre-existing row becomes
..._registry_viewer, and the new Tier-3 row takes ..._registry_read. The
claim that renaming would orphan the old xmlid was wrong — CSV rows load
with noupdate false, and _process_end removes exactly those identifiers
when a reloaded module no longer declares them — so the rationale is
dropped from the changelog rather than restated.

Also from review: the spp.cycle.membership reason now names the real
dependency (latest_cycle_state, computed by searching that model as the
acting user) rather than cycle_id; the consent changelog records why the
wizards are excluded; Command.link() replaces (4, id) tuples per the
Odoo 19 compatibility guidance; spp_consent/tests/__init__.py is
alphabetical again; and ./e2e/reports takes ,z, the last bind mount
without it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant