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

Implement a better API with tenancy-based configuration and storage #13

Open
wants to merge 20 commits into
base: staging
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
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
9 changes: 8 additions & 1 deletion .env.example
Original file line number Diff line number Diff line change
@@ -1,2 +1,9 @@
PORT=8080
DEBUG=FALSE
DEBUG=FALSE
APIKEY=KEY

DBHOST=localhost
DBPORT=3306
DBUSER=root
DBPASSWORD=root
DBNAME=devoteMe
Comment on lines +3 to +9
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve security in environment variable examples

  1. Using root credentials in examples is not recommended
  2. API key example is too simple
  3. Missing documentation for required formats
-APIKEY=KEY
+# Required: Strong API key for authentication
+APIKEY=<STRONG_API_KEY_HERE>
 
-DBHOST=localhost
-DBPORT=3306
-DBUSER=root
-DBPASSWORD=root
-DBNAME=devoteMe
+# Database Configuration
+DBHOST=localhost
+DBPORT=3306
+# Avoid using root credentials in production
+DBUSER=app_user
+DBPASSWORD=<STRONG_PASSWORD_HERE>
+DBNAME=devoteMe
📝 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
APIKEY=KEY
DBHOST=localhost
DBPORT=3306
DBUSER=root
DBPASSWORD=root
DBNAME=devoteMe
# Required: Strong API key for authentication
APIKEY=<STRONG_API_KEY_HERE>
# Database Configuration
DBHOST=localhost
DBPORT=3306
# Avoid using root credentials in production
DBUSER=app_user
DBPASSWORD=<STRONG_PASSWORD_HERE>
DBNAME=devoteMe

1 change: 1 addition & 0 deletions CODEOWNERS
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* @benrobson
89 changes: 89 additions & 0 deletions api/common.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import dotenv from "dotenv";
dotenv.config();
import fetch from "node-fetch";
import db from "../controllers/databaseController.js";

/*
Ensure that a required field is present and has a non-null value,
and to return an error message if this is not the case.

@param body Passing through the req.body
@param field The name of the field.
@param res Passing through res.
*/
export function required(body, field, res) {
// Prematurely exits an API request if a required field has not been
// defined or null. If the body is not defined then we error as well.
// This can happen when no parameters exist.
if (!body || !(field in body))
return res.send({
success: false,
message: `Body requires field '${field}'`,
});

if (body[field] === null)
return res.send({
success: false,
message: `Field ${field} cannot be null`,
});

return body[field];
}

/*
Check if an optional field is present in the body object, and return its value if it is.

@param body Passing through the req.body
@param field The name of the field.
*/
export function optional(body, field) {
// Jaedan: I am aware that this is pretty much default behaviour, however
// this takes into consideration times where no body is included. Without
// this check requests with only optional fields (that are all unused) will
// cause a null object to be referenced, causing an error.
if (!body || !(field in body) || body[field] === null) return null;

return body[field];
}

/*
Makes a POST API request to the specified postURL with the provided apiPostBody.
It includes a header with the x-access-token value taken from an environment variable named APIKEY.
If the request is successful, it logs the response data.
If the request fails, it sets a cookie with a "danger" alert type and an error message,
then redirects the user to the specified failureRedirectURL.

@param postURL The POST url that the apiPostBody will go to in the API.
@param apiPostBody The request body for the postURL.
@param failureRedirectURL If the request returns false, where the API will redirect the user to.
@param res Passing through res.
*/
export async function postAPIRequest(
postURL,
apiPostBody,
failureRedirectURL,
res
) {
const response = await fetch(postURL, {
method: "POST",
body: JSON.stringify(apiPostBody),
headers: {
"Content-Type": "application/json",
"x-access-token": process.env.APIKEY,
},
});

const data = await response.json();

console.log(data);

if (data.alertType) {
setBannerCookie(`${data.alertType}`, `${data.alertContent}`, res);
}

if (!data.success) {
return res.redirect(failureRedirectURL);
}

return console.log(data);
}
Comment on lines +61 to +89
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix critical issues in postAPIRequest function

