chore: atomic updates

.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, and deleting an integration now enforces the creator-only permission scope in the delete itself

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
@@ -14,29 +14,28 @@ declare module '@rocket.chat/ddp-client' {
 }
 
 export const deleteIncomingIntegration = async (integrationId: string, userId: string): Promise<void> => {
-	let integration;
-
-	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 {
+	const canManageAllIntegrations = !!userId && (await hasPermissionAsync(userId, 'manage-incoming-integrations'));
+	const canManageOwnIntegrations =
+		!canManageAllIntegrations && !!userId && (await hasPermissionAsync(userId, 'manage-own-incoming-integrations'));
+
+	if (!canManageAllIntegrations && !canManageOwnIntegrations) {
 		throw new Meteor.Error('not_authorized', 'Unauthorized', {
 			method: 'deleteIncomingIntegration',
 		});
 	}
 
-	if (!(await integration)) {
+	const integration = await Integrations.removeByIdAndCreatedByIfExists({
+		_id: integrationId,
+		...(canManageOwnIntegrations && { createdBy: userId }),
+	});
+
+	if (!integration) {
 		throw new Meteor.Error('error-invalid-integration', 'Invalid integration', {
 			method: 'deleteIncomingIntegration',
 		});
 	}
 
-	await Integrations.removeById(integrationId);
-	void notifyOnIntegrationChangedById(integrationId, 'removed');
+	void notifyOnIntegrationChanged(integration, 'removed');
 };
 
 Meteor.methods<ServerMethods>({

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
@@ -14,37 +14,35 @@ declare module '@rocket.chat/ddp-client' {
 }
 
 export const deleteOutgoingIntegration = async (integrationId: string, userId: string): Promise<void> => {
-	let integration;
-
 	if (!userId) {
 		throw new Meteor.Error('not_authorized', 'Unauthorized', {
 			method: 'deleteOutgoingIntegration',
 		});
 	}
 
-	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 {
+	const canManageAllIntegrations = await hasPermissionAsync(userId, 'manage-outgoing-integrations');
+	const canManageOwnIntegrations = !canManageAllIntegrations && (await hasPermissionAsync(userId, 'manage-own-outgoing-integrations'));
+
+	if (!canManageAllIntegrations && !canManageOwnIntegrations) {
 		throw new Meteor.Error('not_authorized', 'Unauthorized', {
 			method: 'deleteOutgoingIntegration',
 		});
 	}
 
-	if (!(await integration)) {
+	const integration = await Integrations.removeByIdAndCreatedByIfExists({
+		_id: integrationId,
+		...(canManageOwnIntegrations && { 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');
+	void notifyOnIntegrationChanged(integration, 'removed');
 };
 
 Meteor.methods<ServerMethods>({

apps/meteor/app/invites/server/functions/removeInvite.ts

@@ -20,16 +20,13 @@ export const removeInvite = async (userId: string, invite: Pick<IInvite, '_id'>)
 		});
 	}
 
-	// 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;
 };

apps/meteor/app/meteor-accounts-saml/server/lib/settings.ts

@@ -150,13 +150,8 @@ export const loadSamlServiceProviders = async function (): Promise<void> {
 					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 { deletedCount } = await LoginServiceConfiguration.removeService(service._id);
-				if (!deletedCount) {
+				const service = await LoginServiceConfiguration.removeByService(serviceName);
+				if (!service) {
 					return false;
 				}
 

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, { projection: { clientId: 1 } });
 	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 });
 

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<ServerMethods>({

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;

apps/meteor/server/lib/cas/findExistingCASUser.ts

@@ -16,12 +16,6 @@ export const findExistingCASUser = async (username: string): Promise<IUser | und
 	// If that user was not found, check if there's any Rocket.Chat user with that username
 	// With this, CAS login will continue to work if the user is renamed on both sides and also if the user is renamed only on Rocket.Chat.
 	// It'll also allow non-CAS users to switch to CAS based login
-	// #TODO: Remove regex based search
-	const regex = new RegExp(`^${username}$`, 'i');
-	const user = await Users.findOne({ username: regex });
-	if (user) {
-		// Update the user's external_id to reflect this new username.
-		await Users.updateOne({ _id: user._id }, { $set: { 'services.cas.external_id': username } });
-		return user;
-	}
+	// Update the user's external_id to reflect this new username.
+	return (await Users.setCasExternalIdByUsername(username)) ?? undefined;
 };

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;
 	});
 });

apps/meteor/server/lib/cas/loginHandler.ts

@@ -14,14 +14,11 @@ export const loginHandlerCAS = async (options: any): Promise<undefined | Account
 		return undefined;
 	}
 
-	// TODO: Sync wrapper due to the chain conversion to async models
-	const credentials = await CredentialTokens.findOneNotExpiredById(options.cas.credentialToken);
+	const credentials = await CredentialTokens.removeNotExpiredById(options.cas.credentialToken);
 	if (credentials === undefined || credentials === null) {
 		throw new Meteor.Error(Accounts.LoginCancelledError.numericError, 'no matching login attempt found');
 	}
 
-	await CredentialTokens.removeById(credentials._id);
-
 	const result = credentials.userInfo;
 	const syncUserDataFieldMap = settings.get<string>('CAS_Sync_User_Data_FieldMap').trim();
 	const casVersion = parseFloat(settings.get('CAS_version') ?? '1.0');

apps/meteor/server/lib/oauth/updateOAuthServices.ts

@@ -119,12 +119,9 @@ export async function updateOAuthServices(): Promise<void> {
 			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');
 			}
 		}
 	}

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<void> {
-		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');
 	}
 

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<string>): Promise<boolean> {
-		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<string>): Promise<boolean> {
-		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<string>): Promise<boolean> {
-		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<Omit<ITeam, 'usernames'> | null> {