Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new password reset process for the mobile app #4294

Merged
merged 3 commits into from
Jan 27, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 79 additions & 0 deletions src/auth-service/controllers/user.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -1664,6 +1664,85 @@ const createUser = {
return;
}
},
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;
}
},
Comment on lines +1667 to +1702
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 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

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;

const result = await userUtil.resetPassword(
{
token,
password,
tenant,
},
next
);

res
.status(httpStatus.OK)
.json({ success: true, message: result.message });
} catch (error) {
logObject("error in controller", error);
logger.error(`🐛🐛 Internal Server Error ${error.message}`);
next(
new HttpError(
"Internal Server Error",
httpStatus.INTERNAL_SERVER_ERROR,
{ message: error.message }
)
);
return;
}
},

subscribeToNewsLetter: async (req, res, next) => {
try {
const errors = extractErrorsFromRequest(req);
Expand Down
12 changes: 12 additions & 0 deletions src/auth-service/routes/v2/users.routes.js
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,18 @@ router.post(
createUserController.forgot
);

router.post(
"/reset-password-request",
userValidations.resetPasswordRequest,
createUserController.resetPasswordRequest
);

router.post(
"/reset-password/:token",
userValidations.resetPassword,
createUserController.resetPassword
);

router.put("/", userValidations.updateUser, createUserController.update);

router.put(
Expand Down
13 changes: 13 additions & 0 deletions src/auth-service/utils/common/email.msgs.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,19 @@ module.exports = {
</tr>`;
return constants.EMAIL_BODY({ email, content });
},
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 });
},
Comment on lines +44 to +56
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 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

joinRequest: (firstName, lastName, email) => {
const name = firstName + " " + lastName;
const content = ` <tr>
Expand Down
49 changes: 49 additions & 0 deletions src/auth-service/utils/common/mailer.js
Original file line number Diff line number Diff line change
Expand Up @@ -1311,6 +1311,55 @@ const mailer = {
);
}
},

sendPasswordResetEmail: async ({ email, token, tenant = "airqo" }, next) => {
try {
const checkResult = await SubscriptionModel(
tenant
).checkNotificationStatus({ email, type: "email" });
if (!checkResult.success) {
return checkResult;
}

const mailOptions = {
from: {
name: constants.EMAIL_NAME,
address: constants.EMAIL,
},
to: email,
subject: `Password Reset Code: ${token}`,
html: msgs.mobilePasswordReset({ token, email }),
attachments: attachments,
};

let response = transporter.sendMail(mailOptions);
let data = await response;

if (isEmpty(data.rejected) && !isEmpty(data.accepted)) {
return {
success: true,
message: "Email sent successfully",
data,
status: httpStatus.OK,
};
} else {
next(
new HttpError("Email not sent", httpStatus.INTERNAL_SERVER_ERROR, {
emailResults: data,
})
);
}
} catch (error) {
logger.error(`Error sending password reset email: ${error.message}`);
next(
new HttpError(
"Internal Server Error",
httpStatus.INTERNAL_SERVER_ERROR,
{ message: error.message }
)
);
}
},
signInWithEmailLink: async (
{ email, token, tenant = "airqo" } = {},
next
Expand Down
104 changes: 104 additions & 0 deletions src/auth-service/utils/user.util.js
Original file line number Diff line number Diff line change
Expand Up @@ -2041,6 +2041,110 @@ const createUserModule = {
);
}
},
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;
},
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

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 }
)
);
}
},
Comment on lines +2442 to +2476
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance security measures in password reset initiation.

The implementation needs additional security measures to prevent abuse:

  1. Add rate limiting for password reset requests
  2. Validate email format before processing
  3. 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.

Suggested change
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 }
)
);
}
},


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 }
)
);
}
},
Comment on lines +2477 to +2537
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
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 }
)
);
}
},

generateResetToken: (next) => {
try {
const token = crypto.randomBytes(20).toString("hex");
Expand Down
42 changes: 42 additions & 0 deletions src/auth-service/validators/users.validators.js
Original file line number Diff line number Diff line change
Expand Up @@ -842,6 +842,46 @@ const getUser = [
],
];

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
];
Comment on lines +874 to +882
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ 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(),
];
Comment on lines +884 to +912
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance password security requirements.

The current password validation could be strengthened by:

  1. Adding maximum length limit
  2. Requiring complexity (uppercase, lowercase, special characters)
  3. 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.

Suggested change
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(),
];


module.exports = {
tenant: validateTenant,
AirqoTenantOnly: validateAirqoTenantOnly,
Expand Down Expand Up @@ -876,4 +916,6 @@ module.exports = {
unSubscribeFromNotifications,
notificationStatus,
getUser,
resetPasswordRequest,
resetPassword,
};
Loading