Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: JWT 기반 인증 및 Spring Security 설정 구현 #23 #26
Changes from 16 commits
9b65ad6
eb5b981
eb23ac5
be5ce80
938d8e8
d0d5795
bc5db59
20ca514
ea88983
35d94a7
7b19943
4a12c87
1d4990a
666e435
74fa689
b69b667
df88534
ee79c9c
8d5f488
b848cb5
85b7ff6
c91c9ab
a0de780
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
그렇군요!! 저도 더 알아보겠습니당
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
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.
🙋🏻♀️ 필드는 필드끼리 모여 있으면 좋을 것 같습니당!
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
)로 만들어서 주입할수도 있어요! 요건 그냥 지식 공유였습니다😉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
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이 활용되는지 잘 몰라서 일단 받아오게 써둔 것 같아요...! 지켜보고 안쓰이면 빼겠습니당
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()
랑 반복되는 로직이 있어서, 메서드 분리해서 재사용하는 것도 좋을 것 같아요!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
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