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.

Sign up for GitHub

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/#385 토큰을 관리하는 방식 수정 #424

Merged
merged 7 commits into from
Sep 21, 2023
Merged

Feat/#385 토큰을 관리하는 방식 수정 #424

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8a02476
refactor: refreshToken으로만 accessToken을 재발급해주던 방식 수정
seokhwan-an Sep 19, 2023
f9f5d3d
feat: scheduler를 통해 InMemoryTokenPairRepository를 갱신하는 기능 추가
seokhwan-an Sep 19, 2023
6d9071c
refactor: 테스트 통과되지 않았던 오류 수정
seokhwan-an Sep 19, 2023
d83945b
refactor: 코드 리뷰 반영
seokhwan-an Sep 20, 2023
4d5282c
refactor: 서브모듈 최신화
seokhwan-an Sep 20, 2023
850dc5e
Merge branch 'main' into feat/#385
seokhwan-an Sep 20, 2023
8c5f57b
Merge branch 'main' into feat/#385
seokhwan-an Sep 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions backend/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ dependencies {

testImplementation 'org.springframework.boot:spring-boot-starter-test'
testImplementation 'io.rest-assured:rest-assured:5.3.1'
testImplementation 'org.awaitility:awaitility:4.2.0'

//log to slack
implementation 'com.github.maricn:logback-slack-appender:1.4.0'
Expand Down
13 changes: 8 additions & 5 deletions backend/src/main/java/shook/shook/auth/application/AuthService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@
import shook.shook.auth.application.dto.GoogleMemberInfoResponse;
import shook.shook.auth.application.dto.ReissueAccessTokenResponse;
import shook.shook.auth.application.dto.TokenPair;
import shook.shook.auth.repository.InMemoryTokenPairRepository;
import shook.shook.member.application.MemberService;
import shook.shook.member.domain.Member;
import shook.shook.member.domain.Nickname;

@RequiredArgsConstructor
@Service
Expand All @@ -18,6 +18,7 @@ public class AuthService {
private final MemberService memberService;
private final GoogleInfoProvider googleInfoProvider;
private final TokenProvider tokenProvider;
private final InMemoryTokenPairRepository inMemoryTokenPairRepository;

public TokenPair login(final String authorizationCode) {
final GoogleAccessTokenResponse accessTokenResponse =
Expand All @@ -34,16 +35,18 @@ public TokenPair login(final String authorizationCode) {
final String nickname = member.getNickname();
final String accessToken = tokenProvider.createAccessToken(memberId, nickname);
final String refreshToken = tokenProvider.createRefreshToken(memberId, nickname);
inMemoryTokenPairRepository.addOrUpdateTokenPair(refreshToken, accessToken);
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

return new TokenPair(accessToken, refreshToken);
}

public ReissueAccessTokenResponse reissueAccessTokenByRefreshToken(final String refreshToken) {
public ReissueAccessTokenResponse reissueAccessTokenByRefreshToken(final String refreshToken, final String accessToken) {
final Claims claims = tokenProvider.parseClaims(refreshToken);
final Long memberId = claims.get("memberId", Long.class);
final String nickname = claims.get("nickname", String.class);
memberService.findByIdAndNicknameThrowIfNotExist(memberId, new Nickname(nickname));

final String accessToken = tokenProvider.createAccessToken(memberId, nickname);
return new ReissueAccessTokenResponse(accessToken);
inMemoryTokenPairRepository.validateTokenPair(refreshToken, accessToken);
final String reissuedAccessToken = tokenProvider.createAccessToken(memberId, nickname);
inMemoryTokenPairRepository.addOrUpdateTokenPair(refreshToken, reissuedAccessToken);
return new ReissueAccessTokenResponse(reissuedAccessToken);
}
}
28 changes: 28 additions & 0 deletions backend/src/main/java/shook/shook/auth/application/TokenPairScheduler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package shook.shook.auth.application;

import java.util.Set;
import lombok.RequiredArgsConstructor;
import org.springframework.scheduling.annotation.Scheduled;
import org.springframework.stereotype.Component;
import shook.shook.auth.exception.TokenException;
import shook.shook.auth.repository.InMemoryTokenPairRepository;

@RequiredArgsConstructor
@Component
public class TokenPairScheduler {

private final TokenProvider tokenProvider;
private final InMemoryTokenPairRepository inMemoryTokenPairRepository;

@Scheduled(cron = "${schedules.cron}")
public void removeExpiredTokenPair() {
final Set<String> refreshTokens = inMemoryTokenPairRepository.getTokenPairs().keySet();
for (String refreshToken : refreshTokens) {
try {
tokenProvider.parseClaims(refreshToken);
} catch (TokenException.ExpiredTokenException e) {
inMemoryTokenPairRepository.delete(refreshToken);
}
}
}
}
4 changes: 2 additions & 2 deletions backend/src/main/java/shook/shook/auth/application/dto/ReissueAccessTokenResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@
import lombok.AccessLevel;
import lombok.AllArgsConstructor;
import lombok.Getter;
import lombok.RequiredArgsConstructor;
import lombok.NoArgsConstructor;

@Schema(description = "액세스 토큰 재발급 응답")
@AllArgsConstructor
@RequiredArgsConstructor(access = AccessLevel.PRIVATE)
@NoArgsConstructor(access = AccessLevel.PRIVATE)
@Getter
public class ReissueAccessTokenResponse {

Expand Down
22 changes: 22 additions & 0 deletions backend/src/main/java/shook/shook/auth/exception/TokenException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,26 @@ public ExpiredTokenException(final Map<String, String> inputValuesByProperty) {
super(ErrorCode.EXPIRED_TOKEN, inputValuesByProperty);
}
}

public static class RefreshTokenNotFoundException extends TokenException {

public RefreshTokenNotFoundException() {
super(ErrorCode.INVALID_REFRESH_TOKEN);
}

public RefreshTokenNotFoundException(final Map<String, String> inputValuesByProperty) {
super(ErrorCode.INVALID_REFRESH_TOKEN, inputValuesByProperty);
}
}

public static class TokenPairNotMatchingException extends TokenException {

public TokenPairNotMatchingException() {
super(ErrorCode.TOKEN_PAIR_NOT_MATCHING_EXCEPTION);
}

public TokenPairNotMatchingException(final Map<String, String> inputValuesByProperty) {
super(ErrorCode.TOKEN_PAIR_NOT_MATCHING_EXCEPTION, inputValuesByProperty);
}
}
}
38 changes: 38 additions & 0 deletions backend/src/main/java/shook/shook/auth/repository/InMemoryTokenPairRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package shook.shook.auth.repository;

import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import org.springframework.stereotype.Repository;
import shook.shook.auth.exception.TokenException;
import shook.shook.auth.exception.TokenException.TokenPairNotMatchingException;

@Repository
public class InMemoryTokenPairRepository {

private final Map<String, String> tokenPairs = new ConcurrentHashMap<>();

Comment on lines +9 to +13
Copy link
Collaborator

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.

db에 저장하는 대신 인메모리를 이용한 이유는 다음과 같습니다.

  1. 현재 저희 서버는 1대로 scale-out이 일어나지 않는 상황입니다.
  2. 메모리의 단점은 휘발성이라는 것인데 현재 이 토큰 정보과 휘발 되었다 하더라도 다시 로그인을 하는 것외에 서비스에 영향이 없다고 생각을 했습니다.
  3. 메모리의 장점으로는 db에 접근해서 데이터를 지우거나 추가하는 것보다 비용이 적다는 것이었습니다. 특히 이 부분은 스케줄로 관리를 해야하는 부분이기에 주기적으로 지우는 작업이 발생하기에 메모리가 더 좋다고 생각했습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

납득완. 좋습니다!!!!

public void validateTokenPair(final String refreshToken, final String accessToken) {
if (!tokenPairs.containsKey(refreshToken)) {
throw new TokenException.RefreshTokenNotFoundException(Map.of("wrongRefreshToken", refreshToken));
}
if (!tokenPairs.get(refreshToken).equals(accessToken)) {
throw new TokenPairNotMatchingException(Map.of("wrongAccessToken", accessToken));
}
}

public void addOrUpdateTokenPair(final String refreshToken, final String accessToken) {
tokenPairs.put(refreshToken, accessToken);
}

public void delete(final String refreshToken) {
tokenPairs.remove(refreshToken);
}

public Map<String, String> getTokenPairs() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

방어적 복사를 적용하는 건 어떨까요?

Copy link
Collaborator Author

@seokhwan-an seokhwan-an Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

먼저 방어적 복사를 이용하지 않았던 이유는 방어적 복사를 이용하게 되서 원본과 복사본의 참조가 끊어서 복사본의 변형이 원본이 반영되지 않게 되는데 저는 현재 이 getTokenPairs()를 scheduler에서 이용해서 외부에 변화가 원본에도 적용이 되어야해서 방어적 복사를 이용하지 않았습니다.

return tokenPairs;
}

public void clear() {
tokenPairs.clear();
}
}
13 changes: 9 additions & 4 deletions backend/src/main/java/shook/shook/auth/ui/AccessTokenReissueController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package shook.shook.auth.ui;

import lombok.RequiredArgsConstructor;
import org.springframework.http.HttpHeaders;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.CookieValue;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.RequestHeader;
import org.springframework.web.bind.annotation.RestController;
import shook.shook.auth.application.AuthService;
import shook.shook.auth.application.dto.ReissueAccessTokenResponse;
Expand All @@ -16,18 +18,21 @@ public class AccessTokenReissueController implements AccessTokenReissueApi {

private static final String EMPTY_REFRESH_TOKEN = "none";
private static final String REFRESH_TOKEN_KEY = "refreshToken";
private static final String TOKEN_PREFIX = "Bearer ";

private final AuthService authService;

@GetMapping("/reissue")
@PostMapping("/reissue")
Copy link
Collaborator

Choose a reason for hiding this comment

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

reissue 요청이 POST로 바뀐 이유가 무엇인가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

처음에는 단순하게 accessToken을 가져온다라고 생각을 했는데 이제는 accessToken을 재발급하고 메모리에 저장과정이 발생하여 저장소를 변경하는 작업이 발생합니다. 그래서 get보다는 post가 적절하다고 생각했습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

그런데 사실 HTTP 요청을 보내는 입장에서 보면, "내가 제공한 refresh Token 정보를 기반으로 accessToken을 발급 받아온다!" 이기 때문에 서버에서 저장소에 저장을 하고, 말고는 사실 중요한 내용이 아닐 것 같다는 생각도 들어요.

HTTP Method를 클라이언트 입장에서 보는지, 서버 입장에서 보는지의 차이인 것 같아요. 더 이야기 해보고 팀 내에서 정해보면 좋을 것 같습니다! :)
https://homoefficio.github.io/2019/12/25/GET%EC%9D%B4%EB%83%90-POST%EB%83%90-%EA%B7%B8%EA%B2%83%EC%9D%B4-%EB%AC%B8%EC%A0%9C%EB%A1%9C%EB%8B%A4/

Copy link
Collaborator

Choose a reason for hiding this comment

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

저도 바론의 의견에 동의합니다. 물론 서버 입장에서는 저장을 하는 거지만, 클라이언트는 결국 값을 받기만 하는거니까요!
추가적으로 데이터를 보낸다기보다는 토큰만 보내는 거라 POST 형식이 맞는지는 좀 더 이야기해보면 좋을 것 같아요.

public ResponseEntity<ReissueAccessTokenResponse> reissueAccessToken(
@CookieValue(value = REFRESH_TOKEN_KEY, defaultValue = EMPTY_REFRESH_TOKEN) final String refreshToken
@CookieValue(value = REFRESH_TOKEN_KEY, defaultValue = EMPTY_REFRESH_TOKEN) final String refreshToken,
@RequestHeader(HttpHeaders.AUTHORIZATION) final String authorization
) {
if (refreshToken.equals(EMPTY_REFRESH_TOKEN)) {
throw new AuthorizationException.RefreshTokenNotFoundException();
}
final String accessToken = authorization.split(TOKEN_PREFIX)[1];
Copy link
Collaborator

Choose a reason for hiding this comment

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

accessToken을 header에서 가져오는 작업이 Controller에서 진행되도록 설정하신 이유가 궁금해요!
그리고 만약 헤더가 Bearer가 아니라면 어떤 결과가 생기나요??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. header를 다루는 것은 httpMessage를 다루는 것이라고 생각을 해서 service가 아닌 present layer인 controller에서 가공하는 것을 택했습니다:)
  2. header가 Bearer인지는 interceptor에서 처리가 되고 있어서 controller 단에서는 처리할 필요가 없다고 생각을 해서 처리를 하지 않았습니다.

Copy link
Collaborator

@somsom13 somsom13 Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

기존의 TokenHeaderExtractor에서도 지금 controller와 유사하게 'Bearer ` 상수를 가지고 있고 토큰을 파싱하는 역할인 것 같아서 여쭤봤어요. 토큰 헤더가 수정되는 일이 생기면 수정해야 할 파일이 많아질 것 같더라구요 🤔 아코는 어떻게 생각하시나요?

Copy link
Collaborator Author

@seokhwan-an seokhwan-an Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

좋은 의견이네요 저는 깊게 고민을 하지 못했던 것 같습니다👍 이 부분 TokenHeaderExtractor를 통해 Bearer를 분리하도록 수정하겠습니다. 현재 인자가 달라서 이 부분은 따로 수정하지 않는 방안으로 진행하겠습니다.

final ReissueAccessTokenResponse response =
authService.reissueAccessTokenByRefreshToken(refreshToken);
authService.reissueAccessTokenByRefreshToken(refreshToken, accessToken);

return ResponseEntity.ok(response);
}
Expand Down
26 changes: 19 additions & 7 deletions backend/src/main/java/shook/shook/auth/ui/openapi/AccessTokenReissueApi.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@

import io.swagger.v3.oas.annotations.Operation;
import io.swagger.v3.oas.annotations.Parameter;
import io.swagger.v3.oas.annotations.Parameters;
import io.swagger.v3.oas.annotations.responses.ApiResponse;
import io.swagger.v3.oas.annotations.tags.Tag;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.RequestHeader;
import shook.shook.auth.application.dto.ReissueAccessTokenResponse;

@Tag(name = "AccessTokenReissue", description = "액세스 토큰 재발급 API")
Expand All @@ -19,13 +21,23 @@ public interface AccessTokenReissueApi {
responseCode = "200",
description = "액세스 토큰 재발급 성공"
)
@Parameter(
name = "refreshToken",
description = "리프레시 토큰",
required = true
@Parameters(
value = {
@Parameter(
name = "refreshToken",
description = "리프레시 토큰",
required = true
),
@Parameter(
name = "authorization",
description = "authorization 헤더",
required = true
)
}
)
@GetMapping("/reissue")
@PostMapping("/reissue")
ResponseEntity<ReissueAccessTokenResponse> reissueAccessToken(
final String refreshToken
final String refreshToken,
@RequestHeader("Authorization") final String authorization
);
}
2 changes: 2 additions & 0 deletions backend/src/main/java/shook/shook/globalexception/ErrorCode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ public enum ErrorCode {
REFRESH_TOKEN_NOT_FOUND_EXCEPTION(1006, "accessToken 을 재발급하기 위해서는 refreshToken 이 필요합니다."),
ACCESS_TOKEN_NOT_FOUND(1007, "accessToken이 필요합니다."),
UNAUTHENTICATED_EXCEPTION(1008, "권한이 없는 요청입니다."),
INVALID_REFRESH_TOKEN(1009, "존재하지 않는 refreshToken 입니다."),
TOKEN_PAIR_NOT_MATCHING_EXCEPTION(1010, "올바르지 않은 TokenPair 입니다."),

// 2000: 킬링파트 - 좋아요, 댓글

Expand Down
1 change: 1 addition & 0 deletions backend/src/main/resources/application-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,5 +42,6 @@ excel:
song-length-suffix: "s"

schedules:
cron: 0/1 * * * * *
in-memory-song:
cron: "0/1 * * * * *" # 1초
3 changes: 3 additions & 0 deletions backend/src/main/resources/application.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ excel:
killingpart-data-delimiter: " "
song-length-suffix: "초"

# Properties Only For local
schedules:
cron: "0 0 0/1 * * *"
schedules:
in-memory-song:
cron: "0 0 0/1 * * *" #1시간
29 changes: 19 additions & 10 deletions backend/src/test/java/shook/shook/auth/application/AuthServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.when;

Expand All @@ -17,6 +18,7 @@
import shook.shook.auth.application.dto.ReissueAccessTokenResponse;
import shook.shook.auth.application.dto.TokenPair;
import shook.shook.auth.exception.TokenException;
import shook.shook.auth.repository.InMemoryTokenPairRepository;
import shook.shook.member.application.MemberService;
import shook.shook.member.domain.Member;
import shook.shook.member.domain.repository.MemberRepository;
Expand All @@ -35,8 +37,15 @@ class AuthServiceTest {
@Autowired
private MemberService memberService;

@Autowired
private InMemoryTokenPairRepository inMemoryTokenPairRepository;

private TokenProvider tokenProvider;

private String refreshToken;

private String accessToken;

private AuthService authService;

@BeforeEach
Expand All @@ -45,8 +54,11 @@ void setUp() {
100000L,
1000000L,
"asdfsdsvsdf2esvsdvsdvs23");
authService = new AuthService(memberService, googleInfoProvider, tokenProvider);
savedMember = memberRepository.save(new Member("[email protected]", "shook"));
refreshToken = tokenProvider.createRefreshToken(savedMember.getId(), savedMember.getNickname());
accessToken = tokenProvider.createAccessToken(savedMember.getId(), savedMember.getNickname());
inMemoryTokenPairRepository.addOrUpdateTokenPair(refreshToken, accessToken);
authService = new AuthService(memberService, googleInfoProvider, tokenProvider, inMemoryTokenPairRepository);
}

@AfterEach
Expand Down Expand Up @@ -80,19 +92,16 @@ void success_login() {

assertThat(result.getAccessToken()).isEqualTo(accessToken);
assertThat(result.getRefreshToken()).isEqualTo(refreshToken);
assertDoesNotThrow(() -> inMemoryTokenPairRepository.validateTokenPair(refreshToken, accessToken));
}

@DisplayName("올바른 refresh 토큰이 들어오면 access 토큰을 재발급해준다.")
@DisplayName("올바른 refresh 토큰과 access 토큰이 들어오면 access 토큰을 재발급해준다.")
@Test
void success_reissue() {
//given
final String refreshToken = tokenProvider.createRefreshToken(
savedMember.getId(),
savedMember.getNickname());

//when
final ReissueAccessTokenResponse result = authService.reissueAccessTokenByRefreshToken(
refreshToken);
refreshToken, accessToken);

//then
final String accessToken = tokenProvider.createAccessToken(
Expand All @@ -111,13 +120,13 @@ void fail_reissue_invalid_refreshToken() {
100L,
"asdzzxcwetg2adfvssd3xZcZXCZvzx");

final String refreshToken = inValidTokenProvider.createRefreshToken(
final String wrongRefreshToken = inValidTokenProvider.createRefreshToken(
savedMember.getId(),
savedMember.getNickname());

//when
//then
assertThatThrownBy(() -> authService.reissueAccessTokenByRefreshToken(refreshToken))
assertThatThrownBy(() -> authService.reissueAccessTokenByRefreshToken(wrongRefreshToken, accessToken))
.isInstanceOf(TokenException.NotIssuedTokenException.class);
}

Expand All @@ -136,7 +145,7 @@ void fail_reissue_expired_refreshToken() {

//when
//then
assertThatThrownBy(() -> authService.reissueAccessTokenByRefreshToken(refreshToken))
assertThatThrownBy(() -> authService.reissueAccessTokenByRefreshToken(refreshToken, accessToken))
.isInstanceOf(TokenException.ExpiredTokenException.class);
}
}
Loading
Loading