Skip to content

security(grm): restrict GRM automation rules to GRM staff (drop portal write/create) - #266

Merged
gonzalesedwin1123 merged 3 commits into
19.0-staging-sec-batch1from
security-grm-cel-portal-rule-acl
Jul 28, 2026
Merged

security(grm): restrict GRM automation rules to GRM staff (drop portal write/create)#266
gonzalesedwin1123 merged 3 commits into
19.0-staging-sec-batch1from
security-grm-cel-portal-rule-acl

Conversation

@gonzalesedwin1123

Copy link
Copy Markdown
Member

Problem

spp_grm_cel/security/ir.model.access.csv granted base.group_portal read + write + create
(1,1,1,0) on the global config models spp.grm.routing.rule and spp.grm.escalation.rule,
with no ir.rule record rules scoping them. A low-privileged portal user could therefore
create an always-matching routing or escalation rule via RPC:

  • Routing rules run on ticket creation and can write team_id, user_id, severity,
    priority.
  • Escalation rules run on stage changes and via the hourly cron over all open tickets
    (executed elevated), and can reassign/escalate, send configured notifications, and create
    cases.

So a single planted rule turns portal access into unauthorized, global GRM workflow
modification and potential mass disruption of grievance handling. Severity: High.

Fix

Reduce the two portal ACL rows to read-only (1,0,0,0), matching the existing
base.group_user rows:

access_spp_grm_routing_rule_portal_user,...,base.group_portal,1,0,0,0
access_spp_grm_escalation_rule_portal_user,...,base.group_portal,1,0,0,0

Rule evaluation runs as the current user (ticket.create()apply_routing()
search() on the rule model), so read access must be retained or routing silently stops
applying — exactly why base.group_user also has read-only. Portal users lose the ability to
create or modify global rules; GRM officers/managers retain full management.

Portal ticket submission via the controller is already sudo()d
(spp_grm/controllers/grm_portal.py), so that flow is unaffected either way.

Tests

New spp_grm_cel/tests/test_rule_acl.py:

  • portal user is denied create on both rule models (AccessError);
  • portal user is denied write on both rule models (AccessError);
  • portal user retains read (evaluation still works);
  • a GRM manager can still create both rule types.

Written test-first (4 denial tests failed pre-fix — confirming portal could create/write —
then passed after). ./spp test spp_grm_cel35 passed, 0 failed; ./spp lint clean.

Out of scope (noted for awareness)

  • spp_grm grants portal 1,1,1,0 on spp.grm.ticket itself (portal can create tickets via
    RPC, not only via the sudo controller) — separate question, not changed here.
  • apply_escalation writes escalation_count without sudo() (unlike routing's sudo'd
    counter) — a pre-existing inconsistency; escalation triggered by a read-only portal user now
    fails closed (caught/logged), which is the intended behavior.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request restricts portal users to read-only access for GRM routing and escalation rules by updating the ACL configurations in ir.model.access.csv. It also introduces a comprehensive suite of integration tests in test_rule_acl.py to verify these access control constraints. There are no review comments, and I have no additional feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.27%. Comparing base (f0a58b0) to head (bebd609).
⚠️ Report is 1 commits behind head on 19.0-staging-sec-batch1.

Additional details and impacted files

Impacted file tree graph

@@                     Coverage Diff                     @@
##           19.0-staging-sec-batch1     #266      +/-   ##
===========================================================
+ Coverage                    71.25%   71.27%   +0.01%     
===========================================================
  Files                          227      226       -1     
  Lines                        15441    15664     +223     
===========================================================
+ Hits                         11002    11164     +162     
- Misses                        4439     4500      +61     
Flag Coverage Δ
spp_base_common 91.07% <ø> (ø)
spp_grm_cel 77.23% <100.00%> (?)
spp_oauth ?
spp_programs 65.27% <ø> (ø)
spp_registry 86.94% <ø> (ø)
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_escalation_rule.py 76.76% <100.00%> (ø)

... and 11 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.

@gonzalesedwin1123

Copy link
Copy Markdown
Member Author

Staff review (post-rebase, head 1c36d437): SHIP-WITH-NITS. Both nits are now addressed in aa06b303:

  • Added the missing readme/HISTORY.md fragment for the 19.0.2.0.1 bump. (Note: CI does not catch this class of omission — an absent fragment produces no README drift, so the oca-gen hook has nothing to diff.)
  • Sudo'd the escalation_count write to match its routing-rule twin at grm_routing_rule.py:263. Previously a caller without write access failed midway through apply_escalation — after the notification was sent and the case created — rather than failing closed.

Review confirmed: the exposure was real and globally scoped (neither rule model has any ir.rule, so the ACL row was the only boundary); portal grievance submission does not regress (all three controller routes are read/read/sudo-create); the retained portal read is deliberate because apply_routing/apply_escalations search as the current user; tests use a genuine portal user with Odoo 19 group_ids and fail without the fix; no upstream drift re-opened it; no migration needed (ACL rows are noupdate=False); no version collision; independent of #327/#329.

Three follow-ups filed, out of scope here: #379 (rule CEL evaluates as superuser — same shape as #364, lets a GRM officer seize every ticket), #380 (portal has read/write/create on every ticket with no record rule — the ticket-model half of this over-grant), #381 (unguarded RPC-callable rule-engine entry points).

The ACL granted base.group_portal read/write/create on spp.grm.routing.rule
and spp.grm.escalation.rule. These are global config models with no record
rules, so a portal user could create an always-matching routing/escalation
rule via RPC. Routing rules run on ticket creation and escalation rules run on
stage changes and via the hourly cron over all open tickets (elevated), so a
single planted rule could reassign/escalate/notify across all grievances.

Reduce the two portal rows to read-only (1,0,0,0), matching the base.group_user
rows. Rule evaluation runs as the current user and only needs read, so ticket
routing still works; portal users can no longer create or modify global rules.
GRM officers/managers retain full management.

Add regression tests: portal users are denied create/write on both rule models
but retain read, while a GRM manager can still create both.
The escalation counter was written in the caller's context while its
routing-rule twin already uses sudo. A caller without write access left
the escalation applied half-way: notification sent and case created,
counter and chatter missing.

Also adds the missing HISTORY fragment for the version bump.
@gonzalesedwin1123
gonzalesedwin1123 force-pushed the security-grm-cel-portal-rule-acl branch from 168d879 to bebd609 Compare July 28, 2026 08:58
@gonzalesedwin1123
gonzalesedwin1123 marked this pull request as ready for review July 28, 2026 09:05
@gemini-code-assist

Copy link
Copy Markdown

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@gonzalesedwin1123
gonzalesedwin1123 merged commit f450212 into 19.0-staging-sec-batch1 Jul 28, 2026
20 checks passed
@gonzalesedwin1123
gonzalesedwin1123 deleted the security-grm-cel-portal-rule-acl branch July 28, 2026 09:05
gonzalesedwin1123 added a commit that referenced this pull request Aug 13, 2026
…g keys, GRM rule ACL (#327, #329, #265, #266) (#399)

* security(dci): stop DCI Administrator group from granting system admin (#327)

Reviewed head: d171acb

* security(key_management): stop Key Management Admin group from granting system admin (#329)

Reviewed head: 180c7fc

* security(oauth): restrict OAuth signing-key settings to system admins (#265)

Reviewed head: e9dc451

* security(grm): restrict GRM automation rules to GRM staff (drop portal write/create) (#266)

Reviewed head: bebd609

* fix(spp_oauth): restore @api.model on default_get, field-gate signing keys (#399)

Review response on PR #399 (findings 1-2):

- default_get() override lacked @api.model. call_kw reads the dispatch
  marker off the most-derived method, so every external RPC call to
  res.config.settings.default_get crashed with a TypeError once this
  module was installed (web Settings UI unaffected - it resolves
  defaults in-process). Regression-tested through the real dispatcher.
- oauth_priv_key/oauth_pub_key now carry groups="base.group_system":
  a settings save by an unauthorized principal fails closed with
  AccessError instead of silently deleting the stored parameters via
  set_param(False). The default_get pop stays - field groups are not
  enforced in default_get. Defence-in-depth: core's ACL already limits
  the model to system admins; the gate holds if that is ever re-widened.

* fix(spp_dci): make DCI PII visibility opt-in per admin, harden migration (#399)

Review response on PR #399 (findings 3, 7, 8):

- Drop the spp_security.group_spp_admin -> group_dci_admin implication:
  PII rendering is now opt-in per administrator via an explicit,
  reviewable Access Rights grant instead of an automatic side effect
  of adminship, mirroring the deliberate key-custody separation in
  spp_key_management. The pinning test is inverted to guard against a
  consistency sweep reintroducing the link. The link never shipped in
  any release and no production DB is built from this staging branch,
  so no unlink migration is needed; the upgrade-test seed DB is
  rebuilt for the refreshed evidence run.
- Migration comment refresh now strips only the stale sentence
  ("Members must already be system administrators") so operator
  rewrites and extensions survive, honouring noupdate.
- Migration warning counts all_user_ids (transitive membership),
  matching the spp_key_management migration's counting basis - safe
  now that nothing implies the group.
- Declare the spp_security dependency explicitly: the security tests
  pin design decisions against spp_security.group_spp_admin, and the
  dependency was previously only transitive via spp_registry.

* fix(spp_key_management): correct encrypted_key wrap comment, add app icon (#399)

Review response on PR #399 (findings 5, 6):

- The encrypted_key field comment claimed the value is always KMS
  ciphertext; for the database provider it is wrapped by the master
  KEK, which in zero-config setups is derived from database.uuid.
  State both provider families accurately. The field gate itself is
  unchanged (the KMS-provider sudo rework is tracked in #330/#331).
- The top-level Key Management menu now carries the module icon -
  every other top-level app menu sets web_icon; without it the app
  switcher shows a generic placeholder tile. Pinned in the existing
  load_menus test via web_icon_data.

* test(spp_grm_cel): internal manager fixture, escalation counter regression (#399)

Review response on PR #399 (findings 10, 11, 12):

- The GRM manager fixture now links base.group_user: the spp_grm group
  chain carries no user-type group, so the manager was created as a
  share=True external principal and the staff-retention tests proved
  less than intended.
- New regression test: a caller with read-only rule access gets a
  fully applied escalation with the counter incremented. The actor is
  a portal user - the population the sudo'd counter write serves in
  practice (officers/managers hold rule write; internal base users and
  GRM viewers are read-only too but do not drive ticket flow).
  Reverting the 19.0.2.0.1 sudo fix now fails loudly instead of
  passing the suite.
- Portal-read docstrings reworded: the read row is a current
  implementation dependency (rule evaluation runs as the acting user,
  reachable by portal via direct-RPC ticket create/stage-write), not a
  security requirement. Tightening it - sudo evaluation, dropping the
  read rows, and the missing portal record rule on spp.grm.ticket - is
  tracked in #413.
- HISTORY: scope the half-way claim to the counter/chatter state this
  fix addresses; notification/case partial failures are pre-existing.

* docs: regenerate READMEs from CI's pinned generator (#399)

Applied verbatim from the pre-commit CI run's printed diff (run
31679116024) - local regeneration is not byte-reproducible against
CI's hook env, so CI output is the sole authority for generated files.

* fix(spp_dci): put noupdate on the odoo root, drop deprecated data node (#399)

With the group_spp_admin record removed, the noupdate <data> wrapper
became the file's sole element, which oca-checks-odoo-module flags as
xml-deprecated-data-node (the failing 'Checks for Odoo modules' CI
hook). Same noupdate semantics, modern form.
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.

1 participant