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

feat(core): Email recipients on resource shared #8408

Merged
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
14 changes: 12 additions & 2 deletions packages/cli/src/InternalHooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -648,7 +648,12 @@ export class InternalHooks {

async onUserTransactionalEmail(userTransactionalEmailData: {
user_id: string;
message_type: 'Reset password' | 'New user invite' | 'Resend invite';
message_type:
| 'Reset password'
| 'New user invite'
| 'Resend invite'
| 'Workflow shared'
| 'Credentials shared';
public_api: boolean;
}): Promise<void> {
return await this.telemetry.track(
Expand Down Expand Up @@ -737,7 +742,12 @@ export class InternalHooks {

async onEmailFailed(failedEmailData: {
user: User;
message_type: 'Reset password' | 'New user invite' | 'Resend invite';
message_type:
| 'Reset password'
| 'New user invite'
| 'Resend invite'
| 'Workflow shared'
| 'Credentials shared';
public_api: boolean;
}): Promise<void> {
void Promise.all([
Expand Down
48 changes: 47 additions & 1 deletion packages/cli/src/UserManagement/email/UserManagementMailer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { NodeMailer } from './NodeMailer';
import { ApplicationError } from 'n8n-workflow';

type Template = HandlebarsTemplateDelegate<unknown>;
type TemplateName = 'invite' | 'passwordReset';
type TemplateName = 'invite' | 'passwordReset' | 'workflowShared' | 'credentialsShared';

const templates: Partial<Record<TemplateName, Template>> = {};

Expand Down Expand Up @@ -81,4 +81,50 @@ export class UserManagementMailer {
// No error, just say no email was sent.
return result ?? { emailSent: false };
}

async notifyWorkflowShared({
recipientEmails,
workflowName,
baseUrl,
workflowId,
sharerFirstName,
}: {
recipientEmails: string[];
workflowName: string;
baseUrl: string;
workflowId: string;
sharerFirstName: string;
}) {
const populateTemplate = await getTemplate('workflowShared', 'workflowShared.html');

const result = await this.mailer?.sendMail({
emailRecipients: recipientEmails,
subject: `${sharerFirstName} has shared an n8n workflow with you`,
body: populateTemplate({ workflowName, workflowUrl: `${baseUrl}/workflow/${workflowId}` }),
});

return result ?? { emailSent: false };
}

async notifyCredentialsShared({
sharerFirstName,
credentialsName,
recipientEmails,
baseUrl,
}: {
sharerFirstName: string;
credentialsName: string;
recipientEmails: string[];
baseUrl: string;
}) {
const populateTemplate = await getTemplate('credentialsShared', 'credentialsShared.html');

const result = await this.mailer?.sendMail({
emailRecipients: recipientEmails,
subject: `${sharerFirstName} has shared an n8n credential with you`,
body: populateTemplate({ credentialsName, credentialsListUrl: `${baseUrl}/credentials` }),
});

return result ?? { emailSent: false };
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<p>Hi there,</p>
<p><b>"{{ credentialsName }}" credential</b> has been shared with you.</p>
<p>To view all the credentials you have access to within n8n, click the following link:</p>
<p><a href="{{ credentialsListUrl }}" target="_blank">{{ credentialsListUrl }}</a></p>
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<p>Hi there,</p>
<p><b>"{{ workflowName }}" workflow</b> has been shared with you.</p>
<p>To access the workflow, click the following link:</p>
<p><a href="{{ workflowUrl }}" target="_blank">{{ workflowUrl }}</a></p>
12 changes: 12 additions & 0 deletions packages/cli/src/config/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -846,6 +846,18 @@ export const schema = {
default: '',
env: 'N8N_UM_EMAIL_TEMPLATES_PWRESET',
},
workflowShared: {
doc: 'Overrides default HTML template for notifying that a workflow was shared (use full path)',
format: String,
default: '',
env: 'N8N_UM_EMAIL_TEMPLATES_WORKFLOW_SHARED',
},
credentialsShared: {
doc: 'Overrides default HTML template for notifying that credentials were shared (use full path)',
format: String,
default: '',
env: 'N8N_UM_EMAIL_TEMPLATES_CREDENTIALS_SHARED',
},
Comment on lines +849 to +860
Copy link
Member

Choose a reason for hiding this comment

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

I've always wondered why we do it this way. can we not ask users to overwrite the files by using docker volumes, or even extending the official images with custom templates.
I believe that the burden of customization related code should lie on the party making those customizations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maspio Do you happen to know if embed customers usually need to customize email templates?

},
},
authenticationMethod: {
Expand Down
37 changes: 37 additions & 0 deletions packages/cli/src/credentials/credentials.controller.ee.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ import { NotFoundError } from '@/errors/response-errors/not-found.error';
import { UnauthorizedError } from '@/errors/response-errors/unauthorized.error';
import { CredentialsRepository } from '@/databases/repositories/credentials.repository';
import * as utils from '@/utils';
import { UserRepository } from '@/databases/repositories/user.repository';
import { UserManagementMailer } from '@/UserManagement/email';
import { UrlService } from '@/services/url.service';
import { Logger } from '@/Logger';
import { InternalServerError } from '@/errors/response-errors/internal-server.error';

export const EECredentialsController = express.Router();

Expand Down Expand Up @@ -185,5 +190,37 @@ EECredentialsController.put(
user_ids_sharees_added: newShareeIds,
sharees_removed: amountRemoved,
});

const recipients = await Container.get(UserRepository).getEmailsByIds(newShareeIds);

if (recipients.length === 0) return;

try {
await Container.get(UserManagementMailer).notifyCredentialsShared({
sharerFirstName: req.user.firstName,
credentialsName: credential.name,
recipientEmails: recipients.map(({ email }) => email),
baseUrl: Container.get(UrlService).getInstanceBaseUrl(),
});
} catch (error) {
void Container.get(InternalHooks).onEmailFailed({
user: req.user,
message_type: 'Credentials shared',
public_api: false,
});
if (error instanceof Error) {
throw new InternalServerError(`Please contact your administrator: ${error.message}`);
}
}

Container.get(Logger).info('Sent credentials shared email successfully', {
sharerId: req.user.id,
});

void Container.get(InternalHooks).onUserTransactionalEmail({
user_id: req.user.id,
message_type: 'Credentials shared',
public_api: false,
});
}),
);
10 changes: 10 additions & 0 deletions packages/cli/src/databases/repositories/user.repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,4 +84,14 @@ export class UserRepository extends Repository<User> {

return findManyOptions;
}

/**
* Get emails of users who have completed setup, by user IDs.
*/
async getEmailsByIds(userIds: string[]) {
return await this.find({
select: ['email'],
where: { id: In(userIds), password: Not(IsNull()) },
});
}
}
35 changes: 35 additions & 0 deletions packages/cli/src/workflows/workflows.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ import { WorkflowRequest } from './workflow.request';
import { EnterpriseWorkflowService } from './workflow.service.ee';
import { WorkflowExecutionService } from './workflowExecution.service';
import { WorkflowSharingService } from './workflowSharing.service';
import { UserManagementMailer } from '@/UserManagement/email';
import { UrlService } from '@/services/url.service';

@Service()
@Authorized()
Expand All @@ -62,6 +64,8 @@ export class WorkflowsController {
private readonly workflowSharingService: WorkflowSharingService,
private readonly sharedWorkflowRepository: SharedWorkflowRepository,
private readonly userRepository: UserRepository,
private readonly mailer: UserManagementMailer,
private readonly urlService: UrlService,
) {}

@Post('/')
Expand Down Expand Up @@ -401,5 +405,36 @@ export class WorkflowsController {
});

void this.internalHooks.onWorkflowSharingUpdate(workflowId, req.user.id, shareWithIds);

const recipients = await this.userRepository.getEmailsByIds(newShareeIds);

if (recipients.length === 0) return;

try {
await this.mailer.notifyWorkflowShared({
recipientEmails: recipients.map(({ email }) => email),
workflowName: workflow.name,
workflowId,
sharerFirstName: req.user.firstName,
baseUrl: this.urlService.getInstanceBaseUrl(),
});
} catch (error) {
void this.internalHooks.onEmailFailed({
user: req.user,
message_type: 'Workflow shared',
public_api: false,
});
if (error instanceof Error) {
throw new InternalServerError(`Please contact your administrator: ${error.message}`);
}
}

this.logger.info('Sent workflow shared email successfully', { sharerId: req.user.id });

void this.internalHooks.onUserTransactionalEmail({
user_id: req.user.id,
message_type: 'Workflow shared',
public_api: false,
});
}
}
19 changes: 19 additions & 0 deletions packages/cli/test/integration/credentials.ee.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import { getCredentialOwnerRole, getGlobalMemberRole, getGlobalOwnerRole } from
import { createManyUsers, createUser, createUserShell } from './shared/db/users';
import { SharedCredentialsRepository } from '@db/repositories/sharedCredentials.repository';
import Container from 'typedi';
import { mockInstance } from '../shared/mocking';
import { UserManagementMailer } from '@/UserManagement/email';

