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.

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

refactor: Improve warnings and error messages to users about sharing #4687

Merged
merged 2 commits into from
Nov 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 9 additions & 6 deletions packages/cli/src/UserManagement/routes/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/credentials/credentials.controller.ee.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
Expand Down
4 changes: 2 additions & 2 deletions packages/cli/src/credentials/credentials.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
Expand Down Expand Up @@ -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,
);
Expand Down
14 changes: 10 additions & 4 deletions packages/cli/src/workflows/workflows.controller.ee.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
Expand All @@ -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(
Expand Down Expand Up @@ -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,
);
Expand Down Expand Up @@ -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) {
Expand Down
4 changes: 2 additions & 2 deletions packages/cli/src/workflows/workflows.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
Expand Down Expand Up @@ -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,
);
Expand Down
4 changes: 2 additions & 2 deletions packages/cli/src/workflows/workflows.services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -197,15 +197,15 @@ 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,
);
}

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,
);
Expand Down
14 changes: 7 additions & 7 deletions packages/cli/test/integration/workflows.controller.ee.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
);
});

Expand Down Expand Up @@ -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',
);
});

Expand Down Expand Up @@ -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',
);
});

Expand Down Expand Up @@ -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',
);
});

Expand Down Expand Up @@ -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',
);
});

Expand Down Expand Up @@ -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',
);
});

Expand Down Expand Up @@ -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',
);
});
});