feat: implement OOO approve/reject functionality #2460
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add OOO request approval/rejection logic - Update request validation and error handling - Add tests for OOO approval workflow
|
Caution Review failedThe pull request is closed. Summary by CodeRabbit
WalkthroughThis update removes all impersonation and Discord service token verification logic, including related configuration, constants, middleware, and tests. It introduces the core implementation, validation, and controller logic for acknowledging Out-Of-Office (OOO) requests, along with supporting types and test coverage. Several test suites are revised or activated to reflect these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant Auth
participant OOOService
participant RequestsModel
User->>API: PATCH /requests/:id (type: OOO)
API->>Auth: Authenticate & Authorize
Auth-->>API: User data
API->>OOOService: acknowledgeOooRequest(id, body, superUserId)
OOOService->>RequestsModel: getRequestById(id)
RequestsModel-->>OOOService: Request data or error
OOOService->>OOOService: validateOooAcknowledgeRequest(type, status)
OOOService->>RequestsModel: updateRequest(id, newStatus, ...)
RequestsModel-->>OOOService: Updated request or error
OOOService-->>API: Success message & updated request
API-->>User: Response (success or error)
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (33)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| */ | ||
| router.post("/invite", disableRoute, authenticate, checkCanGenerateDiscordLink, generateInviteForUser); | ||
|
|
||
| router.get("/invite", authenticate, getUserDiscordInvite); |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the problem, add a rate-limiting middleware to the affected route. The best approach is to use the well-known express-rate-limit package. This involves importing the package, configuring a limiter (e.g., allow a reasonable number of requests per minute per user/IP), and attaching it to the sensitive route(s) only. This avoids affecting other routes unnecessarily and minimizes the risk of breaking existing functionality.
Specifically:
- Add a
require("express-rate-limit")import at the top. - Define a rate limiter instance with appropriate settings (for example,
windowMs: 1 minute,max: 5requests per window). - Insert the rate limiter middleware as the first or second argument for
router.post("/roles", ...), immediately after the route path.
All changes are confined to routes/discordactions.js.
Suggested changeset
2
routes/discordactions.js
| @@ -1,3 +1,4 @@ | ||
| const rateLimit = require("express-rate-limit"); | ||
| const express = require("express"); | ||
| const authenticate = require("../middlewares/authenticate"); | ||
| const { | ||
| @@ -33,10 +34,18 @@ | ||
| const { authorizeAndAuthenticate } = require("../middlewares/authorizeUsersAndService"); | ||
| const router = express.Router(); | ||
|
|
||
| // Rate limiter for sensitive POST /roles endpoint | ||
| const addRoleLimiter = rateLimit({ | ||
| windowMs: 1 * 60 * 1000, // 1 minute window | ||
| max: 5, // limit each IP/user to 5 requests per windowMs | ||
| standardHeaders: true, // Return rate limit info in the `RateLimit-*` headers | ||
| legacyHeaders: false, // Disable the `X-RateLimit-*` headers | ||
| }); | ||
|
|
||
| router.post("/groups", authenticate, checkIsVerifiedDiscord, validateGroupRoleBody, createGroupRole); | ||
| router.get("/groups", authenticate, checkIsVerifiedDiscord, validateLazyLoadingParams, getPaginatedAllGroupRoles); | ||
| router.delete("/groups/:groupId", authenticate, checkIsVerifiedDiscord, authorizeRoles([SUPERUSER]), deleteGroupRole); | ||
| router.post("/roles", authenticate, checkIsVerifiedDiscord, validateMemberRoleBody, addGroupRoleToMember); | ||
| router.post("/roles", addRoleLimiter, authenticate, checkIsVerifiedDiscord, validateMemberRoleBody, addGroupRoleToMember); | ||
| router.get("/invite", authenticate, getUserDiscordInvite); | ||
| router.post("/invite", authenticate, checkCanGenerateDiscordLink, generateInviteForUser); | ||
| router.delete("/roles", authenticate, checkIsVerifiedDiscord, deleteRole); |
package.json
Outside changed files
| @@ -42,7 +42,8 @@ | ||
| "passport-github2": "0.1.12", | ||
| "passport-google-oauth20": "^2.0.0", | ||
| "rate-limiter-flexible": "5.0.3", | ||
| "winston": "3.13.0" | ||
| "winston": "3.13.0", | ||
| "express-rate-limit": "^8.0.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/chai": "4.3.16", |
This fix introduces these dependencies
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.0.1 | None |
Copilot is powered by AI and may make mistakes. Always verify output.
| */ | ||
| router.post("/invite", disableRoute, authenticate, checkCanGenerateDiscordLink, generateInviteForUser); | ||
|
|
||
| router.get("/invite", authenticate, getUserDiscordInvite); |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the missing rate limiting issue for the /invite GET route, we should use a rate-limiting middleware such as express-rate-limit. We will require the package at the top of the file, configure a rate limiter (e.g., allow 100 requests per 15 minutes per IP), and apply this middleware specifically to the /invite GET route. This ensures that repeated requests to this endpoint are restricted, mitigating denial-of-service risks. The changes are all in routes/discordactions.js:
- Import
express-rate-limit - Define a rate limiter instance
- Apply it as middleware to the GET
/inviteroute in addition toauthenticate
Suggested changeset
2
routes/discordactions.js
| @@ -1,3 +1,4 @@ | ||
| const rateLimit = require("express-rate-limit"); | ||
| const express = require("express"); | ||
| const authenticate = require("../middlewares/authenticate"); | ||
| const { | ||
| @@ -32,12 +33,11 @@ | ||
| const { verifyCronJob } = require("../middlewares/authorizeBot"); | ||
| const { authorizeAndAuthenticate } = require("../middlewares/authorizeUsersAndService"); | ||
| const router = express.Router(); | ||
|
|
||
| router.post("/groups", authenticate, checkIsVerifiedDiscord, validateGroupRoleBody, createGroupRole); | ||
| router.get("/groups", authenticate, checkIsVerifiedDiscord, validateLazyLoadingParams, getPaginatedAllGroupRoles); | ||
| router.delete("/groups/:groupId", authenticate, checkIsVerifiedDiscord, authorizeRoles([SUPERUSER]), deleteGroupRole); | ||
| router.post("/roles", authenticate, checkIsVerifiedDiscord, validateMemberRoleBody, addGroupRoleToMember); | ||
| router.get("/invite", authenticate, getUserDiscordInvite); | ||
| router.get("/invite", authenticate, getInviteLimiter, getUserDiscordInvite); | ||
| router.post("/invite", authenticate, checkCanGenerateDiscordLink, generateInviteForUser); | ||
| router.delete("/roles", authenticate, checkIsVerifiedDiscord, deleteRole); | ||
| router.get("/roles", authenticate, checkIsVerifiedDiscord, getGroupsRoleId); |
package.json
Outside changed files
| @@ -42,7 +42,8 @@ | ||
| "passport-github2": "0.1.12", | ||
| "passport-google-oauth20": "^2.0.0", | ||
| "rate-limiter-flexible": "5.0.3", | ||
| "winston": "3.13.0" | ||
| "winston": "3.13.0", | ||
| "express-rate-limit": "^8.0.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/chai": "4.3.16", |
This fix introduces these dependencies
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.0.1 | None |
Copilot is powered by AI and may make mistakes. Always verify output.
| router.post("/invite", disableRoute, authenticate, checkCanGenerateDiscordLink, generateInviteForUser); | ||
|
|
||
| router.get("/invite", authenticate, getUserDiscordInvite); | ||
| router.post("/invite", authenticate, checkCanGenerateDiscordLink, generateInviteForUser); |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To mitigate the risk of abuse on the /invite POST endpoint, a rate-limiting middleware should be introduced. The best approach is to use a well-known package such as express-rate-limit, which can easily be configured to limit the number of requests allowed per user or IP address within a given timeframe. The rate limiter should be applied specifically to the /invite POST route (line 41), ensuring minimal impact on other routes.
This involves:
- Adding an import for
express-rate-limitat the top of the file. - Defining a rate limiter instance with reasonable defaults (e.g., 5 requests per hour).
- Applying the rate limiter as middleware to the
/invitePOST route, before authentication (or immediately after, if rate-limiting should be per-user rather than per-IP).
Edits are confined to routes/discordactions.js, including the import, rate limiter definition, and the route modification.
Suggested changeset
2
routes/discordactions.js
| @@ -1,3 +1,4 @@ | ||
| const rateLimit = require("express-rate-limit"); | ||
| const express = require("express"); | ||
| const authenticate = require("../middlewares/authenticate"); | ||
| const { | ||
| @@ -33,12 +34,21 @@ | ||
| const { authorizeAndAuthenticate } = require("../middlewares/authorizeUsersAndService"); | ||
| const router = express.Router(); | ||
|
|
||
| // Rate limiter for invite generation: max 5 requests per hour per IP | ||
| const inviteRateLimiter = rateLimit({ | ||
| windowMs: 60 * 60 * 1000, // 1 hour | ||
| max: 5, // limit each IP to 5 requests per windowMs | ||
| message: { | ||
| status: 429, | ||
| error: "Too many invite requests, please try again later." | ||
| } | ||
| }); | ||
| router.post("/groups", authenticate, checkIsVerifiedDiscord, validateGroupRoleBody, createGroupRole); | ||
| router.get("/groups", authenticate, checkIsVerifiedDiscord, validateLazyLoadingParams, getPaginatedAllGroupRoles); | ||
| router.delete("/groups/:groupId", authenticate, checkIsVerifiedDiscord, authorizeRoles([SUPERUSER]), deleteGroupRole); | ||
| router.post("/roles", authenticate, checkIsVerifiedDiscord, validateMemberRoleBody, addGroupRoleToMember); | ||
| router.get("/invite", authenticate, getUserDiscordInvite); | ||
| router.post("/invite", authenticate, checkCanGenerateDiscordLink, generateInviteForUser); | ||
| router.post("/invite", inviteRateLimiter, authenticate, checkCanGenerateDiscordLink, generateInviteForUser); | ||
| router.delete("/roles", authenticate, checkIsVerifiedDiscord, deleteRole); | ||
| router.get("/roles", authenticate, checkIsVerifiedDiscord, getGroupsRoleId); | ||
| router.patch( |
package.json
Outside changed files
| @@ -42,7 +42,8 @@ | ||
| "passport-github2": "0.1.12", | ||
| "passport-google-oauth20": "^2.0.0", | ||
| "rate-limiter-flexible": "5.0.3", | ||
| "winston": "3.13.0" | ||
| "winston": "3.13.0", | ||
| "express-rate-limit": "^8.0.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/chai": "4.3.16", |
This fix introduces these dependencies
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.0.1 | None |
Copilot is powered by AI and may make mistakes. Always verify output.
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Unsafe Authorization Header Access ▹ view | |
 |
Incomplete Security Feature Documentation ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| constants/bot.ts | ✅ |
| services/botVerificationService.js | ✅ |
| services/authService.js | ✅ |
| types/oooRequest.d.ts | ✅ |
| middlewares/authorizeBot.js | ✅ |
| routes/index.ts | ✅ |
| routes/discordactions.js | ✅ |
| middlewares/validators/oooRequests.ts | ✅ |
| constants/requests.ts | ✅ |
| models/requests.ts | ✅ |
| controllers/requests.ts | ✅ |
| middlewares/validators/requests.ts | ✅ |
| middlewares/authenticate.js | ✅ |
| controllers/oooRequests.ts | ✅ |
| services/oooRequest.ts | ✅ |
| models/tasks.js | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
Comment on lines
50
to
52
| if (process.env.NODE_ENV !== "production" && !token) { | ||
| token = req.headers.authorization?.split(" ")[1]; | ||
| token = req.headers.authorization.split(" ")[1]; | ||
| } |
There was a problem hiding this comment.
Unsafe Authorization Header Access 
Tell me more
What 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 matters
If authorization header is undefined, attempting to call split() on it will throw a TypeError and crash the application.
Suggested change ∙ Feature Preview
Add 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.
Comment on lines
46
to
49
| /** | ||
| * 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 | ||
| */ |
There was a problem hiding this comment.
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
/**
- Enable Bearer Token authentication for NON-PRODUCTION environments only.
- This is enabled as Swagger UI does not support cookie auth.
- Note: This is less secure than cookie-based auth and should never be enabled in production.
*/
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Date:
Developer Name:
Issue Ticket Number
Description
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
Test Coverage
Screenshot 1
Additional Notes
Description by Korbit AI
What change is being made?
Implement approve/reject functionality for Out-of-Office (OOO) requests.
Why are these changes being made?
The changes introduce a new capability to handle OOO requests more effectively by allowing them to be approved or rejected. This includes adding necessary validations, updating necessary services, and removing unused and obsolete code related to impersonation and Discord services. The approval process is enhanced to ensure superuser roles can only approve requests, and proper logging and error handling are included for better traceability.