const sharingSpy = jest.spyOn(UserManagementHelpers, 'isSharingEnabled').mockReturnValue(true);
const testServer = utils.setupTestServer({ endpointGroups: ['credentials'] });
Expand All @@ -27,6 +29,7 @@ let anotherMember: User;
let authOwnerAgent: SuperAgentTest;
let authAnotherMemberAgent: SuperAgentTest;
let saveCredential: SaveCredentialFunction;
const mailer = mockInstance(UserManagementMailer);

beforeAll(async () => {
const globalOwnerRole = await getGlobalOwnerRole();
Expand All @@ -47,6 +50,10 @@ beforeEach(async () => {
await testDb.truncate(['SharedCredentials', 'Credentials']);
});

afterEach(() => {
jest.clearAllMocks();
});

// ----------------------------------------
// dynamic router switching
// ----------------------------------------
Expand Down Expand Up @@ -363,6 +370,8 @@ describe('PUT /credentials/:id/share', () => {
expect(sharedCredential.role.name).toBe('user');
expect(sharedCredential.role.scope).toBe('credential');
});

expect(mailer.notifyCredentialsShared).toHaveBeenCalledTimes(1);
});

test('should share the credential with the provided userIds', async () => {
Expand Down Expand Up @@ -400,6 +409,7 @@ describe('PUT /credentials/:id/share', () => {

expect(ownerSharedCredential.role.name).toBe('owner');
expect(ownerSharedCredential.role.scope).toBe('credential');
expect(mailer.notifyCredentialsShared).toHaveBeenCalledTimes(1);
});

test('should respond 403 for non-existing credentials', async () => {
Expand All @@ -408,6 +418,7 @@ describe('PUT /credentials/:id/share', () => {
.send({ shareWithIds: [member.id] });

expect(response.statusCode).toBe(403);
expect(mailer.notifyCredentialsShared).toHaveBeenCalledTimes(0);
});

test('should respond 403 for non-owned credentials for shared members', async () => {
Expand All @@ -424,6 +435,7 @@ describe('PUT /credentials/:id/share', () => {
where: { credentialsId: savedCredential.id },
});
expect(sharedCredentials).toHaveLength(2);
expect(mailer.notifyCredentialsShared).toHaveBeenCalledTimes(0);
});

test('should respond 403 for non-owned credentials for non-shared members sharing with self', async () => {
Expand All @@ -439,6 +451,7 @@ describe('PUT /credentials/:id/share', () => {
where: { credentialsId: savedCredential.id },
});
expect(sharedCredentials).toHaveLength(1);
expect(mailer.notifyCredentialsShared).toHaveBeenCalledTimes(0);
});

test('should respond 403 for non-owned credentials for non-shared members sharing', async () => {
Expand All @@ -455,6 +468,7 @@ describe('PUT /credentials/:id/share', () => {
where: { credentialsId: savedCredential.id },
});
expect(sharedCredentials).toHaveLength(1);
expect(mailer.notifyCredentialsShared).toHaveBeenCalledTimes(0);
});

test('should respond 200 for non-owned credentials for owners', async () => {
Expand All @@ -469,6 +483,7 @@ describe('PUT /credentials/:id/share', () => {
where: { credentialsId: savedCredential.id },
});
expect(sharedCredentials).toHaveLength(2);
expect(mailer.notifyCredentialsShared).toHaveBeenCalledTimes(1);
});

test('should ignore pending sharee', async () => {
Expand All @@ -487,6 +502,7 @@ describe('PUT /credentials/:id/share', () => {

expect(sharedCredentials).toHaveLength(1);
expect(sharedCredentials[0].userId).toBe(owner.id);
expect(mailer.notifyCredentialsShared).toHaveBeenCalledTimes(0);
});

test('should ignore non-existing sharee', async () => {
Expand All @@ -504,6 +520,7 @@ describe('PUT /credentials/:id/share', () => {

expect(sharedCredentials).toHaveLength(1);
expect(sharedCredentials[0].userId).toBe(owner.id);
expect(mailer.notifyCredentialsShared).toHaveBeenCalledTimes(0);
});

test('should respond 400 if invalid payload is provided', async () => {
Expand All @@ -515,6 +532,7 @@ describe('PUT /credentials/:id/share', () => {
]);

responses.forEach((response) => expect(response.statusCode).toBe(400));
expect(mailer.notifyCredentialsShared).toHaveBeenCalledTimes(0);
});
test('should unshare the credential', async () => {
const savedCredential = await saveCredential(randomCredentialPayload(), { user: owner });
Expand All @@ -537,6 +555,7 @@ describe('PUT /credentials/:id/share', () => {

expect(sharedCredentials).toHaveLength(1);
expect(sharedCredentials[0].userId).toBe(owner.id);
expect(mailer.notifyCredentialsShared).toHaveBeenCalledTimes(0);
});
});

Expand Down
Loading
Loading