From 2f538de4446b85dde8b2f4dc4a9394c310dbb90a Mon Sep 17 00:00:00 2001 From: RIcardo Garim Date: Fri, 19 Apr 2024 17:42:09 -0300 Subject: [PATCH 1/6] refactor: add roles out of db watcher --- apps/meteor/ee/server/api/roles.ts | 8 +--- apps/meteor/ee/server/lib/roles/insertRole.ts | 20 ++++---- apps/meteor/ee/server/lib/roles/updateRole.ts | 9 +++- .../.npm/package/npm-shrinkwrap.json | 2 +- .../server/database/watchCollections.ts | 4 +- .../lib/roles/createOrUpdateProtectedRole.ts | 2 + apps/meteor/server/models/raw/Roles.ts | 47 +++++++++++++------ .../modules/listeners/listeners.module.ts | 9 ++-- .../modules/watchers/watchers.module.ts | 6 +++ .../model-typings/src/models/IRolesModel.ts | 6 +-- 10 files changed, 70 insertions(+), 43 deletions(-) diff --git a/apps/meteor/ee/server/api/roles.ts b/apps/meteor/ee/server/api/roles.ts index 7a71613b3548..7e8048387ec3 100644 --- a/apps/meteor/ee/server/api/roles.ts +++ b/apps/meteor/ee/server/api/roles.ts @@ -130,9 +130,7 @@ API.v1.addRoute( const role = await insertRoleAsync(roleData, options); - return API.v1.success({ - role, - }); + return API.v1.success({ role }); }, }, ); @@ -172,9 +170,7 @@ API.v1.addRoute( const updatedRole = await updateRole(roleId, roleData, options); - return API.v1.success({ - role: updatedRole, - }); + return API.v1.success({ role: updatedRole }); }, }, ); diff --git a/apps/meteor/ee/server/lib/roles/insertRole.ts b/apps/meteor/ee/server/lib/roles/insertRole.ts index 80aa4e57385e..f435941a2ec4 100644 --- a/apps/meteor/ee/server/lib/roles/insertRole.ts +++ b/apps/meteor/ee/server/lib/roles/insertRole.ts @@ -1,4 +1,4 @@ -import { api, MeteorError } from '@rocket.chat/core-services'; +import { api, MeteorError, dbWatchersDisabled } from '@rocket.chat/core-services'; import type { IRole } from '@rocket.chat/core-typings'; import { Roles } from '@rocket.chat/models'; @@ -19,21 +19,21 @@ export const insertRoleAsync = async (roleData: Omit, options: Ins throw new MeteorError('error-invalid-scope', 'Invalid scope'); } - const result = await Roles.createWithRandomId(name, scope, description, false, mandatory2fa); + const role = await Roles.createWithRandomId(name, scope, description, false, mandatory2fa); - const roleId = result.insertedId; + if (dbWatchersDisabled) { + void api.broadcast('watch.roles', { + clientAction: 'inserted', + role, + }); + } if (options.broadcastUpdate) { void api.broadcast('user.roleUpdate', { type: 'changed', - _id: roleId, + _id: role._id, }); } - const newRole = await Roles.findOneById(roleId); - if (!newRole) { - throw new MeteorError('error-role-not-found', 'Role not found'); - } - - return newRole; + return role; }; diff --git a/apps/meteor/ee/server/lib/roles/updateRole.ts b/apps/meteor/ee/server/lib/roles/updateRole.ts index e3a9a7063526..9d31f89ba9bf 100644 --- a/apps/meteor/ee/server/lib/roles/updateRole.ts +++ b/apps/meteor/ee/server/lib/roles/updateRole.ts @@ -1,4 +1,4 @@ -import { api, MeteorError } from '@rocket.chat/core-services'; +import { api, MeteorError, dbWatchersDisabled } from '@rocket.chat/core-services'; import type { IRole } from '@rocket.chat/core-typings'; import { Roles } from '@rocket.chat/models'; @@ -38,6 +38,13 @@ export const updateRole = async (roleId: IRole['_id'], roleData: Omit 0) { diff --git a/apps/meteor/server/lib/roles/createOrUpdateProtectedRole.ts b/apps/meteor/server/lib/roles/createOrUpdateProtectedRole.ts index 9361689d2b5f..60e1c758aac8 100644 --- a/apps/meteor/server/lib/roles/createOrUpdateProtectedRole.ts +++ b/apps/meteor/server/lib/roles/createOrUpdateProtectedRole.ts @@ -1,6 +1,8 @@ import type { IRole, AtLeast } from '@rocket.chat/core-typings'; import { Roles } from '@rocket.chat/models'; +// TODO: roles-out-of-dbwatcher +// No need to broadcast from here since this function will only iterate over default roles export const createOrUpdateProtectedRoleAsync = async ( roleId: string, roleData: AtLeast, 'name'>, diff --git a/apps/meteor/server/models/raw/Roles.ts b/apps/meteor/server/models/raw/Roles.ts index 355faae256e5..94e3fd9df00a 100644 --- a/apps/meteor/server/models/raw/Roles.ts +++ b/apps/meteor/server/models/raw/Roles.ts @@ -1,7 +1,7 @@ import type { IRole, IRoom, IUser, RocketChatRecordDeleted } from '@rocket.chat/core-typings'; import type { IRolesModel } from '@rocket.chat/model-typings'; import { Subscriptions, Users } from '@rocket.chat/models'; -import type { Collection, FindCursor, Db, Filter, FindOptions, InsertOneResult, UpdateResult, WithId, Document } from 'mongodb'; +import type { Collection, FindCursor, Db, Filter, FindOptions, Document } from 'mongodb'; import { BaseRaw } from './BaseRaw'; @@ -190,21 +190,31 @@ export class RolesRaw extends BaseRaw implements IRolesModel { return this.find(query, options || {}); } - updateById( + async updateById( _id: IRole['_id'], name: IRole['name'], scope: IRole['scope'], description: IRole['description'] = '', mandatory2fa: IRole['mandatory2fa'] = false, - ): Promise { - const queryData = { - name, - scope, - description, - mandatory2fa, - }; + ): Promise { + const response = await this.findOneAndUpdate( + { _id }, + { + $set: { + name, + scope, + description, + mandatory2fa, + }, + }, + { upsert: true, returnDocument: 'after' }, + ); + + if (!response.value) { + throw new Error('Role not found'); + } - return this.updateOne({ _id }, { $set: queryData }, { upsert: true }); + return response.value; } findUsersInRole(roleId: IRole['_id'], scope?: IRoom['_id']): Promise>; @@ -242,22 +252,29 @@ export class RolesRaw extends BaseRaw implements IRolesModel { } } - createWithRandomId( + async createWithRandomId( name: IRole['name'], scope: IRole['scope'] = 'Users', description = '', protectedRole = true, mandatory2fa = false, - ): Promise>> { - const role = { + ): Promise { + const res = await this.insertOne({ name, scope, description, protected: protectedRole, mandatory2fa, - }; + }); - return this.insertOne(role); + return { + _id: res.insertedId as IRole['_id'], + name, + scope, + description, + protected: protectedRole, + mandatory2fa, + }; } async canAddUserToRole(uid: IUser['_id'], roleId: IRole['_id'], scope?: IRoom['_id']): Promise { diff --git a/apps/meteor/server/modules/listeners/listeners.module.ts b/apps/meteor/server/modules/listeners/listeners.module.ts index 68e616a233c8..17b26fd9183d 100644 --- a/apps/meteor/server/modules/listeners/listeners.module.ts +++ b/apps/meteor/server/modules/listeners/listeners.module.ts @@ -3,7 +3,7 @@ import type { ISetting as AppsSetting } from '@rocket.chat/apps-engine/definitio import type { IServiceClass } from '@rocket.chat/core-services'; import { EnterpriseSettings } from '@rocket.chat/core-services'; import { isSettingColor, isSettingEnterprise, UserStatus } from '@rocket.chat/core-typings'; -import type { IUser, IRoom, VideoConference, ISetting, IOmnichannelRoom, IMessage, IOTRMessage } from '@rocket.chat/core-typings'; +import type { IUser, IRoom, IRole, VideoConference, ISetting, IOmnichannelRoom, IMessage, IOTRMessage } from '@rocket.chat/core-typings'; import { Logger } from '@rocket.chat/logger'; import { parse } from '@rocket.chat/message-parser'; @@ -201,11 +201,10 @@ export class ListenersModule { }); service.onEvent('watch.roles', ({ clientAction, role }): void => { - const payload = { + notifications.streamRoles.emitWithoutBroadcast('roles', { type: clientAction, - ...role, - }; - notifications.streamRoles.emitWithoutBroadcast('roles', payload as any); + ...(role as IRole), + }); }); service.onEvent('watch.inquiries', async ({ clientAction, inquiry, diff }): Promise => { diff --git a/apps/meteor/server/modules/watchers/watchers.module.ts b/apps/meteor/server/modules/watchers/watchers.module.ts index 05a393c9ada6..097d4f2699b6 100644 --- a/apps/meteor/server/modules/watchers/watchers.module.ts +++ b/apps/meteor/server/modules/watchers/watchers.module.ts @@ -191,6 +191,12 @@ export function initWatchers(watcher: DatabaseWatcher, broadcast: BroadcastCallb clientAction: 'changed', role, }); + + console.log({ + fromWatchers: true, + clientAction: 'changed', + role, + }); }); watcher.on(LivechatInquiry.getCollectionName(), async ({ clientAction, id, data, diff }) => { diff --git a/packages/model-typings/src/models/IRolesModel.ts b/packages/model-typings/src/models/IRolesModel.ts index 43ff1fc2f9e6..a6e8354a0bc0 100644 --- a/packages/model-typings/src/models/IRolesModel.ts +++ b/packages/model-typings/src/models/IRolesModel.ts @@ -1,5 +1,5 @@ import type { IRole, IUser, IRoom } from '@rocket.chat/core-typings'; -import type { FindCursor, FindOptions, InsertOneResult, UpdateResult, WithId } from 'mongodb'; +import type { FindCursor, FindOptions } from 'mongodb'; import type { IBaseModel } from './IBaseModel'; @@ -32,7 +32,7 @@ export interface IRolesModel extends IBaseModel { scope: IRole['scope'], description?: IRole['description'], mandatory2fa?: IRole['mandatory2fa'], - ): Promise; + ): Promise; findUsersInRole(roleId: IRole['_id'], scope?: IRoom['_id']): Promise>; findUsersInRole(roleId: IRole['_id'], scope: IRoom['_id'] | undefined, options: FindOptions): Promise>; @@ -58,7 +58,7 @@ export interface IRolesModel extends IBaseModel { description?: string, protectedRole?: boolean, mandatory2fa?: boolean, - ): Promise>>; + ): Promise; canAddUserToRole(uid: IUser['_id'], roleId: IRole['_id'], scope?: IRoom['_id']): Promise; } From e7b0fa8c71f3d717bffb202d2f207404398e5ee5 Mon Sep 17 00:00:00 2001 From: RIcardo Garim Date: Sun, 21 Apr 2024 23:12:48 -0300 Subject: [PATCH 2/6] reverts the state of files to erase logs and new lines --- .../packages/flow-router/.npm/package/npm-shrinkwrap.json | 2 +- apps/meteor/server/modules/watchers/watchers.module.ts | 6 ------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/apps/meteor/packages/flow-router/.npm/package/npm-shrinkwrap.json b/apps/meteor/packages/flow-router/.npm/package/npm-shrinkwrap.json index 8110d45c9f30..47445b724946 100644 --- a/apps/meteor/packages/flow-router/.npm/package/npm-shrinkwrap.json +++ b/apps/meteor/packages/flow-router/.npm/package/npm-shrinkwrap.json @@ -25,4 +25,4 @@ "integrity": "sha512-VH4FeG98gs6AkHivaW2O14vsOPBL9E80Sj7fITunoDijiYQ1lsVwJYmm1CSL+oLyO2N5HPdo23GXAG64uKOAZQ==" } } -} +} \ No newline at end of file diff --git a/apps/meteor/server/modules/watchers/watchers.module.ts b/apps/meteor/server/modules/watchers/watchers.module.ts index 097d4f2699b6..05a393c9ada6 100644 --- a/apps/meteor/server/modules/watchers/watchers.module.ts +++ b/apps/meteor/server/modules/watchers/watchers.module.ts @@ -191,12 +191,6 @@ export function initWatchers(watcher: DatabaseWatcher, broadcast: BroadcastCallb clientAction: 'changed', role, }); - - console.log({ - fromWatchers: true, - clientAction: 'changed', - role, - }); }); watcher.on(LivechatInquiry.getCollectionName(), async ({ clientAction, id, data, diff }) => { From afd2fdff914785a5a951556a8e73e058855ab187 Mon Sep 17 00:00:00 2001 From: RIcardo Garim Date: Sun, 28 Apr 2024 23:19:32 -0300 Subject: [PATCH 3/6] refactor: adds listener calls roles insert, update, and delete routes --- apps/meteor/app/api/server/v1/roles.ts | 3 +++ .../server/lib/notifyListenerOnRoleChanges.ts | 23 +++++++++++++++++++ apps/meteor/ee/server/lib/roles/insertRole.ts | 10 +++----- apps/meteor/ee/server/lib/roles/updateRole.ts | 10 +++----- 4 files changed, 32 insertions(+), 14 deletions(-) create mode 100644 apps/meteor/app/lib/server/lib/notifyListenerOnRoleChanges.ts diff --git a/apps/meteor/app/api/server/v1/roles.ts b/apps/meteor/app/api/server/v1/roles.ts index a0fff9683b80..6727f4f970cb 100644 --- a/apps/meteor/app/api/server/v1/roles.ts +++ b/apps/meteor/app/api/server/v1/roles.ts @@ -9,6 +9,7 @@ import { getUsersInRolePaginated } from '../../../authorization/server/functions import { hasPermissionAsync } from '../../../authorization/server/functions/hasPermission'; import { hasRoleAsync, hasAnyRoleAsync } from '../../../authorization/server/functions/hasRole'; import { apiDeprecationLogger } from '../../../lib/server/lib/deprecationWarningLogger'; +import { notifyListenerOnRoleChanges } from '../../../lib/server/lib/notifyListenerOnRoleChanges'; import { settings } from '../../../settings/server/index'; import { API } from '../api'; import { getPaginationItems } from '../helpers/getPaginationItems'; @@ -179,6 +180,8 @@ API.v1.addRoute( await Roles.removeById(role._id); + void notifyListenerOnRoleChanges(role._id, 'removed', role); + return API.v1.success(); }, }, diff --git a/apps/meteor/app/lib/server/lib/notifyListenerOnRoleChanges.ts b/apps/meteor/app/lib/server/lib/notifyListenerOnRoleChanges.ts new file mode 100644 index 000000000000..159fa9f59e8d --- /dev/null +++ b/apps/meteor/app/lib/server/lib/notifyListenerOnRoleChanges.ts @@ -0,0 +1,23 @@ +import { api, dbWatchersDisabled } from '@rocket.chat/core-services'; +import type { IRole } from '@rocket.chat/core-typings'; +import { Roles } from '@rocket.chat/models'; + +type ClientAction = 'inserted' | 'updated' | 'removed'; + +export async function notifyListenerOnRoleChanges( + rid: IRole['_id'], + clientAction: ClientAction = 'updated', + existingRoleData?: IRole, + args?: { [key: string]: any }, +): Promise { + if (!dbWatchersDisabled) return; + + const role = existingRoleData || (await Roles.findOneById(rid)); + + if (role) { + void api.broadcast('watch.roles', { + clientAction, + role: { ...role, ...args }, + }); + } +} diff --git a/apps/meteor/ee/server/lib/roles/insertRole.ts b/apps/meteor/ee/server/lib/roles/insertRole.ts index f435941a2ec4..23cb394a913c 100644 --- a/apps/meteor/ee/server/lib/roles/insertRole.ts +++ b/apps/meteor/ee/server/lib/roles/insertRole.ts @@ -1,7 +1,8 @@ -import { api, MeteorError, dbWatchersDisabled } from '@rocket.chat/core-services'; +import { api, MeteorError } from '@rocket.chat/core-services'; import type { IRole } from '@rocket.chat/core-typings'; import { Roles } from '@rocket.chat/models'; +import { notifyListenerOnRoleChanges } from '../../../../app/lib/server/lib/notifyListenerOnRoleChanges'; import { isValidRoleScope } from '../../../../lib/roles/isValidRoleScope'; type InsertRoleOptions = { @@ -21,12 +22,7 @@ export const insertRoleAsync = async (roleData: Omit, options: Ins const role = await Roles.createWithRandomId(name, scope, description, false, mandatory2fa); - if (dbWatchersDisabled) { - void api.broadcast('watch.roles', { - clientAction: 'inserted', - role, - }); - } + void notifyListenerOnRoleChanges(role._id, 'inserted', role); if (options.broadcastUpdate) { void api.broadcast('user.roleUpdate', { diff --git a/apps/meteor/ee/server/lib/roles/updateRole.ts b/apps/meteor/ee/server/lib/roles/updateRole.ts index 9d31f89ba9bf..976abbd8921f 100644 --- a/apps/meteor/ee/server/lib/roles/updateRole.ts +++ b/apps/meteor/ee/server/lib/roles/updateRole.ts @@ -1,7 +1,8 @@ -import { api, MeteorError, dbWatchersDisabled } from '@rocket.chat/core-services'; +import { api, MeteorError } from '@rocket.chat/core-services'; import type { IRole } from '@rocket.chat/core-typings'; import { Roles } from '@rocket.chat/models'; +import { notifyListenerOnRoleChanges } from '../../../../app/lib/server/lib/notifyListenerOnRoleChanges'; import { isValidRoleScope } from '../../../../lib/roles/isValidRoleScope'; type UpdateRoleOptions = { @@ -38,12 +39,7 @@ export const updateRole = async (roleId: IRole['_id'], roleData: Omit Date: Wed, 1 May 2024 15:00:02 -0300 Subject: [PATCH 4/6] refactor: minors on helper notifyListenerOnRoleChanges function signature and Roles model method call --- .../lib/server/lib/notifyListenerOnRoleChanges.ts | 7 ++++--- apps/meteor/server/models/raw/Roles.ts | 14 ++++++-------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/apps/meteor/app/lib/server/lib/notifyListenerOnRoleChanges.ts b/apps/meteor/app/lib/server/lib/notifyListenerOnRoleChanges.ts index 159fa9f59e8d..b05e54af3625 100644 --- a/apps/meteor/app/lib/server/lib/notifyListenerOnRoleChanges.ts +++ b/apps/meteor/app/lib/server/lib/notifyListenerOnRoleChanges.ts @@ -8,16 +8,17 @@ export async function notifyListenerOnRoleChanges( rid: IRole['_id'], clientAction: ClientAction = 'updated', existingRoleData?: IRole, - args?: { [key: string]: any }, ): Promise { - if (!dbWatchersDisabled) return; + if (!dbWatchersDisabled) { + return; + } const role = existingRoleData || (await Roles.findOneById(rid)); if (role) { void api.broadcast('watch.roles', { clientAction, - role: { ...role, ...args }, + role, }); } } diff --git a/apps/meteor/server/models/raw/Roles.ts b/apps/meteor/server/models/raw/Roles.ts index 94e3fd9df00a..84a5b088ea30 100644 --- a/apps/meteor/server/models/raw/Roles.ts +++ b/apps/meteor/server/models/raw/Roles.ts @@ -259,21 +259,19 @@ export class RolesRaw extends BaseRaw implements IRolesModel { protectedRole = true, mandatory2fa = false, ): Promise { - const res = await this.insertOne({ + const role = { name, scope, description, protected: protectedRole, mandatory2fa, - }); + }; + + const res = await this.insertOne(role); return { - _id: res.insertedId as IRole['_id'], - name, - scope, - description, - protected: protectedRole, - mandatory2fa, + _id: res.insertedId, + ...role, }; } From 07783163ce90f3124c90763d0d98b464f236ba97 Mon Sep 17 00:00:00 2001 From: RIcardo Garim Date: Wed, 1 May 2024 15:23:11 -0300 Subject: [PATCH 5/6] refactor: adds listener calls on createOrUpdateProtectedRole function --- .../server/lib/roles/createOrUpdateProtectedRole.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/apps/meteor/server/lib/roles/createOrUpdateProtectedRole.ts b/apps/meteor/server/lib/roles/createOrUpdateProtectedRole.ts index 60e1c758aac8..aa075a1e0366 100644 --- a/apps/meteor/server/lib/roles/createOrUpdateProtectedRole.ts +++ b/apps/meteor/server/lib/roles/createOrUpdateProtectedRole.ts @@ -1,7 +1,8 @@ import type { IRole, AtLeast } from '@rocket.chat/core-typings'; import { Roles } from '@rocket.chat/models'; -// TODO: roles-out-of-dbwatcher +import { notifyListenerOnRoleChanges } from '../../../app/lib/server/lib/notifyListenerOnRoleChanges'; + // No need to broadcast from here since this function will only iterate over default roles export const createOrUpdateProtectedRoleAsync = async ( roleId: string, @@ -10,8 +11,9 @@ export const createOrUpdateProtectedRoleAsync = async ( const role = await Roles.findOneById>(roleId, { projection: { name: 1, scope: 1, description: 1, mandatory2fa: 1 }, }); + if (role) { - await Roles.updateById( + const updatedRole = await Roles.updateById( roleId, roleData.name || role.name, roleData.scope || role.scope, @@ -19,10 +21,12 @@ export const createOrUpdateProtectedRoleAsync = async ( roleData.mandatory2fa || role.mandatory2fa, ); + void notifyListenerOnRoleChanges(updatedRole._id); + return; } - await Roles.insertOne({ + const insertedRole = await Roles.insertOne({ _id: roleId, scope: 'Users', description: '', @@ -30,4 +34,6 @@ export const createOrUpdateProtectedRoleAsync = async ( ...roleData, protected: true, }); + + void notifyListenerOnRoleChanges(insertedRole.insertedId, 'inserted'); }; From 91adf27f23de0e2bfa92f46a59d8859588c916a2 Mon Sep 17 00:00:00 2001 From: Diego Sampaio Date: Wed, 8 May 2024 15:50:48 -0300 Subject: [PATCH 6/6] fix review --- apps/meteor/server/lib/roles/createOrUpdateProtectedRole.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/apps/meteor/server/lib/roles/createOrUpdateProtectedRole.ts b/apps/meteor/server/lib/roles/createOrUpdateProtectedRole.ts index aa075a1e0366..487c74c0f76b 100644 --- a/apps/meteor/server/lib/roles/createOrUpdateProtectedRole.ts +++ b/apps/meteor/server/lib/roles/createOrUpdateProtectedRole.ts @@ -3,7 +3,6 @@ import { Roles } from '@rocket.chat/models'; import { notifyListenerOnRoleChanges } from '../../../app/lib/server/lib/notifyListenerOnRoleChanges'; -// No need to broadcast from here since this function will only iterate over default roles export const createOrUpdateProtectedRoleAsync = async ( roleId: string, roleData: AtLeast, 'name'>, @@ -21,7 +20,7 @@ export const createOrUpdateProtectedRoleAsync = async ( roleData.mandatory2fa || role.mandatory2fa, ); - void notifyListenerOnRoleChanges(updatedRole._id); + void notifyListenerOnRoleChanges(roleId, 'updated', updatedRole); return; }