Test: integration tests for vet#195
Conversation
- Implements generating JWT tokens with claims for user authentication. - Adds decoding and validation of tokens with HS256 algorithm. - Exposes JwtDecoder as a bean for Spring Security integration.
- Implements a filter to authenticate HTTP requests using JWT tokens. - Extracts and validates tokens from the Authorization header. - Loads user details and sets authentication in SecurityContext.
- Added SecurityFilterChain to define security rules, including JWT-based authentication and CORS configuration. - Introduced AuthenticationManager, JwtDecoder, and DaoAuthenticationProvider beans. - Enabled stateless session handling for RESTful API requests.
…gration - Provides a bridge between Spring Security and the database. - Loads user details by email for JWT-based authentication. - Logs user lookup actions and handles not found scenarios.
- Provides a bridge between Spring Security and the database. - Loads user details by email for JWT-based authentication. - Logs user lookup actions and handles not found scenarios.
…ordControllerTest
…chmentControllerTest - Integrated `UsernamePasswordAuthenticationToken` for authentication in `ActivityLogIntegrationTest`. - Added `JwtService` and `CustomUserDetailsService` mocks in `AttachmentControllerTest`.
# Conflicts: # src/test/java/org/example/vet1177/controller/UserControllerTest.java
- Introduced `AuthRequest` record with email and password fields. - Added validation annotations to ensure required fields are provided.
- Created `RegisterRequest` record with fields for name, email, password, and role. - Added validation annotations to ensure all required fields are provided with appropriate constraints.
- Introduced `AuthResponse` record with fields for token, userId, name, email, and role.
…dation and password protection - Added `@Email` annotation in `AuthRequest` for email validation. - Introduced `@JsonProperty(access = JsonProperty.Access.WRITE_ONLY)` for password fields to ensure write-only access. - Overrode `toString()` to mask sensitive password data in logs.
Feat/security auth and register
annikaholmqvist94
left a comment
There was a problem hiding this comment.
Bra jobb bara kaninens grejer så MERGEEEE :)
test: add unit tests for UserController
feat: Add admin-only endpoint to search user by email
… test/integration_tests_vet # Conflicts: # src/test/java/org/example/vet1177/integration/vet/VetIntegrationTest.java
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (8)
src/main/java/org/example/vet1177/services/UserService.java (1)
74-80: Consolidate duplicate email lookup logic.
searchByEmailrepeats query/exception logic already implemented ingetByEmail, which increases maintenance drift risk.♻️ Suggested refactor
public UserResponse searchByEmail(String email) { - log.debug("Searching user by email={}", email); - User user = userRepository.findByEmail(email) - .orElseThrow(() -> new ResourceNotFoundException("User", email)); - return mapToResponse(user); + log.debug("Searching user by email={}", email); + return mapToResponse(getByEmail(email)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/services/UserService.java` around lines 74 - 80, The searchByEmail method duplicates the find-and-throw logic already implemented in getByEmail; update searchByEmail to call getByEmail(email) and then return mapToResponse(...) instead of re-querying userRepository and rethrowing ResourceNotFoundException, removing the duplicated repository lookup and exception construction (keep mapToResponse, getByEmail, and ResourceNotFoundException references as the single source of truth).src/test/java/org/example/vet1177/controller/VetControllerTest.java (1)
44-48: These mocks still don't cover the authenticated-principal path.With filters disabled and no
Authenticationattached to the request,@AuthenticationPrincipalresolves tonullhere. The new mocks make the slice boot, but they won't catch regressions in the controller's security wiring.Either import the security config and use
.with(authentication(...))like the other controller tests in this PR, or exclude the JWT filter entirely if security is intentionally out of scope for this slice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/vet1177/controller/VetControllerTest.java` around lines 44 - 48, The test currently leaves `@AuthenticationPrincipal` null because filters are disabled and no Authentication is attached; update VetControllerTest to exercise the authenticated-principal path by either (A) importing your security config and mocking/creating an Authentication object and attaching it to the mock request using .with(authentication(yourAuth)) so controller methods that read `@AuthenticationPrincipal` see a non-null principal, or (B) explicitly exclude the JWT filter from the test slice if you intend to test controller behavior without security; adjust the test setup where JwtService and CustomUserDetailsService are declared to follow the chosen approach so regressions in security wiring are caught.src/test/java/org/example/vet1177/controller/PetControllerTest.java (2)
97-104: Remove obsoletecurrentUserIdheader.The
PetControllernow uses@AuthenticationPrincipal User currentUserinstead of acurrentUserIdheader. The header is ignored since authentication is handled viaauthenticatedAs(). Remove the header to avoid confusion.♻️ Proposed fix for this test method
mockMvc.perform(post("/pets") .with(authenticatedAs(owner)) - .header("currentUserId", ownerId) .contentType(MediaType.APPLICATION_JSON) .content(objectMapper.writeValueAsString(validPetRequest()))) .andExpect(status().isOk()) .andExpect(jsonPath("$.name").value("Molly"));This same change should be applied to all other test methods that include
.header("currentUserId", ...):
- Lines 113, 126, 139 (createPet tests)
- Lines 153, 166, 178 (getPetById tests)
- Lines 189, 201 (getPetsByOwner tests)
- Lines 214, 228, 241 (updatePet tests)
- Lines 257, 269, 281 (deletePet tests)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/vet1177/controller/PetControllerTest.java` around lines 97 - 104, In PetControllerTest, remove the obsolete .header("currentUserId", ownerId) from all mockMvc.perform(...) calls that already use .with(authenticatedAs(owner)) (e.g., the createPet/getPetById/getPetsByOwner/updatePet/deletePet test methods) so the tests rely only on the AuthenticationPrincipal provided by authenticatedAs(owner); update each mockMvc.perform call in those test methods to stop setting the currentUserId header.
3-3: Remove commented-out debug note.This comment appears to be a personal development note and should be removed before merging.
🧹 Suggested cleanup
-//Används i commentControllerTest också, verkar funka där?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/vet1177/controller/PetControllerTest.java` at line 3, Remove the commented-out debug note present at the top of the PetControllerTest class (the line starting with "//Används i commentControllerTest också, verkar funka där?") so the test class contains no personal development/debug comments; search for that exact comment in the PetControllerTest class and delete it (and any similar leftover dev comments) to keep the test file clean.API.md (1)
585-585: Minor: Swedish abbreviation formatting.Per Swedish language conventions, abbreviations should use a colon when inflected: "URL:er" instead of "URLer".
📝 Suggested fix
-**Allt annat ar likadant** — samma URLer, samma request bodies, samma svar. Bara headern andras. +**Allt annat är likadant** — samma URL:er, samma request bodies, samma svar. Bara headern ändras.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@API.md` at line 585, Replace the Swedish abbreviation "URLer" with the correctly inflected form "URL:er" in the API.md text snippet (the sentence starting "**Allt annat ar likadant** — samma URLer, samma request bodies, samma svar. Bara headern andras."); update that exact token to "URL:er" to comply with Swedish abbreviation formatting conventions.src/test/java/org/example/vet1177/services/UserServiceTest.java (2)
466-470: Consider using a test builder or factory instead of reflection.While reflection works, it couples tests to the internal structure of entities. Consider adding a package-private constructor or using a test builder pattern for better maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/vet1177/services/UserServiceTest.java` around lines 466 - 470, The test currently uses the private helper setPrivateField in UserServiceTest to inject state via reflection; replace this by adding a package-private constructor or a test builder/factory for the entity under test (e.g., User) and update tests to instantiate entities with that constructor/builder instead of calling setPrivateField; remove or stop using the setPrivateField helper once tests are refactored. Locate the setPrivateField method in UserServiceTest and the entity class (e.g., User) to add a package-private constructor or create a TestUserBuilder in the same package so tests can construct instances without reflection.
1-491: Missing test coverage forsearchByEmailmethod.According to the AI summary,
UserService.searchByEmail(String)was added and is used by the newGET /api/users/searchendpoint. Consider adding test coverage for this method.Do you want me to generate test cases for the
searchByEmailmethod?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/vet1177/services/UserServiceTest.java` around lines 1 - 491, Add unit tests for UserService.searchByEmail(String): create a test that mocks the repository call used by searchByEmail (e.g., userRepository.findByEmailContainingIgnoreCase or whatever repository method UserService.searchByEmail delegates to) to return matching users (use ownerUser and vetUser), call userService.searchByEmail("query"), and assert the returned list contains expected UserResponse entries and correct roles/ids; also add a test where the repository returns an empty list and assert an empty result is returned. Use existing fixtures (ownerUser, vetUser, userRepository, userService) and verify repository interaction and mapping behavior.src/test/java/org/example/vet1177/integration/vet/VetIntegrationTest.java (1)
69-79: Optional cleanup: merge duplicated lifecycle setup.You can combine the two
@BeforeEachmethods (Line [69] and Line [76]) into one for a single, explicit test setup flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/vet1177/integration/vet/VetIntegrationTest.java` around lines 69 - 79, Merge the duplicated `@BeforeEach` lifecycle methods by combining the cleanup and mock reset into a single setup method: move the reset(adminPolicy) call into the existing setUp() method (which already calls vetRepository.deleteAll(), userRepository.deleteAll(), clinicRepository.deleteAll()) and remove the separate resetMocks() method, keeping a single `@BeforeEach-annotated` method (e.g., setUp) to perform both repository cleanup and adminPolicy reset.
🤖 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/vet1177/security/auth/dto/AuthRequest.java`:
- Around line 12-15: AuthRequest.toString currently returns the raw email which
can leak identifiers; update AuthRequest's toString method to avoid printing the
full email (e.g., replace email with a masked value like "REDACTED" or a masked
pattern that preserves no PII or only minimal non-identifying portions) and keep
the password redacted as already done; modify the AuthRequest.toString
implementation to use the new maskedEmail or constant placeholder instead of the
plain email so logs no longer contain the full address.
In `@src/main/java/org/example/vet1177/security/auth/dto/RegisterRequest.java`:
- Around line 13-16: The RegisterRequest.toString() currently exposes PII by
including name and email; update the RegisterRequest.toString() method to avoid
printing personal data and instead return a non-identifying representation
(e.g., class name and masked/omitted fields), keeping password masked as before;
ensure the change is made in the RegisterRequest.toString() implementation so
logs no longer contain name or email.
In `@src/main/java/org/example/vet1177/security/CustomUserDetailsService.java`:
- Around line 46-47: The code in CustomUserDetailsService leaks the raw email in
both the log.warn call and the UsernameNotFoundException; update the
loadUserByUsername path to avoid exposing the identifier by removing or masking
the email from logs (e.g., log a generic "user not found" or a hashed/masked
value) and throw a generic UsernameNotFoundException message without the email;
change the log.warn("User not found email={}", email) and the new
UsernameNotFoundException("Användare hittades inte: " + email) usages
accordingly so only non-identifying, generic text is recorded and returned.
In `@src/main/java/org/example/vet1177/security/JwtAuthenticationFilter.java`:
- Around line 111-122: JwtAuthenticationFilter currently logs raw email and
exception messages containing the email (log.debug("Authenticated user email={}
via JWT", email) and log.warn("User from JWT no longer exists: {}",
e.getMessage())), which exposes PII; update JwtAuthenticationFilter to stop
logging raw email and instead log non-identifying info (e.g., "Authenticated
user via JWT" or a masked identifier) and change the UsernameNotFoundException
handling to avoid logging e.getMessage() if it contains the email. Also update
CustomUserDetailsService.loadUserByUsername so the UsernameNotFoundException it
throws does not include the full email (use a generic message or include a
non-PII id), and prefer logging a masked email only when strictly necessary.
Ensure references: JwtAuthenticationFilter, loadUserByUsername, and
UsernameNotFoundException are adjusted consistently.
In `@src/main/java/org/example/vet1177/security/JwtService.java`:
- Around line 57-58: Validate the JWT expiration config in the JwtService
constructor by checking the value returned from jwtProperties.getExpirationMs()
(assigned to expirationMs) is a positive number; if not, throw an
IllegalArgumentException (or similar) with a clear message so the application
fails fast on misconfiguration. Locate the JwtService constructor where
this.expirationMs = jwtProperties.getExpirationMs() is set and add the guard
that validates expirationMs > 0 and throws a descriptive exception referencing
JwtService and expirationMs.
In `@src/main/java/org/example/vet1177/security/SecurityConfig.java`:
- Around line 141-149: corsConfigurationSource() currently registers CORS only
for "/api/**", so requests to PetController (mapped at "/pets") won't receive
CORS headers; update the UrlBasedCorsConfigurationSource registration in
corsConfigurationSource() to also register the pet routes (for example
registerCorsConfiguration("/pets/**", config) or broaden to
registerCorsConfiguration("/**", config)) so that PetController endpoints
receive the configured CORS settings; locate the method
corsConfigurationSource() and the UrlBasedCorsConfigurationSource usage to apply
the change.
In `@src/test/java/org/example/vet1177/controller/PetControllerTest.java`:
- Line 7: The test import is wrong: replace the non-existent
tools.jackson.databind.ObjectMapper import in PetControllerTest (same issue as
UserControllerTest) with the standard Jackson class
com.fasterxml.jackson.databind.ObjectMapper so the test can compile and use
Jackson's ObjectMapper.
In `@src/test/java/org/example/vet1177/controller/UserControllerTest.java`:
- Line 6: Replace the incorrect import in UserControllerTest (the line importing
tools.jackson.databind.ObjectMapper) with the correct Jackson package; update
the import to com.fasterxml.jackson.databind.ObjectMapper so the ObjectMapper
reference in the test class resolves and the file compiles.
In `@src/test/java/org/example/vet1177/entities/UserEntityTest.java`:
- Around line 69-77: The test currently allows a no-op because
assertThat(user.getUpdatedAt()).isNotNull().isAfterOrEqualTo(beforeUpdate)
passes when updatedAt is unchanged; update the assertion in
onUpdate_shouldRefreshUpdatedAt() to require a strictly later timestamp (e.g.,
replace isAfterOrEqualTo(beforeUpdate) with isAfter(beforeUpdate) or
assertThat(...).isNotEqualTo(beforeUpdate).isAfter(beforeUpdate)) so that
onUpdate() must actually refresh the value returned by getUpdatedAt() after
calling onCreate() and then onUpdate().
---
Nitpick comments:
In `@API.md`:
- Line 585: Replace the Swedish abbreviation "URLer" with the correctly
inflected form "URL:er" in the API.md text snippet (the sentence starting
"**Allt annat ar likadant** — samma URLer, samma request bodies, samma svar.
Bara headern andras."); update that exact token to "URL:er" to comply with
Swedish abbreviation formatting conventions.
In `@src/main/java/org/example/vet1177/services/UserService.java`:
- Around line 74-80: The searchByEmail method duplicates the find-and-throw
logic already implemented in getByEmail; update searchByEmail to call
getByEmail(email) and then return mapToResponse(...) instead of re-querying
userRepository and rethrowing ResourceNotFoundException, removing the duplicated
repository lookup and exception construction (keep mapToResponse, getByEmail,
and ResourceNotFoundException references as the single source of truth).
In `@src/test/java/org/example/vet1177/controller/PetControllerTest.java`:
- Around line 97-104: In PetControllerTest, remove the obsolete
.header("currentUserId", ownerId) from all mockMvc.perform(...) calls that
already use .with(authenticatedAs(owner)) (e.g., the
createPet/getPetById/getPetsByOwner/updatePet/deletePet test methods) so the
tests rely only on the AuthenticationPrincipal provided by
authenticatedAs(owner); update each mockMvc.perform call in those test methods
to stop setting the currentUserId header.
- Line 3: Remove the commented-out debug note present at the top of the
PetControllerTest class (the line starting with "//Används i
commentControllerTest också, verkar funka där?") so the test class contains no
personal development/debug comments; search for that exact comment in the
PetControllerTest class and delete it (and any similar leftover dev comments) to
keep the test file clean.
In `@src/test/java/org/example/vet1177/controller/VetControllerTest.java`:
- Around line 44-48: The test currently leaves `@AuthenticationPrincipal` null
because filters are disabled and no Authentication is attached; update
VetControllerTest to exercise the authenticated-principal path by either (A)
importing your security config and mocking/creating an Authentication object and
attaching it to the mock request using .with(authentication(yourAuth)) so
controller methods that read `@AuthenticationPrincipal` see a non-null principal,
or (B) explicitly exclude the JWT filter from the test slice if you intend to
test controller behavior without security; adjust the test setup where
JwtService and CustomUserDetailsService are declared to follow the chosen
approach so regressions in security wiring are caught.
In `@src/test/java/org/example/vet1177/integration/vet/VetIntegrationTest.java`:
- Around line 69-79: Merge the duplicated `@BeforeEach` lifecycle methods by
combining the cleanup and mock reset into a single setup method: move the
reset(adminPolicy) call into the existing setUp() method (which already calls
vetRepository.deleteAll(), userRepository.deleteAll(),
clinicRepository.deleteAll()) and remove the separate resetMocks() method,
keeping a single `@BeforeEach-annotated` method (e.g., setUp) to perform both
repository cleanup and adminPolicy reset.
In `@src/test/java/org/example/vet1177/services/UserServiceTest.java`:
- Around line 466-470: The test currently uses the private helper
setPrivateField in UserServiceTest to inject state via reflection; replace this
by adding a package-private constructor or a test builder/factory for the entity
under test (e.g., User) and update tests to instantiate entities with that
constructor/builder instead of calling setPrivateField; remove or stop using the
setPrivateField helper once tests are refactored. Locate the setPrivateField
method in UserServiceTest and the entity class (e.g., User) to add a
package-private constructor or create a TestUserBuilder in the same package so
tests can construct instances without reflection.
- Around line 1-491: Add unit tests for UserService.searchByEmail(String):
create a test that mocks the repository call used by searchByEmail (e.g.,
userRepository.findByEmailContainingIgnoreCase or whatever repository method
UserService.searchByEmail delegates to) to return matching users (use ownerUser
and vetUser), call userService.searchByEmail("query"), and assert the returned
list contains expected UserResponse entries and correct roles/ids; also add a
test where the repository returns an empty list and assert an empty result is
returned. Use existing fixtures (ownerUser, vetUser, userRepository,
userService) and verify repository interaction and mapping behavior.
🪄 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: ee588f02-cbad-41b4-9489-bec25a350c75
📒 Files selected for processing (28)
API.mdpom.xmlsrc/main/java/org/example/vet1177/controller/ActivityLogController.javasrc/main/java/org/example/vet1177/controller/PetController.javasrc/main/java/org/example/vet1177/controller/UserController.javasrc/main/java/org/example/vet1177/security/CustomUserDetailsService.javasrc/main/java/org/example/vet1177/security/JwtAuthenticationFilter.javasrc/main/java/org/example/vet1177/security/JwtProperties.javasrc/main/java/org/example/vet1177/security/JwtService.javasrc/main/java/org/example/vet1177/security/SecurityConfig.javasrc/main/java/org/example/vet1177/security/auth/dto/AuthRequest.javasrc/main/java/org/example/vet1177/security/auth/dto/AuthResponse.javasrc/main/java/org/example/vet1177/security/auth/dto/RegisterRequest.javasrc/main/java/org/example/vet1177/services/UserService.javasrc/main/resources/application.propertiessrc/test/java/org/example/vet1177/controller/ActivityLogControllerTest.javasrc/test/java/org/example/vet1177/controller/AttachmentControllerTest.javasrc/test/java/org/example/vet1177/controller/ClinicControllerTest.javasrc/test/java/org/example/vet1177/controller/CommentControllerTest.javasrc/test/java/org/example/vet1177/controller/MedicalRecordControllerTest.javasrc/test/java/org/example/vet1177/controller/PetControllerTest.javasrc/test/java/org/example/vet1177/controller/UserControllerTest.javasrc/test/java/org/example/vet1177/controller/VetControllerTest.javasrc/test/java/org/example/vet1177/entities/UserEntityTest.javasrc/test/java/org/example/vet1177/integration/activitylog/ActivityLogIntegrationTest.javasrc/test/java/org/example/vet1177/integration/vet/VetIntegrationTest.javasrc/test/java/org/example/vet1177/services/UserServiceTest.javasrc/test/resources/application-test.properties
✅ Files skipped from review due to trivial changes (3)
- src/test/java/org/example/vet1177/controller/CommentControllerTest.java
- src/main/resources/application.properties
- src/main/java/org/example/vet1177/security/JwtProperties.java
Summary by CodeRabbit
New Features
Documentation
Tests