-
Notifications
You must be signed in to change notification settings - Fork 273
feat: implement OOO approve/reject functionality #2460
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,10 @@ | ||
const CLOUDFLARE_WORKER = "Cloudflare Worker"; | ||
const BAD_TOKEN = "BAD.JWT.TOKEN"; | ||
const CRON_JOB_HANDLER = "Cron Job Handler"; | ||
const DISCORD_SERVICE = "Discord Service"; | ||
|
||
const Services = { | ||
CLOUDFLARE_WORKER: CLOUDFLARE_WORKER, | ||
CRON_JOB_HANDLER: CRON_JOB_HANDLER, | ||
}; | ||
|
||
const DiscordServiceHeader = { | ||
name: "x-service-name" | ||
} | ||
|
||
module.exports = { CLOUDFLARE_WORKER, BAD_TOKEN, CRON_JOB_HANDLER, Services, DISCORD_SERVICE, DiscordServiceHeader }; | ||
module.exports = { CLOUDFLARE_WORKER, BAD_TOKEN, CRON_JOB_HANDLER, Services }; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,82 +1,62 @@ | ||
const authService = require("../services/authService"); | ||
const dataAccess = require("../services/dataAccessLayer"); | ||
const logger = require("../utils/logger"); | ||
|
||
/** | ||
* Middleware to check if the user is restricted or in an impersonation session. | ||
* | ||
* - If the user is impersonating, only GET requests and the STOP impersonation route are allowed. | ||
* - If the user is restricted (based on roles), only GET requests are permitted. | ||
* Middleware to check if the user has been restricted. If user is restricted, | ||
* then only allow read requests and do not allow to any edit/create requests. | ||
* | ||
* Note: This requires that user is authenticated hence must be called after | ||
* the user authentication middleware. We are calling it from within the | ||
* `authenticate` middleware itself to avoid explicitly adding this middleware | ||
* while defining routes. | ||
* | ||
* @async | ||
* @function checkRestricted | ||
* @param {import('express').Request} req - Express request object | ||
* @param {import('express').Response} res - Express response object | ||
* @param {Function} next - Express next middleware function | ||
* @returns {void} | ||
* @param {Object} req - Express request object | ||
* @param {Object} res - Express response object | ||
* @param {Function} next - Express middleware function | ||
* @returns {Object} - Returns unauthorized object if user has been restricted. | ||
*/ | ||
const checkRestricted = async (req, res, next) => { | ||
const { roles } = req.userData; | ||
|
||
if (req.isImpersonating) { | ||
const isStopImpersonationRoute = | ||
req.method === "PATCH" && | ||
req.baseUrl === "/impersonation" && | ||
/^\/[a-zA-Z0-9_-]+$/.test(req.path) && | ||
req.query.action === "STOP"; | ||
|
||
if (req.method !== "GET" && !isStopImpersonationRoute) { | ||
return res.boom.forbidden("Only viewing is permitted during impersonation"); | ||
} | ||
} | ||
|
||
if (roles && roles.restricted && req.method !== "GET") { | ||
return res.boom.forbidden("You are restricted from performing this action"); | ||
} | ||
|
||
return next(); | ||
}; | ||
|
||
/** | ||
* Authentication middleware that: | ||
* 1. Verifies JWT token from cookies (or headers in non-production). | ||
* 2. Handles impersonation if applicable. | ||
* 3. Refreshes token if it's expired but still within the refresh TTL window. | ||
* 4. Attaches user data to `req.userData` for downstream use. | ||
* Middleware to validate the authenticated routes | ||
* 1] Verifies the token and adds user info to `req.userData` for further use | ||
* 2] In case of JWT expiry, adds a new JWT to the response if `currTime - tokenInitialisationTime <= refreshTtl` | ||
* | ||
* @async | ||
* @function | ||
* @param {import('express').Request} req - Express request object | ||
* @param {import('express').Response} res - Express response object | ||
* @param {Function} next - Express next middleware function | ||
* @returns {Promise<void>} - Calls `next()` on successful authentication or returns an error response. | ||
* The currently implemented mechanism satisfies the current use case. | ||
* Authentication with JWT and a refreshToken to be added once we have user permissions and authorizations to be handled | ||
* | ||
* @todo: Add tests to assert on refreshed JWT generation by modifying the TTL values for the specific test. Currently not possible in the absence of a test-suite. | ||
* | ||
* | ||
* @param req {Object} - Express request object | ||
* @param res {Object} - Express response object | ||
* @param next {Function} - Express middleware function | ||
* @return {Object} - Returns unauthenticated object if token is invalid | ||
*/ | ||
module.exports = async (req, res, next) => { | ||
try { | ||
let token = req.cookies[config.get("userToken.cookieName")]; | ||
|
||
/** | ||
* Enable Bearer Token authentication for NON-PRODUCTION environments. | ||
* Useful for Swagger or manual testing where cookies are not easily managed. | ||
* Enable Bearer Token authentication for NON-PRODUCTION environments | ||
* This is enabled as Swagger UI does not support cookie authe | ||
*/ | ||
if (process.env.NODE_ENV !== "production" && !token) { | ||
token = req.headers.authorization?.split(" ")[1]; | ||
token = req.headers.authorization.split(" ")[1]; | ||
} | ||
Comment on lines
50
to
52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unsafe Authorization Header Access
Tell me moreWhat is the issue?The code assumes req.headers.authorization exists when accessing it, which could cause a runtime error if authorization header is missing. Why this mattersIf authorization header is undefined, attempting to call split() on it will throw a TypeError and crash the application. Suggested change ∙ Feature PreviewAdd a check for the existence of the authorization header before accessing it: if (process.env.NODE_ENV !== 'production' && !token) {
token = req.headers.authorization?.split(' ')[1] || null;
} Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||
|
||
const { userId, impersonatedUserId } = authService.verifyAuthToken(token); | ||
// `req.isImpersonating` keeps track of the impersonation session | ||
req.isImpersonating = Boolean(impersonatedUserId); | ||
|
||
const userData = impersonatedUserId | ||
? await dataAccess.retrieveUsers({ id: impersonatedUserId }) | ||
: await dataAccess.retrieveUsers({ id: userId }); | ||
const { userId } = authService.verifyAuthToken(token); | ||
|
||
// add user data to `req.userData` for further use | ||
const userData = await dataAccess.retrieveUsers({ id: userId }); | ||
req.userData = userData.user; | ||
|
||
return checkRestricted(req, res, next); | ||
} catch (err) { | ||
logger.error(err); | ||
|
@@ -98,13 +78,14 @@ module.exports = async (req, res, next) => { | |
sameSite: "lax", | ||
}); | ||
|
||
const userData = await dataAccess.retrieveUsers({ id: userId }); | ||
req.userData = userData.user; | ||
|
||
// add user data to `req.userData` for further use | ||
req.userData = await dataAccess.retrieveUsers({ id: userId }); | ||
return checkRestricted(req, res, next); | ||
} else { | ||
return res.boom.unauthorized("Unauthenticated User"); | ||
} | ||
} else { | ||
return res.boom.unauthorized("Unauthenticated User"); | ||
} | ||
return res.boom.unauthorized("Unauthenticated User"); | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incomplete Security Feature Documentation
Tell me more
What is the issue?
The comment contains a typo ('authe' instead of 'auth') and doesn't explain the security implications of this feature.
Why this matters
Unclear documentation of security-related features can lead to misuse or security vulnerabilities.
Suggested change ∙ Feature Preview
/**
*/
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.