-
Notifications
You must be signed in to change notification settings - Fork 118
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_정진택 2주차 과제 (3단계) #394
base: jjt4515
Are you sure you want to change the base?
Conversation
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.
진택님, 안녕하세요! 3단계를 함께 진행할 호선우에요.
현재 서버에서 토큰을 생성하는 로직은 있지만, 클라이언트가 보낸 토큰을 복호화하여 사용하는 부분은 구현이 되어있지 않은 것 같아요.
토큰 기반 인증은 데이터 무결성, 기밀성, 인증 및 권한 부여 장점이 있기때문에 요청 객체로 memberId 필드를 받을 때보다 사용자의 신원과 권한을 안전하게 확인하고 보호할 수 있어요! 해당 로직 구현을 해보면 좋을 것 같아요😀 로직 구현에 어려움이 있다면 DM 이나 코멘트주시면 함께 확인해볼 수 있을 것 같아요.
함께 작업하면서 반영하면 좋을 것 같은 코멘트를 남겼는데, 확인 부탁드립니다🙇♀️
@PostMapping("/wishlist/add") | ||
public void addToWishlist(@Valid @RequestBody WishlistNameRequest request) { | ||
wishlistService.addItemToWishlist(request); | ||
} | ||
|
||
@DeleteMapping("/wishlist/delete") | ||
public void deleteFromWishlist(@Valid @RequestBody WishlistIdRequest request) { | ||
wishlistService.deleteItemFromWishlist(request); | ||
} | ||
|
||
@GetMapping("/wishlist/get/{memberId}") | ||
public List<WishlistItem> getWishlist(@PathVariable Long memberId) { | ||
return wishlistService.getWishlistByMemberId(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.
RESTful API 에 맞춰서 api path 를 변경해볼 수 있을 것 같아요😀
첫 번째, URI는 정보의 자원을 표현해야 한다.
두 번째, 자원에 대한 행위는 HTTP Method(GET, POST, PUT, DELETE)로 표현한다.
fyi; RESTful API 문서
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.
넵 변경했습니다!
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.
좋습니다👍
@DeleteMapping("/{itemId}")
public void deleteFromWishlist(@PathVariable Long itemId, @RequestParam Long memberId) {
wishlistService.deleteItemFromWishlist(itemId, memberId);
}
@GetMapping("/{memberId}")
public List<WishlistItem> getWishlist(@PathVariable Long memberId) {
return wishlistService.getWishlistByMemberId(memberId);
}
api 를 호출한다고 생각했을 때, DELETE wishlist/1 은 itemId 라고 생각할 수 도 있을 것 같아요! memberId 이기때문에 member 라는 리소스를 path 에 추가해줘도 좋을 것 같아요.
@GetMapping("/member/{memberId}")
public List<WishlistItem> getWishlist(@PathVariable Long memberId) {
return wishlistService.getWishlistByMemberId(memberId);
}
진택님 2단계 PR 에서 반영이 필요하다고 생각되는 부분 반영 부탁드릴게요! 그 과정에서 고민되는 부분 공유주시면 같이 고민해볼 수 있을 것 같아요😀 3단계도 화이팅입니다💪💪💪 |
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.
고생 많으셨습니다🙇♀️
피드백 끝까지 반영해주셔서 감사합니다 진택님👍
진택님, 요거 충돌만 확인 부탁드릴게요🥲 충돌 해결 후에 바로 머지하겠습니다🙇♀️ |
매번 정성스런 피드백 감사합니다 !!! |
아래 설정으로 login 페이지로 시큐리티 의존성 제거를 통해서 login 페이지가 뜨는 것을 막을 수 있을 것 같아요😀 사용하지 않는다면 @SpringBootApplication(exclude = SecurityAutoConfiguration.class) |
WishlistService 등에서 토큰으로 검증하고 에러 처리하는 부분에서 부족하거나 빼먹은 부분은 없는지 궁금합니다! 이번주 과제도 열심히 하겠습니다 답변 정말 감사합니다!!! |
public class TokenService { | ||
|
||
private final TokenRepository tokenRepository; | ||
private final String secretKey = "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.
해당 값을 yml 에서 관리해도 좋을 것 같습니다😀 보안상의 이유와 환경마다 달라질 수 있기 때문입니다!
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.
네 감사합니다!
.claim("email", member.getEmail()) | ||
.signWith(getSecretKey()) | ||
.compact(); | ||
return tokenRepository.save(accessToken, member.getEmail()); |
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.
토큰을 저장하는 이유가 있을까요~? 단순히 어떤 의도를 가지고 구현하셨는지에 대한 궁금증으로 질뮨드립니다😎
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.
LoginFilter에서 토큰을 얻을 때 해당 토큰이 이미 저장되어있는지(유효한지)를 검사하기 위해 토큰을 저장하였습니다!
진택님, 답이 늦어 죄송합니다🥲 잘 구현해주신 것 같아요! 현재는 컨트롤러으 메서드마다 인증 헤더값을 가지고 있지만, 메서드의 수가 많아 질 수록 토큰을 복호화하는 로직이 늘어날 것 같아요. 요청이 들어와서, 요청의 토큰을 검증하는 클래스를 생각해볼 수 있을 것 같아요. Interceptor, ArgumentResolver 등등 다양하게 구현해볼 수 있을 것 같습니다! |
반영해보겠습니다 감사합니다!! |
토큰을 사용하여 인증하는 과정이 아직 미숙한 것 같습니다..
코드를 자율적으로 수정할 예정이긴 하나, 전반적으로 피드백 해주시면 적극 반영해보겠습니다!