[permission] Override query builders db-executing methods #11714

ijreilly · 2025-04-24T09:52:27Z

closes twentyhq/core-team-issues#843

…-twenty-global-orm-manager

…ty-global-orm-manager

…ilders

ijreilly · 2025-04-24T11:57:27Z

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

-
-import { WorkspaceSelectQueryBuilder } from 'src/engine/twenty-orm/repository/workspace-select-query-builder';
-
-export class WorkspaceQueryBuilder<


I thought we actually don't need this: we use a SelectQueryBuilder in the initial createQueryBuilder method in workspace.repository

greptile-apps

PR Summary

This PR implements a comprehensive permission system overhaul for database operations in the Twenty server, replacing the single WorkspaceQueryBuilder with specialized query builders for different operations.

Added new specialized query builders (WorkspaceInsertQueryBuilder, WorkspaceDeleteQueryBuilder, WorkspaceSoftDeleteQueryBuilder) with proper permission validation
Implemented permission checks in execute() methods across all query builders using validateQueryIsPermittedOrThrow
Added support for permissions V2 feature flag with backward compatibility
Added extensive integration tests covering permission scenarios for guest roles, admin roles, and API keys
Note: PR description appears to be incorrect as it references mobile display improvements but changes are about permission handling

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

packages/twenty-server/src/engine/twenty-orm/repository/permissions.util.ts

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

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

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

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

greptile-apps · 2025-04-24T12:00:30Z

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

+    afterAll(async () => {
+      const disablePermissionsQuery = updateFeatureFlagFactory(
+        SEED_APPLE_WORKSPACE_ID,
+        'IsPermissionsV2Enabled',
+        false,
+      );

-    expect(response.body.data).toStrictEqual({ deletePerson: 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);
-  });
+      await makeGraphqlAPIRequest(disablePermissionsQuery);
+    });


style: ensure feature flag is disabled even if test fails by using try/finally

greptile-apps · 2025-04-24T12:01:01Z

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

+    });
+
+    it('should throw a permission error when user does not have permission (guest role)', async () => {
+      const personId = randomUUID();


logic: personId is redefined here but never used in the test - the recordId in the operation could use the outer personId

greptile-apps · 2025-04-24T12:01:09Z

...suites/object-records-permissions/update-many-object-records-permissions.integration-spec.ts

+          id: {
+            in: [randomUUID(), randomUUID()],
+          },


logic: Using random UUIDs in the filter when testing permission denial could mask actual permission issues. Consider using the same personId1/personId2 that were created to ensure the test is checking permissions and not just failing because records don't exist.

Suggested change

id: {

in: [randomUUID(), randomUUID()],

},

id: {

in: [personId1, personId2],

},

greptile-apps · 2025-04-24T12:01:10Z

...suites/object-records-permissions/update-many-object-records-permissions.integration-spec.ts

+    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: Feature flag state changes in beforeAll/afterAll could affect other test suites running in parallel. Consider using a unique workspace ID for these tests.

greptile-apps · 2025-04-24T12:01:24Z

...uites/object-records-permissions/restore-many-object-records-permissions.integration-spec.ts

+    afterAll(async () => {
+      const disablePermissionsQuery = updateFeatureFlagFactory(
+        SEED_APPLE_WORKSPACE_ID,
+        'IsPermissionsV2Enabled',
+        false,
+      );

-    expect(response.body.data).toStrictEqual({ restorePeople: 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);
-  });
+      await makeGraphqlAPIRequest(disablePermissionsQuery);
+    });


style: Ensure feature flag is properly reset even if tests fail by using try/finally

Suggested change

afterAll(async () => {

const disablePermissionsQuery = updateFeatureFlagFactory(

SEED_APPLE_WORKSPACE_ID,

'IsPermissionsV2Enabled',

false,

);

expect(response.body.data).toStrictEqual({ restorePeople: 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);

});

await makeGraphqlAPIRequest(disablePermissionsQuery);

});

afterAll(async () => {

try {

const disablePermissionsQuery = updateFeatureFlagFactory(

SEED_APPLE_WORKSPACE_ID,

'IsPermissionsV2Enabled',

false,

);

await makeGraphqlAPIRequest(disablePermissionsQuery);

} catch (error) {

console.error('Failed to reset permissions feature flag:', error);

throw error;

}

});

