added by MaksOther · Pull Request #240 · mate-academy/node_auth-app

MaksOther · 2026-03-01T15:25:07Z

No description provided.

mateacademy-ai-mentor

Review Summary

You've built a good architectural foundation for the application, but several critical issues and missing features prevent it from meeting the requirements. Changes are needed before this can be approved.

Here are the most important issues to address:

Critical Security Flaw: In src/controllers/todo.controller.js and src/services/todo.services.js, database queries are not scoped by user. This means any authenticated user can view, update, and delete todos belonging to any other user. You must use req.user.id to ensure users can only access their own data.
Data Persistence Bug: In src/index.js, the line User.sync({ force: true }) deletes all user data every time the server restarts. This option must be removed to allow user data to persist.
Missing Core Features: The application is missing several required features, including user logout, password reset, and profile management (changing name, password, email). The registration process in src/controllers/auth.controller.js is also missing the required name field.
Crashing Error Middleware: The error handler in src/middlewares/errorMiddlewares.js attempts to send two responses for a single error, which will crash the application. You need to add a return or an else block to prevent this.

Please focus on resolving these critical items.

✨ 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-01T15:27:37Z

src/index.js

+app.use("/todos", todoRouter);
+app.use("/", authRouter);
+
+app.use(errorMiddleware);


The error handling middleware should be registered after all other middleware and routes. This ensures it can act as a catch-all for any errors that occur during request processing in your application.

mateacademy-ai-mentor · 2026-03-01T15:27:37Z

src/index.js

+app.use("/", authRouter);
+
+app.use(errorMiddleware);
+app.use(cookieParser());


This cookieParser middleware should be registered before your routers. Middleware in Express is processed in order, so placing this before app.use("/todos", todoRouter) and app.use("/", authRouter) will ensure that req.cookies is populated and available in your route handlers.

mateacademy-ai-mentor · 2026-03-01T15:27:37Z

src/index.js

