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

[refactor] : Auth, User, Jwt 관련 코드 리팩토링을 진행한다 #79

Merged
merged 14 commits into from
Nov 23, 2024

Conversation

bbbang105
Copy link
Member

@bbbang105 bbbang105 commented Nov 21, 2024

✅ PR 유형

어떤 변경 사항이 있었나요?

  • 새로운 기능 추가
  • 버그 수정
  • 코드에 영향을 주지 않는 변경사항(오타 수정, 탭 사이즈 변경, 변수명 변경)
  • 코드 리팩토링
  • 주석 추가 및 수정
  • 문서 수정
  • 빌드 부분 혹은 패키지 매니저 수정
  • 파일 혹은 폴더명 수정
  • 파일 혹은 폴더 삭제

📝 작업 내용

이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)

  • 아래는 제가 해결하고자 한 코드리뷰 코멘트입니다.
  1. [A팀] 백엔드 파트 코드리뷰용 PR #44 (comment)
DB 트랜잭션(@Transactional) 안에서 redis를 사용하고 있는데요.

불필요한 I/O작업을 트랜잭션 안에서 하면 무슨 단점이 있을까요?
트랜잭션 안에서 외부 호출을 하다가 실패하면 어떻게 될까요?
  1. [A팀] 백엔드 파트 코드리뷰용 PR #44 (comment)
Transactional이 필요한 로직일까요?
  1. [A팀] 백엔드 파트 코드리뷰용 PR #44 (comment)
Cookie에 값을 세팅하는게 application layer의 역할일까요?
HTTP로 통신하다가 다른 프로토콜을 이용해 통신하게 된다면 어떻게 될까요?
  1. [A팀] 백엔드 파트 코드리뷰용 PR #44 (comment)
`HttpServletResponse` application 레이어가 서버가 클라이언트와 어떤 프로토콜을 사용해서 통신하는지 알 필요가 있을까요?
  1. [A팀] 백엔드 파트 코드리뷰용 PR #44 (comment)
마찬가지로 트랜잭션 안에서 너무 많은 일을 하고 있는것 같아요
  1. [A팀] 백엔드 파트 코드리뷰용 PR #44 (comment)
단순 조회로직에서 트랜잭션이 필요할까요?
트랜잭션은 개발자한테 어떤 장점을 줄까요?

✏️ 관련 이슈

본인이 작업한 내용이 어떤 Issue Number와 관련이 있는지만 작성해주세요

#78


🎸 기타 사항 or 추가 코멘트

코드에서 변경된 점은 코멘트로 남겨두었습니다! 확인 부탁드립니다.

@bbbang105 bbbang105 added 🚀 feat 새로운 기능 추가 / 일부 코드 추가 / 일부 코드 수정 (리팩토링과 구분) / 디자인 요소 수정 🔄 refactor 코드 리팩토링 🤯 SANGHO 상호 Issue or PR labels Nov 21, 2024
@bbbang105 bbbang105 requested a review from juuuunny November 21, 2024 18:36
@bbbang105 bbbang105 self-assigned this Nov 21, 2024
Copy link
Member Author

@bbbang105 bbbang105 left a comment

Choose a reason for hiding this comment

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

리팩토링 한 부분들에 대해서 설명 달아두었습니다!

Comment on lines -20 to -25
@Value("${spring.jwt.access-token.expiration-time}")
private long ACCESS_TOKEN_EXPIRATION_TIME;

@Value("${spring.jwt.refresh-token.expiration-time}")
private long REFRESH_TOKEN_EXPIRATION_TIME;

Copy link
Member Author

Choose a reason for hiding this comment

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

해당 부분은 여러 서비스 레이어에서 쓰일 수 있기에 JwtUtil에서 불러와 사용할 수 있도록 변경하였습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

좋습니다!
추가적으로 전에 이야기했었던 JwtUtil을 의존성 주입이 아닌 static으로 선언해서 가져오는 것과 관련해서 어떻게 생각하시나요??
저는 util관련된 부분이라 그렇게 바꾸는 것도 좋은 것 같습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

아 그런데 다른 도메인들에서도 jwtUtil의 토큰파싱을 사용하는 부분이 있어서 중간에 꼬일 수도 있을 것 같아 이건 이 브랜치 제일 마지막에 바꾸는 게 좋을 것 같아요!!!

