Conversation
…, activation, login, logout, password reset, and profile management
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving this submission as an educational override to help you progress, but there are several critical issues to address. You've built a great foundation with a well-organized project structure! However, your authentication middleware in src/middlewares/authMiddleware.js is incomplete—after verifying the token, you must fetch the user from the database and attach them to req.user. The logout functionality is also insecure; the route in src/routes/auth.route.js needs the authMiddleware and should use the POST method, not GET. Additionally, the User model is missing the resetToken and resetTokenExpire fields, which breaks the password reset flow. To fix the failed check run, simply rename .eslintrc.js to .eslintrc.cjs. Addressing these points will make your application much more robust and secure!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| user.activationToken = null; | ||
| await user.save(); | ||
|
|
||
| return res.redirect(`${process.env.CLIENT_HOST}/profile`); |
There was a problem hiding this comment.
According to the requirements, you correctly redirect to the profile page after activation. However, the user is not authenticated at this point. This means the client-side application will likely redirect them to the login page immediately, which isn't a great user experience. Consider automatically logging the user in by generating and providing authentication tokens after a successful activation.
| const logout = async (req, res) => { | ||
| const { refreshToken } = req.cookies; | ||
| const user = await jwtService.verifyRefresh(refreshToken); | ||
|
|
||
| if (!user || !refreshToken) { | ||
| throw ApiError.unauthorized(); | ||
| } | ||
|
|
||
| await tokenService.remove(user.id); | ||
|
|
||
| res.clearCookie('refreshToken'); | ||
|
|
||
| return res.sendStatus(204); | ||
| }; |
There was a problem hiding this comment.
The task requires the logout functionality to be available only to authenticated users. While you check for a refresh token, this endpoint should be protected by the authMiddleware to ensure a valid access token is present, which is the standard way to protect routes. Additionally, logout is an action that changes the server's state (by invalidating a token), so it should be handled by a POST or DELETE request, not a GET request.
| const resetPassword = async (req, res) => { | ||
| const { token } = req.params; | ||
| const { password, confirm } = req.body; | ||
|
|
||
| if (password !== confirm) { | ||
| throw ApiError.badRequest('Passwords do not match'); | ||
| } | ||
|
|
||
| const user = await User.findOne({ where: { resetToken: token } }); | ||
|
|
||
| if (!user || Date.now() > user.resetTokenExpire) { | ||
| throw ApiError.badRequest('Invalid or expired token'); | ||
| } | ||
|
|
||
| user.password = await bcrypt.hash(password, 10); | ||
| user.resetToken = null; | ||
| user.resetTokenExpire = null; | ||
| await user.save(); | ||
|
|
||
| res.json({ message: 'Password updated' }); | ||
| }; |
There was a problem hiding this comment.
The new password provided during a password reset is not being validated against the rules defined in your validatePassword function. This could allow users to set a weak password, which contradicts the rules enforced during registration. You should add a validation step for the new password here.
| const changeEmail = async (req, res) => { | ||
| const { password, newEmail } = req.body; | ||
|
|
||
| if (!password || !newEmail) { | ||
| throw ApiError.badRequest('All fields are required'); | ||
| } |
There was a problem hiding this comment.
According to the requirements, the user must confirm their new email. Your current implementation validates the user's password and the presence of a new email, but it's missing a check to confirm the new email (e.g., by comparing newEmail with a confirmNewEmail field from the request body).
|
|
||
| app.use(authRouter); | ||
| app.use('/users', userRouter); | ||
| app.use(profileRouter); |
There was a problem hiding this comment.
While this works, it's a good practice to group related routes under a common prefix. Mounting the profileRouter at the root (/) means your profile endpoints are available at paths like /, /name, and /password. Consider mounting it with a prefix, like app.use('/profile', profileRouter);, to make your API structure more organized and avoid potential conflicts.
| if (err) { | ||
| return res.status(500).json({ error: 'Server error' }); | ||
| } |
There was a problem hiding this comment.
This catch-all block incorrectly reports all non-ApiError types with a generic 500 status. This means your 404 "Not Found" error, which has a status of 404, will be incorrectly reported as a 500 server error. You should check if the err object has its own status property and use that instead of hardcoding 500.
| return res.status(500).json({ error: 'Server error' }); | ||
| } | ||
|
|
||
| next(); |
There was a problem hiding this comment.
This next() call is unreachable. Error-handling middleware is only invoked when an error exists. If err is truthy, one of the return statements above will execute, preventing this line from being reached. This line can be safely removed.
| router.get('/activate/:activationToken', catchError(authController.activate)); | ||
| router.post('/login', catchError(authController.login)); | ||
| router.get('/refresh', catchError(authController.refresh)); | ||
| router.get('/logout', catchError(authController.logout)); |
There was a problem hiding this comment.
According to the task requirements, the logout functionality should only be available to authenticated users. This route is missing the authMiddleware to protect it.
Additionally, logout is an action that modifies server state (invalidates a token), so it should use the POST or DELETE HTTP method, not GET.
| export const User = client.define('user', { | ||
| name: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, | ||
| }, | ||
| email: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, | ||
| unique: true, | ||
| }, | ||
| password: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, | ||
| }, | ||
| activationToken: { | ||
| type: DataTypes.STRING, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Your application's password reset functionality uses resetToken and resetTokenExpire fields. While Sequelize might save these dynamically, it's a best practice to explicitly define all model attributes in the schema. Please add resetToken (STRING) and resetTokenExpire (DATE or BIGINT) to this model to ensure data integrity and make the database schema clear.
| function normalize({ id, email }) { | ||
| return { id, email }; | ||
| } |
There was a problem hiding this comment.
The normalize function only returns the user's id and email. According to the requirements, the profile page should display the user's name. To support this, you should also include the name field in the object returned by this function.
2 participants
No description provided.