JWT를 이용한 로그인시 인증 #6
Conversation
JaeMoonNoh
commented
Dec 27, 2024
JaeMoonNoh
commented
- JWT를 사용해 인증을 했습니다.
- AccessToken, RefreshToken을 사용했습니다.
Auth/authJWT.js
Outdated
| @@ -0,0 +1,66 @@ | |||
| const { sign, decodedAccessToken, getAccessTokenPayload, refreshTokenComapre } = require('../JWT/jwt'); | |||
|
|
|||
| module.exports.authJWT = async (req, res, next) => { | |||
There was a problem hiding this comment.
authJWT 이름으로는 어떤 역할을 하는지 유추하기 어려워 보입니다.
naming 을 개선하면 좋겠습니다.
There was a problem hiding this comment.
네 코드 분할해서 개선해보겠습니다!
Auth/authJWT.js
Outdated
| return next(); | ||
| } | ||
|
|
||
| if(accessTokenPayload.message === 'jwt expired'){ |
There was a problem hiding this comment.
string 을 비교 해서 판단하는 것은 약간의 변경에도 코드가 취약합니다. symbol 을 사용해서 제어하는 편이 좋겠습니다.
There was a problem hiding this comment.
Symbol 사용해 제어해보겠습니다!
Auth/authJWT.js
Outdated
| } | ||
|
|
||
| return unauthorizedResponse(res, 'invalid Access Token 입니다.'); | ||
| } catch(err) { |
There was a problem hiding this comment.
err 발생시 반드시 권한이 존재 하지 않는다고 단정짓기 어려울 것 같은데요. 어떻게 생각하실까요?
There was a problem hiding this comment.
네 맞습니다. Reference error등 다양한 오류가 발생할 수 있는데 그런 점을 생각 못했던것 같습니다! 수정하겠습니다!
Auth/authJWT.js
Outdated
| } | ||
|
|
||
| const splitBearerAccessToken = (accessToken) => { | ||
| return accessToken.split('Bearer ')[1]; |
There was a problem hiding this comment.
이 부분은 중복 코드 입니다. 어디가 중복인지 찾아보시고, 중복을 제거해 보세요.
There was a problem hiding this comment.
제 생각엔 splitBearer이라는 단어가 중복된다고 생각되어 extractAccessToken이라는 함수명으로 변경했습니다!
JWT/jwt.js
Outdated
|
|
||
| module.exports.refreshTokenComapre = async (represhTokenInCookie, userId) => { | ||
| try{ | ||
| const refreshTokenInRedis = await redisClient.get(userId.toString()); |
There was a problem hiding this comment.
[난이도 상] redisClient 와 jwt 간 강하게 결합되어 있어 보입니다. 이 부분을 추상화해서 결합도를 낮춰보실 수 있겠어요?
There was a problem hiding this comment.
Interface를 사용해서 redis말고 다른 cache db가 들어왔을때 바꿔 넣을 수 있도록 변경을 했습니다.
JWT/jwt.js
Outdated
| return tokenPayload(token); | ||
| } | ||
|
|
||
| module.exports.refreshTokenComapre = async (represhTokenInCookie, userId) => { |
There was a problem hiding this comment.
[참고] function 은 가능하면 return 형태가 동일해야 code 안정성에 좋습니다.
요 PR 이후에 코드를 작성하실때에는 interface 를 가능한 맞춰주세요.
JWT/jwt.js
Outdated
| } | ||
|
|
||
| // payload를 어떻게 관리할까? | ||
| const createPayload = (UserInformation) => { |
User/repository/userRepository.js
Outdated
| module.exports.loginVerification = async (username) => { | ||
| const connection = await getConnection(); | ||
| try{ | ||
| const [result] = await connection.query("select * from User where username = ?", username); |
There was a problem hiding this comment.
이거 SQL injection 공격에 취약하지 않나요?
예를 들면 username 을 "hahaha OR 1 = 1" 이렇게 넣었을때 결과가 이상하게 되지 않을까 싶어서요.
There was a problem hiding this comment.
네 username을 받을때 정확한 형식이 들어오지 않으면 오류발생하게끔 바꿔놓겠습니다!
| } catch(err) { | ||
| throw err; | ||
| } |
There was a problem hiding this comment.
지난번 PR 에도 말씀 드렸던 것 같은데요, 불필요한 catch 인 것 같습니다.
There was a problem hiding this comment.
네 까먹고 처리를 못한것 같습니다. 코드 변경하겠습니다!
| } catch(err) { | ||
| throw err; | ||
| } |
There was a problem hiding this comment.
네 비슷한 코드들도 찾아서 고쳐보겠습니다!
|
| } | ||
|
|
||
| const newAccessToken = this.sign(payload); | ||
| res.setHeader('Authorization', `Bearer ${newAccessToken}`); |
There was a problem hiding this comment.
지난 번에 말씀 드린 #6 (comment) 는 아직 해결 되지 않았어요.
authJWT 의 63라인과 userController 의 18번 라인은 중복된 코드입니다. 개선해 보면 좋겠어요.
