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

feat/be/status page implementation #1138

Merged
merged 13 commits into from
Nov 11, 2024
51 changes: 51 additions & 0 deletions Server/controllers/statusPageController.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import { handleError, handleValidationError } from "./controllerUtils.js";
import {
createStatusPageBodyValidation,
getStatusPageParamValidation,
} from "../validation/joi.js";
import { successMessages } from "../utils/messages.js";

const SERVICE_NAME = "statusPageController";

const createStatusPage = async (req, res, next) => {
try {
await createStatusPageBodyValidation.validateAsync(req.body);
} catch (error) {
next(handleValidationError(error, SERVICE_NAME));
return;
}

try {
const statusPage = await req.db.createStatusPage(req.body);
return res.status(200).json({
success: true,
msg: successMessages.STATUS_PAGE_CREATE,
data: statusPage,
});
} catch (error) {
next(handleError(error, SERVICE_NAME, "createStatusPage"));
}
};

const getStatusPageByUrl = async (req, res, next) => {
try {
await getStatusPageParamValidation.validateAsync(req.params);
} catch (error) {
next(handleValidationError(error, SERVICE_NAME));
return;
}

try {
const { url } = req.params;
const statusPage = await req.db.getStatusPageByUrl(url);
return res.status(200).json({
success: true,
msg: successMessages.STATUS_PAGE_BY_URL,
data: statusPage,
});
} catch (error) {
next(handleError(error, SERVICE_NAME, "getStatusPage"));
}
};

export { createStatusPage, getStatusPageByUrl };
41 changes: 41 additions & 0 deletions Server/db/models/StatusPage.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import mongoose from "mongoose";

const StatusPageSchema = mongoose.Schema(
{
companyName: {
type: String,
required: true,
default: "",
},
url: {
type: String,
required: true,
default: "",
},
timezone: {
type: String,
required: true,
default: "America/Toronto",
},
Comment on lines +16 to +20
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Timezone validation's weak, arms are heavy! 🍝

The timezone field accepts any string, which could lead to invalid timezone values being stored. We should validate against a list of valid IANA timezone identifiers.

Drop this validation like it's hot:

 timezone: {
   type: String,
   required: true,
   default: "America/Toronto",
+  validate: {
+    validator: function(v) {
+      return Intl.supportedValuesOf('timeZone').includes(v);
+    },
+    message: props => `${props.value} is not a valid timezone!`
+  }
 },
📝 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
timezone: {
type: String,
required: true,
default: "America/Toronto",
},
timezone: {
type: String,
required: true,
default: "America/Toronto",
validate: {
validator: function(v) {
return Intl.supportedValuesOf('timeZone').includes(v);
},
message: props => `${props.value} is not a valid timezone!`
}
},

color: {
type: String,
required: true,
default: "#4169E1",
},
Comment on lines +21 to +25
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Color field's got no chill! 🍝

The color field needs validation to ensure it contains valid hex color codes.

Let's make this validation cleaner than your sweater:

 color: {
   type: String,
   required: true,
   default: "#4169E1",
+  validate: {
+    validator: function(v) {
+      return /^#[0-9A-F]{6}$/i.test(v);
+    },
+    message: props => `${props.value} is not a valid hex color code!`
+  }
 },
📝 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
color: {
type: String,
required: true,
default: "#4169E1",
},
color: {
type: String,
required: true,
default: "#4169E1",
validate: {
validator: function(v) {
return /^#[0-9A-F]{6}$/i.test(v);
},
message: props => `${props.value} is not a valid hex color code!`
}
},

theme: {
type: String,
required: true,
default: "light",
},
Comment on lines +26 to +30
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Theme field's not ready for the spotlight! 🍝

The theme field should be restricted to specific valid options rather than accepting any string.

Here's an improvement that'll make your knees weak:

 theme: {
   type: String,
   required: true,
   default: "light",
+  enum: {
+    values: ["light", "dark"],
+    message: '{VALUE} is not a supported theme'
+  }
 },
📝 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
theme: {
type: String,
required: true,
default: "light",
},
theme: {
type: String,
required: true,
default: "light",
enum: {
values: ["light", "dark"],
message: '{VALUE} is not a supported theme'
}
},

Comment on lines +26 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to store the theme as well? I thought that was dealt with locally

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@marcelluscaio yes I'm not quite sure of the usage of this one either, I just followed the spec and there was a theme field in it 😛 It could be that the spec was from an earlier iteration before we had theme defined at all actually.

If it turns out we don't need this we can remove it when we're clear on its usage 👍 Thanks for being vigilant!

monitors: [
{
type: mongoose.Schema.Types.ObjectId,
ref: "Monitor",
required: true,
},
],
Comment on lines +31 to +37
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

These monitor refs need some indexes to perform! 🍝

The monitors array field should be indexed to improve query performance when looking up status pages by monitor IDs.

Add this index like you're dropping bars:

 monitors: [
   {
     type: mongoose.Schema.Types.ObjectId,
     ref: "Monitor",
     required: true,
   },
 ],
+}, {
+  timestamps: true,
+  indexes: [{ monitors: 1 }]

Committable suggestion skipped: line range outside the PR's diff.

},
{ timestamps: true }
);