Several issues need to be addressed:

  1. Missing error handling for fetch failures
  2. Logging sensitive data to console
  3. Undefined setBannerCookie function
  4. Incorrect return value
 export async function postAPIRequest(
   postURL,
   apiPostBody,
   failureRedirectURL,
   res
 ) {
-  const response = await fetch(postURL, {
-    method: "POST",
-    body: JSON.stringify(apiPostBody),
-    headers: {
-      "Content-Type": "application/json",
-      "x-access-token": process.env.APIKEY,
-    },
-  });
+  try {
+    const response = await fetch(postURL, {
+      method: "POST",
+      body: JSON.stringify(apiPostBody),
+      headers: {
+        "Content-Type": "application/json",
+        "x-access-token": process.env.APIKEY,
+      },
+    });
 
-  const data = await response.json();
+    if (!response.ok) {
+      throw new Error(`HTTP error! status: ${response.status}`);
+    }
 
-  console.log(data);
+    const data = await response.json();
 
-  if (data.alertType) {
-    setBannerCookie(`${data.alertType}`, `${data.alertContent}`, res);
-  }
+    // TODO: Import or implement setBannerCookie function
+    if (data.alertType) {
+      setBannerCookie(`${data.alertType}`, `${data.alertContent}`, res);
+    }
 
-  if (!data.success) {
-    return res.redirect(failureRedirectURL);
-  }
+    if (!data.success) {
+      return res.redirect(failureRedirectURL);
+    }
 
-  return console.log(data);
+    return data;
+  } catch (error) {
+    console.error('API request failed:', error.message);
+    return res.redirect(failureRedirectURL);
+  }
 }
📝 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
export async function postAPIRequest(
postURL,
apiPostBody,
failureRedirectURL,
res
) {
const response = await fetch(postURL, {
method: "POST",
body: JSON.stringify(apiPostBody),
headers: {
"Content-Type": "application/json",
"x-access-token": process.env.APIKEY,
},
});
const data = await response.json();
console.log(data);
if (data.alertType) {
setBannerCookie(`${data.alertType}`, `${data.alertContent}`, res);
}
if (!data.success) {
return res.redirect(failureRedirectURL);
}
return console.log(data);
}
export async function postAPIRequest(
postURL,
apiPostBody,
failureRedirectURL,
res
) {
try {
const response = await fetch(postURL, {
method: "POST",
body: JSON.stringify(apiPostBody),
headers: {
"Content-Type": "application/json",
"x-access-token": process.env.APIKEY,
},
});
if (!response.ok) {
throw new Error(`HTTP error! status: ${response.status}`);
}
const data = await response.json();
// TODO: Import or implement setBannerCookie function
if (data.alertType) {
setBannerCookie(`${data.alertType}`, `${data.alertContent}`, res);
}
if (!data.success) {
return res.redirect(failureRedirectURL);
}
return data;
} catch (error) {
console.error('API request failed:', error.message);
return res.redirect(failureRedirectURL);
}
}

57 changes: 57 additions & 0 deletions api/routes/devotion.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import { required, optional } from "../common.js";

export default function devotionApiRoute(app, db) {
const baseEndpoint = "/api/devotion";

app.get(baseEndpoint + "/check", async function (req, res) {
const tenantId = required(req.query, "tenantId", res);
const messageId = required(req.query, "messageId", res);
const userId = required(req.query, "userId", res);

try {
// Perform a SELECT query to check if the record exists
const [rows] = await db.query(
`SELECT 1 FROM devotions WHERE tenantId = ? AND messageId = ? AND userId = ? LIMIT 1`,
[tenantId, messageId, userId]
);

// If any row is returned, that means the record exists
const exists = rows.length > 0;

// Return a boolean indicating if the record exists
res.send({
success: true,
exists: exists,
});
} catch (error) {
res.send({
success: false,
message: `Error checking Devotion entry: ${error}`,
});
}
});

// Create devotion endpoint
app.post(baseEndpoint + "/add", async function (req, res) {
const tenantId = required(req.body, "tenantId", res);
const messageId = required(req.body, "messageId", res);
const userId = required(req.body, "userId", res);

try {
await db.query(
`INSERT INTO devotions (tenantId, messageId, userId) VALUES (?, ?, ?)`,
[tenantId, messageId, userId]
);

res.send({
success: true,
content: `Devotion entry created for tenant ${tenantId} with message ID ${messageId}`,
});
} catch (error) {
res.send({
success: false,
message: `Error creating devotion entry: ${error}`,
});
}
});
}
Comment on lines +1 to +57
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Extract common API functionality to reduce duplication

The code is nearly identical to votd.js. Consider creating a reusable factory function for these endpoints.

Create a new file api/common/recordEndpoints.js:

