From 25da5280a5d99b73bbae322b52a59dc4f6d532f3 Mon Sep 17 00:00:00 2001 From: Martin Schoeler Date: Mon, 15 Jul 2024 22:42:39 -0300 Subject: [PATCH 1/5] fix: Wrong federation setting translation value (#32788) --- .changeset/grumpy-worms-appear.md | 5 +++++ packages/i18n/src/locales/en.i18n.json | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) create mode 100644 .changeset/grumpy-worms-appear.md diff --git a/.changeset/grumpy-worms-appear.md b/.changeset/grumpy-worms-appear.md new file mode 100644 index 000000000000..fb9fab77b24c --- /dev/null +++ b/.changeset/grumpy-worms-appear.md @@ -0,0 +1,5 @@ +--- +"@rocket.chat/i18n": patch +--- + +Fixed wrong wording on a federation setting diff --git a/packages/i18n/src/locales/en.i18n.json b/packages/i18n/src/locales/en.i18n.json index 875996b53828..f9fcf0163caf 100644 --- a/packages/i18n/src/locales/en.i18n.json +++ b/packages/i18n/src/locales/en.i18n.json @@ -2320,8 +2320,8 @@ "Federation_Matrix_not_allowed_to_change_moderator": "You are not allowed to change the moderator", "Federation_Matrix_not_allowed_to_change_owner": "You are not allowed to change the owner", "Federation_Matrix_join_public_rooms_is_premium": "Join federated rooms is a Premium feature", - "Federation_Matrix_max_size_of_public_rooms_users": "Maximum number of users when joining a public room in a remote server", - "Federation_Matrix_max_size_of_public_rooms_users_desc": "The number of the maximum users when joining a public room in a remote server. Public Rooms with more users will be ignored in the list of Public Rooms to join.", + "Federation_Matrix_max_size_of_public_rooms_users": "Maximum number of members when joining a public room in a remote server", + "Federation_Matrix_max_size_of_public_rooms_users_desc": "The user limit from a public room in a remote server that can still be joined. Rooms that exceed this setting will still be listed, but users won't be able to join them", "Federation_Matrix_max_size_of_public_rooms_users_Alert": "Keep in mind, that the bigger the room you allow for users to join, the more time it will take to join that room, besides the amount of resource it will use. Read more", "Federation_Matrix_serve_well_known": "Serve Well Known", "Federation_Matrix_serve_well_known_Description": "Serve /.well-known/matrix/server and /.well-known/matrix/client directly from within Rocket.Chat instead of reverse proxy for federation", From d0bedf7bb14446c1f22c47b60ad96ddd62d9e85f Mon Sep 17 00:00:00 2001 From: gabriellsh <40830821+gabriellsh@users.noreply.github.com> Date: Tue, 16 Jul 2024 00:07:51 -0300 Subject: [PATCH 2/5] feat: replicate IPreRoomUserLeave and IPostRoomUserLeave event in meteor method and add removedBy to IRoomUserLeaveContext (#32724) --- .changeset/sour-forks-breathe.md | 5 +++++ apps/meteor/app/apps/server/bridges/listeners.js | 3 ++- .../app/lib/server/functions/removeUserFromRoom.ts | 10 +++------- apps/meteor/server/methods/removeUserFromRoom.ts | 14 ++++++++++++++ apps/meteor/server/services/room/service.ts | 2 +- apps/meteor/server/services/team/service.ts | 4 ++-- 6 files changed, 27 insertions(+), 11 deletions(-) create mode 100644 .changeset/sour-forks-breathe.md diff --git a/.changeset/sour-forks-breathe.md b/.changeset/sour-forks-breathe.md new file mode 100644 index 000000000000..2d1076845fa9 --- /dev/null +++ b/.changeset/sour-forks-breathe.md @@ -0,0 +1,5 @@ +--- +"@rocket.chat/meteor": minor +--- + +Extended apps-engine events for users leaving a room to also fire when being removed by another user. Also added the triggering user's information to the event's context payload. diff --git a/apps/meteor/app/apps/server/bridges/listeners.js b/apps/meteor/app/apps/server/bridges/listeners.js index ab2632c912b0..13db1179310c 100644 --- a/apps/meteor/app/apps/server/bridges/listeners.js +++ b/apps/meteor/app/apps/server/bridges/listeners.js @@ -143,10 +143,11 @@ export class AppListenerBridge { }; case AppInterface.IPreRoomUserLeave: case AppInterface.IPostRoomUserLeave: - const [leavingUser] = payload; + const [leavingUser, removedBy] = payload; return { room: rm, leavingUser: this.orch.getConverters().get('users').convertToApp(leavingUser), + removedBy: this.orch.getConverters().get('users').convertToApp(removedBy), }; default: return rm; diff --git a/apps/meteor/app/lib/server/functions/removeUserFromRoom.ts b/apps/meteor/app/lib/server/functions/removeUserFromRoom.ts index 3b065c68f15c..c55ee382f10c 100644 --- a/apps/meteor/app/lib/server/functions/removeUserFromRoom.ts +++ b/apps/meteor/app/lib/server/functions/removeUserFromRoom.ts @@ -10,11 +10,7 @@ import { beforeLeaveRoomCallback } from '../../../../lib/callbacks/beforeLeaveRo import { settings } from '../../../settings/server'; import { notifyOnRoomChangedById } from '../lib/notifyListener'; -export const removeUserFromRoom = async function ( - rid: string, - user: IUser, - options?: { byUser: Pick }, -): Promise { +export const removeUserFromRoom = async function (rid: string, user: IUser, options?: { byUser: IUser }): Promise { const room = await Rooms.findOneById(rid); if (!room) { @@ -22,7 +18,7 @@ export const removeUserFromRoom = async function ( } try { - await Apps.self?.triggerEvent(AppEvents.IPreRoomUserLeave, room, user); + await Apps.self?.triggerEvent(AppEvents.IPreRoomUserLeave, room, user, options?.byUser); } catch (error: any) { if (error.name === AppsEngineException.name) { throw new Meteor.Error('error-app-prevented', error.message); @@ -75,5 +71,5 @@ export const removeUserFromRoom = async function ( void notifyOnRoomChangedById(rid); - await Apps.self?.triggerEvent(AppEvents.IPostRoomUserLeave, room, user); + await Apps.self?.triggerEvent(AppEvents.IPostRoomUserLeave, room, user, options?.byUser); }; diff --git a/apps/meteor/server/methods/removeUserFromRoom.ts b/apps/meteor/server/methods/removeUserFromRoom.ts index 08cd6c5b25e0..6662ae8d22cf 100644 --- a/apps/meteor/server/methods/removeUserFromRoom.ts +++ b/apps/meteor/server/methods/removeUserFromRoom.ts @@ -1,3 +1,5 @@ +import { Apps, AppEvents } from '@rocket.chat/apps'; +import { AppsEngineException } from '@rocket.chat/apps-engine/definition/exceptions'; import { Message, Team } from '@rocket.chat/core-services'; import { Subscriptions, Rooms, Users } from '@rocket.chat/models'; import type { ServerMethods } from '@rocket.chat/ui-contexts'; @@ -75,6 +77,16 @@ export const removeUserFromRoomMethod = async (fromId: string, data: { rid: stri } } + try { + await Apps.self?.triggerEvent(AppEvents.IPreRoomUserLeave, room, removedUser, fromUser); + } catch (error: any) { + if (error.name === AppsEngineException.name) { + throw new Meteor.Error('error-app-prevented', error.message); + } + + throw error; + } + await callbacks.run('beforeRemoveFromRoom', { removedUser, userWhoRemoved: fromUser }, room); await Subscriptions.removeByRoomIdAndUserId(data.rid, removedUser._id); @@ -99,6 +111,8 @@ export const removeUserFromRoomMethod = async (fromId: string, data: { rid: stri void notifyOnRoomChanged(room); }); + await Apps.self?.triggerEvent(AppEvents.IPostRoomUserLeave, room, removedUser, fromUser); + return true; }; diff --git a/apps/meteor/server/services/room/service.ts b/apps/meteor/server/services/room/service.ts index b112209b681b..4b0fe2d177b5 100644 --- a/apps/meteor/server/services/room/service.ts +++ b/apps/meteor/server/services/room/service.ts @@ -66,7 +66,7 @@ export class RoomService extends ServiceClassInternal implements IRoomService { return addUserToRoom(roomId, user, inviter, silenced); } - async removeUserFromRoom(roomId: string, user: IUser, options?: { byUser: Pick }): Promise { + async removeUserFromRoom(roomId: string, user: IUser, options?: { byUser: IUser }): Promise { return removeUserFromRoom(roomId, user, options); } diff --git a/apps/meteor/server/services/team/service.ts b/apps/meteor/server/services/team/service.ts index f81b21d7fa01..be153d565f18 100644 --- a/apps/meteor/server/services/team/service.ts +++ b/apps/meteor/server/services/team/service.ts @@ -771,7 +771,7 @@ export class TeamService extends ServiceClassInternal implements ITeamService { const usersToRemove = await Users.findByIds(membersIds, { projection: { _id: 1, username: 1 }, }).toArray(); - const byUser = (await Users.findOneById(uid, { projection: { _id: 1, username: 1 } })) as Pick; + const byUser = await Users.findOneById(uid); for await (const member of members) { if (!member.userId) { @@ -802,7 +802,7 @@ export class TeamService extends ServiceClassInternal implements ITeamService { await removeUserFromRoom( team.roomId, removedUser, - uid !== member.userId + uid !== member.userId && byUser ? { byUser, } From eff91b44a5eeaf3d84065d8231831e7549a315ee Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Tue, 16 Jul 2024 12:26:51 -0600 Subject: [PATCH 3/5] test: Flaky `mention @all @here` yet again (#32799) --- apps/meteor/tests/e2e/message-mentions.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/meteor/tests/e2e/message-mentions.spec.ts b/apps/meteor/tests/e2e/message-mentions.spec.ts index 9d5cd7e622ce..15abd9c36b40 100644 --- a/apps/meteor/tests/e2e/message-mentions.spec.ts +++ b/apps/meteor/tests/e2e/message-mentions.spec.ts @@ -56,7 +56,7 @@ test.describe.serial('Should not allow to send @all mention if permission to do await expect(page).toHaveURL(`/group/${targetChannel2}`); }); await test.step('receive notify message', async () => { - await adminPage.content.dispatchSlashCommand('@all'); + await adminPage.content.sendMessage('@all '); await expect(adminPage.content.lastUserMessage).toContainText('Notify all in this room is not allowed'); }); }); @@ -98,7 +98,7 @@ test.describe.serial('Should not allow to send @here mention if permission to do await expect(page).toHaveURL(`/group/${targetChannel2}`); }); await test.step('receive notify message', async () => { - await adminPage.content.dispatchSlashCommand('@here'); + await adminPage.content.sendMessage('@here '); await expect(adminPage.content.lastUserMessage).toContainText('Notify all in this room is not allowed'); }); }); From 8fc6ca8b4e09b38ea9c74dbed586b8bc3694cf09 Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Tue, 16 Jul 2024 13:19:07 -0600 Subject: [PATCH 4/5] fix: `Accounts_LoginExpiration` being used differently on codebase (#32527) --- .changeset/empty-readers-teach.md | 8 +++++ apps/meteor/app/api/server/v1/users.ts | 6 ++-- .../authentication/server/startup/index.js | 3 +- ee/apps/account-service/Dockerfile | 3 ++ ee/apps/account-service/package.json | 1 + ee/apps/account-service/src/Account.ts | 11 +++--- ee/apps/account-service/src/lib/utils.ts | 3 +- packages/tools/jest.config.ts | 3 ++ packages/tools/package.json | 2 ++ packages/tools/src/converter.spec.ts | 15 ++++++++ packages/tools/src/converter.ts | 7 ++++ packages/tools/src/getLoginExpiration.spec.ts | 35 +++++++++++++++++++ packages/tools/src/getLoginExpiration.ts | 16 +++++++++ packages/tools/src/index.ts | 2 ++ yarn.lock | 1 + 15 files changed, 106 insertions(+), 10 deletions(-) create mode 100644 .changeset/empty-readers-teach.md create mode 100644 packages/tools/jest.config.ts create mode 100644 packages/tools/src/converter.spec.ts create mode 100644 packages/tools/src/converter.ts create mode 100644 packages/tools/src/getLoginExpiration.spec.ts create mode 100644 packages/tools/src/getLoginExpiration.ts diff --git a/.changeset/empty-readers-teach.md b/.changeset/empty-readers-teach.md new file mode 100644 index 000000000000..b4bd075ef654 --- /dev/null +++ b/.changeset/empty-readers-teach.md @@ -0,0 +1,8 @@ +--- +"@rocket.chat/meteor": patch +"@rocket.chat/tools": patch +"@rocket.chat/account-service": patch +--- + +Fixed an inconsistent evaluation of the `Accounts_LoginExpiration` setting over the codebase. In some places, it was being used as milliseconds while in others as days. Invalid values produced different results. A helper function was created to centralize the setting validation and the proper value being returned to avoid edge cases. +Negative values may be saved on the settings UI panel but the code will interpret any negative, NaN or 0 value to the default expiration which is 90 days. diff --git a/apps/meteor/app/api/server/v1/users.ts b/apps/meteor/app/api/server/v1/users.ts index c26957fa1991..041ef0410df0 100644 --- a/apps/meteor/app/api/server/v1/users.ts +++ b/apps/meteor/app/api/server/v1/users.ts @@ -19,6 +19,7 @@ import { isUsersCheckUsernameAvailabilityParamsGET, isUsersSendConfirmationEmailParamsPOST, } from '@rocket.chat/rest-typings'; +import { getLoginExpirationInMs } from '@rocket.chat/tools'; import { Accounts } from 'meteor/accounts-base'; import { Match, check } from 'meteor/check'; import { Meteor } from 'meteor/meteor'; @@ -1048,8 +1049,9 @@ API.v1.addRoute( const token = me.services?.resume?.loginTokens?.find((token) => token.hashedToken === hashedToken); - const tokenExpires = - (token && 'when' in token && new Date(token.when.getTime() + settings.get('Accounts_LoginExpiration') * 1000)) || undefined; + const loginExp = settings.get('Accounts_LoginExpiration'); + + const tokenExpires = (token && 'when' in token && new Date(token.when.getTime() + getLoginExpirationInMs(loginExp))) || undefined; return API.v1.success({ token: xAuthToken, diff --git a/apps/meteor/app/authentication/server/startup/index.js b/apps/meteor/app/authentication/server/startup/index.js index bffbe1f9876d..2e4c599ce558 100644 --- a/apps/meteor/app/authentication/server/startup/index.js +++ b/apps/meteor/app/authentication/server/startup/index.js @@ -2,6 +2,7 @@ import { Apps, AppEvents } from '@rocket.chat/apps'; import { User } from '@rocket.chat/core-services'; import { Roles, Settings, Users } from '@rocket.chat/models'; import { escapeRegExp, escapeHTML } from '@rocket.chat/string-helpers'; +import { getLoginExpirationInDays } from '@rocket.chat/tools'; import { Accounts } from 'meteor/accounts-base'; import { Match } from 'meteor/check'; import { Meteor } from 'meteor/meteor'; @@ -31,7 +32,7 @@ Accounts.config({ Meteor.startup(() => { settings.watchMultiple(['Accounts_LoginExpiration', 'Site_Name', 'From_Email'], () => { - Accounts._options.loginExpirationInDays = settings.get('Accounts_LoginExpiration'); + Accounts._options.loginExpirationInDays = getLoginExpirationInDays(settings.get('Accounts_LoginExpiration')); Accounts.emailTemplates.siteName = settings.get('Site_Name'); diff --git a/ee/apps/account-service/Dockerfile b/ee/apps/account-service/Dockerfile index c662d8765300..acbc5b0371d2 100644 --- a/ee/apps/account-service/Dockerfile +++ b/ee/apps/account-service/Dockerfile @@ -43,6 +43,9 @@ COPY ./ee/packages/license/dist packages/license/dist COPY ./packages/ui-kit/package.json packages/ui-kit/package.json COPY ./packages/ui-kit/dist packages/ui-kit/dist +COPY ./packages/tools/package.json packages/tools/package.json +COPY ./packages/tools/dist packages/tools/dist + COPY ./ee/apps/${SERVICE}/dist . COPY ./package.json . diff --git a/ee/apps/account-service/package.json b/ee/apps/account-service/package.json index c41932159e0e..194a3e1760e5 100644 --- a/ee/apps/account-service/package.json +++ b/ee/apps/account-service/package.json @@ -22,6 +22,7 @@ "@rocket.chat/models": "workspace:^", "@rocket.chat/rest-typings": "workspace:^", "@rocket.chat/string-helpers": "~0.31.25", + "@rocket.chat/tools": "workspace:^", "@types/node": "^14.18.63", "bcrypt": "^5.0.1", "ejson": "^2.2.3", diff --git a/ee/apps/account-service/src/Account.ts b/ee/apps/account-service/src/Account.ts index 4521574c12c7..c71376d301f3 100644 --- a/ee/apps/account-service/src/Account.ts +++ b/ee/apps/account-service/src/Account.ts @@ -1,6 +1,7 @@ import { ServiceClass } from '@rocket.chat/core-services'; import type { IAccount, ILoginResult } from '@rocket.chat/core-services'; import { Settings } from '@rocket.chat/models'; +import { getLoginExpirationInDays } from '@rocket.chat/tools'; import { loginViaResume } from './lib/loginViaResume'; import { loginViaUsername } from './lib/loginViaUsername'; @@ -22,9 +23,8 @@ export class Account extends ServiceClass implements IAccount { if (_id !== 'Accounts_LoginExpiration') { return; } - if (typeof value === 'number') { - this.loginExpiration = value; - } + + this.loginExpiration = getLoginExpirationInDays(value as number); }); } @@ -46,8 +46,7 @@ export class Account extends ServiceClass implements IAccount { async started(): Promise { const expiry = await Settings.findOne({ _id: 'Accounts_LoginExpiration' }, { projection: { value: 1 } }); - if (expiry?.value) { - this.loginExpiration = expiry.value as number; - } + + this.loginExpiration = getLoginExpirationInDays(expiry?.value as number); } } diff --git a/ee/apps/account-service/src/lib/utils.ts b/ee/apps/account-service/src/lib/utils.ts index 1d4a3a6ad580..f52f323606de 100644 --- a/ee/apps/account-service/src/lib/utils.ts +++ b/ee/apps/account-service/src/lib/utils.ts @@ -1,5 +1,6 @@ import crypto from 'crypto'; +import { convertFromDaysToMilliseconds } from '@rocket.chat/tools'; import bcrypt from 'bcrypt'; import { v4 as uuidv4 } from 'uuid'; @@ -60,4 +61,4 @@ export const validatePassword = (password: string, bcryptPassword: string): Prom bcrypt.compare(getPassword(password), bcryptPassword); export const _tokenExpiration = (when: string | Date, expirationInDays: number): Date => - new Date(new Date(when).getTime() + expirationInDays * 60 * 60 * 24 * 1000); + new Date(new Date(when).getTime() + convertFromDaysToMilliseconds(expirationInDays)); diff --git a/packages/tools/jest.config.ts b/packages/tools/jest.config.ts new file mode 100644 index 000000000000..959a31a7c6bf --- /dev/null +++ b/packages/tools/jest.config.ts @@ -0,0 +1,3 @@ +export default { + preset: 'ts-jest', +}; diff --git a/packages/tools/package.json b/packages/tools/package.json index e6bbae9b5be6..ac79955314b8 100644 --- a/packages/tools/package.json +++ b/packages/tools/package.json @@ -13,7 +13,9 @@ "lint": "eslint --ext .js,.jsx,.ts,.tsx .", "lint:fix": "eslint --ext .js,.jsx,.ts,.tsx . --fix", "test": "jest", + "test:cov": "jest --coverage", "build": "rm -rf dist && tsc -p tsconfig.json", + "testunit": "jest", "dev": "tsc -p tsconfig.json --watch --preserveWatchOutput" }, "main": "./dist/index.js", diff --git a/packages/tools/src/converter.spec.ts b/packages/tools/src/converter.spec.ts new file mode 100644 index 000000000000..5a27b6a97b95 --- /dev/null +++ b/packages/tools/src/converter.spec.ts @@ -0,0 +1,15 @@ +import { convertFromDaysToMilliseconds } from './converter'; + +describe('convertFromDaysToMilliseconds', () => { + it('should throw an error when a non number is passed', () => { + // @ts-expect-error - Testing + expect(() => convertFromDaysToMilliseconds('90')).toThrow(); + }); + it('should return the value passed when its valid', () => { + expect(convertFromDaysToMilliseconds(85)).toBe(85 * 24 * 60 * 60 * 1000); + }); + it('should fail if anything but an integer is passed', () => { + expect(() => convertFromDaysToMilliseconds(85.5)).toThrow(); + expect(() => convertFromDaysToMilliseconds(-2.3)).toThrow(); + }); +}); diff --git a/packages/tools/src/converter.ts b/packages/tools/src/converter.ts new file mode 100644 index 000000000000..e71c264857dc --- /dev/null +++ b/packages/tools/src/converter.ts @@ -0,0 +1,7 @@ +export const convertFromDaysToMilliseconds = (days: number) => { + if (typeof days !== 'number' || !Number.isInteger(days)) { + throw new Error('days must be a number'); + } + + return days * 24 * 60 * 60 * 1000; +}; diff --git a/packages/tools/src/getLoginExpiration.spec.ts b/packages/tools/src/getLoginExpiration.spec.ts new file mode 100644 index 000000000000..edd652172a5e --- /dev/null +++ b/packages/tools/src/getLoginExpiration.spec.ts @@ -0,0 +1,35 @@ +import { getLoginExpirationInDays, getLoginExpirationInMs } from './getLoginExpiration'; + +describe('getLoginExpirationInDays', () => { + it('should return 90 by default', () => { + expect(getLoginExpirationInDays()).toBe(90); + }); + it('should return 90 when value is 0', () => { + expect(getLoginExpirationInDays(0)).toBe(90); + }); + it('should return 90 when value is NaN', () => { + expect(getLoginExpirationInDays(NaN)).toBe(90); + }); + it('should return 90 when value is negative', () => { + expect(getLoginExpirationInDays(-1)).toBe(90); + }); + it('should return 90 when value is undefined', () => { + expect(getLoginExpirationInDays(undefined)).toBe(90); + }); + it('should return 90 when value is not a number', () => { + // @ts-expect-error - Testing + expect(getLoginExpirationInDays('90')).toBe(90); + }); + it('should return the value passed when its valid', () => { + expect(getLoginExpirationInDays(85)).toBe(85); + }); +}); + +describe('getLoginExpirationInMs', () => { + it('should return 90 days in milliseconds when no value is passed', () => { + expect(getLoginExpirationInMs()).toBe(90 * 24 * 60 * 60 * 1000); + }); + it('should return the value passed when its valid', () => { + expect(getLoginExpirationInMs(85)).toBe(85 * 24 * 60 * 60 * 1000); + }); +}); diff --git a/packages/tools/src/getLoginExpiration.ts b/packages/tools/src/getLoginExpiration.ts new file mode 100644 index 000000000000..de21fdce21a8 --- /dev/null +++ b/packages/tools/src/getLoginExpiration.ts @@ -0,0 +1,16 @@ +import { convertFromDaysToMilliseconds } from './converter'; + +const ACCOUNTS_DEFAULT_LOGIN_EXPIRATION_DAYS = 90; + +// Given a value, validates if it mets the conditions to be a valid login expiration. +// Else, returns the default login expiration (which for Meteor is 90 days) +export const getLoginExpirationInDays = (expiry?: number) => { + if (expiry && typeof expiry === 'number' && !Number.isNaN(expiry) && expiry > 0) { + return expiry; + } + return ACCOUNTS_DEFAULT_LOGIN_EXPIRATION_DAYS; +}; + +export const getLoginExpirationInMs = (expiry?: number) => { + return convertFromDaysToMilliseconds(getLoginExpirationInDays(expiry)); +}; diff --git a/packages/tools/src/index.ts b/packages/tools/src/index.ts index b1b53ab71a90..96faa4d55969 100644 --- a/packages/tools/src/index.ts +++ b/packages/tools/src/index.ts @@ -4,3 +4,5 @@ export * from './pick'; export * from './stream'; export * from './timezone'; export * from './wrapExceptions'; +export * from './getLoginExpiration'; +export * from './converter'; diff --git a/yarn.lock b/yarn.lock index 3b68dcde3806..add9a5e19468 100644 --- a/yarn.lock +++ b/yarn.lock @@ -8432,6 +8432,7 @@ __metadata: "@rocket.chat/models": "workspace:^" "@rocket.chat/rest-typings": "workspace:^" "@rocket.chat/string-helpers": ~0.31.25 + "@rocket.chat/tools": "workspace:^" "@types/bcrypt": ^5.0.1 "@types/gc-stats": ^1.4.2 "@types/node": ^14.18.63 From fa82159492bb1589372ba4f7dcca716da7a8a74b Mon Sep 17 00:00:00 2001 From: Matheus Barbosa Silva <36537004+matheusbsilva137@users.noreply.github.com> Date: Tue, 16 Jul 2024 18:01:41 -0300 Subject: [PATCH 5/5] fix: Agents can't leave omnichannel rooms that have already been closed (#32707) --- .changeset/happy-peaches-nail.md | 5 + .../lib/server/functions/closeLivechatRoom.ts | 81 +++++++++ .../meteor/app/livechat/server/api/v1/room.ts | 49 +----- .../app/livechat/server/methods/closeRoom.ts | 15 +- .../QuickActions/hooks/useQuickActions.tsx | 4 +- .../functions/closeLivechatRoom.tests.ts | 154 ++++++++++++++++++ 6 files changed, 256 insertions(+), 52 deletions(-) create mode 100644 .changeset/happy-peaches-nail.md create mode 100644 apps/meteor/app/lib/server/functions/closeLivechatRoom.ts create mode 100644 apps/meteor/tests/unit/app/lib/server/functions/closeLivechatRoom.tests.ts diff --git a/.changeset/happy-peaches-nail.md b/.changeset/happy-peaches-nail.md new file mode 100644 index 000000000000..2dfb2151ced0 --- /dev/null +++ b/.changeset/happy-peaches-nail.md @@ -0,0 +1,5 @@ +--- +"@rocket.chat/meteor": patch +--- + +Fixed issue with livechat agents not being able to leave omnichannel rooms if joining after a room has been closed by the visitor (due to race conditions) diff --git a/apps/meteor/app/lib/server/functions/closeLivechatRoom.ts b/apps/meteor/app/lib/server/functions/closeLivechatRoom.ts new file mode 100644 index 000000000000..b716be044d57 --- /dev/null +++ b/apps/meteor/app/lib/server/functions/closeLivechatRoom.ts @@ -0,0 +1,81 @@ +import type { IUser, IRoom, IOmnichannelRoom } from '@rocket.chat/core-typings'; +import { isOmnichannelRoom } from '@rocket.chat/core-typings'; +import { LivechatRooms, Subscriptions } from '@rocket.chat/models'; + +import { hasPermissionAsync } from '../../../authorization/server/functions/hasPermission'; +import type { CloseRoomParams } from '../../../livechat/server/lib/LivechatTyped'; +import { Livechat } from '../../../livechat/server/lib/LivechatTyped'; + +export const closeLivechatRoom = async ( + user: IUser, + roomId: IRoom['_id'], + { + comment, + tags, + generateTranscriptPdf, + transcriptEmail, + }: { + comment?: string; + tags?: string[]; + generateTranscriptPdf?: boolean; + transcriptEmail?: + | { + sendToVisitor: false; + } + | { + sendToVisitor: true; + requestData: Pick, 'email' | 'subject'>; + }; + }, +): Promise => { + const room = await LivechatRooms.findOneById(roomId); + if (!room || !isOmnichannelRoom(room)) { + throw new Error('error-invalid-room'); + } + + if (!room.open) { + const subscriptionsLeft = await Subscriptions.countByRoomId(roomId); + if (subscriptionsLeft) { + await Subscriptions.removeByRoomId(roomId); + return; + } + throw new Error('error-room-already-closed'); + } + + const subscription = await Subscriptions.findOneByRoomIdAndUserId(roomId, user._id, { projection: { _id: 1 } }); + if (!subscription && !(await hasPermissionAsync(user._id, 'close-others-livechat-room'))) { + throw new Error('error-not-authorized'); + } + + const options: CloseRoomParams['options'] = { + clientAction: true, + tags, + ...(generateTranscriptPdf && { pdfTranscript: { requestedBy: user._id } }), + ...(transcriptEmail && { + ...(transcriptEmail.sendToVisitor + ? { + emailTranscript: { + sendToVisitor: true, + requestData: { + email: transcriptEmail.requestData.email, + subject: transcriptEmail.requestData.subject, + requestedAt: new Date(), + requestedBy: user, + }, + }, + } + : { + emailTranscript: { + sendToVisitor: false, + }, + }), + }), + }; + + await Livechat.closeRoom({ + room, + user, + options, + comment, + }); +}; diff --git a/apps/meteor/app/livechat/server/api/v1/room.ts b/apps/meteor/app/livechat/server/api/v1/room.ts index 8a663fb0bd6d..94674f801ad5 100644 --- a/apps/meteor/app/livechat/server/api/v1/room.ts +++ b/apps/meteor/app/livechat/server/api/v1/room.ts @@ -1,7 +1,7 @@ import { Omnichannel } from '@rocket.chat/core-services'; import type { ILivechatAgent, IUser, SelectedAgent, TransferByData } from '@rocket.chat/core-typings'; import { isOmnichannelRoom, OmnichannelSourceType } from '@rocket.chat/core-typings'; -import { LivechatVisitors, Users, LivechatRooms, Subscriptions, Messages } from '@rocket.chat/models'; +import { LivechatVisitors, Users, LivechatRooms, Messages } from '@rocket.chat/models'; import { isLiveChatRoomForwardProps, isPOSTLivechatRoomCloseParams, @@ -21,6 +21,7 @@ import { isWidget } from '../../../../api/server/helpers/isWidget'; import { canAccessRoomAsync, roomAccessAttributes } from '../../../../authorization/server'; import { hasPermissionAsync } from '../../../../authorization/server/functions/hasPermission'; import { addUserToRoom } from '../../../../lib/server/functions/addUserToRoom'; +import { closeLivechatRoom } from '../../../../lib/server/functions/closeLivechatRoom'; import { settings as rcSettings } from '../../../../settings/server'; import { normalizeTransferredByData } from '../../lib/Helper'; import type { CloseRoomParams } from '../../lib/LivechatTyped'; @@ -178,51 +179,7 @@ API.v1.addRoute( async post() { const { rid, comment, tags, generateTranscriptPdf, transcriptEmail } = this.bodyParams; - const room = await LivechatRooms.findOneById(rid); - if (!room || !isOmnichannelRoom(room)) { - throw new Error('error-invalid-room'); - } - - if (!room.open) { - throw new Error('error-room-already-closed'); - } - - const subscription = await Subscriptions.findOneByRoomIdAndUserId(rid, this.userId, { projection: { _id: 1 } }); - if (!subscription && !(await hasPermissionAsync(this.userId, 'close-others-livechat-room'))) { - throw new Error('error-not-authorized'); - } - - const options: CloseRoomParams['options'] = { - clientAction: true, - tags, - ...(generateTranscriptPdf && { pdfTranscript: { requestedBy: this.userId } }), - ...(transcriptEmail && { - ...(transcriptEmail.sendToVisitor - ? { - emailTranscript: { - sendToVisitor: true, - requestData: { - email: transcriptEmail.requestData.email, - subject: transcriptEmail.requestData.subject, - requestedAt: new Date(), - requestedBy: this.user, - }, - }, - } - : { - emailTranscript: { - sendToVisitor: false, - }, - }), - }), - }; - - await LivechatTyped.closeRoom({ - room, - user: this.user, - options, - comment, - }); + await closeLivechatRoom(this.user, rid, { comment, tags, generateTranscriptPdf, transcriptEmail }); return API.v1.success(); }, diff --git a/apps/meteor/app/livechat/server/methods/closeRoom.ts b/apps/meteor/app/livechat/server/methods/closeRoom.ts index 1374d86ab9f7..5fdf9e7d504f 100644 --- a/apps/meteor/app/livechat/server/methods/closeRoom.ts +++ b/apps/meteor/app/livechat/server/methods/closeRoom.ts @@ -60,6 +60,16 @@ Meteor.methods({ }); } + const subscription = await SubscriptionRaw.findOneByRoomIdAndUserId(roomId, userId, { + projection: { + _id: 1, + }, + }); + if (!room.open && subscription) { + await SubscriptionRaw.removeByRoomId(roomId); + return; + } + if (!room.open) { throw new Meteor.Error('room-closed', 'Room closed', { method: 'livechat:closeRoom' }); } @@ -71,11 +81,6 @@ Meteor.methods({ }); } - const subscription = await SubscriptionRaw.findOneByRoomIdAndUserId(roomId, user._id, { - projection: { - _id: 1, - }, - }); if (!subscription && !(await hasPermissionAsync(userId, 'close-others-livechat-room'))) { throw new Meteor.Error('error-not-authorized', 'Not authorized', { method: 'livechat:closeRoom', diff --git a/apps/meteor/client/views/room/Header/Omnichannel/QuickActions/hooks/useQuickActions.tsx b/apps/meteor/client/views/room/Header/Omnichannel/QuickActions/hooks/useQuickActions.tsx index 7446d0630b09..edf5ffcbdc8a 100644 --- a/apps/meteor/client/views/room/Header/Omnichannel/QuickActions/hooks/useQuickActions.tsx +++ b/apps/meteor/client/views/room/Header/Omnichannel/QuickActions/hooks/useQuickActions.tsx @@ -10,6 +10,7 @@ import { useMethod, useTranslation, useRouter, + useUserSubscription, } from '@rocket.chat/ui-contexts'; import React, { useCallback, useState, useEffect } from 'react'; @@ -47,6 +48,7 @@ export const useQuickActions = (): { const visitorRoomId = room.v._id; const rid = room._id; const uid = useUserId(); + const subscription = useUserSubscription(rid); const roomLastMessage = room.lastMessage; const getVisitorInfo = useEndpoint('GET', '/v1/livechat/visitors.info'); @@ -330,7 +332,7 @@ export const useQuickActions = (): { case QuickActionsEnum.TranscriptPDF: return hasLicense && !isRoomOverMacLimit && canSendTranscriptPDF; case QuickActionsEnum.CloseChat: - return !!roomOpen && (canCloseRoom || canCloseOthersRoom); + return (subscription && (canCloseRoom || canCloseOthersRoom)) || (!!roomOpen && canCloseOthersRoom); case QuickActionsEnum.OnHoldChat: return !!roomOpen && canPlaceChatOnHold; default: diff --git a/apps/meteor/tests/unit/app/lib/server/functions/closeLivechatRoom.tests.ts b/apps/meteor/tests/unit/app/lib/server/functions/closeLivechatRoom.tests.ts new file mode 100644 index 000000000000..07ee437832d2 --- /dev/null +++ b/apps/meteor/tests/unit/app/lib/server/functions/closeLivechatRoom.tests.ts @@ -0,0 +1,154 @@ +import { expect } from 'chai'; +import { describe, it } from 'mocha'; +import proxyquire from 'proxyquire'; +import sinon from 'sinon'; + +import { createFakeRoom, createFakeSubscription, createFakeUser } from '../../../../../mocks/data'; + +const subscriptionsStub = { + findOneByRoomIdAndUserId: sinon.stub(), + removeByRoomId: sinon.stub(), + countByRoomId: sinon.stub(), +}; + +const livechatRoomsStub = { + findOneById: sinon.stub(), +}; + +const livechatStub = { + closeRoom: sinon.stub(), +}; + +const hasPermissionStub = sinon.stub(); + +const { closeLivechatRoom } = proxyquire.noCallThru().load('../../../../../../app/lib/server/functions/closeLivechatRoom.ts', { + '../../../livechat/server/lib/LivechatTyped': { + Livechat: livechatStub, + }, + '../../../authorization/server/functions/hasPermission': { + hasPermissionAsync: hasPermissionStub, + }, + '@rocket.chat/models': { + Subscriptions: subscriptionsStub, + LivechatRooms: livechatRoomsStub, + }, +}); + +describe('closeLivechatRoom', () => { + const user = createFakeUser(); + const room = createFakeRoom({ t: 'l', open: true }); + const subscription = createFakeSubscription({ rid: room._id, u: { username: user.username, _id: user._id } }); + + beforeEach(() => { + subscriptionsStub.findOneByRoomIdAndUserId.reset(); + subscriptionsStub.removeByRoomId.reset(); + subscriptionsStub.countByRoomId.reset(); + livechatRoomsStub.findOneById.reset(); + livechatStub.closeRoom.reset(); + hasPermissionStub.reset(); + }); + + it('should not perform any operation when an invalid room id is provided', async () => { + livechatRoomsStub.findOneById.resolves(null); + hasPermissionStub.resolves(true); + + await expect(closeLivechatRoom(user, room._id, {})).to.be.rejectedWith('error-invalid-room'); + expect(livechatStub.closeRoom.notCalled).to.be.true; + expect(livechatRoomsStub.findOneById.calledOnceWith(room._id)).to.be.true; + expect(subscriptionsStub.findOneByRoomIdAndUserId.notCalled).to.be.true; + expect(subscriptionsStub.removeByRoomId.notCalled).to.be.true; + }); + + it('should not perform any operation when a non-livechat room is provided', async () => { + livechatRoomsStub.findOneById.resolves({ ...room, t: 'c' }); + subscriptionsStub.findOneByRoomIdAndUserId.resolves(subscription); + hasPermissionStub.resolves(true); + + await expect(closeLivechatRoom(user, room._id, {})).to.be.rejectedWith('error-invalid-room'); + expect(livechatStub.closeRoom.notCalled).to.be.true; + expect(livechatRoomsStub.findOneById.calledOnceWith(room._id)).to.be.true; + expect(subscriptionsStub.findOneByRoomIdAndUserId.notCalled).to.be.true; + expect(subscriptionsStub.removeByRoomId.notCalled).to.be.true; + }); + + it('should not perform any operation when a closed room with no subscriptions is provided and the caller is not subscribed to it', async () => { + livechatRoomsStub.findOneById.resolves({ ...room, open: false }); + subscriptionsStub.countByRoomId.resolves(0); + subscriptionsStub.findOneByRoomIdAndUserId.resolves(null); + hasPermissionStub.resolves(true); + + await expect(closeLivechatRoom(user, room._id, {})).to.be.rejectedWith('error-room-already-closed'); + expect(livechatStub.closeRoom.notCalled).to.be.true; + expect(livechatRoomsStub.findOneById.calledOnceWith(room._id)).to.be.true; + expect(subscriptionsStub.findOneByRoomIdAndUserId.notCalled).to.be.true; + expect(subscriptionsStub.countByRoomId.calledOnceWith(room._id)).to.be.true; + expect(subscriptionsStub.removeByRoomId.notCalled).to.be.true; + }); + + it('should remove dangling subscription when a closed room with subscriptions is provided and the caller is not subscribed to it', async () => { + livechatRoomsStub.findOneById.resolves({ ...room, open: false }); + subscriptionsStub.countByRoomId.resolves(1); + subscriptionsStub.findOneByRoomIdAndUserId.resolves(null); + hasPermissionStub.resolves(true); + + await closeLivechatRoom(user, room._id, {}); + expect(livechatStub.closeRoom.notCalled).to.be.true; + expect(livechatRoomsStub.findOneById.calledOnceWith(room._id)).to.be.true; + expect(subscriptionsStub.findOneByRoomIdAndUserId.notCalled).to.be.true; + expect(subscriptionsStub.countByRoomId.calledOnceWith(room._id)).to.be.true; + expect(subscriptionsStub.removeByRoomId.calledOnceWith(room._id)).to.be.true; + }); + + it('should remove dangling subscription when a closed room is provided but the user is still subscribed to it', async () => { + livechatRoomsStub.findOneById.resolves({ ...room, open: false }); + subscriptionsStub.findOneByRoomIdAndUserId.resolves(subscription); + subscriptionsStub.countByRoomId.resolves(1); + hasPermissionStub.resolves(true); + + await closeLivechatRoom(user, room._id, {}); + expect(livechatStub.closeRoom.notCalled).to.be.true; + expect(livechatRoomsStub.findOneById.calledOnceWith(room._id)).to.be.true; + expect(subscriptionsStub.findOneByRoomIdAndUserId.notCalled).to.be.true; + expect(subscriptionsStub.countByRoomId.calledOnceWith(room._id)).to.be.true; + expect(subscriptionsStub.removeByRoomId.calledOnceWith(room._id)).to.be.true; + }); + + it('should not perform any operation when the caller is not subscribed to an open room and does not have the permission to close others rooms', async () => { + livechatRoomsStub.findOneById.resolves(room); + subscriptionsStub.findOneByRoomIdAndUserId.resolves(null); + subscriptionsStub.countByRoomId.resolves(1); + hasPermissionStub.resolves(false); + + await expect(closeLivechatRoom(user, room._id, {})).to.be.rejectedWith('error-not-authorized'); + expect(livechatStub.closeRoom.notCalled).to.be.true; + expect(livechatRoomsStub.findOneById.calledOnceWith(room._id)).to.be.true; + expect(subscriptionsStub.findOneByRoomIdAndUserId.calledOnceWith(room._id, user._id)).to.be.true; + expect(subscriptionsStub.removeByRoomId.notCalled).to.be.true; + }); + + it('should close the room when the caller is not subscribed to it but has the permission to close others rooms', async () => { + livechatRoomsStub.findOneById.resolves(room); + subscriptionsStub.findOneByRoomIdAndUserId.resolves(null); + subscriptionsStub.countByRoomId.resolves(1); + hasPermissionStub.resolves(true); + + await closeLivechatRoom(user, room._id, {}); + expect(livechatStub.closeRoom.calledOnceWith(sinon.match({ room, user }))).to.be.true; + expect(livechatRoomsStub.findOneById.calledOnceWith(room._id)).to.be.true; + expect(subscriptionsStub.findOneByRoomIdAndUserId.calledOnceWith(room._id, user._id)).to.be.true; + expect(subscriptionsStub.removeByRoomId.notCalled).to.be.true; + }); + + it('should close the room when the caller is subscribed to it and does not have the permission to close others rooms', async () => { + livechatRoomsStub.findOneById.resolves(room); + subscriptionsStub.findOneByRoomIdAndUserId.resolves(subscription); + subscriptionsStub.countByRoomId.resolves(1); + hasPermissionStub.resolves(false); + + await closeLivechatRoom(user, room._id, {}); + expect(livechatStub.closeRoom.calledOnceWith(sinon.match({ room, user }))).to.be.true; + expect(livechatRoomsStub.findOneById.calledOnceWith(room._id)).to.be.true; + expect(subscriptionsStub.findOneByRoomIdAndUserId.calledOnceWith(room._id, user._id)).to.be.true; + expect(subscriptionsStub.removeByRoomId.notCalled).to.be.true; + }); +});