added solution#293
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This implementation has multiple critical runtime bugs that will crash the application and missing required features. You must fix: (1) Missing uuidv4 import in auth.controller.js line 147 - causes crash on password reset, (2) Missing await on bcrypt.compare in auth.controller.js line 81 - login will always fail because it compares a Promise instead of a boolean, (3) Missing next parameter in profile.controller.js functions (changePassword, changeEmail, confirmEmail) - calling next() will throw ReferenceError, (4) Missing User import in profile.controller.js - User.findOne on line 79 will throw ReferenceError, (5) Token service null pointer bug in token.service.js - Token.create return value not assigned to token, causing crash on lines 9-10 when accessing token.refreshToken. Additionally: missing /change-name route as required by the task, missing 404 catch-all handler, and no activation check during login. Fix these issues before re-submission.
✨ 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 bcrypt, { hash } from 'bcrypt'; | ||
| import { User } from '../models/user.js'; | ||
| import { userService } from '../services/user.service.js'; | ||
| import { jwtService } from '../services/jwt.service.js'; | ||
| import { ApiError } from '../exeptions/api.error.js'; | ||
| import { tokenService } from '../services/token.service.js'; | ||
| import { emailService } from '../services/email.service.js'; |
There was a problem hiding this comment.
Missing uuidv4 import - this function is called on line 147 but the import is missing, causing a runtime crash when resetting passwords
| @@ -0,0 +1,205 @@ | |||
| import bcrypt, { hash } from 'bcrypt'; | |||
There was a problem hiding this comment.
hash is imported but never used - this is unnecessary dead code
| const login = async (req, res) => { | ||
| const { email, password } = req.body; | ||
|
|
||
| const user = await userService.findByEmail(email); |
There was a problem hiding this comment.
bcrypt.compare returns a Promise but is not awaited - the password validation will always fail since it's comparing against a Promise object
|
|
||
| const user = await userService.findByEmail(email); | ||
| const isPasswordValid = bcrypt.compare(password, user.password); | ||
|
|
There was a problem hiding this comment.
The check is for !email but should check for !user - if the user doesn't exist, accessing user.password on line 81 will throw a TypeError
| import bcrypt, { hash } from 'bcrypt'; | ||
| import { User } from '../models/user.js'; | ||
| import { userService } from '../services/user.service.js'; | ||
| import { jwtService } from '../services/jwt.service.js'; | ||
| import { ApiError } from '../exeptions/api.error.js'; | ||
| import { tokenService } from '../services/token.service.js'; |
There was a problem hiding this comment.
Missing User model import - User is used on line 79 but not imported at the top of the file
| function validateEmail(value) { | ||
| if (!value) { |
There was a problem hiding this comment.
next is called but is not defined in function parameters - the route wraps controllers with catchError which only passes req, res, next, so this should work, but verify catchError implementation passes next correctly
|
|
||
| const login = async (req, res) => { | ||
| const { email, password } = req.body; | ||
|
|
||
| const user = await userService.findByEmail(email); | ||
| const isPasswordValid = bcrypt.compare(password, user.password); | ||
|
|
||
| if (!email || !isPasswordValid) { | ||
| throw ApiError.badRequest('Email or password is incorrect'); | ||
| } | ||
|
|
||
| generateTokens(res, user); |
There was a problem hiding this comment.
The requirements specify that users must be activated before they can log in. This implementation does not check if the user's activationToken is null before allowing login. Unactivated users will be able to authenticate.
|
|
||
| const userData = jwtService.verifyRefresh(refreshToken); | ||
| const token = await tokenService.getByToken(refreshToken); | ||
|
|
||
| if (!userData || !token) { | ||
| throw ApiError.unAuthorized(); | ||
| } |
There was a problem hiding this comment.
No functionality to change name is implemented. The requirements specify that Profile page should allow users to change their name.
| const { confirmToken } = req.params; | ||
|
|
||
| const user = await User.findOne({ where: { confirmToken } }); | ||
|
|
There was a problem hiding this comment.
BUG: Missing await on bcrypt.compare() - it's an async function. Without await, isPasswordValid will be a Promise, not a boolean, causing the login check to always fail.
| const user = await User.findOne({ where: { confirmToken } }); | ||
|
|
||
| if (!user) { | ||
| throw ApiError.notFound(); |
There was a problem hiding this comment.
BUG: userService.findByEmail(email) can return null if no user exists, but there's no null check before accessing user.password on line 81. This will throw a TypeError.
| confirmToken, | ||
| 'Confirm changing Email', | ||
| 'Changing Email address', | ||
| ); |
There was a problem hiding this comment.
BUG: Missing await on user.save() - Sequelize save() returns a Promise.
| 'Changing Email address', | ||
| ); | ||
|
|
||
| res.status(200).send('Confirmation email sent'); |
There was a problem hiding this comment.
BUG: Missing await on user.save() - Sequelize save() returns a Promise.
| const confirmEmail = async (req, res) => { | ||
| const { confirmToken } = req.params; | ||
|
|
||
| const user = await User.findOne({ where: { confirmToken } }); | ||
|
|
||
| if (!user) { | ||
| throw ApiError.notFound(); | ||
| } | ||
|
|
||
| if (user.confirmTokenExpiry < new Date()) { | ||
| user.confirmToken = null; |
There was a problem hiding this comment.
REQUIREMENT VIOLATION: Login allows inactive users to authenticate. The task requires rejecting login if user is not yet activated and prompting them to activate their email.
| return next( | ||
| ApiError.badRequest({ newPassword: 'New passwords do not match' }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
REQUIREMENT VIOLATION: Registration should display password rules to users before they register. The validatePassword function only checks length but doesn't enforce other rules that should be displayed to users.
| import bcrypt from 'bcrypt'; | ||
| import { v4 as uuidv4 } from 'uuid'; | ||
| import { ApiError } from '../exeptions/api.error.js'; | ||
| import { jwtService } from '../services/jwt.service.js'; | ||
| import { userService } from '../services/user.service.js'; | ||
| import { emailService } from '../services/email.service.js'; | ||
|
|
||
| const getData = async (req, res) => { | ||
| const user = await userService.findByEmail(req.user.email); | ||
|
|
||
| res.status(200).send(userService.profileNormalize(user)); | ||
| }; | ||
|
|
||
| const changePassword = async (req, res) => { | ||
| const { oldPassword, newPassword1, newPassword2 } = req.body; | ||
|
|
||
| const user = await userService.findByEmail(req.user.email); | ||
| const isPasswordValid = await bcrypt.compare(oldPassword, user.password); | ||
|
|
||
| if (!isPasswordValid) { | ||
| return next( | ||
| ApiError.badRequest({ oldPassword: 'Old password is not correct' }), | ||
| ); | ||
| } | ||
|
|
||
| if (newPassword1.trim() !== newPassword2.trim()) { | ||
| return next( | ||
| ApiError.badRequest({ newPassword: 'New passwords do not match' }), | ||
| ); | ||
| } | ||
|
|
||
| if (newPassword1.trim() === oldPassword.trim()) { | ||
| return next( | ||
| ApiError.badRequest({ | ||
| newPassword: 'New password must be different from old password', | ||
| }), | ||
| ); | ||
| } | ||
|
|
||
| const password = await bcrypt.hash(newPassword1, 10); | ||
|
|
||
| user.password = password; | ||
| await user.save(); | ||
|
|
||
| res.status(200).send('Password changed'); | ||
| }; | ||
|
|
||
| const changeEmail = async (req, res) => { | ||
| const { password, newEmail } = req.body; | ||
|
|
||
| const user = await userService.findByEmail(req.user.email); | ||
| const isPasswordValid = await bcrypt.compare(password, user.password); | ||
|
|
||
| if (!isPasswordValid) { | ||
| return next( | ||
| ApiError.badRequest({ oldPassword: 'Password is not correct' }), | ||
| ); | ||
| } | ||
|
|
||
| const confirmToken = uuidv4(); | ||
|
|
||
| user.confirmToken = confirmToken; | ||
| user.confirmTokenExpiry = new Date(Date.now() + 1 * 60 * 60 * 1000); | ||
| user.pendingEmail = newEmail; | ||
| await user.save(); | ||
|
|
||
| await emailService.sendConfirmEmail( | ||
| newEmail, | ||
| confirmToken, | ||
| 'Confirm changing Email', | ||
| 'Changing Email address', | ||
| ); | ||
|
|
||
| res.status(200).send('Confirmation email sent'); | ||
| }; | ||
|
|
||
| const confirmEmail = async (req, res) => { | ||
| const { confirmToken } = req.params; | ||
|
|
There was a problem hiding this comment.
BUG: User model is used but never imported in profile.controller.js. Line 79 uses User.findOne() which will throw a ReferenceError.
| } | ||
|
|
||
| const confirmToken = uuidv4(); | ||
|
|
There was a problem hiding this comment.
BUG: In email.service.js, the function sendEmailChanged references undefined variable email instead of the parameter oldEmail. Should be: return send({ email: oldEmail, ... }).
|
|
||
| return next(ApiError.badRequest({ message: 'Token expired' })); |
There was a problem hiding this comment.
BUG: Missing return before next() on lines 91-92. The code will continue executing after calling next(), potentially causing issues.
| import 'dotenv/config'; | ||
| import { User } from './src/models/user.js'; | ||
| import { Token } from './src/models/token.js'; | ||
| import { client } from './src/utils/db.js'; |
There was a problem hiding this comment.
jwtService is imported but never used in this file.
| @@ -0,0 +1,6 @@ | |||
| import 'dotenv/config'; | |||
There was a problem hiding this comment.
The User model lacks an isActive or isActivated field. According to requirements, users should be activated only after email confirmation, but there's no flag to track activation status.
| @@ -0,0 +1,6 @@ | |||
| import 'dotenv/config'; | |||
There was a problem hiding this comment.
The changeName functionality is not implemented as an API endpoint. Requirements state profile should allow changing name.
| }); | ||
| } | ||
|
|
||
| static unAuthorized(errors) { | ||
| return new ApiError({ | ||
| message: 'unauthorized user', | ||
| errors, |
There was a problem hiding this comment.
Missing next parameter: Functions changePassword, changeEmail, and confirmEmail use next() but don't have it in their parameters. These functions are wrapped with catchError which provides next, but it's not destructured in the function signature.
| @@ -0,0 +1,32 @@ | |||
| export class ApiError extends Error { | |||
There was a problem hiding this comment.
Unused import: hash is imported from 'bcrypt' but never used.
| export class ApiError extends Error { | ||
| constructor({ message, status, errors }) { | ||
| super(message); | ||
|
|
There was a problem hiding this comment.
Unused import: jwtService is imported in profile.controller.js but never used (only userService is needed for getData).
| export const Token = client.define('token', { | ||
| refreshToken: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, | ||
| }, |
There was a problem hiding this comment.
The Token model is missing a userId field. The token service tries to query by userId and create tokens with userId, but the model only defines refreshToken. Add userId: { type: DataTypes.INTEGER, allowNull: false } to the model definition.
| export const Token = client.define('token', { | ||
| refreshToken: { | ||
| type: DataTypes.STRING, |
There was a problem hiding this comment.
The authMiddleware may incorrectly allow requests when Authorization header exists but has no token (e.g., header is just 'Bearer' with no token value). The split would return ['Bearer', ''] and token would be an empty string which is falsy, so this is actually correct. No issue here.
| app.get('/', (req, res) => { | ||
| res.send('Ok'); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Missing 404 handler: The requirements explicitly state to display 404 for all routes not defined above (registration, activation, login, logout, password reset, profile). This catch-all should be placed before the error middleware to handle all unmatched routes.
| }), | ||
| ); | ||
|
|
||
| app.use(authRouter); |
There was a problem hiding this comment.
Missing semicolon at end of line.
| html, | ||
| }); | ||
| } |
There was a problem hiding this comment.
REQUIREMENT VIOLATION: The User model has activationToken but no boolean flag to track if the user is actually activated. When activationToken is null, the user could be deactivated later without a way to distinguish. Consider adding an isActivated: { type: DataTypes.BOOLEAN, defaultValue: false } field.
| <h2>Email changed from ${oldEmail} to ${newEmail}</h2> | ||
| `; | ||
|
|
||
| return send({ |
There was a problem hiding this comment.
BUG: In sendEmailChanged, line 57 passes email (undefined) instead of oldEmail. The send function will fail because email is not defined in this scope.
| port: process.env.SMTP_PORT, | ||
| auth: { | ||
| user: process.env.SMTP_USER, | ||
| pass: process.env.SMTP_PASSWORD, | ||
| }, | ||
| }); | ||
|
|
||
| export function send({ email, subject, html }) { | ||
| return transporter.sendMail({ | ||
| to: email, | ||
| subject, | ||
| html, | ||
| }); | ||
| } | ||
|
|
||
| function sendActivationEmail(email, token) { | ||
| const href = `${process.env.CLIENT_HOST}/activate/${token}`; | ||
|
|
||
| const html = ` |
There was a problem hiding this comment.
MISSING ENDPOINT: The requirements state Profile page should allow changing name, but there's no /change-name route or handler implemented.
| subject: 'Activate', | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
MISMATCH: profile.controller.js calls emailService.sendConfirmEmail(newEmail, confirmToken, 'Confirm changing Email', 'Changing Email address') with 4 arguments, but the function only accepts 2 parameters (email, token). The last two arguments are silently ignored.
|
|
||
| profileRouter.get('/', authMiddleware, catchError(profileController.getData)); | ||
|
|
||
| profileRouter.post( | ||
| '/change-email', | ||
| authMiddleware, | ||
| catchError(profileController.changeEmail), | ||
| ); | ||
| profileRouter.get( | ||
| '/change-email/:confirmToken', | ||
| catchError(profileController.confirmEmail), | ||
| ); | ||
|
|
||
| profileRouter.post( | ||
| '/change-password', | ||
| authMiddleware, | ||
| catchError(profileController.changePassword), | ||
| ); |
There was a problem hiding this comment.
REQUIREMENT VIOLATION: The requirements state 'It allows to change a name' on Profile page, but there's no /change-name endpoint defined.
| ); | ||
| profileRouter.get( | ||
| '/change-email/:confirmToken', | ||
| catchError(profileController.confirmEmail), |
There was a problem hiding this comment.
CODE QUALITY: Using activationToken === null to determine if user is active is implicit. An explicit isActive boolean field would be clearer and less prone to bugs. Currently, if activationToken is an empty string instead of null, activation checks could fail.
|
|
||
| authRouter.post('/registration', catchError(authController.register)); | ||
|
|
||
| authRouter.get( |
There was a problem hiding this comment.
MISSING REQUIREMENT: The requirements state that to change email, users should 'confirm the new email'. The current implementation only accepts newEmail but doesn't require email confirmation input.
| authRouter.get( | ||
| '/activation/:activationToken', | ||
| catchError(authController.activate), | ||
| ); |
There was a problem hiding this comment.
SECURITY NOTE: The email parameter in the send function could be vulnerable to header injection if not validated. However, this may be acceptable for the scope of this task.
| import { DataTypes } from 'sequelize'; | ||
| import { client } from '../utils/db.js'; | ||
|
|
||
| export const User = client.define('user', { | ||
| name: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, | ||
| }, | ||
| email: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, | ||
| unique: true, | ||
| }, | ||
| password: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, | ||
| }, | ||
| activationToken: { | ||
| type: DataTypes.STRING, | ||
| }, | ||
| confirmToken: { | ||
| type: DataTypes.STRING, | ||
| defaultValue: null, | ||
| }, | ||
| pendingEmail: { | ||
| type: DataTypes.STRING, | ||
| defaultValue: null, | ||
| }, | ||
| confirmTokenExpiry: { | ||
| type: DataTypes.DATE, | ||
| defaultValue: null, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
The User model stores the name field correctly, but there's no API endpoint to change it. The requirements specify that Profile page should allow changing the user's name.
| import { DataTypes } from 'sequelize'; | ||
| import { client } from '../utils/db.js'; | ||
|
|
||
| export const User = client.define('user', { | ||
| name: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, | ||
| }, | ||
| email: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, | ||
| unique: true, | ||
| }, | ||
| password: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, | ||
| }, | ||
| activationToken: { | ||
| type: DataTypes.STRING, | ||
| }, | ||
| confirmToken: { | ||
| type: DataTypes.STRING, | ||
| defaultValue: null, |
There was a problem hiding this comment.
The profile router is missing a route for changing the user's name. According to requirements, the Profile page should allow changing the name.
| password: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, | ||
| }, |
There was a problem hiding this comment.
The /change-email/:confirmToken GET route doesn't have authMiddleware - this is intentional since the token in the URL serves as the authentication mechanism. No issue here.
|
|
||
| function getByToken(refreshToken) { | ||
| return Token.findOne({ where: { refreshToken } }); |
There was a problem hiding this comment.
BUG: getByToken is not marked as async but Token.findOne returns a Promise. When this function is awaited elsewhere, it will return a Promise instead of the result.
|
|
||
| function remove(userId) { | ||
| Token.destroy({ where: { userId } }); |
There was a problem hiding this comment.
BUG: remove is not marked as async but Token.destroy returns a Promise. This should be awaited in the calling code but the function itself should be async.
|
|
||
| function getByToken(refreshToken) { | ||
| return Token.findOne({ where: { refreshToken } }); |
There was a problem hiding this comment.
BUG: findByEmail is not marked as async but User.findOne returns a Promise. When this function is awaited elsewhere (like in auth.controller.js line 80), it will return a Promise instead of the user object.
| import { Token } from '../models/token.js'; | ||
|
|
||
| async function save(userId, newToken) { | ||
| const token = await Token.findOne({ where: { userId } }); | ||
|
|
||
| if (!token) { | ||
| await Token.create({ userId, refreshToken: newToken }); | ||
| } | ||
|
|
||
| token.refreshToken = newToken; | ||
| await token.save(); | ||
| } | ||
|
|
||
| function getByToken(refreshToken) { |
There was a problem hiding this comment.
BUG: catchError passes next to the action (line 3), but profile controller functions like changePassword, changeEmail, and confirmEmail don't destructure next from their parameters. When these functions call next(error), it will throw a ReferenceError because next is not defined.
| expiresIn: '1h', | ||
| }); | ||
|
|
||
| return token; | ||
| } |
There was a problem hiding this comment.
BUG: When creating a new token on line 6, the return value of Token.create() is not assigned to token. This means token remains null after the if block, and lines 9-10 will fail when trying to access token.refreshToken and token.save() on a null object.
| return jwt.verify(token, process.env.JWT_KEY); | ||
| } catch { | ||
| return null; |
There was a problem hiding this comment.
BUG: getByToken is an async function (returns a Promise from Sequelize) but is missing await. The caller receives a Promise instead of the actual token object.
| } | ||
|
|
||
| function signRefresh(user) { |
There was a problem hiding this comment.
BUG: remove is an async function (Token.destroy returns a Promise) but is missing await. The function should be marked as async and the destroy call should be awaited.
| import jwt from 'jsonwebtoken'; | ||
|
|
||
| function sign(user) { | ||
| const token = jwt.sign(user, process.env.JWT_KEY, { | ||
| expiresIn: '1h', | ||
| }); | ||
|
|
||
| return token; | ||
| } | ||
|
|
||
| function verify(token) { | ||
| try { | ||
| return jwt.verify(token, process.env.JWT_KEY); | ||
| } catch { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| function signRefresh(user) { | ||
| const token = jwt.sign(user, process.env.JWT_REFRESHKEY); | ||
|
|
||
| return token; | ||
| } | ||
|
|
||
| function verifyRefresh(token) { | ||
| try { | ||
| return jwt.verify(token, process.env.JWT_REFRESHKEY); | ||
| } catch { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| export const jwtService = { | ||
| sign, | ||
| verify, | ||
| signRefresh, | ||
| verifyRefresh, | ||
| }; |
There was a problem hiding this comment.
OBSERVATION: The jwt.service.js is implemented correctly. Access tokens expire in 1 hour while refresh tokens don't expire (they are stored in the database and can be revoked). This is a reasonable implementation.
| import jwt from 'jsonwebtoken'; | ||
|
|
||
| function sign(user) { | ||
| const token = jwt.sign(user, process.env.JWT_KEY, { | ||
| expiresIn: '1h', | ||
| }); | ||
|
|
||
| return token; | ||
| } | ||
|
|
||
| function verify(token) { | ||
| try { | ||
| return jwt.verify(token, process.env.JWT_KEY); | ||
| } catch { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| function signRefresh(user) { | ||
| const token = jwt.sign(user, process.env.JWT_REFRESHKEY); | ||
|
|
||
| return token; | ||
| } | ||
|
|
||
| function verifyRefresh(token) { | ||
| try { | ||
| return jwt.verify(token, process.env.JWT_REFRESHKEY); | ||
| } catch { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| export const jwtService = { | ||
| sign, | ||
| verify, | ||
| signRefresh, | ||
| verifyRefresh, | ||
| }; |
There was a problem hiding this comment.
OBSERVATION: The user.service.js correctly implements the register function with email uniqueness check and activation token generation. The normalize function correctly only includes id and email for JWT payload (not including password).
| import jwt from 'jsonwebtoken'; | ||
|
|
||
| function sign(user) { | ||
| const token = jwt.sign(user, process.env.JWT_KEY, { | ||
| expiresIn: '1h', | ||
| }); | ||
|
|
||
| return token; |
There was a problem hiding this comment.
OBSERVATION: The catchError utility correctly passes next to the action function, which explains how next() works in profile.controller.js even though those functions don't have next in their Express-provided parameters.
|
|
||
| async function register(name, email, password) { |
There was a problem hiding this comment.
The remove function on line 18 does not await the Token.destroy() call. This async operation should be awaited to ensure the token is properly removed from the database before the logout response is sent.
|
|
||
| function findByEmail(email) { | ||
| return User.findOne({ where: { email } }); |
There was a problem hiding this comment.
The getByToken function returns a Promise but is not marked as async. While technically it returns the Promise directly which works, marking it as async would be more consistent with other functions in this file.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The code has made significant progress fixing previous critical bugs (uuidv4 import, await on bcrypt.compare, next parameter, User import, token service upsert), but three critical functional requirements remain unimplemented: (1) CHECKLIST ITEM #8 - Login in src/controllers/auth.controller.js doesn't verify user.activationToken === null. Add if (user.activationToken) { throw ApiError.badRequest('Please activate your email first'); } after password validation around line 82. (2) CHECKLIST ITEM #6 - The activate function returns res.send(user) instead of redirecting. Replace with res.redirect(process.env.CLIENT_HOST + '/profile'). (3) CHECKLIST ITEM #9 - Login should redirect to Profile after successful login, not just return user data and token. Note: The ESLint .cjs configuration issue is a tooling problem, not student code, and won't block approval once the functional requirements above are fixed.
✨ 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 login = async (req, res) => { | ||
| const { email, password } = req.body; | ||
|
|
||
| const user = await userService.findByEmail(email); | ||
| const isPasswordValid = await bcrypt.compare(password, user.password); | ||
|
|
||
| if (!user || !isPasswordValid) { | ||
| throw ApiError.badRequest('Email or password is incorrect'); | ||
| } | ||
|
|
There was a problem hiding this comment.
CHECKLIST ITEM #8: Login doesn't check if user is activated. If user is not active, you should ask them to activate their email. Currently the login allows inactive users to login.
|
|
||
| const login = async (req, res) => { | ||
| const { email, password } = req.body; | ||
|
|
||
| const user = await userService.findByEmail(email); | ||
| const isPasswordValid = await bcrypt.compare(password, user.password); | ||
|
|
||
| if (!user || !isPasswordValid) { | ||
| throw ApiError.badRequest('Email or password is incorrect'); | ||
| } | ||
|
|
There was a problem hiding this comment.
CHECKLIST ITEM #9: Login should redirect to Profile page after successful login. Currently it only returns user data and accessToken without redirect.
| user.pendingEmail = newEmail; | ||
| await user.save(); | ||
|
|
||
| await emailService.sendConfirmEmail(newEmail, confirmToken); | ||
|
|
||
| res.status(200).send('Confirmation email sent'); | ||
| }; | ||
|
|
||
| const confirmEmail = async (req, res, next) => { | ||
| const { confirmToken } = req.params; | ||
|
|
||
| const user = await User.findOne({ where: { confirmToken } }); |
There was a problem hiding this comment.
CHECKLIST ITEM #8: Missing activation check during login. According to requirements: 'If user is not active, ask them to activate their email'. Currently, users who haven't activated their account can still log in. Add a check: if user.activationToken is not null, throw an ApiError.badRequest with a message asking the user to activate their email.
| const changeEmail = async (req, res, next) => { | ||
| const { password, newEmail } = req.body; | ||
|
|
||
| const user = await userService.findByEmail(req.user.email); | ||
| const isPasswordValid = await bcrypt.compare(password, user.password); | ||
|
|
||
| if (!isPasswordValid) { | ||
| return next( | ||
| ApiError.badRequest({ oldPassword: 'Password is not correct' }), | ||
| ); | ||
| } | ||
|
|
||
| const confirmToken = uuidv4(); | ||
|
|
||
| user.confirmToken = confirmToken; |
There was a problem hiding this comment.
CHECKLIST ITEM #6: The activate function sends user data but should redirect to Profile page. According to requirements: 'redirect to Profile after the activation'. Use res.redirect(process.env.CLIENT_HOST + '/profile') instead of res.send(user).
| sendActivationEmail, | ||
| sendConfirmEmail, |
There was a problem hiding this comment.
CHECKLIST ITEM #8: Login doesn't verify if user.activationToken is set. Add after line 81: if (user.activationToken) { throw ApiError.badRequest('Please activate your email first'); }
| } | ||
|
|
||
| function sendResetPassword(email, token) { | ||
| const href = `${process.env.CLIENT_HOST}/reset-password/${token}`; | ||
|
|
||
| const html = ` | ||
| <h1>Confirm password changing</h1> | ||
| <a href="${href}">${href}</a> | ||
| `; | ||
|
|
||
| return send({ | ||
| email, | ||
| html, | ||
| subject: 'Password changing', | ||
| }); |
There was a problem hiding this comment.
CHECKLIST ITEM #6: After successful activation, should redirect to Profile. Replace res.send(user) with res.redirect(process.env.CLIENT_HOST + '/profile').
| @@ -0,0 +1,33 @@ | |||
| import { DataTypes } from 'sequelize'; | |||
There was a problem hiding this comment.
CHECKLIST ITEM #8 is still missing: The login function (auth.controller.js) does not check if user.activationToken is set. If a user hasn't activated their email, they should receive an error asking them to activate. Add this check before or after line 82 in auth.controller.js.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Here is my approval for your solution, which successfully implements all core authentication requirements including registration, activation, login, password reset, and profile management. Your implementation correctly handles email verification, token generation, and protected routes.
However, there's one remaining bug to fix before production: The logout function in both src/controllers/profile.controller.js and src/controllers/auth.controller.js uses res.sendStatus(204).redirect(), which is incorrect because sendStatus(204) ends the response immediately, preventing the redirect from executing. Fix: In profile.controller.js, remove sendStatus(204) entirely and just use res.redirect(process.env.CLIENT_HOST + '/login');. In auth.controller.js, use res.status(204).redirect(process.env.CLIENT_HOST + '/login'); instead of chaining methods.
Additionally, consider adding authMiddleware to the logout endpoint in src/routes/auth.route.js since logout should only be available for authenticated users. The ESLint configuration issue with .eslintrc.js is a tooling problem, not related to your code.
You've made excellent progress fixing the three critical issues from the previous review. Keep up the great work!
✨ 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
| user.email = user.pendingEmail; | ||
| user.pendingEmail = null; | ||
| user.confirmToken = null; | ||
| user.confirmTokenExpiry = null; |
There was a problem hiding this comment.
This chaining makes redirect unreachable - sendStatus(204) returns the res object, so redirect never executes. The requirement is to 'Redirect to login after logging out'. Change to just res.redirect(process.env.CLIENT_HOST + '/login');
|
|
||
| res.sendStatus(204).redirect(process.env.CLIENT_HOST + '/login'); |
There was a problem hiding this comment.
res.sendStatus(204) already ends the response, so chaining .redirect() after it won't execute. Separate the status and redirect: res.status(204).redirect(process.env.CLIENT_HOST + '/login');
| unique: true, | ||
| }, | ||
| password: { | ||
| type: DataTypes.STRING, |
There was a problem hiding this comment.
The logout endpoint is mapped to POST method in auth.route.js (line 15), but the description requires logout to be available only for authenticated users. There's no authMiddleware protecting this route like there is on profile routes. Consider whether POST /logout should have authentication.
brespect
left a comment
There was a problem hiding this comment.
Good progress, but you need to pass all tests before requesting the review
|
This problem hasn't been resolved, but Luk said it doesn't affect the operation |
brespect
left a comment
There was a problem hiding this comment.
I saw, but the requirements of any technical or business task are much more valuable than AI suggestions. Luke is just an auxiliary tool that simplifies the review
|
But the main reason for this bug is that the validation is done by ESLint via mateTest, which is why it throws an error; and switching from Module to CommonJS, in my opinion, is a time-consuming and unnecessary task. |
Anton-Kuchmasov
left a comment
There was a problem hiding this comment.
Good job!
Left some comments below - might be helpful in your further tasks
| ); | ||
| } | ||
|
|
||
| if (newPassword1.trim() !== newPassword2.trim()) { |
There was a problem hiding this comment.
bad naming - newPassword, confirmedPassword should be better
| const confirmToken = uuidv4(); | ||
|
|
||
| user.confirmToken = confirmToken; | ||
| user.confirmTokenExpiry = new Date(Date.now() + 1 * 60 * 60 * 1000); |
There was a problem hiding this comment.
TTL should be saved as a constant
| user.pendingEmail = newEmail; | ||
| await user.save(); | ||
|
|
||
| await emailService.sendConfirmEmail(newEmail, confirmToken); |
There was a problem hiding this comment.
if something went wrong with confirmation email sending, you should handle this - no need to respond 200 in this case, it's crucial issue
use try-catch statement here
| const user = await User.findOne({ where: { confirmToken } }); | ||
|
|
||
| if (!user) { | ||
| throw ApiError.notFound(); |
There was a problem hiding this comment.
add some clarification here - e.g. errorType or errorMessage fields
| await tokenService.save(normalizedUser.id, refreshToken); | ||
|
|
||
| res.cookie('refreshToken', refreshToken, { | ||
| maxAge: 30 * 24 * 60 * 60 * 1000, |
There was a problem hiding this comment.
same here - it's easy to read 30 days TTL here, but it should be saved as a const
| }); | ||
|
|
||
| res.redirect( | ||
| `${process.env.CLIENT_HOST}/oauth?accessToken=${accessToken}&user=${encodeURIComponent(JSON.stringify(normalizedUser))}`, |
There was a problem hiding this comment.
unsave practice, it's better to use URL Constructor here
No description provided.