[permissions] Writing permission does not go without reading permission (#12573)

Closes https://github.com/twentyhq/core-team-issues/issues/868 We should not allow to grant any writing permission (update, soft delete, delete) on an object or at role-level without the reading permission at the same level. This has been implemented in the front-end at role level, and is yet to be done at object level (@Weiko)
2025-06-16 12:04:38 +02:00
parent bee1717d37
commit cdc4badec3
11 changed files with 1009 additions and 30 deletions
--- a/packages/twenty-server/src/engine/metadata-modules/object-permission/tests/object-permission.service.spec.ts
+++ b/packages/twenty-server/src/engine/metadata-modules/object-permission/tests/object-permission.service.spec.ts
@ -4,6 +4,9 @@ import { getRepositoryToken } from '@nestjs/typeorm';
 import { Repository } from 'typeorm';

 import { ObjectMetadataEntity } from 'src/engine/metadata-modules/object-metadata/object-metadata.entity';
+import { UpsertObjectPermissionsInput } from 'src/engine/metadata-modules/object-permission/dtos/upsert-object-permissions.input';
+import { ObjectPermissionEntity } from 'src/engine/metadata-modules/object-permission/object-permission.entity';
+import { ObjectPermissionService } from 'src/engine/metadata-modules/object-permission/object-permission.service';
 import {
  PermissionsException,
  PermissionsExceptionCode,
@ -14,11 +17,6 @@ import { ObjectMetadataItemWithFieldMaps } from 'src/engine/metadata-modules/typ
 import { WorkspacePermissionsCacheService } from 'src/engine/metadata-modules/workspace-permissions-cache/workspace-permissions-cache.service';
 import { WorkspaceCacheStorageService } from 'src/engine/workspace-cache-storage/workspace-cache-storage.service';

-import { ObjectPermissionEntity } from './object-permission.entity';
-import { ObjectPermissionService } from './object-permission.service';
-
-import { UpsertObjectPermissionsInput } from './dtos/upsert-object-permissions.input';
-
 describe('ObjectPermissionService', () => {
  let service: ObjectPermissionService;
  let objectPermissionRepository: jest.Mocked<
@ -89,7 +87,8 @@ describe('ObjectPermissionService', () => {
        id: roleId,
        workspaceId,
        isEditable: true,
-      } as RoleEntity);
+        objectPermissions: [],
+      } as unknown as RoleEntity);
    });

    it('should throw PermissionsException when trying to add object permission on system object', async () => {
--- a/packages/twenty-server/src/engine/metadata-modules/object-permission/object-permission.service.ts
+++ b/packages/twenty-server/src/engine/metadata-modules/object-permission/object-permission.service.ts
@ -4,7 +4,10 @@ import { isDefined } from 'twenty-shared/utils';
 import { In, Repository } from 'typeorm';

 import { ObjectMetadataEntity } from 'src/engine/metadata-modules/object-metadata/object-metadata.entity';
-import { UpsertObjectPermissionsInput } from 'src/engine/metadata-modules/object-permission/dtos/upsert-object-permissions.input';
+import {
+  ObjectPermissionInput,
+  UpsertObjectPermissionsInput,
+} from 'src/engine/metadata-modules/object-permission/dtos/upsert-object-permissions.input';
 import { ObjectPermissionEntity } from 'src/engine/metadata-modules/object-permission/object-permission.entity';
 import {
  PermissionsException,
@ -35,11 +38,20 @@ export class ObjectPermissionService {
    input: UpsertObjectPermissionsInput;
  }): Promise<ObjectPermissionEntity[]> {
    try {
-      await this.validateRoleIsEditableOrThrow({
+      const role = await this.getRoleOrThrow({
        roleId: input.roleId,
        workspaceId,
      });

+      await this.validateRoleIsEditableOrThrow({
+        role,
+      });
+
+      await this.validateObjectPermissionsReadAndWriteConsistencyOrThrow({
+        objectPermissions: input.objectPermissions,
+        roleWithObjectPermissions: role,
+      });
+
      const { byId: objectMetadataMapsById } =
        await this.workspaceCacheStorageService.getObjectMetadataMapsOrThrow(
          workspaceId,
@ -117,6 +129,58 @@ export class ObjectPermissionService {
    }
  }

+  private async validateObjectPermissionsReadAndWriteConsistencyOrThrow({
+    objectPermissions: newObjectPermissions,
+    roleWithObjectPermissions,
+  }: {
+    objectPermissions: ObjectPermissionInput[];
+    roleWithObjectPermissions: RoleEntity;
+  }) {
+    const existingObjectPermissions =
+      roleWithObjectPermissions.objectPermissions;
+
+    for (const newObjectPermission of newObjectPermissions) {
+      const existingObjectRecordPermission = existingObjectPermissions.find(
+        (objectPermission) =>
+          objectPermission.objectMetadataId ===
+          newObjectPermission.objectMetadataId,
+      );
+
+      const hasReadPermissionAfterUpdate =
+        newObjectPermission.canReadObjectRecords ??
+        existingObjectRecordPermission?.canReadObjectRecords ??
+        roleWithObjectPermissions.canReadAllObjectRecords;
+
+      if (hasReadPermissionAfterUpdate === false) {
+        const hasUpdatePermissionAfterUpdate =
+          newObjectPermission.canUpdateObjectRecords ??
+          existingObjectRecordPermission?.canUpdateObjectRecords ??
+          roleWithObjectPermissions.canUpdateAllObjectRecords;
+
+        const hasSoftDeletePermissionAfterUpdate =
+          newObjectPermission.canSoftDeleteObjectRecords ??
+          existingObjectRecordPermission?.canSoftDeleteObjectRecords ??
+          roleWithObjectPermissions.canSoftDeleteAllObjectRecords;
+
+        const hasDestroyPermissionAfterUpdate =
+          newObjectPermission.canDestroyObjectRecords ??
+          existingObjectRecordPermission?.canDestroyObjectRecords ??
+          roleWithObjectPermissions.canDestroyAllObjectRecords;
+
+        if (
+          hasUpdatePermissionAfterUpdate ||
+          hasSoftDeletePermissionAfterUpdate ||
+          hasDestroyPermissionAfterUpdate
+        ) {
+          throw new PermissionsException(
+            PermissionsExceptionMessage.CANNOT_GIVE_WRITING_PERMISSION_ON_NON_READABLE_OBJECT,
+            PermissionsExceptionCode.CANNOT_GIVE_WRITING_PERMISSION_ON_NON_READABLE_OBJECT,
+          );
+        }
+      }
+    }
+  }
+
  private async handleForeignKeyError({
    error,
    roleId,
@ -159,7 +223,7 @@ export class ObjectPermissionService {
    }
  }

-  private async validateRoleIsEditableOrThrow({
+  private async getRoleOrThrow({
    roleId,
    workspaceId,
  }: {
@ -171,9 +235,21 @@ export class ObjectPermissionService {
        id: roleId,
        workspaceId,
      },
+      relations: ['objectPermissions'],
    });

-    if (!role?.isEditable) {
+    if (!isDefined(role)) {
+      throw new PermissionsException(
+        PermissionsExceptionMessage.ROLE_NOT_FOUND,
+        PermissionsExceptionCode.ROLE_NOT_FOUND,
+      );
+    }
+
+    return role;
+  }
+
+  private async validateRoleIsEditableOrThrow({ role }: { role: RoleEntity }) {
+    if (!role.isEditable) {
      throw new PermissionsException(
        PermissionsExceptionMessage.ROLE_NOT_EDITABLE,
        PermissionsExceptionCode.ROLE_NOT_EDITABLE,
--- a/packages/twenty-server/src/engine/metadata-modules/permissions/permissions.exception.ts
+++ b/packages/twenty-server/src/engine/metadata-modules/permissions/permissions.exception.ts
@ -34,6 +34,8 @@ export enum PermissionsExceptionCode {
  CANNOT_ADD_OBJECT_PERMISSION_ON_SYSTEM_OBJECT = 'CANNOT_ADD_OBJECT_PERMISSION_ON_SYSTEM_OBJECT',
  METHOD_NOT_ALLOWED = 'METHOD_NOT_ALLOWED',
  RAW_SQL_NOT_ALLOWED = 'RAW_SQL_NOT_ALLOWED',
+  CANNOT_GIVE_WRITING_PERMISSION_ON_NON_READABLE_OBJECT = 'CANNOT_GIVE_WRITING_PERMISSION_ON_NON_READABLE_OBJECT',
+  CANNOT_GIVE_WRITING_PERMISSION_WITHOUT_READING_PERMISSION = 'CANNOT_GIVE_WRITING_PERMISSION_WITHOUT_READING_PERMISSION',
 }

 export enum PermissionsExceptionMessage {
@ -60,4 +62,6 @@ export enum PermissionsExceptionMessage {
  DEFAULT_ROLE_CANNOT_BE_DELETED = 'Default role cannot be deleted',
  NO_PERMISSIONS_FOUND_IN_DATASOURCE = 'No permissions found in datasource',
  CANNOT_ADD_OBJECT_PERMISSION_ON_SYSTEM_OBJECT = 'Cannot add object permission on system object',
+  CANNOT_GIVE_WRITING_PERMISSION_ON_NON_READABLE_OBJECT = 'Cannot give update permission to non-readable object',
+  CANNOT_GIVE_WRITING_PERMISSION_WITHOUT_READING_PERMISSION = 'Cannot give writing permission without reading permission',
 }
--- 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
@ -23,6 +23,8 @@ export const permissionGraphqlApiExceptionHandler = (
      throw new ForbiddenError(error.message);
    case PermissionsExceptionCode.INVALID_ARG:
    case PermissionsExceptionCode.INVALID_SETTING:
+    case PermissionsExceptionCode.CANNOT_GIVE_WRITING_PERMISSION_ON_NON_READABLE_OBJECT:
+    case PermissionsExceptionCode.CANNOT_GIVE_WRITING_PERMISSION_WITHOUT_READING_PERMISSION:
      throw new UserInputError(error.message);
    case PermissionsExceptionCode.ROLE_NOT_FOUND:
    case PermissionsExceptionCode.USER_WORKSPACE_NOT_FOUND:
--- a/packages/twenty-server/src/engine/metadata-modules/role/role.service.ts
+++ b/packages/twenty-server/src/engine/metadata-modules/role/role.service.ts
@ -67,7 +67,7 @@ export class RoleService {
    input: CreateRoleInput;
    workspaceId: string;
  }): Promise<RoleEntity> {
-    await this.validateRoleInput({ input, workspaceId });
+    await this.validateRoleInputOrThrow({ input, workspaceId });

    const role = await this.roleRepository.save({
      label: input.label,
@ -117,7 +117,7 @@ export class RoleService {
      );
    }

-    await this.validateRoleInput({
+    await this.validateRoleInputOrThrow({
      input: input.update,
      workspaceId,
      roleId: input.id,
@ -243,7 +243,7 @@ export class RoleService {
    });
  }

-  private async validateRoleInput({
+  private async validateRoleInputOrThrow({
    input,
    workspaceId,
    roleId,
@ -277,33 +277,64 @@ export class RoleService {
      }
    }

+    const workspaceRoles = await this.getWorkspaceRoles(workspaceId);
+
    if (isDefined(input.label)) {
-      let workspaceRoles = await this.getWorkspaceRoles(workspaceId);
+      let rolesForLabelComparison = workspaceRoles;

      if (isDefined(roleId)) {
-        workspaceRoles = workspaceRoles.filter((role) => role.id !== roleId);
+        rolesForLabelComparison = workspaceRoles.filter(
+          (role) => role.id !== roleId,
+        );
      }

-      if (workspaceRoles.some((role) => role.label === input.label)) {
+      if (rolesForLabelComparison.some((role) => role.label === input.label)) {
        throw new PermissionsException(
          PermissionsExceptionMessage.ROLE_LABEL_ALREADY_EXISTS,
          PermissionsExceptionCode.ROLE_LABEL_ALREADY_EXISTS,
        );
      }
    }
+
+    const existingRole = workspaceRoles.find((role) => role.id === roleId);
+
+    await this.validateRoleReadAndWirtePermissionsConsistencyOrThrow({
+      input,
+      existingRole,
+    });
  }

-  private async validateRoleIsNotDefaultRoleOrThrow({
-    roleId,
-    defaultRoleId,
+  private async validateRoleReadAndWirtePermissionsConsistencyOrThrow({
+    input,
+    existingRole,
  }: {
-    roleId: string;
-    defaultRoleId: string;
-  }): Promise<void> {
-    if (defaultRoleId === roleId) {
+    input: CreateRoleInput | UpdateRolePayload;
+    existingRole?: RoleEntity;
+  }) {
+    const hasReadingPermissionsAfterUpdate =
+      input.canReadAllObjectRecords ?? existingRole?.canReadAllObjectRecords;
+
+    const hasUpdatePermissionsAfterUpdate =
+      input.canUpdateAllObjectRecords ??
+      existingRole?.canUpdateAllObjectRecords;
+
+    const hasSoftDeletePermissionsAfterUpdate =
+      input.canSoftDeleteAllObjectRecords ??
+      existingRole?.canSoftDeleteAllObjectRecords;
+
+    const hasDestroyPermissionsAfterUpdate =
+      input.canDestroyAllObjectRecords ??
+      existingRole?.canDestroyAllObjectRecords;
+
+    if (
+      hasReadingPermissionsAfterUpdate === false &&
+      (hasUpdatePermissionsAfterUpdate ||
+        hasSoftDeletePermissionsAfterUpdate ||
+        hasDestroyPermissionsAfterUpdate)
+    ) {
      throw new PermissionsException(
-        PermissionsExceptionMessage.DEFAULT_ROLE_CANNOT_BE_DELETED,
-        PermissionsExceptionCode.DEFAULT_ROLE_CANNOT_BE_DELETED,
+        PermissionsExceptionMessage.CANNOT_GIVE_WRITING_PERMISSION_WITHOUT_READING_PERMISSION,
+        PermissionsExceptionCode.CANNOT_GIVE_WRITING_PERMISSION_WITHOUT_READING_PERMISSION,
      );
    }
  }
@ -362,4 +393,19 @@ export class RoleService {
      );
    }
  }
+
+  private async validateRoleIsNotDefaultRoleOrThrow({
+    roleId,
+    defaultRoleId,
+  }: {
+    roleId: string;
+    defaultRoleId: string;
+  }): Promise<void> {
+    if (defaultRoleId === roleId) {
+      throw new PermissionsException(
+        PermissionsExceptionMessage.DEFAULT_ROLE_CANNOT_BE_DELETED,
+        PermissionsExceptionCode.DEFAULT_ROLE_CANNOT_BE_DELETED,
+      );
+    }
+  }
 }
--- a/packages/twenty-server/src/engine/twenty-orm/entity-manager/workspace-entity-manager.ts
+++ b/packages/twenty-server/src/engine/twenty-orm/entity-manager/workspace-entity-manager.ts
@ -965,7 +965,7 @@ export class WorkspaceEntityManager extends EntityManager {
      .execute();
  }

-  override findByIds<Entity extends ObjectLiteral>(
+  override async findByIds<Entity extends ObjectLiteral>(
    entityClass: EntityTarget<Entity>,
    ids: string[],
    permissionOptions?: PermissionOptions,
@ -1033,7 +1033,10 @@ export class WorkspaceEntityManager extends EntityManager {
    permissionOptions?: PermissionOptions,
  ): Promise<T>;

-  override save<Entity extends ObjectLiteral, T extends DeepPartial<Entity>>(
+  override async save<
+    Entity extends ObjectLiteral,
+    T extends DeepPartial<Entity>,
+  >(
    targetOrEntity: EntityTarget<Entity> | Entity | Entity[],
    entityOrMaybeOptions:
      | T
@ -1117,7 +1120,7 @@ export class WorkspaceEntityManager extends EntityManager {
    permissionOptions?: PermissionOptions,
  ): Promise<Entity[]>;

-  override remove<Entity extends ObjectLiteral>(
+  override async remove<Entity extends ObjectLiteral>(
    targetOrEntity: EntityTarget<Entity> | Entity[] | Entity,
    entityOrMaybeOptions: Entity | Entity[] | RemoveOptions,
    maybeOptionsOrMaybePermissionOptions?: RemoveOptions | PermissionOptions,
@ -1279,7 +1282,10 @@ export class WorkspaceEntityManager extends EntityManager {
    permissionOptions?: PermissionOptions,
  ): Promise<T>;

-  override recover<Entity extends ObjectLiteral, T extends DeepPartial<Entity>>(
+  override async recover<
+    Entity extends ObjectLiteral,
+    T extends DeepPartial<Entity>,
+  >(
    targetOrEntityOrEntities: EntityTarget<Entity> | Entity | Entity[],
    entityOrEntitiesOrMaybeOptions: T | T[] | SaveOptions,
    maybeOptionsOrMaybePermissionOptions?: SaveOptions | PermissionOptions,