-
Notifications
You must be signed in to change notification settings - Fork 0
JWT 인증 및 인가 기능 구현 #12
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
Conversation
m2ri1
left a comment
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.
추가적으로, 일반적인 Java의 패키지 네이밍 컨벤션은
소문자로 맞추어주는 것이 일반적입니다!
/auth/presentation/data 경로의 Response, Request등의 패키지를 소문자로 맞추어 작성해주시면 좋을 것 같아요
| UserDetails userDetails = customUserDetailsService.loadUserByUsername(email); | ||
| UsernamePasswordAuthenticationToken authentication = | ||
| new UsernamePasswordAuthenticationToken(userDetails, null, userDetails.getAuthorities()); | ||
| SecurityContextHolder.getContext().setAuthentication(authentication); |
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.
customUserDetailsService.loadUserByUsername(email)에서 null이 반환되어 NPE가 발생되는 경우를 방지해주시면 더욱 안전한 코드가 될 것 같습니다!
아래와 같은 방식으로 userDetails의 null 여부 검증후 이어서 작업을 수행하도록 하고,
null일 경우의 예외처리까지 해주시면 좋을 것 같아요
if (userDetails != null) {
UsernamePasswordAuthenticationToken authentication =
new UsernamePasswordAuthenticationToken(userDetails, null, userDetails.getAuthorities());
SecurityContextHolder.getContext().setAuthentication(authentication);
}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 LocalDateTime accessTokenExpiredAt; | ||
| private final LocalDateTime refreshTokenExpiredAt; |
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.
다른 DTO와 일관성을 위해서 accessTokenExpiresAt, refreshTokenExpiredAt등의 이름으로 맞춰주시는게 좋아보이는데, 혹시 다르게 네이밍하신 이유가 있을까요?
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.
오타 확인 후 피드백 반영했습니다!
리뷰 감사합니다.
| public String createRefreshToken(String email) { | ||
| String refreshToken = generateToken(email, refreshTokenExpiration); | ||
| redisTemplate.opsForHash().delete("refresh_token", refreshToken); | ||
| redisTemplate.opsForHash().put("refresh_token", email, refreshToken); | ||
| return refreshToken; | ||
| } |
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.
refreshToken을 저장할 때 email을 키로 사용하고 있으니,
삭제할 때도 email을 키로 사용해야 정상적으로 삭제가 될 것 같습니다
public String createRefreshToken(String email) {
String refreshToken = generateToken(email, refreshTokenExpiration);
redisTemplate.opsForHash().delete("refresh_token", email);
redisTemplate.opsForHash().put("refresh_token", email, refreshToken);
return refreshToken;
}이런식으로요!
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.
피드백 반영했습니다.
리뷰 감사합니다!
snowykte0426
left a comment
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.
우선 수고하셨습니다
확인해주세요😁
| public interface AuthPort { | ||
|
|
||
| Optional<MemberJpaEntity> findByEmail(String email); | ||
|
|
||
| void save(MemberJpaEntity member); | ||
| } |
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.
Member 리소스를 찾는 행위는 Member Pesistence Port 같은 곳을 보면 이미 구현되어 있거나 관련 기능이 모여 있으니 이미 존재하는 것을 사용해도 될 것 같습니다
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.
findMebersByCriteria 사용하여 피드백 반영하였습니다.
리뷰 감사합니다!
| if (email == null || password == null || email.isEmpty() || password.isEmpty() || email.length() > EMAIL_MAX_LENGTH || password.length() > PASSWORD_MAX_LENGTH) { | ||
| throw new EmailOrPasswordEmptyException(); | ||
| } |
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.
Request DTO에 값 검증 관련 어노테이션으로 충분히 이미 검증이 가능한 로직인 것 같습니다
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.
피드백 반영하였습니다!
리뷰 감사합니다.
snowykte0426
left a comment
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.
수고하셨습니다😎
m2ri1
left a comment
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.
고생하셨습니다!
💡 PR 요약
POST/auth/signup엔드포인트를 통해 회원가입 기능을 구현하였습니다.POST/auth/signin엔드포인트를 통해 로그인 기능을 구현하였습니다.PATCH/auth/refresh엔드포인트를 통해 JWT 리프레시 토큰 갱신 기능을 구현하였습니다.Resolves: [Feat]
Auth관련 기능 구현 #11📋 작업 내용
🤝 리뷰 시 참고사항
✅ 체크리스트
README,.env.example)