Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
[FEAT/#16] JWT Provider를 구현합니다. #17
Changes from all commits
39666f2
95f6b13
efd7fde
761b673
f3f2d84
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
제가 기억하기로는 이전에
두가지 방식을 논의했던 것으로 기억합니다 !
그 중 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를 얘기해본다면 아래와 같을텐데
kid
가 할당되어 있을 것이고 이를 통해 식별 (kid가 매칭이 안되면 우리의 예상 접근자가 아니기 때문에 에러를 발생시켜야 함)만약 위와 같은 구조(해당 라이브러리가 JWK 보관 등과 같이 특화된 기능들을 지원하는 이유)를 충족하지 못한다면
단순 시큐리티 호환성이 좋은 것 하나만으로 도입하는 것은 조금 근거가 부족하다고 생각했어요!!
(작은 사이즈의 라이브러리도 아닐뿐더러 Java 자료구조를 잘 사용하면 충분히 보관 기능은 구현 가능할 것 같다는...? 개인적 생각)
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 정도만 들어가면 좋을 것 같은데, 어떻게 생각하시나요 ?!
This file was deleted.
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.
좋습니다 분리하겠습니다 ~ !
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
이 부분도 boolean validExpiration = expiration == null | expiration.before(new Date()); 로 분리할 수 있을 것 같아요!
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.
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.
확인했습니다.. ! 클린코드 클린코드