Skip to content

fix: get bookings handler for pbac and fallback roles#25434

Merged
sean-brydon merged 29 commits intomainfrom
fix/get-bookings-pbac
Jan 16, 2026
Merged

fix: get bookings handler for pbac and fallback roles#25434
sean-brydon merged 29 commits intomainfrom
fix/get-bookings-pbac

Conversation

@sean-brydon
Copy link
Member

@sean-brydon sean-brydon commented Nov 27, 2025

What does this PR do?

This PR is stacked upon #25387

Fixes the issue where PBAC/fallback roles were not taken into consideration correctly when calling get booking. The changes include:

  • Refactored get.handler.ts to use PermissionCheckService instead of direct membership queries
  • Added orgId parameter to getTeamIdsWithPermission and getTeamIdsWithPermissions to properly scope results to the user's organization
  • Updated SQL queries in PermissionRepository to filter teams by organization scope
  • Added comprehensive unit tests for PBAC permission checks in the bookings handler
  • Added integration tests for the orgId filtering functionality

Updates since last revision

  • Merged latest main to resolve conflicts
  • Fixed failing unit tests by updating the PermissionCheckService mock to use function() instead of arrow function (required for proper constructor mocking in Vitest)
  • Renamed scopedOrgId parameter to orgId for consistency
  • Changed fallback roles to use MembershipRole enum instead of hardcoded strings

Visual Demo (For contributors especially)

N/A - Backend logic changes only

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. N/A
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

  1. Run the unit tests: TZ=UTC yarn vitest run packages/trpc/server/routers/viewer/bookings/get.handler.test.ts
  2. Run the integration tests: VITEST_MODE=integration yarn test packages/features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration-test.ts
  3. Verify that users with PBAC permissions or ADMIN/OWNER fallback roles can view bookings for their team members
  4. Verify that users cannot view bookings for users outside their permission scope

Human Review Checklist

  • Verify the SQL queries in PermissionRepository.ts correctly handle the orgId filtering (including null/undefined cases)
  • Verify the permission checks work correctly for both PBAC-enabled teams and fallback role scenarios
  • Confirm the test mock fix using function() instead of arrow function is the correct pattern

Link to Devin run: https://app.devin.ai/sessions/8454efaba8ea4ecdb672f6ec9bde2876
Requested by: sean@cal.com (@sean-brydon)

@keithwillcode keithwillcode added consumer core area: core, team members only labels Nov 27, 2025
@sean-brydon sean-brydon marked this pull request as ready for review November 28, 2025 09:21
@sean-brydon sean-brydon requested a review from a team as a code owner November 28, 2025 09:21
@graphite-app graphite-app bot requested a review from a team November 28, 2025 09:21
CarinaWolli
CarinaWolli previously approved these changes Nov 28, 2025
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

emrysal
emrysal previously approved these changes Nov 28, 2025
Copy link
Contributor

@emrysal emrysal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still needs some work elsewhere in this file, but good work on implementing PBAC!

@github-actions
Copy link
Contributor

github-actions bot commented Nov 28, 2025

E2E results are ready!

@sean-brydon sean-brydon dismissed stale reviews from emrysal and CarinaWolli via cd6b781 December 1, 2025 09:05
@vercel
Copy link

vercel bot commented Dec 1, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

3 Skipped Deployments
Project Deployment Review Updated (UTC)
cal Ignored Ignored Jan 7, 2026 10:58am
cal-companion Ignored Ignored Preview Jan 7, 2026 10:58am
cal-eu Ignored Ignored Jan 7, 2026 10:58am

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 3 files

return eventTypeIdsFromDb;
}

async function getEventTypeIdsWhereUserIsAdminOrOwner(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i like this deletion 🔥 no more need for custom logic to check resource authorization

eunjae-lee
eunjae-lee previously approved these changes Dec 1, 2025
Copy link
Contributor

@eunjae-lee eunjae-lee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good and it works well

@github-actions
Copy link
Contributor

This PR has been marked as stale due to inactivity. If you're still working on it or need any help, please let us know or update the PR to keep it active.

Copy link
Contributor

@hbjORbj hbjORbj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you fix the failing tests? 🙏

@github-actions
Copy link
Contributor

This PR has been marked as stale due to inactivity. If you're still working on it or need any help, please let us know or update the PR to keep it active.

@github-actions github-actions bot added the Stale label Jan 15, 2026
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 6 files

devin-ai-integration bot and others added 2 commits January 16, 2026 14:15
Co-Authored-By: sean@cal.com <Sean@brydon.io>
Co-Authored-By: sean@cal.com <Sean@brydon.io>
Copy link
Contributor

@eunjae-lee eunjae-lee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants