Implement updateRole #10009
Implement updateRole #10009
Conversation
| } | ||
|
|
||
| const userWorkspace = userWorkspaces?.find( |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR implements a comprehensive role management system with permission checks for workspace members, focusing on role assignments and admin role protection.
- Added
updateWorkspaceMemberRolemutation in/packages/twenty-server/src/engine/metadata-modules/role/role.resolver.tswith admin permission validation - Implemented safeguards in
/packages/twenty-server/src/engine/metadata-modules/userRole/userRole.service.tsto prevent unassigning the last admin role - Enhanced
WorkspaceMemberDTO in/packages/twenty-server/src/engine/core-modules/user/dtos/workspace-member.dto.tswith roles array and userWorkspaceId fields - Added new permission exception codes in
/packages/twenty-server/src/engine/metadata-modules/permissions/permissions.exception.tsfor role-specific error handling - Modified
workspaceMembersresolver in/packages/twenty-server/src/engine/core-modules/user/user.resolver.tsto include role information
7 file(s) reviewed, 9 comment(s)
Edit PR Review Bot Settings | Greptile
| const avatarUrlToken = await this.fileService.encodeFileToken({ | ||
| workspaceMemberId: workspaceMember.id, | ||
| workspaceMemberEntityId: workspaceMemberEntity.id, |
There was a problem hiding this comment.
logic: workspaceMemberEntityId is used here but workspaceMemberId is used on line 150. This inconsistency could cause issues with file token generation.
| workspaceMemberEntityId: workspaceMemberEntity.id, | |
| workspaceMemberId: workspaceMemberEntity.id, |
| const role = await this.userRoleService | ||
| .getRolesForUserWorkspace(userWorkspace.id) | ||
| .then(([roleEntity]) => { |
There was a problem hiding this comment.
style: getRolesForUserWorkspace returns an array but only the first role is used. This should be documented or handled explicitly if multiple roles are not supported.
| if (!userWorkspace) { | ||
| throw new Error('User workspace not found'); | ||
| } |
There was a problem hiding this comment.
style: This error should include more context like the userWorkspace.id to help with debugging
| if (!isDefined(roleId)) { | ||
| return this.userRoleService.unassignRolesFromWorkspaceMember({ | ||
| workspaceMemberId, | ||
| workspaceId: workspace.id, | ||
| }); | ||
| } |
There was a problem hiding this comment.
logic: Missing validation to prevent unassigning the last admin role from a workspace. This could leave the workspace without any admin users.
| return this.userRoleService.assignRoleToWorkspaceMember({ | ||
| workspaceId: workspace.id, | ||
| workspaceMemberId, | ||
| roleId, | ||
| }); |
There was a problem hiding this comment.
style: No error handling around the assignRoleToWorkspaceMember call. Should wrap in try/catch to handle potential database errors or role assignment failures gracefully.
| @Args('roleId', { type: () => String, nullable: true }) | ||
| roleId: string | null, |
There was a problem hiding this comment.
style: Consider validating that roleId exists in the workspace before attempting assignment. Currently no validation that the role is valid.
| ): Promise<void> { | ||
| const roles = await this.getRolesForUserWorkspace(userWorkspaceId); | ||
|
|
||
| const adminRole = roles.find((role: RoleDTO) => role.isEditable === false); |
There was a problem hiding this comment.
logic: isEditable === false is not a reliable way to identify admin roles. Consider adding an explicit isAdmin flag or role type enum
| if (!isEmpty(userWorkspaceRoles)) { | ||
| await Promise.all( | ||
| userWorkspaceRoles.map((userWorkspaceRole) => | ||
| this.userWorkspaceRoleRepository.delete({ | ||
| id: userWorkspaceRole.id, | ||
| }), | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
logic: Transaction needed here to ensure atomic role unassignment when multiple roles exist
| const [roleOfUserWorkspace] = | ||
| await this.userRoleService.getRolesForUserWorkspace(userWorkspaceId); |
There was a problem hiding this comment.
logic: Array destructuring assumes first role is the most relevant. Consider what happens if user has multiple roles with different permissions - could lead to incorrect permission checks.
| const [roleOfUserWorkspace] = | |
| await this.userRoleService.getRolesForUserWorkspace(userWorkspaceId); | |
| const userWorkspaceRoles = | |
| await this.userRoleService.getRolesForUserWorkspace(userWorkspaceId); |
In this PR, we are implementing the updateRole endpoint with the following rules