-
Notifications
You must be signed in to change notification settings - Fork 15
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
[김현묵] sprint9 #28
base: express-김현묵
Are you sure you want to change the base?
[김현묵] sprint9 #28
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.
이전에 멘토링때 언급했던 대로 레이어를 잘 나눠주신것 같아서 좋습니다!
코드 내부적으로 봐도 에러처리도 잘 되어있고, 각 함수의 역할이 명확하게 분리되어 있기 때문에 리뷰할 내용이 많이 없을 정도에요.
백엔드측 코드를 작성해주실대 함수형태로 작성해주고 계신데, 백엔드에서 클래스를 사용하는 방식도 익혀두시면 좋습니다. 함수형/OOP 두가지는 서로 대척점에 있는것 같지만 폴리패러다임 관점에서는 두개의 방식이 모두 사용될 수 있고, 트렌드 자체도 각각의 장점만을 합쳐서 사용하는 형태로 진화하고 있습니다. 때문에 OOP의 개념중 하나인 클래스를 사용하는것도 고민해보시면 좋을 것 같습니다.
|
||
router.get("/logout", auth.verifyAccessToken, async (req, res) => { | ||
try { | ||
res.status(201).send({ accecssToken: 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.
로그아웃은 고민을 해볼 필요가 있을 것 같아요. 유저의 세션을 날려야 하니 응답으로 accessToken만 null로 보낸다면, 서버측에서는 유저의 세션이 그대로 남아있을 수 있습니다.
로그아웃 시에 서버가 가지고있는 유저의 refreshToken을 삭제하고, accessToken에 대하여 만료기간까지 블랙리스트로 처리하는 등 서버측에서도 유저의 세션정보를 비활성화 하는 방법을 고민해야 합니다.
클라이언트가 이응답을 받을때 악의적으로 accessToken을 탈취하여 로그인정보를 유지할 수도 있음을 인지해야 합니다.
findById, | ||
filterUserData, | ||
}; | ||
export default userRepository; |
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.
유저의 DB(Schema)에 접근하는 코드만 잘 분리해주신것 같아요! 여기서 하나 고민해볼 것은 구성한 함수들을 객체화 하고, 그것을 모듈로 보내주고 있는데, 함수로 구성하는방법과 클래스로 구성하는 방법이 있을 것 같아요. 두개의 방식을 모두 구현해보고 어떤 방식이 유리한지 고민해볼 수 있을 것 같습니다.
function createToken(user, type) { | ||
if (user) { | ||
const payload = { userId: user.email }; | ||
const option = { expiresIn: type === "refresh" ? "2w" : "1h" }; |
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.
(사소) 상수를 사용할때는 변수화하여 작성해주는것이 좋습니다. 2w, 1h와 같은 string값은 코드의 작성자는 이해할 수 있으나 다른사람이 보면 어떤 의미인지, 또 경우에 따라서 해석이 갈리는 경우가 존재할 수 있습니다.
요구사항
backend 분리해서 커밋이 하나 입니다. 전부 새로운 파일로 인식하고 있습니다.
기본