[BE] JWT 토큰 사용 제거, Interceptor와 ArgumentResolver 중복 로직 제거 #733

jaehee329 · 2023-11-14T10:02:00Z

구현 기능 및 변경 사항

클라이언트쪽에서 JWT 토큰을 열어보지 않으므로 AES를 활용하는 방향으로 바꾼다.
Interceptor에서 파싱한 사용자 정보를 request에 넣어놓고 ArgumentResolver에서 필요하다면 가져오도록 바꾼다.

aes 기반 설정 정보 추가

Interceptor에서 파싱한 memberId를 Request에 넣도록 변경

aak2075

고생하셨습니다! 암호화하는 방법도 알게 되어서 좋았습니다.
catch하는 부분만 설명해 주시면 좋을 것 같아요!

aak2075 · 2023-11-14T10:38:08Z

backend/src/main/java/harustudy/backend/auth/util/AesTokenProvider.java

+            String formatted = DATE_FORMAT.format(expireAt);
+            String text = subject + " " + formatted;
+            return encrypt(text, secretKey);
+        } catch (Exception e) {


try-catch는 encrypt의 책임이 아닐까요? 그리고 구체적인 예외를 catch하는 것이 좋지 않을까요?

아래 댓글 달아주신 parseSubject메서드와 같은 구조로 외부에서 한 번에 예외를 잡아주는게 어떨까 했는데 다시 보니 예외를 발생시키는 경우가 encrypt 밖에 없어서 내부에서 해 주는게 맞겠네요~
encrypt 과정에서 굉장히 다양한 예외들이 발생하는데 GeneralSecurityException 정도로 묶어주겠습니다👍

aak2075 · 2023-11-14T10:38:46Z

backend/src/main/java/harustudy/backend/auth/util/AesTokenProvider.java

+            validateLength(splitted);
+            validateExpiration(splitted);
+            return parseSubject(splitted);
+        } catch (Exception e) {


여기도 Exception을 catch하고 있군요

여기는 uncheckedException과 checkedException 모든 상황에 대한 예외가 다양해서 구체적인 예외를 모두 나열하는 것이 필요가 없을 것 같아서 Exception으로 처리했어요. 또 Exception으로 모아도 잃는 stacktrace 정보도 없으니 괜찮을 것 같고요.

내부에서는 CheckedException인 ParseException과 RuntimeException을 상속한 배열 크기 관련한 ArrayIndexOutOfBoundsException도 발생하고 상속한 저희의 커스텀 예외인 InvalidAccessTokenException 을 던지기도 하고요.

사실 private 메서드에서는 그냥 상위로 throw하고 가장 외부의 public 메서드에서 모두 catch하는 방법이 가장 좋을 것 같은데
이렇게 되면 private 메서드들에서 public으로 throw할 만한 적절한 exception이 없다 느껴졌습니다.
그래서 부분적으로 내부에서 InvalidAccessTokenException을 발생시키고 외부에서 다시 전환하게 했어요.

더 구체적으로 좋은 방법 제안해주시면 좋겠습니다~

Suggested change

} catch (Exception e) {

} catch (ParseException |

GeneralSecurityException|

RuntimeException e) {

throw new InvalidAccessTokenException(e);

}

조금 번거롭더라도 이 코드를 나중에 보게될 저희 스스로를 위해서 catch문을 분리하는 것도 하나의 방법이 될 수 있지 않을까 제안해봅니다.

그게 아니라면 개인적으로 private 검증 메서드마다 발생하는 checkedException에 대해서 예외 전환을 각각 해주는 편이 좋다고 생각이 들었어요. encrypt메서드의 경우는 이미 내부에서 예외를 런타임 예외로 전환해주고 있어서 크게 걸릴 부분은 없을 것 같은데 어떻게 생각하시나요?

해당 부분은 별도 클래스에서 처리하도록 해서 예외들을 깔끔하게 처리했습니다!

추가로 확인해주세요~!

aak2075 · 2023-11-14T10:42:15Z

backend/src/main/java/harustudy/backend/auth/util/AesTokenProvider.java

+    private Long parseSubject(String[] splitted) {
+        return Long.parseLong(splitted[0]);
+    }
+}


개행 부탁드립니다 🙏

제 IDE와 깃헙에서는 괜찮아보이는데 다른 분들도 없는 것으로 보이면 말씀해주세요!

제꺼에서는 일단 개행이 있는 것으로 표기되기는 하네요~

aak2075 · 2023-11-14T10:43:31Z

backend/src/main/java/harustudy/backend/auth/util/AesTokenProvider.java

+        IvParameterSpec ivParamSpec = new IvParameterSpec(iv.getBytes());
+        cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivParamSpec);
+
+        byte[] encrypted = cipher.doFinal(text.getBytes(StandardCharsets.UTF_8));


