-
Notifications
You must be signed in to change notification settings - Fork 0
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: JWT 기반 인증 및 Spring Security 설정 구현 #23 #26
Conversation
- SecurityConfig 작성 - JwtAuthenticationFilter 작성 - JwtUtil 보충 - 작업자 이해를 돕기 위한 주석 포함
- SecurityConfig 작성 - JwtAuthenticationFilter 작성 - JwtUtil 보충 - 작업자 이해를 돕기 위한 주석 포함
- UserDetails에서 객체를 받아오게 JwtUtil 변경 - jjwt 최신 변경 방식에 따라 코드 수정
- UsernameNotFoundException 처리 핸들러 추가 - StatudsCode에 상태코드 추가 - CustomUserDetailsService 구현 진행 중
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.
로그인 쉽지 않으셨을텐데 고생하셨습니다!👍👍 저도 좀 더 알아볼 수 있으면 알아보고 리뷰 달아볼게요ㅎㅎ
import team05.integrated_feed_backend.module.member.entity.Member; | ||
import team05.integrated_feed_backend.module.member.repository.MemberRepository; | ||
|
||
@Service |
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.
🙋♀️ 저번에 저희 클래스 영역에서 @Transactional(readOnly = true)
해주기로 했던 것 같은데, 보니까 로그인은 post만 있는 것 같아서 딱히 필요없는 것 같기도 하고.. 다른 분들 의견 궁금합니다!
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.
어노테이션들 추가했습니다 b69b667
import team05.integrated_feed_backend.module.member.entity.Member; | ||
|
||
public interface MemberRepository extends JpaRepository<Member, Long> { | ||
Optional<Member> findByAccount(String account); |
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.
💭 Optional
사용하셔서 null 체크가 더 효율적인 것 같네요!
src/main/java/team05/integrated_feed_backend/module/auth/jwt/JwtAuthenticationFilter.java
Show resolved
Hide resolved
- @transactional(readOnly = true), @requiredargsconstructor, @slf4j 적용
src/main/java/team05/integrated_feed_backend/module/auth/security/CustomUserDetails.java
Show resolved
Hide resolved
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.
로그인 부분이 많이 까다로운데, 로깅까지 꼼꼼하게 해주셔서 감사합니다
늦게 리뷰한 주제에 리뷰 양이 많아서 죄송해요 허허😂 단순한 의견이니 모두 반영하실 필요없고, 넘어가 주셔도 괜찮습니당!!
src/main/java/team05/integrated_feed_backend/core/config/SecurityConfig.java
Outdated
Show resolved
Hide resolved
|
||
@Bean | ||
public PasswordEncoder passwordEncoder() { | ||
return new BCryptPasswordEncoder(); |
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.
헉 요부분은 BCryptPasswordEncoder
말고 Argon2PasswordEncoder
를 쓸 것 같아요!
얘가 조금 더 보안이 좋다고 하더라구요! 근데 이건 제가 작업하면서 바꿔놓겠습니당😉
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 BaseApiResponse<Void> handleUsernameNotFoundException(UsernameNotFoundException e) { | ||
log.warn(e.getMessage(), e); | ||
|
||
return BaseApiResponse.of(StatusCode.USER_NOT_FOUND); |
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.
💭 요건 유저가 없는 거긴 한데, 토큰이 잘못된거니까 404에러 보다는 401 Unauthorized 에러는 어떠세용?
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.
그게 맞는 것 같아 반영하면서 import도 수정해두었습니당! 8d5f488
src/main/java/team05/integrated_feed_backend/module/auth/jwt/JwtAuthenticationFilter.java
Show resolved
Hide resolved
private final Key secretKey; | ||
|
||
//jjwt: String secretKey -> Key 객체 방식으로 대체됨 | ||
public JwtUtil(@Value("${jwt.secret}") String secret) { | ||
byte[] keyBytes = Base64.getDecoder().decode(secret); | ||
this.secretKey = new SecretKeySpec(keyBytes, SignatureAlgorithm.HS256.getJcaName()); | ||
} | ||
|
||
@Value("${jwt.token-validity-in-seconds}") | ||
private long tokenValidityInseconds; |
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 Key secretKey; | |
//jjwt: String secretKey -> Key 객체 방식으로 대체됨 | |
public JwtUtil(@Value("${jwt.secret}") String secret) { | |
byte[] keyBytes = Base64.getDecoder().decode(secret); | |
this.secretKey = new SecretKeySpec(keyBytes, SignatureAlgorithm.HS256.getJcaName()); | |
} | |
@Value("${jwt.token-validity-in-seconds}") | |
private long tokenValidityInseconds; | |
private final Key secretKey; | |
@Value("${jwt.token-validity-in-seconds}") | |
private long tokenValidityInseconds; | |
//jjwt: String secretKey -> Key 객체 방식으로 대체됨 | |
public JwtUtil(@Value("${jwt.secret}") String secret) { | |
byte[] keyBytes = Base64.getDecoder().decode(secret); | |
this.secretKey = new SecretKeySpec(keyBytes, SignatureAlgorithm.HS256.getJcaName()); | |
} |
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.
그리고 @Value
로 yml에 있는 환경변수를 받아 오고 계신데,
@ConfigurationProperties()
애너테이션을 사용해서 해당 설정들을 가진 객체(ex. JWTProperties
)로 만들어서 주입할수도 있어요! 요건 그냥 지식 공유였습니다😉
} | ||
|
||
// JWT 토큰 유효성 검증 | ||
public boolean validateToken(String token) { |
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.
💭 사소한 내용이긴한데, validateToken()
은 반환값이 없을 것 같은 네이밍이라, isValidToken(String token)
같은 네이밍은 어떠세욤?
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.
반영했습니다! b848cb5
return Long.parseLong(Jwts.parserBuilder() | ||
.setSigningKey(secretKey) | ||
.build() | ||
.parseClaimsJws(token) | ||
.getBody() |
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.
💭 요부분 밑에 getAccount()
랑 반복되는 로직이 있어서, 메서드 분리해서 재사용하는 것도 좋을 것 같아요!
return null; | ||
} | ||
|
||
public Authentication getAuthentication(String token, UserDetails userDetails) { |
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.
❗️호옥시 token
을 인자로 받는 이유가 있을까요?
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.
어디까지 token이 활용되는지 잘 몰라서 일단 받아오게 써둔 것 같아요...! 지켜보고 안쓰이면 빼겠습니당
claims.put("memberId", memberId); | ||
claims.put("account", account); |
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.
💭 요런 "memberId", "account" 같은 string은 private static final String 으로 상수화 해도 될 것 같아요!
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.
반영했습니다! 85b7ff6
@RequiredArgsConstructor | ||
public class JwtAuthenticationFilter extends OncePerRequestFilter { | ||
|
||
private final JwtUtil jwtUtil; |
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.
💭 요것도 정말 사소한데.. 인스턴스화 해서 사용하면 Util이라는 네이밍보단 JwtManager, JwtHandler 같은 네이밍은 어떠세욤?? 그리고 JwtUtil
클래스를 보면 HttpServletRequest
, UserDetails
등 웹 계층이랑 의존성이 있어서 순수 유틸클래스는 아닌 것 같아서요! 근데 개인적인 의견이고 안바꾸셔도 됩니다!!
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.
JwtManager 좋은 것 같아요! 반영했습니다 c91c9ab
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 class Member extends BaseEntity { | ||
@Id | ||
@GeneratedValue(strategy = GenerationType.IDENTITY) | ||
private Long 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.
💭: 아주 사소한 의견인데요! 저희 저번에 강의에서 보안이 중요한 데이터는 UUID로 id를 정의하면 좋다고 했어서 혹시 나중에 시간 괜찮으시다면 한번 생각해봐주시면 감사할 것 같습니다:)
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.
넵 생각해보겠습니다!!
# Conflicts: # build.gradle # src/main/java/team05/integrated_feed_backend/module/post/entity/Hashtag.java # src/main/java/team05/integrated_feed_backend/module/post/entity/Post.java # src/main/resources/application-template.yml
- swagger ui가 빈 페이지 반환하는 문제 확인, ui 관련 요청을 필터에서 제외 - `SecurityConfig`에서 Swagger ui관련 접근 허용 설정 - `JwtAuthenticationFilter`에서 `filterChain.doFilter` 조건문 안에 있어 필터로 전달 안되는 문제 확인 후 수정
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.
저는 공부가 필요한 부분인데, 도움이 많이 되었습니다! 고생 많으셨습니다! 👍
…THORIZED로 변경 (#23) - USER_NOT_FOUND에서 UNAUTHORIZED 에러로 수정, 인증 과정 예외임을 명확히함
📊 Jacoco Test Coverage
|
🎟️ 관련 이슈
Fixes #23
👩💻 구현 내용
JwtUtil
생성JwtAuthenticationFilter
구현SecurityConfig
설정 파일 작성CustomUserDetailsService
및CustomUserDetails
구현💬 코멘트
💭 고려한 점