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.

Sign up for GitHub

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

Minor refactoring in DB modules for readability #1111

Merged
merged 1 commit into from
Nov 5, 2024
Merged

Minor refactoring in DB modules for readability #1111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 12 additions & 12 deletions Server/db/mongo/modules/monitorModule.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import Monitor from "../../models/Monitor.js";
import Check from "../../models/Check.js";
import PageSpeedCheck from "../../models/PageSpeedCheck.js";
import HardwareCheck from "../../models/HardwareCheck.js";
import { errorMessages } from "../../../utils/messages.js";
import Notification from "../../models/Notification.js";
import { NormalizeData } from "../../../utils/dataUtils.js";
Expand All @@ -16,6 +17,13 @@ const demoMonitors = JSON.parse(fs.readFileSync(demoMonitorsPath, "utf8"));

const SERVICE_NAME = "monitorModule";

const CHECK_MODEL_LOOKUP = {
http: Check,
ping: Check,
pageSpeed: PageSpeedCheck,
hardware: HardwareCheck,
};

/**
* Get all monitors
* @async
Expand Down Expand Up @@ -163,8 +171,7 @@ const getMonitorStatsById = async (req) => {
// Default sort order is newest -> oldest
sortOrder = sortOrder === "asc" ? 1 : -1;

let model =
monitor.type === "http" || monitor.type === "ping" ? Check : PageSpeedCheck;
let model = CHECK_MODEL_LOOKUP[monitor.type];

const monitorStats = {
...monitor.toObject(),
Expand Down Expand Up @@ -325,13 +332,9 @@ const getMonitorsAndSummaryByTeamId = async (teamId, type) => {
(acc, monitor) => {
if (monitor.status === true) {
acc.up += 1;
}

if (monitor.status === false) {
} else if (monitor.status === false) {
acc.down += 1;
}

if (monitor.isActive === false) {
} else if (monitor.isActive === false) {
acc.paused += 1;
}
return acc;
Expand Down Expand Up @@ -397,8 +400,6 @@ const getMonitorsByTeamId = async (req, res) => {
// Default sort order is newest -> oldest
if (checkOrder === "asc") {
checkOrder = 1;
} else if (checkOrder === "desc") {
checkOrder = -1;
} else checkOrder = -1;

// Sort order for monitors
Expand Down Expand Up @@ -428,8 +429,7 @@ const getMonitorsByTeamId = async (req, res) => {
checksQuery.status = status;
}

let model =
monitor.type === "http" || monitor.type === "ping" ? Check : PageSpeedCheck;
let model = CHECK_MODEL_LOOKUP[monitor.type];

// Checks are order newest -> oldest
let checks = await model
Expand Down
28 changes: 14 additions & 14 deletions Server/db/mongo/modules/recoveryModule.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,25 +56,25 @@ const resetPassword = async (req, res) => {
const recoveryToken = await validateRecoveryToken(req, res);
const user = await UserModel.findOne({ email: recoveryToken.email });

if (user === null) {
throw new Error(errorMessages.DB_USER_NOT_FOUND);
}

const match = await user.comparePassword(newPassword);
if (match === true) {
throw new Error(errorMessages.DB_RESET_PASSWORD_BAD_MATCH);
}

if (user !== null) {
user.password = newPassword;
await user.save();
await RecoveryToken.deleteMany({ email: recoveryToken.email });
// Fetch the user again without the password
const userWithoutPassword = await UserModel.findOne({
email: recoveryToken.email,
})
.select("-password")
.select("-profileImage");
return userWithoutPassword;
} else {
throw new Error(errorMessages.DB_USER_NOT_FOUND);
}
user.password = newPassword;
await user.save();
await RecoveryToken.deleteMany({ email: recoveryToken.email });
// Fetch the user again without the password
const userWithoutPassword = await UserModel.findOne({
email: recoveryToken.email,
})
.select("-password")
.select("-profileImage");
return userWithoutPassword;
Comment on lines +68 to +77
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Yo dawg, we need to wrap these operations in a transaction! 💪

The password update and token cleanup should be atomic operations to prevent inconsistent states. Also, that second user fetch is making me nervous like mom's spaghetti.

Consider these improvements:

- user.password = newPassword;
- await user.save();
- await RecoveryToken.deleteMany({ email: recoveryToken.email });
- // Fetch the user again without the password
- const userWithoutPassword = await UserModel.findOne({
-   email: recoveryToken.email,
- })
-   .select("-password")
-   .select("-profileImage");
- return userWithoutPassword;
+ const session = await UserModel.startSession();
+ try {
+   await session.withTransaction(async () => {
+     user.password = newPassword;
+     await user.save({ session });
+     await RecoveryToken.deleteMany(
+       { email: recoveryToken.email },
+       { session }
+     );
+   });
+ } finally {
+   await session.endSession();
+ }
+ // Return the user object we already have, just remove sensitive fields
+ user.password = undefined;
+ user.profileImage = undefined;
+ return user;

This change:

  1. Makes operations atomic with MongoDB transactions
  2. Optimizes by reusing the existing user object instead of an extra DB query
  3. Properly handles session cleanup
📝 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
user.password = newPassword;
await user.save();
await RecoveryToken.deleteMany({ email: recoveryToken.email });
// Fetch the user again without the password
const userWithoutPassword = await UserModel.findOne({
email: recoveryToken.email,
})
.select("-password")
.select("-profileImage");
return userWithoutPassword;
const session = await UserModel.startSession();
try {
await session.withTransaction(async () => {
user.password = newPassword;
await user.save({ session });
await RecoveryToken.deleteMany(
{ email: recoveryToken.email },
{ session }
);
});
} finally {
await session.endSession();
}
// Return the user object we already have, just remove sensitive fields
user.password = undefined;
user.profileImage = undefined;
return user;

} catch (error) {
error.service = SERVICE_NAME;
error.method = "resetPassword";
Expand Down
5 changes: 2 additions & 3 deletions Server/db/mongo/modules/userModule.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,10 @@ const getUserByEmail = async (email) => {
// Need the password to be able to compare, removed .select()
// We can strip the hash before returning the user
const user = await UserModel.findOne({ email: email }).select("-profileImage");
if (user) {
return user;
} else {
if (!user) {
throw new Error(errorMessages.DB_USER_NOT_FOUND);
}
return user;
Comment on lines +73 to +76
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Yo dawg, we need some rate limiting up in here! 🔒

After checking the codebase, there's no rate limiting implemented for the login endpoint. This could make the system vulnerable to brute force attacks. Here's what's missing:

  • No rate limiting middleware (like express-rate-limit) in the dependencies
  • Login route at Server/routes/authRoute.js lacks rate limiting protection
  • Authentication flow in Server/controllers/authController.js could benefit from request throttling
🔗 Analysis chain

Yo dawg, this error handling is straight fire! 🔥

The simplified error flow makes the code cleaner and more direct. It follows the fail-fast principle by checking for the error condition first.

However, since this function is used during login, we might want to consider adding some rate limiting to prevent brute force attacks.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if rate limiting is implemented elsewhere
rg -A 5 "rate.?limit|throttle"

Length of output: 129

Script:

#!/bin/bash
# Let's check for any rate limiting middleware or authentication related code
rg -A 5 "middleware.*auth|authenticate|login"

# Also check for any express-rate-limit or similar package usage
fd package.json --exec cat {} | jq -r '.dependencies,.devDependencies' 2>/dev/null

# Look for any route handlers that might implement rate limiting
ast-grep --pattern 'app.use($$$rate$$$)'

Length of output: 19745

} catch (error) {
error.service = SERVICE_NAME;
error.method = "getUserByEmail";
Expand Down