From aea2d8bbeb5437710a15d0acf9f0e18ee6d11297 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Wed, 5 Dec 2018 21:27:49 +0100 Subject: [PATCH] FIX: properly secure poll message bus Co-authored-by: Sam --- app/models/post.rb | 29 ++++++---- plugins/poll/lib/polls_updater.rb | 2 +- plugins/poll/plugin.rb | 6 +- .../spec/controllers/polls_controller_spec.rb | 57 ++++++++++++++++++- 4 files changed, 76 insertions(+), 18 deletions(-) diff --git a/app/models/post.rb b/app/models/post.rb index ac1a9659c8a..6ea6765c074 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -152,13 +152,12 @@ class Post < ActiveRecord::Base end end - def publish_change_to_clients!(type, options = {}) - # special failsafe for posts missing topics consistency checks should fix, but message - # is safe to skip + def publish_change_to_clients!(type, opts = {}) + # special failsafe for posts missing topics consistency checks should fix, + # but message is safe to skip return unless topic - channel = "/topic/#{topic_id}" - msg = { + message = { id: id, post_number: post_number, updated_at: Time.now, @@ -166,20 +165,26 @@ class Post < ActiveRecord::Base last_editor_id: last_editor_id, type: type, version: version - }.merge(options) + }.merge(opts) + + publish_message!("/topic/#{topic_id}", message) + end + + def publish_message!(channel, message, opts = {}) + return unless topic if Topic.visible_post_types.include?(post_type) if topic.private_message? - user_ids = User.where('admin or moderator').pluck(:id) - user_ids |= topic.allowed_users.pluck(:id) - MessageBus.publish(channel, msg, user_ids: user_ids) + opts[:user_ids] = User.where("admin OR moderator").pluck(:id) + opts[:user_ids] |= topic.allowed_users.pluck(:id) else - MessageBus.publish(channel, msg, group_ids: topic.secure_group_ids) + opts[:group_ids] = topic.secure_group_ids end else - user_ids = User.where('admin or moderator or id = ?', user_id).pluck(:id) - MessageBus.publish(channel, msg, user_ids: user_ids) + opts[:user_ids] = User.where("admin OR moderator OR id = ?", user_id).pluck(:id) end + + MessageBus.publish(channel, message, opts) end def trash!(trashed_by = nil) diff --git a/plugins/poll/lib/polls_updater.rb b/plugins/poll/lib/polls_updater.rb index e697728072b..d90c6625f58 100644 --- a/plugins/poll/lib/polls_updater.rb +++ b/plugins/poll/lib/polls_updater.rb @@ -91,7 +91,7 @@ module DiscoursePoll if has_changed polls = ::Poll.includes(poll_options: :poll_votes).where(post: post) polls = ActiveModel::ArraySerializer.new(polls, each_serializer: PollSerializer, root: false).as_json - MessageBus.publish("/polls/#{post.topic_id}", post_id: post.id, polls: polls) + post.publish_message!("/polls/#{post.topic_id}", post_id: post.id, polls: polls) end end end diff --git a/plugins/poll/plugin.rb b/plugins/poll/plugin.rb index cb2ef67216b..cbf7ab43582 100644 --- a/plugins/poll/plugin.rb +++ b/plugins/poll/plugin.rb @@ -102,7 +102,7 @@ after_initialize do serialized_poll = PollSerializer.new(poll, root: false).as_json payload = { post_id: post_id, polls: [serialized_poll] } - MessageBus.publish("/polls/#{post.topic_id}", payload) + post.publish_message!("/polls/#{post.topic_id}", payload) [serialized_poll, options] end @@ -137,7 +137,7 @@ after_initialize do serialized_poll = PollSerializer.new(poll, root: false).as_json payload = { post_id: post_id, polls: [serialized_poll] } - MessageBus.publish("/polls/#{post.topic_id}", payload) + post.publish_message!("/polls/#{post.topic_id}", payload) serialized_poll end @@ -425,7 +425,7 @@ after_initialize do unless post.is_first_post? polls = ActiveModel::ArraySerializer.new(post.polls, each_serializer: PollSerializer, root: false).as_json - MessageBus.publish("/polls/#{post.topic_id}", post_id: post.id, polls: polls) + post.publish_message!("/polls/#{post.topic_id}", post_id: post.id, polls: polls) end end diff --git a/plugins/poll/spec/controllers/polls_controller_spec.rb b/plugins/poll/spec/controllers/polls_controller_spec.rb index d03d5616da2..fb4fe5e3458 100644 --- a/plugins/poll/spec/controllers/polls_controller_spec.rb +++ b/plugins/poll/spec/controllers/polls_controller_spec.rb @@ -17,16 +17,69 @@ describe ::DiscoursePoll::PollsController do put :vote, params: { post_id: poll.id, poll_name: "poll", options: ["5c24fc1df56d764b550ceae1b9319125"] }, format: :json - - expect(response.status).to eq(200) end.first + expect(response.status).to eq(200) + json = ::JSON.parse(response.body) expect(json["poll"]["name"]).to eq("poll") expect(json["poll"]["voters"]).to eq(1) expect(json["vote"]).to eq(["5c24fc1df56d764b550ceae1b9319125"]) expect(message.channel).to eq("/polls/#{poll.topic_id}") + expect(message.user_ids).to eq(nil) + expect(message.group_ids).to eq(nil) + end + + it "works in PM" do + user2 = Fabricate(:user) + topic = Fabricate(:private_message_topic, topic_allowed_users: [ + Fabricate.build(:topic_allowed_user, user: user), + Fabricate.build(:topic_allowed_user, user: user2) + ]) + poll = Fabricate(:post, topic: topic, user: user, raw: "[poll]\n- A\n- B\n[/poll]") + + message = MessageBus.track_publish do + put :vote, params: { + post_id: poll.id, poll_name: "poll", options: ["5c24fc1df56d764b550ceae1b9319125"] + }, format: :json + end.first + + expect(response.status).to eq(200) + + json = ::JSON.parse(response.body) + expect(json["poll"]["name"]).to eq("poll") + expect(json["poll"]["voters"]).to eq(1) + expect(json["vote"]).to eq(["5c24fc1df56d764b550ceae1b9319125"]) + + expect(message.channel).to eq("/polls/#{poll.topic_id}") + expect(message.user_ids).to contain_exactly(-2, -1, user.id, user2.id) + expect(message.group_ids).to eq(nil) + end + + it "works in secure categories" do + group = Fabricate(:group) + group.add_owner(user) + category = Fabricate(:private_category, group: group) + topic = Fabricate(:topic, category: category) + poll = Fabricate(:post, topic: topic, user: user, raw: "[poll]\n- A\n- B\n[/poll]") + + message = MessageBus.track_publish do + put :vote, params: { + post_id: poll.id, poll_name: "poll", options: ["5c24fc1df56d764b550ceae1b9319125"] + }, format: :json + end.first + + expect(response.status).to eq(200) + + json = ::JSON.parse(response.body) + expect(json["poll"]["name"]).to eq("poll") + expect(json["poll"]["voters"]).to eq(1) + expect(json["vote"]).to eq(["5c24fc1df56d764b550ceae1b9319125"]) + + expect(message.channel).to eq("/polls/#{poll.topic_id}") + expect(message.user_ids).to eq(nil) + expect(message.group_ids).to contain_exactly(group.id) end it "requires at least 1 valid option" do