-
Notifications
You must be signed in to change notification settings - Fork 1
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(#16) jwt 토큰 프로바이더 구현 #17
The head ref may contain hidden characters: "feature/#16-feat-\uD1A0\uD070-\uD504\uB85C\uBC14\uC774\uB354-\uAD6C\uD604"
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.
수고하셨습니다!! 진짜 빨리 구현하셨네요 🫢
+) 저는 jwt 패키지 좋은 것 같습니다! 공통으로 쓰는 거니까 common 밑에 있어도 될 것 같구요!
jaknaeso-core/src/main/java/org/nexters/jaknaesocore/common/support/error/ErrorCode.java
Outdated
Show resolved
Hide resolved
jaknaeso-core/src/main/java/org/nexters/jaknaesocore/common/support/error/ErrorType.java
Outdated
Show resolved
Hide resolved
jaknaeso-server/src/main/java/org/nexters/jaknaesoserver/jwt/JwtAuthFilter.java
Outdated
Show resolved
Hide resolved
jaknaeso-server/src/main/java/org/nexters/jaknaesoserver/jwt/JwtAuthFilter.java
Outdated
Show resolved
Hide resolved
jaknaeso-server/src/main/java/org/nexters/jaknaesoserver/jwt/JwtGenerator.java
Outdated
Show resolved
Hide resolved
jaknaeso-server/src/test/java/org/nexters/jaknaesoserver/jwt/JwtParserTest.java
Outdated
Show resolved
Hide resolved
jaknaeso-server/src/test/java/org/nexters/jaknaesoserver/jwt/JwtParserTest.java
Outdated
Show resolved
Hide resolved
jaknaeso-server/src/main/java/org/nexters/jaknaesoserver/jwt/JwtGenerator.java
Outdated
Show resolved
Hide resolved
- 관용적인 표현인 Provider 사용 - subject값을 알기 쉽게 userId로 변경
jaknaeso-server/src/main/java/org/nexters/jaknaesoserver/jwt/JwtAuthFilter.java
Outdated
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.
- 혹시 jwt라는 디렉토리를 최상위에 둔 이유가 무엇인가요? 궁금합니다.
- jwt가 common과 config만큼 일반적인 개념은 아니라고 생각해서,
domain
이라는 디렉토리에 들어가야 하는 것이 아닌가? 라고 생각했어요. 예를 들어,domain/auth
라는 디렉토리 안에filter
,service
,handler
등으로 개념을 나눠서 거기에 넣어두는 방식을 구상했었어요! (제 방식대로 하자는 것은 아니고, 그냥 생각만 전달한거니 참고만 해주세요.)
jaknaeso-server/src/main/java/org/nexters/jaknaesoserver/config/SecurityConfig.java
Show resolved
Hide resolved
jaknaeso-server/src/main/java/org/nexters/jaknaesoserver/jwt/SecurityExceptionHandler.java
Outdated
Show resolved
Hide resolved
jaknaeso-server/src/main/java/org/nexters/jaknaesoserver/jwt/JwtAuthFilter.java
Outdated
Show resolved
Hide resolved
jaknaeso-server/src/main/java/org/nexters/jaknaesoserver/config/SecurityConfig.java
Show resolved
Hide resolved
jaknaeso-server/src/main/java/org/nexters/jaknaesoserver/jwt/JwtAuthFilter.java
Outdated
Show resolved
Hide resolved
jaknaeso-server/src/main/java/org/nexters/jaknaesoserver/jwt/JwtAuthFilter.java
Outdated
Show resolved
Hide resolved
- 시큐리티 설정에 필터 추가 - 권한 관련 예외 발생 시 CustomException으로 전파 될 수 있게 설정 - 토큰 추출시 예외 ControllerAdvice에서 처리 할 수 있도록 추가
jaknaeso-server/src/main/java/org/nexters/jaknaesoserver/security/JwtAuthFilter.java
Outdated
Show resolved
Hide resolved
jaknaeso-core/src/main/java/org/nexters/jaknaesocore/config/PropertiesConfig.java
Outdated
Show resolved
Hide resolved
jaknaeso-server/src/main/java/org/nexters/jaknaesoserver/controller/TokenController.java
Outdated
Show resolved
Hide resolved
jaknaeso-server/src/main/java/org/nexters/jaknaesoserver/controller/TokenController.java
Outdated
Show resolved
Hide resolved
jaknaeso-server/src/main/java/org/nexters/jaknaesoserver/config/SecurityConfig.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.
고생하셨어요! 일단 Approve 하겠습니다.
TokenProvider의 위치에 관한 것은 @kimyu0218 님이랑 합의하는 것이 좋을 것 같습니다!
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.
수고하셨습니다!!
jwtparser랑 jwtprovider는domain/auth/service/support
에 있어도 좋을 것 같습니다- token 컨트롤러를 따로 둔 이유가 있으신가요?
+) 슬랙으로 남기신 jwt provider 관련 프로젝트 내용 확인했습니다. 인증/인가는 api 쪽이 더 적절할 것 같아요! 추후에 도입할 batch에서는 사용하지 않을 것 같습니다
@@ -3,5 +3,8 @@ | |||
public enum ErrorCode { | |||
E500, | |||
E400, | |||
E401, | |||
E4011, |
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.
토큰 기간 만료되었을 때만 4011 보내주도록 하였습니다 ! 뭔가 좀 더 구체화 하면 좋을 것 같긴한데 예를 들어 형식 안맞을 때와 토큰이 없을 때 등에 따라서 예외코드를 만들면 좋을 것 같긴합니다
jaknaeso-server/src/main/java/org/nexters/jaknaesoserver/security/SecurityExceptionHandler.java
Outdated
Show resolved
Hide resolved
|
||
@DisplayName("액세스 토큰 만료시간은 0보다 커야 한다.") | ||
@Test | ||
void 액세스_토큰_만료시간은_0보다_커야_한다() { |
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.
저번에 함수명 영여 + display name 한글
하셨다고 하셔서 이렇게 작성중인데 함수명을 한글로 작성하셨네요?
- 함수명이 한글인데 display name을 붙여주신 이유가 있으신가요?
- 해당 컨벤션에 대해 제대로 논의한 것 같진 않아서 이번에 얘기해보면 좋을 것 같아요!
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.
당시에 DisplayName 적어두고 영어로 뭐로 해야 할 지 도저히 생각이 안나서 한글로 작성했습니다.... 아마도 늦게 작업하다보니 그랬던 것 같습니다. 😭
그냥 DisplayName 없애고 한글로 작성해도 괜찮은것 같긴하더라구요
작업 개요
oauth2에서 사용할 jwt 토큰 기능 구현 및 시큐리티 필터 구현
작업 사항
고민한 점들
작업하면서 아주 기본적인 부분들만 구현해 두었습니다. claims에 필요한 부분이 존재하면 추가로 넣으면 될 것 같고, 리프레시 토큰을 사용한다면 어디에 저장할 지 정해지지 않아서 틀만 잡아두었습니다. 그리고 Role 또한 지금 당장에 구분이 필요하지 않다고 생각해서 비어두었습니다. 필요한 부분에 맞춰서 추가하면서 변경하면 될 것 같습니다.
토큰 시간 또한 정해진 건 없어서 대충 넣어둬서 정책 세워서 변경하면 될 것 같습니다 !
@kimyu0218 기본적인 부분들만 구현해두어서 만약에 추가로 필요한 부분 존재하시면 리뷰 달아주시면 추가로 구현해두도록 하겠습니다.
@pythonstrup 모듈을 분리된 기준이 있었던 것 같은데 해당 모듈에 구현하는게 맞았는지 확인해주시면 감사하겠습니다.
그리고 패키지를 그냥 jwt로 만들고 때려박았는데.. 네이밍이나 패키지도 변경 필요한 부분 있으면 리뷰 달아주시면 반영하겠습니다 !
이외에 정책적으로 논의되어서 코드에 반영되어야 되는 것이나 빼먹은 내용 존재한다면 리뷰달아주시면 반영하겠습니다!