Adds the security layer to the Comment package, and updates tests.#39
Merged
Conversation
Comments now use the ID of the authenticated user, rather than requiring it to be set in the DTO
📝 WalkthroughWalkthroughClass-level authentication was added to CommentController; comment creation now uses the authenticated UserPrincipal (userId forwarded to CommentService). DTOs were changed: removed Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller as CommentController
participant Security as SecurityContextHolder
participant Service as CommentService
participant UserRepo as UserRepository
participant DB as Database
Client->>Controller: POST /api/comments (CreateCommentDTO{visaId, text})
Controller->>Controller: `@PreAuthorize` isAuthenticated()
Controller->>Security: retrieve Authentication -> principal
Security-->>Controller: UserPrincipal (userId)
Controller->>Service: createComment(dto, userId)
Service->>UserRepo: findById(userId)
UserRepo->>DB: query user by id
DB-->>UserRepo: User entity
UserRepo-->>Service: User (author)
Service->>DB: save Comment(visaId, author, text)
DB-->>Service: persisted Comment
Service->>Controller: CommentDTO(visaId, authorName, text, createdAt)
Controller-->>Client: 201 Created (CommentDTO)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/org/example/visacasemanagementsystem/comment/CommentServiceIntegrationTest.java (1)
47-77:⚠️ Potential issue | 🟡 MinorClear
SecurityContextHolderbetween tests to avoid state leakage.
SecurityContextHolderis backed by a thread-local strategy by default;@Transactionalrolls back DB state but does not clear the security context. Tests that run aftercreateComment_shouldSaveAndRetrieveComment/createComment_shouldThrowException_WhenUserDoesNotExistwill inherit whateverAuthenticationwas last set, which can mask bugs (e.g.createComment_shouldThrowException_WhenTextIsBlankcurrently relies on the text check short-circuiting before the principal is read — if that ordering ever changes, a leaked auth would silently keep the test green).🧪 Proposed fix
import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; @@ `@BeforeEach` void setUp() { @@ } + + `@AfterEach` + void tearDown() { + SecurityContextHolder.clearContext(); + }Also applies to: 164-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/comment/CommentServiceIntegrationTest.java` around lines 47 - 77, The tests leak authentication state because SecurityContextHolder (thread-local) isn’t cleared between tests; update CommentServiceIntegrationTest by adding an `@AfterEach` teardown (or adding a clear at the end of setUp) that calls SecurityContextHolder.clearContext() so no Authentication from prior tests (e.g., createComment_shouldSaveAndRetrieveComment, createComment_shouldThrowException_WhenUserDoesNotExist, createComment_shouldThrowException_WhenTextIsBlank) persists and masks failures; locate the setUp() method and add a teardown method (or clear) referencing SecurityContextHolder.clearContext().
🧹 Nitpick comments (2)
src/test/java/org/example/visacasemanagementsystem/comment/CommentServiceIntegrationTest.java (1)
58-65:nonExistentUserfixture is fragile.Two small issues:
nonExistentUseris never persisted, but setsusernameidentical totestUser. That's misleading and would immediately break if someone later decides to persist this fixture for another test.- Hardcoding
setId(999L)couples the test to the assumption that 999 is never generated by the DB sequence in this Spring context. Harmless today, but swapping to a distinctly out-of-range value (e.g.Long.MAX_VALUE) or deriving the id fromuserRepository.findAll().size() + 1000Lmakes the intent — "an id that definitely doesn't exist" — unambiguous.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/comment/CommentServiceIntegrationTest.java` around lines 58 - 65, The nonExistentUser fixture (nonExistentUser) is fragile because it duplicates testUser's email/username and uses a hardcoded id (setId(999L)); update the fixture to use a clearly unique email/username (e.g., suffix with "-nonexistent" or similar) and remove the brittle hardcoded id—instead assign an unmistakably out-of-range id (e.g., Long.MAX_VALUE) or compute one relative to userRepository (e.g., userRepository.findAll().size() + 1000L) to guarantee it does not exist, and ensure nonExistentUser remains unpersisted in tests that expect a missing entity.src/main/java/org/example/visacasemanagementsystem/comment/service/CommentService.java (1)
53-61: Brittle authentication resolution — add defensive null/type checks and consider controller-level resolution.The unchecked
(UserPrincipal)cast will throwClassCastExceptionif the principal is ever a different type. While integration tests correctly set upUserPrincipal, any test or caller using@WithMockUserwithout manual principal setup, or invoking the service outside HTTP context (schedulers, other services), bypasses the@PreAuthorizeguard and risks runtime failure.Additionally:
Objects.requireNonNull(...)without a message yields opaqueNullPointerExceptioninstead of a meaningful auth error- The nested chain is hard to scan
Two paths forward:
- In-place hardening (if keeping auth resolution in service): Check the principal type explicitly and throw
AuthenticationCredentialsNotFoundExceptionwith a clear message- Idiomatic approach (recommended): Resolve the principal in the controller via
@AuthenticationPrincipal UserPrincipal principaland passprincipal.getUserId()to the service, per Spring MVC best practices♻️ In-place hardening example
- Long authorId = ((UserPrincipal) Objects - .requireNonNull(Objects - .requireNonNull(SecurityContextHolder - .getContext() - .getAuthentication()) - .getPrincipal())) - .getUserId(); + Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); + if (authentication == null || !(authentication.getPrincipal() instanceof UserPrincipal principal)) { + throw new AuthenticationCredentialsNotFoundException("No authenticated user available"); + } + Long authorId = principal.getUserId();Or move resolution to the controller:
`@PostMapping` public ResponseEntity<CommentDTO> createComment(`@Valid` `@RequestBody` CreateCommentDTO dto, `@AuthenticationPrincipal` UserPrincipal principal) { return new ResponseEntity<>(commentService.createComment(dto, principal.getUserId()), HttpStatus.CREATED); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/comment/service/CommentService.java` around lines 53 - 61, The current cast of getPrincipal() to UserPrincipal in CommentService (the Long authorId = ((UserPrincipal) ...).getUserId() block) is brittle and can throw ClassCastException/opaque NPEs; either (preferred) move principal resolution into the controller and pass principal.getUserId() into CommentService.createComment(...) using `@AuthenticationPrincipal` UserPrincipal principal, or (if keeping service-side) replace the nested Objects.requireNonNull chain with explicit checks: retrieve Authentication via SecurityContextHolder.getContext().getAuthentication(), if null throw AuthenticationCredentialsNotFoundException with a clear message, check that authentication.getPrincipal() is an instance of UserPrincipal before casting and throw AuthenticationCredentialsNotFoundException (with message) if not, then extract userId and continue to findById(userRepository).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@src/test/java/org/example/visacasemanagementsystem/comment/CommentServiceIntegrationTest.java`:
- Around line 47-77: The tests leak authentication state because
SecurityContextHolder (thread-local) isn’t cleared between tests; update
CommentServiceIntegrationTest by adding an `@AfterEach` teardown (or adding a
clear at the end of setUp) that calls SecurityContextHolder.clearContext() so no
Authentication from prior tests (e.g.,
createComment_shouldSaveAndRetrieveComment,
createComment_shouldThrowException_WhenUserDoesNotExist,
createComment_shouldThrowException_WhenTextIsBlank) persists and masks failures;
locate the setUp() method and add a teardown method (or clear) referencing
SecurityContextHolder.clearContext().
---
Nitpick comments:
In
`@src/main/java/org/example/visacasemanagementsystem/comment/service/CommentService.java`:
- Around line 53-61: The current cast of getPrincipal() to UserPrincipal in
CommentService (the Long authorId = ((UserPrincipal) ...).getUserId() block) is
brittle and can throw ClassCastException/opaque NPEs; either (preferred) move
principal resolution into the controller and pass principal.getUserId() into
CommentService.createComment(...) using `@AuthenticationPrincipal` UserPrincipal
principal, or (if keeping service-side) replace the nested
Objects.requireNonNull chain with explicit checks: retrieve Authentication via
SecurityContextHolder.getContext().getAuthentication(), if null throw
AuthenticationCredentialsNotFoundException with a clear message, check that
authentication.getPrincipal() is an instance of UserPrincipal before casting and
throw AuthenticationCredentialsNotFoundException (with message) if not, then
extract userId and continue to findById(userRepository).
In
`@src/test/java/org/example/visacasemanagementsystem/comment/CommentServiceIntegrationTest.java`:
- Around line 58-65: The nonExistentUser fixture (nonExistentUser) is fragile
because it duplicates testUser's email/username and uses a hardcoded id
(setId(999L)); update the fixture to use a clearly unique email/username (e.g.,
suffix with "-nonexistent" or similar) and remove the brittle hardcoded
id—instead assign an unmistakably out-of-range id (e.g., Long.MAX_VALUE) or
compute one relative to userRepository (e.g., userRepository.findAll().size() +
1000L) to guarantee it does not exist, and ensure nonExistentUser remains
unpersisted in tests that expect a missing entity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de8425c5-d518-438f-90f0-6b02480efb15
📒 Files selected for processing (9)
src/main/java/org/example/visacasemanagementsystem/comment/controller/CommentController.javasrc/main/java/org/example/visacasemanagementsystem/comment/dto/CommentDTO.javasrc/main/java/org/example/visacasemanagementsystem/comment/dto/CreateCommentDTO.javasrc/main/java/org/example/visacasemanagementsystem/comment/mapper/CommentMapper.javasrc/main/java/org/example/visacasemanagementsystem/comment/service/CommentService.javasrc/test/java/org/example/visacasemanagementsystem/comment/CommentControllerTest.javasrc/test/java/org/example/visacasemanagementsystem/comment/CommentMapperTest.javasrc/test/java/org/example/visacasemanagementsystem/comment/CommentServiceIntegrationTest.javasrc/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java
💤 Files with no reviewable changes (4)
- src/main/java/org/example/visacasemanagementsystem/comment/mapper/CommentMapper.java
- src/main/java/org/example/visacasemanagementsystem/comment/dto/CreateCommentDTO.java
- src/main/java/org/example/visacasemanagementsystem/comment/dto/CommentDTO.java
- src/test/java/org/example/visacasemanagementsystem/comment/CommentMapperTest.java
eeebbaandersson
approved these changes
Apr 20, 2026
eeebbaandersson
left a comment
eeebbaandersson
left a comment
Contributor
There was a problem hiding this comment.
Snyggt uppdaterat här med!
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/test/java/org/example/visacasemanagementsystem/comment/CommentControllerTest.java (1)
66-85: MoveSecurityContextHoldercleanup into@AfterEach.Cleanup currently lives at the end of
createComment_shouldReturnCreatedonly, and will be skipped if an assertion fails earlier, leaking the manually-setUserPrincipalinto subsequent tests (e.g.getCommentsByVisa_ShouldReturnList, which is not covered by@WithMockUser's context cleanup for the manually-injected authentication). The integration test in this PR already uses an@AfterEachpattern — worth mirroring here for consistency and robustness.♻️ Suggested change
+ `@org.junit.jupiter.api.AfterEach` + void tearDown() { + SecurityContextHolder.clearContext(); + } + `@Test` `@WithMockUser` void createComment_shouldReturnCreated() throws Exception { ... .andExpect(jsonPath("$.authorName").value("Test User")); - - // Cleanup - SecurityContextHolder.clearContext(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/comment/CommentControllerTest.java` around lines 66 - 85, Move the manual SecurityContextHolder.clearContext() call out of the end of createComment_shouldReturnCreated and into a dedicated `@AfterEach` teardown method so the authentication set by the test's UserPrincipal is always cleared even on failures; add a method annotated with `@AfterEach` (e.g., void tearDown()) that calls SecurityContextHolder.clearContext(), ensuring tests like getCommentsByVisa_ShouldReturnList (and any tests using `@WithMockUser` or manual authentication) do not leak authentication state between runs.src/test/java/org/example/visacasemanagementsystem/comment/CommentServiceIntegrationTest.java (2)
59-67:nonExistentUsersetup: confirmsetId(Long.MAX_VALUE)won't collide and is never persisted.The fixture sets an explicit id and is intentionally not saved, which works as long as (a) no other row in the test DB has id
Long.MAX_VALUE, and (b) nothing downstream cascades a save ofnonExistentUser. Both hold today, but a safer pattern is to compute a definitely-unused id dynamically (e.g.userRepository.findAll().stream().mapToLong(User::getId).max().orElse(0L) + 1_000_000L) so the test remains robust if@GeneratedValuestrategy or data seeding changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/comment/CommentServiceIntegrationTest.java` around lines 59 - 67, The test builds a nonExistentUser by calling User#setId(Long.MAX_VALUE) which risks colliding with persisted IDs or accidental persistence; instead compute a guaranteed-unused id at runtime (e.g., query userRepository to get the current max id and add an offset) and assign that to nonExistentUser via setId, ensuring nonExistentUser remains unsaved; update the fixture setup in CommentServiceIntegrationTest to use userRepository (or a repository helper) to derive the safe id rather than using Long.MAX_VALUE and ensure no subsequent code calls save on nonExistentUser.
94-131: Authentication helpers are effectively no-ops in these service-level tests.
CommentService.createCommentreads the user from itsuserIdparameter, not fromSecurityContextHolder, soauthenticateTestUser()/authenticateNonExistentUser()don't influence behavior here — the id passed as the second argument is what drives the outcome. That's fine for now, but it can be misleading to future readers (suggesting the service somehow consults the security context).Consider either:
- dropping the
authenticate*calls from these service tests and keeping them only in controller-layer tests, or- adding a short comment noting that the context is set purely to mirror production call conditions.
Not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/comment/CommentServiceIntegrationTest.java` around lines 94 - 131, The authentication helper calls (authenticateTestUser and authenticateNonExistentUser) in these service-level tests are no-ops for CommentService.createComment because the service uses the explicit userId parameter; remove those authenticate* calls from the tests (e.g., in createComment_successful, createComment_shouldThrowException_WhenUserDoesNotExist, createComment_shouldThrowException_WhenVisaDoesNotExist) to avoid confusion, or if you prefer to keep them for parity with controller tests add a single clarifying inline comment above each test referencing that CommentService.reads user by the userId param (not SecurityContextHolder) so future readers aren’t misled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/test/java/org/example/visacasemanagementsystem/comment/CommentControllerTest.java`:
- Around line 66-85: Move the manual SecurityContextHolder.clearContext() call
out of the end of createComment_shouldReturnCreated and into a dedicated
`@AfterEach` teardown method so the authentication set by the test's UserPrincipal
is always cleared even on failures; add a method annotated with `@AfterEach`
(e.g., void tearDown()) that calls SecurityContextHolder.clearContext(),
ensuring tests like getCommentsByVisa_ShouldReturnList (and any tests using
`@WithMockUser` or manual authentication) do not leak authentication state between
runs.
In
`@src/test/java/org/example/visacasemanagementsystem/comment/CommentServiceIntegrationTest.java`:
- Around line 59-67: The test builds a nonExistentUser by calling
User#setId(Long.MAX_VALUE) which risks colliding with persisted IDs or
accidental persistence; instead compute a guaranteed-unused id at runtime (e.g.,
query userRepository to get the current max id and add an offset) and assign
that to nonExistentUser via setId, ensuring nonExistentUser remains unsaved;
update the fixture setup in CommentServiceIntegrationTest to use userRepository
(or a repository helper) to derive the safe id rather than using Long.MAX_VALUE
and ensure no subsequent code calls save on nonExistentUser.
- Around line 94-131: The authentication helper calls (authenticateTestUser and
authenticateNonExistentUser) in these service-level tests are no-ops for
CommentService.createComment because the service uses the explicit userId
parameter; remove those authenticate* calls from the tests (e.g., in
createComment_successful,
createComment_shouldThrowException_WhenUserDoesNotExist,
createComment_shouldThrowException_WhenVisaDoesNotExist) to avoid confusion, or
if you prefer to keep them for parity with controller tests add a single
clarifying inline comment above each test referencing that CommentService.reads
user by the userId param (not SecurityContextHolder) so future readers aren’t
misled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71ef68da-0e75-4a68-ae03-8f264d9d36c0
📒 Files selected for processing (5)
src/main/java/org/example/visacasemanagementsystem/comment/controller/CommentController.javasrc/main/java/org/example/visacasemanagementsystem/comment/service/CommentService.javasrc/test/java/org/example/visacasemanagementsystem/comment/CommentControllerTest.javasrc/test/java/org/example/visacasemanagementsystem/comment/CommentServiceIntegrationTest.javasrc/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java
Martin-E-Karlsson
approved these changes
Apr 21, 2026
Martin-E-Karlsson
left a comment
Martin-E-Karlsson
left a comment
Contributor
There was a problem hiding this comment.
Bra jobbat! Jag tror vi kan göra en merge med en gång!
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
3 participants
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.
Comments now use the ID of the authenticated user, rather than requiring it to be set in the DTO
Summary by CodeRabbit
Security
API Changes
Tests