…ilders

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

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

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

...y-server/src/modules/contact-creation-manager/services/create-company-and-contact.service.ts

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

packages/twenty-server/src/engine/twenty-orm/repository/permissions.util.ts

Weiko

LGTM! 👏

closes twentyhq/core-team-issues#843

ijreilly added 30 commits April 7, 2025 11:02

wip

53e6fb9

working version

7add8cc

refactor workspaceDatasourceFactory

9c87265

greptile comments

d6d3ab5

recompute object permissions after role update

076da2e

fix linter

a9c9792

Add shouldBypassPermissionChecks for twentyOrmGlobalManager

0052622

improvements

e96f627

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

0a69d0a

Compute object record permissions taking into account ObjectPermission

015ea62

fix test on findDuplicates

87c76e6

add featureFlagMap to datasource

73cc82e

fixes

1f28c39

some improvements

0689041

Merge branch 'perm--orm' of github.com:twentyhq/twenty into perm--orm…

8fc9b54

…-twenty-global-orm-manager

fix

355e1cd

Merge branch 'main' of github.com:twentyhq/twenty into perm--orm-twen…

d6f0269

…ty-global-orm-manager

bypass permission checks if apiKey

3af8d2e

recompute after object creation + organize flushing

debc96f

handle apiKey

5ed8837

add a first test for permissions V2

5d8613b

fix resolver

31b631f

handle rest api

5f369e8

Merge branch 'main' of github.com:twentyhq/twenty into perm--orm-twen…

9de5fec

…ty-global-orm-manager

add integration test with api key

f079c53

Merge branch 'main' of github.com:twentyhq/twenty into perm--orm-twen…

42a8052

…ty-global-orm-manager

adress greptile comments

40cdce2

Merge branch 'main' of github.com:twentyhq/twenty into perm--orm-twen…

c8d00bf

…ty-global-orm-manager

commiting block-inherited-methods.util to keep a trace

4cb9fb2

custom query builders

0c9c571

ijreilly added 6 commits April 22, 2025 16:31

remove block-inherited-methods util

c91fa6b

add tests

2ff456a

Merge branch 'main' of github.com:twentyhq/twenty into perm--query-bu…

938cd73

…ilders

fixes

d657593

fixes

a0d2fa2

fix test

8045a84

ijreilly commented Apr 24, 2025

View reviewed changes

ijreilly marked this pull request as ready for review April 24, 2025 11:57

greptile-apps bot reviewed Apr 24, 2025

View reviewed changes

ijreilly added 2 commits April 24, 2025 15:00

fix

c4c7073

Merge branch 'main' of github.com:twentyhq/twenty into perm--query-bu…

8338eee

…ilders

Weiko reviewed Apr 24, 2025

View reviewed changes

throw when query builder is morphing

88438b8

Weiko approved these changes Apr 24, 2025

View reviewed changes

ijreilly merged commit e750ef2 into main Apr 24, 2025
36 checks passed

ijreilly deleted the perm--query-builders branch April 24, 2025 16:20

ehconitin pushed a commit that referenced this pull request Apr 24, 2025

[permission] Override query builders db-executing methods (#11714)

d52f1ca

closes twentyhq/core-team-issues#843

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[permission] Override query builders db-executing methods #11714

[permission] Override query builders db-executing methods #11714

Uh oh!

ijreilly commented Apr 24, 2025 •

edited

Loading

Uh oh!

ijreilly Apr 24, 2025

Uh oh!

greptile-apps bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

greptile-apps bot Apr 24, 2025

Uh oh!

greptile-apps bot Apr 24, 2025

Uh oh!

greptile-apps bot Apr 24, 2025

Uh oh!

greptile-apps bot Apr 24, 2025

Uh oh!

greptile-apps bot Apr 24, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Weiko left a comment

Uh oh!

Uh oh!

Uh oh!


		import { WorkspaceSelectQueryBuilder } from 'src/engine/twenty-orm/repository/workspace-select-query-builder';

		export class WorkspaceQueryBuilder<

[permission] Override query builders db-executing methods #11714

[permission] Override query builders db-executing methods #11714

Uh oh!

Conversation

ijreilly commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ijreilly Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

greptile-apps bot Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Weiko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ijreilly commented Apr 24, 2025 •

edited

Loading