MOSU-165 feat: OAuth Auth 회원가입 로직 병합#167
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis change introduces new user profile retrieval features, adds and modifies endpoints for user information and certification, and reorganizes DTO package structures. It also updates OAuth user info extraction to use hardcoded values and enhances the sign-up request with additional required fields and validation. Several classes are updated to reflect the new DTO structure. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UserController
participant UserService
participant UserRepository
participant UserInfoResponse
Client->>UserController: GET /user/info (with userId)
UserController->>UserService: getUserInfo(userId)
UserService->>UserRepository: findById(userId)
UserRepository-->>UserService: UserJpaEntity or not found
UserService->>UserInfoResponse: from(UserJpaEntity)
UserInfoResponse-->>UserService: UserInfoResponse
UserService-->>UserController: UserInfoResponse
UserController-->>Client: 200 OK + ApiResponseWrapper<UserInfoResponse>
sequenceDiagram
participant Client
participant MasterController
participant OneTimeTokenProvider
participant KmcEventTxService
Client->>MasterController: GET /master/kmc
MasterController->>OneTimeTokenProvider: create(certNumber)
OneTimeTokenProvider-->>MasterController: token
MasterController->>KmcEventTxService: publishIssueEvent(certNumber, token, expire)
alt Success
MasterController-->>Client: 200 OK + token
else Failure
MasterController->>KmcEventTxService: publishFailEvent(certNumber)
MasterController-->>Client: Exception thrown
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested labels
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (18)
✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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 @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 primarily focuses on enhancing the user registration and profile management system. It expands the user signup request to include more personal details, introduces a new flow for KMC-based identity verification, and provides an endpoint for retrieving user profile information, particularly for users in a 'PENDING' state. Additionally, it includes significant refactoring of DTOs into separate request and response packages to improve code structure.
Highlights
- Enhanced User Registration DTO: The
SignUpAccountRequestDTO has been significantly expanded to include more detailed personal information such asuserName,birth,gender, andphoneNumber. This change likely supports a more comprehensive user profile or integration with identity verification processes. Note thatagreedToTermsOfServiceandagreedToPrivacyPolicyare now hardcoded totruein thetoAuthEntitymethod, which might be a temporary measure. - New KMC Signup Flow: A new API endpoint,
GET /master/kmc, has been introduced inMasterController. This endpoint initiates a KMC (Korean Mobile Certification) based signup flow, involving the generation of a one-time token and publishing KMC events. This suggests a new identity verification step in the user registration process. - User Information Retrieval for Pending Users: A new service method
getUserInfo(Long userId)has been added toUserService, along with a corresponding API endpointGET /users/infoinUserController. This endpoint is secured with@PreAuthorize("isAuthenticated() and hasRole('PENDING')"), indicating that it's designed to retrieve user profile information specifically for users who are in a 'PENDING' registration state, suggesting a multi-step or staged registration process. - DTO Package Refactoring: A major refactoring of the DTO package structure has been performed. DTOs are now separated into dedicated
requestandresponsesubpackages withinpresentation.user.dtofor improved organization and clarity. This involved renaming several existing DTO files and updating their import paths across various services and controllers.
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 merges the OAuth and standard user sign-up flows, refactors DTOs into request/response packages, and adds new endpoints for KMC authentication and fetching user information.
The review identified several critical issues that need to be addressed before merging:
- Hardcoded test data in the OAuth flow which will break Kakao login.
- Bypassing user consent for terms of service by hardcoding agreement values.
- A security regression in cookie handling that makes tokens less secure.
- A bug where new fields in the sign-up request are ignored.
Additionally, there are suggestions for improving code maintainability by reducing duplication, improving error handling, and fixing an incorrectly packaged DTO.
| .name("김모수") | ||
| .phoneNumber("010-1234-5678") | ||
| .birthDay(LocalDate.EPOCH) | ||
| .email("test123@gmali.com") |
There was a problem hiding this comment.
These user details are hardcoded. This appears to be for testing purposes and must be reverted before merging to production. These values should be dynamically retrieved from the OAuth provider's response.
For example:
nameshould come from theprofileobject (as it was before).phoneNumber,birthDay, andemailshould be extracted from thekakaoAccountobject.
Also, the email test123@gmali.com seems to have a typo and should probably be gmail.com.
| headers.add(HttpHeaders.SET_COOKIE, CookieBuilderUtil.createCookie( | ||
| ACCESS_TOKEN_COOKIE_NAME, | ||
| token.accessToken(), | ||
| token.accessTokenExpireTime() | ||
| )); | ||
| headers.add(HttpHeaders.SET_COOKIE, CookieBuilderUtil.temporaryCookieString( | ||
| headers.add(HttpHeaders.SET_COOKIE, CookieBuilderUtil.createCookie( | ||
| REFRESH_TOKEN_COOKIE_NAME, | ||
| token.refreshToken(), | ||
| token.refreshTokenExpireTime() |
There was a problem hiding this comment.
The switch from CookieBuilderUtil.temporaryCookieString to CookieBuilderUtil.createCookie significantly reduces the security of the cookies being set.
createCookie sets secure(false) and doesn't specify domain or SameSite attributes. This means the access and refresh tokens could be sent over unencrypted HTTP connections, and cross-domain behavior might be broken or less secure.
For sensitive tokens, it's crucial to use secure(true). The temporaryCookieString method seems more appropriate for production environments. If this change was not intentional, it should be reverted.
| headers.add(HttpHeaders.SET_COOKIE, CookieBuilderUtil.createCookie( | |
| ACCESS_TOKEN_COOKIE_NAME, | |
| token.accessToken(), | |
| token.accessTokenExpireTime() | |
| )); | |
| headers.add(HttpHeaders.SET_COOKIE, CookieBuilderUtil.temporaryCookieString( | |
| headers.add(HttpHeaders.SET_COOKIE, CookieBuilderUtil.createCookie( | |
| REFRESH_TOKEN_COOKIE_NAME, | |
| token.refreshToken(), | |
| token.refreshTokenExpireTime() | |
| headers.add(HttpHeaders.SET_COOKIE, CookieBuilderUtil.temporaryCookieString( | |
| ACCESS_TOKEN_COOKIE_NAME, | |
| token.accessToken(), | |
| token.accessTokenExpireTime() | |
| )); | |
| headers.add(HttpHeaders.SET_COOKIE, CookieBuilderUtil.temporaryCookieString( | |
| REFRESH_TOKEN_COOKIE_NAME, | |
| token.refreshToken(), | |
| token.refreshTokenExpireTime() | |
| )); |
| } catch (Exception ex) { | ||
| eventTxService.publishFailureEvent(certNum); | ||
| throw new RuntimeException("KMC 인증 요청 생성에 실패했습니다.", ex); | ||
| } |
There was a problem hiding this comment.
Catching a generic Exception is discouraged as it can hide the actual problems and catch unexpected runtime exceptions like NullPointerException. It's better to catch more specific exceptions that publishIssueEvent might throw.
Additionally, instead of throwing a generic RuntimeException, consider using the project's CustomRuntimeException with a dedicated ErrorCode. This will ensure consistent error handling and response formatting across the application.
Lastly, the success message "프로필 등록 성공" seems misleading. This endpoint appears to initiate an authentication process, so a message like "KMC 인증 요청 생성 성공" would be more accurate.
| UserJpaEntity user = userJpaRepository.findById(userId) | ||
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.USER_NOT_FOUND)); | ||
|
|
||
| return UserInfoResponse.from(user); |
There was a problem hiding this comment.
There's an opportunity to reduce code duplication here. The logic to find a user by ID or throw an exception is already encapsulated in the getUserOrThrow(Long userId) method. You can reuse it here to make the code more concise and maintainable.
| UserJpaEntity user = userJpaRepository.findById(userId) | |
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.USER_NOT_FOUND)); | |
| return UserInfoResponse.from(user); | |
| UserJpaEntity user = getUserOrThrow(userId); | |
| return UserInfoResponse.from(user); |
| @@ -1,4 +1,4 @@ | |||
| package life.mosu.mosuserver.presentation.user.dto; | |||
| package life.mosu.mosuserver.presentation.user.dto.request; | |||
There was a problem hiding this comment.
The IsLoginIdAvailableResponse class is used as a response body for an endpoint. According to the new package structure refactoring in this PR, it should be in the ...dto.response package, not ...dto.request. Please move it to the correct package for consistency.
| package life.mosu.mosuserver.presentation.user.dto.request; | |
| package life.mosu.mosuserver.presentation.user.dto.response; |
1 participant
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation