Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Centralized error throwing for encryption key #3105

Merged
merged 6 commits into from
Apr 15, 2022
Merged

Centralized error throwing for encryption key #3105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bc0be05
Centralized error throwing for encryption key
krynble Apr 8, 2022
cca7c99
Unifying the error message used by cli and core packages
krynble Apr 13, 2022
f53b793
Improvements to error messages to make it more DRY
krynble Apr 13, 2022
6d6b872
Removed unnecessary throw
krynble Apr 13, 2022
2c2d2ff
:twisted_rightwards_arrows: Merge master
ivov Apr 14, 2022
9b5d3d6
Throwing error when credential does not exist to simplify node behavi…
krynble Apr 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions packages/cli/commands/export/credentials.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,6 @@ export class ExportCredentialsCommand extends Command {

if (flags.decrypted) {
const encryptionKey = await UserSettings.getEncryptionKey();
if (encryptionKey === undefined) {
throw new Error('No encryption key got found to decrypt the credentials!');
}

for (let i = 0; i < credentials.length; i++) {
const { name, type, nodesAccess, data } = credentials[i];
Expand Down
3 changes: 0 additions & 3 deletions packages/cli/commands/import/credentials.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,6 @@ export class ImportCredentialsCommand extends Command {
await UserSettings.prepareUserSettings();

const encryptionKey = await UserSettings.getEncryptionKey();
if (encryptionKey === undefined) {
throw new Error('No encryption key found to encrypt the credentials!');
}

if (flags.separate) {
const files = await glob(
Expand Down
10 changes: 1 addition & 9 deletions packages/cli/commands/start.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import {
} from '../src';

import { getLogger } from '../src/Logger';
import { RESPONSE_ERROR_MESSAGES } from '../src/constants';

// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-var-requires
const open = require('open');
Expand Down Expand Up @@ -173,9 +172,6 @@ export class Start extends Command {
// If we don't have a JWT secret set, generate
// one based and save to config.
const encryptionKey = await UserSettings.getEncryptionKey();
if (!encryptionKey) {
throw new Error('Fatal error setting up user management: no encryption key set.');
}

// For a key off every other letter from encryption key
// CAREFUL: do not change this or it breaks all existing tokens.
Expand Down Expand Up @@ -210,11 +206,7 @@ export class Start extends Command {
// Wait till the database is ready
await startDbInitPromise;

const encryptionKey = await UserSettings.getEncryptionKey();

if (!encryptionKey) {
throw new Error(RESPONSE_ERROR_MESSAGES.NO_ENCRYPTION_KEY);
}
await UserSettings.getEncryptionKey();

// Load settings from database and set them to config.
const databaseSettings = await Db.collections.Settings.find({ loadOnStartup: true });
Expand Down
52 changes: 20 additions & 32 deletions packages/cli/src/Server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1703,13 +1703,11 @@ class App {
);
}

const encryptionKey = await UserSettings.getEncryptionKey();
if (!encryptionKey) {
throw new ResponseHelper.ResponseError(
RESPONSE_ERROR_MESSAGES.NO_ENCRYPTION_KEY,
undefined,
500,
);
let encryptionKey: string;
try {
encryptionKey = await UserSettings.getEncryptionKey();
} catch (error) {
Comment on lines +1706 to +1709
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ref comment in UserSettings.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IN this case specifically we're changing the regular error to a new type of error, so this try / catch block is still necessary

throw new ResponseHelper.ResponseError(error.message, undefined, 500);
}

const mode: WorkflowExecuteMode = 'internal';
Expand Down Expand Up @@ -1842,15 +1840,11 @@ class App {
return ResponseHelper.sendErrorResponse(res, errorResponse);
}

const encryptionKey = await UserSettings.getEncryptionKey();

if (!encryptionKey) {
const errorResponse = new ResponseHelper.ResponseError(
RESPONSE_ERROR_MESSAGES.NO_ENCRYPTION_KEY,
undefined,
503,
);
return ResponseHelper.sendErrorResponse(res, errorResponse);
let encryptionKey: string;
try {
encryptionKey = await UserSettings.getEncryptionKey();
} catch (error) {
throw new ResponseHelper.ResponseError(error.message, undefined, 500);
}

const mode: WorkflowExecuteMode = 'internal';
Expand Down Expand Up @@ -1962,13 +1956,11 @@ class App {
);
}

const encryptionKey = await UserSettings.getEncryptionKey();
if (!encryptionKey) {
throw new ResponseHelper.ResponseError(
RESPONSE_ERROR_MESSAGES.NO_ENCRYPTION_KEY,
undefined,
500,
);
let encryptionKey: string;
try {
encryptionKey = await UserSettings.getEncryptionKey();
} catch (error) {
throw new ResponseHelper.ResponseError(error.message, undefined, 500);
}

const mode: WorkflowExecuteMode = 'internal';
Expand Down Expand Up @@ -2103,15 +2095,11 @@ class App {
return ResponseHelper.sendErrorResponse(res, errorResponse);
}

const encryptionKey = await UserSettings.getEncryptionKey();

if (!encryptionKey) {
const errorResponse = new ResponseHelper.ResponseError(
RESPONSE_ERROR_MESSAGES.NO_ENCRYPTION_KEY,
undefined,
503,
);
return ResponseHelper.sendErrorResponse(res, errorResponse);
let encryptionKey: string;
try {
encryptionKey = await UserSettings.getEncryptionKey();
} catch (error) {
throw new ResponseHelper.ResponseError(error.message, undefined, 500);
}

const mode: WorkflowExecuteMode = 'internal';
Expand Down
4 changes: 1 addition & 3 deletions packages/cli/src/WorkflowExecuteAdditionalData.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ import {
getWorkflowOwner,
} from './UserManagement/UserManagementHelper';
import { whereClause } from './WorkflowHelpers';
import { RESPONSE_ERROR_MESSAGES } from './constants';

const ERROR_TRIGGER_TYPE = config.getEnv('nodes.errorTriggerType');

Expand Down Expand Up @@ -1030,9 +1031,6 @@ export async function getBase(
const webhookTestBaseUrl = urlBaseWebhook + config.getEnv('endpoints.webhookTest');

const encryptionKey = await UserSettings.getEncryptionKey();
if (encryptionKey === undefined) {
throw new Error('No encryption key got found to decrypt the credentials!');
}

return {
credentialsHelper: new CredentialsHelper(encryptionKey),
Expand Down
37 changes: 21 additions & 16 deletions packages/cli/src/api/credentials.api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,13 +115,15 @@ credentialsController.post(
ResponseHelper.send(async (req: CredentialRequest.Test): Promise<INodeCredentialTestResult> => {
const { credentials, nodeToTestWith } = req.body;

const encryptionKey = await UserSettings.getEncryptionKey();

if (!encryptionKey) {
return {
status: 'Error',
message: RESPONSE_ERROR_MESSAGES.NO_ENCRYPTION_KEY,
};
let encryptionKey: string;
try {
encryptionKey = await UserSettings.getEncryptionKey();
} catch (error) {
throw new ResponseHelper.ResponseError(
RESPONSE_ERROR_MESSAGES.NO_ENCRYPTION_KEY,
undefined,
500,
);
}

const helper = new CredentialsHelper(encryptionKey);
Expand Down Expand Up @@ -149,9 +151,10 @@ credentialsController.post(
nodeAccess.date = new Date();
}

const encryptionKey = await UserSettings.getEncryptionKey();

if (!encryptionKey) {
let encryptionKey: string;
try {
encryptionKey = await UserSettings.getEncryptionKey();
} catch (error) {
throw new ResponseHelper.ResponseError(
RESPONSE_ERROR_MESSAGES.NO_ENCRYPTION_KEY,
undefined,
Expand Down Expand Up @@ -285,9 +288,10 @@ credentialsController.patch(
}
}

const encryptionKey = await UserSettings.getEncryptionKey();

if (!encryptionKey) {
let encryptionKey: string;
try {
encryptionKey = await UserSettings.getEncryptionKey();
} catch (error) {
throw new ResponseHelper.ResponseError(
RESPONSE_ERROR_MESSAGES.NO_ENCRYPTION_KEY,
undefined,
Expand Down Expand Up @@ -393,9 +397,10 @@ credentialsController.get(

const { data, id, ...rest } = credential;

const encryptionKey = await UserSettings.getEncryptionKey();

if (!encryptionKey) {
let encryptionKey: string;
try {
encryptionKey = await UserSettings.getEncryptionKey();
} catch (error) {
throw new ResponseHelper.ResponseError(
RESPONSE_ERROR_MESSAGES.NO_ENCRYPTION_KEY,
undefined,
Expand Down
6 changes: 5 additions & 1 deletion packages/cli/src/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
/* eslint-disable @typescript-eslint/no-unsafe-assignment */
/* eslint-disable @typescript-eslint/no-unsafe-member-access */
/* eslint-disable @typescript-eslint/naming-convention */

import { RESPONSE_ERROR_MESSAGES as CORE_RESPONSE_ERROR_MESSAGES } from 'n8n-core';

export const RESPONSE_ERROR_MESSAGES = {
NO_CREDENTIAL: 'Credential not found',
NO_ENCRYPTION_KEY: 'Encryption key missing to decrypt credentials',
NO_ENCRYPTION_KEY: CORE_RESPONSE_ERROR_MESSAGES.NO_ENCRYPTION_KEY,
};

export const AUTH_COOKIE_NAME = 'n8n-auth';
9 changes: 6 additions & 3 deletions packages/cli/test/integration/credentials.api.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import type { CredentialPayload, SaveCredentialFunction } from './shared/types';
import type { Role } from '../../src/databases/entities/Role';
import type { User } from '../../src/databases/entities/User';
import * as testDb from './shared/testDb';
import { RESPONSE_ERROR_MESSAGES } from '../../src/constants';
import { CredentialsEntity } from '../../src/databases/entities/CredentialsEntity';

jest.mock('../../src/telemetry');
Expand Down Expand Up @@ -91,7 +92,7 @@ test('POST /credentials should fail with invalid inputs', async () => {

test('POST /credentials should fail with missing encryption key', async () => {
const mock = jest.spyOn(UserSettings, 'getEncryptionKey');
mock.mockResolvedValue(undefined);
mock.mockRejectedValue(new Error(RESPONSE_ERROR_MESSAGES.NO_ENCRYPTION_KEY));

const ownerShell = await testDb.createUserShell(globalOwnerRole);
const authOwnerAgent = utils.createAgent(app, { auth: true, user: ownerShell });
Expand Down Expand Up @@ -354,7 +355,8 @@ test('PATCH /credentials/:id should fail if cred not found', async () => {

test('PATCH /credentials/:id should fail with missing encryption key', async () => {
const mock = jest.spyOn(UserSettings, 'getEncryptionKey');
mock.mockResolvedValue(undefined);
mock.mockRejectedValue(new Error(RESPONSE_ERROR_MESSAGES.NO_ENCRYPTION_KEY));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we also check for the correct messages in the tests?
atm we just check for the 500 status-code but not for what actually gets returned to the user as the message



const ownerShell = await testDb.createUserShell(globalOwnerRole);
const authOwnerAgent = utils.createAgent(app, { auth: true, user: ownerShell });
Expand Down Expand Up @@ -504,7 +506,8 @@ test('GET /credentials/:id should fail with missing encryption key', async () =>
const savedCredential = await saveCredential(credentialPayload(), { user: ownerShell });

const mock = jest.spyOn(UserSettings, 'getEncryptionKey');
mock.mockResolvedValue(undefined);
mock.mockRejectedValue(new Error(RESPONSE_ERROR_MESSAGES.NO_ENCRYPTION_KEY));


const response = await authOwnerAgent
.get(`/credentials/${savedCredential.id}`)
Expand Down
4 changes: 0 additions & 4 deletions packages/cli/test/integration/shared/testDb.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,10 +392,6 @@ export const getMySqlOptions = ({ name }: { name: string }): ConnectionOptions =
async function encryptCredentialData(credential: CredentialsEntity) {
const encryptionKey = await UserSettings.getEncryptionKey();

if (!encryptionKey) {
throw new Error(RESPONSE_ERROR_MESSAGES.NO_ENCRYPTION_KEY);
}

const coreCredential = new Credentials(
{ id: null, name: credential.name },
credential.type,
Expand Down
5 changes: 5 additions & 0 deletions packages/core/src/Constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* eslint-disable @typescript-eslint/naming-convention */
export const BINARY_ENCODING = 'base64';
export const CUSTOM_EXTENSION_ENV = 'N8N_CUSTOM_EXTENSIONS';
export const ENCRYPTION_KEY_ENV_OVERWRITE = 'N8N_ENCRYPTION_KEY';
Expand All @@ -9,3 +10,7 @@ export const PLACEHOLDER_EMPTY_EXECUTION_ID = '__UNKOWN__';
export const PLACEHOLDER_EMPTY_WORKFLOW_ID = '__EMPTY__';
export const TUNNEL_SUBDOMAIN_ENV = 'N8N_TUNNEL_SUBDOMAIN';
export const WAIT_TIME_UNLIMITED = '3000-01-01T00:00:00.000Z';

export const RESPONSE_ERROR_MESSAGES = {
NO_ENCRYPTION_KEY: 'Encryption key is missing or was not set',
};
Loading