Updated certificate API changes #62
Conversation
WalkthroughThis pull request introduces configuration entries for ABHA V3 APIs across multiple environment properties files and updates the Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (8)
src/main/environment/common_ci.properties (2)
Line range hint
88-98: Standardize environment variable usage across V3 API configurations.The configuration shows inconsistent usage of environment variables. Some endpoints use
@env.ABDM_BASE_URL@while others use direct URLs. This could make environment management more difficult.Consider using environment variables consistently:
abdmV3UserAuthenticate = @env.ABDM_BASE_URL@/api/hiecm/gateway/v3/sessions -getAuthCertPublicKey = @env.ABDM_HEALTH_ID_BASE_URL@/api/v1/auth/cert -requestOtpForEnrollment = @env.ABDM_BASE_URL@/abha/api/v3/enrollment/request/otp -abhaEnrollByAadhaar = @env.ABDM_BASE_URL@/abha/api/v3/enrollment/enrol/byAadhaar -printAbhaCard = @env.ABDM_BASE_URL@/abha/api/v3/profile/account/abha-card -abhaLoginRequestOtp = @env.ABDM_BASE_URL@/abha/api/v3/profile/login/request/otp -verifyAbhaLogin = @env.ABDM_BASE_URL@/abha/api/v3/profile/login/verify +getAuthCertPublicKey = @env.ABDM_HEALTH_ID_BASE_URL@/api/v1/auth/cert +requestOtpForEnrollment = @env.ABDM_V3_BASE_URL@/abha/api/v3/enrollment/request/otp +abhaEnrollByAadhaar = @env.ABDM_V3_BASE_URL@/abha/api/v3/enrollment/enrol/byAadhaar +printAbhaCard = @env.ABDM_V3_BASE_URL@/abha/api/v3/profile/account/abha-card +abhaLoginRequestOtp = @env.ABDM_V3_BASE_URL@/abha/api/v3/profile/login/request/otp +verifyAbhaLogin = @env.ABDM_V3_BASE_URL@/abha/api/v3/profile/login/verify
Line range hint
88-98: Ensure consistent environment usage across ABHA V3 endpoints.The configuration mixes production and sandbox environments:
abdmV3UserAuthenticateuses production URL (dev.abdm.gov.in)- Other endpoints use sandbox URLs (abhasbx.abdm.gov.in)
This inconsistency could lead to authentication issues in the CI environment.
Consider using environment variables consistently:
-abdmV3UserAuthenticate = @env.ABDM_BASE_URL@/api/hiecm/gateway/v3/sessions -getAuthCertPublicKey = @env.ABDM_HEALTH_ID_BASE_URL@/api/v1/auth/cert -requestOtpForEnrollment = @env.ABDM_BASE_URL@/abha/api/v3/enrollment/request/otp +abdmV3UserAuthenticate = @env.ABDM_V3_BASE_URL@/api/hiecm/gateway/v3/sessions +getAuthCertPublicKey = @env.ABDM_V3_BASE_URL@/api/v1/auth/cert +requestOtpForEnrollment = @env.ABDM_V3_BASE_URL@/abha/api/v3/enrollment/request/otpsrc/main/java/com/wipro/fhir/service/v3/abha/CreateAbhaV3ServiceImpl.java (3)
Line range hint
106-111: Enhance error handling for encryption operations.The encryption of
loginIdshould have proper error handling to prevent potential security vulnerabilities. Consider adding specific exception handling for encryption failures.- publicKeyString = certificateKeyService.getCertPublicKey(ndhmAuthToken); - if (loginMethod.getLoginId() != null) { - encryptedLoginId = encryption.encrypt(loginMethod.getLoginId(), publicKeyString); + try { + publicKeyString = certificateKeyService.getCertPublicKey(ndhmAuthToken); + if (loginMethod.getLoginId() == null) { + throw new FHIRException("Login ID cannot be null"); + } + encryptedLoginId = encryption.encrypt(loginMethod.getLoginId(), publicKeyString); + } catch (Exception e) { + logger.error("Failed to encrypt login ID", e); + throw new FHIRException("Failed to secure your data. Please try again.");Also applies to: 174-179, 265-270
Line range hint
89-90: Consider using a dedicated HTTP client configuration.The class creates new
RestTemplateinstances for each request. Consider using a configuredRestTemplatebean with proper timeout and retry settings.+ @Value("${rest.template.timeout:5000}") + private int timeout; + + @Bean + public RestTemplate restTemplate() { + RestTemplate restTemplate = new RestTemplate(); + HttpComponentsClientHttpRequestFactory factory = new HttpComponentsClientHttpRequestFactory(); + factory.setConnectTimeout(timeout); + factory.setReadTimeout(timeout); + restTemplate.setRequestFactory(factory); + return restTemplate; + } + @Override public String getOtpForEnrollment(String request) throws FHIRException { String res = null; Map<String, String> responseMap = new HashMap<>(); - RestTemplate restTemplate = new RestTemplate(); + RestTemplate restTemplate = restTemplate();Also applies to: 91-92
Line range hint
428-429: Improve error handling inconstructHealthIdResponse.The method assumes that all required fields are present in the JSON response. Consider adding null checks and proper error handling.
private void constructHealthIdResponse(HealthIDResponse healthIDResp, JsonObject profile) throws ParseException { - healthIDResp.setHealthIdNumber(profile.get("ABHANumber").getAsString()); + if (!profile.has("ABHANumber")) { + throw new FHIRException("ABHA number is missing in the response"); + } + healthIDResp.setHealthIdNumber(profile.get("ABHANumber").getAsString()); + + if (!profile.has("phrAddress")) { + throw new FHIRException("PHR address is missing in the response"); + } JsonArray phrAddressArray = profile.getAsJsonArray("phrAddress");Also applies to: 430-431
src/main/environment/common_example.properties (1)
Line range hint
91-97: Use environment variables instead of hardcoded URLs in example configuration.The example configuration contains hardcoded URLs which may become outdated. Consider using placeholder variables to maintain consistency with other environment files and make updates easier.
Apply this change:
-abdmV3UserAuthenticate = https://dev.abdm.gov.in/api/hiecm/gateway/v3/sessions -getAuthCertPublicKey = https://healthidsbx.abdm.gov.in/api/v1/auth/cert -requestOtpForEnrollment = https://abhasbx.abdm.gov.in/abha/api/v3/enrollment/request/otp +abdmV3UserAuthenticate = @env.ABDM_V3_BASE_URL@/api/hiecm/gateway/v3/sessions +getAuthCertPublicKey = @env.ABDM_V3_BASE_URL@/api/v1/auth/cert +requestOtpForEnrollment = @env.ABDM_V3_BASE_URL@/abha/api/v3/enrollment/request/otpsrc/main/environment/common_dev.properties (1)
Line range hint
88-98: Avoid mixing production and sandbox URLs in development environment.The configuration uses both production (dev.abdm.gov.in) and sandbox (abhasbx.abdm.gov.in) URLs. This mixing of environments could lead to unexpected behavior during development and testing.
Consider using consistent sandbox URLs for the development environment:
-abdmV3UserAuthenticate = https://dev.abdm.gov.in/api/hiecm/gateway/v3/sessions +abdmV3UserAuthenticate = https://abhasbx.abdm.gov.in/api/hiecm/gateway/v3/sessionssrc/main/environment/common_test.properties (1)
Standardize test environment URLs to use sandbox endpoints consistently
The review comment is correct. The test configuration file mixes development and sandbox URLs, which could lead to integration issues. This is particularly concerning given that
abhaMode=sbxindicates sandbox environment should be used.URLs that need to be changed to sandbox endpoints:
abdmV3UserAuthenticate: Change from dev to sandboxndhmuserAuthenticate: Change from dev to sandboxgenerateOTPForCareContext: Change from dev to sandboxvalidateOTPForCareContext: Change from dev to sandboxgenerateABDM_NotifySMS: Change from dev to sandboxgetAbdmFacilityServicies: Change from dev to sandboxAll ABDM endpoints in the test environment should consistently use the sandbox domain (abhasbx.abdm.gov.in or healthidsbx.abdm.gov.in) to ensure proper integration testing.
🔗 Analysis chain
Line range hint
88-98: Standardize environment configuration for test environment.Similar to the development environment, the test configuration mixes production and sandbox URLs. Additionally, consider adding validation for these endpoints in the test suite.
- Standardize URLs to use sandbox environment:
-abdmV3UserAuthenticate = https://dev.abdm.gov.in/api/hiecm/gateway/v3/sessions +abdmV3UserAuthenticate = https://abhasbx.abdm.gov.in/api/hiecm/gateway/v3/sessions
- Verify the endpoints with this script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all ABHA V3 endpoints are accessible in test environment echo "Checking ABHA V3 endpoints accessibility..." for endpoint in \ "https://abhasbx.abdm.gov.in/api/hiecm/gateway/v3/sessions" \ "https://abhasbx.abdm.gov.in/api/v1/auth/cert" \ "https://abhasbx.abdm.gov.in/abha/api/v3/enrollment/request/otp" do response=$(curl -s -o /dev/null -w "%{http_code}" $endpoint) if [ $response -eq 000 ]; then echo "Warning: Endpoint $endpoint is not accessible" fi doneLength of output: 386
Script:
#!/bin/bash # Search for different environment URLs in property files echo "Checking for environment URLs in property files..." echo -e "\nFiles containing 'dev.abdm.gov.in':" rg "dev\.abdm\.gov\.in" --type properties echo -e "\nFiles containing 'abhasbx.abdm.gov.in':" rg "abhasbx\.abdm\.gov\.in" --type properties echo -e "\nFiles containing other ABDM domains:" rg "abdm\.gov\.in" --type properties | grep -v "dev\." | grep -v "abhasbx\."Length of output: 669
Script:
#!/bin/bash # Find all .properties files and search for ABDM URLs echo "Searching .properties files for ABDM URLs..." # Find all .properties files properties_files=$(fd -e properties) if [ -n "$properties_files" ]; then echo -e "\nFiles containing 'dev.abdm.gov.in':" echo "$properties_files" | xargs grep -l "dev.abdm.gov.in" 2>/dev/null echo -e "\nFiles containing 'abhasbx.abdm.gov.in':" echo "$properties_files" | xargs grep -l "abhasbx.abdm.gov.in" 2>/dev/null echo -e "\nFull content of common_test.properties:" cat src/main/environment/common_test.properties 2>/dev/null else echo "No .properties files found" fiLength of output: 6923
🧹 Nitpick comments (4)
src/main/java/com/wipro/fhir/service/v3/abha/CertificateKeyServiceImpl.java (1)
45-54: Centralize header creation and reuse across multiple calls.
You are repeatedly building headers with similar entries. Consider extracting a helper method that populates standard headers—improving consistency and reducing the risk of missing or duplicating headers in other parts of the code.src/main/java/com/wipro/fhir/service/v3/abha/CreateAbhaV3ServiceImpl.java (3)
106-106: Consider caching the public key to reduce API calls.The
getCertPublicKeymethod is called multiple times with the samendhmAuthToken. Consider caching the public key for a specific token to avoid redundant API calls, while ensuring proper cache invalidation when the token expires.@Service public class CreateAbhaV3ServiceImpl implements CreateAbhaV3Service { + private Map<String, CachedPublicKey> publicKeyCache = new ConcurrentHashMap<>(); + + @Value("${public.key.cache.duration:300000}") // 5 minutes default + private long publicKeyCacheDuration; + + private String getCachedPublicKey(String ndhmAuthToken) { + CachedPublicKey cached = publicKeyCache.get(ndhmAuthToken); + if (cached != null && !cached.isExpired()) { + return cached.getKey(); + } + String newKey = certificateKeyService.getCertPublicKey(ndhmAuthToken); + publicKeyCache.put(ndhmAuthToken, new CachedPublicKey(newKey, publicKeyCacheDuration)); + return newKey; + } + + private static class CachedPublicKey { + private final String key; + private final long expiryTime; + + public CachedPublicKey(String key, long cacheDuration) { + this.key = key; + this.expiryTime = System.currentTimeMillis() + cacheDuration; + } + + public String getKey() { + return key; + } + + public boolean isExpired() { + return System.currentTimeMillis() > expiryTime; + } + }Also applies to: 174-174, 265-265
Line range hint
63-67: Refactor timestamp generation to reduce code duplication.The timestamp generation logic is duplicated across multiple methods. Consider extracting it into a utility method.
+ private String generateUTCTimestamp() { + TimeZone tz = TimeZone.getTimeZone("UTC"); + DateFormat df = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSS'Z'"); + df.setTimeZone(tz); + return df.format(new Date()); + } + @Override public String getOtpForEnrollment(String request) throws FHIRException { // ... - TimeZone tz = TimeZone.getTimeZone("UTC"); - DateFormat df = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSS'Z'"); - df.setTimeZone(tz); - String nowAsISO = df.format(new Date()); + String nowAsISO = generateUTCTimestamp();Also applies to: 156-160, 252-256
Line range hint
58-59: Consider using constructor injection for required dependencies.The class has multiple required dependencies that are autowired. Consider using constructor injection to make dependencies explicit and improve testability.
@Service public class CreateAbhaV3ServiceImpl implements CreateAbhaV3Service { - @Autowired - private GenerateSession_NDHMService generateSession_NDHM; - @Autowired - private Common_NDHMService common_NDHMService; - @Autowired - HttpUtils httpUtils; - @Autowired - Encryption encryption; - @Autowired - HealthIDRepo healthIDRepo; - @Autowired - CertificateKeyService certificateKeyService; + private final GenerateSession_NDHMService generateSession_NDHM; + private final Common_NDHMService common_NDHMService; + private final HttpUtils httpUtils; + private final Encryption encryption; + private final HealthIDRepo healthIDRepo; + private final CertificateKeyService certificateKeyService; + + public CreateAbhaV3ServiceImpl( + GenerateSession_NDHMService generateSession_NDHM, + Common_NDHMService common_NDHMService, + HttpUtils httpUtils, + Encryption encryption, + HealthIDRepo healthIDRepo, + CertificateKeyService certificateKeyService) { + this.generateSession_NDHM = generateSession_NDHM; + this.common_NDHMService = common_NDHMService; + this.httpUtils = httpUtils; + this.encryption = encryption; + this.healthIDRepo = healthIDRepo; + this.certificateKeyService = certificateKeyService; + }Also applies to: 61-62
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/environment/common_ci.properties(1 hunks)src/main/environment/common_dev.properties(1 hunks)src/main/environment/common_example.properties(1 hunks)src/main/environment/common_test.properties(1 hunks)src/main/java/com/wipro/fhir/service/v3/abha/CertificateKeyService.java(1 hunks)src/main/java/com/wipro/fhir/service/v3/abha/CertificateKeyServiceImpl.java(1 hunks)src/main/java/com/wipro/fhir/service/v3/abha/CreateAbhaV3ServiceImpl.java(3 hunks)src/main/java/com/wipro/fhir/service/v3/abha/LoginAbhaV3ServiceImpl.java(2 hunks)
🧰 Additional context used
📓 Learnings (1)
src/main/java/com/wipro/fhir/service/v3/abha/LoginAbhaV3ServiceImpl.java (1)
Learnt from: helenKaryamsetty
PR: PSMRI/FHIR-API#52
File: src/main/java/com/wipro/fhir/service/v3/abha/LoginAbhaV3ServiceImpl.java:84-91
Timestamp: 2024-12-09T16:01:39.809Z
Learning: In the `requestOtpForAbhaLogin` and `verifyAbhaLogin` methods of `LoginAbhaV3ServiceImpl.java`, `loginId` is mandatory, but even if it is null, the process can proceed without input validation.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (4)
src/main/java/com/wipro/fhir/service/v3/abha/CertificateKeyServiceImpl.java (2)
40-40: Validate the authorization token's presence and format.
The updated method signature includesndhmAuthToken. Consider validating that the token is neither null nor empty before proceeding, to avoid unexpectedNullPointerExceptionor authentication failures.
58-63: 🛠️ Refactor suggestionCheck for missing 'publicKey' field and non-200 status codes.
Currently, the code retrievespublicKeyassuming it always exists in the response for a 200 status. If the field is missing or the status code differs, the method returnsnullwithout logging or throwing an exception. This could lead to unhandled null references downstream. Consider rigorous error handling and logging for non-200 responses or malformed responses.src/main/java/com/wipro/fhir/service/v3/abha/CertificateKeyService.java (1)
7-7: Document the new parameter in the interface.
ThendhmAuthTokenparameter changes the contract. Update the method’s documentation to explain the usage and necessity of this token so that implementations and consumers can properly comply.src/main/java/com/wipro/fhir/service/v3/abha/LoginAbhaV3ServiceImpl.java (1)
93-93: 🛠️ Refactor suggestionPossibility of null or empty login IDs and tokens.
Lines 93 and 184 now retrieve the public key viagetCertPublicKey(ndhmAuthToken), but there is no explicit validation ofndhmAuthToken, nor is there a guard against a null or emptyloginId. IfloginIdis null or empty, encryption and subsequent processes could fail without clear error messages.Also applies to: 184-184
📋 Description
JIRA ID:
Please provide a summary of the change and the motivation behind it. Include relevant context and details.
✅ Type of Change
ℹ️ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.
Summary by CodeRabbit
New Features
Improvements