Refactor rate limiting code around sending invites

services/web/app/coffee/Features/Collaborators/CollaboratorsInviteController.coffee

@@ -23,7 +23,7 @@ module.exports = CollaboratorsInviteController =
 				return next(err)
 			res.json({invites: invites})
 
-	_checkShouldInviteEmail: (sendingUser, email, callback=(err, shouldAllowInvite)->) ->
+	_checkShouldInviteEmail: (email, callback=(err, shouldAllowInvite)->) ->
 		if Settings.restrictInvitesToExistingAccounts == true
 			logger.log {email}, "checking if user exists with this email"
 			UserGetter.getUser {email: email}, {_id: 1}, (err, user) ->
@@ -31,17 +31,18 @@ module.exports = CollaboratorsInviteController =
 				userExists = user? and user?._id?
 				callback(null, userExists)
 		else
-			UserGetter.getUser sendingUser._id, {features:1, _id:1}, (err, user)->
-				if err?
-					return callback(err)
-				collabLimit = user?.features?.collaborators || 1
+			callback(null, true)
+	
+	_checkRateLimit: (user_id, callback = (error) ->) ->
+		LimitationsManager.allowedNumberOfCollaboratorsForUser user_id, (err, collabLimit = 1)->
+			return callback(err) if err?
 			if collabLimit == -1 
 				collabLimit = 20
 			collabLimit = collabLimit * 10
 			opts = 
-					endpointName: "invite_to_project"
+				endpointName: "invite-to-project-by-user-id"
 				timeInterval: 60 * 30
-					subjectName: sendingUser._id
+				subjectName: user_id
 				throttle: collabLimit
 			rateLimiter.addCount opts, callback
 
@@ -64,7 +65,11 @@ module.exports = CollaboratorsInviteController =
 			if !email? or email == ""
 				logger.log {projectId, email, sendingUserId}, "invalid email address"
 				return res.sendStatus(400)
-			CollaboratorsInviteController._checkShouldInviteEmail sendingUser, email, (err, shouldAllowInvite)->
+			CollaboratorsInviteController._checkRateLimit sendingUserId, (error, underRateLimit) ->
+				return next(error) if error?
+				if !underRateLimit
+					return res.sendStatus(429)
+				CollaboratorsInviteController._checkShouldInviteEmail email, (err, shouldAllowInvite)->
 					if err?
 						logger.err {err, email, projectId, sendingUserId}, "error checking if we can invite this email address"
 						return next(err)

services/web/app/coffee/Features/Collaborators/CollaboratorsInviteHandler.coffee

@@ -80,7 +80,7 @@ module.exports = CollaboratorsInviteHandler =
 				# Send email and notification in background
 				CollaboratorsInviteHandler._sendMessages projectId, sendingUser, invite, (err) ->
 					if err?
-						logger.err {projectId, email}, "error sending messages for invite"
+						logger.err {err, projectId, email}, "error sending messages for invite"
 				callback(null, invite)
 
 

services/web/app/coffee/Features/Collaborators/CollaboratorsRouter.coffee

