[permissions] forbid deletion of last admin user #10504
Conversation
![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 protection against deleting the last admin user in a workspace, ensuring workspace continuity.
- Added new
NO_ROLE_FOUND_FOR_USER_WORKSPACEerror code inpermissions.exception.tsfor proper error handling - Implemented
validateUserWorkspaceIsNotUniqueAdminOrThrowmethod inuser-role.service.tsto check if a user is the last admin - Enhanced
deleteUsermethod inuser.service.tsto prevent deletion of the last admin with clear error messaging - Added
UserWorkspaceModuletoUserModuleimports to access necessary services for admin validation - Created integration test in
user.integration-spec.tsto verify the functionality works correctly
6 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
| public async validateUserWorkspaceIsNotUniqueAdminOrThrow({ | ||
| userWorkspaceId, | ||
| workspaceId, | ||
| }) { |
There was a problem hiding this comment.
style: Missing type definition for method parameters. Consider adding explicit types: { userWorkspaceId: string, workspaceId: string }
| public async validateUserWorkspaceIsNotUniqueAdminOrThrow({ | |
| userWorkspaceId, | |
| workspaceId, | |
| }) { | |
| public async validateUserWorkspaceIsNotUniqueAdminOrThrow({ | |
| userWorkspaceId, | |
| workspaceId, | |
| }: { userWorkspaceId: string; workspaceId: string }) { |
| private async validateMoreThanOneWorkspaceMemberHasAdminRoleOrThrow({ | ||
| adminRoleId, | ||
| workspaceId, | ||
| }) { |
There was a problem hiding this comment.
style: Missing type definition for method parameters. Consider adding explicit types: { adminRoleId: string, workspaceId: string }
| private async validateMoreThanOneWorkspaceMemberHasAdminRoleOrThrow({ | |
| adminRoleId, | |
| workspaceId, | |
| }) { | |
| private async validateMoreThanOneWorkspaceMemberHasAdminRoleOrThrow({ | |
| adminRoleId, | |
| workspaceId, | |
| }: { adminRoleId: string; workspaceId: string }) { |
packages/twenty-server/src/engine/metadata-modules/user-role/user-role.service.ts
Show resolved
Hide resolved
| FeatureFlagKey.IsPermissionsEnabled, | ||
| workspaceId, | ||
| ); | ||
|
|
||
| const workspaceMembers = await workspaceDataSource?.query( |
There was a problem hiding this comment.
It seems you fetch workspaceMembers here but you don't do anything with them if permission is not enabled. Is it intended? I think we still want to keep the old logic in case permission is not enabled
There was a problem hiding this comment.
oops i misplaced the end of the if block. thanks for spotting that
| }); | ||
| } catch (error: any) { | ||
| if ( | ||
| error instanceof PermissionsException && |
There was a problem hiding this comment.
Not sure about this logic? Is it only to specify a different message for the exception or am I missing something? It seems you are catching an exception to re-throw the same?
There was a problem hiding this comment.
Yes it's only to change the message exception to have something more relevant, with more context. This message will be shown to users in production when attempting to delete their account in this situation.
the other solution was to prevent account deletion in the product, but it required to compute roles of the user's other workspaces etc. to determine whether or not their account will be deletable, + to store that information in a new graphql field. seemed a bit overkill. wdyt?
There was a problem hiding this comment.
I see, should we re-throw a different code then?
CANNOT_DELETE_LAST_ADMIN?
Not a strong requirement, up to you
A user should not be able to delete their account if they are the last admin of a workspace.
It means that if a user wants to sign out of twenty, they should delete their workspace, not their account