diff --git a/spp_alerts/README.rst b/spp_alerts/README.rst index d59786052..e9b706a83 100644 --- a/spp_alerts/README.rst +++ b/spp_alerts/README.rst @@ -771,6 +771,19 @@ 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 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. 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/__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/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..1f2632225 --- /dev/null +++ b/spp_alerts/migrations/19.0.2.0.1/post-migration.py @@ -0,0 +1,17 @@ +# 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 + # 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 158a44080..dce6dd3fb 100644 --- a/spp_alerts/models/alert_rule.py +++ b/spp_alerts/models/alert_rule.py @@ -173,6 +173,64 @@ class AlertRule(models.Model): help="Number of alerts created by this rule", ) + eval_as_user_id = fields.Many2one( + "res.users", + string="Evaluated As", + 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.", + ) + # 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. + + 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): + """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. 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. + """ + 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): """Compute the number of alerts associated with each rule.""" alert_data = self.env["spp.alert"].read_group( @@ -294,6 +352,31 @@ 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 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 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: domain = safe_eval.safe_eval( # nosemgrep: odoo-unsafe-safe-eval @@ -453,9 +536,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..144755285 100644 --- a/spp_alerts/readme/HISTORY.md +++ b/spp_alerts/readme/HISTORY.md @@ -1,3 +1,14 @@ +### 19.0.2.0.1 + +- 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. 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 - Initial migration to OpenSPP2 diff --git a/spp_alerts/static/description/index.html b/spp_alerts/static/description/index.html index f75ce6af5..a40eab528 100644 --- a/spp_alerts/static/description/index.html +++ b/spp_alerts/static/description/index.html @@ -1164,6 +1164,20 @@

Changelog

+

19.0.2.0.1

+ +
+

19.0.2.0.0