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
[TNT-28] feat: Authentication Filter 구현 #9
[TNT-28] feat: Authentication Filter 구현 #9
Changes from 78 commits
2765da8
bbd6f6d
397688b
348aa14
4881e56
57e3db9
19cbf55
7df0f63
4a22cd0
287a966
a98cdbc
a5090c1
3ec4f7a
ddcebf1
88df790
436f484
f60cdda
2dd0d6a
8523fdb
2ac8730
7e22fab
fff07b4
38ee34f
f71c593
f1668bb
de025d6
b3f7b7a
1ac7bde
0c56e0e
8144fe5
446702c
4356b0b
aace3c2
f0ac237
0786625
11e8c66
531186d
e16edc5
7266c03
d83e7f8
aa9bca4
4bba1c1
c4b227c
1a170ed
be6a353
b517e65
7a7dd5d
91c6d12
a8a26e2
71ce996
f80c5bd
6b2b7a0
5a33c7e
6e15931
a9efc0b
ee987e0
86e8605
b8f65dc
08dbcf8
9c176e4
36e3566
8495903
733b374
281858d
c88469f
f35949e
3335217
6f330ff
11aed26
ccf892e
94ee88b
9bf6614
b759144
1028f01
0e248b4
39dd95b
b7c75a7
2590e9b
33fb5aa
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.
아까 초기화가 안되는 것들은 위에 두자고 했지만 그거는 로직상 어쩔 수 없을때만 하고 웬만해서는 중간에
String sessionId = authHeader.substring(SESSION_ID_PREFIX.length());
가 더 좋을 것 같긴 합니다 ㅎㅎ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.
id(pk)나 int 등 primitive는 제외하고 객체들은
Objects.requireNonNull
(static import)로null
체크는 어떨까요? + 값 validation 등..이건 상의해서 정해보죵
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.
아하 좋습니다 ! null 체크나 유효성 검사 컨벤션은 조만간 상의해봐요 !!
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.
log들은 로직에 섞여 보이지 않게 개행 처리로 구분해주는건 어떨까요!? 다른쪽들도 다 마찬가지 입니다!
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.
여기에
sessionService.createSession()
도 있어야하지 않나요!?그리고 saveAuthentication(Long.parseLong(sessionId))의 매개변수는 memberId인데 여기는 sessionId입니다!
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.
createSession은 추후 개발할 회원 로그인 로직 쪽에서 사용하려고 했습니다 !!
로그인 요청(로그인 api 엔드포인트는 허용 uri) -> 회원 세션 생성
이런 플로우로 세션 생성과 인증을 구성하려고 했습니다 !
sessionId가 곧 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.
좋습니다~
이건 특히 중요한 부분이니 혹시라도 자동으로 ContextHolder가 비워지지 못하는 일이 없는지 고민은 같이 해봐야겠습니다!
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.
비동기 상황이 보통 그럴까요??
시큐리티 컨텍스트에 인증 정보가 남아있는 상황에 대해서 좀 더 찾아보고 오겠습니다 !!