solution#280
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The implementation has multiple critical blockers that prevent meeting the task requirements. Missing 'name' field: Registration requires name, email, and password per the task, but the User model and register function only handle email/password. Password reset entirely missing: No endpoints exist for email submission, reset confirmation page, or success page. Profile page not implemented: No routes/controllers/services for authenticated profile management (change name, change password with old/new/confirmation, change email with password verification and old email notification). No 404 handler: Missing catch-all route handler for undefined routes as explicitly required. Login activation check missing: Code doesn't verify user.activationToken === null before allowing login - inactive users should be prompted to activate. Auth middleware bug: req.user is never attached to the request, breaking downstream authenticated user access. ErrorMiddleware bug: Missing return statement causes ApiError responses to be overwritten with 500 errors. These issues must be addressed before approval.
✨ 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 'dotenv/config'; | ||
| import express from 'express'; | ||
| import cors from 'cors'; | ||
| import { authRouter } from './routes/auth.route.js'; | ||
| import { userRouter } from './routes/user.route.js'; | ||
| import { errorMiddleware } from './middlewares/errorMiddleware.js'; | ||
| import cookieParse from 'cookie-parser'; | ||
|
|
||
| const PORT = process.env.PORT || 3005; | ||
|
|
||
| export const app = express(); | ||
| app.use(express.json()); | ||
| app.use(cookieParse()); | ||
|
|
||
| app.use( | ||
| cors({ | ||
| origin: process.env.CLIENT_HOST, | ||
| credentials: true, |
There was a problem hiding this comment.
Missing password reset routes: (1) request reset form, (2) email sent confirmation, (3) reset password page with password/confirmation validation, (4) success page. None of these are implemented.
| import 'dotenv/config'; | ||
| import express from 'express'; | ||
| import cors from 'cors'; | ||
| import { authRouter } from './routes/auth.route.js'; | ||
| import { userRouter } from './routes/user.route.js'; | ||
| import { errorMiddleware } from './middlewares/errorMiddleware.js'; | ||
| import cookieParse from 'cookie-parser'; |
There was a problem hiding this comment.
Missing profile routes: (1) get profile, (2) change name, (3) change password (requires old password, new password, confirmation), (4) change email (requires password, new email confirmation, notify old email).
| import 'dotenv/config'; | ||
| import express from 'express'; | ||
| import cors from 'cors'; | ||
| import { authRouter } from './routes/auth.route.js'; | ||
| import { userRouter } from './routes/user.route.js'; | ||
| import { errorMiddleware } from './middlewares/errorMiddleware.js'; | ||
| import cookieParse from 'cookie-parser'; | ||
|
|
||
| const PORT = process.env.PORT || 3005; | ||
|
|
||
| export const app = express(); | ||
| app.use(express.json()); | ||
| app.use(cookieParse()); | ||
|
|
||
| app.use( | ||
| cors({ | ||
| origin: process.env.CLIENT_HOST, | ||
| credentials: true, | ||
| }), | ||
| ); | ||
| app.use(authRouter); | ||
|
|
||
| app.use('/users', userRouter); | ||
| app.use(errorMiddleware); | ||
| app.listen(PORT); |
There was a problem hiding this comment.
No 404 middleware is implemented to handle undefined routes as required by the task ('404 for all other pages').
| } catch (error) { | ||
| console.error('Unable to connect to the database:', error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The task requires registration with 'name, email and password', but only email and password are being captured from req.body. The User model may also need a 'name' field.
| import { User } from '../models/user.js'; | ||
| import { emailService } from '../services/email.service.js'; | ||
| import { userService } from '../services/user.service.js'; | ||
| import { jwtService } from '../services/jwt.service.js'; | ||
| import { ApiError } from '../exceptions/api.error.js'; | ||
| import bcrypt from 'bcrypt'; | ||
| import { tokenService } from '../services/token.service.js'; | ||
|
|
||
| function validateEmail(value) { | ||
| const EMAIL_PATTERN = /^[\w.+-]+@([\w-]+\.){1,3}[\w-]{2,}$/; | ||
|
|
||
| if (!value) return 'Email is required'; | ||
| if (!EMAIL_PATTERN.test(value)) return 'Email is not valid'; | ||
| } | ||
| function validatePassword(value) { | ||
| if (!value) return 'Password is required'; | ||
| if (value.length < 6) return 'At least 6 characters'; | ||
| } | ||
|
|
There was a problem hiding this comment.
User model is missing the required 'name' field. The requirements state: 'Register using name, email and password'.
| const register = async (req, res) => { | ||
| const { email, password } = req.body; |
There was a problem hiding this comment.
Registration only accepts email and password, but requirements specify 'name, email and password'. The 'name' field is missing from destructuring.
|
|
||
| const login = async (req, res) => { | ||
| const { email, password } = req.body; | ||
| const user = await userService.findByEmail(email); | ||
|
|
||
| if (!user) { | ||
| throw ApiError.badRequest('No such user'); | ||
| } | ||
|
|
||
| const isPasswordValid = bcrypt.compare(password, user.password); | ||
|
|
||
| if (!isPasswordValid) { | ||
| throw ApiError.badRequest('Wrong password'); | ||
| } | ||
|
|
||
| generateTokens(res, user); |
There was a problem hiding this comment.
Login doesn't check if the user is active. Requirements state: 'If user is not active ask them to activate their email'.
|
|
||
| const activate = async (req, res) => { | ||
| const { activationToken } = req.params; | ||
| const user = await User.findOne({ where: { activationToken } }); | ||
|
|
||
| if (!user) { | ||
| return res.status(404).json({ | ||
| message: | ||
| 'Користувача з таким токеном не знайдено або він вже активований', | ||
| }); | ||
| } | ||
|
|
||
| user.activationToken = null; | ||
|
|
||
| await user.save(); | ||
| res.status(200).send(user); |
There was a problem hiding this comment.
Activation doesn't redirect to Profile page as required. It only sends user data back.
| import { User } from '../models/user.js'; | ||
| import { emailService } from '../services/email.service.js'; | ||
| import { userService } from '../services/user.service.js'; | ||
| import { jwtService } from '../services/jwt.service.js'; | ||
| import { ApiError } from '../exceptions/api.error.js'; | ||
| import bcrypt from 'bcrypt'; | ||
| import { tokenService } from '../services/token.service.js'; | ||
|
|
||
| function validateEmail(value) { | ||
| const EMAIL_PATTERN = /^[\w.+-]+@([\w-]+\.){1,3}[\w-]{2,}$/; | ||
|
|
||
| if (!value) return 'Email is required'; | ||
| if (!EMAIL_PATTERN.test(value)) return 'Email is not valid'; | ||
| } | ||
| function validatePassword(value) { | ||
| if (!value) return 'Password is required'; | ||
| if (value.length < 6) return 'At least 6 characters'; | ||
| } | ||
|
|
||
| const register = async (req, res) => { | ||
| const { email, password } = req.body; | ||
|
|
||
| const errors = { | ||
| email: validateEmail(email), |
There was a problem hiding this comment.
User model is missing the required 'name' field. This field is needed for registration as specified in requirements.
| import { User } from '../models/user.js'; | ||
| import { emailService } from '../services/email.service.js'; | ||
| import { userService } from '../services/user.service.js'; | ||
| import { jwtService } from '../services/jwt.service.js'; | ||
| import { ApiError } from '../exceptions/api.error.js'; | ||
| import bcrypt from 'bcrypt'; | ||
| import { tokenService } from '../services/token.service.js'; | ||
|
|
||
| function validateEmail(value) { | ||
| const EMAIL_PATTERN = /^[\w.+-]+@([\w-]+\.){1,3}[\w-]{2,}$/; | ||
|
|
||
| if (!value) return 'Email is required'; | ||
| if (!EMAIL_PATTERN.test(value)) return 'Email is not valid'; | ||
| } | ||
| function validatePassword(value) { | ||
| if (!value) return 'Password is required'; | ||
| if (value.length < 6) return 'At least 6 characters'; | ||
| } | ||
|
|
||
| const register = async (req, res) => { | ||
| const { email, password } = req.body; | ||
|
|
||
| const errors = { | ||
| email: validateEmail(email), | ||
| password: validatePassword(password), | ||
| }; | ||
|
|
||
| if (errors.email || errors.password) { | ||
| throw ApiError.badRequest('Bad request', errors); | ||
| } | ||
|
|
||
| const hashedPass = await bcrypt.hash(password, 10); | ||
|
|
||
| await userService.register(email, hashedPass); | ||
| res.send({ message: 'OK' }); | ||
| }; | ||
|
|
||
| const activate = async (req, res) => { | ||
| const { activationToken } = req.params; | ||
| const user = await User.findOne({ where: { activationToken } }); | ||
|
|
||
| if (!user) { | ||
| return res.status(404).json({ |
There was a problem hiding this comment.
User service is missing update methods needed for profile page: updateName, updatePassword, updateEmail. These are required by the profile page requirements.
| import { User } from '../models/user.js'; | ||
| import { emailService } from '../services/email.service.js'; | ||
| import { userService } from '../services/user.service.js'; | ||
| import { jwtService } from '../services/jwt.service.js'; | ||
| import { ApiError } from '../exceptions/api.error.js'; | ||
| import bcrypt from 'bcrypt'; | ||
| import { tokenService } from '../services/token.service.js'; | ||
|
|
||
| function validateEmail(value) { | ||
| const EMAIL_PATTERN = /^[\w.+-]+@([\w-]+\.){1,3}[\w-]{2,}$/; | ||
|
|
||
| if (!value) return 'Email is required'; | ||
| if (!EMAIL_PATTERN.test(value)) return 'Email is not valid'; | ||
| } | ||
| function validatePassword(value) { | ||
| if (!value) return 'Password is required'; | ||
| if (value.length < 6) return 'At least 6 characters'; | ||
| } | ||
|
|
There was a problem hiding this comment.
No routes defined for password reset flow (email submission, reset confirmation, success page).
| import { User } from '../models/user.js'; | ||
| import { emailService } from '../services/email.service.js'; | ||
| import { userService } from '../services/user.service.js'; | ||
| import { jwtService } from '../services/jwt.service.js'; | ||
| import { ApiError } from '../exceptions/api.error.js'; | ||
| import bcrypt from 'bcrypt'; | ||
| import { tokenService } from '../services/token.service.js'; | ||
|
|
||
| function validateEmail(value) { | ||
| const EMAIL_PATTERN = /^[\w.+-]+@([\w-]+\.){1,3}[\w-]{2,}$/; |
There was a problem hiding this comment.
No routes defined for profile page operations (get profile, update name, update password, update email).
| email: validateEmail(email), | ||
| password: validatePassword(password), |
There was a problem hiding this comment.
No 404 handler implemented for undefined routes. Requirements specify: '404 for all the other pages'.
| } | ||
| function validatePassword(value) { | ||
| if (!value) return 'Password is required'; | ||
| if (value.length < 6) return 'At least 6 characters'; | ||
| } | ||
|
|
||
| const register = async (req, res) => { | ||
| const { email, password } = req.body; | ||
|
|
||
| const errors = { | ||
| email: validateEmail(email), |
There was a problem hiding this comment.
Auth routes (registration, activation, login) are not protected to be accessible only by non-authenticated users. Requirements specify these should work 'only non authenticated'.
| export class ApiError extends Error { | ||
| constructor({ message, status, errors = {} }) { | ||
| super(message); | ||
|
|
||
| this.status = status; | ||
| this.errors = errors; | ||
| } | ||
|
|
||
| static badRequest(message, errors) { | ||
| return new ApiError({ | ||
| message, | ||
| errors, | ||
| status: 400, |
There was a problem hiding this comment.
The user.controller.js only has getAllActivated which lists all activated users. This does NOT implement the required 'Profile page (only authenticated)' functionality. The following are required but missing: (1) get current user profile, (2) change name, (3) change password with old password + new password + confirmation, (4) change email with password verification + new email confirmation + notify old email.
|
|
||
| export const userController = { | ||
| getAllActivated, |
There was a problem hiding this comment.
The user.controller.js only implements getAllActivated, but the task requires a Profile page with: display profile, change name, change password (old password + new password + confirmation), and change email (password + new email confirmation + notify old email). None of these profile management functions are implemented.
| import { jwtService } from '../services/jwt.service.js'; | ||
|
|
||
| export const authMiddleware = (req, res, next) => { | ||
| const authorization = req.headers['authorization'] || ''; | ||
| const [, token] = authorization.split(' '); | ||
|
|
||
| if (!authorization || !token) { | ||
| res.sendStatus(401); | ||
| return; | ||
| } | ||
|
|
||
| const userData = jwtService.verify(token); | ||
| if (!userData) { | ||
| res.sendStatus(401); |
There was a problem hiding this comment.
The user.controller.js only contains getAllActivated, which is not a profile feature. All required profile page functionality is missing: (1) getProfile - display authenticated user's profile, (2) updateName - allow changing name, (3) updatePassword - allow changing password with required old password, new password, and confirmation fields, (4) updateEmail - allow changing email with password verification, new email confirmation, and notification to the old email address.
| export const authRouter = express.Router(); | ||
|
|
||
| authRouter.post('/registration', catchError(authController.register)); | ||
| authRouter.get( | ||
| '/activation/:email/:activationToken', | ||
| catchError(authController.activate), | ||
| ); | ||
| authRouter.post('/login', catchError(authController.login)); | ||
| authRouter.get('/refresh', catchError(authController.refresh)); | ||
| authRouter.post('/logout', catchError(authController.logout)); |
There was a problem hiding this comment.
Missing password reset routes. The task requires: (1) route to display email input form, (2) route to send reset email and show 'email sent' page, (3) route for reset password confirmation page with password/confirmation validation, (4) route for success page with link to login.
| import { ApiError } from '../exceptions/api.error.js'; | ||
|
|
||
| export const errorMiddleware = (error, req, res, next) => { | ||
| if (error instanceof ApiError) { | ||
| res.status(error.status).send({ | ||
| message: error.message, | ||
| errors: error.errors, | ||
| }); | ||
| } | ||
| res.statusCode = 500; | ||
| res.send({ | ||
| message: 'Server error', | ||
| }); | ||
| }; |
There was a problem hiding this comment.
The auth.route.js is missing all password reset endpoints. The task requires: (1) email input form route, (2) email sent confirmation route, (3) reset password confirmation route with password/confirmation validation, (4) success page route. None of these are implemented.
|
|
||
| export const User = client.define('user', { | ||
| 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.
The User model is missing the required name field. The task specifies 'Register using name, email and password', but the model only has email, password, and activationToken fields.
| import { DataTypes } from 'sequelize'; | ||
| import { client } from '../utils/db.js'; | ||
|
|
||
| export const User = client.define('user', { | ||
| email: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, | ||
| unique: true, | ||
| }, | ||
| password: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, | ||
| }, | ||
| activationToken: { |
There was a problem hiding this comment.
The auth routes are missing password reset functionality. The task requires: (1) email input form endpoint, (2) reset password confirmation endpoint (with password and confirmation fields that must be equal), (3) success page endpoint. These routes are not defined.
|
|
||
| export const Token = client.define('token', { | ||
| refreshToken: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, | ||
| }, | ||
| }); | ||
|
|
||
| Token.belongsTo(User); |
There was a problem hiding this comment.
When an ApiError is caught (lines 4-8), the response is sent but execution continues to lines 9-12, which overwrites the response with a 500 error. Add return after line 7 or else before line 9 to prevent this.
|
|
||
| userRouter.get('/', authMiddleware, catchError(userController.getAllActivated)); |
There was a problem hiding this comment.
The user.route.js only has GET '/' for listing users. It's missing all profile routes for authenticated users: (1) GET '/profile' - get current user profile, (2) PATCH '/profile/name' - change name, (3) PATCH '/profile/password' - change password (old password, new password, confirmation), (4) PATCH '/profile/email' - change email (password, new email, confirmation, notify old email).
| import 'dotenv/config'; | ||
| import nodemailer from 'nodemailer'; | ||
|
|
||
| const transporter = nodemailer.createTransport({ | ||
| host: process.env.SMTP_HOST, | ||
| port: process.env.SMTP_PORT, | ||
| auth: { | ||
| user: process.env.SMTP_USER, |
There was a problem hiding this comment.
The user routes only have one endpoint for getting all activated users. The task requires a Profile page (only authenticated) with routes for: (1) display profile, (2) change name, (3) change password (with old password, new password, confirmation), (4) change email (with password, new email confirmation, notify old email). These routes are completely missing.
|
|
||
| export const emailService = { | ||
| sendActivationEmail, | ||
| send, |
There was a problem hiding this comment.
The email service is missing functions required for password reset and email change flows: (1) sendPasswordResetEmail - needed for password reset workflow, (2) sendEmailChangeNotification - needed to notify old email address when email is changed.
| const transporter = nodemailer.createTransport({ | ||
| host: process.env.SMTP_HOST, |
There was a problem hiding this comment.
The access token has a 5-second expiration time which is extremely short. While not explicitly forbidden by the task, this will cause the token to expire almost immediately during normal usage, leading to frequent authentication failures.
| expiresIn: '5s', | ||
| }); | ||
|
|
||
| return token; |
There was a problem hiding this comment.
The user.route.js only has a route to get all activated users (/users). The task requires a 'Profile page (only authenticated)' with: (1) get profile, (2) update name, (3) update password (old password + new password + confirmation), (4) update email (password + new email confirmation + notify old email). None of these profile routes are implemented.
| verify, | ||
| verifyRefresh, | ||
| signRefresh, | ||
| }; |
There was a problem hiding this comment.
The emailService is missing a function to send email change notifications. The task requires 'notify the old email address about the change' when the user's email is changed. A function like sendEmailChangeNotification(to: oldEmail, newEmail) is needed.
| import { Token } from '../models/token.js'; | ||
|
|
||
| async function save(userId, newToken) { | ||
| if (!userId) { | ||
| throw new Error( | ||
| 'tokenService.save: Очікувався userId, але отримано undefined або null', | ||
| ); | ||
| } |
There was a problem hiding this comment.
The user.route.js only has one route (GET /) which lists all users. It's missing all required profile page routes: (1) GET /profile - display profile, (2) PATCH /profile/name - change name, (3) PATCH /profile/password - change password with old password + new password + confirmation, (4) PATCH /profile/email - change email with password verification + new email confirmation + notify old email.
|
|
||
| export const tokenService = { | ||
| save, | ||
| getByToken, |
There was a problem hiding this comment.
The emailService is missing password reset email function. The task requires a password reset flow that sends an email to the user with a reset link.
| if (!userId) { | ||
| throw new Error( |
There was a problem hiding this comment.
The access token expires in 5 seconds (expiresIn: '5s'), which is impractically short. Users will need to refresh their tokens almost constantly, causing potential UX issues.
| username: POSTGRES_USER || 'postgres', | ||
| host: POSTGRES_HOST || 'localhost', | ||
| dialect: 'postgres', |
There was a problem hiding this comment.
The normalize function only returns {id, email}, but should also include 'name' field since the task requires registration with name and profile page should display the name.
|
|
||
| async function register(email, password) { | ||
| const activationToken = uuidv4(); | ||
|
|
||
| const existUser = await findByEmail(email); | ||
|
|
||
| if (existUser) { | ||
| throw ApiError.badRequest('User already exist', { | ||
| email: 'User already exist', | ||
| }); | ||
| } | ||
|
|
||
| await User.create({ email, password, activationToken }); | ||
|
|
||
| await emailService.sendActivationEmail(email, activationToken); |
There was a problem hiding this comment.
The register function only accepts email and password parameters, but the task requires 'name, email and password'. The name parameter should be added to the function signature and stored when creating the user.
|
|
||
| function normalize({ id, email }) { | ||
| return { id, email }; |
There was a problem hiding this comment.
The normalize function returns only {id, email}, but should also return 'name' field since the User model needs a name field for registration.
|
|
||
| export const userService = { | ||
| getAllActivated, | ||
| normalize, | ||
| findByEmail, | ||
| register, |
There was a problem hiding this comment.
The userService is missing profile management functions required by the task: (1) updateName(userId, name) - change name, (2) updatePassword(userId, oldPassword, newPassword, confirmation) - change password with validation, (3) updateEmail(userId, password, newEmail, confirmation) - change email with password verification and old email notification, (4) getById(userId) - get user by ID for profile display.
|
|
||
| export const userService = { | ||
| getAllActivated, | ||
| normalize, | ||
| findByEmail, | ||
| register, |
There was a problem hiding this comment.
The userService is missing password reset functions required by the task: (1) generateResetToken(email) - generate token and send reset email, (2) findByResetToken(resetToken) - find user by reset token, (3) resetPassword(resetToken, newPassword, confirmation) - validate password/confirmation match and update password.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The implementation has critical blockers that prevent core functionality from working. Password reset is completely missing - no endpoints exist for forgot password, email sent confirmation, reset password page, or success page. The userService.activate() function is called in auth.controller.js line 41 but is not implemented in user.service.js, which will cause a runtime error. The bcrypt.compare() on line 65 of auth.controller.js is async but not awaited, making password validation broken. The errorMiddleware.js is missing a return statement after handling ApiError, causing all error responses to be overwritten with 500 errors. Additionally, the updateEmail function in user.service.js captures oldEmail but never sends a notification to it as required.
These issues must be fixed before the application can meet basic 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
| const activate = async (req, res, next) => { | ||
| try { | ||
| const activationToken = req.params.activationToken; | ||
|
|
There was a problem hiding this comment.
The userService.activate() function is called here but does not exist in user.service.js - this will cause a runtime error 'activate is not a function' when activating a user.
|
|
||
| const register = async (req, res) => { | ||
| const { email, name, password } = req.body; | ||
|
|
There was a problem hiding this comment.
Missing return statement causes the response to continue executing, overwriting the ApiError response with a 500 error on the next line.
| import { User } from '../models/user.js'; | ||
| import { emailService } from '../services/email.service.js'; | ||
| import { userService } from '../services/user.service.js'; | ||
| import { jwtService } from '../services/jwt.service.js'; | ||
| import { ApiError } from '../exceptions/api.error.js'; | ||
| import bcrypt from 'bcrypt'; | ||
| import { tokenService } from '../services/token.service.js'; | ||
|
|
||
| function validateEmail(value) { | ||
| const EMAIL_PATTERN = /^[\w.+-]+@([\w-]+\.){1,3}[\w-]{2,}$/; | ||
|
|
||
| if (!value) return 'Email is required'; | ||
| if (!EMAIL_PATTERN.test(value)) return 'Email is not valid'; | ||
| } | ||
| function validatePassword(value) { | ||
| if (!value) return 'Password is required'; | ||
| if (value.length < 6) return 'At least 6 characters'; | ||
| } | ||
|
|
||
| const register = async (req, res) => { | ||
| const { email, name, password } = req.body; | ||
|
|
||
| const errors = { | ||
| email: validateEmail(email), | ||
| password: validatePassword(password), | ||
| }; | ||
|
|
||
| if (errors.email || errors.password) { | ||
| throw ApiError.badRequest('Bad request', errors); | ||
| } | ||
|
|
||
| const hashedPass = await bcrypt.hash(password, 10); | ||
|
|
||
| await userService.register(email, hashedPass, name); | ||
| res.send({ message: 'OK' }); | ||
| }; | ||
|
|
||
| const activate = async (req, res, next) => { | ||
| try { | ||
| const activationToken = req.params.activationToken; | ||
|
|
||
| await userService.activate(activationToken); | ||
|
|
||
| const clientUrl = 'http://localhost:5173'; | ||
|
|
||
| return res.redirect(`${clientUrl}/profile`); | ||
| } catch (e) { | ||
| next(e); | ||
| } | ||
| }; | ||
|
|
||
| const login = async (req, res) => { | ||
| const { email, password } = req.body; | ||
| const user = await userService.findByEmail(email); | ||
|
|
||
| if (!user) { | ||
| throw ApiError.badRequest('No such user'); | ||
| } | ||
|
|
||
| if (user.activationToken !== null) { | ||
| throw ApiError.badRequest( | ||
| 'If user is not active ask them to activate their email', | ||
| ); | ||
| } | ||
|
|
||
| const isPasswordValid = bcrypt.compare(password, user.password); | ||
|
|
||
| if (!isPasswordValid) { | ||
| throw ApiError.badRequest('Wrong password'); | ||
| } | ||
|
|
||
| generateTokens(res, user); | ||
| }; | ||
|
|
||
| async function generateTokens(res, user) { | ||
| const normalizedUser = userService.normalize(user); | ||
| const accessToken = jwtService.sign(normalizedUser); | ||
| const refreshAccessToken = jwtService.signRefresh(normalizedUser); | ||
|
|
||
| await tokenService.save(normalizedUser.id, refreshAccessToken); | ||
|
|
||
| res.cookie('refreshToken', refreshAccessToken, { | ||
| HttpOnly: true, | ||
| maxAge: 30 * 24 * 60 * 60 * 1000, | ||
| }); | ||
|
|
||
| res.send({ | ||
| user: normalizedUser, | ||
| accessToken, | ||
| }); | ||
| } | ||
|
|
||
| const refresh = async (req, res) => { | ||
| const { refreshToken } = req.cookies; | ||
|
|
||
| const userData = await jwtService.verifyRefresh(refreshToken); | ||
| const token = await tokenService.getByToken(refreshToken); | ||
|
|
||
| if (!userData || !token) { | ||
| throw ApiError.unauthorized(); | ||
| } | ||
|
|
||
| const user = await userService.findByEmail(userData.email); | ||
|
|
||
| if (!user) { | ||
| throw ApiError.unauthorized(); | ||
| } | ||
|
|
||
| generateTokens(res, user); | ||
| }; | ||
|
|
||
| const logout = async (req, res) => { | ||
| const { refreshToken } = req.cookies; | ||
| const userData = await jwtService.verifyRefresh(refreshToken); | ||
|
|
||
| if (!userData || !refreshToken) { | ||
| throw ApiError.unauthorized(); | ||
| return; | ||
| } | ||
|
|
||
| await tokenService.remove(userData.id); | ||
| res.sendStatus(204); | ||
| }; | ||
|
|
||
| export const authController = { | ||
| register, | ||
| activate, | ||
| login, | ||
| refresh, | ||
| logout, |
There was a problem hiding this comment.
The password reset feature is entirely missing from the application. According to requirements, the app needs: (1) a route/endpoint to request password reset by email, (2) an email sent confirmation page endpoint, (3) a reset password confirmation page with password/confirmation fields that must be equal, (4) a success page with link to login.
| const refreshAccessToken = jwtService.signRefresh(normalizedUser); | ||
|
|
||
| await tokenService.save(normalizedUser.id, refreshAccessToken); | ||
|
|
||
| res.cookie('refreshToken', refreshAccessToken, { | ||
| HttpOnly: true, | ||
| maxAge: 30 * 24 * 60 * 60 * 1000, | ||
| }); | ||
|
|
||
| res.send({ | ||
| user: normalizedUser, | ||
| accessToken, | ||
| }); | ||
| } | ||
|
|
||
| const refresh = async (req, res) => { | ||
| const { refreshToken } = req.cookies; | ||
|
|
||
| const userData = await jwtService.verifyRefresh(refreshToken); |
There was a problem hiding this comment.
When email is changed, the old email address should be notified about this change. The function updates the email and activationToken but does not notify the old email address as required by the task.
| const { newEmail, password } = req.body; | ||
| const userId = req.user.id; | ||
|
|
||
| await userService.updateEmail(userId, newEmail, password); |
There was a problem hiding this comment.
Missing semicolon at the end of the return statement. While JavaScript allows this, it's better practice to include it consistently.
| } | ||
|
|
||
| static badRequest(message, errors) { | ||
| return new ApiError({ |
There was a problem hiding this comment.
The console.log only outputs the PORT environment variable, which would just be undefined or a number. Consider logging a more useful message like Server running on port ${PORT} or the full URL to help with development.
|
|
||
| app.listen(PORT, () => { | ||
| // eslint-disable-next-line no-console | ||
| console.log(`Go to URL: ${process.env.PORT}`); |
There was a problem hiding this comment.
The console.log logs process.env.PORT which will be undefined if PORT is not set in environment variables. Should log PORT (the local variable) or the full URL. Also, logging only the port without a scheme/host isn't very useful for users.
| if (!authorization || !token) { | ||
| res.sendStatus(401); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Bug: process.env.PORT will always be undefined in the console.log because the environment variable was assigned to the local PORT variable on line 5. Should be console.log(Go to URL: ${PORT})
| if (error instanceof ApiError) { | ||
| res.status(error.status).send({ | ||
| message: error.message, | ||
| errors: error.errors, | ||
| }); |
There was a problem hiding this comment.
CRITICAL BUG: Missing return after handling ApiError. The code on lines 4-8 sends the correct ApiError response, but then execution continues to line 9 which sends a 500 error, overwriting the ApiError response. Add return before res.status(error.status) or restructure with if/else.
|
|
||
| export const Token = client.define('token', { | ||
| refreshToken: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, |
There was a problem hiding this comment.
Critical bug: After sending the ApiError response (lines 4-7), execution continues to line 9 which overwrites the response with a 500 error. Add return before res.status(error.status) or use else to prevent this. Correct: return res.status(error.status).send(...) or } else { return res.status(500)... }
| export const User = client.define('user', { | ||
| email: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, | ||
| unique: true, |
There was a problem hiding this comment.
CRITICAL BUG: After sending the ApiError response (lines 4-8), the code continues to line 9 and sends a 500 error response, overwriting the original ApiError response. Add return before res.status(error.status) or use else to prevent this. Current behavior: all ApiErrors become 500 errors.
| import { DataTypes } from 'sequelize'; | ||
| import { client } from '../utils/db.js'; | ||
|
|
||
| export const User = client.define('user', { | ||
| email: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, | ||
| unique: true, | ||
| }, | ||
| password: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, | ||
| }, | ||
| name: { |
There was a problem hiding this comment.
Password reset routes are completely missing from auth.route.js. The task requires unauthenticated users to: (1) request password reset via email, (2) see email sent confirmation, (3) access reset password page with password/confirmation fields, (4) see success page with login link. Add routes like: POST /forgot-password, GET /reset-password/:token, POST /reset-password, GET /reset-password/success.
|
|
||
| export const authRouter = express.Router(); | ||
|
|
||
| authRouter.post('/registration', catchError(authController.register)); | ||
| authRouter.get( |
There was a problem hiding this comment.
Critical bug: Missing return statement. After sending ApiError response (lines 4-8), execution continues to line 9 which sends a 500 error, overwriting the ApiError response. Add return before res.status(error.status) on line 4.
| import express from 'express'; | ||
| import { authController } from '../controllers/auth.controller.js'; | ||
| import { catchError } from '../utils/catchError.js'; | ||
|
|
||
| export const authRouter = express.Router(); | ||
|
|
||
| authRouter.post('/registration', catchError(authController.register)); | ||
| authRouter.get( | ||
| '/activation/:email/:activationToken', | ||
| catchError(authController.activate), | ||
| ); | ||
| authRouter.post('/login', catchError(authController.login)); | ||
| authRouter.get('/refresh', catchError(authController.refresh)); | ||
| authRouter.post('/logout', catchError(authController.logout)); |
There was a problem hiding this comment.
The password reset flow is missing entirely from auth.route.js. You need to add routes for: (1) POST /forgot-password - receive email and send reset link, (2) GET /reset-password/:resetToken - show reset password page, (3) POST /reset-password - process the new password. All these should be accessible to non-authenticated users only.
| authRouter.post('/registration', catchError(authController.register)); | ||
| authRouter.get( | ||
| '/activation/:email/:activationToken', | ||
| catchError(authController.activate), |
There was a problem hiding this comment.
The activation route /activation/:email/:activationToken includes an :email parameter, but the activate function in auth.controller.js only uses req.params.activationToken. The :email part of the URL is never used. Either remove it from the route or use it in the controller.
| import express from 'express'; | ||
| import { authController } from '../controllers/auth.controller.js'; | ||
| import { catchError } from '../utils/catchError.js'; | ||
|
|
||
| export const authRouter = express.Router(); | ||
|
|
||
| authRouter.post('/registration', catchError(authController.register)); | ||
| authRouter.get( | ||
| '/activation/:email/:activationToken', | ||
| catchError(authController.activate), | ||
| ); | ||
| authRouter.post('/login', catchError(authController.login)); | ||
| authRouter.get('/refresh', catchError(authController.refresh)); |
There was a problem hiding this comment.
The token model stores refresh tokens but there's no model for password reset tokens. You'll need a separate model or field to store password reset tokens with expiration for the password reset feature.
| import { catchError } from '../utils/catchError.js'; | ||
|
|
There was a problem hiding this comment.
Critical issue: The access token expires in just 5 seconds (expiresIn: '5s'). This is impractical - users will need to refresh their token constantly. Standard practice is 15-60 minutes for access tokens. Consider using '15m' or '1h' instead.
|
|
||
| export const emailService = { | ||
| sendActivationEmail, | ||
| send, |
There was a problem hiding this comment.
email.service.js is missing a password reset email function. The password reset flow requires sending emails with reset links, but only sendActivationEmail exists. You need to add a function like sendPasswordResetEmail(email, resetToken) that sends an email with a link to the reset password page.
| const transporter = nodemailer.createTransport({ | ||
| host: process.env.SMTP_HOST, |
There was a problem hiding this comment.
The JWT access token expires in 5 seconds (line 5). This is extremely short and will cause frequent re-authentication issues. Consider increasing this to something more practical like '15m' (15 minutes) or '1h' (1 hour). The refresh token can handle session persistence.
|
|
||
| export const tokenService = { | ||
| save, | ||
| getByToken, |
There was a problem hiding this comment.
email.service.js is missing functions for: (1) sending password reset emails, and (2) notifying the old email address when a user changes their email. Both are required by the task description. Add functions like sendPasswordResetEmail(email, resetToken) and sendEmailChangeNotification(oldEmail).
| if (!userId) { | ||
| throw new Error( |
There was a problem hiding this comment.
The access token expires in 5 seconds (line 5). This is extremely short - users would need to refresh their token almost constantly. Consider using a more practical expiration time like '15m' or '1h' for better UX.
| import { Token } from '../models/token.js'; | ||
|
|
||
| async function save(userId, newToken) { | ||
| if (!userId) { | ||
| throw new Error( | ||
| 'tokenService.save: Очікувався userId, але отримано undefined або null', | ||
| ); | ||
| } | ||
|
|
||
| if (!newToken) { | ||
| throw new Error( | ||
| 'tokenService.save: Очікувався newToken, але його не передано', | ||
| ); | ||
| } | ||
|
|
||
| const token = await Token.findOne({ where: { userId } }); | ||
|
|
||
| if (!token) { | ||
| await Token.create({ userId, refreshToken: newToken }); | ||
| return; | ||
| } | ||
|
|
||
| token.refreshToken = newToken; | ||
|
|
||
| await token.save(); | ||
| } | ||
|
|
||
| function getByToken(refreshToken) { | ||
| return Token.findOne({ where: { refreshToken } }); | ||
| } | ||
|
|
||
| function remove(userId) { | ||
| Token.destroy({ where: { userId } }); | ||
| } | ||
|
|
||
| export const tokenService = { | ||
| save, | ||
| getByToken, | ||
| remove, | ||
| }; |
There was a problem hiding this comment.
The token.service.js looks correct. However, note that it's designed for refresh tokens only. For password reset functionality, you'll need a separate mechanism to store password reset tokens (UUIDs with expiration) - either a new model or a separate table.
| verify, | ||
| verifyRefresh, | ||
| signRefresh, | ||
| }; |
There was a problem hiding this comment.
Missing password reset email function: The email service only has sendActivationEmail. You need to add a function to send password reset emails (e.g., sendPasswordResetEmail) that includes a reset link with a token.
| verify, | ||
| verifyRefresh, | ||
| signRefresh, | ||
| }; |
There was a problem hiding this comment.
Missing email change notification: When a user changes their email, the old email address should be notified. Add a function like sendEmailChangeNotification(oldEmail, newEmail) to inform the user that their email was changed.
| const token = jwt.sign(user, process.env.JWT_KEY, { | ||
| expiresIn: '5s', |
There was a problem hiding this comment.
The access token expires in 5 seconds (expiresIn: '5s'), which is extremely short. Users will need to refresh tokens constantly, causing poor UX. Consider increasing this to something more practical like '1h' or at least '15m'.
|
|
||
| export const userService = { | ||
| getAllActivated, | ||
| normalize, | ||
| findByEmail, | ||
| register, | ||
| getUserById, | ||
| updateName, | ||
| updateEmail, | ||
| updatePassword, |
There was a problem hiding this comment.
CRITICAL: userService.activate() is called in auth.controller.js line 41 but is NOT implemented in user.service.js. This will cause 'activate is not a function' runtime error. Implement: async function activate(activationToken) { const user = await User.findOne({ where: { activationToken } }); if (!user) throw ApiError.notFound(); user.activationToken = null; await user.save(); } and add it to exports.
|
|
||
| const oldEmail = user.email; | ||
|
|
||
| user.email = newEmail; | ||
| user.activationToken = uuidv4(); | ||
| await user.save(); | ||
|
|
||
| return user; |
There was a problem hiding this comment.
The updateEmail function captures oldEmail (line 56) but never sends a notification to the old email address. According to requirements: 'notify the old email about the change'. Call emailService.sendEmailChangeNotification(oldEmail) or similar after saving.
|
|
||
| export const userService = { | ||
| getAllActivated, | ||
| normalize, | ||
| findByEmail, | ||
| register, | ||
| getUserById, | ||
| updateName, | ||
| updateEmail, | ||
| updatePassword, |
There was a problem hiding this comment.
Password reset functions are missing from user.service.js. You need to implement: (1) forgotPassword(email) - find user by email, generate reset token, send reset email, (2) resetPassword(resetToken, newPassword) - validate token, update password. These are required by the task description.
|
|
||
| async function register(email, password, name) { | ||
| const activationToken = uuidv4(); | ||
|
|
||
| const existUser = await findByEmail(email); | ||
|
|
||
| if (existUser) { | ||
| throw ApiError.badRequest('User already exist', { | ||
| email: 'User already exist', | ||
| }); | ||
| } | ||
|
|
||
| await User.create({ email, password, name, activationToken }); | ||
| await emailService.sendActivationEmail(email, activationToken); |
There was a problem hiding this comment.
The register function doesn't validate that name is provided (though it accepts it). The registration form should inform users about password rules and validate them. Consider adding validation for name if it's required during registration.
| import { ApiError } from '../exceptions/api.error.js'; | ||
| import { User } from '../models/user.js'; | ||
| import { v4 as uuidv4 } from 'uuid'; | ||
| import { emailService } from './email.service.js'; |
There was a problem hiding this comment.
The email service is missing functions for password reset and email change notifications. The user.service needs to call emailService.sendPasswordResetEmail(email, resetToken) and emailService.sendEmailChangeNotification(oldEmail) but these don't exist yet.
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. You've implemented all core authentication functionality successfully - registration with activation, login/logout, password reset, and profile management all work correctly. The bcrypt.compare() is properly awaited, error middleware has return statements, and email change notifications go to the old email address. However, there are a few issues to address before production: (1) Missing confirmPassword validation in resetPassword function (auth.controller.js) - the task requires password and confirmation fields that must match. (2) Missing newEmailConfirmation field in email change (user.controller.js and user.service.js) - users should confirm the new email by typing it twice. (3) Remove unused imports User, emailService, uuidv4 from user.controller.js lines 1-3. (4) ESLint configuration conflict - your .eslintrc.js uses CommonJS (module.exports) but package.json has "type": "module". Rename to .eslintrc.cjs or convert to ES module syntax. These are valuable learning points about input validation and module configuration. Keep up the excellent work on the core functionality!
✨ 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.js'; | ||
| import { emailService } from '../services/email.service.js'; | ||
| import { v4 as uuidv4 } from 'uuid'; |
There was a problem hiding this comment.
Unused imports: User, emailService, and uuidv4 are imported but never used in this file
|
|
||
| const updateEmail = async (req, res) => { | ||
| const { newEmail, password } = req.body; | ||
| const userId = req.user.id; | ||
|
|
||
| await userService.updateEmail(userId, newEmail, password); |
There was a problem hiding this comment.
According to the requirements, email change should require newEmailConfirmation field (in addition to newEmail and password) to confirm the user typed the new email correctly
| export class ApiError extends Error { | ||
| constructor({ message, status, errors = {} }) { | ||
| super(message); |
There was a problem hiding this comment.
Unused imports: User, emailService, and uuidv4 are imported but never used in this file.
| @@ -0,0 +1,19 @@ | |||
| import { jwtService } from '../services/jwt.service.js'; | |||
There was a problem hiding this comment.
This import is unused - User model is accessed through userService instead
| @@ -0,0 +1,19 @@ | |||
| import { jwtService } from '../services/jwt.service.js'; | |||
|
|
|||
There was a problem hiding this comment.
This import is unused in this file
| @@ -0,0 +1,19 @@ | |||
| import { jwtService } from '../services/jwt.service.js'; | |||
|
|
|||
| export const authMiddleware = (req, res, next) => { | |||
There was a problem hiding this comment.
This import is unused in this file
No description provided.