security(grm): evaluate rules as their owner, scope portal tickets, guard entry points (#379, #380, #381) - #415
Conversation
…ts (#379, #381) - eval_as_user_id (system-managed, no default) on both rule models; forced to the creator in create() and re-bound to the editor when targeting changes in write(), so it cannot be forged via context or direct write. Re-bind excludes operational toggles (sequence, active) so a manager archiving/reordering an officer's rule cannot silently transfer ownership to the manager's scope. - apply_routing/apply_escalations evaluate and apply each rule with_user(owner): an officer's always-match rule can no longer ride the superuser cron to seize tickets outside the officer's record-rule scope (#379). Owner-unreadable/unwritable tickets are skipped, not applied elevated. - @api.private on apply_routing, apply_escalations, apply_escalation, check_escalations: no longer RPC-dispatchable (#381). Cron/SLA callers are in-process and unaffected. - Drop the portal and internal-user read rows on both rule models — owner identity removes the need, closing the enumeration surface (#380). - Atomic UPDATE for match_count/escalation_count (no sudo, no lost updates). - CEL validation reports any parser error as ValidationError. - Migration backfills eval_as_user_id from create_uid.
spp.grm.ticket granted base.group_portal read/write/create with no ir.rule targeting portal, so any portal user could read and rewrite every grievance in the system over RPC (the controller's partner scoping is presentation-only). - New portal record rule: partner_id == user.partner_id (own tickets only). - Portal ACL row reduced to read-only (1,0,0,0); submission is handled by the sudo'd portal controller, which needs no direct model write/create. - New tests/test_portal_ticket_acl.py: portal cannot read/search/write/create others' tickets; can read own.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 19.0 #415 +/- ##
==========================================
+ Coverage 72.24% 73.19% +0.94%
==========================================
Files 419 453 +34
Lines 29813 31329 +1516
==========================================
+ Hits 21539 22930 +1391
- Misses 8274 8399 +125
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…#415) README.rst / index.html for spp_grm and spp_grm_cel applied verbatim from the pre-commit CI run's printed diff (local RST regen is not byte-reproducible). test_rule_owner_identity.py reformatted per ruff-format.
| if not owner: | ||
| continue | ||
| # nosemgrep: semgrep.odoo-with-user-unvalidated -- owner is system-set in create()/write(), not client input | ||
| rule_as_owner = rule.with_user(owner.id) |
There was a problem hiding this comment.
with_user(SUPERUSER_ID) re-opens the exact bypass this PR closes.
Odoo 19 Environment.__new__ (odoo/orm/environments.py:66) forces su = True whenever uid == SUPERUSER_ID, and with_user's own docstring says so: "in non-superuser mode, unless user is the superuser (by convention, the superuser is always in superuser mode)".
So for any rule whose eval_as_user_id (or create_uid fallback) is uid 1, rule.with_user(owner.id) / ticket.with_user(owner.id) produce a su=True environment: ACLs and record rules are fully bypassed and the rule applies to every ticket in the DB — the pre-fix #379 behaviour, silently.
That is not a hypothetical: create() stores self.env.uid, which is SUPERUSER_ID for anything created during module data load, from odoo shell, from an import/upgrade script, or from a TransactionCase — and the new migration mints exactly these owners by copying create_uid. It is also why the pre-existing test_routing_rules.py / test_escalation_rules.py still pass unchanged: their rules are admin-created, so owner-identity never actually constrains them.
At minimum, refuse (or loudly log) a uid‑1 owner rather than letting it silently mean "unrestricted":
owner = rule.eval_as_user_id or rule.create_uid
if not owner or owner.id == SUPERUSER_ID:
_logger.warning("Rule %s has no bounded owner identity; skipping", rule.name)
continueSame applies at grm_routing_rule.py:303.
There was a problem hiding this comment.
Verified against core — the mechanism is exactly as you say (environments.py:66 forces su=True for uid 1, and with_user can't override it), and the migration can indeed carry uid-1 owners forward from shell/script-created rules. Two corrections to the narrative, though: no XML/demo data anywhere creates these rules, and an RPC caller always mints their own uid, so the #379 escalation path itself stays closed — the exposure is confined to rules authored from already-privileged contexts. Also a nuance in the test claim: base.user_admin is uid 2 (record-rule-bounded, just broad), but the legacy suites use the bare test env, which IS uid 1 — so they were exercising the bypass path outright. Addressed as follows rather than skipping (a silent skip would make legitimately system-provisioned rules stop firing on upgrade with no signal): both apply loops now log a warning when a rule's owner is the superuser, the migration calls out uid-1 rows by name, and both legacy suites now author their rules as a real GRM manager so the whole suite runs through the scoped path.
| @@ -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 | |||
There was a problem hiding this comment.
The same enumeration hole #380 fixes for portal is still wide open for every internal user.
base.group_user keeps unscoped read on spp.grm.ticket, and security/rules.xml has no ir.rule targeting base.group_user. Because Odoo ORs the record rules of the groups a user belongs to, a plain internal user (Registry Viewer, Farm User, Program Viewer — none of them in a group_grm_* group) matches no rule on this model and therefore reads every grievance in the database: complainant identity, description, contact. Only users who do hold a GRM group get scoped down by rule_spp_grm_ticket_viewer / _officer.
This PR drops the base.group_user read rows on both rule models for precisely this reason ("only exposed the routing/escalation map to enumeration"), so leaving the far more sensitive ticket model unscoped is inconsistent. Either add a base.group_user record rule or drop this ACL row.
There was a problem hiding this comment.
Confirmed, including the core semantics (no matching group rule and no global rule ⇒ unrestricted), and agreed it's the same class of hole as #380 — thank you. Two wrinkles argue for doing it as an immediate follow-up rather than in this PR: dropping the ACL row would break res_partner._compute_grm_ticket_count (an unsudo'd search that runs for every internal user opening a partner form), so the right shape is an added base.group_user record rule, not a removal; and scoping every internal user's GRM visibility is a behavior change for non-GRM staff that deserves its own release-note headline and its own review rather than riding along unannounced here. Filed as #486 with the proposed rule domain; happy to have it land right behind this PR.
| <field name="domain_force">[('partner_id', '=', user.partner_id.id)]</field> | ||
| <field name="groups" eval="[Command.link(ref('base.group_portal'))]" /> | ||
| <field name="perm_read" eval="True" /> | ||
| <field name="perm_write" eval="False" /> |
There was a problem hiding this comment.
perm_write/perm_create/perm_unlink = False removes the defence-in-depth the comment claims to add.
An ir.rule with only perm_read constrains reads only. The comment above says "the perms here mirror the ACL as defense in depth" — but mirroring is the opposite of defence in depth here: the ACL row becomes the single thing preventing portal writes. The moment any module (or a future edit to this very CSV) re-grants base.group_portal write on spp.grm.ticket, portal users can write every ticket again, because this rule will not apply to the write.
Odoo's own portal rules (helpdesk, project, sale) set all four perms true for this reason. Suggest:
| <field name="perm_write" eval="False" /> | |
| <field name="perm_write" eval="True" /> | |
| <field name="perm_create" eval="True" /> | |
| <field name="perm_unlink" eval="True" /> |
(costs nothing today — portal has no write/create/unlink ACL — and keeps the scoping if that ever changes)
There was a problem hiding this comment.
Taken — all four perms are now enabled, and the "mirror the ACL" comment is rewritten to say what the flags actually do. One correction for the record: core does not consistently set all four on portal rules — project_task_rule_portal (project_security.xml) is read-only exactly like this rule was, while sale mixes forms. The change is still free defense-in-depth, which is why we took it. Separate product question your comment surfaced: our domain is partner_id == user.partner_id, where core portal patterns often use child_of commercial_partner_id — whether a household member should see the household's grievance is tracked in #487.
| @@ -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 | |||
There was a problem hiding this comment.
Officers can still steer manager-owned rules — the active/sequence exclusion is one-sided.
spp_grm_cel/security/ ships no rules.xml, so there is no ir.rule on either rule model: this officer row (1,1,1,0) lets any GRM officer write any rule, including ones owned by a manager.
The PR deliberately excludes active and sequence from _EVAL_TARGETING_FIELDS so that a manager tidying an officer's rule doesn't inherit ownership. The reverse case is not handled: an officer can un-archive a dormant manager-owned rule, or reorder manager-owned routing rules so a different one wins — and those rules then execute with the manager's org-wide ticket scope, with no ownership re-bind and no audit signal. That is the confused-deputy shape the exclusion was meant to prevent, just pointed the other way.
The deeper fix is a record rule on both models scoping non-managers to rules they own, e.g. [('eval_as_user_id', '=', user.id)] for group_grm_officer (write/unlink), leaving managers global. That makes the active/sequence exclusion safe in both directions instead of only one.
There was a problem hiding this comment.
Confirmed, and it's arguably worse than stated: the disable direction (an officer archiving the manager's SLA-breach escalation rule — org-wide escalations silently off, no ownership change, no trace) is the likelier real harm, and the rule-config menus are already manager-only, so the officer rows are RPC-only reach. The fix needs care though: group_grm_manager implies group_grm_officer, so an officer-scoped write rule alone would also cage managers — it takes BOTH a manager [(1,'=',1)] rule and an officer [('eval_as_user_id','=',user.id)] write/unlink rule. That design interacts with the ownership machinery this PR introduces, so it's filed as #488 with the two-rule shape spelled out rather than rushed in here.
| if matched: | ||
| try: | ||
| rule_as_owner.apply_escalation(ticket_as_owner) | ||
| except AccessError: |
There was a problem hiding this comment.
This except AccessError can fire after the escalation has already been applied — the comment's promise is not kept.
apply_escalation (line 349) performs, in order: ticket.write(vals) → ticket.write({"escalation_rule_ids": ...}) → optional notification/case → the raw UPDATE ... escalation_count + 1 → ticket.message_post(...). Only the first step is the one the comment is guarding against.
If an AccessError is raised by anything from the second step onward — message_post, or flush_recordset(["escalation_count"]) when the owner has read-only rule access — this handler swallows it and continues without setting applied = True. Net result: the ticket is flagged escalated, reassigned and the counter is incremented in the DB, while apply_escalations returns False and check_escalations under-reports its escalated count. The comment ("skip rather than apply with elevated rights") is then actively misleading: it was already applied.
Either pre-check writability before calling apply_escalation (see the _filtered_access suggestion on the routing side) so the except only ever covers a no-op, or set applied = True / re-raise once the ticket write has landed.
There was a problem hiding this comment.
Confirmed, and it was worse than the comment promised — thank you, this was the best catch of the review. The reachable path is message_post: mail.message create requires write access on the document, and an officer's rule that escalates a ticket to another team removes the officer's own write access mid-apply, so the final chatter post raised and the handler swallowed a half-applied escalation (reassigned, counter bumped, message lost, applied False). Fixed with a savepoint around apply_escalation — a post-write denial now rolls the whole escalation back to a clean un-escalated state — plus the chatter post moved ahead of the external notification (so a rolled-back escalation can never have already sent an email), an info log on the skip, and a regression test that pins the rollback end-to-end (no state change, no counter, no message). Note the resulting semantics, called out in HISTORY: an officer rule that reassigns a ticket out of the officer's own scope now fails closed entirely; cross-team escalation rules must be owned by a manager, consistent with the release notes' owner-scope rule.
| for rule in rules: | ||
| if rule.evaluate(ticket): | ||
| # Apply the rule's actions | ||
| owner = rule.eval_as_user_id or rule.create_uid |
There was a problem hiding this comment.
Nothing checks that the owner is still an active user.
eval_as_user_id carries ondelete="restrict", which blocks deletion — but the normal offboarding action in Odoo is archive, and with_user() does not check active. An officer who has left keeps granting their full record-rule scope to every rule they authored, indefinitely and invisibly, because the identity is resolved from the stored m2o rather than from a live principal.
Worth either skipping (with a warning) when not owner.active, or documenting this explicitly next to the ondelete="restrict" follow-up already noted in the PR description. There is no test covering an archived owner.
There was a problem hiding this comment.
Confirmed — with_user never checks active, and archiving removes neither groups nor team membership, so an offboarded officer's rules keep their frozen scope indefinitely. It's stale authority rather than a widening, but for a grievance system that's still an audit finding. Filed as #489 (surface archived owners, consider warn/deactivate). One thing your comment surfaced that we've documented in HISTORY now: ondelete="restrict" means a user who owns rules can no longer be deleted at all — archive is the supported offboarding path.
| if rule.evaluate(ticket): | ||
| rule.apply_escalation(ticket) | ||
| owner = rule.eval_as_user_id or rule.create_uid | ||
| if not owner: |
There was a problem hiding this comment.
A rule that loses its owner silently stops firing, with zero diagnostics.
create_uid is a standard m2o whose FK is ON DELETE SET NULL, so a row can legitimately end up with both eval_as_user_id IS NULL and create_uid IS NULL. This continue then disables the rule permanently — no log line, no UI signal, and the form still renders "Evaluated As" as empty rather than "never runs".
Add a _logger.warning("Escalation rule %s has no evaluation identity; skipping", rule.name) before the continue so this is diagnosable in production. Same at grm_routing_rule.py:300.
Separately: because of the or rule.create_uid fallback, the new post-migration.py backfill is behaviourally redundant — either drop the fallback and rely on the migration, or drop the migration. Keeping both means the UI shows an empty owner while the engine quietly uses a different one.
There was a problem hiding this comment.
The silent skip is fixed — both models now log a warning naming the rule when it has no evaluation identity. On the redundancy claim, partially disagree: the fallback and the migration overlap only on the evaluation path. The migration additionally makes ownership visible in the form, engages ondelete="restrict", and leaves an audit record — and dropping the fallback would make everything depend on the migration having run. The fair version of your point (the fallback masks a skipped migration) is noted in #489 alongside the archived-owner work. Also, for completeness: we re-checked the write()/create() path you were circling — eval_as_user_id is popped from client writes and force-set on create, so it can be neither set nor cleared directly; that's pinned by two existing tests.
| ticket_as_owner = ticket.with_user(owner.id) | ||
| try: | ||
| matched = rule_as_owner.evaluate(ticket_as_owner) | ||
| except AccessError: |
There was a problem hiding this comment.
Exception-driven access control: this except AccessError catches far more than "the owner cannot see this ticket".
evaluate() → _build_evaluation_context(ticket) touches ticket.category_id, channel_id, stage_id, partner_id, team_id, user_id (and on the escalation side sla_status, days_open). An AccessError from any of those related models — or from _check_time_trigger, or from a future callee — is indistinguishable here from "out of scope", and is silently reclassified as "the rule doesn't match". Routing/escalation then stops working for reasons that produce no log line at all.
Odoo 19 has a first-class helper for the check you actually want (odoo/orm/models.py:4121):
ticket_as_owner = ticket.with_user(owner.id)._filtered_access("write")
if not ticket_as_owner:
continue
matched = rule.with_user(owner.id).evaluate(ticket_as_owner)That is one explicit access check instead of two try/except blocks, it cannot mask unrelated AccessErrors, and it removes the ordering hazard flagged on apply_escalation.
There was a problem hiding this comment.
Half taken, half rebutted. Taken: both except AccessError branches now log (debug on the read side, info on the write side), so a rule that never fires is diagnosable. Rebutted: the outer handler cannot swallow related-model AccessErrors — _build_evaluation_context reads only ticket-model fields (the escalation extras sla_status/days_open/is_escalated are stored columns), comodel records are browsed lazily, and CEL-evaluation errors are already caught and logged inside evaluate() — so an AccessError there really does mean "the owner cannot read this ticket". On _filtered_access: it exists as you describe, but it short-circuits under env.su (inheriting the uid-1 issue), it can't replace the try/except (post-write failures — see thread 5), and since the write-denial skip happens before the counter increment it wouldn't change any observable behavior. Happy to take it as a clarity refactor in the mixin follow-up (#491).
| P.parse(rule.condition_cel) | ||
| # If parser not available, skip validation | ||
| except SyntaxError as e: | ||
| except Exception as e: |
There was a problem hiding this comment.
except Exception is broader than the bug it fixes, and only 2 of ~6 copies of this pattern were fixed.
cel_parser.parse() raises SyntaxError, plus RecursionError (cel_parser.py:580) and IndexError on truncated token streams — that's the real gap. Catching bare Exception additionally converts genuine parser bugs (AttributeError, TypeError) into a user-facing "Invalid CEL expression", which hides them. (SyntaxError, RecursionError, IndexError, ValueError) states the intent precisely.
More importantly, this is the wrong altitude: the identical except SyntaxError-only validation lives in five other places that this PR leaves broken —
spp_programs/models/cel/entitlement_inkind_cel.py:182and:328spp_programs/wizard/create_program_wizard_cel.py:713and:908spp_studio/models/logic.py:339,spp_studio/wizard/variable_install_wizard.py:230
A validate(expr) helper in spp_cel_domain.services.cel_parser that owns the exception set would fix all of them once; two more hand-rolled copies here entrenches the drift. (Same code at grm_escalation_rule.py:207.)
There was a problem hiding this comment.
Rebutting both halves, with one genuinely useful thing your comment led us to. (1) In an @api.constrains validator, except Exception is the fail-closed direction: any parser failure rejects the expression. Narrowing to a fixed tuple fails open into a traceback for whatever the parser throws next — and it does have unguarded raises beyond SyntaxError (MAX_RECURSION_DEPTH guards evaluate, not parse). We did add a debug log so a genuine parser bug is distinguishable from a bad expression. (2) The five listed sites don't share this bug: entitlement_inkind_cel.py:182 uses ast.parse, :328 and both wizard sites use compile() — for Python's own parser, SyntaxError-only is correct — and the two spp_studio sites are best-effort extractors with an intended regex fallback, not validators. The real defect at those coordinates is different and bigger: four of them validate expressions the product calls "CEL" against the Python grammar, so the wizard's "Formula syntax is valid" disagrees with what the actual CEL parser accepts. Filed as #490 — genuinely good outcome of this thread.
| # 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 = ( |
There was a problem hiding this comment.
~60 lines of owner-identity plumbing are duplicated verbatim across the two rule models.
eval_as_user_id + the "No Python default on purpose" comment + the _EVAL_TARGETING_FIELDS doc comment + create() + write() are byte-identical between grm_routing_rule.py:108-162 and grm_escalation_rule.py:142-195; only the tuple contents differ. The owner-resolution preamble in apply_routing/apply_escalations (owner lookup, two with_user calls, two nosemgrep annotations, the AccessError handling) is duplicated too.
An AbstractModel mixin — say spp.grm.rule.owner.mixin — holding the field, create, write and a _rule_owner_env(rule, ticket) helper, with each concrete model supplying only _EVAL_TARGETING_FIELDS, would halve this and guarantee the two copies can't drift. They already have: only the routing create() carries the docstring explaining why the key is set rather than popped.
There was a problem hiding this comment.
Agreed in direction, quibble on size: the truly verbatim region is ~30 lines (the field + create/write), not ~60 — _EVAL_TARGETING_FIELDS differs by design and the apply-loop preambles are embedded in differently-shaped loops. The drift you spotted was real and is fixed in this PR (both files now carry the identical full rationale in create/write docstrings). The mixin itself is filed as #491: with exactly two copies and an open question about where such a mixin should live so spp_alerts-style adopters can reach it, it's a refactor that shouldn't ride a security fix.
| self.assertTrue(applied) | ||
| self.assertEqual(rule.escalation_count, before + 1) | ||
| self.assertTrue(ticket.is_escalated) | ||
| def test_portal_user_cannot_read_rules(self): |
There was a problem hiding this comment.
test_rule_readonly_caller_escalation_increments_counter was deleted and its scenario is not re-covered.
AGENTS.md ("Tests"): "NEVER remove or weaken existing tests without explicit approval".
Inverting test_portal_user_can_read_rules into test_portal_user_cannot_read_rules is fine — the assertion genuinely flipped. But the deleted counter test asserted something orthogonal that nothing now covers: that a caller holding only read on the rule models still gets a fully applied escalation, counter included. The replacement (test_escalation_counter_increments_under_owner_identity) exercises a manager-owned rule via the superuser cron, i.e. the fully-privileged path in both dimensions.
Given that the raw-SQL counter increment replaced the sudo() write in the very same PR, this is exactly the regression that test existed to catch. Please re-add an equivalent under the new model (e.g. rule owned by a manager, apply_escalations invoked by a GRM viewer).
Also note the docstring rewrite attributes dropping the base-user read rows to #380 (a portal-ticket issue), and no test covers a plain base.group_user being denied read on the rule models.
There was a problem hiding this comment.
Both concrete sub-claims taken: the #380 attribution was wrong in the test docstring AND in HISTORY (now corrected to "hardening alongside #379/#381" — #380 is the ticket-side portal issue), and base.group_user read denial on the rule models is now pinned by a new test. On the deleted counter test, rebutting: the scenario is not reproducible under the new model even in spirit — the caller's rights stopped mattering by design (everything after the rule search runs as the owner), and the regression it guarded (counter write blocked by caller rights) is structurally impossible now that the increment is raw SQL. The nearest live equivalent (a viewer triggering action_escalate) is noted for the follow-up test pass.
| # Atomic increment: avoids lost updates under concurrent cron/UI | ||
| # escalation, and needs no sudo (raw SQL bypasses ACL). Invisible to | ||
| # spp_audit ORM write-hooks, which is acceptable for a stats counter. | ||
| self.flush_recordset(["escalation_count"]) |
There was a problem hiding this comment.
Legacy (4, id) tuple in a function this PR edits.
Line 392, six lines above this hunk, is still:
ticket.write({"escalation_rule_ids": [(4, self.id)]})AGENTS.md ("Views and XML"): "Always use Command.create() not (0, 0, {...}) tuples for relational writes" — Odoo 19. This should be Command.link(self.id) with Command added to the from odoo import ... line. The PR description files it as an out-of-scope follow-up, but apply_escalation is being modified here, so it costs one line now.
On the increment itself: the rationale comment says the atomic UPDATE prevents "lost updates". Odoo cursors run at REPEATABLE READ, where the previous read-modify-write would have raised a serialization failure and been retried, not silently lost. The change is still an improvement (one statement, no sudo), but the comment overstates what it fixes.
There was a problem hiding this comment.
Tuple fixed (Command.link). On the isolation-level point: you're right that cursors run REPEATABLE READ and the old read-modify-write would have raised a serialization failure rather than silently losing the update — but the conclusion doesn't follow, because Odoo only auto-retries at whole-dispatch granularity, and re-running check_escalations re-fires force_send notifications and case creation for every already-processed ticket. Avoiding the row conflict entirely is the point; the comment now says exactly that instead of "lost updates".
| 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 |
There was a problem hiding this comment.
spp_grm/security/compliance.yaml now contradicts the code it declares.
That file is the module's declared access spec (python -m scripts.compliance.checker spp_grm, wired into .pre-commit-config.yaml:213). Two statements in it are now false and were not updated:
- line 290:
# - base.group_portal: Create/edit own tickets (for portal/self-service)— portal is read-only as of this row. record_rules:does not declare the newrule_spp_grm_ticket_portal.
I ran the checker against this branch: 0 errors, 0 warnings — it validates declared entries against reality but does not detect entries that exist in code and are missing from the spec. So this drift is silent, and the next reader of compliance.yaml will get the pre-#380 picture. (rule_spp_grm_ticket_officer_create is already undeclared for the same reason — worth fixing both while here.)
There was a problem hiding this comment.
Confirmed and fixed: the portal comment now describes read-only access via the sudo'd controller, and both rule_spp_grm_ticket_portal and the pre-existing undeclared rule_spp_grm_ticket_officer_create are declared under record_rules. Good catch on the checker's blind direction.
| Private: invoked server-side by ir.cron, never via RPC. | ||
| """ | ||
| # Find all open tickets | ||
| tickets = self.env["spp.grm.ticket"].search( |
There was a problem hiding this comment.
Hourly cron is now O(open_tickets × rules) access checks, on top of a pre-existing N+1.
check_escalations loops every open ticket and calls apply_escalations, which re-runs self.search([("active", "=", True)], order="sequence, id") per ticket (line 544). This PR then adds, per (ticket × rule): an Environment construction for with_user, and an AccessError-driven access check on the ticket — each of which recomputes the owner's ir.rule domain and issues SQL. At 10k open tickets × 20 rules that is 200k access evaluations per hour.
Two cheap structural fixes:
- Hoist the rule search out of
apply_escalations(passrulesin, or split an internal_apply_escalations(ticket, rules)). - Group rules by
eval_as_user_idand resolve the visible ticket set once per owner —tickets.with_user(owner)._filtered_access("write")— instead of per (ticket, rule) pair. Distinct owners are typically a handful, so this collapses the access work toO(owners).
There was a problem hiding this comment.
Partially taken. The per-ticket rule re-search was real waste and is hoisted (searched once per cron pass, passed down). The rest is smaller than it looks: environments are interned, with_env preserves the prefetch set (so ticket reads batch per owner), and rule-domain checks run in memory over the prefetched cache — the per-pair cost is not 200k SQL access evaluations. The owner-grouped restructure as described would change semantics: routing is first-match-wins in global sequence, id order and escalation is last-writer-wins in that order, so iterating owner-by-owner can change which rule wins and the final ticket state. The safe version (precompute per-owner allowed-ticket sets, keep the original iteration order) plus batching and per-ticket error isolation (one bad ticket currently aborts the whole hourly run — found while verifying this) is filed as #492.
| """#381: the three rule-engine methods must be rejected for RPC dispatch.""" | ||
| from odoo.service.model import call_kw | ||
|
|
||
| for model, method, args in [ |
There was a problem hiding this comment.
Two gaps in the new suite.
1. apply_escalation is not covered here. The changelog and README claim four guarded entry points (apply_routing, apply_escalations, apply_escalation, check_escalations); this loop tests three. The missing one is the record-level method that actually writes the ticket, so it is the one most worth pinning:
(ESCALATION, "apply_escalation", [rule.id, self.foreign_ticket.id]),2. test_escalation_counter_increments_under_owner_identity (line 156) assumes exactly one open ticket exists database-wide. check_escalations() scans every is_closed = False ticket and the rule uses condition_cel: "" (always matches), so the counter increments once per open ticket. The before + 1 assertion holds today only because nothing else ships an open ticket; it breaks the moment demo data or another post_install fixture adds one. Assert against the ticket count, or call apply_escalations(self.foreign_ticket) directly instead of the whole-DB cron.
Also worth noting for test_officer_rule_cannot_seize_foreign_ticket: with condition_cel: "", evaluate() returns True without ever reading the ticket, so that test only proves the ticket write was denied — it would still pass if owner-identity evaluation were removed entirely. A non-empty condition referencing a ticket field would make it a real regression test for the evaluate path.
There was a problem hiding this comment.
All three taken, with one wording correction. (a) apply_escalation added to the dispatch-guard loop — it was @api.private all along, but HISTORY advertises four guarded methods and the test now pins all four. (b) The counter test now targets one explicit ticket instead of the DB-wide cron scan; for the record nothing in the dependency closure ships an install-time ticket today, so it was latent rather than live, but agreed it was one fixture away from flaking. (c) A second seize test with a non-empty condition now pins the read-side bound (the empty-condition short-circuit meant only the write bound was covered). The strong form — "would pass even if owner-identity evaluation were removed entirely" — wasn't quite right (reverting to the superuser cron makes the original test fail loudly), but the blind spot was real and is closed.
…gging, scoped test surface Review-response fixes (kneckinator, 15 threads; verification dossier in internal docs): - apply_escalation is now atomic: a savepoint rolls the whole escalation back when a post-write step is denied (e.g. an officer's rule reassigns the ticket out of the officer's own scope, then the chatter post fails), instead of persisting a half-applied escalation with its message lost. The chatter post moves ahead of the external notification so a rolled- back escalation can never have already sent an email. Regression test. - Rules with no evaluation identity, superuser-owned rules (which evaluate with record rules bypassed), and owner-access skips are now logged; the migration calls out uid-1 backfills by name. - The legacy routing/escalation suites author rules as a GRM manager instead of the superuser test env, so they exercise the owner-scoped path rather than the bypass; new tests pin base-user read denial on the rule models, the read-side (evaluate) access bound, apply_escalation's RPC guard, and the out-of-scope rollback. - Portal ticket rule: all four perms enabled so the scoping holds if a future ACL change re-grants portal write; comment rewritten. - compliance.yaml: portal access description corrected; declare rule_spp_grm_ticket_portal and rule_spp_grm_ticket_officer_create. - Escalation cron searches the active-rule set once per pass, not once per open ticket. - HISTORY: #380 misattribution on the rule-model row drop corrected (ticket-side portal scoping is #380; the row drop is #379/#381 hardening); ondelete=restrict user-deletion consequence documented. - Command.link for the escalation m2m; counter comments now describe the serialization-failure/retry rationale accurately; owner-identity create/write docstrings re-synced between the two rule models.
|
@kneckinator thanks for a genuinely strong review — every one of the 15 threads got an evidence-checked reply (we verified each claim against Odoo 19 core sources before responding). Fixed in 1bbcdf6 (spp_grm_cel 46/46, spp_grm 30/30 tests green):
Filed as follow-ups (with your threads referenced): #486 internal-user ticket read scoping, #487 portal household-visibility question, #488 officer write access to manager rules, #489 rule-ownership lifecycle, #490 Python-grammar "CEL" validation in spp_programs (found while verifying thread 9), #491 owner-identity mixin, #492 cron batching/error isolation. Rebutted with receipts (details in the threads): the REPEATABLE READ point (dispatch-granularity retry blast radius), the related-model README regen for the HISTORY changes will be applied from CI's pinned generator diff as usual. Ready for re-review. |
Code review — 15 findingsReviewed at high effort against Verified clean locally: Correctness — high severity1. 2. 3. 4. 5. 6. 7. Performance / log noise8. 9. 10. Reuse / clarity11. 12. 13. if any(f in vals for f in self._EVAL_TARGETING_FIELDS):
vals = dict(vals, eval_as_user_id=self.env.uid)
elif "eval_as_user_id" in vals:
vals = {k: v for k, v in vals.items() if k != "eval_as_user_id"}Duplicated identically in Tests14. 15. |
…osed side effects, once-per-ticket rules spp_grm - SLA-breach handling no longer runs inside the stored sla_status compute: the compute schedules it and it runs at the transaction's precommit stage (same transaction), so the escalation engine's writes/savepoints never execute mid-computation. - HttpCase for the sudo'd portal submit route (the justification for the read-only portal ACL). spp_grm_cel - Engine loads the active rule set elevated and resolves each rule's owner once per cron pass (warnings once, not once per ticket); a rule whose owner is archived is skipped; superuser-owned rules still warn and run. - apply_escalations: a rule applies at most once per ticket; one failing ticket is rolled back and logged instead of aborting the pass. - apply_escalation: single ticket write; notification and case creation re-raise AccessError (whole escalation rolls back) and isolate other failures in their own savepoint; case creation fixed (presenting_issue, required case_worker_id) — it had never worked. - "Take Ownership" action on both rule forms; write() accepts eval_as_user_id only when it is the acting user; remediation wording fixed in migration, engine and HISTORY. "Check Escalation" button limited to officers+. - Tests: engine behaviour suite (batch breach with in-compute rollbacks, fail-closed side effects, once-per-ticket, pass isolation, warning dedupe, button for users without rule access), ownership tests, restored counter-test assertions.
|
@kneckinator thanks — round 2 was as sharp as round 1. Every finding was verified against Odoo 19 core and, where it made sense, with a reproduction test before touching code (10 of them; most are now permanent tests). Fixes are in 0a072d1 (plus 3b2446a line-wraps and 205ba28 regenerated READMEs). Per finding: 1. Savepoint inside the 2. Rule search in the caller's env — fixed as you suggested: 3. 4. Post-write steps never triggered the rollback — confirmed, and the reproduction found the feature was broken before ownership entered the picture: 5. Only AccessError caught — fixed: 6. Archived owner — confirmed (core never checks 7. "Re-save" remediation — you're right, it did nothing. New Take Ownership button on both rule forms ( 8. Warnings per rule per ticket — fixed: owners are resolved once per pass in 9. Re-escalation every hour — confirmed by test (two passes → count 2, two chatter posts). A rule now applies at most once per ticket ( 10. Second write raises — confirmed by traceback (L398). Single merged write now; with it, the out-of-scope case is caught by 11. Duplication / mixin — #491 is the follow-up (a foundation-module mixin shared with spp_alerts is a cross-module change we don't want in a security PR). Fair point on the citations: PR #364 is not merged, so "see spp_alerts #364" now reads "see PR #364, spp_alerts". 12. 13. write() double scan — collapsed as suggested (with the self-only allowance from 7). 14. Portal submit untested — 15. Tests / PR body — (a) the deleted portal-read test's own docstring asked to be replaced by a read-denial test once evaluation stopped depending on it; (b) the counter test's Suites: spp_grm_cel 60/60, spp_grm 33/33; ruff/pylint-odoo/semgrep/compliance clean. Re-requesting review. |
sla_status is a stored compute rendered on the ticket form, so it also runs on the pseudo-record of an onchange. records.ids resolves those to their origin, so an edit the user never saved queued the real ticket and escalated it at the next commit: counter, chatter, notification. Drop pseudo-records before queueing; NewId is falsy, so ticket.id tells them apart.
Three fixes from review of #415: - The escalation notification was sent before case creation, so an owner denied the case create had already mailed a rolled-back escalation. The rollback also drops the escalation_rule_ids link that suppresses the rule, so the hourly cron repeated the ghost mail every pass. Send it last, after every effect that can still be denied. - apply_routing wrote the ticket and bumped the counter with no savepoint. Its caller swallows exceptions, so a database failure there left an aborted cursor that took down every later statement of the request. Wrap it the way the escalation path already is. - action_escalate is RPC-dispatchable, so the button's groups= gated nothing: any internal user could force a full escalation pass on any ticket. Require write access on the ticket, which also keeps an officer inside their own scope.
Two more fixes from review of #415: - write() dropped a third-party eval_as_user_id silently and returned True, so a data fix doing rules.write({"eval_as_user_id": owner.id}) reported success while the rules kept evaluating as their old owner. Raise UserError pointing at Take Ownership instead. The 19.0.2.0.2 migration backfills with raw SQL, so it is unaffected. - Routing resolved the active rule set and each rule's evaluation owner inside the per-ticket loop, so a superuser-owned or archived-owner rule logged its warning on every single ticket created. Resolve it once for the created batch, as the escalation cron already does.
…egible Last three fixes from review of #415: - _create_case_from_ticket assigned the ticket assignee "else whoever the rule evaluates as". For an unassigned ticket under a superuser-owned rule that is __system__, so the required case worker was satisfied with OdooBot. Vet the resolved worker and refuse the escalation (rolled back by the caller's savepoint) rather than file a case nobody is answerable for. _compute_user_id falls back to the creating user, so a ticket created from a shell or cron env hits this directly. - The CEL constraint caught bare Exception and reported everything as "Invalid CEL expression", so a defect inside the parser surfaced as the user's own data-entry error with the traceback discarded. Report SyntaxError/RecursionError as before; log anything else with its traceback and say it is internal. - test_sla_breach asserted the breach queue key was gone from cr.precommit.data, which Callbacks.run() clears unconditionally, so the assertion could not fail. The pop is really pinned by the note count (the compute schedules per ticket, so duplicate hooks would reprocess the batch) — say so where it is asserted, and cover the cross-flush case that had no test.
kneckinator
left a comment
kneckinator
left a comment
There was a problem hiding this comment.
I ran a review pass over this branch and pushed the fixes directly to it (6d8e4460..ebee9073, 4 fix commits + 3 README regenerations). Every fix has a test written first that fails on the preceding commit. CI is green.
Effects that outlive a rollback
1. Ghost escalation mail — apply_escalation sent the notification (force_send=True, real SMTP) before _create_case_from_ticket. An owner who may send mail but not create cases therefore mailed an escalation that was then rolled back — and since the rollback also drops the escalation_rule_ids link that suppresses the rule, the hourly cron re-sent the same mail every pass, forever. The notification now goes last, after every effect that can still be denied. test_no_notification_is_sent_for_an_escalation_that_rolls_back observed a real send on the old code ([37] != []).
2. Routing had no savepoint — apply_routing wrote the ticket and issued the raw match_count UPDATE unguarded. write() only defers its SQL, so a constraint or FK failure (a rule pointing at a since-deleted user) surfaced at a later flush, was swallowed by _apply_routing_rules' bare except, and left an aborted cursor that took down every subsequent statement of the request — the rest of a portal submission's own create() included. Now wrapped in cr.savepoint() with the same except AccessError / except Exception split the escalation path already had. test_routing_failure_is_contained_by_a_savepoint.
3. action_escalate was RPC-reachable — apply_routing/apply_escalations/apply_escalation/check_escalations are @api.private, but spp.grm.ticket.action_escalate is public and reaches apply_escalations in-process. The button's groups="spp_grm.group_grm_officer" is presentation-only, and base.group_user holds unscoped read on the tickets, so any internal user could call_kw a full escalation pass — counters, chatter, mail, case creation — on any ticket. It now calls check_access("write"), which given the ACLs means officers and above (matching the changelog claim) and additionally keeps an officer inside their own record-rule scope.
4. Unsaved form edits queued a breach hook — sla_status is a stored compute rendered on the ticket form, so it also runs on the pseudo-record of an onchange. Worth flagging precisely, because it is not what it looks like: in Odoo 19 recordset.ids maps new records to their origin ids (orm/models.py:5905-5909), so this queued the real ticket, and the precommit hook escalated it at commit for a change the user never saved. Guarded with filtered(lambda t: t.id) (NewId is falsy). Two related things that do not apply here: sorted() over mixed ids does not raise (NewId is @functools.total_ordering with an int-aware __lt__), and exists() keeps new records by convention, so it is no filter.
Attribution and diagnostics
5. Owner warnings once per pass, not per ticket — the rule set and each rule's evaluation owner were resolved inside the per-ticket routing loop, so a superuser-owned or archived-owner rule logged its warning on every ticket created. apply_routing now takes an optional pre-resolved rules= and create() resolves once for the batch, as check_escalations already did. (A single-ticket create still logs once — there the batch is one ticket, and a rule evaluating unbounded should stay visible.)
6. write() dropped a third-party eval_as_user_id silently and returned True, so a data fix doing rules.write({"eval_as_user_id": new_owner.id}) reported success while the rules kept evaluating as their old owner. It now raises UserError pointing at Take Ownership. UserError rather than AccessError because the write itself is permitted — the field simply is not the caller's to set. The 19.0.2.0.2 migration backfills with raw SQL, so it is unaffected.
7. Case worker could be OdooBot — case_worker_id is required=True on spp.case, and "ticket assignee, else whoever the rule evaluates as" resolves to __system__ for an unassigned ticket under a superuser-owned rule. _compute_user_id falls back to the creating user, so a ticket created from a shell or cron env hits this directly. The resolved worker is now vetted (real, active) and the escalation is refused — rolled back by the caller's savepoint — rather than filing a case nobody is answerable for.
8. except Exception in the CEL constraint reported every failure as "Invalid CEL expression", so a defect inside the parser surfaced as the user's own data-entry error with the traceback discarded. SyntaxError/RecursionError (what cel_parser actually raises for bad input) still map to that message; anything else is logged with its traceback and reported as internal.
One test-only fix, with a caveat worth reading
9. assertNotIn("spp_grm.sla_breach_ids", cr.precommit.data) cannot fail — Callbacks.run() clears data unconditionally (tools/misc.py:1170-1181). Removed. But the follow-on conclusion I first drew from that ("so the pop isn't pinned") was wrong, and I only caught it by mutating .pop( → .get(: two tests failed. Callbacks.add() appends duplicates and _compute_sla_status schedules per ticket, so a two-ticket breach queues the hook twice and without the drain the second call reprocesses the batch. The drain is pinned by the note count, not by inspecting data — that is now said where it is asserted, plus a new cross-flush test that had no coverage.
Three existing tests changed expectations — please eyeball these
Each encoded the behaviour a finding identified as wrong. Original intent and assertions are preserved, and the suite gained 7 tests net:
test_check_escalation_button_for_user_without_rule_accessasserted a plain internal user could press "Check Escalation" — the hole in #3, and contrary to this branch's own changelog line. Replaced by a denial test plus an officer-actor success test.test_eval_as_user_id_not_writable_and_rebinds_on_retargetandtest_eval_as_user_id_client_write_only_accepted_for_selfasserted the silent drop from #6; they now expect the raise and keep their "must not take effect" assertion.
Changelog fragments updated for all of the above; the two READMEs were regenerated with the pinned generator (it also drifts six unrelated modules, which I reverted). Local runs: spp_grm_cel 67/67, spp_grm 35/35, and spp_grm_case_link 78, spp_grm_demo 31, spp_grm_programs 42, spp_grm_registry 35 — no failures, no stray ERROR lines in any log.
2 participants
Fixes the GRM security cluster surfaced during the PR #266 staff review and confirmed by the PR #399 review verification. Three interlocking issues, one PR because they share a trust chain: #380 gives portal RPC reach to tickets → #381 gives reach to the rule engine → #379 means the engine acts as superuser.
Closes #379, #380, #381.
What's in it
spp_grm_cel19.0.2.0.1 → 2.0.2 + migrationspp_grm19.0.2.0.1 → 2.0.2@api.private(not RPC-callable)spp_grm_cel#379 — owner-identity evaluation
The hourly
check_escalationscron ran as superuser with record rules bypassed. A GRM officer (who can author rules) could create one always-match escalation rule pointing at themselves; within the hour the cron reassigned every open ticket in the database to them. Same elevated-evaluation shape as spp_alerts #364.Fix (mirrors the #364 owner-identity pattern):
eval_as_user_idon both rule models — no Python default (a default would let_init_columnbackfill the upgrade user and let a client forge it viadefault_eval_as_user_id); forced to the creator increate(), and re-bound to the editor only when a rule's targeting/action fields change inwrite().sequence,active) are deliberately excluded from the re-bind set: archiving/reordering an officer's rule must not silently transfer ownership to the manager doing that routine cleanup (a confused-deputy escalation caught in review).apply_routing/apply_escalationsload the active rule set elevated (no acting user needs read on the rules) and evaluate/apply each rulewith_user(owner); a ticket the owner can't read/write is skipped, never applied elevated. So an officer's rule can only ever act within the officer's own record-rule scope. The cron and the sudo'd SLA path inherit this automatically (the identity comes from the rule, not the caller).spp_grm) is deferred to the end of the triggering transaction (precommit) instead of running inside the storedsla_statuscompute, so the engine's writes/savepoints never execute mid-computation.eval_as_user_idfromcreate_uidfor pre-existing rules.#380 — portal ticket isolation
spp.grm.ticketgrantedbase.group_portalread/write/create with noir.ruletargeting portal, so any authenticated portal user could read and rewrite every grievance in the system over RPC (the controller'spartner_idscoping is presentation-only).partner_id == user.partner_id(own tickets only).#381 — entry-point guards
apply_routing,apply_escalations,apply_escalation,check_escalationsare now@api.private— rejected forcall_kwRPC dispatch. The cron (server-sidemodel.check_escalations()), the SLA-breach path, and ticket create/stage-write are all in-process Python calls and unaffected.Also (folded in from the #399 review)
UPDATEformatch_count/escalation_count(drops thesudo()read-modify-write; no lost updates under concurrent cron/UI escalation).ValidationError(wasSyntaxError-only).🔴 Release notes — behavior changes
eval_as_user_idto any user other than yourself now raisesUserError(it used to be dropped silently, so a data-fix script could report success while nothing changed).action_escalate("Check Escalation") requires write access on the ticket — enforced on the method, not only via the view'sgroups=.SyntaxError/RecursionError) are reported as the user's error; an unexpected parser failure is logged with its traceback and surfaced as an internal error rather than blamed on the expression.Verification
spp_grm_cel43 tests,spp_grm30 tests — 0 failed, 0 errors.openspp2-code-reviewer(conventions/principles) + an adversarial pass against Odoo 19 core. Both confirmed the two HIGH holes closed and the owner-identity/guard mechanisms sound; the one Important finding (theactive/sequenceconfused-deputy) is fixed with a regression test. Lint clean (ruff, pylint-odoo, bandit, semgrep).Test adaptations (existing tests replaced, not dropped)
test_portal_user_can_read_rules→test_portal_user_cannot_read_rules: the old test documented portal read as a current implementation dependency and asked to be replaced by a read-denial test once evaluation no longer ran as the acting user. That is now the case.test_rule_readonly_caller_escalation_increments_counter→test_escalation_counter_increments_under_owner_identity: same three assertions (applied, counter +1,is_escalated), exercised under owner identity instead of a portal caller (portal users no longer reach the engine).Follow-ups (not in scope)
spp.grm.ticket(pre-existing; needs a base.group_user record rule and a look at the res_partner compute that depends on the ACL row).active/sequence.ondelete="restrict",create_uidfallback).