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 1 commit
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 @@ -86,9 +86,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
9 changes: 3 additions & 6 deletions packages/cli/commands/start.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,9 +173,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,9 +207,9 @@ export class Start extends Command {
// Wait till the database is ready
await startDbInitPromise;

const encryptionKey = await UserSettings.getEncryptionKey();

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

Expand Down
36 changes: 20 additions & 16 deletions packages/cli/src/Server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1696,8 +1696,10 @@ class App {
);
}

const encryptionKey = await UserSettings.getEncryptionKey();
if (!encryptionKey) {
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(
RESPONSE_ERROR_MESSAGES.NO_ENCRYPTION_KEY,
undefined,
Expand Down Expand Up @@ -1832,15 +1834,15 @@ class App {
return ResponseHelper.sendErrorResponse(res, errorResponse);
}

const encryptionKey = await UserSettings.getEncryptionKey();

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

const mode: WorkflowExecuteMode = 'internal';
Expand Down Expand Up @@ -1949,8 +1951,10 @@ class App {
);
}

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 @@ -2087,15 +2091,15 @@ class App {
return ResponseHelper.sendErrorResponse(res, errorResponse);
}

const encryptionKey = await UserSettings.getEncryptionKey();

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

const mode: WorkflowExecuteMode = 'internal';
Expand Down
9 changes: 6 additions & 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.get('nodes.errorTriggerType') as string;

Expand Down Expand Up @@ -1033,9 +1034,11 @@ export async function getBase(
const webhookWaitingBaseUrl = urlBaseWebhook + config.get('endpoints.webhookWaiting');
const webhookTestBaseUrl = urlBaseWebhook + config.get('endpoints.webhookTest');

const encryptionKey = await UserSettings.getEncryptionKey();
if (encryptionKey === undefined) {
throw new Error('No encryption key got found to decrypt the credentials!');
let encryptionKey: string;
try {
encryptionKey = await UserSettings.getEncryptionKey();
} catch (error) {
throw new Error(RESPONSE_ERROR_MESSAGES.NO_ENCRYPTION_KEY);
}

return {
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
8 changes: 5 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 @@ -87,7 +87,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('Encryption key not found.'));
Copy link
Contributor

Choose a reason for hiding this comment

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

if we apply the change in UserSettings.ts then we could insert the variable here so even if we change the wording the tests wouldn't break

if we don't change it then we should move Encryption key not found. to a variable and import here 👍🏼 (also at the other places in this file)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly, the wording is not super important but def if we can get everything equal that is best for consistency


const owner = await Db.collections.User!.findOneOrFail();
const authOwnerAgent = utils.createAgent(app, { auth: true, user: owner });
Expand Down Expand Up @@ -348,7 +348,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('Encryption key not found.'));


const owner = await Db.collections.User!.findOneOrFail();
const authOwnerAgent = utils.createAgent(app, { auth: true, user: owner });
Expand Down Expand Up @@ -494,7 +495,8 @@ test('GET /credentials/:id should fail with missing encryption key', async () =>
const savedCredential = await saveCredential(credentialPayload(), { user: owner });

const mock = jest.spyOn(UserSettings, 'getEncryptionKey');
mock.mockResolvedValue(undefined);
mock.mockRejectedValue(new Error('Encryption key not found.'));


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 @@ -373,10 +373,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
12 changes: 4 additions & 8 deletions packages/core/src/UserSettings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,19 +73,15 @@ export async function prepareUserSettings(): Promise<IUserSettings> {
* @returns
*/

export async function getEncryptionKey(): Promise<string | undefined> {
export async function getEncryptionKey(): Promise<string> {
if (process.env[ENCRYPTION_KEY_ENV_OVERWRITE] !== undefined) {
return process.env[ENCRYPTION_KEY_ENV_OVERWRITE];
return process.env[ENCRYPTION_KEY_ENV_OVERWRITE] as string;
}

const userSettings = await getUserSettings();

if (userSettings === undefined) {
return undefined;
}

if (userSettings.encryptionKey === undefined) {
return undefined;
if (userSettings === undefined || userSettings.encryptionKey === undefined) {
throw new Error('Encryption key not set');
Copy link
Contributor

Choose a reason for hiding this comment

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

could we throw RESPONSE_ERROR_MESSAGES.NO_ENCRYPTION_KEY here? because we are always catching this error and overwrite it. This would simplify things I think. wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately no, this file resides in the core package and the constant is in cli. What I could do is use the same string, but unfortunately I cannot relate to it here.

}

return userSettings.encryptionKey;
Expand Down