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

seokhwan-an · 2023-09-19T04:02:44Z

📝작업 내용

refreshToken과 accessToken을 concurrentHashMap으로 관리하도록 구성 (key는 refreshToken으로 value는 accessToken로 관리합니다.)
현재 스케줄러를 통해 InMemoryTokenPairRespository를 갱신하고 있습니다.
- 스케줄러로 관리를 했던 이유는 처음에는 요청 올 때 refreshToken의 유효성을 판단해서 제거를 하려고 했는데 실제로 refreshToken은 cookie로 관리가 되고 있기 때문에 기간이 지나면 refreshToken을 담은 요청이 오지 않아서 메모리에서 삭제되지 않고 계속 유지되는 문제가 있었습니다. 그래서 스케줄러로 관리하는 방식을 택했습니다.

💬리뷰 참고사항

swagger를 다루어 본적이 없어서 swagger 설정을 올바르게 했는지 잘 모르겠습니다.
혹시 토큰 메모리를 스케줄러 외의 더 잘 관리할 수 있는 방안이 있으면 의견 부탁드랍나다.🙇‍♂️

#️⃣연관된 이슈

close [FEAT] 토큰 보안 처리를 추가한다. #385

세부사항 - inMemoryTokenPairRepository 도입

github-actions · 2023-09-19T04:05:06Z

Unit Test Results

  78 files   78 suites 10s ⏱️
303 tests 303 ✔️ 0 💤 0 ❌
306 runs 306 ✔️ 0 💤 0 ❌

Results for commit 8c5f57b.

♻️ This comment has been updated with latest results.

somsom13

고생하셨어요 아코! 구현 속도가 엄청 빠르네요 😺
몇 가지 질문과 피드백 남겼습니다~!!

somsom13 · 2023-09-19T04:14:31Z

backend/src/main/java/shook/shook/auth/application/AuthService.java

+        for (String a : tokenPairs.keySet()) {
+            System.out.println(a.equals(refreshToken));
+        }


이게 뭐죠? 불필요한 로그는 삭제해주세요. 😺

디버깅 한다고 남겼는데 지우겠습니다..😥

somsom13 · 2023-09-19T04:19:44Z

backend/src/main/java/shook/shook/auth/application/TokenPairScheduler.java

+    private final InMemoryTokenPairRepository inMemoryTokenPairRepository;
+
+    @Scheduled(cron = "${schedules.cron}")
+    public void renewInMemoryTokenPairRepository() {


renew 가 어떤 의미인가요? 이 스케줄러가 어떤 상황에서 어떤 작업을 수행하는지 궁금해요~!

InMemoryTokenPairRepository를 갱신하다는 의미를 담고 있었는데 이부분이 와닿지 않았던 것 같습니다. 스케줄러는 특정 시간(로컬에서는 1시간마다)마다 동작하게 구성을 했고 (실제 프로덕트의 경우 팀원들과 이야기를 나누고 정해야 할 것 같아요) 동작하는 내용은 refreshToken의 만료기간이 지나면 그 refreshToken들과 그에 해당하는 accessToken을 지우는 작업을 합니다!

좀더 쉽게 파악할 수 있게 자세한 네이밍을 하겠습니다!

somsom13 · 2023-09-19T04:22:37Z

backend/src/main/java/shook/shook/auth/repository/InMemoryTokenPairRepository.java

+@Repository
+public class InMemoryTokenPairRepository {
+
+    private final Map<String, String> tokenPairs = new ConcurrentHashMap<>();
+


토큰 페어 저장소를 인메모리로 선택하신 이유가 궁금해요!

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

현재 저희 서버는 1대로 scale-out이 일어나지 않는 상황입니다.

메모리의 단점은 휘발성이라는 것인데 현재 이 토큰 정보과 휘발 되었다 하더라도 다시 로그인을 하는 것외에 서비스에 영향이 없다고 생각을 했습니다.

메모리의 장점으로는 db에 접근해서 데이터를 지우거나 추가하는 것보다 비용이 적다는 것이었습니다. 특히 이 부분은 스케줄로 관리를 해야하는 부분이기에 주기적으로 지우는 작업이 발생하기에 메모리가 더 좋다고 생각했습니다.

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

somsom13 · 2023-09-19T04:23:39Z

backend/src/main/java/shook/shook/auth/ui/AccessTokenReissueController.java


    private final AuthService authService;

-    @GetMapping("/reissue")
+    @PostMapping("/reissue")


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

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

그런데 사실 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/

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

somsom13 · 2023-09-19T04:25:37Z

backend/src/main/java/shook/shook/auth/ui/AccessTokenReissueController.java

    ) {
        if (refreshToken.equals(EMPTY_REFRESH_TOKEN)) {
            throw new AuthorizationException.RefreshTokenNotFoundException();
        }
+        final String accessToken = authorization.split(TOKEN_PREFIX)[1];


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

header를 다루는 것은 httpMessage를 다루는 것이라고 생각을 해서 service가 아닌 present layer인 controller에서 가공하는 것을 택했습니다:)

header가 Bearer인지는 interceptor에서 처리가 되고 있어서 controller 단에서는 처리할 필요가 없다고 생각을 해서 처리를 하지 않았습니다.

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

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

Cyma-s

조금 반영할 부분이 있을 것 같아서 RC 드렸습니다 🙂
수고 많으셨어요! 시간 되실 때 반영해주세요 :)

