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 : ArgumentResolver -> Interceptor로 변경 #200

Merged
merged 12 commits into from
Nov 8, 2024

Conversation

2iedo
Copy link
Collaborator

@2iedo 2iedo commented Nov 8, 2024

#️⃣ 연관된 이슈

ex) #198

📝 작업 내용

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

  • ArgumentResolver -> Interceptor로 변경

스크린샷 (선택)

💬 리뷰 요구사항(선택)

리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요
기존의 argumentResolver는 @memberid가 파라미터로 있는 메소드에 해당하는 경로만 자동으로 작업을 수행하여 memberId를 반환했으나, Interceptor의 경우 경로에 따라 해당 interceptor를 적용할 경로를 지정해야 하는데, 몇몇 도메인(auth, callback, review 등등)은 일부 경로에서 memberId를 사용하지 않습니다. 몇 개의 경로에 대하여 테스트를 해보았으나, 혹시 jwt가 필요하지 않은데 적용된 경로가 있으면 피드백 부탁드립니다! (review 도메인의 리뷰 조회 - 등록의 경우 경로가 일치한데, 요청만 달라 리뷰 조회 시에 뒤에 '/all' 경로를 추가하였습니다.)

21:21 컨트롤러의 메소드의 파라미터로 Long타입(memberId)를 가질 때만 jwt를 검증하도록 수정했습니다. 모든 메소드를 확인해보니 memberId가 있는 메소드만 Long타입을 가져서 이렇게 적용했습니다. 인터셉터의 로직을 이렇게 수정했기 때문에, webConfig부분에서 경로를 지정할 때 훨씬 짧게 설정할 수 있습니다. (callId, seniorId 등이 있긴 한데, 해당 경우 모두 memberId를 파라미터로 가집니다!)

⏰ 현재 버그

✏ Git Close

close #198

@2iedo 2iedo added the ♻️ Refactoring 코드 리팩토링 & 클린 코드 작업을 진행하는 경우 label Nov 8, 2024
@2iedo 2iedo requested review from zzoe2346, eunsoni and GitJIHO November 8, 2024 09:16
@2iedo 2iedo self-assigned this Nov 8, 2024
@2iedo 2iedo linked an issue Nov 8, 2024 that may be closed by this pull request
1 task
Copy link
Contributor

@zzoe2346 zzoe2346 left a comment

Choose a reason for hiding this comment

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

변경하시느라 고생하셨습니다! 궁금한거 코멘트하나 남겼습니다 :)

guardService.deleteGuard(memberId);
return ResponseEntity.ok("보호자가 삭제되었습니다.");
}

@Operation(summary = "모든 보호자 조회", description = "관리자용 API입니다.")
@GetMapping("/all")
public ResponseEntity<List<GuardResponse>> getAllGuards(@MemberId Long memberId) {
public ResponseEntity<List<GuardResponse>> getAllGuards(@RequestAttribute("memberId") Long memberId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

이 메서드에서는 memberId 가 안쓰이는데 필요한 이유가 있을까요? HelloCallController 에도 3개정도 안쓰이는 memberId가있는데 사용 안하면 제거하는게 깔끔하지 않나 하는 생각입니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

관리자용은 나중에 따로 보안을 적용해서 제거하는게 옳다고 생각하지만, HelloCallController에 있는 memberId의 경우 제거했을 때 회원이 아니어도 조회가 가능하다고 생각되어 일단 남겨두겠습니다!

Comment on lines 73 to 83
@Override
public void addInterceptors(InterceptorRegistry registry) {
registry.addInterceptor(jwtInterceptor).addPathPatterns("/api/**")
.excludePathPatterns("/api/auth/**")
.excludePathPatterns("/api/reviews")
.excludePathPatterns("/api/members/sinitto")
.excludePathPatterns("/api/members/guard")
.excludePathPatterns("/api/reviews/all")
.excludePathPatterns("/api/callbacks/twilio")
.excludePathPatterns("/api/hellocalls/admin/reports");
}
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 20 to 41
@Override
public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler){
String authorizationHeader = request.getHeader("Authorization");
if (authorizationHeader == null || !authorizationHeader.startsWith("Bearer ")) {
throw new UnauthorizedException("토큰이 없거나, 헤더 형식에 맞지 않습니다.");
}
if (handler instanceof HandlerMethod) {
HandlerMethod handlerMethod = (HandlerMethod) handler;
Method method = handlerMethod.getMethod();

Class<?>[] parameterTypes = method.getParameterTypes();

String token = authorizationHeader.substring(7);
for (Class<?> paramType : parameterTypes) {
if (paramType.equals(Long.class)) {
String authorizationHeader = request.getHeader("Authorization");
if (authorizationHeader == null || !authorizationHeader.startsWith("Bearer ")) {
throw new UnauthorizedException("토큰이 없거나, 헤더 형식에 맞지 않습니다.");
}

request.setAttribute("memberId", memberTokenService.getMemberIdByToken(token));
String token = authorizationHeader.substring(7);

request.setAttribute("memberId", memberTokenService.getMemberIdByToken(token));
return true;
}
}
}
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
Collaborator

@eunsoni eunsoni left a comment

Choose a reason for hiding this comment

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

인터셉터로 JWT 인증하는 방식으로 깔끔하게 잘 바꿔주신 것 같아요!
인증 제외한 path도 확인해봤는데 제가 맡은 부분은 문제없을 것 같습니다
수고많으셨어요 😄

Comment on lines +29 to +33
if (paramType.equals(Long.class)) {
String authorizationHeader = request.getHeader("Authorization");
if (authorizationHeader == null || !authorizationHeader.startsWith("Bearer ")) {
throw new UnauthorizedException("토큰이 없거나, 헤더 형식에 맞지 않습니다.");
}
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
Member

@GitJIHO GitJIHO 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 +26 to +29
Class<?>[] parameterTypes = method.getParameterTypes();

for (Class<?> paramType : parameterTypes) {
if (paramType.equals(Long.class)) {
Copy link
Member

Choose a reason for hiding this comment

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

오 ㅋㅋ 신박하네요 👍

@2iedo 2iedo merged commit 6472e57 into Weekly Nov 8, 2024
1 check passed
@2iedo 2iedo deleted the Refactor/issue-#198 branch November 14, 2024 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
♻️ Refactoring 코드 리팩토링 & 클린 코드 작업을 진행하는 경우
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor : ArgumentResolver -> Interceptor로 변경
4 participants