Trim group chat messages (#15646)

Co-authored-by: Kalista Payne <sabrecat@gmail.com>

package-lock.json

@@ -1,12 +1,12 @@
 {
   "name": "habitica",
-  "version": "5.47.7",
+  "version": "5.47.8",
   "lockfileVersion": 3,
   "requires": true,
   "packages": {
     "": {
       "name": "habitica",
-      "version": "5.47.7",
+      "version": "5.47.8",
       "hasInstallScript": true,
       "dependencies": {
         "@babel/core": "^7.22.10",

package.json

@@ -1,7 +1,7 @@
 {
   "name": "habitica",
   "description": "A habit tracker app which treats your goals like a Role Playing Game.",
-  "version": "5.47.7",
+  "version": "5.47.8",
   "main": "./website/server/index.js",
   "dependencies": {
     "@babel/core": "^7.22.10",

test/api/unit/middlewares/rateLimiter.test.js

@@ -32,8 +32,7 @@ describe('rateLimiter middleware', () => {
 
   it('is disabled when the env var is not defined', () => {
     nconfGetStub.withArgs('RATE_LIMITER_ENABLED').returns(undefined);
-    const setupRateLimiter = requireAgain(pathToRateLimiter).default;
-    const attachRateLimiter = setupRateLimiter();
+    const attachRateLimiter = requireAgain(pathToRateLimiter).default;
     attachRateLimiter(req, res, next);
 
     expect(next).to.have.been.calledOnce;
@@ -44,8 +43,7 @@ describe('rateLimiter middleware', () => {
 
   it('is disabled when the env var is an not "true"', () => {
     nconfGetStub.withArgs('RATE_LIMITER_ENABLED').returns('false');
-    const setupRateLimiter = requireAgain(pathToRateLimiter).default;
-    const attachRateLimiter = setupRateLimiter();
+    const attachRateLimiter = requireAgain(pathToRateLimiter).default;
     attachRateLimiter(req, res, next);
 
     expect(next).to.have.been.calledOnce;
@@ -57,8 +55,7 @@ describe('rateLimiter middleware', () => {
   it('does not throw when there are available points', async () => {
     nconfGetStub.withArgs('RATE_LIMITER_ENABLED').returns('true');
     nconfGetStub.withArgs('RATE_LIMITER_IP_COST').returns(1);
-    const setupRateLimiter = requireAgain(pathToRateLimiter).default;
-    const attachRateLimiter = setupRateLimiter();
+    const attachRateLimiter = requireAgain(pathToRateLimiter).default;
     await attachRateLimiter(req, res, next);
 
     expect(next).to.have.been.calledOnce;
@@ -80,8 +77,7 @@ describe('rateLimiter middleware', () => {
     sandbox.stub(RateLimiterMemory.prototype, 'consume')
       .returns(Promise.reject(new Error('Unknown error.')));
 
-    const setupRateLimiter = requireAgain(pathToRateLimiter).default;
-    const attachRateLimiter = setupRateLimiter();
+    const attachRateLimiter = requireAgain(pathToRateLimiter).default;
     await attachRateLimiter(req, res, next);
 
     expect(next).to.have.been.calledOnce;
@@ -96,8 +92,7 @@ describe('rateLimiter middleware', () => {
   it('does not throw when LIVELINESS_PROBE_KEY is correct', async () => {
     nconfGetStub.withArgs('RATE_LIMITER_ENABLED').returns('true');
     nconfGetStub.withArgs('LIVELINESS_PROBE_KEY').returns('abc');
-    const setupRateLimiter = requireAgain(pathToRateLimiter).default;
-    const attachRateLimiter = setupRateLimiter();
+    const attachRateLimiter = requireAgain(pathToRateLimiter).default;
 
     req.query.liveliness = 'abc';
     await attachRateLimiter(req, res, next);
@@ -112,8 +107,7 @@ describe('rateLimiter middleware', () => {
     nconfGetStub.withArgs('RATE_LIMITER_ENABLED').returns('true');
     nconfGetStub.withArgs('LIVELINESS_PROBE_KEY').returns('abc');
     nconfGetStub.withArgs('RATE_LIMITER_IP_COST').returns(1);
-    const setupRateLimiter = requireAgain(pathToRateLimiter).default;
-    const attachRateLimiter = setupRateLimiter();
+    const attachRateLimiter = requireAgain(pathToRateLimiter).default;
 
     req.query.liveliness = 'das';
     await attachRateLimiter(req, res, next);
@@ -130,8 +124,7 @@ describe('rateLimiter middleware', () => {
     nconfGetStub.withArgs('RATE_LIMITER_ENABLED').returns('true');
     nconfGetStub.withArgs('LIVELINESS_PROBE_KEY').returns(undefined);
     nconfGetStub.withArgs('RATE_LIMITER_IP_COST').returns(1);
-    const setupRateLimiter = requireAgain(pathToRateLimiter).default;
-    const attachRateLimiter = setupRateLimiter();
+    const attachRateLimiter = requireAgain(pathToRateLimiter).default;
 
     await attachRateLimiter(req, res, next);
 
@@ -147,8 +140,7 @@ describe('rateLimiter middleware', () => {
     nconfGetStub.withArgs('RATE_LIMITER_ENABLED').returns('true');
     nconfGetStub.withArgs('LIVELINESS_PROBE_KEY').returns('');
     nconfGetStub.withArgs('RATE_LIMITER_IP_COST').returns(1);
-    const setupRateLimiter = requireAgain(pathToRateLimiter).default;
-    const attachRateLimiter = setupRateLimiter();
+    const attachRateLimiter = requireAgain(pathToRateLimiter).default;
 
     req.query.liveliness = '';
     await attachRateLimiter(req, res, next);
@@ -164,8 +156,7 @@ describe('rateLimiter middleware', () => {
   it('throws when there are no available points remaining', async () => {
     nconfGetStub.withArgs('RATE_LIMITER_ENABLED').returns('true');
     nconfGetStub.withArgs('RATE_LIMITER_IP_COST').returns(1);
-    const setupRateLimiter = requireAgain(pathToRateLimiter).default;
-    const attachRateLimiter = setupRateLimiter();
+    const attachRateLimiter = requireAgain(pathToRateLimiter).default;
 
     // call for 31 times
     for (let i = 0; i < 31; i += 1) {
@@ -189,8 +180,7 @@ describe('rateLimiter middleware', () => {
   it('uses the user id if supplied or the ip address', async () => {
     nconfGetStub.withArgs('RATE_LIMITER_ENABLED').returns('true');
     nconfGetStub.withArgs('RATE_LIMITER_IP_COST').returns(1);
-    const setupRateLimiter = requireAgain(pathToRateLimiter).default;
-    const attachRateLimiter = setupRateLimiter();
+    const attachRateLimiter = requireAgain(pathToRateLimiter).default;
 
     req.ip = 1;
     await attachRateLimiter(req, res, next);
@@ -220,8 +210,7 @@ describe('rateLimiter middleware', () => {
   it('applies increased cost for registration calls with and without user id', async () => {
     nconfGetStub.withArgs('RATE_LIMITER_ENABLED').returns('true');
     nconfGetStub.withArgs('RATE_LIMITER_REGISTRATION_COST').returns(3);
-    const setupRateLimiter = requireAgain(pathToRateLimiter).default;
-    const attachRateLimiter = setupRateLimiter();
+    const attachRateLimiter = requireAgain(pathToRateLimiter).default;
     req.path = '/api/v4/user/auth/local/register';
 
     req.ip = 1;
@@ -252,8 +241,7 @@ describe('rateLimiter middleware', () => {
   it('applies increased cost for unauthenticated API calls', async () => {
     nconfGetStub.withArgs('RATE_LIMITER_ENABLED').returns('true');
     nconfGetStub.withArgs('RATE_LIMITER_IP_COST').returns(10);
-    const setupRateLimiter = requireAgain(pathToRateLimiter).default;
-    const attachRateLimiter = setupRateLimiter();
+    const attachRateLimiter = requireAgain(pathToRateLimiter).default;
 
     req.ip = 1;
     await attachRateLimiter(req, res, next);

test/api/unit/models/group.test.js

@@ -6,6 +6,8 @@ import {
   SPAM_MESSAGE_LIMIT,
   SPAM_MIN_EXEMPT_CONTRIB_LEVEL,
   SPAM_WINDOW_LENGTH,
+  MAX_CHAT_COUNT,
+  MAX_SUBBED_GROUP_CHAT_COUNT,
   INVITES_LIMIT,
   model as Group,
 } from '../../../../website/server/models/group';
@@ -18,6 +20,7 @@ import {
 import * as email from '../../../../website/server/libs/email';
 import { TAVERN_ID } from '../../../../website/common/script/constants';
 import shared from '../../../../website/common';
+import { chatModel as Chat } from '../../../../website/server/models/message';
 
 describe('Group Model', () => {
   let party; let questLeader; let participatingMember;
@@ -1356,6 +1359,29 @@ describe('Group Model', () => {
       });
     });
 
+    describe('#getEffectiveChatLimit', () => {
+      it('returns the correct chat limit', () => {
+        const group = new Group();
+        expect(group.getEffectiveChatLimit()).to.eql(MAX_CHAT_COUNT);
+      });
+
+      it('returns the passed limit if it is lower than the max', () => {
+        const group = new Group();
+        expect(group.getEffectiveChatLimit(10)).to.eql(10);
+      });
+
+      it('returns the max if the passed limit is higher', () => {
+        const group = new Group();
+        expect(group.getEffectiveChatLimit(MAX_CHAT_COUNT + 10)).to.eql(MAX_CHAT_COUNT);
+      });
+
+      it('returns the max for group plans', () => {
+        const group = new Group();
+        group.purchased.plan.customerId = '110002222333';
+        expect(group.getEffectiveChatLimit()).to.eql(MAX_SUBBED_GROUP_CHAT_COUNT);
+      });
+    });
+
     describe('#sendChat', () => {
       beforeEach(() => {
         sandbox.spy(User, 'updateOne');
@@ -1462,6 +1488,34 @@ describe('Group Model', () => {
       });
     });
 
+    describe('#trimChat', () => {
+      it('Only checks last message when not enough messages to trim', async () => {
+        sandbox.spy(Chat, 'find');
+        sandbox.spy(Chat, 'deleteMany');
+        await Chat.insertOne({ groupId: party._id, timestamp: new Date() });
+        await Chat.insertOne({ groupId: party._id, timestamp: new Date() });
+        await Chat.insertOne({ groupId: party._id, timestamp: new Date() });
+        await party.trimChat();
+
+        expect(Chat.find).to.be.calledOnce;
+        expect(Chat.deleteMany).to.not.be.called;
+        expect(await Chat.countDocuments({ groupId: party._id })).to.eql(3);
+      });
+      it('Deletes messages over the limit', async () => {
+        sandbox.spy(Chat, 'find');
+        sandbox.spy(Chat, 'deleteMany');
+        await Chat.insertOne({ groupId: party._id, timestamp: new Date() });
+        await Chat.insertOne({ groupId: party._id, timestamp: new Date() });
+        await Chat.insertOne({ groupId: party._id, timestamp: new Date() });
+
+        await party.trimChat(1);
+
+        expect(Chat.find).to.be.calledOnce;
+        expect(Chat.deleteMany).to.be.calledOnce;
+        expect(await Chat.countDocuments({ groupId: party._id })).to.eql(1);
+      });
+    });
+
     describe('#startQuest', () => {
       context('Failure Conditions', () => {
         it('throws an error if group is not a party', async () => {

website/server/controllers/api-v3/chat.js

@@ -227,6 +227,7 @@ api.postChat = {
     }
 
     await Promise.all(toSave);
+    await group.trimChat();
 
     if (chatUpdated) {
       res.respond(200, { chat: chatRes.chat });

website/server/libs/chat/group-chat.js

@@ -1,25 +1,13 @@
 import _ from 'lodash';
 import { chatModel as Chat } from '../../models/message';
 import shared from '../../../common';
-import { // eslint-disable-line import/no-cycle
-  MAX_CHAT_COUNT,
-  MAX_SUBBED_GROUP_CHAT_COUNT,
-} from '../../models/group';
 
 const questScrolls = shared.content.quests;
 
 // @TODO: Don't use this method when the group can be saved.
 export async function getGroupChat (group, options = {}) {
   const { limit, before } = options;
-
-  let maxChatCount = MAX_CHAT_COUNT;
-  if (group.chatLimitCount && group.chatLimitCount >= MAX_CHAT_COUNT) {
-    maxChatCount = group.chatLimitCount;
-  } else if (group.hasActiveGroupPlan()) {
-    maxChatCount = MAX_SUBBED_GROUP_CHAT_COUNT;
-  }
-
-  const effectiveLimit = limit !== undefined ? Math.min(limit, maxChatCount) : maxChatCount;
+  const effectiveLimit = group.getEffectiveChatLimit(limit);
 
   let query = Chat.find({ groupId: group._id })
     .sort('-timestamp');

website/server/middlewares/appRoutes.js

@@ -1,9 +1,8 @@
-import nconf from 'nconf';
 import express from 'express';
 import expressValidator from 'express-validator';
 import path from 'path';
 import setupBody from './setupBody';
-import setupRateLimiter from './rateLimiter';
+import rateLimiter from './rateLimiter';
 import setupExpress from '../libs/setupExpress';
 import * as routes from '../libs/routes';
 
@@ -11,9 +10,6 @@ const API_V3_CONTROLLERS_PATH = path.join(__dirname, '/../controllers/api-v3/');
 const API_V4_CONTROLLERS_PATH = path.join(__dirname, '/../controllers/api-v4/');
 const TOP_LEVEL_CONTROLLERS_PATH = path.join(__dirname, '/../controllers/top-level/');
 
-const RATE_LIMITER_V4_POINTS = nconf.get('RATE_LIMITER_V4_POINTS') || 100;
-const RATE_LIMITER_V4_REGISTRATION_COST = nconf.get('RATE_LIMITER_V4_REGISTRATION_COST') || 10;
-
 const app = express();
 
 // re-set the view options because they are not inherited from the top level app
@@ -29,10 +25,7 @@ app.use('/', topLevelRouter);
 
 const v3Router = express.Router(); // eslint-disable-line new-cap
 routes.walkControllers(v3Router, API_V3_CONTROLLERS_PATH);
-const v3RateLimiter = setupRateLimiter({
-  keyPrefix: 'api-v3',
-});
-app.use('/api/v3', v3RateLimiter, v3Router);
+app.use('/api/v3', rateLimiter, v3Router);
 
 // API v4 proxies API v3 routes by default.
 // It can also disable or override v3 routes
@@ -56,11 +49,6 @@ const v4RouterOverrides = [
 const v4Router = express.Router(); // eslint-disable-line new-cap
 routes.walkControllers(v4Router, API_V3_CONTROLLERS_PATH, v4RouterOverrides);
 routes.walkControllers(v4Router, API_V4_CONTROLLERS_PATH);
-const v4RateLimiter = setupRateLimiter({
-  keyPrefix: 'api-v4',
-  points: RATE_LIMITER_V4_POINTS,
-  registrationCost: RATE_LIMITER_V4_REGISTRATION_COST,
-});
-app.use('/api/v4', v4RateLimiter, v4Router);
+app.use('/api/v4', v4Router);
 
 export default app;

website/server/middlewares/rateLimiter.js

@@ -23,39 +23,55 @@ const REDIS_HOST = nconf.get('REDIS_HOST');
 const REDIS_PASSWORD = nconf.get('REDIS_PASSWORD');
 const REDIS_PORT = nconf.get('REDIS_PORT');
 const LIVELINESS_PROBE_KEY = nconf.get('LIVELINESS_PROBE_KEY');
-const BASE_POINTS = nconf.get('RATE_LIMITER_BASE_POINTS') || 30;
-const BASE_DURATION = nconf.get('RATE_LIMITER_BASE_DURATION') || 60;
 const REGISTRATION_COST = nconf.get('RATE_LIMITER_REGISTRATION_COST') || 5;
 const IP_RATE_LIMIT_COST = nconf.get('RATE_LIMITER_IP_COST') || 5;
 
 let redisClient;
+let rateLimiter;
 
-if (RATE_LIMITER_ENABLED && !IS_TEST) {
-  redisClient = redis.createClient({
-    host: REDIS_HOST,
-    password: REDIS_PASSWORD,
-    port: REDIS_PORT,
-    enable_offline_queue: false,
-  });
+const rateLimiterOpts = {
+  keyPrefix: 'api-v3',
+  points: 30, // 30 requests
+  duration: 60, // per 1 minute by User ID or IP
+};
 
-  redisClient.on('ready', () => {
-    SERVER_STATUS.REDIS = true;
-  });
+if (RATE_LIMITER_ENABLED) {
+  if (IS_TEST) {
+    rateLimiter = new RateLimiterMemory({
+      ...rateLimiterOpts,
+    });
+  } else {
+    redisClient = redis.createClient({
+      host: REDIS_HOST,
+      password: REDIS_PASSWORD,
+      port: REDIS_PORT,
+      enable_offline_queue: false,
+    });
 
-  redisClient.on('reconnecting', () => {
-    SERVER_STATUS.REDIS = false;
-  });
+    redisClient.on('ready', () => {
+      SERVER_STATUS.REDIS = true;
+    });
 
-  redisClient.on('error', error => {
-    logger.error(error, 'Redis Error');
-  });
+    redisClient.on('reconnecting', () => {
+      SERVER_STATUS.REDIS = false;
+    });
+
+    redisClient.on('error', error => {
+      logger.error(error, 'Redis Error');
+    });
+
+    rateLimiter = new RateLimiterRedis({
+      ...rateLimiterOpts,
+      storeClient: redisClient,
+    });
+  }
 } else {
   SERVER_STATUS.REDIS = true;
 }
 
-function setResponseHeaders (res, points, rateLimiterRes) {
+function setResponseHeaders (res, rateLimiterRes) {
   const headers = {
-    'X-RateLimit-Limit': points,
+    'X-RateLimit-Limit': rateLimiterOpts.points,
     'X-RateLimit-Remaining': rateLimiterRes.remainingPoints,
     'X-RateLimit-Reset': new Date(Date.now() + rateLimiterRes.msBeforeNext),
   };
@@ -67,55 +83,34 @@ function setResponseHeaders (res, points, rateLimiterRes) {
   res.set(headers);
 }
 
-export default function setupRateLimiter (options = {}) {
-  const rateLimiterOpts = {
-    keyPrefix: options.keyPrefix || 'api',
-    points: options.points || BASE_POINTS, // 30 requests
-    duration: options.duration || BASE_DURATION, // per 1 minute by User ID or IP
-  };
-  let rateLimiter;
-  if (!RATE_LIMITER_ENABLED) {
-    return (req, res, next) => next();
+export default function rateLimiterMiddleware (req, res, next) {
+  if (!RATE_LIMITER_ENABLED) return next();
+  if (LIVELINESS_PROBE_KEY && req.query.liveliness === LIVELINESS_PROBE_KEY) return next();
+
+  const userId = req.header('x-api-user');
+
+  let cost = 1;
+  if (req.path === '/api/v4/user/auth/local/register' || req.path === '/api/v3/user/auth/local/register') {
+    cost = REGISTRATION_COST;
+  } else if (!userId) {
+    cost = IP_RATE_LIMIT_COST;
   }
-  if (IS_TEST) {
-    rateLimiter = new RateLimiterMemory({
-      ...rateLimiterOpts,
+
+  return rateLimiter.consume(userId || req.ip, cost)
+    .then(rateLimiterRes => {
+      setResponseHeaders(res, rateLimiterRes);
+      return next();
+    })
+    .catch(rateLimiterRes => {
+      if (rateLimiterRes instanceof RateLimiterRes) {
+        setResponseHeaders(res, rateLimiterRes);
+        return next(new TooManyRequests(apiError('clientRateLimited')));
+      }
+
+      // In case of an unhandled error we skip the middleware as it could mean
+      // , for example, that the connection to the redis database is not working.
+      // We do not want to block all requests in these cases.
+      logger.error(rateLimiterRes, 'Rate Limiter Error');
+      return next();
     });
-  } else {
-    rateLimiter = new RateLimiterRedis({
-      ...rateLimiterOpts,
-      storeClient: redisClient,
-    });
-  }
-  return function rateLimiterMiddleware (req, res, next) {
-    if (!RATE_LIMITER_ENABLED) return next();
-    if (LIVELINESS_PROBE_KEY && req.query.liveliness === LIVELINESS_PROBE_KEY) return next();
-
-    const userId = req.header('x-api-user');
-
-    let cost = 1;
-    if (req.path === '/api/v4/user/auth/local/register' || req.path === '/api/v3/user/auth/local/register') {
-      cost = options.registrationCost || REGISTRATION_COST;
-    } else if (!userId) {
-      cost = options.ipRateLimitCost || IP_RATE_LIMIT_COST;
-    }
-
-    return rateLimiter.consume(userId || req.ip, cost)
-      .then(rateLimiterRes => {
-        setResponseHeaders(res, rateLimiterOpts.points, rateLimiterRes);
-        return next();
-      })
-      .catch(rateLimiterRes => {
-        if (rateLimiterRes instanceof RateLimiterRes) {
-          setResponseHeaders(res, rateLimiterOpts.points, rateLimiterRes);
-          return next(new TooManyRequests(apiError('clientRateLimited')));
-        }
-
-        // In case of an unhandled error we skip the middleware as it could mean
-        // , for example, that the connection to the redis database is not working.
-        // We do not want to block all requests in these cases.
-        logger.error(rateLimiterRes, 'Rate Limiter Error');
-        return next();
-      });
-  };
 }

website/server/models/group.js

@@ -497,6 +497,17 @@ schema.statics.validateInvitations = async function getInvitationErr (invites, r
   }
 };
 
+schema.methods.getEffectiveChatLimit = function getEffectiveChatLimit (limit) {
+  let maxChatCount = MAX_CHAT_COUNT;
+  if (this.chatLimitCount && this.chatLimitCount >= MAX_CHAT_COUNT) {
+    maxChatCount = this.chatLimitCount;
+  } else if (this.hasActiveGroupPlan()) {
+    maxChatCount = MAX_SUBBED_GROUP_CHAT_COUNT;
+  }
+
+  return limit !== undefined ? Math.min(limit, maxChatCount) : maxChatCount;
+};
+
 schema.methods.getParticipatingQuestMembers = function getParticipatingQuestMembers () {
   return Object.keys(this.quest.members).filter(member => this.quest.members[member]);
 };
@@ -654,6 +665,18 @@ schema.methods.sendChat = async function sendChat (options = {}) {
   return newChatMessage;
 };
 
+schema.methods.trimChat = async function trimChat (limit) {
+  const query = Chat.find({ groupId: this._id })
+    .sort('-timestamp')
+    .skip(limit || (this.getEffectiveChatLimit() * 2))
+    .limit(1);
+  const lastMessage = await query.exec();
+  if (lastMessage && lastMessage.length > 0) {
+    const lastMessageTimestamp = lastMessage[0].timestamp;
+    await Chat.deleteMany({ groupId: this._id, timestamp: { $lte: lastMessageTimestamp } }).exec();
+  }
+};
+
 // eslint-disable-next-line max-len
 schema.methods.handleQuestInvitation = async function handleQuestInvitation (user, accept, session) {
   if (!user) throw new InternalServerError('Must provide user to handle quest invitation');