fix(security): let Tier-3 registry read reach the registrant form's models - #454
Open
kneckinator wants to merge 2 commits into
Open
fix(security): let Tier-3 registry read reach the registrant form's models#454kneckinator wants to merge 2 commits into
kneckinator wants to merge 2 commits into
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 76.28% 76.84% +0.56%
==========================================
Files 654 672 +18
Lines 44035 43564 -471
==========================================
- Hits 33592 33477 -115
+ Misses 10443 10087 -356
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.
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.
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.3, skipping2.3.1and2.3.2which PR #422 claims. 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.A sweep test asserting the roles lose no unlisted ACL belongs with
spp_change_request_v2/tests/test_cr_roles_registry_scope.py, which security(roles): scope program/CR roles to Tier-3 registry read (drop registry-search menu) #353 introduces and which does not exist on19.0. It should be added to security: batch 2 — CR + programs chain (#264, #261, #338, #336, #353, #343, #365) #422 once this merges, or in a small follow-up after both.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.