Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions .github/workflows/test.yml-template
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
name: Test

on:
pull_request:
branches: [ master ]

jobs:
build:

runs-on: ubuntu-latest

strategy:
matrix:
node-version: [20.x]

steps:
- uses: actions/checkout@v2
- name: Use Node.js ${{ matrix.node-version }}
uses: actions/setup-node@v1
with:
node-version: ${{ matrix.node-version }}
- run: npm install
- run: npm test
14 changes: 14 additions & 0 deletions prisma.config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// 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";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The import path is incorrect — defineConfig should be imported from the prisma package, not prisma/config. This will cause a module resolution/runtime error. Change to: import { defineConfig } from "prisma" or the correct package export for your Prisma version.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The import path is incorrect and will cause a runtime/module resolution error. Replace this with the official Prisma import: import { defineConfig } from "prisma";. Also ensure the prisma package (Prisma CLI/runtime) is installed in the project so this import resolves.


export default defineConfig({
schema: "prisma/schema.prisma",
migrations: {
path: "prisma/migrations",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You set migrations.path to prisma/migrations which is correct. Make sure the prisma/migrations directory is tracked in version control (remove it from .gitignore) so migrations are preserved for other developers and deployments, per the project requirements.

},
datasource: {
url: process.env["DATABASE_URL"],

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider validating process.env['DATABASE_URL'] here and throwing a clear error if it's missing (e.g. if (!process.env.DATABASE_URL) throw new Error('DATABASE_URL is not set');). This prevents obscure errors from Prisma later and makes failures easier to debug.

},
});
38 changes: 38 additions & 0 deletions prisma/migrations/20260206215804_init/migration.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
-- CreateTable
CREATE TABLE "users_auth" (
"id" SERIAL NOT NULL,
"name" VARCHAR(255) NOT NULL,
"email" VARCHAR(255) NOT NULL,
"password" VARCHAR(255) NOT NULL,
"activationToken" VARCHAR(255),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

activationToken exists which can be used for activation, but there is no explicit activation flag/timestamp (for example isActive BOOLEAN DEFAULT FALSE or activatedAt TIMESTAMPTZ). The requirements state the user should be activated only after email confirmation — adding an isActive or activatedAt column makes the activation state explicit and simpler to query.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

activationToken exists which can be used for activation, but there is no explicit activation flag/timestamp (for example isActive BOOLEAN DEFAULT FALSE or activatedAt TIMESTAMPTZ). The requirements state the user should be activated only after email confirmation — adding an isActive or activatedAt column makes the activation state explicit and simpler to query.

"createdAt" TIMESTAMPTZ(6) NOT NULL DEFAULT CURRENT_TIMESTAMP,
"updatedAt" TIMESTAMPTZ(6) NOT NULL,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updatedAt is declared NOT NULL but has no default value. If the application does not explicitly set updatedAt on INSERT you will get errors. Consider adding a default such as DEFAULT CURRENT_TIMESTAMP or a trigger to set/refresh updatedAt on updates, or allow NULL if your app will set it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updatedAt is declared NOT NULL but has no default value. If the application does not explicitly set updatedAt on INSERT you will get errors. Consider adding a default such as DEFAULT CURRENT_TIMESTAMP or a trigger to set/refresh updatedAt on updates, or allow NULL if your app will set it.


CONSTRAINT "users_auth_pkey" PRIMARY KEY ("id")
);

-- CreateTable
CREATE TABLE "tokens_auth" (
"id" SERIAL NOT NULL,
"refreshToken" VARCHAR(255) NOT NULL,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tokens_auth contains only refreshToken and no column to distinguish token purposes. The app needs password-reset tokens and also an email-change confirmation token (or another mechanism). Either add a type column (e.g. 'refresh' | 'resetPassword' | 'emailChange') and a token column, or create a separate table for one-time action tokens. Without this, implementing password reset and email-change confirmation per the task will be awkward or error-prone.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tokens_auth contains only refreshToken and no column to distinguish token purposes. The app needs password-reset tokens and also an email-change confirmation token (or another mechanism). Either add a type column (e.g. 'refresh' | 'resetPassword' | 'emailChange') and a token column, or create a separate table for one-time action tokens. Without this, implementing password reset and email-change confirmation per the task will be awkward or error-prone.

"createdAt" TIMESTAMPTZ(6) NOT NULL DEFAULT CURRENT_TIMESTAMP,
"updatedAt" TIMESTAMPTZ(6) NOT NULL,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tokens_auth.updatedAt is also NOT NULL with no default (same issue as the users table). Ensure updatedAt has an appropriate default or that the application always supplies it on INSERT, otherwise inserts will fail.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tokens_auth.updatedAt is also NOT NULL with no default (same issue as the users table). Ensure updatedAt has an appropriate default or that the application always supplies it on INSERT, otherwise inserts will fail.

"userId" INTEGER NOT NULL,

CONSTRAINT "tokens_auth_pkey" PRIMARY KEY ("id")
);

