[BE] 로그아웃 기능 추가 #151

seokmyungham · 2024-08-02T13:18:29Z

작업 내용

로그아웃 기능을 추가하였습니다.
- API 요청 시 저장되어있는 쿠키 만료 시간을 0으로 설정하여 쿠키를 만료시킵니다.
AccessToken의 만료 시간을 새롭게 설정하였습니다.
로그인 시 로그인한 참가자의 이름(닉네임)을 ResponseBody에 저장하여 반환하도록 변경하였습니다.
약속 생성 시 주최자의 토큰을 Set-Cookie 헤더에 저장하여 응답합니다.
- 주최자는 약속 생성 후 로그인 작업을 수행하지 않아도 되어 사용자 경험이 개선됩니다.

특이 사항

리뷰 요구사항 (선택)

github-actions · 2024-08-02T13:19:41Z

Test Results

68 tests 68 ✅ 4s ⏱️
15 suites 0 💤
15 files 0 ❌

Results for commit b1a6fce.

♻️ This comment has been updated with latest results.

hw0603

늦은 시간에 고생하셨습니다~

몇 가지 코멘트 남겨 두었어요.

hw0603 · 2024-08-02T19:35:29Z

backend/src/main/java/kr/momo/controller/JwtCookieManager.java

+public class JwtCookieManager {
+
+    private static final String ACCESS_TOKEN = "ACCESS_TOKEN";
+    private static final String SAME_SITE_SETTING = "None";


SAME_SITE_OPTION이 좀 더 자연스럽지 않을까요?
사소한 건데, 다른 분들의 option, setting, config을 사용하시는 맥락이 궁금하네요ㅎㅎ

개인적인 생각으로는
SETTING을 사용하면 해당 상수는 SameSite를 구성하는 목적을 갖는 값이라는 의미가 잘 드러나는 것 같고
OPRION을 사용하면 해당 상수는 SameSite의 여러 옵션 중 하나라는 맥락이 더 잘 담기겠네요.. ㅋㅋㅋ

개발자간에는 서로 동의어처럼 사용되는 것 같고, UX와 관련된 것이 아니라면 일관성이 제일 중요할 것 같아요
Option, Setting, Config.. 정답이 없는 문제라 너무 어려운데요 😂

저는 개인적으로 Config는 @Configuration과 같이 스프링 설정과 혼동 방지를 위해 클래스명을 제외한 변수, 상수명에는 잘 안쓰곤 해요. 😀
OPTION은 추가적인 선택지가 있는 느낌인 반면 SETTING은 뭔가 박혀있는 느낌이 강해 보입니다.

지금은 Same-Site 설정이 4 종류가 있고 그 중 하나를 골랐으니OPTION이란 단어가 더 적절해보이네요 :)
(제가 추천했던 상수명이라 열렬히 답변 적어봅니다. 😅)
+ 이건 Enum으로 안만들어 뒀나 하는 의문도 남네요.

OPITON을 사용하면 해당 상수는 SameSite의 여러 옵션 중 하나라는 맥락이 더 잘 담기겠네요.. ㅋㅋㅋ

위에 써주신 이유로, None은 옵션 중 하나이기 때문에 SAME_SITE_OPTION이 더 적절해 보입니다~
CookieResponse.sameSite(SAME_SITE_OPTION) 함수를 통해서 그 옵션을 설정(setting)해 주는 것이라고 직관적으로 이해할 수 있을 것 같아요!

저도 배키와 같은 이유로 현 상황에는OPTION 쪽이 적절해 보여요~

팀원들의 소중한 의견 반영했습니다 😁

hw0603 · 2024-08-02T19:37:43Z

backend/src/main/java/kr/momo/controller/attendee/AttendeeController.java

    private final AttendeeService attendeeService;
+    private final JwtCookieManager jwtCookieManager;


질문) 꼭 필요한 인스턴스화인가요? JwtCookieManager.createCookie 처럼 사용하는 것과 차이는 무엇인가요?

스프링 빈으로 관리하느냐와 정적 클래스로 사용하느냐의 생각은 사람마다 조금씩 다를 것 같은데 몇 가지 기준이 있을 것 같아요.

정적 클래스로 사용할만큼 클래스의 목적이 명확하고 항상 일관된 입출력을 가지는지

유틸 성격을 띌 경우 공통적인 메서드로만 구성되어 프로젝트 전역에 광범위하게 영향을 미치는지

추후 해당 클래스가 상태나 의존성을 가질 가능성이 존재하는지

스프링 컨테이너에 관리하는 이점이 존재하는지 (Test, LifeCycle, DI)

제가 생각하는 차이는 다음과 같습니다.

정적 클래스로 사용

간단하게 사용할 수 있다.

의존성에 관한 코드를 작성하지 않아도 된다.

유연하지 못하고 강결합이 발생한다.

Mock이 불가능하기 때문에 의존하는 클래스를 테스트하기 어렵다.

스프링 빈으로 관리하여 사용

DI의 다양한 장점들을 가질 수 있다. (유연한 설계, 확장 및 테스트)

스프링이 지원하는 환경 설정과 연동할 수 있다 (yml)

컨테이너에 빈이 하나 추가 됨으로 관리 포인트가 늘어난다.

스프링의 DI 탄생 배경을 생각해봤을 때, 저는 스프링을 사용한다면 거의 웬만한 상황에서 스프링의 도움을 받는 것이 좋다는 생각입니다. 미래 확장 가능성을 배제하고 현재 상황에만 집중하여 하나 둘 씩 정적 클래스가 늘어나면, 나중에는 감당 못할만큼 유연하지 못한 설계가 되어있을 가능성이 크다 생각합니다. (그치만 이 의견은 사람마다 많이 갈리는 것 같습니다)

제 개인적인 선호를 배제하고, 이 클래스를 빈으로 만든 이유는 제가 추후 확장 가능성을 정확히 예측할 수 없었기 때문인데요. 추후 환경 변수에 의존할 가능성이 없을지? 로그나 모니터링 툴을 도입했을 때 체크 포인트로 사용되어 의존성을 추가할 가능성이 없는지 잘 모르겠습니다. (현재 학습하지 않아 이 부분에 대한 제 지식이 부족합니다.)

현재 상황만 본다면 정적 클래스로 사용해도 무방한 기능들을 가지고 있는데요, 될 수 있으면 해당 기술에 지식이 있는 팀원들 모두의 동의를 거쳐 결정했으면 좋겠습니다.

스프링의 DI 탄생 배경을 생각해봤을 때, 저는 스프링을 사용한다면 거의 웬만한 상황에서 스프링의 도움을 받는 것이 좋다는 생각입니다.

좋은 접근법인것 같네요🙂

backend/src/main/java/kr/momo/controller/meeting/MeetingController.java

backend/src/main/java/kr/momo/exception/code/AuthErrorCode.java

hw0603 · 2024-08-02T19:41:08Z

backend/src/main/java/kr/momo/service/attendee/dto/AttendeeLoginResponse.java

+
+import kr.momo.domain.attendee.Attendee;
+
+public record AttendeeLoginResponse(String token, String name) {


현재는 이름만 내려주고 있기는 한데, 사용자의 HOST 여부도 필요할지 프론트 측과 이야기 해 봐야 할 것 같아요

(명시적인 사용자 권한을 응답으로 내려도 되는 건지는 잘 모르겠네요🥲 어떻게 생각하시나요?)

음.. 정말 HOST, GUEST 여부에 따라 사용자에게 화면을 다르게 보여줘야 한다면, 해당 값을 응답으로 내려주어야 할 것 같네요.

그런데 사용자 권한이기는 하지만 현재 약속 만들고 확정하는 일이
게시글을 작성하고 삭제하거나 투표를 만들고 종료하는 상황과 비슷하다는 느낌이 들긴 해요. 🤔
엄밀하게 검토해보고 비슷한 상황에 대한 레퍼런스를 찾는 건 어렵지 않으니 한 번 적용해볼 수 있을 것 같아요 😀

backend/src/main/java/kr/momo/service/auth/JwtManager.java

backend/src/test/java/kr/momo/controller/attendee/AttendeeControllerTest.java

ikjo39

재즈 고생하셨어요 😄
몇가지 리뷰 남겼으니 확인 부탁드립니다!

ikjo39 · 2024-08-03T07:57:19Z

backend/src/main/java/kr/momo/controller/JwtCookieManager.java

+public class JwtCookieManager {
+
+    private static final String ACCESS_TOKEN = "ACCESS_TOKEN";
+    private static final String SAME_SITE_SETTING = "None";


저는 개인적으로 Config는 @Configuration과 같이 스프링 설정과 혼동 방지를 위해 클래스명을 제외한 변수, 상수명에는 잘 안쓰곤 해요. 😀
OPTION은 추가적인 선택지가 있는 느낌인 반면 SETTING은 뭔가 박혀있는 느낌이 강해 보입니다.

지금은 Same-Site 설정이 4 종류가 있고 그 중 하나를 골랐으니OPTION이란 단어가 더 적절해보이네요 :)
(제가 추천했던 상수명이라 열렬히 답변 적어봅니다. 😅)
+ 이건 Enum으로 안만들어 뒀나 하는 의문도 남네요.

backend/src/main/java/kr/momo/service/auth/JwtManager.java

hw0603

JWT 만료 시간 관련 리뷰 하나 더 드렸습니다. 확인 부탁드려요!

hw0603 · 2024-08-03T09:44:24Z

backend/src/main/java/kr/momo/service/auth/JwtManager.java

        return JWT.create()
                .withClaim(CLAIM_ID, id)
+                .withIssuedAt(new Date())


Date 객체는 생성자에서 밀리초 단위의 값을 받지만, JWT 헤더에 기록될 때는 밀리초 단위가 절사되어 기록됩니다. (초단위로 기록)

expirationPeriod의 로컬 기본값이 3600으로 설정되어 있는 것으로 확인했는데요, 이는 3.6초에 해당하여 현재 설정에서 JWT 디코드 시 발행일자와 만료일자가 4초(3.6초에서 반올림) 차이가 납니다.
의도하신 사항이 맞으실까요? 아니라면 단위 수정이 필요할 것 같아요.

이런 혼선을 방지하기 위해 프로퍼티 이름을 expiration-period-in-hour 과 같이 단위를 명시해 주는 방법도 있겠네요. 그 단위에 맞춰서 JWTManager에서는 밀리초 단위로 변환하여 Date객체를 만들어야 할 것 같습니다.

(yml 프로퍼티명은 외부 파일에서 주입받는 값이기 때문에 이름만 보고도 값을 "오해 없이" 정확하게 알 수 있어야 한다고 생각해요.)

이때 ms 단위로 변환하기 위해서는 값이 매우 커질 텐데요, long에서는 오버플로우 문제가 발생할 수 있습니다.

참고하실 만한 링크 하나 공유해 드릴게요.

https://sundries-in-myidea.tistory.com/150

+ 생성하신 JWT 토큰을 꼭 Decode 해 보시고, 원하는 값이 잘 들어갔는지 눈으로 확인해 보시는 걸 추천드려요!

ms 단위인 걸 알면서도 휴먼에러를.. 😮 좋은 지적 감사합니다 👍
단위를 명시해서 의도적으로 변환하도록 하면 실수가 줄어들 수 있겠어요. 반영하겠습니다 😊

System.currentTimeMillis() 메서드로 현재 기준 약 1.7조ms 를 확인했고 long 타입의 정수 범위를 생각했을 때 오버플로우 문제는 발생하지 않을 것 같습니다. int 타입이 문제가 될 것 같아요.

현재 타입을 유지하고 코드를 개선해보겠습니다 🚀

ehBeak

재즈 고생하셨어요!
팀원들이 좋은 리뷰 남겨주셔서 저는 간단하게 코멘트 남깁니다~👍

ehBeak · 2024-08-03T09:35:57Z

backend/src/main/java/kr/momo/controller/JwtCookieManager.java

+public class JwtCookieManager {
+
+    private static final String ACCESS_TOKEN = "ACCESS_TOKEN";
+    private static final String SAME_SITE_SETTING = "None";


OPITON을 사용하면 해당 상수는 SameSite의 여러 옵션 중 하나라는 맥락이 더 잘 담기겠네요.. ㅋㅋㅋ

위에 써주신 이유로, None은 옵션 중 하나이기 때문에 SAME_SITE_OPTION이 더 적절해 보입니다~
CookieResponse.sameSite(SAME_SITE_OPTION) 함수를 통해서 그 옵션을 설정(setting)해 주는 것이라고 직관적으로 이해할 수 있을 것 같아요!

backend/src/main/java/kr/momo/controller/JwtCookieManager.java

ehBeak

🎉🎉🎉

seokmyungham · 2024-08-03T13:42:44Z

변경 사항

JWT와 관련된 환경 변수들을 POJO로 관리할 수 있도록 JwtProperties를 추가하였습니다.

Spring docs에 따르면 관련된 설정 속성이 여러개인 경우 @Value 보다는 @ConfigurationProperties로 관리하기를 권장합니다.
Properties Conversion 기능을 활용하여 Duration 단위를 환경 변수에 명시할 수 있습니다. 기존 millis 단위를 계산해야 했던 개발자의 실수를 줄이도록 개선하였습니다.
더 나아가 추가로 발생할 수 있는 실수를 방지하고자 Duration의 기본 단위를 @DurationUnit 으로 명시하여 문서화 하였습니다.
더불어 @Validated @NotNull을 사용하여 환경 변수가 존재하지 않는 상황에 스프링 컨텍스트가 로드되는 상황을 방지하였습니다.

backend/src/main/java/kr/momo/controller/CookieManager.java

backend/src/main/java/kr/momo/MomoApplication.java

backend/src/main/java/kr/momo/controller/meeting/MeetingController.java

seunghye218

CookieManager를 분리하여 다양한 컨트롤러에서 사용해야할 로직을 잘 재사용하네요. JWT 가 들어있는 쿠키에 httpOnly, secure 옵션을 통해 보안이 잘 되어있는 것 같아요 👍

seunghye218 · 2024-08-03T11:10:07Z

backend/src/main/java/kr/momo/controller/JwtCookieManager.java

+public class JwtCookieManager {
+
+    private static final String ACCESS_TOKEN = "ACCESS_TOKEN";
+    private static final String SAME_SITE_SETTING = "None";


저도 배키와 같은 이유로 현 상황에는OPTION 쪽이 적절해 보여요~

- JWT 토큰 만료 시간 추가

seokmyungham added 🐈‍⬛ 백엔드 백엔드 관련 이슈에요 :) 🚀 기능 기능을 개발해요 :) labels Aug 2, 2024

seokmyungham added this to the 3차 데모데이 milestone Aug 2, 2024

seokmyungham self-assigned this Aug 2, 2024

seokmyungham requested review from ehBeak, ikjo39, hw0603 and seunghye218 as code owners August 2, 2024 13:18

hw0603 requested changes Aug 2, 2024

View reviewed changes

ikjo39 reviewed Aug 3, 2024

View reviewed changes

hw0603 requested changes Aug 3, 2024

View reviewed changes

ehBeak reviewed Aug 3, 2024

View reviewed changes

ehBeak previously approved these changes Aug 3, 2024

View reviewed changes

ikjo39 previously approved these changes Aug 3, 2024

View reviewed changes

seokmyungham dismissed stale reviews from ikjo39 and ehBeak via dfb14f5 August 3, 2024 16:09

hw0603 requested changes Aug 3, 2024

View reviewed changes

backend/src/main/java/kr/momo/controller/CookieManager.java Outdated Show resolved Hide resolved

backend/src/main/java/kr/momo/MomoApplication.java Show resolved Hide resolved

backend/src/main/java/kr/momo/controller/meeting/MeetingController.java Outdated Show resolved Hide resolved

hw0603 approved these changes Aug 4, 2024

View reviewed changes

ikjo39 approved these changes Aug 4, 2024

View reviewed changes

seunghye218 approved these changes Aug 4, 2024

View reviewed changes

seokmyungham added 8 commits August 4, 2024 20:35

refactor(jwtManager): jwt 토큰에 만료 시간을 추가

209d12a

refactor: 로그인 시 참가자의 이름을 같이 반환하도록 변경

6e4e331

feat: path 경로 수정 및 로그아웃 api 추가

93229ee

chore: 토큰 만료시간 추가

9eb67ae

chore: 서브모듈 업데이트 반영

b99e92b

- JWT 토큰 만료 시간 추가

refactor(JwtCookieManager): 쿠키 생성의 책임을 컨트롤러에서 분리

b4c10a9

refactor(MeetingController): 약속 최초 생성시 주최자에 대한 JWT 토큰을 쿠키로 전송하도록 변경

599990b

style(JwtManager): 생성자 매개변수 팀 컨벤션 개행 적용

ece6223

seokmyungham added 8 commits August 4, 2024 20:35

refactor: 쿠키 관련 상수 명 설정

267f41a

refactor(JwtCookieManager): 클래스 이름을 CookieManager 로 변경

ed7f6a9

refactor(CookieManager): SAME SITE 상수 명을 변경하여 의미를 개선

0c615d6

refactor(JwtProperties): JWT 관련 환경 변수들을 POJO로 관리하도록 개선

7b56899

chore: 서브모듈 업데이트 반영

8cdb967

refactor(JwtProperties): expirationPeriod 필드 NotNull 검증 추가

02b07bd

refactor: ConfigurationPropertiesScan을 사용하여 전역으로 관리하도록 변경

f9d31b2

refactor(CookieManager): 클래스의 책임에 맞도록 상수 및 변수 이동

b1a6fce

seokmyungham force-pushed the feat/132-logout branch from a7cb04e to b1a6fce Compare August 4, 2024 11:35

seokmyungham merged commit f970575 into develop Aug 4, 2024
6 checks passed

hw0603 deleted the feat/132-logout branch August 4, 2024 12:31

Yoonkyoungme mentioned this pull request Aug 8, 2024

[FE] 로그아웃 기능을 구현해요 :) #194

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BE] 로그아웃 기능 추가 #151

[BE] 로그아웃 기능 추가 #151

seokmyungham commented Aug 2, 2024 •

edited

Loading

github-actions bot commented Aug 2, 2024 •

edited

Loading

hw0603 left a comment

hw0603 Aug 2, 2024

seokmyungham Aug 3, 2024

ikjo39 Aug 3, 2024 •

edited

Loading

ehBeak Aug 3, 2024

seunghye218 Aug 3, 2024

seokmyungham Aug 3, 2024

hw0603 Aug 2, 2024

seokmyungham Aug 3, 2024 •

edited

Loading

hw0603 Aug 3, 2024

hw0603 Aug 2, 2024

seokmyungham Aug 3, 2024 •

edited

Loading

ikjo39 left a comment

ikjo39 Aug 3, 2024 •

edited

Loading

hw0603 left a comment

hw0603 Aug 3, 2024

hw0603 Aug 3, 2024 •

edited

Loading

seokmyungham Aug 3, 2024 •

edited

Loading

seokmyungham Aug 3, 2024 •

edited

Loading

ehBeak left a comment

ehBeak Aug 3, 2024

ehBeak left a comment

seokmyungham commented Aug 3, 2024 •

edited

Loading

seunghye218 left a comment

seunghye218 Aug 3, 2024

		private final AttendeeService attendeeService;
		private final JwtCookieManager jwtCookieManager;


		import kr.momo.domain.attendee.Attendee;

		public record AttendeeLoginResponse(String token, String name) {

[BE] 로그아웃 기능 추가 #151

[BE] 로그아웃 기능 추가 #151

Conversation

seokmyungham commented Aug 2, 2024 • edited Loading

관련 이슈

작업 내용

특이 사항

리뷰 요구사항 (선택)

github-actions bot commented Aug 2, 2024 • edited Loading

Test Results

hw0603 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikjo39 Aug 3, 2024 • 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

seokmyungham Aug 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seokmyungham Aug 3, 2024 • edited Loading

Choose a reason for hiding this comment

ikjo39 left a comment

Choose a reason for hiding this comment

ikjo39 Aug 3, 2024 • edited Loading

Choose a reason for hiding this comment

hw0603 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hw0603 Aug 3, 2024 • edited Loading

Choose a reason for hiding this comment

seokmyungham Aug 3, 2024 • edited Loading

Choose a reason for hiding this comment

seokmyungham Aug 3, 2024 • edited Loading

Choose a reason for hiding this comment

ehBeak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehBeak left a comment

Choose a reason for hiding this comment

seokmyungham commented Aug 3, 2024 • edited Loading

변경 사항

seunghye218 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seokmyungham commented Aug 2, 2024 •

edited

Loading

github-actions bot commented Aug 2, 2024 •

edited

Loading

ikjo39 Aug 3, 2024 •

edited

Loading

seokmyungham Aug 3, 2024 •

edited

Loading

seokmyungham Aug 3, 2024 •

edited

Loading

ikjo39 Aug 3, 2024 •

edited

Loading

hw0603 Aug 3, 2024 •

edited

Loading

seokmyungham Aug 3, 2024 •

edited

Loading

seokmyungham Aug 3, 2024 •

edited

Loading

seokmyungham commented Aug 3, 2024 •

edited

Loading