Feat :Feature password reset(Create Forgot Password Page & Integrate Password Reset Flow)[Issue #58] #89
Conversation
Summary of ChangesHello @Rohanrathod7, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a robust and secure password reset system for the LocalMind application. It encompasses new user interfaces for initiating and completing password resets, corresponding backend API endpoints with strong validation, and critical security measures to protect user accounts. The changes are thoroughly tested and documented to ensure reliability and maintainability. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive password reset feature, complete with backend endpoints, frontend UI, and extensive tests. The implementation demonstrates strong security practices, such as hashing the reset token and preventing email enumeration. The addition of detailed tests for validation and API flows is commendable. My review focuses on further improving security, error handling, and maintainability. Key suggestions include tightening the CORS policy for production, ensuring proper error propagation from the email service, and improving type safety by removing as any casts. Overall, this is a well-executed feature that significantly enhances the application's authentication capabilities.
| logger.token('time', () => new Date().toLocaleString()) | ||
| app.use(logger(':time :method :url :status')) | ||
|
|
||
| app.use(cors({ origin: true, credentials: true })) |
There was a problem hiding this comment.
The current CORS configuration origin: true is too permissive for a production environment as it reflects any origin from the request header. This can pose a security risk. It's highly recommended to use a whitelist of allowed origins, which can be loaded from environment variables.
| app.use(cors({ origin: true, credentials: true })) | |
| app.use(cors({ origin: ['http://localhost:5173', 'https://your-production-domain.com'], credentials: true })) |
| } catch (error) { | ||
| console.error('❌ Error sending email:', error) | ||
| // Fallback to console log if sending fails | ||
| console.log('⚠️ Fallback: Logging email to console due to send failure.') | ||
| console.log(`Reset Link: ${options.message}`) | ||
| // Do not throw error so the flow continues | ||
| } |
There was a problem hiding this comment.
The catch block in sendEmail swallows the error and does not re-throw it. This prevents the calling service (userService.forgotPassword) from handling the email sending failure. The catch block in userService.forgotPassword which is supposed to clean up the reset token is currently unreachable. The error should be re-thrown to ensure failures are handled correctly.
} catch (error) {
console.error('❌ Error sending email:', error)
// Fallback to console log if sending fails
console.log('⚠️ Fallback: Logging email to console due to send failure.')
console.log(`Reset Link: ${options.message}`)
// Re-throw the error so the caller can handle it.
throw error;
}| it('should successfully login with valid credentials', async () => { | ||
| try { | ||
| const res = await axios.post(`${API_URL}/user/login`, { | ||
| email: loginTestEmail, | ||
| password: validPassword, | ||
| }) | ||
|
|
||
| expect(res.status).toBe(200) | ||
| expect(res.data).toBeDefined() | ||
| expect(res.data.message).toBeDefined() | ||
| } catch (error: any) { | ||
| if (error.response?.status === 401 || error.response?.status === 404) { | ||
| console.log('Test user not found, skipping login success test') | ||
| expect(true).toBe(true) | ||
| } else { | ||
| throw error | ||
| } | ||
| } | ||
| }, 10000) |
There was a problem hiding this comment.
This test case for a successful login is not robust because it depends on a user existing from a previous test suite. If the user doesn't exist, it catches the error and considers the test passed, which can hide real issues. It's better to make tests independent by creating the necessary user in a beforeAll or beforeEach hook for this test suite to ensure a consistent and reliable test environment.
|
|
||
| const user = await userService.createUser(validatedData) | ||
|
|
||
| const user = await userService.createUser(validatedData as any) |
There was a problem hiding this comment.
The as any cast here, and also on line 73 in the login method, hides potential type mismatches between the Zod schema's inferred type and the type expected by the service methods. To improve type safety, consider adjusting the IUser interface or the service method signatures to be compatible with the Zod-inferred types, removing the need for these casts.
| } catch (err: any) { | ||
| SendResponse.error(res, err.message || UserConstant.PASSWORD_RESET_FAILED, 500, err) | ||
| } |
There was a problem hiding this comment.
Returning a generic 500 Internal Server Error for client-side issues like an invalid token is not ideal. It's better to catch specific, expected errors and return a more appropriate 4xx status code, like 400 Bad Request. This provides clearer feedback to the client.
} catch (err: any) {
if (err.message === UserConstant.INVALID_TOKEN) {
return SendResponse.error(res, err.message, StatusConstant.BAD_REQUEST, err)
}
SendResponse.error(res, err.message || UserConstant.PASSWORD_RESET_FAILED, 500, err)
}|
|
||
| // Construct reset URL (frontend URL) | ||
| // NOTE: In production, FRONTEND_URL should be in env | ||
| const frontendUrl = process.env.FRONTEND_URL || 'http://localhost:5173' |
There was a problem hiding this comment.
Hardcoding a fallback URL for FRONTEND_URL can lead to incorrect links in different environments (e.g., staging, production). It's better to ensure this environment variable is always set and fail fast if it's missing in production environments.
const frontendUrl = process.env.FRONTEND_URL
if (!frontendUrl) {
throw new Error('FRONTEND_URL environment variable is not set.');
}| bio: z | ||
| .string() | ||
| .min(5, 'Bio must be at least 5 characters') | ||
| .max(50, 'Bio must be less than 50 characters'), |
There was a problem hiding this comment.
The bio field has a min(5) validation, but it's optional on the backend. This creates an inconsistency where the user is forced to enter a bio of at least 5 characters. To align with the backend, you should remove the minimum length requirement, which will still allow empty strings.
bio: z.string().max(50, 'Bio must be less than 50 characters'),
| const signUpSchema = z.object({ | ||
| firstName: z.string().min(1, 'First name is required'), | ||
| role: z.string().optional(), | ||
| email: z.string().email('Please enter a valid email address'), | ||
| birthPlace: z.string().min(1, 'Birth place is required'), | ||
| location: z.string().min(1, 'Location is required'), | ||
| password: z | ||
| .string() | ||
| .min(8, 'Password must be at least 8 characters') | ||
| .max(128, 'Password must be less than 128 characters') | ||
| .regex(/[A-Z]/, 'Must contain uppercase letter') | ||
| .regex(/[a-z]/, 'Must contain lowercase letter') | ||
| .regex(/[0-9]/, 'Must contain a number') | ||
| .regex(/[@$!%*?&]/, 'Must contain special character'), | ||
| portfolioUrl: z.string().url('Please enter a valid URL').or(z.literal('')), | ||
| bio: z.string().max(50, 'Bio must be less than 50 characters'), | ||
| }) |
There was a problem hiding this comment.
The Zod schema is duplicated from SignUp.tsx. It's better to export the schema from SignUp.tsx and import it here to avoid inconsistencies and ensure you're testing the exact schema used in the component. This prevents the test from becoming out of sync with the implementation.
|
@Rohanrathod7 Please improve this Ui |
abhishek-nexgen-dev
left a comment
abhishek-nexgen-dev
left a comment
There was a problem hiding this comment.
The Forgot Password page is not Looking good make sure the Ui looks good and Attrect Others
Reference login Page |
Rohanrathod7
force-pushed
the
feature-password-reset
branch
from
04466fa to
ce2499f
Compare
|
Just Updated Feature: Password Reset please review it |
|
@Rohanrathod7 add screenshot |
resolved margin and responsivenessA screenshots are added in PR's description |
|
Whenever you are ready to merge, remind me |
2 participants
Password Reset Data Flow
Closes #58
🔐 Password Reset Implementation
Description
This PR implements the secure password reset flow, including:
/forgot-passwordand/reset-passwordendpoints.Related PRs
Process Flow Explained
Phase 1: Requesting a Reset Link (Forgot Password)
ForgotPassword.tsxand clicks "Send Reset Link".forgotPassword(email)inauth.service.ts.POST /api/v1/user/auth/forgot-passworduser.service.ts):token).sha256hash of that token.resetPasswordToken: The hashed version.resetPasswordExpire: Expiration time (10 minutes).http://localhost:5173/reset-password/${resetToken}Phase 2: Resetting the Password
ResetPassword.tsx. Thetokenis extracted from the URL (useParams).resetPassword(token, password)inauth.service.ts.POST /api/v1/user/auth/reset-password/:tokenuser.service.ts):tokenreceived from the URL usingsha256.resetPasswordTokenmatches the hashed token.resetPasswordExpireis greater thanDate.now()(not expired).passwordfield.resetPasswordTokenandresetPasswordExpiretonull(consumes the token).🔑 Key Security Features
Previous Screens