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/__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/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. 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. 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

+ +
+

19.0.3.0.0

-
+

19.0.2.0.8

-
+

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/__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..33f16dcd6 --- /dev/null +++ b/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)