Release 3.6.0 #284
Release 3.6.0 #284
Conversation
fix IDENTITY_API_BASE_URL variable in docker file
Fix url issue
fix IDENTITY_API_BASE_URL variable in docker file
fix IDENTITY_API_BASE_URL variable in docker file
fix IDENTITY_API_BASE_URL variable in docker file
fix IDENTITY_API_BASE_URL variable in docker file
Fix identity varible issue
fix IDENTITY_API_BASE_URL variable in docker 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (4)
299-307: Enforce refresh-token rotation: revoke old JTI.Old refresh tokens remain valid until expiry, enabling reuse. Delete the old jti on rotation.
Apply:
// Generate and store a new refresh token (token rotation) String newRefreshToken = jwtUtil.generateRefreshToken(user.getUserName(), userId); String newJti = jwtUtil.getJtiFromToken(newRefreshToken); + // Revoke the old refresh token to prevent reuse + if (jti != null) { + redisTemplate.delete("refresh:" + jti); + } redisTemplate.opsForValue().set( "refresh:" + newJti, userId, jwtUtil.getRefreshTokenExpiration(), TimeUnit.MILLISECONDS );
561-586: Fix NPE on Authorization header; validate token and handle Bearer before extracting userId.authHeader.isEmpty() NPE when header is null. Also, token is not validated and Bearer prefix is ignored.
Apply:
- String authHeader = request.getHeader("Authorization"); - if (authHeader.isEmpty()) { + String authHeader = request.getHeader("Authorization"); + if (authHeader == null || authHeader.isBlank()) { // Try JWT token from header first - String jwtToken = request.getHeader("Jwttoken"); + String jwtToken = request.getHeader("Jwttoken"); + if (jwtToken == null) { + String bearer = request.getHeader("Authorization"); + if (bearer != null && bearer.startsWith("Bearer ")) { + jwtToken = bearer.substring(7); + } + } // If not in header, try cookie if (jwtToken == null) { Cookie[] cookies = request.getCookies(); if (cookies != null) { for (Cookie cookie : cookies) { if ("Jwttoken".equalsIgnoreCase(cookie.getName())) { jwtToken = cookie.getValue(); break; } } } } - if (jwtToken == null) { + if (jwtToken == null || jwtToken.isBlank()) { logger.warn("Authentication failed: no token found in header or cookies."); throw new IEMRException("Authentication failed. Please log in again."); } - // Extract user ID from the JWT token - String userId = jwtUtil.getUserIdFromToken(jwtToken); + // Validate token then extract userId + Claims claims = jwtUtil.validateToken(jwtToken); + if (claims == null) { + logger.warn("Authentication failed: invalid token provided."); + throw new IEMRException("Authentication failed. Please log in again."); + } + String userId = claims.get("userId", String.class);
1170-1188: Do not expose JWT via GET endpoint; undermines HttpOnly.Returning the JWT from cookies to the caller defeats HttpOnly protections and increases XSS/CSRF risk.
Apply one of:
- Remove the endpoint.
- Or restrict and validate: require Authorization Bearer + CSRF token and never echo cookie value.
Minimal safe action (remove):
- @GetMapping("/get-jwt-token") - public ResponseEntity<String> getJwtTokenFromCookie(HttpServletRequest httpRequest) { - // Retrieve the cookie named 'jwtToken' - Cookie[] cookies = httpRequest.getCookies(); - if (cookies != null) { - for (Cookie cookie : cookies) { - if ("Jwttoken".equalsIgnoreCase(cookie.getName())) { - String jwtToken = cookie.getValue(); - // Return the JWT token in the response - return ResponseEntity.ok(jwtToken); - } - } - } - // Return 404 if the token is not found in the cookies - return ResponseEntity.status(HttpStatus.NOT_FOUND).body("JWT token not found"); - }If you must keep it, confirm CORS is locked down and add CSRF validation; otherwise, remove.
I can provide a CSRF/nonce-based design if needed.
140-140: Stop logging plaintext passwords.Avoid logging sensitive data.
Apply:
- logger.info("userAuthenticate request - " + m_User + " " + m_User.getUserName() + " " + m_User.getPassword()); + logger.info("userAuthenticate request - userName={}", m_User != null ? m_User.getUserName() : "null");Also sanitize similar logs elsewhere.
🧹 Nitpick comments (3)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (2)
83-85: Provide a safe default for enableCaptcha to avoid startup failure.If captcha.enable-captcha is missing in config, the app fails to start. Inject a default.
Apply:
- @Value("${captcha.enable-captcha}") + @Value("${captcha.enable-captcha:false}") private boolean enableCaptcha;
766-771: Typo in operation summary and method naming.“Get security quetions” and getSecurityts are misspelled. Rename for clarity.
- @Operation(summary = "Get security quetions") - @RequestMapping(value = "/getsecurityquetions", method = RequestMethod.GET) - public String getSecurityts() { + @Operation(summary = "Get security questions") + @RequestMapping(value = "/getSecurityQuestions", method = RequestMethod.GET) + public String getSecurityQuestions() {Note: Update callers accordingly.
src/main/java/com/iemr/common/utils/http/HTTPRequestInterceptor.java (1)
142-142: Prefer Spring’s MediaType constant to avoid javax.ws.rs dependency coupling.Use org.springframework.http.MediaType.APPLICATION_JSON_VALUE.
Apply this diff:
- response.setContentType(MediaType.APPLICATION_JSON); + response.setContentType(org.springframework.http.MediaType.APPLICATION_JSON_VALUE);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java(12 hunks)src/main/java/com/iemr/common/utils/JwtUserIdValidationFilter.java(1 hunks)src/main/java/com/iemr/common/utils/http/HTTPRequestInterceptor.java(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-02-03T12:42:38.278Z
Learnt from: indraniBan
PR: PSMRI/Common-API#138
File: src/main/java/com/iemr/common/utils/JwtAuthenticationUtil.java:0-0
Timestamp: 2025-02-03T12:42:38.278Z
Learning: In the JwtAuthenticationUtil class, catching a generic Exception in the validateUserIdAndJwtToken method is acceptable as per the team's preference.
Applied to files:
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
📚 Learning: 2025-02-21T07:43:03.828Z
Learnt from: sandipkarmakar3
PR: PSMRI/Common-API#162
File: src/main/java/com/iemr/common/utils/CookieUtil.java:52-66
Timestamp: 2025-02-21T07:43:03.828Z
Learning: In the Common-API project's CookieUtil class, the current implementation of addJwtTokenToCookie using both response.addCookie() and manual Set-Cookie header has been tested and confirmed to work as required for their use case. URL encoding of the JWT token is not necessary.
Applied to files:
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
🔇 Additional comments (6)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (4)
173-176: LGTM: Concurrency check gated by withCredentials.This aligns the check with cookie-based sessions. Verify all clients set withCredentials true when using cookies.
288-291: Confirm requirement: allow token refresh for “New” users.Treating “New” as active changes auth semantics. Ensure this matches policy and doesn’t enable refresh for un-onboarded/blocked users.
951-972: Harden forceLogout: accept tokens from headers, null-safe claims, guard TTL.Current flow ignores headers, risks NPE on claims.isEmpty(), and may add expired tokens with negative TTL.
Apply:
- // Extract token from cookies or headers - String token = getJwtTokenFromCookies(httpRequest); - if (token == null) { - response.setStatus(HttpServletResponse.SC_BAD_REQUEST); - outputResponse.setError(new RuntimeException("No JWT token found in request")); - return outputResponse.toString(); - } + // Extract token from cookies or headers + String token = getJwtTokenFromCookies(httpRequest); + if (token == null) { + token = httpRequest.getHeader(Constants.JWT_TOKEN); + } + if (token == null) { + String authz = httpRequest.getHeader("Authorization"); + if (authz != null && authz.startsWith("Bearer ")) { + token = authz.substring(7); + } + } + if (token == null || token.isBlank()) { + response.setStatus(HttpServletResponse.SC_BAD_REQUEST); + outputResponse.setError(new RuntimeException("No JWT token found in request")); + return outputResponse.toString(); + } // Validate the token: Check if it is expired or in the deny list Claims claims = jwtUtil.validateToken(token); - if (claims.isEmpty() || claims.getExpiration() == null || claims.getId() == null) { // If token is either expired or in the deny list, return 401 Unauthorized + if (claims == null || claims.getExpiration() == null || claims.getId() == null) { response.setStatus(HttpServletResponse.SC_UNAUTHORIZED); outputResponse.setError(new RuntimeException("Token is expired or has been logged out")); return outputResponse.toString(); } // Extract the jti (JWT ID) and expiration time from the validated claims String jti = claims.getId(); // jti is in the 'id' field of claims long expirationTime = claims.getExpiration().getTime(); // Use expiration from claims long ttlMillis = expirationTime - System.currentTimeMillis(); - tokenDenylist.addTokenToDenylist(jti, ttlMillis); + if (ttlMillis > 0) { + tokenDenylist.addTokenToDenylist(jti, ttlMillis); + }
983-991: JWT cookie name is consistent Constants.JWT_TOKEN is defined as"Jwttoken"and matches the literal used inaddJwtTokenToCookieandgetJwtTokenFromCookies.src/main/java/com/iemr/common/utils/http/HTTPRequestInterceptor.java (2)
38-38: Import is correct for Boot 3 (jakarta). LGTM.*No issues with using jakarta.servlet.ServletOutputStream.
141-151: Fix CORS on error path: avoid "*", add Vary, and drop manual Content-Length.Using "*" breaks credentialed requests (JWT cookie) and weakens security. Also, let the container handle Content-Length. Set headers before writing body (you already do) and echo only an allowed Origin from config.
Apply this diff:
- response.setStatus(HttpServletResponse.SC_UNAUTHORIZED); // 401 - response.setContentType(MediaType.APPLICATION_JSON); - response.setHeader("Access-Control-Allow-Origin", "*"); - - // Better to use getBytes().length for accurate byte size - byte[] responseBytes = jsonErrorResponse.getBytes(StandardCharsets.UTF_8); - response.setContentLength(responseBytes.length); - - ServletOutputStream out = response.getOutputStream(); - out.write(responseBytes); - out.flush(); + response.setStatus(HttpServletResponse.SC_UNAUTHORIZED); // 401 + response.setContentType(MediaType.APPLICATION_JSON); + response.setCharacterEncoding(StandardCharsets.UTF_8.name()); + String allowOrigin = resolveAllowedOrigin(request); + if (allowOrigin != null) { + response.setHeader("Access-Control-Allow-Origin", allowOrigin); + if (!"*".equals(allowOrigin)) { + response.addHeader("Vary", "Origin"); + } + } + try (ServletOutputStream out = response.getOutputStream()) { + out.write(jsonErrorResponse.getBytes(StandardCharsets.UTF_8)); + out.flush(); + }Add these members/imports outside the shown lines:
// imports import org.springframework.beans.factory.annotation.Value; // field @Value("${cors.allowed-origins:}") private String allowedOrigins; // helper private String resolveAllowedOrigin(HttpServletRequest request) { String origin = request.getHeader("Origin"); if (origin == null || origin.isEmpty()) return null; if (allowedOrigins == null || allowedOrigins.isEmpty()) return null; String cfg = allowedOrigins.trim(); if ("*".equals(cfg)) return "*"; for (String allowed : cfg.split(",")) { if (origin.equals(allowed.trim())) return origin; } return null; }#!/bin/bash # Verify CORS config presence/keys and other places setting ACAO rg -nP -C2 'cors\.allowed-origin|cors\.allowed-origins|CorsConfig|CorsFilter|Access-Control-Allow-Origin' --hidden
| String errorMessage = e.getMessage(); | ||
| if (errorMessage == null || errorMessage.trim().isEmpty()) { | ||
| errorMessage = "Unauthorized access or session expired."; | ||
| } | ||
|
|
||
| String jsonErrorResponse = "{" | ||
| + "\"status\": \"Unauthorized\"," | ||
| + "\"statusCode\": 401," | ||
| + "\"errorMessage\": \"" + errorMessage.replace("\"", "\\\"") + "\"" | ||
| + "}"; |
There was a problem hiding this comment.
Do not leak exception details; build JSON safely (use Jackson) and keep a generic client message.
Embedding raw exception messages risks information disclosure and brittle string-escaped JSON. Use ObjectMapper and a fixed client-facing message.
Apply this diff:
- String errorMessage = e.getMessage();
- if (errorMessage == null || errorMessage.trim().isEmpty()) {
- errorMessage = "Unauthorized access or session expired.";
- }
-
- String jsonErrorResponse = "{"
- + "\"status\": \"Unauthorized\","
- + "\"statusCode\": 401,"
- + "\"errorMessage\": \"" + errorMessage.replace("\"", "\\\"") + "\""
- + "}";
+ final String clientMessage = "Unauthorized access or session expired.";
+ String jsonErrorResponse = OBJECT_MAPPER.writeValueAsString(
+ java.util.Map.of(
+ "status", "Unauthorized",
+ "statusCode", 401,
+ "errorMessage", clientMessage
+ )
+ );Add these outside the shown lines:
import com.fasterxml.jackson.databind.ObjectMapper;
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/utils/http/HTTPRequestInterceptor.java around
lines 130 to 139, the code currently inserts raw exception messages into a
hand-built JSON string which risks leaking internals and brittle escaping;
replace that block to always return a generic client-facing message (e.g.,
"Unauthorized access or session expired.") and build the JSON using Jackson: add
the suggested import and a private static final ObjectMapper OBJECT_MAPPER = new
ObjectMapper(); outside the shown lines, then create a small Map<String,Object>
(or a simple POJO) with keys status="Unauthorized", statusCode=401,
errorMessage=<fixed message> and call OBJECT_MAPPER.writeValueAsString(map) to
produce the JSON string; ensure you do not include e.getMessage() or other
exception details in the response.
| || path.startsWith(contextPath + "/user/logOutUserFromConcurrentSession") | ||
| || path.startsWith(contextPath + "/user/refreshToken"); |
There was a problem hiding this comment.
Keep refresh/logout skip checks exact to avoid auth bypass
We already have exact matches for /user/logOutUserFromConcurrentSession and /user/refreshToken above. Adding startsWith(...) broadens the skip-list so that any path beginning with those segments bypasses JWT validation (e.g. /user/refreshToken/rotate), re‑introducing the security hole we’ve been trying to close. Please drop these two new predicates and keep the stricter equality checks.
- || path.startsWith(contextPath + "/user/logOutUserFromConcurrentSession")
- || path.startsWith(contextPath + "/user/refreshToken");📝 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.
| || path.startsWith(contextPath + "/user/logOutUserFromConcurrentSession") | |
| || path.startsWith(contextPath + "/user/refreshToken"); |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/utils/JwtUserIdValidationFilter.java around
lines 172-173, the two conditions using path.startsWith(contextPath +
"/user/logOutUserFromConcurrentSession") and path.startsWith(contextPath +
"/user/refreshToken") broaden the skip-list and allow auth bypass; remove these
two startsWith predicates and rely on the existing exact equality checks above
(or replace them with explicit equals(...) checks) so only the exact endpoints
are skipped.
fix: missing env added
added missing envs for bengen
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (2)
271-275: Rotate refresh tokens securely: revoke the old JTI.Token rotation currently issues a new refresh token but keeps the old one valid in Redis. Revoke the old JTI before/when storing the new one.
- // Check revocation using JTI - String jti = claims.getId(); - if (!redisTemplate.hasKey("refresh:" + jti)) { + // Check revocation using JTI (old token) + String oldJti = claims.getId(); + if (!redisTemplate.hasKey("refresh:" + oldJti)) { logger.warn("Token validation failed: refresh token is revoked or not found in store."); return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body("Unauthorized."); } @@ - // Generate and store a new refresh token (token rotation) + // Generate and store a new refresh token (token rotation) + // Revoke the old refresh token + redisTemplate.delete("refresh:" + oldJti); String newRefreshToken = jwtUtil.generateRefreshToken(user.getUserName(), userId); String newJti = jwtUtil.getJtiFromToken(newRefreshToken); redisTemplate.opsForValue().set( "refresh:" + newJti, userId, jwtUtil.getRefreshTokenExpiration(), TimeUnit.MILLISECONDS );Also applies to: 299-307
140-140: Remove plaintext password from logs.Logging credentials is a compliance and security risk. Strip or mask the password.
- logger.info("userAuthenticate request - " + m_User + " " + m_User.getUserName() + " " + m_User.getPassword()); + logger.info("userAuthenticate request - userName={}", m_User.getUserName());- logger.info("userAuthenticate request - " + m_User + " " + m_User.getUserName() + " " + m_User.getPassword()); + logger.info("userAuthenticate request - userName={}", m_User.getUserName());Also applies to: 1121-1121
🧹 Nitpick comments (2)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (2)
1179-1181: Unify cookie name with Constants.JWT_TOKEN.Avoid string literals for cookie name to prevent drift across endpoints.
- if ("Jwttoken".equalsIgnoreCase(cookie.getName())) { + if (Constants.JWT_TOKEN.equalsIgnoreCase(cookie.getName())) {
380-380: Avoid System.out.println in server code; use logger at debug level.Swap to logger.debug for consistency with other logging.
- System.out.println(mUser); + logger.debug("User mapping for ID={}", mUser != null ? mUser.getUserID() : null);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/environment/common_ci.properties(3 hunks)src/main/environment/common_docker.properties(2 hunks)src/main/environment/common_example.properties(1 hunks)src/main/java/com/iemr/common/controller/users/IEMRAdminController.java(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/environment/common_ci.properties
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-02-03T12:42:38.278Z
Learnt from: indraniBan
PR: PSMRI/Common-API#138
File: src/main/java/com/iemr/common/utils/JwtAuthenticationUtil.java:0-0
Timestamp: 2025-02-03T12:42:38.278Z
Learning: In the JwtAuthenticationUtil class, catching a generic Exception in the validateUserIdAndJwtToken method is acceptable as per the team's preference.
Applied to files:
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
📚 Learning: 2025-02-21T07:43:03.828Z
Learnt from: sandipkarmakar3
PR: PSMRI/Common-API#162
File: src/main/java/com/iemr/common/utils/CookieUtil.java:52-66
Timestamp: 2025-02-21T07:43:03.828Z
Learning: In the Common-API project's CookieUtil class, the current implementation of addJwtTokenToCookie using both response.addCookie() and manual Set-Cookie header has been tested and confirmed to work as required for their use case. URL encoding of the JWT token is not necessary.
Applied to files:
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
📚 Learning: 2025-05-09T12:55:44.224Z
Learnt from: vanitha1822
PR: PSMRI/Common-API#180
File: src/main/environment/common_example.properties:192-192
Timestamp: 2025-05-09T12:55:44.224Z
Learning: The video-call-url property for Jitsi integration is configured in local.properties file, while common_example.properties contains only a placeholder value.
Applied to files:
src/main/environment/common_docker.properties
🔇 Additional comments (7)
src/main/environment/common_example.properties (1)
211-212: Beneficiary ID endpoint config aligns with other environmentsThe added local URL matches the pattern introduced in the CI/Docker property sets, so no issues here.
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (6)
83-85: Provide a safe default for enableCaptcha to avoid startup failure.If the property is missing in some envs, app will fail to start. Add a default.
- @Value("${captcha.enable-captcha}") + @Value("${captcha.enable-captcha:false}") private boolean enableCaptcha;
173-176: Confirm intended gating of concurrent-session check with withCredentials.The check only runs when withCredentials == true. Verify this won’t skip protection for web/non-credential flows unintentionally.
288-291: LGTM: allow refresh for Active or New users.This aligns refresh with the onboarding state while still restricting disabled users.
566-587: Validate the JWT before extracting userId; use a single cookie/header name.Currently calls getUserIdFromToken on an unvalidated token and uses a hardcoded cookie/header name. Validate first and use Constants.JWT_TOKEN.
- if (authHeader.isEmpty()) { - // Try JWT token from header first - String jwtToken = request.getHeader("Jwttoken"); + if (authHeader.isEmpty()) { + // Try JWT token from header first + String jwtToken = request.getHeader(Constants.JWT_TOKEN); // If not in header, try cookie if (jwtToken == null) { Cookie[] cookies = request.getCookies(); if (cookies != null) { for (Cookie cookie : cookies) { - if ("Jwttoken".equalsIgnoreCase(cookie.getName())) { + if (Constants.JWT_TOKEN.equalsIgnoreCase(cookie.getName())) { jwtToken = cookie.getValue(); break; } } } } if (jwtToken == null) { logger.warn("Authentication failed: no token found in header or cookies."); throw new IEMRException("Authentication failed. Please log in again."); } - // Extract user ID from the JWT token - String userId = jwtUtil.getUserIdFromToken(jwtToken); + // Validate token first, then extract userId + Claims claims = jwtUtil.validateToken(jwtToken); + if (claims == null) { + logger.warn("Authentication failed: invalid token provided."); + throw new IEMRException("Authentication failed. Please log in again."); + } + String userId = claims.get("userId", String.class);
952-973: Harden forceLogout: accept tokens from headers, validate null-safely, and guard TTL.
- Accept token from cookies, JWT_TOKEN header, or Authorization: Bearer.
- Avoid NPE by null-checking claims.
- Only add to denylist with positive TTL.
- // Extract token from cookies or headers - String token = getJwtTokenFromCookies(httpRequest); - if (token == null) { - response.setStatus(HttpServletResponse.SC_BAD_REQUEST); - outputResponse.setError(new RuntimeException("No JWT token found in request")); - return outputResponse.toString(); - } + // Extract token from cookies or headers + String token = getJwtTokenFromCookies(httpRequest); + if (token == null) { + token = httpRequest.getHeader(Constants.JWT_TOKEN); + } + if (token == null) { + String authz = httpRequest.getHeader("Authorization"); + if (authz != null && authz.startsWith("Bearer ")) { + token = authz.substring(7); + } + } + if (token == null || token.isBlank()) { + response.setStatus(HttpServletResponse.SC_BAD_REQUEST); + outputResponse.setError(new RuntimeException("No JWT token found in request")); + return outputResponse.toString(); + } // Validate the token: Check if it is expired or in the deny list Claims claims = jwtUtil.validateToken(token); - if (claims.isEmpty() || claims.getExpiration() == null || claims.getId() == null) { // If token is either expired or in the deny list, return 401 Unauthorized + if (claims == null || claims.getExpiration() == null || claims.getId() == null) { response.setStatus(HttpServletResponse.SC_UNAUTHORIZED); outputResponse.setError(new RuntimeException("Token is expired or has been logged out")); return outputResponse.toString(); } // Extract the jti (JWT ID) and expiration time from the validated claims String jti = claims.getId(); // jti is in the 'id' field of claims long expirationTime = claims.getExpiration().getTime(); // Use expiration from claims long ttlMillis = expirationTime - System.currentTimeMillis(); - tokenDenylist.addTokenToDenylist(jti, ttlMillis); + if (ttlMillis > 0) { + tokenDenylist.addTokenToDenylist(jti, ttlMillis); + }
2-6: LGTM: license header updates.
| video.recording.path={VIDEO_RECORDING_PATH} | ||
| generateBeneficiaryIDs-api-url={GEN_BENEFICIARY_IDS_API_URL} |
There was a problem hiding this comment.
Fix placeholder syntax for new Docker properties
These entries use {VAR} instead of Spring’s ${VAR} syntax, so the deployed container will literally see {VIDEO_RECORDING_PATH} / {GEN_BENEFICIARY_IDS_API_URL} and fail to resolve the configured paths. Please switch to ${...} (and, while touching this block, fix the same issue for jibri.output.path on Line 196).
-video.recording.path={VIDEO_RECORDING_PATH}
-generateBeneficiaryIDs-api-url={GEN_BENEFICIARY_IDS_API_URL}
+video.recording.path=${VIDEO_RECORDING_PATH}
+generateBeneficiaryIDs-api-url=${GEN_BENEFICIARY_IDS_API_URL}📝 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.
| video.recording.path={VIDEO_RECORDING_PATH} | |
| generateBeneficiaryIDs-api-url={GEN_BENEFICIARY_IDS_API_URL} | |
| video.recording.path=${VIDEO_RECORDING_PATH} | |
| generateBeneficiaryIDs-api-url=${GEN_BENEFICIARY_IDS_API_URL} |
🤖 Prompt for AI Agents
In src/main/environment/common_docker.properties around lines 196 to 198, the
new Docker property placeholders use {VAR} instead of Spring's required ${VAR},
causing literal unresolved values; replace jibri.output.path,
video.recording.path, and generateBeneficiaryIDs-api-url values to use
${VIDEO_RECORDING_PATH}, ${GEN_BENEFICIARY_IDS_API_URL} (and the correct env var
for jibri.output.path) respectively so Spring resolves them at runtime.
* fix: add logs * fix: add url * fix: add logs * fix: revert the hardcoded url * removing static field type for env variables * fix: merge with 3.6.0 * fix: merge with 3.6.0 --------- Co-authored-by: 5Amogh <amoghavarsh@navadhiti.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/com/iemr/common/service/beneficiary/IdentityBeneficiaryServiceImpl.java (2)
29-29: Avoid wildcard imports for Gson.Import only the used classes to keep the surface explicit and reduce accidental symbol shadowing.
Apply:
-import com.google.gson.*; +import com.google.gson.JsonArray; +import com.google.gson.JsonElement; +import com.google.gson.JsonObject; +import com.google.gson.JsonParser; +import com.google.gson.JsonSyntaxException;
61-67: Rename config fields to camelCase; UPPER_CASE implies constants.These are mutable instance fields; prefer benGenBaseUrl and benGenApiPath for readability and convention.
Example:
-@Value("${genben-api}") -private String BEN_GEN ; -@Value("${generateBeneficiaryIDs-api-url}") -private String BEN_GEN_API_URL ; +@Value("${genben-api}") +private String benGenBaseUrl; +@Value("${generateBeneficiaryIDs-api-url}") +private String benGenApiPath;Update call sites accordingly (Lines 534–536).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/iemr/common/service/beneficiary/IdentityBeneficiaryServiceImpl.java(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/iemr/common/service/beneficiary/IdentityBeneficiaryServiceImpl.java (1)
src/main/java/com/iemr/common/utils/response/OutputResponse.java (1)
OutputResponse(38-244)
🔇 Additional comments (3)
src/main/java/com/iemr/common/service/beneficiary/IdentityBeneficiaryServiceImpl.java (3)
412-425: Defensive checks and JSON parsing guard are solid.Null/empty response handling and JsonSyntaxException guard improve resilience.
407-409: Don’t log full request payloads at INFO; redact PII.Payloads likely contain beneficiary PII. Keep URL at info, move/redact payload to debug.
Apply:
-logger.info("Request Payload: {}", request); +logger.debug("Request Payload: [REDACTED]");
54-58: Property coverage verified across environments All required keys (identity-api-url, identity-1097-api-url, genben-api, generateBeneficiaryIDs-api-url) are present in application.properties, common_ci.properties, common_docker.properties, and common_example.properties.
| logger.info("Request to generate ben IDs: " + request); | ||
| logger.info("Generating ben IDs API URL: " + BEN_GEN + BEN_GEN_API_URL); | ||
| result = httpUtils.post(BEN_GEN + BEN_GEN_API_URL, request, header); | ||
|
|
||
| logger.info("Response from generate ben IDs: " + result); | ||
| OutputResponse identityResponse = inputMapper.gson().fromJson(result, OutputResponse.class); |
There was a problem hiding this comment.
Redact payload/response logs; build URL safely; use parameterized logging.
- PII risk: request/response logged at INFO. Redact and move to DEBUG.
- URL join: BEN_GEN + BEN_GEN_API_URL can double-miss/multi-slash; normalize parts.
- Prefer {} placeholders over string concatenation.
Apply:
-logger.info("Request to generate ben IDs: " + request);
-logger.info("Generating ben IDs API URL: " + BEN_GEN + BEN_GEN_API_URL);
-result = httpUtils.post(BEN_GEN + BEN_GEN_API_URL, request, header);
-logger.info("Response from generate ben IDs: " + result);
+String base = BEN_GEN != null ? BEN_GEN.replaceAll("/+$", "") : "";
+String path = BEN_GEN_API_URL != null ? BEN_GEN_API_URL.replaceAll("^/+", "") : "";
+String apiUrl = base.isEmpty() ? path : (path.isEmpty() ? base : base + "/" + path);
+logger.info("Generating ben IDs API URL: {}", apiUrl);
+logger.debug("Request to generate ben IDs: [REDACTED]");
+result = httpUtils.post(apiUrl, request, header);
+if (result == null || result.isEmpty()) {
+ logger.error("Empty response from generate ben IDs API");
+ throw new IEMRException("No response received from generate ben IDs API");
+}
+logger.debug("Response from generate ben IDs: [REDACTED]");Follow-up: consider wrapping the OutputResponse parsing here in a try/catch for JsonSyntaxException similar to getIdentityResponse for parity.
📝 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.
| logger.info("Request to generate ben IDs: " + request); | |
| logger.info("Generating ben IDs API URL: " + BEN_GEN + BEN_GEN_API_URL); | |
| result = httpUtils.post(BEN_GEN + BEN_GEN_API_URL, request, header); | |
| logger.info("Response from generate ben IDs: " + result); | |
| OutputResponse identityResponse = inputMapper.gson().fromJson(result, OutputResponse.class); | |
| // normalize base URL and path to avoid duplicate or missing slashes | |
| String base = BEN_GEN != null ? BEN_GEN.replaceAll("/+$", "") : ""; | |
| String path = BEN_GEN_API_URL != null ? BEN_GEN_API_URL.replaceAll("^/+", "") : ""; | |
| String apiUrl = base.isEmpty() | |
| ? path | |
| : (path.isEmpty() ? base : base + "/" + path); | |
| // log the final URL at INFO; redact PII in payload/response at DEBUG | |
| logger.info("Generating ben IDs API URL: {}", apiUrl); | |
| logger.debug("Request to generate ben IDs: [REDACTED]"); | |
| result = httpUtils.post(apiUrl, request, header); | |
| if (result == null || result.isEmpty()) { | |
| logger.error("Empty response from generate ben IDs API"); | |
| throw new IEMRException("No response received from generate ben IDs API"); | |
| } | |
| logger.debug("Response from generate ben IDs: [REDACTED]"); | |
| OutputResponse identityResponse = | |
| inputMapper.gson().fromJson(result, OutputResponse.class); |
🤖 Prompt for AI Agents
In
src/main/java/com/iemr/common/service/beneficiary/IdentityBeneficiaryServiceImpl.java
around lines 533 to 537, change the logging and URL construction: move
request/response payload logs from INFO to DEBUG and avoid printing raw PII (log
a redacted summary or length instead), replace string concatenation with
parameterized logger calls (logger.debug("Request to generate ben IDs: {}",
redactedRequest) / logger.debug("Response from generate ben IDs: {}",
redactedResponse)), build the endpoint URL using a safe join/normalize method
(trim trailing/leading slashes or use URI/URL builder) instead of BEN_GEN +
BEN_GEN_API_URL to avoid double slashes, and wrap the
inputMapper.gson().fromJson(...) call in a try/catch that catches
JsonSyntaxException and handles/logs it similarly to getIdentityResponse so
parsing failures are handled gracefully.
Moving Signture related changes from 3.5.0 to 3.6.0
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (1)
1171-1189: New endpoint exposes JWT without authentication or validation.The
/get-jwt-tokenendpoint retrieves and returns the JWT cookie value without:
- Authenticating the request
- Validating the token before returning it
- Rate limiting or abuse prevention
This allows anyone to extract JWT tokens from their own cookies, which may facilitate token exfiltration attacks.
Consider one of these approaches:
Option 1: Remove if not needed
- @GetMapping("/get-jwt-token") - public ResponseEntity<String> getJwtTokenFromCookie(HttpServletRequest httpRequest) { - // ... implementation ... - }Option 2: Add authentication and validation
- @GetMapping("/get-jwt-token") + @GetMapping(value = "/get-jwt-token", headers = "Authorization") public ResponseEntity<String> getJwtTokenFromCookie(HttpServletRequest httpRequest) { Cookie[] cookies = httpRequest.getCookies(); if (cookies != null) { for (Cookie cookie : cookies) { if ("Jwttoken".equalsIgnoreCase(cookie.getName())) { String jwtToken = cookie.getValue(); + // Validate before returning + Claims claims = jwtUtil.validateToken(jwtToken); + if (claims == null) { + return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body("Invalid token"); + } return ResponseEntity.ok(jwtToken); } } } return ResponseEntity.status(HttpStatus.NOT_FOUND).body("JWT token not found"); }
♻️ Duplicate comments (1)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (1)
566-587: Token extraction proceeds without validation (previously flagged).The JWT token is extracted from headers and cookies but
getUserIdFromTokenis called before validating the token, creating a security risk.This issue was previously identified. Apply the recommended fix:
if (jwtToken == null) { logger.warn("Authentication failed: no token found in header or cookies."); throw new IEMRException("Authentication failed. Please log in again."); } - // Extract user ID from the JWT token - String userId = jwtUtil.getUserIdFromToken(jwtToken); + // Validate token first, then extract user ID + Claims claims = jwtUtil.validateToken(jwtToken); + if (claims == null) { + logger.warn("Authentication failed: invalid token provided."); + throw new IEMRException("Authentication failed. Please log in again."); + } + String userId = claims.get("userId", String.class);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java(14 hunks)src/main/java/com/iemr/common/service/users/IEMRAdminUserService.java(1 hunks)src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-02-21T07:43:03.828Z
Learnt from: sandipkarmakar3
PR: PSMRI/Common-API#162
File: src/main/java/com/iemr/common/utils/CookieUtil.java:52-66
Timestamp: 2025-02-21T07:43:03.828Z
Learning: In the Common-API project's CookieUtil class, the current implementation of addJwtTokenToCookie using both response.addCookie() and manual Set-Cookie header has been tested and confirmed to work as required for their use case. URL encoding of the JWT token is not necessary.
Applied to files:
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
🔇 Additional comments (1)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (1)
287-292: Verify intent: allow “New” users to refresh tokens
The refreshToken endpoint now accepts users whose M_status is “New” as well as “Active”; confirm this aligns with your authentication policy or restrict it to only “Active” users.
| @Operation(summary = "Get UserId based on userName") | ||
| @GetMapping(value = "/userName/{userName}", produces = MediaType.APPLICATION_JSON, headers = "Authorization") | ||
| public ResponseEntity<?> getUserDetails(@PathVariable("userName") String userName) { | ||
| try { | ||
| List<User> users = iemrAdminUserServiceImpl.getUserIdbyUserName(userName); | ||
| if (users.isEmpty()) { | ||
| return new ResponseEntity<>(Map.of("error", "UserName Not Found"), HttpStatus.NOT_FOUND); | ||
| } | ||
| User user = users.get(0); | ||
| return new ResponseEntity<>(Map.of("userName", user.getUserName(), "userId", user.getUserID()), HttpStatus.OK); | ||
| } catch (Exception e) { | ||
| return new ResponseEntity<>(Map.of("error", "Internal server error"), HttpStatus.INTERNAL_SERVER_ERROR); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Input validation missing for username parameter.
The new /userName/{userName} endpoint accepts the userName path variable without validation for:
- Null/empty values (Spring may allow empty strings)
- SQL injection patterns
- Length constraints
- Special characters
Additionally, the error response leaks implementation details ("Internal server error" suggests unhandled exceptions).
Apply this diff:
@Operation(summary = "Get UserId based on userName")
@GetMapping(value = "/userName/{userName}", produces = MediaType.APPLICATION_JSON, headers = "Authorization")
public ResponseEntity<?> getUserDetails(@PathVariable("userName") String userName) {
try {
+ // Validate input
+ if (userName == null || userName.trim().isEmpty()) {
+ return new ResponseEntity<>(Map.of("error", "Username is required"), HttpStatus.BAD_REQUEST);
+ }
+ if (userName.length() > 255) {
+ return new ResponseEntity<>(Map.of("error", "Username too long"), HttpStatus.BAD_REQUEST);
+ }
+
List<User> users = iemrAdminUserServiceImpl.getUserIdbyUserName(userName);
if (users.isEmpty()) {
return new ResponseEntity<>(Map.of("error", "UserName Not Found"), HttpStatus.NOT_FOUND);
}
User user = users.get(0);
return new ResponseEntity<>(Map.of("userName", user.getUserName(), "userId", user.getUserID()), HttpStatus.OK);
} catch (Exception e) {
+ logger.error("Error fetching user details for userName: {}", userName, e);
return new ResponseEntity<>(Map.of("error", "Internal server error"), HttpStatus.INTERNAL_SERVER_ERROR);
}
}📝 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.
| @Operation(summary = "Get UserId based on userName") | |
| @GetMapping(value = "/userName/{userName}", produces = MediaType.APPLICATION_JSON, headers = "Authorization") | |
| public ResponseEntity<?> getUserDetails(@PathVariable("userName") String userName) { | |
| try { | |
| List<User> users = iemrAdminUserServiceImpl.getUserIdbyUserName(userName); | |
| if (users.isEmpty()) { | |
| return new ResponseEntity<>(Map.of("error", "UserName Not Found"), HttpStatus.NOT_FOUND); | |
| } | |
| User user = users.get(0); | |
| return new ResponseEntity<>(Map.of("userName", user.getUserName(), "userId", user.getUserID()), HttpStatus.OK); | |
| } catch (Exception e) { | |
| return new ResponseEntity<>(Map.of("error", "Internal server error"), HttpStatus.INTERNAL_SERVER_ERROR); | |
| } | |
| } | |
| @Operation(summary = "Get UserId based on userName") | |
| @GetMapping(value = "/userName/{userName}", produces = MediaType.APPLICATION_JSON, headers = "Authorization") | |
| public ResponseEntity<?> getUserDetails(@PathVariable("userName") String userName) { | |
| try { | |
| // Validate input | |
| if (userName == null || userName.trim().isEmpty()) { | |
| return new ResponseEntity<>(Map.of("error", "Username is required"), HttpStatus.BAD_REQUEST); | |
| } | |
| if (userName.length() > 255) { | |
| return new ResponseEntity<>(Map.of("error", "Username too long"), HttpStatus.BAD_REQUEST); | |
| } | |
| List<User> users = iemrAdminUserServiceImpl.getUserIdbyUserName(userName); | |
| if (users.isEmpty()) { | |
| return new ResponseEntity<>(Map.of("error", "UserName Not Found"), HttpStatus.NOT_FOUND); | |
| } | |
| User user = users.get(0); | |
| return new ResponseEntity<>(Map.of("userName", user.getUserName(), "userId", user.getUserID()), HttpStatus.OK); | |
| } catch (Exception e) { | |
| logger.error("Error fetching user details for userName: {}", userName, e); | |
| return new ResponseEntity<>(Map.of("error", "Internal server error"), HttpStatus.INTERNAL_SERVER_ERROR); | |
| } | |
| } |
| List<User> getUserIdbyUserName(String userName) throws IEMRException; | ||
|
|
There was a problem hiding this comment.
Method naming violates Java conventions; return type suggests username isn't unique.
The method name getUserIdbyUserName should follow camelCase: getUserByUserName or getUserIdByUserName. Additionally, returning List<User> implies multiple users can share a username, which conflicts with typical authentication constraints where usernames are unique identifiers.
Apply this diff:
- List<User> getUserIdbyUserName(String userName) throws IEMRException;
+ User getUserByUserName(String userName) throws IEMRException;Note: If username uniqueness is guaranteed, return a single User (or Optional<User>). If not, verify whether userExitsCheck (line 51) already provides this functionality.
📝 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.
| List<User> getUserIdbyUserName(String userName) throws IEMRException; | |
| User getUserByUserName(String userName) throws IEMRException; |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/service/users/IEMRAdminUserService.java around
lines 124-125, the method name and return type are problematic: rename
getUserIdbyUserName to follow camelCase and meaning (e.g., getUserByUserName or
getUserIdByUserName) and change the return type to a single User or
Optional<User> if usernames are unique; if usernames truly can be non-unique,
keep List<User> but document that fact. Update the interface signature
accordingly, then update all implementing classes and call sites to the new
method name and return type; also verify whether the existing userExitsCheck
(around line 51) already covers this behavior and reconcile duplication.
| @Override | ||
| public List<User> getUserIdbyUserName(String userName) { | ||
|
|
||
| return iEMRUserRepositoryCustom.findByUserName(userName); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Duplicate functionality: identical to userExitsCheck at line 439.
This method duplicates userExitsCheck(String userName) which already delegates to iEMRUserRepositoryCustom.findByUserName(userName). Additionally, no validation is performed on the userName parameter before the repository call.
Consider one of these approaches:
Option 1: Remove duplicate and use existing method
- @Override
- public List<User> getUserIdbyUserName(String userName) {
-
- return iEMRUserRepositoryCustom.findByUserName(userName);
- }
+ // Removed: use userExitsCheck() insteadOption 2: Add validation if keeping this method
@Override
- public List<User> getUserIdbyUserName(String userName) {
-
+ public List<User> getUserByUserName(String userName) throws IEMRException {
+ if (userName == null || userName.trim().isEmpty()) {
+ throw new IEMRException("Username cannot be null or empty");
+ }
return iEMRUserRepositoryCustom.findByUserName(userName);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java
around lines 1222-1226, getUserIdbyUserName duplicates userExitsCheck(String
userName) at ~line 439 and lacks validation; either remove this method and
update all callers to use userExitsCheck, or if you must keep it, add input
validation (null/empty/trim) and throw or return an empty list for invalid
input, and ensure both methods delegate to a single private helper that calls
iEMRUserRepositoryCustom.findByUserName(userName) to avoid duplication.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/environment/common_ci.properties(4 hunks)src/main/resources/application.properties(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-02-03T12:41:59.244Z
Learnt from: indraniBan
PR: PSMRI/Common-API#138
File: src/main/environment/common_uat.properties:0-0
Timestamp: 2025-02-03T12:41:59.244Z
Learning: In the Common-API project, JWT secret configuration should use environment variables (e.g., `jwt.secret=env.JWT_SECRET_KEY@`) across all environments for better security.
Applied to files:
src/main/resources/application.properties
📚 Learning: 2025-02-03T12:41:45.354Z
Learnt from: indraniBan
PR: PSMRI/Common-API#138
File: src/main/environment/common_test.properties:0-0
Timestamp: 2025-02-03T12:41:45.354Z
Learning: In Common-API, JWT secrets should be configured using placeholders (e.g., `<Enter_Your_Secret_Key>`) in properties files, similar to other sensitive credentials, to prevent accidental commit of actual secrets.
Applied to files:
src/main/resources/application.properties
📚 Learning: 2025-01-30T09:11:34.734Z
Learnt from: indraniBan
PR: PSMRI/Common-API#138
File: src/main/environment/common_example.properties:0-0
Timestamp: 2025-01-30T09:11:34.734Z
Learning: In example/template properties files, sensitive values like JWT secrets should use placeholders (e.g. `<Enter_Your_Secret_Key>`) instead of actual values to avoid exposing secrets in version control.
Applied to files:
src/main/resources/application.properties
| km-base-protocol=http | ||
| km-username=okmAdmin | ||
| km-password=admin | ||
| km-base-url=http://localhost:8084/OpenKM | ||
| km-base-path=localhost:8084/OpenKM | ||
| km-root-path=/okm:personal/users/ | ||
| km-guest-user=guest | ||
| km-guest-password=guest | ||
|
|
||
| # CTI Config | ||
| cti-server-ip=10.208.122.99 | ||
| cti-logger_base_url=http://10.208.122.99/logger | ||
| sms-gateway-url = | ||
|
|
||
| # Identity Config | ||
| identity-api-url = http://localhost:8094/ | ||
| #Verify whether 1097 and identity are same? | ||
| identity-1097-api-url = http://localhost:8095/ | ||
| ##Generate Benificiary Config | ||
| genben-api=http://localhost:8092/ | ||
|
|
||
| #### SMS Configuration | ||
| send-sms=false | ||
| sendSMSUrl = http://localhost:8080/sms/sendSMS | ||
| source-address=AIDSHL | ||
| sms-username=<Enter SMS username> | ||
| sms-password=<Enter SMS password> | ||
| send-message-url=<Enter SMS sending URL here> | ||
|
|
||
| ###SMS Scheduler configurations | ||
| start-sms-scheduler=false | ||
| cron-scheduler-sms=0 0/1 * * * ? * | ||
|
|
||
| #### Email Configuration | ||
| send-email=false | ||
| spring.mail.host=smtp.gmail.com | ||
| spring.mail.port=587 | ||
| spring.mail.username=<Enter mail username> | ||
| spring.mail.password=<Enter mail password> | ||
| spring.mail.properties.mail.smtp.auth=true | ||
| spring.mail.properties.mail.smtp.starttls.enable=true | ||
|
|
There was a problem hiding this comment.
Hard-coded KM/SMS credentials violate secrets policy.
This block commits real or fill-in password fields (km-password, km-guest-password, sms-password, spring.mail.password, etc.) into VCS. We recently aligned env files to load these via environment placeholders; adding them back here regresses that effort and exposes secrets (even defaults like admin/guest). Please switch these entries to ${…} env-backed placeholders (matching the pattern used in common_ci.properties and other env files) and ensure no plaintext credentials live in application.properties. Remove any actual values that may already be in use and rotate them.
🤖 Prompt for AI Agents
In src/main/resources/application.properties around lines 182-223, several
credential and secret properties are hard-coded (km-password, km-guest-password,
sms-password, spring.mail.password, km-username,
sendSMSUrl/source-address/sms-username/send-message-url, identity URLs if they
contain secrets, etc.); replace each hard-coded value with environment-backed
placeholders following the existing pattern in common_ci.properties (for example
use ${KM_PASSWORD}, ${KM_GUEST_PASSWORD}, ${SMS_PASSWORD},
${SPRING_MAIL_PASSWORD}, ${KM_USERNAME}, ${SMS_USERNAME}, ${SEND_SMS_URL},
${SEND_MESSAGE_URL}, etc.), remove any plaintext defaults like "admin"/"guest"
and empty literal placeholders, and ensure no secret appears in the file; after
changing, instruct ops to set/rotate the corresponding environment variables in
CI/production and update documentation.
| # DB Connections | ||
| spring.datasource.url=jdbc:mysql://localhost:3306/db_iemr | ||
| spring.datasource.username=<Enter userName> | ||
| spring.datasource.password=<Enter password> | ||
|
|
||
|
|
||
| secondary.datasource.username=<Enter username> | ||
| secondary.datasource.password=<Enter password> | ||
| secondary.datasource.url=jdbc:mysql://localhost:3306/db_reporting |
There was a problem hiding this comment.
Database credentials must be externalized.
These datasource username/password placeholders encourage committing real DB secrets into this file. We already standardised on ${…} environment indirection for credentials (see retrieved learnings on JWT/secret handling); please replace these literals with env-driven placeholders and ensure any actual passwords are rotated.
Based on learnings
🤖 Prompt for AI Agents
In src/main/resources/application.properties around lines 368 to 376, the file
currently contains literal DB credential placeholders; change the datasource
username/password and secondary datasource username/password and URLs to use
environment-property placeholders (e.g. ${ENV_VAR_NAME}) instead of hardcoded
values, mapping each property to a distinct env var (e.g.
spring.datasource.username=${DB_IEMR_USERNAME},
spring.datasource.password=${DB_IEMR_PASSWORD},
secondary.datasource.username=${DB_REPORTING_USERNAME},
secondary.datasource.password=${DB_REPORTING_PASSWORD}, and optionally
externalize URLs like spring.datasource.url=${DB_IEMR_URL}), and ensure any real
credentials are rotated and removed from version control and documented for
deployment pipelines to supply via environment variables or secure secret store.
| secondary.datasource.username=<Enter username> | ||
| secondary.datasource.password=<Enter password> | ||
| secondary.datasource.url=jdbc:mysql://localhost:3306/db_reporting | ||
| secondary.datasource.driver-class-name=com.mysql.jdbc.Driver | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Update to the current MySQL driver class.
com.mysql.jdbc.Driver has been deprecated since Connector/J 8; continuing to use it keeps us on legacy behaviour and surfaces warnings. Switch both datasource entries to com.mysql.cj.jdbc.Driver to stay compatible with supported drivers.
🤖 Prompt for AI Agents
In src/main/resources/application.properties around lines 374 to 378, the MySQL
driver class is set to the deprecated com.mysql.jdbc.Driver; update this (for
both primary and secondary datasource entries in this file) to the current
driver class com.mysql.cj.jdbc.Driver to avoid legacy behavior and
warnings—replace the driver-class-name values accordingly and verify no other
entries reference the old class.
fix: aam-1752 , fetch only active signature
| @Query("SELECT es.userSignatureID FROM EmployeeSignature es WHERE es.userID=:userid AND es.deleted=0") | ||
| Long findUserSignature(@Param("userid") Long userid); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Ensure consistency across all repository methods.
Adding the deleted=0 filter to findUserSignature is correct for soft-delete semantics, but the other query methods in this repository (findOneByUserID at line 34 and countByUserIDAndSignatureNotNull at line 39) lack this filter, which may return deleted records inconsistently.
Consider applying the same soft-delete filter to all methods for consistency:
@Repository
public interface EmployeeSignatureRepo extends CrudRepository<EmployeeSignature, Long> {
- EmployeeSignature findOneByUserID(Long userID);
+ @Query("SELECT es FROM EmployeeSignature es WHERE es.userID=:userID AND es.deleted=0")
+ EmployeeSignature findOneByUserID(@Param("userID") Long userID);
@Query("SELECT es.userSignatureID FROM EmployeeSignature es WHERE es.userID=:userid AND es.deleted=0")
Long findUserSignature(@Param("userid") Long userid);
- Long countByUserIDAndSignatureNotNull(Long userID);
+ @Query("SELECT COUNT(es) FROM EmployeeSignature es WHERE es.userID=:userID AND es.signature IS NOT NULL AND es.deleted=0")
+ Long countByUserIDAndSignatureNotNull(@Param("userID") Long userID);Verify that this behavior change doesn't break existing callers:
🏁 Script executed:
#!/bin/bash
# Description: Find all usages of EmployeeSignatureRepo methods to verify impact of the deleted=0 filter.
# Search for calls to findUserSignature
echo "=== Usages of findUserSignature ==="
rg -nP --type=java -C3 '\.findUserSignature\s*\('
# Search for calls to findOneByUserID
echo -e "\n=== Usages of findOneByUserID ==="
rg -nP --type=java -C3 '\.findOneByUserID\s*\('
# Search for calls to countByUserIDAndSignatureNotNull
echo -e "\n=== Usages of countByUserIDAndSignatureNotNull ==="
rg -nP --type=java -C3 '\.countByUserIDAndSignatureNotNull\s*\('Length of output: 1639
Ensure consistent soft-delete filtering across repository methods.
findOneByUserID and countByUserIDAndSignatureNotNull currently return deleted entries; add AND es.deleted=0 (or rename to include AndDeletedFalse) to their queries to match findUserSignature.
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/repository/users/EmployeeSignatureRepo.java
around lines 36-37, the repository methods findOneByUserID and
countByUserIDAndSignatureNotNull are currently returning soft-deleted rows while
findUserSignature filters with es.deleted=0; update those methods so they
consistently exclude deleted entries by adding AND es.deleted=0 to their JPQL
queries or, if using Spring Data derived queries, rename them to include
AndDeletedFalse (e.g., findOneByUserIDAndDeletedFalse /
countByUserIDAndSignatureNotNullAndDeletedFalse) so all three methods apply the
same soft-delete filter.
Revert "fix: aam-1752 , fetch only active signature"
Download active signature.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/main/java/com/iemr/common/utils/exception/GlobalExceptionHandler.java (2)
22-29: Return structured field errors instead of a concatenated string
A flat message forces clients to parse text; send a field→message map so consumers can act on each violation.- String errorMessage = ex.getBindingResult().getFieldErrors().stream() - .map(error -> error.getField() + ": " + error.getDefaultMessage()) - .collect(Collectors.joining(", ")); + var fieldErrors = ex.getBindingResult().getFieldErrors().stream() + .collect(Collectors.toMap( + err -> err.getField(), + err -> err.getDefaultMessage(), + (existing, replacement) -> existing + "; " + replacement + )); return ResponseEntity.status(HttpStatus.BAD_REQUEST) - .body(ApiResponse.error("Validation Error: " + errorMessage, HttpStatus.BAD_REQUEST.value(), null)); + .body(ApiResponse.error("Validation Error", HttpStatus.BAD_REQUEST.value(), fieldErrors));
16-20: Log unhandled exceptions via SLF4J
handleExceptionswallows the stack trace—without logging we lose production diagnostics and incident forensics. Restore structured logging (as previously requested).+import lombok.extern.slf4j.Slf4j; @@ -@RestControllerAdvice +@RestControllerAdvice +@Slf4j public class GlobalExceptionHandler { @@ @ExceptionHandler(Exception.class) public ResponseEntity<ApiResponse<Object>> handleException(Exception ex) { + log.error("Unhandled exception in request pipeline", ex); return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) .body(ApiResponse.error("Something went wrong", HttpStatus.INTERNAL_SERVER_ERROR.value(), null)); }src/main/java/com/iemr/common/controller/users/EmployeeSignatureController.java (1)
64-102: Still need ResponseStatusException with causeWe’re still rethrowing a brand-new
Exception, dropping the original cause and leaking a vague message (with the trailing “..”). Please switch toResponseStatusException(or similar) and propagate the caught exception, and remove the now-unnecessarythrows Exceptionfrom the signature. This mirrors the earlier feedback on this exact spot.- public ResponseEntity<byte[]> fetchFile(@PathVariable("userID") Long userID) throws Exception { + public ResponseEntity<byte[]> fetchFile(@PathVariable("userID") Long userID) { ... - } catch (Exception e) { - logger.error("File download for userID failed with exception " + e.getMessage(), e); - throw new Exception("Error while downloading file. Please contact administrator.."); - } + } catch (Exception e) { + logger.error("File download for userID failed with exception " + e.getMessage(), e); + throw new ResponseStatusException( + HttpStatus.INTERNAL_SERVER_ERROR, + "Error while downloading file. Please contact administrator.", + e); + }src/main/java/com/iemr/common/repository/users/EmployeeSignatureRepo.java (1)
41-41: The soft-delete inconsistency flagged in previous review remains.While this new method correctly supports soft-delete filtering, the existing methods (
findOneByUserIDat line 34,findUserSignatureat line 36, andcountByUserIDAndSignatureNotNullat line 39) still lack consistentdeletedfiltering, as noted in the previous review.
🧹 Nitpick comments (1)
src/main/java/com/iemr/common/service/users/EmployeeSignatureService.java (1)
30-30: LGTM, but consider clarifying the API distinction.The new method addition is correct. However, the interface now exposes both
fetchSignature(line 28) andfetchActiveSignature, which could confuse callers about soft-delete semantics. Consider either:
- Updating
fetchSignatureto also filter deleted records, or- Deprecating
fetchSignatureand migrating callers tofetchActiveSignature
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/iemr/common/controller/users/EmployeeSignatureController.java(2 hunks)src/main/java/com/iemr/common/repository/users/EmployeeSignatureRepo.java(1 hunks)src/main/java/com/iemr/common/service/users/EmployeeSignatureService.java(1 hunks)src/main/java/com/iemr/common/service/users/EmployeeSignatureServiceImpl.java(1 hunks)src/main/java/com/iemr/common/utils/exception/GlobalExceptionHandler.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_check_coverage
| @Override | ||
| public EmployeeSignature fetchActiveSignature(Long userSignID) { | ||
| // New method - fetches only non-deleted records | ||
| return employeeSignatureRepo.findOneByUserIDAndDeleted(userSignID, false); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Implementation is correct, but API inconsistency may confuse callers.
The new method correctly fetches only non-deleted records. However, fetchSignature (line 39-42) still returns both deleted and non-deleted records via findOneByUserID. This creates an inconsistent API surface where similar methods have different soft-delete semantics, potentially leading to incorrect usage.
Consider updating fetchSignature to also filter deleted records or deprecating it in favor of fetchActiveSignature.
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/service/users/EmployeeSignatureServiceImpl.java
around lines 39 to 48, the API is inconsistent: fetchSignature(Long) currently
returns records regardless of soft-delete while fetchActiveSignature(Long)
filters deleted=false. Update fetchSignature to match soft-delete semantics by
calling the repository method that filters deleted (e.g.,
findOneByUserIDAndDeleted(userSignID, false)), or mark fetchSignature as
@Deprecated and have it delegate to fetchActiveSignature to maintain backward
compatibility; update Javadoc and any callers accordingly.
📋 Description
JIRA ID: FLW
1.Set up Firebase in the project.
2.Implemented OTP Consent functionality.
3.Worked on related code integration and configuration.
4. dynamic form api.
✅ 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
Configuration
Improvements
Chores