From ad6c6f60a1a6a21abc596498fe1f8a7ca65b9797 Mon Sep 17 00:00:00 2001 From: Omar Ajoue Date: Tue, 22 Nov 2022 13:05:51 +0100 Subject: [PATCH] refactor: Improve warnings and error messages to users about sharing (#4687) (no-changelog) * refactor: Improve warnings and error messages to users about sharing --- packages/cli/src/UserManagement/routes/auth.ts | 15 +++++++++------ .../src/credentials/credentials.controller.ee.ts | 2 +- .../cli/src/credentials/credentials.controller.ts | 4 ++-- .../cli/src/workflows/workflows.controller.ee.ts | 14 ++++++++++---- .../cli/src/workflows/workflows.controller.ts | 4 ++-- packages/cli/src/workflows/workflows.services.ts | 4 ++-- .../integration/workflows.controller.ee.test.ts | 14 +++++++------- 7 files changed, 33 insertions(+), 24 deletions(-) diff --git a/packages/cli/src/UserManagement/routes/auth.ts b/packages/cli/src/UserManagement/routes/auth.ts index 54652f37ba..522553df73 100644 --- a/packages/cli/src/UserManagement/routes/auth.ts +++ b/packages/cli/src/UserManagement/routes/auth.ts @@ -77,22 +77,25 @@ export function authenticationMethods(this: N8nApp): void { } if (config.get('userManagement.isInstanceOwnerSetUp')) { - const error = new Error('Not logged in'); - // @ts-ignore - error.httpStatusCode = 401; - throw error; + throw new ResponseHelper.ResponseError('Not logged in', undefined, 401); } try { user = await Db.collections.User.findOneOrFail({ relations: ['globalRole'] }); } catch (error) { - throw new Error( + throw new ResponseHelper.ResponseError( 'No users found in database - did you wipe the users table? Create at least one user.', + undefined, + 500, ); } if (user.email || user.password) { - throw new Error('Invalid database state - user has password set.'); + throw new ResponseHelper.ResponseError( + 'Invalid database state - user has password set.', + undefined, + 500, + ); } await issueCookie(res, user); diff --git a/packages/cli/src/credentials/credentials.controller.ee.ts b/packages/cli/src/credentials/credentials.controller.ee.ts index 93eddb8998..4c744d226d 100644 --- a/packages/cli/src/credentials/credentials.controller.ee.ts +++ b/packages/cli/src/credentials/credentials.controller.ee.ts @@ -64,7 +64,7 @@ EECredentialsController.get( if (!credential) { throw new ResponseHelper.ResponseError( - `Credential with ID "${credentialId}" could not be found.`, + 'Could not load the credential. If you think this is an error, ask the owner to share it with you again', undefined, 404, ); diff --git a/packages/cli/src/credentials/credentials.controller.ts b/packages/cli/src/credentials/credentials.controller.ts index ff25a5ff47..5d069d175b 100644 --- a/packages/cli/src/credentials/credentials.controller.ts +++ b/packages/cli/src/credentials/credentials.controller.ts @@ -160,7 +160,7 @@ credentialsController.patch( userId: req.user.id, }); throw new ResponseHelper.ResponseError( - `Credential with ID "${credentialId}" could not be found to be updated.`, + 'Credential to be updated not found. You can only update credentials owned by you', undefined, 404, ); @@ -218,7 +218,7 @@ credentialsController.delete( userId: req.user.id, }); throw new ResponseHelper.ResponseError( - `Credential with ID "${credentialId}" could not be found to be deleted.`, + 'Credential to be deleted not found. You can only removed credentials owned by you', undefined, 404, ); diff --git a/packages/cli/src/workflows/workflows.controller.ee.ts b/packages/cli/src/workflows/workflows.controller.ee.ts index 7e76cbade4..8d211af95a 100644 --- a/packages/cli/src/workflows/workflows.controller.ee.ts +++ b/packages/cli/src/workflows/workflows.controller.ee.ts @@ -87,7 +87,7 @@ EEWorkflowController.get( if (!workflow) { throw new ResponseHelper.ResponseError( - `Workflow with ID "${workflowId}" could not be found.`, + `Workflow with ID "${workflowId}" does not exist`, undefined, 404, ); @@ -96,7 +96,11 @@ EEWorkflowController.get( const userSharing = workflow.shared?.find((shared) => shared.user.id === req.user.id); if (!userSharing && req.user.globalRole.name !== 'owner') { - throw new ResponseHelper.ResponseError(`Forbidden.`, undefined, 403); + throw new ResponseHelper.ResponseError( + 'It looks like you cannot access this workflow. Ask the owner to share it with you.', + undefined, + 403, + ); } return EEWorkflows.addCredentialsToWorkflow( @@ -140,7 +144,7 @@ EEWorkflowController.post( EEWorkflows.validateCredentialPermissionsToUser(newWorkflow, allCredentials); } catch (error) { throw new ResponseHelper.ResponseError( - 'The workflow contains credentials that you do not have access to', + 'The workflow you are trying to save contains credentials that are not shared with you', undefined, 400, ); @@ -169,7 +173,9 @@ EEWorkflowController.post( if (!savedWorkflow) { LoggerProxy.error('Failed to create workflow', { userId: req.user.id }); - throw new ResponseHelper.ResponseError('Failed to save workflow'); + throw new ResponseHelper.ResponseError( + 'An error occurred while saving your workflow. Please try again.', + ); } if (tagIds && !config.getEnv('workflowTagsDisabled') && savedWorkflow.tags) { diff --git a/packages/cli/src/workflows/workflows.controller.ts b/packages/cli/src/workflows/workflows.controller.ts index aef763854b..cb5418342f 100644 --- a/packages/cli/src/workflows/workflows.controller.ts +++ b/packages/cli/src/workflows/workflows.controller.ts @@ -223,7 +223,7 @@ workflowsController.get( userId: req.user.id, }); throw new ResponseHelper.ResponseError( - `Workflow with ID "${workflowId}" could not be found.`, + 'Could not load the workflow - you can only access workflows owned by you', undefined, 404, ); @@ -298,7 +298,7 @@ workflowsController.delete( userId: req.user.id, }); throw new ResponseHelper.ResponseError( - `Workflow with ID "${workflowId}" could not be found to be deleted.`, + 'Could not delete the workflow - you can only remove workflows owned by you', undefined, 400, ); diff --git a/packages/cli/src/workflows/workflows.services.ts b/packages/cli/src/workflows/workflows.services.ts index 42199c4361..d497ddecb6 100644 --- a/packages/cli/src/workflows/workflows.services.ts +++ b/packages/cli/src/workflows/workflows.services.ts @@ -197,7 +197,7 @@ export class WorkflowsService { userId: user.id, }); throw new ResponseHelper.ResponseError( - `Workflow with ID "${workflowId}" could not be found to be updated.`, + 'You do not have permission to update this workflow. Ask the owner to share it with you.', undefined, 404, ); @@ -205,7 +205,7 @@ export class WorkflowsService { if (!forceSave && workflow.hash !== '' && workflow.hash !== shared.workflow.hash) { throw new ResponseHelper.ResponseError( - `Workflow ID ${workflowId} cannot be saved because it was changed by another user.`, + 'We are sorry, but the workflow has been changed in the meantime. Please reload the workflow and try again.', undefined, 400, ); diff --git a/packages/cli/test/integration/workflows.controller.ee.test.ts b/packages/cli/test/integration/workflows.controller.ee.test.ts index 0a3c1f4390..8a9a249d95 100644 --- a/packages/cli/test/integration/workflows.controller.ee.test.ts +++ b/packages/cli/test/integration/workflows.controller.ee.test.ts @@ -471,7 +471,7 @@ describe('POST /workflows', () => { expect(response.statusCode).toBe(400); expect(response.body.message).toBe( - 'The workflow contains credentials that you do not have access to', + 'The workflow you are trying to save contains credentials that are not shared with you', ); }); @@ -760,7 +760,7 @@ describe('PATCH /workflows/:id - validate interim updates', () => { expect(updateAttemptResponse.status).toBe(400); expect(updateAttemptResponse.body.message).toContain( - 'cannot be saved because it was changed by another user', + 'the workflow has been changed in the meantime', ); }); @@ -802,7 +802,7 @@ describe('PATCH /workflows/:id - validate interim updates', () => { expect(updateAttemptResponse.status).toBe(400); expect(updateAttemptResponse.body.message).toContain( - 'cannot be saved because it was changed by another user', + 'the workflow has been changed in the meantime', ); }); @@ -832,7 +832,7 @@ describe('PATCH /workflows/:id - validate interim updates', () => { expect(activationAttemptResponse.status).toBe(400); expect(activationAttemptResponse.body.message).toContain( - 'cannot be saved because it was changed by another user', + 'the workflow has been changed in the meantime', ); }); @@ -871,7 +871,7 @@ describe('PATCH /workflows/:id - validate interim updates', () => { expect(updateAttemptResponse.status).toBe(400); expect(updateAttemptResponse.body.message).toContain( - 'cannot be saved because it was changed by another user', + 'the workflow has been changed in the meantime', ); }); @@ -906,7 +906,7 @@ describe('PATCH /workflows/:id - validate interim updates', () => { expect(updateAttemptResponse.status).toBe(400); expect(updateAttemptResponse.body.message).toContain( - 'cannot be saved because it was changed by another user', + 'the workflow has been changed in the meantime', ); }); @@ -941,7 +941,7 @@ describe('PATCH /workflows/:id - validate interim updates', () => { expect(updateAttemptResponse.status).toBe(400); expect(updateAttemptResponse.body.message).toContain( - 'cannot be saved because it was changed by another user', + 'the workflow has been changed in the meantime', ); }); });