@@ -22,13 +22,13 @@ module.exports =
 		webRouter.post(
 			'/project/:Project_id/invite',
 			RateLimiterMiddlewear.rateLimit({
-				endpointName: "invite-to-project"
+				endpointName: "invite-to-project-by-project-id"
 				params: ["Project_id"]
 				maxRequests: 100
 				timeInterval: 60 * 10
 			}),
 			RateLimiterMiddlewear.rateLimit({
-				endpointName: "invite-to-project-ip"
+				endpointName: "invite-to-project-by-ip"
 				ipOnly:true
 				maxRequests: 100
 				timeInterval: 60 * 10

services/web/app/coffee/Features/Email/EmailBuilder.coffee

@@ -97,7 +97,7 @@ Thank you
 
 
 templates.projectInvite =
-	subject: _.template "<%= project.name.slice(0, 40) %> - shared by <%= owner.email %>"
+	subject: _.template "<%= project.name %> - shared by <%= owner.email %>"
 	layout: BaseWithHeaderEmailLayout
 	type:"notification"
 	plainTextTemplate: _.template """
@@ -111,16 +111,16 @@ Thank you
 """
 	compiledTemplate: (opts) -> 
 		SingleCTAEmailBody({
-			title: "#{ opts.project.name.slice(0, 40) } &ndash; shared by #{ opts.owner.email }"
+			title: "#{ opts.project.name } &ndash; shared by #{ opts.owner.email }"
 			greeting: "Hi,"
-			message: "#{ opts.owner.email } wants to share &ldquo;#{ opts.project.name.slice(0, 40) }&rdquo; with you."
+			message: "#{ opts.owner.email } wants to share &ldquo;#{ opts.project.name }&rdquo; with you."
 			secondaryMessage: null
 			ctaText: "View project"
 			ctaURL: opts.inviteUrl
 			gmailGoToAction: 
 				target: opts.inviteUrl
 				name: "View project"
-				description: "Join #{ opts.project.name.slice(0, 40) } at ShareLaTeX"
+				description: "Join #{ opts.project.name } at ShareLaTeX"
 		})
 
 templates.completeJoinGroupAccount =

services/web/app/coffee/Features/Subscription/LimitationsManager.coffee

@@ -1,21 +1,26 @@
 logger = require("logger-sharelatex")
 Project = require("../../models/Project").Project
-User = require("../../models/User").User
+UserGetter = require("../User/UserGetter")
 SubscriptionLocator = require("./SubscriptionLocator")
 Settings = require("settings-sharelatex")
 CollaboratorsHandler = require("../Collaborators/CollaboratorsHandler")
 CollaboratorsInvitesHandler = require("../Collaborators/CollaboratorsInviteHandler")
 
 module.exports =
-
 	allowedNumberOfCollaboratorsInProject: (project_id, callback) ->
-		getOwnerOfProject project_id, (error, owner)->
+		Project.findById project_id, 'owner_ref', (error, project) =>
 			return callback(error) if error?
-			if owner.features? and owner.features.collaborators?
-				callback null, owner.features.collaborators
+			@allowedNumberOfCollaboratorsForUser project.owner_ref, callback
+	
+	allowedNumberOfCollaboratorsForUser: (user_id, callback) ->
+		UserGetter.getUser user_id, {features: 1}, (error, user) ->
+			return callback(error) if error?
+			if user.features? and user.features.collaborators?
+				callback null, user.features.collaborators
 			else
 				callback null, Settings.defaultPlanCode.collaborators
 		
+
 	canAddXCollaborators: (project_id, x_collaborators, callback = (error, allowed)->) ->
 		@allowedNumberOfCollaboratorsInProject project_id, (error, allowed_number) =>
 			return callback(error) if error?
@@ -63,8 +68,4 @@ module.exports =
 			logger.log user_id:user_id, limitReached:limitReached, currentTotal: subscription.member_ids.length, membersLimit: subscription.membersLimit, "checking if subscription members limit has been reached"
 			callback(err, limitReached, subscription)
 
-getOwnerOfProject = (project_id, callback)->
-	Project.findById project_id, 'owner_ref', (error, project) ->
-		return callback(error) if error?
-		User.findById project.owner_ref, (error, owner) ->
-			callback(error, owner)
+getOwnerIdOfProject = (project_id, callback)->

services/web/test/UnitTests/coffee/Collaborators/CollaboratorsInviteControllerTests.coffee

@@ -114,7 +114,8 @@ describe "CollaboratorsInviteController", ->
 		describe 'when all goes well', ->
 
 			beforeEach ->
-				@CollaboratorsInviteController._checkShouldInviteEmail = sinon.stub().callsArgWith(2, null, true)
+				@CollaboratorsInviteController._checkShouldInviteEmail = sinon.stub().callsArgWith(1, null, true)
+				@CollaboratorsInviteController._checkRateLimit = sinon.stub().yields(null, true)
 				@LimitationsManager.canAddXCollaborators = sinon.stub().callsArgWith(2, null, true)
 				@CollaboratorsInviteController.inviteToProject @req, @res, @next
 
@@ -128,7 +129,7 @@ describe "CollaboratorsInviteController", ->
 
 			it 'should have called _checkShouldInviteEmail', ->
 				@CollaboratorsInviteController._checkShouldInviteEmail.callCount.should.equal 1
-				@CollaboratorsInviteController._checkShouldInviteEmail.calledWith(@sendingUser, @targetEmail).should.equal true
+				@CollaboratorsInviteController._checkShouldInviteEmail.calledWith(@targetEmail).should.equal true
 
 			it 'should have called inviteToProject', ->
 				@CollaboratorsInviteHandler.inviteToProject.callCount.should.equal 1
@@ -141,7 +142,8 @@ describe "CollaboratorsInviteController", ->
 		describe 'when the user is not allowed to add more collaborators', ->
 
 			beforeEach ->
-				@CollaboratorsInviteController._checkShouldInviteEmail = sinon.stub().callsArgWith(2, null, true)
+				@CollaboratorsInviteController._checkShouldInviteEmail = sinon.stub().callsArgWith(1, null, true)
+				@CollaboratorsInviteController._checkRateLimit = sinon.stub().yields(null, true)
 				@LimitationsManager.canAddXCollaborators = sinon.stub().callsArgWith(2, null, false)
 				@CollaboratorsInviteController.inviteToProject @req, @res, @next
 
@@ -159,7 +161,8 @@ describe "CollaboratorsInviteController", ->
 		describe 'when canAddXCollaborators produces an error', ->
 
 			beforeEach ->
-				@CollaboratorsInviteController._checkShouldInviteEmail = sinon.stub().callsArgWith(2, null, true)
+				@CollaboratorsInviteController._checkShouldInviteEmail = sinon.stub().callsArgWith(1, null, true)
+				@CollaboratorsInviteController._checkRateLimit = sinon.stub().yields(null, true)
 				@err = new Error('woops')
 				@LimitationsManager.canAddXCollaborators = sinon.stub().callsArgWith(2, @err)
 				@CollaboratorsInviteController.inviteToProject @req, @res, @next
@@ -178,7 +181,8 @@ describe "CollaboratorsInviteController", ->
 		describe 'when inviteToProject produces an error', ->
 
 			beforeEach ->
-				@CollaboratorsInviteController._checkShouldInviteEmail = sinon.stub().callsArgWith(2, null, true)
+				@CollaboratorsInviteController._checkShouldInviteEmail = sinon.stub().callsArgWith(1, null, true)
+				@CollaboratorsInviteController._checkRateLimit = sinon.stub().yields(null, true)
 				@err = new Error('woops')
 				@CollaboratorsInviteHandler.inviteToProject = sinon.stub().callsArgWith(4, @err)
 				@CollaboratorsInviteController.inviteToProject @req, @res, @next
@@ -193,7 +197,7 @@ describe "CollaboratorsInviteController", ->
 
 			it 'should have called _checkShouldInviteEmail', ->
 				@CollaboratorsInviteController._checkShouldInviteEmail.callCount.should.equal 1
-				@CollaboratorsInviteController._checkShouldInviteEmail.calledWith(@sendingUser, @targetEmail).should.equal true
+				@CollaboratorsInviteController._checkShouldInviteEmail.calledWith(@targetEmail).should.equal true
 
 			it 'should have called inviteToProject', ->
 				@CollaboratorsInviteHandler.inviteToProject.callCount.should.equal 1
@@ -202,7 +206,8 @@ describe "CollaboratorsInviteController", ->
 		describe 'when _checkShouldInviteEmail disallows the invite', ->
 
 			beforeEach ->
-				@CollaboratorsInviteController._checkShouldInviteEmail = sinon.stub().callsArgWith(2, null, false)
+				@CollaboratorsInviteController._checkShouldInviteEmail = sinon.stub().callsArgWith(1, null, false)
+				@CollaboratorsInviteController._checkRateLimit = sinon.stub().yields(null, true)
 				@LimitationsManager.canAddXCollaborators = sinon.stub().callsArgWith(2, null, true)
 				@CollaboratorsInviteController.inviteToProject @req, @res, @next
 
@@ -212,7 +217,7 @@ describe "CollaboratorsInviteController", ->
 
 			it 'should have called _checkShouldInviteEmail', ->
 				@CollaboratorsInviteController._checkShouldInviteEmail.callCount.should.equal 1
-				@CollaboratorsInviteController._checkShouldInviteEmail.calledWith(@sendingUser, @targetEmail).should.equal true
+				@CollaboratorsInviteController._checkShouldInviteEmail.calledWith(@targetEmail).should.equal true
 
 			it 'should not have called inviteToProject', ->
 				@CollaboratorsInviteHandler.inviteToProject.callCount.should.equal 0
@@ -220,7 +225,8 @@ describe "CollaboratorsInviteController", ->
 		describe 'when _checkShouldInviteEmail produces an error', ->
 
 			beforeEach ->
-				@CollaboratorsInviteController._checkShouldInviteEmail = sinon.stub().callsArgWith(2, new Error('woops'))
+				@CollaboratorsInviteController._checkShouldInviteEmail = sinon.stub().callsArgWith(1, new Error('woops'))
+				@CollaboratorsInviteController._checkRateLimit = sinon.stub().yields(null, true)
 				@LimitationsManager.canAddXCollaborators = sinon.stub().callsArgWith(2, null, true)
 				@CollaboratorsInviteController.inviteToProject @req, @res, @next
 
@@ -230,7 +236,7 @@ describe "CollaboratorsInviteController", ->
 
 			it 'should have called _checkShouldInviteEmail', ->
 				@CollaboratorsInviteController._checkShouldInviteEmail.callCount.should.equal 1
-				@CollaboratorsInviteController._checkShouldInviteEmail.calledWith(@sendingUser, @targetEmail).should.equal true
+				@CollaboratorsInviteController._checkShouldInviteEmail.calledWith(@targetEmail).should.equal true
 
 			it 'should not have called inviteToProject', ->
 				@CollaboratorsInviteHandler.inviteToProject.callCount.should.equal 0
@@ -240,7 +246,8 @@ describe "CollaboratorsInviteController", ->
 			beforeEach ->
 				@req.session.user = {_id: 'abc', email: 'me@example.com'}
 				@req.body.email = 'me@example.com'
-				@CollaboratorsInviteController._checkShouldInviteEmail = sinon.stub().callsArgWith(2, null, true)
+				@CollaboratorsInviteController._checkShouldInviteEmail = sinon.stub().callsArgWith(1, null, true)
+				@CollaboratorsInviteController._checkRateLimit = sinon.stub().yields(null, true)
 				@LimitationsManager.canAddXCollaborators = sinon.stub().callsArgWith(2, null, true)
 				@CollaboratorsInviteController.inviteToProject @req, @res, @next
 
@@ -261,6 +268,22 @@ describe "CollaboratorsInviteController", ->
 			it 'should not have called emitToRoom', ->
 				@EditorRealTimeController.emitToRoom.callCount.should.equal 0
 
+		describe 'when _checkRateLimit returns false', ->
+
+			beforeEach ->
+				@CollaboratorsInviteController._checkShouldInviteEmail = sinon.stub().callsArgWith(1, null, true)
+				@CollaboratorsInviteController._checkRateLimit = sinon.stub().yields(null, false)
+				@LimitationsManager.canAddXCollaborators = sinon.stub().callsArgWith(2, null, true)
+				@CollaboratorsInviteController.inviteToProject @req, @res, @next
+
+			it 'should send a 429 response', ->
+				@res.sendStatus.calledWith(429).should.equal true
+
+			it 'should not call inviteToProject', ->
+				@CollaboratorsInviteHandler.inviteToProject.called.should.equal false
+
+			it 'should not call emitToRoom', ->
+				@EditorRealTimeController.emitToRoom.called.should.equal false
 
 	describe "viewInvite", ->
 
@@ -679,13 +702,12 @@ describe "CollaboratorsInviteController", ->
 		beforeEach ->
 			@email = 'user@example.com'
 
-
 		describe 'when we should be restricting to existing accounts', ->
 
 			beforeEach ->
 				@settings.restrictInvitesToExistingAccounts = true
 				@call = (callback) =>
-					@CollaboratorsInviteController._checkShouldInviteEmail {}, @email, callback
+					@CollaboratorsInviteController._checkShouldInviteEmail @email, callback
 
 			describe 'when user account is present', ->
 
@@ -730,46 +752,43 @@ describe "CollaboratorsInviteController", ->
 						expect(shouldAllow).to.equal undefined
 						done()
 
-		describe 'when we should not be restricting on only registered users but do rate limit', ->
-
+	describe '_checkRateLimit', ->
 		beforeEach ->
 			@settings.restrictInvitesToExistingAccounts = false
-				@sendingUser = 
-					_id:"32312313"
-					features:
-						collaborators:17.8
-				@UserGetter.getUser = sinon.stub().callsArgWith(2, null, @sendingUser)
+			@sendingUserId = "32312313"
+			@LimitationsManager.allowedNumberOfCollaboratorsForUser = sinon.stub()
+			@LimitationsManager.allowedNumberOfCollaboratorsForUser.withArgs(@sendingUserId).yields(null, 17)
 
 		it 'should callback with `true` when rate limit under', (done) ->
 			@RateLimiter.addCount = sinon.stub().callsArgWith(1, null, true)
-				@CollaboratorsInviteController._checkShouldInviteEmail @sendingUser, @email, (err, result)=>
+			@CollaboratorsInviteController._checkRateLimit @sendingUserId, (err, result)=>
 				@RateLimiter.addCount.called.should.equal true
 				result.should.equal true
 				done()
 
 		it 'should callback with `false` when rate limit hit', (done) ->
 			@RateLimiter.addCount = sinon.stub().callsArgWith(1, null, false)
-				@CollaboratorsInviteController._checkShouldInviteEmail @sendingUser, @email, (err, result)=>
+			@CollaboratorsInviteController._checkRateLimit @sendingUserId, (err, result)=>
 				@RateLimiter.addCount.called.should.equal true
 				result.should.equal false
 				done()
 	
 		it 'should call rate limiter with 10x the collaborators', (done) ->
 			@RateLimiter.addCount = sinon.stub().callsArgWith(1, null, true)
-				@CollaboratorsInviteController._checkShouldInviteEmail @sendingUser, @email, (err, result)=>
-					@RateLimiter.addCount.args[0][0].throttle.should.equal(178)
+			@CollaboratorsInviteController._checkRateLimit @sendingUserId, (err, result)=>
+				@RateLimiter.addCount.args[0][0].throttle.should.equal(170)
 				done()
 
 		it 'should call rate limiter with 200 when collaborators is -1', (done) ->
-				@sendingUser.features.collaborators = -1
+			@LimitationsManager.allowedNumberOfCollaboratorsForUser.withArgs(@sendingUserId).yields(null, -1)
 			@RateLimiter.addCount = sinon.stub().callsArgWith(1, null, true)
-				@CollaboratorsInviteController._checkShouldInviteEmail @sendingUser, @email, (err, result)=>
+			@CollaboratorsInviteController._checkRateLimit @sendingUserId, (err, result)=>
 				@RateLimiter.addCount.args[0][0].throttle.should.equal(200)
 				done()
 
 		it 'should call rate limiter with 10 when user has no collaborators set', (done) ->
-				delete @sendingUser.features
+			@LimitationsManager.allowedNumberOfCollaboratorsForUser.withArgs(@sendingUserId).yields(null)
 			@RateLimiter.addCount = sinon.stub().callsArgWith(1, null, true)
-				@CollaboratorsInviteController._checkShouldInviteEmail @sendingUser, @email, (err, result)=>
+			@CollaboratorsInviteController._checkRateLimit @sendingUserId, (err, result)=>
 				@RateLimiter.addCount.args[0][0].throttle.should.equal(10)
 				done()

services/web/test/UnitTests/coffee/Subscription/LimitationsManagerTests.coffee

@@ -6,17 +6,17 @@ Settings = require("settings-sharelatex")
 
 describe "LimitationsManager", ->
 	beforeEach ->
-		@project = { _id: "project-id" }
-		@user = { _id: "user-id", features:{} }
+		@project = { _id: @project_id = "project-id" }
+		@user = { _id: @user_id = "user-id", features:{} }
 		@Project =
 			findById: (project_id, fields, callback) =>
 				if project_id == @project_id
 					callback null, @project
 				else
 					callback null, null
-		@User =
-			findById: (user_id, callback) =>
-				if user_id == @user.id
+		@UserGetter =
+			getUser: (user_id, filter, callback) =>
+				if user_id == @user_id
 					callback null, @user
 				else
 					callback null, null
@@ -26,7 +26,7 @@ describe "LimitationsManager", ->
 
 		@LimitationsManager = SandboxedModule.require modulePath, requires:
 			'../../models/Project' : Project: @Project
-			'../../models/User' : User: @User
+			'../User/UserGetter' : @UserGetter
 			'./SubscriptionLocator':@SubscriptionLocator
 			'settings-sharelatex' : @Settings = {}
 			"../Collaborators/CollaboratorsHandler": @CollaboratorsHandler = {}
@@ -37,6 +37,7 @@ describe "LimitationsManager", ->
 		describe "when the project is owned by a user without a subscription", ->
 			beforeEach ->
 				@Settings.defaultPlanCode = collaborators: 23
+				@project.owner_ref = @user_id
 				delete @user.features
 				@callback = sinon.stub()
 				@LimitationsManager.allowedNumberOfCollaboratorsInProject(@project_id, @callback)
@@ -46,6 +47,7 @@ describe "LimitationsManager", ->
 
 		describe "when the project is owned by a user with a subscription", ->
 			beforeEach ->
+				@project.owner_ref = @user_id
 				@user.features =
 					collaborators: 21
 				@callback = sinon.stub()
@@ -54,6 +56,27 @@ describe "LimitationsManager", ->
 			it "should return the number of collaborators the user is allowed", ->
 				@callback.calledWith(null, @user.features.collaborators).should.equal true
 	
+	describe "allowedNumberOfCollaboratorsForUser", ->
+		describe "when the user has no features", ->
+			beforeEach ->
+				@Settings.defaultPlanCode = collaborators: 23
+				delete @user.features
+				@callback = sinon.stub()
+				@LimitationsManager.allowedNumberOfCollaboratorsForUser(@user_id, @callback)
+
+			it "should return the default number", ->
+				@callback.calledWith(null, @Settings.defaultPlanCode.collaborators).should.equal true
+
+		describe "when the user has features", ->
+			beforeEach ->
+				@user.features =
+					collaborators: 21
+				@callback = sinon.stub()
+				@LimitationsManager.allowedNumberOfCollaboratorsForUser(@user_id, @callback)
+
+			it "should return the number of collaborators the user is allowed", ->
+				@callback.calledWith(null, @user.features.collaborators).should.equal true
+
 	describe "canAddXCollaborators", ->
 		beforeEach ->
 			@CollaboratorsHandler.getCollaboratorCount = (project_id, callback) => callback(null, @current_number)