From a27f960e8f40cfe2df1efabb3574cc61706b749a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Thu, 26 Dec 2024 10:47:07 +0100 Subject: [PATCH 1/4] feat(core): Port over AI controller request payloads to DTOs --- .../ai-apply-suggestion-request.dto.test.ts | 36 +++ .../ai/__tests__/ai-ask-request.dto.test.ts | 252 ++++++++++++++++++ .../ai/__tests__/ai-chat-request.dto.test.ts | 34 +++ .../dto/ai/ai-apply-suggestion-request.dto.ts | 7 + .../src/dto/ai/ai-ask-request.dto.ts | 53 ++++ .../src/dto/ai/ai-chat-request.dto.ts | 10 + packages/@n8n/api-types/src/dto/index.ts | 3 + .../config/src/configs/aiAssistant.config.ts | 8 + packages/@n8n/config/src/index.ts | 4 + packages/@n8n/config/test/config.test.ts | 3 + packages/cli/src/config/schema.ts | 9 - .../__tests__/ai.controller.test.ts | 111 ++++++++ packages/cli/src/controllers/ai.controller.ts | 27 +- packages/cli/src/requests.ts | 13 - .../src/services/__tests__/ai.service.test.ts | 132 +++++++++ packages/cli/src/services/ai.service.ts | 17 +- 16 files changed, 679 insertions(+), 40 deletions(-) create mode 100644 packages/@n8n/api-types/src/dto/ai/__tests__/ai-apply-suggestion-request.dto.test.ts create mode 100644 packages/@n8n/api-types/src/dto/ai/__tests__/ai-ask-request.dto.test.ts create mode 100644 packages/@n8n/api-types/src/dto/ai/__tests__/ai-chat-request.dto.test.ts create mode 100644 packages/@n8n/api-types/src/dto/ai/ai-apply-suggestion-request.dto.ts create mode 100644 packages/@n8n/api-types/src/dto/ai/ai-ask-request.dto.ts create mode 100644 packages/@n8n/api-types/src/dto/ai/ai-chat-request.dto.ts create mode 100644 packages/@n8n/config/src/configs/aiAssistant.config.ts create mode 100644 packages/cli/src/controllers/__tests__/ai.controller.test.ts create mode 100644 packages/cli/src/services/__tests__/ai.service.test.ts diff --git a/packages/@n8n/api-types/src/dto/ai/__tests__/ai-apply-suggestion-request.dto.test.ts b/packages/@n8n/api-types/src/dto/ai/__tests__/ai-apply-suggestion-request.dto.test.ts new file mode 100644 index 0000000000000..568900e409f7f --- /dev/null +++ b/packages/@n8n/api-types/src/dto/ai/__tests__/ai-apply-suggestion-request.dto.test.ts @@ -0,0 +1,36 @@ +import { AiApplySuggestionRequestDto } from '../ai-apply-suggestion-request.dto'; + +describe('AiApplySuggestionRequestDto', () => { + it('should validate a valid suggestion application request', () => { + const validRequest = { + sessionId: 'session-123', + suggestionId: 'suggestion-456', + }; + + const result = AiApplySuggestionRequestDto.safeParse(validRequest); + + expect(result.success).toBe(true); + }); + + it('should fail if sessionId is missing', () => { + const invalidRequest = { + suggestionId: 'suggestion-456', + }; + + const result = AiApplySuggestionRequestDto.safeParse(invalidRequest); + + expect(result.success).toBe(false); + expect(result.error?.issues[0].path).toEqual(['sessionId']); + }); + + it('should fail if suggestionId is missing', () => { + const invalidRequest = { + sessionId: 'session-123', + }; + + const result = AiApplySuggestionRequestDto.safeParse(invalidRequest); + + expect(result.success).toBe(false); + expect(result.error?.issues[0].path).toEqual(['suggestionId']); + }); +}); diff --git a/packages/@n8n/api-types/src/dto/ai/__tests__/ai-ask-request.dto.test.ts b/packages/@n8n/api-types/src/dto/ai/__tests__/ai-ask-request.dto.test.ts new file mode 100644 index 0000000000000..a87eb5f3a4c81 --- /dev/null +++ b/packages/@n8n/api-types/src/dto/ai/__tests__/ai-ask-request.dto.test.ts @@ -0,0 +1,252 @@ +import { AiAskRequestDto } from '../ai-ask-request.dto'; + +describe('AiAskRequestDto', () => { + const validRequest = { + question: 'How can I improve this workflow?', + context: { + schema: [ + { + nodeName: 'TestNode', + schema: { + type: 'string', + key: 'testKey', + value: 'testValue', + path: '/test/path', + }, + }, + ], + inputSchema: { + nodeName: 'InputNode', + schema: { + type: 'object', + key: 'inputKey', + value: [ + { + type: 'string', + key: 'nestedKey', + value: 'nestedValue', + path: '/nested/path', + }, + ], + path: '/input/path', + }, + }, + pushRef: 'push-123', + ndvPushRef: 'ndv-push-456', + }, + forNode: 'TestWorkflowNode', + }; + + it('should validate a valid AI ask request', () => { + const result = AiAskRequestDto.safeParse(validRequest); + + expect(result.success).toBe(true); + }); + + it('should fail if question is missing', () => { + const invalidRequest = { + ...validRequest, + question: undefined, + }; + + const result = AiAskRequestDto.safeParse(invalidRequest); + + expect(result.success).toBe(false); + expect(result.error?.issues[0].path).toEqual(['question']); + }); + + it('should fail if context is invalid', () => { + const invalidRequest = { + ...validRequest, + context: { + ...validRequest.context, + schema: [ + { + nodeName: 'TestNode', + schema: { + type: 'invalid-type', // Invalid type + value: 'testValue', + path: '/test/path', + }, + }, + ], + }, + }; + + const result = AiAskRequestDto.safeParse(invalidRequest); + + expect(result.success).toBe(false); + }); + + it('should fail if forNode is missing', () => { + const invalidRequest = { + ...validRequest, + forNode: undefined, + }; + + const result = AiAskRequestDto.safeParse(invalidRequest); + + expect(result.success).toBe(false); + expect(result.error?.issues[0].path).toEqual(['forNode']); + }); + + it('should validate all possible schema types', () => { + const allTypesRequest = { + question: 'Test all possible types', + context: { + schema: [ + { + nodeName: 'AllTypesNode', + schema: { + type: 'object', + key: 'typesRoot', + value: [ + { type: 'string', key: 'stringType', value: 'string', path: '/types/string' }, + { type: 'number', key: 'numberType', value: 'number', path: '/types/number' }, + { type: 'boolean', key: 'booleanType', value: 'boolean', path: '/types/boolean' }, + { type: 'bigint', key: 'bigintType', value: 'bigint', path: '/types/bigint' }, + { type: 'symbol', key: 'symbolType', value: 'symbol', path: '/types/symbol' }, + { type: 'array', key: 'arrayType', value: [], path: '/types/array' }, + { type: 'object', key: 'objectType', value: [], path: '/types/object' }, + { + type: 'function', + key: 'functionType', + value: 'function', + path: '/types/function', + }, + { type: 'null', key: 'nullType', value: 'null', path: '/types/null' }, + { + type: 'undefined', + key: 'undefinedType', + value: 'undefined', + path: '/types/undefined', + }, + ], + path: '/types/root', + }, + }, + ], + inputSchema: { + nodeName: 'InputNode', + schema: { + type: 'object', + key: 'simpleInput', + value: [ + { + type: 'string', + key: 'simpleKey', + value: 'simpleValue', + path: '/simple/path', + }, + ], + path: '/simple/input/path', + }, + }, + pushRef: 'push-types-123', + ndvPushRef: 'ndv-push-types-456', + }, + forNode: 'TypeCheckNode', + }; + + const result = AiAskRequestDto.safeParse(allTypesRequest); + expect(result.success).toBe(true); + }); + + it('should fail with invalid type', () => { + const invalidTypeRequest = { + question: 'Test invalid type', + context: { + schema: [ + { + nodeName: 'InvalidTypeNode', + schema: { + type: 'invalid-type', // This should fail + key: 'invalidKey', + value: 'invalidValue', + path: '/invalid/path', + }, + }, + ], + inputSchema: { + nodeName: 'InputNode', + schema: { + type: 'object', + key: 'simpleInput', + value: [ + { + type: 'string', + key: 'simpleKey', + value: 'simpleValue', + path: '/simple/path', + }, + ], + path: '/simple/input/path', + }, + }, + pushRef: 'push-invalid-123', + ndvPushRef: 'ndv-push-invalid-456', + }, + forNode: 'InvalidTypeNode', + }; + + const result = AiAskRequestDto.safeParse(invalidTypeRequest); + expect(result.success).toBe(false); + }); + + it('should validate multiple schema entries', () => { + const multiSchemaRequest = { + question: 'Multiple schema test', + context: { + schema: [ + { + nodeName: 'FirstNode', + schema: { + type: 'string', + key: 'firstKey', + value: 'firstValue', + path: '/first/path', + }, + }, + { + nodeName: 'SecondNode', + schema: { + type: 'object', + key: 'secondKey', + value: [ + { + type: 'number', + key: 'nestedKey', + value: 'nestedValue', + path: '/second/nested/path', + }, + ], + path: '/second/path', + }, + }, + ], + inputSchema: { + nodeName: 'InputNode', + schema: { + type: 'object', + key: 'simpleInput', + value: [ + { + type: 'string', + key: 'simpleKey', + value: 'simpleValue', + path: '/simple/path', + }, + ], + path: '/simple/input/path', + }, + }, + pushRef: 'push-multi-123', + ndvPushRef: 'ndv-push-multi-456', + }, + forNode: 'MultiSchemaNode', + }; + + const result = AiAskRequestDto.safeParse(multiSchemaRequest); + expect(result.success).toBe(true); + }); +}); diff --git a/packages/@n8n/api-types/src/dto/ai/__tests__/ai-chat-request.dto.test.ts b/packages/@n8n/api-types/src/dto/ai/__tests__/ai-chat-request.dto.test.ts new file mode 100644 index 0000000000000..ce1ccffac5f95 --- /dev/null +++ b/packages/@n8n/api-types/src/dto/ai/__tests__/ai-chat-request.dto.test.ts @@ -0,0 +1,34 @@ +import { AiChatRequestDto } from '../ai-chat-request.dto'; + +describe('AiChatRequestDto', () => { + it('should validate a request with a payload and session ID', () => { + const validRequest = { + payload: { someKey: 'someValue' }, + sessionId: 'session-123', + }; + + const result = AiChatRequestDto.safeParse(validRequest); + + expect(result.success).toBe(true); + }); + + it('should validate a request with only a payload', () => { + const validRequest = { + payload: { complexObject: { nested: 'value' } }, + }; + + const result = AiChatRequestDto.safeParse(validRequest); + + expect(result.success).toBe(true); + }); + + it('should fail if payload is missing', () => { + const invalidRequest = { + sessionId: 'session-123', + }; + + const result = AiChatRequestDto.safeParse(invalidRequest); + + expect(result.success).toBe(false); + }); +}); diff --git a/packages/@n8n/api-types/src/dto/ai/ai-apply-suggestion-request.dto.ts b/packages/@n8n/api-types/src/dto/ai/ai-apply-suggestion-request.dto.ts new file mode 100644 index 0000000000000..cc808dfd24a66 --- /dev/null +++ b/packages/@n8n/api-types/src/dto/ai/ai-apply-suggestion-request.dto.ts @@ -0,0 +1,7 @@ +import { z } from 'zod'; +import { Z } from 'zod-class'; + +export class AiApplySuggestionRequestDto extends Z.class({ + sessionId: z.string(), + suggestionId: z.string(), +}) {} diff --git a/packages/@n8n/api-types/src/dto/ai/ai-ask-request.dto.ts b/packages/@n8n/api-types/src/dto/ai/ai-ask-request.dto.ts new file mode 100644 index 0000000000000..9039243e051b2 --- /dev/null +++ b/packages/@n8n/api-types/src/dto/ai/ai-ask-request.dto.ts @@ -0,0 +1,53 @@ +import type { AiAssistantSDK, SchemaType } from '@n8n_io/ai-assistant-sdk'; +import { z } from 'zod'; +import { Z } from 'zod-class'; + +// Note: This is copied from the sdk, since this type is not exported +type Schema = { + type: SchemaType; + key?: string; + value: string | Schema[]; + path: string; +}; + +// Create a lazy validator to handle the recursive type +const schemaValidator: z.ZodType = z.lazy(() => + z.object({ + type: z.enum([ + 'string', + 'number', + 'boolean', + 'bigint', + 'symbol', + 'array', + 'object', + 'function', + 'null', + 'undefined', + ]), + key: z.string().optional(), + value: z.union([z.string(), z.lazy(() => schemaValidator.array())]), + path: z.string(), + }), +); + +export class AiAskRequestDto + extends Z.class({ + question: z.string(), + context: z.object({ + schema: z.array( + z.object({ + nodeName: z.string(), + schema: schemaValidator, + }), + ), + inputSchema: z.object({ + nodeName: z.string(), + schema: schemaValidator, + }), + pushRef: z.string(), + ndvPushRef: z.string(), + }), + forNode: z.string(), + }) + implements AiAssistantSDK.AskAiRequestPayload {} diff --git a/packages/@n8n/api-types/src/dto/ai/ai-chat-request.dto.ts b/packages/@n8n/api-types/src/dto/ai/ai-chat-request.dto.ts new file mode 100644 index 0000000000000..59e7a26aa3628 --- /dev/null +++ b/packages/@n8n/api-types/src/dto/ai/ai-chat-request.dto.ts @@ -0,0 +1,10 @@ +import type { AiAssistantSDK } from '@n8n_io/ai-assistant-sdk'; +import { z } from 'zod'; +import { Z } from 'zod-class'; + +export class AiChatRequestDto + extends Z.class({ + payload: z.object({}).passthrough(), // Allow any object shape + sessionId: z.string().optional(), + }) + implements AiAssistantSDK.ChatRequestPayload {} diff --git a/packages/@n8n/api-types/src/dto/index.ts b/packages/@n8n/api-types/src/dto/index.ts index 97d5d38459b4d..0e57e07110a54 100644 --- a/packages/@n8n/api-types/src/dto/index.ts +++ b/packages/@n8n/api-types/src/dto/index.ts @@ -1,3 +1,6 @@ +export { AiAskRequestDto } from './ai/ai-ask-request.dto'; +export { AiChatRequestDto } from './ai/ai-chat-request.dto'; +export { AiApplySuggestionRequestDto } from './ai/ai-apply-suggestion-request.dto'; export { PasswordUpdateRequestDto } from './user/password-update-request.dto'; export { RoleChangeRequestDto } from './user/role-change-request.dto'; export { SettingsUpdateRequestDto } from './user/settings-update-request.dto'; diff --git a/packages/@n8n/config/src/configs/aiAssistant.config.ts b/packages/@n8n/config/src/configs/aiAssistant.config.ts new file mode 100644 index 0000000000000..ff8a3986f2c4f --- /dev/null +++ b/packages/@n8n/config/src/configs/aiAssistant.config.ts @@ -0,0 +1,8 @@ +import { Config, Env } from '../decorators'; + +@Config +export class AiAssistantConfig { + /** Base URL of the AI assistant service */ + @Env('N8N_AI_ASSISTANT_BASE_URL') + baseUrl: string = ''; +} diff --git a/packages/@n8n/config/src/index.ts b/packages/@n8n/config/src/index.ts index a5144d419602f..945b5f123786d 100644 --- a/packages/@n8n/config/src/index.ts +++ b/packages/@n8n/config/src/index.ts @@ -1,3 +1,4 @@ +import { AiAssistantConfig } from './configs/aiAssistant.config'; import { CacheConfig } from './configs/cache.config'; import { CredentialsConfig } from './configs/credentials.config'; import { DatabaseConfig } from './configs/database.config'; @@ -121,4 +122,7 @@ export class GlobalConfig { @Nested diagnostics: DiagnosticsConfig; + + @Nested + aiAssistant: AiAssistantConfig; } diff --git a/packages/@n8n/config/test/config.test.ts b/packages/@n8n/config/test/config.test.ts index 771d915ee4970..d6d19c47fe2fc 100644 --- a/packages/@n8n/config/test/config.test.ts +++ b/packages/@n8n/config/test/config.test.ts @@ -289,6 +289,9 @@ describe('GlobalConfig', () => { apiHost: 'https://ph.n8n.io', }, }, + aiAssistant: { + baseUrl: '', + }, }; it('should use all default values when no env variables are defined', () => { diff --git a/packages/cli/src/config/schema.ts b/packages/cli/src/config/schema.ts index 54fa07e7f5dcb..e8d28cb782e7c 100644 --- a/packages/cli/src/config/schema.ts +++ b/packages/cli/src/config/schema.ts @@ -341,15 +341,6 @@ export const schema = { }, }, - aiAssistant: { - baseUrl: { - doc: 'Base URL of the AI assistant service', - format: String, - default: '', - env: 'N8N_AI_ASSISTANT_BASE_URL', - }, - }, - expression: { evaluator: { doc: 'Expression evaluator to use', diff --git a/packages/cli/src/controllers/__tests__/ai.controller.test.ts b/packages/cli/src/controllers/__tests__/ai.controller.test.ts new file mode 100644 index 0000000000000..30785ae938c07 --- /dev/null +++ b/packages/cli/src/controllers/__tests__/ai.controller.test.ts @@ -0,0 +1,111 @@ +import type { + AiAskRequestDto, + AiApplySuggestionRequestDto, + AiChatRequestDto, +} from '@n8n/api-types'; +import type { AiAssistantSDK } from '@n8n_io/ai-assistant-sdk'; +import { mock } from 'jest-mock-extended'; + +import { InternalServerError } from '@/errors/response-errors/internal-server.error'; +import type { AuthenticatedRequest } from '@/requests'; +import type { AiService } from '@/services/ai.service'; + +import { AiController, type FlushableResponse } from '../ai.controller'; + +describe('AiController', () => { + const aiService = mock(); + const controller = new AiController(aiService); + + const request = mock({ + user: { id: 'user123' }, + }); + const response = mock(); + + beforeEach(() => { + jest.clearAllMocks(); + + response.header.mockReturnThis(); + }); + + describe('chat', () => { + const payload = mock(); + + it('should handle chat request successfully', async () => { + aiService.chat.mockResolvedValue( + mock({ + body: mock({ + pipeTo: jest.fn().mockImplementation(async (writableStream) => { + // Simulate stream writing + const writer = writableStream.getWriter(); + await writer.write(JSON.stringify({ message: 'test response' })); + await writer.close(); + }), + }), + }), + ); + + await controller.chat(request, response, payload); + + expect(aiService.chat).toHaveBeenCalledWith(payload, request.user); + expect(response.header).toHaveBeenCalledWith('Content-type', 'application/json-lines'); + expect(response.flush).toHaveBeenCalled(); + expect(response.end).toHaveBeenCalled(); + }); + + it('should throw InternalServerError if chat fails', async () => { + const mockError = new Error('Chat failed'); + + aiService.chat.mockRejectedValue(mockError); + + await expect(controller.chat(request, response, payload)).rejects.toThrow( + InternalServerError, + ); + }); + }); + + describe('applySuggestion', () => { + const payload = mock(); + + it('should apply suggestion successfully', async () => { + const clientResponse = mock(); + aiService.applySuggestion.mockResolvedValue(clientResponse); + + const result = await controller.applySuggestion(request, response, payload); + + expect(aiService.applySuggestion).toHaveBeenCalledWith(payload, request.user); + expect(result).toEqual(clientResponse); + }); + + it('should throw InternalServerError if applying suggestion fails', async () => { + const mockError = new Error('Apply suggestion failed'); + aiService.applySuggestion.mockRejectedValue(mockError); + + await expect(controller.applySuggestion(request, response, payload)).rejects.toThrow( + InternalServerError, + ); + }); + }); + + describe('askAi method', () => { + const payload = mock(); + + it('should ask AI successfully', async () => { + const clientResponse = mock(); + aiService.askAi.mockResolvedValue(clientResponse); + + const result = await controller.askAi(request, response, payload); + + expect(aiService.askAi).toHaveBeenCalledWith(payload, request.user); + expect(result).toEqual(clientResponse); + }); + + it('should throw InternalServerError if asking AI fails', async () => { + const mockError = new Error('Ask AI failed'); + aiService.askAi.mockRejectedValue(mockError); + + await expect(controller.askAi(request, response, payload)).rejects.toThrow( + InternalServerError, + ); + }); + }); +}); diff --git a/packages/cli/src/controllers/ai.controller.ts b/packages/cli/src/controllers/ai.controller.ts index be1231911a001..59499112a3f9a 100644 --- a/packages/cli/src/controllers/ai.controller.ts +++ b/packages/cli/src/controllers/ai.controller.ts @@ -1,23 +1,24 @@ +import { AiChatRequestDto, AiApplySuggestionRequestDto, AiAskRequestDto } from '@n8n/api-types'; import type { AiAssistantSDK } from '@n8n_io/ai-assistant-sdk'; -import type { Response } from 'express'; +import { Response } from 'express'; import { strict as assert } from 'node:assert'; import { WritableStream } from 'node:stream/web'; -import { Post, RestController } from '@/decorators'; +import { Body, Post, RestController } from '@/decorators'; import { InternalServerError } from '@/errors/response-errors/internal-server.error'; -import { AiAssistantRequest } from '@/requests'; +import { AuthenticatedRequest } from '@/requests'; import { AiService } from '@/services/ai.service'; -type FlushableResponse = Response & { flush: () => void }; +export type FlushableResponse = Response & { flush: () => void }; @RestController('/ai') export class AiController { constructor(private readonly aiService: AiService) {} @Post('/chat', { rateLimit: { limit: 100 } }) - async chat(req: AiAssistantRequest.Chat, res: FlushableResponse) { + async chat(req: AuthenticatedRequest, res: FlushableResponse, @Body payload: AiChatRequestDto) { try { - const aiResponse = await this.aiService.chat(req.body, req.user); + const aiResponse = await this.aiService.chat(payload, req.user); if (aiResponse.body) { res.header('Content-type', 'application/json-lines').flush(); await aiResponse.body.pipeTo( @@ -38,10 +39,12 @@ export class AiController { @Post('/chat/apply-suggestion') async applySuggestion( - req: AiAssistantRequest.ApplySuggestionPayload, + req: AuthenticatedRequest, + _: Response, + @Body payload: AiApplySuggestionRequestDto, ): Promise { try { - return await this.aiService.applySuggestion(req.body, req.user); + return await this.aiService.applySuggestion(payload, req.user); } catch (e) { assert(e instanceof Error); throw new InternalServerError(e.message, e); @@ -49,9 +52,13 @@ export class AiController { } @Post('/ask-ai') - async askAi(req: AiAssistantRequest.AskAiPayload): Promise { + async askAi( + req: AuthenticatedRequest, + _: Response, + @Body payload: AiAskRequestDto, + ): Promise { try { - return await this.aiService.askAi(req.body, req.user); + return await this.aiService.askAi(payload, req.user); } catch (e) { assert(e instanceof Error); throw new InternalServerError(e.message, e); diff --git a/packages/cli/src/requests.ts b/packages/cli/src/requests.ts index 7afb1e1bd3c7b..f7ac415a7547f 100644 --- a/packages/cli/src/requests.ts +++ b/packages/cli/src/requests.ts @@ -1,5 +1,4 @@ import type { Scope } from '@n8n/permissions'; -import type { AiAssistantSDK } from '@n8n_io/ai-assistant-sdk'; import type express from 'express'; import type { BannerName, @@ -574,15 +573,3 @@ export declare namespace NpsSurveyRequest { // once some schema validation is added type NpsSurveyUpdate = AuthenticatedRequest<{}, {}, unknown>; } - -// ---------------------------------- -// /ai-assistant -// ---------------------------------- - -export declare namespace AiAssistantRequest { - type Chat = AuthenticatedRequest<{}, {}, AiAssistantSDK.ChatRequestPayload>; - - type SuggestionPayload = { sessionId: string; suggestionId: string }; - type ApplySuggestionPayload = AuthenticatedRequest<{}, {}, SuggestionPayload>; - type AskAiPayload = AuthenticatedRequest<{}, {}, AiAssistantSDK.AskAiRequestPayload>; -} diff --git a/packages/cli/src/services/__tests__/ai.service.test.ts b/packages/cli/src/services/__tests__/ai.service.test.ts new file mode 100644 index 0000000000000..dbdcaa3e718ff --- /dev/null +++ b/packages/cli/src/services/__tests__/ai.service.test.ts @@ -0,0 +1,132 @@ +import type { + AiAskRequestDto, + AiApplySuggestionRequestDto, + AiChatRequestDto, +} from '@n8n/api-types'; +import type { GlobalConfig } from '@n8n/config'; +import { AiAssistantClient, type AiAssistantSDK } from '@n8n_io/ai-assistant-sdk'; +import { mock } from 'jest-mock-extended'; +import type { IUser } from 'n8n-workflow'; + +import { N8N_VERSION } from '@/constants'; +import type { License } from '@/license'; + +import { AiService } from '../ai.service'; + +jest.mock('@n8n_io/ai-assistant-sdk', () => ({ + AiAssistantClient: jest.fn(), +})); + +describe('AiService', () => { + let aiService: AiService; + + const baseUrl = 'https://ai-assistant-url.com'; + const user = mock({ id: 'user123' }); + const client = mock(); + const license = mock(); + const globalConfig = mock({ + logging: { level: 'info' }, + aiAssistant: { baseUrl }, + }); + + beforeEach(() => { + jest.clearAllMocks(); + (AiAssistantClient as jest.Mock).mockImplementation(() => client); + aiService = new AiService(license, globalConfig); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + describe('init', () => { + it('should not initialize client if AI assistant is not enabled', async () => { + license.isAiAssistantEnabled.mockReturnValue(false); + + await aiService.init(); + + expect(AiAssistantClient).not.toHaveBeenCalled(); + }); + + it('should initialize client when AI assistant is enabled', async () => { + license.isAiAssistantEnabled.mockReturnValue(true); + license.loadCertStr.mockResolvedValue('mock-license-cert'); + license.getConsumerId.mockReturnValue('mock-consumer-id'); + + await aiService.init(); + + expect(AiAssistantClient).toHaveBeenCalledWith({ + licenseCert: 'mock-license-cert', + consumerId: 'mock-consumer-id', + n8nVersion: N8N_VERSION, + baseUrl, + logLevel: 'info', + }); + }); + }); + + describe('chat', () => { + const payload = mock(); + + it('should call client chat method after initialization', async () => { + license.isAiAssistantEnabled.mockReturnValue(true); + const clientResponse = mock(); + client.chat.mockResolvedValue(clientResponse); + + const result = await aiService.chat(payload, user); + + expect(client.chat).toHaveBeenCalledWith(payload, { id: user.id }); + expect(result).toEqual(clientResponse); + }); + + it('should throw error if client is not initialized', async () => { + license.isAiAssistantEnabled.mockReturnValue(false); + + await expect(aiService.chat(payload, user)).rejects.toThrow('Assistant client not setup'); + }); + }); + + describe('applySuggestion', () => { + const payload = mock(); + + it('should call client applySuggestion', async () => { + license.isAiAssistantEnabled.mockReturnValue(true); + const clientResponse = mock(); + client.applySuggestion.mockResolvedValue(clientResponse); + + const result = await aiService.applySuggestion(payload, user); + + expect(client.applySuggestion).toHaveBeenCalledWith(payload, { id: user.id }); + expect(result).toEqual(clientResponse); + }); + + it('should throw error if client is not initialized', async () => { + license.isAiAssistantEnabled.mockReturnValue(false); + + await expect(aiService.applySuggestion(payload, user)).rejects.toThrow( + 'Assistant client not setup', + ); + }); + }); + + describe('askAi', () => { + const payload = mock(); + + it('should call client askAi method after initialization', async () => { + license.isAiAssistantEnabled.mockReturnValue(true); + const clientResponse = mock(); + client.askAi.mockResolvedValue(clientResponse); + + const result = await aiService.askAi(payload, user); + + expect(client.askAi).toHaveBeenCalledWith(payload, { id: user.id }); + expect(result).toEqual(clientResponse); + }); + + it('should throw error if client is not initialized', async () => { + license.isAiAssistantEnabled.mockReturnValue(false); + + await expect(aiService.askAi(payload, user)).rejects.toThrow('Assistant client not setup'); + }); + }); +}); diff --git a/packages/cli/src/services/ai.service.ts b/packages/cli/src/services/ai.service.ts index a7b07219b5aca..74e28ad288841 100644 --- a/packages/cli/src/services/ai.service.ts +++ b/packages/cli/src/services/ai.service.ts @@ -1,12 +1,13 @@ +import type { + AiApplySuggestionRequestDto, + AiAskRequestDto, + AiChatRequestDto, +} from '@n8n/api-types'; import { GlobalConfig } from '@n8n/config'; -import type { AiAssistantSDK } from '@n8n_io/ai-assistant-sdk'; import { AiAssistantClient } from '@n8n_io/ai-assistant-sdk'; import { assert, type IUser } from 'n8n-workflow'; import { Service } from 'typedi'; -import config from '@/config'; -import type { AiAssistantRequest } from '@/requests'; - import { N8N_VERSION } from '../constants'; import { License } from '../license'; @@ -27,7 +28,7 @@ export class AiService { const licenseCert = await this.licenseService.loadCertStr(); const consumerId = this.licenseService.getConsumerId(); - const baseUrl = config.get('aiAssistant.baseUrl'); + const baseUrl = this.globalConfig.aiAssistant.baseUrl; const logLevel = this.globalConfig.logging.level; this.client = new AiAssistantClient({ @@ -39,7 +40,7 @@ export class AiService { }); } - async chat(payload: AiAssistantSDK.ChatRequestPayload, user: IUser) { + async chat(payload: AiChatRequestDto, user: IUser) { if (!this.client) { await this.init(); } @@ -48,7 +49,7 @@ export class AiService { return await this.client.chat(payload, { id: user.id }); } - async applySuggestion(payload: AiAssistantRequest.SuggestionPayload, user: IUser) { + async applySuggestion(payload: AiApplySuggestionRequestDto, user: IUser) { if (!this.client) { await this.init(); } @@ -57,7 +58,7 @@ export class AiService { return await this.client.applySuggestion(payload, { id: user.id }); } - async askAi(payload: AiAssistantSDK.AskAiRequestPayload, user: IUser) { + async askAi(payload: AiAskRequestDto, user: IUser) { if (!this.client) { await this.init(); } From 5f262b91442046989a7bde693b9da7c951e005c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Thu, 26 Dec 2024 13:08:59 +0100 Subject: [PATCH 2/4] refactor(core): Update Auth endpoints to use DTOs. add tests --- .../auth/__tests__/login-request.dto.test.ts | 93 +++++++++++++++++++ .../resolve-signup-token-query.dto.test.ts | 87 +++++++++++++++++ .../src/dto/auth/login-request.dto.ts | 9 ++ .../auth/resolve-signup-token-query.dto.ts | 7 ++ packages/@n8n/api-types/src/dto/index.ts | 2 + .../cli/src/controllers/auth.controller.ts | 43 +++------ .../cli/src/controllers/users.controller.ts | 12 ++- packages/cli/src/decorators/index.ts | 2 +- .../variables/variables.controller.ee.ts | 12 ++- packages/cli/src/requests.ts | 27 ------ .../cli/test/integration/auth.api.test.ts | 15 +++ 11 files changed, 249 insertions(+), 60 deletions(-) create mode 100644 packages/@n8n/api-types/src/dto/auth/__tests__/login-request.dto.test.ts create mode 100644 packages/@n8n/api-types/src/dto/auth/__tests__/resolve-signup-token-query.dto.test.ts create mode 100644 packages/@n8n/api-types/src/dto/auth/login-request.dto.ts create mode 100644 packages/@n8n/api-types/src/dto/auth/resolve-signup-token-query.dto.ts diff --git a/packages/@n8n/api-types/src/dto/auth/__tests__/login-request.dto.test.ts b/packages/@n8n/api-types/src/dto/auth/__tests__/login-request.dto.test.ts new file mode 100644 index 0000000000000..f222f1d93ecd9 --- /dev/null +++ b/packages/@n8n/api-types/src/dto/auth/__tests__/login-request.dto.test.ts @@ -0,0 +1,93 @@ +import { LoginRequestDto } from '../login-request.dto'; + +describe('LoginRequestDto', () => { + describe('Valid requests', () => { + test.each([ + { + name: 'complete valid login request', + request: { + email: 'test@example.com', + password: 'securePassword123', + mfaCode: '123456', + }, + }, + { + name: 'login request without optional MFA', + request: { + email: 'test@example.com', + password: 'securePassword123', + }, + }, + { + name: 'login request with both mfaCode and mfaRecoveryCode', + request: { + email: 'test@example.com', + password: 'securePassword123', + mfaCode: '123456', + mfaRecoveryCode: 'recovery-code-123', + }, + }, + { + name: 'login request with only mfaRecoveryCode', + request: { + email: 'test@example.com', + password: 'securePassword123', + mfaRecoveryCode: 'recovery-code-123', + }, + }, + ])('should validate $name', ({ request }) => { + const result = LoginRequestDto.safeParse(request); + expect(result.success).toBe(true); + }); + }); + + describe('Invalid requests', () => { + test.each([ + { + name: 'invalid email', + request: { + email: 'invalid-email', + password: 'securePassword123', + }, + expectedErrorPath: ['email'], + }, + { + name: 'empty password', + request: { + email: 'test@example.com', + password: '', + }, + expectedErrorPath: ['password'], + }, + { + name: 'missing email', + request: { + password: 'securePassword123', + }, + expectedErrorPath: ['email'], + }, + { + name: 'missing password', + request: { + email: 'test@example.com', + }, + expectedErrorPath: ['password'], + }, + { + name: 'whitespace in email and password', + request: { + email: ' test@example.com ', + password: ' securePassword123 ', + }, + expectedErrorPath: ['email'], + }, + ])('should fail validation for $name', ({ request, expectedErrorPath }) => { + const result = LoginRequestDto.safeParse(request); + expect(result.success).toBe(false); + + if (expectedErrorPath) { + expect(result.error?.issues[0].path).toEqual(expectedErrorPath); + } + }); + }); +}); diff --git a/packages/@n8n/api-types/src/dto/auth/__tests__/resolve-signup-token-query.dto.test.ts b/packages/@n8n/api-types/src/dto/auth/__tests__/resolve-signup-token-query.dto.test.ts new file mode 100644 index 0000000000000..218fe9107a43e --- /dev/null +++ b/packages/@n8n/api-types/src/dto/auth/__tests__/resolve-signup-token-query.dto.test.ts @@ -0,0 +1,87 @@ +import { ResolveSignupTokenQueryDto } from '../resolve-signup-token-query.dto'; + +describe('ResolveSignupTokenQueryDto', () => { + const validUuid = '123e4567-e89b-12d3-a456-426614174000'; + + describe('Valid requests', () => { + test.each([ + { + name: 'standard UUID', + request: { + inviterId: validUuid, + inviteeId: validUuid, + }, + }, + ])('should validate $name', ({ request }) => { + const result = ResolveSignupTokenQueryDto.safeParse(request); + expect(result.success).toBe(true); + }); + }); + + describe('Invalid requests', () => { + test.each([ + { + name: 'invalid inviterId UUID', + request: { + inviterId: 'not-a-valid-uuid', + inviteeId: validUuid, + }, + expectedErrorPath: ['inviterId'], + }, + { + name: 'invalid inviteeId UUID', + request: { + inviterId: validUuid, + inviteeId: 'not-a-valid-uuid', + }, + expectedErrorPath: ['inviteeId'], + }, + { + name: 'missing inviterId', + request: { + inviteeId: validUuid, + }, + expectedErrorPath: ['inviterId'], + }, + { + name: 'missing inviteeId', + request: { + inviterId: validUuid, + }, + expectedErrorPath: ['inviteeId'], + }, + { + name: 'UUID with invalid characters', + request: { + inviterId: '123e4567-e89b-12d3-a456-42661417400G', + inviteeId: validUuid, + }, + expectedErrorPath: ['inviterId'], + }, + { + name: 'UUID too long', + request: { + inviterId: '123e4567-e89b-12d3-a456-426614174001234', + inviteeId: validUuid, + }, + expectedErrorPath: ['inviterId'], + }, + { + name: 'UUID too short', + request: { + inviterId: '123e4567-e89b-12d3-a456', + inviteeId: validUuid, + }, + expectedErrorPath: ['inviterId'], + }, + ])('should fail validation for $name', ({ request, expectedErrorPath }) => { + const result = ResolveSignupTokenQueryDto.safeParse(request); + + expect(result.success).toBe(false); + + if (expectedErrorPath) { + expect(result.error?.issues[0].path).toEqual(expectedErrorPath); + } + }); + }); +}); diff --git a/packages/@n8n/api-types/src/dto/auth/login-request.dto.ts b/packages/@n8n/api-types/src/dto/auth/login-request.dto.ts new file mode 100644 index 0000000000000..894263992c6e0 --- /dev/null +++ b/packages/@n8n/api-types/src/dto/auth/login-request.dto.ts @@ -0,0 +1,9 @@ +import { z } from 'zod'; +import { Z } from 'zod-class'; + +export class LoginRequestDto extends Z.class({ + email: z.string().email(), + password: z.string().min(1), + mfaCode: z.string().optional(), + mfaRecoveryCode: z.string().optional(), +}) {} diff --git a/packages/@n8n/api-types/src/dto/auth/resolve-signup-token-query.dto.ts b/packages/@n8n/api-types/src/dto/auth/resolve-signup-token-query.dto.ts new file mode 100644 index 0000000000000..768202ff04ed9 --- /dev/null +++ b/packages/@n8n/api-types/src/dto/auth/resolve-signup-token-query.dto.ts @@ -0,0 +1,7 @@ +import { z } from 'zod'; +import { Z } from 'zod-class'; + +export class ResolveSignupTokenQueryDto extends Z.class({ + inviterId: z.string().uuid(), + inviteeId: z.string().uuid(), +}) {} diff --git a/packages/@n8n/api-types/src/dto/index.ts b/packages/@n8n/api-types/src/dto/index.ts index 0e57e07110a54..70b598015899b 100644 --- a/packages/@n8n/api-types/src/dto/index.ts +++ b/packages/@n8n/api-types/src/dto/index.ts @@ -1,6 +1,8 @@ export { AiAskRequestDto } from './ai/ai-ask-request.dto'; export { AiChatRequestDto } from './ai/ai-chat-request.dto'; export { AiApplySuggestionRequestDto } from './ai/ai-apply-suggestion-request.dto'; +export { LoginRequestDto } from './auth/login-request.dto'; +export { ResolveSignupTokenQueryDto } from './auth/resolve-signup-token-query.dto'; export { PasswordUpdateRequestDto } from './user/password-update-request.dto'; export { RoleChangeRequestDto } from './user/role-change-request.dto'; export { SettingsUpdateRequestDto } from './user/settings-update-request.dto'; diff --git a/packages/cli/src/controllers/auth.controller.ts b/packages/cli/src/controllers/auth.controller.ts index b012e571312ab..fb06c1a80b96c 100644 --- a/packages/cli/src/controllers/auth.controller.ts +++ b/packages/cli/src/controllers/auth.controller.ts @@ -1,14 +1,13 @@ +import { LoginRequestDto, ResolveSignupTokenQueryDto } from '@n8n/api-types'; import { Response } from 'express'; import { Logger } from 'n8n-core'; -import { ApplicationError } from 'n8n-workflow'; -import validator from 'validator'; import { handleEmailLogin, handleLdapLogin } from '@/auth'; import { AuthService } from '@/auth/auth.service'; import { RESPONSE_ERROR_MESSAGES } from '@/constants'; import type { User } from '@/databases/entities/user'; import { UserRepository } from '@/databases/repositories/user.repository'; -import { Get, Post, RestController } from '@/decorators'; +import { Body, Get, Post, Query, RestController } from '@/decorators'; import { AuthError } from '@/errors/response-errors/auth.error'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; @@ -17,7 +16,7 @@ import type { PublicUser } from '@/interfaces'; import { License } from '@/license'; import { MfaService } from '@/mfa/mfa.service'; import { PostHogClient } from '@/posthog'; -import { AuthenticatedRequest, LoginRequest, UserRequest } from '@/requests'; +import { AuthenticatedRequest, AuthlessRequest } from '@/requests'; import { UserService } from '@/services/user.service'; import { getCurrentAuthenticationMethod, @@ -40,10 +39,12 @@ export class AuthController { /** Log in a user */ @Post('/login', { skipAuth: true, rateLimit: true }) - async login(req: LoginRequest, res: Response): Promise { - const { email, password, mfaCode, mfaRecoveryCode } = req.body; - if (!email) throw new ApplicationError('Email is required to log in'); - if (!password) throw new ApplicationError('Password is required to log in'); + async login( + req: AuthlessRequest, + res: Response, + @Body payload: LoginRequestDto, + ): Promise { + const { email, password, mfaCode, mfaRecoveryCode } = payload; let user: User | undefined; @@ -117,8 +118,12 @@ export class AuthController { /** Validate invite token to enable invitee to set up their account */ @Get('/resolve-signup-token', { skipAuth: true }) - async resolveSignupToken(req: UserRequest.ResolveSignUp) { - const { inviterId, inviteeId } = req.query; + async resolveSignupToken( + _req: AuthlessRequest, + _res: Response, + @Query payload: ResolveSignupTokenQueryDto, + ) { + const { inviterId, inviteeId } = payload; const isWithinUsersLimit = this.license.isWithinUsersLimit(); if (!isWithinUsersLimit) { @@ -129,24 +134,6 @@ export class AuthController { throw new ForbiddenError(RESPONSE_ERROR_MESSAGES.USERS_QUOTA_REACHED); } - if (!inviterId || !inviteeId) { - this.logger.debug( - 'Request to resolve signup token failed because of missing user IDs in query string', - { inviterId, inviteeId }, - ); - throw new BadRequestError('Invalid payload'); - } - - // Postgres validates UUID format - for (const userId of [inviterId, inviteeId]) { - if (!validator.isUUID(userId)) { - this.logger.debug('Request to resolve signup token failed because of invalid user ID', { - userId, - }); - throw new BadRequestError('Invalid userId'); - } - } - const users = await this.userRepository.findManyByIds([inviterId, inviteeId]); if (users.length !== 2) { diff --git a/packages/cli/src/controllers/users.controller.ts b/packages/cli/src/controllers/users.controller.ts index e290d294638f2..3177c2c23be63 100644 --- a/packages/cli/src/controllers/users.controller.ts +++ b/packages/cli/src/controllers/users.controller.ts @@ -11,8 +11,16 @@ import { ProjectRepository } from '@/databases/repositories/project.repository'; import { SharedCredentialsRepository } from '@/databases/repositories/shared-credentials.repository'; import { SharedWorkflowRepository } from '@/databases/repositories/shared-workflow.repository'; import { UserRepository } from '@/databases/repositories/user.repository'; -import { GlobalScope, Delete, Get, RestController, Patch, Licensed, Body } from '@/decorators'; -import { Param } from '@/decorators/args'; +import { + GlobalScope, + Delete, + Get, + RestController, + Patch, + Licensed, + Body, + Param, +} from '@/decorators'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; import { NotFoundError } from '@/errors/response-errors/not-found.error'; diff --git a/packages/cli/src/decorators/index.ts b/packages/cli/src/decorators/index.ts index bd32add475d6e..8002bbe0940f9 100644 --- a/packages/cli/src/decorators/index.ts +++ b/packages/cli/src/decorators/index.ts @@ -1,4 +1,4 @@ -export { Body } from './args'; +export { Body, Query, Param } from './args'; export { RestController } from './rest-controller'; export { Get, Post, Put, Patch, Delete } from './route'; export { Middleware } from './middleware'; diff --git a/packages/cli/src/environments.ee/variables/variables.controller.ee.ts b/packages/cli/src/environments.ee/variables/variables.controller.ee.ts index a38906b800c26..460d5fa0093ed 100644 --- a/packages/cli/src/environments.ee/variables/variables.controller.ee.ts +++ b/packages/cli/src/environments.ee/variables/variables.controller.ee.ts @@ -1,7 +1,15 @@ import { VariableListRequestDto } from '@n8n/api-types'; -import { Delete, Get, GlobalScope, Licensed, Patch, Post, RestController } from '@/decorators'; -import { Query } from '@/decorators/args'; +import { + Delete, + Get, + GlobalScope, + Licensed, + Patch, + Post, + Query, + RestController, +} from '@/decorators'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; import { NotFoundError } from '@/errors/response-errors/not-found.error'; import { VariableCountLimitReachedError } from '@/errors/variable-count-limit-reached.error'; diff --git a/packages/cli/src/requests.ts b/packages/cli/src/requests.ts index f7ac415a7547f..6b8a57aeb62b8 100644 --- a/packages/cli/src/requests.ts +++ b/packages/cli/src/requests.ts @@ -253,18 +253,6 @@ export declare namespace UserRequest { error?: string; }; - export type ResolveSignUp = AuthlessRequest< - {}, - {}, - {}, - { inviterId?: string; inviteeId?: string } - >; - - export type SignUp = AuthenticatedRequest< - { id: string }, - { inviterId?: string; inviteeId?: string } - >; - export type Delete = AuthenticatedRequest< { id: string; email: string; identifier: string }, {}, @@ -295,21 +283,6 @@ export declare namespace UserRequest { >; } -// ---------------------------------- -// /login -// ---------------------------------- - -export type LoginRequest = AuthlessRequest< - {}, - {}, - { - email: string; - password: string; - mfaCode?: string; - mfaRecoveryCode?: string; - } ->; - // ---------------------------------- // MFA endpoints // ---------------------------------- diff --git a/packages/cli/test/integration/auth.api.test.ts b/packages/cli/test/integration/auth.api.test.ts index 6c1ddc5892011..4fa2a7145cbce 100644 --- a/packages/cli/test/integration/auth.api.test.ts +++ b/packages/cli/test/integration/auth.api.test.ts @@ -147,6 +147,21 @@ describe('POST /login', () => { const response = await testServer.authAgentFor(ownerUser).get('/login'); expect(response.statusCode).toBe(200); }); + + test('should fail on invalid email in the payload', async () => { + const response = await testServer.authlessAgent.post('/login').send({ + email: 'invalid-email', + password: ownerPassword, + }); + + expect(response.statusCode).toBe(400); + expect(response.body).toEqual({ + validation: 'email', + code: 'invalid_string', + message: 'Invalid email', + path: ['email'], + }); + }); }); describe('GET /login', () => { From 6414ae7282a300b27d57b057b3a9b91fe6bf462c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Thu, 26 Dec 2024 13:45:42 +0100 Subject: [PATCH 3/4] refactor(core): Update password-reset endpoints to use DTOs --- packages/@n8n/api-types/src/dto/index.ts | 8 ++ .../change-password-request.dto.test.ts | 114 ++++++++++++++++++ .../forgot-password-request.dto.test.ts | 47 ++++++++ .../resolve-password-token-query.dto.test.ts | 42 +++++++ .../change-password-request.dto.ts | 11 ++ .../forgot-password-request.dto.ts | 6 + .../resolve-password-token-query.dto.ts | 7 ++ .../api-types/src/schemas/password.schema.ts | 16 +++ .../src/schemas/passwordResetToken.schema.ts | 3 + .../controllers/password-reset.controller.ts | 74 ++++-------- packages/cli/src/requests.ts | 18 +-- packages/cli/src/services/password.utility.ts | 1 + 12 files changed, 282 insertions(+), 65 deletions(-) create mode 100644 packages/@n8n/api-types/src/dto/password-reset/__tests__/change-password-request.dto.test.ts create mode 100644 packages/@n8n/api-types/src/dto/password-reset/__tests__/forgot-password-request.dto.test.ts create mode 100644 packages/@n8n/api-types/src/dto/password-reset/__tests__/resolve-password-token-query.dto.test.ts create mode 100644 packages/@n8n/api-types/src/dto/password-reset/change-password-request.dto.ts create mode 100644 packages/@n8n/api-types/src/dto/password-reset/forgot-password-request.dto.ts create mode 100644 packages/@n8n/api-types/src/dto/password-reset/resolve-password-token-query.dto.ts create mode 100644 packages/@n8n/api-types/src/schemas/password.schema.ts create mode 100644 packages/@n8n/api-types/src/schemas/passwordResetToken.schema.ts diff --git a/packages/@n8n/api-types/src/dto/index.ts b/packages/@n8n/api-types/src/dto/index.ts index 70b598015899b..f3a1604ad9c07 100644 --- a/packages/@n8n/api-types/src/dto/index.ts +++ b/packages/@n8n/api-types/src/dto/index.ts @@ -1,11 +1,19 @@ export { AiAskRequestDto } from './ai/ai-ask-request.dto'; export { AiChatRequestDto } from './ai/ai-chat-request.dto'; export { AiApplySuggestionRequestDto } from './ai/ai-apply-suggestion-request.dto'; + export { LoginRequestDto } from './auth/login-request.dto'; export { ResolveSignupTokenQueryDto } from './auth/resolve-signup-token-query.dto'; + +export { ForgotPasswordRequestDto } from './password-reset/forgot-password-request.dto'; +export { ResolvePasswordTokenQueryDto } from './password-reset/resolve-password-token-query.dto'; +export { ChangePasswordRequestDto } from './password-reset/change-password-request.dto'; + export { PasswordUpdateRequestDto } from './user/password-update-request.dto'; export { RoleChangeRequestDto } from './user/role-change-request.dto'; export { SettingsUpdateRequestDto } from './user/settings-update-request.dto'; export { UserUpdateRequestDto } from './user/user-update-request.dto'; + export { CommunityRegisteredRequestDto } from './license/community-registered-request.dto'; + export { VariableListRequestDto } from './variables/variables-list-request.dto'; diff --git a/packages/@n8n/api-types/src/dto/password-reset/__tests__/change-password-request.dto.test.ts b/packages/@n8n/api-types/src/dto/password-reset/__tests__/change-password-request.dto.test.ts new file mode 100644 index 0000000000000..86b230ba5ad4d --- /dev/null +++ b/packages/@n8n/api-types/src/dto/password-reset/__tests__/change-password-request.dto.test.ts @@ -0,0 +1,114 @@ +import { ChangePasswordRequestDto } from '../change-password-request.dto'; + +describe('ChangePasswordRequestDto', () => { + describe('Valid requests', () => { + test.each([ + { + name: 'valid password reset with token', + request: { + token: 'valid-reset-token-with-sufficient-length', + password: 'newSecurePassword123', + }, + }, + { + name: 'valid password reset with MFA code', + request: { + token: 'another-valid-reset-token', + password: 'newSecurePassword123', + mfaCode: '123456', + }, + }, + ])('should validate $name', ({ request }) => { + const result = ChangePasswordRequestDto.safeParse(request); + expect(result.success).toBe(true); + }); + }); + + describe('Invalid requests', () => { + test.each([ + { + name: 'missing token', + request: { password: 'newSecurePassword123' }, + expectedErrorPath: ['token'], + }, + { + name: 'empty token', + request: { token: '', password: 'newSecurePassword123' }, + expectedErrorPath: ['token'], + }, + { + name: 'short token', + request: { token: 'short', password: 'newSecurePassword123' }, + expectedErrorPath: ['token'], + }, + { + name: 'missing password', + request: { token: 'valid-reset-token' }, + expectedErrorPath: ['password'], + }, + { + name: 'password too short', + request: { + token: 'valid-reset-token', + password: 'short', + }, + expectedErrorPath: ['password'], + }, + { + name: 'password too long', + request: { + token: 'valid-reset-token', + password: 'a'.repeat(65), + }, + expectedErrorPath: ['password'], + }, + { + name: 'password without number', + request: { + token: 'valid-reset-token', + password: 'NoNumberPassword', + }, + expectedErrorPath: ['password'], + }, + { + name: 'password without uppercase letter', + request: { + token: 'valid-reset-token', + password: 'nouppercasepassword123', + }, + expectedErrorPath: ['password'], + }, + ])('should fail validation for $name', ({ request, expectedErrorPath }) => { + const result = ChangePasswordRequestDto.safeParse(request); + + expect(result.success).toBe(false); + + if (expectedErrorPath) { + expect(result.error?.issues[0].path).toEqual(expectedErrorPath); + } + }); + + describe('Edge cases', () => { + test('should handle optional MFA code correctly', () => { + const validRequest = { + token: 'valid-reset-token', + password: 'newSecurePassword123', + mfaCode: undefined, + }; + + const result = ChangePasswordRequestDto.safeParse(validRequest); + expect(result.success).toBe(true); + }); + + test('should handle token with special characters', () => { + const validRequest = { + token: 'valid-reset-token-with-special-!@#$%^&*()_+', + password: 'newSecurePassword123', + }; + + const result = ChangePasswordRequestDto.safeParse(validRequest); + expect(result.success).toBe(true); + }); + }); + }); +}); diff --git a/packages/@n8n/api-types/src/dto/password-reset/__tests__/forgot-password-request.dto.test.ts b/packages/@n8n/api-types/src/dto/password-reset/__tests__/forgot-password-request.dto.test.ts new file mode 100644 index 0000000000000..891d52fdad022 --- /dev/null +++ b/packages/@n8n/api-types/src/dto/password-reset/__tests__/forgot-password-request.dto.test.ts @@ -0,0 +1,47 @@ +import { ForgotPasswordRequestDto } from '../forgot-password-request.dto'; + +describe('ForgotPasswordRequestDto', () => { + describe('Valid requests', () => { + test.each([ + { + name: 'valid email', + request: { email: 'test@example.com' }, + }, + { + name: 'email with subdomain', + request: { email: 'user@sub.example.com' }, + }, + ])('should validate $name', ({ request }) => { + const result = ForgotPasswordRequestDto.safeParse(request); + expect(result.success).toBe(true); + }); + }); + + describe('Invalid requests', () => { + test.each([ + { + name: 'invalid email format', + request: { email: 'invalid-email' }, + expectedErrorPath: ['email'], + }, + { + name: 'missing email', + request: {}, + expectedErrorPath: ['email'], + }, + { + name: 'empty email', + request: { email: '' }, + expectedErrorPath: ['email'], + }, + ])('should fail validation for $name', ({ request, expectedErrorPath }) => { + const result = ForgotPasswordRequestDto.safeParse(request); + + expect(result.success).toBe(false); + + if (expectedErrorPath) { + expect(result.error?.issues[0].path).toEqual(expectedErrorPath); + } + }); + }); +}); diff --git a/packages/@n8n/api-types/src/dto/password-reset/__tests__/resolve-password-token-query.dto.test.ts b/packages/@n8n/api-types/src/dto/password-reset/__tests__/resolve-password-token-query.dto.test.ts new file mode 100644 index 0000000000000..a2f5881ac81a6 --- /dev/null +++ b/packages/@n8n/api-types/src/dto/password-reset/__tests__/resolve-password-token-query.dto.test.ts @@ -0,0 +1,42 @@ +import { ResolvePasswordTokenQueryDto } from '../resolve-password-token-query.dto'; + +describe('ResolvePasswordTokenQueryDto', () => { + describe('Valid requests', () => { + test.each([ + { + name: 'valid token', + request: { token: 'valid-reset-token' }, + }, + { + name: 'long token', + request: { token: 'x'.repeat(50) }, + }, + ])('should validate $name', ({ request }) => { + const result = ResolvePasswordTokenQueryDto.safeParse(request); + expect(result.success).toBe(true); + }); + }); + + describe('Invalid requests', () => { + test.each([ + { + name: 'missing token', + request: {}, + expectedErrorPath: ['token'], + }, + { + name: 'empty token', + request: { token: '' }, + expectedErrorPath: ['token'], + }, + ])('should fail validation for $name', ({ request, expectedErrorPath }) => { + const result = ResolvePasswordTokenQueryDto.safeParse(request); + + expect(result.success).toBe(false); + + if (expectedErrorPath) { + expect(result.error?.issues[0].path).toEqual(expectedErrorPath); + } + }); + }); +}); diff --git a/packages/@n8n/api-types/src/dto/password-reset/change-password-request.dto.ts b/packages/@n8n/api-types/src/dto/password-reset/change-password-request.dto.ts new file mode 100644 index 0000000000000..33ef47b3f173f --- /dev/null +++ b/packages/@n8n/api-types/src/dto/password-reset/change-password-request.dto.ts @@ -0,0 +1,11 @@ +import { z } from 'zod'; +import { Z } from 'zod-class'; + +import { passwordSchema } from '../../schemas/password.schema'; +import { passwordResetTokenSchema } from '../../schemas/passwordResetToken.schema'; + +export class ChangePasswordRequestDto extends Z.class({ + token: passwordResetTokenSchema, + password: passwordSchema, + mfaCode: z.string().optional(), +}) {} diff --git a/packages/@n8n/api-types/src/dto/password-reset/forgot-password-request.dto.ts b/packages/@n8n/api-types/src/dto/password-reset/forgot-password-request.dto.ts new file mode 100644 index 0000000000000..f6ab3cfac5bcc --- /dev/null +++ b/packages/@n8n/api-types/src/dto/password-reset/forgot-password-request.dto.ts @@ -0,0 +1,6 @@ +import { z } from 'zod'; +import { Z } from 'zod-class'; + +export class ForgotPasswordRequestDto extends Z.class({ + email: z.string().email(), +}) {} diff --git a/packages/@n8n/api-types/src/dto/password-reset/resolve-password-token-query.dto.ts b/packages/@n8n/api-types/src/dto/password-reset/resolve-password-token-query.dto.ts new file mode 100644 index 0000000000000..88385df244e8d --- /dev/null +++ b/packages/@n8n/api-types/src/dto/password-reset/resolve-password-token-query.dto.ts @@ -0,0 +1,7 @@ +import { Z } from 'zod-class'; + +import { passwordResetTokenSchema } from '../../schemas/passwordResetToken.schema'; + +export class ResolvePasswordTokenQueryDto extends Z.class({ + token: passwordResetTokenSchema, +}) {} diff --git a/packages/@n8n/api-types/src/schemas/password.schema.ts b/packages/@n8n/api-types/src/schemas/password.schema.ts new file mode 100644 index 0000000000000..3c60470af730d --- /dev/null +++ b/packages/@n8n/api-types/src/schemas/password.schema.ts @@ -0,0 +1,16 @@ +import { z } from 'zod'; + +// TODO: Delete these from `cli` after all password-validation code starts using this schema +const minLength = 8; +const maxLength = 64; + +export const passwordSchema = z + .string() + .min(minLength, `Password must be ${minLength} to ${maxLength} characters long.`) + .max(maxLength, `Password must be ${minLength} to ${maxLength} characters long.`) + .refine((password) => /\d/.test(password), { + message: 'Password must contain at least 1 number.', + }) + .refine((password) => /[A-Z]/.test(password), { + message: 'Password must contain at least 1 uppercase letter.', + }); diff --git a/packages/@n8n/api-types/src/schemas/passwordResetToken.schema.ts b/packages/@n8n/api-types/src/schemas/passwordResetToken.schema.ts new file mode 100644 index 0000000000000..b7c55bb8869fb --- /dev/null +++ b/packages/@n8n/api-types/src/schemas/passwordResetToken.schema.ts @@ -0,0 +1,3 @@ +import { z } from 'zod'; + +export const passwordResetTokenSchema = z.string().min(10, 'Token too short'); diff --git a/packages/cli/src/controllers/password-reset.controller.ts b/packages/cli/src/controllers/password-reset.controller.ts index 7fc266c8e84e3..c2652aa785891 100644 --- a/packages/cli/src/controllers/password-reset.controller.ts +++ b/packages/cli/src/controllers/password-reset.controller.ts @@ -1,11 +1,15 @@ +import { + ChangePasswordRequestDto, + ForgotPasswordRequestDto, + ResolvePasswordTokenQueryDto, +} from '@n8n/api-types'; import { Response } from 'express'; import { Logger } from 'n8n-core'; -import validator from 'validator'; import { AuthService } from '@/auth/auth.service'; import { RESPONSE_ERROR_MESSAGES } from '@/constants'; import { UserRepository } from '@/databases/repositories/user.repository'; -import { Get, Post, RestController } from '@/decorators'; +import { Body, Get, Post, Query, RestController } from '@/decorators'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; import { InternalServerError } from '@/errors/response-errors/internal-server.error'; @@ -15,7 +19,7 @@ import { EventService } from '@/events/event.service'; import { ExternalHooks } from '@/external-hooks'; import { License } from '@/license'; import { MfaService } from '@/mfa/mfa.service'; -import { PasswordResetRequest } from '@/requests'; +import { AuthlessRequest } from '@/requests'; import { PasswordUtility } from '@/services/password.utility'; import { UserService } from '@/services/user.service'; import { isSamlCurrentAuthenticationMethod } from '@/sso.ee/sso-helpers'; @@ -40,7 +44,11 @@ export class PasswordResetController { * Send a password reset email. */ @Post('/forgot-password', { skipAuth: true, rateLimit: { limit: 3 } }) - async forgotPassword(req: PasswordResetRequest.Email) { + async forgotPassword( + _req: AuthlessRequest, + _res: Response, + @Body payload: ForgotPasswordRequestDto, + ) { if (!this.mailer.isEmailSetUp) { this.logger.debug( 'Request to send password reset email failed because emailing was not set up', @@ -50,22 +58,7 @@ export class PasswordResetController { ); } - const { email } = req.body; - if (!email) { - this.logger.debug( - 'Request to send password reset email failed because of missing email in payload', - { payload: req.body }, - ); - throw new BadRequestError('Email is mandatory'); - } - - if (!validator.isEmail(email)) { - this.logger.debug( - 'Request to send password reset email failed because of invalid email in payload', - { invalidEmail: email }, - ); - throw new BadRequestError('Invalid email address'); - } + const { email } = payload; // User should just be able to reset password if one is already present const user = await this.userRepository.findNonShellUser(email); @@ -138,19 +131,12 @@ export class PasswordResetController { * Verify password reset token and user ID. */ @Get('/resolve-password-token', { skipAuth: true }) - async resolvePasswordToken(req: PasswordResetRequest.Credentials) { - const { token } = req.query; - - if (!token) { - this.logger.debug( - 'Request to resolve password token failed because of missing password reset token', - { - queryString: req.query, - }, - ); - throw new BadRequestError(''); - } - + async resolvePasswordToken( + _req: AuthlessRequest, + _res: Response, + @Query payload: ResolvePasswordTokenQueryDto, + ) { + const { token } = payload; const user = await this.authService.resolvePasswordResetToken(token); if (!user) throw new NotFoundError(''); @@ -170,20 +156,12 @@ export class PasswordResetController { * Verify password reset token and update password. */ @Post('/change-password', { skipAuth: true }) - async changePassword(req: PasswordResetRequest.NewPassword, res: Response) { - const { token, password, mfaCode } = req.body; - - if (!token || !password) { - this.logger.debug( - 'Request to change password failed because of missing user ID or password or reset password token in payload', - { - payload: req.body, - }, - ); - throw new BadRequestError('Missing user ID or password or reset password token'); - } - - const validPassword = this.passwordUtility.validate(password); + async changePassword( + req: AuthlessRequest, + res: Response, + @Body payload: ChangePasswordRequestDto, + ) { + const { token, password, mfaCode } = payload; const user = await this.authService.resolvePasswordResetToken(token); if (!user) throw new NotFoundError(''); @@ -198,7 +176,7 @@ export class PasswordResetController { if (!validToken) throw new BadRequestError('Invalid MFA token.'); } - const passwordHash = await this.passwordUtility.hash(validPassword); + const passwordHash = await this.passwordUtility.hash(password); await this.userService.update(user.id, { password: passwordHash }); diff --git a/packages/cli/src/requests.ts b/packages/cli/src/requests.ts index 6b8a57aeb62b8..d476aedebe817 100644 --- a/packages/cli/src/requests.ts +++ b/packages/cli/src/requests.ts @@ -19,7 +19,7 @@ import type { AssignableRole, GlobalRole, User } from '@/databases/entities/user import type { Variables } from '@/databases/entities/variables'; import type { WorkflowEntity } from '@/databases/entities/workflow-entity'; import type { WorkflowHistory } from '@/databases/entities/workflow-history'; -import type { PublicUser, SecretsProvider, SecretsProviderState } from '@/interfaces'; +import type { SecretsProvider, SecretsProviderState } from '@/interfaces'; import type { ProjectRole } from './databases/entities/project-relation'; import type { ScopesField } from './services/role.service'; @@ -215,22 +215,6 @@ export declare namespace OwnerRequest { type DismissBanner = AuthenticatedRequest<{}, {}, Partial<{ bannerName: BannerName }>, {}>; } -// ---------------------------------- -// password reset endpoints -// ---------------------------------- - -export declare namespace PasswordResetRequest { - export type Email = AuthlessRequest<{}, {}, Pick>; - - export type Credentials = AuthlessRequest<{}, {}, {}, { userId?: string; token?: string }>; - - export type NewPassword = AuthlessRequest< - {}, - {}, - Pick & { token?: string; userId?: string; mfaCode?: string } - >; -} - // ---------------------------------- // /users // ---------------------------------- diff --git a/packages/cli/src/services/password.utility.ts b/packages/cli/src/services/password.utility.ts index 9719db44bb09f..6ae6aad61f8a2 100644 --- a/packages/cli/src/services/password.utility.ts +++ b/packages/cli/src/services/password.utility.ts @@ -19,6 +19,7 @@ export class PasswordUtility { return await compare(plaintext, hashed); } + /** @deprecated. All input validation should move to DTOs */ validate(plaintext?: string) { if (!plaintext) throw new BadRequestError('Password is mandatory'); From b501248035622bca8cfdeddc58fb45b2ed710457 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Thu, 26 Dec 2024 13:59:16 +0100 Subject: [PATCH 4/4] refactor(core): Update owner endpoints to use DTOs --- packages/@n8n/api-types/src/dto/index.ts | 3 + .../dismiss-banner-request.dto.test.ts | 64 ++++++++++ .../__tests__/owner-setup-request.dto.test.ts | 93 ++++++++++++++ .../dto/owner/dismiss-banner-request.dto.ts | 7 ++ .../src/dto/owner/owner-setup-request.dto.ts | 11 ++ packages/@n8n/api-types/src/index.ts | 1 + .../src/schemas/bannerName.schema.ts | 11 ++ .../__tests__/owner.controller.test.ts | 117 ++++++++++-------- .../cli/src/controllers/e2e.controller.ts | 11 +- .../cli/src/controllers/owner.controller.ts | 39 +++--- packages/cli/src/requests.ts | 21 ---- .../cli/test/integration/mfa/mfa.api.test.ts | 4 +- packages/editor-ui/src/Interface.ts | 2 +- packages/editor-ui/src/api/ui.ts | 2 +- .../src/components/banners/BaseBanner.vue | 2 +- packages/editor-ui/src/stores/ui.store.ts | 2 +- packages/workflow/src/Interfaces.ts | 7 -- 17 files changed, 282 insertions(+), 115 deletions(-) create mode 100644 packages/@n8n/api-types/src/dto/owner/__tests__/dismiss-banner-request.dto.test.ts create mode 100644 packages/@n8n/api-types/src/dto/owner/__tests__/owner-setup-request.dto.test.ts create mode 100644 packages/@n8n/api-types/src/dto/owner/dismiss-banner-request.dto.ts create mode 100644 packages/@n8n/api-types/src/dto/owner/owner-setup-request.dto.ts create mode 100644 packages/@n8n/api-types/src/schemas/bannerName.schema.ts diff --git a/packages/@n8n/api-types/src/dto/index.ts b/packages/@n8n/api-types/src/dto/index.ts index f3a1604ad9c07..96f55087a159a 100644 --- a/packages/@n8n/api-types/src/dto/index.ts +++ b/packages/@n8n/api-types/src/dto/index.ts @@ -5,6 +5,9 @@ export { AiApplySuggestionRequestDto } from './ai/ai-apply-suggestion-request.dt export { LoginRequestDto } from './auth/login-request.dto'; export { ResolveSignupTokenQueryDto } from './auth/resolve-signup-token-query.dto'; +export { OwnerSetupRequestDto } from './owner/owner-setup-request.dto'; +export { DismissBannerRequestDto } from './owner/dismiss-banner-request.dto'; + export { ForgotPasswordRequestDto } from './password-reset/forgot-password-request.dto'; export { ResolvePasswordTokenQueryDto } from './password-reset/resolve-password-token-query.dto'; export { ChangePasswordRequestDto } from './password-reset/change-password-request.dto'; diff --git a/packages/@n8n/api-types/src/dto/owner/__tests__/dismiss-banner-request.dto.test.ts b/packages/@n8n/api-types/src/dto/owner/__tests__/dismiss-banner-request.dto.test.ts new file mode 100644 index 0000000000000..97371de16a10c --- /dev/null +++ b/packages/@n8n/api-types/src/dto/owner/__tests__/dismiss-banner-request.dto.test.ts @@ -0,0 +1,64 @@ +import { bannerNameSchema } from '../../../schemas/bannerName.schema'; +import { DismissBannerRequestDto } from '../dismiss-banner-request.dto'; + +describe('DismissBannerRequestDto', () => { + describe('Valid requests', () => { + test.each( + bannerNameSchema.options.map((banner) => ({ + name: `valid banner: ${banner}`, + request: { banner }, + })), + )('should validate $name', ({ request }) => { + const result = DismissBannerRequestDto.safeParse(request); + expect(result.success).toBe(true); + }); + }); + + describe('Invalid requests', () => { + test.each([ + { + name: 'invalid banner string', + request: { + banner: 'not-a-valid-banner', + }, + expectedErrorPath: ['banner'], + }, + { + name: 'non-string banner', + request: { + banner: 123, + }, + expectedErrorPath: ['banner'], + }, + ])('should fail validation for $name', ({ request, expectedErrorPath }) => { + const result = DismissBannerRequestDto.safeParse(request); + + expect(result.success).toBe(false); + + if (expectedErrorPath) { + expect(result.error?.issues[0].path).toEqual(expectedErrorPath); + } + }); + }); + + describe('Optional banner', () => { + test('should validate empty request', () => { + const result = DismissBannerRequestDto.safeParse({}); + expect(result.success).toBe(true); + }); + }); + + describe('Exhaustive banner name check', () => { + test('should have all banner names defined', () => { + const expectedBanners = [ + 'V1', + 'TRIAL_OVER', + 'TRIAL', + 'NON_PRODUCTION_LICENSE', + 'EMAIL_CONFIRMATION', + ]; + + expect(bannerNameSchema.options).toEqual(expectedBanners); + }); + }); +}); diff --git a/packages/@n8n/api-types/src/dto/owner/__tests__/owner-setup-request.dto.test.ts b/packages/@n8n/api-types/src/dto/owner/__tests__/owner-setup-request.dto.test.ts new file mode 100644 index 0000000000000..facf808ec3f34 --- /dev/null +++ b/packages/@n8n/api-types/src/dto/owner/__tests__/owner-setup-request.dto.test.ts @@ -0,0 +1,93 @@ +import { OwnerSetupRequestDto } from '../owner-setup-request.dto'; + +describe('OwnerSetupRequestDto', () => { + describe('Valid requests', () => { + test.each([ + { + name: 'complete valid setup request', + request: { + email: 'owner@example.com', + firstName: 'John', + lastName: 'Doe', + password: 'SecurePassword123', + }, + }, + ])('should validate $name', ({ request }) => { + const result = OwnerSetupRequestDto.safeParse(request); + expect(result.success).toBe(true); + }); + }); + + describe('Invalid requests', () => { + test.each([ + { + name: 'invalid email', + request: { + email: 'invalid-email', + firstName: 'John', + lastName: 'Doe', + password: 'SecurePassword123', + }, + expectedErrorPath: ['email'], + }, + { + name: 'missing first name', + request: { + email: 'owner@example.com', + firstName: '', + lastName: 'Doe', + password: 'SecurePassword123', + }, + expectedErrorPath: ['firstName'], + }, + { + name: 'missing last name', + request: { + email: 'owner@example.com', + firstName: 'John', + lastName: '', + password: 'SecurePassword123', + }, + expectedErrorPath: ['lastName'], + }, + { + name: 'password too short', + request: { + email: 'owner@example.com', + firstName: 'John', + lastName: 'Doe', + password: 'short', + }, + expectedErrorPath: ['password'], + }, + { + name: 'password without number', + request: { + email: 'owner@example.com', + firstName: 'John', + lastName: 'Doe', + password: 'NoNumberPassword', + }, + expectedErrorPath: ['password'], + }, + { + name: 'password without uppercase letter', + request: { + email: 'owner@example.com', + firstName: 'John', + lastName: 'Doe', + password: 'nouppercasepassword123', + }, + expectedErrorPath: ['password'], + }, + ])('should fail validation for $name', ({ request, expectedErrorPath }) => { + const result = OwnerSetupRequestDto.safeParse(request); + + expect(result.success).toBe(false); + + if (expectedErrorPath) { + expect(result.error?.issues[0].path).toEqual(expectedErrorPath); + } + }); + }); +}); diff --git a/packages/@n8n/api-types/src/dto/owner/dismiss-banner-request.dto.ts b/packages/@n8n/api-types/src/dto/owner/dismiss-banner-request.dto.ts new file mode 100644 index 0000000000000..1f42381e7a71a --- /dev/null +++ b/packages/@n8n/api-types/src/dto/owner/dismiss-banner-request.dto.ts @@ -0,0 +1,7 @@ +import { Z } from 'zod-class'; + +import { bannerNameSchema } from '../../schemas/bannerName.schema'; + +export class DismissBannerRequestDto extends Z.class({ + banner: bannerNameSchema.optional(), +}) {} diff --git a/packages/@n8n/api-types/src/dto/owner/owner-setup-request.dto.ts b/packages/@n8n/api-types/src/dto/owner/owner-setup-request.dto.ts new file mode 100644 index 0000000000000..ccaa06b18e78a --- /dev/null +++ b/packages/@n8n/api-types/src/dto/owner/owner-setup-request.dto.ts @@ -0,0 +1,11 @@ +import { z } from 'zod'; +import { Z } from 'zod-class'; + +import { passwordSchema } from '../../schemas/password.schema'; + +export class OwnerSetupRequestDto extends Z.class({ + email: z.string().email(), + firstName: z.string().min(1, 'First name is required'), + lastName: z.string().min(1, 'Last name is required'), + password: passwordSchema, +}) {} diff --git a/packages/@n8n/api-types/src/index.ts b/packages/@n8n/api-types/src/index.ts index d0067f7fff67e..b446ca3971903 100644 --- a/packages/@n8n/api-types/src/index.ts +++ b/packages/@n8n/api-types/src/index.ts @@ -5,5 +5,6 @@ export type * from './scaling'; export type * from './frontend-settings'; export type * from './user'; +export type { BannerName } from './schemas/bannerName.schema'; export type { Collaborator } from './push/collaboration'; export type { SendWorkerStatusMessage } from './push/worker'; diff --git a/packages/@n8n/api-types/src/schemas/bannerName.schema.ts b/packages/@n8n/api-types/src/schemas/bannerName.schema.ts new file mode 100644 index 0000000000000..445bc31d1ad52 --- /dev/null +++ b/packages/@n8n/api-types/src/schemas/bannerName.schema.ts @@ -0,0 +1,11 @@ +import { z } from 'zod'; + +export const bannerNameSchema = z.enum([ + 'V1', + 'TRIAL_OVER', + 'TRIAL', + 'NON_PRODUCTION_LICENSE', + 'EMAIL_CONFIRMATION', +]); + +export type BannerName = z.infer; diff --git a/packages/cli/src/controllers/__tests__/owner.controller.test.ts b/packages/cli/src/controllers/__tests__/owner.controller.test.ts index 0fd42aae438f1..b5065fa283688 100644 --- a/packages/cli/src/controllers/__tests__/owner.controller.test.ts +++ b/packages/cli/src/controllers/__tests__/owner.controller.test.ts @@ -1,7 +1,7 @@ +import type { DismissBannerRequestDto, OwnerSetupRequestDto } from '@n8n/api-types'; import type { Response } from 'express'; -import { anyObject, mock } from 'jest-mock-extended'; -import jwt from 'jsonwebtoken'; -import Container from 'typedi'; +import { mock } from 'jest-mock-extended'; +import type { Logger } from 'n8n-core'; import type { AuthService } from '@/auth/auth.service'; import config from '@/config'; @@ -10,27 +10,31 @@ import type { User } from '@/databases/entities/user'; import type { SettingsRepository } from '@/databases/repositories/settings.repository'; import type { UserRepository } from '@/databases/repositories/user.repository'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; -import { License } from '@/license'; -import type { OwnerRequest } from '@/requests'; -import { PasswordUtility } from '@/services/password.utility'; +import type { EventService } from '@/events/event.service'; +import type { PublicUser } from '@/interfaces'; +import type { AuthenticatedRequest } from '@/requests'; +import type { PasswordUtility } from '@/services/password.utility'; import type { UserService } from '@/services/user.service'; -import { mockInstance } from '@test/mocking'; -import { badPasswords } from '@test/test-data'; describe('OwnerController', () => { const configGetSpy = jest.spyOn(config, 'getEnv'); + const configSetSpy = jest.spyOn(config, 'set'); + + const logger = mock(); + const eventService = mock(); const authService = mock(); const userService = mock(); const userRepository = mock(); const settingsRepository = mock(); - mockInstance(License).isWithinUsersLimit.mockReturnValue(true); + const passwordUtility = mock(); + const controller = new OwnerController( - mock(), - mock(), + logger, + eventService, settingsRepository, authService, userService, - Container.get(PasswordUtility), + passwordUtility, mock(), userRepository, ); @@ -38,38 +42,18 @@ describe('OwnerController', () => { describe('setupOwner', () => { it('should throw a BadRequestError if the instance owner is already setup', async () => { configGetSpy.mockReturnValue(true); - await expect(controller.setupOwner(mock(), mock())).rejects.toThrowError( + await expect(controller.setupOwner(mock(), mock(), mock())).rejects.toThrowError( new BadRequestError('Instance owner already setup'), ); - }); - it('should throw a BadRequestError if the email is invalid', async () => { - configGetSpy.mockReturnValue(false); - const req = mock({ body: { email: 'invalid email' } }); - await expect(controller.setupOwner(req, mock())).rejects.toThrowError( - new BadRequestError('Invalid email address'), - ); - }); - - describe('should throw if the password is invalid', () => { - Object.entries(badPasswords).forEach(([password, errorMessage]) => { - it(password, async () => { - configGetSpy.mockReturnValue(false); - const req = mock({ body: { email: 'valid@email.com', password } }); - await expect(controller.setupOwner(req, mock())).rejects.toThrowError( - new BadRequestError(errorMessage), - ); - }); - }); - }); - - it('should throw a BadRequestError if firstName & lastName are missing ', async () => { - configGetSpy.mockReturnValue(false); - const req = mock({ - body: { email: 'valid@email.com', password: 'NewPassword123', firstName: '', lastName: '' }, - }); - await expect(controller.setupOwner(req, mock())).rejects.toThrowError( - new BadRequestError('First and last names are mandatory'), + expect(userRepository.findOneOrFail).not.toHaveBeenCalled(); + expect(userRepository.save).not.toHaveBeenCalled(); + expect(authService.issueCookie).not.toHaveBeenCalled(); + expect(settingsRepository.update).not.toHaveBeenCalled(); + expect(configSetSpy).not.toHaveBeenCalled(); + expect(eventService.emit).not.toHaveBeenCalled(); + expect(logger.debug).toHaveBeenCalledWith( + 'Request to claim instance ownership failed because instance owner already exists', ); }); @@ -80,29 +64,52 @@ describe('OwnerController', () => { authIdentities: [], }); const browserId = 'test-browser-id'; - const req = mock({ - body: { - email: 'valid@email.com', - password: 'NewPassword123', - firstName: 'Jane', - lastName: 'Doe', - }, - user, - browserId, - }); + const req = mock({ user, browserId }); const res = mock(); + const payload = mock({ + email: 'valid@email.com', + password: 'NewPassword123', + firstName: 'Jane', + lastName: 'Doe', + }); configGetSpy.mockReturnValue(false); - userRepository.findOneOrFail.calledWith(anyObject()).mockResolvedValue(user); - userRepository.save.calledWith(anyObject()).mockResolvedValue(user); - jest.spyOn(jwt, 'sign').mockImplementation(() => 'signed-token'); + userRepository.findOneOrFail.mockResolvedValue(user); + userRepository.save.mockResolvedValue(user); + userService.toPublic.mockResolvedValue(mock({ id: 'newUserId' })); - await controller.setupOwner(req, res); + const result = await controller.setupOwner(req, res, payload); expect(userRepository.findOneOrFail).toHaveBeenCalledWith({ where: { role: 'global:owner' }, }); expect(userRepository.save).toHaveBeenCalledWith(user, { transaction: false }); expect(authService.issueCookie).toHaveBeenCalledWith(res, user, browserId); + expect(settingsRepository.update).toHaveBeenCalledWith( + { key: 'userManagement.isInstanceOwnerSetUp' }, + { value: JSON.stringify(true) }, + ); + expect(configSetSpy).toHaveBeenCalledWith('userManagement.isInstanceOwnerSetUp', true); + expect(eventService.emit).toHaveBeenCalledWith('instance-owner-setup', { userId: 'userId' }); + expect(result.id).toEqual('newUserId'); + }); + }); + + describe('dismissBanner', () => { + it('should not call dismissBanner if no banner is provided', async () => { + const payload = mock({ banner: undefined }); + + const result = await controller.dismissBanner(mock(), mock(), payload); + + expect(settingsRepository.dismissBanner).not.toHaveBeenCalled(); + expect(result).toBeUndefined(); + }); + + it('should call dismissBanner with the correct banner name', async () => { + const payload = mock({ banner: 'TRIAL' }); + + await controller.dismissBanner(mock(), mock(), payload); + + expect(settingsRepository.dismissBanner).toHaveBeenCalledWith({ bannerName: 'TRIAL' }); }); }); }); diff --git a/packages/cli/src/controllers/e2e.controller.ts b/packages/cli/src/controllers/e2e.controller.ts index 9d0404d3126d2..4430dfc9fa7fc 100644 --- a/packages/cli/src/controllers/e2e.controller.ts +++ b/packages/cli/src/controllers/e2e.controller.ts @@ -17,7 +17,6 @@ import type { FeatureReturnType } from '@/license'; import { License } from '@/license'; import { MfaService } from '@/mfa/mfa.service'; import { Push } from '@/push'; -import type { UserSetupPayload } from '@/requests'; import { CacheService } from '@/services/cache/cache.service'; import { PasswordUtility } from '@/services/password.utility'; @@ -48,6 +47,16 @@ const tablesToTruncate = [ 'workflows_tags', ]; +type UserSetupPayload = { + email: string; + password: string; + firstName: string; + lastName: string; + mfaEnabled?: boolean; + mfaSecret?: string; + mfaRecoveryCodes?: string[]; +}; + type ResetRequest = Request< {}, {}, diff --git a/packages/cli/src/controllers/owner.controller.ts b/packages/cli/src/controllers/owner.controller.ts index 1db250c488a40..5c7e8d1e2ac84 100644 --- a/packages/cli/src/controllers/owner.controller.ts +++ b/packages/cli/src/controllers/owner.controller.ts @@ -1,17 +1,17 @@ +import { DismissBannerRequestDto, OwnerSetupRequestDto } from '@n8n/api-types'; import { Response } from 'express'; import { Logger } from 'n8n-core'; -import validator from 'validator'; import { AuthService } from '@/auth/auth.service'; import config from '@/config'; import { SettingsRepository } from '@/databases/repositories/settings.repository'; import { UserRepository } from '@/databases/repositories/user.repository'; -import { GlobalScope, Post, RestController } from '@/decorators'; +import { Body, GlobalScope, Post, RestController } from '@/decorators'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; import { EventService } from '@/events/event.service'; import { validateEntity } from '@/generic-helpers'; import { PostHogClient } from '@/posthog'; -import { OwnerRequest } from '@/requests'; +import { AuthenticatedRequest } from '@/requests'; import { PasswordUtility } from '@/services/password.utility'; import { UserService } from '@/services/user.service'; @@ -33,8 +33,8 @@ export class OwnerController { * and enable `isInstanceOwnerSetUp` setting. */ @Post('/setup', { skipAuth: true }) - async setupOwner(req: OwnerRequest.Post, res: Response) { - const { email, firstName, lastName, password } = req.body; + async setupOwner(req: AuthenticatedRequest, res: Response, @Body payload: OwnerSetupRequestDto) { + const { email, firstName, lastName, password } = payload; if (config.getEnv('userManagement.isInstanceOwnerSetUp')) { this.logger.debug( @@ -43,31 +43,15 @@ export class OwnerController { throw new BadRequestError('Instance owner already setup'); } - if (!email || !validator.isEmail(email)) { - this.logger.debug('Request to claim instance ownership failed because of invalid email', { - invalidEmail: email, - }); - throw new BadRequestError('Invalid email address'); - } - - const validPassword = this.passwordUtility.validate(password); - - if (!firstName || !lastName) { - this.logger.debug( - 'Request to claim instance ownership failed because of missing first name or last name in payload', - { payload: req.body }, - ); - throw new BadRequestError('First and last names are mandatory'); - } - let owner = await this.userRepository.findOneOrFail({ where: { role: 'global:owner' }, }); owner.email = email; owner.firstName = firstName; owner.lastName = lastName; - owner.password = await this.passwordUtility.hash(validPassword); + owner.password = await this.passwordUtility.hash(password); + // TODO: move XSS validation out into the DTO class await validateEntity(owner); owner = await this.userRepository.save(owner, { transaction: false }); @@ -92,8 +76,13 @@ export class OwnerController { @Post('/dismiss-banner') @GlobalScope('banner:dismiss') - async dismissBanner(req: OwnerRequest.DismissBanner) { - const bannerName = 'banner' in req.body ? (req.body.banner as string) : ''; + async dismissBanner( + _req: AuthenticatedRequest, + _res: Response, + @Body payload: DismissBannerRequestDto, + ) { + const bannerName = payload.banner; + if (!bannerName) return; return await this.settingsRepository.dismissBanner({ bannerName }); } } diff --git a/packages/cli/src/requests.ts b/packages/cli/src/requests.ts index d476aedebe817..b49c46c7ffed3 100644 --- a/packages/cli/src/requests.ts +++ b/packages/cli/src/requests.ts @@ -1,7 +1,6 @@ import type { Scope } from '@n8n/permissions'; import type express from 'express'; import type { - BannerName, ICredentialDataDecryptedObject, IDataObject, ILoadOptions, @@ -195,26 +194,6 @@ export declare namespace MeRequest { export type SurveyAnswers = AuthenticatedRequest<{}, {}, IPersonalizationSurveyAnswersV4>; } -export interface UserSetupPayload { - email: string; - password: string; - firstName: string; - lastName: string; - mfaEnabled?: boolean; - mfaSecret?: string; - mfaRecoveryCodes?: string[]; -} - -// ---------------------------------- -// /owner -// ---------------------------------- - -export declare namespace OwnerRequest { - type Post = AuthenticatedRequest<{}, {}, UserSetupPayload, {}>; - - type DismissBanner = AuthenticatedRequest<{}, {}, Partial<{ bannerName: BannerName }>, {}>; -} - // ---------------------------------- // /users // ---------------------------------- diff --git a/packages/cli/test/integration/mfa/mfa.api.test.ts b/packages/cli/test/integration/mfa/mfa.api.test.ts index 498be7abd53a0..f69d98a74fdcd 100644 --- a/packages/cli/test/integration/mfa/mfa.api.test.ts +++ b/packages/cli/test/integration/mfa/mfa.api.test.ts @@ -1,4 +1,4 @@ -import { randomInt, randomString } from 'n8n-workflow'; +import { randomString } from 'n8n-workflow'; import Container from 'typedi'; import { AuthService } from '@/auth/auth.service'; @@ -239,7 +239,7 @@ describe('Change password with MFA enabled', () => { .send({ password: newPassword, token: resetPasswordToken, - mfaCode: randomInt(10), + mfaCode: randomString(10), }) .expect(404); }); diff --git a/packages/editor-ui/src/Interface.ts b/packages/editor-ui/src/Interface.ts index a2a1e8e065c21..1f799a3fa168f 100644 --- a/packages/editor-ui/src/Interface.ts +++ b/packages/editor-ui/src/Interface.ts @@ -2,6 +2,7 @@ import type { Component } from 'vue'; import type { NotificationOptions as ElementNotificationOptions } from 'element-plus'; import type { Connection } from '@jsplumb/core'; import type { + BannerName, FrontendSettings, Iso8601DateTimeString, IUserManagementSettings, @@ -38,7 +39,6 @@ import type { ITelemetryTrackProperties, WorkflowSettings, IUserSettings, - BannerName, INodeExecutionData, INodeProperties, NodeConnectionType, diff --git a/packages/editor-ui/src/api/ui.ts b/packages/editor-ui/src/api/ui.ts index 5b27669e53b85..7716415c6afb9 100644 --- a/packages/editor-ui/src/api/ui.ts +++ b/packages/editor-ui/src/api/ui.ts @@ -1,6 +1,6 @@ import type { IRestApiContext } from '@/Interface'; import { makeRestApiRequest } from '@/utils/apiUtils'; -import type { BannerName } from 'n8n-workflow'; +import type { BannerName } from '@n8n/api-types'; export async function dismissBannerPermanently( context: IRestApiContext, diff --git a/packages/editor-ui/src/components/banners/BaseBanner.vue b/packages/editor-ui/src/components/banners/BaseBanner.vue index 6a0c4a343bb4f..aac7e3428e2e2 100644 --- a/packages/editor-ui/src/components/banners/BaseBanner.vue +++ b/packages/editor-ui/src/components/banners/BaseBanner.vue @@ -1,7 +1,7 @@