Cyma-s · 2023-09-19T09:08:54Z

backend/src/main/java/shook/shook/auth/repository/InMemoryTokenPairRepository.java

+        tokenPairs.remove(refreshToken);
+    }
+
+    public Map<String, String> getTokenPairs() {


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

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

Cyma-s · 2023-09-19T09:09:26Z

backend/src/main/java/shook/shook/auth/repository/InMemoryTokenPairRepository.java

+    public void add(final String refreshToken, final String accessToken) {
+        tokenPairs.put(refreshToken, accessToken);
+    }
+
+    public void update(final String refreshToken, final String accessToken) {
+        tokenPairs.put(refreshToken, accessToken);
+    }


이 부분은 하는 일이 동일한데, 같은 메서드로 합치면 좋을 것 같아요

같은 동작을 하더라고 위와 같이 나누었던 이유는 같은 동작을 하더라도 의미가 다른 작업이라고 생각했기 때문입니다. 리뷰를 받고 다시 생각해보았는데 토큰 저장소를 update하는 것이기에 모두 update 하나로 처리하겠습니다.

Cyma-s · 2023-09-19T09:12:11Z

backend/src/main/java/shook/shook/auth/ui/AccessTokenReissueController.java

    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("Authorization") final String authorization


HttpHeaders 의 상수를 사용하면 더 좋을 것 같아요

저도 그렇게하면 오타의 실수를 줄일 수 있을 것 같습니다 좋은 리뷰 감사합니다!👍

세부사항 - InMemoryTokenPairRepository에서 add와 update를 하나의 메소드로 관리하기 - HttpHeaders 이용하기 - 코드 가독성 향상 시키기

somsom13

제가 남겨드렸던 피드백은 잘 반영된 것 같아요!! 고생하셨습니다 👍 👍

Cyma-s

큰 리뷰들은 아니고 궁금한 부분 + 의견 궁금한 부분만 코멘트 달았습니다 :)
시간 되실 때 반영해주세요!

Cyma-s · 2023-09-20T06:11:45Z

backend/src/main/java/shook/shook/auth/application/TokenPairScheduler.java

+        final Set<String> refreshTokens = inMemoryTokenPairRepository.getTokenPairs().keySet();
+        refreshTokens.forEach(refreshToken -> {
+            try {
+                tokenProvider.parseClaims(refreshToken);
+            } catch (TokenException.ExpiredTokenException e) {
+                inMemoryTokenPairRepository.delete(refreshToken);
+            }
+        });


forEach 는 for 문에 비해 속도가 떨어지는데, forEach 를 채택하신 이유가 있나요??

가독성이 더 좋다고 생각을 해서 forEach를 이용했는데 내부적으로 try catch를 이용하다보니 큰 메리트를 얻지 못한 것 같습니다. 이 부분 카카오 소셜 로그인 구현에서 반영하겠습니다!

Cyma-s · 2023-09-20T14:30:54Z

backend/src/main/java/shook/shook/auth/application/AuthService.java

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


Cyma-s · 2023-09-20T14:36:00Z

backend/src/main/java/shook/shook/auth/ui/AccessTokenReissueController.java


    private final AuthService authService;

-    @GetMapping("/reissue")
+    @PostMapping("/reissue")


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

