Still working about solution#275
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This implementation has multiple critical gaps that prevent completing the task requirements. Several explicit features are either missing entirely or implemented incorrectly.
Critical Issues That Must Be Fixed
-
Password validation is missing - The requirements state: "Inform the users about the rules for a password and check them." You must implement password strength validation (minimum length, special characters, etc.) during registration in
authService.jsorauthController.js. -
Activation email not sent - The
registerfunction onlyconsole.logsthe activation link. You must import and callemailService.sendActivationEmail()after user creation inauthService.js. -
Password reset flow is completely missing - The requirements explicitly require: "Ask for an email, Show email sent page, add Reset Password confirmation page, Show Success page with a link to login." No endpoints exist for this functionality. You need to add routes (
/forgot-password,/reset-password/:token), controllers, and an email service function for password reset emails. -
Email change functionality not implemented - Requirements state: "To change an email you should type the password, confirm the new email and notify the old email about the change." No
changeEmailendpoint exists inuserRoute.js. -
Password confirmation not validated - The
changePasswordfunction acceptsnewPasswordandconfirmationfields but never validates they match. This validation must be added. -
Activation route mismatch -
authRoute.jsusesrouter.post('/activate', activate)but the controller expectsreq.paramsfor email and token. Change torouter.get('/activate/:email/:token', activate). -
No 404 handler - Add a catch-all route handler in
index.jsfor undefined routes.
Summary
You've implemented the basic structure with proper separation of concerns, but the core authentication features specified in the task are either missing or broken. Please address all seven issues above before resubmission.
✨ 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
| }); | ||
|
|
There was a problem hiding this comment.
The changePassword function is missing the confirmation field validation. The requirements specify: 'It allows to change a password (require an old one, new password and confirmation)'. Currently only validates oldPassword and newPassword.
| /* eslint-disable no-console */ | ||
| import bcrypt from 'bcrypt'; | ||
| import jwt from 'jsonwebtoken'; | ||
| import { User } from '../models/User.js'; | ||
| import { v4 as uuidv4 } from 'uuid'; | ||
|
|
||
| // REGISTER | ||
| export const register = async (req, res) => { | ||
| const { name, email, password } = req.body; | ||
|
|
||
| const hash = await bcrypt.hash(password, 10); | ||
|
|
||
| const user = await User.create({ | ||
| name, | ||
| email, | ||
| password: hash, | ||
| activationToken: uuidv4(), | ||
| isActive: false, | ||
| }); | ||
|
|
||
| console.log(`Activate: /auth/activate/${user.email}/${user.activationToken}`); | ||
|
|
||
| res.json({ message: 'Check email' }); | ||
| }; | ||
|
|
||
| // ACTIVATE | ||
| export const activate = async (req, res) => { | ||
| const { email, token } = req.params; | ||
|
|
||
| const user = await User.findOne({ | ||
| where: { email, activationToken: token }, | ||
| }); | ||
|
|
||
| if (!user) { |
There was a problem hiding this comment.
The User model is missing fields needed for email change functionality. The requirements state: 'To change an email you should type the password, confirm the new email and notify the old email about the change'. Consider adding newEmail, newEmailToken, or similar fields to track pending email changes.
| export const register = async (req, res) => { | ||
| const { name, email, password } = req.body; | ||
|
|
||
| const hash = await bcrypt.hash(password, 10); | ||
|
|
||
| const user = await User.create({ | ||
| name, | ||
| email, | ||
| password: hash, | ||
| activationToken: uuidv4(), | ||
| isActive: false, | ||
| }); | ||
|
|
||
| console.log(`Activate: /auth/activate/${user.email}/${user.activationToken}`); | ||
|
|
||
| res.json({ message: 'Check email' }); |
There was a problem hiding this comment.
The register function only console.logs the activation link instead of sending an actual email. The requirements state: 'send an activation email'. The emailService.js file exists but is not imported or used.
| export const register = async (req, res) => { | ||
| const { name, email, password } = req.body; | ||
|
|
||
| const hash = await bcrypt.hash(password, 10); | ||
|
|
||
| const user = await User.create({ | ||
| name, | ||
| email, | ||
| password: hash, | ||
| activationToken: uuidv4(), | ||
| isActive: false, | ||
| }); | ||
|
|
||
| console.log(`Activate: /auth/activate/${user.email}/${user.activationToken}`); | ||
|
|
||
| res.json({ message: 'Check email' }); |
There was a problem hiding this comment.
The register function does not validate password rules. The requirements state: 'Inform the users about the rules for a password and check them'. No password validation is implemented.
| // ACTIVATE | ||
| export const activate = async (req, res) => { | ||
| const { email, token } = req.params; | ||
|
|
||
| const user = await User.findOne({ | ||
| where: { email, activationToken: token }, | ||
| }); | ||
|
|
||
| if (!user) { | ||
| return res.status(400).json({ message: 'Invalid link' }); | ||
| } | ||
|
|
||
| user.isActive = true; | ||
| await user.save(); | ||
|
|
||
| res.json({ message: 'Activated' }); |
There was a problem hiding this comment.
The activate function returns JSON instead of redirecting to Profile. The requirements state: 'redirect to Profile after the activation'. This should return a redirect response or frontend URL.
| // LOGIN | ||
| export const login = async (req, res) => { | ||
| const { email, password } = req.body; | ||
|
|
||
| const user = await User.findOne({ where: { email } }); | ||
|
|
||
| if (!user) { | ||
| return res.status(400).json({ message: 'Invalid credentials' }); | ||
| } | ||
|
|
||
| if (!user.isActive) { | ||
| return res.status(403).json({ message: 'Activate email first' }); | ||
| } | ||
|
|
||
| const ok = await bcrypt.compare(password, user.password); | ||
|
|
||
| if (!ok) { | ||
| return res.status(400).json({ message: 'Invalid credentials' }); | ||
| } | ||
|
|
||
| const accessToken = jwt.sign({ id: user.id }, process.env.JWT_SECRET, { | ||
| expiresIn: '15m', | ||
| }); | ||
|
|
||
| const refreshToken = jwt.sign( | ||
| { id: user.id }, | ||
| process.env.JWT_REFRESH_SECRET, | ||
| { expiresIn: '7d' }, | ||
| ); | ||
|
|
||
| res.json({ | ||
| accessToken, | ||
| refreshToken, | ||
| user: { | ||
| id: user.id, | ||
| name: user.name, | ||
| email: user.email, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
The login function returns JSON with tokens but no redirect information. The requirements state: 'Redirect to profile after login'. This needs to include a redirect URL for the frontend.
| // LOGOUT | ||
| export const logout = async (req, res) => { | ||
| res.clearCookie('refreshToken'); | ||
|
|
||
| res.json({ message: 'Logged out' }); |
There was a problem hiding this comment.
The logout function only clears the cookie and returns JSON. The requirements state: 'Redirect to login after logging out'. This needs to include a redirect URL for the frontend.
| /* eslint-disable no-console */ | ||
| import bcrypt from 'bcrypt'; | ||
| import jwt from 'jsonwebtoken'; | ||
| import { User } from '../models/User.js'; | ||
| import { v4 as uuidv4 } from 'uuid'; | ||
|
|
||
| // REGISTER | ||
| export const register = async (req, res) => { | ||
| const { name, email, password } = req.body; | ||
|
|
||
| const hash = await bcrypt.hash(password, 10); | ||
|
|
||
| const user = await User.create({ | ||
| name, | ||
| email, | ||
| password: hash, | ||
| activationToken: uuidv4(), | ||
| isActive: false, | ||
| }); | ||
|
|
||
| console.log(`Activate: /auth/activate/${user.email}/${user.activationToken}`); | ||
|
|
||
| res.json({ message: 'Check email' }); | ||
| }; | ||
|
|
||
| // ACTIVATE | ||
| export const activate = async (req, res) => { | ||
| const { email, token } = req.params; | ||
|
|
||
| const user = await User.findOne({ | ||
| where: { email, activationToken: token }, | ||
| }); | ||
|
|
||
| if (!user) { | ||
| return res.status(400).json({ message: 'Invalid link' }); | ||
| } | ||
|
|
||
| user.isActive = true; | ||
| await user.save(); | ||
|
|
||
| res.json({ message: 'Activated' }); | ||
| }; | ||
|
|
||
| // LOGIN | ||
| export const login = async (req, res) => { | ||
| const { email, password } = req.body; | ||
|
|
||
| const user = await User.findOne({ where: { email } }); | ||
|
|
||
| if (!user) { | ||
| return res.status(400).json({ message: 'Invalid credentials' }); | ||
| } | ||
|
|
||
| if (!user.isActive) { | ||
| return res.status(403).json({ message: 'Activate email first' }); | ||
| } | ||
|
|
||
| const ok = await bcrypt.compare(password, user.password); | ||
|
|
||
| if (!ok) { | ||
| return res.status(400).json({ message: 'Invalid credentials' }); | ||
| } | ||
|
|
||
| const accessToken = jwt.sign({ id: user.id }, process.env.JWT_SECRET, { | ||
| expiresIn: '15m', | ||
| }); | ||
|
|
||
| const refreshToken = jwt.sign( | ||
| { id: user.id }, | ||
| process.env.JWT_REFRESH_SECRET, | ||
| { expiresIn: '7d' }, | ||
| ); | ||
|
|
||
| res.json({ | ||
| accessToken, | ||
| refreshToken, | ||
| user: { | ||
| id: user.id, | ||
| name: user.name, | ||
| email: user.email, | ||
| }, | ||
| }); | ||
| }; | ||
|
|
||
| // REFRESH | ||
| export const refresh = async (req, res) => { | ||
| try { | ||
| const refreshToken = req.cookies.refreshToken; | ||
|
|
||
| if (!refreshToken) { | ||
| return res.status(401).json({ message: 'No refresh token' }); | ||
| } | ||
|
|
||
| const data = jwt.verify(refreshToken, process.env.JWT_REFRESH_SECRET); | ||
|
|
||
| const accessToken = jwt.sign({ id: data.id }, process.env.JWT_SECRET, { | ||
| expiresIn: '15m', | ||
| }); | ||
|
|
||
| res.json({ accessToken }); | ||
| } catch (e) { | ||
| res.status(401).json({ message: 'Invalid refresh token' }); | ||
| } | ||
| }; | ||
|
|
||
| // LOGOUT | ||
| export const logout = async (req, res) => { | ||
| res.clearCookie('refreshToken'); | ||
|
|
||
| res.json({ message: 'Logged out' }); | ||
| }; |
There was a problem hiding this comment.
The entire password reset flow is missing. The requirements specify: 'Password reset (only non-authenticated) - Ask for an email, Show email sent page, add Reset Password confirmation page (with password and confirmation fields that must be equal), Show Success page with a link to login'. No reset password routes, controllers, or endpoints exist.
| /* eslint-disable no-console */ | ||
| import bcrypt from 'bcrypt'; | ||
| import jwt from 'jsonwebtoken'; | ||
| import { User } from '../models/User.js'; | ||
| import { v4 as uuidv4 } from 'uuid'; | ||
|
|
||
| // REGISTER | ||
| export const register = async (req, res) => { | ||
| const { name, email, password } = req.body; | ||
|
|
||
| const hash = await bcrypt.hash(password, 10); | ||
|
|
||
| const user = await User.create({ | ||
| name, | ||
| email, | ||
| password: hash, | ||
| activationToken: uuidv4(), | ||
| isActive: false, | ||
| }); | ||
|
|
||
| console.log(`Activate: /auth/activate/${user.email}/${user.activationToken}`); | ||
|
|
||
| res.json({ message: 'Check email' }); | ||
| }; | ||
|
|
||
| // ACTIVATE | ||
| export const activate = async (req, res) => { | ||
| const { email, token } = req.params; | ||
|
|
||
| const user = await User.findOne({ | ||
| where: { email, activationToken: token }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
The email change functionality is missing. The requirements state: 'To change an email you should type the password, confirm the new email and notify the old email about the change'. No controller or route exists for changing email.
| /* eslint-disable no-console */ | ||
| import bcrypt from 'bcrypt'; | ||
| import jwt from 'jsonwebtoken'; | ||
| import { User } from '../models/User.js'; | ||
| import { v4 as uuidv4 } from 'uuid'; | ||
|
|
||
| // REGISTER | ||
| export const register = async (req, res) => { | ||
| const { name, email, password } = req.body; | ||
|
|
||
| const hash = await bcrypt.hash(password, 10); | ||
|
|
||
| const user = await User.create({ | ||
| name, | ||
| email, | ||
| password: hash, | ||
| activationToken: uuidv4(), | ||
| isActive: false, | ||
| }); | ||
|
|
||
| console.log(`Activate: /auth/activate/${user.email}/${user.activationToken}`); | ||
|
|
||
| res.json({ message: 'Check email' }); | ||
| }; | ||
|
|
||
| // ACTIVATE | ||
| export const activate = async (req, res) => { | ||
| const { email, token } = req.params; | ||
|
|
||
| const user = await User.findOne({ | ||
| where: { email, activationToken: token }, | ||
| }); | ||
|
|
||
| if (!user) { | ||
| return res.status(400).json({ message: 'Invalid link' }); | ||
| } | ||
|
|
||
| user.isActive = true; | ||
| await user.save(); | ||
|
|
||
| res.json({ message: 'Activated' }); | ||
| }; | ||
|
|
||
| // LOGIN | ||
| export const login = async (req, res) => { |
There was a problem hiding this comment.
The index.js is missing a 404 handler for all other pages. The requirements state: '404 for all the other pages'.
| const hash = await bcrypt.hash(password, 10); | ||
|
|
||
| const user = await User.create({ | ||
| name, |
There was a problem hiding this comment.
The activation route is mapped to POST /activate in authRoute.js (line 14: router.post('/activate', activate)), but the activate function in authController.js uses req.params (line 27: const { email, token } = req.params). POST requests don't have URL parameters in the traditional sense - this routing is incorrect.
| if (!ok) return res.status(400).json({ message: 'Wrong password' }); | ||
|
|
||
| user.password = await bcrypt.hash(newPassword, 10); | ||
| await user.save(); |
There was a problem hiding this comment.
The route uses router.post('/activate', activate) expecting a body, but the controller's activate function uses req.params for email and token. These don't match - either change the route to router.get('/activate/:email/:token', activate) or change the controller to use req.body.
| export const changePassword = async (req, res) => { | ||
| const { oldPassword, newPassword } = req.body; | ||
|
|
||
| const user = await User.findByPk(req.user.id); |
There was a problem hiding this comment.
The sendActivationEmail function from emailService.js is never called. Registration only logs the activation link to console (line 21) instead of actually sending an email. The emailService should be imported and called here.
| await user.save(); | ||
|
|
||
| res.json(user); | ||
| }; |
There was a problem hiding this comment.
Task requirement states: 'Inform the users about the rules for a password and check them'. There's no password validation (e.g., minimum length, special characters, uppercase) being performed before creating the user.
|
|
||
| export const changePassword = async (req, res) => { | ||
| const { oldPassword, newPassword } = req.body; | ||
|
|
||
| const user = await User.findByPk(req.user.id); | ||
|
|
||
| const ok = await bcrypt.compare(oldPassword, user.password); | ||
|
|
||
| if (!ok) return res.status(400).json({ message: 'Wrong password' }); | ||
|
|
||
| user.password = await bcrypt.hash(newPassword, 10); | ||
| await user.save(); | ||
|
|
||
| res.json({ message: 'Password changed' }); |
There was a problem hiding this comment.
The task requires a password change endpoint that checks 'new password' and 'confirmation' are equal. Currently, changePassword only takes oldPassword and newPassword without a confirmation field or validation that newPassword === confirmation.
| await user.save(); | ||
|
|
||
| res.json({ message: 'Password changed' }); | ||
| }; |
There was a problem hiding this comment.
Email change functionality is not implemented. Task requirement states: 'To change an email you should type the password, confirm the new email and notify the old email about the change'.
|
|
||
| const ok = await bcrypt.compare(oldPassword, user.password); | ||
|
|
||
| if (!ok) return res.status(400).json({ message: 'Wrong password' }); |
There was a problem hiding this comment.
The task requires '404 for all other pages'. There's no catch-all route handler for unmatched routes in index.js.
| credentials: true, | ||
| }), |
There was a problem hiding this comment.
The changePassword function only destructures oldPassword and newPassword from req.body. According to the task requirements, it must also accept and validate a confirmation field that must equal the new password.
| credentials: true, | ||
| }), | ||
| ); | ||
|
|
||
| app.use('/auth', authRouter); | ||
| app.use('/users', userRouter); | ||
|
|
||
| app.get('/', (req, res) => { | ||
| res.send('Hello'); | ||
| }); | ||
|
|
||
| const startServer = async () => { | ||
| try { | ||
| await sequelize.authenticate(); |
There was a problem hiding this comment.
The changePassword function doesn't validate that newPassword equals confirmation. This validation is explicitly required by the task: "add Reset Password confirmation page (with password and confirmation fields that must be equal)".
| /* eslint-disable no-console */ | ||
| import 'dotenv/config'; | ||
| import express from 'express'; | ||
| import cors from 'cors'; | ||
|
|
||
| import sequelize from './config/db.js'; | ||
| import { authRouter } from './routes/authRoute.js'; | ||
| import { userRouter } from './routes/userRoute.js'; | ||
|
|
||
| const PORT = process.env.PORT || 3005; | ||
|
|
||
| const app = express(); | ||
|
|
||
| app.use(express.json()); | ||
|
|
||
| app.use( | ||
| cors({ | ||
| origin: process.env.CLIENT_HOST, | ||
| credentials: true, | ||
| }), | ||
| ); | ||
|
|
||
| app.use('/auth', authRouter); | ||
| app.use('/users', userRouter); | ||
|
|
||
| app.get('/', (req, res) => { | ||
| res.send('Hello'); | ||
| }); | ||
|
|
||
| const startServer = async () => { | ||
| try { | ||
| await sequelize.authenticate(); | ||
| console.log('DB connected'); |
There was a problem hiding this comment.
No changeEmail function exists. The task requires: "To change an email you should type the password, confirm the new email and notify the old email about the change". This endpoint is completely missing.
|
|
||
| try { | ||
| const data = jwt.verify(token, process.env.JWT_SECRET); | ||
| req.user = data; |
There was a problem hiding this comment.
The activate route uses router.post('/activate', activate) which sends data in the request body, but the activate controller in authController.js (line 27) uses req.params to extract email and token from the URL path. This mismatch will cause the activation to always fail. Should be router.get('/activate/:email/:token', activate) to match the controller's expectation.
| /* eslint-disable padding-line-between-statements */ | ||
| import jwt from 'jsonwebtoken'; | ||
|
|
||
| export const authMiddleware = (req, res, next) => { | ||
| const token = req.headers.authorization?.split(' ')[1]; | ||
|
|
||
| if (!token) { | ||
| return res.sendStatus(401); | ||
| } | ||
|
|
||
| try { | ||
| const data = jwt.verify(token, process.env.JWT_SECRET); | ||
| req.user = data; | ||
| next(); | ||
| } catch { |
There was a problem hiding this comment.
The email change endpoint is completely missing from userRoute.js. The task requirements state: 'To change an email you should type the password, confirm the new email and notify the old email about the change'. There's no route for this functionality.
| /* eslint-disable padding-line-between-statements */ | ||
| import jwt from 'jsonwebtoken'; | ||
|
|
||
| export const authMiddleware = (req, res, next) => { | ||
| const token = req.headers.authorization?.split(' ')[1]; | ||
|
|
||
| if (!token) { | ||
| return res.sendStatus(401); | ||
| } | ||
|
|
||
| try { | ||
| const data = jwt.verify(token, process.env.JWT_SECRET); | ||
| req.user = data; | ||
| next(); | ||
| } catch { | ||
| res.sendStatus(401); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The password reset flow routes are missing from authRoute.js. The task requires: 'ask for email, show email sent page, add Reset Password confirmation page, show Success page with link to login'. No reset password routes exist.
| unique: true, | ||
| }, | ||
|
|
||
| password: DataTypes.STRING, |
There was a problem hiding this comment.
The activate route uses POST but the controller expects req.params. Either change to router.get('/activate/:email/:token', activate) or change controller to use req.body. POST requests don't pass URL parameters like GET does.
| import { DataTypes } from 'sequelize'; | ||
| import sequelize from '../config/db.js'; | ||
| import { v4 as uuidv4 } from 'uuid'; | ||
|
|
||
| export const User = sequelize.define('User', { | ||
| name: DataTypes.STRING, | ||
|
|
||
| email: { | ||
| type: DataTypes.STRING, | ||
| unique: true, | ||
| }, | ||
|
|
||
| password: DataTypes.STRING, | ||
|
|
||
| isActive: { | ||
| type: DataTypes.BOOLEAN, | ||
| defaultValue: false, |
There was a problem hiding this comment.
authRoute is missing password reset endpoints. Requirements state: 'Password reset (only non-authenticated) - Ask for an email, Show email sent page, add Reset Password confirmation page, Show Success page with a link to login'.
| import { DataTypes } from 'sequelize'; | ||
| import sequelize from '../config/db.js'; | ||
| import { v4 as uuidv4 } from 'uuid'; | ||
|
|
||
| export const User = sequelize.define('User', { | ||
| name: DataTypes.STRING, | ||
|
|
||
| email: { | ||
| type: DataTypes.STRING, | ||
| unique: true, | ||
| }, | ||
|
|
||
| password: DataTypes.STRING, | ||
|
|
||
| isActive: { |
There was a problem hiding this comment.
userRoute is missing email change endpoint. Requirements state: 'To change an email you should type the password, confirm the new email and notify the old email about the change'.
|
|
||
| export const User = sequelize.define('User', { | ||
| name: DataTypes.STRING, | ||
|
|
||
| email: { | ||
| type: DataTypes.STRING, | ||
| unique: true, | ||
| }, | ||
|
|
||
| password: DataTypes.STRING, | ||
|
|
||
| isActive: { | ||
| type: DataTypes.BOOLEAN, | ||
| defaultValue: false, | ||
| }, | ||
|
|
||
| activationToken: { | ||
| type: DataTypes.STRING, | ||
| defaultValue: uuidv4, | ||
| }, | ||
|
|
||
| resetToken: DataTypes.STRING, |
There was a problem hiding this comment.
The User model has resetToken field but no model-level validations. Requirements state: 'Inform users about rules for password and check them'. Model should include password validation constraints (min length, etc.).
|
|
||
| router.get('/profile', authMiddleware, getProfile); | ||
| router.patch('/profile', authMiddleware, updateName); | ||
| router.patch('/password', authMiddleware, changePassword); |
There was a problem hiding this comment.
The activation route uses router.post('/activate', activate) but the controller's activate function uses req.params for email and token. POST requests don't use URL parameters. Should be router.get('/activate/:email/:token', activate).
| import { Router } from 'express'; | ||
| import { authMiddleware } from '../middlewares/authMiddleware.js'; | ||
| import { | ||
| getProfile, | ||
| updateName, | ||
| changePassword, | ||
| } from '../controllers/userController.js'; | ||
|
|
||
| const router = Router(); | ||
|
|
||
| router.get('/profile', authMiddleware, getProfile); | ||
| router.patch('/profile', authMiddleware, updateName); | ||
| router.patch('/password', authMiddleware, changePassword); | ||
|
|
||
| export const userRouter = router; |
There was a problem hiding this comment.
User routes are missing the email change endpoint. Need PATCH /email that requires password, new email, and sends notification to old email.
| import { Router } from 'express'; | ||
| import { | ||
| register, | ||
| login, | ||
| activate, | ||
| refresh, | ||
| logout, | ||
| } from '../controllers/authController.js'; | ||
|
|
||
| const router = Router(); | ||
|
|
||
| router.post('/register', register); | ||
| router.post('/login', login); | ||
| router.post('/activate', activate); | ||
| router.get('/refresh', refresh); |
There was a problem hiding this comment.
The requirements state: 'To change an email you should type the password, confirm the new email and notify the old email about the change'. No route exists for this functionality. Need endpoints for: (1) verifying password before email change, (2) sending confirmation to new email, (3) confirming the new email, and (4) notifying the old email about the change.
|
|
||
| const router = Router(); | ||
|
|
||
| router.post('/register', register); |
There was a problem hiding this comment.
The password change route exists at line 12, but the corresponding changePassword function in userController.js only destructures oldPassword and newPassword. The requirements state: 'require an old one, new password and confirmation'. The controller must also accept and validate that newPassword equals confirmation.
|
|
||
| export const register = async (email, password, name) => { | ||
| const hash = await bcrypt.hash(password, 10); | ||
|
|
||
| const user = await User.create({ | ||
| email, | ||
| password: hash, | ||
| name, | ||
| }); | ||
|
|
||
| return user; |
There was a problem hiding this comment.
The register function in authService.js creates a user but doesn't set activationToken or isActive: false. The User model defaults these, but explicit setting ensures the account starts inactive with a valid token for email verification.
|
|
||
| export const register = async (email, password, name) => { | ||
| const hash = await bcrypt.hash(password, 10); | ||
|
|
||
| const user = await User.create({ | ||
| email, | ||
| password: hash, | ||
| name, | ||
| }); | ||
|
|
||
| return user; |
There was a problem hiding this comment.
The register function doesn't validate password rules before hashing. The task requires: 'Inform the users about the rules for a password and check them'. Add validation for password complexity (min length, special chars, etc.).
|
|
||
| export const register = async (email, password, name) => { | ||
| const hash = await bcrypt.hash(password, 10); | ||
|
|
||
| const user = await User.create({ | ||
| email, | ||
| password: hash, | ||
| name, | ||
| }); | ||
|
|
||
| return user; |
There was a problem hiding this comment.
The register function doesn't call sendActivationEmail. After creating the user, this service should invoke the email service to send the activation link to the user's email.
|
|
||
| export const login = async (email, password) => { | ||
| const user = await User.findOne({ where: { email } }); | ||
|
|
||
| if (!user) { | ||
| throw new Error('Invalid credentials'); | ||
| } | ||
|
|
||
| const isMatch = await bcrypt.compare(password, user.password); | ||
|
|
||
| if (!isMatch) { | ||
| throw new Error('Invalid credentials'); | ||
| } | ||
|
|
||
| if (!user.isActive) { | ||
| throw new Error('Please activate your email'); | ||
| } | ||
|
|
||
| return generateTokens(user); |
There was a problem hiding this comment.
The login function only returns an access token via generateTokens(user). The authController.js needs both access and refresh tokens, plus user info. This service should return all three or be extended to include refresh token generation.
|
|
||
| const generateTokens = (user) => { | ||
| const access = jwt.sign({ id: user.id }, process.env.JWT_SECRET, { | ||
| expiresIn: '15m', | ||
| }); | ||
|
|
||
| return { access }; |
There was a problem hiding this comment.
The generateTokens function only generates access token. The task requirements and authController.js show that both access and refresh tokens should be generated. Should include refresh token generation here.
| import bcrypt from 'bcrypt'; | ||
| import jwt from 'jsonwebtoken'; | ||
| import { User } from '../models/User.js'; | ||
|
|
||
| const generateTokens = (user) => { | ||
| const access = jwt.sign({ id: user.id }, process.env.JWT_SECRET, { | ||
| expiresIn: '15m', | ||
| }); | ||
|
|
||
| return { access }; | ||
| }; | ||
|
|
||
| export const register = async (email, password, name) => { | ||
| const hash = await bcrypt.hash(password, 10); | ||
|
|
||
| const user = await User.create({ | ||
| email, | ||
| password: hash, | ||
| name, | ||
| }); | ||
|
|
||
| return user; | ||
| }; | ||
|
|
||
| export const login = async (email, password) => { | ||
| const user = await User.findOne({ where: { email } }); | ||
|
|
||
| if (!user) { | ||
| throw new Error('Invalid credentials'); | ||
| } | ||
|
|
||
| const isMatch = await bcrypt.compare(password, user.password); | ||
|
|
||
| if (!isMatch) { | ||
| throw new Error('Invalid credentials'); | ||
| } | ||
|
|
||
| if (!user.isActive) { | ||
| throw new Error('Please activate your email'); | ||
| } | ||
|
|
||
| return generateTokens(user); | ||
| }; |
There was a problem hiding this comment.
authService.js is missing password reset functions. The task requires: 'ask for an email, show email sent page, add Reset Password confirmation page, show Success page with a link to login'. Need functions like requestPasswordReset, resetPassword, etc.
| import bcrypt from 'bcrypt'; | ||
| import jwt from 'jsonwebtoken'; | ||
| import { User } from '../models/User.js'; | ||
|
|
||
| const generateTokens = (user) => { | ||
| const access = jwt.sign({ id: user.id }, process.env.JWT_SECRET, { | ||
| expiresIn: '15m', | ||
| }); | ||
|
|
||
| return { access }; | ||
| }; | ||
|
|
||
| export const register = async (email, password, name) => { | ||
| const hash = await bcrypt.hash(password, 10); | ||
|
|
||
| const user = await User.create({ | ||
| email, | ||
| password: hash, | ||
| name, | ||
| }); | ||
|
|
||
| return user; | ||
| }; | ||
|
|
||
| export const login = async (email, password) => { | ||
| const user = await User.findOne({ where: { email } }); | ||
|
|
||
| if (!user) { | ||
| throw new Error('Invalid credentials'); | ||
| } | ||
|
|
||
| const isMatch = await bcrypt.compare(password, user.password); | ||
|
|
||
| if (!isMatch) { | ||
| throw new Error('Invalid credentials'); | ||
| } | ||
|
|
||
| if (!user.isActive) { | ||
| throw new Error('Please activate your email'); | ||
| } | ||
|
|
||
| return generateTokens(user); | ||
| }; |
There was a problem hiding this comment.
authService.js is missing email change functionality. The task requires: 'To change an email you should type the password, confirm the new email and notify the old email about the change'. Need functions for email change workflow.
| export const register = async (email, password, name) => { | ||
| const hash = await bcrypt.hash(password, 10); | ||
|
|
||
| const user = await User.create({ | ||
| email, | ||
| password: hash, | ||
| name, | ||
| }); | ||
|
|
||
| return user; | ||
| }; | ||
|
|
||
| export const login = async (email, password) => { | ||
| const user = await User.findOne({ where: { email } }); | ||
|
|
||
| if (!user) { | ||
| throw new Error('Invalid credentials'); | ||
| } | ||
|
|
||
| const isMatch = await bcrypt.compare(password, user.password); | ||
|
|
There was a problem hiding this comment.
emailService.js only has sendActivationEmail. The task requires password reset functionality which needs sendPasswordResetEmail with a reset token link. Consider adding a new function for password reset emails.
| }); | ||
|
|
||
| export const sendActivationEmail = async (user) => { | ||
| try { | ||
| const link = `http://localhost:3000/activate/${user.email}/${user.activationToken}`; | ||
|
|
||
| const info = await transporter.sendMail({ | ||
| from: process.env.SMTP_USER, | ||
| to: user.email, | ||
| subject: 'Activate account', | ||
| html: ` |
There was a problem hiding this comment.
The register function in authService.js creates a user but doesn't set isActive: false or generate an activationToken. This means new users won't be inactive by default and won't have a token for email activation. Should set these fields and return the user with token for email sending.
| }); | ||
|
|
||
| export const sendActivationEmail = async (user) => { | ||
| try { | ||
| const link = `http://localhost:3000/activate/${user.email}/${user.activationToken}`; | ||
|
|
||
| const info = await transporter.sendMail({ | ||
| from: process.env.SMTP_USER, | ||
| to: user.email, | ||
| subject: 'Activate account', | ||
| html: ` |
There was a problem hiding this comment.
The register function doesn't call the email service to send the activation email. The task requires: 'send an activation email'. The emailService's sendActivationEmail function should be called here after user creation.
|
|
||
| export const sendActivationEmail = async (user) => { |
There was a problem hiding this comment.
The register function doesn't validate password rules as required by the task: 'Inform the users about the rules for a password and check them'. Should validate password complexity before hashing.
|
|
||
| export const sendActivationEmail = async (user) => { | ||
| try { | ||
| const link = `http://localhost:3000/activate/${user.email}/${user.activationToken}`; | ||
|
|
||
| const info = await transporter.sendMail({ | ||
| from: process.env.SMTP_USER, | ||
| to: user.email, | ||
| subject: 'Activate account', | ||
| html: ` | ||
| <h2>Activate your account</h2> | ||
| <p>Click the link below:</p> | ||
| <a href="${link}">${link}</a> | ||
| `, | ||
| }); | ||
|
|
||
| console.log('EMAIL SENT:', info.messageId); | ||
| } catch (err) { | ||
| console.error('EMAIL ERROR:', err); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The emailService only has sendActivationEmail but no password reset email function. A password reset email function is needed for the password reset flow (generate reset token, send email with reset link).
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation has several critical gaps that prevent the task requirements from being fully met. Password reset endpoints are unreachable - forgotPassword and resetPassword functions exist in authController.js but are not registered in authRoute.js (missing router.post('/forgot-password', forgotPassword) and router.post('/reset-password', resetPassword)). The changeEmail endpoint is inaccessible - the function exists in userController.js but there's no route defined in userRoute.js (add router.patch('/email', authMiddleware, changeEmail)). Additionally, the password confirmation field is not validated - your changePassword implementation extracts oldPassword and newPassword but the task explicitly requires "new password and confirmation" validation where both must match. The password validation is incomplete - it checks for length, uppercase, and numbers but the requirements state special characters should also be required. Finally, there's duplicated forgotPassword/resetPassword logic in both authController.js and userController.js - consolidate to one location to avoid confusion. These issues must be resolved before the task 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 changeEmail = async (req, res) => { | ||
| const { password, newEmail } = req.body; | ||
|
|
||
| const user = await User.findByPk(req.user.id); | ||
|
|
||
| const ok = await bcrypt.compare(password, user.password); | ||
|
|
||
| if (!ok) { | ||
| return res.status(400).json({ message: 'Wrong password' }); | ||
| } | ||
|
|
||
| const oldEmail = user.email; | ||
|
|
||
| user.email = newEmail; | ||
|
|
||
| await user.save(); | ||
|
|
||
| console.log(`Email changed from ${oldEmail}`); | ||
|
|
||
| res.json({ message: 'Email updated' }); | ||
| }; |
There was a problem hiding this comment.
The changeEmail function exists in userController.js but is NOT exposed via userRoute.js. The requirements state 'To change an email you should type the password, confirm the new email'. You need to add a route like router.patch('/email', authMiddleware, changeEmail);
| }); | ||
| }; | ||
|
|
||
| export const changePassword = async (req, res) => { |
There was a problem hiding this comment.
The changePassword function only extracts oldPassword and newPassword from req.body, but the requirements state it should also accept a confirmation field. This validation field is missing.
| if (!req.user?.id) { | ||
| return res.status(401).json({ message: 'Unauthorized' }); | ||
| } | ||
|
|
||
| const user = await User.findByPk(req.user.id); | ||
|
|
||
| if (!user) { | ||
| return res.status(404).json({ message: 'User not found' }); |
There was a problem hiding this comment.
The password validation checks for minimum length, uppercase, and numbers (lines 11-15), but the requirements state: 'Inform the users about the rules for a password and check them' which implies special characters should also be required.
| }); | ||
|
|
||
| if (!user) { | ||
| return res.status(400).json({ message: 'Invalid link' }); | ||
| } | ||
|
|
||
| user.isActive = true; | ||
| user.activationToken = null; | ||
|
|
||
| await user.save(); | ||
|
|
||
| const accessToken = jwt.sign({ id: user.id }, process.env.JWT_SECRET, { | ||
| expiresIn: '15m', | ||
| }); | ||
|
|
||
| res.json({ | ||
| accessToken, | ||
| user: { | ||
| id: user.id, | ||
| email: user.email, | ||
| name: user.name, | ||
| }, | ||
| }); | ||
| }; | ||
|
|
||
| // LOGIN | ||
| export const login = async (req, res) => { | ||
| const { email, password } = req.body; |
There was a problem hiding this comment.
The changePassword function requires old password verification but never validates that newPassword and confirmation match. The requirements explicitly state: 'change a password (require an old one, new password and confirmation)'. You must add confirmation field validation.
| export const refresh = async (req, res) => { | ||
| try { | ||
| const refreshToken = req.cookies.refreshToken; | ||
|
|
||
| if (!refreshToken) { | ||
| return res.status(401).json({ message: 'No refresh token' }); | ||
| } | ||
|
|
||
| const data = jwt.verify(refreshToken, process.env.JWT_REFRESH_SECRET); | ||
|
|
||
| const accessToken = jwt.sign({ id: data.id }, process.env.JWT_SECRET, { | ||
| expiresIn: '15m', | ||
| }); | ||
|
|
||
| res.json({ accessToken }); | ||
| } catch (e) { | ||
| res.status(401).json({ message: 'Invalid refresh token' }); | ||
| } | ||
| }; | ||
|
|
||
| // LOGOUT |
There was a problem hiding this comment.
The changeEmail endpoint exists in this controller but is never exposed via any route. Add router.patch('/email', authMiddleware, changeEmail); to userRoute.js to make this functionality accessible.
| /* eslint-disable padding-line-between-statements */ | ||
| import jwt from 'jsonwebtoken'; | ||
|
|
||
| export const authMiddleware = (req, res, next) => { | ||
| const token = req.headers.authorization?.split(' ')[1]; | ||
|
|
||
| if (!token) { | ||
| return res.sendStatus(401); | ||
| } | ||
|
|
||
| try { | ||
| const data = jwt.verify(token, process.env.JWT_SECRET); | ||
| req.user = data; | ||
| next(); | ||
| } catch { |
There was a problem hiding this comment.
The changeEmail function exists in userController.js but is not exposed via any route in userRoute.js. Add router.patch('/email', authMiddleware, changeEmail); to make this endpoint accessible.
| import bcrypt from 'bcrypt'; | ||
| import jwt from 'jsonwebtoken'; | ||
| import { User } from '../models/User.js'; | ||
|
|
||
| const generateTokens = (user) => { | ||
| const access = jwt.sign({ id: user.id }, process.env.JWT_SECRET, { | ||
| expiresIn: '15m', | ||
| }); | ||
|
|
||
| return { access }; | ||
| }; | ||
|
|
||
| export const register = async (email, password, name) => { | ||
| const hash = await bcrypt.hash(password, 10); | ||
|
|
||
| const user = await User.create({ | ||
| email, | ||
| password: hash, |
There was a problem hiding this comment.
The forgotPassword route exists in authController.js (line 143) but is not defined in authRoute.js. Add router.post('/forgot-password', forgotPassword);
| import bcrypt from 'bcrypt'; | ||
| import jwt from 'jsonwebtoken'; | ||
| import { User } from '../models/User.js'; | ||
|
|
||
| const generateTokens = (user) => { | ||
| const access = jwt.sign({ id: user.id }, process.env.JWT_SECRET, { | ||
| expiresIn: '15m', | ||
| }); | ||
|
|
||
| return { access }; | ||
| }; | ||
|
|
||
| export const register = async (email, password, name) => { | ||
| const hash = await bcrypt.hash(password, 10); | ||
|
|
||
| const user = await User.create({ | ||
| email, | ||
| password: hash, |
There was a problem hiding this comment.
The resetPassword route exists in authController.js (line 160) but is not defined in authRoute.js. Add router.post('/reset-password', resetPassword);
| import bcrypt from 'bcrypt'; | ||
| import jwt from 'jsonwebtoken'; | ||
| import { User } from '../models/User.js'; | ||
|
|
||
| const generateTokens = (user) => { | ||
| const access = jwt.sign({ id: user.id }, process.env.JWT_SECRET, { | ||
| expiresIn: '15m', | ||
| }); | ||
|
|
||
| return { access }; | ||
| }; | ||
|
|
||
| export const register = async (email, password, name) => { | ||
| const hash = await bcrypt.hash(password, 10); | ||
|
|
There was a problem hiding this comment.
The changeEmail function exists in userController.js (line 117) but is not exposed via any route in userRoute.js. Add the import and router.patch('/email', authMiddleware, changeEmail);
| /* eslint-disable no-console */ | ||
| import nodemailer from 'nodemailer'; | ||
|
|
||
| const transporter = nodemailer.createTransport({ | ||
| host: process.env.SMTP_HOST, | ||
| port: Number(process.env.SMTP_PORT), | ||
| secure: true, | ||
| auth: { | ||
| user: process.env.SMTP_USER, | ||
| pass: process.env.SMTP_PASSWORD, | ||
| }, | ||
| }); | ||
|
|
||
| export const sendActivationEmail = async (user) => { | ||
| const link = `http://localhost:5173/activate/${user.activationToken}`; |
There was a problem hiding this comment.
The changeEmail function exists in userController.js but is not exposed via any route in userRoute.js. Add the import and router.patch('/email', authMiddleware, changeEmail); to make this endpoint accessible.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This is an educational approval to support your learning progress. Your backend authentication system demonstrates solid understanding of core auth flows—registration, activation, login, and profile management are all well-implemented. The routes are properly organized with authMiddleware protecting most endpoints, which is good practice. However, there are three areas worth strengthening: First, add authMiddleware to your logout route in userRoute.js (it should be router.post('/logout', authMiddleware, logout) to ensure only authenticated users can logout as specified). Second, apply the same password complexity validation you use during registration—checking for uppercase letters, numbers, and special characters—to both resetPassword and changePassword functions (currently they only validate length). Third, make sure your forgotPassword function actually sends emails using your existing sendResetEmail service rather than just logging the URL. These security gaps are common learning points and addressing them will make your authentication system more robust. Keep building on this solid foundation!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| router.patch('/password', authMiddleware, changePassword); | ||
| router.patch('/email', authMiddleware, changeEmail); | ||
|
|
||
| export const userRouter = router; |
There was a problem hiding this comment.
The logout endpoint should be protected with authMiddleware to ensure only authenticated users can logout. Add authMiddleware as the first argument: router.post('/logout', authMiddleware, logout). This is required by the task specification 'Logout (authenticated users only)'.
No description provided.