From dc486df21ccd8fafd1ce392c23c654f7b49825d0 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 14 Aug 2026 00:28:22 +0800 Subject: [PATCH 01/15] fix(spp_grm_cel): evaluate GRM rules as their owner, guard entry points (#379, #381) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - eval_as_user_id (system-managed, no default) on both rule models; forced to the creator in create() and re-bound to the editor when targeting changes in write(), so it cannot be forged via context or direct write. Re-bind excludes operational toggles (sequence, active) so a manager archiving/reordering an officer's rule cannot silently transfer ownership to the manager's scope. - apply_routing/apply_escalations evaluate and apply each rule with_user(owner): an officer's always-match rule can no longer ride the superuser cron to seize tickets outside the officer's record-rule scope (#379). Owner-unreadable/unwritable tickets are skipped, not applied elevated. - @api.private on apply_routing, apply_escalations, apply_escalation, check_escalations: no longer RPC-dispatchable (#381). Cron/SLA callers are in-process and unaffected. - Drop the portal and internal-user read rows on both rule models — owner identity removes the need, closing the enumeration surface (#380). - Atomic UPDATE for match_count/escalation_count (no sudo, no lost updates). - CEL validation reports any parser error as ValidationError. - Migration backfills eval_as_user_id from create_uid. --- spp_grm_cel/__manifest__.py | 2 +- .../migrations/19.0.2.0.2/post-migration.py | 35 ++++ spp_grm_cel/models/grm_escalation_rule.py | 108 ++++++++++- spp_grm_cel/models/grm_routing_rule.py | 108 ++++++++++- spp_grm_cel/readme/HISTORY.md | 18 ++ spp_grm_cel/security/ir.model.access.csv | 4 - spp_grm_cel/tests/__init__.py | 1 + spp_grm_cel/tests/test_rule_acl.py | 52 ++---- spp_grm_cel/tests/test_rule_owner_identity.py | 175 ++++++++++++++++++ .../views/grm_escalation_rule_views.xml | 1 + spp_grm_cel/views/grm_routing_rule_views.xml | 1 + 11 files changed, 447 insertions(+), 58 deletions(-) create mode 100644 spp_grm_cel/migrations/19.0.2.0.2/post-migration.py create mode 100644 spp_grm_cel/tests/test_rule_owner_identity.py diff --git a/spp_grm_cel/__manifest__.py b/spp_grm_cel/__manifest__.py index 5678cb426..be031ca91 100644 --- a/spp_grm_cel/__manifest__.py +++ b/spp_grm_cel/__manifest__.py @@ -2,7 +2,7 @@ { "name": "OpenSPP GRM: CEL Rules", "summary": "CEL-based routing and escalation rules for GRM tickets", - "version": "19.0.2.0.1", + "version": "19.0.2.0.2", "license": "LGPL-3", "development_status": "Production/Stable", "maintainers": ["jeremi", "gonzalesedwin1123", "emjay0921"], diff --git a/spp_grm_cel/migrations/19.0.2.0.2/post-migration.py b/spp_grm_cel/migrations/19.0.2.0.2/post-migration.py new file mode 100644 index 000000000..8086cb156 --- /dev/null +++ b/spp_grm_cel/migrations/19.0.2.0.2/post-migration.py @@ -0,0 +1,35 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Backfill eval_as_user_id on GRM rules from create_uid. + +The new ``eval_as_user_id`` field (added for #379) bounds a rule's evaluation +to its author's ticket scope. Existing rules predate the field, so attribute it +to whoever created each rule. The field carries no Python ``default`` on purpose +(a default would make Odoo's ``_init_column`` prefill every row with the upgrade +user before this runs), so every pre-existing row is NULL here and the backfill +is effectively unconditional; the ``WHERE ... IS NULL`` guard only makes a re-run +a no-op. +""" + +import logging + +_logger = logging.getLogger(__name__) + + +def migrate(cr, version): + if not version: + return + + cr.execute("UPDATE spp_grm_routing_rule SET eval_as_user_id = create_uid WHERE eval_as_user_id IS NULL") + routing = cr.rowcount + cr.execute("UPDATE spp_grm_escalation_rule SET eval_as_user_id = create_uid WHERE eval_as_user_id IS NULL") + escalation = cr.rowcount + + if routing or escalation: + _logger.warning( + "Backfilled eval_as_user_id from create_uid on %s routing rule(s) and " + "%s escalation rule(s). These rules now evaluate with their creator's " + "record-rule scope; review any rule whose creator's permissions have " + "changed since it was authored.", + routing, + escalation, + ) diff --git a/spp_grm_cel/models/grm_escalation_rule.py b/spp_grm_cel/models/grm_escalation_rule.py index c4c1547a1..ddcb4c621 100644 --- a/spp_grm_cel/models/grm_escalation_rule.py +++ b/spp_grm_cel/models/grm_escalation_rule.py @@ -1,7 +1,7 @@ import logging from odoo import _, api, fields, models -from odoo.exceptions import ValidationError +from odoo.exceptions import AccessError, ValidationError _logger = logging.getLogger(__name__) @@ -139,6 +139,61 @@ class GRMEscalationRule(models.Model): help="Number of times this rule has triggered an escalation", ) + eval_as_user_id = fields.Many2one( + "res.users", + string="Evaluated As", + readonly=True, + ondelete="restrict", + help="User whose record-rule visibility bounds this rule's evaluation and " + "the ticket writes it performs. Set to whoever last defined what the rule " + "targets, so an elevated cron can never apply a rule beyond its author's " + "reach. System-managed; not editable.", + ) + # No Python `default` on purpose (see spp_alerts #364): a default makes + # _init_column backfill existing rows with the UPGRADE user before the + # migration runs, and lets a client forge the value via a + # default_eval_as_user_id context key. The identity is set explicitly in + # create(); the migration backfills existing rows from create_uid. + + # Fields that decide what a rule matches or does. Changing any re-binds the + # evaluation identity to the editor (see write), so a rule can never be + # repointed to act beyond its editor's ticket scope. Deliberately excludes + # operational toggles (sequence, active): reordering or archiving/unarchiving + # a rule must not silently transfer ownership to the person doing that + # routine action (a manager cleaning up an officer's rule would otherwise + # re-bind it to the manager's broad scope). + _EVAL_TARGETING_FIELDS = ( + "condition_cel", + "escalate_to_user_id", + "escalate_to_team_id", + "escalate_severity", + "escalate_priority", + "trigger_after_hours", + "should_send_notification", + "notification_template_id", + "create_case", + "case_type_id", + ) + + @api.model_create_multi + def create(self, vals_list): + """Force the evaluation identity to the creator; never client-supplied.""" + 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. + + self.env.uid is the acting user, preserved even under sudo() (only an + explicit with_user() write re-widens scope). + """ + if "eval_as_user_id" in vals or any(f in vals for f in self._EVAL_TARGETING_FIELDS): + vals = dict(vals) + vals.pop("eval_as_user_id", None) + if any(f in vals for f in self._EVAL_TARGETING_FIELDS): + vals["eval_as_user_id"] = self.env.uid + return super().write(vals) + @api.constrains("condition_cel") def _check_condition_cel(self): """Validate CEL expression syntax using CEL parser.""" @@ -149,7 +204,9 @@ def _check_condition_cel(self): # Use proper CEL parser for validation P.parse(rule.condition_cel) # If parser not available, skip validation - except SyntaxError as e: + except Exception as e: + # Any parser failure (not only SyntaxError) is a bad + # expression the user must fix, surfaced as a ValidationError. raise ValidationError( _( "Invalid CEL expression in rule '%(rule_name)s': %(error)s", @@ -288,9 +345,14 @@ def _evaluate_expression(self, expression, context): _logger.warning("Expression evaluation error: %s", str(e)) raise + @api.private def apply_escalation(self, ticket): """Apply this escalation rule to a ticket. + Private: not RPC-dispatchable. Callers must evaluate the rule with its + owner identity first (see apply_escalations); the ticket writes below + run in whatever context ``self``/``ticket`` carry. + Args: ticket: spp.grm.ticket record @@ -344,9 +406,15 @@ def apply_escalation(self, ticket): if self.create_case and self.case_type_id: self._create_case_from_ticket(ticket) - # Update escalation count - # nosemgrep: semgrep.odoo-sudo-without-context -- counter update needs sudo - self.sudo().write({"escalation_count": self.escalation_count + 1}) + # Atomic increment: avoids lost updates under concurrent cron/UI + # escalation, and needs no sudo (raw SQL bypasses ACL). Invisible to + # spp_audit ORM write-hooks, which is acceptable for a stats counter. + self.flush_recordset(["escalation_count"]) + self.env.cr.execute( + "UPDATE spp_grm_escalation_rule SET escalation_count = escalation_count + 1 WHERE id = %s", + (self.id,), + ) + self.invalidate_recordset(["escalation_count"]) # Post message to chatter ticket.message_post( @@ -432,10 +500,12 @@ def _create_case_from_ticket(self, ticket): ) @api.model + @api.private def check_escalations(self): """Cron job to check and apply escalation rules to open tickets. This should be called periodically (e.g., hourly) by a scheduled action. + Private: invoked server-side by ir.cron, never via RPC. """ # Find all open tickets tickets = self.env["spp.grm.ticket"].search( @@ -455,9 +525,15 @@ def check_escalations(self): return escalated_count @api.model + @api.private def apply_escalations(self, ticket): """Apply all matching escalation rules to a ticket. + Each rule is evaluated and applied with the identity of whoever defined + it (``eval_as_user_id``), so an elevated caller (the hourly cron, the + sudo'd SLA-breach path) can never make a rule act beyond its author's + ticket scope. Private: not RPC-dispatchable. + Args: ticket: spp.grm.ticket record @@ -469,8 +545,26 @@ def apply_escalations(self, ticket): applied = False for rule in rules: - if rule.evaluate(ticket): - rule.apply_escalation(ticket) + owner = rule.eval_as_user_id or rule.create_uid + if not owner: + continue + # nosemgrep: semgrep.odoo-with-user-unvalidated -- owner is system-set in create()/write(), not client input + rule_as_owner = rule.with_user(owner.id) + # nosemgrep: semgrep.odoo-with-user-unvalidated -- owner is system-set; scopes ticket writes + ticket_as_owner = ticket.with_user(owner.id) + try: + matched = rule_as_owner.evaluate(ticket_as_owner) + except AccessError: + # The rule owner cannot see this ticket -> the rule does not + # apply to it. Correct behaviour, not an error. + continue + if matched: + try: + rule_as_owner.apply_escalation(ticket_as_owner) + except AccessError: + # Owner matched but cannot write this ticket -> skip rather + # than apply with elevated rights. + continue applied = True # Continue checking other rules (unlike routing, multiple escalations can apply) diff --git a/spp_grm_cel/models/grm_routing_rule.py b/spp_grm_cel/models/grm_routing_rule.py index 5d13a3ba5..73a762ccf 100644 --- a/spp_grm_cel/models/grm_routing_rule.py +++ b/spp_grm_cel/models/grm_routing_rule.py @@ -1,7 +1,7 @@ import logging from odoo import _, api, fields, models -from odoo.exceptions import ValidationError +from odoo.exceptions import AccessError, ValidationError _logger = logging.getLogger(__name__) @@ -105,6 +105,62 @@ class GRMRoutingRule(models.Model): help="Number of tickets this rule has matched", ) + eval_as_user_id = fields.Many2one( + "res.users", + string="Evaluated As", + readonly=True, + ondelete="restrict", + help="User whose record-rule visibility bounds this rule's evaluation and " + "the ticket writes it performs. Set to whoever last defined what the rule " + "targets, so an elevated cron can never apply a rule beyond its author's " + "reach. System-managed; not editable.", + ) + # No Python `default` on purpose (see spp_alerts #364): a default makes + # _init_column backfill existing rows with the UPGRADE user before the + # migration runs, and lets a client forge the value via a + # default_eval_as_user_id context key. The identity is set explicitly in + # create(); the migration backfills existing rows from create_uid. + + # Fields that decide what a rule matches or does. Changing any re-binds the + # evaluation identity to the editor (see write), so a rule can never be + # repointed to act beyond its editor's ticket scope. Deliberately excludes + # operational toggles (sequence, active): reordering or archiving/unarchiving + # a rule must not silently transfer ownership to the person doing that + # routine action (a manager cleaning up an officer's rule would otherwise + # re-bind it to the manager's broad scope). + _EVAL_TARGETING_FIELDS = ( + "condition_cel", + "assign_user_id", + "assign_team_id", + "set_severity", + "set_priority", + ) + + @api.model_create_multi + def create(self, vals_list): + """Force the evaluation identity to the creator; never client-supplied. + + Setting the key explicitly (rather than popping it) keeps the field + present in vals so 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. self.env.uid is the acting user, preserved + even under sudo() (only an explicit with_user() write re-widens). + """ + if "eval_as_user_id" in vals or any(f in vals for f in self._EVAL_TARGETING_FIELDS): + vals = dict(vals) + vals.pop("eval_as_user_id", None) + if any(f in vals for f in self._EVAL_TARGETING_FIELDS): + vals["eval_as_user_id"] = self.env.uid + return super().write(vals) + @api.constrains("condition_cel") def _check_condition_cel(self): """Validate CEL expression syntax using CEL parser.""" @@ -115,7 +171,9 @@ def _check_condition_cel(self): # Use proper CEL parser for validation P.parse(rule.condition_cel) # If parser not available, skip validation - except SyntaxError as e: + except Exception as e: + # Any parser failure (not only SyntaxError) is a bad + # expression the user must fix, surfaced as a ValidationError. raise ValidationError( _( "Invalid CEL expression in rule '%(rule_name)s': %(error)s", @@ -218,10 +276,16 @@ def _evaluate_expression(self, expression, context): _logger.warning("Expression evaluation error: %s", str(e)) raise - @api.model + @api.private def apply_routing(self, ticket): """Apply the first matching routing rule to a ticket. + Each rule is evaluated and applied with the identity of whoever defined + it (``eval_as_user_id``), so an elevated caller (the sudo'd portal + controller, an admin) can never make a rule act beyond its author's + ticket scope. Private: not RPC-dispatchable; call from trusted server + code only. + Args: ticket: spp.grm.ticket record @@ -232,8 +296,22 @@ def apply_routing(self, ticket): rules = self.search([("active", "=", True)], order="sequence, id") for rule in rules: - if rule.evaluate(ticket): - # Apply the rule's actions + owner = rule.eval_as_user_id or rule.create_uid + if not owner: + continue + # nosemgrep: semgrep.odoo-with-user-unvalidated -- owner is system-set in create()/write(), not client input + rule_as_owner = rule.with_user(owner.id) + # nosemgrep: semgrep.odoo-with-user-unvalidated -- owner is system-set; scopes ticket writes + ticket_as_owner = ticket.with_user(owner.id) + try: + matched = rule_as_owner.evaluate(ticket_as_owner) + except AccessError: + # The rule owner cannot see this ticket -> the rule does not + # apply to it. Correct behaviour, not an error. + continue + if matched: + # Apply the rule's actions as the owner: the ticket.write is + # bounded by the owner's record rules. vals = { "routing_rule_id": rule.id, # Track which rule was applied } @@ -250,7 +328,12 @@ def apply_routing(self, ticket): if rule.set_priority: vals["priority"] = rule.set_priority - ticket.write(vals) + try: + ticket_as_owner.write(vals) + except AccessError: + # Owner may match the ticket but not be allowed to write it + # (e.g. read-only scope). Skip rather than apply elevated. + continue _logger.info( "Applied routing rule '%s' to ticket %s: %s", rule.name, @@ -258,9 +341,16 @@ def apply_routing(self, ticket): vals, ) - # Update match count - # nosemgrep: semgrep.odoo-sudo-without-context -- counter update needs sudo - rule.sudo().write({"match_count": rule.match_count + 1}) + # Atomic increment: avoids lost updates under concurrent + # cron/UI escalation, and needs no sudo (raw SQL bypasses ACL). + # Invisible to spp_audit ORM write-hooks, which is acceptable + # for a statistics counter. + rule.flush_recordset(["match_count"]) + self.env.cr.execute( + "UPDATE spp_grm_routing_rule SET match_count = match_count + 1 WHERE id = %s", + (rule.id,), + ) + rule.invalidate_recordset(["match_count"]) # Only apply the first matching rule return True diff --git a/spp_grm_cel/readme/HISTORY.md b/spp_grm_cel/readme/HISTORY.md index 29b4e01b1..ec719d487 100644 --- a/spp_grm_cel/readme/HISTORY.md +++ b/spp_grm_cel/readme/HISTORY.md @@ -1,3 +1,21 @@ +### 19.0.2.0.2 + +- fix(security): GRM routing and escalation rules now evaluate with the identity of whoever + defined them (``eval_as_user_id``, system-managed), not as the superuser cron. An officer's + rule can only route/escalate tickets within that officer's own record-rule scope, closing the + path where an always-match rule applied by the hourly cron could seize every ticket in the + database (#379). Existing rules are backfilled from ``create_uid`` by a migration. +- fix(security): the rule-engine entry points (``apply_routing``, ``apply_escalations``, + ``apply_escalation``, ``check_escalations``) are marked ``@api.private`` — no longer callable + over RPC (#381). +- fix(security): drop the portal and internal-user read rows on both rule models. With + owner-identity evaluation the acting user never reads the rules, so the rows only exposed the + routing/escalation map (conditions, targets, thresholds) to enumeration (#380). +- fix: increment ``match_count`` / ``escalation_count`` with an atomic ``UPDATE`` instead of a + read-modify-write, so concurrent cron and UI escalations cannot lose counts. +- fix: rule CEL validation now reports any parser error as a ``ValidationError`` (previously only + ``SyntaxError`` was caught). + ### 19.0.2.0.1 - fix(security): restrict GRM routing and escalation rules to GRM staff. Portal users no longer diff --git a/spp_grm_cel/security/ir.model.access.csv b/spp_grm_cel/security/ir.model.access.csv index 5520d43c2..2a7d713c6 100644 --- a/spp_grm_cel/security/ir.model.access.csv +++ b/spp_grm_cel/security/ir.model.access.csv @@ -2,10 +2,6 @@ id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink access_spp_grm_routing_rule_viewer,GRM Routing Rule Viewer Access,model_spp_grm_routing_rule,spp_grm.group_grm_viewer,1,0,0,0 access_spp_grm_routing_rule_officer,GRM Routing Rule Officer Access,model_spp_grm_routing_rule,spp_grm.group_grm_officer,1,1,1,0 access_spp_grm_routing_rule_manager,GRM Routing Rule Manager Access,model_spp_grm_routing_rule,spp_grm.group_grm_manager,1,1,1,1 -access_spp_grm_routing_rule_base_user,GRM Routing Rule Base User Access,model_spp_grm_routing_rule,base.group_user,1,0,0,0 -access_spp_grm_routing_rule_portal_user,GRM Routing Rule Portal User Access,model_spp_grm_routing_rule,base.group_portal,1,0,0,0 access_spp_grm_escalation_rule_viewer,GRM Escalation Rule Viewer Access,model_spp_grm_escalation_rule,spp_grm.group_grm_viewer,1,0,0,0 access_spp_grm_escalation_rule_officer,GRM Escalation Rule Officer Access,model_spp_grm_escalation_rule,spp_grm.group_grm_officer,1,1,1,0 access_spp_grm_escalation_rule_manager,GRM Escalation Rule Manager Access,model_spp_grm_escalation_rule,spp_grm.group_grm_manager,1,1,1,1 -access_spp_grm_escalation_rule_base_user,GRM Escalation Rule Base User Access,model_spp_grm_escalation_rule,base.group_user,1,0,0,0 -access_spp_grm_escalation_rule_portal_user,GRM Escalation Rule Portal User Access,model_spp_grm_escalation_rule,base.group_portal,1,0,0,0 diff --git a/spp_grm_cel/tests/__init__.py b/spp_grm_cel/tests/__init__.py index bb606d642..5a8d20e2c 100644 --- a/spp_grm_cel/tests/__init__.py +++ b/spp_grm_cel/tests/__init__.py @@ -3,3 +3,4 @@ from . import test_routing_rules from . import test_escalation_rules from . import test_rule_acl +from . import test_rule_owner_identity diff --git a/spp_grm_cel/tests/test_rule_acl.py b/spp_grm_cel/tests/test_rule_acl.py index 44cfb6025..bebc4c566 100644 --- a/spp_grm_cel/tests/test_rule_acl.py +++ b/spp_grm_cel/tests/test_rule_acl.py @@ -9,14 +9,11 @@ therefore plant an always-matching rule via RPC and disrupt grievance handling globally. -Portal users must never hold write/create/unlink. The retained portal READ row -is a current implementation dependency, not a security requirement: rule -evaluation runs as the acting user, and portal users can reach it by creating -or stage-writing tickets over direct RPC (they hold write/create on -``spp.grm.ticket``), so dropping read today would silently skip routing and -escalation on those paths. Tightening it requires moving rule evaluation to -``sudo()`` first — tracked in OpenSPP2 issue #413 together with the missing -portal record rule on ``spp.grm.ticket``. GRM staff retain full management. +Portal users must never hold write/create/unlink — and, as of #379, no read +either: rules now evaluate with their owner's identity (``eval_as_user_id``), so +the acting/portal user never needs to read them. Dropping the portal and +base-user read rows closes the rule-enumeration surface (#380). GRM staff retain +full management. """ from odoo import Command @@ -81,35 +78,16 @@ def test_portal_user_cannot_write_escalation_rule(self): with self.assertRaises(AccessError): self.escalation_rule.with_user(self.portal_user).write({"name": "Hijacked"}) - def test_portal_user_can_read_rules(self): - """Portal read is a CURRENT IMPLEMENTATION DEPENDENCY, not a security - requirement: rule evaluation runs as the acting user, and portal users - reach it via direct-RPC ticket create/stage-write. When rule evaluation - moves to sudo() (issue #413), replace this with a read-denial test.""" - self.env[ROUTING_MODEL].with_user(self.portal_user).check_access("read") - self.env[ESCALATION_MODEL].with_user(self.portal_user).check_access("read") - - def test_rule_readonly_caller_escalation_increments_counter(self): - """A caller with read-only rule access must still get a fully applied - escalation — the counter write runs with elevated rights. Regression - test for the 19.0.2.0.1 sudo fix: with it reverted, the counter write - raises AccessError and this test fails loudly.""" - rule = self.env[ESCALATION_MODEL].create({"name": "Counter Rule", "condition_cel": "severity == 'critical'"}) - ticket = self.env["spp.grm.ticket"].create( - { - "name": "Counter Test Ticket", - "description": "Escalation counter regression", - "partner_id": self.portal_user.partner_id.id, - "severity": "critical", - } - ) - before = rule.escalation_count - applied = ( - self.env[ESCALATION_MODEL].with_user(self.portal_user).apply_escalations(ticket.with_user(self.portal_user)) - ) - self.assertTrue(applied) - self.assertEqual(rule.escalation_count, before + 1) - self.assertTrue(ticket.is_escalated) + def test_portal_user_cannot_read_rules(self): + """Portal users have no read on the rule models. Rules are evaluated with + their owner's identity (eval_as_user_id), so the acting/portal user never + needs to read them — closing the enumeration surface (#380). The escalation + counter write is exercised under owner identity in + test_rule_owner_identity.py.""" + with self.assertRaises(AccessError): + self.env[ROUTING_MODEL].with_user(self.portal_user).check_access("read") + with self.assertRaises(AccessError): + self.env[ESCALATION_MODEL].with_user(self.portal_user).check_access("read") def test_grm_manager_can_create_rules(self): """GRM staff must retain full management of both rule models.""" diff --git a/spp_grm_cel/tests/test_rule_owner_identity.py b/spp_grm_cel/tests/test_rule_owner_identity.py new file mode 100644 index 000000000..13abf0fb5 --- /dev/null +++ b/spp_grm_cel/tests/test_rule_owner_identity.py @@ -0,0 +1,175 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Security: GRM rules evaluate with their creator's identity, not superuser. + +Regression tests for the GRM rule cluster: + +- #379 (HIGH): the hourly escalation cron runs as superuser and applied rules + bypassing record rules, so an officer could author an always-match rule + escalating tickets to themselves and seize every ticket in the database. + Rules must evaluate and act with the identity of whoever defined them + (eval_as_user_id), so an officer's rule can only touch tickets the officer's + own record rule already permits. Mirrors the spp_alerts #364 owner-identity fix. +- #381 (Medium): apply_routing / apply_escalations / check_escalations were + public @api.model methods, RPC-dispatchable; they must be private. +""" + +from odoo import Command +from odoo.exceptions import AccessError +from odoo.tests import TransactionCase, tagged + +ROUTING = "spp.grm.routing.rule" +ESCALATION = "spp.grm.escalation.rule" + + +@tagged("post_install", "-at_install") +class TestGRMRuleOwnerIdentity(TransactionCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + Users = cls.env["res.users"] + internal = cls.env.ref("base.group_user") + officer = cls.env.ref("spp_grm.group_grm_officer") + manager = cls.env.ref("spp_grm.group_grm_manager") + + cls.officer = Users.create( + { + "name": "GRM Officer", + "login": "grm_owner_officer", + "group_ids": [Command.link(internal.id), Command.link(officer.id)], + } + ) + cls.manager = Users.create( + { + "name": "GRM Manager", + "login": "grm_owner_manager", + "group_ids": [Command.link(internal.id), Command.link(manager.id)], + } + ) + # Two teams; the officer is a member of team A only. + Team = cls.env["spp.grm.team"] + cls.team_a = Team.create({"name": "Team A", "member_ids": [Command.link(cls.officer.id)]}) + cls.team_b = Team.create({"name": "Team B"}) + + partner = cls.env["res.partner"].create({"name": "Complainant"}) + Ticket = cls.env["spp.grm.ticket"] + # A ticket owned by team B — outside the officer's record-rule scope. + cls.foreign_ticket = Ticket.create( + { + "name": "Foreign grievance", + "description": "Assigned to team B, officer cannot see it", + "partner_id": partner.id, + "team_id": cls.team_b.id, + "severity": "critical", + } + ) + + def test_officer_rule_cannot_seize_foreign_ticket(self): + """#379: an officer's always-match escalation rule, applied by the + superuser cron, must NOT escalate/reassign a ticket the officer cannot + see. Owner-identity evaluation bounds the rule to the officer's scope.""" + self.env[ESCALATION].with_user(self.officer).create( + { + "name": "Seize everything", + "condition_cel": "", # empty == always matches + "escalate_to_user_id": self.officer.id, + "trigger_after_hours": 0, + } + ) + # Run the cron path as superuser (as ir.cron would). + self.env[ESCALATION].sudo().check_escalations() + self.foreign_ticket.invalidate_recordset() + self.assertFalse( + self.foreign_ticket.is_escalated, + "Officer-authored rule must not escalate a ticket outside the officer's scope", + ) + self.assertNotEqual( + self.foreign_ticket.user_id, + self.officer, + "Officer-authored rule must not reassign a ticket it cannot see", + ) + + def test_manager_rule_applies_broadly(self): + """A manager (broad record-rule scope) authoring the same rule DOES + escalate — owner identity does not over-restrict legitimate rules.""" + self.env[ESCALATION].with_user(self.manager).create( + { + "name": "Manager broad rule", + "condition_cel": "", + "escalate_severity": "high", + "trigger_after_hours": 0, + } + ) + self.env[ESCALATION].sudo().check_escalations() + self.foreign_ticket.invalidate_recordset() + self.assertTrue( + self.foreign_ticket.is_escalated, + "Manager-authored rule should apply across teams", + ) + + def test_eval_as_user_id_set_to_creator(self): + rule = ( + self.env[ROUTING].with_user(self.officer).create({"name": "R", "condition_cel": "severity == 'critical'"}) + ) + self.assertEqual(rule.eval_as_user_id, self.officer) + + def test_eval_as_user_id_not_forgeable_via_context(self): + rule = ( + self.env[ROUTING] + .with_user(self.officer) + .with_context(default_eval_as_user_id=self.env.ref("base.user_admin").id) + .create({"name": "R2", "condition_cel": "severity == 'critical'"}) + ) + self.assertEqual(rule.eval_as_user_id, self.officer) + + def test_eval_as_user_id_not_writable_and_rebinds_on_retarget(self): + rule = self.env[ROUTING].with_user(self.manager).create({"name": "R3", "condition_cel": "severity == 'low'"}) + # Direct write of the identity is ignored. + rule.with_user(self.officer).write({"eval_as_user_id": self.env.ref("base.user_admin").id}) + rule.invalidate_recordset() + self.assertNotEqual(rule.eval_as_user_id, self.env.ref("base.user_admin")) + # Changing what the rule targets re-binds identity to the editor. + rule.with_user(self.officer).write({"condition_cel": "severity == 'high'"}) + rule.invalidate_recordset() + self.assertEqual(rule.eval_as_user_id, self.officer) + + def test_operational_toggle_does_not_rebind_owner(self): + """Reordering or archiving/unarchiving a rule is a routine action that + must NOT transfer ownership: otherwise a manager cleaning up an + officer's rule would silently re-bind it to the manager's broad scope + (a confused-deputy escalation).""" + rule = self.env[ESCALATION].with_user(self.officer).create( + {"name": "Officer rule", "condition_cel": "severity == 'critical'"} + ) + self.assertEqual(rule.eval_as_user_id, self.officer) + # Manager archives then re-enables and reorders it — owner stays the officer. + rule.with_user(self.manager).write({"active": False}) + rule.with_user(self.manager).write({"active": True, "sequence": 99}) + rule.invalidate_recordset() + self.assertEqual( + rule.eval_as_user_id, + self.officer, + "Toggling active/sequence must not re-author rule ownership", + ) + + def test_escalation_counter_increments_under_owner_identity(self): + """The escalation counter is incremented (atomically) when a manager's + rule applies via the elevated cron path.""" + rule = self.env[ESCALATION].with_user(self.manager).create( + {"name": "Counter rule", "condition_cel": "", "escalate_severity": "high"} + ) + before = rule.escalation_count + self.env[ESCALATION].sudo().check_escalations() + rule.invalidate_recordset() + self.assertEqual(rule.escalation_count, before + 1) + + def test_entry_points_not_rpc_callable(self): + """#381: the three rule-engine methods must be rejected for RPC dispatch.""" + from odoo.service.model import call_kw + + for model, method, args in [ + (ROUTING, "apply_routing", [self.foreign_ticket.id]), + (ESCALATION, "apply_escalations", [self.foreign_ticket.id]), + (ESCALATION, "check_escalations", []), + ]: + with self.assertRaises(AccessError): + call_kw(self.env[model], method, args, {}) diff --git a/spp_grm_cel/views/grm_escalation_rule_views.xml b/spp_grm_cel/views/grm_escalation_rule_views.xml index 22c75f0f2..32a324ee9 100644 --- a/spp_grm_cel/views/grm_escalation_rule_views.xml +++ b/spp_grm_cel/views/grm_escalation_rule_views.xml @@ -51,6 +51,7 @@ + diff --git a/spp_grm_cel/views/grm_routing_rule_views.xml b/spp_grm_cel/views/grm_routing_rule_views.xml index d8627817f..64d949e5d 100644 --- a/spp_grm_cel/views/grm_routing_rule_views.xml +++ b/spp_grm_cel/views/grm_routing_rule_views.xml @@ -52,6 +52,7 @@ + From f6861ca0f75673da73fdecf84d8fe82b4a3f3ae5 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 14 Aug 2026 00:28:30 +0800 Subject: [PATCH 02/15] fix(spp_grm): scope portal users to their own grievance tickets (#380) spp.grm.ticket granted base.group_portal read/write/create with no ir.rule targeting portal, so any portal user could read and rewrite every grievance in the system over RPC (the controller's partner scoping is presentation-only). - New portal record rule: partner_id == user.partner_id (own tickets only). - Portal ACL row reduced to read-only (1,0,0,0); submission is handled by the sudo'd portal controller, which needs no direct model write/create. - New tests/test_portal_ticket_acl.py: portal cannot read/search/write/create others' tickets; can read own. --- spp_grm/__manifest__.py | 2 +- spp_grm/readme/HISTORY.md | 8 +++ spp_grm/security/ir.model.access.csv | 2 +- spp_grm/security/rules.xml | 16 +++++ spp_grm/tests/__init__.py | 1 + spp_grm/tests/test_portal_ticket_acl.py | 87 +++++++++++++++++++++++++ 6 files changed, 114 insertions(+), 2 deletions(-) create mode 100644 spp_grm/tests/test_portal_ticket_acl.py diff --git a/spp_grm/__manifest__.py b/spp_grm/__manifest__.py index 179c43009..c0ec11cde 100644 --- a/spp_grm/__manifest__.py +++ b/spp_grm/__manifest__.py @@ -3,7 +3,7 @@ { "name": "OpenSPP - Grievance Redress Mechanism", "summary": "Provides a centralized Grievance Redress Mechanism for receiving, tracking, and resolving beneficiary complaints and feedback. It supports multi-channel submission, manages resolution workflows through customizable stages, and links grievances directly to individual or group registrants.", - "version": "19.0.2.0.1", + "version": "19.0.2.0.2", "sequence": 1, "author": "OpenSPP.org", "website": "https://github.com/OpenSPP/OpenSPP2", diff --git a/spp_grm/readme/HISTORY.md b/spp_grm/readme/HISTORY.md index ffafdb8f6..20f52cc01 100644 --- a/spp_grm/readme/HISTORY.md +++ b/spp_grm/readme/HISTORY.md @@ -1,3 +1,11 @@ +### 19.0.2.0.2 + +- fix(security): portal users can now only access their OWN grievance tickets. The + ``spp.grm.ticket`` portal access was read/write/create with no record rule, so any portal user + could read and rewrite every grievance in the system over RPC (#380). Added a portal record rule + scoping to the user's own partner and reduced the portal access-control entry to read-only + (submission is handled by the sudo'd portal controller, which needs no direct model write). + ### 19.0.2.0.1 - fix(views): gate the "Helpdesk" top-level menu (`spp_grm_ticket_main_menu`) on `group_grm_viewer`. Previously the root menu had no `groups=` attribute and was visible to every logged-in user; the OP#951 menu audit requires several roles to NOT see it (Registry Viewer, Global Finance, Global Program Manager, Program Viewer/Validator/Cycle Approver, Global Registrar, CR roles, Farm User/Manager). diff --git a/spp_grm/security/ir.model.access.csv b/spp_grm/security/ir.model.access.csv index 4b1393e91..833763453 100644 --- a/spp_grm/security/ir.model.access.csv +++ b/spp_grm/security/ir.model.access.csv @@ -3,7 +3,7 @@ access_spp_grm_ticket_viewer,GRM Ticket Viewer Access,model_spp_grm_ticket,group access_spp_grm_ticket_officer,GRM Ticket Officer Access,model_spp_grm_ticket,group_grm_officer,1,1,1,0 access_spp_grm_ticket_manager,GRM Ticket Manager Access,model_spp_grm_ticket,group_grm_manager,1,1,1,1 access_spp_grm_ticket_base_user,GRM Ticket Base User Access,model_spp_grm_ticket,base.group_user,1,0,0,0 -access_spp_grm_ticket_portal_user,GRM Ticket Portal User Access,model_spp_grm_ticket,base.group_portal,1,1,1,0 +access_spp_grm_ticket_portal_user,GRM Ticket Portal User Access,model_spp_grm_ticket,base.group_portal,1,0,0,0 access_spp_grm_ticket_stage_viewer,GRM Ticket Stage Viewer Access,model_spp_grm_ticket_stage,group_grm_viewer,1,0,0,0 access_spp_grm_ticket_stage_officer,GRM Ticket Stage Officer Access,model_spp_grm_ticket_stage,group_grm_officer,1,0,0,0 diff --git a/spp_grm/security/rules.xml b/spp_grm/security/rules.xml index 9191c4979..3f60375e2 100644 --- a/spp_grm/security/rules.xml +++ b/spp_grm/security/rules.xml @@ -104,4 +104,20 @@ [(1, '=', 1)] + + + + GRM Ticket: Portal Own Tickets Only + + [('partner_id', '=', user.partner_id.id)] + + + + + + diff --git a/spp_grm/tests/__init__.py b/spp_grm/tests/__init__.py index 376402d2a..cfab33a28 100644 --- a/spp_grm/tests/__init__.py +++ b/spp_grm/tests/__init__.py @@ -2,3 +2,4 @@ from . import test_grm_ticket_stage from . import test_res_partner from . import test_grm_security +from . import test_portal_ticket_acl diff --git a/spp_grm/tests/test_portal_ticket_acl.py b/spp_grm/tests/test_portal_ticket_acl.py new file mode 100644 index 000000000..73ca2ccdb --- /dev/null +++ b/spp_grm/tests/test_portal_ticket_acl.py @@ -0,0 +1,87 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Security: portal users must only reach their OWN grievance tickets. + +Regression test for #380: spp.grm.ticket granted base.group_portal read/write/create +with NO ir.rule targeting portal, so a portal user could read and rewrite every +grievance in the system over RPC. The controller's partner_id scoping is +presentation-only. Fix: a portal record rule scoping to the user's own partner, and +the portal ACL row reduced to read-only (portal submission runs through the sudo'd +controller, which needs no direct model write/create). +""" + +from odoo import Command +from odoo.exceptions import AccessError +from odoo.tests import TransactionCase, tagged + + +@tagged("post_install", "-at_install") +class TestPortalTicketAcl(TransactionCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + Users = cls.env["res.users"] + cls.portal_a = Users.create( + { + "name": "Portal A", + "login": "grm_portal_a", + "group_ids": [Command.link(cls.env.ref("base.group_portal").id)], + } + ) + cls.portal_b = Users.create( + { + "name": "Portal B", + "login": "grm_portal_b", + "group_ids": [Command.link(cls.env.ref("base.group_portal").id)], + } + ) + Ticket = cls.env["spp.grm.ticket"] + cls.ticket_a = Ticket.create( + { + "name": "A's grievance", + "description": "Private to A", + "partner_id": cls.portal_a.partner_id.id, + } + ) + cls.ticket_b = Ticket.create( + { + "name": "B's grievance", + "description": "Private to B", + "partner_id": cls.portal_b.partner_id.id, + } + ) + + def test_portal_can_read_own_ticket(self): + """A portal user reads their own grievance (controller-created).""" + own = self.ticket_a.with_user(self.portal_a) + self.assertEqual(own.name, "A's grievance") + + def test_portal_cannot_read_others_ticket(self): + """A portal user must NOT be able to read another user's grievance.""" + with self.assertRaises(AccessError): + self.ticket_b.with_user(self.portal_a).read(["name"]) + + def test_portal_cannot_search_others_ticket(self): + """search must not surface other users' grievances to a portal user.""" + visible = self.env["spp.grm.ticket"].with_user(self.portal_a).search([]) + self.assertIn(self.ticket_a, visible) + self.assertNotIn(self.ticket_b, visible) + + def test_portal_cannot_write_any_ticket(self): + """Portal ACL is read-only: no write on own or others' tickets over RPC + (edits go through the controller, not direct model writes).""" + with self.assertRaises(AccessError): + self.ticket_a.with_user(self.portal_a).write({"name": "tampered"}) + with self.assertRaises(AccessError): + self.ticket_b.with_user(self.portal_a).write({"name": "hijacked"}) + + def test_portal_cannot_create_ticket_directly(self): + """Portal ACL is read-only: direct model create is denied (submission is + controller-mediated via sudo).""" + with self.assertRaises(AccessError): + self.env["spp.grm.ticket"].with_user(self.portal_a).create( + { + "name": "direct", + "description": "bypass controller", + "partner_id": self.portal_a.partner_id.id, + } + ) From 341966a3f4858571333cc62545eed7b0d3795272 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 14 Aug 2026 00:42:30 +0800 Subject: [PATCH 03/15] docs,test: regenerate GRM READMEs from CI generator, ruff-format test (#415) README.rst / index.html for spp_grm and spp_grm_cel applied verbatim from the pre-commit CI run's printed diff (local RST regen is not byte-reproducible). test_rule_owner_identity.py reformatted per ruff-format. --- spp_grm/README.rst | 11 ++++++++ spp_grm/static/description/index.html | 14 +++++++++- spp_grm_cel/README.rst | 23 ++++++++++++++++ spp_grm_cel/static/description/index.html | 26 ++++++++++++++++++- spp_grm_cel/tests/test_rule_owner_identity.py | 12 ++++++--- 5 files changed, 80 insertions(+), 6 deletions(-) diff --git a/spp_grm/README.rst b/spp_grm/README.rst index d3502d33a..7edcf3c2e 100644 --- a/spp_grm/README.rst +++ b/spp_grm/README.rst @@ -153,6 +153,17 @@ Dependencies Changelog ========= +19.0.2.0.2 +~~~~~~~~~~ + +- fix(security): portal users can now only access their OWN grievance + tickets. The ``spp.grm.ticket`` portal access was read/write/create + with no record rule, so any portal user could read and rewrite every + grievance in the system over RPC (#380). Added a portal record rule + scoping to the user's own partner and reduced the portal + access-control entry to read-only (submission is handled by the sudo'd + portal controller, which needs no direct model write). + 19.0.2.0.1 ~~~~~~~~~~ diff --git a/spp_grm/static/description/index.html b/spp_grm/static/description/index.html index 0d1621de8..4439520bc 100644 --- a/spp_grm/static/description/index.html +++ b/spp_grm/static/description/index.html @@ -536,6 +536,18 @@

Changelog

+

19.0.2.0.2

+
    +
  • fix(security): portal users can now only access their OWN grievance +tickets. The spp.grm.ticket portal access was read/write/create +with no record rule, so any portal user could read and rewrite every +grievance in the system over RPC (#380). Added a portal record rule +scoping to the user’s own partner and reduced the portal +access-control entry to read-only (submission is handled by the sudo’d +portal controller, which needs no direct model write).
    • +
+
+

19.0.2.0.1

  • fix(views): gate the “Helpdesk” top-level menu @@ -547,7 +559,7 @@

    19.0.2.0.1

    Farm User/Manager).
-
+

19.0.2.0.0

  • Initial migration to OpenSPP2
    • diff --git a/spp_grm_cel/README.rst b/spp_grm_cel/README.rst index 4324e16bf..03fcef7e6 100644 --- a/spp_grm_cel/README.rst +++ b/spp_grm_cel/README.rst @@ -124,6 +124,29 @@ Dependencies Changelog ========= +19.0.2.0.2 +~~~~~~~~~~ + +- fix(security): GRM routing and escalation rules now evaluate with the + identity of whoever defined them (``eval_as_user_id``, + system-managed), not as the superuser cron. An officer's rule can only + route/escalate tickets within that officer's own record-rule scope, + closing the path where an always-match rule applied by the hourly cron + could seize every ticket in the database (#379). Existing rules are + backfilled from ``create_uid`` by a migration. +- fix(security): the rule-engine entry points (``apply_routing``, + ``apply_escalations``, ``apply_escalation``, ``check_escalations``) + are marked ``@api.private`` — no longer callable over RPC (#381). +- fix(security): drop the portal and internal-user read rows on both + rule models. With owner-identity evaluation the acting user never + reads the rules, so the rows only exposed the routing/escalation map + (conditions, targets, thresholds) to enumeration (#380). +- fix: increment ``match_count`` / ``escalation_count`` with an atomic + ``UPDATE`` instead of a read-modify-write, so concurrent cron and UI + escalations cannot lose counts. +- fix: rule CEL validation now reports any parser error as a + ``ValidationError`` (previously only ``SyntaxError`` was caught). + 19.0.2.0.1 ~~~~~~~~~~ diff --git a/spp_grm_cel/static/description/index.html b/spp_grm_cel/static/description/index.html index d14b31b8a..4e2607fd0 100644 --- a/spp_grm_cel/static/description/index.html +++ b/spp_grm_cel/static/description/index.html @@ -511,6 +511,30 @@

    Changelog

+

19.0.2.0.2

+
    +
  • fix(security): GRM routing and escalation rules now evaluate with the +identity of whoever defined them (eval_as_user_id, +system-managed), not as the superuser cron. An officer’s rule can only +route/escalate tickets within that officer’s own record-rule scope, +closing the path where an always-match rule applied by the hourly cron +could seize every ticket in the database (#379). Existing rules are +backfilled from create_uid by a migration.
    • +
  • fix(security): the rule-engine entry points (apply_routing, +apply_escalations, apply_escalation, check_escalations) +are marked @api.private — no longer callable over RPC (#381).
    • +
  • fix(security): drop the portal and internal-user read rows on both +rule models. With owner-identity evaluation the acting user never +reads the rules, so the rows only exposed the routing/escalation map +(conditions, targets, thresholds) to enumeration (#380).
    • +
  • fix: increment match_count / escalation_count with an atomic +UPDATE instead of a read-modify-write, so concurrent cron and UI +escalations cannot lose counts.
    • +
  • fix: rule CEL validation now reports any parser error as a +ValidationError (previously only SyntaxError was caught).
    • +
+
+

19.0.2.0.1

  • fix(security): restrict GRM routing and escalation rules to GRM staff. @@ -527,7 +551,7 @@

    19.0.2.0.1

    skipped) are pre-existing and unchanged.
-
+

19.0.2.0.0

  • Initial migration to OpenSPP2
    • diff --git a/spp_grm_cel/tests/test_rule_owner_identity.py b/spp_grm_cel/tests/test_rule_owner_identity.py index 13abf0fb5..eb32d411e 100644 --- a/spp_grm_cel/tests/test_rule_owner_identity.py +++ b/spp_grm_cel/tests/test_rule_owner_identity.py @@ -137,8 +137,10 @@ def test_operational_toggle_does_not_rebind_owner(self): must NOT transfer ownership: otherwise a manager cleaning up an officer's rule would silently re-bind it to the manager's broad scope (a confused-deputy escalation).""" - rule = self.env[ESCALATION].with_user(self.officer).create( - {"name": "Officer rule", "condition_cel": "severity == 'critical'"} + rule = ( + self.env[ESCALATION] + .with_user(self.officer) + .create({"name": "Officer rule", "condition_cel": "severity == 'critical'"}) ) self.assertEqual(rule.eval_as_user_id, self.officer) # Manager archives then re-enables and reorders it — owner stays the officer. @@ -154,8 +156,10 @@ def test_operational_toggle_does_not_rebind_owner(self): def test_escalation_counter_increments_under_owner_identity(self): """The escalation counter is incremented (atomically) when a manager's rule applies via the elevated cron path.""" - rule = self.env[ESCALATION].with_user(self.manager).create( - {"name": "Counter rule", "condition_cel": "", "escalate_severity": "high"} + rule = ( + self.env[ESCALATION] + .with_user(self.manager) + .create({"name": "Counter rule", "condition_cel": "", "escalate_severity": "high"}) ) before = rule.escalation_count self.env[ESCALATION].sudo().check_escalations() From 1bbcdf6d4b9eea3d13ceab8667340dd9e92492c7 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Tue, 1 Sep 2026 12:28:00 +0800 Subject: [PATCH 04/15] =?UTF-8?q?fix(grm):=20address=20PR=20#415=20review?= =?UTF-8?q?=20=E2=80=94=20atomic=20escalations,=20silent-skip=20logging,?= =?UTF-8?q?=20scoped=20test=20surface?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review-response fixes (kneckinator, 15 threads; verification dossier in internal docs): - apply_escalation is now atomic: a savepoint rolls the whole escalation back when a post-write step is denied (e.g. an officer's rule reassigns the ticket out of the officer's own scope, then the chatter post fails), instead of persisting a half-applied escalation with its message lost. The chatter post moves ahead of the external notification so a rolled- back escalation can never have already sent an email. Regression test. - Rules with no evaluation identity, superuser-owned rules (which evaluate with record rules bypassed), and owner-access skips are now logged; the migration calls out uid-1 backfills by name. - The legacy routing/escalation suites author rules as a GRM manager instead of the superuser test env, so they exercise the owner-scoped path rather than the bypass; new tests pin base-user read denial on the rule models, the read-side (evaluate) access bound, apply_escalation's RPC guard, and the out-of-scope rollback. - Portal ticket rule: all four perms enabled so the scoping holds if a future ACL change re-grants portal write; comment rewritten. - compliance.yaml: portal access description corrected; declare rule_spp_grm_ticket_portal and rule_spp_grm_ticket_officer_create. - Escalation cron searches the active-rule set once per pass, not once per open ticket. - HISTORY: #380 misattribution on the rule-model row drop corrected (ticket-side portal scoping is #380; the row drop is #379/#381 hardening); ondelete=restrict user-deletion consequence documented. - Command.link for the escalation m2m; counter comments now describe the serialization-failure/retry rationale accurately; owner-identity create/write docstrings re-synced between the two rule models. --- spp_grm/readme/HISTORY.md | 4 +- spp_grm/security/compliance.yaml | 25 ++++- spp_grm/security/rules.xml | 14 ++- .../migrations/19.0.2.0.2/post-migration.py | 19 ++++ spp_grm_cel/models/grm_escalation_rule.py | 106 ++++++++++++++---- spp_grm_cel/models/grm_routing_rule.py | 41 ++++++- spp_grm_cel/readme/HISTORY.md | 19 +++- spp_grm_cel/tests/test_escalation_rules.py | 20 +++- spp_grm_cel/tests/test_routing_rules.py | 19 +++- spp_grm_cel/tests/test_rule_acl.py | 28 ++++- spp_grm_cel/tests/test_rule_owner_identity.py | 76 ++++++++++++- 11 files changed, 317 insertions(+), 54 deletions(-) diff --git a/spp_grm/readme/HISTORY.md b/spp_grm/readme/HISTORY.md index 20f52cc01..6e74eff15 100644 --- a/spp_grm/readme/HISTORY.md +++ b/spp_grm/readme/HISTORY.md @@ -4,7 +4,9 @@ ``spp.grm.ticket`` portal access was read/write/create with no record rule, so any portal user could read and rewrite every grievance in the system over RPC (#380). Added a portal record rule scoping to the user's own partner and reduced the portal access-control entry to read-only - (submission is handled by the sudo'd portal controller, which needs no direct model write). + (submission is handled by the sudo'd portal controller, which needs no direct model write). The + rule covers all four operations, so the scoping also holds if a future access-control change + ever re-grants portal write. ### 19.0.2.0.1 diff --git a/spp_grm/security/compliance.yaml b/spp_grm/security/compliance.yaml index 67bd19248..d50b69c76 100644 --- a/spp_grm/security/compliance.yaml +++ b/spp_grm/security/compliance.yaml @@ -144,6 +144,28 @@ record_rules: perm_create: true perm_unlink: false + # Officer - unscoped create only (new tickets have no assignment yet) + - id: rule_spp_grm_ticket_officer_create + model: spp.grm.ticket + groups: [group_grm_officer] + domain_description: "Officer can create tickets regardless of assignment" + perm_read: false + perm_write: false + perm_create: true + perm_unlink: false + + # Portal - own tickets only (#380); all perms so scoping holds if a + # future ACL change ever re-grants portal write/create/unlink + - id: rule_spp_grm_ticket_portal + model: spp.grm.ticket + groups: [base.group_portal] + domain_description: + "Portal user can only access their own tickets (partner_id = user.partner_id)" + perm_read: true + perm_write: true + perm_create: true + perm_unlink: true + # Supervisor - team tickets they supervise - id: rule_spp_grm_ticket_supervisor model: spp.grm.ticket @@ -287,7 +309,8 @@ actions: [] # # Additional Access: # - base.group_user: Read-only access to tickets (for internal users) -# - base.group_portal: Create/edit own tickets (for portal/self-service) +# - base.group_portal: Read own tickets only (submission goes through the +# sudo'd portal controller; no direct model write/create — see #380) # # Record Rules: # - Viewer: user_id = current user diff --git a/spp_grm/security/rules.xml b/spp_grm/security/rules.xml index 3f60375e2..34095e0a2 100644 --- a/spp_grm/security/rules.xml +++ b/spp_grm/security/rules.xml @@ -107,17 +107,19 @@ + read every grievance in the system (#380). Portal submission is mediated + by the sudo'd controller, so the ACL grants no direct write/create; all + four perms are enabled here so the scoping also holds for write/create/ + unlink if any future ACL change re-grants those (a rule with a perm + disabled simply does not apply to that operation). --> GRM Ticket: Portal Own Tickets Only [('partner_id', '=', user.partner_id.id)] - - - + + + diff --git a/spp_grm_cel/migrations/19.0.2.0.2/post-migration.py b/spp_grm_cel/migrations/19.0.2.0.2/post-migration.py index 8086cb156..947f15f3f 100644 --- a/spp_grm_cel/migrations/19.0.2.0.2/post-migration.py +++ b/spp_grm_cel/migrations/19.0.2.0.2/post-migration.py @@ -33,3 +33,22 @@ def migrate(cr, version): routing, escalation, ) + + # Rules created from privileged contexts (odoo shell, import scripts, data + # loads) carry create_uid = 1, and a superuser owner evaluates with record + # rules bypassed (with_user(SUPERUSER_ID) is always superuser mode). Call + # these out specifically: they stay unbounded until re-saved by a real user. + cr.execute( + "SELECT id, name FROM spp_grm_routing_rule WHERE eval_as_user_id = 1 " + "UNION ALL " + "SELECT id, name FROM spp_grm_escalation_rule WHERE eval_as_user_id = 1" + ) + superuser_rules = cr.fetchall() + if superuser_rules: + _logger.warning( + "%s GRM rule(s) are owned by the superuser and will evaluate WITHOUT " + "record-rule bounds: %s. Re-save each as the user who should own it " + "to scope its evaluation.", + len(superuser_rules), + ", ".join(f"{name!r} (id {rid})" for rid, name in superuser_rules), + ) diff --git a/spp_grm_cel/models/grm_escalation_rule.py b/spp_grm_cel/models/grm_escalation_rule.py index ddcb4c621..74dc0b1da 100644 --- a/spp_grm_cel/models/grm_escalation_rule.py +++ b/spp_grm_cel/models/grm_escalation_rule.py @@ -1,6 +1,6 @@ import logging -from odoo import _, api, fields, models +from odoo import SUPERUSER_ID, Command, _, api, fields, models from odoo.exceptions import AccessError, ValidationError _logger = logging.getLogger(__name__) @@ -177,15 +177,21 @@ class GRMEscalationRule(models.Model): @api.model_create_multi def create(self, vals_list): - """Force the evaluation identity to the creator; never client-supplied.""" + """Force the evaluation identity to the creator; never client-supplied. + + Setting the key explicitly (rather than popping it) keeps the field + present in vals so 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. - self.env.uid is the acting user, preserved even under sudo() (only an - explicit with_user() write re-widens scope). + eval_as_user_id is never client-writable directly; it tracks whoever last + defined what the rule targets. self.env.uid is the acting user, preserved + even under sudo() (only an explicit with_user() write re-widens). """ if "eval_as_user_id" in vals or any(f in vals for f in self._EVAL_TARGETING_FIELDS): vals = dict(vals) @@ -389,7 +395,7 @@ def apply_escalation(self, ticket): ticket.write(vals) # Track which escalation rule was applied (add to many2many) - ticket.write({"escalation_rule_ids": [(4, self.id)]}) + ticket.write({"escalation_rule_ids": [Command.link(self.id)]}) _logger.info( "Applied escalation rule '%s' to ticket %s: %s", @@ -398,6 +404,19 @@ def apply_escalation(self, ticket): vals, ) + # Post to chatter BEFORE any external side effect: posting needs write + # access on the ticket, which the rule owner may have just lost by + # reassigning it out of their own scope. When that happens the caller's + # savepoint rolls everything back — and because nothing external has + # fired yet, no ghost notification announces a rolled-back escalation. + ticket.message_post( + body=_( + "Ticket escalated by rule: %(rule_name)s", + rule_name=self.name, + ), + subject=_("Ticket Escalated"), + ) + # Send notification if configured if self.should_send_notification and self.notification_template_id: self._send_escalation_notification(ticket) @@ -406,9 +425,12 @@ def apply_escalation(self, ticket): if self.create_case and self.case_type_id: self._create_case_from_ticket(ticket) - # Atomic increment: avoids lost updates under concurrent cron/UI - # escalation, and needs no sudo (raw SQL bypasses ACL). Invisible to - # spp_audit ORM write-hooks, which is acceptable for a stats counter. + # Atomic increment: a read-modify-write here would raise a serialization + # failure under concurrent cron/UI escalation (cursors run REPEATABLE + # READ), and the only auto-retry is the whole dispatch — re-running the + # entire cron pass and re-firing notifications. A single UPDATE avoids + # the conflict entirely, and needs no sudo (raw SQL bypasses ACL). + # Invisible to spp_audit ORM write-hooks, acceptable for a stats counter. self.flush_recordset(["escalation_count"]) self.env.cr.execute( "UPDATE spp_grm_escalation_rule SET escalation_count = escalation_count + 1 WHERE id = %s", @@ -416,15 +438,6 @@ def apply_escalation(self, ticket): ) self.invalidate_recordset(["escalation_count"]) - # Post message to chatter - ticket.message_post( - body=_( - "Ticket escalated by rule: %(rule_name)s", - rule_name=self.name, - ), - subject=_("Ticket Escalated"), - ) - return True def _send_escalation_notification(self, ticket): @@ -516,9 +529,13 @@ def check_escalations(self): _logger.info("Checking escalation rules for %d open tickets", len(tickets)) + # Hoisted out of the per-ticket loop: the active-rule set is identical + # for every ticket in this pass, so search it once, not once per ticket. + rules = self.search([("active", "=", True)], order="sequence, id") + escalated_count = 0 for ticket in tickets: - if self.apply_escalations(ticket): + if self.apply_escalations(ticket, rules=rules): escalated_count += 1 _logger.info("Escalated %d tickets", escalated_count) @@ -526,7 +543,7 @@ def check_escalations(self): @api.model @api.private - def apply_escalations(self, ticket): + def apply_escalations(self, ticket, rules=None): """Apply all matching escalation rules to a ticket. Each rule is evaluated and applied with the identity of whoever defined @@ -536,18 +553,39 @@ def apply_escalations(self, ticket): Args: ticket: spp.grm.ticket record + rules: optional pre-searched active rules in ``sequence, id`` order; + a caller looping many tickets (the cron) passes them once + instead of re-searching per ticket Returns: bool: True if any rule was applied, False otherwise """ - # Search for active rules in sequence order - rules = self.search([("active", "=", True)], order="sequence, id") + if rules is None: + # Search for active rules in sequence order + rules = self.search([("active", "=", True)], order="sequence, id") applied = False for rule in rules: owner = rule.eval_as_user_id or rule.create_uid if not owner: + _logger.warning( + "Escalation rule %s (id %s) has no evaluation identity (owner and " + "create_uid both unset); skipping — it will never fire until re-saved.", + rule.name, + rule.id, + ) continue + if owner.id == SUPERUSER_ID: + # with_user(SUPERUSER_ID) always runs in superuser mode (record + # rules bypassed), so this rule evaluates unrestricted. Only + # privileged contexts (shell, data load, migration from a + # script-created rule) can mint such an owner — surface it. + _logger.warning( + "Escalation rule %s (id %s) is owned by the superuser and evaluates " + "without record-rule bounds; re-save it as a real user to scope it.", + rule.name, + rule.id, + ) # nosemgrep: semgrep.odoo-with-user-unvalidated -- owner is system-set in create()/write(), not client input rule_as_owner = rule.with_user(owner.id) # nosemgrep: semgrep.odoo-with-user-unvalidated -- owner is system-set; scopes ticket writes @@ -557,13 +595,33 @@ def apply_escalations(self, ticket): except AccessError: # The rule owner cannot see this ticket -> the rule does not # apply to it. Correct behaviour, not an error. + _logger.debug( + "Escalation rule %s: owner %s cannot read ticket %s; rule does not apply.", + rule.name, + owner.login, + ticket.id, + ) continue if matched: try: - rule_as_owner.apply_escalation(ticket_as_owner) + # Savepoint: apply_escalation has side effects after its + # ticket writes (notification, case creation, counter, + # chatter post), any of which can raise AccessError once the + # write itself has reassigned the ticket out of the owner's + # own scope. Roll all of it back rather than leave a + # half-applied escalation with a lost chatter message. + with self.env.cr.savepoint(): + rule_as_owner.apply_escalation(ticket_as_owner) except AccessError: - # Owner matched but cannot write this ticket -> skip rather - # than apply with elevated rights. + # Owner matched but cannot write this ticket (or lost access + # mid-apply) -> skip rather than apply with elevated rights. + _logger.info( + "Escalation rule %s: owner %s lacks write access on ticket %s; " + "escalation rolled back and skipped.", + rule.name, + owner.login, + ticket.id, + ) continue applied = True # Continue checking other rules (unlike routing, multiple escalations can apply) diff --git a/spp_grm_cel/models/grm_routing_rule.py b/spp_grm_cel/models/grm_routing_rule.py index 73a762ccf..33a39c009 100644 --- a/spp_grm_cel/models/grm_routing_rule.py +++ b/spp_grm_cel/models/grm_routing_rule.py @@ -1,6 +1,6 @@ import logging -from odoo import _, api, fields, models +from odoo import SUPERUSER_ID, _, api, fields, models from odoo.exceptions import AccessError, ValidationError _logger = logging.getLogger(__name__) @@ -298,7 +298,24 @@ def apply_routing(self, ticket): for rule in rules: owner = rule.eval_as_user_id or rule.create_uid if not owner: + _logger.warning( + "Routing rule %s (id %s) has no evaluation identity (owner and " + "create_uid both unset); skipping — it will never fire until re-saved.", + rule.name, + rule.id, + ) continue + if owner.id == SUPERUSER_ID: + # with_user(SUPERUSER_ID) always runs in superuser mode (record + # rules bypassed), so this rule evaluates unrestricted. Only + # privileged contexts (shell, data load, migration from a + # script-created rule) can mint such an owner — surface it. + _logger.warning( + "Routing rule %s (id %s) is owned by the superuser and evaluates " + "without record-rule bounds; re-save it as a real user to scope it.", + rule.name, + rule.id, + ) # nosemgrep: semgrep.odoo-with-user-unvalidated -- owner is system-set in create()/write(), not client input rule_as_owner = rule.with_user(owner.id) # nosemgrep: semgrep.odoo-with-user-unvalidated -- owner is system-set; scopes ticket writes @@ -308,6 +325,12 @@ def apply_routing(self, ticket): except AccessError: # The rule owner cannot see this ticket -> the rule does not # apply to it. Correct behaviour, not an error. + _logger.debug( + "Routing rule %s: owner %s cannot read ticket %s; rule does not apply.", + rule.name, + owner.login, + ticket.id, + ) continue if matched: # Apply the rule's actions as the owner: the ticket.write is @@ -333,6 +356,12 @@ def apply_routing(self, ticket): except AccessError: # Owner may match the ticket but not be allowed to write it # (e.g. read-only scope). Skip rather than apply elevated. + _logger.info( + "Routing rule %s: owner %s lacks write access on ticket %s; skipped.", + rule.name, + owner.login, + ticket.id, + ) continue _logger.info( "Applied routing rule '%s' to ticket %s: %s", @@ -341,10 +370,12 @@ def apply_routing(self, ticket): vals, ) - # Atomic increment: avoids lost updates under concurrent - # cron/UI escalation, and needs no sudo (raw SQL bypasses ACL). - # Invisible to spp_audit ORM write-hooks, which is acceptable - # for a statistics counter. + # Atomic increment: a read-modify-write here would raise a + # serialization failure under concurrent routing (cursors run + # REPEATABLE READ), retried only at whole-dispatch granularity. + # A single UPDATE avoids the conflict entirely, and needs no + # sudo (raw SQL bypasses ACL). Invisible to spp_audit ORM + # write-hooks, which is acceptable for a statistics counter. rule.flush_recordset(["match_count"]) self.env.cr.execute( "UPDATE spp_grm_routing_rule SET match_count = match_count + 1 WHERE id = %s", diff --git a/spp_grm_cel/readme/HISTORY.md b/spp_grm_cel/readme/HISTORY.md index ec719d487..63f05cd1f 100644 --- a/spp_grm_cel/readme/HISTORY.md +++ b/spp_grm_cel/readme/HISTORY.md @@ -4,17 +4,30 @@ defined them (``eval_as_user_id``, system-managed), not as the superuser cron. An officer's rule can only route/escalate tickets within that officer's own record-rule scope, closing the path where an always-match rule applied by the hourly cron could seize every ticket in the - database (#379). Existing rules are backfilled from ``create_uid`` by a migration. + database (#379). Existing rules are backfilled from ``create_uid`` by a migration; note that + a user who owns rules can no longer be deleted (``ondelete="restrict"``) — archive them + instead, and a rule owned by the superuser (created from a shell, import script, or data + load) still evaluates without record-rule bounds and is called out with a warning by both + the migration and the rule engine. - fix(security): the rule-engine entry points (``apply_routing``, ``apply_escalations``, ``apply_escalation``, ``check_escalations``) are marked ``@api.private`` — no longer callable over RPC (#381). - fix(security): drop the portal and internal-user read rows on both rule models. With owner-identity evaluation the acting user never reads the rules, so the rows only exposed the - routing/escalation map (conditions, targets, thresholds) to enumeration (#380). + routing/escalation map (conditions, targets, thresholds) to enumeration (hardening alongside + #379/#381). +- fix: an escalation is now applied atomically (savepoint): if any post-write step — the + chatter post, notification, or case creation — is denied because the rule just reassigned + the ticket out of its owner's own scope, the whole escalation rolls back and is skipped + instead of persisting half-applied with its chatter message silently lost. - fix: increment ``match_count`` / ``escalation_count`` with an atomic ``UPDATE`` instead of a - read-modify-write, so concurrent cron and UI escalations cannot lose counts. + read-modify-write, avoiding a serialization failure under concurrent cron/UI escalation whose + dispatch-level retry would re-run the whole cron pass. - fix: rule CEL validation now reports any parser error as a ``ValidationError`` (previously only ``SyntaxError`` was caught). +- fix: the hourly escalation cron searches the active rule set once per pass instead of once per + open ticket, and the engine logs (instead of silently skipping) rules with no evaluation + identity and tickets skipped for lack of owner access. ### 19.0.2.0.1 diff --git a/spp_grm_cel/tests/test_escalation_rules.py b/spp_grm_cel/tests/test_escalation_rules.py index 153639b0d..d662c194f 100644 --- a/spp_grm_cel/tests/test_escalation_rules.py +++ b/spp_grm_cel/tests/test_escalation_rules.py @@ -2,7 +2,7 @@ from datetime import timedelta -from odoo import fields +from odoo import Command, fields from odoo.tests.common import TransactionCase @@ -12,7 +12,23 @@ class TestEscalationRules(TransactionCase): @classmethod def setUpClass(cls): super().setUpClass() - cls.EscalationRule = cls.env["spp.grm.escalation.rule"] + # Rules are created as a GRM manager, not via the bare test env: the + # test env is the superuser, and a superuser-owned rule evaluates with + # record rules bypassed (with_user(SUPERUSER_ID) is always superuser + # mode) — i.e. the pre-#379 unbounded path. Binding the handle to a + # manager makes every rule in this suite exercise the owner-scoped + # evaluation the fix introduces, with the same broad manager reach. + cls.rule_author = cls.env["res.users"].create( + { + "name": "Escalation Rule Author", + "login": "grm_escalation_rule_author", + "group_ids": [ + Command.link(cls.env.ref("base.group_user").id), + Command.link(cls.env.ref("spp_grm.group_grm_manager").id), + ], + } + ) + cls.EscalationRule = cls.env["spp.grm.escalation.rule"].with_user(cls.rule_author) cls.Ticket = cls.env["spp.grm.ticket"] cls.Team = cls.env["spp.grm.team"] cls.User = cls.env["res.users"] diff --git a/spp_grm_cel/tests/test_routing_rules.py b/spp_grm_cel/tests/test_routing_rules.py index 83075e93a..7c3d26596 100644 --- a/spp_grm_cel/tests/test_routing_rules.py +++ b/spp_grm_cel/tests/test_routing_rules.py @@ -1,5 +1,6 @@ # Part of OpenSPP. See LICENSE file for full copyright and licensing details. +from odoo import Command from odoo.exceptions import ValidationError from odoo.tests.common import TransactionCase @@ -10,7 +11,23 @@ class TestRoutingRules(TransactionCase): @classmethod def setUpClass(cls): super().setUpClass() - cls.RoutingRule = cls.env["spp.grm.routing.rule"] + # Rules are created as a GRM manager, not via the bare test env: the + # test env is the superuser, and a superuser-owned rule evaluates with + # record rules bypassed (with_user(SUPERUSER_ID) is always superuser + # mode) — i.e. the pre-#379 unbounded path. Binding the handle to a + # manager makes every rule in this suite exercise the owner-scoped + # evaluation the fix introduces, with the same broad manager reach. + cls.rule_author = cls.env["res.users"].create( + { + "name": "Routing Rule Author", + "login": "grm_routing_rule_author", + "group_ids": [ + Command.link(cls.env.ref("base.group_user").id), + Command.link(cls.env.ref("spp_grm.group_grm_manager").id), + ], + } + ) + cls.RoutingRule = cls.env["spp.grm.routing.rule"].with_user(cls.rule_author) cls.Ticket = cls.env["spp.grm.ticket"] cls.Team = cls.env["spp.grm.team"] cls.Category = cls.env["spp.grm.ticket.category"] diff --git a/spp_grm_cel/tests/test_rule_acl.py b/spp_grm_cel/tests/test_rule_acl.py index bebc4c566..560a479d4 100644 --- a/spp_grm_cel/tests/test_rule_acl.py +++ b/spp_grm_cel/tests/test_rule_acl.py @@ -11,9 +11,9 @@ Portal users must never hold write/create/unlink — and, as of #379, no read either: rules now evaluate with their owner's identity (``eval_as_user_id``), so -the acting/portal user never needs to read them. Dropping the portal and -base-user read rows closes the rule-enumeration surface (#380). GRM staff retain -full management. +the acting user never needs to read them. Dropping the portal and base-user +read rows closes the rule-enumeration surface (hardening alongside #379/#381; +the ticket-side portal scoping is #380). GRM staff retain full management. """ from odoo import Command @@ -81,14 +81,30 @@ def test_portal_user_cannot_write_escalation_rule(self): def test_portal_user_cannot_read_rules(self): """Portal users have no read on the rule models. Rules are evaluated with their owner's identity (eval_as_user_id), so the acting/portal user never - needs to read them — closing the enumeration surface (#380). The escalation - counter write is exercised under owner identity in - test_rule_owner_identity.py.""" + needs to read them — closing the enumeration surface (hardening alongside + #379/#381). The escalation counter write is exercised under owner identity + in test_rule_owner_identity.py.""" with self.assertRaises(AccessError): self.env[ROUTING_MODEL].with_user(self.portal_user).check_access("read") with self.assertRaises(AccessError): self.env[ESCALATION_MODEL].with_user(self.portal_user).check_access("read") + def test_internal_user_cannot_read_rules(self): + """A plain internal user (base.group_user, no GRM group) has no read on + the rule models either: the base-user read rows were dropped for the same + reason as the portal ones, and this pins them from silently coming back.""" + internal_user = self.env["res.users"].create( + { + "name": "Plain Internal User", + "login": "grm_internal_acl_test", + "group_ids": [Command.link(self.env.ref("base.group_user").id)], + } + ) + with self.assertRaises(AccessError): + self.env[ROUTING_MODEL].with_user(internal_user).check_access("read") + with self.assertRaises(AccessError): + self.env[ESCALATION_MODEL].with_user(internal_user).check_access("read") + def test_grm_manager_can_create_rules(self): """GRM staff must retain full management of both rule models.""" routing = self.env[ROUTING_MODEL].with_user(self.grm_manager).create({"name": "Manager Routing Rule"}) diff --git a/spp_grm_cel/tests/test_rule_owner_identity.py b/spp_grm_cel/tests/test_rule_owner_identity.py index eb32d411e..5fd96470c 100644 --- a/spp_grm_cel/tests/test_rule_owner_identity.py +++ b/spp_grm_cel/tests/test_rule_owner_identity.py @@ -9,8 +9,8 @@ Rules must evaluate and act with the identity of whoever defined them (eval_as_user_id), so an officer's rule can only touch tickets the officer's own record rule already permits. Mirrors the spp_alerts #364 owner-identity fix. -- #381 (Medium): apply_routing / apply_escalations / check_escalations were - public @api.model methods, RPC-dispatchable; they must be private. +- #381 (Medium): apply_routing / apply_escalations / apply_escalation / + check_escalations were public methods, RPC-dispatchable; they must be private. """ from odoo import Command @@ -88,6 +88,68 @@ def test_officer_rule_cannot_seize_foreign_ticket(self): "Officer-authored rule must not reassign a ticket it cannot see", ) + def test_officer_rule_cannot_seize_foreign_ticket_via_condition(self): + """Same seize scenario, but with a non-empty CEL condition: evaluating + the condition must READ the ticket, which the owner cannot, so the rule + is skipped on the evaluate path (not just denied on the write path — + an empty condition short-circuits evaluate() without touching the + ticket, so only this variant pins the read-side bound).""" + self.env[ESCALATION].with_user(self.officer).create( + { + "name": "Seize critical tickets", + "condition_cel": "severity == 'critical'", # foreign_ticket matches + "escalate_to_user_id": self.officer.id, + "trigger_after_hours": 0, + } + ) + self.env[ESCALATION].sudo().check_escalations() + self.foreign_ticket.invalidate_recordset() + self.assertFalse( + self.foreign_ticket.is_escalated, + "Rule condition must not be evaluated against a ticket its owner cannot read", + ) + + def test_officer_escalation_out_of_scope_rolls_back_cleanly(self): + """An officer's rule that reassigns a ticket OUT of the officer's own + scope must not leave a half-applied escalation: the reassignment write + succeeds (access is checked pre-write), but the follow-up steps then + fail for lack of access — the savepoint must roll the whole escalation + back: no state change, no counter increment, no chatter message.""" + own_ticket = self.env["spp.grm.ticket"].create( + { + "name": "Own-team grievance", + "description": "In team A, inside the officer's scope", + "partner_id": self.foreign_ticket.partner_id.id, + "team_id": self.team_a.id, + "severity": "critical", + } + ) + rule = ( + self.env[ESCALATION] + .with_user(self.officer) + .create( + { + "name": "Escalate out of scope", + "condition_cel": "", + "escalate_to_team_id": self.team_b.id, # officer is not in team B + "trigger_after_hours": 0, + } + ) + ) + messages_before = len(own_ticket.message_ids) + applied = self.env[ESCALATION].sudo().apply_escalations(own_ticket) + own_ticket.invalidate_recordset() + rule.invalidate_recordset() + self.assertFalse(applied, "Out-of-scope escalation must report not-applied") + self.assertFalse(own_ticket.is_escalated, "Escalation state must be rolled back") + self.assertEqual(own_ticket.team_id, self.team_a, "Reassignment must be rolled back") + self.assertEqual(rule.escalation_count, 0, "Counter must not survive the rollback") + self.assertEqual( + len(own_ticket.message_ids), + messages_before, + "No chatter message may survive a rolled-back escalation", + ) + def test_manager_rule_applies_broadly(self): """A manager (broad record-rule scope) authoring the same rule DOES escalate — owner identity does not over-restrict legitimate rules.""" @@ -155,24 +217,28 @@ def test_operational_toggle_does_not_rebind_owner(self): def test_escalation_counter_increments_under_owner_identity(self): """The escalation counter is incremented (atomically) when a manager's - rule applies via the elevated cron path.""" + rule applies via the elevated path. Applied to one explicit ticket — + not via the DB-wide check_escalations scan — so the +1 assertion stays + valid if a fixture ever adds another open ticket.""" rule = ( self.env[ESCALATION] .with_user(self.manager) .create({"name": "Counter rule", "condition_cel": "", "escalate_severity": "high"}) ) before = rule.escalation_count - self.env[ESCALATION].sudo().check_escalations() + self.env[ESCALATION].sudo().apply_escalations(self.foreign_ticket) rule.invalidate_recordset() self.assertEqual(rule.escalation_count, before + 1) def test_entry_points_not_rpc_callable(self): - """#381: the three rule-engine methods must be rejected for RPC dispatch.""" + """#381: all four rule-engine methods must be rejected for RPC dispatch.""" from odoo.service.model import call_kw + rule = self.env[ESCALATION].create({"name": "Dispatch probe", "condition_cel": ""}) for model, method, args in [ (ROUTING, "apply_routing", [self.foreign_ticket.id]), (ESCALATION, "apply_escalations", [self.foreign_ticket.id]), + (ESCALATION, "apply_escalation", [[rule.id], self.foreign_ticket.id]), (ESCALATION, "check_escalations", []), ]: with self.assertRaises(AccessError): From 4f9797705810c2f35d410d776fefd2e8fd0be0d9 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Tue, 1 Sep 2026 12:41:37 +0800 Subject: [PATCH 05/15] docs(grm): regenerate READMEs from CI generator (#415) --- spp_grm/README.rst | 4 +++- spp_grm/static/description/index.html | 4 +++- spp_grm_cel/README.rst | 25 +++++++++++++++++++---- spp_grm_cel/static/description/index.html | 25 +++++++++++++++++++---- 4 files changed, 48 insertions(+), 10 deletions(-) diff --git a/spp_grm/README.rst b/spp_grm/README.rst index 7edcf3c2e..9e58df251 100644 --- a/spp_grm/README.rst +++ b/spp_grm/README.rst @@ -162,7 +162,9 @@ Changelog grievance in the system over RPC (#380). Added a portal record rule scoping to the user's own partner and reduced the portal access-control entry to read-only (submission is handled by the sudo'd - portal controller, which needs no direct model write). + portal controller, which needs no direct model write). The rule covers + all four operations, so the scoping also holds if a future + access-control change ever re-grants portal write. 19.0.2.0.1 ~~~~~~~~~~ diff --git a/spp_grm/static/description/index.html b/spp_grm/static/description/index.html index 4439520bc..37db20135 100644 --- a/spp_grm/static/description/index.html +++ b/spp_grm/static/description/index.html @@ -544,7 +544,9 @@

    19.0.2.0.2

    grievance in the system over RPC (#380). Added a portal record rule scoping to the user’s own partner and reduced the portal access-control entry to read-only (submission is handled by the sudo’d -portal controller, which needs no direct model write). +portal controller, which needs no direct model write). The rule covers +all four operations, so the scoping also holds if a future +access-control change ever re-grants portal write.
diff --git a/spp_grm_cel/README.rst b/spp_grm_cel/README.rst index 03fcef7e6..20e75a7d8 100644 --- a/spp_grm_cel/README.rst +++ b/spp_grm_cel/README.rst @@ -133,19 +133,36 @@ Changelog route/escalate tickets within that officer's own record-rule scope, closing the path where an always-match rule applied by the hourly cron could seize every ticket in the database (#379). Existing rules are - backfilled from ``create_uid`` by a migration. + backfilled from ``create_uid`` by a migration; note that a user who + owns rules can no longer be deleted (``ondelete="restrict"``) — + archive them instead, and a rule owned by the superuser (created from + a shell, import script, or data load) still evaluates without + record-rule bounds and is called out with a warning by both the + migration and the rule engine. - fix(security): the rule-engine entry points (``apply_routing``, ``apply_escalations``, ``apply_escalation``, ``check_escalations``) are marked ``@api.private`` — no longer callable over RPC (#381). - fix(security): drop the portal and internal-user read rows on both rule models. With owner-identity evaluation the acting user never reads the rules, so the rows only exposed the routing/escalation map - (conditions, targets, thresholds) to enumeration (#380). + (conditions, targets, thresholds) to enumeration (hardening alongside + #379/#381). +- fix: an escalation is now applied atomically (savepoint): if any + post-write step — the chatter post, notification, or case creation — + is denied because the rule just reassigned the ticket out of its + owner's own scope, the whole escalation rolls back and is skipped + instead of persisting half-applied with its chatter message silently + lost. - fix: increment ``match_count`` / ``escalation_count`` with an atomic - ``UPDATE`` instead of a read-modify-write, so concurrent cron and UI - escalations cannot lose counts. + ``UPDATE`` instead of a read-modify-write, avoiding a serialization + failure under concurrent cron/UI escalation whose dispatch-level retry + would re-run the whole cron pass. - fix: rule CEL validation now reports any parser error as a ``ValidationError`` (previously only ``SyntaxError`` was caught). +- fix: the hourly escalation cron searches the active rule set once per + pass instead of once per open ticket, and the engine logs (instead of + silently skipping) rules with no evaluation identity and tickets + skipped for lack of owner access. 19.0.2.0.1 ~~~~~~~~~~ diff --git a/spp_grm_cel/static/description/index.html b/spp_grm_cel/static/description/index.html index 4e2607fd0..d9b399110 100644 --- a/spp_grm_cel/static/description/index.html +++ b/spp_grm_cel/static/description/index.html @@ -519,19 +519,36 @@

19.0.2.0.2

route/escalate tickets within that officer’s own record-rule scope, closing the path where an always-match rule applied by the hourly cron could seize every ticket in the database (#379). Existing rules are -backfilled from create_uid by a migration. +backfilled from create_uid by a migration; note that a user who +owns rules can no longer be deleted (ondelete="restrict") — +archive them instead, and a rule owned by the superuser (created from +a shell, import script, or data load) still evaluates without +record-rule bounds and is called out with a warning by both the +migration and the rule engine.
  • fix(security): the rule-engine entry points (apply_routing, apply_escalations, apply_escalation, check_escalations) are marked @api.private — no longer callable over RPC (#381).
  • fix(security): drop the portal and internal-user read rows on both rule models. With owner-identity evaluation the acting user never reads the rules, so the rows only exposed the routing/escalation map -(conditions, targets, thresholds) to enumeration (#380).
    • +(conditions, targets, thresholds) to enumeration (hardening alongside +#379/#381). +
  • fix: an escalation is now applied atomically (savepoint): if any +post-write step — the chatter post, notification, or case creation — +is denied because the rule just reassigned the ticket out of its +owner’s own scope, the whole escalation rolls back and is skipped +instead of persisting half-applied with its chatter message silently +lost.
  • fix: increment match_count / escalation_count with an atomic -UPDATE instead of a read-modify-write, so concurrent cron and UI -escalations cannot lose counts.
    • +UPDATE instead of a read-modify-write, avoiding a serialization +failure under concurrent cron/UI escalation whose dispatch-level retry +would re-run the whole cron pass.
  • fix: rule CEL validation now reports any parser error as a ValidationError (previously only SyntaxError was caught).
    • +
  • fix: the hourly escalation cron searches the active rule set once per +pass instead of once per open ticket, and the engine logs (instead of +silently skipping) rules with no evaluation identity and tickets +skipped for lack of owner access.
    • From 0a072d156d11704f14175d0403840718b189e499 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Wed, 2 Sep 2026 11:14:06 +0800 Subject: [PATCH 06/15] =?UTF-8?q?fix(grm):=20address=20PR=20#415=20round-2?= =?UTF-8?q?=20review=20=E2=80=94=20deferred=20SLA=20hook,=20fail-closed=20?= =?UTF-8?q?side=20effects,=20once-per-ticket=20rules?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit spp_grm - SLA-breach handling no longer runs inside the stored sla_status compute: the compute schedules it and it runs at the transaction's precommit stage (same transaction), so the escalation engine's writes/savepoints never execute mid-computation. - HttpCase for the sudo'd portal submit route (the justification for the read-only portal ACL). spp_grm_cel - Engine loads the active rule set elevated and resolves each rule's owner once per cron pass (warnings once, not once per ticket); a rule whose owner is archived is skipped; superuser-owned rules still warn and run. - apply_escalations: a rule applies at most once per ticket; one failing ticket is rolled back and logged instead of aborting the pass. - apply_escalation: single ticket write; notification and case creation re-raise AccessError (whole escalation rolls back) and isolate other failures in their own savepoint; case creation fixed (presenting_issue, required case_worker_id) — it had never worked. - "Take Ownership" action on both rule forms; write() accepts eval_as_user_id only when it is the acting user; remediation wording fixed in migration, engine and HISTORY. "Check Escalation" button limited to officers+. - Tests: engine behaviour suite (batch breach with in-compute rollbacks, fail-closed side effects, once-per-ticket, pass isolation, warning dedupe, button for users without rule access), ownership tests, restored counter-test assertions. --- spp_grm/models/grm_ticket.py | 28 +- spp_grm/readme/HISTORY.md | 4 + spp_grm/tests/__init__.py | 2 + spp_grm/tests/test_portal_ticket_submit.py | 55 ++++ spp_grm/tests/test_sla_breach.py | 56 ++++ .../migrations/19.0.2.0.2/post-migration.py | 9 +- spp_grm_cel/models/grm_escalation_rule.py | 294 +++++++++++------- spp_grm_cel/models/grm_routing_rule.py | 108 +++++-- spp_grm_cel/readme/HISTORY.md | 42 ++- spp_grm_cel/tests/__init__.py | 1 + spp_grm_cel/tests/test_escalation_engine.py | 273 ++++++++++++++++ spp_grm_cel/tests/test_rule_owner_identity.py | 71 ++++- .../views/grm_escalation_rule_views.xml | 11 + spp_grm_cel/views/grm_routing_rule_views.xml | 10 + 14 files changed, 805 insertions(+), 159 deletions(-) create mode 100644 spp_grm/tests/test_portal_ticket_submit.py create mode 100644 spp_grm/tests/test_sla_breach.py create mode 100644 spp_grm_cel/tests/test_escalation_engine.py diff --git a/spp_grm/models/grm_ticket.py b/spp_grm/models/grm_ticket.py index d2b2500a5..0636cd30f 100644 --- a/spp_grm/models/grm_ticket.py +++ b/spp_grm/models/grm_ticket.py @@ -536,10 +536,30 @@ def _compute_sla_status(self): # Trigger escalation when status transitions to 'breached' # Only trigger if the status actually changed to breached (not already breached) if ticket.sla_status == "breached" and old_status != "breached": - # Use sudo() to call _on_sla_breach in a new environment context - # to avoid triggering compute dependencies during the compute itself - # nosemgrep: semgrep.odoo-sudo-without-context - ticket.sudo()._on_sla_breach() + ticket._schedule_sla_breach() + + def _schedule_sla_breach(self): + """Queue ``_on_sla_breach`` for these tickets once the compute is over. + + The breach hook drives the escalation engine: ticket writes, chatter + posts and savepoints that flush and may roll back. None of that may run + inside a stored compute, so it is deferred to the transaction's + precommit stage and runs at the next full flush/commit, still in the + same transaction (the way mail.thread defers its tracking messages). + """ + pending = self.env.cr.precommit.data.setdefault("spp_grm.sla_breach_ids", set()) + pending.update(self.ids) + self.env.cr.precommit.add(self._run_sla_breach_hooks) + + def _run_sla_breach_hooks(self): + """Precommit callback: run the breach hook for every ticket queued so far.""" + pending = self.env.cr.precommit.data.pop("spp_grm.sla_breach_ids", set()) + if not pending: + return + # Breach handling runs elevated, as it always did; every escalation + # rule effect is bounded by the rule owner's identity, not by this env. + # nosemgrep: semgrep.odoo-sudo-without-context + self.browse(sorted(pending)).exists().sudo()._on_sla_breach() def _on_sla_breach(self): """Called when ticket SLA status changes to breached. diff --git a/spp_grm/readme/HISTORY.md b/spp_grm/readme/HISTORY.md index 6e74eff15..7ba3e0f47 100644 --- a/spp_grm/readme/HISTORY.md +++ b/spp_grm/readme/HISTORY.md @@ -7,6 +7,10 @@ (submission is handled by the sudo'd portal controller, which needs no direct model write). The rule covers all four operations, so the scoping also holds if a future access-control change ever re-grants portal write. +- fix: SLA-breach handling (auto-escalation and the breach chatter note) no longer runs inside + the stored ``sla_status`` compute. It is deferred to the end of the triggering transaction, so + the escalation engine's writes, savepoints and flushes never execute mid-computation. Same + transaction, same outcome. ### 19.0.2.0.1 diff --git a/spp_grm/tests/__init__.py b/spp_grm/tests/__init__.py index cfab33a28..c46e49d82 100644 --- a/spp_grm/tests/__init__.py +++ b/spp_grm/tests/__init__.py @@ -3,3 +3,5 @@ from . import test_res_partner from . import test_grm_security from . import test_portal_ticket_acl +from . import test_portal_ticket_submit +from . import test_sla_breach diff --git a/spp_grm/tests/test_portal_ticket_submit.py b/spp_grm/tests/test_portal_ticket_submit.py new file mode 100644 index 000000000..7e1768538 --- /dev/null +++ b/spp_grm/tests/test_portal_ticket_submit.py @@ -0,0 +1,55 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Portal grievance submission still works end to end with a read-only portal ACL. + +The portal access-control entry on ``spp.grm.ticket`` grants read only (#380); +submission goes through the sudo'd ``/my/ticket/submit`` controller. This pins +that route: the form page loads for a portal user (non-sudo category/channel +lookups), the POST creates the ticket for the submitter's partner on the web +channel, and the submitter can read it back. +""" + +import re + +from odoo import Command +from odoo.tests import HttpCase, tagged + + +@tagged("post_install", "-at_install") +class TestPortalTicketSubmit(HttpCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.portal = cls.env["res.users"].create( + { + "name": "Portal Submitter", + "login": "grm_portal_submit", + "password": "grm_portal_submit_pw", + "group_ids": [Command.link(cls.env.ref("base.group_portal").id)], + } + ) + cls.category = cls.env["spp.grm.ticket.category"].create({"name": "Portal Cat"}) + + def test_portal_submit_route_creates_own_ticket(self): + self.authenticate("grm_portal_submit", "grm_portal_submit_pw") + page = self.url_open("/my/ticket/new") + self.assertEqual(page.status_code, 200, page.text[:500]) + match = re.search(r'name="csrf_token"\s+value="([^"]+)"', page.text) + self.assertTrue(match, "csrf token not found in /my/ticket/new form") + resp = self.url_open( + "/my/ticket/submit", + data={ + "csrf_token": match.group(1), + "ticket_name": "Portal grievance", + "description": "Submitted through the portal", + "category_id": str(self.category.id), + }, + ) + self.assertEqual(resp.status_code, 200, resp.text[:500]) + self.assertTrue(resp.url.endswith("/my/tickets"), resp.url) + ticket = self.env["spp.grm.ticket"].search([("name", "=", "Portal grievance")]) + self.assertEqual(len(ticket), 1) + self.assertEqual(ticket.partner_id, self.portal.partner_id) + self.assertEqual(ticket.channel_id, self.env.ref("spp_grm.grm_ticket_channel_web")) + self.assertEqual(ticket.category_id, self.category) + # The submitter can read their own ticket back over the model layer. + self.assertEqual(ticket.with_user(self.portal).name, "Portal grievance") diff --git a/spp_grm/tests/test_sla_breach.py b/spp_grm/tests/test_sla_breach.py new file mode 100644 index 000000000..6ca57f7bb --- /dev/null +++ b/spp_grm/tests/test_sla_breach.py @@ -0,0 +1,56 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""SLA-breach handling is deferred out of the stored ``sla_status`` compute. + +The breach hook (auto-escalation + breach chatter note) runs at the +transaction's precommit stage, not from inside ``_compute_sla_status``: the +escalation engine writes, posts to chatter and uses savepoints, none of which +may run mid-computation. +""" + +from odoo.tests import TransactionCase, tagged + + +@tagged("post_install", "-at_install") +class TestSlaBreachDeferral(TransactionCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.partner = cls.env["res.partner"].create({"name": "Breach Complainant"}) + cls.past = cls.env["spp.grm.ticket.category"].create({"name": "Past SLA", "default_sla_hours": -1}) + Ticket = cls.env["spp.grm.ticket"] + cls.tickets = Ticket.create( + [ + {"name": "b1", "description": "b1", "partner_id": cls.partner.id}, + {"name": "b2", "description": "b2", "partner_id": cls.partner.id}, + ] + ) + + def _breach_notes(self): + return self.env["mail.message"].search_count( + [("model", "=", "spp.grm.ticket"), ("res_id", "in", self.tickets.ids), ("subject", "=", "SLA Breach Alert")] + ) + + def test_breach_note_posted_at_precommit_for_the_whole_batch(self): + self.env.flush_all() + self.assertEqual(self.tickets.mapped("sla_status"), ["on_track", "on_track"]) + before = self._breach_notes() + + self.tickets.write({"category_id": self.past.id}) + self.assertEqual(self.tickets.mapped("sla_status"), ["breached", "breached"]) + # The compute ran (field read) but only scheduled the hook. + self.assertEqual(self._breach_notes(), before) + + # cr.flush() is what commit does: pending computes, then precommit hooks. + self.env.cr.flush() + self.assertEqual(self._breach_notes() - before, 2) + + def test_breach_hook_runs_once_per_ticket(self): + """Several schedulings within one transaction collapse into one run.""" + self.env.flush_all() + before = self._breach_notes() + self.tickets[0].write({"category_id": self.past.id}) + self.tickets[1].write({"category_id": self.past.id}) + self.env.cr.flush() + self.assertEqual(self._breach_notes() - before, 2) + # Nothing left queued. + self.assertNotIn("spp_grm.sla_breach_ids", self.env.cr.precommit.data) diff --git a/spp_grm_cel/migrations/19.0.2.0.2/post-migration.py b/spp_grm_cel/migrations/19.0.2.0.2/post-migration.py index 947f15f3f..62488bb45 100644 --- a/spp_grm_cel/migrations/19.0.2.0.2/post-migration.py +++ b/spp_grm_cel/migrations/19.0.2.0.2/post-migration.py @@ -37,7 +37,10 @@ def migrate(cr, version): # Rules created from privileged contexts (odoo shell, import scripts, data # loads) carry create_uid = 1, and a superuser owner evaluates with record # rules bypassed (with_user(SUPERUSER_ID) is always superuser mode). Call - # these out specifically: they stay unbounded until re-saved by a real user. + # these out specifically: they stay unbounded until a real user takes + # ownership (the "Take Ownership" button on the rule form, or a change to + # the rule's condition/targets — a plain re-save sends no fields and does + # not re-bind). cr.execute( "SELECT id, name FROM spp_grm_routing_rule WHERE eval_as_user_id = 1 " "UNION ALL " @@ -47,8 +50,8 @@ def migrate(cr, version): if superuser_rules: _logger.warning( "%s GRM rule(s) are owned by the superuser and will evaluate WITHOUT " - "record-rule bounds: %s. Re-save each as the user who should own it " - "to scope its evaluation.", + "record-rule bounds: %s. Have the user who should own each rule open it " + 'and use "Take Ownership" (or edit its condition/targets) to scope its evaluation.', len(superuser_rules), ", ".join(f"{name!r} (id {rid})" for rid, name in superuser_rules), ) diff --git a/spp_grm_cel/models/grm_escalation_rule.py b/spp_grm_cel/models/grm_escalation_rule.py index 74dc0b1da..9f650d2a4 100644 --- a/spp_grm_cel/models/grm_escalation_rule.py +++ b/spp_grm_cel/models/grm_escalation_rule.py @@ -149,7 +149,7 @@ class GRMEscalationRule(models.Model): "targets, so an elevated cron can never apply a rule beyond its author's " "reach. System-managed; not editable.", ) - # No Python `default` on purpose (see spp_alerts #364): a default makes + # No Python `default` on purpose (see PR #364, spp_alerts): a default makes # _init_column backfill existing rows with the UPGRADE user before the # migration runs, and lets a client forge the value via a # default_eval_as_user_id context key. The identity is set explicitly in @@ -189,17 +189,31 @@ def create(self, 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. self.env.uid is the acting user, preserved - even under sudo() (only an explicit with_user() write re-widens). + eval_as_user_id tracks whoever last defined what the rule targets. A + client can never point it at a third party; the only accepted explicit + value is the acting user's own id (see action_take_ownership), which is + the same re-bind the targeting-field path performs. self.env.uid is the + acting user, preserved even under sudo() (only an explicit + with_user() write re-widens). """ - if "eval_as_user_id" in vals or any(f in vals for f in self._EVAL_TARGETING_FIELDS): - vals = dict(vals) - vals.pop("eval_as_user_id", None) - if any(f in vals for f in self._EVAL_TARGETING_FIELDS): - vals["eval_as_user_id"] = self.env.uid + if any(f in vals for f in self._EVAL_TARGETING_FIELDS) or vals.get("eval_as_user_id") == self.env.uid: + vals = dict(vals, eval_as_user_id=self.env.uid) + elif "eval_as_user_id" in vals: + vals = {k: v for k, v in vals.items() if k != "eval_as_user_id"} return super().write(vals) + def action_take_ownership(self): + """Re-bind these rules' evaluation identity to the acting user. + + The remediation path for a rule owned by the superuser (created from a + shell, import or data load), by an archived user, or by someone whose + ticket scope no longer fits: from now on the rule evaluates within the + acting user's own record-rule scope. Saving the form without changing a + targeting field does not re-bind (the client only sends dirty fields). + """ + self.write({"eval_as_user_id": self.env.uid}) + return True + @api.constrains("condition_cel") def _check_condition_cel(self): """Validate CEL expression syntax using CEL parser.""" @@ -391,11 +405,14 @@ def apply_escalation(self, ticket): rule_name=self.name, ) - # Apply changes to ticket - ticket.write(vals) + # Track which escalation rule was applied (many2many) + vals["escalation_rule_ids"] = [Command.link(self.id)] - # Track which escalation rule was applied (add to many2many) - ticket.write({"escalation_rule_ids": [Command.link(self.id)]}) + # One write: write() checks the owner's access against the ticket's + # current state, so a single call succeeds whenever the owner may edit + # the ticket (the UI semantics). A second write would re-check against + # the reassigned state and be denied for nothing. + ticket.write(vals) _logger.info( "Applied escalation rule '%s' to ticket %s: %s", @@ -404,11 +421,12 @@ def apply_escalation(self, ticket): vals, ) - # Post to chatter BEFORE any external side effect: posting needs write - # access on the ticket, which the rule owner may have just lost by - # reassigning it out of their own scope. When that happens the caller's - # savepoint rolls everything back — and because nothing external has - # fired yet, no ghost notification announces a rolled-back escalation. + # Post to chatter BEFORE any external side effect. spp.grm.ticket sets + # _mail_post_access = "read", so posting needs read access on the + # ticket, which the rule owner may have just lost by reassigning it out + # of their own scope. When that happens the caller's savepoint rolls + # everything back — and because nothing external has fired yet, no + # ghost notification announces a rolled-back escalation. ticket.message_post( body=_( "Ticket escalated by rule: %(rule_name)s", @@ -443,29 +461,45 @@ def apply_escalation(self, ticket): def _send_escalation_notification(self, ticket): """Send escalation notification email. + Delivery problems are logged and skipped (best effort), but a denied + template or ticket read re-raises so the caller's savepoint rolls the + whole escalation back: the rule owner must be entitled to every effect + the rule produces. The inner savepoint keeps a failed send from leaving + the transaction in an aborted state for the steps that follow. + Args: ticket: spp.grm.ticket record """ try: - self.notification_template_id.send_mail( - ticket.id, - force_send=True, - ) - _logger.info( - "Sent escalation notification for ticket %s using template %s", - ticket.number, - self.notification_template_id.name, - ) + with self.env.cr.savepoint(): + self.notification_template_id.send_mail( + ticket.id, + force_send=True, + ) + except AccessError: + raise except Exception as e: _logger.error( "Failed to send escalation notification for ticket %s: %s", ticket.number, str(e), ) + return + _logger.info( + "Sent escalation notification for ticket %s using template %s", + ticket.number, + self.notification_template_id.name, + ) def _create_case_from_ticket(self, ticket): """Create a case management record from an escalated ticket. + A denied create (the rule owner is not allowed to create cases) + re-raises so the caller's savepoint rolls the whole escalation back + rather than reporting an escalation whose case silently never existed. + Other failures are logged and skipped; the inner savepoint keeps them + from leaving the transaction in an aborted state. + Args: ticket: spp.grm.ticket record """ @@ -478,39 +512,43 @@ def _create_case_from_ticket(self, ticket): return try: - case_vals = { - "name": _("Escalated from ticket: %s") % ticket.number, - "case_type_id": self.case_type_id.id, - "partner_id": ticket.partner_id.id, - "description": ticket.description, - "user_id": ticket.user_id.id if ticket.user_id else False, - } - - case = self.env["spp.case"].create(case_vals) - - # Link case to ticket if spp_grm_case_link is installed - if hasattr(ticket, "case_id"): - ticket.write({"case_id": case.id}) - - _logger.info( - "Created case %s from escalated ticket %s", - case.id, - ticket.number, - ) + with self.env.cr.savepoint(): + case = self.env["spp.case"].create( + { + "name": _("Escalated from ticket: %s") % ticket.number, + "case_type_id": self.case_type_id.id, + "partner_id": ticket.partner_id.id, + "presenting_issue": ticket.description, + # Required on spp.case: the ticket assignee, else whoever + # the rule evaluates as (an internal user). + "case_worker_id": (ticket.user_id or self.env.user).id, + } + ) - # Post message to ticket - ticket.message_post( - body=_("Case created: %(case_name)s") - % {"case_id": case.id, "case_name": case.name}, - subject=_("Case Created"), - ) + # Link case to ticket if spp_grm_case_link is installed + if hasattr(ticket, "case_id"): + ticket.write({"case_id": case.id}) + # Post message to ticket + ticket.message_post( + body=_("Case created: %(case_name)s") + % {"case_id": case.id, "case_name": case.name}, + subject=_("Case Created"), + ) + except AccessError: + raise except Exception as e: _logger.error( "Failed to create case for ticket %s: %s", ticket.number, str(e), ) + return + _logger.info( + "Created case %s from escalated ticket %s", + case.id, + ticket.number, + ) @api.model @api.private @@ -529,9 +567,10 @@ def check_escalations(self): _logger.info("Checking escalation rules for %d open tickets", len(tickets)) - # Hoisted out of the per-ticket loop: the active-rule set is identical - # for every ticket in this pass, so search it once, not once per ticket. - rules = self.search([("active", "=", True)], order="sequence, id") + # Hoisted out of the per-ticket loop: the active-rule set (and each + # rule's evaluation owner, with its warnings) is identical for every + # ticket in this pass, so resolve it once, not once per ticket. + rules = self._active_rules_with_owners() escalated_count = 0 for ticket in tickets: @@ -541,6 +580,58 @@ def check_escalations(self): _logger.info("Escalated %d tickets", escalated_count) return escalated_count + def _evaluation_owner(self): + """Return the user this rule evaluates as, or an empty recordset. + + Empty means the rule must not fire: it has no identity at all, or its + owner is archived (an offboarded user's scope must not keep driving + automation). A superuser owner is kept but called out: with_user(1) + always runs in superuser mode, so such a rule evaluates unbounded until + someone takes ownership (checked before the archive test, since Odoo's + ``__system__`` user is itself inactive). Logs once per call. + """ + self.ensure_one() + owner = self.eval_as_user_id or self.create_uid + if not owner: + _logger.warning( + "Escalation rule %s (id %s) has no evaluation identity (owner and " + "create_uid both unset); skipping until someone takes ownership of it.", + self.name, + self.id, + ) + return self.env["res.users"] + if owner.id == SUPERUSER_ID: + _logger.warning( + "Escalation rule %s (id %s) is owned by the superuser and evaluates " + "without record-rule bounds; take ownership of it as a real user to scope it.", + self.name, + self.id, + ) + return owner + if not owner.active: + _logger.warning( + "Escalation rule %s (id %s) is owned by archived user %s; skipping until someone takes ownership of it.", + self.name, + self.id, + owner.login, + ) + return self.env["res.users"] + return owner + + @api.model + def _active_rules_with_owners(self): + """Active rules paired with their evaluation owner, in sequence order. + + Searched with sudo(): the acting user (the cron, the sudo'd SLA path, + an officer pressing "Check Escalation") never needs read access on the + rules, because every effect is bounded by the owner identity each rule + is applied with. Rules without a usable owner are dropped here, so the + cron logs each warning once per pass rather than once per ticket. + """ + # nosemgrep: semgrep.odoo-sudo-without-context -- reads only the rule set; every effect below runs with_user(owner), never elevated + rules = self.sudo().search([("active", "=", True)], order="sequence, id") + return [(rule, owner) for rule in rules if (owner := rule._evaluation_owner())] + @api.model @api.private def apply_escalations(self, ticket, rules=None): @@ -549,43 +640,29 @@ def apply_escalations(self, ticket, rules=None): Each rule is evaluated and applied with the identity of whoever defined it (``eval_as_user_id``), so an elevated caller (the hourly cron, the sudo'd SLA-breach path) can never make a rule act beyond its author's - ticket scope. Private: not RPC-dispatchable. + ticket scope. A rule applies at most once per ticket. Private: not + RPC-dispatchable. Args: ticket: spp.grm.ticket record - rules: optional pre-searched active rules in ``sequence, id`` order; - a caller looping many tickets (the cron) passes them once - instead of re-searching per ticket + rules: optional pre-resolved ``[(rule, owner), ...]`` from + ``_active_rules_with_owners``; a caller looping many tickets + (the cron) resolves them once instead of once per ticket Returns: bool: True if any rule was applied, False otherwise """ if rules is None: - # Search for active rules in sequence order - rules = self.search([("active", "=", True)], order="sequence, id") + rules = self._active_rules_with_owners() + already_applied = set(ticket.escalation_rule_ids.ids) applied = False - for rule in rules: - owner = rule.eval_as_user_id or rule.create_uid - if not owner: - _logger.warning( - "Escalation rule %s (id %s) has no evaluation identity (owner and " - "create_uid both unset); skipping — it will never fire until re-saved.", - rule.name, - rule.id, - ) + for rule, owner in rules: + if rule.id in already_applied: + # Without this the hourly cron re-escalated every still-open + # matching ticket each pass: counter, chatter post and + # notification repeated every hour. continue - if owner.id == SUPERUSER_ID: - # with_user(SUPERUSER_ID) always runs in superuser mode (record - # rules bypassed), so this rule evaluates unrestricted. Only - # privileged contexts (shell, data load, migration from a - # script-created rule) can mint such an owner — surface it. - _logger.warning( - "Escalation rule %s (id %s) is owned by the superuser and evaluates " - "without record-rule bounds; re-save it as a real user to scope it.", - rule.name, - rule.id, - ) # nosemgrep: semgrep.odoo-with-user-unvalidated -- owner is system-set in create()/write(), not client input rule_as_owner = rule.with_user(owner.id) # nosemgrep: semgrep.odoo-with-user-unvalidated -- owner is system-set; scopes ticket writes @@ -602,28 +679,37 @@ def apply_escalations(self, ticket, rules=None): ticket.id, ) continue - if matched: - try: - # Savepoint: apply_escalation has side effects after its - # ticket writes (notification, case creation, counter, - # chatter post), any of which can raise AccessError once the - # write itself has reassigned the ticket out of the owner's - # own scope. Roll all of it back rather than leave a - # half-applied escalation with a lost chatter message. - with self.env.cr.savepoint(): - rule_as_owner.apply_escalation(ticket_as_owner) - except AccessError: - # Owner matched but cannot write this ticket (or lost access - # mid-apply) -> skip rather than apply with elevated rights. - _logger.info( - "Escalation rule %s: owner %s lacks write access on ticket %s; " - "escalation rolled back and skipped.", - rule.name, - owner.login, - ticket.id, - ) - continue - applied = True - # Continue checking other rules (unlike routing, multiple escalations can apply) + if not matched: + continue + try: + # Savepoint: apply_escalation has side effects after its ticket + # write (chatter post, notification, case creation, counter), + # any of which can be denied once the write itself has + # reassigned the ticket out of the owner's own scope. Roll all + # of it back rather than leave a half-applied escalation. + with self.env.cr.savepoint(): + rule_as_owner.apply_escalation(ticket_as_owner) + except AccessError: + # Owner matched but is not allowed to perform every effect on + # this ticket -> skip rather than apply with elevated rights. + _logger.info( + "Escalation rule %s: owner %s was denied on ticket %s; escalation rolled back and skipped.", + rule.name, + owner.login, + ticket.id, + ) + continue + except Exception: + # Per-ticket isolation: the savepoint has already rolled this + # escalation back. A constraint or database error on one ticket + # must not abort the whole cron pass. + _logger.exception( + "Escalation rule %s failed on ticket %s; rolled back and skipped.", + rule.name, + ticket.id, + ) + continue + applied = True + # Continue checking other rules (unlike routing, multiple escalations can apply) return applied diff --git a/spp_grm_cel/models/grm_routing_rule.py b/spp_grm_cel/models/grm_routing_rule.py index 33a39c009..eb7dd2e7c 100644 --- a/spp_grm_cel/models/grm_routing_rule.py +++ b/spp_grm_cel/models/grm_routing_rule.py @@ -115,7 +115,7 @@ class GRMRoutingRule(models.Model): "targets, so an elevated cron can never apply a rule beyond its author's " "reach. System-managed; not editable.", ) - # No Python `default` on purpose (see spp_alerts #364): a default makes + # No Python `default` on purpose (see PR #364, spp_alerts): a default makes # _init_column backfill existing rows with the UPGRADE user before the # migration runs, and lets a client forge the value via a # default_eval_as_user_id context key. The identity is set explicitly in @@ -150,17 +150,82 @@ def create(self, 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. self.env.uid is the acting user, preserved - even under sudo() (only an explicit with_user() write re-widens). + eval_as_user_id tracks whoever last defined what the rule targets. A + client can never point it at a third party; the only accepted explicit + value is the acting user's own id (see action_take_ownership), which is + the same re-bind the targeting-field path performs. self.env.uid is the + acting user, preserved even under sudo() (only an explicit + with_user() write re-widens). """ - if "eval_as_user_id" in vals or any(f in vals for f in self._EVAL_TARGETING_FIELDS): - vals = dict(vals) - vals.pop("eval_as_user_id", None) - if any(f in vals for f in self._EVAL_TARGETING_FIELDS): - vals["eval_as_user_id"] = self.env.uid + if any(f in vals for f in self._EVAL_TARGETING_FIELDS) or vals.get("eval_as_user_id") == self.env.uid: + vals = dict(vals, eval_as_user_id=self.env.uid) + elif "eval_as_user_id" in vals: + vals = {k: v for k, v in vals.items() if k != "eval_as_user_id"} return super().write(vals) + def action_take_ownership(self): + """Re-bind these rules' evaluation identity to the acting user. + + The remediation path for a rule owned by the superuser (created from a + shell, import or data load), by an archived user, or by someone whose + ticket scope no longer fits: from now on the rule evaluates within the + acting user's own record-rule scope. Saving the form without changing a + targeting field does not re-bind (the client only sends dirty fields). + """ + self.write({"eval_as_user_id": self.env.uid}) + return True + + def _evaluation_owner(self): + """Return the user this rule evaluates as, or an empty recordset. + + Empty means the rule must not fire: it has no identity at all, or its + owner is archived (an offboarded user's scope must not keep driving + automation). A superuser owner is kept but called out: with_user(1) + always runs in superuser mode, so such a rule evaluates unbounded until + someone takes ownership (checked before the archive test, since Odoo's + ``__system__`` user is itself inactive). Logs once per call. + """ + self.ensure_one() + owner = self.eval_as_user_id or self.create_uid + if not owner: + _logger.warning( + "Routing rule %s (id %s) has no evaluation identity (owner and " + "create_uid both unset); skipping until someone takes ownership of it.", + self.name, + self.id, + ) + return self.env["res.users"] + if owner.id == SUPERUSER_ID: + _logger.warning( + "Routing rule %s (id %s) is owned by the superuser and evaluates " + "without record-rule bounds; take ownership of it as a real user to scope it.", + self.name, + self.id, + ) + return owner + if not owner.active: + _logger.warning( + "Routing rule %s (id %s) is owned by archived user %s; skipping until someone takes ownership of it.", + self.name, + self.id, + owner.login, + ) + return self.env["res.users"] + return owner + + @api.model + def _active_rules_with_owners(self): + """Active rules paired with their evaluation owner, in sequence order. + + Searched with sudo(): the acting user (the sudo'd portal controller, an + officer creating a ticket) never needs read access on the rules, + because every effect is bounded by the owner identity each rule is + applied with. Rules without a usable owner are dropped here. + """ + # nosemgrep: semgrep.odoo-sudo-without-context -- reads only the rule set; every effect below runs with_user(owner), never elevated + rules = self.sudo().search([("active", "=", True)], order="sequence, id") + return [(rule, owner) for rule in rules if (owner := rule._evaluation_owner())] + @api.constrains("condition_cel") def _check_condition_cel(self): """Validate CEL expression syntax using CEL parser.""" @@ -292,30 +357,7 @@ def apply_routing(self, ticket): Returns: bool: True if a rule was applied, False otherwise """ - # Search for active rules in sequence order - rules = self.search([("active", "=", True)], order="sequence, id") - - for rule in rules: - owner = rule.eval_as_user_id or rule.create_uid - if not owner: - _logger.warning( - "Routing rule %s (id %s) has no evaluation identity (owner and " - "create_uid both unset); skipping — it will never fire until re-saved.", - rule.name, - rule.id, - ) - continue - if owner.id == SUPERUSER_ID: - # with_user(SUPERUSER_ID) always runs in superuser mode (record - # rules bypassed), so this rule evaluates unrestricted. Only - # privileged contexts (shell, data load, migration from a - # script-created rule) can mint such an owner — surface it. - _logger.warning( - "Routing rule %s (id %s) is owned by the superuser and evaluates " - "without record-rule bounds; re-save it as a real user to scope it.", - rule.name, - rule.id, - ) + for rule, owner in self._active_rules_with_owners(): # nosemgrep: semgrep.odoo-with-user-unvalidated -- owner is system-set in create()/write(), not client input rule_as_owner = rule.with_user(owner.id) # nosemgrep: semgrep.odoo-with-user-unvalidated -- owner is system-set; scopes ticket writes diff --git a/spp_grm_cel/readme/HISTORY.md b/spp_grm_cel/readme/HISTORY.md index 63f05cd1f..0bfb43471 100644 --- a/spp_grm_cel/readme/HISTORY.md +++ b/spp_grm_cel/readme/HISTORY.md @@ -4,29 +4,43 @@ defined them (``eval_as_user_id``, system-managed), not as the superuser cron. An officer's rule can only route/escalate tickets within that officer's own record-rule scope, closing the path where an always-match rule applied by the hourly cron could seize every ticket in the - database (#379). Existing rules are backfilled from ``create_uid`` by a migration; note that - a user who owns rules can no longer be deleted (``ondelete="restrict"``) — archive them - instead, and a rule owned by the superuser (created from a shell, import script, or data - load) still evaluates without record-rule bounds and is called out with a warning by both - the migration and the rule engine. + database (#379). Existing rules are backfilled from ``create_uid`` by a migration. A user who + owns rules can no longer be deleted (``ondelete="restrict"``) — archive them instead; a rule + whose owner is archived stops firing (logged) until someone takes ownership of it. A rule + owned by the superuser (created from a shell, import script, or data load) still evaluates + without record-rule bounds and is called out with a warning by both the migration and the + rule engine. The new **Take Ownership** button on the rule form re-binds a rule to yourself; + saving the form without changing what the rule targets does not. - fix(security): the rule-engine entry points (``apply_routing``, ``apply_escalations``, ``apply_escalation``, ``check_escalations``) are marked ``@api.private`` — no longer callable over RPC (#381). -- fix(security): drop the portal and internal-user read rows on both rule models. With - owner-identity evaluation the acting user never reads the rules, so the rows only exposed the +- fix(security): drop the portal and internal-user read rows on both rule models. The engine + now loads the active rule set with elevated rights and applies each rule with its owner's + identity, so no acting user needs read access on the rules; the rows only exposed the routing/escalation map (conditions, targets, thresholds) to enumeration (hardening alongside - #379/#381). -- fix: an escalation is now applied atomically (savepoint): if any post-write step — the - chatter post, notification, or case creation — is denied because the rule just reassigned - the ticket out of its owner's own scope, the whole escalation rolls back and is skipped - instead of persisting half-applied with its chatter message silently lost. + #379/#381). The ticket form's "Check Escalation" button is limited to GRM officers and above. +- fix: an escalation is now applied atomically (savepoint). The ticket write, the chatter post + and the counter succeed or roll back together, and if the rule owner is denied any effect the + rule produces — posting to a ticket just reassigned out of their own scope, sending the + configured template, or creating the configured case — the whole escalation rolls back and is + skipped instead of persisting half-applied. Delivery or data errors in the notification and + case steps remain best effort: logged, skipped, and isolated so they cannot abort the pass. +- fix: a rule applies at most once per ticket. Previously the hourly cron re-escalated every + still-open matching ticket on every pass, repeating the counter increment, the chatter post + and the notification each hour. +- fix: one failing ticket no longer aborts the whole escalation pass; the failure is logged and + the remaining tickets are processed. +- fix: case creation from an escalation rule never worked (it passed a field ``spp.case`` does + not have and omitted the required case worker); it now fills ``presenting_issue`` and assigns + the ticket assignee or the rule owner as case worker. - fix: increment ``match_count`` / ``escalation_count`` with an atomic ``UPDATE`` instead of a read-modify-write, avoiding a serialization failure under concurrent cron/UI escalation whose dispatch-level retry would re-run the whole cron pass. - fix: rule CEL validation now reports any parser error as a ``ValidationError`` (previously only ``SyntaxError`` was caught). -- fix: the hourly escalation cron searches the active rule set once per pass instead of once per - open ticket, and the engine logs (instead of silently skipping) rules with no evaluation +- fix: the hourly escalation cron resolves the active rule set and each rule's evaluation owner + once per pass instead of once per open ticket (so owner warnings are logged once, not once + per ticket), and the engine logs (instead of silently skipping) rules with no evaluation identity and tickets skipped for lack of owner access. ### 19.0.2.0.1 diff --git a/spp_grm_cel/tests/__init__.py b/spp_grm_cel/tests/__init__.py index 5a8d20e2c..a9a89daec 100644 --- a/spp_grm_cel/tests/__init__.py +++ b/spp_grm_cel/tests/__init__.py @@ -4,3 +4,4 @@ from . import test_escalation_rules from . import test_rule_acl from . import test_rule_owner_identity +from . import test_escalation_engine diff --git a/spp_grm_cel/tests/test_escalation_engine.py b/spp_grm_cel/tests/test_escalation_engine.py new file mode 100644 index 000000000..b65c423fd --- /dev/null +++ b/spp_grm_cel/tests/test_escalation_engine.py @@ -0,0 +1,273 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Escalation engine behaviour under owner-identity evaluation. + +Covers the engine mechanics around #379's owner-identity model: what happens +when a rule's side effects are denied to its owner, how the hourly pass isolates +failures, that a rule applies once per ticket, that the SLA-breach path (which +runs from a stored compute and is deferred to precommit) behaves in batches, and +that the "Check Escalation" button works for users who cannot read the rules. +""" + +from unittest.mock import patch + +from odoo import Command +from odoo.exceptions import UserError +from odoo.tests import TransactionCase, tagged +from odoo.tools import mute_logger + +ESCALATION = "spp.grm.escalation.rule" +ESC_LOGGER = "odoo.addons.spp_grm_cel.models.grm_escalation_rule" +TICKET_LOGGER = "odoo.addons.spp_grm_cel.models.grm_ticket" +GRM_TICKET_LOGGER = "odoo.addons.spp_grm.models.grm_ticket" + + +@tagged("post_install", "-at_install") +class TestEscalationEngine(TransactionCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + Users = cls.env["res.users"] + internal = cls.env.ref("base.group_user") + officer = cls.env.ref("spp_grm.group_grm_officer") + manager = cls.env.ref("spp_grm.group_grm_manager") + cls.officer = Users.create( + { + "name": "Engine Officer", + "login": "grm_engine_officer", + "group_ids": [Command.link(internal.id), Command.link(officer.id)], + } + ) + cls.manager = Users.create( + { + "name": "Engine Manager", + "login": "grm_engine_manager", + "group_ids": [Command.link(internal.id), Command.link(manager.id)], + } + ) + cls.plain_internal = Users.create( + { + "name": "Engine Plain Internal", + "login": "grm_engine_plain", + "group_ids": [Command.link(internal.id)], + } + ) + Team = cls.env["spp.grm.team"] + cls.team_a = Team.create({"name": "Engine Team A", "member_ids": [Command.link(cls.officer.id)]}) + cls.team_b = Team.create({"name": "Engine Team B"}) + cls.partner = cls.env["res.partner"].create({"name": "Engine Complainant"}) + + def _ticket(self, name, team=None, **extra): + vals = { + "name": name, + "description": name, + "partner_id": self.partner.id, + "team_id": (team or self.team_a).id, + "severity": "critical", + } + vals.update(extra) + return self.env["spp.grm.ticket"].create(vals) + + def _officer_rule(self, **vals): + base = {"name": "Officer rule", "condition_cel": "", "trigger_after_hours": 0} + base.update(vals) + return self.env[ESCALATION].with_user(self.officer).create(base) + + def _manager_rule(self, **vals): + base = {"name": "Manager rule", "condition_cel": "", "trigger_after_hours": 0} + base.update(vals) + return self.env[ESCALATION].with_user(self.manager).create(base) + + def _breach_batch(self, batch): + """Move the batch to a category whose SLA is already past: sla_deadline + and sla_status recompute for the whole batch in one compute call, each + breach schedules the hook, and the flush's precommit stage runs it.""" + past = self.env["spp.grm.ticket.category"].create({"name": "Past SLA", "default_sla_hours": -1}) + self.env.flush_all() + batch.write({"category_id": past.id}) + # cr.flush() is what commit does: run the pending computes (which + # schedule the breach hooks), then the precommit hooks. + self.env.cr.flush() + batch.invalidate_recordset() + + def _db_rows(self, batch): + self.env.cr.execute( + "SELECT id, sla_status, is_escalated, team_id, severity FROM spp_grm_ticket WHERE id IN %s ORDER BY id", + (tuple(batch.ids),), + ) + return self.env.cr.fetchall() + + # ---- SLA-breach path: batch compute, savepoint rollbacks, precommit deferral + + @mute_logger(GRM_TICKET_LOGGER, ESC_LOGGER, TICKET_LOGGER) + def test_batch_breach_out_of_scope_rule_rolls_back_every_ticket(self): + """An officer-owned rule reassigning tickets out of the officer's own + scope is rolled back for each ticket of a batch breach; the batch's + computed sla_status survives intact.""" + self._officer_rule(escalate_to_team_id=self.team_b.id) + batch = self._ticket("t1") | self._ticket("t2") | self._ticket("t3") + self.env.flush_all() + self.assertEqual([t.sla_status for t in batch], ["on_track"] * 3) + self._breach_batch(batch) + rows = self._db_rows(batch) + self.assertEqual([r[1] for r in rows], ["breached"] * 3, rows) + self.assertEqual([r[2] for r in rows], [False] * 3, rows) + self.assertEqual([r[3] for r in rows], [self.team_a.id] * 3, rows) + self.assertEqual([t.sla_status for t in batch], ["breached"] * 3) + + @mute_logger(GRM_TICKET_LOGGER, ESC_LOGGER, TICKET_LOGGER) + def test_batch_breach_in_scope_rule_applies_to_every_ticket(self): + self._manager_rule(escalate_severity="critical") + batch = ( + self._ticket("t1", severity="low") | self._ticket("t2", severity="low") | self._ticket("t3", severity="low") + ) + self.env.flush_all() + self._breach_batch(batch) + rows = self._db_rows(batch) + self.assertEqual([r[1] for r in rows], ["breached"] * 3, rows) + self.assertEqual([r[2] for r in rows], [True] * 3, rows) + self.assertEqual([r[4] for r in rows], ["critical"] * 3, rows) + + @mute_logger(GRM_TICKET_LOGGER, ESC_LOGGER, TICKET_LOGGER) + def test_batch_breach_mixed_scope(self): + """t2 sits in team B (owner cannot read it -> rule does not apply); + its in-scope neighbours are escalated and keep their values.""" + self._officer_rule(escalate_to_user_id=self.officer.id) + batch = self._ticket("t1") | self._ticket("t2", team=self.team_b) | self._ticket("t3") + self.env.flush_all() + self._breach_batch(batch) + rows = self._db_rows(batch) + self.assertEqual([r[1] for r in rows], ["breached"] * 3, rows) + self.assertEqual([r[2] for r in rows], [True, False, True], rows) + + @mute_logger(GRM_TICKET_LOGGER, ESC_LOGGER, TICKET_LOGGER) + def test_breach_hook_is_deferred_out_of_the_compute(self): + """Inside the transaction, right after the write that breaches the SLA, + nothing has been escalated yet: the engine runs at precommit.""" + self._manager_rule(escalate_severity="critical") + ticket = self._ticket("deferred", severity="low") + past = self.env["spp.grm.ticket.category"].create({"name": "Past SLA", "default_sla_hours": -1}) + self.env.flush_all() + ticket.write({"category_id": past.id}) + self.assertEqual(ticket.sla_status, "breached") # compute ran, hook only scheduled + self.assertFalse(ticket.is_escalated) + self.env.cr.flush() + self.assertTrue(ticket.is_escalated) + self.assertEqual(ticket.severity, "critical") + + # ---- side effects the owner is not entitled to fail the escalation closed + + @mute_logger(ESC_LOGGER) + def test_case_creation_denied_to_owner_rolls_escalation_back(self): + """An officer without case-management rights owns a rule with + create_case: the case create is denied, so the escalation is rolled + back and reported as not applied — never 'applied' with no case.""" + case_type = self.env["spp.case.type"].create({"name": "Escalation", "code": "ESC"}) + rule = self._officer_rule(create_case=True, case_type_id=case_type.id, escalate_to_user_id=self.officer.id) + ticket = self._ticket("case denied") + applied = self.env[ESCALATION].sudo().apply_escalations(ticket) + self.assertFalse(applied) + ticket.invalidate_recordset() + self.assertFalse(ticket.is_escalated) + self.assertEqual(rule.escalation_count, 0) + self.assertFalse(self.env["spp.case"].sudo().search([("name", "ilike", ticket.number)])) + + @mute_logger(ESC_LOGGER) + def test_case_creation_by_entitled_owner_creates_case(self): + """A superuser-owned rule (shell/data-load created) with create_case + actually creates the case, with the required case worker set.""" + case_type = self.env["spp.case.type"].create({"name": "Escalation", "code": "ESC"}) + self.env[ESCALATION].create( + {"name": "Case rule", "condition_cel": "", "create_case": True, "case_type_id": case_type.id} + ) + ticket = self._ticket("case created", user_id=self.officer.id) + applied = self.env[ESCALATION].sudo().apply_escalations(ticket) + self.assertTrue(applied) + case = self.env["spp.case"].sudo().search([("name", "ilike", ticket.number)]) + self.assertEqual(len(case), 1) + self.assertEqual(case.case_worker_id, self.officer) + self.assertEqual(case.partner_id, self.partner) + + def test_notification_sent_under_owner_identity(self): + model = self.env["ir.model"]._get("spp.grm.ticket") + template = self.env["mail.template"].create( + { + "name": "Escalation template", + "model_id": model.id, + "subject": "Escalated {{ object.number }}", + "body_html": "

    Escalated

    ", + "email_to": "escalations@example.com", + } + ) + self._officer_rule( + should_send_notification=True, + notification_template_id=template.id, + escalate_to_user_id=self.officer.id, + ) + ticket = self._ticket("notified") + Mail = self.env["mail.mail"].sudo() + domain = [("model", "=", "spp.grm.ticket"), ("res_id", "=", ticket.id)] + before = Mail.search_count(domain) + self.assertTrue(self.env[ESCALATION].sudo().apply_escalations(ticket)) + self.assertEqual(Mail.search_count(domain) - before, 1) + + # ---- pass-level behaviour + + @mute_logger(ESC_LOGGER) + def test_rule_applies_once_per_ticket(self): + rule = self._manager_rule(escalate_severity="critical") + ticket = self._ticket("once", severity="low") + self.env[ESCALATION].sudo().check_escalations() + self.env[ESCALATION].sudo().check_escalations() + rule.invalidate_recordset() + posts = ( + self.env["mail.message"] + .sudo() + .search_count( + [("model", "=", "spp.grm.ticket"), ("res_id", "=", ticket.id), ("subject", "=", "Ticket Escalated")] + ) + ) + self.assertEqual((rule.escalation_count, posts), (1, 1)) + + def test_one_failing_ticket_does_not_abort_the_pass(self): + rule = self._manager_rule(escalate_severity="critical") + failing = self._ticket("failing", severity="low") + healthy = self._ticket("healthy", severity="low") + Model = self.env.registry[ESCALATION] + original = Model.apply_escalation + + def flaky(rule_rec, ticket): + if ticket.id == failing.id: + raise UserError(self.env._("simulated constraint failure")) + return original(rule_rec, ticket) + + with patch.object(Model, "apply_escalation", flaky), self.assertLogs(ESC_LOGGER, level="ERROR") as cm: + self.env[ESCALATION].sudo().check_escalations() + self.assertTrue(any("rolled back and skipped" in line for line in cm.output), cm.output) + (failing | healthy).invalidate_recordset() + self.assertFalse(failing.is_escalated) + self.assertTrue(healthy.is_escalated) + rule.invalidate_recordset() + self.assertEqual(rule.escalation_count, 1) + + def test_owner_warnings_logged_once_per_pass(self): + """A superuser-owned rule is warned about once per cron pass, not once + per open ticket.""" + self.env[ESCALATION].create({"name": "Shell-created rule", "condition_cel": "severity == 'nonexistent'"}) + self._ticket("t1") + self._ticket("t2") + with self.assertLogs(ESC_LOGGER, level="WARNING") as cm: + self.env[ESCALATION].sudo().check_escalations() + hits = [line for line in cm.output if "Shell-created rule" in line and "owned by the superuser" in line] + self.assertEqual(len(hits), 1, cm.output) + + def test_check_escalation_button_for_user_without_rule_access(self): + """A plain internal user (no GRM group, no read on the rules) presses + "Check Escalation": the engine loads the rules itself and applies a + manager's rule with the manager's identity — no swallowed AccessError.""" + self._manager_rule(escalate_severity="critical") + ticket = self._ticket("button", severity="low") + with self.assertNoLogs(TICKET_LOGGER, level="ERROR"): + res = ticket.with_user(self.plain_internal).action_escalate() + self.assertEqual(res["params"]["type"], "info") + ticket.invalidate_recordset() + self.assertTrue(ticket.is_escalated) diff --git a/spp_grm_cel/tests/test_rule_owner_identity.py b/spp_grm_cel/tests/test_rule_owner_identity.py index 5fd96470c..4da98ee41 100644 --- a/spp_grm_cel/tests/test_rule_owner_identity.py +++ b/spp_grm_cel/tests/test_rule_owner_identity.py @@ -215,6 +215,72 @@ def test_operational_toggle_does_not_rebind_owner(self): "Toggling active/sequence must not re-author rule ownership", ) + def test_take_ownership_rebinds_to_acting_user(self): + """The remediation path for superuser-, archived- or mis-scoped owners: + "Take Ownership" re-binds the rule to whoever presses it, on both models.""" + for model in (ROUTING, ESCALATION): + rule = self.env[model].with_user(self.officer).create({"name": "Handed over", "condition_cel": ""}) + self.assertEqual(rule.eval_as_user_id, self.officer) + rule.with_user(self.manager).action_take_ownership() + rule.invalidate_recordset() + self.assertEqual(rule.eval_as_user_id, self.manager, model) + + def test_eval_as_user_id_client_write_only_accepted_for_self(self): + """A direct write may only set the identity to the acting user; any other + value is stripped (the forgery guard from #379 stands).""" + rule = self.env[ESCALATION].with_user(self.officer).create({"name": "Self only", "condition_cel": ""}) + rule.with_user(self.manager).write({"eval_as_user_id": self.env.ref("base.user_admin").id}) + rule.invalidate_recordset() + self.assertEqual(rule.eval_as_user_id, self.officer, "third-party identity must be stripped") + rule.with_user(self.manager).write({"eval_as_user_id": self.manager.id}) + rule.invalidate_recordset() + self.assertEqual(rule.eval_as_user_id, self.manager, "self identity is the take-ownership path") + + def test_archived_owner_rule_does_not_fire(self): + """An offboarded (archived) owner's scope must not keep driving + automation: the rule is skipped with a warning until someone takes + ownership. Both engines.""" + officer_ticket = self.env["spp.grm.ticket"].create( + { + "name": "Officer ticket", + "description": "In the officer's team", + "partner_id": self.foreign_ticket.partner_id.id, + "team_id": self.team_a.id, + "severity": "critical", + } + ) + esc_rule = ( + self.env[ESCALATION] + .with_user(self.officer) + .create({"name": "Archived owner", "condition_cel": "", "escalate_to_user_id": self.officer.id}) + ) + route_rule = ( + self.env[ROUTING] + .with_user(self.officer) + .create({"name": "Archived owner", "condition_cel": "", "assign_user_id": self.officer.id}) + ) + self.officer.sudo().write({"active": False}) + + with self.assertLogs("odoo.addons.spp_grm_cel.models.grm_escalation_rule", level="WARNING") as cm: + self.env[ESCALATION].sudo().check_escalations() + self.assertTrue(any("archived user" in line for line in cm.output), cm.output) + officer_ticket.invalidate_recordset() + self.assertFalse(officer_ticket.is_escalated) + self.assertEqual(esc_rule.escalation_count, 0) + + with self.assertLogs("odoo.addons.spp_grm_cel.models.grm_routing_rule", level="WARNING") as cm: + routed = self.env["spp.grm.ticket"].create( + { + "name": "New ticket", + "description": "Routed after the owner was archived", + "partner_id": self.foreign_ticket.partner_id.id, + "team_id": self.team_a.id, + } + ) + self.assertTrue(any("archived user" in line for line in cm.output), cm.output) + self.assertNotEqual(routed.user_id, self.officer) + self.assertNotEqual(routed.routing_rule_id, route_rule) + def test_escalation_counter_increments_under_owner_identity(self): """The escalation counter is incremented (atomically) when a manager's rule applies via the elevated path. Applied to one explicit ticket — @@ -226,9 +292,12 @@ def test_escalation_counter_increments_under_owner_identity(self): .create({"name": "Counter rule", "condition_cel": "", "escalate_severity": "high"}) ) before = rule.escalation_count - self.env[ESCALATION].sudo().apply_escalations(self.foreign_ticket) + applied = self.env[ESCALATION].sudo().apply_escalations(self.foreign_ticket) + self.assertTrue(applied) rule.invalidate_recordset() self.assertEqual(rule.escalation_count, before + 1) + self.foreign_ticket.invalidate_recordset() + self.assertTrue(self.foreign_ticket.is_escalated) def test_entry_points_not_rpc_callable(self): """#381: all four rule-engine methods must be rejected for RPC dispatch.""" diff --git a/spp_grm_cel/views/grm_escalation_rule_views.xml b/spp_grm_cel/views/grm_escalation_rule_views.xml index 32a324ee9..475b9cc4f 100644 --- a/spp_grm_cel/views/grm_escalation_rule_views.xml +++ b/spp_grm_cel/views/grm_escalation_rule_views.xml @@ -24,6 +24,16 @@ spp.grm.escalation.rule
    +
    +
    Check diff --git a/spp_grm_cel/views/grm_routing_rule_views.xml b/spp_grm_cel/views/grm_routing_rule_views.xml index 64d949e5d..f011199b0 100644 --- a/spp_grm_cel/views/grm_routing_rule_views.xml +++ b/spp_grm_cel/views/grm_routing_rule_views.xml @@ -25,6 +25,16 @@ spp.grm.routing.rule +
    +
    Date: Wed, 2 Sep 2026 11:27:21 +0800 Subject: [PATCH 07/15] style(grm_cel): wrap over-long log/nosemgrep lines (#415) --- spp_grm_cel/models/grm_escalation_rule.py | 5 +++-- spp_grm_cel/models/grm_routing_rule.py | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/spp_grm_cel/models/grm_escalation_rule.py b/spp_grm_cel/models/grm_escalation_rule.py index 9f650d2a4..52bacea32 100644 --- a/spp_grm_cel/models/grm_escalation_rule.py +++ b/spp_grm_cel/models/grm_escalation_rule.py @@ -610,7 +610,8 @@ def _evaluation_owner(self): return owner if not owner.active: _logger.warning( - "Escalation rule %s (id %s) is owned by archived user %s; skipping until someone takes ownership of it.", + "Escalation rule %s (id %s) is owned by archived user %s; skipping until " + "someone takes ownership of it.", self.name, self.id, owner.login, @@ -628,7 +629,7 @@ def _active_rules_with_owners(self): is applied with. Rules without a usable owner are dropped here, so the cron logs each warning once per pass rather than once per ticket. """ - # nosemgrep: semgrep.odoo-sudo-without-context -- reads only the rule set; every effect below runs with_user(owner), never elevated + # nosemgrep: semgrep.odoo-sudo-without-context -- reads the rule set only; effects run with_user(owner) rules = self.sudo().search([("active", "=", True)], order="sequence, id") return [(rule, owner) for rule in rules if (owner := rule._evaluation_owner())] diff --git a/spp_grm_cel/models/grm_routing_rule.py b/spp_grm_cel/models/grm_routing_rule.py index eb7dd2e7c..9fae207ac 100644 --- a/spp_grm_cel/models/grm_routing_rule.py +++ b/spp_grm_cel/models/grm_routing_rule.py @@ -222,7 +222,7 @@ def _active_rules_with_owners(self): because every effect is bounded by the owner identity each rule is applied with. Rules without a usable owner are dropped here. """ - # nosemgrep: semgrep.odoo-sudo-without-context -- reads only the rule set; every effect below runs with_user(owner), never elevated + # nosemgrep: semgrep.odoo-sudo-without-context -- reads the rule set only; effects run with_user(owner) rules = self.sudo().search([("active", "=", True)], order="sequence, id") return [(rule, owner) for rule in rules if (owner := rule._evaluation_owner())] From 205ba2898dae6cbb7b8055d9b927934a009d0abd Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Wed, 2 Sep 2026 11:27:21 +0800 Subject: [PATCH 08/15] docs(grm): regenerate READMEs from CI generator (#415) --- spp_grm/README.rst | 5 ++ spp_grm/static/description/index.html | 5 ++ spp_grm_cel/README.rst | 57 +++++++++++++++-------- spp_grm_cel/static/description/index.html | 57 +++++++++++++++-------- 4 files changed, 86 insertions(+), 38 deletions(-) diff --git a/spp_grm/README.rst b/spp_grm/README.rst index 9e58df251..0f0e0f737 100644 --- a/spp_grm/README.rst +++ b/spp_grm/README.rst @@ -165,6 +165,11 @@ Changelog portal controller, which needs no direct model write). The rule covers all four operations, so the scoping also holds if a future access-control change ever re-grants portal write. +- fix: SLA-breach handling (auto-escalation and the breach chatter note) + no longer runs inside the stored ``sla_status`` compute. It is + deferred to the end of the triggering transaction, so the escalation + engine's writes, savepoints and flushes never execute mid-computation. + Same transaction, same outcome. 19.0.2.0.1 ~~~~~~~~~~ diff --git a/spp_grm/static/description/index.html b/spp_grm/static/description/index.html index 37db20135..8e1923c9c 100644 --- a/spp_grm/static/description/index.html +++ b/spp_grm/static/description/index.html @@ -547,6 +547,11 @@

    19.0.2.0.2

    portal controller, which needs no direct model write). The rule covers all four operations, so the scoping also holds if a future access-control change ever re-grants portal write. +
  • fix: SLA-breach handling (auto-escalation and the breach chatter note) +no longer runs inside the stored sla_status compute. It is +deferred to the end of the triggering transaction, so the escalation +engine’s writes, savepoints and flushes never execute mid-computation. +Same transaction, same outcome.
    • diff --git a/spp_grm_cel/README.rst b/spp_grm_cel/README.rst index 20e75a7d8..bca185b1c 100644 --- a/spp_grm_cel/README.rst +++ b/spp_grm_cel/README.rst @@ -133,36 +133,55 @@ Changelog route/escalate tickets within that officer's own record-rule scope, closing the path where an always-match rule applied by the hourly cron could seize every ticket in the database (#379). Existing rules are - backfilled from ``create_uid`` by a migration; note that a user who - owns rules can no longer be deleted (``ondelete="restrict"``) — - archive them instead, and a rule owned by the superuser (created from - a shell, import script, or data load) still evaluates without + backfilled from ``create_uid`` by a migration. A user who owns rules + can no longer be deleted (``ondelete="restrict"``) — archive them + instead; a rule whose owner is archived stops firing (logged) until + someone takes ownership of it. A rule owned by the superuser (created + from a shell, import script, or data load) still evaluates without record-rule bounds and is called out with a warning by both the - migration and the rule engine. + migration and the rule engine. The new **Take Ownership** button on + the rule form re-binds a rule to yourself; saving the form without + changing what the rule targets does not. - fix(security): the rule-engine entry points (``apply_routing``, ``apply_escalations``, ``apply_escalation``, ``check_escalations``) are marked ``@api.private`` — no longer callable over RPC (#381). - fix(security): drop the portal and internal-user read rows on both - rule models. With owner-identity evaluation the acting user never - reads the rules, so the rows only exposed the routing/escalation map - (conditions, targets, thresholds) to enumeration (hardening alongside - #379/#381). -- fix: an escalation is now applied atomically (savepoint): if any - post-write step — the chatter post, notification, or case creation — - is denied because the rule just reassigned the ticket out of its - owner's own scope, the whole escalation rolls back and is skipped - instead of persisting half-applied with its chatter message silently - lost. + rule models. The engine now loads the active rule set with elevated + rights and applies each rule with its owner's identity, so no acting + user needs read access on the rules; the rows only exposed the + routing/escalation map (conditions, targets, thresholds) to + enumeration (hardening alongside #379/#381). The ticket form's "Check + Escalation" button is limited to GRM officers and above. +- fix: an escalation is now applied atomically (savepoint). The ticket + write, the chatter post and the counter succeed or roll back together, + and if the rule owner is denied any effect the rule produces — posting + to a ticket just reassigned out of their own scope, sending the + configured template, or creating the configured case — the whole + escalation rolls back and is skipped instead of persisting + half-applied. Delivery or data errors in the notification and case + steps remain best effort: logged, skipped, and isolated so they cannot + abort the pass. +- fix: a rule applies at most once per ticket. Previously the hourly + cron re-escalated every still-open matching ticket on every pass, + repeating the counter increment, the chatter post and the notification + each hour. +- fix: one failing ticket no longer aborts the whole escalation pass; + the failure is logged and the remaining tickets are processed. +- fix: case creation from an escalation rule never worked (it passed a + field ``spp.case`` does not have and omitted the required case + worker); it now fills ``presenting_issue`` and assigns the ticket + assignee or the rule owner as case worker. - fix: increment ``match_count`` / ``escalation_count`` with an atomic ``UPDATE`` instead of a read-modify-write, avoiding a serialization failure under concurrent cron/UI escalation whose dispatch-level retry would re-run the whole cron pass. - fix: rule CEL validation now reports any parser error as a ``ValidationError`` (previously only ``SyntaxError`` was caught). -- fix: the hourly escalation cron searches the active rule set once per - pass instead of once per open ticket, and the engine logs (instead of - silently skipping) rules with no evaluation identity and tickets - skipped for lack of owner access. +- fix: the hourly escalation cron resolves the active rule set and each + rule's evaluation owner once per pass instead of once per open ticket + (so owner warnings are logged once, not once per ticket), and the + engine logs (instead of silently skipping) rules with no evaluation + identity and tickets skipped for lack of owner access. 19.0.2.0.1 ~~~~~~~~~~ diff --git a/spp_grm_cel/static/description/index.html b/spp_grm_cel/static/description/index.html index d9b399110..86f593c9d 100644 --- a/spp_grm_cel/static/description/index.html +++ b/spp_grm_cel/static/description/index.html @@ -519,36 +519,55 @@

    19.0.2.0.2

    route/escalate tickets within that officer’s own record-rule scope, closing the path where an always-match rule applied by the hourly cron could seize every ticket in the database (#379). Existing rules are -backfilled from create_uid by a migration; note that a user who -owns rules can no longer be deleted (ondelete="restrict") — -archive them instead, and a rule owned by the superuser (created from -a shell, import script, or data load) still evaluates without +backfilled from create_uid by a migration. A user who owns rules +can no longer be deleted (ondelete="restrict") — archive them +instead; a rule whose owner is archived stops firing (logged) until +someone takes ownership of it. A rule owned by the superuser (created +from a shell, import script, or data load) still evaluates without record-rule bounds and is called out with a warning by both the -migration and the rule engine. +migration and the rule engine. The new Take Ownership button on +the rule form re-binds a rule to yourself; saving the form without +changing what the rule targets does not.
  • fix(security): the rule-engine entry points (apply_routing, apply_escalations, apply_escalation, check_escalations) are marked @api.private — no longer callable over RPC (#381).
  • fix(security): drop the portal and internal-user read rows on both -rule models. With owner-identity evaluation the acting user never -reads the rules, so the rows only exposed the routing/escalation map -(conditions, targets, thresholds) to enumeration (hardening alongside -#379/#381).
    • -
  • fix: an escalation is now applied atomically (savepoint): if any -post-write step — the chatter post, notification, or case creation — -is denied because the rule just reassigned the ticket out of its -owner’s own scope, the whole escalation rolls back and is skipped -instead of persisting half-applied with its chatter message silently -lost.
    • +rule models. The engine now loads the active rule set with elevated +rights and applies each rule with its owner’s identity, so no acting +user needs read access on the rules; the rows only exposed the +routing/escalation map (conditions, targets, thresholds) to +enumeration (hardening alongside #379/#381). The ticket form’s “Check +Escalation” button is limited to GRM officers and above. +
  • fix: an escalation is now applied atomically (savepoint). The ticket +write, the chatter post and the counter succeed or roll back together, +and if the rule owner is denied any effect the rule produces — posting +to a ticket just reassigned out of their own scope, sending the +configured template, or creating the configured case — the whole +escalation rolls back and is skipped instead of persisting +half-applied. Delivery or data errors in the notification and case +steps remain best effort: logged, skipped, and isolated so they cannot +abort the pass.
    • +
  • fix: a rule applies at most once per ticket. Previously the hourly +cron re-escalated every still-open matching ticket on every pass, +repeating the counter increment, the chatter post and the notification +each hour.
    • +
  • fix: one failing ticket no longer aborts the whole escalation pass; +the failure is logged and the remaining tickets are processed.
    • +
  • fix: case creation from an escalation rule never worked (it passed a +field spp.case does not have and omitted the required case +worker); it now fills presenting_issue and assigns the ticket +assignee or the rule owner as case worker.
  • fix: increment match_count / escalation_count with an atomic UPDATE instead of a read-modify-write, avoiding a serialization failure under concurrent cron/UI escalation whose dispatch-level retry would re-run the whole cron pass.
  • fix: rule CEL validation now reports any parser error as a ValidationError (previously only SyntaxError was caught).
    • -
  • fix: the hourly escalation cron searches the active rule set once per -pass instead of once per open ticket, and the engine logs (instead of -silently skipping) rules with no evaluation identity and tickets -skipped for lack of owner access.
    • +
  • fix: the hourly escalation cron resolves the active rule set and each +rule’s evaluation owner once per pass instead of once per open ticket +(so owner warnings are logged once, not once per ticket), and the +engine logs (instead of silently skipping) rules with no evaluation +identity and tickets skipped for lack of owner access.
    • From 6d8e446054cf65832f2786e9033a202ced9488a8 Mon Sep 17 00:00:00 2001 From: Ken Lewerentz Date: Wed, 2 Sep 2026 12:34:24 +0700 Subject: [PATCH 09/15] fix(grm): keep unsaved form edits out of the SLA breach queue sla_status is a stored compute rendered on the ticket form, so it also runs on the pseudo-record of an onchange. records.ids resolves those to their origin, so an edit the user never saved queued the real ticket and escalated it at the next commit: counter, chatter, notification. Drop pseudo-records before queueing; NewId is falsy, so ticket.id tells them apart. --- spp_grm/models/grm_ticket.py | 13 +++++++++++-- spp_grm/readme/HISTORY.md | 4 +++- spp_grm/tests/test_sla_breach.py | 14 +++++++++++++- 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/spp_grm/models/grm_ticket.py b/spp_grm/models/grm_ticket.py index 0636cd30f..ff304e8a8 100644 --- a/spp_grm/models/grm_ticket.py +++ b/spp_grm/models/grm_ticket.py @@ -546,10 +546,19 @@ def _schedule_sla_breach(self): inside a stored compute, so it is deferred to the transaction's precommit stage and runs at the next full flush/commit, still in the same transaction (the way mail.thread defers its tracking messages). + + Pseudo-records are dropped first. A stored compute also runs on the + unsaved record of a form onchange, and ``ids`` resolves those to their + origin, so queueing them would run the breach hook — escalation engine + included — against the real ticket for an edit that was never saved. + A NewId is falsy, so ``ticket.id`` tells the two apart. """ + tickets = self.filtered(lambda ticket: ticket.id) + if not tickets: + return pending = self.env.cr.precommit.data.setdefault("spp_grm.sla_breach_ids", set()) - pending.update(self.ids) - self.env.cr.precommit.add(self._run_sla_breach_hooks) + pending.update(tickets.ids) + self.env.cr.precommit.add(tickets._run_sla_breach_hooks) def _run_sla_breach_hooks(self): """Precommit callback: run the breach hook for every ticket queued so far.""" diff --git a/spp_grm/readme/HISTORY.md b/spp_grm/readme/HISTORY.md index 7ba3e0f47..b36732d13 100644 --- a/spp_grm/readme/HISTORY.md +++ b/spp_grm/readme/HISTORY.md @@ -10,7 +10,9 @@ - fix: SLA-breach handling (auto-escalation and the breach chatter note) no longer runs inside the stored ``sla_status`` compute. It is deferred to the end of the triggering transaction, so the escalation engine's writes, savepoints and flushes never execute mid-computation. Same - transaction, same outcome. + transaction, same outcome. An unsaved form edit queues nothing: the compute also runs on the + pseudo-record of an onchange, whose ids resolve back to the real ticket, which would have + escalated it for a change the user never saved. ### 19.0.2.0.1 diff --git a/spp_grm/tests/test_sla_breach.py b/spp_grm/tests/test_sla_breach.py index 6ca57f7bb..e66421a61 100644 --- a/spp_grm/tests/test_sla_breach.py +++ b/spp_grm/tests/test_sla_breach.py @@ -7,7 +7,7 @@ may run mid-computation. """ -from odoo.tests import TransactionCase, tagged +from odoo.tests import Form, TransactionCase, tagged @tagged("post_install", "-at_install") @@ -44,6 +44,18 @@ def test_breach_note_posted_at_precommit_for_the_whole_batch(self): self.env.cr.flush() self.assertEqual(self._breach_notes() - before, 2) + def test_form_onchange_does_not_queue_unsaved_records(self): + """``sla_status`` is a stored compute shown on the ticket form, so it + also runs on the unsaved pseudo-record of an onchange. Those pseudo-ids + must not reach the queue: ``exists()`` keeps new records by convention, + so the hook would drive the escalation engine — counter, chatter, + notification — from a form edit that was never saved.""" + self.env.flush_all() + form = Form(self.tickets[0]) + form.category_id = self.past + self.assertEqual(form.sla_status, "breached", "the compute must still run on the pseudo-record") + self.assertEqual(self.env.cr.precommit.data.get("spp_grm.sla_breach_ids", set()), set()) + def test_breach_hook_runs_once_per_ticket(self): """Several schedulings within one transaction collapse into one run.""" self.env.flush_all() From 1926b20e0aca627436b1eff36e1d94558e0a6a10 Mon Sep 17 00:00:00 2001 From: Ken Lewerentz Date: Wed, 2 Sep 2026 12:34:32 +0700 Subject: [PATCH 10/15] fix(grm_cel): bound rule side effects and the manual escalation check Three fixes from review of #415: - The escalation notification was sent before case creation, so an owner denied the case create had already mailed a rolled-back escalation. The rollback also drops the escalation_rule_ids link that suppresses the rule, so the hourly cron repeated the ghost mail every pass. Send it last, after every effect that can still be denied. - apply_routing wrote the ticket and bumped the counter with no savepoint. Its caller swallows exceptions, so a database failure there left an aborted cursor that took down every later statement of the request. Wrap it the way the escalation path already is. - action_escalate is RPC-dispatchable, so the button's groups= gated nothing: any internal user could force a full escalation pass on any ticket. Require write access on the ticket, which also keeps an officer inside their own scope. --- spp_grm_cel/models/grm_escalation_rule.py | 14 +++-- spp_grm_cel/models/grm_routing_rule.py | 46 +++++++++----- spp_grm_cel/models/grm_ticket.py | 11 ++++ spp_grm_cel/readme/HISTORY.md | 16 ++++- spp_grm_cel/tests/test_escalation_engine.py | 67 ++++++++++++++++++--- spp_grm_cel/tests/test_routing_rules.py | 51 ++++++++++++++++ 6 files changed, 173 insertions(+), 32 deletions(-) diff --git a/spp_grm_cel/models/grm_escalation_rule.py b/spp_grm_cel/models/grm_escalation_rule.py index 52bacea32..3a1639b4a 100644 --- a/spp_grm_cel/models/grm_escalation_rule.py +++ b/spp_grm_cel/models/grm_escalation_rule.py @@ -421,7 +421,7 @@ def apply_escalation(self, ticket): vals, ) - # Post to chatter BEFORE any external side effect. spp.grm.ticket sets + # Post to chatter before the notification goes out. spp.grm.ticket sets # _mail_post_access = "read", so posting needs read access on the # ticket, which the rule owner may have just lost by reassigning it out # of their own scope. When that happens the caller's savepoint rolls @@ -435,10 +435,6 @@ def apply_escalation(self, ticket): subject=_("Ticket Escalated"), ) - # Send notification if configured - if self.should_send_notification and self.notification_template_id: - self._send_escalation_notification(ticket) - # Create case if configured if self.create_case and self.case_type_id: self._create_case_from_ticket(ticket) @@ -456,6 +452,14 @@ def apply_escalation(self, ticket): ) self.invalidate_recordset(["escalation_count"]) + # Sent last, once every effect that can still be denied has succeeded: + # delivery is the one step no rollback takes back. A mail sent for an + # escalation the caller then rolls back is a ghost that also repeats — + # the rolled-back escalation_rule_ids link no longer suppresses the + # rule, so the next hourly pass sends it again. + if self.should_send_notification and self.notification_template_id: + self._send_escalation_notification(ticket) + return True def _send_escalation_notification(self, ticket): diff --git a/spp_grm_cel/models/grm_routing_rule.py b/spp_grm_cel/models/grm_routing_rule.py index 9fae207ac..a3f2860f8 100644 --- a/spp_grm_cel/models/grm_routing_rule.py +++ b/spp_grm_cel/models/grm_routing_rule.py @@ -394,7 +394,28 @@ def apply_routing(self, ticket): vals["priority"] = rule.set_priority try: - ticket_as_owner.write(vals) + # Savepoint: the write and the counter succeed or roll back + # together, and the write's deferred SQL is flushed on the + # savepoint's close, so a constraint or foreign-key failure + # (a rule pointing at a since-deleted user) rolls back here. + # Our caller swallows exceptions; without this the aborted + # cursor would take down every later statement of the same + # request, including the rest of a portal submission. + with self.env.cr.savepoint(): + ticket_as_owner.write(vals) + + # Atomic increment: a read-modify-write here would raise a + # serialization failure under concurrent routing (cursors run + # REPEATABLE READ), retried only at whole-dispatch granularity. + # A single UPDATE avoids the conflict entirely, and needs no + # sudo (raw SQL bypasses ACL). Invisible to spp_audit ORM + # write-hooks, which is acceptable for a statistics counter. + rule.flush_recordset(["match_count"]) + self.env.cr.execute( + "UPDATE spp_grm_routing_rule SET match_count = match_count + 1 WHERE id = %s", + (rule.id,), + ) + rule.invalidate_recordset(["match_count"]) except AccessError: # Owner may match the ticket but not be allowed to write it # (e.g. read-only scope). Skip rather than apply elevated. @@ -405,6 +426,16 @@ def apply_routing(self, ticket): ticket.id, ) continue + except Exception: + # Per-ticket isolation, as on the escalation side: the + # savepoint has already rolled this routing back, and one + # bad rule must not cost the ticket its creation. + _logger.exception( + "Routing rule %s failed on ticket %s; rolled back and skipped.", + rule.name, + ticket.id, + ) + continue _logger.info( "Applied routing rule '%s' to ticket %s: %s", rule.name, @@ -412,19 +443,6 @@ def apply_routing(self, ticket): vals, ) - # Atomic increment: a read-modify-write here would raise a - # serialization failure under concurrent routing (cursors run - # REPEATABLE READ), retried only at whole-dispatch granularity. - # A single UPDATE avoids the conflict entirely, and needs no - # sudo (raw SQL bypasses ACL). Invisible to spp_audit ORM - # write-hooks, which is acceptable for a statistics counter. - rule.flush_recordset(["match_count"]) - self.env.cr.execute( - "UPDATE spp_grm_routing_rule SET match_count = match_count + 1 WHERE id = %s", - (rule.id,), - ) - rule.invalidate_recordset(["match_count"]) - # Only apply the first matching rule return True diff --git a/spp_grm_cel/models/grm_ticket.py b/spp_grm_cel/models/grm_ticket.py index 8dd8ea55f..28f157f22 100644 --- a/spp_grm_cel/models/grm_ticket.py +++ b/spp_grm_cel/models/grm_ticket.py @@ -105,8 +105,19 @@ def action_escalate(self): """Manual action to trigger escalation rule evaluation. Can be called from the UI to manually check if any escalation rules apply. + + The form button carries ``groups="spp_grm.group_grm_officer"``, but this + method is dispatchable over RPC, where a view attribute guards nothing: + base.group_user holds unscoped read on the tickets, so any internal user + could otherwise force a full escalation pass — counters, chatter posts, + notification mail, case creation — on any ticket in the database. The + engine itself stays elevated (each rule is bounded by its own owner); + what is checked here is entitlement to drive it. Write access is the + button's audience — officers and above — and, unlike a group test, it + also honours the caller's own ticket scope. """ self.ensure_one() + self.check_access("write") self._check_escalation_rules(self) return { diff --git a/spp_grm_cel/readme/HISTORY.md b/spp_grm_cel/readme/HISTORY.md index 0bfb43471..7bd199ff9 100644 --- a/spp_grm_cel/readme/HISTORY.md +++ b/spp_grm_cel/readme/HISTORY.md @@ -18,13 +18,23 @@ now loads the active rule set with elevated rights and applies each rule with its owner's identity, so no acting user needs read access on the rules; the rows only exposed the routing/escalation map (conditions, targets, thresholds) to enumeration (hardening alongside - #379/#381). The ticket form's "Check Escalation" button is limited to GRM officers and above. + #379/#381). The ticket form's "Check Escalation" button is limited to GRM officers and above, + enforced on the method itself — a view ``groups=`` does not bind an RPC call — by requiring + write access on the ticket, which also keeps an officer to their own ticket scope. - fix: an escalation is now applied atomically (savepoint). The ticket write, the chatter post and the counter succeed or roll back together, and if the rule owner is denied any effect the rule produces — posting to a ticket just reassigned out of their own scope, sending the configured template, or creating the configured case — the whole escalation rolls back and is - skipped instead of persisting half-applied. Delivery or data errors in the notification and - case steps remain best effort: logged, skipped, and isolated so they cannot abort the pass. + skipped instead of persisting half-applied. The notification is sent last, after every effect + that can still be denied, because delivery is the one step a rollback cannot take back — and a + ghost mail would repeat, the rolled-back rule link no longer suppressing the rule on the next + pass. Delivery or data errors in the notification and case steps remain best effort: logged, + skipped, and isolated so they cannot abort the pass. +- fix: applying a routing rule is atomic in the same way. The ticket write and the match counter + succeed or roll back together, and a database error while routing (a rule pointing at a + since-deleted user) no longer leaves the transaction aborted: the ticket create that triggered + the routing swallows the error, so every later statement of the same request — the rest of a + portal submission — used to fail behind it. - fix: a rule applies at most once per ticket. Previously the hourly cron re-escalated every still-open matching ticket on every pass, repeating the counter increment, the chatter post and the notification each hour. diff --git a/spp_grm_cel/tests/test_escalation_engine.py b/spp_grm_cel/tests/test_escalation_engine.py index b65c423fd..430f87fa0 100644 --- a/spp_grm_cel/tests/test_escalation_engine.py +++ b/spp_grm_cel/tests/test_escalation_engine.py @@ -11,7 +11,7 @@ from unittest.mock import patch from odoo import Command -from odoo.exceptions import UserError +from odoo.exceptions import AccessError, UserError from odoo.tests import TransactionCase, tagged from odoo.tools import mute_logger @@ -187,17 +187,19 @@ def test_case_creation_by_entitled_owner_creates_case(self): self.assertEqual(case.case_worker_id, self.officer) self.assertEqual(case.partner_id, self.partner) - def test_notification_sent_under_owner_identity(self): - model = self.env["ir.model"]._get("spp.grm.ticket") - template = self.env["mail.template"].create( + def _notification_template(self): + return self.env["mail.template"].create( { "name": "Escalation template", - "model_id": model.id, + "model_id": self.env["ir.model"]._get("spp.grm.ticket").id, "subject": "Escalated {{ object.number }}", "body_html": "

    Escalated

    ", "email_to": "escalations@example.com", } ) + + def test_notification_sent_under_owner_identity(self): + template = self._notification_template() self._officer_rule( should_send_notification=True, notification_template_id=template.id, @@ -210,6 +212,37 @@ def test_notification_sent_under_owner_identity(self): self.assertTrue(self.env[ESCALATION].sudo().apply_escalations(ticket)) self.assertEqual(Mail.search_count(domain) - before, 1) + @mute_logger(ESC_LOGGER) + def test_no_notification_is_sent_for_an_escalation_that_rolls_back(self): + """The sent mail is the one effect a rollback cannot undo, so it must + be the last thing an escalation does. An officer who may send mail but + not create cases owns a rule configured for both: the case create is + denied and the escalation rolls back — including the + escalation_rule_ids link that stops the rule applying twice, so a mail + sent here would go out again on every hourly pass.""" + case_type = self.env["spp.case.type"].create({"name": "Escalation", "code": "ESC"}) + self._officer_rule( + should_send_notification=True, + notification_template_id=self._notification_template().id, + create_case=True, + case_type_id=case_type.id, + escalate_to_user_id=self.officer.id, + ) + ticket = self._ticket("rolled back") + sent = [] + MailTemplate = self.env.registry["mail.template"] + original = MailTemplate.send_mail + + def tracking_send_mail(template, res_id, *args, **kwargs): + sent.append(res_id) + return original(template, res_id, *args, **kwargs) + + with patch.object(MailTemplate, "send_mail", tracking_send_mail): + applied = self.env[ESCALATION].sudo().apply_escalations(ticket) + + self.assertFalse(applied) + self.assertEqual(sent, [], "notification sent for an escalation that was then rolled back") + # ---- pass-level behaviour @mute_logger(ESC_LOGGER) @@ -260,14 +293,28 @@ def test_owner_warnings_logged_once_per_pass(self): hits = [line for line in cm.output if "Shell-created rule" in line and "owned by the superuser" in line] self.assertEqual(len(hits), 1, cm.output) - def test_check_escalation_button_for_user_without_rule_access(self): - """A plain internal user (no GRM group, no read on the rules) presses - "Check Escalation": the engine loads the rules itself and applies a - manager's rule with the manager's identity — no swallowed AccessError.""" + def test_check_escalation_button_denied_to_a_read_only_internal_user(self): + """action_escalate is dispatchable over RPC, so the view's ``groups=`` + gates nothing: without a server-side check any internal user ( + base.group_user holds unscoped read on the tickets) can force a full + escalation pass — counters, chatter, notifications, cases — on any + ticket in the database.""" + self._manager_rule(escalate_severity="critical") + ticket = self._ticket("guarded", severity="low") + with self.assertRaises(AccessError): + ticket.with_user(self.plain_internal).action_escalate() + ticket.invalidate_recordset() + self.assertFalse(ticket.is_escalated) + self.assertEqual(ticket.severity, "low") + + def test_check_escalation_button_applies_a_rule_owned_by_someone_else(self): + """An officer presses "Check Escalation" on a ticket in their own team: + the engine loads the rules itself and applies a manager's rule with the + manager's identity — no swallowed AccessError.""" self._manager_rule(escalate_severity="critical") ticket = self._ticket("button", severity="low") with self.assertNoLogs(TICKET_LOGGER, level="ERROR"): - res = ticket.with_user(self.plain_internal).action_escalate() + res = ticket.with_user(self.officer).action_escalate() self.assertEqual(res["params"]["type"], "info") ticket.invalidate_recordset() self.assertTrue(ticket.is_escalated) diff --git a/spp_grm_cel/tests/test_routing_rules.py b/spp_grm_cel/tests/test_routing_rules.py index 7c3d26596..42e29a5d6 100644 --- a/spp_grm_cel/tests/test_routing_rules.py +++ b/spp_grm_cel/tests/test_routing_rules.py @@ -1,8 +1,13 @@ # Part of OpenSPP. See LICENSE file for full copyright and licensing details. +from unittest.mock import patch + from odoo import Command from odoo.exceptions import ValidationError from odoo.tests.common import TransactionCase +from odoo.tools import mute_logger + +ROUTING_LOGGER = "odoo.addons.spp_grm_cel.models.grm_routing_rule" class TestRoutingRules(TransactionCase): @@ -372,3 +377,49 @@ def test_routing_rule_validates_cel_syntax(self): "assign_team_id": self.team1.id, } ) + + def test_routing_failure_is_contained_by_a_savepoint(self): + """A database error while a rule is applied must not poison the + caller's transaction. The caller (``_apply_routing_rules``) swallows + the exception, so without a savepoint the cursor stays aborted and + every later statement of the same request — the rest of a portal + submission's own create — dies with InFailedSqlTransaction.""" + self.RoutingRule.create( + { + "name": "Failing Rule", + "condition_cel": "severity == 'critical'", + "assign_team_id": self.team1.id, + } + ) + + Ticket = self.env.registry["spp.grm.ticket"] + original_write = Ticket.write + + def failing_write(tickets, vals): + if "routing_rule_id" in vals: + # A genuine PostgreSQL error (what a stale assign_user_id FK + # would raise on flush): only a savepoint rollback makes the + # cursor usable again. + tickets.env.cr.execute("SELECT 1 / 0") + return original_write(tickets, vals) + + with ( + patch.object(Ticket, "write", failing_write), + mute_logger("odoo.sql_db"), + self.assertLogs(ROUTING_LOGGER, level="ERROR") as cm, + ): + ticket = self.Ticket.create( + { + "name": "Critical Issue", + "description": "Test", + "severity": "critical", + "stage_id": self.stage.id, + "partner_id": self.partner.id, + } + ) + + self.assertTrue(any("rolled back and skipped" in line for line in cm.output), cm.output) + # The cursor survived: this query would raise InFailedSqlTransaction + # if the failed statement had aborted the transaction. + self.assertEqual(self.Ticket.search_count([("id", "=", ticket.id)]), 1) + self.assertFalse(ticket.routing_rule_id) From 9b6a9776f9eda9b2312a918c4beb5298ea19bbca Mon Sep 17 00:00:00 2001 From: Ken Lewerentz Date: Wed, 2 Sep 2026 12:34:37 +0700 Subject: [PATCH 11/15] docs(grm): regenerate READMEs from the updated changelog fragments --- spp_grm/README.rst | 5 ++++- spp_grm/static/description/index.html | 5 ++++- spp_grm_cel/README.rst | 21 +++++++++++++++++---- spp_grm_cel/static/description/index.html | 21 +++++++++++++++++---- 4 files changed, 42 insertions(+), 10 deletions(-) diff --git a/spp_grm/README.rst b/spp_grm/README.rst index 0f0e0f737..59cc9f9f9 100644 --- a/spp_grm/README.rst +++ b/spp_grm/README.rst @@ -169,7 +169,10 @@ Changelog no longer runs inside the stored ``sla_status`` compute. It is deferred to the end of the triggering transaction, so the escalation engine's writes, savepoints and flushes never execute mid-computation. - Same transaction, same outcome. + Same transaction, same outcome. An unsaved form edit queues nothing: + the compute also runs on the pseudo-record of an onchange, whose ids + resolve back to the real ticket, which would have escalated it for a + change the user never saved. 19.0.2.0.1 ~~~~~~~~~~ diff --git a/spp_grm/static/description/index.html b/spp_grm/static/description/index.html index 8e1923c9c..d49a10b09 100644 --- a/spp_grm/static/description/index.html +++ b/spp_grm/static/description/index.html @@ -551,7 +551,10 @@

    19.0.2.0.2

    no longer runs inside the stored sla_status compute. It is deferred to the end of the triggering transaction, so the escalation engine’s writes, savepoints and flushes never execute mid-computation. -Same transaction, same outcome. +Same transaction, same outcome. An unsaved form edit queues nothing: +the compute also runs on the pseudo-record of an onchange, whose ids +resolve back to the real ticket, which would have escalated it for a +change the user never saved.
    diff --git a/spp_grm_cel/README.rst b/spp_grm_cel/README.rst index bca185b1c..32bb7c718 100644 --- a/spp_grm_cel/README.rst +++ b/spp_grm_cel/README.rst @@ -151,16 +151,29 @@ Changelog user needs read access on the rules; the rows only exposed the routing/escalation map (conditions, targets, thresholds) to enumeration (hardening alongside #379/#381). The ticket form's "Check - Escalation" button is limited to GRM officers and above. + Escalation" button is limited to GRM officers and above, enforced on + the method itself — a view ``groups=`` does not bind an RPC call — by + requiring write access on the ticket, which also keeps an officer to + their own ticket scope. - fix: an escalation is now applied atomically (savepoint). The ticket write, the chatter post and the counter succeed or roll back together, and if the rule owner is denied any effect the rule produces — posting to a ticket just reassigned out of their own scope, sending the configured template, or creating the configured case — the whole escalation rolls back and is skipped instead of persisting - half-applied. Delivery or data errors in the notification and case - steps remain best effort: logged, skipped, and isolated so they cannot - abort the pass. + half-applied. The notification is sent last, after every effect that + can still be denied, because delivery is the one step a rollback + cannot take back — and a ghost mail would repeat, the rolled-back rule + link no longer suppressing the rule on the next pass. Delivery or data + errors in the notification and case steps remain best effort: logged, + skipped, and isolated so they cannot abort the pass. +- fix: applying a routing rule is atomic in the same way. The ticket + write and the match counter succeed or roll back together, and a + database error while routing (a rule pointing at a since-deleted user) + no longer leaves the transaction aborted: the ticket create that + triggered the routing swallows the error, so every later statement of + the same request — the rest of a portal submission — used to fail + behind it. - fix: a rule applies at most once per ticket. Previously the hourly cron re-escalated every still-open matching ticket on every pass, repeating the counter increment, the chatter post and the notification diff --git a/spp_grm_cel/static/description/index.html b/spp_grm_cel/static/description/index.html index 86f593c9d..797668540 100644 --- a/spp_grm_cel/static/description/index.html +++ b/spp_grm_cel/static/description/index.html @@ -537,16 +537,29 @@

    19.0.2.0.2

    user needs read access on the rules; the rows only exposed the routing/escalation map (conditions, targets, thresholds) to enumeration (hardening alongside #379/#381). The ticket form’s “Check -Escalation” button is limited to GRM officers and above. +Escalation” button is limited to GRM officers and above, enforced on +the method itself — a view groups= does not bind an RPC call — by +requiring write access on the ticket, which also keeps an officer to +their own ticket scope.
  • fix: an escalation is now applied atomically (savepoint). The ticket write, the chatter post and the counter succeed or roll back together, and if the rule owner is denied any effect the rule produces — posting to a ticket just reassigned out of their own scope, sending the configured template, or creating the configured case — the whole escalation rolls back and is skipped instead of persisting -half-applied. Delivery or data errors in the notification and case -steps remain best effort: logged, skipped, and isolated so they cannot -abort the pass.
    • +half-applied. The notification is sent last, after every effect that +can still be denied, because delivery is the one step a rollback +cannot take back — and a ghost mail would repeat, the rolled-back rule +link no longer suppressing the rule on the next pass. Delivery or data +errors in the notification and case steps remain best effort: logged, +skipped, and isolated so they cannot abort the pass. +
  • fix: applying a routing rule is atomic in the same way. The ticket +write and the match counter succeed or roll back together, and a +database error while routing (a rule pointing at a since-deleted user) +no longer leaves the transaction aborted: the ticket create that +triggered the routing swallows the error, so every later statement of +the same request — the rest of a portal submission — used to fail +behind it.
  • fix: a rule applies at most once per ticket. Previously the hourly cron re-escalated every still-open matching ticket on every pass, repeating the counter increment, the chatter post and the notification From 6b44f4c7bfcec7b8ed9eb6824c37130118b96c7d Mon Sep 17 00:00:00 2001 From: Ken Lewerentz Date: Wed, 2 Sep 2026 13:39:40 +0700 Subject: [PATCH 12/15] fix(grm_cel): fail loudly on forged rule ownership, hoist rule lookup Two more fixes from review of #415: - write() dropped a third-party eval_as_user_id silently and returned True, so a data fix doing rules.write({"eval_as_user_id": owner.id}) reported success while the rules kept evaluating as their old owner. Raise UserError pointing at Take Ownership instead. The 19.0.2.0.2 migration backfills with raw SQL, so it is unaffected. - Routing resolved the active rule set and each rule's evaluation owner inside the per-ticket loop, so a superuser-owned or archived-owner rule logged its warning on every single ticket created. Resolve it once for the created batch, as the escalation cron already does. --- spp_grm_cel/models/grm_escalation_rule.py | 20 +++++++++--- spp_grm_cel/models/grm_routing_rule.py | 31 +++++++++++++++---- spp_grm_cel/models/grm_ticket.py | 20 ++++++++++-- spp_grm_cel/readme/HISTORY.md | 12 ++++--- spp_grm_cel/tests/test_routing_rules.py | 24 ++++++++++++++ spp_grm_cel/tests/test_rule_owner_identity.py | 17 ++++++---- 6 files changed, 101 insertions(+), 23 deletions(-) diff --git a/spp_grm_cel/models/grm_escalation_rule.py b/spp_grm_cel/models/grm_escalation_rule.py index 3a1639b4a..baf78d4e1 100644 --- a/spp_grm_cel/models/grm_escalation_rule.py +++ b/spp_grm_cel/models/grm_escalation_rule.py @@ -1,7 +1,7 @@ import logging from odoo import SUPERUSER_ID, Command, _, api, fields, models -from odoo.exceptions import AccessError, ValidationError +from odoo.exceptions import AccessError, UserError, ValidationError _logger = logging.getLogger(__name__) @@ -195,11 +195,23 @@ def write(self, vals): the same re-bind the targeting-field path performs. self.env.uid is the acting user, preserved even under sudo() (only an explicit with_user() write re-widens). + + Any other explicit value raises rather than being dropped: a silent + no-op returning True let a data fix or migration script report success + while the rules kept evaluating as their old owner. UserError, not + AccessError — the write itself is allowed, the field is simply not the + caller's to set. """ - if any(f in vals for f in self._EVAL_TARGETING_FIELDS) or vals.get("eval_as_user_id") == self.env.uid: + if "eval_as_user_id" in vals and vals["eval_as_user_id"] != self.env.uid: + raise UserError( + _( + "A rule's evaluation identity is managed by the system and cannot be " + 'assigned to another user. Open the rule and use "Take Ownership" to ' + "bind it to yourself." + ) + ) + if any(f in vals for f in self._EVAL_TARGETING_FIELDS) or "eval_as_user_id" in vals: vals = dict(vals, eval_as_user_id=self.env.uid) - elif "eval_as_user_id" in vals: - vals = {k: v for k, v in vals.items() if k != "eval_as_user_id"} return super().write(vals) def action_take_ownership(self): diff --git a/spp_grm_cel/models/grm_routing_rule.py b/spp_grm_cel/models/grm_routing_rule.py index a3f2860f8..e8a7cdd01 100644 --- a/spp_grm_cel/models/grm_routing_rule.py +++ b/spp_grm_cel/models/grm_routing_rule.py @@ -1,7 +1,7 @@ import logging from odoo import SUPERUSER_ID, _, api, fields, models -from odoo.exceptions import AccessError, ValidationError +from odoo.exceptions import AccessError, UserError, ValidationError _logger = logging.getLogger(__name__) @@ -156,11 +156,23 @@ def write(self, vals): the same re-bind the targeting-field path performs. self.env.uid is the acting user, preserved even under sudo() (only an explicit with_user() write re-widens). + + Any other explicit value raises rather than being dropped: a silent + no-op returning True let a data fix or migration script report success + while the rules kept evaluating as their old owner. UserError, not + AccessError — the write itself is allowed, the field is simply not the + caller's to set. """ - if any(f in vals for f in self._EVAL_TARGETING_FIELDS) or vals.get("eval_as_user_id") == self.env.uid: + if "eval_as_user_id" in vals and vals["eval_as_user_id"] != self.env.uid: + raise UserError( + _( + "A rule's evaluation identity is managed by the system and cannot be " + 'assigned to another user. Open the rule and use "Take Ownership" to ' + "bind it to yourself." + ) + ) + if any(f in vals for f in self._EVAL_TARGETING_FIELDS) or "eval_as_user_id" in vals: vals = dict(vals, eval_as_user_id=self.env.uid) - elif "eval_as_user_id" in vals: - vals = {k: v for k, v in vals.items() if k != "eval_as_user_id"} return super().write(vals) def action_take_ownership(self): @@ -342,7 +354,7 @@ def _evaluate_expression(self, expression, context): raise @api.private - def apply_routing(self, ticket): + def apply_routing(self, ticket, rules=None): """Apply the first matching routing rule to a ticket. Each rule is evaluated and applied with the identity of whoever defined @@ -353,11 +365,18 @@ def apply_routing(self, ticket): Args: ticket: spp.grm.ticket record + rules: optional pre-resolved ``[(rule, owner), ...]`` from + ``_active_rules_with_owners``; a caller routing a batch of + tickets (``create``) resolves them once instead of once per + ticket, so a misconfigured rule is warned about once per batch Returns: bool: True if a rule was applied, False otherwise """ - for rule, owner in self._active_rules_with_owners(): + if rules is None: + rules = self._active_rules_with_owners() + + for rule, owner in rules: # nosemgrep: semgrep.odoo-with-user-unvalidated -- owner is system-set in create()/write(), not client input rule_as_owner = rule.with_user(owner.id) # nosemgrep: semgrep.odoo-with-user-unvalidated -- owner is system-set; scopes ticket writes diff --git a/spp_grm_cel/models/grm_ticket.py b/spp_grm_cel/models/grm_ticket.py index 28f157f22..5b945eaa0 100644 --- a/spp_grm_cel/models/grm_ticket.py +++ b/spp_grm_cel/models/grm_ticket.py @@ -32,9 +32,21 @@ def create(self, vals_list): """Override create to apply routing rules to new tickets.""" tickets = super().create(vals_list) + # Hoisted out of the per-ticket loop: the active rule set (and each + # rule's evaluation owner, with its warnings) is identical for every + # ticket of this batch, so resolve it once — a misconfigured rule is + # then warned about once per create, not once per ticket created. + try: + rules = self.env["spp.grm.routing.rule"]._active_rules_with_owners() + except Exception: + # Routing must never cost a ticket its creation; this is the same + # guarantee _apply_routing_rules gives for a single ticket. + _logger.exception("Could not resolve routing rules; tickets %s were not routed.", tickets.ids) + return tickets + # Apply routing rules to each new ticket for ticket in tickets: - self._apply_routing_rules(ticket) + self._apply_routing_rules(ticket, rules=rules) return tickets @@ -50,11 +62,13 @@ def write(self, vals): return result @api.model - def _apply_routing_rules(self, ticket): + def _apply_routing_rules(self, ticket, rules=None): """Apply routing rules to a ticket. Args: ticket: spp.grm.ticket record + rules: optional pre-resolved ``[(rule, owner), ...]`` shared by a + batch (see ``spp.grm.routing.rule.apply_routing``) """ try: _logger.debug("Applying routing rules to ticket %s", ticket.number) @@ -63,7 +77,7 @@ def _apply_routing_rules(self, ticket): routing_model = self.env["spp.grm.routing.rule"] # Apply routing rules - if routing_model.apply_routing(ticket): + if routing_model.apply_routing(ticket, rules=rules): _logger.info("Routing rules applied to ticket %s", ticket.number) else: _logger.debug("No routing rules matched for ticket %s", ticket.number) diff --git a/spp_grm_cel/readme/HISTORY.md b/spp_grm_cel/readme/HISTORY.md index 7bd199ff9..dce22f2fe 100644 --- a/spp_grm_cel/readme/HISTORY.md +++ b/spp_grm_cel/readme/HISTORY.md @@ -10,7 +10,9 @@ owned by the superuser (created from a shell, import script, or data load) still evaluates without record-rule bounds and is called out with a warning by both the migration and the rule engine. The new **Take Ownership** button on the rule form re-binds a rule to yourself; - saving the form without changing what the rule targets does not. + saving the form without changing what the rule targets does not. Writing the identity to any + other user raises: dropping it silently let a data fix or migration script report success + while the rules kept evaluating as their old owner. - fix(security): the rule-engine entry points (``apply_routing``, ``apply_escalations``, ``apply_escalation``, ``check_escalations``) are marked ``@api.private`` — no longer callable over RPC (#381). @@ -49,9 +51,11 @@ - fix: rule CEL validation now reports any parser error as a ``ValidationError`` (previously only ``SyntaxError`` was caught). - fix: the hourly escalation cron resolves the active rule set and each rule's evaluation owner - once per pass instead of once per open ticket (so owner warnings are logged once, not once - per ticket), and the engine logs (instead of silently skipping) rules with no evaluation - identity and tickets skipped for lack of owner access. + once per pass instead of once per open ticket, and ticket creation does the same for the batch + it routes (so owner warnings are logged once per pass, not once per ticket — on a busy portal + a single shell-created rule warned on every submitted grievance), and the engine logs (instead + of silently skipping) rules with no evaluation identity and tickets skipped for lack of owner + access. ### 19.0.2.0.1 diff --git a/spp_grm_cel/tests/test_routing_rules.py b/spp_grm_cel/tests/test_routing_rules.py index 42e29a5d6..0bd7f73b7 100644 --- a/spp_grm_cel/tests/test_routing_rules.py +++ b/spp_grm_cel/tests/test_routing_rules.py @@ -423,3 +423,27 @@ def failing_write(tickets, vals): # if the failed statement had aborted the transaction. self.assertEqual(self.Ticket.search_count([("id", "=", ticket.id)]), 1) self.assertFalse(ticket.routing_rule_id) + + def test_owner_warnings_logged_once_per_create_batch(self): + """A misconfigured rule — here superuser-owned, the shell/data-load + case — is warned about once for the batch that triggered the routing, + not once per ticket in it. The active rule set and each rule's + evaluation owner are identical for every ticket of the batch.""" + self.env[self.RoutingRule._name].create( + {"name": "Shell-created rule", "condition_cel": "severity == 'nonexistent'"} + ) + with self.assertLogs(ROUTING_LOGGER, level="WARNING") as cm: + self.Ticket.create( + [ + { + "name": f"Batched {i}", + "description": "Test", + "severity": "critical", + "stage_id": self.stage.id, + "partner_id": self.partner.id, + } + for i in range(3) + ] + ) + hits = [line for line in cm.output if "Shell-created rule" in line and "owned by the superuser" in line] + self.assertEqual(len(hits), 1, cm.output) diff --git a/spp_grm_cel/tests/test_rule_owner_identity.py b/spp_grm_cel/tests/test_rule_owner_identity.py index 4da98ee41..a68e9c3af 100644 --- a/spp_grm_cel/tests/test_rule_owner_identity.py +++ b/spp_grm_cel/tests/test_rule_owner_identity.py @@ -14,7 +14,7 @@ """ from odoo import Command -from odoo.exceptions import AccessError +from odoo.exceptions import AccessError, UserError from odoo.tests import TransactionCase, tagged ROUTING = "spp.grm.routing.rule" @@ -185,8 +185,10 @@ def test_eval_as_user_id_not_forgeable_via_context(self): def test_eval_as_user_id_not_writable_and_rebinds_on_retarget(self): rule = self.env[ROUTING].with_user(self.manager).create({"name": "R3", "condition_cel": "severity == 'low'"}) - # Direct write of the identity is ignored. - rule.with_user(self.officer).write({"eval_as_user_id": self.env.ref("base.user_admin").id}) + # Direct write of the identity is refused — loudly, so a data fix that + # tries it cannot report success while the rule keeps its old owner. + with self.assertRaises(UserError): + rule.with_user(self.officer).write({"eval_as_user_id": self.env.ref("base.user_admin").id}) rule.invalidate_recordset() self.assertNotEqual(rule.eval_as_user_id, self.env.ref("base.user_admin")) # Changing what the rule targets re-binds identity to the editor. @@ -227,11 +229,14 @@ def test_take_ownership_rebinds_to_acting_user(self): def test_eval_as_user_id_client_write_only_accepted_for_self(self): """A direct write may only set the identity to the acting user; any other - value is stripped (the forgery guard from #379 stands).""" + value is refused (the forgery guard from #379 stands). It raises rather + than being dropped: a silent no-op returning True leaves an operator + believing rules.write({"eval_as_user_id": new_owner.id}) re-homed them.""" rule = self.env[ESCALATION].with_user(self.officer).create({"name": "Self only", "condition_cel": ""}) - rule.with_user(self.manager).write({"eval_as_user_id": self.env.ref("base.user_admin").id}) + with self.assertRaises(UserError): + rule.with_user(self.manager).write({"eval_as_user_id": self.env.ref("base.user_admin").id}) rule.invalidate_recordset() - self.assertEqual(rule.eval_as_user_id, self.officer, "third-party identity must be stripped") + self.assertEqual(rule.eval_as_user_id, self.officer, "third-party identity must not take effect") rule.with_user(self.manager).write({"eval_as_user_id": self.manager.id}) rule.invalidate_recordset() self.assertEqual(rule.eval_as_user_id, self.manager, "self identity is the take-ownership path") From e51dc298fb3d3ed16f7fc48648ee6d7c8f987e4b Mon Sep 17 00:00:00 2001 From: Ken Lewerentz Date: Wed, 2 Sep 2026 13:39:40 +0700 Subject: [PATCH 13/15] docs(grm_cel): regenerate README from the updated changelog fragments --- spp_grm_cel/README.rst | 16 +++++++++++----- spp_grm_cel/static/description/index.html | 16 +++++++++++----- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/spp_grm_cel/README.rst b/spp_grm_cel/README.rst index 32bb7c718..3e992557c 100644 --- a/spp_grm_cel/README.rst +++ b/spp_grm_cel/README.rst @@ -141,7 +141,10 @@ Changelog record-rule bounds and is called out with a warning by both the migration and the rule engine. The new **Take Ownership** button on the rule form re-binds a rule to yourself; saving the form without - changing what the rule targets does not. + changing what the rule targets does not. Writing the identity to any + other user raises: dropping it silently let a data fix or migration + script report success while the rules kept evaluating as their old + owner. - fix(security): the rule-engine entry points (``apply_routing``, ``apply_escalations``, ``apply_escalation``, ``check_escalations``) are marked ``@api.private`` — no longer callable over RPC (#381). @@ -191,10 +194,13 @@ Changelog - fix: rule CEL validation now reports any parser error as a ``ValidationError`` (previously only ``SyntaxError`` was caught). - fix: the hourly escalation cron resolves the active rule set and each - rule's evaluation owner once per pass instead of once per open ticket - (so owner warnings are logged once, not once per ticket), and the - engine logs (instead of silently skipping) rules with no evaluation - identity and tickets skipped for lack of owner access. + rule's evaluation owner once per pass instead of once per open ticket, + and ticket creation does the same for the batch it routes (so owner + warnings are logged once per pass, not once per ticket — on a busy + portal a single shell-created rule warned on every submitted + grievance), and the engine logs (instead of silently skipping) rules + with no evaluation identity and tickets skipped for lack of owner + access. 19.0.2.0.1 ~~~~~~~~~~ diff --git a/spp_grm_cel/static/description/index.html b/spp_grm_cel/static/description/index.html index 797668540..f5ad84c17 100644 --- a/spp_grm_cel/static/description/index.html +++ b/spp_grm_cel/static/description/index.html @@ -527,7 +527,10 @@

    19.0.2.0.2

    record-rule bounds and is called out with a warning by both the migration and the rule engine. The new Take Ownership button on the rule form re-binds a rule to yourself; saving the form without -changing what the rule targets does not.
    • +changing what the rule targets does not. Writing the identity to any +other user raises: dropping it silently let a data fix or migration +script report success while the rules kept evaluating as their old +owner.
  • fix(security): the rule-engine entry points (apply_routing, apply_escalations, apply_escalation, check_escalations) are marked @api.private — no longer callable over RPC (#381).
    • @@ -577,10 +580,13 @@

    19.0.2.0.2

  • fix: rule CEL validation now reports any parser error as a ValidationError (previously only SyntaxError was caught).
  • fix: the hourly escalation cron resolves the active rule set and each -rule’s evaluation owner once per pass instead of once per open ticket -(so owner warnings are logged once, not once per ticket), and the -engine logs (instead of silently skipping) rules with no evaluation -identity and tickets skipped for lack of owner access.
    • +rule’s evaluation owner once per pass instead of once per open ticket, +and ticket creation does the same for the batch it routes (so owner +warnings are logged once per pass, not once per ticket — on a busy +portal a single shell-created rule warned on every submitted +grievance), and the engine logs (instead of silently skipping) rules +with no evaluation identity and tickets skipped for lack of owner +access.
    From 3834ea53bfe60fc7315071c7a639fc6e0523d3d9 Mon Sep 17 00:00:00 2001 From: Ken Lewerentz Date: Wed, 2 Sep 2026 14:05:37 +0700 Subject: [PATCH 14/15] fix(grm_cel): refuse cases with no real worker, keep parser defects legible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Last three fixes from review of #415: - _create_case_from_ticket assigned the ticket assignee "else whoever the rule evaluates as". For an unassigned ticket under a superuser-owned rule that is __system__, so the required case worker was satisfied with OdooBot. Vet the resolved worker and refuse the escalation (rolled back by the caller's savepoint) rather than file a case nobody is answerable for. _compute_user_id falls back to the creating user, so a ticket created from a shell or cron env hits this directly. - The CEL constraint caught bare Exception and reported everything as "Invalid CEL expression", so a defect inside the parser surfaced as the user's own data-entry error with the traceback discarded. Report SyntaxError/RecursionError as before; log anything else with its traceback and say it is internal. - test_sla_breach asserted the breach queue key was gone from cr.precommit.data, which Callbacks.run() clears unconditionally, so the assertion could not fail. The pop is really pinned by the note count (the compute schedules per ticket, so duplicate hooks would reprocess the batch) — say so where it is asserted, and cover the cross-flush case that had no test. --- spp_grm/tests/test_sla_breach.py | 31 +++++++++++-- spp_grm_cel/models/grm_escalation_rule.py | 50 ++++++++++++++++++--- spp_grm_cel/models/grm_routing_rule.py | 23 ++++++++-- spp_grm_cel/readme/HISTORY.md | 12 +++-- spp_grm_cel/tests/test_escalation_engine.py | 24 ++++++++++ spp_grm_cel/tests/test_escalation_rules.py | 22 +++++++++ spp_grm_cel/tests/test_routing_rules.py | 20 +++++++++ 7 files changed, 166 insertions(+), 16 deletions(-) diff --git a/spp_grm/tests/test_sla_breach.py b/spp_grm/tests/test_sla_breach.py index e66421a61..9591214fd 100644 --- a/spp_grm/tests/test_sla_breach.py +++ b/spp_grm/tests/test_sla_breach.py @@ -57,12 +57,35 @@ def test_form_onchange_does_not_queue_unsaved_records(self): self.assertEqual(self.env.cr.precommit.data.get("spp_grm.sla_breach_ids", set()), set()) def test_breach_hook_runs_once_per_ticket(self): - """Several schedulings within one transaction collapse into one run.""" + """Several schedulings within one transaction collapse into one run. + + The compute schedules per ticket, so the hook is queued once per + breached ticket. The first call drains the queue and the duplicates + find nothing left; counting the notes is what pins that. Asserting the + key is gone from ``cr.precommit.data`` would not — ``Callbacks.run()`` + clears its data unconditionally once the callbacks have run, drained or + not. + """ self.env.flush_all() before = self._breach_notes() self.tickets[0].write({"category_id": self.past.id}) self.tickets[1].write({"category_id": self.past.id}) self.env.cr.flush() - self.assertEqual(self._breach_notes() - before, 2) - # Nothing left queued. - self.assertNotIn("spp_grm.sla_breach_ids", self.env.cr.precommit.data) + self.assertEqual( + self._breach_notes() - before, + 2, + "the queue was not drained: a duplicate hook re-posted for tickets already handled", + ) + + def test_a_later_breach_only_handles_the_new_ticket(self): + """A breach later in the same transaction posts for the new ticket + only: the tickets handled by the earlier flush are not revisited.""" + self.env.flush_all() + before = self._breach_notes() + self.tickets[0].write({"category_id": self.past.id}) + self.env.cr.flush() + self.assertEqual(self._breach_notes() - before, 1) + + self.tickets[1].write({"category_id": self.past.id}) + self.env.cr.flush() + self.assertEqual(self._breach_notes() - before, 2, "the first ticket's breach note was posted twice") diff --git a/spp_grm_cel/models/grm_escalation_rule.py b/spp_grm_cel/models/grm_escalation_rule.py index baf78d4e1..4cc1ba5e9 100644 --- a/spp_grm_cel/models/grm_escalation_rule.py +++ b/spp_grm_cel/models/grm_escalation_rule.py @@ -236,9 +236,9 @@ def _check_condition_cel(self): # Use proper CEL parser for validation P.parse(rule.condition_cel) # If parser not available, skip validation - except Exception as e: - # Any parser failure (not only SyntaxError) is a bad - # expression the user must fix, surfaced as a ValidationError. + except (SyntaxError, RecursionError) as e: + # What the parser raises for an expression the user must + # fix (bad syntax, or nesting past its depth limit). raise ValidationError( _( "Invalid CEL expression in rule '%(rule_name)s': %(error)s", @@ -246,6 +246,23 @@ def _check_condition_cel(self): error=str(e), ) ) from e + except Exception as e: + # Anything else is a defect in the parser, not bad input: + # keep the traceback in the log instead of discarding it + # and blaming the user's expression. + _logger.exception( + "Validating the CEL expression of rule '%s' failed unexpectedly", + rule.name, + ) + raise ValidationError( + _( + "Could not validate the CEL expression in rule '%(rule_name)s': " + "%(error)s. This is an internal error, not a problem with the " + "expression; see the server log.", + rule_name=rule.name, + error=str(e), + ) + ) from e @api.constrains("trigger_after_hours") def _check_trigger_after_hours(self): @@ -527,6 +544,28 @@ def _create_case_from_ticket(self, ticket): ) return + # spp.case requires a case worker, and it has to be a real person who + # is answerable for the case. The candidates are the ticket assignee + # and, failing that, whoever the rule evaluates as — but a superuser + # owner (a rule created from a shell, import or data load) resolves to + # __system__, an inactive non-human account, and an archived assignee + # is no better. Raised before the savepoint below so the caller rolls + # the escalation back rather than reporting it applied with the case + # filed under OdooBot. + case_worker = ticket.user_id or self.env.user + if case_worker.id == SUPERUSER_ID or not case_worker.active: + raise UserError( + _( + "Escalation rule '%(rule_name)s' creates a case but has no one to assign " + "it to: ticket %(ticket_number)s resolves to %(worker)s, which is not a " + 'real active user. Assign the ticket, or use "Take Ownership" on the rule ' + "as the user who should own it.", + rule_name=self.name, + ticket_number=ticket.number, + worker=case_worker.display_name, + ) + ) + try: with self.env.cr.savepoint(): case = self.env["spp.case"].create( @@ -535,9 +574,8 @@ def _create_case_from_ticket(self, ticket): "case_type_id": self.case_type_id.id, "partner_id": ticket.partner_id.id, "presenting_issue": ticket.description, - # Required on spp.case: the ticket assignee, else whoever - # the rule evaluates as (an internal user). - "case_worker_id": (ticket.user_id or self.env.user).id, + # Required on spp.case; resolved and vetted above. + "case_worker_id": case_worker.id, } ) diff --git a/spp_grm_cel/models/grm_routing_rule.py b/spp_grm_cel/models/grm_routing_rule.py index e8a7cdd01..0f081db62 100644 --- a/spp_grm_cel/models/grm_routing_rule.py +++ b/spp_grm_cel/models/grm_routing_rule.py @@ -248,9 +248,9 @@ def _check_condition_cel(self): # Use proper CEL parser for validation P.parse(rule.condition_cel) # If parser not available, skip validation - except Exception as e: - # Any parser failure (not only SyntaxError) is a bad - # expression the user must fix, surfaced as a ValidationError. + except (SyntaxError, RecursionError) as e: + # What the parser raises for an expression the user must + # fix (bad syntax, or nesting past its depth limit). raise ValidationError( _( "Invalid CEL expression in rule '%(rule_name)s': %(error)s", @@ -258,6 +258,23 @@ def _check_condition_cel(self): error=str(e), ) ) from e + except Exception as e: + # Anything else is a defect in the parser, not bad input: + # keep the traceback in the log instead of discarding it + # and blaming the user's expression. + _logger.exception( + "Validating the CEL expression of rule '%s' failed unexpectedly", + rule.name, + ) + raise ValidationError( + _( + "Could not validate the CEL expression in rule '%(rule_name)s': " + "%(error)s. This is an internal error, not a problem with the " + "expression; see the server log.", + rule_name=rule.name, + error=str(e), + ) + ) from e def evaluate(self, ticket): """Evaluate if this rule applies to the given ticket. diff --git a/spp_grm_cel/readme/HISTORY.md b/spp_grm_cel/readme/HISTORY.md index dce22f2fe..54b6f3d92 100644 --- a/spp_grm_cel/readme/HISTORY.md +++ b/spp_grm_cel/readme/HISTORY.md @@ -44,12 +44,18 @@ the remaining tickets are processed. - fix: case creation from an escalation rule never worked (it passed a field ``spp.case`` does not have and omitted the required case worker); it now fills ``presenting_issue`` and assigns - the ticket assignee or the rule owner as case worker. + the ticket assignee or the rule owner as case worker. When neither is a real active user — + an unassigned ticket under a superuser-owned rule resolves to ``__system__`` — the escalation + is refused and rolled back rather than filing a case with OdooBot as the worker responsible; + the message names the ticket and the remediation. - fix: increment ``match_count`` / ``escalation_count`` with an atomic ``UPDATE`` instead of a read-modify-write, avoiding a serialization failure under concurrent cron/UI escalation whose dispatch-level retry would re-run the whole cron pass. -- fix: rule CEL validation now reports any parser error as a ``ValidationError`` (previously only - ``SyntaxError`` was caught). +- fix: rule CEL validation reports a bad expression as a ``ValidationError`` whether the parser + raises ``SyntaxError`` or ``RecursionError`` (nesting past its depth limit); previously only + ``SyntaxError`` was caught. An unexpected parser failure is no longer reported as the user's + own invalid expression with the traceback discarded: it is logged with its traceback and + surfaced as an internal error. - fix: the hourly escalation cron resolves the active rule set and each rule's evaluation owner once per pass instead of once per open ticket, and ticket creation does the same for the batch it routes (so owner warnings are logged once per pass, not once per ticket — on a busy portal diff --git a/spp_grm_cel/tests/test_escalation_engine.py b/spp_grm_cel/tests/test_escalation_engine.py index 430f87fa0..d8ce7849d 100644 --- a/spp_grm_cel/tests/test_escalation_engine.py +++ b/spp_grm_cel/tests/test_escalation_engine.py @@ -198,6 +198,30 @@ def _notification_template(self): } ) + @mute_logger(ESC_LOGGER) + def test_case_creation_without_a_real_case_worker_rolls_back(self): + """spp.case requires a case worker, and an unassigned ticket under a + superuser-owned rule (the shell/data-load case) resolves it to + __system__ — an inactive non-human account. Refuse rather than file a + case OdooBot owns and report the escalation as applied.""" + case_type = self.env["spp.case.type"].create({"name": "Escalation", "code": "ESC"}) + rule = self.env[ESCALATION].create( + {"name": "Shell case rule", "condition_cel": "", "create_case": True, "case_type_id": case_type.id} + ) + # _compute_user_id falls back to the creating user, so a ticket created + # from a shell/cron env is "assigned" to the superuser. + ticket = self._ticket("no worker") + self.assertEqual(ticket.user_id, self.env.ref("base.user_root")) + + applied = self.env[ESCALATION].sudo().apply_escalations(ticket) + + self.assertFalse(applied) + ticket.invalidate_recordset() + self.assertFalse(ticket.is_escalated) + rule.invalidate_recordset() + self.assertEqual(rule.escalation_count, 0) + self.assertFalse(self.env["spp.case"].sudo().search([("name", "ilike", ticket.number)])) + def test_notification_sent_under_owner_identity(self): template = self._notification_template() self._officer_rule( diff --git a/spp_grm_cel/tests/test_escalation_rules.py b/spp_grm_cel/tests/test_escalation_rules.py index d662c194f..7ca943a15 100644 --- a/spp_grm_cel/tests/test_escalation_rules.py +++ b/spp_grm_cel/tests/test_escalation_rules.py @@ -398,3 +398,25 @@ def test_check_escalations_cron(self): self.assertFalse(ticket_low.is_escalated) self.assertTrue(ticket_high.is_escalated) self.assertEqual(ticket_high.team_id, self.team2) + + def test_unexpected_parser_failure_is_logged_not_blamed_on_the_expression(self): + """A defect inside the parser (not a bad expression) must keep its + traceback in the log instead of being reported to the user as their own + invalid CEL.""" + from unittest.mock import Mock, patch + + from odoo.exceptions import ValidationError + + from odoo.addons.spp_grm_cel.models import grm_escalation_rule + + parser = Mock() + parser.parse.side_effect = TypeError("parser defect") + with ( + patch.object(grm_escalation_rule, "P", parser), + self.assertLogs("odoo.addons.spp_grm_cel.models.grm_escalation_rule", level="ERROR") as cm, + self.assertRaises(ValidationError) as caught, + ): + self.EscalationRule.create({"name": "Parser Bug", "condition_cel": "severity == 'critical'"}) + + self.assertIn("internal error", str(caught.exception)) + self.assertTrue(any("Traceback" in line for line in cm.output), cm.output) diff --git a/spp_grm_cel/tests/test_routing_rules.py b/spp_grm_cel/tests/test_routing_rules.py index 0bd7f73b7..92235dc49 100644 --- a/spp_grm_cel/tests/test_routing_rules.py +++ b/spp_grm_cel/tests/test_routing_rules.py @@ -447,3 +447,23 @@ def test_owner_warnings_logged_once_per_create_batch(self): ) hits = [line for line in cm.output if "Shell-created rule" in line and "owned by the superuser" in line] self.assertEqual(len(hits), 1, cm.output) + + def test_unexpected_parser_failure_is_logged_not_blamed_on_the_expression(self): + """A defect inside the parser (not a bad expression) must keep its + traceback in the log instead of being reported to the user as their own + invalid CEL.""" + from unittest.mock import Mock + + from odoo.addons.spp_grm_cel.models import grm_routing_rule + + parser = Mock() + parser.parse.side_effect = TypeError("parser defect") + with ( + patch.object(grm_routing_rule, "P", parser), + self.assertLogs(ROUTING_LOGGER, level="ERROR") as cm, + self.assertRaises(ValidationError) as caught, + ): + self.RoutingRule.create({"name": "Parser Bug", "condition_cel": "severity == 'critical'"}) + + self.assertIn("internal error", str(caught.exception)) + self.assertTrue(any("Traceback" in line for line in cm.output), cm.output) From ebee907366b88a758389d211af65a6c64c207954 Mon Sep 17 00:00:00 2001 From: Ken Lewerentz Date: Wed, 2 Sep 2026 14:05:37 +0700 Subject: [PATCH 15/15] docs(grm_cel): regenerate README from the updated changelog fragments --- spp_grm_cel/README.rst | 15 ++++++++++++--- spp_grm_cel/static/description/index.html | 15 ++++++++++++--- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/spp_grm_cel/README.rst b/spp_grm_cel/README.rst index 3e992557c..9e687ec9c 100644 --- a/spp_grm_cel/README.rst +++ b/spp_grm_cel/README.rst @@ -186,13 +186,22 @@ Changelog - fix: case creation from an escalation rule never worked (it passed a field ``spp.case`` does not have and omitted the required case worker); it now fills ``presenting_issue`` and assigns the ticket - assignee or the rule owner as case worker. + assignee or the rule owner as case worker. When neither is a real + active user — an unassigned ticket under a superuser-owned rule + resolves to ``__system__`` — the escalation is refused and rolled back + rather than filing a case with OdooBot as the worker responsible; the + message names the ticket and the remediation. - fix: increment ``match_count`` / ``escalation_count`` with an atomic ``UPDATE`` instead of a read-modify-write, avoiding a serialization failure under concurrent cron/UI escalation whose dispatch-level retry would re-run the whole cron pass. -- fix: rule CEL validation now reports any parser error as a - ``ValidationError`` (previously only ``SyntaxError`` was caught). +- fix: rule CEL validation reports a bad expression as a + ``ValidationError`` whether the parser raises ``SyntaxError`` or + ``RecursionError`` (nesting past its depth limit); previously only + ``SyntaxError`` was caught. An unexpected parser failure is no longer + reported as the user's own invalid expression with the traceback + discarded: it is logged with its traceback and surfaced as an internal + error. - fix: the hourly escalation cron resolves the active rule set and each rule's evaluation owner once per pass instead of once per open ticket, and ticket creation does the same for the batch it routes (so owner diff --git a/spp_grm_cel/static/description/index.html b/spp_grm_cel/static/description/index.html index f5ad84c17..d5ed6d899 100644 --- a/spp_grm_cel/static/description/index.html +++ b/spp_grm_cel/static/description/index.html @@ -572,13 +572,22 @@

    19.0.2.0.2

  • fix: case creation from an escalation rule never worked (it passed a field spp.case does not have and omitted the required case worker); it now fills presenting_issue and assigns the ticket -assignee or the rule owner as case worker.
    • +assignee or the rule owner as case worker. When neither is a real +active user — an unassigned ticket under a superuser-owned rule +resolves to __system__ — the escalation is refused and rolled back +rather than filing a case with OdooBot as the worker responsible; the +message names the ticket and the remediation.
  • fix: increment match_count / escalation_count with an atomic UPDATE instead of a read-modify-write, avoiding a serialization failure under concurrent cron/UI escalation whose dispatch-level retry would re-run the whole cron pass.
    • -
  • fix: rule CEL validation now reports any parser error as a -ValidationError (previously only SyntaxError was caught).
    • +
  • fix: rule CEL validation reports a bad expression as a +ValidationError whether the parser raises SyntaxError or +RecursionError (nesting past its depth limit); previously only +SyntaxError was caught. An unexpected parser failure is no longer +reported as the user’s own invalid expression with the traceback +discarded: it is logged with its traceback and surfaced as an internal +error.
  • fix: the hourly escalation cron resolves the active rule set and each rule’s evaluation owner once per pass instead of once per open ticket, and ticket creation does the same for the batch it routes (so owner