security(cr): route and apply the same single field for dynamic approvals

spp_change_request_v2/models/change_request.py

@@ -625,6 +625,38 @@ def create(self, vals_list):
                 record._run_conflict_checks()
         return records
 
+    # Fields that bind a submitted CR to exactly what was routed and approved:
+    # the dynamic-approval selection (synced from the detail's field_to_modify in
+    # draft) and the detail record pointer that get_detail() resolves for both
+    # routing and apply. Once the CR leaves draft/revision these are frozen — else
+    # a user could route on a low-risk field / benign detail and then swap the
+    # selection or repoint detail_res_id to a substituted detail before apply.
+    # Editing requires reset to draft, which re-routes. (These fields are never
+    # written by the apply strategies, so the guard needs no apply-path exemption.)
+    _FROZEN_ON_SUBMIT_FIELDS = (
+        "selected_field_name",
+        "selected_field_old_value",
+        "selected_field_new_value",
+        "detail_res_id",
+        "detail_res_model",
+    )
+
+    def write(self, vals):
+        guarded = [f for f in self._FROZEN_ON_SUBMIT_FIELDS if f in vals]
+        if guarded:
+            for rec in self:
+                if rec.approval_state in ("draft", "revision") or not rec.approval_state:
+                    continue
+                if any(vals[f] != rec[f] for f in guarded):
+                    raise UserError(
+                        _(
+                            "A submitted change request is locked to the change it was "
+                            "routed and approved for; its selected field and detail record "
+                            "cannot be changed. Reset the request to draft to re-route."
+                        )
+                    )
+        return super().write(vals)
+
     def unlink(self):
         """Delete associated detail records and archive DMS directory."""
         directories_to_archive = self.env["spp.dms.directory"]

spp_change_request_v2/models/change_request_detail_base.py

