Merge pull request #3966 from overleaf/jpa-acceptance-tests-rate-limiter

[misc] replace RateLimiter unit tests with acceptance tests on /metrics

GitOrigin-RevId: c4ef502b38bb4b300c6b9812e1c97858e93a38fe

services/web/test/acceptance/src/RegistrationTests.js

@@ -2,11 +2,12 @@
     handle-callback-err
 */
 
-const { assert, expect } = require('chai')
+const { expect } = require('chai')
 const async = require('async')
+const metrics = require('./helpers/metrics')
 const User = require('./helpers/User')
+const UserPromises = require('./helpers/User').promises
 const redis = require('./helpers/redis')
-const _ = require('lodash')
 const Features = require('../../../app/src/infrastructure/Features')
 
 // Currently this is testing registration via the 'public-registration' module,
@@ -64,55 +65,155 @@ const tryLoginThroughRegistrationForm = function (
 
 describe('Registration', function () {
   describe('LoginRateLimit', function () {
+    let userA
     beforeEach(function () {
-      this.user = new User()
-      this.badEmail = 'bademail@example.com'
-      this.badPassword = 'badpassword'
+      userA = new UserPromises()
+    })
+    function loginRateLimited(line) {
+      return line.includes('rate_limit_hit') && line.includes('login')
+    }
+    async function getLoginRateLimitHitMetricValue() {
+      return await metrics.promises.getMetric(loginRateLimited)
+    }
+    let beforeCount
+    beforeEach('get baseline metric value', async function () {
+      beforeCount = await getLoginRateLimitHitMetricValue()
+    })
+    beforeEach('setup csrf token', async function () {
+      await userA.getCsrfToken()
     })
 
-    it('should rate limit login attempts after 10 within two minutes', function (done) {
-      this.user.request.get('/login', (err, res, body) => {
-        async.timesSeries(
-          15,
-          (n, cb) => {
-            this.user.getCsrfToken(error => {
-              if (error != null) {
-                return cb(error)
-              }
-              this.user.request.post(
-                {
-                  url: '/login',
-                  json: {
-                    email: this.badEmail,
-                    password: this.badPassword,
-                  },
-                },
-                (err, response, body) => {
-                  const message = body && body.message && body.message.text
-                  return cb(null, message)
-                }
-              )
-            })
-          },
-          (err, results) => {
-            // ten incorrect-credentials messages, then five rate-limit messages
-            expect(results.length).to.equal(15)
-            assert.deepEqual(
-              results,
-              _.concat(
-                _.fill(
-                  [1, 2, 3, 4, 5, 6, 7, 8, 9, 10],
-                  'Your email or password is incorrect. Please try again'
-                ),
-                _.fill(
-                  [1, 2, 3, 4, 5],
+    describe('pushing an account just below the rate limit', function () {
+      async function doLoginAttempts(user, n, pushInto) {
+        while (n--) {
+          const { body } = await user.doRequest('POST', {
+            url: '/login',
+            json: {
+              email: user.email,
+              password: 'invalid-password',
+            },
+          })
+          const message = body && body.message && body.message.text
+          pushInto.push(message)
+        }
+      }
+
+      let results = []
+      beforeEach('do 9 login attempts', async function () {
+        results = []
+        await doLoginAttempts(userA, 9, results)
+      })
+
+      it('should not record any rate limited requests', async function () {
+        const afterCount = await getLoginRateLimitHitMetricValue()
+        expect(afterCount).to.equal(beforeCount)
+      })
+
+      it('should produce the correct responses so far', function () {
+        expect(results.length).to.equal(9)
+        expect(results).to.deep.equal(
+          Array(9).fill('Your email or password is incorrect. Please try again')
+        )
+      })
+
+      describe('pushing the account past the limit', function () {
+        beforeEach('do 6 login attempts', async function () {
+          await doLoginAttempts(userA, 6, results)
+        })
+
+        it('should record 5 rate limited requests', async function () {
+          const afterCount = await getLoginRateLimitHitMetricValue()
+          expect(afterCount).to.equal(beforeCount + 5)
+        })
+
+        it('should produce the correct responses', function () {
+          expect(results.length).to.equal(15)
+          expect(results).to.deep.equal(
+            Array(10)
+              .fill('Your email or password is incorrect. Please try again')
+              .concat(
+                Array(5).fill(
                   'This account has had too many login requests. Please wait 2 minutes before trying to log in again'
                 )
               )
+          )
+        })
+
+        describe('logging in with another user', function () {
+          let userB
+          beforeEach(function () {
+            userB = new UserPromises()
+          })
+
+          beforeEach('update baseline metric value', async function () {
+            beforeCount = await getLoginRateLimitHitMetricValue()
+          })
+          beforeEach('setup csrf token', async function () {
+            await userB.getCsrfToken()
+          })
+
+          let messages = []
+          beforeEach('do bad login', async function () {
+            messages = []
+            await doLoginAttempts(userB, 1, messages)
+          })
+
+          it('should not rate limit their request', function () {
+            expect(messages).to.deep.equal([
+              'Your email or password is incorrect. Please try again',
+            ])
+          })
+
+          it('should not record any further rate limited requests', async function () {
+            const afterCount = await getLoginRateLimitHitMetricValue()
+            expect(afterCount).to.equal(beforeCount)
+          })
+        })
+      })
+
+      describe('performing a valid login for clearing the limit', function () {
+        beforeEach('do login', async function () {
+          await userA.login()
+        })
+
+        it('should log the user in', async function () {
+          const { response } = await userA.doRequest('GET', '/project')
+          expect(response.statusCode).to.equal(200)
+        })
+
+        it('should not record any rate limited requests', async function () {
+          const afterCount = await getLoginRateLimitHitMetricValue()
+          expect(afterCount).to.equal(beforeCount)
+        })
+
+        describe('logging out and performing more invalid login requests', function () {
+          beforeEach('logout', async function () {
+            await userA.logout()
+          })
+          beforeEach('fetch new csrf token', async function () {
+            await userA.getCsrfToken()
+          })
+
+          let results = []
+          beforeEach('do 9 login attempts', async function () {
+            results = []
+            await doLoginAttempts(userA, 9, results)
+          })
+
+          it('should not record any rate limited requests yet', async function () {
+            const afterCount = await getLoginRateLimitHitMetricValue()
+            expect(afterCount).to.equal(beforeCount)
+          })
+
+          it('should not emit any rate limited responses yet', function () {
+            expect(results.length).to.equal(9)
+            expect(results).to.deep.equal(
+              Array(9).fill(
+                'Your email or password is incorrect. Please try again'
+              )
             )
-            return done()
-          }
-        )
+          })
+        })
       })
     })
   })

services/web/test/acceptance/src/helpers/metrics.js

@@ -0,0 +1,16 @@
+const { callbackify } = require('util')
+const request = require('./request')
+
+async function getMetric(matcher) {
+  const { body } = await request.promises.request('/metrics')
+  const found = body.split('\n').find(matcher)
+  if (!found) return 0
+  return parseInt(found.split(' ')[1], 0)
+}
+
+module.exports = {
+  getMetric: callbackify(getMetric),
+  promises: {
+    getMetric,
+  },
+}

services/web/test/unit/src/infrastructure/RateLimterTests.js

@@ -1,155 +0,0 @@
-/* eslint-disable
-    node/handle-callback-err,
-    max-len,
-    no-return-assign,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
-const { assert, expect } = require('chai')
-const sinon = require('sinon')
-const modulePath = '../../../../app/src/infrastructure/RateLimiter.js'
-const SandboxedModule = require('sandboxed-module')
-
-describe('RateLimiter', function () {
-  beforeEach(function () {
-    this.settings = {
-      redis: {
-        web: {
-          port: '1234',
-          host: 'somewhere',
-          password: 'password',
-        },
-      },
-    }
-    this.rclient = {
-      incr: sinon.stub(),
-      get: sinon.stub(),
-      expire: sinon.stub(),
-      exec: sinon.stub(),
-    }
-    this.rclient.multi = sinon.stub().returns(this.rclient)
-    this.RedisWrapper = { client: sinon.stub().returns(this.rclient) }
-
-    this.endpointName = 'compiles'
-    this.subject = 'some-project-id'
-    this.timeInterval = 20
-    this.throttleLimit = 5
-
-    this.requires = {
-      'settings-sharelatex': this.settings,
-      '@overleaf/metrics': (this.Metrics = { inc: sinon.stub() }),
-      './RedisWrapper': this.RedisWrapper,
-    }
-
-    this.details = {
-      endpointName: this.endpointName,
-      subjectName: this.subject,
-      throttle: this.throttleLimit,
-      timeInterval: this.timeInterval,
-    }
-    return (this.key = `RateLimiter:${this.endpointName}:{${this.subject}}`)
-  })
-
-  describe('when action is permitted', function () {
-    beforeEach(function () {
-      this.requires['rolling-rate-limiter'] = opts => {
-        return sinon.stub().callsArgWith(1, null, 0, 22)
-      }
-      return (this.limiter = SandboxedModule.require(modulePath, {
-        requires: this.requires,
-      }))
-    })
-
-    it('should not produce and error', function (done) {
-      return this.limiter.addCount({}, (err, should) => {
-        expect(err).to.equal(null)
-        return done()
-      })
-    })
-
-    it('should callback with true', function (done) {
-      return this.limiter.addCount({}, (err, should) => {
-        expect(should).to.equal(true)
-        return done()
-      })
-    })
-
-    it('should not increment the metric', function (done) {
-      return this.limiter.addCount(
-        { endpointName: this.endpointName },
-        (err, should) => {
-          sinon.assert.notCalled(this.Metrics.inc)
-          return done()
-        }
-      )
-    })
-  })
-
-  describe('when action is not permitted', function () {
-    beforeEach(function () {
-      this.requires['rolling-rate-limiter'] = opts => {
-        return sinon.stub().callsArgWith(1, null, 4000, 0)
-      }
-      return (this.limiter = SandboxedModule.require(modulePath, {
-        globals: {
-          console: console,
-        },
-        requires: this.requires,
-      }))
-    })
-
-    it('should not produce and error', function (done) {
-      return this.limiter.addCount({}, (err, should) => {
-        expect(err).to.equal(null)
-        return done()
-      })
-    })
-
-    it('should callback with false', function (done) {
-      return this.limiter.addCount({}, (err, should) => {
-        expect(should).to.equal(false)
-        return done()
-      })
-    })
-
-    it('should increment the metric', function (done) {
-      return this.limiter.addCount(
-        { endpointName: this.endpointName },
-        (err, should) => {
-          sinon.assert.calledWith(this.Metrics.inc, `rate-limit-hit`, 1, {
-            path: this.endpointName,
-          })
-          return done()
-        }
-      )
-    })
-  })
-
-  describe('when limiter produces an error', function () {
-    beforeEach(function () {
-      this.requires['rolling-rate-limiter'] = opts => {
-        return sinon.stub().callsArgWith(1, new Error('woops'))
-      }
-      return (this.limiter = SandboxedModule.require(modulePath, {
-        globals: {
-          console: console,
-        },
-        requires: this.requires,
-      }))
-    })
-
-    it('should produce and error', function (done) {
-      return this.limiter.addCount({}, (err, should) => {
-        expect(err).to.not.equal(null)
-        expect(err).to.be.instanceof(Error)
-        return done()
-      })
-    })
-  })
-})