-
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/#16] JWT Provider를 구현합니다. #17
base: dev
Are you sure you want to change the base?
Conversation
1. ATK 생성 2. RTK 생성 3. 파싱 ( 검증 이후 ) 로 구현했습니다.
|
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.
몇 가지 눈에 띄는 부분 리뷰를 달아놨습니다!!
확인해주시고 다양한 의견 혹은 이견이 있으시다면 가감없이 댓글 남겨주세요!😚
@@ -12,6 +12,9 @@ dependencies { | |||
implementation ("org.springframework.boot:spring-boot-starter-web") | |||
implementation("org.springframework.boot:spring-boot-starter-security") | |||
|
|||
//JWT & Nimbus | |||
implementation ("org.springframework.boot:spring-boot-starter-oauth2-resource-server") |
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.
P2
오 저도 쓰고 싶었던 라이브러리인데요!
다만 조금 우려되는 부분이 한 가지 있습니다!
현재 저희가 구현중인 프로젝트의 경우,
프로덕트 Client 측에서 auth_code
와 함께 로그인 요청이 들어오면 저희가 OAuth 벤더의 Resource Server에 요청하여 사용자 정보를 받아오는 "OAuth2Client"의 역할과 흡사한 상태입니다.
또한 보통 Resource Server를 구현할 때, Jwk 조회 api도 구현해주는데요!
(인증 체계를 활용하는 타 프로덕트 서비스에서 해당 api 호출을 통해 Jwk를 가져와 활용하기 위해서)
어찌보면 OAuth 2 Client의 역할을 겸하고 있는 상황에서 Resource Server의 역할까지 겸하게 된다면 모든 api를 한 WAS에서 설계하고 처리해줘야 하기에
이것이 바람직할지 한 번 같이 논의해보는 것이 좋을 것 같습니다!
(제가 잘못 알고 있는 부분이 있다면 가감없이 지적 부탁드립니다!!)
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.
제가 기억하기로는 이전에
- 비대칭 키 각 팀에 배부
- API 형식으로 pubKey 호출하기
두가지 방식을 논의했던 것으로 기억합니다 !
그 중 1번을 채택하기로 해서 JWK(public key)를 각 서버별로 들고있는 방식으로 이해하고 있었습니다. !
저희가 구현하는 인증 서버는 파싱에 privateKey가 필요하구요.
Oauth2 Client와 Resource Server의 역할을 겸한다.. 가 사실 제가 이해를 못했습니다.. 하하
pub Key를 API로 제공할 경우에 말씀하신건가요??
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.
이번에 짚은 이유는 oauth2-resource-server
라이브러리를 사용했기 때문이에요!!
JWT 기반 인증 흐름에서 위 라이브러리를 사용하게 된다면
해당 프로젝트는 "리소스 서버"로서의 역할을 수행하겠다는 의미가 될거에요!!
그럴 경우, 간단하게 JWT 기반 인증 flow를 얘기해본다면 아래와 같을텐데
- 각 API 서버로 보내는 요청에 클라이언트들은 발급받은 JWT를 같이 전송
- 각 API 서버에서 JWT의 유효성을 검증
- JWT발급자 (우리 서버)가 제공하는 URL에서 JWK 형식으로 된 public key를 다운로드 후 서명 일치 여부 확인 (각 팀에게 Private 키를 배부하는 것은 동일)
- 각 서버마다
kid
가 할당되어 있을 것이고 이를 통해 식별 (kid가 매칭이 안되면 우리의 예상 접근자가 아니기 때문에 에러를 발생시켜야 함)
- 검증 통과시 JWT에서 유저ID를 추출
- 해당 요청이 추출된 유저ID 사용자가 보낸 것임을 알 수 있음 (인증 완료)
만약 위와 같은 구조(해당 라이브러리가 JWK 보관 등과 같이 특화된 기능들을 지원하는 이유)를 충족하지 못한다면
단순 시큐리티 호환성이 좋은 것 하나만으로 도입하는 것은 조금 근거가 부족하다고 생각했어요!!
(작은 사이즈의 라이브러리도 아닐뿐더러 Java 자료구조를 잘 사용하면 충분히 보관 기능은 구현 가능할 것 같다는...? 개인적 생각)
|
||
T parse(final String token) throws IOException; | ||
String generateRefreshToken(final T value); |
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.
P2
Refresh 토큰의 경우, 어떤 값을 활용해서 생성할지 궁금합니다!
본 Provider는 제네릭을 통해 <T>
로 선언해서 위 generateAccessToken(final T value)
경우와 동일한 데이터가 필요해지게 된다는 것인데
AccessToken은 권한 식별 값이 포함되어 있는 값이 필요하기 때문에 파라미터로 T
타입 value를 받았지만
RefreshToken을 생성할 때까지 해당 T
타입 value가 필요할지는 개인적으로 모르겠습니다!!
이 부분 설명해주시면 감사하겠습니다!
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은 AccessToken의 갱신만을 위한 정보를 가지고 생성해야 하는게 목표인데,
전반적으로 제 구현을 보면 ATK ≈ RTK로 이해하고 있었던 것 같습니다 !!
제 생각에는 UserId + Expire 정도만 들어가면 좋을 것 같은데, 어떻게 생각하시나요 ?!
List<String> roles = | ||
value.getAuthorities().stream() | ||
.map(GrantedAuthority::getAuthority) | ||
.collect(Collectors.toUnmodifiableList()); |
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.
P1
RefreshToken에 까지 유저의 권한 정보(주요 정보)가 주입되는 이유가 무엇일까요?
RefreshToken의 쓰임은 그저 "AccessToken 갱신"을 위함이라고 생각합니다.
그렇기에 Subject로 주입되는 Principal 값부터 roles
의 값이 "AccessToken 갱신"용 토큰에 주입되면 바람직하지 않다고 생각합니다.
(현재 구조는 개인적으로 AccessToken을 갱신하기 위해 AccessToken을 활용하는 구조가 되었다고 보여집니다.)
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.
P3
전체적으로 단일 클래스 내부에 함수가 많다고 생각이 듭니다!!
validate 함수의 경우, 특정한 관심사가 존재하는 행위이기 때문에 검증 객체로 어느정도 책임과 역할의 분리가 가능해보이는데
예를 들어 JwtTokenValidator
와 같은 검증 객체를 구현하는 것은 어떻게 생각하시나요?!!
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.
P3
저도 Validator를 분리하는거에 동의합니다!
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.
좋습니다 분리하겠습니다 ~ !
...ort/src/main/java/sopt/makers/authentication/support/util/jwt/provider/JwtTokenProvider.java
Show resolved
Hide resolved
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.
설명적 변수 및 메서드 추출 관련하여 리뷰 남겨보았습니다!
support/src/main/java/sopt/makers/authentication/support/util/code/failure/TokenFailure.java
Show resolved
Hide resolved
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.
P3
저도 Validator를 분리하는거에 동의합니다!
...ort/src/main/java/sopt/makers/authentication/support/util/jwt/provider/JwtTokenProvider.java
Show resolved
Hide resolved
...ort/src/main/java/sopt/makers/authentication/support/util/jwt/provider/JwtTokenProvider.java
Show resolved
Hide resolved
String tokenValue = "mockRefreshToken"; // 실제 JWT 토큰 값 | ||
String subject = claimsSet.getSubject(); // subject 추출 | ||
String issuer = JwtConstant.ISSUER; // issuer 추출 | ||
Instant issuedAt = claimsSet.getIssuedAt(); // issuedAt 추출 | ||
Instant expiresAt = claimsSet.getExpiresAt(); // expiresAt 추출 | ||
Object roles = claimsSet.getClaim("roles"); // roles 클레임 추출 |
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.
p1
여기 주석도 변수 선언을 통해 알 수 있는 부분인 것 같습니다..!
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.
확인했습니다.. ! 클린코드 클린코드
|
||
@Test | ||
@DisplayName("RTK 토큰 생성 테스트") | ||
public void test2() { |
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.
p3
의미있는 이름으로 테스트 이름을 짓는건 어떨까요?!
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.
넵 확인했습니다!
|
||
@Test | ||
@DisplayName("ATK 토큰 생성 테스트") | ||
public void generate1() { |
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.
p3
generateAccessTokenTest
등과 같이 직관적인 테스트 이름을 지으면 좋을 것 같습니다!
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.
넵 수정하겠습니다 !
JwtClaimsSet claimsSet = | ||
JwtClaimsSet.builder() | ||
.subject(userId) | ||
.issuer(JwtConstant.ISSUER) | ||
.issuedAt(now) | ||
.expiresAt(expiration) | ||
.claim("roles", List.of("ROLE_USER")) | ||
.build(); |
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.
p3
작성된 테스트에서 동일한 구조가 여기 말고도 더 보이는데 private 메서드로 추출하는건 어떨까요?
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.
Helper를 만들어두긴 했는데, ClaimSet은 안담긴거 같네요 ! 이 부분도 따로 추출해놓겠습니다 !
// @Test | ||
// @DisplayName("생성된 토큰은 유효해야 한다.") | ||
|
||
// @Test | ||
// @DisplayName("토큰을 파싱할 수 있어야 한다.") | ||
|
||
// @Test | ||
// @DisplayName("파싱된 토큰 안의 Claims에는 User 정보가 담겨 있어야 한다.") |
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.
p4
논의 이후 구현되어야 할 부분이라면 TODO 코멘트 추가하면 좋을 것 같습니다! 나중에 검색하여 어떤 부분이 구현되지 않았는지 추적하기 쉬울 것 같아서요!
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.
넵 확인했습니다 !
Related Issue 🚀
Work Description ✏️
PR Point 📸
build.gradle & Enum 추가
토큰관련해서 발생할 수 있는 에러와 종속성을 추가했습니다. 비대칭키 방식을 이용한 인코딩과 파싱은
Nimbus
를 보통 사용하는데,Oauth2 Server는 해당 라이브러리를 내장하고 스프링 시큐리티랑 호환이 괜찮은거 같습니다.
(시큐리티 컨텍스트에 JWK 보관 가능 등)
공개 & 개인 키 프로퍼티
기존 PropertySource처럼, 주입받아 사용할 수 있게 구현했습니다.
key pem , yml은 노션에 업데이트 할게요 !
Provider 구현
구현은 위와 같이
으로 진행됩니다.
검증시 Issuer는 현재까지는 플그와 함께 사용할 것 같아서 playground/operation 두가지로 설정했습니다. (해당 없을시 예외)
토큰 생성에는 "Bearer " prefix를 추가하고, 파싱시 prefix와 Issuer, expire, subject(id) 를 검증합니다.
다만 subject (id)는 DB접속이 필요한 부분이라 일단은 보류한 상태입니다. 논의하고 구현하면 좋을것 같네요!
또한 토큰 생성에대한 단위테스트는 작성했지만, pem키가 필요하기 때문에, 아직 encoding, decoding된 실제토큰을 테스트하지 못했습니다.
통합테스트 시 구현해보면 좋을 것 같습니다.
토큰 관련 상수
RTK 만료시간, ATK만료시간, prefix, issuer가 들어있습니다 !
빠진 부분이나 수정 필요한 부분 알려주시면 감사하겠습니다 ~