@@ -72,7 +72,51 @@ def _get_field_to_modify_selection(self):
         """
         return []
 
+    def _protected_content_fields(self, change_request):
+        """Fields whose value defines the proposed change / approval routing.
+
+        These must not change once the CR has left draft/revision, otherwise a
+        user could re-route the approval (change the selected field) or alter the
+        value that was routed and approved (see dynamic-approval routing). For
+        the field_mapping strategy that is the routing selector plus every mapped
+        source field; apply-output fields (e.g. created_*_id) are NOT included so
+        the apply strategies can still record their results post-approval.
+        """
+        protected = {"field_to_modify"}
+        cr_type = change_request.request_type_id
+        if cr_type.apply_strategy == "field_mapping":
+            protected |= {m.source_field for m in cr_type.apply_mapping_ids if m.source_field}
+        return protected
+
+    def _assert_content_editable(self, vals):
+        """Reject edits to proposed-change fields once the CR is submitted.
+
+        Mirrors the view-level readonly (approval_state not in draft/revision) at
+        the server so it cannot be bypassed via RPC. Editing requires resetting
+        the CR to draft, which re-routes the approval.
+        """
+        for rec in self:
+            change_request = rec.change_request_id
+            state = change_request.approval_state
+            if not change_request or state in ("draft", "revision") or not state:
+                continue
+            for field_name in rec._protected_content_fields(change_request):
+                if field_name not in vals or field_name not in rec._fields:
+                    continue
+                current = rec[field_name]
+                if hasattr(current, "id"):
+                    current = current.id
+                if vals[field_name] != current:
+                    raise UserError(
+                        _(
+                            "This change request has already been submitted for approval, "
+                            "so its proposed changes are locked. Reset it to draft to edit "
+                            "(this re-routes the approval)."
+                        )
+                    )
+
     def write(self, vals):
+        self._assert_content_editable(vals)
         result = super().write(vals)
         if "field_to_modify" in vals:
             for rec in self:

spp_change_request_v2/strategies/field_mapping.py

@@ -15,17 +15,35 @@ class SPPCRStrategyFieldMapping(models.AbstractModel):
     _inherit = "spp.cr.strategy.base"
     _description = "CR Apply Strategy: Field Mapping"
 
+    def _effective_mappings(self, change_request):
+        """Return the mappings that may be applied for this change request.
+
+        For dynamic-approval CR types the approval workflow is routed and
+        approved based on a single selected field, so ONLY that field's mapping
+        may be written to the registrant — regardless of any other mapped detail
+        fields that were also changed. This keeps the applied change in lockstep
+        with what was actually approved. Fail closed: if no field is selected, or
+        the selection maps to no configured field, nothing is applied.
+        """
+        cr_type = change_request.request_type_id
+        mappings = cr_type.apply_mapping_ids
+        if not cr_type.use_dynamic_approval:
+            return mappings
+        selected = change_request.selected_field_name
+        if not selected:
+            return mappings.browse()
+        return mappings.filtered(lambda m: m.source_field == selected)
+
     def apply(self, change_request):
         """Apply field mappings from detail to registrant."""
         registrant = change_request.registrant_id
         detail = change_request.get_detail()
-        cr_type = change_request.request_type_id
 
         if not detail:
             raise UserError(_("No detail record found."))
 
         values = {}
-        for mapping in cr_type.apply_mapping_ids:
+        for mapping in self._effective_mappings(change_request):
             source_value = getattr(detail, mapping.source_field, None)
             current_value = getattr(registrant, mapping.target_field, None)
 
@@ -147,13 +165,14 @@ def preview(self, change_request):
         """Preview what changes will be applied."""
         registrant = change_request.registrant_id
         detail = change_request.get_detail()
-        cr_type = change_request.request_type_id
 
         if not detail:
             return {}
 
         changes = {}
-        for mapping in cr_type.apply_mapping_ids:
+        # Mirror apply(): a dynamic-approval CR previews only the selected field,
+        # so the approver sees exactly what will be written.
+        for mapping in self._effective_mappings(change_request):
             source_raw = getattr(detail, mapping.source_field, None)
             current_raw = getattr(registrant, mapping.target_field, None)
 

spp_change_request_v2/tests/test_dynamic_approval.py

@@ -14,7 +14,7 @@
 import logging
 
 from odoo import Command, api
-from odoo.exceptions import ValidationError
+from odoo.exceptions import UserError, ValidationError
 from odoo.tests import TransactionCase, tagged
 
 _logger = logging.getLogger(__name__)
@@ -1057,3 +1057,141 @@ def test_normalize_many2one_with_parent(self):
         self.assertIn("id", normalized["parent"])
         self.assertIn("name", normalized["parent"])
         self.assertIn("code", normalized["parent"])
+
+    # ──────────────────────────────────────────────────────────────────────────
+    # APPLY RESTRICTION — a dynamic-approval CR must apply ONLY the selected
+    # field, even if other mapped detail fields were also changed. Otherwise a
+    # user could route a low-risk field to a weak workflow and smuggle changes
+    # to other (higher-risk) mapped fields through the same weak approval.
+    # ──────────────────────────────────────────────────────────────────────────
+
+    def _field_mapping_strategy(self):
+        return self.env["spp.cr.strategy.field_mapping"]
+
+    def test_dynamic_apply_writes_only_selected_field(self):
+        cr = self._create_cr()
+        detail = cr.get_detail()
+        # Select the low-risk field (phone) but ALSO change a high-risk field.
+        detail.write({"field_to_modify": "phone", "phone": "999-000", "given_name": "HACKED"})
+        self.assertEqual(cr.selected_field_name, "phone")
+
+        self._field_mapping_strategy().apply(cr)
+
+        self.assertEqual(self.registrant.phone, "999-000", "the selected field must be applied")
+        self.assertEqual(
+            self.registrant.given_name,
+            "Original Given",
+            "a non-selected mapped field must NOT be applied for a dynamic-approval CR",
+        )
+
+    def test_dynamic_preview_shows_only_selected_field(self):
+        cr = self._create_cr()
+        detail = cr.get_detail()
+        detail.write({"field_to_modify": "phone", "phone": "999-000", "given_name": "HACKED"})
+
+        changes = self._field_mapping_strategy().preview(cr)
+
+        self.assertEqual(len(changes), 1, "preview must show only the selected field for a dynamic CR")
+        self.assertEqual(next(iter(changes.values()))["new"], "999-000")
+
+    def test_dynamic_apply_unmapped_selected_field_writes_nothing(self):
+        """Fail-closed: a dynamic CR whose selected field has no mapping writes nothing,
+        even if another mapped detail field was changed."""
+        cr = self._create_cr()
+        detail = cr.get_detail()
+        detail.write({"phone": "999-000"})
+        # Force a selected field that is not present in apply_mapping_ids.
+        cr.selected_field_name = "email"
+
+        self._field_mapping_strategy().apply(cr)
+
+        self.assertEqual(self.registrant.phone, "111-222", "nothing may be applied for an unmapped selection")
+
+    def test_non_dynamic_apply_still_writes_all_changed_fields(self):
+        """Regression: non-dynamic CR types keep applying every changed mapping."""
+        nd_type = self.CRType.create(
+            {
+                "name": "Non-Dynamic With Mappings",
+                "code": "nd_with_mappings_test",
+                "target_type": "individual",
+                "detail_model": "spp.cr.detail.edit_individual",
+                "apply_strategy": "field_mapping",
+                "approval_definition_id": self.static_def.id,
+                "use_dynamic_approval": False,
+                "apply_mapping_ids": [
+                    Command.create({"source_field": "phone", "target_field": "phone", "sequence": 10}),
+                    Command.create({"source_field": "given_name", "target_field": "given_name", "sequence": 20}),
+                ],
+            }
+        )
+        reg = self.env["res.partner"].create(
+            {
+                "name": "ND Registrant",
+                "given_name": "OldGiven",
+                "phone": "000-000",
+                "is_registrant": True,
+                "is_group": False,
+            }
+        )
+        cr = self.CR.create({"request_type_id": nd_type.id, "registrant_id": reg.id})
+        detail = cr.get_detail()
+        detail.write({"phone": "555-555", "given_name": "NewGiven"})
+
+        self._field_mapping_strategy().apply(cr)
+
+        self.assertEqual(reg.phone, "555-555")
+        self.assertEqual(reg.given_name, "NewGiven", "non-dynamic CR must apply all changed mappings")
+
+    # ──────────────────────────────────────────────────────────────────────────
+    # POST-SUBMIT FREEZE — once routed, the proposed change (selected field and
+    # the mapped values) is frozen. This closes the desync where a user routes on
+    # a low-risk field, then swaps the field or its value before apply.
+    # ──────────────────────────────────────────────────────────────────────────
+
+    def _submit_dynamic_cr(self, selected="phone", **detail_vals):
+        cr = self._create_cr()
+        detail = cr.get_detail()
+        detail.write({"field_to_modify": selected, selected: detail_vals.get(selected, "999-000"), **detail_vals})
+        cr.action_submit_for_approval()
+        cr.invalidate_recordset()
+        self.assertEqual(cr.approval_state, "pending")
+        return cr, detail
+
+    def test_cannot_change_field_to_modify_after_submit(self):
+        _cr, detail = self._submit_dynamic_cr(selected="phone")
+        with self.assertRaises(UserError):
+            detail.write({"field_to_modify": "given_name", "given_name": "HACKED"})
+
+    def test_cannot_change_selected_field_name_directly_after_submit(self):
+        cr, _detail = self._submit_dynamic_cr(selected="phone")
+        with self.assertRaises(UserError):
+            cr.write({"selected_field_name": "given_name"})
+
+    def test_cannot_change_selected_field_value_after_submit(self):
+        """Value-swap: even the same (selected) field's value is frozen post-submit,
+        because the value was what the approval was routed on."""
+        _cr, detail = self._submit_dynamic_cr(selected="phone", phone="111-orig")
+        with self.assertRaises(UserError):
+            detail.write({"phone": "222-swapped"})
+
+    def test_cannot_repoint_detail_after_submit(self):
+        """Substitution bypass: create a second detail and repoint detail_res_id
+        to it. get_detail() resolves strictly by detail_res_id, so this would
+        otherwise apply the substituted values under the original routing."""
+        cr, _detail = self._submit_dynamic_cr(selected="phone", phone="111-orig")
+        substitute = self.env["spp.cr.detail.edit_individual"].create(
+            {"change_request_id": cr.id, "phone": "999-SUBSTITUTED"}
+        )
+        with self.assertRaises(UserError):
+            cr.write({"detail_res_id": substitute.id})
+
+    def test_can_change_selection_while_draft(self):
+        """The freeze must not over-block: while still in draft the user can
+        freely change the selected field (which re-routes on submission)."""
+        cr = self._create_cr()
+        detail = cr.get_detail()
+        detail.write({"field_to_modify": "phone", "phone": "111-222-draft"})
+        # Still draft — switching the selected field is allowed.
+        detail.write({"field_to_modify": "given_name", "given_name": "Draft Edit"})
+        self.assertEqual(cr.approval_state, "draft")
+        self.assertEqual(cr.selected_field_name, "given_name")