seokhwan-an added 3 commits September 19, 2023 09:13

refactor: refreshToken으로만 accessToken을 재발급해주던 방식 수정

8a02476

세부사항 - inMemoryTokenPairRepository 도입

feat: scheduler를 통해 InMemoryTokenPairRepository를 갱신하는 기능 추가

f9f5d3d

refactor: 테스트 통과되지 않았던 오류 수정

6d9071c

seokhwan-an added [ 🌙 BE ] 백엔드 크루들의 멋진 개발 이야기 하나둘셋 야! ✨ Feat 꼼꼼한 기능 구현 중요하죠 labels Sep 19, 2023

seokhwan-an requested a review from Cyma-s as a code owner September 19, 2023 04:02

seokhwan-an self-assigned this Sep 19, 2023

seokhwan-an requested review from splitCoding and somsom13 as code owners September 19, 2023 04:02

somsom13 requested changes Sep 19, 2023

View reviewed changes

Cyma-s requested changes Sep 19, 2023

View reviewed changes

seokhwan-an mentioned this pull request Sep 20, 2023

Feat/#425 카카오 소셜 로그인을 구현한다. #434

Merged

somsom13 added this to the [5차] 성능 개선 및 사용자 피드백 반영된 기능 구현 milestone Sep 20, 2023

seokhwan-an and others added 3 commits September 20, 2023 15:46

refactor: 코드 리뷰 반영

d83945b

세부사항 - InMemoryTokenPairRepository에서 add와 update를 하나의 메소드로 관리하기 - HttpHeaders 이용하기 - 코드 가독성 향상 시키기

refactor: 서브모듈 최신화

4d5282c

Merge branch 'main' into feat/#385

850dc5e

somsom13 approved these changes Sep 20, 2023

View reviewed changes

Cyma-s approved these changes Sep 20, 2023

View reviewed changes

Merge branch 'main' into feat/#385

8c5f57b

seokhwan-an merged commit 3393c7b into main Sep 21, 2023
4 checks passed

seokhwan-an deleted the feat/#385 branch September 21, 2023 06:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

seokhwan-an commented Sep 19, 2023

github-actions bot commented Sep 19, 2023 •

edited

Loading

somsom13 left a comment

somsom13 Sep 19, 2023 •

edited

Loading

seokhwan-an Sep 19, 2023

somsom13 Sep 19, 2023

seokhwan-an Sep 20, 2023

seokhwan-an Sep 20, 2023

somsom13 Sep 19, 2023

seokhwan-an Sep 20, 2023

somsom13 Sep 20, 2023

somsom13 Sep 19, 2023

seokhwan-an Sep 20, 2023

somsom13 Sep 20, 2023

Cyma-s Sep 20, 2023

somsom13 Sep 19, 2023

seokhwan-an Sep 19, 2023

somsom13 Sep 19, 2023 •

edited

Loading

seokhwan-an Sep 20, 2023 •

edited

Loading

Cyma-s left a comment

Cyma-s Sep 19, 2023

seokhwan-an Sep 20, 2023 •

edited

Loading

Cyma-s Sep 19, 2023

seokhwan-an Sep 20, 2023

Cyma-s Sep 19, 2023

seokhwan-an Sep 20, 2023

somsom13 left a comment

Cyma-s left a comment

Cyma-s Sep 20, 2023

seokhwan-an Sep 21, 2023

Cyma-s Sep 20, 2023

Cyma-s Sep 20, 2023

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

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

Conversation

seokhwan-an commented Sep 19, 2023

📝작업 내용

💬리뷰 참고사항

#️⃣연관된 이슈

github-actions bot commented Sep 19, 2023 • edited Loading

Unit Test Results

somsom13 left a comment

Choose a reason for hiding this comment

somsom13 Sep 19, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

somsom13 Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

seokhwan-an Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Cyma-s left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seokhwan-an Sep 20, 2023 • edited Loading

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

somsom13 left a comment

Choose a reason for hiding this comment

Cyma-s 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

github-actions bot commented Sep 19, 2023 •

edited

Loading

somsom13 Sep 19, 2023 •

edited

Loading

somsom13 Sep 19, 2023 •

edited

Loading

seokhwan-an Sep 20, 2023 •

edited

Loading

seokhwan-an Sep 20, 2023 •

edited

Loading