Conversation
Signed-off-by: Madhuravas reddy <[email protected]>
WalkthroughAdds widespread audit logging: new AuditEvent constants, AuditManagerService wired into DI and service constructors, and audit calls inserted across API, service, batch, and UI layers; no public API removals, only two constructors extended to accept AuditManagerService. Changes
Sequence Diagram(s)(Skipped — changes are primarily audit logging insertions and wiring without a new multi-component sequential feature that requires flow visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In
`@android/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.java`:
- Around line 243-244: The audit call for SYNC_DEVICE_DETAILS in
MasterDataSyncApi.syncMasterData() is misleading because this method does not
sync device details; remove the
auditManagerService.audit(AuditEvent.SYNC_DEVICE_DETAILS,
Components.REGISTRATION) invocation or wrap it in a conditional that only runs
when device details are actually synced (e.g., after a device-sync code path or
when a DeviceDetails result/flag is present). Update only the audit call
(located next to auditManagerService.audit(AuditEvent.SYNC_MASTER_DATA,
Components.REGISTRATION)) so the logged events accurately reflect the work done
and remain consistent with the masterSyncJob path.
In
`@android/app/src/main/java/io/mosip/registration_client/api_services/PacketAuthenticationApi.java`:
- Line 119: The audit events PACKET_SYNCED_TO_SERVER and PACKET_UPLOADED are
being recorded with Components.REGISTRATION in PacketAuthenticationApi (via
auditManagerService.audit(...)), but BatchJob.java emits the same events with
Components.REG_PACKET_LIST; make them consistent by updating the
auditManagerService.audit calls in PacketAuthenticationApi for
AuditEvent.PACKET_SYNCED_TO_SERVER and AuditEvent.PACKET_UPLOADED to use
Components.REG_PACKET_LIST (so both PacketAuthenticationApi and BatchJob.java
attribute the same component).
- Line 219: Update the misspelled audit enum and its usages: rename the
AuditEvent constant PACKET_RETRIVE to PACKET_RETRIEVE in the AuditEvent enum
(and correct its enum description text from "retrived" to "retrieved"), then
update all references such as the call
auditManagerService.audit(AuditEvent.PACKET_RETRIVE, Components.REGISTRATION) in
PacketAuthenticationApi to use AuditEvent.PACKET_RETRIEVE so compilation and
logs reflect the correct spelling.
In
`@android/app/src/main/java/io/mosip/registration_client/api_services/UserDetailsApi.java`:
- Line 72: The audit event AuditEvent.VALIDATE_USER_CRED is emitted in
validateUser immediately after calling isValidUserId even though no credential
validation occurs; update the audit to accurately reflect the action by either
replacing AuditEvent.VALIDATE_USER_CRED with a more appropriate event like
AuditEvent.VALIDATE_USER_ID (or ADD/CREATE a new enum constant) inside the
validateUser flow, or move the auditManagerService.audit(...) call to the point
where credentials are actually checked—identify and modify the
auditManagerService.audit(...) invocation and the AuditEvent enum to keep event
names consistent with the actual operation.
In
`@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/constant/AuditEvent.java`:
- Around line 175-179: The AuditEvent enum contains duplicate audit codes:
PACKET_CREATION_SUCCESS, PACKET_ENCRYPTED, PACKET_UPLOADED,
PACKET_SYNCED_TO_SERVER (using REG-EVT-066..069) and REG_BIO_LEFT_SLAP_SCAN /
REG_BIO_SCAN share REG-EVT-030; update these enum constants to use unique,
non-colliding codes (e.g., choose the next unused REG-EVT numbers) in
AuditEvent, confirm no other constants (like APPR_* or NEXT_BUTTON_CLICKED) use
those codes, and adjust any downstream consumers/tests/configs that reference
the old values so analytics and correlation remain correct. Ensure you search
the repo for the old codes before changing and keep the enum names unchanged to
avoid API breaks.
In
`@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/PacketServiceImpl.java`:
- Line 428: The audit call
auditManagerService.audit(AuditEvent.SYNC_PKT_COUNT_VALIDATE,
Components.REGISTRATION) is invoked before the packet count validation and is
executed inside the same try that can short-circuit validation; move this audit
invocation to after the actual validation logic (so it only logs when validation
ran) and wrap the audit call in its own small try-catch that logs audit failures
but does not rethrow, ensuring packet count validation in the PacketServiceImpl
method always runs even if audit() fails.
In
`@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/PreCheckValidatorServiceImpl.java`:
- Line 84: The call to auditManagerService.audit(...) in
PreCheckValidatorServiceImpl may throw and is currently allowed to bubble up and
be re-wrapped as a ClientCheckedException which can incorrectly block
registration; modify the code around the audit call
(auditManagerService.audit(AuditEvent.SYNC_INFO_VALIDATE,
Components.JOB_SERVICE)) to execute it inside its own try-catch, catch and log
any exception (or use auditManagerService-specific error handling) but do not
rethrow or translate it into OPT_TO_REG_TIME_SYNC_EXCEED — treat it as
fire-and-forget so the rest of the validation flow continues even if audit
fails.
In
`@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.java`:
- Around line 308-313: In RegistrationServiceImpl adjust the flow so
auditManagerService.audit(AuditEvent.PACKET_ENCRYPTED, ...) and
AuditEvent.PACKET_INTERNAL_ZIP are only invoked after persistPacket returns a
non-null result: move or wrap those audit calls to occur after the persistPacket
null-check (i.e., after verifying the returned packet != null). Also fix the
containerPath null-check by changing the condition from "containerPath != null
|| !containerPath.trim().isEmpty()" to use logical AND ("containerPath != null
&& !containerPath.trim().isEmpty()") so it doesn't NPE when containerPath is
null.
🧹 Nitpick comments (2)
android/app/src/main/java/io/mosip/registration_client/api_services/DemographicsDetailsApi.java (1)
49-49: All four demographic mutation methods use the sameSAVE_DETAIL_TO_DTOevent.Consider whether the audit trail needs to distinguish between
addDemographicField,addSimpleTypeDemographicField,setDateField, andsetConsentField. Currently, all four emit the identical event, so any downstream audit analysis cannot differentiate the operation type. If this is intentional (coarse-grained auditing for demographics), no action is needed.Also applies to: 87-87, 123-123, 142-142
android/app/src/main/java/io/mosip/registration_client/api_services/UserDetailsApi.java (1)
51-52:USER_STATUS_FETCHandFETCH_USR_DETare logged before any input validation.Both events will be recorded even when
usernameis empty/null and the method returns early withREG_USER_EMPTY. This is fine if the intent is to track all validation attempts, but if you only want to audit successful lookups, move these after the input checks.
android/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.java
Outdated
Show resolved
Hide resolved
...oid/app/src/main/java/io/mosip/registration_client/api_services/PacketAuthenticationApi.java
Outdated
Show resolved
Hide resolved
...oid/app/src/main/java/io/mosip/registration_client/api_services/PacketAuthenticationApi.java
Outdated
Show resolved
Hide resolved
android/app/src/main/java/io/mosip/registration_client/api_services/UserDetailsApi.java
Outdated
Show resolved
Hide resolved
...oid/clientmanager/src/main/java/io/mosip/registration/clientmanager/constant/AuditEvent.java
Outdated
Show resolved
Hide resolved
...ientmanager/src/main/java/io/mosip/registration/clientmanager/service/PacketServiceImpl.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/mosip/registration/clientmanager/service/PreCheckValidatorServiceImpl.java
Outdated
Show resolved
Hide resolved
...nager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Madhuravas reddy <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@android/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.java`:
- Line 438: The unguarded call to
auditManagerService.audit(AuditEvent.SYNCJOB_INFO_FETCH, Components.JOB_SERVICE)
inside getActiveSyncJobs can throw and block returning results; wrap that audit
invocation in its own try-catch so any exceptions are swallowed or logged (do
not rethrow) to preserve the method's normal return path. Specifically, locate
the audit call in getActiveSyncJobs, surround auditManagerService.audit(...)
with try { ... } catch (Exception e) { /* log audit failure via logger or
auditManagerService.log/error but do not throw */ }, and ensure the catch only
records the failure without impacting the method's result.
In
`@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/constant/AuditEvent.java`:
- Around line 176-179: Fix the typos in the audit event description strings for
PACKET_CREATION_SUCCESS, PACKET_ENCRYPTED, and PACKET_SYNCED_TO_SERVER in the
AuditEvent enum: change "Packet Succesfully Created", "Packet Encrypted
Sucessfully", and "Packet Synced to Server Sucesfully" to use the correct
spelling "Successfully" (e.g., "Packet Successfully Created", "Packet Encrypted
Successfully", "Packet Synced to Server Successfully") so persisted audit logs
use the corrected text.
In
`@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/PreCheckValidatorServiceImpl.java`:
- Around line 296-297: The audit call
auditManagerService.audit(AuditEvent.SYNC_GEO_VALIDATE, Components.REGISTRATION)
in validateCenterToMachineDistance is unguarded and can throw
non-NumberFormatException errors that will block registration; wrap that single
audit call in its own try-catch that catches Exception (or Throwable) and logs
the failure via the existing logger (e.g., logger.error(..., e)) without
rethrowing, mirroring the handling used for SYNC_INFO_VALIDATE so the audit
remains fire-and-forget.
In
`@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.java`:
- Around line 342-344: persistPacket can return a null/empty containerPath but
the code still calls insertRegistration and emits
auditManagerService.audit(AuditEvent.PACKET_CREATION_SUCCESS,
Components.REGISTRATION); — change the flow in RegistrationServiceImpl so you
check the result of persistPacket (containerPath) immediately: if containerPath
is null/empty, log/audit a failure and return or throw (so insertRegistration is
not called), otherwise proceed to call insertRegistration and then emit
PACKET_CREATION_SUCCESS; reference the persistPacket call site,
insertRegistration(...) invocation, and the auditManagerService.audit(...) call
when making the change.
🧹 Nitpick comments (1)
android/app/src/main/java/io/mosip/registration_client/api_services/PacketAuthenticationApi.java (1)
194-197: InconsistentComponentsvalue in upload error path.
PACKET_INTERNAL_ERRORusesComponents.REGISTRATIONwhile all other audit calls inuploadPacketAlluseComponents.REG_PACKET_LIST. This will make error events harder to correlate with the corresponding upload flow in audit queries.- auditManagerService.audit(AuditEvent.PACKET_INTERNAL_ERROR, Components.REGISTRATION); + auditManagerService.audit(AuditEvent.PACKET_INTERNAL_ERROR, Components.REG_PACKET_LIST);
android/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.java
Outdated
Show resolved
Hide resolved
...oid/clientmanager/src/main/java/io/mosip/registration/clientmanager/constant/AuditEvent.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/mosip/registration/clientmanager/service/PreCheckValidatorServiceImpl.java
Outdated
Show resolved
Hide resolved
...nager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.java
Show resolved
Hide resolved
Signed-off-by: Madhuravas reddy <[email protected]>
| }); | ||
| } catch (Exception e) { | ||
| Log.e(getClass().getSimpleName(), e.getMessage()); | ||
| auditManagerService.audit(AuditEvent.PACKET_INTERNAL_ERROR, Components.REGISTRATION); |
There was a problem hiding this comment.
Why are we not passing the exception or error to the audit method?
| @Override | ||
| public void updatePacketStatus(@NonNull String packetId, @Nullable String serverStatus, @NonNull String clientStatus, @NonNull PacketAuthPigeon.Result<Void> result) { | ||
| registrationRepository.updateStatus(packetId, serverStatus, clientStatus); | ||
| if (PacketClientStatus.APPROVED.name().equals(clientStatus)) { |
There was a problem hiding this comment.
What are the other packetclient status we maintain?
If it’s only 2, do we need if-else if block here just for audit?
| public void supervisorReview(@NonNull String packetId, @NonNull String supervisorStatus, @NonNull String supervisorComment, @NonNull PacketAuthPigeon.Result<Void> result) { | ||
| auditManagerService.audit(AuditEvent.PACKET_UPDATE, Components.REGISTRATION); | ||
| registrationRepository.updateSupervisorReview(packetId, supervisorStatus, supervisorComment); | ||
| if (PacketClientStatus.APPROVED.name().equals(supervisorStatus)) { |
There was a problem hiding this comment.
What are the other packetclient status we maintain?
If it’s only 2, do we need if-else if block here just for audit?
|
|
||
| @Override | ||
| public void validateUser(@NonNull String username, @NonNull String langCode, @NonNull UserPigeon.Result<UserPigeon.User> result) { | ||
| auditManagerService.audit(AuditEvent.USER_STATUS_FETCH, Components.LOGIN); |
There was a problem hiding this comment.
Why are we creating 2 different audits for the same action?
| } catch (Exception e) { | ||
| syncAndUploadInProgressStatus = false; | ||
| Log.e(getClass().getSimpleName(), e.getMessage()); | ||
| auditManagerService.audit(AuditEvent.PACKET_INTERNAL_ERROR, Components.REG_PACKET_LIST); |
There was a problem hiding this comment.
Why is the error object not passed to audit method?
| // Audit after successful validation; don't let audit failures skip validation result. | ||
| try { | ||
| auditManagerService.audit(AuditEvent.SYNC_PKT_COUNT_VALIDATE, Components.REGISTRATION); | ||
| } catch (Exception auditEx) { |
There was a problem hiding this comment.
We should handle all the exception inside the audit method instead of writing try-catch block in the caller method.
| try { | ||
| validatingSyncJobsConfig(); | ||
| try { | ||
| auditManagerService.audit(AuditEvent.SYNC_INFO_VALIDATE, Components.JOB_SERVICE); |
There was a problem hiding this comment.
We should handle all the exception inside the audit method instead of writing try-catch block in the caller method.
| } | ||
|
|
||
| try { | ||
| auditManagerService.audit(AuditEvent.SYNC_GEO_VALIDATE, Components.REGISTRATION); |
There was a problem hiding this comment.
We should handle all the exception inside the audit method instead of writing try-catch block in the caller method.
Summary by CodeRabbit