From 17dbb634cadec6c0085dab09272a1e11e13a80d9 Mon Sep 17 00:00:00 2001 From: Marie <51697796+ijreilly@users.noreply.github.com> Date: Thu, 27 Feb 2025 12:44:51 +0100 Subject: [PATCH] [permissions] forbid deletion of last admin user (#10504) 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 --- .../services/__tests__/user.service.spec.ts | 62 ------------------- .../user/services/user.service.ts | 51 ++++++++++++++- .../engine/core-modules/user/user.module.ts | 2 + .../engine/core-modules/user/user.resolver.ts | 4 +- .../permissions/permissions.exception.ts | 4 ++ ...sion-graphql-api-exception-handler.util.ts | 1 + .../user-role/user-role.service.ts | 44 ++++++++++++- .../graphql/suites/user.integration-spec.ts | 55 ++++++++++++++++ 8 files changed, 158 insertions(+), 65 deletions(-) delete mode 100644 packages/twenty-server/src/engine/core-modules/user/services/__tests__/user.service.spec.ts create mode 100644 packages/twenty-server/test/integration/graphql/suites/user.integration-spec.ts diff --git a/packages/twenty-server/src/engine/core-modules/user/services/__tests__/user.service.spec.ts b/packages/twenty-server/src/engine/core-modules/user/services/__tests__/user.service.spec.ts deleted file mode 100644 index 1dc841451..000000000 --- a/packages/twenty-server/src/engine/core-modules/user/services/__tests__/user.service.spec.ts +++ /dev/null @@ -1,62 +0,0 @@ -import { Test, TestingModule } from '@nestjs/testing'; -import { getRepositoryToken } from '@nestjs/typeorm'; - -import { TypeORMService } from 'src/database/typeorm/typeorm.service'; -import { UserWorkspace } from 'src/engine/core-modules/user-workspace/user-workspace.entity'; -import { UserService } from 'src/engine/core-modules/user/services/user.service'; -import { User } from 'src/engine/core-modules/user/user.entity'; -import { WorkspaceService } from 'src/engine/core-modules/workspace/services/workspace.service'; -import { DataSourceService } from 'src/engine/metadata-modules/data-source/data-source.service'; -import { TwentyORMGlobalManager } from 'src/engine/twenty-orm/twenty-orm-global.manager'; -import { WorkspaceEventEmitter } from 'src/engine/workspace-event-emitter/workspace-event-emitter'; -import { ObjectMetadataEntity } from 'src/engine/metadata-modules/object-metadata/object-metadata.entity'; - -describe('UserService', () => { - let service: UserService; - - beforeEach(async () => { - const module: TestingModule = await Test.createTestingModule({ - providers: [ - UserService, - { - provide: getRepositoryToken(User, 'core'), - useValue: {}, - }, - { - provide: getRepositoryToken(UserWorkspace, 'core'), - useValue: {}, - }, - { - provide: getRepositoryToken(ObjectMetadataEntity, 'metadata'), - useValue: {}, - }, - { - provide: DataSourceService, - useValue: {}, - }, - { - provide: TypeORMService, - useValue: {}, - }, - { - provide: WorkspaceEventEmitter, - useValue: {}, - }, - { - provide: WorkspaceService, - useValue: {}, - }, - { - provide: TwentyORMGlobalManager, - useValue: {}, - }, - ], - }).compile(); - - service = module.get(UserService); - }); - - it('should be defined', () => { - expect(service).toBeDefined(); - }); -}); diff --git a/packages/twenty-server/src/engine/core-modules/user/services/user.service.ts b/packages/twenty-server/src/engine/core-modules/user/services/user.service.ts index ac4226702..03bb2f104 100644 --- a/packages/twenty-server/src/engine/core-modules/user/services/user.service.ts +++ b/packages/twenty-server/src/engine/core-modules/user/services/user.service.ts @@ -12,12 +12,21 @@ import { AuthException, AuthExceptionCode, } from 'src/engine/core-modules/auth/auth.exception'; +import { FeatureFlagKey } from 'src/engine/core-modules/feature-flag/enums/feature-flag-key.enum'; +import { FeatureFlagService } from 'src/engine/core-modules/feature-flag/services/feature-flag.service'; +import { UserWorkspaceService } from 'src/engine/core-modules/user-workspace/user-workspace.service'; import { User } from 'src/engine/core-modules/user/user.entity'; import { userValidator } from 'src/engine/core-modules/user/user.validate'; import { WorkspaceService } from 'src/engine/core-modules/workspace/services/workspace.service'; import { Workspace } from 'src/engine/core-modules/workspace/workspace.entity'; import { DataSourceService } from 'src/engine/metadata-modules/data-source/data-source.service'; import { ObjectMetadataEntity } from 'src/engine/metadata-modules/object-metadata/object-metadata.entity'; +import { + PermissionsException, + PermissionsExceptionCode, + PermissionsExceptionMessage, +} from 'src/engine/metadata-modules/permissions/permissions.exception'; +import { UserRoleService } from 'src/engine/metadata-modules/user-role/user-role.service'; import { TwentyORMGlobalManager } from 'src/engine/twenty-orm/twenty-orm-global.manager'; import { WorkspaceEventEmitter } from 'src/engine/workspace-event-emitter/workspace-event-emitter'; import { WorkspaceMemberWorkspaceEntity } from 'src/modules/workspace-member/standard-objects/workspace-member.workspace-entity'; @@ -34,6 +43,9 @@ export class UserService extends TypeOrmQueryService { private readonly workspaceEventEmitter: WorkspaceEventEmitter, private readonly workspaceService: WorkspaceService, private readonly twentyORMGlobalManager: TwentyORMGlobalManager, + private readonly userRoleService: UserRoleService, + private readonly userWorkspaceService: UserWorkspaceService, + private readonly featureFlagService: FeatureFlagService, ) { super(userRepository); } @@ -85,9 +97,28 @@ export class UserService extends TypeOrmQueryService { const workspaceDataSource = await this.typeORMService.connectToDataSource(dataSourceMetadata); + const isPermissionsEnabled = await this.featureFlagService.isFeatureEnabled( + FeatureFlagKey.IsPermissionsEnabled, + workspaceId, + ); + const workspaceMembers = await workspaceDataSource?.query( `SELECT * FROM ${dataSourceMetadata.schema}."workspaceMember"`, ); + + if (isPermissionsEnabled && workspaceMembers.length > 1) { + const userWorkspace = + await this.userWorkspaceService.getUserWorkspaceForUserOrThrow({ + userId, + workspaceId, + }); + + await this.userRoleService.validateUserWorkspaceIsNotUniqueAdminOrThrow({ + workspaceId, + userWorkspaceId: userWorkspace.id, + }); + } + const workspaceMember = workspaceMembers.filter( (member: WorkspaceMemberWorkspaceEntity) => member.userId === userId, )?.[0]; @@ -138,7 +169,25 @@ export class UserService extends TypeOrmQueryService { userValidator.assertIsDefinedOrThrow(user); await Promise.all( - user.workspaces.map(this.deleteUserFromWorkspace.bind(this)), + user.workspaces.map(async (userWorkspace) => { + try { + await this.deleteUserFromWorkspace({ + userId, + workspaceId: userWorkspace.workspaceId, + }); + } catch (error: any) { + if ( + error instanceof PermissionsException && + error.code === PermissionsExceptionCode.CANNOT_UNASSIGN_LAST_ADMIN + ) { + throw new PermissionsException( + PermissionsExceptionMessage.CANNOT_DELETE_LAST_ADMIN_USER, + PermissionsExceptionCode.CANNOT_DELETE_LAST_ADMIN_USER, + ); + } + throw error; + } + }), ); return user; diff --git a/packages/twenty-server/src/engine/core-modules/user/user.module.ts b/packages/twenty-server/src/engine/core-modules/user/user.module.ts index e06e4c769..823c6844e 100644 --- a/packages/twenty-server/src/engine/core-modules/user/user.module.ts +++ b/packages/twenty-server/src/engine/core-modules/user/user.module.ts @@ -15,6 +15,7 @@ import { FileModule } from 'src/engine/core-modules/file/file.module'; import { KeyValuePair } from 'src/engine/core-modules/key-value-pair/key-value-pair.entity'; import { OnboardingModule } from 'src/engine/core-modules/onboarding/onboarding.module'; import { UserWorkspace } from 'src/engine/core-modules/user-workspace/user-workspace.entity'; +import { UserWorkspaceModule } from 'src/engine/core-modules/user-workspace/user-workspace.module'; import { UserVarsModule } from 'src/engine/core-modules/user/user-vars/user-vars.module'; import { User } from 'src/engine/core-modules/user/user.entity'; import { UserResolver } from 'src/engine/core-modules/user/user.resolver'; @@ -50,6 +51,7 @@ import { UserService } from './services/user.service'; UserRoleModule, FeatureFlagModule, PermissionsModule, + UserWorkspaceModule, ], exports: [UserService], providers: [UserService, UserResolver, TypeORMService], diff --git a/packages/twenty-server/src/engine/core-modules/user/user.resolver.ts b/packages/twenty-server/src/engine/core-modules/user/user.resolver.ts index 91ea29024..04e1bed0d 100644 --- a/packages/twenty-server/src/engine/core-modules/user/user.resolver.ts +++ b/packages/twenty-server/src/engine/core-modules/user/user.resolver.ts @@ -1,4 +1,4 @@ -import { UseGuards } from '@nestjs/common'; +import { UseFilters, UseGuards } from '@nestjs/common'; import { Args, Mutation, @@ -50,6 +50,7 @@ import { OriginHeader } from 'src/engine/decorators/auth/origin-header.decorator import { WorkspaceAuthGuard } from 'src/engine/guards/workspace-auth.guard'; import { SettingsPermissions } from 'src/engine/metadata-modules/permissions/constants/settings-permissions.constants'; import { PermissionsService } from 'src/engine/metadata-modules/permissions/permissions.service'; +import { PermissionsGraphqlApiExceptionFilter } from 'src/engine/metadata-modules/permissions/utils/permissions-graphql-api-exception.filter'; import { RoleDTO } from 'src/engine/metadata-modules/role/dtos/role.dto'; import { UserRoleService } from 'src/engine/metadata-modules/user-role/user-role.service'; import { AccountsToReconnectKeys } from 'src/modules/connected-account/types/accounts-to-reconnect-key-value.type'; @@ -65,6 +66,7 @@ const getHMACKey = (email?: string, key?: string | null) => { @UseGuards(WorkspaceAuthGuard) @Resolver(() => User) +@UseFilters(PermissionsGraphqlApiExceptionFilter) export class UserResolver { constructor( @InjectRepository(User, 'core') diff --git a/packages/twenty-server/src/engine/metadata-modules/permissions/permissions.exception.ts b/packages/twenty-server/src/engine/metadata-modules/permissions/permissions.exception.ts index 42535a62b..542d9b0d1 100644 --- a/packages/twenty-server/src/engine/metadata-modules/permissions/permissions.exception.ts +++ b/packages/twenty-server/src/engine/metadata-modules/permissions/permissions.exception.ts @@ -16,9 +16,11 @@ export enum PermissionsExceptionCode { WORKSPACE_MEMBER_NOT_FOUND = 'WORKSPACE_MEMBER_NOT_FOUND', ROLE_NOT_FOUND = 'ROLE_NOT_FOUND', CANNOT_UNASSIGN_LAST_ADMIN = 'CANNOT_UNASSIGN_LAST_ADMIN', + CANNOT_DELETE_LAST_ADMIN_USER = 'CANNOT_DELETE_LAST_ADMIN_USER', UNKNOWN_OPERATION_NAME = 'UNKNOWN_OPERATION_NAME', UNKNOWN_REQUIRED_PERMISSION = 'UNKNOWN_REQUIRED_PERMISSION', CANNOT_UPDATE_SELF_ROLE = 'CANNOT_UPDATE_SELF_ROLE', + NO_ROLE_FOUND_FOR_USER_WORKSPACE = 'NO_ROLE_FOUND_FOR_USER_WORKSPACE', } export enum PermissionsExceptionMessage { @@ -31,7 +33,9 @@ export enum PermissionsExceptionMessage { WORKSPACE_MEMBER_NOT_FOUND = 'Workspace member not found', ROLE_NOT_FOUND = 'Role not found', CANNOT_UNASSIGN_LAST_ADMIN = 'Cannot unassign admin role from last admin of the workspace', + CANNOT_DELETE_LAST_ADMIN_USER = 'Cannot delete account: user is the unique admin of a workspace', UNKNOWN_OPERATION_NAME = 'Unknown operation name, cannot determine required permission', UNKNOWN_REQUIRED_PERMISSION = 'Unknown required permission', CANNOT_UPDATE_SELF_ROLE = 'Cannot update self role', + NO_ROLE_FOUND_FOR_USER_WORKSPACE = 'No role found for userWorkspace', } diff --git a/packages/twenty-server/src/engine/metadata-modules/permissions/utils/permission-graphql-api-exception-handler.util.ts b/packages/twenty-server/src/engine/metadata-modules/permissions/utils/permission-graphql-api-exception-handler.util.ts index d960f0ade..b05a63ae6 100644 --- a/packages/twenty-server/src/engine/metadata-modules/permissions/utils/permission-graphql-api-exception-handler.util.ts +++ b/packages/twenty-server/src/engine/metadata-modules/permissions/utils/permission-graphql-api-exception-handler.util.ts @@ -15,6 +15,7 @@ export const permissionGraphqlApiExceptionHandler = ( case PermissionsExceptionCode.PERMISSION_DENIED: case PermissionsExceptionCode.CANNOT_UNASSIGN_LAST_ADMIN: case PermissionsExceptionCode.CANNOT_UPDATE_SELF_ROLE: + case PermissionsExceptionCode.CANNOT_DELETE_LAST_ADMIN_USER: throw new ForbiddenError(error.message); case PermissionsExceptionCode.ROLE_NOT_FOUND: case PermissionsExceptionCode.USER_WORKSPACE_NOT_FOUND: diff --git a/packages/twenty-server/src/engine/metadata-modules/user-role/user-role.service.ts b/packages/twenty-server/src/engine/metadata-modules/user-role/user-role.service.ts index fbc653063..3d2ddcf82 100644 --- a/packages/twenty-server/src/engine/metadata-modules/user-role/user-role.service.ts +++ b/packages/twenty-server/src/engine/metadata-modules/user-role/user-role.service.ts @@ -137,6 +137,35 @@ export class UserRoleService { return workspaceMembers; } + public async validateUserWorkspaceIsNotUniqueAdminOrThrow({ + userWorkspaceId, + workspaceId, + }: { + userWorkspaceId: string; + workspaceId: string; + }) { + const roleOfUserWorkspace = await this.getRolesByUserWorkspaces({ + userWorkspaceIds: [userWorkspaceId], + workspaceId, + }).then((roles) => roles.get(userWorkspaceId)?.[0]); + + if (!isDefined(roleOfUserWorkspace)) { + throw new PermissionsException( + PermissionsExceptionMessage.NO_ROLE_FOUND_FOR_USER_WORKSPACE, + PermissionsExceptionCode.NO_ROLE_FOUND_FOR_USER_WORKSPACE, + ); + } + + if (roleOfUserWorkspace.label === ADMIN_ROLE_LABEL) { + const adminRole = roleOfUserWorkspace; + + await this.validateMoreThanOneWorkspaceMemberHasAdminRoleOrThrow({ + adminRoleId: adminRole.id, + workspaceId, + }); + } + } + private async validateAssignRoleInput({ userWorkspaceId, workspaceId, @@ -187,8 +216,21 @@ export class UserRoleService { return; } + await this.validateMoreThanOneWorkspaceMemberHasAdminRoleOrThrow({ + workspaceId, + adminRoleId: currentRole.id, + }); + } + + private async validateMoreThanOneWorkspaceMemberHasAdminRoleOrThrow({ + adminRoleId, + workspaceId, + }: { + adminRoleId: string; + workspaceId: string; + }) { const workspaceMembersWithAdminRole = - await this.getWorkspaceMembersAssignedToRole(currentRole.id, workspaceId); + await this.getWorkspaceMembersAssignedToRole(adminRoleId, workspaceId); if (workspaceMembersWithAdminRole.length === 1) { throw new PermissionsException( diff --git a/packages/twenty-server/test/integration/graphql/suites/user.integration-spec.ts b/packages/twenty-server/test/integration/graphql/suites/user.integration-spec.ts new file mode 100644 index 000000000..6fb6b88c5 --- /dev/null +++ b/packages/twenty-server/test/integration/graphql/suites/user.integration-spec.ts @@ -0,0 +1,55 @@ +import request from 'supertest'; +import { makeGraphqlAPIRequest } from 'test/integration/graphql/utils/make-graphql-api-request.util'; +import { updateFeatureFlagFactory } from 'test/integration/graphql/utils/update-feature-flag-factory.util'; + +import { SEED_APPLE_WORKSPACE_ID } from 'src/database/typeorm-seeds/core/workspaces'; +import { ErrorCode } from 'src/engine/core-modules/graphql/utils/graphql-errors.util'; + +const client = request(`http://localhost:${APP_PORT}`); + +describe('deleteUser', () => { + beforeAll(async () => { + const enablePermissionsQuery = updateFeatureFlagFactory( + SEED_APPLE_WORKSPACE_ID, + 'IsPermissionsEnabled', + true, + ); + + await makeGraphqlAPIRequest(enablePermissionsQuery); + }); + + afterAll(async () => { + const disablePermissionsQuery = updateFeatureFlagFactory( + SEED_APPLE_WORKSPACE_ID, + 'IsPermissionsEnabled', + false, + ); + + await makeGraphqlAPIRequest(disablePermissionsQuery); + }); + + it('should not allow to delete user if they are the unique admin of a workspace', async () => { + const query = { + query: ` + mutation DeleteUser { + deleteUser { + id + } + } + `, + }; + + await client + .post('/graphql') + .set('Authorization', `Bearer ${ADMIN_ACCESS_TOKEN}`) + .send(query) + .expect((res) => { + expect(res.body.data).toBeNull(); + expect(res.body.errors).toBeDefined(); + expect(res.body.errors[0].message).toBe( + 'Cannot delete account: user is the unique admin of a workspace', + ); + expect(res.body.errors[0].extensions.code).toBe(ErrorCode.FORBIDDEN); + }); + }); +});