Skip to content

[Refactor] 불필요한 코드 제거 및 문제점 보완#154

Open
dotae1 wants to merge 1 commit intodevelopfrom
refactor#153/auth
Open

[Refactor] 불필요한 코드 제거 및 문제점 보완#154
dotae1 wants to merge 1 commit intodevelopfrom
refactor#153/auth

Conversation

@dotae1
Copy link
Contributor

@dotae1 dotae1 commented Feb 3, 2026

📌 개요

  • 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 생성하도록 수정

🛠️ 작업 내용

  • 불필요한 코드 제거
  • Filter와 Spring Security 책임 분할
  • Token claim 포함 정보 개선

📌 차후 계획 (Optional)

📌 테스트 케이스

  • 기능 정상 동작 확인
  • 새로운 의존성 추가 여부 확인 (package.json, build.gradle 등)
  • 코드 스타일 및 컨벤션 준수 확인
  • 기존 테스트 통과 여부 확인
  • (구현한 로직 검증을 위해 테스트한 내용을 설명해주세요)

📌 기타 참고 사항

📌 리뷰어가 확인해야 할 추가 내용, 고민한 점, 결정 과정 등


🙏🏻아래와 같이 PR을 리뷰해주세요.

  • PR 내용이 부족하다면 보충 요청해주세요.
  • 코드 스타일이 팀의 규칙에 맞게 작성되었는지, 일관성을 유지하고 있는지 확인해주세요.
  • 코드에 대한 문서화나 주석이 필요한 부분에 적절하게 작성되어 있는지 확인해주세요.
  • 구현된 로직이 효율적이고 올바르게 작성되었는지, 아키텍처를 잘 준수하고 있는지 검토해주세요.
  • 네이밍, 포매팅, 주석 등 코드의 일관성이 유지되고 있는지 확인해주세요.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())) {
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
if(emailOpt.isPresent() && jwtService.isRefreshTokenValid(refreshToken, emailOpt.get())) {
if (emailOpt.isPresent() && roleOpt.isPresent()
&& jwtService.isRefreshTokenValid(refreshToken, emailOpt.get())) {

Copilot uses AI. Check for mistakes.
.asString());
} catch (Exception e) {
log.error("액세스 토큰이 유효하지 않습니다.");
e.getMessage();
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
e.getMessage();
log.error("토큰에서 이메일 추출 실패: {}", e.getMessage());

Copilot uses AI. Check for mistakes.
.withExpiresAt(new Date(now.getTime() + accessTokenExpirationPeriod)) //토큰 만료 시간
// 클레임으로 email만 사용 추가로 더 작성해도 됨
.withClaim(EMAIL_CLAIM, email)
.withClaim("role", role.name())
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
Comment on lines 48 to +49
String accessToken = jwtService.createAccessToken(userDetails.getUsername());
String refreshToken = jwtService.createRefreshToken();
String refreshToken = jwtService.createRefreshToken(userDetails.getUsername());
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
}

/**
* RefreshToken 생성, Claim에 email도 넣지 않는다.
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
* RefreshToken 생성, Claim에 email도 넣지 않는다.
* RefreshToken 생성 메서드.
* 이메일과 역할(Role) Claim에 포함하여 RefreshToken을 생성한다.

Copilot uses AI. Check for mistakes.
.password(password)
.roles(myMember.getRole().name())
.username(email)
.password("")
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +83 to +84
.withClaim(EMAIL_CLAIM, email)
.withClaim("role", role.name())
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant