Solution by Dima-Semenov · Pull Request #241 · mate-academy/node_auth-app

Dima-Semenov · 2026-03-01T20:41:00Z

No description provided.

mateacademy-ai-mentor

Review Summary

This is a strong submission with a well-organized project structure, and you've correctly implemented many of the core features. However, some changes are needed to address a few critical issues before I can approve it.

First, a key feature is missing: the ability for a user to change their email. This requires a new route, controller logic, and a mailer function to notify the old email address. Additionally, there are a few security and functionality bugs that need attention:

Unprotected Logout: The /auth/logout route in src/routers/auth.router.js needs to be protected by authMiddleware so only authenticated users can access it.
Broken Token Invalidation: The logic in src/middlewares/authMiddleware.js to check if a password has changed is incorrect. You should compare the token's issued-at timestamp (iat) with the passwordChangedAt time from the user record.
Critical Error Handling: Your src/middlewares/errorMiddleware.js will crash the server by trying to send multiple responses. The fallback error handler should be in an else block to ensure it only runs if the ApiError check fails.
Missing Password Confirmation: The password reset controller in src/controllers/auth.controller.js must verify that the password and confirmation fields from the request body are identical.

Once these issues are resolved, your project will fully meet the requirements. You're on the right track!

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-03-01T20:43:59Z

src/controllers/auth.controller.js

