added solution#274
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation covers most core authentication functionality, but there are critical issues that need to be addressed. First, the auth routes (registration, activation, login, password reset) lack middleware to restrict access based on authentication status - these endpoints should only be accessible to non-authenticated users per checklist items #2, #5, #9, and #15, and logout should only be for authenticated users per #13. Currently these routes are accessible to everyone. Second, the password validation only checks minimum length (6 characters) without informing users about actual password rules as required by checklist item #3. Additionally, there's a typo on line ~47 in profile.controller.js where 'Password is incorect' should be 'Password is incorrect'. The API also returns tokens after activation and login but doesn't provide redirect URLs for the frontend as specified in checklist items #7 and #11. Please add the appropriate middleware to enforce authentication-based access control on auth routes, enhance password validation messaging, fix the typo, and consider including redirect URLs in API responses.
✨ 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 function createServer() { | ||
| const app = express(); |
There was a problem hiding this comment.
Registration endpoint lacks middleware to ensure it's only accessible to non-authenticated users (checklist item #2). Consider adding a middleware that blocks authenticated users from accessing registration.
|
|
||
| app.use( |
There was a problem hiding this comment.
Password validation only checks for minimum length (6 characters), but the description mentions 'rules for a password' (checklist item #3). Consider adding more validation rules (uppercase, numbers, special characters) and informing users about them.
| }); | ||
| } | ||
|
|
||
| const isOldPasswordValid = await bcrypt.compare(oldPassword, user.password); | ||
|
|
||
| if (!isOldPasswordValid) { | ||
| return res.status(401).json({ | ||
| errors: { oldPassword: 'Current password is incorrect' }, | ||
| message: 'Validation error', | ||
| }); | ||
| } | ||
|
|
||
| const hasNewPasswordError = userService.validatePassword(newPassword); | ||
|
|
||
| if (hasNewPasswordError) { | ||
| return res.status(400).json({ | ||
| errors: { newPassword: hasNewPasswordError }, |
There was a problem hiding this comment.
The activate function returns authentication tokens after activation instead of redirecting to Profile as required by checklist item #7. Consider either returning a redirect URL for the frontend or redirecting directly from the backend.
|
|
||
| await mailer.sendChangeEmailNotification(oldEmail, newEmail); | ||
|
|
||
| res.json({ | ||
| message: 'Email changed. Please confirm your new email address', | ||
| }); | ||
| }; | ||
|
|
||
| export const profileController = { | ||
| getProfile, | ||
| updateName, |
There was a problem hiding this comment.
The logout function only sends 204 status without redirecting to the login page as required by checklist item #14.
| const { id } = req.user; | ||
|
|
||
| const user = await usersRepository.getById(id); | ||
|
|
||
| if (!user) { | ||
| return res.status(404).json({ | ||
| message: 'User not found', | ||
| }); | ||
| } | ||
|
|
||
| res.json({ user: userService.normalize(user) }); | ||
| }; | ||
|
|
||
| const updateName = async (req, res) => { | ||
| const { name } = req.body; | ||
| const { id } = req.user; | ||
|
|
||
| if (!name) { | ||
| return res.status(400).json({ | ||
| errors: { name: 'Name is required' }, | ||
| message: 'Validation error', | ||
| }); | ||
| } | ||
|
|
||
| const hasNameError = userService.validateName(name); | ||
|
|
||
| if (hasNameError) { | ||
| return res.status(400).json({ | ||
| errors: { name: hasNameError }, | ||
| message: 'Validation error', | ||
| }); | ||
| } | ||
|
|
||
| const user = await usersRepository.getById(id); | ||
|
|
||
| if (!user) { | ||
| return res.status(404).json({ | ||
| message: 'User not found', | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
| import { catchError } from '../utils/catchError.js'; | ||
| import { profileController } from '../controllers/profile.controller.js'; | ||
|
|
||
| export const profileRouter = Router(); |
There was a problem hiding this comment.
Registration endpoint is missing middleware to restrict access to non-authenticated users only (checklist item #2). Consider adding a middleware that blocks authenticated users from registering.
| profileRouter.use(authMiddleware); | ||
|
|
||
| profileRouter.get('/', catchError(profileController.getProfile)); | ||
| profileRouter.patch('/name', catchError(profileController.updateName)); |
There was a problem hiding this comment.
Activation endpoint is missing middleware to restrict access to non-authenticated users (checklist item #5).
| profileRouter.get('/', catchError(profileController.getProfile)); | ||
| profileRouter.patch('/name', catchError(profileController.updateName)); | ||
| profileRouter.patch('/password', catchError(profileController.updatePassword)); | ||
| profileRouter.patch('/email', catchError(profileController.updateEmail)); |
There was a problem hiding this comment.
Login endpoint is missing middleware to restrict access to non-authenticated users (checklist item #9).
| function normalize({ id, email, name }) { | ||
| return { id, email, name }; | ||
| } | ||
|
|
||
| function validateEmail(email) { | ||
| const emailPattern = /^[\w.+-]+@([\w-]+\.){1,3}[\w-]{2,}$/; | ||
|
|
||
| if (!email) { | ||
| return 'Email is required'; | ||
| } | ||
|
|
||
| if (!emailPattern.test(email)) { | ||
| return 'Email is not valid'; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| function validatePassword(password) { | ||
| if (!password) { | ||
| return 'Password is required'; | ||
| } | ||
|
|
||
| if (password.length < 6) { | ||
| return 'At least 6 characters'; | ||
| } |
There was a problem hiding this comment.
Auth routes (registration, activation, login, logout, password reset) are missing middleware to restrict access based on authentication status. According to checklist items #2, #5, #9, #13, and #15, registration/activation/login/password reset should be for non-authenticated users only, while logout should be for authenticated users. Consider adding middleware to check authentication status for these routes.
|
|
||
| function validatePassword(password) { | ||
| if (!password) { | ||
| return 'Password is required'; | ||
| } | ||
|
|
||
| if (password.length < 6) { | ||
| return 'At least 6 characters'; | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
The password validation only checks minimum length (6 characters). The task mentions 'rules for a password' (checklist item #3). Consider adding more comprehensive validation (uppercase, numbers, special characters) and returning all errors at once instead of just the first one found.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This implementation requires changes due to two critical blockers. First, the /logout route in auth.router.js (line 21) lacks authMiddleware, which violates checklist item #13 requiring logout to be accessible only to authenticated users. Second, the typo 'Password is incorect' on line 135 of profile.controller.js was flagged in a previous review but remains unfixed - it should read 'Password is incorrect'. Additionally, there's a minor grammar issue in the password validation message on line 38 of user.service.js where 'must be contain' should be 'must contain'. These issues directly impact requirement compliance and code quality standards.
✨ 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
|
|
||
| await mailer.sendActivationLink(newEmail, activationToken); | ||
|
|
||
| await mailer.sendChangeEmailNotification(oldEmail, newEmail); |
There was a problem hiding this comment.
The /logout route lacks authentication middleware. According to the requirements, logout should only be accessible to authenticated users. Add authMiddleware to protect this route.
| res.clearCookie('refreshToken'); | ||
|
|
||
| res.status(401).json({ | ||
| message: 'Invalid token', |
There was a problem hiding this comment.
This typo was flagged in the previous review and remains unfixed. Change 'Password is incorect' to 'Password is incorrect'.
|
|
||
| const activate = async (req, res) => { | ||
| const { email, token } = req.params; | ||
|
|
There was a problem hiding this comment.
The logout route lacks authentication middleware. Per requirements, logout should only be accessible to authenticated users. Consider adding authMiddleware to this route.
| const hashedPassword = await bcrypt.hash(password, 10); | ||
| const user = await usersRepository.create({ | ||
| name, | ||
| email, |
There was a problem hiding this comment.
Minor grammar issue in error message: 'Password must be contain' should be 'Password must contain' (remove 'be').
| ); | ||
|
|
||
| authRouter.post('/login', guestMiddleware, catchError(authController.login)); | ||
| authRouter.get('/refresh', catchError(authController.refresh)); |
There was a problem hiding this comment.
The /logout route lacks authentication middleware. Per requirements, logout should only be accessible to authenticated users. Add authMiddleware from '../middlewares/auth.middleware.js' to this route to enforce this restriction.
| email: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, | ||
| unique: true, |
There was a problem hiding this comment.
The /logout route should be restricted to authenticated users only (per requirements: 'Logout (only authenticated)'). Add authMiddleware to this route.
| <a href="${link}">${link}</a> | ||
| `; | ||
|
|
||
| return send(email, 'Password reset', html); |
There was a problem hiding this comment.
Minor grammar issue: 'Password must be contain' should be 'Password must contain' (remove 'be')
| errors.push('Password must contain letters'); | ||
| } | ||
|
|
||
| if (errors.length > 0) { |
There was a problem hiding this comment.
Minor grammar issue: 'Password must be contain' should be 'Password must contain' (remove 'be').
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving this solution as an educational override to help you continue learning. Good progress: the previous HIGH severity issue (missing authMiddleware on /logout) has been fixed. However, there are two issues to address:
-
Fix the typo: In
src/controllers/profile.controller.jsline 135, change'Password is incorect'to'Password is incorrect'— this was flagged in the previous review and still needs correction. -
ESLint configuration: The project uses ES modules (
"type": "module"inpackage.json), but.eslintrc.jsuses CommonJS syntax (module.exports). Rename.eslintrc.jsto.eslintrc.cjsso Node.js treats it as CommonJS, resolving the linting error. -
Optional improvement: The activation route in
auth.router.jslacksguestMiddleware— while not critical, adding it would restrict the route to non-authenticated users only, matching the pattern used for login/registration routes.
Keep up the good work on the functional implementation!
✨ 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
| res.clearCookie('refreshToken'); | ||
|
|
||
| res.status(401).json({ | ||
| message: 'Invalid token', |
There was a problem hiding this comment.
Typo: 'incorect' should be 'incorrect'
| const app = express(); | ||
|
|
||
| app.use(express.json()); | ||
|
|
There was a problem hiding this comment.
The activation route lacks guestMiddleware. Per task requirements, this route should only be accessible to non-authenticated users (like login/registration routes on lines 14 and 16 which have this middleware).
| const isPasswordValid = await bcrypt.compare(password, user.password); | ||
|
|
||
| if (!isPasswordValid) { | ||
| return res.status(401).json({ |
There was a problem hiding this comment.
Typo: 'incorect' should be 'incorrect'
No description provided.