-- CreateIndex
CREATE UNIQUE INDEX "users_auth_email_key" ON "users_auth"("email");

-- CreateIndex
CREATE UNIQUE INDEX "users_auth_activationToken_key" ON "users_auth"("activationToken");

-- CreateIndex
CREATE UNIQUE INDEX "tokens_auth_refreshToken_key" ON "tokens_auth"("refreshToken");

-- CreateIndex
CREATE UNIQUE INDEX "tokens_auth_userId_key" ON "tokens_auth"("userId");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a UNIQUE index on tokens_auth.userId, which enforces at most one token row per user. That prevents multiple refresh tokens (multi-device sessions). If you want to support multiple active refresh tokens, remove the unique constraint on userId and use a non-unique index instead. If one-token-per-user is intentional, keep it but be aware of the limitation.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a UNIQUE index on tokens_auth.userId, which enforces at most one token row per user. That prevents multiple refresh tokens (multi-device sessions). If you want to support multiple active refresh tokens, remove the unique constraint on userId and use a non-unique index instead. If one-token-per-user is intentional, keep it but be aware of the limitation.


-- AddForeignKey
ALTER TABLE "tokens_auth" ADD CONSTRAINT "tokens_auth_userId_fkey" FOREIGN KEY ("userId") REFERENCES "users_auth"("id") ON DELETE RESTRICT ON UPDATE CASCADE;
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/*
Warnings:

- A unique constraint covering the columns `[resetToken]` on the table `users_auth` will be added. If there are existing duplicate values, this will fail.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The warning here is a bit misleading: because you're adding a new nullable column (no default), there cannot be pre-existing duplicate values in it. Consider clarifying or removing this line so it doesn't confuse reviewers or operators running the migration.

Comment on lines +3 to +4

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good that the migration includes a warning about duplicates. Add an explicit pre-migration check (and instructions or SQL) so the migration fails fast with a clear message — e.g. run SELECT resetToken, count(*) FROM users_auth WHERE resetToken IS NOT NULL GROUP BY resetToken HAVING count(*) > 1; and clean any duplicates before applying this migration.


*/
-- AlterTable
ALTER TABLE "users_auth" ADD COLUMN "resetToken" VARCHAR(255);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a timestamp column such as resetTokenExpiresAt (or similar) to store token expiry. Without an expiry you can't enforce short-lived reset tokens in the DB layer which is important for security and for cleaning up old tokens.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be explicit about the column nullability and intent. For reset tokens you likely want a nullable column; consider ADD COLUMN "resetToken" VARCHAR(255) NULL; so the intent is clear to readers and future maintainers.


-- CreateIndex
CREATE UNIQUE INDEX "users_auth_resetToken_key" ON "users_auth"("resetToken");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer creating a partial unique index that enforces uniqueness only for non-null tokens, e.g. CREATE UNIQUE INDEX ... ON "users_auth"("resetToken") WHERE "resetToken" IS NOT NULL; — this makes the intent explicit and avoids ambiguity across DB engines/environments.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a UNIQUE index may lock the table while it builds. For large tables consider CREATE UNIQUE INDEX CONCURRENTLY ... to avoid long locks (note: CONCURRENTLY cannot run inside a transaction). Also consider making the index partial to be explicit: CREATE UNIQUE INDEX "users_auth_resetToken_key" ON "users_auth" ("resetToken") WHERE "resetToken" IS NOT NULL; Ensure your migration runner (Prisma) supports the chosen approach.

