-
Notifications
You must be signed in to change notification settings - Fork 56
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
[Spring MVC] 윤성원 미션 제출합니다. #24
base: mete0rfish
Are you sure you want to change the base?
Conversation
…er and unauthorized user
…er and unauthorized user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요
미션을 일찍 제출하셨는데 리뷰가 늦어져서 죄송합니다!
제가 2단계,3단계 요구사항에 대한 이해도가 낮아서 양질의 리뷰를 작성하지 못하여 죄송하단 말씀 드리고 싶습니다.
구조에 대해 고민을 많이 하셨다고 하셨는데 느껴지는 코드였습니다. 제 코드에 반영해볼 부분이 많은 것 같아서 사실상 리뷰보단 궁금한 점 위주로 남긴 것 같습니다.
이번 미션도 고생 많으셨습니다!
public class JwtDecoder { | ||
public static Long decodeJwtToken(String token) { | ||
Long memberId = Long.valueOf(Jwts.parserBuilder() | ||
.setSigningKey(Keys.hmacShaKeyFor("Yn2kjibddFAWtnPJ2AFlL8WXmohJMCvigQggaEypa5E=".getBytes())) | ||
.build() | ||
.parseClaimsJws(token) | ||
.getBody().getSubject()); | ||
return memberId; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JwtDecoder
와 JwtProvider
를 나눠서 진행한 것이 인상깊습니다.
시크릿키가 여러 곳에서 사용되는 것을 막기위해 JwtProvider에서만 사용하셨다고 하셨는데, 디코더에서는 노출되게 작성해도 괜찮은 것일까요? 이 부분 궁금합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
실수가 있었네요. 9번째 줄에 있는 scretkey 또한 필드에서 @value를 통해 사용하도록 수정해볼께요.
@Value("${roomescape.auth.jwt.secret}") | ||
private String secretKey; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 private final String SECRET_KEY = "Yn2kjibddFAWtnPJ2AFlL8WXmohJMCvigQggaEypa5E=";
로 시크릿키를 노출되게 작성하였는데 참고해서 바꿔봐야 할 것 같아요. 새로운 점 배워갑니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 쿠키에 관한 메서드도 MemberService에 작성했는데 분리하는게 더 좋은 것 같아보이네요 참고하겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
도메인형 패키지 구조로 작성 시, global
과 local
적인 면을 많이 고려했습니다. memberService
의 경우, member 도메인에 대해 global 적으로 사용됩니다. 그러나 Cookie와 관련된 로직
은 member 외에 reservation 등 global적으로 사용될 수 있자고 생각하여 util로 분류 하였습니다
@Override | ||
public boolean supportsParameter(MethodParameter parameter) { | ||
/* | ||
커스텀 어노테이션 사용 시 사용할 수 있는 코드 | ||
boolean hasLoginMemberAnnotation = parameter.hasParameterAnnotation(LoginMember.class); | ||
*/ | ||
return Member.class.isAssignableFrom(parameter.getParameterType()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹시 주석으로 /* 커스텀 어노테이션 사용 시 사용할 수 있는 코드 boolean hasLoginMemberAnnotation = parameter.hasParameterAnnotation(LoginMember.class); */
남겨두신 이유가 있을까요? 궁금해요
cookie.setHttpOnly(true); | ||
cookie.setPath("/"); | ||
response.addCookie(cookie); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이부분은 MemberService를 통해서 하나의 메서드로 만들어도 괜찮지 않을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cookie의 경우, 요청 시 특정 값만 추출(예: 토큰) 합니다. 따라서 Service Layer
까지 들어올 경우 비즈니스 로직 관심사에 벗어난다고 생각했습니다.
드디어 종강이네요! 이번 한 학기 고생 많으셨습니다~
이번 미션은 낮선 부분이여서 많이 헤맸던 것 같아요 😁
고민한 부분
1. 패키지 구조
2. secretKey의 위치
3. 어드민과 일반 회원 구분은 인터셉트에서
그 외에 이상한 부분 혹은 비효율적인 부분도 지적해주시면 많은 도움이 될 것 같습니다!