fix(starter_sp_mis): make registry access control persist and enforce - #411
fix(starter_sp_mis): make registry access control persist and enforce#411emjay0921 wants to merge 4 commits into
Conversation
The "Restrict Registry Edits to Admin Only" setting could not be turned
off, and did not restrict anything when on.
Odoo stores a False config_parameter by deleting the row, and default_get
falls back to the field default when the row is missing, so default=True
made "off" unrepresentable: the toggle sprang back on at every reload.
The enforcement side read a missing row as False, so unticking the box in
fact disabled the restriction while the form went on claiming the registry
was locked. Persist the value explicitly as a string and take the install
default from the data file, which is now noupdate so an upgrade stops
re-locking a registry an administrator deliberately opened.
Enforcement was client-side only: JavaScript hid buttons with display:none
and nothing on the server refused the write, so RPC and import went
straight through. The list "New" button was never hidden either, because
the patch assigned a canCreate property that Odoo 19's ListController does
not read - its template gates on activeActions.create.
Replace all of it with a _check_access override on res.partner. That is
the single chokepoint behind check_access, has_access and the ORM's own
create/write/unlink guards, so one override refuses the change on every
path and removes New/Edit/Delete from registry views for free:
ir.ui.view._postprocess_access_rights stamps create="false" onto an arch
whenever has_access('create') is False. Scoped to registrants so the
setting cannot lock the Contacts app.
The demo access-control tests assert the plain role model and were only
passing because the restriction did nothing, so they now pin the switch
off explicitly.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #411 +/- ##
==========================================
+ Coverage 71.49% 71.52% +0.03%
==========================================
Files 243 242 -1
Lines 20785 20820 +35
==========================================
+ Hits 14860 14892 +32
- Misses 5925 5928 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
CI's semgrep flags odoo-sudo-without-context on all three sudo() calls. Each is deliberate, so record why next to it: - writing the config parameter is a Settings-manager operation, and the settings form is already gated on that group; - reading it is how the guard decides whether to withhold access, so every user has to be able to read it; - filtering on is_registrant as the acting user would recurse straight back into the access check being evaluated. The pragma has to sit on the line immediately above the match, not at the head of the comment block, or semgrep does not associate the two. Also applies ruff's preferred spacing on a docstring that opens with a quoted word.
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
Thanks — the design here is right, and the root-cause analysis in the PR body is accurate. I verified the load-bearing claims against the Odoo 19 source: _check_access is the single chokepoint the docstring says it is, create() re-runs check_access('create') on the populated records (odoo/orm/models.py:4948), so a bare RPC create of a registrant with no registry context really is refused; both registry actions carry default_is_registrant, so the arch gating works; and the ListController/activeActions.create point about the old JS is correct. The test suite is thorough.
Two blockers, one major gap, one coordination item, and some nits.
1. Blocker — the noupdate move has no effect on any existing database
Verified in odoo/addons/base/models/ir_model.py (_build_update_xmlids_query): the xml_id upsert is ON CONFLICT … DO UPDATE SET (model, res_id, write_date) — the stored noupdate flag of an existing xml_id is never updated on upgrade. Every database that already has spp_starter_sp_mis.config_registry_admin_only_crud keeps noupdate = False, so each module upgrade re-applies value=True — silently re-locking a registry the administrator deliberately opened. That is the exact re-lock bug this PR sets out to fix; as written, the fix only takes effect on fresh installs, and the reviewer note "recreated as True once, then stays where it is put" doesn't hold for existing DBs.
Ask: add a migration that sets ir_model_data.noupdate = true for ('spp_starter_sp_mis', 'config_registry_admin_only_crud').
2. Blocker — no version bump
spp_starter_sp_mis stays at 19.0.2.0.0, and the module shipped in release 2026.08. The asset removal, deleted controller, new model file and data-file change all need an upgrade to take effect — without a bump, deployments keep referencing the deleted JS asset and never get the enforcement. This folds naturally into the migration from #1.
3. Major — two-step bypass: create a contact, then flag it as a registrant
is_registrant is a plain Boolean with no field-level groups (spp_registry/models/registrant.py:36), and write() checks access against the record's current values only — there is no post-write pass. So with the restriction on, a non-admin can:
create({'name': X})— allowed, plain contact;write({'is_registrant': True, …})— allowed, because_check_access('write')filters on the current value (False) — and the partner is now a registrant.
The same move promotes any existing plain contact into the registry. Not a regression (the old JS enforced nothing), but it defeats the PR's stated purpose over RPC/import.
Ask: guard the flag flip — e.g. a small write override refusing a truthy is_registrant in vals for restricted users. (Unflagging is already blocked, since the record is a registrant at check time.)
4. Coordination — open PR #299 modifies the file this PR deletes
#299 (feat/registry-restriction-context-bypass) adds a per-action context opt-out to registry_restriction.js, which this PR removes. If this lands first, #299 is obsolete and the opt-out has no server-side equivalent — and note that honoring a context key inside _check_access would not be a safe port, since context is client-supplied on RPC and would let any caller waive the restriction. Whatever replaces the per-action opt-out (an exemption group, or simply running with the setting off) should be decided before one of these merges; flagging so the two PRs don't land past each other.
5. Nits
readme/DESCRIPTION.md(and the generatedREADME.rst) still document the deleted mechanism: "JavaScript-based restriction", "PatchesFormControllerandListController", and the removed/spp_starter_sp_mis/registry_restrictionendpoint. Worth rewriting for the server-side model — as it stands the docs advertise client-side enforcement.- The empty-recordset probe refuses any create in a context carrying
default_is_registrant, even if the vals wouldn't create a registrant (a wizard inheriting registry context but creating a plain contact would be refused early, where the post-create pass alone would have allowed it). Marginal — a code comment accepting this explicitly would do. - Confirming the intended behaviour change extends to
spp_mis_demo_v2: it ships the restriction on, so demo registrar walkthroughs lose registrant editing after this PR.
…ion bypass Marking the config parameter noupdate only governs xml_ids created from then on: _build_update_xmlids_query upserts with DO UPDATE SET (model, res_id, write_date) and never rewrites an existing row's noupdate flag. Every database that already carries this xml_id therefore kept re-applying value=True on each upgrade — silently re-locking a registry an administrator had deliberately opened, which is the bug the change set out to fix. A migration flips the flag on the row itself, and a test asserts it is set. The access check filters on a record's current values and there is no post-write pass, so a restricted user could create a plain contact and then set is_registrant on it — two allowed steps adding up to a registrant they were never allowed to create, and the same move promotes any existing contact. write() now refuses that flip while the restriction is on. Unflagging needs no guard: the record is already a registrant when the check runs. Version bump with its changelog entry. This one is load-bearing rather than convention: without an upgrade a deployment keeps referencing the deleted JS asset and never gains the server-side enforcement. DESCRIPTION.md described the mechanism this branch removes — a JavaScript patch, a JSON-RPC endpoint and a MutationObserver — so it advertised client-side enforcement that no longer exists. Rewritten for what the module now does.
|
Thanks — the two blockers and the major gap are fixed, the nits are done, and the coordination item needs a decision that is not mine alone. Pushed as 1. The 2. Version bump — 3. The two-step bypass — closed. 4. Coordination with #299 — flagging rather than deciding. I agree the context-key opt-out cannot be ported into 5. Nits:
|
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
Approved — both blockers and the bypass are fixed correctly, and the fixes carry their own regression pins.
What I verified on 9a3a9a3e:
- The noupdate migration is exactly right. Literal SQL (semgrep-safe per the repo convention), idempotent (
AND noupdate IS NOT TRUE), fresh-install guarded, and aimed at the correct row — the xml_id name matches what yesterday's check of_build_update_xmlids_queryestablished would otherwise never be rewritten. The accompanying test asserting the row's flag is a nice touch: on fresh databases it pins the data-file declaration, on upgraded ones the migration satisfies it — one assertion guarding both paths. - The promotion bypass is closed with the correct scoping. The
write()guard fires only on a truthyis_registrantin vals, only for restricted users, and only for records currently outside the registry — and the reasoning that unflagging needs no guard (the record is already a registrant when_check_accessruns) is proven by the test keeping that direction refused. Promote-when-off and admin-exempt are both covered. - Version bump is load-bearing here, as your commit message says: without the upgrade a deployment keeps referencing the deleted JS asset and never gains the enforcement.
19.0.2.1.0+ HISTORY entry present. - The docs now describe the module that exists — DESCRIPTION/README rewritten off the JavaScript-patch mechanism.
CI fully green.
One gate before merge, and it sits with Edwin rather than with this diff: the fate of #299's per-action opt-out. Merging this PR deletes the file #299 modifies, and a context-key bypass cannot be ported into _check_access safely (context is client-supplied on RPC). The replacement — an exemption group, or the affected deployment running with the setting off — needs deciding before this lands so the two PRs don't merge past each other. That decision is tracked on our side; nothing further is needed from you on this branch.
3 participants
Why is this change needed?
OP#1142 — SPMIS settings unable to toggle off. Test pass.
The "Restrict Registry Edits to Admin Only" setting could not be turned off, and did not restrict anything when it was on.
Odoo stores a False
config_parameterby deleting the row, anddefault_getfalls back to the field default when the row is missing — sodefault=Truemade "off" unrepresentable and the toggle sprang back on at every reload. Worse, the enforcement side read a missing row as False, so unticking the box in fact disabled the restriction while the form went on claiming the registry was locked.Enforcement was client-side only: JavaScript hid buttons with
display: noneand nothing on the server refused the write, so RPC and import went straight through. The list "New" button was never hidden either, because the patch assigned acanCreateproperty that Odoo 19's ListController does not read — its template gates onactiveActions.create.How was the change implemented?
set_valueswrites the value explicitly as a string, so "off" is a stored fact rather than an absence. The install default moved todata/config_parameters.xml, nownoupdateso an upgrade stops silently re-locking a registry an administrator deliberately opened._check_accessoverride onres.partner. That is the single chokepoint behindcheck_access,has_accessand the ORM's own create/write/unlink guards, so one override refuses the change on every path and removes New/Edit/Delete from registry views for free:ir.ui.view._postprocess_access_rightsstampscreate="false"onto an arch wheneverhas_access('create')is False.registry_restriction.js(260 lines) and the controller it called are deleted — the DOM hiding is no longer needed and never worked on lists anyway.New unit tests
14, in
spp_starter_sp_mis/tests/test_registry_restriction.py, covering the toggle round-trip in both directions, that the form and the enforcement can never disagree, that non-admins are refused create/write/unlink on registrants, that admins are exempt, that plain contacts are unaffected, and that the registry arch dropscreatefor a restricted user.Unit tests executed by the author
Re-run after merging
19.0up:spp_mis_demo_v2is the only dependent module. Four of its access-control tests asserted the plain role model and were only passing because the restriction did nothing; they now pin the switch off explicitly, since they measure ACLs rather than this setting../spp lintclean.How to test manually
Related links
https://openspp.openproject.com/work_packages/1142
Reviewer notes
This is a real behaviour change for the SP-MIS bundle. The setting ships enabled and now actually works, so registrars lose registrant create/edit/delete by default where the restriction was previously decorative.
One residual on upgrade: databases where an administrator had already unticked the setting have no parameter row, because the old code deleted it. The
noupdaterecord is recreated as True once, then stays where it is put.