m#279
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This implementation has several critical blockers that prevent the application from functioning correctly. Broken route definitions in authRoutes.ts export routers directly instead of mounting them to a single router, which will crash the app on import. Registration ignores the 'name' field entirely—only email and password are extracted from req.body, but the task requires registering with name, email, and password. Login uses GET method (router.get('/login')) when POST is required to receive credentials in the request body. Password change functionality is completely missing from the ProfileController despite being explicitly required for the profile page. Email change doesn't notify the old email address as specified in the requirements. Additionally, there's a missing return statement in resetPasswordConfirmation after password/confirmation mismatch check, allowing execution to continue with invalid data. These issues must be addressed before the implementation can be approved.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
|
|
||
| export const resetName = async (req, res) => { | ||
| try { | ||
| const { newName } = req.body; |
There was a problem hiding this comment.
The 'name' field from req.body is missing in registration. The requirements specify registering with 'name, email and password'.
| return res.status(400).json({ message: 'Ім’я не може бути порожнім' }); | ||
| } | ||
|
|
||
| const user = await User.findByPk(userId); |
There was a problem hiding this comment.
The User model requires 'name' field but doesn't set allowNull: false, which is inconsistent. The name field should be properly validated at model level.
| try { | ||
| const { newName } = req.body; | ||
| const userId = req.session.userId; | ||
|
|
There was a problem hiding this comment.
The auth middleware returns a redirect to '/login' instead of returning a 401 status code for unauthenticated users. The requirements specify that profile and other authenticated pages should be restricted.
| user.email = newEmail; | ||
| user.isActive = false; | ||
| user.activationToken = uuidv4(); | ||
|
|
There was a problem hiding this comment.
When email is changed, there's no notification sent to the old email address as required by the task. The requirements state: 'notify the old email about the change'.
| import { User } from '../models/User'; | ||
| import bcrypt from 'bcrypt'; | ||
| import { v4 as uuidv4 } from 'uuid'; | ||
|
|
||
| export const resetName = async (req, res) => { | ||
| try { | ||
| const { newName } = req.body; | ||
| const userId = req.session.userId; | ||
|
|
||
| if (!newName) { | ||
| return res.status(400).json({ message: 'Ім’я не може бути порожнім' }); | ||
| } | ||
|
|
||
| const user = await User.findByPk(userId); | ||
|
|
||
| if (!user) { | ||
| return res.status(404).json({ message: 'Користувача не знайдено' }); | ||
| } | ||
|
|
||
| user.name = newName; | ||
| await user.save(); | ||
|
|
||
| return res | ||
| .status(200) | ||
| .json({ message: 'Ім’я успішно оновлено', name: user.name }); | ||
| } catch (error) { | ||
| res.status(500).json({ message: 'Помилка на сервері під час реєстрації' }); | ||
| } | ||
| }; | ||
|
|
||
| export const changeEmail = async (req, res) => { | ||
| try { | ||
| const { password, newEmail } = req.body; | ||
| const userId = req.session.userId; | ||
|
|
||
| const user = await User.findByPk(userId); | ||
|
|
||
| if (!user) { | ||
| return res.status(404).json({ message: 'Користувача не знайдено' }); | ||
| } | ||
|
|
||
| const isPasswordValid = await bcrypt.compare(password, user.password); | ||
|
|
||
| if (!isPasswordValid) { | ||
| return res.status(401).json({ message: 'Невірний пароль' }); | ||
| } | ||
|
|
||
| const checkMail = await User.findOne({ where: { email: newEmail } }); | ||
|
|
||
| if (checkMail) { | ||
| return res | ||
| .status(400) | ||
| .json({ message: 'Цей email вже використовується' }); | ||
| } | ||
| user.email = newEmail; | ||
| user.isActive = false; | ||
| user.activationToken = uuidv4(); | ||
|
|
||
| await user.save(); | ||
|
|
||
| return res.status(200).json({ | ||
| message: 'Email змінено. Будь ласка, активуйте нову пошту.', | ||
| }); | ||
| } catch (error) { | ||
| res.status(500).json({ message: 'Помилка на сервері під час реєстрації' }); | ||
| } |
There was a problem hiding this comment.
The password change endpoint (with old password, new password and confirmation) is completely missing from the ProfileController. The requirements specify this is required for the profile page.
| if (!newName) { | ||
| return res.status(400).json({ message: 'Ім’я не може бути порожнім' }); | ||
| } | ||
|
|
||
| const user = await User.findByPk(userId); | ||
|
|
||
| if (!user) { | ||
| return res.status(404).json({ message: 'Користувача не знайдено' }); | ||
| } | ||
|
|
||
| user.name = newName; | ||
| await user.save(); | ||
|
|
||
| return res | ||
| .status(200) | ||
| .json({ message: 'Ім’я успішно оновлено', name: user.name }); | ||
| } catch (error) { | ||
| res.status(500).json({ message: 'Помилка на сервері під час реєстрації' }); | ||
| } | ||
| }; | ||
|
|
||
| export const changeEmail = async (req, res) => { | ||
| try { | ||
| const { password, newEmail } = req.body; | ||
| const userId = req.session.userId; | ||
|
|
There was a problem hiding this comment.
Routes are defined incorrectly - each route is exported directly without being added to the router. This is an anti-pattern that breaks the router structure. Routes should use router.post(...) format, not direct exports.
| import { User } from '../models/User'; | ||
| import bcrypt from 'bcrypt'; | ||
| import { v4 as uuidv4 } from 'uuid'; |
There was a problem hiding this comment.
The middleware only checks for session existence but doesn't verify if the user is actually active (isActive flag). Non-active users should be handled appropriately.
| import { User } from '../models/User'; | ||
| import bcrypt from 'bcrypt'; | ||
| import { v4 as uuidv4 } from 'uuid'; |
There was a problem hiding this comment.
Mixing CommonJS require() with ES6 export syntax. Use consistent module syntax throughout.
| host: 'localhost', | ||
| dialect: 'postgres', | ||
| logging: false, | ||
| define: { |
There was a problem hiding this comment.
Registration doesn't capture 'name' from req.body - the task requires registering with name, email and password. Currently only email and password are extracted.
| import { Op } from 'sequelize'; | ||
|
|
||
| export const register = async (req, res) => { | ||
| const { email, password } = req.body; |
There was a problem hiding this comment.
The register function only extracts email and password from req.body, but the task requires registration with name, email, and password. The name field is missing and not being saved to the database.
|
|
||
| const candidate = await User.findOne({ where: { email } }); | ||
|
|
||
| if (candidate) { |
There was a problem hiding this comment.
The login route is defined with GET method, but login requires receiving email and password in the request body. GET requests cannot have a body, so the credentials will never be received. This should be POST.
| import { Op } from 'sequelize'; | ||
|
|
||
| export const register = async (req, res) => { | ||
| const { email, password } = req.body; |
There was a problem hiding this comment.
The authMiddleware returns res.status(401).redirect('/login'). A redirect response should have status 302, not 401. Browsers/clients may not properly handle a 401 with a redirect. Either use res.redirect(302, '/login') or return a 401 JSON response without redirect.
| const { token } = req.params; | ||
| const { confirmation, password } = req.body; | ||
|
|
||
| if (password !== confirmation) { |
There was a problem hiding this comment.
When password !== confirmation, the function continues execution without returning. It should have return res.status(400).json({...}) to stop processing and return an error response.
| return res.status(200).redirect('/profile'); | ||
| } catch (error) { | ||
| res.status(500).json({ message: 'Помилка на сервері під час реєстрації' }); | ||
| } |
There was a problem hiding this comment.
The changeEmail function doesn't send a notification to the old email address when the email is changed. The task specifically requires: 'notify the old email about the change'.
| import bcrypt from 'bcrypt'; | ||
| import { User } from '../models/User'; | ||
| import { v4 as uuidv4 } from 'uuid'; |
There was a problem hiding this comment.
The database.js file uses CommonJS require syntax but exports with ES6 export. This mixing will cause a syntax error. Use either module.exports or convert entirely to ES6 imports.
| import bcrypt from 'bcrypt'; | ||
| import { User } from '../models/User'; | ||
| import { v4 as uuidv4 } from 'uuid'; | ||
| import { Op } from 'sequelize'; | ||
|
|
||
| export const register = async (req, res) => { | ||
| const { email, password } = req.body; | ||
| const passwordRegex = /^(?=.*[a-z])(?=.*[A-Z])(?=.*\d).{8,}$/; | ||
|
|
||
| try { | ||
| if (!passwordRegex.test(password)) { | ||
| return res.status(400).json({ | ||
| message: | ||
| 'Пароль занадто слабкий: мінімум 8 символів, одна велика ' + | ||
| 'літера та одна цифра.', | ||
| }); | ||
| } | ||
|
|
||
| const candidate = await User.findOne({ where: { email } }); | ||
|
|
||
| if (candidate) { | ||
| return res | ||
| .status(400) | ||
| .json({ message: 'Користувач з таким email вже існує' }); | ||
| } | ||
|
|
||
| const hashedPassword = await bcrypt.hash(password, 10); | ||
| const activationToken = uuidv4(); |
There was a problem hiding this comment.
No route protects unauthenticated users from accessing authenticated-only pages. Routes for login, register, forgot-password should redirect to profile if user is already authenticated. Currently, any user can access these routes.
| import { sequelize } from './config/database'; | ||
| import 'dotenv/config'; |
There was a problem hiding this comment.
The registration function only captures email and password from req.body, but the task requires registering with name, email, and password. The name field is missing.
| try { | ||
| await sequelize.sync({ alter: true }); | ||
|
|
||
| app.listen(PORT, () => {}); | ||
| } catch (error) {} | ||
| }; |
There was a problem hiding this comment.
The name field is not being passed to User.create(). The task requires registration with name, email, and password. This will cause registration to fail or create users without names.
|
|
||
| export const authRouter = router.post('/register', register); |
There was a problem hiding this comment.
Routes are exported directly without being added to the router - this is incorrect. Each line like export const authRouter = router.post(...) creates a new mini-router but doesn't attach it to the main router. The pattern should be: router.post('/register', register) followed by export default router or export { router }.
| export const tokenRouter = router.get('/activate/:token', activate); | ||
| export const logInRouter = router.get('/login', login); |
There was a problem hiding this comment.
The login route is defined with GET method, but login requires receiving email and password in the request body. GET requests cannot send a body, so credentials will never be received. Change to router.post('/login', login).
| export const logOutRouter = router.post('/logout', isAuthenticated, logOut); | ||
| export const resetNameRouter = router.put( | ||
| '/profile/name', | ||
| isAuthenticated, | ||
| resetName, | ||
| ); | ||
| export const changeEmailRouter = router.put( | ||
| '/profile/email', | ||
| isAuthenticated, | ||
| changeEmail, |
There was a problem hiding this comment.
No route exists for changing password in the profile. The task requires: 'It allows to change a password (require an old one, new password and confirmation)'. A route like router.put('/profile/password', isAuthenticated, changePassword) is needed, along with the corresponding controller function.
|
|
||
| export const authRouter = router.post('/register', register); | ||
| export const tokenRouter = router.get('/activate/:token', activate); | ||
| export const logInRouter = router.get('/login', login); | ||
| export const forgotPasswordRouter = router.post( | ||
| '/forgot-password', | ||
| forgotPassword, | ||
| ); | ||
| export const resetPasswordRouter = router.post( | ||
| '/reset-password/:token', | ||
| resetPasswordConfirmation, |
There was a problem hiding this comment.
Routes for non-authenticated users (/register, /login, /forgot-password) should check if the user is already logged in and redirect to profile. Currently, any user can access these routes even when authenticated.
| import { DataTypes } from 'sequelize'; | ||
| import { sequelize } from '../config/database'; | ||
|
|
There was a problem hiding this comment.
The middleware only checks if session exists but doesn't verify if the user is actually active. The task requires checking user activation status before allowing access to authenticated pages.
| email: { | ||
| type: DataTypes.STRING, | ||
| unique: true, | ||
| }, |
There was a problem hiding this comment.
The login route is defined with GET method, but credentials need to be sent in the request body which requires POST method.
| email: { | ||
| type: DataTypes.STRING, |
There was a problem hiding this comment.
The User model email field lacks validation constraints. Should include allowNull: false and validate email format properly.
| password: { | ||
| type: DataTypes.STRING, |
There was a problem hiding this comment.
The User model password field lacks validation constraints. Should include allowNull: false since it's required for authentication.
| }, | ||
| resetPasswordToken: { | ||
| type: DataTypes.STRING, | ||
| }, | ||
| resetPasswordExpires: { | ||
| type: DataTypes.DATE, | ||
| }, |
There was a problem hiding this comment.
The User model lacks a changePasswordToken or similar field to track password change requests separately from resetPasswordToken.
| } | ||
|
|
||
| return res.status(401).redirect('/login'); | ||
| }; |
There was a problem hiding this comment.
The register function only extracts email and password from req.body, but the task requires registration with name, email, and password. The name field is missing and not being saved to the database.
| if (req.session && req.session.userId) { | ||
| return next(); | ||
| } | ||
|
|
There was a problem hiding this comment.
The authMiddleware returns res.status(401).redirect('/login'). A redirect response should have status 302, not 401. Browsers/clients may not properly handle a 401 with a redirect. Either use res.redirect(302, '/login') or return a 401 JSON response without redirect.
| export const isAuthenticated = (req, res, next) => { | ||
| if (req.session && req.session.userId) { | ||
| return next(); |
There was a problem hiding this comment.
The database.js file uses CommonJS require syntax but exports with ES6 export. This mixing will cause a syntax error. Use either module.exports or convert entirely to ES6 imports.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The implementation has made good progress fixing several previous issues, but one critical feature remains completely missing from the Profile page: password change functionality. The task explicitly requires that users can change their password by providing their old password, a new password, and a confirmation. Currently, ProfileController.js only implements resetName and changeEmail functions with no route or handler for password changes. Critical issues include missing password change endpoint and route, which must be implemented to meet requirements.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| import { User } from '../models/User'; | ||
| import bcrypt from 'bcrypt'; | ||
| import { v4 as uuidv4 } from 'uuid'; | ||
| import { | ||
| sendActivationMail, | ||
| sendEmailChangeNotification, | ||
| } from '../services/mailService'; | ||
|
|
||
| export const resetName = async (req, res) => { | ||
| try { | ||
| const { newName } = req.body; | ||
| const userId = req.session.userId; | ||
|
|
||
| if (!newName) { | ||
| return res.status(400).json({ message: 'Ім’я не може бути порожнім' }); | ||
| } | ||
|
|
||
| const user = await User.findByPk(userId); | ||
|
|
||
| if (!user) { | ||
| return res.status(404).json({ message: 'Користувача не знайдено' }); | ||
| } | ||
|
|
||
| user.name = newName; | ||
| await user.save(); | ||
|
|
||
| return res | ||
| .status(200) | ||
| .json({ message: 'Ім’я успішно оновлено', name: user.name }); | ||
| } catch (error) { | ||
| res.status(500).json({ message: 'Помилка на сервері під час реєстрації' }); | ||
| } | ||
| }; | ||
|
|
||
| export const changeEmail = async (req, res) => { | ||
| try { | ||
| const { password, newEmail } = req.body; | ||
| const userId = req.session.userId; | ||
|
|
||
| const user = await User.findByPk(userId); | ||
|
|
||
| if (!user) { | ||
| return res.status(404).json({ message: 'Користувача не знайдено' }); | ||
| } | ||
|
|
||
| const isPasswordValid = await bcrypt.compare(password, user.password); | ||
|
|
||
| if (!isPasswordValid) { | ||
| return res.status(401).json({ message: 'Невірний пароль' }); | ||
| } | ||
|
|
||
| const checkMail = await User.findOne({ where: { email: newEmail } }); | ||
|
|
||
| if (checkMail) { | ||
| return res | ||
| .status(400) | ||
| .json({ message: 'Цей email вже використовується' }); | ||
| } | ||
|
|
||
| const oldEmail = user.email; | ||
|
|
||
| user.email = newEmail; | ||
| user.isActive = false; | ||
| user.activationToken = uuidv4(); | ||
| await user.save(); | ||
|
|
||
| try { | ||
| await sendEmailChangeNotification(oldEmail, newEmail); | ||
|
|
||
| await sendActivationMail(newEmail, user.activationToken); | ||
| } catch (mailError) {} | ||
|
|
||
| return res.status(200).json({ | ||
| message: 'Email змінено. Будь ласка, активуйте нову пошту.', | ||
| }); | ||
| } catch (error) { | ||
| res.status(500).json({ message: 'Помилка на сервері під час реєстрації' }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The Profile page requirements specify: 'It allows to change a password (require an old one, new password and confirmation)'. This functionality is not implemented anywhere in the file - there's only resetName and changeEmail functions.
|
|
||
| user.password = hashedPassword; | ||
| user.resetPasswordToken = null; |
There was a problem hiding this comment.
The resetPasswordExpires field is set to a new Date() but never cleared after successful password reset. After nullifying resetPasswordToken, you should also set resetPasswordExpires to null to properly clean up.
| unique: true, | ||
| }, | ||
| password: { | ||
| type: DataTypes.STRING, |
There was a problem hiding this comment.
The route for password change is missing. The task requires a password change feature on the Profile page that accepts old password, new password, and confirmation. Add: router.put('/profile/password', isAuthenticated, changePassword);
| } | ||
| req.user = user; | ||
|
|
||
| return next(); |
There was a problem hiding this comment.
Empty catch block silently swallows all errors. If database query fails during authentication check, the error is ignored and next() is called, potentially allowing unauthenticated requests to proceed.
| }); | ||
| } | ||
| req.user = user; | ||
|
|
There was a problem hiding this comment.
Missing password change route. The Profile page requires a password change endpoint (with old password, new password, and confirmation). Add a route like: router.put('/profile/password', isAuthenticated, changePassword);
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your implementation with some important issues to address.
What you did well: Your authentication system is well-structured with proper password validation, email activation flow, protected routes, and most profile management features working correctly. The code organization and middleware usage show good understanding of Express patterns.
Critical issues to fix:
-
Password change verification is inverted in
src/controllers/ProfileController.js- The requirements explicitly state password change must require "old password, new password and confirmation," but yourpasswordChangefunction:- Doesn't receive
oldPasswordfromreq.bodyat all - Compares
newPasswordagainst the current password instead of verifying the old one first
The fix requires:
- Add
oldPasswordto destructuring:const { oldPassword, newPassword, confirmation } = req.body; - Use
bcrypt.compare(oldPassword, user.password)to verify identity before allowing the change
- Doesn't receive
-
Empty catch block at line 117 will cause hanging requests on errors - add a proper error response like
res.status(500).json({ message: 'Помилка при зміні пароля' });
Minor improvement: The resetPasswordConfirmation function doesn't validate password strength (the regex used in registration), which could allow weak passwords during reset flow.
These are standard security patterns for password changes - verifying the old password proves identity before allowing modifications. Keep up the good work on the overall architecture!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| }; | ||
|
|
||
| export const passwordChange = async (req, res) => { | ||
| try { |
There was a problem hiding this comment.
The requirements state that changing password must require an 'old one' (old password), but this function doesn't verify the submitted old password. It only checks if the new password differs from the current one. You should receive 'oldPassword' from req.body and verify it with bcrypt.compare() before allowing the password change.
| user.password = hashedPassword; | ||
| await user.save(); | ||
|
|
||
| return res.status(200).json({ message: 'Пароль успішно змінено' }); |
There was a problem hiding this comment.
The catch block is empty - if an error occurs, no response will be sent to the client, causing a hanging request. Add proper error handling like: catch (error) { res.status(500).json({ message: 'Помилка при зміні пароля' }); }
| .status(400) | ||
| .json({ message: 'Будь ласка, активуйте свій email' }); |
There was a problem hiding this comment.
The oldPassword field is missing from the destructured request body. According to requirements, password change requires 'old password, new password and confirmation'. Add oldPassword to the destructuring on line 82.
|
|
||
| req.session.destroy((err) => { | ||
| if (err) { | ||
| return res.status(500).json({ message: 'Не вдалося вийти з системи' }); | ||
| } |
There was a problem hiding this comment.
The comparison logic here is checking if newPassword equals user.password - this prevents password reuse but doesn't verify the old password as required. The requirements state 'require an old one' meaning you must verify the current password first before allowing the change. Consider comparing the oldPassword with the user's current password instead.
| }); | ||
| }; | ||
|
|
||
| export const forgotPassword = async (req, res) => { |
There was a problem hiding this comment.
Empty catch block on line 117 will silently swallow errors. Consider adding error logging or returning a proper error response.
brespect
left a comment
There was a problem hiding this comment.
Good progress, but you need to pass all tests and build before requesting the review
Anton-Kuchmasov
left a comment
There was a problem hiding this comment.
Good job!
Don't push .env as well as any other secrets/envs in Github repo again
No description provided.