ABHA - Address Search API changes #56
Conversation
WalkthroughThe changes in this pull request involve the addition of new properties related to web login functionality for the ABHA API across multiple configuration files. Specifically, two new properties, Changes
Possibly related PRs
Suggested reviewers
Poem
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: 3
🧹 Outside diff range and nitpick comments (3)
src/main/java/com/wipro/fhir/service/v3/abha/LoginAbhaV3ServiceImpl.java (3)
Line range hint
99-125: Fix incorrect conditions inif-elseblocksThere are redundant or incorrect conditions in the
if-elsestatements:
At line 115, the condition compares
loginMethodwith itself:} else if ("mobile".equalsIgnoreCase(loginAbhaRequest.getLoginMethod()) && "mobile".equalsIgnoreCase(loginAbhaRequest.getLoginMethod())) {This should compare
loginMethodwithloginHint.Similarly, at line 119, the condition repeats the same comparison with "aadhaar":
} else if ("aadhaar".equalsIgnoreCase(loginAbhaRequest.getLoginMethod()) && "aadhaar".equalsIgnoreCase(loginAbhaRequest.getLoginMethod())) {Apply the following diff to correct the conditions:
- } else if ("mobile".equalsIgnoreCase(loginAbhaRequest.getLoginMethod()) && "mobile".equalsIgnoreCase(loginAbhaRequest.getLoginMethod())) { + } else if ("mobile".equalsIgnoreCase(loginAbhaRequest.getLoginMethod()) && "mobile".equalsIgnoreCase(loginAbhaRequest.getLoginHint())) { ... - } else if ("aadhaar".equalsIgnoreCase(loginAbhaRequest.getLoginMethod()) && "aadhaar".equalsIgnoreCase(loginAbhaRequest.getLoginMethod())) { + } else if ("aadhaar".equalsIgnoreCase(loginAbhaRequest.getLoginMethod()) && "aadhaar".equalsIgnoreCase(loginAbhaRequest.getLoginHint())) {
Line range hint
99-125: Consider refactoring conditional statements for maintainabilityThe series of
if-elseconditions can be simplified to reduce redundancy and improve readability. Consider mapping the combinations ofloginMethodandloginHintto their corresponding configurations.For example, using a
Map<Pair<String, String>, Configuration>can streamline the logic.
Line range hint
74-225: Input validation forloginIdinrequestOtpForAbhaLoginandverifyAbhaLoginBased on previous learnings,
loginIdis mandatory, but the process can proceed even if it is null due to lack of input validation. This could lead to unexpected behavior or errors.Add input validation to ensure
loginIdis not null or empty before proceeding.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
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/LoginAbhaV3ServiceImpl.java(6 hunks)
🧰 Additional context used
📓 Learnings (3)
src/main/environment/common_test.properties (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.
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.
src/main/environment/common_dev.properties (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.
🔇 Additional comments (8)
src/main/java/com/wipro/fhir/service/v3/abha/LoginAbhaV3ServiceImpl.java (2)
56-61: Addition of new configuration properties looks good
The new configuration properties webLoginAbhaRequestOtp and webLoginAbhaVerify are appropriately added.
207-211: Verify scopes in verifyAbhaLogin method
Ensure that the scopes set for "abha-mobile" and "abha-aadhaar" login methods are correct and consistent with the expected authentication flow.
Please confirm that "abha-address-login" is the intended scope for these login methods.
src/main/environment/common_ci.properties (1)
95-96: New properties added correctly
The properties webLoginAbhaRequestOtp and webLoginAbhaVerify have been added with the correct placeholders for the base URL.
src/main/environment/common_example.properties (1)
58-59: New properties added with correct URLs
The addition of webLoginAbhaRequestOtp and webLoginAbhaVerify with the specified URLs is appropriate.
src/main/environment/common_test.properties (1)
95-96: New properties added correctly
The new properties webLoginAbhaRequestOtp and webLoginAbhaVerify are appropriately added to support web login functionality.
src/main/environment/common_dev.properties (3)
95-96: Verify configuration consistency across environments.
The new properties should be consistently defined across all environment configurations (dev, test, ci, prod).
✅ Verification successful
Properties are consistently defined across relevant environment configurations
The verification shows that the webLoginAbhaRequestOtp and webLoginAbhaVerify properties are properly defined in all environment-specific configuration files:
common_ci.properties(using environment variable)common_dev.propertiescommon_example.propertiescommon_test.properties
The properties are missing in application.properties and version.properties, but this is expected as these files serve different purposes and are not environment-specific configuration files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the properties exist in all environment files
echo "Checking environment files for web login properties..."
for env_file in $(fd -e properties); do
echo "=== $env_file ==="
rg "webLoginAbhaRequestOtp|webLoginAbhaVerify" "$env_file" || echo "Properties missing in $env_file"
done
Length of output: 2535
95-96: Confirm sandbox URLs for development environment.
The URLs are currently pointing to the sandbox environment (abhasbx). This is consistent with abhaMode=sbx setting, but ensure this is intentional for the development environment.
✅ Verification successful
Sandbox URLs are correctly configured for development environment
The verification confirms that the sandbox URLs are intentionally configured for the development environment:
- All environment files (dev, test, ci) consistently use
abhaMode=sbx - The codebase has proper fallback logic in multiple services that defaults to
abhaMode="sbx"if the mode is invalid - The sandbox URLs (abhasbx.abdm.gov.in) are consistently used across all environment property files
- The implementation validates that
abhaModecan only be "abdm" or "sbx"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the abhaMode setting and URL patterns across files
echo "Checking ABHA mode and URL patterns..."
rg "abhaMode|abhasbx"
Length of output: 6035
95-96: 💡 Codebase verification
Input validation is present but could be strengthened
The code shows basic validation is implemented:
- Null checks for
loginId - Login method and hint validation with specific error messages
- Exception handling for invalid responses
However, there are opportunities for improvement:
- No validation for empty strings or malformed input
- Missing validation annotations (@NotNull, @notblank) on the request object
- Validation logic is scattered across the implementation
🔗 Analysis chain
Verify input validation in the web login implementation.
Based on previous learnings from PR #52, ensure proper input validation is implemented in the corresponding service methods that use these new web login endpoints. The related LoginAbhaV3ServiceImpl has known issues with null loginId handling.
Consider implementing proper input validation at the service layer to prevent potential security issues.
Let me gather more information about the input validation in LoginAbhaV3ServiceImpl.
Let me search for the input validation logic in a different way.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for web login related implementations to verify input validation
ast-grep --pattern 'webLoginAbhaRequestOtp|webLoginAbhaVerify'
# Search for potential null checks in the implementation
rg -A 5 'webLoginAbhaRequestOtp|webLoginAbhaVerify'
Length of output: 4902
Script:
#!/bin/bash
# Search for method implementations in LoginAbhaV3ServiceImpl that use these endpoints
ast-grep --pattern 'class LoginAbhaV3ServiceImpl {
$$$
$method($$$) {
$$$
webLoginAbhaRequestOtp
$$$
}
$$$
}'
# Look for input validation patterns around these endpoints
rg -B 10 -A 2 'webLoginAbhaRequestOtp|webLoginAbhaVerify' src/main/java/com/wipro/fhir/service/v3/abha/LoginAbhaV3ServiceImpl.java
# Search for any validation utilities or annotations used
rg "@Valid|@NotNull|@NotBlank|StringUtils\.isEmpty|Objects\.isNull" src/main/java/com/wipro/fhir/service/v3/abha/LoginAbhaV3ServiceImpl.java
Length of output: 2015
Script:
#!/bin/bash
# Search for the method that handles the login request
ast-grep --pattern 'class LoginAbhaV3ServiceImpl {
$$$
public $_ loginAbha($_) {
$$$
}
}'
# Look for validation or error handling patterns
rg -B 5 -A 5 "throw new|if.*null|isEmpty|invalid" src/main/java/com/wipro/fhir/service/v3/abha/LoginAbhaV3ServiceImpl.java
# Search for the request object structure
rg -A 10 "class LoginAbhaRequest|class ReqOtpEnrollment" --type java
Length of output: 3843
⛔ Skipped due to learnings
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.
| @@ -65,6 +71,7 @@ public String requestOtpForAbhaLogin(String request) throws FHIRException { | |||
| RestTemplate restTemplate = new RestTemplate(); | |||
| String encryptedLoginId = null; | |||
| String publicKeyString = null; | |||
| ResponseEntity<String> responseEntity; | |||
There was a problem hiding this comment.
Ensure proper initialization of responseEntity
The variable responseEntity is declared but may not be initialized in all code paths before use. Verify that it is properly assigned before any operation that depends on it.
| if("abha-aadhaar".equalsIgnoreCase(loginData.getLoginMethod()) || "abha-mobile".equalsIgnoreCase(loginData.getLoginMethod())) { | ||
| responseEntity = restTemplate.exchange(webLoginAbhaVerify, HttpMethod.POST, | ||
| httpEntity, String.class); | ||
| } else { | ||
| responseEntity = restTemplate.exchange(verifyAbhaLoginUrl, HttpMethod.POST, | ||
| httpEntity, String.class); | ||
| } |
There was a problem hiding this comment.
Potential issue with conditional logic in response handling
At line 220, the condition may not cover all necessary cases appropriately. Review the logic to ensure the correct URL endpoint is used based on loginData.getLoginMethod().
Consider revising the condition to match the handling in the requestOtpForAbhaLogin method for consistency.
| if("abha-address".equalsIgnoreCase(loginAbhaRequest.getLoginHint())) { | ||
| responseEntity = restTemplate.exchange(webLoginAbhaRequestOtp, HttpMethod.POST, | ||
| httpEntity, String.class); | ||
| } else { | ||
| responseEntity = restTemplate.exchange(abhaLoginRequestOtp, HttpMethod.POST, | ||
| httpEntity, String.class); | ||
| } |
There was a problem hiding this comment.
Null check for loginHint before comparison
Before comparing loginAbhaRequest.getLoginHint() with "abha-address", ensure that loginHint is not null to prevent potential NullPointerException.
Apply this diff to add null checks:
+ if ("abha-address".equalsIgnoreCase(StringUtils.defaultString(loginAbhaRequest.getLoginHint()))) {📝 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.
| if("abha-address".equalsIgnoreCase(loginAbhaRequest.getLoginHint())) { | |
| responseEntity = restTemplate.exchange(webLoginAbhaRequestOtp, HttpMethod.POST, | |
| httpEntity, String.class); | |
| } else { | |
| responseEntity = restTemplate.exchange(abhaLoginRequestOtp, HttpMethod.POST, | |
| httpEntity, String.class); | |
| } | |
| if("abha-address".equalsIgnoreCase(StringUtils.defaultString(loginAbhaRequest.getLoginHint()))) { | |
| responseEntity = restTemplate.exchange(webLoginAbhaRequestOtp, HttpMethod.POST, | |
| httpEntity, String.class); | |
| } else { | |
| responseEntity = restTemplate.exchange(abhaLoginRequestOtp, HttpMethod.POST, | |
| httpEntity, String.class); | |
| } |
📋 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
Bug Fixes
abdmVerifyOTPproperty to ensure proper URL structure.Documentation