Implemented otp based forgot password #282
Conversation
WalkthroughThe changes in this pull request introduce a comprehensive password recovery feature, including the addition of a new email verification function in the Nodemailer module, a controller for managing password recovery processes, and a new router for handling related routes. The customer model is updated to store a verification code, while frontend components are created to facilitate email verification and OTP input. Routing is adjusted to accommodate these new functionalities, enhancing the overall user experience for password resets. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for submitting your pull request! 🙌 We'll review it as soon as possible. In the meantime, please ensure that your changes align with our CONTRIBUTING.md. If there are any specific instructions or feedback regarding your PR, we'll provide them here. Thanks again for your contribution! 😊 |
|
@Sourabh782 is attempting to deploy a commit to the bunty's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@RamakrushnaBiswal can you check this? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (19)
backend/routes/forgotRouter.js (2)
1-10: LGTM! Consider moving dotenv configuration.The imports and router setup look good. The use of destructuring for importing controller functions is a clean approach.
Consider moving the
dotenvconfiguration to the main application file (e.g.,app.jsorserver.js) if it's not already there. This ensures that environment variables are loaded once at application startup.
11-13: LGTM! Consider consistent naming for resetpassword route.The route definitions are clear and align well with the PR objectives. The separation of routing and controller logic is well-maintained.
For consistency, consider changing the "/resetpassword" route to "/reset-password" to match the kebab-case format of the other routes.
backend/models/customer.model.js (1)
Line range hint
1-35: Enhance overall schema security and validation.While reviewing the new changes, I noticed some areas in the existing schema that could be improved:
Password Security: The current
passwordfield doesn't specify any hashing mechanism. It's crucial to ensure passwords are properly hashed before storage.Email Validation: The current regex for email validation could be improved to catch more edge cases.
Role Validation: The
rolefield could benefit from an enum to restrict possible values.Consider applying these improvements:
const customerSchema = new Schema( { name: { type: String, required: true }, - password: String, + password: { + type: String, + required: true, + select: false // Excludes password from query results by default + }, email: { type: String, required: true, unique: true, validate: { validator: function (v) { - return /^\w+([\.-]?\w+)*@\w+([\.-]?\w+)*(\.\w{2,3})+$/.test(v); + return /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(v); }, message: (props) => `${props.value} is not a valid email address!`, }, }, // ... (other fields) role: { type: String, + enum: ['customer', 'admin'], // Add other roles as needed default: "customer", }, // ... (rest of the schema) }, { timestamps: true }, ); +// Add a pre-save hook to hash the password +customerSchema.pre('save', async function(next) { + if (this.isModified('password')) { + this.password = await bcrypt.hash(this.password, 10); + } + next(); +});Don't forget to import bcrypt at the top of the file:
const bcrypt = require('bcrypt');These changes will significantly enhance the security and robustness of your customer model.
backend/routes/index.js (2)
Line range hint
32-40: Approved changes with a minor suggestion.The modifications to the root route response are good. Using
return res.json()ensures that the function execution stops after sending the response, which is a best practice. The addition of the Feedback endpoint to the documentation improves API discoverability.Consider adding the newly implemented "forgot" endpoint to the documentation as well:
endpoints: { Reservation: "/reservation", Feedback: "/feedback", + ForgotPassword: "/forgot", },
Line range hint
1-51: Summary of changes and final verification suggestion.The changes in this file successfully introduce the forgot password functionality and improve the API documentation. The implementation aligns well with the PR objectives.
To ensure the complete implementation of the forgot password feature, please verify the following:
- The
forgotRouter.jsfile is properly implemented with necessary routes for initiating password reset, verifying OTP, and setting a new password.- The customer model has been updated to store the verification code (as mentioned in the AI-generated summary).
- The Nodemailer module is correctly set up for sending OTP emails.
- Frontend components for email verification and OTP input are implemented and connected to these backend routes.
Consider running integration tests to verify the end-to-end flow of the forgot password feature.
frontend/src/router/index.jsx (3)
37-37: LGTM: Updated ResetPassword route with dynamic ID.The modification to include a dynamic
:idin the ResetPassword route enhances security and flexibility in the password reset process. This change aligns well with the PR objectives.Consider adding a comment explaining the purpose of the
:idparameter for better code documentation:{/* Route for resetting password, :id represents a unique reset token */} <Route path="/reset-password/:id" element={<ResetPassword />} />
39-40: LGTM: New routes for OTP verification and email verification.The addition of routes for
VerifyOtpandEmailVerifycomponents is well-implemented and aligns perfectly with the PR objectives. The use of a dynamic:idfor the OTP verification route is a good practice for handling unique verification processes.For consistency with the
ResetPasswordroute, consider adding comments to explain the purpose of these new routes:{/* Route for OTP verification, :id represents a unique verification token */} <Route path="/verifyotp/:id" element={<VerifyOtp />} /> {/* Route for email verification in the password recovery process */} <Route path="/email-verify" element={<EmailVerify />} />
Line range hint
1-45: Overall assessment: Well-implemented routing changes for OTP-based password recovery.The changes to this file effectively implement the necessary routing structure for the OTP-based forgot password feature. The new imports, route modifications, and additions are well-organized and consistent with the existing code style. These changes align perfectly with the PR objectives and enhance the application's authentication flow.
To further improve the routing structure:
- Consider grouping related routes (e.g., all authentication-related routes) using nested routes for better organization.
- Implement lazy loading for these new components to optimize initial load time, especially as the application grows.
Example of lazy loading:
import React, { lazy } from 'react'; const VerifyOtp = lazy(() => import('../components/Pages/VerifyOtp')); const EmailVerify = lazy(() => import('../components/Pages/EmailVerify')); // Wrap the routes with React.Suspense in the router configurationfrontend/src/components/Pages/VerifyOtp.tsx (4)
1-18: LGTM! Consider adding type annotations for better type safety.The imports and initial setup look good. The use of environment variables for the API URL is a good practice. However, to improve type safety and code maintainability, consider adding TypeScript type annotations to your state variables and function parameters.
Here's an example of how you could add type annotations:
const [otp, setOtp] = useState<string>(""); const [isLoading, setIsLoading] = useState<boolean>(false); const [error, setError] = useState<string | null>(null); const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => { setOtp(e.target.value); };
20-50: Good error handling. Consider adding OTP validation before submission.The
handleSubmitfunction is well-structured with proper error handling and state management. However, it's recommended to add client-side validation for the OTP before making the API call. This can improve user experience and reduce unnecessary server requests.Consider adding a validation check before the API call:
const handleSubmit = async (e: React.FormEvent<HTMLFormElement>) => { e.preventDefault(); if (!otp || otp.length !== 6) { // Assuming OTP is 6 digits setError("Please enter a valid 6-digit OTP"); return; } setIsLoading(true); setError(null); try { // ... rest of the function } catch (err) { // ... error handling } };
52-91: Good responsive design. Enhance accessibility with proper labeling.The UI structure is clean and responsive. The use of Tailwind CSS classes for styling is effective. However, the accessibility of the form can be improved by adding proper labels and ARIA attributes.
Consider these accessibility enhancements:
- Add a label for the OTP input:
<label htmlFor="otp" className="sr-only">Enter OTP</label> <input id="otp" aria-label="Enter OTP" aria-describedby="otp-error" // ... other props />
- Improve error message accessibility:
{error && ( <div id="otp-error" role="alert" className="w-full p-2 bg-red-100 text-red-700 border border-red-400 rounded-md"> {error} </div> )}
- Add aria-live region for dynamic content:
<div aria-live="polite" className="sr-only"> {isLoading ? 'Verifying OTP...' : ''} </div>
1-94: Well-implemented OTP verification component. Consider adding rate limiting.The VerifyOtp component is well-structured and implements the required OTP verification functionality effectively. It aligns with the PR objectives and uses modern React practices. The UI is responsive and the error handling is good.
To further enhance security, consider implementing rate limiting for OTP verification attempts. This can be done on the backend, but you can also add a client-side check to prevent excessive attempts.
Example of client-side rate limiting:
const MAX_ATTEMPTS = 5; const LOCKOUT_TIME = 15 * 60 * 1000; // 15 minutes in milliseconds const [attempts, setAttempts] = useState(0); const [lockoutUntil, setLockoutUntil] = useState(0); const handleSubmit = async (e: React.FormEvent<HTMLFormElement>) => { e.preventDefault(); const now = Date.now(); if (now < lockoutUntil) { setError(`Too many attempts. Please try again in ${Math.ceil((lockoutUntil - now) / 60000)} minutes.`); return; } if (attempts >= MAX_ATTEMPTS) { setLockoutUntil(now + LOCKOUT_TIME); setAttempts(0); setError(`Too many attempts. Please try again in 15 minutes.`); return; } // Existing submit logic here // ... setAttempts(prev => prev + 1); };This approach should be combined with server-side rate limiting for robust protection against brute-force attacks.
backend/config/nodemailer.js (1)
105-145: Approve implementation with minor suggestions for improvementThe
sendVerificationMailfunction is well-implemented and consistent with the existing code structure. It correctly handles email sending, error logging, and follows the established patterns in the file. Here are a few suggestions for improvement:
There's a minor typo in the email content. On line 111, "RCode" should be changed to "Code" for clarity.
Consider adding HTML formatting to the email for better presentation. This can improve readability for users.
The verification code in the email body could be made more visually distinct to help users easily identify it.
Here's a suggested improvement for the email content with HTML formatting and a more distinct verification code:
const emailHTML = ` <html> <body> <h2>Dear Customer,</h2> <p>Please use this verification code for resetting your password:</p> <h1 style="color: #4CAF50; background-color: #f1f1f1; padding: 10px; display: inline-block;">Code: ${verificationCode}</h1> <p>Thank you for choosing our service. We are happy to help you.</p> <p>Best regards,<br>PlayCafe</p> </body> </html> `; // In the transporter.sendMail() call: html: emailHTML,This change would require adding the
htmlproperty to thesendMailoptions and removing thetextproperty.frontend/src/components/Pages/EmailVerify.jsx (2)
1-9: LGTM! Consider adding a fallback for API_URL.The imports and component declaration look good. Using an environment variable for the API URL is a best practice. However, consider adding a fallback URL or error handling if the environment variable is not set.
You could modify line 7 to include a fallback URL:
const API_URL = import.meta.env.VITE_BACKEND_URL || 'http://localhost:3000';
62-107: Enhance accessibility and user experience.The component render function looks good overall. Here are some suggestions to improve accessibility and user experience:
- Add a label for the email input field.
- Consider adding aria-live attribute to the error message div for screen readers.
- Add a title attribute to the "Go Back To Login Page" link for better context.
Apply these changes to enhance accessibility:
+ <label htmlFor="email" className="sr-only">Email address</label> <input + id="email" className="input w-full h-10 rounded-md border-2 border-black bg-beige shadow-[4px_4px_0px_0px_black] text-[15px] font-semibold text-[#323232] p-2.5 focus:outline-none focus:border-[#2d8cf0] placeholder-[#666] placeholder-opacity-80" name="email" placeholder="Email" type="email" aria-required="true" autoComplete="email" onChange={(e) => handleChange(e)} /> {error && ( - <div className="w-full p-2 bg-red-100 text-red-700 border border-red-400 rounded-md"> + <div className="w-full p-2 bg-red-100 text-red-700 border border-red-400 rounded-md" aria-live="polite"> {error} </div> )} <span className="block text-[#666] font-semibold text-xl transform hover:scale-110 hover:-translate-y-1 hover:text-green-500 transition"> - <Link to={'/login'}>Go Back To Login Page</Link> + <Link to={'/login'} title="Return to the login page">Go Back To Login Page</Link> </span>frontend/src/components/Pages/Login.jsx (1)
97-99: LGTM: Added "Forgot Password?" linkThe addition of the "Forgot Password?" link is a crucial improvement that directly addresses the PR objective. It provides users with a clear path to recover their passwords.
Consider updating the link text to be more explicit, e.g., "Forgot Password? Reset here" to provide clearer guidance to users.
backend/controller/forgotPassword.controller.js (2)
33-37: Unnecessary Try-Catch Block Around ResponseWrapping
res.status(201).json(...)in a try-catch block is unnecessary because sending a response typically does not throw an exception. The potential errors are more likely to occur in the asynchronous operations before this line.Consider removing the try-catch block or adjusting its scope to include asynchronous operations that may throw errors.
-try { res.status(201).json({ id: existingCustomer._id, success: true }); -} catch (error) { - res.status(500).json({ error: "Internal server error" }); -}
54-55: Reset Verification Code Properly After Successful OTP VerificationSetting
verificationCodeto an empty string may not be the best practice. It's better to remove the field or set it tonullto clearly indicate that there's no active code.Modify the code to reset the verification code more explicitly:
-existingCustomer.verificationCode = "" +existingCustomer.verificationCode = nullfrontend/src/components/Pages/ResetPassword.jsx (1)
Line range hint
17-19: Remove unusedhandleChangefunctionThe
handleChangefunction references adataobject that is not defined or used elsewhere in the component. Since you're managing state through individual state variables (passwordandconfirmPassword), this function is no longer needed and can be safely removed to clean up the code.Apply this diff to remove the unused function:
- const handleChange = (e) => { - setData({ ...data, [e.target.name]: e.target.value }); - };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- backend/config/nodemailer.js (1 hunks)
- backend/controller/forgotPassword.controller.js (1 hunks)
- backend/models/customer.model.js (1 hunks)
- backend/routes/forgotRouter.js (1 hunks)
- backend/routes/index.js (2 hunks)
- frontend/src/components/Pages/EmailVerify.jsx (1 hunks)
- frontend/src/components/Pages/Login.jsx (2 hunks)
- frontend/src/components/Pages/ResetPassword.jsx (5 hunks)
- frontend/src/components/Pages/VerifyOtp.tsx (1 hunks)
- frontend/src/router/index.jsx (2 hunks)
🧰 Additional context used
🔇 Additional comments (9)
backend/routes/forgotRouter.js (1)
15-15: LGTM!The router export is correctly implemented, allowing it to be imported and used in other parts of the application.
backend/models/customer.model.js (1)
Line range hint
1-35: Summary of suggested improvementsOverall, the addition of the
verificationCodefield is a good step towards implementing the OTP-based forgot password functionality. However, there are several areas where both the new implementation and the existing schema can be improved:
- Enhance the
verificationCodefield with expiration, hashing, and validation.- Improve password security by implementing hashing.
- Refine email validation for better accuracy.
- Implement role validation using an enum.
These changes will significantly improve the security, performance, and robustness of your customer model. Please review the suggested implementations and let me know if you need any clarification or assistance in implementing these changes.
To ensure these changes don't conflict with existing functionality, please run the following verification script:
This script will help identify any potential conflicts or security issues related to the suggested changes.
✅ Verification successful
Verification Successful: All checks passed without detecting any conflicts or security issues related to the suggested improvements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of customer model fields in the codebase # Test: Check for direct password comparisons (which should be avoided) echo "Checking for direct password comparisons:" rg --type js 'customer\.password\s*===?' || echo "No direct password comparisons found (good)" # Test: Check for proper use of the 'role' field echo "Checking 'role' field usage:" rg --type js 'customer\.role' # Test: Check for any hardcoded email validation that might conflict with the new regex echo "Checking for hardcoded email validations:" rg --type js 'email.*validate' # Test: Check for any use of 'verificationCode' field to ensure it's used securely echo "Checking 'verificationCode' usage:" rg --type js 'verificationCode'Length of output: 1090
backend/routes/index.js (1)
49-49: Approved new route with a verification suggestion.The addition of the forgot password route aligns well with the PR objectives. The route is correctly placed and follows the naming convention of other routes.
To ensure the forgotRouter is implemented correctly, please run the following verification script:
This script will help verify the existence and basic structure of the forgotRouter, as well as check for essential functionalities like OTP generation and email sending.
frontend/src/router/index.jsx (1)
21-22: LGTM: New component imports for OTP verification.The imports for
VerifyOtpandEmailVerifycomponents are correctly added and align with the PR objectives of implementing an OTP-based forgot password feature.frontend/src/components/Pages/VerifyOtp.tsx (1)
94-94: LGTM! Default export is appropriate for this component.The default export of the VerifyOtp component follows React best practices and conventions.
frontend/src/components/Pages/EmailVerify.jsx (1)
111-111: LGTM! Component export is correct.The default export of the EmailVerify component is implemented correctly.
frontend/src/components/Pages/Login.jsx (3)
64-64: LGTM: Minor UI adjustmentThe reduction in gap size from 'gap-5' to 'gap-4' is a subtle improvement to the form's layout. This change helps in creating a more compact design without affecting the overall functionality.
100-100: LGTM: Improved formattingThe addition of a blank line improves the readability of the code by clearly separating the new "Forgot Password?" section from the existing content.
Line range hint
64-100: Overall assessment: Changes align well with PR objectivesThe modifications to the Login component successfully introduce the "Forgot Password?" functionality, which is a key objective of this PR. The UI adjustments, including the reduced gap and the addition of the password recovery link, contribute to an improved user experience without introducing any apparent issues.
To further enhance the implementation:
- Consider making the "Forgot Password?" link text more explicit to guide users.
- Ensure that the '/email-verify' route is properly set up to handle the password recovery process.
- Test the new functionality thoroughly to confirm it integrates well with the existing login flow.
To verify the integration of the new route, please run the following script:
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
RamakrushnaBiswal
merged commit b6e8228
into
RamakrushnaBiswal:main
RamakrushnaBiswal
added
enhancement
Closes #263
Video walkthrough:
BoardGame.Cafe.-.Google.Chrome.2024-10-14.16-19-25.mp4
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
Documentation