chore: refactor role manager factory classes#24146
Conversation
WalkthroughThis change extracts role-management into dedicated modules. It adds IRoleManager (role-manager.interface.ts) and two implementations: LegacyRoleManager (isPBACEnabled=false) and PBACRoleManager (isPBACEnabled=true). LegacyRoleManager validates role changes with explicit rules and uses Prisma to assign roles and retrieve roles. PBACRoleManager delegates permission checks to PermissionCheckService and role queries/assignments to RoleService, supporting enum and custom role IDs. RoleManagementFactory now imports these classes and returns an instance based on a feature flag, removing in-file implementations and the former IRoleManager declaration. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (15)
packages/features/pbac/services/role-manager.interface.ts (1)
3-20: Make the capability flag immutable (+ small API polish).
- Mark isPBACEnabled as readonly; it’s a fixed capability, not state.
- Optional: introduce a RoleRef type to avoid repeating the shape.
export interface IRoleManager { - isPBACEnabled: boolean; + readonly isPBACEnabled: boolean; checkPermissionToChangeRole(If you like, also:
+export type RoleRef = Readonly<{ id: string; name: string }>; - getAllRoles(organizationId: number): Promise<{ id: string; name: string }[]>; - getTeamRoles(teamId: number): Promise<{ id: string; name: string }[]>; + getAllRoles(organizationId: number): Promise<RoleRef[]>; + getTeamRoles(teamId: number): Promise<RoleRef[]>;packages/features/pbac/services/role-management.factory.ts (2)
16-21: Replace comma operator assignment and fix misleading comment.The comma operator hurts readability, and the comment is inaccurate (both dependencies are used). Use clear assignments.
private constructor() { - // 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.permissionCheckService = new PermissionCheckService(); + this.featuresRepository = new FeaturesRepository(prisma); + this.roleService = new RoleService(); + this.permissionCheckService = new PermissionCheckService(); }
30-36: Nit: consider naming consistency.Method takes organizationId but calls checkIfTeamHasFeature. If “org” and “team” are the same entity, consider harmonizing parameter naming for clarity.
packages/features/pbac/services/legacy-role-manager.service.ts (6)
11-12: Capability flag should be immutable.Make isPBACEnabled readonly to reflect a fixed mode.
- public isPBACEnabled = false; + public readonly isPBACEnabled = false;
77-86: Prisma: select only what you need + use enums instead of raw strings.
- Per repo guideline, add select to avoid fetching full rows.
- Prefer MembershipRole.ADMIN/OWNER for type safety.
[coding_guidelines]
- const team = await prisma.membership.findFirst({ + const team = await prisma.membership.findFirst({ where: { userId, teamId: targetId, accepted: true, - OR: [{ role: "ADMIN" }, { role: "OWNER" }], + OR: [{ role: MembershipRole.ADMIN }, { role: MembershipRole.OWNER }], }, + select: { id: true }, });
98-107: Prisma: avoid full-row fetch; select minimal fields.You only use userId and role downstream. Select just those to reduce payload and align with guidelines.
[coding_guidelines]
- const memberships = await prisma.membership.findMany({ + const memberships = await prisma.membership.findMany({ where: { teamId: targetId, accepted: true, }, + select: { userId: true, role: true }, });Note: This changes the shape; update validateRoleChange signature accordingly (see next comment).
13-19: Type narrow validateRoleChange input to match selected fields.After adding select in findMany, adjust the parameter type to what you actually use.
- memberships: Membership[] + memberships: Array<Pick<Membership, "userId" | "role">>
1-9: File naming convention (new .service.ts files).Repo guideline discourages new .service.ts suffixes. Since this file is newly introduced, consider naming without the suffix (e.g., legacy-role-manager.ts). Apply consistently if you decide to align.
[coding_guidelines]
34-66: Behavioral nit: self “no-op” updates.The “admins cannot change themselves to a higher role” check also blocks an ADMIN setting their role to ADMIN (no-op). If no-op updates are allowed elsewhere, consider explicitly allowing newRole === myMembership.role.
- newRole !== MembershipRole.MEMBER + newRole !== MembershipRole.MEMBER && + newRole !== myMembership.rolepackages/features/pbac/services/pbac-role-manager.service.ts (6)
1-1: File naming: avoid new “.service.ts” suffixFor new files we avoid
.service.tssuffixes; preferpbac-role-manager.ts. If keeping the current name for consistency, consider adding a follow-up to align naming across the folder.As per coding guidelines.
49-61: Tighten type checks; removeas any; simplify role resolutionUse explicit key/value sets to distinguish enum keys from default role IDs and avoid
as any. Also early-return once a default role resolves.Apply:
- // Check if role is one of the default MembershipRole enum values - const isDefaultRole = role in DEFAULT_ROLE_IDS; - - // Also check if the role is a default role ID value - const isDefaultRoleId = Object.values(DEFAULT_ROLE_IDS).includes(role as any); - - if (isDefaultRole) { - // Handle enum values like MembershipRole.ADMIN - await this.roleService.assignRoleToMember(DEFAULT_ROLE_IDS[role as MembershipRole], membershipId); - } else if (isDefaultRoleId) { - // Handle default role IDs like "admin_role" - await this.roleService.assignRoleToMember(role as string, membershipId); - } else { + const defaultRoleKeys = Object.keys(DEFAULT_ROLE_IDS) as MembershipRole[]; + const defaultRoleIds = Object.values(DEFAULT_ROLE_IDS) as string[]; + + if (defaultRoleKeys.includes(role as MembershipRole)) { + await this.roleService.assignRoleToMember(DEFAULT_ROLE_IDS[role as MembershipRole], membershipId); + return; + } + if (defaultRoleIds.includes(role as string)) { + await this.roleService.assignRoleToMember(role as string, membershipId); + return; + } + else { // Handle custom roles const roleExists = await this.roleService.roleBelongsToTeam(role as string, organizationId); if (!roleExists) { throw new RoleManagementError( "You do not have access to this role", RoleManagementErrorCode.INVALID_ROLE ); } await this.roleService.assignRoleToMember(role as string, membershipId); }
43-48: Parameter naming is confusing (“organizationId” is used as a teamId)Here and in
getAllRoles, the param namedorganizationIdis passed to team-scoped methods. Consider renaming locally toteamId(and documenting that for PBAC, the org is represented by its org teamId) to reduce confusion.
74-88: DRY up role mapping
getAllRolesandgetTeamRolesduplicate the same map. Extract a small helper to map roles to{id,name}to keep both methods consistent.Example:
private toPublic(roles: { id:string; name:string }[]) { return roles.map(({id,name}) => ({ id, name })); }
21-26: Redundant no-unused-vars disablesArgs are already prefixed with
_. If your ESLint config respects leading underscores, the per-arg disables can be removed. If not, keep them; otherwise delete for cleaner code.
17-48: Enforce permission guard inassignRolefor defense in depth
Although both callers (changeMemberRole.handler.tsandupdateUser.handler.ts) invokecheckPermissionToChangeRolebeforeassignRole, the PBAC manager’sassignRolemethod itself doesn’t perform any permission check. Embed an explicit call tocheckPermissionToChangeRoleinsideassignRole—or codify the caller contract and assert it via unit tests—to avoid relying solely on upstream callers.
📜 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 (4)
packages/features/pbac/services/legacy-role-manager.service.ts(1 hunks)packages/features/pbac/services/pbac-role-manager.service.ts(1 hunks)packages/features/pbac/services/role-management.factory.ts(1 hunks)packages/features/pbac/services/role-manager.interface.ts(1 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.tspackages/features/pbac/services/legacy-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.tspackages/features/pbac/services/role-manager.interface.tspackages/features/pbac/services/legacy-role-manager.service.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/pbac-role-manager.service.tspackages/features/pbac/services/role-manager.interface.tspackages/features/pbac/services/legacy-role-manager.service.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/pbac-role-manager.service.tspackages/features/pbac/services/role-manager.interface.tspackages/features/pbac/services/legacy-role-manager.service.tspackages/features/pbac/services/role-management.factory.ts
🧬 Code graph analysis (3)
packages/features/pbac/services/pbac-role-manager.service.ts (5)
packages/features/pbac/services/role-manager.interface.ts (1)
IRoleManager(3-20)packages/features/pbac/services/role.service.ts (1)
RoleService(12-112)packages/features/pbac/services/permission-check.service.ts (2)
PermissionCheckService(19-306)hasPermission(183-201)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-manager.interface.ts (1)
packages/platform/libraries/index.ts (1)
MembershipRole(34-34)
packages/features/pbac/services/legacy-role-manager.service.ts (4)
packages/features/pbac/services/role-manager.interface.ts (1)
IRoleManager(3-20)packages/features/pbac/domain/errors/role-management.error.ts (1)
RoleManagementError(1-6)packages/features/ee/teams/lib/queries.ts (1)
isTeamOwner(388-397)packages/lib/server/queries/organisations/index.ts (1)
isOrganisationAdmin(7-17)
🔇 Additional comments (1)
packages/features/pbac/services/pbac-role-manager.service.ts (1)
17-41: Verify organization permission key andteamIdusage
- Confirm
organization.changeMemberRoleis supported byPermissionMapper/PERMISSION_REGISTRY(tests coverteam.changeMemberRole; registry definesChangeMemberRole).- Ensure passing
teamId: targetId(orgId) forscope="org"invokes the correct org‐level check inPermissionCheckService.
| hasPermission = !!(await isOrganisationAdmin(userId, targetId)); | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Double-check org-scope acceptance semantics.
isOrganisationAdmin (external) in the provided snippet doesn’t filter accepted: true. If that’s accurate, a pending admin invite could pass this check. Either ensure that helper enforces accepted, or mirror the accepted: true constraint here.
🏁 Script executed:
#!/bin/bash
# Verify isOrganisationAdmin enforces accepted=true
rg -n -C3 --type=ts '\bisOrganisationAdmin\s*\(' packages/lib/server/queries/organisations | sed -n '1,120p'Length of output: 628
🏁 Script executed:
#!/bin/bash
# Show implementation of isOrganisationAdmin around the findFirst call
rg -n -C5 'membership.findFirst' packages/lib/server/queries/organisations/index.tsLength of output: 675
🏁 Script executed:
#!/bin/bash
# Locate Prisma schema files
rg -l 'datasource' -g '*.prisma'Length of output: 82
🏁 Script executed:
#!/bin/bash
# Extract Membership model from Prisma schema to verify acceptance field
rg -n -C5 'model Membership' packages/prisma/schema.prismaLength of output: 310
Enforce accepted=true in isOrganisationAdmin
isOrganisationAdmin currently omits the accepted filter, so pending invites (accepted: false) with ADMIN/OWNER role would pass. Add accepted: true to the where clause in packages/lib/server/queries/organisations/index.ts to restrict it to accepted memberships.
🤖 Prompt for AI Agents
In packages/features/pbac/services/legacy-role-manager.service.ts around lines
87-89, the call to isOrganisationAdmin can currently return true for pending
invites because the underlying query omits an accepted filter; edit
packages/lib/server/queries/organisations/index.ts and add accepted: true to the
membership where clause used by isOrganisationAdmin so only accepted memberships
(not pending invites) with ADMIN/OWNER roles satisfy the check.
| async assignRole( | ||
| userId: number, | ||
| organizationId: number, | ||
| role: MembershipRole | string, | ||
| // Used in other implementation | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| _membershipId: number | ||
| ): Promise<void> { | ||
| await prisma.membership.update({ | ||
| where: { | ||
| userId_teamId: { | ||
| userId, | ||
| teamId: organizationId, | ||
| }, | ||
| }, | ||
| data: { | ||
| role: role as MembershipRole, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Guard against custom roles in legacy mode before updating DB.
assignRole blindly casts role to MembershipRole. If a custom PBAC role string is passed while PBAC is disabled, this will break at runtime. Validate and throw a domain error instead.
async assignRole(
userId: number,
organizationId: number,
role: MembershipRole | string,
// Used in other implementation
// eslint-disable-next-line @typescript-eslint/no-unused-vars
_membershipId: number
): Promise<void> {
+ // Legacy mode only accepts classic membership roles
+ if (!Object.values(MembershipRole).includes(role as MembershipRole)) {
+ throw new RoleManagementError(
+ "Custom roles are not supported in legacy mode",
+ RoleManagementErrorCode.UNAUTHORIZED
+ );
+ }
await prisma.membership.update({
where: {
userId_teamId: {
userId,
teamId: organizationId,
},
},
data: {
role: role as MembershipRole,
},
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async assignRole( | |
| userId: number, | |
| organizationId: number, | |
| role: MembershipRole | string, | |
| // Used in other implementation | |
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | |
| _membershipId: number | |
| ): Promise<void> { | |
| await prisma.membership.update({ | |
| where: { | |
| userId_teamId: { | |
| userId, | |
| teamId: organizationId, | |
| }, | |
| }, | |
| data: { | |
| role: role as MembershipRole, | |
| }, | |
| }); | |
| } | |
| async assignRole( | |
| userId: number, | |
| organizationId: number, | |
| role: MembershipRole | string, | |
| // Used in other implementation | |
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | |
| _membershipId: number | |
| ): Promise<void> { | |
| // Legacy mode only accepts classic membership roles | |
| if (!Object.values(MembershipRole).includes(role as MembershipRole)) { | |
| throw new RoleManagementError( | |
| "Custom roles are not supported in legacy mode", | |
| RoleManagementErrorCode.UNAUTHORIZED | |
| ); | |
| } | |
| await prisma.membership.update({ | |
| where: { | |
| userId_teamId: { | |
| userId, | |
| teamId: organizationId, | |
| }, | |
| }, | |
| data: { | |
| role: role as MembershipRole, | |
| }, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In packages/features/pbac/services/legacy-role-manager.service.ts around lines
110 to 129, assignRole currently casts the incoming role to MembershipRole
unconditionally; validate the incoming role first and reject custom PBAC role
strings when PBAC is disabled by checking whether the provided role is a valid
MembershipRole value (e.g., compare against Object.values(MembershipRole) or the
enum members) and if it is not, throw a domain error (use the app's DomainError
type or a clearly named error) instead of proceeding; only after validation cast
to MembershipRole and perform the prisma.membership.update.
| async assignRole( | ||
| _userId: number, | ||
| organizationId: number, | ||
| role: MembershipRole | string, | ||
| membershipId: number | ||
| ): Promise<void> { | ||
| // Check if role is one of the default MembershipRole enum values | ||
| const isDefaultRole = role in DEFAULT_ROLE_IDS; | ||
|
|
||
| // Also check if the role is a default role ID value | ||
| const isDefaultRoleId = Object.values(DEFAULT_ROLE_IDS).includes(role as any); | ||
|
|
||
| if (isDefaultRole) { | ||
| // Handle enum values like MembershipRole.ADMIN | ||
| await this.roleService.assignRoleToMember(DEFAULT_ROLE_IDS[role as MembershipRole], membershipId); | ||
| } else if (isDefaultRoleId) { | ||
| // Handle default role IDs like "admin_role" | ||
| await this.roleService.assignRoleToMember(role as string, membershipId); | ||
| } else { | ||
| // Handle custom roles | ||
| const roleExists = await this.roleService.roleBelongsToTeam(role as string, organizationId); | ||
| if (!roleExists) { | ||
| throw new RoleManagementError( | ||
| "You do not have access to this role", | ||
| RoleManagementErrorCode.INVALID_ROLE | ||
| ); | ||
| } | ||
| await this.roleService.assignRoleToMember(role as string, membershipId); | ||
| } | ||
| } |
There was a problem hiding this comment.
Membership-to-target validation missing (risk: cross-tenant assignment)
assignRole never verifies that membershipId belongs to the target team/org (organizationId here). If a caller supplies a membership from another team, this could assign a default or custom role across boundaries. Add a guard that asserts the membership is within the target team (or the org tree, as intended) before any assignment.
Apply this change locally (call-site here), and add a helper in RoleService:
async assignRole(
_userId: number,
organizationId: number,
role: MembershipRole | string,
membershipId: number
): Promise<void> {
+ // Ensure membership belongs to the target team/org before assigning any role
+ const isMemberInTarget = await this.roleService.membershipBelongsToTeamOrOrg(membershipId, organizationId);
+ if (!isMemberInTarget) {
+ throw new RoleManagementError(
+ "Membership does not belong to target organization/team",
+ RoleManagementErrorCode.UNAUTHORIZED
+ );
+ }Add this method to RoleService (example):
// packages/features/pbac/services/role.service.ts
async membershipBelongsToTeamOrOrg(membershipId: number, teamOrOrgId: number) {
const membership = await db.membership.findUnique({
where: { id: membershipId },
select: { teamId: true, team: { select: { parentId: true } } }, // select-only, no include
});
if (!membership) return false;
const { teamId, team } = membership;
return teamId === teamOrOrgId || team?.parentId === teamOrOrgId;
}I can wire this through and update unit tests if you want a follow-up PR. As per coding guidelines (Prisma: use select).
volnei
left a comment
volnei
left a comment
There was a problem hiding this comment.
It's good to remove all // eslint-disable-next-line @typescript-eslint/no-unused-vars
Other than that looks good
E2E results are ready! |
3 participants
What does this PR do?
PR refactors role manager factory into seperate folders for better SOC