fix(security): let Tier-3 registry read reach the registrant form's models - #454
Conversation
…odels 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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #454 +/- ##
==========================================
+ Coverage 75.93% 76.94% +1.00%
==========================================
Files 627 673 +46
Lines 43000 43704 +704
==========================================
+ Hits 32654 33627 +973
+ Misses 10346 10077 -269
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
19.0 released spp_programs 19.0.2.3.0 (deduplication, #445), above the 19.0.2.2.4 this branch claimed. Left alone the manifest would regress and the new Tier-3 access-control entries would never be applied to a database already upgraded to 2.3.0. This branch's spp_programs entry moves to 19.0.2.3.3, above the 19.0.2.3.1 / 19.0.2.3.2 that the security batch-2 branch (#422) takes, so the two stack cleanly with #422 merging first. The access-control file conflict is a both-sides append: 19.0's deduplication wizard entries and this branch's Tier-3 registry-read entries are both kept. The two new entries are also renamed from access_spp_cycle_registry_registry_read / access_spp_cycle_membership_registry_registry_read, which doubled a word through a generation slip; nothing references the old identifiers and neither has shipped.
Absorbs the security batch-2 merge (#422), which took spp_programs to 19.0.2.3.3. This branch keeps 19.0.2.3.4, so no renumbering was needed — the version stack held. All conflicts are metadata: the manifest version, both sides of the changelog, both sides of the test registrations, and the two generated README files.
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.
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
Reviewed with independent verification: I re-ran the completeness sweep over every ir.model.access.csv (viewer-granted minus Tier-3-granted minus base.group_user-granted) and got exactly your residual list; confirmed the res.partner.bank reasoning live (base grants it at odoo/addons/base/security/ir.model.access.csv:77, verified by probe with a base.group_user + group_registry_read user); confirmed the bite property locally (removing the irrigation ACL row makes exactly the new test fail); spp_irrigation 33/33 locally, CI green across the board. The version chain is correct now that #422 has landed (2.0.2 / 2.1.1 / 2.3.4), and the spp_change_request_v2 README restore leaves zero residue. The core analysis is sound and the per-model live-DB methodology paid off.
Requesting changes on two points, both inline: the two consent wizard grants (an over-grant with a concrete UI regression), and the irrigation ACL xmlid (naming convention plus an incorrect rationale in user-facing release notes).
Observation (non-blocking): spp.program read for the CR roles rides on an accidental grant
spp.program has no registry-tier ACL row at all, and the registrant form renders program_id in the ungated program_membership_ids list (spp_programs/views/registrant_view.xml:90-103). A bare Tier-3 user (base.group_user + group_registry_read only, i.e. exactly this PR's test-user construction) still hits an AccessError opening an enrolled registrant. The shipped roles are saved by side grants: Program Viewer via group_programs_viewer, and the three CR roles via spp_hazard.group_hazard_viewer -> spp_hazard_programs's access_spp_program_hazard_viewer (auto_install on {spp_hazard, spp_programs}, and spp_change_request_v2 hard-depends on spp_hazard, so it is always present wherever the program list can render). That coupling is accidental and fragile: a future security pass dropping group_hazard_viewer from the CR roles would break their registrant form. Not a #353 regression (the viewer tier never had spp.program either), so not blocking here, but consider adding access_spp_program_registry_read,Program Registry Read,spp_programs.model_spp_program,spp_registry.group_registry_read,1,0,0,0 plus "spp.program" in the test's _MODELS while you are in the file, or filing a follow-up documenting the reliance.
Suggestions (non-blocking)
- The
spp_programsHISTORY reason forspp.cycle.membershipis not quite right: the actual registrant-form dependency is_compute_latest_cycle_state(spp_programs/models/program_membership.py:99-133, non-stored, searchesspp.cycle.membershipas the acting user, rendered ungated atregistrant_view.xml:145). "Entitlement lists rendercycle_id" only justifiesspp.cycle. - Stale note in the PR body:
spp_change_request_v2/tests/test_cr_roles_registry_scope.pynow exists on19.0(#422 merged), so the no-unlisted-ACL-loss sweep test can proceed as a direct follow-up rather than waiting on #422. - Tests: consider
Command.link()over(4, id)tuples perdocs/principles/odoo19-compatibility.md(style only). A single functionalweb_read-style test on an enrolled registrant would assert the behaviour the changelog claims and would have caught thespp.programblind spot that a hand-maintained model list cannot. spp_consent/tests/__init__.py: the new import is appended aftertest_security; the file is otherwise alphabetical.docker-compose.yml:261(./e2e/reports, added by #442 after this branch point) is now the only bind mount without,z- same SELinux failure class your fix addresses; worth folding in when you merge19.0in. The:ro,zchange itself is correct, consistent with the other five mounts, and a no-op on non-SELinux hosts.
Absorbs #442 (Playwright e2e suite). Clean merge — it touches the e2e service's environment block, adjacent to but not overlapping the volume mount this branch adds `,z` to.
…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.
|
Thanks — both blocking points were right, and I verified each rather than taking them on trust. Pushed as 1. Consent wizard grants — droppedConfirmed on every limb:
Your transient point matches what came out of the batch-2 review independently: Dropped both rows and both Agreed the pre-existing Tier-2 rows and the ungated binding deserve a follow-up; 2. Irrigation xmlid — renamed, and my rationale was wrongYou're right and I was wrong. Took your first option: access_spp_irrigation_asset_registry_viewer,SPP Irrigation Asset Viewer Read,...group_registry_viewer,1,0,0,0
access_spp_irrigation_asset_registry_read,SPP Irrigation Asset Registry Read,...group_registry_read,1,0,0,0The old The names above follow this cited docs; 3.
|
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
Re-reviewed 7a46a5a3 with the same rigor as the first pass. Both blocking points are resolved exactly as requested, and the verification chain in your reply matches what I see in the tree:
- Wizard grants: both rows gone,
_MODELS9 → 7, and the changelog now records why the wizards are excluded — that last part is the piece that keeps this from being "restored" by the next parity-minded pass. The follow-up you agreed on (Tier-2 viewer rows +group_idson the binding action) stands. - Irrigation xmlids: both conventional now, and the repurposing is upgrade-safe from released 2.1.0 — the existing
_registry_readir.model.data record gets its group flipped viewer → read in place,_registry_vieweris created fresh, no xmlid disappears, and thetier3_readid never reached any database. Rationale correctly removed from the user-facing changelog. - #463 is a model split-out: right scope call (granting
spp.programis a new decision, not a restoration), and leading with the functionalweb_readtest rather than the ACL row is the correct priority — the hand-maintained-list blind spot is the disease, the missing row only a symptom. - Suggestions verified applied:
latest_cycle_statereason (now more precise than my phrasing),Command.link()in all three tests, alphabetical__init__.py, stale sweep-test note gone from the body.
One discrepancy, non-blocking since it was never a requested change: your reply and the commit message both say ./e2e/reports now takes ,z, but the head has it unchanged (docker-compose.yml:265, no ,z). Looks like that hunk didn't make it into the commit. Fine to push as a one-liner here or leave it for a follow-up touching #442's compose section — just flagging it so it isn't remembered as done.
CI is green across the board on 7a46a5a3 (including all three module suites and pre-commit, which is the authority on the regenerated READMEs). Approving.
2 participants
Restores the model read access that the Tier-2 → Tier-3 registry-role swap in #353 (security batch 2, PR #422) silently revokes.
The mechanism
spp_registry.group_registry_viewer(Tier-2) carriesimplied_ids = [group_registry_read](spp_registry/security/groups.xml:36). Tier-2 strictly contains Tier-3, so re-pointing a role from viewer to read can only lose ACLs. #353's migration states registrant read is unchanged — true for the models granted to both tiers, but several modules that extend the registrant form grant their models to the viewer tier alone. A role scoped to Tier-3 then gets anAccessErrorsimply opening a registrant.What actually breaks
Determined per model against a live database (
ir.model.access.checkas abase.group_user+group_registry_readuser), not by reading ACL files:spp_consentviews/registrant_view.xmlextends the registrant formspp_programsspp.cycle,spp.cycle.membershipviews/registrant_view.xmlrenderscycle_idin the entitlement listsspp_irrigationspp.irrigation.assetviews/irrigation_view.xmlrendersirrigation_asset_idsres.partner.bankis deliberately not included. It is viewer-only within this repo, so a repo-local ACL diff flags it — butodoo/addons/base/security/ir.model.access.csv:77already grants it tobase.group_user, so every internal user can read it and nothing is lost.spp_bankingneeds no change. Any earlier analysis naming it (including review notes on #353) is wrong on that point.Not restored, and correctly dropped — none has a registrant-form path:
spp.disable.registrant.wizard,spp.queue.background.task(utility models inspp_registry) and the threespp.dci.*models.Tests
One regression test per module asserting a Tier-3 user can read that module's registrant-form models. Each was confirmed to fail without the ACL rows and pass with them — the first draft of the
spp_bankingtest passed either way, which is what exposed theres.partner.bankfalse positive.Full suites on the merged tree:
spp_consent157,spp_programs685,spp_irrigation33 — 0 failures, 0 errors.⚠ Merge order
spp_programsgoes 19.0.2.3.0 → 19.0.2.3.4, skipping2.3.1–2.3.3which PR #422 claims (2.3.1/2.3.2for the batch fixes,2.3.3for guarding the operation lock on create as well as write). This assumes #422 merges first. If this PR needs to land first instead,spp_programsmust be renumbered here and #422 rebased onto it — the two orders cannot both be safe.19.0moved tospp_programs19.0.2.3.0 (deduplication, #445) after this branch was opened, so19.0has been merged in and the numbering re-based on it. The earlier2.2.4would have been a version regression, and — the real hazard — the new access-control entries would never have been applied to a database already upgraded to2.3.0, since Odoo only replays data for a higher manifest version. Same correction was applied to #422.Deployment-wise the dependency runs the other way: #353 should not reach production without this, or Tier-3-scoped Program Viewer / CR Requestor / CR Local Validator / CR HQ Validator roles lose the registrant form.
Notes
Two entries added here are named
access_spp_cycle_registry_read/access_spp_cycle_membership_registry_read. An earlier revision of this branch generated them with a doubled word (..._registry_registry_read); they were renamed before merge, so no identifier is orphaned.The pre-existing
access_spp_irrigation_asset_registry_readentry is misnamed — it grants the Tier-2 viewer group, not Tier-3 read. Renaming it would orphan the xmlid on upgraded databases, so the new entry usesaccess_spp_irrigation_asset_tier3_readinstead.docker-compose.ymlnow mountsdocker/postgresql.confwith:ro,z. Without the SELinux relabel flag the database container cannot read it from a fresh git worktree, breakingscripts/test_single_module.shthere.