fix: team admin/owner can't change others member role#23161
fix: team admin/owner can't change others member role#23161anikdhabal merged 4 commits intocalcom:mainfrom
Conversation
WalkthroughIRoleManager.checkPermissionToChangeRole signature changed from (userId, organizationId) to (userId, targetId, scope: "org" | "team"). PBACRoleManager now invokes PBAC checks with teamId = targetId and permission = (scope === "team" ? "team.changeMemberRole" : "organization.changeMemberRole") and falls back to OWNER/ADMIN roles; LegacyRoleManager resolves membership using isTeamAdmin(userId, targetId) or isOrganisationAdmin(userId, targetId) based on scope. Both paths throw RoleManagementError with UNAUTHORIZED on failure and retain their existing messages. Handlers that change member roles (team/org) were updated to pass the new scope argument and removed the earlier pre-check in the team handler. Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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
CodeRabbit Configuration File (
|
|
@anikdhabal is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
dosubot
bot
added
teams
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/18/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add ready-for-e2e label" took an action on this PR • (08/18/25)1 label was added to this PR based on Keith Williams's automation. |
![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
🔭 Outside diff range comments (2)
packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts (1)
109-117: Avoid Prismainclude; useselectwith a minimal safe shapePer repo guidelines: “only select data you need; never use include”. Returning entire
teamandusercan leak unnecessary PII and bloat the response.- const updatedMembership = await prisma.membership.findUnique({ - where: { - userId_teamId: { userId: input.memberId, teamId: input.teamId }, - }, - include: { - team: true, - user: true, - }, - }); + const updatedMembership = await prisma.membership.findUnique({ + where: { + userId_teamId: { userId: input.memberId, teamId: input.teamId }, + }, + select: { + id: true, + role: true, + teamId: true, + userId: true, + // Adjust fields below to what the client actually consumes + team: { + select: { id: true, name: true, slug: true, isOrganization: true }, + }, + user: { + select: { id: true, name: true, username: true }, // avoid unnecessary PII + }, + }, + });If the client needs additional fields, add them explicitly under
select.packages/features/pbac/services/role-management.factory.ts (1)
1-5: Missing import:isTeamAdminis used but not imported (compile error)
LegacyRoleManager.checkPermissionToChangeRolecallsisTeamAdminwithout an import.import { FeaturesRepository } from "@calcom/features/flags/features.repository"; import { isOrganisationAdmin } from "@calcom/lib/server/queries/organisations"; +import { isTeamAdmin } from "@calcom/lib/server/queries/teams"; import { prisma } from "@calcom/prisma"; import { MembershipRole } from "@calcom/prisma/enums";
🧹 Nitpick comments (4)
packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts (3)
55-59: Narrow membership query: select only required fields and filter accepted membersThis reduces payload, aligns with Prisma guidelines, and avoids counting pending invites in ownership/role checks.
- const memberships = await prisma.membership.findMany({ - where: { - teamId: input.teamId, - }, - }); + const memberships = await prisma.membership.findMany({ + where: { + teamId: input.teamId, + accepted: true, + }, + select: { + id: true, + userId: true, + role: true, + }, + });
49-52: Minor:ctx.useris NonNullable; drop optional chaining
ctx.useris typed asNonNullable<TrpcSessionUser>, soctx.user?.idis unnecessary.- if (!(await isTeamAdmin(ctx.user?.id, input.teamId))) throw new TRPCError({ code: "UNAUTHORIZED" }); + if (!(await isTeamAdmin(ctx.user.id, input.teamId))) throw new TRPCError({ code: "UNAUTHORIZED" }); @@ - if (input.role === MembershipRole.OWNER && !(await isTeamOwner(ctx.user?.id, input.teamId))) + if (input.role === MembershipRole.OWNER && !(await isTeamOwner(ctx.user.id, input.teamId)))
35-35: Use team.isOrganization for PBAC permission scopeVerified that TeamRepository.findById includes the isOrganization flag. Pass that to checkPermissionToChangeRole so PBAC uses the correct permission (
organization.changeMemberRolevsteam.changeMemberRole).• File: packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts
Line: 35- await roleManager.checkPermissionToChangeRole(ctx.user.id, input.teamId, false); + await roleManager.checkPermissionToChangeRole(ctx.user.id, input.teamId, team.isOrganization);packages/features/pbac/services/role-management.factory.ts (1)
52-80: Minor naming confusion in PBAC assignRole:organizationIdactually represents teamIdThe parameter name can mislead readers. Consider renaming the parameter (and interface param) to
teamIdfor clarity in a follow-up refactor. No functional issue.
📜 Review details
Configuration used: CodeRabbit UI
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/role-management.factory.ts(3 hunks)packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/trpc/server/routers/viewer/teams/changeMemberRole.handler.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/trpc/server/routers/viewer/teams/changeMemberRole.handler.tspackages/features/pbac/services/role-management.factory.ts
🧠 Learnings (1)
📚 Learning: 2025-08-17T22:00:16.304Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/trpc/server/routers/viewer/aiVoiceAgent/_router.ts:117-126
Timestamp: 2025-08-17T22:00:16.304Z
Learning: In calcom/cal.com PR #22995, packages/trpc/server/routers/viewer/aiVoiceAgent/_router.ts, the enabled input parameter in the update endpoint is intentionally not forwarded to aiService.updateAgentConfiguration() as the enabled/disabled agent functionality is not required at the moment (per maintainer Udit-takkar). Future reviews should not flag this as missing functionality unless requirements change.
Applied to files:
packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts
🧬 Code Graph Analysis (1)
packages/features/pbac/services/role-management.factory.ts (3)
packages/features/pbac/services/permission-check.service.ts (1)
hasPermission(205-234)packages/lib/server/queries/organisations/index.ts (1)
isOrganisationAdmin(7-17)packages/lib/server/queries/teams/index.ts (1)
isTeamAdmin(387-407)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Detect changes
🔇 Additional comments (4)
packages/features/pbac/services/role-management.factory.ts (4)
13-13: Interface change to target-based permission check looks goodThe generalized signature increases flexibility across team/org contexts.
102-110: Legacy path: logic is right; ensure parity with org/team checksGood fallback to org vs team admin checks. Once the missing import is added, this will behave as intended.
174-181: Factory wiring LGTMFeature-flag-based manager selection is clean and preserves existing behavior.
32-41: checkPermission call is correct for both teams and organizations
We’ve verified thatPermissionCheckService.checkPermissionalways expects ateamIdfield—there is no separateorganizationIdparameter—so passingteamId: targetIdis the intended shape even whenisOrganizationis true. The permission string toggle is sufficient to distinguish org-scoped checks.Optional nitpick: consider renaming
targetId→resourceIdfor clearer intent.
packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/features/pbac/services/role-management.factory.ts (1)
32-41: Clarify scope semantics and guard against misuse of targetId.The method takes a generic targetId and always passes it to PermissionCheckService as teamId, switching permission string by checkAtTeamLevel. This works if targetId is “the team’s id” for team-level checks and “the organization team’s id” for org-level checks. To avoid future misuse:
- Add a brief JSDoc clarifying that targetId must be the team id for team checks and the organization (team) id for org checks.
- Optionally default the flag to false in the implementation for clarity.
- Consider renaming targetId to teamOrOrgId for self-documentation.
Example minimal tweak:
- async checkPermissionToChangeRole( - userId: number, - targetId: number, - checkAtTeamLevel?: boolean - ): Promise<void> { + /** + * targetId: pass the team id if checkAtTeamLevel=true, otherwise the organization (team) id. + */ + async checkPermissionToChangeRole( + userId: number, + targetId: number, + checkAtTeamLevel = false + ): Promise<void> {packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts (1)
106-115: Prisma include → select per repo guidelines. Only select the fields you need.Current code returns full team and user records. To adhere to “never use include; always use select” and minimize PII exposure, select only required fields.
Apply a tailored select. Example (adjust fields to consumer needs):
const updatedMembership = await prisma.membership.findUnique({ where: { userId_teamId: { userId: input.memberId, teamId: input.teamId }, }, - include: { - team: true, - user: true, - }, + select: { + userId: true, + teamId: true, + role: true, + team: { + select: { id: true, name: true, slug: true, parentId: true, isOrganization: true }, + }, + user: { + select: { id: true, username: true, name: true, email: true }, + }, + }, });If email is not required by the caller, omit it to further reduce exposure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/features/pbac/services/role-management.factory.ts(3 hunks)packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/trpc/server/routers/viewer/teams/changeMemberRole.handler.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/trpc/server/routers/viewer/teams/changeMemberRole.handler.tspackages/features/pbac/services/role-management.factory.ts
🧠 Learnings (1)
📚 Learning: 2025-08-17T22:00:16.304Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/trpc/server/routers/viewer/aiVoiceAgent/_router.ts:117-126
Timestamp: 2025-08-17T22:00:16.304Z
Learning: In calcom/cal.com PR #22995, packages/trpc/server/routers/viewer/aiVoiceAgent/_router.ts, the enabled input parameter in the update endpoint is intentionally not forwarded to aiService.updateAgentConfiguration() as the enabled/disabled agent functionality is not required at the moment (per maintainer Udit-takkar). Future reviews should not flag this as missing functionality unless requirements change.
Applied to files:
packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts
🧬 Code Graph Analysis (1)
packages/features/pbac/services/role-management.factory.ts (3)
packages/features/pbac/services/permission-check.service.ts (1)
hasPermission(205-234)packages/lib/server/queries/teams/index.ts (1)
isTeamAdmin(387-407)packages/lib/server/queries/organisations/index.ts (1)
isOrganisationAdmin(7-17)
🔇 Additional comments (4)
packages/features/pbac/services/role-management.factory.ts (1)
13-14: All call-sites use the updated signature correctly.
The organization handler omits the optionalcheckAtTeamLevelflag (defaults tofalse), the team handler passestrue, and all tests invoke the two-arg form as intended. No further updates required.packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts (3)
2-2: Import cleanup looks good.Switching to only import isTeamOwner is consistent with the removal of the pre isTeamAdmin check.
33-41: Using the role manager’s permission check at team scope is aligned with the PR goal.Calling checkPermissionToChangeRole(ctx.user.id, input.teamId, true) enforces team-level permissions and fixes the inability of team admins/owners to change member roles.
Please confirm this matches product expectations for org-level roles: with this change, an org owner/admin who is not a member of the subteam still cannot change that team’s members. If cross-team control by org owners is desired, we could fall back to an org-level check when the team-level check fails.
49-51: Owner gating retained — good.Preserving the “only owners can award owner role” rule maintains current security posture while PBAC rolls out.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/features/pbac/services/role-management.factory.ts (1)
3-3: Fixed missing import for isTeamAdmin — good catchThis resolves the compile-time error flagged earlier and enables the legacy team-level admin check.
🧹 Nitpick comments (1)
packages/features/pbac/services/role-management.factory.ts (1)
14-14: Nit: clarify parameter name targetId → teamOrOrgId for readabilityThe current name is ambiguous. Renaming improves self-documentation without changing behavior.
Apply:
- checkPermissionToChangeRole(userId: number, targetId: number, checkAtTeamLevel?: boolean): Promise<void>; + checkPermissionToChangeRole(userId: number, teamOrOrgId: number, checkAtTeamLevel?: boolean): Promise<void>;- async checkPermissionToChangeRole( - userId: number, - targetId: number, - checkAtTeamLevel?: boolean - ): Promise<void> { + async checkPermissionToChangeRole( + userId: number, + teamOrOrgId: number, + checkAtTeamLevel?: boolean + ): Promise<void> { @@ - teamId: targetId, + teamId: teamOrOrgId,- async checkPermissionToChangeRole( - userId: number, - targetId: number, - checkAtTeamLevel?: boolean - ): Promise<void> { + async checkPermissionToChangeRole( + userId: number, + teamOrOrgId: number, + checkAtTeamLevel?: boolean + ): Promise<void> { @@ - const membership = checkAtTeamLevel - ? !!(await isTeamAdmin(userId, targetId)) - : !!(await isOrganisationAdmin(userId, targetId)); + const membership = checkAtTeamLevel + ? !!(await isTeamAdmin(userId, teamOrOrgId)) + : !!(await isOrganisationAdmin(userId, teamOrOrgId));Also applies to: 33-37, 40-41, 103-111
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/features/pbac/services/role-management.factory.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/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/role-management.factory.ts
🧬 Code Graph Analysis (1)
packages/features/pbac/services/role-management.factory.ts (3)
packages/features/pbac/services/permission-check.service.ts (1)
hasPermission(205-234)packages/lib/server/queries/teams/index.ts (1)
isTeamAdmin(387-407)packages/lib/server/queries/organisations/index.ts (1)
isOrganisationAdmin(7-17)
🔇 Additional comments (3)
packages/features/pbac/services/role-management.factory.ts (3)
103-111: Legacy path gating looks correctSwitching between isTeamAdmin and isOrganisationAdmin based on checkAtTeamLevel cleanly fixes the permission issue for legacy setups.
33-42: Ignore PBAC scope‐mapping suggestion:PermissionCheckonly supportsteamId
ThePermissionCheckinterface (packages/features/pbac/domain/models/Permission.ts) defines onlyteamId(noorganizationId), andPermissionCheckServicenever accepts anorganizationIdfield. Organization‐level checks are driven solely by the permission string ("organization.changeMemberRole"), so passingteamId: targetIdin both branches is correct.Likely an incorrect or invalid review comment.
14-14: Signature change: Add explicit scope argument
The methodcheckPermissionToChangeRolenow takes a third boolean parameter (checkAtTeamLevel). Please confirm that no call sites are still invoking it with only two arguments. For example, you can run:# Find any two-arg invocations that need updating rg -nP --include '*.ts' 'checkPermissionToChangeRole\(\s*[^,]+,\s*[^,]+\)'If this returns any results, update those calls to include the new
checkAtTeamLevelargument.
CarinaWolli
left a comment
CarinaWolli
left a comment
There was a problem hiding this comment.
Fix works, but can you take a look at eunjae's suggestion
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (4)
packages/trpc/server/routers/viewer/organizations/updateUser.handler.ts (3)
44-45: Typo in error message: “organizaiton” → “organization”User-facing error text should be correct.
Apply:
- if (!organizationId) - throw new TRPCError({ code: "UNAUTHORIZED", message: "You must be a member of an organizaiton" }); + if (!organizationId) + throw new TRPCError({ code: "UNAUTHORIZED", message: "You must be a member of an organization" });
60-95: Use Prisma select instead of include; narrow fields and filter relationsPer guidelines, avoid include. Also, only fetch the fields you actually use:
- You only need membership.id, user.profiles.username for this organization, and for children: the membership for the same user to derive teamIds.
- Remove user.username (unused).
- Filter profiles by organizationId to avoid reading an unrelated profile at index 0.
Apply:
- const requestedMember = await prisma.membership.findFirst({ + const requestedMember = await prisma.membership.findFirst({ where: { userId: input.userId, teamId: organizationId, accepted: true, }, - include: { - team: { - include: { - children: { - where: { - members: { - some: { - userId: input.userId, - }, - }, - }, - include: { - members: true, - }, - }, - }, - }, - user: { - select: { - username: true, - profiles: { - select: { - username: true, - }, - }, - }, - }, - }, + select: { + id: true, // membership id + team: { + select: { + children: { + where: { + members: { + some: { userId: input.userId }, + }, + }, + select: { + members: { + where: { userId: input.userId }, + select: { teamId: true, userId: true }, + }, + }, + }, + }, + }, + user: { + select: { + profiles: { + where: { organizationId }, + select: { username: true }, + }, + }, + }, + }, });Follow-up: With the above change, the downstream usage stays valid:
- requestedMember.id is selected
- requestedMember.user.profiles[0]?.username remains correct
- requestedMember.team.children[].members contains only the target user, so the map/find still works (can even be simplified later).
174-181: Dead/unreachable condition: PBAC flag inversion likely intendedComment says “we know pbac isn’t enabled on this instance,” but the condition currently requires PBAC to be enabled. In PBAC mode, input.role is typically a custom/default role ID string, so checkAdminOrOwner(input.role as MembershipRole) will be false and this block won’t run. In legacy mode (where the block makes sense), roleManager.isPBACEnabled is false, so the block still won’t run.
Switch to negated PBAC flag.
Apply:
- if (checkAdminOrOwner(input.role as MembershipRole) && roleManager.isPBACEnabled) { + if (checkAdminOrOwner(input.role as MembershipRole) && !roleManager.isPBACEnabled) { const teamIds = requestedMember.team.children .map((sub_team) => sub_team.members.find((item) => item.userId === input.userId)?.teamId) .filter(Boolean) as number[]; //filter out undefined await applyRoleToAllTeams(input.userId, teamIds, input.role as MembershipRole); }packages/features/pbac/services/__tests__/role-management.factory.test.ts (1)
104-129: Add coverage for “team” scope and assert permission slug routingThe new scope adds meaningful behavior: PBAC should check "team.changeMemberRole" and Legacy should consult isTeamAdmin. Add tests to prevent regressions.
Here’s a minimal set of tests to add:
describe("PBACRoleManager - team scope", () => { it("should call checkPermission with team.changeMemberRole", async () => { mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValue(true); mockPermissionCheckService.checkPermission.mockResolvedValue(true); const manager = await factory.createRoleManager(organizationId); await manager.checkPermissionToChangeRole(userId, 42, "team"); expect(mockPermissionCheckService.checkPermission).toHaveBeenCalledWith({ userId, teamId: 42, permission: "team.changeMemberRole", fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN], }); }); }); describe("LegacyRoleManager - team scope", () => { it("should allow when isTeamAdmin returns truthy", async () => { mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValue(false); // Mock isTeamAdmin to return truthy (object or boolean true) const { isTeamAdmin } = await import("@calcom/lib/server/queries/teams"); vi.mocked(isTeamAdmin).mockResolvedValue({ id: 1 } as any); const manager = await factory.createRoleManager(organizationId); await expect(manager.checkPermissionToChangeRole(userId, 42, "team")).resolves.not.toThrow(); }); });Also consider asserting the exact error message for the team scope unauthorized path mirrors the org scope behavior (UNAUTHORIZED).
Also applies to: 184-213
♻️ Duplicate comments (1)
packages/features/pbac/services/role-management.factory.ts (1)
3-3: Missing import previously identified — now correctly addedImporting isTeamAdmin resolves the earlier compile error and enables team-scope checks in LegacyRoleManager.
🧹 Nitpick comments (3)
packages/trpc/server/routers/viewer/organizations/updateUser.handler.ts (1)
163-171: Map RoleManagementError from assignRole to proper TRPC codesassignRole (PBAC path) can throw RoleManagementError with INVALID_ROLE. Currently this would bubble as an internal error; better to convert to a client-safe TRPC error.
Apply:
- await roleManager.assignRole(input.userId, organizationId, input.role, requestedMember.id); + try { + await roleManager.assignRole(input.userId, organizationId, input.role, requestedMember.id); + } catch (error) { + if (error instanceof RoleManagementError) { + // INVALID_ROLE -> BAD_REQUEST, others -> UNAUTHORIZED + const code = (error as RoleManagementError).code === "INVALID_ROLE" ? "BAD_REQUEST" : "UNAUTHORIZED"; + throw new TRPCError({ code, message: error.message }); + } + throw error; + }Also add this import at the top if you choose to branch on the enum directly:
// add near other imports import { RoleManagementErrorCode } from "@calcom/features/pbac/domain/errors/role-management.error";packages/features/pbac/services/__tests__/role-management.factory.test.ts (1)
1-235: Double-check toThrow Error instance usageThese tests assert with
toThrow(new RoleManagementError(...)). Some runners prefertoThrow(RoleManagementError)ortoThrowErrorMatchingInlineSnapshot/Regex. If you see flakiness, switch to type or message-based assertions.packages/features/pbac/services/role-management.factory.ts (1)
155-159: Minor readability nit: avoid comma operator in constructorThis is valid but unconventional; separate statements improve clarity.
- private constructor() { - (this.featuresRepository = new FeaturesRepository(prisma)), (this.roleService = new RoleService()); - this.permissionCheckService = new PermissionCheckService(); - } + private constructor() { + this.featuresRepository = new FeaturesRepository(prisma); + this.roleService = new RoleService(); + this.permissionCheckService = new PermissionCheckService(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/features/pbac/services/__tests__/role-management.factory.test.ts(2 hunks)packages/features/pbac/services/role-management.factory.ts(4 hunks)packages/trpc/server/routers/viewer/organizations/updateUser.handler.ts(1 hunks)packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/trpc/server/routers/viewer/organizations/updateUser.handler.tspackages/features/pbac/services/role-management.factory.tspackages/features/pbac/services/__tests__/role-management.factory.test.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/trpc/server/routers/viewer/organizations/updateUser.handler.tspackages/features/pbac/services/role-management.factory.tspackages/features/pbac/services/__tests__/role-management.factory.test.ts
🧬 Code Graph Analysis (2)
packages/features/pbac/services/role-management.factory.ts (3)
packages/features/pbac/services/permission-check.service.ts (1)
hasPermission(205-234)packages/lib/server/queries/teams/index.ts (1)
isTeamAdmin(387-407)packages/lib/server/queries/organisations/index.ts (1)
isOrganisationAdmin(7-17)
packages/features/pbac/services/__tests__/role-management.factory.test.ts (1)
packages/lib/server/queries/organisations/index.ts (1)
isOrganisationAdmin(7-17)
🔇 Additional comments (10)
packages/trpc/server/routers/viewer/organizations/updateUser.handler.ts (1)
49-51: No remaining outdated calls — LGTM
- Ran
rg -nP --type=ts -C2 '\bcheckPermissionToChangeRole\s*\(\s*[^,]+,\s*[^,]+\s*\)'and found no two-argument invocations.- Please manually inspect any multi-line or computed call sites to ensure all usages include the new
scopeparameter.packages/features/pbac/services/__tests__/role-management.factory.test.ts (5)
113-116: PBAC “org” scope test updated — LGTMUsing the third argument "org" matches the new API and validates the allow path with PBAC enabled.
121-127: PBAC unauthorized “org” scope test updated — LGTMTest correctly reflects the new signature and expected error.
198-201: Legacy “org” scope allow path updated — LGTMValidates OWNER path under legacy gate with the updated method signature.
206-212: Legacy unauthorized “org” scope test updated — LGTMSignature change is reflected and expected error remains unchanged.
1-235: AllcheckPermissionToChangeRolecalls now include the newscopeargumentVerified that every invocation—both in the TRPC handlers and in the PBAC factory tests—passes three parameters (
userId,targetId, and the"org"/"team"scope). No leftover two-argument calls remain.packages/features/pbac/services/role-management.factory.ts (4)
12-15: Interface signature updated — LGTMIRoleManager now models scope explicitly; this improves clarity vs. prior boolean flags.
99-111: Legacy membership resolution by scope — LGTMSwitches to isTeamAdmin for "team" and isOrganisationAdmin for "org". Throws correct UNAUTHORIZED message on failure, preserving legacy behavior.
119-130: Legacy assignRole updates membership.role — sanity checkThis sets MembershipRole directly; in PBAC we assign custom roles via RoleService. Current split remains coherent.
33-47: Verify and centralize permission slugs
- Confirm that the permission identifiers
"team.changeMemberRole"and"organization.changeMemberRole"are defined in your PBAC permission catalog; if they’re missing, all checks will fail.- To avoid typos and streamline future updates, extract these strings into a shared constant or enum. For example:
// packages/features/pbac/constants/permissions.ts export const PERMISSIONS = { TeamChangeMemberRole: "team.changeMemberRole" as const, OrgChangeMemberRole: "organization.changeMemberRole" as const, };Then update your factory to use:
permission: scope === "team" ? PERMISSIONS.TeamChangeMemberRole : PERMISSIONS.OrgChangeMemberRole,
E2E results are ready! |
4 participants
What does this PR do?
Fixes an issue where an org member who is a team admin/owner couldn’t change other team members' roles