Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements server session-based authentication for the web admin page, replacing custom JWT authentication handlers with more appropriately named ones and adding comprehensive admin authentication infrastructure.
- Refactored JWT authentication components with clearer naming conventions
- Added admin authentication system with session management, CSRF protection, and dedicated handlers
- Introduced admin entity, repository, and controller for CSRF token management
Reviewed Changes
Copilot reviewed 17 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| SpotService.java | Updated error type from UN_LOGIN_ERROR to UNAUTHORIZED_ERROR for consistency |
| GlobalExceptionHandler.java | Fixed package declaration and removed redundant imports |
| ErrorType.java | Added new admin-specific error types and renamed UN_LOGIN_ERROR to UNAUTHORIZED_ERROR |
| WebConfig.java | Added CORS configuration for admin endpoints |
| SecurityConfig.java | Restructured security with separate filter chains for API and admin, added session management |
| JwtTokenProvider.java | Minor formatting cleanup |
| JwtAuthenticationFilter.java | Updated reference to renamed authentication entry point |
| JwtAuthenticationEntryPoint.java | Renamed from CustomJwtAuthenticationEntryPoint and cleaned up formatting |
| JwtAccessDeniedHandler.java | Renamed from CustomAccessDeniedHandler and cleaned up formatting |
| AdminUserDetailsService.java | New service for loading admin user details |
| AdminAuthenticationEntryPoint.java | New entry point for admin authentication failures |
| AdminAccessDeniedHandler.java | New handler for admin access denied scenarios with CSRF support |
| AdminRepository.java | New repository for admin entity operations |
| AdminEntity.java | New entity representing admin users |
| CsrfTokenResponse.java | New response DTO for CSRF token information |
| AdminController.java | New controller providing CSRF token endpoint |
| prepare-commit-msg | Script improvements with better shell practices |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @Configuration | ||
| public class WebConfig implements WebMvcConfigurer { | ||
|
|
||
| @Value("${cors.allowed-origins}") |
There was a problem hiding this comment.
The @value annotation should include a default value or the property should be documented. Consider adding a default value like @Value(\"${cors.allowed-origins:http://localhost:3000}\") to prevent application startup failures if the property is not configured.
| @Value("${cors.allowed-origins}") | |
| @Value("${cors.allowed-origins:http://localhost:3000}") |
| .logoutUrl("/admin/logout") | ||
| .logoutSuccessHandler(adminLogoutSuccessHandler()) | ||
| .invalidateHttpSession(true) | ||
| .deleteCookies("JSESSIONID")) // TODO: 추후 Spring Session – Redis (SESSION) 으로 전환하여 무중단 배포 지원 |
There was a problem hiding this comment.
The TODO comment indicates a pending architectural change to Redis-based sessions. Consider creating a GitHub issue to track this technical debt and include the issue number in the comment for better traceability.
| .deleteCookies("JSESSIONID")) // TODO: 추후 Spring Session – Redis (SESSION) 으로 전환하여 무중단 배포 지원 | |
| .deleteCookies("JSESSIONID")) // TODO (GitHub #42): 추후 Spring Session – Redis (SESSION) 으로 전환하여 무중단 배포 지원 |
|
|
||
| @Bean | ||
| public PasswordEncoder passwordEncoder() { | ||
| return new BCryptPasswordEncoder(12); |
There was a problem hiding this comment.
The BCrypt strength of 12 is quite high and may cause performance issues during authentication. Consider using the default strength (10) unless there's a specific security requirement that justifies the higher computational cost.
| return new BCryptPasswordEncoder(12); | |
| return new BCryptPasswordEncoder(); |
💡 Issue
📄 Description