Copy link
Member Author

Choose a reason for hiding this comment

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

찾아보니 이 부분은 사람마다 차이가 많은 것 같아서 저희가 결정하면 될 것 같습니다!!

static : 항상 같은 값을 반환, 내부 상태가 없음, 단순한 로직일 때 유용
Bean 등록 : 비즈니스 로직과 연관이 깊음, 내부 상태가 있음 ..

Comment on lines -29 to -30
@Transactional
public void reIssueToken(String refreshToken, HttpServletResponse response) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Redis와 Cookie만 건드는 로직이라 불필요한 트랜잭션을 제거하였습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

확인했습니다!
Transactional(readOnly = true)와 관련해서 실제 DB에 접근해서 조회하는 것에는 Transactional(readOnly = true)를 붙이고, 외부 접근 메서드에는 트랜잭션을 붙이지 않는게 좋을까요?

Copy link
Member Author

@bbbang105 bbbang105 Nov 23, 2024

Choose a reason for hiding this comment

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

네 저는 그렇게 하고 있어요!
Transactional(readOnly = true) 의 이점에 대해서는 추후 조금 더 공부해서 개인 블로그에 정리해보려고 합니다 😁

Comment on lines +20 to +25
/**
* Refresh Token 검증 및 새로운 토큰 발급.
*
* @param refreshToken 클라이언트로부터 받은 리프레시 토큰
* @return 새로 생성된 Access Token과 Refresh Token
*/
Copy link
Member Author

Choose a reason for hiding this comment

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

아래에도 계속해서 이어지는데, 이런 것처럼 Javadoc 주석 가이드에 맞춰서 작성하고자 하였습니다.
맨 위에는 API나 메서드에 대한 설명이 들어가고, 반드시 마침표(.)로 끝나야 합니다.
아래에는 파라미터나 리턴 값, 예외에 대한 설명이 들어가고, 마침표는 찍지 않는 것이 관례라고 합니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

확인했습니다!! 그럼 controller의 api부분에도 이렇게 주석을 작성하고
domain이나 service 안에 있는 메서드 부분에서도 이렇게 주석을 작성하나요?!

Copy link
Member Author

Choose a reason for hiding this comment

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

주석을 작성 안하고도 알아볼 수 있는 코드가 좋다는 말 을 이전에 봐서 그동안에는 주석을 최대한 작성하지 않으려 했는데요,
아무리 잘 짜려고 해도 남이 보기에는 이해하기 쉽지 않을 것 같더라구요!

그래서 API나 메서드 상단에는 앞으로 Javadoc을 활용해서 주석을 달아보려고 합니다 😁

Comment on lines 3 to 9
public record TokenResponse(
String accessToken,
String refreshToken,
long accessTokenExpiration,
long refreshTokenExpiration
) {
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Service 레이어에서 HttpResponse까지 건들게 되는 것은 불필요하다고 판단하였고,
이를 Presentation 레이어로 위임하기 위한 DTO를 추가하였습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

사소하긴한데 파일명 TokenResponseDto로 바꿔주시면 감사하겠습니다!!

Copy link
Member Author

Choose a reason for hiding this comment

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

변경 완료!

}
// 기존 사용자 처리
else {
userService.handleExistingUser(token, response);
getRedirectStrategy().sendRedirect(request, response, REDIRECT_URI_BASE);
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

핸들러에서 너무 많은 부분을 맡고 있다고 판단했습니다.
유저 정보를 가져오는 것부터, 신규 & 기존 유저에 따라 나누어 토큰을 발급하고 쿠키를 세팅하는 것까지..
이를 userService에 위임하여 처리하고자 하였고, 핸들러에서는 클라이언트 요청 처리와 리다이렉트만 주로 담당하도록 하였습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

주요 로직은 서비스에서 처리하도록 하는 것이 좋은 것 같아요.
그리고 상위에서 하위에서 작성한 메서드를 불러오는 식으로요!
향후 코드 작성 시에도 유의하겠습니다!

Comment on lines 19 to 20
private static final int AUTH_CODE_EXPIRATION_MINUTES = 5; // 인증 코드 유효기간 (분)
private static final int REFRESH_TOKEN_EXPIRATION_DAYS = 14; // 리프레시 토큰 유효기간 (일)
Copy link
Member Author

Choose a reason for hiding this comment

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

기존에는 코드 내부에서 5, 14 이런 식으로 숫자형으로 나와 있었는데, 이는 매직넘버라고 하여 지양해야 할 부분입니다!
해당 숫자가 무엇을 의미하는지 파악하기 어렵기 때문에 이렇게 상수 처리를 해 주는 것이 좋습니다 😁

Copy link
Contributor

Choose a reason for hiding this comment

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

정보 감사합니다!!
궁금한 부분이 토큰 만료시간과 같은 값은 application.yml에서 받아와서 사용을 하는데
이러한 위의 값도 application.yml에 추가해서 @value로 가져오는 것이 좋을 지, 아니면 이렇게 상수로 그대로 사용하는 것이 좋을 지 고민입니다!

Copy link
Member Author

@bbbang105 bbbang105 Nov 23, 2024

Choose a reason for hiding this comment

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

아 이 부분도 yaml 파일로 관리하는 게 조금 더 좋을 것 같기는합니다!
리프레쉬 토큰 유효기간을 바꾸려할 때 이 부분도 바꿔야하는데 지금으로썬 코드까지 변경해야해서 불필요할 것 같아요.

해당 부분 제가 주입 받는 식으로 변경해두겠습니다!

Copy link
Member Author

@bbbang105 bbbang105 Nov 23, 2024

Choose a reason for hiding this comment

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

 @Slf4j
@Component
@RequiredArgsConstructor
public class RedisManager {

    private final StringRedisTemplate redisTemplate;
    private final JWTUtil jwtUtil;

    @Value("${spring.redis.auth-code-expiration-minutes}")
    private int authCodeExpirationMinutes; // 인증 코드 유효기간 (분)

    @Value("${spring.redis.refresh-token-expiration-days}")
    private int refreshTokenExpirationDays; // 리프레시 토큰 유효기간 (일)

...
  redis:
    auth-code-expiration-minutes: ${AUTH_CODE_EXPIRATION_MINUTES} # 인증 코드 유효기간 (분)
    refresh-token-expiration-days: ${REFRESH_TOKEN_EXPIRATION_DAYS} # 리프레시 토큰 유효기간 (일)

요 두개 로컬에서 환경 변수 추가해주시면 됩니다! 인스턴스는 제가 추가해둘게요

Comment on lines +11 to +27
/**
* 인증 쿠키를 설정합니다 (Access Token, Refresh Token, Expiration Time).
*
* @param response HTTP 응답 객체
* @param accessToken 액세스 토큰
* @param refreshToken 리프레시 토큰
* @param accessTokenExpiry 액세스 토큰 만료 시간 (밀리초)
* @param refreshTokenExpiry 리프레시 토큰 만료 시간 (밀리초)
*/
public static void setAuthCookies(HttpServletResponse response, String accessToken, String refreshToken,
long accessTokenExpiry, long refreshTokenExpiry) {
setCookie(response, "accessToken", accessToken, (int) (accessTokenExpiry * 1.5) / 1000);
setCookie(response, "refreshToken", refreshToken, (int) refreshTokenExpiry / 1000);
setNotHttpOnlyCookie(response, "expirationTime",
String.valueOf((int) accessTokenExpiry / 1000),
(int) (accessTokenExpiry * 1.5) / 1000);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

역할을 최대한 분리하기 위해 메서드를 만들어 따로 분리해보았습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

감사합니다! static으로 작성한 것 좋아요

Comment on lines 39 to 50
// 1인 1계정 처리를 위한 이미 등록된 회원인지 확인하여 알려주기
// User existedUser = userRepository.findByPhoneNumber(request.phoneNumber());
// if (existedUser != null && existedUser.getProvider() != ProviderStatusType.of(provider)) {
// if (existedUser.getProvider() == ProviderStatusType.of("kakao")) {
// throw new CustomException(UserErrorStatus._EXISTING_USER_ACCOUNT_KAKAO);
// } else if (existedUser.getProvider() == ProviderStatusType.of("google")) {
// throw new CustomException(UserErrorStatus._EXISTING_USER_ACCOUNT_GOOGLE);
// } else if (existedUser.getProvider() == ProviderStatusType.of("naver")) {
// throw new CustomException(UserErrorStatus._EXISTING_USER_ACCOUNT_NAVER);
// }
// }

Copy link
Member Author

Choose a reason for hiding this comment

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

이 부분 코드가 아예 제거돼버렸네요..!
주석 처리로 다시 바꿔두겠습니다

Comment on lines +38 to +64
/**
* OAuth2 사용자 정보 추출 및 신규 여부 확인.
*
* @param token OAuth2 인증 토큰
* @return 신규 사용자 여부
*/
public boolean isNewUser(OAuth2AuthenticationToken token) {
OAuth2UserInfo oAuth2UserInfo = extractOAuth2UserInfo(token);
return userRepository.findByProviderId(oAuth2UserInfo.getProviderId()) == null;
}

/**
* 신규 사용자 처리.
*
* @param token OAuth2 인증 토큰
* @param response HTTP 응답 객체
*/
public void handleNewUser(OAuth2AuthenticationToken token, HttpServletResponse response) {
OAuth2UserInfo oAuth2UserInfo = extractOAuth2UserInfo(token);
String registerToken = jwtUtil.generateRegisterToken(
oAuth2UserInfo.getProvider(),
oAuth2UserInfo.getProviderId(),
oAuth2UserInfo.getEmail()
);
CookieUtil.setCookie(response, "registerToken", registerToken, (int) jwtUtil.getRegisterTokenExpirationTime() / 1000);
log.info("신규 사용자 처리 완료: {}", oAuth2UserInfo.getEmail());
}
Copy link
Member Author

Choose a reason for hiding this comment

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

OAuth 핸들러에서 모두 처리하던 부분을 UserService에서 위임하도록 하였습니다!
하고 보니 여기서는 서비스 레이어에서 cookie를 설정하고 있네요...이 부분 어떻게 개선하면 좋을지 고민해봐야겠습니다 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

쿠키나 레디스와 같은 설정은 컨트롤러 기반에서 하는 게 좋다하긴 했는데 레지스터 토큰은 API 부분이 아니라 고민해봐야겠네요.

return UserInfoResponseDto.from(user);
}

@Transactional
Copy link
Member Author

Choose a reason for hiding this comment

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

불필요한 트랜잭션 제거하였습니다.

Copy link
Contributor

@juuuunny juuuunny left a comment

Choose a reason for hiding this comment

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

빠르게 수정해주셔서 감사합니다!!

Comment on lines -29 to -30
@Transactional
public void reIssueToken(String refreshToken, HttpServletResponse response) {
Copy link
Contributor

Choose a reason for hiding this comment

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

확인했습니다!
Transactional(readOnly = true)와 관련해서 실제 DB에 접근해서 조회하는 것에는 Transactional(readOnly = true)를 붙이고, 외부 접근 메서드에는 트랜잭션을 붙이지 않는게 좋을까요?

Comment on lines -20 to -25
@Value("${spring.jwt.access-token.expiration-time}")
private long ACCESS_TOKEN_EXPIRATION_TIME;

@Value("${spring.jwt.refresh-token.expiration-time}")
private long REFRESH_TOKEN_EXPIRATION_TIME;

Copy link
Contributor

Choose a reason for hiding this comment

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

좋습니다!
추가적으로 전에 이야기했었던 JwtUtil을 의존성 주입이 아닌 static으로 선언해서 가져오는 것과 관련해서 어떻게 생각하시나요??
저는 util관련된 부분이라 그렇게 바꾸는 것도 좋은 것 같습니다!

Comment on lines -20 to -25
@Value("${spring.jwt.access-token.expiration-time}")
private long ACCESS_TOKEN_EXPIRATION_TIME;

@Value("${spring.jwt.refresh-token.expiration-time}")
private long REFRESH_TOKEN_EXPIRATION_TIME;

Copy link
Contributor

Choose a reason for hiding this comment

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

아 그런데 다른 도메인들에서도 jwtUtil의 토큰파싱을 사용하는 부분이 있어서 중간에 꼬일 수도 있을 것 같아 이건 이 브랜치 제일 마지막에 바꾸는 게 좋을 것 같아요!!!

Comment on lines +20 to +25
/**
* Refresh Token 검증 및 새로운 토큰 발급.
*
* @param refreshToken 클라이언트로부터 받은 리프레시 토큰
* @return 새로 생성된 Access Token과 Refresh Token
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

확인했습니다!! 그럼 controller의 api부분에도 이렇게 주석을 작성하고
domain이나 service 안에 있는 메서드 부분에서도 이렇게 주석을 작성하나요?!

CookieUtil.setCookie(response, "refreshToken", newRefreshToken, (int) REFRESH_TOKEN_EXPIRATION_TIME / 1000);
CookieUtil.setNotHttpOnlyCookie(response, "expirationTime", String.valueOf((int) ACCESS_TOKEN_EXPIRATION_TIME / 1000), (int) (ACCESS_TOKEN_EXPIRATION_TIME * 1.5) / 1000);
redisManager.saveRefreshToken(userId.toString(), newRefreshToken);
return new TokenResponse(newAccessToken, newRefreshToken, jwtUtil.getAccessTokenExpirationTime(), jwtUtil.getRefreshTokenExpirationTime());
Copy link
Contributor

Choose a reason for hiding this comment

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

이렇게 토큰 값들을 dto로 반환하고 쿠키 저장하는 메소드를 따로 분리한 것 좋은 것 같아요!!!
감사합니다!

Comment on lines 19 to 20
private static final int AUTH_CODE_EXPIRATION_MINUTES = 5; // 인증 코드 유효기간 (분)
private static final int REFRESH_TOKEN_EXPIRATION_DAYS = 14; // 리프레시 토큰 유효기간 (일)
Copy link
Contributor

Choose a reason for hiding this comment

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

정보 감사합니다!!
궁금한 부분이 토큰 만료시간과 같은 값은 application.yml에서 받아와서 사용을 하는데
이러한 위의 값도 application.yml에 추가해서 @value로 가져오는 것이 좋을 지, 아니면 이렇게 상수로 그대로 사용하는 것이 좋을 지 고민입니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

RedisManager 내부에서 커스텀 에러 던지는 것으로 바뀐 것 좋은 것 같습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

저도 그동안 로그를 너무 작성 안하였는데 로그 작성 연습을 조금 해야할 것 같아요.

Comment on lines +11 to +27
/**
* 인증 쿠키를 설정합니다 (Access Token, Refresh Token, Expiration Time).
*
* @param response HTTP 응답 객체
* @param accessToken 액세스 토큰
* @param refreshToken 리프레시 토큰
* @param accessTokenExpiry 액세스 토큰 만료 시간 (밀리초)
* @param refreshTokenExpiry 리프레시 토큰 만료 시간 (밀리초)
*/
public static void setAuthCookies(HttpServletResponse response, String accessToken, String refreshToken,
long accessTokenExpiry, long refreshTokenExpiry) {
setCookie(response, "accessToken", accessToken, (int) (accessTokenExpiry * 1.5) / 1000);
setCookie(response, "refreshToken", refreshToken, (int) refreshTokenExpiry / 1000);
setNotHttpOnlyCookie(response, "expirationTime",
String.valueOf((int) accessTokenExpiry / 1000),
(int) (accessTokenExpiry * 1.5) / 1000);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

감사합니다! static으로 작성한 것 좋아요

return String.valueOf((int) (Math.random() * 900000) + 100000); // 6자리 인증 코드 생성
final int AUTH_CODE_BOUND = 900000;
final int AUTH_CODE_OFFSET = 100000;
return String.valueOf((int) (Math.random() * AUTH_CODE_BOUND) + AUTH_CODE_OFFSET);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

아하 이런 숫자값 들도 다 상수로 처리해주는 게 좋은 가보네요!!

Copy link
Member Author

Choose a reason for hiding this comment

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

고정값은 상수로 표현하는 게 좋은 것 같습니다!!

Comment on lines 3 to 9
public record TokenResponse(
String accessToken,
String refreshToken,
long accessTokenExpiration,
long refreshTokenExpiration
) {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

사소하긴한데 파일명 TokenResponseDto로 바꿔주시면 감사하겠습니다!!

@bbbang105 bbbang105 merged commit c8794ab into refactor-v1 Nov 23, 2024
@bbbang105 bbbang105 deleted the feature/#78/first-refactor branch November 23, 2024 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚀 feat 새로운 기능 추가 / 일부 코드 추가 / 일부 코드 수정 (리팩토링과 구분) / 디자인 요소 수정 🔄 refactor 코드 리팩토링 🤯 SANGHO 상호 Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants