-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat #14 인증 인가 로직 구현 #14
The head ref may contain hidden characters: "feat/#13/\uC778\uC99D-\uC778\uAC00-\uB85C\uC9C1-\uAD6C\uD604"
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
구현 고생하셨어요!
제 생각이나 수정하면 좋을 부분 적어 뒀습니다!
경로 관리는 차후 운영할 때 편의성을 생각하면 별도의 파일로 관리해두면 좋을 것 같아요!
src/main/java/com/gachtaxi/global/auth/jwt/filter/JwtAuthenticationFilter.java
Show resolved
Hide resolved
src/main/java/com/gachtaxi/global/auth/jwt/filter/JwtAuthenticationFilter.java
Show resolved
Hide resolved
private final static String JWT_ERROR = "jwtError"; | ||
|
||
@Override | ||
protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 필터에서 JWT 토큰의 유무, 만기 일자만 검증을 하고 토큰의 유효성 검증은 빠져있는 것 같아요!
별도로 토큰에 대한 유효성 검증이 jwtExtractor.getEmail
여기서 일어나고 있는 걸까요!?
필터에서는 조금 더 명시적으로 validate
등의 메서드로 유효성 검증이 표현이 되면 좋을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
맞습니다!
조금 더 정확히는 getEmail(), getRole(), isExpired() 등의 메서드가 호출하는 getClaimFromToken()의 parseClaims()에서 유효성 검증이 수행됩니다.
이 메서드는 jwtToken의 유효성 검증 및 디코딩을 수행 후 Claims를 반환합니다!
private Claims parseClaims(String token) {
try{
JwtParser parser = Jwts.parserBuilder()
.setSigningKey(key)
.build();
return parser.parseClaimsJws(token).getBody();
}catch (JwtException e){
throw new TokenInvalidException();
}
}
저는 토큰의 값을 꺼내는 전제 조건이 유효한 토큰
이라고 생각했습니다.
Email, Role, Expiration과 같은 Claims를 반환할 때 유효성 검증이 수행되는 JwtParser의 parseClaimsJws를 사용했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그럼 해당 메서드에 유효성 검증이라는 의미가 들어나도 좋을 것 같아요!
현재 필터만 봤을 때 검증의 역할을 하는 메서드가 확인이 안돼서 jwtExtractor
로 내려가서 확인을 할 때 해당 클래스에서도 검증
을 하는 메서드는 보이지 않아서 메서드 명이 validateToken 등으로 수정이 되는 것도 좋을 것 같아요
어떻게 보면 추출의 역할은 이 메서드를 호출하는 쪽에서 진행하는 것이니 이 메서드의 책임은 검증
이라고 봐도 괜찮을 것 같아서요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그리고 갑자기 든 생각인데, 현재 jwtExtractor에 구현된 추출 메서드 들은 매번 parseClaims를 거쳐야하는데, 이것이 조금 비 효율적일 수도 있겠다는 생각이 들었어요
가장 초기에 토큰 검증 후에 추출 추출 추출 이렇게 사용할 수 있는게 아니라 토큰 검증+추출 이다보니 id, email 등이 같이 필요할 때는 매번 검증을 할 필요는 없지 않나 라는 생각이 들었습니다!
src/main/java/com/gachtaxi/global/auth/jwt/exception/TokenInvalidException.java
Outdated
Show resolved
Hide resolved
src/main/java/com/gachtaxi/global/auth/jwt/util/JwtExtractor.java
Outdated
Show resolved
Hide resolved
import org.springframework.stereotype.Component; | ||
|
||
@Component | ||
public class PermitUrlConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전 이렇게 관리하는 것도 좋을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋습니다!
다른 분들 의견도 궁금하네요!
src/main/java/com/gachtaxi/global/auth/jwt/filter/JwtAuthenticationFilter.java
Outdated
Show resolved
Hide resolved
…nto feat/#13/인증-인가-로직-구현 Merge dev branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다 !!
src/main/java/com/gachtaxi/global/auth/jwt/annotation/CurrentMember.java
Outdated
Show resolved
Hide resolved
|
||
@Retention(RetentionPolicy.RUNTIME) | ||
@Target(ElementType.PARAMETER) | ||
@AuthenticationPrincipal(expression = "#this == 'anonymousUser' ? null : id") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CurrentMemberId 부분도 위 코멘트 내용과 동일합니다 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리뷰 내용을 참고해 코드를 수정해봤는데 검토 부탁드립니다!!
import static com.gachtaxi.global.auth.jwt.annotation.CurrentMemberId.*;
@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.PARAMETER)
@AuthenticationPrincipal(expression = "#this == '" + ANONYMOUS_USER + "' ? null : id")
public @interface CurrentMemberId {
String ANONYMOUS_USER = "anonymousUser";
}
제가 찾은 그나마 깔끔한 방법입니다.
id는 문자열이 아니라 id 자체를 반환해야하므로 String으로 상수화 시키면 null이 반환되더군요!
위 방법은 하드코딩을 지양하고 상수화를 거쳤다는 장점이 있습니다.
그런데 문자열과 + 연산을 사용해 문자열이 계속 생기게 되어 메모리 성능 상 좋지 않을 거 같습니다
그리고 일부는 상수화 일부는 id라는 값 자체를 사용해서 뭔가 어색하기도 하네요....
'anonymousUser'는 @CurrentMemberId 안에서만 사용되기 때문에 유지보수에도 그렇게 어렵지 않을 거 같다는 생각도 듭니다!
혹시 더 나은 개선 방법이 있다면 알려주실 수 있을까요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
anonymousUser를 상수화하는 방식 자체에 대해서는 긍정적으로 생각합니다.
상수화를 통해서 하드코딩된 문자열을 제거하는 방식만으로 진행을 해도 유지보수성이 향상될 수 있다고 생각했습니다.
근데 문자열을 + 연산자로 이어붙이는 방식이 저희는 어노테이션을 사용하는 메소드가 빈번하게 호출될 경우가 있을 수도 있어,
저도 이런 경우에 연산이 성능에 미치게 될 수도 있다고 판단했습니다. 큰 차이는 없다고 생각되긴하지만 최적화를 위해서는 + 연산자 보다는 String.format()이나 StringBuilder를 이용하는 방법이 더 적절하지 않을까 하는 생각은 있습니다 !
src/main/java/com/gachtaxi/global/auth/jwt/filter/JwtAuthenticationFilter.java
Show resolved
Hide resolved
src/main/java/com/gachtaxi/global/auth/jwt/user/JwtUserDetails.java
Outdated
Show resolved
Hide resolved
src/main/java/com/gachtaxi/global/auth/jwt/service/JwtService.java
Outdated
Show resolved
Hide resolved
…nto feat/#13/인증-인가-로직-구현 feat: Merge dev branch
@@ -26,4 +30,13 @@ public ApiResponse<OauthKakaoResponse> kakaoLogin(@RequestParam("code") String a | |||
: UN_REGISTER; | |||
return ApiResponse.response(HttpStatus.OK, OAUTH_STATUS.getMessage(), res); | |||
} | |||
|
|||
@PostMapping("/refresh") | |||
public ApiResponse<Void> reissueRefreshToken(HttpServletRequest request, HttpServletResponse response) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
헤더에서 토큰을 추출할 때 request를 직접 넘겨주기 보다는 @CookieValue, @RequestHeader 등의 어노테이션을 사용해서 쿠키를 가져올 수 있을 것 같아요!
저도 쿠키는 가져와보진 못했지만, 헤더에서 값을 빼올 때는 @RequestHeader를 사용해서 빼오는 것이 해당 컨트롤러에서 필요로 하는 값이 한 눈에 보여서 조은 것 같습니다
private final static String JWT_ERROR = "jwtError"; | ||
|
||
@Override | ||
protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그리고 갑자기 든 생각인데, 현재 jwtExtractor에 구현된 추출 메서드 들은 매번 parseClaims를 거쳐야하는데, 이것이 조금 비 효율적일 수도 있겠다는 생각이 들었어요
가장 초기에 토큰 검증 후에 추출 추출 추출 이렇게 사용할 수 있는게 아니라 토큰 검증+추출 이다보니 id, email 등이 같이 필요할 때는 매번 검증을 할 필요는 없지 않나 라는 생각이 들었습니다!
setHeader(jwtToken.accessToken(), response); | ||
setCookie(jwtToken.refreshToken(), response); | ||
} | ||
|
||
public JwtTokenDto reissueJwtToken(HttpServletRequest request) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reissueJwtToken
은 토큰 재생성 + 재발급의 책임을 가져야한다고 생각합니다.
따라서 해당 메서드는 하위 private 메서드들을 호출하는 파사드의 역할을 수행하고, 하위 메서드로 토큰 추출, 검증, 발급, setHeader를 수행해서 컨트롤러에서 jwtService.setHeader
, jwtService.setCookie
를 호출 해주기 보다는 서비스 클래스에서 전부 수행한 후에 반환해주는 방식도 좋을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그리고 현재 토큰 재발급시 이전에 저장된 리프레시 토큰을 삭제하진 않는 것으로 보이는데 특별한 이유가 있을까요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RefreshToken을 재발급 받고 Redis에 저장 시 덮어쓰기 되어 삭제하지 않았습니다!
📌 관련 이슈
관련 이슈 번호 #13
Close #13
추가 내용
프론트 측과 상의 후 회원가입 플로우가 변경됐습니다.
현재 PR이 치명적 오류는 없고 코드 개선상 리뷰가 많아 팀원들의 동의 하에 일단 병합하고 추후 개선하도록 하겠습니다.
🚀 작업 내용
JwtAuthentication Filter 구현
CustomAuthenticationEntryPoint 구현
필터 단에서 일어나는 Authentication 관련 예외를 처리하기 위해 CustomAuthenticationEntryPoint 구현
CustomAccessDeniedHandler 구현
필터 단에서 일어나는 Authorization 관련 예외 (인가)를 처리하기 위해 CustomAccessDeniedHandler 구현
CustomAnnotation 구현
컨트롤러 단에서 인증된 로그인 객체 정보를 가져오기 위해서 @CurrentMember와 @CureentMemberId를 구현했습니다.
토큰 재발급 구현
📸 스크린샷
크롬 테스트 CORS 설정
AccessToken과 RefreshToken이 크롬에서도 잘 전달 되는지 테스트 (CORS)
헤더에 토큰이 없는 경우
토큰에 권한이 적절치 않은 경우 (인가)
커스텀 어노테이션 @CurrentMember, @CurrentMemberId
토큰 재발급
로컬 Redis에서 재발급 전과 후 RefreshToken 값 비교 (Refresh 토큰 재발급 - RTR)
AccessToken도 같이 재발급 되는 결과
📢 리뷰 요구사항
SpringSecurityConfig
항상 잘 설정한거 같아도 프론트와 연동 시 CORS에서 문제가 있더군요.
제가 검토했을 때 빼먹은 부분은 보이지 않았지만, 리뷰어 분들이 봤을 때 부족한 부분이 있다면 말씀해주시면 감사하겠습니다!
PermitUrlConfig
기존에는 인가별 경로 설정을 SecurityConfig에 직접 적어줬습니다.
가독성이 좋지 않고 지저분하다고 느꼈습니다.
따라서 인가 별 경로를 PermitUrlConfig 파일에 만들어 사용하려고 합니다.
SpringSecurityConfig에서 가장 자주 변경되는 경로 허용 부분인데요. (다른 세팅은 잘 변경하지 X)
이를 SpringSecurityConfig에서
"/auth/**","/api/**","/login"
이런 식으로 문자열을 계속 적는 방식보다는따로 분리하여 관리하면 더 깔끔할거라 생각했습니다!
위 방식처럼 인가 별 경로를 관리하고자 하는데 어떻게 생각하시는 지 궁금합니다!