feat: orgs conferencing apps #22211
feat: orgs conferencing apps #22211
Conversation
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (07/02/25)1 reviewer was added to this PR based on Keith Williams's automation. |
|
✅ No security or compliance issues detected. Reviewed everything up to b270660. Security Overview
Detected Code Changes
Reply to this PR with |
There was a problem hiding this comment.
cubic found 9 issues across 22 files. Review them in cubic.dev
React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai to give specific feedback.
| @@ -9,13 +9,15 @@ import http from "../../../lib/http"; | |||
|
|
|||
| export const QUERY_KEY = "get-installed-conferencing-apps"; | |||
|
|
|||
| export const useAtomsGetInstalledConferencingApps = (teamId?: number) => { | |||
| export const useAtomsGetInstalledConferencingApps = (teamId?: number, orgId?: number) => { | |||
There was a problem hiding this comment.
The function signature now allows both teamId and orgId, but the logic does not handle the case where both are provided. This could lead to ambiguous or unintended API calls. Consider validating that only one of teamId or orgId is provided at a time.
| @@ -61,6 +61,14 @@ export async function getLocationGroupedOptions( | |||
| id: userOrTeamId.userId, | |||
| }, | |||
| }); | |||
|
|
|||
| if (user?.organizationId) { | |||
There was a problem hiding this comment.
When the user belongs to an organization this branch replaces the previous { userId } filter with { teamId: { in: [user.organizationId] } }, causing personal credentials owned by the user to be ignored. As a result users who are part of an org will no longer see their own conferencing credentials in the location selector.
apps/api/v2/src/modules/atoms/controllers/atoms.conferencing-apps.controller.ts
Show resolved
Hide resolved
apps/api/v2/src/modules/atoms/controllers/atoms.conferencing-apps.controller.ts
Outdated
Show resolved
Hide resolved
| @@ -169,6 +169,21 @@ export class ConferencingController { | |||
| const fallbackUrl = decodedCallbackState.onErrorReturnTo || ""; | |||
| return { url: fallbackUrl }; | |||
| } | |||
| } else if (decodedCallbackState.orgId) { | |||
There was a problem hiding this comment.
This new branch repeats almost the same logic as the preceding team-level branch (building the same params / headers, performing the same axios call, and handling identical success & error flows). Copy-pasting this block increases maintenance cost and risks future inconsistencies. Extract the shared logic into a reusable helper instead of duplicating it.
packages/platform/atoms/connect/conferencing-apps/hooks/useGetDefaultConferencingApp.ts
Show resolved
Hide resolved
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
This reverts commit 82db763.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/app-store/server.ts(2 hunks)packages/features/bookings/lib/getAllCredentialsForUsersOnEvent/getAllCredentials.ts(1 hunks)packages/features/eventtypes/components/Locations.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/app-store/server.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code. Functions like .add, .diff, .isBefore, and .isAfter are slow, especially in timezone mode. Prefer .utc() for better performance. Where possible, replace with native Date and direct .valueOf() comparisons for faster execution. Recommend using native methods or Day.js .utc() consistently in hot paths like loops.
Files:
packages/features/bookings/lib/getAllCredentialsForUsersOnEvent/getAllCredentials.tspackages/features/eventtypes/components/Locations.tsx
🧠 Learnings (1)
packages/features/bookings/lib/getAllCredentialsForUsersOnEvent/getAllCredentials.ts (1)
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-21T13:54:11.770Z
Learning: Applies to backend/**/*.{ts,tsx} : Make sure the credential.key field is never returned back from tRPC endpoints or APIs.
⏰ 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). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (5)
packages/features/eventtypes/components/Locations.tsx (5)
76-89: LGTM! Function signature update handles credential filtering correctly.The
getLocationFromTypefunction has been properly updated to accept an optionalcredentialIdparameter and filters location options accordingly. The logic correctly falls back to type-only matching when nocredentialIdis provided.
272-272: LGTM! Credential-aware location retrieval.The function call correctly passes the
credentialIdfrom the location field to enable proper filtering of location options.
283-286: LGTM! Enhanced option selection logic with credential awareness.The
isOptionSelectedprop implementation correctly compares bothvalueandcredentialId(when present) to determine if an option is selected. The fallback logic for options withoutcredentialIdis appropriate.
303-306: LGTM! Improved location uniqueness validation.The validation logic now correctly checks for uniqueness based on both
typeandcredentialId, preventing duplicate locations with the same credential. This ensures proper handling of multiple credentials for the same location type.
550-553: LGTM! Consistent credential-aware selection logic.The
isOptionSelectedimplementation in the empty location select component matches the logic used in the main location fields, ensuring consistent behavior across the component.
| const { profile } = await new UserRepository(prisma).enrichUserWithItsProfileSkipPlatformCheck({ | ||
| user: user, | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Clarify the security implications of skipping the platform check.
The change from enrichUserWithItsProfile to enrichUserWithItsProfileSkipPlatformCheck suggests that some platform organization validation is being bypassed. Given that this function aggregates credentials from multiple sources (user, team, org) and returns service account keys, it's important to understand:
- What specific platform check is being skipped and why?
- Are there any security implications of bypassing this check in the context of credential aggregation?
- Is this change necessary for the organization-level conferencing app management feature?
Please verify the security implications and provide documentation on why the platform check bypass is safe in this context:
🏁 Script executed:
#!/bin/bash
# Search for the implementation of both methods to understand the difference
ast-grep --pattern $'enrichUserWithItsProfile($$$) {
$$$
}'
ast-grep --pattern $'enrichUserWithItsProfileSkipPlatformCheck($$$) {
$$$
}'
# Look for any security-related comments or documentation
rg -A 5 -B 5 "enrichUserWithItsProfileSkipPlatformCheck"Length of output: 2277
Provide context & justification for skipping the platform check
The switch to enrichUserWithItsProfileSkipPlatformCheck (bypassing the platform-org validation) can inadvertently surface credentials from users in un-related orgs or expose service-account keys. Please:
- Document exactly which validation is being skipped and why it’s safe here.
- Add an inline comment in
packages/features/bookings/lib/getAllCredentialsForUsersOnEvent/getAllCredentials.ts(at the call on line 60) referencing theuser.tsdoc for future maintainers. - Confirm via tests or code review that no unauthorized credentials can be returned by this change.
Tagging user.ts for reference:
packages/lib/server/repository/user.ts
/**
* Similar to enrichUserWithItsProfile but skips the platform org check.
* This function directly returns the profile without checking if it's a platform organization.
*/
async enrichUserWithItsProfileSkipPlatformCheck...🤖 Prompt for AI Agents
In
packages/features/bookings/lib/getAllCredentialsForUsersOnEvent/getAllCredentials.ts
at line 60, add an inline comment explaining that
enrichUserWithItsProfileSkipPlatformCheck is used to bypass the platform
organization validation because in this context it is safe to do so, referencing
the detailed documentation in packages/lib/server/repository/user.ts. Clearly
state which validation is skipped and why it does not risk exposing unauthorized
credentials. Also, ensure through tests or code review that this change does not
allow retrieval of credentials from unrelated organizations or service accounts.
What does this PR do?
Visual Demo (For contributors especially)
A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).
Video Demo (if applicable):
Image Demo (if applicable):
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist
Summary by cubic
Added organization-level conferencing app management, allowing org admins to connect, list, set default, and disconnect conferencing apps for the entire organization.