export function createRecordEndpoints(options) {
  const { app, db, tableName, baseEndpoint } = options;

  const checkRecord = async (req, res) => {
    const tenantId = required(req.query, "tenantId", res);
    const messageId = required(req.query, "messageId", res);
    const userId = required(req.query, "userId", res);

    if (!Number.isInteger(Number(tenantId)) || !Number.isInteger(Number(messageId))) {
      return res.status(400).send({
        success: false,
        error: "Invalid tenantId or messageId format"
      });
    }

    try {
      const [rows] = await db.query(
        `SELECT 1 FROM ${tableName} WHERE tenantId = ? AND messageId = ? AND userId = ? LIMIT 1`,
        [tenantId, messageId, userId]
      );

      res.status(200).send({
        success: true,
        exists: rows.length > 0,
      });
    } catch (error) {
      res.status(500).send({
        success: false,
        error: 'Internal server error'
      });
    }
  };

  const addRecord = async (req, res) => {
    // Similar implementation for POST
  };

  app.get(`${baseEndpoint}/check`, checkRecord);
  app.post(`${baseEndpoint}/add`, addRecord);
}

Then simplify both route files:

// devotion.js
import { createRecordEndpoints } from '../common/recordEndpoints.js';

export default function devotionApiRoute(app, db) {
  createRecordEndpoints({
    app,
    db,
    tableName: 'devotions',
    baseEndpoint: '/api/devotion'
  });
}

9 changes: 9 additions & 0 deletions api/routes/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import devotionApiRoute from "./devotion.js";
import tenantApiRoute from "./tenant.js";
import votdApiRoute from "./votd.js";

export default (app, db) => {
tenantApiRoute(app, db);
devotionApiRoute(app, db);
votdApiRoute(app, db);
};
197 changes: 197 additions & 0 deletions api/routes/tenant.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,197 @@
import { required, optional } from "../common.js";

