[hoony-lab-issue23] authentication: jwt token 기능 #26

hoony-lab · 2021-08-16T13:22:53Z

Issue: #23

작업 내용

JWT

생성/변경 로직

Security

SecurityConfig

JwtAuthenticationFilter

JwtAuthenticationProvider

JwtUserDetailsService

JwtAuthenticationFilter (OncePerRequestFilter)
JwtProvider

개인 코멘트

저번주도 머리빠지고 이번주도 머리빠지고 다음주도 머리빠질예정이라 미리 PR을 올리겠습니다.

빈 순환참조부터해서 어디선가 자꾸 빼먹고,,, 안되고,,, 엉망진창이였습니다.

~~filter~~, ~~test~~

POST /api/users response 에서의 token 은 구현 안했습니다.
SECRET_KEY 는 곧 @Value 가 될 예정입니다.
GET /api/user 에서 SecurityContext 를 이렇게 핸들링 해도 되는지 모르겠습니다.

`@Transactional은 보통 Class단에 어노테이션으로 먹일 때에는 readOnly = true, CUD 메소드에 먹일 때 readOnly = false로 먹이니 참고만 해주셔용` Co-Authored-By: povia <47127607+povia@users.noreply.github.com>

(Security) Filter > Manager > Provider > UserDetailsService

will delete

hoony-lab · 2021-08-22T18:51:08Z

따흑......

with JwtProvider

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-commenter · 2021-08-26T12:44:34Z

Codecov Report

Merging #26 (97af41c) into hoony-lab (b5adfca) will increase coverage by 1.66%.
The diff coverage is 94.36%.

@@               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

Impacted Files	Coverage Δ
...realworld/config/auth/JwtAuthenticationFilter.java	`84.61% <84.61%> (ø)`
...a/com/study/realworld/config/auth/JwtProvider.java	`92.30% <92.30%> (ø)`
...ava/com/study/realworld/config/SecurityConfig.java	`100.00% <100.00%> (ø)`
.../com/study/realworld/user/service/UserService.java	`100.00% <100.00%> (+10.52%)`	⬆️
...a/com/study/realworld/user/web/UserController.java	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5adfca...97af41c. Read the comment docs.

DolphaGo

몇 가지 의견 드렸습니다. 확인 부탁드릴게요. 거를건 거르셔도 됩니다.ㅎㅎ

build.gradle

src/main/java/com/study/realworld/config/auth/JwtProvider.java

src/main/java/com/study/realworld/user/service/UserService.java

src/main/java/com/study/realworld/user/web/UserController.java

src/test/java/com/study/realworld/config/auth/JwtAuthenticationFilterTest.java

src/main/java/com/study/realworld/config/auth/JwtAuthenticationFilter.java

이게 왜 여깄지 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>

`@Value` constructor Co-Authored-By: DolphaGo <adamdoha@naver.com>

save_validateDuplicateUser_exception login_validateMatchesPassword_exception

DolphaGo

LGTM

jinyoungchoi95

jwt 고비를 잘 마무리하신거 같네요 💯

몇가지 코드에 대한 생각 코멘트 남겨두었으니 확인 부탁드려요 :)

jinyoungchoi95 · 2021-08-30T11:52:57Z

src/main/java/com/study/realworld/config/auth/JwtProvider.java

+    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;
+        }
+    }


getClaimsFromToken을 두 메소드에서쓰는데 동시에 Filter에서도 두 메소드를 하나의 flow에서 호출하는 이유가 있을까요?

이렇게 작성한 이유는 100% 제가 짠 코드가 아니기 때문인 듯 싶습니다.
곰곰히 보니까 getClaimsFromToken 함과 동시에 valid 확인을 하면 좋을 것 같습니다 !

jinyoungchoi95 · 2021-08-30T11:55:49Z

src/main/java/com/study/realworld/user/service/UserService.java

-    public User deleteById(Long id) {
-        User user = userRepository.findById(id)
-                .orElseThrow(() -> new NoSuchElementException(id + " not found"));
+        validateDuplicateUser(user.getEmail());


validateDuplicateUser라는 db내의 동일 email 검증 메소드네요 :)

