-
Notifications
You must be signed in to change notification settings - Fork 279
fix(progresses): enforce discord membership for progress updates #2517
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
Conversation
- block POST /progresses when the requester is archived or lacks in_discord role - adjust test fixtures to model active members and add a regression test rejecting non-discord users
WalkthroughAuthorization logic in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
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.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
controllers/progresses.js(1 hunks)test/integration/progressesTasks.test.js(6 hunks)test/integration/progressesUsers.test.js(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/integration/progressesUsers.test.js (2)
controllers/progresses.js (3)
require(1-1)require(3-9)require(10-10)test/integration/progressesTasks.test.js (3)
require(11-15)require(19-19)withDiscordMembership(23-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (22.10.0)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
controllers/progresses.js (1)
49-51: Authorization logic correctly enforces Discord membership.The check appropriately uses optional chaining on
req.userData.rolesand strictly validates both archived status and Discord membership. This prevents archived users and non-Discord members from creating progress updates.Note: This assumes
req.userDatais always defined at this point (typically guaranteed by authentication middleware). If there's any path wherereq.userDatacould be undefined, consider adding optional chaining:req.userData?.roles?.archived.test/integration/progressesUsers.test.js (3)
45-48: Good: Test fixtures updated to reflect Discord membership requirement.Wrapping user fixtures with
withDiscordMembershipensures tests reflect the new authorization requirements while maintaining existing test behavior.
166-184: Excellent regression test for non-Discord users.The test correctly validates that users without Discord membership receive a 403 response. The unique username and github_id generation prevents fixture collisions.
193-195: Consistent test data setup across all test suites.All test users are now properly configured with Discord membership, ensuring tests accurately reflect the production authorization requirements.
Also applies to: 298-299, 370-371, 453-454
test/integration/progressesTasks.test.js (2)
49-51: Good: Intentional omission of Discord membership for archived user test.Line 50 correctly creates
archivedUserIdwithoutwithDiscordMembership, allowing the test at lines 178-192 to validate that archived or non-Discord users are properly blocked. This provides coverage for the archived status check in the authorization logic.
204-205: Consistent test fixture updates for Discord membership.All test users in GET endpoint tests are now properly configured with Discord membership, aligning with the updated authorization requirements.
Also applies to: 401-401, 477-477, 564-564
- reject POST /progresses when roles.archived is true or roles.in_discord is not true - centralize Discord-member test fixture helper and re-use it across progress integration suites - add regression test proving non-Discord users receive UNAUTHORIZED_WRITE
Date:
December 11, 2025Developer Name: @Achintya-Chatterjee
Issue Ticket Number
/progresses POSTendpoint to Discord members #2518Description
POST /progresseswhenroles.archivedistrueorroles.in_discordis not trueDocumentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
Screen.Recording.2025-12-11.at.21.26.24.mp4
Test Coverage
Screenshot 1
Additional Notes