[Feat/admin authorization] - 사용자 권한에 따른 시큐리티 설정#103
Conversation
WalkthroughThe changes introduce centralized handling for unauthorized access and insufficient permissions within the authentication and security layers. A custom access denied handler, new exception classes, error codes, and security constants are added. The security configuration is refactored for modularity, and global exception handling is expanded to cover new error scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SecurityFilter
participant SecurityContext
participant JwtAuthFilter
participant AccessDeniedHandler
participant ExceptionHandler
Client->>SecurityFilter: HTTP Request
SecurityFilter->>JwtAuthFilter: doFilterInternal(request)
JwtAuthFilter->>SecurityContext: Check authentication
alt Already authenticated and not anonymous
JwtAuthFilter-->>SecurityFilter: Proceed with filterChain
else Not authenticated
JwtAuthFilter->>JwtAuthFilter: Validate JWT
alt JWT valid and authorized
JwtAuthFilter-->>SecurityFilter: Proceed with filterChain
else JWT invalid or insufficient permissions
JwtAuthFilter->>AccessDeniedHandler: handle(request, response, exception)
AccessDeniedHandler->>Client: JSON 403 Forbidden error response
end
end
alt Exception occurs
SecurityFilter->>ExceptionHandler: Handle exception
ExceptionHandler->>Client: JSON error response (404/403)
end
Suggested reviewers
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:
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 (
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the security configuration to support finer-grained URL authorization, exception handling, and error reporting for admin and user endpoints.
- Introduces new exception handlers in GlobalExceptionHandler for resource not found and unauthorized access.
- Adds a custom access denied handler and refactors the security filter chain with environment-specific authorization.
- Extracts duplicated configurations and adds new error codes and exception types for proper handling of authorization errors.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/org/runimo/runimo/exceptions/GlobalExceptionHandler.java | Adds new exception handlers for NoResourceFoundException and UnauthorizedAccessException. |
| src/main/java/org/runimo/runimo/exceptions/CustomAccessDeniedHandler.java | Implements a custom access denied handler for better error responses. |
| src/main/java/org/runimo/runimo/config/SecurityConstants.java | Consolidates endpoint patterns and role constants. |
| src/main/java/org/runimo/runimo/config/SecurityConfig.java | Refactors security filter chain construction and authorization settings. |
| src/main/java/org/runimo/runimo/auth/filters/UserErrorCode.java | Introduces a new error code for insufficient permissions (HTTP 403). |
| src/main/java/org/runimo/runimo/auth/filters/JwtAuthenticationFilter.java | Adds a condition to bypass filtering when an authenticated context is already set. |
| src/main/java/org/runimo/runimo/auth/exceptions/UnauthorizedAccessException.java | Creates a new exception type for unauthorized access scenarios. |
Comments suppressed due to low confidence (1)
src/main/java/org/runimo/runimo/exceptions/GlobalExceptionHandler.java:33
- [nitpick] Consider extracting the error message '요청한 리소스를 찾을 수 없습니다.' into a constant to promote reuse and ensure consistency.
NoResourceFoundException e) {
| @ExceptionHandler(UnauthorizedAccessException.class) | ||
| public ResponseEntity<ErrorResponse> handleUnauthorizedAccessException( | ||
| UnauthorizedAccessException e) { | ||
| log.debug("{} {}", ERROR_LOG_HEADER, e.getMessage(), e); |
There was a problem hiding this comment.
[nitpick] Consider using a higher log level (e.g., warn) for UnauthorizedAccessException to ensure that potentially critical security issues are adequately logged in production.
| log.debug("{} {}", ERROR_LOG_HEADER, e.getMessage(), e); | |
| log.warn("{} {}", ERROR_LOG_HEADER, e.getMessage(), e); |
There was a problem hiding this comment.
@jeeheaG 권한 없는 사용자에 대한 로그를 warn으로 두는게 좋을까요?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/main/java/org/runimo/runimo/exceptions/GlobalExceptionHandler.java (1)
69-74: Fix misleading method name.The method is handling
UserExceptionbut is namedhandleUserJwtException, which appears to be a copy-paste error from the JWT exception handler above.- public ResponseEntity<ErrorResponse> handleUserJwtException(UserException e) { + public ResponseEntity<ErrorResponse> handleUserException(UserException e) {
🧹 Nitpick comments (2)
src/main/java/org/runimo/runimo/config/SecurityConstants.java (1)
1-29: Well-designed constants class with good separation of concerns.The implementation effectively centralizes security constants with appropriate access modifiers and follows the utility class pattern. The separation between common and development-specific endpoints is particularly good for environment-specific configurations.
Consider adding JavaDoc comments to document the purpose of each constant group, especially for
DEV_PUBLIC_ENDPOINTSto clarify when these should be used.src/main/java/org/runimo/runimo/exceptions/GlobalExceptionHandler.java (1)
31-37: Consider internationalizing the error message.The hardcoded Korean message "요청한 리소스를 찾을 수 없습니다." limits the API's usability for non-Korean users. Consider using message keys with internationalization support.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/org/runimo/runimo/auth/exceptions/UnauthorizedAccessException.java(1 hunks)src/main/java/org/runimo/runimo/auth/filters/JwtAuthenticationFilter.java(2 hunks)src/main/java/org/runimo/runimo/auth/filters/UserErrorCode.java(1 hunks)src/main/java/org/runimo/runimo/config/SecurityConfig.java(2 hunks)src/main/java/org/runimo/runimo/config/SecurityConstants.java(1 hunks)src/main/java/org/runimo/runimo/exceptions/CustomAccessDeniedHandler.java(1 hunks)src/main/java/org/runimo/runimo/exceptions/GlobalExceptionHandler.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (5)
src/main/java/org/runimo/runimo/auth/filters/UserErrorCode.java (1)
19-21: LGTM! Well-structured error code addition.The new
INSUFFICIENT_PERMISSIONSerror code is properly placed in the 403 section, follows the established naming convention, and uses appropriate HTTP status code and Korean error messages consistent with the rest of the codebase.src/main/java/org/runimo/runimo/auth/filters/JwtAuthenticationFilter.java (2)
18-18: LGTM! Proper import addition.The import for
AnonymousAuthenticationTokenis necessary for the authentication bypass logic.
44-49: ```shell
#!/bin/bashRefined search for admin endpoint protections
echo "=== antMatchers for admin paths ==="
rg -B2 -A2 "antMatchers.*admin" --type java src/main/java/echo -e "\n=== Role restrictions in SecurityConfig ==="
rg -B2 -A2 "hasRole.*ADMIN" --type java src/main/java/echo -e "\n=== Controller mappings for admin paths ==="
rg -n '@(GetMapping|PostMapping|RequestMapping).*"/.*admin' --type java src/main/java/echo -e "\n=== Role-based annotations on controller methods ==="
rg -n "@(PreAuthorize|Secured|RolesAllowed)" --type java src/main/java/</details> <details> <summary>src/main/java/org/runimo/runimo/config/SecurityConfig.java (2)</summary> `32-78`: **Well-structured security configuration refactoring!** The refactoring effectively separates common security configuration from profile-specific authorization rules, improving maintainability and reducing code duplication. The use of constants instead of hardcoded strings is also a good practice. --- `3-8`: **Verify SecurityConstants definitions.** Ensure that all imported constants from `SecurityConstants` are properly defined and contain the expected values. ```shell #!/bin/bash # Description: Verify that SecurityConstants class exists and contains all the imported constants # Check if SecurityConstants.java exists and examine its content fd -t f "SecurityConstants.java" --exec cat {} \; # Search for the specific constant definitions ast-grep --pattern 'static final $_ ADMIN_ENDPOINT_PATTERN = $_' ast-grep --pattern 'static final $_ ADMIN_ROLE = $_' ast-grep --pattern 'static final $_ USER_ROLE = $_'
src/main/java/org/runimo/runimo/auth/exceptions/UnauthorizedAccessException.java
Show resolved
Hide resolved
src/main/java/org/runimo/runimo/exceptions/CustomAccessDeniedHandler.java
Show resolved
Hide resolved
| @ExceptionHandler(UnauthorizedAccessException.class) | ||
| public ResponseEntity<ErrorResponse> handleUnauthorizedAccessException( | ||
| UnauthorizedAccessException e) { | ||
| log.debug("{} {}", ERROR_LOG_HEADER, e.getMessage(), e); | ||
| return ResponseEntity.status(HttpStatus.FORBIDDEN) | ||
| .body(ErrorResponse.of(e.getErrorCode())); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Increase log level for security violations.
Unauthorized access attempts should be logged at a higher level than debug for security monitoring. Consider using log.warn() or log.info() for better visibility in production logs.
- log.debug("{} {}", ERROR_LOG_HEADER, e.getMessage(), e);
+ log.warn("{} {}", ERROR_LOG_HEADER, e.getMessage(), e);📝 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.
| @ExceptionHandler(UnauthorizedAccessException.class) | |
| public ResponseEntity<ErrorResponse> handleUnauthorizedAccessException( | |
| UnauthorizedAccessException e) { | |
| log.debug("{} {}", ERROR_LOG_HEADER, e.getMessage(), e); | |
| return ResponseEntity.status(HttpStatus.FORBIDDEN) | |
| .body(ErrorResponse.of(e.getErrorCode())); | |
| } | |
| @ExceptionHandler(UnauthorizedAccessException.class) | |
| public ResponseEntity<ErrorResponse> handleUnauthorizedAccessException( | |
| UnauthorizedAccessException e) { | |
| log.warn("{} {}", ERROR_LOG_HEADER, e.getMessage(), e); | |
| return ResponseEntity.status(HttpStatus.FORBIDDEN) | |
| .body(ErrorResponse.of(e.getErrorCode())); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/org/runimo/runimo/exceptions/GlobalExceptionHandler.java around
lines 39 to 45, the log level for unauthorized access exceptions is set to
debug, which is too low for security-related events. Change the log method from
log.debug() to log.warn() or log.info() to increase visibility of these security
violations in production logs.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/runimo/runimo/exceptions/CustomAccessDeniedHandler.java(1 hunks)src/test/java/org/runimo/runimo/security/SecurityConfigDevTest.java(1 hunks)src/test/java/org/runimo/runimo/security/SecurityConfigTest.java(1 hunks)src/test/java/org/runimo/runimo/security/SecurityFilterIntegrationTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/runimo/runimo/exceptions/CustomAccessDeniedHandler.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (9)
src/test/java/org/runimo/runimo/security/SecurityConfigDevTest.java (3)
14-20: Well-structured test class setup.The test class is properly configured with the necessary Spring Boot test annotations for integration testing in the dev profile.
22-30: Correct Swagger accessibility test.The test properly verifies that Swagger endpoints are accessible in the development environment. The 302 redirect for
/swagger-ui.htmlis expected behavior.
32-40: Important security verification test.This test correctly ensures that basic security rules remain enforced in the development environment, preventing unauthorized access to protected API endpoints even when Swagger is accessible.
src/test/java/org/runimo/runimo/security/SecurityFilterIntegrationTest.java (2)
16-22: Proper integration test setup.The test class is correctly configured for integration testing of JWT authentication filters with appropriate annotations and profile selection.
59-67: Correct whitelist endpoint verification.The test properly verifies that whitelisted endpoints bypass JWT filtering by expecting 404 (endpoint not found) rather than JWT authentication errors.
src/test/java/org/runimo/runimo/security/SecurityConfigTest.java (4)
17-23: Comprehensive security test class setup.The test class is well-configured with appropriate annotations for testing security configurations across different scenarios.
25-33: Correct public endpoint accessibility test.The test properly verifies that public endpoints are accessible without authentication, with appropriate status code expectations for each endpoint.
51-74: Excellent role-based authorization testing.The tests thoroughly verify role-based access control:
- USER role can access user endpoints
- USER role is denied admin endpoints (403 Forbidden)
- ADMIN role can access all endpoints
This provides comprehensive coverage of the authorization rules.
76-95: Important error response format validation.These tests ensure consistent error response formats for both unauthorized (401) and forbidden (403) scenarios, verifying proper JSON structure with required fields. This helps maintain API consistency.
src/test/java/org/runimo/runimo/security/SecurityFilterIntegrationTest.java
Show resolved
Hide resolved
|
|
endpoint에 따라 필터링되는지 확인하는 테스트 코드를 추가했습니다. |
2 participants
작업 내역
시큐리티 Config 수정 및 리팩토링
/api/v1/admin에 대한 url기반 권한 필터링 추가 : aec6b40에러코드 추가
핸들러 추가
/exceptions/CustomAccessDeniedHandlerjwt 필터에 AuthContext가 존재하면 필터링 넘어가도록 설정 : f8fd195
@WithMockUser을 사용해도 실행되도록 설정짚고 넘어갈점
SecurityContext가 채워진 경우에는 넘어가도록 기능 추가했어요.
@WithMockUser로 설정가능합니다.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor