Enhance audit logging: add event name and description, refine mutatio…#74
Conversation
…n tracking, and update UI to display detailed audit event information.
|
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 5 minutes and 49 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 (4)
📝 WalkthroughWalkthroughReplaces persisted HTTP method with semantic Changes
Sequence DiagramsequenceDiagram
participant Client
participant Interceptor as AuditInterceptor
participant Domain as DomainService
participant AuditSvc as AuditService
participant DB as AuditEventEntity (Repository/DB)
rect rgba(100,150,200,0.5)
Note over Client,Interceptor: Incoming web request
Client->>Interceptor: HTTP Request (method, uri)
Interceptor->>Interceptor: Is mutation? (POST/PUT/DELETE/PATCH) or error present
alt Mutation or error
Interceptor->>AuditSvc: Build AuditEvent (eventName="WEB_ACTION", description="<METHOD> <URI>", path..., occurredAt=now)
AuditSvc->>DB: Persist audit event (after transaction commit)
AuditSvc->>AuditSvc: Broadcast DTO -> active SseEmitters
else Non-mutation without error
Interceptor-->>Interceptor: Skip audit
end
end
rect rgba(150,200,100,0.5)
Note over Client,Domain: Domain-triggered audit
Client->>Domain: Domain operation (create/update/upload)
Domain->>Domain: Business logic
Domain->>AuditSvc: Build AuditEvent (eventName e.g., "CASE_CREATED", description, occurredAt=now)
AuditSvc->>DB: Persist audit event (after transaction commit)
AuditSvc->>AuditSvc: Broadcast DTO -> active SseEmitters
end
rect rgba(200,150,180,0.5)
Note over Client,AuditSvc: SSE consumer (UI)
Client->>AuditSvc: GET /api/audit/stream (requires manager)
AuditSvc-->>Client: SseEmitter stream ("connected", "audit-event"...)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
AUDIT.md (2)
33-51:⚠️ Potential issue | 🟡 MinorData model table is missing the new
eventNameanddescriptionfields.The PR removed
httpMethodfrom the table but did not add rows for the newly persistedeventNameanddescriptioncolumns ofAuditEventEntity. The documentation now under-describes the schema.| `statusChange` | String | ... | +| `eventName` | String | High-level event identifier (e.g., `WEB_ACTION`, `CASE_CREATED`, `STATUS_CHANGED`, `DOCUMENT_UPLOADED`) | +| `description` | String | Human-readable description of the event | | `clientIp` | String | `X-Forwarded-For` first hop, else `remoteAddr` |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AUDIT.md` around lines 33 - 51, The audit_events schema doc was not updated to include the new fields from AuditEventEntity: add rows for eventName (String - logical event identifier) and description (String - human-readable event description) to the table definition, and remove the obsolete httpMethod row if still present; ensure the documentation mentions these fields are persisted by the interceptor (matching AuditEventEntity.eventName and AuditEventEntity.description) and update the Indexes/notes if either field requires indexing or sanitization like queryString.
193-196:⚠️ Potential issue | 🟡 MinorTest description is out of date.
The referenced test now performs
POST /ui/cases/new(a mutation), notGET /ui/cases. Also, the new audit policy only audits mutation requests, which is the entire point of the test change — worth reflecting here.-`ProjektArendehanteringApplicationTests.uiRequest_createsAuditEvent` asserts that an authenticated `GET /ui/cases` increases `AuditEventRepository.count()`, verifying the interceptor → `record` path. +`ProjektArendehanteringApplicationTests.uiRequest_createsAuditEvent` asserts that an authenticated `POST /ui/cases/new` (a mutation) increases `AuditEventRepository.count()`, verifying the interceptor → `record` path and the mutation-only audit policy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AUDIT.md` around lines 193 - 196, Update the Tests section to reflect that ProjektArendehanteringApplicationTests.uiRequest_createsAuditEvent now issues a POST to /ui/cases/new (a mutation) rather than GET /ui/cases, and state that the current audit policy audits mutation requests only; specifically mention that the test asserts an authenticated POST /ui/cases/new increases AuditEventRepository.count(), verifying the interceptor → record path under the mutation-only audit policy.src/main/java/org/example/projektarendehantering/application/service/DocumentService.java (1)
99-128:⚠️ Potential issue | 🟠 MajorAudit recording inside the compensation
try— verify intended semantics.
auditService.record(...)is invoked inside thetryblock that triggers S3 cleanup onRuntimeException. If the audit save itself throws (e.g., DB constraint oncedescriptionlength is bounded, see sibling comment), the outercatchwill clean up the S3 object and rethrow, which means an audit failure can now cascade into failing the user's upload. PerAUDIT.mdline 92, audit failures are supposed to never break the user’s request. Consider wrappingauditService.record(...)in its own try/catch here as well, mirroring the interceptor contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java` around lines 99 - 128, The auditService.record(...) calls inside DocumentService's save/compensation try block must be isolated so audit failures don't trigger the outer RuntimeException path and S3 cleanup; wrap each auditService.record(...) invocation in its own try/catch that catches Exception (or RuntimeException), logs the audit failure (with context: caseId, eventName, description, actorId) and swallows the exception (do not rethrow) so the user's document upload succeeds even if audit persistence fails.
🧹 Nitpick comments (8)
src/main/java/org/example/projektarendehantering/application/service/CaseService.java (1)
328-343:occurredAtis now set twice — benign but redundant.
AuditService.record()already defaultsoccurredAttoInstant.now()when null (documented inAUDIT.mdline 89). Setting it here is harmless but creates two sources of truth; if someone later adds server-clock-sensitive logic inrecord()(e.g., clock skew handling), the pre-set value will silently win. Consider either leaving it torecord()or — preferably — makingrecord()authoritative and dropping it from the builders in bothCaseServiceandDocumentService.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/application/service/CaseService.java` around lines 328 - 343, Remove the redundant occurredAt assignment in the AuditEventEntity builder inside recordStatusChange so auditService.record(...) can set the timestamp authoritatively; specifically, edit the recordStatusChange method to stop calling .occurredAt(Instant.now()) on the AuditEventEntity builder (and make the same change in DocumentService where AuditEventEntity is built) so that AuditService.record() is the single place that populates occurredAt.src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java (2)
112-133: Validation error details are dropped.For
MethodArgumentNotValidException, the generic"Validation failed"message hides the actual field errors, which makes it difficult for API clients (and the user-facing error view) to know what to fix. Consider surfacing field-level errors fromgetBindingResult().getFieldErrors()for REST requests (e.g., include afieldErrorslist in the body). Same forHttpMessageNotReadableException's most-specific cause when safe to expose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java` around lines 112 - 133, In handleBadRequestException in GlobalControllerAdvice, don't return only the generic "Validation failed": when e is a MethodArgumentNotValidException extract field errors via ((MethodArgumentNotValidException)e).getBindingResult().getFieldErrors() and include a serializable fieldErrors list (field, rejectedValue, defaultMessage) in the REST ResponseEntity body and also add it to the Model for non-REST views; for HttpMessageNotReadableException include the most-specific safe cause message (e.getMostSpecificCause().getMessage()) in the response as an additional detail property (only for REST or sanitized for views) so clients receive actionable error details.
106-110: 404 handler bypasses REST/HTML response conventions.Unlike the other handlers in this class,
handleNoResourceFoundExceptionreturnsvoidwith@ResponseStatus, so Spring falls back to its default error dispatch (e.g.BasicErrorController) instead of producing the JSON body{errorCode, message, status}used by/api/**callers or theerrorview used elsewhere. If REST clients rely on the consistent error envelope, a missing API path will now return a differently-shaped response than e.g.AccessDeniedExceptionor the newhandleBadRequestException.Consider mirroring the REST/HTML branching used in the neighboring handlers if consistency matters:
♻️ Suggested shape
- `@ExceptionHandler`(NoResourceFoundException.class) - `@ResponseStatus`(HttpStatus.NOT_FOUND) - public void handleNoResourceFoundException(NoResourceFoundException e) { - // Just return 404, don't log as ERROR - } + `@ExceptionHandler`(NoResourceFoundException.class) + public Object handleNoResourceFoundException(NoResourceFoundException e, + HttpServletRequest request, + HttpServletResponse response, + Model model) { + HttpStatus status = HttpStatus.NOT_FOUND; + if (isRestRequest(request)) { + return ResponseEntity.status(status).body(Map.of( + "errorCode", "NOT_FOUND", + "message", "Resource not found", + "status", status.value())); + } + response.setStatus(status.value()); + model.addAttribute("status", status.value() + " " + status.getReasonPhrase()); + model.addAttribute("message", "Resource not found"); + model.addAttribute("errorCode", "NOT_FOUND"); + return "error"; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java` around lines 106 - 110, The NoResourceFoundException handler currently returns void with `@ResponseStatus` which triggers Spring's default error dispatch; update handleNoResourceFoundException(NoResourceFoundException e) to mirror the neighboring handlers' REST/HTML branching: accept HttpServletRequest (and optionally HttpServletResponse), detect API requests (e.g. request.getServletPath().startsWith("/api/") or check Accept header), and for API callers return the same JSON error envelope used elsewhere (errorCode, message, status) as a ResponseEntity, while for non-API callers return the same error view (ModelAndView "error") used by the other handlers so responses are consistent across clients. Ensure you reuse the same error envelope construction logic and view name used by the other handlers in this class.src/test/java/org/example/projektarendehantering/ProjektArendehanteringApplicationTests.java (1)
40-46: Brittle assumption about 200 OK for missing body.The comment says the endpoint returns 200 because the body is missing (re-renders the form). This is a behavioral coupling — if
/ui/cases/newever validates CSRF + empty body and redirects/returns 4xx, the test will fail for reasons unrelated to auditing. Since the intent here is only to assert that a mutation produces an audit row, consider assertingstatus().is2xxSuccessful()(or a broader matcher) so the test remains focused on the audit count delta.- mockMvc.perform(post("/ui/cases/new").with(csrf())) - .andExpect(status().isOk()); // Returns 200 (re-renders form) because body is missing + mockMvc.perform(post("/ui/cases/new").with(csrf())); + // Status intentionally not asserted; this test only verifies that + // mutation requests produce an audit event.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/projektarendehantering/ProjektArendehanteringApplicationTests.java` around lines 40 - 46, The test assumes a 200 OK for mockMvc.perform(post("/ui/cases/new").with(csrf())) which is brittle; change the status assertion to a broader 2xx check (e.g., use status().is2xxSuccessful()) so the test remains focused on the auditEventRepository.count() delta and not the exact response behavior; update the assertion around mockMvc.perform(...) and keep the subsequent assertThat(after).isGreaterThan(before) as-is.AUDIT.md (1)
70-82: Interceptor description no longer mentionseventName/description.Step 3 was updated to drop HTTP method, but the interceptor now also sets
eventName = "WEB_ACTION"anddescription = "<method> <requestURI>". Adding a step (or note) here would keep the workflow description accurate and discoverable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AUDIT.md` around lines 70 - 82, The audit doc's "Building the event" steps omit that the interceptor now sets eventName and description; update the section (near the list of fields like actorId/principalName and the step about request metadata) to state that the interceptor sets eventName = "WEB_ACTION" and description = "<method> <requestURI>" (i.e., HTTP method followed by the request URI), so the documentation reflects the actual behavior of the interceptor that also constructs these two fields.src/test/java/org/example/projektarendehantering/AuditIntegrationTest.java (1)
60-68: Consider asserting the neweventNameanddescriptionfields.The PR introduces
eventName("WEB_ACTION") anddescription(" ") on audit events, but the integration test no longer asserts either. Adding lightweight assertions (e.g.,assertThat(latest.getEventName()).isEqualTo("WEB_ACTION");andassertThat(latest.getDescription()).contains("POST").contains("/api/cases");) would lock in the new contract and catch regressions in the interceptor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/projektarendehantering/AuditIntegrationTest.java` around lines 60 - 68, AuditIntegrationTest currently checks the persisted AuditEventEntity via auditEventRepository but misses assertions for the new fields; update the test to assert the new eventName and description contract by adding assertions on the retrieved AuditEventEntity (latest) such as asserting latest.getEventName().equals("WEB_ACTION") and that latest.getDescription() contains the HTTP method and path (e.g., contains("POST") and contains("/api/cases")), keeping the existing requestPath assertion via latest.getRequestPath() to validate the interceptor behavior.src/main/resources/templates/audit/list.html (1)
65-65:#strings.substringon a UUID relies on implicittoString().
e.caseIdis likely aUUID, not aString. Thymeleaf's#strings.substringgenerally coerces viatoString(), but being explicit avoids surprises and is more robust. Also consider using${#strings.abbreviate(e.caseId, 11)}or similar for the '...' affordance.Proposed fix
- <a th:if="${e.caseId != null}" th:href="@{/ui/cases/{id}(id=${e.caseId})}" th:text="${`#strings.substring`(e.caseId,0,8)} + '...'">case</a> + <a th:if="${e.caseId != null}" th:href="@{/ui/cases/{id}(id=${e.caseId})}" th:text="${`#strings.substring`(e.caseId.toString(),0,8) + '...'}">case</a>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/audit/list.html` at line 65, The template currently calls `#strings.substring` on e.caseId which may be a UUID; explicitly convert e.caseId to string (e.g., use e.caseId.toString()) before substring to avoid implicit coercion, or replace the expression with Thymeleaf's `#strings.abbreviate`(e.caseId.toString(), 11) to produce a safe 8-char + '...' display; update the expression that references e.caseId and the `#strings.substring/`#strings.abbreviate call accordingly.src/main/java/org/example/projektarendehantering/infrastructure/web/AuditInterceptor.java (1)
60-61: Description duplicates fields already captured; consider sanitizing path tokens.
descriptionis"<method> <requestURI>", whilerequestPathalready stores the URI and the HTTP method is implicit in the mutation filter. If the goal is a human-readable summary, consider a more meaningful value (e.g., handler-derived action). Additionally,requestURIcan embed sensitive path variables (tokens, emails);AuditServiceonly sanitizesqueryString, so such values will land unsanitized indescription. Worth confirming this is acceptable for your audit retention/PII posture.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/infrastructure/web/AuditInterceptor.java` around lines 60 - 61, The description currently duplicates requestPath and may leak sensitive path variables: update the AuditInterceptor so event.setDescription(...) does not include raw request.getRequestURI(); instead derive a concise human-readable action (e.g., handler or controller method name) or sanitize path tokens before setting description; reference the event.setDescription call and request.getRequestURI usage and either replace it with a handler-derived value (use the handler/controller info available in the interceptor) or run the URI through the same sanitization used for queryString in AuditService so requestPath and description remain safe and non-duplicative.
🤖 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/projektarendehantering/application/service/DocumentService.java`:
- Around line 118-127: The audit entry writes the raw uploaded filename into
AuditEventEntity.description via the DocumentService call to
auditService.record(...) using entity.getFileName(), which risks leaking PII and
unbounded length; update DocumentService to sanitize/redact and bound the
filename before building the AuditEventEntity (e.g., replace sensitive segments
or mask characters and/or truncate to a safe max length) and ensure
AuditEventEntity.description has a matching column length constraint so the
stored description cannot exceed DB limits; reference the builder usage around
AuditEventEntity.builder() and the entity.getFileName() call when applying these
changes.
In
`@src/main/java/org/example/projektarendehantering/infrastructure/web/AuditInterceptor.java`:
- Around line 31-40: The current guard in afterCompletion uses isMutation and ex
to skip auditing non-mutations, which misses non-mutating requests that failed
without throwing; update the condition in AuditInterceptor.afterCompletion to
also consider HTTP response status by checking HttpServletResponse.getStatus()
(e.g., treat status >= 400 as an error) so that when ex == null but
response.getStatus() >= 400 you still proceed with audit logging; refer to the
isMutation boolean, the ex parameter and response.getStatus() when modifying the
early-return logic.
In `@src/main/resources/templates/audit/list.html`:
- Line 51: Replace the fragile expression that directly calls
`#temporals.format`(e.occurredAt, ...) with a null-safe, zone-explicit expression
in the template cell: guard e.occurredAt (e.g. use a ternary or th:if) so a null
occurredAt renders a safe placeholder like "—" instead of throwing, and
convert/format the timestamp in UTC (for example by converting to a
ZonedDateTime with T(java.time.ZoneId).of('UTC') before formatting) so the audit
time is not rendered using the JVM default zone; update the expression that
references e.occurredAt in list.html accordingly.
---
Outside diff comments:
In `@AUDIT.md`:
- Around line 33-51: The audit_events schema doc was not updated to include the
new fields from AuditEventEntity: add rows for eventName (String - logical event
identifier) and description (String - human-readable event description) to the
table definition, and remove the obsolete httpMethod row if still present;
ensure the documentation mentions these fields are persisted by the interceptor
(matching AuditEventEntity.eventName and AuditEventEntity.description) and
update the Indexes/notes if either field requires indexing or sanitization like
queryString.
- Around line 193-196: Update the Tests section to reflect that
ProjektArendehanteringApplicationTests.uiRequest_createsAuditEvent now issues a
POST to /ui/cases/new (a mutation) rather than GET /ui/cases, and state that the
current audit policy audits mutation requests only; specifically mention that
the test asserts an authenticated POST /ui/cases/new increases
AuditEventRepository.count(), verifying the interceptor → record path under the
mutation-only audit policy.
In
`@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java`:
- Around line 99-128: The auditService.record(...) calls inside
DocumentService's save/compensation try block must be isolated so audit failures
don't trigger the outer RuntimeException path and S3 cleanup; wrap each
auditService.record(...) invocation in its own try/catch that catches Exception
(or RuntimeException), logs the audit failure (with context: caseId, eventName,
description, actorId) and swallows the exception (do not rethrow) so the user's
document upload succeeds even if audit persistence fails.
---
Nitpick comments:
In `@AUDIT.md`:
- Around line 70-82: The audit doc's "Building the event" steps omit that the
interceptor now sets eventName and description; update the section (near the
list of fields like actorId/principalName and the step about request metadata)
to state that the interceptor sets eventName = "WEB_ACTION" and description =
"<method> <requestURI>" (i.e., HTTP method followed by the request URI), so the
documentation reflects the actual behavior of the interceptor that also
constructs these two fields.
In
`@src/main/java/org/example/projektarendehantering/application/service/CaseService.java`:
- Around line 328-343: Remove the redundant occurredAt assignment in the
AuditEventEntity builder inside recordStatusChange so auditService.record(...)
can set the timestamp authoritatively; specifically, edit the recordStatusChange
method to stop calling .occurredAt(Instant.now()) on the AuditEventEntity
builder (and make the same change in DocumentService where AuditEventEntity is
built) so that AuditService.record() is the single place that populates
occurredAt.
In
`@src/main/java/org/example/projektarendehantering/infrastructure/web/AuditInterceptor.java`:
- Around line 60-61: The description currently duplicates requestPath and may
leak sensitive path variables: update the AuditInterceptor so
event.setDescription(...) does not include raw request.getRequestURI(); instead
derive a concise human-readable action (e.g., handler or controller method name)
or sanitize path tokens before setting description; reference the
event.setDescription call and request.getRequestURI usage and either replace it
with a handler-derived value (use the handler/controller info available in the
interceptor) or run the URI through the same sanitization used for queryString
in AuditService so requestPath and description remain safe and non-duplicative.
In
`@src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java`:
- Around line 112-133: In handleBadRequestException in GlobalControllerAdvice,
don't return only the generic "Validation failed": when e is a
MethodArgumentNotValidException extract field errors via
((MethodArgumentNotValidException)e).getBindingResult().getFieldErrors() and
include a serializable fieldErrors list (field, rejectedValue, defaultMessage)
in the REST ResponseEntity body and also add it to the Model for non-REST views;
for HttpMessageNotReadableException include the most-specific safe cause message
(e.getMostSpecificCause().getMessage()) in the response as an additional detail
property (only for REST or sanitized for views) so clients receive actionable
error details.
- Around line 106-110: The NoResourceFoundException handler currently returns
void with `@ResponseStatus` which triggers Spring's default error dispatch; update
handleNoResourceFoundException(NoResourceFoundException e) to mirror the
neighboring handlers' REST/HTML branching: accept HttpServletRequest (and
optionally HttpServletResponse), detect API requests (e.g.
request.getServletPath().startsWith("/api/") or check Accept header), and for
API callers return the same JSON error envelope used elsewhere (errorCode,
message, status) as a ResponseEntity, while for non-API callers return the same
error view (ModelAndView "error") used by the other handlers so responses are
consistent across clients. Ensure you reuse the same error envelope construction
logic and view name used by the other handlers in this class.
In `@src/main/resources/templates/audit/list.html`:
- Line 65: The template currently calls `#strings.substring` on e.caseId which may
be a UUID; explicitly convert e.caseId to string (e.g., use e.caseId.toString())
before substring to avoid implicit coercion, or replace the expression with
Thymeleaf's `#strings.abbreviate`(e.caseId.toString(), 11) to produce a safe
8-char + '...' display; update the expression that references e.caseId and the
`#strings.substring/`#strings.abbreviate call accordingly.
In `@src/test/java/org/example/projektarendehantering/AuditIntegrationTest.java`:
- Around line 60-68: AuditIntegrationTest currently checks the persisted
AuditEventEntity via auditEventRepository but misses assertions for the new
fields; update the test to assert the new eventName and description contract by
adding assertions on the retrieved AuditEventEntity (latest) such as asserting
latest.getEventName().equals("WEB_ACTION") and that latest.getDescription()
contains the HTTP method and path (e.g., contains("POST") and
contains("/api/cases")), keeping the existing requestPath assertion via
latest.getRequestPath() to validate the interceptor behavior.
In
`@src/test/java/org/example/projektarendehantering/ProjektArendehanteringApplicationTests.java`:
- Around line 40-46: The test assumes a 200 OK for
mockMvc.perform(post("/ui/cases/new").with(csrf())) which is brittle; change the
status assertion to a broader 2xx check (e.g., use status().is2xxSuccessful())
so the test remains focused on the auditEventRepository.count() delta and not
the exact response behavior; update the assertion around mockMvc.perform(...)
and keep the subsequent assertThat(after).isGreaterThan(before) as-is.
🪄 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: a4136a6f-edd6-486a-8074-6d8242a18a6d
📒 Files selected for processing (12)
AUDIT.mdsrc/main/java/org/example/projektarendehantering/application/service/AuditEventMapper.javasrc/main/java/org/example/projektarendehantering/application/service/CaseService.javasrc/main/java/org/example/projektarendehantering/application/service/DocumentService.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/AuditEventEntity.javasrc/main/java/org/example/projektarendehantering/infrastructure/web/AuditInterceptor.javasrc/main/java/org/example/projektarendehantering/presentation/dto/AuditEventDTO.javasrc/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.javasrc/main/resources/templates/audit/list.htmlsrc/test/java/org/example/projektarendehantering/AuditIntegrationTest.javasrc/test/java/org/example/projektarendehantering/ProjektArendehanteringApplicationTests.javasrc/test/java/org/example/projektarendehantering/presentation/rest/AuditControllerTest.java
💤 Files with no reviewable changes (1)
- src/test/java/org/example/projektarendehantering/presentation/rest/AuditControllerTest.java
| } else { | ||
| auditService.record(AuditEventEntity.builder() | ||
| .caseId(caseEntity.getId()) | ||
| .eventName("DOCUMENT_UPLOADED") | ||
| .description("Document uploaded: " + entity.getFileName()) | ||
| .actorId(actor.userId()) | ||
| .actorRole(actor.role() != null ? actor.role().name() : null) | ||
| .occurredAt(Instant.now()) | ||
| .build()); | ||
| } |
There was a problem hiding this comment.
DOCUMENT_UPLOADED audit entry includes raw filename — consider length/PII handling.
entity.getFileName() comes directly from the uploaded MultipartFile.getOriginalFilename() and is written verbatim into description. Since:
- Filenames can legitimately contain PII or sensitive content (e.g.,
patient-1234-ssn.pdf), and - The
queryStringsanitization path inAuditService.record()(perAUDIT.md) only scrubs thequeryStringfield, leavingdescriptionuntouched,
this can leak sensitive data into the audit table and onto the audit UI. Consider either truncating/escaping the filename, or documenting that audit description is a non-sanitized field so operators can reason about it.
Additionally, no upper bound on description length means a pathologically long filename could exceed the default column length once a DDL is generated with a length constraint. A @Column(length = ...) on AuditEventEntity.description with matching truncation here would make behavior predictable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java`
around lines 118 - 127, The audit entry writes the raw uploaded filename into
AuditEventEntity.description via the DocumentService call to
auditService.record(...) using entity.getFileName(), which risks leaking PII and
unbounded length; update DocumentService to sanitize/redact and bound the
filename before building the AuditEventEntity (e.g., replace sensitive segments
or mask characters and/or truncate to a safe max length) and ensure
AuditEventEntity.description has a matching column length constraint so the
stored description cannot exceed DB limits; reference the builder usage around
AuditEventEntity.builder() and the entity.getFileName() call when applying these
changes.
| String method = request.getMethod(); | ||
| boolean isMutation = "POST".equalsIgnoreCase(method) || | ||
| "PUT".equalsIgnoreCase(method) || | ||
| "DELETE".equalsIgnoreCase(method) || | ||
| "PATCH".equalsIgnoreCase(method); | ||
|
|
||
| // Only record mutations or errors to reduce noise | ||
| if (!isMutation && ex == null) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Non-mutating requests that fail without throwing will silently skip audit.
afterCompletion receives ex only for exceptions that propagate out of the handler. Failures handled by @ExceptionHandler/@ControllerAdvice (e.g., AccessDeniedException, AuthenticationException, validation errors mapped to 4xx) typically arrive here with ex == null and a non-2xx response.getStatus(). With the new guard, a GET that is rejected with 401/403/500 via a controller advice will not be audited, which weakens audit coverage for exactly the reads security auditors usually care about.
Consider also auditing non-mutating requests when the response status indicates an error:
Proposed fix
- // Only record mutations or errors to reduce noise
- if (!isMutation && ex == null) {
- return;
- }
+ // Only record mutations or errors to reduce noise
+ int status = response != null ? response.getStatus() : 0;
+ boolean isError = ex != null || status >= 400;
+ if (!isMutation && !isError) {
+ return;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/example/projektarendehantering/infrastructure/web/AuditInterceptor.java`
around lines 31 - 40, The current guard in afterCompletion uses isMutation and
ex to skip auditing non-mutations, which misses non-mutating requests that
failed without throwing; update the condition in
AuditInterceptor.afterCompletion to also consider HTTP response status by
checking HttpServletResponse.getStatus() (e.g., treat status >= 400 as an error)
so that when ex == null but response.getStatus() >= 400 you still proceed with
audit logging; refer to the isMutation boolean, the ex parameter and
response.getStatus() when modifying the early-return logic.
| <td th:text="${e.actorRole}">role</td> | ||
| <td th:text="${e.actorId}">actor</td> | ||
| <td th:text="${e.httpMethod}">GET</td> | ||
| <td th:text="${#temporals.format(e.occurredAt, 'yyyy-MM-dd HH:mm:ss')}">time</td> |
There was a problem hiding this comment.
Formatting occurredAt can NPE and uses JVM default timezone.
#temporals.format(e.occurredAt, 'yyyy-MM-dd HH:mm:ss') will throw a TemplateProcessingException if any row has a null occurredAt, breaking the whole table render. Although AuditService.record backfills occurredAt, the DB/entity permits null and a single legacy row would break the audit page. Also, formatting without a zone uses the JVM default, which is surprising for audit timestamps — prefer UTC or a consistent zone.
Proposed fix
- <td th:text="${`#temporals.format`(e.occurredAt, 'yyyy-MM-dd HH:mm:ss')}">time</td>
+ <td th:text="${e.occurredAt != null ? `#temporals.formatISO`(e.occurredAt) : ''}">time</td>📝 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.
| <td th:text="${#temporals.format(e.occurredAt, 'yyyy-MM-dd HH:mm:ss')}">time</td> | |
| <td th:text="${e.occurredAt != null ? `#temporals.formatISO`(e.occurredAt) : ''}">time</td> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/templates/audit/list.html` at line 51, Replace the fragile
expression that directly calls `#temporals.format`(e.occurredAt, ...) with a
null-safe, zone-explicit expression in the template cell: guard e.occurredAt
(e.g. use a ternary or th:if) so a null occurredAt renders a safe placeholder
like "—" instead of throwing, and convert/format the timestamp in UTC (for
example by converting to a ZonedDateTime with T(java.time.ZoneId).of('UTC')
before formatting) so the audit time is not rendered using the JVM default zone;
update the expression that references e.occurredAt in list.html accordingly.
…and refine error handling in GlobalControllerAdvice
There was a problem hiding this comment.
Actionable comments posted: 5
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/org/example/projektarendehantering/application/service/AuditService.java (1)
68-99:⚠️ Potential issue | 🟠 MajorBroadcasting inside the transaction can leak phantom audit events on rollback.
broadcast(event)runs inside the@Transactionalboundary ofrecord(...)and pushes the DTO to all SSE clients before the transaction commits. If the surrounding transaction rolls back (e.g., a subsequent failure in the calling service), subscribers have already observed an "audit event" that was never persisted — a correctness problem for a live audit stream and potentially misleading from a compliance standpoint. Additionally, the sequentialemitter.send(...)calls perform blocking I/O while holding the DB transaction open, so a slow/stuck client can extend transaction duration.Consider deferring the broadcast to after commit:
🛠️ Proposed fix
`@Transactional` public void record(AuditEventEntity event) { if (event == null) return; if (event.getId() == null) { event.setId(UUID.randomUUID()); } if (event.getOccurredAt() == null) { event.setOccurredAt(Instant.now()); } event.setQueryString(sanitizeAuditPayload(event.getQueryString())); - auditEventRepository.save(event); - - broadcast(event); + AuditEventEntity saved = auditEventRepository.save(event); + + if (TransactionSynchronizationManager.isSynchronizationActive()) { + TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() { + `@Override` + public void afterCommit() { + broadcast(saved); + } + }); + } else { + broadcast(saved); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/application/service/AuditService.java` around lines 68 - 99, The broadcast is executed inside the `@Transactional` record(AuditEventEntity) method which can leak phantom events and perform blocking I/O while the DB transaction is open; refactor so persistence happens in record(...) but broadcasting runs after commit (and off the transaction thread). Specifically: keep auditEventRepository.save(event) in record(AuditEventEntity) and remove the direct call to broadcast(event); instead publish an application event or register a TransactionSynchronization that calls broadcast(AuditEventEntity) afterCommit (or use `@TransactionalEventListener`(phase = AFTER_COMMIT)) so broadcast(...) and its SseEmitter.send(...) calls run only after the transaction commits and off the DB transaction context. Ensure broadcast(AuditEventEntity) still handles IOException/IllegalStateException and emitter cleanup as before.
🧹 Nitpick comments (2)
src/main/java/org/example/projektarendehantering/application/service/AuditService.java (1)
118-129: Returning a pre-failed emitter back to Spring MVC.If the initial
"connected"event fails, the emitter is removed fromemittersbut still returned to the controller, which hands it to Spring MVC. Depending on the failure state, downstream logging may be noisy and the client gets a broken stream without a clear signal. Consider callingemitter.completeWithError(e)and returning early (or letting theIOExceptionpropagate) so the client sees a clean failure and Spring doesn't attempt to drive a dead emitter:🛠️ Proposed fix
} catch (IOException e) { log.error("Failed to send initial SSE event", e); emitters.remove(emitter); + emitter.completeWithError(e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/application/service/AuditService.java` around lines 118 - 129, The code currently removes the failed SseEmitter (emitter) from the emitters set but still returns it to the controller; change the error handling in the block that sends the initial "connected" event so that on IOException you call emitter.completeWithError(e) (or rethrow the IOException) and return early instead of returning the (now-broken) emitter; update the method that constructs the emitter (the code surrounding emitter, emitters, and the try/catch that sends the "connected" event in AuditService) to ensure the failed emitter is not returned to Spring MVC and that the exception is propagated or an explicit null/clean failure is returned.src/main/resources/templates/fragments/header.html (1)
44-84: Stream is opened for managers on every page regardless of endpoint access.Every non-
/ui/auditpage load for a manager opens a long-lived SSE connection to/api/audit/stream. Combined with the server side's single global emitter list and 30‑minute timeout, this can accumulate one connection per open tab per manager, multiplying server resources needed (threads/connections) and audit broadcast work. Consider opening the stream only on a dedicated page (or a shared worker), or closing it onvisibilitychange/pagehideto bound resource usage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/fragments/header.html` around lines 44 - 84, The SSE is being opened on every page because the current pathname check is inverted; change the guard to only create EventSource when on the audit page (replace if (window.location.pathname === '/ui/audit') return; with if (window.location.pathname !== '/ui/audit') return;), and add lifecycle cleanup: after creating const eventSource = new EventSource('/api/audit/stream'); register document.addEventListener('visibilitychange', ...) and window.addEventListener('pagehide', ...) handlers that call eventSource.close(), and also ensure eventSource.onerror continues to close on terminal states; reference the eventSource variable, the '/api/audit/stream' URL, and the 'auditLink' click handler for coordination.
🤖 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/projektarendehantering/presentation/web/GlobalControllerAdvice.java`:
- Around line 107-112: Split the combined handler in GlobalControllerAdvice into
two: keep a `@ExceptionHandler`(NoResourceFoundException.class) method (e.g.,
handleNotFound) annotated with `@ResponseStatus`(HttpStatus.NOT_FOUND) that
returns void and does not log, and move AsyncRequestNotUsableException to its
own `@ExceptionHandler`(AsyncRequestNotUsableException.class) method (e.g.,
handleAsyncDisconnect) that does NOT set a 404 status but logs at debug level
(use the logger) and returns void; remove AsyncRequestNotUsableException from
the existing handleSilentExceptions signature and ensure the new async handler
does not change the HTTP status.
In `@src/main/resources/templates/audit/list.html`:
- Around line 83-84: The SSE client code creates timeStr from data.occurredAt
using new Date(...).toISOString() (UTC), while server-rendered rows use
`#temporals.format`(...) with the JVM default zone, causing inconsistent
timestamps; make both use UTC: update the server-side formatting call (the
`#temporals.format`(...) invocation that formats occurredAt) to specify
ZoneId.of("UTC") so it renders yyyy-MM-dd HH:mm:ss in UTC, and keep the
client-side creation of timeStr from data.occurredAt (new
Date(...).toISOString().replace('T',' ').substring(0,19)); also apply the same
ZoneId.of("UTC") change to the other server-side occurrence referenced (the
second `#temporals.format` at the other location).
- Around line 64-78: The event stream handler currently double-reconnects and
can crash on bad JSON; update the onerror/connect logic to stop unconditionally
scheduling connect(): in the eventSource.onerror handler check the instance's
readyState and only call setTimeout(connect, 5000) when eventSource.readyState
=== EventSource.CLOSED (or remove the timer and rely on EventSource
auto-reconnect and only call connect() when readyState === CLOSED), and ensure
connect() still closes the previous eventSource reference before creating a new
one; wrap the JSON.parse(e.data) inside a try/catch in the 'audit-event'
listener (log and return on parse errors) to avoid aborting the handler; and
instead of calling location.reload() when document.getElementById('audit-table')
is missing, create a minimal table shell in the DOM (header/ tbody) so incoming
events can append rows without repeatedly reloading the page—refer to
eventSource, connect(), the 'audit-event' listener, JSON.parse(e.data), and
document.getElementById('audit-table') to locate the changes.
- Around line 85-108: The code injects untrusted fields into the DOM via
row.innerHTML (notably data.description, data.requestPath, data.errorType,
data.actorRole, data.actorId, data.caseId, data.eventName, data.statusChange,
data.responseStatus), causing XSS; fix by replacing the innerHTML build with
DOM-safe construction: create table cells and child elements via
document.createElement and set their textContent (or use a proper HTML-escaping
helper) for every interpolated value, only set attributes like href using an
encoded caseId (use encodeURIComponent(data.caseId) when building the link) and
avoid inserting any raw HTML from data.* fields; ensure conditional
badges/status are appended as text nodes or elements rather than string
fragments so no unescaped input is ever assigned to innerHTML.
In `@src/main/resources/templates/fragments/header.html`:
- Around line 54-58: The onerror handler currently calls eventSource.close()
when readyState is CLOSED or CONNECTING, which prevents built-in reconnects;
update the eventSource.onerror implementation to avoid closing during
EventSource.CONNECTING — only call eventSource.close() when
eventSource.readyState === EventSource.CLOSED (or remove the close call
entirely), keeping the function name eventSource.onerror and the use of
eventSource.close and EventSource.CONNECTING/EventSource.CLOSED to locate and
modify the logic.
---
Outside diff comments:
In
`@src/main/java/org/example/projektarendehantering/application/service/AuditService.java`:
- Around line 68-99: The broadcast is executed inside the `@Transactional`
record(AuditEventEntity) method which can leak phantom events and perform
blocking I/O while the DB transaction is open; refactor so persistence happens
in record(...) but broadcasting runs after commit (and off the transaction
thread). Specifically: keep auditEventRepository.save(event) in
record(AuditEventEntity) and remove the direct call to broadcast(event); instead
publish an application event or register a TransactionSynchronization that calls
broadcast(AuditEventEntity) afterCommit (or use
`@TransactionalEventListener`(phase = AFTER_COMMIT)) so broadcast(...) and its
SseEmitter.send(...) calls run only after the transaction commits and off the DB
transaction context. Ensure broadcast(AuditEventEntity) still handles
IOException/IllegalStateException and emitter cleanup as before.
---
Nitpick comments:
In
`@src/main/java/org/example/projektarendehantering/application/service/AuditService.java`:
- Around line 118-129: The code currently removes the failed SseEmitter
(emitter) from the emitters set but still returns it to the controller; change
the error handling in the block that sends the initial "connected" event so that
on IOException you call emitter.completeWithError(e) (or rethrow the
IOException) and return early instead of returning the (now-broken) emitter;
update the method that constructs the emitter (the code surrounding emitter,
emitters, and the try/catch that sends the "connected" event in AuditService) to
ensure the failed emitter is not returned to Spring MVC and that the exception
is propagated or an explicit null/clean failure is returned.
In `@src/main/resources/templates/fragments/header.html`:
- Around line 44-84: The SSE is being opened on every page because the current
pathname check is inverted; change the guard to only create EventSource when on
the audit page (replace if (window.location.pathname === '/ui/audit') return;
with if (window.location.pathname !== '/ui/audit') return;), and add lifecycle
cleanup: after creating const eventSource = new
EventSource('/api/audit/stream'); register
document.addEventListener('visibilitychange', ...) and
window.addEventListener('pagehide', ...) handlers that call eventSource.close(),
and also ensure eventSource.onerror continues to close on terminal states;
reference the eventSource variable, the '/api/audit/stream' URL, and the
'auditLink' click handler for coordination.
🪄 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: 0f72f960-b1ba-4997-8425-b7cf935adacb
📒 Files selected for processing (5)
src/main/java/org/example/projektarendehantering/application/service/AuditService.javasrc/main/java/org/example/projektarendehantering/presentation/rest/AuditController.javasrc/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.javasrc/main/resources/templates/audit/list.htmlsrc/main/resources/templates/fragments/header.html
…xception handling with UNAUTHORIZED status
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/org/example/projektarendehantering/infrastructure/config/AuditWebMvcConfig.java (1)
22-22: Consider scoping the CSRF eager-init interceptor to the same paths as auditing.
csrfEagerInitInterceptoris registered without anyaddPathPatterns(...)/excludePathPatterns(...), so it runs for every request including/static/**,/app.css,/app.js,/error**, and/login**. The work it does is cheap, but it's inconsistent with the narrower scope applied toauditInterceptorand means the CSRF token is eagerly generated on requests that don't need it (e.g., static asset fetches).If the motivation is to ensure the token cookie is set on pages that will later make mutating API calls, scoping this to
/ui/**and/api/**(mirroring the audit interceptor) would be cleaner and avoid unnecessary work on static/error/login paths.♻️ Proposed scoping
- registry.addInterceptor(csrfEagerInitInterceptor); + registry.addInterceptor(csrfEagerInitInterceptor) + .addPathPatterns("/ui/**", "/api/**") + .excludePathPatterns( + "/static/**", + "/app.css", + "/app.js", + "/error**", + "/login**" + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/infrastructure/config/AuditWebMvcConfig.java` at line 22, The csrfEagerInitInterceptor is registered globally via registry.addInterceptor(csrfEagerInitInterceptor) causing it to run on all requests (static, error, login); scope it the same as auditInterceptor by registering with path patterns (e.g., addPathPatterns("/ui/**", "/api/**")) or the existing patterns used for auditInterceptor inside AuditWebMvcConfig so the CSRF eager-init only runs for the UI/API paths that need it and avoids static/error/login requests.src/main/java/org/example/projektarendehantering/infrastructure/config/SecurityConfig.java (1)
58-63: Consider using aRequestMatcherthat respects context path.
request.getRequestURI()includes the servlet context path, so if the application is ever deployed under a non-root context (e.g.,/app), requests to/app/api/...won't match and will fall through to the default entry point (login redirect). Using Spring'sAntPathRequestMatcher/PathPatternRequestMatcherwould handle this correctly and also be consistent with theauthorizeHttpRequestsmatchers above.♻️ Suggested refactor
- .exceptionHandling(ex -> ex - .defaultAuthenticationEntryPointFor( - new HttpStatusEntryPoint(HttpStatus.UNAUTHORIZED), - request -> request.getRequestURI().startsWith("/api/") - ) - ) + .exceptionHandling(ex -> ex + .defaultAuthenticationEntryPointFor( + new HttpStatusEntryPoint(HttpStatus.UNAUTHORIZED), + new AntPathRequestMatcher("/api/**") + ) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/infrastructure/config/SecurityConfig.java` around lines 58 - 63, The exception handling uses a lambda with request.getRequestURI() which ignores servlet context path; update SecurityConfig's exceptionHandling/defaultAuthenticationEntryPointFor to use a RequestMatcher that respects context path (e.g., new AntPathRequestMatcher("/api/**") or PathPatternRequestMatcher("/api/**")) instead of the request -> request.getRequestURI().startsWith("/api/") predicate so the HttpStatusEntryPoint(HttpStatus.UNAUTHORIZED) is applied correctly when the app is deployed under a non-root context.
🤖 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/test/java/org/example/projektarendehantering/presentation/rest/CaseControllerTest.java`:
- Around line 220-224: The test method name
getClosedCases_shouldReturnRedirect_whenNotLoggedIn is inconsistent with its
assertion; rename the method to reflect the expected Unauthorized status (e.g.,
getClosedCases_shouldReturnUnauthorized_whenNotLoggedIn) so it matches the other
updated tests in CaseControllerTest and improves readability; update the method
name only (keep the test body and annotations as-is) to the new identifier in
the CaseControllerTest class.
---
Nitpick comments:
In
`@src/main/java/org/example/projektarendehantering/infrastructure/config/AuditWebMvcConfig.java`:
- Line 22: The csrfEagerInitInterceptor is registered globally via
registry.addInterceptor(csrfEagerInitInterceptor) causing it to run on all
requests (static, error, login); scope it the same as auditInterceptor by
registering with path patterns (e.g., addPathPatterns("/ui/**", "/api/**")) or
the existing patterns used for auditInterceptor inside AuditWebMvcConfig so the
CSRF eager-init only runs for the UI/API paths that need it and avoids
static/error/login requests.
In
`@src/main/java/org/example/projektarendehantering/infrastructure/config/SecurityConfig.java`:
- Around line 58-63: The exception handling uses a lambda with
request.getRequestURI() which ignores servlet context path; update
SecurityConfig's exceptionHandling/defaultAuthenticationEntryPointFor to use a
RequestMatcher that respects context path (e.g., new
AntPathRequestMatcher("/api/**") or PathPatternRequestMatcher("/api/**"))
instead of the request -> request.getRequestURI().startsWith("/api/") predicate
so the HttpStatusEntryPoint(HttpStatus.UNAUTHORIZED) is applied correctly when
the app is deployed under a non-root context.
🪄 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: b1ba43aa-7f59-41d3-9518-627084aa4749
📒 Files selected for processing (5)
src/main/java/org/example/projektarendehantering/infrastructure/config/AuditWebMvcConfig.javasrc/main/java/org/example/projektarendehantering/infrastructure/config/SecurityConfig.javasrc/main/java/org/example/projektarendehantering/infrastructure/web/CsrfEagerInitInterceptor.javasrc/test/java/org/example/projektarendehantering/presentation/rest/AuditControllerTest.javasrc/test/java/org/example/projektarendehantering/presentation/rest/CaseControllerTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/org/example/projektarendehantering/presentation/rest/AuditControllerTest.java
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java (1)
117-138: Consider surfacing validation error details.For
MethodArgumentNotValidException, the binding result (field errors, rejected values) is discarded and replaced with a generic"Validation failed"message. API clients typically need the per-field details to render actionable feedback. Consider extractinge.getBindingResult().getFieldErrors()into the response body for REST requests.♻️ Suggested enhancement
`@ExceptionHandler`({HttpMessageNotReadableException.class, MethodArgumentNotValidException.class}) public Object handleBadRequestException(Exception e, HttpServletRequest request, HttpServletResponse response, Model model) { HttpStatus status = HttpStatus.BAD_REQUEST; String message = "Invalid request payload"; - if (e instanceof MethodArgumentNotValidException) { + Map<String, String> fieldErrors = Map.of(); + if (e instanceof MethodArgumentNotValidException manv) { message = "Validation failed"; + fieldErrors = manv.getBindingResult().getFieldErrors().stream() + .collect(java.util.stream.Collectors.toMap( + org.springframework.validation.FieldError::getField, + fe -> fe.getDefaultMessage() != null ? fe.getDefaultMessage() : "invalid", + (a, b) -> a)); } if (isRestRequest(request)) { return ResponseEntity.status(status) .body(Map.of( "errorCode", "BAD_REQUEST", "message", message, - "status", status.value() + "status", status.value(), + "errors", fieldErrors )); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java` around lines 117 - 138, The handler handleBadRequestException currently returns a generic "Validation failed" for MethodArgumentNotValidException; modify it to extract field details from ((MethodArgumentNotValidException)e).getBindingResult().getFieldErrors() and include a structured "errors" array (field, rejectedValue, defaultMessage) in the REST ResponseEntity body (when isRestRequest(request) is true), while preserving the existing errorCode/message/status; keep non-REST behavior unchanged. Use the class/field names MethodArgumentNotValidException, getBindingResult(), getFieldErrors(), isRestRequest(request), and ResponseEntity to locate and implement the 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/projektarendehantering/application/service/AuditService.java`:
- Around line 82-87: afterCommit() currently calls broadcast(event) directly
which allows exceptions (e.g., from auditEventMapper.toDTO or SSE emitter.send)
to propagate after DB commit; wrap the entire call to broadcast(event) inside a
try-catch in the TransactionSynchronization registered in
TransactionSynchronizationManager.registerSynchronization (i.e., in the
afterCommit override) and log any Throwable with context so SSE/mapper failures
do not affect the committed request response.
In `@src/main/resources/templates/audit/list.html`:
- Around line 71-168: The SSE handler
(eventSource.addEventListener('audit-event', ...)) currently prepends every
incoming row without checking the active filters; update the handler to read the
current filter inputs (e.g., from/to date inputs, caseId, and size/pageSize
controls used to populate the initial table) and apply them before inserting:
parse data.occurredAt and ignore events outside the from/to window, ignore when
data.caseId doesn't match the active caseId filter (if set), and ignore events
that don't match other active filters (status/actor/event if present). Also
enforce the current page size after inserting by checking
table.querySelectorAll('tbody tr').length and trimming excess rows from the end
so the streamed updates never make the table exceed the selected
size/pagination. Use the existing symbols (data, occurredAt, caseId, tbody, row,
and the SSE handler registered via eventSource.addEventListener('audit-event'))
to locate where to add these checks and trimming logic.
- Around line 37-54: The page currently always instantiates an EventSource to
'/api/audit/stream' (see connect(), eventSource and element
'connection-status'), causing repeating 403 reconnects for non-manager users;
change the template to include a server-side boolean (e.g., canOpenAuditStream
or isManager) and gate the client-side stream logic so connect() / new
EventSource('/api/audit/stream') only runs when that flag is true, otherwise
update 'connection-status' to show streaming is disabled/unauthorized and do not
call EventSource; ensure the server-side variable is populated from the same
permission check that protects the '/api/audit/stream' endpoint so UI and
backend permissions stay consistent.
---
Nitpick comments:
In
`@src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java`:
- Around line 117-138: The handler handleBadRequestException currently returns a
generic "Validation failed" for MethodArgumentNotValidException; modify it to
extract field details from
((MethodArgumentNotValidException)e).getBindingResult().getFieldErrors() and
include a structured "errors" array (field, rejectedValue, defaultMessage) in
the REST ResponseEntity body (when isRestRequest(request) is true), while
preserving the existing errorCode/message/status; keep non-REST behavior
unchanged. Use the class/field names MethodArgumentNotValidException,
getBindingResult(), getFieldErrors(), isRestRequest(request), and ResponseEntity
to locate and implement the change.
🪄 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: 7b752dc6-e77b-44f2-a715-6fa67d3ef125
📒 Files selected for processing (5)
src/main/java/org/example/projektarendehantering/application/service/AuditService.javasrc/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.javasrc/main/resources/templates/audit/list.htmlsrc/main/resources/templates/fragments/header.htmlsrc/test/java/org/example/projektarendehantering/presentation/rest/CaseControllerTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/org/example/projektarendehantering/presentation/rest/CaseControllerTest.java
| <div style="display:flex; justify-content: space-between; align-items:center; margin-bottom: 12px;"> | ||
| <h2 style="margin:0;">Audit Log</h2> | ||
| <div id="connection-status" class="muted" style="font-size: 0.8em;"> | ||
| <span style="color: var(--danger-color);">●</span> Real-time: Disconnected | ||
| </div> | ||
| </div> | ||
|
|
||
| <script th:inline="javascript"> | ||
| (function() { | ||
| const statusEl = document.getElementById('connection-status'); | ||
| if (!statusEl) return; | ||
|
|
||
| let eventSource = null; | ||
|
|
||
| function connect() { | ||
| if (eventSource) eventSource.close(); | ||
|
|
||
| eventSource = new EventSource('/api/audit/stream'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.java" | xargs rg -l "audit" -i | head -20Repository: ithsjava25/project-backend-code-trauma-team
Length of output: 1655
🏁 Script executed:
fd -type f -name "*Audit*.java" -o -name "*audit*.java"Repository: ithsjava25/project-backend-code-trauma-team
Length of output: 257
🏁 Script executed:
# Search for the audit controller and endpoints
rg -i "class.*audit.*controller" --type java -A 5Repository: ithsjava25/project-backend-code-trauma-team
Length of output: 2220
🏁 Script executed:
# Search for /api/audit/stream endpoint
rg "/api/audit/stream|audit/stream" --type javaRepository: ithsjava25/project-backend-code-trauma-team
Length of output: 69
🏁 Script executed:
# Search for /ui/audit endpoint
rg "/ui/audit" --type javaRepository: ithsjava25/project-backend-code-trauma-team
Length of output: 204
🏁 Script executed:
cat -n src/main/java/org/example/projektarendehantering/presentation/web/AuditUiController.javaRepository: ithsjava25/project-backend-code-trauma-team
Length of output: 2637
🏁 Script executed:
cat -n src/main/java/org/example/projektarendehantering/presentation/rest/AuditController.javaRepository: ithsjava25/project-backend-code-trauma-team
Length of output: 3070
🏁 Script executed:
cat -n src/main/java/org/example/projektarendehantering/application/service/AuditService.javaRepository: ithsjava25/project-backend-code-trauma-team
Length of output: 14931
🏁 Script executed:
# Also check the list.html template to see if currentActor is available
cat -n src/main/resources/templates/audit/list.html | head -80Repository: ithsjava25/project-backend-code-trauma-team
Length of output: 4163
🏁 Script executed:
# Search for currentActor in templates
rg "currentActor" --type htmlRepository: ithsjava25/project-backend-code-trauma-team
Length of output: 2004
🏁 Script executed:
# Check the header fragment
find . -name "header*" -type f | grep -i fragment | head -5Repository: ithsjava25/project-backend-code-trauma-team
Length of output: 139
🏁 Script executed:
# Also check if there's a configuration that adds currentActor to all models
rg "addAttribute.*currentActor|currentActor.*addAttribute" --type javaRepository: ithsjava25/project-backend-code-trauma-team
Length of output: 69
🏁 Script executed:
# Search for where currentActor is added to the model
rg "currentActor" --type java -B 2 -A 2 | head -50Repository: ithsjava25/project-backend-code-trauma-team
Length of output: 796
🏁 Script executed:
# Check if there's an interceptor or config that adds currentActor to all models
rg "HandlerInterceptor|addAttributes|Model.*currentActor" --type javaRepository: ithsjava25/project-backend-code-trauma-team
Length of output: 842
Gate the live stream UI to users allowed to open the stream.
The template unconditionally opens /api/audit/stream, which rejects non-manager users with a 403 error. Since /ui/audit has no access control while the stream endpoint is manager-only, doctors and nurses reaching the page will see repeated reconnection attempts and a "Disconnected (reconnecting...)" status state. Meanwhile, AuditService.listEvents correctly supports their filtered audit access.
Proposed fix
- <div style="display:flex; justify-content: space-between; align-items:center; margin-bottom: 12px;">
+ <div style="display:flex; justify-content: space-between; align-items:center; margin-bottom: 12px;">
<h2 style="margin:0;">Audit Log</h2>
- <div id="connection-status" class="muted" style="font-size: 0.8em;">
+ <div th:if="${currentActor != null and currentActor.role.name() == 'MANAGER'}"
+ id="connection-status" class="muted" style="font-size: 0.8em;">
<span style="color: var(--danger-color);">●</span> Real-time: Disconnected
</div>
</div>
- <script th:inline="javascript">
+ <script th:if="${currentActor != null and currentActor.role.name() == 'MANAGER'}" th:inline="javascript">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/templates/audit/list.html` around lines 37 - 54, The page
currently always instantiates an EventSource to '/api/audit/stream' (see
connect(), eventSource and element 'connection-status'), causing repeating 403
reconnects for non-manager users; change the template to include a server-side
boolean (e.g., canOpenAuditStream or isManager) and gate the client-side stream
logic so connect() / new EventSource('/api/audit/stream') only runs when that
flag is true, otherwise update 'connection-status' to show streaming is
disabled/unauthorized and do not call EventSource; ensure the server-side
variable is populated from the same permission check that protects the
'/api/audit/stream' endpoint so UI and backend permissions stay consistent.
1 participant
…n tracking, and update UI to display detailed audit event information.
Summary by CodeRabbit
New Features
Bug Fixes
UI Improvements
Tests/Chores