feat: 지원서 전체 현황 관리자만 볼 수 있게 aop 적용 #254
Conversation
WalkthroughThe changes implement an admin access control mechanism within the application. A new annotation Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as ApplicationController
participant Aspect as AdminAuthorizationAspect
participant User as SiteUser
Client->>Controller: Call getApplicants()
Controller->>Aspect: Intercept method (@RequireAdminAccess)
Aspect->>User: Retrieve and check user role
alt User role is ADMIN
Aspect->>Controller: Proceed with method execution
Controller->>Client: Return ApplicationsResponse
else User role is NOT ADMIN
Aspect->>Controller: Throw CustomException (ACCESS_DENIED)
Controller->>Client: Return error response
end
Poem
✨ Finishing Touches
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:
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 (
|
|
Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here. PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here. PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||
![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 (5)
src/main/java/com/example/solidconnection/custom/security/annotation/RequireAdminAccess.java (1)
8-11: Consider making the annotation future-proof by supporting multiple roles.
Although this annotation name is descriptive for the current use case (ADMIN access), you may want to add optional metadata or rename it in the future if you introduce more roles (e.g., MENTOR). This ensures extensibility without introducing additional annotations or rewriting existing logic.src/main/java/com/example/solidconnection/application/controller/ApplicationController.java (1)
42-42: Evaluate redundancy between AOP-based access check andvalidateSiteUserCanViewApplicants.
You are already restricting this endpoint to admin users via the@RequireAdminAccessaspect. CallingapplicationQueryService.validateSiteUserCanViewApplicantsmay be redundant unless it includes extra logic beyond pure admin role validation. Consider consolidating or removing duplicate checks to avoid confusion.src/main/java/com/example/solidconnection/custom/security/aspect/AdminAuthorizationAspect.java (2)
15-19: Good use of Spring AOP and dependency injection.
Annotating this class with@Aspect,@Component, and@RequiredArgsConstructorcleanly ties it into the Spring ecosystem. If your role-based needs grow, consider exploring advanced Spring Security features to keep your authorization logic centralized and maintainable.
20-34: Consider expanding role checks and adding logging.
The loop neatly retrieves the firstSiteUserargument, but if multiple user objects exist, only the first is checked. You might also add a log entry when access is granted or denied, to aid in troubleshooting. Future expansions might account for multiple roles or a more granular permission system if needed.src/test/java/com/example/solidconnection/custom/security/aspect/AdminAuthorizationAspectTest.java (1)
28-64: Consider adding a test for null user scenario.The current tests cover admin and non-admin user scenarios, but there's no test for how the aspect handles a null SiteUser parameter. Consider adding a test case to verify the behavior when a null user is passed to an admin-only method.
@Test void 사용자가_null인_경우_어드민_전용_메소드에_접근하면_예외_응답을_반환한다() { // given SiteUser nullUser = null; // when & then assertThatCode(() -> testService.adminOnlyMethod(nullUser)) .isInstanceOf(CustomException.class) .hasMessage(ACCESS_DENIED.getMessage()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/example/solidconnection/application/controller/ApplicationController.java(2 hunks)src/main/java/com/example/solidconnection/custom/security/annotation/RequireAdminAccess.java(1 hunks)src/main/java/com/example/solidconnection/custom/security/aspect/AdminAuthorizationAspect.java(1 hunks)src/test/java/com/example/solidconnection/custom/security/aspect/AdminAuthorizationAspectTest.java(1 hunks)
🔇 Additional comments (4)
src/main/java/com/example/solidconnection/application/controller/ApplicationController.java (1)
9-9: No concerns with this import.
This is a standard import statement, so there’s nothing special to address.src/test/java/com/example/solidconnection/custom/security/aspect/AdminAuthorizationAspectTest.java (3)
21-64: Well-structured test cases with good coverage of authorization scenarios.The test class effectively verifies the admin authorization aspect with three key test cases:
- Admin users can access admin-only methods
- Non-admin users receive an access denied exception when attempting to access admin-only methods
- Methods without the RequireAdminAccess annotation are accessible to all users
The Given-When-Then structure makes the tests clear and readable.
66-76: Good utility method for test user creation.This helper method cleanly encapsulates the creation of different user types with various roles, making the test cases more concise and maintainable.
79-98: Effective test configuration with sample service implementation.The test configuration and TestService implementation provide a clean way to test the aspect functionality in isolation. The contrast between annotated and non-annotated methods allows for thorough verification of the aspect's behavior.
wibaek
left a comment
wibaek
left a comment
There was a problem hiding this comment.
Spring AOP를 활용하여 권한 체크를 구현하셨군요! 쉽게 사용하기 좋은 접근 방식인 것 같습니다.
기존에는 /admin 경로에 대해 Spring Security의 필터 체인을 사용하여 관리자 페이지 접근을 제한했던 것으로 알고 있습니다. 그런데 이번 방식이 도입되면서 관리자 권한 체크 방식이 두 가지로 나뉘게 되는데, 이에 대한 의견이 궁금합니다.
저는 /admin 경로 기반 화이트리스트 방식을 유지하면서 AOP 기반의 권한 체크를 병행하는 것도 좋은 방법이라고 생각합니다. 이렇게 하면, 관리자 API들이 /admin 경로 아래에 있을 때 어노테이션을 실수로 누락하여 발생할 수 있는 문제를 사전에 방지할 수도 있고 말이죠.
|
맞습니다! 영서님과 논의했을 때, 모든 권한을 Spring Security의 필터 체인에서만 관리하면 신규 개발자가 이에 대해 실수할 가능성이 높다고 판단했습니다. 그래서 컨트롤러 단에서 권한 체크를 할 수 있도록 AOP를 활용하여 구현하게 되었습니다. 말씀하신 것처럼, admin 관련 api는 SecurityFilterChain에서 한 번에 관리하는 것이 적절하다고 생각했습니다. 이를 통해 어드민 API가 /admin 경로 아래에 있을 때, 어노테이션 누락 등의 실수를 방지할 수 있다는 점에서 동의합니다! |
There was a problem hiding this comment.
저는 /admin 경로 기반 화이트리스트 방식을 유지하면서 AOP 기반의 권한 체크를 병행하는 것도 좋은 방법이라고 생각합니다.
동의합니다😊
- AOP 기반의 권한 체크 → uri 는 유지하면서 권한만 변경
- /admin 경로 기반 화이트리스트 방식 → uri 패턴으로도 방어해서 실수도 방지
할 수 있으니 좋다 생각합니다.
제가 AOP를 실제 적용해보는 건 사실 처음이라 부족한 부분 말씀해주시면 좀 더 공부해서 반영해보겠습니다
코드도 적절히 구현하시고 테스트도 잘 작성하셨습니다!
어노테이션 이름을 RequireAdminAccess로 지었는데 괜찮나요?
적절하다 생각합니다👍
3 participants
User description
관련 이슈
작업 내용
지원서 전체 현황 조회 API에 관리자(ADMIN) 전용 접근 제한 기능을 구현했습니다.
특이 사항
현재는 관리자(ADMIN) 전용 API만 구분하도록 간단하게 구현하였는데 추후에 멘토 등의 복잡한 구분이 생기게 될 때 좀 더 범용적인 역할 기반 접근 제어로 확장해야할 거 같습니다.
리뷰 요구사항 (선택)
PR Type
Enhancement, Tests
Description
Added
@RequireAdminAccessannotation for admin-only API access.Implemented AOP-based admin authorization logic.
Restricted access to application overview API to admins.
Added unit tests for admin authorization aspect.
Changes walkthrough 📝
ApplicationController.java
Restrict application overview API to adminssrc/main/java/com/example/solidconnection/application/controller/ApplicationController.java
@RequireAdminAccessannotation to restrict access.getApplicantsmethod.RequireAdminAccess.java
Add custom annotation for admin accesssrc/main/java/com/example/solidconnection/custom/security/annotation/RequireAdminAccess.java
@RequireAdminAccess.AdminAuthorizationAspect.java
Implement AOP-based admin authorization logicsrc/main/java/com/example/solidconnection/custom/security/aspect/AdminAuthorizationAspect.java
SiteUserrole as ADMIN.AdminAuthorizationAspectTest.java
Add tests for admin authorization aspectsrc/test/java/com/example/solidconnection/custom/security/aspect/AdminAuthorizationAspectTest.java
Summary by CodeRabbit