From 544b64855486e3b22d50ad528cac5b03109f5e58 Mon Sep 17 00:00:00 2001 From: Ricardo Garim Date: Tue, 18 Aug 2026 11:20:33 -0300 Subject: [PATCH 1/3] fix: direct messages addressed by username never resolve --- .changeset/witty-beds-bow.md | 8 ++ .../meteor/client/lib/utils/mapRoomFromApi.ts | 19 ++++ .../views/room/hooks/useOpenRoom.spec.ts | 28 ++--- .../client/views/room/hooks/useOpenRoom.ts | 36 ++---- apps/meteor/server/api/v1/rooms.ts | 63 ++++++++++- .../lib/rooms/findDirectRoomByIdentifier.ts | 34 ++++++ apps/meteor/server/publications/room/index.ts | 96 +++++++++------- apps/meteor/tests/end-to-end/api/methods.ts | 94 ++++++++++++++++ apps/meteor/tests/end-to-end/api/rooms.ts | 54 +++++++++ .../rooms/findDirectRoomByIdentifier.spec.ts | 104 ++++++++++++++++++ packages/rest-typings/src/v1/rooms.ts | 5 + 11 files changed, 461 insertions(+), 80 deletions(-) create mode 100644 .changeset/witty-beds-bow.md create mode 100644 apps/meteor/client/lib/utils/mapRoomFromApi.ts create mode 100644 apps/meteor/server/lib/rooms/findDirectRoomByIdentifier.ts create mode 100644 apps/meteor/tests/unit/server/lib/rooms/findDirectRoomByIdentifier.spec.ts diff --git a/.changeset/witty-beds-bow.md b/.changeset/witty-beds-bow.md new file mode 100644 index 0000000000000..49806bbcbfd33 --- /dev/null +++ b/.changeset/witty-beds-bow.md @@ -0,0 +1,8 @@ +--- +'@rocket.chat/rest-typings': minor +'@rocket.chat/meteor': minor +--- + +Adds `POST /v1/rooms.getOrCreate`, an idempotent endpoint that resolves a room by type and name, creating the direct message when it does not exist yet. + +Uses it to fix direct messages addressed by username, such as the ones opened with **Reply in direct message**, not being found on the first lookup — which made opening a conversation cost an extra request and log an avoidable `Invalid Room` error. diff --git a/apps/meteor/client/lib/utils/mapRoomFromApi.ts b/apps/meteor/client/lib/utils/mapRoomFromApi.ts new file mode 100644 index 0000000000000..ceaabfca90507 --- /dev/null +++ b/apps/meteor/client/lib/utils/mapRoomFromApi.ts @@ -0,0 +1,19 @@ +import type { IOmnichannelRoom, IRoom, Serialized } from '@rocket.chat/core-typings'; + +import { mapMessageFromApi } from './mapMessageFromApi'; + +// Livechat rooms are opened through the same flow, so their own date fields have to be covered too. +type SerializedRoom = Serialized & Partial>>; + +// REST serializes Date fields to strings; the Rooms store is Date-typed. Every date-bearing key +// published by `roomFields` has to be revived, or it lands in the store as a string. +export const mapRoomFromApi = ({ _updatedAt, ts, lm, queuedAt, closedAt, lastMessage, ...room }: SerializedRoom): IRoom => + ({ + ...room, + ...(_updatedAt && { _updatedAt: new Date(_updatedAt) }), + ...(ts && { ts: new Date(ts) }), + ...(lm && { lm: new Date(lm) }), + ...(queuedAt && { queuedAt: new Date(queuedAt) }), + ...(closedAt && { closedAt: new Date(closedAt) }), + ...(lastMessage && { lastMessage: mapMessageFromApi(lastMessage) }), + }) as unknown as IRoom; diff --git a/apps/meteor/client/views/room/hooks/useOpenRoom.spec.ts b/apps/meteor/client/views/room/hooks/useOpenRoom.spec.ts index 82a160f1e9118..4d9d6aae56397 100644 --- a/apps/meteor/client/views/room/hooks/useOpenRoom.spec.ts +++ b/apps/meteor/client/views/room/hooks/useOpenRoom.spec.ts @@ -40,7 +40,7 @@ describe('useOpenRoom', () => { wrapper: mockAppRoot() .withJohnDoe() .withPermission('preview-c-room') - .withMethod('getRoomByTypeAndName', () => channelRoom as any) + .withEndpoint('POST', '/v1/rooms.getOrCreate', () => ({ room: channelRoom }) as any) .build(), }); @@ -80,31 +80,31 @@ describe('useOpenRoom', () => { describe('error classification', () => { it('maps error-no-permission to RoomNotFoundError without retrying (regression for #40991)', async () => { - const getRoomByTypeAndName = jest.fn().mockImplementation(() => { - throw Object.assign(new Error('No permission'), { error: 'error-no-permission' }); + const getOrCreateRoom = jest.fn().mockImplementation(() => { + throw Object.assign(new Error('No permission'), { errorType: 'error-no-permission' }); }); const { result } = renderHook(() => useOpenRoom({ type: 'p', reference: 'private-channel' }), { - wrapper: mockAppRoot().withJohnDoe().withMethod('getRoomByTypeAndName', getRoomByTypeAndName).build(), + wrapper: mockAppRoot().withJohnDoe().withEndpoint('POST', '/v1/rooms.getOrCreate', getOrCreateRoom).build(), }); await waitFor(() => expect(result.current.isError).toBe(true)); expect(result.current.error).toBeInstanceOf(RoomNotFoundError); - expect(getRoomByTypeAndName).toHaveBeenCalledTimes(1); + expect(getOrCreateRoom).toHaveBeenCalledTimes(1); }); - it('maps error-invalid-room to RoomNotFoundError without retrying for non-DM rooms', async () => { - const getRoomByTypeAndName = jest.fn().mockImplementation(() => { - throw Object.assign(new Error('Invalid room'), { error: 'error-invalid-room' }); + it('maps error-invalid-room to RoomNotFoundError without retrying', async () => { + const getOrCreateRoom = jest.fn().mockImplementation(() => { + throw Object.assign(new Error('Invalid room'), { errorType: 'error-invalid-room' }); }); const { result } = renderHook(() => useOpenRoom({ type: 'c', reference: 'missing-channel' }), { - wrapper: mockAppRoot().withJohnDoe().withMethod('getRoomByTypeAndName', getRoomByTypeAndName).build(), + wrapper: mockAppRoot().withJohnDoe().withEndpoint('POST', '/v1/rooms.getOrCreate', getOrCreateRoom).build(), }); await waitFor(() => expect(result.current.isError).toBe(true)); expect(result.current.error).toBeInstanceOf(RoomNotFoundError); - expect(getRoomByTypeAndName).toHaveBeenCalledTimes(1); + expect(getOrCreateRoom).toHaveBeenCalledTimes(1); }); it('retries unclassified transient errors and recovers on a later attempt', async () => { @@ -112,25 +112,25 @@ describe('useOpenRoom', () => { const channelName = 'flaky-channel'; const channelRoom = createFakeRoom({ _id: channelRid, t: 'c', name: channelName }); - const getRoomByTypeAndName = jest + const getOrCreateRoom = jest .fn() .mockImplementationOnce(() => { throw new Error('network down'); }) - .mockImplementation(() => channelRoom); + .mockImplementation(() => ({ room: channelRoom })); const { result } = renderHook(() => useOpenRoom({ type: 'c', reference: channelName }), { wrapper: mockAppRoot() .withJohnDoe() .withPermission('preview-c-room') - .withMethod('getRoomByTypeAndName', getRoomByTypeAndName) + .withEndpoint('POST', '/v1/rooms.getOrCreate', getOrCreateRoom) .build(), }); // Backoff is Math.min(1000 * 2 ** attempt, 5000); first retry fires after ~1s. await waitFor(() => expect(result.current.isSuccess).toBe(true), { timeout: 3000 }); expect(result.current.data?.rid).toBe(channelRid); - expect(getRoomByTypeAndName).toHaveBeenCalledTimes(2); + expect(getOrCreateRoom).toHaveBeenCalledTimes(2); }); }); }); diff --git a/apps/meteor/client/views/room/hooks/useOpenRoom.ts b/apps/meteor/client/views/room/hooks/useOpenRoom.ts index ed6e6547751ae..8fd20dd0240fd 100644 --- a/apps/meteor/client/views/room/hooks/useOpenRoom.ts +++ b/apps/meteor/client/views/room/hooks/useOpenRoom.ts @@ -1,6 +1,6 @@ import { isPublicRoom, type IRoom, type RoomType } from '@rocket.chat/core-typings'; import { getObjectKeys } from '@rocket.chat/tools'; -import { useEndpoint, useMethod, usePermission, useRoute, useSetting, useUser } from '@rocket.chat/ui-contexts'; +import { useEndpoint, usePermission, useRoute, useSetting, useUser } from '@rocket.chat/ui-contexts'; import { useQuery, useQueryClient } from '@tanstack/react-query'; import { useCallback, useEffect } from 'react'; @@ -13,14 +13,14 @@ import { NotSubscribedToRoomError } from '../../../lib/errors/NotSubscribedToRoo import { OldUrlRoomError } from '../../../lib/errors/OldUrlRoomError'; import { RoomNotFoundError } from '../../../lib/errors/RoomNotFoundError'; import { roomsQueryKeys } from '../../../lib/queryKeys'; +import { mapRoomFromApi } from '../../../lib/utils/mapRoomFromApi'; import { Rooms, Subscriptions } from '../../../stores'; export function useOpenRoom({ type, reference }: { type: RoomType; reference: string }) { const user = useUser(); const hasPreviewPermission = usePermission('preview-c-room'); const allowAnonymousRead = useSetting('Accounts_AllowAnonymousRead', true); - const getRoomByTypeAndName = useMethod('getRoomByTypeAndName'); - const createDirectMessage = useEndpoint('POST', '/v1/im.create'); + const getOrCreateRoom = useEndpoint('POST', '/v1/rooms.getOrCreate'); const directRoute = useRoute('direct'); const openRoom = useOpenRoomMutation(); @@ -76,33 +76,19 @@ export function useOpenRoom({ type, reference }: { type: RoomType; reference: st let roomData: IRoom; try { - roomData = await getRoomByTypeAndName(type, reference); + const { room } = await getOrCreateRoom({ type, name: reference }); + roomData = mapRoomFromApi(room); } catch (error) { - const errorCode = error && typeof error === 'object' && 'error' in error ? error.error : undefined; + const errorCode = + error && typeof error === 'object' + ? ('errorType' in error && error.errorType) || ('error' in error && error.error) || undefined + : undefined; - // "No permission" means the room exists but the user can't see it — surface the - // not-found/no-access screen rather than retrying it as a transient failure. - if (errorCode === 'error-no-permission') { + if (errorCode === 'error-no-permission' || errorCode === 'error-invalid-room') { throw new RoomNotFoundError(undefined, { type, reference }); } - if (errorCode !== 'error-invalid-room') { - throw error; - } - - if (type !== 'd') { - throw new RoomNotFoundError(undefined, { type, reference }); - } - - try { - const { room } = await createDirectMessage({ usernames: reference }); - - directRoute.push({ rid: room._id }, (prev) => prev); - } catch (error) { - throw new RoomNotFoundError(undefined, { type, reference }); - } - - throw new OldUrlRoomError(undefined, { type, reference }); + throw error; } if (!roomData._id) { diff --git a/apps/meteor/server/api/v1/rooms.ts b/apps/meteor/server/api/v1/rooms.ts index c12a18653e1df..5b9dc60013a3b 100644 --- a/apps/meteor/server/api/v1/rooms.ts +++ b/apps/meteor/server/api/v1/rooms.ts @@ -9,6 +9,7 @@ import { isPrivateRoom, isPublicRoom, type IUser, + type RoomType, } from '@rocket.chat/core-typings'; import { Messages, Rooms, Users, Uploads, Subscriptions } from '@rocket.chat/models'; import type { Notifications } from '@rocket.chat/rest-typings'; @@ -60,8 +61,10 @@ 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 { syncRolePrioritiesForRoomIfRequired } from '../../lib/rooms/syncRolePrioritiesForRoomIfRequired'; import { unbanUserFromRoom } from '../../lib/unbanUserFromRoom'; +import { createDirectMessage } from '../../meteor-methods/messages/createDirectMessage'; import { createDiscussion } from '../../meteor-methods/messages/createDiscussion'; import { sendFileMessage } from '../../meteor-methods/messages/sendFileMessage'; import { executeArchiveRoom } from '../../meteor-methods/rooms/archiveRoom'; @@ -76,7 +79,7 @@ import { executeUnarchiveRoom } from '../../meteor-methods/rooms/unarchiveRoom'; import { unmuteUserInRoom } from '../../meteor-methods/rooms/unmuteUserInRoom'; import { saveNotificationSettingsMethod } from '../../meteor-methods/users/saveNotificationSettings'; import type { NotificationFieldType } from '../../meteor-methods/users/saveNotificationSettings'; -import { roomsGetMethod } from '../../publications/room'; +import { findRoomByTypeAndName, roomsGetMethod } from '../../publications/room'; import { settings } from '../../settings'; import type { ExtractRoutesFromAPI } from '../ApiClass'; import { API } from '../api'; @@ -109,7 +112,7 @@ export async function findRoomByIdOrName({ }): Promise { if ( (!('roomId' in params) && !('roomName' in params)) || - ('roomId' in params && !(params as { roomId?: string }).roomId && 'roomName' in params && !(params as { roomName?: string }).roomName) + ('roomId' in params && !params.roomId && 'roomName' in params && !(params as { roomName?: string }).roomName) ) { throw new Meteor.Error('error-roomid-param-not-provided', 'The parameter "roomId" or "roomName" is required'); } @@ -516,6 +519,62 @@ API.v1.post( }, ); +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', + properties: { + type: { type: 'string', enum: ['c', 'd', 'p', 'l'] }, + name: { type: 'string', minLength: 1 }, + }, + required: ['type', 'name'], + additionalProperties: false, + }), + response: { + 200: ajv.compile<{ room: IRoom }>({ + type: 'object', + properties: { + room: { type: 'object' }, + success: { type: 'boolean', enum: [true] }, + }, + required: ['room', 'success'], + additionalProperties: false, + }), + 400: validateBadRequestErrorResponse, + 401: validateUnauthorizedErrorResponse, + }, + }, + async function action() { + const { type, name } = this.bodyParams; + + const room = await findRoomByTypeAndName(this.userId ?? null, type, name); + if (room) { + return API.v1.success({ room }); + } + + // Only direct messages can be created on demand; every other type must already exist. The + // caller may be anonymous here, since the route stays readable without a session. + if (type !== 'd' || !this.userId) { + return API.v1.failure('Invalid room [error-invalid-room]', 'error-invalid-room'); + } + + const { rid } = await createDirectMessage(parseDirectRoomTargets(name), this.userId); + + const created = await findRoomByTypeAndName(this.userId ?? null, type, rid); + if (!created) { + return API.v1.failure('Invalid room [error-invalid-room]', 'error-invalid-room'); + } + + return API.v1.success({ room: created }); + }, +); + API.v1.get( 'rooms.info', { diff --git a/apps/meteor/server/lib/rooms/findDirectRoomByIdentifier.ts b/apps/meteor/server/lib/rooms/findDirectRoomByIdentifier.ts new file mode 100644 index 0000000000000..c09f0f86b0ff6 --- /dev/null +++ b/apps/meteor/server/lib/rooms/findDirectRoomByIdentifier.ts @@ -0,0 +1,34 @@ +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. +export const findDirectRoomByIdentifier = async (identifier: string, user: Pick): Promise => { + const targets = parseDirectRoomTargets(identifier); + + if (targets.length === 1) { + const byId = await Rooms.findByTypeAndNameOrId('d', targets[0]); + if (byId) { + return byId; + } + } + + if (!user.username) { + return null; + } + + const usernames = [...new Set([user.username, ...targets])]; + const members = await Users.findUsersByUsernames(usernames, { projection: { _id: 1 } }).toArray(); + if (members.length !== usernames.length) { + return null; + } + + const uids = members.map(({ _id }) => _id).sort(); + + return Rooms.findOneDirectRoomContainingAllUserIDs(uids); +}; diff --git a/apps/meteor/server/publications/room/index.ts b/apps/meteor/server/publications/room/index.ts index 40b30996a4025..76193ceaed5b4 100644 --- a/apps/meteor/server/publications/room/index.ts +++ b/apps/meteor/server/publications/room/index.ts @@ -1,12 +1,13 @@ import type { IOmnichannelRoom, IRoom, RoomType } from '@rocket.chat/core-typings'; import type { ServerMethods } from '@rocket.chat/ddp-client'; -import { Rooms } from '@rocket.chat/models'; +import { Rooms, Users } from '@rocket.chat/models'; import { Meteor } from 'meteor/meteor'; import _ from 'underscore'; import { roomFields } from '../../../lib/publishFields'; import { canAccessRoomAsync } from '../../lib/authorization'; import { hasPermissionAsync } from '../../lib/authorization/hasPermission'; +import { findDirectRoomByIdentifier } from '../../lib/rooms/findDirectRoomByIdentifier'; import { roomCoordinator } from '../../lib/rooms/roomCoordinator'; import { settings } from '../../settings'; @@ -45,55 +46,72 @@ export const roomsGetMethod = async (userId?: string | null, updatedAt?: Date): return (await Rooms.findBySubscriptionUserId(userId, options)).toArray(); }; -Meteor.methods({ - async 'rooms/get'(updatedAt) { - return roomsGetMethod(Meteor.userId(), updatedAt); - }, +export const findRoomByTypeAndName = async (userId: string | null, type: RoomType, name: string): Promise => { + if (!type || !name) { + return null; + } - async 'getRoomByTypeAndName'(type, name) { - if (!type || !name) { - throw new Meteor.Error('error-invalid-room', 'Invalid room', { + const user = userId ? await Users.findOneById(userId) : null; + const isAnonymous = !user?._id; + + if (isAnonymous) { + const allowAnon = settings.get('Accounts_AllowAnonymousRead'); + if (!allowAnon || type !== 'c') { + throw new Meteor.Error('error-invalid-user', 'Invalid user', { method: 'getRoomByTypeAndName', }); } + } - const user = await Meteor.userAsync(); - const isAnonymous = !user?._id; - - if (isAnonymous) { - const allowAnon = settings.get('Accounts_AllowAnonymousRead'); - if (!allowAnon || type !== 'c') { - throw new Meteor.Error('error-invalid-user', 'Invalid user', { - method: 'getRoomByTypeAndName', - }); - } - } + let room: IRoom | IOmnichannelRoom | null | undefined; + if (type === 'd' && user) { + room = await findDirectRoomByIdentifier(name, user); + } else { const roomFind = roomCoordinator.getRoomFind(type); + room = roomFind ? await roomFind(name) : await Rooms.findByTypeAndNameOrId(type, name); + } - const room = roomFind ? await roomFind.call(this, name) : await Rooms.findByTypeAndNameOrId(type, name); + if (!room) { + return null; + } - if (!room) { - throw new Meteor.Error('error-invalid-room', 'Invalid room', { - method: 'getRoomByTypeAndName', - }); - } + if ( + user && + !(await canAccessRoomAsync(room, user, { + includeInvitations: true, + })) + ) { + throw new Meteor.Error('error-no-permission', 'No permission', { + method: 'getRoomByTypeAndName', + }); + } - if ( - user && - !(await canAccessRoomAsync(room, user, { - includeInvitations: true, - })) - ) { - throw new Meteor.Error('error-no-permission', 'No permission', { - method: 'getRoomByTypeAndName', - }); - } + if (settings.get('Store_Last_Message') && user && !(await hasPermissionAsync(user, 'preview-c-room'))) { + delete room.lastMessage; + } - if (settings.get('Store_Last_Message') && user && !(await hasPermissionAsync(user, 'preview-c-room'))) { - delete room.lastMessage; - } + return roomMap(room); +}; + +export const getRoomByTypeAndNameMethod = async (userId: string | null, type: RoomType, name: string): Promise => { + const room = await findRoomByTypeAndName(userId, type, name); + + if (!room) { + throw new Meteor.Error('error-invalid-room', 'Invalid room', { + method: 'getRoomByTypeAndName', + }); + } - return roomMap(room); + return room; +}; + +Meteor.methods({ + async 'rooms/get'(updatedAt) { + return roomsGetMethod(Meteor.userId(), updatedAt); + }, + + async 'getRoomByTypeAndName'(type, name) { + return getRoomByTypeAndNameMethod(Meteor.userId(), type, name); }, }); diff --git a/apps/meteor/tests/end-to-end/api/methods.ts b/apps/meteor/tests/end-to-end/api/methods.ts index 38f71aaa8c1c6..ccffdd982473c 100644 --- a/apps/meteor/tests/end-to-end/api/methods.ts +++ b/apps/meteor/tests/end-to-end/api/methods.ts @@ -2706,6 +2706,100 @@ describe('Meteor.methods', () => { done(); }); }); + + it('should return the room object for a DM addressed by username', async () => { + const res = await request + .post(methodCall('getRoomByTypeAndName')) + .set(credentials) + .send({ + message: JSON.stringify({ + method: 'getRoomByTypeAndName', + params: ['d', testUser2.username], + id: 'id', + msg: 'method', + }), + }) + .expect(200); + + const parsedResponse = JSON.parse(res.body.message); + expect(parsedResponse.result._id).to.equal(dmId); + expect(parsedResponse.result.t).to.equal('d'); + }); + + it('should throw error when the DM addressed by username does not exist', async () => { + const stranger = await createUser(); + + const res = await request + .post(methodCall('getRoomByTypeAndName')) + .set(credentials) + .send({ + message: JSON.stringify({ + method: 'getRoomByTypeAndName', + params: ['d', stranger.username], + id: 'id', + msg: 'method', + }), + }) + .expect(200); + + const parsedResponse = JSON.parse(res.body.message); + expect(parsedResponse).to.have.property('error'); + expect(parsedResponse.error.error).to.equal('error-invalid-room'); + + await deleteUser(stranger); + }); + + it('should return the room object for a group DM addressed by a comma separated username list', async () => { + const usernames = `${testUser.username},${testUser2.username}`; + + const groupDm = (await request.post(api('im.create')).set(credentials).send({ usernames }).expect(200)).body.room; + + const res = await request + .post(methodCall('getRoomByTypeAndName')) + .set(credentials) + .send({ + message: JSON.stringify({ + method: 'getRoomByTypeAndName', + params: ['d', usernames], + id: 'id', + msg: 'method', + }), + }) + .expect(200); + + const parsedResponse = JSON.parse(res.body.message); + expect(parsedResponse.result._id).to.equal(groupDm._id); + + await deleteRoom({ type: 'd', roomId: groupDm._id }); + }); + + it('should keep resolving a DM by username after the other member is renamed', async () => { + const renamedUser = await createUser(); + const renamedDmId = (await request.post(api('im.create')).set(credentials).send({ username: renamedUser.username }).expect(200)).body + .room._id; + + const username = `renamed.${Date.now()}`; + await request.post(api('users.update')).set(credentials).send({ userId: renamedUser._id, data: { username } }).expect(200); + + const res = await request + .post(methodCall('getRoomByTypeAndName')) + .set(credentials) + .send({ + message: JSON.stringify({ + method: 'getRoomByTypeAndName', + params: ['d', username], + id: 'id', + msg: 'method', + }), + }) + .expect(200); + + const parsedResponse = JSON.parse(res.body.message); + expect(parsedResponse.result._id).to.equal(renamedDmId); + + await deleteRoom({ type: 'd', roomId: renamedDmId }); + await deleteUser(renamedUser); + }); }); describe('[@setUserActiveStatus]', () => { diff --git a/apps/meteor/tests/end-to-end/api/rooms.ts b/apps/meteor/tests/end-to-end/api/rooms.ts index 022b6434b1e70..dd73cd60d0e3d 100644 --- a/apps/meteor/tests/end-to-end/api/rooms.ts +++ b/apps/meteor/tests/end-to-end/api/rooms.ts @@ -96,6 +96,60 @@ describe('[Rooms]', () => { }); }); + describe('/rooms.getOrCreate', () => { + let dmTarget: TestUser; + let publicChannel: IRoom; + + before(async () => { + dmTarget = await createUser(); + publicChannel = (await createRoom({ type: 'c', name: `getorcreate-${Date.now()}` })).body.channel; + }); + + after(async () => { + await Promise.all([deleteRoom({ type: 'c', roomId: publicChannel._id }), deleteUser(dmTarget)]); + }); + + it('should create the direct message when it does not exist yet', async () => { + const res = await request.post(api('rooms.getOrCreate')).set(credentials).send({ type: 'd', name: dmTarget.username }).expect(200); + + expect(res.body).to.have.property('success', true); + expect(res.body.room).to.have.property('t', 'd'); + expect(res.body.room).to.have.property('_id').that.is.a('string'); + + await deleteRoom({ type: 'd', roomId: res.body.room._id }); + }); + + it('should be idempotent, returning the same room on a second call', async () => { + const first = (await request.post(api('rooms.getOrCreate')).set(credentials).send({ type: 'd', name: dmTarget.username }).expect(200)) + .body.room; + + const second = ( + await request.post(api('rooms.getOrCreate')).set(credentials).send({ type: 'd', name: dmTarget.username }).expect(200) + ).body.room; + + expect(second._id).to.equal(first._id); + + await deleteRoom({ type: 'd', roomId: first._id }); + }); + + it('should resolve an existing channel by name without creating anything', async () => { + const res = await request.post(api('rooms.getOrCreate')).set(credentials).send({ type: 'c', name: publicChannel.name }).expect(200); + + expect(res.body.room).to.have.property('_id', publicChannel._id); + }); + + 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')) + .set(credentials) + .send({ type: 'c', name: `missing-${Date.now()}` }) + .expect(400); + + expect(res.body).to.have.property('success', false); + expect(res.body).to.have.property('errorType', 'error-invalid-room'); + }); + }); + describe('[/rooms.saveDraft]', () => { let testChannel: IRoom; let threadId: IMessage['_id']; diff --git a/apps/meteor/tests/unit/server/lib/rooms/findDirectRoomByIdentifier.spec.ts b/apps/meteor/tests/unit/server/lib/rooms/findDirectRoomByIdentifier.spec.ts new file mode 100644 index 0000000000000..11132693c7bb5 --- /dev/null +++ b/apps/meteor/tests/unit/server/lib/rooms/findDirectRoomByIdentifier.spec.ts @@ -0,0 +1,104 @@ +import { expect } from 'chai'; +import { describe, it, beforeEach } from 'mocha'; +import proxyquire from 'proxyquire'; +import Sinon from 'sinon'; + +const RoomsStub = { + findByTypeAndNameOrId: Sinon.stub(), + findOneDirectRoomContainingAllUserIDs: Sinon.stub(), +}; + +const UsersStub = { + findUsersByUsernames: Sinon.stub(), +}; + +const { findDirectRoomByIdentifier } = proxyquire.noCallThru().load('../../../../../server/lib/rooms/findDirectRoomByIdentifier.ts', { + '@rocket.chat/models': { Rooms: RoomsStub, Users: UsersStub }, +}); + +const cursorOf = (docs: unknown[]) => ({ toArray: async () => docs }); + +const me = { _id: 'me', username: 'me' }; + +describe('findDirectRoomByIdentifier', () => { + beforeEach(() => { + RoomsStub.findByTypeAndNameOrId.reset(); + RoomsStub.findOneDirectRoomContainingAllUserIDs.reset(); + UsersStub.findUsersByUsernames.reset(); + + RoomsStub.findByTypeAndNameOrId.resolves(null); + }); + + it('should return the room when the identifier is a room id', async () => { + const room = { _id: 'rid1', t: 'd' }; + RoomsStub.findByTypeAndNameOrId.resolves(room); + + expect(await findDirectRoomByIdentifier('rid1', me)).to.equal(room); + expect(UsersStub.findUsersByUsernames.called).to.be.false; + }); + + it('should resolve a two-person DM by the other username, over sorted uids', async () => { + const room = { _id: 'rid1', t: 'd' }; + UsersStub.findUsersByUsernames.returns(cursorOf([{ _id: 'zeta' }, { _id: 'alpha' }])); + RoomsStub.findOneDirectRoomContainingAllUserIDs.resolves(room); + + expect(await findDirectRoomByIdentifier('alice', me)).to.equal(room); + expect(RoomsStub.findOneDirectRoomContainingAllUserIDs.calledWith(['alpha', 'zeta'])).to.be.true; + }); + + it('should include the caller in the member set exactly once for a self-DM', async () => { + UsersStub.findUsersByUsernames.returns(cursorOf([{ _id: 'me' }])); + RoomsStub.findOneDirectRoomContainingAllUserIDs.resolves({ _id: 'self', t: 'd' }); + + await findDirectRoomByIdentifier('me', me); + + expect(UsersStub.findUsersByUsernames.firstCall.args[0]).to.deep.equal(['me']); + expect(RoomsStub.findOneDirectRoomContainingAllUserIDs.calledWith(['me'])).to.be.true; + }); + + it('should resolve a group DM from a comma-separated identifier', async () => { + UsersStub.findUsersByUsernames.returns(cursorOf([{ _id: 'me' }, { _id: 'a' }, { _id: 'b' }])); + RoomsStub.findOneDirectRoomContainingAllUserIDs.resolves({ _id: 'group', t: 'd' }); + + await findDirectRoomByIdentifier('a,b', me); + + expect(UsersStub.findUsersByUsernames.firstCall.args[0]).to.deep.equal(['me', 'a', 'b']); + expect(RoomsStub.findOneDirectRoomContainingAllUserIDs.calledWith(['a', 'b', 'me'])).to.be.true; + }); + + it('should not look the identifier up as a room id when it is a username list', async () => { + UsersStub.findUsersByUsernames.returns(cursorOf([{ _id: 'me' }, { _id: 'a' }, { _id: 'b' }])); + RoomsStub.findOneDirectRoomContainingAllUserIDs.resolves({ _id: 'group', t: 'd' }); + + await findDirectRoomByIdentifier('a,b', me); + + expect(RoomsStub.findByTypeAndNameOrId.called).to.be.false; + }); + + it('should trim whitespace around each username in the list', async () => { + UsersStub.findUsersByUsernames.returns(cursorOf([{ _id: 'me' }, { _id: 'a' }, { _id: 'b' }])); + RoomsStub.findOneDirectRoomContainingAllUserIDs.resolves({ _id: 'group', t: 'd' }); + + await findDirectRoomByIdentifier('a, b', me); + + expect(UsersStub.findUsersByUsernames.firstCall.args[0]).to.deep.equal(['me', 'a', 'b']); + }); + + it('should return null when any username does not resolve to a user', async () => { + UsersStub.findUsersByUsernames.returns(cursorOf([{ _id: 'me' }])); + + expect(await findDirectRoomByIdentifier('ghost', me)).to.be.null; + expect(RoomsStub.findOneDirectRoomContainingAllUserIDs.called).to.be.false; + }); + + it('should return null when no direct room contains that member set', async () => { + UsersStub.findUsersByUsernames.returns(cursorOf([{ _id: 'me' }, { _id: 'alice' }])); + RoomsStub.findOneDirectRoomContainingAllUserIDs.resolves(null); + + expect(await findDirectRoomByIdentifier('alice', me)).to.be.null; + }); + + it('should return null when the caller has no username', async () => { + expect(await findDirectRoomByIdentifier('alice', { _id: 'me' })).to.be.null; + }); +}); diff --git a/packages/rest-typings/src/v1/rooms.ts b/packages/rest-typings/src/v1/rooms.ts index b91a847898140..103ce21f0a88a 100644 --- a/packages/rest-typings/src/v1/rooms.ts +++ b/packages/rest-typings/src/v1/rooms.ts @@ -10,6 +10,7 @@ import type { ISubscription, RequiredField, MessageTypesValues, + RoomType, } from '@rocket.chat/core-typings'; import { ajv, ajvQuery } from './Ajv'; @@ -915,6 +916,10 @@ export type RoomsEndpoints = { }; }; + '/v1/rooms.getOrCreate': { + POST: (params: { type: RoomType; name: string }) => { room: IRoom }; + }; + '/v1/rooms.info': { GET: (params: RoomsInfoProps) => { room: IRoom | undefined; From 39897b3484d7f57ec7d97e134576d69aa4f82001 Mon Sep 17 00:00:00 2001 From: Ricardo Garim Date: Tue, 18 Aug 2026 11:50:46 -0300 Subject: [PATCH 2/3] test: expect 400 when the direct message does not exist --- apps/meteor/tests/end-to-end/api/methods.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/meteor/tests/end-to-end/api/methods.ts b/apps/meteor/tests/end-to-end/api/methods.ts index ccffdd982473c..0cf1d56042cbd 100644 --- a/apps/meteor/tests/end-to-end/api/methods.ts +++ b/apps/meteor/tests/end-to-end/api/methods.ts @@ -2740,7 +2740,7 @@ describe('Meteor.methods', () => { msg: 'method', }), }) - .expect(200); + .expect(400); const parsedResponse = JSON.parse(res.body.message); expect(parsedResponse).to.have.property('error'); From fe9ba7728783cb9c71f74c47785d80af47a73e5e Mon Sep 17 00:00:00 2001 From: Ricardo Garim Date: Tue, 18 Aug 2026 23:55:51 -0300 Subject: [PATCH 3/3] chore: remove comments and improve dm detection --- apps/meteor/server/api/v1/rooms.ts | 13 ++++---- .../lib/rooms/findDirectRoomByIdentifier.ts | 24 +++++++++----- apps/meteor/tests/end-to-end/api/rooms.ts | 31 ++++++++++++++++- .../rooms/findDirectRoomByIdentifier.spec.ts | 33 +++++++++++++++++-- 4 files changed, 83 insertions(+), 18 deletions(-) diff --git a/apps/meteor/server/api/v1/rooms.ts b/apps/meteor/server/api/v1/rooms.ts index 5b9dc60013a3b..0c3974f5a14b8 100644 --- a/apps/meteor/server/api/v1/rooms.ts +++ b/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,10 +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', properties: { @@ -564,7 +560,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) { diff --git a/apps/meteor/server/lib/rooms/findDirectRoomByIdentifier.ts b/apps/meteor/server/lib/rooms/findDirectRoomByIdentifier.ts index c09f0f86b0ff6..6c479ec7f5981 100644 --- a/apps/meteor/server/lib/rooms/findDirectRoomByIdentifier.ts +++ b/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[] | null> => { + const users = await Users.findUsersByUsernames>(usernames, { + projection: { _id: 1, username: 1 }, + }).toArray(); + + return users.length === usernames.length ? users : null; +}; + +export const resolveDirectRoomTargets = async (identifier: string): Promise => { + const targets = [...new Set(parseDirectRoomTargets(identifier))]; + + return (await resolveUsernames(targets)) ? targets : null; +}; + export const findDirectRoomByIdentifier = async (identifier: string, user: Pick): Promise => { 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; } diff --git a/apps/meteor/tests/end-to-end/api/rooms.ts b/apps/meteor/tests/end-to-end/api/rooms.ts index dd73cd60d0e3d..cda514c966149 100644 --- a/apps/meteor/tests/end-to-end/api/rooms.ts +++ b/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')) diff --git a/apps/meteor/tests/unit/server/lib/rooms/findDirectRoomByIdentifier.spec.ts b/apps/meteor/tests/unit/server/lib/rooms/findDirectRoomByIdentifier.spec.ts index 11132693c7bb5..8057a41059e86 100644 --- a/apps/meteor/tests/unit/server/lib/rooms/findDirectRoomByIdentifier.spec.ts +++ b/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']); + }); +});