fix: apply PBAC to routing form CRUD#22859
Conversation
WalkthroughThis change set implements a detailed policy-based access control (PBAC) mechanism for routing forms. It enhances server-side permission checks by distinguishing personal and team-scoped forms, verifying ownership and team membership, and retrieving granular permissions for create, read, update, and delete actions. A new Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 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 (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
dosubot
bot
added
routing-forms
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/05/25)1 reviewer was added to this PR based on Keith Williams's automation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (5)
packages/lib/server/repository/PrismaRoutingFormRepository.ts (1)
34-68: Coding guideline violation: Useselectinstead ofinclude.The
findFormByIdIncludeUserTeamAndOrgmethod violates the coding guideline that specifies "For Prisma queries, only select data you need; never useinclude, always useselect".Consider refactoring this method to use
selectinstead ofinclude:static async findFormByIdIncludeUserTeamAndOrg(formId: string) { return await prisma.app_RoutingForms_Form.findUnique({ where: { id: formId, }, - include: { + select: { + id: true, + description: true, + position: true, + routes: true, + createdAt: true, + updatedAt: true, + name: true, + fields: true, + updatedById: true, + userId: true, + teamId: true, + disabled: true, + settings: true, user: { select: { id: true, username: true, email: true, movedToProfileId: true, metadata: true, organization: { select: { slug: true, }, }, }, }, team: { select: { parentId: true, parent: { select: { slug: true, }, }, slug: true, metadata: true, }, }, }, }); }packages/app-store/routing-forms/pages/incomplete-booking/[...appPages].tsx (2)
112-112: Uset()for text localization.Hard-coded text should be localized using the
t()function per coding guidelines for.tsxfiles.- Write to Salesforce contact/lead record + {t("write_to_salesforce_contact_lead_record")}
270-270: Uset()for text localization.Hard-coded text in toast messages should be localized using the
t()function per coding guidelines.- showToast("Field already exists", "error"); + showToast(t("field_already_exists"), "error");packages/app-store/routing-forms/trpc/formQuery.handler.ts (1)
23-40: Useselectinstead ofincludein Prisma queryPer coding guidelines, Prisma queries should only select the data needed and never use
include.- include: { - team: { select: { slug: true, name: true } }, - _count: { - select: { - responses: true, - }, - }, - }, + select: { + id: true, + userId: true, + teamId: true, + name: true, + description: true, + routes: true, + fields: true, + settings: true, + disabled: true, + position: true, + createdAt: true, + updatedAt: true, + duplicateFrom: true, + migrateJSRouterConfigOnUpgrade: true, + team: { + select: { + slug: true, + name: true, + }, + }, + _count: { + select: { + responses: true, + }, + }, + },packages/app-store/routing-forms/trpc/formMutation.handler.ts (1)
15-15: Remove unused importThe
isFormCreateEditAllowedimport is no longer used after refactoring to the new permission system.-import { isFormCreateEditAllowed } from "../lib/isFormCreateEditAllowed";
🧹 Nitpick comments (1)
packages/app-store/routing-forms/trpc/permissions.ts (1)
39-55: LGTM!The team-scoped permission check correctly integrates with the PBAC system and provides appropriate error handling with descriptive messages.
Consider optimizing by injecting the PermissionCheckService as a dependency rather than creating new instances, especially if this function is called frequently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
packages/app-store/routing-forms/components/SingleForm.tsx(2 hunks)packages/app-store/routing-forms/components/_components/Header.tsx(6 hunks)packages/app-store/routing-forms/components/getServerSidePropsSingleForm.ts(3 hunks)packages/app-store/routing-forms/pages/form-edit/[...appPages].tsx(2 hunks)packages/app-store/routing-forms/pages/incomplete-booking/[...appPages].tsx(1 hunks)packages/app-store/routing-forms/pages/reporting/[...appPages].tsx(2 hunks)packages/app-store/routing-forms/pages/route-builder/[...appPages].tsx(1 hunks)packages/app-store/routing-forms/trpc/deleteForm.handler.ts(2 hunks)packages/app-store/routing-forms/trpc/formMutation.handler.ts(3 hunks)packages/app-store/routing-forms/trpc/formQuery.handler.ts(2 hunks)packages/app-store/routing-forms/trpc/permissions.ts(1 hunks)packages/app-store/routing-forms/types/shared.ts(1 hunks)packages/features/pbac/domain/types/permission-registry.ts(2 hunks)packages/lib/server/getRoutedUrl.test.ts(1 hunks)packages/lib/server/getRoutedUrl.ts(2 hunks)packages/lib/server/repository/PrismaRoutingFormRepository.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/lib/server/getRoutedUrl.tspackages/lib/server/repository/PrismaRoutingFormRepository.tspackages/app-store/routing-forms/trpc/deleteForm.handler.tspackages/app-store/routing-forms/types/shared.tspackages/lib/server/getRoutedUrl.test.tspackages/app-store/routing-forms/trpc/formQuery.handler.tspackages/app-store/routing-forms/trpc/formMutation.handler.tspackages/app-store/routing-forms/components/getServerSidePropsSingleForm.tspackages/features/pbac/domain/types/permission-registry.tspackages/app-store/routing-forms/trpc/permissions.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/lib/server/getRoutedUrl.tspackages/app-store/routing-forms/pages/incomplete-booking/[...appPages].tsxpackages/app-store/routing-forms/components/SingleForm.tsxpackages/app-store/routing-forms/pages/reporting/[...appPages].tsxpackages/lib/server/repository/PrismaRoutingFormRepository.tspackages/app-store/routing-forms/trpc/deleteForm.handler.tspackages/app-store/routing-forms/types/shared.tspackages/app-store/routing-forms/pages/form-edit/[...appPages].tsxpackages/lib/server/getRoutedUrl.test.tspackages/app-store/routing-forms/trpc/formQuery.handler.tspackages/app-store/routing-forms/pages/route-builder/[...appPages].tsxpackages/app-store/routing-forms/trpc/formMutation.handler.tspackages/app-store/routing-forms/components/getServerSidePropsSingleForm.tspackages/features/pbac/domain/types/permission-registry.tspackages/app-store/routing-forms/trpc/permissions.tspackages/app-store/routing-forms/components/_components/Header.tsx
**/*.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/app-store/routing-forms/pages/incomplete-booking/[...appPages].tsxpackages/app-store/routing-forms/components/SingleForm.tsxpackages/app-store/routing-forms/pages/reporting/[...appPages].tsxpackages/app-store/routing-forms/pages/form-edit/[...appPages].tsxpackages/app-store/routing-forms/pages/route-builder/[...appPages].tsxpackages/app-store/routing-forms/components/_components/Header.tsx
**/*Repository.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Repository files must include
Repositorysuffix, prefix with technology if applicable (e.g.,PrismaAppRepository.ts), and use PascalCase matching the exported class
Files:
packages/lib/server/repository/PrismaRoutingFormRepository.ts
🧬 Code Graph Analysis (5)
packages/lib/server/getRoutedUrl.ts (1)
packages/lib/server/repository/PrismaRoutingFormRepository.ts (1)
PrismaRoutingFormRepository(20-69)
packages/app-store/routing-forms/components/SingleForm.tsx (1)
packages/app-store/routing-forms/types/shared.ts (1)
SingleFormComponentProps(8-20)
packages/app-store/routing-forms/trpc/deleteForm.handler.ts (2)
packages/app-store/routing-forms/trpc/permissions.ts (1)
checkPermissionOnExistingRoutingForm(8-56)packages/platform/libraries/index.ts (1)
MembershipRole(98-98)
packages/app-store/routing-forms/types/shared.ts (1)
packages/app-store/routing-forms/components/getServerSidePropsSingleForm.ts (1)
getServerSidePropsForSingleFormView(10-167)
packages/app-store/routing-forms/trpc/permissions.ts (3)
packages/platform/libraries/index.ts (2)
MembershipRole(98-98)TRPCError(56-56)packages/lib/server/repository/PrismaRoutingFormRepository.ts (1)
PrismaRoutingFormRepository(20-69)packages/features/pbac/services/permission-check.service.ts (2)
PermissionCheckService(20-370)hasPermission(205-234)
🔇 Additional comments (27)
packages/lib/server/repository/PrismaRoutingFormRepository.ts (1)
20-32: LGTM! Well-implemented repository method with proper typing.The new
findByIdmethod follows best practices with:
- Proper generic constraints for type safety
- Flexible field selection via options parameter
- Adherence to the coding guideline of using
selectinstead ofincludepackages/lib/server/getRoutedUrl.ts (2)
24-24: LGTM! Clean migration to the new repository class.The import change aligns with the introduction of
PrismaRoutingFormRepositoryand maintains the same functionality.
97-97: LGTM! Consistent usage of the new repository method.The method call correctly uses the new
PrismaRoutingFormRepository.findFormByIdIncludeUserTeamAndOrgmethod.packages/lib/server/getRoutedUrl.test.ts (1)
17-17: LGTM! Smart aliasing maintains test compatibility.The import change with aliasing (
as RoutingFormRepository) is a clean way to migrate to the new repository class while avoiding the need to update all test references.packages/app-store/routing-forms/types/shared.ts (1)
19-19: LGTM! Clean type extension for PBAC permissions.The addition of the
permissionsproperty usinginferSSRPropsensures type safety and consistency with the server-side props implementation. This properly supports the new Policy-Based Access Control functionality.packages/app-store/routing-forms/pages/route-builder/[...appPages].tsx (2)
1432-1432: LGTM! Clean prop addition for PBAC support.The addition of the
permissionsprop to theRouteBuildercomponent props correctly supports the Policy-Based Access Control implementation.
1440-1440: LGTM! Proper prop forwarding to SingleForm.The
permissionsprop is correctly passed down to theSingleFormcomponent, maintaining the data flow for access control enforcement.packages/app-store/routing-forms/pages/incomplete-booking/[...appPages].tsx (2)
323-323: LGTM: Permissions prop correctly added to component signature.This change aligns with the PBAC implementation being applied across routing form components.
330-330: LGTM: Permissions prop correctly passed to SingleForm.This completes the prop threading pattern for PBAC implementation.
packages/app-store/routing-forms/pages/reporting/[...appPages].tsx (1)
269-269: LGTM: Permissions prop correctly implemented.The permissions prop is properly added to the component signature and passed to SingleForm, following the established PBAC pattern.
Also applies to: 284-284
packages/app-store/routing-forms/components/SingleForm.tsx (2)
75-81: LGTM: Function signature correctly updated with permissions prop.The permissions prop is properly included in the SingleFormComponentProps destructuring, aligning with the type definition.
162-162: LGTM: Permissions correctly passed to Header component.This continues the proper prop threading pattern for PBAC implementation to control UI elements.
packages/app-store/routing-forms/pages/form-edit/[...appPages].tsx (1)
351-351: LGTM: Permissions prop correctly implemented.The permissions prop is properly added and passed to SingleForm, maintaining consistency with the PBAC implementation pattern.
Also applies to: 360-360
packages/app-store/routing-forms/trpc/deleteForm.handler.ts (2)
3-3: LGTM: Required imports added for new permission system.The MembershipRole and checkPermissionOnExistingRoutingForm imports are necessary for the enhanced PBAC implementation.
Also applies to: 10-10
22-27: LGTM: Proper implementation of granular permission checking.The new permission check uses the centralized
checkPermissionOnExistingRoutingFormfunction with appropriate:
- Permission string: "routingForm.delete" for delete operations
- Fallback roles: ADMIN and OWNER for team-scoped forms
- Proper error handling through the centralized function
This replaces the previous generic permission check with a more granular PBAC approach.
packages/app-store/routing-forms/trpc/formQuery.handler.ts (1)
46-52: Permission check implementation looks goodThe permission check is correctly placed after verifying form existence and uses appropriate fallback roles for read access.
packages/app-store/routing-forms/components/_components/Header.tsx (2)
62-76: Permission-based UI controls implemented correctlyThe Actions component properly uses the permissions object to disable buttons based on user permissions. The implementation is clean and follows best practices.
Also applies to: 175-175, 193-193
218-231: Permissions correctly propagated through component hierarchyThe Header component properly receives and passes the permissions prop to the Actions component in both mobile and desktop layouts.
Also applies to: 343-343, 377-377
packages/features/pbac/domain/types/permission-registry.ts (1)
10-10: Well-structured permission registry for RoutingForm resourceThe new RoutingForm resource and its permissions follow the established pattern. The Manage action is appropriately scoped to Organization level, and all i18n keys and descriptions are consistent with other resources.
Also applies to: 388-423
packages/app-store/routing-forms/components/getServerSidePropsSingleForm.ts (2)
92-97: Good security practice for personal form access controlReturning 404 instead of 403 prevents information leakage about form existence. The ownership check is straightforward and secure.
119-152: Well-implemented permission checking for team-scoped formsThe permission logic correctly:
- Defaults to full permissions for personal forms
- Validates team membership before checking permissions
- Uses appropriate fallback roles (MEMBER for read, ADMIN/OWNER for write operations)
- Returns 404 for unauthorized access to prevent information leakage
packages/app-store/routing-forms/trpc/formMutation.handler.ts (1)
37-62: Well-structured permission checks for form mutationsThe implementation correctly handles three scenarios:
- Updates - uses
checkPermissionOnExistingRoutingFormwith update permission- Team form creation - uses
PermissionCheckServicewith create permission- Personal form creation - implicitly allowed (no teamId)
The error handling provides a clear, descriptive message for forbidden access.
packages/app-store/routing-forms/trpc/permissions.ts (5)
1-6: LGTM!The imports are well-organized and include all necessary dependencies for implementing PBAC permission checks on routing forms.
8-18: LGTM!The function signature is well-designed with descriptive naming and proper TypeScript typing. The parameter structure clearly defines the required inputs for permission checking.
20-22: LGTM!Excellent adherence to coding guidelines by using
selectinstead ofincludeand only fetching the minimal required fields for permission checking.
24-29: LGTM!Proper error handling with appropriate TRPC error code and clear messaging for the not found case.
31-37: LGTM!The personal-scoped form permission check correctly implements ownership-based access control with appropriate error handling.
...prisma/migrations/20250806093054_insert_role_permissions_for_routing_form_pbac/migration.sql
Show resolved
Hide resolved
E2E results are ready! |
|
@hariombalhara regarding your feedback, I just did it here. It's not disabling the list item, but puts "read only" badge only now. |
- Fix TypeScript error in routing forms component
- Change readonly={readOnly} to readOnly={readOnly} to match component interface
- Resolves type check failure in CI
Co-Authored-By: hariom@cal.com <hariombalhara@gmail.com>
* fix: apply PBAC to routing form CRUD * apply permission checks to the UI * moving prisma call to repository [WIP] * rename repository and fix type error * update implementation * fix formMutation handler * remove unused import * revert some rename * add RolePermission for 'routingForm' * Revert "revert some rename" This reverts commit 0ef3114. * clean up PrismaRoutingFormRepository * fix unit test * remove no longer necessary code * fix type definition * explicit permission handling * do not disable un-editable routing form * fix: correct property name from readonly to readOnly in ListLinkItem - Fix TypeScript error in routing forms component - Change readonly={readOnly} to readOnly={readOnly} to match component interface - Resolves type check failure in CI Co-Authored-By: hariom@cal.com <hariombalhara@gmail.com> --------- Co-authored-by: Hariom Balhara <hariombalhara@gmail.com> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
5 participants
What does this PR do?
This PR applies PBAC to routing form CRUD.
Mandatory Tasks (DO NOT REMOVE)