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

[BE] 로그아웃 기능 및 인증 관련 정비 #608

Merged
merged 8 commits into from
Oct 4, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -51,28 +51,54 @@ public ResponseEntity<TokenResponse> oauthLogin(
@Operation(summary = "access 토큰, refresh 토큰 갱신")
@PostMapping("/api/auth/refresh")
public ResponseEntity<TokenResponse> refresh(
HttpServletRequest httpServletRequest,
HttpServletResponse httpServletResponse
HttpServletRequest request,
HttpServletResponse response
) {
String refreshToken = extractRefreshTokenFromCookie(httpServletRequest);
String refreshToken = extractRefreshTokenFromCookie(request);
TokenResponse tokenResponse = authService.refresh(refreshToken);
Cookie cookie = setUpRefreshTokenCookie(tokenResponse);
httpServletResponse.addCookie(cookie);
response.addCookie(cookie);
return ResponseEntity.ok(tokenResponse);
}

private Cookie setUpRefreshTokenCookie(TokenResponse tokenResponse) {
Cookie cookie = new Cookie("refreshToken", tokenResponse.refreshToken().toString());
cookie.setMaxAge(refreshTokenExpireLength.intValue() / 1000);
cookie.setPath("/");
cookie.setHttpOnly(true);
cookie.setSecure(true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

굳 👍

return cookie;
}

private String extractRefreshTokenFromCookie(HttpServletRequest httpServletRequest) {
return Arrays.stream(httpServletRequest.getCookies())
private String extractRefreshTokenFromCookie(HttpServletRequest request) {
return Arrays.stream(request.getCookies())
.filter(cookie -> cookie.getName().equals("refreshToken"))
.map(Cookie::getValue)
.findAny()
.orElseThrow(RefreshTokenNotExistsException::new);
}

@Operation(summary = "로그아웃, access & refresh 토큰 삭제")
@PostMapping("/api/auth/logout")
public ResponseEntity<Void> logout(
HttpServletRequest request,
HttpServletResponse response
) {
String refreshToken = extractRefreshTokenFromCookie(request);
authService.deleteStringifiedRefreshToken(refreshToken);

deleteCookies(request, response);

return ResponseEntity.ok().build();
}

private void deleteCookies(HttpServletRequest request, HttpServletResponse response) {
Cookie[] cookies = request.getCookies();
if (cookies != null) {
for (Cookie cookie : cookies) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

모든 쿠키를 지우는 것보다 refreshToken인 쿠키만 지우는 게 안정적이지 않을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

굿 반영했습니다~

cookie.setMaxAge(0);
response.addCookie(cookie);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,16 @@
import java.util.Optional;
import java.util.UUID;
import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.data.jpa.repository.Modifying;
import org.springframework.data.jpa.repository.Query;

public interface RefreshTokenRepository extends JpaRepository<RefreshToken, Long> {

Optional<RefreshToken> findByUuid(UUID refreshToken);

Optional<RefreshToken> findByMember(Member member);

@Modifying
@Query("delete from RefreshToken r where r.uuid = :uuid")
void deleteTokenWithoutContextUpdate(UUID uuid);
Copy link
Collaborator

Choose a reason for hiding this comment

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

JPA 가물가물 하네요ㅋㅋㅋㅋ
영속성 컨텍스트를 업데이트하지 않고 쿼리가 나가서 withoutContextUpdate라는 이름이 붙은 것 같네용 표현 좋습니다👍
이름의 앞 부분만 deleteToken 보다는 deleteByUuidWithout~ 요런 식의 관례?와 같은 이름은 어떤가요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ModifyingflushAutomatically 옵션 주는건 어떻게 생각하시는지요?

같은 모씨로써 답변 부탁드립니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@aak2075
마코 말씀처럼 네이밍 수정 했습니다~

@woosung1223
flush & clear를 할 지 말 지 되게 고민했는데 ㅋㅋㅋ
flush 혹은 clear가 들어간다는 것은 서비스 로직에서 이 메서드 외에 다른 쿼리가 나갈 일이 있다는 것인데요,

우선은 flush를 하게 된다면 clear 옵션도 같이 줘야 할 것 같아요.
하지만 저는 로그아웃만을 위한 단건 쿼리만 날라가는 것을 바라는 것인데, flush & clear를 한다는 것은 추가적인 쿼리가 함께 수행되는 서비스 로직을 고려한다는 의미니까 만든 의도와 조금 달라지는 것 같아서요,

네이밍으로 우리가 유의하며 사용하도록 했다고 생각하는데 부족하다고 생각하시나요?


추가로 https://joojimin.tistory.com/71 hibernate의 기본 설정에 의해 쿼리 발생 시점에 flush가 자동 실행된다고 하네요 ㅎㅎ
서비스 로직 상 나갈 일은 없겠지만...

Copy link
Collaborator

Choose a reason for hiding this comment

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

오호 Hibernate가 자동으로 flush를 해주면 상관은 없겠네요~

다만 저는 메소드 시그니쳐에서 ContextUpdate라는 부분이 걸려 코멘트 달았던 것이었습니다.
사용하는 입장에서 영속성 컨텍스트의 존재에 대해 알아야 할 필요가 있을까? 싶어서요.

어쨌거나 flush가 발생한다면 deleteTokenWithoutContextUpdate 보다는 deleteToken이 명확한 것 같은데
(매번 사용할 때마다 컨텍스트를 거쳐야 하는지, 거치지 말아야 하는지 고민하지 않아도 되고요)

어떻게 생각하시는지요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

저는 사용하는 입장에서 의도적으로 영속성 컨텍스트와 관련해 어떻게 처리되는지 확인해보라는 의미에서 별도의 이름을 지정한 것인데요,
컨텍스트까지 고려하여 사용해야 할 서비스 로직에서는 이 메서드가 아닌 기본으로 제공되는 deleteBy..를 사용하지 않을까요?
hibernate 기본 설정이 어떤 것이든과 무관하게요.

저는 JPA 특성 상 select 후 delete처리를 하는 불필요한 로직을 간소화하고 싶었거든요,
이 의도에 대한 네이밍이 부족하다 느껴지시면 추가 코멘트 부탁드립니다 ㅎㅎㅎ

Copy link
Collaborator

@woosung1223 woosung1223 Oct 4, 2023

Choose a reason for hiding this comment

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

오호 그렇군요! 그런거라면 납득이 됩니다 굿

그런데 조금 생각해보니까 JPARepository의 Query Method를 사용하지 않은 이유가 궁금해졌습니다.
단순 컬럼으로 삭제하는거라면 @query 를 달아주지 않아도 괜찮지 않으려나요?

+++ 추가로

저는 JPA 특성 상 select 후 delete처리를 하는 불필요한 로직을 간소화하고 싶었거든요,

아 여기에 적혀 있었군요 ㅋㅋ 그런거라면 이해가 충분히 됩니다~

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

제가 틀릴수도 있는데 Query Method를 사용하면 무조건 컨텍스트 정합성을 위해 select 후 delete가 나가지 않나요?
방법이 있다면 알려주세요 😅

Copy link
Collaborator

@MoonJeWoong MoonJeWoong Oct 4, 2023

Choose a reason for hiding this comment

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

모디가 알고 계신게 맞습니당. 저도 이번 기회에 몰랐던 부분 알게 되었네요 ㅎㅎ

// 삭제 검증
memberRepository.delete(member1);
memberRepository.delete(member2);
// 결과
Hibernate: 
    select
        member0_.member_id as member_i1_1_0_,
        member0_.created_date as created_2_1_0_,
        member0_.updated_date as updated_3_1_0_,
        member0_.age as age4_1_0_,
        member0_.team_id as team_id6_1_0_,
        member0_.username as username5_1_0_ 
    from
        member member0_ 
    where
        member0_.member_id=?

Hibernate: 
    select
        member0_.member_id as member_i1_1_0_,
        member0_.created_date as created_2_1_0_,
        member0_.updated_date as updated_3_1_0_,
        member0_.age as age4_1_0_,
        member0_.team_id as team_id6_1_0_,
        member0_.username as username5_1_0_ 
    from
        member member0_ 
    where
        member0_.member_id=?

Hibernate: 
    delete 
    from
        member 
    where
        member_id=?

Hibernate: 
    delete 
    from
        member 
    where
        member_id=?

저는 JPA 특성 상 select 후 delete처리를 하는 불필요한 로직을 간소화하고 싶었거든요,

그리고 수행하는 로직으로 인한 영향이 Repository 단 메서드 네이밍에 반영되어야 하는지 계속 고민해봤는데 요 의도를 반영하고 싶으셨다는 이유로 이해할 수 있었습니다. 깊은 고민 내용 공유해주셔서 감사해용~ 👍

}
Original file line number Diff line number Diff line change
Expand Up @@ -93,4 +93,9 @@ public void validateAccessToken(String accessToken) {
public String parseMemberId(String accessToken) {
return jwtTokenProvider.parseSubject(accessToken, tokenConfig.secretKey());
}

public void deleteStringifiedRefreshToken(String refreshToken) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

역시 독수리의 영어실력은 다르군요 👀

UUID uuid = UUID.fromString(refreshToken);
refreshTokenRepository.deleteTokenWithoutContextUpdate(uuid);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,19 @@
import java.util.Map;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Component;
import org.springframework.transaction.annotation.Transactional;

@RequiredArgsConstructor
@Component
@Transactional
Copy link
Collaborator

@aak2075 aak2075 Oct 2, 2023

Choose a reason for hiding this comment

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

이거 oauth 쪽 액세스 토큰 받아오고, userInfo 받아오는 과정은 외부와 통신이고 우리 쪽 db와는 관련이 없어서 transactional을 안 걸어줬던 걸로 기억합니다. (db작업이 없는데 외부 API 통신 때문에 커넥션을 오래 들고 있으면 안되니까요)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Facade가 사실 Facade Service니까 그러한 측면에선 상위의 트랜잭션이 전파되는게 맞지 않나 싶어서 달아줄까 했는데
말씀처럼 최근 OSIV 설정을 껐으니 성능 측면에서 여기에 걸지 않는 편이 유리하겠네요 ㅎㅎ

우선은 다시 제거했습니다!

public class OauthLoginFacade {

private final OauthProperties oauthProperties;
private final GoogleOauthClient googleOauthClient;
private final AuthService authService;

public TokenResponse oauthLogin(OauthLoginRequest request) {
// TODO: 에러 핸들링
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.

처음에는 webClient단에서 발생하는 예외를 500으로 처리해줬었는데
이게 서버 내부 문제가 아니니 500이 아닌 예외로 잡아주는게 좋지 않을까 합니다~ provider가 늘어나기도 했고요

webClient 관련 에러 핸들링으로 변경해줬습니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

추가로 #605 마쌤이 이번 PR에서 처리를 해준 것 같네요~
확인해보고 조금 뒤에 TODO 지우는 커밋 추가로 남길게요

UserInfo userInfo = requestUserInfo(request.oauthProvider(), request.code());
return authService.userLogin(request, userInfo);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
package harustudy.backend.auth.service;

import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
import static org.assertj.core.api.SoftAssertions.assertSoftly;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.BDDMockito.given;

Expand All @@ -12,7 +16,9 @@
import harustudy.backend.auth.dto.OauthTokenResponse;
import harustudy.backend.auth.dto.TokenResponse;
import harustudy.backend.auth.dto.UserInfo;
import harustudy.backend.auth.exception.InvalidAccessTokenException;
import harustudy.backend.auth.infrastructure.GoogleOauthClient;
import harustudy.backend.auth.util.JwtTokenProvider;
import harustudy.backend.member.domain.LoginType;
import harustudy.backend.member.domain.Member;
import jakarta.persistence.EntityManager;
Expand All @@ -38,6 +44,9 @@ class AuthServiceTest {
@Autowired
private AuthService authService;

@Autowired
private JwtTokenProvider jwtTokenProvider;

@Autowired
private TokenConfig tokenConfig;

Expand Down Expand Up @@ -109,4 +118,55 @@ class AuthServiceTest {
// then
assertThat(response.refreshToken()).isNotEqualTo(refreshToken.getUuid());
}

@Test
void 유효한_액세스_토큰의_유효성_검사_시_예외를_반환한다() {
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.

좋은 지적이네요~~

// given
Long memberId = 1L;
String accessToken = jwtTokenProvider.builder()
.subject(String.valueOf(memberId))
.accessTokenExpireLength(999999L)
.secretKey(tokenConfig.secretKey())
.build();

// when, then
assertDoesNotThrow(() -> authService.validateAccessToken(accessToken));
}

@Test
void 만료된_액세스_토큰의_유효성_검사_시_예외를_반환한다() {
// given
Long memberId = 1L;
String accessToken = jwtTokenProvider.builder()
.subject(String.valueOf(memberId))
.accessTokenExpireLength(0L)
.secretKey(tokenConfig.secretKey())
.build();

// when, then
assertThatThrownBy(() -> authService.validateAccessToken(accessToken)).isInstanceOf(
InvalidAccessTokenException.class);
}

@Test
void 갱신_토큰을_삭제한다() {
// given
Member member = new Member("test", "[email protected]", "test.png", LoginType.GOOGLE);
RefreshToken refreshToken = new RefreshToken(member,
tokenConfig.refreshTokenExpireLength());

entityManager.persist(member);
entityManager.persist(refreshToken);
entityManager.flush();
entityManager.clear();

// when
String stringifiedUUID = refreshToken.getUuid().toString();

// then
assertNotNull(entityManager.find(RefreshToken.class, refreshToken.getId()));
authService.deleteStringifiedRefreshToken(stringifiedUUID);
entityManager.clear();
assertNull(entityManager.find(RefreshToken.class, refreshToken.getId()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

사소한거긴 하지만
Junit의 assertion을 사용하는 이유가 있나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

성능 상 차이가 없으면 assertJ나 JUnit 구분하지 않고 네이밍이 가장 분명한 것으로 사용하곤 있는데 혹시 물어보신 이유가 있을까요?

}
}