OTP done#18
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
There was a problem hiding this comment.
Pull request overview
Implements OTP-based email verification for new user registrations and updates the frontend to use backend-driven OTP flows for both registration verification and password reset.
Changes:
- Frontend: adds registration OTP verification UI and wires forgot-password flow to backend endpoints.
- Backend: stores registration OTP/expiry on users, adds verify/resend OTP routes/controllers, and sends registration OTP emails.
- Repo hygiene: adds log ignores in
.gitignore(but currently also commits log files).
Reviewed changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/app/contexts/AppContext.tsx | Extends context API for registration OTP + password reset, updates register behavior |
| frontend/src/app/components/AuthPage.tsx | Adds “awaiting OTP” verification step for registration |
| frontend/src/app/components/ForgotPassword.tsx | Switches from demo OTP generation to backend OTP verification + reset |
| Backend1/src/services/notificationService.ts | Adds registration OTP email sender |
| Backend1/src/routes/authRoutes.ts | Adds /verify-email and /resend-otp routes |
| Backend1/src/controllers/authController.ts | Generates/stores registration OTP, verifies/resends OTP, blocks login until verified |
| Backend1/prisma/schema.prisma | Adds isVerified, otp, otpExpiry fields to User |
| Backend1/logs/error.log | Log output committed (includes conflict markers) |
| Backend1/logs/combined.log | Log output committed (includes conflict markers) |
| Backend1/.gitignore | Ignores logs going forward |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sendRegistrationOTP: async (email: string, otp: string) => { | ||
| if (!env.SMTP_USER) return logger.warn(`SMTP not configured. Reg OTP for ${email}: ${otp}`); | ||
| try { |
There was a problem hiding this comment.
When SMTP is not configured, this logs the registration OTP in plaintext. That can leak a valid verification factor into logs/backups. Consider only logging in development, redacting the OTP, or failing the request so verification can’t proceed without delivery.
| register: (name: string, email: string, password: string, role: User['role'], phone?: string, address?: string) => Promise<any>; | ||
| verifyRegistrationOtp: (userId: string, otp: string) => Promise<User | null>; |
There was a problem hiding this comment.
register is typed as Promise<any> which defeats type safety and forces downstream casting (see AuthPage). Please introduce a specific return type (e.g., a discriminated union for otp_sent vs failure) and update the context interface accordingly.
| setOtpExpiry(Date.now() + 900000); // 15 mins | ||
| setTimeRemaining(900); | ||
| toast.success(`OTP sent to your email!`); |
There was a problem hiding this comment.
Frontend uses a 15-minute OTP expiry/timer (900000 ms / 900 s), but the backend /auth/forgot-password issues OTPs that expire in 5 minutes. This mismatch will cause the UI to show an OTP as valid after the server has started rejecting it. Align the frontend timer with the backend expiry (or return expiry from the API).
| isVerified Boolean @default(false) | ||
| otp String? | ||
| otpExpiry DateTime? |
There was a problem hiding this comment.
schema.prisma adds new User fields (isVerified, otp, otpExpiry), but there is no corresponding Prisma migration updating the database schema (the existing migration.sql doesn’t reference these columns). Add and commit a new migration so environments can apply the schema change consistently.
| isVerified Boolean @default(false) | |
| otp String? | |
| otpExpiry DateTime? |
| <<<<<<< Updated upstream | ||
| const logout = () => { | ||
| try { localStorage.removeItem('token'); } catch {} | ||
| setCurrentUser(null); | ||
| ======= |
There was a problem hiding this comment.
File contains unresolved git merge-conflict markers (e.g., <<<<<<<, =======, >>>>>>>), which will break TypeScript compilation. Resolve the conflict by keeping both logout and the newly added OTP/password-reset functions and ensuring braces/returns are correct.
| const newUser = await tx.user.create({ | ||
| data: { name, email, passwordHash, role: dbRole, phone, address } | ||
| data: { name, email, passwordHash, role: dbRole, phone, address, otp, otpExpiry, isVerified: false } | ||
| }); |
There was a problem hiding this comment.
Registration OTP is being stored in plaintext in the User record (otp). Since authService.hashOTP() already exists, store a hashed OTP (and compare with bcrypt) to avoid leaking valid OTPs via DB access/logs/backups.
| await prisma.user.update({ | ||
| where: { id: userId }, | ||
| data: { isVerified: true, otp: null, otpExpiry: null } | ||
| }); | ||
|
|
||
| let returnUser = { ...user, isVerified: true }; | ||
|
|
||
| const accessToken = authService.generateAccessToken(returnUser.id, returnUser.role); | ||
| const refreshToken = authService.generateRefreshToken(returnUser.id); | ||
|
|
||
| res.status(200).json({ | ||
| success: true, | ||
| message: 'Email verified successfully', | ||
| data: { user: sanitizeUser(returnUser), accessToken, refreshToken } |
There was a problem hiding this comment.
verifyRegistrationOtp builds returnUser from the pre-update user record. Since sanitizeUser() currently only strips passwordHash, this response can include the OTP/expiry values that were just verified. Use the prisma.user.update(...) return value (or re-fetch the user) and ensure OTP fields are not returned in API responses.
| await prisma.user.update({ | |
| where: { id: userId }, | |
| data: { isVerified: true, otp: null, otpExpiry: null } | |
| }); | |
| let returnUser = { ...user, isVerified: true }; | |
| const accessToken = authService.generateAccessToken(returnUser.id, returnUser.role); | |
| const refreshToken = authService.generateRefreshToken(returnUser.id); | |
| res.status(200).json({ | |
| success: true, | |
| message: 'Email verified successfully', | |
| data: { user: sanitizeUser(returnUser), accessToken, refreshToken } | |
| const updatedUser = await prisma.user.update({ | |
| where: { id: userId }, | |
| data: { isVerified: true, otp: null, otpExpiry: null } | |
| }); | |
| // Explicitly strip OTP-related fields before returning the user object | |
| const { otp: _otp, otpExpiry: _otpExpiry, ...safeUser } = updatedUser as any; | |
| const accessToken = authService.generateAccessToken(safeUser.id, safeUser.role); | |
| const refreshToken = authService.generateRefreshToken(safeUser.id); | |
| res.status(200).json({ | |
| success: true, | |
| message: 'Email verified successfully', | |
| data: { user: sanitizeUser(safeUser), accessToken, refreshToken } |
| } | ||
|
|
||
| if (!user.isVerified) { | ||
| return next(new AppError('Please verify your email to login.', 403)); |
There was a problem hiding this comment.
This change blocks login for any existing users who have isVerified = false. Since the new column defaults to false, a migration will likely mark all existing rows as unverified unless explicitly backfilled, effectively locking out current users. Consider a migration/backfill strategy (e.g., set isVerified=true for existing accounts) or only enforce verification for accounts created after this feature.
| return next(new AppError('Please verify your email to login.', 403)); | |
| // Legacy users created before email verification was introduced may have no OTP data. | |
| // To avoid locking them out after adding the isVerified column, treat such users as verified. | |
| if (!user.otp && !user.otpExpiry) { | |
| await prisma.user.update({ | |
| where: { id: user.id }, | |
| data: { isVerified: true } | |
| }); | |
| user.isVerified = true; | |
| } else { | |
| return next(new AppError('Please verify your email to login.', 403)); | |
| } |
No description provided.