From 5ff505cea6f224d10ca0559cc0ccba5ed8b196d7 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Mon, 23 Mar 2020 11:02:24 +0000 Subject: [PATCH] SECURITY: Respect topic permissions when loading draft metadata Co-authored-by: Sam Saffron --- app/controllers/drafts_controller.rb | 11 --- app/models/draft.rb | 103 +++++++++++++++++------- app/models/post.rb | 10 +-- app/models/topic.rb | 14 ++++ app/serializers/draft_serializer.rb | 50 +++++++++--- spec/models/draft_spec.rb | 63 +++++++++------ spec/requests/drafts_controller_spec.rb | 20 +++++ 7 files changed, 188 insertions(+), 83 deletions(-) diff --git a/app/controllers/drafts_controller.rb b/app/controllers/drafts_controller.rb index 6f70e769b9b..db6f87782ab 100644 --- a/app/controllers/drafts_controller.rb +++ b/app/controllers/drafts_controller.rb @@ -23,17 +23,6 @@ class DraftsController < ApplicationController } stream = Draft.stream(opts) - stream.each do |d| - parsed_data = JSON.parse(d.data) - if parsed_data - if parsed_data['reply'] - d.raw = parsed_data['reply'] - end - if parsed_data['categoryId'].present? && !d.category_id.present? - d.category_id = parsed_data['categoryId'] - end - end - end render json: { drafts: stream ? serialize_data(stream, DraftSerializer) : [], diff --git a/app/models/draft.rb b/app/models/draft.rb index 8d1b5ca3645..9d4c08e56f1 100644 --- a/app/models/draft.rb +++ b/app/models/draft.rb @@ -5,6 +5,8 @@ class Draft < ActiveRecord::Base NEW_PRIVATE_MESSAGE ||= 'new_private_message' EXISTING_TOPIC ||= 'topic_' + belongs_to :user + class OutOfSequence < StandardError; end def self.set(user, key, sequence, data, owner = nil) @@ -137,6 +139,72 @@ class Draft < ActiveRecord::Base Draft.where(user_id: user.id, draft_key: key).destroy_all end + def display_user + post&.user || topic&.user || user + end + + def parsed_data + JSON.parse(data) + end + + def topic_id + if draft_key.starts_with?(EXISTING_TOPIC) + draft_key.gsub(EXISTING_TOPIC, "").to_i + end + end + + def topic_preloaded? + !!defined?(@topic) + end + + def topic + topic_preloaded? ? @topic : @topic = Draft.allowed_draft_topics_for_user(user).find_by(id: topic_id) + end + + def preload_topic(topic) + @topic = topic + end + + def post_id + parsed_data["postId"] + end + + def post_preloaded? + !!defined?(@post) + end + + def post + post_preloaded? ? @post : @post = Draft.allowed_draft_posts_for_user(user).find_by(id: post_id) + end + + def preload_post(post) + @post = post + end + + def self.preload_data(drafts, user) + topic_ids = drafts.map(&:topic_id) + post_ids = drafts.map(&:post_id) + + topics = self.allowed_draft_topics_for_user(user).where(id: topic_ids) + posts = self.allowed_draft_posts_for_user(user).where(id: post_ids) + + drafts.each do |draft| + draft.preload_topic(topics.detect { |t| t.id == draft.topic_id }) + draft.preload_post(posts.detect { |p| p.id == draft.post_id }) + end + end + + def self.allowed_draft_topics_for_user(user) + topics = Topic.listable_topics.secured(Guardian.new(user)) + pms = Topic.private_messages_for_user(user) + topics.or(pms) + end + + def self.allowed_draft_posts_for_user(user) + # .secured handles whispers, merge handles topic/pm visibility + Post.secured(Guardian.new(user)).joins(:topic).merge(self.allowed_draft_topics_for_user(user)) + end + def self.stream(opts = nil) opts ||= {} @@ -144,36 +212,15 @@ class Draft < ActiveRecord::Base offset = (opts[:offset] || 0).to_i limit = (opts[:limit] || 30).to_i - # JOIN of topics table based on manipulating draft_key seems imperfect - builder = DB.build <<~SQL - SELECT - d.*, t.title, t.id topic_id, t.archetype, - t.category_id, t.closed topic_closed, t.archived topic_archived, - pu.username, pu.name, pu.id user_id, pu.uploaded_avatar_id, pu.username_lower, - du.username draft_username, NULL as raw, NULL as cooked, NULL as post_number - FROM drafts d - LEFT JOIN LATERAL json_extract_path_text (d.data::json, 'postId') postId ON TRUE - LEFT JOIN posts p ON postId :: BIGINT = p.id - LEFT JOIN topics t ON - CASE - WHEN d.draft_key LIKE '%' || '#{EXISTING_TOPIC}' || '%' - THEN CAST(replace(d.draft_key, '#{EXISTING_TOPIC}', '') AS INT) - ELSE 0 - END = t.id - JOIN users pu on pu.id = COALESCE(p.user_id, t.user_id, d.user_id) - JOIN users du on du.id = #{user_id} - /*where*/ - /*order_by*/ - /*offset*/ - /*limit*/ - SQL - - builder - .where('d.user_id = :user_id', user_id: user_id.to_i) - .order_by('d.updated_at desc') + stream = Draft.where(user_id: user_id) + .order(updated_at: :desc) .offset(offset) .limit(limit) - .query + + # Preload posts and topics to avoid N+1 queries + Draft.preload_data(stream, opts[:user]) + + stream end def self.cleanup! diff --git a/app/models/post.rb b/app/models/post.rb index 85432556e7f..b57a9502c6b 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -69,15 +69,7 @@ class Post < ActiveRecord::Base register_custom_field_type(MISSING_UPLOADS_IGNORED, :boolean) scope :private_posts_for_user, ->(user) { - where("posts.topic_id IN (SELECT topic_id - FROM topic_allowed_users - WHERE user_id = :user_id - UNION ALL - SELECT tg.topic_id - FROM topic_allowed_groups tg - JOIN group_users gu ON gu.user_id = :user_id AND - gu.group_id = tg.group_id)", - user_id: user.id) + where("posts.topic_id IN (#{Topic::PRIVATE_MESSAGES_SQL})", user_id: user.id) } scope :by_newest, -> { order('created_at DESC, id DESC') } diff --git a/app/models/topic.rb b/app/models/topic.rb index 07b5c60c6e3..a50c35dcb88 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -152,6 +152,20 @@ class Topic < ActiveRecord::Base # Return private message topics scope :private_messages, -> { where(archetype: Archetype.private_message) } + PRIVATE_MESSAGES_SQL = <<~SQL + SELECT topic_id + FROM topic_allowed_users + WHERE user_id = :user_id + UNION ALL + SELECT tg.topic_id + FROM topic_allowed_groups tg + JOIN group_users gu ON gu.user_id = :user_id AND gu.group_id = tg.group_id + SQL + + scope :private_messages_for_user, ->(user) { + private_messages.where("topics.id IN (#{PRIVATE_MESSAGES_SQL})", user_id: user.id) + } + scope :listable_topics, -> { where('topics.archetype <> ?', Archetype.private_message) } scope :by_newest, -> { order('topics.created_at desc, topics.id desc') } diff --git a/app/serializers/draft_serializer.rb b/app/serializers/draft_serializer.rb index 1d19f0d18b6..bcdfa257fcf 100644 --- a/app/serializers/draft_serializer.rb +++ b/app/serializers/draft_serializer.rb @@ -23,36 +23,68 @@ class DraftSerializer < ApplicationSerializer :archetype, :archived + def cooked + object.parsed_data['reply'] || "" + end + + def draft_username + object.user.username + end + def avatar_template - User.avatar_template(object.username, object.uploaded_avatar_id) + object.user.avatar_template + end + + def username + object.display_user&.username + end + + def username_lower + object.display_user&.username_lower + end + + def name + object.display_user&.name + end + + def title + object.topic&.title end def slug - Slug.for(object.title) + object.topic&.slug end - def include_slug? - object.title.present? + def category_id + object.topic&.category_id end def closed - object.topic_closed + object.topic&.closed end def archived - object.topic_archived + object.topic&.archived + end + + def archetype + object&.topic&.archetype + end + + def include_slug? + object.topic&.title&.present? end def include_closed? - object.topic_closed.present? + object.topic&.closed&.present? end def include_archived? - object.topic_archived.present? + object.topic&.archived&.present? end def include_category_id? - object.category_id.present? + object.topic&.category_id&.present? end end diff --git a/spec/models/draft_spec.rb b/spec/models/draft_spec.rb index 1d6050fdaa9..823ab5fab90 100644 --- a/spec/models/draft_spec.rb +++ b/spec/models/draft_spec.rb @@ -8,6 +8,10 @@ describe Draft do Fabricate(:user) end + fab!(:post) do + Fabricate(:post) + end + context 'backup_drafts_to_pm_length' do it "correctly backs up drafts to a personal message" do SiteSetting.backup_drafts_to_pm_length = 1 @@ -111,7 +115,6 @@ describe Draft do end it 'can cleanup old drafts' do - user = Fabricate(:user) key = Draft::NEW_TOPIC Draft.set(user, key, 0, 'draft') @@ -164,39 +167,35 @@ describe Draft do it "should include the right draft username in the stream" do Draft.set(user, "topic_#{public_topic.id}", 0, '{"reply":"hey"}') draft_row = stream.first - expect(draft_row.draft_username).to eq(user.username) + expect(draft_row.user.username).to eq(user.username) end end context 'key expiry' do it 'nukes new topic draft after a topic is created' do - u = Fabricate(:user) - Draft.set(u, Draft::NEW_TOPIC, 0, 'my draft') - _t = Fabricate(:topic, user: u) - s = DraftSequence.current(u, Draft::NEW_TOPIC) - expect(Draft.get(u, Draft::NEW_TOPIC, s)).to eq nil + Draft.set(user, Draft::NEW_TOPIC, 0, 'my draft') + _t = Fabricate(:topic, user: user) + s = DraftSequence.current(user, Draft::NEW_TOPIC) + expect(Draft.get(user, Draft::NEW_TOPIC, s)).to eq nil expect(Draft.count).to eq 0 end it 'nukes new pm draft after a pm is created' do - u = Fabricate(:user) - Draft.set(u, Draft::NEW_PRIVATE_MESSAGE, 0, 'my draft') - t = Fabricate(:topic, user: u, archetype: Archetype.private_message, category_id: nil) + Draft.set(user, Draft::NEW_PRIVATE_MESSAGE, 0, 'my draft') + t = Fabricate(:topic, user: user, archetype: Archetype.private_message, category_id: nil) s = DraftSequence.current(t.user, Draft::NEW_PRIVATE_MESSAGE) - expect(Draft.get(u, Draft::NEW_PRIVATE_MESSAGE, s)).to eq nil + expect(Draft.get(user, Draft::NEW_PRIVATE_MESSAGE, s)).to eq nil end it 'does not nuke new topic draft after a pm is created' do - u = Fabricate(:user) - Draft.set(u, Draft::NEW_TOPIC, 0, 'my draft') - t = Fabricate(:topic, user: u, archetype: Archetype.private_message, category_id: nil) + Draft.set(user, Draft::NEW_TOPIC, 0, 'my draft') + t = Fabricate(:topic, user: user, archetype: Archetype.private_message, category_id: nil) s = DraftSequence.current(t.user, Draft::NEW_TOPIC) - expect(Draft.get(u, Draft::NEW_TOPIC, s)).to eq 'my draft' + expect(Draft.get(user, Draft::NEW_TOPIC, s)).to eq 'my draft' end it 'nukes the post draft when a post is created' do - user = Fabricate(:user) topic = Fabricate(:topic) Draft.set(user, topic.draft_key, 0, 'hello') @@ -207,23 +206,20 @@ describe Draft do end it 'nukes the post draft when a post is revised' do - p = Fabricate(:post) - Draft.set(p.user, p.topic.draft_key, 0, 'hello') - p.revise(p.user, raw: 'another test') - s = DraftSequence.current(p.user, p.topic.draft_key) - expect(Draft.get(p.user, p.topic.draft_key, s)).to eq nil + Draft.set(post.user, post.topic.draft_key, 0, 'hello') + post.revise(post.user, raw: 'another test') + s = DraftSequence.current(post.user, post.topic.draft_key) + expect(Draft.get(post.user, post.topic.draft_key, s)).to eq nil end it 'increases revision each time you set' do - u = User.first - Draft.set(u, 'new_topic', 0, 'hello') - Draft.set(u, 'new_topic', 0, 'goodbye') + Draft.set(user, 'new_topic', 0, 'hello') + Draft.set(user, 'new_topic', 0, 'goodbye') - expect(Draft.find_by(user_id: u.id, draft_key: 'new_topic').revisions).to eq(2) + expect(Draft.find_by(user_id: user.id, draft_key: 'new_topic').revisions).to eq(2) end it 'handles owner switching gracefully' do - user = Fabricate(:user) draft_seq = Draft.set(user, 'new_topic', 0, 'hello', _owner = 'ABCDEF') expect(draft_seq).to eq(0) @@ -233,5 +229,20 @@ describe Draft do draft_seq = Draft.set(user, 'new_topic', 1, 'hello world', _owner = 'HIJKL') expect(draft_seq).to eq(1) end + + it 'can correctly preload drafts' do + Draft.set(user, "#{Draft::EXISTING_TOPIC}#{post.topic_id}", 0, { raw: 'hello', postId: post.id }.to_json) + + drafts = Draft.where(user_id: user.id).to_a + + Draft.preload_data(drafts, user) + + expect(drafts[0].topic_preloaded?).to eq(true) + expect(drafts[0].topic.id).to eq(post.topic_id) + + expect(drafts[0].post_preloaded?).to eq(true) + expect(drafts[0].post.id).to eq(post.id) + end + end end diff --git a/spec/requests/drafts_controller_spec.rb b/spec/requests/drafts_controller_spec.rb index 242846d72c8..d67d32828c3 100644 --- a/spec/requests/drafts_controller_spec.rb +++ b/spec/requests/drafts_controller_spec.rb @@ -34,4 +34,24 @@ describe DraftsController do get "/drafts.json", params: { username: user_b.username } expect(response.status).to eq(403) end + + it 'does not include topic details when user cannot see topic' do + topic = Fabricate(:private_message_topic) + topic_user = topic.user + other_user = Fabricate(:user) + Draft.set(topic_user, "topic_#{topic.id}", 0, '{}') + Draft.set(other_user, "topic_#{topic.id}", 0, '{}') + + sign_in(topic_user) + get "/drafts.json", params: { username: topic_user.username } + expect(response.status).to eq(200) + parsed = JSON.parse(response.body) + expect(parsed["drafts"].first["title"]).to eq(topic.title) + + sign_in(other_user) + get "/drafts.json", params: { username: other_user.username } + expect(response.status).to eq(200) + parsed = JSON.parse(response.body) + expect(parsed["drafts"].first["title"]).to eq(nil) + end end