feat: add Workflow resource to PBAC system with permission enforcement#22845
feat: add Workflow resource to PBAC system with permission enforcement#22845sean-brydon merged 22 commits intomainfrom
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
…esQuery.handler.ts Co-authored-by: Carina Wollendorfer <30310907+CarinaWolli@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
| 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
|
|
||
| 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>
- Add Webhook resource to PBAC permission registry with CRUD actions - Implement PBAC permission checks in webhook handlers (create, edit, delete) - Add webhook permission translations to common.json - Use PermissionCheckService with fallback roles [ADMIN, OWNER] for team webhooks - Maintain backward compatibility when PBAC is disabled - Follow same pattern as workflow PBAC implementation from PR #22845 Co-Authored-By: sean@cal.com <Sean@brydon.io>
…#23614) * feat: add Webhook resource to PBAC system with permission enforcement - Add Webhook resource to PBAC permission registry with CRUD actions - Implement PBAC permission checks in webhook handlers (create, edit, delete) - Add webhook permission translations to common.json - Use PermissionCheckService with fallback roles [ADMIN, OWNER] for team webhooks - Maintain backward compatibility when PBAC is disabled - Follow same pattern as workflow PBAC implementation from PR #22845 Co-Authored-By: sean@cal.com <Sean@brydon.io> * fix: implement PBAC permission filtering in webhook list handler - Add PermissionCheckService to filter team webhooks by webhook.read permission - Only show webhooks from teams where user has proper permissions - Maintain backward compatibility with fallback to all team memberships Co-Authored-By: sean@cal.com <Sean@brydon.io> * add migration for default roles * new forUserMethod * update webhook repository * fix UI showing/hiding webhooks for webhoo.create teams * WIP pbac procedure migratoin + tests * add more roles to get fallback * permissions in cmponents instead of readOnly * passPermissions to list item * push instant events logic * Git merge * wip teamId accessable refactor * fix delete handler --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
3 participants
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