From 270eabdc6923e7b1149c8da1b5270de627de5ab4 Mon Sep 17 00:00:00 2001 From: Ken Lewerentz Date: Wed, 26 Aug 2026 15:07:23 +0700 Subject: [PATCH 1/4] fix(change_request): run auto-apply through the public action_apply MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Requiring change-request manager rights to apply meant auto-apply-on- approve was routed to the internal mechanism instead, so that the approver could be a validator rather than a manager. But action_apply is the extension point modules override to hang post-apply work off an apply, and bypassing it left those overrides silently not running on approval — no error, just missing side effects, which is the hardest kind of regression to notice. Auto-apply now calls action_apply under sudo(), which the manager gate already exempts (env.su), so the authorization boundary is unchanged for RPC callers. sudo() sets su without changing uid, so applied_by_id still records the real approver — asserted by a test. The two guarantees from the original change are kept and still covered: a validator cannot call action_apply directly, and approving still applies. --- spp_change_request_v2/README.rst | 14 +++++ spp_change_request_v2/__manifest__.py | 2 +- .../models/change_request.py | 14 +++-- spp_change_request_v2/readme/HISTORY.md | 4 ++ .../static/description/index.html | 53 ++++++++++++------- .../tests/test_apply_authorization.py | 35 ++++++++++++ 6 files changed, 98 insertions(+), 24 deletions(-) diff --git a/spp_change_request_v2/README.rst b/spp_change_request_v2/README.rst index 4ee4aba4..0202d406 100644 --- a/spp_change_request_v2/README.rst +++ b/spp_change_request_v2/README.rst @@ -853,6 +853,20 @@ Before declaring a new CR type complete: Changelog ========= +19.0.3.1.12 +~~~~~~~~~~~ + +- fix(change_request): auto-apply-on-approve runs through the public + ``action_apply`` again. Requiring change-request manager rights to + apply meant auto-apply was routed to the internal mechanism instead, + so the approver could be a validator — but ``action_apply`` is the + extension point modules override to hang post-apply work off an apply, + and bypassing it left those overrides silently not running on + approval: no error, just missing side effects. Auto-apply now calls + ``action_apply`` under ``sudo()``, which the manager gate already + exempts. ``sudo()`` sets superuser mode without changing the user, so + the applying user is still recorded as the approver. + 19.0.3.1.10 ~~~~~~~~~~~ diff --git a/spp_change_request_v2/__manifest__.py b/spp_change_request_v2/__manifest__.py index 1f10e1b9..d3d91b88 100644 --- a/spp_change_request_v2/__manifest__.py +++ b/spp_change_request_v2/__manifest__.py @@ -1,6 +1,6 @@ { "name": "OpenSPP Change Request V2", - "version": "19.0.3.1.10", + "version": "19.0.3.1.12", "sequence": 50, "category": "OpenSPP", "summary": "Configuration-driven change request system with UX improvements, conflict detection and duplicate prevention", diff --git a/spp_change_request_v2/models/change_request.py b/spp_change_request_v2/models/change_request.py index 6a703e54..9ff44b40 100644 --- a/spp_change_request_v2/models/change_request.py +++ b/spp_change_request_v2/models/change_request.py @@ -1093,10 +1093,16 @@ def _on_approve(self): self._create_log("approved") if self.request_type_id.auto_apply_on_approve: # Auto-apply is authorized by the approval workflow itself, so it - # goes through the internal mechanism rather than the manager-gated - # public action_apply (the approver may be a validator, not a - # manager). - self._apply_change_request() + # runs with sudo: ``action_apply``'s manager gate exempts + # ``env.su``, which lets the approver be a validator rather than a + # manager. Going through the public entry point rather than the + # internal mechanism keeps ``action_apply`` the single extension + # point for apply -- downstream modules override it to hang + # post-apply work off the apply, and routing around it left those + # overrides silently not running on approval. ``sudo()`` sets + # ``su`` without changing ``uid``, so ``applied_by_id`` still + # records the real approver. + self.sudo().action_apply() def _on_reject(self, reason): super()._on_reject(reason) diff --git a/spp_change_request_v2/readme/HISTORY.md b/spp_change_request_v2/readme/HISTORY.md index 3ffd3b06..86e91f46 100644 --- a/spp_change_request_v2/readme/HISTORY.md +++ b/spp_change_request_v2/readme/HISTORY.md @@ -1,3 +1,7 @@ +### 19.0.3.1.12 + +- fix(change_request): auto-apply-on-approve runs through the public `action_apply` again. Requiring change-request manager rights to apply meant auto-apply was routed to the internal mechanism instead, so the approver could be a validator — but `action_apply` is the extension point modules override to hang post-apply work off an apply, and bypassing it left those overrides silently not running on approval: no error, just missing side effects. Auto-apply now calls `action_apply` under `sudo()`, which the manager gate already exempts. `sudo()` sets superuser mode without changing the user, so the applying user is still recorded as the approver. + ### 19.0.3.1.10 - fix(security): conflict and duplicate detection now decide whether a mapped field changed using the same comparison the apply strategy uses. Detection compared through a helper that lowercases and strips strings while apply compares raw, so a case- or whitespace-only edit was invisible to detection yet still written to the registrant — enough to sidestep a field-scoped conflict rule with a cosmetic edit. Detection also ignored transform expressions, which apply evaluates before comparing. Similarity scoring is unchanged and stays case-insensitive, since that is the point of a fuzzy match. diff --git a/spp_change_request_v2/static/description/index.html b/spp_change_request_v2/static/description/index.html index 22feeaea..e66745b1 100644 --- a/spp_change_request_v2/static/description/index.html +++ b/spp_change_request_v2/static/description/index.html @@ -1339,6 +1339,21 @@

Changelog

+

19.0.3.1.12

+ +
+

19.0.3.1.10

-
+

19.0.3.1.9

  • fix(security): duplicate detection now scores the fields both change @@ -1395,7 +1410,7 @@

    19.0.3.1.9

    requester-writable selected_field_name / field_to_modify.
-
+

19.0.3.1.8

  • fix(security): scope the Create-Group member wizards to the parent @@ -1413,7 +1428,7 @@

    19.0.3.1.8

    access-control entry grants.
-
+

19.0.3.1.7

  • fix(security): require change-request manager rights to apply a change @@ -1428,7 +1443,7 @@

    19.0.3.1.7

    endpoint.
-
+

19.0.3.1.6

  • fix(security): derive conflict and duplicate detection from the change @@ -1442,7 +1457,7 @@

    19.0.3.1.6

    an empty one, so detection cannot silently disable itself.
-
+

19.0.3.1.5

  • fix(security): scope the CR Requestor, Local Validator and HQ @@ -1454,7 +1469,7 @@

    19.0.3.1.5

    are noupdate.
-
+

19.0.3.1.4

  • fix(security): add ownership and area record rules to every concrete @@ -1471,7 +1486,7 @@

    19.0.3.1.4

    unrestricted delete their access-control entries grant.
-
+

19.0.3.1.3

  • fix(security): route and apply the same single field for @@ -1484,7 +1499,7 @@

    19.0.3.1.3

    the routing selector.
-
+

19.0.3.1.2

  • fix(change_request_v2): adding an ID now looks for a live one of that @@ -1493,7 +1508,7 @@

    19.0.3.1.2

    (#1136)
-
+

19.0.3.1.1

  • fix(change_request): enforce the (cr_type_id, reason) uniqueness @@ -1507,7 +1522,7 @@

    19.0.3.1.1

    applied) so the constraint applies cleanly on upgrade.
-
+

19.0.3.1.0

  • revert(change_request): restore the create-a-new-individual Add @@ -1525,7 +1540,7 @@

    19.0.3.1.0

    not restored here; reinstate separately if needed.
-
+

19.0.3.0.0

  • feat(change_request): redesign the group/membership CR flows (#242) — @@ -1547,7 +1562,7 @@

    19.0.3.0.0

    must adapt (see #1133).
-
+

19.0.2.0.8

  • fix(views): disable inline creation of CR document types on the Change @@ -1558,7 +1573,7 @@

    19.0.2.0.8

    Documents” modal (missing Name field) that blocked saving (#1125)
-
+

19.0.2.0.7

  • fix(security): align CR Requestor / CR Local Validator / CR HQ @@ -1570,7 +1585,7 @@

    19.0.2.0.7

    dependencies.
-
+

19.0.2.0.6

  • fix(views): route post-submit CRs (pending / approved / applied / @@ -1585,7 +1600,7 @@

    19.0.2.0.6

    list so row-click goes through the stage router.
-
+

19.0.2.0.5

  • fix(security): add a global ir.rule on spp.change.request that @@ -1598,27 +1613,27 @@

    19.0.2.0.5

    roles).
-
+

19.0.2.0.3

  • fix: add HTML escaping to all computed Html fields with sanitize=False to prevent stored XSS (#50)
-
+

19.0.2.0.2

  • fix: fix batch approval wizard line deletion (#130)
-
+

19.0.2.0.1

  • fix: skip field types before getattr and isolate detail prefetch (#129)
-
+

19.0.2.0.0

  • Initial migration to OpenSPP2
    • diff --git a/spp_change_request_v2/tests/test_apply_authorization.py b/spp_change_request_v2/tests/test_apply_authorization.py index 33f16dcd..2a960ccf 100644 --- a/spp_change_request_v2/tests/test_apply_authorization.py +++ b/spp_change_request_v2/tests/test_apply_authorization.py @@ -99,6 +99,41 @@ def test_manager_can_apply(self): self.assertTrue(cr.is_applied) self.assertTrue(self.membership.ended_date) + def test_auto_apply_runs_the_public_apply_entry_point(self): + """``action_apply`` is the extension point downstream modules override. + + Auto-apply used to bypass it, so any override hung off apply silently + stopped running on approval -- no error, just missing side effects. + """ + from unittest.mock import patch + + cr = self._make_approved_cr() + cr.request_type_id.auto_apply_on_approve = True + + seen = [] + original = type(cr).action_apply + + def spy(records): + seen.append(tuple(records.ids)) + return original(records) + + with patch.object(type(cr), "action_apply", spy): + cr.with_user(self.cr_validator)._on_approve() + + self.assertTrue(seen, "auto-apply must go through the public action_apply") + self.assertTrue(cr.is_applied) + + def test_auto_apply_records_the_real_approver(self): + """sudo() sets su without changing uid, so attribution is preserved.""" + cr = self._make_approved_cr() + cr.request_type_id.auto_apply_on_approve = True + cr.with_user(self.cr_validator)._on_approve() + self.assertEqual( + cr.applied_by_id, + self.cr_validator, + "applying under sudo must still record the approver, not the superuser", + ) + def test_auto_apply_on_approve_runs_for_non_manager_approver(self): """Auto-apply-on-approve must still work when the approver is a validator (not a manager): _on_approve routes through the internal From d4def25b964368ca3ca7e3c8eeda8d7914becc18 Mon Sep 17 00:00:00 2001 From: Ken Lewerentz Date: Wed, 26 Aug 2026 15:25:13 +0700 Subject: [PATCH 2/4] chore: annotate the intentional sudo on the auto-apply call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rationale is already in the comment above it — the manager gate exempts env.su, and sudo() preserves uid so attribution is unaffected — but semgrep's odoo-sudo-without-context needs the marker to see it. --- spp_change_request_v2/models/change_request.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spp_change_request_v2/models/change_request.py b/spp_change_request_v2/models/change_request.py index 9ff44b40..7dd3033a 100644 --- a/spp_change_request_v2/models/change_request.py +++ b/spp_change_request_v2/models/change_request.py @@ -1102,7 +1102,7 @@ def _on_approve(self): # overrides silently not running on approval. ``sudo()`` sets # ``su`` without changing ``uid``, so ``applied_by_id`` still # records the real approver. - self.sudo().action_apply() + self.sudo().action_apply() # nosemgrep: odoo-sudo-without-context def _on_reject(self, reason): super()._on_reject(reason) From 35d410362bbf2fb6294d8c7bf49ba9f2b1516989 Mon Sep 17 00:00:00 2001 From: Ken Lewerentz Date: Fri, 28 Aug 2026 09:36:35 +0700 Subject: [PATCH 3/4] docs: correct the apply authorization docstrings for the new path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two explanations were left describing the behaviour this branch replaced, both flagged in review. action_apply's docstring still said auto-apply-on-approve "invokes _apply_change_request directly". It now arrives at action_apply as a superuser caller instead, and since that paragraph documents the authorization boundary it has to describe what actually happens. Also notes that action_apply is the extension point, so an override runs under su on the auto-apply path and as the clicking manager on the manual one, with applied_by_id recording the approver either way. The docstring on test_auto_apply_on_approve_runs_for_non_manager_approver said _on_approve routes through the ungated internal mechanism. The assertion it makes is unchanged and still right — a validator approver must work — but the reason is now that the manager gate exempts superuser callers. Documentation only; no behaviour change. --- spp_change_request_v2/models/change_request.py | 13 ++++++++++--- .../tests/test_apply_authorization.py | 5 +++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/spp_change_request_v2/models/change_request.py b/spp_change_request_v2/models/change_request.py index 7dd3033a..81d9e481 100644 --- a/spp_change_request_v2/models/change_request.py +++ b/spp_change_request_v2/models/change_request.py @@ -1454,9 +1454,16 @@ def action_apply(self): roles cannot (e.g. ``spp.group.membership``), so it must be gated server-side to managers: the XML button ``groups=`` is NOT an authorization boundary because Odoo object methods are callable over - RPC. Superuser (sudo) callers and the auto-apply-on-approve path (which - invokes ``_apply_change_request`` directly, already authorized by the - approval workflow) are unaffected. + RPC. Superuser (``env.su``) callers are exempt, and + auto-apply-on-approve is one of them: ``_on_approve`` reaches this + method through ``sudo()``, already authorized by the approval workflow + itself, so the approver may be a validator rather than a manager. + + This is also the extension point for apply, so an override runs on both + paths -- under ``su`` when auto-applied on approval, and as the manager + who clicked Apply on the manual path. ``sudo()`` sets ``su`` without + changing ``uid``, so ``self.env.user`` is the approver either way and + ``applied_by_id`` records them, not the superuser. """ if not (self.env.su or self.env.user.has_group("spp_change_request_v2.group_cr_manager")): raise AccessError(_("Only Change Request managers can apply change requests.")) diff --git a/spp_change_request_v2/tests/test_apply_authorization.py b/spp_change_request_v2/tests/test_apply_authorization.py index 2a960ccf..c4a700a0 100644 --- a/spp_change_request_v2/tests/test_apply_authorization.py +++ b/spp_change_request_v2/tests/test_apply_authorization.py @@ -136,8 +136,9 @@ def test_auto_apply_records_the_real_approver(self): def test_auto_apply_on_approve_runs_for_non_manager_approver(self): """Auto-apply-on-approve must still work when the approver is a - validator (not a manager): _on_approve routes through the internal - apply mechanism, which is not gated.""" + validator (not a manager): ``_on_approve`` reaches the public + ``action_apply`` through ``sudo()``, and the manager gate exempts + superuser callers.""" cr = self._make_approved_cr() cr.request_type_id.auto_apply_on_approve = True cr.with_user(self.cr_validator)._on_approve() From 5a5bc4791ab4cccf7d3ad67af3ccd12f94d7c1c0 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 28 Aug 2026 12:22:44 +0800 Subject: [PATCH 4/4] chore(change_request): align generated README with CI's generator output The local oca-gen-addon-readme rendering differs from CI's pinned env in RST table column widths. Applied CI's printed expected diff verbatim; both files now match CI's blob hashes (99b63c9, c8d57bc). --- spp_change_request_v2/README.rst | 32 +++++++++---------- .../static/description/index.html | 4 +-- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/spp_change_request_v2/README.rst b/spp_change_request_v2/README.rst index 6787cb5e..99b63c96 100644 --- a/spp_change_request_v2/README.rst +++ b/spp_change_request_v2/README.rst @@ -752,22 +752,22 @@ Methods available for override on detail models (all inherited from Related fields available on all detail models (from ``spp.cr.detail.base``): -+----------------------------+-----------+------------------------------------------------------------+ -| Field | Type | Source | -+============================+===========+============================================================+ -| ``change_request_id`` | Many2one | Direct link to parent CR | -+----------------------------+-----------+------------------------------------------------------------+ -| ``registrant_id`` | Many2one | ``change_request_id.registrant_id`` | -+----------------------------+-----------+------------------------------------------------------------+ -| ``approval_state`` | Selection | ``change_request_id.approval_state`` | -+----------------------------+-----------+------------------------------------------------------------+ -| ``is_applied`` | Boolean | ``change_request_id.is_applied`` | -+----------------------------+-----------+------------------------------------------------------------+ -| ``use_dynamic_approval`` | Boolean | ``change_request_id.request_type_id.use_dynamic_approval`` | -+----------------------------+-----------+------------------------------------------------------------+ -| ``field_to_modify`` | Selection | Dynamic field selector (populated by | -| | | ``_get_field_to_modify_selection``) | -+----------------------------+-----------+------------------------------------------------------------+ ++--------------------------+-----------+------------------------------------------------------------+ +| Field | Type | Source | ++==========================+===========+============================================================+ +| ``change_request_id`` | Many2one | Direct link to parent CR | ++--------------------------+-----------+------------------------------------------------------------+ +| ``registrant_id`` | Many2one | ``change_request_id.registrant_id`` | ++--------------------------+-----------+------------------------------------------------------------+ +| ``approval_state`` | Selection | ``change_request_id.approval_state`` | ++--------------------------+-----------+------------------------------------------------------------+ +| ``is_applied`` | Boolean | ``change_request_id.is_applied`` | ++--------------------------+-----------+------------------------------------------------------------+ +| ``use_dynamic_approval`` | Boolean | ``change_request_id.request_type_id.use_dynamic_approval`` | ++--------------------------+-----------+------------------------------------------------------------+ +| ``field_to_modify`` | Selection | Dynamic field selector (populated by | +| | | ``_get_field_to_modify_selection``) | ++--------------------------+-----------+------------------------------------------------------------+ CR Type Fields Reference ~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/spp_change_request_v2/static/description/index.html b/spp_change_request_v2/static/description/index.html index 2bca1f55..c8d57bc4 100644 --- a/spp_change_request_v2/static/description/index.html +++ b/spp_change_request_v2/static/description/index.html @@ -1160,9 +1160,9 @@

    Methods Reference

    spp.cr.detail.base):

    -+-+
    Field