MOSU-132 chore: admin role 인가 로직 추가#136
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update enforces method-level security across various controllers by adding Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Security
participant Service
Client->>Controller: API Request (e.g., POST /apply)
Controller->>Security: @PreAuthorize check
alt Authorized
Security-->>Controller: Access granted
Controller->>Service: Business logic (with userId from @UserId)
Service-->>Controller: Result
Controller-->>Client: Response
else Not authorized
Security-->>Client: 403 Forbidden
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 (19)
✨ 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 significantly enhances the application's security posture by implementing robust role-based access control (RBAC) across a wide range of API endpoints. It leverages Spring Security to enforce granular permissions for both 'ADMIN' and 'USER' roles, ensuring that functionalities are accessible only to authorized individuals. Additionally, it includes minor refactorings in the KMC integration module and standardizes user ID handling, contributing to a more secure and maintainable codebase.
Highlights
- Role-Based Access Control (RBAC): Implemented comprehensive role-based access control across numerous API endpoints using Spring Security's
@PreAuthorizeannotation. This restricts access to functionalities based on whether a user is authenticated and possesses the 'ADMIN' or 'USER' role. - Admin Functionality Security: Secured all administrative endpoints (e.g., managing applications, banners, dashboards, refunds, students, events, FAQs, and answering inquiries) to be accessible only by users with the 'ADMIN' role.
- User Functionality Security: Ensured user-specific functionalities (e.g., applying for services, viewing applications, managing exam applications, payments, profiles, and recommendations, and creating/deleting inquiries) are accessible only to authenticated users, often specifically requiring the 'USER' role.
- KMC Integration Refactoring: Refactored KMC (Korea Mobile Certification) related utility classes (
KmcCryptoManager,KmcDataMapper) by removing unnecessary decryption logic for CI/DI values and simplifying logging, streamlining the handling of sensitive KMC data. - Standardized User ID Retrieval: Standardized user ID retrieval in several controllers by replacing
@AuthenticationPrincipalor@RequestParamwith a custom@UserIdannotation, improving code consistency and simplifying authentication integration.
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 role-based authorization logic to numerous controller endpoints using Spring Security's @PreAuthorize annotation, which is a great improvement for the application's security. The refactoring to use a custom @UserId annotation is also a positive change for code clarity and consistency.
I've identified a few areas for improvement. Some refactoring has led to the removal of useful Javadocs and comments, which impacts maintainability. More critically, I've found several instances in the PaymentWidgetController where the authenticated user's ID is passed to methods but not used in the underlying service calls. This is a significant security concern that must be addressed to prevent users from performing actions on behalf of others.
| public ApiResponseWrapper<PaymentPrepareResponse> prepare( | ||
| @UserId Long userId, | ||
| @Valid @RequestBody PreparePaymentRequest request | ||
| ) { |
There was a problem hiding this comment.
The userId obtained from the security context is not being used in the paymentService.prepare(request) call. This is a potential security risk. The service layer should use the userId to ensure that the payment is being prepared for the currently authenticated user, preventing one user from initiating actions on behalf of another.
| public ApiResponseWrapper<PaymentPrepareResponse> prepare( | |
| @UserId Long userId, | |
| @Valid @RequestBody PreparePaymentRequest request | |
| ) { | |
| @PreAuthorize("isAuthenticated() and hasRole('USER')") | |
| public ApiResponseWrapper<PaymentPrepareResponse> prepare( | |
| @UserId Long userId, | |
| @Valid @RequestBody PreparePaymentRequest request | |
| ) { | |
| PaymentPrepareResponse response = paymentService.prepare(userId, request); |
| @PreAuthorize("isAuthenticated() and hasRole('USER')") | ||
| public ApiResponseWrapper<Void> confirm( | ||
| @UserId Long userId, | ||
| @RequestBody PaymentRequest request |
There was a problem hiding this comment.
Similar to the prepare method, the userId from the authenticated user is not being used in the paymentService.confirm(request) call. For security, the service method should validate that the user confirming the payment is the one who initiated it. Please pass the userId to the service layer and use it for validation.
| @PreAuthorize("isAuthenticated() and hasRole('USER')") | |
| public ApiResponseWrapper<Void> confirm( | |
| @UserId Long userId, | |
| @RequestBody PaymentRequest request | |
| @PreAuthorize("isAuthenticated() and hasRole('USER')") | |
| public ApiResponseWrapper<Void> confirm( | |
| @UserId Long userId, | |
| @RequestBody PaymentRequest request | |
| ) { | |
| paymentService.confirm(userId, request); |
| public String decryptResponseData(String recCert) { | ||
| try { | ||
| // 1차 복호화 | ||
| String firstDecrypted = decrypt(recCert); | ||
|
|
||
| // 데이터와 HMAC(무결성 검증 값) 분리 | ||
| int firstIdx = firstDecrypted.indexOf(DELIMITER); | ||
| String encPara = firstDecrypted.substring(0, firstIdx); | ||
| String receivedHmac = firstDecrypted.substring(firstIdx + 1, firstDecrypted.lastIndexOf(DELIMITER)); | ||
|
|
||
| // 무결성 검증 | ||
| String receivedHmac = firstDecrypted.substring(firstIdx + 1, | ||
| firstDecrypted.lastIndexOf(DELIMITER)); | ||
| String generatedHmac = secuManager.getMsg(encPara); | ||
| if (!generatedHmac.equals(receivedHmac)) { | ||
| throw new SecurityException("KMC 데이터의 위변조가 의심됩니다."); | ||
| } | ||
|
|
||
| // 2차 복호화하여 최종 데이터 반환 | ||
| return decrypt(encPara); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException("KMC 인증 결과를 처리하는 중 오류가 발생했습니다.", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
The Javadocs and explanatory inline comments for decryptResponseData have been removed. While the code might be clear now, this documentation is very valuable for future maintainers to understand the multi-step decryption and HMAC verification process. Please consider restoring the comments and Javadoc to improve code maintainability.
| log.info(" - Name: {}", tokens[NAME_INDEX]); | ||
| log.info(" - PhoneNo: {}", tokens[PHONE_NO_INDEX]); | ||
| log.info(" - PlusInfo: {}", tokens[PLUS_INFO_INDEX]); | ||
| log.info(" - PlusInfo -> {}", tokens[PLUS_INFO_INDEX]); |
There was a problem hiding this comment.
This log statement appears to be a duplicate of the one on the previous line and seems like a leftover from debugging. It should be removed to keep the code clean.
| log.info(" - PlusInfo -> {}", tokens[PLUS_INFO_INDEX]); | |
| log.info(" - PlusInfo -> {}", tokens[PLUS_INFO_INDEX]); |
1 participant
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Refactor
Style