feat: add audit logging for all user actions#108
Merged
Conversation
- Add AuditLog entity, repository, service, and REST controller - Add Flyway migration V11__create_audit_log_table.sql - Add audit logging to IncidentService (create, view, delete, assign, status changes) - Add audit logging to DocumentService (upload, download, delete) - Add audit logging to UserService (role changes, status changes, approve, delete) - Update AdminController to pass currentAdmin to UserService methods - Add admin audit log viewer page (Vanilla JS frontend) - Update all tests with AuditLogService mock and new method signatures
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 53 minutes and 27 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughA new audit logging subsystem is added (entity, repo, service, controller). Controllers and services are updated to accept authenticated AppUser context and emit audit events for admin actions, document operations, and incident lifecycle changes. Database migration creates the audit_log table. Changes
Sequence Diagram(s)sequenceDiagram
actor Admin as Admin User
participant AC as AdminController
participant US as UserService
participant ALS as AuditLogService
participant DB as Database
Admin->>AC: PATCH /api/admin/users/{id}/role
AC->>AC: Resolve authenticated AppUser
AC->>US: updateUserRole(userId, newRole, admin)
US->>DB: persist role change
DB-->>US: OK
US->>ALS: log("UPDATE_USER_ROLE", details, admin)
ALS->>DB: insert audit_log
DB-->>ALS: OK
US-->>AC: updated user
AC-->>Admin: 200 OK
sequenceDiagram
actor User as Application User
participant DC as DocumentController
participant DS as DocumentService
participant Minio as Minio Storage
participant ALS as AuditLogService
participant DB as Database
User->>DC: POST /api/documents/upload
DC->>DC: Resolve authenticated AppUser
DC->>DS: uploadFile(file, incident, user)
DS->>Minio: putObject(file)
Minio-->>DS: OK
DS->>DB: save Document entity
DB-->>DS: saved
DS->>ALS: log("UPLOAD_DOCUMENT", details, user)
ALS->>DB: insert audit_log
DB-->>ALS: OK
DS-->>DC: return document
DC-->>User: 201 Created
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/test/java/org/example/team6backend/admin/AdminControllerTest.java (1)
62-111:⚠️ Potential issue | 🟡 MinorClear
SecurityContextHolderbetween tests.
setUp()writes into the globalSecurityContextHolderbut nothing ever clears it. SinceSecurityContextHolderuses a thread-local strategy, this state leaks into subsequent tests that run on the same thread (e.g.,IncidentControllerTestalready bothers to clear it in@AfterEach). Add the symmetric cleanup:+ `@org.junit.jupiter.api.AfterEach` + void tearDown() { + SecurityContextHolder.clearContext(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/team6backend/admin/AdminControllerTest.java` around lines 62 - 111, Tests set the Authentication in SecurityContextHolder within setUp() but never clear it, causing state leakage across tests; add a symmetric cleanup method annotated with `@AfterEach` (e.g., tearDown()) that calls SecurityContextHolder.clearContext() to remove the Authentication set by setUp(), referencing the existing setUp() and SecurityContextHolder to locate where to add the cleanup.src/main/java/org/example/team6backend/document/controller/DocumentController.java (1)
38-53:⚠️ Potential issue | 🟡 MinorGuard against a null
userDetails.
userDetails.getUser()will NPE if the endpoint is ever reached without an authenticatedCustomUserDetailsprincipal (anonymous or OAuth2 token without a resolvedCustomUserDetails). The same pattern applies touploadFile(Line 69) anddeleteFile(Line 96). Since these are now on the audit path, failing with a 500 here also means no audit event is emitted.Consider a small guard that returns 401:
AppUser user = userDetails.getUser(); +if (userDetails == null) { + throw new ResponseStatusException(HttpStatus.UNAUTHORIZED); +} +AppUser user = userDetails.getUser();(or rely on method‑security annotations to prevent anonymous entry.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/document/controller/DocumentController.java` around lines 38 - 53, Guard against a null AuthenticationPrincipal in the document controller methods: add an explicit null check for the method parameter userDetails at the start of getFile (and replicate in uploadFile and deleteFile) and throw a ResponseStatusException with HttpStatus.UNAUTHORIZED if userDetails is null rather than calling userDetails.getUser(); reference the CustomUserDetails type and use the existing logging pattern (log.info) to record the unauthorized access before throwing so the request fails with 401 and still allows auditing/handling.src/main/java/org/example/team6backend/document/service/DocumentService.java (1)
67-94:⚠️ Potential issue | 🟡 Minor
DOWNLOAD_DOCUMENTis audited before the stream is actually consumed.
auditLogService.log(...)fires as soon as theInputStreamhandle is obtained from Minio. If the controller fails while streaming bytes to the client (connection reset, serialization error, etc.), the audit row is already committed, giving false positives for completed downloads. For an audit trail this is usually tolerable, but worth calling out — if strict fidelity is needed, log on stream close/after successful transfer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/document/service/DocumentService.java` around lines 67 - 94, The audit is currently recorded immediately in DocumentService.downloadFile right after obtaining the InputStream from minioService.downloadFile, producing false positives if the stream fails during transfer; instead wrap the returned InputStream in a proxy (e.g., a FilterInputStream/Wrapper) that overrides close() to call auditLogService.log("DOWNLOAD_DOCUMENT", ...) only after the stream is successfully closed (and optionally on successful EOF), and return that wrapped stream from downloadFile; keep existing Minio Service exception handling intact and ensure the wrapper delegates all read/close methods to the underlying stream so behavior is unchanged except for logging on close.src/main/java/org/example/team6backend/incident/service/IncidentService.java (1)
134-154:⚠️ Potential issue | 🟠 MajorAuditing every
VIEW_INCIDENTwill dominate the audit table.
getByIdruns on every detail view and is also invoked from document flows (DocumentControllercallsincidentService.getById(...)during download/access checks per the test updates). One audit row per read will:
- explode
audit_logsize quickly (far beyond mutations),- add a synchronous DB write to the hot read path (every view pays an extra insert),
- drown out the security-relevant mutation events in the admin viewer.
Consider: (a) only auditing "sensitive" views (admin reading others' incidents, handlers outside their assignments), (b) rate-limiting per user/incident/day, or (c) moving view events to a separate (or async) sink.
If views must be audited at full resolution, make the insert asynchronous (
@Asyncor event publisher) so reads don't block on audit writes, and plan table partitioning/retention foraudit_log.
🧹 Nitpick comments (7)
src/main/java/org/example/team6backend/auditlog/entity/AuditLog.java (2)
19-24: Tighten non-null fields and use@Column(columnDefinition = "TEXT")fordetails.
actionandcreatedAtare always set byAuditLogService.log(...)and should be@Column(nullable = false)to match aNOT NULLmigration (see the migration comment).detailscan legitimately be long — annotate it so JPA/DDL tooling maps it toTEXTrather thanVARCHAR(255)if the schema is ever regenerated.- private String action; + `@Column`(nullable = false) + private String action; private String targetType; private String targetId; - private String details; - private Instant createdAt; + `@Column`(columnDefinition = "TEXT") + private String details; + `@Column`(nullable = false, updatable = false) + private Instant createdAt;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/auditlog/entity/AuditLog.java` around lines 19 - 24, Add JPA column annotations on the AuditLog entity: mark the fields action and createdAt with `@Column`(nullable = false) to reflect the NOT NULL migration and ensure AuditLogService.log(...) invariants are enforced at the schema level, and annotate details with `@Column`(columnDefinition = "TEXT") so JPA/DDL tooling maps it to a TEXT column instead of a default VARCHAR(255); update the AuditLog class fields action, createdAt, and details accordingly.
26-28: MakeperformedBylazy and tighten the column mapping.
@ManyToOnedefaults toFetchType.EAGER, so every audit-log read (including paginated admin list views) triggers an additional join/select forAppUser. Given that the admin audit viewer is designed to render potentially large lists, preferLAZYand fetch join explicitly where needed. Also mirror the DB-level FK intent at the JPA layer.- `@ManyToOne` - `@JoinColumn`(name = "user_id") + `@ManyToOne`(fetch = FetchType.LAZY) + `@JoinColumn`(name = "user_id", foreignKey = `@ForeignKey`(name = "fk_audit_log_user")) private AppUser performedBy;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/auditlog/entity/AuditLog.java` around lines 26 - 28, The performedBy association on AuditLog is currently eager; change the `@ManyToOne` on the performedBy field in AuditLog to use FetchType.LAZY and tighten the join column by specifying explicit column constraints (e.g. set nullable appropriately and add a ForeignKey name) in the `@JoinColumn` so the JPA mapping mirrors the DB FK intent; update any repository queries or service code that need the user to use fetch joins (or EntityGraph) when loading AuditLog with its AppUser.src/test/java/org/example/team6backend/incident/service/IncidentServiceTest.java (1)
57-58: LGTM.Mock and signature updates correctly track the new
DocumentService.uploadFile(..., user)andIncidentService.deleteIncident(id, user)APIs. Consider adding averify(auditLogService).log(...)in at least one happy-path test (e.g.,deleteIncident) to lock the audit contract — otherwise regressions that silently stop emitting audit events will go unnoticed.Also applies to: 109-114, 208-208
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/team6backend/incident/service/IncidentServiceTest.java` around lines 57 - 58, Add a verification that an audit event is emitted in the happy-path delete test: after calling IncidentService.deleteIncident(id, user) in IncidentServiceTest, add a Mockito verify call to AuditLogService (auditLogService) such as verify(auditLogService).log(...) with an ArgumentMatcher or ArgumentCaptor to assert the expected audit entry (user, action/type, target id). Locate the deleteIncident test method in IncidentServiceTest and add this verify; you can mirror the same pattern in other happy-path tests (e.g., the test exercising DocumentService.uploadFile(..., user)) to lock the audit contract.src/test/java/org/example/team6backend/user/service/UserServiceTest.java (1)
41-43: LGTM.Signature updates are consistent and the explicit
verify(auditLogService).log(anyString(), anyString(), eq(adminUser))inupdateUserRole_ShouldUpdateRoleis a nice guardrail. Consider replicating this verification pattern in at least one of thedeleteUser/approvePendingUser/updateUserActiveStatushappy-path tests so all four audited operations have at least one test that asserts the audit event is emitted.Also applies to: 362-366
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/team6backend/user/service/UserServiceTest.java` around lines 41 - 43, One of the audited operations is missing an assertion that AuditLogService.log was invoked; mirror the pattern used in updateUserRole_ShouldUpdateRole by adding a verify(auditLogService).log(anyString(), anyString(), eq(adminUser)) assertion to at least one happy-path test (e.g., deleteUser_ShouldDeleteUser, approvePendingUser_ShouldApproveUser, or updateUserActiveStatus_ShouldUpdateStatus) so the test verifies the audit event is emitted; locate the corresponding test method (deleteUser/approvePendingUser/updateUserActiveStatus) and add the verify call after exercising the service, making sure necessary Mockito matchers (anyString(), eq(adminUser)) are available.src/test/java/org/example/team6backend/admin/AdminControllerTest.java (1)
113-221:@WithMockUseris redundant alongside.with(authentication(auth)).Each test now supplies an explicit
UsernamePasswordAuthenticationTokenvia.with(authentication(auth)), which takes precedence over the@WithMockUser-provisioned context. WithaddFilters = falseon the class, neither is actually driving authorization — the class-level annotation just confuses the intent. Drop the@WithMockUser(roles = "ADMIN")on these methods (or remove the post-processor) so the test intent is unambiguous.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/team6backend/admin/AdminControllerTest.java` around lines 113 - 221, Remove the redundant `@WithMockUser`(roles = "ADMIN") annotations from each test method that already uses .with(authentication(auth)) (e.g., shouldGetAllUsers, shouldGetUsersWithSearch, shouldGetPendingUsers, shouldGetUserById, shouldApprovePendingUser, shouldUpdateUserRole, shouldUpdateUserStatus, shouldDeleteUser) so the tests rely only on the explicit UsernamePasswordAuthenticationToken provided via authentication(auth); delete the `@WithMockUser` lines and any unused imports related to it to keep intent unambiguous.src/main/java/org/example/team6backend/auditlog/controller/AuditLogController.java (1)
12-37: Optional: route DB access through a service, not the repository.Controllers elsewhere in this codebase delegate to services (
UserService,DocumentService,IncidentService). InjectingAuditLogRepositorydirectly into a controller bypasses that layer — a smallAuditLogService.findLogs(...)/findByUser(...)would keep things consistent and give you a natural place to enforce pagination, search sanitization, and (later) cross-cutting rules like retention filters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/auditlog/controller/AuditLogController.java` around lines 12 - 37, Replace direct repository access in AuditLogController with a service layer: create an AuditLogService (e.g., class AuditLogService with methods List<AuditLog> findLogs(String search, Sort sort) and List<AuditLog> findByUser(Long userId)) that encapsulates the calls to AuditLogRepository.searchLogs(...) and findAll(...) / findByPerformedBy(...), then inject AuditLogService into AuditLogController (replace the AuditLogRepository field) and delegate getLogs(...) to auditLogService.findLogs(search, Sort.by(...)) and getLogsByUser(...) to auditLogService.findByUser(userId); keep repository usage only inside AuditLogService so future pagination, sanitization and retention logic can be added there.src/main/java/org/example/team6backend/user/service/UserService.java (1)
119-166: Minor: no-op role/status changes still write audit rows.Neither
updateUserRolenorupdateUserActiveStatuscompare old vs new before saving and auditing. Posting the same role/status twice produces a misleading "changed X from RESIDENT to RESIDENT" audit entry and an unnecessary DB write. A simple early-return whenoldRole == newRole/oldStatus == activeavoids both.♻️ Suggested tweak
AppUser user = getUserById(userId); UserRole oldRole = user.getRole(); + if (oldRole == newRole) { + return user; + }(And analogously for
updateUserActiveStatus.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/user/service/UserService.java` around lines 119 - 166, Both updateUserRole and updateUserActiveStatus perform unnecessary saves and audit logs when there is no actual change; modify updateUserRole (compare oldRole to newRole) and updateUserActiveStatus (compare oldStatus to active) after retrieving the AppUser via getUserById and, if equal, log a debug message and return the user immediately without calling userRepository.save or auditLogService.log; keep the existing checks for last-admin deactivation in updateUserActiveStatus before the equality check or adjust to only run when attempting to deactivate (active == false) and the status would actually change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/team6backend/admin/AdminController.java`:
- Around line 37-46: The getCurrentUser(CustomUserDetails currentUser) method
currently throws IllegalStateException when no authenticated principal is found;
change it to throw an appropriate Spring security exception instead (e.g., throw
new AuthenticationCredentialsNotFoundException("No authenticated user found")
for unauthenticated cases or new AccessDeniedException("No principal available")
if you prefer 403 semantics) so the framework returns 401/403 instead of 500;
update the branches where Authentication auth =
SecurityContextHolder.getContext().getAuthentication() and the instanceof
CustomUserDetails check fail to throw the chosen security exception and ensure
imports reference
org.springframework.security.core.AuthenticationCredentialsNotFoundException or
org.springframework.security.access.AccessDeniedException.
In
`@src/main/java/org/example/team6backend/auditlog/controller/AuditLogController.java`:
- Around line 31-35: The endpoint getLogsByUser in AuditLogController currently
accepts a Long userId which mismatches AppUser.id (String); change the method
signature to use `@PathVariable` String userId and update the call to
auditLogRepository.findByPerformedBy(userId) to pass the String userId (matching
the corrected AuditLogRepository.findByPerformedBy signature). Ensure method
parameter type and repository invocation both use String to stay consistent with
AppUser.id.
- Around line 20-29: The getLogs endpoint in AuditLogController currently
returns unbounded lists and must be converted to paginated responses: change
AuditLogController.getLogs to accept a Pageable (or use `@PageableDefault`) and
return ResponseEntity<Page<AuditLog>>; replace calls to
auditLogRepository.findAll(Sort...) and auditLogRepository.searchLogs(search,
Sort...) with their pageable equivalents (e.g.,
auditLogRepository.findAll(pageable) and auditLogRepository.searchLogs(search,
pageable)). Update the repository method searchLogs signature to accept a
Pageable and return Page<AuditLog>, and ensure callers and imports are adjusted
accordingly; also keep sorting via Pageable to preserve createdAt DESC.
In
`@src/main/java/org/example/team6backend/auditlog/repository/AuditLogRepository.java`:
- Around line 12-15: The search is filtering on AuditLog.userName but
AuditLogService.log(...) never sets that field, so either populate userName when
creating the audit record or change the repository query to search the
relationship field; specifically, either update AuditLogService.log to call
log.setUserName(user.getName()) (or the appropriate getter) when building the
AuditLog entity (ensure AuditLog.setUserName and the user variable are used), or
modify AuditLogRepository.searchLogs' JPQL to use LOWER(a.performedBy.name) LIKE
LOWER(CONCAT('%', :s, '%')) (keeping the rest of the query and method signature
intact).
- Line 17: AuditLogRepository.findByPerformedBy currently takes a Long but
AuditLog.performedBy maps to AppUser whose id is a String; update the repository
to query by the performedBy's id (e.g. declare List<AuditLog>
findByPerformedBy_Id(String userId) or accept AppUser) and change
AuditLogController.getLogsByUser to accept a String path variable (and pass that
String into the updated repository method) so the parameter types match
AppUser.id; reference AuditLog.performedBy and AppUser.id when making the
change.
In
`@src/main/java/org/example/team6backend/auditlog/service/AuditLogService.java`:
- Around line 16-24: The log(...) method in AuditLogService is not populating
the AuditLog.entity fields userName, targetType, and targetId (only action,
details, createdAt, performedBy are set), so searches by userName and structured
queries by targetType/targetId will fail; fix by adding an overloaded method
AuditLogService.log(String action, String details, AppUser user, String
userName, String targetType, String targetId) (or optional params) and set
log.setUserName(userName), log.setTargetType(targetType), and
log.setTargetId(targetId) before calling auditLogRepository.save(log), then
update callers to pass the known target info (or call the new overload where
available) so those DB columns are populated for AuditLogRepository queries.
- Around line 11-24: AuditLogService.log currently calls auditLogRepository.save
directly (joining the caller transaction) so audit failures can roll back
business work; change to a post-commit approach so auditing observes successful
transactions only: publish a simple AuditEvent from AuditLogService.log (or
register a TransactionSynchronization via
TransactionSynchronizationManager.registerSynchronization) and move the actual
persist logic into an event listener annotated with
`@TransactionalEventListener`(phase = TransactionPhase.AFTER_COMMIT) (or a
TransactionSynchronization.afterCommit callback) that writes via
auditLogRepository.save; if you keep REQUIRES_NEW for AuditLogService.log, wrap
the save in try/catch and log errors to avoid bubbling exceptions and document
the trade-offs.
In
`@src/main/java/org/example/team6backend/document/service/DocumentService.java`:
- Around line 96-110: The deleteFile method currently deletes the DB row
(documentRepository.delete) inside the transaction and then calls
minioService.deleteFile before the transaction commits, risking divergence if
the DB commit fails; change the flow so the irreversible Minio deletion happens
only after a successful commit by registering a post-commit action: remove or
move the minioService.deleteFile call out of the transactional block and either
(a) use TransactionSynchronizationManager.registerSynchronization to call
minioService.deleteFile(document.getFileKey()) in afterCommit(), or (b) emit an
event and handle it with a `@TransactionalEventListener`(phase = AFTER_COMMIT)
that performs minioService.deleteFile; keep auditLogService.log inside the
transaction (or log before the post-commit deletion) and ensure exceptions from
the post-commit Minio delete are logged/warned but do not affect the
already-committed DB state.
In
`@src/main/java/org/example/team6backend/incident/service/IncidentService.java`:
- Around line 286-289: The audit log call in IncidentService currently
concatenates the AppUser object previousHandler directly, causing a
toString()/PII leak; update the UNASSIGN_INCIDENT audit message (the
auditLogService.log(...) invocation in IncidentService) to use
previousHandler.getName() (or safely resolve a display name) instead of
previousHandler, so the log contains only the handler's name and not the full
AppUser object.
In `@src/main/resources/db/migration/V11__create_audit_log_table.sql`:
- Around line 1-10: Update the audit_log table schema: add a foreign key
constraint on audit_log.user_id referencing app_user(id) with your chosen delete
behavior (e.g., ON DELETE SET NULL if you want to keep audit rows, or ON DELETE
CASCADE to remove them), make created_at and action NOT NULL, and create indexes
to support queries (at minimum an index on created_at for ordering plus indexes
on user_id, user_name, and action; consider composite indexes like (user_id,
created_at DESC), (user_name, created_at DESC), or (action, created_at DESC) to
optimize the controller’s typical filter+order queries).
---
Outside diff comments:
In
`@src/main/java/org/example/team6backend/document/controller/DocumentController.java`:
- Around line 38-53: Guard against a null AuthenticationPrincipal in the
document controller methods: add an explicit null check for the method parameter
userDetails at the start of getFile (and replicate in uploadFile and deleteFile)
and throw a ResponseStatusException with HttpStatus.UNAUTHORIZED if userDetails
is null rather than calling userDetails.getUser(); reference the
CustomUserDetails type and use the existing logging pattern (log.info) to record
the unauthorized access before throwing so the request fails with 401 and still
allows auditing/handling.
In
`@src/main/java/org/example/team6backend/document/service/DocumentService.java`:
- Around line 67-94: The audit is currently recorded immediately in
DocumentService.downloadFile right after obtaining the InputStream from
minioService.downloadFile, producing false positives if the stream fails during
transfer; instead wrap the returned InputStream in a proxy (e.g., a
FilterInputStream/Wrapper) that overrides close() to call
auditLogService.log("DOWNLOAD_DOCUMENT", ...) only after the stream is
successfully closed (and optionally on successful EOF), and return that wrapped
stream from downloadFile; keep existing Minio Service exception handling intact
and ensure the wrapper delegates all read/close methods to the underlying stream
so behavior is unchanged except for logging on close.
In `@src/test/java/org/example/team6backend/admin/AdminControllerTest.java`:
- Around line 62-111: Tests set the Authentication in SecurityContextHolder
within setUp() but never clear it, causing state leakage across tests; add a
symmetric cleanup method annotated with `@AfterEach` (e.g., tearDown()) that calls
SecurityContextHolder.clearContext() to remove the Authentication set by
setUp(), referencing the existing setUp() and SecurityContextHolder to locate
where to add the cleanup.
---
Nitpick comments:
In
`@src/main/java/org/example/team6backend/auditlog/controller/AuditLogController.java`:
- Around line 12-37: Replace direct repository access in AuditLogController with
a service layer: create an AuditLogService (e.g., class AuditLogService with
methods List<AuditLog> findLogs(String search, Sort sort) and List<AuditLog>
findByUser(Long userId)) that encapsulates the calls to
AuditLogRepository.searchLogs(...) and findAll(...) / findByPerformedBy(...),
then inject AuditLogService into AuditLogController (replace the
AuditLogRepository field) and delegate getLogs(...) to
auditLogService.findLogs(search, Sort.by(...)) and getLogsByUser(...) to
auditLogService.findByUser(userId); keep repository usage only inside
AuditLogService so future pagination, sanitization and retention logic can be
added there.
In `@src/main/java/org/example/team6backend/auditlog/entity/AuditLog.java`:
- Around line 19-24: Add JPA column annotations on the AuditLog entity: mark the
fields action and createdAt with `@Column`(nullable = false) to reflect the NOT
NULL migration and ensure AuditLogService.log(...) invariants are enforced at
the schema level, and annotate details with `@Column`(columnDefinition = "TEXT")
so JPA/DDL tooling maps it to a TEXT column instead of a default VARCHAR(255);
update the AuditLog class fields action, createdAt, and details accordingly.
- Around line 26-28: The performedBy association on AuditLog is currently eager;
change the `@ManyToOne` on the performedBy field in AuditLog to use FetchType.LAZY
and tighten the join column by specifying explicit column constraints (e.g. set
nullable appropriately and add a ForeignKey name) in the `@JoinColumn` so the JPA
mapping mirrors the DB FK intent; update any repository queries or service code
that need the user to use fetch joins (or EntityGraph) when loading AuditLog
with its AppUser.
In `@src/main/java/org/example/team6backend/user/service/UserService.java`:
- Around line 119-166: Both updateUserRole and updateUserActiveStatus perform
unnecessary saves and audit logs when there is no actual change; modify
updateUserRole (compare oldRole to newRole) and updateUserActiveStatus (compare
oldStatus to active) after retrieving the AppUser via getUserById and, if equal,
log a debug message and return the user immediately without calling
userRepository.save or auditLogService.log; keep the existing checks for
last-admin deactivation in updateUserActiveStatus before the equality check or
adjust to only run when attempting to deactivate (active == false) and the
status would actually change.
In `@src/test/java/org/example/team6backend/admin/AdminControllerTest.java`:
- Around line 113-221: Remove the redundant `@WithMockUser`(roles = "ADMIN")
annotations from each test method that already uses .with(authentication(auth))
(e.g., shouldGetAllUsers, shouldGetUsersWithSearch, shouldGetPendingUsers,
shouldGetUserById, shouldApprovePendingUser, shouldUpdateUserRole,
shouldUpdateUserStatus, shouldDeleteUser) so the tests rely only on the explicit
UsernamePasswordAuthenticationToken provided via authentication(auth); delete
the `@WithMockUser` lines and any unused imports related to it to keep intent
unambiguous.
In
`@src/test/java/org/example/team6backend/incident/service/IncidentServiceTest.java`:
- Around line 57-58: Add a verification that an audit event is emitted in the
happy-path delete test: after calling IncidentService.deleteIncident(id, user)
in IncidentServiceTest, add a Mockito verify call to AuditLogService
(auditLogService) such as verify(auditLogService).log(...) with an
ArgumentMatcher or ArgumentCaptor to assert the expected audit entry (user,
action/type, target id). Locate the deleteIncident test method in
IncidentServiceTest and add this verify; you can mirror the same pattern in
other happy-path tests (e.g., the test exercising
DocumentService.uploadFile(..., user)) to lock the audit contract.
In `@src/test/java/org/example/team6backend/user/service/UserServiceTest.java`:
- Around line 41-43: One of the audited operations is missing an assertion that
AuditLogService.log was invoked; mirror the pattern used in
updateUserRole_ShouldUpdateRole by adding a
verify(auditLogService).log(anyString(), anyString(), eq(adminUser)) assertion
to at least one happy-path test (e.g., deleteUser_ShouldDeleteUser,
approvePendingUser_ShouldApproveUser, or
updateUserActiveStatus_ShouldUpdateStatus) so the test verifies the audit event
is emitted; locate the corresponding test method
(deleteUser/approvePendingUser/updateUserActiveStatus) and add the verify call
after exercising the service, making sure necessary Mockito matchers
(anyString(), eq(adminUser)) are available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60847024-49fc-40fb-8e54-6d5af4064ad4
📒 Files selected for processing (16)
src/main/java/org/example/team6backend/admin/AdminController.javasrc/main/java/org/example/team6backend/auditlog/controller/AuditLogController.javasrc/main/java/org/example/team6backend/auditlog/entity/AuditLog.javasrc/main/java/org/example/team6backend/auditlog/repository/AuditLogRepository.javasrc/main/java/org/example/team6backend/auditlog/service/AuditLogService.javasrc/main/java/org/example/team6backend/document/controller/DocumentController.javasrc/main/java/org/example/team6backend/document/service/DocumentService.javasrc/main/java/org/example/team6backend/incident/service/IncidentService.javasrc/main/java/org/example/team6backend/user/service/UserService.javasrc/main/resources/db/migration/V11__create_audit_log_table.sqlsrc/test/java/org/example/team6backend/admin/AdminControllerTest.javasrc/test/java/org/example/team6backend/document/controller/DocumentControllerTest.javasrc/test/java/org/example/team6backend/document/service/DocumentServiceTest.javasrc/test/java/org/example/team6backend/incident/controller/IncidentControllerTest.javasrc/test/java/org/example/team6backend/incident/service/IncidentServiceTest.javasrc/test/java/org/example/team6backend/user/service/UserServiceTest.java
- Populate userName, targetType, and targetId in AuditLogService.log() - Fix findByPerformedBy type mismatch (Long → String for AppUser.id) - Add pagination to audit log endpoints (Pageable instead of Sort) - Reorder DocumentService.deleteFile() to delete MinIO after commit - Change IllegalStateException to AccessDeniedException in AdminController - Fix Flyway migration file naming (double underscore) - Add missing AuditLogService mocks to all tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements