diff --git a/packages/@n8n/backend-test-utils/src/random.ts b/packages/@n8n/backend-test-utils/src/random.ts index 9e073614188..86ef0a7d3ba 100644 --- a/packages/@n8n/backend-test-utils/src/random.ts +++ b/packages/@n8n/backend-test-utils/src/random.ts @@ -46,9 +46,10 @@ export const randomEmail = () => `${randomName()}@${randomName()}.${randomTopLev export const randomCredentialPayload = ({ isManaged = false, isGlobal, -}: { isManaged?: boolean; isGlobal?: boolean } = {}): CredentialPayload => ({ + type, +}: { isManaged?: boolean; isGlobal?: boolean; type?: string } = {}): CredentialPayload => ({ name: randomName(), - type: randomName(), + type: type ?? 'githubApi', data: { accessToken: randomString(6, 16) }, isManaged, isGlobal, diff --git a/packages/cli/src/credentials/__tests__/credentials.service.test.ts b/packages/cli/src/credentials/__tests__/credentials.service.test.ts index 5aa3ab46686..f2f657cd764 100644 --- a/packages/cli/src/credentials/__tests__/credentials.service.test.ts +++ b/packages/cli/src/credentials/__tests__/credentials.service.test.ts @@ -1,3 +1,4 @@ +import type { Logger } from '@n8n/backend-common'; import type { CredentialsEntity, CredentialsRepository, @@ -10,17 +11,17 @@ import { GLOBAL_OWNER_ROLE, GLOBAL_MEMBER_ROLE } from '@n8n/db'; import { mock } from 'jest-mock-extended'; import { CREDENTIAL_ERRORS, CredentialDataError, Credentials, type ErrorReporter } from 'n8n-core'; import { CREDENTIAL_EMPTY_VALUE, type ICredentialType } from 'n8n-workflow'; -import type { Logger } from '@n8n/backend-common'; import { CREDENTIAL_BLANKING_VALUE } from '@/constants'; import type { CredentialTypes } from '@/credential-types'; -import { CredentialsService } from '@/credentials/credentials.service'; import type { CredentialsFinderService } from '@/credentials/credentials-finder.service'; +import { CredentialsService } from '@/credentials/credentials.service'; +import type { CredentialsHelper } from '@/credentials-helper'; +import type { ExternalHooks } from '@/external-hooks'; +import type { CredentialsTester } from '@/services/credentials-tester.service'; import type { OwnershipService } from '@/services/ownership.service'; import type { ProjectService } from '@/services/project.service.ee'; import type { RoleService } from '@/services/role.service'; -import type { CredentialsTester } from '@/services/credentials-tester.service'; -import type { ExternalHooks } from '@/external-hooks'; describe('CredentialsService', () => { const credType = mock({ @@ -54,6 +55,7 @@ describe('CredentialsService', () => { const roleService = mock(); const userRepository = mock(); const credentialsFinderService = mock(); + const credentialsHelper = mock(); const service = new CredentialsService( credentialsRepository, @@ -69,6 +71,7 @@ describe('CredentialsService', () => { roleService, userRepository, credentialsFinderService, + credentialsHelper, ); beforeEach(() => jest.resetAllMocks()); @@ -1383,6 +1386,7 @@ describe('CredentialsService', () => { id: 'project-1', } as any); projectService.getProjectRelationsForUser.mockResolvedValue([]); + credentialsHelper.getCredentialsProperties.mockReturnValue([]); }); it('should allow owner to create global credential', async () => { @@ -1476,4 +1480,96 @@ describe('CredentialsService', () => { expect(savedCredential.isGlobal).toBeUndefined(); }); }); + + describe('createManagedCredential', () => { + const ownerUser = mock({ id: 'owner-id', role: GLOBAL_OWNER_ROLE }); + + const credentialData = { + name: 'Managed Credential', + type: 'oauth2', + oauthProvider: 'google', + projectId: 'project-1', + data: { someData: 'value' }, + }; + + beforeEach(async () => { + // Mock the save chain + roleService.addScopes.mockImplementation( + (c) => + ({ + ...c, + scopes: ['credential:read', 'credential:update'], + }) as any, + ); + roleService.combineResourceScopes.mockReturnValue([ + 'credential:read', + 'credential:update', + ] as any); + sharedCredentialsRepository.findOne.mockResolvedValue({ role: 'credential:owner' } as any); + sharedCredentialsRepository.create.mockImplementation((data) => data as any); + sharedCredentialsRepository.find.mockResolvedValue([]); + externalHooks.run.mockResolvedValue(); + projectService.getProjectWithScope.mockResolvedValue({ + id: 'project-1', + } as any); + projectService.getProjectRelationsForUser.mockResolvedValue([]); + }); + + it('should throw BadRequestError when credential is missing required properties', async () => { + // ARRANGE + const payload = { ...credentialData }; + credentialsHelper.getCredentialsProperties.mockReturnValue([ + { + displayName: 'required prop', + name: 'requiredProp', + type: 'string', + required: true, + default: null, + }, + ]); + + // ACT + await expect(service.createManagedCredential(payload, ownerUser)).rejects.toThrow( + 'The field "requiredProp" is mandatory for credentials of type "oauth2"', + ); + }); + + it('should create managed credential when all required properties are provided', async () => { + // ARRANGE + const payload = { + ...credentialData, + data: { + requiredProp: 'some-value', + }, + }; + credentialsHelper.getCredentialsProperties.mockReturnValue([ + { + displayName: 'required prop', + name: 'requiredProp', + type: 'string', + required: true, + default: null, + }, + ]); + credentialsRepository.create.mockImplementation((data) => ({ ...data }) as any); + // @ts-expect-error - Mocking manager for testing + credentialsRepository.manager = { + transaction: jest.fn().mockImplementation(async (callback) => { + const mockManager = { + save: jest.fn().mockImplementation(async (entity) => { + return { ...entity, id: 'new-managed-cred-id' }; + }), + }; + return await callback(mockManager); + }), + }; + + // ACT + const result = await service.createManagedCredential(payload, ownerUser); + + // ASSERT + expect(result).toHaveProperty('id', 'new-managed-cred-id'); + expect(result).toHaveProperty('name', 'Managed Credential'); + }); + }); }); diff --git a/packages/cli/src/credentials/credentials.service.ts b/packages/cli/src/credentials/credentials.service.ts index 4be07531fad..fa9fb18b07a 100644 --- a/packages/cli/src/credentials/credentials.service.ts +++ b/packages/cli/src/credentials/credentials.service.ts @@ -30,6 +30,7 @@ import type { import { CREDENTIAL_EMPTY_VALUE, deepCopy, + displayParameter, isINodePropertyCollection, NodeHelpers, UnexpectedError, @@ -39,7 +40,7 @@ import { CredentialsFinderService } from './credentials-finder.service'; import { CREDENTIAL_BLANKING_VALUE } from '@/constants'; import { CredentialTypes } from '@/credential-types'; -import { createCredentialsFromCredentialsEntity } from '@/credentials-helper'; +import { createCredentialsFromCredentialsEntity, CredentialsHelper } from '@/credentials-helper'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; import { NotFoundError } from '@/errors/response-errors/not-found.error'; import { ExternalHooks } from '@/external-hooks'; @@ -76,6 +77,7 @@ export class CredentialsService { private readonly roleService: RoleService, private readonly userRepository: UserRepository, private readonly credentialsFinderService: CredentialsFinderService, + private readonly credentialsHelper: CredentialsHelper, ) {} private async addGlobalCredentials( @@ -863,6 +865,23 @@ export class CredentialsService { return await this.createCredential({ ...dto, isManaged: false }, user); } + private checkCredentialData(type: string, data: ICredentialDataDecryptedObject) { + // check mandatory fields are present + const credentialProperties = this.credentialsHelper.getCredentialsProperties(type); + for (const property of credentialProperties) { + if (property.required && displayParameter(data, property, null, null)) { + const value = data[property.name]; + if (value === undefined || value === null || value === '') { + throw new BadRequestError( + `The field "${property.name}" is mandatory for credentials of type "${type}"`, + ); + } + } + } + + // TODO: add further validation if needed + } + /** * Create a new managed credential in user's account and return it along the scopes. * Managed credentials are managed by n8n and cannot be edited by the user. @@ -872,6 +891,7 @@ export class CredentialsService { } private async createCredential(opts: CreateCredentialOptions, user: User) { + this.checkCredentialData(opts.type, opts.data as ICredentialDataDecryptedObject); const encryptedCredential = this.createEncryptedData({ id: null, name: opts.name, diff --git a/packages/cli/test/integration/access-control/built-in-roles.test.ts b/packages/cli/test/integration/access-control/built-in-roles.test.ts index e66120a3d3a..d68a84dc1bc 100644 --- a/packages/cli/test/integration/access-control/built-in-roles.test.ts +++ b/packages/cli/test/integration/access-control/built-in-roles.test.ts @@ -60,6 +60,8 @@ describe('Built-in Role Matrix Testing', () => { passwordReset: jest.fn(), }); + await utils.initCredentialsTypes(); + // Create standard users owner = await createOwner(); member1 = await createMember(); diff --git a/packages/cli/test/integration/access-control/cross-project-access.test.ts b/packages/cli/test/integration/access-control/cross-project-access.test.ts index 29b11466999..abd5d87707b 100644 --- a/packages/cli/test/integration/access-control/cross-project-access.test.ts +++ b/packages/cli/test/integration/access-control/cross-project-access.test.ts @@ -63,6 +63,8 @@ describe('Cross-Project Access Control Tests', () => { passwordReset: jest.fn(), }); + await utils.initCredentialsTypes(); + // Create standard users owner = await createOwner(); member1 = await createMember(); diff --git a/packages/cli/test/integration/access-control/custom-roles-functionality.test.ts b/packages/cli/test/integration/access-control/custom-roles-functionality.test.ts index 8741f64edb8..bd1fbfbcc0b 100644 --- a/packages/cli/test/integration/access-control/custom-roles-functionality.test.ts +++ b/packages/cli/test/integration/access-control/custom-roles-functionality.test.ts @@ -67,6 +67,8 @@ describe('Custom Role Functionality Tests', () => { passwordReset: jest.fn(), }); + await utils.initCredentialsTypes(); + // Create standard users owner = await createOwner(); member1 = await createMember(); diff --git a/packages/cli/test/integration/access-control/resource-access-matrix.test.ts b/packages/cli/test/integration/access-control/resource-access-matrix.test.ts index 91adc896065..0238f093247 100644 --- a/packages/cli/test/integration/access-control/resource-access-matrix.test.ts +++ b/packages/cli/test/integration/access-control/resource-access-matrix.test.ts @@ -64,6 +64,8 @@ describe('Resource Access Control Matrix Tests', () => { ownerAgent = testServer.authAgentFor(owner); testUserAgent = testServer.authAgentFor(testUser); + await utils.initCredentialsTypes(); + // Create custom roles with specific scopes workflowReadOnlyRole = await createCustomRoleWithScopeSlugs( ['workflow:read', 'workflow:list'], diff --git a/packages/cli/test/integration/ai/ai.api.test.ts b/packages/cli/test/integration/ai/ai.api.test.ts index 77d1d8705dc..5a70e1df841 100644 --- a/packages/cli/test/integration/ai/ai.api.test.ts +++ b/packages/cli/test/integration/ai/ai.api.test.ts @@ -16,7 +16,7 @@ import { AiService } from '@/services/ai.service'; import { createOwner } from '../shared/db/users'; import type { SuperAgentTest } from '../shared/types'; -import { setupTestServer } from '../shared/utils'; +import { initCredentialsTypes, setupTestServer } from '../shared/utils'; const createAiCreditsResponse = { apiKey: randomUUID(), @@ -37,6 +37,10 @@ let ownerPersonalProject: Project; let authOwnerAgent: SuperAgentTest; +beforeAll(async () => { + await initCredentialsTypes(); +}); + beforeEach(async () => { await testDb.truncate(['SharedCredentials', 'CredentialsEntity']); diff --git a/packages/cli/test/integration/credentials/credentials.api.ee.test.ts b/packages/cli/test/integration/credentials/credentials.api.ee.test.ts index edbe31af3aa..726e13e15f4 100644 --- a/packages/cli/test/integration/credentials/credentials.api.ee.test.ts +++ b/packages/cli/test/integration/credentials/credentials.api.ee.test.ts @@ -62,6 +62,8 @@ let projectRepository: ProjectRepository; beforeAll(async () => { await Container.get(RoleCacheService).refreshCache(); + + await utils.initCredentialsTypes(); }); beforeEach(async () => { diff --git a/packages/cli/test/integration/credentials/credentials.api.test.ts b/packages/cli/test/integration/credentials/credentials.api.test.ts index 243adccdf28..d413d3d72ab 100644 --- a/packages/cli/test/integration/credentials/credentials.api.test.ts +++ b/packages/cli/test/integration/credentials/credentials.api.test.ts @@ -20,6 +20,7 @@ import { randomString } from 'n8n-workflow'; import { CREDENTIAL_BLANKING_VALUE } from '@/constants'; import { CredentialsService } from '@/credentials/credentials.service'; +import { createCredentialsFromCredentialsEntity } from '@/credentials-helper'; import { CredentialsTester } from '@/services/credentials-tester.service'; import { @@ -31,11 +32,13 @@ import { } from '../shared/db/credentials'; import { createAdmin, createManyUsers, createMember, createOwner } from '../shared/db/users'; import type { SuperAgentTest } from '../shared/types'; -import { setupTestServer } from '../shared/utils'; +import { initCredentialsTypes, setupTestServer } from '../shared/utils'; const { any } = expect; -const testServer = setupTestServer({ endpointGroups: ['credentials'] }); +const testServer = setupTestServer({ + endpointGroups: ['credentials'], +}); let owner: User; let member: User; @@ -52,6 +55,10 @@ let authAdminAgent: SuperAgentTest; let projectRepository: ProjectRepository; let sharedCredentialsRepository: SharedCredentialsRepository; +beforeAll(async () => { + await initCredentialsTypes(); +}); + beforeEach(async () => { await testDb.truncate(['SharedCredentials', 'CredentialsEntity']); @@ -792,7 +799,6 @@ describe('POST /credentials', () => { const payload = randomCredentialPayload(); const response = await authMemberAgent.post('/credentials').send(payload); - expect(response.statusCode).toBe(200); const { id, name, type, data: encryptedData, scopes } = response.body.data; @@ -1301,15 +1307,16 @@ describe('PATCH /credentials/:id', () => { .query({ includeData: true }) .expect(200); - const { id, data } = response.body.data; + const { id } = response.body.data; expect(id).toBe(savedCredential.id); - // was overwritten - expect(data.accessToken).toBe(patchPayload.data.accessToken); // was not overwritten const dbCredential = await getCredentialById(savedCredential.id); - const unencryptedData = Container.get(CredentialsService).decrypt(dbCredential!); - expect(unencryptedData.oauthTokenData).toEqual(credential.data.oauthTokenData); + const unencryptedData = createCredentialsFromCredentialsEntity(dbCredential!); + expect(unencryptedData.getData().oauthTokenData).toEqual(credential.data.oauthTokenData); + + // was overwritten + expect(unencryptedData.getData().accessToken).toBe(patchPayload.data.accessToken); }); test('should fail with invalid inputs', async () => { diff --git a/packages/cli/test/integration/shared/utils/index.ts b/packages/cli/test/integration/shared/utils/index.ts index 0f9beb495b3..ce9e02457a8 100644 --- a/packages/cli/test/integration/shared/utils/index.ts +++ b/packages/cli/test/integration/shared/utils/index.ts @@ -13,6 +13,9 @@ import { } from 'n8n-core'; import { Ftp } from 'n8n-nodes-base/credentials/Ftp.credentials'; import { GithubApi } from 'n8n-nodes-base/credentials/GithubApi.credentials'; +import { HttpBasicAuth } from 'n8n-nodes-base/credentials/HttpBasicAuth.credentials'; +import { HttpHeaderAuth } from 'n8n-nodes-base/credentials/HttpHeaderAuth.credentials'; +import { OpenAiApi } from 'n8n-nodes-base/credentials/OpenAiApi.credentials'; import { Cron } from 'n8n-nodes-base/nodes/Cron/Cron.node'; import { FormTrigger } from 'n8n-nodes-base/nodes/Form/FormTrigger.node'; import { ScheduleTrigger } from 'n8n-nodes-base/nodes/Schedule/ScheduleTrigger.node'; @@ -64,6 +67,18 @@ export async function initCredentialsTypes(): Promise { type: new Ftp(), sourcePath: '', }, + openAiApi: { + type: new OpenAiApi(), + sourcePath: '', + }, + httpHeaderAuth: { + type: new HttpHeaderAuth(), + sourcePath: '', + }, + httpBasicAuth: { + type: new HttpBasicAuth(), + sourcePath: '', + }, }; } diff --git a/packages/frontend/editor-ui/src/features/credentials/components/CredentialEdit/CredentialEdit.vue b/packages/frontend/editor-ui/src/features/credentials/components/CredentialEdit/CredentialEdit.vue index c800fdea9ea..910db3d523b 100644 --- a/packages/frontend/editor-ui/src/features/credentials/components/CredentialEdit/CredentialEdit.vue +++ b/packages/frontend/editor-ui/src/features/credentials/components/CredentialEdit/CredentialEdit.vue @@ -690,6 +690,7 @@ async function saveCredential(): Promise { if (!requiredPropertiesFilled.value) { showValidationWarning.value = true; scrollToTop(); + return null; } else { showValidationWarning.value = false; } diff --git a/packages/testing/playwright/composables/TemplatesComposer.ts b/packages/testing/playwright/composables/TemplatesComposer.ts index 98010052d27..c658be7bd21 100644 --- a/packages/testing/playwright/composables/TemplatesComposer.ts +++ b/packages/testing/playwright/composables/TemplatesComposer.ts @@ -31,10 +31,14 @@ export class TemplatesComposer { * Opens credential creation, fills name, saves, and closes modal * @param appName - The name of the app (e.g. 'Shopify', 'X (Formerly Twitter)') */ - async fillDummyCredentialForApp(appName: string): Promise { + async fillDummyCredentialForApp( + appName: string, + { fields }: { fields: Record } = { fields: {} }, + ): Promise { await this.n8n.templateCredentialSetup.openCredentialCreation(appName); await this.n8n.templateCredentialSetup.credentialModal.getCredentialName().click(); await this.n8n.templateCredentialSetup.credentialModal.getNameInput().fill('test'); + await this.n8n.templateCredentialSetup.credentialModal.fillAllFields(fields); await this.n8n.templateCredentialSetup.credentialModal.save(); await this.n8n.templateCredentialSetup.credentialModal.close(); } @@ -43,8 +47,11 @@ export class TemplatesComposer { * Fill in dummy credentials for an app and handle confirmation dialog * @param appName - The name of the app */ - async fillDummyCredentialForAppWithConfirm(appName: string): Promise { - await this.fillDummyCredentialForApp(appName); + async fillDummyCredentialForAppWithConfirm( + appName: string, + { fields }: { fields: Record } = { fields: {} }, + ): Promise { + await this.fillDummyCredentialForApp(appName, { fields }); await this.n8n.templateCredentialSetup.dismissMessageBox(); } } diff --git a/packages/testing/playwright/tests/ui/34-template-credentials-setup.spec.ts b/packages/testing/playwright/tests/ui/34-template-credentials-setup.spec.ts index 7b963c47371..61000b25a1b 100644 --- a/packages/testing/playwright/tests/ui/34-template-credentials-setup.spec.ts +++ b/packages/testing/playwright/tests/ui/34-template-credentials-setup.spec.ts @@ -113,12 +113,18 @@ test.describe('Template credentials setup @db:reset', () => { await expect(n8n.templateCredentialSetup.getContinueButton()).toBeDisabled(); - await n8n.templatesComposer.fillDummyCredentialForApp('Shopify'); + await n8n.templatesComposer.fillDummyCredentialForApp('Shopify', { + fields: { shopSubdomain: 'test-shop', apiKey: 'test-token', password: 'test-key' }, + }); await expect(n8n.templateCredentialSetup.getContinueButton()).toBeEnabled(); - await n8n.templatesComposer.fillDummyCredentialForAppWithConfirm('X (Formerly Twitter)'); - await n8n.templatesComposer.fillDummyCredentialForApp('Telegram'); + await n8n.templatesComposer.fillDummyCredentialForAppWithConfirm('X (Formerly Twitter)', { + fields: { consumerKey: 'consumer-key', consumerSecret: 'consumer-secret' }, + }); + await n8n.templatesComposer.fillDummyCredentialForApp('Telegram', { + fields: { accessToken: 'bot-token' }, + }); await n8n.notifications.quickCloseAll(); @@ -214,7 +220,9 @@ test.describe('Template credentials setup @db:reset', () => { n8n, }) => { await n8n.navigate.toTemplateCredentialSetup(TEMPLATE_ID); - await n8n.templatesComposer.fillDummyCredentialForApp('Shopify'); + await n8n.templatesComposer.fillDummyCredentialForApp('Shopify', { + fields: { shopSubdomain: 'test-shop', apiKey: 'test-token', password: 'test-key' }, + }); await n8n.notifications.quickCloseAll(); @@ -232,9 +240,15 @@ test.describe('Template credentials setup @db:reset', () => { await n8n.canvas.getSetupWorkflowCredentialsButton().click(); await expect(n8n.workflowCredentialSetupModal.getModal()).toBeVisible(); - await n8n.templatesComposer.fillDummyCredentialForApp('Shopify'); - await n8n.templatesComposer.fillDummyCredentialForAppWithConfirm('X (Formerly Twitter)'); - await n8n.templatesComposer.fillDummyCredentialForApp('Telegram'); + await n8n.templatesComposer.fillDummyCredentialForApp('Shopify', { + fields: { shopSubdomain: 'test-shop', apiKey: 'test-token', password: 'test-key' }, + }); + await n8n.templatesComposer.fillDummyCredentialForAppWithConfirm('X (Formerly Twitter)', { + fields: { consumerKey: 'consumer-key', consumerSecret: 'consumer-secret' }, + }); + await n8n.templatesComposer.fillDummyCredentialForApp('Telegram', { + fields: { accessToken: 'bot-token' }, + }); await n8n.notifications.quickCloseAll(); diff --git a/packages/workflow/src/node-helpers.ts b/packages/workflow/src/node-helpers.ts index 45e7a90a82a..733d9bc7591 100644 --- a/packages/workflow/src/node-helpers.ts +++ b/packages/workflow/src/node-helpers.ts @@ -2,11 +2,11 @@ /* eslint-disable @typescript-eslint/no-unsafe-assignment */ /* eslint-disable @typescript-eslint/prefer-nullish-coalescing */ +import { ApplicationError } from '@n8n/errors'; import get from 'lodash/get'; import isEqual from 'lodash/isEqual'; import { EXECUTE_WORKFLOW_NODE_TYPE, WORKFLOW_TOOL_LANGCHAIN_NODE_TYPE } from './constants'; -import { ApplicationError } from '@n8n/errors'; import { NodeConnectionTypes } from './interfaces'; import type { FieldType, @@ -34,6 +34,7 @@ import type { GenericValue, DisplayCondition, NodeConnectionType, + ICredentialDataDecryptedObject, } from './interfaces'; import { validateFilterParameter } from './node-parameters/filter-parameter'; import type { IRunExecutionData } from './run-execution-data/run-execution-data'; @@ -252,11 +253,11 @@ export function isSubNodeType( } const getPropertyValues = ( - nodeValues: INodeParameters, + nodeValues: INodeParameters | ICredentialDataDecryptedObject, propertyName: string, node: Pick | null, nodeTypeDescription: INodeTypeDescription | null, - nodeValuesRoot: INodeParameters, + nodeValuesRoot: INodeParameters | ICredentialDataDecryptedObject, ) => { let value; if (propertyName.charAt(0) === '/') { @@ -350,11 +351,11 @@ const checkConditions = ( * @param {INodeParameters} [nodeValuesRoot] The root node-parameter-data */ export function displayParameter( - nodeValues: INodeParameters, + nodeValues: INodeParameters | ICredentialDataDecryptedObject, parameter: INodeProperties | INodeCredentialDescription | INodePropertyOptions, node: Pick | null, // Allow null as it does also get used by credentials and they do not have versioning yet nodeTypeDescription: INodeTypeDescription | null, - nodeValuesRoot?: INodeParameters, + nodeValuesRoot?: INodeParameters | ICredentialDataDecryptedObject, displayKey: 'displayOptions' | 'disabledOptions' = 'displayOptions', ) { if (!parameter[displayKey]) {