chore: remove comments and improve dm detection

Author: ricardogarim

apps/meteor/server/api/v1/rooms.ts

@@ -61,7 +61,7 @@ import { FileUpload } from '../../lib/media/file-upload';
 import { notifyOnSubscriptionChanged } from '../../lib/notifyListener';
 import { openRoom } from '../../lib/openRoom';
 import type { RoomRoles } from '../../lib/roles/getRoomRoles';
-import { parseDirectRoomTargets } from '../../lib/rooms/findDirectRoomByIdentifier';
+import { resolveDirectRoomTargets } from '../../lib/rooms/findDirectRoomByIdentifier';
 import { syncRolePrioritiesForRoomIfRequired } from '../../lib/rooms/syncRolePrioritiesForRoomIfRequired';
 import { unbanUserFromRoom } from '../../lib/unbanUserFromRoom';
 import { createDirectMessage } from '../../meteor-methods/messages/createDirectMessage';
@@ -523,9 +523,6 @@ API.v1.post(
 	'rooms.getOrCreate',
 	{
 		authRequired: false,
-		// Opting out, not omitting: the limiter keys on IP, so a cap here would be shared by every user
-		// behind the same egress address, and omitting this inherits the 10/min default. Still open:
-		// this route can create rooms, so the right guard is likely per-user on the create branch.
 		rateLimiterOptions: false,
 		body: ajv.compile<{ type: RoomType; name: string }>({
 			type: 'object',
@@ -564,7 +561,12 @@ API.v1.post(
 			return API.v1.failure('Invalid room [error-invalid-room]', 'error-invalid-room');
 		}
 
-		const { rid } = await createDirectMessage(parseDirectRoomTargets(name), this.userId);
+		const targets = await resolveDirectRoomTargets(name);
+		if (!targets) {
+			return API.v1.failure('Invalid room [error-invalid-room]', 'error-invalid-room');
+		}
+
+		const { rid } = await createDirectMessage(targets, this.userId);
 
 		const created = await findRoomByTypeAndName(this.userId ?? null, type, rid);
 		if (!created) {

apps/meteor/server/lib/rooms/findDirectRoomByIdentifier.ts

@@ -1,13 +1,22 @@
 import type { IRoom, IUser } from '@rocket.chat/core-typings';
 import { Rooms, Users } from '@rocket.chat/models';
 
-// `/direct/:rid` carries either a room id or the participants themselves: one username for a
-// regular DM, a comma separated list for a group one. Lookup and creation must read it the same
-// way, so both go through here.
 export const parseDirectRoomTargets = (identifier: string): string[] => identifier.split(',').map((username) => username.trim());
 
-// Direct rooms carry no `name`; the member set is what identifies them, and it is the same
-// primitive `createDirectRoom` resolves against.
+const resolveUsernames = async (usernames: string[]): Promise<Pick<IUser, '_id' | 'username'>[] | null> => {
+	const users = await Users.findUsersByUsernames<Pick<IUser, '_id' | 'username'>>(usernames, {
+		projection: { _id: 1, username: 1 },
+	}).toArray();
+
+	return users.length === usernames.length ? users : null;
+};
+
+export const resolveDirectRoomTargets = async (identifier: string): Promise<string[] | null> => {
+	const targets = [...new Set(parseDirectRoomTargets(identifier))];
+
+	return (await resolveUsernames(targets)) ? targets : null;
+};
+
 export const findDirectRoomByIdentifier = async (identifier: string, user: Pick<IUser, '_id' | 'username'>): Promise<IRoom | null> => {
 	const targets = parseDirectRoomTargets(identifier);
 
@@ -22,9 +31,8 @@ export const findDirectRoomByIdentifier = async (identifier: string, user: Pick<
 		return null;
 	}
 
-	const usernames = [...new Set([user.username, ...targets])];
-	const members = await Users.findUsersByUsernames(usernames, { projection: { _id: 1 } }).toArray();
-	if (members.length !== usernames.length) {
+	const members = await resolveUsernames([...new Set([user.username, ...targets])]);
+	if (!members) {
 		return null;
 	}
 

apps/meteor/tests/end-to-end/api/rooms.ts

@@ -32,7 +32,7 @@ import {
 import { assignRoleToUser, createCustomRole, deleteCustomRole } from '../../data/roles.helper';
 import { createRoom, deleteRoom } from '../../data/rooms.helper';
 import { createTeam, deleteTeam } from '../../data/teams.helper';
-import { password } from '../../data/user';
+import { adminUsername, password } from '../../data/user';
 import type { TestUser } from '../../data/users.helper';
 import { createUser, deleteUser, login } from '../../data/users.helper';
 import { IS_EE } from '../../e2e/config/constants';
@@ -138,6 +138,35 @@ describe('[Rooms]', () => {
 			expect(res.body.room).to.have.property('_id', publicChannel._id);
 		});
 
+		it('should create the self-DM when the caller addresses their own username', async () => {
+			const res = await request.post(api('rooms.getOrCreate')).set(credentials).send({ type: 'd', name: adminUsername }).expect(200);
+
+			expect(res.body.room).to.have.property('t', 'd');
+			expect(res.body.room).to.have.property('usersCount', 1);
+		});
+
+		it('should fail when a direct message target does not exist, instead of creating a smaller room', async () => {
+			const res = await request
+				.post(api('rooms.getOrCreate'))
+				.set(credentials)
+				.send({ type: 'd', name: `ghost-${Date.now()}` })
+				.expect(400);
+
+			expect(res.body).to.have.property('success', false);
+			expect(res.body).to.have.property('errorType', 'error-invalid-room');
+		});
+
+		it('should fail when one of the group direct message targets does not exist', async () => {
+			const res = await request
+				.post(api('rooms.getOrCreate'))
+				.set(credentials)
+				.send({ type: 'd', name: `${dmTarget.username},ghost-${Date.now()}` })
+				.expect(400);
+
+			expect(res.body).to.have.property('success', false);
+			expect(res.body).to.have.property('errorType', 'error-invalid-room');
+		});
+
 		it('should fail for a channel that does not exist, since only DMs can be created on demand', async () => {
 			const res = await request
 				.post(api('rooms.getOrCreate'))

apps/meteor/tests/unit/server/lib/rooms/findDirectRoomByIdentifier.spec.ts

@@ -12,9 +12,11 @@ const UsersStub = {
 	findUsersByUsernames: Sinon.stub(),
 };
 
-const { findDirectRoomByIdentifier } = proxyquire.noCallThru().load('../../../../../server/lib/rooms/findDirectRoomByIdentifier.ts', {
-	'@rocket.chat/models': { Rooms: RoomsStub, Users: UsersStub },
-});
+const { findDirectRoomByIdentifier, resolveDirectRoomTargets } = proxyquire
+	.noCallThru()
+	.load('../../../../../server/lib/rooms/findDirectRoomByIdentifier.ts', {
+		'@rocket.chat/models': { Rooms: RoomsStub, Users: UsersStub },
+	});
 
 const cursorOf = (docs: unknown[]) => ({ toArray: async () => docs });
 
@@ -102,3 +104,28 @@ describe('findDirectRoomByIdentifier', () => {
 		expect(await findDirectRoomByIdentifier('alice', { _id: 'me' })).to.be.null;
 	});
 });
+
+describe('resolveDirectRoomTargets', () => {
+	beforeEach(() => {
+		UsersStub.findUsersByUsernames.reset();
+	});
+
+	it('should return the targets when every username resolves to a user', async () => {
+		UsersStub.findUsersByUsernames.returns(cursorOf([{ _id: 'a' }, { _id: 'b' }]));
+
+		expect(await resolveDirectRoomTargets('a, b')).to.deep.equal(['a', 'b']);
+	});
+
+	it('should return null when any username does not resolve to a user', async () => {
+		UsersStub.findUsersByUsernames.returns(cursorOf([{ _id: 'a' }]));
+
+		expect(await resolveDirectRoomTargets('a,ghost')).to.be.null;
+	});
+
+	it('should not ask for the same username twice', async () => {
+		UsersStub.findUsersByUsernames.returns(cursorOf([{ _id: 'a' }]));
+
+		expect(await resolveDirectRoomTargets('a,a')).to.deep.equal(['a']);
+		expect(UsersStub.findUsersByUsernames.firstCall.args[0]).to.deep.equal(['a']);
+	});
+});