Skip to content

security(grm): evaluate rules as their owner, scope portal tickets, guard entry points (#379, #380, #381) - #415

Open
gonzalesedwin1123 wants to merge 3 commits into
19.0from
security/grm-rule-eval-cluster
Open

security(grm): evaluate rules as their owner, scope portal tickets, guard entry points (#379, #380, #381)#415
gonzalesedwin1123 wants to merge 3 commits into
19.0from
security/grm-rule-eval-cluster

Conversation

@gonzalesedwin1123

Copy link
Copy Markdown
Member

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

Issue Severity Fix Module (version)
#379 HIGH GRM routing/escalation rules evaluate with their creator's identity, not the superuser cron spp_grm_cel 19.0.2.0.1 → 2.0.2 + migration
#380 HIGH Portal users scoped to their own grievance tickets (record rule + read-only ACL) spp_grm 19.0.2.0.1 → 2.0.2
#381 Medium Rule-engine entry points made @api.private (not RPC-callable) spp_grm_cel

#379 — owner-identity evaluation

The hourly check_escalations cron 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):

  • New system-managed eval_as_user_id on both rule models — no Python default (a default would let _init_column backfill the upgrade user and let a client forge it via default_eval_as_user_id); forced to the creator in create(), and re-bound to the editor only when a rule's targeting/action fields change in write().
  • Operational toggles (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_escalations evaluate and apply each rule with_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).
  • A migration backfills eval_as_user_id from create_uid for pre-existing rules.

#380 — portal ticket isolation

spp.grm.ticket granted base.group_portal read/write/create with no ir.rule targeting portal, so any authenticated portal user could read and rewrite every grievance in the system over RPC (the controller's partner_id scoping is presentation-only).

  • New portal record rule: partner_id == user.partner_id (own tickets only).
  • Portal ACL row reduced to read-only — submission is handled by the sudo'd portal controller, which needs no direct model write/create.

#381 — entry-point guards

apply_routing, apply_escalations, apply_escalation, check_escalations are now @api.private — rejected for call_kw RPC dispatch. The cron (server-side model.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)

  • Atomic UPDATE for match_count/escalation_count (drops the sudo() read-modify-write; no lost updates under concurrent cron/UI escalation).
  • CEL validation now reports any parser error as ValidationError (was SyntaxError-only).
  • Dropped the portal/internal-user read rows on both rule models — with owner-identity evaluation the acting user never reads the rules, so those rows only exposed the routing/escalation map to enumeration.

🔴 Release notes — behavior changes

  1. GRM automation rules now evaluate with their creator's identity. A rule can only route/escalate within that user's ticket scope. Officer-authored broad rules will scope down; cross-team rules must be owned by a manager/admin. In particular, officer-authored routing rules no longer apply to brand-new unassigned tickets (no officer can see them yet) — route those with manager/admin-owned rules. Existing rules are attributed to their original creator by the migration; review any rule whose creator's permissions have changed since it was authored.
  2. Portal users can no longer read or modify other users' grievance tickets over RPC. No portal-UI change.
  3. The three rule-engine methods are no longer callable over RPC.

Verification

  • TDD: red tests first (officer seizes all tickets; portal reads others' tickets; RPC dispatch succeeds) → green after fix.
  • spp_grm_cel 43 tests, spp_grm 30 tests — 0 failed, 0 errors.
  • Reviewed twice before push: 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 (the active/sequence confused-deputy) is fixed with a regression test. Lint clean (ruff, pylint-odoo, bandit, semgrep).
  • README.rst / index.html regen deferred to CI's pinned generator (will apply its printed diff).

Follow-ups (not in scope)

  • trigger_after_hours is not enforced at apply time (_check_time_trigger is unused) — pre-existing; file to wire it in or remove the field.
  • ondelete="restrict" on eval_as_user_id blocks deleting a user who owns rules — consider reassign-on-archive if it bites.
  • Legacy (4, id) tuple at grm_escalation_rule.py — Odoo 19 Command.link nit, pre-existing.

…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

codecov Bot commented Aug 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.88235% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.39%. Comparing base (0820667) to head (341966a).

Files with missing lines Patch % Lines
spp_grm_cel/models/grm_escalation_rule.py 77.14% 8 Missing ⚠️
spp_grm_cel/models/grm_routing_rule.py 84.84% 5 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             19.0     #415      +/-   ##
==========================================
+ Coverage   72.24%   72.39%   +0.15%     
==========================================
  Files         419      450      +31     
  Lines       29813    30972    +1159     
==========================================
+ Hits        21539    22423     +884     
- Misses       8274     8549     +275     
Flag Coverage Δ
spp_base_common 91.07% <ø> (ø)
spp_grm 63.34% <ø> (?)
spp_grm_case_link 100.00% <ø> (?)
spp_grm_cel 77.53% <80.88%> (+0.29%) ⬆️
spp_grm_demo 80.13% <ø> (?)
spp_grm_programs 92.13% <ø> (?)
spp_grm_registry 100.00% <ø> (?)
spp_programs 65.27% <ø> (ø)
spp_registry 87.22% <ø> (+0.07%) ⬆️
spp_security 69.56% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
spp_grm_cel/models/grm_routing_rule.py 83.01% <84.84%> (-0.53%) ⬇️
spp_grm_cel/models/grm_escalation_rule.py 76.74% <77.14%> (-0.02%) ⬇️

... and 32 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

security(spp_grm_cel): escalation/routing rules evaluate as superuser — GRM officer can seize every ticket [Severity: High]

1 participant