오오 이렇게 암호화 할 수 있군요 신기하네용👍

MoonJeWoong

jwt 걷어내신다고 고생 많으셨습니다. 이런저런 궁금한 부분들 리뷰드려봤는데 확인 부탁드립니다!

MoonJeWoong · 2023-11-16T04:43:24Z

backend/src/main/java/harustudy/backend/auth/service/AuthService.java

+        return TokenResponse.forLoggedIn(accessToken, refreshToken,
+                tokenConfig.refreshTokenExpireLength());


요기 개행은 안해도 되지 않을까 싶습니다!

IDE 설정을 맞췄을텐데 저는 여기에서는 글자수가 길어서 그런지 자동으로 리포맷을 해주네요..😂

MoonJeWoong · 2023-11-16T04:49:57Z

backend/src/test/java/harustudy/backend/auth/util/AesTokenProviderTest.java

+        SoftAssertions.assertSoftly(softly -> {
+            assertThat(accessToken.length()).isGreaterThan(0);
+            assertThat(aesTokenProvider.parseSubject(accessToken, tokenConfig.secretKey()))
+                    .isEqualTo(memberId);
+        });


요기 테스트에서 softly.assertThat() 을 사용해야 저희가 의동하는 softAssertion이 수행될 것 같습니다!

MoonJeWoong · 2023-11-16T07:20:36Z

backend/src/main/java/harustudy/backend/auth/util/AesTokenProvider.java

+    private static final String alg = "AES/CBC/PKCS5Padding";
+    private static final String iv = "0123456789abcdef"; // 16byte


AES 암호화 방식에서는 어떤 알고리즘을 사용하는지, IV가 고정인지 유동인지 등의 정보가 외부로 노출되어도 큰 보안적 위험은 이론적으로 없다고는 하지만 그럼에도 최대한 정보를 노출하지 않는 편이 좋다고 생각합니다. 요 정보들을 submodule로 같이 분리해서 TokenConfig를 통해 접근하도록 통일하는 방향은 어떠신가요? 혹시 이렇게 구현하신 다른 이유가 있으셨다면 말씀 부탁드립니다!

오프라인으로 얘기하여 굳이 적용하지 않는 것으로...

MoonJeWoong · 2023-11-16T07:28:41Z

backend/src/main/java/harustudy/backend/auth/util/AesTokenProvider.java

+    private Long parseSubject(String[] splitted) {
+        return Long.parseLong(splitted[0]);
+    }
+}


제꺼에서는 일단 개행이 있는 것으로 표기되기는 하네요~

MoonJeWoong · 2023-11-16T07:31:54Z

backend/src/main/java/harustudy/backend/auth/util/AesTokenProvider.java

+    private void validateExpiration(String[] splitted) throws ParseException {
+        Date expireAt = DATE_FORMAT.parse(splitted[1]);
+        if (expireAt.before(new Date())) {
+            throw new InvalidAccessTokenException();
+        }
+    }


리프레시 토큰은 Expired 된 상황에 대한 예외를 처리하고 있어서 accessToken에서도 통일성있게 AccessTokenExpiredException으로 처리하는 건 어떤가요?

(추가) 아 프론트랑 논의되지 않은 부분이라 섣불리 수정하기 어려울 수도 있겠네요...! 이 부분은 좀 더 얘기해보면 좋을 것 같습니다.

말씀하신 예외가 더 깔끔할 것 같긴 한데 당장은 프론트랑 연계되는 내용이라 바로 반영은 어렵겠네요!😅

MoonJeWoong · 2023-11-16T08:00:35Z

backend/src/main/java/harustudy/backend/auth/util/AesTokenProvider.java

+            validateLength(splitted);
+            validateExpiration(splitted);
+            return parseSubject(splitted);
+        } catch (Exception e) {


Suggested change

} catch (Exception e) {

} catch (ParseException |

GeneralSecurityException|

RuntimeException e) {

throw new InvalidAccessTokenException(e);

}

조금 번거롭더라도 이 코드를 나중에 보게될 저희 스스로를 위해서 catch문을 분리하는 것도 하나의 방법이 될 수 있지 않을까 제안해봅니다.

그게 아니라면 개인적으로 private 검증 메서드마다 발생하는 checkedException에 대해서 예외 전환을 각각 해주는 편이 좋다고 생각이 들었어요. encrypt메서드의 경우는 이미 내부에서 예외를 런타임 예외로 전환해주고 있어서 크게 걸릴 부분은 없을 것 같은데 어떻게 생각하시나요?

MoonJeWoong · 2023-11-16T08:01:35Z

backend/src/main/java/harustudy/backend/auth/util/AesTokenProvider.java

+    private String[] decrypt(String accessToken, String secretKey) throws GeneralSecurityException {
+        Cipher cipher = Cipher.getInstance(alg);
+        SecretKeySpec keySpec = new SecretKeySpec(secretKey.getBytes(), "AES");
+        IvParameterSpec ivParamSpec = new IvParameterSpec(iv.getBytes());
+        cipher.init(Cipher.DECRYPT_MODE, keySpec, ivParamSpec);
+
+        byte[] decodedBytes = Base64.getDecoder().decode(accessToken);
+        byte[] decrypted = cipher.doFinal(decodedBytes);
+        String string = new String(decrypted, StandardCharsets.UTF_8);
+        return string.split(" ");
+    }


encrypt 메서드는 내부에서 예외 전환을 해주는 방향으로 수정해주신 것 같아서 비슷한 방향으로 런타임 예외로 전환해주시면 좋을 것 같습니다. 😃

jaehee329 · 2023-11-16T08:44:07Z

예외 핸들링 관련한 내용 추가 수정하고
tokenize하는 형태를 별도의 객체로 관리할 수 있도록 로직 추가 수정해볼게요~

jaehee329 · 2023-11-16T10:59:46Z

token 내용에 대한 추상화를 한 단계 높여서 예외 처리 로직이 깔끔해지도록 수정했습니다!

woosung1223

고생하셨습니다~ 의존성이 줄어들어서 좋네요!

사소한 코멘트 남겼으니 확인 부탁드립니다.

woosung1223 · 2023-11-18T14:11:50Z

backend/src/test/java/harustudy/backend/auth/util/AesTokenProviderTest.java

+    @Autowired
+    private AesTokenProvider aesTokenProvider;
+
+    @Autowired
+    private TokenConfig tokenConfig;


SpringBootTest가 필요하지 않을 것 같은데 어떻게 생각하시나요?

생성자로 ObjectMapper 넣어주면 단위테스트가 깔끔할 것 같네용

tokenConfig의 경우에도 테스트이므로 임의의 값으로 넣어주면 될 것 같습니다!
(테스트용 application.yml에도 임의의 값으로 정의되어 있기도 하고요)

컨텍스트를 하나 덜 띄우는게 좋다는 말에 저도 당연히 동의합니다!
yml에서 테스트에서 쓰이는 설정에 대해 전역으로 관리되지 못하는 점이 조금 아쉽기는 하나 괜찮을 것 같아요.

다만 말씀하신 방법대로 실험해보니

private AesTokenProvider aesTokenProvider = new AesTokenProvider(new ObjectMapper());

스프링에서 주입해주는 ObjectMapper는 스프링만의 직렬화/역직렬화와 관련한 설정 클래스가 함께 포함되어있어
Jackson의 설정이 달라져 테스트가 완벽히 호환되지 않네요.(역직렬화 과정에서 예외)
스프링에서 어떻게 설정을 지정하는지 찾아보고 동일하게 만들어주어야 하는데 이는 당장 좀 오래 걸릴 듯 하고..
아니면 프로덕션 코드를 테스트에 의존적으로 조금 바꿔주어야 하네요 🥲

혹시 아시는 방법 있으신가요?

조금 찾아보니 스프링에서 주입해주는 ObjectMapper가 new ObjectMapper() 로 생성한 인스턴스와 다른 점이 있긴 있네요 ㄷㄷㄷ..(처음 암)

Gradle의 java 플러그인에서 parameters 옵션을 제공해주고, Spring Boot가 이를 사용해 auto configure를 하는군요..! 그래서 말씀해주신대로 부가 기능이 조금씩 달라지는 것 같습니다~!

그런데 제가 클론받아서 실험해보니, @RequiredArgsConstructor를 제거하고 @NoArgsConstructor를 붙이면 정상적으로 리플렉션을 사용한 역직렬화가 동작하는 것 같더라고요.

현재는 굳이 Spring Boot만의 추가 기능이 필요하지 않은 상황이므로 DI를 활용하지 않아도 될 것 같은데 어떻게 생각하시나요?

그리고 ObjectMapper를 인스턴스 변수로 선언해 여러 스레드가 공유하게 하는 건 지양하라는 아티클도 본 적 있네여! (아래 링크에서는 static 필드를 예시로 들지만, 싱글톤 빈이므로 비슷한 상황이라고 생각합니다)
https://stackoverflow.com/questions/3907929/should-i-declare-jacksons-objectmapper-as-a-static-field

=> 팀 회의에서 별도의 이슈에서 테스트 ObjectMapper에 대한 처리를 하도록 결정했습니다.

요거 테오와 얘기하면서 정리된 부분이 있는데 해당 PR은 일단 merge하고 유틸성 클래스 관련 이슈를 발행해서 따로 작업하는 편이 좋을 듯 합니다!

jaehee329 added 2 commits November 14, 2023 18:59

feat: jwt 대신 aes를 사용하도록 변경

