refactor: replace checkAdminOrOwner with PBAC availability.read permission #24115
Conversation
…ssion - Replace role-based check with PermissionCheckService - Use availability.read permission with OWNER/ADMIN fallback roles - Maintain existing privacy logic for organizations - Follow established PBAC pattern from bookings implementation Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
🤖 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:
|
WalkthroughReplaces admin/owner role checks with a permission-based guard using PermissionCheckService for the "availability.read" permission. Computes team access by retrieving team IDs with that permission, with a fallback to OWNER/ADMIN roles. Updates canViewTeamAvailability to depend on the presence of permitted team IDs and the organization privacy state. Applies this logic to the main availability page and the team availability toggle, enabling team visibility when the user has the required permission and the org is not private. No exported/public API changes. Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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. Comment |
![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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/web/app/(use-page-wrapper)/(main-nav)/availability/page.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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:
apps/web/app/(use-page-wrapper)/(main-nav)/availability/page.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:
apps/web/app/(use-page-wrapper)/(main-nav)/availability/page.tsx
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
apps/web/app/(use-page-wrapper)/(main-nav)/availability/page.tsx
🧠 Learnings (4)
📓 Common learnings
Learnt from: ShashwatPS
PR: calcom/cal.com#23638
File: packages/trpc/server/routers/viewer/calendars/setDestinationReminder.handler.test.ts:198-199
Timestamp: 2025-09-06T11:00:34.372Z
Learning: In calcom/cal.com PR #23638, the maintainer ShashwatPS determined that authorization checks in the setDestinationReminder handler are "not applicable" when CodeRabbit suggested adding user-scoped WHERE clauses to prevent users from modifying other users' destination calendar reminder settings.
Learnt from: eunjae-lee
PR: calcom/cal.com#24006
File: apps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsx:37-48
Timestamp: 2025-09-24T11:53:40.185Z
Learning: In bookings listing views, when checking permissions for the member filter UI, use "booking.read" permission rather than "team.listMembers" because the filter's purpose is to read bookings of other team members, not just to list the members themselves. The permission check should align with the actual capability being granted.
📚 Learning: 2025-08-20T17:36:46.521Z
Learnt from: mdm317
PR: calcom/cal.com#23221
File: packages/features/schedules/components/NewScheduleButton.tsx:35-35
Timestamp: 2025-08-20T17:36:46.521Z
Learning: PR #20804 "perf: server-side fetching for /availability" moved availability data fetching from TRPC to server-side in the Cal.com codebase. This means TRPC cache updates like `utils.viewer.availability.list.setData()` have no effect on the availability page, making server actions like `revalidateAvailabilityList()` necessary for proper cache invalidation.
Applied to files:
apps/web/app/(use-page-wrapper)/(main-nav)/availability/page.tsx
📚 Learning: 2025-08-20T17:34:35.004Z
Learnt from: mdm317
PR: calcom/cal.com#23221
File: packages/features/schedules/components/NewScheduleButton.tsx:1-1
Timestamp: 2025-08-20T17:34:35.004Z
Learning: In the Cal.com codebase, server actions like `revalidateAvailabilityList()` from `app/(use-page-wrapper)/(main-nav)/availability/actions` are imported and called directly in client components within mutation `onSuccess` callbacks. This pattern is consistently used across availability-related components (both in availability-view.tsx for updates and NewScheduleButton.tsx for creates) to ensure the availability list cache is fresh after mutations.
Applied to files:
apps/web/app/(use-page-wrapper)/(main-nav)/availability/page.tsx
📚 Learning: 2025-09-24T11:53:40.185Z
Learnt from: eunjae-lee
PR: calcom/cal.com#24006
File: apps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsx:37-48
Timestamp: 2025-09-24T11:53:40.185Z
Learning: In bookings listing views, when checking permissions for the member filter UI, use "booking.read" permission rather than "team.listMembers" because the filter's purpose is to read bookings of other team members, not just to list the members themselves. The permission check should align with the actual capability being granted.
Applied to files:
apps/web/app/(use-page-wrapper)/(main-nav)/availability/page.tsx
🧬 Code graph analysis (1)
apps/web/app/(use-page-wrapper)/(main-nav)/availability/page.tsx (2)
packages/features/pbac/services/permission-check.service.ts (1)
PermissionCheckService(19-306)packages/features/auth/lib/next-auth-options.ts (1)
session(746-771)
⏰ Context from checks skipped due to timeout of 180000ms. 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: Detect changes
| const permissionService = new PermissionCheckService(); | ||
| const teamIdsWithPermission = await permissionService.getTeamIdsWithPermission({ | ||
| userId: session.user.id, | ||
| permission: "availability.read", | ||
| fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN], | ||
| }); | ||
| const canViewTeamAvailability = teamIdsWithPermission.length > 0 || !isOrgPrivate; | ||
|
|
There was a problem hiding this comment.
Org-level admins lose access to team availability
getTeamIdsWithPermission() returns only teams where the user has direct membership (PBAC or fallback membership roles). Organization-level OWNER/ADMIN users without team memberships previously passed checkAdminOrOwner(session?.user?.org?.role) but now fail this check, so private-org admins cannot see the team toggle anymore. Please fold the org-role fallback back into the condition (e.g., keep the previous checkAdminOrOwner(session?.user?.org?.role) OR use a PBAC helper that considers org roles).
Apply this diff to restore the org-role fallback while keeping the PBAC check:
- const canViewTeamAvailability = teamIdsWithPermission.length > 0 || !isOrgPrivate;
+ const canViewTeamAvailability =
+ teamIdsWithPermission.length > 0 ||
+ !isOrgPrivate ||
+ checkAdminOrOwner(session?.user?.org?.role);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const permissionService = new PermissionCheckService(); | |
| const teamIdsWithPermission = await permissionService.getTeamIdsWithPermission({ | |
| userId: session.user.id, | |
| permission: "availability.read", | |
| fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN], | |
| }); | |
| const canViewTeamAvailability = teamIdsWithPermission.length > 0 || !isOrgPrivate; | |
| const permissionService = new PermissionCheckService(); | |
| const teamIdsWithPermission = await permissionService.getTeamIdsWithPermission({ | |
| userId: session.user.id, | |
| permission: "availability.read", | |
| fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN], | |
| }); | |
| const canViewTeamAvailability = | |
| teamIdsWithPermission.length > 0 || | |
| !isOrgPrivate || | |
| checkAdminOrOwner(session?.user?.org?.role); |
🤖 Prompt for AI Agents
In apps/web/app/(use-page-wrapper)/(main-nav)/availability/page.tsx around lines
77 to 84, the computed canViewTeamAvailability only checks teamIdsWithPermission
and org privacy, which excludes users who are org-level OWNER/ADMIN but not
members of any team; restore the org-role fallback by OR-ing the existing PBAC
result with a check for org-level admin/owner (e.g., call the existing
checkAdminOrOwner(session?.user?.org?.role) or use a PBAC helper that accounts
for org roles) so that either team membership permissions OR org role grants
access while keeping the original teamIdsWithPermission logic intact.
What does this PR do?
Refactors the availability page to replace role-based permission checking (
checkAdminOrOwner) with Cal.com's Permission-Based Access Control (PBAC) system using theavailability.readpermission. This change aligns with the ongoing PBAC migration initiative.Key Changes:
checkAdminOrOwner(session?.user?.org?.role)withPermissionCheckService.getTeamIdsWithPermission()availability.readpermission withOWNERandADMINfallback roles|| !isOrgPrivate)Context: This addresses a Slack request from @eunjae-lee to migrate away from
checkAdminOrOwnerusage in favor of PBAC'savailability.readpermission.How should this be tested?
Test scenarios:
/availabilitywith different user roles?type=teamis selectedVisual Demo
No UI changes expected - this is a backend permission logic change that should preserve existing behavior.
Mandatory Tasks
Security & Access Control:
checkAdminOrOwnerlogicavailability.readis the correct permission string for this use caseOWNER,ADMIN) match original admin/owner scopesession?.user?.org?.roleoptional chaining vs newsession.user.idBehavioral Verification:
|| !isOrgPrivate) still works correctlyLink to Devin run: https://app.devin.ai/sessions/42aa58f552d247148068f19b884b63b1
Requested by: @eunjae-lee via Slack