Merge branch 'shadab14meb346-feat/subscription-based-rate-limit-custo…

…mization'

fern/docs/contents/rate-limits.mdx

@@ -1,5 +1,5 @@
 ### Rate Limits
 
-Revert will return a rate limit error with the HTTP status code `429` when the request rate limit for an application or an individual IP address has been exceeded.
+Revert will return a rate limit error with the HTTP status code `429` when the request rate limit for a tenant has been exceeded.
 
-As a default, 4 requests per minute are allowed from a single IP address but if you're needs are higher please get in touch and we can tailor it to your use-case.
+As a default, 100 requests per minute per connection are allowed for a single tenant but if you're needs are higher please get in touch and we can tailor it to your use-case.

packages/backend/.env.example

@@ -44,4 +44,5 @@ MS_DYNAMICS_SALES_ORG_URL=
 OPEN_INT_API_KEY=
 TWENTY_ACCOUNT_ID=
 BITBUCKET_CLIENT_ID= 
-BITBUCKET_CLIENT_SECRET=
+BITBUCKET_CLIENT_SECRET=
+DEFAULT_RATE_LIMIT_DEVELOPER_PLAN=

packages/backend/config.ts

@@ -53,6 +53,7 @@ const config = {
     OPEN_INT_API_KEY: process.env.OPEN_INT_API_KEY,
     OPEN_INT_BASE_API_URL: process.env.OPEN_INT_BASE_API_URL,
     TWENTY_ACCOUNT_ID: process.env.TWENTY_ACCOUNT_ID,
+    DEFAULT_RATE_LIMIT_DEVELOPER_PLAN: process.env.DEFAULT_RATE_LIMIT_DEVELOPER_PLAN,
 };
 
 export default config;

packages/backend/helpers/authMiddleware.ts

@@ -31,6 +31,7 @@ const revertAuthMiddleware = () => async (req: Request, res: Response, next: ()
             include: {
                 environments: true,
                 accountFieldMappingConfig: true,
+                subscription: true,
             },
         });
         if (!account || !account.length) {

packages/backend/helpers/rateLimitMiddleware.ts

@@ -0,0 +1,52 @@
+import { Request, Response } from 'express';
+import { RateLimiterRedis, IRateLimiterStoreOptions } from 'rate-limiter-flexible';
+
+import redis from '../redis/client';
+import { skipRateLimitRoutes } from './utils';
+import config from '../config';
+
+// In Memory Cache for storing RateLimiterRedis instances to prevent the creation of a new instance for each request.
+// The cache key is the 'rate_limit' value derived from the subscriptions table. Currently, we cache by the 'rate_limit' value for simplicity.
+// Using 'subscriptionId' as a key would be more precise but would add complexity in keeping the cache in sync with the database.
+const rateLimiters = new Map<number, RateLimiterRedis>();
+
+//We can make this dynamic based on the subscription as well
+const RATE_LIMIT_DURATION_IN_MINUTES = 1;
+const FALL_BACK_DEFAULT_RATE_LIMIT = 100;
+
+const getRateLimiter = (rateLimit: number): RateLimiterRedis => {
+    if (!rateLimiters.has(rateLimit)) {
+        const opts: IRateLimiterStoreOptions = {
+            storeClient: redis,
+            points: rateLimit, // Points represent the maximum number of requests allowed within the set duration.
+            duration: RATE_LIMIT_DURATION_IN_MINUTES * 60, // Converts minutes to seconds for the duration.
+        };
+        rateLimiters.set(rateLimit, new RateLimiterRedis(opts));
+    }
+    return rateLimiters.get(rateLimit)!;
+};
+
+const rateLimitMiddleware = () => async (req: Request, res: Response, next: Function) => {
+    if (skipRateLimitRoutes(req)) next();
+    try {
+        const { 'x-revert-t-id': tenantId } = req.headers;
+        const { subscription, id: accountId } = res.locals.account; // Subscription details are retrieved from response locals set earlier in the revertAuthMiddleware.
+        const rateLimit =
+            subscription?.rate_limit ?? config.DEFAULT_RATE_LIMIT_DEVELOPER_PLAN ?? FALL_BACK_DEFAULT_RATE_LIMIT; //incase subscription undefined, we will use the default rate limit this is to make sure backward compatibility as currently some accounts might not have subscription attached to them. We can remove the optional chaining and nullish coalescing once we are sure that all accounts have subscription attached to them. In case of DEFAULT_RATE_LIMIT_DEVELOPER_PLAN is missing in config, we will fallback to 1
+        const rateLimiter = getRateLimiter(rateLimit);
+        //added accountId to make the key unique
+        const uniqueKey = `${accountId}-${tenantId}`;
+        // rate limit is per tenantId
+        await rateLimiter.consume(uniqueKey, 1); // consume 1 point for each request
+        next();
+    } catch (rejRes) {
+        //currently not handling the redis failure here explicitly as it is getting handled in the redis client. If required in future we can handle it here so service does't go down due to rate limit middleware failure which might occur due to redis failure
+        if (rejRes instanceof Error) {
+            throw rejRes;
+        } else {
+            res.status(429).send('Rate limit reached.');
+        }
+    }
+};
+
+export default rateLimitMiddleware;

packages/backend/helpers/utils.ts

@@ -0,0 +1,17 @@
+import { Request } from 'express';
+
+const skipRateLimitRoutes = (req: Request) => {
+    const nonSecurePaths = ['/oauth-callback', '/oauth/refresh'];
+    const nonSecurePathsPartialMatch = ['/integration-status', '/trello-request-token'];
+    const allowedRoutes = ['/health-check'];
+    if (
+        nonSecurePaths.includes(req.path) ||
+        nonSecurePathsPartialMatch.some((path) => req.path.includes(path)) ||
+        allowedRoutes.includes(req.baseUrl + req.path)
+    ) {
+        return true;
+    }
+    return false;
+};
+
+export { skipRateLimitRoutes };

packages/backend/index.ts

@@ -15,32 +15,8 @@ import versionMiddleware, { manageRouterVersioning } from './helpers/versionMidd
 import { ShortloopSDK } from '@shortloop/node';
 import endpointLogger from './helpers/endPointLoggerMiddleWare';
 
-const rateLimit = require('express-rate-limit');
-const limiter = rateLimit({
-    windowMs: 15 * 60 * 1000, // 15 minutes
-    max: 60, // Limit each IP to 60 requests per `window` (here, per 15 minutes)
-    standardHeaders: true, // Return rate limit info in the `RateLimit-*` headers
-    legacyHeaders: false, // Disable the `X-RateLimit-*` headers
-    message: async () => {
-        return JSON.stringify({ message: 'Rate limit reached.' });
-    },
-    skip: (req: Request, _res: Response) => {
-        const basePath = req.baseUrl + req.path;
-        const allowedRoutes = ['/health-check'];
-        if (allowedRoutes.includes(basePath)) {
-            return true;
-        }
-        return false;
-    },
-});
-
 const app: Express = express();
 
-// Debug rate limiting / trust proxy issue as per: https://github.com/express-rate-limit/express-rate-limit/wiki/Troubleshooting-Proxy-Issues
-app.set('trust proxy', 1);
-app.get('/ip', (request, response) => response.send(request.ip));
-app.get('/x-forwarded-for', (request, response) => response.send(request.headers['x-forwarded-for']));
-
 Sentry.init({
     dsn: config.SENTRY_DSN,
     integrations: [
@@ -107,7 +83,6 @@ app.use(
     })
 );
 
-app.use(limiter);
 app.use(versionMiddleware());
 
 ShortloopSDK.init({

packages/backend/package.json

@@ -50,6 +50,7 @@
         "concurrently": "^8.2.1",
         "eslint-plugin-deprecation": "^1.3.2",
         "jest": "^29.6.4",
+        "ts-jest": "^29.1.2",
         "ts-node": "^10.9.1",
         "ts-node-dev": "^2.0.0",
         "typescript": "^4.8.4"
@@ -69,14 +70,14 @@
         "dayjs": "^1.11.10",
         "dotenv": "^16.3.1",
         "express": "^4.18.1",
-        "express-rate-limit": "^7.1.5",
         "ip": "^1.1.8",
         "lodash": "^4.17.21",
         "moesif-nodejs": "^3.5.3",
         "morgan": "^1.10.0",
         "node-cron": "^3.0.2",
         "oauth": "^0.10.0",
         "prisma": "^4.16.0",
+        "rate-limiter-flexible": "^5.0.3",
         "redis": "^4.6.8",
         "svix": "^1.14.0",
         "winston": "^3.11.0"

packages/backend/prisma/migrations/20240508161507_create_subscriptions_table/migration.sql

@@ -0,0 +1,22 @@
+-- AlterTable
+ALTER TABLE "accounts" ADD COLUMN     "subscriptionId" TEXT;
+
+-- CreateTable
+CREATE TABLE "subscriptions" (
+    "id" TEXT NOT NULL,
+    "name" TEXT NOT NULL,
+    "rate_limit" INTEGER NOT NULL,
+    "rate_limit_duration_in_minutes" INTEGER NOT NULL,
+    "price" DOUBLE PRECISION NOT NULL,
+    "features" JSONB,
+    "createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP,
+    "updatedAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP,
+
+    CONSTRAINT "subscriptions_pkey" PRIMARY KEY ("id")
+);
+
+-- CreateIndex
+CREATE UNIQUE INDEX "subscriptions_name_key" ON "subscriptions"("name");
+
+-- AddForeignKey
+ALTER TABLE "accounts" ADD CONSTRAINT "accounts_subscriptionId_fkey" FOREIGN KEY ("subscriptionId") REFERENCES "subscriptions"("id") ON DELETE SET NULL ON UPDATE CASCADE;

packages/backend/prisma/schema.prisma

@@ -42,6 +42,8 @@ model accounts {
   accountFieldMappingConfig accountFieldMappingConfig?
   createdAt                 DateTime                   @default(now())
   updatedAt                 DateTime                   @default(now()) @updatedAt
+  subscription              subscriptions?             @relation(fields: [subscriptionId], references: [id])
+  subscriptionId            String?
 }
 
 model environments {
@@ -155,3 +157,17 @@ model telemetry {
   createdAt DateTime @default(now())
   updatedAt DateTime @default(now()) @updatedAt
 }
+
+model subscriptions {
+  id                             String     @id @default(uuid())
+  name                           String
+  rate_limit                     Int
+  rate_limit_duration_in_minutes Int
+  price                          Float
+  features                       Json?
+  accounts                       accounts[]
+  createdAt                      DateTime   @default(now())
+  updatedAt                      DateTime   @default(now()) @updatedAt
+
+  @@unique([name])
+}

packages/backend/routes/index.ts

@@ -10,6 +10,7 @@ import { logDebug } from '../helpers/logger';
 import crmRouter from './v1/crm';
 import config from '../config';
 import revertAuthMiddleware from '../helpers/authMiddleware';
+import rateLimitMiddleware from '../helpers/rateLimitMiddleware';
 import { register } from '../generated/typescript';
 import { metadataService } from '../services/metadata';
 import { accountService } from '../services/Internal/account';
@@ -140,11 +141,11 @@ router.get('/connection/integration-status/:publicToken', async (req, res) => {
     }
 });
 
-router.use('/crm', cors(), revertAuthMiddleware(), crmRouter);
+router.use('/crm', cors(), [revertAuthMiddleware(), rateLimitMiddleware()], crmRouter);
 
-router.use('/chat', cors(), revertAuthMiddleware(), chatRouter);
+router.use('/chat', cors(), [revertAuthMiddleware(), rateLimitMiddleware()], chatRouter);
 
-router.use('/ticket', cors(), revertAuthMiddleware(), ticketRouter);
+router.use('/ticket', cors(), [revertAuthMiddleware(), rateLimitMiddleware()], ticketRouter);
 
 register(router, {
     metadata: metadataService,

packages/backend/tests/rateLimitMiddleware.test.ts

@@ -0,0 +1,56 @@
+import { Response } from 'express';
+import rateLimitMiddleware from '../helpers/rateLimitMiddleware';
+import { skipRateLimitRoutes } from '../helpers/utils';
+
+jest.mock('../redis/client', () => {
+    return {
+        createClient: jest.fn().mockReturnThis(),
+        on: jest.fn(),
+        connect: jest.fn(),
+    };
+});
+jest.mock('../helpers/utils', () => {
+    return {
+        skipRateLimitRoutes: jest.fn(),
+    };
+});
+jest.mock('rate-limiter-flexible', () => {
+    return {
+        RateLimiterRedis: jest.fn().mockImplementation(() => ({
+            consume: jest.fn(),
+        })),
+    };
+});
+//TODO: some more extensive test case are required
+describe('Rate Limit Middleware', () => {
+    const mockRequest = (options = {}) => ({
+        headers: { 'x-revert-t-id': 'tenant123' },
+        ...options,
+    });
+    const mockResponse = () => {
+        const res = {
+            locals: { account: { subscription: { rate_limit: 100 }, id: 'account123' } },
+        } as unknown as Response;
+        res.status = jest.fn().mockReturnValue(res);
+        res.send = jest.fn().mockReturnValue(res);
+        return res;
+    };
+    const nextFunction = jest.fn();
+
+    beforeEach(() => {
+        jest.clearAllMocks();
+    });
+
+    it('should call next if route should be skipped', async () => {
+        //@ts-ignore
+        skipRateLimitRoutes.mockReturnValue(true);
+        const req = mockRequest();
+        const res = mockResponse();
+
+        //@ts-ignore
+        await rateLimitMiddleware()(req, res, nextFunction);
+
+        expect(skipRateLimitRoutes).toHaveBeenCalled();
+        expect(nextFunction).toHaveBeenCalled();
+    });
+});