solution#295
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because one core requirement is not met and it directly affects the main auth flow. The spec says that a user should only be activated after email confirmation and that if a user is not active, the login must ask them to activate their email instead of logging them in; however, your login handler in src/controllers/authController.js never checks activationToken (or any active flag), so newly registered but unconfirmed users can log in normally. To fix this, after you fetch the user in login, add a check like if (user.activationToken) { /* return 4xx with a message telling them to activate via email */ } before issuing the JWT and cookie. A second, smaller but still spec-related issue is that /auth/activate/:activationToken is not guarded by isGuest, even though the activation flow is described as “only non authenticated”; it would be better to add isGuest to that route for consistency. There is also a lint/blocking issue from your checks: the console.log('here'); in src/controllers/authController.js (around line 168) violates the no-console rule and should be removed or replaced with proper logging. Other issues (like the possible null-destructuring in profilePage, typos in messages and route names, and the deletByUserId naming) are minor, but worth cleaning up after you fix the activation logic. Overall, you’ve implemented most of the required flows very well—once you enforce activation on login and address these small points, your solution will fully match 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
| migrations: { | ||
| path: "prisma/migrations", | ||
| }, | ||
| datasource: { |
There was a problem hiding this comment.
The .env file is correctly ignored here, which is important since your Prisma config and application code expect DB_URL and other secrets to come from environment variables, not be committed to version control.
| // This file was generated by Prisma, and assumes you have installed the following: | ||
| // npm install --save-dev prisma dotenv | ||
| import "dotenv/config"; | ||
| import { defineConfig } from "prisma/config"; |
There was a problem hiding this comment.
This migration defines change_email_tokens with a tokenHash column, but your repository code uses a token field and a later migration (20260613105456_init) converts tokenHash → token. Ensure that all migrations are applied in order; otherwise the runtime code (changeEmailTokenRepo) will not match this schema.
| "created_at" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
|
|
||
| CONSTRAINT "users_pkey" PRIMARY KEY ("id") | ||
| ); |
There was a problem hiding this comment.
You ignore .env here, which is good, but for a Node/Express app using Prisma and uploads you may also want to ensure any SQLite/Postgres dump files or other generated artifacts are ignored if you add them later. Not required for the current task, just something to keep in mind.
| DROP INDEX "change_email_tokens_tokenHash_key"; | ||
|
|
||
| -- AlterTable |
There was a problem hiding this comment.
verifyAccessToken can return null when the token is invalid, so destructuring { id } directly may throw. In practice this route is behind isAuthorize, but it would be safer to handle the null case (e.g., call verifyAccessToken once in middleware and reuse the decoded payload).
|
|
||
| const user = await userRepository.getByEmail(email); | ||
| const isValidPassword = await bcrypt.compare(password, user?.password || ''); | ||
|
|
||
| if (!user || !isValidPassword) { | ||
| return res.status(403).send('Invalid email or password'); | ||
| } | ||
|
|
||
| const normalizeUser = userRepository.normalize(user); | ||
| const accessToken = jwt.getAccessToken(normalizeUser); |
There was a problem hiding this comment.
After activation you only clear activationToken, but the login logic never checks it. The requirements demand blocking login for non-activated users and prompting them to activate their email, so login should verify that user.activationToken is null (or some other active flag) and respond accordingly instead of issuing an access token.
| const newPassword = data['new-password']; | ||
| const confirmPassword = data['confirm-password']; | ||
|
|
||
| console.log('here'); |
There was a problem hiding this comment.
This console.log('here'); appears to be leftover debug code; it’s not needed for the password reset flow and could be removed to keep logs clean.
| const register = async (req, res) => { | ||
| const { name, email, password } = req.body; | ||
|
|
There was a problem hiding this comment.
You destructure id from jwt.verifyAccessToken(accessToken) without checking for a null result. If, for any reason, this controller is called without valid middleware or with an invalid token, this will throw. Consider handling the case where verification fails (e.g., by checking for a truthy value before destructuring).
|
|
||
| const register = async (req, res) => { | ||
| const { name, email, password } = req.body; | ||
|
|
||
| const errors = { | ||
| name: checkData({ field: 'name', data: name }), | ||
| email: checkData({ field: 'email', data: email }), | ||
| password: checkData({ field: 'password', data: password }), | ||
| }; | ||
|
|
||
| if (Object.values(errors).some((e) => e)) { | ||
| return res.status(400).json(errors); | ||
| } | ||
|
|
||
| const emailIsTaken = await userRepository.getByEmail(email); | ||
|
|
||
| if (emailIsTaken) { |
There was a problem hiding this comment.
The repository exposes getToken, but in the current codebase only create and remove are used. While not harmful, you can remove unused functions to keep the module focused and reduce confusion.
| }; | ||
|
|
||
| const changeEmail = async (req, res) => { | ||
| const { password, email } = req.body; |
There was a problem hiding this comment.
Right now login doesn’t check whether the user is activated. The requirements say that if a user is not active you must ask them to activate their email. Consider checking a field derived from activation (e.g. activationToken not null) and returning an appropriate message instead of logging them in.
| if (invalidNewPassword) { | ||
| return res.status(400).json({ message: invalidNewPassword }); | ||
| } | ||
|
|
||
| if (data.new !== data.confirm) { | ||
| const response = { message: `the passwords dont match` }; | ||
|
|
||
| return res.status(400).json(response); | ||
| } | ||
|
|
||
| const hashadPassword = await bcrypt.hash(data.new, 10); | ||
| const updatedUser = await userRepository.updateUser( | ||
| { |
There was a problem hiding this comment.
After clearing the activationToken, you redirect to /profile but you never authenticate the user (set a JWT cookie). Because /profile is protected by middleware, the user will be redirected back to login instead of seeing their profile. You should fetch the user by token, normalize it, issue an access token (like in login), and then redirect.
| const profilePage = async (req, res) => { | ||
| const { accessToken } = req.cookies; | ||
| const { id } = jwt.verifyAccessToken(accessToken); |
There was a problem hiding this comment.
Because profilePage is behind isAuthorize, verifyAccessToken should always return a payload, but if anything slips through and it returns null, destructuring { id } will throw. A small defensive check here (or relying on the req.user set by middleware) would make this more robust.
| return db.changeEmailToken.findFirst({ | ||
| where: { token }, | ||
| include: { user: true }, |
There was a problem hiding this comment.
You destructure { id } from verifyAccessToken(accessToken) without checking for a null return. Because this route is protected by middleware it usually works, but adding a guard (e.g. if no payload, 401 or redirect) would prevent potential runtime errors if this function is reused.
| const deletByUserId = (userId) => { | ||
| return db.resetPasswordToken.deleteMany({ where: { userId } }); | ||
| }; |
There was a problem hiding this comment.
The function name deletByUserId has a small typo; while it works where used, renaming it to deleteByUserId (and updating imports) would make the code clearer and more self-explanatory.
|
|
||
| const changePassword = (password, id) => { |
There was a problem hiding this comment.
The function name deletByUserId works because you call it with the same spelling, but the missing ‘e’ in delete makes it slightly harder to read. Consider renaming it to deleteByUserId throughout the codebase for clarity (being careful to update all usages).
| cors({ origin: process.env.CLIENT_HOST, credentials: true }), | ||
| ); |
There was a problem hiding this comment.
The function name deletByUserId has a typo; since you call it with the same name it works, but it’s easy to misread and could cause confusion later. Consider renaming it to deleteByUserId and updating all usages for clarity.
| const isAuthorize = (req, res, next) => { | ||
| const { accessToken } = req.cookies; | ||
|
|
||
| if (!accessToken || !verifyAccessToken(accessToken)) { |
There was a problem hiding this comment.
You call verifyAccessToken here only to check validity; if you also attached the decoded payload to req (e.g. req.user = verifyAccessToken(accessToken)), your profile controllers wouldn’t need to call verifyAccessToken again and could avoid unsafe destructuring. Not required by the task, but it would improve code reuse and safety.
| const isGuest = (req, res, next) => { | ||
| const { accessToken } = req.cookies; | ||
|
|
||
| if (verifyAccessToken(accessToken)) { | ||
| return res.redirect(process.env.CLIENT_HOST + '/profile'); |
There was a problem hiding this comment.
This middleware correctly ensures that authenticated users (with a valid access token) are redirected away from guest-only pages like login/register to /profile, matching the “only non-authenticated” requirement.
| export const changeName = (payload) => { | ||
| return sendRequest('/profile/change-name', payload, 'PATCH'); | ||
| }; | ||
|
|
||
| export const changePassword = (payload) => { |
There was a problem hiding this comment.
This middleware correctly prevents authenticated users from accessing guest-only routes by redirecting them to /profile, which matches the requirement that registration, login, and password reset should only be available for non-authenticated users.
| form.addEventListener('submit', async (e) => { | ||
| e.preventDefault(); | ||
|
|
||
| const formInputs = document.querySelector('.wrapper-inputs'); | ||
|
|
||
| formInputs.classList.remove('bad-login'); | ||
|
|
||
| const formData = new FormData(e.currentTarget); | ||
| const loginData = Object.fromEntries(formData); | ||
| const respons = await authApi.login(loginData); | ||
|
|
||
| if (!respons.ok) { | ||
| return formInputs.classList.add('bad-login'); | ||
| } | ||
|
|
||
| const newUrl = `${window.location.origin}/profile`; | ||
|
|
||
| window.location.replace(newUrl); |
There was a problem hiding this comment.
This login handler redirects to /profile on any 2xx response without checking whether the server considers the account active. Once you add activation enforcement to the backend login, you might also want to surface that specific message here instead of only a generic 'bad login' state.
| if (!response.ok) { | ||
| const responsInfo = await response.json(); | ||
|
|
||
| if (Object.hasOwn(responsInfo, 'fields')) { | ||
| for (const key in responsInfo.fields) { | ||
| const input = form[key]; | ||
| const label = input.labels[0]; | ||
|
|
||
| label.classList.add('form--has-error'); | ||
| label.setAttribute('data-error', responsInfo.fields[key]); | ||
|
|
||
| input.addEventListener( | ||
| 'input', | ||
| () => { | ||
| label.classList.remove('form--has-error'); | ||
| label.removeAttribute('data-error'); | ||
| }, | ||
| { once: true }, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| window.location.replace('/auth/reset-password/succes'); | ||
| }); | ||
|
|
||
| // label.classList.add('form--has-error'); | ||
| // label.setAttribute('data-error', message); | ||
|
|
||
| // input.addEventListener( | ||
| // 'input', | ||
| // () => { | ||
| // label.classList.remove('form--has-error'); | ||
| // label.removeAttribute('data-error'); | ||
| // }, | ||
| // { once: true }, | ||
| // ); |
There was a problem hiding this comment.
The flow from this script matches the requirement: on error it shows per-field messages based on the server’s fields object, and on success it moves the user to the "success" page. The commented-out block below is dead code and can be removed to keep this file clean.
| 'input', | ||
| () => { | ||
| label.classList.remove('form--has-error'); |
There was a problem hiding this comment.
You correctly guard the activation info page (/auth/activate) with isGuest, but the actual activation handler (/activate/:activationToken) isn’t protected. The spec says the activation page should be available only for non-authenticated users, so consider adding isGuest here as well for consistency.
| () => { | ||
| label.classList.remove('form--has-error'); | ||
| label.removeAttribute('data-error'); | ||
| }, |
There was a problem hiding this comment.
The route and subsequent redirects use succes instead of success. This isn’t a functional bug since all references are consistent, but it’s a visible typo in your URL and could be confusing to users.
|
|
||
| box-sizing: border-box; | ||
| font-family: Roboto; | ||
| margin: 0; | ||
| padding: 0; | ||
| } | ||
|
|
||
| header { | ||
| height: var(--header-height); | ||
| background-color: #edededba; | ||
| box-shadow: 0 -10px 5px 14px #cdcccc5c; | ||
| } | ||
|
|
||
| main { | ||
| min-height: calc(100vh - var(--header-height)); | ||
| padding-inline: 40px; | ||
| background-color: #d6d4d429; | ||
| } | ||
|
|
||
| h1 { | ||
| color: #000; | ||
| font-size: 34px; | ||
| } | ||
|
|
||
| /* #region Header */ | ||
| .header_flex-wrap { | ||
| display: flex; | ||
| justify-content: space-between; | ||
| align-items: center; | ||
| height: 100%; | ||
| padding-inline: 40px; | ||
| } | ||
|
|
There was a problem hiding this comment.
This script correctly handles both validation errors and the success case for resetting a password. On success it redirects to /auth/reset-password/succes, where you render a success message with a link to login, which matches the password reset flow described in the requirements.
| .form-list { | ||
| margin-top: -10px; | ||
| list-style: none; | ||
|
|
||
| display: flex; | ||
| flex-direction: column; | ||
| gap: 3px; | ||
|
|
||
| & > :first-child { | ||
| font-size: 14px; | ||
| font-weight: bold; | ||
| margin-bottom: 5px; | ||
| padding: 0; | ||
|
|
||
| &::before { | ||
| display: none; | ||
| } | ||
| } | ||
|
|
||
| & > li { | ||
| display: flex; | ||
| align-items: center; | ||
| gap: 5px; | ||
| color: #ea8436; | ||
| font-size: 12px; | ||
| line-height: 12px; | ||
| font-weight: 500; | ||
|
|
||
| &::before { | ||
| content: ''; | ||
| width: 4px; | ||
| height: 4px; | ||
| background-color: #ea8436; | ||
| border-radius: 100%; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Your styles for .form-list and error states like .form--has-error and .wrapper-inputs.bad-login support showing password rules and validation feedback on the forms, helping meet the requirement to inform users about password rules and show errors clearly.
| h1 { | ||
| color: #000; | ||
| font-size: 34px; |
There was a problem hiding this comment.
All guest-only auth routes are consistently guarded by isGuest, but this activation endpoint is not. The requirements say the activation page is available only for non-authenticated users; consider also adding isGuest here to keep behavior consistent with that requirement.
| const authRouter = express.Router(); | ||
|
|
||
| authRouter.get('/register', isGuest, authController.registrationPage); | ||
| authRouter.post('/register', isGuest, authController.register); | ||
|
|
||
| authRouter.get('/login', isGuest, authController.loginPage); | ||
| authRouter.post('/login', isGuest, authController.login); | ||
|
|
||
| authRouter.get( | ||
| '/login/forgot-password', | ||
| isGuest, | ||
| authController.forgotPasswordPage, | ||
| ); | ||
|
|
||
| authRouter.post( | ||
| '/login/forgot-password', | ||
| isGuest, | ||
| authController.forgotPassword, | ||
| ); | ||
|
|
||
| authRouter.get('/activate', isGuest, authController.actvationPage); | ||
| authRouter.get('/activate/:activationToken', authController.activateUser); | ||
|
|
||
| authRouter.get('/reset-password/succes', isGuest, (req, res) => { | ||
| const message = `Password changed successfully. | ||
| <a style="color: #ea8436; text-decoration: none;" href="/auth/login"> | ||
| Login | ||
| </a>`; | ||
|
|
||
| res.render('showMessage', { isAuthorize: false, message }); | ||
| }); | ||
|
|
||
| authRouter.get( |
There was a problem hiding this comment.
This front-end script correctly handles the reset-password confirmation flow: it displays field-level validation errors from the server and, on success, redirects to /auth/reset-password/succes, which is your success page with a login link—matching the task’s reset-password requirements.
|
|
||
| authRouter.get('/activate', isGuest, authController.actvationPage); | ||
| authRouter.get('/activate/:activationToken', authController.activateUser); |
There was a problem hiding this comment.
All the guest-only auth pages (register, login, forgot/reset password, activation info, success page) are wrapped with isGuest, but the actual activation endpoint /activate/:activationToken is not. To fully align with “Activation page (only non authenticated)”, consider also applying isGuest here so logged-in users can’t reuse activation links.
| }; | ||
|
|
||
| if (!patterns[field].test(data)) { |
There was a problem hiding this comment.
Destructuring { id } from verifyAccessToken(accessToken) assumes the token is always valid; if it ever returns null (outside of middleware protection), this will throw. Even though isAuthorize protects the route, it’s safer to handle a possible null return before destructuring.
| return jwt.sign(payload, secretAccess, { expiresIn: '24h' }); | ||
| }; | ||
|
|
||
| const verifyAccessToken = (token) => { |
There was a problem hiding this comment.
The password regex here enforces at least one letter, one digit, minimum length of 6, and no whitespace, which gives concrete password rules and is used consistently in registration and password change flows to satisfy the requirement for checking password rules.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution because all core requirements of the task are implemented and work together coherently: registration with password rules and activation email, activation-only-before-login, proper guest-only /auth and auth-only /profile routing, logout, full password-reset flow including confirmation and success page, and profile updates for name, password (old/new/confirm), and email with confirmation and old-email notification. The Prisma migrations, repositories, controllers, middleware, routes, and frontend scripts are consistent with each other and with the database schema, so the main flows do not show functional blockers.
The remaining issues are minor: a few potential robustness edge cases where jwt.verifyAccessToken might return null and you destructure without a guard (e.g. in authController.js and profileController.js), small typos in function names and messages (like deletByUserId, 'Token not fund', 'the passwords dont match', /reset-password/succes), and some inconsistent error message formats. None of these prevent the app from meeting the assignment’s behavioral requirements, but they’re worth cleaning up when you have time. Overall, you’ve done a solid job implementing the specified flows end to end, and these small refinements would mainly improve robustness and polish rather than fix broken functionality.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| -- CreateTable | ||
| CREATE TABLE "users" ( | ||
| "id" UUID NOT NULL DEFAULT gen_random_uuid(), | ||
| "name" TEXT NOT NULL, |
There was a problem hiding this comment.
Right now the change_email_tokens table still uses a tokenHash column, but later migration 20260613105456_init switches to a token column. Make sure your Prisma schema and repository code are aligned with the latest migration (use the same column name/type throughout) to avoid runtime errors when querying or deleting tokens.
| try { | ||
| await userRepository.updateUser( | ||
| { activationToken: null }, | ||
| { activationToken }, | ||
| ); | ||
| } catch { |
There was a problem hiding this comment.
UPDATE with a WHERE activationToken = $1 that doesn’t match any row will throw in Prisma, so using a try/catch to return “Invalid activation token” is a reasonable way to enforce “user should be activated only after email confirmation” and handle bad tokens. Just be aware that any DB error here will be treated as “invalid token”; if you want more precise behavior later, you could explicitly check for PrismaClientKnownRequestError with the relevant code.
|
|
||
| if (user.activationToken) { | ||
| return res.status(403).json({ message: 'Account is not activated' }); |
There was a problem hiding this comment.
This new check against user.activationToken fixes the earlier requirement breach: users with a non-null activation token (i.e., not yet confirmed) are now prevented from logging in and are told their account is not activated, matching “If user is not active ask them to activate their email.”
| if (Object.values(errors.fields).some((e) => e)) { | ||
| return res.status(400).json(errors); | ||
| } else if (newPassword !== confirmPassword) { | ||
| return res.status(400).json({ | ||
| message: 'Passwords must match', | ||
| fields: { 'confirm-password': 'Passwords do not match' }, | ||
| }); | ||
| } | ||
|
|
||
| const hashedToken = hashToken(data.token || ''); | ||
| const findedToken = await resetPasswordTokenRepo.getToken(hashedToken); | ||
|
|
||
| if (!findedToken) { | ||
| return res.status(404).json({ message: 'Token not fund' }); |
There was a problem hiding this comment.
The reset-password logic correctly enforces both the general password rules via checkData and the equality constraint between new-password and confirm-password, as required by the spec. You might want to fix the small typo in the message: 'Token not fund' response for clarity, but it doesn’t affect functionality.
| const register = async (req, res) => { | ||
| const { name, email, password } = req.body; | ||
|
|
There was a problem hiding this comment.
Here you’re using jwt.verifyAccessToken(accessToken) without a null check before destructuring id. In normal use the isAuthorize middleware should guarantee a valid token, but if this controller is ever reused or verifyAccessToken returns null, this will throw. A simple guard (like checking the result before destructuring) would make this more robust.
|
|
||
| header { | ||
| height: var(--header-height); | ||
| background-color: #edededba; | ||
| box-shadow: 0 -10px 5px 14px #cdcccc5c; | ||
| } | ||
|
|
||
| main { | ||
| min-height: calc(100vh - var(--header-height)); | ||
| padding-inline: 40px; | ||
| background-color: #d6d4d429; | ||
| } | ||
|
|
||
| h1 { | ||
| color: #000; | ||
| font-size: 34px; | ||
| } | ||
|
|
||
| /* #region Header */ | ||
| .header_flex-wrap { | ||
| display: flex; | ||
| justify-content: space-between; | ||
| align-items: center; | ||
| height: 100%; | ||
| padding-inline: 40px; | ||
| } | ||
|
|
There was a problem hiding this comment.
The script correctly handles backend validation responses by applying the form--has-error styles to each invalid field and, on a successful response, redirects to your /auth/reset-password/succes page, which fulfills the “Success page with a link to login” requirement.
| main { | ||
| min-height: calc(100vh - var(--header-height)); | ||
| padding-inline: 40px; | ||
| background-color: #d6d4d429; | ||
| } | ||
|
|
||
| h1 { | ||
| color: #000; |
There was a problem hiding this comment.
This route wires the reset-password success page, rendering a message and including a link back to /auth/login, which matches the requirement to show a success page with a login link after resetting the password. You might want to fix the small typo in the path (succes → success) later for clarity, but it doesn’t break functionality.
| pagesRouter.get('/send-message', (req, res) => { | ||
| const { accessToken } = req.cookies; | ||
| const authorized = Boolean(jwt.verifyAccessToken(accessToken)); | ||
|
|
||
| res.render('showMessage', { | ||
| isAuthorize: authorized, | ||
| message: 'A message has been sent to your email.', | ||
| }); |
There was a problem hiding this comment.
The /send-message route is used as the “email sent” page in both the forgot-password and change-email flows, and it passes isAuthorize based on the cookie so the header can adapt to auth state. This satisfies the requirement to show an email-sent page without leaking whether an email exists.
| const getAccessToken = (payload) => { | ||
| return jwt.sign(payload, secretAccess, { expiresIn: '24h' }); | ||
| }; | ||
|
|
||
| const verifyAccessToken = (token) => { | ||
| try { | ||
| return jwt.verify(token, secretAccess); | ||
| } catch { | ||
| return null; |
There was a problem hiding this comment.
Because /profile is mounted behind isAuthorize, all of these routes (/, /logout, change-email/name/password) are correctly limited to authenticated users, which matches the “Profile page (only authenticated)” and logout requirements.
| const jwt = require('jsonwebtoken'); | ||
|
|
||
| const secretAccess = process.env.JWT_ACCESS_SECRET || ''; | ||
|
|
||
| const getAccessToken = (payload) => { | ||
| return jwt.sign(payload, secretAccess, { expiresIn: '24h' }); | ||
| }; | ||
|
|
||
| const verifyAccessToken = (token) => { |
There was a problem hiding this comment.
This utility enforces your validation rules for name, email, and password in one place. The password pattern (at least one letter and one digit, no spaces, length ≥ 6) is used throughout registration, profile changes, and reset flows, so it effectively implements the spec’s password rules check; you may want to tweak the error string ("is require") for grammar, but it doesn’t affect functionality.
No description provided.