From 23f7ccd4c3454310ae579749d8ba40bef4116865 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Mon, 27 Jul 2026 11:24:01 +0800 Subject: [PATCH 1/6] fix(spp_alerts): evaluate alert rules as their owner, not elevated cron The alert-rule evaluation cron runs with an elevated identity (ir.cron's default user is the installer/superuser). The monitored search ran with that identity, so a rule authored by a non-system-admin group_alerts_manager could surface records the author is not allowed to see; the resulting alerts, readable by all alert managers, leaked data across the record-rule boundary. Evaluate each rule's monitored search as the rule's owner (create_uid) via with_user(), so record rules are enforced against whoever configured the rule regardless of who (or what cron) triggers evaluation. An admin-authored rule keeps its wider scope. Adds tests proving the elevated cron no longer surfaces owner-hidden records, and that an unrestricted owner's rule still spans all records. --- spp_alerts/__manifest__.py | 2 +- spp_alerts/models/alert_rule.py | 14 +- spp_alerts/readme/HISTORY.md | 8 ++ spp_alerts/tests/__init__.py | 1 + .../tests/test_rule_evaluation_access.py | 132 ++++++++++++++++++ 5 files changed, 153 insertions(+), 4 deletions(-) create mode 100644 spp_alerts/tests/test_rule_evaluation_access.py diff --git a/spp_alerts/__manifest__.py b/spp_alerts/__manifest__.py index 65b48aa4f..f6f96ea68 100644 --- a/spp_alerts/__manifest__.py +++ b/spp_alerts/__manifest__.py @@ -4,7 +4,7 @@ "summary": "Generic alert engine for threshold monitoring, expiry tracking, " "and deadline management across OpenSPP modules.", "category": "OpenSPP/Infrastructure", - "version": "19.0.2.0.0", + "version": "19.0.2.0.1", "sequence": 1, "author": "OpenSPP.org", "website": "https://github.com/OpenSPP/OpenSPP2", diff --git a/spp_alerts/models/alert_rule.py b/spp_alerts/models/alert_rule.py index 158a44080..c851f1822 100644 --- a/spp_alerts/models/alert_rule.py +++ b/spp_alerts/models/alert_rule.py @@ -294,6 +294,14 @@ def _evaluate_rule(self): _logger.warning("Alert rule '%s' (ID: %d): model '%s' not found, skipping.", self.name, self.id, model_name) return 0 + # Evaluate the monitored search as the rule's owner, not the (elevated) + # cron/superuser identity that may be triggering the run. Record rules are + # then enforced against whoever configured the rule, so a non-admin author + # cannot surface — and leak, via alerts readable by all managers — records + # they are not allowed to see. An admin-authored rule keeps its wider scope. + if self.create_uid: + Model = Model.with_user(self.create_uid.id) + # Parse domain filter try: domain = safe_eval.safe_eval( # nosemgrep: odoo-unsafe-safe-eval @@ -453,9 +461,9 @@ def _prepare_alert_vals(self, record, model_name, current_value=None, days_until # Cron # ------------------------------------------------------------------------- - # Cron runs as superuser (OdooBot). Rule evaluation searches monitored models with - # full access, bypassing record rules. This is intentional — only managers can create - # rules, so the monitored scope is admin-controlled. + # Cron runs as superuser (OdooBot), but each rule's monitored search is evaluated as + # the rule's owner (see _evaluate_rule), so record rules still bound what a rule can + # surface to whoever configured it — the elevated cron identity does not widen scope. @api.model def _cron_evaluate_rules(self): """Scheduled action to evaluate all active, configured rules.""" diff --git a/spp_alerts/readme/HISTORY.md b/spp_alerts/readme/HISTORY.md index 4aaf9afef..ae75d9462 100644 --- a/spp_alerts/readme/HISTORY.md +++ b/spp_alerts/readme/HISTORY.md @@ -1,3 +1,11 @@ +### 19.0.2.0.1 + +- fix(security): evaluate each alert rule's monitored search as the rule's owner + (`create_uid`) instead of the elevated cron/superuser identity, so record rules + bound what a rule can surface to whoever configured it. A non-admin Alerts + Manager can no longer author a rule that leaks records they are not allowed to + see via the alerts the cron creates. + ### 19.0.2.0.0 - Initial migration to OpenSPP2 diff --git a/spp_alerts/tests/__init__.py b/spp_alerts/tests/__init__.py index fe16640a2..6fea5883e 100644 --- a/spp_alerts/tests/__init__.py +++ b/spp_alerts/tests/__init__.py @@ -3,3 +3,4 @@ from . import test_alert from . import test_alert_rule from . import test_rule_evaluation +from . import test_rule_evaluation_access diff --git a/spp_alerts/tests/test_rule_evaluation_access.py b/spp_alerts/tests/test_rule_evaluation_access.py new file mode 100644 index 000000000..2a6a8de46 --- /dev/null +++ b/spp_alerts/tests/test_rule_evaluation_access.py @@ -0,0 +1,132 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Security: rule evaluation must respect the rule owner's record rules. + +The evaluation cron runs elevated (`ir.cron` default user is the installer / +superuser). If the monitored search ran with that elevated identity, a rule +authored by a non-system-admin `group_alerts_manager` could surface records the +author is not allowed to see — the resulting alerts (readable by all alert +managers) then leak data across the record-rule boundary. + +The fix evaluates each rule's monitored search as the rule's owner +(`create_uid`), so record rules are enforced against whoever configured the +rule regardless of who (or what cron) triggers the evaluation. +""" + +from odoo import SUPERUSER_ID +from odoo.tests import tagged + +from .common import AlertsTestCommon + + +@tagged("post_install", "-at_install") +class TestRuleEvaluationAccess(AlertsTestCommon): + """Elevated evaluation of a manager-owned rule honors the manager's rules.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + + cls.partner_model = cls.env["ir.model"].search([("model", "=", "res.partner")], limit=1) + cls.field_color = cls.env["ir.model.fields"].search( + [("model_id", "=", cls.partner_model.id), ("name", "=", "color")], + limit=1, + ) + + # A plain internal user with no alert groups, so the record rule below does + # not restrict them — stands in for an author whose visibility spans all + # partners. (group_system transitively joins group_alerts_manager via the + # SPP admin chain, so it would be restricted too — hence a bare group_user.) + cls.user_unrestricted = cls.env["res.users"].create( + { + "name": "Unrestricted Rule Author", + "login": "alert_unrestricted", + "email": "unrestricted@test.com", + "group_ids": [(4, cls.env.ref("base.group_user").id)], + } + ) + + # Two registrants with a low color so both match a `< 8` threshold. + cls.partner_visible = cls.env["res.partner"].create({"name": "Visible Partner", "color": 1}) + cls.partner_hidden = cls.env["res.partner"].create({"name": "Hidden Partner", "color": 1}) + + # Record rule scoped to the Alerts Manager group: managers can see every + # partner EXCEPT the hidden one. Admin/superuser is not in this group, so + # the elevated cron identity would still see the hidden partner. + cls.env["ir.rule"].create( + { + "name": "Alerts Manager cannot see Hidden Partner", + "model_id": cls.partner_model.id, + "groups": [(4, cls.env.ref("spp_alerts.group_alerts_manager").id)], + "domain_force": f'[("id", "!=", {cls.partner_hidden.id})]', + "perm_read": True, + "perm_write": True, + "perm_create": True, + "perm_unlink": True, + } + ) + + cls.domain_both = f'[("id", "in", [{cls.partner_visible.id}, {cls.partner_hidden.id}])]' + + def _manager_rule(self, **kwargs): + """Create a threshold rule OWNED by the alerts manager (create_uid).""" + vals = { + "name": "Manager Owned Rule", + "alert_type_id": self.alert_type_threshold.id, + "model_id": self.partner_model.id, + "rule_type": "threshold", + "monitored_field_id": self.field_color.id, + "comparison": "lt", + "threshold_value": 8.0, + "domain_filter": self.domain_both, + "priority": "medium", + } + vals.update(kwargs) + return self.env["spp.alert.rule"].with_user(self.user_manager).create(vals) + + def _alert_res_ids(self, rule): + return set(self.env["spp.alert"].search([("rule_id", "=", rule.id)]).mapped("res_id")) + + def test_manager_cannot_see_hidden_partner(self): + """Sanity: the record rule actually hides the partner from the manager.""" + visible_to_manager = self.env["res.partner"].with_user(self.user_manager).search([]).ids + self.assertIn(self.partner_visible.id, visible_to_manager) + self.assertNotIn(self.partner_hidden.id, visible_to_manager) + + def test_elevated_eval_does_not_surface_owner_hidden_records(self): + """A manager-owned rule, evaluated elevated, must not alert on hidden records.""" + rule = self._manager_rule() + self.assertEqual(rule.create_uid, self.user_manager) + + # Evaluate as the superuser cron would (bypasses record rules itself). + rule.with_user(SUPERUSER_ID)._evaluate_rule() + + res_ids = self._alert_res_ids(rule) + self.assertIn(self.partner_visible.id, res_ids) + self.assertNotIn(self.partner_hidden.id, res_ids) + + def test_cron_path_respects_owner_record_rules(self): + """The full cron entrypoint likewise honors the rule owner's visibility.""" + rule = self._manager_rule() + + self.env["spp.alert.rule"].with_user(SUPERUSER_ID)._cron_evaluate_rules() + + res_ids = self._alert_res_ids(rule) + self.assertIn(self.partner_visible.id, res_ids) + self.assertNotIn(self.partner_hidden.id, res_ids) + + def test_unrestricted_owner_rule_still_spans_all_records(self): + """A rule whose owner can see all partners keeps system-wide scope (intended).""" + rule = self._manager_rule(name="Unrestricted Owned Rule") + # Reassign ownership to the unrestricted user (create_uid is not ORM-writable). + self.env.cr.execute( + "UPDATE spp_alert_rule SET create_uid = %s WHERE id = %s", + (self.user_unrestricted.id, rule.id), + ) + rule.invalidate_recordset(["create_uid"]) + self.assertEqual(rule.create_uid, self.user_unrestricted) + + rule.with_user(SUPERUSER_ID)._evaluate_rule() + + res_ids = self._alert_res_ids(rule) + self.assertIn(self.partner_visible.id, res_ids) + self.assertIn(self.partner_hidden.id, res_ids) From 5c632db48f6d6c7bd29f810a4734dcb05e5011c1 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Mon, 27 Jul 2026 11:51:55 +0800 Subject: [PATCH 2/6] chore(spp_alerts): suppress with_user semgrep FP; regenerate README create_uid is an ORM-readonly system field (not user input), so switching the monitored search to it is safe despite odoo-with-user-unvalidated. Also regenerate README.rst/index.html from the HISTORY fragment (CI oca-gen). --- spp_alerts/README.rst | 10 ++++++++++ spp_alerts/models/alert_rule.py | 4 +++- spp_alerts/static/description/index.html | 11 +++++++++++ 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/spp_alerts/README.rst b/spp_alerts/README.rst index d59786052..7c416425a 100644 --- a/spp_alerts/README.rst +++ b/spp_alerts/README.rst @@ -771,6 +771,16 @@ Only test this section if multi-company is enabled. Changelog ========= +19.0.2.0.1 +~~~~~~~~~~ + +- fix(security): evaluate each alert rule's monitored search as the + rule's owner (``create_uid``) instead of the elevated cron/superuser + identity, so record rules bound what a rule can surface to whoever + configured it. A non-admin Alerts Manager can no longer author a rule + that leaks records they are not allowed to see via the alerts the cron + creates. + 19.0.2.0.0 ~~~~~~~~~~ diff --git a/spp_alerts/models/alert_rule.py b/spp_alerts/models/alert_rule.py index c851f1822..6448b85ab 100644 --- a/spp_alerts/models/alert_rule.py +++ b/spp_alerts/models/alert_rule.py @@ -299,8 +299,10 @@ def _evaluate_rule(self): # then enforced against whoever configured the rule, so a non-admin author # cannot surface — and leak, via alerts readable by all managers — records # they are not allowed to see. An admin-authored rule keeps its wider scope. + # create_uid is an ORM-readonly system field (not user input), so switching + # to it is safe despite odoo-with-user-unvalidated. if self.create_uid: - Model = Model.with_user(self.create_uid.id) + Model = Model.with_user(self.create_uid.id) # nosemgrep: odoo-with-user-unvalidated # Parse domain filter try: diff --git a/spp_alerts/static/description/index.html b/spp_alerts/static/description/index.html index f75ce6af5..d1b505af6 100644 --- a/spp_alerts/static/description/index.html +++ b/spp_alerts/static/description/index.html @@ -1164,6 +1164,17 @@

Changelog

+

19.0.2.0.1

+
    +
  • fix(security): evaluate each alert rule’s monitored search as the +rule’s owner (create_uid) instead of the elevated cron/superuser +identity, so record rules bound what a rule can surface to whoever +configured it. A non-admin Alerts Manager can no longer author a rule +that leaks records they are not allowed to see via the alerts the cron +creates.
    • +
+
+

19.0.2.0.0

  • Initial migration to OpenSPP2
    • From eb17b2d120d0f5566ebeaa19a7ed943e07e333be Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Mon, 27 Jul 2026 12:49:37 +0800 Subject: [PATCH 3/6] fix(spp_alerts): re-bind rule evaluation identity on targeting change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Keying evaluation solely on create_uid closed the create vector but not the edit vector: group_alerts_manager has model-wide write on spp.alert.rule and no record rule confines managers to their own rules, so a non-admin manager could repoint an admin-authored rule (change model/domain/monitored field) and have it still evaluate as the admin create_uid — re-opening the leak. Add a system-managed eval_as_user_id (default create_uid) that is re-bound to the editor whenever a targeting field (model_id, domain_filter, monitored_field_id, date_field_id, rule_type) changes, and never client-writable (stripped from create/write vals). Evaluate the monitored search as eval_as_user_id so record rules always bound a rule to whoever last defined its targeting. Ship a migration backfilling eval_as_user_id = create_uid for existing rules. Adds tests: manager repointing an unrestricted-owned rule de-escalates to the manager; editing a non-targeting field preserves owner scope; eval_as_user_id cannot be forged via create or write. --- .../migrations/19.0.2.0.1/post-migration.py | 13 +++ spp_alerts/models/alert_rule.py | 54 ++++++++++--- spp_alerts/readme/HISTORY.md | 12 +-- .../tests/test_rule_evaluation_access.py | 79 +++++++++++++++++-- 4 files changed, 137 insertions(+), 21 deletions(-) create mode 100644 spp_alerts/migrations/19.0.2.0.1/post-migration.py diff --git a/spp_alerts/migrations/19.0.2.0.1/post-migration.py b/spp_alerts/migrations/19.0.2.0.1/post-migration.py new file mode 100644 index 000000000..622f330e7 --- /dev/null +++ b/spp_alerts/migrations/19.0.2.0.1/post-migration.py @@ -0,0 +1,13 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Backfill eval_as_user_id for alert rules that predate 19.0.2.0.1. + +The evaluation identity that bounds a rule's monitored search to its +configurer's record-rule visibility is new in this version. For rules created +before the upgrade it defaults to whoever created the rule. +""" + + +def migrate(cr, version): + if not version: + return + cr.execute("UPDATE spp_alert_rule SET eval_as_user_id = create_uid WHERE eval_as_user_id IS NULL") diff --git a/spp_alerts/models/alert_rule.py b/spp_alerts/models/alert_rule.py index 6448b85ab..85fb36f3e 100644 --- a/spp_alerts/models/alert_rule.py +++ b/spp_alerts/models/alert_rule.py @@ -173,6 +173,40 @@ class AlertRule(models.Model): help="Number of alerts created by this rule", ) + eval_as_user_id = fields.Many2one( + "res.users", + string="Evaluated As", + default=lambda self: self.env.user, + readonly=True, + help="User whose record-rule visibility bounds this rule's monitored search. " + "Set to whoever last defined what the rule targets, so evaluation can never " + "surface records the configurer cannot see. System-managed; not editable.", + ) + + # Fields that define what a rule targets. Changing any of them re-binds the + # evaluation identity to the editor (see write), so a rule can never be + # repointed to surface records its editor is not allowed to see. + _EVAL_TARGETING_FIELDS = ("model_id", "domain_filter", "monitored_field_id", "date_field_id", "rule_type") + + @api.model_create_multi + def create(self, vals_list): + """Force the evaluation identity to the creator; it is never client-supplied.""" + for vals in vals_list: + vals.pop("eval_as_user_id", None) + return super().create(vals_list) + + def write(self, vals): + """Re-bind the evaluation identity to the editor when targeting changes. + + eval_as_user_id is never client-writable directly; it tracks whoever last + defined what the rule targets, so record rules bound the monitored search + to that user's visibility regardless of the elevated cron that runs it. + """ + vals.pop("eval_as_user_id", None) + if any(field in vals for field in self._EVAL_TARGETING_FIELDS): + vals["eval_as_user_id"] = self.env.uid + return super().write(vals) + def _compute_alert_count(self): """Compute the number of alerts associated with each rule.""" alert_data = self.env["spp.alert"].read_group( @@ -294,15 +328,17 @@ def _evaluate_rule(self): _logger.warning("Alert rule '%s' (ID: %d): model '%s' not found, skipping.", self.name, self.id, model_name) return 0 - # Evaluate the monitored search as the rule's owner, not the (elevated) - # cron/superuser identity that may be triggering the run. Record rules are - # then enforced against whoever configured the rule, so a non-admin author - # cannot surface — and leak, via alerts readable by all managers — records - # they are not allowed to see. An admin-authored rule keeps its wider scope. - # create_uid is an ORM-readonly system field (not user input), so switching - # to it is safe despite odoo-with-user-unvalidated. - if self.create_uid: - Model = Model.with_user(self.create_uid.id) # nosemgrep: odoo-with-user-unvalidated + # Evaluate the monitored search as the user who configured what the rule + # targets (eval_as_user_id), not the elevated cron/superuser identity that + # may be triggering the run. Record rules are then enforced against the + # configurer, so a non-admin cannot surface — and leak, via alerts readable + # by all managers — records they are not allowed to see. eval_as_user_id is + # system-managed (re-bound to the editor on any targeting change) and never + # client-writable, so it cannot be forged to escalate. create_uid is the + # fallback for rows predating this field. + eval_user = self.eval_as_user_id or self.create_uid + if eval_user: + Model = Model.with_user(eval_user.id) # nosemgrep: odoo-with-user-unvalidated # Parse domain filter try: diff --git a/spp_alerts/readme/HISTORY.md b/spp_alerts/readme/HISTORY.md index ae75d9462..7ecb59bd3 100644 --- a/spp_alerts/readme/HISTORY.md +++ b/spp_alerts/readme/HISTORY.md @@ -1,10 +1,12 @@ ### 19.0.2.0.1 -- fix(security): evaluate each alert rule's monitored search as the rule's owner - (`create_uid`) instead of the elevated cron/superuser identity, so record rules - bound what a rule can surface to whoever configured it. A non-admin Alerts - Manager can no longer author a rule that leaks records they are not allowed to - see via the alerts the cron creates. +- fix(security): evaluate each alert rule's monitored search as the user who + configured what the rule targets (new system-managed `eval_as_user_id`, re-bound + to the editor whenever a targeting field changes) instead of the elevated + cron/superuser identity, so record rules bound what a rule can surface to that + user's own visibility. A non-admin Alerts Manager can no longer author — or + repoint an admin-authored rule — to leak records they are not allowed to see via + the alerts the cron creates. ### 19.0.2.0.0 diff --git a/spp_alerts/tests/test_rule_evaluation_access.py b/spp_alerts/tests/test_rule_evaluation_access.py index 2a6a8de46..a8c18bce5 100644 --- a/spp_alerts/tests/test_rule_evaluation_access.py +++ b/spp_alerts/tests/test_rule_evaluation_access.py @@ -86,6 +86,18 @@ def _manager_rule(self, **kwargs): def _alert_res_ids(self, rule): return set(self.env["spp.alert"].search([("rule_id", "=", rule.id)]).mapped("res_id")) + def _set_eval_owner(self, rule, user): + """Force a rule's ownership + evaluation identity to `user` (bypasses ORM). + + Stands in for a rule authored by that user (create_uid / eval_as_user_id + are system-managed and not writable through the ORM). + """ + self.env.cr.execute( + "UPDATE spp_alert_rule SET create_uid = %s, eval_as_user_id = %s WHERE id = %s", + (user.id, user.id, rule.id), + ) + rule.invalidate_recordset(["create_uid", "eval_as_user_id"]) + def test_manager_cannot_see_hidden_partner(self): """Sanity: the record rule actually hides the partner from the manager.""" visible_to_manager = self.env["res.partner"].with_user(self.user_manager).search([]).ids @@ -117,16 +129,69 @@ def test_cron_path_respects_owner_record_rules(self): def test_unrestricted_owner_rule_still_spans_all_records(self): """A rule whose owner can see all partners keeps system-wide scope (intended).""" rule = self._manager_rule(name="Unrestricted Owned Rule") - # Reassign ownership to the unrestricted user (create_uid is not ORM-writable). - self.env.cr.execute( - "UPDATE spp_alert_rule SET create_uid = %s WHERE id = %s", - (self.user_unrestricted.id, rule.id), - ) - rule.invalidate_recordset(["create_uid"]) - self.assertEqual(rule.create_uid, self.user_unrestricted) + self._set_eval_owner(rule, self.user_unrestricted) rule.with_user(SUPERUSER_ID)._evaluate_rule() res_ids = self._alert_res_ids(rule) self.assertIn(self.partner_visible.id, res_ids) self.assertIn(self.partner_hidden.id, res_ids) + + def test_manager_repointing_owner_rule_de_escalates(self): + """A manager who repoints an unrestricted-owned rule re-binds it to themselves. + + Managers have model-wide write on rules; without re-binding, editing an + admin-authored rule's targeting would still evaluate as the admin and leak. + """ + rule = self._manager_rule(name="Admin Authored Rule") + self._set_eval_owner(rule, self.user_unrestricted) + + # Manager repoints the rule's domain (still matching both partners). + new_domain = ( + f'[("id", "in", [{self.partner_visible.id}, {self.partner_hidden.id}]), ("active", "in", [True, False])]' + ) + rule.with_user(self.user_manager).write({"domain_filter": new_domain}) + self.assertEqual(rule.eval_as_user_id, self.user_manager) + + rule.with_user(SUPERUSER_ID)._evaluate_rule() + res_ids = self._alert_res_ids(rule) + self.assertIn(self.partner_visible.id, res_ids) + self.assertNotIn(self.partner_hidden.id, res_ids) + + def test_manager_editing_nontargeting_field_preserves_owner_scope(self): + """Editing a non-targeting field must NOT re-bind the evaluation identity.""" + rule = self._manager_rule(name="Admin Authored Rule 2") + self._set_eval_owner(rule, self.user_unrestricted) + + # priority is not a targeting field — the rule still targets what the owner defined. + rule.with_user(self.user_manager).write({"priority": "high"}) + self.assertEqual(rule.eval_as_user_id, self.user_unrestricted) + + rule.with_user(SUPERUSER_ID)._evaluate_rule() + res_ids = self._alert_res_ids(rule) + self.assertIn(self.partner_hidden.id, res_ids) + + def test_eval_as_user_id_not_client_writable(self): + """A client cannot forge the evaluation identity via create or write.""" + rule = ( + self.env["spp.alert.rule"] + .with_user(self.user_manager) + .create( + { + "name": "Forge Attempt Rule", + "alert_type_id": self.alert_type_threshold.id, + "model_id": self.partner_model.id, + "rule_type": "threshold", + "monitored_field_id": self.field_color.id, + "comparison": "lt", + "threshold_value": 8.0, + "domain_filter": self.domain_both, + "priority": "medium", + "eval_as_user_id": self.user_unrestricted.id, + } + ) + ) + self.assertEqual(rule.eval_as_user_id, self.user_manager) + + rule.with_user(self.user_manager).write({"eval_as_user_id": self.user_unrestricted.id}) + self.assertEqual(rule.eval_as_user_id, self.user_manager) From 0257be4fbf18b66f867c8c3e493368c82b7fb312 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Mon, 27 Jul 2026 13:22:52 +0800 Subject: [PATCH 4/6] chore(spp_alerts): regenerate README for updated HISTORY fragment --- spp_alerts/README.rst | 14 ++++++++------ spp_alerts/static/description/index.html | 14 ++++++++------ 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/spp_alerts/README.rst b/spp_alerts/README.rst index 7c416425a..0e6d5babb 100644 --- a/spp_alerts/README.rst +++ b/spp_alerts/README.rst @@ -774,12 +774,14 @@ Changelog 19.0.2.0.1 ~~~~~~~~~~ -- fix(security): evaluate each alert rule's monitored search as the - rule's owner (``create_uid``) instead of the elevated cron/superuser - identity, so record rules bound what a rule can surface to whoever - configured it. A non-admin Alerts Manager can no longer author a rule - that leaks records they are not allowed to see via the alerts the cron - creates. +- fix(security): evaluate each alert rule's monitored search as the user + who configured what the rule targets (new system-managed + ``eval_as_user_id``, re-bound to the editor whenever a targeting field + changes) instead of the elevated cron/superuser identity, so record + rules bound what a rule can surface to that user's own visibility. A + non-admin Alerts Manager can no longer author — or repoint an + admin-authored rule — to leak records they are not allowed to see via + the alerts the cron creates. 19.0.2.0.0 ~~~~~~~~~~ diff --git a/spp_alerts/static/description/index.html b/spp_alerts/static/description/index.html index d1b505af6..066cbb7e4 100644 --- a/spp_alerts/static/description/index.html +++ b/spp_alerts/static/description/index.html @@ -1166,12 +1166,14 @@

    Changelog

    19.0.2.0.1

      -
    • fix(security): evaluate each alert rule’s monitored search as the -rule’s owner (create_uid) instead of the elevated cron/superuser -identity, so record rules bound what a rule can surface to whoever -configured it. A non-admin Alerts Manager can no longer author a rule -that leaks records they are not allowed to see via the alerts the cron -creates.
      • +
    • fix(security): evaluate each alert rule’s monitored search as the user +who configured what the rule targets (new system-managed +eval_as_user_id, re-bound to the editor whenever a targeting field +changes) instead of the elevated cron/superuser identity, so record +rules bound what a rule can surface to that user’s own visibility. A +non-admin Alerts Manager can no longer author — or repoint an +admin-authored rule — to leak records they are not allowed to see via +the alerts the cron creates.
    From 67a56d4cb049d65d2c232dbaa63038038b2a26ea Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Mon, 27 Jul 2026 13:58:19 +0800 Subject: [PATCH 5/6] fix(spp_alerts): harden eval-identity binding per staff review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses two-reviewer adversarial staff review of the A2 fix: - HIGH (forge via context): create() force-sets eval_as_user_id instead of popping it, so the field is always present and Odoo default_get never honours a client default_eval_as_user_id context key (also covers copy()). - HIGH (migration no-op): drop the Python default (which made _init_column pre-fill existing rows with the upgrade user before the migration) and make the backfill unconditional (SET eval_as_user_id = create_uid) so it is authoritative. - MED: expand _EVAL_TARGETING_FIELDS with comparison, threshold_value, days_before, active — fields that change what a rule surfaces without touching model/domain. - MED: fail closed — if no eval user resolves, skip the rule instead of searching with the elevated cron identity. - MED: evaluate in the owner's own company scope, not the triggering cron's default company, so multi-company record rules apply as they would for that user. - Low: write() copies vals (no caller mutation); comment that eval identity is the acting user (preserved under sudo). Adds tests: context-default forge (create + copy), threshold/active rebind, fail-closed, owner-company scoping, and the shipped migration's authoritative backfill. 118 tests, 0 failed. --- .../migrations/19.0.2.0.1/post-migration.py | 6 +- spp_alerts/models/alert_rule.py | 67 ++++++-- spp_alerts/readme/HISTORY.md | 3 +- .../tests/test_rule_evaluation_access.py | 148 +++++++++++++++++- 4 files changed, 204 insertions(+), 20 deletions(-) diff --git a/spp_alerts/migrations/19.0.2.0.1/post-migration.py b/spp_alerts/migrations/19.0.2.0.1/post-migration.py index 622f330e7..1f2632225 100644 --- a/spp_alerts/migrations/19.0.2.0.1/post-migration.py +++ b/spp_alerts/migrations/19.0.2.0.1/post-migration.py @@ -10,4 +10,8 @@ def migrate(cr, version): if not version: return - cr.execute("UPDATE spp_alert_rule SET eval_as_user_id = create_uid WHERE eval_as_user_id IS NULL") + # Every existing row predates eval_as_user_id, so set it authoritatively from + # create_uid. This is unconditional (not `WHERE ... IS NULL`) because the field + # carries no Python default: nothing else populates the column at upgrade time, + # and an IS NULL guard would be defeated if Odoo's _init_column ever pre-filled it. + cr.execute("UPDATE spp_alert_rule SET eval_as_user_id = create_uid") diff --git a/spp_alerts/models/alert_rule.py b/spp_alerts/models/alert_rule.py index 85fb36f3e..dce6dd3fb 100644 --- a/spp_alerts/models/alert_rule.py +++ b/spp_alerts/models/alert_rule.py @@ -176,23 +176,42 @@ class AlertRule(models.Model): eval_as_user_id = fields.Many2one( "res.users", string="Evaluated As", - default=lambda self: self.env.user, readonly=True, help="User whose record-rule visibility bounds this rule's monitored search. " "Set to whoever last defined what the rule targets, so evaluation can never " "surface records the configurer cannot see. System-managed; not editable.", ) - - # Fields that define what a rule targets. Changing any of them re-binds the - # evaluation identity to the editor (see write), so a rule can never be - # repointed to surface records its editor is not allowed to see. - _EVAL_TARGETING_FIELDS = ("model_id", "domain_filter", "monitored_field_id", "date_field_id", "rule_type") + # No Python `default` on purpose: a default would make Odoo's _init_column + # backfill existing rows with the *upgrade* user on module update (before the + # migration runs), and would let a client forge the value through a + # `default_eval_as_user_id` context key via default_get. The identity is set + # explicitly in create() instead, and the migration backfills existing rows. + + # Fields that define what a rule reads or which records it surfaces. Changing + # any of them re-binds the evaluation identity to the editor (see write), so a + # rule can never be repointed — by model, domain, field, type, threshold, or by + # (re)activation — to surface records its editor is not allowed to see. + _EVAL_TARGETING_FIELDS = ( + "model_id", + "domain_filter", + "monitored_field_id", + "date_field_id", + "rule_type", + "comparison", + "threshold_value", + "days_before", + "active", + ) @api.model_create_multi def create(self, vals_list): - """Force the evaluation identity to the creator; it is never client-supplied.""" - for vals in vals_list: - vals.pop("eval_as_user_id", None) + """Force the evaluation identity to the creator; it is never client-supplied. + + Setting the key explicitly (rather than popping it) keeps the field present + in vals so Odoo's default_get — which honours a client `default_eval_as_user_id` + context key — is never consulted for it. + """ + vals_list = [dict(vals, eval_as_user_id=self.env.uid) for vals in vals_list] return super().create(vals_list) def write(self, vals): @@ -200,11 +219,16 @@ def write(self, vals): eval_as_user_id is never client-writable directly; it tracks whoever last defined what the rule targets, so record rules bound the monitored search - to that user's visibility regardless of the elevated cron that runs it. + to that user's visibility regardless of the elevated cron that runs it. Note + the identity is `self.env.uid` (the acting user, preserved even under + `sudo()`); an explicit `with_user()` write would re-widen scope, + which is why only trusted internal callers should do that. """ - vals.pop("eval_as_user_id", None) - if any(field in vals for field in self._EVAL_TARGETING_FIELDS): - vals["eval_as_user_id"] = self.env.uid + if "eval_as_user_id" in vals or any(field in vals for field in self._EVAL_TARGETING_FIELDS): + vals = dict(vals) + vals.pop("eval_as_user_id", None) + if any(field in vals for field in self._EVAL_TARGETING_FIELDS): + vals["eval_as_user_id"] = self.env.uid return super().write(vals) def _compute_alert_count(self): @@ -337,8 +361,21 @@ def _evaluate_rule(self): # client-writable, so it cannot be forged to escalate. create_uid is the # fallback for rows predating this field. eval_user = self.eval_as_user_id or self.create_uid - if eval_user: - Model = Model.with_user(eval_user.id) # nosemgrep: odoo-with-user-unvalidated + if not eval_user: + # Fail closed: without a resolvable configurer we must not fall back to + # the elevated cron identity, which would search with record rules bypassed. + _logger.warning( + "Alert rule '%s' (ID: %d): no evaluation user resolved; skipping to avoid an elevated search.", + self.name, + self.id, + ) + return 0 + # Bind to the configurer AND their own company scope, so multi-company record + # rules apply as they would for that user — not as the triggering cron's + # default company. eval_user is a system-managed field, not client input. + Model = Model.with_user(eval_user.id).with_context( # nosemgrep: odoo-with-user-unvalidated + allowed_company_ids=eval_user.company_ids.ids or eval_user.company_id.ids + ) # Parse domain filter try: diff --git a/spp_alerts/readme/HISTORY.md b/spp_alerts/readme/HISTORY.md index 7ecb59bd3..144755285 100644 --- a/spp_alerts/readme/HISTORY.md +++ b/spp_alerts/readme/HISTORY.md @@ -6,7 +6,8 @@ cron/superuser identity, so record rules bound what a rule can surface to that user's own visibility. A non-admin Alerts Manager can no longer author — or repoint an admin-authored rule — to leak records they are not allowed to see via - the alerts the cron creates. + the alerts the cron creates. The search now also runs in that user's own company + scope rather than the triggering cron's default company. ### 19.0.2.0.0 diff --git a/spp_alerts/tests/test_rule_evaluation_access.py b/spp_alerts/tests/test_rule_evaluation_access.py index a8c18bce5..51f44553d 100644 --- a/spp_alerts/tests/test_rule_evaluation_access.py +++ b/spp_alerts/tests/test_rule_evaluation_access.py @@ -7,9 +7,10 @@ author is not allowed to see — the resulting alerts (readable by all alert managers) then leak data across the record-rule boundary. -The fix evaluates each rule's monitored search as the rule's owner -(`create_uid`), so record rules are enforced against whoever configured the -rule regardless of who (or what cron) triggers the evaluation. +The fix evaluates each rule's monitored search as the user who configured what +the rule targets (system-managed `eval_as_user_id`, re-bound to the editor on any +targeting change and never client-writable), so record rules are enforced against +that user's visibility regardless of who (or what cron) triggers the evaluation. """ from odoo import SUPERUSER_ID @@ -195,3 +196,144 @@ def test_eval_as_user_id_not_client_writable(self): rule.with_user(self.user_manager).write({"eval_as_user_id": self.user_unrestricted.id}) self.assertEqual(rule.eval_as_user_id, self.user_manager) + + def test_eval_as_user_id_not_forgeable_via_context_default(self): + """A `default_eval_as_user_id` context key must not seed the evaluation identity. + + Popping (vs force-setting) the field would leave it missing and let + default_get honour this client-controlled context key. + """ + rule = ( + self.env["spp.alert.rule"] + .with_user(self.user_manager) + .with_context(default_eval_as_user_id=SUPERUSER_ID) + .create( + { + "name": "Context Forge Rule", + "alert_type_id": self.alert_type_threshold.id, + "model_id": self.partner_model.id, + "rule_type": "threshold", + "monitored_field_id": self.field_color.id, + "comparison": "lt", + "threshold_value": 8.0, + "domain_filter": self.domain_both, + "priority": "medium", + } + ) + ) + self.assertEqual(rule.eval_as_user_id, self.user_manager) + + def test_copy_rebinds_evaluation_identity_to_copier(self): + """Duplicating a rule (incl. via a context default) binds it to the copier.""" + rule = self._manager_rule(name="Original Rule") + self._set_eval_owner(rule, self.user_unrestricted) + + copied = rule.with_user(self.user_manager).with_context(default_eval_as_user_id=SUPERUSER_ID).copy() + self.assertEqual(copied.eval_as_user_id, self.user_manager) + + def test_threshold_change_rebinds_and_de_escalates(self): + """Changing a post-search filter field (threshold) re-binds the identity.""" + rule = self._manager_rule(name="Dormant Admin Rule") + self._set_eval_owner(rule, self.user_unrestricted) + + # threshold_value is not the model/domain, but it changes which records leak. + rule.with_user(self.user_manager).write({"threshold_value": 999.0}) + self.assertEqual(rule.eval_as_user_id, self.user_manager) + + rule.with_user(SUPERUSER_ID)._evaluate_rule() + res_ids = self._alert_res_ids(rule) + self.assertNotIn(self.partner_hidden.id, res_ids) + + def test_reactivating_rule_rebinds_identity(self): + """(Re)activating a rule re-binds the identity to whoever activated it.""" + rule = self._manager_rule(name="Inactive Admin Rule", active=False) + self._set_eval_owner(rule, self.user_unrestricted) + + rule.with_user(self.user_manager).write({"active": True}) + self.assertEqual(rule.eval_as_user_id, self.user_manager) + + def test_no_eval_user_fails_closed(self): + """With no resolvable owner, evaluation must skip rather than run elevated.""" + rule = self._manager_rule(name="Orphaned Rule") + self.env.cr.execute( + "UPDATE spp_alert_rule SET create_uid = NULL, eval_as_user_id = NULL WHERE id = %s", + (rule.id,), + ) + rule.invalidate_recordset(["create_uid", "eval_as_user_id"]) + + count = rule.with_user(SUPERUSER_ID)._evaluate_rule() + self.assertEqual(count, 0) + self.assertFalse(self.env["spp.alert"].search([("rule_id", "=", rule.id)])) + + def test_evaluation_scoped_to_owner_companies(self): + """The search runs in the owner's company scope, not the triggering cron's.""" + main = self.company_main + secondary = self.company_secondary + # A global multi-company rule on res.partner so company scoping is deterministic. + self.env["ir.rule"].create( + { + "name": "Partner multi-company (test)", + "model_id": self.partner_model.id, + "global": True, + "domain_force": "['|', ('company_id', '=', False), ('company_id', 'in', company_ids)]", + } + ) + # Owner belongs only to the main company. + owner = self.env["res.users"].create( + { + "name": "Main-Company Owner", + "login": "alert_main_owner", + "email": "main_owner@test.com", + "company_id": main.id, + "company_ids": [(6, 0, [main.id])], + "group_ids": [(4, self.env.ref("base.group_user").id)], + } + ) + partner_secondary = self.env["res.partner"].create( + {"name": "Secondary Co Partner", "color": 1, "company_id": secondary.id} + ) + rule = self._manager_rule( + name="Company Scoped Rule", + domain_filter=f'[("id", "in", [{self.partner_visible.id}, {partner_secondary.id}])]', + ) + self._set_eval_owner(rule, owner) + + # Cron sees both companies; the owner only sees main — the secondary-company + # record must be filtered by the owner's scope, not surfaced by the cron's. + rule.with_user(SUPERUSER_ID).with_context(allowed_company_ids=[main.id, secondary.id])._evaluate_rule() + + res_ids = self._alert_res_ids(rule) + self.assertIn(self.partner_visible.id, res_ids) + self.assertNotIn(partner_secondary.id, res_ids) + + def _load_post_migration(self): + import importlib.util + import os + + path = os.path.join(os.path.dirname(__file__), "..", "migrations", "19.0.2.0.1", "post-migration.py") + spec = importlib.util.spec_from_file_location("spp_alerts_post_migration_test", path) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + def test_migration_backfills_eval_as_user_id_authoritatively(self): + """The shipped migration sets eval_as_user_id = create_uid for existing rules. + + Guards against the IS-NULL-only regression: on a real upgrade Odoo's + _init_column pre-fills the new column with the upgrade user, so the backfill + must overwrite it (not skip non-NULL rows) to bind each rule to its creator. + """ + rule = self._manager_rule(name="Pre-upgrade Rule") + # Simulate the upgrade state: create_uid is the real author; eval_as_user_id + # was wrongly pre-filled with a different (elevated) user by _init_column. + self.env.cr.execute( + "UPDATE spp_alert_rule SET create_uid = %s, eval_as_user_id = %s WHERE id = %s", + (self.user_manager.id, self.user_unrestricted.id, rule.id), + ) + rule.invalidate_recordset(["create_uid", "eval_as_user_id"]) + self.assertEqual(rule.eval_as_user_id, self.user_unrestricted) # wrong, pre-migration + + self._load_post_migration().migrate(self.env.cr, "19.0.2.0.0") + rule.invalidate_recordset(["eval_as_user_id"]) + + self.assertEqual(rule.eval_as_user_id, self.user_manager) From 0c5898bf3626fdf670ebe7763e5117ad48cb20d3 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Mon, 27 Jul 2026 14:10:52 +0800 Subject: [PATCH 6/6] chore(spp_alerts): regenerate README for company-scope HISTORY note --- spp_alerts/README.rst | 3 ++- spp_alerts/static/description/index.html | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/spp_alerts/README.rst b/spp_alerts/README.rst index 0e6d5babb..e9b706a83 100644 --- a/spp_alerts/README.rst +++ b/spp_alerts/README.rst @@ -781,7 +781,8 @@ Changelog rules bound what a rule can surface to that user's own visibility. A non-admin Alerts Manager can no longer author — or repoint an admin-authored rule — to leak records they are not allowed to see via - the alerts the cron creates. + the alerts the cron creates. The search now also runs in that user's + own company scope rather than the triggering cron's default company. 19.0.2.0.0 ~~~~~~~~~~ diff --git a/spp_alerts/static/description/index.html b/spp_alerts/static/description/index.html index 066cbb7e4..a40eab528 100644 --- a/spp_alerts/static/description/index.html +++ b/spp_alerts/static/description/index.html @@ -1173,7 +1173,8 @@

    19.0.2.0.1

    rules bound what a rule can surface to that user’s own visibility. A non-admin Alerts Manager can no longer author — or repoint an admin-authored rule — to leak records they are not allowed to see via -the alerts the cron creates. +the alerts the cron creates. The search now also runs in that user’s +own company scope rather than the triggering cron’s default company.