MOSU-144 feat: Login 후 profile 등록 여부 판단#146
Conversation
WalkthroughThis update introduces a new exam date validation to the application process, modifies authentication and login response structures to include more user details, and refactors cookie handling in the authentication controller. Additional error codes, DTOs, and comprehensive tests for login responses are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthController
participant AuthService
participant UserRepo
participant LoginCommandResponse
participant LoginResponse
Client->>AuthController: POST /login (LoginRequest)
AuthController->>AuthService: login(request)
AuthService->>UserRepo: find user by credentials
UserRepo-->>AuthService: UserJpaEntity
AuthService->>LoginCommandResponse: of(token, isProfileRegistered, user)
AuthService-->>AuthController: LoginCommandResponse
AuthController->>LoginResponse: from(isProfileRegistered, user)
AuthController-->>Client: LoginResponse (with oauthUser if not registered)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
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 (5)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧬 Code Graph Analysis (1)src/main/java/life/mosu/mosuserver/presentation/auth/SignUpController.java (1)
🔇 Additional comments (5)
✨ 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 enhances the application's robustness and user experience by refining the exam application process and improving user authentication flows. It adds a crucial validation to prevent applications for past exams and enriches the login response with more detailed user information, particularly for newly registered users. These changes are supported by updates to the core UserJpaEntity and a significant refactoring of cookie handling in the authentication layer, all backed by new DTOs and dedicated unit tests to ensure stability.
Highlights
- Application Validation Enhancement: Introduced a new validation rule in
ApplicationServiceto prevent users from applying for exams whose dates have already passed, ensuring that only future exams can be applied for. - User Information in Login Response: Modified the login process to include detailed user information (
UserJpaEntity) within theLoginCommandResponseandLoginResponseDTOs. This allows the frontend to receive more comprehensive user data, especially for users who have not yet completed their profile registration. - User Entity Update: The
UserJpaEntityconstructor has been updated to include aphoneNumberfield, enabling the storage of more complete user contact information. - Auth Controller Refactoring: Centralized the logic for building and applying HTTP
Set-Cookieheaders for access and refresh tokens into a new private utility method (applyTokenHeader) withinAuthController, significantly improving code readability and maintainability. - New Login User DTO and Tests: Introduced a new
LoginUserResponseDTO to specifically encapsulate a subset of user details returned during the login process when a profile is not yet fully registered. Comprehensive unit tests have also been added forLoginResponseto ensure its correct behavior across various login scenarios (standard and OAuth).
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 introduces two main features. First, it enhances the login response to include user details if a user profile has not yet been created, which is useful for pre-filling profile forms, especially after an OAuth login. This involves changes to DTOs, services, and controllers. Second, it adds a validation step to prevent users from applying for exams that have already passed.
The changes are generally in the right direction, but there are several points to address:
- A critical security and functionality regression was introduced in the cookie creation logic, where
secure,domain, andsameSiteattributes were dropped. - There's a potential
NullPointerExceptionin the newLoginResponseDTO due to how aBooleanis checked. - Several areas could be improved for code quality and maintainability, including adherence to Java naming conventions, simplifying complex
Optionalusage, and improving test practices.
I've left detailed comments on these points. Addressing them will improve the robustness and security of the application.
| private HttpHeaders applyTokenHeader(Token token) { | ||
| HttpHeaders headers = new HttpHeaders( | ||
|
|
||
| ); | ||
| headers.add(HttpHeaders.SET_COOKIE, CookieBuilderUtil.createCookie( | ||
| ACCESS_TOKEN_COOKIE_NAME, | ||
| token.accessToken(), | ||
| token.accessTokenExpireTime() | ||
| )); | ||
| headers.add(HttpHeaders.SET_COOKIE, CookieBuilderUtil.createCookie( | ||
| REFRESH_TOKEN_COOKIE_NAME, | ||
| token.refreshToken(), | ||
| token.refreshTokenExpireTime() | ||
| )); | ||
| return headers; | ||
| } |
There was a problem hiding this comment.
The new helper method relies on CookieBuilderUtil, which creates cookies without the secure, domain, and sameSite attributes that were present in the original implementation. This is a critical regression.
- The
secureattribute is essential for security, ensuring cookies are only sent over HTTPS. - The
domainandsameSiteattributes are crucial for the cookie to work correctly across subdomains (e.g.,api.mosuedu.comandwww.mosuedu.com).
The CookieBuilderUtil needs to be updated to correctly configure these attributes.
| public static LoginResponse from(final OAuthUser user) { | ||
| if (user.getIsProfileRegistered().equals(true)) { | ||
| return new LoginResponse(true, null); | ||
| } | ||
| return new LoginResponse(false, LoginUserResponse.from(user.getUser())); | ||
| } | ||
|
|
||
| public static LoginResponse from(Boolean isProfileRegistered, final UserJpaEntity user) { | ||
| if (isProfileRegistered.equals(true)) { | ||
| return new LoginResponse(true, null); | ||
| } | ||
| return new LoginResponse(false, LoginUserResponse.from(user)); | ||
| } | ||
| } |
There was a problem hiding this comment.
There are a couple of improvements that can be made here:
- The logic in the two
frommethods is duplicated. You can refactor this by having one method call the other to reduce code duplication. - Using
isProfileRegistered.equals(true)can lead to aNullPointerExceptionifisProfileRegisteredisnull. It's safer to useBoolean.TRUE.equals(isProfileRegistered), which is null-safe.
public static LoginResponse from(final OAuthUser user) {
return from(user.getIsProfileRegistered(), user.getUser());
}
public static LoginResponse from(Boolean isProfileRegistered, final UserJpaEntity user) {
if (Boolean.TRUE.equals(isProfileRegistered)) {
return new LoginResponse(true, null);
}
return new LoginResponse(false, LoginUserResponse.from(user));
}|
|
||
| Set<Subject> subjects = request.getSubjects(); | ||
|
|
||
| validator.ExamDateNotPassed(examIds); |
There was a problem hiding this comment.
Consider calling the validator method with a lowercase starting letter to adhere to Java naming conventions.
| validator.ExamDateNotPassed(examIds); | |
| validator.examDateNotPassed(examIds); |
| public void ExamDateNotPassed(List<Long> examIds) { | ||
| List<ExamJpaEntity> exams = examJpaRepository.findAllById(examIds); | ||
| boolean hasPassedExam = exams.stream() | ||
| .anyMatch(exam -> exam.getExamDate().isBefore(LocalDate.now())); | ||
|
|
||
| if (hasPassedExam) { | ||
| throw new CustomRuntimeException(ErrorCode.EXAM_DATE_PASSED); | ||
| } | ||
| } |
There was a problem hiding this comment.
According to Java naming conventions, method names should start with a lowercase letter and be in camelCase. Please rename ExamDateNotPassed to examDateNotPassed to follow this convention.
| public void ExamDateNotPassed(List<Long> examIds) { | |
| List<ExamJpaEntity> exams = examJpaRepository.findAllById(examIds); | |
| boolean hasPassedExam = exams.stream() | |
| .anyMatch(exam -> exam.getExamDate().isBefore(LocalDate.now())); | |
| if (hasPassedExam) { | |
| throw new CustomRuntimeException(ErrorCode.EXAM_DATE_PASSED); | |
| } | |
| } | |
| public void examDateNotPassed(List<Long> examIds) { | |
| List<ExamJpaEntity> exams = examJpaRepository.findAllById(examIds); | |
| boolean hasPassedExam = exams.stream() | |
| .anyMatch(exam -> exam.getExamDate().isBefore(LocalDate.now())); | |
| if (hasPassedExam) { | |
| throw new CustomRuntimeException(ErrorCode.EXAM_DATE_PASSED); | |
| } | |
| } |
| private final static String ACCESS_TOKEN_COOKIE_NAME = "accessToken"; | ||
| private final static String REFRESH_TOKEN_COOKIE_NAME = "refreshToken"; |
There was a problem hiding this comment.
According to standard Java conventions, the static modifier should come before final for constants.
| private final static String ACCESS_TOKEN_COOKIE_NAME = "accessToken"; | |
| private final static String REFRESH_TOKEN_COOKIE_NAME = "refreshToken"; | |
| private static final String ACCESS_TOKEN_COOKIE_NAME = "accessToken"; | |
| private static final String REFRESH_TOKEN_COOKIE_NAME = "refreshToken"; |
| HttpHeaders headers = new HttpHeaders( | ||
|
|
||
| ); |
There was a problem hiding this comment.
The HttpHeaders instantiation can be simplified. The empty lines are unnecessary.
HttpHeaders headers = new HttpHeaders();| public static LoginUserResponse from(UserJpaEntity user) { | ||
| return Optional.ofNullable(user) | ||
| .map(userEntity -> { | ||
| String gender = Optional.ofNullable(userEntity.getGender()) | ||
| .map(Enum::name) | ||
| .orElse(null); | ||
|
|
||
| return new LoginUserResponse( | ||
| gender, | ||
| userEntity.getName(), | ||
| userEntity.getBirth(), | ||
| userEntity.getPhoneNumber() | ||
| ); | ||
| }) | ||
| .orElse(null); | ||
| } |
There was a problem hiding this comment.
The use of nested Optional makes this factory method more complex than necessary and can be difficult to read. A simple null check would be more straightforward and achieve the same result with better readability.
public static LoginUserResponse from(UserJpaEntity user) {
if (user == null) {
return null;
}
String gender = (user.getGender() != null) ? user.getGender().name() : null;
return new LoginUserResponse(
gender,
user.getName(),
user.getBirth(),
user.getPhoneNumber()
);
}| import static org.junit.Assert.assertFalse; | ||
| import static org.junit.Assert.assertNotNull; | ||
| import static org.junit.Assert.assertNull; | ||
| import static org.junit.Assert.assertTrue; |
There was a problem hiding this comment.
These tests are using JUnit 5 (@org.junit.jupiter.api.Test), but the assertions are from JUnit 4 (org.junit.Assert). For consistency and to leverage JUnit 5 features, it's recommended to use assertions from org.junit.jupiter.api.Assertions.
| import static org.junit.Assert.assertFalse; | |
| import static org.junit.Assert.assertNotNull; | |
| import static org.junit.Assert.assertNull; | |
| import static org.junit.Assert.assertTrue; | |
| import static org.junit.jupiter.api.Assertions.assertFalse; | |
| import static org.junit.jupiter.api.Assertions.assertNotNull; | |
| import static org.junit.jupiter.api.Assertions.assertNull; | |
| import static org.junit.jupiter.api.Assertions.assertTrue; |
| String jsonResponse = objectMapper.writeValueAsString(response); | ||
| log.info("카카오_로그인_사용자가_기존의_회원인_경우_응답 JSON : {}", jsonResponse); |
There was a problem hiding this comment.
Logging the JSON response in a test can be useful for debugging, but it adds noise to the test execution logs. Tests should ideally be silent on success. Consider removing these log statements from the tests to keep the logs clean, which helps in identifying real issues faster.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/java/life/mosu/mosuserver/application/application/vaildator/ApplicationValidator.java (1)
71-79: Consider improving robustness and method naming consistency.The validation logic is correct, but there are two areas for improvement:
Method naming inconsistency: This method uses
ExamDateNotPassedwhile others useRequestNoDuplicateExams,ExamIdsAndLunchSelection. Consider renaming tovalidateExamDateNotPassedfor consistency.Handling non-existent exam IDs: If some
examIdsdon't exist in the database, they won't be included in the stream check, potentially allowing the validation to pass incorrectly. Consider adding an existence check similar toExamIdsAndLunchSelection.public void ExamDateNotPassed(List<Long> examIds) { List<ExamJpaEntity> exams = examJpaRepository.findAllById(examIds); + + if (exams.size() != examIds.size()) { + throw new CustomRuntimeException(ErrorCode.EXAM_NOT_FOUND); + } + boolean hasPassedExam = exams.stream() .anyMatch(exam -> exam.getExamDate().isBefore(LocalDate.now())); if (hasPassedExam) { throw new CustomRuntimeException(ErrorCode.EXAM_DATE_PASSED); } }src/main/java/life/mosu/mosuserver/presentation/auth/dto/LoginResponse.java (2)
13-18: Consider using direct boolean comparison for better performance.The logic is correct, but using
user.getIsProfileRegistered().equals(true)is less efficient than a direct boolean comparison.Apply this diff for better performance:
- if (user.getIsProfileRegistered().equals(true)) { + if (user.getIsProfileRegistered()) {
20-25: Consider using direct boolean comparison for consistency.Same optimization opportunity as the previous method - direct boolean comparison would be more efficient and cleaner.
Apply this diff for consistency:
- if (isProfileRegistered.equals(true)) { + if (isProfileRegistered) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/main/java/life/mosu/mosuserver/application/application/ApplicationService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/application/vaildator/ApplicationValidator.java(2 hunks)src/main/java/life/mosu/mosuserver/application/auth/AuthService.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/user/UserJpaEntity.java(1 hunks)src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java(1 hunks)src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginSuccessHandler.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/AuthController.java(4 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/dto/LoginCommandResponse.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/dto/LoginResponse.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/dto/LoginUserResponse.java(1 hunks)src/test/java/life/mosu/mosuserver/presentation/auth/dto/LoginResponseTest.java(1 hunks)
⏰ 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 (23)
src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java (1)
104-104: LGTM! New error code follows established patterns.The addition of
EXAM_DATE_PASSEDerror code is well-placed among exam-related errors and uses appropriate HTTP status and descriptive Korean message consistent with the codebase style.src/main/java/life/mosu/mosuserver/application/application/ApplicationService.java (1)
44-44: LGTM! Validation placed appropriately in the chain.The exam date validation is correctly positioned before other business rule validations, ensuring expired exams are caught early in the process.
src/main/java/life/mosu/mosuserver/application/application/vaildator/ApplicationValidator.java (1)
3-3: LGTM! Import addition is necessary.The
LocalDateimport is correctly added to support the new date validation functionality.src/main/java/life/mosu/mosuserver/application/auth/AuthService.java (1)
40-40: LGTM! Clean enhancement to include user entity in response.The change correctly passes the already-extracted user entity to the
LoginCommandResponse.ofmethod, enabling richer response data without affecting the method's core authentication logic.src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginSuccessHandler.java (1)
56-56: LGTM! Enhanced OAuth2 response with richer user context.Passing the complete
OAuthUserobject instead of just a boolean flag allows theLoginResponseto access more detailed user information for response construction.src/main/java/life/mosu/mosuserver/presentation/auth/dto/LoginCommandResponse.java (3)
3-3: LGTM! Proper import for the new field.Clean addition of the necessary import for
UserJpaEntity.
7-9: LGTM! Well-structured record enhancement.The addition of the
UserJpaEntity userfield extends the response capabilities while maintaining the existing structure.
11-14: LGTM! Factory method properly updated.The
ofmethod signature correctly reflects the new parameter, maintaining consistency with the calling code inAuthService.src/main/java/life/mosu/mosuserver/domain/user/UserJpaEntity.java (2)
72-75: LGTM! Constructor parameter addition is well-structured.The
phoneNumberparameter is cleanly added to the@Builderconstructor, maintaining the existing parameter organization.
80-80: LGTM! Proper field assignment.The
phoneNumberfield is correctly assigned from the constructor parameter, completing the enhancement to include phone number during entity construction.src/main/java/life/mosu/mosuserver/presentation/auth/dto/LoginUserResponse.java (2)
7-12: LGTM! Well-designed DTO record structure.The record fields are appropriately typed and organized, providing a clean interface for user response data.
14-29: LGTM! Excellent factory method with robust null safety.The
frommethod demonstrates good defensive programming practices:
- Uses
Optional.ofNullableto handle null user entities- Safely converts enum to string using
Enum::name- Provides graceful fallback with
orElse(null)for both the gender field and the entire responseThis implementation ensures the DTO creation won't fail even with incomplete user data.
src/main/java/life/mosu/mosuserver/presentation/auth/dto/LoginResponse.java (2)
3-6: LGTM! Clean import additions.The new imports are well-organized and necessary for the enhanced LoginResponse functionality.
8-11: Good use of Jackson annotation for conditional serialization.The
@JsonInclude(Include.NON_NULL)annotation on theoauthUserfield properly excludes null values from JSON serialization, which aligns with the business logic where this field should only be present when the profile is not registered.src/test/java/life/mosu/mosuserver/presentation/auth/dto/LoginResponseTest.java (5)
1-58: Excellent test setup with comprehensive configuration.The test class is well-structured with proper ObjectMapper configuration for Java time serialization and realistic test data setup. The use of Korean naming conventions aligns well with the domain context.
64-84: Good test coverage for OAuth registered user scenario.The test properly verifies that when an OAuth user has a registered profile, the response contains
isProfileRegistered: trueandoauthUser: null. The JSON logging is helpful for debugging.
86-106: Good test coverage for OAuth unregistered user scenario.The test correctly verifies that when an OAuth user doesn't have a registered profile, the response contains
isProfileRegistered: falseandoauthUserwith user details.
113-125: Thorough testing of MOSU login registered scenario.The test properly validates the factory method that accepts explicit parameters, ensuring correct behavior when the profile is registered.
127-139: Complete test coverage for MOSU login unregistered scenario.The test validates the scenario where a MOSU user's profile is not registered, ensuring the
oauthUserfield is populated correctly.src/main/java/life/mosu/mosuserver/presentation/auth/AuthController.java (4)
6-6: Good addition of utility import.The import of
CookieBuilderUtilsupports the refactored cookie handling approach.
26-27: Well-defined constants for cookie names.The static final constants follow Java naming conventions and improve maintainability by centralizing cookie name definitions.
42-45: Clean integration of refactored cookie handling and enhanced response.The login method now properly uses the extracted helper method for cookie handling and passes the user entity to create a richer login response. This aligns well with the LoginResponse changes.
58-73: Excellent refactoring of cookie creation logic.The
applyTokenHeadermethod effectively centralizes cookie creation logic, making the code more maintainable and reusable. The use ofCookieBuilderUtilpromotes consistency across the application.
1 participant
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests