feat(core): Add credential runtime checks and prevent tampering in manual run (#4481)

*  Create `PermissionChecker`

*  Adjust helper

* 🔥 Remove superseded helpers

*  Use `PermissionChecker`

* 🧪 Add test for dynamic router switching

*  Simplify checks

*  Export utils

*  Add missing `init` method

* 🧪 Write tests for `PermissionChecker`

* 📘 Update types

* 🧪 Fix tests

*  Set up `runManually()`

*  Refactor to reuse methods

* 🧪 Clear shared tables first

* 🔀 Adjust merge

*  Adjust imports
This commit is contained in:
Iván Ovejero
2022-11-11 11:14:45 +01:00
committed by GitHub
parent 50f7538779
commit d35d63a855
16 changed files with 497 additions and 233 deletions

View File

@@ -17,7 +17,13 @@ export function randomApiKey() {
const chooseRandomly = <T>(array: T[]) => array[Math.floor(Math.random() * array.length)];
const randomDigit = () => Math.floor(Math.random() * 10);
export const randomDigit = () => Math.floor(Math.random() * 10);
export const randomPositiveDigit = (): number => {
const digit = randomDigit();
return digit === 0 ? randomPositiveDigit() : digit;
};
const randomUppercaseLetter = () => chooseRandomly('ABCDEFGHIJKLMNOPQRSTUVWXYZ'.split(''));

View File

@@ -322,6 +322,10 @@ export async function createUser(attributes: Partial<User> = {}): Promise<User>
return Db.collections.User.save(user);
}
export async function createOwner() {
return createUser({ globalRole: await getGlobalOwnerRole() });
}
export function createUserShell(globalRole: Role): Promise<User> {
if (globalRole.scope !== 'global') {
throw new Error(`Invalid role received: ${JSON.stringify(globalRole)}`);

View File

@@ -17,12 +17,12 @@ jest.mock('@/telemetry');
let app: express.Application;
let testDbName = '';
let globalOwnerRole: Role;
let globalMemberRole: Role;
let credentialOwnerRole: Role;
let authAgent: AuthAgent;
let saveCredential: SaveCredentialFunction;
let isSharingEnabled: jest.SpyInstance<boolean>;
let workflowRunner: ActiveWorkflowRunner;
let sharingSpy: jest.SpyInstance<boolean>;
@@ -45,7 +45,9 @@ beforeAll(async () => {
utils.initTestLogger();
utils.initTestTelemetry();
config.set('enterprise.workflowSharingEnabled', true);
isSharingEnabled = jest.spyOn(UserManagementHelpers, 'isSharingEnabled').mockReturnValue(true);
config.set('enterprise.workflowSharingEnabled', true); // @TODO: Remove once temp flag is removed
await utils.initNodeTypes();
workflowRunner = await utils.initActiveWorkflowRunner();
@@ -62,6 +64,32 @@ afterAll(async () => {
await testDb.terminate(testDbName);
});
test('Router should switch dynamically', async () => {
const owner = await testDb.createUser({ globalRole: globalOwnerRole });
const member = await testDb.createUser({ globalRole: globalMemberRole });
const createWorkflowResponse = await authAgent(owner).post('/workflows').send(makeWorkflow());
const { id } = createWorkflowResponse.body.data;
// free router
isSharingEnabled.mockReturnValueOnce(false);
const freeShareResponse = await authAgent(owner)
.put(`/workflows/${id}/share`)
.send({ shareWithIds: [member.id] });
expect(freeShareResponse.status).toBe(404);
// EE router
const paidShareResponse = await authAgent(owner)
.put(`/workflows/${id}/share`)
.send({ shareWithIds: [member.id] });
expect(paidShareResponse.status).toBe(200);
});
describe('PUT /workflows/:id', () => {
test('PUT /workflows/:id/share should save sharing with new users', async () => {
const owner = await testDb.createUser({ globalRole: globalOwnerRole });

View File

@@ -36,7 +36,9 @@ class NodeTypesClass implements INodeTypes {
},
};
async init(nodeTypes: INodeTypeData): Promise<void> {}
async init(nodeTypes: INodeTypeData): Promise<void> {
this.nodeTypes = nodeTypes;
}
getAll(): INodeType[] {
return Object.values(this.nodeTypes).map((data) => NodeHelpers.getVersionedNodeType(data.type));

View File

@@ -0,0 +1,223 @@
import { v4 as uuid } from 'uuid';
import { INodeTypeData, INodeTypes, Workflow } from 'n8n-workflow';
import { Db } from '../../src';
import * as testDb from '../integration/shared/testDb';
import { NodeTypes as MockNodeTypes } from './Helpers';
import { PermissionChecker } from '../../src/UserManagement/PermissionChecker';
import {
randomCredentialPayload as randomCred,
randomPositiveDigit,
} from '../integration/shared/random';
import type { Role } from '../../src/databases/entities/Role';
import type { SaveCredentialFunction } from '../integration/shared/types';
let testDbName = '';
let mockNodeTypes: INodeTypes;
let credentialOwnerRole: Role;
let workflowOwnerRole: Role;
let saveCredential: SaveCredentialFunction;
beforeAll(async () => {
const initResult = await testDb.init();
testDbName = initResult.testDbName;
mockNodeTypes = MockNodeTypes();
await mockNodeTypes.init(MOCK_NODE_TYPES_DATA);
credentialOwnerRole = await testDb.getCredentialOwnerRole();
workflowOwnerRole = await testDb.getWorkflowOwnerRole();
saveCredential = testDb.affixRoleToSaveCredential(credentialOwnerRole);
});
beforeEach(async () => {
await testDb.truncate(['SharedWorkflow', 'SharedCredentials'], testDbName);
await testDb.truncate(['User', 'Workflow', 'Credentials'], testDbName);
});
afterAll(async () => {
await testDb.terminate(testDbName);
});
describe('PermissionChecker.check()', () => {
test('should allow if workflow has no creds', async () => {
const userId = uuid();
const workflow = new Workflow({
id: randomPositiveDigit().toString(),
name: 'test',
active: false,
connections: {},
nodeTypes: mockNodeTypes,
nodes: [
{
id: uuid(),
name: 'Start',
type: 'n8n-nodes-base.start',
typeVersion: 1,
parameters: {},
position: [0, 0],
},
],
});
expect(() => PermissionChecker.check(workflow, userId)).not.toThrow();
});
test('should allow if requesting user is instance owner', async () => {
const owner = await testDb.createOwner();
const workflow = new Workflow({
id: randomPositiveDigit().toString(),
name: 'test',
active: false,
connections: {},
nodeTypes: mockNodeTypes,
nodes: [
{
id: uuid(),
name: 'Action Network',
type: 'n8n-nodes-base.actionNetwork',
parameters: {},
typeVersion: 1,
position: [0, 0],
credentials: {
actionNetworkApi: {
id: randomPositiveDigit().toString(),
name: 'Action Network Account',
},
},
},
],
});
expect(async () => await PermissionChecker.check(workflow, owner.id)).not.toThrow();
});
test('should allow if workflow creds are valid subset', async () => {
const [owner, member] = await Promise.all([testDb.createOwner(), testDb.createUser()]);
const ownerCred = await saveCredential(randomCred(), { user: owner });
const memberCred = await saveCredential(randomCred(), { user: member });
const workflow = new Workflow({
id: randomPositiveDigit().toString(),
name: 'test',
active: false,
connections: {},
nodeTypes: mockNodeTypes,
nodes: [
{
id: uuid(),
name: 'Action Network',
type: 'n8n-nodes-base.actionNetwork',
parameters: {},
typeVersion: 1,
position: [0, 0],
credentials: {
actionNetworkApi: {
id: ownerCred.id.toString(),
name: ownerCred.name,
},
},
},
{
id: uuid(),
name: 'Action Network 2',
type: 'n8n-nodes-base.actionNetwork',
parameters: {},
typeVersion: 1,
position: [0, 0],
credentials: {
actionNetworkApi: {
id: memberCred.id.toString(),
name: memberCred.name,
},
},
},
],
});
expect(async () => await PermissionChecker.check(workflow, owner.id)).not.toThrow();
});
test('should deny if workflow creds are not valid subset', async () => {
const member = await testDb.createUser();
const memberCred = await saveCredential(randomCred(), { user: member });
const workflowDetails = {
id: randomPositiveDigit(),
name: 'test',
active: false,
connections: {},
nodeTypes: mockNodeTypes,
nodes: [
{
id: uuid(),
name: 'Action Network',
type: 'n8n-nodes-base.actionNetwork',
parameters: {},
typeVersion: 1,
position: [0, 0] as [number, number],
credentials: {
actionNetworkApi: {
id: memberCred.id.toString(),
name: memberCred.name,
},
},
},
{
id: uuid(),
name: 'Action Network 2',
type: 'n8n-nodes-base.actionNetwork',
parameters: {},
typeVersion: 1,
position: [0, 0] as [number, number],
credentials: {
actionNetworkApi: {
id: 'non-existing-credential-id',
name: 'Non-existing credential name',
},
},
},
],
};
const workflowEntity = await Db.collections.Workflow.save(workflowDetails);
await Db.collections.SharedWorkflow.save({
workflow: workflowEntity,
user: member,
role: workflowOwnerRole,
});
const workflow = new Workflow({ ...workflowDetails, id: workflowDetails.id.toString() });
expect(PermissionChecker.check(workflow, member.id)).rejects.toThrow();
});
});
const MOCK_NODE_TYPES_DATA = ['start', 'actionNetwork'].reduce<INodeTypeData>((acc, nodeName) => {
return (
(acc[`n8n-nodes-base.${nodeName}`] = {
sourcePath: '',
type: {
description: {
displayName: nodeName,
name: nodeName,
group: [],
description: '',
version: 1,
defaults: {},
inputs: [],
outputs: [],
properties: [],
},
},
}),
acc
);
}, {});