diff --git a/spp_change_request_v2/README.rst b/spp_change_request_v2/README.rst index 914b1a0e5..52b1d2669 100644 --- a/spp_change_request_v2/README.rst +++ b/spp_change_request_v2/README.rst @@ -853,6 +853,21 @@ Before declaring a new CR type complete: Changelog ========= +19.0.3.0.3 +~~~~~~~~~~ + +- fix(security): scope dynamic-approval conflict and duplicate detection + to the fields that actually differ from the registrant (the changes + the apply strategy will write), instead of a user-writable label. Both + ``selected_field_name`` (view-readonly only) and the detail's + ``field_to_modify`` are writable by a change-request user and do not + constrain what apply changes, so a user could point them at a field + outside a rule's ``conflict_fields`` to clear a field-scoped conflict + and submit without the configured override path. Detection now derives + the changed fields server-side from the detail-vs-registrant diff and + ignores those labels; it is skipped for non-dynamic-approval request + types. + 19.0.3.0.0 ~~~~~~~~~~ diff --git a/spp_change_request_v2/__manifest__.py b/spp_change_request_v2/__manifest__.py index 1e32f323f..516bfd937 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.3", "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/conflict_mixin.py b/spp_change_request_v2/models/conflict_mixin.py index 4f450a29c..cfb754b0c 100644 --- a/spp_change_request_v2/models/conflict_mixin.py +++ b/spp_change_request_v2/models/conflict_mixin.py @@ -291,12 +291,61 @@ def _get_group_member_ids(self): return list(set(member_ids)) + def _proposed_changed_fields(self): + """Return the detail (source) fields a dynamic-approval CR actually + proposes to change. + + These are the mapped fields whose detail value differs from the + registrant's current value — exactly the set the ``field_mapping`` apply + strategy will write. This is derived server-side from the actual data, + NOT from a declared label: both ``selected_field_name`` (only + view-readonly) and the detail's ``field_to_modify`` (freely writable and + validated only to be a real field name, not tied to what apply changes) + are attacker-controlled. A user could otherwise clear a field-scoped + conflict/duplicate by labelling a different, unchanged field while still + changing a scoped field. Scoping to the real diff makes those labels + irrelevant to the security decision. + + Returns a set of detail field names for dynamic-approval CR types, or + ``None`` for non-dynamic types (where field scoping does not apply and + the full configured field set must always be considered — their details + are not registrant-prefilled snapshots). + """ + self.ensure_one() + if not self.request_type_id.use_dynamic_approval: + return None + detail = self.get_detail() + registrant = self.registrant_id + if not detail or not registrant: + return set() + changed = set() + for mapping in self.request_type_id.apply_mapping_ids: + source_field = mapping.source_field + target_field = mapping.target_field + if source_field not in detail._fields or target_field not in registrant._fields: + continue + detail_value = self._normalize_field_value(getattr(detail, source_field, None)) + registrant_value = self._normalize_field_value(getattr(registrant, target_field, None)) + if detail_value != registrant_value: + changed.add(source_field) + return changed + + def _effective_conflict_fields(self, conflict_fields): + """Return the subset of ``conflict_fields`` this CR actually proposes to + change (the security-relevant scope). Full set for non-dynamic types.""" + self.ensure_one() + conflict_fields = set(conflict_fields) + proposed = self._proposed_changed_fields() + if proposed is None: + return conflict_fields + return proposed & conflict_fields + def _filter_by_field_conflicts(self, candidates, rule): """Filter candidate CRs by checking if they modify the same fields. - For dynamic-approval CRs (where selected_field_name is set), only the - selected field is treated as a proposed change. Prefilled fields from - the registrant are ignored for conflict purposes. + For dynamic-approval CRs, the proposed changes are the conflict fields + whose value actually differs from the registrant (derived server-side), + not a user-writable label. Prefilled/unchanged fields are ignored. """ self.ensure_one() @@ -308,14 +357,10 @@ def _filter_by_field_conflicts(self, candidates, rule): if not my_detail: return self.env["spp.change.request"] - # Dynamic approval: only the selected field is a proposed change - my_selected = self.selected_field_name - if my_selected: - if my_selected not in conflict_fields: - return self.env["spp.change.request"] - my_effective_fields = [my_selected] - else: - my_effective_fields = conflict_fields + my_effective_fields = self._effective_conflict_fields(conflict_fields) + if not my_effective_fields: + # This CR changes none of the rule's fields — nothing to conflict on. + return self.env["spp.change.request"] matching = self.env["spp.change.request"] @@ -324,18 +369,10 @@ def _filter_by_field_conflicts(self, candidates, rule): if not candidate_detail: continue - # Determine candidate's effective fields - candidate_selected = candidate.selected_field_name - if candidate_selected: - # Both use dynamic approval: conflict only if same field - if my_selected and candidate_selected != my_selected: - continue - candidate_effective = [candidate_selected] - else: - candidate_effective = conflict_fields + candidate_effective = candidate._effective_conflict_fields(conflict_fields) - # Check overlapping effective fields - fields_to_check = set(my_effective_fields) & set(candidate_effective) + # Overlap = fields BOTH CRs actually propose to change. + fields_to_check = my_effective_fields & candidate_effective for field_name in fields_to_check: if field_name not in my_detail._fields: @@ -439,9 +476,9 @@ def _detect_duplicates(self): def _calculate_similarity(self, other_cr, config): """Calculate similarity percentage between this CR and another. - For dynamic-approval CRs (where selected_field_name is set), only the - selected field is compared. Prefilled fields are ignored to prevent - inflated similarity scores. + For dynamic-approval CRs, only the selected field (derived server-side + from the detail's validated ``field_to_modify``) is compared. Prefilled + fields are ignored to prevent inflated similarity scores. Args: other_cr: Another spp.change.request record @@ -458,22 +495,29 @@ def _calculate_similarity(self, other_cr, config): if not my_detail or not other_detail: return 0.0 - # Dynamic approval: compare only the selected field - my_selected = self.selected_field_name - other_selected = other_cr.selected_field_name - if my_selected and other_selected: - # Different fields selected = not duplicates - if my_selected != other_selected: + # Dynamic approval: compare only the fields actually changed (derived + # server-side from the detail-vs-registrant diff, not a writable label). + my_changed = self._proposed_changed_fields() + other_changed = other_cr._proposed_changed_fields() + if my_changed is not None and other_changed is not None: + # Different set of changed fields (or neither changed anything) = + # not duplicates. + if my_changed != other_changed or not my_changed: return 0.0 - # Same field: compare that field's value only - if my_selected in my_detail._fields and my_selected in other_detail._fields: - my_value = self._normalize_field_value(getattr(my_detail, my_selected, None)) - other_value = self._normalize_field_value(getattr(other_detail, my_selected, None)) - if my_value == other_value: - return 100.0 - elif self._are_similar(my_value, other_value): - return 80.0 - return 0.0 + all_match = True + any_similar = False + for field_name in my_changed: + if field_name not in my_detail._fields or field_name not in other_detail._fields: + continue + my_value = self._normalize_field_value(getattr(my_detail, field_name, None)) + other_value = self._normalize_field_value(getattr(other_detail, field_name, None)) + if my_value != other_value: + all_match = False + if self._are_similar(my_value, other_value): + any_similar = True + if all_match: + return 100.0 + return 80.0 if any_similar else 0.0 # Static CRs (or mixed): original logic check_fields = config.get_check_fields_list() diff --git a/spp_change_request_v2/readme/HISTORY.md b/spp_change_request_v2/readme/HISTORY.md index 0f9d181e5..6e953628f 100644 --- a/spp_change_request_v2/readme/HISTORY.md +++ b/spp_change_request_v2/readme/HISTORY.md @@ -1,3 +1,16 @@ +### 19.0.3.0.3 + +- fix(security): scope dynamic-approval conflict and duplicate detection to the + fields that actually differ from the registrant (the changes the apply + strategy will write), instead of a user-writable label. Both + ``selected_field_name`` (view-readonly only) and the detail's + ``field_to_modify`` are writable by a change-request user and do not constrain + what apply changes, so a user could point them at a field outside a rule's + ``conflict_fields`` to clear a field-scoped conflict and submit without the + configured override path. Detection now derives the changed fields + server-side from the detail-vs-registrant diff and ignores those labels; it + is skipped for non-dynamic-approval request types. + ### 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..0d39262b6 100644 --- a/spp_change_request_v2/static/description/index.html +++ b/spp_change_request_v2/static/description/index.html @@ -1339,6 +1339,22 @@

Changelog

+

19.0.3.0.3

+ +
+

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 +1399,7 @@

    19.0.2.0.7

    dependencies.
-
+

19.0.2.0.6

  • fix(views): route post-submit CRs (pending / approved / applied / @@ -1398,7 +1414,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 +1427,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_conflict_dynamic_approval.py b/spp_change_request_v2/tests/test_conflict_dynamic_approval.py index dfebdd915..aa3ef09fd 100644 --- a/spp_change_request_v2/tests/test_conflict_dynamic_approval.py +++ b/spp_change_request_v2/tests/test_conflict_dynamic_approval.py @@ -70,6 +70,19 @@ def _test_field_to_modify_selection(self): "enable_conflict_detection": True, } ) + # Field-mapping definitions: which detail fields map to which registrant + # fields. Conflict/duplicate detection derives the "actually changed" + # fields from these (detail value vs registrant), so a field_mapping + # type needs them — same-named here (given_name -> given_name, etc.). + cls.dynamic_cr_type.write( + { + "apply_mapping_ids": [ + Command.create({"source_field": "given_name", "target_field": "given_name"}), + Command.create({"source_field": "family_name", "target_field": "family_name"}), + Command.create({"source_field": "phone", "target_field": "phone"}), + ], + } + ) # Field-scope conflict rule: checks given_name, family_name cls.field_rule = cls.env["spp.cr.conflict.rule"].create( @@ -481,3 +494,120 @@ def test_static_cr_create_still_runs_conflict_check(self): cr2.conflict_detection_date, "Static CR must run conflict checks at create time (existing behavior).", ) + + # ────────────────────────────────────────────────────────────────────────── + # Security: a user-writable selected_field_name must NOT bypass field-scoped + # conflict/duplicate detection. selected_field_name is only view-readonly; + # CR users have write on their own CRs, so the detection must derive the + # effective changed field from the trusted detail.field_to_modify selection. + # ────────────────────────────────────────────────────────────────────────── + + def test_writing_selected_field_name_cannot_bypass_field_conflict(self): + """Writing selected_field_name to a field outside conflict_fields must + not clear a real field-scoped conflict on a dynamic-approval CR.""" + cr_a = self._create_dynamic_cr() + cr_a.get_detail().write({"field_to_modify": "given_name", "given_name": "NewGivenA"}) + cr_a._run_conflict_checks() + + cr_b = self._create_dynamic_cr() + cr_b.get_detail().write({"field_to_modify": "given_name", "given_name": "NewGivenB"}) + cr_b._run_conflict_checks() + self.assertEqual(cr_b.conflict_status, "warning", "Baseline: same field = conflict.") + + # Attack: re-point selected_field_name to a field NOT in conflict_fields + # (phone). This re-triggers conflict detection via the write override. + cr_b.write({"selected_field_name": "phone"}) + + self.assertEqual( + cr_b.conflict_status, + "warning", + "Writing selected_field_name must not bypass the field-scoped conflict.", + ) + self.assertIn(cr_a, cr_b.conflicting_cr_ids) + + def test_writing_selected_field_name_on_static_cr_cannot_bypass(self): + """On a non-dynamic-approval CR, selected_field_name must be ignored + entirely — writing it cannot narrow the field-scoped conflict.""" + cr_a = self._create_static_cr() + cr_a.get_detail().write({"given_name": "StaticGivenA"}) + cr_a._run_conflict_checks() + + cr_b = self._create_static_cr() + cr_b.get_detail().write({"given_name": "StaticGivenB"}) + cr_b._run_conflict_checks() + self.assertIn(cr_a, cr_b.conflicting_cr_ids, "Baseline: static CRs conflict on given_name.") + + cr_b.write({"selected_field_name": "phone"}) + + self.assertIn( + cr_a, + cr_b.conflicting_cr_ids, + "selected_field_name must not affect conflict detection for non-dynamic CR types.", + ) + + def test_mislabeled_field_to_modify_cannot_bypass_conflict(self): + """Labelling field_to_modify as an unchanged field must not bypass the + conflict on the field actually changed. field_to_modify is user-writable + and does not constrain apply (field_mapping applies every changed field), + so detection must scope to what actually differs from the registrant.""" + cr_a = self._create_dynamic_cr() + cr_a.get_detail().write({"field_to_modify": "given_name", "given_name": "NewGivenA"}) + cr_a._run_conflict_checks() + + # cr_b really changes given_name (a conflict field) but labels the CR as + # modifying phone (unchanged — still the registrant's prefilled value). + cr_b = self._create_dynamic_cr() + cr_b.get_detail().write({"field_to_modify": "phone", "given_name": "NewGivenB"}) + cr_b._run_conflict_checks() + + self.assertEqual( + cr_b.conflict_status, + "warning", + "A real change to a conflict field must be detected regardless of field_to_modify.", + ) + self.assertIn(cr_a, cr_b.conflicting_cr_ids) + + def test_mislabeled_candidate_still_detected_as_conflict(self): + """A prior CR that mislabels field_to_modify while actually changing a + conflict field must still be found as a conflicting candidate.""" + # cr_a really changes given_name but labels field_to_modify = phone. + cr_a = self._create_dynamic_cr() + cr_a.get_detail().write({"field_to_modify": "phone", "given_name": "MislabeledGivenA"}) + cr_a._run_conflict_checks() + + cr_b = self._create_dynamic_cr() + cr_b.get_detail().write({"field_to_modify": "given_name", "given_name": "NewGivenB"}) + cr_b._run_conflict_checks() + + self.assertIn( + cr_a, + cr_b.conflicting_cr_ids, + "A candidate that actually changed the conflict field must be detected even if mislabeled.", + ) + + def test_writing_selected_field_name_cannot_bypass_duplicate(self): + """Writing selected_field_name must not drop duplicate similarity to 0.""" + dup_config = self.env["spp.cr.duplicate.config"].create( + {"cr_type_id": self.dynamic_cr_type.id, "similarity_threshold": 50.0} + ) + self.dynamic_cr_type.write({"enable_duplicate_detection": True, "duplicate_detection_config_id": dup_config.id}) + try: + cr_a = self._create_dynamic_cr() + cr_a.get_detail().write({"field_to_modify": "given_name", "given_name": "SameValue"}) + cr_a._run_conflict_checks() + + cr_b = self._create_dynamic_cr() + cr_b.get_detail().write({"field_to_modify": "given_name", "given_name": "SameValue"}) + self.assertEqual(cr_b._calculate_similarity(cr_a, dup_config), 100.0, "Baseline duplicate.") + + # Attack: re-point selected_field_name away from the real field. + cr_b.write({"selected_field_name": "phone"}) + + self.assertEqual( + cr_b._calculate_similarity(cr_a, dup_config), + 100.0, + "Writing selected_field_name must not bypass duplicate detection.", + ) + finally: + self.dynamic_cr_type.write({"enable_duplicate_detection": False, "duplicate_detection_config_id": False}) + dup_config.unlink()