Conversation
Co-authored-by: jbh010204 <jbh010204@naver.com>
WalkthroughThis set of changes refactors authentication and permission handling in inquiry-related services and controllers, removes the access token reissue endpoint and its documentation, and centralizes endpoint skipping logic in the access token filter. It also introduces a new error code for invalid sign-up tokens and updates the sign-up token validation flow to throw custom exceptions. Additionally, profile registration logic and related classes were removed or simplified, and user entity update methods were extended to include phone number handling. Minor DTO and import adjustments were made accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant InquiryController
participant InquiryService
Client->>InquiryController: create(request, PrincipalDetails)
InquiryController->>InquiryService: createInquiry(user, request)
InquiryService->>InquiryService: hasPermission(user, inquiry)
InquiryService-->>InquiryController: (success or error)
InquiryController-->>Client: Response
sequenceDiagram
participant Client
participant AccessTokenFilter
Client->>AccessTokenFilter: HTTP Request
AccessTokenFilter->>AccessTokenFilter: Check skippedUrlPrefixes
alt Skipped
AccessTokenFilter-->>Client: Proceed without token validation
else Not Skipped
AccessTokenFilter->>AccessTokenFilter: Validate token
AccessTokenFilter-->>Client: Proceed or error
end
sequenceDiagram
participant Client
participant SignUpTokenService
Client->>SignUpTokenService: validateSignUpToken(token)
alt Token is null
SignUpTokenService-->>Client: Throw NOT_FOUND_SIGN_UP_TOKEN
else Token not found
SignUpTokenService-->>Client: Throw INVALID_SIGN_UP_TOKEN
else Token valid
SignUpTokenService->>SignUpTokenService: Delete token
SignUpTokenService-->>Client: (void)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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 authorization mechanisms within the application, particularly for inquiry management. It refactors how user permissions are checked by passing UserJpaEntity directly to service methods and introducing a centralized permission check. Additionally, the token filtering logic has been streamlined to improve clarity and maintainability, and error handling for sign-up tokens has been made more granular.
Highlights
- Authorization Enhancement: Implemented a new
hasPermissionutility inInquiryServiceto centralize access control for inquiry creation and deletion, ensuring only the post owner or an ADMIN can perform these actions. This refactors how user permissions are checked by passingUserJpaEntitydirectly to service methods. - API Contract Update: Modified
InquiryControllerand its documentation to passPrincipalDetails(containingUserJpaEntity) directly to service methods for authorization, replacing the previoususerIdparameter and removing the custom@UserIdannotation. - Token Filter Refinement: The
AccessTokenFilternow uses a predefined list ofskippedUrlPrefixesto bypass token validation for specific public or authentication-related endpoints (e.g., login, logout, OAuth, Swagger/API docs), simplifying the filter logic and removing hardcoded endpoint checks and thereissueEndpointconfiguration. - Sign-Up Token Validation Improvement: The
SignUpTokenServicenow uses more specificCustomRuntimeExceptionandErrorCodefor token validation failures, providing clearer error messages and improving robustness, and no longer returns theSignUpTokenobject after validation.
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
The pull request enhances authorization by integrating Spring Security's PrincipalDetails and refactoring the AccessTokenFilter. The changes improve code clarity and security. The review suggests improvements such as reordering the permission check in InquiryService, hiding PrincipalDetails from Swagger documentation, correcting a syntax issue in ErrorCode, updating the Javadoc in SignUpTokenService, and extracting the signup URL to a constant in AccessTokenFilter.
| REFUND_CALCULATION_FAILED(HttpStatus.CONFLICT, "환불 금액 계산에 실패하였습니다."), | ||
| ; |
There was a problem hiding this comment.
The last enum value REFUND_CALCULATION_FAILED should not have a comma after it. This is a syntax issue that could cause problems.
| REFUND_CALCULATION_FAILED(HttpStatus.CONFLICT, "환불 금액 계산에 실패하였습니다."), | |
| ; | |
| REFUND_CALCULATION_FAILED(HttpStatus.CONFLICT, "환불 금액 계산에 실패하였습니다.") | |
| ; |
| public void createInquiry(UserJpaEntity user, InquiryCreateRequest request) { | ||
| InquiryJpaEntity inquiryEntity = inquiryJpaRepository.save(request.toEntity(user)); | ||
|
|
||
| hasPermission(inquiryEntity.getUserId(), user); |
There was a problem hiding this comment.
The hasPermission check is performed after saving the inquiry. It's more appropriate to perform this check before saving to prevent unnecessary database operations if the user lacks permission. This can improve efficiency and prevent potential data inconsistencies.
| PrincipalDetails principalDetails, | ||
| @Parameter(name = "postId", description = "삭제할 문의의 ID", in = ParameterIn.PATH) | ||
| @PathVariable Long postId | ||
| ); |
There was a problem hiding this comment.
The principalDetails parameter in the deleteInquiry method should be hidden from the API documentation because it's resolved by Spring Security and not provided directly by the client. Hiding it will prevent confusion for API consumers.
| PrincipalDetails principalDetails, | |
| @Parameter(name = "postId", description = "삭제할 문의의 ID", in = ParameterIn.PATH) | |
| @PathVariable Long postId | |
| ); | |
| @Parameter(hidden = true) PrincipalDetails principalDetails, | |
| @Parameter(name = "postId", description = "삭제할 문의의 ID", in = ParameterIn.PATH) | |
| @PathVariable Long postId |
| * | ||
| * @param token 검증할 토큰 | ||
| * @return 유효한 SignUpToken 객체 | ||
| * @throws AuthenticationException 토큰이 유효하지 않을 경우 |
There was a problem hiding this comment.
The @throws tag should refer to CustomRuntimeException instead of AuthenticationException to accurately reflect the exceptions thrown by this method.
| * @throws AuthenticationException 토큰이 유효하지 않을 경우 | |
| * @throws CustomRuntimeException 토큰이 유효하지 않을 경우 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/life/mosu/mosuserver/presentation/oauth/SignUpTokenService.java (1)
19-25: Update JavaDoc to match the new method signature.The method signature correctly changed to
voidfor a validation method, but the JavaDoc still mentions returning aSignUpTokenobject, which is now inconsistent./** - * 회원가입 토큰을 검증하고, 유효하면 SignUpToken 객체를 반환합니다. + * 회원가입 토큰을 검증합니다. 유효하지 않으면 예외를 발생시킵니다. * * @param token 검증할 토큰 - * @throws AuthenticationException 토큰이 유효하지 않을 경우 + * @throws CustomRuntimeException 토큰이 유효하지 않거나 찾을 수 없을 경우 */src/main/java/life/mosu/mosuserver/application/inquiry/InquiryService.java (1)
9-9: Consider adding logging statements or remove unused @slf4j.The
@Slf4jannotation andUserRoleimport were added, but no logging statements are visible in the provided code. If logging isn't being used, consider removing the annotation to avoid unused dependencies.Also applies to: 17-17, 24-24
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/java/life/mosu/mosuserver/application/inquiry/InquiryService.java(4 hunks)src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java(2 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/AuthControllerDocs.java(0 hunks)src/main/java/life/mosu/mosuserver/presentation/inquiry/InquiryController.java(5 hunks)src/main/java/life/mosu/mosuserver/presentation/inquiry/InquiryControllerDocs.java(4 hunks)src/main/java/life/mosu/mosuserver/presentation/oauth/AccessTokenFilter.java(4 hunks)src/main/java/life/mosu/mosuserver/presentation/oauth/SignUpTokenService.java(1 hunks)src/main/resources/security-config.yml(0 hunks)
💤 Files with no reviewable changes (2)
- src/main/resources/security-config.yml
- src/main/java/life/mosu/mosuserver/presentation/auth/AuthControllerDocs.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/life/mosu/mosuserver/presentation/oauth/SignUpTokenService.java (1)
src/main/java/life/mosu/mosuserver/application/auth/provider/OneTimeTokenProvider.java (1)
Service(16-66)
🔇 Additional comments (16)
src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java (2)
30-31: LGTM! Well-structured error code addition.The new
INVALID_SIGN_UP_TOKENerror code follows existing conventions with appropriate HTTP status and clear Korean error message. It's properly placed in the sign-up token error section.
137-137: Good syntax cleanup.Removing the trailing comma before the semicolon improves code consistency and follows proper Java enum syntax conventions.
src/main/java/life/mosu/mosuserver/presentation/oauth/SignUpTokenService.java (3)
7-8: LGTM! Proper imports for enhanced exception handling.The added imports support the transition to using
CustomRuntimeExceptionwith specific error codes, improving error handling consistency across the application.
26-28: Excellent defensive programming with proper error handling.The explicit null check prevents potential
NullPointerExceptionand provides clear error messaging using the appropriateNOT_FOUND_SIGN_UP_TOKENerror code.
32-34: LGTM! Improved exception handling with specific error codes.The change from
AuthenticationExceptiontoCustomRuntimeExceptionwith the specificINVALID_SIGN_UP_TOKENerror code provides better error specificity and aligns with the application's centralized exception handling approach.src/main/java/life/mosu/mosuserver/presentation/oauth/AccessTokenFilter.java (4)
10-10: LGTM! Required import for the refactored URL handling.The
Listimport is necessary to support the new consolidated approach for handling skipped URL prefixes.
24-33: Excellent refactoring! Centralized and maintainable URL configuration.Consolidating all skipped URL prefixes into a single immutable list significantly improves maintainability and makes it easier to manage which endpoints bypass token validation. The use of
List.of()is appropriate for static configuration.
47-56: Excellent code simplification with improved readability.The replacement of multiple conditional checks with a single stream-based approach using
anyMatchandstartsWithmaintains the same functionality while significantly improving code readability and maintainability. The added logging is also helpful for debugging.
104-104: Minor formatting improvement.The added blank line improves code readability by providing visual separation between the conditional logic and the return statement.
src/main/java/life/mosu/mosuserver/presentation/inquiry/InquiryControllerDocs.java (2)
12-12: LGTM: Import updates align with authorization enhancement.The addition of
PrincipalDetailsand@AuthenticationPrincipalimports properly supports the shift from custom user ID extraction to Spring Security's standard authentication principal approach.Also applies to: 23-23
36-36: LGTM: Consistent use of @AuthenticationPrincipal annotation.The create method properly uses
@AuthenticationPrincipal PrincipalDetailswhich aligns with Spring Security best practices for accessing authenticated user information.src/main/java/life/mosu/mosuserver/application/inquiry/InquiryService.java (2)
61-63: LGTM: Proper permission check placement for deletion.The permission validation is correctly placed before the deletion operation, preventing unauthorized access and avoiding unnecessary processing.
88-92: LGTM: Well-implemented centralized permission check.The
hasPermissionmethod properly implements role-based and ownership-based authorization logic, checking both admin privileges and resource ownership. The use of appropriate error codes enhances the user experience.src/main/java/life/mosu/mosuserver/presentation/inquiry/InquiryController.java (3)
4-4: LGTM: Proper imports for Spring Security integration.The addition of
PrincipalDetailsand@AuthenticationPrincipalimports correctly supports the authorization enhancement by integrating with Spring Security's authentication context.Also applies to: 21-21
43-45: LGTM: Consistent authentication principal usage.The controller properly extracts the user entity from
PrincipalDetailsand passes it to the service layer, maintaining the authentication context throughout the call chain.
74-80: LGTM: Well-structured method with proper authorization flow.The
deleteInquirymethod correctly:
- Uses
@AuthenticationPrincipalfor consistent authentication handling- Extracts the user entity from
PrincipalDetails- Maintains the
@PreAuthorizeannotation for layered security- Passes the full user context to the service layer
| public void createInquiry(UserJpaEntity user, InquiryCreateRequest request) { | ||
| InquiryJpaEntity inquiryEntity = inquiryJpaRepository.save(request.toEntity(user)); | ||
|
|
||
| hasPermission(inquiryEntity.getUserId(), user); | ||
| inquiryAttachmentService.createAttachment(request.attachments(), inquiryEntity); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move permission check before inquiry creation for efficiency.
The permission check occurs after saving the inquiry entity, which could result in unnecessary database writes if the user lacks permission. Consider restructuring to validate permissions before persistence.
Apply this diff to move the permission check:
public void createInquiry(UserJpaEntity user, InquiryCreateRequest request) {
+ // Pre-validate user has permission to create inquiries for themselves
+ // (This assumes the request contains user context or we validate current user can create)
InquiryJpaEntity inquiryEntity = inquiryJpaRepository.save(request.toEntity(user));
-
- hasPermission(inquiryEntity.getUserId(), user);
inquiryAttachmentService.createAttachment(request.attachments(), inquiryEntity);
}Note: Since the user is creating their own inquiry, the permission check may be redundant here unless there are specific business rules about inquiry creation.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/application/inquiry/InquiryService.java
between lines 36 and 41, the permission check is currently performed after
saving the inquiry entity, which can cause unnecessary database writes if the
user lacks permission. To fix this, move the hasPermission call to before the
inquiryJpaRepository.save invocation to validate permissions prior to
persistence. This ensures that the inquiry is only saved if the user has the
appropriate permissions.
| @ApiResponse(responseCode = "200", description = "질문 삭제 성공") | ||
| }) | ||
| ResponseEntity<ApiResponseWrapper<Void>> deleteInquiry( | ||
| PrincipalDetails principalDetails, |
There was a problem hiding this comment.
Add missing @AuthenticationPrincipal annotation for consistency.
The deleteInquiry method signature uses PrincipalDetails but is missing the @AuthenticationPrincipal annotation, unlike the create method. This inconsistency could cause confusion and potential runtime issues.
Apply this diff to fix the inconsistency:
- ResponseEntity<ApiResponseWrapper<Void>> deleteInquiry(
- PrincipalDetails principalDetails,
+ ResponseEntity<ApiResponseWrapper<Void>> deleteInquiry(
+ @AuthenticationPrincipal PrincipalDetails principalDetails,📝 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.
| PrincipalDetails principalDetails, | |
| ResponseEntity<ApiResponseWrapper<Void>> deleteInquiry( | |
| @AuthenticationPrincipal PrincipalDetails principalDetails, |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/presentation/inquiry/InquiryControllerDocs.java
at line 69, the method parameter PrincipalDetails is missing the
@AuthenticationPrincipal annotation. Add the @AuthenticationPrincipal annotation
before PrincipalDetails to maintain consistency with the create method and avoid
potential runtime issues.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/life/mosu/mosuserver/application/auth/AuthService.java (1)
46-53: Refactor for improved readability and conciseness.The current if-else-if structure can be simplified. The final
return Boolean.FALSEis redundant since the else-if already handles the ROLE_PENDING case.Consider this more concise implementation:
private Boolean isProfileRegistered(UserJpaEntity user) { - if (user.getUserRole() == UserRole.ROLE_USER) { - return Boolean.TRUE; - } else if (user.getUserRole() == UserRole.ROLE_PENDING) { - return Boolean.FALSE; - } - return Boolean.FALSE; + return user.getUserRole() == UserRole.ROLE_USER; }This simplification assumes that only
ROLE_USERindicates a registered profile, while all other roles (includingROLE_PENDING) indicate an unregistered profile.src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserService.java (1)
61-61: TODO comment indicates incomplete implementation.The TODO comment suggests that handling of null Kakao information and service term approval needs further implementation. This could lead to issues in production.
Would you like me to help implement the null value handling logic or create an issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/main/java/life/mosu/mosuserver/application/auth/AuthService.java(3 hunks)src/main/java/life/mosu/mosuserver/application/auth/SignUpService.java(0 hunks)src/main/java/life/mosu/mosuserver/application/auth/processor/SignUpProfileStepProcessor.java(0 hunks)src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserInfo.java(2 hunks)src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/profile/ProfileService.java(3 hunks)src/main/java/life/mosu/mosuserver/application/user/UserService.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/user/UserJpaEntity.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/AuthController.java(0 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/AuthControllerDocs.java(0 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/dto/SignUpRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/oauth/AccessTokenFilter.java(4 hunks)src/main/java/life/mosu/mosuserver/presentation/profile/ProfileController.java(2 hunks)src/main/java/life/mosu/mosuserver/presentation/profile/ProfileControllerDocs.java(2 hunks)src/main/java/life/mosu/mosuserver/presentation/profile/dto/ProfileRequest.java(0 hunks)src/main/java/life/mosu/mosuserver/presentation/profile/dto/SignUpProfileRequest.java(1 hunks)
💤 Files with no reviewable changes (5)
- src/main/java/life/mosu/mosuserver/presentation/auth/AuthController.java
- src/main/java/life/mosu/mosuserver/presentation/auth/AuthControllerDocs.java
- src/main/java/life/mosu/mosuserver/application/auth/SignUpService.java
- src/main/java/life/mosu/mosuserver/application/auth/processor/SignUpProfileStepProcessor.java
- src/main/java/life/mosu/mosuserver/presentation/profile/dto/ProfileRequest.java
✅ Files skipped from review due to trivial changes (2)
- src/main/java/life/mosu/mosuserver/presentation/auth/dto/SignUpRequest.java
- src/main/java/life/mosu/mosuserver/presentation/profile/dto/SignUpProfileRequest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/life/mosu/mosuserver/presentation/oauth/AccessTokenFilter.java
🔇 Additional comments (17)
src/main/java/life/mosu/mosuserver/application/user/UserService.java (1)
34-35: Confirmed signature match in UserJpaEntity – LGTMVerified that
UserJpaEntity.updateUserInfo(Gender, String, String, LocalDate)exactly matches the call inUserService(gender, userName, phoneNumber, birth). The new phone number parameter is correctly placed and typed.src/main/java/life/mosu/mosuserver/presentation/profile/ProfileController.java (2)
9-9: LGTM! Import updated for DTO refactoring.Clean import change from
ProfileRequesttoSignUpProfileRequestas part of the profile registration flow simplification.
34-34: LGTM! Parameter type updated consistently.The method parameter type is correctly updated to match the new import. The change maintains consistency with the DTO refactoring.
src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserInfo.java (2)
9-11: LGTM! Logging capability added.The
@Slf4jannotation is properly added to enable logging functionality for OAuth user info processing.
17-17: LGTM! Phone number field added to record.The
phoneNumberfield is correctly added to the record structure to support enhanced user information.src/main/java/life/mosu/mosuserver/presentation/profile/ProfileControllerDocs.java (2)
16-16: LGTM! Documentation import updated consistently.The import change from
ProfileRequesttoSignUpProfileRequestcorrectly aligns with the controller refactoring.
29-29: LGTM! API documentation parameter updated.The method signature in the documentation interface is correctly updated to match the controller implementation.
src/main/java/life/mosu/mosuserver/domain/user/UserJpaEntity.java (2)
90-100: LGTM! OAuth user update method enhanced with phone number support.The
updateOAuthUsermethod signature and implementation are correctly updated to include phone number handling. The parameter order is consistent and the field assignment is proper.
102-112: LGTM! User info update method enhanced with phone number support.The
updateUserInfomethod signature and implementation are correctly updated to include phone number handling. The parameter order matches the OAuth update method, maintaining consistency across the codebase.src/main/java/life/mosu/mosuserver/application/auth/AuthService.java (2)
6-6: LGTM - UserRole import added for role-based profile checks.The import addition supports the new role-based profile registration determination logic.
37-37: Verify role-based profile registration logic coverageAll instances of
profileJpaRepository.existsByUserId(...)have been removed—only the role-based check remains:• src/main/java/life/mosu/mosuserver/application/auth/AuthService.java:37
Boolean isProfileRegistered = isProfileRegistered(user);Please confirm that:
- Every existing user in your data has the correct role indicating a registered profile.
- No other parts of the application still rely on repository-based existence checks.
src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserService.java (2)
52-56: LGTM - Phone number support added to OAuth user updates.The addition of
phoneNumberparameter to theupdateOAuthUsermethod call enhances user data synchronization for existing OAuth users. This change is consistent with the broader user profile data handling improvements.
62-67: No unique constraints on email/loginId
Verified that the User JPA entity does not declare any@Column(unique = true)or@Table(uniqueConstraints…)for the email or loginId fields. Defaulting to"NA"will not violate database uniqueness constraints.src/main/java/life/mosu/mosuserver/application/profile/ProfileService.java (4)
7-7: LGTM - Import and DTO changes support refactored profile registration.The addition of
UserRoleimport and change fromProfileRequesttoSignUpProfileRequestalign with the broader refactoring of profile registration logic.Also applies to: 12-12
26-26: Profile registration method simplified and consistent with refactoring.The changes improve the method by:
- Using the consolidated
SignUpProfileRequestDTO- Encapsulating profile existence check in a private method
- Simplifying profile saving with direct repository call
Also applies to: 30-30, 33-33
56-60: Role-based profile existence check aligns with architectural changes.The method correctly implements the new role-based approach where
ROLE_PENDINGindicates no profile exists yet, while other roles indicate an existing profile. This is consistent with the changes inAuthService.
62-65: Enhanced user info synchronization with phone number support.The method now synchronizes phone number data along with gender, username, and birth date. This enhancement is consistent with the broader phone number support added across the user management system.
| //TODO: 카카오 승인 시 파씽 로직 고도화 | ||
| log.info("Kakao attributes: {}", attributes); |
There was a problem hiding this comment.
Security concern: Potential PII exposure in logs.
Logging OAuth attributes at INFO level may expose sensitive user information including email, phone numbers, and other personal data.
Consider using DEBUG level and sanitizing sensitive data:
- log.info("Kakao attributes: {}", attributes);
+ log.debug("Kakao attributes received for user: {}",
+ attributes.containsKey("kakao_account") ? "present" : "missing");📝 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.
| //TODO: 카카오 승인 시 파씽 로직 고도화 | |
| log.info("Kakao attributes: {}", attributes); | |
| //TODO: 카카오 승인 시 파씅 로직 고도화 | |
| log.debug("Kakao attributes received for user: {}", | |
| attributes.containsKey("kakao_account") ? "present" : "missing"); |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserInfo.java
around lines 32 to 33, the code logs OAuth attributes at INFO level, which risks
exposing sensitive personal information. Change the log level from INFO to DEBUG
and sanitize or mask any personally identifiable information before logging to
prevent PII exposure.
| String gender = null; | ||
| String birthDay = null; | ||
| String birthYear = null; | ||
| String phoneNumber = "010-1234-5678"; |
There was a problem hiding this comment.
Critical issue: Hardcoded phone number and incomplete builder.
Two issues found:
- Hardcoded placeholder phone number
"010-1234-5678" - The
phoneNumberfield is not included in the builder call (lines 52-55)
Apply this fix to include phoneNumber in the builder and use the actual value:
return OAuthUserInfo.builder()
.name(name)
.email("test123@gmali.com")
+ .phoneNumber(phoneNumber)
.build();Also consider extracting phone number from Kakao attributes instead of using hardcoded value when the TODO is addressed.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserInfo.java at
line 40, replace the hardcoded phone number string with a variable that will
hold the actual phone number value. Then, in the builder call between lines 52
and 55, add the phoneNumber field to ensure it is properly set when creating the
object. Later, update the code to extract the phone number dynamically from
Kakao attributes instead of using a hardcoded value once the related TODO is
implemented.
2 participants
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Refactor
Chores
Documentation