security(spp_change_request_v2): enforce manager authorization on CR apply (server-side)

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
 ~~~~~~~~~~
 

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",

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.

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.

spp_change_request_v2/static/description/index.html

@@ -1339,6 +1339,23 @@ <h2>Changelog</h2>
 </div>
 </div>
 <div class="section" id="section-1">
+<h1>19.0.3.0.4</h1>
+<ul class="simple">
+<li>fix(security): enforce manager authorization server-side when applying
+a change request. <tt class="docutils literal">action_apply()</tt> runs the apply strategy under
+sudo (which can write <tt class="docutils literal">spp.group.membership</tt> 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 <tt class="docutils literal">group_cr_user</tt> could invoke <tt class="docutils literal">action_apply()</tt> directly on an
+approved change request and drive superuser membership writes (e.g.
+remove/transfer member). The public <tt class="docutils literal">action_apply()</tt> now requires
+<tt class="docutils literal">group_cr_manager</tt> (or a superuser/sudo context); the apply
+mechanism was moved to an internal, non-RPC
+<tt class="docutils literal">_apply_change_request()</tt> that auto-apply-on-approve continues to
+use so validator-driven approvals still apply.</li>
+</ul>
+</div>
+<div class="section" id="section-2">
 <h1>19.0.3.0.0</h1>
 <ul class="simple">
 <li>feat(change_request): redesign the group/membership CR flows (#242) —
@@ -1360,7 +1377,7 @@ <h1>19.0.3.0.0</h1>
 must adapt (see #1133).</li>
 </ul>
 </div>
-<div class="section" id="section-2">
+<div class="section" id="section-3">
 <h1>19.0.2.0.8</h1>
 <ul class="simple">
 <li>fix(views): disable inline creation of CR document types on the Change
@@ -1371,7 +1388,7 @@ <h1>19.0.2.0.8</h1>
 Documents” modal (missing Name field) that blocked saving (#1125)</li>
 </ul>
 </div>
-<div class="section" id="section-3">
+<div class="section" id="section-4">
 <h1>19.0.2.0.7</h1>
 <ul class="simple">
 <li>fix(security): align CR Requestor / CR Local Validator / CR HQ
@@ -1383,7 +1400,7 @@ <h1>19.0.2.0.7</h1>
 dependencies.</li>
 </ul>
 </div>
-<div class="section" id="section-4">
+<div class="section" id="section-5">
 <h1>19.0.2.0.6</h1>
 <ul class="simple">
 <li>fix(views): route post-submit CRs (pending / approved / applied /
@@ -1398,7 +1415,7 @@ <h1>19.0.2.0.6</h1>
 list so row-click goes through the stage router.</li>
 </ul>
 </div>
-<div class="section" id="section-5">
+<div class="section" id="section-6">
 <h1>19.0.2.0.5</h1>
 <ul class="simple">
 <li>fix(security): add a global <tt class="docutils literal">ir.rule</tt> on <tt class="docutils literal">spp.change.request</tt> that
@@ -1411,27 +1428,27 @@ <h1>19.0.2.0.5</h1>
 roles).</li>
 </ul>
 </div>
-<div class="section" id="section-6">
+<div class="section" id="section-7">
 <h1>19.0.2.0.3</h1>
 <ul class="simple">
 <li>fix: add HTML escaping to all computed Html fields with
 <tt class="docutils literal">sanitize=False</tt> to prevent stored XSS (#50)</li>
 </ul>
 </div>
-<div class="section" id="section-7">
+<div class="section" id="section-8">
 <h1>19.0.2.0.2</h1>
 <ul class="simple">
 <li>fix: fix batch approval wizard line deletion (#130)</li>
 </ul>
 </div>
-<div class="section" id="section-8">
+<div class="section" id="section-9">
 <h1>19.0.2.0.1</h1>
 <ul class="simple">
 <li>fix: skip field types before getattr and isolate detail prefetch
 (#129)</li>
 </ul>
 </div>
-<div class="section" id="section-9">
+<div class="section" id="section-10">
 <h1>19.0.2.0.0</h1>
 <ul class="simple">
 <li>Initial migration to OpenSPP2</li>

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

spp_change_request_v2/tests/test_apply_authorization.py

@@ -0,0 +1,110 @@
+# 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_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()
+        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)