Skip to content
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
121 changes: 117 additions & 4 deletions api/api.js
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we review the functions in this file to add logger.info and logger.error statements so that we have data in our logs about what is happening here

If we don't log errors or info about what is happening, we won't be able to debug problems.

Note. We should try to write detailed logs but we don't want to write any sensitve information to the log such as ids, passwords or email address

Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import { Router } from "express";
import dotenv from "dotenv";
import express from "express";
const { Router } = express;

import db from "./db.js";
import { lookupEmail } from "./functions/lookupEmail.js";
Expand All @@ -8,12 +10,123 @@ import { updateUsersActivity } from "./functions/updateUsersActivity.js";
import messageRouter from "./messages/messageRouter.js";
import { processUpload } from "./middlewares/processUpload.js";
import { zipExtractor } from "./middlewares/zipExtractor.js";
import { CLIENT_ID, CLIENT_SECRET } from "./utils/config.cjs";
import logger from "./utils/logger.js";
const fetch = (...args) =>
import("node-fetch").then(({ default: fetch }) => fetch(...args));

dotenv.config();
const api = Router();

const verifyToken = async (req, res, next) => {
const authHeader = req.headers.authorization;

if (!authHeader?.startsWith("Bearer ")) {
return res.status(401).json({ error: "Unauthorized: No token provided" });
}

const accessToken = authHeader;

try {
const response = await fetch("https://api.github.com/user", {
method: "GET",
headers: {
Authorization: accessToken,
Accept: "application/json",
},
});

if (!response.ok) {
return res
.status(401)
.json({ success: false, message: "Invalid or expired token" });
}

const userData = await response.json();
req.user = userData;
next();
} catch (error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Everywhere where we have an error we should capture this with logger.error

return res
.status(500)
.json({ success: false, message: "Error verifying token", error });
}
};

api.use("/message", messageRouter);

api.post("/getAccessToken", async (req, res) => {
const { code } = req.body;

if (!code) {
return res
.status(400)
.json({ success: false, message: "Code parameter is required" });
}

const params = new URLSearchParams({
client_id: CLIENT_ID,
client_secret: CLIENT_SECRET,
code,
});

try {
const tokenResponse = await fetch(
`https://github.com/login/oauth/access_token?${params.toString()}`,
{
method: "POST",
headers: { Accept: "application/json" },
},
);

const tokenData = await tokenResponse.json();

if (!tokenData.access_token) {
return res
.status(400)
.json({ success: false, message: "Failed to retrieve access token" });
}

return res.json({ success: true, access_token: tokenData.access_token });
} catch (error) {
return res.status(500).json({
success: false,
message: "Internal server error",
error: error.toString(),
});
}
});

api.post("/getUserData", async (req, res) => {
const { access_token } = req.body;

if (!access_token) {
return res.status(400).json({
success: false,
message: "Access token is required to fetch user data",
});
}

try {
const userResponse = await fetch("https://api.github.com/user", {
method: "GET",
headers: {
Authorization: `Bearer ${access_token}`,
Accept: "application/json",
},
});

const userData = await userResponse.json();

return res.json({ success: true, user: userData });
} catch (error) {
return res.status(500).json({
success: false,
message: "Internal server error",
error: error.toString(),
});
}
});

api.post("/subscribe", async (req, res) => {
const email = req.body.email;
try {
Expand Down Expand Up @@ -60,11 +173,11 @@ api.post("/subscribe", async (req, res) => {
}
});

api.get("/fetch-users", async (req, res) => {
api.get("/fetch-users", verifyToken, async (req, res) => {
try {
const result = await db.query("SELECT * FROM all_users");

if (!result.rows.length === 0) {
if (!result.rows.length) {
res.status(404).json({ success: false, message: "User not fund" });
} else {
res.status(200).json(result.rows);
Expand All @@ -74,7 +187,7 @@ api.get("/fetch-users", async (req, res) => {
}
});

api.post("/upload", processUpload, async (req, res) => {
api.post("/upload", verifyToken, processUpload, async (req, res) => {
try {
const slackZipBuffer = req.file.buffer;
const extractedDir = zipExtractor(slackZipBuffer);
Expand Down
4 changes: 4 additions & 0 deletions api/app.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import bodyParser from "body-parser";
import cors from "cors";
Comment on lines +1 to +2
Copy link
Collaborator

Choose a reason for hiding this comment

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

What was the reason for including these?

It's always good to comment your own PRs with explanations for things so it's clear to reviewers why things area happening

import express from "express";

import apiRouter from "./api.js";
Expand All @@ -14,6 +16,7 @@ import {

const apiRoot = "/api";
const app = express();
app.use(cors());

app.use(express.urlencoded({ extended: true }));
app.use(express.json());
Expand All @@ -33,6 +36,7 @@ app.get(
}),
);

app.use(bodyParser.json());
app.use(apiRoot, apiRouter);

app.use(clientRouter(apiRoot));
Expand Down
3 changes: 3 additions & 0 deletions api/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,14 @@
},
"dependencies": {
"@slack/web-api": "^7.8.0",
"body-parser": "^1.20.3",
"cors": "^2.8.5",
"adm-zip": "^0.5.16",
"dotenv": "^16.4.7",
"express": "^4.21.2",
"helmet": "^7.2.0",
"morgan": "^1.10.0",
"node-fetch": "^3.3.2",
"multer": "^1.4.5-lts.1",
"node-pg-migrate": "^7.9.0",
"pg": "^8.13.1",
Expand Down
3 changes: 3 additions & 0 deletions api/utils/config.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,6 @@ function requireArgs(required) {
);
}
}

module.exports.CLIENT_SECRET = process.env.GITHUB_CLIENT_SECRET;
module.exports.CLIENT_ID = process.env.GITHUB_CLIENT_ID;
Comment on lines +59 to +60
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is the correct way to include these values. We already have an existing module.exports line

Better would be to have

github: {
  GITHUB_CLIENT_SECRET: process.env.GITHUB_CLIENT_SECRET,
  // etc.
}

and add this to the extsing config object

2 changes: 1 addition & 1 deletion e2e/tests/home.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { test, expect } from "@playwright/test";
test("has correct page title", async ({ page }) => {
await page.goto("/", { waitUntil: "domcontentloaded" });

await expect(page).toHaveTitle("CYF Slack Dashboard");
await expect(page).toHaveTitle("CYF Slack");
});

test("homepage loads successfully", async ({ page }) => {
Expand Down
Loading