feat(spp_dci_client): DCI client improvements and audit trail logging#39
Conversation
Summary of ChangesHello @jeremi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the DCI client by broadening access for non-admin users to DCI verification, introducing a system-level approval mechanism for automated processes, and establishing a robust audit trail for all outgoing DCI API requests. The changes improve both functionality and accountability, ensuring that all external communications are logged for review and troubleshooting, while also making the system more flexible for automated workflows. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces significant improvements, including the addition of action_approve_system for programmatic approvals, a fix allowing non-admin users to trigger DCI verification via sudo() for credential access, and robust audit trail logging for all outgoing DCI requests, complemented by comprehensive integration tests. However, two critical security issues were identified: a potential bypass of the approval workflow in ApprovalMixin and the exposure of cached OAuth2 access tokens to non-admin users in the DCIDataSource model. Addressing these vulnerabilities is crucial to ensure system security. Additionally, consider minor suggestions in spp_dci_client/services/client.py for more specific exception handling.
| def action_approve_system(self, comment=None): | ||
| """System-initiated approval bypassing user permission checks. | ||
|
|
||
| Use this for automated approvals triggered by system events (e.g., DCI | ||
| verification match, scheduled jobs) where there is no human approver. | ||
|
|
||
| This method uses sudo() to bypass access controls and skips the | ||
| _check_can_approve() permission validation. | ||
|
|
||
| Args: | ||
| comment: Optional approval comment for audit trail | ||
| """ | ||
| for record in self: | ||
| if record.approval_state != "pending": | ||
| _logger.warning( | ||
| "Skipping system approval for %s %s: state is %s, not pending", | ||
| record._name, | ||
| record.id, | ||
| record.approval_state, | ||
| ) | ||
| continue | ||
| record.sudo()._do_approve(comment=comment) | ||
| _logger.info( | ||
| "System auto-approved %s %s: %s", | ||
| record._name, | ||
| record.id, | ||
| comment or "(no comment)", | ||
| ) |
There was a problem hiding this comment.
The action_approve_system method allows bypassing all approval authorization checks. Since it starts with the action_ prefix and lacks any caller verification, it is exposed to Odoo's RPC interface. Any user with 'write' access to a record using this mixin can call this method to approve it, even if they are not an authorized approver. This method explicitly uses sudo() and skips the _check_can_approve() validation, which is intended for system-initiated approvals but creates a significant security risk if exposed. Consider renaming it to start with an underscore (e.g., _action_approve_system) to prevent RPC exposure, or adding a check to ensure it is only called in a superuser context (e.g., if not self.env.su: raise AccessError(...)).
spp_dci_client/services/client.py
Outdated
| log_error_detail = "401 Unauthorized - retrying with fresh token" | ||
| try: | ||
| log_response_data = response.json() | ||
| except Exception: |
There was a problem hiding this comment.
Catching a broad Exception can hide unexpected errors. It's better to catch the specific exception that response.json() might raise, which is json.JSONDecodeError. This makes the code more robust and the intent clearer. Note that you'll need to add import json at the top of the file for this change.
| except Exception: | |
| except json.JSONDecodeError: |
spp_dci_client/services/client.py
Outdated
| technical_detail += f": {error_data['header']['status_reason_message']}" | ||
| else: | ||
| technical_detail += f": {response_text}" | ||
| except Exception: |
There was a problem hiding this comment.
Similar to my other comment, it's better to catch specific exceptions here. The try block performs JSON decoding and dictionary lookups. Catching (json.JSONDecodeError, KeyError, TypeError) would be more precise than a broad Exception and would make debugging easier if an unexpected error occurs. Note that you'll need to add import json at the top of the file for this change.
| except Exception: | |
| except (json.JSONDecodeError, KeyError, TypeError): |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| # Cache token with expiry (use sudo to write to restricted model) | ||
| expires_at = now + timedelta(seconds=expires_in) | ||
| self.write( | ||
| sudo_self.write( |
There was a problem hiding this comment.
Missing sudo in token cache clearing breaks non-admin 401 retry
Medium Severity
The get_oauth2_token method now correctly uses sudo_self.write() to cache the token for non-admin users, but clear_oauth2_token_cache() still calls self.write() without sudo(). Per ir.model.access.csv, non-admin users have read-only access to spp.dci.data.source (perm_write=0). When a non-admin user hits a 401 response in _make_request, the call to self.data_source.clear_oauth2_token_cache() raises an AccessError, preventing the token-refresh retry from ever executing. Before this PR, non-admin users couldn't reach this code path at all (they'd fail at the credential read), so the sudo changes make this a newly reachable failure.
jeremi
force-pushed
the
feat/api-outgoing-log
branch
from
ff1b282 to
6438991
Compare
- Use sudo() when accessing OAuth2 credentials in data source - Use sudo() when caching OAuth2 token (write to restricted model) - Add _after_submit() hook to auto-approve CR on submit if DCI verified This enables users like demo_officer to use DCI birth verification without requiring administrator privileges.
Add a dedicated method for system-initiated approvals that bypasses user permission checks. This enables automated approval workflows triggered by system events (e.g., DCI verification match) where there is no human approver. Also adds security control to invalidate verification when verified fields (name, DOB, gender, BRN) are edited after verification.
Instrument _make_request to log all outgoing calls via spp.api.outgoing.log (soft dependency). Logs persist in a separate cursor so they survive transaction rollback on UserError. Captures timing, status codes, request/response payloads, and error details for success, HTTP errors, connection errors, timeouts, and 401 retries.
…oval and narrow exception handling Rename action_approve_system to _action_approve_system to prevent Odoo from exposing it via the RPC interface, since it uses sudo() and skips _check_can_approve() authorization checks. Replace broad Exception catches in spp_dci_client with specific exception types (json.JSONDecodeError, KeyError, TypeError) and add json import.
…d fix test log ordering Pass auto=True to _do_approve in _action_approve_system so automated approvals show "(auto)" in activity feedback and skip submitter notification, matching the intent of system-initiated approvals. Fix reversed log assertions in test_401_retry_creates_two_log_entries: due to try-finally semantics with recursive calls, the inner retry's finally creates the success log first (lower ID), so with order="id desc" logs[0] is the 401 entry and logs[1] is the success entry.
jeremi
force-pushed
the
feat/dci-client-improvements
branch
from
db3b9a4 to
3ea418e
Compare
emjay0921
left a comment
emjay0921
left a comment
There was a problem hiding this comment.
Code review complete. Clean implementation across all three changes. No issues found.
Review NotesCode looks good overall — the separate-cursor logging pattern, soft dependency on
|
3 participants
Summary
action_approve_system()tospp_approvalmixin for automated/programmatic approvalsdci_data_source.xmlto.gitignoreDependencies
feat/api-outgoing-log) for the outgoing log modelTest plan
test_single_module.sh spp_dci_clienttest_single_module.sh spp_approvalNote
Medium Risk
Introduces
sudo()paths and a new approval bypass method, plus adds persistent logging in the DCI request finally-block; mistakes here could impact access control expectations or create noisy/large audit logs.Overview
Adds a new
ApprovalMixin._action_approve_system()helper to allow system-initiated approvals that bypass user permission checks (viasudo()) and only runs when the record ispending.Updates DCI OAuth2 token fetching to read/write admin-restricted credentials and token cache using
sudo(), enabling non-admin flows that need tokens.Extends
DCIClient._make_request()to always emit an outgoing API audit log (soft-depending onspp_api_v2) with duration, status/status_code, response payload (including 401 pre-retry), and error detail, using a separate cursor so logs persist across rollbacks; adds a comprehensive integration test suite covering success, retries, and error cases.Written by Cursor Bugbot for commit 3ea418e. This will update automatically on new commits. Configure here.