From a3a05c63f66b1b87658661afc1fb5a8c09e740dc Mon Sep 17 00:00:00 2001 From: Marie <51697796+ijreilly@users.noreply.github.com> Date: Fri, 28 Feb 2025 07:50:49 +0100 Subject: [PATCH] =?UTF-8?q?[permisions]=C2=A0Bypass=20permission=20checks?= =?UTF-8?q?=20with=20api=20key=20(#10516)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes https://github.com/twentyhq/core-team-issues/issues/325 --- .../interfaces/base-resolver-service.ts | 86 ++++++++----------- .../core-modules/billing/billing.resolver.ts | 7 ++ .../workspace/services/workspace.service.ts | 10 +++ .../workspace/workspace.resolver.ts | 3 + .../decorators/auth/auth-api-key.decorator.ts | 11 +++ .../guards/settings-permissions.guard.ts | 3 + .../permissions/permissions.service.ts | 36 +++++++- ...workspace-member-pre-query-hook.service.ts | 1 + 8 files changed, 103 insertions(+), 54 deletions(-) create mode 100644 packages/twenty-server/src/engine/decorators/auth/auth-api-key.decorator.ts diff --git a/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/interfaces/base-resolver-service.ts b/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/interfaces/base-resolver-service.ts index 87dcde5af..1da2f1b34 100644 --- a/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/interfaces/base-resolver-service.ts +++ b/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/interfaces/base-resolver-service.ts @@ -1,7 +1,11 @@ import { Inject, Injectable } from '@nestjs/common'; import graphqlFields from 'graphql-fields'; -import { capitalize, PermissionsOnAllObjectRecords } from 'twenty-shared'; +import { + capitalize, + isDefined, + PermissionsOnAllObjectRecords, +} from 'twenty-shared'; import { DataSource, ObjectLiteral } from 'typeorm'; import { ObjectRecord } from 'src/engine/api/graphql/workspace-query-builder/interfaces/object-record.interface'; @@ -24,10 +28,6 @@ import { QueryRunnerArgsFactory } from 'src/engine/api/graphql/workspace-query-r import { workspaceQueryRunnerGraphqlApiExceptionHandler } from 'src/engine/api/graphql/workspace-query-runner/utils/workspace-query-runner-graphql-api-exception-handler.util'; import { WorkspaceQueryHookService } from 'src/engine/api/graphql/workspace-query-runner/workspace-query-hook/workspace-query-hook.service'; import { RESOLVER_METHOD_NAMES } from 'src/engine/api/graphql/workspace-resolver-builder/constants/resolver-method-names'; -import { - AuthException, - AuthExceptionCode, -} from 'src/engine/core-modules/auth/auth.exception'; import { FeatureFlagKey } from 'src/engine/core-modules/feature-flag/enums/feature-flag-key.enum'; import { FeatureFlagService } from 'src/engine/core-modules/feature-flag/services/feature-flag.service'; import { SettingsPermissions } from 'src/engine/metadata-modules/permissions/constants/settings-permissions.constants'; @@ -193,32 +193,24 @@ export abstract class GraphqlQueryBaseResolverService< objectMetadataItemWithFieldMaps.nameSingular, ) ) { - if (!authContext.apiKey) { - if (!authContext.userWorkspaceId) { - throw new AuthException( - 'Missing userWorkspaceId in authContext', - AuthExceptionCode.USER_WORKSPACE_NOT_FOUND, - ); - } + const permissionRequired: SettingsPermissions = + SYSTEM_OBJECTS_PERMISSIONS_REQUIREMENTS[ + objectMetadataItemWithFieldMaps.nameSingular + ]; - const permissionRequired: SettingsPermissions = - SYSTEM_OBJECTS_PERMISSIONS_REQUIREMENTS[ - objectMetadataItemWithFieldMaps.nameSingular - ]; + const userHasPermission = + await this.permissionsService.userHasWorkspaceSettingPermission({ + userWorkspaceId: authContext.userWorkspaceId, + _setting: permissionRequired, + workspaceId: authContext.workspace.id, + isExecutedByApiKey: isDefined(authContext.apiKey), + }); - const userHasPermission = - await this.permissionsService.userHasWorkspaceSettingPermission({ - userWorkspaceId: authContext.userWorkspaceId, - _setting: permissionRequired, - workspaceId: authContext.workspace.id, - }); - - if (!userHasPermission) { - throw new PermissionsException( - PermissionsExceptionMessage.PERMISSION_DENIED, - PermissionsExceptionCode.PERMISSION_DENIED, - ); - } + if (!userHasPermission) { + throw new PermissionsException( + PermissionsExceptionMessage.PERMISSION_DENIED, + PermissionsExceptionCode.PERMISSION_DENIED, + ); } } } @@ -230,30 +222,22 @@ export abstract class GraphqlQueryBaseResolverService< operationName: WorkspaceResolverBuilderMethodNames; options: WorkspaceQueryRunnerOptions; }) { - if (!options.authContext.apiKey) { - if (!options.authContext.userWorkspaceId) { - throw new AuthException( - 'Missing userWorkspaceId in authContext', - AuthExceptionCode.USER_WORKSPACE_NOT_FOUND, - ); - } + const requiredPermission = + this.getRequiredPermissionForMethod(operationName); - const requiredPermission = - this.getRequiredPermissionForMethod(operationName); + const userHasPermission = + await this.permissionsService.userHasObjectRecordsPermission({ + userWorkspaceId: options.authContext.userWorkspaceId, + requiredPermission, + workspaceId: options.authContext.workspace.id, + isExecutedByApiKey: isDefined(options.authContext.apiKey), + }); - const userHasPermission = - await this.permissionsService.userHasObjectRecordsPermission({ - userWorkspaceId: options.authContext.userWorkspaceId, - requiredPermission, - workspaceId: options.authContext.workspace.id, - }); - - if (!userHasPermission) { - throw new PermissionsException( - PermissionsExceptionMessage.PERMISSION_DENIED, - PermissionsExceptionCode.PERMISSION_DENIED, - ); - } + if (!userHasPermission) { + throw new PermissionsException( + PermissionsExceptionMessage.PERMISSION_DENIED, + PermissionsExceptionCode.PERMISSION_DENIED, + ); } } diff --git a/packages/twenty-server/src/engine/core-modules/billing/billing.resolver.ts b/packages/twenty-server/src/engine/core-modules/billing/billing.resolver.ts index 4e9aaa57c..aaf3bc4a2 100644 --- a/packages/twenty-server/src/engine/core-modules/billing/billing.resolver.ts +++ b/packages/twenty-server/src/engine/core-modules/billing/billing.resolver.ts @@ -4,6 +4,7 @@ import { UseFilters, UseGuards } from '@nestjs/common'; import { Args, Mutation, Query, Resolver } from '@nestjs/graphql'; import { GraphQLError } from 'graphql'; +import { isDefined } from 'twenty-shared'; import { BillingCheckoutSessionInput } from 'src/engine/core-modules/billing/dtos/inputs/billing-checkout-session.input'; import { BillingProductInput } from 'src/engine/core-modules/billing/dtos/inputs/billing-product.input'; @@ -25,6 +26,7 @@ import { FeatureFlagKey } from 'src/engine/core-modules/feature-flag/enums/featu import { FeatureFlagService } from 'src/engine/core-modules/feature-flag/services/feature-flag.service'; import { User } from 'src/engine/core-modules/user/user.entity'; import { Workspace } from 'src/engine/core-modules/workspace/workspace.entity'; +import { AuthApiKey } from 'src/engine/decorators/auth/auth-api-key.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'; @@ -98,10 +100,12 @@ export class BillingResolver { plan, requirePaymentMethod, }: BillingCheckoutSessionInput, + @AuthApiKey() apiKey?: string, ) { await this.validateCanCheckoutSessionPermissionOrThrow({ workspaceId: workspace.id, userWorkspaceId, + isExecutedByApiKey: isDefined(apiKey), }); const isBillingPlansEnabled = await this.featureFlagService.isFeatureEnabled( @@ -177,9 +181,11 @@ export class BillingResolver { private async validateCanCheckoutSessionPermissionOrThrow({ workspaceId, userWorkspaceId, + isExecutedByApiKey, }: { workspaceId: string; userWorkspaceId: string; + isExecutedByApiKey: boolean; }) { const isPermissionsEnabled = await this.featureFlagService.isFeatureEnabled( FeatureFlagKey.IsPermissionsEnabled, @@ -203,6 +209,7 @@ export class BillingResolver { userWorkspaceId, workspaceId, _setting: SettingsPermissions.WORKSPACE, + isExecutedByApiKey, }); if (!userHasPermission) { diff --git a/packages/twenty-server/src/engine/core-modules/workspace/services/workspace.service.ts b/packages/twenty-server/src/engine/core-modules/workspace/services/workspace.service.ts index 81da9d10a..b403fdf4c 100644 --- a/packages/twenty-server/src/engine/core-modules/workspace/services/workspace.service.ts +++ b/packages/twenty-server/src/engine/core-modules/workspace/services/workspace.service.ts @@ -139,9 +139,11 @@ export class WorkspaceService extends TypeOrmQueryService { async updateWorkspaceById({ payload, userWorkspaceId, + apiKey, }: { payload: Partial & { id: string }; userWorkspaceId?: string; + apiKey?: string; }) { const workspace = await this.workspaceRepository.findOneBy({ id: payload.id, @@ -159,12 +161,14 @@ export class WorkspaceService extends TypeOrmQueryService { payload, userWorkspaceId, workspaceId: workspace.id, + apiKey, }); await this.validateWorkspacePermissions({ payload, userWorkspaceId, workspaceId: workspace.id, + apiKey, }); } @@ -395,10 +399,12 @@ export class WorkspaceService extends TypeOrmQueryService { payload, userWorkspaceId, workspaceId, + apiKey, }: { payload: Partial; userWorkspaceId?: string; workspaceId: string; + apiKey?: string; }) { if ( 'isGoogleAuthEnabled' in payload || @@ -415,6 +421,7 @@ export class WorkspaceService extends TypeOrmQueryService { userWorkspaceId, _setting: SettingsPermissions.SECURITY, workspaceId: workspaceId, + isExecutedByApiKey: isDefined(apiKey), }); if (!userHasPermission) { @@ -430,10 +437,12 @@ export class WorkspaceService extends TypeOrmQueryService { payload, userWorkspaceId, workspaceId, + apiKey, }: { payload: Partial; userWorkspaceId?: string; workspaceId: string; + apiKey?: string; }) { if ( 'displayName' in payload || @@ -450,6 +459,7 @@ export class WorkspaceService extends TypeOrmQueryService { userWorkspaceId, workspaceId, _setting: SettingsPermissions.WORKSPACE, + isExecutedByApiKey: isDefined(apiKey), }); if (!userHasPermission) { diff --git a/packages/twenty-server/src/engine/core-modules/workspace/workspace.resolver.ts b/packages/twenty-server/src/engine/core-modules/workspace/workspace.resolver.ts index f9b88216b..696db918d 100644 --- a/packages/twenty-server/src/engine/core-modules/workspace/workspace.resolver.ts +++ b/packages/twenty-server/src/engine/core-modules/workspace/workspace.resolver.ts @@ -39,6 +39,7 @@ import { workspaceUrls } from 'src/engine/core-modules/workspace/dtos/workspace- import { getAuthProvidersByWorkspace } from 'src/engine/core-modules/workspace/utils/get-auth-providers-by-workspace.util'; import { workspaceGraphqlApiExceptionHandler } from 'src/engine/core-modules/workspace/utils/workspace-graphql-api-exception-handler.util'; import { workspaceValidator } from 'src/engine/core-modules/workspace/workspace.validate'; +import { AuthApiKey } from 'src/engine/decorators/auth/auth-api-key.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'; @@ -110,6 +111,7 @@ export class WorkspaceResolver { @Args('data') data: UpdateWorkspaceInput, @AuthWorkspace() workspace: Workspace, @AuthUserWorkspaceId() userWorkspaceId: string, + @AuthApiKey() apiKey?: string, ) { try { return await this.workspaceService.updateWorkspaceById({ @@ -118,6 +120,7 @@ export class WorkspaceResolver { id: workspace.id, }, userWorkspaceId, + apiKey, }); } catch (error) { workspaceGraphqlApiExceptionHandler(error); diff --git a/packages/twenty-server/src/engine/decorators/auth/auth-api-key.decorator.ts b/packages/twenty-server/src/engine/decorators/auth/auth-api-key.decorator.ts new file mode 100644 index 000000000..37c83e537 --- /dev/null +++ b/packages/twenty-server/src/engine/decorators/auth/auth-api-key.decorator.ts @@ -0,0 +1,11 @@ +import { createParamDecorator, ExecutionContext } from '@nestjs/common'; + +import { getRequest } from 'src/utils/extract-request'; + +export const AuthApiKey = createParamDecorator( + (data: unknown, ctx: ExecutionContext) => { + const request = getRequest(ctx); + + return request.apiKey; + }, +); diff --git a/packages/twenty-server/src/engine/guards/settings-permissions.guard.ts b/packages/twenty-server/src/engine/guards/settings-permissions.guard.ts index e26a1ab05..7333c2601 100644 --- a/packages/twenty-server/src/engine/guards/settings-permissions.guard.ts +++ b/packages/twenty-server/src/engine/guards/settings-permissions.guard.ts @@ -7,6 +7,8 @@ import { } from '@nestjs/common'; import { GqlExecutionContext } from '@nestjs/graphql'; +import { isDefined } from 'twenty-shared'; + import { FeatureFlagKey } from 'src/engine/core-modules/feature-flag/enums/feature-flag-key.enum'; import { FeatureFlagService } from 'src/engine/core-modules/feature-flag/services/feature-flag.service'; import { SettingsPermissions } from 'src/engine/metadata-modules/permissions/constants/settings-permissions.constants'; @@ -47,6 +49,7 @@ export const SettingsPermissionsGuard = ( userWorkspaceId, _setting: requiredPermission, workspaceId, + isExecutedByApiKey: isDefined(ctx.getContext().req.apiKey), }); if (hasPermission === true) { 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 adf255225..31402dedc 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 @@ -1,7 +1,11 @@ import { Injectable } from '@nestjs/common'; -import { PermissionsOnAllObjectRecords } from 'twenty-shared'; +import { isDefined, PermissionsOnAllObjectRecords } from 'twenty-shared'; +import { + AuthException, + AuthExceptionCode, +} from 'src/engine/core-modules/auth/auth.exception'; import { EnvironmentService } from 'src/engine/core-modules/environment/environment.service'; import { SettingsPermissions } from 'src/engine/metadata-modules/permissions/constants/settings-permissions.constants'; import { @@ -74,11 +78,24 @@ export class PermissionsService { userWorkspaceId, workspaceId, _setting, + isExecutedByApiKey, }: { - userWorkspaceId: string; + userWorkspaceId?: string; workspaceId: string; _setting: SettingsPermissions; + isExecutedByApiKey: boolean; }): Promise { + if (isExecutedByApiKey) { + return true; + } + + if (!isDefined(userWorkspaceId)) { + throw new AuthException( + 'Missing userWorkspaceId or apiKey in authContext', + AuthExceptionCode.USER_WORKSPACE_NOT_FOUND, + ); + } + const [roleOfUserWorkspace] = await this.userRoleService .getRolesByUserWorkspaces({ userWorkspaceIds: [userWorkspaceId], @@ -97,11 +114,24 @@ export class PermissionsService { userWorkspaceId, workspaceId, requiredPermission, + isExecutedByApiKey, }: { - userWorkspaceId: string; + userWorkspaceId?: string; workspaceId: string; requiredPermission: PermissionsOnAllObjectRecords; + isExecutedByApiKey: boolean; }): Promise { + if (isExecutedByApiKey) { + return true; + } + + if (!isDefined(userWorkspaceId)) { + throw new AuthException( + 'Missing userWorkspaceId or apiKey in authContext', + AuthExceptionCode.USER_WORKSPACE_NOT_FOUND, + ); + } + const [roleOfUserWorkspace] = await this.userRoleService .getRolesByUserWorkspaces({ userWorkspaceIds: [userWorkspaceId], diff --git a/packages/twenty-server/src/modules/workspace-member/query-hooks/workspace-member-pre-query-hook.service.ts b/packages/twenty-server/src/modules/workspace-member/query-hooks/workspace-member-pre-query-hook.service.ts index b94934073..ef618b070 100644 --- a/packages/twenty-server/src/modules/workspace-member/query-hooks/workspace-member-pre-query-hook.service.ts +++ b/packages/twenty-server/src/modules/workspace-member/query-hooks/workspace-member-pre-query-hook.service.ts @@ -66,6 +66,7 @@ export class WorkspaceMemberPreQueryHookService { userWorkspaceId, workspaceId, _setting: SettingsPermissions.WORKSPACE_MEMBERS, + isExecutedByApiKey: isDefined(apiKey), }) ) { return;