Skip to content
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

BTVN8: Add Authorization and API Register/Login with JWT #45

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

dngiang2003
Copy link
Collaborator

em nộp bài tập về nhà ạ

@dngiang2003 dngiang2003 requested review from ChristianGreyy and removed request for dinhhonglieu May 18, 2023 13:29
const userRouter = express.Router();

userRouter.route("/").get(getUsers).post(createUser);
userRouter.route("/").get(authMiddleware, getUsers).post(createUser);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • createUser cũng nên đặt authMiddleware nhé em

const login = async (req, res, next) => {
try {
const { studentCode, password } = req.body;
const user = await User.findOne({ studentCode });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Em nên check user có tồn tại hay không ở đây luôn nhé, nếu không thì throw luôn ra error nhé

try {
const { studentCode, password } = req.body;
const user = await User.findOne({ studentCode });
const isPassword = await bcrypt.compare(password, user.password);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Nếu thằng user mà không tồn tại thì không càn phải thực hiện bước này nữa

err.status = 401;
throw err;
}
const token = jwt.sign({ userId: user._id }, process.env.SECRET_KEY, { expiresIn: "1h" });

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sử dụng env cho thời gian sống của token em nhé

Suggested change
const token = jwt.sign({ userId: user._id }, process.env.SECRET_KEY, { expiresIn: "1h" });
const token = jwt.sign({ userId: user._id }, process.env.SECRET_KEY, { expiresIn: process.env.JWT_EXPIRES_IN });

Comment on lines 21 to 24
res.status(200).json({
message: "Login successfully!",
token,
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
res.status(200).json({
message: "Login successfully!",
token,
});
res.status(200).json({
token,
});

Comment on lines 38 to 43
const checkUser = await User.findOne({ studentCode });
if (checkUser) {
const err = new Error("Student code is exit!");
err.status = 400;
throw err;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tên biến để danh từ em nhé, động từ để cho tên hàm

Suggested change
const checkUser = await User.findOne({ studentCode });
if (checkUser) {
const err = new Error("Student code is exit!");
err.status = 400;
throw err;
}
const existingUser = await User.findOne({ studentCode });
if (existingUser) {
const err = new Error("Student code is exit!");
err.status = 400;
throw err;
}

Comment on lines 5 to 21
const authorization = req.headers.authorization;
if (!authorization) {
const err = new Error("Unauthorized!");
err.status = 401;
throw err;
}
const token = authorization.split(" ")[1];
const payload = jwt.verify(token, process.env.SECRET_KEY);
const userId = payload.userId;
const user = await User.findById(userId);
if (!user || user.role !== "admin") {
const err = new Error("Unauthorized");
err.status = 401;
throw err;
}
req.user = user;
next();
Copy link

@trankhacbinhduong trankhacbinhduong May 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hàm này em nên tách nhỏ ra cho dễ đọc chứ để cả đống logic không cách hàng gì cả. Em có thể cấu trúc lại theo sườn tham khảo như dưới nhé, chỗ trả về lỗi kia chỉ là ví dụ nhé, không phải lỗi nào cũng unauthorized, tìm hiểu thêm các lỗi khác nhé

const extractTokenFromHeader = (request) => {
    const [type, token] = request.headers.authorization?.split(' ') ?? [];
    return type === 'Bearer' ? token : undefined;
}
  
const unauthorized = () => {
  const err = new Error("Unauthorized!");
  err.status = 401;
  throw err;
};

const authMiddleware = (req,res,next) => {
      const token =  extractTokenFromHeader(req)
      if(!token) return  unauthorized()
      
      // verify token
      
      // �check user
      
      // check permission
      if(!admin) return unauthorized()
      
      next()
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vâng ạ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants