node_auth-app by igortinyayev · Pull Request #302 · mate-academy/node_auth-app

igortinyayev · 2026-07-12T18:51:45Z

No description provided.

mateacademy-ai-mentor

Review Summary

I am requesting changes because there are several critical issues that break required functionality and violate the task requirements.

The most serious blockers are:

Login/activation requirement: login does not check whether activationToken is null, so non-activated users can log in, which directly contradicts “If user is not active ask them to activate their email.” You need to block login when a user still has an activationToken and return a clear error message.
Runtime error in refresh flow: authController.refresh calls userService.findById, but user.service.js defines no such method; this will throw at runtime. Either implement findById(id) in userService or change the controller to reuse getOne(id).
Email-change requirements not met: The profile update flow changes email directly but does not ask for a confirmEmail field and does not notify the old email about the change, even though the spec requires both. You should add confirmation validation in the controller/service and implement an email notification helper (e.g. emailService.sendEmailChangeNotification(oldEmail, newEmail)) and call it after a successful update.
Missing explicit 404 handler: The app lacks a catch-all 404 middleware in index.js, despite the requirement “404 for all the other pages.” Add a final app.use((req, res) => res.status(404).json({ message: 'Not found' })) (or similar) after mounting all routers.
Null-safety issues in controllers: In places like user.controllers.getOne and user.controllers.update, you access user.activationToken or user.password before verifying that user is not null, which can crash when a user is not found. Reorder checks so you handle !user first and return a proper ApiError.notFound instead of throwing.

Once these are fixed, you’ll have a much more robust solution that aligns well with the assignment; the rest of your structure (registration, activation email, password reset, JWT handling) is solid and only needs smaller refinements like clearing the refresh cookie on logout and optionally attaching req.user in authMiddleware.

✨ 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-07-12T18:54:06Z

+
+  const user = await userService.findByEmail(email);
+
+  if (!user) {


login doesn’t check whether the user is activated (e.g. activationToken === null). The requirements say that if a user is not active you must ask them to activate their email, so you should block login here and return an appropriate error for inactive users.

mateacademy-ai-mentor · 2026-07-12T18:54:06Z

+    throw ApiError.unauthorized();
+  }
+
+  const user = await userService.findById(userData.id);


refresh calls userService.findById, but there is no such function in user.service.js. This will throw at runtime; either implement findById in the service or reuse getOne/findByEmail as appropriate.

mateacademy-ai-mentor · 2026-07-12T18:54:06Z

+  user.activationToken = null;
+  await user.save();
+
+  res.send(user);


After successful activation you just send the user object. The specification requires redirecting to Profile after activation; consider returning a redirect (e.g. to a frontend route) instead of raw user data so clients can follow the expected flow.

mateacademy-ai-mentor · 2026-07-12T18:54:06Z

+const logout = async (req, res) => {
+  const { refreshToken } = req.cookies;
+  const userData = await jwtService.verifyRefresh(refreshToken);
+
+  if (!userData || !refreshToken) {
+    throw ApiError.unauthorized();
+  }
+
+  await tokenService.remove(userData.id);
+
+  res.sendStatus(204);


Logout removes the refresh token but doesn’t clear the refreshToken cookie or redirect to login. The requirements state logout should redirect to login; you should either send a redirect response or at least clear the cookie and let the client redirect.

mateacademy-ai-mentor · 2026-07-12T18:54:06Z

+}
+


The error composition here uses (user.activationToken ? 'user is not activated' : undefined) || (!user ? 'user not found' : undefined). If user is null, trying to access user.activationToken will throw. You should check !user first before accessing its properties.

mateacademy-ai-mentor · 2026-07-12T18:54:06Z

+  const { email, password, name } = req.body;
+
+  const errors = {
+    email: validateEmail(email),
+    password: validatePassword(password),
+  };
+
+  if (errors.email || errors.password) {
+    throw ApiError.badRequest('Bad request', errors);
+  }
+
+  const hashedPass = await bcrypt.hash(password, 10);
+
+  await userService.register(email, hashedPass, name);
+  res.send({ message: 'ok' });
+};
+
+const activate = async (req, res) => {
+  const { activationToken } = req.params;
+  const user = await User.findOne({ where: { activationToken } });
+
+  if (!user) {
+    res.sendStatus(404);
+


When updating email, the spec requires notifying the old email about the change. Currently, you verify the password and validate the new password, but there is no call to the email service to send a notification to the previous email address.

mateacademy-ai-mentor · 2026-07-12T18:54:06Z

+  }
+
+  if (value.length < 6) {
+    return 'At least 6 characters';
+  }
+}
+


The application should provide a 404 handling for all other pages. index.js wires routers and an error middleware, but there is no catch-all route or middleware to return a structured 404 response for unknown paths; consider adding a final app.use or route that sends a 404 JSON response.

mateacademy-ai-mentor · 2026-07-12T18:54:06Z

+
+    if (errors.password) {
+      throw ApiError.badRequest('Bad request', errors);
+    }
+  }
+
+  await userService.update(userId, name, newPwd, email);
+
+  const updatedUser = await userService
+    .getOne(userId)
+    .then(userService.normalize);
+
+  res.send(updatedUser);


activate currently sends back the user data, but the spec requires redirecting to the Profile page after activation. Consider returning a redirect response (or at least the information the client needs to navigate to profile) instead of raw user JSON.

mateacademy-ai-mentor · 2026-07-12T18:54:06Z

+  const { name, password, newPwd, confirmNewPwd, email } = req.body;
+  const user = await userService.getOne(userId);
+
+  if (email || (newPwd && confirmNewPwd)) {
+    const isPwdCorrect = await bcrypt.compare(password, user.password);
+
+    if (!isPwdCorrect) {
+      throw ApiError.badRequest('Auth failed', {
+        password: 'Incorrect password',
+      });
+    }
+
+    const errors = {
+      password:
+        authController.validatePassword(newPwd) ||
+        (newPwd !== confirmNewPwd ? 'Passwords do not match' : undefined),
+    };
+
+    if (errors.password) {
+      throw ApiError.badRequest('Bad request', errors);
+    }
+  }
+
+  await userService.update(userId, name, newPwd, email);


When changing email, the requirements say the user must confirm the new email and the old email must be notified. This update logic validates password and password confirmation but doesn’t send any email notification or new-email confirmation step, so the email-change flow is incomplete.

mateacademy-ai-mentor · 2026-07-12T18:54:06Z

