WEB-600 feat:External National ID System integration for client creat…#3145
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Configuration & Environment angular.json, src/environments/environment.ts, src/environments/environment.prod.ts, src/typings.d.ts, src/assets/env.js, src/assets/env.template.js, env.sample |
Added five new external national ID system properties (enableExternalNationalIdSystem, externalNationalIdSystemUrl, externalNationalIdSystemApiHeader, externalNationalIdSystemApiKey, externalNationalIdRegex) across all environment and configuration files; angular.json adds proxyConfig pointing to proxy.conf.js. |
Proxy & Reverse Proxy Infrastructure proxy.conf.js, proxy.localhost.conf.js, nginx.conf.template, docker-compose.yml, docker-compose.external-nationalid.yml |
Configured proxy routing for external national ID API with path rewriting, header injection (X-Gravitee-Api-Key), error handling, and onProxyReq/onError callbacks in both dev proxy files; nginx template implements rate limiting (2r/s per IP, burst 5), resolver configuration, and request/response header manipulation; removed MIFOS_PRODUCTION_MODE environment variable from docker-compose.yml; added override docker-compose file for external national ID system integration. |
Backend Service Layer src/app/clients/clients.service.ts |
Added lookupExternalNationalId() method that bypasses interceptors via dedicated HttpBackend, validates environment configuration, injects API headers, and proxies requests to external national ID system. |
External National ID Service & Constants src/app/clients/services/external-national-id.service.ts, src/app/clients/services/external-national-id.constants.ts |
Introduced new ExternalNationalIdService for managing external ID validation, API lookups, debounced form changes, field auto-fill/disable on success, and status message handling; companion constants file provides strongly-typed status keys, gender mappings, and field name references. |
Client Creation UI Component src/app/clients/client-stepper/client-general-step/client-general-step.component.ts, src/app/clients/client-stepper/client-general-step/client-general-step.component.html, src/app/clients/client-stepper/client-general-step/client-general-step.component.scss |
Integrated ExternalNationalIdService via dependency injection and watchExternalId call; replaced form.contains() checks with form.get() for name fields; added status hint display with dynamic styling (loading, success, error states) below External ID field. |
Client Edit UI Component src/app/clients/edit-client/edit-client.component.ts, src/app/clients/edit-client/edit-client.component.html, src/app/clients/edit-client/edit-client.component.scss |
Integrated ExternalNationalIdService with skipInitialValue flag for edit mode; added status hint UI and corresponding styling; updated name field presence checks from contains() to get(). |
Translations & Documentation src/assets/translations/en-US.json, README.md |
Added six translation keys for external ID status messages (not found, lookup failed, invalid format, verified successfully, loading, timed out); documentation section describing feature behavior, configuration variables, and docker-compose usage. |
Sequence Diagram
sequenceDiagram
participant Client as Client (Browser)
participant Component as Client Component
participant Service as ExternalNationalIdService
participant ClientService as ClientsService
participant Proxy as Dev Proxy/Nginx
participant API as External National ID API
Client->>Component: Enter External ID
Component->>Service: watchExternalId (observes form changes)
Service->>Service: Validate ID against regex
Service->>Service: Emit loading state
Component->>Component: Render loading indicator
Service->>ClientService: lookupExternalNationalId(id)
ClientService->>ClientService: Build request headers<br/>(inject API key)
ClientService->>Proxy: HTTP POST /external-nationalid
Proxy->>Proxy: Rate limit check (2r/s)
Proxy->>Proxy: Rewrite path to /1.0/nationalid
Proxy->>Proxy: Inject X-Gravitee-Api-Key header
Proxy->>API: Forward request
API-->>Proxy: Return person details<br/>(name, DOB, gender)
Proxy-->>ClientService: Response with metadata
ClientService-->>Service: Observable<response>
Service->>Service: Parse date, map gender
Service->>Component: Fill form fields
Service->>Component: Set success status
Service->>Component: Disable name/DOB/gender fields
Component->>Client: Render success message<br/>& auto-filled form
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
- WEB-605 Change the vNEXT to MIFOS_INTERBANK_TRANSFERS variable and se… #3016: Modifies environment configuration surface (env.js/env.template.js, environment.ts files, typings) by adding external national ID system properties, paralleling this PR's environment expansion.
- WEB-547 feat(proxy): forward fineract provider requests to sandbox for local dev #2958: Updates proxy configuration files (proxy.conf.js, proxy.localhost.conf.js) with proxy route handlers and onProxyReq/onError logic for API integration, similar infrastructure pattern to this PR.
- WEB-563 feat(login): modernize login UI with Material Design 3 and pr… #3061: Affects docker-compose.yml environment configuration (MIFOS_PRODUCTION_MODE flag), directly related to this PR's docker-compose modifications.
Suggested reviewers
- IOhacker
- adamsaghy
🚥 Pre-merge checks | ✅ 3 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Merge Conflict Detection | ❌ Merge conflicts detected (20 files): ⚔️ README.md (content)⚔️ angular.json (content)⚔️ docker-compose.yml (content)⚔️ env.sample (content)⚔️ proxy.conf.js (content)⚔️ proxy.localhost.conf.js (content)⚔️ src/app/clients/client-stepper/client-general-step/client-general-step.component.html (content)⚔️ src/app/clients/client-stepper/client-general-step/client-general-step.component.scss (content)⚔️ src/app/clients/client-stepper/client-general-step/client-general-step.component.ts (content)⚔️ src/app/clients/clients.service.ts (content)⚔️ src/app/clients/edit-client/edit-client.component.html (content)⚔️ src/app/clients/edit-client/edit-client.component.scss (content)⚔️ src/app/clients/edit-client/edit-client.component.ts (content)⚔️ src/app/tasks/checker-inbox-and-tasks-tabs/loan-disbursal/loan-disbursal.component.ts (content)⚔️ src/assets/env.js (content)⚔️ src/assets/env.template.js (content)⚔️ src/assets/translations/en-US.json (content)⚔️ src/environments/environment.prod.ts (content)⚔️ src/environments/environment.ts (content)⚔️ src/typings.d.ts (content)These conflicts must be resolved before merging into dev. |
Resolve conflicts locally and push changes to this branch. |
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly summarizes the main change: implementing external national ID system integration for client creation/editing workflows, directly matching the PR's primary objective. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
⚔️ Resolve merge conflicts (beta)
- Auto-commit resolved conflicts to branch
feature/WEB-600-external-national-id-integration - Post resolved changes as copyable diffs in a comment
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@Dockerfile`:
- Around line 45-51: The CMD's envsubst whitelist is missing several variables
used by nginx.conf.template, so update the command that runs envsubst (the
existing CMD which pipes default.conf.template to
/etc/nginx/conf.d/default.conf) to include EXTERNAL_NATIONALID_API_URL,
EXTERNAL_NATIONAL_ID_SYSTEM_API_HEADER, and EXTERNAL_NATIONAL_ID_SYSTEM_API_KEY
in addition to FINERACT_API_URL and EXTERNAL_NATIONAL_ID_SYSTEM_URL so all
referenced placeholders are substituted at container startup by envsubst.
In `@nginx.conf.template`:
- Around line 1-59: The Dockerfile's envsubst call is missing variables used in
nginx.conf.template, causing literals like ${EXTERNAL_NATIONALID_API_URL},
${EXTERNAL_NATIONAL_ID_SYSTEM_API_HEADER}, and
${EXTERNAL_NATIONAL_ID_SYSTEM_API_KEY} to remain unexpanded; update the second
envsubst invocation in the Dockerfile (the explicit envsubst line) to include
FINERACT_API_URL, EXTERNAL_NATIONALID_API_URL,
EXTERNAL_NATIONAL_ID_SYSTEM_API_HEADER, and EXTERNAL_NATIONAL_ID_SYSTEM_API_KEY
so the template variables in nginx.conf.template are properly substituted at
container start.
- Around line 20-47: The nginx template uses ${EXTERNAL_NATIONALID_API_URL},
${EXTERNAL_NATIONAL_ID_SYSTEM_API_HEADER}, and
${EXTERNAL_NATIONAL_ID_SYSTEM_API_KEY} but those vars aren’t added to the
envsubst whitelist in the Dockerfile, and the /external-nationalid location is
unconditionally included; update the Dockerfile’s envsubst command to include
these three variable names so they are substituted at container start, and wrap
the entire location /external-nationalid block in a conditional controlled by
ENABLE_EXTERNAL_NATIONAL_ID_SYSTEM (or remove the block unless that flag is
true) so the config and proxy_pass/header injection only exist when the feature
is enabled.
In `@proxy.conf.js`:
- Around line 48-62: The onProxyReq handler logs req.url which may contain PII;
update the onProxyReq function to avoid printing raw National ID values by
stripping the query string and/or path segments that may contain IDs before
logging: compute a safe path (e.g., use rewrittenPath without query or replace
query portion with '?[redacted]' or mask last path segment) and log req.method
plus this sanitized path instead of req.url; reference the onProxyReq function,
proxyReq.setHeader, req.url and rewrittenPath when making the change.
In `@proxy.localhost.conf.js`:
- Around line 45-58: The proxy currently logs raw req.url (which can contain
PII) inside the onProxyReq handler; change the log to avoid printing full URLs
or query strings by logging only non-sensitive parts (e.g., req.method and the
rewrittenPath or a sanitized path with query stripped) instead of req.url, and
ensure onProxyReq uses rewrittenPath (or a sanitized version derived from
req.url without query/identifier segments) when calling console.log; keep the
API key injection logic (process.env.EXTERNAL_NATIONAL_ID_SYSTEM_API_KEY and
proxyReq.setHeader) untouched.
In `@src/app/clients/services/external-national-id.service.ts`:
- Around line 211-224: The current truthy checks (response.firstname,
response.middlename, response.lastname) skip setting form controls when the API
returns an empty string, leaving stale values in
firstnameCtrl/middlenameCtrl/lastnameCtrl; change the guards to test for
undefined/null (e.g., response.firstname !== undefined && response.firstname !==
null) so empty strings from the API will overwrite existing values (and still
disable the controls), or if the intent is to only overwrite non-empty values,
add a short comment above these blocks documenting that design decision; refer
to firstnameCtrl, middlenameCtrl, lastnameCtrl and
response.firstname/response.middlename/response.lastname when making the change.
- Around line 78-92: Update the misleading comment in the constructor to reflect
that compiling once reduces overhead but does not mitigate ReDoS, and implement
a basic safety check when reading environment.externalNationalIdRegex in the
constructor: scan the pattern string for common dangerous constructs (e.g.,
nested/adjacent quantifiers or group-plus-quantifier patterns like (.+)+, (a+)+,
or back-to-back quantifiers) and if detected reject the pattern (set
this.pattern = null) and log a clear warning; keep the existing try/catch for
invalid regex syntax and ensure isValidExternalId uses this.pattern safely (no
further compilation) so the code references include constructor,
environment.externalNationalIdRegex, this.pattern, and isValidExternalId.
In `@src/assets/env.template.js`:
- Around line 106-112: Remove the client-side exposure of the external National
ID API header and key by deleting the
window['env']['externalNationalIdSystemApiHeader'] and
window['env']['externalNationalIdSystemApiKey'] template entries from
src/assets/env.template.js, and ensure the frontend (ClientsService in
src/app/clients/clients.service.ts) continues to handle their absence by relying
on the existing conditional check around
environment.externalNationalIdSystemApiHeader and
environment.externalNationalIdSystemApiKey when building request headers;
confirm no other code reads those env vars and add a brief comment in
ClientsService noting that authentication headers are injected by the
server/nginx proxy so frontend secrets must not be present.
In `@src/environments/environment.ts`:
- Line 93: The property enableExternalNationalIdSystem only checks for the
string 'true' and is inconsistent with other boolean flags; update its
expression to accept both the string and boolean true (e.g., change the value to
check loadedEnv.enableExternalNationalIdSystem === 'true' ||
loadedEnv.enableExternalNationalIdSystem === true || false) so it matches the
pattern used by complianceHideClientData and productionModeEnableRBAC.
🧹 Nitpick comments (5)
env.sample (1)
54-66: Inconsistent env var naming:EXTERNAL_NATIONALID_API_URLvsEXTERNAL_NATIONAL_ID_SYSTEM_*.All other variables in this block use the
EXTERNAL_NATIONAL_ID_SYSTEM_prefix (with underscores between NATIONAL, ID, and SYSTEM), but line 66 usesEXTERNAL_NATIONALID_API_URL. This inconsistency can confuse operators during deployment.Consider renaming to
EXTERNAL_NATIONAL_ID_SYSTEM_API_URLfor consistency, updating references innginx.conf.template,docker-compose.yml, andDockerfileaccordingly.src/app/clients/client-stepper/client-general-step/client-general-step.component.html (1)
42-54: Extract success status into a service getter to avoid hardcoded string duplication.Lines 47–48 compare
statusMessageKeyagainst the literal string'External ID verified successfully'. The same pattern is duplicated inedit-client.component.html. The constantEXTERNAL_ID_STATUS_KEYS.SUCCESSalready exists insrc/app/clients/services/external-national-id.constants.ts; reference it instead of the magic string.Expose a computed boolean from
ExternalNationalIdService:// In ExternalNationalIdService get isSuccess(): boolean { return this.statusMessageKey === EXTERNAL_ID_STATUS_KEYS.SUCCESS; }Then update both templates:
success: externalNationalIdService.isSuccess, error: !externalNationalIdService.isSuccesssrc/app/clients/clients.service.ts (1)
456-474: Return type should beObservable<ExternalNationalIdResponse>instead ofObservable<any>.The response interface
ExternalNationalIdResponseis already defined inexternal-national-id.service.ts. Importing and using it here would provide type safety at the call site and align with the coding guidelines for strict type safety.♻️ Suggested fix
Import at the top of the file:
import { ExternalNationalIdResponse } from './services/external-national-id.service';Then update the method signature:
- lookupExternalNationalId(externalId: string): Observable<any> { + lookupExternalNationalId(externalId: string): Observable<ExternalNationalIdResponse> {Note: Be mindful of circular dependency if
ExternalNationalIdServiceimportsClientsService— you may need to extract the interface into a separate file (e.g.,external-national-id.models.ts).As per coding guidelines,
src/app/**: verify strict type safety.src/app/clients/services/external-national-id.constants.ts (1)
65-68: Hardcoded external gender IDs are fragile — consider a comment or configuration note.The magic numbers
36and37are tightly coupled to the external API's gender ID scheme. If the external system is ever updated or a different external provider is used, this will silently break gender mapping. The inline comment helps, but consider whether this mapping should be configurable (e.g., via environment) for deployments targeting different external ID systems.src/app/clients/services/external-national-id.service.ts (1)
254-284:parseDateonly splits on'/'regardless of the format string.If the external API returns a date with a different delimiter (e.g.,
"15-03-1990"withdateFormat: "dd-MM-yyyy"), thesplit('/')on line 260 will produce a single-element array and fall through to nativeDateparsing, which is locale-dependent and unreliable.♻️ Suggested improvement — extract delimiter from format
parseDate(dateStr: string, dateFormat?: string): Date | null { if (!dateStr) { return null; } const format = dateFormat || 'dd/MM/yyyy'; - const parts = dateStr.split('/'); + // Extract delimiter from format (first non-letter character) + const delimiterMatch = format.match(/[^a-zA-Z]/); + const delimiter = delimiterMatch ? delimiterMatch[0] : '/'; + const parts = dateStr.split(delimiter); if (format === 'dd/MM/yyyy' && parts.length === 3) {You'd also need to generalize the format check (e.g.,
format.startsWith('dd') && format includes 'MM' && format.endsWith('yyyy')), or better yet, use a lightweight date-parsing utility.
| # Rate limiting zone for external National ID API (2 requests/second per IP) | ||
| limit_req_zone $binary_remote_addr zone=external_nationalid:10m rate=2r/s; | ||
|
|
||
| server { | ||
| listen 80; | ||
| server_name _; | ||
|
|
||
| root /usr/share/nginx/html; | ||
| index index.html; | ||
|
|
||
| # Fineract API proxy | ||
| location /fineract-provider/ { | ||
| proxy_pass ${FINERACT_API_URL}/; | ||
| proxy_set_header Host $host; | ||
| proxy_set_header X-Real-IP $remote_addr; | ||
| proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; | ||
| proxy_set_header X-Forwarded-Proto $scheme; | ||
| } | ||
|
|
||
| # External National ID API proxy (only if configured) | ||
| location /external-nationalid { | ||
| # Use Docker DNS resolver for upstream resolution | ||
| resolver 127.0.0.11 valid=30s; | ||
|
|
||
| # Rate limit to prevent abuse of the upstream National ID API | ||
| limit_req zone=external_nationalid burst=5 nodelay; | ||
|
|
||
| # Read target from env var at runtime (set via envsubst on container start) | ||
| set $external_nationalid_target "${EXTERNAL_NATIONALID_API_URL}"; | ||
|
|
||
| # Rewrite path: strip /external-nationalid prefix before proxying | ||
| rewrite ^/external-nationalid(.*)$ $1 break; | ||
|
|
||
| proxy_pass $external_nationalid_target; | ||
| proxy_set_header Host $proxy_host; | ||
| proxy_set_header X-Real-IP $remote_addr; | ||
| proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; | ||
| proxy_set_header X-Forwarded-Proto $scheme; | ||
|
|
||
| # Inject API key server-side so it is never exposed to the browser | ||
| proxy_set_header ${EXTERNAL_NATIONAL_ID_SYSTEM_API_HEADER} "${EXTERNAL_NATIONAL_ID_SYSTEM_API_KEY}"; | ||
|
|
||
| # Pass through the original headers from the client | ||
| proxy_pass_request_headers on; | ||
|
|
||
| # CORS is not needed since requests come from the same origin | ||
| } | ||
|
|
||
| # Angular app - serve index.html for all routes | ||
| location / { | ||
| try_files $uri $uri/ /index.html; | ||
| } | ||
|
|
||
| # Cache static assets | ||
| location ~* \.(js|css|png|jpg|jpeg|gif|ico|svg|woff|woff2|ttf|eot)$ { | ||
| expires 1y; | ||
| add_header Cache-Control "public, immutable"; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "envsubst" --type-add 'docker:Dockerfile*' -g 'Dockerfile*' -g 'docker-compose*' -g '*.sh' -C5Repository: openMF/web-app
Length of output: 698
Add missing environment variables to the envsubst variable list in Dockerfile.
The Dockerfile correctly uses an explicit variable list for envsubst, which prevents clobbering nginx variables. However, the list is incomplete. The second envsubst call includes only ${FINERACT_API_URL} and ${EXTERNAL_NATIONAL_ID_SYSTEM_URL}, but nginx.conf.template also uses ${EXTERNAL_NATIONALID_API_URL}, ${EXTERNAL_NATIONAL_ID_SYSTEM_API_HEADER}, and ${EXTERNAL_NATIONAL_ID_SYSTEM_API_KEY}. These unmapped variables will remain as literal strings in the nginx config, breaking the external National ID API proxy.
Update the Dockerfile command to include all four variables:
envsubst '${FINERACT_API_URL} ${EXTERNAL_NATIONALID_API_URL} ${EXTERNAL_NATIONAL_ID_SYSTEM_API_HEADER} ${EXTERNAL_NATIONAL_ID_SYSTEM_API_KEY}' < /etc/nginx/templates/default.conf.template > /etc/nginx/conf.d/default.conf🤖 Prompt for AI Agents
In `@nginx.conf.template` around lines 1 - 59, The Dockerfile's envsubst call is
missing variables used in nginx.conf.template, causing literals like
${EXTERNAL_NATIONALID_API_URL}, ${EXTERNAL_NATIONAL_ID_SYSTEM_API_HEADER}, and
${EXTERNAL_NATIONAL_ID_SYSTEM_API_KEY} to remain unexpanded; update the second
envsubst invocation in the Dockerfile (the explicit envsubst line) to include
FINERACT_API_URL, EXTERNAL_NATIONALID_API_URL,
EXTERNAL_NATIONAL_ID_SYSTEM_API_HEADER, and EXTERNAL_NATIONAL_ID_SYSTEM_API_KEY
so the template variables in nginx.conf.template are properly substituted at
container start.
| # External National ID API proxy (only if configured) | ||
| location /external-nationalid { | ||
| # Use Docker DNS resolver for upstream resolution | ||
| resolver 127.0.0.11 valid=30s; | ||
|
|
||
| # Rate limit to prevent abuse of the upstream National ID API | ||
| limit_req zone=external_nationalid burst=5 nodelay; | ||
|
|
||
| # Read target from env var at runtime (set via envsubst on container start) | ||
| set $external_nationalid_target "${EXTERNAL_NATIONALID_API_URL}"; | ||
|
|
||
| # Rewrite path: strip /external-nationalid prefix before proxying | ||
| rewrite ^/external-nationalid(.*)$ $1 break; | ||
|
|
||
| proxy_pass $external_nationalid_target; | ||
| proxy_set_header Host $proxy_host; | ||
| proxy_set_header X-Real-IP $remote_addr; | ||
| proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; | ||
| proxy_set_header X-Forwarded-Proto $scheme; | ||
|
|
||
| # Inject API key server-side so it is never exposed to the browser | ||
| proxy_set_header ${EXTERNAL_NATIONAL_ID_SYSTEM_API_HEADER} "${EXTERNAL_NATIONAL_ID_SYSTEM_API_KEY}"; | ||
|
|
||
| # Pass through the original headers from the client | ||
| proxy_pass_request_headers on; | ||
|
|
||
| # CORS is not needed since requests come from the same origin | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's any conditional logic around these environment variables
rg -n "EXTERNAL_NATIONALID_API_URL|ENABLE_EXTERNAL_NATIONAL_ID|EXTERNAL_NATIONAL_ID_SYSTEM_API_HEADER" --max-count=50Repository: openMF/web-app
Length of output: 925
🏁 Script executed:
#!/bin/bash
# Find Docker entrypoint scripts and nginx startup logic
find . -type f \( -name "Dockerfile*" -o -name "entrypoint*" -o -name "docker-entrypoint*" -o -name "start*" \) | head -20Repository: openMF/web-app
Length of output: 70
🏁 Script executed:
#!/bin/bash
# Read the Dockerfile to understand nginx config generation
cat -n DockerfileRepository: openMF/web-app
Length of output: 2228
🏁 Script executed:
#!/bin/bash
# Read the nginx template to see exact variable usage
sed -n '20,47p' nginx.conf.template | cat -nRepository: openMF/web-app
Length of output: 1486
External National ID proxy block uses environment variables that are not included in the envsubst substitution whitelist.
The variables ${EXTERNAL_NATIONALID_API_URL}, ${EXTERNAL_NATIONAL_ID_SYSTEM_API_HEADER}, and ${EXTERNAL_NATIONAL_ID_SYSTEM_API_KEY} used in the nginx template (lines 29, 41) are not present in the Dockerfile's envsubst command (line 51), which only substitutes ${FINERACT_API_URL} and ${EXTERNAL_NATIONAL_ID_SYSTEM_URL}. This means the nginx config will contain literal unsubstituted variable references instead of actual values, causing proxy_pass to receive a string like "${EXTERNAL_NATIONALID_API_URL}" instead of the actual URL, and proxy_set_header to use an invalid literal header name.
Additionally, the location block itself is unconditionally included regardless of whether the feature is enabled. Add these variables to the envsubst whitelist in the Dockerfile and consider making the entire block conditional based on the ENABLE_EXTERNAL_NATIONAL_ID_SYSTEM flag.
🤖 Prompt for AI Agents
In `@nginx.conf.template` around lines 20 - 47, The nginx template uses
${EXTERNAL_NATIONALID_API_URL}, ${EXTERNAL_NATIONAL_ID_SYSTEM_API_HEADER}, and
${EXTERNAL_NATIONAL_ID_SYSTEM_API_KEY} but those vars aren’t added to the
envsubst whitelist in the Dockerfile, and the /external-nationalid location is
unconditionally included; update the Dockerfile’s envsubst command to include
these three variable names so they are substituted at container start, and wrap
the entire location /external-nationalid block in a conditional controlled by
ENABLE_EXTERNAL_NATIONAL_ID_SYSTEM (or remove the block unless that flag is
true) so the config and proxy_pass/header injection only exist when the feature
is enabled.
| context: ['/external-nationalid'], | ||
| target: 'https://apis.mifos.community', | ||
| pathRewrite: { '^/external-nationalid': '/1.0/nationalid' }, | ||
| changeOrigin: true, | ||
| secure: true, | ||
| logLevel: 'debug', | ||
| onProxyReq: function (proxyReq, req, res) { | ||
| // Inject API key server-side (same as nginx proxy_set_header in production) | ||
| const apiKey = process.env.EXTERNAL_NATIONAL_ID_SYSTEM_API_KEY || ''; | ||
| if (apiKey) { | ||
| proxyReq.setHeader('X-Gravitee-Api-Key', apiKey); | ||
| } | ||
| const rewrittenPath = (req.url || '').replace(/^\/external-nationalid/, '/1.0/nationalid'); | ||
| console.log('[Proxy] Proxying:', req.method, req.url, '->', this.target + rewrittenPath); | ||
| }, |
There was a problem hiding this comment.
Avoid logging raw External ID values (PII).
req.url can carry the National ID in query/path, which will be printed in debug logs. Please sanitize or remove it.
🛡️ Proposed fix (strip query before logging)
- const rewrittenPath = (req.url || '').replace(/^\/external-nationalid/, '/1.0/nationalid');
- console.log('[Proxy] Proxying:', req.method, req.url, '->', this.target + rewrittenPath);
+ const rewrittenPath = (req.url || '').replace(/^\/external-nationalid/, '/1.0/nationalid');
+ const safePath = rewrittenPath.split('?')[0];
+ console.log('[Proxy] Proxying:', req.method, safePath, '->', this.target + safePath);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| context: ['/external-nationalid'], | |
| target: 'https://apis.mifos.community', | |
| pathRewrite: { '^/external-nationalid': '/1.0/nationalid' }, | |
| changeOrigin: true, | |
| secure: true, | |
| logLevel: 'debug', | |
| onProxyReq: function (proxyReq, req, res) { | |
| // Inject API key server-side (same as nginx proxy_set_header in production) | |
| const apiKey = process.env.EXTERNAL_NATIONAL_ID_SYSTEM_API_KEY || ''; | |
| if (apiKey) { | |
| proxyReq.setHeader('X-Gravitee-Api-Key', apiKey); | |
| } | |
| const rewrittenPath = (req.url || '').replace(/^\/external-nationalid/, '/1.0/nationalid'); | |
| console.log('[Proxy] Proxying:', req.method, req.url, '->', this.target + rewrittenPath); | |
| }, | |
| context: ['/external-nationalid'], | |
| target: 'https://apis.mifos.community', | |
| pathRewrite: { '^/external-nationalid': '/1.0/nationalid' }, | |
| changeOrigin: true, | |
| secure: true, | |
| logLevel: 'debug', | |
| onProxyReq: function (proxyReq, req, res) { | |
| // Inject API key server-side (same as nginx proxy_set_header in production) | |
| const apiKey = process.env.EXTERNAL_NATIONAL_ID_SYSTEM_API_KEY || ''; | |
| if (apiKey) { | |
| proxyReq.setHeader('X-Gravitee-Api-Key', apiKey); | |
| } | |
| const rewrittenPath = (req.url || '').replace(/^\/external-nationalid/, '/1.0/nationalid'); | |
| const safePath = rewrittenPath.split('?')[0]; | |
| console.log('[Proxy] Proxying:', req.method, safePath, '->', this.target + safePath); | |
| }, |
🤖 Prompt for AI Agents
In `@proxy.conf.js` around lines 48 - 62, The onProxyReq handler logs req.url
which may contain PII; update the onProxyReq function to avoid printing raw
National ID values by stripping the query string and/or path segments that may
contain IDs before logging: compute a safe path (e.g., use rewrittenPath without
query or replace query portion with '?[redacted]' or mask last path segment) and
log req.method plus this sanitized path instead of req.url; reference the
onProxyReq function, proxyReq.setHeader, req.url and rewrittenPath when making
the change.
| context: ['/external-nationalid'], | ||
| target: 'https://apis.mifos.community', | ||
| pathRewrite: { '^/external-nationalid': '/1.0/nationalid' }, | ||
| changeOrigin: true, | ||
| secure: true, | ||
| logLevel: 'debug', | ||
| onProxyReq: function (proxyReq, req, res) { | ||
| // Inject API key server-side (same as nginx proxy_set_header in production) | ||
| const apiKey = process.env.EXTERNAL_NATIONAL_ID_SYSTEM_API_KEY || ''; | ||
| if (apiKey) { | ||
| proxyReq.setHeader('X-Gravitee-Api-Key', apiKey); | ||
| } | ||
| const rewrittenPath = (req.url || '').replace(/^\/external-nationalid/, '/1.0/nationalid'); | ||
| console.log('[Proxy] Proxying:', req.method, req.url, '->', this.target + rewrittenPath); |
There was a problem hiding this comment.
Avoid logging raw External ID values (PII).
The dev proxy prints req.url, which can include the National ID. Please mask or omit it.
🛡️ Proposed fix (strip query before logging)
- const rewrittenPath = (req.url || '').replace(/^\/external-nationalid/, '/1.0/nationalid');
- console.log('[Proxy] Proxying:', req.method, req.url, '->', this.target + rewrittenPath);
+ const rewrittenPath = (req.url || '').replace(/^\/external-nationalid/, '/1.0/nationalid');
+ const safePath = rewrittenPath.split('?')[0];
+ console.log('[Proxy] Proxying:', req.method, safePath, '->', this.target + safePath);🤖 Prompt for AI Agents
In `@proxy.localhost.conf.js` around lines 45 - 58, The proxy currently logs raw
req.url (which can contain PII) inside the onProxyReq handler; change the log to
avoid printing full URLs or query strings by logging only non-sensitive parts
(e.g., req.method and the rewrittenPath or a sanitized path with query stripped)
instead of req.url, and ensure onProxyReq uses rewrittenPath (or a sanitized
version derived from req.url without query/identifier segments) when calling
console.log; keep the API key injection logic
(process.env.EXTERNAL_NATIONAL_ID_SYSTEM_API_KEY and proxyReq.setHeader)
untouched.
| constructor() { | ||
| this.enabled = environment.enableExternalNationalIdSystem === true; | ||
|
|
||
| const regexStr = environment.externalNationalIdRegex; | ||
| if (regexStr) { | ||
| try { | ||
| this.pattern = new RegExp(regexStr); | ||
| } catch { | ||
| console.warn('[ExternalNationalId] Invalid regex pattern:', regexStr); | ||
| this.pattern = null; | ||
| } | ||
| } else { | ||
| this.pattern = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
ReDoS mitigation comment is misleading — compiling once does not prevent ReDoS.
The comment on line 63 states "Compiled once in the constructor to avoid repeated new RegExp() calls (CWE-1333 mitigation)." However, CWE-1333 (ReDoS) is about catastrophic backtracking in the regex engine when matching input — not about how many times the regex is compiled. Since externalNationalIdRegex is admin-configured via environment variables, the risk is limited to misconfiguration rather than user-supplied attack vectors. Still, a malicious or poorly written regex could cause the UI to freeze when isValidExternalId is called on every keystroke (via the debounced valueChanges pipe).
Consider adding a basic safety check (e.g., reject patterns with nested quantifiers) or at minimum correcting the comment to reflect the actual risk profile.
🧰 Tools
🪛 ast-grep (0.40.5)
[warning] 83-83: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(regexStr)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
In `@src/app/clients/services/external-national-id.service.ts` around lines 78 -
92, Update the misleading comment in the constructor to reflect that compiling
once reduces overhead but does not mitigate ReDoS, and implement a basic safety
check when reading environment.externalNationalIdRegex in the constructor: scan
the pattern string for common dangerous constructs (e.g., nested/adjacent
quantifiers or group-plus-quantifier patterns like (.+)+, (a+)+, or back-to-back
quantifiers) and if detected reject the pattern (set this.pattern = null) and
log a clear warning; keep the existing try/catch for invalid regex syntax and
ensure isValidExternalId uses this.pattern safely (no further compilation) so
the code references include constructor, environment.externalNationalIdRegex,
this.pattern, and isValidExternalId.
| if (firstnameCtrl && response.firstname) { | ||
| firstnameCtrl.setValue(response.firstname); | ||
| firstnameCtrl.disable(); | ||
| } | ||
|
|
||
| if (middlenameCtrl && response.middlename) { | ||
| middlenameCtrl.setValue(response.middlename); | ||
| middlenameCtrl.disable(); | ||
| } | ||
|
|
||
| if (lastnameCtrl && response.lastname) { | ||
| lastnameCtrl.setValue(response.lastname); | ||
| lastnameCtrl.disable(); | ||
| } |
There was a problem hiding this comment.
Empty-string responses won't clear pre-existing field values.
The guards response.firstname, response.middlename, and response.lastname are truthy checks — an empty string "" from the API would skip setValue, leaving stale data in the form. If the external API can return an empty string for fields like middlename (e.g., person has no middle name), the form would retain whatever was previously entered.
If that is intentional (i.e., only overwrite when the API provides a non-empty value), consider adding a brief comment to document the design choice.
🤖 Prompt for AI Agents
In `@src/app/clients/services/external-national-id.service.ts` around lines 211 -
224, The current truthy checks (response.firstname, response.middlename,
response.lastname) skip setting form controls when the API returns an empty
string, leaving stale values in firstnameCtrl/middlenameCtrl/lastnameCtrl;
change the guards to test for undefined/null (e.g., response.firstname !==
undefined && response.firstname !== null) so empty strings from the API will
overwrite existing values (and still disable the controls), or if the intent is
to only overwrite non-empty values, add a short comment above these blocks
documenting that design decision; refer to firstnameCtrl, middlenameCtrl,
lastnameCtrl and response.firstname/response.middlename/response.lastname when
making the change.
| // External National ID System Integration | ||
| // API key is injected server-side via nginx proxy_set_header — not exposed to browser | ||
| window['env']['enableExternalNationalIdSystem'] = '$ENABLE_EXTERNAL_NATIONAL_ID_SYSTEM'; | ||
| window['env']['externalNationalIdSystemUrl'] = '$EXTERNAL_NATIONAL_ID_SYSTEM_URL'; | ||
| window['env']['externalNationalIdSystemApiHeader'] = '$EXTERNAL_NATIONAL_ID_SYSTEM_API_HEADER'; | ||
| window['env']['externalNationalIdSystemApiKey'] = '$EXTERNAL_NATIONAL_ID_SYSTEM_API_KEY'; | ||
| window['env']['externalNationalIdRegex'] = '$EXTERNAL_NATIONAL_ID_REGEX'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the API key / header are actually consumed by the Angular service in HTTP calls
rg -n 'externalNationalIdSystemApiKey|externalNationalIdSystemApiHeader' --type=ts -C3Repository: openMF/web-app
Length of output: 2654
🏁 Script executed:
# Also check if similar patterns exist in env.js
cat src/assets/env.js | head -120Repository: openMF/web-app
Length of output: 3689
🏁 Script executed:
# Check overall structure of env.template.js around the flagged lines
cat -n src/assets/env.template.js | sed -n '100,120p'Repository: openMF/web-app
Length of output: 1504
🏁 Script executed:
# Search for any usage of these env keys across all file types
rg 'externalNationalIdSystemApiKey|externalNationalIdSystemApiHeader' --no-headingRepository: openMF/web-app
Length of output: 1114
🏁 Script executed:
# Get full context around the API header/key usage in clients.service.ts
sed -n '460,480p' src/app/clients/clients.service.tsRepository: openMF/web-app
Length of output: 667
🏁 Script executed:
# Check if there are any tests or documentation explaining the architecture
rg -i 'externalNationalIdSystem.*api' --type=md --type=ts -C5 | head -80Repository: openMF/web-app
Length of output: 3740
Remove API header and key from client-side env template—they contradict the stated architecture.
The comment claims "API key is injected server-side via nginx proxy_set_header — not exposed to browser," yet lines 110–111 define template variables that will be rendered into the browser-served env.js if the corresponding environment variables are populated at runtime.
The frontend code in src/app/clients/clients.service.ts (lines 463–471) actively reads these values and adds them as HTTP headers to requests:
const apiHeader = environment.externalNationalIdSystemApiHeader;
const apiKey = environment.externalNationalIdSystemApiKey;
if (apiHeader && apiKey) {
headers = headers.set(apiHeader, apiKey);
}If this is truly a server-side-only design (nginx proxy handling auth), the frontend should never see these secrets. Remove lines 110–111 from the template to prevent accidental secret leakage, and update the frontend to work without them (it already handles the case where they're absent via the if check).
Proposed fix
// External National ID System Integration
- // API key is injected server-side via nginx proxy_set_header — not exposed to browser
window['env']['enableExternalNationalIdSystem'] = '$ENABLE_EXTERNAL_NATIONAL_ID_SYSTEM';
window['env']['externalNationalIdSystemUrl'] = '$EXTERNAL_NATIONAL_ID_SYSTEM_URL';
- window['env']['externalNationalIdSystemApiHeader'] = '$EXTERNAL_NATIONAL_ID_SYSTEM_API_HEADER';
- window['env']['externalNationalIdSystemApiKey'] = '$EXTERNAL_NATIONAL_ID_SYSTEM_API_KEY';
window['env']['externalNationalIdRegex'] = '$EXTERNAL_NATIONAL_ID_REGEX';🤖 Prompt for AI Agents
In `@src/assets/env.template.js` around lines 106 - 112, Remove the client-side
exposure of the external National ID API header and key by deleting the
window['env']['externalNationalIdSystemApiHeader'] and
window['env']['externalNationalIdSystemApiKey'] template entries from
src/assets/env.template.js, and ensure the frontend (ClientsService in
src/app/clients/clients.service.ts) continues to handle their absence by relying
on the existing conditional check around
environment.externalNationalIdSystemApiHeader and
environment.externalNationalIdSystemApiKey when building request headers;
confirm no other code reads those env vars and add a brief comment in
ClientsService noting that authentication headers are injected by the
server/nginx proxy so frontend secrets must not be present.
| * When enabled, client creation/editing will lookup external National ID | ||
| * and auto-fill client details (name, DOB, gender) from the external system. | ||
| */ | ||
| enableExternalNationalIdSystem: loadedEnv.enableExternalNationalIdSystem === 'true' || false, |
There was a problem hiding this comment.
Inconsistent boolean parsing — missing === true check.
Other boolean flags in this same file (e.g., complianceHideClientData on line 106, productionModeEnableRBAC on line 115) and the production environment (environment.prod.ts line 90-92) check for both === 'true' and === true. This property only checks the string form.
🐛 Proposed fix
- enableExternalNationalIdSystem: loadedEnv.enableExternalNationalIdSystem === 'true' || false,
+ enableExternalNationalIdSystem:
+ loadedEnv.enableExternalNationalIdSystem === 'true' || loadedEnv.enableExternalNationalIdSystem === true || false,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| enableExternalNationalIdSystem: loadedEnv.enableExternalNationalIdSystem === 'true' || false, | |
| enableExternalNationalIdSystem: | |
| loadedEnv.enableExternalNationalIdSystem === 'true' || loadedEnv.enableExternalNationalIdSystem === true || false, |
🤖 Prompt for AI Agents
In `@src/environments/environment.ts` at line 93, The property
enableExternalNationalIdSystem only checks for the string 'true' and is
inconsistent with other boolean flags; update its expression to accept both the
string and boolean true (e.g., change the value to check
loadedEnv.enableExternalNationalIdSystem === 'true' ||
loadedEnv.enableExternalNationalIdSystem === true || false) so it matches the
pattern used by complianceHideClientData and productionModeEnableRBAC.
…ion/editing improve
shubhamkumar9199
force-pushed
the
feature/WEB-600-external-national-id-integration
branch
from
d8e3fdb to
7711b72
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/app/clients/clients.service.ts`:
- Around line 462-471: The code is injecting the External National ID API header
client-side using environment.externalNationalIdSystemApiHeader and
environment.externalNationalIdSystemApiKey (headers variable in
clients.service.ts); remove this client-side header injection block entirely so
the request relies on the server/nginx proxy to set the header, and delete or
disable the validation and headers = headers.set(...) logic; if you must keep
the code path for local testing, replace it with a clearly named debug flag and
document that it should remain disabled in browser builds.
In `@src/app/clients/services/external-national-id.service.ts`:
- Around line 254-284: parseDate currently always splits by '/' which breaks
when dateFormat uses '-' (e.g., "yyyy-MM-dd"); update parseDate to respect the
provided dateFormat by choosing the delimiter or token positions dynamically (or
add an explicit branch for "yyyy-MM-dd") and parse components accordingly (map
tokens -> indexes, parse ints, adjust month-1), then perform the same range and
component validation (isNaN, getDate/getMonth/getFullYear) before returning the
Date; keep the existing fallback to native parsing only after the explicit
format-handling fails.
- Around line 113-174: watchExternalId currently attaches a new subscription to
externalIdCtrl.valueChanges each time it's called, causing duplicate
subscriptions; create a per-watch subject (e.g., externalIdWatch$ on the
service/component) and before creating the new subscription call
externalIdWatch$.next() (and optionally .complete()/recreate) to cancel any
previous watch, then add takeUntil(this.externalIdWatch$) into the existing pipe
in watchExternalId alongside the existing takeUntil(this.destroy$) (or replace
destroy$ for that stream) so each new call unsubscribes the prior observable and
avoids duplicate API calls and conflicting updates; update ngOnDestroy to
complete externalIdWatch$ if needed.
🧹 Nitpick comments (2)
proxy.localhost.conf.js (1)
44-75: Consider extracting the sharedonErrorhandler.The
onErrorcallback at lines 60–74 is identical to lines 28–42. Extract it into a shared function to reduce duplication.♻️ Proposed refactor
+'use strict'; + +function onProxyError(err, req, res) { + console.error( + '[Proxy] Error while proxying request:', + req && req.method, + req && req.url, + '->', + this.target, + '-', + err && err.message + ); + if (res && !res.headersSent) { + res.writeHead(502, { 'Content-Type': 'text/plain' }); + res.end('Proxy error: ' + (err && err.message ? err.message : 'Unknown error')); + } +} + module.exports = [ { context: ['/fineract-provider'], // ... - onError: function (err, req, res) { /* ... */ } + onError: onProxyError }, { context: ['/external-nationalid'], // ... - onError: function (err, req, res) { /* ... */ } + onError: onProxyError } ];src/app/clients/clients.service.ts (1)
456-474: UseExternalNationalIdResponseinstead ofObservable<any>.The response interface already exists in
external-national-id.service.ts. Consider importing and using it as the return type for better type safety downstream.♻️ Proposed change
- lookupExternalNationalId(externalId: string): Observable<any> { + lookupExternalNationalId(externalId: string): Observable<ExternalNationalIdResponse> {As per coding guidelines,
src/app/**: "verify … strict type safety".
| let headers = new HttpHeaders({ 'Content-Type': 'application/json' }); | ||
| const apiHeader = environment.externalNationalIdSystemApiHeader; | ||
| const apiKey = environment.externalNationalIdSystemApiKey; | ||
| if (apiHeader && apiKey) { | ||
| // Validate header name to prevent Angular from throwing on invalid header | ||
| if (!/^[a-zA-Z][a-zA-Z0-9-]*$/.test(apiHeader)) { | ||
| return throwError(() => new Error(`Invalid API header name: '${apiHeader}'`)); | ||
| } | ||
| headers = headers.set(apiHeader, apiKey); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "environment*.ts" -type f | head -20Repository: openMF/web-app
Length of output: 130
🏁 Script executed:
find . -name "env.js" -o -name "env.template.js" -type f | head -20Repository: openMF/web-app
Length of output: 106
🏁 Script executed:
fd "environment" --type f | grep -E "(environment|env)" | head -30Repository: openMF/web-app
Length of output: 126
🏁 Script executed:
# Check the service file to understand the API key usage
cat -n src/app/clients/clients.service.ts | sed -n '456,475p'Repository: openMF/web-app
Length of output: 1047
🏁 Script executed:
# Check if there's a proxy configuration file
fd "proxy" --type f | grep -i "conf\|config" | head -20Repository: openMF/web-app
Length of output: 95
🏁 Script executed:
cat -n src/environments/environment.tsRepository: openMF/web-app
Length of output: 7389
🏁 Script executed:
cat -n src/environments/environment.prod.tsRepository: openMF/web-app
Length of output: 7174
🏁 Script executed:
cat -n src/assets/env.template.jsRepository: openMF/web-app
Length of output: 5832
🏁 Script executed:
cat -n src/assets/env.jsRepository: openMF/web-app
Length of output: 4466
🏁 Script executed:
cat -n proxy.localhost.conf.jsRepository: openMF/web-app
Length of output: 3069
🏁 Script executed:
cat -n proxy.conf.jsRepository: openMF/web-app
Length of output: 4230
Remove client-side API key injection; rely on proxy/nginx for server-side header injection.
The service code (line 470) attempts to set externalNationalIdSystemApiKey as a request header. However, both env.template.js and env.js explicitly document that the API key is "injected server-side via nginx proxy_set_header" and "never set" in the browser configuration. The proxy configurations (proxy.localhost.conf.js and proxy.conf.js) confirm this design: they read process.env.EXTERNAL_NATIONAL_ID_SYSTEM_API_KEY and inject it server-side.
This creates a contradiction: the code attempts client-side injection when the infrastructure is designed to handle it server-side only. Remove the client-side header injection logic entirely, or clearly document when/why the app should set this header instead of relying on the proxy.
🤖 Prompt for AI Agents
In `@src/app/clients/clients.service.ts` around lines 462 - 471, The code is
injecting the External National ID API header client-side using
environment.externalNationalIdSystemApiHeader and
environment.externalNationalIdSystemApiKey (headers variable in
clients.service.ts); remove this client-side header injection block entirely so
the request relies on the server/nginx proxy to set the header, and delete or
disable the validation and headers = headers.set(...) logic; if you must keep
the code path for local testing, replace it with a clearly named debug flag and
document that it should remain disabled in browser builds.
| watchExternalId(form: UntypedFormGroup, genderOptions: GenderOption[], skipInitialValue = false): void { | ||
| if (!this.enabled) { | ||
| return; | ||
| } | ||
|
|
||
| const externalIdCtrl = form.get('externalId'); | ||
| if (!externalIdCtrl) { | ||
| return; | ||
| } | ||
|
|
||
| // In edit mode, if the client already has a valid external ID, lock the fields | ||
| // without making an API call | ||
| if (skipInitialValue && externalIdCtrl.value && this.isValidExternalId(externalIdCtrl.value)) { | ||
| this.disablePersonFields(form); | ||
| return; | ||
| } | ||
|
|
||
| externalIdCtrl.valueChanges | ||
| .pipe( | ||
| debounceTime(500), | ||
| distinctUntilChanged(), | ||
| filter((value: string) => { | ||
| if (!value || !this.isValidExternalId(value)) { | ||
| // Re-enable fields if ID is cleared or invalid | ||
| this.enablePersonFields(form); | ||
| if (value && value.length > 3 && !this.isValidExternalId(value)) { | ||
| this.statusMessageKey = EXTERNAL_ID_STATUS_KEYS.INVALID_FORMAT; | ||
| } else { | ||
| this.statusMessageKey = EXTERNAL_ID_STATUS_KEYS.EMPTY; | ||
| } | ||
| return false; | ||
| } | ||
| return true; | ||
| }), | ||
| switchMap((value: string) => { | ||
| this.isLoading = true; | ||
| this.statusMessageKey = EXTERNAL_ID_STATUS_KEYS.LOADING; | ||
| return this.clientsService.lookupExternalNationalId(value).pipe( | ||
| timeout(10000), | ||
| catchError((error) => { | ||
| this.isLoading = false; | ||
| if (error.name === 'TimeoutError') { | ||
| this.statusMessageKey = EXTERNAL_ID_STATUS_KEYS.TIMEOUT; | ||
| } else if (error.status === 404) { | ||
| this.statusMessageKey = EXTERNAL_ID_STATUS_KEYS.NOT_FOUND; | ||
| } else { | ||
| this.statusMessageKey = EXTERNAL_ID_STATUS_KEYS.FAILED; | ||
| } | ||
| this.enablePersonFields(form); | ||
| return of(null); | ||
| }) | ||
| ); | ||
| }), | ||
| takeUntil(this.destroy$) | ||
| ) | ||
| .subscribe((response: ExternalNationalIdResponse | null) => { | ||
| this.isLoading = false; | ||
| if (response) { | ||
| this.fillFormFromResponse(form, response, genderOptions); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Multiple calls to watchExternalId will create duplicate subscriptions.
If watchExternalId is called more than once (e.g., form rebuilt on legal-form change), a new subscription is added each time without unsubscribing from the previous one, because destroy$ only fires on component destroy. This would cause duplicate API calls and conflicting field updates.
Consider adding a per-watch subject to cancel the previous subscription:
♻️ Proposed fix
+ private watchCancel$ = new Subject<void>();
+
watchExternalId(form: UntypedFormGroup, genderOptions: GenderOption[], skipInitialValue = false): void {
if (!this.enabled) {
return;
}
+ // Cancel any previous watch subscription
+ this.watchCancel$.next();
const externalIdCtrl = form.get('externalId');
// ...
externalIdCtrl.valueChanges
.pipe(
// ...
- takeUntil(this.destroy$)
+ takeUntil(merge(this.watchCancel$, this.destroy$))
)
.subscribe(/* ... */);
}
ngOnDestroy(): void {
+ this.watchCancel$.next();
+ this.watchCancel$.complete();
this.destroy$.next();
this.destroy$.complete();
}🤖 Prompt for AI Agents
In `@src/app/clients/services/external-national-id.service.ts` around lines 113 -
174, watchExternalId currently attaches a new subscription to
externalIdCtrl.valueChanges each time it's called, causing duplicate
subscriptions; create a per-watch subject (e.g., externalIdWatch$ on the
service/component) and before creating the new subscription call
externalIdWatch$.next() (and optionally .complete()/recreate) to cancel any
previous watch, then add takeUntil(this.externalIdWatch$) into the existing pipe
in watchExternalId alongside the existing takeUntil(this.destroy$) (or replace
destroy$ for that stream) so each new call unsubscribes the prior observable and
avoids duplicate API calls and conflicting updates; update ngOnDestroy to
complete externalIdWatch$ if needed.
| parseDate(dateStr: string, dateFormat?: string): Date | null { | ||
| if (!dateStr) { | ||
| return null; | ||
| } | ||
|
|
||
| const format = dateFormat || 'dd/MM/yyyy'; | ||
| const parts = dateStr.split('/'); | ||
|
|
||
| if (format === 'dd/MM/yyyy' && parts.length === 3) { | ||
| const day = parseInt(parts[0], 10); | ||
| const month = parseInt(parts[1], 10) - 1; | ||
| const year = parseInt(parts[2], 10); | ||
|
|
||
| // Validate ranges before constructing | ||
| if (day < 1 || day > 31 || month < 0 || month > 11 || year < 1900 || year > 2100) { | ||
| return null; | ||
| } | ||
|
|
||
| const date = new Date(year, month, day); | ||
|
|
||
| // Verify the date components match (catches invalid dates like Feb 30) | ||
| if (isNaN(date.getTime()) || date.getDate() !== day || date.getMonth() !== month || date.getFullYear() !== year) { | ||
| return null; | ||
| } | ||
| return date; | ||
| } | ||
|
|
||
| // Fallback: try native parsing | ||
| const fallback = new Date(dateStr); | ||
| return isNaN(fallback.getTime()) ? null : fallback; | ||
| } |
There was a problem hiding this comment.
parseDate only handles '/' delimiters, silently falls through for other formats.
Line 260 always splits on '/'. If the API returns a format like "yyyy-MM-dd" (with dateFormat: "yyyy-MM-dd"), the split produces a single-element array, the dd/MM/yyyy branch is skipped, and native Date parsing takes over — which may produce incorrect results depending on the browser's locale and the date string format.
Consider handling at least the most common alternative format (yyyy-MM-dd) explicitly, or splitting on a dynamic delimiter.
♻️ Proposed improvement
parseDate(dateStr: string, dateFormat?: string): Date | null {
if (!dateStr) {
return null;
}
const format = dateFormat || 'dd/MM/yyyy';
- const parts = dateStr.split('/');
if (format === 'dd/MM/yyyy' && parts.length === 3) {
+ const parts = dateStr.split('/');
+ if (parts.length !== 3) return null;
const day = parseInt(parts[0], 10);
const month = parseInt(parts[1], 10) - 1;
const year = parseInt(parts[2], 10);
// ... existing validation ...
+ } else if (format === 'yyyy-MM-dd') {
+ const parts = dateStr.split('-');
+ if (parts.length !== 3) return null;
+ const year = parseInt(parts[0], 10);
+ const month = parseInt(parts[1], 10) - 1;
+ const day = parseInt(parts[2], 10);
+ if (day < 1 || day > 31 || month < 0 || month > 11 || year < 1900 || year > 2100) return null;
+ const date = new Date(year, month, day);
+ if (isNaN(date.getTime()) || date.getDate() !== day || date.getMonth() !== month || date.getFullYear() !== year) return null;
+ return date;
}
// Fallback: try native parsing
const fallback = new Date(dateStr);
return isNaN(fallback.getTime()) ? null : fallback;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| parseDate(dateStr: string, dateFormat?: string): Date | null { | |
| if (!dateStr) { | |
| return null; | |
| } | |
| const format = dateFormat || 'dd/MM/yyyy'; | |
| const parts = dateStr.split('/'); | |
| if (format === 'dd/MM/yyyy' && parts.length === 3) { | |
| const day = parseInt(parts[0], 10); | |
| const month = parseInt(parts[1], 10) - 1; | |
| const year = parseInt(parts[2], 10); | |
| // Validate ranges before constructing | |
| if (day < 1 || day > 31 || month < 0 || month > 11 || year < 1900 || year > 2100) { | |
| return null; | |
| } | |
| const date = new Date(year, month, day); | |
| // Verify the date components match (catches invalid dates like Feb 30) | |
| if (isNaN(date.getTime()) || date.getDate() !== day || date.getMonth() !== month || date.getFullYear() !== year) { | |
| return null; | |
| } | |
| return date; | |
| } | |
| // Fallback: try native parsing | |
| const fallback = new Date(dateStr); | |
| return isNaN(fallback.getTime()) ? null : fallback; | |
| } | |
| parseDate(dateStr: string, dateFormat?: string): Date | null { | |
| if (!dateStr) { | |
| return null; | |
| } | |
| const format = dateFormat || 'dd/MM/yyyy'; | |
| if (format === 'dd/MM/yyyy') { | |
| const parts = dateStr.split('/'); | |
| if (parts.length !== 3) return null; | |
| const day = parseInt(parts[0], 10); | |
| const month = parseInt(parts[1], 10) - 1; | |
| const year = parseInt(parts[2], 10); | |
| // Validate ranges before constructing | |
| if (day < 1 || day > 31 || month < 0 || month > 11 || year < 1900 || year > 2100) { | |
| return null; | |
| } | |
| const date = new Date(year, month, day); | |
| // Verify the date components match (catches invalid dates like Feb 30) | |
| if (isNaN(date.getTime()) || date.getDate() !== day || date.getMonth() !== month || date.getFullYear() !== year) { | |
| return null; | |
| } | |
| return date; | |
| } else if (format === 'yyyy-MM-dd') { | |
| const parts = dateStr.split('-'); | |
| if (parts.length !== 3) return null; | |
| const year = parseInt(parts[0], 10); | |
| const month = parseInt(parts[1], 10) - 1; | |
| const day = parseInt(parts[2], 10); | |
| if (day < 1 || day > 31 || month < 0 || month > 11 || year < 1900 || year > 2100) return null; | |
| const date = new Date(year, month, day); | |
| if (isNaN(date.getTime()) || date.getDate() !== day || date.getMonth() !== month || date.getFullYear() !== year) return null; | |
| return date; | |
| } | |
| // Fallback: try native parsing | |
| const fallback = new Date(dateStr); | |
| return isNaN(fallback.getTime()) ? null : fallback; | |
| } |
🤖 Prompt for AI Agents
In `@src/app/clients/services/external-national-id.service.ts` around lines 254 -
284, parseDate currently always splits by '/' which breaks when dateFormat uses
'-' (e.g., "yyyy-MM-dd"); update parseDate to respect the provided dateFormat by
choosing the delimiter or token positions dynamically (or add an explicit branch
for "yyyy-MM-dd") and parse components accordingly (map tokens -> indexes, parse
ints, adjust month-1), then perform the same range and component validation
(isNaN, getDate/getMonth/getFullYear) before returning the Date; keep the
existing fallback to native parsing only after the explicit format-handling
fails.
2 participants
…ion/editing
Implements External National ID System integration for client creation and editing workflows. When a user enters a valid National ID (e.g., Mexico's CURP), the system auto-validates, fetches client data from an external government API, and auto-fills the form fields
WEB-600
Environment Variables Added
Security
proxy_set_headerChecklist
Please make sure these boxes are checked before submitting your pull request - thanks!
If you have multiple commits please combine them into one commit by squashing them.
Read and understood the contribution guidelines at
web-app/.github/CONTRIBUTING.md.Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores