feat: enhance Discord invite generation with application-specific handling#2579
Merged
iamitprakash merged 2 commits intodevelopfrom Feb 24, 2026
Merged
feat: enhance Discord invite generation with application-specific handling#2579iamitprakash merged 2 commits intodevelopfrom
iamitprakash merged 2 commits intodevelopfrom
Conversation
…dling - Added applicationId and role validation in the invite generation process. - Updated the invite retrieval method to support application-scoped invites. - Enhanced middleware to pass applicationId to the request object. - Implemented tests for application-specific invite generation scenarios.
WalkthroughThis pull request extends Discord invite functionality to be application-scoped. Changes include modifying the controller to extract and validate application identifiers, implementing a new model function to fetch invites by both user and application, updating the middleware to populate the application ID in the request, extending TypeScript type definitions, and adding corresponding test coverage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controllers/discordactions.js (1)
497-503:⚠️ Potential issue | 🟡 MinorTOCTOU race condition between existence check and invite creation.
Two concurrent requests from the same user+application can both pass the
getUserDiscordInviteByApplicationcheck before either commits toaddInviteToInviteModel, resulting in duplicate invite documents. Since Firestore doesn't offer row-level locks, the safest mitigation is wrapping both operations in a Firestore transaction, or alternatively accepting the low probability and adding a unique constraint at the data layer (e.g., a dedicated document keyed byuserId_applicationId).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/discordactions.js` around lines 497 - 503, The current two-step check using discordRolesModel.getUserDiscordInviteByApplication followed by addInviteToInviteModel introduces a TOCTOU race; modify the logic so existence-check and create happen atomically: either (1) perform both operations inside a Firestore transaction (use the same firestore instance and transaction.get on the invite query/document then transaction.create/update to add the invite) around getUserDiscordInviteByApplication/addInviteToInviteModel, or (2) switch to a single write that fails if a sentinel/unique document keyed by `${userId}_${applicationId}` already exists (create that document with a deterministic ID and treat create failure as “already exists”); change the code paths that call getUserDiscordInviteByApplication and addInviteToInviteModel to use the chosen atomic approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controllers/discordactions.js`:
- Around line 491-494: The new guard blocks super users because
checkCanGenerateDiscordLink short-circuits without setting
req.approvedApplicationId/req.approvedApplicationRole, so update the controller
around the applicationId/role check to handle the super-user path separately:
detect super users (e.g., req.user.isSuperUser or the same flag used by
checkCanGenerateDiscordLink) and either accept applicationId from
req.query/req.body for super users or skip the application-scoped guard for
them; keep the existing behavior for non-super users by continuing to read
req.approvedApplicationId and req.approvedApplicationRole and returning 403 only
when those are missing for non-super users.
In `@models/discordactions.js`:
- Around line 1269-1273: The compound query using
discordInvitesModel.where("userId","==", userId).where("applicationId","==",
applicationId) requires a composite Firestore index to avoid inefficient
index-merge scans; add a composite index entry in firestore.indexes.json for the
collection "discord-invites" with fields userId ASC and applicationId ASC so the
query in models/discordactions.js (the discordInvitesModel compound equality
query) uses the dedicated composite index.
- Line 1280: Replace the incorrect logger call in the catch block that currently
uses logger.log("error in getting user invite by application", err) with
logger.error so the error is recorded at the proper severity; locate the catch
handling around the code that fetches user invites (search for the string "error
in getting user invite by application" or the surrounding function where user
invites are retrieved) and change logger.log to logger.error while keeping the
same message and error variable.
In `@test/integration/discordactions.test.js`:
- Around line 1176-1197: Add integration tests to cover the successful and
missing-application cases for the application-scoped POST
/discord-actions/invite flow: add a test that stubs
ApplicationModel.getUserApplications to return the accepted app, stubs
discordRolesModel.getUserDiscordInviteByApplication to return notFound: true and
stubs discordRolesModel.createDiscordInvite (or the method used to create
invites) to resolve with an object containing inviteLink, then assert the
endpoint returns 201 and res.body.inviteLink; also add a test that simulates
ApplicationModel.getUserApplications returning no application (or missing
application data) and assert the endpoint returns 403 with message "Application
data is required". Use the same test harness variables (app, userAuthToken,
cookieName) and the existing describe "POST /discord-actions/invite
(application-scoped)" to place these tests.
In `@test/unit/models/discordactions.test.js`:
- Around line 894-911: The test assumes getUserDiscordInvite returns the latest
invite by createdAt but the implementation (getUserDiscordInvite in
models/discordactions.js) does a plain where("userId","==",userId).get() with no
ordering; either update getUserDiscordInvite to explicitly order and limit
(e.g., append .orderBy("createdAt","desc").limit(1) to the query and return that
single doc) if the intended behavior is "latest", or remove/adjust the unit test
to not assert a specific inviteLink and instead test only deterministic behavior
(such as using getUserDiscordInviteByApplication or checking notFound). Ensure
you change only the function/query or the test assertion so behavior and tests
align.
- Around line 914-942: The new tests in the describe blocks
getUserDiscordInviteByApplication and the recently-added test in
getUserDiscordInvite create Firestore documents but lack cleanup; add an
afterEach(() => cleanDb()) inside each affected describe block (or move the new
test into its own nested describe with afterEach) so cleanDb() is run after
every test; target the describe blocks named "getUserDiscordInviteByApplication"
and "getUserDiscordInvite" and ensure afterEach invokes the existing cleanDb
helper.
---
Outside diff comments:
In `@controllers/discordactions.js`:
- Around line 497-503: The current two-step check using
discordRolesModel.getUserDiscordInviteByApplication followed by
addInviteToInviteModel introduces a TOCTOU race; modify the logic so
existence-check and create happen atomically: either (1) perform both operations
inside a Firestore transaction (use the same firestore instance and
transaction.get on the invite query/document then transaction.create/update to
add the invite) around getUserDiscordInviteByApplication/addInviteToInviteModel,
or (2) switch to a single write that fails if a sentinel/unique document keyed
by `${userId}_${applicationId}` already exists (create that document with a
deterministic ID and treat create failure as “already exists”); change the code
paths that call getUserDiscordInviteByApplication and addInviteToInviteModel to
use the chosen atomic approach.
MayankBansal12
approved these changes
Feb 23, 2026
iamitprakash
approved these changes
Feb 24, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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: 22 feb, 2026
Developer Name: @AnujChhikara
Issue Ticket Number
Description
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
screen-recording-2026-02-23-at-95438-am_8zl6qODr.mp4
Test Coverage
Screenshot 1
Additional Notes