revert: "fix: remove fallback permssions on organization Id. (#22769)"#23013
revert: "fix: remove fallback permssions on organization Id. (#22769)"#23013CarinaWolli merged 5 commits intomainfrom
Conversation
This reverts commit 30061c0. Removes the manage permission concept and fallback logic to simplify permission checking back to direct role-based checks.
|
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: |
WalkthroughThis PR removes "manage" as a CRUD action and its permission registry entries, deletes a migration that inserted manage permissions for default roles, and updates PBAC logic to stop treating manage as an implicit "all" permission. Permission-check.service no longer expands or falls back to manage-based checks; client-side getResourcePermissionLevel now requires all explicit actions to grant "all". Tests and i18n strings referencing manage were removed or adjusted, and workflow permission surfaces dropped the canManage field. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (3)**/*.ts📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Files:
**/*.{ts,tsx}📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Files:
**/*.tsx📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Files:
⏰ 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)
🔇 Additional comments (10)
✨ 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/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/11/25)1 reviewer was added to this PR based on Keith Williams's automation. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.ts (1)
54-61: Correctly aligns "all" with all non-internal actions (manage removed).The refactor to compute allResourcePerms from all non-internal actions and require all of them for "all" is consistent with removing manage-based escalation. Looks good.
One minor UX safeguard: consider deduping permissions in outputs from toggleResourcePermissionLevel/toggleSinglePermission to avoid accumulating duplicates during multiple toggles.
Example tweak (outside this hunk): return a unique array.
// at Line 114 return Array.from(new Set(newPermissions)); // at Line 167 return Array.from(new Set(newPermissions));packages/features/pbac/services/permission-check.service.ts (1)
205-206: Explicit-permission checks (no manage fallback) — good simplification.Team-level first, then org-level, with no manage-based escalation is correct for this revert. Reads cleanly.
Optional follow-ups:
- Consider standardizing membership lookups via the injected repository rather than using MembershipRepository static in checkPermission/checkPermissions for consistency and easier testing.
- Minor perf nit (non-blocking): when the caller already has membership (found earlier), passing the org customRoleId (if any) into hasPermission(s) could avoid one repository roundtrip.
Also applies to: 228-229
packages/features/pbac/services/__tests__/permission-check.service.test.ts (1)
270-289: Empty permissions semantics are now explicit — confirm intent.The test asserts false for an empty permissions array and verifies a repository call with []. If the intended contract is “empty set = false”, consider short-circuiting earlier to avoid a repository call. If not, keeping this test is fine; just confirm this is the desired behavior across callers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/__tests__/usePermissions.test.ts(0 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.ts(1 hunks)apps/web/public/static/locales/en/common.json(2 hunks)packages/features/pbac/domain/types/permission-registry.ts(0 hunks)packages/features/pbac/services/__tests__/permission-check.service.test.ts(2 hunks)packages/features/pbac/services/permission-check.service.ts(2 hunks)packages/prisma/migrations/20250728091301_add_manage_permissions_to_default_roles/migration.sql(0 hunks)
💤 Files with no reviewable changes (3)
- apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/tests/usePermissions.test.ts
- packages/prisma/migrations/20250728091301_add_manage_permissions_to_default_roles/migration.sql
- packages/features/pbac/domain/types/permission-registry.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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:
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.tspackages/features/pbac/services/permission-check.service.tspackages/features/pbac/services/__tests__/permission-check.service.test.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:
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.tspackages/features/pbac/services/permission-check.service.tspackages/features/pbac/services/__tests__/permission-check.service.test.ts
**/*.{service,repository}.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Avoid dot-suffixes like
.service.tsor.repository.tsfor new files; reserve.test.ts,.spec.ts,.types.tsfor their specific purposes
Files:
packages/features/pbac/services/permission-check.service.ts
🔇 Additional comments (1)
apps/web/public/static/locales/en/common.json (1)
3332-3332: String updates match revert (remove “across organization teams”).These wording changes are consistent with removing manage as a special capability. Please ensure other locale files reflect the same updates to avoid inconsistent translations.
Also applies to: 3340-3340, 3355-3355
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.ts (1)
112-115: Avoid duplicate permission entries when togglingtoggleResourcePermissionLevel and toggleSinglePermission can push duplicates. Before returning, normalize with a Set for idempotency.
- return newPermissions; + // De-duplicate + return Array.from(new Set(newPermissions));Also applies to: 162-166
packages/features/pbac/services/permission-check.service.ts (2)
111-132: Minor: Avoid double membership fetch on PBAC pathscheckPermission/checkPermissions fetch membership via MembershipRepository and then hasPermission/hasPermissions fetch it again via repository. Pass the membership (and orgMembership) down to avoid an extra query.
Also applies to: 234-249
214-223: Optional: short-circuit empty permissionsCurrently, hasPermissions can hit the repo with an empty array. Consider returning false early to avoid unnecessary calls and to keep semantics explicit, then adjust tests accordingly.
packages/features/pbac/services/__tests__/permission-check.service.test.ts (2)
270-289: Test over-coupled to implementation (asserts repo call with [])Asserting that checkRolePermissions is called with [] couples the test to an internal detail. Prefer asserting just the boolean result. This gives leeway to short-circuit on empty arrays later.
- expect(mockRepository.checkRolePermissions).toHaveBeenCalledWith("admin_role", []); + // No need to assert internal calls; only outcome matters
591-619: Test name vs. behavior mismatch: it’s union, not precedenceThe test named “org permissions should take precedence” asserts union of team and org actions. Consider renaming to reflect union behavior to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/__tests__/usePermissions.test.ts(0 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.ts(1 hunks)apps/web/public/static/locales/en/common.json(2 hunks)packages/features/pbac/domain/types/permission-registry.ts(0 hunks)packages/features/pbac/services/__tests__/permission-check.service.test.ts(2 hunks)packages/features/pbac/services/permission-check.service.ts(2 hunks)packages/prisma/migrations/20250728091301_add_manage_permissions_to_default_roles/migration.sql(0 hunks)
💤 Files with no reviewable changes (3)
- packages/prisma/migrations/20250728091301_add_manage_permissions_to_default_roles/migration.sql
- apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/tests/usePermissions.test.ts
- packages/features/pbac/domain/types/permission-registry.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{service,repository}.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Avoid dot-suffixes like
.service.tsor.repository.tsfor new files; reserve.test.ts,.spec.ts,.types.tsfor their specific purposes
Files:
packages/features/pbac/services/permission-check.service.ts
**/*.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/features/pbac/services/permission-check.service.tsapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.tspackages/features/pbac/services/__tests__/permission-check.service.test.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/features/pbac/services/permission-check.service.tsapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.tspackages/features/pbac/services/__tests__/permission-check.service.test.ts
🔇 Additional comments (5)
apps/web/public/static/locales/en/common.json (2)
3332-3332: Copy update aligns with removal of manage fallbackThe simplified phrasing (“All actions on …”) matches the PR’s removal of cross-team “manage” semantics. No issues.
Also applies to: 3340-3340, 3355-3355
3296-3300: No removal needed forpbac_action_manage
The i18n key is actively used in your permission registry (packages/features/pbac/domain/types/permission-registry.tsat lines ~362 and ~427) under[CrudAction.Manage]. SinceCrudAction.Manageis still present, you should keep this key.Likely an incorrect or invalid review comment.
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.ts (1)
54-62: “All” now requires every explicit action (not just CRUD) — verify UX intentFiltering out only internal “_” keys means “all” includes custom actions (invite/remove/etc.). This matches the PR (no manage), but changes semantics: CRUD-only roles won’t show as “all.” Confirm this is the intended UX and that tests reflect the broader set.
packages/features/pbac/services/permission-check.service.ts (2)
203-206: Org-level check now uses exact permission (no manage fallback) — LGTMDirectly checking the requested permission on the org role is consistent with dropping manage-based expansion.
226-229: Org-level multi-permission check mirrors single-permission path — LGTMUnion semantics (team first, then org) remain; no manage fallback. Good.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
CarinaWolli
left a comment
There was a problem hiding this comment.
works great, @sean-brydon can you fix the type error 🙏
E2E results are ready! |
| -- Add manage permissions to admin and owner roles | ||
| -- These permissions are for organization-level management capabilities | ||
|
|
||
| -- Insert manage permissions for admin role | ||
| INSERT INTO "RolePermission" (id, "roleId", resource, action, "createdAt") | ||
| SELECT | ||
| gen_random_uuid(), 'admin_role', resource, action, NOW() | ||
| FROM ( | ||
| VALUES | ||
| -- Role management permissions (organization scope) | ||
| ('role', 'manage'), | ||
|
|
||
| -- Event Type management permissions (organization scope) | ||
| ('eventType', 'manage'), | ||
|
|
||
| -- Team management permissions (organization scope) | ||
| ('team', 'manage'), | ||
|
|
||
| -- Booking management permissions (organization scope) | ||
| ('booking', 'manage') | ||
| ) AS permissions(resource, action) | ||
| ON CONFLICT ("roleId", resource, action) DO NOTHING; | ||
|
|
||
| -- Note: Owner role already has wildcard permissions (*.*) so it inherits all manage permissions automatically No newline at end of file |
There was a problem hiding this comment.
Do we really want to remove the migration file? cc @calcom/dba
There was a problem hiding this comment.
This didn't run yet on prod. Let's revert
This reverts commit 30061c0.
Removes the manage permission concept and fallback logic to simplify permission checking back to direct role-based checks.
"manage"
permissions for roles, event types, teams, and bookings
fallback logic in permission-check.service.ts
permissions
manage permissions to default roles
check for manage permissions
manage permissions