From 2524d64687e92a2ae07b06a3e4f828777f2668cf Mon Sep 17 00:00:00 2001 From: Antoine Moreaux Date: Sat, 7 Dec 2024 16:48:53 +0100 Subject: [PATCH] Feat/improve error management in core module (#8933) ## Summary This Pull Request introduces a custom validator for checking forbidden words in workspaces and refines how exceptions are handled within the workspace module. - Introduced `ForbiddenWords` custom class validator for validating forbidden words against specific fields in `UpdateWorkspaceInput`. - Added `EnvironmentService` usage in `WorkspaceService` to check default subdomains. - New file `workspaceGraphqlApiExceptionHandler` to handle GraphQL API exceptions with specific error mappings. - Expanded `WorkspaceExceptionCode` with `SUBDOMAIN_ALREADY_TAKEN`. - Added new unit tests for validating forbidden words and exception handler behavior. --- .../workspace/dtos/update-workspace-input.ts | 3 + .../workspace/services/workspace.service.ts | 13 +++- ...orkspaceGraphqlApiExceptionHandler.spec.ts | 63 +++++++++++++++++++ .../workspaceGraphqlApiExceptionHandler.ts | 25 ++++++++ .../workspace/workspace.exception.ts | 1 + .../workspace/workspace.resolver.ts | 13 ++-- ...idden-words-custom-class-validator.spec.ts | 38 +++++++++++ .../custom-class-validator/ForbiddenWords.ts | 39 ++++++++++++ 8 files changed, 188 insertions(+), 7 deletions(-) create mode 100644 packages/twenty-server/src/engine/core-modules/workspace/utils/workspaceGraphqlApiExceptionHandler.spec.ts create mode 100644 packages/twenty-server/src/engine/core-modules/workspace/utils/workspaceGraphqlApiExceptionHandler.ts create mode 100644 packages/twenty-server/src/engine/utils/__tests__/forbidden-words-custom-class-validator.spec.ts create mode 100644 packages/twenty-server/src/engine/utils/custom-class-validator/ForbiddenWords.ts diff --git a/packages/twenty-server/src/engine/core-modules/workspace/dtos/update-workspace-input.ts b/packages/twenty-server/src/engine/core-modules/workspace/dtos/update-workspace-input.ts index e86a58202..19d86e3a3 100644 --- a/packages/twenty-server/src/engine/core-modules/workspace/dtos/update-workspace-input.ts +++ b/packages/twenty-server/src/engine/core-modules/workspace/dtos/update-workspace-input.ts @@ -2,6 +2,8 @@ import { Field, InputType } from '@nestjs/graphql'; import { IsBoolean, IsOptional, IsString, Matches } from 'class-validator'; +import { ForbiddenWords } from 'src/engine/utils/custom-class-validator/ForbiddenWords'; + @InputType() export class UpdateWorkspaceInput { @Field({ nullable: true }) @@ -13,6 +15,7 @@ export class UpdateWorkspaceInput { @IsString() @IsOptional() @Matches(/^[a-z0-9][a-z0-9-]{1,28}[a-z0-9]$/) + @ForbiddenWords(['demo']) subdomain?: string; @Field({ nullable: true }) 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 f801f7bfb..20fe027db 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 @@ -23,7 +23,7 @@ import { WorkspaceExceptionCode, } from 'src/engine/core-modules/workspace/workspace.exception'; import { workspaceValidator } from 'src/engine/core-modules/workspace/workspace.validate'; -import { ConflictError } from 'src/engine/core-modules/graphql/utils/graphql-errors.util'; +import { EnvironmentService } from 'src/engine/core-modules/environment/environment.service'; @Injectable() // eslint-disable-next-line @nx/workspace-inject-workspace-repository @@ -39,6 +39,7 @@ export class WorkspaceService extends TypeOrmQueryService { private readonly featureFlagService: FeatureFlagService, private readonly billingSubscriptionService: BillingSubscriptionService, private readonly userWorkspaceService: UserWorkspaceService, + private readonly environmentService: EnvironmentService, ) { super(workspaceRepository); } @@ -61,8 +62,14 @@ export class WorkspaceService extends TypeOrmQueryService { payload.subdomain, ); - if (!subdomainAvailable) { - throw new ConflictError('Subdomain already taken'); + if ( + !subdomainAvailable || + this.environmentService.get('DEFAULT_SUBDOMAIN') === payload.subdomain + ) { + throw new WorkspaceException( + 'Subdomain already taken', + WorkspaceExceptionCode.SUBDOMAIN_ALREADY_TAKEN, + ); } } diff --git a/packages/twenty-server/src/engine/core-modules/workspace/utils/workspaceGraphqlApiExceptionHandler.spec.ts b/packages/twenty-server/src/engine/core-modules/workspace/utils/workspaceGraphqlApiExceptionHandler.spec.ts new file mode 100644 index 000000000..351f5ab87 --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/workspace/utils/workspaceGraphqlApiExceptionHandler.spec.ts @@ -0,0 +1,63 @@ +import { + ConflictError, + InternalServerError, + NotFoundError, +} from 'src/engine/core-modules/graphql/utils/graphql-errors.util'; +import { + WorkspaceException, + WorkspaceExceptionCode, +} from 'src/engine/core-modules/workspace/workspace.exception'; + +import { workspaceGraphqlApiExceptionHandler } from './workspaceGraphqlApiExceptionHandler'; + +describe('workspaceGraphqlApiExceptionHandler', () => { + it('should throw NotFoundError when WorkspaceExceptionCode is SUBDOMAIN_NOT_FOUND', () => { + const error = new WorkspaceException( + 'Subdomain not found', + WorkspaceExceptionCode.SUBDOMAIN_NOT_FOUND, + ); + + expect(() => workspaceGraphqlApiExceptionHandler(error)).toThrow( + NotFoundError, + ); + }); + + it('should throw NotFoundError when WorkspaceExceptionCode is WORKSPACE_NOT_FOUND', () => { + const error = new WorkspaceException( + 'Workspace not found', + WorkspaceExceptionCode.WORKSPACE_NOT_FOUND, + ); + + expect(() => workspaceGraphqlApiExceptionHandler(error)).toThrow( + NotFoundError, + ); + }); + + it('should throw ConflictError when WorkspaceExceptionCode is SUBDOMAIN_ALREADY_TAKEN', () => { + const error = new WorkspaceException( + 'Subdomain already taken', + WorkspaceExceptionCode.SUBDOMAIN_ALREADY_TAKEN, + ); + + expect(() => workspaceGraphqlApiExceptionHandler(error)).toThrow( + ConflictError, + ); + }); + + it('should throw InternalServerError for unknown WorkspaceExceptionCode', () => { + // @ts-expect-error - should never happen but it allow to test the default case + const error = new WorkspaceException('Unknown error', 'UNKNOWN_CODE'); + + expect(() => workspaceGraphqlApiExceptionHandler(error)).toThrow( + InternalServerError, + ); + }); + + it('should throw the original error if it is not a WorkspaceException', () => { + const genericError = new Error('Generic error'); + + expect(() => workspaceGraphqlApiExceptionHandler(genericError)).toThrow( + genericError, + ); + }); +}); diff --git a/packages/twenty-server/src/engine/core-modules/workspace/utils/workspaceGraphqlApiExceptionHandler.ts b/packages/twenty-server/src/engine/core-modules/workspace/utils/workspaceGraphqlApiExceptionHandler.ts new file mode 100644 index 000000000..f3d38d859 --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/workspace/utils/workspaceGraphqlApiExceptionHandler.ts @@ -0,0 +1,25 @@ +import { + ConflictError, + InternalServerError, + NotFoundError, +} from 'src/engine/core-modules/graphql/utils/graphql-errors.util'; +import { + WorkspaceException, + WorkspaceExceptionCode, +} from 'src/engine/core-modules/workspace/workspace.exception'; + +export const workspaceGraphqlApiExceptionHandler = (error: Error) => { + if (error instanceof WorkspaceException) { + switch (error.code) { + case WorkspaceExceptionCode.SUBDOMAIN_NOT_FOUND: + case WorkspaceExceptionCode.WORKSPACE_NOT_FOUND: + throw new NotFoundError(error.message); + case WorkspaceExceptionCode.SUBDOMAIN_ALREADY_TAKEN: + throw new ConflictError(error.message); + default: + throw new InternalServerError(error.message); + } + } + + throw error; +}; diff --git a/packages/twenty-server/src/engine/core-modules/workspace/workspace.exception.ts b/packages/twenty-server/src/engine/core-modules/workspace/workspace.exception.ts index cd9d2f980..00d05dfa3 100644 --- a/packages/twenty-server/src/engine/core-modules/workspace/workspace.exception.ts +++ b/packages/twenty-server/src/engine/core-modules/workspace/workspace.exception.ts @@ -8,5 +8,6 @@ export class WorkspaceException extends CustomException { export enum WorkspaceExceptionCode { SUBDOMAIN_NOT_FOUND = 'SUBDOMAIN_NOT_FOUND', + SUBDOMAIN_ALREADY_TAKEN = 'SUBDOMAIN_ALREADY_TAKEN', WORKSPACE_NOT_FOUND = 'WORKSPACE_NOT_FOUND', } 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 10f79331a..b45336e36 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 @@ -40,6 +40,7 @@ import { workspaceValidator } from 'src/engine/core-modules/workspace/workspace. import { LoginTokenService } from 'src/engine/core-modules/auth/token/services/login-token.service'; import { DomainManagerService } from 'src/engine/core-modules/domain-manager/service/domain-manager.service'; import { getAuthProvidersByWorkspace } from 'src/engine/core-modules/workspace/utils/getAuthProvidersByWorkspace'; +import { workspaceGraphqlApiExceptionHandler } from 'src/engine/core-modules/workspace/utils/workspaceGraphqlApiExceptionHandler'; import { Workspace } from './workspace.entity'; @@ -91,10 +92,14 @@ export class WorkspaceResolver { @Args('data') data: UpdateWorkspaceInput, @AuthWorkspace() workspace: Workspace, ) { - return this.workspaceService.updateWorkspaceById({ - ...data, - id: workspace.id, - }); + try { + return this.workspaceService.updateWorkspaceById({ + ...data, + id: workspace.id, + }); + } catch (error) { + workspaceGraphqlApiExceptionHandler(error); + } } @Mutation(() => String) diff --git a/packages/twenty-server/src/engine/utils/__tests__/forbidden-words-custom-class-validator.spec.ts b/packages/twenty-server/src/engine/utils/__tests__/forbidden-words-custom-class-validator.spec.ts new file mode 100644 index 000000000..b5e83a10f --- /dev/null +++ b/packages/twenty-server/src/engine/utils/__tests__/forbidden-words-custom-class-validator.spec.ts @@ -0,0 +1,38 @@ +import { validate } from 'class-validator'; + +import { ForbiddenWords } from 'src/engine/utils/custom-class-validator/ForbiddenWords'; + +describe('ForbiddenWordsConstraint', () => { + test('should throw error when word is forbidden', async () => { + class Test { + @ForbiddenWords(['forbidden', 'restricted']) + subdomain: string; + } + + const instance = new Test(); + + instance.subdomain = 'forbidden'; + + const errors = await validate(instance); + + expect(errors.length).toBeGreaterThan(0); + expect(errors[0].constraints).toEqual({ + ForbiddenWordsConstraint: 'forbidden, restricted are not allowed', + }); + }); + + test('should pass validation word is not in the list', async () => { + class Test { + @ForbiddenWords(['forbidden', 'restricted']) + subdomain: string; + } + + const instance = new Test(); + + instance.subdomain = 'valid'; + + const errors = await validate(instance); + + expect(errors.length).toEqual(0); + }); +}); diff --git a/packages/twenty-server/src/engine/utils/custom-class-validator/ForbiddenWords.ts b/packages/twenty-server/src/engine/utils/custom-class-validator/ForbiddenWords.ts new file mode 100644 index 000000000..1e3412d9b --- /dev/null +++ b/packages/twenty-server/src/engine/utils/custom-class-validator/ForbiddenWords.ts @@ -0,0 +1,39 @@ +import { + registerDecorator, + ValidationArguments, + ValidationOptions, + ValidatorConstraint, + ValidatorConstraintInterface, +} from 'class-validator'; + +@ValidatorConstraint({ async: false }) +export class ForbiddenWordsConstraint implements ValidatorConstraintInterface { + private forbiddenWords: Set; + + constructor() {} + + validate(value: string, validationArguments: ValidationArguments) { + this.forbiddenWords = new Set(validationArguments.constraints[0]); + + return !this.forbiddenWords.has(value); + } + + defaultMessage() { + return `${Array.from(this.forbiddenWords).join(', ')} are not allowed`; + } +} + +export function ForbiddenWords( + forbiddenWords: string[], + validationOptions?: ValidationOptions, +) { + return function (object: object, propertyName: string) { + registerDecorator({ + target: object.constructor, + propertyName: propertyName, + options: validationOptions, + constraints: [forbiddenWords], + validator: ForbiddenWordsConstraint, + }); + }; +}