From fed88504f8f95f509b1e45447b69540f1143e219 Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Sat, 14 Jan 2017 14:52:32 +0000 Subject: [PATCH] rate limit emails sent sharing projects by users --- .../CollaboratorsEmailHandler.coffee | 3 +- .../CollaboratorsInviteHandler.coffee | 2 +- .../coffee/Features/Email/EmailSender.coffee | 49 ++++++++++++------- .../CollaboratorsInviteHandlerTests.coffee | 4 +- .../coffee/Email/EmailSenderTests.coffee | 30 ++++++++++++ 5 files changed, 67 insertions(+), 21 deletions(-) diff --git a/services/web/app/coffee/Features/Collaborators/CollaboratorsEmailHandler.coffee b/services/web/app/coffee/Features/Collaborators/CollaboratorsEmailHandler.coffee index bc7eb90c3f..913562f417 100644 --- a/services/web/app/coffee/Features/Collaborators/CollaboratorsEmailHandler.coffee +++ b/services/web/app/coffee/Features/Collaborators/CollaboratorsEmailHandler.coffee @@ -11,7 +11,7 @@ module.exports = CollaboratorsEmailHandler = "user_first_name=#{encodeURIComponent(project.owner_ref.first_name)}" ].join("&") - notifyUserOfProjectInvite: (project_id, email, invite, callback)-> + notifyUserOfProjectInvite: (project_id, email, invite, sendingUser, callback)-> Project .findOne(_id: project_id ) .select("name owner_ref") @@ -24,4 +24,5 @@ module.exports = CollaboratorsEmailHandler = name: project.name inviteUrl: CollaboratorsEmailHandler._buildInviteUrl(project, invite) owner: project.owner_ref + sendingUser_id: sendingUser._id EmailHandler.sendEmail "projectInvite", emailOptions, callback diff --git a/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteHandler.coffee b/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteHandler.coffee index 5ed6570c3a..0e6cd8876c 100644 --- a/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteHandler.coffee +++ b/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteHandler.coffee @@ -53,7 +53,7 @@ module.exports = CollaboratorsInviteHandler = _sendMessages: (projectId, sendingUser, invite, callback=(err)->) -> logger.log {projectId, inviteId: invite._id}, "sending notification and email for invite" - CollaboratorsEmailHandler.notifyUserOfProjectInvite projectId, invite.email, invite, (err)-> + CollaboratorsEmailHandler.notifyUserOfProjectInvite projectId, invite.email, invite, sendingUser, (err)-> return callback(err) if err? CollaboratorsInviteHandler._trySendInviteNotification projectId, sendingUser, invite, (err)-> return callback(err) if err? diff --git a/services/web/app/coffee/Features/Email/EmailSender.coffee b/services/web/app/coffee/Features/Email/EmailSender.coffee index a7bcc82ed7..7a909e083e 100644 --- a/services/web/app/coffee/Features/Email/EmailSender.coffee +++ b/services/web/app/coffee/Features/Email/EmailSender.coffee @@ -4,7 +4,7 @@ Settings = require('settings-sharelatex') nodemailer = require("nodemailer") sesTransport = require('nodemailer-ses-transport') sgTransport = require('nodemailer-sendgrid-transport') - +rateLimiter = require('../../infrastructure/RateLimiter') _ = require("underscore") if Settings.email? and Settings.email.fromAddress? @@ -39,24 +39,39 @@ if nm_client? else logger.warn "Failed to create email transport. Please check your settings. No email will be sent." +checkCanSendEmail = (options, callback)-> + if !options.sendingUser_id? #email not sent from user, not rate limited + callback(null, true) + opts = + endpointName: "send_email" + timeInterval: 60 * 60 * 3 + subjectName: options.sendingUser_id + throttle: 100 + rateLimiter.addCount opts, callback module.exports = sendEmail : (options, callback = (error) ->)-> logger.log receiver:options.to, subject:options.subject, "sending email" - metrics.inc "email" - options = - to: options.to - from: defaultFromAddress - subject: options.subject - html: options.html - text: options.text - replyTo: options.replyTo || Settings.email.replyToAddress - socketTimeout: 30 * 1000 - if Settings.email.textEncoding? - opts.textEncoding = textEncoding - client.sendMail options, (err, res)-> + checkCanSendEmail options, (err, canContinue)-> if err? - logger.err err:err, "error sending message" - else - logger.log "Message sent to #{options.to}" - callback(err) + return callback(err) + if !canContinue + logger.log sendingUser_id:options.sendingUser_id, to:options.to, subject:options.subject, canContinue:canContinue, "rate limit hit for sending email, not sending" + return callback("rate limit hit sending email") + metrics.inc "email" + options = + to: options.to + from: defaultFromAddress + subject: options.subject + html: options.html + text: options.text + replyTo: options.replyTo || Settings.email.replyToAddress + socketTimeout: 30 * 1000 + if Settings.email.textEncoding? + opts.textEncoding = textEncoding + client.sendMail options, (err, res)-> + if err? + logger.err err:err, "error sending message" + else + logger.log "Message sent to #{options.to}" + callback(err) diff --git a/services/web/test/UnitTests/coffee/Collaborators/CollaboratorsInviteHandlerTests.coffee b/services/web/test/UnitTests/coffee/Collaborators/CollaboratorsInviteHandlerTests.coffee index ac94fcf10d..177c42d4ba 100644 --- a/services/web/test/UnitTests/coffee/Collaborators/CollaboratorsInviteHandlerTests.coffee +++ b/services/web/test/UnitTests/coffee/Collaborators/CollaboratorsInviteHandlerTests.coffee @@ -185,7 +185,7 @@ describe "CollaboratorsInviteHandler", -> describe '_sendMessages', -> beforeEach -> - @CollaboratorsEmailHandler.notifyUserOfProjectInvite = sinon.stub().callsArgWith(3, null) + @CollaboratorsEmailHandler.notifyUserOfProjectInvite = sinon.stub().callsArgWith(4, null) @CollaboratorsInviteHandler._trySendInviteNotification = sinon.stub().callsArgWith(3, null) @call = (callback) => @CollaboratorsInviteHandler._sendMessages @projectId, @sendingUser, @fakeInvite, callback @@ -213,7 +213,7 @@ describe "CollaboratorsInviteHandler", -> describe 'when CollaboratorsEmailHandler.notifyUserOfProjectInvite produces an error', -> beforeEach -> - @CollaboratorsEmailHandler.notifyUserOfProjectInvite = sinon.stub().callsArgWith(3, new Error('woops')) + @CollaboratorsEmailHandler.notifyUserOfProjectInvite = sinon.stub().callsArgWith(4, new Error('woops')) it 'should produce an error', (done) -> @call (err, invite) => diff --git a/services/web/test/UnitTests/coffee/Email/EmailSenderTests.coffee b/services/web/test/UnitTests/coffee/Email/EmailSenderTests.coffee index 4f08dd6790..beba91fcb3 100644 --- a/services/web/test/UnitTests/coffee/Email/EmailSenderTests.coffee +++ b/services/web/test/UnitTests/coffee/Email/EmailSenderTests.coffee @@ -10,6 +10,9 @@ describe "EmailSender", -> beforeEach -> + @RateLimiter = + addCount:sinon.stub() + @settings = email: transport: "ses" @@ -21,11 +24,15 @@ describe "EmailSender", -> @sesClient = sendMail: sinon.stub() + @ses = createTransport: => @sesClient + + @sender = SandboxedModule.require modulePath, requires: 'nodemailer': @ses "settings-sharelatex":@settings + '../../infrastructure/RateLimiter':@RateLimiter "logger-sharelatex": log:-> warn:-> @@ -84,6 +91,29 @@ describe "EmailSender", -> args.replyTo.should.equal @opts.replyTo done() + + it "should not send an email when the rate limiter says no", (done)-> + @opts.sendingUser_id = "12321312321" + @RateLimiter.addCount.callsArgWith(1, null, false) + @sender.sendEmail @opts, => + @sesClient.sendMail.called.should.equal false + done() + + it "should send the email when the rate limtier says continue", (done)-> + @sesClient.sendMail.callsArgWith(1) + @opts.sendingUser_id = "12321312321" + @RateLimiter.addCount.callsArgWith(1, null, true) + @sender.sendEmail @opts, => + @sesClient.sendMail.called.should.equal true + done() + + it "should not check the rate limiter when there is no sendingUser_id", (done)-> + @sesClient.sendMail.callsArgWith(1) + @sender.sendEmail @opts, => + @sesClient.sendMail.called.should.equal true + @RateLimiter.addCount.called.should.equal false + done() + describe 'with plain-text email content', () -> beforeEach ->