Skip to content
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/#146/리프레쉬 토큰 도입 #156

Merged
merged 27 commits into from
Sep 26, 2024

Conversation

seongjunnoh
Copy link
Collaborator

@seongjunnoh seongjunnoh commented Sep 22, 2024

📝 요약

refresh token 도입 완료

로그인 성공 시 access token 과 refresh token 발급
-> 인증 인가가 필요한 api 요청은 login 인터셉터가 유효한 access token 을 가지고 있는지 확인
-> access token이 만료된 것을 인터셉터에서 캐치한 경우 403 error를 response 로 전달
-> 프론트단에서 access token 갱신 요청 api의 요청헤더에 만료된 access token, refresh token 을 주입하여 서버단으로 전송
-> 요청 헤더에 포함된 만료된 access token 으로부터 userId get
-> 1. 만약 refresh token의 만료시간이 지난 경우 db에 저장된 해당 user의 refresh token 값을 delete 한 후, 403 error 발생 (refresh token의 유효시간이 지났음을 프론트단에게 알림)
2. 만약 refresh token이 db에 저장되어 있는 값이 아니라면 마찬가지로 해당 user의 refresh token 값을 delete 한 후, 403 error 발생 (refresh token 값이 잘못되었음을 프론트단에게 알림)
3. 위의 두 경우가 아닐 경우, refresh token 값에 문제가 없는 것이므로 해당 user에게 새로운 access token & 새로운 refresh token을 발급
-> db에 새로운 refresh token 값 update & 새로운 access token 과 refresh token 을 프론트단으로 response

commit 들이 너무 많네여,, 2주 넘게 수정에 수정을 거친 코드들이어서 commit 메시지가 많습니다 하하
file changed 에서 한번에 변경된 코드들을 보시길 추천드립니다!!

이슈 번호 : #146

🔖 변경 사항

  1. JpaRepository 를 상속받은 UserRepository, JwtRepository 를 생성하여 활용하였습니다.
    기존의 dao 들은 하나씩 JpaRepository 를 상속받는 repo로 수정해보겠습니다.

  2. 기존의 util 패키지에서 수행하던 영속성 계층으로부터 가져온 데이터가 없을 경우의 예외 처리를, 해당 메서드를 호출한 서비스단에서 책임지도록 코드를 수정했습니다
    기존 utils 를 이용한 코드들 역시 하나씩 수정해보겠습니다.

아직 테스트 코드는 작성 전 입니다.
기존의 구조가 테스트 코드를 짜기에 너무 많은 수정이 필요한것같고, pr이 너무 늦어지는거 같아 일단 기능구현까지만 된 채로 pr 날립니당
추후에 따로 이슈생성해서 작업해보겠습니다

✅ 리뷰 요구사항

📸 확인 방법 (선택)



📌 PR 진행 시 이러한 점들을 참고해 주세요

* P1 : 꼭 반영해 주세요 (Request Changes) - 이슈가 발생하거나 취약점이 발견되는 케이스 등
* P2 : 반영을 적극적으로 고려해 주시면 좋을 것 같아요 (Comment)
* P3 : 이런 방법도 있을 것 같아요~ 등의 사소한 의견입니다 (Chore)

@seongjunnoh seongjunnoh linked an issue Sep 22, 2024 that may be closed by this pull request
1 task
Copy link
Collaborator

@drbug2000 drbug2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 : 토큰 재발급 API와 관련 service 코드들이 왜 "OAuth" domain에 속해 있는지 궁금합니다

@NoArgsConstructor
@AllArgsConstructor
@Builder
public class TokenDTO {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 : TokenPairDTO 처럼 토큰"쌍"을 강조하는 이름이 어떨까요

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 좋은거 같아요. 수정해보겠습니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수정 완료 후 push 했습니다

* 엑세스 토큰 갱신 요청 처리
* -> 엑세스 토큰, 리프레시 토큰 갱신 (RTR 패턴)
*/
@PostMapping("/new-token")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 :

  1. GET 요청은 어떤가요? 내용을 수정한다기 보단 재"발급"을 받는 상황이라 더 적절하지 않을까 싶습니다.
  2. url을 "/token"도 괜찮은 것 같습니다. "자원"의 이름이기도 하고, "재"발급과 헷갈릴 요소도 있을것 같아서

정답은 없는 것 같고 보통 어떻게 사용하는지 저는 모르지만 제안 드려봅니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

음 사실 requestBody 가 없어서 Post 메서드를 사용하는게 맞을지 고민을 했지만, Get 은 뭔가 조회의 역할이 강하다고 생각했고, token의 재발급(== 새로운 토큰의 발급 == 새로운 토큰의 생성)이 목적인 api 요청이라 Post 메서드를 사용했긴 합니다.
마찬가지로 재발급이라는 의미를 담아서 url을 new-token 이라 명명해보았습니다.

이건 어디까지나 저의 의견이었고, restful 한 url 을 위해 뭐가 더 적절할 지 조금 더 생각해보겠습니다!
의견 감사합니다!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 "발급"도 일종의 "조회"(데이터를 가져오는 것)이라고 생각했는데,