a071f21

aes 기반 설정 정보 추가

refactor: Interceptor와 ArgumentResolver의 중복 로직 제거

8becd6b

Interceptor에서 파싱한 memberId를 Request에 넣도록 변경

jaehee329 added BE 백엔드 작업 feature labels Nov 14, 2023

jaehee329 requested review from MoonJeWoong, aak2075 and woosung1223 November 14, 2023 10:02

jaehee329 self-assigned this Nov 14, 2023

jaehee329 linked an issue Nov 14, 2023 that may be closed by this pull request

[BE] JWT 토큰 사용 제거, Interceptor와 ArgumentResolver 중복 로직 제거 #725

Closed

2 tasks

aak2075 approved these changes Nov 14, 2023

View reviewed changes

jaehee329 added 3 commits November 14, 2023 20:04

refactor: 예외 처리 위치 수정

ebee485

chore: jwt 관련 의존성 build.gradle에서 제거

04595b3

fix: submodule 관련 수정

9d09e10

MoonJeWoong requested changes Nov 16, 2023

View reviewed changes

jaehee329 added 5 commits November 16, 2023 17:46

refactor: parseSubject()의 예외 처리 로직 수정

9966cc0

fix: 누락된 예외 추가

7c1b8f4

refactor: 토큰 관련 로직을 별도의 클래스로 추상화

f3a3dfe

refactor: AccessTokenUtils 추가

8893087

fix: 잘못된 softAssertion 사용 수정

8919c1e

woosung1223 approved these changes Nov 18, 2023

View reviewed changes

MoonJeWoong approved these changes Nov 21, 2023

View reviewed changes

MoonJeWoong merged commit 1466390 into develop Nov 21, 2023
3 checks passed

MoonJeWoong deleted the be/feature/725-jwt-deletion branch November 21, 2023 02:17

MoonJeWoong mentioned this pull request Nov 21, 2023

[BE] AesTokenProvider 관련 SpringBootTest를 POJO 테스트로 변경 #736

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BE] JWT 토큰 사용 제거, Interceptor와 ArgumentResolver 중복 로직 제거 #733

[BE] JWT 토큰 사용 제거, Interceptor와 ArgumentResolver 중복 로직 제거 #733

jaehee329 commented Nov 14, 2023

aak2075 left a comment

aak2075 Nov 14, 2023

jaehee329 Nov 14, 2023

aak2075 Nov 14, 2023

jaehee329 Nov 14, 2023 •

edited

Loading

MoonJeWoong Nov 16, 2023

jaehee329 Nov 16, 2023

aak2075 Nov 14, 2023

jaehee329 Nov 14, 2023

MoonJeWoong Nov 16, 2023

aak2075 Nov 14, 2023

MoonJeWoong left a comment

MoonJeWoong Nov 16, 2023

jaehee329 Nov 16, 2023

MoonJeWoong Nov 16, 2023

MoonJeWoong Nov 16, 2023

jaehee329 Nov 16, 2023

MoonJeWoong Nov 16, 2023

MoonJeWoong Nov 16, 2023

jaehee329 Nov 16, 2023

MoonJeWoong Nov 16, 2023

MoonJeWoong Nov 16, 2023

jaehee329 commented Nov 16, 2023

jaehee329 commented Nov 16, 2023

woosung1223 left a comment

woosung1223 Nov 18, 2023

jaehee329 Nov 19, 2023

woosung1223 Nov 19, 2023 •

edited

Loading

jaehee329 Nov 21, 2023

MoonJeWoong Nov 21, 2023 •

edited

Loading

-        } catch (Exception e) {
+        } catch (ParseException |
+                 GeneralSecurityException|
+                 RuntimeException e) {
+            throw new InvalidAccessTokenException(e);
+        }

		return TokenResponse.forLoggedIn(accessToken, refreshToken,
		tokenConfig.refreshTokenExpireLength());

		private static final String alg = "AES/CBC/PKCS5Padding";
		private static final String iv = "0123456789abcdef"; // 16byte

[BE] JWT 토큰 사용 제거, Interceptor와 ArgumentResolver 중복 로직 제거 #733

[BE] JWT 토큰 사용 제거, Interceptor와 ArgumentResolver 중복 로직 제거 #733

Conversation

jaehee329 commented Nov 14, 2023

관련 이슈

구현 기능 및 변경 사항

aak2075 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

jaehee329 Nov 14, 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

MoonJeWoong 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

jaehee329 commented Nov 16, 2023

jaehee329 commented Nov 16, 2023

woosung1223 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woosung1223 Nov 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoonJeWoong Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

jaehee329 Nov 14, 2023 •

edited

Loading

woosung1223 Nov 19, 2023 •

edited

Loading

MoonJeWoong Nov 21, 2023 •

edited

Loading