11 changes: 11 additions & 0 deletions prisma/migrations/20260210160621_add_pending_email/migration.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/*
Warnings:

- A unique constraint covering the columns `[pendingEmail]` on the table `users_auth` will be added. If there are existing duplicate values, this will fail.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The top-of-file warning correctly notes potential failure due to duplicates; you should codify a migration-safe approach: either (a) add a pre-migration step that removes or resolves duplicate pendingEmail values, or (b) create the unique index as a partial index and/or use CONCURRENTLY to avoid locks and make the migration safer.

Comment on lines +3 to +4

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The top comment warns that adding a unique constraint could fail if there are existing duplicate pendingEmail values. Since this migration adds the pendingEmail column (it didn't exist before), there can't be pre-existing values for it — the warning is misleading. Consider removing or clarifying this comment so it doesn't confuse future maintainers.


*/
-- AlterTable
ALTER TABLE "users_auth" ADD COLUMN "pendingEmail" VARCHAR(255);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ALTER TABLE adds the pendingEmail column which is necessary to support the email-change verification flow. Consider making the intent explicit by marking the column nullable (e.g. VARCHAR(255) NULL) in the SQL so it's clear to readers and schema tools that existing rows are unaffected.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ALTER TABLE adds pendingEmail as VARCHAR(255) (nullable). That's appropriate for storing a pending/unconfirmed email. Make sure your application normalizes emails (e.g., lowercases) before inserting into this column, or add DB-level normalization to avoid case-related duplicates.


-- CreateIndex
CREATE UNIQUE INDEX "users_auth_pendingEmail_key" ON "users_auth"("pendingEmail");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a plain UNIQUE INDEX can fail if there are duplicate non-null pendingEmail values in existing data (the file warning mentions this). To reduce risk consider creating a partial unique index (applying uniqueness only for non-null values) or ensure you run a pre-migration cleanup. Example: CREATE UNIQUE INDEX CONCURRENTLY name ON users_auth(pendingEmail) WHERE pendingEmail IS NOT NULL (also reduces locking).

Comment on lines +10 to +11

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The UNIQUE INDEX on pendingEmail is case-sensitive in PostgreSQL. To avoid duplicates that differ only by case, either:

  • normalize emails to lowercase in the application, or
  • create a case-insensitive index (e.g. on LOWER("pendingEmail")) or use the citext extension. Also, for very large tables think about index creation strategy (e.g. CREATE INDEX CONCURRENTLY) — but be careful: some migration runners wrap statements in transactions and CONCURRENTLY cannot run inside a transaction.

Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
Warnings:

- You are about to drop the column `resetToken` on the `users_auth` table. All the data in the column will be lost.
- A unique constraint covering the columns `[passwordResetToken]` on the table `users_auth` will be added. If there are existing duplicate values, this will fail.
- A unique constraint covering the columns `[pendingEmailToken]` on the table `users_auth` will be added. If there are existing duplicate values, this will fail.

*/
-- DropIndex
DROP INDEX "users_auth_resetToken_key";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping the index users_auth_resetToken_key will fail if that index/column wasn't created by earlier migrations. Use DROP INDEX IF EXISTS or verify earlier migrations create resetToken before trying to drop it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping the index users_auth_resetToken_key will fail if that index/column wasn't created by earlier migrations. Use DROP INDEX IF EXISTS or verify earlier migrations create resetToken before trying to drop it.


-- AlterTable
ALTER TABLE "users_auth" DROP COLUMN "resetToken",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ALTER tries to DROP COLUMN "resetToken". If that column does not exist in the current schema this will fail. Either use DROP COLUMN IF EXISTS or ensure previous migration includes that column. Also note the in-file warning about data loss — migrate or back up values if needed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ALTER tries to DROP COLUMN "resetToken". If that column does not exist in the current schema this will fail. Either use DROP COLUMN IF EXISTS or ensure previous migration includes that column. Also note the in-file warning about data loss — migrate or back up values if needed.

ADD COLUMN "passwordResetToken" VARCHAR(255),
ADD COLUMN "pendingEmailToken" VARCHAR(255);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You add pendingEmailToken but do not add a pendingEmail column to store the new email address being confirmed. The task requires the user to "confirm the new email" and notifying the old email; without storing the pending email you cannot complete that flow. Add a pendingEmail VARCHAR(255) (and consider a pendingEmailRequestedAt) to support confirmation and notifications.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You add pendingEmailToken but do not add a pendingEmail column to store the new email address being confirmed. The task requires the user to "confirm the new email" and notifying the old email; without storing the pending email you cannot complete that flow. Add a pendingEmail VARCHAR(255) (and consider a pendingEmailRequestedAt) to support confirmation and notifications.


