new password reset process for the mobile app #4294
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive password reset mechanism for the authentication service. The implementation spans multiple files, adding new methods to handle password reset requests, actual password resets, and mobile user registration. The changes include creating routes, validators, controllers, utility functions, and email messaging to support a secure and user-friendly password reset process. Additionally, it enhances user verification and registration functionalities, allowing users to verify their mobile email and register through mobile devices. Changes
Sequence DiagramsequenceDiagram
participant User
participant AuthService
participant EmailService
participant Database
User->>AuthService: Request Password Reset
AuthService->>Database: Generate Reset Token
Database-->>AuthService: Return Token
AuthService->>EmailService: Send Reset Email
EmailService-->>User: Email with Reset Token
User->>AuthService: Submit New Password with Token
AuthService->>Database: Validate Token & Update Password
Database-->>AuthService: Confirm Password Reset
AuthService-->>User: Password Reset Successful
Possibly Related PRs
Poem
✨ Finishing Touches
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #4294 +/- ##
===========================================
- Coverage 11.33% 11.31% -0.03%
===========================================
Files 154 154
Lines 17882 17932 +50
Branches 380 388 +8
===========================================
+ Hits 2027 2029 +2
- Misses 15853 15901 +48
Partials 2 2
|
|
Auth-service changes in this PR available for preview here |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
src/auth-service/utils/common/email.msgs.js (1)
44-56: Well-structured email template with clear instructions!The template effectively communicates the password reset process. Consider adding security advice like "If you didn't request this reset, please contact support."
Apply this diff to add the security notice:
<p>You requested a password reset for your AirQo account associated with ${email}.</p> <p>Use this code to finish setting up your new password:</p> <h1 style="font-size: 36px; font-weight: bold; margin: 20px 0;">${token}</h1> <p>This code will expire in 24 hours.</p> + <p>If you didn't request this password reset, please contact our support team immediately.</p> </td>src/auth-service/controllers/user.controller.js (1)
1703-1744: Enhance error handling and logging.The error handling could be improved by:
- Adding specific error types for different failure scenarios
- Sanitizing error messages in production
- Adding structured logging for security events
Apply this diff to enhance error handling:
resetPassword: async (req, res, next) => { try { const errors = extractErrorsFromRequest(req); if (errors) { next( new HttpError("bad request errors", httpStatus.BAD_REQUEST, errors) ); return; } const { token } = req.params; const { password } = req.body; const defaultTenant = constants.DEFAULT_TENANT || "airqo"; const tenant = isEmpty(req.query.tenant) ? defaultTenant : req.query.tenant; + // Log security event + logger.info(`Password reset attempted for tenant: ${tenant}`); + const result = await userUtil.resetPassword( { token, password, tenant, }, next ); + // Log successful password reset + logger.info(`Password reset successful for tenant: ${tenant}`); + res .status(httpStatus.OK) - .json({ success: true, message: result.message }); + .json({ success: true, message: "Password has been reset successfully" }); } catch (error) { - logObject("error in controller", error); + // Log error without sensitive details + logger.error(`Password reset failed for tenant: ${tenant}`, { + error_type: error.name, + error_code: error.code + }); logger.error(`🐛🐛 Internal Server Error ${error.message}`); next( new HttpError( "Internal Server Error", httpStatus.INTERNAL_SERVER_ERROR, - { message: error.message } + { message: "An error occurred while resetting the password" } ) ); return; } },src/auth-service/utils/common/mailer.js (1)
1315-1362: LGTM! The implementation follows established patterns.The new
sendPasswordResetEmailmethod is well-structured and follows the existing patterns in the codebase. It properly handles email notifications, error cases, and includes the token in both subject and content.A few suggestions to enhance the implementation:
- Consider moving the token from email subject to body only for better security
- Add rate limiting to prevent abuse
- subject: `Password Reset Code: ${token}`, + subject: `Password Reset Request`,src/auth-service/utils/user.util.js (2)
2051-2085: LGTM! Well-structured password reset initiation.The implementation includes proper error handling, atomic updates, and token expiration. Good use of findOneAndUpdate with new:true option.
Consider adding rate limiting to prevent abuse:
initiatePasswordReset: async ({ email, token, tenant }, next) => { try { + const rateLimitKey = `pwd_reset_${email}`; + const attempts = await redis.incr(rateLimitKey); + if (attempts === 1) await redis.expire(rateLimitKey, 3600); + if (attempts > 3) { + throw new HttpError("Too many attempts", httpStatus.TOO_MANY_REQUESTS); + } const update = {
2087-2147: LGTM! Comprehensive password reset implementation.The implementation includes proper token validation, password update, and confirmation email. Good use of moment.js for timezone handling.
Consider adding password complexity validation:
resetPassword: async ({ token, password, tenant }, next) => { try { + const passwordRegex = /^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)[a-zA-Z\d]{8,}$/; + if (!passwordRegex.test(password)) { + throw new HttpError( + "Password must be at least 8 characters and contain uppercase, lowercase and numbers", + httpStatus.BAD_REQUEST + ); + } const resetPasswordToken = token;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/auth-service/controllers/user.controller.js(1 hunks)src/auth-service/routes/v2/users.routes.js(1 hunks)src/auth-service/utils/common/email.msgs.js(1 hunks)src/auth-service/utils/common/mailer.js(1 hunks)src/auth-service/utils/user.util.js(1 hunks)src/auth-service/validators/users.validators.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-push-deploy-auth-service
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
src/auth-service/routes/v2/users.routes.js (1)
206-216: Well-structured route implementation!The new password reset routes are properly organized with appropriate middleware chain and follow RESTful conventions.
| const resetPasswordRequest = [ | ||
| body("email") | ||
| .exists() | ||
| .withMessage("Email is required") | ||
| .isEmail() | ||
| .withMessage("Invalid email format") | ||
| .trim(), | ||
| //Potentially add tenant validation here as well, using the oneOf approach if necessary | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add tenant validation as noted in the comment.
The validator should include tenant validation to maintain consistency with other endpoints in the system.
Apply this diff to add tenant validation:
const resetPasswordRequest = [
+ validateTenant,
+ [
body("email")
.exists()
.withMessage("Email is required")
.isEmail()
.withMessage("Invalid email format")
.trim(),
+ ]
//Potentially add tenant validation here as well, using the oneOf approach if necessary
];Committable suggestion skipped: line range outside the PR's diff.
| const resetPassword = [ | ||
| param("token") | ||
| .exists() | ||
| .withMessage("Token is required") | ||
| .bail() | ||
| .isNumeric() | ||
| .withMessage("Token must be numeric") | ||
| .isLength({ min: 5, max: 5 }) | ||
| .withMessage("Token must be 5 digits") | ||
| .trim(), | ||
| body("password") | ||
| .exists() | ||
| .withMessage("Password is required") | ||
| .bail() | ||
| .isLength({ min: 6 }) | ||
| .withMessage("Password must be at least 6 characters long") | ||
| .trim(), | ||
| body("confirmPassword") | ||
| .exists() | ||
| .withMessage("Confirm Password is required") | ||
| .bail() | ||
| .custom((value, { req }) => { | ||
| if (value !== req.body.password) { | ||
| throw new Error("Passwords do not match"); | ||
| } | ||
| return true; | ||
| }) | ||
| .trim(), | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance password security requirements.
The current password validation could be strengthened by:
- Adding maximum length limit
- Requiring complexity (uppercase, lowercase, special characters)
- Preventing common passwords
Apply this diff to enhance password security:
body("password")
.exists()
.withMessage("Password is required")
.bail()
.isLength({ min: 6 })
.withMessage("Password must be at least 6 characters long")
+ .isLength({ max: 128 })
+ .withMessage("Password must not exceed 128 characters")
+ .matches(/^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[@$!%*?&])[A-Za-z\d@$!%*?&]{6,}$/)
+ .withMessage("Password must contain at least one uppercase letter, one lowercase letter, one number and one special character")
.trim(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const resetPassword = [ | |
| param("token") | |
| .exists() | |
| .withMessage("Token is required") | |
| .bail() | |
| .isNumeric() | |
| .withMessage("Token must be numeric") | |
| .isLength({ min: 5, max: 5 }) | |
| .withMessage("Token must be 5 digits") | |
| .trim(), | |
| body("password") | |
| .exists() | |
| .withMessage("Password is required") | |
| .bail() | |
| .isLength({ min: 6 }) | |
| .withMessage("Password must be at least 6 characters long") | |
| .trim(), | |
| body("confirmPassword") | |
| .exists() | |
| .withMessage("Confirm Password is required") | |
| .bail() | |
| .custom((value, { req }) => { | |
| if (value !== req.body.password) { | |
| throw new Error("Passwords do not match"); | |
| } | |
| return true; | |
| }) | |
| .trim(), | |
| ]; | |
| const resetPassword = [ | |
| param("token") | |
| .exists() | |
| .withMessage("Token is required") | |
| .bail() | |
| .isNumeric() | |
| .withMessage("Token must be numeric") | |
| .isLength({ min: 5, max: 5 }) | |
| .withMessage("Token must be 5 digits") | |
| .trim(), | |
| body("password") | |
| .exists() | |
| .withMessage("Password is required") | |
| .bail() | |
| .isLength({ min: 6 }) | |
| .withMessage("Password must be at least 6 characters long") | |
| .isLength({ max: 128 }) | |
| .withMessage("Password must not exceed 128 characters") | |
| .matches(/^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[@$!%*?&])[A-Za-z\d@$!%*?&]{6,}$/) | |
| .withMessage("Password must contain at least one uppercase letter, one lowercase letter, one number and one special character") | |
| .trim(), | |
| body("confirmPassword") | |
| .exists() | |
| .withMessage("Confirm Password is required") | |
| .bail() | |
| .custom((value, { req }) => { | |
| if (value !== req.body.password) { | |
| throw new Error("Passwords do not match"); | |
| } | |
| return true; | |
| }) | |
| .trim(), | |
| ]; |
| resetPasswordRequest: async (req, res, next) => { | ||
| try { | ||
| const errors = extractErrorsFromRequest(req); | ||
| if (errors) { | ||
| next( | ||
| new HttpError("bad request errors", httpStatus.BAD_REQUEST, errors) | ||
| ); | ||
| return; | ||
| } | ||
| const { email } = req.body; | ||
| const tenant = req.query.tenant; | ||
| const token = userUtil.generateNumericToken(5); | ||
| const result = await userUtil.initiatePasswordReset( | ||
| { | ||
| email, | ||
| token, | ||
| tenant, | ||
| }, | ||
| next | ||
| ); | ||
|
|
||
| res | ||
| .status(httpStatus.OK) | ||
| .json({ success: true, message: result.message }); | ||
| } catch (error) { | ||
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | ||
| next( | ||
| new HttpError( | ||
| "Internal Server Error", | ||
| httpStatus.INTERNAL_SERVER_ERROR, | ||
| { message: error.message } | ||
| ) | ||
| ); | ||
| return; | ||
| } | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Rate limiting implementation is required for password reset endpoints
The current rate limiting middleware in the auth service is designed for authenticated API clients and isn't suitable for public endpoints like password reset. You'll need to:
- Implement IP-based rate limiting for the password reset endpoints
- Use express-rate-limit package which is already installed
- Consider a stricter limit (e.g., 5 requests per 15 minutes) for these sensitive endpoints
Example implementation:
const rateLimit = require('express-rate-limit');
const passwordResetLimiter = rateLimit({
windowMs: 15 * 60 * 1000, // 15 minutes
max: 5 // limit each IP to 5 requests per window
});
router.post('/resetPasswordRequest', passwordResetLimiter, ...);🔗 Analysis chain
Add rate limiting to prevent brute force attacks.
The password reset request endpoint should implement rate limiting to prevent abuse. Also, consider using a consistent response format that doesn't leak information about email existence.
Let's verify if rate limiting is implemented elsewhere in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for rate limiting implementations
rg -A 5 "rate.?limit|rateLimiter"
Length of output: 9490
Script:
#!/bin/bash
# Check user routes and rate limit implementation
echo "=== User Routes Configuration ==="
rg -A 10 "router.*user" src/auth-service/routes/
echo -e "\n=== Rate Limit Middleware Implementation ==="
cat src/auth-service/middleware/rate-limit.js
Length of output: 5957
src/auth-service/utils/user.util.js
Outdated
| generateNumericToken: (length) => { | ||
| let token = ""; | ||
| for (let i = 0; i < length; i++) { | ||
| token += Math.floor(Math.random() * 10); // Generates a random digit (0-9) | ||
| } | ||
| return token; | ||
| }, |
There was a problem hiding this comment.
Security: Use cryptographically secure random number generation.
The current implementation uses Math.random() which is not cryptographically secure and could be predictable. This is critical for security tokens.
Replace with crypto.randomBytes():
- generateNumericToken: (length) => {
- let token = "";
- for (let i = 0; i < length; i++) {
- token += Math.floor(Math.random() * 10); // Generates a random digit (0-9)
- }
- return token;
- },
+ generateNumericToken: (length) => {
+ const charset = "0123456789";
+ let token = "";
+ const byteLength = Math.ceil(length * 0.5);
+ while (token.length < length) {
+ const randomBytes = crypto.randomBytes(byteLength);
+ for (let i = 0; i < randomBytes.length && token.length < length; i++) {
+ const randomIndex = randomBytes[i] % charset.length;
+ token += charset[randomIndex];
+ }
+ }
+ return token;
+ },Committable suggestion skipped: line range outside the PR's diff.
|
Auth-service changes in this PR available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
src/auth-service/validators/users.validators.js (2)
874-882:⚠️ Potential issueEnhance security for password reset request endpoint
The current implementation needs additional security measures:
- Add tenant validation as noted in the comment
- Implement rate limiting to prevent brute force attacks
Apply this diff to add tenant validation:
const resetPasswordRequest = [ + validateTenant, body("email") .exists() .withMessage("Email is required") .isEmail() .withMessage("Invalid email format") .trim(), - //Potentially add tenant validation here as well, using the oneOf approach if necessary ];
884-912: 🛠️ Refactor suggestionStrengthen password validation requirements
The current password validation only checks for minimum length. Consider adding:
- Maximum length limit
- Complexity requirements (uppercase, lowercase, numbers, special characters)
- Common password check
Apply this diff to enhance password security:
body("password") .exists() .withMessage("Password is required") .bail() .isLength({ min: 6 }) .withMessage("Password must be at least 6 characters long") + .isLength({ max: 128 }) + .withMessage("Password must not exceed 128 characters") + .matches(/^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[@$!%*?&])[A-Za-z\d@$!%*?&]{6,}$/) + .withMessage("Password must contain at least one uppercase letter, one lowercase letter, one number and one special character") .trim(),
🧹 Nitpick comments (6)
src/auth-service/utils/common/email.templates.js (1)
149-152: Consider adding JSDoc documentation for the updated method signature.The
afterEmailVerificationmethod has been updated with new parameters, but lacks documentation explaining the purpose ofanalyticsVersion.+/** + * Generates email content after email verification + * @param {Object} options - The options object + * @param {string} options.firstName - User's first name + * @param {string} options.username - User's username + * @param {string} options.email - User's email + * @param {number} [options.analyticsVersion=3] - Version number (3 for web, 4 for mobile) + * @param {Function} next - Callback function + * @returns {Object} Email content object + */ afterEmailVerification: ( { firstName, username, email, analyticsVersion = 3 } = {}, next ) => {🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 149-149: src/auth-service/utils/common/email.templates.js#L149
Added line #L149 was not covered by testssrc/auth-service/middleware/passport.js (1)
335-372: Consider reducing code duplication between email and username strategies.The mobile verification logic is duplicated between email and username strategies.
Extract the common logic into a shared function:
+const handleMobileVerification = async (user, tenant, req, next) => { + await createUserUtil + .mobileVerificationReminder({ tenant, email: user.email }, next) + .then((verificationResponse) => { + if (!verificationResponse || verificationResponse.success === false) { + logger.error( + `Verification reminder failed: ${ + verificationResponse ? verificationResponse.message : "No response" + }` + ); + } + }) + .catch((err) => { + logger.error(`Error sending verification reminder: ${err.message}`); + }); + + req.auth.success = false; + req.auth.message = "account not verified, verification email has been sent to your email"; + req.auth.status = httpStatus.FORBIDDEN; + next( + new HttpError( + "account not verified, verification email has been sent to your email", + httpStatus.FORBIDDEN, + { + message: "account not verified, verification email has been sent to your email", + } + ) + ); +};Then use it in both strategies:
} else if (user.analyticsVersion === 4 && !user.verified) { return handleMobileVerification(user, tenant, req, next); }src/auth-service/routes/v2/users.routes.js (1)
221-223: Consider grouping related routes together.The user update routes could be grouped with other user modification routes for better organization.
Consider moving these routes near other user update routes for better maintainability.
src/auth-service/controllers/user.controller.js (2)
1703-1744: Add detailed logging for password reset attemptsConsider adding more detailed logging to help with security auditing and debugging:
Apply this diff to enhance logging:
const result = await userUtil.resetPassword( { token, password, tenant, }, next ); +logger.info(`Password reset attempt for token: ${token.slice(-2)} (last 2 digits)`);
1746-1788: Add input sanitization for mobile user registrationConsider adding input sanitization to prevent potential XSS or injection attacks:
Apply this diff to add sanitization:
+const sanitizeInput = (input) => { + return input.replace(/[<>]/g, ''); +}; + request.body.analyticsVersion = 4; +if (request.body.firstName) { + request.body.firstName = sanitizeInput(request.body.firstName); +} +if (request.body.lastName) { + request.body.lastName = sanitizeInput(request.body.lastName); +}src/auth-service/utils/user.util.js (1)
2870-2870: Consider separating utility functions.The module exports are mixing utility functions with the main user module. Consider moving utility functions like
generateNumericTokento a separate utilities file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/auth-service/controllers/user.controller.js(1 hunks)src/auth-service/middleware/passport.js(2 hunks)src/auth-service/models/User.js(1 hunks)src/auth-service/routes/v2/users.routes.js(2 hunks)src/auth-service/utils/common/email.msgs.js(2 hunks)src/auth-service/utils/common/email.templates.js(2 hunks)src/auth-service/utils/common/mailer.js(4 hunks)src/auth-service/utils/user.util.js(4 hunks)src/auth-service/validators/users.validators.js(3 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/auth-service/utils/common/email.templates.js
[warning] 149-149: src/auth-service/utils/common/email.templates.js#L149
Added line #L149 was not covered by tests
[warning] 154-154: src/auth-service/utils/common/email.templates.js#L154
Added line #L154 was not covered by tests
[warning] 157-157: src/auth-service/utils/common/email.templates.js#L157
Added line #L157 was not covered by tests
[warning] 173-173: src/auth-service/utils/common/email.templates.js#L173
Added line #L173 was not covered by tests
src/auth-service/utils/common/mailer.js
[warning] 883-885: src/auth-service/utils/common/mailer.js#L883-L885
Added lines #L883 - L885 were not covered by tests
[warning] 889-889: src/auth-service/utils/common/mailer.js#L889
Added line #L889 was not covered by tests
[warning] 891-891: src/auth-service/utils/common/mailer.js#L891
Added line #L891 was not covered by tests
[warning] 894-894: src/auth-service/utils/common/mailer.js#L894
Added line #L894 was not covered by tests
[warning] 897-897: src/auth-service/utils/common/mailer.js#L897
Added line #L897 was not covered by tests
[warning] 899-901: src/auth-service/utils/common/mailer.js#L899-L901
Added lines #L899 - L901 were not covered by tests
[warning] 906-906: src/auth-service/utils/common/mailer.js#L906
Added line #L906 was not covered by tests
[warning] 910-910: src/auth-service/utils/common/mailer.js#L910
Added line #L910 was not covered by tests
[warning] 913-913: src/auth-service/utils/common/mailer.js#L913
Added line #L913 was not covered by tests
[warning] 956-956: src/auth-service/utils/common/mailer.js#L956
Added line #L956 was not covered by tests
[warning] 964-965: src/auth-service/utils/common/mailer.js#L964-L965
Added lines #L964 - L965 were not covered by tests
[warning] 967-967: src/auth-service/utils/common/mailer.js#L967
Added line #L967 was not covered by tests
[warning] 974-974: src/auth-service/utils/common/mailer.js#L974
Added line #L974 was not covered by tests
[warning] 986-988: src/auth-service/utils/common/mailer.js#L986-L988
Added lines #L986 - L988 were not covered by tests
[warning] 1445-1447: src/auth-service/utils/common/mailer.js#L1445-L1447
Added lines #L1445 - L1447 were not covered by tests
[warning] 1451-1451: src/auth-service/utils/common/mailer.js#L1451
Added line #L1451 was not covered by tests
[warning] 1454-1454: src/auth-service/utils/common/mailer.js#L1454
Added line #L1454 was not covered by tests
[warning] 1465-1466: src/auth-service/utils/common/mailer.js#L1465-L1466
Added lines #L1465 - L1466 were not covered by tests
[warning] 1469-1469: src/auth-service/utils/common/mailer.js#L1469
Added line #L1469 was not covered by tests
[warning] 1476-1476: src/auth-service/utils/common/mailer.js#L1476
Added line #L1476 was not covered by tests
[warning] 1483-1484: src/auth-service/utils/common/mailer.js#L1483-L1484
Added lines #L1483 - L1484 were not covered by tests
src/auth-service/utils/common/email.msgs.js
[warning] 44-45: src/auth-service/utils/common/email.msgs.js#L44-L45
Added lines #L44 - L45 were not covered by tests
[warning] 55-55: src/auth-service/utils/common/email.msgs.js#L55
Added line #L55 was not covered by tests
[warning] 57-58: src/auth-service/utils/common/email.msgs.js#L57-L58
Added lines #L57 - L58 were not covered by tests
[warning] 69-69: src/auth-service/utils/common/email.msgs.js#L69
Added line #L69 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build-push-deploy-auth-service
- GitHub Check: Code Coverage for device-registry
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (14)
src/auth-service/utils/common/email.templates.js (1)
156-192: LGTM! Clear separation of email templates based on analytics version.The implementation properly handles different email templates:
- Version 4 (mobile): Simplified message focusing on mobile app access
- Version 3 (web): Detailed message with username and access link
However, consider adding unit tests for both versions to ensure proper coverage.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 157-157: src/auth-service/utils/common/email.templates.js#L157
Added line #L157 was not covered by tests
[warning] 173-173: src/auth-service/utils/common/email.templates.js#L173
Added line #L173 was not covered by testssrc/auth-service/models/User.js (1)
406-413: LGTM! Improved user creation flexibility with email fallback.The implementation properly handles userName assignment:
- Uses email as userName if userName is not provided
- Maintains data integrity by requiring userName to be set
Consider adding a test case to verify this behavior.
src/auth-service/middleware/passport.js (1)
175-212: LGTM! Well-structured mobile verification handling in email strategy.The implementation properly handles mobile verification:
- Checks for analyticsVersion 4 and unverified status
- Sends verification reminder
- Provides clear user feedback
src/auth-service/routes/v2/users.routes.js (2)
197-201: LGTM! Well-structured password reset routes.The implementation follows RESTful practices:
/reset-password-requestfor initiating reset/reset-password/:tokenfor completing resetAlso applies to: 203-206
209-219: LGTM! Clear mobile-specific registration and verification routes.The implementation properly separates mobile-specific functionality:
/registerfor mobile user registration/verify-email/:tokenfor mobile email verificationsrc/auth-service/validators/users.validators.js (1)
332-359: LGTM! Well-implemented validation for mobile email verificationThe implementation includes all necessary validations:
- Tenant validation
- Token format and length check
- Email format validation
src/auth-service/utils/common/email.msgs.js (2)
559-560: LGTM! Clear separation of web and mobile instructionsThe modifications provide clear instructions for both web and mobile users, improving the user experience.
57-70: Add test coverage for mobile email verification templateThe template implementation looks good, but test coverage is missing according to the static analysis report.
Run this script to check existing test coverage:
Would you like me to help create test cases for this email template?
✅ Verification successful
Review comment is accurate - test coverage needed for mobile email verification template
The codebase has extensive test coverage for email sending functionality, but lacks specific tests for the mobile email verification template content. Adding template tests would help ensure:
- Correct token placement and formatting
- Proper email structure
- Consistent welcome messaging
- 24-hour expiration notice
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing tests for email templates rg -l "mobileEmailVerification" test/Length of output: 83
Script:
#!/bin/bash # Find all test-related files to understand testing structure echo "=== Finding test files ===" fd -t f ".(test|spec)\.(js|jsx|ts|tsx)$" echo -e "\n=== Finding test directories ===" fd -t d "(test|tests|__tests__|spec)" echo -e "\n=== Searching for mobileEmailVerification in all files ===" rg "mobileEmailVerification" -lLength of output: 3136
Script:
#!/bin/bash # Check test coverage in ut_mailer.js echo "=== Checking test content ===" cat src/auth-service/utils/common/test/ut_mailer.js echo -e "\n=== Checking mailer implementation ===" cat src/auth-service/utils/common/mailer.jsLength of output: 110576
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 57-58: src/auth-service/utils/common/email.msgs.js#L57-L58
Added lines #L57 - L58 were not covered by tests
[warning] 69-69: src/auth-service/utils/common/email.msgs.js#L69
Added line #L69 was not covered by testssrc/auth-service/controllers/user.controller.js (2)
1790-1828: LGTM! Well-implemented mobile email verificationThe implementation includes proper error handling, success responses, and follows the established patterns in the codebase.
1667-1702:⚠️ Potential issueImprove security of password reset request endpoint
- The success response could leak user existence. Consider using a consistent response regardless of whether the email exists.
- Rate limiting should be implemented to prevent brute force attacks.
Apply this diff to use consistent responses:
- res - .status(httpStatus.OK) - .json({ success: true, message: result.message }); + res.status(httpStatus.OK).json({ + success: true, + message: "If your email exists in our system, you will receive a password reset code." + });Likely invalid or redundant comment.
src/auth-service/utils/common/mailer.js (3)
Line range hint
1120-1177: LGTM! Well-implemented email verification handlingThe modifications properly handle the new analyticsVersion parameter and maintain the existing error handling patterns.
1445-1492: Add test coverage for password reset email sendingThe implementation looks good but lacks test coverage according to the static analysis report.
Run this script to check existing test coverage:
Would you like me to help create test cases for this functionality?
✅ Verification successful
Test coverage needed for password reset email functionality
The review comment is correct. While the mailer module has extensive test coverage for other email functions, the
sendPasswordResetEmailfunction lacks test coverage. Consider adding tests following the established pattern inut_mailer.js:
- Test successful email sending
- Test email delivery failure scenario
- Test internal server error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing tests for email sending rg -l "sendPasswordResetEmail" test/Length of output: 82
Script:
#!/bin/bash # First find all test files in the repository echo "=== Finding test directories ===" fd -t d "test|tests|__tests__" echo -e "\n=== Looking for test files related to mailer ===" fd -t f "mail|mailer" -e test.js -e spec.js -e test.ts -e spec.ts echo -e "\n=== Looking for any test files in auth-service ===" fd -t f "(test|spec)\.(js|ts)$" "src/auth-service" echo -e "\n=== Check source file location ===" fd -t f "mailer.js$"Length of output: 3433
Script:
#!/bin/bash # Check content of the mailer test file echo "=== Checking test coverage for sendPasswordResetEmail ===" cat "src/auth-service/utils/common/test/ut_mailer.js"Length of output: 45066
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1445-1447: src/auth-service/utils/common/mailer.js#L1445-L1447
Added lines #L1445 - L1447 were not covered by tests
[warning] 1451-1451: src/auth-service/utils/common/mailer.js#L1451
Added line #L1451 was not covered by tests
[warning] 1454-1454: src/auth-service/utils/common/mailer.js#L1454
Added line #L1454 was not covered by tests
[warning] 1465-1466: src/auth-service/utils/common/mailer.js#L1465-L1466
Added lines #L1465 - L1466 were not covered by tests
[warning] 1469-1469: src/auth-service/utils/common/mailer.js#L1469
Added line #L1469 was not covered by tests
[warning] 1476-1476: src/auth-service/utils/common/mailer.js#L1476
Added line #L1476 was not covered by tests
[warning] 1483-1484: src/auth-service/utils/common/mailer.js#L1483-L1484
Added lines #L1483 - L1484 were not covered by tests
883-996: Add test coverage for verification email sendingThe implementation looks good but lacks test coverage according to the static analysis report.
Run this script to check existing test coverage:
Would you like me to help create test cases for this functionality?
✅ Verification successful
Test coverage needed for sendVerificationEmail function
The review comment is accurate. While the codebase has extensive test coverage for other email-related functions like
candidate,inquiry,user,forgot, etc., thesendVerificationEmailfunction lacks test coverage. This should be addressed following the project's established testing patterns.Key areas to test:
- Successful email verification
- Failed notification status check
- Email sending failures
- Automated test email handling
- BCC email subscription verification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing tests for email sending rg -l "sendVerificationEmail" test/Length of output: 81
Script:
#!/bin/bash # Find all potential test files and package.json echo "=== Searching for test files ===" fd -e test.js -e spec.js echo -e "\n=== Checking package.json for test patterns ===" find . -name "package.json" -exec cat {} \; | grep -A 5 '"test":' echo -e "\n=== Searching for any file mentioning sendVerificationEmail ===" rg -l "sendVerificationEmail"Length of output: 1649
Script:
#!/bin/bash # Find all test directories and ut_* files echo "=== Searching for test directories and unit test files ===" fd -t d "test$" fd "ut_.*\.js$" echo -e "\n=== Checking content of any mailer or user util tests ===" rg -l "mailer|sendVerificationEmail" -g "ut_*.js"Length of output: 19992
Script:
#!/bin/bash echo "=== Checking mailer test content ===" cat src/auth-service/utils/common/test/ut_mailer.js echo -e "\n=== Checking user util test content ===" cat src/auth-service/utils/test/ut_user.util.jsLength of output: 110706
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 883-885: src/auth-service/utils/common/mailer.js#L883-L885
Added lines #L883 - L885 were not covered by tests
[warning] 889-889: src/auth-service/utils/common/mailer.js#L889
Added line #L889 was not covered by tests
[warning] 891-891: src/auth-service/utils/common/mailer.js#L891
Added line #L891 was not covered by tests
[warning] 894-894: src/auth-service/utils/common/mailer.js#L894
Added line #L894 was not covered by tests
[warning] 897-897: src/auth-service/utils/common/mailer.js#L897
Added line #L897 was not covered by tests
[warning] 899-901: src/auth-service/utils/common/mailer.js#L899-L901
Added lines #L899 - L901 were not covered by tests
[warning] 906-906: src/auth-service/utils/common/mailer.js#L906
Added line #L906 was not covered by tests
[warning] 910-910: src/auth-service/utils/common/mailer.js#L910
Added line #L910 was not covered by tests
[warning] 913-913: src/auth-service/utils/common/mailer.js#L913
Added line #L913 was not covered by tests
[warning] 956-956: src/auth-service/utils/common/mailer.js#L956
Added line #L956 was not covered by tests
[warning] 964-965: src/auth-service/utils/common/mailer.js#L964-L965
Added lines #L964 - L965 were not covered by tests
[warning] 967-967: src/auth-service/utils/common/mailer.js#L967
Added line #L967 was not covered by tests
[warning] 974-974: src/auth-service/utils/common/mailer.js#L974
Added line #L974 was not covered by tests
[warning] 986-988: src/auth-service/utils/common/mailer.js#L986-L988
Added lines #L986 - L988 were not covered by testssrc/auth-service/utils/user.util.js (1)
2168-2202: LGTM! Well-implemented password reset initiation.The initiatePasswordReset function follows security best practices:
- Sets token expiration
- Uses secure token generation
- Proper error handling with next()
- Sends email notification
| const { log } = require("async"); | ||
| const { logObject } = require("@utils/shared"); |
There was a problem hiding this comment.
💡 Codebase verification
Remove unused imports log and logObject
These imports are not used anywhere in the file and should be removed:
const { log } = require("async");const { logObject } = require("@utils/shared");
🔗 Analysis chain
Verify the necessity of unused imports.
The newly added imports log from 'async' and logObject from '@utils/shared' appear to be unused in this file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if these imports are used elsewhere in the file
rg -l "log\(" src/auth-service/utils/common/email.templates.js
rg -l "logObject\(" src/auth-service/utils/common/email.templates.js
Length of output: 135
Script:
#!/bin/bash
# Search for any usage of these imports with context
echo "=== Searching for 'log' usage ==="
rg "log" -A 2 -B 2 src/auth-service/utils/common/email.templates.js
echo -e "\n=== Searching for 'logObject' usage ==="
rg "logObject" -A 2 -B 2 src/auth-service/utils/common/email.templates.js
# Also check the full content to understand the context
echo -e "\n=== Full file content ==="
cat src/auth-service/utils/common/email.templates.js
Length of output: 16099
| mobilePasswordReset: ({ token, email }) => { | ||
| const content = ` | ||
| <tr> | ||
| <td style="color: #344054; font-size: 16px; font-family: Inter; font-weight: 400; line-height: 24px; word-wrap: break-word;"> | ||
| <p>You requested a password reset for your AirQo account associated with ${email}.</p> | ||
| <p>Use this code to finish setting up your new password:</p> | ||
| <h1 style="font-size: 36px; font-weight: bold; margin: 20px 0;">${token}</h1> | ||
| <p>This code will expire in 24 hours.</p> | ||
| </td> | ||
| </tr> | ||
| `; | ||
| return constants.EMAIL_BODY({ email, content }); | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
Test coverage missing for mobile password reset email template - Confirmed
The test file src/auth-service/utils/common/test/ut_email.msgs.js exists but lacks test coverage for the mobilePasswordReset template, while other email templates are well tested. The template is actively used in the password reset functionality.
🔗 Analysis chain
Add test coverage for mobile password reset email template
The template implementation looks good, but test coverage is missing according to the static analysis report.
Run this script to check existing test coverage:
Would you like me to help create test cases for this email template?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing tests for email templates
rg -l "mobilePasswordReset" test/
Length of output: 79
Script:
#!/bin/bash
# First, verify the actual file location
fd email.msgs.js
# Search for test files in common test directories and patterns
fd -e test.js
fd -e spec.js
# Search for any files containing mobilePasswordReset
rg -l "mobilePasswordReset"
Length of output: 356
Script:
#!/bin/bash
# Check the content of the test file
cat src/auth-service/utils/common/test/ut_email.msgs.js
# Also check mailer.js for context
cat src/auth-service/utils/common/mailer.js
Length of output: 88628
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 44-45: src/auth-service/utils/common/email.msgs.js#L44-L45
Added lines #L44 - L45 were not covered by tests
[warning] 55-55: src/auth-service/utils/common/email.msgs.js#L55
Added line #L55 was not covered by tests
src/auth-service/utils/user.util.js
Outdated
| verifyMobileEmail: async (token) => { | ||
| try { | ||
| // ... your logic to retrieve the user associated with the verification token from the database ... | ||
| const user = await UserModel("airqo").findOne({ | ||
| verificationToken: token, | ||
| }); | ||
|
|
||
| if (!user) { | ||
| return { success: false, message: "Invalid verification token." }; | ||
| } | ||
|
|
||
| user.verified = true; | ||
| user.verificationToken = undefined; // Clear the token after verification | ||
| await user.save(); | ||
|
|
||
| return { | ||
| success: true, | ||
| message: "Email verified successfully.", | ||
| user: user, | ||
| }; | ||
| } catch (error) { | ||
| return { success: false, message: error.message }; | ||
| } | ||
| }, |
There was a problem hiding this comment.
Fix hardcoded tenant in verifyMobileEmail function.
The function has a hardcoded tenant "airqo" which limits its reusability.
Apply this diff to make the tenant configurable:
-verifyMobileEmail: async (token) => {
+verifyMobileEmail: async (token, tenant = "airqo") => {
try {
- const user = await UserModel("airqo").findOne({
+ const user = await UserModel(tenant).findOne({
verificationToken: token,
});
if (!user) {
return { success: false, message: "Invalid verification token." };
}
user.verified = true;
user.verificationToken = undefined;
await user.save();
return {
success: true,
message: "Email verified successfully.",
user: user,
};
} catch (error) {
return { success: false, message: error.message };
}
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| verifyMobileEmail: async (token) => { | |
| try { | |
| // ... your logic to retrieve the user associated with the verification token from the database ... | |
| const user = await UserModel("airqo").findOne({ | |
| verificationToken: token, | |
| }); | |
| if (!user) { | |
| return { success: false, message: "Invalid verification token." }; | |
| } | |
| user.verified = true; | |
| user.verificationToken = undefined; // Clear the token after verification | |
| await user.save(); | |
| return { | |
| success: true, | |
| message: "Email verified successfully.", | |
| user: user, | |
| }; | |
| } catch (error) { | |
| return { success: false, message: error.message }; | |
| } | |
| }, | |
| verifyMobileEmail: async (token, tenant = "airqo") => { | |
| try { | |
| // ... your logic to retrieve the user associated with the verification token from the database ... | |
| const user = await UserModel(tenant).findOne({ | |
| verificationToken: token, | |
| }); | |
| if (!user) { | |
| return { success: false, message: "Invalid verification token." }; | |
| } | |
| user.verified = true; | |
| user.verificationToken = undefined; // Clear the token after verification | |
| await user.save(); | |
| return { | |
| success: true, | |
| message: "Email verified successfully.", | |
| user: user, | |
| }; | |
| } catch (error) { | |
| return { success: false, message: error.message }; | |
| } | |
| }, |
src/auth-service/utils/user.util.js
Outdated
| registerMobileUser: async (request, next) => { | ||
| try { | ||
| const { tenant } = { | ||
| ...request.body, | ||
| ...request.query, | ||
| ...request.params, | ||
| }; | ||
|
|
||
| const userData = request.body; | ||
| const verificationToken = generateNumericToken(5); | ||
| userData.verificationToken = verificationToken; | ||
|
|
||
| const newUserResponse = await UserModel(tenant).register(userData, next); | ||
|
|
||
| if (newUserResponse.success === true) { | ||
| await mailer.sendVerificationEmail({ | ||
| email: userData.email, | ||
| token: verificationToken, | ||
| }); | ||
|
|
||
| return { | ||
| success: true, | ||
| message: "User registered successfully. Please verify your email.", | ||
| user: newUserResponse.data, | ||
| }; | ||
| } else { | ||
| return newUserResponse; | ||
| } | ||
| } catch (error) { | ||
| logObject("error in reg", error); | ||
| return { success: false, message: error.message }; | ||
| } | ||
| }, |
There was a problem hiding this comment.
Improve error handling in registerMobileUser function.
The function needs more robust error handling and validation:
- No validation for required fields
- Catches all errors without specific handling
- Returns generic error messages
Apply this diff to improve error handling:
registerMobileUser: async (request, next) => {
try {
const { tenant } = {
...request.body,
...request.query,
...request.params,
};
+ const requiredFields = ['email', 'password'];
+ const missingFields = requiredFields.filter(field => !request.body[field]);
+
+ if (missingFields.length > 0) {
+ next(
+ new HttpError(
+ "Bad Request Error",
+ httpStatus.BAD_REQUEST,
+ { message: `Missing required fields: ${missingFields.join(', ')}` }
+ )
+ );
+ }
const userData = request.body;
const verificationToken = generateNumericToken(5);
userData.verificationToken = verificationToken;
const newUserResponse = await UserModel(tenant).register(userData, next);
if (newUserResponse.success === true) {
await mailer.sendVerificationEmail({
email: userData.email,
token: verificationToken,
});
return {
success: true,
message: "User registered successfully. Please verify your email.",
user: newUserResponse.data,
};
} else {
return newUserResponse;
}
} catch (error) {
- logObject("error in reg", error);
- return { success: false, message: error.message };
+ logger.error(`Registration error: ${error.message}`);
+ next(
+ new HttpError(
+ "Internal Server Error",
+ httpStatus.INTERNAL_SERVER_ERROR,
+ { message: "Failed to register user", details: error.message }
+ )
+ );
}
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| registerMobileUser: async (request, next) => { | |
| try { | |
| const { tenant } = { | |
| ...request.body, | |
| ...request.query, | |
| ...request.params, | |
| }; | |
| const userData = request.body; | |
| const verificationToken = generateNumericToken(5); | |
| userData.verificationToken = verificationToken; | |
| const newUserResponse = await UserModel(tenant).register(userData, next); | |
| if (newUserResponse.success === true) { | |
| await mailer.sendVerificationEmail({ | |
| email: userData.email, | |
| token: verificationToken, | |
| }); | |
| return { | |
| success: true, | |
| message: "User registered successfully. Please verify your email.", | |
| user: newUserResponse.data, | |
| }; | |
| } else { | |
| return newUserResponse; | |
| } | |
| } catch (error) { | |
| logObject("error in reg", error); | |
| return { success: false, message: error.message }; | |
| } | |
| }, | |
| registerMobileUser: async (request, next) => { | |
| try { | |
| const { tenant } = { | |
| ...request.body, | |
| ...request.query, | |
| ...request.params, | |
| }; | |
| const requiredFields = ['email', 'password']; | |
| const missingFields = requiredFields.filter(field => !request.body[field]); | |
| if (missingFields.length > 0) { | |
| next( | |
| new HttpError( | |
| "Bad Request Error", | |
| httpStatus.BAD_REQUEST, | |
| { message: `Missing required fields: ${missingFields.join(', ')}` } | |
| ) | |
| ); | |
| } | |
| const userData = request.body; | |
| const verificationToken = generateNumericToken(5); | |
| userData.verificationToken = verificationToken; | |
| const newUserResponse = await UserModel(tenant).register(userData, next); | |
| if (newUserResponse.success === true) { | |
| await mailer.sendVerificationEmail({ | |
| email: userData.email, | |
| token: verificationToken, | |
| }); | |
| return { | |
| success: true, | |
| message: "User registered successfully. Please verify your email.", | |
| user: newUserResponse.data, | |
| }; | |
| } else { | |
| return newUserResponse; | |
| } | |
| } catch (error) { | |
| logger.error(`Registration error: ${error.message}`); | |
| next( | |
| new HttpError( | |
| "Internal Server Error", | |
| httpStatus.INTERNAL_SERVER_ERROR, | |
| { message: "Failed to register user", details: error.message } | |
| ) | |
| ); | |
| } | |
| }, |
| mobileVerificationReminder: async (request, next) => { | ||
| try { | ||
| const { tenant, email } = request; | ||
|
|
||
| const user = await UserModel(tenant) | ||
| .findOne({ email }) | ||
| .select("_id email firstName lastName verified") | ||
| .lean(); | ||
|
|
||
| if (isEmpty(user)) { | ||
| next( | ||
| new HttpError("Bad Request Error", httpStatus.BAD_REQUEST, { | ||
| message: "User not provided or does not exist", | ||
| }) | ||
| ); | ||
| } | ||
|
|
||
| const token = generateNumericToken(5); | ||
|
|
||
| const tokenCreationBody = { | ||
| token, | ||
| name: user.firstName, | ||
| }; | ||
| const responseFromCreateToken = await VerifyTokenModel( | ||
| tenant.toLowerCase() | ||
| ).register(tokenCreationBody, next); | ||
|
|
||
| if (responseFromCreateToken.success === false) { | ||
| return responseFromCreateToken; | ||
| } else { | ||
| const emailResponse = await mailer.sendVerificationEmail( | ||
| { email, token, tenant }, | ||
| next | ||
| ); | ||
| logObject("emailResponse", emailResponse); | ||
| if (emailResponse.success === false) { | ||
| logger.error( | ||
| `Failed to send mobile verification email to user (${email}) with id ${user._id}` | ||
| ); | ||
| return emailResponse; | ||
| } | ||
|
|
||
| const userDetails = { | ||
| firstName: user.firstName, | ||
| lastName: user.lastName, | ||
| email: user.email, | ||
| verified: user.verified, | ||
| }; | ||
| return { | ||
| success: true, | ||
| message: "Verification code sent to your email.", | ||
| data: userDetails, | ||
| }; | ||
| } | ||
| } catch (error) { | ||
| logObject("error in mobileVerificationReminder", error); | ||
|
|
||
| logger.error(`Error sending verification reminder: ${error.message}`); | ||
| next( | ||
| new HttpError( | ||
| "Internal Server Error", | ||
| httpStatus.INTERNAL_SERVER_ERROR, | ||
| { message: error.message } | ||
| ) | ||
| ); | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance security in mobileVerificationReminder function.
The function has good error handling but could benefit from rate limiting and token expiration:
Apply this diff to add token expiration:
const tokenCreationBody = {
token,
name: user.firstName,
+ expiresAt: new Date(Date.now() + 3600000), // 1 hour expiration
};Additionally, consider implementing rate limiting to prevent abuse of the verification system.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mobileVerificationReminder: async (request, next) => { | |
| try { | |
| const { tenant, email } = request; | |
| const user = await UserModel(tenant) | |
| .findOne({ email }) | |
| .select("_id email firstName lastName verified") | |
| .lean(); | |
| if (isEmpty(user)) { | |
| next( | |
| new HttpError("Bad Request Error", httpStatus.BAD_REQUEST, { | |
| message: "User not provided or does not exist", | |
| }) | |
| ); | |
| } | |
| const token = generateNumericToken(5); | |
| const tokenCreationBody = { | |
| token, | |
| name: user.firstName, | |
| }; | |
| const responseFromCreateToken = await VerifyTokenModel( | |
| tenant.toLowerCase() | |
| ).register(tokenCreationBody, next); | |
| if (responseFromCreateToken.success === false) { | |
| return responseFromCreateToken; | |
| } else { | |
| const emailResponse = await mailer.sendVerificationEmail( | |
| { email, token, tenant }, | |
| next | |
| ); | |
| logObject("emailResponse", emailResponse); | |
| if (emailResponse.success === false) { | |
| logger.error( | |
| `Failed to send mobile verification email to user (${email}) with id ${user._id}` | |
| ); | |
| return emailResponse; | |
| } | |
| const userDetails = { | |
| firstName: user.firstName, | |
| lastName: user.lastName, | |
| email: user.email, | |
| verified: user.verified, | |
| }; | |
| return { | |
| success: true, | |
| message: "Verification code sent to your email.", | |
| data: userDetails, | |
| }; | |
| } | |
| } catch (error) { | |
| logObject("error in mobileVerificationReminder", error); | |
| logger.error(`Error sending verification reminder: ${error.message}`); | |
| next( | |
| new HttpError( | |
| "Internal Server Error", | |
| httpStatus.INTERNAL_SERVER_ERROR, | |
| { message: error.message } | |
| ) | |
| ); | |
| } | |
| }, | |
| mobileVerificationReminder: async (request, next) => { | |
| try { | |
| const { tenant, email } = request; | |
| const user = await UserModel(tenant) | |
| .findOne({ email }) | |
| .select("_id email firstName lastName verified") | |
| .lean(); | |
| if (isEmpty(user)) { | |
| next( | |
| new HttpError("Bad Request Error", httpStatus.BAD_REQUEST, { | |
| message: "User not provided or does not exist", | |
| }) | |
| ); | |
| } | |
| const token = generateNumericToken(5); | |
| const tokenCreationBody = { | |
| token, | |
| name: user.firstName, | |
| expiresAt: new Date(Date.now() + 3600000), // 1 hour expiration | |
| }; | |
| const responseFromCreateToken = await VerifyTokenModel( | |
| tenant.toLowerCase() | |
| ).register(tokenCreationBody, next); | |
| if (responseFromCreateToken.success === false) { | |
| return responseFromCreateToken; | |
| } else { | |
| const emailResponse = await mailer.sendVerificationEmail( | |
| { email, token, tenant }, | |
| next | |
| ); | |
| logObject("emailResponse", emailResponse); | |
| if (emailResponse.success === false) { | |
| logger.error( | |
| `Failed to send mobile verification email to user (${email}) with id ${user._id}` | |
| ); | |
| return emailResponse; | |
| } | |
| const userDetails = { | |
| firstName: user.firstName, | |
| lastName: user.lastName, | |
| email: user.email, | |
| verified: user.verified, | |
| }; | |
| return { | |
| success: true, | |
| message: "Verification code sent to your email.", | |
| data: userDetails, | |
| }; | |
| } | |
| } catch (error) { | |
| logObject("error in mobileVerificationReminder", error); | |
| logger.error(`Error sending verification reminder: ${error.message}`); | |
| next( | |
| new HttpError( | |
| "Internal Server Error", | |
| httpStatus.INTERNAL_SERVER_ERROR, | |
| { message: error.message } | |
| ) | |
| ); | |
| } | |
| }, |
| resetPassword: async ({ token, password, tenant }, next) => { | ||
| try { | ||
| const resetPasswordToken = token; | ||
| const timeZone = moment.tz.guess(); | ||
| const filter = { | ||
| resetPasswordToken, | ||
| resetPasswordExpires: { | ||
| $gt: moment().tz(timeZone).toDate(), | ||
| }, | ||
| }; | ||
|
|
||
| const user = await UserModel(tenant).findOne(filter); | ||
| if (!user) { | ||
| throw new HttpError( | ||
| "Password reset token is invalid or has expired.", | ||
| httpStatus.BAD_REQUEST | ||
| ); | ||
| } | ||
| const update = { | ||
| resetPasswordToken: null, | ||
| resetPasswordExpires: null, | ||
| password, | ||
| }; | ||
|
|
||
| const responseFromModifyUser = await UserModel(tenant) | ||
| .findOneAndUpdate({ _id: ObjectId(user._id) }, update, { new: true }) | ||
| .select("firstName lastName email"); | ||
|
|
||
| const { email, firstName, lastName } = responseFromModifyUser._doc; | ||
|
|
||
| const responseFromSendEmail = await mailer.updateForgottenPassword( | ||
| { | ||
| email, | ||
| firstName, | ||
| lastName, | ||
| }, | ||
| next | ||
| ); | ||
|
|
||
| logObject("responseFromSendEmail", responseFromSendEmail); | ||
|
|
||
| if (responseFromSendEmail.success === true) { | ||
| return { | ||
| success: true, | ||
| message: "Password reset successful", | ||
| }; | ||
| } else if (responseFromSendEmail.success === false) { | ||
| return responseFromSendEmail; | ||
| } | ||
| } catch (error) { | ||
| logObject("error", error); | ||
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | ||
| next( | ||
| new HttpError( | ||
| "Internal Server Error", | ||
| httpStatus.INTERNAL_SERVER_ERROR, | ||
| { message: error.message } | ||
| ) | ||
| ); | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add password complexity validation in resetPassword function.
While the function handles token validation well, it should validate password complexity before updating.
Apply this diff to add password validation:
resetPassword: async ({ token, password, tenant }, next) => {
try {
+ const passwordRegex = /^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[@$!%*?&])[A-Za-z\d@$!%*?&]{8,}$/;
+ if (!passwordRegex.test(password)) {
+ throw new HttpError(
+ "Password must be at least 8 characters long and contain uppercase, lowercase, number and special character",
+ httpStatus.BAD_REQUEST
+ );
+ }
+
const resetPasswordToken = token;
const timeZone = moment.tz.guess();
// ... rest of the function📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resetPassword: async ({ token, password, tenant }, next) => { | |
| try { | |
| const resetPasswordToken = token; | |
| const timeZone = moment.tz.guess(); | |
| const filter = { | |
| resetPasswordToken, | |
| resetPasswordExpires: { | |
| $gt: moment().tz(timeZone).toDate(), | |
| }, | |
| }; | |
| const user = await UserModel(tenant).findOne(filter); | |
| if (!user) { | |
| throw new HttpError( | |
| "Password reset token is invalid or has expired.", | |
| httpStatus.BAD_REQUEST | |
| ); | |
| } | |
| const update = { | |
| resetPasswordToken: null, | |
| resetPasswordExpires: null, | |
| password, | |
| }; | |
| const responseFromModifyUser = await UserModel(tenant) | |
| .findOneAndUpdate({ _id: ObjectId(user._id) }, update, { new: true }) | |
| .select("firstName lastName email"); | |
| const { email, firstName, lastName } = responseFromModifyUser._doc; | |
| const responseFromSendEmail = await mailer.updateForgottenPassword( | |
| { | |
| email, | |
| firstName, | |
| lastName, | |
| }, | |
| next | |
| ); | |
| logObject("responseFromSendEmail", responseFromSendEmail); | |
| if (responseFromSendEmail.success === true) { | |
| return { | |
| success: true, | |
| message: "Password reset successful", | |
| }; | |
| } else if (responseFromSendEmail.success === false) { | |
| return responseFromSendEmail; | |
| } | |
| } catch (error) { | |
| logObject("error", error); | |
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | |
| next( | |
| new HttpError( | |
| "Internal Server Error", | |
| httpStatus.INTERNAL_SERVER_ERROR, | |
| { message: error.message } | |
| ) | |
| ); | |
| } | |
| }, | |
| resetPassword: async ({ token, password, tenant }, next) => { | |
| try { | |
| const passwordRegex = /^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[@$!%*?&])[A-Za-z\d@$!%*?&]{8,}$/; | |
| if (!passwordRegex.test(password)) { | |
| throw new HttpError( | |
| "Password must be at least 8 characters long and contain uppercase, lowercase, number and special character", | |
| httpStatus.BAD_REQUEST | |
| ); | |
| } | |
| const resetPasswordToken = token; | |
| const timeZone = moment.tz.guess(); | |
| const filter = { | |
| resetPasswordToken, | |
| resetPasswordExpires: { | |
| $gt: moment().tz(timeZone).toDate(), | |
| }, | |
| }; | |
| const user = await UserModel(tenant).findOne(filter); | |
| if (!user) { | |
| throw new HttpError( | |
| "Password reset token is invalid or has expired.", | |
| httpStatus.BAD_REQUEST | |
| ); | |
| } | |
| const update = { | |
| resetPasswordToken: null, | |
| resetPasswordExpires: null, | |
| password, | |
| }; | |
| const responseFromModifyUser = await UserModel(tenant) | |
| .findOneAndUpdate({ _id: ObjectId(user._id) }, update, { new: true }) | |
| .select("firstName lastName email"); | |
| const { email, firstName, lastName } = responseFromModifyUser._doc; | |
| const responseFromSendEmail = await mailer.updateForgottenPassword( | |
| { | |
| email, | |
| firstName, | |
| lastName, | |
| }, | |
| next | |
| ); | |
| logObject("responseFromSendEmail", responseFromSendEmail); | |
| if (responseFromSendEmail.success === true) { | |
| return { | |
| success: true, | |
| message: "Password reset successful", | |
| }; | |
| } else if (responseFromSendEmail.success === false) { | |
| return responseFromSendEmail; | |
| } | |
| } catch (error) { | |
| logObject("error", error); | |
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | |
| next( | |
| new HttpError( | |
| "Internal Server Error", | |
| httpStatus.INTERNAL_SERVER_ERROR, | |
| { message: error.message } | |
| ) | |
| ); | |
| } | |
| }, |
|
Auth-service changes in this PR available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/auth-service/utils/user.util.js (2)
2477-2537: 🛠️ Refactor suggestionAdd password validation and improve ObjectId handling.
The current implementation needs:
- Password complexity validation
- Safe ObjectId conversion
The password complexity validation was already suggested in a previous review. Additionally, apply this diff to improve ObjectId handling:
const responseFromModifyUser = await UserModel(tenant) - .findOneAndUpdate({ _id: ObjectId(user._id) }, update, { new: true }) + .findOneAndUpdate( + { _id: mongoose.Types.ObjectId.isValid(user._id) ? ObjectId(user._id) : null }, + update, + { new: true } + ) .select("firstName lastName email");
1678-1744: 🛠️ Refactor suggestionAdd token expiry and rate limiting for verification reminders.
The current implementation needs:
- Token expiry time
- Rate limiting for verification reminders
- Cleanup of expired tokens
The token expiry suggestion was already made in a previous review. Additionally, apply this diff to add rate limiting:
mobileVerificationReminder: async (request, next) => { try { const { tenant, email } = request; + + // Check if user has requested too many reminders + const lastHourReminders = await VerifyTokenModel(tenant) + .countDocuments({ + 'created_by.email': email, + createdAt: { $gte: new Date(Date.now() - 3600000) } + }); + + if (lastHourReminders >= 3) { + throw new HttpError( + 'Too many verification attempts', + httpStatus.TOO_MANY_REQUESTS, + { message: 'Please wait before requesting another verification code.' } + ); + } const user = await UserModel(tenant)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/auth-service/utils/user.util.js(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build-push-deploy-auth-service
- GitHub Check: Code Coverage for device-registry
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript)
| registerMobileUser: async (request, next) => { | ||
| try { | ||
| const { tenant } = { | ||
| ...request.body, | ||
| ...request.query, | ||
| ...request.params, | ||
| }; | ||
|
|
||
| const userData = request.body; | ||
| const verificationToken = generateNumericToken(5); | ||
|
|
||
| const newUserResponse = await UserModel(tenant).register(userData, next); | ||
|
|
||
| if (newUserResponse.success === true) { | ||
| const newUser = newUserResponse.data; | ||
|
|
||
| // Add this block to store the token in the VerifyToken collection: | ||
| const tokenExpiry = 86400; //24hrs in seconds. Feel free to use any value | ||
|
|
||
| const tokenCreationBody = { | ||
| token: verificationToken, | ||
| name: newUser.firstName, | ||
| expires: new Date(Date.now() + tokenExpiry * 1000), // Set token expiry | ||
| }; | ||
|
|
||
| const verifyTokenResponse = await VerifyTokenModel( | ||
| tenant.toLowerCase() | ||
| ).register(tokenCreationBody, next); | ||
|
|
||
| if (verifyTokenResponse.success === false) { | ||
| // Consider rolling back user creation | ||
| logger.error( | ||
| `Failed to create verification token for user ${newUser.email}: ${verifyTokenResponse.message}` | ||
| ); | ||
|
|
||
| return verifyTokenResponse; | ||
| } | ||
|
|
||
| await mailer.sendVerificationEmail({ | ||
| email: userData.email, | ||
| token: verificationToken, | ||
| }); | ||
|
|
||
| return { | ||
| success: true, | ||
| message: "User registered successfully. Please verify your email.", | ||
| user: newUser, | ||
| }; | ||
| } else { | ||
| return newUserResponse; | ||
| } | ||
| } catch (error) { | ||
| logObject("error in reg", error); | ||
| return { success: false, message: error.message }; | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement transaction handling and improve error handling.
The function needs:
- Transaction handling for atomic operations
- Proper rollback on token creation failure
- More detailed error handling
Apply this diff to improve the implementation:
registerMobileUser: async (request, next) => {
try {
+ const session = await mongoose.startSession();
+ session.startTransaction();
+
const { tenant } = {
...request.body,
...request.query,
...request.params,
};
const userData = request.body;
const verificationToken = generateNumericToken(5);
- const newUserResponse = await UserModel(tenant).register(userData, next);
+ const newUserResponse = await UserModel(tenant).register(userData, next, session);
if (newUserResponse.success === true) {
const newUser = newUserResponse.data;
const tokenCreationBody = {
token: verificationToken,
name: newUser.firstName,
expires: new Date(Date.now() + tokenExpiry * 1000),
};
const verifyTokenResponse = await VerifyTokenModel(
tenant.toLowerCase()
- ).register(tokenCreationBody, next);
+ ).register(tokenCreationBody, next, session);
if (verifyTokenResponse.success === false) {
+ await session.abortTransaction();
+ session.endSession();
logger.error(
`Failed to create verification token for user ${newUser.email}: ${verifyTokenResponse.message}`
);
return verifyTokenResponse;
}
await mailer.sendVerificationEmail({
email: userData.email,
token: verificationToken,
});
+ await session.commitTransaction();
+ session.endSession();
return {
success: true,
message: "User registered successfully. Please verify your email.",
user: newUser,
};
} else {
+ await session.abortTransaction();
+ session.endSession();
return newUserResponse;
}
} catch (error) {
+ if (session) {
+ await session.abortTransaction();
+ session.endSession();
+ }
logObject("error in reg", error);
- return { success: false, message: error.message };
+ return {
+ success: false,
+ message: "Registration failed",
+ errors: {
+ message: error.message,
+ code: error.code,
+ type: error.name
+ }
+ };
}
},Committable suggestion skipped: line range outside the PR's diff.
| initiatePasswordReset: async ({ email, token, tenant }, next) => { | ||
| try { | ||
| const update = { | ||
| resetPasswordToken: token, | ||
| resetPasswordExpires: Date.now() + 3600000, | ||
| }; | ||
| const responseFromModifyUser = await UserModel(tenant) | ||
| .findOneAndUpdate({ email }, update, { new: true }) | ||
| .select("firstName lastName email"); | ||
|
|
||
| if (isEmpty(responseFromModifyUser)) { | ||
| next( | ||
| new HttpError("Bad Request Error", httpStatus.INTERNAL_SERVER_ERROR, { | ||
| message: "user does not exist, please crosscheck", | ||
| }) | ||
| ); | ||
| } | ||
|
|
||
| await mailer.sendPasswordResetEmail({ email, token, tenant }); | ||
|
|
||
| return { | ||
| success: true, | ||
| message: "Password reset email sent successfully", | ||
| }; | ||
| } catch (error) { | ||
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | ||
| next( | ||
| new HttpError( | ||
| "Unable to initiate password reset", | ||
| httpStatus.INTERNAL_SERVER_ERROR, | ||
| { message: error.message } | ||
| ) | ||
| ); | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance security measures in password reset initiation.
The implementation needs additional security measures to prevent abuse:
- Add rate limiting for password reset requests
- Validate email format before processing
- Consider making token expiry configurable
Apply this diff to improve the implementation:
initiatePasswordReset: async ({ email, token, tenant }, next) => {
try {
+ if (!email || !email.match(/^[^\s@]+@[^\s@]+\.[^\s@]+$/)) {
+ throw new HttpError('Invalid email format', httpStatus.BAD_REQUEST);
+ }
+
const update = {
resetPasswordToken: token,
- resetPasswordExpires: Date.now() + 3600000,
+ resetPasswordExpires: Date.now() + (process.env.PASSWORD_RESET_EXPIRY || 3600000),
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| initiatePasswordReset: async ({ email, token, tenant }, next) => { | |
| try { | |
| const update = { | |
| resetPasswordToken: token, | |
| resetPasswordExpires: Date.now() + 3600000, | |
| }; | |
| const responseFromModifyUser = await UserModel(tenant) | |
| .findOneAndUpdate({ email }, update, { new: true }) | |
| .select("firstName lastName email"); | |
| if (isEmpty(responseFromModifyUser)) { | |
| next( | |
| new HttpError("Bad Request Error", httpStatus.INTERNAL_SERVER_ERROR, { | |
| message: "user does not exist, please crosscheck", | |
| }) | |
| ); | |
| } | |
| await mailer.sendPasswordResetEmail({ email, token, tenant }); | |
| return { | |
| success: true, | |
| message: "Password reset email sent successfully", | |
| }; | |
| } catch (error) { | |
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | |
| next( | |
| new HttpError( | |
| "Unable to initiate password reset", | |
| httpStatus.INTERNAL_SERVER_ERROR, | |
| { message: error.message } | |
| ) | |
| ); | |
| } | |
| }, | |
| initiatePasswordReset: async ({ email, token, tenant }, next) => { | |
| try { | |
| if (!email || !email.match(/^[^\s@]+@[^\s@]+\.[^\s@]+$/)) { | |
| throw new HttpError('Invalid email format', httpStatus.BAD_REQUEST); | |
| } | |
| const update = { | |
| resetPasswordToken: token, | |
| resetPasswordExpires: Date.now() + (process.env.PASSWORD_RESET_EXPIRY || 3600000), | |
| }; | |
| const responseFromModifyUser = await UserModel(tenant) | |
| .findOneAndUpdate({ email }, update, { new: true }) | |
| .select("firstName lastName email"); | |
| if (isEmpty(responseFromModifyUser)) { | |
| next( | |
| new HttpError("Bad Request Error", httpStatus.INTERNAL_SERVER_ERROR, { | |
| message: "user does not exist, please crosscheck", | |
| }) | |
| ); | |
| } | |
| await mailer.sendPasswordResetEmail({ email, token, tenant }); | |
| return { | |
| success: true, | |
| message: "Password reset email sent successfully", | |
| }; | |
| } catch (error) { | |
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | |
| next( | |
| new HttpError( | |
| "Unable to initiate password reset", | |
| httpStatus.INTERNAL_SERVER_ERROR, | |
| { message: error.message } | |
| ) | |
| ); | |
| } | |
| }, |
Description
new password reset process for the mobile app
Changes Made
Testing
Affected Services
Endpoints Ready for Testing
API Documentation Updated?
Summary by CodeRabbit
Release Notes: Password Reset Functionality
New Features
Improvements
Security