security(grm): evaluate rules as their owner, scope portal tickets, guard entry points (#379, #380, #381) - #415
security(grm): evaluate rules as their owner, scope portal tickets, guard entry points (#379, #380, #381)#415gonzalesedwin1123 wants to merge 5 commits into
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% 72.94% +0.69%
==========================================
Files 419 453 +34
Lines 29813 31257 +1444
==========================================
+ Hits 21539 22800 +1261
- Misses 8274 8457 +183
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. |
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_escalationsevaluate and 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).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
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).Follow-ups (not in scope)
trigger_after_hoursis not enforced at apply time (_check_time_triggeris unused) — pre-existing; file to wire it in or remove the field.ondelete="restrict"oneval_as_user_idblocks deleting a user who owns rules — consider reassign-on-archive if it bites.(4, id)tuple atgrm_escalation_rule.py— Odoo 19Command.linknit, pre-existing.