chore(core): Add more tests

packages/@n8n/permissions/src/__tests__/__snapshots__/scope-information.test.ts.snap

@@ -37,7 +37,6 @@ exports[`Scope Information ensure scopes are defined correctly 1`] = `
   "externalSecretsProvider:list",
   "externalSecretsProvider:*",
   "externalSecret:list",
-  "externalSecret:use",
   "externalSecret:*",
   "eventBusDestination:test",
   "eventBusDestination:create",

packages/@n8n/permissions/src/constants.ee.ts

@@ -8,7 +8,7 @@ export const RESOURCES = {
 	communityPackage: ['install', 'uninstall', 'update', 'list', 'manage'] as const,
 	credential: ['share', 'shareGlobally', 'move', ...DEFAULT_OPERATIONS] as const,
 	externalSecretsProvider: ['sync', ...DEFAULT_OPERATIONS] as const,
-	externalSecret: ['list', 'use'] as const,
+	externalSecret: ['list'] as const,
 	eventBusDestination: ['test', ...DEFAULT_OPERATIONS] as const,
 	ldap: ['sync', 'manage'] as const,
 	license: ['manage'] as const,

packages/@n8n/permissions/src/roles/scopes/global-scopes.ee.ts

@@ -34,7 +34,6 @@ export const GLOBAL_OWNER_SCOPES: Scope[] = [
 	'externalSecretsProvider:list',
 	'externalSecretsProvider:sync',
 	'externalSecret:list',
-	'externalSecret:use',
 	'ldap:manage',
 	'ldap:sync',
 	'license:manage',

packages/cli/src/credentials/__tests__/credentials.controller.test.ts

@@ -1,22 +1,57 @@
-import type { AuthenticatedRequest, SharedCredentialsRepository, CredentialsEntity } from '@n8n/db';
-import { GLOBAL_OWNER_ROLE, GLOBAL_MEMBER_ROLE } from '@n8n/db';
+import type {
+	AuthenticatedRequest,
+	CredentialsEntity,
+	CredentialsRepository,
+	SharedCredentialsRepository,
+} from '@n8n/db';
+import { GLOBAL_MEMBER_ROLE, GLOBAL_OWNER_ROLE } from '@n8n/db';
 import { mock } from 'jest-mock-extended';
 
 import { createRawProjectData } from '@/__tests__/project.test-data';
 import type { EventService } from '@/events/event.service';
 
-import { createdCredentialsWithScopes, createNewCredentialsPayload } from './credentials.test-data';
-import { CredentialsController } from '../credentials.controller';
-import type { CredentialsService } from '../credentials.service';
-import type { CredentialsFinderService } from '../credentials-finder.service';
 import type { CredentialRequest } from '@/requests';
+import type { CredentialsFinderService } from '../credentials-finder.service';
+import { CredentialsController } from '../credentials.controller';
+import { CredentialsService } from '../credentials.service';
+import { createdCredentialsWithScopes, createNewCredentialsPayload } from './credentials.test-data';
 
 describe('CredentialsController', () => {
 	const eventService = mock<EventService>();
-	const credentialsService = mock<CredentialsService>();
 	const sharedCredentialsRepository = mock<SharedCredentialsRepository>();
 	const credentialsFinderService = mock<CredentialsFinderService>();
 
+	// Mock the credentialsRepository with a working create method
+	const credentialsRepository = mock<CredentialsRepository>();
+
+	// real CredentialsService instance with mocked dependencies
+	const credentialsService = new CredentialsService(
+		credentialsRepository,
+		mock(), // sharedCredentialsRepository
+		mock(), // ownershipService
+		mock(), // logger
+		mock(), // errorReporter
+		mock(), // credentialsTester
+		mock(), // externalHooks
+		mock(), // credentialTypes
+		mock(), // projectRepository
+		mock(), // projectService
+		mock(), // roleService
+		mock(), // userRepository
+		mock(), // credentialsFinderService
+		mock(), // credentialsHelper
+	);
+
+	// Spy on methods that need to be mocked in tests
+	// This allows us to mock specific behavior while keeping real implementations
+	// for isChangingExternalSecretExpression and validateExternalSecretsPermissions
+	jest.spyOn(credentialsService, 'decrypt');
+	jest.spyOn(credentialsService, 'prepareUpdateData');
+	jest.spyOn(credentialsService, 'createEncryptedData');
+	jest.spyOn(credentialsService, 'getCredentialScopes');
+	jest.spyOn(credentialsService, 'update');
+	jest.spyOn(credentialsService, 'createUnmanagedCredential');
+
 	const credentialsController = new CredentialsController(
 		mock(),
 		credentialsService,
@@ -39,12 +74,12 @@ describe('CredentialsController', () => {
 
 	beforeEach(() => {
 		jest.resetAllMocks();
+		// Set up credentialsRepository.create to return the input data
+		credentialsRepository.create.mockImplementation((data) => data as CredentialsEntity);
 	});
 
 	describe('createCredentials', () => {
 		it('should create new credentials and emit "credentials-created"', async () => {
-			// Arrange
-
 			const newCredentialsPayload = createNewCredentialsPayload();
 
 			req.body = newCredentialsPayload;
@@ -57,8 +92,9 @@ describe('CredentialsController', () => {
 				id: newCredentialsPayload.projectId,
 			});
 
-			// @ts-ignore
-			credentialsService.createUnmanagedCredential.mockResolvedValue(createdCredentials);
+			jest
+				.mocked(credentialsService.createUnmanagedCredential)
+				.mockResolvedValue(createdCredentials);
 
 			sharedCredentialsRepository.findCredentialOwningProject.mockResolvedValue(
 				projectOwningCredentialData,
@@ -106,24 +142,18 @@ describe('CredentialsController', () => {
 		});
 
 		beforeEach(() => {
-			credentialsService.decrypt.mockReturnValue({ apiKey: 'test-key' });
-			credentialsService.prepareUpdateData.mockResolvedValue({
-				name: 'Updated Credential',
-				type: 'apiKey',
-				data: { apiKey: 'updated-key' },
-			} as any);
-			credentialsService.createEncryptedData.mockReturnValue({
+			jest.mocked(credentialsService.decrypt).mockReturnValue({ apiKey: 'test-key' });
+			jest.mocked(credentialsService.createEncryptedData).mockReturnValue({
 				name: 'Updated Credential',
 				type: 'apiKey',
 				data: 'encrypted-data',
 				id: 'cred-123',
 				createdAt: new Date(),
 				updatedAt: new Date(),
 			} as any);
-			credentialsService.getCredentialScopes.mockResolvedValue([
-				'credential:read',
-				'credential:update',
-			] as any);
+			jest
+				.mocked(credentialsService.getCredentialScopes)
+				.mockResolvedValue(['credential:read', 'credential:update'] as any);
 		});
 
 		it('should allow owner to set isGlobal to true', async () => {
@@ -140,7 +170,7 @@ describe('CredentialsController', () => {
 			} as unknown as CredentialRequest.Update;
 
 			credentialsFinderService.findCredentialForUser.mockResolvedValue(existingCredential);
-			credentialsService.update.mockResolvedValue({
+			jest.mocked(credentialsService.update).mockResolvedValue({
 				...existingCredential,
 				name: 'Updated Credential',
 				isGlobal: true,
@@ -181,7 +211,7 @@ describe('CredentialsController', () => {
 			} as unknown as CredentialRequest.Update;
 
 			credentialsFinderService.findCredentialForUser.mockResolvedValue(globalCredential);
-			credentialsService.update.mockResolvedValue({
+			jest.mocked(credentialsService.update).mockResolvedValue({
 				...globalCredential,
 				isGlobal: false,
 			});
@@ -263,7 +293,7 @@ describe('CredentialsController', () => {
 			} as unknown as CredentialRequest.Update;
 
 			credentialsFinderService.findCredentialForUser.mockResolvedValue(existingCredential);
-			credentialsService.update.mockResolvedValue({
+			jest.mocked(credentialsService.update).mockResolvedValue({
 				...existingCredential,
 				name: 'Updated Credential',
 			});

packages/cli/src/credentials/__tests__/credentials.service.test.ts

@@ -16,6 +16,7 @@ import { CREDENTIAL_BLANKING_VALUE } from '@/constants';
 import type { CredentialTypes } from '@/credential-types';
 import type { CredentialsFinderService } from '@/credentials/credentials-finder.service';
 import { CredentialsService } from '@/credentials/credentials.service';
+import * as validation from '@/credentials/validation';
 import type { CredentialsHelper } from '@/credentials-helper';
 import type { ExternalHooks } from '@/external-hooks';
 import type { CredentialsTester } from '@/services/credentials-tester.service';
@@ -1548,6 +1549,23 @@ describe('CredentialsService', () => {
 				'The field "apiKey" is mandatory for credentials of type "apiKey"',
 			);
 		});
+
+		it('should prevent use of external secret expression when required permission is missing', async () => {
+			credentialsHelper.getCredentialsProperties.mockReturnValue([]);
+			const payload = {
+				name: 'Test Credential',
+				type: 'apiKey',
+				data: {
+					apiKey: '$secrets.myApiKey',
+					url: 'https://api.example.com',
+				},
+				projectId: 'project-1',
+			};
+
+			await expect(service.createUnmanagedCredential(payload, memberUser)).rejects.toThrow(
+				'Lacking permissions to reference external secrets in credentials',
+			);
+		});
 	});
 
 	describe('createManagedCredential', () => {
@@ -1641,4 +1659,136 @@ describe('CredentialsService', () => {
 			expect(result).toHaveProperty('name', 'Managed Credential');
 		});
 	});
+
+	describe('checkCredentialData', () => {
+		const ownerUser = mock<User>({ id: 'owner-id', role: GLOBAL_OWNER_ROLE });
+
+		beforeEach(() => {
+			jest.clearAllMocks();
+		});
+
+		it('should pass when all required fields are provided', () => {
+			credentialsHelper.getCredentialsProperties.mockReturnValue([
+				{
+					displayName: 'API Key',
+					name: 'apiKey',
+					type: 'string',
+					required: true,
+					default: '',
+				},
+				{
+					displayName: 'Domain',
+					name: 'domain',
+					type: 'string',
+					required: true,
+					default: '',
+				},
+			]);
+
+			const data = {
+				apiKey: 'test-key',
+				domain: 'example.com',
+			};
+
+			expect(() => service.checkCredentialData('apiCredential', data, ownerUser)).not.toThrow();
+		});
+
+		it('should pass when required field with valid default is not provided', () => {
+			credentialsHelper.getCredentialsProperties.mockReturnValue([
+				{
+					displayName: 'Host',
+					name: 'host',
+					type: 'string',
+					required: true,
+					default: 'https://api.example.com',
+				},
+			]);
+
+			const data = {};
+
+			expect(() => service.checkCredentialData('apiCredential', data, ownerUser)).not.toThrow();
+		});
+
+		it('should pass when credential has no required fields', () => {
+			credentialsHelper.getCredentialsProperties.mockReturnValue([
+				{
+					displayName: 'Optional Field',
+					name: 'optionalField',
+					type: 'string',
+					required: false,
+					default: '',
+				},
+			]);
+
+			const data = {};
+
+			expect(() => service.checkCredentialData('apiCredential', data, ownerUser)).not.toThrow();
+		});
+
+		it('should call validateExternalSecretsPermissions', () => {
+			credentialsHelper.getCredentialsProperties.mockReturnValue([]);
+			const validateSpy = jest.spyOn(validation, 'validateExternalSecretsPermissions');
+
+			const data = { apiKey: 'test-key' };
+
+			service.checkCredentialData('apiCredential', data, ownerUser);
+
+			expect(validateSpy).toHaveBeenCalledWith(ownerUser, data);
+		});
+
+		it('should throw BadRequestError when required field is missing and has no default', () => {
+			credentialsHelper.getCredentialsProperties.mockReturnValue([
+				{
+					displayName: 'API Key',
+					name: 'apiKey',
+					type: 'string',
+					required: true,
+					default: undefined,
+				},
+			]);
+
+			const data = {}; // apiKey is missing
+
+			expect(() => service.checkCredentialData('apiCredential', data, ownerUser)).toThrow(
+				'The field "apiKey" is mandatory for credentials of type "apiCredential"',
+			);
+		});
+
+		it('should throw when required field value is undefined', () => {
+			credentialsHelper.getCredentialsProperties.mockReturnValue([
+				{
+					displayName: 'API Key',
+					name: 'apiKey',
+					type: 'string',
+					required: true,
+					default: '',
+				},
+			]);
+
+			const data = { apiKey: undefined };
+
+			// @ts-expect-error - Testing edge case with undefined value
+			expect(() => service.checkCredentialData('apiCredential', data, ownerUser)).toThrow(
+				'The field "apiKey" is mandatory for credentials of type "apiCredential"',
+			);
+		});
+
+		it('should throw when required field value is empty string', () => {
+			credentialsHelper.getCredentialsProperties.mockReturnValue([
+				{
+					displayName: 'API Key',
+					name: 'apiKey',
+					type: 'string',
+					required: true,
+					default: '',
+				},
+			]);
+
+			const data = { apiKey: '' };
+
+			expect(() => service.checkCredentialData('apiCredential', data, ownerUser)).toThrow(
+				'The field "apiKey" is mandatory for credentials of type "apiCredential"',
+			);
+		});
+	});
 });

packages/cli/src/credentials/__tests__/credentials.test-data.ts

@@ -1,18 +1,10 @@
 import type { CreateCredentialDto } from '@n8n/api-types';
+import type { CredentialsEntity } from '@n8n/db';
 import type { Scope } from '@n8n/permissions';
 import { nanoId, date } from 'minifaker';
 import { randomString } from 'n8n-workflow';
 
-type NewCredentialWithSCopes = {
-	scopes: Scope[];
-	name: string;
-	data: string;
-	type: string;
-	isManaged: boolean;
-	id: string;
-	createdAt: Date;
-	updatedAt: Date;
-};
+export type NewCredentialWithScopes = CredentialsEntity & { scopes: Scope[] };
 
 const name = 'new Credential';
 const type = 'openAiApi';
@@ -43,17 +35,17 @@ export const createNewCredentialsPayload = (payload?: Partial<CreateCredentialDt
 };
 
 export const createdCredentialsWithScopes = (
-	payload?: Partial<NewCredentialWithSCopes>,
-): NewCredentialWithSCopes => {
+	payload?: Partial<NewCredentialWithScopes>,
+): NewCredentialWithScopes => {
 	return {
-		name,
-		type,
-		data: randomString(20),
-		id: nanoId.nanoid(),
-		createdAt: date(),
-		updatedAt: date(),
-		isManaged: false,
-		scopes: credentialScopes,
-		...payload,
-	};
+		name: payload?.name ?? name,
+		type: payload?.type ?? type,
+		data: payload?.data ?? randomString(20),
+		id: payload?.id ?? nanoId.nanoid(),
+		createdAt: payload?.createdAt ?? date(),
+		updatedAt: payload?.updatedAt ?? date(),
+		isManaged: payload?.isManaged ?? false,
+		isGlobal: payload?.isGlobal ?? false,
+		scopes: payload?.scopes ?? credentialScopes,
+	} as NewCredentialWithScopes;
 };