export default function tenantApiRoute(app, db) {
const baseEndpoint = "/api/tenant";

// Function to fetch tenants
async function getTenants(dbQuery) {
const [results, fields] = await db.query(dbQuery);
return results;
}

// Get tenant(s) endpoint
app.get(baseEndpoint + "/get", async function (req, res) {
const tenantId = optional(req.query, "id");

try {
// Get Tenant by specific ID
if (tenantId) {
let dbQuery = `SELECT * FROM tenants WHERE tenantId=${tenantId};`;
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Prevent SQL Injection by using parameterized queries

The current query constructs the SQL statement by directly embedding tenantId, which can lead to SQL injection vulnerabilities if tenantId is not properly sanitized.

Apply this diff to use a parameterized query:

-let dbQuery = `SELECT * FROM tenants WHERE tenantId=${tenantId};`;
+let dbQuery = `SELECT * FROM tenants WHERE tenantId = ?;`;
+const results = await getTenants(dbQuery, [tenantId]);

Update the getTenants function to accept parameters:

-async function getTenants(dbQuery) {
+async function getTenants(dbQuery, params = []) {
   const [results, fields] = await db.query(dbQuery, params);
   return results;
}

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

const results = await getTenants(dbQuery);
if (!results.length) {
return res.send({
success: false,
message: `There are no tenants`,
});
}

res.send({
success: true,
data: results,
});
return;
}

// Show all Tenants
let dbQuery = `SELECT * FROM tenants;`;
const results = await getTenants(dbQuery);
if (!results.length) {
return res.send({
success: false,
message: `There are no tenants`,
});
}

res.send({
success: true,
data: results,
});
} catch (error) {
res.send({
success: false,
message: `${error}`,
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid exposing detailed error messages to clients

Returning raw error messages can reveal sensitive information. It's better to send a generic message to the client and log the detailed error on the server.

Apply this diff to modify the error responses:

-        message: `${error}`,
+        message: 'An unexpected error occurred. Please try again later.',

Repeat this change for all error handlers.

Also applies to: 80-80, 126-126, 193-193

});
}
});

// Get tenant configuration by ID endpoint
app.get(baseEndpoint + "/configuration/get", async function (req, res) {
const tenantId = required(req.query, "id", res);

try {
// Get Tenant Configuration by specific ID
const dbQuery = `SELECT * FROM tenantConfiguration WHERE tenantId = ?`;
const results = await db.query(dbQuery, [tenantId]);
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Destructure query results to access data correctly

The db.query() method returns an array [results, fields]. Assigning it directly to results can lead to unexpected behavior when accessing results.length.

Apply this diff to destructure the returned array:

-const results = await db.query(dbQuery, [tenantId]);
+const [results, fields] = await db.query(dbQuery, [tenantId]);
📝 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 results = await db.query(dbQuery, [tenantId]);
const [results, fields] = await db.query(dbQuery, [tenantId]);


if (!results.length) {
return res.send({
success: false,
message: `No configuration found for tenantId ${tenantId}`,
});
}

res.send({
success: true,
data: results[0],
});
} catch (error) {
res.send({
success: false,
message: `Error: ${error.message}`,
});
}
});

// Create tenant endpoint
app.post(baseEndpoint + "/create", async function (req, res) {
const tenantId = required(req.body, "tenantId", res);
const tenantName = required(req.body, "tenantName", res);

console.log(
`Received request to create tenant. Tenant ID: ${tenantId}, Tenant Name: ${tenantName}`
);

try {
// Insert into tenants table
console.log(`Inserting into tenants table: ${tenantId}, ${tenantName}`);
await db.query(
`INSERT INTO tenants (tenantId, tenantName) VALUES (?, ?)`,
[tenantId, tenantName]
);

// Insert into tenantConfiguration table with the tenantId
console.log(
`Inserting into tenantConfiguration table for tenantId: ${tenantId}`
);
await db.query(`INSERT INTO tenantConfiguration (tenantId) VALUES (?)`, [
tenantId,
]);

console.log(
`Successfully created tenant: ${tenantName} (ID: ${tenantId})`
);

res.send({
success: true,
content: `New Tenant Created: ${tenantName}`,
});
} catch (error) {
console.error(
`Error occurred while creating tenant: ${tenantName} (ID: ${tenantId})`,
error
);

res.send({
success: false,
message: `${error.message}`,
});
}
});

// Update tenant endpoint
app.post(baseEndpoint + "/update", async function (req, res) {
const tenantId = required(req.body, "tenantId", res);
const votdChannel = optional(req.body, "votd_channel", res);
const devotionChannel = optional(req.body, "devotion_channel", res);

try {
// Determine the field and value to update
let fieldToUpdate = "";
let valueToUpdate = null;

if (votdChannel) {
fieldToUpdate = "votd_channel";
valueToUpdate = votdChannel;
} else if (devotionChannel) {
fieldToUpdate = "devotion_channel";
valueToUpdate = devotionChannel;
}

if (fieldToUpdate) {
// Check if the tenant's configuration already exists in tenantConfiguration
const existingConfig = await db.query(
`SELECT id FROM tenantConfiguration WHERE tenantId = ?`,
[tenantId]
);
Comment on lines +153 to +155
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Correctly handle existing configuration query results

Similar to earlier, destructure the result of db.query() to properly access existingConfig.

Apply this diff:

-const existingConfig = await db.query(
+const [existingConfig, fields] = await db.query(
   `SELECT id FROM tenantConfiguration WHERE tenantId = ?`,
   [tenantId]
);
📝 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
`SELECT id FROM tenantConfiguration WHERE tenantId = ?`,
[tenantId]
);
const [existingConfig, fields] = await db.query(
`SELECT id FROM tenantConfiguration WHERE tenantId = ?`,
[tenantId]
);


if (existingConfig.length > 0) {
// If configuration exists, update the field
await db.query(
`
UPDATE tenantConfiguration
SET ${fieldToUpdate} = ?
WHERE tenantId = ?;
`,
[valueToUpdate, tenantId]
);
} else {
// If configuration doesn't exist, insert new values
await db.query(
`
INSERT INTO tenantConfiguration (tenantId, ${fieldToUpdate})
VALUES (?, ?);
`,
[tenantId, valueToUpdate]
);
}

return res.send({
success: true,
message: `Tenant configuration updated`,
});
} else {
return res.send({
success: false,
message: "No valid field provided to update.",
});
}
} catch (error) {
console.log(error);

res.send({
success: false,
message: `Error: ${error.message}`,
});
}
});
}
23 changes: 23 additions & 0 deletions api/routes/verifyToken.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@

export default function verifyToken(req, res, done) {
var token = req.headers["x-access-token"];

if (!token) {
// Token not included
return res.send({
success: false,
message: `lang.api.noToken`,
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Replace placeholder messages with meaningful error responses

The messages lang.api.noToken and lang.api.invalidToken appear to be placeholders. Clients will receive these as-is, which might be confusing.

Update the messages to provide clear feedback:

-      message: `lang.api.noToken`,
+      message: 'No access token provided.',
-      message: `lang.api.invalidToken`,
+      message: 'Invalid access token.',

Also applies to: 20-20

});
}

if (token === process.env.APIKEY) {
// Passed
done();
} else {
// Token was incorrect.
return res.send({
success: false,
message: `lang.api.invalidToken`,
});
}
}
Loading