+  await sendAuthentication(res, user);
+};
+
+const logout = async (req, res) => {


According to the requirements, the logout feature should be for authenticated users only. This endpoint isn't protected by authentication middleware, meaning anyone can call it. It should be protected to ensure only logged-in users can log out.

mateacademy-ai-mentor · 2026-03-01T20:43:59Z

src/controllers/auth.controller.js

+};
+
+const passwordReset = async (req, res) => {
+  const { token, newPassword } = req.body || {};


The task requirements state that the password reset confirmation should have password and confirmation fields that must be equal. This function only accepts newPassword. You need to destructure the confirmation password from req.body as well and add logic to verify that it matches newPassword.

mateacademy-ai-mentor · 2026-03-01T20:43:59Z

src/controllers/user.controller.js

+export const userController = {
+  getAllActiveUsers,
+  updateUser,
+  changePassword,
+};


The functionality to change a user's email is missing. According to the requirements, the profile page should allow a user to change their email. You should implement a controller function to handle this, which includes checking the user's password, validating the new email, and then updating it.

mateacademy-ai-mentor · 2026-03-01T20:43:59Z

src/exeptions/api.error.js

+  static tooManyRequests(message = DEFAULT_ERRORS.TOO_MANY_REQUESTS) {
+    return new ApiError({
+      message,
+      errors,


The errors variable is used here but it's not defined as a parameter for the tooManyRequests function. This will cause a ReferenceError when this method is called. You should add errors to the function's signature, similar to the other static methods in this class.

mateacademy-ai-mentor · 2026-03-01T20:43:59Z

src/middlewares/errorMiddleware.js

@@ -0,0 +1,14 @@
+export const errorMiddleware = (error, req, res, next) => {
+  if (error instanceof Error) {


The condition error instanceof Error is too broad. A generic Error object doesn't have a .status property, so res.status(error.status) would cause the application to crash. This check should be more specific, like error instanceof ApiError, to handle your custom errors correctly.

mateacademy-ai-mentor · 2026-03-01T20:43:59Z

src/routers/auth.router.js

+router.post('/login', authController.login);
+router.post('/activation-link', authController.resendActivationLink);
+router.post('/refresh', authController.refresh);
+router.post('/logout', authController.logout);


According to the task description, the logout feature should only be available to authenticated users. This route is currently public and should be protected by the authMiddleware to enforce this rule.

mateacademy-ai-mentor · 2026-03-01T20:43:59Z

src/services/user.service.js

+const validatePassword = (password) => {
+  if (!password) {
+    return 'Password is required';
+  }
+
+  if (password.length < 6) {
+    return 'At least 6 characters';
+  }
+};


The task requires informing users about password 'rules' (plural) and checking them. This function currently only checks for minimum length. Consider adding more validation rules, such as requiring a mix of character types (e.g., uppercase, numbers, special characters), to make passwords more secure and fully meet the requirement.

mateacademy-ai-mentor · 2026-03-01T20:43:59Z

src/services/user.service.js

+const getActiveUsers = async () => {
+  const users = await User.findAll({ where: { activationToken: null } });
+
+  return users;


For consistency, consider having this function return an array of plain objects, similar to getUserById and getUserByEmail. You can achieve this by mapping over the results and calling .get({ plain: true }) on each user instance.

mateacademy-ai-mentor · 2026-03-01T20:43:59Z

src/routers/user.router.js

+  userController.changePassword,
+);
+
+export const userRouter = router;


The task requirements specify that a user should be able to change their email from the profile page. This router is missing a dedicated endpoint to handle that functionality. You'll need to add a new route, for example, router.patch('/:id/email', ...) and connect it to a new controller function for changing the email.

mateacademy-ai-mentor · 2026-03-01T20:43:59Z

src/utils/mailer.js

+export const mailer = {
+  send,
+  sendActivationLink,
+  sendResetPasswordLink,
+};


The task requires notifying the old email address when a user changes their email. This module is missing a dedicated function for sending that specific notification email. You should add a function, for example sendEmailChangeNotification, to handle this requirement.

mateacademy-ai-mentor

Review Summary

Great job on implementing the new features and addressing much of the previous feedback! However, there are two critical issues that need to be fixed. First, the errorMiddleware in src/middlewares/errorMiddleware.js will still crash the server because it tries to send two responses for a single error. Please use an if...else structure to ensure only one response is sent. Second, in src/controllers/user.controller.js, when an authenticated user changes their password, you must update the passwordChangedAt timestamp and delete their existing refresh tokens to invalidate old sessions. This is a crucial security step that is currently missing. Once these two issues are resolved, your solution will be ready for approval!

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-03-02T18:24:55Z

src/controllers/user.controller.js

+  }
+
+  const hashedPassword = await passwordUtils.getHashedPassword(newPassword);
+  await userService.updateUser(user.id, { password: hashedPassword });


When the password is changed, you should also update the passwordChangedAt timestamp. This is crucial for the token invalidation logic in authMiddleware to work correctly. Additionally, it's good practice to delete all of the user's existing refresh tokens by calling tokenService.deleteByUserId(user.id) to log them out from other devices.

mateacademy-ai-mentor · 2026-03-02T18:24:56Z

src/middlewares/errorMiddleware.js

+export const errorMiddleware = (error, req, res, next) => {
+  if (error instanceof ApiError) {
+    res.status(error.status).send({
+      message: error.message,
+      errors: error.errors,
+    });
+  }
+
+  if (error) {
+    res.status(500).send({ message: 'Server error' });
+  }
+
+  next();
+};


This middleware implementation is incorrect and will crash the server. If error is an ApiError, the first if block will execute and send a response. The code will then proceed to the second if (error) block, which will also be true, and attempt to send a second response, causing a fatal error.

To fix this:

The second if block should be an else block, so it only acts as a fallback for non-ApiError types.

The next() call at the end must be removed. Once a response is sent, the request cycle is finished, and next() should not be called.

mateacademy-ai-mentor · 2026-03-02T18:24:56Z

src/services/user.service.js

+const generateToken = () => {
+  return uuidv4();
+};


This generateToken function seems to be unused throughout the application. The controllers consistently use authService.generateActivationToken to create tokens. It's a good practice to remove dead code like this to improve maintainability.

mateacademy-ai-mentor

Review Summary

Excellent work on this project, especially with fixing the errorMiddleware! I am approving this submission as an educational override to help you progress, but there is one critical security issue that needs your attention.

In src/controllers/user.controller.js, the changePassword function incorrectly calls tokenService.deleteByToken(user.id). This function expects a token string, not a user ID, which means old sessions are not being invalidated after a password change.

To resolve this, you should use the deleteByUserId(user.id) function instead. This will correctly remove all of the user's refresh tokens and secure their account.

Once you make that small change, your implementation will be perfect. Keep up the fantastic effort!

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Rate AI review example ↩

mateacademy-ai-mentor · 2026-03-02T18:36:27Z

src/controllers/user.controller.js

+    password: hashedPassword,
+    passwordChangedAt: new Date(),
+  });
+  await tokenService.deleteByToken(user.id);


This function call is incorrect. The deleteByToken function expects a refresh token string as its argument, but you are passing the user's ID. To correctly invalidate all of the user's sessions after a password change, you should use the deleteByUserId function instead.

Solution

fe2a0f6

mateacademy-ai-mentor suggested changes Mar 1, 2026

View reviewed changes

Fix comments

337026e

Dima-Semenov requested a review from mateacademy-ai-mentor March 2, 2026 18:22

mateacademy-ai-mentor suggested changes Mar 2, 2026

View reviewed changes

Fix comments v2

8456f00

Dima-Semenov requested a review from mateacademy-ai-mentor March 2, 2026 18:34

mateacademy-ai-mentor approved these changes Mar 2, 2026

View reviewed changes

Fix comments v3

baabe05

Dima-Semenov requested a review from mateacademy-ai-mentor March 2, 2026 18:39

		@@ -0,0 +1,14 @@
		export const errorMiddleware = (error, req, res, next) => {
		if (error instanceof Error) {

Conversation

Dima-Semenov commented Mar 1, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants