security(dci): make OAuth token methods non-RPC and lock the token cache#269
Open
gonzalesedwin1123 wants to merge 4 commits into
Open
security(dci): make OAuth token methods non-RPC and lock the token cache#269gonzalesedwin1123 wants to merge 4 commits into
gonzalesedwin1123 wants to merge 4 commits into
Conversation
spp.dci.data.source grants base.group_user read, and get_oauth2_token()/
get_headers() were public model methods that sudo() to read the admin-only
oauth2_client_secret and RETURN a live access token / Bearer header. So any
internal user who can browse a data source could call them over RPC to mint a
token and impersonate the DCI client to the external registry. The token cache
fields (_oauth2_access_token/_oauth2_token_expires_at) had no field-group, so a
minted token was also readable by every internal user via search_read.
- Rename get_oauth2_token -> _get_oauth2_token and get_headers -> _get_headers.
Odoo rejects RPC call_kw to underscore-prefixed methods, so they are no longer
reachable via RPC while the trusted server-side DCIClient service (the public
API) still calls them internally. Callers updated (data_source, client.py).
- Restrict _oauth2_access_token/_oauth2_token_expires_at to base.group_system
and read them via sudo in _get_oauth2_token so the cache still hits.
- test_connection now requires check_access('write') so a read-only user cannot
trigger a credentialed outbound call.
The sudo() for the client secret stays (safe: the method is now internal-only).
ir.model.access.csv is unchanged: base.group_user read is required by legit
user-context DCIClient callers, and credentials + cache are now protected.
Tests: token methods are private (public names removed); the cache token field
is hidden from a regular user and unreadable; test_connection raises AccessError
for a read-only user. spp_dci_client 151/151; crvs 98/98, sr 134/134.
Post-review nits on PR (DCI OAuth token minting):
- Update the stale '_get_headers()' debug log string (was 'get_headers()').
- Clarify test_token_methods_are_private docstring (the hasattr check proves the
public entrypoints are removed; the RPC block for underscore methods is a
framework guarantee).
- Add test_action_test_connection_requires_write_access — the public button
alias inherits the check_access('write') gate. spp_dci_client 152/152.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances security by restricting access to sensitive OAuth2 token cache fields to system administrators, renaming authentication methods to private (underscore-prefixed) to prevent RPC access, and adding a write access check to connection testing. It also introduces a security test suite. The review feedback highlights critical access control issues where non-admin users will encounter AccessError exceptions: first, when clearing the token cache automatically upon receiving a 401 response, and second, when retrieving headers for bearer-authenticated data sources. The reviewer suggests using sudo() in these contexts and adding corresponding test cases to verify standard user access.
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 Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #269 +/- ##
==========================================
+ Coverage 75.31% 75.53% +0.22%
==========================================
Files 1098 1115 +17
Lines 65317 66397 +1080
==========================================
+ Hits 49191 50151 +960
- Misses 16126 16246 +120
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
CodeQL (clear-text logging, data_source.py): the cached-token info log included _oauth2_token_expires_at, a token-named cache field, tripping the sensitive-data logging heuristic. Log only the data source code (the expiry is not needed). Gemini (both HIGH — restricting the cache fields broke legit non-admin flows): - clear_oauth2_token_cache -> _clear_oauth2_token_cache (private, not RPC, so no low-priv force-remint) and write via sudo, so the 401-retry cache clear works regardless of the current user's privilege. Caller in client.py updated. - _get_headers bearer branch now reads the admin-only bearer_token via sudo (matching the OAuth2 branch), so a non-admin internal caller can build the header. Both sudos carry nosemgrep: odoo-sudo-without-context. Tests: a non-admin user context can clear the token cache and build bearer headers. spp_dci_client 154/154.
| return self._oauth2_access_token | ||
| # Do not log the token expiry field (it is a credential-adjacent | ||
| # cache field); log only the data source code. | ||
| _logger.info("Using cached OAuth2 token for data source: %s", self.code) |
Record the OAuth token-minting hardening in the module version and HISTORY changelog (regenerated README).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
spp.dci.data.sourcegrantsbase.group_userread, andget_oauth2_token()/get_headers()were public model methods thatself.sudo()to read the administrator-onlyoauth2_client_secretand return a live OAuth access token /Authorization: Bearerheader. Public Odoo model methods are callable over RPC (call_kw) by any user who can browse the record, so a low-privilege internal user could mint a token and impersonate the OpenSPP DCI client to the external registry for whatever scopes the OAuth client holds. The token cache fields (_oauth2_access_token/_oauth2_token_expires_at) had no field-levelgroups=, so a minted token was also readable by every internal user viasearch_read.Fix
get_oauth2_token→_get_oauth2_tokenandget_headers→_get_headers. Odoo's RPC dispatcher rejectscall_kwto underscore-prefixed methods (service/model.py::get_public_methodraisesAccessError), so they can no longer be invoked over RPC — while the trusted server-sideDCIClientservice (a plain Python class, the documented public API) still calls them internally. Internal callers updated (_get_headers,test_connection,services/client.py).groups="base.group_system", and read them viasudoinside_get_oauth2_tokenso the cache still hits for non-admin internal callers.test_connection(and itsaction_test_connectionbutton alias) now requireself.check_access("write"), so a read-only user cannot trigger a credentialed outbound call.The
sudo()for the client secret stays — now safe, since_get_oauth2_tokenis reachable only from trusted server-side code.Why
ir.model.access.csvis unchangedThe legit
DCIClientcaller services (sr / crvs / dr / ibr / compliance / indicators) fetch the data source in the current user's context (env[...].get_by_code(...), no sudo) and call the header method as that user. Removingbase.group_userread would break non-admin DCI lookups (and gating the methods by group/su would too). With the credentials and cached token protected by field groups and minting no longer RPC-reachable, config-metadata read is not the exposure.Tests
get_oauth2_token/get_headersnames removed).fields_get) and unreadable (read→AccessError), while an admin can see it.test_connectionandaction_test_connectionraiseAccessErrorfor a read-onlybase.group_user.DCIClientstill mint and cache tokens (existing HTTP-mocked tests, updated to the private names).spp_dci_client152/152; downstreamspp_dci_client_crvs98/98,spp_dci_client_sr134/134. Lint/semgrep clean.Reviewed
Adversarial staff review: APPROVE, no blockers. It verified the RPC block against the running Odoo 19 source, inventoried every public method (none mint/leak;
clear_oauth2_token_cacheis blocked by the write ACL), confirmed the cache fields are unreadable via all read paths, and found no stale callers/overrides repo-wide. All three review nits addressed.