From a76a7d540c1284bfd1d3f9ac3c6413351024bf0a Mon Sep 17 00:00:00 2001 From: Pierre Lehnen Date: Tue, 19 May 2020 14:58:06 -0300 Subject: [PATCH 1/7] [FIX] chat.postMessage API not validating if the room is readonly --- .../server/functions/canSendMessage.js | 13 ++++++++----- app/authorization/server/index.js | 3 ++- app/lib/server/functions/processWebhookMessage.js | 3 +++ 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/app/authorization/server/functions/canSendMessage.js b/app/authorization/server/functions/canSendMessage.js index 10f1a33c72810..18ecfb777381d 100644 --- a/app/authorization/server/functions/canSendMessage.js +++ b/app/authorization/server/functions/canSendMessage.js @@ -10,21 +10,19 @@ const subscriptionOptions = { }, }; -export const canSendMessageAsync = async (rid, { uid, username, type }, extraData) => { - const room = await Rooms.findOneById(rid); - +export const validateRoomMessagePermissionsAsync = async (room, { uid, username, type }, extraData) => { if (type !== 'app' && !await canAccessRoomAsync(room, { _id: uid, username }, extraData)) { throw new Error('error-not-allowed'); } if (roomTypes.getConfig(room.t).allowMemberAction(room, RoomMemberActions.BLOCK)) { - const subscription = await Subscriptions.findOneByRoomIdAndUserId(rid, uid, subscriptionOptions); + const subscription = await Subscriptions.findOneByRoomIdAndUserId(room._id, uid, subscriptionOptions); if (subscription && (subscription.blocked || subscription.blocker)) { throw new Error('room_is_blocked'); } } - if (room.ro === true && !await hasPermissionAsync(uid, 'post-readonly', rid)) { + if (room.ro === true && !await hasPermissionAsync(uid, 'post-readonly', room._id)) { // Unless the user was manually unmuted if (!(room.unmuted || []).includes(username)) { throw new Error('You can\'t send messages because the room is readonly.'); @@ -34,8 +32,13 @@ export const canSendMessageAsync = async (rid, { uid, username, type }, extraDat if ((room.muted || []).includes(username)) { throw new Error('You_have_been_muted'); } +}; +export const canSendMessageAsync = async (rid, { uid, username, type }, extraData) => { + const room = await Rooms.findOneById(rid); + await validateRoomMessagePermissionsAsync(room, { uid, username, type }, extraData); return room; }; export const canSendMessage = (rid, { uid, username, type }, extraData) => Promise.await(canSendMessageAsync(rid, { uid, username, type }, extraData)); +export const validateRoomMessagePermissions = (room, { uid, username, type }, extraData) => Promise.await(validateRoomMessagePermissionsAsync(room, { uid, username, type }, extraData)); diff --git a/app/authorization/server/index.js b/app/authorization/server/index.js index 0ebc74f0ce772..d16360af03aee 100644 --- a/app/authorization/server/index.js +++ b/app/authorization/server/index.js @@ -4,7 +4,7 @@ import { canAccessRoom, roomAccessValidators, } from './functions/canAccessRoom'; -import { canSendMessage } from './functions/canSendMessage'; +import { canSendMessage, validateRoomMessagePermissions } from './functions/canSendMessage'; import { getRoles } from './functions/getRoles'; import { getUsersInRole } from './functions/getUsersInRole'; import { @@ -30,6 +30,7 @@ export { hasRole, removeUserFromRoles, canSendMessage, + validateRoomMessagePermissions, addRoomAccessValidator, roomAccessValidators, addUserRoles, diff --git a/app/lib/server/functions/processWebhookMessage.js b/app/lib/server/functions/processWebhookMessage.js index a667203b47a5d..f45076476ddca 100644 --- a/app/lib/server/functions/processWebhookMessage.js +++ b/app/lib/server/functions/processWebhookMessage.js @@ -4,6 +4,7 @@ import s from 'underscore.string'; import { getRoomByNameOrIdWithOptionToJoin } from './getRoomByNameOrIdWithOptionToJoin'; import { sendMessage } from './sendMessage'; +import { validateRoomMessagePermissions } from '../../../authorization/server/functions/canSendMessage'; import { Subscriptions } from '../../../models'; import { getDirectMessageByIdWithOptionToJoin, getDirectMessageByNameOrIdWithOptionToJoin } from './getDirectMessageByNameOrIdWithOptionToJoin'; @@ -82,6 +83,8 @@ export const processWebhookMessage = function(messageObj, user, defaultValues = } } + validateRoomMessagePermissions(room, { uid: user._id, ...user }); + const messageReturn = sendMessage(user, message, room); sentData.push({ channel, message: messageReturn }); } From b79486fd74ef4a62b00c6a1e2df5e42ee72bc12b Mon Sep 17 00:00:00 2001 From: Pierre Lehnen Date: Tue, 19 May 2020 14:58:48 -0300 Subject: [PATCH 2/7] [FIX] chat.update API not validating if the room is readonly --- app/lib/server/methods/updateMessage.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/lib/server/methods/updateMessage.js b/app/lib/server/methods/updateMessage.js index 3b90cc484f7b1..49d7720ec73c9 100644 --- a/app/lib/server/methods/updateMessage.js +++ b/app/lib/server/methods/updateMessage.js @@ -4,7 +4,7 @@ import moment from 'moment'; import { Messages } from '../../../models'; import { settings } from '../../../settings'; -import { hasPermission } from '../../../authorization'; +import { hasPermission, canSendMessage } from '../../../authorization/server'; import { updateMessage } from '../functions'; Meteor.methods({ @@ -47,6 +47,9 @@ Meteor.methods({ } } + const user = Meteor.users.findOne(Meteor.userId()); + canSendMessage(message.rid, { uid: user._id, ...user }); + // It is possible to have an empty array as the attachments property, so ensure both things exist if (originalMessage.attachments && originalMessage.attachments.length > 0 && originalMessage.attachments[0].description !== undefined) { message.attachments = originalMessage.attachments; From fca4b84b91fa1582788a08e38c69b81c66a2903a Mon Sep 17 00:00:00 2001 From: Pierre Lehnen Date: Tue, 19 May 2020 17:41:57 -0300 Subject: [PATCH 3/7] [FIX] chat.sendMessage API throwing random exception when room is readonly --- app/api/server/v1/chat.js | 4 ++-- app/lib/server/methods/sendMessage.js | 23 +++++++++++++++++------ 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/app/api/server/v1/chat.js b/app/api/server/v1/chat.js index 69685a3f6f978..2fd4225b58196 100644 --- a/app/api/server/v1/chat.js +++ b/app/api/server/v1/chat.js @@ -5,6 +5,7 @@ import { Messages } from '../../../models'; import { canAccessRoom, hasPermission } from '../../../authorization'; import { normalizeMessagesForUser } from '../../../utils/server/lib/normalizeMessagesForUser'; import { processWebhookMessage } from '../../../lib/server'; +import { executeSendMessage } from '../../../lib/server/methods/sendMessage'; import { API } from '../api'; import Rooms from '../../../models/server/models/Rooms'; import Users from '../../../models/server/models/Users'; @@ -172,8 +173,7 @@ API.v1.addRoute('chat.sendMessage', { authRequired: true }, { throw new Meteor.Error('error-invalid-params', 'The "message" parameter must be provided.'); } - const sent = Meteor.runAsUser(this.userId, () => Meteor.call('sendMessage', this.bodyParams.message)); - + const sent = executeSendMessage(this.userId, this.bodyParams.message); const [message] = normalizeMessagesForUser([sent], this.userId); return API.v1.success({ diff --git a/app/lib/server/methods/sendMessage.js b/app/lib/server/methods/sendMessage.js index be4ef0670d9cc..ae59502e151b8 100644 --- a/app/lib/server/methods/sendMessage.js +++ b/app/lib/server/methods/sendMessage.js @@ -72,18 +72,21 @@ export function executeSendMessage(uid, message) { metrics.messagesSent.inc(); // TODO This line needs to be moved to it's proper place. See the comments on: https://github.com/RocketChat/Rocket.Chat/pull/5736 return sendMessage(user, message, room, false); } catch (error) { - if (error === 'error-not-allowed') { - throw new Meteor.Error('error-not-allowed'); - } - SystemLogger.error('Error sending message:', error); + const errorMessage = typeof error === 'string' ? error : error.error || error.message; Notifications.notifyUser(uid, 'message', { _id: Random.id(), rid: message.rid, ts: new Date(), - msg: TAPi18n.__(error, {}, user.language), + msg: TAPi18n.__(errorMessage, {}, user.language), }); + + if (typeof error === 'string') { + throw new Error(error); + } + + throw error; } } @@ -98,7 +101,15 @@ Meteor.methods({ }); } - return executeSendMessage(uid, message); + try { + return executeSendMessage(uid, message); + } catch (error) { + if ((error.error || error.message) === 'error-not-allowed') { + throw new Meteor.Error(error.error || error.message, error.reason, { + method: 'sendMessage', + }); + } + } }, }); // Limit a user, who does not have the "bot" role, to sending 5 msgs/second From 6cc7a3925eb81bab438ae7dac315725421c16dd8 Mon Sep 17 00:00:00 2001 From: Pierre Lehnen Date: Wed, 20 May 2020 14:03:22 -0300 Subject: [PATCH 4/7] [FIX] Chat.deleteMessage API not validating if the room is readonly --- .../server/functions/canDeleteMessage.js | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/app/authorization/server/functions/canDeleteMessage.js b/app/authorization/server/functions/canDeleteMessage.js index aeb1e06ef1268..d35297dfa3785 100644 --- a/app/authorization/server/functions/canDeleteMessage.js +++ b/app/authorization/server/functions/canDeleteMessage.js @@ -1,5 +1,6 @@ import { hasPermissionAsync } from './hasPermission'; import { getValue } from '../../../settings/server/raw'; +import { Rooms } from '../../../models'; const elapsedTime = (ts) => { const dif = Date.now() - ts; @@ -30,12 +31,21 @@ export const canDeleteMessageAsync = async (uid, { u, rid, ts }) => { } const blockDeleteInMinutes = await getValue('Message_AllowDeleting_BlockDeleteInMinutes'); - if (!blockDeleteInMinutes) { - return true; + if (blockDeleteInMinutes) { + const timeElapsedForMessage = elapsedTime(ts); + return timeElapsedForMessage <= blockDeleteInMinutes; + } + + const room = await Rooms.findOneById(rid, { fields: { ro: 1, unmuted: 1 } }); + console.log(room, u.username); + if (room.ro === true && !await hasPermissionAsync(uid, 'post-readonly', rid)) { + // Unless the user was manually unmuted + if (!(room.unmuted || []).includes(u.username)) { + throw new Error('You can\'t delete messages because the room is readonly.'); + } } - const timeElapsedForMessage = elapsedTime(ts); - return timeElapsedForMessage <= blockDeleteInMinutes; + return true; }; export const canDeleteMessage = (uid, { u, rid, ts }) => Promise.await(canDeleteMessageAsync(uid, { u, rid, ts })); From b8bcaa06119b87cb1a46cd7440bcfe9aabe109b4 Mon Sep 17 00:00:00 2001 From: Pierre Lehnen Date: Wed, 20 May 2020 23:54:22 -0300 Subject: [PATCH 5/7] [FIX] chat.react API returnig positive result even when an error occurs --- app/api/server/v1/chat.js | 3 +- app/reactions/server/setReaction.js | 55 ++++++++++++++++++----------- 2 files changed, 37 insertions(+), 21 deletions(-) diff --git a/app/api/server/v1/chat.js b/app/api/server/v1/chat.js index 2fd4225b58196..666725f3bb322 100644 --- a/app/api/server/v1/chat.js +++ b/app/api/server/v1/chat.js @@ -6,6 +6,7 @@ import { canAccessRoom, hasPermission } from '../../../authorization'; import { normalizeMessagesForUser } from '../../../utils/server/lib/normalizeMessagesForUser'; import { processWebhookMessage } from '../../../lib/server'; import { executeSendMessage } from '../../../lib/server/methods/sendMessage'; +import { executeSetReaction } from '../../../reactions/server/setReaction'; import { API } from '../api'; import Rooms from '../../../models/server/models/Rooms'; import Users from '../../../models/server/models/Users'; @@ -294,7 +295,7 @@ API.v1.addRoute('chat.react', { authRequired: true }, { throw new Meteor.Error('error-emoji-param-not-provided', 'The required "emoji" param is missing.'); } - Meteor.runAsUser(this.userId, () => Meteor.call('setReaction', emoji, msg._id, this.bodyParams.shouldReact)); + Meteor.runAsUser(this.userId, () => Promise.await(executeSetReaction(emoji, msg._id, this.bodyParams.shouldReact))); return API.v1.success(); }, diff --git a/app/reactions/server/setReaction.js b/app/reactions/server/setReaction.js index 52adec6d8c290..6729baf00ceef 100644 --- a/app/reactions/server/setReaction.js +++ b/app/reactions/server/setReaction.js @@ -33,13 +33,9 @@ async function setReaction(room, user, message, reaction, shouldReact) { } if (Array.isArray(room.muted) && room.muted.indexOf(user.username) !== -1) { - Notifications.notifyUser(Meteor.userId(), 'message', { - _id: Random.id(), + throw new Meteor.Error('error-not-allowed', TAPi18n.__('You_have_been_muted', {}, user.language), { rid: room._id, - ts: new Date(), - msg: TAPi18n.__('You_have_been_muted', {}, user.language), }); - return false; } const userAlreadyReacted = Boolean(message.reactions) && Boolean(message.reactions[reaction]) && message.reactions[reaction].usernames.indexOf(user.username) !== -1; @@ -88,26 +84,45 @@ async function setReaction(room, user, message, reaction, shouldReact) { msgStream.emit(message.rid, message); } -Meteor.methods({ - setReaction(reaction, messageId, shouldReact) { - const user = Meteor.user(); +export const executeSetReaction = async function(reaction, messageId, shouldReact) { + const user = Meteor.user(); - if (!user) { - throw new Meteor.Error('error-invalid-user', 'Invalid user', { method: 'setReaction' }); - } + if (!user) { + throw new Meteor.Error('error-invalid-user', 'Invalid user', { method: 'setReaction' }); + } - const message = Messages.findOneById(messageId); + const message = Messages.findOneById(messageId); - if (!message) { - throw new Meteor.Error('error-not-allowed', 'Not allowed', { method: 'setReaction' }); - } + if (!message) { + throw new Meteor.Error('error-not-allowed', 'Not allowed', { method: 'setReaction' }); + } - const room = Meteor.call('canAccessRoom', message.rid, Meteor.userId()); + const room = Meteor.call('canAccessRoom', message.rid, Meteor.userId()); - if (!room) { - throw new Meteor.Error('error-not-allowed', 'Not allowed', { method: 'setReaction' }); - } + if (!room) { + throw new Meteor.Error('error-not-allowed', 'Not allowed', { method: 'setReaction' }); + } - setReaction(room, user, message, reaction, shouldReact); + return setReaction(room, user, message, reaction, shouldReact); +}; + +Meteor.methods({ + setReaction(reaction, messageId, shouldReact) { + try { + return Promise.await(executeSetReaction(reaction, messageId, shouldReact)); + } catch (e) { + if (e.error === 'error-not-allowed' && e.reason && e.details && e.details.rid) { + Notifications.notifyUser(Meteor.userId(), 'message', { + _id: Random.id(), + rid: e.details.rid, + ts: new Date(), + msg: e.reason, + }); + + return false; + } + + throw e; + } }, }); From ab50878a1f2a6ddb9fdc85664faa37bee24c58d8 Mon Sep 17 00:00:00 2001 From: Pierre Lehnen Date: Thu, 21 May 2020 19:57:21 -0300 Subject: [PATCH 6/7] Removed left over console.log call --- app/authorization/server/functions/canDeleteMessage.js | 1 - 1 file changed, 1 deletion(-) diff --git a/app/authorization/server/functions/canDeleteMessage.js b/app/authorization/server/functions/canDeleteMessage.js index d35297dfa3785..3f3d436e0786b 100644 --- a/app/authorization/server/functions/canDeleteMessage.js +++ b/app/authorization/server/functions/canDeleteMessage.js @@ -37,7 +37,6 @@ export const canDeleteMessageAsync = async (uid, { u, rid, ts }) => { } const room = await Rooms.findOneById(rid, { fields: { ro: 1, unmuted: 1 } }); - console.log(room, u.username); if (room.ro === true && !await hasPermissionAsync(uid, 'post-readonly', rid)) { // Unless the user was manually unmuted if (!(room.unmuted || []).includes(u.username)) { From b97b60a47d3c1960843f28336ea2b0bdd0721dda Mon Sep 17 00:00:00 2001 From: Pierre Lehnen Date: Mon, 25 May 2020 16:38:18 -0300 Subject: [PATCH 7/7] Added new end-to-end tests for readonly rooms --- tests/end-to-end/api/05-chat.js | 79 +++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/tests/end-to-end/api/05-chat.js b/tests/end-to-end/api/05-chat.js index 41c3896cb163e..5091071b6f645 100644 --- a/tests/end-to-end/api/05-chat.js +++ b/tests/end-to-end/api/05-chat.js @@ -702,6 +702,31 @@ describe('[Chat]', function() { describe('Read only channel', () => { let readOnlyChannel; + const userCredentials = {}; + let user; + before((done) => { + const username = `user.test.readonly.${ Date.now() }`; + const email = `${ username }@rocket.chat`; + request.post(api('users.create')) + .set(credentials) + .send({ email, name: username, username, password }) + .end((err, res) => { + user = res.body.user; + request.post(api('login')) + .send({ + user: username, + password, + }) + .expect('Content-Type', 'application/json') + .expect(200) + .expect((res) => { + userCredentials['X-Auth-Token'] = res.body.data.authToken; + userCredentials['X-User-Id'] = res.body.data.userId; + }) + .end(done); + }); + }); + it('Creating a read-only channel', (done) => { request.post(api('channels.create')) .set(credentials) @@ -734,6 +759,60 @@ describe('[Chat]', function() { }) .end(done); }); + it('Inviting regular user to read-only channel', (done) => { + request.post(api('channels.invite')) + .set(credentials) + .send({ + roomId: readOnlyChannel._id, + userId: user._id, + }) + .expect('Content-Type', 'application/json') + .expect(200) + .expect((res) => { + expect(res.body).to.have.property('success', true); + }) + .end(() => { + done(); + }); + }); + + it('should fail to send message when the user lacks permission', (done) => { + request.post(api('chat.sendMessage')) + .set(userCredentials) + .send({ + message: { + rid: readOnlyChannel._id, + msg: 'Sample blocked message', + }, + }) + .expect('Content-Type', 'application/json') + .expect(400) + .expect((res) => { + expect(res.body).to.have.property('success', false); + expect(res.body).to.have.property('error'); + }) + .end(done); + }); + + it('should send a message when the user has permission to send messages on readonly channels', (done) => { + updatePermission('post-readonly', ['user']).then(() => { + request.post(api('chat.sendMessage')) + .set(userCredentials) + .send({ + message: { + rid: readOnlyChannel._id, + msg: 'Sample message overwriting readonly status', + }, + }) + .expect('Content-Type', 'application/json') + .expect(200) + .expect((res) => { + expect(res.body).to.have.property('success', true); + expect(res.body).to.have.property('message').and.to.be.an('object'); + }) + .end(done); + }); + }); }); });