-- CreateIndex
CREATE UNIQUE INDEX "users_auth_passwordResetToken_key" ON "users_auth"("passwordResetToken");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a UNIQUE index on passwordResetToken can fail if duplicate non-null values already exist (the file warning notes this). Ensure you clean duplicates before applying or consider a non-unique index or a separate tokens table to avoid migration failures.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a UNIQUE index on passwordResetToken can fail if duplicate non-null values already exist (the file warning notes this). Ensure you clean duplicates before applying or consider a non-unique index or a separate tokens table to avoid migration failures.


-- CreateIndex
CREATE UNIQUE INDEX "users_auth_pendingEmailToken_key" ON "users_auth"("pendingEmailToken");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, a UNIQUE index on pendingEmailToken restricts tokens to be unique and will fail on duplicates. Also consider adding token metadata (createdAt/expiry) or using a dedicated tokens table so you can manage token expiry, revocation and multiple concurrent tokens per user.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, a UNIQUE index on pendingEmailToken restricts tokens to be unique and will fail on duplicates. Also consider adding token metadata (createdAt/expiry) or using a dedicated tokens table so you can manage token expiry, revocation and multiple concurrent tokens per user.

3 changes: 3 additions & 0 deletions prisma/migrations/migration_lock.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Please do not edit this file manually
# It should be added in your version-control system (e.g., Git)
provider = "postgresql"
34 changes: 34 additions & 0 deletions prisma/schema.prisma
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
generator client {
provider = "prisma-client-js"
}

datasource db {
provider = "postgresql"
}

model User {
id Int @id @default(autoincrement())
name String @db.VarChar(255)
email String @unique @db.VarChar(255)
password String @db.VarChar(255)
activationToken String? @unique @db.VarChar(255)
createdAt DateTime @default(now()) @db.Timestamptz(6)
updatedAt DateTime @updatedAt @db.Timestamptz(6)
passwordResetToken String? @unique @db.VarChar(255)
pendingEmail String? @unique @db.VarChar(255)
pendingEmailToken String? @unique @db.VarChar(255)
tokens Token[]

@@map("users_auth")
}

model Token {
id Int @id @default(autoincrement())
refreshToken String @unique @db.VarChar(255)
createdAt DateTime @default(now()) @db.Timestamptz(6)
updatedAt DateTime @updatedAt @db.Timestamptz(6)
userId Int @unique
users User @relation(fields: [userId], references: [id])

@@map("tokens_auth")
}
100 changes: 100 additions & 0 deletions src/controllers/auth.controller.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
import { userService } from '../services/user.service.js';
import { authService } from '../services/auth.service.js';

const register = async (req, res) => {
const { name, email, password } = req.body;

await authService.register(name, email, password);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The register handler simply forwards name, email, password to authService without exposing password rules or validating them here. The task requires informing users about password rules and checking them. Either validate and return human-friendly rule errors here, or ensure authService.register enforces the rules and returns clear messages that you surface to the client.


res.send({ message: 'Registration successful' });
Comment on lines +4 to +9

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Registration handler doesn't communicate password rules or show them to the client. The task requires that users be informed of password rules and that they are enforced. Either (a) include a description of the password rules in the registration response, or (b) validate the password here and return a clear 4xx with the rule violations. If you rely on authService.register to enforce this, make sure it returns descriptive errors and document that here.

};

const activate = async (req, res) => {
const { activationToken } = req.params;

const activatedUser = await authService.activate(activationToken);

res.send(userService.normalize(activatedUser));
Comment on lines +15 to +17

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After activation you call authService.activate and return the normalized user object. The requirements state: “Redirect to Profile after the activation.” Returning the user may be fine for an API, but the spec expects a redirect or a clear flow that leads the client to the Profile. Please ensure the activation endpoint triggers the expected redirect flow (or document why redirect is handled on the client and provide a clear response/token to allow the client to navigate to profile).

Comment on lines +12 to +17

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Activation handler currently returns the activated user JSON. The specification requires redirecting to the Profile page after activation. Decide whether the server should perform the redirect (e.g. res.redirect('/profile')) or explicitly tell the frontend to redirect; implement the chosen behavior. Also ensure this route is only accessible to non-authenticated users (middleware or an explicit check).

};

const sendAuthentication = async (res, user) => {
const {
user: normalizedUser,
accessToken,
refreshToken,
} = await authService.authenticate(user);

res.cookie('refreshToken', refreshToken, {
maxAge: 30 * 24 * 60 * 60 * 1000,
httpOnly: true,
});
res.send({ user: normalizedUser, accessToken });
};

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

