[permissions] Add permissions check layer in entityManager #11818

ijreilly · 2025-04-30T14:25:56Z

First and main step of twentyhq/core-team-issues#747

We are implementing a permission check layer in our custom WorkspaceEntityManager by overriding all the db-executing methods (this PR only overrides some as a POC, the rest will be done in the next PR).
Our custom repositories call entity managers under the hood to interact with the db so this solves the repositories case too.
This is still behind the feature flag IsPermissionsV2Enabled.

In the next PR

finish overriding all the methods required in WorkspaceEntityManager
add tests

…r used

github-actions · 2025-04-30T14:38:21Z

🚀 Preview Environment Ready!

Your preview environment is available at: http://bore.pub:47835

This environment will automatically shut down when the PR is closed or after 5 hours.

ijreilly · 2025-05-02T11:24:05Z

packages/twenty-server/src/database/typeorm-seeds/workspace/api-key.ts

  schemaName: string,
 ) => {
  await entityManager
-    .createQueryBuilder()
+    .createQueryBuilder(undefined, undefined, undefined, {


looks a bit ugly but needed if we want to override EntityManager's createQueryBuilder, + is just in the seeds

Could be an object as an input to avoid that

ijreilly · 2025-05-02T11:28:55Z

packages/twenty-server/src/engine/twenty-orm/datasource/workspace.datasource.ts

    this.featureFlagMap = featureFlagMap;
    this.featureFlagMapVersion = featureFlagMapVersion;
+    // Recreate manager after internalContext has been initialized
+    this.manager = this.createEntityManager();


order matters, otherwise this.featureFlagMap is empty

ijreilly · 2025-05-02T12:46:49Z

packages/twenty-server/src/engine/twenty-orm/entity-manager/entity.manager.ts

@@ -68,6 +81,88 @@ export class WorkspaceEntityManager extends EntityManager {
    return newRepository;
  }

+  override createQueryBuilder<Entity extends ObjectLiteral>(


this is currently only used in seeds

greptile-apps

PR Summary

This PR implements a comprehensive permission check layer in the WorkspaceEntityManager by overriding database-executing methods. Here are the key changes:

Replaced TypeORM's EntityManager with custom WorkspaceEntityManager across all database operations to enforce permission checks
Added shouldBypassPermissionChecks flag to allow seeding operations to bypass permission validation
Implemented permission validation utilities and query builders with proper checks before executing operations
Feature is controlled by IsPermissionsV2Enabled flag for gradual rollout

Key points to review:

Permission validation is only implemented for some methods as proof of concept, with more to come in next PR
Tests are deferred to a future PR which could be risky for such a fundamental change
Raw SQL queries in some repositories bypass the new permission layer
Transaction management changes could impact data consistency in some services

The PR appears to be incorrectly linked to issue #747 about probability picker UI, as this is a backend permissions infrastructure change.

_{59 file(s) reviewed, 8 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2025-05-02T12:57:08Z

packages/twenty-server/src/database/typeorm-seeds/workspace/message-channels.ts

+    .createQueryBuilder(undefined, undefined, undefined, {
+      shouldBypassPermissionChecks: true,
+    })


style: createQueryBuilder parameters are undefined but options are provided - consider documenting why these parameters are undefined or remove them if not needed

greptile-apps · 2025-05-02T12:57:40Z

packages/twenty-server/src/database/typeorm-seeds/workspace/connected-account.ts

+    .createQueryBuilder(undefined, undefined, undefined, {
+      shouldBypassPermissionChecks: true,
+    })


style: createQueryBuilder parameters are undefined but used in options object - consider documenting why these parameters are undefined or remove them if not needed

greptile-apps · 2025-05-02T13:00:10Z

packages/twenty-server/src/engine/twenty-orm/entity-manager/entity.manager.ts

+
+    validateOperationIsPermittedOrThrow({
+      entityName: this.extractTargetNameSingularFromEntityTarget(target),
+      operationType: 'update',


logic: operationType is hardcoded as 'update' for both insert and upsert operations. This is incorrect and could lead to wrong permission checks.

Suggested change

operationType: 'update',

operationType: 'create',

is it expected?

greptile-apps · 2025-05-02T13:00:11Z

packages/twenty-server/src/engine/twenty-orm/entity-manager/entity.manager.ts

+      if (options?.roleId) {
+        const dataSource = this.connection as WorkspaceDataSource;
+        const objectPermissionsByRoleId = dataSource.permissionsPerRoleId;


style: Redundant type casting of this.connection to WorkspaceDataSource since it's already declared as that type on line 26

greptile-apps · 2025-05-02T13:00:12Z

packages/twenty-server/src/engine/twenty-orm/entity-manager/entity.manager.ts

+    } = {
+      shouldBypassPermissionChecks: false,
+    },
+  ): SelectQueryBuilder<Entity> | WorkspaceSelectQueryBuilder<Entity> {


style: Return type union is unnecessary since WorkspaceSelectQueryBuilder extends SelectQueryBuilder

greptile-apps · 2025-05-02T13:02:40Z

packages/twenty-server/src/engine/twenty-orm/repository/workspace.repository.ts

@@ -456,12 +459,15 @@ export class WorkspaceRepository<
   */
  override async insert(
    entity: QueryDeepPartialEntity<T> | QueryDeepPartialEntity<T>[],
-    entityManager?: EntityManager,
+    entityManager?: WorkspaceEntityManager,
  ): Promise<InsertResult> {
    const manager = entityManager || this.manager;

    const formatedEntity = await this.formatData(entity);


syntax: inconsistent spelling: 'formatedEntity' vs 'formattedResult' used later

Suggested change

const formatedEntity = await this.formatData(entity);

const formattedEntity = await this.formatData(entity);

greptile-apps · 2025-05-02T13:06:09Z

.../suites/object-records-permissions/create-one-object-records-permissions.integration-spec.ts

+  describe('permissions V2 enabled', () => {
+    beforeAll(async () => {
+      const enablePermissionsQuery = updateFeatureFlagFactory(
+        SEED_APPLE_WORKSPACE_ID,
+        'IsPermissionsV2Enabled',
+        true,
+      );
+
+      await makeGraphqlAPIRequest(enablePermissionsQuery);
+    });
+
+    afterAll(async () => {
+      const disablePermissionsQuery = updateFeatureFlagFactory(
+        SEED_APPLE_WORKSPACE_ID,
+        'IsPermissionsV2Enabled',
+        false,
+      );
+
+      await makeGraphqlAPIRequest(disablePermissionsQuery);
+    });


style: Consider adding error handling for feature flag updates in beforeAll/afterAll. If these fail, subsequent tests could run in an incorrect state.

Suggested change

describe('permissions V2 enabled', () => {

beforeAll(async () => {

const enablePermissionsQuery = updateFeatureFlagFactory(

SEED_APPLE_WORKSPACE_ID,

'IsPermissionsV2Enabled',

true,

);

await makeGraphqlAPIRequest(enablePermissionsQuery);

});

afterAll(async () => {

const disablePermissionsQuery = updateFeatureFlagFactory(

SEED_APPLE_WORKSPACE_ID,

'IsPermissionsV2Enabled',

false,

);

await makeGraphqlAPIRequest(disablePermissionsQuery);

});

describe('permissions V2 enabled', () => {

beforeAll(async () => {

const enablePermissionsQuery = updateFeatureFlagFactory(

SEED_APPLE_WORKSPACE_ID,

'IsPermissionsV2Enabled',

true,

);

const response = await makeGraphqlAPIRequest(enablePermissionsQuery);

if (!response.body.data) {

throw new Error('Failed to enable permissions V2 feature flag');

}

});

afterAll(async () => {

const disablePermissionsQuery = updateFeatureFlagFactory(

SEED_APPLE_WORKSPACE_ID,

'IsPermissionsV2Enabled',

false,

);

const response = await makeGraphqlAPIRequest(disablePermissionsQuery);

if (!response.body.data) {

throw new Error('Failed to disable permissions V2 feature flag');

}

});

greptile-apps · 2025-05-02T13:06:09Z

.../suites/object-records-permissions/create-one-object-records-permissions.integration-spec.ts

+      expect(response.body.data).toStrictEqual({ createPerson: null });
+      expect(response.body.errors).toBeDefined();
+      expect(response.body.errors[0].message).toBe(
+        PermissionsExceptionMessage.PERMISSION_DENIED,
+      );
+      expect(response.body.errors[0].extensions.code).toBe(ErrorCode.FORBIDDEN);
+    });


style: Test could be more specific by checking error array length is exactly 1 to ensure no unexpected additional errors

Weiko

This is great!! Good job @ijreilly 👏

Weiko · 2025-05-02T13:39:15Z

packages/twenty-server/src/database/typeorm-seeds/workspace/api-key.ts

  schemaName: string,
 ) => {
  await entityManager
-    .createQueryBuilder()
+    .createQueryBuilder(undefined, undefined, undefined, {


Could be an object as an input to avoid that

Weiko · 2025-05-02T14:00:52Z

packages/twenty-server/src/engine/twenty-orm/datasource/workspace.datasource.ts

+    mode = 'master' as ReplicationMode,
+  ): WorkspaceQueryRunner {
+    const queryRunner = this.driver.createQueryRunner(mode);
+    const manager = this.createEntityManager(queryRunner);


should we use this.manager here?

Weiko · 2025-05-02T14:03:09Z

...rc/engine/metadata-modules/field-metadata/services/field-metadata-related-records.service.ts

@@ -28,7 +28,6 @@ export class FieldMetadataRelatedRecordsService {
  public async updateRelatedViewGroups(
    oldFieldMetadata: FieldMetadataEntity,
    newFieldMetadata: FieldMetadataEntity,
-    transactionManager?: EntityManager,


do we want to remove the transactionManager?

Weiko · 2025-05-02T14:04:51Z

packages/twenty-server/src/engine/twenty-orm/entity-manager/entity.manager.ts

 import { WorkspaceDataSource } from 'src/engine/twenty-orm/datasource/workspace.datasource';
+import { validateOperationIsPermittedOrThrow } from 'src/engine/twenty-orm/repository/permissions.utils';
+import { WorkspaceSelectQueryBuilder } from 'src/engine/twenty-orm/repository/workspace-select-query-builder';
 import { WorkspaceRepository } from 'src/engine/twenty-orm/repository/workspace.repository';

 export class WorkspaceEntityManager extends EntityManager {


Just realised this but could we rename the file accordingly?

Weiko · 2025-05-02T14:06:20Z

packages/twenty-server/src/engine/core-modules/auth/services/google-apis.service.ts

@@ -103,202 +104,208 @@ export class GoogleAPIsService {

    const scopes = getGoogleApisOauthScopes();

-    await workspaceDataSource.transaction(async (manager: EntityManager) => {


hard to review this one but I guess this is mostly indentation?

Weiko · 2025-05-02T14:08:43Z

packages/twenty-server/src/engine/seeder/seeder.service.ts

@@ -60,12 +61,13 @@ export class SeederService {
    const schemaName =
      this.workspaceDataSourceService.getSchemaName(workspaceId);

-    const workspaceDataSource =
+    const workspaceDataSource: DataSource =


Not your fault and note for myself but I've just realised that this one is very misleading since it returns the main datasource from TypeORMService (which is not the workspace datasource (also why your type is DataSource here)

I know, initially I had left two comments around workspaceDataSource and the below entityManager to raise attention around that, but thought that was maybe too much and added explicit types instead. wdyt ?

Should be fine for now but I think our next goal should be:

Removing DataSource table, move schema name to Workspace table and we don't need the rest.

connectedToWorkspaceDataSourceAndReturnMetadata should be removed, this is only called during seeding or internal code and it's actually returning the mainDataSource as stated above. We should use typeorm service in those places directly and have a getMainDataSource() instead of connectToDataSource()

Weiko · 2025-05-02T14:10:34Z

packages/twenty-server/src/engine/twenty-orm/datasource/workspace.datasource.ts

+    return queryRunner as any as WorkspaceQueryRunner;
+  }
+
+  override transaction<T>(


is this override needed? Looks like the implementation from typeorm already with no particular changes or maybe I'm missing something?

Weiko · 2025-05-02T14:13:22Z

packages/twenty-server/src/engine/twenty-orm/entity-manager/entity.manager.ts

+
+    validateOperationIsPermittedOrThrow({
+      entityName: this.extractTargetNameSingularFromEntityTarget(target),
+      operationType: 'update',


is it expected?

ijreilly added 5 commits April 30, 2025 16:17

Update WorkspaceEntityManager to integrate permission check

3c9a836

adapt entityManager.createQueryBuilder and bypass checks in seeds

1d2b648

update entityManager type

a327c23

remove optional arg transactionManager from signature when it is neve…

c33a6d0

…r used

add test

981a4cb

ijreilly added 2 commits April 30, 2025 16:58

Merge branch 'main' of github.com:twentyhq/twenty into perm--repo

008d320

fix entity manager stale featureFlagMap issue + api key

4d639be

ijreilly commented May 2, 2025

View reviewed changes

replace comments with explicity type declaration

6928956

ijreilly commented May 2, 2025

View reviewed changes

ijreilly changed the title ~~Perm repo~~ [permissions] Add permissions check layer in entityManager May 2, 2025

ijreilly marked this pull request as ready for review May 2, 2025 12:54

ijreilly added 2 commits May 2, 2025 14:55

Merge branch 'main' of github.com:twentyhq/twenty into perm--repo

180d047

fix + missing methods in WorkspaceSelectQueryBuilder

c722d22

greptile-apps bot reviewed May 2, 2025

View reviewed changes

prastoin added -PR: wip -PR: awaiting author labels May 2, 2025

Weiko reviewed May 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[permissions] Add permissions check layer in entityManager #11818

[permissions] Add permissions check layer in entityManager #11818

ijreilly commented Apr 30, 2025 •

edited

Loading

github-actions bot commented Apr 30, 2025 •

edited

Loading

ijreilly May 2, 2025

Weiko May 2, 2025

ijreilly May 2, 2025

ijreilly May 2, 2025

greptile-apps bot left a comment

greptile-apps bot May 2, 2025

greptile-apps bot May 2, 2025

greptile-apps bot May 2, 2025

Weiko May 2, 2025

greptile-apps bot May 2, 2025

greptile-apps bot May 2, 2025

greptile-apps bot May 2, 2025

greptile-apps bot May 2, 2025

greptile-apps bot May 2, 2025

Weiko left a comment

Weiko May 2, 2025

Weiko May 2, 2025

Weiko May 2, 2025

Weiko May 2, 2025

Weiko May 2, 2025

Weiko May 2, 2025

ijreilly May 2, 2025

Weiko May 2, 2025

Weiko May 2, 2025

Weiko May 2, 2025

	const formatedEntity = await this.formatData(entity);
	const formattedEntity = await this.formatData(entity);

		@@ -103,202 +104,208 @@ export class GoogleAPIsService {

		const scopes = getGoogleApisOauthScopes();

		await workspaceDataSource.transaction(async (manager: EntityManager) => {

[permissions] Add permissions check layer in entityManager #11818

Are you sure you want to change the base?

[permissions] Add permissions check layer in entityManager #11818

Conversation

ijreilly commented Apr 30, 2025 • edited Loading

github-actions bot commented Apr 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot May 2, 2025

Choose a reason for hiding this comment

greptile-apps bot May 2, 2025

Choose a reason for hiding this comment

greptile-apps bot May 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greptile-apps bot May 2, 2025

Choose a reason for hiding this comment

greptile-apps bot May 2, 2025

Choose a reason for hiding this comment

greptile-apps bot May 2, 2025

Choose a reason for hiding this comment

greptile-apps bot May 2, 2025

Choose a reason for hiding this comment

greptile-apps bot May 2, 2025

Choose a reason for hiding this comment

Weiko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijreilly commented Apr 30, 2025 •

edited

Loading

github-actions bot commented Apr 30, 2025 •

edited

Loading