Make LoginRateLimiter a thin wrapper around RateLimiter

services/web/app/coffee/Features/Security/LoginRateLimiter.coffee

@@ -1,23 +1,21 @@
-Settings = require('settings-sharelatex')
+RateLimiter = require('../../infrastructure/RateLimiter')
-redis = require("redis-sharelatex")
-rclient = redis.createClient(Settings.redis.web)
 
-buildKey = (k)->
-	return "LoginRateLimit:#{k}"
 
 ONE_MIN = 60
 ATTEMPT_LIMIT = 10
 
+
 module.exports =
-	processLoginRequest: (email, callback)->
+
-		multi = rclient.multi()
+	processLoginRequest: (email, callback) ->
-		multi.incr(buildKey(email))
+		opts =
-		multi.get(buildKey(email))
+			endpointName: 'login'
-		multi.expire(buildKey(email), ONE_MIN * 2)
+			throttle: ATTEMPT_LIMIT
-		multi.exec (err, results)->
+			timeInterval: ONE_MIN * 2
-			loginCount = results[1]
+			subjectName: email
-			allow = loginCount <= ATTEMPT_LIMIT
+		RateLimiter.addCount opts, (err, shouldAllow) ->
-			callback err, allow
+			callback(err, shouldAllow)
 
 	recordSuccessfulLogin: (email, callback = ->)->
-		rclient.del buildKey(email), callback
+		RateLimiter.clearRateLimit 'login', email, callback
+

services/web/test/UnitTests/coffee/Security/LoginRateLimiterTests.coffee

@@ -1,78 +1,74 @@
 SandboxedModule = require('sandboxed-module')
 sinon = require('sinon')
 require('chai').should()
+expect = require('chai').expect
 modulePath = require('path').join __dirname, '../../../../app/js/Features/Security/LoginRateLimiter'
 
-buildKey = (k)->
-	return "LoginRateLimit:#{k}"
 
 describe "LoginRateLimiter", ->
+
 	beforeEach ->
 		@email = "bob@bob.com"
-		@incrStub = sinon.stub()
+		@RateLimiter =
-		@getStub = sinon.stub()
+			clearRateLimit: sinon.stub()
-		@execStub = sinon.stub()
+			addCount: sinon.stub()
-		@expireStub = sinon.stub()
-		@delStub = sinon.stub().callsArgWith(1)
-
-		@rclient = 
-			auth:->
-			del: @delStub
-			multi: =>
-				incr: @incrStub
-				expire: @expireStub
-				get: @getStub
-				exec: @execStub
 
 		@LoginRateLimiter = SandboxedModule.require modulePath, requires:
-			'redis-sharelatex' : createClient: () => @rclient
+			'../../infrastructure/RateLimiter': @RateLimiter
-			"settings-sharelatex":{redis:{}}
+
-	
 	describe "processLoginRequest", ->
 
-		it "should inc the counter for login requests in redis", (done)->
+		beforeEach ->
-			@execStub.callsArgWith(0, "null", ["",""])
+			@RateLimiter.addCount = sinon.stub().callsArgWith(1, null, true)
-			@LoginRateLimiter.processLoginRequest @email, =>
+
-				@incrStub.calledWith(buildKey(@email)).should.equal true
+		it 'should call RateLimiter.addCount', (done) ->
+			@LoginRateLimiter.processLoginRequest @email, (err, allow) =>
+				@RateLimiter.addCount.callCount.should.equal 1
+				expect(@RateLimiter.addCount.lastCall.args[0].endpointName).to.equal 'login'
+				expect(@RateLimiter.addCount.lastCall.args[0].subjectName).to.equal @email
 				done()
 
-		it "should set a expire", (done)->
+		describe 'when login is allowed', ->
-			@execStub.callsArgWith(0, "null", ["",""])
-			@LoginRateLimiter.processLoginRequest @email, =>
-				@expireStub.calledWith(buildKey(@email), 60 * 2).should.equal true
-				done()
 
-		it "should return true if the count is below 10", (done)->
+			beforeEach ->
-			@execStub.callsArgWith(0, "null", ["", 9])
+				@RateLimiter.addCount = sinon.stub().callsArgWith(1, null, true)
-			@LoginRateLimiter.processLoginRequest @email, (err, isAllowed)=>
-				isAllowed.should.equal true
-				done()
 
-		it "should return true if the count is 10", (done)->
+			it 'should call pass allow=true', (done) ->
-			@execStub.callsArgWith(0, "null", ["", 10])
+				@LoginRateLimiter.processLoginRequest @email, (err, allow) =>
-			@LoginRateLimiter.processLoginRequest @email, (err, isAllowed)=>
+					expect(err).to.equal null
-				isAllowed.should.equal true
+					expect(allow).to.equal true
-				done()
+					done()
 
-		it "should return false if the count is above 10", (done)->
+		describe 'when login is blocked', ->
-			@execStub.callsArgWith(0, "null", ["", 11])
-			@LoginRateLimiter.processLoginRequest @email, (err, isAllowed)=>
-				isAllowed.should.equal false
-				done()
 
+			beforeEach ->
+				@RateLimiter.addCount = sinon.stub().callsArgWith(1, null, false)
 
-	describe "smoke test user", ->
+			it 'should call pass allow=false', (done) ->
-
+				@LoginRateLimiter.processLoginRequest @email, (err, allow) =>
-		it "should have a higher limit", (done)->
+					expect(err).to.equal null
-			done()
+					expect(allow).to.equal false
+					done()
 
+		describe 'when addCount produces an error', ->
 
+			beforeEach ->
+				@RateLimiter.addCount = sinon.stub().callsArgWith(1, new Error('woops'))
 
+			it 'should produce an error', (done) ->
+				@LoginRateLimiter.processLoginRequest @email, (err, allow) =>
+					expect(err).to.not.equal null
+					expect(err).to.be.instanceof Error
+					done()
 
 
 	describe "recordSuccessfulLogin", ->
 
-		it "should delete the user key", (done)->
+		beforeEach ->
+			@RateLimiter.clearRateLimit = sinon.stub().callsArgWith 2, null
+
+		it "should call clearRateLimit", (done)->
 			@LoginRateLimiter.recordSuccessfulLogin @email, =>
-				@delStub.calledWith(buildKey(@email)).should.equal true
+				@RateLimiter.clearRateLimit.callCount.should.equal 1
-				done()
+				@RateLimiter.clearRateLimit.calledWith('login', @email).should.equal true
+				done()