encodePassword를 하기전에 먼저 email 검증을 하는것이 좋지 않을까요? 아무리 password를 encoding해도 여기서 중복 excpetion이 발생해버린다면, encodePassword가 무의미한 동작이 되어버리는거 같네요.

jinyoungchoi95 · 2021-08-30T11:58:28Z

src/main/java/com/study/realworld/user/web/UserController.java


-        return ResponseEntity.ok().body(UserResponse.of(user, token));
+        return ResponseEntity.ok()
+                .body(UserResponse.of(user, "token"));


Suggested change

.body(UserResponse.of(user, "token"));

.body(UserResponse.of(user, ???));

토큰 생성 로직을 만드셨는데 추가를 잊으신거같네요 :)

jinyoungchoi95 · 2021-08-30T11:59:10Z

src/main/java/com/study/realworld/user/web/UserController.java

+        String token = jwtProvider.generateJwtToken(user);
+
+        return ResponseEntity.ok()
+                .header(HttpHeaders.AUTHORIZATION, "Token " + token)


현재 리얼월드 스펙에서는 header에 굳이 token을 담아 전달하지 않아도됩니다 :)

호오,,,,, 제가 자꾸 뭘 까먹나보네요 흑흑 참고해서 수정하겠습니다

jinyoungchoi95 · 2021-08-30T11:59:43Z

src/test/java/com/study/realworld/config/auth/JwtAuthenticationFilterTest.java

+    private static final String EMAIL = "email";
+    private static final String TOKEN = "token";


static으로 설정한 이유가 있을까요?

음 어디서 본것같아서 무지성 static 이긴 합니다. 지양하는게 좋을까요?

물론 static이 생각할 부분이 적어서 편하기는 한데 해당 클래스를 테스트하지 않는 경우에도 static이 올라가게 되니까 굳이 필요없는 공수를 들이는것같다고 생각해서요 :)
크게 문제되는 사항은 아니긴해요 ㅎㅎ

확장성(다른 테스트 메소드 추가)를 고려하면
static 선언이 약이 될수도 독이 될수도 있을 것 같아요

예를 들어 Controller Test 같은 곳에서 공통되는 uri (/api/users)는 모든 테스트 코드에서 공통적으로 쓰일테니까 URI 같은걸로 선언 해두면 확실히 편할 수 있습니다. swagger에서 사용되는 변수들도 이에 해당될 수도 있겠네요

그런데 파라미터, 변수같은 것들은 웬만하면 비슷하긴 하겠지만 테스트 케이스마다 다른 것을 쓸 수 있어서 애매할 수도 있을 것 같아요. 일단 무지성으로 다 쓰다가 나중에 공통되는 부분들 나오면 나중에 따로 model로 만들어서 가져오는 방법도 있을 것 같네요

물론 항상 애매한것도 아닌것 같아서 그냥 어떠한 방법으로 써도 될 것 같다는.. 그런 의견입니다 ㅎㅎ (사실 저도 지금 테스트 하는데 변수들 하나하나 다 쓰는데 너무 귀찮더라구용)

kwj1270

와우.. 리뷰 드릴게 많이 없습니다.
매직 리터럴이나 매직 스트링만 상수로 변환하는것에 대해서만 리뷰 남겼습니다!

kwj1270 · 2021-08-30T15:10:51Z

src/main/java/com/study/realworld/config/auth/JwtAuthenticationFilter.java

+
+    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);


Suggested change

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

매직 스트링 제안드립니다!

소스코드에 주로 여러 번 등장하는 일반적인 리터럴 값을 매직 리터럴이라고 한다.
의견 감사드립니다 ! 반영해보도록할게요 ~

delete isValidToken Co-Authored-By: 최진영 <jinyoungchoi95@gmail.com>

Co-Authored-By: 최진영 <jinyoungchoi95@gmail.com>

Co-Authored-By: KimWooJae <50267433+kwj1270@users.noreply.github.com>

chance0523

수고하셨습니다 몇가지 코멘트 달아두었습니다 👍