export default mongoose.model("StatusPage", StatusPageSchema);
7 changes: 7 additions & 0 deletions Server/db/mongo/MongoDB.js
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,11 @@ import {
//****************************************
import { getAppSettings, updateAppSettings } from "./modules/settingsModule.js";

//****************************************
// Status Page
//****************************************
import { createStatusPage, getStatusPageByUrl } from "./modules/statusPageModule.js";

export default {
connect,
disconnect,
Expand Down Expand Up @@ -223,4 +228,6 @@ export default {
deleteNotificationsByMonitorId,
getAppSettings,
updateAppSettings,
createStatusPage,
getStatusPageByUrl,
};
47 changes: 47 additions & 0 deletions Server/db/mongo/modules/statusPageModule.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import StatusPage from "../../models/StatusPage.js";
import { errorMessages } from "../../../utils/messages.js";

const SERVICE_NAME = "statusPageModule";

const createStatusPage = async (statusPageData) => {
try {
const isUnique = await urlIsUnique(statusPageData.url);
if (!isUnique) {
const error = new Error(errorMessages.STATUS_PAGE_URL_NOT_UNIQUE);
error.status = 400;
throw error;
}
const statusPage = new StatusPage({ ...statusPageData });
await statusPage.save();
return statusPage;
} catch (error) {
error.service = SERVICE_NAME;
error.method = "createStatusPage";
throw error;
}
};

const getStatusPageByUrl = async (url) => {
try {
const statusPage = await StatusPage.findOne({ url });
if (statusPage === null || statusPage === undefined) {
const error = new Error(errorMessages.STATUS_PAGE_NOT_FOUND);
error.status = 404;

throw error;
}
return statusPage;
} catch (error) {
error.service = SERVICE_NAME;
error.method = "getStatusPageByUrl";
throw error;
}
};

const urlIsUnique = async (url) => {
const statusPage = await StatusPage.find({ url });
if (statusPage.length > 0) return false;
return true;
};

export { createStatusPage, getStatusPageByUrl };
2 changes: 2 additions & 0 deletions Server/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import monitorRouter from "./routes/monitorRoute.js";
import checkRouter from "./routes/checkRoute.js";
import maintenanceWindowRouter from "./routes/maintenanceWindowRoute.js";
import settingsRouter from "./routes/settingsRoute.js";
import statusPageRouter from "./routes/statusPageRoute.js";
import { fileURLToPath } from "url";

import queueRouter from "./routes/queueRoute.js";
Expand Down Expand Up @@ -88,6 +89,7 @@ const startApp = async () => {
app.use("/api/v1/checks", verifyJWT, checkRouter);
app.use("/api/v1/maintenance-window", verifyJWT, maintenanceWindowRouter);
app.use("/api/v1/queue", verifyJWT, queueRouter);
app.use("/api/v1/status-page", statusPageRouter);

//health check
app.use("/api/v1/healthy", (req, res) => {
Expand Down
11 changes: 11 additions & 0 deletions Server/routes/statusPageRoute.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import express from "express";
import { verifyJWT } from "../middleware/verifyJWT.js";
import {
createStatusPage,
getStatusPageByUrl,
} from "../controllers/statusPageController.js";
const router = express.Router();

router.get("/:url", getStatusPageByUrl);
router.post("/:url", verifyJWT, createStatusPage);
Dismissed Show dismissed Hide dismissed
Comment on lines +9 to +10
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

Yo! There's no rate limiting on these routes, and that's a fact! 🍝

Looking through the server setup like mom's recipe book:

  • No express-rate-limit in package.json dependencies
  • Main server setup (index.js) only uses basic middleware (cors, helmet)
  • No rate limiting middleware configured anywhere

You better lose yourself in implementing rate limiting, the moment you own it:

// First, add to package.json dependencies:
// "express-rate-limit": "^7.1.5"

import rateLimit from 'express-rate-limit';

const apiLimiter = rateLimit({
  windowMs: 15 * 60 * 1000, // 15 minutes
  max: 100 // limit each IP to 100 requests per windowMs
});

router.get("/:url", apiLimiter, getStatusPageByUrl);
router.post("/:url", apiLimiter, verifyJWT, createStatusPage);
🔗 Analysis chain

Knees weak, arms heavy - these routes need rate limiting already! 🏋️

While the authentication is solid with JWT, there are some security considerations to address:

  1. Add rate limiting to prevent abuse
  2. Consider adding URL parameter validation middleware

Let's check if rate limiting is implemented elsewhere:

Here's a suggested implementation:

import rateLimit from 'express-rate-limit';

const apiLimiter = rateLimit({
  windowMs: 15 * 60 * 1000, // 15 minutes
  max: 100 // limit each IP to 100 requests per windowMs
});

router.get("/:url", apiLimiter, getStatusPageByUrl);
router.post("/:url", apiLimiter, verifyJWT, createStatusPage);

Let me check if there's any rate limiting setup in the server configuration before jumping to conclusions. The previous commands didn't find the package.json, so let's also verify its existence and location.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for rate limiting implementations
echo "Checking for rate limiting packages..."
grep -r "express-rate-limit\|rate-limit" package.json || echo "No rate limiting package found"

echo "Checking for rate limiting middleware..."
rg -l "rateLimit|rateLimiter" --type js

Length of output: 392


Script:

#!/bin/bash
# Let's check for any middleware configurations and rate limiting in the server setup
echo "Checking server setup files..."
fd -e js -e ts server config app index main | xargs rg -l "middleware|express\(\)|app\."

echo "Checking for rate limit related imports..."
rg "import.*rate|require.*rate" --type js

# Check the actual content of the main server file
echo "Content of main server setup:"
fd -e js -e ts server config app index main | xargs cat

Length of output: 1677


Script:

#!/bin/bash
# Find package.json files
echo "Looking for package.json files..."
fd package.json

Length of output: 134


Script:

#!/bin/bash
# Let's check the Server's package.json for rate limiting dependencies
echo "Checking Server's package.json content..."
cat Server/package.json

# Let's also check the main server setup file
echo "Checking Server's index.js content..."
cat Server/index.js

# And let's check if there's any middleware folder with rate limiting
echo "Checking for middleware directory..."
fd middleware Server

Length of output: 6447

🧰 Tools
🪛 GitHub Check: CodeQL

[failure] 10-10: Missing rate limiting
This route handler performs authorization, but is not rate-limited.

export default router;
8 changes: 8 additions & 0 deletions Server/utils/messages.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ const errorMessages = {

// PING Operations
PING_CANNOT_RESOLVE: "No response",

// Status Page Errors
STATUS_PAGE_NOT_FOUND: "Status page not found",
STATUS_PAGE_URL_NOT_UNIQUE: "Status page url must be unique",
};

const successMessages = {
Expand Down Expand Up @@ -115,6 +119,10 @@ const successMessages = {
// App Settings
GET_APP_SETTINGS: "Got app settings successfully",
UPDATE_APP_SETTINGS: "Updated app settings successfully",

// Status Page
STATUS_PAGE_BY_URL: "Got status page by url successfully",
STATUS_PAGE_CREATE: "Status page created successfully",
Comment on lines +122 to +125
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, we're missing some success messages for common operations!

While the current success messages are on point, we should add messages for other standard CRUD operations to maintain consistency.

Add these success messages to complete the set:

 // Status Page
 STATUS_PAGE_BY_URL: "Got status page by url successfully",
 STATUS_PAGE_CREATE: "Status page created successfully",
+STATUS_PAGE_UPDATE: "Status page updated successfully",
+STATUS_PAGE_DELETE: "Status page deleted successfully",
+STATUS_PAGE_GET_ALL: "Got all status pages successfully",
📝 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
// Status Page
STATUS_PAGE_BY_URL: "Got status page by url successfully",
STATUS_PAGE_CREATE: "Status page created successfully",
// Status Page
STATUS_PAGE_BY_URL: "Got status page by url successfully",
STATUS_PAGE_CREATE: "Status page created successfully",
STATUS_PAGE_UPDATE: "Status page updated successfully",
STATUS_PAGE_DELETE: "Status page deleted successfully",
STATUS_PAGE_GET_ALL: "Got all status pages successfully",

};

export { errorMessages, successMessages };
28 changes: 28 additions & 0 deletions Server/validation/joi.js
Original file line number Diff line number Diff line change
Expand Up @@ -410,6 +410,32 @@ const updateAppSettingsBodyValidation = joi.object({
systemEmailPassword: joi.string().allow(""),
});

//****************************************
// Status Page Validation
//****************************************

const getStatusPageParamValidation = joi.object({
url: joi.string().required(),
});
Comment on lines +417 to +419
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add URL format validation to prevent invalid URLs.

The URL validation could be strengthened by adding format checks.

 const getStatusPageParamValidation = joi.object({
-	url: joi.string().required(),
+	url: joi.string().uri().lowercase().required()
+		.pattern(/^[a-z0-9-]+$/)
+		.messages({
+			'string.uri': 'URL must be a valid URI',
+			'string.pattern.base': 'URL can only contain lowercase letters, numbers, and hyphens'
+		}),
 });

Committable suggestion skipped: line range outside the PR's diff.


const createStatusPageBodyValidation = joi.object({
companyName: joi.string().required(),
url: joi.string().required(),
timezone: joi.string().required(),
color: joi.string().required(),
theme: joi.string().required(),
monitors: joi
.array()
.items(joi.string().pattern(/^[0-9a-fA-F]{24}$/))
.required()
.messages({
"string.pattern.base": "Must be a valid monitor ID",
"array.base": "Monitors must be an array",
"array.empty": "At least one monitor is required",
"any.required": "Monitors are required",
}),
});

export {
roleValidatior,
loginValidation,
Expand Down Expand Up @@ -465,4 +491,6 @@ export {
editMaintenanceWindowByIdParamValidation,
editMaintenanceByIdWindowBodyValidation,
updateAppSettingsBodyValidation,
createStatusPageBodyValidation,
getStatusPageParamValidation,
};