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 expands the system's capabilities for Data Convergence Initiative (DCI) integration, particularly focusing on birth verification within Change Request workflows. It introduces a new demo module to illustrate DCI-powered birth verification for adding new household members, complete with auto-approval and program enrollment features. Concurrently, the API for Change Requests has been extended to provide detailed type schemas, and the DCI client has been made more flexible to accommodate different DCI endpoint implementations like OpenCRVS. Core models have also been updated to robustly track verification details. Highlights
Changelog
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 a powerful new API for Change Request type schemas and a comprehensive DCI integration demo. However, it presents significant security concerns, including a bypass in the birth verification logic in the demo module via API updates, potential IDOR vulnerabilities due to a lack of ownership checks in Change Request API endpoints, and an insecure OAuth2 configuration allowing secrets in query parameters. Additionally, a critical issue exists where a file is both gitignored and included in a manifest, which will break module installation. There's also a minor bug with a date/datetime mismatch and an inconsistency with a system parameter in the new demo module.
I am having trouble creating individual review comments. Click here to see my feedback.
spp_dci_demo/models/cr_detail_add_member.py (92-112)
The birth verification status is not invalidated when verified fields (e.g., names, birthdate) are updated via the API. The _onchange_invalidate_verification method only runs in the web client and is bypassed during write() operations performed by the API service. This allows an attacker to modify a verified Change Request with unverified data and still trigger auto-approval if the auto_approve_on_match feature is enabled.
Recommendation: Implement a write override or use @api.constrains to ensure that any modification to verified fields resets birth_verification_status to unverified and dci_data_match to False.
.gitignore (110)
This change introduces a contradiction. The file spp_dci_demo/data/dci_data_source.xml is added to .gitignore, but it is also listed in the data section of spp_dci_demo/__manifest__.py. Files listed in a manifest's data key must be present in the repository for the module to be installable. Gitignoring this file will cause ModuleNotFoundError or similar errors during installation on a clean checkout.
If this file contains sensitive credentials, it should not be in the data files. A common pattern is to include a template file (e.g., dci_data_source.xml.template) in the repository and have developers create the actual file locally, which would then be gitignored. Please resolve this contradiction.
spp_change_request_v2/strategies/add_member.py (57)
The field start_date on the spp.group.membership model is likely a Date field, not a Datetime field, based on its name. Using fields.Datetime.now() assigns a datetime object, which is incorrect. The original code using fields.Date.today() was correct. Please revert this change or use fields.Date.context_today(self) if timezone awareness is needed.
"start_date": fields.Date.today(),
spp_dci_demo/models/cr_detail_add_member.py (121)
This line attempts to read the system parameter spp_dci_demo.default_crvs_data_source. However, this parameter is not defined in any of the data files within the spp_dci_demo module. While the code correctly falls back to searching for a data source, this creates an inconsistency and might be confusing for future maintenance. Please either add the definition for this system parameter in a data file (e.g., data/system_parameters.xml) or remove this logic if the fallback search is the intended primary mechanism.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| "Birth verification for BRN %s completed with status: %s, data_match: %s", | ||
| self.birth_registration_number, | ||
| verification_status, | ||
| data_matches, |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
General approach: Avoid logging raw personal attributes (names, birthdates, gender) and instead log only high-level status or aggregated, non-identifying information. In our case, we can keep the overall “match vs mismatch” signal and the BRN identifier (if that is considered acceptable in this system), but we should not include the full textual description of every mismatch that embeds PII.
Best concrete fix in this codebase:
- In
spp_dci_demo/utils/dci_verification.py, changecheck_data_matchesso it no longer constructs mismatch strings that embed the exact CR/DCI field values. Instead, it can record only which fields mismatched, e.g."given_name","family_name","birthdate","gender". This preserves functionality (ability to see which field failed) without logging the sensitive values themselves. - Return
(matches, mismatches)as before, butmismatcheswill now be a list of field labels, not detailed value comparisons. No external callers need to change their call signatures or logic; they still treatmismatchesas a list andmatchesas a boolean. - In
spp_dci_demo/models/cr_detail_add_member.py, in_check_data_matches_dci_response, keep the existing INFO logs but they will now log an aggregated list of field names instead of sensitive values. The “data_matches” boolean logged inverify_birth_registration_with_dciremains untouched, as it is safe.
No new methods or imports are needed. All changes stay within the provided snippets and do not alter external behavior beyond what is logged.
Specifically:
-
Edit the body of
check_data_matchesinspp_dci_demo/utils/dci_verification.py:- Replace each
mismatches.append(f"... CR='{...}' vs DCI='{...}'")with something likemismatches.append("given_name"),"family_name","birthdate","gender"respectively. - Do not modify the function signature or return type.
- Replace each
-
Leave
_check_data_matches_dci_responseunchanged; it will now log non-sensitive mismatch summaries.
| @@ -143,14 +143,15 @@ | ||
| cr_given_name = (given_name or "").strip().upper() | ||
| dci_given_name = person_data["given_name"] | ||
| if cr_given_name != dci_given_name: | ||
| mismatches.append(f"given_name: CR='{cr_given_name}' vs DCI='{dci_given_name}'") | ||
| # Do not log or expose the actual name values; just note the field mismatch | ||
| mismatches.append("given_name") | ||
|
|
||
| # Compare family name | ||
| if person_data.get("family_name"): | ||
| cr_family_name = (family_name or "").strip().upper() | ||
| dci_family_name = person_data["family_name"] | ||
| if cr_family_name != dci_family_name: | ||
| mismatches.append(f"family_name: CR='{cr_family_name}' vs DCI='{dci_family_name}'") | ||
| mismatches.append("family_name") | ||
|
|
||
| # Compare birth date | ||
| if person_data.get("birth_date") and birthdate: | ||
| @@ -158,13 +153,13 @@ | ||
| dci_birthdate = person_data["birth_date"] | ||
| # Handle different date formats (YYYY-MM-DD) | ||
| if cr_birthdate[:10] != dci_birthdate[:10]: | ||
| mismatches.append(f"birthdate: CR='{cr_birthdate}' vs DCI='{dci_birthdate}'") | ||
| mismatches.append("birthdate") | ||
|
|
||
| # Compare gender/sex | ||
| if person_data.get("sex") and gender_display: | ||
| cr_gender = gender_display.lower() | ||
| dci_sex = person_data["sex"].lower() | ||
| if cr_gender != dci_sex: | ||
| mismatches.append(f"gender: CR='{cr_gender}' vs DCI='{dci_sex}'") | ||
| mismatches.append("gender") | ||
|
|
||
| return (len(mismatches) == 0, mismatches) |
| _logger.info( | ||
| "DCI data mismatch for BRN %s: %s", | ||
| self.birth_registration_number, | ||
| "; ".join(mismatches), |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general, the problem should be fixed by avoiding logging raw sensitive field values. Logs can still indicate that a mismatch occurred and which fields disagreed, but should not include the actual personal data values from either the CR or DCI side. This keeps logs useful for debugging (you know what mismatched and for which BRN) while avoiding exposure of PII such as name, birthdate, and gender.
The best targeted fix here is to change check_data_matches so that it no longer embeds the actual values into the mismatches strings. Instead, it should only record which field mismatched (e.g., "given_name mismatch", "birthdate mismatch", "gender mismatch"). Since _check_data_matches_dci_response only ever logs "; ".join(mismatches), scrubbing the contents of mismatches ensures that no sensitive data is logged, while leaving the surrounding logging calls and their semantics intact. We do not need to change the method signature or its boolean matches behavior, so existing callers continue to function.
Concretely:
- In
spp_dci_demo/utils/dci_verification.py, incheck_data_matches, replace eachmismatches.append(...)that interpolates CR/DCI values with a generic message that does not contain those values. - Optionally, we can include a generic indication that both CR and DCI values were present but did not match, without specifying them.
- No changes are needed to
_check_data_matches_dci_responseincr_detail_add_member.pyother than benefiting from the sanitized mismatch messages.
No new imports or helper methods are required; we are only changing string literals.
| @@ -143,14 +143,16 @@ | ||
| cr_given_name = (given_name or "").strip().upper() | ||
| dci_given_name = person_data["given_name"] | ||
| if cr_given_name != dci_given_name: | ||
| mismatches.append(f"given_name: CR='{cr_given_name}' vs DCI='{dci_given_name}'") | ||
| # Do not include actual name values in logs; just record that a mismatch occurred. | ||
| mismatches.append("given_name mismatch between CR and DCI data") | ||
|
|
||
| # Compare family name | ||
| if person_data.get("family_name"): | ||
| cr_family_name = (family_name or "").strip().upper() | ||
| dci_family_name = person_data["family_name"] | ||
| if cr_family_name != dci_family_name: | ||
| mismatches.append(f"family_name: CR='{cr_family_name}' vs DCI='{dci_family_name}'") | ||
| # Do not include actual family name values in logs. | ||
| mismatches.append("family_name mismatch between CR and DCI data") | ||
|
|
||
| # Compare birth date | ||
| if person_data.get("birth_date") and birthdate: | ||
| @@ -158,13 +154,15 @@ | ||
| dci_birthdate = person_data["birth_date"] | ||
| # Handle different date formats (YYYY-MM-DD) | ||
| if cr_birthdate[:10] != dci_birthdate[:10]: | ||
| mismatches.append(f"birthdate: CR='{cr_birthdate}' vs DCI='{dci_birthdate}'") | ||
| # Do not include actual birth dates in logs. | ||
| mismatches.append("birthdate mismatch between CR and DCI data") | ||
|
|
||
| # Compare gender/sex | ||
| if person_data.get("sex") and gender_display: | ||
| cr_gender = gender_display.lower() | ||
| dci_sex = person_data["sex"].lower() | ||
| if cr_gender != dci_sex: | ||
| mismatches.append(f"gender: CR='{cr_gender}' vs DCI='{dci_sex}'") | ||
| # Do not include actual gender/sex values in logs. | ||
| mismatches.append("gender/sex mismatch between CR and DCI data") | ||
|
|
||
| return (len(mismatches) == 0, mismatches) |
Features: - Birth verification via OpenCRVS DCI integration - Auto-approval when DCI data matches CR detail fields - Auto-enrollment of household in configured program on CR apply - Add Child wizard for streamlined UX - Verified BRN registry ID created on apply Technical: - Add search_by_id_opencrvs method for OpenCRVS-specific format - Extract DCI verification logic to utils module - Add system parameters for configuration - Add post_init_hook for auto-configuration Note: DCI data source credentials must be configured manually via Settings > Technical > System Parameters or UI.
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.
- 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.
…d validation gaps - Fix pagination URLs not being URL-encoded (broken links with pipe/colon chars) - Raise ValidationError on unresolved vocabulary/partner lookups instead of silently skipping - Add detail input validation against type schema (unknown, internal, readonly fields) - Fix manifest: PATCH→PUT, add missing $request-revision and $reset endpoints - Remove unused ChangeRequestAction and ChangeRequestSearchParams schemas - Remove unnecessary hasattr check for revision_notes (always on mixin) - Add comments explaining why _do_reject/_do_request_revision are called directly - Extract shared test fixtures into tests/common.py ChangeRequestTestCase - Add tests for reject/approve wrong state, unknown fields, unresolved vocab, readonly fields
…exists Add computed single_dci_data_source field to auto-hide the DCI data source dropdown when there is zero or one active Civil registry, removing an unnecessary selection step from the UI.
…after_submit The DCI demo wizard adds no value over the standard CR flow now that detail forms have Submit buttons. Remove the wizard and its tests, and remove the duplicated _try_auto_approve() from the detail model. Auto- approval now only happens via the _after_submit() hook on the CR model.
Restrict sensitive PII fields in API logs to a dedicated group_api_v2_auditor group. Regular viewers can see log metadata (timestamp, endpoint, status, duration) but not payloads or error details. - Add privilege_api_v2_auditor and group_api_v2_auditor - Auditor implies Viewer; Admin implies Auditor - Restrict outgoing log: request_summary, response_summary, error_detail - Restrict audit log: search_parameters, fields_returned, extensions_returned, ip_address, user_agent, error_detail - Hide payload pages and error_detail in outgoing log form view - Add field-level security tests for both models
Wire up the spp.api.outgoing.log model with manifest registration, ACL rules (viewer read-only, officer/manager read+write), the OutgoingApiLogService wrapper with payload truncation and error resilience, and an "Outgoing API Logs" menu item under API V2.
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.
…emo household - Add Masters household (Adam + Mary) as demo data for DCI CR flow - Hide Contact Information and Relationship fields in add-member form - Target Conditional Child Grant program in post_init_hook - Add Conditional Child Grant program with first-1,000-days eligibility - Add Health Visit event type for compliance tracking - Configure compliance manager with CEL expression support
…pe schema with JSON Schema 2020-12 Add generic OdooModelSchemaBuilder that converts any Odoo model's fields to a standard JSON Schema 2020-12 document. Refactor CR type schema endpoint to return detailSchema (JSON Schema) instead of proprietary FieldDefinition objects, enabling third-party tooling (ajv, jsonschema, react-jsonschema-form) to work out of the box. Key changes: - Add spp_api_v2/services/schema_builder.py with field type mapping, vocabulary extraction, and selection choice handling - Replace FieldDefinition/VocabularyInfo pydantic models with a plain dict[str, Any] detailSchema field on ChangeRequestTypeSchema - Optimize _validate_detail_input to use direct field introspection instead of building full schema (avoids unnecessary DB queries) - Use anyOf for many2one reference types (2020-12 conformance)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 19.0 #36 +/- ##
==========================================
- Coverage 71.31% 68.30% -3.02%
==========================================
Files 299 389 +90
Lines 23618 31038 +7420
==========================================
+ Hits 16844 21200 +4356
- Misses 6774 9838 +3064
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Why is this change needed?
How was the change implemented?
New unit tests
Unit tests executed by the author
How to test manually
Related links