Conversation
MOSU-216 refactor: OAuth 인증 처리
|
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 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 focuses on preparing the v1 production server for deployment. It includes significant cleanup by removing unnecessary initialization code and deprecated API test endpoints, refining CORS settings for production environments, and enhancing OAuth2 integration to handle user marketing agreement data from Kakao. These changes aim to improve security, streamline the codebase, and ensure the application is robust for production use.
Highlights
- Production Readiness & Security Hardening: I've significantly refined the CORS configurations by removing various development and test domains from the allowed origins in both
SecurityConfig.javaandWebMvcConfig.java. This change narrows down the allowed origins to only production and essential local development environments, which is crucial for enhancing the security posture of the application as it moves to production. - Enhanced OAuth2 Integration for Kakao: I've enhanced the OAuth2 integration, specifically for Kakao, to retrieve and store users' marketing agreement status. This involved modifying
OAuthUserInfoto include a new field for marketing consent and updatingOAuthUserServiceto fetch and process the 'service_terms' from Kakao's API. Additionally,AuthorizationRequestRedirectResolverwas updated to ensure these terms are properly communicated during the OAuth flow, improving user data management and compliance. - Codebase Cleanup and Optimization: I've performed a substantial cleanup of the codebase by removing unused or deprecated components. This includes the complete removal of KMC (Korean Mobile Certification) related files and controllers, the
MasterController, and a large portion of theDatabaseInitializer's dummy data generation logic. These removals streamline the project, reduce its footprint, and eliminate unnecessary dependencies, making the application leaner and more maintainable. - API Endpoint and DTO Refactoring: I've refactored the authentication-related Data Transfer Objects (DTOs) by reorganizing them into dedicated 'request' and 'response' sub-packages within
presentation/auth/dto. This improves clarity and maintainability by clearly separating incoming request payloads from outgoing response structures. Concurrently, I've updated theWhitelistfor API endpoints, removing a 'MASTER' endpoint and adjusting the entry for 'APPLICATION_GUEST', further refining the API surface.
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 prepares the application for a production deployment by cleaning up configurations, removing test-related code, and enhancing the OAuth flow to handle marketing consent. The changes are generally positive and move the project towards a production-ready state. I've provided a few suggestions to improve maintainability, robustness, and code consistency, particularly around handling external API responses, reducing code duplication, and ensuring correct package structure for DTOs.
| if (serviceTermsAttributes.get("service_terms") instanceof List<?> termsList) { | ||
| agreedToMarketing = termsList.stream() | ||
| .filter(term -> term instanceof Map) | ||
| .map(term -> (Map<String, Object>) term) | ||
| .filter(termMap -> "terms_03".equals(termMap.get("tag"))) | ||
| .findFirst() | ||
| .map(termMap -> (Boolean) termMap.get("agreed")) | ||
| .orElse(false); | ||
| } |
There was a problem hiding this comment.
The logic to parse the marketing agreement status relies on instanceof checks and casting from Map<String, Object>. This approach is not type-safe and can be brittle if the API response structure changes.
To improve robustness and readability, I recommend creating dedicated DTOs for the Kakao service terms API response. This would allow you to leverage your JSON mapper (e.g., Jackson) for safe deserialization and eliminate manual casting.
For example, you could define classes like this:
@JsonIgnoreProperties(ignoreUnknown = true)
public record KakaoServiceTermsResponse(List<ServiceTerm> service_terms) {}
@JsonIgnoreProperties(ignoreUnknown = true)
public record ServiceTerm(String tag, boolean agreed) {}Using these DTOs in your WebClient call would make this parsing logic much cleaner and safer.
| private Map<String, Object> getServiceTerms(String accessToken) { | ||
|
|
||
| String url = "https://kapi.kakao.com/v2/user/service_terms"; | ||
|
|
||
| try { | ||
| Map<String, Object> response = webClient.get() | ||
| .uri(url) | ||
| .header("Authorization", "Bearer " + accessToken) | ||
| .retrieve() | ||
| .bodyToMono(new ParameterizedTypeReference<Map<String, Object>>() { | ||
| }) | ||
| .block(); | ||
|
|
||
| log.info(response.toString()); | ||
| return response != null ? response : Collections.emptyMap(); | ||
| } catch (Exception e) { | ||
| log.error("Failed to fetch service terms from Kakao with WebClient", e); | ||
| return Collections.emptyMap(); | ||
| } | ||
| } |
There was a problem hiding this comment.
This method has a couple of areas for improvement:
- Hardcoded URL: The Kakao API URL on line 101 is hardcoded. It's better to externalize this to a configuration file (e.g.,
application.yml) to improve maintainability and flexibility between different environments. - Generic Exception Handling: The
catch (Exception e)on line 114 is too broad. It's better to catch more specific exceptions, such asWebClientResponseExceptionfrom Spring's WebClient, to handle different HTTP error statuses (e.g., 4xx vs. 5xx) appropriately.
| .allowedOrigins( | ||
| "http://localhost:3000", | ||
| "http://localhost:8080", | ||
| "http://api.mosuedu.com", | ||
| "https://mosuedu.com", | ||
| "https://api.mosuedu.com", | ||
| "https://www.mosuedu.com", | ||
| "http://www.mosuedu.com:3000", | ||
| "https://test.mosuedu.com", | ||
| "http://localhost:5173", | ||
| "https://partnership.mosuedu.com", | ||
| "http://dev.mosuedu.com:3000", | ||
| "https://dev.mosuedu.com", | ||
| "http://admin.mosuedu.com:3000", | ||
| "https://admin.mosuedu.com" | ||
| ) |
There was a problem hiding this comment.
The list of allowed origins is duplicated here and in SecurityConfig.java. To avoid inconsistencies and improve maintainability, you should reuse the clients list defined in SecurityConfig.
You can convert the list to an array as required by the allowedOrigins method.
.allowedOrigins(SecurityConfig.clients.toArray(new String[0]))| @@ -1,9 +1,10 @@ | |||
| package life.mosu.mosuserver.presentation.auth.dto; | |||
| package life.mosu.mosuserver.presentation.auth.dto.request; | |||
There was a problem hiding this comment.
The LoginResponse class is a Data Transfer Object for responses, but it has been moved to the ...dto.request package. For consistency with the new package structure, it should be in the ...dto.response package.
| package life.mosu.mosuserver.presentation.auth.dto.request; | |
| package life.mosu.mosuserver.presentation.auth.dto.response; |
1 participant
⚡ 배포 설정