-
Notifications
You must be signed in to change notification settings - Fork 1
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
[hoony-lab-issue23] authentication: jwt token 기능 #26
Conversation
`@Transactional은 보통 Class단에 어노테이션으로 먹일 때에는 readOnly = true, CUD 메소드에 먹일 때 readOnly = false로 먹이니 참고만 해주셔용` Co-Authored-By: povia <47127607+povia@users.noreply.github.com>
(Security) Filter > Manager > Provider > UserDetailsService
SECRET_KEY, ACCES_TIME MUST BE modified later
modify `@Transactional`
JwtProviderTest
doFilterInternal
mock `SecurityContext (SecurityContextHolder.getContext())` with setup `webAppContextSetup`, `when(securityContext.getAuthentication()).thenReturn(authentication);`, `SecurityContextHolder.setContext(securityContext);`
Codecov Report
@@ Coverage Diff @@
## hoony-lab #26 +/- ##
===============================================
+ Coverage 90.90% 92.57% +1.66%
- Complexity 54 67 +13
===============================================
Files 10 12 +2
Lines 121 175 +54
Branches 1 3 +2
===============================================
+ Hits 110 162 +52
- Misses 8 9 +1
- Partials 3 4 +1
Continue to review full report at Codecov.
|
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/study/realworld/user/service/UserService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/study/realworld/user/service/UserService.java
Outdated
Show resolved
Hide resolved
src/test/java/com/study/realworld/config/auth/JwtAuthenticationFilterTest.java
Outdated
Show resolved
Hide resolved
src/main/java/com/study/realworld/config/auth/JwtAuthenticationFilter.java
Show resolved
Hide resolved
src/main/java/com/study/realworld/config/auth/JwtAuthenticationFilter.java
Show resolved
Hide resolved
이게 왜 여깄지 Co-Authored-By: DolphaGo <adamdoha@naver.com>
overparam Co-Authored-By: DolphaGo <adamdoha@naver.com>
Co-Authored-By: DolphaGo <adamdoha@naver.com>
Bearer --> Token Co-Authored-By: DolphaGo <adamdoha@naver.com>
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.
LGTM
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 고비를 잘 마무리하신거 같네요 💯
몇가지 코드에 대한 생각 코멘트 남겨두었으니 확인 부탁드려요 :)
public String getSubjectFromToken(String token) { | ||
return this.getClaimsFromToken(token) | ||
.getSubject(); | ||
} | ||
|
||
public boolean isValidToken(String token) { | ||
try { | ||
Claims claims = this.getClaimsFromToken(token); | ||
return true; | ||
} catch (JwtException | NullPointerException e) { | ||
return false; | ||
} | ||
} |
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.
getClaimsFromToken
을 두 메소드에서쓰는데 동시에 Filter에서도 두 메소드를 하나의 flow에서 호출하는 이유가 있을까요?
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.
이렇게 작성한 이유는 100% 제가 짠 코드가 아니기 때문인 듯 싶습니다.
곰곰히 보니까 getClaimsFromToken
함과 동시에 valid 확인을 하면 좋을 것 같습니다 !
public User deleteById(Long id) { | ||
User user = userRepository.findById(id) | ||
.orElseThrow(() -> new NoSuchElementException(id + " not found")); | ||
validateDuplicateUser(user.getEmail()); |
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.
validateDuplicateUser
라는 db내의 동일 email 검증 메소드네요 :)
encodePassword를 하기전에 먼저 email 검증을 하는것이 좋지 않을까요? 아무리 password를 encoding해도 여기서 중복 excpetion이 발생해버린다면, encodePassword가 무의미한 동작이 되어버리는거 같네요.
|
||
return ResponseEntity.ok().body(UserResponse.of(user, token)); | ||
return ResponseEntity.ok() | ||
.body(UserResponse.of(user, "token")); |
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.
.body(UserResponse.of(user, "token")); | |
.body(UserResponse.of(user, ???)); |
토큰 생성 로직을 만드셨는데 추가를 잊으신거같네요 :)
String token = jwtProvider.generateJwtToken(user); | ||
|
||
return ResponseEntity.ok() | ||
.header(HttpHeaders.AUTHORIZATION, "Token " + token) |
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.
현재 리얼월드 스펙에서는 header에 굳이 token을 담아 전달하지 않아도됩니다 :)
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.
호오,,,,, 제가 자꾸 뭘 까먹나보네요 흑흑 참고해서 수정하겠습니다
private static final String EMAIL = "email"; | ||
private static final String TOKEN = "token"; |
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.
static으로 설정한 이유가 있을까요?
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.
음 어디서 본것같아서 무지성 static
이긴 합니다. 지양하는게 좋을까요?
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.
물론 static이 생각할 부분이 적어서 편하기는 한데 해당 클래스를 테스트하지 않는 경우에도 static이 올라가게 되니까 굳이 필요없는 공수를 들이는것같다고 생각해서요 :)
크게 문제되는 사항은 아니긴해요 ㅎㅎ
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.
확장성(다른 테스트 메소드 추가)를 고려하면
static 선언이 약이 될수도 독이 될수도 있을 것 같아요
예를 들어 Controller Test 같은 곳에서 공통되는 uri (/api/users)는 모든 테스트 코드에서 공통적으로 쓰일테니까 URI 같은걸로 선언 해두면 확실히 편할 수 있습니다. swagger에서 사용되는 변수들도 이에 해당될 수도 있겠네요
그런데 파라미터, 변수같은 것들은 웬만하면 비슷하긴 하겠지만 테스트 케이스마다 다른 것을 쓸 수 있어서 애매할 수도 있을 것 같아요. 일단 무지성으로 다 쓰다가 나중에 공통되는 부분들 나오면 나중에 따로 model로 만들어서 가져오는 방법도 있을 것 같네요
물론 항상 애매한것도 아닌것 같아서 그냥 어떠한 방법으로 써도 될 것 같다는.. 그런 의견입니다 ㅎㅎ (사실 저도 지금 테스트 하는데 변수들 하나하나 다 쓰는데 너무 귀찮더라구용)
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.
와우.. 리뷰 드릴게 많이 없습니다.
매직 리터럴이나 매직 스트링만 상수로 변환하는것에 대해서만 리뷰 남겼습니다!
|
||
private final JwtProvider jwtProvider; | ||
|
||
@Override | ||
protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException { | ||
log.info("{} {} : {}", request.getMethod(), request.getRequestURI(), request.getHeader(HttpHeaders.AUTHORIZATION)); | ||
String header = request.getHeader(HttpHeaders.AUTHORIZATION); |
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.
private final JwtProvider jwtProvider; | |
@Override | |
protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException { | |
log.info("{} {} : {}", request.getMethod(), request.getRequestURI(), request.getHeader(HttpHeaders.AUTHORIZATION)); | |
String header = request.getHeader(HttpHeaders.AUTHORIZATION); | |
private static final String DoFilterMessage = "{} {} : {}"; | |
private final JwtProvider jwtProvider; | |
@Override | |
protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException { | |
log.info(DoFilterMessage, request.getMethod(), request.getRequestURI(), request.getHeader(HttpHeaders.AUTHORIZATION)); | |
String header = request.getHeader(HttpHeaders.AUTHORIZATION); |
매직 스트링 제안드립니다!
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.
소스코드에 주로 여러 번 등장하는 일반적인 리터럴 값을 매직 리터럴이라고 한다.
의견 감사드립니다 ! 반영해보도록할게요 ~
delete isValidToken Co-Authored-By: 최진영 <jinyoungchoi95@gmail.com>
Co-Authored-By: 최진영 <jinyoungchoi95@gmail.com>
Co-Authored-By: KimWooJae <50267433+kwj1270@users.noreply.github.com>
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.
수고하셨습니다 몇가지 코멘트 달아두었습니다 👍
private static final String EMAIL = "email"; | ||
private static final String TOKEN = "token"; |
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.
확장성(다른 테스트 메소드 추가)를 고려하면
static 선언이 약이 될수도 독이 될수도 있을 것 같아요
예를 들어 Controller Test 같은 곳에서 공통되는 uri (/api/users)는 모든 테스트 코드에서 공통적으로 쓰일테니까 URI 같은걸로 선언 해두면 확실히 편할 수 있습니다. swagger에서 사용되는 변수들도 이에 해당될 수도 있겠네요
그런데 파라미터, 변수같은 것들은 웬만하면 비슷하긴 하겠지만 테스트 케이스마다 다른 것을 쓸 수 있어서 애매할 수도 있을 것 같아요. 일단 무지성으로 다 쓰다가 나중에 공통되는 부분들 나오면 나중에 따로 model로 만들어서 가져오는 방법도 있을 것 같네요
물론 항상 애매한것도 아닌것 같아서 그냥 어떠한 방법으로 써도 될 것 같다는.. 그런 의견입니다 ㅎㅎ (사실 저도 지금 테스트 하는데 변수들 하나하나 다 쓰는데 너무 귀찮더라구용)
/** | ||
* This exception may occur if matchers are combined with raw values: | ||
* //incorrect: | ||
* someMethod(anyObject(), "raw String"); | ||
* When using matchers, all arguments have to be provided by matchers. | ||
* For example: | ||
* //correct: | ||
* someMethod(anyObject(), eq("String by matcher")); | ||
* | ||
* For more info see javadoc for Matchers class. | ||
*/ |
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/test/java/com/study/realworld/user/service/UserServiceTest.java
Outdated
Show resolved
Hide resolved
Co-authored-by: 서정준 <51807128+chance0523@users.noreply.github.com>
Issue: #23
작업 내용
생성/변경 로직
개인 코멘트
POST /api/users
response 에서의 token 은 구현 안했습니다.@Value
가 될 예정입니다.GET /api/user
에서SecurityContext
를 이렇게 핸들링 해도 되는지 모르겠습니다.