Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AMM-1048 | Assam Pre Prod in MO Save and Frequency is not working #77

Merged
merged 7 commits into from
Nov 20, 2024
Merged

AMM-1048 | Assam Pre Prod in MO Save and Frequency is not working #77

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6f448ac
Merge pull request #1 from PSMRI/develop
srishtigrp78 Sep 18, 2024
1b7f584
Merge pull request #4 from PSMRI/develop
srishtigrp78 Nov 14, 2024
576bcdd
adding changes for getByBenRegIdList API
Nov 19, 2024
fef5c2d
added changes suggested by sonar cloud analysis
Nov 19, 2024
866ec9f
removing commented code
Nov 19, 2024
2be7f43
making sure to not log user controlled data
Nov 19, 2024
4edcf18
adding changes related to class Casting and removing new method
Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 22 additions & 18 deletions src/main/java/com/iemr/common/identity/controller/IdentityController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,26 +697,30 @@
@Operation(summary = "Get beneficiaries by beneficiary registration id")
@PostMapping(path = "/getByBenRegIdList", headers = "Authorization")
public String getBeneficiariesByBenRegIds(
@Param(value = " {\"beneficiaryRegID\": \"Long\"}") @RequestBody String benRegIds) {
logger.info("IdentityController.getBeneficiariesByBenRegIds - start. benRegIdList = " + benRegIds);
BigInteger[] benRegIdarray = null;
JsonElement json = JsonParser.parseString(benRegIds);

if (json instanceof JsonNull) {
return getErrorResponseString("Null/Empty Phone Number.", 200, "success", "");
}

benRegIdarray = InputMapper.getInstance().gson().fromJson(json, BigInteger[].class);

List<BeneficiariesDTO> list = svc.getBeneficiariesDeatilsByBenRegIdList(Arrays.asList(benRegIdarray));
list.removeIf(Objects::isNull);
Collections.sort(list);
String response = getSuccessResponseString(list, 200, "success", "getBeneficiariesByBenRegIds");

logger.info("IdentityController.getBeneficiariesByBenRegIds - end : ");
return response;
@RequestBody Long[] benRegIds) { // Accepting an array of Longs directly
logger.info("IdentityController.getBeneficiariesByBenRegIds - start. benRegIdList = " + benRegIds.length);

Comment on lines +701 to +702
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Security: Fix log injection vulnerability

The current logging implementation directly interpolates user input into the log message, which could be exploited for log injection attacks.

Apply this diff to sanitize the log message:

-logger.info("IdentityController.getBeneficiariesByBenRegIds - start. benRegIdList = " + benRegIds.length);
+logger.info("IdentityController.getBeneficiariesByBenRegIds - start. benRegIdList size = {}", benRegIds.length);
📝 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.

Suggested change
logger.info("IdentityController.getBeneficiariesByBenRegIds - start. benRegIdList = " + benRegIds.length);
logger.info("IdentityController.getBeneficiariesByBenRegIds - start. benRegIdList size = {}", benRegIds.length);
🧰 Tools
🪛 GitHub Check: SonarCloud

[notice] 702-702: Logging should not be vulnerable to injection attacks

Change this code to not log user-controlled data.

See more on SonarQube Cloud

// If benRegIds is null or empty, return an error response
if (benRegIds == null || benRegIds.length == 0) {
return getErrorResponseString("No beneficiary registration IDs provided", 400, "error", "Array is empty");
}

// Convert the Long[] to BigInteger[] for further processing
BigInteger[] benRegIdArray = Arrays.stream(benRegIds)
.map(BigInteger::valueOf)
.toArray(BigInteger[]::new);

List<BeneficiariesDTO> list = svc.getBeneficiariesDeatilsByBenRegIdList(Arrays.asList(benRegIdArray));
list.removeIf(Objects::isNull);
Collections.sort(list);

String response = getSuccessResponseString(list, 200, "success", "getBeneficiariesByBenRegIds");

logger.info("IdentityController.getBeneficiariesByBenRegIds - end : ");
return response;
Comment on lines +713 to +720
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding error handling for service layer exceptions

The service call and response handling could benefit from more robust error handling.

Apply this diff to improve error handling:

-    List<BeneficiariesDTO> list = svc.getBeneficiariesDeatilsByBenRegIdList(Arrays.asList(benRegIdArray));
-    list.removeIf(Objects::isNull);
-    Collections.sort(list);
-
-    String response = getSuccessResponseString(list, 200, "success", "getBeneficiariesByBenRegIds");
-
-    logger.info("IdentityController.getBeneficiariesByBenRegIds - end : ");
-    return response;
+    try {
+        List<BeneficiariesDTO> list = svc.getBeneficiariesDeatilsByBenRegIdList(Arrays.asList(benRegIdArray));
+        if (list == null) {
+            return getErrorResponseString("No beneficiaries found", 404, "not_found", "getBeneficiariesByBenRegIds");
+        }
+        list.removeIf(Objects::isNull);
+        Collections.sort(list);
+
+        String response = getSuccessResponseString(list, 200, "success", "getBeneficiariesByBenRegIds");
+        logger.info("IdentityController.getBeneficiariesByBenRegIds - end");
+        return response;
+    } catch (Exception e) {
+        logger.error("Error retrieving beneficiaries: {}", e.getMessage(), e);
+        return getErrorResponseString("Error retrieving beneficiaries", 500, "error", "getBeneficiariesByBenRegIds");
+    }
📝 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.

Suggested change
List<BeneficiariesDTO> list = svc.getBeneficiariesDeatilsByBenRegIdList(Arrays.asList(benRegIdArray));
list.removeIf(Objects::isNull);
Collections.sort(list);
String response = getSuccessResponseString(list, 200, "success", "getBeneficiariesByBenRegIds");
logger.info("IdentityController.getBeneficiariesByBenRegIds - end : ");
return response;
try {
List<BeneficiariesDTO> list = svc.getBeneficiariesDeatilsByBenRegIdList(Arrays.asList(benRegIdArray));
if (list == null) {
return getErrorResponseString("No beneficiaries found", 404, "not_found", "getBeneficiariesByBenRegIds");
}
list.removeIf(Objects::isNull);
Collections.sort(list);
String response = getSuccessResponseString(list, 200, "success", "getBeneficiariesByBenRegIds");
logger.info("IdentityController.getBeneficiariesByBenRegIds - end");
return response;
} catch (Exception e) {
logger.error("Error retrieving beneficiaries: {}", e.getMessage(), e);
return getErrorResponseString("Error retrieving beneficiaries", 500, "error", "getBeneficiariesByBenRegIds");
}

}


/**
* Overloaded method with string
*
Expand Down
26 changes: 25 additions & 1 deletion src/main/java/com/iemr/common/identity/service/IdentityService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -571,6 +571,30 @@ public List<BeneficiariesDTO> searhBeneficiaryByGovIdentity(String identity)
return beneficiaryList;
}

private MBeneficiarymapping getBeneficiariesDTONewForList(Object[] benMapArr) {
MBeneficiarymapping benMapOBJ = new MBeneficiarymapping();
if (benMapArr != null && benMapArr.length == 12 && benMapArr[8] != null && benMapArr[9] != null) {
benMapOBJ.setBenMapId(getBigIntegerValueFromObject(benMapArr[0]));
benMapOBJ.setCreatedBy(String.valueOf(benMapArr[10]));
benMapOBJ.setCreatedDate((Timestamp) benMapArr[11]);
benMapOBJ = mappingRepo.getMapping(getBigIntegerValueFromObject(benMapArr[9]), (Integer) benMapArr[8]);

BigInteger regId = BigInteger.valueOf(((Long) benMapArr[5]).longValue());
RMNCHBeneficiaryDetailsRmnch obj = rMNCHBeneficiaryDetailsRmnchRepo.getByRegID(regId);

if (obj != null) {
if (obj.getHouseoldId() != null)
benMapOBJ.setHouseHoldID(obj.getHouseoldId());
if (obj.getGuidelineId() != null)
benMapOBJ.setGuideLineID(obj.getGuidelineId());
if (obj.getRchid() != null)
benMapOBJ.setRchID(obj.getRchid());
}

}
return benMapOBJ;
}
Copy link

@coderabbitai coderabbitai bot Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling and reduce code duplication

The method has several issues that need to be addressed:

  1. Unsafe type casting from Object to Long to BigInteger could throw ClassCastException
  2. Nested null checks can be simplified
  3. Similar implementation to getBeneficiariesDTONew - consider refactoring to reduce duplication

Consider applying these improvements:

+ /**
+  * Creates a MBeneficiarymapping object from an array of beneficiary data
+  * @param benMapArr Array containing beneficiary mapping data
+  * @return MBeneficiarymapping object with populated data
+  * @throws IllegalArgumentException if required data is missing or invalid
+  */
 private MBeneficiarymapping getBeneficiariesDTONewForList(Object[] benMapArr) {
     MBeneficiarymapping benMapOBJ = new MBeneficiarymapping();
-    if (benMapArr != null && benMapArr.length == 12 && benMapArr[8] != null && benMapArr[9] != null) {
+    if (benMapArr == null || benMapArr.length != 12 || benMapArr[8] == null || benMapArr[9] == null) {
+        throw new IllegalArgumentException("Invalid or incomplete beneficiary mapping data");
+    }
+    
+    try {
         benMapOBJ.setBenMapId(getBigIntegerValueFromObject(benMapArr[0]));
         benMapOBJ.setCreatedBy(String.valueOf(benMapArr[10]));
         benMapOBJ.setCreatedDate((Timestamp) benMapArr[11]);
         benMapOBJ = mappingRepo.getMapping(getBigIntegerValueFromObject(benMapArr[9]), (Integer) benMapArr[8]);
         
-        BigInteger regId = BigInteger.valueOf(((Long) benMapArr[5]).longValue());
+        BigInteger regId = getBigIntegerValueFromObject(benMapArr[5]);
         RMNCHBeneficiaryDetailsRmnch obj = rMNCHBeneficiaryDetailsRmnchRepo.getByRegID(regId);
 
         if (obj != null) {
-            if (obj.getHouseoldId() != null)
-                benMapOBJ.setHouseHoldID(obj.getHouseoldId());
-            if (obj.getGuidelineId() != null)
-                benMapOBJ.setGuideLineID(obj.getGuidelineId());
-            if (obj.getRchid() != null)
-                benMapOBJ.setRchID(obj.getRchid());
+            benMapOBJ.setHouseHoldID(obj.getHouseoldId());
+            benMapOBJ.setGuideLineID(obj.getGuidelineId());
+            benMapOBJ.setRchID(obj.getRchid());
         }
-    }
+    } catch (ClassCastException e) {
+        throw new IllegalArgumentException("Invalid data type in beneficiary mapping array", e);
+    }
     return benMapOBJ;
 }

Additionally, consider extracting common code between this method and getBeneficiariesDTONew into a shared utility method to reduce duplication.

📝 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.

Suggested change
private MBeneficiarymapping getBeneficiariesDTONewForList(Object[] benMapArr) {
MBeneficiarymapping benMapOBJ = new MBeneficiarymapping();
if (benMapArr != null && benMapArr.length == 12 && benMapArr[8] != null && benMapArr[9] != null) {
benMapOBJ.setBenMapId(getBigIntegerValueFromObject(benMapArr[0]));
benMapOBJ.setCreatedBy(String.valueOf(benMapArr[10]));
benMapOBJ.setCreatedDate((Timestamp) benMapArr[11]);
benMapOBJ = mappingRepo.getMapping(getBigIntegerValueFromObject(benMapArr[9]), (Integer) benMapArr[8]);
BigInteger regId = BigInteger.valueOf(((Long) benMapArr[5]).longValue());
RMNCHBeneficiaryDetailsRmnch obj = rMNCHBeneficiaryDetailsRmnchRepo.getByRegID(regId);
if (obj != null) {
if (obj.getHouseoldId() != null)
benMapOBJ.setHouseHoldID(obj.getHouseoldId());
if (obj.getGuidelineId() != null)
benMapOBJ.setGuideLineID(obj.getGuidelineId());
if (obj.getRchid() != null)
benMapOBJ.setRchID(obj.getRchid());
}
}
return benMapOBJ;
}
/**
* Creates a MBeneficiarymapping object from an array of beneficiary data
* @param benMapArr Array containing beneficiary mapping data
* @return MBeneficiarymapping object with populated data
* @throws IllegalArgumentException if required data is missing or invalid
*/
private MBeneficiarymapping getBeneficiariesDTONewForList(Object[] benMapArr) {
MBeneficiarymapping benMapOBJ = new MBeneficiarymapping();
if (benMapArr == null || benMapArr.length != 12 || benMapArr[8] == null || benMapArr[9] == null) {
throw new IllegalArgumentException("Invalid or incomplete beneficiary mapping data");
}
try {
benMapOBJ.setBenMapId(getBigIntegerValueFromObject(benMapArr[0]));
benMapOBJ.setCreatedBy(String.valueOf(benMapArr[10]));
benMapOBJ.setCreatedDate((Timestamp) benMapArr[11]);
benMapOBJ = mappingRepo.getMapping(getBigIntegerValueFromObject(benMapArr[9]), (Integer) benMapArr[8]);
BigInteger regId = getBigIntegerValueFromObject(benMapArr[5]);
RMNCHBeneficiaryDetailsRmnch obj = rMNCHBeneficiaryDetailsRmnchRepo.getByRegID(regId);
if (obj != null) {
benMapOBJ.setHouseHoldID(obj.getHouseoldId());
benMapOBJ.setGuideLineID(obj.getGuidelineId());
benMapOBJ.setRchID(obj.getRchid());
}
} catch (ClassCastException e) {
throw new IllegalArgumentException("Invalid data type in beneficiary mapping array", e);
}
return benMapOBJ;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@srishtigrp78 As discussed in call Pls remove the new method to avoid duplicate code.
And make the changes like I suggested.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!


private MBeneficiarymapping getBeneficiariesDTONew(Object[] benMapArr) {
MBeneficiarymapping benMapOBJ = new MBeneficiarymapping();
if (benMapArr != null && benMapArr.length == 12 && benMapArr[8] != null && benMapArr[9] != null) {
Expand Down Expand Up @@ -1559,7 +1583,7 @@ public List<BeneficiariesDTO> getBeneficiariesDeatilsByBenRegIdList(List<BigInte
benMapObjArr = mappingRepo.getBenMappingByRegIDList(benRegIds);
if (benMapObjArr != null && !benMapObjArr.isEmpty()) {
for (Object[] objArr : benMapObjArr) {
MBeneficiarymapping benMap = this.getBeneficiariesDTONew(objArr);
MBeneficiarymapping benMap = this.getBeneficiariesDTONewForList(objArr);
list.add(this.getBeneficiariesDTO(benMap));
}
}
Expand Down
Loading