+client
+  .sync()
+  .then(async() => {
+    await User.sync({ force: true });


Using { force: true } causes the User table to be dropped and recreated on every application restart. This means any registered users will be deleted, which defeats the purpose of an authentication system. This option should be removed to allow user data to persist between server sessions.

mateacademy-ai-mentor · 2026-03-01T15:27:37Z

src/controllers/auth.controller.js

+};
+
+const registration = catchError(async (req, res) => {
+  const { email, password } = req.body;


The task description specifies that registration should use name, email, and password. The name field is missing here and is not saved when creating a new user.

mateacademy-ai-mentor · 2026-03-01T15:27:37Z

src/controllers/auth.controller.js

+  if (!user) {
+    throw ApiError.badRequest("No such user"); 
+  }


The requirements state that if a user is not active, you should ask them to activate their email. A check for the user's activation status is missing here before you proceed with the password validation.

mateacademy-ai-mentor · 2026-03-01T15:27:37Z

src/controllers/auth.controller.js

+});
+
+const refresh = (req, res) => {
+    const {refreshToken } = refresh.cookies;


It seems there's a typo. You are trying to access cookies from the refresh function itself. It should likely be req.cookies to get the cookies from the incoming request.

mateacademy-ai-mentor · 2026-03-01T15:27:37Z

src/controllers/auth.controller.js

+const refresh = (req, res) => {
+    const {refreshToken } = refresh.cookies;
+
+    const user = jwtService.verifyRefresh(refreshToken);


The jwtService defined in src/services/jwt.sevrices.js does not contain a verifyRefresh method. This will cause a runtime error. Additionally, this refresh function is defined but never exported or used in any routes.

mateacademy-ai-mentor · 2026-03-01T15:27:37Z

src/controllers/auth.controller.js

+export const AuthController = {
+  registration,
+  activate,
+  login,
+};


Several required features are missing from this controller, including logout, password reset, and profile management functions (changing name, password, email).

mateacademy-ai-mentor · 2026-03-01T15:27:37Z

src/controllers/todo.controller.js

+import * as todoService from "../services/todo.services.js";
+
+export const get = async (req, res) => {
+    const todos = await todoService.getAll();


The todoService.getAll() call retrieves all todos from the database. Since authentication is in place, this should be scoped to only fetch todos belonging to the currently logged-in user. You can get the user's information from req.user.

mateacademy-ai-mentor · 2026-03-01T15:27:37Z

src/controllers/todo.controller.js

+    const { title } = req.body;
+    if (!title) return res.sendStatus(422);
+
+    const todo = await todoService.create(title);


When creating a new todo, it needs to be associated with the user who is creating it. The todoService.create() method should probably accept a user ID from req.user to link the new todo to the user.

mateacademy-ai-mentor · 2026-03-01T15:27:41Z

src/controllers/todo.controller.js

+    const { items } = req.body;
+    if (!Array.isArray(items)) return res.sendStatus(422);
+
+    const updatedTodos = await todoService.updateMany(items);


The updateMany service call doesn't seem to be scoped to the current user. This could allow a user to update todos that don't belong to them. The user's ID from req.user should be used to ensure only their own todos are updated.

mateacademy-ai-mentor · 2026-03-01T15:27:41Z

src/controllers/todo.controller.js

+};
+
+export const patchOne = async (req, res) => {
+    const updatedTodo = await todoService.update(req.params.id, req.body);


This update operation is not restricted to the current user's todos. A user could potentially update another user's todo by providing its ID. Consider passing req.user.id to the update service to ensure ownership.

mateacademy-ai-mentor · 2026-03-01T15:27:41Z

src/controllers/todo.controller.js

+};
+
+export const getOne = async (req, res) => {
+    const todo = await todoService.getById(req.params.id);


This function retrieves a todo by its ID, but it doesn't check if the todo belongs to the current user. This could expose other users' data. The query in getById should also filter by the user ID from req.user.

mateacademy-ai-mentor · 2026-03-01T15:27:41Z

src/controllers/todo.controller.js

+};
+
+export const deleteOne = async (req, res) => {
+    const success = await todoService.remove(req.params.id);


The remove operation is not checking for todo ownership. This means any authenticated user can delete any todo if they know its ID. The delete operation should be scoped to the current user's todos using req.user.id.

mateacademy-ai-mentor · 2026-03-01T15:27:41Z

src/middlewares/errorMiddlewares.js

+    res.statusCode = 500;
+    res.send({
+      message: 'Server error'
+    })


After a response is sent for an ApiError inside the if block, this code will execute and attempt to send a second response. This will cause a runtime error. You should add a return statement inside the if block after res.send(), or wrap this generic error handling logic in an else block to prevent it from running for ApiError instances.

mateacademy-ai-mentor · 2026-03-01T15:27:41Z

src/routes/auth.route.js

+authRouter.post("/registration", catchError(AuthController.registration));
+authRouter.get("/activate/:activateToken", catchError(AuthController.activate));
+authRouter.post("/login", catchError(AuthController.login));


This router is missing several endpoints required by the task description. You need to add routes for:

Logout: A route to handle user logout.

Password Reset: Routes for initiating and confirming a password reset.

Profile Management: Routes for changing user name, password, and email.

Also, consider adding middleware to protect these routes based on authentication status (e.g., logout should only be for authenticated users, login for non-authenticated).

mateacademy-ai-mentor · 2026-03-01T15:27:41Z

src/services/jwt.sevrices.js

+export const jwtService = {
+  sign,
+  verify,
+};


The auth.controller.js includes a refresh function that attempts to call jwtService.verifyRefresh(), but that function is not defined here. To support token refreshing, you should implement separate logic for access and refresh tokens, as they typically have different secrets and expiration times. Consider adding signRefresh, verifyRefresh, signAccess, and verifyAccess functions to make the service more robust.

mateacademy-ai-mentor · 2026-03-01T15:27:41Z

src/services/email.services.js

+export const emailService = {
+  sendActivationEmail,
+  send,
+};


This service is responsible for sending emails, but it only implements the activation email functionality. According to the task requirements, you also need to send emails for password resets and to notify users when their email address has been changed. You should add functions here to handle those cases as well.

mateacademy-ai-mentor · 2026-03-01T15:27:41Z

src/services/todo.services.js

+import { User } from "../models/User.js";
+import { client } from "../utils/db.js";
+
+export const getAll = async () => {


This function should be scoped to a specific user. It needs to accept a userId and use it in the findAll query to return only the todos belonging to that user. For example: Todo.findAll({ where: { userId } }).

mateacademy-ai-mentor · 2026-03-01T15:27:41Z

src/services/todo.services.js

+  });
+};
+
+export const getById = async (id) => {


This function should also be scoped by user. It should accept a userId along with the id and find a todo that matches both. This prevents a user from accessing another user's todo by guessing its ID. The query should look something like Todo.findOne({ where: { id, userId } }).

mateacademy-ai-mentor · 2026-03-01T15:27:45Z

src/services/todo.services.js

+  return Todo.findByPk(id);
+};
+
+export const create = (title) => {


When creating a todo, it must be associated with the user who is creating it. This function needs to accept a userId and include it in the create call, like Todo.create({ title, userId }).

mateacademy-ai-mentor · 2026-03-01T15:27:45Z

src/services/todo.services.js

+  return Todo.create({ title });
+};
+
+export const update = async (id, data, transaction = null) => {


The update function needs to ensure it's updating a todo that belongs to the current user. It should accept a userId and pass it to the getById call to correctly retrieve the user's todo. Without this, a user could update another user's data.

mateacademy-ai-mentor · 2026-03-01T15:27:45Z

src/services/todo.services.js

+  return currentTodo;
+};
+
+export const updateMany = async (items) => {


This function orchestrates multiple updates and fetches all todos. It needs to accept a userId to pass down to both the update function (inside the map) and the final getAll() call to ensure all operations are scoped to the correct user.

mateacademy-ai-mentor · 2026-03-01T15:27:45Z

src/services/todo.services.js

+  return User.findOne({ where : { email }});
+}
+
+export const remove = async (id) => {


This function should only delete a todo if it belongs to the current user. It needs to accept a userId and include it in the where clause of the destroy call: Todo.destroy({ where: { id, userId } }).

mateacademy-ai-mentor

Review Summary

You've made excellent progress on the project, especially with fixing the critical security and data persistence issues from the previous review. However, I am requesting changes because several core features required by the task are not yet implemented.

The main blockers are:

Missing Core Features: The password reset and profile management (changing name, password, email) flows are completely missing. Please implement the necessary routes, controllers, and services for these features.
Incomplete Models: The User model in src/models/User.js is missing the required name field, which will cause registration to fail. The Todo model also needs a userId field to associate todos with their owners.
Missing 404 Handler: The task requires the application to return a 404 error for any undefined routes. This middleware should be added in src/index.js.

Once these key requirements are addressed, your project will be in great shape. Keep up the great work!

✨ 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-01T15:42:46Z

src/controllers/auth.controller.js

+export const AuthController = {
+  registration,
+  activate,
+  login,
+  refresh,
+  logout,
+};


The core functionality for password reset (requesting a reset link, updating the password) and profile management (changing name, password, or email) is missing. According to the task description, you need to implement API endpoints to handle these user actions.

mateacademy-ai-mentor · 2026-03-01T15:42:47Z

src/index.js

+app.use("/todos", todoRouter);
+app.use("/", authRouter);
+
+app.use(errorMiddleware);


The task requires the application to return a 404 status for any routes that are not defined. You should add a middleware here, after all other routes but before the errorMiddleware, to handle these cases and respond with a 404 error.

mateacademy-ai-mentor · 2026-03-01T15:42:47Z

src/index.js

+client
+  .sync()
+  .then(async () => {
+    await User.sync();


The client.sync() call on line 24 already synchronizes all models (including User) that have been defined using the client instance. This means User.sync() is redundant and can be removed.

mateacademy-ai-mentor · 2026-03-01T15:42:47Z

src/models/User.js

+export const User = client.define('user', {
+    email: {
+        type: DataTypes.STRING,
+        allowNull: false,
+        unique: true,
+    },
+    password: {
+        type: DataTypes.STRING,
+        allowNull: false,
+    }, 
+    activateToken: {
+        type: DataTypes.STRING,
+    }
+
+})


The task requires users to register with a name, but this field is missing from the model definition. You should add a name property here to ensure it's saved to the database.

mateacademy-ai-mentor · 2026-03-01T15:42:47Z

src/models/User.js

+    activateToken: {
+        type: DataTypes.STRING,
+    }


To implement the required password reset functionality, you'll need a way to track reset requests. Consider adding a field like passwordResetToken to this model to store the unique token sent to the user's email, similar to how activateToken works.

mateacademy-ai-mentor · 2026-03-01T15:42:47Z

src/models/Todo.js

+  {
+    id: {
+      type: DataTypes.UUID,
+      defaultValue: DataTypes.UUIDV4,
+      primaryKey: true,
+    },
+    title: {
+      type: DataTypes.STRING,
+      allowNull: false,
+    },
+    completed: {
+      type: DataTypes.BOOLEAN,
+      allowNull: false,
+      defaultValue: false,
+    },


This model is missing a userId field to associate a todo with a user. The queries in todo.services.js rely on this field to ensure users can only access their own todos (e.g., where: { userId }). You should add a userId field here that references the User model.

mateacademy-ai-mentor · 2026-03-01T15:42:47Z

src/services/email.services.js

+export const emailService = {
+  sendActivationEmail,
+  send,
+};


The task requirements include sending emails for password resets and notifying users of email address changes. You should add functions here to handle these scenarios, similar to how sendActivationEmail is implemented. Centralizing all email logic in this service is a good practice.

mateacademy-ai-mentor · 2026-03-01T15:42:47Z

src/routes/auth.route.js

+
+authRouter.post("/registration", catchError(AuthController.registration));
+authRouter.get("/activate/:activateToken", catchError(AuthController.activate));
+authRouter.post("/login", catchError(AuthController.login));
+authRouter.post("/refresh", catchError(AuthController.refresh));
+
+authRouter.post("/logout", authMiddlewares, catchError(AuthController.logout));


This router is missing several required endpoints. According to the task description, you need to implement routes for the password reset flow (e.g., requesting a reset token, submitting a new password) and for the profile page (e.g., changing name, password, and email).

mateacademy-ai-mentor · 2026-03-01T15:42:47Z

src/services/jwt.sevrices.js

+  sign: signAccess,
+  verify: verifyAccess,


The properties sign and verify are duplicates of signAccess and verifyAccess respectively. To avoid redundancy and make the service's API clearer, you could remove these two lines.

mateacademy-ai-mentor · 2026-03-01T15:42:47Z

src/services/todo.services.js

+export function findByEmail(email) {
+  return User.findOne({ where: { email } });
+}


This function is related to the User model, not the Todo model. For better separation of concerns, it would be more appropriate to move this logic into a dedicated user.service.js file.

mateacademy-ai-mentor

Review Summary

I'm approving your submission as an educational override to help you keep moving forward. You've made excellent progress on the core authentication flows and overall application structure!

To make your solution fully functional and secure, please focus on these key areas next. In src/controllers/auth.controller.js, the password reset and change password functions need to validate a confirmation field to prevent user typos. The email change flow also needs to verify the user's current password for security. Finally, you'll need to implement the sendPasswordResetEmail and sendEmailChangedNotification functions in src/services/email.services.js to prevent runtime errors.

Also, be sure to fix the linting issues from the check runs, like removing console.log statements. Keep up the great work!

✨ 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-01T15:56:40Z

src/controllers/auth.controller.js

+    const resetToken = uuidv4();
+    user.passwordResetToken = resetToken;
+    await user.save();
+    await emailService.sendPasswordResetEmail(email, resetToken);


The sendPasswordResetEmail function is not implemented or exported in the emailService. This call will result in a runtime error.

mateacademy-ai-mentor · 2026-03-01T15:56:40Z

src/controllers/auth.controller.js

+
+const confirmPasswordReset = catchError(async (req, res) => {
+  const { token } = req.params;
+  const { password } = req.body;


The task requires a confirmation field for resetting the password. You need to get this field from the request body and verify that it matches the password field.

mateacademy-ai-mentor · 2026-03-01T15:56:40Z

src/controllers/auth.controller.js

+});
+
+const changePassword = catchError(async (req, res) => {
+  const { oldPassword, newPassword } = req.body;


The requirements state that changing a password requires a confirmation field for the new password. You're missing the logic to get this field and validate that it's identical to newPassword.

mateacademy-ai-mentor · 2026-03-01T15:56:40Z

src/controllers/auth.controller.js

+});
+
+const changeEmail = catchError(async (req, res) => {
+  const { email } = req.body;


According to the task description, changing an email requires verifying the user's current password and also confirming the new email address (e.g., with a second emailConfirmation field). These validation steps are missing.

mateacademy-ai-mentor · 2026-03-01T15:56:40Z

src/controllers/auth.controller.js

+  user.email = email;
+  await user.save();
+
+  await emailService.sendEmailChangedNotification(oldEmail, email);


The sendEmailChangedNotification function is not implemented or exported in the emailService, which will cause a runtime error. This function is required to notify the user at their old email address about the change.

mateacademy-ai-mentor · 2026-03-01T15:56:40Z

src/services/email.services.js

+export const emailService = {
+  sendActivationEmail,
+  send,
+};


This service is missing implementations for sending password reset emails and email change notifications. The auth.controller.js calls sendPasswordResetEmail and sendEmailChangedNotification, but these functions are not defined here. Please implement and export them to fulfill the task requirements.

added

36eaa14

mateacademy-ai-mentor suggested changes Mar 1, 2026

View reviewed changes

mateacademy-ai-mentor reviewed Mar 1, 2026

View reviewed changes

fixed

c41e1ea

MaksOther requested a review from mateacademy-ai-mentor March 1, 2026 15:40

mateacademy-ai-mentor suggested changes Mar 1, 2026

View reviewed changes

aded

bbe12d4

MaksOther requested a review from mateacademy-ai-mentor March 1, 2026 15:54

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

View reviewed changes

Conversation

MaksOther 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!

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!

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!

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