From 75bf9e3c69e1e1687416997ea23bc25ee70ad6fa Mon Sep 17 00:00:00 2001 From: Antoine Moreaux Date: Fri, 10 Jan 2025 12:30:42 +0100 Subject: [PATCH] fix(admin-panel): resolve feature flag key mismatch (#9530) Update feature flag handling by mapping input keys to enum values. This ensures compatibility and prevents potential runtime errors when updating workspace feature flags. --- .../admin-panel/admin-panel.resolver.ts | 3 +- .../admin-panel/admin-panel.service.ts | 20 +- .../admin-panel/admin-panel.spec.ts | 214 ++++++++++++++++++ .../feature-flag/feature-flag.entity.ts | 2 +- .../validates/feature-flag.validate.spec.ts | 24 ++ .../validates/feature-flag.validate.ts | 17 ++ .../engine/guards/impersonate-guard.spec.ts | 54 +++++ 7 files changed, 324 insertions(+), 10 deletions(-) create mode 100644 packages/twenty-server/src/engine/core-modules/admin-panel/admin-panel.spec.ts create mode 100644 packages/twenty-server/src/engine/core-modules/feature-flag/validates/feature-flag.validate.spec.ts create mode 100644 packages/twenty-server/src/engine/core-modules/feature-flag/validates/feature-flag.validate.ts create mode 100644 packages/twenty-server/src/engine/guards/impersonate-guard.spec.ts diff --git a/packages/twenty-server/src/engine/core-modules/admin-panel/admin-panel.resolver.ts b/packages/twenty-server/src/engine/core-modules/admin-panel/admin-panel.resolver.ts index ea18ea80b..16ac678aa 100644 --- a/packages/twenty-server/src/engine/core-modules/admin-panel/admin-panel.resolver.ts +++ b/packages/twenty-server/src/engine/core-modules/admin-panel/admin-panel.resolver.ts @@ -11,6 +11,7 @@ import { UserAuthGuard } from 'src/engine/guards/user-auth.guard'; import { WorkspaceAuthGuard } from 'src/engine/guards/workspace-auth.guard'; import { ImpersonateGuard } from 'src/engine/guards/impersonate-guard'; import { ImpersonateOutput } from 'src/engine/core-modules/admin-panel/dtos/impersonate.output'; +import { FeatureFlagKey } from 'src/engine/core-modules/feature-flag/enums/feature-flag-key.enum'; @Resolver() @UseFilters(AuthGraphqlApiExceptionFilter) @@ -40,7 +41,7 @@ export class AdminPanelResolver { ): Promise { await this.adminService.updateWorkspaceFeatureFlags( updateFlagInput.workspaceId, - updateFlagInput.featureFlag, + FeatureFlagKey[updateFlagInput.featureFlag], updateFlagInput.value, ); diff --git a/packages/twenty-server/src/engine/core-modules/admin-panel/admin-panel.service.ts b/packages/twenty-server/src/engine/core-modules/admin-panel/admin-panel.service.ts index a50708e72..0d34a6462 100644 --- a/packages/twenty-server/src/engine/core-modules/admin-panel/admin-panel.service.ts +++ b/packages/twenty-server/src/engine/core-modules/admin-panel/admin-panel.service.ts @@ -15,6 +15,7 @@ import { Workspace } from 'src/engine/core-modules/workspace/workspace.entity'; import { userValidator } from 'src/engine/core-modules/user/user.validate'; import { workspaceValidator } from 'src/engine/core-modules/workspace/workspace.validate'; import { LoginTokenService } from 'src/engine/core-modules/auth/token/services/login-token.service'; +import { featureFlagValidator } from 'src/engine/core-modules/feature-flag/validates/feature-flag.validate'; @Injectable() export class AdminPanelService { @@ -44,14 +45,9 @@ export class AdminPanelService { userValidator.assertIsDefinedOrThrow( user, - new AuthException('User not found', AuthExceptionCode.INVALID_INPUT), - ); - - workspaceValidator.assertIsDefinedOrThrow( - user.workspaces[0].workspace, new AuthException( - 'Impersonation not allowed', - AuthExceptionCode.FORBIDDEN_EXCEPTION, + 'User not found or impersonation not enable on workspace', + AuthExceptionCode.INVALID_INPUT, ), ); @@ -122,9 +118,17 @@ export class AdminPanelService { async updateWorkspaceFeatureFlags( workspaceId: string, - featureFlag: FeatureFlagKey, + featureFlag: string, value: boolean, ) { + featureFlagValidator.assertIsFeatureFlagKey( + featureFlag, + new AuthException( + 'Invalid feature flag key', + AuthExceptionCode.INVALID_INPUT, + ), + ); + const workspace = await this.workspaceRepository.findOne({ where: { id: workspaceId }, relations: ['featureFlags'], diff --git a/packages/twenty-server/src/engine/core-modules/admin-panel/admin-panel.spec.ts b/packages/twenty-server/src/engine/core-modules/admin-panel/admin-panel.spec.ts new file mode 100644 index 000000000..1868576df --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/admin-panel/admin-panel.spec.ts @@ -0,0 +1,214 @@ +import { Test, TestingModule } from '@nestjs/testing'; +import { getRepositoryToken } from '@nestjs/typeorm'; + +import { expect, jest } from '@jest/globals'; + +import { Workspace } from 'src/engine/core-modules/workspace/workspace.entity'; +import { User } from 'src/engine/core-modules/user/user.entity'; +import { AdminPanelService } from 'src/engine/core-modules/admin-panel/admin-panel.service'; +import { FeatureFlagEntity } from 'src/engine/core-modules/feature-flag/feature-flag.entity'; +import { LoginTokenService } from 'src/engine/core-modules/auth/token/services/login-token.service'; +import { + AuthException, + AuthExceptionCode, +} from 'src/engine/core-modules/auth/auth.exception'; + +const UserFindOneMock = jest.fn(); +const WorkspaceFindOneMock = jest.fn(); +const FeatureFlagUpdateMock = jest.fn(); +const FeatureFlagSaveMock = jest.fn(); +const LoginTokenServiceGenerateLoginTokenMock = jest.fn(); + +jest.mock( + 'src/engine/core-modules/feature-flag/enums/feature-flag-key.enum', + () => { + return { + FeatureFlagKey: { + IsFlagEnabled: 'IS_FLAG_ENABLED', + }, + }; + }, +); + +describe('AdminPanelService', () => { + let service: AdminPanelService; + + beforeEach(async () => { + const module: TestingModule = await Test.createTestingModule({ + providers: [ + AdminPanelService, + { + provide: getRepositoryToken(Workspace, 'core'), + useValue: { + findOne: WorkspaceFindOneMock, + }, + }, + { + provide: getRepositoryToken(User, 'core'), + useValue: { + findOne: UserFindOneMock, + }, + }, + { + provide: getRepositoryToken(FeatureFlagEntity, 'core'), + useValue: { + update: FeatureFlagUpdateMock, + save: FeatureFlagSaveMock, + }, + }, + { + provide: LoginTokenService, + useValue: { + generateLoginToken: LoginTokenServiceGenerateLoginTokenMock, + }, + }, + ], + }).compile(); + + service = module.get(AdminPanelService); + }); + + it('should be defined', async () => { + expect(service).toBeDefined(); + }); + + it('should update an existing feature flag if it exists', async () => { + const workspaceId = 'workspace-id'; + const featureFlag = 'IsFlagEnabled'; + const value = true; + const existingFlag = { id: 'flag-id', key: featureFlag, value: false }; + + WorkspaceFindOneMock.mockReturnValueOnce({ + id: workspaceId, + featureFlags: [existingFlag], + }); + + await service.updateWorkspaceFeatureFlags(workspaceId, featureFlag, value); + + expect(FeatureFlagUpdateMock).toHaveBeenCalledWith(existingFlag.id, { + value, + }); + expect(FeatureFlagSaveMock).not.toHaveBeenCalled(); + }); + + it('should create a new feature flag if it does not exist', async () => { + const workspaceId = 'workspace-id'; + const featureFlag = 'IsFlagEnabled'; + const value = true; + + WorkspaceFindOneMock.mockReturnValueOnce({ + id: workspaceId, + featureFlags: [], + }); + + await service.updateWorkspaceFeatureFlags(workspaceId, featureFlag, value); + + expect(FeatureFlagSaveMock).toHaveBeenCalledWith({ + key: featureFlag, + value, + workspaceId, + }); + expect(FeatureFlagUpdateMock).not.toHaveBeenCalled(); + }); + + it('should throw an exception if the workspace is not found', async () => { + const workspaceId = 'non-existent-workspace'; + const featureFlag = 'IsFlagEnabled'; + const value = true; + + WorkspaceFindOneMock.mockReturnValueOnce(null); + + await expect( + service.updateWorkspaceFeatureFlags(workspaceId, featureFlag, value), + ).rejects.toThrowError( + new AuthException('Workspace not found', AuthExceptionCode.INVALID_INPUT), + ); + }); + + it('should throw an exception if the flag is not found', async () => { + const workspaceId = 'non-existent-workspace'; + const featureFlag = 'IsUnknownFlagEnabled'; + const value = true; + + WorkspaceFindOneMock.mockReturnValueOnce(null); + + await expect( + service.updateWorkspaceFeatureFlags(workspaceId, featureFlag, value), + ).rejects.toThrowError( + new AuthException( + 'Invalid feature flag key', + AuthExceptionCode.INVALID_INPUT, + ), + ); + }); + + it('should impersonate a user and return workspace and loginToken on success', async () => { + const mockUser = { + id: 'user-id', + email: 'user@example.com', + workspaces: [ + { + workspace: { + id: 'workspace-id', + allowImpersonation: true, + subdomain: 'example-subdomain', + }, + }, + ], + }; + + UserFindOneMock.mockReturnValueOnce(mockUser); + LoginTokenServiceGenerateLoginTokenMock.mockReturnValueOnce({ + token: 'mock-login-token', + expiresAt: new Date(), + }); + + const result = await service.impersonate('user-id', 'workspace-id'); + + expect(UserFindOneMock).toHaveBeenCalledWith( + expect.objectContaining({ + where: expect.objectContaining({ + id: 'user-id', + workspaces: { + workspaceId: 'workspace-id', + workspace: { allowImpersonation: true }, + }, + }), + relations: ['workspaces', 'workspaces.workspace'], + }), + ); + + expect(LoginTokenServiceGenerateLoginTokenMock).toHaveBeenCalledWith( + 'user@example.com', + 'workspace-id', + ); + + expect(result).toEqual( + expect.objectContaining({ + workspace: { + id: 'workspace-id', + subdomain: 'example-subdomain', + }, + loginToken: expect.objectContaining({ + token: 'mock-login-token', + expiresAt: expect.any(Date), + }), + }), + ); + }); + + it('should throw an error when user is not found', async () => { + UserFindOneMock.mockReturnValueOnce(null); + + await expect( + service.impersonate('invalid-user-id', 'workspace-id'), + ).rejects.toThrow( + new AuthException( + 'User not found or impersonation not enable on workspace', + AuthExceptionCode.INVALID_INPUT, + ), + ); + + expect(UserFindOneMock).toHaveBeenCalled(); + }); +}); diff --git a/packages/twenty-server/src/engine/core-modules/feature-flag/feature-flag.entity.ts b/packages/twenty-server/src/engine/core-modules/feature-flag/feature-flag.entity.ts index 218dc047f..5fc9422e8 100644 --- a/packages/twenty-server/src/engine/core-modules/feature-flag/feature-flag.entity.ts +++ b/packages/twenty-server/src/engine/core-modules/feature-flag/feature-flag.entity.ts @@ -26,7 +26,7 @@ export class FeatureFlagEntity { @Field(() => FeatureFlagKey) @Column({ nullable: false, type: 'text' }) - key: FeatureFlagKey; + key: `${FeatureFlagKey}`; @Field() @Column({ nullable: false, type: 'uuid' }) diff --git a/packages/twenty-server/src/engine/core-modules/feature-flag/validates/feature-flag.validate.spec.ts b/packages/twenty-server/src/engine/core-modules/feature-flag/validates/feature-flag.validate.spec.ts new file mode 100644 index 000000000..19286d646 --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/feature-flag/validates/feature-flag.validate.spec.ts @@ -0,0 +1,24 @@ +import { CustomException } from 'src/utils/custom-exception'; +import { featureFlagValidator } from 'src/engine/core-modules/feature-flag/validates/feature-flag.validate'; + +describe('featureFlagValidator', () => { + describe('assertIsFeatureFlagKey', () => { + it('should not throw error if featureFlagKey is valid', () => { + expect(() => + featureFlagValidator.assertIsFeatureFlagKey( + 'IsWorkflowEnabled', + new CustomException('Error', 'Error'), + ), + ).not.toThrow(); + }); + + it('should throw error if featureFlagKey is invalid', () => { + const invalidKey = 'InvalidKey'; + const exception = new CustomException('Error', 'Error'); + + expect(() => + featureFlagValidator.assertIsFeatureFlagKey(invalidKey, exception), + ).toThrow(exception); + }); + }); +}); diff --git a/packages/twenty-server/src/engine/core-modules/feature-flag/validates/feature-flag.validate.ts b/packages/twenty-server/src/engine/core-modules/feature-flag/validates/feature-flag.validate.ts new file mode 100644 index 000000000..40245a9fc --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/feature-flag/validates/feature-flag.validate.ts @@ -0,0 +1,17 @@ +import { CustomException } from 'src/utils/custom-exception'; +import { FeatureFlagKey } from 'src/engine/core-modules/feature-flag/enums/feature-flag-key.enum'; +import { isDefined } from 'src/utils/is-defined'; + +const assertIsFeatureFlagKey = ( + featureFlagKey: string, + exceptionToThrow: CustomException, +): asserts featureFlagKey is FeatureFlagKey => { + if (isDefined(FeatureFlagKey[featureFlagKey])) return; + throw exceptionToThrow; +}; + +export const featureFlagValidator: { + assertIsFeatureFlagKey: typeof assertIsFeatureFlagKey; +} = { + assertIsFeatureFlagKey: assertIsFeatureFlagKey, +}; diff --git a/packages/twenty-server/src/engine/guards/impersonate-guard.spec.ts b/packages/twenty-server/src/engine/guards/impersonate-guard.spec.ts new file mode 100644 index 000000000..5135bd417 --- /dev/null +++ b/packages/twenty-server/src/engine/guards/impersonate-guard.spec.ts @@ -0,0 +1,54 @@ +import { ExecutionContext } from '@nestjs/common'; +import { GqlExecutionContext } from '@nestjs/graphql'; + +import { expect, jest } from '@jest/globals'; + +import { ImpersonateGuard } from 'src/engine/guards/impersonate-guard'; + +describe('ImpersonateGuard', () => { + const guard = new ImpersonateGuard(); + + it('should return true if user can impersonate', async () => { + const mockContext = { + getContext: jest.fn(() => ({ + req: { + user: { + canImpersonate: true, + }, + }, + })), + }; + + jest + .spyOn(GqlExecutionContext, 'create') + .mockReturnValue(mockContext as any); + + const mockExecutionContext = {} as ExecutionContext; + + const result = await guard.canActivate(mockExecutionContext); + + expect(result).toBe(true); + }); + + it('should return false if user cannot impersonate', async () => { + const mockContext = { + getContext: jest.fn(() => ({ + req: { + user: { + canImpersonate: false, + }, + }, + })), + }; + + jest + .spyOn(GqlExecutionContext, 'create') + .mockReturnValue(mockContext as any); + + const mockExecutionContext = {} as ExecutionContext; + + const result = await guard.canActivate(mockExecutionContext); + + expect(result).toBe(false); + }); +});