Conversation
# Conflicts: # src/main/java/life/mosu/mosuserver/global/filter/Whitelist.java # src/main/java/life/mosu/mosuserver/presentation/user/UserController.java
MOSU-270 feat: 회원가입 시 전화번호로 중복 사용자 검증
MOSU-273 feat: 등록된 전화번호 확인 api 구현
![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!
I've implemented two key features in this pull request: the ability to inquire about refund amounts for payments and enhanced phone number verification logic. The refund inquiry provides a new API endpoint for users to check potential refunds, while the phone number verification ensures uniqueness during user registration and streamlines OAuth user identification by prioritizing phone numbers.
Highlights
- Refund Amount Inquiry Functionality: I've added a new feature that allows users to inquire about the refundable amount for a specific payment. This includes a new service method and a corresponding REST API endpoint.
- Phone Number Verification and Uniqueness: I've implemented logic to verify phone numbers during user registration and OAuth processes. This includes checking for existing phone numbers, adding a new API endpoint for phone number availability checks, and updating user persistence logic to prioritize phone numbers for OAuth users.
- DTO Package Refactoring: I've refactored the
IsLoginIdAvailableResponseDTO by moving it from therequestpackage to theresponsepackage, aligning with common API design patterns.
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 or fill out our survey 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 a feature to query refund amounts and enhances user validation by checking for phone number uniqueness. The implementation is mostly correct, but I've identified a few significant issues related to security and error handling.
Specifically:
- A critical security issue in
Whitelist.javaexposes an authenticated endpoint. - The new refund amount endpoint in
RefundController.javalacks authorization checks, leading to potential information leakage. - A logic change in
OAuthUserPersistenceProcessor.javacould lead to unhandled exceptions during the OAuth sign-in process.
Addressing these points is crucial for maintaining the application's security and stability. Other changes look good.
| USER_ID_CHECK("/api/v1/user/check-id", WhitelistMethod.GET), | ||
| USER_PHONE_NUMBER_CHECK("/api/v1/user/check-phone-number", WhitelistMethod.GET), | ||
|
|
||
| CUSTOMER_KEY_CHECK("/api/v1/user/customer-key", WhitelistMethod.GET), |
There was a problem hiding this comment.
The endpoint /api/v1/user/customer-key has been added to the whitelist, making it publicly accessible without authentication. However, the corresponding controller method UserController.getCustomerKey is protected with @PreAuthorize("isAuthenticated() and hasRole('USER')"), which requires an authenticated user. This creates a security vulnerability. If the authentication filter is bypassed, the security context will be empty, and the @PreAuthorize check will fail, likely causing an exception. This line should be removed if the endpoint is intended to be protected.
| @Transactional | ||
| public UserJpaEntity process(final OAuthUserInfo info) { | ||
| return userRepository.findByLoginId(info.email()) | ||
| return userRepository.findByPhoneNumber(info.phoneNumber()) |
There was a problem hiding this comment.
Changing the user lookup from loginId (email) to phoneNumber introduces a potential issue in the orElseGet block (lines 31-45). If no user is found by phone number, the code proceeds to create a new user using info.email() as the loginId. However, it doesn't check if a user with that loginId already exists. This can lead to an unhandled DataIntegrityViolationException if the email is already in use by another account (e.g., one created via standard signup). This should be handled gracefully to avoid a 500 error.
| @GetMapping() | ||
| ResponseEntity<ApiResponseWrapper<RefundAmountResponse>> getRefundAmount( | ||
| @RequestParam(required = true) String paymentKey, | ||
| @RequestParam(required = true) Long examApplicationId) { | ||
| var response = refundService.getRefundAmount(paymentKey, examApplicationId); | ||
| return ResponseEntity.ok( | ||
| ApiResponseWrapper.success(HttpStatus.OK, "환불 금액 조회 성공", response)); | ||
| } |
There was a problem hiding this comment.
The new getRefundAmount endpoint is not protected by any authentication or authorization mechanism. This means anyone who knows a valid paymentKey and examApplicationId can query the refund amount. This could lead to information leakage. It is recommended to secure this endpoint, for example by adding @PreAuthorize("isAuthenticated()") and verifying that the authenticated user is authorized to view this information.
2 participants
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타