diff --git a/spp_grm/README.rst b/spp_grm/README.rst index d3502d33a..59cc9f9f9 100644 --- a/spp_grm/README.rst +++ b/spp_grm/README.rst @@ -153,6 +153,27 @@ 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). 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. 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/__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/models/grm_ticket.py b/spp_grm/models/grm_ticket.py index d2b2500a5..ff304e8a8 100644 --- a/spp_grm/models/grm_ticket.py +++ b/spp_grm/models/grm_ticket.py @@ -536,10 +536,39 @@ 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). + + 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(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.""" + 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 ffafdb8f6..b36732d13 100644 --- a/spp_grm/readme/HISTORY.md +++ b/spp_grm/readme/HISTORY.md @@ -1,3 +1,19 @@ +### 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). 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. 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 - 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/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/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..34095e0a2 100644 --- a/spp_grm/security/rules.xml +++ b/spp_grm/security/rules.xml @@ -104,4 +104,22 @@ [(1, '=', 1)] + + + + GRM Ticket: Portal Own Tickets Only + + [('partner_id', '=', user.partner_id.id)] + + + + + + diff --git a/spp_grm/static/description/index.html b/spp_grm/static/description/index.html index 0d1621de8..d49a10b09 100644 --- a/spp_grm/static/description/index.html +++ b/spp_grm/static/description/index.html @@ -536,6 +536,28 @@

Changelog

+

19.0.2.0.2

+ +
+

19.0.2.0.1

-
+

19.0.2.0.0

  • Initial migration to OpenSPP2
    • diff --git a/spp_grm/tests/__init__.py b/spp_grm/tests/__init__.py index 376402d2a..c46e49d82 100644 --- a/spp_grm/tests/__init__.py +++ b/spp_grm/tests/__init__.py @@ -2,3 +2,6 @@ from . import test_grm_ticket_stage 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_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, + } + ) 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..9591214fd --- /dev/null +++ b/spp_grm/tests/test_sla_breach.py @@ -0,0 +1,91 @@ +# 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 Form, 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_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. + + 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, + "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/README.rst b/spp_grm_cel/README.rst index 4324e16bf..9e687ec9c 100644 --- a/spp_grm_cel/README.rst +++ b/spp_grm_cel/README.rst @@ -124,6 +124,93 @@ 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. 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. 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). +- 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). 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. 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. +- 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. 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 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 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/__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..62488bb45 --- /dev/null +++ b/spp_grm_cel/migrations/19.0.2.0.2/post-migration.py @@ -0,0 +1,57 @@ +# 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, + ) + + # 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 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 " + "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. 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 c4c1547a1..4cc1ba5e9 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 import SUPERUSER_ID, Command, _, api, fields, models +from odoo.exceptions import AccessError, UserError, ValidationError _logger = logging.getLogger(__name__) @@ -139,6 +139,93 @@ 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 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 + # 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. + + 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 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). + + 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 "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) + 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.""" @@ -149,7 +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 SyntaxError as e: + 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", @@ -157,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): @@ -288,9 +394,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 @@ -323,11 +434,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": [(4, 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", @@ -336,19 +450,12 @@ def apply_escalation(self, ticket): vals, ) - # 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) - - # Update escalation count - # nosemgrep: semgrep.odoo-sudo-without-context -- counter update needs sudo - self.sudo().write({"escalation_count": self.escalation_count + 1}) - - # Post message to chatter + # 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 + # 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", @@ -357,34 +464,75 @@ def apply_escalation(self, ticket): subject=_("Ticket Escalated"), ) + # Create case if configured + if self.create_case and self.case_type_id: + self._create_case_from_ticket(ticket) + + # 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", + (self.id,), + ) + 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): """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 """ @@ -396,46 +544,73 @@ 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, - ) - - # Post message to ticket - ticket.message_post( - body=_("Case created: %(case_name)s") - % {"case_id": case.id, "case_name": case.name}, - subject=_("Case Created"), + # 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( + { + "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; resolved and vetted above. + "case_worker_id": case_worker.id, + } + ) + + # 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 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( @@ -446,32 +621,150 @@ 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 (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: - if self.apply_escalations(ticket): + if self.apply_escalations(ticket, rules=rules): escalated_count += 1 _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 apply_escalations(self, ticket): + 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 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())] + + @api.model + @api.private + 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 + 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. A rule applies at most once per ticket. Private: not + RPC-dispatchable. + Args: ticket: spp.grm.ticket record + 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 """ - # Search for active rules in sequence order - rules = self.search([("active", "=", True)], order="sequence, id") + if rules is None: + rules = self._active_rules_with_owners() + already_applied = set(ticket.escalation_rule_ids.ids) applied = False - for rule in rules: - if rule.evaluate(ticket): - rule.apply_escalation(ticket) - applied = True - # Continue checking other rules (unlike routing, multiple escalations can apply) + 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 + # 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. + _logger.debug( + "Escalation rule %s: owner %s cannot read ticket %s; rule does not apply.", + rule.name, + owner.login, + ticket.id, + ) + continue + 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 5d13a3ba5..0f081db62 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 import SUPERUSER_ID, _, api, fields, models +from odoo.exceptions import AccessError, UserError, ValidationError _logger = logging.getLogger(__name__) @@ -105,6 +105,139 @@ 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 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 + # 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 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). + + 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 "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) + 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 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())] + @api.constrains("condition_cel") def _check_condition_cel(self): """Validate CEL expression syntax using CEL parser.""" @@ -115,7 +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 SyntaxError as e: + 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", @@ -123,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. @@ -218,22 +370,49 @@ def _evaluate_expression(self, expression, context): _logger.warning("Expression evaluation error: %s", str(e)) raise - @api.model - def apply_routing(self, ticket): + @api.private + 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 + 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 + 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 """ - # Search for active rules in sequence order - rules = self.search([("active", "=", True)], order="sequence, id") - - for rule in rules: - if rule.evaluate(ticket): - # Apply the rule's actions + 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 + 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. + _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 + # bounded by the owner's record rules. vals = { "routing_rule_id": rule.id, # Track which rule was applied } @@ -250,7 +429,49 @@ def apply_routing(self, ticket): if rule.set_priority: vals["priority"] = rule.set_priority - ticket.write(vals) + try: + # 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. + _logger.info( + "Routing rule %s: owner %s lacks write access on ticket %s; skipped.", + rule.name, + owner.login, + 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, @@ -258,10 +479,6 @@ 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}) - # 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..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) @@ -105,8 +119,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 29b4e01b1..54b6f3d92 100644 --- a/spp_grm_cel/readme/HISTORY.md +++ b/spp_grm_cel/readme/HISTORY.md @@ -1,3 +1,68 @@ +### 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. 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. 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). +- 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). 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. 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. +- 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. 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 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 + 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 - 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/static/description/index.html b/spp_grm_cel/static/description/index.html index d14b31b8a..d5ed6d899 100644 --- a/spp_grm_cel/static/description/index.html +++ b/spp_grm_cel/static/description/index.html @@ -511,6 +511,94 @@

    Changelog

+

19.0.2.0.2

+ +
+

19.0.2.0.1

-
+

19.0.2.0.0

  • Initial migration to OpenSPP2
    • diff --git a/spp_grm_cel/tests/__init__.py b/spp_grm_cel/tests/__init__.py index bb606d642..a9a89daec 100644 --- a/spp_grm_cel/tests/__init__.py +++ b/spp_grm_cel/tests/__init__.py @@ -3,3 +3,5 @@ from . import test_routing_rules 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..d8ce7849d --- /dev/null +++ b/spp_grm_cel/tests/test_escalation_engine.py @@ -0,0 +1,344 @@ +# 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 AccessError, 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 _notification_template(self): + return self.env["mail.template"].create( + { + "name": "Escalation template", + "model_id": self.env["ir.model"]._get("spp.grm.ticket").id, + "subject": "Escalated {{ object.number }}", + "body_html": "

    Escalated

    ", + "email_to": "escalations@example.com", + } + ) + + @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( + 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) + + @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) + 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_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.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_escalation_rules.py b/spp_grm_cel/tests/test_escalation_rules.py index 153639b0d..7ca943a15 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"] @@ -382,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 83075e93a..92235dc49 100644 --- a/spp_grm_cel/tests/test_routing_rules.py +++ b/spp_grm_cel/tests/test_routing_rules.py @@ -1,7 +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): @@ -10,7 +16,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"] @@ -355,3 +377,93 @@ 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) + + 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) + + 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) diff --git a/spp_grm_cel/tests/test_rule_acl.py b/spp_grm_cel/tests/test_rule_acl.py index 44cfb6025..560a479d4 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 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,35 +78,32 @@ 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_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 (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_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( + 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": "Counter Test Ticket", - "description": "Escalation counter regression", - "partner_id": self.portal_user.partner_id.id, - "severity": "critical", + "name": "Plain Internal User", + "login": "grm_internal_acl_test", + "group_ids": [Command.link(self.env.ref("base.group_user").id)], } ) - 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) + 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.""" 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..a68e9c3af --- /dev/null +++ b/spp_grm_cel/tests/test_rule_owner_identity.py @@ -0,0 +1,319 @@ +# 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 / apply_escalation / + check_escalations were public methods, RPC-dispatchable; they must be private. +""" + +from odoo import Command +from odoo.exceptions import AccessError, UserError +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_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.""" + 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 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. + 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_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 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": ""}) + 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 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") + + 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 — + 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 + 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.""" + 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): + 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..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
    +
    +
    + @@ -229,6 +240,7 @@ type="object" class="oe_stat_button" icon="fa-level-up" + groups="spp_grm.group_grm_officer" >
    Check diff --git a/spp_grm_cel/views/grm_routing_rule_views.xml b/spp_grm_cel/views/grm_routing_rule_views.xml index d8627817f..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 +
    +
    +