  1. GET : 같은 요청을 여러번 해도 같은 결과를 내야함(멱등성)
  2. GET : 데이터를 수정하는 것은 비권장
  3. POST : 데이터를 생성하는가
    의 기준들로 보았을때, POST도 적절하다고 생각합니다.
    /new-token도 의미가 확실한 것 같네요

아무래도 refresh token이 보안 도메인이기도 하고 다른 HTTP 통신들과 다른 흐름을 가지고 있어서 REST API에 딱 맞게 설계하기가 어렵네요

Comment on lines +15 to +23
public class TokenStorage {

@Id @GeneratedValue
@Column(name = "token_storage_id")
private Long tokenStorageId;

@OneToOne
// @Column(name = "user_id")
private User user;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 : findByUser이 주로 사용될 것이 분명하기 때문에 User을 index 설정 해주는 것이 성능 향상에 큰 도움이 될 것이라고 생각합니다.
JPA에서 인덱스 설정하는 블로그 포스트 첨부합니다
https://velog.io/@ljinsk3/JPA%EB%A1%9C-%EC%9D%B8%EB%8D%B1%EC%8A%A4-%EC%82%AC%EC%9A%A9%ED%95%98%EA%B8%B0

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오호 안그래도 저희가 아직 redis 를 도입하지않아 token 같이 자주 변경되는 값을 저장할때의 성능에 대해 의문이 있었는데 첨부해주신 블로그 한번 참고해보겠습니다!
추가로 refresh token이 유효하지 않다면 그냥 프론트단으로 예외만 던지기보다 아예 해당 tuple을 지워버리는것이 db 용량측면에서 낫다고 생각해서 JpaRepo 의 delete 메서드를 호출하기는 합니다.
다만 아직 테스트 코드를 작성하기 전이라 이 부분도 추후에 테스트 코드를 통해 검증해보겠습니다

Comment on lines 105 to 115
public Long getUserIdFromToken(String token, TokenType tokenType) {
try {
Jws<Claims> claims = Jwts.parserBuilder()
.setSigningKey(JWT_LOGIN_SECRET_KEY).build()
.parseClaimsJws(accessToken);
.setSigningKey(choiceSecretKey(tokenType)).build()
.parseClaimsJws(token);
return claims.getBody().get("userId", Long.class);
} catch (ExpiredJwtException e) {
// 만료된 토큰에서 userId 추출
return e.getClaims().get("userId", Long.class);
} catch (JwtException e) {
log.error("[JwtTokenProvider.getJwtPayloadDtoFromToken]", e);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 : TokenType을 파라미터로 받는 만큼 RefreshToken의 경우를 처리(예외처리, 혹은 다른 return값) 해주거나,
그렇게 사용되는 경우가 아예 없다고 한다면, 아예 getUserIdByAccessToken으로 함수를 바꾸는 것도 좋아보입니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

앗 refresh token 으로부터 userId 를 받아서 jwt 갱신 작업을 처리하도록 코드를 구성하다가, 만료된 access 로부터 userId 를 얻고, refresh 에는 아무 데이터를 넣지 않는 방식으로 수정해서 파라미터로 tokenType 이 아직 있는겁니다.
미처 수정하지 못한 부분이긴한데, 나중에 error 가 발생할 여지가 있어보여서 아예 access 의 SecretKey를 사용하도록 메서드를 수정하는게 좋아보이네요!! 수정해보겠습니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수정 완료 후 push 했습니다

@seongjunnoh
Copy link
Collaborator Author

P1 : 토큰 재발급 API와 관련 service 코드들이 왜 "OAuth" domain에 속해 있는지 궁금합니다

인증, 인가 라는 느낌을 담아서 User 가 아니라 OAuth 에 위치시켜보았습니다.
혹시 OAuth 도메인에는 소셜 로그인 관련 코드들을 위치시키고, jwt 관련 코드는 User 도메인으로 이동시키는게 더 낫다고 생각하시나요??

@drbug2000
Copy link
Collaborator

P1 : 토큰 재발급 API와 관련 service 코드들이 왜 "OAuth" domain에 속해 있는지 궁금합니다

인증, 인가 라는 느낌을 담아서 User 가 아니라 OAuth 에 위치시켜보았습니다. 혹시 OAuth 도메인에는 소셜 로그인 관련 코드들을 위치시키고, jwt 관련 코드는 User 도메인으로 이동시키는게 더 낫다고 생각하시나요??

OAuth는 아무래도 소셜로그인 프로토콜 명칭이기 때문에 다른 도메인이 좋다고 생각합니다.
다만 User도메인에 넣을지, 아니면 authorization 도메인을 따로 만들어야 할지는 고민해봐야 할 것 같습니다.

  1. User도메인에 포함 시킨다
    의미는 맞지만 user도메인이 너무 많은 책임을 가지지 않을까 걱정됩니다.
  2. Authorization 도메인을 새로 만든다.
    기존 login url을 다 바꾸려면 API 문서를 수정하고, 프론트엔드 코드들 또한 수정해야하는 소요가 발생합니다.
  3. Authorization 도메인을 새로 만들고, URL을 /user/으로 유지한다.
    1,2의 절충안입니다.

@seongjunnoh
Copy link
Collaborator Author

P1 : 토큰 재발급 API와 관련 service 코드들이 왜 "OAuth" domain에 속해 있는지 궁금합니다

인증, 인가 라는 느낌을 담아서 User 가 아니라 OAuth 에 위치시켜보았습니다. 혹시 OAuth 도메인에는 소셜 로그인 관련 코드들을 위치시키고, jwt 관련 코드는 User 도메인으로 이동시키는게 더 낫다고 생각하시나요??

OAuth는 아무래도 소셜로그인 프로토콜 명칭이기 때문에 다른 도메인이 좋다고 생각합니다. 다만 User도메인에 넣을지, 아니면 authorization 도메인을 따로 만들어야 할지는 고민해봐야 할 것 같습니다.

  1. User도메인에 포함 시킨다
    의미는 맞지만 user도메인이 너무 많은 책임을 가지지 않을까 걱정됩니다.
  2. Authorization 도메인을 새로 만든다.
    기존 login url을 다 바꾸려면 API 문서를 수정하고, 프론트엔드 코드들 또한 수정해야하는 소요가 발생합니다.
  3. Authorization 도메인을 새로 만들고, URL을 /user/으로 유지한다.
    1,2의 절충안입니다.

오호 jwt를 이용한 인증, 인가 코드 (login 포함) 들을 따로 분리하는거도 나쁘지 않은거 같네요. 다만 말씀해주신것처럼 프론트엔드의 엔드포인트 또한 변경해야할 필요가 있을지는 다같이 한번 의논해보는게 좋을꺼같습니다! 오늘 회의때 한번 말 꺼내 보겠습니다

Copy link
Collaborator

@drbug2000 drbug2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수정 내용 깔끔하고 좋습니다

@Turtle-Hwan
Copy link
Member

P3: 혹시 2. refresh token 값이 잘못된 경우 삭제하는 - 이유가 있나요?? 악의적인 유저가 고의로 보내는 경우에 refresh를 재발급 받기 위함인가요?

@@ -1,9 +1,9 @@
package space.space_spring.dto.user;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: GetUserProfileListDto와 PostLoginDto는 request와 response를 다른 파일로 분리하고자 dto 디렉토리에 넣지 않고 빼두신 건가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

음 이 2개의 dto는 내부에 저희가 이전에 도입하기로 결정했던, dto 객체 내부에 static class로 request, response 를 가지고 있는 설계 구조를 포함해서 우선 model 내부의 어디에도 속하지않게 밖으로 빼놓았습니다.
추후에 리펙토링 작업을 거치면 request, response, dto 로 나뉘어 들어가있을것 같습니다!

@@ -1,4 +1,4 @@
package space.space_spring.dao;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: 추후 UserRepository로 병합 예정이신 걸까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵! 현재는 UserDao를 제거할 경우 무수한 컴파일 에러와 마주하므로 일단 repository 디렉토리 내부에 2개의 영속성 계층을 놔두었습니다
리펙토링 후에는 UserRepository로 병합될것 같습니다!

@seongjunnoh
Copy link
Collaborator Author

P3: 혹시 2. refresh token 값이 잘못된 경우 삭제하는 - 이유가 있나요?? 악의적인 유저가 고의로 보내는 경우에 refresh를 재발급 받기 위함인가요?

만약 악의적인 유저가 access token, refresh token을 탈취한 후, 이걸 가지고 access token 갱신 요청을 보낼 경우 db에 저장되는 해당 user의 refresh token 값이 update 됩니다
그런데 기존의 유저는 이 사실을 모르므로 update 이전의 refresh token의 정보만을 알고 있고, 이 토큰을 이용해서 access token 갱신 요청을 보낼 경우 서버단에서는 유효하지 않은 refresh token으로 인식해 errorr를 발생시킵니다.
이때 db에 저장된 해당 유저의 refresh token의 정보를 제거함으로써, 악의적인 유저가 새로 발급한 refresh token도 또한 무효화 시킬 수 있습니다.

-> 이런 플로우를 고려해서 리프레시 토큰값이 유효하지 않을 경우, db에서 토큰을 삭제하는 로직을 추가하였습니다!

@seongjunnoh seongjunnoh merged commit 38ea31d into develop Sep 26, 2024
3 checks passed
@hyunn522 hyunn522 deleted the feat/#146/리프레쉬-토큰-도입 branch September 30, 2024 00:18
seongjunnoh added a commit that referenced this pull request Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feat : 리프레쉬 토큰 도입
4 participants