const user = await authService.login(email, password);

await sendAuthentication(res, user);
};

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

const user = await authService.refresh(refreshToken);

await sendAuthentication(res, user);
};

const logout = async (req, res) => {
const { refreshToken } = req.cookies;

await authService.logout(refreshToken);

res.clearCookie('refreshToken');
res.send({ message: 'Logout successful' });
Comment on lines +50 to +56

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logout and some other endpoints have access-control requirements (e.g. only authenticated users can logout; some flows only for non-authenticated). This controller does not enforce those checks itself — make sure route-level middleware or explicit checks are in place. If you prefer controller-level enforcement, add guards here (e.g. check req.user).

};

const resetPasswordNotification = async (req, res) => {
const { email } = req.body;

await authService.resetPasswordNotification(email);

res.clearCookie('refreshToken');
res.send({ message: 'Password reset email sent' });
};

const confirmPasswordReset = async (req, res) => {
const { passwordResetToken } = req.params;
const { password, confirmation } = req.body;

await authService.confirmPasswordReset(
passwordResetToken,
password,
confirmation,
);
Comment on lines +72 to +76

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

confirmPasswordReset accepts password and confirmation and forwards them to the service. The task requires these fields must be equal. Make sure authService.confirmPasswordReset validates equality and returns a helpful error if they differ; if you prefer to validate in the controller, add the check before calling the service.

Comment on lines +68 to +76

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Password reset confirmation does not validate that password and confirmation are equal before calling the service. The spec mandates these fields must match. Add a check and return a 400 (or suitable error) when they differ to avoid unnecessary service calls and to satisfy the requirement.


res.clearCookie('refreshToken');
res.send({ message: 'Password reset successful' });
};

const confirmEmailChange = async (req, res) => {
const { pendingEmailToken } = req.params;

await authService.confirmEmailChange(pendingEmailToken);
Comment on lines +82 to +85

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a confirmEmailChange endpoint that accepts only a confirmation token, but I don't see an endpoint here to initiate the email change (where the user supplies their current password and the new email). The requirements specify: To change an email the user should type the password, confirm the new email and notify the old email about the change. Add a controller action (e.g. requestEmailChange) that accepts password and newEmail, verifies the password, creates a pending token sent to the newEmail, and sends a notification to the old email.


res.clearCookie('refreshToken');
res.send({ message: 'Email updated successfully' });
Comment on lines +82 to +88

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Email-change confirmation endpoint exists (good), but the task requires that the new email is confirmed before updating and that the OLD email is notified about the change. Ensure authService.confirmEmailChange performs the final update only after confirmation and sends a notification to the previous email. If that logic is in the service, add an assertion/comment here or move/duplicate checks as needed.

};

export const authController = {
register,
activate,
login,
refresh,
logout,
resetPasswordNotification,
confirmPasswordReset,
confirmEmailChange,
};
Comment on lines +91 to +100

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The controller does not expose any handler to change the user's name on the Profile page. The previous review requested adding a route/controller/service to allow users to change their name. Please add an authenticated endpoint (e.g. updateName) that accepts the new name and delegates to the service layer to persist it.

55 changes: 55 additions & 0 deletions src/controllers/user.controller.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import { authService } from '../services/auth.service.js';
import { userService } from '../services/user.service.js';

const getProfile = async (req, res) => {
const { refreshToken } = req.cookies;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These handlers require a refreshToken cookie (extracted here). However authMiddleware validates the access token from Authorization header and sets req.userId. If a client has a valid access token but no refresh-token cookie, userService.getProfile / authService calls will throw unauthorized. Consider either relying on the access token (req.userId) only for profile reads/changes or documenting/ensuring the refresh cookie is always sent by the client.

const profile = await userService.getProfile(req.userId, refreshToken);
Comment on lines +5 to +6

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getProfile passes refreshToken from cookies to the service but doesn't handle a missing cookie. Either validate and return a clear error or ensure the service gracefully handles absent refreshToken and document that assumption.


res.send(profile);
Comment on lines +4 to +8

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getProfile assumes req.userId is set. The task requires the profile page to be accessible only to authenticated users — add an explicit guard (return 401 if !req.userId) or ensure route-level authentication middleware is documented and always applied.

};

const updateName = async (req, res) => {
const { name } = req.body;
const { refreshToken } = req.cookies;

await authService.updateName(req.userId, name, refreshToken);

res.send({ message: 'Name changed successfully' });
Comment on lines +11 to +17

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updateName correctly calls the service to change the name (good). However, validate the name input (required, non-empty, max length) before calling authService.updateName to avoid invalid updates. Also ensure this endpoint is restricted to authenticated users (check req.userId).

};

const updatePassword = async (req, res) => {
const { password, newPassword, confirmation } = req.body;
const { refreshToken } = req.cookies;

await authService.updatePassword(
refreshToken,
req.userId,
password,
newPassword,
confirmation,
);

res.clearCookie('refreshToken');
res.send({ message: 'Password changed successfully' });
Comment on lines +20 to +33

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updatePassword accepts password, newPassword, and confirmation but does not check that newPassword and confirmation are equal. The spec mandates these must match — add a check and return a 400 if they differ before calling the service.

Comment on lines +20 to +33

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also for updatePassword: validate that password (old password) is provided and that newPassword meets the required password rules. The spec requires enforcing password rules when changing passwords.

Comment on lines +31 to +33

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updatePassword clears the refreshToken cookie after changing password (good). Ensure the service verifies the old password and returns appropriate errors; if it does not, add that verification here or in the service.

};

const updateEmailNotification = async (req, res) => {
const { newEmail, password } = req.body;
const { refreshToken } = req.cookies;

await authService.updateEmailNotification(
refreshToken,
req.userId,
newEmail,
password,
);

res.send({ message: 'Change email notification sent' });
Comment on lines +36 to +47

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updateEmailNotification starts the email-change flow by sending a notification to the new email (good). The specification requires the new email to be confirmed before updating and that the OLD email be notified about the change — ensure authService.updateEmailNotification only sends a confirmation email (does not swap emails immediately) and that authService.confirmEmailChange performs the final update and notifies the old email. If that logic is in the service, add a short comment here to make the contract explicit.

Comment on lines +36 to +47

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updateEmailNotification does not validate newEmail format or presence of password. Add input validation for newEmail (valid email format) and ensure password is provided for authentication as required by the spec.

};

export const userController = {
getProfile,
updateName,
updatePassword,
updateEmailNotification,
};
Comment on lines 50 to 55

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requirements specify that the profile page should allow a user to change their name. This controller is missing the necessary function to handle that update request.

20 changes: 20 additions & 0 deletions src/index.js
Original file line number Diff line number Diff line change
@@ -1 +1,21 @@
'use strict';
import cors from 'cors';
import express from 'express';
import cookieParser from 'cookie-parser';
import { authRouter } from './routes/auth.route.js';
import { userRouter } from './routes/user.route.js';
import { authMiddleware } from './middlewares/auth.middleware.js';
import { errorMiddleware } from './middlewares/error.middleware.js';

const app = express();
const PORT = process.env.PORT || 3005;

app.use(cors());

app.use(express.json());
app.use(cookieParser());
app.use(authRouter);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure that endpoints which must only be accessible to non-authenticated users (registration, activation, login, password reset flows) are actually protected by middleware — either inside authRouter or by adding route-level guards here. Right now authRouter is mounted without explicit guards in this file; confirm the router implements the required access-control checks.

app.use('/profile', authMiddleware, userRouter);
app.use(errorMiddleware);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires returning a 404 error for all pages not explicitly defined. Currently, the application relies on Express's default behavior for unhandled routes, which might not be a JSON response. It's better to add a catch-all middleware right before this line to handle any requests that didn't match a route and forward a proper ApiError.notFound() to your errorMiddleware for a consistent API error response.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing the required catch-all 404 handler. Per the task and the previous review, you must add a middleware before the error handler that returns a standardized JSON 404 for undefined routes. For example:

app.use((req, res) => {
  res.status(404).json({ status: 404, message: 'Not Found' });
});

Add this before app.use(errorMiddleware); so the error middleware remains last.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file lacks the required global catch‑all 404 handler. The task requires returning a standardized JSON 404 for undefined API routes. Add a middleware after all routers and before the error handler that sends a 404 JSON response (for example: app.use((req, res) => res.status(404).json({ status: 404, message: 'Not Found' }))). Place it before errorMiddleware so not-found routes are handled correctly.


app.listen(PORT);
31 changes: 31 additions & 0 deletions src/middlewares/auth.middleware.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { jwtService } from '../services/jwt.service.js';
import { userService } from '../services/user.service.js';
import { ApiError } from '../utils/api.error.js';
import { logger } from '../utils/logger.js';

