fix: Always enforce one owner with pbac #24144
Conversation
WalkthroughAdds owner-preservation checks to PBACRoleManager: imports 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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
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: 2
🧹 Nitpick comments (6)
packages/features/pbac/services/role-management.factory.ts (3)
72-75: Prisma guideline: select only needed fields.Per repo guidelines, use
selectfor Prisma queries. The proposed diff above already scopes fields to{ teamId, role, customRoleId }.
126-131: Harden enum/key checks; avoidinon objects andanycasts.Use own‑property checks and precise typing for default role ID detection.
-const isDefaultRole = role in DEFAULT_ROLE_IDS; +const isDefaultRole = Object.prototype.hasOwnProperty.call( + DEFAULT_ROLE_IDS, + role as string +); @@ -const isDefaultRoleId = Object.values(DEFAULT_ROLE_IDS).includes(role as any); +const isDefaultRoleId = (Object.values(DEFAULT_ROLE_IDS) as string[]).includes( + role as string +);Also applies to: 128-130, 131-147
108-114: Align terminology: “team” vs “organization”.Elsewhere (legacy manager) errors say “team”. Consider standardizing the message here for consistency.
packages/features/pbac/services/__tests__/role-management.factory.test.ts (3)
207-233: Ownership‑enforcement tests: good; add one more assertion.
- Nice coverage for last‑owner demotion and multi‑owner allowance.
- In the “owner to owner” test, assert that
prisma.membership.countwas not called to prove the early return.await expect( manager.assignRole(userId, organizationId, MembershipRole.OWNER, membershipId) ).resolves.not.toThrow(); expect(mockRoleService.assignRoleToMember).toHaveBeenCalledWith( DEFAULT_ROLE_IDS[MembershipRole.OWNER], membershipId ); +expect(prisma.membership.count).not.toHaveBeenCalled();Also applies to: 235-261, 263-290
350-360: Strengthen error assertions.
toThrow(new RoleManagementError(...))can be brittle. Prefer checking the error type and message/code separately.-await expect(...).rejects.toThrow( - new RoleManagementError("Membership not found", RoleManagementErrorCode.UNAUTHORIZED) -); +await expect(...).rejects.toMatchObject({ + name: "RoleManagementError", + message: "Membership not found", + code: RoleManagementErrorCode.UNAUTHORIZED, +});
132-155: Add a test for cross-team membership IDs.Given the potential for a
membershipIdfrom a different team, add a test wherefindUniquereturnsteamId≠organizationIdand expect UNAUTHORIZED.I can add this case if helpful.
Also applies to: 159-171, 186-198, 207-233, 235-261, 263-290, 292-319, 321-349
📜 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 (2)
packages/features/pbac/services/__tests__/role-management.factory.test.ts(4 hunks)packages/features/pbac/services/role-management.factory.ts(1 hunks)
🧰 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:
packages/features/pbac/services/__tests__/role-management.factory.test.tspackages/features/pbac/services/role-management.factory.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__/role-management.factory.test.tspackages/features/pbac/services/role-management.factory.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__/role-management.factory.test.tspackages/features/pbac/services/role-management.factory.ts
🧬 Code graph analysis (2)
packages/features/pbac/services/__tests__/role-management.factory.test.ts (3)
packages/platform/libraries/index.ts (1)
MembershipRole(34-34)packages/features/pbac/domain/errors/role-management.error.ts (1)
RoleManagementError(1-6)packages/features/pbac/lib/constants.ts (1)
DEFAULT_ROLE_IDS(16-20)
packages/features/pbac/services/role-management.factory.ts (3)
packages/platform/libraries/index.ts (1)
MembershipRole(34-34)packages/features/pbac/domain/errors/role-management.error.ts (1)
RoleManagementError(1-6)packages/features/pbac/lib/constants.ts (1)
DEFAULT_ROLE_IDS(16-20)
⏰ 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 (2)
packages/features/pbac/services/__tests__/role-management.factory.test.ts (2)
22-24: Mocks and singleton reset look good.Resetting mocks and reinitializing the singleton per test prevents bleed‑through. LGTM.
Also applies to: 45-82
134-154: Default and custom role assignment paths: solid coverage.Good use of non‑owner membership to bypass owner validation and verify default/custom role flows.
Also applies to: 156-180, 182-205
| const currentMembership = await prisma.membership.findUnique({ | ||
| where: { id: membershipId }, | ||
| }); | ||
|
|
||
| if (!currentMembership) { | ||
| throw new RoleManagementError("Membership not found", RoleManagementErrorCode.UNAUTHORIZED); | ||
| } | ||
|
|
There was a problem hiding this comment.
Block cross-team role changes; add select to Prisma call.
findUnique returns the full row and does not verify the membership belongs to the organizationId (team). An attacker could pass a membershipId from another team; the owner-count check would run against the wrong team, risking unauthorized changes. Add a select and assert teamId matches organizationId.
- const currentMembership = await prisma.membership.findUnique({
- where: { id: membershipId },
- });
+ const currentMembership = await prisma.membership.findUnique({
+ where: { id: membershipId },
+ select: { teamId: true, role: true, customRoleId: true },
+ });
@@
- if (!currentMembership) {
+ if (!currentMembership) {
throw new RoleManagementError("Membership not found", RoleManagementErrorCode.UNAUTHORIZED);
}
+
+ // Ensure the membership belongs to this team/organization
+ if (currentMembership.teamId !== organizationId) {
+ throw new RoleManagementError(
+ "Membership does not belong to this team",
+ RoleManagementErrorCode.UNAUTHORIZED
+ );
+ }Also applies to: 80-89
| // Count total owners in the organization/team | ||
| const ownerCount = await prisma.membership.count({ | ||
| where: { | ||
| teamId: organizationId, | ||
| accepted: true, | ||
| OR: [{ customRoleId: DEFAULT_ROLE_IDS[MembershipRole.OWNER] }, { role: MembershipRole.OWNER }], | ||
| }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Eliminate TOCTOU: enforce “at least one owner” atomically.
Counting owners and then updating outside a transaction is vulnerable to races (two concurrent demotions can leave zero owners). Wrap the validation and assignment in a single DB transaction and perform the owner-count check inside it. Consider passing a transaction client into RoleService.
Example shape:
await prisma.$transaction(async (tx) => {
await this.validateNotLastOwner(organizationId, membershipId, role, tx); // accept tx client
// use tx-aware RoleService: assignRoleToMember(roleId, membershipId, tx)
});Minimal API change: let validateNotLastOwner(..., db = prisma) use db.membership.*, and overload RoleService.assignRoleToMember to accept an optional tx client. I can draft this change if you want.
Also applies to: 108-114, 122-124
🤖 Prompt for AI Agents
In packages/features/pbac/services/role-management.factory.ts around lines
98–106 (and similarly for 108–114 and 122–124), the owner-count check and role
assignment are done outside a transaction (TOCTOU risk); change
validateNotLastOwner to accept an optional db client parameter (defaulting to
prisma) and make RoleService.assignRoleToMember accept an optional tx client,
then wrap the validate + assignment in a single prisma.$transaction so the
membership.count and the role update execute atomically using the same tx
client; update call sites to pass the tx into both validation and assignment.
| } | ||
| } | ||
|
|
||
| private async validateNotLastOwner( |
There was a problem hiding this comment.
Random: Just noticed this file is called .factory but contains 2 managers and a factory. We should split out into separate files.
There was a problem hiding this comment.
Yeah let me do this in a follow up!
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
E2E results are ready! |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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 (2)
packages/features/pbac/services/pbac-role-manager.service.ts(3 hunks)packages/features/pbac/services/role-management.factory.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/features/pbac/services/role-management.factory.ts
🧰 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/pbac-role-manager.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/pbac-role-manager.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/pbac-role-manager.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/pbac-role-manager.service.ts
🧬 Code graph analysis (1)
packages/features/pbac/services/pbac-role-manager.service.ts (2)
packages/features/pbac/domain/errors/role-management.error.ts (1)
RoleManagementError(1-6)packages/features/pbac/lib/constants.ts (1)
DEFAULT_ROLE_IDS(16-20)
⏰ 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). (8)
- GitHub Check: Production builds / Build Docs
- GitHub Check: Production builds / Build Web App
- GitHub Check: Production builds / Build Atoms
- GitHub Check: Production builds / Build API v1
- GitHub Check: Linters / lint
- GitHub Check: Production builds / Build API v2
- GitHub Check: Tests / Unit
- GitHub Check: Type check / check-types
🔇 Additional comments (3)
packages/features/pbac/services/pbac-role-manager.service.ts (3)
1-1: LGTM!The prisma import is necessary for the new owner validation logic.
7-7: LGTM!The type-only import is appropriate since
IRoleManageris only used for type annotations in this file.
96-96: LGTM!The validation is correctly placed before the role assignment logic, ensuring that the last owner protection is enforced. However, note the race condition concern raised separately for the
validateNotLastOwnermethod.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/features/pbac/services/pbac-role-manager.service.ts (1)
40-94: Race condition remains: validation and assignment are not atomic.The TOCTOU vulnerability identified in the past review is still present. Between validating the owner count (line 79) and executing the role assignment in
assignRole(lines 113/116/126), concurrent requests could demote other owners, potentially leaving the organization without any owners.Consider wrapping the validation and assignment in a database transaction to ensure atomicity:
await prisma.$transaction(async (tx) => { // Perform validation using tx // Perform role assignment using tx });Alternatively, implement optimistic locking by re-validating the owner count immediately before committing the role change.
🧹 Nitpick comments (1)
packages/features/pbac/services/pbac-role-manager.service.ts (1)
108-109: Consider improving type safety instead of disabling the lint rule.The
as anycast works but weakens type safety. Consider refactoring to use a type guard or a more explicit check.Apply this diff to improve type safety:
- // eslint-disable-next-line @typescript-eslint/no-explicit-any - const isDefaultRoleId = Object.values(DEFAULT_ROLE_IDS).includes(role as any); + const defaultRoleIdValues = Object.values(DEFAULT_ROLE_IDS) as string[]; + const isDefaultRoleId = typeof role === 'string' && defaultRoleIdValues.includes(role);
📜 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/pbac-role-manager.service.ts(3 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/pbac-role-manager.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/pbac-role-manager.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/pbac-role-manager.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/pbac-role-manager.service.ts
🧬 Code graph analysis (1)
packages/features/pbac/services/pbac-role-manager.service.ts (2)
packages/features/pbac/domain/errors/role-management.error.ts (1)
RoleManagementError(1-6)packages/features/pbac/lib/constants.ts (1)
DEFAULT_ROLE_IDS(16-20)
🔇 Additional comments (5)
packages/features/pbac/services/pbac-role-manager.service.ts (5)
1-1: LGTM! Clean import additions.The prisma import is necessary for the new validation logic, and using
import typeforIRoleManageris the correct TypeScript practice for type-only imports.Also applies to: 7-7
11-11: LGTM! Clear feature flag.The
isPBACEnabledflag clearly identifies this manager as using PBAC, which aligns with the PR objective.
46-54: Select clause now addresses the coding guideline.The Prisma query now uses
selectto fetch only the required fields (id,customRoleId,role,teamId), which aligns with the coding guidelines.
102-102: LGTM! Validation properly integrated.The
validateNotLastOwnercall is correctly placed before the role assignment logic, ensuring that the last owner cannot be demoted (subject to the TOCTOU issue noted separately).
79-85: Remove organizationId/teamId mapping concern
organizationIdcorresponds toTeam.id, which represents both teams and organizations, so using it asteamIdin this query is correct.Likely an incorrect or invalid review comment.
3 participants
What does this PR do?
Ensure we always have one owner with PBAC - making it sure nobody can ever brick a team
How should this be tested?
Login as admin of an org with pbac enabled
try to make only owner a admin
Checklist