feat: enhance user sign-up process with logging and phone number retr… #327
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 @polyglot-k, 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 enhances the user sign-up process by improving logging capabilities and addressing a potential issue with phone number validation. The changes introduce detailed logging at key stages of the sign-up flow and ensure that phone number existence checks are performed against the original, unformatted phone number, thereby increasing the robustness and observability of the sign-up mechanism.
Highlights
- Enhanced Logging: Logging has been integrated into the SignUpService and SignUpAccountStepProcessor classes using Lombok's @slf4j annotation and log.info statements. This provides better visibility into the user sign-up flow, allowing for easier debugging and monitoring.
- Phone Number Validation Fix: The phone number validation logic in SignUpAccountStepProcessor was updated to use a new getOriginPhoneNumber() method from UserJpaEntity. This change ensures that the unformatted, original phone number is used for existence checks, preventing potential issues with formatted phone numbers causing incorrect validation.
- User Entity Update for Phone Number Retrieval and Logging: A new method, getOriginPhoneNumber(), was added to the UserJpaEntity to expose the raw phone number. Additionally, the toString() method was overridden to provide a more comprehensive representation of the UserJpaEntity, which aids in debugging and logging.
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 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
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 enhances the user sign-up process by adding logging and fixing an issue with phone number validation. While the functional fix for phone number retrieval seems correct, the introduction of logging has unfortunately created a critical security vulnerability by exposing user passwords in logs via the new toString() method. Additionally, there are opportunities to improve error handling by using more specific error codes and to enhance code clarity through better naming conventions and more descriptive log messages. Addressing these points will significantly improve the security and maintainability of the code.
| public String toString() { | ||
| return "UserJpaEntity{" + | ||
| "id=" + id + | ||
| ", loginId='" + loginId + '\'' + | ||
| ", password='" + password + '\'' + | ||
| ", gender=" + gender + | ||
| ", name='" + name + '\'' + | ||
| ", birth=" + birth + | ||
| ", phoneNumber='" + phoneNumber + '\'' + | ||
| ", customerKey='" + customerKey + '\'' + | ||
| ", agreedToMarketing=" + agreedToMarketing + | ||
| ", userRole=" + userRole + | ||
| ", provider=" + provider + | ||
| '}'; | ||
| } |
There was a problem hiding this comment.
The current implementation of toString() includes the user's password. This is a critical security risk, as it will expose sensitive user data in the logs whenever this object is logged. Passwords and other sensitive information should never be written to logs in plain text. Please mask the password field.
@Override
public String toString() {
return "UserJpaEntity{" +
"id=" + id +
", loginId='" + loginId + '\'' +
", password='[PROTECTED]'" +
", gender=" + gender +
", name='" + name + '\'' +
", birth=" + birth +
", phoneNumber='" + phoneNumber + '\'' +
", customerKey='" + customerKey + '\'' +
", agreedToMarketing=" + agreedToMarketing +
", userRole=" + userRole +
", provider=" + provider +
'}';
}| if (userRepository.existsByPhoneNumber(user.getOriginPhoneNumber())) { | ||
| throw new CustomRuntimeException(ErrorCode.USER_ALREADY_EXISTS); | ||
| } else if (userRepository.existsByLoginId(user.getLoginId())) { | ||
| throw new CustomRuntimeException(ErrorCode.USER_ALREADY_EXISTS); | ||
| } |
There was a problem hiding this comment.
Both existence checks for phone number and login ID throw the same generic USER_ALREADY_EXISTS error. This prevents the client from knowing which specific field caused the conflict. To provide better feedback, you should use distinct error codes for each case. Consider adding PHONE_NUMBER_ALREADY_EXISTS and LOGIN_ID_ALREADY_EXISTS to your ErrorCode enum and using them here.
|
|
||
| private UserJpaEntity doAccountStep(SignUpAccountRequest request) { | ||
| UserJpaEntity user = request.toAuthEntity(passwordEncoder); | ||
| log.info("log : {}", user); |
There was a problem hiding this comment.
The log message log : {} is not very descriptive. Good log messages should provide context about what is being logged. This helps with debugging and monitoring. Please provide a more meaningful message.
| log.info("log : {}", user); | |
| log.info("User entity created from sign-up request: {}", user); |
| public String getOriginPhoneNumber() { | ||
| return phoneNumber; | ||
| } |
There was a problem hiding this comment.
The addition of getOriginPhoneNumber() was necessary because the existing getPhoneNumber() method contains logic that modifies the value, which is not what a standard getter should do. This can be confusing and error-prone. While this change works, for better long-term maintainability, consider refactoring this. A better approach would be to have getPhoneNumber() return the raw phoneNumber field, and move the transformation logic to a new method with a more descriptive name, like getFormattedPhoneNumber() or getPhoneNumberForDisplay(). This would make the code more intuitive and align with standard Java practices.
…ieval fix
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타