chance0523 · 2021-09-03T10:48:14Z

src/test/java/com/study/realworld/config/auth/JwtAuthenticationFilterTest.java

+    private static final String EMAIL = "email";
+    private static final String TOKEN = "token";


확장성(다른 테스트 메소드 추가)를 고려하면
static 선언이 약이 될수도 독이 될수도 있을 것 같아요

예를 들어 Controller Test 같은 곳에서 공통되는 uri (/api/users)는 모든 테스트 코드에서 공통적으로 쓰일테니까 URI 같은걸로 선언 해두면 확실히 편할 수 있습니다. swagger에서 사용되는 변수들도 이에 해당될 수도 있겠네요

그런데 파라미터, 변수같은 것들은 웬만하면 비슷하긴 하겠지만 테스트 케이스마다 다른 것을 쓸 수 있어서 애매할 수도 있을 것 같아요. 일단 무지성으로 다 쓰다가 나중에 공통되는 부분들 나오면 나중에 따로 model로 만들어서 가져오는 방법도 있을 것 같네요

물론 항상 애매한것도 아닌것 같아서 그냥 어떠한 방법으로 써도 될 것 같다는.. 그런 의견입니다 ㅎㅎ (사실 저도 지금 테스트 하는데 변수들 하나하나 다 쓰는데 너무 귀찮더라구용)

chance0523 · 2021-09-03T10:51:15Z

src/test/java/com/study/realworld/user/service/UserServiceTest.java

+        /**
+         * 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.
+         */


요거 무슨 뜻인지 아시나용? 저는 잘 이해가 안돼서...
좀 어려운 주석이면 나의 말로 바꿔서 써놔도 좋을 것 같아용

src/test/java/com/study/realworld/user/service/UserServiceTest.java

Co-authored-by: 서정준 <51807128+chance0523@users.noreply.github.com>

hoony-lab and others added 4 commits August 14, 2021 13:17

