fix: missing authorization check in getAgendaPacingReport (#884) - #892
Conversation
…imuniqueshiv#384) Enforces organization visibility checks, invitation validation, duplicate membership prevention, and pending request checks when joining an organization.
…rval (imuniqueshiv#543) Extends assignment expiration time window from 24 hours to 48 hours and adjusts reminder comments to fire every 8 hours instead of every 6 hours.
|
@Pratyush-Panda-2006 is attempting to deploy a commit to the Shiv Raj Singh's projects Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
Bot
added
the
ECSoC26
|
Hi @Pratyush-Panda-2006, thank you so much for your PR! 🎉 📋 PR Review Checklist
We appreciate the effort you've put in — let us know if you need any help getting this across the finish line! 💪 |
📝 WalkthroughWalkthroughThe PR standardizes activity organization resolution, adds organization-aware authorization for agenda timers and pacing reports, applies organization membership middleware to activity routes, and adds controller tests for activity, agenda authorization, and organization joining. ChangesActivity organization context
Agenda authorization
Organization join security coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/tests/agendaTimerController.test.js (1)
1-222: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPrettier check is failing CI for this file.
Run
npx prettier --write server/tests/agendaTimerController.test.jsbefore merge; long single-line object literals (e.g. Line 11-12 destructuring, and the repeated inlineuser: {...}objects) are likely the culprits.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/tests/agendaTimerController.test.js` around lines 1 - 222, Run Prettier on the agendaTimerController authorization test file and commit the resulting formatting changes, especially for the import destructuring and repeated inline user objects. Preserve all test behavior and assertions.Source: Pipeline failures
🧹 Nitpick comments (2)
server/controllers/agendaTimerController.js (1)
3-16: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConfirm whether plain org members should be able to view the pacing report.
hasPermissionis shared by manage-timer actions and the newgetAgendaPacingReportguard. Issue#884says a user should be authorized if they "belong to the meeting's organization or otherwise has permission to view the meeting," but the current check rejects any same-org user who isn't the uploader or an admin/owner (confirmed by the "same org, non-admin" test inagendaTimerController.test.js). If viewing should be less restrictive than managing timers, consider splitting this into distinctcanManageAgenda/canViewAgendachecks.Also applies to: 215-219
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/controllers/agendaTimerController.js` around lines 3 - 16, Split the shared hasPermission logic into distinct canManageAgenda and canViewAgenda checks, and use canViewAgenda for getAgendaPacingReport. Ensure same-organization members are authorized to view the pacing report while preserving the stricter uploader/admin/owner requirements for manage-timer actions.server/tests/agendaTimerController.test.js (1)
181-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding admin/owner and cross-org cases for
getAgendaPacingReport.Only uploader-success and same-org-member-reject are tested here; the admin/owner-success and different-org-reject paths are only exercised indirectly via the timer-action tests. Since this endpoint is the direct target of issue
#884, dedicated coverage would increase confidence, especially given thehasPermissionuploader-check edge case flagged in the controller review.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/tests/agendaTimerController.test.js` around lines 181 - 220, Extend the getAgendaPacingReport authorization tests with direct cases for an admin or owner from the meeting’s organization succeeding and a user from a different organization receiving 403. Reuse the existing meeting and mockFindById setup, and verify the same success payload and “Not authorized to view agenda report” response used by the current tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/controllers/agendaTimerController.js`:
- Around line 6-7: Update the uploader authorization check in the isUploader
assignment to require both meeting.uploadedBy and userIdStr to be present before
comparing their string values, preventing an undefined-to-undefined match from
granting access.
In `@server/tests/activityController.test.js`:
- Around line 1-120: Run the repository’s Prettier formatter on the
activityController Jest suite and apply its formatting changes without altering
the test behavior or assertions, so format:check:changed passes.
In `@server/tests/organizationJoinSecurity.test.js`:
- Line 83: Run the repository-configured Prettier formatter on
organizationJoinSecurity.test.js and retain the formatter’s wrapped output for
the affected test declarations at the referenced locations, ensuring
format:check:changed passes.
- Around line 220-234: The test fixture around the accepted-invitation flow
should match the actual Invitation.findOne({ token }) lookup and invitee email
validation. Update the mocks and assertions in the relevant test to verify the
invitation token and email comparison, rather than relying only on save-related
behavior; do not add invitationToken handling to createOrJoinOrganization.
---
Outside diff comments:
In `@server/tests/agendaTimerController.test.js`:
- Around line 1-222: Run Prettier on the agendaTimerController authorization
test file and commit the resulting formatting changes, especially for the import
destructuring and repeated inline user objects. Preserve all test behavior and
assertions.
---
Nitpick comments:
In `@server/controllers/agendaTimerController.js`:
- Around line 3-16: Split the shared hasPermission logic into distinct
canManageAgenda and canViewAgenda checks, and use canViewAgenda for
getAgendaPacingReport. Ensure same-organization members are authorized to view
the pacing report while preserving the stricter uploader/admin/owner
requirements for manage-timer actions.
In `@server/tests/agendaTimerController.test.js`:
- Around line 181-220: Extend the getAgendaPacingReport authorization tests with
direct cases for an admin or owner from the meeting’s organization succeeding
and a user from a different organization receiving 403. Reuse the existing
meeting and mockFindById setup, and verify the same success payload and “Not
authorized to view agenda report” response used by the current tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6099acd2-bc69-4faf-bfe7-bd7a2cee8b3a
📒 Files selected for processing (7)
client/src/pages/ActivityFeed.jsxserver/controllers/activityController.jsserver/controllers/agendaTimerController.jsserver/routes/activityRoutes.jsserver/tests/activityController.test.jsserver/tests/agendaTimerController.test.jsserver/tests/organizationJoinSecurity.test.js
| const userIdStr = (user._id || user.id)?.toString(); | ||
| const isUploader = meeting.uploadedBy?.toString() === userIdStr; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Uploader check can bypass auth via undefined === undefined.
meeting.uploadedBy?.toString() === userIdStr has no truthiness guard, unlike the org check on Lines 9-13 which explicitly verifies both ids are present. If meeting.uploadedBy is unset and the caller's _id/id are both absent, both sides evaluate to undefined and the comparison incorrectly returns true, granting access.
🔒 Proposed fix
- const isUploader = meeting.uploadedBy?.toString() === userIdStr;
+ const isUploader =
+ !!meeting.uploadedBy && !!userIdStr &&
+ meeting.uploadedBy.toString() === userIdStr;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const userIdStr = (user._id || user.id)?.toString(); | |
| const isUploader = meeting.uploadedBy?.toString() === userIdStr; | |
| const userIdStr = (user._id || user.id)?.toString(); | |
| const isUploader = | |
| !!meeting.uploadedBy && !!userIdStr && | |
| meeting.uploadedBy.toString() === userIdStr; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/controllers/agendaTimerController.js` around lines 6 - 7, Update the
uploader authorization check in the isUploader assignment to require both
meeting.uploadedBy and userIdStr to be present before comparing their string
values, preventing an undefined-to-undefined match from granting access.
| import { jest } from "@jest/globals"; | ||
|
|
||
| const mockGetOrgActivities = jest.fn(); | ||
| const mockGetActivityStats = jest.fn(); | ||
|
|
||
| jest.unstable_mockModule("../services/activityService.js", () => ({ | ||
| getOrgActivities: mockGetOrgActivities, | ||
| getActivityStats: mockGetActivityStats, | ||
| })); | ||
|
|
||
| const { getActivities, getActivityStats } = await import( | ||
| "../controllers/activityController.js" | ||
| ); | ||
|
|
||
| describe("activityController - organization resolution (#812)", () => { | ||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| describe("getActivities", () => { | ||
| it("should resolve organization using req.user.organization and return activities", async () => { | ||
| const req = { | ||
| user: { | ||
| _id: "user_123", | ||
| organization: "org_456", | ||
| }, | ||
| query: { page: "1", limit: "10" }, | ||
| }; | ||
| const res = { | ||
| status: jest.fn().mockReturnThis(), | ||
| json: jest.fn(), | ||
| }; | ||
|
|
||
| const mockResult = { | ||
| activities: [{ _id: "act_1", action: "meeting.created" }], | ||
| totalActivities: 1, | ||
| totalPages: 1, | ||
| currentPage: 1, | ||
| }; | ||
| mockGetOrgActivities.mockResolvedValue(mockResult); | ||
|
|
||
| await getActivities(req, res); | ||
|
|
||
| expect(mockGetOrgActivities).toHaveBeenCalledWith("org_456", { | ||
| page: 1, | ||
| limit: 10, | ||
| action: undefined, | ||
| actor: undefined, | ||
| }); | ||
| expect(res.status).toHaveBeenCalledWith(200); | ||
| expect(res.json).toHaveBeenCalledWith(mockResult); | ||
| }); | ||
|
|
||
| it("should return 400 when user has no organization set", async () => { | ||
| const req = { | ||
| user: { | ||
| _id: "user_123", | ||
| organization: null, | ||
| }, | ||
| query: {}, | ||
| }; | ||
| const res = { | ||
| status: jest.fn().mockReturnThis(), | ||
| json: jest.fn(), | ||
| }; | ||
|
|
||
| await getActivities(req, res); | ||
|
|
||
| expect(res.status).toHaveBeenCalledWith(400); | ||
| expect(res.json).toHaveBeenCalledWith({ | ||
| error: "No organization selected.", | ||
| }); | ||
| expect(mockGetOrgActivities).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("getActivityStats", () => { | ||
| it("should resolve organization using req.user.organization and return stats", async () => { | ||
| const req = { | ||
| user: { | ||
| _id: "user_123", | ||
| organization: "org_789", | ||
| }, | ||
| }; | ||
| const res = { | ||
| status: jest.fn().mockReturnThis(), | ||
| json: jest.fn(), | ||
| }; | ||
|
|
||
| const mockStats = { totalActivities: 42, activeUsers: 5 }; | ||
| mockGetActivityStats.mockResolvedValue(mockStats); | ||
|
|
||
| await getActivityStats(req, res); | ||
|
|
||
| expect(mockGetActivityStats).toHaveBeenCalledWith("org_789"); | ||
| expect(res.status).toHaveBeenCalledWith(200); | ||
| expect(res.json).toHaveBeenCalledWith(mockStats); | ||
| }); | ||
|
|
||
| it("should return 400 when user has no organization set", async () => { | ||
| const req = { | ||
| user: { | ||
| _id: "user_123", | ||
| }, | ||
| }; | ||
| const res = { | ||
| status: jest.fn().mockReturnThis(), | ||
| json: jest.fn(), | ||
| }; | ||
|
|
||
| await getActivityStats(req, res); | ||
|
|
||
| expect(res.status).toHaveBeenCalledWith(400); | ||
| expect(res.json).toHaveBeenCalledWith({ | ||
| error: "No organization selected.", | ||
| }); | ||
| expect(mockGetActivityStats).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Format this new Jest suite with Prettier.
format:check:changed fails on this file, blocking CI. Run the repository’s Prettier formatter and commit the result.
🧰 Tools
🪛 GitHub Actions: CI Pipeline / 0_Root Prettier.txt
[error] 1-1: Prettier formatting check failed under step 'format:check:changed' (command: node scripts/validate-format-changed.mjs). Included in failed --check list: server/tests/activityController.test.js.
🪛 GitHub Actions: CI Pipeline / 2_Server Validation.txt
[error] 1-1: Prettier formatting check failed (--check). Run Prettier with --write to fix.
🪛 GitHub Actions: CI Pipeline / Root Prettier
[error] 1-1: Prettier formatting check failed for server/tests/activityController.test.js as part of the format:check:changed step. Command included: prettier --check server/tests/activityController.test.js ...
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/tests/activityController.test.js` around lines 1 - 120, Run the
repository’s Prettier formatter on the activityController Jest suite and apply
its formatting changes without altering the test behavior or assertions, so
format:check:changed passes.
Source: Pipeline failures
| }); | ||
|
|
||
| describe("Unauthenticated User", () => { | ||
| it("should return 401 for unauthenticated user in createOrJoinOrganization", async () => { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Restore required Prettier formatting.
CI currently fails format:check:changed for this file. Run the repository’s configured Prettier formatter and commit the wrapped output.
Also applies to: 96-96, 143-143, 171-171, 202-202, 251-251, 283-283
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/tests/organizationJoinSecurity.test.js` at line 83, Run the
repository-configured Prettier formatter on organizationJoinSecurity.test.js and
retain the formatter’s wrapped output for the affected test declarations at the
referenced locations, ensuring format:check:changed passes.
Source: Pipeline failures
| mockUserFindById.mockResolvedValue({ | ||
| _id: "user_123", | ||
| email: "user@example.com", | ||
| }); | ||
| mockInvitationFindOne.mockResolvedValue(mockInvitation); | ||
| mockUserFindByIdAndUpdate.mockResolvedValue(true); | ||
|
|
||
| mockUserFindById.mockReturnValue({ | ||
| populate: jest.fn().mockResolvedValue({ | ||
| _id: "user_123", | ||
| role: "member", | ||
| organization: mockPrivateOrg, | ||
| _doc: { name: "User 123" }, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'organizationJoinSecurity.test.js' . || true
echo "== file outline =="
ast-grep outline server/tests/organizationJoinSecurity.test.js --view summary || true
echo "== relevant lines =="
sed -n '180,260p' server/tests/organizationJoinSecurity.test.js | cat -n -v
echo "== mocks/usages around org join tests =="
rg -n "mockInvitationFindOne|mockUserFindByIdAndUpdate|mockUserFindById|invitation|invitee|organization" server/tests/organizationJoinSecurity.test.js
echo "== backend source candidates =="
rg -n "invitee|invitationFindOne|User.findByIdAndUpdate|organization" server -g '*.js' -g '*.ts' | head -200Repository: imuniqueshiv/MeetOnMemory
Length of output: 24524
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== organizationController excerpt =="
fd -a 'organizationController.js' . | xargs -r -n1 sh -c 'echo "--- $0"; wc -l "$0"; sed -n "1,260p" "$0"'
echo "== invitation model =="
fd -a 'invitationModel.js' . | xargs -r -n1 sh -c 'echo "--- $0"; cat -n "$0"'
echo "== invitation service relevant excerpts =="
fd -a 'InvitationService.js' . | xargs -r -n1 sh -c 'echo "--- $0"; sed -n "210,360p" "$0" | cat -n'Repository: imuniqueshiv/MeetOnMemory
Length of output: 14553
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== organization service create or join excerpts =="
fd -a 'OrganizationService.js' . | xargs -r -n1 sh -c 'echo "--- $0"; wc -l "$0"; sed -n "1,260p" "$0" | cat -n'
echo "== static verifier: test mock setup vs invitation filter behavior =="
python3 - <<'PY'
from pathlib import Path
import re
test = Path("server/tests/organizationJoinSecurity.test.js").read_text()
impl = re.search(r"export const acceptInvitation = async \(userId, token\) \{(?P<body>.*?)(?=\n\});\n\*\* ✅ Reject Invitation", Path("server/services/InvitationService.js").read_text(), re.S)
join_test = re.search(r"it\(\"should allow joining private org when user has a valid pending invitation\".*?expect\(res\.json\)", test, re.S)
check_passes_with_arbitrary_invitation = (
re.search(r"mockInvitationFindOne\.mockResolvedValue\(mockInvitation\)", test) is not None
and re.search(r"mockInvitation\.status\)\.toBe\(\"accepted\"\)", join_test) is not None
and re.search(r"mockInvitation\.save\)\.toHaveBeenCalled\(\)", join_test) is not None
and re.search(r"expect\([^)]*\.toHaveBeenCalledWith\([^)]*invitationToken[^)]*\)", join_test) is None
)
impl_body = impl.group("body") if impl else ""
uses_findone_token_only = re.search(r"Invitation\.findOne\(\{\s*token\s*\}\)", impl_body) is not None
asserts_invitee_or_org = bool(re.search(r"invitation\.(email|organization)\s*([=!]=|<)", impl_body) or re.search(r"invitation\.(email|organization)\s*\?\?", impl_body))
print({"check_passes_with_arbitrary_invitation": check_passes_with_arbitrary_invitation})
print({"uses_findone_token_only": uses_findone_token_only})
print({"does_impl_assert_invitee_or_org": asserts_invitee_or_org})
print({"token": re.search(r"mockInvitation=([^}]+token)', 'token') is not None})
PYRepository: imuniqueshiv/MeetOnMemory
Length of output: 10040
Align this test with the actual invitation lookup.
The accepted-invitation flow uses Invitation.findOne({ token }), then checks the invitee by email; createOrJoinOrganization does not pass invitationToken to any service. Make this fixture assert the token lookup and invitee email comparison instead of only save() being called.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/tests/organizationJoinSecurity.test.js` around lines 220 - 234, The
test fixture around the accepted-invitation flow should match the actual
Invitation.findOne({ token }) lookup and invitee email validation. Update the
mocks and assertions in the relevant test to verify the invitation token and
email comparison, rather than relying only on save-related behavior; do not add
invitationToken handling to createOrJoinOrganization.
|
@Pratyush-Panda-2006 Could you please clear all the checks? |
2 participants
Closes #884
hasPermission(meeting, req.user)check togetAgendaPacingReportinserver/controllers/agendaTimerController.js.Summary by CodeRabbit
New Features
Bug Fixes
Tests