Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a complete audit subsystem: JPA entity and repository, DTO and mapper, AuditService with recording/sanitization and role-filtered listing, WebMVC interceptor and config, REST/UI controllers and templates, Pom dependency for Jackson, and an integration test exercising UI-triggered audit persistence. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant SpringMVC as Spring MVC
participant AuditInterceptor as AuditInterceptor
participant AuditService as AuditService
participant Repo as AuditEventRepository
Client->>SpringMVC: HTTP request (/ui/... or /api/...)
SpringMVC->>AuditInterceptor: afterCompletion(request,response,handler,ex)
AuditInterceptor->>AuditService: record(AuditEventEntity)
AuditService->>AuditService: sanitize query / normalize timestamp / apply role filters (for list)
AuditService->>Repo: save(entity) or query(...) for listEvents
Repo-->>AuditService: persisted entity / page
AuditService-->>SpringMVC: (for list) Page<AuditEventDTO>
SpringMVC-->>Client: HTTP response (view or JSON)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
🧹 Nitpick comments (2)
src/main/java/org/example/projektarendehantering/infrastructure/persistence/AuditEventRepository.java (1)
13-27: Use deterministic ordering for paginated audit queries.Sorting only by
occurredAtcan produce unstable page boundaries when timestamps collide. Add a secondary key (e.g.,id) to keep pagination deterministic.💡 Proposed refactor
- Page<AuditEventEntity> findAllByOccurredAtBetweenOrderByOccurredAtDesc(Instant from, Instant to, Pageable pageable); + Page<AuditEventEntity> findAllByOccurredAtBetweenOrderByOccurredAtDescIdDesc(Instant from, Instant to, Pageable pageable); @@ - Page<AuditEventEntity> findAllByCaseIdInAndOccurredAtBetweenOrderByOccurredAtDesc( + Page<AuditEventEntity> findAllByCaseIdInAndOccurredAtBetweenOrderByOccurredAtDescIdDesc( Collection<UUID> caseIds, Instant from, Instant to, Pageable pageable ); @@ - Page<AuditEventEntity> findAllByCaseIdAndOccurredAtBetweenOrderByOccurredAtDesc( + Page<AuditEventEntity> findAllByCaseIdAndOccurredAtBetweenOrderByOccurredAtDescIdDesc( UUID caseId, Instant from, Instant to, Pageable pageable );🤖 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/persistence/AuditEventRepository.java` around lines 13 - 27, The repository queries currently order only by occurredAt which can yield non-deterministic pagination when timestamps are equal; update the three methods in AuditEventRepository (findAllByOccurredAtBetweenOrderByOccurredAtDesc, findAllByCaseIdInAndOccurredAtBetweenOrderByOccurredAtDesc, findAllByCaseIdAndOccurredAtBetweenOrderByOccurredAtDesc) to include a secondary tie-breaker on the entity id (e.g., rename to OrderByOccurredAtDescIdDesc or OrderByOccurredAtDescIdAsc consistently) so pagination becomes deterministic; ensure all three signatures use the same id ordering direction.src/main/java/org/example/projektarendehantering/infrastructure/persistence/AuditEventEntity.java (1)
35-37: Add explicit column mappings for audit String fields when audit recording is integrated.While
requestPath,queryString, anduserAgentfields in this entity currently lack explicit@Columnannotations (defaulting to VARCHAR(255)), note thatAuditServiceis not currently integrated into any controller or business logic—no code invokesauditService.record(). When audit recording is actually wired up, add explicit column definitions to prevent truncation of long query strings and user agents. Use@Column(columnDefinition = "text")for variable-length fields and@Column(length = 45)forclientIp(IPv6 max).Proposed fix (apply when audit is integrated)
+import jakarta.persistence.Column; import jakarta.persistence.Entity; import jakarta.persistence.GeneratedValue; import jakarta.persistence.GenerationType; import jakarta.persistence.Id; import jakarta.persistence.Index; import jakarta.persistence.Table; @@ - private String httpMethod; - private String requestPath; - private String queryString; - private String handler; + `@Column`(length = 16) + private String httpMethod; + `@Column`(columnDefinition = "text") + private String requestPath; + `@Column`(columnDefinition = "text") + private String queryString; + `@Column`(columnDefinition = "text") + private String handler; @@ - private String clientIp; - private String userAgent; + `@Column`(length = 45) + private String clientIp; + `@Column`(columnDefinition = "text") + private String userAgent;Also applies to: 44-45
🤖 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/persistence/AuditEventEntity.java` around lines 35 - 37, The AuditEventEntity currently relies on default column sizes which may truncate long values; update the entity by adding explicit `@Column` mappings: annotate requestPath, queryString, and userAgent with `@Column`(columnDefinition = "text") to allow variable-length content, and annotate clientIp with `@Column`(length = 45) to safely store IPv6 addresses; modify the AuditEventEntity class fields requestPath, queryString, userAgent, and clientIp accordingly so the schema supports long query strings and user agents when AuditService.record() is used.
🤖 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 47-48: After computing safeFrom and safeTo in AuditService (use
the same logic: Instant safeFrom = from != null ? from : Instant.EPOCH; Instant
safeTo = to != null ? to : Instant.now();), validate the time-window order by
adding a check like if (safeFrom.isAfter(safeTo)) throw new
IllegalArgumentException("Invalid time range: 'from' must be <= 'to'"); so the
method fails fast when a caller supplies an inverted range instead of silently
returning empty results.
- Around line 36-42: AuditEventEntity currently persists raw payloads in
AuditService.record; before calling auditEventRepository.save(event) implement
payload sanitization: add a private sanitizeAuditPayload method in AuditService
that detects and redacts sensitive keys (e.g.,
"password","token","authorization","apiKey","access_token","secret","ssn","creditCard",
etc.) from the event payload (handle both Map/structured payloads and
JSON/string query strings by parsing, replacing values with "[REDACTED]" and
preserving structure), then set the sanitized payload back on the
AuditEventEntity (use event.getPayload()/setPayload(...) or appropriate
accessors) and only then call auditEventRepository.save(event); keep null checks
(event != null, occurredAt) intact.
- Around line 61-67: The current logic in AuditService returns
Page.empty(pageable) when allowedCaseIds.isEmpty() even if a caller passed an
explicit caseId, bypassing authorization; reorder the checks so that if (caseId
!= null) { if (!allowedCaseIds.contains(caseId)) throw new
NotAuthorizedException(...) } is executed before the allowedCaseIds.isEmpty()
check, ensuring the explicit caseId is rejected with NotAuthorizedException
rather than returning an empty Page; keep references to allowedCaseIds, caseId,
NotAuthorizedException and Page.empty(pageable) when making the change.
---
Nitpick comments:
In
`@src/main/java/org/example/projektarendehantering/infrastructure/persistence/AuditEventEntity.java`:
- Around line 35-37: The AuditEventEntity currently relies on default column
sizes which may truncate long values; update the entity by adding explicit
`@Column` mappings: annotate requestPath, queryString, and userAgent with
`@Column`(columnDefinition = "text") to allow variable-length content, and
annotate clientIp with `@Column`(length = 45) to safely store IPv6 addresses;
modify the AuditEventEntity class fields requestPath, queryString, userAgent,
and clientIp accordingly so the schema supports long query strings and user
agents when AuditService.record() is used.
In
`@src/main/java/org/example/projektarendehantering/infrastructure/persistence/AuditEventRepository.java`:
- Around line 13-27: The repository queries currently order only by occurredAt
which can yield non-deterministic pagination when timestamps are equal; update
the three methods in AuditEventRepository
(findAllByOccurredAtBetweenOrderByOccurredAtDesc,
findAllByCaseIdInAndOccurredAtBetweenOrderByOccurredAtDesc,
findAllByCaseIdAndOccurredAtBetweenOrderByOccurredAtDesc) to include a secondary
tie-breaker on the entity id (e.g., rename to OrderByOccurredAtDescIdDesc or
OrderByOccurredAtDescIdAsc consistently) so pagination becomes deterministic;
ensure all three signatures use the same id ordering direction.
🪄 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: b57b16d4-71d5-40cc-9681-18eb9ffe2807
📒 Files selected for processing (5)
src/main/java/org/example/projektarendehantering/application/service/AuditEventMapper.javasrc/main/java/org/example/projektarendehantering/application/service/AuditService.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/AuditEventEntity.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/AuditEventRepository.javasrc/main/java/org/example/projektarendehantering/presentation/dto/AuditEventDTO.java
src/main/java/org/example/projektarendehantering/application/service/AuditService.java
Show resolved
Hide resolved
src/main/java/org/example/projektarendehantering/application/service/AuditService.java
Show resolved
Hide resolved
src/main/java/org/example/projektarendehantering/application/service/AuditService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
pom.xml (1)
71-75: Redundant test dependency.
spring-boot-test-autoconfigureis already included transitively byspring-boot-starter-test. This explicit declaration is unnecessary.🧹 Proposed fix
- <dependency> - <groupId>org.springframework.boot</groupId> - <artifactId>spring-boot-test-autoconfigure</artifactId> - <scope>test</scope> - </dependency>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` around lines 71 - 75, Remove the redundant explicit dependency declaration for artifactId "spring-boot-test-autoconfigure" from the pom.xml because it is provided transitively by "spring-boot-starter-test"; locate the <dependency> block containing groupId org.springframework.boot and artifactId spring-boot-test-autoconfigure and delete that entire dependency element so the project relies on the existing spring-boot-starter-test transitive inclusion.src/main/java/org/example/projektarendehantering/presentation/web/AuditUiController.java (1)
39-43: Consider extracting shared pagination logic.The pagination clamping logic (
Math.max(page, 0),Math.min(Math.max(size, 1), 200)) is duplicated in bothAuditControllerandAuditUiController. This could be extracted to a shared utility method.🤖 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/AuditUiController.java` around lines 39 - 43, Extract the duplicated pagination clamping into a shared utility method and replace the inline logic in both AuditUiController and AuditController with a call to it; specifically, create a helper (e.g., PaginationUtils.createPageRequest or PaginationHelper.getPageable) that accepts page, size, and Sort and inside performs Math.max(page, 0) and Math.min(Math.max(size, 1), 200) before returning PageRequest.of(...), then update the Pageable construction in AuditUiController (the PageRequest.of(...) usage) and the analogous code in AuditController to use this new helper.src/main/java/org/example/projektarendehantering/infrastructure/web/AuditInterceptor.java (1)
90-97: Consider X-Forwarded-For spoofing risk.The
X-Forwarded-Forheader can be spoofed by clients. If accurate client IP tracking is important for security audit purposes, ensure your infrastructure (load balancer/reverse proxy) overwrites or sanitizes this header before it reaches the application.🤖 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 90 - 97, The clientIp method trusts the X-Forwarded-For header which can be spoofed; change it to only use X-Forwarded-For when the immediate peer is a trusted proxy. Modify AuditInterceptor.clientIp to consult a configurable set/list of trusted proxy IPs (e.g., trustedProxies) and only parse X-Forwarded-For if request.getRemoteAddr() is in that set; otherwise always return request.getRemoteAddr(). Make the trustedProxies configurable (constructor/config property) and ensure trimming/parsing of the first entry in the header remains the same when allowed.src/main/java/org/example/projektarendehantering/application/service/AuditService.java (1)
38-38: Consider injectingObjectMapperinstead of creating inline.Creating a new
ObjectMapperinstance as a field prevents configuration sharing (e.g., custom serializers, modules) and makes testing harder. Spring Boot auto-configures anObjectMapperbean that can be injected.💡 Proposed change
- private final ObjectMapper objectMapper = new ObjectMapper(); + private final ObjectMapper objectMapper; - public AuditService(AuditEventRepository auditEventRepository, AuditEventMapper auditEventMapper, CaseRepository caseRepository) { + public AuditService(AuditEventRepository auditEventRepository, AuditEventMapper auditEventMapper, CaseRepository caseRepository, ObjectMapper objectMapper) { this.auditEventRepository = auditEventRepository; this.auditEventMapper = auditEventMapper; this.caseRepository = caseRepository; + this.objectMapper = objectMapper; }🤖 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` at line 38, The AuditService currently instantiates a new ObjectMapper inline (private final ObjectMapper objectMapper = new ObjectMapper();) which prevents shared configuration and makes testing harder; change AuditService to accept an ObjectMapper as a dependency (constructor injection or `@Autowired`) and remove the inline new ObjectMapper() so the class uses the Spring-configured ObjectMapper bean; update any tests to provide a mocked or test-configured ObjectMapper via the constructor or test configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pom.xml`:
- Around line 115-118: The POM contains a duplicate dependency declaration for
com.fasterxml.jackson.core:jackson-databind; remove the redundant <dependency>
block that declares groupId com.fasterxml.jackson.core and artifactId
jackson-databind (the second occurrence) so it only appears once (or remove both
and rely on the transitive jackson-databind from spring-boot-starter-web if you
prefer), ensuring the final pom only has a single jackson-databind declaration
or none if relying on the starter.
In
`@src/main/java/org/example/projektarendehantering/application/service/AuditService.java`:
- Around line 91-117: Remove the unused overload sanitizeAuditPayload(Object):
delete the private Object sanitizeAuditPayload(Object) method and ensure callers
use the existing sanitizeAuditPayload(String) and other type-specific logic;
update any recursive sanitization to be handled within the String overload
(sanitizeAuditPayload(String)) or by converting/dispatching callers to the
appropriate overload so there are no orphaned recursive calls like the ones that
referenced sanitizeAuditPayload(Object).
In
`@src/test/java/org/example/projektarendehantering/ProjektArendehanteringApplicationTests.java`:
- Around line 29-40: The MockMvc in uiRequest_createsAuditEvent is built without
the Spring Security filter chain so `@WithMockUser` isn't applied; update the
MockMvc construction (the webAppContextSetup(...) call that builds MockMvc) to
apply Spring Security before build — e.g., use
MockMvcBuilders.webAppContextSetup(webApplicationContext).apply(springSecurity()).build()
or enable filters via AutoConfigureMockMvc so that security filters are included
and the `@WithMockUser` security context is propagated during the request.
---
Nitpick comments:
In `@pom.xml`:
- Around line 71-75: Remove the redundant explicit dependency declaration for
artifactId "spring-boot-test-autoconfigure" from the pom.xml because it is
provided transitively by "spring-boot-starter-test"; locate the <dependency>
block containing groupId org.springframework.boot and artifactId
spring-boot-test-autoconfigure and delete that entire dependency element so the
project relies on the existing spring-boot-starter-test transitive inclusion.
In
`@src/main/java/org/example/projektarendehantering/application/service/AuditService.java`:
- Line 38: The AuditService currently instantiates a new ObjectMapper inline
(private final ObjectMapper objectMapper = new ObjectMapper();) which prevents
shared configuration and makes testing harder; change AuditService to accept an
ObjectMapper as a dependency (constructor injection or `@Autowired`) and remove
the inline new ObjectMapper() so the class uses the Spring-configured
ObjectMapper bean; update any tests to provide a mocked or test-configured
ObjectMapper via the constructor or test configuration.
In
`@src/main/java/org/example/projektarendehantering/infrastructure/web/AuditInterceptor.java`:
- Around line 90-97: The clientIp method trusts the X-Forwarded-For header which
can be spoofed; change it to only use X-Forwarded-For when the immediate peer is
a trusted proxy. Modify AuditInterceptor.clientIp to consult a configurable
set/list of trusted proxy IPs (e.g., trustedProxies) and only parse
X-Forwarded-For if request.getRemoteAddr() is in that set; otherwise always
return request.getRemoteAddr(). Make the trustedProxies configurable
(constructor/config property) and ensure trimming/parsing of the first entry in
the header remains the same when allowed.
In
`@src/main/java/org/example/projektarendehantering/presentation/web/AuditUiController.java`:
- Around line 39-43: Extract the duplicated pagination clamping into a shared
utility method and replace the inline logic in both AuditUiController and
AuditController with a call to it; specifically, create a helper (e.g.,
PaginationUtils.createPageRequest or PaginationHelper.getPageable) that accepts
page, size, and Sort and inside performs Math.max(page, 0) and
Math.min(Math.max(size, 1), 200) before returning PageRequest.of(...), then
update the Pageable construction in AuditUiController (the PageRequest.of(...)
usage) and the analogous code in AuditController to use this new helper.
🪄 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: 7dc96df7-b5c0-42f7-8cd4-24ed1347258f
📒 Files selected for processing (9)
pom.xmlsrc/main/java/org/example/projektarendehantering/application/service/AuditService.javasrc/main/java/org/example/projektarendehantering/infrastructure/config/AuditWebMvcConfig.javasrc/main/java/org/example/projektarendehantering/infrastructure/web/AuditInterceptor.javasrc/main/java/org/example/projektarendehantering/presentation/rest/AuditController.javasrc/main/java/org/example/projektarendehantering/presentation/web/AuditUiController.javasrc/main/resources/templates/audit/list.htmlsrc/main/resources/templates/fragments/header.htmlsrc/test/java/org/example/projektarendehantering/ProjektArendehanteringApplicationTests.java
✅ Files skipped from review due to trivial changes (1)
- src/main/resources/templates/fragments/header.html
Comment on lines
+91
to
+117
| @SuppressWarnings("unchecked") | ||
| private Object sanitizeAuditPayload(Object payload) { | ||
| if (payload == null) return null; | ||
| if (payload instanceof String s) return sanitizeAuditPayload(s); | ||
|
|
||
| if (payload instanceof Map<?, ?> map) { | ||
| Map<String, Object> out = new LinkedHashMap<>(); | ||
| for (Map.Entry<?, ?> e : map.entrySet()) { | ||
| String key = e.getKey() == null ? null : String.valueOf(e.getKey()); | ||
| Object value = e.getValue(); | ||
| if (key != null && isSensitiveKey(key)) { | ||
| out.put(key, REDACTED); | ||
| } else { | ||
| out.put(key, sanitizeAuditPayload(value)); | ||
| } | ||
| } | ||
| return out; | ||
| } | ||
|
|
||
| if (payload instanceof Collection<?> col) { | ||
| List<Object> out = new ArrayList<>(col.size()); | ||
| for (Object v : col) out.add(sanitizeAuditPayload(v)); | ||
| return out; | ||
| } | ||
|
|
||
| return payload; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that sanitizeAuditPayload(Object) is never called
rg -n 'sanitizeAuditPayload\s*\(' --type java | grep -v 'private.*sanitizeAuditPayload'Repository: ithsjava25/project-backend-code-trauma-team
Length of output: 727
🏁 Script executed:
#!/bin/bash
# Find all sanitizeAuditPayload method signatures/definitions
rg -n 'private.*sanitizeAuditPayload' --type java -A 1Repository: ithsjava25/project-backend-code-trauma-team
Length of output: 673
Remove unused method overload: sanitizeAuditPayload(Object).
The Object overload is never called. The only external call site (line 71) passes a String, which routes to the String overload (line 75). The Object overload's recursive calls (lines 104, 112) are only reachable if the Object overload itself were initially invoked, which never happens in practice. Remove this dead code.
🤖 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 91 - 117, Remove the unused overload sanitizeAuditPayload(Object):
delete the private Object sanitizeAuditPayload(Object) method and ensure callers
use the existing sanitizeAuditPayload(String) and other type-specific logic;
update any recursive sanitization to be handled within the String overload
(sanitizeAuditPayload(String)) or by converting/dispatching callers to the
appropriate overload so there are no orphaned recursive calls like the ones that
referenced sanitizeAuditPayload(Object).
src/test/java/org/example/projektarendehantering/ProjektArendehanteringApplicationTests.java
Show resolved
Hide resolved
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.
Closes #9
Summary by CodeRabbit
New Features
Tests