fix: remove legacy logic into legacy service#24095
Conversation
WalkthroughThe role-management API signatures were extended so 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. Comment |
dosubot
bot
added
teams
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts (1)
48-52: Scope the membership lookup to the fields we actually useWe only need the membership id for the subsequent
assignRolecall. Narrowing the query avoids pulling unnecessary data and keeps us aligned with our Prisma usage guidelines.Apply this diff:
const targetMembership = await prisma.membership.findUnique({ where: { userId_teamId: { userId: input.memberId, teamId: input.teamId }, }, + select: { id: true }, });As per coding guidelines
📜 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/role-management.factory.ts(8 hunks)packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.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/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
**/*.{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/trpc/server/routers/viewer/teams/changeMemberRole.handler.tspackages/features/pbac/services/role-management.factory.ts
🧬 Code graph analysis (1)
packages/features/pbac/services/role-management.factory.ts (2)
packages/features/pbac/domain/errors/role-management.error.ts (1)
RoleManagementError(1-6)packages/features/ee/teams/lib/queries.ts (1)
isTeamOwner(388-397)
⏰ 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
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
packages/features/pbac/services/role-management.factory.ts (6)
15-21: Clarify what “memberId” represents (userId vs membership.id) and document itAmbiguity here risks misuse at call sites. If it’s the target user’s userId, rename to targetUserId or add JSDoc.
Would you confirm that memberId is the target user’s userId (not membership.id) at all call sites? I can update names/docs accordingly.
124-128: Simplify enum check (type guard is redundant)Both enum values and custom roles are strings at runtime. The typeof check can be dropped.
Apply:
- if (typeof newRole !== "string" || !Object.values(MembershipRole).includes(newRole as MembershipRole)) { + if (!Object.values(MembershipRole).includes(newRole as MembershipRole)) { return; }
138-141: Avoid extra DB round-trip; use myMembership for owner checkYou already loaded memberships. Use myMembership instead of isTeamOwner().
Apply:
- if (newRole === MembershipRole.OWNER && !(await isTeamOwner(userId, teamId))) { + if (newRole === MembershipRole.OWNER && myMembership?.role !== MembershipRole.OWNER) { throw new RoleManagementError("Only owners can award owner role", RoleManagementErrorCode.UNAUTHORIZED); }
159-169: Self-change rule likely too strict (blocks ADMIN -> ADMIN no-op)Current condition blocks any self-change except demotion to MEMBER. If intent is “admins cannot promote themselves” (esp. to OWNER), narrow the condition.
Option A (only block promotion to OWNER):
- if ( - myMembership?.role === MembershipRole.ADMIN && - memberId === userId && - newRole !== MembershipRole.MEMBER - ) { + if ( + myMembership?.role === MembershipRole.ADMIN && + memberId === userId && + newRole === MembershipRole.OWNER + ) { throw new RoleManagementError( "You can not change yourself to a higher role.", RoleManagementErrorCode.UNAUTHORIZED ); }If broader “no upward moves” is desired, compare role precedence via a small rank map; I can add that.
203-211: Prisma: select only required fieldsReturn only userId and role for validation to reduce data surface.
Apply (and mirror this pattern in the findFirst above to select a minimal field, e.g., id). As per coding guidelines.
- const memberships = await prisma.membership.findMany({ - where: { - teamId: targetId, - accepted: true, - }, - }); + const memberships = await prisma.membership.findMany({ + where: { teamId: targetId, accepted: true }, + select: { userId: true, role: true }, + });
265-267: Avoid comma operator and no-unused-expressions suppressionUse plain assignments; drop the lint suppression.
Apply:
- // Not used but needed for DI - // eslint-disable-next-line @typescript-eslint/no-unused-expressions - (this.featuresRepository = new FeaturesRepository(prisma)), (this.roleService = new RoleService()); + this.featuresRepository = new FeaturesRepository(prisma); + this.roleService = new RoleService();
📜 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/role-management.factory.ts(8 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/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
**/*.{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/role-management.factory.ts
🧬 Code graph analysis (1)
packages/features/pbac/services/role-management.factory.ts (2)
packages/features/pbac/domain/errors/role-management.error.ts (1)
RoleManagementError(1-6)packages/features/ee/teams/lib/queries.ts (1)
isTeamOwner(388-397)
⏰ 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/role-management.factory.ts (2)
203-211: Good fix: filter to accepted memberships for owner-guardThis addresses the prior bug about counting pending owners; thanks for tightening the query.
256-257: Explicit export of LegacyRoleManagerMakes testing and DI easier. LGTM.
| async checkPermissionToChangeRole( | ||
| userId: number, | ||
| targetId: number, | ||
| scope: "org" | "team", | ||
| // Not required for this instance | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| _memberId?: number, | ||
| // Not required for this instance | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| _newRole?: MembershipRole | string | ||
| ): Promise<void> { |
There was a problem hiding this comment.
🧩 Analysis chain
Verify org-scope permission path with PermissionCheckService
checkPermission expects a teamId; for scope="org" we still pass targetId as teamId with permission "organization.changeMemberRole". Confirm this is supported by PermissionCheckService.
Run to locate definitions/usage of these permission strings and validate org handling:
🏁 Script executed:
#!/bin/bash
rg -nP -C2 '\borganization\.changeMemberRole\b|\bteam\.changeMemberRole\b'
rg -nP -C3 'class\s+PermissionCheckService\b(?s).*?checkPermission\s*\('Length of output: 2930
🏁 Script executed:
#!/bin/bash
# Locate PermissionCheckService and show its checkPermission signature
perm_file=$(rg -lP 'class\s+PermissionCheckService\b')
echo "PermissionCheckService file: $perm_file"
rg -nP -C3 'class\s+PermissionCheckService\b' "$perm_file"
rg -nP -C3 'checkPermission\s*\(' "$perm_file"
# Check if 'organization.changeMemberRole' is registered as a valid permission
echo -e "\nRegistry entries for organization.changeMemberRole:"
rg -nP 'organization\.changeMemberRole' -g 'packages/features/pbac/domain/types/**'
# Show organization permissions section in README.md
echo -e "\nOrganization permissions in README.md:"
sed -n '360,410p' packages/features/pbac/README.mdLength of output: 2154
Add missing “organization.changeMemberRole” permission and support org context
The factory uses "organization.changeMemberRole", but this permission isn’t registered (domain/types, README or tests) and PermissionCheckService only accepts a teamId. Register the new permission, extend the test suite, and update checkPermission to handle organization-level checks (e.g. accept an orgId or dispatch accordingly).
🤖 Prompt for AI Agents
In packages/features/pbac/services/role-management.factory.ts around lines
40-50, the permission "organization.changeMemberRole" is referenced but not
registered and the permission check only supports teamId; register the new
permission constant in the domain/types (and document it in README and tests),
update PermissionCheckService to accept an organization context (e.g., orgId) or
add a separate org check method, then modify checkPermissionToChangeRole to
accept/route when scope === "org" (pass orgId to the permission check with
permission "organization.changeMemberRole") and keep the existing team flow for
scope === "team"; update and add unit tests to cover organization-level
permission checks and the new permission registration.
E2E results are ready! |
3 participants
What does this PR do?
This PR moves legacy role logic into the legacy role service as it doesnt need to be ran when we are dealing with PBAC
s.
How should this be tested?
Impersonate a user like "teampro"
Make users member,admin,owner -> verify
impersonate a admin -> verify cannot make owner