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

feat: 멘토 승인 후 재로그인 로직 구현 #290

Merged
merged 6 commits into from
Nov 27, 2023
Merged

Conversation

ddongpuri
Copy link
Contributor

🖥️ 이런 PR 입니다

  • 멘토로 승인된 후, 토큰을 재발급받아 사용할 수 있도록 구현

CC. 리뷰어

테스트 설명

기타

Prefix

PR 코멘트를 작성할 때 항상 Prefix를 붙여주세요.

  • K1: 꼭 반영해주세요 (Request changes)
  • K2: 웬만하면 반영해 주세요 (Comment)
  • K3: 그냥 사소한 의견입니다 (Approve)

@ddongpuri ddongpuri added the Feat label Nov 26, 2023
@ddongpuri ddongpuri self-assigned this Nov 26, 2023
if (accessToken.isEmpty()) {
return;
private void checkRefreshToken(String accessToken, HttpServletRequest request, HttpServletResponse response) {
Claims claims = jwtService.extractClaim(accessToken);
Copy link
Member

Choose a reason for hiding this comment

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

accessToken이 아니라 claim을 파라미터로 받는건 어떤가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

jwtService.setAccessTokenHeader(response, issuedAccessToken);
}
} else {
throw new TokenException(INVALID_ACCESS_TOKEN);
Copy link
Member

Choose a reason for hiding this comment

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

에러를 던지면 로그인 하지 않은 사용자는 못들어오지 않나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

로그인 안한 사용자는 애초에 accessToken이 없어서 여기 로직을 안탑니당

Comment on lines 53 to 66
public Optional<String> extractRefreshToken(HttpServletRequest request) {
return Optional.ofNullable(request.getHeader(jwtProperties.refresh().headerName()))
.filter(refreshToken -> refreshToken.startsWith(BEARER))
.filter(this::isNotManipulated)
.map(JwtService::refineToken);
}

public Optional<String> extractAccessToken(HttpServletRequest request) {
return Optional.ofNullable(request.getHeader(jwtProperties.access().headerName()))
.filter(refreshToken -> refreshToken.startsWith(BEARER))
.filter(accessToken -> accessToken.startsWith(BEARER))
.filter(this::isNotManipulated)
.map(JwtService::refineToken);
}
Copy link
Member

Choose a reason for hiding this comment

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

getHeader에 넘기는 값만 다르고 나머지는 다 같은데 extractToken(HttpServletRequest request, String type)로 private 메소드를 만드는 건 어떤가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines 80 to 83
if (isOutdatedClaim(claims)) {
log.info("pending -> mentor : 새로 로그인 해야합니다.");
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

authentication 객체를 저장하는 메소드에서 검증이 있는것도 이상하고 단순히 로그만 서버에 남기는 것은 아무 의미가 없어보여요 사용자가 어떻게 알 수 있죠????

차라리 Authorization 필터 앞에 pending 상태인 멘토를 검증하는 필터가 따로 있어서 거기서 약속된 예외를 넘기는 것이 더 좋아보여요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

저도 authentication 객체를 저장하는 메소드에서 검증이 있는 건 좀 이상하다는데 동의합니다 ㅠ
근데 pending에서 mentor로 role이 바뀌었다는 걸 꼭 알아야할까요? 자연스럽게 재로그인하게끔 만들면 되지 않을까요?

그리고 앞단에 pending 멘토 검증용 필터를 따로 둔다면 토큰에 대한 검증이 오히려 중복으로 이루어진다는 단점이 있지 않을까요.?

Copy link
Member

Choose a reason for hiding this comment

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

그럼 단순 로그 하나로 사용자가 어떻게 알 수 있죠?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

return 을 하게되면 이후 필터를 타게 되고,
로그인이 필요한 요청이었다면 401이 반환됩니다

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ddongpuri ddongpuri requested a review from Juhongseok November 26, 2023 15:06
Comment on lines 35 to 37
JwtUser user = (JwtUser) auth.getPrincipal();
return isPending(auth) && mentorService.getOne(user.id()).getRole().equals(ROLE_MENTOR);
Copy link
Member

Choose a reason for hiding this comment

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

개행

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -62,7 +65,8 @@ public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Excepti

http.addFilterBefore(exceptionHandlerFilter, LogoutFilter.class);
http.addFilterAfter(jwtAuthorizationFilter, ExceptionHandlerFilter.class);
http.addFilterAfter(oAuthTokenResponseFilter, JwtAuthorizationFilter.class);
http.addFilterAfter(roleConsistencyCheckFilter, JwtAuthorizationFilter.class);
Copy link
Member

Choose a reason for hiding this comment

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

AuthorizationFilter 앞이 조금더 좋아보여요 권한 관련 체크니까

Copy link
Contributor Author

Choose a reason for hiding this comment

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

필터 위치를 바꾸니까 형변환 에러가 나서 일단 위치 그대로 둔 뒤 내일 함께 얘기해보면 좋을것 같습니다.!

@ddongpuri ddongpuri merged commit ae6e309 into develop Nov 27, 2023
1 check passed
@ddongpuri ddongpuri deleted the feat/GMMQ-709 branch November 27, 2023 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants