Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the JWT authentication system by removing unnecessary code and improving token management. The main goals are to eliminate hardcoded URL lists, delegate authentication responsibility to Spring Security, and optimize Redis operations by including email and role claims in tokens.
Changes:
- Removed hardcoded NO_CHECK_URL list from JwtAuthenticationProcessingFilter, relying on SecurityConfig for authorization rules
- Added Role claim to both AccessToken and RefreshToken to avoid database queries during authentication
- Added email claim to RefreshToken to eliminate Redis full-scan operations (keys("RT:*"))
- Removed unused code: LoginDto, LoginResponse, PasswordUtil classes and unused imports
- Modified filter logic to always pass through to Spring Security instead of returning errors directly
- Fixed Naver OAuth name extraction to properly access nested response object
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| JwtAuthenticationProcessingFilter.java | Removed hardcoded URL whitelist, removed Redis/MemberRepository dependencies, simplified token validation flow to delegate to Spring Security |
| JwtService.java | Added Role parameter to token creation methods, added email/role claims to RefreshToken, removed unused method (checkSecret), improved error handling |
| LoginSuccessHandler.java | Extract role from authorities and pass to token creation methods, removed unnecessary saveAndFlush call |
| LoginController.java | Updated to use new token creation signature (compilation error - missing Role parameter) |
| OAuth2LoginSuccessHandler.java | Updated to pass Role parameter to token creation methods |
| OAuthAttributes.java | Fixed Naver OAuth name extraction to access nested response object |
| SecurityConfig.java | Removed unused CustomOAuth2UserService field, removed RedisTemplate parameter from filter bean |
| MailService.java | Removed unused MemberRepository dependency and debug log statements |
| LoginDto.java | Deleted unused DTO |
| LoginResponse.java | Deleted unused DTO |
| PasswordUtil.java | Deleted unused utility class |
| CustomJsonUsernamePasswordAuthenticationFilter.java | Added clarifying comment about JSON parsing |
| .DS_Store | macOS system file added (should be in .gitignore) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| //수정된 메서드 사용 (Bearer 포함) | ||
| jwtService.sendAccessTokenAndRefreshToken(response, newAccessToken, newRefreshToken); | ||
| if(emailOpt.isPresent() && jwtService.isRefreshTokenValid(refreshToken, emailOpt.get())) { |
There was a problem hiding this comment.
When refreshToken validation succeeds but roleOpt is empty, the code will call createAccessToken and createRefreshToken with roleOpt.get() which will throw NoSuchElementException. There should be a check to ensure roleOpt.isPresent() before proceeding with token regeneration.
| if(emailOpt.isPresent() && jwtService.isRefreshTokenValid(refreshToken, emailOpt.get())) { | |
| if (emailOpt.isPresent() && roleOpt.isPresent() | |
| && jwtService.isRefreshTokenValid(refreshToken, emailOpt.get())) { |
| .asString()); | ||
| } catch (Exception e) { | ||
| log.error("액세스 토큰이 유효하지 않습니다."); | ||
| e.getMessage(); |
There was a problem hiding this comment.
This error handling is insufficient. Calling e.getMessage() without logging or using the result doesn't provide any useful information. Consider either logging the error message or removing this line entirely since the empty Optional is already being returned.
| e.getMessage(); | |
| log.error("토큰에서 이메일 추출 실패: {}", e.getMessage()); |
| .withExpiresAt(new Date(now.getTime() + accessTokenExpirationPeriod)) //토큰 만료 시간 | ||
| // 클레임으로 email만 사용 추가로 더 작성해도 됨 | ||
| .withClaim(EMAIL_CLAIM, email) | ||
| .withClaim("role", role.name()) |
There was a problem hiding this comment.
This change introduces a breaking change for existing tokens. Any tokens issued before this change won't have the "role" claim, which means extractRole will return empty, and users with old tokens won't be able to authenticate until they get new tokens. Consider adding a migration strategy or handling the case where role claim might be missing from older tokens (e.g., by falling back to database lookup for role if the claim is not present).
| String accessToken = jwtService.createAccessToken(userDetails.getUsername()); | ||
| String refreshToken = jwtService.createRefreshToken(); | ||
| String refreshToken = jwtService.createRefreshToken(userDetails.getUsername()); |
There was a problem hiding this comment.
The LoginController's login method is missing the Role parameter when calling createAccessToken and createRefreshToken. This will cause a compilation error since these methods now require both email and Role parameters. You should extract the role from userDetails.getAuthorities() similar to how it's done in LoginSuccessHandler (lines 37-46).
| } | ||
|
|
||
| /** | ||
| * RefreshToken 생성, Claim에 email도 넣지 않는다. |
There was a problem hiding this comment.
The JavaDoc comment is now outdated. It states "Claim에 email도 넣지 않는다" (does not include email in Claim), but the implementation now adds email and role claims to the RefreshToken. The comment should be updated to reflect the current implementation.
| * RefreshToken 생성, Claim에 email도 넣지 않는다. | |
| * RefreshToken 생성 메서드. | |
| * 이메일과 역할(Role)을 Claim에 포함하여 RefreshToken을 생성한다. |
| .password(password) | ||
| .roles(myMember.getRole().name()) | ||
| .username(email) | ||
| .password("") |
There was a problem hiding this comment.
Setting an empty password in the UserDetails might cause issues with certain security configurations or password validators. Consider using a more explicit placeholder or documenting why an empty password is acceptable here. Since the authentication is already validated via JWT, this is likely safe, but it's worth adding a comment explaining this decision.
| .withClaim(EMAIL_CLAIM, email) | ||
| .withClaim("role", role.name()) |
There was a problem hiding this comment.
Similar to AccessToken, existing RefreshTokens issued before this change won't have email and role claims. When users try to refresh with old tokens, extractEmail and extractRole will return empty, preventing token refresh. This is a breaking change that will force all users to re-login. Consider documenting this breaking change or implementing a graceful migration period.
📌 개요
NO_CHECK_URL 하드코딩 제거
: Filter안에 NO_CHECK_URL 리스트를 관리하는것과 SecurityConfig에서 관리하는것 이중 관리가 일어나 Filter안에 있는 하드코딩된 URL리스트를 제거하였습니다.
JwtAuthenticationFilter에서 AccessToken & RefreshToken을 검증 후 재발급하는 로직을 구성하였으나, Filter에서는 BlackList처리된 토큰 즉 만료된 토큰이 왔을 경우에 대한 차후 대비는 Spring Security쪽에 책임을 넘기도록 하였습니다.
유효 토큰이 없어도 일단 다음 필터로 넘기어 Security에서 판단하도록 수정하였습니다.
RefreshToken 검증 시 keys("RT:*") 명령어로 전체 스캔을 수행하여 성능적인 우려가 있었으나 RefreshToken내부에 email정보를 포함시켜 전체 조회 로직을 제거하였습니다.
사용하지않는 죽은 코드들을 모두 제거하고 불필요한 의존성 & 로그 코드들을 제거하였습니다.
Token에 Claim(Role) 추가함으로써 MemberRepository 매번 조회가 일어나지 않고 토큰의 정보만으로 UserDetail 생성하도록 수정
🛠️ 작업 내용
📌 차후 계획 (Optional)
📌 테스트 케이스
package.json,build.gradle등)📌 기타 참고 사항
📌 리뷰어가 확인해야 할 추가 내용, 고민한 점, 결정 과정 등
🙏🏻아래와 같이 PR을 리뷰해주세요.