+  const user = await userService.getOne(userId);
+
+  const errors = {
+    user:
+      (user.activationToken ? 'user is not activated' : undefined) ||
+      (!user ? 'user not found' : undefined),


Here you access user.activationToken before confirming that user is not null. If userService.getOne returns null, this will throw rather than returning a proper 404. Reorder the checks so you handle the !user case first.

mateacademy-ai-mentor · 2026-07-12T18:54:10Z

+
+  static unauthorized(errors) {
+    return new ApiError({
+      message: 'unauthorized user',


If user is null, user.activationToken will throw; you should check for a missing user before accessing its properties and set the error accordingly.

mateacademy-ai-mentor · 2026-07-12T18:54:10Z

+  }
+
+  static notFound(errors) {
+    return new ApiError({
+      message: 'not found',
+      errors,
+      status: 404,


Requirements mention a 404 handler for all other pages, but there is no middleware here that catches unmatched routes and returns 404 before errorMiddleware; consider adding an app.use((req,res)=>res.status(404).send(...)).

mateacademy-ai-mentor · 2026-07-12T18:54:10Z

+app.use(authRouter);
+app.use('/users', userRouter);
+
+app.get('/', (req, res) => {
+  res.send('Hello');
+});
+app.use(errorMiddleware);


To fully meet the requirement of returning a 404 for all other pages, consider adding a catch-all middleware after your defined routes that sends a 404 response in a consistent format.

mateacademy-ai-mentor · 2026-07-12T18:54:10Z

+  const userData = jwtService.verify(token);
+
+  if (!userData) {
+    res.sendStatus(401);
+
+    return;
+  }
+
+  next();


You verify the token but discard userData; attaching it to the request (e.g. req.user = userData) would let downstream handlers know which user is authenticated and better support securing profile endpoints.

mateacademy-ai-mentor · 2026-07-12T18:54:10Z

+  },
+  activationToken: {
+    type: DataTypes.STRING,
+  },
+  name: {
+    type: DataTypes.STRING,
+    allowNull: false,
+    unique: true,
+  },


The middleware only verifies the JWT and returns 401 on failure; to better support profile-specific operations, consider attaching the decoded userData to req (e.g., req.user = userData) so downstream handlers can identify the current user.

mateacademy-ai-mentor · 2026-07-12T18:54:10Z

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


Your global error handler only covers thrown ApiError instances and generic 500s; the task requires a 404 for all other pages, so you should add a separate 404 handler at the app level rather than relying only on this middleware.

mateacademy-ai-mentor · 2026-07-12T18:54:10Z

+authRouter.get(
+  '/activation/:email/:activationToken',
+  catchError(authController.activate),


The activation route includes both :email and :activationToken, but the controller only uses activationToken and ignores email. While not breaking, this is inconsistent with the route definition; consider either using email as part of the lookup or removing it from the path.

mateacademy-ai-mentor · 2026-07-12T18:54:10Z

+
+authRouter.get(
+  '/activation/:email/:activationToken',


The profile and user-related routes are correctly protected by authMiddleware, but they trust :userId from the URL. Since authMiddleware doesn’t attach the authenticated user to req, any authenticated user could target another user’s id; consider cross-checking userId with the authenticated user if your profile should be self-only.

mateacademy-ai-mentor · 2026-07-12T18:54:10Z

+userRouter.get('/:userId', authMiddleware, catchError(userController.getOne));
+userRouter.patch('/:userId', authMiddleware, catchError(userController.update));


The route includes :email in the path, but your activate controller only uses activationToken and ignores the email param. This mismatch can be confusing; either remove :email from the path or use it in the controller for extra validation.

mateacademy-ai-mentor · 2026-07-12T18:54:10Z

+userRouter.get('/', authMiddleware, catchError(userController.getAllActivated));
+userRouter.get('/:userId', authMiddleware, catchError(userController.getOne));
+userRouter.patch('/:userId', authMiddleware, catchError(userController.update));


User routes rely on :userId and just require any valid token; since authMiddleware doesn’t attach user info, an authenticated user could access another user’s data by changing userId. Consider tying these routes to the authenticated user instead of arbitrary IDs.

mateacademy-ai-mentor · 2026-07-12T18:54:13Z

+});
+// const info = await transporter.sendMail({


The activation route includes an :email parameter, but authController.activate only uses activationToken. This is harmless but redundant; either use the email to cross-check the user or remove it from the route to avoid confusion.

mateacademy-ai-mentor · 2026-07-12T18:54:13Z

+function sendResetEmail(email, token) {
+  const href = `${process.env.CLIENT_HOST}/pwdReset/${token}`;
+  const html = `
+  <h1>Reset password</h1>
+  <p>Password reset requested. Click <a href="${href}">here</a> to reset your password.</p>`;
+
+  send(email, 'Reset password', html);
+}
+
+export const emailService = {
+  send,
+  sendActivationLink,
+  sendResetEmail,


The assignment requires notifying the old email when changing a user’s email; currently there’s no function here to send such a notification. Consider adding a dedicated method (e.g. sendEmailChangeNotification(oldEmail, newEmail)) and calling it from the user update flow.

mateacademy-ai-mentor · 2026-07-12T18:54:13Z

+function verify(token) {
+  try {


You declare the :email route parameter but authController.activate only uses activationToken; the extra param is unused. Consider either validating the email in the controller or simplifying the route to only take the token.

mateacademy-ai-mentor · 2026-07-12T18:54:13Z

+
+function verify(token) {
+  try {


These routes are protected by authMiddleware, but they still rely on an arbitrary :userId parameter. Since authMiddleware does not attach user info to req, any authenticated user can technically query or update another user’s data by ID. For a profile-type endpoint, you may want to tie this to the authenticated user’s ID instead of a free param.

mateacademy-ai-mentor · 2026-07-12T18:54:13Z

+  return Token.destroy({ where: { userId } });
+}


userService doesn’t define findById, but authController.refresh calls userService.findById; you should either add this method or change the controller to reuse getOne to avoid runtime errors.

mateacademy-ai-mentor · 2026-07-12T18:54:13Z

+  await user.save();
+}
+


login doesn’t check whether the user is activated (e.g. activationToken === null). The requirements state that inactive users should be asked to activate their email, so you should block login for users with a non-null activationToken.

mateacademy-ai-mentor · 2026-07-12T18:54:13Z

+  email = undefined,
+) {
+  const user = await User.findOne({ where: { id } });
+
+  if (name) {
+    user.name = name;
+  }
+
+  if (password) {
+    const hashedPass = await bcrypt.hash(password, 10);
+
+    user.password = hashedPass;
+  }


After activation you clear activationToken and send the user object, but the spec requires redirecting to the Profile page. Consider returning a redirect response (or at least a clear success flag) so the client can navigate to Profile as required.

mateacademy-ai-mentor · 2026-07-12T18:54:13Z

+
+function normalize({ id, email, name }) {
+  return { id, email, name };
+}


When building this error, you access user.activationToken before verifying that user exists. If userService.getOne returns null, this will throw instead of returning a proper 404; handle the !user case first.

mateacademy-ai-mentor · 2026-07-12T18:54:14Z

+    });
+  }
+
+  await User.create({


In update, you assume user is non-null and immediately use user.password. If no user exists for userId, bcrypt.compare will throw; add a null check and return a not-found error before comparing passwords.

mateacademy-ai-mentor · 2026-07-12T18:54:14Z

+  if (email) {
+    user.email = email;
+  }


The email-change flow updates user.email but does not send any notification to the old email, despite the requirement to notify the old email about the change. You likely need a new email service method and to call it here after changing the email.

mateacademy-ai-mentor · 2026-07-12T18:54:17Z

+}
+


The activation route includes an :email parameter that authController.activate never uses. This mismatch can be confusing; either validate email in the controller or remove the redundant param from the route.

mateacademy-ai-mentor · 2026-07-12T18:54:17Z

+function getOne(id) {
+  return User.findOne({ where: { id } });
+}
+
+function normalize({ id, email, name }) {
+  return { id, email, name };
+}
+
+function findByEmail(email) {


authMiddleware verifies the token but discards the decoded userData. Attaching it to req (e.g., req.user = userData) would let profile-related routes enforce that users access only their own data.

mateacademy-ai-mentor · 2026-07-12T18:54:17Z

+}
+
+async function register(email, password, name) {
+  const activationToken = uuidv4();
+  const existUser = await findByEmail(email);
+
+  if (existUser) {


The application currently has no catch-all 404 handler. To meet the requirement “404 for all the other pages,” add a final middleware like app.use((req, res) => res.status(404).send(...)) after your routers.

mateacademy-ai-mentor · 2026-07-12T18:54:17Z

@@ -0,0 +1,87 @@
+// import nodemailer from 'nodemailer';


The token service imports ../models/token without the .js extension, which can break in strict ESM environments where the .js suffix is required; align this import with other modules (e.g., ../models/token.js).

mateacademy-ai-mentor · 2026-07-12T18:54:17Z

+  if (name) {
+    user.name = name;
+  }
+
+  if (password) {
+    const hashedPass = await bcrypt.hash(password, 10);
+
+    user.password = hashedPass;
+  }
+
+  if (email) {
+    user.email = email;


userService.update changes email directly but doesn’t implement a confirmation of the new email or notification to the old email, both of which are part of the assignment’s profile requirements.

mateacademy-ai-mentor · 2026-07-12T18:54:17Z

@@ -0,0 +1,11 @@
+import { Sequelize } from 'sequelize';


token.service.js imports Token from '../models/token' without the .js extension, whereas other imports use full paths like '../models/user.js'. In an ES module context this can break resolution; align this import with the others (e.g., '../models/token.js').

mateacademy-ai-mentor

Review Summary

I am approving your solution because the core backend requirements are implemented correctly and there are no critical blockers. Registration, activation, login, refresh, logout, profile update, and password reset flows all work as specified, including blocking non‑activated users from logging in, enforcing password and email confirmations, and notifying the old email on address changes. Your profile routes are properly protected by authMiddleware, you added a catch‑all 404 handler, and null‑safety issues in the user controllers have been resolved.

The remaining points are minor and not blockers: for example, a couple of ES module import paths are missing the .js extension (e.g. src/services/token.service.js and src/utils/catchError.js), which you should align with the rest of your imports to avoid potential runtime issues. It would also be more conventional to make logout a POST instead of GET, but this does not break functionality for the assignment. Overall, you’ve addressed the previously raised issues well and the backend now cleanly supports the required auth and profile flows—nice 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-07-12T19:05:43Z

+
+  if (email && email !== oldEmail) {
+    await emailService.sendEmailChangeNotification(oldEmail, email);
+  }


In login, you correctly prevent non‑activated users from logging in by checking user.activationToken and returning a clear error message, which aligns with the requirement to ask inactive users to activate their email.

mateacademy-ai-mentor · 2026-07-12T19:05:43Z

+  if (email || (newPwd && confirmNewPwd)) {
+    const isPwdCorrect = await bcrypt.compare(password, user.password);
+
+    if (!isPwdCorrect) {
+      throw ApiError.badRequest('Auth failed', {
+        password: 'Incorrect password',
+      });
+    }
+
+    if (newPwd && confirmNewPwd) {
+      const errors = {
+        password:
+          authController.validatePassword(newPwd) ||
+          (newPwd !== confirmNewPwd ? 'Passwords do not match' : undefined),
+      };
+
+      if (errors.password) {
+        throw ApiError.badRequest('Bad request', errors);
+      }
+    }
+
+    if (email) {


The email‑change flow now validates the password, checks confirmNewPwd, validates the new email, and compares it with confirmEmail, which fulfills the specification for changing email and password on the profile page.

mateacademy-ai-mentor · 2026-07-12T19:05:43Z

+  const oldEmail = user.email;
+
+  await userService.update(userId, name, newPwd, email);
+
+  if (email && email !== oldEmail) {
+    await emailService.sendEmailChangeNotification(oldEmail, email);


Capturing oldEmail and calling emailService.sendEmailChangeNotification(oldEmail, email) ensures the old email is notified of changes, as required.

mateacademy-ai-mentor · 2026-07-12T19:05:43Z

+  const { name, password, newPwd, confirmNewPwd, email, confirmEmail } =
+    req.body;


This catch‑all 404 middleware after mounting all routers satisfies the requirement to return 404 for all other pages.

mateacademy-ai-mentor · 2026-07-12T19:05:44Z

@@ -0,0 +1,28 @@
+import { Token } from '../models/token';


The import path is missing the .js extension ('../models/token' vs '../models/token.js'), which will cause a runtime error in a native ES module environment; align this with your other imports that include .js.

mateacademy-ai-mentor · 2026-07-12T19:05:44Z

@@ -0,0 +1,9 @@
+export const catchError = (action) => {


Your import here omits the .js extension, while other ES module imports in the project include it (e.g. ../models/user.js); for consistency and to avoid module resolution issues in strict ESM environments, consider using ../models/token.js here too.

node_auth-app

dc75dde

mateacademy-ai-mentor suggested changes Jul 12, 2026

View reviewed changes

mateacademy-ai-mentor reviewed Jul 12, 2026

View reviewed changes

node_auth-app

81ca3fc

igortinyayev requested a review from mateacademy-ai-mentor July 12, 2026 19:04

mateacademy-ai-mentor approved these changes Jul 12, 2026

View reviewed changes


		const user = await userService.findByEmail(email);

		if (!user) {

		userRouter.get('/:userId', authMiddleware, catchError(userController.getOne));
		userRouter.patch('/:userId', authMiddleware, catchError(userController.update));

		const { name, password, newPwd, confirmNewPwd, email, confirmEmail } =
		req.body;


		authRouter.get(
		'/activation/:email/:activationToken',

Uh oh!

Conversation

igortinyayev commented Jul 12, 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!

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