refactor: platform managed user org admin access #22597
Conversation
WalkthroughThis change refactors organization admin and owner permissions for managed users across several modules. It introduces new service and repository methods to check for organization admin/owner roles, enables organization admins to bypass team membership checks when accessing or updating team event types, and modifies endpoints and authorization logic to allow organization admins to view all organization teams and manage team event types. Helper functions and test cases are added to support and verify these behaviors. The changes are focused on backend service, repository, controller, and API handler layers, with some adjustments in example platform code. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ 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)
✨ Finishing Touches🧪 Generate unit tests
🪧 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 (
|
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (07/17/25)1 reviewer was added to this PR based on Keith Williams's automation. |
|
✅ No security or compliance issues detected. Reviewed everything up to 276b033. Security Overview
Detected Code Changes
Reply to this PR with |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/platform/examples/base/src/pages/_app.tsx (1)
16-27: Consider making the names configurable for better maintainability.The implementation correctly generates predictable emails with name prefixes. However, the hardcoded names could be made more flexible.
Consider extracting the names to a constant or configuration:
+const DEMO_NAMES = ["keith", "somay", "rajiv", "morgan", "lauris"]; + function generateRandomEmail(name: string) { const localPartLength = 5; const domain = ["example.com", "example.net", "example.org"]; // ... rest of implementation }This would make it easier to modify the demo users without changing the function calls.
apps/api/v2/src/modules/atoms/services/event-types-atom.service.ts (1)
78-99: Verify security implications of context switching for organization admins.This change allows organization admins to operate in the context of the event type's owner rather than their own context. While this aligns with the PR's goal of enhancing org admin access, it's important to ensure:
- This doesn't bypass any critical security checks
- Audit logs properly reflect who made changes (the admin, not the effective user)
- The behavior is clearly documented for API consumers
The logic is sound, but the security implications should be carefully considered.
Consider adding audit logging to track when an org admin operates on behalf of another user. This would help with compliance and debugging.
packages/platform/examples/base/src/pages/api/managed-user.ts (1)
68-75: Use optional chaining for cleaner code.The static analysis correctly identifies an opportunity to use optional chaining.
- if (existingUser && existingUser.calcomUserId) { + if (existingUser?.calcomUserId) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/api/v2/src/modules/atoms/services/event-types-atom.service.ts(3 hunks)apps/api/v2/src/modules/organizations/memberships/services/organizations-membership.service.ts(1 hunks)apps/api/v2/src/modules/organizations/teams/index/organizations-teams.controller.ts(3 hunks)apps/api/v2/src/modules/organizations/teams/index/organizations-teams.repository.ts(1 hunks)apps/api/v2/src/modules/organizations/teams/index/services/organizations-teams.service.ts(1 hunks)packages/platform/examples/base/src/pages/_app.tsx(3 hunks)packages/platform/examples/base/src/pages/api/managed-user.ts(4 hunks)packages/trpc/server/routers/viewer/eventTypes/create.handler.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
apps/api/v2/src/modules/organizations/teams/index/organizations-teams.controller.ts (1)
apps/api/v2/src/modules/organizations/memberships/services/organizations-membership.service.ts (1)
isOrgAdminOrOwner(30-41)
apps/api/v2/src/modules/atoms/services/event-types-atom.service.ts (2)
apps/api/v2/src/modules/memberships/memberships.repository.ts (1)
isUserOrganizationAdmin(50-61)packages/lib/event-types/getEventTypeById.ts (1)
getEventTypeById(36-260)
packages/platform/examples/base/src/pages/api/managed-user.ts (1)
packages/platform/constants/api.ts (2)
X_CAL_SECRET_KEY(49-49)X_CAL_CLIENT_ID(50-50)
🪛 Biome (1.9.4)
packages/platform/examples/base/src/pages/api/managed-user.ts
[error] 68-68: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ 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: Check for E2E label
- GitHub Check: Security Check
🔇 Additional comments (7)
apps/api/v2/src/modules/organizations/memberships/services/organizations-membership.service.ts (1)
30-41: LGTM! Well-implemented role checking method.The
isOrgAdminOrOwnermethod correctly implements role-based access control by:
- Reusing existing repository method for consistency
- Providing clear error handling with descriptive messages
- Following the established service pattern
- Using proper return type for boolean checks
This method will enable proper authorization checks across the organization modules.
packages/platform/examples/base/src/pages/_app.tsx (1)
53-57: Email generation calls are consistent with the refactored function.The updated calls to
generateRandomEmailwith specific names align well with the function signature change and support the demo user creation workflow.apps/api/v2/src/modules/organizations/teams/index/services/organizations-teams.service.ts (1)
27-34: LGTM! Method follows established service patterns.The
getPaginatedOrgTeamsWithMembersmethod correctly:
- Maintains consistency with existing service methods
- Uses proper pagination parameters with sensible defaults
- Delegates to the repository layer appropriately
- Supports the role-based access control requirements
This method enables organization admins/owners to retrieve all teams with member information.
packages/trpc/server/routers/viewer/eventTypes/create.handler.ts (1)
92-93: LGTM! Authorization logic correctly exempts organization admins.The updated condition properly implements the organization admin privilege by:
- Adding
!isOrgAdmincheck to bypass membership role requirements- Maintaining existing system admin and membership role validations
- Following logical precedence: system admin → org admin → membership role
This change aligns with the PR objective of refactoring platform managed user org admin access.
apps/api/v2/src/modules/organizations/teams/index/organizations-teams.repository.ts (1)
111-122: LGTM! Repository method efficiently retrieves teams with member data.The
findOrgTeamsPaginatedWithMembersmethod is well-implemented:
- Correctly filters teams by organization ID
- Uses selective field inclusion for members (accepted, userId, role) to optimize performance
- Applies pagination properly with skip/take parameters
- Follows established repository patterns
- Supports the role-based access control requirements
The selective member field inclusion is particularly good for performance while providing necessary data for authorization checks.
apps/api/v2/src/modules/organizations/teams/index/organizations-teams.controller.ts (1)
60-63: LGTM! Proper dependency injection.The OrganizationsMembershipService is correctly injected following NestJS conventions.
packages/platform/examples/base/src/pages/api/managed-user.ts (1)
15-56: Excellent refactoring to reduce code duplication!The helper function properly consolidates the user creation logic and makes the code more maintainable.
apps/api/v2/src/modules/organizations/teams/index/organizations-teams.controller.ts
Show resolved
Hide resolved
apps/api/v2/src/modules/atoms/services/event-types-atom.service.ts
Outdated
Show resolved
Hide resolved
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
…er-admin-permissions
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/lib/server/repository/eventTypeRepository.ts (2)
751-764: Optimize the organization users query for better performanceThe nested query with
profiles.somecould impact performance. Consider adding database indexes and potentially restructuring the query.
Add indexes to improve query performance:
- Index on
Profile.organizationId- Index on
Team.parentId- Composite index on
EventType.userIdandEventType.teamIdConsider denormalizing the organization relationship on the EventType table for faster queries if this becomes a performance bottleneck.
Would you like me to generate the Prisma schema changes for these indexes?
749-777: Extract organization admin conditions into a separate methodConsider extracting the organization admin condition building logic into a separate private method for better readability and testability.
+ private buildOrganizationAdminConditions(currentOrganizationId: number) { + const organizationUsersEventTypesQuery = { + AND: [ + { userId: { not: null } }, + { + owner: { + profiles: { + some: { + organizationId: currentOrganizationId, + }, + }, + }, + }, + ], + }; + + const organizationTeamsEventTypesQuery = { + AND: [ + { teamId: { not: null } }, + { + team: { + parentId: currentOrganizationId, + }, + }, + ], + }; + + return [organizationUsersEventTypesQuery, organizationTeamsEventTypesQuery]; + } const orgAdminConditions = []; if (isUserOrganizationAdmin && currentOrganizationId) { - const organizationUsersEventTypesQuery = { - AND: [ - { userId: { not: null } }, - { - owner: { - profiles: { - some: { - organizationId: currentOrganizationId, - }, - }, - }, - }, - ], - }; - const organizationTeamsEventTypesQuery = { - AND: [ - { teamId: { not: null } }, - { - team: { - parentId: currentOrganizationId, - }, - }, - ], - }; - orgAdminConditions.push(organizationUsersEventTypesQuery); - orgAdminConditions.push(organizationTeamsEventTypesQuery); + orgAdminConditions.push(...this.buildOrganizationAdminConditions(currentOrganizationId)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/lib/event-types/getEventTypeById.ts(1 hunks)packages/lib/server/repository/eventTypeRepository.ts(2 hunks)packages/trpc/server/routers/viewer/eventTypes/create.handler.ts(1 hunks)
📓 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/lib/server/repository/eventTypeRepository.ts
🧬 Code Graph Analysis (1)
packages/lib/server/repository/eventTypeRepository.ts (1)
apps/api/v2/src/modules/memberships/memberships.repository.ts (1)
isUserOrganizationAdmin(50-61)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/lib/event-types/getEventTypeById.ts
- packages/trpc/server/routers/viewer/eventTypes/create.handler.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/lib/server/repository/eventTypeRepository.ts
🧬 Code Graph Analysis (1)
packages/lib/server/repository/eventTypeRepository.ts (1)
apps/api/v2/src/modules/memberships/memberships.repository.ts (1)
isUserOrganizationAdmin(50-61)
⏰ 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: Install dependencies / Yarn install & cache
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (1)
packages/lib/server/repository/eventTypeRepository.ts (1)
471-481: Ignore admin‐status verification in the repository
ThefindByIdmethod is a data-access layer API that intentionally trusts theisUserOrganizationAdminflag provided by its caller. Authorization (including any necessaryMembershipRepository.isUserOrganizationAdminchecks) is performed in the service or controller layer before invoking this repository method. You can safely ignore the previous suggestion to re-verify admin status insidefindById.Likely an incorrect or invalid review comment.
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 (2)
packages/lib/event-types/getEventTypeById.test.ts(1 hunks)packages/lib/event-types/getEventTypeById.ts(2 hunks)
📓 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/lib/event-types/getEventTypeById.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/lib/event-types/getEventTypeById.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/lib/event-types/getEventTypeById.test.ts
⏰ 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: Install dependencies / Yarn install & cache
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (6)
packages/lib/event-types/getEventTypeById.test.ts (6)
1-16: LGTM! Well-structured test setup.The imports and test setup are properly configured with appropriate mocking for database operations and translations.
19-54: LGTM! Comprehensive test for basic user access.The test properly sets up user ownership and verifies that users can access their own event types with appropriate assertions.
113-165: LGTM! Good test coverage for organization admin team access.The test properly demonstrates the new permission model where organization admins can access team event types within their organization hierarchy.
167-228: LGTM! Comprehensive test for organization admin access to user event types.The test properly verifies that organization admins can access event types owned by users within their organization, with appropriate user profile setup.
230-288: LGTM! Critical security test for cross-organization access control.This test properly verifies that organization admins cannot access event types from different organizations, which is essential for multi-tenant security.
290-325: LGTM! Good edge case handling for fallback behavior.The test properly verifies graceful degradation to regular user permissions when organization context is inconsistent, ensuring robust error handling.
| test.skip("should return null when user doesn't have access to event type", async () => { | ||
| // note(Lauris): test skipped because somehow when creating event type eventType.users includes otherUser | ||
| const owner = await prismock.user.create({ | ||
| data: { | ||
| username: "owner", | ||
| email: "owner1@example.com", | ||
| }, | ||
| }); | ||
|
|
||
| const otherUser = await prismock.user.create({ | ||
| data: { | ||
| username: "otheruser", | ||
| email: "otheruser@example.com", | ||
| }, | ||
| }); | ||
|
|
||
| const eventType = await prismock.eventType.create({ | ||
| data: { | ||
| title: "Owner's Event Type", | ||
| slug: "owner-event", | ||
| length: 30, | ||
| userId: owner.id, | ||
| users: { | ||
| connect: [{ id: owner.id }], | ||
| }, | ||
| }, | ||
| select: { | ||
| id: true, | ||
| userId: true, | ||
| users: true, | ||
| }, | ||
| }); | ||
|
|
||
| await prismock.user.update({ | ||
| where: { | ||
| id: otherUser.id, | ||
| }, | ||
| data: { | ||
| eventTypes: { | ||
| disconnect: [{ id: eventType.id }], | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| const result = await getRawEventType({ | ||
| userId: otherUser.id, | ||
| eventTypeId: eventType.id, | ||
| isUserOrganizationAdmin: false, | ||
| currentOrganizationId: null, | ||
| prisma: prismock as any, | ||
| }); | ||
|
|
||
| expect(result).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address the skipped security test.
This test covers an important security scenario (access denial for non-owners) but is disabled due to prismock limitations. Consider using a different approach to enable this test.
Possible solutions:
- Use a real test database with proper cleanup
- Try a different mocking approach that handles user-eventType associations correctly
- Mock the repository layer directly instead of the database
This test gap could miss authorization vulnerabilities in production.
🤖 Prompt for AI Agents
In packages/lib/event-types/getEventTypeById.test.ts between lines 56 and 109,
the test verifying access denial for non-owners is skipped due to prismock not
correctly handling user-eventType associations. To fix this, replace prismock
with a real test database setup that supports proper user-eventType relations
and includes cleanup after tests, or alternatively mock the repository layer
directly to simulate these associations accurately. This will enable the test to
run and properly verify the security scenario.
…er-admin-permissions
…er-admin-permissions
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Linear CAL-6118