From e623315952b4afe869836d2972a2f9f0282112fb Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Fri, 3 Jul 2026 15:22:13 -0600 Subject: [PATCH 01/11] refactor: collapse check-then-write model flows into atomic operations --- .changeset/atomic-model-operations.md | 8 ++++ .../server/lib/deleteCustomSound.ts | 3 +- .../app/e2e/server/methods/setRoomKeyID.ts | 12 ++---- .../server/methods/deleteEmojiCustom.ts | 3 +- .../incoming/deleteIncomingIntegration.ts | 19 +++++---- .../outgoing/deleteOutgoingIntegration.ts | 21 +++++----- .../invites/server/functions/removeInvite.ts | 7 +--- .../server/lib/settings.ts | 13 ++---- .../server/admin/methods/deleteOAuthApp.ts | 4 +- .../server/admin/methods/updateOAuthApp.ts | 40 ++++++++----------- .../server/methods/deleteCustomUserStatus.ts | 3 +- .../server/lib/cas/findExistingCASUser.ts | 10 +---- apps/meteor/server/lib/cas/loginHandler.ts | 5 +-- .../server/lib/oauth/updateOAuthServices.ts | 9 ++--- apps/meteor/server/services/room/service.ts | 5 +-- apps/meteor/server/services/team/service.ts | 31 +++----------- packages/core-typings/src/IOAuthApps.ts | 4 ++ .../model-typings/src/models/IBaseModel.ts | 1 + .../src/models/ICredentialTokensModel.ts | 1 + .../src/models/IIntegrationsModel.ts | 1 + .../models/ILoginServiceConfigurationModel.ts | 4 +- .../src/models/IOAuthAppsModel.ts | 5 +++ .../model-typings/src/models/IRoomsModel.ts | 1 + .../src/models/ISubscriptionsModel.ts | 1 + .../model-typings/src/models/IUsersModel.ts | 1 + packages/models/src/dummy/BaseDummy.ts | 4 ++ packages/models/src/models/BaseRaw.ts | 4 ++ .../models/src/models/CredentialTokens.ts | 9 +++++ packages/models/src/models/Integrations.ts | 7 ++++ .../src/models/LoginServiceConfiguration.ts | 6 +-- packages/models/src/models/OAuthApps.ts | 7 ++++ packages/models/src/models/Rooms.ts | 15 +++++++ packages/models/src/models/Subscriptions.ts | 10 +++++ packages/models/src/models/Users.ts | 6 +++ 34 files changed, 152 insertions(+), 128 deletions(-) create mode 100644 .changeset/atomic-model-operations.md diff --git a/.changeset/atomic-model-operations.md b/.changeset/atomic-model-operations.md new file mode 100644 index 0000000000000..47a8be85332c4 --- /dev/null +++ b/.changeset/atomic-model-operations.md @@ -0,0 +1,8 @@ +--- +'@rocket.chat/meteor': patch +'@rocket.chat/core-typings': patch +'@rocket.chat/model-typings': patch +'@rocket.chat/models': patch +--- + +Fixes race conditions in several check-then-write database flows by collapsing them into single atomic operations: CAS login tokens can no longer be consumed by two concurrent logins, revoking a room invite no longer emits duplicate removal notifications, setting a room's E2E key ID no longer overwrites a key set concurrently, and deleting an integration now enforces the creator-only permission scope in the delete itself diff --git a/apps/meteor/app/custom-sounds/server/lib/deleteCustomSound.ts b/apps/meteor/app/custom-sounds/server/lib/deleteCustomSound.ts index ec5721164b1fe..e2b3ff1fcbd65 100644 --- a/apps/meteor/app/custom-sounds/server/lib/deleteCustomSound.ts +++ b/apps/meteor/app/custom-sounds/server/lib/deleteCustomSound.ts @@ -5,7 +5,7 @@ import { Meteor } from 'meteor/meteor'; import { RocketChatFileCustomSoundsInstance } from '../startup/custom-sounds'; export const deleteCustomSound = async (_id: string): Promise => { - const sound = await CustomSounds.findOneById(_id); + const sound = await CustomSounds.findOneAndDeleteById(_id); if (!sound) { throw new Meteor.Error('Custom_Sound_Error_Invalid_Sound', 'Invalid sound', { @@ -14,7 +14,6 @@ export const deleteCustomSound = async (_id: string): Promise => { } await RocketChatFileCustomSoundsInstance.deleteFile(`${sound._id}.${sound.extension}`); - await CustomSounds.removeById(_id); void api.broadcast('notify.deleteCustomSound', { soundData: sound }); }; diff --git a/apps/meteor/app/e2e/server/methods/setRoomKeyID.ts b/apps/meteor/app/e2e/server/methods/setRoomKeyID.ts index a9b4ae5dda49e..68e2f19505df2 100644 --- a/apps/meteor/app/e2e/server/methods/setRoomKeyID.ts +++ b/apps/meteor/app/e2e/server/methods/setRoomKeyID.ts @@ -19,21 +19,15 @@ export const setRoomKeyIDMethod = async (userId: string, rid: IRoom['_id'], keyI throw new Meteor.Error('error-invalid-room', 'Invalid room', { method: 'e2e.setRoomKeyID' }); } - const room = await Rooms.findOneById>(rid, { projection: { e2eKeyId: 1 } }); + const { matchedCount } = await Rooms.setE2eKeyIdIfNotSet(rid, keyID); - if (!room) { - throw new Meteor.Error('error-invalid-room', 'Invalid room', { method: 'e2e.setRoomKeyID' }); - } - - if (room.e2eKeyId) { + if (!matchedCount) { throw new Meteor.Error('error-room-e2e-key-already-exists', 'E2E Key ID already exists', { method: 'e2e.setRoomKeyID', }); } - await Rooms.setE2eKeyId(room._id, keyID); - - void notifyOnRoomChangedById(room._id); + void notifyOnRoomChangedById(rid); }; Meteor.methods({ diff --git a/apps/meteor/app/emoji-custom/server/methods/deleteEmojiCustom.ts b/apps/meteor/app/emoji-custom/server/methods/deleteEmojiCustom.ts index 0ca581f7fe1c1..9e0adcb90793b 100644 --- a/apps/meteor/app/emoji-custom/server/methods/deleteEmojiCustom.ts +++ b/apps/meteor/app/emoji-custom/server/methods/deleteEmojiCustom.ts @@ -20,7 +20,7 @@ export const deleteEmojiCustom = async (userId: string, emojiID: ICustomEmojiDes throw new Meteor.Error('not_authorized'); } - const emoji = await EmojiCustom.findOneById(emojiID); + const emoji = await EmojiCustom.findOneAndDeleteById(emojiID); if (emoji == null) { throw new Meteor.Error('Custom_Emoji_Error_Invalid_Emoji', 'Invalid emoji', { method: 'deleteEmojiCustom', @@ -28,7 +28,6 @@ export const deleteEmojiCustom = async (userId: string, emojiID: ICustomEmojiDes } await RocketChatFileEmojiCustomInstance.deleteFile(encodeURIComponent(`${emoji.name}.${emoji.extension}`)); - await EmojiCustom.removeById(emojiID); void api.broadcast('emoji.deleteCustom', emoji); return true; diff --git a/apps/meteor/app/integrations/server/methods/incoming/deleteIncomingIntegration.ts b/apps/meteor/app/integrations/server/methods/incoming/deleteIncomingIntegration.ts index 3258aeed8aebc..10e760bc33e97 100644 --- a/apps/meteor/app/integrations/server/methods/incoming/deleteIncomingIntegration.ts +++ b/apps/meteor/app/integrations/server/methods/incoming/deleteIncomingIntegration.ts @@ -14,28 +14,27 @@ declare module '@rocket.chat/ddp-client' { } export const deleteIncomingIntegration = async (integrationId: string, userId: string): Promise => { - let integration; + let canManageAllIntegrations = false; if (userId && (await hasPermissionAsync(userId, 'manage-incoming-integrations'))) { - integration = Integrations.findOneById(integrationId); - } else if (userId && (await hasPermissionAsync(userId, 'manage-own-incoming-integrations'))) { - integration = Integrations.findOne({ - '_id': integrationId, - '_createdBy._id': userId, - }); - } else { + canManageAllIntegrations = true; + } else if (!userId || !(await hasPermissionAsync(userId, 'manage-own-incoming-integrations'))) { throw new Meteor.Error('not_authorized', 'Unauthorized', { method: 'deleteIncomingIntegration', }); } - if (!(await integration)) { + const integration = await Integrations.removeByIdAndCreatedByIfExists({ + _id: integrationId, + ...(!canManageAllIntegrations && { createdBy: userId }), + }); + + if (!integration) { throw new Meteor.Error('error-invalid-integration', 'Invalid integration', { method: 'deleteIncomingIntegration', }); } - await Integrations.removeById(integrationId); void notifyOnIntegrationChangedById(integrationId, 'removed'); }; diff --git a/apps/meteor/app/integrations/server/methods/outgoing/deleteOutgoingIntegration.ts b/apps/meteor/app/integrations/server/methods/outgoing/deleteOutgoingIntegration.ts index b9e44d7d5adbd..a659386817f2c 100644 --- a/apps/meteor/app/integrations/server/methods/outgoing/deleteOutgoingIntegration.ts +++ b/apps/meteor/app/integrations/server/methods/outgoing/deleteOutgoingIntegration.ts @@ -14,34 +14,33 @@ declare module '@rocket.chat/ddp-client' { } export const deleteOutgoingIntegration = async (integrationId: string, userId: string): Promise => { - let integration; - if (!userId) { throw new Meteor.Error('not_authorized', 'Unauthorized', { method: 'deleteOutgoingIntegration', }); } + let canManageAllIntegrations = false; + if (await hasPermissionAsync(userId, 'manage-outgoing-integrations')) { - integration = Integrations.findOneById(integrationId); - } else if (await hasPermissionAsync(userId, 'manage-own-outgoing-integrations')) { - integration = Integrations.findOne({ - '_id': integrationId, - '_createdBy._id': userId, - }); - } else { + canManageAllIntegrations = true; + } else if (!(await hasPermissionAsync(userId, 'manage-own-outgoing-integrations'))) { throw new Meteor.Error('not_authorized', 'Unauthorized', { method: 'deleteOutgoingIntegration', }); } - if (!(await integration)) { + const integration = await Integrations.removeByIdAndCreatedByIfExists({ + _id: integrationId, + ...(!canManageAllIntegrations && { createdBy: userId }), + }); + + if (!integration) { throw new Meteor.Error('error-invalid-integration', 'Invalid integration', { method: 'deleteOutgoingIntegration', }); } - await Integrations.removeById(integrationId); // Don't sending to IntegrationHistory listener since it don't waits for 'removed' events. await IntegrationHistory.removeByIntegrationId(integrationId); void notifyOnIntegrationChangedById(integrationId, 'removed'); diff --git a/apps/meteor/app/invites/server/functions/removeInvite.ts b/apps/meteor/app/invites/server/functions/removeInvite.ts index c754d82d071cc..266d4f7f2fc81 100644 --- a/apps/meteor/app/invites/server/functions/removeInvite.ts +++ b/apps/meteor/app/invites/server/functions/removeInvite.ts @@ -20,16 +20,13 @@ export const removeInvite = async (userId: string, invite: Pick) }); } - // Before anything, let's check if there's an existing invite - const existing = await Invites.findOneById(invite._id); + const { deletedCount } = await Invites.removeById(invite._id); - if (!existing) { + if (!deletedCount) { throw new Meteor.Error('invalid-invitation-id', 'Invalid Invitation _id', { method: 'removeInvite', }); } - await Invites.removeById(invite._id); - return true; }; diff --git a/apps/meteor/app/meteor-accounts-saml/server/lib/settings.ts b/apps/meteor/app/meteor-accounts-saml/server/lib/settings.ts index 0316045acb5c8..b068d011a4696 100644 --- a/apps/meteor/app/meteor-accounts-saml/server/lib/settings.ts +++ b/apps/meteor/app/meteor-accounts-saml/server/lib/settings.ts @@ -150,18 +150,11 @@ export const loadSamlServiceProviders = async function (): Promise { SAMLUtils.logger?.warn({ msg: 'SAML Provider not loaded due to invalid configuration', key }); } - const service = await LoginServiceConfiguration.findOneByService(serviceName, { projection: { _id: 1 } }); - if (!service?._id) { - return false; + const service = await LoginServiceConfiguration.removeByService(serviceName); + if (service) { + void notifyOnLoginServiceConfigurationChanged({ _id: service._id }, 'removed'); } - const { deletedCount } = await LoginServiceConfiguration.removeService(service._id); - if (!deletedCount) { - return false; - } - - void notifyOnLoginServiceConfigurationChanged({ _id: service._id }, 'removed'); - return false; }), ) diff --git a/apps/meteor/app/oauth2-server-config/server/admin/methods/deleteOAuthApp.ts b/apps/meteor/app/oauth2-server-config/server/admin/methods/deleteOAuthApp.ts index ea86151253ec4..872eff77f1ab6 100644 --- a/apps/meteor/app/oauth2-server-config/server/admin/methods/deleteOAuthApp.ts +++ b/apps/meteor/app/oauth2-server-config/server/admin/methods/deleteOAuthApp.ts @@ -18,15 +18,13 @@ export const deleteOAuthApp = async (userId: string, applicationId: IOAuthApps[' throw new Meteor.Error('error-not-allowed', 'Not allowed', { method: 'deleteOAuthApp' }); } - const application = await OAuthApps.findOneById(applicationId); + const application = await OAuthApps.findOneAndDeleteById(applicationId); if (!application) { throw new Meteor.Error('error-application-not-found', 'Application not found', { method: 'deleteOAuthApp', }); } - await OAuthApps.deleteOne({ _id: applicationId }); - await OAuthAccessTokens.deleteMany({ clientId: application.clientId }); await OAuthAuthCodes.deleteMany({ clientId: application.clientId }); diff --git a/apps/meteor/app/oauth2-server-config/server/admin/methods/updateOAuthApp.ts b/apps/meteor/app/oauth2-server-config/server/admin/methods/updateOAuthApp.ts index a5adfbaea4904..cf7c866acf3b8 100644 --- a/apps/meteor/app/oauth2-server-config/server/admin/methods/updateOAuthApp.ts +++ b/apps/meteor/app/oauth2-server-config/server/admin/methods/updateOAuthApp.ts @@ -42,13 +42,6 @@ export const updateOAuthApp = async ( }); } - const currentApplication = await OAuthApps.findOneById(applicationId); - if (currentApplication == null) { - throw new Meteor.Error('error-application-not-found', 'Application not found', { - method: 'updateOAuthApp', - }); - } - const redirectUri = parseUriList(application.redirectUri); if (redirectUri.length === 0) { @@ -57,23 +50,24 @@ export const updateOAuthApp = async ( }); } - await OAuthApps.updateOne( - { _id: applicationId }, - { - $set: { - name: application.name, - active: application.active, - redirectUri, - _updatedAt: new Date(), - _updatedBy: await Users.findOneById(userId, { - projection: { - username: 1, - }, - }), + const updatedApplication = await OAuthApps.updateById(applicationId, { + name: application.name, + active: application.active, + redirectUri, + _updatedBy: await Users.findOneById(userId, { + projection: { + username: 1, }, - }, - ); - return OAuthApps.findOneById(applicationId); + }), + }); + + if (updatedApplication == null) { + throw new Meteor.Error('error-application-not-found', 'Application not found', { + method: 'updateOAuthApp', + }); + } + + return updatedApplication; }; Meteor.methods({ diff --git a/apps/meteor/app/user-status/server/methods/deleteCustomUserStatus.ts b/apps/meteor/app/user-status/server/methods/deleteCustomUserStatus.ts index 7592e99413d30..260220eea3f15 100644 --- a/apps/meteor/app/user-status/server/methods/deleteCustomUserStatus.ts +++ b/apps/meteor/app/user-status/server/methods/deleteCustomUserStatus.ts @@ -18,12 +18,11 @@ export const deleteCustomUserStatus = async (userId: string, userStatusID: strin throw new Meteor.Error('not_authorized'); } - const userStatus = await CustomUserStatus.findOneById(userStatusID); + const userStatus = await CustomUserStatus.findOneAndDeleteById(userStatusID); if (userStatus == null) { throw new Meteor.Error('Custom_User_Status_Error_Invalid_User_Status', 'Invalid user status', { method: 'deleteCustomUserStatus' }); } - await CustomUserStatus.removeById(userStatusID); void api.broadcast('user.deleteCustomStatus', userStatus); return true; diff --git a/apps/meteor/server/lib/cas/findExistingCASUser.ts b/apps/meteor/server/lib/cas/findExistingCASUser.ts index 2c65ab77546dc..9d6354b81cf77 100644 --- a/apps/meteor/server/lib/cas/findExistingCASUser.ts +++ b/apps/meteor/server/lib/cas/findExistingCASUser.ts @@ -16,12 +16,6 @@ export const findExistingCASUser = async (username: string): Promise('CAS_Sync_User_Data_FieldMap').trim(); const casVersion = parseFloat(settings.get('CAS_version') ?? '1.0'); diff --git a/apps/meteor/server/lib/oauth/updateOAuthServices.ts b/apps/meteor/server/lib/oauth/updateOAuthServices.ts index 784628cae44a9..393474fe4b24e 100644 --- a/apps/meteor/server/lib/oauth/updateOAuthServices.ts +++ b/apps/meteor/server/lib/oauth/updateOAuthServices.ts @@ -119,12 +119,9 @@ export async function updateOAuthServices(): Promise { await LoginServiceConfiguration.createOrUpdateService(serviceKey, data); void notifyOnLoginServiceConfigurationChangedByService(serviceKey); } else { - const service = await LoginServiceConfiguration.findOneByService(serviceName, { projection: { _id: 1 } }); - if (service?._id) { - const { deletedCount } = await LoginServiceConfiguration.removeService(service._id); - if (deletedCount > 0) { - void notifyOnLoginServiceConfigurationChanged({ _id: service._id }, 'removed'); - } + const service = await LoginServiceConfiguration.removeByService(serviceName); + if (service) { + void notifyOnLoginServiceConfigurationChanged({ _id: service._id }, 'removed'); } } } diff --git a/apps/meteor/server/services/room/service.ts b/apps/meteor/server/services/room/service.ts index 5f417e530a084..606f979eb7d4b 100644 --- a/apps/meteor/server/services/room/service.ts +++ b/apps/meteor/server/services/room/service.ts @@ -153,12 +153,11 @@ export class RoomService extends ServiceClassInternal implements IRoomService { } async revokeInvite(room: IRoom, user: IUser): Promise { - const subscription = await Subscriptions.findOneByRoomIdAndUserId(room._id, user._id); - if (subscription?.status !== 'INVITED') { + const subscription = await Subscriptions.removeInvitedByRoomIdAndUserId(room._id, user._id); + if (!subscription) { return; } - await Subscriptions.removeById(subscription._id); void notifyOnSubscriptionChanged(subscription, 'removed'); } diff --git a/apps/meteor/server/services/team/service.ts b/apps/meteor/server/services/team/service.ts index 9c3c7c8b746fa..5da5df9380d03 100644 --- a/apps/meteor/server/services/team/service.ts +++ b/apps/meteor/server/services/team/service.ts @@ -936,40 +936,21 @@ export class TeamService extends ServiceClassInternal implements ITeamService { } async addRolesToMember(teamId: string, userId: string, roles: Array): Promise { - const isMember = await TeamMember.findOneByUserIdAndTeamId(userId, teamId, { - projection: { _id: 1 }, - }); - - if (!isMember) { - // TODO should this throw an error instead? - return false; - } + const { matchedCount } = await TeamMember.updateRolesByTeamIdAndUserId(teamId, userId, roles); - return !!(await TeamMember.updateRolesByTeamIdAndUserId(teamId, userId, roles)); + return matchedCount > 0; } async addRolesToSubscription(roomId: string, userId: string, roles: Array): Promise { - const subscription = await Subscriptions.findOneByRoomIdAndUserId(roomId, userId); + const { matchedCount } = await Subscriptions.addRolesByUserId(userId, roles, roomId); - if (!subscription) { - // TODO should this throw an error instead? - return false; - } - - return !!(await Subscriptions.addRolesByUserId(userId, roles, roomId)); + return matchedCount > 0; } async removeRolesFromMember(teamId: string, userId: string, roles: Array): Promise { - const isMember = await TeamMember.findOneByUserIdAndTeamId(userId, teamId, { - projection: { _id: 1 }, - }); - - if (!isMember) { - // TODO should this throw an error instead? - return false; - } + const { matchedCount } = await TeamMember.removeRolesByTeamIdAndUserId(teamId, userId, roles); - return !!(await TeamMember.removeRolesByTeamIdAndUserId(teamId, userId, roles)); + return matchedCount > 0; } async getInfoByName(teamName: string): Promise | null> { diff --git a/packages/core-typings/src/IOAuthApps.ts b/packages/core-typings/src/IOAuthApps.ts index 8003170233aa7..2c92fb6e78e53 100644 --- a/packages/core-typings/src/IOAuthApps.ts +++ b/packages/core-typings/src/IOAuthApps.ts @@ -11,5 +11,9 @@ export interface IOAuthApps extends IRocketChatRecord { _id: string; username: string; }; + _updatedBy?: { + _id: string; + username?: string; + } | null; appId?: string; } diff --git a/packages/model-typings/src/models/IBaseModel.ts b/packages/model-typings/src/models/IBaseModel.ts index e293dfa52d750..93a513d17cbfb 100644 --- a/packages/model-typings/src/models/IBaseModel.ts +++ b/packages/model-typings/src/models/IBaseModel.ts @@ -55,6 +55,7 @@ export interface IBaseModel< updateFromUpdater(query: Filter, updater: Updater, options?: UpdateOptions): Promise; findOneAndDelete(filter: Filter, options?: FindOneAndDeleteOptions): Promise>; + findOneAndDeleteById(_id: T['_id'], options?: FindOneAndDeleteOptions): Promise>; findOneAndUpdate(query: Filter, update: UpdateFilter | T, options?: FindOneAndUpdateOptions): Promise>; findOneById(_id: T['_id'], options?: FindOptions | undefined): Promise; diff --git a/packages/model-typings/src/models/ICredentialTokensModel.ts b/packages/model-typings/src/models/ICredentialTokensModel.ts index 600d2aac82502..6ffb3fc04cfa8 100644 --- a/packages/model-typings/src/models/ICredentialTokensModel.ts +++ b/packages/model-typings/src/models/ICredentialTokensModel.ts @@ -5,4 +5,5 @@ import type { IBaseModel } from './IBaseModel'; export interface ICredentialTokensModel extends IBaseModel { create(_id: string, userInfo: ICredentialToken['userInfo']): Promise; findOneNotExpiredById(_id: string): Promise; + removeNotExpiredById(_id: string): Promise; } diff --git a/packages/model-typings/src/models/IIntegrationsModel.ts b/packages/model-typings/src/models/IIntegrationsModel.ts index fd7b4cc692b5b..8eb4888e16b06 100644 --- a/packages/model-typings/src/models/IIntegrationsModel.ts +++ b/packages/model-typings/src/models/IIntegrationsModel.ts @@ -8,6 +8,7 @@ export interface IIntegrationsModel extends IBaseModel { findByChannels(channels: IIntegration['channel']): FindCursor; findByUserId(userId: IIntegration['userId']): FindCursor>; findOneByIdAndCreatedByIfExists(params: { _id: IIntegration['_id']; createdBy?: IUser['_id'] }): Promise; + removeByIdAndCreatedByIfExists(params: { _id: IIntegration['_id']; createdBy?: IUser['_id'] }): Promise; findOneByUrl(url: string): Promise; updateRoomName(oldRoomName: string, newRoomName: string): ReturnType['updateMany']>; findOneByIdAndToken

( diff --git a/packages/model-typings/src/models/ILoginServiceConfigurationModel.ts b/packages/model-typings/src/models/ILoginServiceConfigurationModel.ts index ce71854b70abf..6a1a564c45d9f 100644 --- a/packages/model-typings/src/models/ILoginServiceConfigurationModel.ts +++ b/packages/model-typings/src/models/ILoginServiceConfigurationModel.ts @@ -1,5 +1,5 @@ import type { LoginServiceConfiguration } from '@rocket.chat/core-typings'; -import type { DeleteResult, Document, FindOptions } from 'mongodb'; +import type { Document, FindOptions } from 'mongodb'; import type { IBaseModel } from './IBaseModel'; @@ -8,7 +8,7 @@ export interface ILoginServiceConfigurationModel extends IBaseModel, ): Promise; - removeService(_id: LoginServiceConfiguration['_id']): Promise; + removeByService(serviceName: LoginServiceConfiguration['service']): Promise | null>; findOneByService

( serviceName: LoginServiceConfiguration['service'], options?: FindOptions

, diff --git a/packages/model-typings/src/models/IOAuthAppsModel.ts b/packages/model-typings/src/models/IOAuthAppsModel.ts index 859c972e45979..3a696465d2ca3 100644 --- a/packages/model-typings/src/models/IOAuthAppsModel.ts +++ b/packages/model-typings/src/models/IOAuthAppsModel.ts @@ -16,6 +16,11 @@ export interface IOAuthAppsModel extends IBaseModel { findOneActiveByClientId(clientId: string, options?: FindOptions): Promise; + updateById( + _id: IOAuthApps['_id'], + data: Partial>, + ): Promise; + findOneActiveByClientIdAndClientSecret( clientId: string, clientSecret: string, diff --git a/packages/model-typings/src/models/IRoomsModel.ts b/packages/model-typings/src/models/IRoomsModel.ts index 39ce7d7ae3379..550649cc66c5a 100644 --- a/packages/model-typings/src/models/IRoomsModel.ts +++ b/packages/model-typings/src/models/IRoomsModel.ts @@ -228,6 +228,7 @@ export interface IRoomsModel extends IBaseModel { unsetAvatarData(roomId: string): Promise; setSystemMessagesById(roomId: string, systemMessages: IRoom['sysMes']): Promise; setE2eKeyId(roomId: string, e2eKeyId: string, options?: FindOptions): Promise; + setE2eKeyIdIfNotSet(roomId: string, e2eKeyId: IRoom['e2eKeyId']): Promise; findOneByImportId(importId: string, options?: FindOptions): Promise; findOneByNameAndNotId(name: string, rid: string): Promise; findOneByIdAndType(roomId: IRoom['_id'], type: IRoom['t'], options?: FindOptions): Promise; diff --git a/packages/model-typings/src/models/ISubscriptionsModel.ts b/packages/model-typings/src/models/ISubscriptionsModel.ts index 73d2a2892aef9..79342e45ca4c9 100644 --- a/packages/model-typings/src/models/ISubscriptionsModel.ts +++ b/packages/model-typings/src/models/ISubscriptionsModel.ts @@ -315,6 +315,7 @@ export interface ISubscriptionsModel extends IBaseModel { ): Promise>; removeByRoomIdsAndUserId(rids: string[], userId: string): Promise; removeByRoomIdAndUserId(roomId: string, userId: string): Promise; + removeInvitedByRoomIdAndUserId(roomId: string, userId: string): Promise; removeByRoomIds(rids: string[], options?: { onTrash: (doc: ISubscription) => void }): Promise; diff --git a/packages/model-typings/src/models/IUsersModel.ts b/packages/model-typings/src/models/IUsersModel.ts index 6e748b49fa2d4..0b34a297ef2a0 100644 --- a/packages/model-typings/src/models/IUsersModel.ts +++ b/packages/model-typings/src/models/IUsersModel.ts @@ -169,6 +169,7 @@ export interface IUsersModel extends IBaseModel { setAbacAttributesById(userId: IUser['_id'], attributes: NonNullable): Promise; unsetAbacAttributesById(userId: IUser['_id']): Promise; findActiveByRoomIds(roomIds: IRoom['_id'][], options?: FindOptions): FindCursor; + setCasExternalIdByUsername(username: string): Promise; updateStatusText(_id: IUser['_id'], statusText: string, options?: UpdateOptions): Promise; diff --git a/packages/models/src/dummy/BaseDummy.ts b/packages/models/src/dummy/BaseDummy.ts index c161b23584b65..5b4363a93b475 100644 --- a/packages/models/src/dummy/BaseDummy.ts +++ b/packages/models/src/dummy/BaseDummy.ts @@ -57,6 +57,10 @@ export class BaseDummy< return null; } + async findOneAndDeleteById(_id: T['_id']): Promise | null> { + return null; + } + async findOneAndUpdate(): Promise | null> { return null; } diff --git a/packages/models/src/models/BaseRaw.ts b/packages/models/src/models/BaseRaw.ts index 3c389e9cad9c1..6c572b9fa70d2 100644 --- a/packages/models/src/models/BaseRaw.ts +++ b/packages/models/src/models/BaseRaw.ts @@ -366,6 +366,10 @@ export abstract class BaseRaw< return doc as WithId; } + findOneAndDeleteById(_id: T['_id'], options?: FindOneAndDeleteOptions): Promise | null> { + return this.findOneAndDelete({ _id } as Filter, options); + } + async deleteMany(filter: Filter, options?: DeleteOptions & { onTrash?: (record: ResultFields) => void }): Promise { if (!this.trash) { if (options) { diff --git a/packages/models/src/models/CredentialTokens.ts b/packages/models/src/models/CredentialTokens.ts index cdeb1d0f59c7e..dd78b9108f3c2 100644 --- a/packages/models/src/models/CredentialTokens.ts +++ b/packages/models/src/models/CredentialTokens.ts @@ -32,4 +32,13 @@ export class CredentialTokensRaw extends BaseRaw implements IC return this.findOne(query); } + + removeNotExpiredById(_id: string): Promise { + const query = { + _id, + expireAt: { $gt: new Date() }, + }; + + return this.findOneAndDelete(query); + } } diff --git a/packages/models/src/models/Integrations.ts b/packages/models/src/models/Integrations.ts index 9acb2496e3805..8ae0c87037b5d 100644 --- a/packages/models/src/models/Integrations.ts +++ b/packages/models/src/models/Integrations.ts @@ -46,6 +46,13 @@ export class IntegrationsRaw extends BaseRaw implements IIntegrati }); } + removeByIdAndCreatedByIfExists({ _id, createdBy }: { _id: IIntegration['_id']; createdBy?: IUser['_id'] }): Promise { + return this.findOneAndDelete({ + _id, + ...(createdBy && { '_createdBy._id': createdBy }), + }); + } + disableByUserId(userId: IIntegration['userId']): ReturnType['updateMany']> { return this.updateMany({ userId }, { $set: { enabled: false } }); } diff --git a/packages/models/src/models/LoginServiceConfiguration.ts b/packages/models/src/models/LoginServiceConfiguration.ts index 06d0c5753f910..721d63ff21b40 100644 --- a/packages/models/src/models/LoginServiceConfiguration.ts +++ b/packages/models/src/models/LoginServiceConfiguration.ts @@ -1,6 +1,6 @@ import type { LoginServiceConfiguration, RocketChatRecordDeleted } from '@rocket.chat/core-typings'; import type { ILoginServiceConfigurationModel } from '@rocket.chat/model-typings'; -import type { Collection, Db, DeleteResult, Document, FindOptions } from 'mongodb'; +import type { Collection, Db, Document, FindOptions } from 'mongodb'; import { BaseRaw } from './BaseRaw'; @@ -44,8 +44,8 @@ export class LoginServiceConfigurationRaw extends BaseRaw { - return this.deleteOne({ _id }); + removeByService(serviceName: LoginServiceConfiguration['service']): Promise | null> { + return this.findOneAndDelete({ service: serviceName.toLowerCase() }, { projection: { _id: 1 } }); } async findOneByService

( diff --git a/packages/models/src/models/OAuthApps.ts b/packages/models/src/models/OAuthApps.ts index ccac7c15eee16..b6d63688331e0 100644 --- a/packages/models/src/models/OAuthApps.ts +++ b/packages/models/src/models/OAuthApps.ts @@ -40,6 +40,13 @@ export class OAuthAppsRaw extends BaseRaw implements IOAuthAppsModel ); } + updateById( + _id: IOAuthApps['_id'], + data: Partial>, + ): Promise { + return this.findOneAndUpdate({ _id }, { $set: data }, { returnDocument: 'after' }); + } + findOneActiveByClientIdAndClientSecret( clientId: string, clientSecret: string, diff --git a/packages/models/src/models/Rooms.ts b/packages/models/src/models/Rooms.ts index c930207f79b13..5ebe563fbb8a2 100644 --- a/packages/models/src/models/Rooms.ts +++ b/packages/models/src/models/Rooms.ts @@ -1229,6 +1229,21 @@ export class RoomsRaw extends BaseRaw implements IRoomsModel { return this.updateOne(query, update, options); } + setE2eKeyIdIfNotSet(_id: IRoom['_id'], e2eKeyId: IRoom['e2eKeyId']): Promise { + const query: Filter = { + _id, + e2eKeyId: { $exists: false }, + }; + + const update: UpdateFilter = { + $set: { + e2eKeyId, + }, + }; + + return this.updateOne(query, update); + } + findOneByImportId(_id: IRoom['_id'], options: FindOptions = {}): Promise { const query: Filter = { importIds: _id }; diff --git a/packages/models/src/models/Subscriptions.ts b/packages/models/src/models/Subscriptions.ts index c4533e58bcbb0..dc3bccd9811bc 100644 --- a/packages/models/src/models/Subscriptions.ts +++ b/packages/models/src/models/Subscriptions.ts @@ -1984,6 +1984,16 @@ export class SubscriptionsRaw extends BaseRaw implements ISubscri return doc; } + removeInvitedByRoomIdAndUserId(roomId: string, userId: string): Promise { + const query = { + 'rid': roomId, + 'u._id': userId, + 'status': 'INVITED' as const, + }; + + return this.findOneAndDelete(query); + } + async removeByRoomIds(rids: string[], options?: { onTrash: (doc: ISubscription) => void }): Promise { const result = await this.deleteMany({ rid: { $in: rids } }, options); diff --git a/packages/models/src/models/Users.ts b/packages/models/src/models/Users.ts index 8c18f2dbdf8f5..6a456c413cb88 100644 --- a/packages/models/src/models/Users.ts +++ b/packages/models/src/models/Users.ts @@ -148,6 +148,12 @@ export class UsersRaw extends BaseRaw> implements IU return this.find({ active: true, __rooms: { $in: roomIds } }, options); } + setCasExternalIdByUsername(username: string): Promise { + // #TODO: Remove regex based search + const regex = new RegExp(`^${escapeRegExp(username)}$`, 'i'); + return this.findOneAndUpdate({ username: regex }, { $set: { 'services.cas.external_id': username } }, { returnDocument: 'after' }); + } + /** * @param {string} uid * @param {IRole['_id'][]} roles list of role ids From 0263d183130839305417168ca09c34431b04dfaf Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Mon, 6 Jul 2026 10:51:59 -0600 Subject: [PATCH 02/11] revert setRoomKeyID atomic rewrite --- .changeset/atomic-model-operations.md | 2 +- apps/meteor/app/e2e/server/methods/setRoomKeyID.ts | 12 +++++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/.changeset/atomic-model-operations.md b/.changeset/atomic-model-operations.md index 47a8be85332c4..4e05f12719360 100644 --- a/.changeset/atomic-model-operations.md +++ b/.changeset/atomic-model-operations.md @@ -5,4 +5,4 @@ '@rocket.chat/models': patch --- -Fixes race conditions in several check-then-write database flows by collapsing them into single atomic operations: CAS login tokens can no longer be consumed by two concurrent logins, revoking a room invite no longer emits duplicate removal notifications, setting a room's E2E key ID no longer overwrites a key set concurrently, and deleting an integration now enforces the creator-only permission scope in the delete itself +Fixes race conditions in several check-then-write database flows by collapsing them into single atomic operations: CAS login tokens can no longer be consumed by two concurrent logins, revoking a room invite no longer emits duplicate removal notifications, and deleting an integration now enforces the creator-only permission scope in the delete itself diff --git a/apps/meteor/app/e2e/server/methods/setRoomKeyID.ts b/apps/meteor/app/e2e/server/methods/setRoomKeyID.ts index 68e2f19505df2..a9b4ae5dda49e 100644 --- a/apps/meteor/app/e2e/server/methods/setRoomKeyID.ts +++ b/apps/meteor/app/e2e/server/methods/setRoomKeyID.ts @@ -19,15 +19,21 @@ export const setRoomKeyIDMethod = async (userId: string, rid: IRoom['_id'], keyI throw new Meteor.Error('error-invalid-room', 'Invalid room', { method: 'e2e.setRoomKeyID' }); } - const { matchedCount } = await Rooms.setE2eKeyIdIfNotSet(rid, keyID); + const room = await Rooms.findOneById>(rid, { projection: { e2eKeyId: 1 } }); - if (!matchedCount) { + if (!room) { + throw new Meteor.Error('error-invalid-room', 'Invalid room', { method: 'e2e.setRoomKeyID' }); + } + + if (room.e2eKeyId) { throw new Meteor.Error('error-room-e2e-key-already-exists', 'E2E Key ID already exists', { method: 'e2e.setRoomKeyID', }); } - void notifyOnRoomChangedById(rid); + await Rooms.setE2eKeyId(room._id, keyID); + + void notifyOnRoomChangedById(room._id); }; Meteor.methods({ From 0c9dced6683ed1d5f7f5b65ae66c4a3c0b82ba56 Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Mon, 6 Jul 2026 10:54:09 -0600 Subject: [PATCH 03/11] remove unused setE2eKeyIdIfNotSet --- packages/model-typings/src/models/IRoomsModel.ts | 1 - packages/models/src/models/Rooms.ts | 15 --------------- 2 files changed, 16 deletions(-) diff --git a/packages/model-typings/src/models/IRoomsModel.ts b/packages/model-typings/src/models/IRoomsModel.ts index 550649cc66c5a..39ce7d7ae3379 100644 --- a/packages/model-typings/src/models/IRoomsModel.ts +++ b/packages/model-typings/src/models/IRoomsModel.ts @@ -228,7 +228,6 @@ export interface IRoomsModel extends IBaseModel { unsetAvatarData(roomId: string): Promise; setSystemMessagesById(roomId: string, systemMessages: IRoom['sysMes']): Promise; setE2eKeyId(roomId: string, e2eKeyId: string, options?: FindOptions): Promise; - setE2eKeyIdIfNotSet(roomId: string, e2eKeyId: IRoom['e2eKeyId']): Promise; findOneByImportId(importId: string, options?: FindOptions): Promise; findOneByNameAndNotId(name: string, rid: string): Promise; findOneByIdAndType(roomId: IRoom['_id'], type: IRoom['t'], options?: FindOptions): Promise; diff --git a/packages/models/src/models/Rooms.ts b/packages/models/src/models/Rooms.ts index 5ebe563fbb8a2..c930207f79b13 100644 --- a/packages/models/src/models/Rooms.ts +++ b/packages/models/src/models/Rooms.ts @@ -1229,21 +1229,6 @@ export class RoomsRaw extends BaseRaw implements IRoomsModel { return this.updateOne(query, update, options); } - setE2eKeyIdIfNotSet(_id: IRoom['_id'], e2eKeyId: IRoom['e2eKeyId']): Promise { - const query: Filter = { - _id, - e2eKeyId: { $exists: false }, - }; - - const update: UpdateFilter = { - $set: { - e2eKeyId, - }, - }; - - return this.updateOne(query, update); - } - findOneByImportId(_id: IRoom['_id'], options: FindOptions = {}): Promise { const query: Filter = { importIds: _id }; From 4ac400801fac3b8ea24f950c913b835bfb0360c3 Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Mon, 6 Jul 2026 11:49:34 -0600 Subject: [PATCH 04/11] use collation equality for CAS username lookup --- packages/models/src/models/Users.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/packages/models/src/models/Users.ts b/packages/models/src/models/Users.ts index 6a456c413cb88..e852a0a884b9f 100644 --- a/packages/models/src/models/Users.ts +++ b/packages/models/src/models/Users.ts @@ -149,9 +149,14 @@ export class UsersRaw extends BaseRaw> implements IU } setCasExternalIdByUsername(username: string): Promise { - // #TODO: Remove regex based search - const regex = new RegExp(`^${escapeRegExp(username)}$`, 'i'); - return this.findOneAndUpdate({ username: regex }, { $set: { 'services.cas.external_id': username } }, { returnDocument: 'after' }); + return this.findOneAndUpdate( + { username }, + { $set: { 'services.cas.external_id': username } }, + { + collation: { locale: 'en', strength: 2 }, // Case insensitive + returnDocument: 'after', + }, + ); } /** From 82aceccc94e892f18145caad0b7407f94fd21d8a Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Mon, 6 Jul 2026 12:01:47 -0600 Subject: [PATCH 05/11] keep CAS username regex lookup verbatim --- packages/models/src/models/Users.ts | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/packages/models/src/models/Users.ts b/packages/models/src/models/Users.ts index e852a0a884b9f..8980827825882 100644 --- a/packages/models/src/models/Users.ts +++ b/packages/models/src/models/Users.ts @@ -149,14 +149,9 @@ export class UsersRaw extends BaseRaw> implements IU } setCasExternalIdByUsername(username: string): Promise { - return this.findOneAndUpdate( - { username }, - { $set: { 'services.cas.external_id': username } }, - { - collation: { locale: 'en', strength: 2 }, // Case insensitive - returnDocument: 'after', - }, - ); + // #TODO: Remove regex based search + const regex = new RegExp(`^${username}$`, 'i'); + return this.findOneAndUpdate({ username: regex }, { $set: { 'services.cas.external_id': username } }, { returnDocument: 'after' }); } /** From e7469bffa3b5d4ea1185af6617cdb484906c8feb Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Mon, 6 Jul 2026 12:09:24 -0600 Subject: [PATCH 06/11] delete sound/emoji blobs before records so failures stay retryable --- .../custom-sounds/server/lib/deleteCustomSound.ts | 13 +++++++++++-- .../server/methods/deleteEmojiCustom.ts | 13 +++++++++++-- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/apps/meteor/app/custom-sounds/server/lib/deleteCustomSound.ts b/apps/meteor/app/custom-sounds/server/lib/deleteCustomSound.ts index e2b3ff1fcbd65..c2d59cd401fd0 100644 --- a/apps/meteor/app/custom-sounds/server/lib/deleteCustomSound.ts +++ b/apps/meteor/app/custom-sounds/server/lib/deleteCustomSound.ts @@ -5,7 +5,7 @@ import { Meteor } from 'meteor/meteor'; import { RocketChatFileCustomSoundsInstance } from '../startup/custom-sounds'; export const deleteCustomSound = async (_id: string): Promise => { - const sound = await CustomSounds.findOneAndDeleteById(_id); + const sound = await CustomSounds.findOneById(_id); if (!sound) { throw new Meteor.Error('Custom_Sound_Error_Invalid_Sound', 'Invalid sound', { @@ -13,7 +13,16 @@ export const deleteCustomSound = async (_id: string): Promise => { }); } + // blob first, record last: a failure leaves a retryable record instead of an unreachable blob await RocketChatFileCustomSoundsInstance.deleteFile(`${sound._id}.${sound.extension}`); - void api.broadcast('notify.deleteCustomSound', { soundData: sound }); + const deletedSound = await CustomSounds.findOneAndDeleteById(_id); + + if (!deletedSound) { + throw new Meteor.Error('Custom_Sound_Error_Invalid_Sound', 'Invalid sound', { + method: 'deleteCustomSound', + }); + } + + void api.broadcast('notify.deleteCustomSound', { soundData: deletedSound }); }; diff --git a/apps/meteor/app/emoji-custom/server/methods/deleteEmojiCustom.ts b/apps/meteor/app/emoji-custom/server/methods/deleteEmojiCustom.ts index 9e0adcb90793b..020fc4eea4a14 100644 --- a/apps/meteor/app/emoji-custom/server/methods/deleteEmojiCustom.ts +++ b/apps/meteor/app/emoji-custom/server/methods/deleteEmojiCustom.ts @@ -20,15 +20,24 @@ export const deleteEmojiCustom = async (userId: string, emojiID: ICustomEmojiDes throw new Meteor.Error('not_authorized'); } - const emoji = await EmojiCustom.findOneAndDeleteById(emojiID); + const emoji = await EmojiCustom.findOneById(emojiID); if (emoji == null) { throw new Meteor.Error('Custom_Emoji_Error_Invalid_Emoji', 'Invalid emoji', { method: 'deleteEmojiCustom', }); } + // blob first, record last: a failure leaves a retryable record instead of an unreachable blob await RocketChatFileEmojiCustomInstance.deleteFile(encodeURIComponent(`${emoji.name}.${emoji.extension}`)); - void api.broadcast('emoji.deleteCustom', emoji); + + const deletedEmoji = await EmojiCustom.findOneAndDeleteById(emojiID); + if (deletedEmoji == null) { + throw new Meteor.Error('Custom_Emoji_Error_Invalid_Emoji', 'Invalid emoji', { + method: 'deleteEmojiCustom', + }); + } + + void api.broadcast('emoji.deleteCustom', deletedEmoji); return true; }; From 0861e8914e4b890e9ba6d8fd9553007ea346eb3f Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Mon, 6 Jul 2026 12:15:54 -0600 Subject: [PATCH 07/11] Revert "delete sound/emoji blobs before records so failures stay retryable" This reverts commit e7469bffa3b5d4ea1185af6617cdb484906c8feb. --- .../custom-sounds/server/lib/deleteCustomSound.ts | 13 ++----------- .../server/methods/deleteEmojiCustom.ts | 13 ++----------- 2 files changed, 4 insertions(+), 22 deletions(-) diff --git a/apps/meteor/app/custom-sounds/server/lib/deleteCustomSound.ts b/apps/meteor/app/custom-sounds/server/lib/deleteCustomSound.ts index c2d59cd401fd0..e2b3ff1fcbd65 100644 --- a/apps/meteor/app/custom-sounds/server/lib/deleteCustomSound.ts +++ b/apps/meteor/app/custom-sounds/server/lib/deleteCustomSound.ts @@ -5,7 +5,7 @@ import { Meteor } from 'meteor/meteor'; import { RocketChatFileCustomSoundsInstance } from '../startup/custom-sounds'; export const deleteCustomSound = async (_id: string): Promise => { - const sound = await CustomSounds.findOneById(_id); + const sound = await CustomSounds.findOneAndDeleteById(_id); if (!sound) { throw new Meteor.Error('Custom_Sound_Error_Invalid_Sound', 'Invalid sound', { @@ -13,16 +13,7 @@ export const deleteCustomSound = async (_id: string): Promise => { }); } - // blob first, record last: a failure leaves a retryable record instead of an unreachable blob await RocketChatFileCustomSoundsInstance.deleteFile(`${sound._id}.${sound.extension}`); - const deletedSound = await CustomSounds.findOneAndDeleteById(_id); - - if (!deletedSound) { - throw new Meteor.Error('Custom_Sound_Error_Invalid_Sound', 'Invalid sound', { - method: 'deleteCustomSound', - }); - } - - void api.broadcast('notify.deleteCustomSound', { soundData: deletedSound }); + void api.broadcast('notify.deleteCustomSound', { soundData: sound }); }; diff --git a/apps/meteor/app/emoji-custom/server/methods/deleteEmojiCustom.ts b/apps/meteor/app/emoji-custom/server/methods/deleteEmojiCustom.ts index 020fc4eea4a14..9e0adcb90793b 100644 --- a/apps/meteor/app/emoji-custom/server/methods/deleteEmojiCustom.ts +++ b/apps/meteor/app/emoji-custom/server/methods/deleteEmojiCustom.ts @@ -20,24 +20,15 @@ export const deleteEmojiCustom = async (userId: string, emojiID: ICustomEmojiDes throw new Meteor.Error('not_authorized'); } - const emoji = await EmojiCustom.findOneById(emojiID); + const emoji = await EmojiCustom.findOneAndDeleteById(emojiID); if (emoji == null) { throw new Meteor.Error('Custom_Emoji_Error_Invalid_Emoji', 'Invalid emoji', { method: 'deleteEmojiCustom', }); } - // blob first, record last: a failure leaves a retryable record instead of an unreachable blob await RocketChatFileEmojiCustomInstance.deleteFile(encodeURIComponent(`${emoji.name}.${emoji.extension}`)); - - const deletedEmoji = await EmojiCustom.findOneAndDeleteById(emojiID); - if (deletedEmoji == null) { - throw new Meteor.Error('Custom_Emoji_Error_Invalid_Emoji', 'Invalid emoji', { - method: 'deleteEmojiCustom', - }); - } - - void api.broadcast('emoji.deleteCustom', deletedEmoji); + void api.broadcast('emoji.deleteCustom', emoji); return true; }; From 9262b928a69c699445e76f832a29260708b74981 Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Mon, 6 Jul 2026 12:16:48 -0600 Subject: [PATCH 08/11] restore sound/emoji delete flows to develop --- apps/meteor/app/custom-sounds/server/lib/deleteCustomSound.ts | 3 ++- .../app/emoji-custom/server/methods/deleteEmojiCustom.ts | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/apps/meteor/app/custom-sounds/server/lib/deleteCustomSound.ts b/apps/meteor/app/custom-sounds/server/lib/deleteCustomSound.ts index e2b3ff1fcbd65..ec5721164b1fe 100644 --- a/apps/meteor/app/custom-sounds/server/lib/deleteCustomSound.ts +++ b/apps/meteor/app/custom-sounds/server/lib/deleteCustomSound.ts @@ -5,7 +5,7 @@ import { Meteor } from 'meteor/meteor'; import { RocketChatFileCustomSoundsInstance } from '../startup/custom-sounds'; export const deleteCustomSound = async (_id: string): Promise => { - const sound = await CustomSounds.findOneAndDeleteById(_id); + const sound = await CustomSounds.findOneById(_id); if (!sound) { throw new Meteor.Error('Custom_Sound_Error_Invalid_Sound', 'Invalid sound', { @@ -14,6 +14,7 @@ export const deleteCustomSound = async (_id: string): Promise => { } await RocketChatFileCustomSoundsInstance.deleteFile(`${sound._id}.${sound.extension}`); + await CustomSounds.removeById(_id); void api.broadcast('notify.deleteCustomSound', { soundData: sound }); }; diff --git a/apps/meteor/app/emoji-custom/server/methods/deleteEmojiCustom.ts b/apps/meteor/app/emoji-custom/server/methods/deleteEmojiCustom.ts index 9e0adcb90793b..0ca581f7fe1c1 100644 --- a/apps/meteor/app/emoji-custom/server/methods/deleteEmojiCustom.ts +++ b/apps/meteor/app/emoji-custom/server/methods/deleteEmojiCustom.ts @@ -20,7 +20,7 @@ export const deleteEmojiCustom = async (userId: string, emojiID: ICustomEmojiDes throw new Meteor.Error('not_authorized'); } - const emoji = await EmojiCustom.findOneAndDeleteById(emojiID); + const emoji = await EmojiCustom.findOneById(emojiID); if (emoji == null) { throw new Meteor.Error('Custom_Emoji_Error_Invalid_Emoji', 'Invalid emoji', { method: 'deleteEmojiCustom', @@ -28,6 +28,7 @@ export const deleteEmojiCustom = async (userId: string, emojiID: ICustomEmojiDes } await RocketChatFileEmojiCustomInstance.deleteFile(encodeURIComponent(`${emoji.name}.${emoji.extension}`)); + await EmojiCustom.removeById(emojiID); void api.broadcast('emoji.deleteCustom', emoji); return true; From d60013a4ee3a4d8d331c393724a5d2231f231306 Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Mon, 6 Jul 2026 12:21:09 -0600 Subject: [PATCH 09/11] escape CAS username before building regex --- packages/models/src/models/Users.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/models/src/models/Users.ts b/packages/models/src/models/Users.ts index 8980827825882..6a456c413cb88 100644 --- a/packages/models/src/models/Users.ts +++ b/packages/models/src/models/Users.ts @@ -150,7 +150,7 @@ export class UsersRaw extends BaseRaw> implements IU setCasExternalIdByUsername(username: string): Promise { // #TODO: Remove regex based search - const regex = new RegExp(`^${username}$`, 'i'); + const regex = new RegExp(`^${escapeRegExp(username)}$`, 'i'); return this.findOneAndUpdate({ username: regex }, { $set: { 'services.cas.external_id': username } }, { returnDocument: 'after' }); } From 6829148bd9e4c920e480299b39a341a52cecb92f Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Mon, 6 Jul 2026 15:49:59 -0600 Subject: [PATCH 10/11] apply review suggestions: guard clause + permission consts --- .../methods/incoming/deleteIncomingIntegration.ts | 10 +++++----- .../methods/outgoing/deleteOutgoingIntegration.ts | 9 ++++----- .../app/meteor-accounts-saml/server/lib/settings.ts | 6 ++++-- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/apps/meteor/app/integrations/server/methods/incoming/deleteIncomingIntegration.ts b/apps/meteor/app/integrations/server/methods/incoming/deleteIncomingIntegration.ts index 10e760bc33e97..afa0cc4c84461 100644 --- a/apps/meteor/app/integrations/server/methods/incoming/deleteIncomingIntegration.ts +++ b/apps/meteor/app/integrations/server/methods/incoming/deleteIncomingIntegration.ts @@ -14,11 +14,11 @@ declare module '@rocket.chat/ddp-client' { } export const deleteIncomingIntegration = async (integrationId: string, userId: string): Promise => { - let canManageAllIntegrations = false; + const canManageAllIntegrations = !!userId && (await hasPermissionAsync(userId, 'manage-incoming-integrations')); + const canManageOwnIntegrations = + !canManageAllIntegrations && !!userId && (await hasPermissionAsync(userId, 'manage-own-incoming-integrations')); - if (userId && (await hasPermissionAsync(userId, 'manage-incoming-integrations'))) { - canManageAllIntegrations = true; - } else if (!userId || !(await hasPermissionAsync(userId, 'manage-own-incoming-integrations'))) { + if (!canManageAllIntegrations && !canManageOwnIntegrations) { throw new Meteor.Error('not_authorized', 'Unauthorized', { method: 'deleteIncomingIntegration', }); @@ -26,7 +26,7 @@ export const deleteIncomingIntegration = async (integrationId: string, userId: s const integration = await Integrations.removeByIdAndCreatedByIfExists({ _id: integrationId, - ...(!canManageAllIntegrations && { createdBy: userId }), + ...(canManageOwnIntegrations && { createdBy: userId }), }); if (!integration) { diff --git a/apps/meteor/app/integrations/server/methods/outgoing/deleteOutgoingIntegration.ts b/apps/meteor/app/integrations/server/methods/outgoing/deleteOutgoingIntegration.ts index a659386817f2c..4a4f82af2abd2 100644 --- a/apps/meteor/app/integrations/server/methods/outgoing/deleteOutgoingIntegration.ts +++ b/apps/meteor/app/integrations/server/methods/outgoing/deleteOutgoingIntegration.ts @@ -20,11 +20,10 @@ export const deleteOutgoingIntegration = async (integrationId: string, userId: s }); } - let canManageAllIntegrations = false; + const canManageAllIntegrations = await hasPermissionAsync(userId, 'manage-outgoing-integrations'); + const canManageOwnIntegrations = !canManageAllIntegrations && (await hasPermissionAsync(userId, 'manage-own-outgoing-integrations')); - if (await hasPermissionAsync(userId, 'manage-outgoing-integrations')) { - canManageAllIntegrations = true; - } else if (!(await hasPermissionAsync(userId, 'manage-own-outgoing-integrations'))) { + if (!canManageAllIntegrations && !canManageOwnIntegrations) { throw new Meteor.Error('not_authorized', 'Unauthorized', { method: 'deleteOutgoingIntegration', }); @@ -32,7 +31,7 @@ export const deleteOutgoingIntegration = async (integrationId: string, userId: s const integration = await Integrations.removeByIdAndCreatedByIfExists({ _id: integrationId, - ...(!canManageAllIntegrations && { createdBy: userId }), + ...(canManageOwnIntegrations && { createdBy: userId }), }); if (!integration) { diff --git a/apps/meteor/app/meteor-accounts-saml/server/lib/settings.ts b/apps/meteor/app/meteor-accounts-saml/server/lib/settings.ts index b068d011a4696..7231d6fa59ad8 100644 --- a/apps/meteor/app/meteor-accounts-saml/server/lib/settings.ts +++ b/apps/meteor/app/meteor-accounts-saml/server/lib/settings.ts @@ -151,10 +151,12 @@ export const loadSamlServiceProviders = async function (): Promise { } const service = await LoginServiceConfiguration.removeByService(serviceName); - if (service) { - void notifyOnLoginServiceConfigurationChanged({ _id: service._id }, 'removed'); + if (!service) { + return false; } + void notifyOnLoginServiceConfigurationChanged({ _id: service._id }, 'removed'); + return false; }), ) From bef7c8abdec939594631cff83d21fb80ea7e5280 Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Mon, 6 Jul 2026 16:30:51 -0600 Subject: [PATCH 11/11] review --- .../incoming/deleteIncomingIntegration.ts | 4 ++-- .../outgoing/deleteOutgoingIntegration.ts | 4 ++-- .../server/admin/methods/deleteOAuthApp.ts | 2 +- .../meteor/server/lib/cas/loginHandler.spec.ts | 18 +++++++++++------- packages/models/src/models/Users.ts | 12 ++++++++---- 5 files changed, 24 insertions(+), 16 deletions(-) diff --git a/apps/meteor/app/integrations/server/methods/incoming/deleteIncomingIntegration.ts b/apps/meteor/app/integrations/server/methods/incoming/deleteIncomingIntegration.ts index afa0cc4c84461..8d37888f37589 100644 --- a/apps/meteor/app/integrations/server/methods/incoming/deleteIncomingIntegration.ts +++ b/apps/meteor/app/integrations/server/methods/incoming/deleteIncomingIntegration.ts @@ -4,7 +4,7 @@ import { Meteor } from 'meteor/meteor'; import { hasPermissionAsync } from '../../../../authorization/server/functions/hasPermission'; import { methodDeprecationLogger } from '../../../../lib/server/lib/deprecationWarningLogger'; -import { notifyOnIntegrationChangedById } from '../../../../lib/server/lib/notifyListener'; +import { notifyOnIntegrationChanged } from '../../../../lib/server/lib/notifyListener'; declare module '@rocket.chat/ddp-client' { // eslint-disable-next-line @typescript-eslint/naming-convention @@ -35,7 +35,7 @@ export const deleteIncomingIntegration = async (integrationId: string, userId: s }); } - void notifyOnIntegrationChangedById(integrationId, 'removed'); + void notifyOnIntegrationChanged(integration, 'removed'); }; Meteor.methods({ diff --git a/apps/meteor/app/integrations/server/methods/outgoing/deleteOutgoingIntegration.ts b/apps/meteor/app/integrations/server/methods/outgoing/deleteOutgoingIntegration.ts index 4a4f82af2abd2..2c9e894d6f910 100644 --- a/apps/meteor/app/integrations/server/methods/outgoing/deleteOutgoingIntegration.ts +++ b/apps/meteor/app/integrations/server/methods/outgoing/deleteOutgoingIntegration.ts @@ -4,7 +4,7 @@ import { Meteor } from 'meteor/meteor'; import { hasPermissionAsync } from '../../../../authorization/server/functions/hasPermission'; import { methodDeprecationLogger } from '../../../../lib/server/lib/deprecationWarningLogger'; -import { notifyOnIntegrationChangedById } from '../../../../lib/server/lib/notifyListener'; +import { notifyOnIntegrationChanged } from '../../../../lib/server/lib/notifyListener'; declare module '@rocket.chat/ddp-client' { // eslint-disable-next-line @typescript-eslint/naming-convention @@ -42,7 +42,7 @@ export const deleteOutgoingIntegration = async (integrationId: string, userId: s // Don't sending to IntegrationHistory listener since it don't waits for 'removed' events. await IntegrationHistory.removeByIntegrationId(integrationId); - void notifyOnIntegrationChangedById(integrationId, 'removed'); + void notifyOnIntegrationChanged(integration, 'removed'); }; Meteor.methods({ diff --git a/apps/meteor/app/oauth2-server-config/server/admin/methods/deleteOAuthApp.ts b/apps/meteor/app/oauth2-server-config/server/admin/methods/deleteOAuthApp.ts index 872eff77f1ab6..bff067a10526e 100644 --- a/apps/meteor/app/oauth2-server-config/server/admin/methods/deleteOAuthApp.ts +++ b/apps/meteor/app/oauth2-server-config/server/admin/methods/deleteOAuthApp.ts @@ -18,7 +18,7 @@ export const deleteOAuthApp = async (userId: string, applicationId: IOAuthApps[' throw new Meteor.Error('error-not-allowed', 'Not allowed', { method: 'deleteOAuthApp' }); } - const application = await OAuthApps.findOneAndDeleteById(applicationId); + const application = await OAuthApps.findOneAndDeleteById(applicationId, { projection: { clientId: 1 } }); if (!application) { throw new Meteor.Error('error-application-not-found', 'Application not found', { method: 'deleteOAuthApp', diff --git a/apps/meteor/server/lib/cas/loginHandler.spec.ts b/apps/meteor/server/lib/cas/loginHandler.spec.ts index 8ba620d2c7f54..7f9d8d61822c9 100644 --- a/apps/meteor/server/lib/cas/loginHandler.spec.ts +++ b/apps/meteor/server/lib/cas/loginHandler.spec.ts @@ -3,14 +3,13 @@ import { describe, it, beforeEach } from 'mocha'; import proxyquire from 'proxyquire'; import sinon from 'sinon'; -const findOneNotExpiredById = sinon.stub().resolves(null); -const removeById = sinon.stub().resolves(); +const removeNotExpiredById = sinon.stub().resolves(null); const findExistingCASUser = sinon.stub().resolves(null); const settingsGet = sinon.stub().returns(true); const { loginHandlerCAS: handler } = proxyquire.noCallThru().load('./loginHandler', { '@rocket.chat/models': { - CredentialTokens: { findOneNotExpiredById, removeById }, + CredentialTokens: { removeNotExpiredById }, Users: { updateOne: sinon.stub().resolves() }, }, 'meteor/accounts-base': { @@ -30,8 +29,8 @@ const { loginHandlerCAS: handler } = proxyquire.noCallThru().load('./loginHandle describe('loginHandlerCAS', () => { beforeEach(() => { - findOneNotExpiredById.reset(); - removeById.reset(); + removeNotExpiredById.reset(); + removeNotExpiredById.resolves(null); findExistingCASUser.reset(); settingsGet.reset(); settingsGet.returns(true); @@ -44,13 +43,18 @@ describe('loginHandlerCAS', () => { expect(await handler({ cas: { credentialToken: ['a'] } })).to.be.undefined; expect(await handler({ cas: { credentialToken: null } })).to.be.undefined; - expect(findOneNotExpiredById.called).to.be.false; + expect(removeNotExpiredById.called).to.be.false; + }); + + it('should consume the credential token and reject when no matching login attempt is found', async () => { + await expect(handler({ cas: { credentialToken: 'valid-token' } })).to.be.rejected; + expect(removeNotExpiredById.calledOnceWith('valid-token')).to.be.true; }); it('should return undefined when CAS is disabled', async () => { settingsGet.returns(false); expect(await handler({ cas: { credentialToken: 'valid-token' } })).to.be.undefined; - expect(findOneNotExpiredById.called).to.be.false; + expect(removeNotExpiredById.called).to.be.false; }); }); diff --git a/packages/models/src/models/Users.ts b/packages/models/src/models/Users.ts index 6a456c413cb88..cd46792cc9a6e 100644 --- a/packages/models/src/models/Users.ts +++ b/packages/models/src/models/Users.ts @@ -33,6 +33,8 @@ import { Rooms, Subscriptions } from '../index'; import { BaseRaw } from './BaseRaw'; import { queryAvailableAgentsForSelection, queryStatusAgentOnline } from '../helpers'; +const usersDefaultFields = { __rooms: 0 } as const; + export class UsersRaw extends BaseRaw> implements IUsersModel { constructor(db: Db, trash?: Collection>) { super(db, 'users', trash, { @@ -41,9 +43,7 @@ export class UsersRaw extends BaseRaw> implements IU }, }); - this.defaultFields = { - __rooms: 0, - }; + this.defaultFields = usersDefaultFields; } // Move index from constructor to here @@ -151,7 +151,11 @@ export class UsersRaw extends BaseRaw> implements IU setCasExternalIdByUsername(username: string): Promise { // #TODO: Remove regex based search const regex = new RegExp(`^${escapeRegExp(username)}$`, 'i'); - return this.findOneAndUpdate({ username: regex }, { $set: { 'services.cas.external_id': username } }, { returnDocument: 'after' }); + return this.findOneAndUpdate( + { username: regex }, + { $set: { 'services.cas.external_id': username } }, + { returnDocument: 'after', projection: usersDefaultFields }, + ); } /**