From c7b8c7b89dcb0811dfe6026760fbd12148881f1e Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Mon, 27 Jul 2026 11:41:12 +0800 Subject: [PATCH 1/5] test(spp_change_request_v2): reproduce cr_user applying CR over RPC via sudo path A group_cr_user, on their OWN approved remove_member CR (owning it so the ownership record rule doesn't mask the check), can call action_apply() over RPC and drive the sudoed strategy to end membership. Test asserts AccessError + untouched membership (fails before the fix: 'AccessError not raised'), that a manager can apply, and that validator-driven auto-apply-on-approve still applies. --- spp_change_request_v2/tests/__init__.py | 1 + .../tests/test_apply_authorization.py | 103 ++++++++++++++++++ 2 files changed, 104 insertions(+) create mode 100644 spp_change_request_v2/tests/test_apply_authorization.py diff --git a/spp_change_request_v2/tests/__init__.py b/spp_change_request_v2/tests/__init__.py index bc75e9cc0..a644295f3 100644 --- a/spp_change_request_v2/tests/__init__.py +++ b/spp_change_request_v2/tests/__init__.py @@ -25,3 +25,4 @@ from . import test_conflict_dynamic_approval from . import test_html_escaping from . import test_wizard_html_escaping +from . import test_apply_authorization diff --git a/spp_change_request_v2/tests/test_apply_authorization.py b/spp_change_request_v2/tests/test_apply_authorization.py new file mode 100644 index 000000000..7a912b317 --- /dev/null +++ b/spp_change_request_v2/tests/test_apply_authorization.py @@ -0,0 +1,103 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Server-side authorization for applying change requests. + +``action_apply()`` sudoes the apply strategy (which writes ``spp.group.membership`` +as superuser, bypassing the ACLs that make membership read-only for CR roles). +The manager restriction used to live only on the XML button, but Odoo object +methods are RPC-callable, so a plain ``group_cr_user`` could invoke +``action_apply()`` directly on an approved CR and drive superuser membership +writes. The public entrypoint must enforce ``group_cr_manager`` server-side, +while the internal apply mechanism (used by auto-apply-on-approve, which runs +as the approving validator) stays reachable. +""" + +from odoo import Command, fields +from odoo.exceptions import AccessError +from odoo.tests import TransactionCase + +from .common import get_or_create_cr_type + + +class TestApplyAuthorization(TransactionCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + P = cls.env["res.partner"] + cls.membership_model = cls.env["spp.group.membership"] + cls.cr_model = cls.env["spp.change.request"] + + cls.group = P.create({"name": "Authz Household", "is_registrant": True, "is_group": True}) + cls.member = P.create({"name": "Authz Member", "is_registrant": True, "is_group": False}) + cls.membership = cls.membership_model.create( + {"group": cls.group.id, "individual": cls.member.id, "start_date": fields.Datetime.now()} + ) + cls.cr_type = get_or_create_cr_type(cls.env, "remove_member") + + base_user = cls.env.ref("base.group_user") + + def _user(login, group_xmlid): + return cls.env["res.users"].create( + { + "name": login, + "login": login, + "email": f"{login}@example.com", + "group_ids": [ + Command.link(base_user.id), + Command.link(cls.env.ref(group_xmlid).id), + ], + } + ) + + cls.cr_user = _user("authz_cr_user", "spp_change_request_v2.group_cr_user") + cls.cr_manager = _user("authz_cr_manager", "spp_change_request_v2.group_cr_manager") + cls.cr_validator = _user("authz_cr_validator", "spp_change_request_v2.group_cr_validator") + + def _make_approved_cr(self, owner=None): + """Create an approved remove_member CR. + + ``owner`` sets create_uid so the CR passes the cr_user ownership record + rule (``rule_cr_user``) — otherwise a non-owning cr_user is blocked by + that rule (a read AccessError) and the apply-authorization gate under + test would never be reached. Approval is stamped via sudo to simulate a + CR already approved through the workflow. + """ + cr_model = self.cr_model.with_user(owner) if owner else self.cr_model + cr = cr_model.create({"request_type_id": self.cr_type.id, "registrant_id": self.group.id}) + cr.get_detail().write( + { + "individual_id": self.member.id, + "membership_id": self.membership.id, + "end_reason": "left_household", + } + ) + cr.sudo().approval_state = "approved" + return cr + + def test_cr_user_cannot_apply_over_rpc(self): + """A non-manager cr_user calling action_apply directly on their OWN + approved CR must be denied by the server-side manager gate, and no + membership write must occur.""" + cr = self._make_approved_cr(owner=self.cr_user) + with self.assertRaises(AccessError): + cr.with_user(self.cr_user).action_apply() + self.assertFalse(cr.is_applied) + self.assertFalse( + self.membership.ended_date, "membership must be untouched when apply is denied" + ) + + def test_manager_can_apply(self): + """A cr_manager may apply (regression).""" + cr = self._make_approved_cr() + cr.with_user(self.cr_manager).action_apply() + self.assertTrue(cr.is_applied) + self.assertTrue(self.membership.ended_date) + + 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.""" + cr = self._make_approved_cr() + cr.request_type_id.auto_apply_on_approve = True + cr.with_user(self.cr_validator)._on_approve() + self.assertTrue(cr.is_applied) + self.assertTrue(self.membership.ended_date) From 78a8c621e25270b8de3dcd2f6406640fc5cd53f5 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Mon, 27 Jul 2026 11:41:12 +0800 Subject: [PATCH 2/5] security(spp_change_request_v2): enforce manager authorization on CR apply action_apply() runs the apply strategy under sudo (writing spp.group.membership etc. that CR roles cannot), but the group_cr_manager restriction existed only on the review button. Odoo object methods are RPC-callable, so a group_cr_user could invoke action_apply() directly on an approved CR (one they own) and drive superuser membership writes via strategies like remove_member/transfer_member. Gate the public action_apply() with a server-side check (env.su or group_cr_manager) and move the apply mechanism to an internal, non-RPC _apply_change_request(). Auto-apply-on-approve (_on_approve) now calls the internal mechanism directly so validator-driven approvals still apply, and sudo/demo callers are unaffected. --- .../models/change_request.py | 80 +++++++++++++------ 1 file changed, 54 insertions(+), 26 deletions(-) diff --git a/spp_change_request_v2/models/change_request.py b/spp_change_request_v2/models/change_request.py index 565185bad..87ea06966 100644 --- a/spp_change_request_v2/models/change_request.py +++ b/spp_change_request_v2/models/change_request.py @@ -3,7 +3,7 @@ from markupsafe import escape as html_escape from odoo import _, api, fields, models -from odoo.exceptions import UserError, ValidationError +from odoo.exceptions import AccessError, UserError, ValidationError _logger = logging.getLogger(__name__) @@ -1020,7 +1020,11 @@ def _on_approve(self): self._create_audit_event("approved", "pending", "approved") self._create_log("approved") if self.request_type_id.auto_apply_on_approve: - self.action_apply() + # 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() def _on_reject(self, reason): super()._on_reject(reason) @@ -1365,32 +1369,56 @@ def _capture_preview_snapshot(self): self.preview_json_snapshot = json.dumps(changes, indent=2, default=str) def action_apply(self): - """Apply the change request to the registrant.""" + """Apply the change request(s) to the registrant. + + Public entrypoint (review button / RPC). Applying runs the apply + strategy under sudo (see ``_do_apply``), which can write models CR + 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. + """ + 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.")) for rec in self: - if rec.is_applied: - raise UserError(_("Changes have already been applied.")) - if rec.approval_state != "approved": - raise UserError(_("Change request must be approved first.")) + rec._apply_change_request() - try: - # Capture preview snapshot before applying - rec._capture_preview_snapshot() - - rec._do_apply() - rec.write( - { - "is_applied": True, - "applied_date": fields.Datetime.now(), - "applied_by_id": self.env.user.id, - "apply_error": False, - } - ) - rec._create_audit_event("applied", "approved", "applied") - rec._create_log("applied") - except Exception as e: - _logger.exception("Failed to apply change request %s", rec.name) - rec.write({"apply_error": str(e)}) - raise + def _apply_change_request(self): + """Apply a single approved change request (no authorization gate). + + Internal mechanism shared by ``action_apply`` (manager-gated public + entrypoint) and auto-apply-on-approve (``_on_approve``, already + authorized by the approval workflow). Underscore-prefixed so it is not + callable over RPC — the authorization boundary lives on + ``action_apply``. + """ + self.ensure_one() + if self.is_applied: + raise UserError(_("Changes have already been applied.")) + if self.approval_state != "approved": + raise UserError(_("Change request must be approved first.")) + + try: + # Capture preview snapshot before applying + self._capture_preview_snapshot() + + self._do_apply() + self.write( + { + "is_applied": True, + "applied_date": fields.Datetime.now(), + "applied_by_id": self.env.user.id, + "apply_error": False, + } + ) + self._create_audit_event("applied", "approved", "applied") + self._create_log("applied") + except Exception as e: + _logger.exception("Failed to apply change request %s", self.name) + self.write({"apply_error": str(e)}) + raise def _do_apply(self): """Execute the apply strategy. From 37aabe0ec6a42f41ad47a9ab6d6a16c70f0036cc Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Mon, 27 Jul 2026 11:41:12 +0800 Subject: [PATCH 3/5] docs(spp_change_request_v2): version bump + HISTORY for CR apply authz fix 19.0.3.0.0 -> 19.0.3.0.4 (dodges #264 .0.1 / #261 .0.2 / #353 .0.3 on this module). README.rst / index.html to be regenerated from CI. --- spp_change_request_v2/__manifest__.py | 2 +- spp_change_request_v2/readme/HISTORY.md | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/spp_change_request_v2/__manifest__.py b/spp_change_request_v2/__manifest__.py index 1e32f323f..5616eb71d 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.0.0", + "version": "19.0.3.0.4", "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/readme/HISTORY.md b/spp_change_request_v2/readme/HISTORY.md index 0f9d181e5..ee3ed6021 100644 --- a/spp_change_request_v2/readme/HISTORY.md +++ b/spp_change_request_v2/readme/HISTORY.md @@ -1,3 +1,17 @@ +### 19.0.3.0.4 + +- fix(security): enforce manager authorization server-side when applying a + change request. ``action_apply()`` runs the apply strategy under sudo (which + can write ``spp.group.membership`` and other models CR roles cannot), and the + manager restriction previously existed only on the review button — but Odoo + object methods are callable over RPC, so a ``group_cr_user`` could invoke + ``action_apply()`` directly on an approved change request and drive superuser + membership writes (e.g. remove/transfer member). The public ``action_apply()`` + now requires ``group_cr_manager`` (or a superuser/sudo context); the apply + mechanism was moved to an internal, non-RPC ``_apply_change_request()`` that + auto-apply-on-approve continues to use so validator-driven approvals still + apply. + ### 19.0.3.0.0 - feat(change_request): redesign the group/membership CR flows (#242) — Create Group (#876), Add Member now searches an existing member (#871), Remove Member first-page/review cleanup (#872), Change Head of Household via a per-member role table (#873), and Split Household as a relational member move with single-head validation (#877). Review pages render the real data as tables / detail sections. From a7b2ca3e8c62d366348036743162bbe9075a42d4 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Mon, 27 Jul 2026 12:08:55 +0800 Subject: [PATCH 4/5] docs(spp_change_request_v2): regenerate README + apply CI ruff-format for CR apply authz README.rst / index.html regenerated from CI's pinned oca-gen output (19.0.3.0.4); test file reflowed to CI's ruff-format line length (local hook env differed). --- spp_change_request_v2/README.rst | 16 +++++++++ .../static/description/index.html | 33 ++++++++++++++----- .../tests/test_apply_authorization.py | 4 +-- 3 files changed, 42 insertions(+), 11 deletions(-) diff --git a/spp_change_request_v2/README.rst b/spp_change_request_v2/README.rst index 914b1a0e5..c66f77ccc 100644 --- a/spp_change_request_v2/README.rst +++ b/spp_change_request_v2/README.rst @@ -853,6 +853,22 @@ Before declaring a new CR type complete: Changelog ========= +19.0.3.0.4 +~~~~~~~~~~ + +- fix(security): enforce manager authorization server-side when applying + a change request. ``action_apply()`` runs the apply strategy under + sudo (which can write ``spp.group.membership`` and other models CR + roles cannot), and the manager restriction previously existed only on + the review button — but Odoo object methods are callable over RPC, so + a ``group_cr_user`` could invoke ``action_apply()`` directly on an + approved change request and drive superuser membership writes (e.g. + remove/transfer member). The public ``action_apply()`` now requires + ``group_cr_manager`` (or a superuser/sudo context); the apply + mechanism was moved to an internal, non-RPC + ``_apply_change_request()`` that auto-apply-on-approve continues to + use so validator-driven approvals still apply. + 19.0.3.0.0 ~~~~~~~~~~ diff --git a/spp_change_request_v2/static/description/index.html b/spp_change_request_v2/static/description/index.html index 58f8d5751..9f25b0014 100644 --- a/spp_change_request_v2/static/description/index.html +++ b/spp_change_request_v2/static/description/index.html @@ -1339,6 +1339,23 @@

Changelog

+

19.0.3.0.4

+
    +
  • fix(security): enforce manager authorization server-side when applying +a change request. action_apply() runs the apply strategy under +sudo (which can write spp.group.membership and other models CR +roles cannot), and the manager restriction previously existed only on +the review button — but Odoo object methods are callable over RPC, so +a group_cr_user could invoke action_apply() directly on an +approved change request and drive superuser membership writes (e.g. +remove/transfer member). The public action_apply() now requires +group_cr_manager (or a superuser/sudo context); the apply +mechanism was moved to an internal, non-RPC +_apply_change_request() that auto-apply-on-approve continues to +use so validator-driven approvals still apply.
    • +
+
+

19.0.3.0.0

  • feat(change_request): redesign the group/membership CR flows (#242) — @@ -1360,7 +1377,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 @@ -1371,7 +1388,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 @@ -1383,7 +1400,7 @@

    19.0.2.0.7

    dependencies.
-
+

19.0.2.0.6

  • fix(views): route post-submit CRs (pending / approved / applied / @@ -1398,7 +1415,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 @@ -1411,27 +1428,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 7a912b317..956fe0807 100644 --- a/spp_change_request_v2/tests/test_apply_authorization.py +++ b/spp_change_request_v2/tests/test_apply_authorization.py @@ -81,9 +81,7 @@ def test_cr_user_cannot_apply_over_rpc(self): with self.assertRaises(AccessError): cr.with_user(self.cr_user).action_apply() self.assertFalse(cr.is_applied) - self.assertFalse( - self.membership.ended_date, "membership must be untouched when apply is denied" - ) + self.assertFalse(self.membership.ended_date, "membership must be untouched when apply is denied") def test_manager_can_apply(self): """A cr_manager may apply (regression).""" From 6c3eea1429d134b96389ab2d963d06fcc321d79e Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Mon, 27 Jul 2026 13:53:35 +0800 Subject: [PATCH 5/5] test(spp_change_request_v2): assert validators are also blocked from direct apply MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From staff review: document the manager-only boundary explicitly — a group_cr_validator (not a manager) calling action_apply directly is denied; validators cause an apply only by approving (auto-apply). --- spp_change_request_v2/tests/test_apply_authorization.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/spp_change_request_v2/tests/test_apply_authorization.py b/spp_change_request_v2/tests/test_apply_authorization.py index 956fe0807..33f16dcd6 100644 --- a/spp_change_request_v2/tests/test_apply_authorization.py +++ b/spp_change_request_v2/tests/test_apply_authorization.py @@ -83,6 +83,15 @@ def test_cr_user_cannot_apply_over_rpc(self): self.assertFalse(cr.is_applied) self.assertFalse(self.membership.ended_date, "membership must be untouched when apply is denied") + def test_validator_cannot_apply_directly(self): + """A validator (not a manager) is also blocked from calling action_apply + directly. Validators cause an apply only by approving (auto-apply via + _on_approve), not by invoking the manager-only public entrypoint.""" + cr = self._make_approved_cr() + with self.assertRaises(AccessError): + cr.with_user(self.cr_validator).action_apply() + self.assertFalse(cr.is_applied) + def test_manager_can_apply(self): """A cr_manager may apply (regression).""" cr = self._make_approved_cr()