export const authMiddleware = async (req, res, next) => {
const authHeader = req.headers.authorization ?? '';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This middleware reads the Authorization header but elsewhere (profile endpoints) the app expects a refreshToken cookie. That creates inconsistent auth behavior between routes — either require the cookie consistently or rely only on the access token validated here. See the profile controller and route wiring for context.


if (!authHeader.startsWith('Bearer ')) {
logger.error('Authorization header missing or malformed');
throw ApiError.unauthorized();
Comment on lines +9 to +11

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throwing ApiError.unauthorized() here inside an async middleware will reject the promise; Express 4 does not handle async promise rejections automatically. Wrap the middleware body in try/catch and call next(error) instead (or return errors via next).

}

const token = authHeader.split(' ')[1];
const userData = jwtService.verify(token);
Comment on lines +14 to +15

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling jwtService.verify directly may throw or may return a Promise. Wrap verification in a try/catch and await it if the service is async. If verification fails, throw ApiError.unauthorized() so the error is handled consistently instead of crashing the process.


if (!userData) {
logger.error('Invalid or expired token');
throw ApiError.unauthorized();
Comment on lines +17 to +19

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same problem as above: throwing inside the async function. Use try/catch and next(error) so the central error middleware receives the error reliably.

}

const user = await userService.getById(userData.id);

if (!user) {
logger.error('User not found for token', { userId: userData.id });
throw ApiError.notFound();
Comment on lines +22 to +26

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the token's user cannot be found you return ApiError.notFound(). For token verification failures it's better to return unauthorized to avoid user-enumeration and to keep behavior consistent for invalid tokens.

Comment on lines +24 to +26

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the user is not found you throw ApiError.notFound(). For tokens that reference deleted users it's safer/clearer to return ApiError.unauthorized() to avoid exposing resource existence differences. Decide and align this behavior with the rest of your API.

}
Comment on lines +21 to +27

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider checking user.isActive here (or ensure another layer does it). Some protected routes should deny access for non-activated users — if you want that globally enforced, add the check and return ApiError.forbidden() or ApiError.unauthorized() as appropriate.


req.userId = user.id;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After loading the user you only set req.userId. Attach the full user object (for example req.user = user) so downstream controllers can access user fields without another DB lookup. This also makes it easy to check user.isActive if needed.

next();
};
9 changes: 9 additions & 0 deletions src/middlewares/catch-error.middleware.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
export const catchError = (action) => {
return async (req, res, next) => {
try {
await action(req, res, next);
} catch (error) {
next(error);
}
};
};
14 changes: 14 additions & 0 deletions src/middlewares/check-login.middleware.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { ApiError } from '../utils/api.error.js';
import { logger } from '../utils/logger.js';

export const checkNotLoggedIn = (req, res, next) => {
const { refreshToken } = req.cookies;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relying only on the presence of the refreshToken cookie is brittle: a cookie may be stale or removed server-side. Consider verifying the token (e.g., check it exists in DB via tokenService.getByToken or validate/verify the token) before treating the request as authenticated. This prevents false positives for logged-in state.


if (refreshToken) {
Comment on lines +5 to +7

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The middleware only checks for a refreshToken cookie. A user might be authenticated with a valid access token in the Authorization header but no cookie — in that case the middleware would treat them as unauthenticated. Consider also checking Authorization header (or verifying the refresh token) to reliably detect logged-in users.

logger.info('Already logged in user attempt', {
userId: req.userId,
Comment on lines +8 to +9

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You log userId: req.userId but req.userId is typically set by authMiddleware (which isn't run for non-authenticated routes). That means the log will usually contain undefined. Either remove/adjust this log or extract/verify the token before logging an id.

Comment on lines +8 to +9

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You log req.userId here but this middleware does not populate req.userId (that is done by authMiddleware after token verification). Either decode/verify the token here and set req.userId before logging, or remove userId from the log to avoid undefined values in logs. See how authMiddleware sets req.userId after verification for reference.

});
throw ApiError.forbidden();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throwing ApiError.forbidden() is acceptable, but include a descriptive message (e.g. ApiError.forbidden('Already logged in')) so clients and logs get clearer information. Also ensure the route-level usage of this middleware matches the spec (these routes should be used only for non-authenticated users — the router shows it's applied to registration/login/reset endpoints).

}
next();
};
Loading
Loading