Conversation
## Walkthrough
This update introduces new user password management features, including password change and recovery, by adding service, controller, and DTO classes. It also enhances input validation with new and updated annotation interfaces, augments API documentation, and expands error handling with a new error code. Swagger and validation annotations are applied to relevant request DTOs.
## Changes
| File(s) | Change Summary |
|-----------------------------------------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------|
| .../application/user/MyUserService.java | Added new service class for password change and recovery with transactional methods and custom exception handling. |
| .../presentation/user/MyUserController.java<br>.../presentation/user/MyUserControllerDocs.java | Added new REST controller and Swagger documentation interface for password-related endpoints. |
| .../presentation/user/dto/ChangePasswordRequest.java<br>.../ChangePasswordResponse.java<br>.../FindPasswordRequest.java | Introduced new DTO records for password change and recovery requests/responses, with validation annotations. |
| .../domain/user/UserJpaEntity.java | Added public method to update the user's password field. |
| .../global/annotation/LoginIdPattern.java | Introduced new annotation for validating login ID format and non-blank constraint. |
| .../global/annotation/PhoneNumberPattern.java | Updated annotation to add `@NotBlank` for phone number validation. |
| .../global/annotation/PasswordPattern.java | Updated annotation to add `@NotBlank` for password validation. |
| .../global/exception/ErrorCode.java | Added new error code `USER_INFO_INVALID` for invalid user information scenarios. |
| .../presentation/auth/dto/LoginRequest.java | Enhanced login request DTO with Swagger and validation annotations for input fields. |
| .../presentation/auth/dto/SignUpRequest.java | Added login ID validation annotation and updated Swagger description for sign-up request DTO. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Client
participant MyUserController
participant MyUserService
participant UserJpaEntity
participant Repository
Client->>MyUserController: POST /user/me/password (userId, ChangePasswordRequest)
MyUserController->>MyUserService: changePassword(userId, request)
MyUserService->>Repository: findById(userId)
Repository-->>MyUserService: UserJpaEntity
MyUserService->>UserJpaEntity: changePassword(encodedNewPassword)
MyUserService-->>MyUserController: ChangePasswordResponse
MyUserController-->>Client: ApiResponseWrapper<ChangePasswordResponse>
Client->>MyUserController: POST /user/me/find-password (userId, FindPasswordRequest)
MyUserController->>MyUserService: findPassword(userId, request)
MyUserService->>Repository: findById(userId)
Repository-->>MyUserService: UserJpaEntity
MyUserService->>MyUserService: validate name/loginId/phone
MyUserService-->>MyUserController: Void
MyUserController-->>Client: ApiResponseWrapper<Void>Suggested labels
Suggested reviewers
Poem
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @wlgns12370, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request implements core functionalities for user password management, enabling users to change their passwords and initiate a password recovery process. It also introduces stricter input validation for user IDs and phone numbers across relevant DTOs, enhancing data integrity and security.
Highlights
- Password Management API: Introduced new API endpoints and corresponding service logic for changing and finding user passwords. This includes
/user/me/passwordfor password updates and/user/me/find-passwordfor password retrieval. - Input Validation Enhancements: A new custom validation annotation,
@LoginIdPattern, has been added to enforce specific format rules for user login IDs. Additionally, the existing@PhoneNumberPatternannotation was enhanced to ensure phone number fields are not blank. - Error Handling: A new error code,
USER_INFO_INVALID, was added to theErrorCodeenum to specifically handle scenarios where provided user information is incorrect during password retrieval attempts. - Domain Model Update: The
UserJpaEntitydomain model now includes achangePasswordmethod, centralizing the logic for updating a user's password within the entity itself.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request adds password change and password finding functionalities. The changes are well-structured, introducing new services, controllers, and DTOs. My review has identified critical security vulnerabilities (IDOR) in the new controller endpoints that must be addressed, as well as a critical bug related to incorrect string comparison. Additionally, I've provided several suggestions to improve code quality and maintainability by removing redundant code and adhering to Java naming conventions.
| UserJpaEntity user = userJpaRepository.findById(userId) | ||
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.USER_NOT_FOUND)); | ||
|
|
||
| if (user.getName() != request.name() || user.getLoginId() != request.loginId()) { |
There was a problem hiding this comment.
String objects are being compared using !=. In Java, == and != compare object references, not the actual string content. This can lead to incorrect behavior where two strings with the same content are considered different because they are different objects in memory. You should use the .equals() method for string comparison. Additionally, since user.getName() and request.name() could potentially be null, using java.util.Objects.equals() is a safer approach to prevent NullPointerExceptions.
| if (user.getName() != request.name() || user.getLoginId() != request.loginId()) { | |
| if (!java.util.Objects.equals(user.getName(), request.name()) || !java.util.Objects.equals(user.getLoginId(), request.loginId())) { |
| public ResponseEntity<ApiResponseWrapper<ChangePasswordResponse>> changePassword( | ||
| @RequestParam Long userId, | ||
| @RequestBody @Valid ChangePasswordRequest request | ||
| ) { |
There was a problem hiding this comment.
The endpoint /user/me/password suggests that the password change should apply to the currently authenticated user. However, the userId is taken from a @RequestParam. This is a serious security vulnerability known as Insecure Direct Object Reference (IDOR), as it allows any authenticated user to attempt to change the password of any other user by simply providing their userId. The user's ID should be retrieved from the security context instead of being passed as a request parameter.
| public ResponseEntity<ApiResponseWrapper<Void>> findPassword( | ||
| @RequestParam Long userId, | ||
| @RequestBody @Valid FindPasswordRequest request | ||
| ) { |
There was a problem hiding this comment.
Similar to the changePassword endpoint, /user/me/find-password takes userId as a @RequestParam, which is a critical Insecure Direct Object Reference (IDOR) vulnerability. The user's identity should be confirmed via the security context, not a request parameter that can be manipulated by a malicious user.
| @NotNull String id, | ||
| @PasswordPattern @NotNull String password | ||
| @Schema(description = "아이디는 6~12자의 영문, 숫자, 특수문자(-, _)만 사용 가능합니다.", example = "user123", required = true) | ||
| @LoginIdPattern @NotNull String id, |
There was a problem hiding this comment.
The custom annotation @LoginIdPattern is already meta-annotated with @NotBlank. The @NotBlank annotation handles null checks, so the @NotNull annotation here is redundant and can be removed for better code clarity.
| @LoginIdPattern @NotNull String id, | |
| @LoginIdPattern String id, |
| @LoginIdPattern | ||
| @NotNull String id, |
There was a problem hiding this comment.
The custom annotation @LoginIdPattern is already meta-annotated with @NotBlank. The @NotBlank annotation handles null checks, so the @NotNull annotation on the next line is redundant and can be removed for better code clarity.
| @LoginIdPattern | |
| @NotNull String id, | |
| @LoginIdPattern String id, |
| public interface MyUserControllerDocs { | ||
|
|
||
| @Operation(summary = "비밀번호 변경", description = "현재 로그인한 사용자의 비밀번호를 변경합니다. 새 비밀번호와 새 비밀번호 확인이 일치해야 합니다.(입력값으로 valificationCode 필드 추가 예정)") | ||
| public ResponseEntity<ApiResponseWrapper<ChangePasswordResponse>> changePassword( |
There was a problem hiding this comment.
In a Java interface, methods are public by default. The public access modifier is redundant here and can be removed to improve code clarity and adhere to common conventions.
| public ResponseEntity<ApiResponseWrapper<ChangePasswordResponse>> changePassword( | |
| ResponseEntity<ApiResponseWrapper<ChangePasswordResponse>> changePassword( |
| ); | ||
|
|
||
| @Operation(summary = "비밀번호 찾기", description = "사용자의 비밀번호를 찾기 위한 API입니다. (반환값으로 전화번호 본인인증 후 valificationCode 필드 추가 예정)") | ||
| public ResponseEntity<ApiResponseWrapper<Void>> findPassword( |
There was a problem hiding this comment.
In a Java interface, methods are public by default. The public access modifier is redundant here and can be removed to improve code clarity and adhere to common conventions.
| public ResponseEntity<ApiResponseWrapper<Void>> findPassword( | |
| ResponseEntity<ApiResponseWrapper<Void>> findPassword( |
| @LoginIdPattern | ||
| String loginId, | ||
| @PhoneNumberPattern | ||
| String PhoneNumber |
There was a problem hiding this comment.
According to Java naming conventions, field names (and record components) should be in camelCase. The component PhoneNumber should be renamed to phoneNumber.
| String PhoneNumber | |
| String phoneNumber |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
src/main/java/life/mosu/mosuserver/presentation/user/dto/ChangePasswordResponse.java (1)
3-9: Consider enhancing response with more detailed information.The implementation is clean and follows good patterns. However, consider if a simple success boolean provides sufficient feedback for password change operations. You might want to include additional context like timestamp or operation details for better user experience.
src/main/java/life/mosu/mosuserver/presentation/user/MyUserControllerDocs.java (2)
15-15: Consider removing implementation details from API documentation.The description mentions future implementation details ("입력값으로 valificationCode 필드 추가 예정") which may confuse current API consumers and should be tracked in development documentation instead.
Consider simplifying the description:
-@Operation(summary = "비밀번호 변경", description = "현재 로그인한 사용자의 비밀번호를 변경합니다. 새 비밀번호와 새 비밀번호 확인이 일치해야 합니다.(입력값으로 valificationCode 필드 추가 예정)") +@Operation(summary = "비밀번호 변경", description = "현재 로그인한 사용자의 비밀번호를 변경합니다. 새 비밀번호와 새 비밀번호 확인이 일치해야 합니다.")
21-21: Consider removing implementation details from API documentation.Similar to the password change endpoint, the description contains future implementation details that should be in development documentation.
Consider simplifying the description:
-@Operation(summary = "비밀번호 찾기", description = "사용자의 비밀번호를 찾기 위한 API입니다. (반환값으로 전화번호 본인인증 후 valificationCode 필드 추가 예정)") +@Operation(summary = "비밀번호 찾기", description = "사용자의 비밀번호를 찾기 위한 API입니다.")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/life/mosu/mosuserver/application/user/MyUserService.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/user/UserJpaEntity.java(1 hunks)src/main/java/life/mosu/mosuserver/global/annotation/LoginIdPattern.java(1 hunks)src/main/java/life/mosu/mosuserver/global/annotation/PhoneNumberPattern.java(2 hunks)src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/dto/LoginRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/dto/SignUpRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/user/MyUserController.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/user/MyUserControllerDocs.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/user/dto/ChangePasswordRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/user/dto/ChangePasswordResponse.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/user/dto/FindPasswordRequest.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/life/mosu/mosuserver/application/user/MyUserService.java (1)
src/main/java/life/mosu/mosuserver/global/util/EncodeUtil.java (1)
EncodeUtil(5-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (9)
src/main/java/life/mosu/mosuserver/global/annotation/PhoneNumberPattern.java (1)
5-5: LGTM! Good enhancement to phone number validation.Adding
@NotBlankalongside the existing@Patternconstraint creates a comprehensive validation that ensures phone numbers are both non-empty and properly formatted. This follows Spring validation best practices for composite annotations.Also applies to: 16-16
src/main/java/life/mosu/mosuserver/presentation/user/dto/ChangePasswordRequest.java (1)
6-14: LGTM! Clean and well-validated DTO implementation.The record structure is concise and appropriate for this use case. The validation annotations (
@NotBlankand@PasswordPattern) ensure proper input validation, and the Korean error message provides clear user feedback.src/main/java/life/mosu/mosuserver/global/annotation/LoginIdPattern.java (1)
12-24: LGTM! Well-structured validation annotation.The annotation correctly combines
@Patternand@NotBlankconstraints with appropriate regex for login ID validation. The 6-12 character length restriction and allowed character set (alphanumeric, hyphen, underscore) provide good security boundaries while maintaining usability. Korean error messages are consistent with the application's localization.src/main/java/life/mosu/mosuserver/domain/user/UserJpaEntity.java (1)
92-94: LGTM! Appropriately simple domain entity method.The
changePasswordmethod correctly focuses on state management without including validation or encoding logic, which should be handled at the service layer. This follows good domain-driven design principles by keeping the entity focused on its core responsibility.src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java (1)
31-31: LGTM! Appropriate error code addition.The
USER_INFO_INVALIDerror code follows established patterns with correct HTTP status (BAD_REQUEST) and clear Korean error message. It's properly placed in the user-related error section and will be useful for password recovery validation scenarios.src/main/java/life/mosu/mosuserver/presentation/auth/dto/SignUpRequest.java (1)
10-10: LGTM! Validation standardization improves consistency.The addition of
@LoginIdPatternannotation and updated schema description enhances input validation consistency across the application.Also applies to: 16-16, 19-19
src/main/java/life/mosu/mosuserver/presentation/auth/dto/LoginRequest.java (1)
3-3: Excellent enhancement of API documentation and validation.The addition of Swagger annotations and
@LoginIdPatternvalidation improves both API documentation and input validation consistency. The schema descriptions provide clear guidance for API consumers.Also applies to: 5-5, 9-12
src/main/java/life/mosu/mosuserver/application/user/MyUserService.java (1)
1-23: LGTM: Clean service structure and dependencies.The service class is well-structured with proper dependency injection, appropriate annotations, and clear separation of concerns.
src/main/java/life/mosu/mosuserver/presentation/user/MyUserControllerDocs.java (1)
12-26: LGTM: Well-structured API documentation interface.The interface properly uses Swagger annotations and provides clear API documentation. The method signatures are consistent with the service layer implementation.
src/main/java/life/mosu/mosuserver/presentation/user/dto/FindPasswordRequest.java
Outdated
Show resolved
Hide resolved
| import life.mosu.mosuserver.global.annotation.PhoneNumberPattern; | ||
|
|
||
| public record FindPasswordRequest( | ||
| String name, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation annotation for the name field.
The name field lacks validation which could lead to security issues or data quality problems. Consider adding appropriate validation such as @NotBlank and potentially a pattern constraint.
- String name,
+ @NotBlank String name,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| String name, | |
| @NotBlank String name, |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/presentation/user/dto/FindPasswordRequest.java
at line 7, the name field lacks validation annotations. Add @NotBlank to ensure
the field is not empty or null, and consider adding a @Pattern annotation with a
suitable regex to enforce allowed characters or format for the name. Import the
necessary validation annotations from javax.validation.constraints.
| @RestController | ||
| @RequestMapping("/user") | ||
| @RequiredArgsConstructor | ||
| public class MyUserController implements MyUserControllerDocs { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add security annotations and implement authentication extraction.
The controller lacks security annotations and proper authentication handling for sensitive operations.
+import org.springframework.security.access.prepost.PreAuthorize;
+import org.springframework.security.core.Authentication;
+import org.springframework.security.core.context.SecurityContextHolder;
+@PreAuthorize("hasRole('USER')")
@RestController
@RequestMapping("/user")
@RequiredArgsConstructor
public class MyUserController implements MyUserControllerDocs {Add a helper method to extract authenticated user ID:
private Long getCurrentAuthenticatedUserId() {
Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
// Extract user ID from authentication principal
// Implementation depends on your authentication setup
}🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/presentation/user/MyUserController.java
around lines 18 to 21, the controller is missing security annotations and a
method to extract the authenticated user's ID. Add appropriate security
annotations such as @PreAuthorize or @Secured to protect sensitive endpoints.
Implement a private helper method getCurrentAuthenticatedUserId() that retrieves
the Authentication object from SecurityContextHolder and extracts the user ID
from the principal according to your authentication setup.
| @PostMapping("/me/password") | ||
| public ResponseEntity<ApiResponseWrapper<ChangePasswordResponse>> changePassword( | ||
| @RequestParam Long userId, | ||
| @RequestBody @Valid ChangePasswordRequest request | ||
| ) { | ||
| ChangePasswordResponse response = myUserService.changePassword(userId, request); | ||
|
|
||
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.OK, "비밀번호 변경 성공", response)); | ||
| } |
There was a problem hiding this comment.
Critical security concern: Implement proper authentication and authorization.
The password change endpoint accepts userId as a request parameter, which is a serious security vulnerability. Users could potentially change other users' passwords. The userId should be extracted from the authenticated user's context, not from request parameters.
@PostMapping("/me/password")
public ResponseEntity<ApiResponseWrapper<ChangePasswordResponse>> changePassword(
- @RequestParam Long userId,
@RequestBody @Valid ChangePasswordRequest request
) {
+ // Extract userId from SecurityContext/Authentication
+ Long userId = getCurrentAuthenticatedUserId();
ChangePasswordResponse response = myUserService.changePassword(userId, request);Also consider adding:
- Rate limiting to prevent brute force attacks
- Audit logging for password change events
- Additional security headers
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/presentation/user/MyUserController.java
around lines 25 to 33, remove the userId parameter from the method signature and
instead retrieve the authenticated user's ID from the security context or
authentication principal. This ensures only the logged-in user can change their
own password. Additionally, implement rate limiting on this endpoint, add audit
logging for password changes, and configure appropriate security headers in the
response to enhance protection.
| @PostMapping("/me/find-password") | ||
| public ResponseEntity<ApiResponseWrapper<Void>> findPassword( | ||
| @RequestParam Long userId, | ||
| @RequestBody @Valid FindPasswordRequest request | ||
| ) { | ||
| myUserService.findPassword(userId, request); | ||
|
|
||
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.OK, "비밀번호 찾기 요청 성공")); | ||
| } |
There was a problem hiding this comment.
Security vulnerability: Protect password recovery endpoint.
Similar to the password change endpoint, accepting userId as a parameter for password recovery is unsafe. This endpoint should also extract the user ID from the authentication context.
@PostMapping("/me/find-password")
public ResponseEntity<ApiResponseWrapper<Void>> findPassword(
- @RequestParam Long userId,
@RequestBody @Valid FindPasswordRequest request
) {
+ // Extract userId from SecurityContext or validate against authenticated user
+ Long userId = getCurrentAuthenticatedUserId();
myUserService.findPassword(userId, request);Additional security measures needed:
- Rate limiting for password recovery attempts
- CAPTCHA or similar anti-automation measures
- Audit logging for security events
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/presentation/user/MyUserController.java
lines 35 to 43, the findPassword endpoint currently accepts userId as a request
parameter, which is a security risk. Modify the method to extract the user ID
from the authenticated user's security context instead of accepting it as a
parameter. Additionally, implement rate limiting, CAPTCHA or similar
anti-automation mechanisms, and audit logging for password recovery attempts to
enhance security.
| if (user.getName() != request.name() || user.getLoginId() != request.loginId()) { | ||
| throw new CustomRuntimeException(ErrorCode.USER_INFO_INVALID); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null safety checks.
The current implementation could throw NullPointerException if user.getName(), user.getLoginId(), request.name(), or request.loginId() return null.
Consider using Objects.equals() for null-safe comparison:
+import java.util.Objects;
-if (!user.getName().equals(request.name()) || !user.getLoginId().equals(request.loginId())) {
+if (!Objects.equals(user.getName(), request.name()) || !Objects.equals(user.getLoginId(), request.loginId())) {
throw new CustomRuntimeException(ErrorCode.USER_INFO_INVALID);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/application/user/MyUserService.java around
lines 39 to 41, the current equality checks between user.getName() and
request.name(), as well as user.getLoginId() and request.loginId(), can throw
NullPointerException if any of these values are null. Replace the '!='
comparisons with Objects.equals() method calls to safely compare these values
while handling nulls without exceptions.
| @Transactional | ||
| public ChangePasswordResponse changePassword(Long userId, ChangePasswordRequest request) { | ||
| var user = userJpaRepository.findById(userId) | ||
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.USER_NOT_FOUND)); | ||
|
|
||
| user.changePassword(passwordEncode(encoder, request.newPassword())); | ||
|
|
||
| return ChangePasswordResponse.from(Boolean.TRUE); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add current password verification for security.
The changePassword method directly changes the password without verifying the user's current password. This poses a security risk if the user session is compromised.
Consider adding current password verification:
@Transactional
-public ChangePasswordResponse changePassword(Long userId, ChangePasswordRequest request) {
+public ChangePasswordResponse changePassword(Long userId, ChangePasswordRequest request) {
var user = userJpaRepository.findById(userId)
.orElseThrow(() -> new CustomRuntimeException(ErrorCode.USER_NOT_FOUND));
+ if (!encoder.matches(request.currentPassword(), user.getPassword())) {
+ throw new CustomRuntimeException(ErrorCode.INVALID_CURRENT_PASSWORD);
+ }
+
user.changePassword(passwordEncode(encoder, request.newPassword()));
return ChangePasswordResponse.from(Boolean.TRUE);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Transactional | |
| public ChangePasswordResponse changePassword(Long userId, ChangePasswordRequest request) { | |
| var user = userJpaRepository.findById(userId) | |
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.USER_NOT_FOUND)); | |
| user.changePassword(passwordEncode(encoder, request.newPassword())); | |
| return ChangePasswordResponse.from(Boolean.TRUE); | |
| } | |
| @Transactional | |
| public ChangePasswordResponse changePassword(Long userId, ChangePasswordRequest request) { | |
| var user = userJpaRepository.findById(userId) | |
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.USER_NOT_FOUND)); | |
| if (!encoder.matches(request.currentPassword(), user.getPassword())) { | |
| throw new CustomRuntimeException(ErrorCode.INVALID_CURRENT_PASSWORD); | |
| } | |
| user.changePassword(passwordEncode(encoder, request.newPassword())); | |
| return ChangePasswordResponse.from(Boolean.TRUE); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/application/user/MyUserService.java
between lines 24 and 32, the changePassword method lacks verification of the
user's current password before allowing a password change. To fix this, add a
check that compares the provided current password from the request with the
stored password using the encoder before proceeding. If the current password
does not match, throw an appropriate exception to prevent unauthorized password
changes.
| UserJpaEntity user = userJpaRepository.findById(userId) | ||
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.USER_NOT_FOUND)); | ||
|
|
||
| if (user.getName() != request.name() || user.getLoginId() != request.loginId()) { |
There was a problem hiding this comment.
Fix string comparison logic bug.
Using != for string comparison will compare object references, not content. This will cause the validation to fail even when the strings are equal.
Apply this fix to use proper string comparison:
-if (user.getName() != request.name() || user.getLoginId() != request.loginId()) {
+if (!user.getName().equals(request.name()) || !user.getLoginId().equals(request.loginId())) {
throw new CustomRuntimeException(ErrorCode.USER_INFO_INVALID);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (user.getName() != request.name() || user.getLoginId() != request.loginId()) { | |
| if (!user.getName().equals(request.name()) || !user.getLoginId().equals(request.loginId())) { | |
| throw new CustomRuntimeException(ErrorCode.USER_INFO_INVALID); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/application/user/MyUserService.java at
line 39, the code uses '!=' to compare strings, which compares references
instead of content. Replace the '!=' comparisons with calls to the .equals()
method for proper string content comparison, ensuring the validation logic
correctly compares the string values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/life/mosu/mosuserver/global/annotation/PasswordPattern.java (1)
12-12: Consider expanding the special character set for better user experience.The current regex limits special characters to
[@$!%*?&], which might be too restrictive for users. Consider including commonly used special characters like-,_,+,=,.,,to improve user experience while maintaining security.-@Pattern(regexp = "^(?=.*[a-z])(?=.*[A-Z])(?=.*\\d)(?=.*[@$!%*?&])[A-Za-z\\d@$!%*?&]{8,20}$", message = "비밀번호 형식이 올바르지 않습니다.") +@Pattern(regexp = "^(?=.*[a-z])(?=.*[A-Z])(?=.*\\d)(?=.*[@$!%*?&_\\-+=.,])[A-Za-z\\d@$!%*?&_\\-+=.,]{8,20}$", message = "비밀번호 형식이 올바르지 않습니다.")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/life/mosu/mosuserver/global/annotation/PasswordPattern.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/dto/LoginRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/dto/SignUpRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/user/dto/ChangePasswordRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/user/dto/FindPasswordRequest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/life/mosu/mosuserver/presentation/user/dto/FindPasswordRequest.java
- src/main/java/life/mosu/mosuserver/presentation/user/dto/ChangePasswordRequest.java
- src/main/java/life/mosu/mosuserver/presentation/auth/dto/LoginRequest.java
- src/main/java/life/mosu/mosuserver/presentation/auth/dto/SignUpRequest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (2)
src/main/java/life/mosu/mosuserver/global/annotation/PasswordPattern.java (2)
5-5: Excellent addition of @notblank constraint!Adding
@NotBlankalongside@Patternis a best practice that ensures password fields cannot be null, empty, or contain only whitespace characters, which@Patternalone might not catch in all scenarios.Also applies to: 13-13
12-12: Password regex validated – no changes requiredAll existing test cases passed as expected. The pattern correctly enforces at least one lowercase, one uppercase, one digit, and one of the allowed special characters, within 8–20 characters.
Optional improvement:
- Add boundary tests for exact lengths:
- Exactly 8 characters with minimal requirements (e.g.,
"aA1@aaaa") → should pass- Exactly 20 characters with minimal requirements (e.g.,
"Ab1@Ab1@Ab1@Ab1@Ab1@") → should pass
1 participant
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Chores