solution#285
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your code has critical syntax errors in 9 TypeScript files that prevent compilation (missing initializers in const declarations, unexpected tokens, missing semicolons). These errors make it impossible to run or test the application. Additionally, several core features are missing: (1) Registration doesn't accept a name field — it derives name from email instead, (2) Password reset functionality is completely unimplemented (no endpoints, mailer functions, or token storage), (3) Profile management endpoints for changing name/password/email are missing entirely, (4) No authentication guards preventing authenticated users from accessing register/login pages. You must fix the TypeScript syntax errors first, then implement the missing authentication guards, registration with name field, password reset flow, and profile management endpoints. The JWT token expiration times (10s/30s) are also impractical and should be increased to realistic values (15min/7d).
✨ 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 usersController = { | ||
| getAll, |
There was a problem hiding this comment.
The register function only extracts email and password from the request body, but the requirements specify registration should use name, email, and password. The name field is missing from the destructuring.
| import { RequestHandler } from 'express'; | ||
| import { usersRepository } from '../entity/users.repository.js'; | ||
| import { userService } from '../services/user.service.js'; | ||
|
|
||
| const getAll: RequestHandler = async (req, res) => { | ||
| const users = await usersRepository.getAllActive(); | ||
|
|
||
| res.json(users.map(userService.normalize)); | ||
| }; |
There was a problem hiding this comment.
There's no route handler for password reset. The requirements explicitly state: ask for email, show email sent page, add reset password confirmation page (with password and confirmation fields), and show success page with link to login. These endpoints are completely missing.
| import { RequestHandler } from 'express'; | ||
| import { usersRepository } from '../entity/users.repository.js'; | ||
| import { userService } from '../services/user.service.js'; | ||
|
|
||
| const getAll: RequestHandler = async (req, res) => { | ||
| const users = await usersRepository.getAllActive(); | ||
|
|
||
| res.json(users.map(userService.normalize)); | ||
| }; | ||
|
|
||
| export const usersController = { | ||
| getAll, |
There was a problem hiding this comment.
There's no profile controller for authenticated users to: change name, change password (with old password, new password, confirmation), or change email (with password, confirmation of new email, notify old email). The users.controller only has a getAll endpoint, not profile management.
| res.json(users.map(userService.normalize)); | ||
| }; | ||
|
|
||
| export const usersController = { |
There was a problem hiding this comment.
The requirements state: 'Registration only allowed for non-authenticated users' and 'Login only allowed for non-authenticated users', but there's no middleware or check preventing already-authenticated users from accessing these endpoints.
|
|
||
| export const authRouter = Router(); | ||
|
|
||
| authRouter.post('/register', authController.register); |
There was a problem hiding this comment.
Missing: Registration endpoint should only be accessible to non-authenticated users (CHECKLIST ITEM #2). Consider adding an auth middleware to prevent already-authenticated users from registering again.
| export const authRouter = Router(); | ||
|
|
||
| authRouter.post('/register', authController.register); | ||
| authRouter.post('/login', authController.login); |
There was a problem hiding this comment.
Missing: Login endpoint should only be accessible to non-authenticated users (CHECKLIST ITEM #4). Consider adding middleware to check if user is already authenticated.
|
|
||
| authRouter.post('/register', authController.register); | ||
| authRouter.post('/login', authController.login); | ||
| authRouter.post('/logout', authController.logout); |
There was a problem hiding this comment.
Missing: Logout endpoint should only be accessible to authenticated users (CHECKLIST ITEM #7). Consider adding authMiddleware to ensure only authenticated users can logout.
| authRouter.post('/login', authController.login); | ||
| authRouter.post('/logout', authController.logout); | ||
| authRouter.get('/refresh', authController.refresh); | ||
| authRouter.get('/activation/:email/:token', authController.activate); |
There was a problem hiding this comment.
Missing: Activation endpoint should only be accessible to non-authenticated users (CHECKLIST ITEM #9). Consider adding a check to prevent already-authenticated users from accessing activation.
| import { Router } from 'express'; | ||
| import { authController } from './auth.controller.js'; | ||
|
|
||
| export const authRouter = Router(); | ||
|
|
||
| authRouter.post('/register', authController.register); | ||
| authRouter.post('/login', authController.login); | ||
| authRouter.post('/logout', authController.logout); | ||
| authRouter.get('/refresh', authController.refresh); | ||
| authRouter.get('/activation/:email/:token', authController.activate); |
There was a problem hiding this comment.
| import bcrypt from 'bcrypt'; | ||
| import crypto from 'crypto'; | ||
| import { RequestHandler, Response } from 'express'; | ||
| import { usersRepository } from '../entity/users.repository.js'; | ||
| import { mailer } from '../utils/mailer.js'; | ||
| import { NormalizedUser, userService } from '../services/user.service.js'; | ||
| import { jwt } from '../utils/jwt.js'; | ||
| import { User } from '@prisma/client'; | ||
| import { tokensRepository } from '../entity/tokens.repository.js'; | ||
|
|
||
| const register: RequestHandler = async (req, res) => { | ||
| const { email, password } = req.body; | ||
|
|
||
| const errors = { | ||
| email: userService.validateEmail(email), | ||
| password: userService.validatePassword(password), | ||
| }; | ||
|
|
||
| if (Object.values(errors).some((error) => error)) { | ||
| return res.status(400).json({ | ||
| errors, | ||
| message: 'Validation error', | ||
| }); | ||
| } | ||
|
|
||
| const existingUser = await usersRepository.getByEmail(email); | ||
|
|
||
| if (existingUser) { | ||
| return res.status(400).json({ | ||
| errors: { email: 'Email is already taken' }, | ||
| message: 'Validation error', | ||
| }); | ||
| } | ||
|
|
||
| const activationToken = crypto.randomUUID(); | ||
| const hashedPassword = await bcrypt.hash(password, 10); | ||
|
|
||
| const user = await usersRepository.create( | ||
| email, | ||
| hashedPassword, | ||
| activationToken, | ||
| ); | ||
|
|
||
| await mailer.sendActivationLink(email, activationToken); | ||
|
|
||
| res.json({ | ||
| user: userService.normalize(user), | ||
| }); | ||
| }; | ||
|
|
||
| async function sendAuthentication(res: Response, user: User) { | ||
| const userData = userService.normalize(user); | ||
| const accessToken = jwt.generateAccessToken(userData); | ||
| const refreshToken = jwt.generateRefreshToken(userData); | ||
|
|
||
| await tokensRepository.deleteByUserId(user.id); | ||
| await tokensRepository.create(user.id, refreshToken); | ||
|
|
||
| res.cookie('refreshToken', refreshToken, { | ||
| maxAge: 30 * 24 * 60 * 60 * 1000, | ||
| httpOnly: true, | ||
| sameSite: 'none', | ||
| secure: true, | ||
| }); | ||
|
|
||
| res.send({ | ||
| user: userData, | ||
| accessToken, | ||
| }); | ||
| } | ||
|
|
||
| const activate: RequestHandler = async (req, res) => { | ||
| const { email, token } = req.params; | ||
|
|
||
| if (typeof email !== 'string' || typeof token !== 'string') { | ||
| return res.status(400).json({ message: 'Invalid activation parameters' }); | ||
| } | ||
|
|
||
| const user = await usersRepository.getByEmail(email); | ||
|
|
||
| if (!user || user.activationToken !== token) { | ||
| return res | ||
| .status(404) | ||
| .json({ message: 'Activation link is invalid or expired' }); | ||
| } | ||
|
|
||
| await usersRepository.activate(email); | ||
|
|
||
| user.activationToken = null; | ||
|
|
||
| await sendAuthentication(res, user); | ||
| }; | ||
|
|
||
| const login: RequestHandler = async (req, res) => { | ||
| const { email, password } = req.body; | ||
|
|
||
| const user = await usersRepository.getByEmail(email); | ||
| const isPasswordValid = await bcrypt.compare(password, user?.password || ''); | ||
|
|
||
| if (!user || !isPasswordValid) { | ||
| return res.status(401).json({ message: 'Invalid credentials' }); | ||
| } | ||
|
|
||
| if (user.activationToken) { | ||
| return res | ||
| .status(403) | ||
| .json({ message: 'Account is not activated. Please check your email.' }); | ||
| } | ||
|
|
||
| await sendAuthentication(res, user); | ||
| }; | ||
|
|
||
| const refresh: RequestHandler = async (req, res) => { | ||
| const refreshToken = req.cookies?.refreshToken || ''; | ||
| const userData = jwt.validateRefreshToken(refreshToken) as NormalizedUser; | ||
| const user = await usersRepository.getByEmail(userData?.email || ''); | ||
| const token = await tokensRepository.getByToken(refreshToken); | ||
|
|
||
| if (!user || !userData || !token || token.userId !== user.id) { | ||
| res.clearCookie('refreshToken'); | ||
| return res.status(401).json({ message: 'Invalid token' }); | ||
| } | ||
|
|
||
| await sendAuthentication(res, user); | ||
| }; | ||
|
|
||
| const logout: RequestHandler = async (req, res) => { | ||
| const refreshToken = req.cookies?.refreshToken || ''; | ||
| const userData = jwt.validateRefreshToken(refreshToken) as NormalizedUser; | ||
|
|
||
| if (userData) { | ||
| await tokensRepository.deleteByUserId(userData.id); | ||
| } | ||
|
|
||
| res.clearCookie('refreshToken', { | ||
| sameSite: 'none', | ||
| secure: true, | ||
| }); | ||
| res.sendStatus(204); | ||
| }; | ||
|
|
||
| export const authController = { | ||
| logout, | ||
| refresh, | ||
| login, | ||
| register, | ||
| activate, | ||
| }; |
There was a problem hiding this comment.
Missing password reset functionality. The description requires: (1) endpoint to request reset with email, (2) email sent confirmation page, (3) reset password confirmation with 'password' and 'confirmation' fields that must be equal, (4) success page with link to login. None of these are implemented.
| const register: RequestHandler = async (req, res) => { | ||
| const { email, password } = req.body; |
There was a problem hiding this comment.
The register function doesn't accept a 'name' field from req.body, but the requirement states 'Register using name, email and password'. The name parameter should be extracted from req.body and passed to usersRepository.create.
| import bcrypt from 'bcrypt'; | ||
| import crypto from 'crypto'; | ||
| import { RequestHandler, Response } from 'express'; | ||
| import { usersRepository } from '../entity/users.repository.js'; | ||
| import { mailer } from '../utils/mailer.js'; | ||
| import { NormalizedUser, userService } from '../services/user.service.js'; | ||
| import { jwt } from '../utils/jwt.js'; | ||
| import { User } from '@prisma/client'; | ||
| import { tokensRepository } from '../entity/tokens.repository.js'; | ||
|
|
||
| const register: RequestHandler = async (req, res) => { | ||
| const { email, password } = req.body; | ||
|
|
There was a problem hiding this comment.
No profile controller exists. The requirements state that the Profile page should: (1) allow changing name, (2) allow changing password (with old password + new password + confirmation), (3) allow changing email (requires password, confirms new email, notifies old email). These endpoints are missing.
| import bcrypt from 'bcrypt'; | ||
| import crypto from 'crypto'; | ||
| import { RequestHandler, Response } from 'express'; | ||
| import { usersRepository } from '../entity/users.repository.js'; | ||
| import { mailer } from '../utils/mailer.js'; | ||
| import { NormalizedUser, userService } from '../services/user.service.js'; | ||
| import { jwt } from '../utils/jwt.js'; |
There was a problem hiding this comment.
The users router has no endpoints for updating user profile data (name, password, email changes). Profile-related routes are completely absent from the router.
| return res.status(400).json({ | ||
| errors, | ||
| message: 'Validation error', | ||
| }); | ||
| } | ||
|
|
||
| const existingUser = await usersRepository.getByEmail(email); | ||
|
|
There was a problem hiding this comment.
User service password validation only checks length >= 6. The requirement states to 'inform users about the rules for a password' - more comprehensive password rules should be implemented and communicated to users.
| } | ||
|
|
||
| function activate(email: string) { |
There was a problem hiding this comment.
The 404 handler is correctly implemented for unmatched routes (CHECKLIST ITEM #24).
| activationToken, | ||
| }, |
There was a problem hiding this comment.
The createApp function properly sets up Express with JSON parsing and CORS. However, there's no profile router registered - profile routes should be added for authenticated user profile management.
|
|
||
| function create(email: string, password: string, activationToken?: string) { |
There was a problem hiding this comment.
The users.repository.ts create function derives name from email (line 9) instead of accepting it as a parameter. The requirements specify 'Register using name, email and password' - the name should come from user input during registration.
| import { db } from '../utils/db.js'; | ||
|
|
||
| function getByEmail(email: string) { | ||
| return db.user.findUnique({ | ||
| where: { email }, | ||
| }); | ||
| } | ||
|
|
||
| function create(email: string, password: string, activationToken?: string) { | ||
| const defaultName = email.split('@')[0]; | ||
|
|
||
| return db.user.create({ | ||
| data: { | ||
| email, | ||
| password, | ||
| name: defaultName, | ||
| activationToken, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| function activate(email: string) { | ||
| return db.user.update({ | ||
| where: { email }, | ||
| data: { activationToken: null }, | ||
| }); | ||
| } | ||
|
|
||
| function getAllActive() { | ||
| return db.user.findMany({ | ||
| where: { activationToken: null }, | ||
| }); | ||
| } | ||
|
|
||
| export const usersRepository = { | ||
| getAllActive, | ||
| activate, | ||
| getByEmail, | ||
| create, | ||
| }; |
There was a problem hiding this comment.
Missing repository methods for profile updates: updateName, updatePassword, updateEmail with the required business logic (password verification for email change, notification to old email).
| }); | ||
| } | ||
|
|
||
| function getAllActive() { | ||
| return db.user.findMany({ | ||
| where: { activationToken: null }, | ||
| }); |
| import { db } from '../utils/db.js'; | ||
|
|
||
| function getByEmail(email: string) { | ||
| return db.user.findUnique({ | ||
| where: { email }, | ||
| }); | ||
| } | ||
|
|
||
| function create(email: string, password: string, activationToken?: string) { | ||
| const defaultName = email.split('@')[0]; | ||
|
|
||
| return db.user.create({ | ||
| data: { | ||
| email, | ||
| password, | ||
| name: defaultName, | ||
| activationToken, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| function activate(email: string) { | ||
| return db.user.update({ | ||
| where: { email }, | ||
| data: { activationToken: null }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The index.ts entry point is well-structured with graceful shutdown handling. No issues found.
| await db.$disconnect(); | ||
| console.log('Server is closed'); | ||
|
|
There was a problem hiding this comment.
The app.ts correctly returns 404 for all unhandled routes (requirement #24). The 404 middleware at the end of the route chain properly catches unmatched requests.
| const server = app.listen(PORT, () => { | ||
| console.log(`Server running at http://localhost:${PORT}/`); | ||
| }); | ||
|
|
||
| const cb: CloseWithGraceCallback = ({ err, signal }, done) => { | ||
| if (err) { | ||
| console.error('Closing server with error', err); | ||
| } else { | ||
| console.log(`${signal} received, closing server`); | ||
| } | ||
|
|
There was a problem hiding this comment.
The users.repository.ts create function auto-generates a default name from the email prefix (line 9, 15) instead of accepting a name parameter. The registration requirement specifies 'Register using name, email and password' - the name should come from user input, not be auto-generated.
|
|
||
| function getByToken(token: string) { | ||
| return db.token.findFirst({ | ||
| where: { token }, | ||
| }); | ||
| } | ||
|
|
||
| function deleteByUserId(userId: string) { | ||
| return db.token.deleteMany({ | ||
| where: { userId }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The create function derives name from email.split('@')[0] (line 9-10), but the registration requirement specifies 'Register using name, email and password'. The function should accept a name parameter instead of generating one automatically.
| create, | ||
| getByToken, | ||
| deleteByUserId, | ||
| deleteSingleToken, | ||
| }; |
There was a problem hiding this comment.
Missing password reset token storage methods. For password reset functionality, you need to store reset tokens with expiration. Consider adding createPasswordResetToken, getByPasswordResetToken, and deletePasswordResetToken methods.
| where: { userId }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The app.ts is missing routers for password reset and profile management. The task requires password reset endpoints (ask email, show email sent, confirmation page with password/confirmation, success page) and profile page endpoints (change name, change password, change email).
|
|
||
| app.use('/auth', authRouter); | ||
| app.use('/users', usersRouter); |
There was a problem hiding this comment.
The app.ts is missing routes for password reset (request, reset confirmation) and profile management (change name, change password, change email). These should be added alongside authRouter and usersRouter.
|
|
||
| app.use(express.json()); | ||
|
|
||
| app.use( | ||
| cors({ | ||
| origin: process.env.CLIENT_URL, | ||
| credentials: true, | ||
| }), | ||
| ); | ||
|
|
||
| app.use('/auth', authRouter); | ||
| app.use('/users', usersRouter); |
There was a problem hiding this comment.
The create function in users.repository.ts generates a default name from email (line 9), but registration should accept the user's actual name. The function signature should include a name parameter and the controller should pass it.
|
|
||
| function generateAccessToken(user: NormalizedUser) { | ||
| return jsonwebtoken.sign({ ...user }, ACCESS_SECRET, { expiresIn: '10s' }); | ||
| } | ||
|
|
||
| function generateRefreshToken(user: NormalizedUser) { | ||
| return jsonwebtoken.sign({ ...user }, REFRESH_SECRET, { expiresIn: '30s' }); |
There was a problem hiding this comment.
The generateAccessToken function sets expiration to '10s' and generateRefreshToken to '30s' (lines 9, 13). These values are impractically short - users would need to re-authenticate every few seconds. Standard practice is 15-60 minutes for access tokens and days/weeks for refresh tokens.
| } | ||
|
|
||
| function validateRefreshToken(token: string) { | ||
| try { | ||
| return jsonwebtoken.verify(token, REFRESH_SECRET); | ||
| } catch (error) { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
The userService.validatePassword function only checks for minimum length (6 characters). The requirement states to 'Inform the users about the rules for a password and check them' - the password rules should be documented/enumerated somewhere and communicated to users during registration.
| import jsonwebtoken from 'jsonwebtoken'; | ||
| import { NormalizedUser } from '../services/user.service'; | ||
|
|
||
| const ACCESS_SECRET = (process.env.JWT_ACCESS_SECRET || | ||
| 'access_secret_key') as string; | ||
| const REFRESH_SECRET = (process.env.JWT_REFRESH_SECRET || | ||
| 'refresh_secret_key') as string; | ||
|
|
||
| function generateAccessToken(user: NormalizedUser) { | ||
| return jsonwebtoken.sign({ ...user }, ACCESS_SECRET, { expiresIn: '10s' }); | ||
| } | ||
|
|
||
| function generateRefreshToken(user: NormalizedUser) { | ||
| return jsonwebtoken.sign({ ...user }, REFRESH_SECRET, { expiresIn: '30s' }); | ||
| } | ||
|
|
||
| function validateAccessToken(token: string) { | ||
| try { | ||
| return jsonwebtoken.verify(token, ACCESS_SECRET); | ||
| } catch (error) { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| function validateRefreshToken(token: string) { |
There was a problem hiding this comment.
Missing a middleware to prevent authenticated users from accessing certain routes. The requirements specify that registration, login, activation, and password reset should be 'only non-authenticated'. You'll need a guestMiddleware or similar to check if a user is NOT authenticated before allowing access to these routes.
| import { NormalizedUser } from '../services/user.service'; | ||
|
|
||
| const ACCESS_SECRET = (process.env.JWT_ACCESS_SECRET || | ||
| 'access_secret_key') as string; | ||
| const REFRESH_SECRET = (process.env.JWT_REFRESH_SECRET || | ||
| 'refresh_secret_key') as string; |
There was a problem hiding this comment.
The normalize function in userService excludes the user's name field (lines 3-7), but registration requires a name and users should be able to view their profile name. Consider including name in the normalized user object.
| const authHeader = req.headers['authorization'] || ''; | ||
| const [, accessToken] = authHeader.split(' '); | ||
|
|
||
| if (!authHeader || !accessToken) { | ||
| res.status(401).json({ message: 'Token is required' }); |
There was a problem hiding this comment.
The access token expires in 10 seconds (line 9) and refresh token in 30 seconds (line 13). These values are impractical - access tokens typically last 15-60 minutes and refresh tokens 7-30 days. Consider changing to '15m' and '7d' respectively.
| import { jwt } from '../utils/jwt.js'; | ||
|
|
||
| export function authMiddleware( | ||
| req: Request, | ||
| res: Response, | ||
| next: NextFunction, |
There was a problem hiding this comment.
The normalize function returns only id and email (lines 4-6), but should also include name for profile display. Add name: user.name to the returned object.
| export function authMiddleware( | ||
| req: Request, | ||
| res: Response, | ||
| next: NextFunction, | ||
| ) { | ||
| const authHeader = req.headers['authorization'] || ''; | ||
| const [, accessToken] = authHeader.split(' '); | ||
|
|
||
| if (!authHeader || !accessToken) { | ||
| res.status(401).json({ message: 'Token is required' }); | ||
| return; | ||
| } | ||
|
|
||
| const userData = jwt.validateAccessToken(accessToken); | ||
|
|
||
| if (!userData) { | ||
| res.status(401).json({ message: 'Invalid token' }); | ||
| return; | ||
| } | ||
|
|
||
| next(); |
There was a problem hiding this comment.
The authMiddleware validates the token but doesn't attach user data to req.user or req.userData. Downstream handlers (like profile update) need access to the authenticated user's ID. Consider attaching req.user = userData after validation.
|
|
||
| export type NormalizedUser = ReturnType<typeof normalize>; | ||
|
|
||
| function validateEmail(email: string): string | void { | ||
| const emailPattern = /^[\w.+-]+@([\w-]+\.){1,3}[\w-]{2,}$/; |
There was a problem hiding this comment.
The access token expires in 10 seconds (line 9) and refresh token in 30 seconds (line 13). These values are unrealistically short and will cause constant re-authentication failures. Access tokens should typically expire in 15-60 minutes, and refresh tokens in 7-30 days.
|
|
||
| function validatePassword(password: string): string | void { | ||
| if (!password) { | ||
| return 'Password is required'; | ||
| } | ||
|
|
||
| if (password.length < 6) { | ||
| return 'At least 6 characters'; | ||
| } |
There was a problem hiding this comment.
The password validation (user.service.ts line 28-29) only checks for a minimum of 6 characters. The task requires 'Inform the users about the rules for a password and check them'. Consider adding more comprehensive rules and exposing them via a method like getPasswordRules() so the frontend can display them to users.
| import { User } from '@prisma/client'; | ||
|
|
||
| function normalize(user: User) { | ||
| return { | ||
| id: user.id, | ||
| email: user.email, | ||
| }; | ||
| } | ||
|
|
||
| export type NormalizedUser = ReturnType<typeof normalize>; | ||
|
|
||
| function validateEmail(email: string): string | void { | ||
| const emailPattern = /^[\w.+-]+@([\w-]+\.){1,3}[\w-]{2,}$/; | ||
|
|
||
| if (!email) { | ||
| return 'Email is required'; | ||
| } | ||
|
|
||
| if (!emailPattern.test(email)) { | ||
| return 'Email is not valid'; | ||
| } | ||
| } | ||
|
|
||
| function validatePassword(password: string): string | void { |
There was a problem hiding this comment.
The auth.middleware.ts correctly validates authenticated routes but there's no middleware to restrict routes to non-authenticated users only (for login, register, password reset, activation pages). Consider adding a guestMiddleware that blocks authenticated users from accessing these routes.
| function normalize(user: User) { | ||
| return { | ||
| id: user.id, | ||
| email: user.email, | ||
| }; |
There was a problem hiding this comment.
The userService.normalize function (line 3-7) doesn't include the user's name field, but the requirements specify profile management includes changing the name. The normalized user should include name for profile display purposes.
| import { User } from '@prisma/client'; | ||
|
|
||
| function normalize(user: User) { |
There was a problem hiding this comment.
The db.ts is correctly initialized with PrismaClient. No issues found.
|
|
||
| export function sendActivationLink(email: string, activationToken: string) { | ||
| const link = `${process.env.CLIENT_URL}/activate/${email}/${activationToken}`; | ||
|
|
||
| const html = ` | ||
| <div style="font-family: sans-serif; padding: 20px; max-width: 600px; border: 1px solid #eee; border-radius: 8px;"> | ||
| <h2 style="color: #333;">Welcome to Auth API!</h2> | ||
| <p>Thank you for registering. Please click the button below to activate your account:</p> | ||
| <div style="margin: 24px 0;"> | ||
| <a href="${link}" style="background-color: #4CAF50; color: white; padding: 12px 24px; text-decoration: none; border-radius: 4px; font-weight: bold; display: inline-block;"> | ||
| Activate Account | ||
| </a> | ||
| </div> | ||
| <p style="font-size: 12px; color: #666;">If the button doesn't work, copy and paste this link into your browser:</p> | ||
| <p style="font-size: 12px; color: #0066cc; word-break: break-all;"><a href="${link}">${link}</a></p> | ||
| </div> | ||
| `; | ||
|
|
||
| return send(email, 'Account activation', html); | ||
| }; | ||
|
|
||
| export const mailer = { | ||
| send, | ||
| sendActivationLink, |
There was a problem hiding this comment.
|
|
||
| export const mailer = { | ||
| send, | ||
| sendActivationLink, |
There was a problem hiding this comment.
Missing function to notify the old email about email change. The profile requirement (CHECKLIST ITEM #21) specifies that when changing email, you should 'notify the old email about the change'. Add a sendEmailChangeNotification function that informs users their email is being changed.
|
|
||
| export const mailer = { | ||
| send, | ||
| sendActivationLink, |
There was a problem hiding this comment.
The mailer object only exports send and sendActivationLink (lines 43-45). Add the missing password reset and email change notification methods to this export.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation has made progress on the core authentication flow, but there are critical issues that must be fixed before this can be approved.
Critical Blocking Issues
-
ESLint Parsing Errors (9 files affected) - The code has syntax errors that prevent it from compiling. For example:
src/api/auth.controller.tsline 2:12 - Missingfromin importsrc/middlewares/auth.middleware.tsline 1:12 - Same issuesrc/entity/tokens.repository.tsline 3:22 - Unexpected token- Check all import statements:
import express './express'should be `import express from 'express'
-
Registration ignores
namefield [CHECKLIST #1] -auth.controller.tsdestructures{ email, password }but never usesname. Theusers.repository.tsderives name from email instead of accepting it as a parameter. -
Password reset functionality is completely missing [CHECKLIST #15-19] - No endpoints for forgot-password request, reset password confirmation page, or password update. No
sendResetLinkfunction inmailer.ts. -
Profile management endpoints missing [CHECKLIST #20-23] - No endpoints to change name, password, or email.
userService.normalize()doesn't include thenamefield. -
JWT token expiration times are unrealistic - Access token expires in 10 seconds (
'10s') and refresh token in 30 seconds ('30s'). These should be'15m'and'7d'respectively. -
Auth middleware doesn't attach user data -
auth.middleware.tsvalidates tokens but doesn't setreq.user, so downstream handlers cannot access the authenticated user.
Minor Issues
mailer.tsis missingsendEmailChangeNotificationfor notifying old email on email changes [CHECKLIST #23]- No guest middleware preventing authenticated users from accessing register/login pages
How to Fix
- Run
npm run formatlocally and fix all import statement syntax errors - Pass the
namefield from registration through tousersRepository.create() - Add password reset endpoints and mailer functions
- Add profile management endpoints (change name, password, email)
- Update JWT expiration to realistic values (
'15m'and'7d') - Attach user data to the request object in auth middleware
The implementation has a solid foundation, but these critical blockers prevent it from meeting the task 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
| 'no-unused-vars': 'off', | ||
| 'no-undef': 'off', |
There was a problem hiding this comment.
Registration doesn't accept name field as required by checklist item #1. It only extracts email and password from req.body, and usersRepository.create() derives name from email. The registration must accept and store the name field from user input.
| module.exports = { | ||
| extends: '@mate-academy/eslint-config', | ||
| parser: '@typescript-eslint/parser', | ||
| plugins: ['jest', '@typescript-eslint'], | ||
| env: { | ||
| jest: true | ||
| jest: true, | ||
| }, | ||
| rules: { | ||
| 'no-proto': 0 | ||
| 'no-proto': 0, | ||
| 'no-unused-vars': 'off', | ||
| 'no-undef': 'off', |
| import { jwt } from '../utils/jwt.js'; | ||
| import { User } from '@prisma/client'; | ||
| import { tokensRepository } from '../entity/tokens.repository.js'; | ||
|
|
||
| const register: RequestHandler = async (req, res) => { |
There was a problem hiding this comment.
JWT access token expires in 10 seconds and refresh token in 30 seconds - these are unrealistic values. The access token should expire in ~15 minutes and refresh token in ~7 days as mentioned in the previous review.
| import { tokensRepository } from '../entity/tokens.repository.js'; | ||
|
|
||
| const register: RequestHandler = async (req, res) => { | ||
| const { email, password } = req.body; |
There was a problem hiding this comment.
Registration endpoint destructures name from req.body but never uses it - the name is not passed to usersRepository.create() and not stored in the database.
| import { mailer } from '../utils/mailer.js'; | ||
| import { NormalizedUser, userService } from '../services/user.service.js'; | ||
| import { jwt } from '../utils/jwt.js'; | ||
| import { User } from '@prisma/client'; |
There was a problem hiding this comment.
The usersRouter imports profileController but profileController is not defined in users.controller.ts - this will cause a runtime error when importing the router.
| const activationToken = crypto.randomUUID(); | ||
| const hashedPassword = await bcrypt.hash(password, 10); | ||
|
|
There was a problem hiding this comment.
Password reset endpoint updates the user before validating that the new password matches the confirmation, meaning if passwords don't match the user state is already corrupted.
| res.json(users.map(userService.normalize)); | ||
| }; | ||
|
|
||
| export const usersController = { |
There was a problem hiding this comment.
Registration must accept name, email, and password fields per the task requirements. Currently only email and password are extracted. The name field is missing from the destructuring assignment.
|
|
||
| export const usersController = { | ||
| getAll, | ||
| }; |
There was a problem hiding this comment.
Missing profile management endpoints. The task requires endpoints to: (1) change name, (2) change password (with old password, new password, confirmation), (3) change email (with password, new email, notify old email).
| import { Router } from 'express'; | ||
| import { authController } from './auth.controller.js'; | ||
|
|
||
| export const authRouter = Router(); | ||
|
|
||
| authRouter.post('/register', authController.register); | ||
| authRouter.post('/login', authController.login); | ||
| authRouter.post('/logout', authController.logout); | ||
| authRouter.get('/refresh', authController.refresh); | ||
| authRouter.get('/activation/:email/:token', authController.activate); |
There was a problem hiding this comment.
CHECKLIST #15-19 violation: Password reset functionality is completely missing. There are no endpoints for: (1) asking for email, (2) sending reset email, (3) displaying reset confirmation page with password/confirmation fields, or (4) showing success page. You need to add endpoints like POST /auth/forgot-password, GET /auth/reset-password/:token, and POST /auth/reset-password.
| import { Router } from 'express'; | ||
| import { authController } from './auth.controller.js'; | ||
|
|
||
| export const authRouter = Router(); | ||
|
|
||
| authRouter.post('/register', authController.register); | ||
| authRouter.post('/login', authController.login); | ||
| authRouter.post('/logout', authController.logout); | ||
| authRouter.get('/refresh', authController.refresh); | ||
| authRouter.get('/activation/:email/:token', authController.activate); |
There was a problem hiding this comment.
|
|
||
| function getByToken(token: string) { |
There was a problem hiding this comment.
CHECKLIST #1 violation: The create function accepts email, password, and activationToken but doesn't accept a name parameter. It derives defaultName from email (line 9). Registration must accept a user-provided name field, not derive it from email.
| return db.token.create({ | ||
| data: { userId, token }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
CHECKLIST #20-#23 violation: The users.router.ts only has a GET /users endpoint. Profile management endpoints are missing: (1) GET /users/me for current user profile, (2) PATCH /users/me/name for changing name, (3) PATCH /users/me/password for changing password, (4) PATCH /users/me/email for changing email.
|
|
||
| function deleteSingleToken(token: string) { | ||
| return db.token.deleteMany({ | ||
| where: { token }, | ||
| }); |
There was a problem hiding this comment.
Minor: deleteSingleToken function is defined in tokens.repository but never used. Either implement it or remove it to avoid dead code.
| import { usersRouter } from './api/users.router.ts'; | ||
|
|
||
| export function createApp() { | ||
| const app = express(); |
There was a problem hiding this comment.
usersRouter imports profileController from './users.controller.js' but users.controller.ts only exports usersController with getAll. The profileController reference will cause a runtime error. This needs to be removed or the controller needs to be created.
|
|
||
| app.use(express.json()); | ||
|
|
||
| app.use( | ||
| cors({ | ||
| origin: process.env.CLIENT_URL, | ||
| credentials: true, | ||
| }), | ||
| ); | ||
|
|
||
| app.use('/auth', authRouter); | ||
| app.use('/users', usersRouter); |
There was a problem hiding this comment.
usersRepository.create() doesn't accept a name parameter - it derives a default name from email (line 9). Per checklist item #1, registration must accept and store a user-provided name field. Update the create function signature and database call to include name.
| } | ||
| } | ||
|
|
||
| function validatePassword(password: string): string | void { | ||
| if (!password) { | ||
| return 'Password is required'; | ||
| } | ||
|
|
||
| if (password.length < 6) { | ||
| return 'At least 6 characters'; |
There was a problem hiding this comment.
authMiddleware validates the token but doesn't attach user data to req.user or req.body.user. Downstream handlers (like profile management) won't have access to the authenticated user's ID. Consider attaching userData to req.user or creating a typed request augmentation.
|
|
||
| function normalize(user: User) { | ||
| return { | ||
| id: user.id, | ||
| email: user.email, | ||
| }; |
There was a problem hiding this comment.
userService.normalize() returns only { id, email } but excludes the user's name. Since registration accepts a name field (CHECKLIST #1), the normalized user object should include name for profile display purposes.
|
|
||
| export const userService = { | ||
| normalize, | ||
| validateEmail, | ||
| validatePassword, |
There was a problem hiding this comment.
CHECKLIST #22 violation: userService is missing a validation function for password change that checks old password verification, new password strength, and confirmation match. Consider adding validatePasswordChange(oldPassword, newPassword, confirmation): { oldPassword?: string, newPassword?: string, confirmation?: string }.
|
|
||
| export const userService = { | ||
| normalize, | ||
| validateEmail, | ||
| validatePassword, |
There was a problem hiding this comment.
CHECKLIST #17 violation: userService is missing a validation function for password reset that checks password and confirmation equality. The reset confirmation endpoint requires fields where 'password and confirmation fields must be equal'. Consider adding validatePasswordReset(password, confirmation): { password?: string, confirmation?: string }.
| await db.$disconnect(); | ||
| console.log('Server is closed'); | ||
|
|
||
| done(); | ||
| }); | ||
| }; | ||
|
|
There was a problem hiding this comment.
The auth middleware validates the token but doesn't attach user data to the request object. Other handlers (like profile endpoints) need access to req.user. Add req.user = userData after validation to make the user data available downstream.
| import { createApp } from './app.js'; | ||
| import { db } from './utils/db.js'; | ||
|
|
||
| const PORT = process.env.PORT || 3000; |
There was a problem hiding this comment.
The normalize function doesn't include the name field (CHECKLIST #21). The profile page needs to display the user's name. Add name: user.name to the returned object so it can be shown on the profile page.
|
|
||
| const userData = jwt.validateAccessToken(accessToken); | ||
|
|
||
| if (!userData) { | ||
| res.status(401).json({ message: 'Invalid token' }); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| next(); |
There was a problem hiding this comment.
The authMiddleware validates the token but doesn't attach the decoded user data to req.user or any request property. Downstream handlers (like profile controllers) need access to the authenticated user's information. Consider adding: (req as any).user = userData; after validation.
| Response as ExpressResponse, | ||
| NextFunction, | ||
| } from 'express'; | ||
| import { jwt } from '../utils/jwt.js'; |
There was a problem hiding this comment.
CHECKLIST #1 violation: The normalize function returns { id, email } but doesn't include name. Since registration must accept and store a name field, the normalized user should also include it for display on the profile page.
|
|
||
| if (!userData) { | ||
| res.status(401).json({ message: 'Invalid token' }); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| next(); | ||
| } |
There was a problem hiding this comment.
CHECKLIST #3: The validatePassword function only checks for minimum 6 characters. The task requires informing users about password rules. Consider adding a getPasswordRules() function that returns the requirements (e.g., 'Password must be at least 6 characters') or exporting a rules constant.
|
|
||
| function generateAccessToken(user: NormalizedUser) { | ||
| return jsonwebtoken.sign({ ...user }, ACCESS_SECRET, { expiresIn: '10s' }); |
There was a problem hiding this comment.
Practical issue: Access token expires in 10 seconds (expiresIn: '10s') - this is impractical for real-world use. Users won't be able to perform any meaningful action before the token expires. Change to '15m' (15 minutes) or similar reasonable duration.
|
|
||
| function generateRefreshToken(user: NormalizedUser) { | ||
| return jsonwebtoken.sign({ ...user }, REFRESH_SECRET, { expiresIn: '30s' }); |
There was a problem hiding this comment.
Practical issue: Refresh token expires in 30 seconds (expiresIn: '30s') - this defeats the purpose of refresh tokens which should provide persistent sessions. Change to '7d' (7 days) or similar reasonable duration.
| user: SMTP_USER, | ||
| pass: SMTP_PASSWORD, | ||
| }, | ||
| }); | ||
|
|
||
| export function send(email: string, subject: string, html: string) { |
There was a problem hiding this comment.
Practical issue: Access token expires in 10 seconds (expiresIn: '10s' at line 9) and refresh token in 30 seconds (expiresIn: '30s' at line 13). These values are impractical for real-world applications. Change access token to ~15 minutes ('15m') and refresh token to ~7 days ('7d').
|
|
||
| export const mailer = { | ||
| send, | ||
| sendActivationLink, |
There was a problem hiding this comment.
CHECKLIST #16 violation: mailer.ts is missing a sendResetLink function for password reset. This function should generate a reset link with a token and send it via email to allow users to reset their password. The current mailer only has activation link functionality.
|
|
||
| export const mailer = { | ||
| send, | ||
| sendActivationLink, |
There was a problem hiding this comment.
CHECKLIST #23 violation: mailer.ts is missing an email change notification function. When a user changes their email, the old email address must be notified about the change (as per the requirement: 'notify the old email about the change'). Add a sendEmailChangeNotification function.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation has made excellent progress from previous iterations—you've resolved the JWT expiration times, properly implemented auth middleware with req.user attachment, and have a solid foundation for the core authentication flows (registration, activation, login, logout). However, there are critical security gaps that need addressing before approval. In auth.controller.ts, the password reset endpoint must validate password === confirmation as required, and the updatePasswordProfile function should verify the old password before allowing changes. Additionally, the email change feature requires both password verification and a confirmation field for the new email. There's also a missing guestMiddleware in auth.router.ts to prevent authenticated users from accessing register/login pages. Finally, please review the syntax errors identified in the check run—several files have incomplete variable declarations or malformed imports that will prevent the code from running. This is an educational override to support your learning while you work through these remaining requirements. Focus on the security-related validations first, as these are essential for protecting user accounts.
✨ 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 { Router } from 'express'; | ||
| import { authController } from './auth.controller.js'; | ||
| import { authMiddleware } from '../middlewares/auth.middleware.js'; | ||
|
|
||
| export const authRouter = Router(); | ||
|
|
||
| authRouter.post('/register', authController.register); | ||
| authRouter.post('/login', authController.login); | ||
| authRouter.post('/activate/:email/:token', authController.activate); | ||
| authRouter.post('/refresh', authController.refresh); | ||
| authRouter.post('/logout', authController.logout); | ||
|
|
||
| // Нові маршрути для пароля | ||
| authRouter.post('/forgot-password', authController.forgotPassword); | ||
| authRouter.post('/reset-password', authController.resetPassword); | ||
|
|
||
| // Нові маршрути для профілю (захищені мідлварою) | ||
| authRouter.patch('/profile', authMiddleware, authController.updateProfile); | ||
| authRouter.patch('/profile/password', authMiddleware, authController.updatePassword); |
There was a problem hiding this comment.
The requirements specify register/login should be accessible only to non-authenticated users. There's authMiddleware for protected routes, but no guestMiddleware to prevent authenticated users from accessing these pages.
|
|
||
| const resetPassword: RequestHandler = async (req, res) => { | ||
| const { token, password } = req.body; | ||
|
|
||
| const error = userService.validatePassword(password); | ||
| if (error) { | ||
| return res.status(400).json({ errors: { password: error }, message: 'Validation error' }); | ||
| } | ||
|
|
||
| const user = await usersRepository.getByResetToken(token); | ||
| if (!user) { | ||
| return res.status(400).json({ message: 'Invalid or expired reset token' }); | ||
| } | ||
|
|
||
| const hashedPassword = await bcrypt.hash(password, 10); | ||
| await usersRepository.updatePassword(user.email, hashedPassword); | ||
| await usersRepository.updateResetToken(user.email, null); | ||
|
|
||
| res.json({ message: 'Password has been successfully reset' }); |
There was a problem hiding this comment.
The password reset endpoint is missing the confirmation field validation. Per requirements: "Reset Password confirmation page (with password and confirmation fields that must be equal)". Add validation to ensure password === confirmation before updating.
|
|
||
| const updatePasswordProfile: RequestHandler = async (req, res) => { | ||
| const { password } = req.body; | ||
| const currentUser = (req as any).user; | ||
|
|
||
| if (!currentUser) { | ||
| return res.status(401).json({ message: 'Unauthorized' }); | ||
| } | ||
|
|
||
| const error = userService.validatePassword(password); | ||
| if (error) { | ||
| return res.status(400).json({ errors: { password: error }, message: 'Validation error' }); | ||
| } | ||
|
|
||
| const hashedPassword = await bcrypt.hash(password, 10); | ||
| await usersRepository.updatePassword(currentUser.email, hashedPassword); | ||
|
|
||
| res.json({ message: 'Password updated successfully' }); |
There was a problem hiding this comment.
The profile password update endpoint doesn't require the old password. Per requirements: "change a password (require an old one, new password and confirmation)". This endpoint only validates the new password format.
|
|
||
| const updateProfile: RequestHandler = async (req, res) => { | ||
| const { name, email } = req.body; | ||
| const currentUser = (req as any).user; | ||
|
|
||
| if (!currentUser) { | ||
| return res.status(401).json({ message: 'Unauthorized' }); | ||
| } | ||
|
|
||
| const updateData: { name?: string; email?: string } = {}; | ||
|
|
||
| if (name) { | ||
| updateData.name = name; | ||
| } | ||
|
|
||
| if (email && email !== currentUser.email) { | ||
| const emailError = userService.validateEmail(email); | ||
| if (emailError) { | ||
| return res.status(400).json({ errors: { email: emailError }, message: 'Validation error' }); | ||
| } | ||
|
|
||
| const existingUser = await usersRepository.getByEmail(email); | ||
| if (existingUser) { | ||
| return res.status(400).json({ errors: { email: 'Email is already taken' }, message: 'Validation error' }); | ||
| } | ||
|
|
||
| updateData.email = email; | ||
| await mailer.sendEmailChangeNotification(currentUser.email); | ||
| } | ||
|
|
||
| const updatedUser = await usersRepository.updateProfile(currentUser.id, updateData); | ||
|
|
||
| res.json({ | ||
| user: userService.normalize(updatedUser), | ||
| }); |
There was a problem hiding this comment.
The email change endpoint doesn't require password verification. Per requirements: "To change an email you should type the password, confirm the new email". Add password field validation before allowing email change.
|
|
||
| const updateProfile: RequestHandler = async (req, res) => { |
There was a problem hiding this comment.
The email change endpoint doesn't have a confirmation field for the new email. Per requirements: "confirm the new email". The body only accepts email without a separate confirmation field.
No description provided.