Feature/change password#57
Conversation
Changes options for updating users in service, mapper and DTO. Also adds a log event when creating a user using OAUTH and updates the redirect
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (8)
📝 WalkthroughWalkthroughAdds transactional OAuth2 success handling that persists and audits newly created users and changes post-login redirect to /home; replaces UpdateUserDTO.email with password and limits mapper updates to fullName; updates service/controller/templates and tests to support optional password updates and authorization checks. Changes
Sequence Diagram(s)sequenceDiagram
participant OAuth2Provider as OAuth2 Provider
participant Handler as OauthSuccessHandler
participant UserRepo as UserRepository
participant LogService as UserLogService
participant DB as Database
OAuth2Provider->>Handler: Authentication success callback
Handler->>UserRepo: findByEmail(email)
alt user exists
UserRepo-->>Handler: existing User
else new user
Handler->>UserRepo: save(new User)
UserRepo->>DB: persist user
DB-->>UserRepo: persisted user (id)
UserRepo-->>Handler: returned persisted user
Handler->>LogService: createUserLog(savedUser.id, ..., CREATED, "User account created via OAUTH2.")
LogService->>DB: insert audit log
DB-->>LogService: audit recorded
end
Handler-->>OAuth2Provider: redirect to /home
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java (2)
90-114:⚠️ Potential issue | 🟠 MajorMissing authorization check on the target user.
updateUser(UpdateUserDTO dto, Long actorUserId)trusts bothdto.id()andactorUserIdfrom the caller and never verifies that the authenticated principal is allowed to modifydto.id(). Combined with the password-update path being added in this PR, that means any authenticated user (or, with the bug above, even an unauthenticated caller of a controller that doesn't hard-gate this) can submit{id: <victim>, password: "..."}and (once the persistence bug is fixed) take over the account.
validateProfileAccess(...)already exists at line 152 and encodes exactly the right rule (own profile orSYSADMIN). It should be invoked here — oractorUserIdshould be derived fromSecurityContextHolderrather than passed in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java` around lines 90 - 114, The updateUser(UpdateUserDTO dto, Long actorUserId) method currently trusts dto.id() and actorUserId; before allowing updates (especially password changes) call the existing validateProfileAccess(actorUserId, dto.id()) to enforce "own profile or SYSADMIN" rules (or alternatively derive actorUserId from SecurityContextHolder inside updateUser and use that value), and abort with an appropriate AccessDeniedException if validation fails; place this check immediately after loading the User and before any password encoding, userMapper.updateEntityFromDTO(...) or userRepository.saveAndFlush(...) operations so unauthorized modifications are blocked.
92-92:⚠️ Potential issue | 🟡 MinorStale "email" wording after the DTO change.
- Line 92 comment:
// Check if user and email exists— email is no longer part of this update path.- Line 105 message:
"A user with this email already exists"— the only field reaching the DB now isfullName, so this catch is effectively unreachable and the message is misleading. Either drop the try/catch or rewrite the message generically (e.g. "Failed to update user").Also applies to: 104-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java` at line 92, Update the stale comment and misleading error handling in UserService: replace the comment "// Check if user and email exists" with a correct description for the current flow (e.g., "// Check if user exists and prepare update for fullName") and change the catch block that currently logs "A user with this email already exists" to a generic message such as "Failed to update user" (or remove the try/catch if you prefer letting the exception propagate); locate this in the UserService update method (the method handling the user update/save) and ensure the log/message and comment reflect that only fullName is being persisted now.src/test/java/org/example/visacasemanagementsystem/user/mapper/UserMapperTest.java (1)
132-132:⚠️ Potential issue | 🟡 MinorUpdate test fixtures to reflect the new third component (
password).
UpdateUserDTO's third field is nowpassword, but a couple of tests still pass email-shaped strings in that slot:
- Line 132:
new UpdateUserDTO(1L, "Changed Name", "changed@example.com")- Line 164:
new UpdateUserDTO(1L, "Some Name", "some@example.com")The tests still pass (the mapper ignores the field), but the fixtures are now misleading to future readers. Consider
"newPassword123"/""instead.Also applies to: 164-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/user/mapper/UserMapperTest.java` at line 132, Update the test fixtures that construct UpdateUserDTO so the third argument represents a password (not an email). Replace the two instances of new UpdateUserDTO(1L, "Changed Name", "changed@example.com") and new UpdateUserDTO(1L, "Some Name", "some@example.com") with password-appropriate values (for example new UpdateUserDTO(1L, "Changed Name", "newPassword123") and new UpdateUserDTO(1L, "Some Name", "")) in the UserMapperTest so the constructor calls accurately reflect the DTO shape.
🧹 Nitpick comments (1)
src/main/java/org/example/visacasemanagementsystem/config/OauthSuccessHandler.java (1)
44-91: Optional: narrow@Transactionalscope to just the user-creation step.
@Transactionalnow wraps the whole handler, includingsecurityContextRepository.saveContext(...)andgetRedirectStrategy().sendRedirect(...). The DB connection is held across response I/O for every OAuth login, even for the common "user already exists" path where no writes occur. Consider extracting the find-or-create-with-audit logic into a small@Transactionalmethod on a service (e.g.,UserService#findOrCreateOAuthUser(email, fullName)) and dropping@Transactionalfrom the handler itself.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/config/OauthSuccessHandler.java` around lines 44 - 91, The handler currently annotates onAuthenticationSuccess with `@Transactional` which holds a DB transaction across response I/O; remove `@Transactional` from OauthSuccessHandler.onAuthenticationSuccess and extract the find-or-create-and-audit logic into a small `@Transactional` service method (e.g., UserService.findOrCreateOAuthUser(String email, String fullName)) that performs the userRepository.findByEmail(...).orElseGet(...) creation, userRepository.saveAndFlush(...), userLogService.createUserLog(...), and the DataIntegrityViolationException fallback to re-query; call that service method from onAuthenticationSuccess to obtain the User, leaving securityContextRepository.saveContext(...) and getRedirectStrategy().sendRedirect(...) outside any transaction.
🤖 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/visacasemanagementsystem/user/dto/UpdateUserDTO.java`:
- Around line 6-10: Update the UpdateUserDTO password declaration to prevent
nulls and enforce the same minimum-length policy as CreateUserDTO while still
allowing the empty string to mean "don't change"; specifically, annotate the
password component (UpdateUserDTO.password) with `@NotNull` to avoid the
dto.password() NPE in UserService.updateUser, and add a validation constraint
(e.g., `@Pattern` or `@Size` with a regex) that accepts either the empty string or a
string with at least 8 characters so the update path cannot set a 1-character
password but still allows "" to skip changing the password—keep the constraint
semantics consistent with CreateUserDTO.
In
`@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java`:
- Around line 96-99: The password encoding result is currently discarded and
dto.password() can NPE; in UserService (where dto.password() is checked) change
the logic to first null-check the DTO password (dto.password() != null &&
!dto.password().isBlank()), then call passwordEncoder.encode(...) and persist
that encoded value to the user entity (e.g., set the encoded string via
user.setPassword(...) or update the mapper behavior in
UserMapper.updateEntityFromDTO to accept and set the encoded password). Also add
an integration test in UserServiceIntegrationTest that calls updateUser with a
new plain password and asserts passwordEncoder.matches(newPlain,
repository.findById(...).getPassword()) to prevent regressions.
In
`@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceIntegrationTest.java`:
- Around line 72-88: Add a sibling integration test in
UserServiceIntegrationTest that exercises the password-update branch in
UserService.updateUser: create and save a user with a known plaintext password
via createAndSaveValidUser, authenticateUser(user), then call
userService.updateUser(new UpdateUserDTO(user.getId(), user.getFullName(),
"newPlainPassword"), user.getId()); finally load the persisted user via
userRepository.findById(...) and assert
passwordEncoder.matches("newPlainPassword", persisted.getPassword()) is true and
that passwordEncoder.matches(oldPlainPassword, persisted.getPassword()) is false
to ensure the new encoded password was stored and the old one no longer matches.
---
Outside diff comments:
In
`@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java`:
- Around line 90-114: The updateUser(UpdateUserDTO dto, Long actorUserId) method
currently trusts dto.id() and actorUserId; before allowing updates (especially
password changes) call the existing validateProfileAccess(actorUserId, dto.id())
to enforce "own profile or SYSADMIN" rules (or alternatively derive actorUserId
from SecurityContextHolder inside updateUser and use that value), and abort with
an appropriate AccessDeniedException if validation fails; place this check
immediately after loading the User and before any password encoding,
userMapper.updateEntityFromDTO(...) or userRepository.saveAndFlush(...)
operations so unauthorized modifications are blocked.
- Line 92: Update the stale comment and misleading error handling in
UserService: replace the comment "// Check if user and email exists" with a
correct description for the current flow (e.g., "// Check if user exists and
prepare update for fullName") and change the catch block that currently logs "A
user with this email already exists" to a generic message such as "Failed to
update user" (or remove the try/catch if you prefer letting the exception
propagate); locate this in the UserService update method (the method handling
the user update/save) and ensure the log/message and comment reflect that only
fullName is being persisted now.
In
`@src/test/java/org/example/visacasemanagementsystem/user/mapper/UserMapperTest.java`:
- Line 132: Update the test fixtures that construct UpdateUserDTO so the third
argument represents a password (not an email). Replace the two instances of new
UpdateUserDTO(1L, "Changed Name", "changed@example.com") and new
UpdateUserDTO(1L, "Some Name", "some@example.com") with password-appropriate
values (for example new UpdateUserDTO(1L, "Changed Name", "newPassword123") and
new UpdateUserDTO(1L, "Some Name", "")) in the UserMapperTest so the constructor
calls accurately reflect the DTO shape.
---
Nitpick comments:
In
`@src/main/java/org/example/visacasemanagementsystem/config/OauthSuccessHandler.java`:
- Around line 44-91: The handler currently annotates onAuthenticationSuccess
with `@Transactional` which holds a DB transaction across response I/O; remove
`@Transactional` from OauthSuccessHandler.onAuthenticationSuccess and extract the
find-or-create-and-audit logic into a small `@Transactional` service method (e.g.,
UserService.findOrCreateOAuthUser(String email, String fullName)) that performs
the userRepository.findByEmail(...).orElseGet(...) creation,
userRepository.saveAndFlush(...), userLogService.createUserLog(...), and the
DataIntegrityViolationException fallback to re-query; call that service method
from onAuthenticationSuccess to obtain the User, leaving
securityContextRepository.saveContext(...) and
getRedirectStrategy().sendRedirect(...) outside any transaction.
🪄 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: 1aa9ee07-2a60-4e8d-ae9e-6fc53cb6c068
📒 Files selected for processing (6)
src/main/java/org/example/visacasemanagementsystem/config/OauthSuccessHandler.javasrc/main/java/org/example/visacasemanagementsystem/user/dto/UpdateUserDTO.javasrc/main/java/org/example/visacasemanagementsystem/user/mapper/UserMapper.javasrc/main/java/org/example/visacasemanagementsystem/user/service/UserService.javasrc/test/java/org/example/visacasemanagementsystem/user/mapper/UserMapperTest.javasrc/test/java/org/example/visacasemanagementsystem/user/service/UserServiceIntegrationTest.java
💤 Files with no reviewable changes (1)
- src/main/java/org/example/visacasemanagementsystem/user/mapper/UserMapper.java
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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 (5)
src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java (2)
33-47:⚠️ Potential issue | 🟠 MajorAdd
@AfterEachto clearSecurityContextHolder— tests are no longer isolated.
authenticateUser(...)writes into the globalSecurityContextHolder, but nothing clears it between tests in this class. Combined with the next finding (a test that depends on auth being already set), this introduces order-dependent flakiness. The integration test has the same teardown — mirror it here.🛡️ Proposed fix
+ import org.junit.jupiter.api.AfterEach; + @@ class UserServiceTest { @@ `@InjectMocks` private UserService userService; + + `@AfterEach` + void tearDown() { + SecurityContextHolder.clearContext(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java` around lines 33 - 47, Tests in UserServiceTest are leaving authentication state in the global SecurityContextHolder because authenticateUser(...) writes to it; add an `@AfterEach` teardown method in the UserServiceTest class that calls SecurityContextHolder.clearContext() (or otherwise clears the Authentication) to reset global security state between tests so they remain isolated and order-independent; place the method alongside the existing `@Mock/`@InjectMocks fields in the UserServiceTest class.
140-158:⚠️ Potential issue | 🟠 MajorThis test invokes
updateUserwithout setting up an authenticated principal.
UserService.updateUsernow callsvalidateProfileAccess(getUserPrincipal(), dto.id())first, andgetUserPrincipal()reads fromSecurityContextHolder. This test never callsauthenticateUser(...), so it only "works" when a previous test happens to have populated the context (e.g.,updateUser_shouldThrowEntityNotFoundException_WhenUserDoesNotExistrunning first leaves a SYSADMIN principal there). Once test order changes — or after the@AfterEachcleanup is added — this will fail with NPE/ClassCastExceptionbefore reaching the duplicate-email branch under test.Add explicit auth setup for this test (e.g., authenticate as the target user or as a SYSADMIN).
🛡️ Proposed fix
void updateUser_shouldThrowIllegalArgumentException_WhenEmailAlreadyExists() { // Arrange Long userId = 1L; UpdateUserDTO dto = new UpdateUserDTO(userId, "User", "taken@test.com"); User existingUser = new User(); existingUser.setId(userId); + authenticateUser(existingUser);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java` around lines 140 - 158, The test updateUser_shouldThrowIllegalArgumentException_WhenEmailAlreadyExists fails to set up a SecurityContext so validateProfileAccess(getUserPrincipal(), dto.id()) throws before the duplicate-email branch; fix by authenticating a principal at the start of the test (e.g., call the existing authenticateUser(...) test helper or create and set a principal representing the target user or a SYSADMIN) before invoking userService.updateUser(dto, userId) so getUserPrincipal() returns a valid principal and the test reaches the DataIntegrityViolationException handling path in updateUser.src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java (2)
109-114:⚠️ Potential issue | 🟡 MinorAudit log overstates what changed.
The message
"User profile updated (fullName/password)."is logged unconditionally, even whendto.password()was blank and the password was deliberately left untouched. This will produce false positives in audit trails (e.g., investigations of password-change events).🔧 Proposed fix
+ boolean passwordChanged = !dto.password().isBlank(); User user = userRepository.findById(dto.id()) .orElseThrow(() -> new EntityNotFoundException(USER_NOT_FOUND)); - if (!dto.password().isBlank()) { + if (passwordChanged) { user.setPassword(passwordEncoder.encode(dto.password())); } @@ userLogService.createUserLog( actorUserId, savedUser.getId(), UserEventType.UPDATED, - "User profile updated (fullName/password)." + passwordChanged + ? "User profile updated (fullName, password)." + : "User profile updated (fullName)." );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java` around lines 109 - 114, The audit message currently passed to userLogService.createUserLog always says "User profile updated (fullName/password)." even when the password was not changed; update the logic in the method that calls userLogService.createUserLog to detect which fields actually changed (at minimum check dto.password() for non-blank vs blank) and build a conditional, accurate message (e.g., "User profile updated (fullName)" or "User profile updated (password)" or "User profile updated (fullName/password)") before calling userLogService.createUserLog with savedUser.getId() and UserEventType.UPDATED so the log reflects the real changes.
102-108:⚠️ Potential issue | 🟡 MinorMisleading exception message after the email→password switch.
UserMapper.updateEntityFromDTOnow only updatesfullName(no email/username writes), so aDataIntegrityViolationExceptionhere is no longer attributable to a duplicate email. Wrapping it as"A user with this email already exists"will mislead callers and end users when the underlying cause is something else.🔧 Proposed message tweak
} catch (DataIntegrityViolationException e) { - throw new IllegalArgumentException("A user with this email already exists", e); + throw new IllegalArgumentException("Could not update profile due to a data integrity violation", e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java` around lines 102 - 108, The catch block in UserService around userMapper.updateEntityFromDTO and userRepository.saveAndFlush is logging/throwing a misleading message that attributes any DataIntegrityViolationException to a duplicate email; change the error handling to either rethrow a more generic message (e.g., "Failed to save user due to data integrity violation") or include the original exception message/cause so callers see the real reason, or inspect the exception cause and only map to "duplicate email" when the SQL constraint name indicates an email unique constraint; update the catch for DataIntegrityViolationException accordingly in UserService.src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java (1)
155-164:⚠️ Potential issue | 🔴 CriticalCritical: plaintext password is being placed in the
UserDTOand leaked back to the view.
UserDTOis(Long id, String fullName, String email, UserAuthorization)— there is no password component. By passingpasswordas the 3rd argument here, you're stuffing the plaintext password intoprofile/edittemplate viath:value="${user.email()}". This:
- Corrupts the displayed email on the error path (the form will show the password instead).
- Leaks the plaintext password into HTML (and any logs/template caches that capture model state).
Don't put the password back into the model — re-prompt the user instead — and preserve the actual email from the existing record.
🛡️ Fix
- model.addAttribute("user", new UserDTO(userId, fullName, password, existing.userAuthorization())); + model.addAttribute("user", new UserDTO(userId, fullName, existing.email(), existing.userAuthorization()));Also ensure the password input in
profile/edit.htmldoes not pre-populate from the model on re-render (which it currently doesn't — keep it that way).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java` around lines 155 - 164, The catch block in UserViewController (handling IllegalArgumentException) is incorrectly constructing a new UserDTO with the plaintext password as the 3rd argument (email); update the construction to use the existing user's email from the re-fetched UserDTO (via userService.findById(...).orElseThrow(...)). Specifically, in the catch you should set model attribute "user" to new UserDTO(userId, fullName, existing.email() or existing.getEmail(), existing.userAuthorization()) instead of passing the password, and ensure you do not add any password back into the model; also verify profile/edit.html’s password input remains unpopulated from the model on re-render.
🧹 Nitpick comments (4)
src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceIntegrationTest.java (1)
75-92: LGTM — password-update branch is now covered.Asserting
passwordEncoder.matches("newPassword", updatedUser.getPassword())exercises the encode-and-persist path and would have caught the prior "encoded password discarded" regression. Optional follow-up: a sibling test passing""to assert the password is not changed would protect the skip-update branch from drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceIntegrationTest.java` around lines 75 - 92, Add a sibling integration test that verifies updateUser does not change the stored password when an empty password is provided: create a test (e.g., updateUser_shouldNotChangePassword_WhenPasswordEmpty) that uses createAndSaveValidUser(), authenticateUser(), constructs an UpdateUserDTO with an empty password for the same user id, calls userService.updateUser(dto, user.getId()), then reloads the user from userRepository and asserts passwordEncoder.matches(originalPlaintext, updatedUser.getPassword()) is true (i.e., password unchanged). Ensure the test asserts fullName behavior as needed and references UpdateUserDTO, userService.updateUser, userRepository.findById, and passwordEncoder to validate the skip-update branch.src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java (1)
131-131: Nit: the third DTO argument is nowpassword, but the values look like emails.After the DTO rename, the third constructor argument represents a password. Strings like
"email@test.com"/"taken@test.com"/"updated@test.com"happen to pass the^$|.{8,}pattern, but they're semantically misleading and obscure intent for future readers. Consider renaming to clearly password-shaped values.♻️ Suggested edits
- UpdateUserDTO dto = new UpdateUserDTO(999L, "Name", "email@test.com"); + UpdateUserDTO dto = new UpdateUserDTO(999L, "Name", "newPassword1"); @@ - UpdateUserDTO dto = new UpdateUserDTO(userId, "User", "taken@test.com"); + UpdateUserDTO dto = new UpdateUserDTO(userId, "User", "newPassword1"); @@ - UpdateUserDTO dto = new UpdateUserDTO(userId, "Updated Name", "updated@test.com"); + UpdateUserDTO dto = new UpdateUserDTO(userId, "Updated Name", "newPassword1");Also applies to: 145-145, 165-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java` at line 131, The UpdateUserDTO constructor's third parameter is a password but tests in UserServiceTest pass email-shaped strings (e.g., "email@test.com"); update all UpdateUserDTO instantiations in UserServiceTest to use realistic password-shaped values (e.g., "Password123!" or "newPassw0rd!") instead of email-looking strings so intent is clear—search for usages of new UpdateUserDTO(...) in the UserServiceTest class and replace the third-argument values accordingly.src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java (2)
91-93:actorUserIdparameter is now redundant — and divergeable — with the authenticated principal.
updateUserderives the access-control identity fromSecurityContextHolderviagetUserPrincipal(), but the actor recorded in the audit log still comes from the caller-suppliedactorUserId. A bug or misuse upstream that passes a different value (e.g., the target id, as happens implicitly in the controller viaprincipal.getUserId()— fine today but easy to break) will silently produce inconsistent audit entries. Consider derivingactorUserIdfrom the sameUserPrincipalyou already resolve, or assert they match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java` around lines 91 - 93, The updateUser method accepts an actorUserId that can diverge from the authenticated principal used by getUserPrincipal() (and validateProfileAccess), leading to inconsistent audit records; change updateUser to derive the actor id from the resolved UserPrincipal (call getUserPrincipal().getUserId()) and use that for audit logging (or, if you must keep the parameter for API compatibility, assert it equals the principal id and throw on mismatch) and update any callers accordingly so audit entries always reflect the authenticated principal; relevant symbols: updateUser, getUserPrincipal, validateProfileAccess, actorUserId.
163-166: SimplifygetUserPrincipal()for readability and clearer error handling.The double-nested
Objects.requireNonNull(...)on a single line is hard to read and produces a generic NPE on failure. While the OAuth success handler converts incomingDefaultOidcUserprincipals toUserPrincipalbefore storing in the SecurityContext, defensive null-checking and clearer exception messages still improve maintainability.♻️ Proposed refactor
- private static UserPrincipal getUserPrincipal() { - return (UserPrincipal) Objects.requireNonNull(Objects.requireNonNull(SecurityContextHolder.getContext().getAuthentication()).getPrincipal()); - } + private static UserPrincipal getUserPrincipal() { + Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); + if (authentication == null || !(authentication.getPrincipal() instanceof UserPrincipal principal)) { + throw new UnauthorizedException("Authenticated user principal is not available."); + } + return principal; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java` around lines 163 - 166, The getUserPrincipal() method uses nested Objects.requireNonNull calls making it hard to read and giving a generic NPE; replace it with a clear, stepwise null-check: retrieve Authentication via SecurityContextHolder.getContext().getAuthentication(), validate it's non-null with a descriptive IllegalStateException (or similar), get the principal, ensure it's an instance of UserPrincipal (or handle DefaultOidcUser conversion if needed), then cast and return; reference the method name getUserPrincipal and checks against SecurityContextHolder.getContext().getAuthentication().getPrincipal to locate where to implement the clearer checks and meaningful exception messages.
🤖 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/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java`:
- Around line 155-164: The catch block in UserViewController (handling
IllegalArgumentException) is incorrectly constructing a new UserDTO with the
plaintext password as the 3rd argument (email); update the construction to use
the existing user's email from the re-fetched UserDTO (via
userService.findById(...).orElseThrow(...)). Specifically, in the catch you
should set model attribute "user" to new UserDTO(userId, fullName,
existing.email() or existing.getEmail(), existing.userAuthorization()) instead
of passing the password, and ensure you do not add any password back into the
model; also verify profile/edit.html’s password input remains unpopulated from
the model on re-render.
In
`@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java`:
- Around line 109-114: The audit message currently passed to
userLogService.createUserLog always says "User profile updated
(fullName/password)." even when the password was not changed; update the logic
in the method that calls userLogService.createUserLog to detect which fields
actually changed (at minimum check dto.password() for non-blank vs blank) and
build a conditional, accurate message (e.g., "User profile updated (fullName)"
or "User profile updated (password)" or "User profile updated
(fullName/password)") before calling userLogService.createUserLog with
savedUser.getId() and UserEventType.UPDATED so the log reflects the real
changes.
- Around line 102-108: The catch block in UserService around
userMapper.updateEntityFromDTO and userRepository.saveAndFlush is
logging/throwing a misleading message that attributes any
DataIntegrityViolationException to a duplicate email; change the error handling
to either rethrow a more generic message (e.g., "Failed to save user due to data
integrity violation") or include the original exception message/cause so callers
see the real reason, or inspect the exception cause and only map to "duplicate
email" when the SQL constraint name indicates an email unique constraint; update
the catch for DataIntegrityViolationException accordingly in UserService.
In
`@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java`:
- Around line 33-47: Tests in UserServiceTest are leaving authentication state
in the global SecurityContextHolder because authenticateUser(...) writes to it;
add an `@AfterEach` teardown method in the UserServiceTest class that calls
SecurityContextHolder.clearContext() (or otherwise clears the Authentication) to
reset global security state between tests so they remain isolated and
order-independent; place the method alongside the existing `@Mock/`@InjectMocks
fields in the UserServiceTest class.
- Around line 140-158: The test
updateUser_shouldThrowIllegalArgumentException_WhenEmailAlreadyExists fails to
set up a SecurityContext so validateProfileAccess(getUserPrincipal(), dto.id())
throws before the duplicate-email branch; fix by authenticating a principal at
the start of the test (e.g., call the existing authenticateUser(...) test helper
or create and set a principal representing the target user or a SYSADMIN) before
invoking userService.updateUser(dto, userId) so getUserPrincipal() returns a
valid principal and the test reaches the DataIntegrityViolationException
handling path in updateUser.
---
Nitpick comments:
In
`@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java`:
- Around line 91-93: The updateUser method accepts an actorUserId that can
diverge from the authenticated principal used by getUserPrincipal() (and
validateProfileAccess), leading to inconsistent audit records; change updateUser
to derive the actor id from the resolved UserPrincipal (call
getUserPrincipal().getUserId()) and use that for audit logging (or, if you must
keep the parameter for API compatibility, assert it equals the principal id and
throw on mismatch) and update any callers accordingly so audit entries always
reflect the authenticated principal; relevant symbols: updateUser,
getUserPrincipal, validateProfileAccess, actorUserId.
- Around line 163-166: The getUserPrincipal() method uses nested
Objects.requireNonNull calls making it hard to read and giving a generic NPE;
replace it with a clear, stepwise null-check: retrieve Authentication via
SecurityContextHolder.getContext().getAuthentication(), validate it's non-null
with a descriptive IllegalStateException (or similar), get the principal, ensure
it's an instance of UserPrincipal (or handle DefaultOidcUser conversion if
needed), then cast and return; reference the method name getUserPrincipal and
checks against
SecurityContextHolder.getContext().getAuthentication().getPrincipal to locate
where to implement the clearer checks and meaningful exception messages.
In
`@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceIntegrationTest.java`:
- Around line 75-92: Add a sibling integration test that verifies updateUser
does not change the stored password when an empty password is provided: create a
test (e.g., updateUser_shouldNotChangePassword_WhenPasswordEmpty) that uses
createAndSaveValidUser(), authenticateUser(), constructs an UpdateUserDTO with
an empty password for the same user id, calls userService.updateUser(dto,
user.getId()), then reloads the user from userRepository and asserts
passwordEncoder.matches(originalPlaintext, updatedUser.getPassword()) is true
(i.e., password unchanged). Ensure the test asserts fullName behavior as needed
and references UpdateUserDTO, userService.updateUser, userRepository.findById,
and passwordEncoder to validate the skip-update branch.
In
`@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java`:
- Line 131: The UpdateUserDTO constructor's third parameter is a password but
tests in UserServiceTest pass email-shaped strings (e.g., "email@test.com");
update all UpdateUserDTO instantiations in UserServiceTest to use realistic
password-shaped values (e.g., "Password123!" or "newPassw0rd!") instead of
email-looking strings so intent is clear—search for usages of new
UpdateUserDTO(...) in the UserServiceTest class and replace the third-argument
values accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 414163e6-0638-41b0-ae33-eb96882de693
📒 Files selected for processing (6)
src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.javasrc/main/java/org/example/visacasemanagementsystem/user/dto/UpdateUserDTO.javasrc/main/java/org/example/visacasemanagementsystem/user/service/UserService.javasrc/test/java/org/example/visacasemanagementsystem/user/controller/UserViewControllerTest.javasrc/test/java/org/example/visacasemanagementsystem/user/service/UserServiceIntegrationTest.javasrc/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/main/resources/templates/user/signup.html (2)
55-66: Addautocomplete="new-password"to both password fields.Same rationale as the profile-edit form: tells browsers/password managers to suggest a generated password and avoid filling an existing one for a different account.
♻️ Proposed change
- <input type="password" name="password" id="password" - placeholder="Min. 8 characters" - minlength="8" - required> + <input type="password" name="password" id="password" + placeholder="Min. 8 characters" + minlength="8" + autocomplete="new-password" + required> </div> <div class="form-group"> <label>Confirm Password</label> - <input type="password" name="confirmPassword" id="confirmPassword" - placeholder="Repeat your password" + <input type="password" name="confirmPassword" id="confirmPassword" + placeholder="Repeat your password" + autocomplete="new-password" required>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/user/signup.html` around lines 55 - 66, The password inputs are missing the autocomplete hint—update the password and confirm password input elements (name="password", id="password" and name="confirmPassword", id="confirmPassword") to include autocomplete="new-password" so browsers/password managers will offer generated passwords and avoid autofilling existing credentials; ensure both <input> tags are updated consistently.
76-88: Consider extracting the confirm-password script to a shared file.This block is duplicated verbatim with
src/main/resources/templates/profile/edit.html(lines 101-113). Moving it to e.g.src/main/resources/static/js/confirm-password.jsand including it via<script th:src="@{/js/confirm-password.js}" defer></script>in both templates avoids the two copies drifting out of sync (e.g. when the validation message or selector logic changes). Same change applies to the profile-edit template.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/user/signup.html` around lines 76 - 88, The inline submit handler duplicated in signup.html and profile/edit.html — the document.querySelector('form').addEventListener('submit', ...) that reads elements with ids 'password' and 'confirmPassword' and uses setCustomValidity/reportValidity/preventDefault — should be extracted into a single shared JS file (e.g. confirm-password.js) containing the same event listener logic (including clearing validity on success), and both templates should include that shared file via a script include using Thymeleaf's th:src with defer so the behavior remains identical and the two copies can't drift out of sync.src/main/resources/templates/profile/edit.html (2)
101-113: Use a specific form selector and clear stale validity while typing.
document.querySelector('form')only attaches to the first form on the page. It works today because the profile-edit form precedes the sysadmin authorization form, but a future template reorder would silently disable password validation. Bind to the form by a more specific selector (e.g. its action) or add anid.Also, once
setCustomValidity('Passwords do not match')fires, the error sticks onconfirmPassworduntil the next submit; clearing it oninputgives smoother feedback as the user retypes.♻️ Proposed change
-<script> - document.querySelector('form').addEventListener('submit', function (e) { - const pw = document.getElementById('password'); - const cpw = document.getElementById('confirmPassword'); - if (pw.value !== cpw.value) { - cpw.setCustomValidity('Passwords do not match'); - cpw.reportValidity(); - e.preventDefault(); - } else { - cpw.setCustomValidity(''); - } - }); -</script> +<script> + const pw = document.getElementById('password'); + const cpw = document.getElementById('confirmPassword'); + const form = pw.closest('form'); + const sync = () => cpw.setCustomValidity(pw.value === cpw.value ? '' : 'Passwords do not match'); + pw.addEventListener('input', sync); + cpw.addEventListener('input', sync); + form.addEventListener('submit', function (e) { + sync(); + if (!cpw.checkValidity()) { + cpw.reportValidity(); + e.preventDefault(); + } + }); +</script>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/profile/edit.html` around lines 101 - 113, Replace the fragile document.querySelector('form') binding with a specific form selector (e.g. give the profile-edit form an id and select by '#profileEditForm' or select by its action attribute) so the validator always targets the intended form; keep using the existing element ids 'password' and 'confirmPassword' to compare values, and add an input listener on the confirmPassword (and optionally password) field to call setCustomValidity('') when the user types so any previous "Passwords do not match" message is cleared immediately rather than persisting until the next submit.
50-59: Addautocomplete="new-password"to the new/confirm password inputs.Without this hint, browsers and password managers may auto-fill the current password into the "new password" field, which is confusing on a password-change form and can also leak a stale value into the request. This is the standard recommendation for password-change/signup flows.
♻️ Proposed change
- <input type="password" name="password" id="password" - placeholder="Min. 8 characters" - minlength="8"> + <input type="password" name="password" id="password" + placeholder="Min. 8 characters" + minlength="8" + autocomplete="new-password"> </div> <div class="form-group"> <label>Confirm New Password</label> - <input type="password" name="confirmPassword" id="confirmPassword" - placeholder="Repeat new password"> + <input type="password" name="confirmPassword" id="confirmPassword" + placeholder="Repeat new password" + autocomplete="new-password">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/profile/edit.html` around lines 50 - 59, Add the autocomplete hint to the new password fields: update the password input with id="password" (name="password") and the confirm input with id="confirmPassword" (name="confirmPassword") to include autocomplete="new-password" so browsers/password managers treat these as new-password fields; ensure both <input> elements are changed accordingly in the template.
🤖 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/main/resources/templates/profile/edit.html`:
- Around line 101-113: Replace the fragile document.querySelector('form')
binding with a specific form selector (e.g. give the profile-edit form an id and
select by '#profileEditForm' or select by its action attribute) so the validator
always targets the intended form; keep using the existing element ids 'password'
and 'confirmPassword' to compare values, and add an input listener on the
confirmPassword (and optionally password) field to call setCustomValidity('')
when the user types so any previous "Passwords do not match" message is cleared
immediately rather than persisting until the next submit.
- Around line 50-59: Add the autocomplete hint to the new password fields:
update the password input with id="password" (name="password") and the confirm
input with id="confirmPassword" (name="confirmPassword") to include
autocomplete="new-password" so browsers/password managers treat these as
new-password fields; ensure both <input> elements are changed accordingly in the
template.
In `@src/main/resources/templates/user/signup.html`:
- Around line 55-66: The password inputs are missing the autocomplete
hint—update the password and confirm password input elements (name="password",
id="password" and name="confirmPassword", id="confirmPassword") to include
autocomplete="new-password" so browsers/password managers will offer generated
passwords and avoid autofilling existing credentials; ensure both <input> tags
are updated consistently.
- Around line 76-88: The inline submit handler duplicated in signup.html and
profile/edit.html — the
document.querySelector('form').addEventListener('submit', ...) that reads
elements with ids 'password' and 'confirmPassword' and uses
setCustomValidity/reportValidity/preventDefault — should be extracted into a
single shared JS file (e.g. confirm-password.js) containing the same event
listener logic (including clearing validity on success), and both templates
should include that shared file via a script include using Thymeleaf's th:src
with defer so the behavior remains identical and the two copies can't drift out
of sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6df16a86-45b7-4a51-9b2a-ac070d1e3411
📒 Files selected for processing (3)
src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.javasrc/main/resources/templates/profile/edit.htmlsrc/main/resources/templates/user/signup.html
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java
|
@coderabbitai Review the pull request again |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java (2)
33-47:⚠️ Potential issue | 🟠 MajorMissing
@AfterEachto clearSecurityContextHolder— leaks authentication between tests.
SecurityContextHolderdefaults to aThreadLocalstrategy, and JUnit 5 typically reuses test threads, so the principals set byauthenticateUser(...)inupdateUser_shouldThrowEntityNotFoundException_WhenUserDoesNotExist(line 129) andupdateUser_shouldUpdateAndReturnUser_WhenDataIsValid(line 169) leak into subsequent tests. This is exactly the pattern that hides the issue flagged below inupdateUser_shouldThrowIllegalArgumentException_WhenEmailAlreadyExists(which never authenticates).
UserServiceIntegrationTestalready does this correctly (@AfterEach { SecurityContextHolder.clearContext(); }).🛡️ Proposed fix
+import org.junit.jupiter.api.AfterEach; @@ `@InjectMocks` private UserService userService; + + `@AfterEach` + void tearDown() { + SecurityContextHolder.clearContext(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java` around lines 33 - 47, Add an `@AfterEach` teardown in UserServiceTest to call SecurityContextHolder.clearContext() so authentication from tests like authenticateUser(...) used in updateUser_shouldThrowEntityNotFoundException_WhenUserDoesNotExist and updateUser_shouldUpdateAndReturnUser_WhenDataIsValid does not leak into other tests; implement a method (e.g., void clearSecurityContext()) annotated with `@AfterEach` that invokes SecurityContextHolder.clearContext() to mirror UserServiceIntegrationTest's cleanup.
142-158:⚠️ Potential issue | 🟠 MajorTest never authenticates — only passes via context leaking from a prior test.
updateUser_shouldThrowIllegalArgumentException_WhenEmailAlreadyExistsdoes not callauthenticateUser(...), butUserService.updateUser(...)now starts withvalidateProfileAccess(getUserPrincipal(), dto.id()). With no authentication inSecurityContextHolder,getUserPrincipal()(line 164–166 ofUserService.java) callsObjects.requireNonNull(SecurityContextHolder.getContext().getAuthentication())and NPEs — which is neither what the test asserts nor what the test description implies. It currently appears green only because earlier tests in the run leak theirSecurityContext(see the missing@AfterEachcomment).Additionally, since the mapper no longer touches
fullName(and conditionallypassword) — there is no realistic path that triggers aDataIntegrityViolationExceptionfor an email collision anymore, so the test's premise is also stale.🐛 Proposed fix
void updateUser_shouldThrowIllegalArgumentException_WhenEmailAlreadyExists() { // Arrange Long userId = 1L; UpdateUserDTO dto = new UpdateUserDTO(userId, "User", "taken@test.com"); User existingUser = new User(); existingUser.setId(userId); + existingUser.setUserAuthorization(UserAuthorization.USER); + authenticateUser(existingUser); when(userRepository.findById(userId)).thenReturn(Optional.of(existingUser)); when(userRepository.saveAndFlush(any(User.class))) .thenThrow(new DataIntegrityViolationException("unique constraint"));Consider also renaming the test (and the underlying service exception message) to reflect that the only remaining trigger for
DataIntegrityViolationExceptionhere would be a non-email constraint, since the email field is no longer mutated byupdateUser.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java` around lines 142 - 158, The test updateUser_shouldThrowIllegalArgumentException_WhenEmailAlreadyExists is leaking SecurityContext and is outdated: call authenticateUser(...) at the start of the test to populate SecurityContext so UserService.updateUser (which calls validateProfileAccess(getUserPrincipal(), dto.id()) and ultimately uses SecurityContextHolder) doesn't NPE; also update the test scenario and name because UpdateUserDTO no longer updates email (mapper/service no longer sets email), so change the setup to simulate a save failing for a non-email DB constraint (have userRepository.saveAndFlush(any(User.class)) throw DataIntegrityViolationException) and assert the appropriate exception/message (or rename the test to reflect a generic DataIntegrityViolation => IllegalArgumentException path) instead of asserting an email-specific message.
🧹 Nitpick comments (8)
src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java (1)
323-333:createAndSaveUsername is misleading in a unit test with mocked repository.
userRepositoryis a Mockito@Mock, souserRepository.save(user)here is a no-op stub that returns null by default. The "saved" user is just the local in-memoryUserreturned from the helper. The name is borrowed from the integration-test helper but means something different here. Consider renaming tobuildUser(...)(drop theuserRepository.save(user)call entirely) to avoid implying persistence:♻️ Proposed rename
- private User createAndSaveUser(String name, UserAuthorization auth) { + private User buildUser(String name, UserAuthorization auth) { User user = new User(); String uniqueEmail = java.util.UUID.randomUUID() + "@test.com"; user.setFullName(name); user.setEmail(uniqueEmail); user.setUsername(uniqueEmail); user.setPassword("password123"); user.setUserAuthorization(auth); - userRepository.save(user); return user; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java` around lines 323 - 333, The helper method createAndSaveUser in UserServiceTest is misleading because userRepository is a Mockito `@Mock` and save(...) is a no-op; rename the method to buildUser (or buildTestUser) and remove the call to userRepository.save(user) so it only constructs and returns a User; update any callers to the new method name and ensure they do any necessary stubbing of userRepository.save(...) in tests where persisted behavior is required.src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java (2)
164-166:getUserPrincipal()is hard to read and brittle to non-UserPrincipalprincipals.The double
Objects.requireNonNullchain plus an unchecked cast produces opaque NPEs / ClassCastExceptions that are hard to diagnose. Class-level@PreAuthorize("isAuthenticated()")already guaranteesgetAuthentication()is non-null at this point, so the inner null-check is redundant. The remaining failure mode (principal not aUserPrincipal) deserves a clearer message:♻️ Suggested rewrite
- private static UserPrincipal getUserPrincipal() { - return (UserPrincipal) Objects.requireNonNull(Objects.requireNonNull(SecurityContextHolder.getContext().getAuthentication()).getPrincipal()); - } + private static UserPrincipal getUserPrincipal() { + Object principal = SecurityContextHolder.getContext().getAuthentication().getPrincipal(); + if (!(principal instanceof UserPrincipal userPrincipal)) { + throw new IllegalStateException( + "Expected UserPrincipal in SecurityContext but found: " + + (principal == null ? "null" : principal.getClass().getName())); + } + return userPrincipal; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java` around lines 164 - 166, getUserPrincipal currently uses nested Objects.requireNonNull and an unchecked cast which yields opaque NPEs/ClassCastExceptions; replace that with a clear runtime check: access SecurityContextHolder.getContext().getAuthentication() (no inner null-check needed due to class-level `@PreAuthorize`), obtain authentication.getPrincipal(), verify it's an instance of UserPrincipal (using instanceof), and if not throw an IllegalStateException (or similar) with a descriptive message that includes the actual principal class; then safely cast to UserPrincipal and return—update the method getUserPrincipal to implement this explicit instanceof check and clear error message.
102-108: Stale catch-block message — updateUser no longer modifies email.
UserMapper.updateEntityFromDTOwas narrowed to only setfullName, and this method only additionally setspassword(not unique). There is no realistic email path that produces aDataIntegrityViolationExceptionhere anymore, so the message"A user with this email already exists"will be misleading if it ever triggers (e.g., a stray unique-constraint violation onusernameor a future column). Consider tightening the message, or removing the catch entirely if no unique constraint can plausibly be hit on this update.♻️ Suggested wording
try { userMapper.updateEntityFromDTO(dto, user); savedUser = userRepository.saveAndFlush(user); } catch (DataIntegrityViolationException e) { - throw new IllegalArgumentException("A user with this email already exists", e); + throw new IllegalArgumentException("Could not update user due to a data integrity conflict", e); }Note:
UserViewControllerTest.updateProfile_WithDuplicateEmail_ShouldReturnEditViewWithErrorandupdateProfile_WhenUserConcurrentlyDeleted_ShouldReturnNotFoundmock the service to throwIllegalArgumentException("A user with this email already exists")— those test names/messages would also need a follow-up if the message changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java` around lines 102 - 108, The catch for DataIntegrityViolationException in the update flow (surrounding userMapper.updateEntityFromDTO(...) and userRepository.saveAndFlush(...)) has a stale, email-specific message; change it to a generic, accurate message (e.g. "Data integrity violation while updating user") and include the original exception as the cause, or remove the catch entirely if you confirm no unique constraint can be hit; also update any tests that mock IllegalArgumentException with the old email message (e.g. UserViewControllerTest cases) to match the new behavior/message.src/main/java/org/example/visacasemanagementsystem/config/OauthSuccessHandler.java (1)
44-91: LGTM — audit logging on OAuth2 first-time creation looks correct.The audit log is correctly scoped inside the
orElseGetbranch so it only fires for newly created accounts; theDataIntegrityViolationExceptionfallback intentionally skips logging since the existing user was already audited at original creation.@Transactionalensures the user save and audit log are atomic.Minor: the transaction stays open across the SecurityContext setup and
sendRedirect(...)HTTP I/O on lines 80–90. Not a blocker, but if you ever see DB connection contention, consider extracting the create-and-audit into a small@Transactionalservice method and keeping the response I/O outside the transaction boundary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/config/OauthSuccessHandler.java` around lines 44 - 91, The method onAuthenticationSuccess currently keeps a `@Transactional` boundary open across SecurityContext setup and HTTP redirect; extract the user creation-and-audit logic into a dedicated `@Transactional` service method (e.g., createUserIfNotExists or ensureUserWithAudit) that encapsulates the userRepository.findByEmail(...).orElseGet logic and the userLogService.createUserLog call, return the persisted User, then call that service from onAuthenticationSuccess so the transaction commits before creating the UserPrincipal, setting SecurityContextHolder, saving the context with securityContextRepository, and calling getRedirectStrategy().sendRedirect.src/test/java/org/example/visacasemanagementsystem/user/mapper/UserMapperTest.java (1)
132-132: Optional: stale email-format strings in the (now password) third DTO argument.
UpdateUserDTO's third component is nowpassword, but lines 132 and 164 still pass"changed@example.com"/"some@example.com". The tests still pass becauseUserMapper.updateEntityFromDTOignores the password field entirely, but a future reader will be confused. Consider replacing with realistic password literals (e.g."newPassword123"/"") to keep intent clear.Also applies to: 164-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/user/mapper/UserMapperTest.java` at line 132, Tests use UpdateUserDTO where the third parameter is now password but still pass email-style strings; update the test instantiations in UserMapperTest (the UpdateUserDTO constructors at the spots around the existing lines) to use realistic password literals (e.g. "newPassword123" or empty "" as appropriate) so the DTO matches its current shape and intent, leaving UserMapper.updateEntityFromDTO behavior unchanged.src/main/resources/templates/user/signup.html (1)
53-66: LGTM — minor: mirrorminlength="8"on the confirm field.There is only one
<form>on this page soquerySelector('form')is unambiguous, and the JS validation correctly blocks submit on mismatch and clearscustomValidityon match. As a small consistency touch, addingminlength="8"on#confirmPasswordkeeps native constraint validation symmetric (otherwise a short paste in confirm could pass length validation while password fails it, producing slightly inconsistent UX).Also applies to: 76-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/user/signup.html` around lines 53 - 66, The confirm password input (`#confirmPassword`) is missing minlength="8", causing asymmetric native validation; add minlength="8" to the <input id="confirmPassword" name="confirmPassword"> (and apply the same change to any other confirmPassword input instances in this template) so both `#password` and `#confirmPassword` have matching minlength constraints.src/main/resources/templates/profile/edit.html (1)
101-113: Fragile JS scoping —querySelector('form')matches the first form on the page.There are two
<form>elements on this page: the profile-edit form (line 38) and, conditionally, the sysadmin authorization form (line 82). It works today because the profile-edit form happens to come first, but if the order is ever reshuffled, the listener will be attached to a form that has no#password/#confirmPasswordinputs andpw.valuewill throw. Scoping the listener to the actual form is safer:♻️ Proposed fix
-<form th:action="@{/profile/edit/{id}(id=${user.id()})}" method="post"> +<form id="profileEditForm" th:action="@{/profile/edit/{id}(id=${user.id()})}" method="post">- document.querySelector('form').addEventListener('submit', function (e) { + document.getElementById('profileEditForm').addEventListener('submit', function (e) { const pw = document.getElementById('password'); const cpw = document.getElementById('confirmPassword');Also worth considering: the equality JS does not enforce
minlength=8onconfirmPassword. Since browsers run native constraint validation before custom JS runsreportValidity, the password field'sminlengthcovers it — but if the user pastes a short string that bypasses the keystroke counter, mirroringminlength=8onconfirmPasswordis a small safety net.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/profile/edit.html` around lines 101 - 113, The submit handler is attached to document.querySelector('form') which can pick the wrong form and then pw/cpw may be undefined; instead locate the password inputs by id ('password' and 'confirmPassword'), ensure they exist, get their owning form via pw.form (or cpw.form) and attach the submit listener to that form only, guarding for nulls and calling e.preventDefault() when values differ; also add minlength="8" to the confirmPassword input to mirror the password constraint as a small extra safety net.src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceIntegrationTest.java (1)
75-92: LGTM — covers the new password-update branch end-to-end.Wiring in the real
PasswordEncoderbean and assertingpasswordEncoder.matches("newPassword", updatedUser.getPassword())correctly verifies that the encoded password is actually persisted (the previously flagged regression).@AfterEachclearingSecurityContextHolderkeeps test isolation clean.Optional: a sibling test asserting that
updateUserwithpassword=""leaves the existing hash unchanged would lock in the "skip-update" branch behavior as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceIntegrationTest.java` around lines 75 - 92, Add a sibling test in UserServiceIntegrationTest that verifies updateUser skips password change when an empty password is provided: create and save a valid User via createAndSaveValidUser(), authenticateUser(user), call userService.updateUser(new UpdateUserDTO(user.getId(), "NameChange", "" ), user.getId()), then fetch the persisted User via userRepository.findById(...) and assert the password hash is unchanged using the injected passwordEncoder.matches(...) (compare against the original hash saved before the update); this ensures the updateUser(password-empty) branch behavior is covered.
🤖 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/visacasemanagementsystem/user/controller/UserViewController.java`:
- Around line 156-161: The update path in UserViewController currently only
compares passwords and constructs UpdateUserDTO before calling
userService.updateUser; add the same explicit minimum-length check used in
createUser to prevent 1–7 char passwords: after verifying
password.equals(confirmPassword) and before creating UpdateUserDTO, if
(!password.isBlank() && password.length() < 8) throw new
IllegalArgumentException("Password must be at least 8 characters"); this ensures
UpdateUserDTO / userService.updateUser receives only passwords meeting the
`@Pattern` requirement.
---
Outside diff comments:
In
`@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java`:
- Around line 33-47: Add an `@AfterEach` teardown in UserServiceTest to call
SecurityContextHolder.clearContext() so authentication from tests like
authenticateUser(...) used in
updateUser_shouldThrowEntityNotFoundException_WhenUserDoesNotExist and
updateUser_shouldUpdateAndReturnUser_WhenDataIsValid does not leak into other
tests; implement a method (e.g., void clearSecurityContext()) annotated with
`@AfterEach` that invokes SecurityContextHolder.clearContext() to mirror
UserServiceIntegrationTest's cleanup.
- Around line 142-158: The test
updateUser_shouldThrowIllegalArgumentException_WhenEmailAlreadyExists is leaking
SecurityContext and is outdated: call authenticateUser(...) at the start of the
test to populate SecurityContext so UserService.updateUser (which calls
validateProfileAccess(getUserPrincipal(), dto.id()) and ultimately uses
SecurityContextHolder) doesn't NPE; also update the test scenario and name
because UpdateUserDTO no longer updates email (mapper/service no longer sets
email), so change the setup to simulate a save failing for a non-email DB
constraint (have userRepository.saveAndFlush(any(User.class)) throw
DataIntegrityViolationException) and assert the appropriate exception/message
(or rename the test to reflect a generic DataIntegrityViolation =>
IllegalArgumentException path) instead of asserting an email-specific message.
---
Nitpick comments:
In
`@src/main/java/org/example/visacasemanagementsystem/config/OauthSuccessHandler.java`:
- Around line 44-91: The method onAuthenticationSuccess currently keeps a
`@Transactional` boundary open across SecurityContext setup and HTTP redirect;
extract the user creation-and-audit logic into a dedicated `@Transactional`
service method (e.g., createUserIfNotExists or ensureUserWithAudit) that
encapsulates the userRepository.findByEmail(...).orElseGet logic and the
userLogService.createUserLog call, return the persisted User, then call that
service from onAuthenticationSuccess so the transaction commits before creating
the UserPrincipal, setting SecurityContextHolder, saving the context with
securityContextRepository, and calling getRedirectStrategy().sendRedirect.
In
`@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java`:
- Around line 164-166: getUserPrincipal currently uses nested
Objects.requireNonNull and an unchecked cast which yields opaque
NPEs/ClassCastExceptions; replace that with a clear runtime check: access
SecurityContextHolder.getContext().getAuthentication() (no inner null-check
needed due to class-level `@PreAuthorize`), obtain authentication.getPrincipal(),
verify it's an instance of UserPrincipal (using instanceof), and if not throw an
IllegalStateException (or similar) with a descriptive message that includes the
actual principal class; then safely cast to UserPrincipal and return—update the
method getUserPrincipal to implement this explicit instanceof check and clear
error message.
- Around line 102-108: The catch for DataIntegrityViolationException in the
update flow (surrounding userMapper.updateEntityFromDTO(...) and
userRepository.saveAndFlush(...)) has a stale, email-specific message; change it
to a generic, accurate message (e.g. "Data integrity violation while updating
user") and include the original exception as the cause, or remove the catch
entirely if you confirm no unique constraint can be hit; also update any tests
that mock IllegalArgumentException with the old email message (e.g.
UserViewControllerTest cases) to match the new behavior/message.
In `@src/main/resources/templates/profile/edit.html`:
- Around line 101-113: The submit handler is attached to
document.querySelector('form') which can pick the wrong form and then pw/cpw may
be undefined; instead locate the password inputs by id ('password' and
'confirmPassword'), ensure they exist, get their owning form via pw.form (or
cpw.form) and attach the submit listener to that form only, guarding for nulls
and calling e.preventDefault() when values differ; also add minlength="8" to the
confirmPassword input to mirror the password constraint as a small extra safety
net.
In `@src/main/resources/templates/user/signup.html`:
- Around line 53-66: The confirm password input (`#confirmPassword`) is missing
minlength="8", causing asymmetric native validation; add minlength="8" to the
<input id="confirmPassword" name="confirmPassword"> (and apply the same change
to any other confirmPassword input instances in this template) so both `#password`
and `#confirmPassword` have matching minlength constraints.
In
`@src/test/java/org/example/visacasemanagementsystem/user/mapper/UserMapperTest.java`:
- Line 132: Tests use UpdateUserDTO where the third parameter is now password
but still pass email-style strings; update the test instantiations in
UserMapperTest (the UpdateUserDTO constructors at the spots around the existing
lines) to use realistic password literals (e.g. "newPassword123" or empty "" as
appropriate) so the DTO matches its current shape and intent, leaving
UserMapper.updateEntityFromDTO behavior unchanged.
In
`@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceIntegrationTest.java`:
- Around line 75-92: Add a sibling test in UserServiceIntegrationTest that
verifies updateUser skips password change when an empty password is provided:
create and save a valid User via createAndSaveValidUser(),
authenticateUser(user), call userService.updateUser(new
UpdateUserDTO(user.getId(), "NameChange", "" ), user.getId()), then fetch the
persisted User via userRepository.findById(...) and assert the password hash is
unchanged using the injected passwordEncoder.matches(...) (compare against the
original hash saved before the update); this ensures the
updateUser(password-empty) branch behavior is covered.
In
`@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java`:
- Around line 323-333: The helper method createAndSaveUser in UserServiceTest is
misleading because userRepository is a Mockito `@Mock` and save(...) is a no-op;
rename the method to buildUser (or buildTestUser) and remove the call to
userRepository.save(user) so it only constructs and returns a User; update any
callers to the new method name and ensure they do any necessary stubbing of
userRepository.save(...) in tests where persisted behavior is required.
🪄 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: c34cff9f-f0ed-4763-ac77-3a9cade3d02f
📒 Files selected for processing (11)
src/main/java/org/example/visacasemanagementsystem/config/OauthSuccessHandler.javasrc/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.javasrc/main/java/org/example/visacasemanagementsystem/user/dto/UpdateUserDTO.javasrc/main/java/org/example/visacasemanagementsystem/user/mapper/UserMapper.javasrc/main/java/org/example/visacasemanagementsystem/user/service/UserService.javasrc/main/resources/templates/profile/edit.htmlsrc/main/resources/templates/user/signup.htmlsrc/test/java/org/example/visacasemanagementsystem/user/controller/UserViewControllerTest.javasrc/test/java/org/example/visacasemanagementsystem/user/mapper/UserMapperTest.javasrc/test/java/org/example/visacasemanagementsystem/user/service/UserServiceIntegrationTest.javasrc/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java
💤 Files with no reviewable changes (1)
- src/main/java/org/example/visacasemanagementsystem/user/mapper/UserMapper.java
…st hygiene Addresses CodeRabbit review feedback across the user signup/profile edit flow, the OAuth success path, and several stale or misleading tests. Controller / service: - UserViewController.updateProfile: add explicit min-length check for non-empty passwords. UpdateUserDTO carries an @pattern constraint, but UserService.updateUser is not annotated @Valid, so the constraint never fired. Mirrors the check already present in createUser. - UserService.getUserPrincipal: replace nested Objects.requireNonNull + unchecked cast with an explicit instanceof UserPrincipal check that throws IllegalStateException with the actual principal class name on mismatch. - UserService.updateUser: replace the stale email-specific message in the DataIntegrityViolationException catch with a generic "Data integrity violation while updating user" message; email is no longer mutable on this path. OAuth success handler: - Extract the find-or-create-and-audit logic out of OauthSuccessHandler into a new @transactional UserService.findOrCreateOauthUser(email, fullName) method (@PreAuthorize("permitAll()")). The transaction now commits before the handler builds the SecurityContext, saves it, and issues the redirect. - OauthSuccessHandler now depends only on UserService + SecurityContextRepository, dropping its direct use of UserRepository, PasswordEncoder, and UserLogService. - Remove @transactional from onAuthenticationSuccess. Templates: - user/signup.html: add minlength="8" to #confirmPassword to mirror #password. - profile/edit.html: add minlength="8" to #confirmPassword. - Both templates: replace document.querySelector('form') in the password-mismatch script with a self-contained IIFE that locates inputs by id, resolves the owning form via pw.form, and null-guards each step. On profile/edit.html this prevents the listener from being attached to the sysadmin authorization form. Test fixes: - UserViewControllerTest: add the now-required confirmPassword param to all POST /user/signup and POST /profile/edit/{id} requests. Resolves seven failing tests: - createUser_WithValidData_ShouldRedirectToLogin - createUser_WithDuplicateEmail_ShouldReturnSignupViewWithError - createUser_WithShortPassword_ShouldReturnSignupViewWithError - updateProfile_WithValidData_ShouldRedirectToProfileView - updateProfile_WithDuplicateEmail_ShouldReturnEditViewWithError - updateProfile_WhenUserConcurrentlyDeleted_ShouldReturnNotFound - updateProfile_AsUnauthorizedUser_ShouldReturnForbidden - UserServiceTest: add @AfterEach SecurityContextHolder.clearContext() to mirror UserServiceIntegrationTest and prevent auth leaking between tests. - UserServiceTest: rename updateUser_shouldThrowIllegalArgumentException_WhenEmailAlreadyExists to updateUser_shouldThrowIllegalArgumentException_WhenDataIntegrityViolation, add the missing authenticateUser(...) call so getUserPrincipal() doesn't NPE, use a password literal in the DTO instead of an email string, and assert against the new generic message. - UserServiceTest: rename createAndSaveUser to buildUser and drop the no-op userRepository.save(user) call (the repository is a @mock, so save was a no-op and the name was misleading). Update callers. - UserServiceIntegrationTest: add updateUser_shouldSkipPasswordChange_WhenPasswordIsEmpty covering the !dto.password().isBlank() skip branch by capturing the original encoded hash and asserting it is unchanged after an empty-password update. - UserMapperTest: in two UpdateUserDTO instantiations replace email-style strings in the third (password) argument with realistic password literals so the test data matches the DTO's current shape.
2 participants
Summary by CodeRabbit
New Features
Changes
Tests