diff --git a/spp_change_request_v2/README.rst b/spp_change_request_v2/README.rst index 97adb511a..832ac30e6 100644 --- a/spp_change_request_v2/README.rst +++ b/spp_change_request_v2/README.rst @@ -853,6 +853,17 @@ Before declaring a new CR type complete: Changelog ========= +19.0.2.0.8 +~~~~~~~~~~ + +- fix(security): for dynamic-approval CR types, the ``field_mapping`` + apply strategy now writes only the routed/approved field, and the + proposed change is frozen once submitted (selected field, mapped field + values, and the detail record pointer). Previously a user could route + a low-risk field to a weak approval and smuggle changes to other + mapped fields — or swap the field/value/detail after routing — so + unapproved changes reached the registrant. + 19.0.2.0.7 ~~~~~~~~~~ diff --git a/spp_change_request_v2/__manifest__.py b/spp_change_request_v2/__manifest__.py index 3a26a2c99..07ffc94e2 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.2.0.7", + "version": "19.0.2.0.8", "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 d1b1012ee..1f45ddfbe 100644 --- a/spp_change_request_v2/models/change_request.py +++ b/spp_change_request_v2/models/change_request.py @@ -625,6 +625,52 @@ 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", + ) + + @staticmethod + def _normalize_frozen_value(value): + """Normalize a value for change detection: recordset -> id, None -> False. + + Odoo stores unset fields as ``False``, but a write payload (JSON-RPC / + integrations) may pass ``None`` for the same field, or a Many2one as a + recordset. Normalizing both sides prevents an idempotent re-save from + being mistaken for a real change and wrongly locked out. + """ + if hasattr(value, "id"): + value = value.id + return value if value is not None else False + + def write(self, vals): + guarded = [f for f in self._FROZEN_ON_SUBMIT_FIELDS if f in vals] + if guarded: + norm = self._normalize_frozen_value + for rec in self: + if rec.approval_state in ("draft", "revision") or not rec.approval_state: + continue + if any(norm(vals[f]) != norm(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"] diff --git a/spp_change_request_v2/models/change_request_detail_base.py b/spp_change_request_v2/models/change_request_detail_base.py index 6d7313c6c..24fd48d76 100644 --- a/spp_change_request_v2/models/change_request_detail_base.py +++ b/spp_change_request_v2/models/change_request_detail_base.py @@ -72,7 +72,64 @@ 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 + + @staticmethod + def _normalize_frozen_value(value): + """Normalize a value for change detection: recordset -> id, None -> False. + + Odoo stores unset fields as ``False``, but a write payload may pass + ``None`` for the same field or a Many2one as a recordset; normalizing + both sides prevents an idempotent re-save from being mistaken for a real + change and wrongly locked out. + """ + if hasattr(value, "id"): + value = value.id + return value if value is not None else False + + 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 + # Normalize both sides (recordset -> id, None -> False) so an + # idempotent re-save, a Many2one written as a recordset, or a + # JSON-RPC None is not mistaken for a real change and locked out. + if self._normalize_frozen_value(vals[field_name]) != self._normalize_frozen_value(rec[field_name]): + 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: diff --git a/spp_change_request_v2/readme/HISTORY.md b/spp_change_request_v2/readme/HISTORY.md index 8644831eb..2be7dd51b 100644 --- a/spp_change_request_v2/readme/HISTORY.md +++ b/spp_change_request_v2/readme/HISTORY.md @@ -1,3 +1,7 @@ +### 19.0.2.0.8 + +- fix(security): for dynamic-approval CR types, the `field_mapping` apply strategy now writes only the routed/approved field, and the proposed change is frozen once submitted (selected field, mapped field values, and the detail record pointer). Previously a user could route a low-risk field to a weak approval and smuggle changes to other mapped fields — or swap the field/value/detail after routing — so unapproved changes reached the registrant. + ### 19.0.2.0.7 - fix(security): align CR Requestor / CR Local Validator / CR HQ Validator roles with the OP#951 menu audit — replace the `spp_registry.group_registry_read` (Tier-3, no menu) link with `spp_registry.group_registry_viewer` so these roles see the Registry menu; add `spp_hazard.group_hazard_viewer` so they retain Hazard visibility once the menu root is gated. Adds `spp_hazard` to module dependencies. diff --git a/spp_change_request_v2/static/description/index.html b/spp_change_request_v2/static/description/index.html index ef26ac45e..8ac2b997b 100644 --- a/spp_change_request_v2/static/description/index.html +++ b/spp_change_request_v2/static/description/index.html @@ -1339,6 +1339,18 @@

Changelog

+

19.0.2.0.8

+ +
+

19.0.2.0.7

-
+

19.0.2.0.6

-
+

19.0.2.0.5

  • fix(security): add a global ir.rule on spp.change.request that @@ -1378,27 +1390,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/strategies/field_mapping.py b/spp_change_request_v2/strategies/field_mapping.py index a1c795e68..4c09972de 100644 --- a/spp_change_request_v2/strategies/field_mapping.py +++ b/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) diff --git a/spp_change_request_v2/tests/test_dynamic_approval.py b/spp_change_request_v2/tests/test_dynamic_approval.py index 7dfa849b2..ec48c4ec5 100644 --- a/spp_change_request_v2/tests/test_dynamic_approval.py +++ b/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,151 @@ 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_no_op_write_of_unset_protected_field_is_allowed(self): + """Regression (false-positive lockout): writing None to a protected source + field that is already unset must not raise post-submit. Odoo stores unset + fields as False while a JSON-RPC payload may send None for the same field; + the freeze must treat them as equal (no change), not lock the user out.""" + _cr, detail = self._submit_dynamic_cr(selected="phone") + self.assertFalse(detail.birthdate) # a protected (mapped) field, unset + # None vs the stored False is a no-op, not a change — must not raise. + detail.write({"birthdate": None}) + + 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")