From f8e3fd110d35778bef7341ef563c2607d065bd2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Malfait?= Date: Mon, 3 Aug 2026 19:01:21 +0200 Subject: [PATCH] Bound an application by its own role as well as the user's (#23680) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Why When an application acts on someone's behalf its token carries `userId` and `userWorkspaceId` alongside `applicationId`, and the application then received **that person's permissions in full**. The role it installs with was never consulted, so it was not a bound on what the application could do for them. It was meant to be an intersection. `permissions.service.ts` made this visible: the branches are `apiKeyId → userWorkspaceId → applicationId` and each returns early, so with both present the user branch won and `application.defaultRoleId` was never read. The same was true on the object and row-level paths, for different reasons. ## What had to change Three independent causes, all of which blocked the intersection from existing or from being enforced. **The application was thrown away before anything could use it.** `workspace-auth-context.middleware.ts` built a `type: 'user'` context when both principals were present, and `UserWorkspaceAuthContext` had no slot for an application. It now carries an optional one. Additive rather than a new union member on purpose. Nothing in the server exhaustively checks this union (no `assertUnreachable`, one `switch`, in Sentry tagging), so a sixth member would have compiled fine and then fallen through actor attribution, that switch, and `metadata-event-emitter.ts` silently. The additive change leaves all four type guards returning identical booleans. **Role resolution returned a single id.** The rule itself now lives in one place, `resolveRoleIdsForUser`: a user's role, narrowed by the application's if it declared one, never the same id twice. `resolveRoleIdsFromAuthContext` and `resolveRolePermissionConfig` build `{ intersectionOf: [...] }` from it, which `getRepository` already applied over N roles. A user with no role still resolves to nothing, so an application can never stand in for a missing user role. **Row-level security ignored all of it.** RLS was re-derived from a single role at query time, so it would have been unaffected by any intersection. Each role is now compiled on its own and the resulting filters are ANDed. That last choice matters. Merging the raw predicates and groups first would have been wrong: `computeRecordGqlOperationFilter` honours only the first parentless group, so concatenating two roles' groups makes one role's predicates vanish, **widening** access. Compiling per role and ANDing needs no synthetic groups, no re-parenting and no `twenty-shared` type change, and reuses the single-role logic untouched. Subscriptions go through the same rule. An event stream resolved only the subscriber's role, so a stream opened by an application acting for someone was filtered by that person's role alone. The stream now records the application it was opened by and the publisher intersects both roles for object permissions, restricted fields and RLS, exactly as a query does. Two smaller fixes fall out: - `getObjectsPermissionsFromRolePermissionConfig` had a `// Multi-role union/intersection is not ready — use the first assigned role only` shortcut and now intersects. - `computePermissionIntersection` hardcoded empty row-level predicate arrays, which is why RLS-constrained fields were not exempted from the field-permission check on insert and could fail spuriously. It now reports the fields **every** role constrains. Reporting fields constrained by only one role would be worse than the original bug: the insert guard waives a field-update deny on them, so one role's row-level rule would cancel another role's deny. ## Behaviour on the edges **An application that declares no role adds no bound.** `defaultRoleId` stays null whenever a manifest omits `defaultRoleUniversalIdentifier`, which is the common case, so denying would have broken a lot of installed applications. Behaviour changes only for applications that actually declared a role. To stop that being permanent, `defaultRoleUniversalIdentifier` should become required for new applications. Hard-requiring it needs a backfill for existing installs, so it is not in this PR. **An application that cannot be found denies.** That is not the same as one that declared no role, and treating it as such would have let a token naming a deleted application fall back to the full permissions of the user it acts for. **A role that cannot be resolved denies.** `application.defaultRoleId` is a plain uuid column with no foreign key, and role deletion does not clear it, so it can dangle. A bound we cannot apply must not let the remaining roles decide on their own, so the ORM path, `getObjectsPermissionsFromRolePermissionConfig` and the subscription publisher all return no permissions in that case rather than falling back. ## Testing - Full `twenty-server` unit suite green (896 suites, 7353 tests) - New spec for `resolveRoleIdsFromAuthContext`: both roles, application with no declared role, application holding the user's own role, user with no role, api key, application-only, system - New spec for multi-role RLS, including the case this fixes (a restricted role intersected with an unrestricted one keeps the restriction) and two restricted roles ANDing - `permissions.service.spec.ts` had **no coverage of the application branch at all** (`ApplicationEntity` was mocked as `{}`); it now has a real mock plus user-grants/application-denies, the reverse, both-grant, the null-role fallback, a shared role, and a missing application - First coverage of non-empty row-level predicates through `computePermissionIntersection`, including a field constrained by one role only - Subscription publisher: application role denies, both allow, application role dangling, and both roles reaching the RLS filter - Updated the two specs that asserted the old behaviour: the middleware dropping the application, and "use the first role when multiple are provided" No schema, cache or GraphQL change: `rolesPermissions` is keyed by role id alone and the intersection is computed per request from cached per-role entries. ## Not in this PR `workflow-execution-context.service.ts` falls back to the **admin** role when an application has no `defaultRoleId`, and to `shouldBypassPermissionChecks: true` if admin is not found. That is the inverse of the rule here and an escalation in its own right, but workflow execution is sensitive, so it is tracked separately in twentyhq/core-team-issues#2753. Three resolvers still carry their own principal precedence and do not use this seam: `rest-api-base.handler.ts`, `mcp-protocol.service.ts` (which never builds a user or application context at all), and actor attribution in `actor-from-auth-context.service.ts`. Separately, `computeRecordGqlOperationFilter` silently discards predicates under any parentless group after the first, with no test coverage. That is a latent bug independent of this work and lives in `twenty-shared`, shared with the front-end filter system. --- _Generated by [Claude Code](https://claude.ai/code/session_01C6nCVbcb5ZZrz67uvqvMWF)_ ``<img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg">`` --- .../workspace-auth-context.middleware.spec.ts | 21 +- .../workspace-auth-context.middleware.ts | 1 + .../auth/types/workspace-auth-context.type.ts | 1 + .../utils/build-user-auth-context.util.ts | 4 + .../__tests__/permissions.service.spec.ts | 178 ++++++++++++- .../permissions/permissions.service.ts | 44 ++++ .../__tests__/event-stream.service.spec.ts | 73 ++++++ .../subscriptions/event-stream.resolver.ts | 12 + .../subscriptions/event-stream.service.ts | 5 +- .../object-record-event-publisher.spec.ts | 179 ++++++++++++- .../object-record-event-publisher.ts | 137 +++++++--- .../workspace-entity-manager.ts | 9 +- ...evel-permission-record-filter.util.spec.ts | 117 +++++++++ ...mpute-permission-intersection.util.spec.ts | 75 ++++++ ...s-from-role-permission-config.util.spec.ts | 26 +- ...ve-role-ids-from-auth-context.util.spec.ts | 103 ++++++++ ...ly-row-level-permission-predicates.util.ts | 6 +- ...row-level-permission-record-filter.util.ts | 44 +++- .../compute-permission-intersection.util.ts | 33 ++- ...ssions-from-role-permission-config.util.ts | 34 ++- .../utils/resolve-role-ids-for-user.util.ts | 20 ++ ...esolve-role-ids-from-auth-context.util.ts} | 25 +- .../resolve-role-permission-config.util.ts | 10 +- ...alidate-rls-predicates-for-records.util.ts | 9 +- ...user-role-intersection.integration-spec.ts | 237 ++++++++++++++++++ 25 files changed, 1304 insertions(+), 99 deletions(-) create mode 100644 packages/twenty-server/src/engine/twenty-orm/utils/__tests__/build-row-level-permission-record-filter.util.spec.ts create mode 100644 packages/twenty-server/src/engine/twenty-orm/utils/__tests__/resolve-role-ids-from-auth-context.util.spec.ts create mode 100644 packages/twenty-server/src/engine/twenty-orm/utils/resolve-role-ids-for-user.util.ts rename packages/twenty-server/src/engine/twenty-orm/utils/{resolve-role-id-from-auth-context.util.ts => resolve-role-ids-from-auth-context.util.ts} (56%) create mode 100644 packages/twenty-server/test/integration/graphql/suites/application-user-role-intersection.integration-spec.ts diff --git a/packages/twenty-server/src/engine/core-modules/auth/middlewares/__tests__/workspace-auth-context.middleware.spec.ts b/packages/twenty-server/src/engine/core-modules/auth/middlewares/__tests__/workspace-auth-context.middleware.spec.ts index 425e5a1bfe..2d0bda6c7a 100644 --- a/packages/twenty-server/src/engine/core-modules/auth/middlewares/__tests__/workspace-auth-context.middleware.spec.ts +++ b/packages/twenty-server/src/engine/core-modules/auth/middlewares/__tests__/workspace-auth-context.middleware.spec.ts @@ -77,7 +77,7 @@ describe('WorkspaceAuthContextMiddleware', () => { ); }); - it('should create a user auth context when both application and user are present', () => { + it('should keep the application on the user auth context when both are present', () => { const req = buildRequest({ application: mockApplication, user: mockUser, @@ -100,10 +100,29 @@ describe('WorkspaceAuthContextMiddleware', () => { userWorkspaceId: 'user-workspace-id', workspaceMemberId: 'workspace-member-id', workspaceMember: mockWorkspaceMember, + application: mockApplication, }), ); }); + it('should not put an application on the user auth context when there is none', () => { + const req = buildRequest({ + user: mockUser, + userWorkspaceId: 'user-workspace-id', + workspaceMemberId: 'workspace-member-id', + workspaceMember: mockWorkspaceMember, + }); + let capturedContext: unknown; + + (mockNext as jest.Mock).mockImplementation(() => { + capturedContext = workspaceAuthContextStorage.getStore(); + }); + + middleware.use(req, mockResponse, mockNext); + + expect(capturedContext).not.toHaveProperty('application'); + }); + it('should create an application auth context when application is present without user', () => { const req = buildRequest({ application: mockApplication }); let capturedContext: unknown; diff --git a/packages/twenty-server/src/engine/core-modules/auth/middlewares/workspace-auth-context.middleware.ts b/packages/twenty-server/src/engine/core-modules/auth/middlewares/workspace-auth-context.middleware.ts index bb1c0a992c..0f6b17efa8 100644 --- a/packages/twenty-server/src/engine/core-modules/auth/middlewares/workspace-auth-context.middleware.ts +++ b/packages/twenty-server/src/engine/core-modules/auth/middlewares/workspace-auth-context.middleware.ts @@ -53,6 +53,7 @@ export class WorkspaceAuthContextMiddleware implements NestMiddleware { user: req.user, workspaceMemberId: req.workspaceMemberId, workspaceMember: req.workspaceMember, + application: req.application, }); } diff --git a/packages/twenty-server/src/engine/core-modules/auth/types/workspace-auth-context.type.ts b/packages/twenty-server/src/engine/core-modules/auth/types/workspace-auth-context.type.ts index ec1b79e768..501ccd91dd 100644 --- a/packages/twenty-server/src/engine/core-modules/auth/types/workspace-auth-context.type.ts +++ b/packages/twenty-server/src/engine/core-modules/auth/types/workspace-auth-context.type.ts @@ -23,6 +23,7 @@ export interface UserWorkspaceAuthContext extends BaseWorkspaceAuthContext { user: NonNullable; workspaceMemberId: NonNullable; workspaceMember: NonNullable; + application?: NonNullable; } export interface ApplicationWorkspaceAuthContext extends BaseWorkspaceAuthContext { diff --git a/packages/twenty-server/src/engine/core-modules/auth/utils/build-user-auth-context.util.ts b/packages/twenty-server/src/engine/core-modules/auth/utils/build-user-auth-context.util.ts index d3cc9d37ae..40878ed341 100644 --- a/packages/twenty-server/src/engine/core-modules/auth/utils/build-user-auth-context.util.ts +++ b/packages/twenty-server/src/engine/core-modules/auth/utils/build-user-auth-context.util.ts @@ -1,3 +1,5 @@ +import { isDefined } from 'twenty-shared/utils'; + import { type RawAuthContext } from 'src/engine/core-modules/auth/types/raw-auth-context.type'; import { type UserWorkspaceAuthContext } from 'src/engine/core-modules/auth/types/workspace-auth-context.type'; @@ -7,6 +9,7 @@ type UserAuthContextInput = { user: NonNullable; workspaceMemberId: NonNullable; workspaceMember: NonNullable; + application?: RawAuthContext['application']; }; export const buildUserAuthContext = ( @@ -19,5 +22,6 @@ export const buildUserAuthContext = ( user: input.user, workspaceMemberId: input.workspaceMemberId, workspaceMember: input.workspaceMember, + ...(isDefined(input.application) ? { application: input.application } : {}), }; }; diff --git a/packages/twenty-server/src/engine/metadata-modules/permissions/__tests__/permissions.service.spec.ts b/packages/twenty-server/src/engine/metadata-modules/permissions/__tests__/permissions.service.spec.ts index bb8d21c209..478a53e361 100644 --- a/packages/twenty-server/src/engine/metadata-modules/permissions/__tests__/permissions.service.spec.ts +++ b/packages/twenty-server/src/engine/metadata-modules/permissions/__tests__/permissions.service.spec.ts @@ -8,6 +8,7 @@ import { import { ApiKeyRoleService } from 'src/engine/core-modules/api-key/services/api-key-role.service'; import { ApplicationEntity } from 'src/engine/core-modules/application/application.entity'; +import { ApplicationException } from 'src/engine/core-modules/application/application.exception'; import { createEmptyFlatEntityMaps } from 'src/engine/metadata-modules/flat-entity/constant/create-empty-flat-entity-maps.constant'; import { type SyncableFlatEntity } from 'src/engine/metadata-modules/flat-entity/types/flat-entity-from.type'; import { type FlatEntityMaps } from 'src/engine/metadata-modules/flat-entity/types/flat-entity-maps.type'; @@ -37,6 +38,8 @@ describe('PermissionsService', () => { let service: PermissionsService; let roleRepository: { find: jest.Mock }; let workspaceCacheService: { getOrRecompute: jest.Mock }; + let userRoleService: { getRolesByUserWorkspaces: jest.Mock }; + let applicationRepository: { findOne: jest.Mock }; beforeEach(async () => { roleRepository = { @@ -45,6 +48,12 @@ describe('PermissionsService', () => { workspaceCacheService = { getOrRecompute: jest.fn(), }; + userRoleService = { + getRolesByUserWorkspaces: jest.fn(), + }; + applicationRepository = { + findOne: jest.fn(), + }; const module: TestingModule = await Test.createTestingModule({ providers: [ @@ -59,7 +68,7 @@ describe('PermissionsService', () => { }, { provide: UserRoleService, - useValue: {}, + useValue: userRoleService, }, { provide: WorkspaceCacheService, @@ -67,7 +76,7 @@ describe('PermissionsService', () => { }, { provide: getRepositoryToken(ApplicationEntity), - useValue: {}, + useValue: applicationRepository, }, ], }).compile(); @@ -637,4 +646,169 @@ describe('PermissionsService', () => { }); }, ); + + describe('userHasWorkspaceSettingPermission with an application acting for a user', () => { + const workspaceId = 'test-workspace-id'; + const userWorkspaceId = 'test-user-workspace-id'; + const applicationId = 'test-application-id'; + const setting = PermissionFlagType.DATA_MODEL; + + const createFlatRole = ({ + id, + canUpdateAllSettings, + }: { + id: string; + canUpdateAllSettings: boolean; + }): FlatRole => + ({ + id, + universalIdentifier: `${id}-universal-identifier`, + canAccessAllTools: false, + canUpdateAllSettings, + rolePermissionFlagIds: [], + }) as unknown as FlatRole; + + const mockUserRole = (role: FlatRole) => { + userRoleService.getRolesByUserWorkspaces.mockResolvedValue( + new Map([ + [ + userWorkspaceId, + [ + { + id: role.id, + canUpdateAllSettings: role.canUpdateAllSettings, + canAccessAllTools: role.canAccessAllTools, + rolePermissionFlags: [], + } as unknown as RoleEntity, + ], + ], + ]), + ); + }; + + const mockApplicationRole = (defaultRoleId: string | null) => { + applicationRepository.findOne.mockResolvedValue({ + id: applicationId, + defaultRoleId, + }); + }; + + const mockCachedRoles = (roles: FlatRole[]) => { + workspaceCacheService.getOrRecompute.mockResolvedValue({ + flatRoleMaps: buildFlatEntityMaps(roles), + flatRolePermissionFlagMaps: buildFlatEntityMaps([]), + }); + }; + + const check = () => + service.userHasWorkspaceSettingPermission({ + userWorkspaceId, + workspaceId, + setting, + applicationId, + }); + + it('should deny when the application role denies, even though the user role grants', async () => { + const userRole = createFlatRole({ + id: 'user-role-id', + canUpdateAllSettings: true, + }); + const applicationRole = createFlatRole({ + id: 'application-role-id', + canUpdateAllSettings: false, + }); + + mockUserRole(userRole); + mockApplicationRole(applicationRole.id); + mockCachedRoles([userRole, applicationRole]); + + await expect(check()).resolves.toBe(false); + }); + + it('should deny when the user role denies, even though the application role grants', async () => { + const userRole = createFlatRole({ + id: 'user-role-id', + canUpdateAllSettings: false, + }); + const applicationRole = createFlatRole({ + id: 'application-role-id', + canUpdateAllSettings: true, + }); + + mockUserRole(userRole); + mockApplicationRole(applicationRole.id); + mockCachedRoles([userRole, applicationRole]); + + await expect(check()).resolves.toBe(false); + }); + + it('should grant when both roles grant', async () => { + const userRole = createFlatRole({ + id: 'user-role-id', + canUpdateAllSettings: true, + }); + const applicationRole = createFlatRole({ + id: 'application-role-id', + canUpdateAllSettings: true, + }); + + mockUserRole(userRole); + mockApplicationRole(applicationRole.id); + mockCachedRoles([userRole, applicationRole]); + + await expect(check()).resolves.toBe(true); + }); + + it('should fall back to the user role when the application declares none', async () => { + const userRole = createFlatRole({ + id: 'user-role-id', + canUpdateAllSettings: true, + }); + + mockUserRole(userRole); + mockApplicationRole(null); + roleRepository.find.mockResolvedValue([]); + + await expect(check()).resolves.toBe(true); + }); + + it('should grant when the application declares the user own role', async () => { + const userRole = createFlatRole({ + id: 'user-role-id', + canUpdateAllSettings: true, + }); + + mockUserRole(userRole); + mockApplicationRole(userRole.id); + + await expect(check()).resolves.toBe(true); + }); + + it('should reject when the application no longer exists', async () => { + mockUserRole( + createFlatRole({ id: 'user-role-id', canUpdateAllSettings: true }), + ); + applicationRepository.findOne.mockResolvedValue(null); + + await expect(check()).rejects.toThrow(ApplicationException); + }); + + it('should not consult the application when the request carries none', async () => { + const userRole = createFlatRole({ + id: 'user-role-id', + canUpdateAllSettings: true, + }); + + mockUserRole(userRole); + + await expect( + service.userHasWorkspaceSettingPermission({ + userWorkspaceId, + workspaceId, + setting, + }), + ).resolves.toBe(true); + expect(applicationRepository.findOne).not.toHaveBeenCalled(); + }); + }); }); diff --git a/packages/twenty-server/src/engine/metadata-modules/permissions/permissions.service.ts b/packages/twenty-server/src/engine/metadata-modules/permissions/permissions.service.ts index f6892ebc73..17e34ff6ab 100644 --- a/packages/twenty-server/src/engine/metadata-modules/permissions/permissions.service.ts +++ b/packages/twenty-server/src/engine/metadata-modules/permissions/permissions.service.ts @@ -29,6 +29,7 @@ import { RoleEntity } from 'src/engine/metadata-modules/role/role.entity'; import { UserRoleService } from 'src/engine/metadata-modules/user-role/user-role.service'; import { type RolePermissionConfig } from 'src/engine/twenty-orm/types/role-permission-config'; import { getRoleIdsFromRolePermissionConfig } from 'src/engine/twenty-orm/utils/get-role-ids-from-role-permission-config.util'; +import { resolveRoleIdsForUser } from 'src/engine/twenty-orm/utils/resolve-role-ids-for-user.util'; import { InjectWorkspaceScopedRepository } from 'src/engine/twenty-orm/workspace-scoped-repository/inject-workspace-scoped-repository.decorator'; import { WorkspaceScopedRepository } from 'src/engine/twenty-orm/workspace-scoped-repository/workspace-scoped-repository'; import { WorkspaceCacheService } from 'src/engine/workspace-cache/services/workspace-cache.service'; @@ -201,6 +202,26 @@ export class PermissionsService { ); } + const applicationRoleId = isDefined(applicationId) + ? await this.findApplicationDefaultRoleIdOrThrow({ + applicationId, + workspaceId, + }) + : undefined; + + const roleIds = resolveRoleIdsForUser({ + userRoleId: roleOfUserWorkspace.id, + applicationRoleId, + }); + + if (roleIds.length > 1) { + return this.checkRolesPermissions( + { intersectionOf: roleIds }, + workspaceId, + setting, + ); + } + return this.checkRolePermissions(roleOfUserWorkspace, setting); } @@ -248,6 +269,29 @@ export class PermissionsService { ); } + // Naming an application that no longer exists is not the same as declaring + // no role, and must not fall back to the full permissions of the user. + private async findApplicationDefaultRoleIdOrThrow({ + applicationId, + workspaceId, + }: { + applicationId: string; + workspaceId: string; + }): Promise { + const application = await this.applicationRepository.findOne({ + where: { id: applicationId, workspaceId }, + }); + + if (!isDefined(application)) { + throw new ApplicationException( + `Could not find application ${applicationId}`, + ApplicationExceptionCode.APPLICATION_NOT_FOUND, + ); + } + + return application.defaultRoleId ?? undefined; + } + public checkRolePermissions( role: RoleEntity, setting: PermissionFlagType, diff --git a/packages/twenty-server/src/engine/subscriptions/__tests__/event-stream.service.spec.ts b/packages/twenty-server/src/engine/subscriptions/__tests__/event-stream.service.spec.ts index 140fd1619e..41baeee0be 100644 --- a/packages/twenty-server/src/engine/subscriptions/__tests__/event-stream.service.spec.ts +++ b/packages/twenty-server/src/engine/subscriptions/__tests__/event-stream.service.spec.ts @@ -184,4 +184,77 @@ describe('EventStreamService', () => { [ACTIVE_STREAM_EXPIRATION_MEMBER, `${WORKSPACE_ID}:stale-stream-id`], ); }); + describe('isAuthorized', () => { + const USER_WORKSPACE_ID = 'user-workspace-id'; + const APPLICATION_ID = 'application-id'; + + const check = ( + callerAuthContext: Record, + streamAuthContext: Record, + ) => + service.isAuthorized({ + streamData: { authContext: streamAuthContext } as never, + authContext: callerAuthContext as never, + }); + + it('authorizes the same user on a stream carrying no application', async () => { + await expect( + check( + { userWorkspaceId: USER_WORKSPACE_ID }, + { userWorkspaceId: USER_WORKSPACE_ID }, + ), + ).resolves.toBe(true); + }); + + it('authorizes the same user carrying the same application', async () => { + await expect( + check( + { userWorkspaceId: USER_WORKSPACE_ID, applicationId: APPLICATION_ID }, + { userWorkspaceId: USER_WORKSPACE_ID, applicationId: APPLICATION_ID }, + ), + ).resolves.toBe(true); + }); + + it('denies the same user when the application is dropped', async () => { + await expect( + check( + { userWorkspaceId: USER_WORKSPACE_ID }, + { userWorkspaceId: USER_WORKSPACE_ID, applicationId: APPLICATION_ID }, + ), + ).resolves.toBe(false); + }); + + it('denies the same user carrying a different application', async () => { + await expect( + check( + { + userWorkspaceId: USER_WORKSPACE_ID, + applicationId: 'other-application-id', + }, + { userWorkspaceId: USER_WORKSPACE_ID, applicationId: APPLICATION_ID }, + ), + ).resolves.toBe(false); + }); + + it('denies a different user', async () => { + await expect( + check( + { userWorkspaceId: 'other-user-workspace-id' }, + { userWorkspaceId: USER_WORKSPACE_ID }, + ), + ).resolves.toBe(false); + }); + + it('authorizes the same api key', async () => { + await expect( + check({ apiKeyId: 'api-key-id' }, { apiKeyId: 'api-key-id' }), + ).resolves.toBe(true); + }); + + it('denies a request carrying no principal', async () => { + await expect( + check({}, { userWorkspaceId: USER_WORKSPACE_ID }), + ).resolves.toBe(false); + }); + }); }); diff --git a/packages/twenty-server/src/engine/subscriptions/event-stream.resolver.ts b/packages/twenty-server/src/engine/subscriptions/event-stream.resolver.ts index 3a04160fa2..8d833070a4 100644 --- a/packages/twenty-server/src/engine/subscriptions/event-stream.resolver.ts +++ b/packages/twenty-server/src/engine/subscriptions/event-stream.resolver.ts @@ -5,12 +5,14 @@ import { isDefined } from 'twenty-shared/utils'; import { MetadataResolver } from 'src/engine/api/graphql/graphql-config/decorators/metadata-resolver.decorator'; import { type ApiKeyEntity } from 'src/engine/core-modules/api-key/api-key.entity'; +import { type FlatApplication } from 'src/engine/core-modules/application/types/flat-application.type'; import { ExceptionHandlerService } from 'src/engine/core-modules/exception-handler/exception-handler.service'; import { PreventNestToAutoLogGraphqlErrorsFilter } from 'src/engine/core-modules/graphql/filters/prevent-nest-to-auto-log-graphql-errors.filter'; import { ResolverValidationPipe } from 'src/engine/core-modules/graphql/pipes/resolver-validation.pipe'; import { type AuthContextUser } from 'src/engine/core-modules/auth/types/auth-context.type'; import { WorkspaceEntity } from 'src/engine/core-modules/workspace/workspace.entity'; import { AuthApiKey } from 'src/engine/decorators/auth/auth-api-key.decorator'; +import { AuthApplication } from 'src/engine/decorators/auth/auth-application.decorator'; import { AuthUserWorkspaceId } from 'src/engine/decorators/auth/auth-user-workspace-id.decorator'; import { AuthUser } from 'src/engine/decorators/auth/auth-user.decorator'; import { AuthWorkspace } from 'src/engine/decorators/auth/auth-workspace.decorator'; @@ -64,6 +66,8 @@ export class EventStreamResolver { @AuthUserWorkspaceId({ allowUndefined: true }) userWorkspaceId: string | undefined, @AuthApiKey() apiKey: ApiKeyEntity | undefined, + @AuthApplication({ allowUndefined: true }) + application: FlatApplication | undefined, ) { const eventStreamChannelId = eventStreamIdToChannelId(eventStreamId); @@ -78,6 +82,7 @@ export class EventStreamResolver { authContext: { userWorkspaceId, apiKeyId: apiKey?.id, + applicationId: application?.id, }, }); @@ -101,6 +106,7 @@ export class EventStreamResolver { userId: user?.id, userWorkspaceId, apiKeyId: apiKey?.id, + applicationId: application?.id, }, }); @@ -170,6 +176,8 @@ export class EventStreamResolver { @AuthUserWorkspaceId({ allowUndefined: true }) userWorkspaceId: string | undefined, @AuthApiKey() apiKey: ApiKeyEntity | undefined, + @AuthApplication({ allowUndefined: true }) + application: FlatApplication | undefined, ): Promise { const eventStreamChannelId = eventStreamIdToChannelId(input.eventStreamId); const streamData = await this.eventStreamService.getStreamData( @@ -186,6 +194,7 @@ export class EventStreamResolver { authContext: { userWorkspaceId, apiKeyId: apiKey?.id, + applicationId: application?.id, }, }); @@ -214,6 +223,8 @@ export class EventStreamResolver { @AuthUserWorkspaceId({ allowUndefined: true }) userWorkspaceId: string | undefined, @AuthApiKey() apiKey: ApiKeyEntity | undefined, + @AuthApplication({ allowUndefined: true }) + application: FlatApplication | undefined, ): Promise { const eventStreamChannelId = eventStreamIdToChannelId(input.eventStreamId); @@ -231,6 +242,7 @@ export class EventStreamResolver { authContext: { userWorkspaceId, apiKeyId: apiKey?.id, + applicationId: application?.id, }, }); diff --git a/packages/twenty-server/src/engine/subscriptions/event-stream.service.ts b/packages/twenty-server/src/engine/subscriptions/event-stream.service.ts index 47893bb15a..48d2239be5 100644 --- a/packages/twenty-server/src/engine/subscriptions/event-stream.service.ts +++ b/packages/twenty-server/src/engine/subscriptions/event-stream.service.ts @@ -175,7 +175,10 @@ export class EventStreamService implements OnModuleInit { }): Promise { if (isDefined(authContext.userWorkspaceId)) { return ( - streamData.authContext.userWorkspaceId === authContext.userWorkspaceId + streamData.authContext.userWorkspaceId === + authContext.userWorkspaceId && + (streamData.authContext.applicationId ?? null) === + (authContext.applicationId ?? null) ); } diff --git a/packages/twenty-server/src/engine/subscriptions/object-record-event/__tests__/object-record-event-publisher.spec.ts b/packages/twenty-server/src/engine/subscriptions/object-record-event/__tests__/object-record-event-publisher.spec.ts index 148a665c9d..2e1d7dbbac 100644 --- a/packages/twenty-server/src/engine/subscriptions/object-record-event/__tests__/object-record-event-publisher.spec.ts +++ b/packages/twenty-server/src/engine/subscriptions/object-record-event/__tests__/object-record-event-publisher.spec.ts @@ -2,11 +2,13 @@ import { Test, type TestingModule } from '@nestjs/testing'; import { FieldMetadataType, + type ObjectsPermissions, type ObjectsPermissionsByRoleId, type RecordGqlOperationFilter, } from 'twenty-shared/types'; import { ProcessNestedRelationsHelper } from 'src/engine/api/common/common-nested-relations-processor/process-nested-relations.helper'; +import { type FlatApplicationCacheMaps } from 'src/engine/core-modules/application/types/flat-application-cache-maps.type'; import { CommonSelectFieldsHelper } from 'src/engine/api/common/common-select-fields/common-select-fields-helper'; import { createEmptyFlatEntityMaps } from 'src/engine/metadata-modules/flat-entity/constant/create-empty-flat-entity-maps.constant'; import { WorkspaceManyOrAllFlatEntityMapsCacheService } from 'src/engine/metadata-modules/flat-entity/services/workspace-many-or-all-flat-entity-maps-cache.service'; @@ -163,6 +165,13 @@ describe('ObjectRecordEventPublisher', () => { ...overrides, }); + type PermissionsContextOverrides = { + flatFieldMetadataMaps?: FlatEntityMaps; + userWorkspaceRoleMap?: Record; + rolesPermissions?: ObjectsPermissionsByRoleId; + flatApplicationMaps?: FlatApplicationCacheMaps; + }; + const mockFlatWorkspaceMemberMaps = { byId: { 'test-workspace-member-id': { @@ -178,11 +187,7 @@ describe('ObjectRecordEventPublisher', () => { }; const createPermissionsContext = ( - overrides: { - flatFieldMetadataMaps?: FlatEntityMaps; - userWorkspaceRoleMap?: Record; - rolesPermissions?: ObjectsPermissionsByRoleId; - } = {}, + overrides: PermissionsContextOverrides = {}, ) => ({ flatRowLevelPermissionPredicateMaps: { byId: {}, @@ -199,14 +204,14 @@ describe('ObjectRecordEventPublisher', () => { userWorkspaceRoleMap: overrides.userWorkspaceRoleMap ?? mockUserWorkspaceRoleMap, rolesPermissions: overrides.rolesPermissions ?? mockRolesPermissions, + flatApplicationMaps: overrides.flatApplicationMaps ?? { + byId: {}, + idByUniversalIdentifier: {}, + }, }); const createCacheMock = ( - permissionsOverrides: { - flatFieldMetadataMaps?: FlatEntityMaps; - userWorkspaceRoleMap?: Record; - rolesPermissions?: ObjectsPermissionsByRoleId; - } = {}, + permissionsOverrides: PermissionsContextOverrides = {}, workspaceMemberMapsOverride?: { byId: Record; idByUserId: Record; @@ -756,6 +761,160 @@ describe('ObjectRecordEventPublisher', () => { ).not.toHaveBeenCalled(); }); + describe('stream opened by an application acting for a user', () => { + const applicationId = 'test-application-id'; + const applicationRoleId = 'test-application-role-id'; + + const mockApplicationStream = ( + overrides: PermissionsContextOverrides = {}, + ) => { + mockEventStreamService.getStreamsData.mockResolvedValue( + new Map([ + [ + streamChannelId, + { + ...mockStreamData, + authContext: { ...mockStreamData.authContext, applicationId }, + }, + ], + ]) as Map, + ); + + mockWorkspaceCacheService.getOrRecompute.mockImplementation( + createCacheMock({ + flatApplicationMaps: { + byId: { + [applicationId]: { + id: applicationId, + defaultRoleId: applicationRoleId, + } as never, + }, + idByUniversalIdentifier: {}, + }, + ...overrides, + }), + ); + }; + + const publishCompanyCreated = async () => { + const eventBatch: WorkspaceEventBatch = { + name: 'company.created', + workspaceId, + objectMetadata: companyObjectMetadata, + events: [createMockEvent()], + }; + + await service.publish(eventBatch as WorkspaceEventBatch); + }; + + const buildRolePermissions = ( + canReadObjectRecords: boolean, + ): ObjectsPermissions => ({ + [companyObjectMetadata.id]: { + canReadObjectRecords, + canUpdateObjectRecords: true, + canSoftDeleteObjectRecords: true, + canDestroyObjectRecords: true, + restrictedFields: {}, + rowLevelPermissionPredicates: [], + rowLevelPermissionPredicateGroups: [], + }, + }); + + it('should not publish when the application role denies read', async () => { + mockApplicationStream({ + rolesPermissions: { + [roleId]: buildRolePermissions(true), + [applicationRoleId]: buildRolePermissions(false), + }, + }); + + await publishCompanyCreated(); + + expect( + mockSubscriptionService.publishToEventStream, + ).not.toHaveBeenCalled(); + }); + + it('should publish when both roles allow read', async () => { + mockApplicationStream({ + rolesPermissions: { + [roleId]: buildRolePermissions(true), + [applicationRoleId]: buildRolePermissions(true), + }, + }); + + await publishCompanyCreated(); + + expect(mockSubscriptionService.publishToEventStream).toHaveBeenCalled(); + }); + + it('should not publish when the application role cannot be resolved', async () => { + mockApplicationStream({ + rolesPermissions: { [roleId]: buildRolePermissions(true) }, + }); + + await publishCompanyCreated(); + + expect( + mockSubscriptionService.publishToEventStream, + ).not.toHaveBeenCalled(); + }); + + it('should filter row level rules with both roles', async () => { + mockApplicationStream({ + rolesPermissions: { + [roleId]: buildRolePermissions(true), + [applicationRoleId]: buildRolePermissions(true), + }, + }); + + await publishCompanyCreated(); + + expect(buildRowLevelPermissionRecordFilter).toHaveBeenCalledWith( + expect.objectContaining({ roleIds: [roleId, applicationRoleId] }), + ); + }); + + it('should not publish when the application no longer exists', async () => { + mockApplicationStream({ + flatApplicationMaps: { byId: {}, idByUniversalIdentifier: {} }, + rolesPermissions: { [roleId]: buildRolePermissions(true) }, + }); + + await publishCompanyCreated(); + + expect( + mockSubscriptionService.publishToEventStream, + ).not.toHaveBeenCalled(); + }); + + it('should not publish when the application has been soft deleted', async () => { + mockApplicationStream({ + flatApplicationMaps: { + byId: { + [applicationId]: { + id: applicationId, + defaultRoleId: applicationRoleId, + deletedAt: new Date(), + } as never, + }, + idByUniversalIdentifier: {}, + }, + rolesPermissions: { + [roleId]: buildRolePermissions(true), + [applicationRoleId]: buildRolePermissions(true), + }, + }); + + await publishCompanyCreated(); + + expect( + mockSubscriptionService.publishToEventStream, + ).not.toHaveBeenCalled(); + }); + }); + it('should combine query filter with RLS filter', async () => { const rlsFilter: RecordGqlOperationFilter = { status: { eq: 'active' } }; diff --git a/packages/twenty-server/src/engine/subscriptions/object-record-event/object-record-event-publisher.ts b/packages/twenty-server/src/engine/subscriptions/object-record-event/object-record-event-publisher.ts index 6c7f941b81..cbb71085e6 100644 --- a/packages/twenty-server/src/engine/subscriptions/object-record-event/object-record-event-publisher.ts +++ b/packages/twenty-server/src/engine/subscriptions/object-record-event/object-record-event-publisher.ts @@ -5,6 +5,7 @@ import { type ObjectRecordEvent } from 'twenty-shared/database-events'; import { Nullable, ObjectRecord, + type ObjectsPermissions, type ObjectsPermissionsByRoleId, type RecordGqlOperationFilter, type RecordGqlOperationSignature, @@ -13,6 +14,7 @@ import { import { combineFilters, isDefined, + isNonEmptyArray, isRecordGqlOperationSignature, } from 'twenty-shared/utils'; import { FindOptionsRelations, ObjectLiteral } from 'typeorm'; @@ -21,6 +23,8 @@ import { ProcessNestedRelationsHelper } from 'src/engine/api/common/common-neste import { CommonSelectFieldsHelper } from 'src/engine/api/common/common-select-fields/common-select-fields-helper'; import { DatabaseEventAction } from 'src/engine/api/graphql/graphql-query-runner/enums/database-event-action'; import { GraphqlQueryParser } from 'src/engine/api/graphql/graphql-query-runner/graphql-query-parsers/graphql-query.parser'; +import { type FlatApplicationCacheMaps } from 'src/engine/core-modules/application/types/flat-application-cache-maps.type'; +import { findActiveFlatApplicationById } from 'src/engine/core-modules/application/utils/find-active-flat-application-by-id.util'; import { type SerializableAuthContext } from 'src/engine/core-modules/auth/types/serializable-auth-context.type'; import { type WorkspaceAuthContext } from 'src/engine/core-modules/auth/types/workspace-auth-context.type'; import { type FlatWorkspaceMemberMaps } from 'src/engine/core-modules/user/types/flat-workspace-member-maps.type'; @@ -43,11 +47,22 @@ import { ObjectRecordSubscriptionEvent } from 'src/engine/subscriptions/types/ob import { GlobalWorkspaceOrmManager } from 'src/engine/twenty-orm/global-workspace-datasource/global-workspace-orm.manager'; import { RolePermissionConfig } from 'src/engine/twenty-orm/types/role-permission-config'; import { buildRowLevelPermissionRecordFilter } from 'src/engine/twenty-orm/utils/build-row-level-permission-record-filter.util'; +import { computePermissionIntersection } from 'src/engine/twenty-orm/utils/compute-permission-intersection.util'; import { isRecordMatchingRLSRowLevelPermissionPredicate } from 'src/engine/twenty-orm/utils/is-record-matching-rls-row-level-permission-predicate.util'; +import { resolveRoleIdsForUser } from 'src/engine/twenty-orm/utils/resolve-role-ids-for-user.util'; import { WorkspaceCacheService } from 'src/engine/workspace-cache/services/workspace-cache.service'; import { WorkspaceEventBatch } from 'src/engine/workspace-event-emitter/types/workspace-event-batch.type'; import { parseEventNameOrThrow } from 'src/engine/workspace-event-emitter/utils/parse-event-name'; +type StreamPermissionsContext = { + flatRowLevelPermissionPredicateMaps: FlatRowLevelPermissionPredicateMaps; + flatRowLevelPermissionPredicateGroupMaps: FlatRowLevelPermissionPredicateGroupMaps; + flatFieldMetadataMaps: FlatEntityMaps; + userWorkspaceRoleMap: UserWorkspaceRoleMap; + rolesPermissions: ObjectsPermissionsByRoleId; + flatApplicationMaps: FlatApplicationCacheMaps; +}; + @Injectable() export class ObjectRecordEventPublisher { private readonly logger = new Logger(ObjectRecordEventPublisher.name); @@ -129,31 +144,29 @@ export class ObjectRecordEventPublisher { streamChannelId: string; streamData: EventStreamData; workspaceEventBatch: WorkspaceEventBatch; - permissionsContext: { - flatRowLevelPermissionPredicateMaps: FlatRowLevelPermissionPredicateMaps; - flatRowLevelPermissionPredicateGroupMaps: FlatRowLevelPermissionPredicateGroupMaps; - flatFieldMetadataMaps: FlatEntityMaps; - userWorkspaceRoleMap: Record; - rolesPermissions: ObjectsPermissionsByRoleId; - }; + permissionsContext: StreamPermissionsContext; flatWorkspaceMemberMaps: FlatWorkspaceMemberMaps; }): Promise { - const { userWorkspaceId } = streamData.authContext; + const roleIds = this.resolveStreamRoleIds( + streamData.authContext, + permissionsContext, + ); - if (!isDefined(userWorkspaceId)) { + if (!isNonEmptyArray(roleIds)) { return; } - const roleId = permissionsContext.userWorkspaceRoleMap[userWorkspaceId]; + const objectsPermissions = this.resolveStreamObjectsPermissions( + roleIds, + permissionsContext.rolesPermissions, + ); - if (!isDefined(roleId)) { + if (!isDefined(objectsPermissions)) { return; } const objectPermissions = - permissionsContext.rolesPermissions[roleId]?.[ - workspaceEventBatch.objectMetadata.id - ]; + objectsPermissions[workspaceEventBatch.objectMetadata.id]; if (!objectPermissions?.canReadObjectRecords) { return; @@ -168,7 +181,7 @@ export class ObjectRecordEventPublisher { const subscriberRLSFilter = this.buildSubscriberRLSFilter( streamData.authContext, - roleId, + roleIds, workspaceEventBatch.objectMetadata, permissionsContext, flatWorkspaceMemberMaps, @@ -228,9 +241,9 @@ export class ObjectRecordEventPublisher { (matchedEvent) => matchedEvent.objectRecordEvent, ), streamData, - permissionsContext, workspaceId: workspaceEventBatch.workspaceId, - roleId, + roleIds, + objectsPermissions, }); } catch (error) { this.logger.warn( @@ -259,21 +272,15 @@ export class ObjectRecordEventPublisher { objectMetadata, events, workspaceId, - permissionsContext, - roleId, + roleIds, + objectsPermissions, }: { streamData: EventStreamData; objectMetadata: FlatObjectMetadata; events: ObjectRecordEvent[]; workspaceId: string; - roleId: string; - permissionsContext: { - flatRowLevelPermissionPredicateMaps: FlatRowLevelPermissionPredicateMaps; - flatRowLevelPermissionPredicateGroupMaps: FlatRowLevelPermissionPredicateGroupMaps; - flatFieldMetadataMaps: FlatEntityMaps; - userWorkspaceRoleMap: UserWorkspaceRoleMap; - rolesPermissions: ObjectsPermissionsByRoleId; - }; + roleIds: string[]; + objectsPermissions: ObjectsPermissions; }) { const { flatFieldMetadataMaps, flatObjectMetadataMaps } = await this.workspaceManyOrAllFlatEntityMapsCacheService.getOrRecomputeManyOrAllFlatEntityMaps( @@ -304,7 +311,7 @@ export class ObjectRecordEventPublisher { } const rolePermissionConfig: RolePermissionConfig = { - intersectionOf: [roleId], + intersectionOf: roleIds, }; const globalWorkspaceDataSource = @@ -315,7 +322,7 @@ export class ObjectRecordEventPublisher { flatObjectMetadata: objectMetadata, flatObjectMetadataMaps, flatFieldMetadataMaps, - objectsPermissions: permissionsContext.rolesPermissions[roleId], + objectsPermissions, onlyUseLabelIdentifierFieldsInRelations: true, recurseIntoJunctionTableRelations: true, }); @@ -346,9 +353,64 @@ export class ObjectRecordEventPublisher { }); } + private resolveStreamRoleIds( + subscriberAuthContext: SerializableAuthContext, + permissionsContext: Pick< + StreamPermissionsContext, + 'userWorkspaceRoleMap' | 'flatApplicationMaps' + >, + ): string[] { + const { userWorkspaceId, applicationId } = subscriberAuthContext; + + if (!isDefined(userWorkspaceId)) { + return []; + } + + const userRoleId = permissionsContext.userWorkspaceRoleMap[userWorkspaceId]; + + if (!isDefined(applicationId)) { + return resolveRoleIdsForUser({ + userRoleId, + applicationRoleId: undefined, + }); + } + + // The cache keeps soft-deleted applications, so absence is not enough. + // An application that has gone away is not one declaring no role: falling + // back to the user alone would widen a stream that is already open. + const application = findActiveFlatApplicationById( + permissionsContext.flatApplicationMaps, + applicationId, + ); + + if (!isDefined(application)) { + return []; + } + + return resolveRoleIdsForUser({ + userRoleId, + applicationRoleId: application.defaultRoleId, + }); + } + + private resolveStreamObjectsPermissions( + roleIds: string[], + rolesPermissions: ObjectsPermissionsByRoleId, + ): ObjectsPermissions | undefined { + const allRolePermissions = roleIds.map( + (roleId) => rolesPermissions[roleId], + ); + + if (!allRolePermissions.every(isDefined)) { + return undefined; + } + + return computePermissionIntersection(allRolePermissions); + } + private buildSubscriberRLSFilter( subscriberAuthContext: SerializableAuthContext, - roleId: string, + roleIds: string[], objectMetadata: FlatObjectMetadata, permissionsContext: { flatRowLevelPermissionPredicateMaps: FlatRowLevelPermissionPredicateMaps; @@ -368,7 +430,7 @@ export class ObjectRecordEventPublisher { permissionsContext.flatRowLevelPermissionPredicateGroupMaps, flatFieldMetadataMaps: permissionsContext.flatFieldMetadataMaps, objectMetadata, - roleId, + roleIds, workspaceMember, }); } @@ -518,25 +580,23 @@ export class ObjectRecordEventPublisher { }); } - private async fetchPermissionsContext(workspaceId: string): Promise<{ - flatRowLevelPermissionPredicateMaps: FlatRowLevelPermissionPredicateMaps; - flatRowLevelPermissionPredicateGroupMaps: FlatRowLevelPermissionPredicateGroupMaps; - flatFieldMetadataMaps: FlatEntityMaps; - userWorkspaceRoleMap: Record; - rolesPermissions: ObjectsPermissionsByRoleId; - }> { + private async fetchPermissionsContext( + workspaceId: string, + ): Promise { const { flatRowLevelPermissionPredicateMaps, flatRowLevelPermissionPredicateGroupMaps, flatFieldMetadataMaps, userWorkspaceRoleMap, rolesPermissions, + flatApplicationMaps, } = await this.workspaceCacheService.getOrRecompute(workspaceId, [ 'flatRowLevelPermissionPredicateMaps', 'flatRowLevelPermissionPredicateGroupMaps', 'flatFieldMetadataMaps', 'userWorkspaceRoleMap', 'rolesPermissions', + 'flatApplicationMaps', ]); return { @@ -545,6 +605,7 @@ export class ObjectRecordEventPublisher { flatFieldMetadataMaps, userWorkspaceRoleMap, rolesPermissions, + flatApplicationMaps, }; } } diff --git 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 index 433267f11e..141e249b19 100644 --- 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 @@ -171,11 +171,14 @@ export class WorkspaceEntityManager extends EntityManager { if (rolePermissionConfig && 'intersectionOf' in rolePermissionConfig) { const allRolePermissions = rolePermissionConfig.intersectionOf.map( - (roleId: string) => - this.getPermissionsForRole(roleId, objectPermissionsByRoleId), + (roleId: string) => objectPermissionsByRoleId?.[roleId], ); - objectPermissions = computePermissionIntersection(allRolePermissions); + // defaultRoleId has no foreign key and can dangle. A bound that cannot + // be resolved denies rather than letting the rest decide alone. + objectPermissions = allRolePermissions.every(isDefined) + ? computePermissionIntersection(allRolePermissions) + : {}; } const newRepository = new WorkspaceRepository( diff --git a/packages/twenty-server/src/engine/twenty-orm/utils/__tests__/build-row-level-permission-record-filter.util.spec.ts b/packages/twenty-server/src/engine/twenty-orm/utils/__tests__/build-row-level-permission-record-filter.util.spec.ts new file mode 100644 index 0000000000..d0aa5f2b55 --- /dev/null +++ b/packages/twenty-server/src/engine/twenty-orm/utils/__tests__/build-row-level-permission-record-filter.util.spec.ts @@ -0,0 +1,117 @@ +import { FieldMetadataType } from 'twenty-shared/types'; + +import { type FlatEntityMaps } from 'src/engine/metadata-modules/flat-entity/types/flat-entity-maps.type'; +import { addFlatEntityToFlatEntityMapsOrThrow } from 'src/engine/metadata-modules/flat-entity/utils/add-flat-entity-to-flat-entity-maps-or-throw.util'; +import { createEmptyFlatEntityMaps } from 'src/engine/metadata-modules/flat-entity/constant/create-empty-flat-entity-maps.constant'; +import { type FlatFieldMetadata } from 'src/engine/metadata-modules/flat-field-metadata/types/flat-field-metadata.type'; +import { type FlatObjectMetadata } from 'src/engine/metadata-modules/flat-object-metadata/types/flat-object-metadata.type'; +import { type FlatRowLevelPermissionPredicateGroupMaps } from 'src/engine/metadata-modules/row-level-permission-predicate/types/flat-row-level-permission-predicate-group-maps.type'; +import { type FlatRowLevelPermissionPredicateMaps } from 'src/engine/metadata-modules/row-level-permission-predicate/types/flat-row-level-permission-predicate-maps.type'; +import { buildRowLevelPermissionRecordFilter } from 'src/engine/twenty-orm/utils/build-row-level-permission-record-filter.util'; + +const OBJECT_ID = 'object-1'; +const FIELD_ID = 'field-1'; +const USER_ROLE_ID = 'user-role-1'; +const APPLICATION_ROLE_ID = 'application-role-1'; +const UNRESTRICTED_ROLE_ID = 'unrestricted-role-1'; + +const buildMaps = ( + entities: ({ id: string; universalIdentifier: string } & Record< + string, + unknown + >)[], +) => + entities.reduce( + (maps, entity) => + addFlatEntityToFlatEntityMapsOrThrow({ + flatEntity: entity as never, + flatEntityMaps: maps, + }), + createEmptyFlatEntityMaps(), + ); + +const flatObjectMetadata = { + id: OBJECT_ID, + nameSingular: 'thing', + namePlural: 'things', + fieldIds: [FIELD_ID], + fieldUniversalIdentifiers: [FIELD_ID], +} as unknown as FlatObjectMetadata; + +const flatFieldMetadataMaps = buildMaps([ + { + id: FIELD_ID, + universalIdentifier: FIELD_ID, + name: 'name', + type: FieldMetadataType.TEXT, + objectMetadataId: OBJECT_ID, + }, +]) as unknown as FlatEntityMaps; + +const buildPredicate = (id: string, roleId: string, value: string) => ({ + id, + universalIdentifier: id, + roleId, + objectMetadataId: OBJECT_ID, + fieldMetadataId: FIELD_ID, + operand: 'CONTAINS', + value, + subFieldName: null, + workspaceMemberFieldMetadataId: null, + workspaceMemberSubFieldName: null, + rowLevelPermissionPredicateGroupId: null, + positionInRowLevelPermissionPredicateGroup: null, + deletedAt: null, +}); + +const flatRowLevelPermissionPredicateMaps = buildMaps([ + buildPredicate('predicate-user', USER_ROLE_ID, 'visible-to-user'), + buildPredicate( + 'predicate-application', + APPLICATION_ROLE_ID, + 'visible-to-application', + ), +]) as unknown as FlatRowLevelPermissionPredicateMaps; + +const flatRowLevelPermissionPredicateGroupMaps = + createEmptyFlatEntityMaps() as unknown as FlatRowLevelPermissionPredicateGroupMaps; + +const build = (roleIds: string[]) => + buildRowLevelPermissionRecordFilter({ + flatRowLevelPermissionPredicateMaps, + flatRowLevelPermissionPredicateGroupMaps, + flatFieldMetadataMaps, + objectMetadata: flatObjectMetadata, + roleIds, + }); + +describe('buildRowLevelPermissionRecordFilter', () => { + it('should return null when no role is given', () => { + expect(build([])).toBeNull(); + }); + + it('should return null when the role has no predicates', () => { + expect(build([UNRESTRICTED_ROLE_ID])).toBeNull(); + }); + + it('should return the role filter as-is for a single role', () => { + expect(build([USER_ROLE_ID])).toEqual({ + name: { ilike: '%visible-to-user%' }, + }); + }); + + it('should keep the restriction when the other role is unrestricted', () => { + expect(build([USER_ROLE_ID, UNRESTRICTED_ROLE_ID])).toEqual({ + name: { ilike: '%visible-to-user%' }, + }); + }); + + it('should require both roles to be satisfied when both restrict', () => { + expect(build([USER_ROLE_ID, APPLICATION_ROLE_ID])).toEqual({ + and: [ + { name: { ilike: '%visible-to-user%' } }, + { name: { ilike: '%visible-to-application%' } }, + ], + }); + }); +}); diff --git a/packages/twenty-server/src/engine/twenty-orm/utils/__tests__/compute-permission-intersection.util.spec.ts b/packages/twenty-server/src/engine/twenty-orm/utils/__tests__/compute-permission-intersection.util.spec.ts index 0695f7b64a..59684a4250 100644 --- a/packages/twenty-server/src/engine/twenty-orm/utils/__tests__/compute-permission-intersection.util.spec.ts +++ b/packages/twenty-server/src/engine/twenty-orm/utils/__tests__/compute-permission-intersection.util.spec.ts @@ -383,4 +383,79 @@ describe('computePermissionIntersection', () => { }); }); }); + + describe('row-level permission predicates', () => { + const buildPermissions = ( + roleId: string, + constrainedFieldMetadataIds: string[], + ): ObjectsPermissions => ({ + [objectMetadataId1]: { + canReadObjectRecords: true, + canUpdateObjectRecords: true, + canSoftDeleteObjectRecords: true, + canDestroyObjectRecords: true, + restrictedFields: {}, + rowLevelPermissionPredicates: constrainedFieldMetadataIds.map( + (fieldMetadataId) => + ({ + id: `${roleId}-${fieldMetadataId}`, + roleId, + fieldMetadataId, + }) as never, + ), + rowLevelPermissionPredicateGroups: [ + { id: `${roleId}-group`, roleId } as never, + ], + }, + }); + + const constrainedFieldMetadataIdsOf = (permissions: ObjectsPermissions) => + permissions[objectMetadataId1].rowLevelPermissionPredicates.map( + (predicate) => predicate.fieldMetadataId, + ); + + it('should keep a field every role constrains', () => { + const result = computePermissionIntersection([ + buildPermissions('user-role-id', ['field-1']), + buildPermissions('application-role-id', ['field-1']), + ]); + + expect(constrainedFieldMetadataIdsOf(result)).toEqual(['field-1']); + }); + + it('should drop a field only one role constrains', () => { + const result = computePermissionIntersection([ + buildPermissions('user-role-id', ['field-1', 'field-2']), + buildPermissions('application-role-id', ['field-1']), + ]); + + expect(constrainedFieldMetadataIdsOf(result)).toEqual(['field-1']); + }); + + it('should drop every field when a role constrains none', () => { + const result = computePermissionIntersection([ + buildPermissions('user-role-id', ['field-1']), + buildPermissions('application-role-id', []), + ]); + + expect(constrainedFieldMetadataIdsOf(result)).toEqual([]); + }); + + it('should not expose a combined predicate group tree', () => { + const result = computePermissionIntersection([ + buildPermissions('user-role-id', ['field-1']), + buildPermissions('application-role-id', ['field-1']), + ]); + + expect( + result[objectMetadataId1].rowLevelPermissionPredicateGroups, + ).toEqual([]); + }); + + it('should leave a single role untouched', () => { + const permissions = buildPermissions('user-role-id', ['field-1']); + + expect(computePermissionIntersection([permissions])).toBe(permissions); + }); + }); }); diff --git a/packages/twenty-server/src/engine/twenty-orm/utils/__tests__/get-objects-permissions-from-role-permission-config.util.spec.ts b/packages/twenty-server/src/engine/twenty-orm/utils/__tests__/get-objects-permissions-from-role-permission-config.util.spec.ts index 1f7d601a8c..35a2e56ec1 100644 --- a/packages/twenty-server/src/engine/twenty-orm/utils/__tests__/get-objects-permissions-from-role-permission-config.util.spec.ts +++ b/packages/twenty-server/src/engine/twenty-orm/utils/__tests__/get-objects-permissions-from-role-permission-config.util.spec.ts @@ -52,7 +52,7 @@ describe('getObjectsPermissionsFromRolePermissionConfig', () => { ).toEqual(defaultRolePermissions); }); - it('should use the first role when multiple are provided', () => { + it('should intersect every role when several are provided', () => { expect( getObjectsPermissionsFromRolePermissionConfig({ rolesPermissions, @@ -60,7 +60,29 @@ describe('getObjectsPermissionsFromRolePermissionConfig', () => { intersectionOf: ['agent-role-id', 'default-role-id'], }, }), - ).toEqual(agentRolePermissions); + ).toEqual(defaultRolePermissions); + }); + + it('should not grant a permission that only one of the roles allows', () => { + expect( + getObjectsPermissionsFromRolePermissionConfig({ + rolesPermissions, + rolePermissionConfig: { + intersectionOf: ['default-role-id', 'agent-role-id'], + }, + })[OBJECT_ID].canReadObjectRecords, + ).toBe(false); + }); + + it('should deny when one of the intersected roles is missing from the cache', () => { + expect( + getObjectsPermissionsFromRolePermissionConfig({ + rolesPermissions, + rolePermissionConfig: { + intersectionOf: ['agent-role-id', 'missing-role-id'], + }, + }), + ).toEqual({}); }); it('should return empty permissions when bypassing checks', () => { diff --git a/packages/twenty-server/src/engine/twenty-orm/utils/__tests__/resolve-role-ids-from-auth-context.util.spec.ts b/packages/twenty-server/src/engine/twenty-orm/utils/__tests__/resolve-role-ids-from-auth-context.util.spec.ts new file mode 100644 index 0000000000..25f92d2c7c --- /dev/null +++ b/packages/twenty-server/src/engine/twenty-orm/utils/__tests__/resolve-role-ids-from-auth-context.util.spec.ts @@ -0,0 +1,103 @@ +import { type WorkspaceAuthContext } from 'src/engine/core-modules/auth/types/workspace-auth-context.type'; +import { resolveRoleIdsFromAuthContext } from 'src/engine/twenty-orm/utils/resolve-role-ids-from-auth-context.util'; + +const USER_WORKSPACE_ID = 'user-workspace-1'; +const USER_ROLE_ID = 'user-role-1'; +const APPLICATION_ROLE_ID = 'application-role-1'; +const API_KEY_ID = 'api-key-1'; +const API_KEY_ROLE_ID = 'api-key-role-1'; + +const userWorkspaceRoleMap = { [USER_WORKSPACE_ID]: USER_ROLE_ID }; +const apiKeyRoleMap = { [API_KEY_ID]: API_KEY_ROLE_ID }; + +const buildUserContext = (application?: { + defaultRoleId: string | null; +}): WorkspaceAuthContext => + ({ + type: 'user', + workspace: { id: 'workspace-1' }, + userWorkspaceId: USER_WORKSPACE_ID, + user: { id: 'user-1' }, + workspaceMemberId: 'workspace-member-1', + workspaceMember: { id: 'workspace-member-1' }, + ...(application ? { application } : {}), + }) as unknown as WorkspaceAuthContext; + +const resolve = (authContext: WorkspaceAuthContext) => + resolveRoleIdsFromAuthContext({ + authContext, + userWorkspaceRoleMap, + apiKeyRoleMap, + }); + +describe('resolveRoleIdsFromAuthContext', () => { + it('should resolve the user role alone for a plain user request', () => { + expect(resolve(buildUserContext())).toEqual([USER_ROLE_ID]); + }); + + it('should resolve both roles when an application acts on the user behalf', () => { + expect( + resolve(buildUserContext({ defaultRoleId: APPLICATION_ROLE_ID })), + ).toEqual([USER_ROLE_ID, APPLICATION_ROLE_ID]); + }); + + it('should add no bound when the application declares no role', () => { + expect(resolve(buildUserContext({ defaultRoleId: null }))).toEqual([ + USER_ROLE_ID, + ]); + }); + + it('should resolve the role once when the application declares the user own role', () => { + expect(resolve(buildUserContext({ defaultRoleId: USER_ROLE_ID }))).toEqual([ + USER_ROLE_ID, + ]); + }); + + it('should resolve nothing when the user has no role, even with an application', () => { + const contextWithUnknownUserWorkspace = { + ...buildUserContext({ defaultRoleId: APPLICATION_ROLE_ID }), + userWorkspaceId: 'unknown-user-workspace', + } as WorkspaceAuthContext; + + expect(resolve(contextWithUnknownUserWorkspace)).toEqual([]); + }); + + it('should resolve the api key role', () => { + expect( + resolve({ + type: 'apiKey', + workspace: { id: 'workspace-1' }, + apiKey: { id: API_KEY_ID }, + } as unknown as WorkspaceAuthContext), + ).toEqual([API_KEY_ROLE_ID]); + }); + + it('should resolve the application role for an application-only request', () => { + expect( + resolve({ + type: 'application', + workspace: { id: 'workspace-1' }, + application: { defaultRoleId: APPLICATION_ROLE_ID }, + } as unknown as WorkspaceAuthContext), + ).toEqual([APPLICATION_ROLE_ID]); + }); + + it('should resolve nothing for an application-only request with no declared role', () => { + expect( + resolve({ + type: 'application', + workspace: { id: 'workspace-1' }, + application: { defaultRoleId: null }, + } as unknown as WorkspaceAuthContext), + ).toEqual([]); + }); + + it('should resolve nothing for a system request', () => { + expect( + resolve({ + type: 'system', + workspace: { id: 'workspace-1' }, + } as unknown as WorkspaceAuthContext), + ).toEqual([]); + }); +}); diff --git a/packages/twenty-server/src/engine/twenty-orm/utils/apply-row-level-permission-predicates.util.ts b/packages/twenty-server/src/engine/twenty-orm/utils/apply-row-level-permission-predicates.util.ts index 3588ad25e5..7e00a20f21 100644 --- a/packages/twenty-server/src/engine/twenty-orm/utils/apply-row-level-permission-predicates.util.ts +++ b/packages/twenty-server/src/engine/twenty-orm/utils/apply-row-level-permission-predicates.util.ts @@ -16,7 +16,7 @@ import { type WorkspaceAuthContext } from 'src/engine/core-modules/auth/types/wo import { type FlatObjectMetadata } from 'src/engine/metadata-modules/flat-object-metadata/types/flat-object-metadata.type'; import { type WorkspaceSelectQueryBuilder } from 'src/engine/twenty-orm/repository/workspace-select-query-builder'; import { buildRowLevelPermissionRecordFilter } from 'src/engine/twenty-orm/utils/build-row-level-permission-record-filter.util'; -import { resolveRoleIdFromAuthContext } from 'src/engine/twenty-orm/utils/resolve-role-id-from-auth-context.util'; +import { resolveRoleIdsFromAuthContext } from 'src/engine/twenty-orm/utils/resolve-role-ids-from-auth-context.util'; type ApplyRowLevelPermissionPredicatesArgs = { queryBuilder: WorkspaceSelectQueryBuilder; @@ -33,7 +33,7 @@ export const applyRowLevelPermissionPredicates = ({ authContext, featureFlagMap: _featureFlagMap, }: ApplyRowLevelPermissionPredicatesArgs): void => { - const roleId = resolveRoleIdFromAuthContext({ + const roleIds = resolveRoleIdsFromAuthContext({ authContext, userWorkspaceRoleMap: internalContext.userWorkspaceRoleMap, apiKeyRoleMap: internalContext.apiKeyRoleMap, @@ -46,7 +46,7 @@ export const applyRowLevelPermissionPredicates = ({ internalContext.flatRowLevelPermissionPredicateGroupMaps, flatFieldMetadataMaps: internalContext.flatFieldMetadataMaps, objectMetadata, - roleId, + roleIds, workspaceMember: isUserAuthContext(authContext) ? authContext.workspaceMember : undefined, diff --git a/packages/twenty-server/src/engine/twenty-orm/utils/build-row-level-permission-record-filter.util.ts b/packages/twenty-server/src/engine/twenty-orm/utils/build-row-level-permission-record-filter.util.ts index 9d34bae4d9..f74358bdec 100644 --- a/packages/twenty-server/src/engine/twenty-orm/utils/build-row-level-permission-record-filter.util.ts +++ b/packages/twenty-server/src/engine/twenty-orm/utils/build-row-level-permission-record-filter.util.ts @@ -31,27 +31,23 @@ import { type FlatRowLevelPermissionPredicateGroupMaps } from 'src/engine/metada import { type FlatRowLevelPermissionPredicateMaps } from 'src/engine/metadata-modules/row-level-permission-predicate/types/flat-row-level-permission-predicate-maps.type'; import { validateEnumValueCompatibility } from 'src/engine/twenty-orm/utils/validate-enum-value-compatibility.util'; -type BuildRowLevelPermissionRecordFilterArgs = { +type BuildRecordFilterForRoleArgs = { flatRowLevelPermissionPredicateMaps: FlatRowLevelPermissionPredicateMaps; flatRowLevelPermissionPredicateGroupMaps: FlatRowLevelPermissionPredicateGroupMaps; flatFieldMetadataMaps: FlatEntityMaps; objectMetadata: FlatObjectMetadata; - roleId: string | undefined; + roleId: string; workspaceMember?: UserWorkspaceAuthContext['workspaceMember']; }; -export const buildRowLevelPermissionRecordFilter = ({ +const buildRecordFilterForRole = ({ flatRowLevelPermissionPredicateMaps, flatRowLevelPermissionPredicateGroupMaps, flatFieldMetadataMaps, objectMetadata, roleId, workspaceMember, -}: BuildRowLevelPermissionRecordFilterArgs): RecordGqlOperationFilter | null => { - if (!isDefined(roleId)) { - return null; - } - +}: BuildRecordFilterForRoleArgs): RecordGqlOperationFilter | null => { const predicates = Object.values( flatRowLevelPermissionPredicateMaps.byUniversalIdentifier, ) @@ -226,3 +222,35 @@ export const buildRowLevelPermissionRecordFilter = ({ }, }); }; + +type BuildRowLevelPermissionRecordFilterArgs = Omit< + BuildRecordFilterForRoleArgs, + 'roleId' +> & { + roleIds: string[]; +}; + +// Each role compiles on its own and the results are ANDed. Merging the raw +// predicates first would be wrong: compilation honours only the first +// parentless group, so one role's restrictions would vanish and widen access. +export const buildRowLevelPermissionRecordFilter = ({ + roleIds, + ...buildRecordFilterForRoleArgs +}: BuildRowLevelPermissionRecordFilterArgs): RecordGqlOperationFilter | null => { + const recordFilters = roleIds + .map((roleId) => + buildRecordFilterForRole({ ...buildRecordFilterForRoleArgs, roleId }), + ) + .filter(isDefined) + .filter((recordFilter) => Object.keys(recordFilter).length > 0); + + if (recordFilters.length === 0) { + return null; + } + + if (recordFilters.length === 1) { + return recordFilters[0]; + } + + return { and: recordFilters }; +}; diff --git a/packages/twenty-server/src/engine/twenty-orm/utils/compute-permission-intersection.util.ts b/packages/twenty-server/src/engine/twenty-orm/utils/compute-permission-intersection.util.ts index 655f074f44..e93cff1d4f 100644 --- a/packages/twenty-server/src/engine/twenty-orm/utils/compute-permission-intersection.util.ts +++ b/packages/twenty-server/src/engine/twenty-orm/utils/compute-permission-intersection.util.ts @@ -1,8 +1,30 @@ import { type ObjectsPermissions, type RestrictedFieldPermissions, + type RowLevelPermissionPredicate, } from 'twenty-shared/types'; +// An intersection has no combined predicate tree to expose, since each role +// compiles separately. Keeping a field constrained by one role alone would +// let that role's rule cancel another role's deny in the insert guard. +const intersectRowLevelPermissionPredicates = ( + rowLevelPermissionPredicatesPerRole: RowLevelPermissionPredicate[][], +): RowLevelPermissionPredicate[] => { + const [firstRolePredicates = [], ...otherRolesPredicates] = + rowLevelPermissionPredicatesPerRole; + + const constrainedFieldMetadataIdsPerOtherRole = otherRolesPredicates.map( + (predicates) => + new Set(predicates.map((predicate) => predicate.fieldMetadataId)), + ); + + return firstRolePredicates.filter((predicate) => + constrainedFieldMetadataIdsPerOtherRole.every((fieldMetadataIds) => + fieldMetadataIds.has(predicate.fieldMetadataId), + ), + ); +}; + export const computePermissionIntersection = ( permissionsArray: ObjectsPermissions[], ): ObjectsPermissions => { @@ -30,6 +52,8 @@ export const computePermissionIntersection = ( let canSoftDeleteObjectRecords = true; let canDestroyObjectRecords = true; const restrictedFields: Record = {}; + const rowLevelPermissionPredicatesPerRole: RowLevelPermissionPredicate[][] = + []; for (const permissions of permissionsArray) { const objPerm = permissions[objectMetadataId]; @@ -39,6 +63,7 @@ export const computePermissionIntersection = ( canUpdateObjectRecords = false; canSoftDeleteObjectRecords = false; canDestroyObjectRecords = false; + rowLevelPermissionPredicatesPerRole.push([]); continue; } @@ -52,6 +77,10 @@ export const computePermissionIntersection = ( canDestroyObjectRecords = canDestroyObjectRecords && objPerm.canDestroyObjectRecords === true; + rowLevelPermissionPredicatesPerRole.push( + objPerm.rowLevelPermissionPredicates, + ); + if (objPerm.restrictedFields) { for (const [fieldName, fieldPerm] of Object.entries( objPerm.restrictedFields, @@ -85,7 +114,9 @@ export const computePermissionIntersection = ( canSoftDeleteObjectRecords, canDestroyObjectRecords, restrictedFields, - rowLevelPermissionPredicates: [], + rowLevelPermissionPredicates: intersectRowLevelPermissionPredicates( + rowLevelPermissionPredicatesPerRole, + ), rowLevelPermissionPredicateGroups: [], }; } diff --git a/packages/twenty-server/src/engine/twenty-orm/utils/get-objects-permissions-from-role-permission-config.util.ts b/packages/twenty-server/src/engine/twenty-orm/utils/get-objects-permissions-from-role-permission-config.util.ts index f2fb20537d..c2b9dfc569 100644 --- a/packages/twenty-server/src/engine/twenty-orm/utils/get-objects-permissions-from-role-permission-config.util.ts +++ b/packages/twenty-server/src/engine/twenty-orm/utils/get-objects-permissions-from-role-permission-config.util.ts @@ -2,11 +2,11 @@ import { type ObjectsPermissions, type ObjectsPermissionsByRoleId, } from 'twenty-shared/types'; -import { isDefined } from 'twenty-shared/utils'; +import { isDefined, isNonEmptyArray } from 'twenty-shared/utils'; import { type RolePermissionConfig } from 'src/engine/twenty-orm/types/role-permission-config'; +import { computePermissionIntersection } from 'src/engine/twenty-orm/utils/compute-permission-intersection.util'; -// Multi-role union/intersection is not ready — use the first assigned role only. export const getObjectsPermissionsFromRolePermissionConfig = ({ rolesPermissions, rolePermissionConfig, @@ -18,16 +18,28 @@ export const getObjectsPermissionsFromRolePermissionConfig = ({ return {}; } - const roleId = - 'intersectionOf' in rolePermissionConfig - ? rolePermissionConfig.intersectionOf[0] - : 'unionOf' in rolePermissionConfig - ? rolePermissionConfig.unionOf[0] - : undefined; + if ('intersectionOf' in rolePermissionConfig) { + const permissionsPerRole = rolePermissionConfig.intersectionOf + .map((roleId) => rolesPermissions[roleId]) + .filter(isDefined); - if (!isDefined(roleId)) { - return {}; + if ( + !isNonEmptyArray(permissionsPerRole) || + permissionsPerRole.length !== rolePermissionConfig.intersectionOf.length + ) { + return {}; + } + + return computePermissionIntersection(permissionsPerRole); } - return rolesPermissions[roleId] ?? {}; + // Multi-role union is unimplemented and every producer emits one role, so + // taking the first is exact rather than lossy. + if ('unionOf' in rolePermissionConfig) { + const roleId = rolePermissionConfig.unionOf[0]; + + return isDefined(roleId) ? (rolesPermissions[roleId] ?? {}) : {}; + } + + return {}; }; diff --git a/packages/twenty-server/src/engine/twenty-orm/utils/resolve-role-ids-for-user.util.ts b/packages/twenty-server/src/engine/twenty-orm/utils/resolve-role-ids-for-user.util.ts new file mode 100644 index 0000000000..b2925b9f6b --- /dev/null +++ b/packages/twenty-server/src/engine/twenty-orm/utils/resolve-role-ids-for-user.util.ts @@ -0,0 +1,20 @@ +import { isDefined } from 'twenty-shared/utils'; + +// An application acting for a user stays within that person's role and within +// the role it declared, so permissions are the intersection of both. +export const resolveRoleIdsForUser = ({ + userRoleId, + applicationRoleId, +}: { + userRoleId: string | null | undefined; + applicationRoleId: string | null | undefined; +}): string[] => { + // The application's role must never stand in for a missing user role. + if (!isDefined(userRoleId)) { + return []; + } + + return isDefined(applicationRoleId) && applicationRoleId !== userRoleId + ? [userRoleId, applicationRoleId] + : [userRoleId]; +}; diff --git a/packages/twenty-server/src/engine/twenty-orm/utils/resolve-role-id-from-auth-context.util.ts b/packages/twenty-server/src/engine/twenty-orm/utils/resolve-role-ids-from-auth-context.util.ts similarity index 56% rename from packages/twenty-server/src/engine/twenty-orm/utils/resolve-role-id-from-auth-context.util.ts rename to packages/twenty-server/src/engine/twenty-orm/utils/resolve-role-ids-from-auth-context.util.ts index e95f4bf10a..4853ffe1e2 100644 --- a/packages/twenty-server/src/engine/twenty-orm/utils/resolve-role-id-from-auth-context.util.ts +++ b/packages/twenty-server/src/engine/twenty-orm/utils/resolve-role-ids-from-auth-context.util.ts @@ -5,8 +5,9 @@ import { isApplicationAuthContext } from 'src/engine/core-modules/auth/guards/is import { isUserAuthContext } from 'src/engine/core-modules/auth/guards/is-user-auth-context.guard'; import { type WorkspaceAuthContext } from 'src/engine/core-modules/auth/types/workspace-auth-context.type'; import { type UserWorkspaceRoleMap } from 'src/engine/metadata-modules/role-target/types/user-workspace-role-map'; +import { resolveRoleIdsForUser } from 'src/engine/twenty-orm/utils/resolve-role-ids-for-user.util'; -export const resolveRoleIdFromAuthContext = ({ +export const resolveRoleIdsFromAuthContext = ({ authContext, userWorkspaceRoleMap, apiKeyRoleMap, @@ -14,21 +15,25 @@ export const resolveRoleIdFromAuthContext = ({ authContext: WorkspaceAuthContext; userWorkspaceRoleMap: UserWorkspaceRoleMap; apiKeyRoleMap: Record; -}): string | undefined => { +}): string[] => { if (isUserAuthContext(authContext)) { - return userWorkspaceRoleMap[authContext.userWorkspaceId]; + return resolveRoleIdsForUser({ + userRoleId: userWorkspaceRoleMap[authContext.userWorkspaceId], + applicationRoleId: authContext.application?.defaultRoleId, + }); } if (isApiKeyAuthContext(authContext)) { - return apiKeyRoleMap[authContext.apiKey.id]; + const apiKeyRoleId = apiKeyRoleMap[authContext.apiKey.id]; + + return isDefined(apiKeyRoleId) ? [apiKeyRoleId] : []; } - if ( - isApplicationAuthContext(authContext) && - isDefined(authContext.application.defaultRoleId) - ) { - return authContext.application.defaultRoleId; + if (isApplicationAuthContext(authContext)) { + const applicationRoleId = authContext.application.defaultRoleId; + + return isDefined(applicationRoleId) ? [applicationRoleId] : []; } - return undefined; + return []; }; diff --git a/packages/twenty-server/src/engine/twenty-orm/utils/resolve-role-permission-config.util.ts b/packages/twenty-server/src/engine/twenty-orm/utils/resolve-role-permission-config.util.ts index 5e1139ee83..0e0e1f7f37 100644 --- a/packages/twenty-server/src/engine/twenty-orm/utils/resolve-role-permission-config.util.ts +++ b/packages/twenty-server/src/engine/twenty-orm/utils/resolve-role-permission-config.util.ts @@ -1,10 +1,10 @@ -import { isDefined } from 'twenty-shared/utils'; +import { isNonEmptyArray } from 'twenty-shared/utils'; import { isSystemAuthContext } from 'src/engine/core-modules/auth/guards/is-system-auth-context.guard'; import { type WorkspaceAuthContext } from 'src/engine/core-modules/auth/types/workspace-auth-context.type'; import { type UserWorkspaceRoleMap } from 'src/engine/metadata-modules/role-target/types/user-workspace-role-map'; import { type RolePermissionConfig } from 'src/engine/twenty-orm/types/role-permission-config'; -import { resolveRoleIdFromAuthContext } from 'src/engine/twenty-orm/utils/resolve-role-id-from-auth-context.util'; +import { resolveRoleIdsFromAuthContext } from 'src/engine/twenty-orm/utils/resolve-role-ids-from-auth-context.util'; export const resolveRolePermissionConfig = ({ authContext, @@ -19,15 +19,15 @@ export const resolveRolePermissionConfig = ({ return { shouldBypassPermissionChecks: true }; } - const roleId = resolveRoleIdFromAuthContext({ + const roleIds = resolveRoleIdsFromAuthContext({ authContext, userWorkspaceRoleMap, apiKeyRoleMap, }); - if (!isDefined(roleId)) { + if (!isNonEmptyArray(roleIds)) { return null; } - return { intersectionOf: [roleId] }; + return { intersectionOf: roleIds }; }; diff --git a/packages/twenty-server/src/engine/twenty-orm/utils/validate-rls-predicates-for-records.util.ts b/packages/twenty-server/src/engine/twenty-orm/utils/validate-rls-predicates-for-records.util.ts index 57b8699728..a3a4ae5482 100644 --- a/packages/twenty-server/src/engine/twenty-orm/utils/validate-rls-predicates-for-records.util.ts +++ b/packages/twenty-server/src/engine/twenty-orm/utils/validate-rls-predicates-for-records.util.ts @@ -1,6 +1,7 @@ /* @license Enterprise */ import { type ObjectRecord } from 'twenty-shared/types'; +import { isNonEmptyArray } from 'twenty-shared/utils'; import { type ObjectLiteral } from 'typeorm'; import { type WorkspaceInternalContext } from 'src/engine/twenty-orm/interfaces/workspace-internal-context.interface'; @@ -14,7 +15,7 @@ import { } from 'src/engine/twenty-orm/exceptions/twenty-orm.exception'; import { buildRowLevelPermissionRecordFilter } from 'src/engine/twenty-orm/utils/build-row-level-permission-record-filter.util'; import { isRecordMatchingRLSRowLevelPermissionPredicate } from 'src/engine/twenty-orm/utils/is-record-matching-rls-row-level-permission-predicate.util'; -import { resolveRoleIdFromAuthContext } from 'src/engine/twenty-orm/utils/resolve-role-id-from-auth-context.util'; +import { resolveRoleIdsFromAuthContext } from 'src/engine/twenty-orm/utils/resolve-role-ids-from-auth-context.util'; type ValidateRLSPredicatesForRecordsArgs = { records: T[]; @@ -37,13 +38,13 @@ export const validateRLSPredicatesForRecords = ({ return; } - const roleId = resolveRoleIdFromAuthContext({ + const roleIds = resolveRoleIdsFromAuthContext({ authContext, userWorkspaceRoleMap: internalContext.userWorkspaceRoleMap, apiKeyRoleMap: internalContext.apiKeyRoleMap, }); - if (!roleId) { + if (!isNonEmptyArray(roleIds)) { return; } @@ -54,7 +55,7 @@ export const validateRLSPredicatesForRecords = ({ internalContext.flatRowLevelPermissionPredicateGroupMaps, flatFieldMetadataMaps: internalContext.flatFieldMetadataMaps, objectMetadata, - roleId, + roleIds, workspaceMember: isUserAuthContext(authContext) ? authContext.workspaceMember : undefined, diff --git a/packages/twenty-server/test/integration/graphql/suites/application-user-role-intersection.integration-spec.ts b/packages/twenty-server/test/integration/graphql/suites/application-user-role-intersection.integration-spec.ts new file mode 100644 index 0000000000..300b4ad93e --- /dev/null +++ b/packages/twenty-server/test/integration/graphql/suites/application-user-role-intersection.integration-spec.ts @@ -0,0 +1,237 @@ +import { randomUUID } from 'crypto'; + +import { COMPANY_GQL_FIELDS } from 'test/integration/constants/company-gql-fields.constants'; +import { createOneOperationFactory } from 'test/integration/graphql/utils/create-one-operation-factory.util'; +import { destroyOneOperationFactory } from 'test/integration/graphql/utils/destroy-one-operation-factory.util'; +import { findManyOperationFactory } from 'test/integration/graphql/utils/find-many-operation-factory.util'; +import { makeGraphqlAPIRequest } from 'test/integration/graphql/utils/make-graphql-api-request.util'; +import { updateOneOperationFactory } from 'test/integration/graphql/utils/update-one-operation-factory.util'; +import { buildBaseManifest } from 'test/integration/metadata/suites/application/utils/build-base-manifest.util'; +import { cleanupApplicationAndAppRegistration } from 'test/integration/metadata/suites/application/utils/cleanup-application-and-app-registration.util'; +import { generateApplicationToken } from 'test/integration/metadata/suites/application/utils/generate-application-token.util'; +import { setupApplicationForSync } from 'test/integration/metadata/suites/application/utils/setup-application-for-sync.util'; +import { syncApplication } from 'test/integration/metadata/suites/application/utils/sync-application.util'; +import { createOneRole } from 'test/integration/metadata/suites/role/utils/create-one-role.util'; +import { type Manifest } from 'twenty-shared/application'; +import { STANDARD_OBJECTS } from 'twenty-shared/metadata'; +import { RowLevelPermissionPredicateOperand } from 'twenty-shared/types'; + +import { SEED_APPLE_WORKSPACE_ID } from 'src/engine/workspace-manager/dev-seeder/core/constants/seeder-workspaces.constant'; + +const TEST_APP_UNIVERSAL_IDENTIFIER = randomUUID(); +const TEST_ROLE_UNIVERSAL_IDENTIFIER = randomUUID(); +const TEST_OBJECT_PERMISSION_UNIVERSAL_IDENTIFIER = randomUUID(); +const TEST_PREDICATE_UNIVERSAL_IDENTIFIER = randomUUID(); + +const VISIBLE_COMPANY_ID = randomUUID(); +const HIDDEN_COMPANY_ID = randomUUID(); +const VISIBLE_COMPANY_NAME = `Intersection Visible ${VISIBLE_COMPANY_ID}`; +const HIDDEN_COMPANY_NAME = `Intersection Hidden ${HIDDEN_COMPANY_ID}`; + +const COMPANY_UNIVERSAL_IDENTIFIER = + STANDARD_OBJECTS.company.universalIdentifier; +const COMPANY_NAME_FIELD_UNIVERSAL_IDENTIFIER = + STANDARD_OBJECTS.company.fields.name.universalIdentifier; + +// The application declares a role that is strictly narrower than the admin who +// holds the token: it may read but not update, and only sees companies whose +// name contains "Intersection Visible". The admin has neither bound, so every +// assertion below fails if the application's role is dropped. +const buildApplicationManifest = (): Manifest => + buildBaseManifest({ + appId: TEST_APP_UNIVERSAL_IDENTIFIER, + roleId: TEST_ROLE_UNIVERSAL_IDENTIFIER, + overrides: { + roles: [ + { + universalIdentifier: TEST_ROLE_UNIVERSAL_IDENTIFIER, + label: 'Intersection Test Role', + description: 'Role narrower than the user acting through the app', + canUpdateAllSettings: false, + canReadAllObjectRecords: true, + canUpdateAllObjectRecords: false, + objectPermissions: [ + { + universalIdentifier: TEST_OBJECT_PERMISSION_UNIVERSAL_IDENTIFIER, + objectUniversalIdentifier: COMPANY_UNIVERSAL_IDENTIFIER, + canReadObjectRecords: true, + canUpdateObjectRecords: false, + }, + ], + rowLevelPermissionPredicates: [ + { + universalIdentifier: TEST_PREDICATE_UNIVERSAL_IDENTIFIER, + objectUniversalIdentifier: COMPANY_UNIVERSAL_IDENTIFIER, + fieldUniversalIdentifier: COMPANY_NAME_FIELD_UNIVERSAL_IDENTIFIER, + operand: RowLevelPermissionPredicateOperand.CONTAINS, + value: 'Intersection Visible', + }, + ], + }, + ], + }, + }); + +const findApplicationId = async (): Promise => { + const rows = await globalThis.testDataSource.query( + `SELECT id FROM core."application" + WHERE "universalIdentifier" = $1 AND "workspaceId" = $2`, + [TEST_APP_UNIVERSAL_IDENTIFIER, SEED_APPLE_WORKSPACE_ID], + ); + + return rows[0]?.id; +}; + +const findApplicationDefaultRoleId = async (): Promise => { + const rows = await globalThis.testDataSource.query( + `SELECT "defaultRoleId" FROM core."application" + WHERE "universalIdentifier" = $1 AND "workspaceId" = $2`, + [TEST_APP_UNIVERSAL_IDENTIFIER, SEED_APPLE_WORKSPACE_ID], + ); + + return rows[0]?.defaultRoleId ?? null; +}; + +const createCompany = async (id: string, name: string) => + makeGraphqlAPIRequest( + createOneOperationFactory({ + objectMetadataSingularName: 'company', + gqlFields: COMPANY_GQL_FIELDS, + data: { id, name }, + }), + ); + +const destroyCompany = async (id: string) => + makeGraphqlAPIRequest( + destroyOneOperationFactory({ + objectMetadataSingularName: 'company', + gqlFields: 'id', + recordId: id, + }), + ); + +const findCompanyNames = async (token?: string): Promise => { + const response = await makeGraphqlAPIRequest( + findManyOperationFactory({ + objectMetadataSingularName: 'company', + objectMetadataPluralName: 'companies', + gqlFields: 'id name', + filter: { id: { in: [VISIBLE_COMPANY_ID, HIDDEN_COMPANY_ID] } }, + }), + token, + ); + + expect(response.body.errors).toBeUndefined(); + + return response.body.data.companies.edges.map( + (edge: { node: { name: string } }) => edge.node.name, + ); +}; + +describe('An application acting for a user is bound by both roles', () => { + let applicationAccessToken: string; + + beforeAll(async () => { + await setupApplicationForSync({ + applicationUniversalIdentifier: TEST_APP_UNIVERSAL_IDENTIFIER, + name: 'Role Intersection Test Application', + description: 'App for testing application and user role intersection', + sourcePath: 'test-role-intersection', + }); + + // setupApplicationForSync leaves fake timers installed. + jest.useRealTimers(); + + const { errors } = await syncApplication({ + manifest: buildApplicationManifest(), + expectToFail: false, + }); + + expect(errors).toBeUndefined(); + + const applicationId = await findApplicationId(); + + expect(applicationId).toBeTruthy(); + expect(await findApplicationDefaultRoleId()).toBeTruthy(); + + await createCompany(VISIBLE_COMPANY_ID, VISIBLE_COMPANY_NAME); + await createCompany(HIDDEN_COMPANY_ID, HIDDEN_COMPANY_NAME); + + // Minted with the admin token, so it carries that admin's userId and + // userWorkspaceId alongside the applicationId. + const { data } = await generateApplicationToken({ + applicationId, + expectToFail: false, + }); + + applicationAccessToken = + data.generateApplicationToken.applicationAccessToken.token; + }, 120000); + + afterAll(async () => { + await destroyCompany(VISIBLE_COMPANY_ID); + await destroyCompany(HIDDEN_COMPANY_ID); + + await cleanupApplicationAndAppRegistration({ + applicationUniversalIdentifier: TEST_APP_UNIVERSAL_IDENTIFIER, + }); + }, 120000); + + it('should let the user see both companies when acting on their own', async () => { + const names = await findCompanyNames(); + + expect(names).toHaveLength(2); + expect(names).toEqual( + expect.arrayContaining([VISIBLE_COMPANY_NAME, HIDDEN_COMPANY_NAME]), + ); + }); + + it('should apply the application row-level predicate even though the user has none', async () => { + const names = await findCompanyNames(applicationAccessToken); + + expect(names).toEqual([VISIBLE_COMPANY_NAME]); + }); + + it('should refuse an update the application role forbids and the user role allows', async () => { + const response = await makeGraphqlAPIRequest( + updateOneOperationFactory({ + objectMetadataSingularName: 'company', + gqlFields: COMPANY_GQL_FIELDS, + recordId: VISIBLE_COMPANY_ID, + data: { name: `${VISIBLE_COMPANY_NAME} edited` }, + }), + applicationAccessToken, + ); + + expect(response.body.errors).toBeDefined(); + expect(response.body.data?.updateCompany).toBeFalsy(); + }); + + it('should refuse a settings mutation the application role forbids', async () => { + const { errors } = await createOneRole({ + expectToFail: true, + token: applicationAccessToken, + input: { + label: `Should Never Exist ${randomUUID()}`, + description: 'Created through an application whose role forbids it', + icon: 'IconLock', + }, + }); + + expect(errors).toBeDefined(); + }); + + it('should let the same update through when the user acts on their own', async () => { + const response = await makeGraphqlAPIRequest( + updateOneOperationFactory({ + objectMetadataSingularName: 'company', + gqlFields: COMPANY_GQL_FIELDS, + recordId: VISIBLE_COMPANY_ID, + data: { name: VISIBLE_COMPANY_NAME }, + }), + ); + + expect(response.body.errors).toBeUndefined(); + expect(response.body.data.updateCompany.id).toBe(VISIBLE_COMPANY_ID); + }); +});