Merge pull request #401 from sharelatex/ja-spam-code-fixes

Refactor rate limiting code around sending invites
This commit is contained in:
James Allen 2017-01-31 10:21:55 +01:00 committed by GitHub
commit e7efa40c75
7 changed files with 149 additions and 101 deletions

View file

@ -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,19 +31,20 @@ 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
if collabLimit == -1
collabLimit = 20
collabLimit = collabLimit * 10
opts =
endpointName: "invite_to_project"
timeInterval: 60 * 30
subjectName: sendingUser._id
throttle: collabLimit
rateLimiter.addCount opts, callback
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-by-user-id"
timeInterval: 60 * 30
subjectName: user_id
throttle: collabLimit
rateLimiter.addCount opts, callback
inviteToProject: (req, res, next) ->
projectId = req.params.Project_id
@ -64,20 +65,24 @@ 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)->
if err?
logger.err {err, email, projectId, sendingUserId}, "error checking if we can invite this email address"
return next(err)
if !shouldAllowInvite
logger.log {email, projectId, sendingUserId}, "not allowed to send an invite to this email address"
return res.json {invite: null, error: 'cannot_invite_non_user'}
CollaboratorsInviteHandler.inviteToProject projectId, sendingUser, email, privileges, (err, invite) ->
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 {projectId, email, sendingUserId}, "error creating project invite"
logger.err {err, email, projectId, sendingUserId}, "error checking if we can invite this email address"
return next(err)
logger.log {projectId, email, sendingUserId}, "invite created"
EditorRealTimeController.emitToRoom(projectId, 'project:membership:changed', {invites: true})
return res.json {invite: invite}
if !shouldAllowInvite
logger.log {email, projectId, sendingUserId}, "not allowed to send an invite to this email address"
return res.json {invite: null, error: 'cannot_invite_non_user'}
CollaboratorsInviteHandler.inviteToProject projectId, sendingUser, email, privileges, (err, invite) ->
if err?
logger.err {projectId, email, sendingUserId}, "error creating project invite"
return next(err)
logger.log {projectId, email, sendingUserId}, "invite created"
EditorRealTimeController.emitToRoom(projectId, 'project:membership:changed', {invites: true})
return res.json {invite: invite}
revokeInvite: (req, res, next) ->
projectId = req.params.Project_id

View file

@ -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)

View file

@ -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

View file

@ -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 =

View file

@ -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)->

View file

@ -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
@sendingUserId = "32312313"
@LimitationsManager.allowedNumberOfCollaboratorsForUser = sinon.stub()
@LimitationsManager.allowedNumberOfCollaboratorsForUser.withArgs(@sendingUserId).yields(null, 17)
beforeEach ->
@settings.restrictInvitesToExistingAccounts = false
@sendingUser =
_id:"32312313"
features:
collaborators:17.8
@UserGetter.getUser = sinon.stub().callsArgWith(2, null, @sendingUser)
it 'should callback with `true` when rate limit under', (done) ->
@RateLimiter.addCount = sinon.stub().callsArgWith(1, null, true)
@CollaboratorsInviteController._checkRateLimit @sendingUserId, (err, result)=>
@RateLimiter.addCount.called.should.equal true
result.should.equal true
done()
it 'should callback with `true` when rate limit under', (done) ->
@RateLimiter.addCount = sinon.stub().callsArgWith(1, null, true)
@CollaboratorsInviteController._checkShouldInviteEmail @sendingUser, @email, (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._checkRateLimit @sendingUserId, (err, result)=>
@RateLimiter.addCount.called.should.equal true
result.should.equal false
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)=>
@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._checkRateLimit @sendingUserId, (err, result)=>
@RateLimiter.addCount.args[0][0].throttle.should.equal(170)
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)
done()
it 'should call rate limiter with 200 when collaborators is -1', (done) ->
@LimitationsManager.allowedNumberOfCollaboratorsForUser.withArgs(@sendingUserId).yields(null, -1)
@RateLimiter.addCount = sinon.stub().callsArgWith(1, null, true)
@CollaboratorsInviteController._checkRateLimit @sendingUserId, (err, result)=>
@RateLimiter.addCount.args[0][0].throttle.should.equal(200)
done()
it 'should call rate limiter with 200 when collaborators is -1', (done) ->
@sendingUser.features.collaborators = -1
@RateLimiter.addCount = sinon.stub().callsArgWith(1, null, true)
@CollaboratorsInviteController._checkShouldInviteEmail @sendingUser, @email, (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
@RateLimiter.addCount = sinon.stub().callsArgWith(1, null, true)
@CollaboratorsInviteController._checkShouldInviteEmail @sendingUser, @email, (err, result)=>
@RateLimiter.addCount.args[0][0].throttle.should.equal(10)
done()
it 'should call rate limiter with 10 when user has no collaborators set', (done) ->
@LimitationsManager.allowedNumberOfCollaboratorsForUser.withArgs(@sendingUserId).yields(null)
@RateLimiter.addCount = sinon.stub().callsArgWith(1, null, true)
@CollaboratorsInviteController._checkRateLimit @sendingUserId, (err, result)=>
@RateLimiter.addCount.args[0][0].throttle.should.equal(10)
done()

View file

@ -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)