[#12] fix: UserService에 Transactional 수정

444d5a8

`@Transactional은 보통 Class단에 어노테이션으로 먹일 때에는 readOnly = true, CUD 메소드에 먹일 때 readOnly = false로 먹이니 참고만 해주셔용` Co-Authored-By: povia <47127607+povia@users.noreply.github.com>

[#23] build: jjwt dependency 추가

ba81367

[#23] feat: JWT 관련 static util 추가

af12b06

[#23] feat: token 생성&주입 로직 추가

0d80ed6

(Security) Filter > Manager > Provider > UserDetailsService

hoony-lab added the hoony-lab 후니의 이슈 label Aug 16, 2021

hoony-lab self-assigned this Aug 16, 2021

hoony-lab added 3 commits August 23, 2021 03:12

[#23] feat: SuccessHandler and EntryPoint for JwtAuth

9bae468

will delete

[#23] fix: JwtUserDetails to security.User

289daa9

[#23] fix: JwtAuthenticationFilter log

4e24753

hoony-lab added 9 commits August 26, 2021 21:33

[#23] fix: delete unnecessary jwt releated files

a41f4a3

[#23] feat: add JwtAuthenticationFilter extends OncePerRequestFilter

b3672fe

with JwtProvider

[#23] feat: add JwtProvider for Component

c8df5b7

SECRET_KEY, ACCES_TIME MUST BE modified later

[#23] feat: addFilter JwtAuthenticationFilter

2ee1c32

[#23] fix: update UserService

533f645

modify `@Transactional`

[#23]: feat: add getUser in UserController

c117a6f

[#23] test: add JwtProviderTest

a090ace

JwtProviderTest

[#23] test: add JwtAuthenticationFilterTest

bc1850e

doFilterInternal

[#23] test: add GET_api_user_getUser in UserControllerTest

d500e12

mock `SecurityContext (SecurityContextHolder.getContext())` with setup `webAppContextSetup`, `when(securityContext.getAuthentication()).thenReturn(authentication);`, `SecurityContextHolder.setContext(securityContext);`

hoony-lab requested review from kwj1270, jinyoungchoi95, povia, chance0523 and DolphaGo August 26, 2021 12:48

DolphaGo reviewed Aug 27, 2021

View reviewed changes

hoony-lab and others added 4 commits August 30, 2021 15:21

[#23] build: delete unused dependency

f10b793

이게 왜 여깄지 Co-Authored-By: DolphaGo <adamdoha@naver.com>

[#23] fix: validateDuplicateUser param

238d351

overparam Co-Authored-By: DolphaGo <adamdoha@naver.com>

[#23] fix: delete unused method

6e2a63b

Co-Authored-By: DolphaGo <adamdoha@naver.com>

[#23] fix: conform to realworld API spec

53cf055

Bearer --> Token Co-Authored-By: DolphaGo <adamdoha@naver.com>

hoony-lab and others added 3 commits August 30, 2021 15:34

[#23] fix: secret, access-time in JwtProvider

ee671a8

`@Value` constructor Co-Authored-By: DolphaGo <adamdoha@naver.com>

[#23] feat: add UserService expcetion test

70af05c

save_validateDuplicateUser_exception login_validateMatchesPassword_exception

[#23] fix

7f32020

DolphaGo approved these changes Aug 30, 2021

View reviewed changes

jinyoungchoi95 reviewed Aug 30, 2021

View reviewed changes

kwj1270 approved these changes Aug 30, 2021

View reviewed changes

hoony-lab and others added 3 commits September 3, 2021 11:53

[#23] fix: integrate getClaimsFromToken and isValidToken in JwtProvider

829940b

delete isValidToken Co-Authored-By: 최진영 <jinyoungchoi95@gmail.com>

[#23] fix: add generated token in joinUser

3283a57

Co-Authored-By: 최진영 <jinyoungchoi95@gmail.com>

[#23] fix: replace magic literal

199c904

Co-Authored-By: KimWooJae <50267433+kwj1270@users.noreply.github.com>

chance0523 approved these changes Sep 3, 2021

View reviewed changes

[#23] fix: 코드 위치 수정

97af41c

Co-authored-by: 서정준 <51807128+chance0523@users.noreply.github.com>

hoony-lab merged commit 66cb514 into hoony-lab Sep 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[hoony-lab-issue23] authentication: jwt token 기능 #26

[hoony-lab-issue23] authentication: jwt token 기능 #26

hoony-lab commented Aug 16, 2021 •

edited

hoony-lab commented Aug 22, 2021

codecov-commenter commented Aug 26, 2021 •

edited

DolphaGo left a comment

DolphaGo left a comment

jinyoungchoi95 left a comment

jinyoungchoi95 Aug 30, 2021

hoony-lab Aug 30, 2021

jinyoungchoi95 Aug 30, 2021

jinyoungchoi95 Aug 30, 2021

jinyoungchoi95 Aug 30, 2021

hoony-lab Aug 30, 2021

jinyoungchoi95 Aug 30, 2021

hoony-lab Aug 30, 2021

jinyoungchoi95 Aug 30, 2021

chance0523 Sep 3, 2021

kwj1270 left a comment

kwj1270 Aug 30, 2021

hoony-lab Sep 3, 2021

chance0523 left a comment

chance0523 Sep 3, 2021

chance0523 Sep 3, 2021

	.body(UserResponse.of(user, "token"));
	.body(UserResponse.of(user, ???));

		private static final String EMAIL = "email";
		private static final String TOKEN = "token";

[hoony-lab-issue23] authentication: jwt token 기능 #26

[hoony-lab-issue23] authentication: jwt token 기능 #26

Conversation

hoony-lab commented Aug 16, 2021 • edited

Issue: #23

작업 내용

생성/변경 로직

개인 코멘트

hoony-lab commented Aug 22, 2021

codecov-commenter commented Aug 26, 2021 • edited

Codecov Report

DolphaGo left a comment

Choose a reason for hiding this comment

DolphaGo left a comment

Choose a reason for hiding this comment

jinyoungchoi95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwj1270 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chance0523 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoony-lab commented Aug 16, 2021 •

edited

codecov-commenter commented Aug 26, 2021 •

edited