feat: PBAC remove dogfood for safe checking#23846
Conversation
WalkthroughThis change removes the internal PBAC "dogfooding" fallback from packages/features/pbac/services/permission-check.service.ts: the DOGFOOD_PBAC_INTERNALLY constant and the private dogfoodFallback method were deleted. PBAC-enabled branches in checkPermission and checkPermissions no longer call the dogfood fallback and now directly return hasPermission/hasPermissions results. The non-PBAC path still uses checkFallbackRoles. Logging for the PBAC-enabled-but-no-custom-role case remains. No exported API signatures were changed. Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ 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)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 0
🧹 Nitpick comments (1)
packages/features/pbac/services/permission-check.service.ts (1)
178-178: Avoid double DB fetch in PBAC path by reusing the already-fetched membership.checkPermission(s) fetches membership, then hasPermission(s) fetches it again via getMembership(membershipId). Add overloads that accept a membership object to eliminate the extra query.
Apply this change at the call sites:
- return this.hasPermission({ membershipId: membership.id }, permission); + return this.hasPermissionFromMembership(membership, permission);- return this.hasPermissions({ membershipId: membership.id }, permissions); + return this.hasPermissionsFromMembership(membership, permissions);And add implementations (outside this hunk):
// Accept a preloaded membership and only fetch orgMembership if needed. private async hasPermissionFromMembership( membership: Awaited<ReturnType<IPermissionRepository["getMembershipByMembershipId"]>>, permission: PermissionString ): Promise<boolean> { // Team-level if (membership?.customRoleId) { if (await this.repository.checkRolePermission(membership.customRoleId, permission)) return true; } // Org-level (fetch only if parent exists) if (membership?.team.parentId) { const orgMembership = await this.repository.getOrgMembership(membership.userId, membership.team.parentId); if (orgMembership?.customRoleId) { return this.repository.checkRolePermission(orgMembership.customRoleId, permission); } } return false; } private async hasPermissionsFromMembership( membership: Awaited<ReturnType<IPermissionRepository["getMembershipByMembershipId"]>>, permissions: PermissionString[] ): Promise<boolean> { if (membership?.customRoleId) { if (await this.repository.checkRolePermissions(membership.customRoleId, permissions)) return true; } if (membership?.team.parentId) { const orgMembership = await this.repository.getOrgMembership(membership.userId, membership.team.parentId); if (orgMembership?.customRoleId) { return this.repository.checkRolePermissions(orgMembership.customRoleId, permissions); } } return false; }Optional: downgrade the “PBAC is enabled... but no custom role” log to debug to avoid log noise during partial migrations.
📜 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)
packages/features/pbac/services/permission-check.service.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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.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.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/permission-check.service.ts
⏰ 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 (1)
packages/features/pbac/services/permission-check.service.ts (1)
129-129: PBAC path now returns direct PBAC decision (no legacy fallback) — aligned with PR intent.This matches the objective to remove the dogfood fallback. Please ensure rollouts guarantee every PBAC‑enabled membership has a customRoleId; otherwise calls will consistently return false for those users.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
emrysal
left a comment
emrysal
left a comment
There was a problem hiding this comment.
Removed redundant test cases & approved, lgtm 🚀
E2E results are ready! |
3 participants
What does this PR do?
Remove Dogfood check for safe throwing errors when permission checking