Skip to content

Restrict DCI sync and cached value access#234

Merged
gonzalesedwin1123 merged 3 commits into
19.0from
codex/propose-fix-for-dci-sync-vulnerability
Jun 26, 2026
Merged

Restrict DCI sync and cached value access#234
gonzalesedwin1123 merged 3 commits into
19.0from
codex/propose-fix-for-dci-sync-vulnerability

Conversation

@jeremi

@jeremi jeremi commented Jun 10, 2026

Copy link
Copy Markdown
Member

Motivation

  • The manual server action and the sync_for_partners() entrypoint allowed arbitrary internal users to trigger outbound DCI lookups and populate sensitive disability/health indicators into the shared cache spp.data.value without authorization.
  • Cached DCI-backed indicators (e.g. dr.dci.has_disability, severity flags) are sensitive and were readable by base.group_user via existing ACLs, creating a confidentiality risk.
  • The sync cron also ran without an explicit privileged user_id, making scheduled execution ambiguous and potentially privileged operations less explicit.

Description

  • Added a service-layer authorization check _check_dci_sync_access() to spp.dci.cel.fetcher which raises AccessError unless the caller has the spp_cel_domain.group_cel_domain_manager group, and invoked it at the start of sync_for_partners() to block low-privileged RPC/server calls.
  • Restricted the bound server action action_sync_dci_values to CEL Domain Managers by adding a group_ids entry and set the scheduled cron user_id to base.user_root so scheduled syncs remain explicit and privileged.
  • Removed the broad base.group_user read permission for model_spp_data_value in spp_cel_domain/security/ir.model.access.csv (set read to 0) so cached DCI values are no longer globally visible to basic internal users.
  • Added a regression test that a plain internal user (only base.group_user) receives AccessError when calling sync_for_partners() directly (test_sync_for_partners_requires_cel_manager).

Testing

  • Ran python3 -m py_compile on the modified files (spp_dci_indicators/models/dci_cel_fetcher.py and tests) and compilation succeeded.
  • Parsed the updated XML (spp_dci_indicators/data/dci_sync.xml) with xml.etree.ElementTree.parse and performed a static ACL check on spp_cel_domain/security/ir.model.access.csv, both of which succeeded.
  • Ran git diff --check and static checks with no reported issues.
  • Attempted to run the Odoo TransactionCase test suite but import odoo failed in this environment so full integration/unit test execution could not be performed here; a new unit test was added and should run in CI where Odoo is available.

Codex Task

@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 introduces access control restrictions for syncing DCI-backed CEL values. It disables default read access to spp.data.value for base users, restricts the sync action to CEL Domain Managers, and adds a permission check in sync_for_partners along with a corresponding unit test. Feedback on the changes suggests updating the access check in _check_dci_sync_access to bypass the group check when running in superuser mode (self.env.su), preventing unexpected AccessError exceptions during sudo() operations.

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.

Comment thread spp_dci_indicators/models/dci_cel_fetcher.py
@codecov

codecov Bot commented Jun 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.60%. Comparing base (8d722ef) to head (561de1c).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             19.0     #234      +/-   ##
==========================================
+ Coverage   74.47%   74.60%   +0.13%     
==========================================
  Files        1062     1077      +15     
  Lines       63833    64319     +486     
==========================================
+ Hits        47540    47987     +447     
- Misses      16293    16332      +39     
Flag Coverage Δ
spp_analytics 93.13% <ø> (ø)
spp_api_v2_change_request 66.85% <ø> (ø)
spp_api_v2_cycles 71.12% <ø> (ø)
spp_api_v2_data 64.41% <ø> (ø)
spp_api_v2_entitlements 70.19% <ø> (ø)
spp_api_v2_gis 84.27% <ø> (ø)
spp_api_v2_programs 92.03% <ø> (?)
spp_api_v2_simulation 71.12% <ø> (ø)
spp_approval 50.29% <ø> (ø)
spp_audit_programs 0.00% <ø> (?)
spp_base_common 90.26% <ø> (ø)
spp_case_cel 89.01% <ø> (ø)
spp_case_entitlements 97.61% <ø> (ø)
spp_case_programs 97.14% <ø> (ø)
spp_cel_domain 61.93% <ø> (ø)
spp_cel_event 85.23% <ø> (ø)
spp_dci_indicators 95.61% <100.00%> (+0.15%) ⬆️
spp_programs 65.12% <ø> (ø)
spp_registry 86.83% <ø> (ø)
spp_security 66.66% <ø> (ø)

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

Files with missing lines Coverage Δ
spp_dci_indicators/models/dci_cel_fetcher.py 95.26% <100.00%> (+0.23%) ⬆️

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

jeremi and others added 2 commits June 26, 2026 15:22
Add an explicit superuser early-return to _check_dci_sync_access so the
scheduled sync cron (user_id=base.user_root, su mode) passes its own gate,
mirroring spp_cel_domain data_credential.py. Add positive tests for the
CEL-manager allow path and the su/cron bypass.
@gonzalesedwin1123 gonzalesedwin1123 force-pushed the codex/propose-fix-for-dci-sync-vulnerability branch from ea59ead to ee403cf Compare June 26, 2026 07:31
@gonzalesedwin1123

Copy link
Copy Markdown
Member

Follow-up: a second outbound-fetch route is not covered by this gate

This PR gates sync_for_partners() (and the bound server action / cron), which closes the explicit-sync route. While validating the change against the current 19.0 base, I noticed a second, pre-existing path to the same outbound DCI fetch that _check_dci_sync_access() does not cover — flagging it here as a follow-up rather than expanding this PR's scope.

The path:

spp.data.cache.manager._compute_variable_values()   # data_cache_manager_dci.py
    -> spp.dci.cel.fetcher.fetch_values()            # outbound DCI call + cache write

_compute_variable_values (the DCI override) calls fetch_values() directly, with no access check and as the calling user (no sudo). It is reached via precompute_variable() in spp_cel_domain/models/data_evaluator.py (CEL precompute / refresh), which is independent of the now-gated sync_for_partners(). So anything that triggers a precompute/refresh of a DCI-backed variable performs the same outbound lookup and caches the same sensitive disability/health values — bypassing the new gate.

Exploitability is unconfirmed. I found no button/action/RPC exposing precompute_variable to non-managers; its callers are internal CEL precompute/refresh orchestration (e.g. eligibility runs, refresh cycles). Whether a low-privileged user can actually trigger those for DCI-backed variables needs a deeper trace.

Suggested follow-up:

  • Trace the precompute / eligibility-evaluation callers to confirm whether a non–CEL-manager can reach this path.
  • If so, apply the same authorization at the fetch boundary — e.g. call _check_dci_sync_access() (or an equivalent self.env.su-aware check) inside the DCI branch of _compute_variable_values / fetch_values, so both routes are gated consistently.

Note: the new sr.dci.* fetch handlers added on 19.0 use this same fetch_values route, so they would be covered by the same fix — they do not introduce a separate entrypoint.

CI pre-commit prettier (plugin-xml) requires the long group_ids server-action
field to be wrapped across lines, matching the file's existing field style.
@gonzalesedwin1123 gonzalesedwin1123 merged commit 8b98b78 into 19.0 Jun 26, 2026
35 checks passed
@gonzalesedwin1123 gonzalesedwin1123 deleted the codex/propose-fix-for-dci-sync-vulnerability branch June 26, 2026 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants