feat: dogfood pbac internally on IS_CALCOM instances #23537
feat: dogfood pbac internally on IS_CALCOM instances #23537sean-brydon merged 4 commits intomainfrom
Conversation
WalkthroughAdded an internal DOGFOOD_PBAC_INTERNALLY toggle and dogfoodFallback logic in PermissionCheckService: after awaiting PBAC checks in checkPermission and checkPermissions, if PBAC denies access and the toggle is enabled the code routes to a new private dogfoodFallback which logs a JSON warning payload and evaluates role-based fallback via checkFallbackRoles using membershipRole and fallbackRoles. Tests were expanded to cover PBAC-failure fallback paths for single and multiple permissions and an empty-permissions case. Note: dogfoodFallback is defined twice in the class (duplicate private method definitions). Possibly related PRs
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (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/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/features/pbac/services/permission-check.service.ts (1)
275-277: Fix unsafe optional chaining that can throw at runtime.membership?.team.parentId will throw if team is undefined.
- if (membership?.team.parentId) { + if (membership?.team?.parentId) {
🧹 Nitpick comments (7)
packages/features/pbac/services/__tests__/permission-check.service.test.ts (4)
17-23: Mocking IS_CALCOM for dogfood path is fine; consider adding a non-Cal.com check.Looks good to force the internal path. Optionally add one test where IS_CALCOM=false (e.g., with vi.resetModules and a different mock) to confirm no RBAC fallback occurs when PBAC denies.
157-193: Dogfood fallback happy-path covered; assert no org fetch to tighten scope.Test is solid. You can also assert that getOrgMembership wasn’t called to make the fallback signal unambiguous in this scenario.
- expect(mockRepository.checkRolePermission).toHaveBeenCalled(); + expect(mockRepository.checkRolePermission).toHaveBeenCalled(); + expect(mockRepository.getOrgMembership).toHaveBeenCalledTimes(1);
299-335: Multi-permission dogfood fallback covered; mirror single-permission assertion.Similarly, consider asserting org lookup was attempted exactly once to document intended flow.
- expect(mockRepository.checkRolePermissions).toHaveBeenCalled(); + expect(mockRepository.checkRolePermissions).toHaveBeenCalled(); + expect(mockRepository.getOrgMembership).toHaveBeenCalledTimes(1);
336-370: Nit: Align test data naming to reduce cognitive load.Role is MEMBER but customRoleId is "admin_role". Rename to "member_role" and update the expectation to avoid confusion.
- role: "MEMBER" as MembershipRole, // Change to MEMBER so fallback also fails - customRoleId: "admin_role", + role: "MEMBER" as MembershipRole, // MEMBER so fallback also fails + customRoleId: "member_role", ... - expect(mockRepository.checkRolePermissions).toHaveBeenCalledWith("admin_role", []); + expect(mockRepository.checkRolePermissions).toHaveBeenCalledWith("member_role", []);packages/features/pbac/services/permission-check.service.ts (3)
20-21: Make dogfood flag runtime-togglable.Hard-tying to IS_CALCOM is fine, but add an env/feature override to disable quickly if needed.
-const DOGFOOD_PBAC_INTERNALLY = IS_CALCOM; +const DOGFOOD_PBAC_INTERNALLY = + (process.env.DOGFOOD_PBAC_INTERNALLY ?? "").toLowerCase() === "true" || IS_CALCOM;
286-331: Improve log grammar and use structured logging; include membershipRole.Message is awkward (“doesnt”) and current stringified JSON reduces queryability in Axiom. Prefer structured fields.
- const debugInfo = { + const debugInfo = { userId, teamId, membershipId, permissions, hasPbacPermission, fallbackRoles, + membershipRole, }; - - this.logger.warn( - `PBAC INTERNAL - Failed but user doesnt have permission string to carry out this action. Falling back to fallback roles. \n JSON${JSON.stringify( - debugInfo, - null, - 2 - )}` - ); + this.logger.warn( + { pbacDogfoodFallback: debugInfo }, + "PBAC INTERNAL - PBAC check failed; falling back to RBAC (dogfood)" + );
222-240: Optional: Avoid redundant membership fetches in hasPermission(s).checkPermission(s) already fetched membership; passing membership/orgMembership into hasPermission(s) would save a DB roundtrip.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/features/pbac/services/__tests__/permission-check.service.test.ts(4 hunks)packages/features/pbac/services/permission-check.service.ts(5 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/features/pbac/services/__tests__/permission-check.service.test.tspackages/features/pbac/services/permission-check.service.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/__tests__/permission-check.service.test.tspackages/features/pbac/services/permission-check.service.ts
**/*.{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:
packages/features/pbac/services/__tests__/permission-check.service.test.tspackages/features/pbac/services/permission-check.service.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
🧬 Code graph analysis (1)
packages/features/pbac/services/permission-check.service.ts (3)
packages/lib/constants.ts (1)
IS_CALCOM(43-49)packages/features/pbac/domain/types/permission-registry.ts (1)
PermissionString(60-60)packages/platform/libraries/index.ts (1)
MembershipRole(98-98)
⏰ 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). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (3)
packages/features/pbac/services/permission-check.service.ts (3)
2-2: Importing IS_CALCOM is appropriate; confirm build-time value.Ensure the bundling/runtime environment doesn’t tree-shake/inline a stale value across deployments. If needed, expose this via config to allow runtime toggling.
132-147: LGTM: PBAC-first with dogfood fallback is correctly gated.The flow validates, checks PBAC, then optionally falls back via roles only on internal dogfooding.
195-210: LGTM: Multi-permission path mirrors single-permission behavior.Consistent gating and return semantics.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
E2E results are ready! |
3 participants
What does this PR do?
This PR enabled the PBAC check for pretty much everywhere if the feature flag is enabled for a org.
However, If PBAC fails for whatever reason - we fall back to our old role checks (current way of checking permissions in prod) and log heavily so we can setup alerts in axiom and figure out where this is happening.
We will be testing this internally on the Cal.com org while we roll this out slowly with/without a feature flag.
How should this be tested?
Use yarn seed-pbac and just explore the app. If you see logs console of
PBAC INTERNALthere is places where the default role permissions do not match the current implementation without a feature flag