fix(rate-limit): use the value of an object key, instead of the key itself

CHANGELOG.md

@@ -57,13 +57,15 @@
 - Handle label params used in form inputs when rendering in action details modal
 - **Staged files getting reset on precommit hook failure** We were running lint-staged separately on each package using lerna which potentially created a race condition causing staged changes to get lost on failure. Now we are running lint-staged directly without depending on lerna. **_This is purely a DX improvement without affecting any functionality of the system_**
 - Fix `informantType` missing in template object which prevented rendering informant relationship data in the certificates [#5952](https://github.com/opencrvs/opencrvs-core/issues/5952)
+- Fix users hitting rate limit when multiple users authenticated the same time with different usernames [#7728](https://github.com/opencrvs/opencrvs-core/issues/7728)
 
 ### Breaking changes
 
 - Remove informant notification configuration from the UI and read notification configuration settings from `record-notification` endpoint in countryconfig
 - **Gateways searchEvents API updated** `operationHistories` only returns `operationType` & `operatedOn` due to the other fields being unused in OpenCRVS
 - **Config changes to review/preview and signatures** Core used to provide review/preview section by default which are now removed and need to be provided from countryconfig. The signature field definitions (e.g. informant signature, bride signature etc.) were hard coded in core which also have now been removed. The signatures can now be added through the review/preview sections defined in countryconfig just like any other field. You can use the following section definition as the default which is without any additional fields. We highly recommend checking out our reference country repository which has the signature fields in its review/preview sections
 - `hasChildLocation` query has been removed from gateway. We have created the query `isLeafLevelLocation` instead which is more descriptive on its intended use.
+
 ```
 {
   id: 'preview',

packages/gateway/src/rate-limit.test.ts

@@ -14,14 +14,14 @@ import { resolvers as locationRootResolvers } from '@gateway/features/location/r
 import * as fetchAny from 'jest-fetch-mock'
 import * as jwt from 'jsonwebtoken'
 import { readFileSync } from 'fs'
-// eslint-disable-next-line import/no-relative-parent-imports
 import {
   startContainer,
   stopContainer,
   flushAll
 } from './utils/redis-test-utils'
 import { StartedTestContainer } from 'testcontainers'
 import { savedAdministrativeLocation } from '@opencrvs/commons/fixtures'
+import { createServer } from '@gateway/server'
 
 const fetch = fetchAny as any
 const resolvers = rootResolvers as any
@@ -38,6 +38,7 @@ jest.mock('./constants', () => {
 })
 describe('Rate limit', () => {
   let authHeaderRegAgent: { Authorization: string }
+  let authHeaderRegAgent2: { Authorization: string }
 
   beforeAll(async () => {
     container = await startContainer()
@@ -63,6 +64,20 @@ describe('Rate limit', () => {
     authHeaderRegAgent = {
       Authorization: `Bearer ${validateToken}`
     }
+
+    const validateToken2 = jwt.sign(
+      { scope: ['validate'] },
+      readFileSync('./test/cert.key'),
+      {
+        subject: '5bdc55ece42c82de9a529c36',
+        algorithm: 'RS256',
+        issuer: 'opencrvs:auth-service',
+        audience: 'opencrvs:gateway-user'
+      }
+    )
+    authHeaderRegAgent2 = {
+      Authorization: `Bearer ${validateToken2}`
+    }
   })
 
   it('allows 10 calls and then throws RateLimitError', async () => {
@@ -143,6 +158,70 @@ describe('Rate limit', () => {
     ).resolves.not.toThrowError()
   })
 
+  it('does not throw RateLimitError when different users try to access the same route', async () => {
+    const users = [
+      { username: 'sakibal.hasan', id: '0' },
+      { username: 'p.rouvila', id: '1' }
+    ]
+
+    // Call the route 7 times for all users, it should not throw RateLimitError for this user yet
+    for (let i = 1; i <= 7; i++) {
+      fetch.mockResponseOnce(
+        JSON.stringify({
+          username: users[0].username,
+          id: users[0].id,
+          scope: ['declare'],
+          status: 'active'
+        })
+      )
+
+      await resolvers.Query.verifyPasswordById(
+        {},
+        { id: users[0].id, password: 'test' },
+        { headers: authHeaderRegAgent },
+        { fieldName: 'verifyPasswordById' }
+      )
+    }
+
+    // ...now call the same route 7 times for the second user, it should not throw RateLimitError for this user either
+    for (let i = 1; i <= 7; i++) {
+      fetch.mockResponseOnce(
+        JSON.stringify({
+          username: users[1].username,
+          id: users[1].id,
+          scope: ['declare'],
+          status: 'active'
+        })
+      )
+
+      await resolvers.Query.verifyPasswordById(
+        {},
+        { id: users[1].id, password: 'test' },
+        { headers: authHeaderRegAgent2 },
+        { fieldName: 'verifyPasswordById' }
+      )
+    }
+
+    // ...now call the same route for the first user again, it should not still throw RateLimitError
+    fetch.mockResponseOnce(
+      JSON.stringify({
+        username: users[0].username,
+        id: users[0].id,
+        scope: ['declare'],
+        status: 'active'
+      })
+    )
+
+    return expect(
+      resolvers.Query.verifyPasswordById(
+        {},
+        { id: users[0].id, password: 'test' },
+        { headers: authHeaderRegAgent },
+        { fieldName: 'verifyPasswordById' }
+      )
+    ).resolves.not.toThrowError()
+  })
+
   it('does not throw RateLimitError when a non-rate-limited route is being called 20 times', async () => {
     const resolverCalls = Array.from({ length: 20 }, async () => {
       fetch.mockResponseOnce(
@@ -160,4 +239,57 @@ describe('Rate limit', () => {
 
     return expect(() => Promise.all(resolverCalls)).not.toThrowError()
   })
+
+  it('handles multiple users authenticating with different usernames', async () => {
+    const server = await createServer()
+
+    // okay to go through 10 times
+    for (let i = 1; i <= 10; i++) {
+      const res = await server.app.inject({
+        method: 'POST',
+        url: '/auth/authenticate',
+        payload: {
+          username: 'test.user',
+          password: 'test'
+        }
+      })
+
+      expect((res.result as any).statusCode).not.toBe(402)
+    }
+
+    // should return 402 Forbidden
+    const res = await server.app.inject({
+      method: 'POST',
+      url: '/auth/authenticate',
+      payload: {
+        username: 'test.user',
+        password: 'test'
+      }
+    })
+    expect((res.result as any).statusCode).toBe(402)
+
+    // okay to go through 10 times with a different username
+    for (let i = 1; i <= 10; i++) {
+      const res = await server.app.inject({
+        method: 'POST',
+        url: '/auth/authenticate',
+        payload: {
+          username: 'test.user2',
+          password: 'test'
+        }
+      })
+      expect((res.result as any).statusCode).not.toBe(402)
+    }
+
+    // should return 402 Forbidden
+    const res2 = await server.app.inject({
+      method: 'POST',
+      url: '/auth/authenticate',
+      payload: {
+        username: 'test.user2',
+        password: 'test'
+      }
+    })
+    expect((res2.result as any).statusCode).toBe(402)
+  })
 })

packages/gateway/src/rate-limit.ts

@@ -105,14 +105,17 @@ export const rateLimitedRoute =
     const key = pathOptionsForKey!.find(
       (path) => get(payload, path) !== undefined
     )
+    const value = get(payload, key!)
 
-    if (!key) {
-      throw new Error("Couldn't find a rate limiting key in payload")
+    if (!value) {
+      throw new Error(
+        "Couldn't find the value for a rate limiting key in payload"
+      )
     }
 
     return withRateLimit(
       {
-        key: `${key}:${route}`,
+        key: `${value}:${route}`,
         requestsPerMinute
       },
       fn