feat: add Workflow resource to PBAC system with permission enforcement #22845
Conversation
- Add Workflow to Resource enum in permission-registry.ts - Add complete workflow CRUD permissions to PERMISSION_REGISTRY - Create migration to add workflow manage permissions to admin role - Update workflow create/update handlers to use PBAC permission checks - Add i18n keys for workflow permission descriptions - Enforce workflow.create permission for team workflow creation - Use fallback roles (ADMIN, OWNER) when PBAC is disabled Fixes workflow creation authorization for team events using PBAC Co-Authored-By: sean@cal.com <Sean@brydon.io>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
WalkthroughThis set of changes introduces a comprehensive permission system for workflow-related resources. It adds workflow-specific permission keys and descriptions to the English localization file and extends the permission registry to include workflows, defining standard CRUD and management actions. A new module implements logic for calculating user permissions on workflows, supporting both personal and team-based workflows, with caching and batch processing. The workflow-related TRPC handlers and UI components are updated to leverage these new permissions, replacing previous role-based checks with permission-based checks throughout. Schema and query interfaces are updated to support permission-aware filtering. Test coverage is added for the new authorization logic. Additionally, minor refactors improve permission check consistency and centralize authorization logic via the PermissionCheckService. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous 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). (1)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…dy exist in main migration Co-Authored-By: sean@cal.com <Sean@brydon.io>
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
E2E results are ready! |
Co-Authored-By: sean@cal.com <Sean@brydon.io>
There was a problem hiding this comment.
Builder class to easily add permissions to a workflow from the Repository OR workflows. See usage of this here https://github.com/calcom/cal.com/pull/22845/files#diff-4ab0b5800b7c3159eb32d82ee40a61e985764f2b7fb136b06407030c9e1ecd6bR67
| } | ||
|
|
||
| interface TeamPermissionsCache { | ||
| [teamId: string]: WorkflowPermissions; |
There was a problem hiding this comment.
Not a true cache. Just for this RUN of fetching workflows. we cache by teamId what permissions the user has access too instead of querying every time
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/features/ee/workflows/components/WorkflowDetailsPage.tsx (1)
26-32: Consider removing duplicate interface definition.The
WorkflowPermissionsinterface is already defined inpackages/lib/server/repository/workflow-permissions.ts(lines 4-10). Having duplicate interface definitions can lead to maintenance issues if they diverge over time.Import the interface from the existing location:
+import type { WorkflowPermissions } from "@calcom/lib/server/repository/workflow-permissions"; -interface WorkflowPermissions { - canView: boolean; - canUpdate: boolean; - canDelete: boolean; - canManage: boolean; - readOnly: boolean; // Keep for backward compatibility -}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/features/ee/workflows/components/WorkflowDetailsPage.tsx(3 hunks)packages/features/ee/workflows/pages/workflow.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/features/ee/workflows/pages/workflow.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/features/ee/workflows/components/WorkflowDetailsPage.tsx
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/ee/workflows/components/WorkflowDetailsPage.tsx
🧬 Code Graph Analysis (1)
packages/features/ee/workflows/components/WorkflowDetailsPage.tsx (1)
packages/lib/server/repository/workflow-permissions.ts (1)
WorkflowPermissions(5-11)
⏰ 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 (1)
packages/features/ee/workflows/components/WorkflowDetailsPage.tsx (1)
44-44: LGTM!The addition of the
permissionsproperty to the Props interface correctly integrates the new permission system.
packages/features/ee/workflows/components/WorkflowDetailsPage.tsx
Outdated
Show resolved
Hide resolved
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/features/ee/workflows/components/WorkflowDetailsPage.tsx(3 hunks)packages/features/ee/workflows/pages/workflow.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/features/ee/workflows/pages/workflow.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/features/ee/workflows/components/WorkflowDetailsPage.tsx
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/ee/workflows/components/WorkflowDetailsPage.tsx
🧠 Learnings (1)
📚 Learning: in the failedbookingsbyfield component (packages/features/insights/components/failedbookingsbyfield....
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
Applied to files:
packages/features/ee/workflows/components/WorkflowDetailsPage.tsx
🧬 Code Graph Analysis (1)
packages/features/ee/workflows/components/WorkflowDetailsPage.tsx (1)
packages/lib/server/repository/workflow-permissions.ts (1)
WorkflowPermissions(5-11)
🪛 Biome (2.1.2)
packages/features/ee/workflows/components/WorkflowDetailsPage.tsx
[error] 61-61: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
⏰ 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 (3)
packages/features/ee/workflows/components/WorkflowDetailsPage.tsx (3)
26-32: LGTM! Well-structured permission interface.The
WorkflowPermissionsinterface correctly defines granular permission flags and maintains backward compatibility with thereadOnlyflag. This aligns well with the broader permission model being introduced across the workflows feature.
44-44: Good addition for permission-aware rendering.The optional
permissionsproperty enables parent components to pass down computed permissions while maintaining backward compatibility through the fallback logic.
189-189: Delete button permission check is correctly implemented.The condition properly checks
permissions.canDeleteto control delete button visibility, which addresses the issue mentioned in the PR comments about unauthorized delete button access.
packages/features/ee/workflows/components/WorkflowDetailsPage.tsx
Outdated
Show resolved
Hide resolved
packages/trpc/server/routers/loggedInViewer/teamsAndUserProfilesQuery.handler.ts
Outdated
Show resolved
Hide resolved
…esQuery.handler.ts Co-authored-by: Carina Wollendorfer <30310907+CarinaWolli@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/trpc/server/routers/loggedInViewer/teamsAndUserProfilesQuery.handler.ts (2)
95-116: Critical logic error in permission filtering.This is the same issue identified in previous reviews. The permission result handling has a significant flaw:
Line 113 filters
permissionChecksto only includetruevalues, losing the positional correspondence with teams. This causes incorrectreadOnlyvalues in the returned data (lines 136-138).Apply this fix to correctly handle permission results:
// Filter teams based on permission if provided - let hasPermissionForFiltered: boolean[] = []; if (input?.withPermission) { const permissionService = new PermissionCheckService(); const { permission, fallbackRoles } = input.withPermission; const permissionChecks = await Promise.all( teamsData.map((membership) => permissionService.checkPermission({ userId: ctx.user.id, teamId: membership.team.id, permission: permission as PermissionString, fallbackRoles: fallbackRoles ? (fallbackRoles as MembershipRole[]) : [], }) ) ); - // Store permission results for teams that passed the filter - hasPermissionForFiltered = permissionChecks.filter((hasPermission) => hasPermission); teamsData = teamsData.filter((_, index) => permissionChecks[index]); }
136-138: Fix readOnly logic to handle filtered teams correctly.Since teams are now filtered based on permissions, all remaining teams have the required permission. The current logic references an incorrect index due to the filtering issue above.
Update the readOnly logic:
readOnly: input?.withPermission - ? !hasPermissionForFiltered[index] + ? false // All remaining teams have permission since they passed the filter : !withRoleCanCreateEntity(membership.role),
🧹 Nitpick comments (1)
packages/trpc/server/routers/loggedInViewer/teamsAndUserProfilesQuery.handler.ts (1)
101-110: Consider performance optimization for permission checks.The current implementation makes individual permission checks for each team. Consider batching these operations if the PermissionCheckService supports it, especially for users with many team memberships.
Check if PermissionCheckService has batch checking capabilities:
// If batch checking is available: const permissionChecks = await permissionService.checkPermissions({ userId: ctx.user.id, checks: teamsData.map(membership => ({ teamId: membership.team.id, permission: permission as PermissionString, fallbackRoles: fallbackRoles ? (fallbackRoles as MembershipRole[]) : [], })) });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/trpc/server/routers/loggedInViewer/teamsAndUserProfilesQuery.handler.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/trpc/server/routers/loggedInViewer/teamsAndUserProfilesQuery.handler.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/trpc/server/routers/loggedInViewer/teamsAndUserProfilesQuery.handler.ts
🧠 Learnings (6)
📚 Learning: in signup-view.tsx, when checking if redirecturl contains certain strings, using explicit && checks ...
Learnt from: Anshumancanrock
PR: calcom/cal.com#22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
Applied to files:
packages/trpc/server/routers/loggedInViewer/teamsAndUserProfilesQuery.handler.ts
📚 Learning: applies to **/*.ts : ensure the `credential.key` field is never returned from trpc endpoints or apis...
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*.ts : Ensure the `credential.key` field is never returned from tRPC endpoints or APIs
Applied to files:
packages/trpc/server/routers/loggedInViewer/teamsAndUserProfilesQuery.handler.ts
📚 Learning: the filterhostsbysameroundrobinhost function in packages/lib/bookings/filterhostsbysameroundrobinhos...
Learnt from: CarinaWolli
PR: calcom/cal.com#22296
File: packages/lib/bookings/filterHostsBySameRoundRobinHost.ts:41-42
Timestamp: 2025-07-22T11:42:47.623Z
Learning: The filterHostsBySameRoundRobinHost function in packages/lib/bookings/filterHostsBySameRoundRobinHost.ts has a known limitation where it doesn't work correctly with fixed hosts or round robin groups. This is pre-existing technical debt that was already broken before the round robin groups feature. CarinaWolli has documented this in Linear issue CAL-6134 for future fix.
Applied to files:
packages/trpc/server/routers/loggedInViewer/teamsAndUserProfilesQuery.handler.ts
📚 Learning: in cal.com's getusereventgroups handler refactor (pr #22618), the membershipcount field for team eve...
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's getUserEventGroups handler refactor (PR #22618), the membershipCount field for team event groups is intentionally set to 0 in the new createTeamEventGroup function, as confirmed by sean-brydon (PR author). This preserves the same behavior as the old implementation that was being refactored, maintaining backward compatibility. While other parts of the codebase may use actual member counts, this specific implementation maintains the previous behavior.
Applied to files:
packages/trpc/server/routers/loggedInViewer/teamsAndUserProfilesQuery.handler.ts
📚 Learning: in the insightsroutingservice (packages/lib/server/service/insightsrouting.ts), multi-select filter ...
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/lib/server/service/insightsRouting.ts:367-368
Timestamp: 2025-07-15T13:02:17.403Z
Learning: In the InsightsRoutingService (packages/lib/server/service/insightsRouting.ts), multi-select filter data is already validated by zod before reaching the buildFormFieldSqlCondition method, so null/undefined values are not present in filterValue.data arrays and don't need to be filtered out.
Applied to files:
packages/trpc/server/routers/loggedInViewer/teamsAndUserProfilesQuery.handler.ts
📚 Learning: in cal.com's event type system, the membershipcount field for team event groups is intentionally set...
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's event type system, the membershipCount field for team event groups is intentionally set to 0, as confirmed by sean-brydon (PR author). This behavior was preserved during the refactor from the old getUserEventGroups.handler.ts implementation to the new createTeamEventGroup function in transformUtils.ts. This is not a bug but the intended behavior that maintains consistency with the previous implementation.
Applied to files:
packages/trpc/server/routers/loggedInViewer/teamsAndUserProfilesQuery.handler.ts
🧬 Code Graph Analysis (1)
packages/trpc/server/routers/loggedInViewer/teamsAndUserProfilesQuery.handler.ts (3)
packages/features/pbac/services/permission-check.service.ts (2)
PermissionCheckService(19-369)hasPermission(204-233)packages/platform/libraries/index.ts (1)
MembershipRole(98-98)packages/lib/entityPermissionUtils.server.ts (1)
withRoleCanCreateEntity(119-121)
⏰ 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 (2)
packages/trpc/server/routers/loggedInViewer/teamsAndUserProfilesQuery.handler.ts (2)
1-2: LGTM: Proper imports for PBAC system integration.The new imports correctly bring in the necessary types and services for permission-based access control functionality.
26-66: LGTM: Proper Prisma select usage.The query correctly uses
selectinstead ofincludeand only retrieves the necessary fields, adhering to the coding guidelines. Thecredential.keyfield is not exposed in this query.
| @@ -99,6 +103,10 @@ function WorkflowsPage({ filteredList }: PageProps) { | |||
| disableMobileButton={true} | |||
| onlyShowWithTeams={true} | |||
| includeOrg={true} | |||
| withPermission={{ | |||
There was a problem hiding this comment.
The user should still be able to create individual workflows even when the team doesn't allow it
| @@ -6,6 +6,7 @@ import { useState } from "react"; | |||
|
|
|||
| import { getPlaceholderAvatar } from "@calcom/lib/defaultAvatarImage"; | |||
| import { useLocale } from "@calcom/lib/hooks/useLocale"; | |||
| import type { WorkflowPermissions } from "@calcom/lib/server/repository/workflow-permissions"; | |||
There was a problem hiding this comment.
This user has no permission to see the team's workflows, but they are still shown as "readonly":
When clicking on the workflow to see the details it throws unauthorized
calcom#22845) Co-authored-by: sean@cal.com <Sean@brydon.io> Co-authored-by: Carina Wollendorfer <30310907+CarinaWolli@users.noreply.github.com> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: CarinaWolli <wollencarina@gmail.com>
feat: add Workflow resource to PBAC system with permission enforcement
Video Demo: https://cap.so/s/zn4pka3ta4yra7x
Summary
This PR adds the missing
Workflowresource to the PBAC (Permission-Based Access Control) system and enforces permissions when creating/updating workflows on team events. The implementation follows the established patterns used by other resources like EventType and Team.Key Changes:
Workflowto the Resource enum and complete CRUD permissions to PERMISSION_REGISTRYReview & Testing Checklist for Human
Recommended Test Plan:
Diagram
%%{ init : { "theme" : "default" }}%% graph TD A["permission-registry.ts<br/>Resource enum + PERMISSION_REGISTRY"]:::major-edit B["create.handler.ts<br/>Workflow creation tRPC endpoint"]:::major-edit C["update.handler.ts<br/>Workflow update tRPC endpoint"]:::major-edit D["migration.sql<br/>Add admin workflow permissions"]:::major-edit E["common.json<br/>i18n keys for workflow permissions"]:::minor-edit F["PermissionCheckService"]:::context G["Database<br/>RolePermission table"]:::context A -->|"defines workflow permissions"| F F -->|"checkPermission('workflow.create')"| B F -->|"checkPermission('workflow.update')"| C D -->|"INSERT workflow manage permissions"| G F -->|"queries role permissions"| G A -->|"references i18n keys"| E subgraph Legend L1[Major Edit]:::major-edit L2[Minor Edit]:::minor-edit L3[Context/No Edit]:::context end classDef major-edit fill:#90EE90 classDef minor-edit fill:#87CEEB classDef context fill:#FFFFFFNotes
yarn type-check:ciand lint checks, but runtime behavior needs verification