done app#271
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This implementation has several critical issues that must be fixed before approval. First, the API calls in src/frontend/src/api/auth.ts are broken - register and logout paths are missing the leading /, which will cause 404 errors (e.g., client.post("register", ...) resolves incorrectly). Second, ActivationPage.tsx redirects to /login but the task explicitly requires 'redirect to Profile after the activation' - this is a direct requirements violation. Third, there's a significant security issue: user.route.ts line 9 has a GET /users endpoint with no authMiddleware, exposing all user data publicly. Fourth, auth.controller.ts line 23 says 'User with this name already exists' when it should reference email - this misleads users. Fifth, the activation handler in auth.controller.ts has no response when the token is invalid, leaving requests hanging. Finally, the backend validation only checks minimum 8 characters but the task requires informing AND checking password rules (uppercase, numbers) - these should be validated server-side as well. Please fix these critical issues and resubmit.
✨ 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
|
|
||
| if (userExists) { | ||
| return res | ||
| .status(400) | ||
| .json({ error: "User with this name already exists" }); |
There was a problem hiding this comment.
Error message says 'User with this name already exists' but this is an email check - should say 'email' not 'name'
| const register = async (req: Request, res: Response) => { | ||
| try { | ||
| const { email, password, name } = req.body; | ||
| const expiringTime = new Date(Date.now() + 30 * 60 * 1000); | ||
| const token = crypto.randomBytes(32).toString("hex"); | ||
| const userExists = await prisma.user.findUnique({ | ||
| where: { email: email }, | ||
| }); | ||
|
|
||
| if (userExists) { | ||
| return res | ||
| .status(400) | ||
| .json({ error: "User with this name already exists" }); | ||
| } | ||
|
|
||
| const saltRounds = 10; | ||
| const passwordHash = await bcrypt.hash(password, saltRounds); | ||
|
|
||
| const newUser = await prisma.user.create({ | ||
| data: { | ||
| name, | ||
| email, | ||
| passwordHash, | ||
| activationToken: token, | ||
| activationTokenExpiring: expiringTime, | ||
| }, | ||
| }); | ||
|
|
||
| sendActivationEmail(email, token); | ||
| res.json({ | ||
| message: "Check your email for verification", | ||
| }); | ||
| } catch (error) { | ||
| console.log(error); | ||
| res.status(500).json({ error: "server error" }); | ||
| } |
There was a problem hiding this comment.
The task requires informing users about password rules AND checking them. The backend validation (auth.validation.ts) only checks min 8 characters but frontend validates uppercase and numbers. For better security, consider adding password complexity validation to the backend schema (e.g., regex for uppercase and numbers).
| const activate = async (req: Request, res: Response) => { | ||
| const tokenFromUser = req.query.token; | ||
| if (typeof tokenFromUser === "string") { | ||
| const user = await prisma.user.findFirst({ | ||
| where: { | ||
| activationToken: tokenFromUser, // Пошук за токеном | ||
| activationTokenExpiring: { gt: new Date() }, // І перевірка часу одним махом | ||
| }, | ||
| }); | ||
|
|
||
| if (user?.isActivated === true) { | ||
| return res.status(400).json({ error: "user already activated" }); | ||
| } | ||
| if (!user) { | ||
| return res.status(404).json({ error: "Invalid token or timed out" }); | ||
| } | ||
|
|
||
| const activatedUser = await prisma.user.update({ | ||
| where: { id: user.id }, | ||
| data: { | ||
| isActivated: true, | ||
| activationToken: null, | ||
| activationTokenExpiring: null, | ||
| }, | ||
| }); | ||
|
|
||
| res.json({ message: "Successful activation" }); | ||
| } |
There was a problem hiding this comment.
If token is not a string, no response is sent to the client. Consider adding an error response for non-string token values.
|
|
||
| res.json({ message: "Successful activation" }); |
There was a problem hiding this comment.
Task requirement states 'redirect to Profile after the activation', but the frontend ActivationPage.tsx redirects to login. Either the backend should return a redirect indicator or the frontend should navigate to /profile instead of /login.
| import { NextFunction, Request, Response } from "express"; | ||
|
|
There was a problem hiding this comment.
The api.error.ts file is empty. This appears to be a placeholder for custom error classes that could be used throughout the application for consistent error handling.
| @@ -0,0 +1,8 @@ | |||
| import { PrismaClient } from "../generated/prisma/client.js"; | |||
| import { PrismaPg } from "@prisma/adapter-pg"; | |||
There was a problem hiding this comment.
The userService import is imported but never used in this file.
| import { PrismaClient } from "../generated/prisma/client.js"; | ||
| import { PrismaPg } from "@prisma/adapter-pg"; |
There was a problem hiding this comment.
The api.error.ts file is empty. If this was intended to contain custom API error classes, it should be implemented; otherwise, the file can be removed.
| if (!isPasswordValid) { | ||
| res.status(401); | ||
| res.json("Old password are incorrect"); |
There was a problem hiding this comment.
The response format is inconsistent. After setting status(401), calling res.json() with a string instead of an object creates an invalid response. Should use: res.status(401).json({ error: "Old password is incorrect" })
| const isPasswordValid = await bcrypt.compare(oldPassword, user.passwordHash); | ||
|
|
||
| if (!isPasswordValid) { | ||
| res.status(401); |
There was a problem hiding this comment.
Typo: 'are incorrect' should be 'is incorrect'
| async function getUsers(req: Request, res: Response) { | ||
| const users = await userService.getPublicUsers(); | ||
|
|
||
| res.json(users); |
There was a problem hiding this comment.
The getUsers endpoint returns all users, which isn't required for the profile page. The profile page should only allow viewing/editing the authenticated user's own data. This endpoint could be removed or secured differently if not needed elsewhere.
| import { authController } from "../controllers/auth.controller.js"; | ||
| import { validateMiddleware } from "../middlewares/validateMiddleware.js"; | ||
| import { | ||
| activationSchema, | ||
| confirmPasswordResetSchema, | ||
| loginSchema, | ||
| registerSchema, | ||
| requestPasswordResetSchema, |
There was a problem hiding this comment.
The errorMiddleware doesn't call next(error) which is the standard Express pattern for error handlers. This means errors aren't passed to subsequent handlers if any exist. Either remove the unused next parameter or call next(error).
|
|
||
| authRouter.post( | ||
| "/register", | ||
| validateMiddleware(registerSchema), | ||
| catchError(authController.register), | ||
| ); | ||
|
|
||
| authRouter.get( | ||
| "/activation", | ||
| validateMiddleware(activationSchema, "query"), | ||
| catchError(authController.activate), | ||
| ); | ||
|
|
||
| authRouter.post( | ||
| "/login", | ||
| validateMiddleware(loginSchema), | ||
| catchError(authController.login), | ||
| ); | ||
|
|
||
| authRouter.post("/logout", catchError(authController.logout)); | ||
|
|
||
| authRouter.get("/refresh", catchError(authController.refresh)); | ||
|
|
||
| authRouter.post( | ||
| "/password-reset", | ||
| validateMiddleware(requestPasswordResetSchema), | ||
| catchError(authController.requestPasswordReset), | ||
| ); | ||
|
|
||
| authRouter.post( | ||
| "/password-reset/confirm", | ||
| validateMiddleware(confirmPasswordResetSchema), | ||
| catchError(authController.confirmPasswordReset), | ||
| ); |
There was a problem hiding this comment.
Backend routes like /register, /login, /password-reset are accessible to both authenticated and non-authenticated users. The authentication guards (PublicRoute/ProtectedRoute) only exist on the frontend. For proper security, consider adding middleware to these routes on the backend to prevent authenticated users from accessing them.
| res.sendStatus(401); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
The logout endpoint should be protected by authMiddleware according to the requirements ('Logout (only authenticated)'). Currently anyone can call logout even without being authenticated.
| } | ||
|
|
||
| req.user = userData as PublicUser; | ||
| next(); |
There was a problem hiding this comment.
The refresh endpoint should also be protected or have additional validation. While token refresh typically doesn't require prior auth, it should verify the refresh token is valid.
| export const errorMiddleware = ( | ||
| error: Error, | ||
| req: Request, | ||
| res: Response, |
There was a problem hiding this comment.
The next parameter is unused in the error middleware. While this is valid for Express error handlers, you could remove it or add error logging for debugging purposes.
| import cookieParser from "cookie-parser"; | ||
| import { errorMiddleware } from "./middlewares/errorMiddleware.js"; | ||
|
|
||
|
|
There was a problem hiding this comment.
The GET /users endpoint has no authMiddleware, meaning any unauthenticated user can access all user data. This is a security issue. Either add authMiddleware or remove this endpoint if not needed.
| const port = process.env.PORT || 3002; | ||
| const corsOptions = { | ||
| origin: 'http://localhost:5173', |
There was a problem hiding this comment.
Missing semicolons at the end of these lines, making the code inconsistent with the rest of the file.
| import "dotenv/config"; | ||
| import cors from "cors"; | ||
| import { authRouter } from "./routes/auth.route.js"; | ||
| import { userRouter } from "./routes/user.route.js"; |
There was a problem hiding this comment.
Typos: 'Pasword' should be 'Password' and 'lenth' should be 'length'
| const port = process.env.PORT || 3002; | ||
| const corsOptions = { | ||
| origin: 'http://localhost:5173', | ||
| credentials: true, |
There was a problem hiding this comment.
The Ukrainian comment should be in English for consistency in a codebase.
|
|
||
| app.listen(port, () => { | ||
| console.log(`server running on localhost:${port}`); | ||
| }); |
There was a problem hiding this comment.
The task requires '404 for all the other pages'. The server.ts doesn't have a catch-all route handler for undefined paths that returns 404. Consider adding: app.use((req, res) => res.status(404).json({ message: 'Not Found' }));
|
|
||
| app.use(authRouter); | ||
| app.use('/users', userRouter); | ||
| app.use(errorMiddleware); |
There was a problem hiding this comment.
Typo: 'Acount activation' should be 'Account activation'
| const corsOptions = { | ||
| origin: 'http://localhost:5173', | ||
| credentials: true, | ||
| }; |
There was a problem hiding this comment.
Typo: 'Activate acount' should be 'Activate account'
| // Create a transporter using SMTP | ||
| const transporter = nodemailer.createTransport({ |
There was a problem hiding this comment.
Typo in error message: 'Pasword minimum lenth is 8' should be 'Password minimum length is 8'
| try { | ||
| const info = await transporter.sendMail({ | ||
| from: '"Zhovtany Vladyslav fullstack App" <>', // sender address | ||
| to: email, // list of recipients |
There was a problem hiding this comment.
Typo in email: 'Acount activation' should be 'Account activation'
|
|
||
| export async function sendActivationEmail(email: string, token: string) { | ||
| const href = `http://${process.env.CLIENT_HOST}:${process.env.CLIENT_PORT}/activation?token=${token}`; | ||
| const html = ` |
There was a problem hiding this comment.
Typo in email HTML: 'Activate acount' should be 'Activate account'
| service: "gmail", | ||
| auth: { | ||
| user: process.env.SMTP_USER, | ||
| pass: process.env.SMTP_PASS, |
There was a problem hiding this comment.
The GET /users/ endpoint returns all users, which isn't needed for the profile page. Users should only be able to view/edit their own profile data. Consider removing this endpoint or restricting it if not required.
| name: z.string().min(3).optional(), | ||
| }) | ||
|
|
||
| export const loginSchema = z.object({ |
There was a problem hiding this comment.
The GET "/" endpoint returns all users. This isn't needed for the profile page which should only show/edit the authenticated user's own data. Consider removing this or making it an admin-only endpoint.
| import z from "zod" | ||
|
|
||
| export const registerSchema = z.object({ | ||
| email: z.string().email("Invalid Email"), |
There was a problem hiding this comment.
Typo in validation message: 'Pasword minimum lenth is 8' should be 'Password minimum length is 8'
| }) | ||
|
|
||
| export const loginSchema = z.object({ | ||
| email: z.string().email("Invalid Email"), |
There was a problem hiding this comment.
Typo in validation message: 'Password are required' should be 'Password is required'
| import z from "zod" | ||
|
|
||
| export const registerSchema = z.object({ | ||
| email: z.string().email("Invalid Email"), |
There was a problem hiding this comment.
The task requires informing users about password rules AND checking them. The backend only validates min 8 characters, but frontend also validates uppercase letters and numbers. For better security, consider adding password complexity validation to the backend (regex for uppercase and numbers).
|
|
||
| export const updateEmailSchema = z.object({ | ||
| newEmail: z.string().email("Invalid email"), | ||
| password: z.string().min(1, "Password is required"), | ||
| }); | ||
|
|
There was a problem hiding this comment.
The requirements state '404 for all the other pages' but the server doesn't have a catch-all route handler for undefined paths. Add app.use((req, res) => res.status(404).json({ error: 'Not found' })) at the end before errorMiddleware.
| newName: z.string().min(3, "Name must be at least 3 characters"), | ||
| }); | ||
|
|
||
| export const updateEmailSchema = z.object({ |
There was a problem hiding this comment.
Typo in email subject: 'Acount activation' should be 'Account activation'
| }) | ||
|
|
||
| export const activationSchema = z.object({ | ||
| token: z.string().min(64, "Invalid token format"), // 32 байти у hex мають довжину 64 символи |
There was a problem hiding this comment.
Typo in email content: 'Activate acount' should be 'Activate account'
| import { catchError } from "../lib/catchError.js"; | ||
| import { validateMiddleware } from "../middlewares/validateMiddleware.js"; |
There was a problem hiding this comment.
Typo in error message: 'Pasword' should be 'Password' and 'lenth' should be 'length'.
| import { Prisma, User } from "../generated/prisma/client.js"; | ||
| import { prisma } from "../lib/db.js"; | ||
|
|
||
| const publicUserSelect = { |
There was a problem hiding this comment.
The token.service.ts file is empty and the token operations (refresh token creation, deletion) are handled directly in the auth controller. Either implement this service or remove the unused file.
| { | ||
| "compilerOptions": { | ||
| "target": "ES2022", |
There was a problem hiding this comment.
The token.service.ts file contains only an import statement with no actual functionality. This appears to be a placeholder that was never implemented. Either implement it or remove it if not needed.
| import jwt from "jsonwebtoken"; | ||
| import "dotenv/config"; | ||
| import { PublicUser } from "./user.service.js"; |
There was a problem hiding this comment.
The token.service.ts file is essentially empty - it imports prisma but has no exported functions or constants. This appears to be a placeholder that was never implemented. If not needed, it should be removed to avoid confusion.
| healthcheck: | ||
| test: ["CMD-SHELL", "pg_isready -U vladok -d mainTestDB"] |
There was a problem hiding this comment.
The Dockerfile references nginx.conf but this file is not provided in the project files. Without it, nginx won't have proper configuration and the frontend won't serve correctly.
| PORT: 3001 | ||
| HOST: localhost | ||
| CLIENT_HOST: localhost | ||
| CLIENT_PORT: 5173 |
There was a problem hiding this comment.
The DATABASE_URL includes ?schema=sample which is unusual. PostgreSQL schemas are typically 'public' by default, not 'sample'. This might cause issues if the migrations don't target this schema.
| <meta charset="UTF-8" /> | ||
| <link rel="icon" type="image/svg+xml" href="/favicon.svg" /> |
There was a problem hiding this comment.
The register path is missing the leading / - should be /register instead of register. Without the slash, axios will append to baseURL incorrectly (e.g., http://localhost:3001register instead of http://localhost:3001/register).
| <body> | ||
| <div id="root"></div> |
There was a problem hiding this comment.
The logout path is missing the leading / - should be /logout instead of logout. Same issue as register.
| register: (data: { email: string; password: string; name?: string }) => | ||
| client.post("register", data), | ||
|
|
||
| login: (data: { email: string; password: string }) => |
There was a problem hiding this comment.
Inconsistent path formatting: 'register' on line 4 doesn't have a leading slash, but '/login' on line 7 does. While axios handles both, this inconsistency could cause issues or confusion. Consider standardizing to use leading slashes for all paths.
| import { LoginPage } from "./pages/LoginPage"; | ||
| import { RegisterPage } from "./pages/RegisterPage"; | ||
| import { ActivationPage } from "./pages/ActivationPage"; | ||
| import { PasswordResetPage } from "./pages/PasswordResetPage"; |
There was a problem hiding this comment.
Inconsistent API paths - register doesn't have a leading slash while login, logout, and other endpoints do. While axios handles both formats, for consistency consider using the same format for all paths.
| import { useState, useEffect, type ReactNode } from "react"; | ||
| import { AuthContext } from "./authContext"; |
There was a problem hiding this comment.
The CSS imports @heroui/styles but this package doesn't appear to be referenced in the project files. This import could cause build errors if the package isn't installed. Consider checking if this dependency is needed.
| @@ -0,0 +1,114 @@ | |||
| @import "tailwindcss"; | |||
| @import "@heroui/styles"; | |||
There was a problem hiding this comment.
The @heroui/styles import on line 2 doesn't appear to be used anywhere in the CSS file. Consider removing it to avoid unnecessary dependencies or unused imports.
| } catch (err: any) { | ||
| setError(err.response?.data?.error || "Something went wrong"); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The task requirement states 'redirect to Profile after the activation', but this page redirects to /login after successful activation. Consider changing the redirect to navigate('/profile') instead.
| navigate("/profile"); | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any |
There was a problem hiding this comment.
The eslint-disable comment is valid in this case since we're handling axios error responses, but consider typing the error properly with AxiosError from axios instead.
|
|
||
| authApi.activate(token) | ||
| .then(() => { | ||
| setStatus("success"); |
There was a problem hiding this comment.
According to the requirements, after activation the user should be redirected to Profile. Currently the page redirects to login on line 22. Change navigate("/login") to navigate("/profile") to match the task requirements.
|
|
||
| const handleLogout = async () => { | ||
| await logout(); | ||
| navigate("/login"); |
There was a problem hiding this comment.
The password validation on this page only checks minimum length (8 characters), but the task requires informing users about AND checking password rules (uppercase letter, number). Consider adding validation for uppercase letters and numbers like the RegisterPage does.
| e.preventDefault(); | ||
| setPasswordError(""); | ||
| setPasswordSuccess(""); | ||
| if (newPassword !== confirmPassword) return setPasswordError("Passwords do not match"); |
There was a problem hiding this comment.
The password validation on the profile page only checks minimum length, but the RegisterPage validates for uppercase and numbers. For consistency, consider adding the same password complexity validation here.
| <div className="min-h-screen bg-zinc-900 flex items-center justify-center"> | ||
| <div className="bg-zinc-800 border border-zinc-700 p-8 rounded-xl shadow-xl w-full max-w-md text-center flex flex-col gap-4"> | ||
| <h1 className="text-2xl font-bold text-white">Check your email</h1> | ||
| <p className="text-zinc-300">If <strong className="text-white">{email}</strong> exists, you will receive a reset link.</p> |
There was a problem hiding this comment.
The PasswordResetConfirmPage only validates password length (min 8), but RegisterPage validates for uppercase and numbers as well. For consistency, consider adding the same password complexity validation to ensure users create strong passwords when resetting.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation is complete and meets all task requirements. All critical issues from the previous review have been successfully fixed: the API paths in auth.ts now have proper leading /, the activation page redirects to /profile as required, the /users endpoint has authentication middleware protection, error messages correctly reference 'email', and server-side password validation includes uppercase letter and number checks. The codebase correctly implements registration with password validation, email activation flow, login/logout, password reset with confirmation, and all profile management features.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| try { | ||
| await action(req, res, next); | ||
| } catch (error) { | ||
| next(error); | ||
| } |
There was a problem hiding this comment.
The getUsers function retrieves all users without any authorization check in the controller. The previous review flagged this as a HIGH security issue at the route level. Verify that authMiddleware is properly applied to this endpoint in user.route.ts.
| services: | ||
| postgres: |
There was a problem hiding this comment.
The token.service.ts file only contains an import statement with no exported functions or logic. This file appears incomplete. Either add token-related service functions here, or remove the file if token management is handled elsewhere (e.g., directly in auth.controller.ts).
| import { Prisma, User } from '../generated/prisma/client.js'; | ||
| import { prisma } from '../lib/db.js'; |
There was a problem hiding this comment.
This file imports prisma but exports nothing. If it's not being used, consider removing it to avoid confusion. The auth.controller.ts directly uses prisma.refreshTokens without this service.
| { | ||
| "compilerOptions": { |
There was a problem hiding this comment.
This file contains only an import statement with no exports or implementation. If this service is not needed, consider removing the file. If it was intended to implement token-related operations, the implementation is missing.
Implement full-stack auth app (backend + frontend)
Added a complete authentication application split into two separate services under
src/:src/backend/— Node.js + Express + TypeScript REST API with Prisma ORMsrc/frontend/— React + Vite SPA + TypeScriptFeatures implemented:
Running with Docker Compose (recommended):
cd src
docker compose up --build
The app uses Prisma to connect to PostgreSQL. See
src/docker-compose.yamlfor the connection string and required environment variables — replace them with your own values before running.Running separately (without Docker):
You can run the backend and frontend independently by navigating to the corresponding directory:
cd src/backend
npm i
npm run dev
cd src/frontend
npm i
npm run dev
Don't forget to create a
.envfile insrc/backend/with the required environment variables (seesrc/docker-compose.yamlfor reference).